[PATCH] [gdb] Refactor svr4_solib_ops::lm_addr_check

[PATCH] [gdb] Refactor svr4_solib_ops::lm_addr_check

[Tom de Vries]

I came across svr4_solib_ops::lm_addr_check and found it hard to follow.

In particular, it's indent-happy and minimizes code using a goto, resulting in
the following end-of-function:
...
            }
        }

    set_addr:
      li.l_addr = l_addr;
      li.l_addr_p = 1;
    }

  return li.l_addr;
}
...

I could just address the indentation, but I'd be adding a goto:
...
-      if (dynaddr + l_addr != l_dynaddr)
+      if (dynaddr + l_addr == l_dynaddr)
+        goto set_addr;
...

Instead, refactor the function into this form:
...
svr4_solib_ops::lm_addr_check (const solib &so, bfd *abfd) const
{
  auto &li = get_lm_info_svr4 (so);
  if (li.l_addr_p)
    return li.l_addr;

  auto set_addr = [&li] (CORE_ADDR addr)
  {
    li.l_addr = addr;
    li.l_addr_p = true;
    return addr;
  }

  if (...)
    return set_addr (l_addr);

  ...

  return set_addr (l_addr);
}
...
---
 gdb/solib-svr4.c | 160 +++++++++++++++++++++++------------------------
 1 file changed, 80 insertions(+), 80 deletions(-)

diff --git a/gdb/solib-svr4.c b/gdb/solib-svr4.c
index c4af1b11a4d..40b0fc55113 100644
--- a/gdb/solib-svr4.c
+++ b/gdb/solib-svr4.c
@@ -225,106 +225,106 @@ CORE_ADDR
 svr4_solib_ops::lm_addr_check (const solib &so, bfd *abfd) const
 {
   auto &li = get_lm_info_svr4 (so);
+  if (li.l_addr_p)
+    return li.l_addr;
 
-  if (!li.l_addr_p)
-    {
-      struct bfd_section *dyninfo_sect;
-      CORE_ADDR l_addr, l_dynaddr, dynaddr;
+  auto set_addr = [&li] (CORE_ADDR addr)
+  {
+    li.l_addr = addr;
+    li.l_addr_p = true;
+    return addr;
+  };
 
-      l_addr = li.l_addr_inferior;
+  struct bfd_section *dyninfo_sect;
+  CORE_ADDR l_addr, l_dynaddr, dynaddr;
 
-      if (!abfd || !this->has_lm_dynamic_from_link_map ())
-	goto set_addr;
+  l_addr = li.l_addr_inferior;
+  if (!abfd || !this->has_lm_dynamic_from_link_map ())
+    return set_addr (l_addr);
 
-      l_dynaddr = li.l_ld;
+  l_dynaddr = li.l_ld;
 
-      dyninfo_sect = bfd_get_section_by_name (abfd, ".dynamic");
-      if (dyninfo_sect == NULL)
-	goto set_addr;
+  dyninfo_sect = bfd_get_section_by_name (abfd, ".dynamic");
+  if (dyninfo_sect == NULL)
+    return set_addr (l_addr);
 
-      dynaddr = bfd_section_vma (dyninfo_sect);
+  dynaddr = bfd_section_vma (dyninfo_sect);
+  if (dynaddr + l_addr == l_dynaddr)
+    return set_addr (l_addr);
 
-      if (dynaddr + l_addr != l_dynaddr)
-	{
-	  CORE_ADDR align = 0x1000;
-	  CORE_ADDR minpagesize = align;
+  CORE_ADDR align = 0x1000;
+  CORE_ADDR minpagesize = align;
 
-	  if (bfd_get_flavour (abfd) == bfd_target_elf_flavour)
-	    {
-	      Elf_Internal_Ehdr *ehdr = elf_tdata (abfd)->elf_header;
-	      Elf_Internal_Phdr *phdr = elf_tdata (abfd)->phdr;
-	      int i;
+  if (bfd_get_flavour (abfd) == bfd_target_elf_flavour)
+    {
+      Elf_Internal_Ehdr *ehdr = elf_tdata (abfd)->elf_header;
+      Elf_Internal_Phdr *phdr = elf_tdata (abfd)->phdr;
+      int i;
 
-	      align = 1;
+      align = 1;
 
-	      for (i = 0; i < ehdr->e_phnum; i++)
-		if (phdr[i].p_type == PT_LOAD && phdr[i].p_align > align)
-		  align = phdr[i].p_align;
+      for (i = 0; i < ehdr->e_phnum; i++)
+	if (phdr[i].p_type == PT_LOAD && phdr[i].p_align > align)
+	  align = phdr[i].p_align;
 
-	      minpagesize = get_elf_backend_data (abfd)->minpagesize;
-	    }
+      minpagesize = get_elf_backend_data (abfd)->minpagesize;
+    }
 
-	  /* Turn it into a mask.  */
-	  align--;
+  /* Turn it into a mask.  */
+  align--;
 
-	  /* If the changes match the alignment requirements, we
-	     assume we're using a core file that was generated by the
-	     same binary, just prelinked with a different base offset.
-	     If it doesn't match, we may have a different binary, the
-	     same binary with the dynamic table loaded at an unrelated
-	     location, or anything, really.  To avoid regressions,
-	     don't adjust the base offset in the latter case, although
-	     odds are that, if things really changed, debugging won't
-	     quite work.
+  /* If the changes match the alignment requirements, we
+     assume we're using a core file that was generated by the
+     same binary, just prelinked with a different base offset.
+     If it doesn't match, we may have a different binary, the
+     same binary with the dynamic table loaded at an unrelated
+     location, or anything, really.  To avoid regressions,
+     don't adjust the base offset in the latter case, although
+     odds are that, if things really changed, debugging won't
+     quite work.
 
-	     One could expect more the condition
-	       ((l_addr & align) == 0 && ((l_dynaddr - dynaddr) & align) == 0)
-	     but the one below is relaxed for PPC.  The PPC kernel supports
-	     either 4k or 64k page sizes.  To be prepared for 64k pages,
-	     PPC ELF files are built using an alignment requirement of 64k.
-	     However, when running on a kernel supporting 4k pages, the memory
-	     mapping of the library may not actually happen on a 64k boundary!
+     One could expect more the condition
+     ((l_addr & align) == 0 && ((l_dynaddr - dynaddr) & align) == 0)
+     but the one below is relaxed for PPC.  The PPC kernel supports
+     either 4k or 64k page sizes.  To be prepared for 64k pages,
+     PPC ELF files are built using an alignment requirement of 64k.
+     However, when running on a kernel supporting 4k pages, the memory
+     mapping of the library may not actually happen on a 64k boundary!
 
-	     (In the usual case where (l_addr & align) == 0, this check is
-	     equivalent to the possibly expected check above.)
+     (In the usual case where (l_addr & align) == 0, this check is
+     equivalent to the possibly expected check above.)
 
-	     Even on PPC it must be zero-aligned at least for MINPAGESIZE.  */
+     Even on PPC it must be zero-aligned at least for MINPAGESIZE.  */
 
-	  l_addr = l_dynaddr - dynaddr;
+  l_addr = l_dynaddr - dynaddr;
 
-	  if ((l_addr & (minpagesize - 1)) == 0
-	      && (l_addr & align) == ((l_dynaddr - dynaddr) & align))
-	    {
-	      if (info_verbose)
-		gdb_printf (_("Using PIC (Position Independent Code) "
-			      "prelink displacement %s for \"%s\".\n"),
-			    paddress (current_inferior ()->arch (), l_addr),
-			    so.name.c_str ());
-	    }
-	  else
-	    {
-	      /* There is no way to verify the library file matches.  prelink
-		 can during prelinking of an unprelinked file (or unprelinking
-		 of a prelinked file) shift the DYNAMIC segment by arbitrary
-		 offset without any page size alignment.  There is no way to
-		 find out the ELF header and/or Program Headers for a limited
-		 verification if it they match.  One could do a verification
-		 of the DYNAMIC segment.  Still the found address is the best
-		 one GDB could find.  */
-
-	      warning (_(".dynamic section for \"%s\" "
-			 "is not at the expected address "
-			 "(wrong library or version mismatch?)"),
-			 so.name.c_str ());
-	    }
-	}
-
-    set_addr:
-      li.l_addr = l_addr;
-      li.l_addr_p = 1;
+  if ((l_addr & (minpagesize - 1)) == 0
+      && (l_addr & align) == ((l_dynaddr - dynaddr) & align))
+    {
+      if (info_verbose)
+	gdb_printf (_("Using PIC (Position Independent Code) "
+		      "prelink displacement %s for \"%s\".\n"),
+		    paddress (current_inferior ()->arch (), l_addr),
+		    so.name.c_str ());
+    }
+  else
+    {
+      /* There is no way to verify the library file matches.  prelink
+	 can during prelinking of an unprelinked file (or unprelinking
+	 of a prelinked file) shift the DYNAMIC segment by arbitrary
+	 offset without any page size alignment.  There is no way to
+	 find out the ELF header and/or Program Headers for a limited
+	 verification if it they match.  One could do a verification
+	 of the DYNAMIC segment.  Still the found address is the best
+	 one GDB could find.  */
+
+      warning (_(".dynamic section for \"%s\" "
+		 "is not at the expected address "
+		 "(wrong library or version mismatch?)"),
+	       so.name.c_str ());
     }
 
-  return li.l_addr;
+  return set_addr (l_addr);
 }
 
 struct svr4_so

base-commit: 0855b93cfb911a4feab76a31bb1914982b3e6979
-- 
2.51.0

[PING][PATCH] [gdb] Refactor svr4_solib_ops::lm_addr_check

[Tom de Vries]

On 9/11/26 9:19 PM, Tom de Vries wrote:
> I came across svr4_solib_ops::lm_addr_check and found it hard to follow.
> 
> In particular, it's indent-happy and minimizes code using a goto, resulting in
> the following end-of-function:
> ...
>              }
>          }
> 
>      set_addr:
>        li.l_addr = l_addr;
>        li.l_addr_p = 1;
>      }
> 
>    return li.l_addr;
> }
> ...
> 
> I could just address the indentation, but I'd be adding a goto:
> ...
> -      if (dynaddr + l_addr != l_dynaddr)
> +      if (dynaddr + l_addr == l_dynaddr)
> +        goto set_addr;
> ...
> 
> Instead, refactor the function into this form:
> ...
> svr4_solib_ops::lm_addr_check (const solib &so, bfd *abfd) const
> {
>    auto &li = get_lm_info_svr4 (so);
>    if (li.l_addr_p)
>      return li.l_addr;
> 
>    auto set_addr = [&li] (CORE_ADDR addr)
>    {
>      li.l_addr = addr;
>      li.l_addr_p = true;
>      return addr;
>    }
> 
>    if (...)
>      return set_addr (l_addr);
> 
>    ...
> 
>    return set_addr (l_addr);
> }
> ...

Ping.

Thanks,
- Tom

> ---
>   gdb/solib-svr4.c | 160 +++++++++++++++++++++++------------------------
>   1 file changed, 80 insertions(+), 80 deletions(-)
> 
> diff --git a/gdb/solib-svr4.c b/gdb/solib-svr4.c
> index c4af1b11a4d..40b0fc55113 100644
> --- a/gdb/solib-svr4.c
> +++ b/gdb/solib-svr4.c
> @@ -225,106 +225,106 @@ CORE_ADDR
>   svr4_solib_ops::lm_addr_check (const solib &so, bfd *abfd) const
>   {
>     auto &li = get_lm_info_svr4 (so);
> +  if (li.l_addr_p)
> +    return li.l_addr;
>   
> -  if (!li.l_addr_p)
> -    {
> -      struct bfd_section *dyninfo_sect;
> -      CORE_ADDR l_addr, l_dynaddr, dynaddr;
> +  auto set_addr = [&li] (CORE_ADDR addr)
> +  {
> +    li.l_addr = addr;
> +    li.l_addr_p = true;
> +    return addr;
> +  };
>   
> -      l_addr = li.l_addr_inferior;
> +  struct bfd_section *dyninfo_sect;
> +  CORE_ADDR l_addr, l_dynaddr, dynaddr;
>   
> -      if (!abfd || !this->has_lm_dynamic_from_link_map ())
> -	goto set_addr;
> +  l_addr = li.l_addr_inferior;
> +  if (!abfd || !this->has_lm_dynamic_from_link_map ())
> +    return set_addr (l_addr);
>   
> -      l_dynaddr = li.l_ld;
> +  l_dynaddr = li.l_ld;
>   
> -      dyninfo_sect = bfd_get_section_by_name (abfd, ".dynamic");
> -      if (dyninfo_sect == NULL)
> -	goto set_addr;
> +  dyninfo_sect = bfd_get_section_by_name (abfd, ".dynamic");
> +  if (dyninfo_sect == NULL)
> +    return set_addr (l_addr);
>   
> -      dynaddr = bfd_section_vma (dyninfo_sect);
> +  dynaddr = bfd_section_vma (dyninfo_sect);
> +  if (dynaddr + l_addr == l_dynaddr)
> +    return set_addr (l_addr);
>   
> -      if (dynaddr + l_addr != l_dynaddr)
> -	{
> -	  CORE_ADDR align = 0x1000;
> -	  CORE_ADDR minpagesize = align;
> +  CORE_ADDR align = 0x1000;
> +  CORE_ADDR minpagesize = align;
>   
> -	  if (bfd_get_flavour (abfd) == bfd_target_elf_flavour)
> -	    {
> -	      Elf_Internal_Ehdr *ehdr = elf_tdata (abfd)->elf_header;
> -	      Elf_Internal_Phdr *phdr = elf_tdata (abfd)->phdr;
> -	      int i;
> +  if (bfd_get_flavour (abfd) == bfd_target_elf_flavour)
> +    {
> +      Elf_Internal_Ehdr *ehdr = elf_tdata (abfd)->elf_header;
> +      Elf_Internal_Phdr *phdr = elf_tdata (abfd)->phdr;
> +      int i;
>   
> -	      align = 1;
> +      align = 1;
>   
> -	      for (i = 0; i < ehdr->e_phnum; i++)
> -		if (phdr[i].p_type == PT_LOAD && phdr[i].p_align > align)
> -		  align = phdr[i].p_align;
> +      for (i = 0; i < ehdr->e_phnum; i++)
> +	if (phdr[i].p_type == PT_LOAD && phdr[i].p_align > align)
> +	  align = phdr[i].p_align;
>   
> -	      minpagesize = get_elf_backend_data (abfd)->minpagesize;
> -	    }
> +      minpagesize = get_elf_backend_data (abfd)->minpagesize;
> +    }
>   
> -	  /* Turn it into a mask.  */
> -	  align--;
> +  /* Turn it into a mask.  */
> +  align--;
>   
> -	  /* If the changes match the alignment requirements, we
> -	     assume we're using a core file that was generated by the
> -	     same binary, just prelinked with a different base offset.
> -	     If it doesn't match, we may have a different binary, the
> -	     same binary with the dynamic table loaded at an unrelated
> -	     location, or anything, really.  To avoid regressions,
> -	     don't adjust the base offset in the latter case, although
> -	     odds are that, if things really changed, debugging won't
> -	     quite work.
> +  /* If the changes match the alignment requirements, we
> +     assume we're using a core file that was generated by the
> +     same binary, just prelinked with a different base offset.
> +     If it doesn't match, we may have a different binary, the
> +     same binary with the dynamic table loaded at an unrelated
> +     location, or anything, really.  To avoid regressions,
> +     don't adjust the base offset in the latter case, although
> +     odds are that, if things really changed, debugging won't
> +     quite work.
>   
> -	     One could expect more the condition
> -	       ((l_addr & align) == 0 && ((l_dynaddr - dynaddr) & align) == 0)
> -	     but the one below is relaxed for PPC.  The PPC kernel supports
> -	     either 4k or 64k page sizes.  To be prepared for 64k pages,
> -	     PPC ELF files are built using an alignment requirement of 64k.
> -	     However, when running on a kernel supporting 4k pages, the memory
> -	     mapping of the library may not actually happen on a 64k boundary!
> +     One could expect more the condition
> +     ((l_addr & align) == 0 && ((l_dynaddr - dynaddr) & align) == 0)
> +     but the one below is relaxed for PPC.  The PPC kernel supports
> +     either 4k or 64k page sizes.  To be prepared for 64k pages,
> +     PPC ELF files are built using an alignment requirement of 64k.
> +     However, when running on a kernel supporting 4k pages, the memory
> +     mapping of the library may not actually happen on a 64k boundary!
>   
> -	     (In the usual case where (l_addr & align) == 0, this check is
> -	     equivalent to the possibly expected check above.)
> +     (In the usual case where (l_addr & align) == 0, this check is
> +     equivalent to the possibly expected check above.)
>   
> -	     Even on PPC it must be zero-aligned at least for MINPAGESIZE.  */
> +     Even on PPC it must be zero-aligned at least for MINPAGESIZE.  */
>   
> -	  l_addr = l_dynaddr - dynaddr;
> +  l_addr = l_dynaddr - dynaddr;
>   
> -	  if ((l_addr & (minpagesize - 1)) == 0
> -	      && (l_addr & align) == ((l_dynaddr - dynaddr) & align))
> -	    {
> -	      if (info_verbose)
> -		gdb_printf (_("Using PIC (Position Independent Code) "
> -			      "prelink displacement %s for \"%s\".\n"),
> -			    paddress (current_inferior ()->arch (), l_addr),
> -			    so.name.c_str ());
> -	    }
> -	  else
> -	    {
> -	      /* There is no way to verify the library file matches.  prelink
> -		 can during prelinking of an unprelinked file (or unprelinking
> -		 of a prelinked file) shift the DYNAMIC segment by arbitrary
> -		 offset without any page size alignment.  There is no way to
> -		 find out the ELF header and/or Program Headers for a limited
> -		 verification if it they match.  One could do a verification
> -		 of the DYNAMIC segment.  Still the found address is the best
> -		 one GDB could find.  */
> -
> -	      warning (_(".dynamic section for \"%s\" "
> -			 "is not at the expected address "
> -			 "(wrong library or version mismatch?)"),
> -			 so.name.c_str ());
> -	    }
> -	}
> -
> -    set_addr:
> -      li.l_addr = l_addr;
> -      li.l_addr_p = 1;
> +  if ((l_addr & (minpagesize - 1)) == 0
> +      && (l_addr & align) == ((l_dynaddr - dynaddr) & align))
> +    {
> +      if (info_verbose)
> +	gdb_printf (_("Using PIC (Position Independent Code) "
> +		      "prelink displacement %s for \"%s\".\n"),
> +		    paddress (current_inferior ()->arch (), l_addr),
> +		    so.name.c_str ());
> +    }
> +  else
> +    {
> +      /* There is no way to verify the library file matches.  prelink
> +	 can during prelinking of an unprelinked file (or unprelinking
> +	 of a prelinked file) shift the DYNAMIC segment by arbitrary
> +	 offset without any page size alignment.  There is no way to
> +	 find out the ELF header and/or Program Headers for a limited
> +	 verification if it they match.  One could do a verification
> +	 of the DYNAMIC segment.  Still the found address is the best
> +	 one GDB could find.  */
> +
> +      warning (_(".dynamic section for \"%s\" "
> +		 "is not at the expected address "
> +		 "(wrong library or version mismatch?)"),
> +	       so.name.c_str ());
>       }
>   
> -  return li.l_addr;
> +  return set_addr (l_addr);
>   }
>   
>   struct svr4_so
> 
> base-commit: 0855b93cfb911a4feab76a31bb1914982b3e6979

Re: [PATCH] [gdb] Refactor svr4_solib_ops::lm_addr_check

[Guinevere Larsen]

On 9/11/26 4:19 PM, Tom de Vries wrote:
> I came across svr4_solib_ops::lm_addr_check and found it hard to follow.
>
> In particular, it's indent-happy and minimizes code using a goto, resulting in
> the following end-of-function:
> ...
>              }
>          }
>
>      set_addr:
>        li.l_addr = l_addr;
>        li.l_addr_p = 1;
>      }
>
>    return li.l_addr;
> }
> ...
>
> I could just address the indentation, but I'd be adding a goto:
> ...
> -      if (dynaddr + l_addr != l_dynaddr)
> +      if (dynaddr + l_addr == l_dynaddr)
> +        goto set_addr;
> ...
>
> Instead, refactor the function into this form:
> ...
> svr4_solib_ops::lm_addr_check (const solib &so, bfd *abfd) const
> {
>    auto &li = get_lm_info_svr4 (so);
>    if (li.l_addr_p)
>      return li.l_addr;
>
>    auto set_addr = [&li] (CORE_ADDR addr)
>    {
>      li.l_addr = addr;
>      li.l_addr_p = true;
>      return addr;
>    }
>
>    if (...)
>      return set_addr (l_addr);
>
>    ...
>
>    return set_addr (l_addr);
> }
> ...
> ---

Hi Tom!

Thanks for doing this! The final result is indeed much simpler to read, 
and the refactor is quite straight-forward.

Reviewed-By: Guinevere Larsen <guinevere@redhat.com>

Hopefully this gets approved soon!

-- 
Cheers,
Guinevere Larsen
it/its
she/her (deprecated)

>   gdb/solib-svr4.c | 160 +++++++++++++++++++++++------------------------
>   1 file changed, 80 insertions(+), 80 deletions(-)
>
> diff --git a/gdb/solib-svr4.c b/gdb/solib-svr4.c
> index c4af1b11a4d..40b0fc55113 100644
> --- a/gdb/solib-svr4.c
> +++ b/gdb/solib-svr4.c
> @@ -225,106 +225,106 @@ CORE_ADDR
>   svr4_solib_ops::lm_addr_check (const solib &so, bfd *abfd) const
>   {
>     auto &li = get_lm_info_svr4 (so);
> +  if (li.l_addr_p)
> +    return li.l_addr;
>   
> -  if (!li.l_addr_p)
> -    {
> -      struct bfd_section *dyninfo_sect;
> -      CORE_ADDR l_addr, l_dynaddr, dynaddr;
> +  auto set_addr = [&li] (CORE_ADDR addr)
> +  {
> +    li.l_addr = addr;
> +    li.l_addr_p = true;
> +    return addr;
> +  };
>   
> -      l_addr = li.l_addr_inferior;
> +  struct bfd_section *dyninfo_sect;
> +  CORE_ADDR l_addr, l_dynaddr, dynaddr;
>   
> -      if (!abfd || !this->has_lm_dynamic_from_link_map ())
> -	goto set_addr;
> +  l_addr = li.l_addr_inferior;
> +  if (!abfd || !this->has_lm_dynamic_from_link_map ())
> +    return set_addr (l_addr);
>   
> -      l_dynaddr = li.l_ld;
> +  l_dynaddr = li.l_ld;
>   
> -      dyninfo_sect = bfd_get_section_by_name (abfd, ".dynamic");
> -      if (dyninfo_sect == NULL)
> -	goto set_addr;
> +  dyninfo_sect = bfd_get_section_by_name (abfd, ".dynamic");
> +  if (dyninfo_sect == NULL)
> +    return set_addr (l_addr);
>   
> -      dynaddr = bfd_section_vma (dyninfo_sect);
> +  dynaddr = bfd_section_vma (dyninfo_sect);
> +  if (dynaddr + l_addr == l_dynaddr)
> +    return set_addr (l_addr);
>   
> -      if (dynaddr + l_addr != l_dynaddr)
> -	{
> -	  CORE_ADDR align = 0x1000;
> -	  CORE_ADDR minpagesize = align;
> +  CORE_ADDR align = 0x1000;
> +  CORE_ADDR minpagesize = align;
>   
> -	  if (bfd_get_flavour (abfd) == bfd_target_elf_flavour)
> -	    {
> -	      Elf_Internal_Ehdr *ehdr = elf_tdata (abfd)->elf_header;
> -	      Elf_Internal_Phdr *phdr = elf_tdata (abfd)->phdr;
> -	      int i;
> +  if (bfd_get_flavour (abfd) == bfd_target_elf_flavour)
> +    {
> +      Elf_Internal_Ehdr *ehdr = elf_tdata (abfd)->elf_header;
> +      Elf_Internal_Phdr *phdr = elf_tdata (abfd)->phdr;
> +      int i;
>   
> -	      align = 1;
> +      align = 1;
>   
> -	      for (i = 0; i < ehdr->e_phnum; i++)
> -		if (phdr[i].p_type == PT_LOAD && phdr[i].p_align > align)
> -		  align = phdr[i].p_align;
> +      for (i = 0; i < ehdr->e_phnum; i++)
> +	if (phdr[i].p_type == PT_LOAD && phdr[i].p_align > align)
> +	  align = phdr[i].p_align;
>   
> -	      minpagesize = get_elf_backend_data (abfd)->minpagesize;
> -	    }
> +      minpagesize = get_elf_backend_data (abfd)->minpagesize;
> +    }
>   
> -	  /* Turn it into a mask.  */
> -	  align--;
> +  /* Turn it into a mask.  */
> +  align--;
>   
> -	  /* If the changes match the alignment requirements, we
> -	     assume we're using a core file that was generated by the
> -	     same binary, just prelinked with a different base offset.
> -	     If it doesn't match, we may have a different binary, the
> -	     same binary with the dynamic table loaded at an unrelated
> -	     location, or anything, really.  To avoid regressions,
> -	     don't adjust the base offset in the latter case, although
> -	     odds are that, if things really changed, debugging won't
> -	     quite work.
> +  /* If the changes match the alignment requirements, we
> +     assume we're using a core file that was generated by the
> +     same binary, just prelinked with a different base offset.
> +     If it doesn't match, we may have a different binary, the
> +     same binary with the dynamic table loaded at an unrelated
> +     location, or anything, really.  To avoid regressions,
> +     don't adjust the base offset in the latter case, although
> +     odds are that, if things really changed, debugging won't
> +     quite work.
>   
> -	     One could expect more the condition
> -	       ((l_addr & align) == 0 && ((l_dynaddr - dynaddr) & align) == 0)
> -	     but the one below is relaxed for PPC.  The PPC kernel supports
> -	     either 4k or 64k page sizes.  To be prepared for 64k pages,
> -	     PPC ELF files are built using an alignment requirement of 64k.
> -	     However, when running on a kernel supporting 4k pages, the memory
> -	     mapping of the library may not actually happen on a 64k boundary!
> +     One could expect more the condition
> +     ((l_addr & align) == 0 && ((l_dynaddr - dynaddr) & align) == 0)
> +     but the one below is relaxed for PPC.  The PPC kernel supports
> +     either 4k or 64k page sizes.  To be prepared for 64k pages,
> +     PPC ELF files are built using an alignment requirement of 64k.
> +     However, when running on a kernel supporting 4k pages, the memory
> +     mapping of the library may not actually happen on a 64k boundary!
>   
> -	     (In the usual case where (l_addr & align) == 0, this check is
> -	     equivalent to the possibly expected check above.)
> +     (In the usual case where (l_addr & align) == 0, this check is
> +     equivalent to the possibly expected check above.)
>   
> -	     Even on PPC it must be zero-aligned at least for MINPAGESIZE.  */
> +     Even on PPC it must be zero-aligned at least for MINPAGESIZE.  */
>   
> -	  l_addr = l_dynaddr - dynaddr;
> +  l_addr = l_dynaddr - dynaddr;
>   
> -	  if ((l_addr & (minpagesize - 1)) == 0
> -	      && (l_addr & align) == ((l_dynaddr - dynaddr) & align))
> -	    {
> -	      if (info_verbose)
> -		gdb_printf (_("Using PIC (Position Independent Code) "
> -			      "prelink displacement %s for \"%s\".\n"),
> -			    paddress (current_inferior ()->arch (), l_addr),
> -			    so.name.c_str ());
> -	    }
> -	  else
> -	    {
> -	      /* There is no way to verify the library file matches.  prelink
> -		 can during prelinking of an unprelinked file (or unprelinking
> -		 of a prelinked file) shift the DYNAMIC segment by arbitrary
> -		 offset without any page size alignment.  There is no way to
> -		 find out the ELF header and/or Program Headers for a limited
> -		 verification if it they match.  One could do a verification
> -		 of the DYNAMIC segment.  Still the found address is the best
> -		 one GDB could find.  */
> -
> -	      warning (_(".dynamic section for \"%s\" "
> -			 "is not at the expected address "
> -			 "(wrong library or version mismatch?)"),
> -			 so.name.c_str ());
> -	    }
> -	}
> -
> -    set_addr:
> -      li.l_addr = l_addr;
> -      li.l_addr_p = 1;
> +  if ((l_addr & (minpagesize - 1)) == 0
> +      && (l_addr & align) == ((l_dynaddr - dynaddr) & align))
> +    {
> +      if (info_verbose)
> +	gdb_printf (_("Using PIC (Position Independent Code) "
> +		      "prelink displacement %s for \"%s\".\n"),
> +		    paddress (current_inferior ()->arch (), l_addr),
> +		    so.name.c_str ());
> +    }
> +  else
> +    {
> +      /* There is no way to verify the library file matches.  prelink
> +	 can during prelinking of an unprelinked file (or unprelinking
> +	 of a prelinked file) shift the DYNAMIC segment by arbitrary
> +	 offset without any page size alignment.  There is no way to
> +	 find out the ELF header and/or Program Headers for a limited
> +	 verification if it they match.  One could do a verification
> +	 of the DYNAMIC segment.  Still the found address is the best
> +	 one GDB could find.  */
> +
> +      warning (_(".dynamic section for \"%s\" "
> +		 "is not at the expected address "
> +		 "(wrong library or version mismatch?)"),
> +	       so.name.c_str ());
>       }
>   
> -  return li.l_addr;
> +  return set_addr (l_addr);
>   }
>   
>   struct svr4_so
>
> base-commit: 0855b93cfb911a4feab76a31bb1914982b3e6979

Re: [PATCH] [gdb] Refactor svr4_solib_ops::lm_addr_check

[Tom de Vries]

On 9/28/26 4:16 PM, Guinevere Larsen wrote:
> Hi Tom!
> 
> Thanks for doing this! The final result is indeed much simpler to read, 
> and the refactor is quite straight-forward.
> 
> Reviewed-By: Guinevere Larsen <guinevere@redhat.com>
> 
> Hopefully this gets approved soon!

Hi Guinevere,

thanks for the review.

I think for a refactoring patch your review (as well as a Claude Code 
one) is sufficient, so I've pushed this.

Thanks,
- Tom