After this commit:
commit 33ae45434d
Date: Mon Dec 4 14:23:17 2023 +0000
gdb: Enable early init of thread pool size
I am now seeing this assert from libstdc++:
/usr/include/c++/9/bits/stl_algo.h:3715: constexpr const _Tp& std::clamp(const _Tp&, const _Tp&, const _Tp&) [with _Tp = int]: Assertion '!(__hi < __lo)' failed.
This may only be visible because I compile with:
-D_GLIBCXX_DEBUG=1 -D_GLIBCXX_DEBUG_PEDANTIC=1
but I haven't checked. The issue the assert is highlighting is real,
and is caused by this block of code:
if (n_threads < 0)
{
const int hardware_threads = std::thread::hardware_concurrency ();
/* Testing in #29959 indicates that parallel efficiency drops between
n_threads=5 to 8. Therefore, clamp the default value to 8 to avoid an
excessive number of threads in the pool on many-core systems. */
const int throttle = 8;
n_threads = std::clamp (hardware_threads, hardware_threads, throttle);
}
The arguments to std::clamp are VALUE, LOW, HIGH, but in the above, if
we have more than 8 hardware threads available the LOW will be greater
than the HIGH, which is triggering the assert I see above.
I believe std::clamp is the wrong tool to use here. Instead std::min
would be a better choice; we want the smaller value of
HARDWARE_THREADS or THROTTLE. If h/w threads is 2, then we want 2,
but if h/w threads is 16 we want 8, this is what std::min gives us.
After this commit, I no longer see the assert.
This reverts commit 1c04f72368 ("[gdb/symtab] Fix assert in set_length"), due
to a regression reported in PR29572, and implements a different fix for PR29453.
The fix is to not use the CU table in a .debug_names section to construct
all_units, but instead use create_all_units, and then verify the CU
table from .debug_names. This also fixes PR25969, so remove the KFAIL.
Approved-By: Tom Tromey <tom@tromey.com>
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=29572
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=25969
Prior to commit 0e3c1eebb2 nm output depended on the host unsigned
long when printing "negative" symbol values for 32-bit targets.
Commit 0e3c1eebb2 made the output match that seen with a 64-bit host
unsigned long. The fact that nm output changed depending on host is
of course a bug, but it is reasonable to expect 32-bit target output
is only 32 bits. So this patch makes 32-bit target output the same as
it was on 32-bit hosts prior to 0e3c1eebb2.
PR 31096
* nm.c (print_format_string): Make it a static buffer.
(get_print_format): Merge into..
(set_print_format): ..this, renamed from set_print_width. When
print_width is 32, set up print_format_string for an int32_t
value. Don't malloc print_format_string. Adjust calls.
(print_value): Correct printing of 32-bit values.
Tobias reported on IRC that the linker fails to build with GCC 4.8.5.
In configure I've tried to use everything actually used in the sha1.c
x86 hw implementation, but unfortunately I forgot about implicit function
declarations. GCC before 7 did have <cpuid.h> header and bit_SHA define
and __get_cpuid function defined inline, but it didn't define
__get_cpuid_count, which compiled fine (and the configure test is
intentionally compile time only) due to implicit function declaration,
but then failed to link when linking the linker, because
__get_cpuid_count wasn't defined anywhere.
The following patch fixes that by using what autoconf uses in AC_CHECK_DECL
to make sure the functions are declared.
2023-12-05 Jakub Jelinek <jakub@redhat.com>
* configure.ac (HAVE_X86_SHA1_HW_SUPPORT): Verify __get_cpuid and
__get_cpuid_count are not implicitly declared.
* configure: Regenerated.
On mingw targets it's possible that there are multiple trampoline
symbols for __cxa_throw, in each module where a throw is done, but
without a corresponding global symbol.
Since commit 77f2120b20 they cancel each
other out in search_minsyms_for_name, which makes it impossible to set
a breakpoint there:
(gdb) b __cxa_throw
Function "__cxa_throw" not defined.
Make breakpoint pending on future shared library load? (y or [n]) y
Breakpoint 2 (__cxa_throw) pending.
(gdb) c
Continuing.
[Inferior 1 (process 10004) exited with code 03]
With catch throw it also doesn't work, and you don't even get an error
message:
(gdb) catch throw
Catchpoint 2 (throw)
(gdb) c
Continuing.
[Inferior 1 (process 5532) exited with code 03]
(gdb)
The fix is to simply ignore other trampoline symbols when looking for
corresponding global symbols, and it's working as expected:
(gdb) b __cxa_throw
Breakpoint 2 at 0x13f091590 (2 locations)
(gdb) c
Continuing.
Breakpoint 2.1, 0x000000013f091590 in __cxa_throw ()
(gdb)
And catch throw also works again:
(gdb) catch throw
Catchpoint 2 (throw)
(gdb) c
Continuing.
Catchpoint 2.1 (exception thrown), 0x000000013f181590 in __cxa_throw ()
(gdb)
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=29548
Approved-By: Tom Tromey <tom@tromey.com>
Commit 33ae45434d updated the text reported by GDB when showing the
number of worker threads. However, it neglected to update the assertions
using this text, which caused index-file.exp to fail. This commit
corrects this omission.
Tested index-file.exp is fixed on my local machine.
Approved-By: Tom Tromey <tom@tromey.com>
PR 31106
* elfcode.h (elf_write_relocs): Do not convert a relocation against a zero-value absolute symbol into a relocation without a symbol if the symbol is being used for a complex relocation.
compute_delayed_physnames does this:
size_t len = strlen (physname);
...
if (physname[len] == ')') /* shortcut */
break;
However, physname[len] will always be \0.
This patch changes it to the correct len-1.
Fix some of the sim_fpu calls to use the right types. While I'm
not familiar with the MIPS ISA in these cases, these look like
simple oversights due to the name/type mismatches. This at least
fixes compiling with -Wenum-conversion.
No functional change here, but makes it a little easier to read the
generated code when editors aren't highlighting all the spurious
trailing whitespace on lines.
The sim_stop argument is an enum and should only be one of those
values, not a signal constant. Fix the logic to pass the right
sim_xxx & SIM_xxx values in the right arguments.
We only support UTF-8 nowadays, so stop using ISO-8859-1.
Maybe we should delete this logic entirely, but for now,
do the bare min conversion to keep it compiling.
This is aimed at fixing holes in two alpha-ecoff relocation functions
that access section contents without first bounds checking offsets.
I've also rewritten ALPHA_R_OP_STORE handling to support writing to
the bytes near the end of the section.
* coff-alpha.c (alpha_ecoff_get_relocated_section_contents): Don't
bother checking ALPHA_R_LITERAL insn. Range check before reading
contents for ALPHA_R_GPDISP, and simplify handling. Rewrite
ALPHA_R_OP_STORE handling. Correct error callback args.
(alpha_relocate_section): Similarly. Don't abort, report errors.
In commit 7ac6d0c38c I made more use of free_contents in
_bfd_elf_slurp_version_tables, a variable added to tag the case where
raw verneed and verdefs have been read locally by the function, and
thus should be freed before returning. In retrospect it may have been
better to do without the extra variable entirely. It's easy to infer
when "contents" should be freed, costing a little extra on an error
path but costing less elsewhere.
* elf.c (_bfd_elf_slurp_version_tables): Don't use free_contents.
This adds the efi target name handling for riscv64 to objcopy.
binutils:
* binutils/objcopy.c: add riscv64 handling to
convert_efi_target()
Signed-off-by: Peter Jones <pjones@redhat.com>
Reviewed-by: Palmer Dabbelt <palmer@rivosinc.com>
Reuse the bfd/development.sh script like most other project to
determine whether the current source tree is a dev build (e.g.
git) or a release build, and disable the warnings for releases.
This code tries to use attach_type enums as hw_phb_decode, and while
they're setup to have compatible values, the compiler doesn't like it
when the cast is missing. So cast it explicitly and then use that.
sim/ppc/hw_phb.c:322:28: error:
implicit conversion from enumeration type 'attach_type'
(aka 'enum _attach_type') to different enumeration type
'hw_phb_decode' [-Werror,-Wenum-conversion]
Don't conflate HAVE_GETRUSAGE & HAVE_SYS_RESOURCE_H. Use the latter
to include the header and nothing else. Use the former to determine
whether to use the function and nothing else. If we find a system
that doesn't follow POSIX and provides only one of these, we can
figure out how to support it then. The manual local definition is
clashing with the system ones and leading to build failures with
newer C standards.
sim/ppc/emul_netbsd.c:51:5: error: a function declaration without a
prototype is deprecated in all versions of C and is treated as a
zero-parameter prototype in C2x, conflicting with a previous
declaration [-Werror,-Wdeprecated-non-prototype]
aarch64-elf fails the ld-aarch64/bfd-far-3.d test, due to the stubs
being emitted in a different order to that of aarch64-linux. They are
emitted in a different order due to stub names for local symbols
having the section id in the stub name. aarch64-linux-ld generates
one more section than aarch64-elf-ld. That section is .gnu.hash. So
the stub names differ and are hashed to different slots in
stub_hash_table.
Fix this by running the test with --hash-style=sysv, and adjust
expected output. I've also changed the branch over stubs emitted at
the start of a group of stubs to not care about the symbol, for all
groups not just the one that needed changing.
* ld-aarch64/bti-far-3.d: Add --hash-style=sysv. Adjust
expected output.
In the commit:
commit 4793f551a5
Date: Mon Nov 27 13:33:17 2023 +0000
gdb: allow use of ~ in 'save gdb-index' command
I added a test which has a directory name within the GDB command,
which then appears in the test name as I failed to give the test a
better name.
Fixed in this commit.
'@' is a special symbol meaning 'command' in GNU texinfo.
If the GDBINIT or GDBINIT_DIR path during configuration
included an '@' character, the makeinfo command would fail,
as it interpreted the '@' in the path as a start of a command
when expanding the path in the docs.
This patch simply escapes any '@' characters in the path,
by replacing them with '@@'. This was already done for the
bugurl variable.
This was detected because the 'Jenkins' tool sometimes puts
an '@' in the workspace path.
Approved-By: Tom Tromey <tom@tromey.com>
While working on gdb's .debug_names writer, I found a couple of small
bugs in binutils .debug_names dumping.
First, the DWARF spec (section 6.1.1.4.6 Name Table) says:
These two arrays are indexed starting at 1, [...]
I think it is clearer for binutils to follow this, particularly
because DW_IDX_parent refers to this number.
Second, I think the handling of an empty hash table is slightly wrong.
Currently the dumping code assumes there is always an array of hashes.
However, section 6.1.1.4.5 Hash Lookup Table says:
The optional hash lookup table immediately follows the list of
type signatures.
and then:
The hash lookup table is actually two separate arrays: an array of
buckets, followed immediately by an array of hashes.
My reading of this is that the hash table as a whole is optional, and
so the hashes will not exist in this case. (This also makes sense
because the hashes are not useful without the buckets anyway.)
This patch fixes both of these problems. FWIW I have some gdb patches
in progress that change gdb both to omit the hash table and to use
DW_IDX_parent.
2023-12-04 Tom Tromey <tom@tromey.com>
* dwarf.c (display_debug_names): Handle empty .debug_names hash
table. Name entries start at 1.
Add support for jump visualization for the s390 architecture in
disassembly:
objdump -d --visualize-jumps ...
Annotate the (conditional) jump and branch relative instructions with
information required for jump visualization:
- jump: Unconditional jump / branch relative.
- condjump: Conditional jump / branch relative.
- jumpsr: Jump / branch relative to subroutine.
Unconditional jump and branch relative instructions are annotated as
jump.
Conditional jump and branch relative instructions, jump / branch
relative on count/index, and compare and jump / branch relative
instructions are annotated as condjump.
Jump and save (jas, jasl) and branch relative and save (bras, brasl)
instructions are annotated as jumpsr (jump to subroutine).
Provide instruction information required for jump visualization during
disassembly.
The instruction type is provided after determining the opcode.
For non-code it is set to dis_noninsn. Otherwise it defaults to
dis_nonbranch. No annotation is done for data reference instructions
(i.e. instruction types dis_dref and dis_dref2). Note that the
instruction type needs to be provided before printing of the
instruction, as it is used in print_address_func() to translate the
argument value into an address if it is assumed to be a PC-relative
offset. Note that this is never the case on s390, as
print_address_func() is only called with addresses and never with
offsets.
The target of the (conditional) jump and branch relative instructions
is provided during print, when the PC relative operand is decoded.
include/
* opcode/s390.h: Define opcode flags to annotate instruction
class information for jump visualization:
S390_INSTR_FLAG_CLASS_BRANCH, S390_INSTR_FLAG_CLASS_RELATIVE,
S390_INSTR_FLAG_CLASS_CONDITIONAL, and
S390_INSTR_FLAG_CLASS_SUBROUTINE.
Define opcode flags mask S390_INSTR_FLAG_CLASS_MASK for above
instruction class information.
Define helpers for common instruction class flag combinations:
S390_INSTR_FLAGS_CLASS_JUMP, S390_INSTR_FLAGS_CLASS_CONDJUMP,
and S390_INSTR_FLAGS_CLASS_JUMPSR.
opcodes/
* s390-mkopc.c: Add opcode flags to annotate information
for jump visualization: jump, condjump, and jumpsr.
* s390-opc.txt: Annotate (conditional) jump and branch relative
instructions with information for jump visualization.
* s390-dis.c (print_insn_s390, s390_print_insn_with_opcode):
Provide instruction information for jump visualization.
Signed-off-by: Jens Remus <jremus@linux.ibm.com>
Reviewed-by: Andreas Krebbel <krebbel@linux.ibm.com>
I noticed that gdbserver/win32-low.cc has a fall-through comment that
should have been converted to use the annotation instead.
This patch makes the change.
This commit enables the early initialization commands (92e4e97a9f) to
modify the number of threads used by gdb's thread pool.
The motivation here is to prevent gdb from spawning a detrimental number
of threads on many-core systems under environments with restrictive
ulimits.
With gdb before this commit, the thread pool takes the following sizes:
1. Thread pool size is initialized to 0.
2. After the maintenance commands are defined, the thread pool size is
set to the number of system cores (if it has not already been set).
3. Using early initialization commands, the thread pool size can be
changed using "maint set worker-threads".
4. After the first prompt, the thread pool size can be changed as in the
previous step.
Therefore after step 2. gdb has potentially launched hundreds of threads
on a many-core system.
After this change, step 2 and 3 are reversed so there is an opportunity
to set the required number of threads without needing to default to the
number of system cores first.
There does exist a configure option (added in 261b07488b) to disable
multithreading, but this does not allow for an already deployed gdb to
be configured.
Additionally, the default number of worker threads is clamped at eight
to control the number of worker threads spawned on many-core systems.
This value was chosen as testing recorded on bugzilla issue 29959
indicates that parallel efficiency drops past this point.
GDB built with GCC 13.
No test suite regressions detected. Compilers: GCC, ACfL, Intel, Intel
LLVM, NVHPC; Platforms: x86_64, aarch64.
The scenario that interests me the most involves preventing GDB from
spawning any worker threads at all. This was tested by counting the
number of clones observed by strace:
strace -e clone,clone3 gdb/gdb -q \
--early-init-eval-command="maint set worker-threads 0" \
-ex q ./gdb/gdb |& grep --count clone
The new test relies on "gdb: install CLI uiout while processing early
init files" developed by Andrew Burgess. This patch will need pushing
prior to this change.
The clamping was tested on machines with both 16 cores and a single
core. "maint show worker-threads" correctly reported eight and one
respectively.
Approved-By: Tom Tromey <tom@tromey.com>
The next commit wants to use a 'show' command within an early
initialisation file, despite these commands not being in the list of
acceptable commands for use within an early initialisation file.
The problem we run into is that the early initialisation files are
processed before GDB has installed the top level interpreter. The
interpreter is responsible to installing the default uiout (accessed
through current_uiout), and as a result code that depends on
uiout (e.g. 'show' commands) will end up dereferencing a nullptr, and
crashing GDB.
I did consider moving the interpreter installation before the early
initialisation, and this would work fine except for the new DAP
interpreter, which relies on having Python available during its
initialisation. Which means we can't install the interpreter until
after Python has been initialised, and the early initialisation
handling has to occur before Python is setup -- that's the whole point
of this feature (to allow customisation of how Python is setup).
So, what I propose is that early within captured_main_1, we install a
temporary cli_ui_out as the current_uiout. This will remain in place
until the top-level interpreter is installed, at which point the
temporary will be replaced.
What this means is that current_uiout will no longer be nullptr,
instead, any commands within an early initialisation file that trigger
output, will perform that output in a CLI style.
I propose that we don't update the documentation for early
initialisation files, we leave the user advice as being only 'set' and
'source' commands are acceptable. But now, if a user does try a
'show' command, then instead of crashing, GDB will do something
predictable.
I've not added a test in this commit. The next commit relies on this
patch and will serve as a test.
Tested-By: Richard Bunt <richard.bunt@linaro.org>
I noticed that after resizing to a narrow window, I got:
...
┌────────────────┐
│ │
│[ No Source Avail
able ] │
│ │
└────────────────┘
...
Fix this by adding two new functions:
- tui_win_info::display_string (int y, int x, const char *str)
- tui_win_info::display_string (const char *str)
that make sure that borders are not overwritten, which get us instead:
...
┌────────────────┐
│ │
│[ No Source Avai│
│ │
│ │
└────────────────┘
...
Tested on x86_64-linux.
When using GDB on native linux, it can happen that, while attempting
to detach an inferior, the inferior may have been exited or have been
killed, yet still be in the list of lwps. Should that happen, the
assert in x86_linux_update_debug_registers in
gdb/nat/x86-linux-dregs.c will trigger. The line in question looks
like this:
gdb_assert (lwp_is_stopped (lwp));
For this case, the lwp isn't stopped - it's dead.
The bug which brought this problem to my attention is one in which the
pwntools library uses GDB to to debug a process; as the script is
shutting things down, it kills the process that GDB is debugging and
also sends GDB a SIGTERM signal, which causes GDB to detach all
inferiors prior to exiting. Here's a link to the bug:
https://bugzilla.redhat.com/show_bug.cgi?id=2192169
The following shell command mimics part of what the pwntools
reproducer script does (with regard to shutting things down), but
reproduces the bug much less reliably. I have found it necessary to
run the command a bunch of times before seeing the bug. (I usually
see it within 5-10 repetitions.) If you choose to try this command,
make sure that you have no running "cat" or "gdb" processes first!
cat </dev/zero >/dev/null & \
(sleep 5; (kill -KILL `pgrep cat` & kill -TERM `pgrep gdb`)) & \
sleep 1 ; \
gdb -q -iex 'set debuginfod enabled off' -ex 'set height 0' \
-ex c /usr/bin/cat `pgrep cat`
So, basically, the idea here is to kill both gdb and cat at roughly
the same time. If we happen to attempt the detach before the process
lwp has been deleted from GDB's (linux native) LWP data structures,
then the assert will trigger. The relevant part of the backtrace
looks like this:
#8 0x00000000008a83ae in x86_linux_update_debug_registers (lwp=0x1873280)
at gdb/nat/x86-linux-dregs.c:146
#9 0x00000000008a862f in x86_linux_prepare_to_resume (lwp=0x1873280)
at gdb/nat/x86-linux.c:81
#10 0x000000000048ea42 in x86_linux_nat_target::low_prepare_to_resume (
this=0x121eee0 <the_amd64_linux_nat_target>, lwp=0x1873280)
at gdb/x86-linux-nat.h:70
#11 0x000000000081a452 in detach_one_lwp (lp=0x1873280, signo_p=0x7fff8ca3441c)
at gdb/linux-nat.c:1374
#12 0x000000000081a85f in linux_nat_target::detach (
this=0x121eee0 <the_amd64_linux_nat_target>, inf=0x16e8f70, from_tty=0)
at gdb/linux-nat.c:1450
#13 0x000000000083a23b in thread_db_target::detach (
this=0x1206ae0 <the_thread_db_target>, inf=0x16e8f70, from_tty=0)
at gdb/linux-thread-db.c:1385
#14 0x0000000000a66722 in target_detach (inf=0x16e8f70, from_tty=0)
at gdb/target.c:2526
#15 0x0000000000a8f0ad in kill_or_detach (inf=0x16e8f70, from_tty=0)
at gdb/top.c:1659
#16 0x0000000000a8f4fa in quit_force (exit_arg=0x0, from_tty=0)
at gdb/top.c:1762
#17 0x000000000070829c in async_sigterm_handler (arg=0x0)
at gdb/event-top.c:1141
My colleague, Andrew Burgess, has done some recent work on other
problems with detach. Upon hearing of this problem, he came up a test
case which reliably reproduces the problem and tests for a few other
problems as well. In addition to testing detach when the inferior has
terminated due to a signal, it also tests detach when the inferior has
exited normally. Andrew observed that the linux-native-only
"checkpoint" command would be affected too, so the test also tests
those cases when there's an active checkpoint.
For the LWP exit / termination case with no checkpoint, that's handled
via newly added checks of the waitstatus in detach_one_lwp in
linux-nat.c.
For the checkpoint detach problem, I chose to pass the lwp_info
to linux_fork_detach in linux-fork.c. With that in place, suitable
tests were added before attempting a PTRACE_DETACH operation.
I added a few asserts at the beginning of linux_fork_detach and
modified the caller code so that the newly added asserts shouldn't
trigger. (That's what the 'pid == inferior_ptid.pid' check is about
in gdb/linux-nat.c.)
Lastly, I'll note that the checkpoint code needs some work with regard
to background execution. This patch doesn't attempt to fix that
problem, but it doesn't make it any worse. It does slightly improve
the situation with detach because, due to the check noted above,
linux_fork_detach() won't be called for the wrong inferior when there
are multiple inferiors. (There are at least two other problems with
the checkpoint code when there are multiple inferiors. See:
https://sourceware.org/bugzilla/show_bug.cgi?id=31065)
This commit also adds a new test,
gdb.base/process-dies-while-detaching.exp. Andrew Burgess is the
primary author of this test case. Its design is similar to that of
gdb.threads/main-thread-exit-during-detach.exp, which was also written
by Andrew.
This test checks that GDB correctly handles several cases that can
occur when GDB attempts to detach an inferior process. The process
can exit or be terminated (e.g. via SIGKILL) prior to GDB's event
loop getting a chance to remove it from GDB's internal data
structures. To complicate things even more, detach works differently
when a checkpoint (created via GDB's "checkpoint" command) exists for
the inferior. This test checks all four possibilities: process exit
with no checkpoint, process termination with no checkpoint, process
exit with a checkpoint, and process termination with a checkpoint.
Co-Authored-By: Andrew Burgess <aburgess@redhat.com>
Approved-By: Andrew Burgess <aburgess@redhat.com>
commit eab996435f
Author: Jan Beulich <jbeulich@suse.com>
Date: Tue Nov 7 13:58:32 2023 +0100
ld/x86: reduce testsuite dependency on system object files
changed some C compiler tests to assembler/linker tests which introduced
2 problems:
1. It broke x32 binutils tests since --64 was passed to assembler, but
-m elf_x86_64 wasn't passed to linker.
2. -nostdlib was passed to C compiler driver to exclude standard run-time
files which should be avoided with -r option for linker tests.
Fix them by passing -m elf_x86_64 to linker and removing -nostdlib for
linker tests with -r.
PR ld/30722
* testsuite/ld-x86-64/x86-64.exp: Pass -m elf_x86_64 to linker
for tests with --64. Remove -nostdlib for tests with -r.
On Linux, threads are treated much like separate processes by the
kernel. In particular, it's possible to ptrace just a single thread.
If gdb tries to attach to a multi-threaded inferior, where a non-main
thread is already being traced (e.g., by strace), then gdb will get
into an infinite loop attempting to attach.
This patch fixes this problem by having the attach fail if ptrace
fails to attach to any thread of the inferior.
This changes linux_proc_attach_tgid_threads to use gdb_dir_up. This
makes it robust against exceptions.
Approved-By: Simon Marchi <simon.marchi@efficios.com>
linux_proc_attach_tgid_threads computes a file name, and then
re-computes it for a warning. It is better to reuse the
already-computed name here.
Approved-By: Simon Marchi <simon.marchi@efficios.com>
Fix two spots in aarch64-linux-tdep.c that build regcache_map_entry
arrays without a null terminator. The null terminators are needed for
regcache::transfer_regset and regcache_map_entry_size to work properly.
Change-Id: I3224a314e1360b319438f32de8c81e70ab42e105
Reviewed-By: John Baldwin <jhb@FreeBSD.org>
Approved-By: Luis Machado <luis.machado@arm.com>
regcache::transfer_regset iterates over an array of regcache_map_entry,
transferring the registers (between regcache and buffer) described by
those entries. It stops either when it reaches the end of the
regcache_map_entry array (marked by a null entry) or (it seems like the
intent is) when it reaches the end of the buffer (in which case not all
described registers are transferred).
I said "seems like the intent is", because there appears to be a small
bug. transfer_regset is made of two loops:
foreach regcache_map_entry:
foreach register described by the regcache_map_entry:
if the register doesn't fit in the remainder of the buffer:
break
transfer register
When stopping because we have reached the end of the buffer, the break
only breaks out of the inner loop.
This problem causes some failures when I run tests such as
gdb.arch/aarch64-sme-core-3.exp (on AArch64 Linux, in qemu). This is
partly due to aarch64_linux_iterate_over_regset_sections failing to add
a null terminator in its regcache_map_entry array, but I think there is
still a problem in transfer_regset.
The sequence to the crash is:
- The `regcache_map_entry za_regmap` object built in
aarch64_linux_iterate_over_regset_sections does not have a null
terminator.
- When the target does not have a ZA register,
aarch64_linux_collect_za_regset calls `regcache->collect_regset` with
a size of 0 (it's actually pointless, but still it should work).
- transfer_regset gets called with a buffer size of 0.
- transfer_regset detects that the register to transfer wouldn't fit in
0 bytes, so it breaks out of the inner loop.
- The outer loop tries to go read the next regcache_map_entry, but
there isn't one, and we start reading garbage.
Obviously, this would get fixed by making
aarch64_linux_iterate_over_regset_sections use a null terminator (which
is what the following patch does). But I think that when detecting that
there is not enough buffer left for the current register,
transfer_regset should return, not only break out of the inner loop.
This is a kind of contrived scenario, but imagine we have these two
regcache_map_entry objects:
- 2 registers of 8 bytes
- 2 registers of 4 bytes
For some reason, the caller passes a buffer of 12 bytes.
transfer_regset will detect that the second 8 byte register does not
fit, and break out of the inner loop. However, it will then go try the
next regcache_map_entry. It will see that it can fit one 4 byte
register in the remaining buffer space, and transfer it from/to there.
This is very likely not an expected behavior, we wouldn't expect to
read/write this sequence of registers from/to the buffer.
In this example, whether passing a 12 bytes buffer makes sense or
whether it is a size computation bug in the caller, we don't know, but I
think that exiting as soon as a register doesn't fit is the sane thing
to do.
Change-Id: Ia349627d2e5d281822ade92a8e7a4dea4f839e07
Reviewed-By: John Baldwin <jhb@FreeBSD.org>
Reviewed-By: Luis Machado <luis.machado@arm.com>
Andrew Burgess
Tom de Vries via Gdb-patches
Mike Frysinger
Alan Modra
GDB Administrator
Jakub Jelinek
Hannes Domani
Richard Bunt
Tom Tromey
Nick Clifton
Tom Tromey
Andreas Schwab
Peter Jones
Ciaran Woodward
Jens Remus
Tom de Vries
Kevin Buettner
Petr Tesarik
H.J. Lu
Simon Marchi