On 2023-07-31 19:02:26+0800, Zhangjin Wu wrote:
> Hi, Willy
Thomas here :-)
> > > > > Why not a simple 'static __attribute__((unused))' line, then, no need to
> > > > > add them again next time.
> > > > >
> > > > > -static int expect_zr(int expr, int llen)
> > > > > +static __attribute__((unused))
> > > > > +int expect_zr(int expr, int llen)
> > > > > {
> > > >
> > > > Personally I don't like having dead code lying around that needs to be
> > > > maintained and skipped over while reading.
> > > > It's not a given that we will need those helpers in the future at all.
> > > >
> > >
> > > It is reasonable in some degree from current status, especially for
> > > these ones are newly added, but let us think about these scenes: when we
> > > would drop or change some test cases in the future and the helpers may
> > > would be not referenced by any test cases in a short time, and warnings
> > > there, but some other cases may be added later to use them again ...
> >
> > That doesn't seem very likely.
> > Did it happen recently?
> >
>
> Yeah, it did happen, but I can not remember which one, a simple statistic
> does show it may be likely:
I can't find it.
> $ grep EXPECT_ -ur tools/testing/selftests/nolibc/nolibc-test.c | grep -v define | sed -e 's/.*\(EXPECT_[A-Z0-9]*\).*/\1/g' | sort | uniq -c | sort -k 1 -g -r
> 55 EXPECT_EQ
> 37 EXPECT_SYSER
> 21 EXPECT_SYSZR
> 11 EXPECT_SYSNE
> 9 EXPECT_VFPRINTF
> 4 EXPECT_PTRGT
> 4 EXPECT_GE
> 3 EXPECT_STRZR
> 3 EXPECT_NE
> 3 EXPECT_LT
> 3 EXPECT_GT
> 2 EXPECT_STRNZ
> 2 EXPECT_STREQ
> 2 EXPECT_PTRLT
> 1 EXPECT_SYSER2
> 1 EXPECT_SYSEQ
> 1 EXPECT_PTRNZ
> 1 EXPECT_PTRNE
> 1 EXPECT_PTRER2
> 1 EXPECT_PTRER
> 1 EXPECT_PTREQ
>
> 7 helpers are only used by once, another 3 helpers are used twice, and
> another 4 are only used by three times.
Why can't we just drop them when they are not used anymore?
> > > I'm ok to drop these ones, but another patch may be required to add
> > > 'static __attribute__((unused))' for all of the currently used ones,
> > > otherwise, there will be warnings randomly by a test case change or
> > > drop.
> >
> > Then we just drop the helper when we don't need it anymore.
> >
> > I also dislike the __attribute__ spam to be honest.
> >
>
> Me too, but it does help sometimes ;-)
>
> > > Or even further, is it possible to merge some of them to some more
> > > generic helpers like the ones used from the selftest.h in your last RFC
> > > patchset?
> >
> > Something like this will indeed be part of the KTAP rework.
> > But it's a change for another time.
>
> Yes, this may be a better solution to such warnings.
>
> Btw, just thought about gc-section, do we need to further remove dead code/data
> in the binary? I don't think it is necessary for nolibc-test itself, but with
> '-Wl,--gc-sections -Wl,--print-gc-sections' may be a good helper to show us
> which ones should be dropped or which ones are wrongly declared as public?
>
> Just found '-O3 + -Wl,--gc-section + -Wl,--print-gc-sections' did tell
> us something as below:
>
> removing unused section '.text.nolibc_raise'
> removing unused section '.text.nolibc_memmove'
> removing unused section '.text.nolibc_abort'
> removing unused section '.text.nolibc_memcpy'
> removing unused section '.text.__stack_chk_init'
> removing unused section '.text.is_setting_valid'
>
> These info may help us further add missing 'static' keyword or find
> another method to to drop the wrongly used status of some functions from
> the code side.
>
> It is very easy to add the missing 'static' keyword for is_setting_valid(), but
> for __stack_chk_init(), is it ok for us to convert it to 'static' and remove
> the 'weak' attrbute and even the 'section' attribute? seems it is only used by
> our _start_c() currently.
Making is_setting_valid(), __stack_chk_init() seems indeed useful.
Also all the run_foo() test functions.
> For the left ones, some are related to libgcc for divide by zero or the other
> divide functions, which may be not possible to drop in code side, but for
> memmove/memset, it is able to add -ffreestanding in our nolibc-test like -Wall
> and only wrap the 'weak' attribute with '#if __STDC_HOSTED__ == 1', for the ARM
> specific one, '#ifdef __ARM_EABI__'.
That seems very excessive.
> And even further, the '_start_c()' should be 'static' too, perhaps the above
> issues are worth a new patchset, If you agree, will send a new patchset to fix
> up them.
_start_c(), too.
