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.
Tom Tromey [Sun, 17 Dec 2023 19:52:33 +0000 (12:52 -0700)]
Rename tui_suppress_output
This patch renames tui_suppress_output to the more descriptive
tui_batch_rendering. This code was never really correct, and was
based on a misunderstanding of the curses API. The updated comments
describe the intended use of this class.
This also removes the erroneous tui_win_info::no_refresh.
wnoutrefresh does not prevent any output; rather, it copies from one
curses buffer to another but (unlike woutrefresh) without then
flushing to the screen.
tui_batch_rendering now works in the correct way: calling doupdate in
the destructor of the outermost instance, thus batching all screen
output until that point.
The patch adds instantiations of tui_batch_rendering to various spots,
to make sure it is active when refreshing.
Tom Tromey [Fri, 31 May 2024 20:41:02 +0000 (14:41 -0600)]
Clean up refreshing in TUI register window
This patch rearranges the TUI register window code a bit, removing a
call to tui_wrefresh and hoisting the calls to refresh_window to "more
outer" spots.
Reviewed-By: Alexandra Petlanova Hajkova <ahajkova@redhat.com>
Andrew Burgess [Wed, 19 Jun 2024 10:14:08 +0000 (11:14 +0100)]
gdb: 'target ...' commands now expect quoted/escaped filenames
This commit changes the 'target ...' commands that accept a filename
to take a quoted or escaped filename rather than a literal filename.
What this means in practice is that if you are specifying a filename
that contains no white space or quote characters, then nothing should
change, e.g.:
target exec /path/to/some/file
works both before and after this commit.
However, if a user wishes to specify a file containing white space
then either the entire filename needs to be quoted, or the special
white space needs to be escaped. Before this patch a user could
write:
target exec /path/to a file/containing spaces
But after this commit the user would have to choose one of:
target exec "/path/to a file/containing spaces"
or
target exec /path/to\ a\ file/containing\ spaces
Obviously this is a potentially breaking change. The benefit of
making this change is consistency. Commands that take multiple
arguments (one of which is a filename) or in the future, commands that
take filename options, will always need to use quoted/escaped
filenames, so converting all unquoted filename commands to use quoting
or escaping makes the UI more consistent.
Additionally (though this is probably not a common problem), GDB
strips trailing white space from commands that the user enters. As
such it is not possible to reference any file that ends in white space
unless the quoting / escaping style is used. Though I suspect very
few users run into this problem!
The downside obviously is that this is a UI breaking change.
Andrew Burgess [Thu, 20 Jun 2024 12:44:28 +0000 (13:44 +0100)]
gdb: allow quoted filenames for commands that have custom completion
This commit changes how GDB processes command arguments for the
following commands:
compile file
maint print c-tdesc
save gdb-index
After this commit these commands will now expect their single filename
argument to be (optionally) quoted if it contains any special
characters (e.g. whit space or quotes).
If the filename does not contain any special characters then nothing
changes. As an example:
(gdb) save gdb-index /path/to/some/directory/
will work before and after this patch. However, if the directory
name contains a white space then before this patch a user would write:
(gdb) save gdb-index /path/to some/directory/
But this will now fail as GDB will consider this as two arguments,
'/path/to' and 'some/directory/'. To pass this single directory name
a user must now do one of these:
(gdb) save gdb-index "/path/to some/directory/"
(gdb) save gdb-index '/path/to some/directory/'
(gdb) save gdb-index /path/to\ some/directory/
This brings these commands into line with commands like 'file' and
'symbol-file', which have supported quoted filenames for a while.
The motivation for this change is to make handling of filename
arguments consistent throughout GDB. We can't move to all commands
taking non-quoted filenames as the non-quoted style only allows for a
single argument. Additionally, the non-quoted style doesn't allow for
filenames that end in white space (though this is probably pretty
rare). So, if we want to have consistency the only choice is to move
towards supporting quote filenames.
Andrew Burgess [Sat, 22 Jun 2024 10:39:38 +0000 (11:39 +0100)]
gdb: add remove-symbol-file command completion
The 'remove-symbol-file' command doesn't currently offer command
completion. This commit addresses this.
The 'remove-symbol-file' uses gdb_argv to split its command arguments,
this means that the filename the command expects can be quoted.
However, the 'remove-symbol-file' command is a little weird in that it
also has a '-a' option, if this option is passed then the command
expects not a filename, but an address.
Currently the remove_symbol_file_command function splits the command
args using gdb_argv, checks for a '-a' flag by looking at the first
argument value, and then expects the filename or address to occupy a
single entry in the gdb_argv array.
The first thing I do is handle the '-a' flag using GDB's option
system. I model this option as a flag_option_def (a boolean option).
I've dropped the use of gdb_argv and instead use the new(ish) function
extract_single_filename_arg, which was added a couple of commits back,
to parse the filename argument (when '-a' is not given).
If '-a' is given the the remove-symbol-file command expects an address
rather than a filename. As we previously split the arguments using
gdb_argv this meant the address needed to appear as a single
argument. So a user could write:
(gdb) remove-symbol-file 0x1234
Or they could write:
(gdb) remove-symbol-file some_function
Both of these would work fine. But a user could not write:
(gdb) remove-symbol-file some_function + 0x1000
As only the 'some_function' part would be processed. Now the user
could do this:
(gdb) remove-symbol-file "some_function + 0x1000"
By enclosing the address expression in quotes this would be handled as
a single argument. However, this is a little weird, that's not how
commands like 'print' or 'x' work. Also this functionality was
neither documented, or tested.
And so, in this commit, by removing the use of gdb_argv I bring the
'remove-symbol-file' command inline with GDB's other commands that
take an expression, the quotes are no longer needed.
Usually in a completer we call 'complete_options', but don't actually
capture the option values. But for remove-symbol-file I do. This
allows me to spot when the '-a' option has been given, I can then
complete the rest of the command line as either a filename or an
expression.
Andrew Burgess [Tue, 2 Jul 2024 08:18:55 +0000 (09:18 +0100)]
gdb: extend completion of quoted filenames to work in brkchars phase
Up to this point filename completion for possibly quoted filenames has
always been handled during the second (non-brkchars) phase of
completion. This works fine for commands that only want to complete
on a single filename argument.
In a later commit though I need to perform completion of a quoted
filename argument during the first (brkchars) phase of completion.
This will allow me to add a custom completer that completes both
command options and arguments for a command (remove-symbol-file) that
takes a possibly quoted filename argument.
This commit doesn't add the remove-symbol-file completer, this commit
is just about putting support for that in place.
To achieve this I've added the new function
advance_to_filename_maybe_quoted_complete_word_point, which is unused
in this commit. I've then had to extend some other functions in order
to extract the quoting state during the brkchars phase.
As this commit doesn't use the new functionality, the important thing
at this point is that I've not regressed the existing filename
completion (or any of the other completion). The next commit in this
series will make use of the new functionality, and will include
tests.
There should be no user visible changes after this commit.
Andrew Burgess [Wed, 19 Jun 2024 10:13:46 +0000 (11:13 +0100)]
gdb: new extract_single_filename_arg helper function
This commit is in preparation for the next few commits, this commit
adds a new function extract_single_filename_arg.
This new function will be used to convert GDB commands that expect a
single filename argument to have these commands take a possibly quoted
or escaped string.
There's no use of the new function in this commit, for that see the
following commits.
Implement the readline rl_directory_rewrite_hook callback function,
this is used when readline needs to offer completions from within a
directory. The important thing is that this function should remove
any escaping, this allows GDB to correctly offer completions in
situations like this:
Note the escaping in 'directory\ with\ spaces'. Without the
rl_directory_rewrite_hook callback readline will try to open a
directory literally called '/tmp/directory\ with\ spaces' which
obviously doesn't exist.
There are tests added to cover this new functionality.
Andrew Burgess [Sat, 24 Feb 2024 20:23:18 +0000 (20:23 +0000)]
gdb: improve gdb_rl_find_completion_word for quoted words
The function gdb_rl_find_completion_word is very similar to the
readline function _rl_find_completion_word, but was either an older
version of that function, or was trimmed when copying to remove code
which was considered unnecessary.
We maintain this copy because the _rl_find_completion_word function is
not part of the public readline API, and we need to replicate the
functionality of that function as part of the 'complete' command.
Within gdb_rl_find_completion_word when looking for the completion
word, if we don't find a unclosed quoted string (which would become
the completion word) then we scan backwards looking for a word break
character. For example, given:
(gdb) complete file /tmp/foo
There is no unclosed quoted string so we end up scanning backwards
from the end looking for a word break character. In this case the
space after 'file' and before '/tmp/foo' is found, so '/tmp/foo'
becomes the completion word.
However, given this:
(gdb) complete file /tmp/foo\"
There is still no unclosed quoted string, however, when we can
backwards the '"' (double quotes) are treated as a word break
character, and so we end up using the empty string as the completion
word.
The readline function _rl_find_completion_word avoids this mistake by
using the rl_char_is_quoted_p hook. This function will return true
for the double quote character as it is preceded by a backslash. An
earlier commit in this series supplied a rl_char_is_quoted_p function
for the filename completion case, however, gdb_rl_find_completion_word
doesn't call rl_char_is_quoted_p so this doesn't help for the
'complete' case.
In this commit I've copied the code to call rl_char_is_quoted_p from
_rl_find_completion_word into gdb_rl_find_completion_word.
This half solves the problem. In the case:
(gdb) complete file /tmp/foo\"
We do now try to complete on the string '/tmp/foo\"', however, when we
reach filename_completer we call back into readline to actually
perform filename completion. However, at this point the WORD variable
points to a string that still contains the backslash. The backslash
isn't part of the actual filename, that's just an escape character.
Our expectation is that readline will remove the backslash when
looking for matching filenames. However, readline contains an
optimisation to avoid unnecessary work trying to remove escape
characters.
The readline variable rl_completion_found_quote is set in the readline
function gen_completion_matches before the generation of completion
matches. This variable is set to true (non-zero) if there is (or
might be) escape characters within the completion word.
The function rl_filename_completion_function, which generates the
filename matches, only removes escape characters when
rl_completion_found_quote is true. When GDB generates completions
through readline (e.g. tab completion) then rl_completion_found_quote
is set correctly.
But when we use the 'complete' command we don't pass through readline,
and so gen_completion_matches is never called and
rl_completion_found_quote is not set. In this case when we call
rl_filename_completion_function readline doesn't remove the escapes
from the completion word, and so in our case above, readline looks for
completions of the exact filename '/tmp/foo\"', that is, the filename
including the backslash.
To work around this problem I've added a new flag to our function
gdb_rl_find_completion_word which is set true when we find any quoting
or escaping. This matches what readline does.
Then in the 'complete' function we can set rl_completion_found_quote
prior to generating completion matches.
With this done the 'complete' command now works correctly when trying
to complete filenames that contain escaped word break characters. The
tests have been updated accordingly.
Andrew Burgess [Fri, 23 Feb 2024 11:05:58 +0000 (11:05 +0000)]
gdb: apply escaping to filenames in 'complete' results
Building on the mechanism added in the previous commit(s), this commit
applies escaping to filenames in the 'complete' command output.
Consider a file: /tmp/xxx/aa"bb -- that is a filename that contains a
double quote, currently the 'complete' command output looks like this:
Notice that the double quote in the output is not escaped. If we
passed this same output back to GDB then the double quote will be
treated as the start of a string.
After this commit then the output looks like this:
The double quote is now escaped. If we feed this output back to GDB
then GDB will treat this as a single filename that contains a double
quote, exactly what we want.
To achieve this I've done a little refactoring, splitting out the core
of gdb_completer_file_name_quote, and then added a new call from the
filename_match_formatter function.
There are updates to the tests to cover this new functionality.
Andrew Burgess [Thu, 22 Feb 2024 14:56:43 +0000 (14:56 +0000)]
gdb: add match formatter mechanism for 'complete' command output
This commit solves a problem that existed prior to the previous
commit, but the previous commit made more common.
When completing a filename with the 'complete' command GDB will always
add a trailing quote character, even if the completion is a directory
name, in which case it would be better if the trailing quote was not
added. Consider:
(gdb) complete file '/tmp/xx
file '/tmp/xxx/'
The completion offered here is really only a partial completion, we've
completed up to the end of the next directory name, but, until we have
a filename then the completion is not finished and the trailing quote
should not be added.
This would match the readline behaviour, e.g.:
(gdb) file '/tmp/xx<TAB>
(gdb) file '/tmp/xxx/
In this case readline completes the directory name, but doesn't add
the trailing quote character.
Remember that the 'complete' command is intended for tools like
e.g. emacs in order that they can emulate GDB's standard readline
completion when implementing a CLI of their own. As such, not adding
the trailing quote in this case matches the readline behaviour, and
seems like the right way to go.
To achieve this, I've added a new function pointer member variable
completion_result::m_match_formatter. This contains a pointer to a
callback function which is used by the 'complete' command to format
each result.
The default behaviour of this callback function is to just append the
quote character (the character from before the completion string) to
the end of the completion result. This matches the current behaviour.
However, for filename completion we override the default value of
m_match_formatter, this new function checks if the completion result
is a directory or not. If the completion result is a directory then
the closing quote is not added, instead we add a trailing '/'
character.
The code to add a trailing '/' character already exists within the
filename_completer function. This is no longer needed in this
location, instead this code is moved into the formatter callback.
Tests are updated to handle the changes in functionality, this removes
an xfail added in the previous commit.
Andrew Burgess [Wed, 21 Feb 2024 16:03:17 +0000 (16:03 +0000)]
gdb: simplify completion_result::print_matches
Simplify completion_result::print_matches by removing one of the code
paths. Now, every time we call ::print_matches we always add the
trailing quote.
Previously, when using the 'complete' command, if there was only one
result then trailing quote was added in ::build_completion_result, but
when we had multiple results the trailing quote was added in
::print_matches. As a consequence, ::print_matches had to understand
not to add the trailing quote for the single result case.
After this commit we don't add the trailing quote in
::build_completion_result, instead ::print_matches always adds the
trailing quote, which makes ::print_matches simpler.
However, there is a slight problem. When completion is being driven
by readline, and not by the 'complete' command, we still need to
manually add the trailing quote in the single result case, and as the
printing is done by readline we can't add the quote at the time of
printing, and so, in ::build_completion_result, we still add the
trailing quote, but only when completion is being done for readline.
And this does cause a small problem. When completing a filename, if
the completion results in a directory name then, when using the
'complete' command, GDB should not be adding a trailing quote. For
example, if we have the file /tmp/xxx/foo.c, then what we should see
is this:
(gdb) complete file '/tmp/xx
file 'tmp/xxx/
But what we actually see after this commit is this:
(gdb) complete file '/tmp/xx
file 'tmp/xxx/'
Previously we didn't get the trailing quote in this case, as when
there is only a single result, the quote was added in
::build_completion_result, and for filename completion, GDB didn't
know what the quote character was in ::build_completion_result, so no
quote was added. Now that the trailing quote is always added in
::print_matches, and GDB does know the quote character at this point,
so we are now getting the trailing quote, which is not correct.
This is a regression, but really, GDB is now broken in a consistent
way, if we create the file /tmp/xxa/bar.c, then previously if we did
this:
Notice how we get the trailing quote in this case, this is the before
patch behaviour, and is also wrong.
A later commit will fix things so that the trailing quote is not added
in this filename completion case, but for now I'm going to accept this
small regression.
This change in behaviour caused some failures in one of the completion
tests, I've tweaked the test case to expect the trailing quote as part
of this commit, but will revert this in a later commit in this series.
I've also added an extra test for when the 'complete' command does
complete to a single complete filename, in which case the trailing
quote is expected.
Andrew Burgess [Wed, 21 Feb 2024 11:28:31 +0000 (11:28 +0000)]
gdb: move display of completion results into completion_result class
This commit moves the printing of the 'complete' command results out
of the 'complete_command' function. The printing is now done in a new
member function 'completion_result::print_matches'. At this point,
this is entirely a refactor.
The motivation for this refactor is how 'complete' should print the
completion of filename arguments. In some cases the filename results
need to have escaping added to the output. This escaping needs to be
done immediately prior to printing the result as adding too early will
result in multiple 'complete' results potentially being sorted
incorrectly. See the subsequent commits for more details.
There should be no user visible changes after this commit.
Andrew Burgess [Tue, 20 Feb 2024 18:20:19 +0000 (18:20 +0000)]
gdb: improve escaping when completing filenames
This improves quoting and escaping when completing filenames for
commands that allow filenames to be quoted and escaped.
I've struggled a bit trying to split this series into chunks. There's
a lot of dependencies between different parts of the completion
system, and trying to get this working correctly is pretty messy.
This first step is really about implementing 3 readline hooks:
rl_char_is_quoted_p
- Is a particular character quoted within readline's input buffer?
rl_filename_dequoting_function
- Remove quoting characters from a filename.
rl_filename_quoting_function
- Add quoting characters to a filename.
See 'info readline' for full details, but with these hooks connected
up, readline (on behalf of GDB) should do a better job inserting
backslash escapes when completing filenames.
There's still a bunch of stuff that doesn't work after this commit,
mostly around the 'complete' command which of course doesn't go
through readline, so doesn't benefit from all of these new functions
yet, I'll add some of this in a later commit.
Tab completion is now slightly improved though, it is possible to
tab-complete a filename that includes a double or single quote, either
in an unquoted string or within a string surrounded by single or
double quotes, backslash escaping is used when necessary.
There are some additional tests to cover the new functionality.
Andrew Burgess [Sat, 22 Jun 2024 08:37:21 +0000 (09:37 +0100)]
gdb: deprecated filename_completer and associated functions
Following on from the previous commit, this commit marks the old
unquoted filename completion related functions as deprecated.
The aim of doing this is to make it more obvious to someone adding a
new command that they should not be using the older unquoted style
filename argument handling.
I split this change from the previous to make for an easier review.
This commit touches more files, but is _just_ function renaming.
Check out gdb/completer.{c,h} for what has been renamed. All the
other files have just been updated to use the new names.
There should be no user visible changes after this commit.
Andrew Burgess [Sat, 22 Jun 2024 09:10:29 +0000 (10:10 +0100)]
gdb: split apart two different types of filename completion
Unfortunately we have two different types of filename completion in
GDB.
The majority of commands have what I call unquoted filename
completion, this is for commands like 'set logging file ...', 'target
core ...', and 'add-auto-load-safe-path ...'. For these commands
everything after the command name (that is not a command option) is
treated as a single filename. If the filename contains white space
then this does not need to be escaped, nor does the filename need to
be quoted. In fact, the filename argument is not de-quoted, and does
not have any escaping removed, so if a user does try to add such
things, they will be treated as part of the filename. As an example:
(gdb) target core "/path/that contains/some white space"
Will look for a directory calls '"' (double quotes) in the local
directory.
A small number of commands do de-quote and remove escapes from
filename arguments. These command accept what I call quoted and
escaped filenames. Right now these are the commands that specify the
file for GDB to debug, so:
Current filename completion always assumes that filenames can be
quoted, though escaping doesn't work in completion right now. But the
assumption that quoting is allowed is clearly wrong.
This commit splits filename completion into two. The existing
filename_completer is retained, and is used for unquoted filenames. A
second filename_maybe_quoted_completer is added which can be used for
completing quoted filenames.
The filename completion test has been extended to cover more cases.
As part of the extended testing I need to know the character that
should be used to separate filenames within a path. For this TCL 8.6+
has $::tcl_platform(pathSeparator). To support older versions of TCL
I've added some code to testsuite/lib/gdb.exp.
You might notice that after this commit the completion for unquoted
files is all done in the brkchars phase, that is the function
filename_completer_handle_brkchars calculates the completions and
marks the completion_tracker as using a custom word point. The reason
for this is that we don't want to break on white space for this
completion, but if we rely on readline to find the completion word,
readline will consider the entire command line, and with no white
space in the word break character set, readline will end up using the
entire command line as the word to complete.
For now at least, the completer for quoted filenames does generate its
completions during the completion phase, though this is going to
change in a later commit.
Andrew Burgess [Mon, 6 May 2024 18:01:40 +0000 (19:01 +0100)]
gdb: unify build-id to objfile lookup code
There are 3 places where we currently call debuginfod_exec_query to
lookup an objfile for a given build-id.
In one of these places we first call build_id_to_exec_bfd which also
looks up an objfile given a build-id, but this function looks on disk
for a symlink in the .build-id/ sub-directory (within the
debug-file-directory).
I can't think of any reason why we shouldn't call build_id_to_exec_bfd
before every call to debuginfod_exec_query.
So, in this commit I have added a new function in build-id.c,
find_objfile_by_build_id, this function calls build_id_to_exec_bfd,
and if that fails, then calls debuginfod_exec_query.
Everywhere we call debuginfod_exec_query is updated to call the new
function, and in locate_exec_from_corefile_build_id, the existing call
to build_id_to_exec_bfd is removed as calling find_objfile_by_build_id
does this for us.
One slight weird thing is in core_target::build_file_mappings, here we
call find_objfile_by_build_id which returns a gdb_bfd_ref_ptr for the
opened file, however we immediately reopen the file as "binary". The
reason for this is that all the bfds opened in ::build_file_mappings
need to be opened as "binary" (see the function comments for why).
I did consider passing a target type into find_objfile_by_build_id,
which could then be forwarded to build_id_to_exec_bfd and used to open
the BFD as "binary", however, if you follow the call chain you'll end
up in build_id_to_debug_bfd_1, where we actually open the bfd. Notice
in here that we call build_id_verify to double check the build-id of
the file we found, this requires that the bfd not be opened as
"binary".
What this means is that we always have to first open the bfd using the
gnutarget target type (for the build-id check), and then we would have
to reopen it as "binary". There seems little point pushing the reopen
logic into find_objfile_by_build_id, so we just do this in the
::build_file_mappings function.
I've extended the tests to cover the two cases which actually changed
in this commit.
Andrew Burgess [Tue, 30 Apr 2024 13:21:47 +0000 (14:21 +0100)]
gdb: improve shared library build-id check for core-files
When GDB opens a core file, in 'core_target::build_file_mappings ()',
we collection information about the files that are mapped into the
core file, specifically, the build-id and the DT_SONAME attribute for
the file, which will be set for some shared libraries.
We then cache the DT_SONAME to build-id information on the core file
bfd object in the function set_cbfd_soname_build_id.
Later, when we are loading the shared libraries for the core file, we
can use the library's file name to look in the DT_SONAME to build-id
map, and, if we find a matching entry, we can use the build-id to
validate that we are loading the correct shared library.
This works OK, but has some limitations: not every shared library will
have a DT_SONAME attribute. Though it is good practice to add such an
attribute, it's not required. A library without this attribute will
not have its build-id checked, which can lead to GDB loading the wrong
shared library.
What I want to do in this commit is to improve GDB's ability to use
the build-ids extracted in core_target::build_file_mappings to both
validate the shared libraries being loaded, and then to use these
build-ids to potentially find (via debuginfod) the shared library.
To do this I propose making the following changes to GDB:
(1) Rather than just recording the DT_SONAME to build-id mapping in
set_cbfd_soname_build_id, we should also record, the full filename to
build-id mapping, and also the memory ranges to build-id mapping for
every memory range covered by every mapped file.
(2) Add a new callback solib_ops::find_solib_addr. This callback
takes a solib object and returns an (optional) address within the
inferior that is part of this library. We can use this address to
find a mapped file using the stored memory ranges which will increase
the cases in which a match can be found.
(3) Move the mapped file record keeping out of solib.c and into
corelow.c. Future commits will make use of this information from
other parts of GDB. This information was never solib specific, it
lived in the solib.c file because that was the only user of the data,
but really, the data is all about the core file, and should be stored
in core_target, other parts of GDB can then query this data as needed.
Now, when we load a shared library for a core file, we do the
following lookups:
1. Is the exact filename of the shared library found in the filename
to build-id map? If so then use this build-id for validation.
2. Find an address within the shared library using ::find_solib_addr
and then look for an entry in the mapped address to build-id map.
If an entry is found then use this build-id.
3. Finally, look in the soname to build-id map. If an entry is
found then use this build-id.
The addition of step #2 here means that GDB is now far more likely to
find a suitable build-id for a shared library. Having acquired a
build-id the existing code for using debuginfod to lookup a shared
library object can trigger more often.
On top of this, we also create a build-id to filename map. This is
useful as often a shared library is implemented as a symbolic link to
the actual shared library file. The mapped file information is stored
based on the actual, real file name, while the shared library
information holds the original symbolic link file name.
If when loading the shared library, we find the symbolic link has
disappeared, we can use the build-id to file name map to check if the
actual file is still around, if it is (and if the build-id matches)
then we can fall back to use that file. This is another way in which
we can slightly increase the chances that GDB will find the required
files when loading a core file.
Adding all of the above required pretty much a full rewrite of the
existing set_cbfd_soname_build_id function and the corresponding
get_cbfd_soname_build_id function, so I have taken the opportunity to
move the information caching out of solib.c and into corelow.c where
it is now accessed through the function core_target_find_mapped_file.
At this point the benefit of this move is not entirely obvious, though
I don't think the new location is significantly worse than where it
was originally. The benefit though is that the cached information is
no longer tied to the shared library loading code.
I already have a second set of patches (not in this series) that make
use of this caching from elsewhere in GDB. I've not included those
patches in this series as this series is already pretty big, but even
if those follow up patches don't arrive, I think the new location is
just as good as the original location.
Rather that caching the information within the core file BFD via the
registry mechanism, the information used for the mapped file lookup is
now stored within the core_file target directly.
This commit improves how GDB handles file backed mappings within a
core file, specifically, this is a restructuring of the function
core_target::build_file_mapping.
The primary motivation for this commit was to put in place the
infrastructure to support the next commit in this series, but this
commit does itself make some improvements.
Currently in core_target::build_file_mapping we use
gdbarch_read_core_file_mappings to iterate over the mapped regions
within a core file.
For each region a callback is invoked which is passed details of the
mapping; the file the mapping is from, the offset into the file, and
the address range at which the mapping exists. We are also passed the
build-id for the mapped file in some cases.
We are only told the build-id for the mapped region which actually
contains the ELF header of the mapped file. Other regions of the same
mapped ELF will not have the build-id passed to the callback.
Within core_target::build_file_mapping, in the per-region callback, we
try to find the mapped file based on its filename. If the file can't
be found, and if we have a build-id then we'll ask debuginfod to
download the file.
However we find the file, we cache the opened bfd object, which is
good. Subsequent mappings from the same file will not have a build-id
set, but by that point we already have a cached open bfd object, so
the lack of build-id is irrelevant.
The problem with the above is that if we find a matching file based on
the filename, then we accept that file, even if we have a build-id,
and the build-id doesn't match.
Currently, the mapped region processing is done in a single pass, we
call gdbarch_read_core_file_mappings, and for each mapping, as we see
it, we create the data structures needed to represent that mapping.
In this commit, I will change this to a two phase process. In the
first phase the mappings are grouped together based on the name of the
mapped file. At the end of phase one we have a 'struct mapped_file',
a new struct, for each mapped file. This struct associates an
optional build-id with a list of mapped regions.
In the second phase we try to find the file using its filename. If
the file is found, and the 'struct mapped_file' has a build-id, then
we'll compare the build-id with the file we found. This allows us to
reject on-disk files which have changed since the core file was
created.
If no suitable file was found (either no file found, or a build-id
mismatch) then we can use debuginfod to potentially download a
suitable file.
NOTE: In the future we could potentially add additional sanity
checks here, for example, if a data-file is mapped, and has no
build-id, we can estimate a minimum file size based on the expected
mappings. If the file we find is not big enough then we can reject
the on-disk file. But I don't know how useful this would actually
be, so I've not done that for now.
Having found (or not) a suitable file then we can create the data
structures for each mapped region just as we did before.
The new functionality here is the extra build-id check, and the
possibility of rejecting an on-disk file if the build-id doesn't
match.
This change could have been done within the existing single phase
approach I think, however, in the next approach I need to have all the
mapped regions associated with the expected build-id, and the new two
phase structure allows me to do that, this is the reason for such an
extensive rewrite in this commit.
There's a new test that exercises GDB's ability to find mapped files
via the build-id, and this downloading from debuginfod.
Andrew Burgess [Wed, 15 May 2024 14:28:45 +0000 (15:28 +0100)]
gdb/corefile: don't pretend unavailable sections are readable
When GDB opens a core file the bfd library processes the core file and
creates sections within the bfd object to represent each of the
segments within the core file.
GDB then creates two target_section lists, m_core_section_table and
m_core_file_mappings, these, along with m_core_unavailable_mappings,
are used by GDB to implement core_target::xfer_partial; this is the
function used when GDB tries to read memory from a core file inferior.
The m_core_section_table list represents sections within the core file
itself. The sections in this list can be split into two groups based
on whether the section has the SEC_HAS_CONTENTS flag set or not.
Sections (from the core file) that have the SEC_HAS_CONTENTS flag had
their contents copied into the core file when the core file was
created. These correspond to writable sections within the original
inferior (the inferior for which the core file was created).
Sections (from the core file) that do not have the SEC_HAS_CONTENTS
flag will not have had their contents copied into the core file when
it was created. These sections correspond to read-only sections
mapped from a file (possibly the initial executable, or possibly some
other file) in the original inferior. The expectation is that the
contents of these sections can still be found by looking in the file
that was originally mapped.
The m_core_file_mappings list is created when GDB parses the mapped
file list in the core file. Every mapped region will be covered by
entries in the m_core_section_table list (see above), but for
read-only mappings the entry in m_core_section_table will not have the
SEC_HAS_CONTENTS flag set. As GDB parses the mapped file list, if the
file that was originally mapped can be found, then GDB creates an
entry in the m_core_file_mappings list which represents the region
of the file that was mapped into the original inferior.
However, GDB only creates entries in m_core_file_mappings if it is
able to find the correct on-disk file to open. If the file can't be
found then an entry is added to m_core_unavailable_mappings instead.
If is the handling m_core_unavailable_mappings which I think is
currently not completely correct.
When a read lands within an m_core_unavailable_mappings region we
currently forward the read to the exec file stratum. The reason for
this is this: when GDB read the mapped file list, if the executable
file could not be found at the expected path then mappings within the
executable will end up in the m_core_unavailable_mappings list.
However, the user might provide the executable to GDB from a different
location. If this happens then forwarding the read to the exec file
stratum might give a result.
But, if the exec file stratum does not resolve the access then
currently we continue through ::xfer_partial, the next step of which
is to handle m_core_section_table entries that don't have the
SEC_HAS_CONTENTS flag set. Every m_core_unavailable_mappings entry
will naturally have an m_core_section_table without the
SEC_HAS_CONTENTS flag set, and so we treat the unavailable mapping as
zero initialised memory and return all zeros.
It is this fall through behaviour that I think is wrong. If a read
falls in an unavailable region, and the exec file stratum cannot help,
then I think the access should fail.
To achieve this goal I have removed the xfer_memory_via_mappings
helper function and moved its content inline into ::xfer_partial.
Now, if an access is within an m_core_unavailable_mappings region, and
the exec file stratum doesn't help, we immediately return with an
error.
The reset of ::xfer_partial is unchanged, I've extended some comments
in the area that I have changed to (I hope) explain better what's
going on.
There's a new test that covers the new functionality, an inferior maps
a file and generates a core file. We then remove the mapped file,
load the core file and try to read from the mapped region. The
expectation is that GDB should give an error rather than claiming that
the region is full of zeros.
Xin Wang [Fri, 16 Aug 2024 03:28:10 +0000 (11:28 +0800)]
Not append rela for absolute symbol
LoongArch: Not append rela for absolute symbol
Use la.global to get absolute symbol like la.abs.
la.global put address of a global symbol into a got
entry and append a rela for it, which will be used
to relocate by dynamic linker. Dynamic linker should
not relocate for got entry of absolute symbol as it
stores symval not symbol's address.
Xin Wang [Fri, 6 Sep 2024 00:54:07 +0000 (08:54 +0800)]
Add macros to get opcode of instructions approriately
LoongArch: Add macros to get opcode and register of instructions appropriately
Currently, we get opcode of an instruction by manipulate the binary with
it's mask, it's a bit of a pain. Now a macro is defined to do this and a
macro to get the RD and RJ registers which is applicable to most instructions
of LoongArch are added.
Tom Tromey [Wed, 14 Aug 2024 13:45:59 +0000 (07:45 -0600)]
Fix 'catch exception' with -flto
A user noticed that when an Ada program (including the runtime) is
compiled with -flto, then "catch exception" does not work -- even
though setting the equivalent breakpoint by hand does work.
Looking into this, it turns out that GCC puts the exception functions
from the Ada runtime into a CU that uses the C language, not Ada.
Then, when trying to look up the relevant symbol,
lookup_name_info::search_name_hash uses the "verbatim" form of the
symbol name (like "<__gnat_debug_raise_exception>") rather than the
"<>"-less form, causing the symbol not to be found.
This patch fixes the problem in two steps.
First, lookup_name_info::search_name_hash is changed to use the same
hack that language_defn::get_symbol_name_matcher uses. That is, when
the current language is Ada, verbatim-mode lookups are special-cased.
(This is a bit unfortunate; perhaps a better long term approach would
be to promote verbatim mode to a fundamental mode of
lookup_name_info.)
Second, although the above fixes the problem in the Ada language mode,
the code still fails in other languages. However, due to the way
these lookups are coded in ada-lang.c, I think it makes sense to
temporarily set the current language to Ada in
create_ada_exception_catchpoint.
Tested on x86-64 Fedora 38.
A new test case that mimics the -flto scenario is included.
Reviewed-By: Alexandra Petlanova Hajkova <ahajkova@redhat.com>
Tom Tromey [Tue, 3 Sep 2024 18:08:43 +0000 (12:08 -0600)]
Test -fgnat-encodings=all in tagged_access.exp
While working on a longer series, I needed to make sure this
particular test kept working with -fgnat-encodings=all, so this patch
adds it to the test.
Tom Tromey [Wed, 8 May 2024 19:46:53 +0000 (13:46 -0600)]
Introduce and use foreach_gnat_encoding
gnat-llvm does not support the -fgnat-encodings flag. This patch
prepares gdb's Ada tests to handle this situation by introducing a new
foreach_gnat_encoding. A subsequent patch may change this to support
gnat-llvm; meanwhile this is a little cleaner anyway.
Fix the build-id option for GCC default configuration
It is possible that the compiler is configured to do
so automatically, but at least for GCC the configure option
--enable-linker-build-id is not enabled by default.
So the option -Wl,--build-id should be used regardless
of which compiler is used.
This patch initializes the "op" variable in skip_cfa_op() function
of bfd/elf-eh-frame.c to "0" at its declaration point to avoid the
"maybe-uninitialized" warning.
Building binutils on a system with GCC version 13.2.0 and a configure
command that sets the optimization level to "-Og" leads to a build
failure because of a warning being treated as an error:
---------------------------------------------------------------------
$ ./configure CFLAGS="-Og"
$ make
...
CC elf-eh-frame.lo
/src/gdb/bfd/elf-eh-frame.c: In function 'skip_cfa_op':
/src/gdb/bfd/elf-eh-frame.c:354:33: error: 'op' may be used
uninitialized [-Werror=maybe-uninitialized]
354 | switch (op & 0xc0 ? op & 0xc0 : op)
| ~~~~~~~~~~~~~~~~~~~~~~^~~~
/src/gdb/bfd/elf-eh-frame.c:348:12: note: 'op' was declared here
348 | bfd_byte op;
| ^~
cc1: all warnings being treated as errors
...
---------------------------------------------------------------------
The relevant code snippet related to this warning looks like:
---------------------------------------------------------------------
static inline bool
read_byte (bfd_byte **iter, bfd_byte *end, unsigned char *result)
{
if (*iter >= end)
return false;
*result = *((*iter)++);
return true;
}
This warning probably happens because "-Og" results in GCC not
inlining the "read_byte()" function. Therefore, GCC treats its
invocation inside "skip_cfa_op()" like a black box and that ends
in the aforementioned warning.
Acknowledgement:
Lancelot Six -- for coming with the idea behind this fix.
Jan Beulich -- for reviewing.
bfd/ChangeLog:
* elf-eh-frame.c (skip_cfa_op): Initialize the "op" variable.
Jan Beulich [Fri, 6 Sep 2024 06:35:07 +0000 (08:35 +0200)]
x86/APX: optimize certain reg-only CFCMOVcc forms
Along the lines of 2513312930b2 ("x86/APX: apply NDD-to-legacy
transformation to further CMOVcc forms") these can similarly be
converted to the shorter legacy-encoded CMOVcc.
Jan Beulich [Fri, 6 Sep 2024 06:34:24 +0000 (08:34 +0200)]
bfd/PE: correct SizeOfImage calculation
We don't really want to align the last section's size to object
alignment (when that section may itself not be aligned as much), we want
image size to be a multiple thereof.
projects / thirdparty / binutils-gdb.git / log
