Hi, Willy, Thomas
Currently, since this part of the discussion is out of the original topic [1],
as suggested by Willy, open a new thread for this.
[1]: https://lore.kernel.org/lkml/[email protected]/#R
The original topic [1] tries to enable -Wall (or even -Wextra) to report some
issues (include the dead unused functions/data) found during compiling stage,
this further propose a method to enable '-ffunction-sections -fdata-sections
-Wl,--gc-sections,--print-gc-sections to' find the dead unused functions/data
during linking stage:
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.
Let's continue the discussion as below.
> On Mon, Jul 31, 2023 at 08:36:05PM +0200, Thomas Wei?schuh wrote:
>
> [...]
>
> > > > > 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,
Willy, did I misunderstand something again? a simple test shows, seems this is
not really always like that, static mainly means 'local', the symbol is still
there if without -O2/-Os and is able to be set a breakpoint at:
// test.c: gcc -o test test.c
#include <stdio.h>
static int test (void)
{
printf("hello, world!\n");
}
int main(void)
{
test();
return 0;
}
Even with -Os/-O2, an additional '-g' is able to generate the 'test' symbol for
debug as we expect.
> 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).
For the empty __stack_chk_init() one (when the arch not support stackprotector)
we used, when with 'weak', it is not possible drop it during link time even
with -O3, the weak one will be dropped may be only when there is a global one
with the same name is used or the 'weak' one is never really used?
#include <stdio.h>
__attribute__((weak,unused,section(".text.nolibc_memset")))
int test (void)
{
printf("hello, world!\n");
}
int main(void)
{
test();
return 0;
}
0000000000001060 <main>:
1060: f3 0f 1e fa endbr64
1064: 48 83 ec 08 sub $0x8,%rsp
1068: e8 03 01 00 00 callq 1170 <test>
106d: 31 c0 xor %eax,%eax
106f: 48 83 c4 08 add $0x8,%rsp
1073: c3 retq
1074: 66 2e 0f 1f 84 00 00 nopw %cs:0x0(%rax,%rax,1)
107b: 00 00 00
107e: 66 90 xchg %ax,%ax
Seems it is either impossible to add a 'inline' keyword again with the 'weak'
attribute (warned by compiler), so, the _start_c (itself is always called by
_start) will always add an empty call to the weak empty __stack_chk_init(),
-Os/-O2/-O3 don't help. for such an empty function, in my opinion, as the size
we want to care about, the calling place should be simply removed by compiler.
Test also shows, with current __inline__ method, the calling place is removed,
but with c89, the __stack_chk_init() itself will not be droped automatically,
only if not with -std=c89, it will be dropped and not appear in the
--print-gc-sections result.
Even for a supported architecture, the shorter __stack_chk_init() may be better
to inlined to the _start_c()?
So, If my above test is ok, then, we'd better simply convert the whole
__stack_chk_init() to a static one as below (I didn't investigate this deeply
due to the warning about static and weak conflict at the first time):
diff --git a/tools/include/nolibc/crt.h b/tools/include/nolibc/crt.h
index 32e128b0fb62..a5f33fef1672 100644
--- a/tools/include/nolibc/crt.h
+++ b/tools/include/nolibc/crt.h
@@ -10,7 +10,7 @@
char **environ __attribute__((weak));
const unsigned long *_auxv __attribute__((weak));
-void __stack_chk_init(void);
+static void __stack_chk_init(void);
static void exit(int);
void _start_c(long *sp)
diff --git a/tools/include/nolibc/stackprotector.h b/tools/include/nolibc/stackprotector.h
index b620f2b9578d..13f1d0e60387 100644
--- a/tools/include/nolibc/stackprotector.h
+++ b/tools/include/nolibc/stackprotector.h
@@ -37,8 +37,7 @@ void __stack_chk_fail_local(void)
__attribute__((weak,section(".data.nolibc_stack_chk")))
uintptr_t __stack_chk_guard;
-__attribute__((weak,section(".text.nolibc_stack_chk"))) __no_stack_protector
-void __stack_chk_init(void)
+static __no_stack_protector void __stack_chk_init(void)
{
my_syscall3(__NR_getrandom, &__stack_chk_guard, sizeof(__stack_chk_guard), 0);
/* a bit more randomness in case getrandom() fails, ensure the guard is never 0 */
@@ -46,7 +45,7 @@ void __stack_chk_init(void)
__stack_chk_guard ^= (uintptr_t) &__stack_chk_guard;
}
#else /* !defined(_NOLIBC_STACKPROTECTOR) */
-__inline__ void __stack_chk_init(void) {}
+static void __stack_chk_init(void) {}
#endif /* defined(_NOLIBC_STACKPROTECTOR) */
#endif /* _NOLIBC_STACKPROTECTOR_H */
> >
> > 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.
For the 'static' keyword we tested above, '-g' may help the debug requirement,
so, is ok for us to apply 'static' for them safely now?
A further test shows, with 'static' on _start_c() doesn't help the size, for it
is always called from _start(), will never save move instructions, but we need
a more 'used' attribute to silence the error 'nolibc-test.c:(.text+0x38cd):
undefined reference to `_start_c'', so, reserve it as the before simpler 'void
_start_c(void)' may be better?
static __attribute__((used)) void _start_c(long *sp)
Thanks,
Zhangjin
>
> Cheers,
> Willy
Hi Zhangjin,
On Tue, Aug 01, 2023 at 12:59:02PM +0800, Zhangjin Wu wrote:
> > > > 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,
>
> Willy, did I misunderstand something again? a simple test shows, seems this is
> not really always like that, static mainly means 'local', the symbol is still
> there if without -O2/-Os and is able to be set a breakpoint at:
>
> // test.c: gcc -o test test.c
> #include <stdio.h>
>
> static int test (void)
> {
> printf("hello, world!\n");
> }
>
> int main(void)
> {
> test();
>
> return 0;
> }
>
> Even with -Os/-O2, an additional '-g' is able to generate the 'test' symbol for
> debug as we expect.
Please compare this:
$ cat test.c
#include <stdio.h>
static int test (void)
{
printf("hello, world!\n");
}
int main(void)
{
test();
return 0;
}
$ gcc -Os -o test test.c
$ objdump --disassemble=main test
(...)
Disassembly of section .text:
0000000000401040 <main>:
401040: 50 push %rax
401041: bf 04 20 40 00 mov $0x402004,%edi
401046: e8 e5 ff ff ff call 401030 <puts@plt>
40104b: 31 c0 xor %eax,%eax
40104d: 5a pop %rdx
40104e: c3 ret
$ gdb ./test
(...)
Reading symbols from ./test...
(gdb) b test
Function "test" not defined.
(gdb) r
Starting program: /dev/shm/test
hello, world!
[Inferior 1 (process 8780) exited normally]
To this:
$ cat test.c
#include <stdio.h>
/*static*/ int test (void)
{
printf("hello, world!\n");
}
int main(void)
{
test();
return 0;
}
$ gcc -Os -o test test.c
$ objdump --disassemble=main test
(...)
Disassembly of section .text:
0000000000401040 <main>:
401040: 50 push %rax
401041: e8 e0 00 00 00 call 401126 <test>
401046: 31 c0 xor %eax,%eax
401048: 5a pop %rdx
401049: c3 ret
$ gdb ./test
(...)
Reading symbols from ./test...
(gdb) b test
Breakpoint 1 at 0x401126
(gdb) r
Starting program: /dev/shm/test
Breakpoint 1, 0x0000000000401126 in test ()
(gdb)
See the difference ?
> > 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).
>
> For the empty __stack_chk_init() one (when the arch not support stackprotector)
> we used, when with 'weak', it is not possible drop it during link time even
> with -O3, the weak one will be dropped may be only when there is a global one
> with the same name is used or the 'weak' one is never really used?
If that's the case for this one, it's sufficient to enclose it within a pair
of #if defined(__SSP__).
>
> #include <stdio.h>
>
> __attribute__((weak,unused,section(".text.nolibc_memset")))
> int test (void)
> {
> printf("hello, world!\n");
> }
>
> int main(void)
> {
> test();
>
> return 0;
> }
>
>
> 0000000000001060 <main>:
> 1060: f3 0f 1e fa endbr64
> 1064: 48 83 ec 08 sub $0x8,%rsp
> 1068: e8 03 01 00 00 callq 1170 <test>
> 106d: 31 c0 xor %eax,%eax
> 106f: 48 83 c4 08 add $0x8,%rsp
> 1073: c3 retq
> 1074: 66 2e 0f 1f 84 00 00 nopw %cs:0x0(%rax,%rax,1)
> 107b: 00 00 00
> 107e: 66 90 xchg %ax,%ax
>
> Seems it is either impossible to add a 'inline' keyword again with the 'weak'
> attribute (warned by compiler), so, the _start_c (itself is always called by
> _start) will always add an empty call to the weak empty __stack_chk_init(),
> -Os/-O2/-O3 don't help. for such an empty function, in my opinion, as the size
> we want to care about, the calling place should be simply removed by compiler.
Obviously it's impossible because the precise reason why we're using weak
is when we *need* the symbol, i.e. it's called from asm code or another
unit. We only use weak as an alternative to static to share the symbol.
> Test also shows, with current __inline__ method, the calling place is removed,
> but with c89, the __stack_chk_init() itself will not be droped automatically,
> only if not with -std=c89, it will be dropped and not appear in the
> --print-gc-sections result.
>
> Even for a supported architecture, the shorter __stack_chk_init() may be better
> to inlined to the _start_c()?
There is not better or worse, it doesn't work like this. What is important
to keep in mind is:
- if the symbol needs to be exported, it must not be static. If it risks
to be declared multiple times (since appearing in a .h possibly included
multiple times), it needs to be marked weak.
- if the symbol benefits from being reused a lot because it's huge and
almost always needed, it can benefit as well from being exported so
that at the end there is only one instead of multiple copies.
- if the symbol is never needed outside and its duplication is not a
problem, it's better static.
__stack_chk_init() used to be called directly from asm(). We had no other
option. With _start_c() it seems this has changed so maybe it can now be
static (I have not checked). But *these* are the only valid justifications,
nothing based on preference or being better or whatever. These serve two
completely different goals.
> So, If my above test is ok, then, we'd better simply convert the whole
> __stack_chk_init() to a static one as below (I didn't investigate this deeply
> due to the warning about static and weak conflict at the first time):
Static and weak together make absolutely no sense. Static says "do not export
it, it's local" and weak says "when you find multiple copies of it, keep only
one". By definition they are mutually exclusive.
> > > 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.
>
> For the 'static' keyword we tested above, '-g' may help the debug requirement,
> so, is ok for us to apply 'static' for them safely now?
Please, read above and do not speculate but actually try by yourself and
see what doesn't work. I can assure you that it's extremely time consuming
to have to justify everything that was done over the last years to reach
the current situation, so as to make sure we don't go back several years
just due to matters of taste. By definition a libc cannot look nice, and
when implemented exclusively in include files there are strong tradeoffs
that are needed. The vast majority of them are already documented in the
code and others in commit messages. Others will obviously depend on
everyone's experience at recognizing certain patterns. I think I'll spend
some time writing some doc explaining some design rules, some choices that
are imposed to us and certain patterns to adopt or avoid. It will surely
help and save us a lot of review and discussion time in the future (at
least I hope).
> A further test shows, with 'static' on _start_c() doesn't help the size, for it
> is always called from _start(), will never save move instructions, but we need
> a more 'used' attribute to silence the error 'nolibc-test.c:(.text+0x38cd):
> undefined reference to `_start_c'', so, reserve it as the before simpler 'void
> _start_c(void)' may be better?
>
> static __attribute__((used)) void _start_c(long *sp)
Again if you test this you'll see that it probably does not work. It's
called from asm so it needs to be public. And it needs the weak
attribute to avoid clashes.
At least you've convinced me about something: we need to split nolibc-test
into multiple files (e.g. one for syscalls, one for stdlib etc) so that we
also catch the cases where we're missing some weak attributes.
Thanks,
Willy
Hi, Willy, Hi, Thomas
> Hi Zhangjin,
>
> On Tue, Aug 01, 2023 at 12:59:02PM +0800, Zhangjin Wu wrote:
[...]
> >
> > Willy, did I misunderstand something again? a simple test shows, seems this is
> > not really always like that, static mainly means 'local', the symbol is still
> > there if without -O2/-Os and is able to be set a breakpoint at:
> >
[...]
> >
> > Even with -Os/-O2, an additional '-g' is able to generate the 'test' symbol for
> > debug as we expect.
>
> Please compare this:
>
> $ cat test.c
> #include <stdio.h>
>
> static int test (void)
> {
> printf("hello, world!\n");
> }
>
> int main(void)
> {
> test();
>
> return 0;
> }
>
> $ gcc -Os -o test test.c
> $ objdump --disassemble=main test
> (...)
> Disassembly of section .text:
>
> 0000000000401040 <main>:
> 401040: 50 push %rax
> 401041: bf 04 20 40 00 mov $0x402004,%edi
> 401046: e8 e5 ff ff ff call 401030 <puts@plt>
> 40104b: 31 c0 xor %eax,%eax
> 40104d: 5a pop %rdx
> 40104e: c3 ret
>
> $ gdb ./test
> (...)
> Reading symbols from ./test...
> (gdb) b test
> Function "test" not defined.
> (gdb) r
> Starting program: /dev/shm/test
> hello, world!
> [Inferior 1 (process 8780) exited normally]
>
> To this:
>
> $ cat test.c
> #include <stdio.h>
>
> /*static*/ int test (void)
> {
> printf("hello, world!\n");
> }
>
> int main(void)
> {
> test();
>
> return 0;
> }
>
> $ gcc -Os -o test test.c
> $ objdump --disassemble=main test
> (...)
> Disassembly of section .text:
>
> 0000000000401040 <main>:
> 401040: 50 push %rax
> 401041: e8 e0 00 00 00 call 401126 <test>
> 401046: 31 c0 xor %eax,%eax
> 401048: 5a pop %rdx
> 401049: c3 ret
>
> $ gdb ./test
> (...)
> Reading symbols from ./test...
> (gdb) b test
> Breakpoint 1 at 0x401126
> (gdb) r
> Starting program: /dev/shm/test
>
> Breakpoint 1, 0x0000000000401126 in test ()
> (gdb)
>
> See the difference ?
>
Thanks, Willy, I did all tests and mentioned this difference in my
reply, but is not obviously described ;-)
"static mainly means 'local', the symbol is still there **if without
-O2/-Os** and is able to be set a breakpoint at".
> > > 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).
> >
> > For the empty __stack_chk_init() one (when the arch not support stackprotector)
> > we used, when with 'weak', it is not possible drop it during link time even
> > with -O3, the weak one will be dropped may be only when there is a global one
> > with the same name is used or the 'weak' one is never really used?
>
> If that's the case for this one, it's sufficient to enclose it within a pair
> of #if defined(__SSP__).
>
Yeah, it is a solution, but to be honest, let's think about the '#ifdef'
ones we have used in in _start asm(), it is better to not add it back
again ;-)
[...]
>
> There is not better or worse, it doesn't work like this. What is important
> to keep in mind is:
> - if the symbol needs to be exported, it must not be static. If it risks
> to be declared multiple times (since appearing in a .h possibly included
> multiple times), it needs to be marked weak.
>
> - if the symbol benefits from being reused a lot because it's huge and
> almost always needed, it can benefit as well from being exported so
> that at the end there is only one instead of multiple copies.
>
> - if the symbol is never needed outside and its duplication is not a
> problem, it's better static.
>
> __stack_chk_init() used to be called directly from asm(). We had no other
> option.
Thanks, it explains clearly about why we use 'weak' for _start asm() before.
> With _start_c() it seems this has changed so maybe it can now be
> static (I have not checked).
In my test environment (gcc 9.3 + ld 2.34 + x86_64), as I just tested
about _start_c(), both
__attribute__((weak))
and
static __attribute__((used))
provide the _start_c symbol as required by the _start asm() and it
compiled without any failure.
> But *these* are the only valid justifications,
> nothing based on preference or being better or whatever. These serve two
> completely different goals.
>
ok, so, the only benefit may be the saving of some bits of the function
calling instructions.
> > So, If my above test is ok, then, we'd better simply convert the whole
> > __stack_chk_init() to a static one as below (I didn't investigate this deeply
> > due to the warning about static and weak conflict at the first time):
>
> Static and weak together make absolutely no sense. Static says "do not export
> it, it's local" and weak says "when you find multiple copies of it, keep only
> one". By definition they are mutually exclusive.
>
Yes, thanks to clarify it, I knew 'weak' is widely used in kernel side to
override the generic weak functions with architecture specific optimized ones.
I have used it frequently especially when I was developing the mainline MIPS
ftrace support, in my memory, the sched_clock() is a very good example.
> > > > 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.
> >
> > For the 'static' keyword we tested above, '-g' may help the debug requirement,
> > so, is ok for us to apply 'static' for them safely now?
>
> Please, read above and do not speculate but actually try by yourself and
> see what doesn't work.
Willy, as I explained above, before sending my last reply, I did all
tests (by default, tests often before any of my replies) but the issue
is what you expected is the one I just not wrote obviously in my reply,
it worths an improvement to avoid info gap ;-)
> I can assure you that it's extremely time consuming
> to have to justify everything that was done over the last years to reach
> the current situation, so as to make sure we don't go back several years
> just due to matters of taste. By definition a libc cannot look nice, and
> when implemented exclusively in include files there are strong tradeoffs
> that are needed. The vast majority of them are already documented in the
> code and others in commit messages. Others will obviously depend on
> everyone's experience at recognizing certain patterns. I think I'll spend
> some time writing some doc explaining some design rules, some choices that
> are imposed to us and certain patterns to adopt or avoid. It will surely
> help and save us a lot of review and discussion time in the future (at
> least I hope).
>
Agree, but I don't think it is a big program, the learning of the commit
message, comments is a basic step before writing something new.
Friendly to mention, something important I have found during the last
several discussions among us is, although all of us are friendly and
kindly enough, but due to some info gap or some info lost, we all may
persist a direction which may have some traps and sometimes very easy to
out-of-topic or out of the issues we are really solving, so, I was even
worried about to send a new patch or start a new reply these days, one
possible reason may be overloaded ;-)
> > A further test shows, with 'static' on _start_c() doesn't help the size, for it
> > is always called from _start(), will never save move instructions, but we need
> > a more 'used' attribute to silence the error 'nolibc-test.c:(.text+0x38cd):
> > undefined reference to `_start_c'', so, reserve it as the before simpler 'void
> > _start_c(void)' may be better?
> >
> > static __attribute__((used)) void _start_c(long *sp)
>
> Again if you test this you'll see that it probably does not work. It's
> called from asm so it needs to be public. And it needs the weak
> attribute to avoid clashes.
Willy, I did test it before sending the last reply, and luckily, it
worked at least with my compiler on x86_64 although we don't really want
it for _start_c, let's reserve global as-is.
I'm not mean we should use 'used' instead of 'weak' we have used in
nolibc, just show my test result here as a reference.
Seems there is also a new 'retain', but it servces something different,
it tells 'ld --gc-sections' not drop it, but 'used' is for compiler not
discard it, this page does explain more: https://reviews.llvm.org/D96838
Yuan and me are exploring the compiler and linker options/attributes to
let our dead syscall patchset being able to drop every syscall (some of
them are wrongly kept by KEEP), so we found some docs about 'used',
'retain' and the other more related attributes in the past weeks.
>
> At least you've convinced me about something: we need to split nolibc-test
> into multiple files (e.g. one for syscalls, one for stdlib etc) so that we
> also catch the cases where we're missing some weak attributes.
Ok, thanks.
As a summary, is it ok for Thomas to add a change like this to his patchset (or
a standalone patchset from me)?
diff --git a/tools/include/nolibc/crt.h b/tools/include/nolibc/crt.h
index 32e128b0fb62..a5f33fef1672 100644
--- a/tools/include/nolibc/crt.h
+++ b/tools/include/nolibc/crt.h
@@ -10,7 +10,7 @@
char **environ __attribute__((weak));
const unsigned long *_auxv __attribute__((weak));
-void __stack_chk_init(void);
+static void __stack_chk_init(void);
static void exit(int);
void _start_c(long *sp)
diff --git a/tools/include/nolibc/stackprotector.h b/tools/include/nolibc/stackprotector.h
index b620f2b9578d..13f1d0e60387 100644
--- a/tools/include/nolibc/stackprotector.h
+++ b/tools/include/nolibc/stackprotector.h
@@ -37,8 +37,7 @@ void __stack_chk_fail_local(void)
__attribute__((weak,section(".data.nolibc_stack_chk")))
uintptr_t __stack_chk_guard;
-__attribute__((weak,section(".text.nolibc_stack_chk"))) __no_stack_protector
-void __stack_chk_init(void)
+static __no_stack_protector void __stack_chk_init(void)
{
my_syscall3(__NR_getrandom, &__stack_chk_guard, sizeof(__stack_chk_guard), 0);
/* a bit more randomness in case getrandom() fails, ensure the guard is never 0 */
@@ -46,7 +45,7 @@ void __stack_chk_init(void)
__stack_chk_guard ^= (uintptr_t) &__stack_chk_guard;
}
#else /* !defined(_NOLIBC_STACKPROTECTOR) */
-__inline__ void __stack_chk_init(void) {}
+static void __stack_chk_init(void) {}
#endif /* defined(_NOLIBC_STACKPROTECTOR) */
#endif /* _NOLIBC_STACKPROTECTOR_H */
Best regards,
Zhangjin
>
> Thanks,
> Willy