On Mon, Jul 31, 2023 at 08:36:05PM +0200, Thomas Weißschuh wrote:
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?
Actually we don't know if they're used or not given that the purpose of the nolibc-test.c file is for it to be easy to add new tests, and the collection of macros above serves this purpose. It's not just a series of test but rather a small test framework. So the fact that right now no single test uses some of them doesn't mean that someone else will not have to reimplement them in two months.
Reimplementing them would mean to copy one of the sibling test macros and changing the name and the condition operator in one place.
Yes but that's the difference between us providing a basis for others to easily contribute tests and just saying "you can implement you test in this directory". Literally adding just one line is simple and encouraging enough.
I regarded that as an acceptable effort instead of having to work around the warnings.
Warnings must always be addressed, and there are tools for this. One of them is the inline keyword which makes them go away. It's fine as long as we expect that functions are worth inlining (size, debuggability). A second one is the "unused" attribute. I know you said you don't find it clean but it's the official clean way to shut some specific warnings, by passing meta-information to the compiler about the intent for certain things. We can very well have a define saying that __maybe_unused maps to __attribute__((unused)) as done everywhere else, but it's in the end it remains the regular way to do it. Finally the third method consists in removing "static" so that the compiler doesn't know if we're going to use them elsewhere. My personal preference goes with the unused attribute because it's well aligned with the spirit of a test framework providing tools to those who need them.
The warnings themselves I see as useful as they can give developers early feedback on their code. They would have avoided some of the issues with the recent pipe() series.
I totally agree with warnings. I compile my code with -W -Wall -Wextra for this exact reason. Also inside a lib test we must try to trigger more of them so as to be in the worst user situation, because if users detect them first, that's painful.
Do you have a preferred solution for the overall situation?
I'd rather put unused everywhere (possibly with a define to make it more readable). And if the code is too large and too ugly (too many utility functions) really moving it into a .h would significantly help I think.
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.
Most of them could theoretically be turned to static. *But* it causes a problem which is that it will multiply their occurrences in multi-unit programs, and that's in part why we've started to use weak instead. Also if you run through gdb and want to mark a break point, you won't have the symbol when it's static, and the code will appear at multiple locations, which is really painful. I'd instead really prefer to avoid static when we don't strictly want to inline the code, and prefer weak when possible because we know many of them will be dropped at link time (and that's the exact purpose).
Thanks for the clarification. I forgot about that completely!
The stuff from nolibc-test.c itself (run_foo() and is_settings_valid()) should still be done.
Yes, likely. Nolibc-test should be done just like users expect to use nolibc, and nolibc should be the most flexible possible.
Cheers, Willy