[PATCH 0/3] Fix elf/tst-dl_find_objects with --enable-hardcoded-path-in-tests

[PATCH 0/3] Fix elf/tst-dl_find_objects with --enable-hardcoded-path-in-tests

[Florian Weimer]

This mini-series reworks the way we we set up l_contiguous for the main
mapping.  This uncovered an oddity in some binaries, tracked as bug
28743.  Due to the oddity, I do not want to assert that the main mapping
is contiguous (on most targets).

Thanks,
Florian

Florian Weimer (3):
  elf: Introduce rtld_setup_main_map
  elf: Set l_contiguous to 1 for the main map in more cases
  elf/tst-dl_find_object: Disable subtests for non-contiguous maps (bug
    28732)

 elf/rtld.c               | 328 ++++++++++++++++++++++-----------------
 elf/tst-dl_find_object.c |  29 ++--
 2 files changed, 201 insertions(+), 156 deletions(-)


base-commit: a51faeee6ae68da63e65eb0a1eb6c9ec2ce2148b
-- 
2.33.1

[PATCH 1/3] elf: Introduce rtld_setup_main_map

[Florian Weimer]

This function collects most of the processing needed to initialize
the link map for the main executable.
---
 elf/rtld.c | 303 ++++++++++++++++++++++++++++-------------------------
 1 file changed, 159 insertions(+), 144 deletions(-)

diff --git a/elf/rtld.c b/elf/rtld.c
index 24e48bf3fa..ba6e31377d 100644
--- a/elf/rtld.c
+++ b/elf/rtld.c
@@ -1126,17 +1126,172 @@ rtld_chain_load (struct link_map *main_map, char *argv0)
 		     rtld_soname, pathname, errcode);
 }
 
+/* Called to complete the initialization of the link map for the main
+   executable.  Returns true if there is a PT_INTERP segment.  */
+static bool
+rtld_setup_main_map (struct link_map *main_map)
+{
+  /* This have already been filled in right after _dl_new_object, or
+     as part of _dl_map_object.  */
+  const ElfW(Phdr) *phdr = main_map->l_phdr;
+  ElfW(Word) phnum = main_map->l_phnum;
+
+  bool has_interp = false;
+
+  main_map->l_map_end = 0;
+  main_map->l_text_end = 0;
+  /* Perhaps the executable has no PT_LOAD header entries at all.  */
+  main_map->l_map_start = ~0;
+  /* And it was opened directly.  */
+  ++main_map->l_direct_opencount;
+
+  /* Scan the program header table for the dynamic section.  */
+  for (const ElfW(Phdr) *ph = phdr; ph < &phdr[phnum]; ++ph)
+    switch (ph->p_type)
+      {
+      case PT_PHDR:
+	/* Find out the load address.  */
+	main_map->l_addr = (ElfW(Addr)) phdr - ph->p_vaddr;
+	break;
+      case PT_DYNAMIC:
+	/* This tells us where to find the dynamic section,
+	   which tells us everything we need to do.  */
+	main_map->l_ld = (void *) main_map->l_addr + ph->p_vaddr;
+	main_map->l_ld_readonly = (ph->p_flags & PF_W) == 0;
+	break;
+      case PT_INTERP:
+	/* This "interpreter segment" was used by the program loader to
+	   find the program interpreter, which is this program itself, the
+	   dynamic linker.  We note what name finds us, so that a future
+	   dlopen call or DT_NEEDED entry, for something that wants to link
+	   against the dynamic linker as a shared library, will know that
+	   the shared object is already loaded.  */
+	_dl_rtld_libname.name = ((const char *) main_map->l_addr
+				 + ph->p_vaddr);
+	/* _dl_rtld_libname.next = NULL;	Already zero.  */
+	GL(dl_rtld_map).l_libname = &_dl_rtld_libname;
+
+	/* Ordinarilly, we would get additional names for the loader from
+	   our DT_SONAME.  This can't happen if we were actually linked as
+	   a static executable (detect this case when we have no DYNAMIC).
+	   If so, assume the filename component of the interpreter path to
+	   be our SONAME, and add it to our name list.  */
+	if (GL(dl_rtld_map).l_ld == NULL)
+	  {
+	    const char *p = NULL;
+	    const char *cp = _dl_rtld_libname.name;
+
+	    /* Find the filename part of the path.  */
+	    while (*cp != '\0')
+	      if (*cp++ == '/')
+		p = cp;
+
+	    if (p != NULL)
+	      {
+		_dl_rtld_libname2.name = p;
+		/* _dl_rtld_libname2.next = NULL;  Already zero.  */
+		_dl_rtld_libname.next = &_dl_rtld_libname2;
+	      }
+	  }
+
+	has_interp = true;
+	break;
+      case PT_LOAD:
+	{
+	  ElfW(Addr) mapstart;
+	  ElfW(Addr) allocend;
+
+	  /* Remember where the main program starts in memory.  */
+	  mapstart = (main_map->l_addr
+		      + (ph->p_vaddr & ~(GLRO(dl_pagesize) - 1)));
+	  if (main_map->l_map_start > mapstart)
+	    main_map->l_map_start = mapstart;
+
+	  /* Also where it ends.  */
+	  allocend = main_map->l_addr + ph->p_vaddr + ph->p_memsz;
+	  if (main_map->l_map_end < allocend)
+	    main_map->l_map_end = allocend;
+	  if ((ph->p_flags & PF_X) && allocend > main_map->l_text_end)
+	    main_map->l_text_end = allocend;
+	}
+	break;
+
+      case PT_TLS:
+	if (ph->p_memsz > 0)
+	  {
+	    /* Note that in the case the dynamic linker we duplicate work
+	       here since we read the PT_TLS entry already in
+	       _dl_start_final.  But the result is repeatable so do not
+	       check for this special but unimportant case.  */
+	    main_map->l_tls_blocksize = ph->p_memsz;
+	    main_map->l_tls_align = ph->p_align;
+	    if (ph->p_align == 0)
+	      main_map->l_tls_firstbyte_offset = 0;
+	    else
+	      main_map->l_tls_firstbyte_offset = (ph->p_vaddr
+						  & (ph->p_align - 1));
+	    main_map->l_tls_initimage_size = ph->p_filesz;
+	    main_map->l_tls_initimage = (void *) ph->p_vaddr;
+
+	    /* This image gets the ID one.  */
+	    GL(dl_tls_max_dtv_idx) = main_map->l_tls_modid = 1;
+	  }
+	break;
+
+      case PT_GNU_STACK:
+	GL(dl_stack_flags) = ph->p_flags;
+	break;
+
+      case PT_GNU_RELRO:
+	main_map->l_relro_addr = ph->p_vaddr;
+	main_map->l_relro_size = ph->p_memsz;
+	break;
+      }
+  /* Process program headers again, but scan them backwards so
+     that PT_NOTE can be skipped if PT_GNU_PROPERTY exits.  */
+  for (const ElfW(Phdr) *ph = &phdr[phnum]; ph != phdr; --ph)
+    switch (ph[-1].p_type)
+      {
+      case PT_NOTE:
+	_dl_process_pt_note (main_map, -1, &ph[-1]);
+	break;
+      case PT_GNU_PROPERTY:
+	_dl_process_pt_gnu_property (main_map, -1, &ph[-1]);
+	break;
+      }
+
+  /* Adjust the address of the TLS initialization image in case
+     the executable is actually an ET_DYN object.  */
+  if (main_map->l_tls_initimage != NULL)
+    main_map->l_tls_initimage
+      = (char *) main_map->l_tls_initimage + main_map->l_addr;
+  if (! main_map->l_map_end)
+    main_map->l_map_end = ~0;
+  if (! main_map->l_text_end)
+    main_map->l_text_end = ~0;
+  if (! GL(dl_rtld_map).l_libname && GL(dl_rtld_map).l_name)
+    {
+      /* We were invoked directly, so the program might not have a
+	 PT_INTERP.  */
+      _dl_rtld_libname.name = GL(dl_rtld_map).l_name;
+      /* _dl_rtld_libname.next = NULL;	Already zero.  */
+      GL(dl_rtld_map).l_libname =  &_dl_rtld_libname;
+    }
+  else
+    assert (GL(dl_rtld_map).l_libname); /* How else did we get here?  */
+
+  return has_interp;
+}
+
 static void
 dl_main (const ElfW(Phdr) *phdr,
 	 ElfW(Word) phnum,
 	 ElfW(Addr) *user_entry,
 	 ElfW(auxv_t) *auxv)
 {
-  const ElfW(Phdr) *ph;
   struct link_map *main_map;
   size_t file_size;
   char *file;
-  bool has_interp = false;
   unsigned int i;
   bool prelinked = false;
   bool rtld_is_main = false;
@@ -1350,7 +1505,7 @@ dl_main (const ElfW(Phdr) *phdr,
 	 load the program below unless it has a PT_GNU_STACK indicating
 	 nonexecutable stack is ok.  */
 
-      for (ph = phdr; ph < &phdr[phnum]; ++ph)
+      for (const ElfW(Phdr) *ph = phdr; ph < &phdr[phnum]; ++ph)
 	if (ph->p_type == PT_GNU_STACK)
 	  {
 	    GL(dl_stack_flags) = ph->p_flags;
@@ -1469,147 +1624,7 @@ dl_main (const ElfW(Phdr) *phdr,
 	 information for the program.  */
     }
 
-  main_map->l_map_end = 0;
-  main_map->l_text_end = 0;
-  /* Perhaps the executable has no PT_LOAD header entries at all.  */
-  main_map->l_map_start = ~0;
-  /* And it was opened directly.  */
-  ++main_map->l_direct_opencount;
-
-  /* Scan the program header table for the dynamic section.  */
-  for (ph = phdr; ph < &phdr[phnum]; ++ph)
-    switch (ph->p_type)
-      {
-      case PT_PHDR:
-	/* Find out the load address.  */
-	main_map->l_addr = (ElfW(Addr)) phdr - ph->p_vaddr;
-	break;
-      case PT_DYNAMIC:
-	/* This tells us where to find the dynamic section,
-	   which tells us everything we need to do.  */
-	main_map->l_ld = (void *) main_map->l_addr + ph->p_vaddr;
-	main_map->l_ld_readonly = (ph->p_flags & PF_W) == 0;
-	break;
-      case PT_INTERP:
-	/* This "interpreter segment" was used by the program loader to
-	   find the program interpreter, which is this program itself, the
-	   dynamic linker.  We note what name finds us, so that a future
-	   dlopen call or DT_NEEDED entry, for something that wants to link
-	   against the dynamic linker as a shared library, will know that
-	   the shared object is already loaded.  */
-	_dl_rtld_libname.name = ((const char *) main_map->l_addr
-				 + ph->p_vaddr);
-	/* _dl_rtld_libname.next = NULL;	Already zero.  */
-	GL(dl_rtld_map).l_libname = &_dl_rtld_libname;
-
-	/* Ordinarilly, we would get additional names for the loader from
-	   our DT_SONAME.  This can't happen if we were actually linked as
-	   a static executable (detect this case when we have no DYNAMIC).
-	   If so, assume the filename component of the interpreter path to
-	   be our SONAME, and add it to our name list.  */
-	if (GL(dl_rtld_map).l_ld == NULL)
-	  {
-	    const char *p = NULL;
-	    const char *cp = _dl_rtld_libname.name;
-
-	    /* Find the filename part of the path.  */
-	    while (*cp != '\0')
-	      if (*cp++ == '/')
-		p = cp;
-
-	    if (p != NULL)
-	      {
-		_dl_rtld_libname2.name = p;
-		/* _dl_rtld_libname2.next = NULL;  Already zero.  */
-		_dl_rtld_libname.next = &_dl_rtld_libname2;
-	      }
-	  }
-
-	has_interp = true;
-	break;
-      case PT_LOAD:
-	{
-	  ElfW(Addr) mapstart;
-	  ElfW(Addr) allocend;
-
-	  /* Remember where the main program starts in memory.  */
-	  mapstart = (main_map->l_addr
-		      + (ph->p_vaddr & ~(GLRO(dl_pagesize) - 1)));
-	  if (main_map->l_map_start > mapstart)
-	    main_map->l_map_start = mapstart;
-
-	  /* Also where it ends.  */
-	  allocend = main_map->l_addr + ph->p_vaddr + ph->p_memsz;
-	  if (main_map->l_map_end < allocend)
-	    main_map->l_map_end = allocend;
-	  if ((ph->p_flags & PF_X) && allocend > main_map->l_text_end)
-	    main_map->l_text_end = allocend;
-	}
-	break;
-
-      case PT_TLS:
-	if (ph->p_memsz > 0)
-	  {
-	    /* Note that in the case the dynamic linker we duplicate work
-	       here since we read the PT_TLS entry already in
-	       _dl_start_final.  But the result is repeatable so do not
-	       check for this special but unimportant case.  */
-	    main_map->l_tls_blocksize = ph->p_memsz;
-	    main_map->l_tls_align = ph->p_align;
-	    if (ph->p_align == 0)
-	      main_map->l_tls_firstbyte_offset = 0;
-	    else
-	      main_map->l_tls_firstbyte_offset = (ph->p_vaddr
-						  & (ph->p_align - 1));
-	    main_map->l_tls_initimage_size = ph->p_filesz;
-	    main_map->l_tls_initimage = (void *) ph->p_vaddr;
-
-	    /* This image gets the ID one.  */
-	    GL(dl_tls_max_dtv_idx) = main_map->l_tls_modid = 1;
-	  }
-	break;
-
-      case PT_GNU_STACK:
-	GL(dl_stack_flags) = ph->p_flags;
-	break;
-
-      case PT_GNU_RELRO:
-	main_map->l_relro_addr = ph->p_vaddr;
-	main_map->l_relro_size = ph->p_memsz;
-	break;
-      }
-  /* Process program headers again, but scan them backwards so
-     that PT_NOTE can be skipped if PT_GNU_PROPERTY exits.  */
-  for (ph = &phdr[phnum]; ph != phdr; --ph)
-    switch (ph[-1].p_type)
-      {
-      case PT_NOTE:
-	_dl_process_pt_note (main_map, -1, &ph[-1]);
-	break;
-      case PT_GNU_PROPERTY:
-	_dl_process_pt_gnu_property (main_map, -1, &ph[-1]);
-	break;
-      }
-
-  /* Adjust the address of the TLS initialization image in case
-     the executable is actually an ET_DYN object.  */
-  if (main_map->l_tls_initimage != NULL)
-    main_map->l_tls_initimage
-      = (char *) main_map->l_tls_initimage + main_map->l_addr;
-  if (! main_map->l_map_end)
-    main_map->l_map_end = ~0;
-  if (! main_map->l_text_end)
-    main_map->l_text_end = ~0;
-  if (! GL(dl_rtld_map).l_libname && GL(dl_rtld_map).l_name)
-    {
-      /* We were invoked directly, so the program might not have a
-	 PT_INTERP.  */
-      _dl_rtld_libname.name = GL(dl_rtld_map).l_name;
-      /* _dl_rtld_libname.next = NULL;	Already zero.  */
-      GL(dl_rtld_map).l_libname =  &_dl_rtld_libname;
-    }
-  else
-    assert (GL(dl_rtld_map).l_libname); /* How else did we get here?  */
+  bool has_interp = rtld_setup_main_map (main_map);
 
   /* If the current libname is different from the SONAME, add the
      latter as well.  */
-- 
2.33.1

Re: [PATCH 1/3] elf: Introduce rtld_setup_main_map

[H.J. Lu]

On Mon, Jan 3, 2022 at 9:12 AM Florian Weimer via Libc-alpha
<libc-alpha@sourceware.org> wrote:
>
> This function collects most of the processing needed to initialize
> the link map for the main executable.
> ---
>  elf/rtld.c | 303 ++++++++++++++++++++++++++++-------------------------
>  1 file changed, 159 insertions(+), 144 deletions(-)
>
> diff --git a/elf/rtld.c b/elf/rtld.c
> index 24e48bf3fa..ba6e31377d 100644
> --- a/elf/rtld.c
> +++ b/elf/rtld.c
> @@ -1126,17 +1126,172 @@ rtld_chain_load (struct link_map *main_map, char *argv0)
>                      rtld_soname, pathname, errcode);
>  }
>
> +/* Called to complete the initialization of the link map for the main
> +   executable.  Returns true if there is a PT_INTERP segment.  */
> +static bool
> +rtld_setup_main_map (struct link_map *main_map)
> +{
> +  /* This have already been filled in right after _dl_new_object, or
> +     as part of _dl_map_object.  */
> +  const ElfW(Phdr) *phdr = main_map->l_phdr;
> +  ElfW(Word) phnum = main_map->l_phnum;
> +
> +  bool has_interp = false;
> +
> +  main_map->l_map_end = 0;
> +  main_map->l_text_end = 0;
> +  /* Perhaps the executable has no PT_LOAD header entries at all.  */
> +  main_map->l_map_start = ~0;
> +  /* And it was opened directly.  */
> +  ++main_map->l_direct_opencount;
> +
> +  /* Scan the program header table for the dynamic section.  */
> +  for (const ElfW(Phdr) *ph = phdr; ph < &phdr[phnum]; ++ph)
> +    switch (ph->p_type)
> +      {
> +      case PT_PHDR:
> +       /* Find out the load address.  */
> +       main_map->l_addr = (ElfW(Addr)) phdr - ph->p_vaddr;
> +       break;
> +      case PT_DYNAMIC:
> +       /* This tells us where to find the dynamic section,
> +          which tells us everything we need to do.  */
> +       main_map->l_ld = (void *) main_map->l_addr + ph->p_vaddr;
> +       main_map->l_ld_readonly = (ph->p_flags & PF_W) == 0;
> +       break;
> +      case PT_INTERP:
> +       /* This "interpreter segment" was used by the program loader to
> +          find the program interpreter, which is this program itself, the
> +          dynamic linker.  We note what name finds us, so that a future
> +          dlopen call or DT_NEEDED entry, for something that wants to link
> +          against the dynamic linker as a shared library, will know that
> +          the shared object is already loaded.  */
> +       _dl_rtld_libname.name = ((const char *) main_map->l_addr
> +                                + ph->p_vaddr);
> +       /* _dl_rtld_libname.next = NULL;        Already zero.  */
> +       GL(dl_rtld_map).l_libname = &_dl_rtld_libname;
> +
> +       /* Ordinarilly, we would get additional names for the loader from
> +          our DT_SONAME.  This can't happen if we were actually linked as
> +          a static executable (detect this case when we have no DYNAMIC).
> +          If so, assume the filename component of the interpreter path to
> +          be our SONAME, and add it to our name list.  */
> +       if (GL(dl_rtld_map).l_ld == NULL)
> +         {
> +           const char *p = NULL;
> +           const char *cp = _dl_rtld_libname.name;
> +
> +           /* Find the filename part of the path.  */
> +           while (*cp != '\0')
> +             if (*cp++ == '/')
> +               p = cp;
> +
> +           if (p != NULL)
> +             {
> +               _dl_rtld_libname2.name = p;
> +               /* _dl_rtld_libname2.next = NULL;  Already zero.  */
> +               _dl_rtld_libname.next = &_dl_rtld_libname2;
> +             }
> +         }
> +
> +       has_interp = true;
> +       break;
> +      case PT_LOAD:
> +       {
> +         ElfW(Addr) mapstart;
> +         ElfW(Addr) allocend;
> +
> +         /* Remember where the main program starts in memory.  */
> +         mapstart = (main_map->l_addr
> +                     + (ph->p_vaddr & ~(GLRO(dl_pagesize) - 1)));
> +         if (main_map->l_map_start > mapstart)
> +           main_map->l_map_start = mapstart;
> +
> +         /* Also where it ends.  */
> +         allocend = main_map->l_addr + ph->p_vaddr + ph->p_memsz;
> +         if (main_map->l_map_end < allocend)
> +           main_map->l_map_end = allocend;
> +         if ((ph->p_flags & PF_X) && allocend > main_map->l_text_end)
> +           main_map->l_text_end = allocend;
> +       }
> +       break;
> +
> +      case PT_TLS:
> +       if (ph->p_memsz > 0)
> +         {
> +           /* Note that in the case the dynamic linker we duplicate work
> +              here since we read the PT_TLS entry already in
> +              _dl_start_final.  But the result is repeatable so do not
> +              check for this special but unimportant case.  */
> +           main_map->l_tls_blocksize = ph->p_memsz;
> +           main_map->l_tls_align = ph->p_align;
> +           if (ph->p_align == 0)
> +             main_map->l_tls_firstbyte_offset = 0;
> +           else
> +             main_map->l_tls_firstbyte_offset = (ph->p_vaddr
> +                                                 & (ph->p_align - 1));
> +           main_map->l_tls_initimage_size = ph->p_filesz;
> +           main_map->l_tls_initimage = (void *) ph->p_vaddr;
> +
> +           /* This image gets the ID one.  */
> +           GL(dl_tls_max_dtv_idx) = main_map->l_tls_modid = 1;
> +         }
> +       break;
> +
> +      case PT_GNU_STACK:
> +       GL(dl_stack_flags) = ph->p_flags;
> +       break;
> +
> +      case PT_GNU_RELRO:
> +       main_map->l_relro_addr = ph->p_vaddr;
> +       main_map->l_relro_size = ph->p_memsz;
> +       break;
> +      }
> +  /* Process program headers again, but scan them backwards so
> +     that PT_NOTE can be skipped if PT_GNU_PROPERTY exits.  */
> +  for (const ElfW(Phdr) *ph = &phdr[phnum]; ph != phdr; --ph)
> +    switch (ph[-1].p_type)
> +      {
> +      case PT_NOTE:
> +       _dl_process_pt_note (main_map, -1, &ph[-1]);
> +       break;
> +      case PT_GNU_PROPERTY:
> +       _dl_process_pt_gnu_property (main_map, -1, &ph[-1]);
> +       break;
> +      }
> +
> +  /* Adjust the address of the TLS initialization image in case
> +     the executable is actually an ET_DYN object.  */
> +  if (main_map->l_tls_initimage != NULL)
> +    main_map->l_tls_initimage
> +      = (char *) main_map->l_tls_initimage + main_map->l_addr;
> +  if (! main_map->l_map_end)
> +    main_map->l_map_end = ~0;
> +  if (! main_map->l_text_end)
> +    main_map->l_text_end = ~0;
> +  if (! GL(dl_rtld_map).l_libname && GL(dl_rtld_map).l_name)
> +    {
> +      /* We were invoked directly, so the program might not have a
> +        PT_INTERP.  */
> +      _dl_rtld_libname.name = GL(dl_rtld_map).l_name;
> +      /* _dl_rtld_libname.next = NULL; Already zero.  */
> +      GL(dl_rtld_map).l_libname =  &_dl_rtld_libname;
> +    }
> +  else
> +    assert (GL(dl_rtld_map).l_libname); /* How else did we get here?  */
> +
> +  return has_interp;
> +}
> +
>  static void
>  dl_main (const ElfW(Phdr) *phdr,
>          ElfW(Word) phnum,
>          ElfW(Addr) *user_entry,
>          ElfW(auxv_t) *auxv)
>  {
> -  const ElfW(Phdr) *ph;
>    struct link_map *main_map;
>    size_t file_size;
>    char *file;
> -  bool has_interp = false;
>    unsigned int i;
>    bool prelinked = false;
>    bool rtld_is_main = false;
> @@ -1350,7 +1505,7 @@ dl_main (const ElfW(Phdr) *phdr,
>          load the program below unless it has a PT_GNU_STACK indicating
>          nonexecutable stack is ok.  */
>
> -      for (ph = phdr; ph < &phdr[phnum]; ++ph)
> +      for (const ElfW(Phdr) *ph = phdr; ph < &phdr[phnum]; ++ph)
>         if (ph->p_type == PT_GNU_STACK)
>           {
>             GL(dl_stack_flags) = ph->p_flags;
> @@ -1469,147 +1624,7 @@ dl_main (const ElfW(Phdr) *phdr,
>          information for the program.  */
>      }
>
> -  main_map->l_map_end = 0;
> -  main_map->l_text_end = 0;
> -  /* Perhaps the executable has no PT_LOAD header entries at all.  */
> -  main_map->l_map_start = ~0;
> -  /* And it was opened directly.  */
> -  ++main_map->l_direct_opencount;
> -
> -  /* Scan the program header table for the dynamic section.  */
> -  for (ph = phdr; ph < &phdr[phnum]; ++ph)
> -    switch (ph->p_type)
> -      {
> -      case PT_PHDR:
> -       /* Find out the load address.  */
> -       main_map->l_addr = (ElfW(Addr)) phdr - ph->p_vaddr;
> -       break;
> -      case PT_DYNAMIC:
> -       /* This tells us where to find the dynamic section,
> -          which tells us everything we need to do.  */
> -       main_map->l_ld = (void *) main_map->l_addr + ph->p_vaddr;
> -       main_map->l_ld_readonly = (ph->p_flags & PF_W) == 0;
> -       break;
> -      case PT_INTERP:
> -       /* This "interpreter segment" was used by the program loader to
> -          find the program interpreter, which is this program itself, the
> -          dynamic linker.  We note what name finds us, so that a future
> -          dlopen call or DT_NEEDED entry, for something that wants to link
> -          against the dynamic linker as a shared library, will know that
> -          the shared object is already loaded.  */
> -       _dl_rtld_libname.name = ((const char *) main_map->l_addr
> -                                + ph->p_vaddr);
> -       /* _dl_rtld_libname.next = NULL;        Already zero.  */
> -       GL(dl_rtld_map).l_libname = &_dl_rtld_libname;
> -
> -       /* Ordinarilly, we would get additional names for the loader from
> -          our DT_SONAME.  This can't happen if we were actually linked as
> -          a static executable (detect this case when we have no DYNAMIC).
> -          If so, assume the filename component of the interpreter path to
> -          be our SONAME, and add it to our name list.  */
> -       if (GL(dl_rtld_map).l_ld == NULL)
> -         {
> -           const char *p = NULL;
> -           const char *cp = _dl_rtld_libname.name;
> -
> -           /* Find the filename part of the path.  */
> -           while (*cp != '\0')
> -             if (*cp++ == '/')
> -               p = cp;
> -
> -           if (p != NULL)
> -             {
> -               _dl_rtld_libname2.name = p;
> -               /* _dl_rtld_libname2.next = NULL;  Already zero.  */
> -               _dl_rtld_libname.next = &_dl_rtld_libname2;
> -             }
> -         }
> -
> -       has_interp = true;
> -       break;
> -      case PT_LOAD:
> -       {
> -         ElfW(Addr) mapstart;
> -         ElfW(Addr) allocend;
> -
> -         /* Remember where the main program starts in memory.  */
> -         mapstart = (main_map->l_addr
> -                     + (ph->p_vaddr & ~(GLRO(dl_pagesize) - 1)));
> -         if (main_map->l_map_start > mapstart)
> -           main_map->l_map_start = mapstart;
> -
> -         /* Also where it ends.  */
> -         allocend = main_map->l_addr + ph->p_vaddr + ph->p_memsz;
> -         if (main_map->l_map_end < allocend)
> -           main_map->l_map_end = allocend;
> -         if ((ph->p_flags & PF_X) && allocend > main_map->l_text_end)
> -           main_map->l_text_end = allocend;
> -       }
> -       break;
> -
> -      case PT_TLS:
> -       if (ph->p_memsz > 0)
> -         {
> -           /* Note that in the case the dynamic linker we duplicate work
> -              here since we read the PT_TLS entry already in
> -              _dl_start_final.  But the result is repeatable so do not
> -              check for this special but unimportant case.  */
> -           main_map->l_tls_blocksize = ph->p_memsz;
> -           main_map->l_tls_align = ph->p_align;
> -           if (ph->p_align == 0)
> -             main_map->l_tls_firstbyte_offset = 0;
> -           else
> -             main_map->l_tls_firstbyte_offset = (ph->p_vaddr
> -                                                 & (ph->p_align - 1));
> -           main_map->l_tls_initimage_size = ph->p_filesz;
> -           main_map->l_tls_initimage = (void *) ph->p_vaddr;
> -
> -           /* This image gets the ID one.  */
> -           GL(dl_tls_max_dtv_idx) = main_map->l_tls_modid = 1;
> -         }
> -       break;
> -
> -      case PT_GNU_STACK:
> -       GL(dl_stack_flags) = ph->p_flags;
> -       break;
> -
> -      case PT_GNU_RELRO:
> -       main_map->l_relro_addr = ph->p_vaddr;
> -       main_map->l_relro_size = ph->p_memsz;
> -       break;
> -      }
> -  /* Process program headers again, but scan them backwards so
> -     that PT_NOTE can be skipped if PT_GNU_PROPERTY exits.  */
> -  for (ph = &phdr[phnum]; ph != phdr; --ph)
> -    switch (ph[-1].p_type)
> -      {
> -      case PT_NOTE:
> -       _dl_process_pt_note (main_map, -1, &ph[-1]);
> -       break;
> -      case PT_GNU_PROPERTY:
> -       _dl_process_pt_gnu_property (main_map, -1, &ph[-1]);
> -       break;
> -      }
> -
> -  /* Adjust the address of the TLS initialization image in case
> -     the executable is actually an ET_DYN object.  */
> -  if (main_map->l_tls_initimage != NULL)
> -    main_map->l_tls_initimage
> -      = (char *) main_map->l_tls_initimage + main_map->l_addr;
> -  if (! main_map->l_map_end)
> -    main_map->l_map_end = ~0;
> -  if (! main_map->l_text_end)
> -    main_map->l_text_end = ~0;
> -  if (! GL(dl_rtld_map).l_libname && GL(dl_rtld_map).l_name)
> -    {
> -      /* We were invoked directly, so the program might not have a
> -        PT_INTERP.  */
> -      _dl_rtld_libname.name = GL(dl_rtld_map).l_name;
> -      /* _dl_rtld_libname.next = NULL; Already zero.  */
> -      GL(dl_rtld_map).l_libname =  &_dl_rtld_libname;
> -    }
> -  else
> -    assert (GL(dl_rtld_map).l_libname); /* How else did we get here?  */
> +  bool has_interp = rtld_setup_main_map (main_map);
>
>    /* If the current libname is different from the SONAME, add the
>       latter as well.  */
> --
> 2.33.1
>

LGTM.

Reviewed-by: H.J. Lu <hjl.tools@gmail.com>

Thanks.

-- 
H.J.

[PATCH 2/3] elf: Set l_contiguous to 1 for the main map in more cases

[Florian Weimer]

l_contiguous was not initialized at all for the main map and
always 0.  This commit adds code to check if the LOAD segments
are adjacent to each other, and sets l_contiguous accordingly.
This helps _dl_find_object because it is more efficient if the
main mapping is contiguous.

Note that not all (PIE or non-PIE) binaries are contiguous in this
way because BFD ld creates executables with LOAD holes:

ELF LOAD segments creating holes in the process image on GNU/Linux
https://sourceware.org/pipermail/binutils/2022-January/119082.html
https://sourceware.org/bugzilla/show_bug.cgi?id=28743
---
 elf/rtld.c | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/elf/rtld.c b/elf/rtld.c
index ba6e31377d..53293f5b13 100644
--- a/elf/rtld.c
+++ b/elf/rtld.c
@@ -1144,6 +1144,22 @@ rtld_setup_main_map (struct link_map *main_map)
   main_map->l_map_start = ~0;
   /* And it was opened directly.  */
   ++main_map->l_direct_opencount;
+  main_map->l_contiguous = 1;
+
+  /* A PT_LOAD segment at an unexpected address will clear the
+     l_contiguous flag.  The ELF specification says that PT_LOAD
+     segments need to be sorted in in increasing order, but perhaps
+     not all executables follow this requirement.  Having l_contiguous
+     equal to 1 is just an optimization, so the code below does not
+     try to sort the segments in case they are unordered.
+
+     There is one corner case in which l_contiguous is not set to 1,
+     but where it could be set: If a PIE (ET_DYN) binary is loaded by
+     glibc itself (not the kernel), it is always contiguous due to the
+     way the glibc loader works.  However, the kernel loader may still
+     create holes in this case, and the code here still uses 0
+     conservatively for the glibc-loaded case, too.  */
+  ElfW(Addr) expected_load_address = 0;
 
   /* Scan the program header table for the dynamic section.  */
   for (const ElfW(Phdr) *ph = phdr; ph < &phdr[phnum]; ++ph)
@@ -1207,12 +1223,21 @@ rtld_setup_main_map (struct link_map *main_map)
 	  if (main_map->l_map_start > mapstart)
 	    main_map->l_map_start = mapstart;
 
+	  if (main_map->l_contiguous && expected_load_address != 0
+	      && expected_load_address != mapstart)
+	    main_map->l_contiguous = 0;
+
 	  /* Also where it ends.  */
 	  allocend = main_map->l_addr + ph->p_vaddr + ph->p_memsz;
 	  if (main_map->l_map_end < allocend)
 	    main_map->l_map_end = allocend;
 	  if ((ph->p_flags & PF_X) && allocend > main_map->l_text_end)
 	    main_map->l_text_end = allocend;
+
+	  /* The next expected address is the page following this load
+	     segment.  */
+	  expected_load_address = ((allocend + GLRO(dl_pagesize) - 1)
+				   & ~(GLRO(dl_pagesize) - 1));
 	}
 	break;
 
-- 
2.33.1

Re: [PATCH 2/3] elf: Set l_contiguous to 1 for the main map in more cases

[H.J. Lu]

On Mon, Jan 3, 2022 at 9:12 AM Florian Weimer via Libc-alpha
<libc-alpha@sourceware.org> wrote:
>
> l_contiguous was not initialized at all for the main map and
> always 0.  This commit adds code to check if the LOAD segments
> are adjacent to each other, and sets l_contiguous accordingly.
> This helps _dl_find_object because it is more efficient if the
> main mapping is contiguous.
>
> Note that not all (PIE or non-PIE) binaries are contiguous in this
> way because BFD ld creates executables with LOAD holes:
>
> ELF LOAD segments creating holes in the process image on GNU/Linux
> https://sourceware.org/pipermail/binutils/2022-January/119082.html
> https://sourceware.org/bugzilla/show_bug.cgi?id=28743
> ---
>  elf/rtld.c | 25 +++++++++++++++++++++++++
>  1 file changed, 25 insertions(+)
>
> diff --git a/elf/rtld.c b/elf/rtld.c
> index ba6e31377d..53293f5b13 100644
> --- a/elf/rtld.c
> +++ b/elf/rtld.c
> @@ -1144,6 +1144,22 @@ rtld_setup_main_map (struct link_map *main_map)
>    main_map->l_map_start = ~0;
>    /* And it was opened directly.  */
>    ++main_map->l_direct_opencount;
> +  main_map->l_contiguous = 1;
> +
> +  /* A PT_LOAD segment at an unexpected address will clear the
> +     l_contiguous flag.  The ELF specification says that PT_LOAD
> +     segments need to be sorted in in increasing order, but perhaps
> +     not all executables follow this requirement.  Having l_contiguous
> +     equal to 1 is just an optimization, so the code below does not
> +     try to sort the segments in case they are unordered.
> +
> +     There is one corner case in which l_contiguous is not set to 1,
> +     but where it could be set: If a PIE (ET_DYN) binary is loaded by
> +     glibc itself (not the kernel), it is always contiguous due to the
> +     way the glibc loader works.  However, the kernel loader may still
> +     create holes in this case, and the code here still uses 0
> +     conservatively for the glibc-loaded case, too.  */
> +  ElfW(Addr) expected_load_address = 0;
>
>    /* Scan the program header table for the dynamic section.  */
>    for (const ElfW(Phdr) *ph = phdr; ph < &phdr[phnum]; ++ph)
> @@ -1207,12 +1223,21 @@ rtld_setup_main_map (struct link_map *main_map)
>           if (main_map->l_map_start > mapstart)
>             main_map->l_map_start = mapstart;
>
> +         if (main_map->l_contiguous && expected_load_address != 0
> +             && expected_load_address != mapstart)
> +           main_map->l_contiguous = 0;
> +
>           /* Also where it ends.  */
>           allocend = main_map->l_addr + ph->p_vaddr + ph->p_memsz;
>           if (main_map->l_map_end < allocend)
>             main_map->l_map_end = allocend;
>           if ((ph->p_flags & PF_X) && allocend > main_map->l_text_end)
>             main_map->l_text_end = allocend;
> +
> +         /* The next expected address is the page following this load
> +            segment.  */
> +         expected_load_address = ((allocend + GLRO(dl_pagesize) - 1)
> +                                  & ~(GLRO(dl_pagesize) - 1));
>         }
>         break;
>
> --
> 2.33.1
>

LGTM.

Reviewed-by: H.J. Lu <hjl.tools@gmail.com>

Thanks.

-- 
H.J.

[PATCH 3/3] elf/tst-dl_find_object: Disable subtests for non-contiguous maps (bug 28732)

[Florian Weimer]

---
 elf/tst-dl_find_object.c | 29 +++++++++++++++++------------
 1 file changed, 17 insertions(+), 12 deletions(-)

diff --git a/elf/tst-dl_find_object.c b/elf/tst-dl_find_object.c
index 21cdc0f848..2ad1924088 100644
--- a/elf/tst-dl_find_object.c
+++ b/elf/tst-dl_find_object.c
@@ -71,19 +71,24 @@ check (void *address,
               __FILE__, line, address,
               actual.dlfo_flags, expected->dlfo_flags);
     }
-  if (actual.dlfo_flags != expected->dlfo_flags)
+  if (expected->dlfo_link_map->l_contiguous)
     {
-      support_record_failure ();
-      printf ("%s:%d: error: %p: map start is %p, expected %p\n",
-              __FILE__, line,
-              address, actual.dlfo_map_start, expected->dlfo_map_start);
-    }
-  if (actual.dlfo_map_end != expected->dlfo_map_end)
-    {
-      support_record_failure ();
-      printf ("%s:%d: error: %p: map end is %p, expected %p\n",
-              __FILE__, line,
-              address, actual.dlfo_map_end, expected->dlfo_map_end);
+      /* If the mappings are not contiguous, the actual and execpted
+         mappings may differ, so this subtest will not work.  */
+      if (actual.dlfo_flags != expected->dlfo_flags)
+        {
+          support_record_failure ();
+          printf ("%s:%d: error: %p: map start is %p, expected %p\n",
+                  __FILE__, line,
+                  address, actual.dlfo_map_start, expected->dlfo_map_start);
+        }
+      if (actual.dlfo_map_end != expected->dlfo_map_end)
+        {
+          support_record_failure ();
+          printf ("%s:%d: error: %p: map end is %p, expected %p\n",
+                  __FILE__, line,
+                  address, actual.dlfo_map_end, expected->dlfo_map_end);
+        }
     }
   if (actual.dlfo_link_map != expected->dlfo_link_map)
     {
-- 
2.33.1

Re: [PATCH 3/3] elf/tst-dl_find_object: Disable subtests for non-contiguous maps (bug 28732)

[H.J. Lu]

On Mon, Jan 3, 2022 at 9:13 AM Florian Weimer via Libc-alpha
<libc-alpha@sourceware.org> wrote:
>
> ---
>  elf/tst-dl_find_object.c | 29 +++++++++++++++++------------
>  1 file changed, 17 insertions(+), 12 deletions(-)
>
> diff --git a/elf/tst-dl_find_object.c b/elf/tst-dl_find_object.c
> index 21cdc0f848..2ad1924088 100644
> --- a/elf/tst-dl_find_object.c
> +++ b/elf/tst-dl_find_object.c
> @@ -71,19 +71,24 @@ check (void *address,
>                __FILE__, line, address,
>                actual.dlfo_flags, expected->dlfo_flags);
>      }
> -  if (actual.dlfo_flags != expected->dlfo_flags)
> +  if (expected->dlfo_link_map->l_contiguous)
>      {
> -      support_record_failure ();
> -      printf ("%s:%d: error: %p: map start is %p, expected %p\n",
> -              __FILE__, line,
> -              address, actual.dlfo_map_start, expected->dlfo_map_start);
> -    }
> -  if (actual.dlfo_map_end != expected->dlfo_map_end)
> -    {
> -      support_record_failure ();
> -      printf ("%s:%d: error: %p: map end is %p, expected %p\n",
> -              __FILE__, line,
> -              address, actual.dlfo_map_end, expected->dlfo_map_end);
> +      /* If the mappings are not contiguous, the actual and execpted
> +         mappings may differ, so this subtest will not work.  */
> +      if (actual.dlfo_flags != expected->dlfo_flags)
> +        {
> +          support_record_failure ();
> +          printf ("%s:%d: error: %p: map start is %p, expected %p\n",
> +                  __FILE__, line,
> +                  address, actual.dlfo_map_start, expected->dlfo_map_start);
> +        }
> +      if (actual.dlfo_map_end != expected->dlfo_map_end)
> +        {
> +          support_record_failure ();
> +          printf ("%s:%d: error: %p: map end is %p, expected %p\n",
> +                  __FILE__, line,
> +                  address, actual.dlfo_map_end, expected->dlfo_map_end);
> +        }
>      }
>    if (actual.dlfo_link_map != expected->dlfo_link_map)
>      {
> --
> 2.33.1
>

I still see

FAIL: elf/tst-dl_find_object

even when using the new linker with the fix for

https://sourceware.org/bugzilla/show_bug.cgi?id=28743

to remove the 1-page gap.  Which file doesn't have
non-contiguous mapping?

-- 
H.J.

Re: [PATCH 3/3] elf/tst-dl_find_object: Disable subtests for non-contiguous maps (bug 28732)

[H.J. Lu]

On Fri, Jan 14, 2022 at 7:06 AM H.J. Lu <hjl.tools@gmail.com> wrote:
>
> On Mon, Jan 3, 2022 at 9:13 AM Florian Weimer via Libc-alpha
> <libc-alpha@sourceware.org> wrote:
> >
> > ---
> >  elf/tst-dl_find_object.c | 29 +++++++++++++++++------------
> >  1 file changed, 17 insertions(+), 12 deletions(-)
> >
> > diff --git a/elf/tst-dl_find_object.c b/elf/tst-dl_find_object.c
> > index 21cdc0f848..2ad1924088 100644
> > --- a/elf/tst-dl_find_object.c
> > +++ b/elf/tst-dl_find_object.c
> > @@ -71,19 +71,24 @@ check (void *address,
> >                __FILE__, line, address,
> >                actual.dlfo_flags, expected->dlfo_flags);
> >      }
> > -  if (actual.dlfo_flags != expected->dlfo_flags)
> > +  if (expected->dlfo_link_map->l_contiguous)
> >      {
> > -      support_record_failure ();
> > -      printf ("%s:%d: error: %p: map start is %p, expected %p\n",
> > -              __FILE__, line,
> > -              address, actual.dlfo_map_start, expected->dlfo_map_start);
> > -    }
> > -  if (actual.dlfo_map_end != expected->dlfo_map_end)
> > -    {
> > -      support_record_failure ();
> > -      printf ("%s:%d: error: %p: map end is %p, expected %p\n",
> > -              __FILE__, line,
> > -              address, actual.dlfo_map_end, expected->dlfo_map_end);
> > +      /* If the mappings are not contiguous, the actual and execpted
> > +         mappings may differ, so this subtest will not work.  */
> > +      if (actual.dlfo_flags != expected->dlfo_flags)
> > +        {
> > +          support_record_failure ();
> > +          printf ("%s:%d: error: %p: map start is %p, expected %p\n",
> > +                  __FILE__, line,
> > +                  address, actual.dlfo_map_start, expected->dlfo_map_start);
> > +        }
> > +      if (actual.dlfo_map_end != expected->dlfo_map_end)
> > +        {
> > +          support_record_failure ();
> > +          printf ("%s:%d: error: %p: map end is %p, expected %p\n",
> > +                  __FILE__, line,
> > +                  address, actual.dlfo_map_end, expected->dlfo_map_end);
> > +        }
> >      }
> >    if (actual.dlfo_link_map != expected->dlfo_link_map)
> >      {
> > --
> > 2.33.1
> >
>
> I still see
>
> FAIL: elf/tst-dl_find_object
>
> even when using the new linker with the fix for
>
> https://sourceware.org/bugzilla/show_bug.cgi?id=28743
>
> to remove the 1-page gap.  Which file doesn't have
> non-contiguous mapping?
>

The linker bug isn't really fixed:

  [21] .eh_frame         PROGBITS        0000000000005ef0 005ef0
000758 00   A  0   0  8
  [22] .init_array       INIT_ARRAY      0000000000007000 007000
000010 08  WA  0   0  8
  [23] .fini_array       FINI_ARRAY      0000000000007010 007010
000008 08  WA  0   0  8
  [24] .data.rel.ro      PROGBITS        0000000000007020 007020
0000a0 00  WA  0   0 32
  [25] .dynamic          DYNAMIC         00000000000070c0 0070c0
000200 10  WA  8   0  8
  [26] .got              PROGBITS        00000000000072c0 0072c0
000078 08  WA  0   0  8
  [27] .got.plt          PROGBITS        0000000000008000 008000
0001d0 08  WA  0   0  8


-- 
H.J.

Re: [PATCH 3/3] elf/tst-dl_find_object: Disable subtests for non-contiguous maps (bug 28732)

[Florian Weimer]

* H. J. Lu:

> On Mon, Jan 3, 2022 at 9:13 AM Florian Weimer via Libc-alpha
> <libc-alpha@sourceware.org> wrote:
>>
>> ---
>>  elf/tst-dl_find_object.c | 29 +++++++++++++++++------------
>>  1 file changed, 17 insertions(+), 12 deletions(-)
>>
>> diff --git a/elf/tst-dl_find_object.c b/elf/tst-dl_find_object.c
>> index 21cdc0f848..2ad1924088 100644
>> --- a/elf/tst-dl_find_object.c
>> +++ b/elf/tst-dl_find_object.c
>> @@ -71,19 +71,24 @@ check (void *address,
>>                __FILE__, line, address,
>>                actual.dlfo_flags, expected->dlfo_flags);
>>      }
>> -  if (actual.dlfo_flags != expected->dlfo_flags)
>> +  if (expected->dlfo_link_map->l_contiguous)
>>      {
>> -      support_record_failure ();
>> -      printf ("%s:%d: error: %p: map start is %p, expected %p\n",
>> -              __FILE__, line,
>> -              address, actual.dlfo_map_start, expected->dlfo_map_start);
>> -    }
>> -  if (actual.dlfo_map_end != expected->dlfo_map_end)
>> -    {
>> -      support_record_failure ();
>> -      printf ("%s:%d: error: %p: map end is %p, expected %p\n",
>> -              __FILE__, line,
>> -              address, actual.dlfo_map_end, expected->dlfo_map_end);
>> +      /* If the mappings are not contiguous, the actual and execpted
>> +         mappings may differ, so this subtest will not work.  */
>> +      if (actual.dlfo_flags != expected->dlfo_flags)
>> +        {
>> +          support_record_failure ();
>> +          printf ("%s:%d: error: %p: map start is %p, expected %p\n",
>> +                  __FILE__, line,
>> +                  address, actual.dlfo_map_start, expected->dlfo_map_start);
>> +        }
>> +      if (actual.dlfo_map_end != expected->dlfo_map_end)
>> +        {
>> +          support_record_failure ();
>> +          printf ("%s:%d: error: %p: map end is %p, expected %p\n",
>> +                  __FILE__, line,
>> +                  address, actual.dlfo_map_end, expected->dlfo_map_end);
>> +        }
>>      }
>>    if (actual.dlfo_link_map != expected->dlfo_link_map)
>>      {
>> --
>> 2.33.1
>>
>
> I still see
>
> FAIL: elf/tst-dl_find_object
>
> even when using the new linker with the fix for
>
> https://sourceware.org/bugzilla/show_bug.cgi?id=28743
>
> to remove the 1-page gap.  Which file doesn't have
> non-contiguous mapping?

We never set l_contiguous for the main executable, so it doesn't matter
what the link editor does.  And none of the glibc fixes went in so far.

Thanks,
Florian

Re: [PATCH 3/3] elf/tst-dl_find_object: Disable subtests for non-contiguous maps (bug 28732)

[H.J. Lu]

On Fri, Jan 14, 2022 at 7:10 AM Florian Weimer <fweimer@redhat.com> wrote:
>
> * H. J. Lu:
>
> > On Mon, Jan 3, 2022 at 9:13 AM Florian Weimer via Libc-alpha
> > <libc-alpha@sourceware.org> wrote:
> >>
> >> ---
> >>  elf/tst-dl_find_object.c | 29 +++++++++++++++++------------
> >>  1 file changed, 17 insertions(+), 12 deletions(-)
> >>
> >> diff --git a/elf/tst-dl_find_object.c b/elf/tst-dl_find_object.c
> >> index 21cdc0f848..2ad1924088 100644
> >> --- a/elf/tst-dl_find_object.c
> >> +++ b/elf/tst-dl_find_object.c
> >> @@ -71,19 +71,24 @@ check (void *address,
> >>                __FILE__, line, address,
> >>                actual.dlfo_flags, expected->dlfo_flags);
> >>      }
> >> -  if (actual.dlfo_flags != expected->dlfo_flags)
> >> +  if (expected->dlfo_link_map->l_contiguous)
> >>      {
> >> -      support_record_failure ();
> >> -      printf ("%s:%d: error: %p: map start is %p, expected %p\n",
> >> -              __FILE__, line,
> >> -              address, actual.dlfo_map_start, expected->dlfo_map_start);
> >> -    }
> >> -  if (actual.dlfo_map_end != expected->dlfo_map_end)
> >> -    {
> >> -      support_record_failure ();
> >> -      printf ("%s:%d: error: %p: map end is %p, expected %p\n",
> >> -              __FILE__, line,
> >> -              address, actual.dlfo_map_end, expected->dlfo_map_end);
> >> +      /* If the mappings are not contiguous, the actual and execpted
> >> +         mappings may differ, so this subtest will not work.  */
> >> +      if (actual.dlfo_flags != expected->dlfo_flags)
> >> +        {
> >> +          support_record_failure ();
> >> +          printf ("%s:%d: error: %p: map start is %p, expected %p\n",
> >> +                  __FILE__, line,
> >> +                  address, actual.dlfo_map_start, expected->dlfo_map_start);
> >> +        }
> >> +      if (actual.dlfo_map_end != expected->dlfo_map_end)
> >> +        {
> >> +          support_record_failure ();
> >> +          printf ("%s:%d: error: %p: map end is %p, expected %p\n",
> >> +                  __FILE__, line,
> >> +                  address, actual.dlfo_map_end, expected->dlfo_map_end);
> >> +        }
> >>      }
> >>    if (actual.dlfo_link_map != expected->dlfo_link_map)
> >>      {
> >> --
> >> 2.33.1
> >>
> >
> > I still see
> >
> > FAIL: elf/tst-dl_find_object
> >
> > even when using the new linker with the fix for
> >
> > https://sourceware.org/bugzilla/show_bug.cgi?id=28743
> >
> > to remove the 1-page gap.  Which file doesn't have
> > non-contiguous mapping?
>
> We never set l_contiguous for the main executable, so it doesn't matter
> what the link editor does.  And none of the glibc fixes went in so far.
>
> Thanks,
> Florian
>

Should l_contiguous be set on the main executable? If not, why?

-- 
H.J.

Re: [PATCH 3/3] elf/tst-dl_find_object: Disable subtests for non-contiguous maps (bug 28732)

[Florian Weimer]

* H. J. Lu:

>> We never set l_contiguous for the main executable, so it doesn't matter
>> what the link editor does.  And none of the glibc fixes went in so far.
>>
>> Thanks,
>> Florian
>>
>
> Should l_contiguous be set on the main executable? If not, why?

I think it should be set even if the kernel loads the main executable.
See patch 2:

  [PATCH 2/3] elf: Set l_contiguous to 1 for the main map in more cases
  <https://sourceware.org/pipermail/libc-alpha/2022-January/134894.html>

Thanks,
Florian

Re: [PATCH 3/3] elf/tst-dl_find_object: Disable subtests for non-contiguous maps (bug 28732)

[H.J. Lu]

On Fri, Jan 14, 2022 at 7:39 AM Florian Weimer <fweimer@redhat.com> wrote:
>
> * H. J. Lu:
>
> >> We never set l_contiguous for the main executable, so it doesn't matter
> >> what the link editor does.  And none of the glibc fixes went in so far.
> >>
> >> Thanks,
> >> Florian
> >>
> >
> > Should l_contiguous be set on the main executable? If not, why?
>
> I think it should be set even if the kernel loads the main executable.
> See patch 2:
>
>   [PATCH 2/3] elf: Set l_contiguous to 1 for the main map in more cases
>   <https://sourceware.org/pipermail/libc-alpha/2022-January/134894.html>

If we have the second patch, do we still need the 3rd one?

-- 
H.J.

Re: [PATCH 3/3] elf/tst-dl_find_object: Disable subtests for non-contiguous maps (bug 28732)

[Florian Weimer]

* H. J. Lu:

> On Fri, Jan 14, 2022 at 7:39 AM Florian Weimer <fweimer@redhat.com> wrote:
>>
>> * H. J. Lu:
>>
>> >> We never set l_contiguous for the main executable, so it doesn't matter
>> >> what the link editor does.  And none of the glibc fixes went in so far.
>> >>
>> >> Thanks,
>> >> Florian
>> >>
>> >
>> > Should l_contiguous be set on the main executable? If not, why?
>>
>> I think it should be set even if the kernel loads the main executable.
>> See patch 2:
>>
>>   [PATCH 2/3] elf: Set l_contiguous to 1 for the main map in more cases
>>   <https://sourceware.org/pipermail/libc-alpha/2022-January/134894.html>
>
> If we have the second patch, do we still need the 3rd one?

I think gaps are expected on some architectures for main executable
(ia64?).

Thanks,
Florian

Re: [PATCH 3/3] elf/tst-dl_find_object: Disable subtests for non-contiguous maps (bug 28732)

[H.J. Lu]

On Fri, Jan 14, 2022 at 7:51 AM Florian Weimer <fweimer@redhat.com> wrote:
>
> * H. J. Lu:
>
> > On Fri, Jan 14, 2022 at 7:39 AM Florian Weimer <fweimer@redhat.com> wrote:
> >>
> >> * H. J. Lu:
> >>
> >> >> We never set l_contiguous for the main executable, so it doesn't matter
> >> >> what the link editor does.  And none of the glibc fixes went in so far.
> >> >>
> >> >> Thanks,
> >> >> Florian
> >> >>
> >> >
> >> > Should l_contiguous be set on the main executable? If not, why?
> >>
> >> I think it should be set even if the kernel loads the main executable.
> >> See patch 2:
> >>
> >>   [PATCH 2/3] elf: Set l_contiguous to 1 for the main map in more cases
> >>   <https://sourceware.org/pipermail/libc-alpha/2022-January/134894.html>
> >
> > If we have the second patch, do we still need the 3rd one?
>
> I think gaps are expected on some architectures for main executable
> (ia64?).
>

Please add a header to indicate that the gap is expected only on some
architectures.

-- 
H.J.

Re: [PATCH 3/3] elf/tst-dl_find_object: Disable subtests for non-contiguous maps (bug 28732)

[Florian Weimer]

* H. J. Lu:

> On Fri, Jan 14, 2022 at 7:51 AM Florian Weimer <fweimer@redhat.com> wrote:
>>
>> * H. J. Lu:
>>
>> > On Fri, Jan 14, 2022 at 7:39 AM Florian Weimer <fweimer@redhat.com> wrote:
>> >>
>> >> * H. J. Lu:
>> >>
>> >> >> We never set l_contiguous for the main executable, so it doesn't matter
>> >> >> what the link editor does.  And none of the glibc fixes went in so far.
>> >> >>
>> >> >> Thanks,
>> >> >> Florian
>> >> >>
>> >> >
>> >> > Should l_contiguous be set on the main executable? If not, why?
>> >>
>> >> I think it should be set even if the kernel loads the main executable.
>> >> See patch 2:
>> >>
>> >>   [PATCH 2/3] elf: Set l_contiguous to 1 for the main map in more cases
>> >>   <https://sourceware.org/pipermail/libc-alpha/2022-January/134894.html>
>> >
>> > If we have the second patch, do we still need the 3rd one?
>>
>> I think gaps are expected on some architectures for main executable
>> (ia64?).
>>
>
> Please add a header to indicate that the gap is expected only on some
> architectures.

Sorry, do you mean there should be a .h file to indicate whether the
architecture expects a gap?

How do I tell whether the gap is expected or the result of binutils
bugs?

Thanks,
Florian

Re: [PATCH 3/3] elf/tst-dl_find_object: Disable subtests for non-contiguous maps (bug 28732)

[H.J. Lu]

On Fri, Jan 14, 2022 at 7:56 AM Florian Weimer <fweimer@redhat.com> wrote:
>
> * H. J. Lu:
>
> > On Fri, Jan 14, 2022 at 7:51 AM Florian Weimer <fweimer@redhat.com> wrote:
> >>
> >> * H. J. Lu:
> >>
> >> > On Fri, Jan 14, 2022 at 7:39 AM Florian Weimer <fweimer@redhat.com> wrote:
> >> >>
> >> >> * H. J. Lu:
> >> >>
> >> >> >> We never set l_contiguous for the main executable, so it doesn't matter
> >> >> >> what the link editor does.  And none of the glibc fixes went in so far.
> >> >> >>
> >> >> >> Thanks,
> >> >> >> Florian
> >> >> >>
> >> >> >
> >> >> > Should l_contiguous be set on the main executable? If not, why?
> >> >>
> >> >> I think it should be set even if the kernel loads the main executable.
> >> >> See patch 2:
> >> >>
> >> >>   [PATCH 2/3] elf: Set l_contiguous to 1 for the main map in more cases
> >> >>   <https://sourceware.org/pipermail/libc-alpha/2022-January/134894.html>
> >> >
> >> > If we have the second patch, do we still need the 3rd one?
> >>
> >> I think gaps are expected on some architectures for main executable
> >> (ia64?).
> >>
> >
> > Please add a header to indicate that the gap is expected only on some
> > architectures.
>
> Sorry, do you mean there should be a .h file to indicate whether the
> architecture expects a gap?
>
> How do I tell whether the gap is expected or the result of binutils
> bugs?
>

You can add a linker test to check for the linker bug.

-- 
H.J.

Re: [PATCH 3/3] elf/tst-dl_find_object: Disable subtests for non-contiguous maps (bug 28732)

[Florian Weimer]

* H. J. Lu:

> You can add a linker test to check for the linker bug.

The linker bug could appear randomly, depending on the input files and
their contents.

Thanks,
Florian

Re: [PATCH 3/3] elf/tst-dl_find_object: Disable subtests for non-contiguous maps (bug 28732)

[H.J. Lu]

On Mon, Jan 3, 2022 at 9:13 AM Florian Weimer via Libc-alpha
<libc-alpha@sourceware.org> wrote:
>
> ---
>  elf/tst-dl_find_object.c | 29 +++++++++++++++++------------
>  1 file changed, 17 insertions(+), 12 deletions(-)
>
> diff --git a/elf/tst-dl_find_object.c b/elf/tst-dl_find_object.c
> index 21cdc0f848..2ad1924088 100644
> --- a/elf/tst-dl_find_object.c
> +++ b/elf/tst-dl_find_object.c
> @@ -71,19 +71,24 @@ check (void *address,
>                __FILE__, line, address,
>                actual.dlfo_flags, expected->dlfo_flags);
>      }
> -  if (actual.dlfo_flags != expected->dlfo_flags)
> +  if (expected->dlfo_link_map->l_contiguous)
>      {
> -      support_record_failure ();
> -      printf ("%s:%d: error: %p: map start is %p, expected %p\n",
> -              __FILE__, line,
> -              address, actual.dlfo_map_start, expected->dlfo_map_start);
> -    }
> -  if (actual.dlfo_map_end != expected->dlfo_map_end)
> -    {
> -      support_record_failure ();
> -      printf ("%s:%d: error: %p: map end is %p, expected %p\n",
> -              __FILE__, line,
> -              address, actual.dlfo_map_end, expected->dlfo_map_end);
> +      /* If the mappings are not contiguous, the actual and execpted
> +         mappings may differ, so this subtest will not work.  */
> +      if (actual.dlfo_flags != expected->dlfo_flags)
> +        {
> +          support_record_failure ();
> +          printf ("%s:%d: error: %p: map start is %p, expected %p\n",
> +                  __FILE__, line,
> +                  address, actual.dlfo_map_start, expected->dlfo_map_start);
> +        }
> +      if (actual.dlfo_map_end != expected->dlfo_map_end)
> +        {
> +          support_record_failure ();
> +          printf ("%s:%d: error: %p: map end is %p, expected %p\n",
> +                  __FILE__, line,
> +                  address, actual.dlfo_map_end, expected->dlfo_map_end);
> +        }
>      }
>    if (actual.dlfo_link_map != expected->dlfo_link_map)
>      {
> --
> 2.33.1
>

LGTM.

Reviewed-by: H.J. Lu <hjl.tools@gmail.com>

Thanks.

-- 
H.J.