[3/3] gdb/mi: include ranges in =library-unloaded event
Checks
| Context |
Check |
Description |
| linaro-tcwg-bot/tcwg_gdb_build--master-aarch64 |
success
|
Build passed
|
Commit Message
Having looked at the dlmopen support in GDB, it occurred to me that
the current MI =library-unloaded event doesn't incude enough
information to be useful.
Consider the gdb.mi/mi-dlmopen.exp test, this test loads libraries
into multiple linker namespaces, and then unloads these libraries.
We should probably figure out a way to include the linker namepsace ID
in GDB's output, e.g. in the =library-loaded and =library-unloaded MI
events, and in the output of 'info sharedlibrary'. But this commit is
not about doing that.
This commit includes the 'ranges' information in the =library-unloaded
event output. This is the same ranges information as is included in
the =library-loaded output. Even without the linker namespace ID,
this should allow MI consumers to figure out which library instance is
being unloaded.
Here is the 'info sharedlibrary' output for mi-dlmopen.exp at the
point where all the shared libraries are loaded:
info sharedlibrary
&"info sharedlibrary\n"
~"From To Syms Read Shared Object Library\n"
~"0x00007ffff7fca000 0x00007ffff7ff03f5 Yes /lib64/ld-linux-x86-64.so.2\n"
~"0x00007ffff7eda3d0 0x00007ffff7f4e898 Yes /lib64/libm.so.6\n"
~"0x00007ffff7d0e800 0x00007ffff7e6dccd Yes /lib64/libc.so.6\n"
~"0x00007ffff7fbd040 0x00007ffff7fbd116 Yes /tmp/build/gdb/testsuite/outputs/gdb.mi/mi-dlmopen/dlmopen-lib.1.so\n"
~"0x00007ffff7fb8040 0x00007ffff7fb80f9 Yes /tmp/build/gdb/testsuite/outputs/gdb.mi/mi-dlmopen/dlmopen-lib-dep.so\n"
~"0x00007ffff7bfe3d0 0x00007ffff7c72898 Yes /lib64/libm.so.6\n"
~"0x00007ffff7a32800 0x00007ffff7b91ccd Yes /lib64/libc.so.6\n"
~"0x00007ffff7fca000 0x00007ffff7ff03f5 Yes /lib64/ld-linux-x86-64.so.2\n"
~"0x00007ffff7fb3040 0x00007ffff7fb3116 Yes /tmp/build/gdb/testsuite/outputs/gdb.mi/mi-dlmopen/dlmopen-lib.1.so\n"
~"0x00007ffff7fae040 0x00007ffff7fae0f9 Yes /tmp/build/gdb/testsuite/outputs/gdb.mi/mi-dlmopen/dlmopen-lib-dep.so\n"
~"0x00007ffff7ce1040 0x00007ffff7ce1116 Yes /tmp/build/gdb/testsuite/outputs/gdb.mi/mi-dlmopen/dlmopen-lib.1.so\n"
~"0x00007ffff7cdc040 0x00007ffff7cdc0f9 Yes /tmp/build/gdb/testsuite/outputs/gdb.mi/mi-dlmopen/dlmopen-lib-dep.so\n"
~"0x00007ffff79253d0 0x00007ffff7999898 Yes /lib64/libm.so.6\n"
~"0x00007ffff7759800 0x00007ffff78b8ccd Yes /lib64/libc.so.6\n"
~"0x00007ffff7fca000 0x00007ffff7ff03f5 Yes /lib64/ld-linux-x86-64.so.2\n"
~"0x00007ffff7cd7040 0x00007ffff7cd7116 Yes /tmp/build/gdb/testsuite/outputs/gdb.mi/mi-dlmopen/dlmopen-lib.2.so\n"
^done
(gdb)
Notice that dlmopen-lib.1.so is loaded multiple times. Here is the
=library-unloaded event when one copy of this library is unloaded
before this patch:
=library-unloaded,id="/tmp/build/gdb/testsuite/outputs/gdb.mi/mi-dlmopen/dlmopen-lib.1.so",
target-name="/tmp/build/gdb/testsuite/outputs/gdb.mi/mi-dlmopen/dlmopen-lib.1.so",
host-name="/tmp/build/gdb/testsuite/outputs/gdb.mi/mi-dlmopen/dlmopen-lib.1.so",
thread-group="i1",
It is not possible, given this information, to know which copy of
dlmopen-lib.1.so has actually been unloaded. An MI consumer would
need to query the full shared library list and update from that
information.
After this patch the new output is:
=library-unloaded,id="/tmp/build/gdb/testsuite/outputs/gdb.mi/mi-dlmopen/dlmopen-lib.1.so",
target-name="/tmp/build/gdb/testsuite/outputs/gdb.mi/mi-dlmopen/dlmopen-lib.1.so",
host-name="/tmp/build/gdb/testsuite/outputs/gdb.mi/mi-dlmopen/dlmopen-lib.1.so",
thread-group="i1",
ranges=[{from="0x00007ffff7fbd040",to="0x00007ffff7fbd116"}],
still-in-use="false"
The new 'ranges' field allows an MI consumer to uniquely identify
which library instance was just unmapped. A frontent could,
e.g. update a library list with no need to query the full shared
library list.
To include the 'ranges' field I updated mi_interp::on_solib_unloaded
to call a new helper function. The new helper function is split out
from the existing mi_output_solib_attribs. I was tempted to just call
mi_output_solib_attribs, but doing so would mean that the
'symbols-loaded' field was also added to the =library-unloaded event,
however, the docs for 'symbols-unloaded' on =library-loaded says:
The @var{symbols-loaded} field is emitted only for backward
compatibility and should not be relied on to convey any useful
information.
And it seemed silly to add a fields to =library-unloaded, which I
would then document as something that should be ignored. The new
helper function means I can avoid emitting the 'symbols-loaded'
field.
I have also added a 'still-in-use' field. When true this indicates
that the library was removed from the inferior's library list, but the
mapping was not removed from the inferior's address space as there is
another copy of the library that is still using the library. In the
above list, notice that ld-linux-x86-64.so.2 appears 3 times, but each
instance is mapped as 0x00007ffff7fca000. When one copy of
ld-linux-x86-64.so.2 is unloaded, here's the event:
=library-unloaded,id="/lib64/ld-linux-x86-64.so.2",
target-name="/lib64/ld-linux-x86-64.so.2",
host-name="/lib64/ld-linux-x86-64.so.2",
thread-group="i1",
ranges=[{from="0x00007ffff7fca000",to="0x00007ffff7ff03f5"}],
still-in-use="true"
The 'still-in-use' field is 'true', this indicates there are at least
one instance of this library remaining mapped at 0x00007ffff7fca000.
---
gdb/NEWS | 12 ++++++++++++
gdb/doc/gdb.texinfo | 20 +++++++++++++++-----
gdb/mi/mi-interp.c | 24 ++++++++++++++++--------
gdb/testsuite/gdb.mi/mi-dlmopen.exp | 15 ++++++++++++++-
4 files changed, 57 insertions(+), 14 deletions(-)
Comments
> From: Andrew Burgess <aburgess@redhat.com>
> Cc: Andrew Burgess <aburgess@redhat.com>
> Date: Tue, 24 Dec 2024 12:05:37 +0000
>
> gdb/NEWS | 12 ++++++++++++
> gdb/doc/gdb.texinfo | 20 +++++++++++++++-----
> gdb/mi/mi-interp.c | 24 ++++++++++++++++--------
> gdb/testsuite/gdb.mi/mi-dlmopen.exp | 15 ++++++++++++++-
> 4 files changed, 57 insertions(+), 14 deletions(-)
Thanks.
> --- a/gdb/NEWS
> +++ b/gdb/NEWS
> @@ -110,6 +110,18 @@
>
> ** The "variables" request will not return artificial symbols.
>
> +* MI changes
> +
> +** The =library-unloaded event now includes the 'ranges' field, which
> + has the same meaning as for the =library-loaded event.
> +
> +** The =library-unloaded event now includes the 'still-in-use' field.
> + This field is 'true' when a library is unloaded (removed from the
> + inferior's list of loaded libraries), but the mapping within the
> + inferior's address space is retained, as the library was mapped
> + multiple times, and the same mapping was being reused. In all
> + other cases, this field will have the value 'false'.
> +
> * New commands
This part is okay.
The documentation parts are approved with these nits fixed.
Reviewed-By: Eli Zaretskii <eliz@gnu.org>
Eli Zaretskii <eliz@gnu.org> writes:
>> From: Andrew Burgess <aburgess@redhat.com>
>> Cc: Andrew Burgess <aburgess@redhat.com>
>> Date: Tue, 24 Dec 2024 12:05:37 +0000
>>
>> gdb/NEWS | 12 ++++++++++++
>> gdb/doc/gdb.texinfo | 20 +++++++++++++++-----
>> gdb/mi/mi-interp.c | 24 ++++++++++++++++--------
>> gdb/testsuite/gdb.mi/mi-dlmopen.exp | 15 ++++++++++++++-
>> 4 files changed, 57 insertions(+), 14 deletions(-)
>
> Thanks.
>
>> --- a/gdb/NEWS
>> +++ b/gdb/NEWS
>> @@ -110,6 +110,18 @@
>>
>> ** The "variables" request will not return artificial symbols.
>>
>> +* MI changes
>> +
>> +** The =library-unloaded event now includes the 'ranges' field, which
>> + has the same meaning as for the =library-loaded event.
>> +
>> +** The =library-unloaded event now includes the 'still-in-use' field.
>> + This field is 'true' when a library is unloaded (removed from the
>> + inferior's list of loaded libraries), but the mapping within the
>> + inferior's address space is retained, as the library was mapped
>> + multiple times, and the same mapping was being reused. In all
>> + other cases, this field will have the value 'false'.
>> +
>> * New commands
>
> This part is okay.
>
> The documentation parts are approved with these nits fixed.
>
> Reviewed-By: Eli Zaretskii <eliz@gnu.org>
Hi Eli!
Just got back around to this patch after the Christmas / New Year
break. I notice you mention fixing some nits ... but don't mention any
nits :)
Is there something you'd like changed?
Thanks,
Andrew
> From: Andrew Burgess <aburgess@redhat.com>
> Cc: gdb-patches@sourceware.org
> Date: Mon, 13 Jan 2025 16:54:21 +0000
>
> Eli Zaretskii <eliz@gnu.org> writes:
>
> >> From: Andrew Burgess <aburgess@redhat.com>
> >> Cc: Andrew Burgess <aburgess@redhat.com>
> >> Date: Tue, 24 Dec 2024 12:05:37 +0000
> >>
> >> gdb/NEWS | 12 ++++++++++++
> >> gdb/doc/gdb.texinfo | 20 +++++++++++++++-----
> >> gdb/mi/mi-interp.c | 24 ++++++++++++++++--------
> >> gdb/testsuite/gdb.mi/mi-dlmopen.exp | 15 ++++++++++++++-
> >> 4 files changed, 57 insertions(+), 14 deletions(-)
> >
> > Thanks.
> >
> >> --- a/gdb/NEWS
> >> +++ b/gdb/NEWS
> >> @@ -110,6 +110,18 @@
> >>
> >> ** The "variables" request will not return artificial symbols.
> >>
> >> +* MI changes
> >> +
> >> +** The =library-unloaded event now includes the 'ranges' field, which
> >> + has the same meaning as for the =library-loaded event.
> >> +
> >> +** The =library-unloaded event now includes the 'still-in-use' field.
> >> + This field is 'true' when a library is unloaded (removed from the
> >> + inferior's list of loaded libraries), but the mapping within the
> >> + inferior's address space is retained, as the library was mapped
> >> + multiple times, and the same mapping was being reused. In all
> >> + other cases, this field will have the value 'false'.
> >> +
> >> * New commands
> >
> > This part is okay.
> >
> > The documentation parts are approved with these nits fixed.
> >
> > Reviewed-By: Eli Zaretskii <eliz@gnu.org>
>
> Hi Eli!
>
> Just got back around to this patch after the Christmas / New Year
> break. I notice you mention fixing some nits ... but don't mention any
> nits :)
>
> Is there something you'd like changed?
Sorry, I guess I pressed Send by mistake.
Yes, there are a couple of typos:
> +It is possible that a library can be appear multiple times in an
^^
That "be" should not be there.
> +inferior's address space. When this happen, and one copy of the
^^^^^^
"happens"
Thanks.
@@ -110,6 +110,18 @@
** The "variables" request will not return artificial symbols.
+* MI changes
+
+** The =library-unloaded event now includes the 'ranges' field, which
+ has the same meaning as for the =library-loaded event.
+
+** The =library-unloaded event now includes the 'still-in-use' field.
+ This field is 'true' when a library is unloaded (removed from the
+ inferior's list of loaded libraries), but the mapping within the
+ inferior's address space is retained, as the library was mapped
+ multiple times, and the same mapping was being reused. In all
+ other cases, this field will have the value 'false'.
+
* New commands
show jit-reader-directory
@@ -32159,12 +32159,22 @@
@item =library-unloaded,...
Reports that a library was unloaded by the program. This notification
-has 3 fields---@var{id}, @var{target-name} and @var{host-name} with
-the same meaning as for the @code{=library-loaded} notification.
+has the following fields---@var{id}, @var{target-name},
+@var{host-name} and @var{ranges} with the same meaning as for the
+@code{=library-loaded} notification.
+
+It is possible that a library can be appear multiple times in an
+inferior's library list, but the library is only mapped once into the
+inferior's address space. When this happen, and one copy of the
+library is unloaded, but there are remaining copies, the
+@var{still-in-use} field will be @samp{true}. In all other
+situations, the @var{still-in-use} field will have the value
+@samp{false}.
+
The @var{thread-group} field, if present, specifies the id of the
-thread group in whose context the library was unloaded. If the field is
-absent, it means the library was unloaded in the context of all present
-thread groups.
+thread group in whose context the library was unloaded. If the field
+is absent, it means the library was unloaded in the context of all
+present thread groups.
@item =traceframe-changed,num=@var{tfnum},tracepoint=@var{tpnum}
@itemx =traceframe-changed,end
@@ -721,15 +721,17 @@ mi_interp::on_target_resumed (ptid_t ptid)
/* See mi-interp.h. */
-void
-mi_output_solib_attribs (ui_out *uiout, const solib &solib)
+static void
+mi_output_solib_attribs_1 (ui_out *uiout, const solib &solib,
+ bool include_symbols_loaded_p)
{
gdbarch *gdbarch = current_inferior ()->arch ();
uiout->field_string ("id", solib.so_original_name);
uiout->field_string ("target-name", solib.so_original_name);
uiout->field_string ("host-name", solib.so_name);
- uiout->field_signed ("symbols-loaded", solib.symbols_loaded);
+ if (include_symbols_loaded_p)
+ uiout->field_signed ("symbols-loaded", solib.symbols_loaded);
if (!gdbarch_has_global_solist (current_inferior ()->arch ()))
uiout->field_fmt ("thread-group", "i%d", current_inferior ()->num);
@@ -742,6 +744,14 @@ mi_output_solib_attribs (ui_out *uiout, const solib &solib)
}
}
+/* See mi-interp.h. */
+
+void
+mi_output_solib_attribs (ui_out *uiout, const solib &solib)
+{
+ mi_output_solib_attribs_1 (uiout, solib, true);
+}
+
void
mi_interp::on_solib_loaded (const solib &solib)
{
@@ -771,11 +781,9 @@ mi_interp::on_solib_unloaded (const solib &solib, bool still_in_use)
ui_out_redirect_pop redir (uiout, this->event_channel);
- uiout->field_string ("id", solib.so_original_name);
- uiout->field_string ("target-name", solib.so_original_name);
- uiout->field_string ("host-name", solib.so_name);
- if (!gdbarch_has_global_solist (current_inferior ()->arch ()))
- uiout->field_fmt ("thread-group", "i%d", current_inferior ()->num);
+ /* Pass false here so that 'symbols-loaded' is not included. */
+ mi_output_solib_attribs_1 (uiout, solib, false);
+ uiout->field_string ("still-in-use", still_in_use ? "true" : "false");
gdb_flush (this->event_channel);
}
@@ -167,13 +167,23 @@ proc check_solib_unload_events {} {
# unload events. Process them now.
set dyld_unload_count 0
array set unload_counts {}
+ set still_in_use_fields_correct true
gdb_test_multiple "" "" -prompt $::mi_gdb_prompt {
- -re "=library-unloaded,id=\"(\[^\"\]+)\",\[^\r\n\]+\r\n" {
+ -re "=library-unloaded,id=\"(\[^\"\]+)\",\[^\r\n\]+,ranges=\\\[\\{from=\"$::hex\",to=\"$::hex\"\\}\\\],still-in-use=\"(true|false)\"\r\n" {
set lib $expect_out(1,string)
+ set in_use $expect_out(2,string)
if {[is_dyln $lib]} {
# This is the dynamic linker being unloaded.
incr dyld_unload_count
+ set expected_in_use "true"
+ } else {
+ set expected_in_use "false"
}
+
+ if { $in_use ne $expected_in_use } {
+ set still_in_use_fields_correct false
+ }
+
set filename [file tail $lib]
incr unload_counts($filename)
exp_continue
@@ -187,6 +197,9 @@ proc check_solib_unload_events {} {
gdb_assert { $dyld_unload_count == $dyld_count - 1 } \
"expected number of dynamic linker unloads"
+ gdb_assert { $still_in_use_fields_correct } \
+ "still-in-use fields were all correct"
+
# Check that we saw the expected number of library-unloaded events for
# each library. Each DESC is a list of two elements, a filename for a
# library, and the number of times it should have been unloaded.