Tom Tromey [Sun, 14 Sep 2025 21:20:25 +0000 (15:20 -0600)]
Rework domain choices in ctfread.c
Another patch I am working on induced some failures in CTF tests.
Looking into this, I found that ctfread.c seems to largely work by
accident. In particular, it often chooses the wrong domain for a
symbol.
In CTF, I believe there are 4 kinds of symbols: types, variables,
functions, and "data objects" (which IIUC may be either a variable or
a function).
ctfread.c was examining the type-kind of a variable and sometimes
treating one as a type. add_stt_entries and
ctf_psymtab_add_stt_entries only ever used VAR_DOMAIN (but are called
for functions, which should be in FUNCTION_DOMAIN). And
ctf_psymtab_type_cb sometimes used VAR_DOMAIN, but is only called for
types, and so should only ever use TYPE_DOMAIN or STRUCT_DOMAIN.
This patch cleans all this up, based on my understanding of the
situation. This passes the existing tests, and also works with my
aforementioned yet-to-be-submitted patch as well.
Finally, I renamed new_symbol because it is only used for type
symbols.
Acked-By: Simon Marchi <simon.marchi@efficios.com>
Tom Tromey [Sat, 13 Sep 2025 21:32:50 +0000 (15:32 -0600)]
Fix name checks in ctfread.c
I noticed that ctfread.c could create a symbol with the name "". This
happens because a couple of spots check that a name is not NULL -- but
libctf never returns such names. Instead check the string contents.
I left the NULL checks in for robustness.
Note that other spots in ctfread.c already do check the contents of
the name. I changed these to avoid strlen and instead check the first
character.
Approved-By: Simon Marchi <simon.marchi@efficios.com>
Andrew Burgess [Tue, 17 Jun 2025 17:09:49 +0000 (18:09 +0100)]
gdb/python: extend gdb.write to support styled output
It is already possible to produce styled output from Python by
converting the gdb.Style to its escape code sequence, and writing that
to the output stream.
But this commit adds an alternative option to the mix by extending the
existing gdb.write() function to accept a 'style' argument. The value
of this argument can be 'None' to indicate no style change should be
performed, this is the default, and matches the existing behaviour.
Or the new 'style' argument can be a gdb.Style object, in which case
the specified style is applied only for the string passed to
gdb.write, after which the default style is re-applied.
Using gdb.write with a style object more closely matches how GDB
handles styling internally, and has the benefit that the user doesn't
need to remember to restore the default style when they are done.
Reviewed-By: Eli Zaretskii <eliz@gnu.org> Approved-By: Tom Tromey <tom@tromey.com>
Andrew Burgess [Wed, 23 Apr 2025 13:51:17 +0000 (14:51 +0100)]
gdb/python: new class gdb.StyleParameterSet
Add a new helper class gdb.StyleParameterSet. This new class can be
used to simplify creation of new style parameter sets. A style
parameter set is the 'foreground', 'background', and (optionally), the
'intensity' settings, all grouped under a single prefix command.
And example usage is:
(gdb) python s = gdb.StyleParameterSet("my-style")
(gdb) show style my-style
style my-style background: The "my-style" style background color is: none
style my-style foreground: The "my-style" style foreground color is: none
style my-style intensity: The "my-style" style display intensity is: normal
(gdb)
Having created a gdb.StyleParameterSet, the object itself can be used
to access a named style corresponding to the setting group, like this:
Of course, having access to the gdb.Style makes it easy to change the
settings, or the settings can be adjusted via the normal CLI 'set'
commands.
As gdb.StyleParameterSet manages a set of parameters, and the
gdb.Parameter class uses Parameter.value as the attribute to read the
parameter's value, there is also StyleParameterSet.value, but this is
just an alias for StyleParameterSet.style, that is, it allows the
gdb.Style object to be read and written too.
It is worth noting that this class only creates a single level of
prefix command. As an example GDB has style 'disassembler mnemonic',
where the 'disassembler' part is a group of related styles. If a user
wanted to create:
style
my-style-group
style-1
style-2
style-3
Where each of 'style-1', 'style-2', and 'style-3' will have the full
set of 'foreground', 'background', and 'intensity', then the
gdb.StyleParameterSet can be used to create the 'style-N' part, but
the user will have to create the 'my-style-group' prefix themselves,
possibly using gdb.ParameterPrefix, e.g.:
Andrew Burgess [Fri, 18 Apr 2025 13:24:03 +0000 (14:24 +0100)]
gdb/python: add gdb.Style class
This commit adds a new gdb.Style class. This class represents a
complete style within GDB. A complete style is a collection of
foreground color, background color, and an intensity.
A gdb.Style comes in two flavours, named, and unnamed.
A named style is one that is based on an existing style within GDB.
For example, we have 'set style filename ...', the name of this style
is 'filename'. We also have 'set style disassembler mnemonic ...',
the name of this style is 'disassembler mnemonic'. A named style is
created by passing the name of the style, like this:
The other type of style is an unnamed style. An unnamed style is
created using a foreground and background color, along with an
intensity. Colors are specified using gdb.Color objects. An example
of creating an unnamed style is:
We can see here an example of the new intensity constants that have
been added in this commit, there is gdb.INTENSITY_NORMAL,
gdb.INTENSITY_BOLD, and gdb.INTENSITY_DIM. All of the arguments are
optional, the default for the colors is gdb.Color(), which will apply
the terminal default, and the default intensity is
gdb.INTENSITY_NORMAL.
Having created a gdb.Style object there are two ways that it can be
used to style GDB's output. The Style.escape_sequence() method
returns the escape sequence needed to apply this style, this can be
used as in:
The problem with this approach is that it is the users responsibility
to restore the style to the default when they are done. In the above
example, all output after the escape sequence is printed, including
the next GDB prompt, will be in the s1 (filename) style. Which is why
the Style.apply method exists. This method takes a string and returns
the same string with escape sequences added before and after. The
before sequence switches to the style, while the after escape sequence
restores the terminal default style. This can be used like:
(gdb) python print(s1.apply("Filename Style"))
Now only the 'Filename Style' text will be styled. The next GDB
prompt will be in the default terminal style. Personally, I think the
apply method is the more useful, but having 'escape_sequence' matches
what gdb.Color offers, though if/when this patch is merged, I might
propose a similar 'apply' type method for the gdb.Color class.
The gdb.Style class has 'foreground', 'background', and 'intensity'
attributes which, when read, return the obvious values. These
attributes can also be written too.
When writing to an attribute of an unnamed Style object then the Style
object itself is updated, as you might expect.
When writing to an attribute of a named Style then the style setting
itself is updated as the following example shows:
(gdb) python s1 = gdb.Style("filename")
(gdb) python print(s1.foreground)
green
(gdb) show style filename foreground
The "filename" style foreground color is: green
(gdb) python s1.foreground=gdb.Color("red")
(gdb) python print(s1.foreground)
red
(gdb) show style filename foreground
The "filename" style foreground color is: red
(gdb)
We can see that a gdb.Style object is connected to the underlying
style settings, it doesn't take a copy of the style settings at
creation time. And the relationship works both ways. Continuing the
above example:
(gdb) set style filename foreground blue
(gdb) python print(s1.foreground)
blue
(gdb)
Here we see that changing the setting value causes the gdb.Style
object to update. And this is what you would want. I imagine this
being used in a Python extension to GDB, where a user might create
global objects for some named styles, and then use these globals to
format output from some custom commands. If a user of an extension
changes a style setting then the extension wants to adapt to that
change.
Both the Style.escape_sequence and Style.apply methods take the global
style enabled setting into consideration. If styling is disabled then
Style.escape_sequence will return an empty string, and Style.apply
will return an unmodified copy of the original string object (actually
the input object with Py_INCREF applied).
There is also support for representing a gdb.Style as a string:
This is the binutils fix for PR 33384. Here we are assuming that no
const char* comma-separated option strings are passed in to
disassemble_info.disassembler_options. That is true for current usage
in gdb and binutils. In fact, there is only one place that passes a
string in read-only memory, gdb/tdep-i386.c:disassembly_flavor, and
that one is a single option.
include/
* dis-asm.h (struct disassemble_info): Comment.
(disassembler_options_cmp, next_disassembler_option),
(FOR_EACH_DISASSEMBLER_OPTION): Delete.
(for_each_disassembler_option): Declare.
opcodes/
* disassemble.c (disassembler_options_cmp): Delete.
(for_each_disassembler_option): New function.
* arc-dis.c (parse_option): Replace disassembler_options_cmp
with strcmp.
(parse_cpu_option): Likewise.
(parse_disassembler_options): Replace FOR_EACH_DISASSEMBLER_OPTION
with for_each_disassembler_option, and extract loop body to..
(arc_parse_option): ..this new function.
* arm-dis.c (parse_arm_disassembler_options): Delete, extracting
loop body to..
(arm_parse_option): ..this new function.
(print_insn): Use for_each_disassembler_option.
* csky-dis.c (parse_csky_dis_options): Delete, extracting loop
body to..
(parse_csky_option): ..this new function.
(print_insn_csky): Use for_each_disassembler_option.
* nfp-dis.c (parse_disassembler_options): Replace
FOR_EACH_DISASSEMBLER_OPTION with for_each_disassembler_option,
and extract loop body to..
(nfp_parse_option): ..this new function. Use opcodes_error_handler
here rather than info->fprintf_func to print error.
* ppc-dis.c (ppc_parse_cpu): Replace disassembler_options_cmp
with strcmp.
(struct ppc_parse_data): New.
(powerpc_init_dialect): Adjust to use new struct. Replace
FOR_EACH_DISASSEMBLER_OPTION with for_each_disassembler_option,
and extract loop body to..
(ppc_parse_option): ..this new function.
Alan Modra [Fri, 3 Oct 2025 23:14:58 +0000 (08:44 +0930)]
gdb: PR 33384 invalid disassembler option message
This is the gdb part of fixing PR33384, where it is noted that an
error in a disassembler option prints the rest of the comma separated
option string rather than just the option in error.
Removing FOR_EACH_DISASSEMBLER_OPTION seemed a good idea to me, as we
then expose the strchr there which is useful in zero terminating the
option, and in the case of arm-tdep.c, to replace strcspn. Also, if
the option is zero terminated we don't need disassembler_options_cmp.
Alternatively, you could do similarly to arm-tdep.c in disasm.c by
changing the error message to use %.*s with a length found by strcspn.
I rejected that smaller patch on the grounds that it makes for churn
in message translation. I also prefer to see code using the standard
string functions.
Regression tested on x86_64-linux. Message behaviour tested on
powerpc64le-linux and arm-linux-eabi.
* arm-tdep.c (show_disassembly_style_sfunc): Don't use
FOR_EACH_DISASSEMBLER_OPTION. Use strchr needed for loop
control to size option len.
* disasm.c (set_disassembler_options): Don't use
FOR_EACH_DISASSEMBLER_OPTION. Overwrite comma in options with
a zero. Replace disassembler_options_cmp with strcmp.
Alan Modra [Fri, 3 Oct 2025 23:09:02 +0000 (08:39 +0930)]
mips gas: expression initialisation
There is a make_expr_symbol in append_insn, which gets called from
macro_build, which is all over the place. Many of these set up an
expression without initialising all fields. Now the uninitialised
fields should not be accessed in a properly functioning assembler,
but I'm inclined to think anything copied ought to be initialised.
* config/tc-mips.c (fix_loongson2f_jump, load_register),
(add_got_offset, add_got_offset_hilo, macro_build_branch_likely),
(macro, mips16_macro, s_cpload, s_cpsetup, s_cprestore)
(s_cpreturn): Use structure initialiser to ensure all fields of
expression are initialised.
(load_address): Copy entire structure for the same reason.
Alan Modra [Fri, 3 Oct 2025 23:07:37 +0000 (08:37 +0930)]
gas: more expression initialisation
There are many more places that copy an uninitialised expressionS to a
symbol via symbol_set_value_expression and make_expr_symbol. This
patch focuses on general gas code that does that, and a few backends.
Note that unlike the i386 case that oss-fuzz found, it is likely that
the tc-alpha.c, tc-ppc.c and tc-tic54x.c changes are not fixing bugs,
alpha and tic54x because they don't use X_md, ppc because it carefully
handles X_md. Also, as an example an O_constant expression should
only ever have its X_add_number field accessed, therefore the other
fields can stay uninitialised. However, I think that copying
uninitialised struct fields around is not good practice. If nothing
else it can be confusing when examining symbols under gdb.
I also replaced gen-sframe.c "#ifdef SFRAME_FRE_TYPE_SELECTION_OPT"
with "if (SFRAME_FRE_TYPE_SELECTION_OPT)" so code in the false
branches is compiled and thus less likely to bitrot. (As far as I can
see, SFRAME_FRE_TYPE_SELECTION_OPT is always 1.)
Alan Modra [Fri, 3 Oct 2025 23:07:02 +0000 (08:37 +0930)]
gas: initialisation of expressionS in operand()
This patch removes clean_up_expression which runs just before operand()
returns. clean_up_expression sets as yet uninitialised fields of
expressionS. Well, it sets fields based on the value of X_op,
trusting that others have been written, and has one notable exception:
X_md is not initialised.
Instead initialise expressionS fully inside operand(), which is called
at the start of expr(), and introduce md_expr_init for the odd
backends that want to mess with X_md.
This is in response to an oss-fuzz report that read.c:pseudo_set calls
expr() leaving exp.X_md uninitialised and can copy that to a symbol
via symbol_set_value_expression. tc-i386-intel.c:565 is one place
that later tests the uninitialised X_md.
* config/tc-z80.h (md_expr_init, md_expr_init_rest): Define.
* config/tc-microblaze.h: Likewise.
* expr.c (clean_up_expression): Delete.
(operand): Init expression early.
(expr): Use md_expr_init_rest to init X_md when necessary.
Alan Modra [Fri, 3 Oct 2025 23:06:22 +0000 (08:36 +0930)]
arc gas: don't use X_md as input to expression()
tc-arc.c:tokenize_arguments tweaks expression() parsing, controlling
whether arc_parse_name does anything by setting X_op and X_md in the
expressionS argument passed to expression(). I want to change expr()
to always fully initialise its result, and that means either a special
expression initialiser for arc, or controlling arc_parse_name by some
other means. Since arc_parse_name already tests "assembling_insn" and
no other code does, change "assembling_insn" to directly control
arc_parse_name. Doing it this way also stops a possible uninitialised
access to right.X_op from expr() in arc_parse_name with current gas.
The next patch in this series will also stop such uninitialised
accesses.
* config/tc-arc.c (assembling_insn): Update comment.
(tokenize_arguments): Don't set X_op and X_md to control
expression(), instead just use assembling_insn.
(md_operand): Similarly.
(arc_parse_name): Don't test X_op and X_md.
(md_assemble): Don't set assembling_insn here.
Tom de Vries [Sat, 4 Oct 2025 00:07:16 +0000 (02:07 +0200)]
[gdb] Fix assertion failure due to null frame
PR gdb/33512 reports an assertion failure in test-case
gdb.ada/access_to_packed_array.exp on i386-linux:
...
(gdb) maint print symbols
gdb/frame.c:3400: internal-error: reinflate: \
Assertion `m_cached_level >= -1' failed.
A problem internal to GDB has been detected,
further debugging may prove unreliable.
Quit this debugging session? (y or n) FAIL: $exp: \
maint print symbols (GDB internal error)
...
I haven't been able to reproduce the failure by running the test-case on
x86_64-linux with target board unix/-m32, but I'm able to reproduce on
x86_64-linux by using the exec attached to the PR:
...
$ cat gdb.in
file foo
maint expand-symtabs
maint print symbols
$ gdb -q -batch -ex "set trace-commands on" -x gdb.in
...
c_to: array (gdb/frame.c:3395: internal-error: reinflate: \
Assertion `m_cached_level >= -1' failed.
...
The backtrace at the point of the assertion failure is:
...
(gdb) bt
#0 __pthread_kill_implementation (threadid=<optimized out>,
signo=signo@entry=6, no_tid=no_tid@entry=0) at pthread_kill.c:44
#1 0x00007ffff62a8e7f in __pthread_kill_internal (signo=6,
threadid=<optimized out>) at pthread_kill.c:78
#2 0x00007ffff6257842 in __GI_raise (sig=sig@entry=6)
at ../sysdeps/posix/raise.c:26
#3 0x00007ffff623f5cf in __GI_abort () at abort.c:79
#4 0x00000000010e7ac6 in dump_core () at gdb/utils.c:223
#5 0x00000000010e81b8 in internal_vproblem(internal_problem *, const char *, int, const char *, typedef __va_list_tag __va_list_tag *) (
problem=0x2ceb0c0 <internal_error_problem>,
file=0x1ad5a90 "gdb/frame.c", line=3395,
fmt=0x1ad5a08 "%s: Assertion `%s' failed.", ap=0x7fffffffc3c0)
at gdb/utils.c:475
#6 0x00000000010e82ac in internal_verror (
file=0x1ad5a90 "gdb/frame.c", line=3395,
fmt=0x1ad5a08 "%s: Assertion `%s' failed.", ap=0x7fffffffc3c0)
at gdb/utils.c:501
#7 0x00000000019be79f in internal_error_loc (
file=0x1ad5a90 "gdb/frame.c", line=3395,
fmt=0x1ad5a08 "%s: Assertion `%s' failed.")
at gdbsupport/errors.cc:57
#8 0x00000000009b5c16 in frame_info_ptr::reinflate (this=0x7fffffffc878)
at gdb/frame.c:3395
#9 0x00000000009b66f9 in frame_info_ptr::operator-> (this=0x7fffffffc878)
at gdb/frame.h:290
#10 0x00000000009b4bd5 in get_frame_arch (this_frame=...)
at gdb/frame.c:3075
#11 0x000000000081dd89 in dwarf_expr_context::fetch_result (
this=0x7fffffffc810, type=0x410d600, subobj_type=0x410d600,
subobj_offset=0, as_lval=true)
at gdb/dwarf2/expr.c:1006
#12 0x000000000081e2ef in dwarf_expr_context::evaluate (this=0x7fffffffc810,
addr=0x7ffff459ce6b "W\aF\003", len=1, as_lval=true,
per_cu=0x7fffd00053f0, frame=..., addr_info=0x7fffffffcc30, type=0x0,
subobj_type=0x0, subobj_offset=0)
at gdb/dwarf2/expr.c:1136
#13 0x0000000000877c14 in dwarf2_locexpr_baton_eval (dlbaton=0x3e99c18,
frame=..., addr_stack=0x7fffffffcc30, valp=0x7fffffffcab0,
push_values=..., is_reference=0x7fffffffc9b0)
at gdb/dwarf2/loc.c:1604
#14 0x0000000000877f71 in dwarf2_evaluate_property (prop=0x3e99ce0,
initial_frame=..., addr_stack=0x7fffffffcc30, value=0x7fffffffcab0,
push_values=...) at gdb/dwarf2/loc.c:1668
#15 0x00000000009def76 in resolve_dynamic_range (dyn_range_type=0x3e99c50,
addr_stack=0x7fffffffcc30, frame=..., rank=0, resolve_p=true)
at gdb/gdbtypes.c:2198
#16 0x00000000009e0ded in resolve_dynamic_type_internal (type=0x3e99c50,
addr_stack=0x7fffffffcc30, frame=..., top_level=true)
at gdb/gdbtypes.c:2934
#17 0x00000000009e1079 in resolve_dynamic_type (type=0x3e99c50, valaddr=...,
addr=0, in_frame=0x0) at gdb/gdbtypes.c:2989
#18 0x0000000000488ebc in ada_discrete_type_low_bound (type=0x3e99c50)
at gdb/ada-lang.c:710
#19 0x00000000004eb734 in print_range (type=0x3e99c50, stream=0x30157b0,
bounds_preferred_p=0) at gdb/ada-typeprint.c:156
#20 0x00000000004ebffe in print_array_type (type=0x3e99d10, stream=0x30157b0,
show=1, level=9, flags=0x1bdcf20 <type_print_raw_options>)
at gdb/ada-typeprint.c:381
#21 0x00000000004eda3c in ada_print_type (type0=0x3e99d10,
varstring=0x401f710 "c_to", stream=0x30157b0, show=1, level=9,
flags=0x1bdcf20 <type_print_raw_options>)
at gdb/ada-typeprint.c:1015
#22 0x00000000004b4627 in ada_language::print_type (
this=0x2f949b0 <ada_language_defn>, type=0x3e99d10,
varstring=0x401f710 "c_to", stream=0x30157b0, show=1, level=9,
flags=0x1bdcf20 <type_print_raw_options>)
at gdb/ada-lang.c:13681
#23 0x0000000000f74646 in print_symbol (gdbarch=0x3256270, symbol=0x3e99db0,
depth=9, outfile=0x30157b0) at gdb/symmisc.c:545
#24 0x0000000000f737e6 in dump_symtab_1 (symtab=0x3ddd7e0, outfile=0x30157b0)
at gdb/symmisc.c:313
#25 0x0000000000f73a69 in dump_symtab (symtab=0x3ddd7e0, outfile=0x30157b0)
at gdb/symmisc.c:370
#26 0x0000000000f7420f in maintenance_print_symbols (args=0x0, from_tty=0)
at gdb/symmisc.c:481
#27 0x00000000006c7fde in do_simple_func (args=0x0, from_tty=0, c=0x321e270)
at gdb/cli/cli-decode.c:94
#28 0x00000000006ce65a in cmd_func (cmd=0x321e270, args=0x0, from_tty=0)
at gdb/cli/cli-decode.c:2826
#29 0x0000000001005b78 in execute_command (p=0x3f48fe3 "", from_tty=0)
at gdb/top.c:564
#30 0x0000000000966095 in command_handler (
command=0x3f48fd0 "maint print symbols")
at gdb/event-top.c:613
#31 0x0000000001005141 in read_command_file (stream=0x3011a40)
at gdb/top.c:333
#32 0x00000000006e2a64 in script_from_file (stream=0x3011a40,
file=0x7fffffffe21f "gdb.in")
at gdb/cli/cli-script.c:1705
#33 0x00000000006bb88c in source_script_from_stream (stream=0x3011a40,
file=0x7fffffffe21f "gdb.in", file_to_open=0x7fffffffd760 "gdb.in")
at gdb/cli/cli-cmds.c:706
#34 0x00000000006bba12 in source_script_with_search (
file=0x7fffffffe21f "gdb.in", from_tty=0, search_path=0)
at gdb/cli/cli-cmds.c:751
#35 0x00000000006bbab2 in source_script (file=0x7fffffffe21f "gdb.in",
from_tty=0) at gdb/cli/cli-cmds.c:760
#36 0x0000000000b835cb in catch_command_errors (
command=0x6bba7e <source_script(char const*, int)>,
arg=0x7fffffffe21f "gdb.in", from_tty=0, do_bp_actions=false)
at gdb/main.c:510
#37 0x0000000000b83803 in execute_cmdargs (cmdarg_vec=0x7fffffffd980,
file_type=CMDARG_FILE, cmd_type=CMDARG_COMMAND, ret=0x7fffffffd8c8)
at gdb/main.c:606
#38 0x0000000000b84d79 in captured_main_1 (context=0x7fffffffdb90)
at gdb/main.c:1349
#39 0x0000000000b84fe4 in captured_main (context=0x7fffffffdb90)
at gdb/main.c:1372
#40 0x0000000000b85092 in gdb_main (args=0x7fffffffdb90)
at gdb/main.c:1401
#41 0x000000000041a382 in main (argc=9, argv=0x7fffffffdcc8)
at gdb/gdb.c:38
(gdb)
...
The immediate problem is in dwarf_expr_context::fetch_result where we're
calling get_frame_arch:
...
switch (this->m_location)
{
case DWARF_VALUE_REGISTER:
{
gdbarch *f_arch = get_frame_arch (this->m_frame);
...
with a null frame:
...
(gdb) p this->m_frame.is_null ()
$1 = true
(gdb)
...
Fix this using ensure_have_frame in dwarf_expr_context::execute_stack_op for
DW_OP_reg<n> and DW_OP_regx, getting us instead:
...
c_to: array (<>) of character; computed at runtime
...
Tested on x86_64-linux.
Approved-By: Tom Tromey <tom@tromey.com>
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=33512
Tom Tromey [Fri, 19 Sep 2025 16:05:38 +0000 (10:05 -0600)]
Clean up "return -1" in gdb.ada
gdb tests historically used "return -1" for some kinds of failure, but
there is no reason for the "-1". This patch removes this from the
gdb.ada tests.
Andrew Burgess [Mon, 24 Jul 2023 16:35:54 +0000 (17:35 +0100)]
gdb: detect when gdbserver has no default executable set
This commit extends the use of the new qExecAndArgs packet (added in
the previous commit) so that GDB now understands when it is connected
to a remote server that doesn't have a default executable set. We
don't do much with this information right now, other than produce more
useful text for 'show remote exec-file'.
Here I've connected to a gdbserver with no default executable set,
this is with this patch in place:
(gdb) target extended-remote | gdbserver --multi --once -
(gdb) show remote exec-file
The remote exec-file is unset, the remote has no default executable set.
(gdb) file /tmp/hello.x
Reading symbols from /tmp/hello.x...
(gdb) run
Starting program: /tmp/hello.x
Running the default executable on the remote target failed; try "set remote exec-file"?
(gdb)
The important line is this one:
The remote exec-file is unset, the remote has no default executable set.
Without this patch we'd get:
The remote exec-file is unset, the default remote executable will be used.
The new message is clearer that there is no default executable set on
the remote.
In the future I plan to make use of this additional information,
coupled with an understanding (via 'set sysroot') of when gdb and
gdbserver share the same filesystem, to allow GDB to automatically use
the current executable (e.g. loaded with the 'file' command) as the
remote exec-file. But this is not part of this patch, or this patch
series, just future planned work.
Andrew Burgess [Thu, 20 Jul 2023 18:13:22 +0000 (19:13 +0100)]
gdb/gdbserver: add new qExecAndArgs packet
This commit adds a new remote protocol packet qExecAndArgs, and
updates GDB to use it.
When gdbserver is started a user can provide an executable and
arguments, these are used (by the remote target) to start an initial
inferior, this is the inferior to which GDB first connects.
When GDB is connected in extended-remote mode, if the user does a
'run' without specifying a new 'remote exec-file' then the executable
given on the gdbserver command line is reused to start the new
inferior.
Interestingly, the arguments given on the gdbserver command line are
only used when starting the first inferior, subsequent inferiors will
be passed an empty argument string by GDB. This might catch out a
user, causing the rerun to behave differently than the first run.
In this commit I will add a new qExecAndArgs packet, which I think
will improve the experience in this area.
The new qExecAndArgs packet is sent from GDB, and gdbserver replies
with a packet that includes the executable filename and the arguments
string that were used for starting the initial inferior.
On the GDB side this information can be used to update GDB's state,
the 'show remote exec-file' will reflect how gdbserver was started,
and 'show args' will reflect the arguments used for starting the
inferior.
As a result of updating the args, if the user restarts the inferior,
then this same argument string will be passed back to the remote
target, and used for the new inferior. Thus, rerunning the inferior
will behave just like the initial inferior, which I think is a good
improvement.
Finally, GDB will warn if the user has 'set remote exec-file' and
then connects to a gdbserver that was started with some alternative
filename, like this:
(gdb) set remote exec-file /tmp/foo
(gdb) target remote | gdbserver --once - /tmp/bar
... snip ...
warning: updating 'remote exec-file' to '/tmp/bar' to match remote target
... snip ...
I made the choice to have GDB update the remote exec-file setting to
match the remote, as, after the 'target remote', we are connected to
an inferior that is running /tmp/bar (in this case), so trying to hang
onto the non-matching user supplied setting doesn't seem helpful.
There is one case where I can see this choice being a problem, if a
user does:
(gdb) set remote exec-file /tmp/foo
(gdb) target extended-remote | gdbserver --multi --once - /tmp/bar
... snip ...
warning: updating 'remote exec-file' to '/tmp/bar' to match remote target
... snip ...
(gdb) run
In this case, prior to this patch, they would 'run' /tmp/foo, while
after this patch, they will run /tmp/bar. I think it is unfortunate
that I'm breaking this use case, but, I'm not _that_ sorry -- just
start gdbserver with the correct executable, or even no executable,
and the problem goes away.
This last point is important, in extended-remote mode, it is possible
to start gdbserver without specifying an executable, like this:
$ gdbserver --multi --once :54321
In this case gdbserver doesn't start an initial inferior. When GDB
connects the qExecAndArgs reply from gdbserver indicates that no
information (executable or arguments) were set, and any existing
information is retained, as in this session:
(gdb) set sysroot
(gdb) set remote exec-file /tmp/foo
(gdb) set args a b c
(gdb) target extended-remote | ./gdbserver/gdbserver --multi --once -
Remote debugging using | ./gdbserver/gdbserver --multi --once -
Remote debugging using stdio
(gdb) show remote exec-file
The remote exec-file is "/tmp/foo".
(gdb) show args
Argument list to give program being debugged when it is started is "a b c".
(gdb)
This is the second time proposing this new packet. The first attempt
can be found here:
The review feedback on this patch was that the inferior arguments
should be passed back as a vector of individual strings. This makes
sense, at the time that feedback was given, GDB would pass arguments
to gdbserver as a vector of individual arguments, so it would seem
sensible that gdbserver should adopt the same approach for passing
arguments back to GDB.
However, since then I have been working on how GDB passes the inferior
arguments to gdbserver, fixing a lot of broken corner cases, which
culminated in this patch:
gdb/gdbserver: pass inferior arguments as a single string
Though we do retain the vector of individual arguments behaviour for
backward compatibility with old remote targets, the preferred approach
now is for GDB to pass arguments to gdbserver as a single string.
This removes the need for GDB/gdbserver to try and figure out what is
the correct escaping to apply to the arguments, and fixes some
argument passing corner cases.
And so, now, I think it makes sense that gdbserver should also pass
the arguments back to GDB as a single string. I've updated the
documentation a little to (I hope) explain how gdbserver should escape
things before passing them back to GDB (TLDR: no additional escaping
should be added just for sending to GDB. The argument string should
be sent to GDB as if it were being sent to the 'set args' GDB
command).
The main test for this new functionality is
gdb.server/fetch-exec-and-args.exp, but I've also added a test
gdb.replay/fetch-exec-and-args.exp, which allows me to test a corner
case that isn't currently exercised by gdbserver, this is the case for
sending pack inferior arguments, but no executable.
The qExecAndArgs reply format is 'S;exec;args;' where 'exec' and
'args' are hex encoded strings. If 'args' is empty then this is
perfectly valid, this just means there were no command line
arguments. But what if 'exec' is empty? I needed to decide what to
do in this case. The easiest choice is to treat empty 'exec' as the
executable is not set. But currently, due to how gdbserver works, it
is not possible to hit this case, so I used the gdbreplay testing
framework to exercise this instead. There were a few supporting
changes needed to write this test though.
Reviewed-By: Eli Zaretskii <eliz@gnu.org> Approved-By: Tom Tromey <tom@tromey.com>
Andrew Burgess [Mon, 15 Sep 2025 13:20:52 +0000 (14:20 +0100)]
gdbserver: prevent assertion caused by passing empty program name
While testing another patch I'm working on I discovered that passing
an empty program name to gdbserver would trigger an assertion, like
this:
$ gdbserver --multi :54321 ""
../../gdb/gdbserver/../gdb/nat/fork-inferior.c:240: A problem internal to GDBserver has been detected.
fork_inferior: Assertion `exec_file != nullptr' failed.
User input, no matter how weird, shouldn't be triggering an assertion,
so lets fix that.
In extended mode, it is valid to start gdbserver without an executable
name, like this:
$ gdbserver --multi :54321
Here gdbserver doesn't start an inferior, and it is up to GDB to
connect, and tell gdbserver what to run, and to then start it running.
I did wonder if the empty string case should handled like the no
executable name case, but then you get into the situation where the
user can specify command line arguments without an inferior, like:
$ gdbserver --multi :54321 "" a b c
And while there's nothing really wrong with this, and I'm sure someone
could come up with a use case for it. I'd like to propose that for
now at least, we take the simple approach of not allowing an empty
executable name, instead we should give an error, like this:
$ gdbserver --multi :54321 ""
No program to debug
Exiting
We can always relax this requirement in the future, and allow the
empty executable with or without inferior arguments, if we decide
there's a compelling reason for it. It would be simple enough to add
this in the future, but once we add support for it, it's much harder
to remove the feature in the future, so lets start simple.
The non-extended remote case works much the same. It too triggers the
assertion currently, and after this patch exits with the same error.
Of course, the non-extended remote case never supported not having an
inferior, if you did:
$ gdbserver :54321
You'd be shown the usage text and gdbserver would exit.
Fix PR libsframe/33437 - libsframe test names are not unique
The TEST () macro definition originally in plt-findfre-2.c, was being
used to differentiate between multiple runs of the testcases. Adapt
that definition a bit to allow for a variable number of arguments following
the test condition: A test name format string may be used by macro
users, such that the name of the tests are unique.
Move the new variadic TEST macro definition in the testsuite's common
header sframe-test.h, and use it throughout the testsuite.
Reviewed-by: Jens Remus <jremus@linux.ibm.com>
libsframe/testsuite/
PR libsframe/33437
* libsframe.decode/be-flipping.c: Use new TEST macro with
suffix.
* libsframe.decode/frecnt-1.c: Likewise.
* libsframe.decode/frecnt-2.c: Likewise.
* libsframe.encode/encode-1.c: Likewise.
* libsframe.find/findfre-1.c: Likewise.
* libsframe.find/findfunc-1.c: Likewise.
* libsframe.find/plt-findfre-1.c: Likewise.
* libsframe.find/plt-findfre-2.c: Likewise.
* sframe-test.h: Move the TEST macro definition to this
testsuite header.
H.J. Lu [Tue, 30 Sep 2025 00:13:56 +0000 (08:13 +0800)]
x86: Keep _GLOBAL_OFFSET_TABLE_ for .eh_frame
Since x86 .eh_frame section may reference _GLOBAL_OFFSET_TABLE_, keep
_GLOBAL_OFFSET_TABLE_ if there is dynamic section and the output
.eh_frame section is non-empty.
PR ld/33499
* elfxx-x86.c (_bfd_x86_elf_late_size_sections): Keep
_GLOBAL_OFFSET_TABLE_ if there is dynamic section and the
output .eh_frame section is non-empty.
Simon Marchi [Wed, 1 Oct 2025 22:10:41 +0000 (18:10 -0400)]
gdb/doc: trim trailing whitespaces
My editor "accidentally" removed all trailing whitespaces from
gdb.texinfo while doing a change. That was mostly just an annoyance
but to avoid it happening again, I suggest removing them for good.
I look at the difference in the output of "make html". The new output
has some trailing whitespaces removed, but none of them seems to cause a
semantic difference. Not sure about other formats like info or pdf
though.
Change-Id: I3f349b28c581af69703365fea07e7b93614c987c Approved-By: Eli Zaretskii <eliz@gnu.org>
Andrew Burgess [Wed, 10 Sep 2025 16:05:26 +0000 (17:05 +0100)]
gdb: add core file name to 'info inferiors' output
This commit builds on the previous commit. In the future I am
proposing to move the core file BFD from the program_space into the
core_target. In the last commit I updated 'maint info program-spaces'
to remove the core file name from the output.
In this commit I'm adding the core file name to the 'info inferiors'
output.
My proposal is to add the core file as auxiliary information beneath
an inferior's line in the 'info inferiors' output. We already do
this vfork parent/child information.
The alternative would be to add the core file as an additional column
in the 'info inferiors' output, indeed, I did initially propose this:
But the problem with this is that the 'info inferiors' output can
easily become very long, and the line wrapping gets very messy, making
the output much harder to parse. The feedback on this initial
approach wasn't super strong, so I'm trying the auxiliary information
approach to see if this is liked more.
The new output looks like this:
(gdb) info inferiors
Num Description Connection Executable
* 1 process 54313 1 (core) /tmp/executable
core file /tmp/core.54313
The only other option I can think of, if this approach is not liked,
would be to add an entirely new command, 'info core-files', with
output like:
Num Core File
* 1 /tmp/corefile.core
The 'Num' column here would just be the inferior number again. In
effect this new command is just splitting the 'info inferiors' into
two commands.
I extended gdb.base/corefile.exp to check the current output style,
and updated the gdb.multi/multi-target-info-inferiors.exp test to take
the new output into account.
Andrew Burgess [Wed, 10 Sep 2025 09:32:48 +0000 (10:32 +0100)]
gdb: remove core file name from 'maint info program-spaces'
I'm currently working towards a goal of moving the core file BFD out
of program_space and into core_target. I believe this is a good
change to make as the core_target already holds a lot of state that is
parsed from the core file BFD, so storing the parsed, structured,
information in a different location to the original core file BFD
doesn't make sense to me.
In preparation for this change, the 'maint info program-spaces'
command needs updating. Currently this command lists the name of the
core file BFD that is loaded into each program space.
Once the core file moves into core_target then the core file really
becomes a property of the inferior.
We could try to retain the existing output by looking up which
inferior is active in a given program space, and find the core file
that way, however, I don't like this plan because GDB does support
shared program spaces, in theory, a target could exist where every
inferior shares a single program space. Even on more common POSIX
targets, after a vfork the parent and child share a program space.
Now the vfork case clearly doesn't impact the core file case, and I
don't know if GDB _actually_ supports any shared program space targets
any more.... but still, I don't think we should try to retain the
existing behaviour.
So, this commit removes the core file name from the 'maint info
program-spaces' output. The next commit will add the core file name
back in a new home.
Reviewed-By: Eli Zaretskii <eliz@gnu.org> Approved-By: Tom Tromey <tom@tromey.com>
Tom Tromey [Wed, 1 Oct 2025 16:27:15 +0000 (10:27 -0600)]
Introduce gdbsupport/cxx-thread.h and use it
This introduces a new file, gdbsupport/cxx-thread.h, which provides
stubs for the C++ threading functionality on systems that don't
support it.
On fully-working ports, this header just supplies a number of aliases
in the gdb namespace. So, for instance, gdb::mutex is just an alias
for std::mutex.
For non-working ports, compatibility stubs are provided for the subset
of threading functionality that's used in gdb. These generally do
nothing and assume single-threaded operation.
The idea behind this is to reduce the number of checks of
CXX_STD_THREAD, making the code cleaner.
Not all spots using CXX_STD_THREAD could readily be converted.
In particular:
* Unit tests
* --config output
* Code manipulating threads themselves
* The extension interrupting handling code
These all seem fine to me.
Note there's also a check in py-dap.c. This one is perhaps slightly
subtle: DAP starts threads on the Python side, but it relies on gdb
itself being thread-savvy, for instance in gdb.post_event.
Approved-By: Simon Marchi <simon.marchi@efficios.com>
Tom Tromey [Wed, 1 Oct 2025 16:27:37 +0000 (10:27 -0600)]
Don't rely on deduction in run-on-main-thread.c
This changes one spot in run-on-main-thread.c to use an explicit
template argument, rather than relying on deduction. The deduction
would otherwise fail with the next patch.
Andrew Burgess [Wed, 17 Sep 2025 12:07:09 +0000 (13:07 +0100)]
gdb: int to bool conversion in find_memory_regions API
Perform int to bool conversion for find_memory_region_ftype function
type. This function type is used in the find_memory_regions API, both
target_find_memory_regions and target_find_memory_regions.
There should be no user visible changes after this commit.
[AUTOFDO][AARCH64] Add support for profilebootstrap
Add support for autoprofiledbootstrap in aarch64.
This is similar to what is done for i386. Added
gcc/config/aarch64/gcc-auto-profile for aarch64 profile
creation.
How to run:
configure --with-build-config=bootstrap-lto
make autoprofiledbootstrap
[PATCH] configure: Always add pre-installed header directories to search path
configure script was adding the target directory flags, including the
'-B' flags for the executable prefix and the '-isystem' flags for the
pre-installed header directories, to the target flags only for
non-Canadian builds under the premise that the host binaries under the
executable prefix will not be able to execute on the build system for
Canadian builds.
While that is true for the '-B' flags specifying the executable prefix,
the '-isystem' flags specifying the pre-installed header directories are
not affected by this and do not need special handling.
This patch updates the configure script to always add the 'include' and
'sys-include' pre-installed header directories to the target search
path, in order to ensure that the availability of the pre-installed
header directories in the search path is consistent across non-Canadian
and Canadian builds.
When '--with-headers' flag is specified, this effectively ensures that
the libc headers, that are copied from the specified header directory to
the sys-include directory, are used by libstdc++.
cobol: Restrict COBOL to supported Linux arches [PR119217]
The COBOL frontend is currently built on all x86_64 and aarch64 hosts
although the code contains some Linux/glibc specifics that break the build
e.g. on Solaris/amd64.
config, toplevel, Darwin: Pass -B instead of -L to C++ commands.
Darwin from 10.11 needs embedded rpaths to find the correct libraries at
runtime. Recent increases in hardening have made it such that the dynamic
loader will no longer fall back to using an installed libstdc++ when the
(new) linked one is not found. This means we fail configure tests (that
should pass) for runtimes that use C++.
We can resolve this by passing '-B' to the C++ command lines instead of '-L'
(-B implies -L on Darwin, but also causes a corresponding embedded rpath).
bootstrap/119513 - fix cobol bootstrap with --enable-generated-files-in-srcdir
This adds gcc/cobol/parse.o to compare_exclusions and makes sure to
ignore errors when copying generated files, like it's done when
copying gengtype-lex.cc.
toplevel, libcobol: Add dependency on libquadmath build [PR119244].
For the configuration of libgcobol to be correct for targets that need
to use libquadmath for 128b FP support, we must be able to find the
quadmath library (or not, for targets that have the support in libc).
In addition to making libstdc++ itself available, this, via enabling
'build-gcc/*/libstdc++-v3/scripts/testsuite_flags', in particular also makes
the standard C++ headers available to 'make check-gcc-c++'. With that, there
are a lot of FAIL/UNRESOLVED -> PASS progressions, where we previously ran
into, for example:
FAIL: g++.dg/coroutines/co-await-syntax-00-needs-expr.C (test for errors, line 6)
FAIL: g++.dg/coroutines/co-await-syntax-00-needs-expr.C (test for excess errors)
Excess errors:
[...]/gcc/testsuite/g++.dg/coroutines/coro.h:132:10: fatal error: cstdlib: No such file or directory
Similarly, there are a lot of FAIL/UNRESOLVED -> UNSUPPORTED "progressions" due
to 'sorry, unimplemented: exception handling not supported'.
The 'make check-target-libstdc++-v3' results don't look too bad, either.
With libstdc++ now available, libgrust gets enabled, which we in turn again
have to disable, for 'sorry, unimplemented: exception handling not supported'
reasons.
Sorry, seems I've screwed up the earlier libgcobol/configure.tgt change.
Looking in more detail, the way e.g. libsanitizer/configure.tgt works is
that it is sourced twice, once at toplevel and there it just sets
UNSUPPORTED=1 for fully unsupported triplets, and then inside of
libsanitizer/configure where it decides to include or not include the
various sublibraries depending on the *_SUPPORTED flags.
So, the following patch attempts to do the same for libgcobol as well.
The BIULD_LIBGCOBOL automake conditional was unused, this patch guards it
on LIBGCOBOL_SUPPORTED as well and guards with it
toolexeclib_LTLIBRARIES = libgcobol.la
Also, AM_CFLAGS has been changed to AM_CXXFLAGS as there are just C++
sources in the library.
configure, Darwin: Require explicit selection of COBOL.
By defult, Darwin does not have sufficient tools to build COBOL
so we do not want to include it in --enable-languages=all since
this will break regular testing of all supported languages.
However, we do want to be able to build it on demand (where the
build system has sufficiently new tools) and so do not want to
disable it permanently.
..., where "support" means that the build doesn't fail, but it doesn't mean
that all target libraries get built and we get pretty test results for the
additional languages.
Since catching-syscalls was added, there had been added files containing
syscalls in xml format. As for now riscv-canonicalize-syscall-gen.py uses
glibc for generating, it may be not so comfortable. I changed this
script for reusing newly generated riscv-linux.xml file. Also, I renamed
riscv64_canonicalize_syscall to riscv_linux_canonicalize_syscall as only 64
system is supported in linux. This will simplify the possible further
generalization of this script to other architectures.
Approved-By: Simon Marchi <simon.marchi@efficios.com>
H.J. Lu [Tue, 30 Sep 2025 03:23:58 +0000 (11:23 +0800)]
binutils: Use AC_TRY_COMPILE to check target clang/gcc
Use AC_TRY_COMPILE to check for the working target clang and gcc when
configuring for cross tools.
PR binutils/33503
* configure: Regenerated.
config/
PR binutils/33503
* clang-plugin.m4 (CLANG_PLUGIN_FILE_FOR_TARGET): Use
AC_TRY_COMPILE to check the target clang and replace
clang_cv_is_clang with clang_target_cv_working.
* gcc-plugin.m4 (GCC_PLUGIN_OPTION_FOR_TARGET): Use
AC_TRY_COMPILE to check the target gcc.
Simon Marchi [Fri, 19 Sep 2025 20:27:05 +0000 (16:27 -0400)]
gdb/dwarf: use dynamic partitioning for DWARF CU indexing
The DWARF indexer splits the work statically based on the unit sizes,
attempting to give each worker thread about the same amount of bytes to
process. This works relatively well with standard compilation. But
when compiling with DWO files (-gsplit-dwarf), it's not as good. I see
this when loading a relatively big program (telegram-desktop, which
includes a lot of static dependencies) compiled with -gsplit-dwarf:
Time for "DWARF indexing worker": wall 0.000, user 0.000, sys 0.000, user+sys 0.000, -nan % CPU
Time for "DWARF indexing worker": wall 0.001, user 0.000, sys 0.000, user+sys 0.000, 0.0 % CPU
Time for "DWARF indexing worker": wall 0.001, user 0.001, sys 0.000, user+sys 0.001, 100.0 % CPU
Time for "DWARF indexing worker": wall 0.748, user 0.284, sys 0.297, user+sys 0.581, 77.7 % CPU
Time for "DWARF indexing worker": wall 0.818, user 0.408, sys 0.262, user+sys 0.670, 81.9 % CPU
Time for "DWARF indexing worker": wall 1.196, user 0.580, sys 0.402, user+sys 0.982, 82.1 % CPU
Time for "DWARF indexing worker": wall 1.250, user 0.511, sys 0.500, user+sys 1.011, 80.9 % CPU
Time for "DWARF indexing worker": wall 7.730, user 5.891, sys 1.729, user+sys 7.620, 98.6 % CPU
Note how the wall times vary from 0 to 7.7 seconds. This is
undesirable, because the time to do that indexing step takes as long as
the slowest worker thread takes.
The imbalance in this step also causes imbalance in the following
"finalize" step:
Time for "DWARF finalize worker": wall 0.007, user 0.004, sys 0.002, user+sys 0.006, 85.7 % CPU
Time for "DWARF finalize worker": wall 0.012, user 0.005, sys 0.005, user+sys 0.010, 83.3 % CPU
Time for "DWARF finalize worker": wall 0.015, user 0.010, sys 0.004, user+sys 0.014, 93.3 % CPU
Time for "DWARF finalize worker": wall 0.389, user 0.359, sys 0.029, user+sys 0.388, 99.7 % CPU
Time for "DWARF finalize worker": wall 0.680, user 0.644, sys 0.035, user+sys 0.679, 99.9 % CPU
Time for "DWARF finalize worker": wall 0.929, user 0.907, sys 0.020, user+sys 0.927, 99.8 % CPU
Time for "DWARF finalize worker": wall 1.093, user 1.055, sys 0.037, user+sys 1.092, 99.9 % CPU
Time for "DWARF finalize worker": wall 2.016, user 1.934, sys 0.082, user+sys 2.016, 100.0 % CPU
Time for "DWARF finalize worker": wall 25.882, user 25.471, sys 0.404, user+sys 25.875, 100.0 % CPU
With DWO files, the split of the workload by size doesn't work, because
it is done using the size of the skeleton units in the main file, which
is not representative of how much DWARF is contained in each DWO file.
I haven't tried it, but a similar problem could occur with cross-unit
imports, which can happen with dwz or LTO. You could have a small unit
that imports a lot from other units, in which case the size of the units
is not representative of the work to accomplish.
To try to improve this situation, change the DWARF indexer to use
dynamic partitioning, using gdb::parallel_for_each_async. With this,
each worker thread pops one unit at a time from a shared work queue to
process it, until the queue is empty. That should result in a more
balance workload split. I chose 1 as the minimum batch size here,
because I judged that indexing one CU was a big enough piece of work
compared to the synchronization overhead of the queue. That can always
be tweaked later if someone wants to do more tests.
As a result, the timings are much more balanced:
Time for "DWARF indexing worker": wall 2.325, user 1.033, sys 0.573, user+sys 1.606, 69.1 % CPU
Time for "DWARF indexing worker": wall 2.326, user 1.028, sys 0.568, user+sys 1.596, 68.6 % CPU
Time for "DWARF indexing worker": wall 2.326, user 1.068, sys 0.513, user+sys 1.581, 68.0 % CPU
Time for "DWARF indexing worker": wall 2.326, user 1.005, sys 0.579, user+sys 1.584, 68.1 % CPU
Time for "DWARF indexing worker": wall 2.326, user 1.070, sys 0.516, user+sys 1.586, 68.2 % CPU
Time for "DWARF indexing worker": wall 2.326, user 1.063, sys 0.584, user+sys 1.647, 70.8 % CPU
Time for "DWARF indexing worker": wall 2.326, user 1.049, sys 0.550, user+sys 1.599, 68.7 % CPU
Time for "DWARF indexing worker": wall 2.328, user 1.058, sys 0.541, user+sys 1.599, 68.7 % CPU
...
Time for "DWARF finalize worker": wall 2.833, user 2.791, sys 0.040, user+sys 2.831, 99.9 % CPU
Time for "DWARF finalize worker": wall 2.939, user 2.896, sys 0.043, user+sys 2.939, 100.0 % CPU
Time for "DWARF finalize worker": wall 3.016, user 2.969, sys 0.046, user+sys 3.015, 100.0 % CPU
Time for "DWARF finalize worker": wall 3.076, user 2.957, sys 0.118, user+sys 3.075, 100.0 % CPU
Time for "DWARF finalize worker": wall 3.159, user 3.054, sys 0.104, user+sys 3.158, 100.0 % CPU
Time for "DWARF finalize worker": wall 3.198, user 3.082, sys 0.114, user+sys 3.196, 99.9 % CPU
Time for "DWARF finalize worker": wall 3.197, user 3.076, sys 0.121, user+sys 3.197, 100.0 % CPU
Time for "DWARF finalize worker": wall 3.268, user 3.136, sys 0.131, user+sys 3.267, 100.0 % CPU
Time for "DWARF finalize worker": wall 1.907, user 1.810, sys 0.096, user+sys 1.906, 99.9 % CPU
In absolute terms, the total time for GDB to load the file and exit goes
down from about 42 seconds to 17 seconds.
Some implementation notes:
- The state previously kept in as local variables in
cooked_index_worker_debug_info::process_units becomes fields of the
new parallel worker object.
- The work previously done for each unit in
cooked_index_worker_debug_info::process_units becomes the operator()
of the new parallel worker object.
- The work previously done at the end of
cooked_index_worker_debug_info::process_units (including calling
bfd_thread_cleanup) becomes the destructor of the new parallel worker
object.
- The "done" callback of gdb::task_group becomes the "done" callback of
gdb::parallel_for_each_async.
- I placed the parallel_indexing_worker struct inside
cooked_index_worker_debug_info, so that it has access to
parallel_indexing_worker's private fields (e.g. m_results, to push
the results). It will also be possible to re-use it for skeletonless
type units in a later patch.
Change-Id: I5dc5cf8793abe9ebe2659e78da38ffc94289e5f2 Approved-By: Tom Tromey <tom@tromey.com>
Simon Marchi [Fri, 19 Sep 2025 20:27:04 +0000 (16:27 -0400)]
gdbsupport: add async parallel_for_each version
I would like to use gdb::parallel_for_each to implement the parallelism
of the DWARF unit indexing. However, the existing implementation of
gdb::parallel_for_each is blocking, which doesn't work with the model
used by the DWARF indexer, which is asynchronous and callback-based.
Add an asynchronouys version of gdb::parallel_for_each that will be
suitable for this task.
This new version accepts a callback that is invoked when the parallel
for each is complete.
This function uses the same strategy as gdb::task_group to invoke the
"done" callback: worker threads have a shared_ptr reference to some
object. The last worker thread to drop its reference causes the object
to be deleted, which invokes the callback.
Unlike for the sync version of gdb::parallel_for_each, it's not possible
to keep any state in the calling thread's stack, because that disappears
immediately after starting the workers. So all the state is kept in
that same shared object.
There is a limitation that the sync version doesn't have, regarding the
arguments you can pass to the worker objects: it's not possibly to rely
on references. There are more details in a comment in the code.
It would be possible to implement the sync version of
gdb::parallel_for_each on top of the async version, but I decided not to
do it to avoid the unnecessary dynamic allocation of the shared object,
and to avoid adding the limitations on passing references I mentioned
just above. But if we judge that it would be an acceptable cost to
avoid the duplication, we could do it.
Add a self test for the new function.
Change-Id: I6173defb1e09856d137c1aa05ad51cbf521ea0b0 Approved-By: Tom Tromey <tom@tromey.com>
Simon Marchi [Fri, 19 Sep 2025 20:27:02 +0000 (16:27 -0400)]
gdbsupport: use iterator range in parallel_for_each interface
I think it would be convenient for parallel_for_each to pass an
iterator_range to the worker function, instead of separate begin and end
parameters. This allows using a ranged for loop directly.
Change-Id: I8f9681da65b0eb00b738379dfd2f4dc6fb1ee612 Approved-By: Tom Tromey <tom@tromey.com>
Simon Marchi [Fri, 19 Sep 2025 20:27:00 +0000 (16:27 -0400)]
gdbsupport: use dynamic partitioning in gdb::parallel_for_each
gdb::parallel_for_each uses static partitioning of the workload, meaning
that each worker thread receives a similar number of work items. Change
it to use dynamic partitioning, where worker threads pull work items
from a shared work queue when they need to.
Note that gdb::parallel_for_each is currently only used for processing
minimal symbols in GDB. I am looking at improving the startup
performance of GDB, where the minimal symbol process is one step.
With static partitioning, there is a risk of workload imbalance if some
threads receive "easier" work than others. Some threads sit still while
others finish working on their share of the work. This is not
desirable, because the gdb::parallel_for_each takes as long as the
slowest thread takes.
When loading a file with a lot of minimal symbols (~600k) in GDB, with
"maint set per-command time on", I observe some imbalance:
Time for "minsyms install worker": wall 0.732, user 0.550, sys 0.041, user+sys 0.591, 80.7 % CPU
Time for "minsyms install worker": wall 0.881, user 0.722, sys 0.071, user+sys 0.793, 90.0 % CPU
Time for "minsyms install worker": wall 2.107, user 1.804, sys 0.147, user+sys 1.951, 92.6 % CPU
Time for "minsyms install worker": wall 2.351, user 2.003, sys 0.151, user+sys 2.154, 91.6 % CPU
Time for "minsyms install worker": wall 2.611, user 2.322, sys 0.235, user+sys 2.557, 97.9 % CPU
Time for "minsyms install worker": wall 3.074, user 2.729, sys 0.203, user+sys 2.932, 95.4 % CPU
Time for "minsyms install worker": wall 3.486, user 3.074, sys 0.260, user+sys 3.334, 95.6 % CPU
Time for "minsyms install worker": wall 3.927, user 3.475, sys 0.336, user+sys 3.811, 97.0 % CPU
^
----´
The fastest thread took 0.732 seconds to complete its work (and then sat
still), while the slowest took 3.927 seconds. This means the
parallel_for_each took a bit less than 4 seconds.
Even if the number of minimal symbols assigned to each worker is the
same, I suppose that some symbols (e.g. those that need demangling) take
longer to process, which could explain the imbalance.
With this patch, things are much more balanced:
Time for "minsym install worker": wall 2.807, user 2.222, sys 0.144, user+sys 2.366, 84.3 % CPU
Time for "minsym install worker": wall 2.808, user 2.073, sys 0.131, user+sys 2.204, 78.5 % CPU
Time for "minsym install worker": wall 2.804, user 1.994, sys 0.151, user+sys 2.145, 76.5 % CPU
Time for "minsym install worker": wall 2.808, user 1.977, sys 0.135, user+sys 2.112, 75.2 % CPU
Time for "minsym install worker": wall 2.808, user 2.061, sys 0.142, user+sys 2.203, 78.5 % CPU
Time for "minsym install worker": wall 2.809, user 2.012, sys 0.146, user+sys 2.158, 76.8 % CPU
Time for "minsym install worker": wall 2.809, user 2.178, sys 0.137, user+sys 2.315, 82.4 % CPU
Time for "minsym install worker": wall 2.820, user 2.141, sys 0.170, user+sys 2.311, 82.0 % CPU
^
----´
In this version, the parallel_for_each took about 2.8 seconds,
representing a reduction of ~1.2 seconds for this step. Not
life-changing, but it's still good I think.
Note that this patch helps when loading big programs. My go-to test
program for this is telegram-desktop that I built from source. For
small programs (including loading gdb itself), it makes no perceptible
difference.
Now the technical bits:
- One impact that this change has on the minimal symbol processing
specifically is that not all calls to compute_and_set_names (a
critical region guarded by a mutex) are done at the end of each
worker thread's task anymore.
Before this patch, each thread would compute the names and hash values for
all the minimal symbols it has been assigned, and then would call
compute_and_set_names for all of them, while holding the mutex (thus
preventing other threads from doing this same step).
With the shared work queue approach, each thread grabs a batch of of
minimal symbols, computes the names and hash values for them, and
then calls compute_and_set_names (with the mutex held) for this batch
only. It then repeats that until the work queue is empty.
There are therefore more small and spread out compute_and_set_names
critical sections, instead of just one per worker thread at the end.
Given that before this patch the work was not well balanced among worker
threads, I guess that threads would enter that critical region at
roughly different times, causing little contention.
In the "with this patch" results, the CPU utilization numbers are not
as good, suggesting that there is some contention. But I don't know
if it's contention due to the compute_and_set_names critical section
or the shared work queue critical section. That can be investigated
later. In any case, what ultimately counts is the wall time, which
improves.
- One choice I had to make was to decide how many work items (in this
case minimal symbols) each worker should pop when getting work from
the shared queue. The general wisdom is that:
- popping too few items, and the synchronization overhead becomes
significant, and the total processing time increases
- popping too many items, and we get some imbalance back, and the
total processing time increases again
I experimented using a dynamic batch size proportional to the number
of remaining work items. It worked well in some cases but not
always. So I decided to keep it simple, with a fixed batch size.
That can always be tweaked later.
- I want to still be able to use scoped_time_it to measure the time
that each worker thread spent working on the task. I find it really
handy when measuring the performance impact of changes.
Unfortunately, the current interface of gdb::parallel_for_each, which
receives a simple callback, is not well-suited for that, once I
introduce the dynamic partitioning. The callback would get called
once for each work item batch (multiple time for each worker thread),
so it's not possible to maintain a per-worker thread object for the
duration of the parallel for.
To allow this, I changed gdb::parallel_for_each to receive a worker
type as a template parameter. Each worker thread creates one local
instance of that type, and calls operator() on it for each work item
batch. By having a scoped_time_it object as a field of that worker,
we can get the timings per worker thread.
The drawbacks of this approach is that we must now define the
parallel task in a separate class and manually capture any context we
need as fields of that class.
Change-Id: Ibf1fea65c91f76a95b9ed8f706fd6fa5ef52d9cf Approved-By: Tom Tromey <tom@tromey.com>
Simon Marchi [Fri, 19 Sep 2025 20:26:59 +0000 (16:26 -0400)]
gdbsupport: re-work parallel_for_each test, again
I started working on this patch because I noticed that this
parallel_for_each test:
/* Check that if there are fewer tasks than threads, then we won't
end up with a null result. */
is not really checking anything. And then, this patch ended with
several changes, leading to general refactor of the whole file.
This test verifies, using std::all_of, that no entry in the intresults
vector is nullptr. However, nothing ever adds anything to intresults.
Since the vector is always empty, std::all_of is always true. This
state probably dates back to afdd1366358c ("Back out some
parallel_for_each features"), which removed the ability for
parallel_for_each to return a vector of results. That commit removed
some tests, but left this one in, I'm guessing as an oversight.
One good idea in this test is to check that the worker never receives
empty ranges. I think we should always test for that. I think it's
also a good idea to test with exactly one item, that's a good edge case.
To achieve this without adding some more code duplication, factor out
the core functionality of the test in yet another test_one function (I'm
running out of ideas for names). In there, check that the range
received by the worker is not empty. Doing this pointed out that the
worker is actually called with empty ranges in some cases, necessitating
some minor changes in parallel-for.h.
Then, instead of only checking that the sum of the ranges received by
worker functions is the right count, save the elements received as part
of those ranges (in a vector), and check that this vector contains each
expected element exactly once. This should make the test a bit more
robust (otherwise we could have the right number of calls, but on the
wrong items).
Then, a subsequent patch in this series changes the interface or
parallel_for_each to use iterator_range. The only hiccup is that it
doesn't really work if the "RandomIt" type of the parallel_for_each is
"int". iterator_range<int>::size wouldn't work, as std::distance
doesn't work on two ints. Fix this in the test right away by building
an std::vector<int> to use as input.
Finally, run the test with the default thread pool thread count in
addition to counts 0, 1 an 3, currently tested. I'm thinking that it
doesn't hurt to test parallel_for_each in the configuration that it's
actually used with.
Change-Id: I5adf3d61e6ffe3bc249996660f0a34b281490d54 Approved-By: Tom Tromey <tom@tromey.com>
Tom de Vries [Tue, 30 Sep 2025 19:32:50 +0000 (21:32 +0200)]
[gdb/testsuite, tclint] Further tclint fixing
Currently stable tclint (v6.0.1) as used in pre-commit doesn't check code that
is passed as arguments to commands specific to the gdb testsuite [1], like
with_test_prefix:
...
with_test_prefix foo {
...
}
...
I wrote a rudimentary tclint patch handling this, skipping the dwarf assembler
procs.
Tom de Vries [Tue, 30 Sep 2025 16:45:44 +0000 (18:45 +0200)]
[pre-commit] Set default_stages to pre-commit
I realized I was seeing the newly added tclint check twice:
...
$ touch gdb/testsuite/gdb.base/foo.exp
$ git add gdb/testsuite/gdb.base/foo.exp
$ git commit -a -m foo 2>&1 | grep tclint
tclint..................................................................Passed
tclint..............................................(no files to check)Skipped
$
...
The hook is run once for stage pre-commit, and once for stage commit-msg.
Since the hook doesn't specify a stage at which it's supposed to be run, it
takes its default from default_stages, which defaults to all stages.
Fix this by setting default_stages to pre-commit:
...
$ git commit -a -m foo 2>&1 | grep tclint
tclint..................................................................Passed
$
...
The only hook sofar that needs a different stage than pre-commit is
codespell-log, and it's not affected by this change because it has an explicit
"stages: [commit-msg]" setting.
Tom Tromey [Fri, 4 Apr 2025 04:13:00 +0000 (22:13 -0600)]
Fix bug in gdb.lookup_type
gdb.lookup_type accepts a 'block' argument, but in some cases does not
use it. This can cause the wrong type to be returned.
This patch fixes the problem by simply passing the block through. I
have no idea why it worked the way it did, and there weren't any tests
for the 'block' parameter. (I didn't look at git blame out of fear
that it was my patch back in the day.)
Tom de Vries [Tue, 30 Sep 2025 13:36:05 +0000 (15:36 +0200)]
[gdb/testsuite] Fix gdb.tui/pr30056.exp arrow right test
On aarch64-linux, I run into:
...
FAIL: gdb.tui/pr30056.exp: arrow right
...
because while the intention is to observe the change from:
...
| 20 main (void) |
...
into:
...
| 20 ain (void) |
...
we're actually looking at another line.
Fix this by looking at the contents of the entire source window.
projects / thirdparty / binutils-gdb.git / log
