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

[Guinevere Larsen <guinevere@redhat.com>]

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