Pedro Alves [Fri, 30 Aug 2024 15:02:21 +0000 (16:02 +0100)]
Add backpointer from windows_thread_info to windows_process_info
The next patch will move some duplicated code in gdb and gdbserver to
gdb/nat/windows-nat.c, where it would be convenient to get at the
Windows process info of a given Windows thread info, from within a
windows_thread_info method.
I first thought of passing down the windows_process_info pointer as
argument to the windows_thread_info method, but that looked a bit odd.
I think it looks better to just add a back pointer, so that's what
this patch does. The following patch will then add a use of it.
I suspect this will help moving more duplicated code to
gdb/nat/windows-nat.c in the future, too.
Approved-By: Tom Tromey <tom@tromey.com>
Change-Id: I47fc0d3323be5b6f6fcfe912b768051a41910666
Pedro Alves [Mon, 22 May 2023 16:33:16 +0000 (17:33 +0100)]
Windows gdb+gdbserver: Make siginfo_er per-thread state
With non-stop mode support, each thread has its own "last event", and
so printing $_siginfo should print the siginfo of the selected thread.
Likewise, with all-stop and scheduler-locking.
This patch reworks the siginfo functions in gdb/windows-nat.c and
gdbserver/win32-low.cc to reuse the exception record already saved
within each thread's 'last_event' field.
Here's an example of what you'll see after the whole non-stop series:
This was in non-stop mode, and the program originally had two threads.
Thread 1 stopped for a breakpoint, then thread 2 was manually
interrupted/paused and then single-stepped. And then I typed Ctrl-C
in the inferior's terminal, which made Windows inject thread 3 in the
inferior, and report a DBG_CONTROL_C exception for it.
Pedro Alves [Thu, 11 May 2023 17:41:27 +0000 (18:41 +0100)]
Windows gdbserver: Eliminate soft-interrupt mechanism
I noticed that faked_breakpoint is write only. And then I hacked
win32_process_target::request_interrupt to force it to stop threads
using the soft_interrupt_requested mechanism (which suspends threads,
and then fakes a breakpoint event in the main thread), and saw that it
no longer works -- gdbserver crashes accessing a NULL current_thread,
because fake_breakpoint_event does not switch to a thread.
This code was originally added for Windows CE, as neither
GenerateConsoleCtrlEvent nor DebugBreakProcess worked there. Windows
CE support has since been removed.
We nowadays require Windows XP or later, and XP has DebugBreakProcess.
The soft_interrupt_requested mechanism has other problems, like for
example faking the event in the main thread, even if that thread was
previously stopped, due to scheduler-locking.
A following patch will add a similar mechanism stopping all threads
with SuspendThread to native GDB, for non-stop mode, which doesn't
have these problems. It's different enough from this old code that I
think we should just rip the old code out, and reimplement it from
scratch (based on gdb's version) when we need it.
Approved-By: Tom Tromey <tom@tromey.com>
Change-Id: I89e98233a9c40c6dcba7c8e1dacee08603843fb1
Pedro Alves [Thu, 11 May 2023 22:07:33 +0000 (23:07 +0100)]
Windows gdb: Enable "set scheduler-locking on"
Surprisingly (to me), enabling scheduler locking on Windows currently
fails:
(gdb)
set scheduler-locking on
Target 'native' cannot support this command.
The backend itself does support scheduler-locking. This patch
implements windows_nat_target::get_thread_control_capabilities so that
the core knows schedlocking works for this target.
Approved-By: Tom Tromey <tom@tromey.com>
Change-Id: Ie762d3768fd70e4ac398c8bcc03c3213bfa26a6a
Pedro Alves [Thu, 11 May 2023 11:15:36 +0000 (12:15 +0100)]
Windows gdbserver: Fix scheduler-locking
This rewrites win32_process_target::resume such that scheduler-locking
is implemented properly.
It also uses the new get_last_debug_event_ptid function to avoid
considering passing a signal to the wrong thread, like done for the
native side in a previous patch.
Note this code/comment being removed:
- /* Yes, we're ignoring resume_info[0].thread. It'd be tricky to make
- the Windows resume code do the right thing for thread switching. */
- tid = windows_process.current_event.dwThreadId;
This meant that scheduler-locking currently is broken badly unless you
stay in the thread that last reported an event. If you switch to a
different thread from the one that last reported an event and step,
you get a spurious SIGTRAP in the thread that last reported a stop,
not the one that you tried to step:
(gdb) t 1
[Switching to thread 1 (Thread 3908)]
#0 0x00007fffc768d6e4 in ntdll!ZwDelayExecution () from target:C:/Windows/SYSTEM32/ntdll.dll
(gdb) set scheduler-locking on
(gdb) set disassemble-next-line on
(gdb) frame
#0 0x00007fffc768d6e4 in ntdll!ZwDelayExecution () from target:C:/Windows/SYSTEM32/ntdll.dll
=> 0x00007fffc768d6e4 <ntdll!ZwDelayExecution+20>: c3 ret
(gdb) si
Thread 3 received signal SIGTRAP, Trace/breakpoint trap.
[Switching to Thread 2660]
0x00007fffc4e4e92e in KERNELBASE!EncodeRemotePointer () from target:C:/Windows/System32/KernelBase.dll
=> 0x00007fffc4e4e92e <KERNELBASE!EncodeRemotePointer+8254>: eb 78 jmp 0x7fffc4e4e9a8 <KERNELBASE!EncodeRemotePointer+8376>
(gdb)
Note how we switched to thread 1, stepped, and GDBserver still stepped
thread 3... This is fixed by this patch. We now get:
(gdb) info threads
Id Target Id Frame
1 Thread 920 0x00007ffe0372d6e4 in ntdll!ZwDelayExecution () from target:C:/Windows/SYSTEM32/ntdll.dll
2 Thread 8528 0x00007ffe03730ad4 in ntdll!ZwWaitForWorkViaWorkerFactory () from target:C:/Windows/SYSTEM32/ntdll.dll
3 Thread 3128 0x00007ffe03730ad4 in ntdll!ZwWaitForWorkViaWorkerFactory () from target:C:/Windows/SYSTEM32/ntdll.dll
* 4 Thread 7164 0x00007ffe0102e929 in KERNELBASE!EncodeRemotePointer () from target:C:/Windows/System32/KernelBase.dll
5 Thread 8348 0x00007ffe0372d6e4 in ntdll!ZwDelayExecution () from target:C:/Windows/SYSTEM32/ntdll.dll
6 Thread 2064 0x00007ffe0372d6e4 in ntdll!ZwDelayExecution () from target:C:/Windows/SYSTEM32/ntdll.dll
(gdb) t 1
[Switching to thread 1 (Thread 920)]
#0 0x00007ffe0372d6e4 in ntdll!ZwDelayExecution () from target:C:/Windows/SYSTEM32/ntdll.dll
(gdb) set scheduler-locking on
(gdb) si
0x00007ffe0372d6e4 in ntdll!ZwDelayExecution () from target:C:/Windows/SYSTEM32/ntdll.dll
(gdb) si
0x00007ffe00f9b44e in SleepEx () from target:C:/Windows/System32/KernelBase.dll
(gdb) si
0x00007ffe00f9b453 in SleepEx () from target:C:/Windows/System32/KernelBase.dll
I.e., we kept stepping the right thread, thread 1.
Note we stopped again at 0x00007ffe0372d6e4 the first time (same PC
the thread already was at before the first stepi) because the thread
had been stopped at a syscall, so that's normal:
(gdb) disassemble
Dump of assembler code for function ntdll!ZwDelayExecution:
0x00007ffe0372d6d0 <+0>: mov %rcx,%r10
0x00007ffe0372d6d3 <+3>: mov $0x34,%eax
0x00007ffe0372d6d8 <+8>: testb $0x1,0x7ffe0308
0x00007ffe0372d6e0 <+16>: jne 0x7ffe0372d6e5 <ntdll!ZwDelayExecution+21>
0x00007ffe0372d6e2 <+18>: syscall
=> 0x00007ffe0372d6e4 <+20>: ret
Pedro Alves [Thu, 11 May 2023 11:27:27 +0000 (12:27 +0100)]
Windows gdb: Can't pass signal to thread other than last stopped thread
Passing a signal to a thread other than the one that last reported an
event will be later possible with DBG_REPLY_LATER and the Windows
backend working in non-stop mode.
With an all-stop backend that isn't possible, so at least don't
incorrectly consider passing DBG_EXCEPTION_NOT_HANDLED if the thread
that we're going to call ContinueDebugEvent for is not the one that
the user issued "signal SIG" on.
Pedro Alves [Tue, 7 May 2024 15:04:50 +0000 (16:04 +0100)]
Windows gdb+gdbserver: Elim desired_stop_thread_id / rework pending_stops
windows_process.desired_stop_thread_id doesn't work for non-stop, as
in that case every thread will have its own independent
WaitForDebugEvent event.
Instead, detect whether we have been reported a stop that was not
supposed to be reported by simply checking whether the thread that is
reporting the event is suspended. This is now easilly possible since
each thread's suspend state is kept in sync with whether infrun wants
the thread executing or not.
windows_process.desired_stop_thread_id was also used as thread to pass
to windows_continue. However, we don't really need that either.
windows_continue is used to update the thread's registers, unsuspend
them, and then finally call ContinueDebugEvent. In most cases, we
only need the ContinueDebugEvent step, so we can convert the
windows_continue calls to continue_last_debug_event_main_thread calls.
The exception is when we see a thread creation event -- in that case,
we need to update the debug registers of the new thread. We can use
continue_one_thread for that.
Since the pending stop is now stored in windows_thread_info,
get_windows_debug_event needs to avoid reaching the bottom code if
there's no thread associated with the event anymore (i.e.,
EXIT_THREAD_DEBUG_EVENT / EXIT_PROCESS_DEBUG_EVENT).
I considered whether it would be possible to keep the pending_stop
handling code shared in gdb/nat/windows-nat.c, in this patch and
throughout the series, but I conclused that it isn't worth it, until
gdbserver is taught about async and non-stop as well.
The pending_stop struct will eventually be eliminated later down the
series.
Pedro Alves [Thu, 18 May 2023 18:13:45 +0000 (19:13 +0100)]
Windows gdb: Pending stop and current_event
I noticed that windows_nat_target::get_windows_debug_event does not
copy the event recorded in pending stop to
windows_process.current_event. This seems like an oversight. The
equivalent code in gdbserver/win32-low.cc does copy it.
This change will become moot later in the series, but I figure its
still clearer to correct the buglet as preparatory patch.
Approved-By: Tom Tromey <tom@tromey.com>
Change-Id: Ic8935d854cf67a3a3c4edcbc1a1e8957b800d907
Pedro Alves [Tue, 9 May 2023 19:34:50 +0000 (20:34 +0100)]
Windows gdb: Factor code out of windows_nat_target::windows_continue
This factors some code out of windows_nat_target::windows_continue
into a new windows_continue_one function. This will make the
following patch easier to read (as well as the resulting code itself).
Approved-By: Tom Tromey <tom@tromey.com>
Change-Id: I14a0386b1b8b03015e86273060af173b5130e375
Pedro Alves [Thu, 21 Oct 2021 17:16:58 +0000 (18:16 +0100)]
Windows gdb: Introduce windows_continue_flags
windows_continue already has two boolean parameters:
(..., int killed, bool last_call = false)
A patch later in the series would need a third. Instead, convert
windows_continue to use an optional enum-flags parameter instead of
multiple booleans.
Approved-By: Tom Tromey <tom@tromey.com>
Change-Id: I17c4d8a12b662190f972c380f838cb3317bd2e1e
Pedro Alves [Thu, 21 Oct 2021 17:16:58 +0000 (18:16 +0100)]
Windows gdb: Introduce continue_last_debug_event_main_thread
We have code using do_synchronously to call continue_last_debug_event,
and later patches in the series would need to add the same code in few
more places. Factor it out to a continue_last_debug_event_main_thread
function so these other places in future patches can just call it.
In v2:
- Fix context_str not used in the body of the method.
Pedro Alves [Tue, 9 May 2023 09:27:04 +0000 (10:27 +0100)]
Windows gdb+gdbserver: Move suspending thread to when returning event
The current code suspends a thread just before calling
GetThreadContext. You can only call GetThreadContext if the thread is
suspended. But, after WaitForDebugEvent, all threads are implicitly
suspended. So I don't think we even needed to call SuspendThread
explictly at all before our GetThreadContext calls.
However, suspending threads when we're about to present a stop to gdb
simplifies adding non-stop support later. This way, the windows
SuspendThread state corresponds to whether a thread is suspended or
resumed from the core's perspective. Curiously, I noticed that Wine's
winedbg does something similar:
https://github.com/wine-mirror/wine/blob/234943344f7495d1e072338f0e06fa2d5cbf0aa1/programs/winedbg/gdbproxy.c#L651
This makes it much easier to reason about a thread's suspend state,
and simplifies adding non-stop mode later on.
Pedro Alves [Thu, 11 May 2023 12:16:09 +0000 (13:16 +0100)]
Windows gdb: Simplify windows_nat_target::wait
The logic in windows_nat_target::wait, where we decide what to do
depending on the result from get_windows_debug_event is harder to
grasp than it looks.
It is not easy to tell what should happen when in async mode
get_windows_debug_event returns that there's no event to process.
And then, if get_windows_debug_event returns null_ptid /
TARGET_WAITKIND_SPURIOUS, then we need to issue a ContinueDebugEvent.
There's also this comment in windows_nat_target::wait, which we're not
really implementing today:
~~~~
/* We loop when we get a non-standard exception rather than return
with a SPURIOUS because resume can try and step or modify things,
which needs a current_thread->h. But some of these exceptions mark
the birth or death of threads, which mean that the current thread
isn't necessarily what you think it is. */
~~~~
This patch changes things a bit so that the code is more obvious:
- look at the status kind, instead of ptid_t.
- add an explicit early return case for no-event.
- add an explicit case for TARGET_WAITKIND_SPURIOUS.
- with those, we no longer need to handle the case of find_thread not
finding a thread, so we can drop one indentation level.
Pedro Alves [Tue, 9 May 2023 09:13:08 +0000 (10:13 +0100)]
Windows gdb+gdbserver: Eliminate DONT_SUSPEND
There's a single call to thread_rec(DONT_SUSPEND), in
windows_process_info::handle_exception.
In GDB, the windows-nat.c thread_rec implementation avoids actually
calling SuspendThread on the event thread by doing:
th->suspended = -1;
I am not exactly sure why, but it kind of looks like it is done as an
optimization, avoiding a SuspendThread call? It is probably done for
the same reason as the code touched in the previous patch avoided
suspending the event thread.
This however gets in the way of non-stop mode, which will really want
to SuspendThread the event thread for DBG_REPLY_LATER.
In gdbserver's thread_rec implementation DONT_SUSPEND is ignored, and
thread_rec actually always suspends, which really suggests that
SuspendThread on the event thread is really not a problem. I really
can't imagine why it would be.
DONT_SUSPEND invalidates the thread's context, but there is no need to
invalidate the context when we get an event for a thread, because we
invalidate it when we previously resumed the thread.
So, we can just remove the thread_rec call from
windows_process_info::handle_exception. That's what this patch does.
Approved-By: Tom Tromey <tom@tromey.com>
Change-Id: I0f328542bda6d8268814ca1ee4ae7a478098ecf2
Pedro Alves [Mon, 8 May 2023 20:36:28 +0000 (21:36 +0100)]
Windows gdb+gdbserver: Eliminate thread_rec(INVALIDATE_CONTEXT) calls
Replace thread_rec(INVALIDATE_CONTEXT) calls with find_thread, and
invalidate_context / suspend calls in the spots that might need those.
I don't know why does the INVALIDATE_CONTEXT implementation in GDB
avoid suspending the event thread:
case INVALIDATE_CONTEXT:
if (ptid.lwp () != current_event.dwThreadId)
th->suspend ();
Checks for a global "current_event" get in the way of non-stop support
later in the series, as each thread will have its own "last debug
event". Regardless, it should be fine to suspend the event thread.
As a data point, the GDBserver implementation always suspends. So
this patch does not try to avoid suspending the event thread on the
native side either.
Approved-By: Tom Tromey <tom@tromey.com>
Change-Id: I8d2f0a749d23329956e62362a7007189902dddb5
Pedro Alves [Tue, 30 Apr 2024 14:33:58 +0000 (15:33 +0100)]
Windows gdb: Eliminate reload_context
We don't need reload_context, because we can get the same information
out of th->context.ContextFlags. If ContextFlags is zero, then we
need to fetch the context out of the inferior thread. This is what
gdbserver does too.
Approved-By: Tom Tromey <tom@tromey.com>
Change-Id: Ied566037c81383414c46c77713bdd1aec6377b23
Pedro Alves [Tue, 2 May 2023 19:42:35 +0000 (20:42 +0100)]
Windows gdb: Eliminate global current_process.dr[8] global
current_process.dr needs to be per-thread for non-stop. Actually, it
doesn't even need to exist at all. We have x86_debug_reg_state
recording intent, and then the
cygwin_get_dr/cygwin_get_dr6/cygwin_get_dr7 functions are registered
as x86_dr_low_type vector functions, so they should return the current
value in the inferior's registers. See this comment in x86-dregs.c:
~~~
/* In non-stop/async, threads can be running while we change the
global dr_mirror (and friends). Say, we set a watchpoint, and
let threads resume. Now, say you delete the watchpoint, or
add/remove watchpoints such that dr_mirror changes while threads
are running. On targets that support non-stop,
inserting/deleting watchpoints updates the global dr_mirror only.
It does not update the real thread's debug registers; that's only
done prior to resume. Instead, if threads are running when the
mirror changes, a temporary and transparent stop on all threads
is forced so they can get their copy of the debug registers
updated on re-resume. Now, say, a thread hit a watchpoint before
having been updated with the new dr_mirror contents, and we
haven't yet handled the corresponding SIGTRAP. If we trusted
dr_mirror below, we'd mistake the real trapped address (from the
last time we had updated debug registers in the thread) with
whatever was currently in dr_mirror. So to fix this, dr_mirror
always represents intention, what we _want_ threads to have in
debug registers. To get at the address and cause of the trap, we
need to read the state the thread still has in its debug
registers.
In sum, always get the current debug register values the current
thread has, instead of trusting the global mirror. If the thread
was running when we last changed watchpoints, the mirror no
longer represents what was set in this thread's debug
registers. */
~~~
This patch makes the Windows native target follow that model as well.
I don't understand why would windows_nat_target::resume want to call
SetThreadContext itself. That duplicates things as it is currently
worrying about setting the debug registers as well. windows_continue
also does that, and windows_nat_target::resume always calls it. So
this patch simplifies windows_nat_target::resume too.
Tromey pointed out that gdb/2388 mentioned in the code being removed
was moved to https://sourceware.org/bugzilla/show_bug.cgi?id=9493 in
the bugzilla migration. I tried the reproducer mentioned there, and
it still works correctly.
Pedro Alves [Tue, 2 May 2023 19:42:35 +0000 (20:42 +0100)]
Windows gdb: Dead code in windows_nat_target::do_initial_windows_stuff
In windows_nat_target::do_initial_windows_stuff, there's no point in
setting windows_process.current_event.dwProcessId. It's a nop, given
the following memset.
Approved-By: Tom Tromey <tom@tromey.com>
Change-Id: I2fe460341b598ad293ea60d5f702b10cefc30711
While working on Windows non-stop support, I ran into a
very-hard-to-track-down bug.
The problem turned out to be that
infrun.c:proceed_resume_thread_checked resumed an already-executing
thread because the thread was marked as "executing=true,
resumed=false", and that function only skips resuming threads that are
marked resumed=true. The consequence was that GDB corrupted the
registers of the Windows DLL loader threads, eventually leading to a
GDB+inferior deadlock.
Originally, the "resumed" flag was only ever set when infrun decided
is was ready to process a thread's pending wait status. infrun has
since evolved to set the resumed flag when we set a thread's executing
flag too. We are not always consistent throughout in guaranteeing
that a thread is marked resumed=true whenever it is marked
executing=true, though. For instance, no target code that supports
non-stop mode (linux-nat, remote, and windows-nat with this series) is
making sure that new threads are marked resumed=true when they are
added to the thread list. They are only marked as {state=running,
executing=true}, the "resumed" flag is not touched.
Making proceed_resume_thread_checked check thr->executing() in
addition to thr->resumed(), feels like papering over a combination of
states that shouldn't happen nowadays.
OTOH, having to have the target backends mark new threads as
resumed=true just feels like too many different states (three) to set:
We really have too many "state tracking" flags in a thread.
Basically:
- whether a thread is "running/stopped/exited" (from the user's
perspective). This is the thread_info::state field.
- whether a thread is "executing" (infrun asked the target to set the
thread executing). This is thread_info::executing().
- whether a thread is "resumed" (infrun wants the thread to be
resumed, but maybe can't yet because the thread has a pending wait
status). This is thread_info::resumed()
"running", "executing", and "resumed" are almost synonyms, so this can
be highly confusing English-wise too.
For "running" vs "executing", in comments, we tipically need to
explain that "running/stopped/exited" is for the user/frontend
perspective, while "executing true/false" is for gdb's internal run
control.
(Also, "executing or not" can also mean something else in GDB's
codebase -- "target has execution" does not mean that threads are
actually running right now -- it's a test for whether we have a live
process vs a core dump!)
One simplification we can do that avoids this running vs executing
ambiguity is to replace the "executing" field with an "internal_state"
field, similar to the thread_info::state field, and make that new
internal_state field reuse the same enum thread_state type that is
used by thread_info::state. Like:
struct thread_info
{
...
/* Frontend/public/external/user view of the thread state. */
enum thread_state m_state = THREAD_STOPPED;
/* The thread's internal state. When the thread is stopped
internally while handling an internal event, like a software
single-step breakpoint, the internal state will be
THREAD_STOPPED, but the external state will still be
THREAD_RUNNING. */
enum thread_state m_internal_state = THREAD_STOPPED;
};
(Assume we'd add state() and internal_state() getters.)
With that, every check for thr->executing() is replaced with a
'thr->internal_state() == THREAD_RUNNING' check, and the code is
clearer by design. There is no confusion between "running" vs
"executing" any more, because they now mean the exact same thing.
Instead, we say e.g., 'thread has (user) state "running", and internal
state "stopped"'. Or simpler, 'thread is running (from the user's
perspective), but internally stopped'. That is after all what we
would way in comments today already.
That still leaves the 'resumed' flag, though. That's the least
obvious one. Turns out we can get rid of it, and make it a new state
tracked by thread_info::internal_state. That is, we make
internal_state have its own enumeration type (decoupled from
thread_info::state's type), and convert the resumed true/false flag to
a new enumerator of this new enumeration. Like so:
The patch adds getters/setters for both (user) state and
internal_state, and adds assertions around state transitions, ensuring
that internal_state doesn't get out of sync with
thread::have_pending_wait_status(). It also adds an assertion to
clear_proceed_status_thread, making sure that we don't try to proceed
a thread that is already running. Turns out that catches
attach_command calling init_wait_for_inferior too late, after
attaching has already created already-running threads.
The code that adds/removes threads from the proc_target's
resumed_with_pending_wait_status list is all centralized within
thread_info::set_internal_state, when we switch to/from the
resumed-pending-status state. With the assertions in place, it should
be impossible to end up with a THREAD_INT_RUNNING thread with a
pending status.
The thread.c:set_running, thread.c:set_executing, thread.c:set_resumed
global functions are all gone, replaced with new thread.c:set_state
and thread.c:set_internal_state functions.
The gdb.base/watchpoint.exp on Windows with non-stop support added
(later in the series) exposed an issue with the currently_stepping
logic when tested with software watchpoints.
The issue only happens when:
- You have multiple threads. gdb.base/watchpoint.exp exposed it on
Windows because there the OS always spawns a few extra threads.
- Displaced stepping is not available. The Windows non-stop work
does not implement displaced stepping yet. That is left as an
optimization for later.
- The target backend is working in non-stop mode.
I've written a new test that exposes the issue on GNU/Linux as well
(on hardware single-step targets, like x86-64). There, we see:
continue
Continuing.
../../src/gdb/infrun.c:2918: internal-error: resume_1: Assertion `!(thread_has_single_step_breakpoints_set (tp) && step)' failed.
A problem internal to GDB has been detected,
further debugging may prove unreliable.
----- Backtrace -----
FAIL: gdb.threads/sw-watchpoint-step-over-bp-with-threads.exp: target-non-stop=on: displaced-stepping=off: continue until exit (GDB internal error)
Currently, software watchpoints are implemented by forcing
single-stepping. That is done by currently_stepping returning true
when we have a software watchpoint. proceed calls resume, which calls
resume_1, which then ends up always requesting a single-step resume,
even if the higher layers wanted a continue.
Now, if you set a software watchpoint, and then continue the program,
and there's a breakpoint at the current PC, GDB needs to step over
that breakpoint first. If displaced stepping is not available, then
GDB temporarily pauses all threads, removes the breakpoint,
single-steps the thread that needs to move past the breakpoint, and
then finally, reinserts the breakpoint, and restarts all threads
again. That last restarting step happens in the restart_threads
infrun function.
restart_threads iterates over all threads trying to restart them one
by one. There, we have:
if (currently_stepping (tp))
{
infrun_debug_printf ("restart threads: [%s] was stepping",
tp->ptid.to_string ().c_str ());
but, what if TP is actually a new thread that hasn't yet ever set
stepping? currently_stepping still returns true, due to the software
watchpoint, and we end up in keep_going_stepped_thread, here:
if (tp->stop_pc () != tp->prev_pc)
{
ptid_t resume_ptid;
... because prev_pc was stale at that point (we had no reason to
update it earlier). E.g. on Windows we see something like:
[infrun] restart_threads: start: event_thread=1867996.1867996.0, inf=-1
[infrun] restart_threads: restart threads: [1867996.1867996.0] is event thread
[infrun] restart_threads: restart threads: [1867996.1868003.0] was stepping
[infrun] keep_going_stepped_thread: resuming previously stepped thread
[infrun] keep_going_stepped_thread: expected thread advanced also (0 -> 0x7ffff7ce57f8)
[infrun] clear_step_over_info: clearing step over info
[infrun] do_target_resume: resume_ptid=1867996.1868003.0, step=0, sig=GDB_SIGNAL_0
On GNU/Linux, we may see:
[infrun] keep_going_stepped_thread: expected thread advanced also (0x7ffff7d2683d -> 0x7ffff7ce57f8)
there prev_pc might have been updated on an earlier proceed call,
which makes the issue harder to see, but it is stale too here.
That means we insert a single-step breakpoint at the current PC, and
continue the thread, with target_resume directly, asking for a normal
continue.
Eventually, something causes a user-visible stop. For example, the
software watchpoint triggers. That makes GDB stop all threads.
Now, if the user re-resumes the program, say, with "continue", we fail
this assertion in resume_1 coming from proceed:
/* If STEP is set, it's a request to use hardware stepping
facilities. But in that case, we should never
use singlestep breakpoint. */
gdb_assert (!(thread_has_single_step_breakpoints_set (tp) && step));
"step" is true because currently_stepping returns true since we have a
software watchpoint. And the thread has a single-step breakpoint
installed from earlier, because of that code mentioned above, in
keep_going_stepped_thread reached from restart_threads.
This patch fixes the root cause -- the currently_stepping call in
restart_threads returned true for a thread that has never set stepping
in the first place. This is because currently_stepping really serves
two purposes currently:
#1 - for a thread that we are about to resume, should we set it
stepping?
#2 - for a thread that just stopped, was it stepping before?
The fix is thus to decouple those two aspects:
- for #1, we simply rename currently_stepping to should_step.
- for #2, we record whether the thread was stepping before in a new
currently_stepping flag in thread_info.
As mentioned, there's a new testcase included. I tested this on
x86-64 GNU/Linux, native and gdbserver, and on Windows x64 with the
non-stop series. The assertion triggers on all of those with the fix,
and is fixed by this patch on all of those, too.
There's one unnecessary check for currently_stepping in
handle_signal_stop that can be removed. It is unnecessary because
currently_stepping is only ever called if
ecs->event_thread->control.trap_expected is true, and then if it is
true, then currently_stepping always returns true too.
Here, the breakpoint only got one location because both the in-charge
and the not-in-charge dtors are identical and got the same address:
$ nm -A ./testsuite/outputs/gdb.cp/cpexprs/cpexprs| c++filt |grep "~base"
./testsuite/outputs/gdb.cp/cpexprs/cpexprs:0000000000001d84 W base::~base()
./testsuite/outputs/gdb.cp/cpexprs/cpexprs:0000000000001d84 W base::~base()
While on Cygwin, we get two locations for the same breakpoint, which
the testcase isn't expecting:
Thread 1 "cpexprs" hit Breakpoint 117.1, base::~base (this=0x7ffffcaf8, __in_chrg=<optimized out>) at .../src/gdb/testsuite/gdb.cp/cpexprs.cc:135
135 ~base (void) { } // base::~base
(gdb) FAIL: gdb.cp/cpexprs.exp: continue to base::~base
We got two locations because the in-charge and the not-in-charge dtors
have different addresses:
$ nm -A outputs/gdb.cp/cpexprs/cpexprs.exe | c++filt | grep "~base"
outputs/gdb.cp/cpexprs/cpexprs.exe:0000000100402680 T base::~base()
outputs/gdb.cp/cpexprs/cpexprs.exe:0000000100402690 T base::~base()
On Cygwin, we also see the typical failure due to not expecting the
inferior to be multi-threaded:
(gdb) continue
Continuing.
[New Thread 628.0xe08]
Thread 1 "cpexprs" hit Breakpoint 200, test_function (argc=1, argv=0x7ffffcc20) at .../src/gdb/testsuite/gdb.cp/cpexprs.cc:336
336 derived d;
(gdb) FAIL: gdb.cp/cpexprs.exp: continue to test_function for policyd3::~policyd
Both issues are fixed by this patch, and now the testcase passes
cleanly on Cygwin, for me.
Reviewed-By: Keith Seitz <keiths@redhat.com>
Change-Id: If7eb95d595f083f36dfebf9045c0fc40ef5c5df1
Pedro Alves [Fri, 23 Jun 2023 20:01:39 +0000 (21:01 +0100)]
gdb.threads/thread-execl, don't re-exec forever
I noticed on Cygwin, gdb.thread/thread-execl.exp would hang, (not that
surprising since we can't follow-exec on Cygwin). Looking at the
process list running on the machine, we end up with a thread-execl.exe
process constantly respawning another process [1].
We see the same constant-reexec if we launch gdb.thread/thread-execl
manually on the shell:
Pedro Alves [Mon, 26 Jun 2023 12:56:26 +0000 (13:56 +0100)]
Support core dumping testcases with Cygwin's dumper
Cygwin supports dumping ELF cores via a dumper.exe utility, see
https://www.cygwin.com/cygwin-ug-net/dumper.html.
When I run a testcase that has the "kernel" generate a corefile, like
gdb.base/corefile.exp, Cygwin invokes dumper.exe correctly and
generates an ELF core file, however, the testsuite doesn't find the
generated core:
foreach i "${coredir}/core ${coredir}/core.coremaker.c ${binfile}.core" {
Note that that isn't looking for "${binfile}.core" inside
${coredir}... That is fixed in this patch.
However, that still isn't sufficient for Cygwin + dumper, as in that
case the core is going to be called foo.exe.core, not foo.core. Fix
that by looking for foo.exe.core in the core dir as well.
With this, gdb.base/corefile.exp and other tests that use core_find
now run. They don't pass cleanly, but at least now they're exercised.
Approved-By: Tom Tromey <tom@tromey.com>
Change-Id: Ic807dd2d7f22c5df291360a18c1d4fbbbb9b993e
Pedro Alves [Mon, 26 Jun 2023 20:03:32 +0000 (21:03 +0100)]
Adjust gdb.base/sigall.exp for Cygwin
The gdb.base/sigall.exp testcase has many FAILs on Cygwin currently.
From:
Thread 1 "sigall" received signal SIGPWR, Power fail/restart.
0x00007ffeac9ed134 in ntdll!ZwWaitForSingleObject () from /cygdrive/c/Windows/SYSTEM32/ntdll.dll
(gdb) FAIL: gdb.base/sigall.exp: get signal LOST
we see two issues. The test is expecting "Program received ..." which
only appears if the inferior is single-threaded. All Cygwin inferiors
are multi-threaded, because both Windows and the Cygwin runtime spawn
a few helper threads.
And then, SIGLOST is the same as SIGPWR on Cygwin. The testcase
already knows to treat them the same on SPARC64 GNU/Linux. We just
need to extend the relevant code to treat Cygwin the same.
With this, the test passes cleanly on Cygwin.
Approved-By: Tom Tromey <tom@tromey.com>
Change-Id: Ie3553d043f4aeafafc011347b6cb61ed58501667
Pedro Alves [Tue, 5 Sep 2023 12:38:14 +0000 (13:38 +0100)]
Adjust gdb.base/bp-permanent.exp for Cygwin
On Cygwin, all inferiors are multi-threaded, because both Windows and
the Cygwin runtime spawn a few helper threads. Adjust the
gdb.base/bp-permanent.exp testcase to work with either single- or
multi-threaded inferiors.
Approved-by: Kevin Buettner <kevinb@redhat.com>
Change-Id: I28935b34fc9f739c2a5490e83aa4995d29927be2
The difference is the "Thread 1" part in the beginning of the quoted
output. It appears on Cygwin, but not on Linux. That's because on
Cygwin, all inferiors are multi-threaded, because both Windows and the
Cygwin runtime spawn a few helper threads.
Fix this by adjusting the gdb.base/bp-cond-failure.exp testcase to
work with either single- or multi-threaded inferiors.
The testcase passes cleanly for me after this.
Approved-by: Kevin Buettner <kevinb@redhat.com>
Change-Id: I5ff11d06ac1748d044cef025f1e78b8f84ad3349
aarch64: Increase the number of feature words to 3
Now that most of the effort of updating the number of feature words is
handled by macros, add an additional one, taking the number of
supported features to 192.
Richard Earnshaw [Thu, 22 May 2025 15:18:11 +0000 (16:18 +0100)]
aarch64: use macro trickery to automate feature array size replication
There are quite a few macros that need to be changed when we need to
increase the number of words in the features data structure. With
some macro trickery we can automate most of this so that a single
macro needs to be updated.
With C2X we could probably do even better by using recursion, but this
is still a much better situation than we had previously.
A static assertion is used to ensure that there is always enough space
in the flags macro for the number of feature bits we need to support.
Alan Modra [Mon, 9 Jun 2025 03:21:01 +0000 (12:51 +0930)]
dwarf2dbg.c line_entry.next assert
I was puzzling over how it was correct to cast what is clearly a
struct line_entry** pointer to a struct line_entry* pointer for a
few moments, and was going to write a comment but then decided we
really don't require the "next" pointer to be where it is. Replace
the assert with an inline function that does any necessary pointer
adjustments.
* dwarf2dbg.c (line_entry.next): Delete static assertion.
(line_entry_at_tail): New inline function.
(dwarf2_gen_line_info_1, dwarf2_finish): Replace casts in
set_or_check_view arguments with line_entry_at_tail.
Alan Modra [Mon, 9 Jun 2025 03:16:23 +0000 (12:46 +0930)]
str_hash_find casts
Putting an explicit cast on the void* return from str_hash_find isn't
necessary and doesn't add much to code clarity. In other cases, poor
choice of function parameter types, eg. "void *value" in
tc-aarch64.c checked_hash_insert rather than "const void *value" leads
to needing (void *) casts all over the place just to cast away const.
Fix that by correcting the parameter type. (And it really is a const,
the function and str_hash_insert don't modify the strings.)
This patch also removes some unnecessary casts in hash.c
Alan Modra [Mon, 9 Jun 2025 02:36:00 +0000 (12:06 +0930)]
str_hash_find_int
This changes the internal representation of string_tuple.value from
a void* to an intptr_t, removing any concerns that code wanting to
store an integer value will use values that are trap encodings or
suchlike for a pointer. The ISO C standard says any void* can be
converted to intptr_t and back again and will compare equal to the
original pointer. It does *not* say any intptr_t can be converted to
void* and back again to get the original integer..
Two new functions, str_hash_find_int and str_hash_insert_int are
provided for handling integer values. str_hash_find_int returns
(intptr_t) -1 on failing to find the key string.
Most target code need minimal changes to use the new interface, but
some simplification is possible since now a zero can be stored and
differentiated from the NULL "can't find" return. (Yes, that means
(intptr_t) -1 can't be stored.)
I've changed the avr_no_sreg_hash dummy value to zero, and the
loongarch register numbers don't need to be incremented. loongarch
also doesn't need to store an empty key string (if it ever did).
Alan Modra [Sat, 7 Jun 2025 11:58:41 +0000 (21:28 +0930)]
metag build error
gas/config/tc-metag.c: In function ‘parse_dsp_addr’:
gas/config/tc-metag.c:4386:29: error: ‘regs[0]’ may be used uninitialized [-Werror=maybe-uninitialized]
4386 | if (!is_addr_unit (regs[0]->unit) &&
| ~~~~~~~^~~~~~
It looks like regs_read can be zero with "l" non-NULL, so this gcc
complaint is accurate.
Tom de Vries [Sat, 7 Jun 2025 21:28:53 +0000 (23:28 +0200)]
[gdb/build] Fix buildbreaker in hardwire_setbaudrate
When building on x86_64-cygwin, I run into:
...
In file included from gdbsupport/common-defs.h:203,
from gdb/defs.h:26,
from <command-line>:
gdb/ser-unix.c: In function ‘void hardwire_setbaudrate(serial*, int)’:
gdbsupport/gdb_locale.h:28:20: error: expected ‘)’ before ‘gettext’
28 | # define _(String) gettext (String)
| ^~~~~~~
gdbsupport/gdb_assert.h:43:43: note: in expansion of macro ‘_’
43 | internal_error_loc (__FILE__, __LINE__, _("%s: " message), __func__, \
| ^
gdb/ser-unix.c:590:7: note: in expansion of macro ‘gdb_assert_not_reached’
590 | gdb_assert_not_reached (_("Serial baud rate was not found in B_codes"));
| ^~~~~~~~~~~~~~~~~~~~~~
gdb/ser-unix.c:590:31: note: in expansion of macro ‘_’
590 | gdb_assert_not_reached (_("Serial baud rate was not found in B_codes"));
| ^
gdbsupport/gdb_locale.h:28:28: note: to match this ‘(’
28 | # define _(String) gettext (String)
| ^
gdbsupport/gdb_assert.h:43:43: note: in expansion of macro ‘_’
43 | internal_error_loc (__FILE__, __LINE__, _("%s: " message), __func__, \
| ^
gdb/ser-unix.c:590:7: note: in expansion of macro ‘gdb_assert_not_reached’
590 | gdb_assert_not_reached (_("Serial baud rate was not found in B_codes"));
| ^~~~~~~~~~~~~~~~~~~~~~
...
Fix this by dropping the unneeded _() on the gdb_assert_not_reached argument.
Tom de Vries [Sat, 7 Jun 2025 11:59:52 +0000 (13:59 +0200)]
[gdb/testsuite] Fix gdb.ada/dyn-bit-offset.exp on s390x
On s390x-linux, with test-case gdb.ada/dyn-bit-offset.exp and gcc 7.5.0 I get:
...
(gdb) print spr^M
$1 = (discr => 3, array_field => (-5, -6, -7), field => -6, another_field => -6)^M
(gdb) FAIL: $exp: print spr
print spr.field^M
$2 = -6^M
(gdb) FAIL: $exp: print spr.field
...
On x86_64-linux, with the same compiler version I get:
...
(gdb) print spr^M
$1 = (discr => 3, array_field => (-5, -6, -7), field => -4, another_field => -4)^M
(gdb) XFAIL: $exp: print spr
print spr.field^M
$2 = -4^M
(gdb) PASS: $exp: print spr.field
...
In both cases, we're hitting the same compiler problem, but it manifests
differently on little and big endian.
Make sure the values seen for both little and big endian trigger xfails
for both tests.
Printing spr.field gives the expected value -4 for x86_64, but that's an
accident. Change the actual spr.field value to -5, to make sure
that we get the same number of xfails on x86_64 and s390x.
Finally, make the xfails conditional on the compiler version.
Tested using gcc 7.5.0 on both x86_64-linux and s390x-linux.
Approved-By: Andrew Burgess <aburgess@redhat.com>
PR testsuite/33042
https://sourceware.org/bugzilla/show_bug.cgi?id=33042
Georg-Johann Lay [Thu, 15 May 2025 08:29:25 +0000 (10:29 +0200)]
AVR: ld/32968 - Assert that .progmem data resides in the lower 64 KiB.
This patch locates the linker stubs / trampolines *after* all the .progmem
sections. This is the natural placement since progmem data has to reside
in the lower 64 KiB (it is accessed using LPM), whereas the linker stubs
are only required to be located in the lower 128 KiB of program memory.
(They must be in the range of EICALL / EIJMP with EIND = 0.)
The current location of the linker stubs was motivated by an invalid test
case from PR13812 that allocates more than 64 KiB of progmem data.
The patch adds an assertion that makes sure that no progmem data is
allocated past 0xffff.
Data that is accessed using ELPM should be located to .progmemx so that
no .progmem addresses are wasted. .progmemx was introduced in 2017 and
is used by __memx, __flashx and by the current AVR-LibC.
(The compiler uses .jumptables.gcc for its jump dispatch tables since
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=63223 / GCC v4.9.2).
PR ld/32968
ld/
* scripttempl/avr.sc: Move the trampolines section after the
.progmem sections. Assert that .progmem is in the lower 64 KiB.
Andrew Burgess [Thu, 5 Jun 2025 13:50:41 +0000 (14:50 +0100)]
gdb/guile: fix memory leak in gdbscm_parse_command_name
For reference see the previous commit.
Fix a memory leak in gdbscm_parse_command_name when a guile exception
is thrown. To reveal the memory leak I placed the following content
into a file 'leak.scm':
(gdb) source leak.scm
ERROR: In procedure register-command!:
In procedure gdbscm_register_command_x: Out of range: 'break' is not a prefix command in position 1: "break cmd"
Error while executing Scheme code.
Running this under valgrind reveals a memory leak for 'result' and
'prefix_text' from gdbscm_parse_command_name.
Another leak can be revealed with this input script:
This one occurs earlier in gdbscm_parse_command_name, and now only
'result' leaks.
The problem is that, when guile throws an exception then a longjmp is
performed from the function that raise the exception back to the guile
run-time. A consequence of this is that no function local destructors
will be run.
In gdbscm_parse_command_name, this means that the two function locals
`result` and `prefix_text` will not have their destructors run, and
any memory managed by these objects will be leaked.
Fix this by assigning nullptr to these two function locals before
throwing an exception. This will cause the managed memory to be
deallocated.
I could have implemented a fix that made use of Guile's dynwind
mechanism to register a cleanup callback, however, this felt like
overkill. At the point the exception is being thrown we know that we
no longer need the managed memory, so we might as well just free the
memory at that point.
With this fix in place, the two leaks are now fixed in the valgrind
output.
Andrew Burgess [Wed, 4 Jun 2025 18:54:01 +0000 (19:54 +0100)]
gdb/python/guile: remove some explicit calls to xmalloc
In gdbpy_parse_command_name (python/py-cmd.c) there is a call to
xmalloc that can easily be replaced with a call to
make_unique_xstrndup, which makes the code easier to read (I think).
In gdbscm_parse_command_name (guile/scm-cmd.c) the same fix can be
applied to remove an identical xmalloc call. And there is an
additional xmalloc call, which can also be replaced with
make_unique_xstrndup in the same way.
The second xmalloc call in gdbscm_parse_command_name was also present
in gdbpy_parse_command_name at one point, but was replaced with a use
of std::string by this commit:
I haven't changed the gdbscm_parse_command_name to use std::string
though, as that doesn't work well with the guile exception model.
Guile exceptions work by performing a longjmp from the function that
raises the exception, back to the guile run-time. The consequence of
this is that destructors are not run. For example, if
gdbscm_parse_command_name calls gdbscm_out_of_range_error, then any
function local objects in gdbscm_parse_command_name will not have
their destructors called.
What this means is that, for the existing `result` and `prefix_text`
locals, any allocated memory managed by these objects will be leaked
if an exception is called. However, fixing this is pretty easy, one
way is to just assign nullptr to these locals before raising the
exception, this would cause the allocated memory to be released.
But for std::string it is harder to ensure that the managed memory has
actually been released. We can call std::string::clear() and then
maybe std::string::shrink_to_fit(), but this is still not guaranteed
to release any managed memory. In fact, I believe the only way to
ensure all managed memory is released, is to call the std::string
destructor.
And so, for functions that can throw a guile exception, it is easier
to just avoid std::string.
As for the memory leak that I identify above; I'll fix that in a
follow on commit.
gdb/solib: make _linker_namespace use selected frame
When the convenience variable $_linker_namespace was introduced, I meant
for it to print the namespace of the frame that where the user was
stopped. However, due to confusing what "current_frame" and
"selected_frame" meant, it instead printed the namespace of the
lowermost frame.
This commit updates the code to follow my original intent. Since the
variable was never in a GDB release, updating the behavior should not
cause any disruption. It also adds a test to verify the functionality.
Tom Tromey [Tue, 27 May 2025 18:18:30 +0000 (12:18 -0600)]
Fix regression with DW_AT_bit_offset handling
Internal AdaCore testing using -gdwarf-4 found a spot where GCC will
emit a negative DW_AT_bit_offset. However, my recent signed/unsigned
changes assumed that this value had to be positive.
I feel this bug somewhat invalidates my previous thinking about how
DWARF attributes should be handled.
In particular, both GCC and LLVM at understand that a negative bit
offset can be generated -- but for positive offsets they might use a
smaller "data" form, which is expected not to be sign-extended. LLVM
has similar code but GCC does:
What this means is that this attribute is "signed but default
unsigned".
To fix this, I've added a new attribute::confused_constant method.
This should be used when a constant value might be signed, but where
narrow forms (e.g., DW_FORM_data1) should *not* cause sign extension.
I examined the GCC and LLVM DWARF writers to come up with the list of
attributes where this applies, namely DW_AT_bit_offset,
DW_AT_const_value and DW_AT_data_member_location (GCC only, but LLVM
always emits it as unsigned, so we're safe here).
This patch corrects the bug and imports the relevant test case.
Regression tested on x86-64 Fedora 41.
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=32680
Bug: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=118837 Approved-By: Simon Marchi <simon.marchi@efficios.com>
The problem is that catch breakpoints don't set the
bp_location::gdbarch member variable, they a "dummy" location added
with a call to add_dummy_location (breakpoint.c).
The breakpoint_location_address_str function (which is only used for
breakpoint debug output) relies on bp_location::gdbarch being set in
order to call the paddress function.
I considered trying to ensure that the bp_location::gdbarch variable
is always set to sane value. For example, in add_dummy_location I
tried copying the gdbarch from the breakpoint object, and this does
work for the catchpoint case, but for some of the watchpoint cases,
even the breakpoint object has no gdbarch value set.
Now this seemed a little suspect, but, the more I thought about it, I
wondered if "fixing" the gdbarch was allowing me to solve the wrong
problem.
If the gdbarch was set, then this would allow us to print the address
field of the bp_location, which is going to be 0, after all, as this
is a dummy location, which has no address.
But does it really make sense to print the address 0? For some
targets, 0 is a valid address. But that wasn't an address we actually
selected, it's just the default value for dummy locations.
And we already have a helper function bl_address_is_meaningful, which
returns false for dummy locations.
So, I propose that in breakpoint_location_address_str, we use
bl_address_is_meaningful to detect dummy locations, and skip the
address printing code in that case.
For testing, I temporarily changed insert_bp_location so that
breakpoint_location_address_str was always called, even when
breakpoint debugging was off. I then ran the whole testsuite.
Without the fixes included in this commit I saw lots of assertion
failures, but with the fixes from this commit in place, I now see no
assertion failures.
I've added a new test which reveals the original assertion failure.
Approved-By: Simon Marchi <simon.marchi@efficios.com>
commit 4b42385c470c5f72f158f382f4d9c36f927aa84f
Author: Guinevere Larsen <guinevere@redhat.com>
Date: Wed Feb 12 08:25:46 2025 -0300
gdb: Make dwarf support optional at compile time
Introduced a change that made the configure script not POSIX compliant,
by using fallthrough in some case statements. This commit reworks that
part of the change to only use if statements, so that no code is
duplicated but things remain POSIX compliant.
Reviewed-by: Sam James <sam@gentoo.org> Approved-By: Tom Tromey <tom@tromey.com>
Pedro Alves [Mon, 10 Mar 2025 20:08:54 +0000 (20:08 +0000)]
Make default_gdb_exit resilient to failed closes
For some reason, when testing GDB on Cygwin, I get:
child process exited abnormally
while executing
"exec sh -c "exec > /dev/null 2>&1 && (kill -2 -$spid || kill -2 $spid)""
(procedure "close_wait_program" line 20)
invoked from within
"close_wait_program $shell_id $pid"
(procedure "standard_close" line 23)
invoked from within
"standard_close "Windows-ROCm""
("eval" body line 1)
invoked from within
"eval ${try}_${proc} \"$dest\" $args"
(procedure "call_remote" line 42)
invoked from within
"call_remote "" close $host"
(procedure "remote_close" line 3)
invoked from within
"remote_close host"
(procedure "log_and_exit" line 30)
invoked from within
"log_and_exit"
When that happens from within clean_restart, clean_restart doesn't
clear the gdb_spawn_id variable, and then when clean_restart starts up
a new GDB, that sees that gdb_spawn_id is already set, so it doesn't
actually spawn a new GDB, and so clean_restart happens to reuse the
same GDB (!). Many tests happen to actually work OK with this, but
some don't, and the failure modes can be head-scratching.
Of course, the failure to close GDB should be fixed, but when it
happens, I think it's good to not end up with the current weird state.
Connecting the "child process exit abnormally" errors at the end of a
testcase run with weird FAILs in other testcases took me a while (as
in, weeks!), it wasn't obvious to me immediately.
Thus, this patch makes default_gdb_exit more resilient to failed
closes, so that gdb_spawn_id is unset even is closing GDB fails, and
we move on to start a new GDB.
Approved-By: Andrew Burgess <aburgess@redhat.com>
Change-Id: I9ec95aa61872a40095775534743525e0ad2097d2
Pedro Alves [Thu, 5 Jun 2025 17:09:44 +0000 (18:09 +0100)]
gdb_test_multiple: Anchor prompt match if -lbl
The testcase added by this patch has a gdb_test_multiple call that
wants to match different lines of output that all have a common
prefix, and do different actions on each. Instead of a single regular
expression with alternatives, it's clearer code if the different
expressions are handled with different "-re", like so:
gdb_test_multiple "command" "" -lbl {
-re "^command(?=\r\n)" {
exp_continue
}
-re "^\r\nprefix foo(?=\r\n)" {
# Some action for "foo".
exp_continue
}
-re "^\r\nprefix bar(?=\r\n)" {
# Some action for "bar".
exp_continue
}
-re "^\r\nprefix \[^\r\n\]*(?=\r\n)" {
# Some action for all others.
exp_continue
}
-re "^\r\n$::gdb_prompt $" {
gdb_assert {$all_prefixes_were_seen} $gdb_test_name
}
}
Above, the leading anchors in the "^\r\nprefix..." matches are needed
to avoid too-eager matching due to the common prefix. Without the
anchors, if the expect output buffer happens to contain at least:
"\r\nprefix xxx\r\nprefix foo\r\n"
... then the "prefix foo" pattern match inadvertently consumes the
first "prefix xxx" line.
... then the prompt regexp matches this, consuming the "prefix" line
inadvertently, and we get a FAIL. The built-in regexp matcher for
-lbl doesn't get a chance to match the
"\r\nmeant-to-be-matched-by-lbl\r\n" part, because the built-in prompt
match appears first within gdb_test_multiple.
By adding the anchor to the prompt regexp, we avoid that problem.
However, the same expect output buffer contents will still match the
built-in prompt regexp. That is what is fixed by this patch. It
makes it so that if -lbl is specified, the built-in prompt regexp has
a leading anchor.
Original idea for turning this into a gdb.testsuite/ testcase by Tom
de Vries <tdevries@suse.de>.
Approved-By: Tom de Vries <tdevries@suse.de>
Change-Id: Ic2571ec793d856a89ee0d533ec363e2ac6036ea2
Tom de Vries [Fri, 6 Jun 2025 06:25:46 +0000 (08:25 +0200)]
[gdb/tdep] Don't call WaitForSingleObject with INFINITE arg
I decided to try to build and test gdb on Windows.
I found a page on the wiki [1] suggesting three ways of building gdb:
- MinGW,
- MinGW on Cygwin, and
- Cygwin.
I picked Cygwin, because I've used it before (though not recently).
I managed to install Cygwin and sufficient packages to build gdb and start the
testsuite.
However, testsuite progress ground to a halt at gdb.base/branch-to-self.exp.
[ AFAICT, similar problems reported here [2]. ]
I managed to reproduce this hang by running just the test-case.
I attempted to kill the hanging processes by:
- first killing the inferior process, using the cygwin "kill -9" command, and
- then killing the gdb process, likewise.
But the gdb process remained, and I had to point-and-click my way through task
manager to actually kill the gdb process.
I investigated this by attaching to the hanging gdb process. Looking at the
main thread, I saw it was stopped in a call to WaitForSingleObject, with
the dwMilliseconds parameter set to INFINITE.
The backtrace in more detail:
...
(gdb) bt
#0 0x00007fff196fc044 in ntdll!ZwWaitForSingleObject () from
/cygdrive/c/windows/SYSTEM32/ntdll.dll
#1 0x00007fff16bbcdcf in WaitForSingleObjectEx () from
/cygdrive/c/windows/System32/KERNELBASE.dll
#2 0x0000000100998065 in wait_for_single (handle=0x1b8, howlong=4294967295) at
gdb/windows-nat.c:435
#3 0x0000000100999aa7 in
windows_nat_target::do_synchronously(gdb::function_view<bool ()>)
(this=this@entry=0xa001c6fe0, func=...) at gdb/windows-nat.c:487
#4 0x000000010099a7fb in windows_nat_target::wait_for_debug_event_main_thread
(event=<optimized out>, this=0xa001c6fe0)
at gdb/../gdbsupport/function-view.h:296
#5 windows_nat_target::kill (this=0xa001c6fe0) at gdb/windows-nat.c:2917
#6 0x00000001008f2f86 in target_kill () at gdb/target.c:901
#7 0x000000010091fc46 in kill_or_detach (from_tty=0, inf=0xa000577d0)
at gdb/top.c:1658
#8 quit_force (exit_arg=<optimized out>, from_tty=from_tty@entry=0)
at gdb/top.c:1759
#9 0x00000001004f9ea8 in quit_command (args=args@entry=0x0,
from_tty=from_tty@entry=0) at gdb/cli/cli-cmds.c:483
#10 0x000000010091c6d0 in quit_cover () at gdb/top.c:295
#11 0x00000001005e3d8a in async_disconnect (arg=<optimized out>)
at gdb/event-top.c:1496
#12 0x0000000100499c45 in invoke_async_signal_handlers ()
at gdb/async-event.c:233
#13 0x0000000100eb23d6 in gdb_do_one_event (mstimeout=mstimeout@entry=-1)
at gdbsupport/event-loop.cc:198
#14 0x00000001006df94a in interp::do_one_event (mstimeout=-1,
this=<optimized out>) at gdb/interps.h:87
#15 start_event_loop () at gdb/main.c:402
#16 captured_command_loop () at gdb/main.c:466
#17 0x00000001006e2865 in captured_main (data=0x7ffffcba0) at gdb/main.c:1346
#18 gdb_main (args=args@entry=0x7ffffcc10) at gdb/main.c:1365
#19 0x0000000100f98c70 in main (argc=10, argv=0xa000129f0) at gdb/gdb.c:38
...
In the docs [3], I read that using an INFINITE argument to WaitForSingleObject
might cause a system deadlock.
This prompted me to try this simple change in wait_for_single:
...
while (true)
{
- DWORD r = WaitForSingleObject (handle, howlong);
+ DWORD r = WaitForSingleObject (handle,
+ howlong == INFINITE ? 100 : howlong);
+ if (howlong == INFINITE && r == WAIT_TIMEOUT)
+ continue;
...
with the timeout of 0.1 second estimated to be:
- small enough for gdb to feel reactive, and
- big enough not to consume too much cpu cycles with looping.
And indeed, the test-case, while still failing, now finishes in ~50 seconds.
While there may be an underlying bug that triggers this behaviour, the failure
mode is so severe that I consider it a bug in itself.
Fix this by avoiding calling WaitForSingleObject with INFINITE argument.
Tested on x86_64-cygwin, by running the testsuite past the test-case.
Approved-By: Pedro Alves <pedro@palves.net>
PR tdep/32894
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=32894
Guinevere Larsen [Thu, 15 May 2025 14:07:03 +0000 (11:07 -0300)]
gdb/solib: rename convenience variable to _linker_namespace
Based on feedback from IRC and PR solib/32959, this commit renames the
recently introduced convenience variable $_current_linker_namespace to
the shorter name $_linker_namespace. This makes it more in line with
existing convenience variables such as $_thread and $_inferior, and
faster to type.
It should be ok to simply change the name because the variable was never
released to the public, so there should be no existing scripts depending
on the name.
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=32959 Reviewed-By: Eli Zaretskii <eliz@gnu.org> Approved-By: Andrew Burgess <aburgess@redhat.com> Approved-By: Tom Tromey <tom@tromey.com>
gdb: add convenience variables around linker namespace debugging
This commit changes the type of the _current_linker_namespace variable
to be a simple integer. This makes it easier to use for expressions,
like breakpoint conditions or printing from a specific namespace once
that is supported, at the cost of making namespace IDs slightly less
consistent.
This is based on PR solib/32960, where no negative feedback was given
for the suggestion.
The commit also changes the usage of "linkage namespaces" to "linker
namespaces" in the NEWS file, to reduce chance of confusion from an end
user.
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=32960 Reviewed-By: Eli Zaretskii <eliz@gnu.org> Approved-By: Tom Tromey <tom@tromey.com>
Tom de Vries [Thu, 5 Jun 2025 05:37:09 +0000 (07:37 +0200)]
[gdb/testsuite] Fix gdb.base/bp-permanent.exp with gcc 15
With test-case gdb.base/bp-permanent.exp and gcc 15 I run into:
...
gdb compile failed, bp-permanent.c: In function 'test_signal_nested':
bp-permanent.c:118:20: error: passing argument 2 of 'signal' from \
incompatible pointer type [-Wincompatible-pointer-types]
118 | signal (SIGALRM, test_signal_nested_handler);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~
| |
| void (*)(void)
In file included from bp-permanent.c:20:
/usr/include/signal.h:88:57: note: expected '__sighandler_t' \
{aka 'void (*)(int)'} but argument is of type 'void (*)(void)'
...
Fix this by adding an int parameter to test_signal_nested_handler.
Nick Alcock [Tue, 3 Jun 2025 12:39:33 +0000 (13:39 +0100)]
libctf: use __attribute__((__gnu_printf__)) where appropriate
We don't use any GNU-specific printf args, but this prevents warnings about
%z, observed on MinGW even though every libc anyone is likely to use there
supports %z perfectly well, and we're not stopping using it just because
MinGW complains. Doing this means we stand more chance of seeing *actual*
problems on such platforms without them being drowned in noise.
We turn this off on clang, which doesn't support __gnu_printf__.
Suggested by Eli Zaretskii.
libctf/
PR libctf/31863
* ctf-impl.h (_libctf_printflike_): Use __gnu_printf__.
Nick Alcock [Tue, 3 Jun 2025 11:01:45 +0000 (12:01 +0100)]
libctf, dedup: reclaim space wasted by duplicate hidden types
In normal deduplicating links, we insert every type (identified by its
unique hash) precisely once. But conflicting types appear in multiple
dicts, so for those, we loop, inserting them into every target dict
in turn (each corresponding to an input dict that type appears in).
But in cu-mapped links, some of those dicts may have been merged into
one: now that we are hiding duplicate conflicting types more
aggressively in such links, we are getting duplicate identical hidden
types turning up in large numbers.
Fix this by eliminating them in cu-mapping phase 1 (the phase in which this
merging takes place), by checking to see if a type with this hash has
already been inserted in this dict and skipping it if so. This is
redundant and a waste of time in other cu-mapping phases and in normal
links, but in cu-mapped links it saves a few tens to hundreds of kilobytes
in kernel-sized links.
libctf/
PR libctf/33047
* ctf-dedup.c (ctf_dedup_emit_type): Check for already-emitted
types in cu-mapping phase 1.
Nick Alcock [Fri, 30 May 2025 21:12:37 +0000 (22:12 +0100)]
libctf: dedup: preserve non-root flag across normal links
The previous commits dropped preservation of the non-root flag in ctf_link
and arranged to use it somewhat differently to track conflicting types in
cu-mapped CUs when doing cu-mapped links. This was necessary to prevent
entirely spuriously hidden types from appearing on the output of such links.
Bring it (and the test for it) back. The problem with the previous design
was that it implicitly assumed that the non-root flag it saw on the input
was always meant to be preserved (when in the final phase of cu-mapped links
it merely means that conflicting types were found in intermediate links),
and also that it could figure out what the non-root flag on the input was by
sucking in the non-root flag of the input type corresponding to an output in
the output mapping (which maps type hashes to a corresponding type on some
input).
This method of getting properties of the input type *does* work *if* that
property was one of those hashed by the ctf_dedup_hash_type process. In
that case, every type with a given hash will have the same value for all
hashed-in properties, so it doesn't matter which one is consulted (the
output mapping points at an arbitrary one of those input types). But the
non-root flag is explicitly *not* hashed in: as a comment in
ctf_dedup_rhash_type notes, being non-root is not a property of a type, and
two types (one non-root, one not) can perfectly well be the same type even
though one is visible and one isn't. So just copying the non-root flag from
the output mapping's idea of the input type will copy in a value that is not
stabilized by the hash, so is more-or-less random!
So we cannot do that. We have to do something else, which means we have to
decide what to do if two identical types with different nonroot flag values
pop up. The most sensible thing to do is probably to say that if all
instances of a type are non-root-visible, the linked output should also be
non-root-visible: any root-visible types in that set, and the output type is
root-visible again.
We implement this with a new cd_nonroot_consistency dynhash, which maps type
hashes to the value 0 ("all instances root-visible"), 1 ("all instances
non-root-visible") or 2 ("inconsistent"). After hashing is over, we save a
bit of memory by deleting everything from this hashtab that doesn't have a
value of 1 ("non-root-visible"), then use this to decide whether to emit any
given type as non-root-visible or not.
However... that's not quite enough. In cu-mapped links, we want to
disregard this whole thing because we just hide everything -- but in phase
2, when we take the smushed-together CUs resulting from phase 1 and
deduplicate them against each other, we want to do what the previous commits
implemented and ignore the non-root flag entirely, instead falling back to
preventing clashes by hiding anything that would be considered conflicting.
We extend the existing cu_mapped parameter to various bits of ctf_dedup so
that it is now tristate: 0 means a normal link, 1 means the smush-it-
together phase of cu-mapped links, and 2 means the final phase of cu-mapped
links. We do the hide-conflicting stuff only in phase 2, meaning that
normal links by GNU ld can always respect the value of the nonroot flag put
on types in the input.
(One extra thing added as part of this: you can now efficiently delete the
last value returned by ctf_dynhash_next() by calling
ctf_dynhash_next_remove.)
We bring back the ctf-nonroot-linking test with one tweak: linking now works
on mingw as long as you're using the ucrt libc, so re-enable it for better
test coverage on that platform.
libctf/
PR libctf/33047
* ctf-hash.c (ctf_dynhash_next_remove): New.
* ctf-impl.h (struct ctf_dedup) [cd_nonroot_consistency]: New.
* ctf-link.c (ctf_link_deduplicating): Differentiate between
cu-mapped and non-cu-mapped links, even in the final phase.
* ctf-dedup.c (ctf_dedup_hash_type): Callback prototype addition.
Get the non-root flag and pass it down.
(ctf_dedup_rhash_type): Callback prototype addition. Document
restrictions on use of the nonroot flag.
(ctf_dedup_populate_mappings): Populate cd_nonroot_consistency.
(ctf_dedup_hash_type_fini): New function: delete now-unnecessary
values from cd_nonroot_consistency.
(ctf_dedup_init): Initialize it.
(ctf_dedup_fini): Destroy it.
(ctf_dedup): cu_mapping is now cu_mapping_phase. Call
ctf_dedup_hash_type_fini.
(ctf_dedup_emit_type): Use cu_mapping_phase and
cd_nonroot_consistency to propagate the non-root flag into outputs
for normal links, and to do name-based conflict checking only for
phase 2 of cu-mapped links.
(ctf_dedup_emit): cu_mapping is now cu_mapping_phase. Adjust
assertion accordingly.
* testsuite/libctf-writable/ctf-nonroot-linking.c: Bring back.
* testsuite/libctf-writable/ctf-nonroot-linking.lk: Likewise.
Nick Alcock [Fri, 30 May 2025 14:26:26 +0000 (15:26 +0100)]
libctf: dedup: improve hiding of conflicting types in the same dict
If types are conflicting, they are usually moved into separate child dicts
-- but not always. If they are added to the same dict by the cu-mapping
mechanism (as used e.g. for multi-TU kernel modules), we can easily end
up adding multiple conflicting types with the same name to the same dict.
The mechanism used for turning on the non-root-visible flag in order to do
this had a kludge attached which always hid types with the same name,
whether or not they were conflicting. This is unnecessary and can hide
types that should not be hidden, as well as hiding bugs. Remove it, and
replace it with two different approaches:
- for everything but cu-mapped links (the in-memory first phase of a link
with ctf_link_add_cu_mapping in force), check for duplicate names if
types are conflicting, and mark them as hidden if the names are found.
This will never happen in normal links (in an upcoming commit we will
suppress doing even this much in such cases).
- for cu-mapped links, the only case that merges multiple distinct target
dicts into one, we apply a big hammer and simply hide everything! The
non-root flag will be ignored in the next link phase anyway (which dedups
the cu-mapped pieces against each other), and this way we can be sure
that merging multiple types cannot incur name clashes at this stage.
The result seems to work: the only annoyance is that when enums with
conflicting enumerators are found in a single cu-mapped child (so, really
multiple merged children), you may end up with every instance of that enum
being hidden for reasons of conflictingness. I don't see a real way to
avoid that.
libctf/
PR libctf/33047
* ctf-dedup.c (ctf_dedup_emit_type): Only consider non
conflicting types. Improve type hiding in the presence of clashing
enumerators. Hide everything when doing a cu-mapped link: they will
be unhidden by the next link pass if nonconflicting.
It is based on a misconception, that hidden types in the deduplicator
input should always be hidden in the output. For cu-mapped links,
and final links following cu-mapped links, this is not true: we want
to hide inputs if they were conflicting on the output and no more.
We will reintroduce the testcase once a better fix is found.
The mistake in the above patch is in how gdbpy_parse_command_name is
used. The BASE_LIST output argument from this function points to the
list of commands for the prefix, not to the prefix command itself.
So when gdbpy_parse_command_name is called for 'prefix-1 prefix-2',
BASE_LIST points to the list of commands associated with 'prefix-1',
not to the actual 'prefix-1' cmd_list_element.
Back in cmdpy_init, from where gdbpy_parse_command_name was called, I
was walking back from the first entry in BASE_LIST to figure out if
this was a "show" prefix command or not. However, if BASE_LIST is
empty then there is no first item, and this would trigger the
segfault.
The solution it to extend gdbpy_parse_command_name to also return the
prefix cmd_list_element in addition to the existing values. With this
done, and cmdpy_init updated, the segfault is now avoided.
There's a new test that would trigger the crash without the patch.
And, of course, the above commit also broke guile in the exact same
way. And the fix is exactly the same. And there's a guile test too.
NOTE: We should investigate possibly sharing some of this boiler plate
helper code between Python and Guile. But not in this commit.
Since commit d462550c91c ("gdb/testsuite: also compile foll-exec.exp as C++"),
we run into:
...
Running gdb.base/exec-invalid-sysroot.exp ...
gdb compile failed, foll-exec.c: In function 'main':
foll-exec.c:35:52: error: 'EXECD_PROG' undeclared (first use in this function)
printf ("foll-exec is about to execlp(%s)...\n", EXECD_PROG);
^~~~~~~~~~
foll-exec.c:35:52: note: each undeclared identifier is reported only once \
for each function it appears in
...
Fix this by default-defining EXECD_PROG to "execd-prog".
Rui Ueyama [Tue, 3 Jun 2025 02:16:02 +0000 (11:16 +0900)]
Reject compressed sections exceding 4 GiB on LLP64 machines
According to the zlib FAQ (*1), zlib does not support compressed data
larger than 4 GiB when the compiler's long type is 32 bits. Therefore,
we need to report an error if a zlib-compressed debug section exceeds
4 GiB on LLP64 machines.
Indu Bhagat [Wed, 4 Jun 2025 06:10:46 +0000 (23:10 -0700)]
sframe: fix PR libsframe/33051
Fix PR libsframe/Bug 33051 - ASAN: heap-buffer-overflow
../../src/libsframe/sframe.c:1054 in
sframe_get_funcdesc_with_addr_internal
The previous commit 9d2a24349e2 (libsframe: correct binary search for
SFrame FDE) adapted the binary search logic in
sframe_get_funcdesc_with_addr_internal. Adjusting the upper end of the
search index was missed.
The search must only be done for FDEs starting at index 0 and up until
num_fdes - 1. Prior logic of searching (before commit 9d2a24349e2) was
a bit different.
libsframe/
* sframe.c: Use the correct high index.
Andrew Burgess [Mon, 2 Jun 2025 13:58:51 +0000 (14:58 +0100)]
gdb/testsuite: also compile foll-exec.exp as C++
For a long time, Fedora GDB has carried a test that performs some
basic testing that GDB can handle 'catch exec' related commands for a
C++ executable.
The exact motivation for this test has been lost in the mists of time,
but looking at the test script, the concern seems to be that GDB would
have problems inserting C++ related internal breakpoints if a non C++
process is execd from a C++ one.
There's no actual GDB fix associated with the Fedora test. This
usually means that the issue was fixed upstream long ago. This patch
does seem to date from around 2010ish (or maybe earlier).
Having a look through the upstream tests, I cannot see anything that
covers this sort of thing (C++ to C exec calls), and I figure it
cannot hurt to have some additional testing in this area, and so I
wrote this patch.
I've taken the existing foll-exec.exp test, which compiles a C
executable and then execs a different C executable, and split it into
two copies.
We now have foll-exec-c.exp and foll-exec-c++.exp. These tests
compile a C and C++ executable respectively. Then within each of
these scripts both a C and C++ helper application is built, which can
then be execd from the main test executable.
And so, we now cover 4 cases, the initial executable can be C or C++,
and the execd process can be C or C++.
As expected, everything passes. This is just increasing test
coverage.
Approved-By: Simon Marchi <simon.marchi@efficios.com>
Guinevere Larsen [Wed, 12 Feb 2025 11:25:46 +0000 (08:25 -0300)]
gdb: Make dwarf support optional at compile time
This commit allows a user to enable or disable dwarf support at
compilation time. To do that, a new configure option is introduced, in
the form of --enable-gdb-dwarf-support (and the accompanying --disable
version). By default, dwarf support is enabled, so no behavior changes
occur if a user doesn't use the new feature. If support is disabled, no
.c files inside the dwarf2/ subfolder will be compiled into the final
binary.
To achieve this, this commit also introduces the new macro
DWARF_FORMAT_AVAILABLE, which guards the definitions of functions
exported from the dwarf reader. If the macro is not defined, there are a
couple behaviors that exported functions may have:
* no-ops: several functions are used to register things at
initialization time, like unwinders. These are turned into no-ops
because the user hasn't attempted to read DWARF yet, there's no point
in warning that DWARF is unavailable.
* warnings: similar to the previous commit, if dwarf would be read or
used, the funciton will emit the warning "No dwarf support available."
* throw exceptions: If the code that calls a function expects an
exceptin in case of errors, and has a try-catch block, an error with
the previous message is thrown.
I believe that the changed functions should probalby be moved to the
dwarf2/public.h header, but that require a much larger refactor, so it
is left as a future improvement.
Finally, the --enable-gdb-compile configure option has been slightly
changed, since compile requires dwarf support. If compile was requested
and dwarf was disabled, configure will throw an error. If the option was
not used, support will follow what was requested for dwarf (warning the
user of what is decided).
Reviewed-By: Eli Zaretskii <eliz@gnu.org> Approved-By: Tom Tromey <tom@tromey.com> Approved-By: Andrew Burgess <aburgess@redhat.com>
This commit aims to allow a user to enable or disable mdebug support at
compilation time. To do that, a new configure option is added, called
--enable-gdb-mdebug-support (and the accompanying --disable version). By
default, support is enabled, and if a user decides to disable support,
the file mdebugread.c won't be compiled in the final binary, and the
macro MDEBUG_FORMAT_AVAILABLE won't be defined.
That macro is used to control the definitions of mdebug reading, either
the actual definition in mdebugread.c, or a static inline version that
only emits the following warning:
> No mdebug support available.
Ideally, we'd like to guard the entirity of mdebugread in the macro, but
the alpha-mdebug-tdep file uses those directly, and I don't think we
should restrict alpha hosts to requiring that debug format compiled in,
nor do I understand the tdep file enough to be comfortable disentangling
the requirements.
Reviewed-By: Eli Zaretskii <eliz@gnu.org> Approved-By: Andrew Burgess <aburgess@redhat.com>
Guinevere Larsen [Thu, 12 Dec 2024 12:37:36 +0000 (09:37 -0300)]
gdb: Use multiple minimal_symbol_readers in mipscoff_symfile_read
Currently, mipscoff_symfile_read uses a single minimal_symbol_reader to
get all minimal symbols from mdebug-formatted debuginfo, and from
alphacoff. This pattern has been around since minimal_symbol_reader has
been introduced, and from own research, there's no need to use the same
reader. This made it so mipscoff_symfile_read could call
mdebug_build_psymtabs directly, since the latter needs a reference to
the minsym reader object. The issue is that future commits need a
unified entrance point to read debuginfo, and this pattern is very
different to how elf does mdebug reading.
In fact, the elf mdebug reader does some preparatory steps and then
calls mdebug_build_psymtabs, so what the mips version does is just
spread these preparatory steps through the mipscoff function instead.
To make it easier for future commits to query debuginfo support
dynamically (as opposed to assuming things at compile time), this commit
introduces a new mipsmdebug_build_psymtabs function, which does similar
preparatory steps as elfmdebug_build_psymtabs. It is added to
mdebugread.c to help with maintaining a separation between reading an
objfile (in mipsread.c) and its debuginfo (mdebug), so that in the
future we have an easier time selectively disabling debuginfo formats
at compilation time. This should have no visible changes for the end
user.
The new function must receive the pointers to ecoff_debug_swap and
ecoff_debug_info because finding those structures based on the bfd
object necessitates including the headers libcoff.h and libecoff.h,
and those headers can't be included at the same time as libaout.h
- which mdebugread.c already uses.
Andrew Burgess [Sat, 12 Apr 2025 08:15:53 +0000 (09:15 +0100)]
gdb/python/guile: user created prefix commands get help list
Consider GDB's builtin prefix set/show prefix sub-commands, if they
are invoked with no sub-command name then they work like this:
(gdb) show print
print address: Printing of addresses is on.
print array: Pretty formatting of arrays is off.
print array-indexes: Printing of array indexes is off.
print asm-demangle: Demangling of C++/ObjC names in disassembly listings is off.
... cut lots of lines ...
(gdb) set print
List of set print subcommands:
set print address -- Set printing of addresses.
set print array -- Set pretty formatting of arrays.
set print array-indexes -- Set printing of array indexes.
set print asm-demangle -- Set demangling of C++/ObjC names in disassembly listings.
... cut lots of lines ...
Type "help set print" followed by set print subcommand name for full documentation.
Type "apropos word" to search for commands related to "word".
Type "apropos -v word" for full documentation of commands related to "word".
Command name abbreviations are allowed if unambiguous.
(gdb)
That is 'show print' lists the values of all settings under the
'print' prefix, and 'set print' lists the help text for all settings
under the 'set print' prefix.
Now, if we try to create something similar using the Python API:
(gdb) python gdb.ParameterPrefix("my-prefix", gdb.COMMAND_NONE)
(gdb) python gdb.Parameter("my-prefix foo", gdb.COMMAND_OBSCURE, gdb.PARAM_BOOLEAN)
(gdb) show my-prefix
(gdb) set my-prefix
Neither 'show my-prefix' or 'set my-prefix' gives us the same details
relating to the sub-commands that we get with the builtin prefix
commands.
This commit aims to address this.
Currently, in cmdpy_init, when a new command is created, we always set
the commands callback function to cmdpy_function. It is within
cmdpy_function that we spot that the command is a prefix command, and
that there is no gdb.Command.invoke method, and so return early.
This commit changes things so that the rules are now:
1. For NON prefix commands, we continue to use cmdpy_function.
2. For prefix commands that do have a gdb.Command.invoke
method (i.e. can handle unknown sub-commands), continue to use
cmdpy_function.
3. For all other prefix commands, don't use cmdpy_function, instead
use GDB's normal callback function for set/show prefixes.
This requires splitting the current call to add_prefix_cmd into either
a call to add_prefix_cmd, add_show_prefix_cmd, or
add_basic_prefix_cmd, as appropriate.
After these changes, we now see this:
(gdb) python gdb.ParameterPrefix("my-prefix", gdb.COMMAND_NONE) │
(gdb) python gdb.Parameter("my-prefix foo", gdb.COMMAND_OBSCURE, gdb.PARAM_BOOLEAN)
(gdb) show my-prefix │
my-prefix foo: The current value of 'my-prefix foo' is "off".
(gdb) set my-prefix
List of "set my-prefix" subcommands:
set my-prefix foo -- Set the current value of 'my-prefix foo'.
Type "help set my-prefix" followed by subcommand name for full documentation.
Type "apropos word" to search for commands related to "word".
Type "apropos -v word" for full documentation of commands related to "word".
Command name abbreviations are allowed if unambiguous.
(gdb)
Which matches how a prefix defined within GDB would act.
Tom Tromey [Tue, 3 Jun 2025 14:05:41 +0000 (08:05 -0600)]
Clean up comment in dw2-ranges-psym-warning.exp
This removes a trailing backslash from a comment in
dw2-ranges-psym-warning.exp. This backslash causes Emacs to try to
reindent the next line. This happens because comments are weird in
Tcl -- they are not exactly syntactic and the backslash still acts as
a line-continuation marker here.
Tom Tromey [Tue, 20 May 2025 16:45:07 +0000 (10:45 -0600)]
Handle dynamic DW_AT_data_bit_offset
In Ada, a field can have a dynamic bit offset in its enclosing record.
In DWARF 3, this was handled using a dynamic
DW_AT_data_member_location, combined with a DW_AT_bit_offset -- this
combination worked out ok because in practice GNAT only needs a
dynamic byte offset with a fixed offset within the byte.
However, this approach was deprecated in DWARF 4 and then removed in
DWARF 5. No replacement approach was given, meaning that in strict
mode there is no way to express this.
This is a DWARF bug, see
https://dwarfstd.org/issues/250501.1.html
In a discussion on the DWARF mailing list, a couple people mentioned
that compilers could use the obvious extension of a dynamic
DW_AT_data_bit_offset. I've implemented this for LLVM:
https://github.com/llvm/llvm-project/pull/141106
In preparation for that landing, this patch implements support for
this construct in gdb.
New in v2: renamed some constants and added a helper method, per
Simon's review.
New in v3: more renamings.
Approved-By: Simon Marchi <simon.marchi@efficios.com>
Andrew Burgess [Wed, 7 May 2025 11:58:41 +0000 (12:58 +0100)]
gdb: support zero inode in generate-core-file command
It is possible, when creating a shared memory segment (i.e. with
shmget), that the id of the segment will be zero.
When looking at the segment in /proc/PID/smaps, the inode field of the
entry holds the shared memory segment id.
And so, it can be the case that an entry (in the smaps file) will have
an inode of zero.
When GDB generates a core file, with the generate-core-file (or its
gcore alias) command, the shared memory segment should be written into
the core file.
Fedora GDB has, since 2008, carried a patch that tests this case.
There is no fix for GDB associated with the test, and unfortunately,
the motivation for the test has been lost to the mists of time. This
likely means that a fix was merged upstream without a suitable test,
but I've not been able to find and relevant commit. The test seems to
be checking that the shared memory segment with id zero, is being
written to the core file.
While looking at this test and trying to work out if it should be
posted upstream, I saw that GDB does appear to write the shared memory
segment into the core file (as expected), which is good. However, GDB
still isn't getting this case exactly right, there appears to be no
NT_FILE entry for the shared memory mapping if the mapping had an id
of zero.
In gcore_memory_sections (gcore.c) we call back into linux-tdep.c (via
the gdbarch_find_memory_regions call) to correctly write the shared
memory segment into the core file, however, in
linux_make_mappings_corefile_notes, when we use
linux_find_memory_regions_full to create the NT_FILE note, we call
back in to dump_note_entry_p for each mapping, and in here we reject
any mapping with a zero inode.
The result of this, is that, for a shared memory segment with a
non-zero id, after loading the core file, the shared memory segment
will appear in the 'proc info mappings' output. But, for a shared
memory segment with a zero id, the segment will not appear in the
'proc info mappings' output.
I initially tried just dropping the inode check in this function (see
previous commit 1e21c846c27, which I then reverted in commit 998165ba99a.
The problem with dropping the inode check is that the special kernel
mappings, e.g. '[vvar]' would now get a NT_FILE entry. In fact, any
special entry except '[vdso]' and '[vsyscall]' which are specifically
checked for in dump_note_entry_p would get a NT_FILE entry, which is
not correct.
So, instead, I propose that if the inode is zero, and the filename
starts with '[' and finished with ']' then we should not create a
NT_FILE entry. But otherwise a zero inode should not prevent a
NT_FILE entry being created.
The test for this change is a bit tricky. The original Fedora
test (mentioned above) has a loop that tries to grab the shared memory
mapping with id zero. This was, unfortunately, not very reliable.
I tried to make this more reliable by going multi-threaded, and
waiting for longer, see my proposal here:
But this was still not great. On further testing this was only
passing (i.e. managing to find the shared memory mapping with id zero)
about 60% of the time.
However, I realised that GDB finds the shared memory id by reading the
/proc/PID/smaps file. But we don't really _need_ the shared memory id
for anything, we just use the value (as an inode) to decide if the
segment should be included in the core file or not. The id isn't even
written to the core file. So, if we could intercept the read of the
smaps file, then maybe, we could lie to GDB, and tell it that the id
was zero, and then see how GDB handles this.
And luckily, we can do that using a preload library!
We already have a test that uses a preload library to modify GDB, see
gdb.threads/attach-slow-waitpid.exp.
So, I have created a new preload library. This one intercepts open,
open64, close, read, and pread. When GDB attempts to open
/proc/PID/smaps, the library spots this and loads the file contents
into a memory buffer. The buffer is then modified to change the id of
any shared memory mapping to zero. Any reads from this file are
served from the modified memory buffer.
I tested on x86-64, AArch64, PPC, s390, and ARM, all running various
versions of GNU/Linux. The requirement for open64() came from my ARM
testing. The other targets used plain open().
And so, the test is now simple. Start GDB with the preload library in
place, start the inferior and generate a core file. Then restart GDB,
load the core file, and check the shared memory mapping was included.
This test will fail with an unpatched GDB, and succeed with the patch
applied.
Piotr Rudnicki [Fri, 30 May 2025 13:21:49 +0000 (15:21 +0200)]
gdb: handle struct and union types in evaluate_subexp_for_address_base
Suppose a function returns a struct and a method of that struct is
called. E.g.:
struct S
{
int a;
int get () { return a; }
};
S f ()
{
S s;
s.a = 42;
return s;
}
...
int z = f().get();
...
GDB is able to evaluate the expression:
(gdb) print f().get()
$1 = 42
However, type-checking the expression fails:
(gdb) ptype f().get()
Attempt to take address of value not located in memory.
This happens because the `get` function takes an implicit `this`
pointer, which in this case is the value returned by `f()`, and GDB
wants to get an address for that value, as if passing the implicit
this pointer. However, during type-checking, the struct value
returned by `f()` is a `not_lval`.
A similar issue exists for union types, where methods called on
temporary union objects would fail type-checking in the same way.
Address the problems by handling `TYPE_CODE_STRUCT` and
`TYPE_CODE_UNION` in `evaluate_subexp_for_address_base`.
With this change, for struct's method call, we get
(gdb) ptype f().get()
type = int
Add new test cases to file gdb.cp/chained-calls.exp to test this change.
Tom de Vries [Tue, 3 Jun 2025 06:59:58 +0000 (08:59 +0200)]
[gdb/cli] Use captured per_command_time in worker threads
With test-case gdb.base/maint.exp, I ran into:
...
(gdb) file maint^M
Reading symbols from maint...^M
(gdb) mt set per-command on^M
(gdb) Time for "DWARF indexing worker": ...^M
Time for "DWARF indexing worker": ...^M
Time for "DWARF indexing worker": ...^M
Time for "DWARF indexing worker": ...^M
Time for "DWARF skeletonless type units": ...^M
Time for "DWARF add parent map": ...^M
Time for "DWARF finalize worker": ...^M
Time for "DWARF finalize worker": ...^M
Time for "DWARF finalize worker": ...^M
Time for "DWARF finalize worker": ...^M
Time for "DWARF finalize worker": ...^M
FAIL: $exp: warnings: per-command: mt set per-command on (timeout)
mt set per-command off^M
2025-05-31 09:33:44.711 - command started^M
(gdb) PASS: $exp: warnings: per-command: mt set per-command off
...
I didn't manage to reproduce this by rerunning the test-case, but it's fairly
easy to reproduce using a file with more debug info, for instance gdb:
...
$ gdb -q -batch -ex "file build/gdb/gdb" -ex "mt set per-command on"
...
Due to the default "mt dwarf synchronous" == off, the file command starts
building the cooked index in the background, and returns immediately without
waiting for the result.
The subsequent "mt set per-command on" implies "mt set per-command time on",
which switches on displaying of per-command execution time.
The "Time for" lines are the result of those two commands, but these lines
shouldn't be there because "mt per-command time" == off at the point of
issuing the file command.
Fix this by capturing the per_command_time variable, and using the captured
value instead.
Tested on x86_64-linux.
Approved-By: Simon Marchi <simon.marchi@efficios.com>
PR cli/33039
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=33039
Simon Marchi [Mon, 2 Jun 2025 20:31:12 +0000 (20:31 +0000)]
gdb/dwarf2: update call_site::target comment
This comment refers to the field location kind enum, even though call
sites were moved to their own enum in 7eb21cc70224 ("Change
call_site_target to use custom type and enum"). Update it.
projects / thirdparty / binutils-gdb.git / log
