Re: [Minios-devel] [UNIKRAFT PATCH v4 2/3] build: always produce 2 images: w/ and w/o debug syms

[Yuri Volchkov <yuri.volchkov@xxxxxxxxx>]

Simon Kuenzer <simon.kuenzer@xxxxxxxxx> writes:

> On 29.05.19, 16:23, "Yuri Volchkov" <yuri.volchkov@xxxxxxxxx> wrote:
>
>     Hey,
>     
>     Simon Kuenzer <simon.kuenzer@xxxxxxxxx> writes:
>     
>     > Hey Yuri,
>     >
>     > thanks for this patch. It looks okay, I have just some minor comments 
> for it. I originally would have preferred a non-python solution for your 
> `sect-strip` wrapper in order to avoid a build dependency to python. However, 
> we may anyway want to update kconfig to the latest version which afaik 
> provides python support.
>     >
>     > Thanks,
>     >
>     > Simon
>     >
>     > On 24.05.19, 12:56, "Minios-devel on behalf of Yuri Volchkov" 
> <minios-devel-bounces@xxxxxxxxxxxxxxxxxxxx on behalf of 
> yuri.volchkov@xxxxxxxxx> wrote:
>     >
>     >      
>     >     -config DEBUG_SYMBOLS
>     >     -   bool "Debugging information"
>     >     -   default n
>     >     -   help
>     >     -           Build with debugging information.
>     >     -
>     >     -if DEBUG_SYMBOLS
>     >
>     > Since you are removing this option, could you add "No Debug (-g0) " to 
> the option list?
>     > For now, it is more for completeness.
>     That defeats the purpose of this patch. The idea is to have debug
>     symbols always, if you do not need them just use a stripped image. Which
>     is effectively the same as it was compiled with -g0.
>
> Okay, fine. I was thinking that there may be a reasonable case where someone 
> wants to have a dbg file without the GCC symbols (e.g., with just having 
> extra debug information produced by a library, like tracing). In such a case 
> we still can add it later. So, don't add it.
>     
>     >     +                $@.o -o $@.dbg)
>     >     +   $(call verbose_cmd,sect-strip,$(notdir $@),\
>     >
>     > I would prefer a different verbose string than 'sect-strip'. It is 
> unfortunately longer than 7 characters, and we usually used upper-case. How 
> about 'SSTRIP', 'SESTRIP', or 'SCSTRIP'?
>     Agreed, I will go with SCSTRIP
>     
>     >
>     >     +           $(SCRIPTS_DIR)/sect-strip.py $(STRIP_SECTIONS_FLAGS-y) \
>     >
>     > You named the program `sect-strip.py` but the flags variable is called 
> `STRIP_SECTIONS_FLAGS`. Why not calling it SECT_STRIP_FLAGS for consistency?
>     OK
>     
>     > Usually we also included two variants to the cmdline for convenience 
> (one with '-y' and the other one without): $(SECT_STRIP_FLAGS) 
> $(SECT_STRIP_FLAGS-y)
>     Sorry, I do not see convenience here :). But it hurts readability. Could
>     you elaborate why non-y version is useful?
>
> So far we have that for all variants of variables in the build system. I 
> think it is better to follow that example for consistency reasons. If you 
> don't like it then provide another patch that removes all non-y option. 
> Applying a new convention just here would end-up in a half-baked state.
Ok, will add non-y option too here. I thought just start doing it
'better' way. Because as it often happens, it's not very feasible to
refactor all the code right away.

>     
>     >
>     >     +                   --with-objcopy=$(OBJCOPY) \
>     >     +                   $@.dbg -o $@)
>     >     +   $(call verbose_cmd,STRIP,$(notdir $@), $(STRIP) $@)
>     >     +
>     >      ifeq ($(CONFIG_OPTIMIZE_SYMFILE),y)
>     >         $(call build_cmd,NM,,$@.sym,\
>     >                $(NM) -n $@ > $@.sym)
>     >      endif
>     >     -# TODO: We have to revisit stripping of KVM binaries. We noticed 
> that sometimes
>     >     -#       the images are broken and cannot be boot with QEMU's 
> direct kernel boot
>     >     -#       option (fread() error is returned).
>     >     -#
>     >
>     > Does this problem not exist anymore? I am asking because we are always 
> stripping the image with your patch.
>     I did not see any problem. That's right we always stripping with my
>     patch. As well as we always keeping a non-stripped version. What I am
>     trying to say, if the problem strikes back, one would effectively have
>     the same workaround - just go with a non-stripped version. So he/she
>     could keep moving after reporting the issue.
>     
>     Even if the problem is still here, how would you fix it if you never
>     experience it.
>
> I was actually wondering if we should keep the TODO comment in. However, for 
> debugging we could still `bisect` and you would obviously end up in this 
> commit... hum, so I guess it is fine then if you remove the comment, too. 
> Let's cross fingers that the original problem was caused by something else 
> and does not occur again.
Well, if it comes again, that will be a perfect opportunity to fix it
finally.

>     
>
> Thanks,
>
> Simon
>
>     -- 
>     Yuri Volchkov
>     Software Specialist
>     
>     NEC Europe Ltd
>     Kurfürsten-Anlage 36
>     D-69115 Heidelberg
>     
>

-- 
Yuri Volchkov
Software Specialist

NEC Europe Ltd
Kurfürsten-Anlage 36
D-69115 Heidelberg

_______________________________________________
Minios-devel mailing list
Minios-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/minios-devel