| Age | Commit message (Collapse) | Author | Files | Lines |
|---|
|
This patch sets GUILE to just plain 'guile'.
In the distant ("devo") past, the top-level build did support building
Guile in-tree. However, I don't think this really works any more.
For one thing, there are no build dependencies on it, so there's no
guarantee it would actually be built before the uses.
This patch also removes the use of "-s" as an option to cgen scheme
scripts. With my latest patch upstream, this is no longer needed.
After the upstream changes, either Guile 2 or Guile 3 will work, with
or without the compiler enabled.
2023-08-24 Tom Tromey <tom@tromey.com>
* cgen.sh: Don't pass "-s" to cgen.
* Makefile.in: Rebuild.
* Makefile.am (GUILE): Simplify.
|
|
The author of 'mold' pointed out that with a certain shared library,
gdb would fail to find the shared library's name in 'bt'.
The function in question appeared at the end of the .so's .text
segment and ended with a call to 'abort'.
This turned out to be a classic case of calling get_frame_pc when
get_frame_address_in_block is needed -- the former will be off-by-one
for purposes of finding the enclosing function or shared library.
The included test fails without the patch on my system. However, I
imagine it can't be assumed to reliably fail. Nevertheless it seemed
worth doing.
Regression tested on x86-64 Fedora 38.
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=29074
Reviewed-by: Kevin Buettner <kevinb@redhat.com>
|
|
i386: warning: format ‘%u’ expects argument of type ‘unsigned int’,
but argument 4 has type ‘size_t’ {aka ‘long unsigned int’} [-Wformat=]
ia64: warning: ignoring return value of ‘fgets’
* i386-gen.c (process_i386_opcodes): Correct format string.
* ia64-gen.c (load_insn_classes, load_depfile): Don't ignore
fgets return value.
|
|
Delete support for old compilers that don't support string
concatentation.
* Makefile.am (stringify.sed): Delete rule.
(GEN_DEPENDS, DISTCLEANFILES): Adjust.
* configure.ac (STRINGIFY): Delete.
* emultempl/beos.em: Use stringify.sed from srcdir.
* emultempl/elf.em: Likewise.
* emultempl/generic.em: Likewise.
* emultempl/msp430.em: Likewise.
* emultempl/pdp11.em: Likewise.
* emultempl/pe.em: Likewise.
* emultempl/pep.em: Likewise.
* emultempl/stringify.sed: Renamed from..
* emultempl/astring.sed: ..this.
* emultempl/ostring.sed: Delete.
* Makefile.in: Regenerate.
* configure: Regenerate.
|
|
|
|
This patch gets rid of the individual rules including the .Pc
dependency files made while generating e*.c files, replacing them with
a fancy GNU make pattern include. I've also moved creation of
ldscripts to the makefile since it is possible to run more than one
genscripts at once, resulting in "ldscripts: File exists" messages.
* Makefile.am: Replace individual include of *.Pc dependency
files with one pattern rule.
(.Pc): New dummy rule.
(ldscripts/stamp): New rule.
(GEN_DEPENDS): Add ldscripts/stamp.
(install-data-local): Exclude ldscripts/stamp from install.
* genscripts.sh: Don't make ldscripts dir.
* Makefile.in: Regenerate.
|
|
The getsymname function tries to emit an error using %ld for an
uintptr_t argument. Use PRIxPTR instead. Which works on any architecture
for uintptr_t.
|
|
This patch addresses an issue with malformed/fuzzed debug information that
was recently reported in gdb/30639. That bug specifically deals with
an ASAN issue, but the reproducer provided by the reporter causes a
another failure outside of ASAN:
$ ./gdb --data-directory data-directory -nx -q UAF_2
Reading symbols from /home/keiths/UAF_2...
Fatal signal: Segmentation fault
----- Backtrace -----
0x59a53a gdb_internal_backtrace_1
../../src/gdb/bt-utils.c:122
0x59a5dd _Z22gdb_internal_backtracev
../../src/gdb/bt-utils.c:168
0x786380 handle_fatal_signal
../../src/gdb/event-top.c:889
0x7864ec handle_sigsegv
../../src/gdb/event-top.c:962
0x7ff354c5fb6f ???
0x611f9a process_coff_symbol
../../src/gdb/coffread.c:1556
0x611025 coff_symtab_read
../../src/gdb/coffread.c:1172
0x60f8ff coff_read_minsyms
../../src/gdb/coffread.c:549
0x60fe4b coff_symfile_read
../../src/gdb/coffread.c:698
0xbde0f6 read_symbols
../../src/gdb/symfile.c:772
0xbde7a3 syms_from_objfile_1
../../src/gdb/symfile.c:966
0xbde867 syms_from_objfile
../../src/gdb/symfile.c:983
0xbded42 symbol_file_add_with_addrs
../../src/gdb/symfile.c:1086
0xbdf083 _Z24symbol_file_add_from_bfdRKN3gdb7ref_ptrI3bfd18gdb_bfd_ref_policyEEPKc10enum_flagsI16symfile_add_flagEPSt6vectorI14other_sectionsSaISC_EES8_I12objfile_flagEP7objfile
../../src/gdb/symfile.c:1166
0xbdf0d2 _Z15symbol_file_addPKc10enum_flagsI16symfile_add_flagEPSt6vectorI14other_sectionsSaIS5_EES1_I12objfile_flagE
../../src/gdb/symfile.c:1179
0xbdf197 symbol_file_add_main_1
../../src/gdb/symfile.c:1203
0xbdf13e _Z20symbol_file_add_mainPKc10enum_flagsI16symfile_add_flagE
../../src/gdb/symfile.c:1194
0x90f97f symbol_file_add_main_adapter
../../src/gdb/main.c:549
0x90f895 catch_command_errors
../../src/gdb/main.c:518
0x9109b6 captured_main_1
../../src/gdb/main.c:1203
0x910fc8 captured_main
../../src/gdb/main.c:1310
0x911067 _Z8gdb_mainP18captured_main_args
../../src/gdb/main.c:1339
0x418c71 main
../../src/gdb/gdb.c:39
---------------------
A fatal error internal to GDB has been detected, further
debugging is not possible. GDB will now terminate.
This is a bug, please report it. For instructions, see:
<https://www.gnu.org/software/gdb/bugs/>.
Segmentation fault (core dumped)
The issue here is that the COFF offset for the fuzzed symbol's
name is outside the string table. That is, the offset is greater
than the actual string table size.
coffread.c:getsymname actually contains a FIXME about this, and that's
what I've chosen to address to fix this issue, following what is done
in the DWARF reader:
$ ./gdb --data-directory data-directory -nx -q UAF_2
Reading symbols from /home/keiths/UAF_2...
COFF Error: string table offset (256) outside string table (length 0)
(gdb)
Unfortunately, I haven't any idea how else to test this patch since
COFF is not very common anymore. GCC removed support for it five
years ago with GCC 8.
|
|
This matches the current set of system calls at the start of the
stable/14 branch (commit 29a16ce065dbc28bc9e87c9bfadb08bb58b137e4).
|
|
In AIX the first eight function parameters are stored from R3 to R10.
If there are more than eight parameters in a function then we store the 9th parameter onwards in the stack.
While doing so, in 64 bit mode the words were not zero extended and was coming like 32 bit mode.
This patch is a fix to the same.
|
|
|
|
While documented to not be reliable, it is still odd for objcopy to
silently produce bad output when converting COFF/PE object files to ELF
ones. The issue there is that relocation addends all are screwed up by
subtracting the symbol's section offset. In the COFF/PE world, to my
knowledge, section contents stores the addends alone, not the result of
symbol value plus addend. Hence the compensation talked about in a
comment ahead of the sole use site of CALC_ADDEND() may need to account
for the VMA (which is always zero for object files anyway), but not for
the symbol value.
The coff-sh.c adjustment is based upon guessing that behavior there is
the same. Note also how coff-aarch64.c short-circuits CALC_ADDEND()
altogether, which may suggest that a much simpler macro might do for the
COFF_WITH_PE case in the three arch-specific files touched here.
For (at least) Arm/WinCE this actually results in more appropriate
objdump output as well, as can be seen in the one testcase which has its
expectations adjusted (the generated binary doesn't change).
|
|
Rather than special-casing rx-*-* for section30, force use of
conventional section names uniformly. By further passing $dump_opts to
a few more tests, a number of xfail-s (and one notarget) can be
eliminated (some of which had wrong justifications in associated
comments anyway). Note that section7 and section15 need to be left
alone: The harness fiddling with section names there didn't help before
and is getting in the way now. For section12b, section16b, and most of
the Dwarf tests nothing changes. Interestingly by passing $dump_opts
the need to xfail section11 for LoongArch and RISC-V also goes away.
|
|
While --sectname-subst is nice, it isn't enough to e.g. mimic
-f{function,data}-sections in assembly code, when such use is to be
optional (e.g. dependent upon some configuration setting).
Assign meaning to '+' and '-' as section attribute letters, allowing
to inherit the prior section's attributes (and possibly type) along
with adding or removing some. Note that documenting the interaction
with '?' as undefined is a precautionary measure.
While touching the function invocation, stop using |= on the result of
obj_elf_parse_section_letters(): "attr" is firmly zero ahead of the
call.
|
|
Use the pattern rule in a comment from commit 77ac17b8453f.
* Makefile.am (run-genscripts): Delete. Use pattern rule
e%.c instead.
* Makefile.in: Regenerate.
|
|
Code in som_write_symbol_strings neglected to allow for padding, which
can result in a buffer overflow. It also used xrealloc, which we're
not supposed to use in libbfd because libbfd isn't supposed to call
exit. Also a realloc is perhaps not a good idea when none of the
buffer contents are needed, so replace with free, bfd_malloc. There
were three copies of the string handling code, so rather than fix them
all I've extracted them to a function. This necessitated making one
of the fields in struct som_symbol unsigned.
* som.c (add_string): New function.
(som_write_space_strings, som_write_symbol_strings): Use it.
* som.h (som_symbol_type <stringtab_offset>): Make unsigned.
|
|
Caused by commit 5a97377e5513, specifically this code added to
Target_powerpc::do_relax
+ if (parameters->options().output_is_position_independent())
+ this->rela_dyn_size_
+ = this->rela_dyn_section(layout)->current_data_size();
The problem here is that if .rela.dyn isn't already created then the
call to rela_dyn_section creates it, and as this comment in
Target_powerpc::do_finalize_sections says:
// Annoyingly, we need to make these sections now whether or
// not we need them. If we delay until do_relax then we
// need to mess with the relaxation machinery checkpointing.
We can't be creating sections in do_relax.
PR 30794
* powerpc.cc (Target_powerpc::do_relax): Only set rela_dyn_size_
for size == 64, and assert that rela_dyn_ already exists.
Tidy code setting plt_thread_safe, which also only needs to be
set when size == 64 for ELFv1.
|
|
|
|
It is possible to debug a process which uses unsupported AMDGPU devices.
In such scenario, we can still use librocm-dbgapi.so to attach to the
process and complete the runtime activation sequence.
However, when listing shared objects loaded on the AMDGPU devices, we
might list SOs loaded on the unsupported devices. If such SO is
seen, one of two things can happen.
First, if the arch of this device is unknown to BFD,
'gdbarch_find_by_info (gdbarch_info info)' will return the gdbarch
matching default_bfd_arch. As a result,
rocm_solib_relocate_section_addresses will delegate the relocation
operation to svr4_so_ops.relocate_section_addresses, but this makes no
sense: this code object was not loaded by the system loader.
The second case is if BFD knows the micro-architecture of the device,
but dbgapi does not support it. In such case, gdbarch_info_fill will
successfully identify an amdgcn architecture (bfd_arch_amdgcn). From
there, gdbarch_find_by_info calls amdgpu_gdbarch_init which will fail to
query arch specific details from dbgapi and subsequently fail to
initialize the gdbarch object. As a result, gdbarch_find_by_info
returns nullptr, which will down the line cause some "gdb_assert
(gdbarch != nullptr)" assertion failures.
This patch proposes to add a check in rocm_solib_bfd_open to ensure that
the architecture associated with the code object to open is fully
supported by both BFD and amd-dbgapi, and error-out otherwise.
Change-Id: Ica97ab7cba45e4944b77d3080c54c1038aaeda54
Approved-By: Pedro Alves <pedro@palves.net>
|
|
In remote_target::thread_info_to_thread_handle we return a copy:
...
gdb::byte_vector
remote_target::thread_info_to_thread_handle (struct thread_info *tp)
{
remote_thread_info *priv = get_remote_thread_info (tp);
return priv->thread_handle;
}
...
Fix this by returning a gdb::array_view instead:
...
gdb::array_view<const gdb_byte>
remote_target::thread_info_to_thread_handle (struct thread_info *tp)
...
Tested on x86_64-linux.
This fixes the build when building with -std=c++20.
Approved-By: Pedro Alves <pedro@palves.net>
|
|
opcodes/
* kvx-dis.c (print_insn_kvx): Change the loop condition so that
wordcount is always less than KVXMAXBUNDLEWORDS.
(decode_prologue_epilogue_bundle): Likewise.
|
|
This commits tackles 2 problems in the test
gdb.reverse/insn-reverse.exp. They are, broadly: flawed logic when an
unexpected error occurs, and badly formed asm expressions.
For the first, what happens is that if the inferior stops progressing
for some reason, the test will emit an UNSUPPORTED and continue testing
by reversing from the current location and checking all registers for
every instruction. However, due to how the outputs are indexed in the
test, this early exit will cause most of the subsequent tests to be
de-synced and will emit many unrelated failures.
This commit changes the UNSUPPORTED for a FAIL, since the test has in
fact failed to record the execution of the whole function, and
decrements the recorded instruction count by one so that the indexes are
in sync once more.
At the time of committing, this reduces the amount of failures when
testing with clang-15 from around 150 to 2, and correctly identifies
where the issue lies.
The second problem is in how the asm statements in the *-x86.c file
are written. As an example, let's examine the following line:
__asm__ volatile ("rdrand %%ebp;" : "=r" (number));
This statement says that number is being used as the output variable,
but is not indicating which registers were clobbered so that the
compiler is able to properly output. GCC decides to just not save
anything, whereas clang assumes that the output is in %rax, and writes
it to the variable. This hid the problem that any compiler is not good
at dealing with asm statements that change the rbp register. It can be
seen more explicitly by informing gcc that rbp has been clobbered like
so:
__asm__ volatile ("rdrand %%ebp;" : "=r" (number) : : "%ebp");
This statement gets compiled into the following assembly:
rdrandl %ebp
movl %eax, -4(%rbp)
Which is clearly using the incorrect rbp to find the memory location of
the variable. Since the test only exercises GDB's ability to record the
register changes, this commit removes the output to memory.
Finally, correctly informing the compiler of clobbered registers
makes gcc throw an error that the rsp is no longer usable at the end of
the function. To avoid that, this commit compresses the 3 asm statements
that would save, change and reset registers into a single asm statement.
Approved-By: Tom Tromey <tom@tromey.com>
|
|
When testing using reverse-stepi to fully step through a function, the
code checks for an infinite loop by seeing if we land on the line that
contains the return statement multiple times. This assumption only works
if there is only one instruction associated with that line, which is how
GCC handles line information, but other compilers may handle it differently.
Clang-15, for instance, associates 6. Because of this, the inferior used
to get seriously out of sync with the test expectations, and result in 13
spurious failures. The same issue occurs with gdb.reverse/step-precsave.exp.
This commit changes the test so that we check for PC instead of line
number. The test still only happens when the same line is detected, to
simplify the resulting log. With this change, no new failures are
emitted when using clang.
Approved-By: Tom Tromey <tom@tromey.com>
|
|
The tests gdb.reverse/solib-precsave.exp and solib-reverse.exp have the
assumption that line tables will have an entry for the closing } in a
function. Not all compiles do this, one example being clang. To fix
this, this commit changes the function in shr2.c to have multiple lines,
and the test to accept either line as a correct step location.
To properly re-sync the inferiors, the function repeat_cmd_until had to
be slightly changed to work with empty "current locations", so that we
are able to step through multiple lines.
This also changes the annotations used to determine the breakpoint
locations in solib-reverse.c, adding a simple variable assignment right
before the return statement. This way GDB will not set a breakpoint in
the closing } line.
Approved-By: Tom Tromey <tom@tromey.com>
|
|
Clang does not add line information for lines that only contain a
closing } in functions. Many tests in the gdb.reverse folder set a
breakpoint in that line, but don't seem to use information available
after the return statement is executed, so this commit moves the
breakpoint to the previous line, where the return statement is.
Approved-By: Tom Tromey <tom@tromey.com>
|
|
kvx-dis.c:1078:10: error: missing initializer
kvx-dis.c:1078:10: error: (near initialization for 'dec.nb_ops')
* kvx-dis.c (print_insn_kvx): Init dec with memset.
(decode_prologue_epilogue_bundle): Likewise.
|
|
Signed-off-by: Oleg Tolmatcev <oleg.tolmatcev@gmail.com>
|
|
As commit fa4f2d46f9 did for x86, there a few other targets that
wrongly use the output section rather than the dynamic section for
DT_JMPREL and others.
* elfnn-kvx.c (elfNN_kvx_finish_dynamic_sections): Use input
section for DT_JMPREL.
* elf32-sh.c (sh_elf_finish_dynamic_sections): Use input
section for DT_JMPREL and DT_PLTRELSZ.
* elf32-nds32.c (nds32_elf_finish_dynamic_sections): Likewise,
and for DT_PLTGOT and when adjusting DT_RELA.
|
|
This fixes test failures caused by the new linker warning which report:
./ld/ld-new: warning: load.S.x has a LOAD segment with RWX permissions
Fix this by splitting the linker MEMORY into ram and rom to avoid
generating RWX sections. This required tests to be adjusted to fix
issues with the move. Namely:
- fpu tests: were incorrectly using l.ori with ha(anchor) which now
that we pushed the anchor up in memory it exposes the bug. Update
to used the correct l.movhi instruction instead.
- adrp test: the test reports ram offset addresses, now that we have
moved memory layout around a bit I adjusted the test output. Some
padding is added before pi to show that the actual address of pi and
the adrp page offset are not the same.
Bug: https://sourceware.org/PR29957
|
|
bfd/
* config.bfd: Remove kvx_elf64_vec from targ_selvecs as it is
already in targ_defvec.
ld/
* configure.tgt: Split long line.
|
|
gas/
* config/kvx-parse.c (promote_token): Use {u,}int32_t and
{u,}int64_t.
(get_token_class): Likewise.
* config/tc-kvx.c (insert_operand): Likewise.
* config/tc-kvx.h (struct token_s): Likewise.
(struct token_list): Likewise.
opcodes/
* kvx-dis.c (struct decoded_insn): Use {u,}int32_t and
{u,}int64_t.
(decode_insn): Likewise.
(print_insn_kvx): Likewise.
(decode_prologue_epilogue_bundle): Likewise.
* kvx-dis.h (struct kvx_prologue_epilogue_insn): Likewise.
|
|
When processing a STB_GNU_UNIQUE symbol we did not update has_gnu_osabi
correctly.
* config/tc-kvx.c (kvx_end): Do not write to e_ident.
(kvx_type): Properly handle STB_GNU_UNIQUE symbols.
|
|
* configure.ac: Remove kvx_elf64_linux_vec.
* configure: Regenerate.
|
|
|
|
Run black on make-target-delegates.py to fix buildbot build breaker.
Tested on x86_64-linux.
|
|
When doing this in target.h:
...
- virtual gdb::byte_vector thread_info_to_thread_handle (struct thread_info *)
+ virtual gdb::byte_vector &thread_info_to_thread_handle (struct thread_info *)
...
make-target-delegates.py drops the function.
By handling '&' in POINTER_PART we can prevent that the function is dropped,
but when recompiling target.o we get:
...
gdb/target-delegates.c: In member function ‘virtual gdb::byte_vector& \
debug_target::thread_info_to_thread_handle(thread_info*)’:
gdb/target-delegates.c:1889:22: error: ‘result’ declared as reference but not \
initialized
gdb::byte_vector & result;
^~~~~~
make: *** [Makefile:1923: target.o] Error 1
...
Fix this by making sure result is initialized.
Regenerate target-delegates.c using this new style.
Tested on x86_64-linux.
Approved-By: Pedro Alves <pedro@palves.net>
|
|
If a linker script does not place the PLT relocations and "normal"
relocations in separate ELF sections, `ld` will currently output incorrect
values for DT_JMPREL and DT_PLTRELSZ - they cover the entire ELF section,
rather than just the PLT relocations
Don't ignore the extent of the BFD section - use the size of the srelplt
BFD section and its offset from the output_secttion
bfd/
PR ld/30787
* elfxx-x86.c (_bfd_x86_elf_finish_dynamic_sections): Use input
section for DT_JMPREL and DT_PLTRELSZ.
ld/
PR ld/30787
* testsuite/ld-i386/i386.exp: Run pr30787.
* testsuite/ld-x86-64/x86-64.exp: Likewise.
* testsuite/ld-i386/pr30787.d: New file.
* testsuite/ld-i386/pr30787.s: Likewise.
* testsuite/ld-i386/pr30787.t: Likewise.
* testsuite/ld-x86-64/pr30787.d: Likewise.
* testsuite/ld-x86-64/pr30787.s: Likewise.
* testsuite/ld-x86-64/pr30787.t: Likewise.
|
|
Since b080fe54fb3 "gdb: add inferior-specific breakpoints", the
breakpoint class has an "inferior" member used to handle
inferior-specific breakpoints. This creates a compilation error
in amd_dbgapi_target_breakpoint::check_status which declares a local
variable "inferior *inf".
Fix this by using "struct inferior *inf" instead.
Change-Id: Icc4dc1ba96c7d3ff9d33f9cb384ffcf64eba26fb
Approved-By: Pedro Alves <pedro@palves.net>
|
|
After running a number of programs under Windows gdb and detaching
them, I typed run in gdb, and got a hang, here:
(top-gdb) bt
#0 sharing_input_terminal (pid=4672) at /home/pedro/gdb/src/gdb/mingw-hdep.c:388
#1 0x00007ff71a2d8678 in sharing_input_terminal (inf=0x23bf23dafb0) at /home/pedro/gdb/src/gdb/inflow.c:269
#2 0x00007ff71a2d887b in child_terminal_save_inferior (self=0x23bf23de060) at /home/pedro/gdb/src/gdb/inflow.c:423
#3 0x00007ff71a2c80c0 in inf_child_target::terminal_save_inferior (this=0x23bf23de060) at /home/pedro/gdb/src/gdb/inf-child.c:111
#4 0x00007ff71a429c0f in target_terminal_is_ours_kind (desired_state=target_terminal_state::is_ours_for_output) at /home/pedro/gdb/src/gdb/target.c:1037
#5 0x00007ff71a429e02 in target_terminal::ours_for_output () at /home/pedro/gdb/src/gdb/target.c:1094
#6 0x00007ff71a2ccc8e in post_create_inferior (from_tty=0) at /home/pedro/gdb/src/gdb/infcmd.c:245
#7 0x00007ff71a2cd431 in run_command_1 (args=0x0, from_tty=0, run_how=RUN_NORMAL) at /home/pedro/gdb/src/gdb/infcmd.c:502
#8 0x00007ff71a2cd58b in run_command (args=0x0, from_tty=0) at /home/pedro/gdb/src/gdb/infcmd.c:527
The problem is that the loop around GetConsoleProcessList looped
forever, because there were exactly 10 processes to return.
GetConsoleProcessList's documentation says:
If the buffer is too small to hold all the valid process identifiers,
the return value is the required number of array elements. The
function will have stored no identifiers in the buffer. In this
situation, use the return value to allocate a buffer that is large
enough to store the entire list and call the function again.
In this case, the buffer wasn't too small, it was exactly the right
size, so we should have broken out of the loop. We didn't due to a
"<" check that should have been "<=". That is fixed by this patch.
Approved-By: Tom Tromey <tom@tromey.com>
Reviewed-By: Eli Zaretskii <eliz@gnu.org>
Change-Id: I14e4909f2ac2fa83d0d9b6e64418b5831ac4e4e3
|
|
Spotted a few places where a char is being treated as a bool. The GDB
style is to use explicit comparisons, so fix things up.
There should be no user visible changes after this commit.
|
|
|
|
PR 30791
* dwarf.h (debug_info): Add range_versions field.
* dwarf.c (read_and_display_attr_value): When recording a range arribute also ecord the dwarf version number.
(is_range_list_for_this_section): New function.
(display_debug_ranges): Only show debug ranges whose version is suitable for the secction being displayed.
|
|
This recent commit:
commit b1c0ab20809a502b2d2224fecb0dca3ada2e9b22
Date: Wed Jul 12 21:56:50 2023 +0100
gdb: avoid double stop after failed breakpoint condition check
Addressed a problem where two MI stopped events would be reported if a
breakpoint condition failed due to a signal, this commit was a
replacement for this commit:
commit 2e411b8c68eb2b035b31d5b00d940d4be1a0928b
Date: Fri Oct 14 14:53:15 2022 +0100
gdb: don't always print breakpoint location after failed condition check
which solved the two stop problem, but only for the CLI. Before both
of these commits, if a b/p condition failed due to a signal then the
user would see two stops reported, the first at the location where the
signal occurred, and the second at the location of the breakpoint.
By default GDB remains at the location where the signal occurred, so
the second reported stop can be confusing, this is the problem that
commit 2e411b8c68eb tried to solve (for the CLI) and b1c0ab20809a
extended also address the issue for MI too.
However, while working on another patch I realised that there was a
problem with GDB after the above commits. Neither of the above
commits considered 'set unwindonsignal on'. With this setting on,
when an inferior function call fails with a signal GDB will unwind the
stack back to the location where the inferior function call started.
In the b/p case we're looking at, the stop should be reported at the
location of the breakpoint, not at the location where the signal
occurred, and this isn't what happens.
This commit fixes this by ensuring that when unwindonsignal is 'on',
GDB reports a single stop event at the location of the breakpoint,
this fixes things for both CLI and MI.
The function call_thread_fsm::should_notify_stop is called when the
inferior function call completes and GDB is figuring out if the user
should be notified about this stop event by calling normal_stop from
fetch_inferior_event in infrun.c. If normal_stop is called, then this
notification will be for the location where the inferior call stopped,
which will be the location at which the signal occurred.
Prior to this commit, the only time that normal_stop was not called,
was if the inferior function call completed successfully, this was
controlled by ::should_notify_stop, which only turns false when the
inferior function call has completed successfully.
In this commit I have extended the logic in ::should_notify_stop. Now
there are three cases in which ::should_notify_stop will return false,
and we will not announce the first stop (by calling normal_stop).
These three reasons are:
1. If the inferior function call completes successfully, this is
unchanged behaviour,
2. If the inferior function call stopped due to a signal and 'set
unwindonsignal on' is in effect, and
3. If the inferior function call stopped due to an uncaught C++
exception, and 'set unwind-on-terminating-exception on' is in
effect.
However, if we don't call normal_stop then we need to call
async_enable_stdin in call_thread_fsm::should_stop. Prior to this
commit this was only done for the case where the inferior function
call completed successfully.
In this commit I now call ::should_notify_stop and use this to
determine if we need to call async_enable_stdin. With this done we
now call async_enable_stdin for each of the three cases listed above,
which means that GDB will exit wait_sync_command_done correctly (see
run_inferior_call in infcall.c).
With these two changes the problem is mostly resolved. However, the
solution isn't ideal, we've still lost some information.
Here is how GDB 13.1 behaves, this is before commits b1c0ab20809a and
2e411b8c68eb:
$ gdb -q /tmp/mi-condbreak-fail \
-ex 'set unwindonsignal on' \
-ex 'break 30 if (cond_fail())' \
-ex 'run'
Reading symbols from /tmp/mi-condbreak-fail...
Breakpoint 1 at 0x40111e: file /tmp/mi-condbreak-fail.c, line 30.
Starting program: /tmp/mi-condbreak-fail
Program received signal SIGSEGV, Segmentation fault.
0x0000000000401116 in cond_fail () at /tmp/mi-condbreak-fail.c:24
24 return *p; /* Crash here. */
Error in testing breakpoint condition:
The program being debugged was signaled while in a function called from GDB.
GDB has restored the context to what it was before the call.
To change this behavior use "set unwindonsignal off".
Evaluation of the expression containing the function
(cond_fail) will be abandoned.
Breakpoint 1, foo () at /tmp/mi-condbreak-fail.c:30
30 global_counter += 1; /* Set breakpoint here. */
(gdb)
In this state we see two stop notifications, the first is where the
signal occurred, while the second is where the breakpoint is located.
As GDB has unwound the stack (thanks to unwindonsignal) the second
stop notification reflects where the inferior is actually located.
Then after commits b1c0ab20809a and 2e411b8c68eb the behaviour changed
to this:
$ gdb -q /tmp/mi-condbreak-fail \
-ex 'set unwindonsignal on' \
-ex 'break 30 if (cond_fail())' \
-ex 'run'
Reading symbols from /tmp/mi-condbreak-fail...
Breakpoint 1 at 0x40111e: file /tmp/mi-condbreak-fail.c, line 30.
Starting program: /tmp/mi-condbreak-fail
Program received signal SIGSEGV, Segmentation fault.
0x0000000000401116 in cond_fail () at /tmp/mi-condbreak-fail.c:24
24 return *p; /* Crash here. */
Error in testing condition for breakpoint 1:
The program being debugged was signaled while in a function called from GDB.
GDB has restored the context to what it was before the call.
To change this behavior use "set unwindonsignal off".
Evaluation of the expression containing the function
(cond_fail) will be abandoned.
(gdb) bt 1
#0 foo () at /tmp/mi-condbreak-fail.c:30
(More stack frames follow...)
(gdb)
This is the broken state. GDB is reports the SIGSEGV location, but
not the unwound breakpoint location. The final 'bt 1' shows that the
inferior is not located in cond_fail, which is the only location GDB
reported, so this is clearly wrong.
After implementing the fixes described above we now get this
behaviour:
$ gdb -q /tmp/mi-condbreak-fail \
-ex 'set unwindonsignal on' \
-ex 'break 30 if (cond_fail())' \
-ex 'run'
Reading symbols from /tmp/mi-condbreak-fail...
Breakpoint 1 at 0x40111e: file /tmp/mi-condbreak-fail.c, line 30.
Starting program: /tmp/mi-condbreak-fail
Error in testing breakpoint condition for breakpoint 1:
The program being debugged was signaled while in a function called from GDB.
GDB has restored the context to what it was before the call.
To change this behavior use "set unwindonsignal off".
Evaluation of the expression containing the function
(cond_fail) will be abandoned.
Breakpoint 1, foo () at /tmp/mi-condbreak-fail.c:30
30 global_counter += 1; /* Set breakpoint here. */
(gdb)
This is better. GDB now reports a single stop at the location of the
breakpoint, which is where the inferior is actually located. However,
by removing the first stop notification we have lost some potentially
useful information about which signal caused the inferior to stop.
To address this I've reworked the message that is printed to include
the signal information. GDB now reports this:
$ gdb -q /tmp/mi-condbreak-fail \
-ex 'set unwindonsignal on' \
-ex 'break 30 if (cond_fail())' \
-ex 'run'
Reading symbols from /tmp/mi-condbreak-fail...
Breakpoint 1 at 0x40111e: file /tmp/mi-condbreak-fail.c, line 30.
Starting program: /tmp/mi-condbreak-fail
Error in testing condition for breakpoint 1:
The program being debugged received signal SIGSEGV, Segmentation fault
while in a function called from GDB. GDB has restored the context
to what it was before the call. To change this behavior use
"set unwindonsignal off". Evaluation of the expression containing
the function (cond_fail) will be abandoned.
Breakpoint 1, foo () at /tmp/mi-condbreak-fail.c:30
30 global_counter += 1; /* Set breakpoint here. */
(gdb)
This is better, the user now sees a single stop notification at the
correct location, and the error message describes which signal caused
the inferior function call to stop.
However, we have lost the information about where the signal
occurred. I did consider trying to include this information in the
error message, but, in the end, I opted not too. I wasn't sure it was
worth the effort. If the user has selected to unwind on signal, then
surely this implies they maybe aren't interested in debugging failed
inferior calls, so, hopefully, just knowing the signal name will be
enough. I figure we can always add this information in later if
there's a demand for it.
|
|
New helper proc mi_info_frame which takes care of running the MI
-stack-info-frame command and matching its output.
Like the breakpoint helper procs, this new proc takes a name/value
argument list and uses this to build the expected result regexp. This
means that we can now write something like:
mi_info_frame "test name here" \
-level 0 -func name -line 123
Instead of the current equivalent:
mi_gdb_test "235-stack-info-frame" \
"235\\^done,frame=\{level=\"0\",addr=\"$hex\",func=\"name\",file=\".*\",fullname=\".*\",line=\"123\",arch=\".*\"\}" \
"test name here"
There's also a helper proc mi_make_info_frame_regexp which is
responsible for building the 'frame={...}' part of the pattern.
I've update the two existing tests that use -stack-info-frame and
expect the command to succeed. There is another test that runs
-stack-info-frame and expects the command to fail -- the helper proc
doesn't help with this case, so that test is not changed.
|
|
Currently, each target backend is responsible for printing "[Thread
...exited]" before deleting a thread. This leads to unnecessary
differences between targets, like e.g. with the remote target, we
never print such messages, even though we do print "[New Thread ...]".
E.g., debugging the gdb.threads/attach-many-short-lived-threads.exp
with gdbserver, letting it run for a bit, and then pressing Ctrl-C, we
currently see:
(gdb) c
Continuing.
^C[New Thread 3850398.3887449]
[New Thread 3850398.3887500]
[New Thread 3850398.3887551]
[New Thread 3850398.3887602]
[New Thread 3850398.3887653]
...
Thread 1 "attach-many-sho" received signal SIGINT, Interrupt.
0x00007ffff7e6a23f in __GI___clock_nanosleep (clock_id=clock_id@entry=0, flags=flags@entry=0, req=req@entry=0x7fffffffda80, rem=rem@entry=0x7fffffffda80)
at ../sysdeps/unix/sysv/linux/clock_nanosleep.c:78
78 in ../sysdeps/unix/sysv/linux/clock_nanosleep.c
(gdb)
Above, we only see "New Thread" notifications, even though threads
were deleted.
After this patch, we'll see:
(gdb) c
Continuing.
^C[Thread 3558643.3577053 exited]
[Thread 3558643.3577104 exited]
[Thread 3558643.3577155 exited]
[Thread 3558643.3579603 exited]
...
[New Thread 3558643.3597415]
[New Thread 3558643.3600015]
[New Thread 3558643.3599965]
...
Thread 1 "attach-many-sho" received signal SIGINT, Interrupt.
0x00007ffff7e6a23f in __GI___clock_nanosleep (clock_id=clock_id@entry=0, flags=flags@entry=0, req=req@entry=0x7fffffffda80, rem=rem@entry=0x7fffffffda80)
at ../sysdeps/unix/sysv/linux/clock_nanosleep.c:78
78 in ../sysdeps/unix/sysv/linux/clock_nanosleep.c
(gdb) q
This commit fixes this by moving the thread exit printing to common
code instead, triggered from within delete_thread (or rather,
set_thread_exited).
There's one wrinkle, though. While most targest want to print:
[Thread ... exited]
the Windows target wants to print:
[Thread ... exited with code <exit_code>]
... and sometimes wants to suppress the notification for the main
thread. To address that, this commits adds a delete_thread_with_code
function, only used by that target (so far).
This fix was originally posted as part of a larger series:
https://inbox.sourceware.org/gdb-patches/20221212203101.1034916-1-pedro@palves.net/
But didn't really need to be part of that series. In order to get
this fix merged sooner, I (Andrew Burgess) have rebased this commit
outside of the original series. Any bugs introduced while splitting
this patch out and rebasing, are entirely my own.
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=30129
Co-Authored-By: Andrew Burgess <aburgess@redhat.com>
|
|
After the previous commit, exit_inferior_1 no longer makes use of the
silent parameter. This commit removes this parameter and cleans up
the callers.
After doing this exit_inferior_1, exit_inferior, and
exit_inferior_silent are all equivalent, so rename exit_inferior_1 to
exit_inferior and delete exit_inferior_silent, update all the callers.
Also I spotted the declaration exit_inferior_num_silent in inferior.h,
but this function is not defined anywhere, so I deleted the
declaration.
There should be no user visible changes after this commit.
|
|
After this commit:
commit a78ef8757418105c35685c5d82b9fdf79459321b
Date: Wed Jun 22 18:10:00 2022 +0100
Always emit =thread-exited notifications, even if silent
The function mi_interp::on_thread_exited (or mi_thread_exit as the
function was called back then) no longer makes use of the "silent"
parameter.
As a result there is no difference between inferior::clear_thread_list
with silent true or false, because:
- None of the interpreter ::on_thread_exited functions rely on the
silent parameter, and
- None of GDB's thread_exit observers rely on the silent parameter
either.
This commit removes the silent parameter from
inferior::clear_thread_list, and makes the function always silent.
This commit was originally part of a larger series:
https://inbox.sourceware.org/gdb-patches/20221212203101.1034916-1-pedro@palves.net/
But didn't really need to be part of that series. I had an interest
in seeing this patch merged:
https://inbox.sourceware.org/gdb-patches/20221212203101.1034916-31-pedro@palves.net/
Which also didn't really need to be part of the larger series, but
does depend, at least a little, on this commit. In order to get the
fix I'm interested in merged quicker, I (Andrew Burgess) have rebased
this commit outside of the original series. Any bugs introduced while
splitting this patch out and rebasing, are entirely my own.
There should be no user visible changes after this commit.
Co-Authored-By: Andrew Burgess <aburgess@redhat.com>
|
|
Remove the static mi_parse::make functions, and instead use the
mi_parse constructor.
This is a partial revert of the commit:
commit fde3f93adb50c9937cd2e1c93561aea2fd167156
Date: Mon Mar 20 10:56:55 2023 -0600
Introduce "static constructor" for mi_parse
which introduced the mi_parse::make functions, though after discussion
on the list the reasons for seem to have been lost[1]. Given there
are no test regressions when moving back to using the constructors, I
propose we should do that for now.
There should be no user visible changes after this commit.
[1] https://inbox.sourceware.org/gdb-patches/20230404-dap-loaded-sources-v2-5-93f229095e03@adacore.com/
Approved-By: Tom Tromey <tom@tromey.com>
|
|
Have the mi_out_new function return a std::unique_ptr instead of a raw
pointer. Update the two uses of mi_out_new.
There should be no user visible changes after this commit.
Approved-By: Tom Tromey <tom@tromey.com>
|
|
While GDB is still C++11, lets add a gdb::make_unique template
function that can be used to create std::unique_ptr objects, just like
the C++14 std::make_unique.
If GDB is being compiled with a C++14 compiler then the new
gdb::make_unique function will delegate to the std::make_unique. I
checked with gcc, and at -O1 and above gdb::make_unique will be
optimised away completely in this case.
If C++14 (or later) becomes our minimum, then it will be easy enough
to go through the code and replace gdb::make_unique with
std::make_unique later on.
I've make use of this function in all the places I think this can
easily be used, though I'm sure I've probably missed some.
Should be no user visible changes after this commit.
Approved-By: Tom Tromey <tom@tromey.com>
|
