[PATCH] Always define _dl_starting_up.

["Carlos O'Donell" <carlos at redhat dot com>]

On 02/20/2014 04:11 PM, Roland McGrath wrote:
> See elf/dl-writev.h.  sysdeps/unix/sysv/linux/dl-writev.h overrides that.
> At the time of the cited changes, that file didn't exist and the code was
> inlined in _dl_debug_vdprintf (elf/dl-misc.c) where it used _dl_starting_up
> if [!HAVE_INLINED_SYSCALLS && RTLD_PRIVATE_ERRNO].

If I'm reading this correctly the Linux implementation assumes the writev
syscall will serialize access to the fd and avoids taking the dl_load_lock. 

In the case of elf/dl-writev.h(_dl_writev) it makes sense to use
_dl_starting_up because it allows you to skip the locking. I assume
that skipping the locking is a correctness issue I don't yet understand.
I wouldn't compilcate _dl_writev with a premature optimization like this.

However, assuming Linux writev is serializing access is wrong, see [1], 
we could loose data if multiple threads call writev at the same time.
Ignoring that, I do understand what's going on now in this particular 
case.

> Hurd configurations still use elf/dl-writev.h and do not define
> HAVE_INLINED_SYSCALLS, but they also set RTLD_PRIVATE_ERRNO to 0.  So for
> _dl_writev, we could perhaps drop the use of _dl_starting_up and just have
> an #error for the #if RTLD_PRIVATE_ERRNO case.  (Then new configurations
> that set RTLD_PRIVATE_ERRNO to 1 would be obliged to provide their own
> dl-writev.h, as Linux configurations do.)

Given [1] I think that elf/dl-writev.h is actually the only correct
implementation on Linux today, and deleting sysdeps/unix/sysv/linux/dl-writev.h.
is the solution.

> Essentially HAVE_INLINED_SYSCALLS is being used as a proxy for identifying
> Linux configurations.  That makes vague sense for the _dl_writev use
> because _dl_starting_up is used there only in non-Linux configurations.
> Obviously that does not make it a clean way to operate.

That clarifies it for me. The use of HAVE_INLINED_SYSCALLS is indeed a
proxy for identifying if the Linux _dl_writev is being used and then
optimizing away the need for _dl_starting_up.

> The other use of _dl_starting_up is to set __libc_multiple_libcs.  That use
> handles it being an undefined weak symbol and that's the case for Linux
> configurations.  The Fedora patch might affect what value
> __libc_multiple_libcs gets under different conditions.  I'm not at all sure
> off hand.  Though I recall the basic issue that necessitated the
> __libc_multiple_libcs logic, I don't recall the exact details of when or
> whether it should be set (i.e. both the value to give it and the chronology
> in a process when it should get that value).

In the current master my expectation is that __libc_multiple_libcs will
always be zero because we remove the weak symbol _dl_starting_up from ld.so.
Therefore in Linux it seems that we assume libc will never be loaded more
than once which I don't think is true. With dlmopen we could certainly load
more than one libc to inspect it, and doing so today would result in that
libc resetting your FPU and possibly aborting if the kernel is too old
(things we skip doing via __libc_multiple_libcs).

The fedora patch we have is as follows, and which I think is correct given
your explanation and my reading of the code.

OK to checkin? It's been in Fedora for almost 4 years, no regressions.

2010-06-11  Andreas Schwab  <schwab@redhat.com>

	* elf/rtld.c (_dl_starting_up): Always define.
	(dl_main): Always set _dl_starting_up.
	* elf/dl-support.c (_dl_starting_up): Always define.
	* elf/dl-init.c (_dl_init): Always clear _dl_starting_up.

diff --git a/elf/dl-init.c b/elf/dl-init.c
index e7b6757..a108529 100644
--- a/elf/dl-init.c
+++ b/elf/dl-init.c
@@ -24,11 +24,9 @@
 /* Type of the initializer.  */
 typedef void (*init_t) (int, char **, char **);
 
-#ifndef HAVE_INLINED_SYSCALLS
 /* Flag, nonzero during startup phase.  */
 extern int _dl_starting_up;
 extern int _dl_starting_up_internal attribute_hidden;
-#endif
 
 
 static void
@@ -133,9 +131,7 @@ _dl_init (struct link_map *main_map, int argc, char **argv, char **env)
   while (i-- > 0)
     call_init (main_map->l_initfini[i], argc, argv, env);
 
-#ifndef HAVE_INLINED_SYSCALLS
   /* Finished starting up.  */
   INTUSE(_dl_starting_up) = 0;
-#endif
 }
 INTDEF (_dl_init)
diff --git a/elf/dl-support.c b/elf/dl-support.c
index f94d2c4..e575cb4 100644
--- a/elf/dl-support.c
+++ b/elf/dl-support.c
@@ -80,10 +80,8 @@ unsigned long long _dl_load_adds;
    create a fake scope containing nothing.  */
 struct r_scope_elem _dl_initial_searchlist;
 
-#ifndef HAVE_INLINED_SYSCALLS
 /* Nonzero during startup.  */
 int _dl_starting_up = 1;
-#endif
 
 /* Random data provided by the kernel.  */
 void *_dl_random;
diff --git a/elf/rtld.c b/elf/rtld.c
index 90f3ff1..80fe0ab 100644
--- a/elf/rtld.c
+++ b/elf/rtld.c
@@ -106,7 +106,6 @@ static struct audit_list
   struct audit_list *next;
 } *audit_list;
 
-#ifndef HAVE_INLINED_SYSCALLS
 /* Set nonzero during loading and initialization of executable and
    libraries, cleared before the executable's entry point runs.  This
    must not be initialized to nonzero, because the unused dynamic
@@ -116,7 +115,6 @@ static struct audit_list
    never be called.  */
 int _dl_starting_up = 0;
 INTVARDEF(_dl_starting_up)
-#endif
 
 /* This is the structure which defines all variables global to ld.so
    (except those which cannot be added for some reason).  */
@@ -922,10 +920,8 @@ dl_main (const ElfW(Phdr) *phdr,
   /* Process the environment variable which control the behaviour.  */
   process_envvars (&mode);
 
-#ifndef HAVE_INLINED_SYSCALLS
   /* Set up a flag which tells we are just starting.  */
   INTUSE(_dl_starting_up) = 1;
-#endif
 
   if (*user_entry == (ElfW(Addr)) ENTRY_POINT)
     {
---

Cheers,
Carlos.

[1] http://lwn.net/Articles/180390/, and remains true today including writev
    and read.