[RFC PATCH] Fix __libc_start_main prototype in [r]crt1.c to match the caller

[RFC PATCH] Fix __libc_start_main prototype in [r]crt1.c to match the caller

[megous]

From: Ondrej Jirman <megous@megous.com>

__libc_start_main function is not using the last three arguments.

GCC in LTO mode complains about mismatch.
---
 crt/crt1.c  | 6 +++---
 crt/rcrt1.c | 7 ++++---
 2 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/crt/crt1.c b/crt/crt1.c
index 7b12665f..81ecf118 100644
--- a/crt/crt1.c
+++ b/crt/crt1.c
@@ -8,12 +8,12 @@
 int main();
 weak void _init();
 weak void _fini();
-_Noreturn int __libc_start_main(int (*)(), int, char **,
-	void (*)(), void(*)(), void(*)());
+_Noreturn
+int __libc_start_main(int (*main)(int,char **,char **), int argc, char **argv);
 
 void _start_c(long *p)
 {
 	int argc = p[0];
 	char **argv = (void *)(p+1);
-	__libc_start_main(main, argc, argv, _init, _fini, 0);
+	__libc_start_main(main, argc, argv);
 }
diff --git a/crt/rcrt1.c b/crt/rcrt1.c
index 7bb3322f..52928b3e 100644
--- a/crt/rcrt1.c
+++ b/crt/rcrt1.c
@@ -5,10 +5,11 @@
 int main();
 weak void _init();
 weak void _fini();
-_Noreturn int __libc_start_main(int (*)(), int, char **,
-	void (*)(), void(*)(), void(*)());
+
+_Noreturn
+int __libc_start_main(int (*main)(int,char **,char **), int argc, char **argv);
 
 hidden _Noreturn void __dls2(unsigned char *base, size_t *sp)
 {
-	__libc_start_main(main, *sp, (void *)(sp+1), _init, _fini, 0);
+	__libc_start_main(main, *sp, (void *)(sp+1));
 }
-- 
2.19.1

Re: [RFC PATCH] Fix __libc_start_main prototype in [r]crt1.c to match the caller

[Szabolcs Nagy]

* megous@megous.com <megous@megous.com> [2018-11-21 15:51:50 +0100]:
> From: Ondrej Jirman <megous@megous.com>
> 
> __libc_start_main function is not using the last three arguments.
> GCC in LTO mode complains about mismatch.

fix it in the other way then.

> -	__libc_start_main(main, argc, argv, _init, _fini, 0);
> +	__libc_start_main(main, argc, argv);

you just completely broke everything there didnt you?

how will the _init/_fini code of executables with
DT_INIT, DT_FINI dynamic tags run?

i think gcc still havent fixed weak object symbol alias
bugs with lto so e.g. you will get incorrect environ if
you lto link the libc.
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=69271

Re: [RFC PATCH] Fix __libc_start_main prototype in [r]crt1.c to match the caller

[Szabolcs Nagy]

* Szabolcs Nagy <nsz@port70.net> [2018-11-21 16:09:04 +0100]:

> * megous@megous.com <megous@megous.com> [2018-11-21 15:51:50 +0100]:
> > From: Ondrej Jirman <megous@megous.com>
> > 
> > __libc_start_main function is not using the last three arguments.
> > GCC in LTO mode complains about mismatch.
> 
> fix it in the other way then.
> 
> > -	__libc_start_main(main, argc, argv, _init, _fini, 0);
> > +	__libc_start_main(main, argc, argv);
> 
> you just completely broke everything there didnt you?
> 

sorry the _init, _fini there is only needed for glibc compat
(i.e. executable linked with musl crt1.o, but using glibc
to run it, which should not be a common use case)

> how will the _init/_fini code of executables with
> DT_INIT, DT_FINI dynamic tags run?
> 
> i think gcc still havent fixed weak object symbol alias
> bugs with lto so e.g. you will get incorrect environ if
> you lto link the libc.
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=69271

these lto issues still apply.

Re: [RFC PATCH] Fix __libc_start_main prototype in [r]crt1.c to match the caller

[Rich Felker]

On Wed, Nov 21, 2018 at 04:22:17PM +0100, Szabolcs Nagy wrote:
> * Szabolcs Nagy <nsz@port70.net> [2018-11-21 16:09:04 +0100]:
> 
> > * megous@megous.com <megous@megous.com> [2018-11-21 15:51:50 +0100]:
> > > From: Ondrej Jirman <megous@megous.com>
> > > 
> > > __libc_start_main function is not using the last three arguments.
> > > GCC in LTO mode complains about mismatch.
> > 
> > fix it in the other way then.
> > 
> > > -	__libc_start_main(main, argc, argv, _init, _fini, 0);
> > > +	__libc_start_main(main, argc, argv);
> > 
> > you just completely broke everything there didnt you?
> > 
> 
> sorry the _init, _fini there is only needed for glibc compat
> (i.e. executable linked with musl crt1.o, but using glibc
> to run it, which should not be a common use case)

This is not remotely a supported case. However the history here is
more subtle. At one point, analysis was done, and the following
conclusions were drawn:

- For static linking, there's no advantage of having the _init and
  _fini pointers passed from crt to libc since there are no external
  interfaces involved. The symbolic binding from __libc_start_main.c
  will resolve to the same thing the symbolic binding in crt1.o would.

- For dynamic linking, DT_INIT/DT_FINI are available and should be
  preferred over the symbolic binding. Historically the symbolic
  binding of _init and _fini in crt1 rather than from
  __libc_start_main.c prevented these two names from being ABI
  (they're not supposed to be ABI), but using the _DYNAMIC entries is
  a strictly better way to achieve the same.

However, at some point in the distant past, musl did take the _init
and _fini pointers as arguments to __libc_start_main from crt1. By
keeping the code that would pass them, but omitting the code that
would receive and use them, compatibility was preserved in case we
ever wanted to go back to the old way of doing things. If any crt1
ever shipped without passing them, going back would be impossible,
since there would be binaries out in the wild that would be passing
junk in those argument slots.

It seems increasingly implausible that we would ever want to go back
to the old way of doing things, and I'm pretty sure we actually can't,
because I think there were historically some archs where the crt1 used
to be written in asm, and the asm did not pass the _init and _fini
pointers. If this is really the case, we should probably just throw
away the pretense that we still support the possibility of going back.

Aside from that, there is some argument to be made that crt1 should
not be musl-specific and should be written such that it works with any
libc respecting the __libc_start_main entry point ABI. However this is
not currently true -- the crt code loses the register-passed argument
ldso is allowed to pass to request registration of an atexit handler
-- and thus it's not something we really need to consider as a
constraint that must be met.

If for some reason it turns out we don't want to drop the convention
of passing the extra 3 unused arguments, we could just update
__libc_start_main to accept and ignore them. I forget why this was
removed to begin with, but it seems like it would be nice; in general,
having extra unused incoming arguments is desirable in that it
improves the ability to tail-call, though I don't think it helps here
anyway.

> > how will the _init/_fini code of executables with
> > DT_INIT, DT_FINI dynamic tags run?
> > 
> > i think gcc still havent fixed weak object symbol alias
> > bugs with lto so e.g. you will get incorrect environ if
> > you lto link the libc.
> > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=69271
> 
> these lto issues still apply.

This affects building *of* libc.so with LTO, but does not affect
linking *to* libc.a with LTO. The latter should be expected to work,
unless _start_c is still getting wrongly optimized out...

Rich

Re: [RFC PATCH] Fix __libc_start_main prototype in [r]crt1.c to match the caller

[Ondřej Jirman]

On Wed, Nov 21, 2018 at 04:22:17PM +0100, Szabolcs Nagy wrote:
> * Szabolcs Nagy <nsz@port70.net> [2018-11-21 16:09:04 +0100]:
> 
> > * megous@megous.com <megous@megous.com> [2018-11-21 15:51:50 +0100]:
> > > From: Ondrej Jirman <megous@megous.com>
> > > 
> > > __libc_start_main function is not using the last three arguments.
> > > GCC in LTO mode complains about mismatch.
> > 
> > fix it in the other way then.
> > 
> > > -	__libc_start_main(main, argc, argv, _init, _fini, 0);
> > > +	__libc_start_main(main, argc, argv);
> > 
> > you just completely broke everything there didnt you?
> > 
> 
> sorry the _init, _fini there is only needed for glibc compat
> (i.e. executable linked with musl crt1.o, but using glibc
> to run it, which should not be a common use case)

I had a hunch that there was some reason outside of musl for these arguments,
therefore just the RFC. I guess, it's better to ignore the patch then.

I just saw a difference of prototypes, so I sent the patch.

> > how will the _init/_fini code of executables with
> > DT_INIT, DT_FINI dynamic tags run?
> > 
> > i think gcc still havent fixed weak object symbol alias
> > bugs with lto so e.g. you will get incorrect environ if
> > you lto link the libc.
> > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=69271
> 
> these lto issues still apply.

I'm attempting a static build, for which this issue should not apply, AFAIK.

Though, I'm trying on mips, and it leads to GOT related error. So LTO of musl+my
code doesn't work anyway, because ld fails. I'll try with some other target just
to see if I can get something that works, without the limitations of mips arch.

regards,
  o.