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

[Simon Kuenzer <simon.kuenzer@xxxxxxxxx>]

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.
    
    >
    >     +                     --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.
    

Thanks,

Simon

    -- 
    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