Re: [PATCHv6 2/4] gdb: introduce program_space::get_entry_point_info function

[Simon Marchi <simark@simark.ca>]

Some comments below, I think they are pretty straightfowrard, so with
those fixed (and if you don't end up making more major changes):

Approved-By: Simon Marchi <simon.marchi@efficios.com>

On 7/10/26 10:24 AM, Andrew Burgess wrote:
> diff --git a/gdb/progspace.c b/gdb/progspace.c
> index 0e8516f4640..26e16cae8d0 100644
> --- a/gdb/progspace.c
> +++ b/gdb/progspace.c
> @@ -26,6 +26,7 @@
>  #include <algorithm>
>  #include "cli/cli-style.h"
>  #include "observable.h"
> +#include "arch-utils.h"
>  
>  /* The last program space number assigned.  */
>  static int last_program_space_num = 0;
> @@ -288,6 +289,55 @@ program_space::exec_entry_point_address () const
>  
>  /* See progspace.h.  */
>  
> +program_space::entry_point_info
> +program_space::get_entry_point_info () const
> +{
> +  std::optional<CORE_ADDR> exec_entry_address
> +    = this->exec_entry_point_address_if_available ();
> +
> +  std::optional<CORE_ADDR> inferior_entry_address;
> +  if (m_solib_ops != nullptr)
> +    inferior_entry_address = m_solib_ops->inferior_entry_point_address ();
> +
> +  return entry_point_info (std::move (inferior_entry_address),
> +			   std::move (exec_entry_address));

I wouldn't std::move those (I think it's the same as cpying on this
type).

> diff --git a/gdb/progspace.h b/gdb/progspace.h
> index 1ae1e42f3bb..d75379fe621 100644
> --- a/gdb/progspace.h
> +++ b/gdb/progspace.h
> @@ -323,6 +323,54 @@ struct program_space
>      return m_target_sections;
>    }
>  
> +  /* Class for tracking two possible entry points that an inferior might
> +     have, the entry point for the entire inferior, and the entry point
> +     within the main executable.  */
> +  struct entry_point_info

I think it would be preferable to define this class outside of struct
program_space.  I think that having (unnecessarily) nested classes just
makes them harder to read.

> +  {
> +    explicit entry_point_info (std::optional<CORE_ADDR> inferior_entry_address,

You can drop explicit, as the constructor has two parameters.

> +			       std::optional<CORE_ADDR> exec_entry_address)
> +      : m_inferior_entry_address (std::move (inferior_entry_address)),
> +	m_exec_entry_address (std::move (exec_entry_address))

I'm not sure it helps to std::move optional<CORE_ADDR>.

> +    { /* Nothing.  */ }
> +
> +    DISABLE_COPY_AND_ASSIGN (entry_point_info);
> +
> +    /* The entry address within the inferior as a whole.  See the member
> +       variable definition below for more details.  */
> +    const std::optional<CORE_ADDR> &inferior_entry_address () const
> +    {
> +      return m_inferior_entry_address;
> +    }
> +
> +    /* The entry address within the main executable.  See the member
> +       variable definition below for more details.  */
> +    const std::optional<CORE_ADDR> &exec_entry_address () const
> +    {
> +      return m_exec_entry_address;
> +    }

std::optional<CORE_ADDR> is small enough (16 bytes) that I would return
it by value.

> diff --git a/gdb/solib-svr4.c b/gdb/solib-svr4.c
> index 8e3de4d3ea1..12f3e783744 100644
> --- a/gdb/solib-svr4.c
> +++ b/gdb/solib-svr4.c
> @@ -3777,6 +3777,91 @@ svr4_solib_ops::get_solibs_in_ns (int nsid) const
>    return ns_solibs;
>  }
>  
> +/* 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 ();
> +
> +  /* For a dynamically linked executable the inferior's true entry point
> +     is the entry point of the dynamic linker.  We find this using the
> +     AT_BASE auxiliary vector entry, which gives the dynamic linker's
> +     load address, combined with e_entry address pulled from the
> +     inferior.  We assume that the ELF header can be read from AT_BASE.  */
> +  CORE_ADDR at_base_addr;
> +  if (target_auxv_search (AT_BASE, &at_base_addr) <= 0)
> +    return {};
> +
> +  /* Determine ELF architecture type.  Use the size of a program header
> +     entry to determine which ELF header we can expect to find.  */
> +  size_t e_entry_offset = 0;
> +  size_t e_entry_size = 0;
> +  CORE_ADDR at_phent;
> +  if (target_auxv_search (AT_PHENT, &at_phent) <= 0)
> +    return {};
> +  if (at_phent == sizeof (Elf32_External_Phdr))
> +    {
> +      e_entry_offset = offsetof (Elf32_External_Ehdr, e_entry);
> +      e_entry_size = 4;
> +    }
> +  else if (at_phent == sizeof (Elf64_External_Phdr))
> +    {
> +      e_entry_offset = offsetof (Elf64_External_Ehdr, e_entry);
> +      e_entry_size = 8;
> +    }
> +  else
> +    return {};
> +
> +  /* Architecture of the current inferior.  */
> +  gdbarch *gdbarch = current_inferior ()->arch ();
> +
> +  /* Read the entry address from the ELF header at AT_BASE_ADDR.  */
> +  CORE_ADDR e_entry;
> +  gdb_byte buffer[sizeof (CORE_ADDR)];
> +  if (target_read_memory (at_base_addr + e_entry_offset, buffer, e_entry_size))
> +    return {};
> +  bfd_endian byte_order = gdbarch_byte_order (gdbarch);
> +  e_entry = extract_unsigned_integer (buffer, e_entry_size, byte_order);
> +
> +  /* Ensure AT_BASE_ADDR has proper sign in its possible upper bits so
> +     that `+ at_base_addr' will overflow CORE_ADDR width not creating
> +     invalid addresses like 0x101234567 for 32bit inferiors on 64bit
> +     GDB.  */
> +  int addr_bit = gdbarch_addr_bit (gdbarch);
> +  if (addr_bit < (sizeof (CORE_ADDR) * HOST_CHAR_BIT))
> +    {
> +      CORE_ADDR space_size = (CORE_ADDR) 1 << addr_bit;
> +
> +      gdb_assert (at_base_addr < space_size);
> +
> +      /* E_ENTRY exceeding SPACE_SIZE would be for prelinked
> +	 64bit ld.so with 32bit executable, it should not happen.  */
> +      if (e_entry < space_size
> +	  && e_entry + at_base_addr >= space_size)
> +	at_base_addr -= space_size;
> +    }
> +
> +  /* Compute the entry address.  */
> +  e_entry = at_base_addr + e_entry;
> +
> +  /* Handle the case where E_ENTRY is a function descriptor.  Also remove
> +     any non-address (e.g. tag) bits from E_ENTRY.  */
> +  e_entry
> +    = gdbarch_convert_from_func_ptr_addr (gdbarch, e_entry,
> +					  current_inferior ()->top_target ());
> +  e_entry
> +    = gdbarch_addr_bits_remove (gdbarch, e_entry);
> +
> +  return e_entry;
> +}

Is there some code to be shared with svr4_solib_ops::enable_break,
regarding fetching AT_BASE and the checks / post processing that
follows?

> diff --git a/gdb/testsuite/gdb.base/bt-after-starti.exp b/gdb/testsuite/gdb.base/bt-after-starti.exp
> new file mode 100644
> index 00000000000..c2a78a05cf4
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/bt-after-starti.exp
> @@ -0,0 +1,215 @@
> +# Copyright 2026 Free Software Foundation, Inc.
> +#
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 3 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +
> +# Check that 'bt' from the very first instruction (where 'starti'
> +# stops GDB) doesn't display any unexpected frames.
> +
> +require !use_gdb_stub

I think you used this because of the use of the gdb_starti_cmd proc.  It
might be possible to make it work for native-gdbserver: after
connection, the inferior should be at the very first instruction.  But
coverage with native-extended-gdbserver is perhaps sufficient too.

> diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
> index a19e8ba6729..085f098fd6e 100644
> --- a/gdb/testsuite/lib/gdb.exp
> +++ b/gdb/testsuite/lib/gdb.exp
> @@ -4217,6 +4217,45 @@ proc is_aarch64_target {} {
>      return [expr {![is_aarch32_target]}]
>  }
>  
> +# Return true for svr4 targets, otherwise, return false.
> +
> +proc is_svr4_target {} {
> +    return [expr {[istarget *-linux*] || [istarget *-freebsd*]
> +		  || [istarget *-netbsd*] || [istarget *-openbsd*]
> +		  || [istarget *-solaris*] || [istarget *-gnu]}]
> +}
> +
> +# Return false for targets that don't support finding the whole
> +# process entry address, otherwise, return true.

I find it surprising to read that you use the negative first, it would
be more typical to read:

# Return true for targets that support finding the whole process entry
# address, otherwise, return false.

> +
> +proc supports_process_entry_point {} {
> +    # SVR4 targets support finding the entry point.  This is done
> +    # within GDB so will work even for remote targets.
> +    if {[is_svr4_target]} {
> +	return true
> +    }
> +
> +    # Windows and Darwin don't currently support this.
> +    if {[istarget *-*-mingw*]
> +	|| [istarget *-*-cygwin*]
> +	|| [istarget *-*-pe*]
> +	|| [istarget *-*-darwin*]} {
> +	return false
> +    }
> +
> +    # For remote targets there is no RSP packet to retrieve the entry
> +    # point, so this won't work unless the solib code can handle this
> +    # within GDB, see the svr4 check above.
> +    if {[gdb_protocol_is_remote]} {
> +	return false
> +    }
> +
> +    # Assume everything else supports this by default.  If a test
> +    # fails because we get here then either fix GDB to support this
> +    # feature, or add a new deny list entry above.
> +    return true
> +}

I can't verify the logic here, but from the comments it sounds fine.

Simon