Tom de Vries [Mon, 23 Sep 2024 05:45:54 +0000 (07:45 +0200)]
[gdb/testsuite] Fix timeout in gdb.mi/mi-multi-commands.exp
On aarch64-linux, with test-case gdb.mi/mi-multi-commands.exp once in a while
I run into (edited for readability):
...
(gdb) ^M
<LOTS-OF-SPACES>-data-evaluate-expression $a^M
-data-evaluate-^done,value="\"FIRST COMMAND\""^M
expression $b(gdb) ^M
^M
^done,value="\"TEST COMPLETE\""^M
(gdb) ^M
PASS: $exp: args=: look for first command output, command length 236
FAIL: $exp: args=: look for second command output, command length 236 (timeout)
...
This is more likely to trigger when running the test-case using
taskset -c <cpu> (where in a big.little setup we pick a little cpu).
The setup here is that the test-case issues these two commands at once:
...
-data-evaluate-expression $a
-data-evaluate-expression $b
...
where the length of the first command is artificially increased by prefixing
it with spaces, show as <LOTS-OF-SPACES> above.
What happens is that gdb, after parsing the first command, executes it.
Then the output of the first command intermixes with the echoing of the second
command, which produces this line containing the first prompt:
...
expression $b(gdb) ^M
...
which doesn't match the \r\n prefix of the regexp supposed to consume the
first prompt:
...
-re "\r\n$mi_gdb_prompt" {
...
turned PLT32 relocation against section into PC32 relocation. But these
transformations are valid only for PC-relative relocations. Add fx_pcrel
check for PC-relative relocations when performing these transformations
to keep PLT32 relocation in `movq $foo@PLT, %rax`.
gas/
PR gas/32196
* config/tc-i386.c (tc_i386_fix_adjustable): Return fixP->fx_pcrel
for PLT32 relocations.
(i386_validate_fix): Turn PLT32 relocation into PC32 relocation
only if fixp->fx_pcrel is set.
* testsuite/gas/i386/reloc32.d: Updated.
* testsuite/gas/i386/reloc64.d: Likewise.
* testsuite/gas/i386/reloc32.s: Add PR gas/32196 test.
* testsuite/gas/i386/reloc64.s: Likewise.
ld/
PR gas/32196
* testsuite/ld-x86-64/plt3.s: New file.
* testsuite/ld-x86-64/x86-64.exp: Run plt3.
Tom de Vries [Sat, 21 Sep 2024 04:12:40 +0000 (06:12 +0200)]
[gdb/testsuite] Fix timeout in gdb.ada/call_pn.exp
With test-case gdb.ada/call_pn.exp and glibc debug info installed, I ran into
this timeout:
...
(gdb) maint expand-symtabs^M
FAIL: gdb.ada/call_pn.exp: maint expand-symtabs (timeout)
...
The timeout was related to running the cpu at base frequency of 400Mhz instead
of boost frequency of 3.5Ghz (efficiency core) or 4.7Ghz (performance core).
But when investigating the test-case I realized that the maint expand-symtabs
could be limited to the source files, so use that to speed up the test-case.
Tested on x86_64-linux.
Co-Authored-By: Tom Tromey <tom@tromey.com> Approved-By: Tom Tromey <tom@tromey.com>
PR testsuite/32177
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=32177
Tom de Vries [Sat, 21 Sep 2024 03:55:18 +0000 (05:55 +0200)]
[gdb/testsuite] Drop -readnow in three gdb.dwarf2 test-cases
When running the testsuite in an enviroment simulating a stressed system, I
ran into timeouts in three test-cases in gdb.dwarf2:
- gdb.dwarf2/count.exp,
- gdb.dwarf2/implptrconst.exp, and
- gdb.dwarf2/implptrpiece.exp.
In all three cases, -readnow is used which results in symtabs being expanded for
the executable, /lib64/libc.so.6 and /lib64/ld-linux-x86-64.so.2.
We could address this by limiting the scope of -readnow to the executable, but
after reviewing the test-cases there doesn't seem to be a clear reason to use
-readnow.
Assembler shouldn't accept invalid TLS instructions, TLS relocations
can only be used with specific instructions as specified in TLS psABI
and linker issues an error when TLS relocations are used with wrong
instructions or format. Since it is inconvenient for gcc to rely on
linker to report errors, adding TLS check in the assembler stage so
that gcc can know TLS errors earlier.
gas/ChangeLog:
PR gas/32022
* config.in: Regenerate.
* config/tc-i386.c
*(enum x86_tls_error_type): New.
*(struct _i386_insn): Added has_gotrel to indicate whether TLS
relocations need to be checked.
(x86_check_tls_relocation): Added a new function to check TLS
relocation.
(x86_report_tls_error): Created a new function to report TLS error.
(i386_assemble): Handle x86_check_tls_relocation.
(lex_got): Set i.has_gotrel.
(OPTION_MTLS_CHECK): Added a new option to contrl TLS check.
(struct option): Ditto.
(md_parse_option): Ditto.
(md_show_usage): Ditto.
* configure.ac: Added a new option to check TLS relocation by
default.
* configure: Regenerated.
* doc/c-i386.texi: Document -mtls-check=.
* testsuite/gas/i386/i386.exp: Added new tests.
* testsuite/gas/i386/ilp32/ilp32.exp: Ditto.
* testsuite/gas/i386/ilp32/reloc64.d: Disable TLS check for it.
* testsuite/gas/i386/ilp32/x32-tls.d: Ditto.
* testsuite/gas/i386/inval-tls.l: Added more test cases.
* testsuite/gas/i386/inval-tls.s: Ditto.
* testsuite/gas/i386/reloc32.d: Disable TLS check for it.
* testsuite/gas/i386/reloc64.d: Ditto.
* testsuite/gas/i386/x86-64-inval-tls.l: Added more test cases.
* testsuite/gas/i386/x86-64-inval-tls.s: Ditto.
* testsuite/gas/i386/x86-64.exp: Added new tests.
* testsuite/gas/i386/ilp32/x32-inval-tls.l: New test.
* testsuite/gas/i386/ilp32/x32-inval-tls.s: Ditto.
* testsuite/gas/i386/ilp32/x86-64-tls.d: Ditto.
* testsuite/gas/i386/tls.d: Ditto.
* testsuite/gas/i386/tls.s: Ditto.
* testsuite/gas/i386/x86-64-tls.d: Ditto.
* testsuite/gas/i386/x86-64-tls.s: Ditto.
H.J. Lu [Fri, 20 Sep 2024 19:44:22 +0000 (03:44 +0800)]
ld: Use --no-rosegment to ld for PR ld/22393 tests
The commit
bf6d7087de0 ld: Move the .note.build-id section to near the start of the memory map
moves the .note.build-id section before text sections. When --rosegment
and -z separate-code are used together, the .note.gnu.property section
is placed between the .note.build-id section and text sections in the
same PT_LOAD segment by orphan placement. Pass --no-rosegment to ld for
PR ld/22393 tests to avoid linker test failures.
PR ld/32190
* testsuite/ld-elf/pr22393-2a.rd: Pass --no-rosegment to ld.
* testsuite/ld-elf/pr22393-2b.rd: Likewise.
* testsuite/ld-elf/shared.exp: Pass --no-rosegment to ld when
building pr22393-2 tests.
* testsuite/ld-x86-64/pr22393-3a.rd: Pass --no-rosegment to ld.
* testsuite/ld-x86-64/pr22393-3b.rd: Likewise.
* testsuite/ld-x86-64/x86-64.exp: Pass --no-rosegment to ld when
building pr22393-3 tests.
With the previous commits, the only thing entangling elf and coff file
reading with dbx file reading is the functions
{elf|coff}stab_build_psymtabs, defined in dbxread.c. These functions
depend on dbx_symfile_read.
To solve this, I renamed read_stabs_symtab to read_stabs_symtab_1, and
created a function with the original name that does what
dbx_symfile_read used to do.
This way, dbx_symfile_read can just call read_stabs_symtab, and the elf
and coff psymtab builders can also call it directly, fully disentangling
the readers, which would allow us to selectively not compile dbxread in
the future.
gdb: Move read_dbx_symtab to stabsread, and rename to read_stabs_symtab
Despite the name, read_dbx_symtab is not only used for the dbx file
format (also called the aout format). It is used by elf and coff
implicitly as well. So I think it makes more sense to have this function
in the generic stabsread file, so that reading elf files or coff files
depends less on GDB's ability to read dbx files.
There were 11 static functions in dbxread that were onlyl helper
functions, they were moved and kept as static in stabsread.c. Notably,
dbx_read_symtab - which is installed as a callback on legacy_psymtab
for aout, elf and coff at least - has been moved to stabsread.c and
renamed as well; the function that is specific to aout is
dbx_symfile_read, and that hasn't been moved.
Some macros had to be moved as well, but since they are still used
in dbxread, they were moved to the .h file that the struct symloc
is declared, so anyone can properly use the struct.
gdb: Move dbx_end_psymtab to stabsread, and rename to stabs_end_psymtab
This function is used by multiple stabs readers (even if not all), and
the comment in stabsread.h even acknowledges it. I believe that the
comment is incorrect in saying that the function should be in dbxread
because not everyone uses it. If any one reader other than dbx uses
it, the function should be in stabsread, in my opinion.
This commit makes also renames the function to stabs_end_psymtab since,
again, this is not specific to dbx/aout format.
struct symloc had to be moved because stabs_end_psymtab dereferences
symloc objects, so stabsread.c must be aware of the full struct.
The function process_one_symbol was defined in the file dbxread.c, but
this function is used by all file formats that handle stabs debug
information. It makes much more sense for it to be in the stabsread.c
file instead.
To move that function, many other static functions had to be moved from
dbxread. A few were only used by process_one_symbol, so they're still
static, but most were used by other functions still in dbxread, so they
are being exported by stabsread.h
Finally, the registry entry has been moved as well, seeing as it was
already exported by gdb-stabs.h, and stabsread.c will need it to
properly use the newly added function.
With this change, reading mdebug files is totally independent of reading
dbx.
The file dbxread.c, which is responsible for reading stabs information
for multiple file formats, relies heavily on setting and using global
variables over the course of reading symbols.
Future patches aim to make stabs reading more file format independent,
and this patch starts that change by introducing a stabs_context struct,
that will hold all the relevant variables. This context struct is saved
on the registry key inside the objfile being read. Some of those global
variables have been deemed irrelevant:
* dbxread_objfile - Since we're saving in an objfile, this is redundant
* symfile_bfd - It is trivial to get the bfd pointer from the objfile,
so also unnecessary
* string_table_offset - was never initialized, just used to set a value.
That usage was substituted by a hardcoded 0
* next_file_string_table_offset - was only used by read_dbx_symtab, so
it was turned into a local variable there.
As I was moving variables, I also couldn't think of a good reason for
the bincl_list to be a pointer, so it was changed to just be an
std::vector.
gdb/testsuite: rework bp-cond-failure to not depend on inlining
The test gdb.base/bp-cond-failure is implicitly expecting that the
function foo will be inlined twice and gdb will be able to find 2
locations to place a breakpoint. When clang is used, gdb only finds
one location which causes the test to fail. Since the test is not
worried about handling breakpoints on inlined functions, but rather on
the format of the message on a breakpoint condition fail, this seems
like a false fail report.
This commit reworks the test to be in c++, and uses function overloading
to ensure that 2 locations will always be found. Empirical testing
showed that, for clang, we will land on location 2 with the currest exp
commands, no matter the order of the functions declared, whereas for gcc
it depends on the order that functions were declared, so they are
ordered to always land on the second location, this way we are able to
hardcode it and check for it.
Reviewed-by: Keith Seitz <keiths@redhat.com> Approved-By: Tom Tromey <tom@tromey.com>
Nick Clifton [Thu, 19 Sep 2024 15:45:30 +0000 (16:45 +0100)]
ld: Move the .note.build-id section to near the start of the memory map.
This helps GDB to locate the debug information associated with a core dump.
Core dumps include the first page of an executable's image, and if this
page include the .note.build-id section then GDB can find it and then track
down a debug info file for that build-id.
Fixed UBSAN runtime errors such as:
- member call on address which does not point to an object of type 'Vector'
- load of misaligned address 0x623e5a670173 for type 'int', which requires 4 byte alignment
gprofng/ChangeLog
2024-09-17 Vladimir Mezentsev <vladimir.mezentsev@oracle.com>.
PR gprofng/32096
* libcollector/unwind.c: Fix UBSAN runtime errors.
* src/CallStack.cc (add_stack_java, add_stack_java_epilogue):
Change argument type to Vector<Histable*>*.
* src/Experiment.cc (update_ts_in_maps): Change variable type.
* src/Experiment.h: Change field type to Vector<Histable*>*.
H.J. Lu [Sun, 15 Sep 2024 23:11:39 +0000 (07:11 +0800)]
x86/APX: Don't promote AVX/AVX2 instructions out of APX spec
V{BROADCAST,EXTRACT,INSERT}{F,I}128 and VROUND{P,S}{S,D} aren't promoted
to support EGPR in APX spec. Don't promote them out of APX spec. This
commit effectively reverted:
ec3babb8c10 x86/APX: V{BROADCAST,EXTRACT,INSERT}{F,I}128 can also be expressed 5a635f1f59a x86/APX: VROUND{P,S}{S,D} encodings require AVX512{F,VL} eea4357967b x86/APX: VROUND{P,S}{S,D} can generally be encoded
gdb/testsuite: skip gdb.mi/dw2-ref-missing-frame.exp with clang
The test gdb.mi/dw2-ref-missing-frame.exp uses the old-school way to set
debug information by hand, using a .S file and assembly labels to get
addresses. Unfortunately, clang will always re-arrange the global labels
to be side by side, making high and low PC for CUs and functions be the
same, and thus they will all be empty ranges. This makes the test fail,
since we never technically enter the functions that we want to check.
This commit skips that test when using clang. If we ever port this test
to use the dwarf assembler, we can reenable it with clang.
gdb/testsuite: fix gdb.mi/mi-var-cp.exp with clang
The inline tests in gdb.mi/mi-var-cp.cc were failing when using clang to
run the test. This happened because inline tests want to step past the C
statements and then run the TCL tests, but in mi-var-cp.cc the statement
to be stepped past is "return s2.i;". Since clang links the epilogue
information to the return statement, not the closing brace,
single-stepping past return had us exiting the function - which made the
expressions invalid.
This commit fixes this by making the function have 2 C statements, and
the return one be after all inline tests, so we know GDB won't leave the
function before running the create_varobj tests.
gdb/testsuite: fix gdb.mi/mi-catch-cpp-exceptions.exp with clang
Clang adds line table information for a try/catch block differently to
gcc. Instead of linking the instructions related to __cxa_begin_catch to
the line containing the "catch" statement in the source code, it links
to the closing brace of the try block.
This was causing gdb.mi/mi-catch-cpp-exceptions.exp to fail when tested
with clang. The test was updated to have the catch in the same line as
the closing brace so it passes with no additional modifications with
clang.
MIPS/GAS: Discard redundant instruction from DDIV/DREM macros
A sequence such as:
li at,-1
bne xx,at,0f
li at,1
dsll32 at,at,0x1f
is produced in the expansion of the DDIV and DREM assembly macros, where
a redundant `li at,1' instruction is used to load an intermediate value
of 1 into $at, which is then left-shifted by 63 with `dsll32 at,at,0x1f'
yielding 0x8000000000000000. However this value likewise results from
left-shifting the value of -1, already present in $at at this point.
Remove the extraneous instruction then, shortening the sequence emitted.
Adjust dumps in the testsuite accordingly.
MIPS/GAS/testsuite: Print instructions in hex in division tests
Add `--show-raw-insn' to division tests so as to verify branch offsets
without the need to know actual offsets into the text section individual
instructions have been assembled at. Add `-z' where applicable to make
interlock NOP instructions appear in output so as to verify them without
the need to know the offsets too. Replace individual offsets to match
against with generic patterns so that a change in the expansion of an
assembly macro does not affect code that follows.
MIPS/opcodes: Rework documentation for instruction args
Rewrite the inline documentation for the characters used in the `args'
member of `struct mips_opcode' to make it consistent in terms of style
and formatting. Discard references to inexistent macros.
Tom de Vries [Sat, 14 Sep 2024 12:09:35 +0000 (14:09 +0200)]
[gdb/symtab] Revert "Change handling of DW_TAG_enumeration_type in DWARF scanner"
After adding dwarf assembly to test-case gdb.dwarf2/enum-type.exp that adds
this debug info:
...
<1><11f>: Abbrev Number: 3 (DW_TAG_enumeration_type)
<120> DW_AT_specification: <0x130>
<2><124>: Abbrev Number: 4 (DW_TAG_enumerator)
<125> DW_AT_name : val1
<12a> DW_AT_const_value : 1
<2><12b>: Abbrev Number: 0
<1><12c>: Abbrev Number: 5 (DW_TAG_namespace)
<12d> DW_AT_name : ns
<2><130>: Abbrev Number: 6 (DW_TAG_enumeration_type)
<131> DW_AT_name : e
<133> DW_AT_type : <0x118>
<137> DW_AT_declaration : 1
...
I run into an assertion failure:
...
(gdb) file enum-type^M
Reading symbols from enum-type...^M
cooked-index.h:214: internal-error: get_parent: \
Assertion `(flags & IS_PARENT_DEFERRED) == 0' failed.^M
...
This was reported in PR32160 comment 1.
This is a regression since commit 4e417d7bb1c ("Change handling of
DW_TAG_enumeration_type in DWARF scanner").
Fix this by reverting the commit.
[ Also drop the kfails for PR31900 and PR32158, which are regressions by that
same commit. ]
That allows us to look at the output of "maint print objfiles", and for val1
we get an entry without parent:
...
[27] ((cooked_index_entry *) 0x7fbbb4002ef0)
name: val1
canonical: val1
qualified: val1
DWARF tag: DW_TAG_enumerator
flags: 0x0 []
DIE offset: 0x124
parent: ((cooked_index_entry *) 0)
...
which is incorrect, as noted in that same comment, but an improvement over the
assertion failure, and I don't think that ever worked. This is to be
addressed in a follow-up patch.
Reverting the commit begs the question: what was it trying to fix in the first
place, and do we need a different fix? I've investigated this and filed
PR32160 to track this.
My guess is that the commit was based on a misunderstand of what we track
in cooked_indexer::m_die_range_map.
Each DIE has two types of parent DIEs:
- a DIE that is the parent as indicated by the tree structure in which DIEs
occur, and
- a DIE that represent the parent scope.
In most cases, these two are the same, but some times they're not.
The debug info above demonstrates such a case. The DIE at 0x11f:
- has a tree-parent: the DIE representing the CU, and
- has a scope-parent: DIE 0x12c representing namespace ns.
In cooked_indexer::m_die_range_map, we track scope-parents, and the commit
tried to add a tree-parent instead.
So, I don't think we need a different fix, and propose we backport the reversal
for gdb 15.2.
Tested on x86_64-linux.
Approved-By: Tom Tromey <tom@tromey.com>
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=31900
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=32158
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=32160
int main () {
return (int)ns::ec::val2;
}
...
compiled with debug info:
...
$ g++ test.c -g
...
When looking at the cooked index entry for val2 using "maint print objfiles",
we get:
...
[7] ((cooked_index_entry *) 0x7f8ecc002ef0)
name: val2
canonical: val2
qualified: ns::val2
DWARF tag: DW_TAG_enumerator
flags: 0x0 []
DIE offset: 0xe9
parent: ((cooked_index_entry *) 0x7f8ecc002e90) [ns]
...
which is wrong, there is no source level entity ns::val2.
This is PR symtab/32158.
This is a regression since commit 4e417d7bb1c ("Change handling of
DW_TAG_enumeration_type in DWARF scanner").
Reverting the commit on current trunk fixes the problem, and gets us instead:
...
[7] ((cooked_index_entry *) 0x7fba70002ef0)
name: val2
canonical: val2
qualified: ns::ec::val2
DWARF tag: DW_TAG_enumerator
flags: 0x0 []
DIE offset: 0xe9
parent: ((cooked_index_entry *) 0x7fba70002ec0) [ec]
...
Add a regression test for this PR in test-case gdb.dwarf2/enum-type-c++.exp.
Tested on x86_64-linux.
Approved-By: Tom Tromey <tom@tromey.com>
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=32158
Tom de Vries [Sat, 14 Sep 2024 12:09:35 +0000 (14:09 +0200)]
[gdb/testsuite] Add gdb.dwarf2/enum-type-c++.exp, regression test for PR31900.
Consider the following test-case:
...
$ cat a.h
namespace ns {
class A {
public:
enum {
val1 = 1
};
};
}
$ cat main.c
ns::A a;
int
main (void)
{
return 0;
}
$ cat val1.c
int u1 = ns::A::val1;
...
compiled with debug info:
...
$ g++ main.c val1.c -g
...
When trying to print ns::A::val with current trunk and gdb 15.1 we get:
...
$ gdb -q -batch a.out -ex "print ns::A::val1"
There is no field named val1
...
This PR c++/31900.
With gdb 14.2 we get the expected:
...
$ gdb -q -batch a.out -ex "print ns::A::val1"
$1 = ns::A::val1
...
This is a regression since commit 4e417d7bb1c ("Change handling of
DW_TAG_enumeration_type in DWARF scanner").
Reverting the commit on current trunk fixes the problem.
So how does this problem happen?
First, let's consider the current trunk, with the commit reverted.
Gdb looks for the entry ns::A::val1, and find this entry:
...
[29] ((cooked_index_entry *) 0x7f7830002ef0)
name: val1
canonical: val1
qualified: ns::A::val1
DWARF tag: DW_TAG_enumerator
flags: 0x0 []
DIE offset: 0x15a
parent: ((cooked_index_entry *) 0x7f7830002ec0) [A]
...
and expands the corresponding CU val1.c containing this debug info:
...
<2><14a>: Abbrev Number: 3 (DW_TAG_class_type)
<14b> DW_AT_name : A
<14d> DW_AT_byte_size : 1
<3><150>: Abbrev Number: 4 (DW_TAG_enumeration_type)
<151> DW_AT_encoding : 7 (unsigned)
<152> DW_AT_byte_size : 4
<153> DW_AT_type : <0x163>
<159> DW_AT_accessibility: 1 (public)
<4><15a>: Abbrev Number: 5 (DW_TAG_enumerator)
<15b> DW_AT_name : val1
<15f> DW_AT_const_value : 1
<4><160>: Abbrev Number: 0
<3><161>: Abbrev Number: 0
<2><162>: Abbrev Number: 0
...
after which it finds ns::A::val1 in the expanded symtabs.
Now let's consider the current trunk as is (so, with the commit present).
Gdb looks for the entry ns::A::val1, but doesn't find it because the val1
entry is missing its parent:
...
[29] ((cooked_index_entry *) 0x7f5240002ef0)
name: val1
canonical: val1
qualified: val1
DWARF tag: DW_TAG_enumerator
flags: 0x0 []
DIE offset: 0x15a
parent: ((cooked_index_entry *) 0)
...
Then gdb looks for the entry ns::A, and finds this entry:
...
[3] ((cooked_index_entry *) 0x7f5248002ec0)
name: A
canonical: A
qualified: ns::A
DWARF tag: DW_TAG_class_type
flags: 0x0 []
DIE offset: 0xdd
parent: ((cooked_index_entry *) 0x7f5248002e90) [ns]
...
which corresponds to this debug info, which doesn't contain val1
due to -fno-eliminate-unused-debug-types:
...
<2><dd>: Abbrev Number: 3 (DW_TAG_class_type)
<de> DW_AT_name : A
<e0> DW_AT_byte_size : 1
<2><e3>: Abbrev Number: 0
...
Gdb expands the corresponding CU main.c, after which it doesn't find
ns::A::val1 in the expanded symtabs.
The root cause of the problem is the missing parent on the val1
cooked_index_entry, but this only becomes user-visible through the
elaborate scenario above.
Add a test-case gdb.dwarf2/enum-type-c++.exp that contains a regression test
for this problem that doesn't rely on expansion state or
-feliminate-unused-debug-types, but simply tests for the root cause by
grepping for ns::A::val1 in the output of "maint print objfile".
Tested on x86_64-linux.
Approved-By: Tom Tromey <tom@tromey.com>
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=31900
Tom Tromey [Thu, 12 Sep 2024 23:22:24 +0000 (17:22 -0600)]
Update more types for section index change
Commit f89276a2f3e ("change type of `general_symbol_info::m_section`
to int") did what it says in the title -- changed the type of the
section index from short to int. However, it seems incomplete, in
that there are uses of the section index that use the type 'short'.
This patch fixes the ones I found, first by searching for
"short.*sect" and then by looking at all the callers of section_index
(and then functions called with the resulting value) just to try to be
more sure.
Approved-by: Kevin Buettner <kevinb@redhat.com> Approved-By: Simon Marchi <simon.marchi@efficios.com>
Tom Tromey [Tue, 10 Sep 2024 17:05:04 +0000 (11:05 -0600)]
Fix quoting in gdb-add-index.sh
When the filename quoting change was merged into the AdaCore tree, we
saw a regression in a test setup that uses the DWARF 5 index (that is
running gdb-add-index), and a filename with a space in it.
Initially I thought this was a change in the 'file' command -- but
looking again, I found out that 'file' has worked this way for a
while, and our immediate error was caused by the (documented) change
to "save gdb-index".
While I'm not sure why this test was working previously, it seems to
me that gdb-add-index.sh requires a change to quote the arguments to
"file" and "save gdb-index".
While working on this, though, it seemed to me that multiple other
spots needed quoting for the script to work correctly. And, I was
unable to get quoting working correctly in the objcopy calls, so I
split it into multiple different invocations.
Tom Tromey [Wed, 11 Sep 2024 15:25:45 +0000 (09:25 -0600)]
Add quoting to 'file' invocations in DAP
Oleg Tolmatcev noticed that DAP launch and attach requests don't
properly handle Windows filenames, because "file" doesn't handle the
backslash characters correctly. This patch adds quoting to the
command in an attempt to fix this.
Simon Marchi [Mon, 12 Aug 2024 17:09:05 +0000 (13:09 -0400)]
gdb/solib: use owning_intrusive_list for solib list
Functions implementing `solib_ops::current_sos` return a list of solib
object, transferring the ownership to their callers. However, the
return type, `intrusive_list<solib>`, does not reflect that.
Also, some of these functions build these lists incrementally, reading
this from the target for each solib. If a target read were to throw,
for instance, the already created solibs would just be leaked.
Change `solib_ops::current_sos` to return an owning_intrusive_list to
address that. Change `program_space::so_list` to be an
owning_intrusive_list as well. This also saves us doing a few manual
deletes.
Change-Id: I6e4071d49744874491625075136c59cce8e608d4 Reviewed-by: Keith Seitz <keiths@redhat.com>
It occured to me that `intrusive_list<solib>`, as returned by
`solib_ops::current_sos`, for instance, is not very safe. The
current_sos method returns the ownership of the solib objects
(heap-allocated) to its caller, but the `intrusive_list<solib>` type
does not convey it. If a function is building an
`intrusive_list<solib>` and something throws, the solibs won't
automatically be deleted. Introduce owning_intrusive_list to fill this
gap.
Interface
---------
The interface of owning_intrusive_list is mostly equivalent to
intrusive_list, with the following differences:
- When destroyed, owning_intrusive_list deletes all element objects.
The clear method does so as well.
- The erase method destroys the removed object.
- The push_front, push_back and insert methods accept a `unique_ptr<T>`
(compared to `T &` for intrusive_list), taking ownership of the
object.
- owning_intrusive_list has emplace_front, emplace_back and emplace
methods, allowing to allocate and construct an object directly in the
list. This is really just a shorthand over std::make_unique and
insert (or push_back / push_front if you don't care about the return
value), but I think it is nicer to read:
These methods are not `noexcept`, since the allocation or the
constructor could throw.
- owning_intrusive_list has a release method, allowing to remove an
element without destroying it. The release method returns a
pair-like struct with an iterator to the next element in the list
(like the erase method) and a unique pointer transferring the
ownership of the released element to the caller.
- owning_intrusive_list does not have a clear_and_dispose method, since
that is typically used to manually free items.
Implementation
--------------
owning_intrusive_list privately inherits from intrusive_list, in order
to re-use the linked list machinery. It adds ownership semantics around
it.
Testing
-------
Because of the subtle differences in the behavior in behavior and what
we want to test for each type of intrusive list, I didn't see how to
share the tests for the two implementations. I chose to copy the
intrusive_list tests and adjust them for owning_intrusive_list.
The verify_items function was made common though, and it tries to
dereference the items in the list, to make sure they have not been
deleted by mistake (which would be caught by Valgrind / ASan).
Change-Id: Idbde09c1417b79992a0a9534d6907433e706f760 Co-Authored-By: Pedro Alves <pedro@palves.net> Reviewed-by: Keith Seitz <keiths@redhat.com>
Simon Marchi [Mon, 12 Aug 2024 17:09:03 +0000 (13:09 -0400)]
gdbsupport/intrusive-list: make insert return an iterator
Make the insert method return an iterator to the inserted element. This
mimics what boost does [1] and what the standard library insert methods
generally do [2].
Simon Marchi [Mon, 12 Aug 2024 17:09:02 +0000 (13:09 -0400)]
gdbsupport/intrusive-list: sprinkle noexcept
Some methods of intrusive_list are marked noexcept. But really,
everything in that file could be noexcept. Add it everywhere.
The only one I had a doubt about is clear_and_dispose: what if the
disposer throws? The boost equivalent [1] is noexcept and requires the
disposer not to throw. The rationale is probably the same as for
destructors. What if the disposer throws for an element in the middle
of the list? Do you skip the remaining elements? Do you swallow the
exception and keep calling the disposer for the remaining elements?
It's simpler to say no exceptions allowed.
Stephan Rohr [Wed, 21 Feb 2024 09:55:37 +0000 (01:55 -0800)]
testsuite, trace: add guards if In-Process Agent library is not found
Several tests in gdb.trace trigger TCL errors if the In-Process Agent
library is not found, e.g.:
Running gdb/testsuite/gdb.trace/change-loc.exp ...
ERROR: tcl error sourcing gdb/testsuite/gdb.trace/change-loc.exp.
ERROR: error copying "gdb/gdb/testsuite/../../gdbserver/libinproctrace.so":
no such file or directory
while executing
"file copy -force $fromfile $tofile"
(procedure "gdb_remote_download" line 29)
invoked from within
"gdb_remote_download target $target_file"
(procedure "gdb_download_shlib" line 6)
invoked from within
"gdb_download_shlib $file"
(procedure "gdb_load_shlib" line 2)
invoked from within
"gdb_load_shlib $libipa"
(file "gdb/testsuite/gdb.trace/change-loc.exp" line 354)
invoked from within
"source gdb/testsuite/gdb.trace/change-loc.exp"
("uplevel" body line 1)
invoked from within
"uplevel #0 source gdb/testsuite/gdb.trace/change-loc.exp"
invoked from within
"catch "uplevel #0 source $test_file_name""
Protect against this error by checking if the library is available.
Simon Marchi [Thu, 12 Sep 2024 12:27:08 +0000 (08:27 -0400)]
gdb: change type of `general_symbol_info::m_section` to int
The binary provided with bug 32165 [1] has 36139 ELF sections. GDB
crashes on it with (note that my GDB is build with -D_GLIBCXX_DEBUG=1:
$ ./gdb -nx -q --data-directory=data-directory ./vmlinux
Reading symbols from ./vmlinux...
(No debugging symbols found in ./vmlinux)
(gdb) info func
/usr/include/c++/14.2.1/debug/vector:508:
In function:
std::debug::vector<_Tp, _Allocator>::reference std::debug::vector<_Tp,
_Allocator>::operator[](size_type) [with _Tp = long unsigned int;
_Allocator = std::allocator<long unsigned int>; reference = long
unsigned int&; size_type = long unsigned int]
Error: attempt to subscript container with out-of-bounds index -29445, but
container only holds 36110 elements.
Objects involved in the operation:
sequence "this" @ 0x514000007340 {
type = std::debug::vector<unsigned long, std::allocator<unsigned long> >;
}
The crash occurs here:
#3 0x00007ffff5e334c3 in __GI_abort () at abort.c:79
#4 0x00007ffff689afc4 in __gnu_debug::_Error_formatter::_M_error (this=<optimized out>) at /usr/src/debug/gcc/gcc/libstdc++-v3/src/c++11/debug.cc:1320
#5 0x0000555561119a16 in std::__debug::vector<unsigned long, std::allocator<unsigned long> >::operator[] (this=0x514000007340, __n=18446744073709522171)
at /usr/include/c++/14.2.1/debug/vector:508
#6 0x0000555562e288e8 in minimal_symbol::value_address (this=0x5190000bb698, objfile=0x514000007240) at /home/smarchi/src/binutils-gdb/gdb/symtab.c:517
#7 0x0000555562e5a131 in global_symbol_searcher::expand_symtabs (this=0x7ffff0f5c340, objfile=0x514000007240, preg=std::optional [no contained value])
at /home/smarchi/src/binutils-gdb/gdb/symtab.c:4983
#8 0x0000555562e5d2ed in global_symbol_searcher::search (this=0x7ffff0f5c340) at /home/smarchi/src/binutils-gdb/gdb/symtab.c:5189
#9 0x0000555562e5ffa4 in symtab_symbol_info (quiet=false, exclude_minsyms=false, regexp=0x0, kind=FUNCTION_DOMAIN, t_regexp=0x0, from_tty=1)
at /home/smarchi/src/binutils-gdb/gdb/symtab.c:5361
#10 0x0000555562e6131b in info_functions_command (args=0x0, from_tty=1) at /home/smarchi/src/binutils-gdb/gdb/symtab.c:5525
That is, at this line of `minimal_symbol::value_address`, where
`objfile->section_offsets` is an `std::vector`:
But going up one frame, the section index is 36091:
(top-gdb) frame
#1 0x0000555562426526 in minimal_symbol_reader::record_full (this=0x7ffff0ead560, name="_sinittext", copy_name=false,
address=-2111475712, ms_type=mst_text, section=36091) at /home/smarchi/src/binutils-gdb/gdb/minsyms.c:1228
1228 msymbol->set_section_index (section);
It seems like the problem is just that the type used for the section
index (short) is not big enough. Change from short to int. If somebody
insists, we could even go long long / int64_t, but I doubt it's
necessary.
This leverages commit ("s390: Simplify (dis)assembly of insn operands
with const bits") to relax the operand constraints of the immediate
operand that contains the constant Z- or T-bit of the following extended
mnemonics:
risbgz, risbgnz, risbhgz, risblgz, rnsbgt, rosbgt, rxsbgt
Previously those instructions were the only ones where the assembler
on s390 restricted the specification of the subject I3/I4 operand values
exactly according to their specification to an unsigned 6- or 5-bit
unsigned integer. For any other instructions the assembler allows to
specify any operand value allowed by the instruction format, regardless
of whether the instruction specification is more restrictive.
Allow to specify the subject I3/I4 operand as unsigned 8-bit integer
with the constant operand bits being ORed during assembly.
Relax the instructions subject significant operand bit masks to only
consider the Z/T-bit as significant, so that the instructions get
disassembled as their *z or *t flavor regardless of whether any reserved
bits are set in addition to the Z/T-bit.
Adapt the rnsbg, rosbg, and rxsbg test cases not to inadvertently set
the T-bit in operand I3, as they otherwise get disassembled as their
rnsbgt, rosbgt, and rxsbgt counterpart.
This aligns GNU Assembler to LLVM Assembler.
opcodes/
* s390-opc.c (U6_18, U5_27, U6_26): Remove.
(INSTR_RIE_RRUUU2, INSTR_RIE_RRUUU3, INSTR_RIE_RRUUU4): Define
as INSTR_RIE_RRUUU while retaining insn fmt mask.
(MASK_RIE_RRUUU2, MASK_RIE_RRUUU3, MASK_RIE_RRUUU4): Treat only
Z/T-bit of I3/I4 operand as significant.
gas/testsuite/
* gas/s390/zarch-z10.s (rnsbg, rosbg, rxsbg): Do not set T-bit.
s390: Simplify (dis)assembly of insn operands with const bits
Simplify assembly and disassembly of extended mnemonics with operands
with constant ORed bits:
Their instruction template already contains the respective constant
operand bits, as they are significant to distinguish the extended from
their base mnemonic. Operands are ORed into the instruction template.
Therefore it is not necessary to OR the constant bits into the operand
value during assembly in s390_insert_operand.
Additionally the constant operand bits from the instruction template
can be used to mask them from the operand value during disassembly in
s390_print_insn_with_opcode. For now do so for non-length unsigned
integer operands only.
The separate instruction formats need to be retained, as their masks
differ, which is relevant during disassembly to distinguish the base
and extended mnemonics from each other.
Fixed UBSAN runtime errors such as:
- load of value 4294967295, which is not a valid value for type 'Cmsg_warn'
- null pointer passed as argument 2, which is declared to never be null
- load of value 4294967295, which is not a valid value for type 'ProfData_type'
- reference binding to misaligned address 0x00000357583c for type 'long unsigned int', which requires 8 byte alignment
gprofng/ChangeLog
2024-09-09 Vladimir Mezentsev <vladimir.mezentsev@oracle.com>.
Test-case gdb.dwarf2/forward-spec.exp contains a non-trivial gdb_test_multiple
to parse this cooked_index_entry:
...
[5] ((cooked_index_entry *) 0x7f01f0004040)^M
name: v^M
canonical: v^M
qualified: ns::v^M
DWARF tag: DW_TAG_variable^M
flags: 0x2 [IS_STATIC]^M
DIE offset: 0xcb^M
parent: ((cooked_index_entry *) 0x7f01f00040a0) [ns]^M
...
which allows us to verify that the entry has a parent.
After commit 8f258a6c979 ("[gdb/symtab] Dump qualified name of
cooked_index_entry") that's no longer necessary.
Simplify this by checking for ns::v instead.
While we're at it, also fix the test-case for target boards readnow,
cc-with-gdb-index and cc-with-debug-names.
A previous patch made ld fail early on Thumb-only where branch_type is
ST_BRANCH_UNKNOWN.
However, this fails erroneously when the target is undefweak: in that
case the branch should be replaced by a branch to the next instruction
(or nop.w on thumb2). This patch accepts this case and restores the
previous behaviour in such cases.
This was reported by failures in the GCC testsuite, where we fail to
link executables because __deregister_frame_info is undefweak:
(__deregister_frame_info): Unknown destination type (ARM/Thumb) in ...crtbegin.o
crtbegin.o: in function `__do_global_dtors_aux':
crtstuff.c:(.text+0x52): dangerous relocation: unsupported relocation
Jan Beulich [Wed, 11 Sep 2024 11:56:36 +0000 (13:56 +0200)]
gas: avoid (scrubber) diagnostics for stuff past .end
What's past an active .end directive (when that has its default purpose)
is supposed to be entirely ignored. That should be true not just for
regular processing, but also for "pre-processing" (aka scrubbing). A
complication is that such a directive may of course occur inside a
(false) conditional or a macro definition. To deal with that make sure
we can continue as usual if called another time.
Note however that .end inside a macro will still have the full macro
body expanded; dealing with that would require further (perhaps
intrusive) adjustments in sb_scrub_and_add_sb() and/or callers thereof.
However, at least some of the warnings issued by do_scrub_chars() are
unlikely to occur when expanding a macro. (If we needed to go that far,
presumably .exitm would also want recognizing.)
Jan Beulich [Wed, 11 Sep 2024 11:55:01 +0000 (13:55 +0200)]
arm: don't engage symver scrubber hack in CCS mode
In that mode the comment char is ; while @ has no special meaning.
Engaging the special logic in that case results in comments not being
respected on .symver lines.
Jan Beulich [Wed, 11 Sep 2024 11:52:42 +0000 (13:52 +0200)]
x86/APX: correct disassembly for EVEX.B4
EVEX.B4 is used only for GPR (or addressing of memory) operands. SIMD
registers encoded via ModR/M.rm (when ModR/M.mod == 3) have their top
bit in EVEX.X3. Supposedly (doc version 004) EVEX.B4 is ignored when
unused, hence also don't flag such encodings as invalid.
Mark Harmstone [Mon, 9 Sep 2024 21:01:00 +0000 (22:01 +0100)]
ld/testsuite: exclude relocs from section contributions PDB test
A bug in ld meant that we were erroneously generating image relocations
for .secrel32 ops, which we then reflected in our PDB section
contributions because the linker was adding a .reloc section.
This was incidental to what we were testing for, so pass
--disable-reloc-section to ld in order to ensure a consistent output.
Andrew Burgess [Sat, 10 Aug 2024 11:26:49 +0000 (12:26 +0100)]
gdb/testsuite: add return after a call to 'untested'
In gdb.base/corefile-buildid.exp, in the function
do_corefile_buildid_tests, if we fail to find the build-id for the
test binary then we call 'untested', but then push on with the test,
which inevitably fails as the rest of the test depends on having found
the build-id.
I think we're missing a 'return' after the call to 'untested' which
I've now added.
Also I noticed that we call build_id_debug_filename_get and then
manually remove '.debug' from the end. This is no longer necessary,
we can just ask build_id_debug_filename_get to not add the suffix.
[gdb/testsuite] Handle missing curses in gdb.python/py-missing-debug.exp
Highlighted that in some cases we might be running on a system with an
older version of Python (earlier than 3.7), and on a system for which
the curses library has not been installed.
In these circumstances the gdb.missing_debug module will not load as
it uses curses to provide isalnum() and isascii() functions.
To avoid this problem I propose that we copy the isalnum() and
isascii() from the Python curses library. These functions are
basically trivial and removing the curses dependency means GDB will
work in more cases without increasing its dependencies.
I did consider keeping the uses of curses and only having the function
definitions be a fallback for when the curses library failed to load,
but this felt like overkill. The function definitions are both tiny
and I think "obvious" given their specifications, so I figure we might
as well just use our own definitions if they are not available as
builtin methods on the str class.
For testing I changed this line:
if sys.version_info >= (3, 7):
to
if sys.version_info >= (3, 7) and False:
then reran gdb.python/py-missing-debug.exp, there were no failures.
Tom de Vries [Tue, 10 Sep 2024 08:25:07 +0000 (10:25 +0200)]
[gdb/testsuite] Fix gdb.xml/tdesc-regs.exp on riscv64
When running test-case gdb.xml/tdesc-regs.exp on riscv64-linux, I get:
...
(gdb) set tdesc file single-reg.xml^M
warning: Architecture rejected target-supplied description^M
(gdb) FAIL: gdb.xml/tdesc-regs.exp: set tdesc file single-reg.xml
UNSUPPORTED: gdb.xml/tdesc-regs.exp: register tests
...
The FAIL and UNSUPPORTED are produced here:
...
# If no core registers were specified, assume this target does not
# support target-defined registers. Verify that we get a warning if
# we try to use them. This not only tests the warning, but also
# reminds maintainers to add test support when they add the feature.
if {[string equal ${core-regs} ""]} {
gdb_test "set tdesc file $single_reg_xml" \
"warning: Target-supplied registers are not supported.*" \
"set tdesc file single-reg.xml"
unsupported "register tests"
return 0
}
...
The test-case contains target-specific setting of the core-regs variable, and
adding this for riscv64 bypasses this code and makes the test-case pass.
However, without that change, the test-case shouldn't produce a FAIL since
gdb isn't doing anything wrong.
Fix this by producing instead:
...
PASS: $exp: set tdesc file single-reg.xml
UNSUPPORTED: $exp: register tests (missing architecture-specific core-regs setting)
...
Tom de Vries [Tue, 10 Sep 2024 08:08:29 +0000 (10:08 +0200)]
[gdb/build] Fix unused var in corelow.c
On x86_64-linux, with gcc 7.5.0 and CFLAGS/CXXFLAGS="-O0 -g -Wall" I ran into
a build breaker:
...
gdb/corelow.c: In member function ‘void mapped_file_info::add(const char*, const char*, const char*, std::vector<mem_range>&&, const bfd_build_id*)’:
gdb/corelow.c:1822:27: error: unused variable ‘it’ [-Werror=unused-variable]
const auto [it, inserted]
^
...
H.J. Lu [Mon, 9 Sep 2024 00:27:00 +0000 (17:27 -0700)]
bfd: Pass true to ld_plugin_object_p
Since linker calls bfd_plugin_object_p, which calls ld_plugin_object_p,
only for command-line input objects, pass true to ld_plugin_object_p so
that the same input IR file won't be included twice if the new LTO hook,
LDPT_REGISTER_CLAIM_FILE_HOOK_V2 isn't used.
PR ld/32153
* plugin.c (bfd_plugin_object_p): Pass true to ld_plugin_object_p.
Tom Tromey [Mon, 26 Aug 2024 17:30:01 +0000 (11:30 -0600)]
Move enum size check into ada_identical_enum_types_p
Currently, the callers of ada_identical_enum_types_p must check that
both enum types have the same number of members. In another series
I'm working on, it was convenient to move this check into the callee
instead; and I broke this patch out to make that series a little
simpler.
Tom Tromey [Fri, 6 Sep 2024 17:50:27 +0000 (11:50 -0600)]
Fix some comments in dwarf2/cooked-index.h
This fixes a couple of comments in dwarf2/cooked-index.h.
The comment by cooked_index_entry::canonical mentions C++, but this
field can also be different from 'name' in other situations. Rather
than enumerate the cases here (which doesn't seem important), make the
text a little less specific.
Also, cooked_index_entry::write_scope doesn't document its "for_main"
parameter -- and it is misnamed in the prototype as well.
This changes cooked_index_shard::handle_gnat_encoded_entry to modify
the incoming entry itself, and to return void rather than a new name.
this simplifies the caller a little, which is convenient for a
different series I am working on.
Tom Tromey [Fri, 6 Sep 2024 16:52:54 +0000 (10:52 -0600)]
Ignore DW_TAG_padding in tag_is_type
DW_TAG_padding isn't a real tag -- it doesn't appear in the DWARF
standard, only in include/dwarf2.def as a placeholder. So, remove it
from dwarf2/tag.h:tag_is_type.
s390: Document syntax to omit base register operand
Document the s390-specific assembler syntax introduced by commit aacf780bca29 ("s390: Allow to explicitly omit base register operand in
assembly") to omit the base register operand B in D(X,B) and D(L,B) by
coding D(X,) and D(L,).
While at it document the alternative syntax to omit the index register
operand X in D(X,B) by coding D(,B) instead of D(B).
gas/
* doc/c-s390.texi (s390 Operands): Document syntax to omit base
register operand.
Fixes: aacf780bca29 ("s390: Allow to explicitly omit base register operand in assembly") Signed-off-by: Jens Remus <jremus@linux.ibm.com>
Andrew Burgess [Mon, 9 Sep 2024 10:27:30 +0000 (11:27 +0100)]
gdb/NEWS: group general news items together
I noticed that the list of general NEWS items seemed to have gotten
mixed up a bit in the NEWS file. This commit just moves things around
so that the general items all appear at the start of the 'Changes
since GDB 15' section. I've not changed any of the actual content.
Lulu Cai [Mon, 2 Sep 2024 04:05:54 +0000 (12:05 +0800)]
LoongArch: Fixed precedence of expression operators in instructions
The precedence of the operators "+" and "-" in the current loongarch
instruction expression is higher than "<<" and ">>", which is different
from the explanation in the user guide.
We modified the precedence of "<<" and ">>" to be higher than "+" and "-".
Introduce a use bug where the value of a temporary variable was being
used after it had gone out of scope. This was picked up by the
address sanitizer and would result in this error:
(gdb) maintenance selftest create_breakpoint_parse_arg_string
Running selftest create_breakpoint_parse_arg_string.
=================================================================
==2265825==ERROR: AddressSanitizer: stack-use-after-scope on address 0x7fbb08046511 at pc 0x000001632230 bp 0x7fff7c2fb770 sp 0x7fff7c2fb768
READ of size 1 at 0x7fbb08046511 thread T0
#0 0x163222f in create_breakpoint_parse_arg_string(char const*, std::unique_ptr<char, gdb::xfree_deleter<char> >*, int*, int*, int*, std::unique_ptr<char, gdb::xfree_deleter<char> >*, bool*) ../../src/gdb/break-cond-parse.c:496
#1 0x1633026 in test ../../src/gdb/break-cond-parse.c:582
#2 0x163391b in create_breakpoint_parse_arg_string_tests ../../src/gdb/break-cond-parse.c:649
#3 0x12cfebc in void std::__invoke_impl<void, void (*&)()>(std::__invoke_other, void (*&)()) /usr/include/c++/13/bits/invoke.h:61
#4 0x12cc8ee in std::enable_if<is_invocable_r_v<void, void (*&)()>, void>::type std::__invoke_r<void, void (*&)()>(void (*&)()) /usr/include/c++/13/bits/invoke.h:111
#5 0x12c81e5 in std::_Function_handler<void (), void (*)()>::_M_invoke(std::_Any_data const&) /usr/include/c++/13/bits/std_function.h:290
#6 0x18bb51d in std::function<void ()>::operator()() const /usr/include/c++/13/bits/std_function.h:591
#7 0x4193ef9 in selftests::run_tests(gdb::array_view<char const* const>, bool) ../../src/gdbsupport/selftest.cc:100
#8 0x21c2206 in maintenance_selftest ../../src/gdb/maint.c:1172
... etc ...
The problem was caused by three lines like this one:
After parsing the thread-id TMPTOK would be left pointing into the
temporary string which had been created on this line. When on the
next line we did this:
gdb_assert (*tmptok == '\0');
The value of *TMPTOK is undefined.
Fix this by creating the std::string earlier in the scope. Now the
contents of the string will remain valid when we check *TMPTOK. The
address sanitizer issue is now resolved.
Tom de Vries [Sun, 8 Sep 2024 05:46:09 +0000 (07:46 +0200)]
[gdb/testsuite] Handle missing curses in gdb.python/py-missing-debug.exp
On a system with python 3.6, module gdb.missing_debug imports module curses,
so when running test-case gdb.python/py-missing-debug.exp on a system without
that module installed, we run into:
...
(gdb) source py-missing-debug.py^M
Python Exception <class 'ImportError'>: Module 'curses' is not installed.^M
Use:^M
sudo zypper install python36-curses^M
to install it.^M
Error occurred in Python: Module 'curses' is not installed.^M
Use:^M
sudo zypper install python36-curses^M
to install it.^M
(gdb) FAIL: gdb.python/py-missing-debug.exp: source python script
...
Fix this by issuing UNSUPPORTED instead, and bailing out.
Tested on x86_64-linux.
Approved-by: Kevin Buettner <kevinb@redhat.com>
PR testsuite/31576
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=31576
Andrew Burgess [Fri, 3 Mar 2023 19:03:15 +0000 (19:03 +0000)]
gdb: only insert thread-specific breakpoints in the relevant inferior
This commit updates GDB so that thread or inferior specific
breakpoints are only inserted into the program space in which the
specific thread or inferior is running.
In terms of implementation, getting this basically working is easy
enough, now that a breakpoint's thread or inferior field is setup
prior to GDB looking for locations, we can easily use this information
to find a suitable program_space and pass this to as a filter when
creating the sals.
Or we could if breakpoint_ops::create_sals_from_location_spec allowed
us to pass in a filter program_space.
So, this commit extends breakpoint_ops::create_sals_from_location_spec
to take a program_space argument, and uses this to filter the set of
returned sals. This accounts for about half the change in this patch.
The second set of changes starts from breakpoint_set_thread and
breakpoint_set_inferior, this is called when the thread or inferior
for a breakpoint changes, e.g. from the Python API.
Previously this call would never result in the locations of a
breakpoint changing, after all, locations were inserted in every
program space, and we just use the thread or inferior variable to
decide when we should stop. Now though, changing a breakpoint's
thread or inferior can mean we need to figure out a new set of
breakpoint locations.
To support this I've added a new breakpoint_re_set_one function, which
is like breakpoint_re_set, but takes a single breakpoint, and just
updates the locations for that one breakpoint. We only need to call
this function if the program_space in which a breakpoint's thread (or
inferior) is running actually changes. If the program_space does
change then we call the new breakpoint_re_set_one function passing in
the program_space which should be used to filter the new locations (or
nullptr to indicate we should set locations in all program spaces).
This filter program_space needs to propagate down to all the re_set
methods, this accounts for the remaining half of the changes in this
patch.
There were a couple of existing tests that created thread or inferior
specific breakpoints and then checked the 'info breakpoints' output,
these needed updating. These were:
Before this breakpoint::pspace was set unconditionally.
If we look at how breakpoint::pspace is used today, some breakpoint
types specifically set this field, either in their constructors, or in
a wrapper function that calls the constructor. So, the watchpoint
type and its sub-class set this variable, as does the catchpoint type,
and all it's sub-classes.
However, code_breakpoint doesn't specifically set this field within
its constructor, though some sub-classes of
code_breakpoint (ada_catchpoint, exception_catchpoint,
internal_breakpoint, and momentary_breakpoint) do set this field.
When I examine all the places that breakpoint::pspace is used, I
believe that in every place where it is expected that this field is
set, the breakpoint type will be one that specifically sets this
field.
Next, I observe two problems with the existing code.
First, the above code is only hit for pending breakpoints, there's no
equivalent code for non-pending breakpoints. This opens up the
possibility of GDB entering non-consistent states; if a breakpoint is
first created pending and then later gets a location, the pspace field
will be set, while if the breakpoint is immediately non-pending, then
the pspace field will never be set.
Second, if we look at how breakpoint::pspace is used in the function
breakpoint_program_space_exit, we see that when a program space is
removed, any breakpoint with breakpoint::pspace set to the removed
program space, will be deleted. This makes sense, but does mean we
need to ensure breakpoint::pspace is only set for breakpoints that
apply to a single program space.
So, if I create a pending dprintf breakpoint (type bp_dprintf) then
the breakpoint::pspace variable will be set even though the dprintf is
not really tied to that one program space. As a result, when the
matching program space is removed the dprintf is incorrectly removed.
Also, if I create a thread specific breakpoint, then, thanks to the
'thread != -1' clause the wrong program space will be stored in
breakpoint::pspace (the current program space is always used, which
might not be the program space that corresponds to the selected
thread), as a result, the thread specific breakpoint will be deleted
when the matching program space is removed.
If we look at commit cc72b2a2da6d which added the 'thread != -1'
clause, we can see this change was entirely redundant, the
breakpoint::pspace is also set in bpfinishpy_init after
create_breakpoint has been called. As such, I think we can safely
drop the 'thread != -1' clause.
For the other problems, I'm proposing to be pretty aggressive - I'd
like to drop the breakpoint::pspace assignment completely from
create_breakpoint. Having looked at how this variable is used, I
believe that it is already set elsewhere in all the cases that it is
needed. Maybe this code was needed at one time, but I can't see how
it's needed any more.
There's tests to expose the issues I've spotted with this code, and
there's no regressions in testing.
The initial motivation for this commit was to allow thread or inferior
specific breakpoints to only be inserted within the appropriate
inferior's program-space. The benefit of this is that inferiors for
which the breakpoint does not apply will no longer need to stop, and
then resume, for such breakpoints. This commit does not make this
change, but is a refactor to allow this to happen in a later commit.
The problem we currently have is that when a thread-specific (or
inferior-specific) breakpoint is created, the thread (or inferior)
number is only parsed by calling find_condition_and_thread_for_sals.
This function is only called for non-pending breakpoints, and requires
that we know the locations at which the breakpoint will be placed (for
expression checking in case the breakpoint is also conditional).
A consequence of this is that by the time we figure out the breakpoint
is thread-specific we have already looked up locations in all program
spaces. This feels wasteful -- if we knew the thread-id earlier then
we could reduce the work GDB does by only looking up locations within
the program space for which the breakpoint applies.
Another consequence of how find_condition_and_thread_for_sals is
called is that pending breakpoints don't currently know they are
thread-specific, nor even that they are conditional! Additionally, by
delaying parsing the thread-id, pending breakpoints can be created for
non-existent threads, this is different to how non-pending
breakpoints are handled, so I can do this:
$ gdb -q ./gdb/testsuite/outputs/gdb.multi/pending-bp/pending-bp
Reading symbols from ./gdb/testsuite/outputs/gdb.multi/pending-bp/pending-bp...
(gdb) break foo thread 99
Function "foo" not defined.
Make breakpoint pending on future shared library load? (y or [n]) y
Breakpoint 1 (foo thread 99) pending.
(gdb) r
Starting program: /tmp/gdb/testsuite/outputs/gdb.multi/pending-bp/pending-bp
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/lib64/libthread_db.so.1".
Error in re-setting breakpoint 1: Unknown thread 99.
[Inferior 1 (process 3329749) exited normally]
(gdb)
GDB only checked the validity of 'thread 99' at the point the 'foo'
location became non-pending. In contrast, if I try this:
$ gdb -q ./gdb/testsuite/outputs/gdb.multi/pending-bp/pending-bp
Reading symbols from ./gdb/testsuite/outputs/gdb.multi/pending-bp/pending-bp...
(gdb) break main thread 99
Unknown thread 99.
(gdb)
GDB immediately checks if 'thread 99' exists. I think inconsistencies
like this are confusing, and should be fixed if possible.
In this commit the create_breakpoint function is updated so that the
extra_string, which contains the thread, inferior, task, and/or
condition information, is parsed immediately, even for pending
breakpoints.
Obviously, the condition still can't be validated until the breakpoint
becomes non-pending, but the thread, inferior, and task information
can be pulled from the extra-string, and can be validated early on,
even for pending breakpoints. The -force-condition flag is also
parsed as part of this early parsing change.
There are a couple of benefits to doing this:
1. Printing of breakpoints is more consistent now. Consider creating
a conditional breakpoint before this commit:
(gdb) set breakpoint pending on
(gdb) break pendingfunc if (0)
Function "pendingfunc" not defined.
Breakpoint 1 (pendingfunc if (0)) pending.
(gdb) break main if (0)
Breakpoint 2 at 0x401198: file /tmp/hello.c, line 18.
(gdb) info breakpoints
Num Type Disp Enb Address What
1 breakpoint keep y <PENDING> pendingfunc if (0)
2 breakpoint keep y 0x0000000000401198 in main at /tmp/hello.c:18
stop only if (0)
(gdb)
And after this commit:
(gdb) set breakpoint pending on
(gdb) break pendingfunc if (0)
Function "pendingfunc" not defined.
Breakpoint 1 (pendingfunc) pending.
(gdb) break main if (0)
Breakpoint 2 at 0x401198: file /home/andrew/tmp/hello.c, line 18.
(gdb) info breakpoints
Num Type Disp Enb Address What
1 breakpoint keep y <PENDING> pendingfunc
stop only if (0)
2 breakpoint keep y 0x0000000000401198 in main at /home/andrew/tmp/hello.c:18
stop only if (0)
(gdb)
Notice that the display of the condition is now the same for the
pending and non-pending breakpoints.
The same is true for the thread, inferior, or task information in
thread, inferior, or task specific breakpoints; this information is
displayed on its own line rather than being part of the 'What'
field.
2. We can check that the thread exists as soon as the pending
breakpoint is created. Currently there is a weird difference
between pending and non-pending breakpoints when creating a
thread-specific breakpoint.
A pending thread-specific breakpoint only checks its thread when it
becomes non-pending, at which point the thread the breakpoint was
intended for might have exited. Here's the behaviour before this
commit:
(gdb) set breakpoint pending on
(gdb) break foo thread 2
Function "foo" not defined.
Breakpoint 2 (foo thread 2) pending.
(gdb) c
Continuing.
[Thread 0x7ffff7c56700 (LWP 2948835) exited]
Error in re-setting breakpoint 2: Unknown thread 2.
[Inferior 1 (process 2948832) exited normally]
(gdb)
Notice the 'Error in re-setting breakpoint 2: Unknown thread 2.'
line, this was triggered when GDB tried to make the breakpoint
non-pending, and GDB discovers that the thread no longer exists.
Compare that to the behaviour after this commit:
(gdb) set breakpoint pending on
(gdb) break foo thread 2
Function "foo" not defined.
Breakpoint 2 (foo) pending.
(gdb) c
Continuing.
[Thread 0x7ffff7c56700 (LWP 2949243) exited]
Thread-specific breakpoint 2 deleted - thread 2 no longer in the thread list.
[Inferior 1 (process 2949240) exited normally]
(gdb)
Now the behaviour for pending breakpoints is identical to
non-pending breakpoints, the thread specific breakpoint is removed
as soon as the thread the breakpoint is associated with exits.
There is an additional change; when the pending breakpoint is
created prior to this patch we see this line:
Breakpoint 2 (foo thread 2) pending.
While after this patch we get this line:
Breakpoint 2 (foo) pending.
Notice that 'thread 2' has disappeared. This might look like a
regression, but I don't think it is. That we said 'thread 2'
before was just a consequence of the lazy parsing of the breakpoint
specification, while with this patch GDB understands, and has
parsed away the 'thread 2' bit of the spec. If folk think the old
information was useful then this would be trivial to add back in
code_breakpoint::say_where.
As a result of this commit the breakpoints 'extra_string' field is now
only used by bp_dprintf type breakpoints to hold the printf format and
arguments. This string should always be empty for other breakpoint
types. This allows some cleanup in print_breakpoint_location.
In code_breakpoint::code_breakpoint I've changed an error case into an
assert. This is because the error is now handled earlier in
create_breakpoint. As a result we know that by this point, the
extra_string will always be nullptr for anything other than a
bp_dprintf style breakpoint.
The find_condition_and_thread_for_sals function is now no longer
needed, this was previously doing the delayed splitting of the extra
string into thread, task, and condition, but this is now all done in
create_breakpoint, so find_condition_and_thread_for_sals can be
deleted, and the code that calls this in
code_breakpoint::location_spec_to_sals can be removed. With this
update this code would only ever be reached for bp_dprintf style
breakpoints, and in these cases the extra_string should not contain
anything other than format and args.
The most interesting changes are all in create_breakpoint and in the
new file break-cond-parse.c. We have a new block of code early on in
create_breakpoint that is responsible for splitting the extra_string
into its component parts by calling create_breakpoint_parse_arg_string
a function in the new break-cond-parse.c file. This means that some
of the later code can be simplified a little.
The new break-cond-parse.c file implements the splitting up the
extra_string and finding all the parts, as well as some self-tests for
the new function.
Finally, now we know all the breakpoint details, these can be stored
within the breakpoint object if we end up creating a deferred
breakpoint. Additionally, if we are creating a deferred bp_dprintf we
can parse the extra_string to build the printf command.
The implementation here aims to maintain backwards compatibility as
much as possible, this means that:
1. We support abbreviations of 'thread', 'task', and 'inferior' in
some places on the breakpoint line. The handling of abbreviations
has (before this patch) been a little weird, so this works:
(gdb) break *main th 1
And creates a breakpoint at '*main' for thread 1 only, while this
does not work:
(gdb) break main th 1
In this case GDB will try to find the symbol 'main th 1'. This
weirdness exists before and after this patch.
2. The handling of '-force-condition' is odd, if this flag appears
immediately after a condition then it will be treated as part of the
condition, e.g.:
(gdb) break main if 0 -force-condition
No symbol "force" in current context.
But we are fine with these alternatives:
(gdb) break main if 0 thread 1 -force-condition
(gdb) break main -force-condition if 0
Again, this is just a quirk of how the breakpoint line used to be
parsed, but I've maintained this for backward compatibility. During
review it was suggested that -force-condition should become an
actual breakpoint flag (i.e. only valid after the 'break' command
but before the function name), and I don't think that would be a
terrible idea, however, that's not currently a trivial change, and I
think should be done as a separate piece of work. For now, this
patch just maintains the current behaviour.
The implementation works by first splitting the breakpoint condition
string (everything after the location specification) into a list of
tokens, each token has a type and a value. (e.g. we have a THREAD
token where the value is the thread-id string). The list of tokens is
validated, and in some cases, tokens are merged. Then the values are
extracted from the remaining token list.
Consider this breakpoint command:
(gdb) break main thread 1 if argc == 2
The condition string passed to create_breakpoint_parse_arg_string is
going to be 'thread 1 if argc == 2', which is then split into the
tokens:
{ THREAD: "1" } { CONDITION: "argc == 2" }
The thread-id (1) and the condition string 'argc == 2' are extracted
from these tokens and returns back to create_breakpoint.
Now consider this breakpoint command:
(gdb) break some_function if ( some_var == thread )
Here the user wants a breakpoint if 'some_var' is equal to the
variable 'thread'. However, when this is initially parsed we will
find these tokens:
{ CONDITION: "( some_var == " } { THREAD: ")" }
This is a consequence of how we have to try and figure out the
contents of the 'if' condition without actually parsing the
expression; parsing the expression requires that we know the location
in order to lookup the variables by name, and this can't be done for
pending breakpoints (their location isn't known yet), and one of the
points of this work is that we extract things like thread-id for
pending breakpoints.
And so, it is in this case that token merging takes place. We check
if the value of a token appearing immediately after the CONDITION
token looks valid. In this case, does ')' look like a valid
thread-id. Clearly, in this case ')' does not, and so me merge the
THREAD token into the condition token, giving:
{ CONDITION: "( some_var == thread )" }
Which is what we want.
I'm sure that we might still be able to come up with some edge cases
where the parser makes the wrong choice. I think long term the best
way to work around these would be to move the thread, inferior, task,
and -force-condition flags to be "real" command options for the break
command. I am looking into doing this, but can't guarantee if/when
that work would be completed, so this patch should be reviewed assume
that the work will never arrive (though I hope it will).
Andrew Burgess [Thu, 28 Dec 2023 22:59:43 +0000 (22:59 +0000)]
gdb: create new is_thread_id helper function
This is a refactoring commit that splits the existing parse_thread_id
function into two parts, and then adds a new is_thread_id function.
The core of parse_thread_id is split into parse_thread_id_1, which is
responsible for actually parsing a thread-id. Then parse_thread_id is
responsible for taking a parsed thread-id and validating that it
references an actually existing inferior thread.
The new is_thread_id function also uses parse_thread_id_1, but doesn't
actually check that the inferior thread exists, instead, this new
function simply checks that a string looks like a thread-id.
This commit does not add a use for is_thread_id, this will be added in
the next commit.
This is a refactoring commit, there should be no user visible changes
after this commit.
Andrew Burgess [Fri, 29 Dec 2023 09:00:51 +0000 (09:00 +0000)]
gdb: add another overload of startswith
We already have one overload of the startswith function that takes a
std::string_view for both arguments. A later patch in this series is
going to be improved by having an overload that takes one argument as
a std::string_view and the other argument as a plain 'char *'.
This commit adds the new overload, but doesn't make use of it (yet).
There should be no user visible changes after this commit.
Tom Tromey [Sat, 29 Jun 2024 16:07:20 +0000 (10:07 -0600)]
Remove tui_wrefresh
This removes tui_wrefresh, moving the code into refresh_window. We
remove tui_norefresh_window as well, because now the command window's
refresh_window has to do what tui_wrefresh previously did.
projects / thirdparty / binutils-gdb.git / log
