Re: [PATCH v3 09/44] gdb, gdbserver, ze: in-memory libraries

[Simon Marchi <simark@simark.ca>]

On 2025-08-01 05:37, Tankut Baris Aktemur wrote:
> From: Markus Metzger <markus.t.metzger@intel.com>
> 
> For Intel GPU devices, device libraries live in the host memory and are
> loaded onto the device from there.
> 
> Add support for reporting such in-memory shared libraries via
> 
>     qXfer:libraries:read
> 
> and have GDB read them from target memory.
> 
> Reviewed-By: Eli Zaretskii <eliz@gnu.org>

From the ROCm point of view, it seems like this might be useful if we
ever do remote debugging, since we do have these in-memory "shared
libraries".

> @@ -48309,6 +48313,10 @@ depend on the library's link-time base addresses.
>  @value{GDBN} must be linked with the Expat library to support XML
>  library lists.  @xref{Expat}.
>  
> +@value{GDBN} indicates support for in-memory library elements by
> +supplying the @code{qXfer:libraries:read:in-memory-library+}
> +@samp{qSupported} feature (@pxref{qSupported}).
> +
>  A simple memory map, with one loaded library relocated by a single
>  offset, looks like this:
>  

Should there be an entry in the "The following values of gdbfeature
(for the packet sent by GDB) are defined:" section of the qSupported
doc?  I'm a bit confused, because it seems like some values that GDB
sends along with qSupported are not listed there.  Perhaps it's an
oversight and they should?

> diff --git a/gdb/features/library-list.dtd b/gdb/features/library-list.dtd
> index 66945cbe97c13cfac5a4d00e9a752ccd8419252f..baa01485af950c58e9e242229365501c043e2fe1 100644
> --- a/gdb/features/library-list.dtd
> +++ b/gdb/features/library-list.dtd
> @@ -5,12 +5,16 @@
>       notice and this notice are preserved.  -->
>  
>  <!-- library-list: Root element with versioning -->
> -<!ELEMENT library-list  (library)*>
> -<!ATTLIST library-list  version CDATA   #FIXED  "1.0">
> +<!ELEMENT library-list  (library | in-memory-library)*>
> +<!ATTLIST library-list  version CDATA   #FIXED  "1.1">

Hmm, looking at the history of this file and other .dtd files, we added
attributes and elements in the past without bumping this version number.
I guess it's not needed because we do our own checking of what the other
side supports via the qSupported packet and adjust the behavior based on
that.  If not useful, I think we could keep 1.0 here for simplicity.

> @@ -246,10 +295,31 @@ target_solib_ops::current_sos () const
>    for (lm_info_target_up &info : library_list)
>      {
>        auto &new_solib = sos.emplace_back (*this);
> +      switch (info->location)
> +	{
> +	case lm_on_disk:
> +	  /* We don't need a copy of the name in INFO anymore.  */
> +	  new_solib.name = std::move (info->name);
> +	  new_solib.original_name = new_solib.name;
> +	  break;
> +
> +	case lm_in_memory:
> +	  if (info->end <= info->begin)
> +	    warning (_("bad in-memory-library location: begin=%s, end=%s"),
> +		     core_addr_to_string_nz (info->begin),
> +		     core_addr_to_string_nz (info->end));
> +	  else
> +	    {
> +	      new_solib.original_name = std::string ("in-memory-")
> +		+ core_addr_to_string_nz (info->begin)
> +		+ "-"
> +		+ core_addr_to_string_nz (info->end);

Align with parenthesis:

	      new_solib.original_name = (std::string ("in-memory-")
					 + core_addr_to_string_nz (info->begin)
					 + "-"
					 + core_addr_to_string_nz (info->end));

Just a note regarding ROCm.  We have a particular syntax for in-memory
objects, like this:

  memory://57663#offset=0x55555562b040&size=62488

It is documented here:

https://llvm.org/docs/AMDGPUUsage.html#loaded-code-object-path-uniform-resource-identifier-uri

Currently, solib-rocm.c generates solibs with that kind of name for
in-memory objects.  If we add proper support for solibs in memory, like
this patch does, we'll probably want to change solib-rocm to use that.
However, we'll want to make sure that "info shared" still shows the URIs
in the format shown above, at least for ROCm.  We can cross this bridge
when we get there, but we'll need to find a way.

> -      /* We don't need a copy of the name in INFO anymore.  */
> -      new_solib.name = std::move (info->name);
> -      new_solib.original_name = new_solib.name;
> +	      new_solib.begin = info->begin;
> +	      new_solib.end = info->end;

I think I'd like to see two solib constructors, one for on-disk and one
for memory.  That would make it clear what information is required in
each case.

> @@ -386,6 +456,14 @@ target_solib_ops::in_dynsym_resolve_code (CORE_ADDR pc) const
>    return in_plt_section (pc);
>  }
>  
> +gdb_bfd_ref_ptr
> +target_solib_ops::bfd_open_from_target_memory (CORE_ADDR addr,
> +					       CORE_ADDR size,
> +					       const char *target) const
> +{
> +  return gdb_bfd_open_from_target_memory (addr, size, target);
> +}

Would it make sense for this to be the default implementation
(solib_ops::bfd_open_from_target_memory)?

> @@ -488,8 +496,25 @@ solib_ops::bfd_open (const char *pathname) const
>  static int
>  solib_map_sections (solib &so)
>  {
> -  gdb::unique_xmalloc_ptr<char> filename (tilde_expand (so.name.c_str ()));
> -  gdb_bfd_ref_ptr abfd (so.ops ().bfd_open (filename.get ()));
> +  gdb_bfd_ref_ptr abfd;
> +  if (!so.name.empty ())
> +    {
> +      gdb::unique_xmalloc_ptr<char> filename (tilde_expand (so.name.c_str ()));
> +      abfd = so.ops ().bfd_open (filename.get ());
> +    }
> +  else if (so.begin != 0 && so.end != 0)
> +    {
> +      if (so.end <= so.begin)
> +	error (_("Bad address range [%s; %s) for in-memory shared library."),
> +	       core_addr_to_string_nz (so.begin),
> +	       core_addr_to_string_nz (so.end));

Sounds like we should never end up with an solib like this, I don't
think we need to handle this case here.

> +
> +      abfd = so.ops ().bfd_open_from_target_memory (so.begin,
> +						    so.end - so.begin,
> +						    gnutarget);
> +    }
> +  else
> +    internal_error (_("bad so_list"));

This error message could be a bit prettier.

> +/* Print a qXfer:libraries:read entry for DLL.  */
> +
> +static std::string
> +print_qxfer_libraries_entry (dll_info &dll)
> +{
> +  switch (dll.location)
> +    {
> +    case dll_info::in_memory:
> +      if (get_client_state ().in_memory_library_supported)
> +	return string_printf
> +	  ("  <in-memory-library begin=\"0x%s\" end=\"0x%s\">"
> +	   "<segment address=\"0x%s\"/></in-memory-library>\n",
> +	   paddress (dll.begin), paddress (dll.end),
> +	   paddress (dll.base_addr));
> +
> +      /* GDB does not support in-memory-library.  Fall back to storing it in a
> +	 temporary file and report that file to GDB.  */

Curious, do you actually need this fallback?  If not, could GDBserver
just not report these in-memory libraries if GDB is too old?  If we can
avoid the complexity...

> @@ -2772,6 +2896,7 @@ handle_query (char *own_buf, int packet_len, int *new_packet_len_p)
>  	  /* We do not have any hook to indicate whether the non-SVR4 target
>  	     backend supports qXfer:libraries:read, so always report it.  */
>  	  strcat (own_buf, ";qXfer:libraries:read+");
> +	  strcat (own_buf, ";qXfer:libraries:read:in-memory-library+");

So, this makes gdbserver reply "I support in memory libraries".  Is it
really needed?  It seems to me like it's only important for GDB to tell
gdbserver, not the other way around.

Simon