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

[Simon Marchi <simark@simark.ca>]

On 12/12/25 6:20 AM, Metzger, Markus T wrote:
> Hello Simon,
> 
> Thanks for your review.
> 
>>> diff --git a/gdb/features/library-list.dtd b/gdb/features/library-list.dtd
>>> index
>> 66945cbe97c13cfac5a4d00e9a752ccd8419252f..baa01485af950c58e9e242229
>> 365501c043e2fe1 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.
> 
> This has been discussed with Thiago, as well in
> https://sourceware.org/pipermail/gdb-patches/2025-July/219545.html
> 
> Thiago thought that the version bump was necessary, but suggested bumping
> it once for the entire series and not for each patch that makes changes to it.
> 
> I'm fine, either way, but I wonder why we major.minor version if we then ignore it.

As far as I know, the GDB client doesn't even check these version
numbers, so I am not sure what bumping the version number would help
with.  It won't prevent an older GDB from trying to read a newer
backwards-incompatible version.

I see two possibilities:

1. Adding a new attribute or element is considered
   backwards-incompatible, in which case this version number should have
   been bumped a few times already.  But they haven't so... not sure
   it's worth starting now.

2. Adding a new attribute or element is considered backwards-compatible
   (an older client would just ignore them), so it's not necessary to
   bump the version.

Would it work to just make gdbserver return these in-memory-library
elements, without even a qSupported flag?  Would older GDBs just ignore
them?

>> 	      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.
> 
> The name that gets printed by 'info shared' is defined by the in-memory bfd.
> 
> I don't know where this name is used.   Looks like it is OK to not provide a name.

Ok, so "info shared" shows solib->name.  For in-memory solibs, you don't
set solib->name?  Does this mean that no name is shown in "info shared"
for these libraries?

As of today, even with this change, solib-rocm.c could set solib->name
to whatever it wants.  If we ever want to support remote debugging with
ROCm, we would want to find a way to get our `memory://...` string as
the name.

Do you think that the remote side could provide a name inside an
<in-memory-library> element?  A ROCm would put that `memory://...`
string.  But in general, it would be a place to put a user-friendly name
that will end up in "info shared".


> 
>>> +      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.
> 
> We're subtracting so.begin from so.end.  If you don't like the error, we
> should assert this.  The only place that currently adds such solibs ignores
> those with a warning, so asserting it should be fine.

Yes I think an assert makes sense here (or in an eventual solib
constructor that takes these two addresses).  Filtering for bad solib
data would always be done earlier than that.

>>> +/* 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...
> 
> Since support will be there from the beginning for our target, we don't
> actually need the fall-back.  A GDB that doesn't support this also wouldn't
> be able to support the IntelGT architecture.
> 
> I'm fine to drop it.

That would be my preference, otherwise it's code that will never
actually be used but will add maintenance cost.

>>> @@ -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.
> 
> Right, GDB needs to opt into this new response.
> 
> I thought it was common practice to announce features in gdbserver.
> If we only do this for opt-ins, I can drop this.

If GDB needs to know what gdbserver supports, it makes sense for
gdbserver to announce it, but I think it's only the other way around
in this case.

But really, if we can just add the new <in-memory-library> element and
it is gracefully ignored by older GDBs, without the need for a feature
flag, that would be the simplest solution (and therefore my preference).

Simon