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

Thiago Jung Bauermann
2025-07-16T04:20:50Z

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