Pedro Alves [Thu, 6 Jul 2023 14:05:11 +0000 (15:05 +0100)]
Fix Solaris regression (PR tdep/30252)
PR tdep/30252 reports that using GDB on Solaris fails an assertion in
target_resume:
target.c:2648: internal-error: target_resume: Assertion `inferior_ptid != null_ptid' failed.
A problem internal to GDB has been detected,
further debugging may prove unreliable.
Quit this debugging session? (y or n)
The backtrace, after running it through c++filt, looks like:
The problem is that the procfs backend, while inside target_wait,
called target_resume without switching to the leader thread of that
resumption.
The target_resume interface is:
/* Resume execution (or prepare for execution) of the current thread
(INFERIOR_PTID), while optionally letting other threads of the
current process or all processes run free.
...
Thus calling target_resume with inferior_ptid == null_ptid is bogus.
target_wait (which leads to procfs_target::wait on Solaris) is called
with inferior_ptid == null_ptid on entry exactly to help catch such
bogus uses.
From the backtrace, it seems that the relevant line in question is
procfs.c:2187:
2186 /* How to keep going without returning to wfi: */
2187 target_continue_no_signal (ptid);
2188 goto wait_again;
target_continue_no_signal is a small wrapper around target_resume,
which would make sense.
The fix is to not call target_resume or go via the target stack at
all. Instead, factor out a new proc_resume function out of
procfs_target::resume, and call that. The new function does not rely
on inferior_ptid.
I've not been able to test it myself, but Petr confirmed it fixes the
assertion failure with his test case, and Marcel Telka also confirmed
it solves the problem.
Jan Vrany [Fri, 19 May 2023 12:20:04 +0000 (13:20 +0100)]
gdb: fix post-hook execution for remote targets
Commit b5661ff2 ("gdb: fix possible use-after-free when
executing commands") attempted to fix possible use-after-free
in case command redefines itself.
Commit 37e5833d ("gdb: fix command lookup in execute_command ()")
updated the previous fix to handle subcommands as well by using the
original command string to lookup the command again after its execution.
This fixed the test in gdb.base/define.exp but it turned out that it
does not work (at least) for "target remote" and "target extended-remote".
The problem is that the command buffer P passed to execute_command ()
gets overwritten in dont_repeat () while executing "target remote"
command itself:
#0 dont_repeat () at top.c:822
#1 0x000055555730982a in target_preopen (from_tty=1) at target.c:2483
#2 0x000055555711e911 in remote_target::open_1 (name=0x55555881c7fe ":1234", from_tty=1, extended_p=0)
at remote.c:5946
#3 0x000055555711d577 in remote_target::open (name=0x55555881c7fe ":1234", from_tty=1) at remote.c:5272
#4 0x00005555573062f2 in open_target (args=0x55555881c7fe ":1234", from_tty=1, command=0x5555589d0490)
at target.c:853
#5 0x0000555556ad22fa in cmd_func (cmd=0x5555589d0490, args=0x55555881c7fe ":1234", from_tty=1)
at cli/cli-decode.c:2737
#6 0x00005555573487fd in execute_command (p=0x55555881c802 "4", from_tty=1) at top.c:688
Therefore the second call to lookup_cmd () at line 697 fails to find
command because the original command string is gone.
This commit addresses this particular problem by creating a *copy* of
original command string for the sole purpose of using it after command
execution to lookup the command again. It may not be the most efficient
way but it's safer given that command buffer is shared and overwritten
in hard-to-foresee situations.
Tom Tromey [Fri, 28 Apr 2023 19:04:15 +0000 (13:04 -0600)]
Correctly handle forward DIE references in scanner
The cooked index scanner has special code to handle forward DIE
references. However, a bug report lead to the discovery that this
code does not work -- the "deferred_entry::spec_offset" field is
written to but never used, i.e., the lookup is done using the wrong
key.
This patch fixes the bug and adds a regression test.
The test in the bug itself used a thread_local variable, which
provoked a failure at runtime. This test instead uses "maint print
objfiles" and then inspects to ensure that the entry in question has a
parent. This lets us avoid a clang dependency in the test.
Simon Marchi [Thu, 23 Feb 2023 17:35:41 +0000 (12:35 -0500)]
gdb: fix -Wsingle-bit-bitfield-constant-conversion warning in z80-tdep.c
When building with clang 16, I see:
/home/smarchi/src/binutils-gdb/gdb/z80-tdep.c:338:32: error: implicit truncation from 'int' to a one-bit wide bit-field changes value from 1 to -1 [-Werror,-Wsingle-bit-bitfield-constant-conversion]
info->prologue_type.load_args = 1;
^ ~
/home/smarchi/src/binutils-gdb/gdb/z80-tdep.c:345:36: error: implicit truncation from 'int' to a one-bit wide bit-field changes value from 1 to -1 [-Werror,-Wsingle-bit-bitfield-constant-conversion]
info->prologue_type.critical = 1;
^ ~
/home/smarchi/src/binutils-gdb/gdb/z80-tdep.c:351:37: error: implicit truncation from 'int' to a one-bit wide bit-field changes value from 1 to -1 [-Werror,-Wsingle-bit-bitfield-constant-conversion]
info->prologue_type.interrupt = 1;
^ ~
/home/smarchi/src/binutils-gdb/gdb/z80-tdep.c:367:36: error: implicit truncation from 'int' to a one-bit wide bit-field changes value from 1 to -1 [-Werror,-Wsingle-bit-bitfield-constant-conversion]
info->prologue_type.fp_sdcc = 1;
^ ~
/home/smarchi/src/binutils-gdb/gdb/z80-tdep.c:375:35: error: implicit truncation from 'int' to a one-bit wide bit-field changes value from 1 to -1 [-Werror,-Wsingle-bit-bitfield-constant-conversion]
info->prologue_type.fp_sdcc = 1;
^ ~
/home/smarchi/src/binutils-gdb/gdb/z80-tdep.c:380:35: error: implicit truncation from 'int' to a one-bit wide bit-field changes value from 1 to -1 [-Werror,-Wsingle-bit-bitfield-constant-conversion]
info->prologue_type.fp_sdcc = 1;
^ ~
Fix that by using "unsigned int" as the bitfield's underlying type.
Simon Marchi [Thu, 23 Feb 2023 17:35:40 +0000 (12:35 -0500)]
gdbsupport: ignore -Wenum-constexpr-conversion in enum-flags.h
When building with clang 16, we get:
CXX gdb.o
In file included from /home/smarchi/src/binutils-gdb/gdb/gdb.c:19:
In file included from /home/smarchi/src/binutils-gdb/gdb/defs.h:65:
/home/smarchi/src/binutils-gdb/gdb/../gdbsupport/enum-flags.h:95:52: error: integer value -1 is outside the valid range of values [0, 15] for this enumeration type [-Wenum-constexpr-conversion]
integer_for_size<sizeof (T), static_cast<bool>(T (-1) < T (0))>::type
^
The error message does not make it clear in the context of which enum
flag this fails (i.e. what is T in this context), but it doesn't really
matter, we have similar warning/errors for many of them, if we let the
build go through.
clang is right that the value -1 is invalid for the enum type we cast -1
to. However, we do need this expression in order to select an integer
type with the appropriate signedness. That is, with the same signedness
as the underlying type of the enum.
I first wondered if that was really needed, if we couldn't use
std::underlying_type for that. It turns out that the comment just above
says:
/* Note that std::underlying_type<enum_type> is not what we want here,
since that returns unsigned int even when the enum decays to signed
int. */
I was surprised, because std::is_signed<std::underlying_type<enum_type>>
returns the right thing. So I tried replacing all this with
std::underlying_type, see if that would work. Doing so causes some
build failures in unittests/enum-flags-selftests.c:
This is a bit hard to decode, but basically enumerations have the
following funny property that they decay into a signed int, even if
their implicit underlying type is unsigned. This code:
enum A {};
enum B {};
int main() {
std::cout << std::is_signed<std::underlying_type<A>::type>::value
<< std::endl;
std::cout << std::is_signed<std::underlying_type<B>::type>::value
<< std::endl;
auto result = true ? A() : B();
std::cout << std::is_signed<decltype(result)>::value << std::endl;
}
produces:
0
0
1
So, the "CHECK_VALID" above checks that this property works for enum flags the
same way as it would if you were using their underlying enum types. And
somehow, changing integer_for_size to use std::underlying_type breaks that.
Since the current code does what we want, and I don't see any way of doing it
differently, ignore -Wenum-constexpr-conversion around it.
projects / thirdparty / binutils-gdb.git / log
