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

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

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.


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

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


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


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

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