RE: [PATCH v2 10/47] gdb, gdbserver, ze: in-memory libraries

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

Hello Thiago,

Thanks for your review.

>>  <!-- 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">
>
>Theoretically, this would cause the parser to reject an XML unless it
>has version="1.1", despite the fact that GDB is still able to parse 1.0
>documents.  Therefore, this should just be changed to CDATA #REQUIRED,
>and we can rely on library_list_start_list to check if the version is
>acceptable.
>
>I said "theoretically" and "would" above because I experimented with it
>(actually, with <library-list-svr4> which is what I can test on Linux)
>and GDB didn't reject an XML reply with a tag that's not present in the
>DTD, and also it didn't care about the value of the version
>attribute. The DTD is simply ignored.
>
>Looking around a bit, I found a comment¹ by an Expat maintainer
>saying:
>
>> The thing is: Expat is a non-validating XML parser.
>
>So I'm not sure what purpose the DTDs have in GDB. Perhaps they're just
>part of the documentation?
>
>If the DTD was enforced, it would be ok to keep version at 1.0 since an
>old GDB would reject a library-list document with in-memory-library
>elements because it wouldn't conform to the expected DTD. As it is
>though, it looks like the version bump is indeed necessary.

IIUC you want this to remain as it is.

>There's another version bump elsewhere in this series though, which I
>think isn't really needed in practice — will there ever be a version of
>GDB in the field which supports version 1.1 but not 1.2?

I don't expect that.  I'll remove the other bump and stay at 1.1.

>> @@ -246,10 +295,35 @@ solib_target_current_sos (void)
>>    for (lm_info_target_up &info : library_list)
>>      {
>>        auto &new_solib = sos.emplace_back ();
>> +      switch (info->location)
>> +	{
>> +	case lm_on_disk:
>> +	  /* We don't need a copy of the name in INFO anymore.  */
>> +	  new_solib.so_name = std::move (info->name);
>> +	  new_solib.so_original_name = new_solib.so_name;
>> +	  break;
>> +
>> +	case lm_in_memory:
>> +	  {
>> +	    if (info->end <= info->begin)
>> +	      error (_("bad in-memory-library location: begin=%s, end=%s"),
>> +		     core_addr_to_string_nz (info->begin),
>> +		     core_addr_to_string_nz (info->end));
>
>Is erroring out better than printing a warning and skipping the bad
>library entry?

Makes sense.  The rest would still work.

>> +	    /* Give it a name although this isn't really needed.  */
>> +	    std::string orig_name
>> +	      = std::string ("in-memory-")
>> +	      + core_addr_to_string_nz (info->begin)
>> +	      + "-"
>> +	      + core_addr_to_string_nz (info->end);
>> +
>> +	    new_solib.so_original_name = orig_name;
>
>I know less about C++ than I would like: would it be better to use
>std::move (orig_name) here, or can we rely on the compiler being smart
>enough to notice that it doesn't need orig_name after this line and
>avoid a copy? If you don't know either, I'd add an std::move just to be
>sure.

We don't really need that local variable.

>> @@ -40,6 +40,17 @@ loaded_dll (process_info *proc, const char *name,
>CORE_ADDR base_addr)
>>    proc->dlls_changed = true;
>>  }
>>
>> +/* Record a newly loaded in-memory DLL at BASE_ADDR for PROC.  */
>
>You should also document the BEGIN and END arguments. In particular, I'm
>interested in learning the difference between BEGIN and BASE_ADDR.

I documented the corresponding fields in struct dll_info.

The arguments of all those functions directly correspond to those fields.
If it is OK, I would not repeat the documentation.

>> +void
>> +loaded_dll (process_info *proc, CORE_ADDR begin, CORE_ADDR end,
>> +	    CORE_ADDR base_addr)
>> +{
>> +  gdb_assert (proc != nullptr);
>> +  proc->all_dlls.emplace_back (begin, end, base_addr);
>> +  proc->dlls_changed = true;
>> +}
>
>Even though both versions of loaded_dll are small, it's a bit
>unfortunate that they're almost identical. This is because of the choice
>to have two constructors for dll_info and the location member being
>initialized depending on which one is called. I understand that design
>choice, but given that it leads to this duplication of code I think it's
>better to have location explicit in the constructor (or a new
>constructor with location explicitly passed in it) and then one version
>of loaded_dll can simply call the other one (or both can call a third,
>internal loaded_dll version) with an explicit location argument.

It's not just the location.  They would also need to provide all the
arguments that are only relevant for the other location.

And if we ever added a third location type (a section in a fat binary,
maybe), we'd need to touch all that code again to add arguments
for that new type.

I could add a check_ack() function to contain

  if (need_ack && !get_client_state ().vack_library_supported)
    throw_error (NOT_SUPPORTED_ERROR,
		 _("library acknowledgement not supported."));

if that helps.

>> +void
>> +unloaded_dll (process_info *proc, CORE_ADDR begin, CORE_ADDR end,
>> +	      CORE_ADDR base_addr)
>> +{
>> +  gdb_assert (proc != nullptr);
>> +  auto pred = [&] (const dll_info &dll)
>> +    {
>> +      if (dll.location != dll_info::in_memory)
>> +	return false;
>> +
>> +      if (base_addr != UNSPECIFIED_CORE_ADDR
>> +	  && base_addr == dll.base_addr)
>> +	return true;
>> +
>> +      /* We do not require the end address to be specified - we don't
>> +	 support partially unloaded libraries, anyway.  */
>> +      if (begin != UNSPECIFIED_CORE_ADDR
>> +	  && begin == dll.begin
>> +	  && (end == UNSPECIFIED_CORE_ADDR
>> +	      || end == dll.end))
>> +	return true;
>> +
>> +      return false;
>> +    };
>> +
>> +  auto iter = std::find_if (proc->all_dlls.begin (), proc->all_dlls.end (),
>> +			    pred);
>> +
>> +  if (iter == proc->all_dlls.end ())
>> +    /* For some inferiors we might get unloaded_dll events without having
>> +       a corresponding loaded_dll.  In that case, the dll cannot be found
>> +       in ALL_DLL, and there is nothing further for us to do.  */
>> +    return;
>> +  else
>> +    {
>> +      /* DLL has been found so remove the entry and free associated
>> +	 resources.  */
>> +      proc->all_dlls.erase (iter);
>> +      proc->dlls_changed = 1;
>> +    }
>> +}
>
>Here the duplication problem between the two versions of unloaded_dll is
>worse. It's possible do define an unloaded_dll_1 internal function with
>the code above and getting an dll_info::location_t as argument that is
>called by both versions of unloaded_dll.

The predicate is different for the two versions.  I added a function that
takes the predicate as input.

>> +  location_t location;
>>    std::string name;
>> +  CORE_ADDR begin;
>
>Here too it would be nice to have a comment explaining the difference
>between begin and base_addr.

That's where I added the comments describing the fields.

>> +{
>> +  if (dll.end <= dll.begin)
>> +    error (_("bad in-memory-library location: begin=%s, end=%s"),
>> +	   core_addr_to_string_nz (dll.begin),
>> +	   core_addr_to_string_nz (dll.end));
>> +
>> +  gdb::byte_vector buffer (dll.end - dll.begin);
>> +  int errcode = gdb_read_memory (dll.begin, buffer.data (), buffer.size ());
>> +  if (errcode != 0)
>> +    error (_("failed to read in-memory library at %s..%s"),
>> +	   core_addr_to_string_nz (dll.begin),
>> +	   core_addr_to_string_nz (dll.end));
>> +
>> +  std::string name
>> +    = std::string ("gdb-in-memory-solib-")
>> +    + core_addr_to_string_nz (dll.begin)
>> +    + "-"
>> +    + core_addr_to_string_nz (dll.end);
>> +
>> +  gdb_file_up file = gdb_create_tmpfile (name);
>
>This is the only use of gdb_create_tmpfile. I think that instead of
>having files that will be deleted when gdbserver exits, it would be
>better if the files were deleted when they aren't necessary anymore.
>
>gdb_create_tmpfile could return not only a gdb_file_up, but also a
>gdb::unlinker (in an std::pair, I suppose). Then dll_to_tmpfile could
>add it to new member of dll_info with type
>std::optional<gdb::unlinker>. Thus, when the dll_info object is
>destroyed, the tmp file will be deleted. WDYT?
>
>Also, this could be a dll_info::to_tmpfile method instead of a separate
>function.

That sounds good and would make patch 9 obsolete.

>> +static std::string
>> +library_list_version_needed (const std::list<dll_info> &dlls)
>> +{
>> +  const client_state &cs = get_client_state ();
>> +  int major = 1, minor = 0;
>> +
>> +  for (const dll_info &dll : dlls)
>> +    {
>> +      switch (dll.location)
>> +	{
>> +	case dll_info::on_disk:
>> +	  major = std::max (major, 1);
>> +	  minor = std::max (minor, 0);
>> +	  break;
>> +
>> +	case dll_info::in_memory:
>> +	  if (cs.in_memory_library_supported)
>> +	    {
>> +	      major = std::max (major, 1);
>> +	      minor = std::max (minor, 1);
>> +	    }
>> +	  break;
>> +	}
>> +    }
>> +
>> +  return std::to_string (major) + std::string (".") + std::to_string (minor);
>> +}
>
>This makes the version depend on what the currnet list of dll_infos
>needs, not just on the features supported by GDB or gdbserver. If
>there's no in-memory library loaded, then the XML will be version
>1.0. If later an in-memory library is loaded, then the next XML will be
>version 1.1
>
>I expected the version to depend just on the features advertised by GDB
>and gdbserver, so I was a bit surprised. I don't think this is a
>problem, but I just mention here in case someone has an opinion about
>it.

This may be overengineered.  It allows gdbserver with in-memory library
support to work with older GDBs that do not support it.  For our use-case,
such a GDB would not support our architecture, anyway.  From a general
feature perspective, though, it may make sense.

If maintainers agree that we do not want this flexibility, all this tempfile
stuff will go away, simplifying the series a bit.

>> diff --git a/gdb/remote.c b/gdb/remote.c
>> index
>f41129915683194237d1bba56d17df61ae89c063..7d074a5df322d68ded8f96c4
>832bc8c247435a4f 100644
>> --- a/gdb/remote.c
>> +++ b/gdb/remote.c
>> @@ -5956,6 +5956,9 @@ remote_target::remote_query_supported ()
>>  	  != AUTO_BOOLEAN_FALSE)
>>  	remote_query_supported_append (&q, "memory-tagging+");
>>
>> +      remote_query_supported_append
>> +	(&q, "qXfer:libraries:read:in-memory-library+");
>
>This is the only feature in this function that isn't guarded by a "set
>remote foo-packet" command state. Shouldn't it?

This is not a new packet.  This indicates support for a new element in the
qXfer::libraries::read response.

Regards,
Markus.
Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Sean Fennelly, Jeffrey Schneiderman, Tiffany Doon Silva
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928