Re: [PATCHv2 2/3] gdb: introduce program_space::get_entry_point_info function

[Andrew Burgess <aburgess@redhat.com>]

Pedro Alves <pedro@palves.net> writes:

> On 2026-06-11 22:59, Andrew Burgess wrote:
>
>>  
>> +/* See solib.h.  */
>> +
>> +std::optional<CORE_ADDR>
>> +svr4_solib_ops::inferior_entry_point_address () const
>> +{
>> +  std::optional<gdb::byte_vector> interp_name_holder
>> +    = svr4_find_program_interpreter ();
>> +
>> +  /* No interpreter means this is a static executable.  Ask the
>> +     program_space for the entry address within the main executable.  */
>> +  if (!interp_name_holder.has_value ())
>> +    return m_pspace->exec_entry_point_address_if_available ();
>> +
>> +  /* If we can find a solib that matches the interpreter name then we can
>> +     use that to find the actual entry address of the inferior.  */
>> +  const solib *interp_solib = nullptr;
>> +  const char *interp_name = (const char *) interp_name_holder->data ();
>> +  for (const solib &so : m_pspace->solibs ())
>> +    {
>> +      if (svr4_same_name (interp_name, so.original_name.c_str ()))
>> +	{
>> +	  interp_solib = &so;
>> +
>> +	  const objfile *objfile = so.objfile;
>> +	  if (objfile != nullptr && objfile->per_bfd->ei.entry_point_p)
>> +	    {
>> +	      const entry_info &ei = objfile->per_bfd->ei;
>> +	      return (ei.entry_point
>> +		      + objfile->section_offsets[ei.the_bfd_section_index]);
>> +	    }
>> +	}
>> +    }
>> +
>> +  gdb_bfd_ref_ptr tmp_bfd;
>> +  target_ops_up tmp_bfd_target;
>> +  try
>> +    {
>> +      tmp_bfd = solib_bfd_open (interp_name);
>> +
>> +      /* Failed to open the interpreter BFD.  */
>> +      if (tmp_bfd == nullptr)
>> +	return {};
>> +
>> +      tmp_bfd_target = target_bfd_reopen (tmp_bfd);
>> +    }
>> +  catch (const gdb_exception &ex)
>> +    {
>
> Swallows Ctrl-C.  (I see you're just copying this from elsewhere.)
>
>> +      return {};
>> +    }
>> +
>> +  CORE_ADDR entry_addr
>> +    = exec_entry_point (tmp_bfd.get (), tmp_bfd_target.get ());
>> +
>> +  /* We found the solib for the interpreter, but we don't have a
>> +     corresponding objfile, or the objfile doesn't have entry point
>> +     information.  Maybe symbols haven't been loaded yet?  */
>> +  if (interp_solib != nullptr)
>> +    {
>> +      gdb_assert (interp_solib->objfile == nullptr
>> +		  || !interp_solib->objfile->per_bfd->ei.entry_point_p);
>> +
>> +      CORE_ADDR load_addr
>> +	= this->lm_addr_check (*interp_solib, tmp_bfd.get ());
>> +
>> +      return load_addr + entry_addr;
>> +    }
>> +
>> +  /* We could not find a solib corresponding to the interpreter, this might
>> +     mean that the name didn't match for some reason, or maybe GDB has
>> +     failed to create a solib due to some other issue reading the solib
>> +     list from the inferior.
>> +
>> +     If we got here we have managed to open the expected interpreter on
>> +     disk though, so we can try using the AT_BASE value.  */
>> +  CORE_ADDR load_addr;
>> +  if (target_auxv_search (AT_BASE, &load_addr) > 0)
>> +    {
>> +      int addr_bit = gdbarch_addr_bit (current_inferior ()->arch ());
>> +
>> +      /* Ensure LOAD_ADDR has proper sign in its possible upper bits so
>> +	 that `+ load_addr' will overflow CORE_ADDR width not creating
>> +	 invalid addresses like 0x101234567 for 32bit inferiors on 64bit
>> +	 GDB.  */
>> +      if (addr_bit < (sizeof (CORE_ADDR) * HOST_CHAR_BIT))
>> +	{
>> +	  CORE_ADDR space_size = (CORE_ADDR) 1 << addr_bit;
>> +
>> +	  gdb_assert (load_addr < space_size);
>> +
>> +	  /* ENTRY_ADDR exceeding SPACE_SIZE would be for prelinked
>> +	     64bit ld.so with 32bit executable, it should not happen.  */
>> +	  if (entry_addr < space_size
>> +	      && entry_addr + load_addr >= space_size)
>> +	    load_addr -= space_size;
>> +	}
>> +
>> +      return load_addr + entry_addr;
>> +    }
>
> I wonder whether just looking at AT_BASE, which points at an ELF, and then
> adding elf_header->e_entr, wouldn't be simpler than all of this.
> We already use Elf32_External/Elf64_External structs in the file, so maybe
> the manual ELF peeking is OK.
>
> Alternatively, of lot of that code looks similar to enable_break.  Maybe
> it could be refactored to be shared.  Or enable_break could cache the 
> entry address.
>
>
>> +# Use 'readelf' to extract the entry point address from TESTFILE.
>> +proc get_exec_entry_address { testfile } {
>> +    set readelf_program [gdb_find_readelf]
>> +    set command "exec $readelf_program -Wh [standard_output_file $testfile]"
>
> If you use "objdump -f" instead, this has a better chance of working on Windows PE/COFF too (eventually):
>
>  $ objdump.exe -f ./foo.exe
>
>  ./foo.exe:     file format pei-x86-64
>  architecture: i386:x86-64, flags 0x0000013b:
>  HAS_RELOC, EXEC_P, HAS_DEBUG, HAS_SYMS, HAS_LOCALS, D_PAGED
>  start address 0x00000001400013f0
>> +# Start TESTFILE using 'starti'.  Check if the 'backtrace' command
>> +# prints any additional frames; we don't expect any.
>> +#
>> +# When IS_DYNAMIC is true then TESTFILE is a dynamically linked
>> +# executable, otherwise TESTFILE is statically linked.
>> +proc run_test { testfile is_dynamic } {
>> +    clean_restart $testfile
>> +
>> +    set exe_entry_addr [get_exec_entry_address $testfile]
>> +
>> +    # The inferior entry address is always unknown before starting the
>> +    # inferior as at the point GDB has no solib_ops, and it is only
>> +    # through that that we figure out the inferior entry address.
>> +    check_maint_info_entry_addr "<unknown>" $exe_entry_addr \
>> +	"check 'maint info entry-address' before inferior starts"
>> +
>> +    # Start inferior with 'starti' and then wait for a prompt.
>> +    gdb_starti_cmd
>> +    gdb_test "" ".*" "prompt after starti"
>> +
>> +    # Only svr4 targets currently support querying the inferior entry
>> +    # address.
>> +    if {[istarget *-linux*]} {
>
> There are more svr4 targets than linux.  I think this style of allow-list has
> a good chance of never getting updated.  Deny-lists are better, IMHO.  If the
> test fails on some port, that might trigger someone to add the feature
> there.

Hey Pedro,

I'm still looking at your other feedback, but before I start making this
specific change, I just wanted to clarify how you see this as being
different from what I have right now.

If I write this as a deny list, e.g.

 if { ![istarget ....] && ![istarget ....] } {
   # Run tests.
 }

Is there not the same problem?  We rely on someone realising that the
reason the test isn't run on their target is some missing GDB
functionality, and them adding the functionality and removing the
`istarget` block for their target.

Another option would be for me to just not add any test skipping right
now.  I know this will lead to the test failing for some targets as the
new feature is only implemented for svr4 targets.  Then, if someone on a
failing target cares, they can add the missing feature.  This approach
feels a bit mean, I don't like adding failing tests for others, but it
does draw attention to the problem.

I wonder if a better approach would to stick with my allow list, but add
an 'unsupported' call in the else block.  Maybe someone on e.g. Windows,
will periodically look at tests that report 'unsupported' and see if
there is anything that could be done?  If 'unsupported' is the wrong
type then I can use whatever you prefer.

I agree with your feedback on svr4 being wider than just Linux, I'll add
a helper function for `is_svr4_target` to lib/gdb.exp and then make use
of that, the svr4 list can then be improved over time.

Thanks,
Andrew