Tom de Vries [Tue, 11 Mar 2025 08:38:50 +0000 (09:38 +0100)]
[gdb/testsuite] Fix gdb.base/step-over-syscall.exp with glibc 2.41
On openSUSE Tumbleweed, with glibc 2.41, when running test-case
gdb.base/step-over-syscall.exp I run into:
...
(gdb) stepi^M
0x00007ffff7cfd09b in __abort_lock_rdlock () from /lib64/libc.so.6^M
1: x/i $pc^M
=> 0x7ffff7cfd09b <__abort_lock_rdlock+29>: syscall^M
(gdb) p $eax^M
$1 = 14^M
(gdb) FAIL: $exp: fork: displaced=off: syscall number matches
FAIL: $exp: fork: displaced=off: find syscall insn in fork (timeout)
...
We're stepi-ing through fork trying to find the fork syscall, but encounter
another syscall.
The test-case attempts to handle this:
...
gdb_test_multiple "stepi" "find syscall insn in $syscall" {
-re ".*$syscall_insn.*$gdb_prompt $" {
# Is the syscall number the correct one?
if {[syscall_number_matches $syscall]} {
pass $gdb_test_name
} else {
exp_continue
}
}
-re "x/i .*=>.*\r\n$gdb_prompt $" {
incr steps
if {$steps == $max_steps} {
fail $gdb_test_name
} else {
send_gdb "stepi\n"
exp_continue
}
}
}
...
but fails to do so because it issues an exp_continue without issuing a new
stepi command, and consequently the "find syscall insn in fork" test times
out.
Also, the call to syscall_number_matches produces a PASS or FAIL, so skipping
one syscall would produce:
...
FAIL: $exp: fork: displaced=off: syscall number matches
PASS: $exp: fork: displaced=off: syscall number matches
DUPLICATE: $exp: fork: displaced=off: syscall number matches
...
Fix this by:
- not producing PASS or FAIL in syscall_number_matches, and
- issuing stepi when encountering another syscall.
While we're at it, fix indentation in syscall_number_matches.
Tested on x86_64-linux, specifically:
- openSUSE Tumbleweed (glibc 2.41), and
- openSUSE Leap 15.6 (glibc 2.38).
Tom Tromey [Mon, 10 Mar 2025 21:11:25 +0000 (15:11 -0600)]
Remove pid from test name in gcore-memory-usage.exp
The new gcore-memory-usage.exp test puts a PID into a test case name,
causing spurious comparison failures. This patch changes the test
name to avoid this.
Tom Tromey [Sun, 9 Mar 2025 16:14:34 +0000 (10:14 -0600)]
Add string cache and use it in cooked index
The cooked index needs to allocate names in some cases -- when
canonicalizing or when synthesizing Ada package names. This process
currently uses a vector of unique_ptrs to manage the memory.
Another series I'm writing adds another spot where this allocation
must be done, and examining the result showed that certain names were
allocated multiple times.
To clean this up, this patch introduces a string cache object and
changes the cooked indexer to use it. I considered using bcache here,
but bcache doesn't work as nicely with string_view -- because bcache
is fundamentally memory-based, a temporary copy of the contents must
be made to ensure that bcache can see the trailing \0. Furthermore,
writing a custom class lets us avoid another copy when canonicalizing
C++ names.
Approved-By: Simon Marchi <simon.marchi@efficios.com>
Simon Marchi [Mon, 10 Mar 2025 20:14:35 +0000 (16:14 -0400)]
Revert past commits
I accidentally pushed my work-in-progress branch... revert that. Sorry
for the noise :(.
The list of commits reverted are:
ae2a50a9ae15 attempt to revamp to the CU/TU list e9386435c94f gdb/dwarf: print DWARF CUs/TUs in "maint print objfiles" 6cbd64aa3eb0 gdb/dwarf: add dwarf_source_language_name 32a187da7622 libiberty: move DW_LANG_* definitions to dwarf2.def b3fa38aef59d gdb/dwarf: move index unit vectors to debug names reader and use them 30ba74418982 gdb/dwarf: track comp and type units count bedb4e09f292 gdb/dwarf: remove unnecessary braces b4f18de12c77 gdb/dwarf: use ranged for loop in some pots
Simon Marchi [Tue, 18 Feb 2025 19:31:28 +0000 (14:31 -0500)]
gdb/dwarf: print DWARF CUs/TUs in "maint print objfiles"
This was useful to me, to debug some problems.
Before printing cooked index entries, print a list of CUs and TUs. The
information printed for each is a bit arbitrary, I took a look at the
types and printed what seemed relevant.
I moved the call to cooked_index_functions::wait before printing the
CU/TU list, otherwise trying to call "maint print objfiles" quickly,
like this, would lead to an internal error:
This is because dwarf2_per_cu_data::m_unit_type was not yet set, when
trying to read it. Waiting for the index to be built ensures that it is
set, since setting the unit type is done as a side-effect somewhere.
Simon Marchi [Tue, 18 Feb 2025 19:31:26 +0000 (14:31 -0500)]
libiberty: move DW_LANG_* definitions to dwarf2.def
In order to get a "DW_LANG_* to string" function:
- move the "DW_LANG_*" definitions from dwarf2.h to dwarf2.def
- add the necessary macros in dwarf2.h to generate the enumeration
- add the necessary macros in dwarfnames.c to generate the "to string"
function
Simon Marchi [Mon, 10 Mar 2025 16:47:21 +0000 (12:47 -0400)]
gdb/dwarf: move index unit vectors to debug names reader and use them
Since these vectors contain the CU and TU lists as found in the
.debug_names header, it seems like they are meant to be used by the
.debug_names reader when handling a DW_IDX_compile_unit or
DW_IDX_type_unit attribute. The value of the attribute would translate
directly into an index into one of these vectors.
However there's something fishy: it looks like these vectors aren't
actually used in practice. They are used in the
dwarf2_per_bfd::get_index_{c,t}u methods, which in turn aren't used
anywhere.
The handlers of DW_IDX_compile_unit and DW_IDX_type_unit use the
dwarf2_per_bfd::get_cu method, assuming that all compile units are
placed before type units in the dwarf2_per_bfd::all_units vector. I see
several problems with that:
1. I found out [1] that the dwarf2_per_bfd::all_units didn't always
have the CUs before the TUs. So indexing dwarf2_per_bfd::all_units
with that assumption will not work.
2. The dwarf2_find_containing_comp_unit function assumes an ordering of
units by section offset (among other criteria) in order to do a
binary search. Even though it's probably commonly the case, nothing
guarantees that the order of CUs and TUs in the .debug_names header
(which defines the indices used to refer to them) will be sorted by
section offset. It's not possible to make
dwarf2_find_containing_comp_unit (assuming it wants to do a binary
search by section offset) and the DW_IDX_compile_unit /
DW_IDX_type_unit handlers use the same vector.
3. I have not tested this, but in the presence of a dwz supplementary
file, the .debug_names reader should probably not put the units from
the main and dwz files in the same vectors to look them up by index.
Presumably, if both the main and dwz files have a .debug_names
index, they have distinct CU / TU lists. So, an CU index of 1 in an
index entry in the main file would refer to a different CU than an
index of 1 in an index entry in the dwz file. The current code
doesn't seem to account for that, it just indexes
dwarf2_per_bfd::all_units.
Since those vectors are kind of specific to the .debug_names reader,
move them there, in the mapped_debug_names_reader struct. Then, update
the handlers of DW_IDX_compile_unit and DW_IDX_type_unit to use them.
Simon Marchi [Mon, 3 Mar 2025 19:43:15 +0000 (14:43 -0500)]
gdb/dwarf: track comp and type units count
A subsequent commit will remove the all_comp_units and all_type_units
array views, since the all_units vector will no longer be segmented
between comp and type units. Some callers still need to know the number
of each kind, so track that separately.
The problem is that dwarf2_per_cu objects created in the
read_cutu_die_from_dwo code path never have their DWARF version set. A
seemingly obvious solution would be to add a call to
dwarf2_per_cu::set_version in there (there's a patch in the referenced
PR that does that). However, this comment in
read_comp_units_from_section is a bit scary:
/* Init this asap, to avoid a data race in the set_version in
cutu_reader::cutu_reader (which may be run in parallel for the cooked
index case). */
this_cu->set_version (cu_header.version);
I don't know if a DWO file can be read while the cooked indexer runs, so
if it would be a problem here, but I prefer to be safe than sorry. This
patch side-steps the problem by deleting the DWARF version from
dwarf2_per_cu.
The only users of dwarf2_per_cu::version are the loclists callbacks in
`loc.c`. Add the DWARF version to dwarf2_loclist_baton and modify those
callbacks to get the version from there instead. Initialize that new
field in fill_in_loclist_baton.
I like this approach because there is no version field that is possibly
unset now.
I wasn't keen on doing this at first because I thought it would waste
space, but the dwarf2_loclist_baton has 7 bytes of padding at the end
anyway, so we might as well use that.
Cc: Ricky Zhou <ricky@rzhou.org> Cc: Tom de Vries <tdevries@suse.de> Cc: Tom Tromey <tom@tromey.com>
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=32309
Change-Id: I30d4ede7d67da5d80ff65c6122f5868e1098ec52 Approved-By: Tom Tromey <tom@tromey.com>
Tom Tromey [Mon, 10 Mar 2025 14:51:01 +0000 (08:51 -0600)]
Use flags enum for cooked_index_entry::full_name
I found a small bug coming from a couple of recent patches of mine for
cooked_index_entry::full_name.
First, commit aab26529b30 (Add "Ada linkage" mode to
cooked_index_entry::full_name) added a small hack to optionally
compute the Ada linkage name.
Then, commit aab2ac34d7f (Avoid excessive CU expansion on failed
matches) changed the relevant expand_symtabs_matching implementation
to use this feature.
However, the feature was used unconditionally, causing a bad side
effect: the non-canonical name is now used for all languages, not just
Ada. But, for C++ this is wrong.
Furthermore, consider the declaration of full_name:
Tom Tromey [Sun, 9 Mar 2025 17:05:40 +0000 (11:05 -0600)]
Fix check-include-guards.py
I noticed that check-include-guards.py doesn't error in certain
situations -- but in situations where the --update flag would cause a
file to be changed.
This patch changes the script to issue an error for any discrepancy.
It also fixes the headers that weren't correct.
Approved-By: Simon Marchi <simon.marchi@efficios.com>
Alan Modra [Mon, 10 Mar 2025 12:31:54 +0000 (23:01 +1030)]
Further tidies to bed->p_align code
align_pagesize was used for two things, reducing p->p_align from
maxpagesize to the bed->p_align value (section alignment permitting),
and increasing p->p_align above maxpagesize if section alignment
required that. This patch untangles those two, making align_pagesize
only do the former. p->p_align is set directly for the latter. I've
made that change to p->p_align only when D_PAGED to keep things
consistent with other early assignments to p->p_align. p->p_align is
set later according to section alignment when not D_PAGED.
I've also moved the place where align_pagesize adjusts p->p_align to
be with other code setting p->p_align. That seemed better to me than
leaving it until the last possible moment. Note that it isn't
necessary to have this adjustment done inside a test for a PT_LOAD
header, since we never set align_pagesize non-zero outside a PT_LOAD
test.
* elf.c (assign_file_positions_for_load_sections): Clear
align_pagesize whenever we have a section alignment more than
bed->p_align. Set p->p_align rather than align_pagesize
when section alignment exceeds maxpagesize. Assign p->p_align
from align_pagesize earlier.
Jens Remus [Mon, 10 Mar 2025 12:14:08 +0000 (13:14 +0100)]
ld: Cleanup sframe_decoder_init_func_bfdinfo use of reloc cookie
The loop did set cookie->rel to the i-th relocation twice. At the
beginning using the loop counter. At the end by incrementing. One
approach is sufficient.
Change cookie to pointer-to-const, replace cookie->rel by rel,
initialize before the loop and increment at the end, and merge the
two assertions (for cookie->rel) into one.
While at it change sec to pointer-to-const.
bfd/
* elf-sframe.c (sframe_decoder_init_func_bfdinfo): Cleanup use
of relocation cookie.
Jens Remus [Mon, 10 Mar 2025 12:14:08 +0000 (13:14 +0100)]
gas: Use SFrame header and FDE field sizes when generating .sframe
The use of SFRAME_RELOC_SIZE in generation of SFrame stack trace
information from CFI directives erroneously suggested that this could
be used to configure a different relocation size. But in practice it
is tied to the SFrame field sizes it is used for and therefore cannot
be changed.
Replace the uses of SFRAME_RELOC_SIZE by the size of the respective
SFrame header and FDE fields when emitting SFrame information. While
at it enhance some comments.
gas/
* gen-sframe.c (SFRAME_RELOC_SIZE): Delete.
(sizeof_member): Define.
(output_sframe_funcdesc): Use size of SFrame FDE fields instead
of SFRAME_RELOC_SIZE.
(output_sframe_internal): Use size of SFrame header fields
instead of SFRAME_RELOC_SIZE.
Nelson Chu [Sun, 9 Mar 2025 06:17:09 +0000 (14:17 +0800)]
RISC-V: PR32772, fixed segfault caused by the accidental removal of `h != NULL'
bfd/
PR 32772
* elfnn-riscv.c (riscv_elf_relocate_section): Fixed segfault caused by
the accidental removal of `h != NULL' when handling a call to an
undefined weak function.
Brandon Belew [Sun, 9 Mar 2025 04:15:07 +0000 (08:15 +0400)]
Fix segfault if target_fileio_read_alloc fails
Check for target_fileio_read_alloc failure in linux_fill_prpsinfo
before dereferencing buffer. This fixes a segfault in the 'gcore'
command when attached to certain remote targets.
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=32441 Approved-By: Andrew Burgess <aburgess@redhat.com>
Alan Modra [Sat, 8 Mar 2025 09:19:06 +0000 (19:49 +1030)]
bfd_elf_parse_attr_section_v1 buffer overflow
This function has a misleading parameter "contents", which usually
means an entire section contents is passed. However in this case the
actual sections contents plus one is passed, leading to miscalculating
the end of the buffer.
* elf-attrs.c (bfd_elf_parse_attr_section_v1): Delete hdr and
contents param. Add p and p_end as params.
(_bfd_elf_parse_attributes): Adjust to suit.
Tom de Vries [Sat, 8 Mar 2025 08:52:08 +0000 (09:52 +0100)]
[gdb/testsuite] Fix gdb.base/step-over-syscall.exp with -m32 for AMD
When running test-case gdb.base/step-over-syscall.exp with target board
unix/-m32 on an AMD processor, I run into:
...
(gdb) x/2i $pc^M
=> 0xf7fc9575 <__kernel_vsyscall+5>: syscall^M
0xf7fc9577 <__kernel_vsyscall+7>: int $0x80^M
(gdb) PASS: $exp: fork: displaced=off: pc before/after syscall instruction
stepi^M
[Detaching after fork from child process 65650]^M
0xf7fc9579 in __kernel_vsyscall ()^M
1: x/i $pc^M
=> 0xf7fc9579 <__kernel_vsyscall+9>: pop %ebp^M
(gdb) $exp: fork: displaced=off: stepi fork insn
print /x $pc^M
$2 = 0xf7fc9579^M
(gdb) PASS: gdb.base/step-over-syscall.exp: fork: displaced=off: pc after stepi
FAIL: $exp: fork: displaced=off: pc after stepi matches insn addr after syscall
...
The problem is that the syscall returns at the "pop %ebp" insn, while the
test-case expects it to return at the "int $0x80" insn.
This is similar to the problem I fixed in commit 14852123287 ("[gdb/testsuite]
Fix gdb.base/step-over-syscall.exp with -m32"), just that the syscall sequence
used there used the "sysenter" insn instead of the "syscall" insn.
Fix this by extending the fix for commit 14852123287 to also handle the
"syscall" insn.
Tested on x86_64-linux, both using an AMD and Intel processor.
The symptom is that for a given dwarf2_per_cu, the language gets set
twice. First, set to `language_ada`, and then, to `language_minimal`.
It's unexpected for the language of a CU to get changed like this.
The CU at offset 0x0 in the main file looks like:
0x00000000: Compile Unit: length = 0x00000030, format = DWARF32, version = 0x0004, abbr_offset = 0x0000, addr_size = 0x08 (next unit at 0x00000034)
The thing to note is that the language attribute is only present in the
DIE in the DWO file, not on the DIE in the main file.
The first time the language gets set is here:
#0 dwarf2_per_cu::set_lang (this=0x50f0000044b0, lang=language_ada, dw_lang=DW_LANG_Ada95) at /home/smarchi/src/binutils-gdb/gdb/dwarf2/read.c:20788
#1 0x0000555561666af6 in cutu_reader::prepare_one_comp_unit (this=0x7ffff10bf2b0, cu=0x51700008e000, pretend_language=language_minimal) at /home/smarchi/src/binutils-gdb/gdb/dwarf2/read.c:21029
#2 0x000055556159f740 in cutu_reader::cutu_reader (this=0x7ffff10bf2b0, this_cu=0x50f0000044b0, per_objfile=0x516000066080, abbrev_table=0x510000004640, existing_cu=0x0, skip_partial=false, pretend_language=language_minimal, cache=0x7ffff11b95e0) at /home/smarchi/src/binutils-gdb/gdb/dwarf2/read.c:3371
#3 0x00005555615a547a in process_psymtab_comp_unit (this_cu=0x50f0000044b0, per_objfile=0x516000066080, storage=0x7ffff11b95e0) at /home/smarchi/src/binutils-gdb/gdb/dwarf2/read.c:3799
#4 0x00005555615a9292 in cooked_index_worker_debug_info::process_cus (this=0x51700008dc80, task_number=0, first=std::unique_ptr<dwarf2_per_cu> = {...}, end=std::unique_ptr<dwarf2_per_cu> = {...}) at /home/smarchi/src/binutils-gdb/gdb/dwarf2/read.c:4122
In this code path (particularly this specific cutu_reader constructir),
the work is done to find and read the DWO file. So the language is
properly identifier as language_ada, all good so far.
The second time the language gets set is:
#0 dwarf2_per_cu::set_lang (this=0x50f0000044b0, lang=language_minimal, dw_lang=0) at /home/smarchi/src/binutils-gdb/gdb/dwarf2/read.c:20788
#1 0x0000555561666af6 in cutu_reader::prepare_one_comp_unit (this=0x7ffff0f42730, cu=0x517000091b80, pretend_language=language_minimal) at /home/smarchi/src/binutils-gdb/gdb/dwarf2/read.c:21029
#2 0x00005555615a1822 in cutu_reader::cutu_reader (this=0x7ffff0f42730, this_cu=0x50f0000044b0, per_objfile=0x516000066080, pretend_language=language_minimal, parent_cu=0x0, dwo_file=0x0) at /home/smarchi/src/binutils-gdb/gdb/dwarf2/read.c:3464
#3 0x000055556158c850 in dw2_get_file_names (this_cu=0x50f0000044b0, per_objfile=0x516000066080) at /home/smarchi/src/binutils-gdb/gdb/dwarf2/read.c:1956
#4 0x000055556158f4f5 in dw_expand_symtabs_matching_file_matcher (per_objfile=0x516000066080, file_matcher=...) at /home/smarchi/src/binutils-gdb/gdb/dwarf2/read.c:2157
#5 0x00005555616329e2 in cooked_index_functions::expand_symtabs_matching (this=0x50200002ab50, objfile=0x516000065780, file_matcher=..., lookup_name=0x0, symbol_matcher=..., expansion_notify=..., search_flags=..., domain=..., lang_matcher=...) at /home/smarchi/src/binutils-gdb/gdb/dwarf2/read.c:15912
#6 0x0000555562ca8a14 in objfile::map_symtabs_matching_filename (this=0x516000065780, name=0x50200002ad90 "break pck.adb", real_path=0x0, callback=...) at /home/smarchi/src/binutils-gdb/gdb/symfile-debug.c:207
#7 0x0000555562d68775 in iterate_over_symtabs (pspace=0x513000005600, name=0x50200002ad90 "break pck.adb", callback=...) at /home/smarchi/src/binutils-gdb/gdb/symtab.c:727
Here, we use the other cutu_reader constructor, the one that does not
look up the DWO file for the passed CU. If a DWO file exists for this
CU, the caller is expected to pass it as a parameter. That cutu_reader
constructor also ends up setting the language of the CU. But because it
didn't read the DWO file, it didn't figure out the language is
language_ada, so it tries to set the language to the default,
language_minimal.
A question is: why do we end up trying to set the CU's language is this
context. This is completely unrelated to what we're trying to do, that
is get the file names from the line table. Setting the language is a
side-effect of just constructing a cutu_reader, which we need to look up
attributes in dw2_get_file_names_reader. There are probably some
cleanups to be done here, to avoid doing useless work like looking up
and setting the CU's language when all we need is an object to help
reading the DIEs and attributes. But that is future work.
The same cutu_reader constructor is used in
`dwarf2_per_cu::ensure_lang`. Since this is the version of cutu_reader
that does not look up the DWO file, it will conclude that the language
is language_minimal and set that as the CU's language. In other words,
`dwarf2_per_cu::ensure_lang` will get the language wrong, pretty ironic.
Fix this by using the other cutu_reader constructor in those two spots.
Pass `per_objfile->get_cu (this_cu)`, as the `existing_cu` parameter. I
think this is necessary, because that constructor has an assert to check
that if `existing_cu` is nullptr, then there must not be an existing
`dwarf2_cu` in the per_objfile.
To avoid getting things wrong like this, I think that the second
cutu_reader constructor should be reserved for the spots that do pass a
non-nullptr dwo_file. The only spot at the moment in
create_cus_hash_table, where we read multiple units from the same DWO
file. In this context, I guess it makes sense for efficiency to get the
dwo_file once and pass it down to cutu_reader. For that constructor,
make the parameters non-optional, add "non-nullptr" asserts, and update
the code to assume the passed values are not nullptr.
What I don't know is if this change is problematic thread-wise, if the
functions I have modified to use the other cutu_reader constructor can
be called concurrently in worker threads. If so, I think it would be
problematic.
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=32742
Change-Id: I980d16875b9a43ab90e251504714d0d41165c7c8 Approved-By: Tom Tromey <tom@tromey.com>
Tom Tromey [Fri, 21 Feb 2025 16:18:28 +0000 (09:18 -0700)]
Avoid excessive CU expansion on failed matches
PR symtab/31010 points out that something like "ptype INT" will expand
all CUs in a typical program. The OP further points out that the
original patch for PR symtab/30520:
... did solve the problem, but the patch changed after (my) review and
reintroduced the bug.
In cooked_index_functions::expand_symtabs_matching, the final
component of a split name is compared with the entry's name using the
usual method of calling get_symbol_name_matcher.
This code iterates over languages and tries to split the original name
according to each style. But, the Ada splitter uses the decoded name
-- "int". This causes every C or C++ CU to be expanded.
Clearly this is wrong. And, it seems to me that looping over
languages and trying to guess the splitting style for the input text
is probably bad. However, fixing the problem is not so easy (again
due to Ada). I've filed a follow-up bug, PR symtab/32733, for this.
Meanwhile, this patch changes the code to be closer to the
originally-submitted patch. This works because the comparison is now
done between the full name and the "lookup_name_without_params"
object, which is a less adulterated variant of the original input.
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=31010 Tested-By: Simon Marchi <simon.marchi@efficios.com>
Tom Tromey [Fri, 21 Feb 2025 18:57:48 +0000 (11:57 -0700)]
Use wild matching for lookup_name_info::match_any
Currently, lookup_name_info::match_any symbol_name_match_type::FULL.
However, this seems wrong. Consider the expand_symtabs_matching
implementation of the cooked index: it compares name components, and
then if all the components match, it checks:
if ((match_type == symbol_name_match_type::FULL
|| (lang != language_ada
&& match_type == symbol_name_match_type::EXPRESSION)))
{
if (parent != nullptr)
continue;
That is, if the component-matching loop did not finish, and a full
match is requested, then fail to match. This handles cases where the
index is asked to look up "b::c" but finds "a::b::c".
However, match_any should match, well, any. So, it seems to me that
checking any parent matches is irrelevant -- and therefore this should
use wild matching.
Tom Tromey [Sun, 23 Feb 2025 22:34:40 +0000 (15:34 -0700)]
Handle ">>" in cp-name-parser.y
I noticed that a certain name didn't work correctly when trying to
remove the parameters. I put this into lookup_name_info-selftests.c.
I tracked this down to the fact that cp-name-parser.y doesn't handle
">>" to end templates. This patch fixes this in a simple way --
accepting the "RSH" token where appropriate and then un-pushing a ">".
Georg-Johann Lay [Sun, 16 Feb 2025 17:43:56 +0000 (18:43 +0100)]
AVR: gas/32704 - Improve code generation for __gcc_isr.
The prologue generated by __gcc_isr can be improved in
situations where:
* ZERO_REG is needed, and
* SREG is not clobbered by the ISR, and
* avr-gcc provides a GPR >= R16 with the Done chunk, and
* Code generation is for ordinary AVRs (not AVRrc).
For example, the prologue for
volatile char var;
__attribute__((signal)) void __vector_1 (void)
{
var = 1;
var = 0;
}
Jan Beulich [Fri, 7 Mar 2025 10:27:58 +0000 (11:27 +0100)]
gas: don't permit "repeat" expressions with .cfi_{escape,fde_data}
Repeat counts greater than 1 will emit data directly into the current
(sub-)section. That's wrong with .cfi_*, which defer data emission until
much later: N-1 instances of the specified data would not end up in
.eh_frame (or whatever the section that CFI data was specified to go
into). Simply disallow "repeat" expressions in such cases.
Jan Beulich [Fri, 7 Mar 2025 10:26:43 +0000 (11:26 +0100)]
gas: centralize declaration of listing_tail
Besides it being somewhat off to have three decls scattered across the
code base, it is generally bad practice for the definition of a symbol
to not also observe its declaration (making sure the two won't go out of
sync).
Compilers may split functions, e.g. into a "hot" and "cold" part, or
they may emit special case instantiations (e.g. as a result of IPA). It
can be helpful to be able to disassemble all of the parts or clones in
one go. Permit using "--disassemble=" multiple times.
Jan Beulich [Fri, 7 Mar 2025 10:23:13 +0000 (11:23 +0100)]
objdump: properly disassemble successive functions of the same name
... when only their symbol was requested for disassembly. Addressing the
respective FIXME is as easy as coverting the "else" there to an if()
with the opposite condition, thus accounting for the disabling the
original if() may have effected.
Tom de Vries [Fri, 7 Mar 2025 08:25:33 +0000 (09:25 +0100)]
[gdb/tdep] Support REX2 and EVEX prefix
The following amd64 insn:
...
0: 67 d5 44 8d 3d 00 00 00 00 lea 0x0(%eip),%r31d
...
uses the REX2 prefix [1], which is currently not supported in
amd64_get_insn_details.
Add the missing support in amd64_get_insn_details, as well as a corresponding
unit test.
Likewise for an amd64 insn using an EVEX prefix [2]:
...
0: 62 f1 7c 48 28 05 00 fc ff ff vmovaps -0x400(%rip),%zmm0
...
Tom de Vries [Fri, 7 Mar 2025 08:25:33 +0000 (09:25 +0100)]
[gdb/tdep] Fix vmovdqu decoding
PR tdep/31952 reports that displaced stepping over an instruction pointer
relative insn "vmovdqu 0x20(%rip),%ymm1" gives the wrong results.
This is caused by misclassification of the insn in amd64_get_insn_details,
which results in details.modrm_offset == -1, while the instruction in fact
does have a modrm byte.
The instruction is encoded as follows:
...
400557: c5 fe 6f 0d 20 00 00 00 vmovdqu 0x20(%rip),%ymm1
...
where:
- "0xc5 0xfe" is the vex2 prefix,
- "0x6f" is the opcode,
- "0x0d" is the modrm byte, and
- "0x20 0x00 0x00 0x00" is a 32-bit displacement.
The problem is related to details.opcode_len, which is 1.
While it is true that the length of the opcode in the insn (0x6f) is 1 byte,
the vex2 prefix implies that we're encoding an 2-byte opcode beginnning
with 0x0f [1].
Consequently, we should be using the twobyte_has_modrm map rather than the
onebyte_has_modrm map.
Fix this in amd64_get_insn_details, and add a selftest to check this.
Tom de Vries [Fri, 7 Mar 2025 08:25:33 +0000 (09:25 +0100)]
[gdb/tdep] Make amd64_get_insn_details more regular
In amd64_get_insn_details, I found this code with a comment explaining why
enc_prefix_offset is not set:
...
else if (vex2_prefix_p (*insn))
{
/* Don't record the offset in this case because this prefix has
no REX.B equivalent. */
insn += 2;
}
...
which I didn't understand until I looked at the only use of enc_prefix_offset,
in fixup_riprel:
...
/* REX.B should be unset (VEX.!B set) as we were using rip-relative
addressing, but ensure it's unset (set for VEX) anyway, tmp_regno
is not r8-r15. */
if (insn_details->enc_prefix_offset != -1)
{
gdb_byte *pfx = &dsc->insn_buf[insn_details->enc_prefix_offset];
if (rex_prefix_p (pfx[0]))
pfx[0] &= ~REX_B;
else if (vex3_prefix_p (pfx[0]))
pfx[1] |= VEX3_NOT_B;
else
gdb_assert_not_reached ("unhandled prefix");
}
...
Fix this by:
- setting enc_prefix_offset for the vex2 case in amd64_get_insn_details,
making the function more regular and easier to understand, and
- handling the vex2 case in the "enc_prefix_offset != -1" clause in
fixup_riprel.
Tom de Vries [Fri, 7 Mar 2025 08:25:33 +0000 (09:25 +0100)]
[gdb/tdep] Add vzeroupper and vzeroall in amd64-insn-decode selftest
After I posted a tentative patch for PR31952, Alexander Monakov pointed out
that the patch broke instruction decoding for instructions vzeroall and
vzeroupper.
Add selftests for these two instructions in amd64-insn-decode, both using
vex2 and vex3 prefixes.
Tom de Vries [Fri, 7 Mar 2025 08:25:33 +0000 (09:25 +0100)]
[gdb/tdep] Add vex2_to_vex3
I noticed here [1] that the vex2 prefix is essentially a special case of the
vex3 prefix, meaning it's possible to rewrite any insn with a vex2 prefix into
an equivalent one with a vex3 prefix.
Add function vex2_to_vex3 that does precisely that, in the selftests
namespace.
Tom de Vries [Fri, 7 Mar 2025 08:25:33 +0000 (09:25 +0100)]
[gdb/tdep] Fix rip-relative insn handling in amd64_get_used_input_int_reg
I wanted to add a unit test for an an rip-relative amd64 insn, so I did:
...
$ gcc -fPIE hello.c
...
and used an rip-relative insn from main:
...
4005db: 48 8d 3d 1e 00 00 00 lea 0x1e(%rip),%rdi
...
While writing the unit test, I found that amd64_get_used_input_int_reg returns
rbp as input register.
Fix this by using rip_relative_p in amd64_get_used_input_int_reg to handle
this case.
Tom de Vries [Fri, 7 Mar 2025 08:25:33 +0000 (09:25 +0100)]
[gdb/tdep] Add amd64-insn-decode selftest
Add a selftest that checks the results of amd64_get_insn_details and related
functions for two basic instructions.
Add a parameter assumptions to amd64_get_used_input_int_regs, to make sure
that this selftest:
...
/* INSN: add %eax,(%rcx). */
...
SELF_CHECK (amd64_get_used_input_int_regs (&details, false)
== ((1 << EAX_REG_NUM) | (1 << ECX_REG_NUM)));
...
passes because it found the "%eax" in the insn, rather than passing because of
this assumption:
...
/* Assume RAX is used. If not, we'd have to detect opcodes that implicitly
use RAX. */
used_regs_mask |= 1 << EAX_REG_NUM;
...
Tom de Vries [Fri, 7 Mar 2025 08:25:33 +0000 (09:25 +0100)]
[gdb/tdep] Refactor amd64_get_unused_input_int_reg, part 3
While reading amd64_get_unused_input_int_reg, I noticed that it avoids picking
RSP, which has to do with how the result of the only call to it is going to be
used.
Likewise for picking a register in the RAX ... RDI range.
Fix this by:
- adding an allowed_regs_mask parameter to amd64_get_unused_input_int_reg, and
- properly documenting the value of the corresponding argument in fixup_riprel.
Tom de Vries [Fri, 7 Mar 2025 08:25:33 +0000 (09:25 +0100)]
[gdb/tdep] Refactor amd64_get_unused_input_int_reg, part 2
I noticed that amd64_get_unused_input_int_reg uses a signed int for a bit
mask:
...
/* 1 bit for each reg */
int used_regs_mask = 0;
...
There's an assert:
...
gdb_assert (used_regs_mask < 256);
...
which is meant to assert on register numbers >= 8, but if for instance
sizeof (used_regs_mask) == 4 and used_regs_mask == (1 << 31), then that is not
caught because of the signedness.
We could fix this by changing the type to unsigned int, but that only
guarantees 16 bits in the reg mask. Intel CPUs with the APX extension support
32 int registers.
The implementation of amd64_get_unused_input_int_reg doesn't support analyzing
registers with register number >= 8 yet, but now that we're changing the type,
it seems like a good idea to anticipate this.
Fix this by using uint32_t.
Likewise, update the loop over the reg mask:
...
for (i = 0; i < 8; ++i)
{
if (! (used_regs_mask & (1 << i)))
return i;
...
to handle any used_regs_mask value rather than just those for
register number < 8.
Tom de Vries [Fri, 7 Mar 2025 08:25:33 +0000 (09:25 +0100)]
[gdb/tdep] Refactor amd64_get_unused_input_int_reg, part 1
While reading amd64_get_unused_input_int_reg, I noticed that it first asserts,
then throws an internal_error if no unused register can be found.
Looking at the documentation of gdbarch_displaced_step_copy_insn, it seems
that a failure can be indicated less abruptly, by returning a nullptr.
Fix this by:
- returning -1 in case of failure to find an unused register in
amd64_get_unused_input_int_reg, and
- propagating this to amd64_displaced_step_copy_insn.
Jan Beulich [Fri, 7 Mar 2025 07:30:36 +0000 (08:30 +0100)]
gas: leave expression symbols alone when processing equates
PR gas/32721
In this bogus piece of code distilled from fuzzing and slightly edited:
A=%eax|%!
Y=A
Z=A
or $6,Z
the first of the equates with A on the rhs changes A's section (due to
the use of S_GET_VALUE()), from expression to register, thus yielding Y
in the expression section (and X_op being O_symbol), but Z in the
register section (and X_op being O_register with X_add_value being -1).
There shouldn't be random O_register expressions, though, for targets
setting md_register_arithmetic to false. Plus both Y and Z would better
be exchangeable.
In pseudo_set() wire handling of O_symbol expressions referencing a
symbol in the expression section to that of other stuff ending up in
this section.
Also avoid bogus O_register expressions to be created, for targets
setting md_register_arithmetic to false: S_GET_VALUE() would resolve
any arithmetic, which must not happen for such targets. To be on the
safe side for such targets, also amend resolve_register(). Correct
another earlier oversight there too (affecting at least Z80), by using
the new expr_copy() helper there as well.
Undo 46b9f07dfe79 ("PR 32721, internal error in
tc-i386.c:parse_register"), albeit without losing the simplification it
did.
Jan Beulich [Fri, 7 Mar 2025 07:29:20 +0000 (08:29 +0100)]
v850: improve linker scripts for relocatable linking
Quite a few constructs where unconditional when they should take
$RELOCATING into account. The original observation was that output of
"ld -r" had .text start at 0x00100000.
Jan Beulich [Fri, 7 Mar 2025 07:28:59 +0000 (08:28 +0100)]
gas: fold is_end_of_line[] into lex_type[]
... by way of introducing LEX_EOL and LEX_EOS. As a prereq convert the
remaining open-coded accesses.
The Alpha change is actually a functional one: The array slot for '!'
having been set to 1 is very unlikely to have been correct. 1 means "end
of line", when surely "end of statement" was always meant.
Jan Beulich [Fri, 7 Mar 2025 07:23:53 +0000 (08:23 +0100)]
rl78: drop redundant statement separator check
With the switch to the use of is_end_of_stmt() in 2dd0370c433d
("rl78: use is_whitespace()") the open-coded checking against
line_separator_chars[] can be dropped.
Jan Beulich [Fri, 7 Mar 2025 07:21:00 +0000 (08:21 +0100)]
C54x: use is_end_of_stmt()
... instead of open-coding it.
In tic54x_stringer() this also fixes an array overrun issue: Converting
plain char to unsigned int could have yielded huge values when plain
char is a signed type.
In subsym_substitute() also convert a local variable to plain char, as
that's what it's really holding (and how it's used everywhere else).
Jan Beulich [Fri, 7 Mar 2025 07:14:11 +0000 (08:14 +0100)]
M68HC1x: use is_end_of_stmt()
... instead of open-coding it. With this there's no need for op_end (and
hence op_start) to be other than pointer to plain char. Which in turn
eliminates the need for several questionable casts.
Jan Beulich [Fri, 7 Mar 2025 07:10:35 +0000 (08:10 +0100)]
Arm: use is_end_of_stmt()
... instead of open-coding it. This also fixes an array underrun issue:
The wrong casting to plain int could have yielded negative values when
plain char is a signed type.
Jan Beulich [Fri, 7 Mar 2025 07:10:23 +0000 (08:10 +0100)]
Alpha: use is_end_of_stmt()
... instead of open-coding it. Note that writes to the array need to be
left alone; they can only be converted when the array is folded into
lex_type[].
projects / thirdparty / binutils-gdb.git / log
