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

[Thiago Jung Bauermann <thiago.bauermann@linaro.org>]

Tankut Baris Aktemur <tankut.baris.aktemur@intel.com> writes:

> diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
> index ddd414659fcc554813fc2a12a79e581ad7a188b9..c84a8372c223e724e3042322a8bee07b12423050 100644
> --- a/gdb/doc/gdb.texinfo
> +++ b/gdb/doc/gdb.texinfo
> @@ -48026,9 +48026,10 @@ queries the target's operating system and reports which libraries
>  are loaded.
>  
>  The @samp{qXfer:libraries:read} packet returns an XML document which
> -lists loaded libraries and their offsets.  Each library has an
> -associated name and one or more segment or section base addresses,
> -which report where the library was loaded in memory.
> +lists loaded libraries and their offsets.  Each library has either an
> +associated name or begin and end addresses and one or more segment or
> +section base addresses, which report where the library was loaded in
> +memory.
>  
>  For the common case of libraries that are fully linked binaries, the
>  library should have a list of segments.  If the target supports
> @@ -48040,6 +48041,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:
>  
> @@ -48051,6 +48056,16 @@ offset, looks like this:
>  </library-list>
>  @end smallexample
>  
> +A corresponding memory map for an in-memory library looks like this:
> +
> +@smallexample
> +<library-list>
> +  <in-memory-library begin="0xa000000" end="0xa001000">
> +    <segment address="0x10000000"/>
> +  </library>

Typo: the end tag doesn't match the start tag.

> +</library-list>
> +@end smallexample
> +
>  Another simple memory map, with one loaded library with three
>  allocated sections (.text, .data, .bss), looks like this:
>  
> @@ -48068,14 +48083,17 @@ The format of a library list is described by this DTD:
>  
>  @smallexample
>  <!-- library-list: Root element with versioning -->
> -<!ELEMENT library-list  (library)*>
> -<!ATTLIST library-list  version CDATA   #FIXED  "1.0">
> -<!ELEMENT library       (segment*, section*)>
> -<!ATTLIST library       name    CDATA   #REQUIRED>
> -<!ELEMENT segment       EMPTY>
> -<!ATTLIST segment       address CDATA   #REQUIRED>
> -<!ELEMENT section       EMPTY>
> -<!ATTLIST section       address CDATA   #REQUIRED>
> +<!ELEMENT library-list            (library | in-memory-library)*>
> +<!ATTLIST library-list            version CDATA   #FIXED  "1.1">
> +<!ELEMENT library                 (segment*, section*)>
> +<!ATTLIST library                 name    CDATA   #REQUIRED>
> +<!ELEMENT in-memory-library       (segment*, section*)>
> +<!ATTLIST in-memory-library       begin    CDATA   #REQUIRED
> +                                  end      CDATA   #REQUIRED>
> +<!ELEMENT segment                 EMPTY>
> +<!ATTLIST segment                 address CDATA   #REQUIRED>
> +<!ELEMENT section                 EMPTY>
> +<!ATTLIST section                 address CDATA   #REQUIRED>
>  @end smallexample
>  
>  In addition, segments and section descriptors cannot be mixed within a
> diff --git a/gdb/features/library-list.dtd b/gdb/features/library-list.dtd
> index 98ed7bc28dcc4562ef4259fdd78138fe69d69d29..f55071c8e906f091752e8ca78ec29bcd76028433 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">

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.

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?

>  <!ELEMENT library       (segment*, section*)>
>  <!ATTLIST library       name    CDATA   #REQUIRED>
>  
> +<!ELEMENT in-memory-library       (segment*, section*)>
> +<!ATTLIST in-memory-library       begin    CDATA   #REQUIRED
> +                                  end      CDATA   #REQUIRED>
> +
>  <!ELEMENT segment       EMPTY>
>  <!ATTLIST segment       address CDATA   #REQUIRED>
>

<snip>

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

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

> +	    new_solib.begin = info->begin;
> +	    new_solib.end = info->end;
> +	  }
> +	  break;
> +	}
>  
> -      /* 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;
>        new_solib.lm_info = std::move (info);
>      }

<snip>

> diff --git a/gdb/solist.h b/gdb/solist.h
> index 8ad0aefd9e32b264b08a853eb3577829a8e74767..c4d053a5c6c0261f1176121b2bc1f904ac2424a4 100644
> --- a/gdb/solist.h
> +++ b/gdb/solist.h
> @@ -67,9 +67,14 @@ struct solib : intrusive_list_node<solib>
>       map we've already loaded.  */
>    std::string so_original_name;
>  
> -  /* Shared object file name, expanded to something GDB can open.  */
> +  /* Shared object file name, expanded to something GDB can open.  This is
> +     an empty string for in-memory shared objects.  */
>    std::string so_name;
>  
> +  /* The address range of an in-memory shared object.  Both BEGIN and END
> +     are zero for on-disk shared objects.  */
> +  CORE_ADDR begin, end;

It's better to initialize these to 0 here.

>    /* The following fields of the structure are built from
>       information gathered from the shared object file itself, and
>       are set when we actually add it to our symbol tables.
> @@ -180,6 +185,12 @@ struct solib_ops
>       name).  */
>  
>    std::optional<CORE_ADDR> (*find_solib_addr) (solib &so);
> +
> +  /* Open an in-memory shared library at ADDR of at most SIZE bytes.  The
> +     TARGET string is used to identify the target.  */
> +  gdb_bfd_ref_ptr (*bfd_open_from_target_memory) (CORE_ADDR addr,
> +						  CORE_ADDR size,
> +						  const char *target);
>  };
>  
>  /* A unique pointer to a so_list.  */
> diff --git a/gdbserver/dll.cc b/gdbserver/dll.cc
> index f49aa560aec57e246948d944ec377b0beb8d1dc5..c2c63e9a69a54ddede77e40ea33b4e19cbdc71af 100644
> --- a/gdbserver/dll.cc
> +++ b/gdbserver/dll.cc
> @@ -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.

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

>  /* Record that the DLL with NAME and BASE_ADDR has been unloaded
>     from the current process.  */
>  
> @@ -58,6 +69,9 @@ unloaded_dll (process_info *proc, const char *name, CORE_ADDR base_addr)
>    gdb_assert (proc != nullptr);
>    auto pred = [&] (const dll_info &dll)
>      {
> +      if (dll.location != dll_info::on_disk)
> +	return false;
> +
>        if (base_addr != UNSPECIFIED_CORE_ADDR
>  	  && base_addr == dll.base_addr)
>  	return true;
> @@ -89,3 +103,48 @@ unloaded_dll (process_info *proc, const char *name, CORE_ADDR base_addr)
>        proc->dlls_changed = true;
>      }
>  }
> +
> +/* Record that the in-memory DLL from BEGIN to END loaded at BASE_ADDR has been
> +   unloaded.  */
> +
> +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.

> diff --git a/gdbserver/dll.h b/gdbserver/dll.h
> index e603df4be2b26017fa5b795f861d8d4111779d85..9bce28e70cf3939b5b062e4b9c0f0d2d10a1d1d7 100644
> --- a/gdbserver/dll.h
> +++ b/gdbserver/dll.h
> @@ -24,19 +24,36 @@ struct process_info;
>  
>  struct dll_info
>  {
> +  enum location_t
> +  {
> +    on_disk,
> +    in_memory
> +  };
> +
>    dll_info (const std::string &name_, CORE_ADDR base_addr_)
> -  : name (name_), base_addr (base_addr_)
> +    : location (on_disk), name (name_), base_addr (base_addr_)
> +  {}
> +
> +  dll_info (CORE_ADDR begin_, CORE_ADDR end_, CORE_ADDR base_addr_)
> +    : location (in_memory), begin (begin_), end (end_), base_addr (base_addr_)
>    {}
>  
> +  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.

> +  CORE_ADDR end;
>    CORE_ADDR base_addr;
>  };
>  
>  extern void loaded_dll (const char *name, CORE_ADDR base_addr);
>  extern void loaded_dll (process_info *proc, const char *name,
>  			CORE_ADDR base_addr);
> +extern void loaded_dll (process_info *proc, CORE_ADDR begin, CORE_ADDR end,
> +			CORE_ADDR base_addr);
>  extern void unloaded_dll (const char *name, CORE_ADDR base_addr);
>  extern void unloaded_dll (process_info *proc, const char *name,
>  			  CORE_ADDR base_addr);
> +extern void unloaded_dll (process_info *proc, CORE_ADDR begin, CORE_ADDR end,
> +			  CORE_ADDR base_addr);
>  
>  #endif /* GDBSERVER_DLL_H */
> diff --git a/gdbserver/server.cc b/gdbserver/server.cc
> index 264fac44b74db69746fdecb1cdd1fd748139cc4f..90902099a1b5a84fe86cb8391bf983a5943e78c5 100644
> --- a/gdbserver/server.cc
> +++ b/gdbserver/server.cc
> @@ -1884,6 +1884,97 @@ handle_qxfer_features (const char *annex,
>    return len;
>  }
>  
> +static std::string
> +dll_to_tmpfile (dll_info &dll)

There should be a documentation comment for this function.

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

> +  size_t written = fwrite (buffer.data (), buffer.size (), 1, file.get ());
> +  if (written != 1)
> +    error (_("failed to write into %s"), name.c_str ());
> +
> +  return name;
> +}
> +
> +/* 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.  */
> +      dll.name = dll_to_tmpfile (dll);
> +      [[fallthrough]];
> +
> +    case dll_info::on_disk:
> +      return string_printf
> +	("  <library name=\"%s\"><segment address=\"0x%s\"/></library>\n",
> +	 dll.name.c_str (), paddress (dll.base_addr));
> +    }
> +
> +  warning (_("unknown dll location: %x"), dll.location);

This can only happen via some internal error in gdbserver, right?
Isn't it better to use gdb_assert_not_reached?

> +  return std::string ();
> +}
> +
> +/* Determine the library-list version required for communicating the shared
> +   libraries.  */
> +
> +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.

<snip>

> diff --git a/gdbserver/server.h b/gdbserver/server.h
> index e1297d41f8406dab670397f5600e4e08558f23b7..3bacd1062f8bf47720189d2d02b36ddb6089e6bc 100644
> --- a/gdbserver/server.h
> +++ b/gdbserver/server.h
> @@ -168,6 +168,9 @@ struct client_state
>       space randomization feature before starting an inferior.  */
>    int disable_randomization = 1;
>  
> +  /* True if qXfer:libraries:read supports in-memory-library.  */
> +  bool in_memory_library_supported = false;
> +

Nit: I would add this to the end of the struct, together with the other
bools about protocol features.

>    int pass_signals[GDB_SIGNAL_LAST];
>    int program_signals[GDB_SIGNAL_LAST];
>    int program_signals_p = 0;

-- 
Thiago

¹ https://github.com/libexpat/libexpat/issues/968#issuecomment-2692251715