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

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

Hello Thiago,

Thanks for your review.

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

I assume you mean moving all the existing entries to the right so they all
align again.

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

vAck:library is the default for on-disk libraries.  There's one special case,
though.  If the target provided an in-memory library and we created a
temporary file for it, GDB will acknowledge it using vAck:library, but we
want to acknowledge it as in-memory library to our target, since that's
what the target provided.

The check is wrong, though.  It should be (dll.begin == 0) as on-disk libraries
are initialized with begin = 0.  We do not expect in-memory libraries to start
at zero.

This is not covered by automated testing since both GDB and gdbserver support
in-memory libraries for our target.  I tested the v3 version manually by disabling
in-memory library support in our GDB.

See my comment in the reply to patch 10.  Since our target does not need that
fall-back, we could remove it and have someone else add it for their target if they
need it.  This way, we will have a natural way of testing it.  But then, testing
combinations of GDB and gdbserver is and will remain a challenge.


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

This is simply wrong.  I suspect this is a workaround that sneaked in as
a fixup a long time ago when the stack was less mature and we have not
spotted it when we prepared the patch series for submission.

For root devices with multiple sub-devices and mirrored module heaps,
we indeed load the same device library multiple times, but either GDB
represents each sub-device as a separate inferior, so they would be on
different dll lists, or the stack below GDB would be responsible for
presenting a single virtual module to GDB.

In theory, you could load the same library at different base addresses,
so we could argue that we'd want the base address in the comparison.
And maybe the dynamic linker namespace, too.

We support none of that in our target, so this would be completely
untested and arguably overengineered.

In v3, I remove the need_ack check and make loaded_dll() error out
when called with a duplicate in-memory library so we don't run into
silent errors here.

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

Common practice seems to be to encode filenames in RSP packets
as hex-encoded strings.  I'll follow that.

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

I agree.  Thanks.

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