Rather than trying to fix this (which would require making an assumption
on the upper half of %esp being zero), simply issue an error. While at
it, since the generated code is in conflict with -momit-lock-prefix=yes,
issue an error in that case as well.
This removes the print_spaces helper function, in favor of using the
"*%s" idiom that's already used in many places in gdb. One spot (in
symmisc.c) is changed to use print_spaces_filtered, because the rest
of that function is using filtered output. (This highlights one way
that the printf idiom is better -- this error is harder to make when
using that.)
Regression tested on x86-64 Fedora 34.
I noticed that puts_debug isn't used in the tree. git log tells me
that the last use was removed in 2015:
commit 40e0b27177
Author: Pedro Alves <palves@redhat.com>
Date: Mon Aug 24 15:40:26 2015 +0100
Delete the remaining ROM monitor targets
... and this commit mentions that the code being removed here probably
hadn't worked for 6 years prior to that.
Based on this, I'm removing puts_debug. I don't think it's useful.
Tested by rebuilding.
n_spaces keeps the spaces in a static buffer. If a caller overwrites
these, it may give an incorrect result to a subsequent caller. So,
make the return type const to help avoid this outcome.
This command adds the "exit" command as an alias for the "quit"
command, as requested in PR gdb/28406.
The documentation is also updated to mention this new command.
Tested on x86_64-linux.
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=28406
While working on another patch I ended up in a situation where I had
async mode disabled (with 'maint set target-async off'), but the async
event token got marked anyway.
In this situation GDB was continually calling into
remote_target::wait, however, the async token would never become
unmarked as the unmarking is guarded by target_is_async_p.
We could just unconditionally unmark the token, but that would feel
like just ignoring a bug, so, instead, lets assert that if
!target_is_async_p, then the async token should not be marked.
This assertion would have caught my earlier mistake.
There should be no user visible changes with this commit.
While working on another patch relating to remote targets, I wanted to
test with 'maint set target-async off' in place. Unfortunately I ran
into some problems. This commit is an attempt to fix one of the
issues I hit.
In my particular case I was actually running with:
maint set target-async off
maint set target-non-stop off
that is, we're telling GDB to force the targets to operate in
non-async mode, and in all-stop mode. Here's my GDB session showing
the problem:
(gdb) maintenance set target-async off
(gdb) maintenance set target-non-stop off
(gdb) target extended-remote :54321
Remote debugging using :54321
(gdb) attach 2365960
Attaching to process 2365960
No unwaited-for children left.
(gdb)
Notice the 'No unwaited-for children left.' error, this is the
problem. There's no reason why GDB should not be able to attach to
the process.
The problem is this:
1. The user runs 'attach PID' and this sends GDB into attach_command
in infcmd.c. From here we call the ::attach method on the attach
target, which will be the extended_remote_target.
2. In extended_remote_target::attach, we attach to the remote target
and get the first reply (which is a stop packet). We put off
processing the stop packet until the end of ::attach. We setup the
inferior and thread to represent the process we attached to, and
download the target description. Finally, we process the initial
stop packet.
If '!target_is_non_stop_p ()' and '!target_can_async_p ()', which is
the case for us given the maintenance commands we used, we cache the
stop packet within the remote_state::buf for later processing.
3. Back in attach_command, if 'target_is_non_stop_p ()' then we
request that the target stops. This will either process any cached
stop replies, or request that the target stops, and process the stop
replies. However, this code is not what we use due to non-stop mode
being disabled. So, we skip to the next step which is to call
validate_exec_file.
4. Calling validate_exec_file can cause packets to be sent to the
remote target, and replies received, the first path I hit is the
call to target_pid_to_exec_file, which calls
remote_target::pid_to_exec_file, which can then try to read the
executable from the remote. Sending an receiving packets will make
use of the remote_state::buf object.
5. The attempt to attach continues, but the damage is already done...
So, the problem is that, in step #2 we cache a stop reply in the
remote_state::buf, and then in step #4 we reuse the remote_state::buf
object, discarding any cached stop reply. As a result, the initial
stop, which is sent when GDB first attaches to the target, is lost.
This problem can clearly be seen, I feel, by looking at the
remote_state::cached_wait_status flag. This flag tells GDB if there
is a wait status cached in remote_state::buf. However, in
remote_target::putpkt_binary and remote_target::getpkt_or_notif_sane_1
this flag is just set back to 0, doing this immediately discards any
cached data.
I don't know if this scheme ever made sense, looking at commit
2d717e4f8a, where the cached_wait_status flag was added, it appears
that there was nothing between where the stop was cached, and where
the stop was consumed, so, I suspect, there never was a situation
where we ended up in putpkt_binary or getpkt_or_notif_sane_1 and
needed to clear to the flag, maybe the clearing was added "just in
case". Whatever the history, I claim that this clearing this flag is
no longer a good idea.
So, my first step toward fixing this issue was to replace the two
instances of 'rs->cached_wait_status = 0;' in ::putpkt_binary and
::getpkt_or_notif_sane_1 with 'gdb_assert (rs->cached_wait_status ==
0);', this, at least would show me when GDB was doing something
dangerous, and indeed, this assert is now hit in my test case above.
I did play with using some kind of scoped restore to backup, and
restore the remote_state::buf object in all the places within remote.c
that I was hitting where the ::buf was being corrupted. The first
problem with this is that, where the ::cached_wait_status flag is
reset is _not_ where ::buf is corrupted. For the ::putpkt_binary
case, by the time we get to the method the buffer has already been
corrupted in many cases, so we end up needing to add the scoped
save/restore within the callers, which means we need the save/restore
in _lots_ of places.
Plus, using this save/restore model feels like the wrong solution. I
don't think that it's obvious that the buffer might be holding cached
data, and I think it would be too easy for new corruptions of the
buffer to be introduced, which could easily go unnoticed for a long
time.
So, I really wanted a solution that didn't require us to cache data in
the ::buf object.
Luckily, I think we already have such a solution in place, the
remote_state::stop_reply_queue, it seems like this does exactly the
same task, just in a slightly different way. With the
::stop_reply_queue, the stop packets are processed upon receipt and
the stop_reply object is added to the queue. With the ::buf cache
solution, the unprocessed stop reply is cached in the ::buf, and
processed later.
So, finally, in this commit, I propose to remove the
remote_state::cached_wait_status flag and to stop using the ::buf to
cache stop replies. Instead, stop replies will now always be stored
in the ::stop_reply_queue.
There are two places where we use the ::buf to hold a cached stop
reply, the first is in the ::attach method, and the second is in
remote_target::start_remote, however, the second of these cases is far
less problematic, as after caching the stop reply in ::buf we call the
global start_remote function, which does very little work before
calling normal_stop, which processes the cached stop reply. However,
my plan is to switch both users over to using ::stop_reply_queue so
that the old (unsafe) ::cached_wait_status mechanism can be completely
removed.
The next problem is that the ::stop_reply_queue is currently only used
for async-mode, and so, in remote_target::push_stop_reply, where we
push stop_reply objects into the ::stop_reply_queue, we currently also
mark the async event token. I've modified this so we only mark the
async event token if 'target_is_async_p ()' - note, _is_, not _can_
here. The ::push_stop_reply method is called in places where async
mode has been temporarily disabled, but, when async mode is switched
back on (see remote_target::async) we will mark the event token if
there are events in the queue.
Another change of interest is in remote_target::remote_interrupt_as.
Previously this code checked ::cached_wait_status, but didn't check
for events in the ::stop_reply_queue. Now that ::cached_wait_status
has been removed we now check the queue length instead, which should
have the same result.
Finally, in remote_target::wait_as, I've tried to merge the processing
of the ::stop_reply_queue with how we used to handle the
::cached_wait_status flag.
Currently, when processing the ::stop_reply_queue we call
process_stop_reply and immediately return. However, when handling
::cached_wait_status we run through the whole of ::wait_as, and return
at the end of the function.
If we consider a standard stop packet, the two differences I see are:
1. Resetting of the remote_state::waiting_for_stop_reply, flag; this
is not currently done when processing a stop from the
::stop_reply_queue.
2. The final return value has the possibility of being adjusted at
the end of ::wait_as, as well as there being calls to
record_currthread, non of which are done if we process a stop from
the ::stop_reply_queue.
After discussion on the mailing list:
https://sourceware.org/pipermail/gdb-patches/2021-December/184535.html
it was suggested that, when an event is pushed into the
::stop_reply_queue, the ::waiting_for_stop_reply flag is never going
to be set. As a result, we don't need to worry about the first
difference. I have however, added a gdb_assert to validate the
assumption that the flag is never going to be set. If in future the
situation ever changes, then we should find out pretty quickly.
As for the second difference, I have resolved this by having all stop
packets taken from the ::stop_reply_queue, pass through the return
value adjustment code at the end of ::wait_as.
An example of a test that reveals the benefits of this commit is:
make check-gdb \
RUNTESTFLAGS="--target_board=native-extended-gdbserver \
GDBFLAGS='-ex maint\ set\ target-async\ off \
-ex maint\ set\ target-non-stop\ off' \
gdb.base/attach.exp"
For testing I've been running test on x86-64/GNU Linux, and run with
target boards unix, native-gdbserver, and native-extended-gdbserver.
For each board I've run with the default GDBFLAGS, as well as with:
GDBFLAGS='-ex maint\ set\ target-async\ off \
-ex maint\ set\ target-non-stop\ off' \
Though running with the above GDBFLAGS is clearly a lot more unstable
both before and after my patch, I'm not seeing any consistent new
failures with my patch, except, with the native-extended-gdbserver
board, where I am seeing new failures, but only because more tests are
now running. For that configuration alone I see the number of
unresolved go down by 49, the number of passes goes up by 446, and the
number of failures also increases by 144. All of the failures are new
tests as far as I can tell.
The new gdbarch generator is a Python program. It reads the
"components.py" that was created in the previous patch, and generates
gdbarch.c and gdbarch-gen.h.
This is a relatively straightforward translation of the existing .sh
code. It doesn't try very hard to be idiomatic Python or to be
especially smart.
It is, however, incredibly faster:
$ time ./gdbarch.sh
real 0m8.197s
user 0m5.779s
sys 0m3.384s
$ time ./gdbarch.py
real 0m0.065s
user 0m0.053s
sys 0m0.011s
Co-Authored-By: Tom Tromey <tom@tromey.com>
The new gdbarch.sh approach will be to edit a Python file, rather than
adding a line to a certain part of gdbarch.sh. We use the existing sh
code, though, to generate the first draft of this .py file.
Documentation on the format will come in a subsequent patch.
Note that some info (like "staticdefault") in the current code is
actually unused, and so is ignored by this new generator.
This changes gdbarch.sh so that it no longer sorts the fields in
gdbarch_dump. This sorting isn't done anywhere else by gdbarch.sh,
and this simplifies the new generator a little bit.
This patch splits gdbarch.h into two files -- gdbarch.h now is
editable and hand-maintained, and the new gdbarch-gen.h file is the
only thing generated by gdbarch.sh. This lets us avoid maintaining
boilerplate in the gdbarch.sh file.
Note that gdbarch.sh still generates gdbarch.h after this patch. This
makes it easier to re-run when rebasing. This code is removed in a
subsequent patch.
While I think it makes sense to generate gdbarch.c, at the same time I
think it is better for ordinary code to be editable in a C file -- not
as a hunk of C code embedded in the generator.
This patch moves this sort of code out of gdbarch.sh and gdbarch.c and
into arch-utils.c, then has arch-utils.c include gdbarch.c.
Move inner dimension's element type determination outside the respective
loops in `fortran_array_walker'. The operation is exactly the same with
each iteration, so there is no point in redoing it for each element and
while a smart compiler might be able to move it outside the loop it is
regardless a bad coding style. No functional change.
Following our coding convention initialize the `m_ndimensions' member in
the member initializer list rather than in the body of the constructor
of the `fortran_array_walker' class. No functional change.
PR26056 reports that when GDB is connected to non-TTY stdin/stdout, it
crashes when it receives a SIGWINCH signal.
This can be reproduced as follows:
$ gdb/gdb -nx -batch -ex 'run' --args sleep 60 </dev/null 2>&1 | cat
# from another terminal:
$ kill -WINCH %(pidof gdb)
When doing so, the process crashes in a call to rl_resize_terminal:
void
rl_resize_terminal (void)
{
_rl_get_screen_size (fileno (rl_instream), 1);
...
}
The problem is that at this point rl_instream has the value NULL.
The rl_instream variable is supposed to be initialized during a call to
readline_initialize_everything, which in a normal startup sequence is
called under this call chain:
tui_interp::init
tui_ensure_readline_initialized
rl_initialize
readline_initialize_everything
In tui_interp::init, we have the following sequence:
tui_initialize_io ();
tui_initialize_win (); // <- Installs SIGWINCH
if (gdb_stdout->isatty ())
tui_ensure_readline_initialized (); // <- Initializes rl_instream
This function unconditionally installs the SIGWINCH signal handler (this
is done by tui_initialize_win), and then if gdb_stdout is a TTY it
initializes readline. Therefore, if stdout is not a TTY, SIGWINCH is
installed but readline is not initialized. In such situation
rl_instream stays NULL, and when GDB receives a SIGWINCH it calls its
handler and in fine tries to access rl_instream leading to the crash.
This patch proposes to fix this issue by installing the SIGWINCH signal
handler only if GDB is connected to a TTY. Given that this
initialization it the only task of tui_initialize_win, this patch moves
tui_initialize_win just after the call to
tui_ensure_readline_initialized.
Tested on x86_64-linux.
Co-authored-by: Pedro Alves <pedro@palves.net>
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=26056
Change-Id: I6458acef7b0d9beda2a10715d0345f02361076d9
* elf-bfd.h (struct output_elf_obj_tdata): Make num_section_syms
unsigned.
* elf.c (bfd_elf_set_group_contents): Bounds check sec->index
and check that entry in elf_section_syms for sec is non-NULL.
(_bfd_elf_symbol_from_bfd_symbol): Adjust.
Leaving entries on mips_hi16_list from a previous pass over relocs
leads to confusing bugs.
* elfxx-mips.c (_bfd_elf_mips_get_relocated_section_contents):
Free mips_hi16_list entries on error exit.
Macros like READ_LEB128 in wasm-module.c that alter control flow are
evil. Maintainers will break your code if you have hidden ways to
reach labels.
* wasm-module.c (wasm_scan_name_function_section): Don't
attempt to bfd_release NULL.
The bpf reloc howtos are a bit weird, using bitpos to specify an
offset from r_offset that is outside the size of the reloc as given by
howto.size. That means bfd_get_reloc_size gives the wrong answer for
range checking, and thus bfd_reloc_offset_in_range can't be used.
* elf64-bpf.c (bpf_elf_generic_reloc): Handle bitpos offset reloc
range checking.
We can't assume .dynamic is a multiple of ElfNN_External_Dyn, at least
not when presented with fuzzed object files.
* elfnn-aarch64.c (get_plt_type): Don't access past end of
improperly sized .dynamic.
dwarf.c:11300:3: error: format not a string literal and no format arguments [-Werror=format-security]
11300 | f += sprintf (f, prefix);
PR 28697
* dwarf.c (try_build_id_prefix): Avoid -Wformat-security error.
This patch adds AArch32 support for -march=armv9.[123]-a.
The behaviour of the new options can be expressed using a
combination of existing feature flags and tables.
The cpu_arch_ver entries for ARM_ARCH_V9_2A and ARM_ARCH_V9_3A
are technically redundant but it seemed less surprising to include
them anyway.
include/
* opcode/arm.h (ARM_ARCH_V9_1A, ARM_ARCH_V9_2A): New macros.
(ARM_ARCH_V9_3A): Likewise.
gas/
* doc/c-arm.texi: Add armv9.1-a, armv9.2-a and armv9.3-a.
* config/tc-arm.c (armv91a_ext_table, armv92a_ext_table): New macros.
(armv93a_ext_table): Likewise.
(arm_archs): Add armv9.1-a, armv9.2-a and armv9.3-a.
(cpu_arch_ver): Add ARM_ARCH_V9_1A, ARM_ARCH_V9_2A and ARM_ARCH_V9_3A.
* NEWS: Mention the above.
* testsuite/gas/arm/attr-march-armv9_1-a.d: New test.
* testsuite/gas/arm/attr-march-armv9_2-a.d: Likewise.
* testsuite/gas/arm/attr-march-armv9_3-a.d: Likewise.
* testsuite/gas/arm/bfloat16-armv9.1-a.d: Likewise.
* testsuite/gas/arm/bfloat16-armv9.2-a.d: Likewise.
* testsuite/gas/arm/bfloat16-armv9.3-a.d: Likewise.
* testsuite/gas/arm/i8mm-armv9.1-a.d: Likewise.
* testsuite/gas/arm/i8mm-armv9.2-a.d: Likewise.
* testsuite/gas/arm/i8mm-armv9.3-a.d: Likewise.
This patch adds AArch32 support for -march=armv8.[78]-a.
The behaviour of the new options can be expressed using a
combination of existing feature flags and tables.
The cpu_arch_ver entries are technically redundant but
it seemed less surprising to include them anyway.
include/
* opcode/arm.h (ARM_ARCH_V8_7A, ARM_ARCH_V8_8A): New macros.
gas/
* doc/c-arm.texi: Add armv8.7-a and armv8.8-a.
* config/tc-arm.c (armv87a_ext_table, armv88a_ext_table): New macros.
(arm_archs): Add armv8.7-a and armv8.8-a.
(cpu_arch_ver): Add ARM_ARCH_V8_7A and ARM_ARCH_V8_8A.
* NEWS: Mention the above.
* testsuite/gas/arm/attr-march-armv8_7-a.d: New test.
* testsuite/gas/arm/attr-march-armv8_8-a.d: Likewise.
* testsuite/gas/arm/bfloat16-armv8.7-a.d: Likewise.
* testsuite/gas/arm/bfloat16-armv8.8-a.d: Likewise.
* testsuite/gas/arm/i8mm-armv8.7-a.d: Likewise.
* testsuite/gas/arm/i8mm-armv8.8-a.d: Likewise.
This patch adds AArch64 support for -march=armv9.[123]-a.
The behaviour of the new options can be expressed using a
combination of existing feature flags, so we don't need to
eat into the vanishing number of spare AARCH64_FEATURE_* bits.
Hoewver, it was more convenient to separate out the |s of
feature flags so that Armv9.1-A could reuse the set for
Armv8.6-A, and so on.
include/
* opcode/aarch64.h (AARCH64_ARCH_V8_FEATURES): New macro,
split out from...
(AARCH64_ARCH_V8): ...here.
(AARCH64_ARCH_V8_1_FEATURES): New macro, split out from...
(AARCH64_ARCH_V8_1): ...here.
(AARCH64_ARCH_V8_2_FEATURES): New macro, split out from...
(AARCH64_ARCH_V8_2): ...here.
(AARCH64_ARCH_V8_3_FEATURES): New macro, split out from...
(AARCH64_ARCH_V8_3): ...here.
(AARCH64_ARCH_V8_4_FEATURES): New macro, split out from...
(AARCH64_ARCH_V8_4): ...here.
(AARCH64_ARCH_V8_5_FEATURES): New macro, split out from...
(AARCH64_ARCH_V8_5): ...here.
(AARCH64_ARCH_V8_6_FEATURES): New macro, split out from...
(AARCH64_ARCH_V8_6): ...here.
(AARCH64_ARCH_V8_7_FEATURES): New macro, split out from...
(AARCH64_ARCH_V8_7): ...here.
(AARCH64_ARCH_V8_8_FEATURES): New macro, split out from...
(AARCH64_ARCH_V8_8): ...here.
(AARCH64_ARCH_V9_FEATURES): New macro, split out from...
(AARCH64_ARCH_V9): ...here.
(AARCH64_ARCH_V9_1_FEATURES, AARCH64_ARCH_V9_1): New macros.
(AARCH64_ARCH_V9_2_FEATURES, AARCH64_ARCH_V9_2): New macros.
(AARCH64_ARCH_V9_3_FEATURES, AARCH64_ARCH_V9_3): New macros.
gas/
* doc/c-aarch64.texi: Add armv9.1-a, armv9-2-a and armv9.3-a.
* config/tc-aarch64.c (aarch64_archs): Likewise.
* NEWS: Mention the above.
* testsuite/gas/aarch64/armv9_invalid.d,
testsuite/gas/aarch64/armv9_invalid.s,
testsuite/gas/aarch64/armv9_invalid.l: New test.
* testsuite/gas/aarch64/armv9_1.d,
testsuite/gas/aarch64/armv9_1.s: Likewise.
* testsuite/gas/aarch64/armv9_1_invalid.d,
testsuite/gas/aarch64/armv9_1_invalid.s,
testsuite/gas/aarch64/armv9_1_invalid.l: Likewise.
* testsuite/gas/aarch64/armv9_2.d,
testsuite/gas/aarch64/armv9_2.s: Likewise.
* testsuite/gas/aarch64/armv9_2_invalid.d,
testsuite/gas/aarch64/armv9_2_invalid.s,
testsuite/gas/aarch64/armv9_2_invalid.l: Likewise.
* testsuite/gas/aarch64/armv9_3.d,
testsuite/gas/aarch64/armv9_3.s: Likewise.
According to the privileged spec, there are five new instructions for
svinval extension. Two of them (HINVAL.VVMA and HINVAL.GVMA) need to
enable the hypervisor extension. But there is no implementation of
hypervisor extension in mainline for now, so let's consider the related
issues later.
31..25 24..20 19..15 14..12 11...7 6..2 1..0
sinval.vma 0001011 rs2 rs1 000 00000 11100 11
sfence.w.inval 0001100 00000 00000 000 00000 11100 11
sfence.inval.ir 0001100 00001 00000 000 00000 11100 11
hinval.vvma 0010011 rs2 rs1 000 00000 11100 11
hinval.gvma 0110011 rs2 rs1 000 00000 11100 11
This patch is cherry-picked from the riscv integration branch since the
svinval extension is frozen for now. Besides, we fix the funct7 encodings
of hinval.vvma and hinval.gvma, from 0x0011011 and 0x0111011 to 0x0010011
and 0x0110011.
bfd/
* elfxx-riscv.c (riscv_supported_std_s_ext): Added svinval.
(riscv_multi_subset_supports): Handle INSN_CLASS_SVINVAL.
gas/
* testsuite/gas/riscv/svinval.d: New testcase.
* testsuite/gas/riscv/svinval.s: Likewise.
include/
* opcode/riscv-opc.h: Added encodings for svinval.
* opcode/riscv.h (enum riscv_insn_class): Added INSN_CLASS_SVINVAL.
opcodes/
* riscv-opc.c (riscv_opcodes): Added svinval instructions.
These are just using the default behavior for the 3rd arg, so drop
it to make it more clear. This also makes them match all other
ports that only use the first 2 arguments.
The logic between these rules are extremely similar, so unify them
into a single variable by leveraging make $@ and $< variables.
Also add automake silent rule support while we're here.
The header shuffling in here broke the workaround for gnulib defining
"open". Move it back before the sim-specific includes to fix. This
is because the callback struct in the headers has an "open" member and
this file tries to call that.
In a cross toolchain for nios2-elf target and x86_64-w64-mingw32 host
using binutils 2.37, we observed a failure that didn't show up on
x86_64-linux-gnu host: testcase pr25490-5.s was failing with
C:\path\to\nios2-elf-ld.exe: looping in map_segments
FAIL: __patchable_function_entries section 5
* ldelfgen.c (compare_link_order): Don't use section id in
sorting. Keep original ordering instead. Update comments.
