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.
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.
Pedro Alves [Sat, 17 May 2025 21:24:45 +0000 (22:24 +0100)]
Fix build when RUSAGE_THREAD is not available & add warning
Building current GDB on Cygwin, fails like so:
/home/pedro/gdb/src/gdbsupport/run-time-clock.cc: In function ‘void get_run_time(user_cpu_time_clock::time_point&, system_cpu_time_clock::time_point&, run_time_scope ’:
/home/pedro/gdb/src/gdbsupport/run-time-clock.cc:52:13: error: ‘RUSAGE_THREAD’ was not declared in this scope; did you mean ‘SIGEV_THREAD’?
52 | who = RUSAGE_THREAD;
| ^~~~~~~~~~~~~
| SIGEV_THREAD
Cygwin does not implement RUSAGE_THREAD. Googling around, I see
Cygwin is not alone, other platforms don't support it either. For
example, here is someone suggesting an alternative for darwin/macos:
https://stackoverflow.com/questions/5652463/equivalent-to-rusage-thread-darwin
Fix this by falling back to process scope if thread scope can't be
supported. I chose this instead of returning zero usage or some other
constant, because if gdb is built without threading support, then
process-scope run time usage is the right info to return.
But instead of falling back silently, print a warning (just once),
like so:
(gdb) maint set per-command time on
⚠️ warning: per-thread run time information not available on this platform
... so that developers on other platforms at least have a hint
upfront.
This new warning also shows on platforms that don't have getrusage in
the first place, but does not show if the build doesn't support
threading at all.
New tests are added to gdb.base/maint.exp, to expect the warning, and
also to ensure other "mt per-command" sub commands don't trigger the
new warning.
Alan Modra [Fri, 16 May 2025 03:01:39 +0000 (12:31 +0930)]
gas .align limit
At the moment we allow alignment of up to half the address space,
which is stupidly large and results in OOM on x86_64. Change that to
1G alignment in text sections. Also fix the warning message on
exceeding max allowed alignment.
* read.c (TC_ALIGN_LIMIT): Limit to 30 in text sections.
(s_align): Correct "alignment too large" value.
Jan Beulich [Fri, 16 May 2025 10:32:39 +0000 (12:32 +0200)]
gas: adjust a comparison in s_align()
In 344b1e0f5f79 ("gas: range-check 3rd argument of .align et al") I
neglected to consider compilers which warn about signed/unsigned
mismatches in comparisons (which is somewhat odd when the signed value is
already known to be non-negative).
Jan Beulich [Fri, 16 May 2025 08:37:46 +0000 (10:37 +0200)]
gas: range-check 3rd argument of .align et al
Negative values would have been silently converted to large positive
ones, which may not be the user's intention. Similarly overly large
values would have been silently truncated. Warn them instead, and zap
such values.
Collin Funk [Fri, 16 May 2025 08:37:16 +0000 (10:37 +0200)]
ld/doc: Remove '.info' suffix in @ref, etc
Texinfo 7.2 began showing warnings like:
ld.texi:1026: warning: do not set .info suffix in reference for manual `gcc.info'
ld.texi:9689: warning: do not set .info suffix in reference for manual `binutils.info'
The Texinfo developers plan to stop removing the '.info' suffix
internally in a future release so without this patch the references will
break in the future.
Collin Funk [Fri, 16 May 2025 08:37:04 +0000 (10:37 +0200)]
binutils/doc: Remove '.info' suffix in @ref, etc
Texinfo 7.2 began showing warnings like:
binutils.texi:882: warning: do not set .info suffix in reference for manual `ld.info'
binutils.texi:1365: warning: do not set .info suffix in reference for manual `ld.info'
The Texinfo developers plan to stop removing the '.info' suffix
internally in a future release so without this patch the references will
break in the future.
Jan Beulich [Fri, 16 May 2025 08:32:19 +0000 (10:32 +0200)]
x86: improve matching diagnostics when %st is involved
Diagnosing operand size vs operand type mismatches doesn't work very
well when GPRs and FPRs are in the same register class, distinguished
just by size. Introduce a separate RegFP class.
Jan Beulich [Fri, 16 May 2025 08:31:35 +0000 (10:31 +0200)]
x86: move Anysize check in operand_size_match()
Anysize is applicable to memory operands only. Move the check to where
memory operands are handled. (The RegSIMD part there was questionable
altogether.)
Jan Beulich [Fri, 16 May 2025 08:27:55 +0000 (10:27 +0200)]
x86: improve matching diagnostics when "accumulator" registers are involved
In templates, the expectation of an "accumulator" register to be used is
expressed solely by operand size; there's no "class" specifier there.
Hence operand_size_match() is too eager in invoking
match_{operand,simd}_size(), resulting in "operand size mismatch" errors
when it's the type (of register), not the size that's wrong.
Interestingly adjustments there alone lead to no error at all then: To
"compensate", operand_type_match() needs to disambiguate register types
when register instances are specified in the template (matching the
actual operand), by checking a match (overlap) in operand sizes.
Jan Beulich [Fri, 16 May 2025 08:27:20 +0000 (10:27 +0200)]
x86: fold Accum checking in operand_size_match()
There's little point invoking match_{operand,simd}_size() twice per
loop; in fact the SIMD case with D set simply doesn't exist. Amend the
checks by one looking at the given operand, just like we already have
been doing for memory ones.
Jan Beulich [Fri, 16 May 2025 08:26:45 +0000 (10:26 +0200)]
x86: improve matching diagnostics
Many times in the past I was puzzled by seeing "operand size mismatch"
when really "operand type mismatch" would be far more appropriate. As it
turns out, there were at least two flaws: In the single operand case we
didn't propagate i.error to match_template()'s local specific_error when
noticing a type mismatch. And then operand_size_match() was too eager in
invoking match_mem_size(): Especially the Unspecified attribute can get
in the way there when the expected operand isn't a memory one (and hence
Unspecified would not be set in the operand template, whereas it's
uniformly set for memory operands in AT&T syntax).
(In the x86-64-lkgs-inval testcase the particular error for the two
bogus Intel syntax forms doesn't really matter; all we ought to care
about there isthat there is _some_ error.)
Jan Beulich [Fri, 16 May 2025 08:25:38 +0000 (10:25 +0200)]
x86: drop bogus accumulator check
Accum is an "instance", not a "class". With present enumerator values of
Reg and Accum, the 2nd check simply did the same as the first. In fact
checking for the accumulator (%rax) isn't necessary here at all, because
there's no case where an individual template would permit alternatively
a memory operand or the (qword) accumulator; only "any GPR" is ever
being paired with "memory".
Tsukasa OI [Tue, 13 May 2025 08:14:01 +0000 (08:14 +0000)]
RISC-V: check offsets when linker relaxation is disabled
The assembler partially relied on the linker to check whether the
offset is valid. However, some optimization logic (added later)
removes relocations relative to local symbols without checking offsets.
For instance, it caused following code to silently emit wrong jumps
(to the jump instruction "." itself) without relocations:
> .option norelax
> j .+0x200000 # J (or JAL) instruction cannot encode this offset.
> j .+1 # Jump to odd address is not valid.
This commit adds offset checks where necessary.
gas/ChangeLog:
* config/tc-riscv.c (md_apply_fix): Check offsets when the
relocation relative to a local symbol is being optimized out.
* testsuite/gas/riscv/no-relax-branch-offset-fail.s: Failure
case where the branch offset is invalid.
* testsuite/gas/riscv/no-relax-branch-offset-fail.d: Ditto.
* testsuite/gas/riscv/no-relax-branch-offset-fail.l: Ditto.
* testsuite/gas/riscv/no-relax-branch-offset-ok.s: Border case.
* testsuite/gas/riscv/no-relax-branch-offset-ok.d: Ditto.
* testsuite/gas/riscv/no-relax-pcrel-offset-fail-64.s: Failure
case only on RV64 where the PC-relative offset exceed limits.
* testsuite/gas/riscv/no-relax-pcrel-offset-fail-64.d: Ditto.
* testsuite/gas/riscv/no-relax-pcrel-offset-fail-64.l: Ditto.
* testsuite/gas/riscv/no-relax-pcrel-offset-fail-not-32.d: Test
case for RV32 so that no errors occur.
* testsuite/gas/riscv/no-relax-pcrel-offset-ok.s: Border case.
* testsuite/gas/riscv/no-relax-pcrel-offset-ok.d: Ditto.
Indu Bhagat [Thu, 15 May 2025 19:21:05 +0000 (12:21 -0700)]
gas: sframe: avoid creating more symbols than necessary for FRE offset
Each SFrame FDE contains an offset to the start of its respective SFrame
FREs in the sfde_func_start_fre_off field. To generate this offset,
fre_symbols[] array is being used. The number of elements of this array
is currently set to the total number of SFrame FREs in the entire SFrame
section. This is more than unnecessary. We only need to track as many
points as the number of SFrame FDEs.
gas/
* gen-sframe.c (output_sframe_internal): Size fde_fre_symbols
with the number of SFrame FDEs.
Tom Tromey [Tue, 13 May 2025 19:41:26 +0000 (13:41 -0600)]
Fix regression with dynamic array bounds
Kévin discovered that commit ba005d32b0f ("Handle dynamic field
properties") regressed a test in the internal AdaCore test suite.
The problem here is that, when writing that patch, I did not consider
the case where an array type's bounds might come from a member of a
structure -- but where the array is not defined in the structure's
scope.
In this scenario the field-resolution logic would trip this condition:
/* Defensive programming in case we see unusual DWARF. */
if (fi == nullptr)
return nullptr;
This patch reworks this area, partly backing out that commit, and
fixes the problem.
In the new code, I chose to simply duplicate the field's location
information. This isn't totally ideal, in that it might result in
multiple copies of a baton. However, this seemed nicer than tracking
the DIE/field correspondence for every field in every CU -- my
thinking here is that this particular dynamic scenario is relatively
rare overall. Also, if the baton cost does prove onerous, we could
intern the batons somewhere.
Regression tested on x86-64 Fedora 41. I also tested this using the
AdaCore internal test suite.
Tested-By: Simon Marchi <simon.marchi@efficios.com>
H.J. Lu [Wed, 14 May 2025 23:30:06 +0000 (07:30 +0800)]
binutils: Don't complain plugin with all LTO sections removed
When all LTO sections have been removed, the BFD lto_type is set to
lto_non_ir_object by bfd_set_lto_type. In this case, don't complain
needing a plugin when seeing a LTO slim symbol.
Alan Modra [Wed, 14 May 2025 22:59:37 +0000 (08:29 +0930)]
gas .file 0 vs. dwarf5
Support added in commit 3417bfca676f for dwarf5 directory table 0
assumed that .file 0 was always the first debug .file directive.
That's not necessarily true.
* dwarf2dbg.c (get_directory_table_entry): Don't assume entry
1 is available after putting DW_AT_comp_dir in entry 0. Pass
pwd as file0_dirname to recursive call to avoid another
getpwd in the case file0_dirname is NULL.
Rohr, Stephan [Mon, 24 Mar 2025 13:05:33 +0000 (13:05 +0000)]
testsuite: fix gdb_exit for MinGW target
GDB is not properly exited via 'remote_close host' when running the
testsuite in a MinGW environment. Use the 'quit' command to properly
exit the GDB debugging session.
Tom Tromey [Mon, 12 May 2025 17:30:33 +0000 (11:30 -0600)]
Fix create_breakpoint_parse_arg_string self-test
The emoji patch broke the create_breakpoint_parse_arg_string self-test
when gdb is running on a suitable terminal. The problem is that the
test case doesn't take the error prefix string into account.
This patch fixes the test by having it compare the exception message
directly, rather than relying on the result of exception_print. I did
try a different approach, of having the test mimic exception_print,
but this one seemed cleaner to me.
Approved-By: Simon Marchi <simon.marchi@efficios.com>
Alan Modra [Wed, 14 May 2025 05:18:33 +0000 (14:48 +0930)]
gas .file sanity check
Currently we allow insane file numbers that cause gas to allocate up
to 4G of memory for a file array. Trim that a little to 1G (which
still allows insane file numbers up to 33554431), and tidy function
parameter types so that we only need one file number sanity check.
* dwarf2dbg.c (assign_file_to_slot): Take a valueT file number.
Reduce max files array size.
(allocate_filename_to_slot): Take a valueT file number.
(dwarf2_directive_filename): Don't duplicate file number
sanity check here.
H.J. Lu [Sat, 3 May 2025 21:12:46 +0000 (05:12 +0800)]
strip: Add GCC LTO IR support
Add GCC LTO IR support to strip by copying GCC LTO IR input as unknown
object file. Don't enable LTO plugin in strip unless all LTO sections
should be removed, assuming all LTO sections will be removed with
-R .gnu.lto_.*. Add linker LTO tests for strip with --strip-unneeded
and GCC LTO IR inputs.
binutils/
PR binutils/21479
* objcopy.c: Include "plugin-api.h" and "plugin.h".
(lto_sections_removed): New.
(command_line_switch): Add OPTION_PLUGIN.
(strip_options): Likewise.
(strip_usage): Display "--plugin NAME".
(copy_unknown_file): New function.
(copy_unknown_object): Call copy_unknown_file.
(copy_archive): Copy input LTO IR member as unknown object.
(copy_file): Set input target to "plugin" for strip if it is
unset unless all LTO sections should be removed. Copy input
LTO IR file as unknown file.
(strip_main): Call bfd_plugin_set_program_name. Handle
OPTION_PLUGIN. Set lto_sections_removed to true if all GCC
LTO sections should be removed.
* doc/binutils.texi: Document --plugin for strip.
Alice Carlotti [Fri, 4 Apr 2025 18:23:11 +0000 (19:23 +0100)]
aarch64: Replace incorrect comment
The comment explaining the placement of the cfinv entry before the
generic msr entry in the opcode table was incorrect. The issue is
unrelated to the all ones bitmask for cfinv, and is actually due the
large number of architectural aliases of the msr instruction.
Andrew Burgess [Sun, 13 Apr 2025 10:26:41 +0000 (11:26 +0100)]
gdb/python: new gdb.ParameterPrefix class
This commit adds a new gdb.ParameterPrefix class to GDB's Python API.
When creating multiple gdb.Parameters, it is often desirable to group
these together under a sub-command, for example, 'set print' has lots
of parameters nested under it, like 'set print address', and 'set
print symbol'. In the Python API the 'print' part of these commands
are called prefix commands, and are created using gdb.Command objects.
However, as parameters are set via the 'set ....' command list, and
shown through the 'show ....' command list, creating a prefix for a
parameter usually requires two prefix commands to be created, one for
the 'set' command, and one for the 'show' command.
This often leads to some duplication, or at the very least, each user
will end up creating their own helper class to simplify creation of
the two prefix commands.
This commit adds a new gdb.ParameterPrefix class. Creating a single
instance of this class will create both the 'set' and 'show' prefix
commands, which can then be used while creating the gdb.Parameter.
This adds 'set my-prefix' and 'show my-prefix', both of which are
prefix commands. The user can then add gdb.Parameter objects under
these prefixes.
The gdb.ParameterPrefix initialise method also supports documentation
strings, so we can write:
gdb.ParameterPrefix('my-prefix', gdb.COMMAND_NONE,
"Configuration setting relating to my special extension.")
which will set the documentation string for the prefix command.
Also, it is possible to support prefix commands that use the `invoke`
functionality to handle unknown sub-commands. This is done by
sub-classing gdb.ParameterPrefix and overriding either 'invoke_set' or
'invoke_show' to handle the 'set' or 'show' prefix command
respectively.
Andrew Burgess [Sat, 12 Apr 2025 14:42:25 +0000 (15:42 +0100)]
gdb/guile: generate general description string for parameters
This commit builds on the previous one, and auto-generates a general
description string for parameters defined via the Guile API. This
brings the Guile API closer inline with the Python API. It is worth
reading the previous commit to see some motivating examples.
This commit updates get_doc_string in guile/scm-param.c to allow for
the generation of a general description string. Then in
gdbscm_make_parameter, if '#:doc' was not given, get_doc_string is
used to generate a suitable default.
This does invalidate (and so the commit removes) this comment that was
in gdbscm_make_parameter:
/* If doc is NULL, leave it NULL. See add_setshow_cmd_full. */
First, Python already does exactly what I'm proposing here, and has
done for a while, with no issues reported. And second, I've gone and
read add_setshow_cmd_full, and some of the functions it calls, and can
see no reasoning behind this comment...
... well, there is one reason that I can think of, but I'll discuss
that more below.
With this commit, if I define a parameter like this:
(gdb) help show print test
Show the current value of 'print test'.
This command is not documented.
(gdb) help set print test
Set the current value of 'print test'.
This command is not documented.
(gdb)
The two 'This command is not documented.' lines are new. This output
is what we get from a similarly defined parameter using the Python
API (see the previous commit for an example).
I mentioned above that I can think of one reason for the (now deleted)
comment in gdbscm_make_parameter about leaving the doc field as NULL,
and that is this: consider the following GDB behaviour:
(gdb) help show style filename foreground
Show the foreground color for this property.
(gdb)
Notice there is only a single line of output. If I want to get the
same behaviour from a parameter defined in Guile, I might try skipping
the #:doc argument, but (after this commit), if I do that, GDB will
auto-generate some text for me, giving two lines of output (see
above).
So, next, maybe I try setting #:doc to the empty string, but if I do
that, then I get this:
(gdb) help show print test
Show the current value of 'print test'.
(gdb)
Notice the blank line, that's not what I wanted. In fact, the only
way to get rid of the second line is to leave the 'doc' variable as
NULL in gdbscm_make_parameter, which, due to the new auto-generation,
is no longer possible.
This issue also existed in the Python API, and was addressed in
commit:
After this commit, an empty __doc__ string for a gdb.Parameter is
translated into a NULL pointer passed to the add_setshow_* command,
which means the second line of output is completely skipped.
And this commit includes the same solution for the Guile API. Now,
with this commit, and the Guile parameter using an empty '#:doc'
string, GDB has the following behaviour:
(gdb) help show print test
Show the current value of 'print test'.
(gdb)
This matches the output for a similarly defined parameter in Python.
Andrew Burgess [Sat, 12 Apr 2025 13:19:20 +0000 (14:19 +0100)]
gdb/guile: improve auto-generated strings for parameters
Consider this user defined parameter created in Python:
class test_param(gdb.Parameter):
def __init__(self, name):
super ().__init__(name, gdb.COMMAND_NONE, gdb.PARAM_BOOLEAN)
self.value = True
test_param('print test')
If this is loaded into GDB, then we observe the following behaviour:
(gdb) show print test
The current value of 'print test' is "on".
(gdb) help show print test
Show the current value of 'print test'.
This command is not documented.
(gdb) help set print test
Set the current value of 'print test'.
This command is not documented.
(gdb)
And load this into a fresh GDB session, we see the following:
(gdb) show print test
Command is not documented is off.
(gdb) help show print test
This command is not documented.
(gdb) help set print test
This command is not documented.
(gdb)
The output of 'show print test' doesn't make much sense, and is
certainly worse than the Python equivalent. For both the 'help'
commands it appears as if the first line is missing, but what is
actually happening is that the first line has become 'This command is
not documented.', and the second line is then missing.
The problems can all be traced back to 'get_doc_string' in
guile/scm-param.c. This is the guile version of this function. There
is a similar function in python/py-param.c, however, the Python
version returns one of three different strings depending on the use
case. In contrast, the Guile version just returns 'This command is
not documented.' in all cases.
The three cases that the Python code handles are, the 'set' string,
the 'show' string, and the general 'description' string.
Right now the Guile get_doc_string only returns the general
'description' string, which is funny, because, in
gdbscm_make_parameter, where get_doc_string is used, the one case that
we currently don't need is the general 'description' string. Instead,
right now, the general 'description' string is used for both the 'set'
and 'show' cases.
In this commit I plan to bring the Guile API a little more inline with
the Python API. I will update get_doc_string (in scm-param.c) to
return either a 'set' or 'show' string, and gdbscm_make_parameter will
make use of these strings.
The changes to the Guile get_doc_string are modelled on the Python
version of this function. It is also worth checking out the next
commit, which is related, and helps motivate how the changes have been
implemented in this commit.
After this commit, the same Guile parameter description shown above,
now gives this behaviour:
(gdb) show print test
The current value of 'print test' is off.
(gdb) help show print test
Show the current value of 'print test'.
(gdb) help set print test
Set the current value of 'print test'.
(gdb)
The 'show print test' output now matches the Python behaviour, and is
much more descriptive. The set and show 'help' output are now missing
the second line when compared to the Python output, but the first line
is now correct, and I think this is better than the previous Guile
output.
In the next commit I'll address the problem of the missing second
line.
Existing tests have been updated to expect the new output.
I was recently attempting to create some parameters via the Python
API. I wanted these parameters to appear similar to how GDB handles
the existing 'style' parameters.
Specifically, I was interested in this behaviour:
(gdb) help show style filename foreground
Show the foreground color for this property.
(gdb) help set style filename foreground
Set the foreground color for this property.
(gdb)
Notice how each 'help' command only gets a single line of output.
I tried to reproduce this behaviour via the Python API and was unable.
The problem is that, in order to get just a single line of output like
this, the style parameters are registered with a call to
add_setshow_color_cmd with the 'help_doc' being passed as nullptr.
On the Python side, when parameters are created, the 'help_doc' is
obtained with a call to get_doc_string (python/py-param.c). This
function either returns the __doc__ string, or a default string: "This
command is not documented.".
To avoid returning the default we could try setting __doc__ to an
empty string, but setting this field to any string means that GDB
prints a line for that string, like this:
class test_param(gdb.Parameter):
__doc__ = ""
def __init__(self, name):
super ().__init__(name, gdb.COMMAND_NONE, gdb.PARAM_BOOLEAN)
self.value = True
test_param('print test')
Then in GDB:
(gdb) help set print test
Set the current value of 'print test'.
(gdb)
The blank line is the problem I'd like to solve.
This commit makes a couple of changes to how parameter doc strings are
handled.
If the doc string is set to an empty string, then GDB now converts
this to nullptr, which removes the blank line problem, the new
behaviour in GDB (for the above `test_param`) is:
(gdb) help set print test
Set the current value of 'print test'.
(gdb)
Next, I noticed that if the set/show docs are set to empty strings,
then the results are less than ideal:
class test_param(gdb.Parameter):
set_doc = ""
def __init__(self, name):
super ().__init__(name, gdb.COMMAND_NONE, gdb.PARAM_BOOLEAN)
self.value = True
test_param('print test')
And in GDB:
(gdb) help set print test
This command is not documented.
(gdb)
So, if the set/show docs are the empty string, GDB now forces these to
be the default string instead, the new behaviour in GDB is:
(gdb) help set print test
Set the current value of 'print test'.
This command is not documented.
(gdb)
I've added some additional asserts; the set/show docs should always be
non-empty strings, which I believe is the case after this commit. And
the 'doc' string returned from get_doc_string should never nullptr,
but could be empty.
This completes without error, however, we didn't get what we were
maybe expecting:
(gdb) show print xxx foo
Undefined show print command: "xxx foo". Try "help show print".
But we did get:
(gdb) show print foo
The current value of 'print foo' is "off".
GDB stopped scanning the prefix string at the unknown 'xxx', and just
created the parameter there. I don't think this makes sense, nor is
it inline with the manual.
An identical problem exists with gdb.Command creation; GDB stops
parsing the prefix at the first unknown prefix, and just creates the
command there. The manual for gdb.Command says:
NAME is the name of the command. If NAME consists of multiple
words, then the initial words are looked for as prefix commands.
In this case, if one of the prefix commands does not exist, an
exception is raised.
So again, the correct action is, I believe, to raise an exception.
The problem is in gdbpy_parse_command_name (python/py-cmd.c), GDB
calls lookup_cmd_1 to look through the prefix string and return the
last prefix group. If the very first prefix word is invalid then
lookup_cmd_1 returns NULL, and this case is handled. However, if
there is a valid prefix, followed by an invalid prefix, then
lookup_cmd_1 will return a pointer to the last valid prefix list, and
will update the input argument to point to the start of the invalid
prefix word. This final case, where the input is left pointing to an
unknown prefix, was previously not handled.
I've fixed gdbpy_parse_command_name, and added tests for command and
parameter creation to cover this case.
The exact same error is present in the guile API too. The guile
documentation for make-parameter and make-command says the same things
about unknown prefixes resulting in an exception, but the same error
is present in gdbscm_parse_command_name (guile/scm-cmd.c), so I've
fixed that too, and added some tests.
Simon Marchi [Tue, 6 May 2025 17:03:47 +0000 (13:03 -0400)]
gdb/dwarf: skip broken .debug_macro.dwo
Running gdb.base/errno.exp with gcc <= 13 with split DWARF results in:
$ make check TESTS="gdb.base/errno.exp" RUNTESTFLAGS="CC_FOR_TARGET=gcc-13 --target_board=fission"
(gdb) break -qualified main
/home/smarchi/src/binutils-gdb/gdb/dwarf2/read.c:7549: internal-error: locate_dwo_sections: Assertion `!dw_sect->readin' failed.
A problem internal to GDB has been detected,
further debugging may prove unreliable.
...
FAIL: gdb.base/errno.exp: macros: gdb_breakpoint: set breakpoint at main (GDB internal error)
The assert being hit has been added in 28f15782adab ("gdb/dwarf: read
multiple .debug_info.dwo sections"), but it merely exposed an existing
problem.
Basically, it produces .dwo files with multiple .debug_macro.dwo
sections, with some unresolved links between them. I think that this
macro debug info is unusable, and all we can do is ignore it.
In locate_dwo_sections, if we detect a second .debug_macro.dwo section,
forget about the previous .debug_macro.dwo and any subsequent one. This
will effectively make it as if the macro debug info wasn't there at all.
The errno test seems happy with it:
# of expected passes 84
# of expected failures 8
Change-Id: I6489b4713954669bf69f6e91865063ddcd1ac2c8 Approved-By: Tom Tromey <tom@tromey.com>
Simon Marchi [Tue, 6 May 2025 17:03:46 +0000 (13:03 -0400)]
gdb/dwarf: move loops into locate_dw{o,z}_sections
For a subsequent patch, it would be easier if the loop over sections
inside locate_dwo_sections (I want to maintain some state for the
duration of the loop). Move the for loop in there. And because
locate_dwz_sections is very similar, modify that one too, to keep both
in sync.
Change-Id: I90b3d44184910cc2d86af265bb4b41828a5d2c2e Approved-By: Tom Tromey <tom@tromey.com>
oltolm [Sat, 10 May 2025 08:56:12 +0000 (10:56 +0200)]
gdb/dap: fix decode_source
The documentation for the Source interface says
* The path of the source to be shown in the UI.
* It is only used to locate and load the content of the source if no
* `sourceReference` is specified (or its value is 0).
but the code used `path` first. I fixed it to use `sourceReference` first.
Keith Seitz [Mon, 12 May 2025 16:28:02 +0000 (09:28 -0700)]
[PATCH] Add syscall tests when following/detaching from fork
breakpoints/13457 discusses issues with syscall catchpoints when
following forks, lamenting that there is no coverage for the
various permutations of `follow-fork-mode' and `detach-on-fork'.
This is an attempt to try and cover some of this ground. Unfortunately
the state of syscall support when detaching after the fork is
very, very inconsistent across various architectures. [I've tested
extensively Fedora/RHEL platforms.]
Right now, the only reliable platform to run tests on is x86_64/i?86
for the specific case where we do not detach from the fork. Consequently,
this patch limits testing to those architectures.
I have updated breakpoints/13457 with my findings on failures with the
detaching case.
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=13457 Approved-By: Andrew Burgess <aburgess@redhat.com>
Ezra Sitorus [Fri, 2 May 2025 17:08:21 +0000 (18:08 +0100)]
aarch64: Support for FEAT_RME_GPC3
FEAT_RME_GPC3 - RME Granule Protection Check 3 Extension - introduces
a method for defining a set of windows in the memory map for which
Granule Protection Checks are skipped, and instead applies a set of
default settings associated with the window.
This patch introduces the sysreg gpcbw_el3. Add -march=armv9.5-a to
access this sysreg since this feature is optional from armv9.5-a.
Ezra Sitorus [Fri, 2 May 2025 16:14:07 +0000 (17:14 +0100)]
aarch64: Support for FEAT_OCCMO
FEAT_OCCMO - Outer Cacheable Cache Maintenance Operation - introduces
system instructions that provides software with a mechanism to publish
writes to the Outer cache level.
Patrick Monnerat [Mon, 12 May 2025 15:36:17 +0000 (17:36 +0200)]
gdbsupport/event-loop: do not truncate poll timeouts to lower second
In update_wait_timeout function, microseconds were not taken into account
in poll timeout computation, resulting in 100% cpu time consumption in
the event loop while waiting for a sub-second timeout.
This patch adds the microseconds converted to milliseconds in poll
timeout computation. Conversion by excess (ceil) is performed to
avoid the same problem with sub-millisecond timeouts too.
Andrew Burgess [Wed, 7 May 2025 10:00:13 +0000 (11:00 +0100)]
gdb: pass std::string from linux_find_memory_regions_full
Update linux_find_memory_region_ftype to take 'const std::string &'
instead of 'const char *', update the two functions which are passed
as callbacks to linux_find_memory_regions_full.
There should be no user visible changes after this commit.
Approved-By: Simon Marchi <simon.marchi@efficios.com>
Andrew Burgess [Wed, 7 May 2025 09:53:41 +0000 (10:53 +0100)]
gdb: move extra checks into dump_note_entry_p
Now that dump_note_entry_p is always called (see previous commit), we
can move some of the checks out of linux_make_mappings_callback into
dump_note_entry_p.
The checks only exist in linux_make_mappings_callback because, before
the previous commit, we couldn't be sure that dump_note_entry_p would
be called or not, so linux_make_mappings_callback had to run its own
checks.
Now that dump_note_entry_p is always called we can rely on that
function to filter out which mappings should result in an NT_FILE
entry, and linux_make_mappings_callback can just create an entry for
everything it is passed.
As a result of this change I was able to remove the inode argument
from linux_make_mappings_callback and
linux_find_memory_regions_thunk. The inode check has now moved to
dump_note_entry_p.
There should be no user visible changes after this commit.
Approved-By: Simon Marchi <simon.marchi@efficios.com>
Andrew Burgess [Wed, 7 May 2025 09:33:09 +0000 (10:33 +0100)]
gdb: always call should_dump_mapping_p during core file creation
This commit moves the logic for whether should_dump_mapping_p is
called out of linux_find_memory_regions_full and pushes it down into
the two callback functions that are used as the should_dump_mapping_p
callback; `dump_mapping_p` and `dump_note_entry_p`.
Older Linux kernels don't make the 'Anonymous' information available
in the smaps file, and currently, GDB handles this by not calling the
should_dump_mapping_p callback in linux_find_memory_regions_full,
instead the answer is hard-coded to true.
This is (maybe) fine for dump_mapping_p, but for dump_note_entry_p,
this choice makes little sense. The dump_note_entry_p function
doesn't even use the anonymous mapping information.
I propose that the 'has_anonymous' check should be moved out of
linux_find_memory_regions_full, and pushed into dump_mapping_p. Then
in dump_note_entry_p there will be no has_anonymous check; it just
isn't needed.
This allows linux_find_memory_regions_full to be simplified a little,
and will allow some additional clean ups in
linux_make_mappings_callback, which is the partner function to
dump_note_entry_p (see linux_make_mappings_corefile_notes), now that
we know dump_note_entry_p is always called. This follow on clean up
will be done in a later commit in this series.
Looking at dump_mapping_p, I do wonder if the ::has_anonymous check
could be moved later in the function. The first few checks in
dump_mapping_p don't rely on the anonymous information, so running
them might give better results. However, the lack of the anonymous
information is only for older kernels, so testing any changes in this
area would likely require spinning up an older kernel, and as the
years pass, we likely care about this case less. So for now I've left
the ::has_anonymous check as the first thing in dump_mapping_p as this
keeps the existing behaviour.
There should be no user visible changes after this commit.
Approved-By: Simon Marchi <simon.marchi@efficios.com>
Andrew Burgess [Wed, 7 May 2025 09:22:37 +0000 (10:22 +0100)]
gdb: pass struct smaps_data to linux_dump_mapping_p_ftype
Simplify the argument passing in linux_find_memory_regions_full when
calling the should_dump_mapping_p callback. Instead of pulling all
the components from the smaps_data object and passing them separately,
just pass the smaps_data object.
I think this change is justified on its own; the code seems cleaner,
and easier to read to my eye. But additionally, in a later commit in
this series I want to pass smaps_data::has_anonymous to the
should_dump_mapping_p callback, which would mean adding yet another
argument, and I think the argument list is already long enough.
Changing the function now to pass the smaps_data object means that I
will already have the ::has_anonymous field available in the later
commit.
There should be no user visible changes after this commit.
Approved-By: Simon Marchi <simon.marchi@efficios.com>
gdb: add '-stopped' and '-running' options to "info threads"
Add two options to "info threads": `-stopped` and `-running`.
The purpose of these options is to filter the output of the command.
The `-stopped` option means "print stopped threads only" and,
similarly, `-running` means "print the running threads only". When
both options are provided by the user, the indication is that the user
wants the union. That is, the output contains both stopped and
running threads.
Suppose we have an application with 5 threads, 2 of which have hit a
breakpoint. The "info threads" command in the non-stop mode gives:
(gdb) info threads
Id Target Id Frame
* 1 Thread 0x7ffff7d99740 (running)
2 Thread 0x7ffff7d98700 something () at file.c:30
3 Thread 0x7ffff7597700 (running)
4 Thread 0x7ffff6d96700 something () at file.c:30
5 Thread 0x7ffff6595700 (running)
(gdb)
Using the "-stopped" flag, we get
(gdb) info threads -stopped
Id Target Id Frame
2 Thread 0x7ffff7d98700 something () at file.c:30
4 Thread 0x7ffff6d96700 something () at file.c:30
(gdb)
Using the "-running" flag, we get
(gdb) info threads -running
Id Target Id Frame
* 1 Thread 0x7ffff7d99740 (running)
3 Thread 0x7ffff7597700 (running)
5 Thread 0x7ffff6595700 (running)
(gdb)
Using both flags prints all:
(gdb) info threads -stopped -running
Id Target Id Frame
* 1 Thread 0x7ffff7d99740 (running)
2 Thread 0x7ffff7d98700 something () at file.c:30
3 Thread 0x7ffff7597700 (running)
4 Thread 0x7ffff6d96700 something () at file.c:30
5 Thread 0x7ffff6595700 (running)
(gdb)
When combined with a thread ID, filtering applies to those threads that
are matched by the ID.
(gdb) info threads 3
Id Target Id Frame
3 Thread 0x7ffff7597700 (running)
(gdb) info threads -stopped 3
No threads matched.
(gdb)
Regression-tested on X86_64 Linux.
Reviewed-By: Eli Zaretskii <eliz@gnu.org> Reviewed-By: Guinevere Larsen <guinevere@redhat.com> Approved-by: Pedro Alves <pedro@palves.net
gdb: update "info threads" output when no threads match the arguments
If "info threads" is provided with the thread ID argument but no such
threads matching the thread ID(s) are found, GDB prints
No threads match '<ID...>'.
Update this output to the more generalized
No threads matched.
The intention is that the next patch, and potentially future ones,
will extend the command with more filter/match arguments. We cannot
customize the output to each such argument. Hence, be more generic.
Reviewed-By: Eli Zaretskii <eliz@gnu.org> Approved-by: Pedro Alves <pedro@palves.net
gdb: pass info_threads_opts to print_thread_info_1
The "info threads" command tracks its options in a struct named
'info_threads_opts', which currently has only one option. Pass the
whole options object to helper functions, instead of passing
the option value individually. This is a refactoring to make adding
more options easier.
Reviewed-By: Guinevere Larsen <guinevere@redhat.com> Approved-by: Pedro Alves <pedro@palves.net
Alan Modra [Fri, 9 May 2025 03:46:24 +0000 (13:16 +0930)]
msan: use of uninitialised data in get_cie_info
This completely bogus oss-fuzz x86 testcase results in a read from an
uninitialised (at the time check_eh_frame is called) part of an insn
frag:
.section .debug_frame
orl $1,x
.long x
.uleb128 0,x,0
x:
Fix the problem by verifying the assumption in get_cie_info that a CIE
starts at the beginning of .eh_frame or .debug_frame. Or at least
exclude silliness involving instructions placed there. That seems a
useful sanity check. Also sanity check sizes of initial FDE fields.
Yes, this doesn't completely stop the problem since you could place an
insn with a relocated field later in the CIE. If fuzzers find such a
testcase I'll ignore it.
* ehopt.c (struct cie_info): Add "f" field.
(get_cie_info): Return a bool. Verify frag at start of chain
is one with the CIE size found by check_eh_frame.
(check_eh_frame): Save CIE start frag. Only accept 4 or 8
byte fields in state_saw_size, state_saw_cie_offset and
state_saw_pc_begin. Formatting. Localise "fix" variable.
Tom Tromey [Fri, 9 May 2025 19:39:55 +0000 (13:39 -0600)]
Move "show style sources" documentation
I noticed that I had inadvertently put the "set style warning-prefix"
documentation between the paragraph for "set style sources" and the
paragraph for "show style sources". This patch moves the latter up a
bit to clean this up.
Alice Carlotti [Sun, 20 Apr 2025 20:42:39 +0000 (21:42 +0100)]
aarch64: Mark predicate-as-counter pseudo instructions
Using explicit pseudo aliases is clearer and more consistent with other
instruction aliases.
This does not change behaviour. For the non-alias instructions
(everything except mov) we already picked the first matching entry for
disassembly by default. For mov we picked the last matching aliased
entry, which remained the original alias since do_misc_decoding doesn't
recognise OP_MOV_PN_PN.
Alice Carlotti [Thu, 17 Apr 2025 19:42:32 +0000 (20:42 +0100)]
aarch64: Mark clearbhb as a pseudo instruction
This was an early name for the clrbhb hint instruction. Some software
was written with the old name before it was renamed, so we support it
for assembly but should never use it in disassembly.
This patch has no functional change, because we already pick (by
default) the last matching alias in the opcode table, and clrbhb is
listed later than clearbhb.
projects / thirdparty / binutils-gdb.git / log
