Re: [PATCH v2 11/47] gdb, gdbserver, rsp, ze: acknowledge libraries

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

Hello Baris,

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

> @@ -48084,16 +48141,18 @@ The format of a library list is described by this DTD:
>  @smallexample
>  <!-- library-list: Root element with versioning -->
>  <!ELEMENT library-list            (library | in-memory-library)*>
> -<!ATTLIST library-list            version CDATA   #FIXED  "1.1">
> +<!ATTLIST library-list            version CDATA        #FIXED  "1.2">
>  <!ELEMENT library                 (segment*, section*)>
> -<!ATTLIST library                 name    CDATA   #REQUIRED>
> +<!ATTLIST library                 name    CDATA        #REQUIRED
> +                                  ack     (yes | no)   'no'>
>  <!ELEMENT in-memory-library       (segment*, section*)>
> -<!ATTLIST in-memory-library       begin    CDATA   #REQUIRED
> -                                  end      CDATA   #REQUIRED>
> +<!ATTLIST in-memory-library       begin    CDATA       #REQUIRED
> +                                  end      CDATA       #REQUIRED
> +                                  ack      (yes | no)  'no'>
>  <!ELEMENT segment                 EMPTY>
> -<!ATTLIST segment                 address CDATA   #REQUIRED>
> +<!ATTLIST segment                 address CDATA        #REQUIRED>
>  <!ELEMENT section                 EMPTY>
> -<!ATTLIST section                 address CDATA   #REQUIRED>
> +<!ATTLIST section                 address CDATA        #REQUIRED>
>  @end smallexample

I would move the last column (the one with "#FIXED", "#REQUIRED" and
'no') a couple more spaces to the right. The change above leaves them
right next to "(segment*, section*)>" and it feels a bit crowded IMHO.

>  
>  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 f55071c8e906f091752e8ca78ec29bcd76028433..473b8eaa719b1d310f9eb4fe8f531df85008845d 100644
> --- a/gdb/features/library-list.dtd
> +++ b/gdb/features/library-list.dtd
> @@ -6,14 +6,16 @@
>  
>  <!-- library-list: Root element with versioning -->
>  <!ELEMENT library-list  (library | in-memory-library)*>
> -<!ATTLIST library-list  version CDATA   #FIXED  "1.1">
> +<!ATTLIST library-list  version CDATA   #FIXED  "1.2">

Same comment here as in patch 10: IMO it's better to just use CDATA
#REQUIRED and rely on library_list_start_list to check if the version is
acceptable.

Also, same comment about bumping the version only once in the series, to
1.1.

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

Same comment here about moving the last column a couple of spaces to the
right.

<snip>

> @@ -15679,6 +15700,60 @@ remote_target::vcont_r_supported ()
>  	  && get_remote_state ()->supports_vCont.r);
>  }
>  
> +void
> +remote_target::ack_library (const char *name)

Missing doc comment for this method.

> +{
> +  struct remote_state *rs = get_remote_state ();

No need for the struct keyword.

> +  char *p = rs->buf.data ();
> +  char *endp = p + get_remote_packet_size ();
> +
> +  xsnprintf (p, endp - p, "vAck:library:%s", name);
> +
> +  putpkt (rs->buf);
> +  getpkt (&rs->buf, 0);
> +
> +  packet_result result
> +    = m_features.packet_ok (rs->buf, PACKET_vAck_library);

This fits 80 columns as one line.

> +  switch (result.status ())
> +    {
> +    case PACKET_OK:
> +      break;
> +    case PACKET_UNKNOWN:
> +      error (_("No support for acknowledging libraries."));
> +    case PACKET_ERROR:
> +      error (_("Acknowledging library '%s' failed: '%s'"), name,
> +	     rs->buf.data ());
> +    }
> +}
> +
> +void
> +remote_target::ack_in_memory_library (CORE_ADDR begin, CORE_ADDR end)

Missing doc comment for this method.

> +{
> +  struct remote_state *rs = get_remote_state ();

No need for the struct keyword.

> +  char *p = rs->buf.data ();
> +  char *endp = p + get_remote_packet_size ();
> +
> +  xsnprintf (p, endp - p, "vAck:in-memory-library:%s,%s",
> +	     core_addr_to_string_nz (begin), core_addr_to_string_nz (end));
> +
> +  putpkt (rs->buf);
> +  getpkt (&rs->buf, 0);
> +
> +  packet_result result
> +    = m_features.packet_ok (rs->buf, PACKET_vAck_in_memory_library);
> +  switch (result.status ())
> +    {
> +    case PACKET_OK:
> +      break;
> +    case PACKET_UNKNOWN:
> +      error (_("No support for acknowledging in-memory libraries."));
> +    case PACKET_ERROR:
> +      error (_("Failed to acknowledge in-memory library %s-%s: %s"),
> +	     core_addr_to_string_nz (begin), core_addr_to_string_nz (end),
> +	     rs->buf.data ());
> +    }
> +}
> +
>  /* The "set/show range-stepping" set hook.  */
>  
>  static void
> @@ -16442,6 +16517,12 @@ Show the maximum size of the address (in bits) in a memory packet."), NULL,
>  
>    add_packet_config_cmd (PACKET_vCtrlC, "vCtrlC", "ctrl-c", 0);
>  
> +  add_packet_config_cmd (PACKET_vAck_library,
> +			 "vAck:library", "ack-library", 0);

This fits 80 columns as one line.

> +
> +  add_packet_config_cmd (PACKET_vAck_in_memory_library,
> +			 "vAck:in-memory-library", "ack-in-memory-library", 0);
> +
>    add_packet_config_cmd (PACKET_QThreadEvents, "QThreadEvents", "thread-events",
>  			 0);

<snip>

> @@ -473,6 +503,32 @@ solib_target_in_dynsym_resolve_code (CORE_ADDR pc)
>    return in_plt_section (pc);
>  }
>  
> +static void
> +solib_target_ack_library (solib &so)
> +{
> +  lm_info_target *lm
> +    = gdb::checked_static_cast<lm_info_target *> (so.lm_info.get ());
> +
> +  if (!lm->need_ack)
> +    return;
> +
> +  /* Try only once, whether we succeed or not.  */
> +  lm->need_ack = false;
> +  switch (lm->location)
> +    {
> +    case lm_on_disk:
> +      target_ack_library (so.so_original_name.c_str ());
> +      return;
> +
> +    case lm_in_memory:
> +      target_ack_in_memory_library (lm->begin, lm->end);
> +      return;
> +    }
> +
> +  warning (_("bad solib location '%d' for %s."), lm->location,
> +	   so.so_original_name.c_str ());

Isn't this an internal error? If so, it should call
gdb_assert_not_reached.

> +}
> +
>  const solib_ops solib_target_so_ops =
>  {
>    solib_target_relocate_section_addresses,
> @@ -489,4 +545,5 @@ const solib_ops solib_target_so_ops =
>    nullptr,
>    default_find_solib_addr,
>    gdb_bfd_open_from_target_memory,
> +  solib_target_ack_library,
>  };
> diff --git a/gdb/solib.c b/gdb/solib.c
> index cb302f9215257ddff18c94f5be39e189b02d84e6..92fc5137a3d469f55043235a65dcadcc1f25fe96 100644
> --- a/gdb/solib.c
> +++ b/gdb/solib.c
> @@ -967,6 +967,7 @@ solib_add (const char *pattern, int from_tty, int readsyms)
>      if (from_tty)
>        add_flags |= SYMFILE_VERBOSE;
>  
> +    std::list<solib *> added_solibs;
>      for (solib &gdb : current_program_space->solibs ())
>        if (!pattern || re_exec (gdb.so_name.c_str ()))
>  	{
> @@ -989,14 +990,23 @@ solib_add (const char *pattern, int from_tty, int readsyms)
>  				styled_string (file_name_style.style (),
>  					       gdb.so_name.c_str ()));
>  		}
> -	      else if (solib_read_symbols (gdb, add_flags))
> -		loaded_any_symbols = true;
> +	      else
> +		added_solibs.emplace_back (&gdb);

This should use the push_back method since we don't want to create a new
object in the list.

>  	    }
>  	}
>  
> +    for (solib *gdb : added_solibs)
> +      if (solib_read_symbols (*gdb, add_flags))
> +	loaded_any_symbols = true;
> +
>      if (loaded_any_symbols)
>        breakpoint_re_set ();
>  
> +    /* Acknowledge loading of new solibs.  This must be called after
> +       breakpoints have been set in this newly loaded solib.  */
> +    for (solib *gdb : added_solibs)
> +      solib_ack_library (*gdb);
> +
>      if (from_tty && pattern && !any_matches)
>        gdb_printf ("No loaded shared libraries match the pattern `%s'.\n",
>  		  pattern);
> @@ -1700,6 +1710,16 @@ default_find_solib_addr (solib &so)
>    return {};
>  }
>  
> +/* See solist.h.  */
> +
> +void solib_ack_library (solib &so)

This function is only called from solib_add, shouldn't it be static?
Or even just inlined into the caller?

> +{
> +  const solib_ops *ops = gdbarch_so_ops (current_inferior ()->arch ());
> +
> +  if (ops->ack_library != nullptr)
> +    (*ops->ack_library) (so);
> +}
> +
>  void _initialize_solib ();
>  
>  void

<snip>

> @@ -60,6 +72,78 @@ unloaded_dll (const char *name, CORE_ADDR base_addr)
>    unloaded_dll (current_process (), name, base_addr);
>  }
>  
> +static void
> +ack_dll (process_info *process, dll_info &dll)
> +{
> +  gdb_assert (dll.need_ack);
> +
> +  switch (dll.location)
> +    {
> +    case dll_info::on_disk:
> +      /* Check if this is a temporary file for an in-memory library.  */
> +      if (dll.begin == UNSPECIFIED_CORE_ADDR)
> +	{
> +	  target_ack_library (process, dll.name.c_str ());
> +	  dll.need_ack = false;
> +	  return;
> +	}
> +
> +      [[fallthrough]];
> +    case dll_info::in_memory:
> +      target_ack_in_memory_library (process, dll.begin, dll.end);
> +      dll.need_ack = false;
> +      return;
> +    }

I'm confused by this switch statement. So a vAck:library packet will be
used only if it's for an in-memory library and vAck:in-memory-library
isn't supported? I would think that a vAck:library would also be used
for an on-disk library.

> +  internal_error (_("bad library location: %d."), dll.location);
> +}
> +
> +void
> +ack_dll (process_info *proc, const char *name)
> +{
> +  std::list<dll_info> &dlls = proc->all_dlls;
> +  std::list<dll_info>::iterator it
> +    = std::find_if (dlls.begin (), dlls.end (),
> +		    [name] (const dll_info &dll)
> +	{
> +	  return (dll.name == std::string (name));
> +	});
> +
> +  if (it != dlls.end ())
> +    ack_dll (proc, *it);
> +}
> +
> +void
> +ack_dll (const char *name)
> +{
> +  ack_dll (current_process (), name);
> +}
> +
> +void
> +ack_dll (process_info *proc, CORE_ADDR begin, CORE_ADDR end)
> +{
> +  std::list<dll_info> &dlls = proc->all_dlls;
> +  std::list<dll_info>::iterator it
> +    = std::find_if (dlls.begin (), dlls.end (),
> +		    [begin, end] (const dll_info &dll)
> +	{
> +	  /* For root devices with multiple sub-devices, modules with
> +	     identical start/end addresses may be received for different
> +	     sub-devices.  Therefore we check for the 'NEED_ACK' flag in
> +	     the search, too.  */

And is it guaranteed that only one of them will have need_ack set to
true? Can you explain that in the comment?

> +	  return ((dll.begin == begin) && (dll.end == end) && dll.need_ack);
> +	});
> +
> +  if (it != dlls.end ())
> +    ack_dll (proc, *it);
> +}

<snip>

> @@ -3371,6 +3398,66 @@ err:
>    return;
>  }
>  
> +/* Parse vAck packets.  */
> +
> +static void
> +handle_v_ack (char *own_buf)
> +{
> +  client_state &cs = get_client_state ();
> +  char *p;
> +
> +  /* Move past vAck: to the first type string.  */
> +  p = &own_buf[5];
> +  do
> +    {
> +      if (cs.vack_library_supported
> +	  && (strncmp (p, "library:", strlen ("library:")) == 0))
> +	{
> +	  p += strlen ("library:");
> +
> +	  /* We expect a single argument: the filename.  */
> +	  const char *name = p;
> +	  p = strchr (p, ';');
> +	  if (p != nullptr)
> +	    *p++ = '\0';
> +
> +	  ack_dll (name);

The documentation for vAck says:

  Acknowledge a ‘;’-separated list of remote stub responses, each with a
  ‘,’- separated list of arguments defined by its type.

But the above will consider everything between the ':' and ';' to be the
library name, even if there's a ','. I think you should check whether
there's a ',' in name, and if so either reject the packet or ignore the
additional arguments — not sure which is better, I'm leaning towards
rejecting the packet.

> +	}
> +      else if (cs.vack_in_memory_library_supported
> +	       && (strncmp (p, "in-memory-library:",
> +			    strlen ("in-memory-library:")) == 0))
> +	{
> +	  p += strlen ("in-memory-library:");
> +
> +	  /* We expect two arguments: begin and end address.  */
> +	  CORE_ADDR begin, end;
> +
> +	  begin = (CORE_ADDR) strtoull (p, &p, 16);
> +	  if (*p != ',')
> +	    break;

If this break is taken and begin is the last argument in the packet,
then won't *p == 0 and cause write_ok () to be called below? An error
should be returned instead.

> +	  end = (CORE_ADDR) strtoull (p+1, &p, 16);
> +	  if (*p == ';')
> +	    p += 1;
> +	  else if (*p != 0)
> +	    break;
> +
> +	  ack_dll (begin, end);
> +	}
> +      else
> +	break;
> +    }
> +  while (p != nullptr && *p != 0);
> +
> +  if (p == nullptr || *p == 0)
> +    write_ok (own_buf);
> +  else
> +    {
> +      std::string junk { p };
> +      sprintf (own_buf, "E.junk in vAck: '%s'.", junk.c_str ());
> +    }
> +}

-- 
Thiago