Similar to the parent commit, simplify schedlock_applies() by only
checking the argument thread.
When resuming that thread, GDB will automatically stop replaying its
inferior. The replay state of other inferiors is not considered by
user_visible_resume_ptid(), so let's not consider them in
schedlock_applies(), either.
Approved-By: Tom Tromey <tom@tromey.com>
When scheduler-locking is set to replay and we're resuming a thread at the
end of its execution history, we check whether anything is replaying in
user_visible_resume_ptid() only to check again in clear_proceed_status()
before we stop replaying the current process.
What really matters is whether the selected thread is replaying or will
start replaying.
Simplify this by removing redundant checks.
Also avoid a redundant pass over all threads to check whether anything is
replaying before stopping replaying. Make record_stop_replaying() handle
the case when we're not replaying gracefully.
Approved-By: Tom Tromey <tom@tromey.com>
Convert symtab to use obstack_new, and have a real constructor. The
filename, filename_for_id and m_compunit, members should really not
change once the symtab has been created, so make these members private
(m_compunit was already private) and set them just once from the
constructor. The set_compunit function has been deleted, and new
getter functions for filename and filename_for_id have been added.
The language is also set at construction time, but can be updated
later, so set the language in the constructor, but retain
symtab::set_language for when the language needs to be updated.
Prior to this patch the symtab was allocated with OBSTACK_ZALLOC which
would zero out the symtab object. With the call to objstack_new
fields in the symtab would no longer be initialised, so I've added
default member initialisation for everything not set in the
constructor.
The interesting changes are in symtab.h, and symfile.c. Everything
else is just updating to handle symfile::filename and
symfile::filename_for_id becoming methods.
Approved-By: Simon Marchi <simon.marchi@efficios.com>
I noticed my IDE (VSCode) starting to automatically trim trailing
whitespaces on save, despite the setting for it being disabled. I
realized that this is because the .editorconfig file now has
trim_trailing_whitespace = true
for many file types. If we have this EditorConfig setting forcing
editors to trim trailing whitespaces, I think it would make sense to
clean up trailing whitespaces from our files. Otherwise, people will
always get spurious whitespace changes when editing these files.
I did a mass cleanup using this command:
$ find gdb gdbserver gdbsupport -type f \( \
-name "*.c" -o \
-name "*.h" -o \
-name "*.cc" -o \
-name "*.texi" -o \
-name "*.exp" -o \
-name "*.tcl" -o \
-name "*.py" -o \
-name "*.s" -o \
-name "*.S" -o \
-name "*.asm" -o \
-name "*.awk" -o \
-name "*.ac" -o \
-name "Makefile*" -o \
-name "*.sh" -o \
-name "*.adb" -o \
-name "*.ads" -o \
-name "*.d" -o \
-name "*.go" -o \
-name "*.F90" -o \
-name "*.f90" \
\) -exec sed -ri 's/[ \t]+$//' {} +
I then did an autotools regen, because we don't actually want to change
the Makefile and Makefile.in files that are generated.
Change-Id: I6f91b83e3b8c4dc7d5d51a2ebf60706120efe691
I noticed this behaviour:
(gdb) info threads
Id Target Id Frame
1 Thread 0xf7dbc700 (LWP 3161872) "thr" 0xf7eb2888 in clone () from /lib/libc.so.6
* 2 Thread 0xf7dbbb40 (LWP 3161884) "thr" breakpt () at thr.c:19
(gdb) set suppress-cli-notifications on
(gdb) thread 1
(gdb) thread 1
[Switching to thread 1 (Thread 0xf7dbc700 (LWP 3161872))]
#0 0xf7eb2888 in clone () from /lib/libc.so.6
(gdb)
I think that the second 'thread 1' should not produce any output just
like the 'inferior' command, continuing in the same GDB session:
(gdb) inferior 1
(gdb)
Without suppress-cli-notifications we would see an inferior, thread,
and frame being printed, but with suppress-cli-notifications set to
on, we get no output.
The difference in behaviours is that in inferior_command (inferior.c),
we always call notify_user_selected_context_changed, even in the case
where the inferior doesn't actually change.
In thread_command (thread.c), we have some code that catches the
thread not changed case, and calls print_selected_thread_frame. The
notify_user_selected_context_changed function is only called if the
thread actually changes.
I did consider simply extending thread_command to check the global
cli_suppress_notification.user_selected_context state and skipping the
call to print_selected_thread_frame if suppression is on.
However, I realised that calling print_selected_thread_frame actually
introduces a bug.
When the 'thread' command is used to select the currently selected
thread, GDB still calls 'thread_selected'. And 'thread_select' always
selects frame #0 within that thread, consider this session:
(gdb) info threads
Id Target Id Frame
1 Thread 0xf7dbc700 (LWP 723986) "thr" 0xf7eb2888 in clone () from /lib/libc.so.6
* 2 Thread 0xf7dbbb40 (LWP 723990) "thr" breakpt () at thr.c:19
(gdb) bt
#0 breakpt () at thr.c:19
#1 0x080491fd in thread_worker (arg=0xffff9514) at thr.c:31
#2 0xf7f7667e in start_thread () from /lib/libpthread.so.0
#3 0xf7eb289a in clone () from /lib/libc.so.6
(gdb) frame 3
#3 0xf7eb289a in clone () from /lib/libc.so.6
(gdb) thread 2
[Switching to thread 2 (Thread 0xf7dbbb40 (LWP 723990))]
#0 breakpt () at thr.c:19
19 while (stop)
(gdb) frame
#0 breakpt () at thr.c:19
19 while (stop)
(gdb)
Notice that the frame resets back to frame #0.
By only calling print_selected_thread_frame, and not calling
notify_user_selected_context_changed, this means that GDB will fail to
emit an MI async notification. It is this async notification which
tells MI consumers that the frame has been reset to #0.
And so, I think that the correct solution is, like with the 'inferior'
command, to always call notify_user_selected_context_changed.
This does mean that in some cases unnecessary MI notifications can be
emitted, however, an MI consumer should be able to handle these. We
could try to avoid these, but we would need to extend thread_command
to check that neither the thread OR frame has changed after the call
to thread_select, and right now, I'm not sure it's worth adding the
extra complexity.
I've rewritten the gdb.base/cli-suppress-notification.exp test to
cover more cases, especially the reselecting the same thread case.
And I've updated the gdb.mi/user-selected-context-sync.exp test to
allow for the additional MI notifications that are emitted, and to
check the frame reset case.
While working on this change, I did wonder about calls to
notify_user_selected_context_changed for frame related commands. In
places we do elide calls to notify_user_selected_context_changed if
the frame hasn't changed. I wondered if there were more bugs here?
I don't think there are though. While changing the inferior will also
change the selected thread, and the selected frame. And changing the
thread will also change the selected frame. Changing the frame is the
"inner most" context related thing that can be changed. There are no
side effect changes that also need to be notified, so for these cases,
I think we are fine.
Also in infrun.c I fixed a code style issue relating to
notify_user_selected_context_changed. It's not a functional change
required by this commit, but it's related to this patch, so I'm
including it here.
Reviewed-By: Tankut Baris Aktemur <tankut.baris.aktemur@intel.com>
Tested-By: Tankut Baris Aktemur <tankut.baris.aktemur@intel.com>
Approved-By: Tom Tromey <tom@tromey.com>
To be consistent with all_threads and the others, following the previous
patch.
Change-Id: I4ee4ff32b005fc5554a9d637725f10fca70cee14
Approved-By: Tom Tromey <tom@tromey.com>
When adding reference_to_pointer_iterator, I saw it as a temporary
thing, to not have to do a codebase-wide change right away. Remove it
from inf_threads_iterator and adjust all the users.
It's very possible that I forgot to update some spots in the files I
can't compile, but it will be very easy to fix if that happens.
Change-Id: Iddc462fecfaafb6a9861d185b217bc714e7dc651
Approved-By: Tom Tromey <tom@tromey.com>
A bug was reported to Red Hat where GDB was crashing with an assertion
failure, the assertion message is:
../../gdb/regcache.c:432: internal-error: get_thread_regcache: Assertion `thread->state != THREAD_EXITED' failed.
The backtrace for the crash is:
#5 0x000055a21da8a880 in internal_vproblem(internal_problem *, const char *, int, const char *, typedef __va_list_tag __va_list_tag *) (problem=problem@entry=0x55a21e289060 <internal_error_problem>, file=<optimized out>, line=<optimized out>, fmt=<optimized out>, ap=ap@entry=0x7ffec7576be0) at ../../gdb/utils.c:477
#6 0x000055a21da8aadf in internal_verror (file=<optimized out>, line=<optimized out>, fmt=<optimized out>, ap=ap@entry=0x7ffec7576be0) at ../../gdb/utils.c:503
#7 0x000055a21dcbd055 in internal_error_loc (file=file@entry=0x55a21dd33b71 "../../gdb/regcache.c", line=line@entry=432, fmt=<optimized out>) at ../../gdbsupport/errors.cc:57
#8 0x000055a21d8baaa9 in get_thread_regcache (thread=thread@entry=0x55a258de3a50) at ../../gdb/regcache.c:432
#9 0x000055a21d74fa18 in print_signal_received_reason (uiout=0x55a258b649b0, siggnal=GDB_SIGNAL_TRAP) at ../../gdb/infrun.c:9287
#10 0x000055a21d7daad9 in mi_interp::on_signal_received (this=0x55a258af5f60, siggnal=GDB_SIGNAL_TRAP) at ../../gdb/mi/mi-interp.c:372
#11 0x000055a21d76ef99 in interps_notify<void (interp::*)(gdb_signal), gdb_signal&> (method=&virtual table offset 88, this adjustment 974682) at ../../gdb/interps.c:369
#12 0x000055a21d76e58f in interps_notify_signal_received (sig=<optimized out>, sig@entry=GDB_SIGNAL_TRAP) at ../../gdb/interps.c:378
#13 0x000055a21d75074d in notify_signal_received (sig=GDB_SIGNAL_TRAP) at ../../gdb/infrun.c:6818
#14 0x000055a21d755af0 in normal_stop () at ../../gdb/gdbthread.h:432
#15 0x000055a21d768331 in fetch_inferior_event () at ../../gdb/infrun.c:4753
The user is using a build of GDB with 32-bit ARM support included, and
they gave the following description for what they were doing at the
time of the crash:
Suspended the execution of the firmware in Eclipse. The gdb was
connected to JLinkGDBServer with activated FreeRTOS awareness JLink
plugin.
So they are remote debugging with a non-gdbserver target.
Looking in normal_stop() we see this code:
/* As we're presenting a stop, and potentially removing breakpoints,
update the thread list so we can tell whether there are threads
running on the target. With target remote, for example, we can
only learn about new threads when we explicitly update the thread
list. Do this before notifying the interpreters about signal
stops, end of stepping ranges, etc., so that the "new thread"
output is emitted before e.g., "Program received signal FOO",
instead of after. */
update_thread_list ();
if (last.kind () == TARGET_WAITKIND_STOPPED && stopped_by_random_signal)
notify_signal_received (inferior_thread ()->stop_signal ());
Which accounts for the transition from frame #14 to frame #13. But it
is the update_thread_list() call which interests me. This call asks
the target (remote target in this case) for the current thread list,
and then marks threads exited based on the answer.
And so, if a (badly behaved) target (incorrectly) removes a thread
from the thread list, then the update_thread_list() call will mark the
impacted thread as exited, even if GDB is currently handling a signal
stop event for that target.
My guess for what's going on here then is this:
1. Thread receives a signal.
2. Remote target sends GDB a stop with signal packet.
3. Remote decides that the thread is going away soon, and marks the
thread as exited.
4. GDB asks for the thread list.
5. Remote sends back the thread list, which doesn't include the
event thread, as the remote things this thread has exited.
6. GDB marks the thread as exited, and then proceeds to try and
print the signal stop event for the event thread.
7. Printing the signal stop requires reading registers, which
requires a regache. We can only get a regcache for a non-exited
thread, and so GDB raises an assertion.
Using the gdbreplay test frame work I was able to reproduce this
failure using gdbserver. I create an inferior with two threads, the
main thread sends a signal to the second thread, GDB sees the signal
arrive and prints this information for the user.
Having captured the trace of this activity, I then find the thread
list reply in the log file, and modify it to remove the second thread.
Now, when I replay the modified log file I see the same assertion
complaining about an attempt to get a regcache for an exited thread.
I'm not entirely sure the best way to fix this. Clearly the problem
here is a bad remote target. But, replies from a remote target
should (in my opinion) not be considered trusted, as a consequence, we
should not be asserting based on data coming from a remote. Instead,
we should be giving warnings or errors and have GDB handle the bad
data as best it can.
This is the second attempt to fix this issue, my first patch can be
seen here:
https://inbox.sourceware.org/gdb-patches/062e438c8677e2ab28fac6183d2ea6d444cb9121.1747567717.git.aburgess@redhat.com
In the first patch I was to checking in normal_stop, immediately after
the call to update_thread_list, to see if the current thread was now
marked as exited. However CI testing showed an issue with this
approach; I was already checking for many different TARGET_WAITKIND_*
kinds where the "is the current thread exited" question didn't make
sense, and it turns out that the list of kinds in my first attempt was
already insufficient.
Rather than trying to just adding to the list, in this revised patch
I'm proposing to move the "is this thread exited" check inside the
block which handles signal stop events.
Right now, the only part of normal_stop which I know relies on the
current thread not being exited is the call to notify_signal_received,
so before calling notify_signal_received I check to see if the current
thread is now exited. If it is then I print a warning to indicate
that the thread has unexpectedly exited and that the current
command (continue/step/etc) has been cancelled, I then change the
current event type to TARGET_WAITKIND_SPURIOUS.
GDB's output now looks like this in all-stop mode:
(gdb) continue
Continuing.
[New Thread 3483690.3483693]
[Thread 3483690.3483693 exited]
warning: Thread 3483690.3483693 unexpectedly exited after non-exit event
[Switching to Thread 3483690.3483693]
(gdb)
The non-stop output is identical, except we don't switch thread (stop
events never trigger a thread switch in non-stop mode).
The include test makes use of the gdbreplay framework, and tests in
all-stop and non-stop modes. I would like to do more extensive
testing of GDB's state after the receiving the unexpected thread list,
but due to using gdbreplay for testing, this is quite hard. Many
commands, especially those looking at thread state, are likely to
trigger additional packets being sent to the remote, which causes
gdbreplay to bail out as the new packet doesn't match the original
recorded state. However, I really don't think it is a good idea to
change gdbserver in order to "fake" this error case, so for now, using
gdbreplay is the best idea I have.
Bug: https://bugzilla.redhat.com/show_bug.cgi?id=2366461
This patch proposes a fix for PR gdb/33147. The bug can be reproduced
like this:
gdb -q -ex 'file /bin/ls' \
-ex 'run &' \
-ex 'add-inferior' \
-ex 'infer 2' \
-ex 'set sysroot' \
-ex 'target remote | gdbserver - ls'
Which will trigger an assertion failure:
target.c:3760: internal-error: target_stop: Assertion `!proc_target->commit_resumed_state' failed.
The problem is that target_stop is being called for a target when
commit_resumed_state is true, the comment on
process_stratum_target::commit_resumed_state is pretty clear:
To simplify the implementation of targets, the following methods
are guaranteed to be called with COMMIT_RESUMED_STATE set to
false:
- resume
- stop
- wait
So clearly we're breaking a precondition of target_stop. In this
example there are two target, the native target (inferior 1), and the
remote target (inferior 2). It is the first, the native target, for
which commit_resumed_state is set incorrectly.
At the point target_stop is called looks like this:
#11 0x00000000009a3c19 in target_stop (ptid=...) at ../../src/gdb/target.c:3760
#12 target_stop (ptid=...) at ../../src/gdb/target.c:3756
#13 0x00000000007042f2 in stop_all_threads (reason=<optimized out>, inf=<optimized out>) at ../../src/gdb/infrun.c:5739
#14 0x0000000000711d3a in wait_for_inferior (inf=0x2b90fd0) at ../../src/gdb/infrun.c:4412
#15 start_remote (from_tty=from_tty@entry=1) at ../../src/gdb/infrun.c:3829
#16 0x0000000000897014 in remote_target::start_remote_1 (this=this@entry=0x2c4a520, from_tty=from_tty@entry=1, extended_p=extended_p@entry=0) at ../../src/gdb/remote.c:5350
#17 0x00000000008976e7 in remote_target::start_remote (extended_p=0, from_tty=1, this=0x2c4a520) at ../../src/gdb/remote.c:5441
#18 remote_target::open_1 (name=<optimized out>, from_tty=1, extended_p=0) at ../../src/gdb/remote.c:6312
#19 0x00000000009a815f in open_target (args=0x7fffffffa93c "| gdbserver - ls", from_tty=1, command=<optimized out>) at ../../src/gdb/target.c:838
For new inferiors commit_resumed_state starts set to false, for this
reason, if we only start a remote inferior, then when
wait_for_inferior is called commit_resumed_state will be false, and
everything will work.
Further, as target_stop is only called for running threads, if, when
the remote inferior is started, all other threads (in other targets)
are already stopped, then GDB will never need to call target_stop for
the other targets, and so GDB will not notice that
commit_resumed_state for those target is set to true.
In this case though, as the first (native) inferior is left running in
the background while the remote inferior is created, and because GDB
is running in all-stop mode (so needs to stop all threads in all
targets), then GDB does call target_stop for the other targets, and so
spots that commit_resumed_state is not set correctly and asserts.
The fix is to add scoped_disable_commit_resumed somewhere in the call
stack. Initially I planned to add the scoped_disable_commit_resumed
in `wait_for_inferior`, however, this isn't good enough. This
location would solve the problem as described in the bug, but when
writing the test I extended the problem to also cover non-stop mode,
and this runs into a second problem, the same assertion, but triggered
from a different call path. For this new case the stack looks like
this:
#1 0x0000000000fb0e50 in target_stop (ptid=...) at ../../src/gdb/target.c:3771
#2 0x0000000000a7f0ae in stop_all_threads (reason=0x1d0ff74 "remote connect in all-stop", inf=0x0) at ../../src/gdb/infrun.c:5756
#3 0x0000000000d9c028 in remote_target::process_initial_stop_replies (this=0x3e10670, from_tty=1) at ../../src/gdb/remote.c:5017
#4 0x0000000000d9cdf0 in remote_target::start_remote_1 (this=0x3e10670, from_tty=1, extended_p=0) at ../../src/gdb/remote.c:5405
#5 0x0000000000d9d0d4 in remote_target::start_remote (this=0x3e10670, from_tty=1, extended_p=0) at ../../src/gdb/remote.c:5457
#6 0x0000000000d9e8ac in remote_target::open_1 (name=0x7fffffffa931 "| gdbserver - /bin/ls", from_tty=1, extended_p=0) at ../../src/gdb/remote.c:6329
#7 0x0000000000d9d167 in remote_target::open (name=0x7fffffffa931 "| gdbserver - /bin/ls", from_tty=1) at ../../src/gdb/remote.c:5479
#8 0x0000000000f9914d in open_target (args=0x7fffffffa931 "| gdbserver - /bin/ls", from_tty=1, command=0x35d1a40) at ../../src/gdb/target.c:838
So I'm now thinking that stop_all_threads would be the best place for
the scoped_disable_commit_resumed. I did leave an assert in
wait_for_inferior as, having thought about the assert some, I do still
think the logic of it is true, and it doesn't hurt to leave it in
place I think.
However, it's not quite that simple, the test throws up yet another
bug when we 'maint set target-non-stop on', but then 'set non-stop
off'. This bug leaves a stopped thread marked as "(running)" in the
'info threads' output. I have a fix for this issue, but I'm leaving
that for the next commit. For now I've just disabled part of the test
in the problem case.
I've also tagged this patch with PR gdb/27322. That bug was created
before the above assert was added, but if you follow the steps to
reproduce for that bug today you will hit the above assert. The
actual issue described in PR gdb/27322 is fixed in the next patch.
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=27322
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=33147
This patch fixes a multi-target issue where the normal_stop function
can fail to finish the thread state of threads from a non current
target, this leaves the threads marked as running in GDB core, while
the threads is actually stopped.
For testing I used this test program:
#include <unistd.h>
int
main ()
{
while (1)
sleep (1);
return 0;
}
Compile this to make '/tmp/spin', then the bug can be shown using this
command:
$ gdb -ex 'file /tmp/spin' \
-ex 'start' \
-ex 'add-inferior' \
-ex 'inferior 2' \
-ex 'set sysroot' \
-ex 'target extended-remote | gdbserver --multi --once - /tmp/spin' \
-ex 'inferior 1' \
-ex 'continue&' \
-ex 'inferior 2' \
-ex 'search sleep' \
-ex 'break $_ inferior 2' \
-ex 'continue' \
-ex 'info threads'
The interesting part of the output is:
Id Target Id Frame
1.1 process 1610445 "spin" (running)
* 2.1 Thread 1610451.1610451 "spin" main () at spin.c:7
(gdb)
Notice that thread 1.1 is marked as running when it should be
stopped. We can see that the thread is actually stopped if we try
this:
(gdb) inferior 1
[Switching to inferior 1 [process 1610445] (/tmp/spin)]
[Switching to thread 1.1 (process 1610445)](running)
(gdb) continue
Cannot execute this command while the selected thread is running.
(gdb) interrupt
(gdb) info threads
Id Target Id Frame
* 1.1 process 1610445 "spin" (running)
2.1 Thread 1610451.1610451 "spin" main () at spin.c:7
(gdb)
We can see the expected behaviour if both inferiors run on the same
target, like this:
$ gdb -ex 'file /tmp/spin' \
-ex 'start' \
-ex 'add-inferior' \
-ex 'inferior 2' \
-ex 'file /tmp/spin' \
-ex 'start' \
-ex 'inferior 1' \
-ex 'continue&' \
-ex 'inferior 2' \
-ex 'search sleep' \
-ex 'break $_ inferior 2' \
-ex 'continue' \
-ex 'info threads'
The 'info threads' from this series of commands looks like this:
Id Target Id Frame
1.1 process 1611589 "spin" 0x00007ffff7e951e7 in nanosleep () from /lib64/libc.so.6
* 2.1 process 1611593 "spin" main () at spin.c:7
(gdb)
Now both threads are stopped as we'd expect.
The problem is in normal_stop. The scoped_finish_thread_state uses
user_visible_resume_target to select the target(s) over which GDB will
iterate to find the threads to update.
The problem with this is that when the ptid_t is minus_one_ptid,
meaning all threads, user_visible_resume_target only returns nullptr,
meaning all targets, when sched_multi is true.
This dependency on sched_multi makes sense when _resuming_ threads.
If we are resuming all threads, then when sched_multi (the
schedule-multiple setting) is off (the default), all threads actually
means all threads in the current inferior only. When sched_multi is
true (schedule-multiple is on) then this means all threads, from all
inferiors, which means GDB needs to consider every target.
However, when stopping an inferior in all-stop mode (non_stop is
false), then GDB wants to stop all threads from all inferiors,
regardless of the sched_multi setting.
What this means is that, when 'non_stop' is false, then we should be
passing nullptr as the target selection to scoped_finish_thread_state.
My proposal is that we should stop using user_visible_resume_target in
the normal_stop function for the target selection of the
scoped_finish_thread_state, instead we should manually figure out the
correct target value and pass this in.
There is precedent for this in GDB, see run_command_1, where
'finish_target' is calculated directly within the function rather than
using user_visible_resume_target.
After this commit, when using two different targets (native and
remote) as in my first example above, both threads will be correctly
stopped.
PR ada/33217 points out that gdb incorrectly calls the <ctype.h>
functions. In particular, gdb feels free to pass a 'char' like:
char *str = ...;
... isdigit (*str)
This is incorrect as isdigit only accepts EOF and values that can be
represented as 'unsigned char' -- that is, a cast is needed here to
avoid undefined behavior when 'char' is signed and a character in the
string might be sign-extended. (As an aside, I think this API seems
obviously bad, but unfortunately this is what the standard says, and
some systems check this.)
Rather than adding casts everywhere, this changes all the code in gdb
that uses any <ctype.h> API to instead call the corresponding c-ctype
function.
Now, c-ctype has some limitations compared to <ctype.h>. It works as
if the C locale is in effect, so in theory some non-ASCII characters
may be misclassified. This would only affect a subset of character
sets, though, and in most places I think ASCII is sufficient -- for
example the many places in gdb that check for whitespace.
Furthermore, in practice most users are using UTF-8-based locales,
where these functions aren't really informative for non-ASCII
characters anyway; see the existing workarounds in gdb/c-support.h.
Note that safe-ctype.h cannot be used because it causes conflicts with
readline.h. And, we canot poison the <ctype.h> identifiers as this
provokes errors from some libstdc++ headers.
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=33217
Approved-By: Simon Marchi <simon.marchi@efficios.com>
In some subsequent patches, solib_ops methods will need to access the
program space they were created for. We currently access the program
space using "current_program_space", but it would better to remember the
program space at construction time instead.
Change-Id: Icf2809435a23c47ddeeb75e603863b201eff2e58
Reviewed-By: Guinevere Larsen <guinevere@redhat.com>
I spotted this while reviewing a patch adding a new
gdbarch_software_single_step implementation. I find the name
"software_single_step" a bit misleading or unclear. It makes it sounds
as if the function executed a single step. In reality, this function
returns the possible next PCs for current instructions.
We have a similar concept in GDBserver:
linux_process_target::low_get_next_pcs. I like that name, it's clear
and straight to the point.
Rename gdbarch_software_single_step to gdbarch_get_next_pcs. I find
this name more indicative of what happens.
There is some code for ARM shared between GDB and GDBserver to implement
both sides, also called "get next pcs", so I think it all fits well
together.
Tested by rebuilding.
Change-Id: Ide74011a5034ba11117b7e7c865a093ef0b1dece
Approved-by: Kevin Buettner <kevinb@redhat.com>
Acked-by: Luis Machado <luis.machado.foss@gmail.com>
This reverts commit 14de1447c9.
An automated tester said that this patch caused a regression on
aarch64:
FAIL: gdb.arch/aarch64-atomic-inst.exp: Step through the ldxr/stxr sequence (timeout)
I looked into it a bit yesterday but couldn't see an obvious problem;
and it's somewhat of a pain to try to debug it at the moment.
Tom de Vries also noticed this and filed it in bugzilla. So, I'm
backing the patch out until I can port the failing test to the AdaCore
internal test suite in order to find out what went wrong.
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=28440
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=33255
When the PikeOS osabi sniffer was added, Pedro suggested that a target
could omit stepping from its vCont? reply packet to tell gdb that
software single-step must be used:
https://sourceware.org/legacy-ml/gdb-patches/2018-09/msg00312.html
This patch implements this idea by moving the call to
target_can_do_single_step into maybe_software_singlestep.
I've also removed some FIXME comments from gdbarch_components.py, and
slightly updated the documentation for gdbarch_software_single_step.
I think these comments are somewhat obsolete now that
target_can_do_single_step exists -- the current approach isn't exactly
what the comments intended, but on the other hand, it exists and
works.
Following review comments from Andrew, this version changes
record-full to use maybe_software_singlestep, and then combines
maybe_software_singlestep with insert_single_step_breakpoint.
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=28440
Convert solib_ops into an abstract base class (with abstract methods,
some of them with default implementations) and convert all the existing
solib_ops instances to solib_ops derived classes / implementations.
Prior to this patch, solib_ops is a structure holding function pointers,
of which there are only a handful of global instances (in the
`solib-*.c` files). When passing an `solib_ops *` around, it's a
pointer to one of these instances. After this patch, there are no more
global solib_ops instances. Instances are created as needed and stored
in struct program_space. These instances could eventually be made to
contain the program space-specific data, which is currently kept in
per-program space registries (I have some pending patches for that).
Prior to this patch, `gdbarch_so_ops` is a gdbarch method that returns a
pointer to the appropriate solib_ops implementation for the gdbarch.
This is replaced with the `gdbarch_make_solib_ops` method, which returns
a new instance of the appropriate solib_ops implementation for this
gdbarch. This requires introducing some factory functions for the
various solib_ops implementation, to be used as `gdbarch_make_solib_ops`
callbacks. For instance:
solib_ops_up
make_linux_ilp32_svr4_solib_ops ()
{
return std::make_unique<linux_ilp32_svr4_solib_ops> ();
}
The previous code is full of cases of tdep files copying some base
solib_ops implementation, and overriding one or more function pointer
(see ppc_linux_init_abi, for instance). I tried to convert all of this
is a class hierarchy. I like that it's now possible to get a good
static view of all the existing solib_ops variants. The hierarchy looks
like this:
solib_ops
├── aix_solib_ops
├── darwin_solib_ops
├── dsbt_solib_ops
├── frv_solib_ops
├── rocm_solib_ops
├── svr4_solib_ops
│ ├── ilp32_svr4_solib_ops
│ ├── lp64_svr4_solib_ops
│ ├── linux_ilp32_svr4_solib_ops
│ │ ├── mips_linux_ilp32_svr4_solib_ops
│ │ └── ppc_linux_ilp32_svr4_solib_ops
│ ├── linux_lp64_svr4_solib_ops
│ │ └── mips_linux_lp64_svr4_solib_ops
│ ├── mips_nbsd_ilp32_svr4_solib_ops
│ ├── mips_nbsd_lp64_svr4_solib_ops
│ ├── mips_fbsd_ilp32_svr4_solib_ops
│ └── mips_fbsd_lp64_svr4_solib_ops
└── target_solib_ops
└── windows_solib_ops
The solib-svr4 code has per-arch specialization to provide a
link_map_offsets, containing the offsets of the interesting fields in
`struct link_map` on that particular architecture. Prior to this patch,
arches would set a callback returning the appropriate link_map_offsets
by calling `set_solib_svr4_fetch_link_map_offsets`, which also happened
to set the gdbarch's so_ops to `&svr_so_ops`. I converted this to an
abstract virtual method of `struct svr4_solib_ops`, meaning that all
classes deriving from svr4_solib_ops must provide a method returning the
appropriate link_map_offsets for the architecture. I renamed
`set_solib_svr4_fetch_link_map_offsets` to `set_solib_svr4_ops`. This
function is still necessary because it also calls
set_gdbarch_iterate_over_objfiles_in_search_order, but if it was not for
that, we could get rid of it.
There is an instance of CRTP in mips-linux-tdep.c, because both
mips_linux_ilp32_svr4_solib_ops and mips_linux_lp64_svr4_solib_ops need
to derive from different SVR4 base classes (linux_ilp32_svr4_solib_ops
and linux_lp64_svr4_solib_ops), but they both want to override the
in_dynsym_resolve_code method with the same implementation.
The solib_ops::supports_namespaces method is new: the support for
namespaces was previously predicated by the presence or absence of a
find_solib_ns method. It now needs to be explicit.
There is a new progspace::release_solib_ops method, which is only needed
for rocm_solib_ops. For the moment, rocm_solib_ops replaces and wraps
the existing svr4_solib_ops instance, in order to combine the results of
the two. The plan is to have a subsequent patch to allow program spaces to have
multiple solib_ops, removing the need for release_solib_ops.
Speaking of rocm_solib_ops: it previously overrode only a few methods by
copying svr4_solib_ops and overwriting some function pointers. Now, it
needs to implement all the methods that svr4_solib_ops implements, in
order to forward the call. Otherwise, the default solib_ops method
would be called, hiding the svr4_solib_ops implementation. Again, this
can be removed once we have support for multiple solib_ops in a
program_space.
There is also a small change in how rocm_solib_ops is activated. Prior
to this patch, it's done at the end of rocm_update_solib_list. Since it
overrides the function pointer in the static svr4_solib_ops, and then
overwrites the host gdbarch, so_ops field, it's something that happens
only once. After the patch though, we need to set rocm_solib_ops in all
the program spaces that appear. We do this in
rocm_solib_target_inferior_created and in the new
rocm_solib_target_inferior_execd. After this, I will explore doing a
change where rocm_solib_ops is only set when we detect the ROCm runtime
is loaded.
Change-Id: I5896b5bcbf8bdb024d67980380feba1ffefaa4c9
Approved-By: Pedro Alves <pedro@palves.net>
The subsequent C++ification patch in this series will allocate one
instance of solib_ops per program space. That instance will be held in
struct program_space. As a small step towards this, add an `solib_ops
*` field to `struct program_space`. This field represents the solib_ops
currently used to manage the solibs in that program space. Initialize
it with the result of `gdbarch_so_ops` in `post_create_inferior`, and
use it whenever we need to do some solib stuff, rather than using
`gdbarch_so_ops` directly.
The difficulty here is knowing when exactly to set and unset the solib
ops. What I have here passes the testsuite on Linux, but with more
testing we will probably discover more spots where it's needed.
The C++ification patch will turn this field into a unique pointer.
With this patch, the message we get when running "info
linker-namespaces" becomes always the same, so update the test in
gdb.base/dlmopen-ns-ids.exp.
Change-Id: Ide8ddc57328895720fcd645d46dc34491f84c656
Approved-By: Pedro Alves <pedro@palves.net>
Reviewed-By: Guinevere Larsen <guinevere@redhat.com>
This patch introduces a new macro, INIT_GDB_FILE. This is used to
replace the current "_initialize_" idiom when introducing a per-file
initialization function. That is, rather than write:
void _initialize_something ();
void
_initialize_something ()
{
...
}
... now you would write:
INIT_GDB_FILE (something)
{
...
}
The macro handles both the declaration and definition of the function.
The point of this approach is that it makes it harder to accidentally
cause an initializer to be omitted; see commit 2711e475 ("Ensure
cooked_index_entry self-tests are run"). Specifically, the regexp now
used by make-init-c seems harder to trick.
New in v2: un-did some erroneous changes made by the script.
The bulk of this patch was written by script.
Regression tested on x86-64 Fedora 41.
I don't think that the file solist.h is useful. It would make sense to
have `struct solib` in solib.h. And then, all that would remain is
`struct solib_ops` and some solib-related function declarations. So,
move it all to solib.h.
Change-Id: I20ecf19787c378066f2c7a6a8a737c1db7c55d9a
Reviewed-By: Guinevere Larsen <guinevere@redhat.com>
This updates the copyright headers to include 2025. I did this by
running gdb/copyright.py and then manually modifying a few files as
noted by the script.
Approved-By: Eli Zaretskii <eliz@gnu.org>
I ran codespell on gdb/*.[chyl] and fixed a bunch of simple typos.
Most of what remains is trickier, i.e., spots where a somewhat natural
name of something in the code is flagged as a typo.
Reviewed-By: Tom de Vries <tdevries@suse.de>
This C++-ifies iterate_over_threads, changing it to accept a
gdb::function_view and to return bool.
Approved-By: Simon Marchi <simon.marchi@efficios.com>
The amd-dbgapi library, used in the AMD GPU port, has the capability to
prepare and cleanup displaced step operations. In order to use it, add
the following target_ops methods:
- supports_displaced_step
- displaced_step_prepare
- displaced_step_finish
- displaced_step_restore_all_in_ptid
Prior to this patch, displaced stepping preparation and cleanup is done
solely by gdbarches. Update infrun to use these new target methods
instead of gdbarch hooks. To keep the behavior for other architectures
unchanged, make the default implementations of the new target_ops method
forward to the thread's gdbarch.
displaced_step_restore_all_in_ptid won't be needed for the AMD GPU port,
but was added for completeness. It would be weird for infrun displaced
stepping code to call target methods except for that one thing where it
calls a gdbarch method.
Since this patch only adds infrastructure, no behavior change is
expected.
Change-Id: I07c68dddb5759a55cd137a711d2679eedc0d9285
Boolify the field. The 'set_stop_requested' function was already
taking a bool parameter, whose value is assigned to the field.
Approved-By: Andrew Burgess <aburgess@redhat.com>
This adds a new "command" style that is used when styling the name of
a gdb command.
Note that not every instance of a command name that is output by gdb
is changed here. There is currently no way to style error() strings,
and there is no way to mark up command help strings.
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=31747
Reviewed-By: Eli Zaretskii <eliz@gnu.org>
Reviewed-By: Keith Seitz <keiths@redhat.com>
Approved-By: Andrew Burgess <aburgess@redhat.com>
Eli mentioned [1] that given that we use US English spelling in our
documentation, we should use "behavior" instead of "behaviour".
In wikipedia-common-misspellings.txt there's a rule:
...
behavour->behavior, behaviour
...
which leaves this as a choice.
Add an overriding rule to hardcode the choice to common-misspellings.txt:
...
behavour->behavior
...
and add a rule to rewrite behaviour into behavior:
...
behaviour->behavior
...
and re-run spellcheck.sh on gdb*.
Tested on x86_64-linux.
[1] https://sourceware.org/pipermail/gdb-patches/2024-November/213371.html
I (Andrew) have split this small change from a larger patch which was
posted here:
https://inbox.sourceware.org/gdb-patches/AS1PR01MB9465608EBD5D62642C51C428E4922@AS1PR01MB9465.eurprd01.prod.exchangelabs.com
And I have written the stand alone test for this issue. The original
patch included this paragraph to explain this change (I've fixed one
typo in this text replacing 'program' with 'function'):
... it may happen that the infrun machinery steps from one inline
range to another inline range of the same inline function. That can
look like jumping back and forth from the calling function to the
inline function, while really the inline function just jumps from a
hot to a cold section of the code, i.e. error handling.
The important thing that happens here is that both the outer function
and the inline function must both have multiple ranges. When the
inferior is within the inline function and moves from one range to
another it is critical that the address we stop at is the start of a
range in both the outer function and the inline function.
The diagram below represents how the functions are split and aligned:
(A) (B)
bar: |------------| |---|
foo: |------------------| |--------|
The inferior is stepping through 'bar' and eventually reaches
point (A) at which point control passes to point (B).
Currently, when the inferior stops, GDB notices that both 'foo' and
'bar' start at address (B), and so GDB uses the inline frame mechanism
to skip 'bar' and tells the user that the inferior is in 'foo'.
However, as we were in 'bar' before the step then it makes sense that
we should be in 'bar' after the step, and this is what the patch does.
There are two tests using the DWARF assembler, the first checks the
above situation and ensures that GDB reports 'bar' after the step.
The second test is similar, but after the step we enter a new range
where a different inline function starts, something like this:
(A) (B)
bar: |------------|
baz: |---|
foo: |------------------| |--------|
In this case as we step at (A) and land at (B) we leave 'bar' and
expect to stop in 'foo', GDB shouldn't automatically enter 'baz' as
that is a completely different inline function. And this is, indeed,
what we see.
Co-Authored-By: Andrew Burgess <aburgess@redhat.com>
In a record session, when we move backward, GDB switches from normal
execution to simulation. Moving forward again, the emulation continues
until the end of the reverse history. When the end is reached, the
execution stops, and a warning message is shown. This message has been
modified to indicate that the forward emulation has reached the end, but
the execution can continue as normal, and the recording will also continue.
Before this patch, the warning message shown in that case was the same as
in the reverse case. This meant that when the end of history was reached in
either backward or forward emulation, the same message was displayed:
"No more reverse-execution history."
This message has changed for these two cases. Backward emulation:
"Reached end of recorded history; stopping.
Backward execution from here not possible."
Forward emulation:
"Reached end of recorded history; stopping.
Following forward execution will be added to history."
The reason for this change is that the initial message was deceiving, for
the forward case, making the user believe that forward debugging could not
continue.
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=31224
Reviewed-By: Markus T. Metzger <markus.t.metzger@intel.com> (btrace)
Approved-By: Guinevere Larsen <blarsen@redhat.com>
Commit bf2813aff8 introduced some logic to
not refresh the step frame id if it detects that the inferior is reverse
stepping out of a recursive call, so that we would still print frame
information once the inferior stops.
However, that logic was overly specific, and wouldn't be hit for
inferiors compiled with clang because clang adds line table entries that
aren't statements, making process_event_stop_test go through a different
branch on the relevant if statement.
Fix this by not making the code that detects "reversing out of a
recursion" an else clause to the previous if, but a standalone if block.
Approved-by: Kevin Buettner <kevinb@redhat.com>
Consider the following test-case:
...
$ cat -n test.c
1 int var;
2
3 int
4 foo (void)
5 {
6 var = 1;
7 #include "test.h"
8 }
9
10 int
11 main ()
12 {
13 return foo ();
14 }
$ cat -n test.h
1 return 1;
$ gcc test.c -g
...
When stepping through the test-case, gdb doesn't make it explicit that line 1
is not in test.c:
...
Temporary breakpoint 1, main () at test.c:13
13 return foo ();
(gdb) step
foo () at test.c:6
6 var = 1;
(gdb) n
1 return 1;
(gdb)
8 }
(gdb)
...
which makes it easy to misinterpret the output.
This is with the default "print frame-info" == auto, with documented
behaviour [1]:
...
stepi will switch between source-line and source-and-location depending on the
program counter.
...
What is actually implemented is that source-line is used unless stepping into
or out of a function.
The problem can be worked around by using
"set print frame-info source-and-location", but that's a bit verbose.
Instead, change the behaviour of "print frame-info" == auto to also use
source-and-location when stepping into another file, which gets us:
...
(gdb) n
foo () at test.h:1
1 return 1;
...
Tested on x86_64-linux.
Reviewed-By: Kevin Buettner <kevinb@redhat.com>
Reviewed-By: Kévin Le Gouguec <legouguec@adacore.com>
PR gdb/32011
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=32011
[1] https://sourceware.org/gdb/current/onlinedocs/gdb.html/Print-Settings.html#index-set-print-frame_002dinfo
Make the current program space reference bubble up one level. Pass
`current_program_space` everywhere, except in some cases where we can
get the pspace another way, and it's relatively obvious that it's the
same as the current program space.
Change-Id: Id86b79f1e44f92a398f49d137d57457174dfa96d
Approved-By: Tom Tromey <tom@tromey.com>
Reviewed-By: Thiago Jung Bauermann <thiago.bauermann@linaro.org>
The `no_shared_libraries` function is currently used to implement the
`nosharedlibrary` command, but it also used internally by other
functions. This does not make a very good internal API.
Add the `no_shared_libraries_command` function to implement the CLI
command. Remove the unused parameters from `no_shared_libraries`.
Remove the `from_tty` parameter of `target_pre_inferior`, since it's now
unused.
Change-Id: I4fcba5ee1e0f7d250aab1a7b62b9ea16265fe962
Approved-By: Tom Tromey <tom@tromey.com>
Reviewed-By: Thiago Jung Bauermann <thiago.bauermann@linaro.org>
Remove some includes reported as unused by clangd. Add some includes in
other files that were previously relying on the transitive include.
Change-Id: Ibdd0a998b04d21362a20d0ca8e5267e21e2e133e
Since the frame variable is now a frame_info_ptr, the issue
with the dangling frame pointer is apparently no longer there.
So remove the re-fetch code and the corresponding meanwhile
misleading comments.
Approved-By: Tom Tromey <tom@tromey.com>
Remove the gdbcmd.h, which is reported as unused by clangd. Add
cli/cli-cmds.h instead, to get access to `cmdlist` and friends.
Change-Id: Ic0c60d2f6d3618f1bd9fd80b95ffd7c33c692a04
Now that defs.h, server.h and common-defs.h are included via the
`-include` option, it is no longer necessary for source files to include
them. Remove all the inclusions of these files I could find. Update
the generation scripts where relevant.
Change-Id: Ia026cff269c1b7ae7386dd3619bc9bb6a5332837
Approved-By: Pedro Alves <pedro@palves.net>
This commit fixes bug PR 28942, that is, creating a conditional
breakpoint in a multi-threaded inferior, where the breakpoint
condition includes an inferior function call.
Currently, when a user tries to create such a breakpoint, then GDB
will fail with:
(gdb) break infcall-from-bp-cond-single.c:61 if (return_true ())
Breakpoint 2 at 0x4011fa: file /tmp/build/gdb/testsuite/../../../src/gdb/testsuite/gdb.threads/infcall-from-bp-cond-single.c, line 61.
(gdb) continue
Continuing.
[New Thread 0x7ffff7c5d700 (LWP 2460150)]
[New Thread 0x7ffff745c700 (LWP 2460151)]
[New Thread 0x7ffff6c5b700 (LWP 2460152)]
[New Thread 0x7ffff645a700 (LWP 2460153)]
[New Thread 0x7ffff5c59700 (LWP 2460154)]
Error in testing breakpoint condition:
Couldn't get registers: No such process.
An error occurred while in a function called from GDB.
Evaluation of the expression containing the function
(return_true) will be abandoned.
When the function is done executing, GDB will silently stop.
Selected thread is running.
(gdb)
Or, in some cases, like this:
(gdb) break infcall-from-bp-cond-simple.c:56 if (is_matching_tid (arg, 1))
Breakpoint 2 at 0x401194: file /tmp/build/gdb/testsuite/../../../src/gdb/testsuite/gdb.threads/infcall-from-bp-cond-simple.c, line 56.
(gdb) continue
Continuing.
[New Thread 0x7ffff7c5d700 (LWP 2461106)]
[New Thread 0x7ffff745c700 (LWP 2461107)]
../../src.release/gdb/nat/x86-linux-dregs.c:146: internal-error: x86_linux_update_debug_registers: Assertion `lwp_is_stopped (lwp)' failed.
A problem internal to GDB has been detected,
further debugging may prove unreliable.
The precise error depends on the exact thread state; so there's race
conditions depending on which threads have fully started, and which
have not. But the underlying problem is always the same; when GDB
tries to execute the inferior function call from within the breakpoint
condition, GDB will, incorrectly, try to resume threads that are
already running - GDB doesn't realise that some threads might already
be running.
The solution proposed in this patch requires an additional member
variable thread_info::in_cond_eval. This flag is set to true (in
breakpoint.c) when GDB is evaluating a breakpoint condition.
In user_visible_resume_ptid (infrun.c), when the in_cond_eval flag is
true, then GDB will only try to resume the current thread, that is,
the thread for which the breakpoint condition is being evaluated.
This solves the problem of GDB trying to resume threads that are
already running.
The next problem is that inferior function calls are assumed to be
synchronous, that is, GDB doesn't expect to start an inferior function
call in thread #1, then receive a stop from thread #2 for some other,
unrelated reason. To prevent GDB responding to an event from another
thread, we update fetch_inferior_event and do_target_wait in infrun.c,
so that, when an inferior function call (on behalf of a breakpoint
condition) is in progress, we only wait for events from the current
thread (the one evaluating the condition).
In do_target_wait I had to change the inferior_matches lambda
function, which is used to select which inferior to wait on.
Previously the logic was this:
auto inferior_matches = [&wait_ptid] (inferior *inf)
{
return (inf->process_target () != nullptr
&& ptid_t (inf->pid).matches (wait_ptid));
};
This compares the pid of the inferior against the complete ptid we
want to wait on. Before this commit wait_ptid was only ever
minus_one_ptid (which is special, and means any process), and so every
inferior would match.
After this commit though wait_ptid might represent a specific thread
in a specific inferior. If we compare the pid of the inferior to a
specific ptid then these will not match. The fix is to compare
against the pid extracted from the wait_ptid, not against the complete
wait_ptid itself.
In fetch_inferior_event, after receiving the event, we only want to
stop all the other threads, and call inferior_event_handler with
INF_EXEC_COMPLETE, if we are not evaluating a conditional breakpoint.
If we are, then all the other threads should be left doing whatever
they were before. The inferior_event_handler call will be performed
once the breakpoint condition has finished being evaluated, and GDB
decides to stop or not.
The final problem that needs solving relates to GDB's commit-resume
mechanism, which allows GDB to collect resume requests into a single
packet in order to reduce traffic to a remote target.
The problem is that the commit-resume mechanism will not send any
resume requests for an inferior if there are already events pending on
the GDB side.
Imagine an inferior with two threads. Both threads hit a breakpoint,
maybe the same conditional breakpoint. At this point there are two
pending events, one for each thread.
GDB selects one of the events and spots that this is a conditional
breakpoint, GDB evaluates the condition.
The condition includes an inferior function call, so GDB sets up for
the call and resumes the one thread, the resume request is added to
the commit-resume queue.
When the commit-resume queue is committed GDB sees that there is a
pending event from another thread, and so doesn't send any resume
requests to the actual target, GDB is assuming that when we wait we
will select the event from the other thread.
However, as this is an inferior function call for a condition
evaluation, we will not select the event from the other thread, we
only care about events from the thread that is evaluating the
condition - and the resume for this thread was never sent to the
target.
And so, GDB hangs, waiting for an event from a thread that was never
fully resumed.
To fix this issue I have added the concept of "forcing" the
commit-resume queue. When enabling commit resume, if the force flag
is true, then any resumes will be committed to the target, even if
there are other threads with pending events.
A note on authorship: this patch was based on some work done by
Natalia Saiapova and Tankut Baris Aktemur from Intel[1]. I have made
some changes to their work in this version.
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=28942
[1] https://sourceware.org/pipermail/gdb-patches/2020-October/172454.html
Co-authored-by: Natalia Saiapova <natalia.saiapova@intel.com>
Co-authored-by: Tankut Baris Aktemur <tankut.baris.aktemur@intel.com>
Reviewed-By: Tankut Baris Aktemur <tankut.baris.aktemur@intel.com>
Tested-By: Luis Machado <luis.machado@arm.com>
Tested-By: Keith Seitz <keiths@redhat.com>
This reverts commit ac0d67ed1d.
There was nothing wrong with the commit which I'm reverting here, but
it removed some functionality that will be needed for a later commit;
that is, the ability for GDB to ask for events from a specific ptid_t
via the do_target_wait function.
In a follow up commit, this functionality will be used to implement
inferior function calls in multi-threaded inferiors.
This is not a straight revert of the above commit. Reverting the
above commit replaces a 'nullptr' with 'NULL', I've gone in and
changed that, preserving the 'nullptr'.
Reviewed-By: Tankut Baris Aktemur <tankut.baris.aktemur@intel.com>
Tested-By: Luis Machado <luis.machado@arm.com>
Tested-By: Keith Seitz <keiths@redhat.com>
We currently pass frames to function by value, as `frame_info_ptr`.
This is somewhat expensive:
- the size of `frame_info_ptr` is 64 bytes, which is a bit big to pass
by value
- the constructors and destructor link/unlink the object in the global
`frame_info_ptr::frame_list` list. This is an `intrusive_list`, so
it's not so bad: it's just assigning a few points, there's no memory
allocation as if it was `std::list`, but still it's useless to do
that over and over.
As suggested by Tom Tromey, change many function signatures to accept
`const frame_info_ptr &` instead of `frame_info_ptr`.
Some functions reassign their `frame_info_ptr` parameter, like:
void
the_func (frame_info_ptr frame)
{
for (; frame != nullptr; frame = get_prev_frame (frame))
{
...
}
}
I wondered what to do about them, do I leave them as-is or change them
(and need to introduce a separate local variable that can be
re-assigned). I opted for the later for consistency. It might not be
clear why some functions take `const frame_info_ptr &` while others take
`frame_info_ptr`. Also, if a function took a `frame_info_ptr` because
it did re-assign its parameter, I doubt that we would think to change it
to `const frame_info_ptr &` should the implementation change such that
it doesn't need to take `frame_info_ptr` anymore. It seems better to
have a simple rule and apply it everywhere.
Change-Id: I59d10addef687d157f82ccf4d54f5dde9a963fd0
Approved-By: Andrew Burgess <aburgess@redhat.com>
By inspection, I believe that breakpoint_init_inferior doesn't call
anything that relies on the current program space or inferior. So,
add an inferior parameter, to make the current inferior / program space
references bubble up one level.
Change-Id: Ib07b7a6d360e324f6ae1aa502dd314b8cce421b7
Approved-By: Andrew Burgess <aburgess@redhat.com>
Make the current_program_space reference bubble up one level.
Change-Id: Idc8ed78d23bf3bb2969f6963d8cc049f26901c29
Approved-By: Andrew Burgess <aburgess@redhat.com>
This code was probably needed before we had reinflatable
frame_info_ptrs, it's not necessary anymore.
Change-Id: I5474c6081ee1e39624c9266b05dbe01351a130b5
Approved-By: Tom Tromey <tom@tromey.com>
This changes lookup_symbol and associated APIs to accept
domain_search_flags rather than a domain_enum.
Note that this introduces some new constants to Python and Guile. I
chose to break out the documentation patch for this, because the
internals here do not change until a later patch, and it seemed
simpler to patch the docs just once, rather than twice.
A recent(ish) change in gdb/infrun.c made process_event_stop_test load
debug information where it would not have done so previously. The
change is:
commit bf2813aff8
AuthorDate: Fri Sep 1 13:47:32 2023 +0200
CommitDate: Mon Nov 20 10:54:03 2023 +0100
gdb/record: print frame information when exiting a recursive call
Currently, when GDB is reverse stepping out of a function into the same
function due to a recursive call, it doesn't print frame information, as
reported by PR record/29178. This happens because when the inferior
leaves the current frame, GDB decides to refresh the step information,
clobbering the original step_frame_id, making it impossible to figure
out later on that the frame has been changed.
This commit changes GDB so that, if we notice we're in this exact
situation, we won't refresh the step information.
Because of implementation details, this change can cause some debug
information to be read when it normally wouldn't before, which showed up
as a regression on gdb.dwarf2/dw2-out-of-range-end-of-seq. Since that
isn't a problem, the test was changed to allow for the new output.
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=29178
Although there is nothing wrong with this change in principle, it
happens to break most of the tests in gdb/testsuite/gdb.rocm/*.exp.
This is because those tests do rely on GDB not loading debug
information. This is necessary because the debug information produced
for AMDGPU code is using DWARF extensions which are not supported by GDB
at this point.
In this patch, I propose to use a lazy loading mechanism so the frame_id
for the current frame is only computed when required instead of when
entering process_event_stop_test. The lazy_loader class is currently
defined locally in infrun.c, but if it turns out to be useful elsewhere,
it could go somewhere under gdbsupport.
This patch should restore the behavior GDB had before
bf2813aff8 when it comes to load debug
info.
Another approach could have been to revert
fb84fbf8a5 (gdb/infrun: simplify
process_event_stop_test) and adjust the implementation of
bf2813aff8 (gdb/record: print frame
information when exiting a recursive call). However, I think that the
lazy loading works well with the simplification done recently, so I went
down that route.
Regression tested on x86_64-linux (Ubuntu 22.04) with AMDGPU support.
Change-Id: Ib63a162128130d1786a77c98623e9e3dcbc363b7
Approved-by: Kevin Buettner <kevinb@redhat.com>
