Andrew Burgess [Tue, 23 May 2023 10:25:21 +0000 (11:25 +0100)]
gdb: handle core files with .reg/0 section names
The previous commit added the test gdb.arch/core-file-pid0.exp which
tests GDB's ability to load a core file containing threads with an
lwpid of 0, which is something we GDB can encounter when loading a
vmcore file -- a core file generated by the Linux kernel. The threads
with an lwpid of 0 represents idle cores.
While the previous commit added the test, which confirms GDB doesn't
crash when confronted with such a core file, there are still some
problems with GDB's handling of these core files. These problems all
originate from the fact that the core file (once opened by bfd)
contains multiple sections called .reg/0, these sections all
represents different threads (cpu cores in the original vmcore dump),
but GDB gets confused and thinks all of these .reg/0 sections are all
referencing the same thread.
Here is a GDB session on an x86-64 machine which loads the core file
from the gdb.arch/core-file-pid0.exp, this core file contains two
threads, both of which have a pid of 0:
$ ./gdb/gdb --data-directory ./gdb/data-directory/ -q
(gdb) core-file /tmp/x86_64-pid0-core.core
[New process 1]
[New process 1]
Failed to read a valid object file image from memory.
Core was generated by `./segv-mt'.
Program terminated with signal SIGSEGV, Segmentation fault.
The current thread has terminated
(gdb) info threads
Id Target Id Frame
2 process 1 0x00000000004017c2 in ?? ()
The current thread <Thread ID 1> has terminated. See `help thread'.
(gdb) maintenance info sections
Core file: `/tmp/x86_64-pid0-core.core', file type elf64-x86-64.
[0] 0x00000000->0x000012d4 at 0x00000318: note0 READONLY HAS_CONTENTS
[1] 0x00000000->0x000000d8 at 0x0000039c: .reg/0 HAS_CONTENTS
[2] 0x00000000->0x000000d8 at 0x0000039c: .reg HAS_CONTENTS
[3] 0x00000000->0x00000080 at 0x0000052c: .note.linuxcore.siginfo/0 HAS_CONTENTS
[4] 0x00000000->0x00000080 at 0x0000052c: .note.linuxcore.siginfo HAS_CONTENTS
[5] 0x00000000->0x00000140 at 0x000005c0: .auxv HAS_CONTENTS
[6] 0x00000000->0x000000a4 at 0x00000714: .note.linuxcore.file/0 HAS_CONTENTS
[7] 0x00000000->0x000000a4 at 0x00000714: .note.linuxcore.file HAS_CONTENTS
[8] 0x00000000->0x00000200 at 0x000007cc: .reg2/0 HAS_CONTENTS
[9] 0x00000000->0x00000200 at 0x000007cc: .reg2 HAS_CONTENTS
[10] 0x00000000->0x00000440 at 0x000009e0: .reg-xstate/0 HAS_CONTENTS
[11] 0x00000000->0x00000440 at 0x000009e0: .reg-xstate HAS_CONTENTS
[12] 0x00000000->0x000000d8 at 0x00000ea4: .reg/0 HAS_CONTENTS
[13] 0x00000000->0x00000200 at 0x00000f98: .reg2/0 HAS_CONTENTS
[14] 0x00000000->0x00000440 at 0x000011ac: .reg-xstate/0 HAS_CONTENTS
[15] 0x00400000->0x00401000 at 0x00002000: load1 ALLOC LOAD READONLY HAS_CONTENTS
[16] 0x00401000->0x004b9000 at 0x00003000: load2 ALLOC READONLY CODE
[17] 0x004b9000->0x004e5000 at 0x00003000: load3 ALLOC READONLY
[18] 0x004e6000->0x004ec000 at 0x00003000: load4 ALLOC LOAD HAS_CONTENTS
[19] 0x004ec000->0x004f2000 at 0x00009000: load5 ALLOC LOAD HAS_CONTENTS
[20] 0x012a8000->0x012cb000 at 0x0000f000: load6 ALLOC LOAD HAS_CONTENTS
[21] 0x7fda77736000->0x7fda77737000 at 0x00032000: load7 ALLOC READONLY
[22] 0x7fda77737000->0x7fda77f37000 at 0x00032000: load8 ALLOC LOAD HAS_CONTENTS
[23] 0x7ffd55f65000->0x7ffd55f86000 at 0x00832000: load9 ALLOC LOAD HAS_CONTENTS
[24] 0x7ffd55fc3000->0x7ffd55fc7000 at 0x00853000: load10 ALLOC LOAD READONLY HAS_CONTENTS
[25] 0x7ffd55fc7000->0x7ffd55fc9000 at 0x00857000: load11 ALLOC LOAD READONLY CODE HAS_CONTENTS
[26] 0xffffffffff600000->0xffffffffff601000 at 0x00859000: load12 ALLOC LOAD READONLY CODE HAS_CONTENTS
(gdb)
Notice when the core file is first loaded we see two lines like:
[New process 1]
And GDB reports:
The current thread has terminated
Which isn't what we'd expect from a core file -- the core file should
only contain threads that are live at the point of the crash, one of
which should be the current thread. The above message is reported
because GDB has deleted what we think is the current thread!
And in the 'info threads' output we are only seeing a single thread,
again, this is because GDB has deleted one of the threads.
Finally, the 'maintenance info sections' output shows the cause of all
our problems, two sections named .reg/0. When GDB sees the first of
these it creates a new thread. But, when we see the second .reg/0 GDB
tries to create another new thread, but this thread has the same
ptid_t as the first thread, so GDB deletes the first thread and
creates the second thread in its place.
Because both these threads are created with an lwpid of 0 GDB reports
these are 'New process NN' rather than 'New LWP NN' which is what we
would normally expect.
The previous commit includes a little more of the history of GDB
support in this area, but these problems were discussed on the mailing
list a while ago in this thread:
In this commit I propose a solution to these problems.
What I propose is that GDB should spot when we have .reg/0 sections
and, when these are found, should rename these sections using some
unique non-zero lwpid.
Note in the above output we also have sections like .reg2/0 and
.reg-xstate/0, these are additional register sets, this commit also
renumbers these sections inline with their .reg section.
The user is warned that some section renumbering has been performed.
GDB takes care to ensure that the new numbers assigned are unique and
don't clash with any of the pid's that might already be in use --
remember, in a real vmcore file, 0 is used to indicate an idle core,
non-idle cores will have the pid of whichever process was running on
that core, so we don't want GDB to assign an lwpid that clashes with
an actual pid that is in use in the core file.
After this commit here's the updated GDB session output:
$ ./gdb/gdb --data-directory ./gdb/data-directory/ -q
(gdb) core-file /tmp/x86_64-pid0-core.core
warning: found threads with pid 0, assigned replacement Target Ids: LWP 1, LWP 2
[New LWP 1]
[New LWP 2]
Failed to read a valid object file image from memory.
Core was generated by `./segv-mt'.
Program terminated with signal SIGSEGV, Segmentation fault.
#0 0x00000000004017c2 in ?? ()
[Current thread is 1 (LWP 1)]
(gdb) info threads
Id Target Id Frame
* 1 LWP 1 0x00000000004017c2 in ?? ()
2 LWP 2 0x000000000040dda5 in ?? ()
(gdb) maintenance info sections
Core file: `/tmp/x86_64-pid0-core.core', file type elf64-x86-64.
[0] 0x00000000->0x000012d4 at 0x00000318: note0 READONLY HAS_CONTENTS
[1] 0x00000000->0x000000d8 at 0x0000039c: .reg/1 HAS_CONTENTS
[2] 0x00000000->0x000000d8 at 0x0000039c: .reg HAS_CONTENTS
[3] 0x00000000->0x00000080 at 0x0000052c: .note.linuxcore.siginfo/1 HAS_CONTENTS
[4] 0x00000000->0x00000080 at 0x0000052c: .note.linuxcore.siginfo HAS_CONTENTS
[5] 0x00000000->0x00000140 at 0x000005c0: .auxv HAS_CONTENTS
[6] 0x00000000->0x000000a4 at 0x00000714: .note.linuxcore.file/1 HAS_CONTENTS
[7] 0x00000000->0x000000a4 at 0x00000714: .note.linuxcore.file HAS_CONTENTS
[8] 0x00000000->0x00000200 at 0x000007cc: .reg2/1 HAS_CONTENTS
[9] 0x00000000->0x00000200 at 0x000007cc: .reg2 HAS_CONTENTS
[10] 0x00000000->0x00000440 at 0x000009e0: .reg-xstate/1 HAS_CONTENTS
[11] 0x00000000->0x00000440 at 0x000009e0: .reg-xstate HAS_CONTENTS
[12] 0x00000000->0x000000d8 at 0x00000ea4: .reg/2 HAS_CONTENTS
[13] 0x00000000->0x00000200 at 0x00000f98: .reg2/2 HAS_CONTENTS
[14] 0x00000000->0x00000440 at 0x000011ac: .reg-xstate/2 HAS_CONTENTS
[15] 0x00400000->0x00401000 at 0x00002000: load1 ALLOC LOAD READONLY HAS_CONTENTS
[16] 0x00401000->0x004b9000 at 0x00003000: load2 ALLOC READONLY CODE
[17] 0x004b9000->0x004e5000 at 0x00003000: load3 ALLOC READONLY
[18] 0x004e6000->0x004ec000 at 0x00003000: load4 ALLOC LOAD HAS_CONTENTS
[19] 0x004ec000->0x004f2000 at 0x00009000: load5 ALLOC LOAD HAS_CONTENTS
[20] 0x012a8000->0x012cb000 at 0x0000f000: load6 ALLOC LOAD HAS_CONTENTS
[21] 0x7fda77736000->0x7fda77737000 at 0x00032000: load7 ALLOC READONLY
[22] 0x7fda77737000->0x7fda77f37000 at 0x00032000: load8 ALLOC LOAD HAS_CONTENTS
[23] 0x7ffd55f65000->0x7ffd55f86000 at 0x00832000: load9 ALLOC LOAD HAS_CONTENTS
[24] 0x7ffd55fc3000->0x7ffd55fc7000 at 0x00853000: load10 ALLOC LOAD READONLY HAS_CONTENTS
[25] 0x7ffd55fc7000->0x7ffd55fc9000 at 0x00857000: load11 ALLOC LOAD READONLY CODE HAS_CONTENTS
[26] 0xffffffffff600000->0xffffffffff601000 at 0x00859000: load12 ALLOC LOAD READONLY CODE HAS_CONTENTS
(gdb)
Notice the new warning which is issued when the core file is being
loaded. The threads are announced as '[New LWP NN]', and we see two
threads in the 'info threads' output. The 'maintenance info sections'
output shows the result of the section renaming.
The gdb.arch/core-file-pid0.exp test has been update to check for the
improved GDB output.
* thread.c (add_thread_silent): Use null_ptid instead of
minus_one_ptid while getting rid of stale inferior_ptid.
This is another test that has been carried in the Fedora GDB tree for
some time, and I thought that it would be worth merging to master. I
don't believe there is any test like this currently in the testsuite.
The problem was that when GDB was used to open a vmcore (core file)
image generated by the Linux kernel GDB would (sometimes) crash with
an assertion failure:
To understand what's going on we need some background; a vmcore file
represents each processor core in the same way that a standard
application core file represents threads. Thus, we might say, a
vmcore file represents cores as threads.
When writing a vmcore file, the kernel will store the pid of the
process currently running on that core as the thread's lwpid.
However, if a core is idle, with no process currently running on it,
then the lwpid for that thread is stored as 0 in the vmcore file. If
multiple cores are idle then multiple threads will have a lwpid of 0.
Back in 2010, the original issue reported tried to change the kernel's
behaviour in this thread:
https://lkml.org/lkml/2010/8/3/75
This change was rejected by the kernel team, the current
behaviour (lwpid of 0) was considered correct. I've checked the
source of a recent kernel. The code mentioned in the lkml.org posting
has moved, it's now in the function crash_save_cpu in the file
kernel/kexec_core.c, but the general behaviour is unchanged, an idle
core will have an lwpid of 0, so I think GDB still needs to be able to
handle this case.
When GDB loads a vmcore file (which is handled just like any other
core file) the sections are processed in core_open to generate the
threads for the core file. The processing is done by calling
add_to_thread_list, a function which looks for sections named .reg/NN
where NN is the lwpid of the thread, GDB then builds a ptid_t for the
new thread and calls add_thread.
Remember, in our case the lwpid is 0. Now for the first thread this
is fine, if a little weird, 0 isn't usually a valid lwpid, but that's
OK, GDB creates a thread with lwpid of 0 and carries on.
When we find the next thread (core) with lwpid of 0, we attempt to
create another thread with an lwpid of 0. This of course clashes with
the previously created thread, they have the same ptid_t, so GDB tries
to delete the first thread.
And it was within this thread delete code that we triggered a bug
which would then cause GDB to assert -- when deleting we tried to
switch to a thread with minus_one_ptid, this resulted in a call to
find_inferior_pid (passing in minus_one_ptid's pid, which is -1), the
find_inferior_pid call fails and returns NULL, which then triggered an
assert in switch_to_thread.
The actual details of the why the assert triggered are really not
important. What's important (I think) is that a vmcore file might
have this interesting lwpid of 0 characteristic, which isn't something
we see in "normal" application core files, and it is this that I think
we should be testing.
Now, you might be thinking: isn't deleting the first thread the wrong
thing to do? If the vmcore file has two threads that represent two
cores, and both have an lwpid of 0 (indicating both cores are idle),
then surely GDB should still represent this as two threads? You're
not wrong. This was mentioned by Pedro in the original GDB mailing
list thread here:
This is indeed a problem, and this problem is still present in GDB
today. I plan to try and address this in a later commit, however,
this first commit is about getting a test in place to confirm that GDB
at a minimum doesn't crash when loading such a vmcore file.
And so, finally, what's in this commit?
This commit contains a new test. The test doesn't actually contain a
vmcore file. Instead I've created a standard application core file
that contains two threads, and then manually edited the core file to
set the lwpid of each thread to 0.
To further reduce the size of the core file (as it will be stored in
git), I've zeroed all of the LOAD-able segments in the core file.
This test really doesn't care about that part of the core file, we
only really care about loading the register's, this is enough to
confirm that the GDB doesn't crash.
Obviously as the core file is pre-generated, this test is architecture
specific. There are already a few tests in gdb.arch/ that include
pre-generate core files. Just as those existing tests do, I've
compressed the core file with bzip2, which reduces it to just 750
bytes. I have structured the test so that if/when this patch is
merged I can add some additional core files for other architectures,
however, these are not included in this commit.
The test simply expands the core file, and then loads it into GDB.
One interesting thing to note is that GDB reports the core file
loading like this:
(gdb) core-file ./gdb/testsuite/outputs/gdb.arch/core-file-pid0/core-file-pid0.x86-64.core
[New process 1]
[New process 1]
Failed to read a valid object file image from memory.
Core was generated by `./segv-mt'.
Program terminated with signal SIGSEGV, Segmentation fault.
The current thread has terminated
(gdb)
There's two interesting things here: first, the repeated "New process
1" message. This is caused because linux_core_pid_to_str reports
anything with an lwpid of 0 as a process, rather than an LWP. And
second, the "The current thread has terminated" message. This is
because the first thread in the core file is the current thread, but
when GDB loads the second thread (which also has lwpid 0) this causes
the first thread to be deleted, as a result GDB thinks that the
current (first) thread has terminated.
As I said previously, both of these problems are a result of the lwpid
0 aliasing, which is not being fixed in this commit -- this commit is
just confirming that GDB doesn't crash when loading this core file.
Andrew Burgess [Thu, 1 Jun 2023 17:30:48 +0000 (18:30 +0100)]
gdb: split inferior and thread setup when opening a core file
I noticed that in corelow.c, when a core file is opened, both the
thread and inferior setup is done in add_to_thread_list. In this
patch I propose hoisting the inferior setup out of add_to_thread_list
into core_target_open.
The only thing about this change that gave me cause for concern is
that in add_to_thread_list, we only setup the inferior after finding
the first section with a name like ".reg/NN". If we find no such
section then the inferior will never be setup.
Is this important?
Well, I don't think so. Back in core_target_open, if there is no
current thread (which there will not be if no ".reg/NN" section was
found), then we look for a thread in the current inferior. If there
are no threads (which there will not be if no ".reg/NN" is found),
then we once again setup the current inferior.
What I think this means, is that, in all cases, the current inferior
will end up being setup. By moving the inferior setup code earlier in
core_target_open and making it non-conditional, we can remove the
later code that sets up the inferior, we now know this will always
have been done.
There should be no user visible changes after this commit.
Tom de Vries [Wed, 31 May 2023 05:39:31 +0000 (07:39 +0200)]
[gdb/tui] Fix fingerprint for cmd-only layout
I added a cmd-only layout:
...
(gdb) tui new-layout cmd cmd 1
...
and set it:
...
(gdb) layout cmd
...
which gave me the expect result: only the cmd window in the screen.
However, after going back to layout src:
...
(gdb) layout src
...
I got a source window with only one line in it, and the cmd window taking most
of the screen.
I traced this back to tui_set_layout, where for both the old and the new
layout the fingerprint of the cmd window in the layout is taken. If the
fingerprint is the same, an effort will be done to preserve the command
window size.
The fingerprint is "VC" for both the old (cmd) and new (src) layouts, which
explains the behaviour.
I think this is essentially a bug in the finger print calculation, and it
should be "C" for the cmd layout.
Fix this by not adding a V or H in the fingerprint if the list size is one.
Andrew Burgess [Thu, 23 Mar 2023 12:12:38 +0000 (12:12 +0000)]
gdb: add support for %V to printf command
This commit adds a new format for the printf and dprintf commands:
'%V'. This new format takes any GDB expression and formats it as a
string, just as GDB would for a 'print' command, e.g.:
This new feature would effectively replace an existing feature of GDB,
the $_as_string builtin convenience function. However, the
$_as_string function has a few problems which this new feature solves:
1. $_as_string doesn't currently work when the inferior is not
running, e.g:
(gdb) printf "%s", $_as_string(a1)
You can't do that without a process to debug.
(gdb)
The reason for this is that $_as_string returns a value object with
string type. When we try to print this we call value_as_address,
which ends up trying to push the string into the inferior's address
space.
Clearly we could solve this problem, the string data exists in GDB, so
there's no reason why we have to push it into the inferior, but this
is an existing problem that would need solving.
2. $_as_string suffers from the fact that C degrades arrays to
pointers, e.g.:
The implementation of $_as_string is passed a gdb.Value object that is
a pointer, it doesn't understand that it's actually an array. Solving
this would be harder than issue #1 I think. The whole array to
pointer transformation is part of our expression evaluation. And in
most cases this is exactly what we want. It's not clear to me how
we'd (easily) tell GDB that we didn't want this reduction in _some_
cases. But I'm sure this is solvable if we really wanted to.
3. $_as_string is a gdb.Function sub-class, and as such is passed
gdb.Value objects. There's no super convenient way to pass formatting
options to $_as_string. By this I mean that the new %V feature
supports print formatting options. Ideally, we might want to add this
feature to $_as_string, we might imagine it working something like:
(gdb) printf "%s\n", $_as_string(a1,
elements = 3,
array_indexes = True)
where the first item is the value to print, while the remaining
options are the print formatting options. However, this relies on
Python calling syntax, which isn't something that convenience
functions handle. We could possibly rely on strictly positional
arguments, like:
(gdb) printf "%s\n", $_as_string(a1, 3, 1)
But that's clearly terrible as there's far more print formatting
options, and if you needed to set the 9th option you'd need to fill in
all the previous options.
And right now, the only way to pass these options to a gdb.Function is
to have GDB first convert them all into gdb.Value objects, which is
really overkill for what we want.
The new %V format solves all these problems: the string is computed
and printed entirely on the GDB side, we are able to print arrays as
actual arrays rather than pointers, and we can pass named format
arguments.
Finally, the $_as_string is sold in the manual as allowing users to
print the string representation of flag enums, so given:
So all functionality of $_as_string is replaced by %V. I'm not
proposing to remove $_as_string, there might be users currently
depending on it, but I am proposing that we don't push $_as_string in
the documentation.
As %V is a feature of printf, GDB's dprintf breakpoints naturally gain
access to this feature too. dprintf breakpoints can be operated in
three different styles 'gdb' (use GDB's printf), 'call' (call a
function in the inferior), or 'agent' (perform the dprintf on the
remote).
The use of '%V' will work just fine when dprintf-style is 'gdb'.
When dprintf-style is 'call' the format string and arguments are
passed to an inferior function (printf by default). In this case GDB
doesn't prevent use of '%V', but the documentation makes it clear that
support for '%V' will depend on the inferior function being called.
I chose this approach because the current implementation doesn't place
any restrictions on the format string when operating in 'call' style.
That is, the user might already be calling a function that supports
custom print format specifiers (maybe including '%V') so, I claim, it
would be wrong to block use of '%V' in this case. The documentation
does make it clear that users shouldn't expect this to "just work"
though.
When dprintf-style is 'agent' then GDB does no support the use of
'%V' (right now). This is handled at the point when GDB tries to
process the format string and send the dprintf command to the remote,
here's an example:
Reading symbols from /tmp/hello.x...
(gdb) dprintf call_me, "%V", a1
Dprintf 1 at 0x401152: file /tmp/hello.c, line 8.
(gdb) set sysroot /
(gdb) target remote | gdbserver --once - /tmp/hello.x
Remote debugging using | gdbserver --once - /tmp/hello.x
stdin/stdout redirected
Process /tmp/hello.x created; pid = 3088822
Remote debugging using stdio
Reading symbols from /lib64/ld-linux-x86-64.so.2...
(No debugging symbols found in /lib64/ld-linux-x86-64.so.2)
0x00007ffff7fd3110 in _start () from /lib64/ld-linux-x86-64.so.2
(gdb) set dprintf-style agent
(gdb) c
Continuing.
Unrecognized format specifier 'V' in printf
Command aborted.
(gdb)
This is exactly how GDB would handle any other invalid format
specifier, for example:
Reading symbols from /tmp/hello.x...
(gdb) dprintf call_me, "%Q", a1
Dprintf 1 at 0x401152: file /tmp/hello.c, line 8.
(gdb) set sysroot /
(gdb) target remote | gdbserver --once - /tmp/hello.x
Remote debugging using | gdbserver --once - /tmp/hello.x
stdin/stdout redirected
Process /tmp/hello.x created; pid = 3089193
Remote debugging using stdio
Reading symbols from /lib64/ld-linux-x86-64.so.2...
(No debugging symbols found in /lib64/ld-linux-x86-64.so.2)
0x00007ffff7fd3110 in _start () from /lib64/ld-linux-x86-64.so.2
(gdb) set dprintf-style agent
(gdb) c
Continuing.
Unrecognized format specifier 'Q' in printf
Command aborted.
(gdb)
The error message isn't the greatest, but improving that can be put
off for another day I hope.
Reviewed-By: Eli Zaretskii <eliz@gnu.org> Acked-By: Simon Marchi <simon.marchi@efficios.com>
Simon Marchi [Fri, 21 Apr 2023 13:45:30 +0000 (09:45 -0400)]
gdb: add interp::on_about_to_proceed method
Same idea as previous patches, but for about_to_proceed. We only need
(and want, as far as the mi_interp implementation is concerned) to
notify the interpreter that caused the proceed.
Simon Marchi [Fri, 21 Apr 2023 13:45:30 +0000 (09:45 -0400)]
gdb: add interp::on_inferior_disappeared method
Same idea as previous patches, but for inferior_disappeared.
For symmetry with on_inferior_appeared, I named this one
on_inferior_disappeared, despite the observer being called
inferior_exit. This is called when detaching an inferior, so I think
that calling it "disappeared" is a bit less misleading (the observer
should probably be renamed later).
Simon Marchi [Fri, 21 Apr 2023 13:45:30 +0000 (09:45 -0400)]
gdb: add interp::on_inferior_added method
Same idea as previous patches, but for inferior_added.
mi_interp::init avoided using mi_inferior_added, since, as the comment
used to say, it would notify all MI interpreters. Now, it's easy to
only notify the new interpreter, so it's possible to just call the
on_inferior_added method in mi_interp::init.
Simon Marchi [Thu, 20 Apr 2023 19:47:59 +0000 (15:47 -0400)]
gdb: add interp::on_sync_execution_done method
Same as previous patches, but for sync_execution_done. Except that
here, we only want to notify the interpreter that is executing the
command, not all interpreters.
Simon Marchi [Thu, 20 Apr 2023 18:46:58 +0000 (14:46 -0400)]
gdb: add interp::on_exited method
Same as previous patch, but for exited. Remove the exited observable,
since nothing uses it anymore, and we don't have anything coming that
will use it.
Simon Marchi [Thu, 20 Apr 2023 18:02:28 +0000 (14:02 -0400)]
gdb: add interp::on_signal_exited method
Same as previous patch, but for signal_exited. Remove the signal_exited
observable, since nothing uses it anymore, and we don't have anything
coming that will use it.
Simon Marchi [Wed, 1 Mar 2023 21:48:36 +0000 (16:48 -0500)]
gdb: add interp::on_signal_received method
Instead of having the interpreter code registering observers for the
signal_received observable, add a "signal_received" virtual method to
struct interp. Add a interps_notify_signal_received function that loops
over all UIs and calls the signal_received method on the interpreter.
Finally, add a notify_signal_received function that calls
interps_notify_signal_received and then notifies the observers. Replace
all existing notifications to the signal_received observers with calls
to notify_signal_received.
Before this patch, the CLI and MI code both register a signal_received
observer. These observer go over all UIs, and, for those that have a
interpreter of the right kind, print the stop notifiation.
After this patch, we have just one "loop over all UIs", inside
interps_notify_signal_received. Since the interp::on_signal_received
method gets called once for each interpreter, the implementations only
need to deal with the current interpreter (the "this" pointer).
The motivation for this patch comes from a future patch, that makes the
amdgpu code register an observer to print a warning after the CLI's
signal stop message. Since the amdgpu and the CLI code both use
observers, the order of the two messages is not stable, unless we define
the priority using the observer dependency system. However, the
approach of using virtual methods on the interpreters seems like a good
change anyway, I think it's more straightforward and simple to
understand than the current solution that uses observers. We are sure
that the amdgpu message gets printed after the CLI message, since
observers are notified after interpreters.
Keep the signal_received, even if nothing uses if, because we will be
using it in the upcoming amdgpu patch implementing the warning described
above.
Tom de Vries [Tue, 30 May 2023 15:49:31 +0000 (17:49 +0200)]
[gdb] Mention --with/without-system-readline for --configuration
Simon reported that the new test-case gdb.tui/pr30056.exp fails with system
readline.
This is because the test-case requires a fix in readline that's present in our
in-repo copy of readline, but most likely not in any system readline yet.
Fix this by:
- mentioning --with-system-readline or --without-system-readline in the
configuration string.
- adding a new proc with_system_readline that makes this information available
in the testsuite.
- using this in test-case gdb.tui/pr30056.exp to declare it unsupported for
--with-system-readline.
Tested on x86_64-linux.
Reported-By: Simon Marchi <simon.marchi@efficios.com> Approved-By: Simon Marchi <simon.marchi@efficios.com>
Nick Clifton [Tue, 30 May 2023 14:43:24 +0000 (15:43 +0100)]
Improve header information displayed with objdump -P for PE binaries.
* od-pe.c (targ_info): New array.
(get_target_specific_info): New function.
(decode_machine_number): Retire. Use get_target_specific_info instead.
(is_pe_object_magic): Likewise.
(dump_pe_file_header): Display more information.
Rework layout to be similar to that from 'objdump -p'.
Add code to handle larger than normnal AOUT headers.
mengqinggang [Thu, 1 Dec 2022 08:17:46 +0000 (16:17 +0800)]
LoongArch: gas: Add support for linker relaxation.
Add gas -mrelax and -mno-relax option.
Add R_LARCH_RELAX reloc for instrction if it can be relaxed.
ADD R_LARCH_ALIGN reloc for align pseudo instruction because relax.
Add ADD/SUB reloc pair for debug and exception data to calculate symbol
substraction because relax.
gas/ChangeLog:
* config/tc-loongarch.c:
(struct loongarch_cl_insn): New macro_id member.
(enum options): New OPTION_RELAX and OPTION_NO_RELAX.
(struct option): New mrelax and mno-relax.
(md_parse_option): Likewise.
(get_internal_label):
(loongarch_args_parser_can_match_arg_helper): Generate relax reloc.
(move_insn): Set fx_frag and fx_where if exist.
(append_fixp_and_insn): Call frag_wane and frag_new for linker relax
relocs.
(loongarch_assemble_INSNs): New loongarch_cl_insn pointer parameter.
(md_assemble): Fix function call.
(fix_reloc_insn): Likewise.
(md_apply_fix): Generate ADD/SUB reloc pair for debug and exception
data.
(loongarch_fix_adjustable): Delete.
(md_convert_frag): Generate new fix.
(loongarch_pre_output_hook): New function.
(loongarch_make_nops): Likewise.
(loongarch_frag_align_code): Likewise.
(loongarch_insert_uleb128_fixes): Likewise.
(loongarch_md_finish): Likewise.
* config/tc-loongarch.h
(md_allow_local_subtract): New macro define.
(loongarch_frag_align_code): New declare.
(md_do_align): Likewise.
(loongarch_fix_adjustable): Delete.
(tc_fix_adjustable): New macro define.
(TC_FORCE_RELOCATION_SUB_SAME): Likewise.
(TC_LINKRELAX_FIXUP): Likewise.
(TC_FORCE_RELOCATION_LOCAL): Likewise.
(DWARF2_USE_FIXED_ADVANCE_PC): Likewise.
(MD_APPLY_SYM_VALUE): Likewise.
(tc_symbol_new_hook): New extern.
(NOP_OPCODE): Delete.
(loongarch_pre_output_hook): New macro define.
(md_pre_output_hook): Likewise.
(md_finish): Likewise.
(loongarch_md_finish): New extern.
* testsuite/gas/all/align.d: Mark as unsupported on LoongArch.
* testsuite/gas/all/gas.exp: Xfail loongarch*-*.
* testsuite/gas/all/relax.d: Likewise.
* testsuite/gas/elf/dwarf-5-irp.d: Likewise.
* testsuite/gas/elf/dwarf-5-loc0.d: Likewise.
* testsuite/gas/elf/dwarf-5-macro-include.d: Likewise.
* testsuite/gas/elf/dwarf-5-macro.d: Likewise.
* testsuite/gas/elf/dwarf2-11.d: Likewise.
* testsuite/gas/elf/dwarf2-15.d: Likewise.
* testsuite/gas/elf/dwarf2-16.d: Likewise.
* testsuite/gas/elf/dwarf2-17.d: Likewise.
* testsuite/gas/elf/dwarf2-18.d: Likewise.
* testsuite/gas/elf/dwarf2-19.d: Likewise.
* testsuite/gas/elf/dwarf2-5.d: Likewise.
* testsuite/gas/elf/ehopt0.d: Likewise.
* testsuite/gas/elf/elf.exp: Likewise.
* testsuite/gas/elf/section11.d: Likewise.
* testsuite/gas/lns/lns.exp: Likewise.
* testsuite/gas/loongarch/jmp_op.d: Regenerated.
* testsuite/gas/loongarch/li.d: Likewise.
* testsuite/gas/loongarch/macro_op.d: Likewise.
* testsuite/gas/loongarch/macro_op_32.d: Likewise.
* testsuite/gas/loongarch/macro_op_large_abs.d: Likewise.
* testsuite/gas/loongarch/macro_op_large_pc.d: Likewise.
* testsuite/gas/loongarch/relax_align.d: New test.
* testsuite/gas/loongarch/relax_align.s: New test.
* testsuite/gas/loongarch/uleb128.d: New test.
* testsuite/gas/loongarch/uleb128.s: New test.
mengqinggang [Thu, 1 Dec 2022 07:03:42 +0000 (15:03 +0800)]
LoongArch: bfd: Add support for linker relaxation.
Add relax support and related relocs in bfd.
bfd/ChangeLog:
* bfd-in2.h: Add relocs related to relax.
* elfnn-loongarch.c (struct loongarch_elf_link_hash_table): New integer
pointer (data_segment_phase) to monitor the data segment phase.
(loongarch_elf_check_relocs): Swap B21/B26 reloc sequence.
(loongarch_elf_adjust_dynamic_symbol): Fix code format.
(loongarch_reloc_rewrite_imm_insn): Fix function call.
(perform_relocation): Handle new relocs related to relax.
(RELOCATE_CALC_PC32_HI20): Fix code format.
(RELOCATE_CALC_PC64_HI32): Likewise.
(loongarch_elf_relocate_section): Handle new relocs related to relax.
(loongarch_relax_delete_bytes): New function.
(loongarch_relax_pcala_addi): Likewise.
(loongarch_relax_pcala_ld): Likewise.
(bfd_elfNN_loongarch_set_data_segment_info): Likewise.
(loongarch_relax_align): Likewise.
(loongarch_elf_relax_section): Likewise.
(bfd_elfNN_bfd_relax_section): New macro define.
* elfxx-loongarch.c (reloc_bits): New bfd point parameter.
(reloc_bits_b16): Likewise.
(reloc_bits_b21): Likewise.
(reloc_bits_b26): Likewise.
(loongarch_adjust_reloc_bitsfield): Likewise.
(reloc_bits_pcrel20_s2): New function.
(loongarch_elf_add_sub_reloc): Likewise.
(loongarch_elf_add_sub_reloc_uleb128): Likewise.
(loongarch_write_unsigned_leb128): New function.
* elfxx-loongarch.h (loongarch_adjust_reloc_bitsfield): New bfd point
parameter.
(bfd_elf32_loongarch_set_data_segment_info): New declare.
(bfd_elf64_loongarch_set_data_segment_info): Likewise.
(loongarch_write_unsigned_leb128): Likewise.
* libbfd.h: Add relocs related to relax.
* reloc.c: Add relocs related to relax.
Alan Modra [Tue, 30 May 2023 03:29:55 +0000 (12:59 +0930)]
arm-pe objdump -P
arm-pe looks to be a very old PE implementation, incompatible with
current arm-wince-pe. arm-pe has different relocations and uses
ARMMAGIC which has this comment: "I just made this up". Well, OK, I
don't know the history but it was probably before Microsoft "just made
up" their constants for ARM windows CE.
This patch supports objdump -P for arm-pe, and another magic constant
that may appear in object files. (I don't think binutils generates
files using ARMV7PEMAGIC aka IMAGE_FILE_MACHINE_ARMNT.)
* od-pe.c (is_pe_object_magic): Handle IMAGE_FILE_MACHINE_ARMNT
and ARMMAGIC.
Tom de Vries [Mon, 29 May 2023 20:11:05 +0000 (22:11 +0200)]
[gdb/testsuite] Fix linefeed scrolling in tuiterm
I came across a bug in the implementation of line feed in tuiterm, and added a
unit test that exposes it.
Before sending the line feed we have:
...
Screen Dump (size 8 columns x 4 rows, cursor at column 0, row 3):
0 abcdefgh
1 ijklmnop
2 qrstuvwx
3 yz01234
...
and after it we have:
...
Screen Dump (size 8 columns x 4 rows, cursor at column 0, row 1):
0 ijklmnop
1 qrstuvwx
2 yz01234
3 yz01234
...
Note how the cursor started at row 3 and after the line feed ended up at
row 1, while it should have stayed in row 3.
Fix this by moving "incr _cur_row -1" one level up in the loop nest in
proc _ctl_0x0a.
Simon Marchi [Fri, 21 Apr 2023 16:08:42 +0000 (12:08 -0400)]
gdb/mi: fix ^running record with multiple MI interpreters
I stumbled on the mi_proceeded and running_result_record_printed
globals, which are shared by all MI interpreter instances (it's unlikely
that people use multiple MI interpreter instances, but it's possible).
After poking at it, I found this bug:
1. Start GDB in MI mode
2. Add a second MI interpreter with the new-ui command
3. Use -exec-run on the second interpreter
This is the output I get on the first interpreter:
The problem here is that the `^running` reply to the -exec-run command
is printed on the wrong UI. It is printed on the first one, it should
be printed on the second (the one on which we sent the -exec-run).
What happens under the hood is that captured_mi_execute_command, while
executing a command for the second intepreter, clears the
running_result_record_printed and mi_proceeded globals.
mi_about_to_proceed then sets mi_proceeded. Then, mi_on_resume_1 gets
called for the first intepreter first. Since the
!running_result_record_printed && mi_proceeded
condition is true, it prints a ^running, and sets
running_result_record_printed. When mi_on_resume_1 gets called for the
second interpreter, running_result_record_printed is already set, so
^running is not printed there.
It took me a while to understand the relationship between these two
variables. I think that in the end, this is what we want to track:
1. When executing an MI command, take note if that command causes a
"proceed". This is done in mi_about_to_proceed.
2. In mi_on_resume_1, if the command indeed caused a "proceed", we want
to output a ^running record. And we want to remember that we did,
because...
3. Back in captured_mi_execute_command, if we did not output a
^running, we want to output a ^done.
Moving those two variables to the mi_interp struture appears to fix it.
Only for the interpreter doing the -exec-run command does the
running_result_record_printed flag get cleared, and therefore only or
that one does the ^running record get printed.
Add a new test for this, that does pretty much what the reproducer above
shows. Without the fix, the test fails because
mi_send_resuming_command_raw never sees the ^running record.
Change-Id: I63ea30e6cb61a8e1dd5ef03377e6003381a9209b Tested-By: Alexandra Petlanova Hajkova <ahajkova@redhat.com>
Tom de Vries [Sun, 28 May 2023 08:17:57 +0000 (10:17 +0200)]
[readline] Fix double free in _rl_scxt_dispose
Consider the following scenario. We start gdb in TUI mode:
...
$ gdb -q -tui
...
and type ^R which gives us the reverse-isearch prompt in the cmd window:
...
(reverse-i-search)`':
...
and then type "foo", right-arrow-key, and ^C.
In TUI mode, gdb uses a custom rl_getc_function tui_getc.
When pressing the right-arrow-key, tui_getc:
- attempts to scroll the TUI src window, without any effect, and
- returns 0.
The intention of returning 0 is mentioned here in tui_dispatch_ctrl_char:
...
/* We intercepted the control character, so return 0 (which readline
will interpret as a no-op). */
return 0;
...
However, after this 0 is returned by the rl_read_key () call in
_rl_search_getchar, _rl_read_mbstring is called, which incorrectly interprets
0 as the first part of an utf-8 multibyte char, and tries to read the next
char.
In this state, the ^C takes effect and we run into a double free because
_rl_isearch_cleanup is called twice.
Both these issues need fixing independently, though after fixing the first we
no longer trigger the second.
The first issue is caused by the subtle difference between:
- a char array containing 0 chars, which is zero-terminated, and
- a char array containing 1 char, which is zero.
In mbrtowc terms, this is the difference between:
...
mbrtowc (&wc, "", 0, &ps);
...
which returns -2, and:
...
mbrtowc (&wc, "", 1, &ps);
...
which returns 0.
Note that _rl_read_mbstring calls _rl_get_char_len without passing it an
explicit length parameter, and consequently it cannot distinguish between the
two, and defaults to the "0 chars" choice.
Note that the same problem doesn't exist in _rl_read_mbchar.
Fix this by defaulting to the "1 char" choice in _rl_get_char_len:
...
- if (_rl_utf8locale && l > 0 && UTF8_SINGLEBYTE(*src))
+ if (_rl_utf8locale && l >= 0 && UTF8_SINGLEBYTE(*src))
...
The second problem happens when the call to _rl_search_getchar in
_rl_isearch_callback returns. At that point _rl_isearch_cleanup has already
been called from the signal handler, but we proceed regardless, using a cxt
pointer that has been freed.
Fix this by checking for "RL_ISSTATE (RL_STATE_ISEARCH)" after the call to
_rl_search_getchar:
...
c = _rl_search_getchar (cxt);
+ if (!RL_ISSTATE (RL_STATE_ISEARCH))
+ return 1;
...
Tested on x86_64-linux.
Approved-By: Chet Ramey <chet.ramey@case.edu>
PR tui/30056
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=30056
Nelson Chu [Thu, 4 May 2023 09:08:50 +0000 (17:08 +0800)]
[PR ld/22263][PR ld/25694] RISC-V: Avoid dynamic TLS relocs in PIE.
Lots of targets already fixed the TEXTREL problem for TLS in PIE.
* For PR ld/25694,
In the check_reloc, refer to spare and loongarch, they don't need to reserve
any local dynamic reloc for TLS LE in pie/pde, and similar to other targets.
So it seems like riscv was too conservative to estimate the TLS LE before.
Just break and don't goto static_reloc for TLS LE in pie/pde can fix the
TEXTREL problem.
* For PR ld/22263,
The risc-v code for TLS GD/IE in the relocate_section seems same as MIPS port.
So similar to MIPS, pr22570, commits 9143e72c6d4d and 1cb83cac9a89, it seems
also the right way to do the same thing for risc-v.
On risc-v, fixes
FAIL: Build pr22263-1
RISC-V haven't supported the TLS transitions, so will need the same fix (use
bfd_link_dll) in the future.
bfd/
PR ld/22263
PR ld/25694
* elfnn-riscv.c (riscv_elf_check_relocs): Replace bfd_link_pic with
bfd_link_dll for TLS IE. Don't need to reserve the local dynamic
relocation for TLS LE in pie/pde, and report error in pic just like
before.
(riscv_elf_relocate_section): For TLS GD/IE, use bfd_link_dll rather
than !bfd_link_pic in determining the dynamic symbol index. Avoid
the index of -1.
Nick Clifton [Fri, 26 May 2023 14:41:20 +0000 (15:41 +0100)]
Enhance objdump's --private option so that it can display the contents of PE format files.
* od-pe.c: New file: Dumps fields in PE format headers.
* configure.ac (od_vectors): Add objdump_private_desc_pe for PE format targets. (od_files): Add od-pe for PE format targets.
* configure: Regenerate.
* Makefile.am (CFILES): Add od-pe.c (EXTRA_objdump_SOURCE): Likewise.
* Makefile.in: Generate.
* NEWS: Mention the new feature.
* doc/binutils.texi: Document the new support.
* objdump.c (wide_output): Change from local to global.
* objdump.h (wide_output): Prototype. (objdump_private_desc_pe): Prototype.
* testsuite/binutils-all/objdump.exp: Add a test of the new feature.
In commit 1a3b4f90bc5f ("x86: convert two pointers to (indexing)
integers") I neglected the fact that compilers may warn about comparing
ptrdiff_t (signed long) with size_t (unsigned long) values. Since just
before we've checked that the value is positive, simply add a cast
(despite my dislike for casts).
Tom de Vries [Fri, 26 May 2023 10:30:24 +0000 (12:30 +0200)]
[gdb/testsuite] Fix reverse attribute in tuiterm
I noticed in proc Term::_csi_m arguments that while parameters 7 and 27 are
supposed to set the reverse attribute to 1 and 0, in fact it's set to 1 in
both cases:
...
7 {
set _attrs(reverse) 1
}
...
27 {
set _attrs(reverse) 1
}
...
Fix this and add a regression test in gdb.tui/tuiterm.exp.
Jan Beulich [Fri, 26 May 2023 08:13:29 +0000 (10:13 +0200)]
x86: split gas testsuite .exp file
The set of 32-bit-only and 64-bit-only tests has grown quite large. In
particular when one's after only the results for the 64-bit set, having
them live in a separate .exp file is easier / faster.
Jan Beulich [Fri, 26 May 2023 07:53:51 +0000 (09:53 +0200)]
x86: convert two pointers to (indexing) integers
This in particular reduces the number of pointers to non-const that we
have (and that could potentially be used for undue modification of
state). As a result, fetch_code()'s 2nd parameter can then also become
pointer-to-const.
Jan Beulich [Fri, 26 May 2023 07:53:25 +0000 (09:53 +0200)]
x86: disassembling over-long insns
The present way of dealing with them - misusing MAX_MNEM_SIZE, which has
nothing to do with insn length - leads to inconsistent results. Since we
allow for up to MAX_CODE_LENGTH - 1 prefix bytes (which then could be
followed by another MAX_CODE_LENGTH "normal" insn bytes until we're done
decoding), size the_buffer[] accordingly.
Move struct dis_private down to be able to use MAX_CODE_LENGTH without
moving its #define. While doing this also alter the order to have the
potentially large array last.
Jan Beulich [Fri, 26 May 2023 07:53:01 +0000 (09:53 +0200)]
x86: use fixed-width type for codep and friends
This first of all removes a dependency on bfd_byte and unsigned char
being the same types. It further eliminates the need to mask by 0xff
when fetching values (which wasn't done fully consistently anyway),
improving code legibility.
Jan Beulich [Fri, 26 May 2023 07:42:03 +0000 (09:42 +0200)]
x86: figure braces aren't really part of mnemonics
Instead they're separators for pseudo-prefixes. Don't insert them in
mnemonic_chars[], handling them explicitly in parse_insn() instead. Note
that this eliminates the need for another separator after a pseudo-
prefix. While maybe not overly interesting for a following real
mnemonic, I view this as quite desirable between multiple successive
pseudo-prefixes (bringing things in line with the other use of figure
braces in AVX512's zeroing-masking).
Drop the unused is_mnemonic_char() at this occasion.
Having to add characters to both arrays can easily lead to oversights.
Consuming extra_symbol_chars[] when populating operand_chars[] also
allows to drop two special cases in md_begin().
Constify operand_special_chars[] at this occasion.
Indu Bhagat [Fri, 26 May 2023 06:44:09 +0000 (23:44 -0700)]
libsframe: revisit sframe_find_fre API
Inspite of implementing a rather simple functionality, this function was
relatively difficult to follow, and maintain. Some changes are done now
to address that - refactor the function and use better names to make it
more readable.
The changes to the implementation do not cause any change in the
contract of the API.
libsframe/
* sframe.c (sframe_fre_get_end_ip_offset): to here...
(sframe_find_fre): Refactor some bits from...
Alan Modra [Fri, 26 May 2023 02:21:57 +0000 (11:51 +0930)]
PR22263 ld test
A number of targets that I test regularly fail the "Build pr22263-1"
test for various reasons.
arm-linux-gnueabi: "undefined reference to `__aeabi_read_tp'"
ia64-linux-gnu: "Explicit stops are ignored in auto mode"
m68k-linux-gnu: "undefined reference to `__m68k_read_tp'"
microblaze-linux-gnu: "undefined reference to `__tls_get_addr'"
nios2-linux-gnu, s390-linux-gnu and sh4-linux-gnu have a tprel reloc in .got
riscv64-linux-gnu has a dynamic relocation in text
So only riscv really fails the pr. The rest fail due to test issues
or lack of a linker optimisation. Lack of an optimisation isn't
really a fail, but it's worth keeping the test to ensure those
optimisations don't regress. The xfail targets may not be an
exhaustive list. This just tidies test results for those for which I
have cross compilers installed.
PR 22263
* testsuite/ld-elf/tls.exp: Split pr22263 test into two parts,
one to check for -z text errors, the other to check tprel
linker optimisation. Supply needed symbols and assembler flags.
xfail the linker optimisation on targets known to fail.
Tom Tromey [Fri, 10 May 2019 22:09:35 +0000 (16:09 -0600)]
Make MI commands const-correct
I've had this patch for a while now and figured I'd update it and send
it. It changes MI commands to use a "const char * const" for their
argv parameter.
Regression tested on x86-64 Fedora 36.
Acked-By: Simon Marchi <simon.marchi@efficios.com>
Ciaran Woodward [Thu, 25 May 2023 11:14:15 +0000 (11:14 +0000)]
Fix scoped_value_mark not working with empty value chain
The scoped_value_mark helper class was setting its internal
mark value to NULL to indicate that the value chain had already
been freed to mark.
However, value_mark() also returns NULL if the value chain is
empty at the time of call.
This lead to the situation that if the value chain was empty
at the time the scoped_value_mark was created, the class
would not correctly clean up the state when it was destroyed,
because it believed it had already been freed.
I noticed this because I was setting a watchpoint very early
in my debug session, and it was becoming a software watchpoint
rather than hardware. Running any command that called evaluate()
beforehand (such as 'x 0') would mean that a hardware watchpoint
was correctly used. After some careful examination of the
differences in execution, I noticed that values were being freed
later in the 'bad case', which lead me to notice the issue with
scoped_value_mark.
Simon Marchi [Mon, 8 May 2023 18:49:33 +0000 (14:49 -0400)]
gdbsupport: make filtered_iterator::operator* return the same thing as underlying iterator
This is the same idea as the previous patch, but for filtered_iterator.
Without this patch, I would see this when applying the patch that
removes reference_to_pointer_iterator from breakpoint_range:
CXX breakpoint.o
/home/smarchi/src/binutils-gdb/gdb/breakpoint.c: In function ‘void download_tracepoint_locations()’:
/home/smarchi/src/binutils-gdb/gdb/breakpoint.c:11007:41: error: cannot allocate an object of abstract type ‘breakpoint’
11007 | for (breakpoint &b : all_tracepoints ())
| ^
In file included from /home/smarchi/src/binutils-gdb/gdb/gdbthread.h:26,
from /home/smarchi/src/binutils-gdb/gdb/infrun.h:21,
from /home/smarchi/src/binutils-gdb/gdb/gdbarch.h:28,
from /home/smarchi/src/binutils-gdb/gdb/arch-utils.h:23,
from /home/smarchi/src/binutils-gdb/gdb/breakpoint.c:21:
/home/smarchi/src/binutils-gdb/gdb/breakpoint.h:619:8: note: because the following virtual functions are pure within ‘breakpoint’:
619 | struct breakpoint : public intrusive_list_node<breakpoint>
| ^~~~~~~~~~
/home/smarchi/src/binutils-gdb/gdb/breakpoint.c:250:1: note: ‘virtual breakpoint::~breakpoint()’
250 | breakpoint::~breakpoint ()
| ^~~~~~~~~~
Change-Id: I05285ff27d21cb0ab80cba392ec4e959167e3cd7 Reviewed-By: Andrew Burgess <aburgess@redhat.com>
Simon Marchi [Mon, 8 May 2023 18:49:33 +0000 (14:49 -0400)]
gdbsupport: make basic_safe_iterator::operator* return the same thing as underlying iterator
Using the following patch that removes the reference_to_pointer_iterator
from breakpoint_range, I would get:
CXX breakpoint.o
/home/smarchi/src/binutils-gdb/gdb/breakpoint.c: In function ‘void breakpoint_program_space_exit(program_space*)’:
/home/smarchi/src/binutils-gdb/gdb/breakpoint.c:3030:46: error: cannot allocate an object of abstract type ‘breakpoint’
3030 | for (breakpoint &b : all_breakpoints_safe ())
| ^
In file included from /home/smarchi/src/binutils-gdb/gdb/gdbthread.h:26,
from /home/smarchi/src/binutils-gdb/gdb/infrun.h:21,
from /home/smarchi/src/binutils-gdb/gdb/gdbarch.h:28,
from /home/smarchi/src/binutils-gdb/gdb/arch-utils.h:23,
from /home/smarchi/src/binutils-gdb/gdb/breakpoint.c:21:
/home/smarchi/src/binutils-gdb/gdb/breakpoint.h:619:8: note: because the following virtual functions are pure within ‘breakpoint’:
619 | struct breakpoint : public intrusive_list_node<breakpoint>
| ^~~~~~~~~~
/home/smarchi/src/binutils-gdb/gdb/breakpoint.c:250:1: note: ‘virtual breakpoint::~breakpoint()’
250 | breakpoint::~breakpoint ()
| ^~~~~~~~~~
This is because the operator* method of the basic_safe_iterator iterator
wrapper returns a value_type. So, even if the method of the underlying
iterator (breakpoint_iterator, an intrusive_list iterator) returns a
`breakpoint &`, the method of the wrapper returns a `breakpoint`.
I think it would make sense for iterator wrappers such as
basic_safe_iterator to return the exact same thing as the iterator they
wrap. At least, it fixes my problem.
Change-Id: Ibbcd390ac03d2fb6ae4854923750c8d7c3c04e8a Reviewed-By: Andrew Burgess <aburgess@redhat.com>
- Add methods for the various manipulations that need to be done on the
location list, while maintaining reasonably good encapsulation.
- bp_location currently has a default constructor because of one use
in hoist_existing_locations. hoist_existing_locations now returns a
bp_location_list, and doesn't need the default-constructor
bp_location anymore, so remove the bp_location default constructor.
- I needed to add a call to clear_locations in delete_breakpoint to
avoid a use-after-free.
- Add a breakpoint::last_loc method, for use in
set_breakpoint_condition.
bp_location_range uses reference_to_pointer_iterator, so that all
existing callers of breakpoint::locations don't need to change right
now. It will be removed in the next patch.
The rest of the changes are to adapt the call sites to use the new
methods, of breakpoint::locations, rather than breakpoint::loc directly.
Change-Id: I25f7ee3d66a4e914a0540589ac414b3b820b6e70 Reviewed-By: Andrew Burgess <aburgess@redhat.com>
Simon Marchi [Wed, 10 May 2023 16:03:13 +0000 (12:03 -0400)]
gdbsupport: add missing increment/decrement operators to reference_to_pointer_iterator
Using the following patch, I would get this build failure:
CXX breakpoint.o
In file included from /usr/include/c++/13.1.1/bits/stl_algobase.h:66,
from /usr/include/c++/13.1.1/bits/hashtable_policy.h:36,
from /usr/include/c++/13.1.1/bits/hashtable.h:35,
from /usr/include/c++/13.1.1/bits/unordered_map.h:33,
from /usr/include/c++/13.1.1/unordered_map:41,
from /usr/include/c++/13.1.1/functional:63,
from /home/smarchi/src/binutils-gdb/gdb/../gdbsupport/ptid.h:35,
from /home/smarchi/src/binutils-gdb/gdb/../gdbsupport/common-defs.h:206,
from /home/smarchi/src/binutils-gdb/gdb/defs.h:26,
from /home/smarchi/src/binutils-gdb/gdb/breakpoint.c:20:
/usr/include/c++/13.1.1/bits/stl_iterator_base_funcs.h: In instantiation of ‘constexpr void std::__advance(_BidirectionalIterator&, _Distance, bidirectional_iterator_tag) [with _BidirectionalIterator = reference_to_pointer_iterator<intrusive_list_iterator<bp_location, intrusive_base_node<bp_location> > >; _Distance = long int]’:
/usr/include/c++/13.1.1/bits/stl_iterator_base_funcs.h:224:21: required from ‘constexpr void std::advance(_InputIterator&, _Distance) [with _InputIterator = reference_to_pointer_iterator<intrusive_list_iterator<bp_location, intrusive_base_node<bp_location> > >; _Distance = long int]’
/usr/include/c++/13.1.1/bits/stl_iterator_base_funcs.h:237:19: required from ‘constexpr _InputIterator std::next(_InputIterator, typename iterator_traits<_Iter>::difference_type) [with _InputIterator = reference_to_pointer_iterator<intrusive_list_iterator<bp_location, intrusive_base_node<bp_location> > >; typename iterator_traits<_Iter>::difference_type = long int]’
/home/smarchi/src/binutils-gdb/gdb/breakpoint.c:1073:19: required from here
/usr/include/c++/13.1.1/bits/stl_iterator_base_funcs.h:179:11: error: no match for ‘operator--’ (operand type is ‘reference_to_pointer_iterator<intrusive_list_iterator<bp_location, intrusive_base_node<bp_location> > >’)
179 | --__i;
| ^~~~~
This points out that while intrusive_list_iterator has an operator--,
the reference_to_pointer_iterator wrapper does not. I'm not to sure why
the compiler chooses the overload of __advance that accepts a
_BidirectionalIterator, given that reference_to_pointer_iterator can't
be decremented, but adding those operators seems like the right thing to
do in any case, for completeness.
Change-Id: I8e2044b6734fadf0f21093047cf35bb7080dbdc3 Reviewed-By: Andrew Burgess <aburgess@redhat.com>
Simon Marchi [Tue, 9 May 2023 14:28:09 +0000 (10:28 -0400)]
gdb: add breakpoint::first_loc methods
Add convenience first_loc methods to struct breakpoint (const and
non-const overloads). A subsequent patch changes the list of locations
to be an intrusive_list and makes the actual list private, so these
spots would need to change from:
b->loc
to something ugly like:
*b->locations ().begin ()
That would make the code much heavier and not readable. There is a
surprisingly big number of places that access the first location of
breakpoints. Whether this is correct, or these spots fail to consider
the possibility of multi-location breakpoints, I don't know. But
anyhow, I think that using this instead:
b->first_loc ()
conveys the intention better than the other two forms.
Change-Id: Ibbefe3e4ca6cdfe570351fe7e2725f2ce11d1e95 Reviewed-By: Andrew Burgess <aburgess@redhat.com>
Simon Marchi [Tue, 9 May 2023 14:23:44 +0000 (10:23 -0400)]
gdb: add breakpoint "has locations" methods
Add three convenience methods to struct breakpoint:
- has_locations: returns true if the breakpoint has at least one
location
- has_single_location: returns true if the breakpoint has exactly one
location
- has_multiple_locations: returns true if the breakpoint has more than
one location
A subsequent patch changes the list of breakpoints to be an
intrusive_list, so all these spots would need to change. But in any
case, I think that this:
if (b->has_multiple_locations ())
conveys the intention better than:
if (b->loc != nullptr && b->loc->next != nullptr)
Change-Id: Ib18c3605fd35d425ef9df82cb7aacff1606c6747 Reviewed-By: Andrew Burgess <aburgess@redhat.com>
Simon Marchi [Mon, 8 May 2023 20:40:40 +0000 (16:40 -0400)]
gdb: constify breakpoint::print_it parameter
The print_it method itself is const. In a subsequent patch, the
locations that come out of a const breakpoint will be const as well. It
will therefore be needed to make the last_loc output parameter const as
well. Make that change now to reduce the size of the following patches.
Change-Id: I7ed962950bc9582646e31e2e42beca2a1c9c5105 Reviewed-By: Andrew Burgess <aburgess@redhat.com>
Simon Marchi [Mon, 8 May 2023 19:54:52 +0000 (15:54 -0400)]
gdb: make some breakpoint methods use `this`
Some implementations of breakpoint::check_status and
breakpoint::print_it do this:
struct breakpoint *b = bs->breakpoint_at;
bs->breakpoint_at is always the same as `this` (we can get convinced by
looking at the call sites of check_status and print_it), so it would
just be clearer to access fields through `this` instead.
Change-Id: Ic542a64fcd88e31ae2aad6feff1da278c7086891 Reviewed-By: Alexandra Petlanova Hajkova <ahajkova@redhat.com> Reviewed-By: Andrew Burgess <aburgess@redhat.com>
Simon Marchi [Mon, 8 May 2023 19:45:52 +0000 (15:45 -0400)]
gdb: get gdbarch from syscall_catchpoint instead of location
I noticed some methods of syscall_catchpoint doing this:
struct gdbarch *gdbarch = loc->owner->gdbarch;
`loc` is the list of locations of this catchpoint. Logically, the owner
the locations are this catchpoint. So this just ends up getting
this->gdbarch. Remove the unnecessary indirection through the loc.
syscall_catchpoint::print_recreate does something slightly different,
getting its arch from the loc:
struct gdbarch *gdbarch = loc->gdbarch;
I suppose it's always going to be the same arch, so get it from the
catchpoint there too.
Change-Id: I6f6a6f8e0cd7cfb754cecfb6249e71ec12ba4855 Reviewed-By: Alexandra Petlanova Hajkova <ahajkova@redhat.com> Reviewed-By: Andrew Burgess <aburgess@redhat.com>
Simon Marchi [Thu, 18 May 2023 19:05:56 +0000 (15:05 -0400)]
gdbsupport: add support for references to checked_static_cast
Add a checked_static_cast overload that works with references. A bad
dynamic cast with references throws std::bad_cast, it would be possible
to implement the new overload based on that, but it seemed simpler to
just piggy back off the existing function.
I found some potential uses of this new overload in amd-dbgapi-target.c,
update them to illustrate the use of the new overload. To build
amd-dbgapi-target.c, on needs the amd-dbgapi library, which I don't
expect many people to have. But I have it, and it builds fine here. I
did test the new overload by making a purposely bad cast and it did
catch it.
Change-Id: Id6b6a7db09fe3b4aa43cddb60575ff5f46761e96 Reviewed-By: Lancelot SIX <lsix@lancelotsix.com> Reviewed-By: Andrew Burgess <aburgess@redhat.com>
[gdb/testsuite] Fix gdb.server/multi-ui-errors.exp for remote target
I noticed the occasional failure in gdb.server/multi-ui-errors.exp,
which looked like this:
(gdb) PASS: gdb.server/multi-ui-errors.exp: interact with GDB's main UI
interrupt
(gdb)
Program received signal SIGINT, Interrupt.
0x00007ffff7d501e7 in nanosleep () from /lib64/libc.so.6
FAIL: gdb.server/multi-ui-errors.exp: interrupt (timeout)
PASS: gdb.server/multi-ui-errors.exp: interrupt arrived
p server_pid
$1 = 718174
(gdb) PASS: gdb.server/multi-ui-errors.exp: p server_pid
This is triggered by this code in gdb.server/multi-ui-errors.exp:
gdb_test "interrupt"
gdb_test_multiple "" "interrupt arrived" {
-re "Program received signal SIGINT, Interrupt\\.\r\n" {
pass $gdb_test_name
}
}
The problem here is that the first interrupt will trigger the prompt
to be printed, and then, after some time the inferior will be
interrupted.
However the default pattern for gdb_test includes a '$' end anchor.
If expect sees the prompt with nothing following it then everything is
fine, and the test passes.
However, if the interrupt is quick and so what expect sees is this:
(gdb)
Program received signal SIGINT, Interrupt.
0x00007ffff7d501e7 in nanosleep () from /lib64/libc.so.6
In this case the end anchor means that the gdb_test fails to match,
and eventually times out.
Fix this by passing -no-prompt-anchor to gdb_test.
Matti Puputti [Fri, 12 May 2023 13:33:20 +0000 (15:33 +0200)]
gdb, infcmd: Support jump command with same line in multiple symtabs
If a header file defining a static function is included in multiple source
files, each calling the function, and GDB is asked to jump to a line inside
that function, there would be multiple locations matching the target. The
solution in this commit is to select the location in the current symtab.
Reviewed-By: Eli Zaretskii <eliz@gnu.org> Approved-By: Andrew Burgess <aburgess@redhat.com>
Tom Tromey [Mon, 1 May 2023 19:59:20 +0000 (13:59 -0600)]
Add "args" and "env" parameters to DAP launch request
This patch augments the DAP launch request with some optional new
parameters that let the client control the command-line arguments and
the environment of the inferior.
Reviewed-By: Andrew Burgess <aburgess@redhat.com> Reviewed-By: Eli Zaretskii <eliz@gnu.org>
projects / thirdparty / binutils-gdb.git / log
