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

["Metzger, Markus T" <markus.t.metzger@intel.com>]

>>>>  <!-- 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?

GDB silently ignores unknown elements and unknown attributes.

I will drop most of this patch and just send the new in-memory-library
response without opt-in and without fallback.


>>> 	      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?

No, 'info shared' prints the name given by

  /* Constructor.  BASE and SIZE define where the BFD can be found in
     target memory.  */
  target_buffer (CORE_ADDR base, ULONGEST size)
    : m_base (base),
      m_size (size),
      m_filename (xstrprintf ("<in-memory@%s-%s>",
			      core_addr_to_string_nz (m_base),
			      core_addr_to_string_nz (m_base + m_size)))

This is existing code.  I'm no longer setting solib->name for in-memory libs.


>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".

We'd need to route that through.  I have not looked into it.  I'm quite happy
with the name GDB chooses.  It makes it easy to copy&paste to a 'dump
memory' command.


>>>> +      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.

I'd leave the assert here.  This is where we need that property asserted.


>>>> +/* 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).

OK.

Regards,
Markus.
Intel Deutschland GmbH
Registered Address: Dornacher Straße 1, 85622 Feldkirchen, Germany
Tel: +49 89 991 430, www.intel.de
Managing Directors: Harry Demas, Jeffrey Schneiderman, Yin Chong Sorrell
Chairperson of the Supervisory Board: Nicole Lau
Registered Seat: Munich
Commercial Register: Amtsgericht München HRB 186928