[PATCH 1/2] Replace __libc_multiple_libcs with tri-state __libc_type

[PATCH 1/2] Replace __libc_multiple_libcs with tri-state __libc_type

[Florian Weimer]

The three states are (1) initial libc without any other libcs loaded
(yet), (2) initial with other libcs loaded, and (3) secondary libc
(audit module, inner libc after dlmopen, inner libc as the result of
static dlopen).

The three states are required because we want to use sbrk for the
malloc in the primary libc, whether or not secondary libcs have been
loaded.  ptmalloc_init currently uses _dl_addr to detect inner
namespaces because __libc_multiple_libcs is not set up correctly for
audit modules (because _dl_starting_up is not used on Linux).
---
 csu/init-first.c                    |  8 +-------
 csu/libc-start.c                    |  4 +---
 dlfcn/dlmopen.c                     |  6 ++++++
 elf/Versions                        |  3 +++
 elf/dl-open.c                       |  2 +-
 elf/dl-sysdep.c                     |  2 --
 elf/libc_early_init.c               | 22 ++++++++++++++++++++++
 include/libc-internal.h             | 18 +++++++++++++++++-
 misc/sbrk.c                         | 11 +++++++++--
 sysdeps/mach/hurd/dl-sysdep.c       |  2 --
 sysdeps/mach/hurd/i386/init-first.c |  5 +----
 11 files changed, 61 insertions(+), 22 deletions(-)

diff --git a/csu/init-first.c b/csu/init-first.c
index 47aaacdbd0..28fa66ad54 100644
--- a/csu/init-first.c
+++ b/csu/init-first.c
@@ -28,10 +28,6 @@
 
 #include <ldsodefs.h>
 
-/* Set nonzero if we have to be prepared for more than one libc being
-   used in the process.  Safe assumption if initializer never runs.  */
-int __libc_multiple_libcs attribute_hidden = 1;
-
 /* Remember the command line argument and enviroment contents for
    later calls of initializers for dynamic libraries.  */
 int __libc_argc attribute_hidden;
@@ -50,10 +46,8 @@ _init_first (int argc, char **argv, char **envp)
 {
 #endif
 
-  __libc_multiple_libcs = &_dl_starting_up && !_dl_starting_up;
-
   /* Make sure we don't initialize twice.  */
-  if (!__libc_multiple_libcs)
+  if (__libc_type != libc_type_secondary)
     {
       /* Set the FPU control word to the proper default value if the
 	 kernel would use a different value.  */
diff --git a/csu/libc-start.c b/csu/libc-start.c
index 2d4d2ed1f9..7e0c4ce610 100644
--- a/csu/libc-start.c
+++ b/csu/libc-start.c
@@ -141,8 +141,6 @@ LIBC_START_MAIN (int (*main) (int, char **, char ** MAIN_AUXVEC_DECL),
   /* Result of the 'main' function.  */
   int result;
 
-  __libc_multiple_libcs = &_dl_starting_up && !_dl_starting_up;
-
 #ifndef SHARED
   _dl_relocate_static_pie ();
 
@@ -213,7 +211,7 @@ LIBC_START_MAIN (int (*main) (int, char **, char ** MAIN_AUXVEC_DECL),
 # endif
 
 # ifdef DL_SYSDEP_OSCHECK
-  if (!__libc_multiple_libcs)
+  if (__libc_type != libc_type_secondary)
     {
       /* This needs to run to initiliaze _dl_osversion before TLS
 	 setup might check it.  */
diff --git a/dlfcn/dlmopen.c b/dlfcn/dlmopen.c
index 1396c818ac..c880c13443 100644
--- a/dlfcn/dlmopen.c
+++ b/dlfcn/dlmopen.c
@@ -22,6 +22,7 @@
 #include <stddef.h>
 #include <unistd.h>
 #include <ldsodefs.h>
+#include <libc-internal.h>
 
 #if !defined SHARED && IS_IN (libdl)
 
@@ -79,6 +80,11 @@ void *
 __dlmopen (Lmid_t nsid, const char *file, int mode DL_CALLER_DECL)
 {
 # ifdef SHARED
+  /* Advertise that we may have multiple libc from now on.  Do not
+     overwrite a libc_type_secondary value.  */
+  if (__libc_type == libc_type_initial_only)
+    __libc_type = libc_type_initial;
+
   if (!rtld_active ())
     return _dlfcn_hook->dlmopen (nsid, file, mode, RETURN_ADDRESS (0));
 # endif
diff --git a/elf/Versions b/elf/Versions
index be88c48e6d..39df728f0a 100644
--- a/elf/Versions
+++ b/elf/Versions
@@ -28,6 +28,9 @@ libc {
     __libc_dlclose; __libc_dlopen_mode; __libc_dlsym; __libc_dlvsym;
     __libc_early_init;
 
+    # Updated from libdl.
+    __libc_type;
+
     # Internal error handling support.  Interposes the functions in ld.so.
     _dl_signal_exception; _dl_catch_exception;
     _dl_signal_error; _dl_catch_error;
diff --git a/elf/dl-open.c b/elf/dl-open.c
index 8769e47051..7b7a9214b8 100644
--- a/elf/dl-open.c
+++ b/elf/dl-open.c
@@ -790,7 +790,7 @@ dl_open_worker (void *a)
 #ifndef SHARED
   /* We must be the static _dl_open in libc.a.  A static program that
      has loaded a dynamic object now has competition.  */
-  __libc_multiple_libcs = 1;
+  __libc_type = libc_type_initial;
 #endif
 
   /* Let the user know about the opencount.  */
diff --git a/elf/dl-sysdep.c b/elf/dl-sysdep.c
index 854570821c..6cc4a76560 100644
--- a/elf/dl-sysdep.c
+++ b/elf/dl-sysdep.c
@@ -58,8 +58,6 @@ ElfW(Addr) _dl_base_addr;
 #endif
 int __libc_enable_secure attribute_relro = 0;
 rtld_hidden_data_def (__libc_enable_secure)
-int __libc_multiple_libcs = 0;	/* Defining this here avoids the inclusion
-				   of init-first.  */
 /* This variable contains the lowest stack address ever used.  */
 void *__libc_stack_end attribute_relro = NULL;
 rtld_hidden_data_def(__libc_stack_end)
diff --git a/elf/libc_early_init.c b/elf/libc_early_init.c
index 725ab2f811..fec5d6035e 100644
--- a/elf/libc_early_init.c
+++ b/elf/libc_early_init.c
@@ -17,9 +17,14 @@
    <https://www.gnu.org/licenses/>.  */
 
 #include <ctype.h>
+#include <ldsodefs.h>
 #include <libc-early-init.h>
+#include <libc-internal.h>
 #include <sys/single_threaded.h>
 
+char __libc_type __attribute__ ((nocommon));
+libc_hidden_def (__libc_type)
+
 void
 __libc_early_init (_Bool initial)
 {
@@ -28,4 +33,21 @@ __libc_early_init (_Bool initial)
 
   /* Only the outer namespace is marked as single-threaded.  */
   __libc_single_threaded = initial;
+
+#ifdef SHARED
+  if (initial)
+    {
+      if (GLRO (dl_naudit) == 0)
+        __libc_type = libc_type_initial_only;
+      else
+        /* If auditing is active, the initial libc is not the only
+           libc.  */
+        __libc_type = libc_type_initial;
+    }
+  else
+    __libc_type = libc_type_secondary;
+#else
+  /* There is no auditing support for statically linked binaries.  */
+  __libc_type = libc_type_initial_only;
+#endif
 }
diff --git a/include/libc-internal.h b/include/libc-internal.h
index 915613c030..7331ff2c51 100644
--- a/include/libc-internal.h
+++ b/include/libc-internal.h
@@ -47,6 +47,22 @@ extern void __init_misc (int, char **, char **) attribute_hidden;
 extern __typeof (__profile_frequency) __profile_frequency attribute_hidden;
 # endif
 
-extern int __libc_multiple_libcs attribute_hidden;
+enum
+  {
+    /* The running libc is the only libc in the process image.  It can
+       transition to libc_type_initial via dlmopen or static
+       dlopen.  */
+    libc_type_initial_only,
+
+    /* The running libc is the libc for the main program, but other
+       libcs may have been loaded.  */
+    libc_type_initial,
+
+    /* The running libc is a secondary libc (loaded via dlmopen, to
+       support auditing, or for static dlopen).  */
+    libc_type_secondary,
+  };
+extern char __libc_type;
+libc_hidden_proto (__libc_type)
 
 #endif /* _LIBC_INTERNAL  */
diff --git a/misc/sbrk.c b/misc/sbrk.c
index ba3322fba6..88840b426b 100644
--- a/misc/sbrk.c
+++ b/misc/sbrk.c
@@ -16,9 +16,10 @@
    <https://www.gnu.org/licenses/>.  */
 
 #include <errno.h>
+#include <libc-internal.h>
+#include <stdbool.h>
 #include <stdint.h>
 #include <unistd.h>
-#include <libc-internal.h>
 
 /* Defined in brk.c.  */
 extern void *__curbrk;
@@ -37,7 +38,13 @@ __sbrk (intptr_t increment)
      __curbrk from the kernel's brk value.  That way two separate
      instances of __brk and __sbrk can share the heap, returning
      interleaved pieces of it.  */
-  if (__curbrk == NULL || __libc_multiple_libcs)
+#if IS_IN (rtld)
+  bool update_curbrk = __curbrk == NULL;
+#else
+  bool update_curbrk = (__curbrk == NULL
+			|| __libc_type != libc_type_initial_only);
+#endif
+  if (update_curbrk)
     if (__brk (0) < 0)		/* Initialize the break.  */
       return (void *) -1;
 
diff --git a/sysdeps/mach/hurd/dl-sysdep.c b/sysdeps/mach/hurd/dl-sysdep.c
index 370495710e..a5169d85e7 100644
--- a/sysdeps/mach/hurd/dl-sysdep.c
+++ b/sysdeps/mach/hurd/dl-sysdep.c
@@ -57,8 +57,6 @@ extern char **_environ;
 
 int __libc_enable_secure = 0;
 rtld_hidden_data_def (__libc_enable_secure)
-int __libc_multiple_libcs = 0;	/* Defining this here avoids the inclusion
-				   of init-first.  */
 /* This variable contains the lowest stack address ever used.  */
 void *__libc_stack_end = NULL;
 rtld_hidden_data_def(__libc_stack_end)
diff --git a/sysdeps/mach/hurd/i386/init-first.c b/sysdeps/mach/hurd/i386/init-first.c
index 1827479f86..8497089f7a 100644
--- a/sysdeps/mach/hurd/i386/init-first.c
+++ b/sysdeps/mach/hurd/i386/init-first.c
@@ -40,7 +40,6 @@ unsigned long int __hurd_threadvar_stack_mask;
 #ifndef SHARED
 int __libc_enable_secure;
 #endif
-int __libc_multiple_libcs attribute_hidden = 1;
 
 extern int __libc_argc attribute_hidden;
 extern char **__libc_argv attribute_hidden;
@@ -56,13 +55,11 @@ DEFINE_HOOK (_hurd_preinit_hook, (void));
 static void
 posixland_init (int argc, char **argv, char **envp)
 {
-  __libc_multiple_libcs = &_dl_starting_up && !_dl_starting_up;
-
   /* Now we have relocations etc. we can start signals etc.  */
   _hurd_libc_proc_init (argv);
 
   /* Make sure we don't initialize twice.  */
-  if (!__libc_multiple_libcs)
+  if (__libc_type != libc_type_secondary)
     {
       /* Set the FPU control word to the proper default value.  */
       __setfpucw (__fpu_control);
-- 
Red Hat GmbH, https://de.redhat.com/ , Registered seat: Grasbrunn,
Commercial register: Amtsgericht Muenchen, HRB 153243,
Managing Directors: Charles Cachera, Brian Klemm, Laurie Krebs, Michael O'Neill

[PATCH 2/2] malloc: Use __libc_type to detect an inner libc

[Florian Weimer]

The inner libc must not use sbrk.  _dl_addr occasionally shows up
in profiles, but had to be used before because __libc_multiple_libs
was unreliable.
---
 malloc/arena.c  | 7 +------
 malloc/malloc.c | 2 ++
 2 files changed, 3 insertions(+), 6 deletions(-)

diff --git a/malloc/arena.c b/malloc/arena.c
index 202daf15b0..865eaa1a84 100644
--- a/malloc/arena.c
+++ b/malloc/arena.c
@@ -296,12 +296,7 @@ ptmalloc_init (void)
 #ifdef SHARED
   /* In case this libc copy is in a non-default namespace, never use brk.
      Likewise if dlopened from statically linked program.  */
-  Dl_info di;
-  struct link_map *l;
-
-  if (_dl_open_hook != NULL
-      || (_dl_addr (ptmalloc_init, &di, &l, NULL) != 0
-          && l->l_ns != LM_ID_BASE))
+  if (__libc_type == libc_type_secondary)
     __morecore = __failing_morecore;
 #endif
 
diff --git a/malloc/malloc.c b/malloc/malloc.c
index 5b87bdb081..0834e4181a 100644
--- a/malloc/malloc.c
+++ b/malloc/malloc.c
@@ -247,6 +247,8 @@
 /* For SINGLE_THREAD_P.  */
 #include <sysdep-cancel.h>
 
+#include <libc-internal.h>
+
 /*
   Debugging:
 
-- 
Red Hat GmbH, https://de.redhat.com/ , Registered seat: Grasbrunn,
Commercial register: Amtsgericht Muenchen, HRB 153243,
Managing Directors: Charles Cachera, Brian Klemm, Laurie Krebs, Michael O'Neill

Re: [PATCH 1/2] Replace __libc_multiple_libcs with tri-state __libc_type

[Adhemerval Zanella]

On 13/11/2020 13:14, Florian Weimer via Libc-alpha wrote:
> The three states are (1) initial libc without any other libcs loaded
> (yet), (2) initial with other libcs loaded, and (3) secondary libc
> (audit module, inner libc after dlmopen, inner libc as the result of
> static dlopen).
> 
> The three states are required because we want to use sbrk for the
> malloc in the primary libc, whether or not secondary libcs have been
> loaded.  ptmalloc_init currently uses _dl_addr to detect inner
> namespaces because __libc_multiple_libcs is not set up correctly for
> audit modules (because _dl_starting_up is not used on Linux).

Looks good in general, although I am not sure if '__libc_type' name
does show its intent.  Although thing that I am not sure is if we need
to access it through atomics (since dlmopen/static dlopen might be
called by multiple threads and it access does not seemed to be protected
by any lock).

> ---
>  csu/init-first.c                    |  8 +-------
>  csu/libc-start.c                    |  4 +---
>  dlfcn/dlmopen.c                     |  6 ++++++
>  elf/Versions                        |  3 +++
>  elf/dl-open.c                       |  2 +-
>  elf/dl-sysdep.c                     |  2 --
>  elf/libc_early_init.c               | 22 ++++++++++++++++++++++
>  include/libc-internal.h             | 18 +++++++++++++++++-
>  misc/sbrk.c                         | 11 +++++++++--
>  sysdeps/mach/hurd/dl-sysdep.c       |  2 --
>  sysdeps/mach/hurd/i386/init-first.c |  5 +----
>  11 files changed, 61 insertions(+), 22 deletions(-)
> 
> diff --git a/csu/init-first.c b/csu/init-first.c
> index 47aaacdbd0..28fa66ad54 100644
> --- a/csu/init-first.c
> +++ b/csu/init-first.c
> @@ -28,10 +28,6 @@
>  
>  #include <ldsodefs.h>
>  
> -/* Set nonzero if we have to be prepared for more than one libc being
> -   used in the process.  Safe assumption if initializer never runs.  */
> -int __libc_multiple_libcs attribute_hidden = 1;
> -
>  /* Remember the command line argument and enviroment contents for
>     later calls of initializers for dynamic libraries.  */
>  int __libc_argc attribute_hidden;

Ok.

> @@ -50,10 +46,8 @@ _init_first (int argc, char **argv, char **envp)
>  {
>  #endif
>  
> -  __libc_multiple_libcs = &_dl_starting_up && !_dl_starting_up;
> -
>    /* Make sure we don't initialize twice.  */
> -  if (!__libc_multiple_libcs)
> +  if (__libc_type != libc_type_secondary)
>      {
>        /* Set the FPU control word to the proper default value if the
>  	 kernel would use a different value.  */

Ok.

> diff --git a/csu/libc-start.c b/csu/libc-start.c
> index 2d4d2ed1f9..7e0c4ce610 100644
> --- a/csu/libc-start.c
> +++ b/csu/libc-start.c
> @@ -141,8 +141,6 @@ LIBC_START_MAIN (int (*main) (int, char **, char ** MAIN_AUXVEC_DECL),
>    /* Result of the 'main' function.  */
>    int result;
>  
> -  __libc_multiple_libcs = &_dl_starting_up && !_dl_starting_up;
> -
>  #ifndef SHARED
>    _dl_relocate_static_pie ();
>  

Ok.

> @@ -213,7 +211,7 @@ LIBC_START_MAIN (int (*main) (int, char **, char ** MAIN_AUXVEC_DECL),
>  # endif
>  
>  # ifdef DL_SYSDEP_OSCHECK
> -  if (!__libc_multiple_libcs)
> +  if (__libc_type != libc_type_secondary)
>      {
>        /* This needs to run to initiliaze _dl_osversion before TLS
>  	 setup might check it.  */

Ok.

> diff --git a/dlfcn/dlmopen.c b/dlfcn/dlmopen.c
> index 1396c818ac..c880c13443 100644
> --- a/dlfcn/dlmopen.c
> +++ b/dlfcn/dlmopen.c
> @@ -22,6 +22,7 @@
>  #include <stddef.h>
>  #include <unistd.h>
>  #include <ldsodefs.h>
> +#include <libc-internal.h>
>  
>  #if !defined SHARED && IS_IN (libdl)
>  
> @@ -79,6 +80,11 @@ void *
>  __dlmopen (Lmid_t nsid, const char *file, int mode DL_CALLER_DECL)
>  {
>  # ifdef SHARED
> +  /* Advertise that we may have multiple libc from now on.  Do not
> +     overwrite a libc_type_secondary value.  */
> +  if (__libc_type == libc_type_initial_only)
> +    __libc_type = libc_type_initial;
> +
>    if (!rtld_active ())
>      return _dlfcn_hook->dlmopen (nsid, file, mode, RETURN_ADDRESS (0));
>  # endif

Ok.

> diff --git a/elf/Versions b/elf/Versions
> index be88c48e6d..39df728f0a 100644
> --- a/elf/Versions
> +++ b/elf/Versions
> @@ -28,6 +28,9 @@ libc {
>      __libc_dlclose; __libc_dlopen_mode; __libc_dlsym; __libc_dlvsym;
>      __libc_early_init;
>  
> +    # Updated from libdl.
> +    __libc_type;
> +
>      # Internal error handling support.  Interposes the functions in ld.so.
>      _dl_signal_exception; _dl_catch_exception;
>      _dl_signal_error; _dl_catch_error;

Ok.

> diff --git a/elf/dl-open.c b/elf/dl-open.c
> index 8769e47051..7b7a9214b8 100644
> --- a/elf/dl-open.c
> +++ b/elf/dl-open.c
> @@ -790,7 +790,7 @@ dl_open_worker (void *a)
>  #ifndef SHARED
>    /* We must be the static _dl_open in libc.a.  A static program that
>       has loaded a dynamic object now has competition.  */
> -  __libc_multiple_libcs = 1;
> +  __libc_type = libc_type_initial;
>  #endif
>  
>    /* Let the user know about the opencount.  */

Ok.

> diff --git a/elf/dl-sysdep.c b/elf/dl-sysdep.c
> index 854570821c..6cc4a76560 100644
> --- a/elf/dl-sysdep.c
> +++ b/elf/dl-sysdep.c
> @@ -58,8 +58,6 @@ ElfW(Addr) _dl_base_addr;
>  #endif
>  int __libc_enable_secure attribute_relro = 0;
>  rtld_hidden_data_def (__libc_enable_secure)
> -int __libc_multiple_libcs = 0;	/* Defining this here avoids the inclusion
> -				   of init-first.  */
>  /* This variable contains the lowest stack address ever used.  */
>  void *__libc_stack_end attribute_relro = NULL;
>  rtld_hidden_data_def(__libc_stack_end)

Ok.

> diff --git a/elf/libc_early_init.c b/elf/libc_early_init.c
> index 725ab2f811..fec5d6035e 100644
> --- a/elf/libc_early_init.c
> +++ b/elf/libc_early_init.c
> @@ -17,9 +17,14 @@
>     <https://www.gnu.org/licenses/>.  */
>  
>  #include <ctype.h>
> +#include <ldsodefs.h>
>  #include <libc-early-init.h>
> +#include <libc-internal.h>
>  #include <sys/single_threaded.h>
>  
> +char __libc_type __attribute__ ((nocommon));
> +libc_hidden_def (__libc_type)
> +

I am not sure about the __libc_type, it seems a bit too vague and does not
indicate its real usage.  Maybe __libc_multiple_state ?

I also forgot why do we need a noncommon attribute here, is it to force
a linker error on multiple definitions?

>  void
>  __libc_early_init (_Bool initial)
>  {
> @@ -28,4 +33,21 @@ __libc_early_init (_Bool initial)
>  
>    /* Only the outer namespace is marked as single-threaded.  */
>    __libc_single_threaded = initial;
> +
> +#ifdef SHARED
> +  if (initial)
> +    {
> +      if (GLRO (dl_naudit) == 0)
> +        __libc_type = libc_type_initial_only;
> +      else
> +        /* If auditing is active, the initial libc is not the only
> +           libc.  */
> +        __libc_type = libc_type_initial;
> +    }
> +  else
> +    __libc_type = libc_type_secondary;
> +#else
> +  /* There is no auditing support for statically linked binaries.  */
> +  __libc_type = libc_type_initial_only;
> +#endif
>  }

Ok.

> diff --git a/include/libc-internal.h b/include/libc-internal.h
> index 915613c030..7331ff2c51 100644
> --- a/include/libc-internal.h
> +++ b/include/libc-internal.h
> @@ -47,6 +47,22 @@ extern void __init_misc (int, char **, char **) attribute_hidden;
>  extern __typeof (__profile_frequency) __profile_frequency attribute_hidden;
>  # endif
>  
> -extern int __libc_multiple_libcs attribute_hidden;
> +enum
> +  {
> +    /* The running libc is the only libc in the process image.  It can
> +       transition to libc_type_initial via dlmopen or static
> +       dlopen.  */
> +    libc_type_initial_only,
> +
> +    /* The running libc is the libc for the main program, but other
> +       libcs may have been loaded.  */
> +    libc_type_initial,
> +
> +    /* The running libc is a secondary libc (loaded via dlmopen, to
> +       support auditing, or for static dlopen).  */
> +    libc_type_secondary,
> +  };
> +extern char __libc_type;
> +libc_hidden_proto (__libc_type)
>  
>  #endif /* _LIBC_INTERNAL  */

Ok.

> diff --git a/misc/sbrk.c b/misc/sbrk.c
> index ba3322fba6..88840b426b 100644
> --- a/misc/sbrk.c
> +++ b/misc/sbrk.c
> @@ -16,9 +16,10 @@
>     <https://www.gnu.org/licenses/>.  */
>  
>  #include <errno.h>
> +#include <libc-internal.h>
> +#include <stdbool.h>
>  #include <stdint.h>
>  #include <unistd.h>
> -#include <libc-internal.h>
>  
>  /* Defined in brk.c.  */
>  extern void *__curbrk;
> @@ -37,7 +38,13 @@ __sbrk (intptr_t increment)
>       __curbrk from the kernel's brk value.  That way two separate
>       instances of __brk and __sbrk can share the heap, returning
>       interleaved pieces of it.  */
> -  if (__curbrk == NULL || __libc_multiple_libcs)
> +#if IS_IN (rtld)
> +  bool update_curbrk = __curbrk == NULL;
> +#else
> +  bool update_curbrk = (__curbrk == NULL
> +			|| __libc_type != libc_type_initial_only);
> +#endif
> +  if (update_curbrk)
>      if (__brk (0) < 0)		/* Initialize the break.  */
>        return (void *) -1;
>  

Ok.

> diff --git a/sysdeps/mach/hurd/dl-sysdep.c b/sysdeps/mach/hurd/dl-sysdep.c
> index 370495710e..a5169d85e7 100644
> --- a/sysdeps/mach/hurd/dl-sysdep.c
> +++ b/sysdeps/mach/hurd/dl-sysdep.c
> @@ -57,8 +57,6 @@ extern char **_environ;
>  
>  int __libc_enable_secure = 0;
>  rtld_hidden_data_def (__libc_enable_secure)
> -int __libc_multiple_libcs = 0;	/* Defining this here avoids the inclusion
> -				   of init-first.  */
>  /* This variable contains the lowest stack address ever used.  */
>  void *__libc_stack_end = NULL;
>  rtld_hidden_data_def(__libc_stack_end)

Ok.

> diff --git a/sysdeps/mach/hurd/i386/init-first.c b/sysdeps/mach/hurd/i386/init-first.c
> index 1827479f86..8497089f7a 100644
> --- a/sysdeps/mach/hurd/i386/init-first.c
> +++ b/sysdeps/mach/hurd/i386/init-first.c
> @@ -40,7 +40,6 @@ unsigned long int __hurd_threadvar_stack_mask;
>  #ifndef SHARED
>  int __libc_enable_secure;
>  #endif
> -int __libc_multiple_libcs attribute_hidden = 1;
>  
>  extern int __libc_argc attribute_hidden;
>  extern char **__libc_argv attribute_hidden;

Ok.

> @@ -56,13 +55,11 @@ DEFINE_HOOK (_hurd_preinit_hook, (void));
>  static void
>  posixland_init (int argc, char **argv, char **envp)
>  {
> -  __libc_multiple_libcs = &_dl_starting_up && !_dl_starting_up;
> -
>    /* Now we have relocations etc. we can start signals etc.  */
>    _hurd_libc_proc_init (argv);
>  
>    /* Make sure we don't initialize twice.  */
> -  if (!__libc_multiple_libcs)
> +  if (__libc_type != libc_type_secondary)
>      {
>        /* Set the FPU control word to the proper default value.  */
>        __setfpucw (__fpu_control);
> 

Ok.

Re: [PATCH 2/2] malloc: Use __libc_type to detect an inner libc

[Adhemerval Zanella]

On 13/11/2020 13:14, Florian Weimer via Libc-alpha wrote:
> The inner libc must not use sbrk.  _dl_addr occasionally shows up
> in profiles, but had to be used before because __libc_multiple_libs
> was unreliable.

LGTM.  Is the performance the only advantage of using sbrk on 
the inner libc? 

> ---
>  malloc/arena.c  | 7 +------
>  malloc/malloc.c | 2 ++
>  2 files changed, 3 insertions(+), 6 deletions(-)
> 
> diff --git a/malloc/arena.c b/malloc/arena.c
> index 202daf15b0..865eaa1a84 100644
> --- a/malloc/arena.c
> +++ b/malloc/arena.c
> @@ -296,12 +296,7 @@ ptmalloc_init (void)
>  #ifdef SHARED
>    /* In case this libc copy is in a non-default namespace, never use brk.
>       Likewise if dlopened from statically linked program.  */
> -  Dl_info di;
> -  struct link_map *l;
> -
> -  if (_dl_open_hook != NULL
> -      || (_dl_addr (ptmalloc_init, &di, &l, NULL) != 0
> -          && l->l_ns != LM_ID_BASE))
> +  if (__libc_type == libc_type_secondary)
>      __morecore = __failing_morecore;
>  #endif
>  
> diff --git a/malloc/malloc.c b/malloc/malloc.c
> index 5b87bdb081..0834e4181a 100644
> --- a/malloc/malloc.c
> +++ b/malloc/malloc.c
> @@ -247,6 +247,8 @@
>  /* For SINGLE_THREAD_P.  */
>  #include <sysdep-cancel.h>
>  
> +#include <libc-internal.h>
> +
>  /*
>    Debugging:
>  
>

Re: [PATCH 2/2] malloc: Use __libc_type to detect an inner libc

[Florian Weimer]

* Adhemerval Zanella via Libc-alpha:

> On 13/11/2020 13:14, Florian Weimer via Libc-alpha wrote:
>> The inner libc must not use sbrk.  _dl_addr occasionally shows up
>> in profiles, but had to be used before because __libc_multiple_libs
>> was unreliable.
>
> LGTM.  Is the performance the only advantage of using sbrk on 
> the inner libc?

Avoiding sbrk in the inner libc is required for correctness because sbrk
is not thread-safe, and there is no external synchronization.

The startup performance improvement comes from not calling _dl_addr.  On
POWER, it saves around 150,000 instructions, according to a quick test.

Thanks,
Florian
-- 
Red Hat GmbH, https://de.redhat.com/ , Registered seat: Grasbrunn,
Commercial register: Amtsgericht Muenchen, HRB 153243,
Managing Directors: Charles Cachera, Brian Klemm, Laurie Krebs, Michael O'Neill

Re: [PATCH 1/2] Replace __libc_multiple_libcs with tri-state __libc_type

[Florian Weimer]

* Adhemerval Zanella via Libc-alpha:

> Looks good in general, although I am not sure if '__libc_type' name
> does show its intent.  Although thing that I am not sure is if we need
> to access it through atomics (since dlmopen/static dlopen might be
> called by multiple threads and it access does not seemed to be protected
> by any lock).

The sbrk code is inherently racy because it lacks locking.

We could move it into ld.so, add locking, and thus make it safe(r) to
use from multiple namespaces (except after static dlopen).

sbrk (0) could bypass the lock in case some crash handler calls that as
part of its error reporting.

Maybe that's the way to go?

Or we could leave it in libc.so (with or without locking), and have it
fail in inner namespaces (for non-zero arguments), in which case we
could do with a two-state __libc_initial variable that has never to be
reset after initialization via __libc_early_init.  I think the
simplicity of that is quite attractive, so I'm going to try that first.

>> diff --git a/elf/libc_early_init.c b/elf/libc_early_init.c
>> index 725ab2f811..fec5d6035e 100644
>> --- a/elf/libc_early_init.c
>> +++ b/elf/libc_early_init.c
>> @@ -17,9 +17,14 @@
>>     <https://www.gnu.org/licenses/>.  */
>>  
>>  #include <ctype.h>
>> +#include <ldsodefs.h>
>>  #include <libc-early-init.h>
>> +#include <libc-internal.h>
>>  #include <sys/single_threaded.h>
>>  
>> +char __libc_type __attribute__ ((nocommon));
>> +libc_hidden_def (__libc_type)
>> +
>
> I am not sure about the __libc_type, it seems a bit too vague and does not
> indicate its real usage.  Maybe __libc_multiple_state ?

__libc_multiple if it remains tri-state?

> I also forgot why do we need a noncommon attribute here, is it to force
> a linker error on multiple definitions?

It's required because of the libc_hidden_proto/libc_hidden_def,
otherwise the alias can't be created.

Thanks,
Florian
-- 
Red Hat GmbH, https://de.redhat.com/ , Registered seat: Grasbrunn,
Commercial register: Amtsgericht Muenchen, HRB 153243,
Managing Directors: Charles Cachera, Brian Klemm, Laurie Krebs, Michael O'Neill

Re: [PATCH 2/2] malloc: Use __libc_type to detect an inner libc

[Adhemerval Zanella]

On 13/11/2020 16:05, Florian Weimer wrote:
> * Adhemerval Zanella via Libc-alpha:
> 
>> On 13/11/2020 13:14, Florian Weimer via Libc-alpha wrote:
>>> The inner libc must not use sbrk.  _dl_addr occasionally shows up
>>> in profiles, but had to be used before because __libc_multiple_libs
>>> was unreliable.
>>
>> LGTM.  Is the performance the only advantage of using sbrk on 
>> the inner libc?
> 
> Avoiding sbrk in the inner libc is required for correctness because sbrk
> is not thread-safe, and there is no external synchronization.
> 
> The startup performance improvement comes from not calling _dl_addr.  On
> POWER, it saves around 150,000 instructions, according to a quick test.

In fact I meant what is the gain of using sbrk on outer libc.  I am asking
because maybe an option would be to use the same strategy regardless whether
it is a inner or outer libc.

Re: [PATCH 1/2] Replace __libc_multiple_libcs with tri-state __libc_type

[Adhemerval Zanella]

On 13/11/2020 19:02, Florian Weimer wrote:
> * Adhemerval Zanella via Libc-alpha:
> 
>> Looks good in general, although I am not sure if '__libc_type' name
>> does show its intent.  Although thing that I am not sure is if we need
>> to access it through atomics (since dlmopen/static dlopen might be
>> called by multiple threads and it access does not seemed to be protected
>> by any lock).
> 
> The sbrk code is inherently racy because it lacks locking.
> 
> We could move it into ld.so, add locking, and thus make it safe(r) to
> use from multiple namespaces (except after static dlopen).
> 
> sbrk (0) could bypass the lock in case some crash handler calls that as
> part of its error reporting.
> 
> Maybe that's the way to go?

This sounds reasonable, and I qam also wondering if we could extend and 
remove sbrk usage from malloc itself.  Depending of the pattern usage, it 
might incur in some performance penalty, but it might also simplify the
malloc implementation and remove some inherent concurrent issues.

> 
> Or we could leave it in libc.so (with or without locking), and have it
> fail in inner namespaces (for non-zero arguments), in which case we
> could do with a two-state __libc_initial variable that has never to be
> reset after initialization via __libc_early_init.  I think the
> simplicity of that is quite attractive, so I'm going to try that first.

I am not very found in this internal failing mechanism because it is
another semantic state we need to be aware while debugging the inner
libc usage (sbrk fail depending on how libc.so is loaded).

> 
>>> diff --git a/elf/libc_early_init.c b/elf/libc_early_init.c
>>> index 725ab2f811..fec5d6035e 100644
>>> --- a/elf/libc_early_init.c
>>> +++ b/elf/libc_early_init.c
>>> @@ -17,9 +17,14 @@
>>>     <https://www.gnu.org/licenses/>.  */
>>>  
>>>  #include <ctype.h>
>>> +#include <ldsodefs.h>
>>>  #include <libc-early-init.h>
>>> +#include <libc-internal.h>
>>>  #include <sys/single_threaded.h>
>>>  
>>> +char __libc_type __attribute__ ((nocommon));
>>> +libc_hidden_def (__libc_type)
>>> +
>>
>> I am not sure about the __libc_type, it seems a bit too vague and does not
>> indicate its real usage.  Maybe __libc_multiple_state ?
> 
> __libc_multiple if it remains tri-state?

Sounds good.

> 
>> I also forgot why do we need a noncommon attribute here, is it to force
>> a linker error on multiple definitions?
> 
> It's required because of the libc_hidden_proto/libc_hidden_def,
> otherwise the alias can't be created.

Thanks.

Re: [PATCH 2/2] malloc: Use __libc_type to detect an inner libc

[Florian Weimer]

* Adhemerval Zanella:

> On 13/11/2020 16:05, Florian Weimer wrote:
>> * Adhemerval Zanella via Libc-alpha:
>> 
>>> On 13/11/2020 13:14, Florian Weimer via Libc-alpha wrote:
>>>> The inner libc must not use sbrk.  _dl_addr occasionally shows up
>>>> in profiles, but had to be used before because __libc_multiple_libs
>>>> was unreliable.
>>>
>>> LGTM.  Is the performance the only advantage of using sbrk on 
>>> the inner libc?
>> 
>> Avoiding sbrk in the inner libc is required for correctness because sbrk
>> is not thread-safe, and there is no external synchronization.
>> 
>> The startup performance improvement comes from not calling _dl_addr.  On
>> POWER, it saves around 150,000 instructions, according to a quick test.
>
> In fact I meant what is the gain of using sbrk on outer libc.  I am asking
> because maybe an option would be to use the same strategy regardless whether
> it is a inner or outer libc.

Oh, that's totally unclear.  The main arena has a very different layout
from the other arenas.

The __morecore hook exposes the inner sbrk-based workings of our malloc
as part of the ABI.  I'm not sure if we can restructure the code so that
__morecore is never called, or if that would break existing users (such
as libhugetlbfs).

Thanks,
Florian
-- 
Red Hat GmbH, https://de.redhat.com/ , Registered seat: Grasbrunn,
Commercial register: Amtsgericht Muenchen, HRB 153243,
Managing Directors: Charles Cachera, Brian Klemm, Laurie Krebs, Michael O'Neill