This commit is the result of running the gdb/copyright.py script,
which automated the update of the copyright year range for all
source files managed by the GDB project to be updated to include
year 2023.
[I sent this earlier today, but I don't see it in the archives.
Resending it through a different computer / SMTP.]
The use of the static buffer in command_line_input is becoming
problematic, as explained here [1]. In short, with this patch [2] that
attempt to fix a post-hook bug, when running gdb.base/commands.exp, we
hit a case where we read a "define" command line from a script file
using command_command_line_input. The command line is stored in
command_line_input's static buffer. Inside the define command's
execution, we read the lines inside the define using command_line_input,
which overwrites the define command, in command_line_input's static
buffer. After the execution of the define command, execute_command does
a command look up to see if a post-hook is registered. For that, it
uses a now stale pointer that used to point to the define command, in
the static buffer, causing a use-after-free. Note that the pointer in
execute_command points to the dynamically-allocated buffer help by the
static buffer in command_line_input, not to the static object itself,
hence why we see a use-after-free.
Fix that by removing the static buffer. I initially changed
command_line_input and other related functions to return an std::string,
which is the obvious but naive solution. The thing is that some callees
don't need to return an allocated string, so this this an unnecessary
pessimization. I changed it to passing in a reference to an std::string
buffer, which the callee can use if it needs to return
dynamically-allocated content. It fills the buffer and returns a
pointers to the C string inside. The callees that don't need to return
dynamically-allocated content simply don't use it.
So, it started with modifying command_line_input as described above, all
the other changes derive directly from that.
One slightly shady thing is in handle_line_of_input, where we now pass a
pointer to an std::string's internal buffer to readline's history_value
function, which takes a `char *`. I'm pretty sure that this function
does not modify the input string, because I was able to change it (with
enough massaging) to take a `const char *`.
A subtle change is that we now clear a UI's line buffer using a
SCOPE_EXIT in command_line_handler, after executing the command.
This was previously done by this line in handle_line_of_input:
/* We have a complete command line now. Prepare for the next
command, but leave ownership of memory to the buffer . */
cmd_line_buffer->used_size = 0;
I think the new way is clearer.
[1] https://inbox.sourceware.org/gdb-patches/becb8438-81ef-8ad8-cc42-fcbfaea8cddd@simark.ca/
[2] https://inbox.sourceware.org/gdb-patches/20221213112241.621889-1-jan.vrany@labware.com/
Change-Id: I8fc89b1c69870c7fc7ad9c1705724bd493596300
Reviewed-By: Tom Tromey <tom@tromey.com>
This renames the fields of cli_interp_base::saved_output_files, as
requested by Simon. I tried to choose names that more obviously
reflect what the field is used for. I also added a couple of
comments.
Approved-By: Simon Marchi <simon.marchi@efficios.com>
When using the "set logging" commands, cli_interp_base::set_logging
will send gdb_stdlog output (among others) to the tee it makes for
gdb_stdout. However, this has the side effect of also causing logging
to use the pager. This is PR gdb/29787.
This patch fixes the problem by keeping stderr and stdlog separate
from stdout, preserving the rule that only gdb_stdout should page.
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=29787
Right now, tee_file owns the second stream it writes to. This is done
for the convenience of the users. In a subsequent patch, this will no
longer be convenient, so this patch moves the responsibility for
ownership to the users of tee_file.
CLI redirect uses a global variable, 'saved_output'. However, globals
are generally bad, and there is no need for this one -- it can be a
member of cli_interp_base. This patch makes this change.
PR cli/29800 points out that "document" will now crash when the
argument is an undefined command. This is a regression due to the
"document user-defined aliases" patch.
Approved-By: Joel Brobecker <brobecker@adacore.com>
Reviewed-By: Philippe Waroquiers <philippe.waroquiers@skynet.be>
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=29800
Consider a hello world a.out, started using gdbserver:
...
$ gdbserver --once 127.0.0.1:2345 ./a.out
Process ./a.out created; pid = 15743
Listening on port 2345
...
that we can connect to using gdb:
...
$ gdb -ex "target remote 127.0.0.1:2345"
Remote debugging using 127.0.0.1:2345
Reading /home/vries/a.out from remote target...
...
0x00007ffff7dd4550 in _start () from target:/lib64/ld-linux-x86-64.so.2
(gdb)
...
After that, we can for instance quit with confirmation:
...
(gdb) quit
A debugging session is active.
Inferior 1 [process 16691] will be killed.
Quit anyway? (y or n) y
$
...
Or, kill with confirmation and quit:
...
(gdb) kill
Kill the program being debugged? (y or n) y
[Inferior 1 (process 16829) killed]
(gdb) quit
$
...
Or, monitor exit, kill with confirmation, and quit:
...
(gdb) monitor exit
(gdb) kill
Kill the program being debugged? (y or n) y
Remote connection closed
(gdb) quit
$
...
But when doing monitor exit followed by quit with confirmation, we get the gdb
prompt back, requiring us to do quit once more:
...
(gdb) monitor exit
(gdb) quit
A debugging session is active.
Inferior 1 [process 16944] will be killed.
Quit anyway? (y or n) y
Remote connection closed
(gdb) quit
$
...
So, the first quit didn't quit. This happens as follows:
- quit_command calls query_if_trace_running
- a TARGET_CLOSE_ERROR is thrown
- it's caught in remote_target::get_trace_status, but then
rethrown because it's TARGET_CLOSE_ERROR
- catch_command_errors catches the error, at which point the quit command
has been aborted.
The TARGET_CLOSE_ERROR is defined as:
...
/* Target throwing an error has been closed. Current command should be
aborted as the inferior state is no longer valid. */
TARGET_CLOSE_ERROR,
...
so in a way this is expected behaviour. But aborting quit because the inferior
state (which we've already confirmed we're not interested in) is no longer
valid, and having to type quit again seems pointless.
Furthermore, the purpose of not catching errors thrown by
query_if_trace_running as per commit 2f9d54cfce ("make -gdb-exit call
disconnect_tracing too, and don't lose history if the target errors on
"quit""), was to make sure that error (_("Not confirmed.") had effect.
Fix this in quit_command by catching only the TARGET_CLOSE_ERROR exception
during query_if_trace_running and reporting it:
...
(gdb) monitor exit
(gdb) quit
A debugging session is active.
Inferior 1 [process 19219] will be killed.
Quit anyway? (y or n) y
Remote connection closed
$
...
Tested on x86_64-linux.
PR server/15746
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=15746
Approved-By: Tom Tromey <tom@tromey.com>
Currently, every internal_error call must be passed __FILE__/__LINE__
explicitly, like:
internal_error (__FILE__, __LINE__, "foo %d", var);
The need to pass in explicit __FILE__/__LINE__ is there probably
because the function predates widespread and portable variadic macros
availability. We can use variadic macros nowadays, and in fact, we
already use them in several places, including the related
gdb_assert_not_reached.
So this patch renames the internal_error function to something else,
and then reimplements internal_error as a variadic macro that expands
__FILE__/__LINE__ itself.
The result is that we now should call internal_error like so:
internal_error ("foo %d", var);
Likewise for internal_warning.
The patch adjusts all calls sites. 99% of the adjustments were done
with a perl/sed script.
The non-mechanical changes are in gdbsupport/errors.h,
gdbsupport/gdb_assert.h, and gdb/gdbarch.py.
Approved-By: Simon Marchi <simon.marchi@efficios.com>
Change-Id: Ia6f372c11550ca876829e8fd85048f4502bdcf06
I looked at all the spots using value_mark, and converted all the
straightforward ones to use scoped_value_mark instead.
Regression tested on x86-64 Fedora 34.
This changes GDB to use frame_info_ptr instead of frame_info *
The substitution was done with multiple sequential `sed` commands:
sed 's/^struct frame_info;/class frame_info_ptr;/'
sed 's/struct frame_info \*/frame_info_ptr /g' - which left some
issues in a few files, that were manually fixed.
sed 's/\<frame_info \*/frame_info_ptr /g'
sed 's/frame_info_ptr $/frame_info_ptr/g' - used to remove whitespace
problems.
The changed files were then manually checked and some 'sed' changes
undone, some constructors and some gets were added, according to what
made sense, and what Tromey originally did
Co-Authored-By: Bruno Larsen <blarsen@redhat.com>
Approved-by: Tom Tomey <tom@tromey.com>
This commit changes the format of 'disassemble /r' to match GNU
objdump. Specifically, GDB will now display the instruction bytes in
as 'objdump --wide --disassemble' does.
Here is an example for RISC-V before this patch:
(gdb) disassemble /r 0x0001018e,0x0001019e
Dump of assembler code from 0x1018e to 0x1019e:
0x0001018e <call_me+66>: 03 26 84 fe lw a2,-24(s0)
0x00010192 <call_me+70>: 83 25 c4 fe lw a1,-20(s0)
0x00010196 <call_me+74>: 61 65 lui a0,0x18
0x00010198 <call_me+76>: 13 05 85 6a addi a0,a0,1704
0x0001019c <call_me+80>: f1 22 jal 0x10368 <printf>
End of assembler dump.
And here's an example after this patch:
(gdb) disassemble /r 0x0001018e,0x0001019e
Dump of assembler code from 0x1018e to 0x1019e:
0x0001018e <call_me+66>: fe842603 lw a2,-24(s0)
0x00010192 <call_me+70>: fec42583 lw a1,-20(s0)
0x00010196 <call_me+74>: 6561 lui a0,0x18
0x00010198 <call_me+76>: 6a850513 addi a0,a0,1704
0x0001019c <call_me+80>: 22f1 jal 0x10368 <printf>
End of assembler dump.
There are two differences here. First, the instruction bytes after
the patch are grouped based on the size of the instruction, and are
byte-swapped to little-endian order.
Second, after the patch, GDB now uses the bytes-per-line hint from
libopcodes to add whitespace padding after the opcode bytes, this
means that in most cases the instructions are nicely aligned.
It is still possible for a very long instruction to intrude into the
disassembled text space. The next example is x86-64, before the
patch:
(gdb) disassemble /r main
Dump of assembler code for function main:
0x0000000000401106 <+0>: 55 push %rbp
0x0000000000401107 <+1>: 48 89 e5 mov %rsp,%rbp
0x000000000040110a <+4>: c7 87 d8 00 00 00 01 00 00 00 movl $0x1,0xd8(%rdi)
0x0000000000401114 <+14>: b8 00 00 00 00 mov $0x0,%eax
0x0000000000401119 <+19>: 5d pop %rbp
0x000000000040111a <+20>: c3 ret
End of assembler dump.
And after the patch:
(gdb) disassemble /r main
Dump of assembler code for function main:
0x0000000000401106 <+0>: 55 push %rbp
0x0000000000401107 <+1>: 48 89 e5 mov %rsp,%rbp
0x000000000040110a <+4>: c7 87 d8 00 00 00 01 00 00 00 movl $0x1,0xd8(%rdi)
0x0000000000401114 <+14>: b8 00 00 00 00 mov $0x0,%eax
0x0000000000401119 <+19>: 5d pop %rbp
0x000000000040111a <+20>: c3 ret
End of assembler dump.
Most instructions are aligned, except for the very long instruction.
Notice too that for x86-64 libopcodes doesn't request that GDB group
the instruction bytes. This matches the behaviour of objdump.
In case the user really wants the old behaviour, I have added a new
modifier 'disassemble /b', this displays the instruction byte at a
time. For x86-64, which never groups instruction bytes, /b and /r are
equivalent, but for RISC-V, using /b gets the old layout back (except
that the whitespace for alignment is still present). Consider our
original RISC-V example, this time using /b:
(gdb) disassemble /b 0x0001018e,0x0001019e
Dump of assembler code from 0x1018e to 0x1019e:
0x0001018e <call_me+66>: 03 26 84 fe lw a2,-24(s0)
0x00010192 <call_me+70>: 83 25 c4 fe lw a1,-20(s0)
0x00010196 <call_me+74>: 61 65 lui a0,0x18
0x00010198 <call_me+76>: 13 05 85 6a addi a0,a0,1704
0x0001019c <call_me+80>: f1 22 jal 0x10368 <printf>
End of assembler dump.
Obviously, this patch is a potentially significant change to the
behaviour or /r. I could have added /b with the new behaviour and
left /r alone. However, personally, I feel the new behaviour is
significantly better than the old, hence, I made /r be what I consider
the "better" behaviour.
The reason I prefer the new behaviour is that, when I use /r, I almost
always want to manually decode the instruction for some reason, and
having the bytes displayed in "instruction order" rather than memory
order, just makes this easier.
The 'record instruction-history' command also takes a /r modifier, and
has been modified in the same way as disassemble; /r gets the new
behaviour, and /b has been added to retain the old behaviour.
Finally, the MI command -data-disassemble, is unchanged in behaviour,
this command now requests the raw bytes of the instruction, which is
equivalent to the /b modifier. This means that the MI output will
remain backward compatible.
Compared to the previous version, this version fixes the comments reported by
Tom Tromey and ensures that the 'help some-user-documented-alias'
shows the alias definition to ensure the user understands this is an
alias even if specifically documented.
When using 'help ALIASNAME', GDB shows the help of the aliased command.
This is a good default behaviour.
However, GDB alias command allows to define aliases with arguments
possibly changing or tuning significantly the behaviour of
the aliased command. In such a case, showing the help of the aliased
command might not be ideal.
This is particularly true when defining an alias as a set of
nested 'with' followed by a last command to launch, such as:
(gdb) alias pp10 = with print pretty -- with print elements 10 -- print
Asking 'help pp10' shows the help of the 'with' command, which is
not particularly useful:
(gdb) help pp10
with, pp10, w
alias pp10 = with print pretty -- with print elements 10 -- print
Temporarily set SETTING to VALUE, run COMMAND, and restore SETTING.
Usage: with SETTING [VALUE] [-- COMMAND]
....
Such an alias can now be documented by the user:
(gdb) document pp10
>Pretty printing an expressiong, printing 10 elements.
>Usage: pp10 [PRINT-COMMAND-OPTIONS] EXP
>See 'help print' for more information.
>end
(gdb) help pp10
alias pp10 = with print pretty -- with print elements 10 -- print
Pretty printing an expressiong, printing 10 elements.
Usage: pp10 [PRINT-COMMAND-OPTIONS] EXP
See 'help print' for more information.
(gdb)
When a user-defined alias is documented specifically, help and apropos
use the provided alias documentation instead of the documentation of
the aliased command.
Such a documented alias is also not shown anymore in the help of the
aliased command, and the alias is not listed anymore in the help
of the aliased command. In particular for cases such as pp10 example above,
indicating that pp10 is an alias of the 'with' command is confusing.
I noticed a couple of initialization functions that aren't really
needed, and that currently require explicit calls in gdb_init. This
patch removes these functions, simplifying gdb a little.
Regression tested on x86-64 Fedora 34.
This commit extends GDB to make use of libopcodes styling support
where available, currently this is just i386 based architectures, and
RISC-V.
For architectures that don't support styling using libopcodes GDB will
fall back to using the Python Pygments package, when the package is
available.
The new libopcodes based styling has the disassembler identify parts
of the disassembled instruction, e.g. registers, immediates,
mnemonics, etc, and can style these components differently.
Additionally, as the styling is now done in GDB we can add settings to
allow the user to configure which colours are used right from the GDB
CLI.
There's some new maintenance commands:
maintenance set libopcodes-styling enabled on|off
maintenance show libopcodes-styling
These can be used to manually disable use of libopcodes styling. This
is a maintenance command as it's not anticipated that a user should
need to do this. But, this could be useful for testing, or, in some
rare cases, a user might want to override the Python hook used for
disassembler styling, and then disable libopcode styling so that GDB
falls back to using Python. Right now I would consider this second
use case a rare situation, which is why I think a maintenance command
is appropriate.
When libopcodes is being used for styling then the user can make use
of the following new styles:
set/show style disassembler comment
set/show style disassembler immediate
set/show style disassembler mnemonic
set/show style disassembler register
The disassembler also makes use of the 'address' and 'function'
styles to style some parts of the disassembler output. I have also
added the following aliases though:
set/show style disassembler address
set/show style disassembler symbol
these are aliases for:
set/show style address
set/show style function
respectively, and exist to make it easier for users to discover
disassembler related style settings. The 'address' style is used to
style numeric addresses in the disassembler output, while the 'symbol'
or 'function' style is used to style the names of symbols in
disassembler output.
As not every architecture supports libopcodes styling, the maintenance
setting 'libopcodes-styling enabled' has an "auto-off" type behaviour.
Consider this GDB session:
(gdb) show architecture
The target architecture is set to "auto" (currently "i386:x86-64").
(gdb) maintenance show libopcodes-styling enabled
Use of libopcodes styling support is "on".
the setting defaults to "on" for architectures that support libopcodes
based styling.
(gdb) set architecture sparc
The target architecture is set to "sparc".
(gdb) maintenance show libopcodes-styling enabled
Use of libopcodes styling support is "off" (not supported on architecture "sparc")
the setting will show as "off" if the user switches to an architecture
that doesn't support libopcodes styling. The underlying setting is
still "on" at this point though, if the user switches back to
i386:x86-64 then the setting would go back to being "on".
(gdb) maintenance set libopcodes-styling enabled off
(gdb) maintenance show libopcodes-styling enabled
Use of libopcodes styling support is "off".
now the setting is "off" for everyone, even if the user switches back
to i386:x86-64 the setting will still show as "off".
(gdb) maintenance set libopcodes-styling enabled on
Use of libopcodes styling not supported on architecture "sparc".
(gdb) maintenance show libopcodes-styling enabled
Use of libopcodes styling support is "off".
attempting to switch the setting "on" for an unsupported architecture
will give an error, and the setting will remain "off".
(gdb) set architecture auto
The target architecture is set to "auto" (currently "i386:x86-64").
(gdb) maintenance show libopcodes-styling enabled
Use of libopcodes styling support is "off".
(gdb) maintenance set libopcodes-styling enabled on
(gdb) maintenance show libopcodes-styling enabled
Use of libopcodes styling support is "on".
the user will need to switch back to a supported architecture before
they can one again turn this setting "on".
Fix a completion consistency issue with `set' commands accepting integer
values and the special `unlimited' keyword:
(gdb) complete print -elements
print -elements NUMBER
print -elements unlimited
(gdb)
vs:
(gdb) complete set print elements
set print elements unlimited
(gdb)
(there is a space entered at the end of both commands, not shown here)
which also means if you strike <Tab> with `set print elements ' input,
it will, annoyingly, complete to `set print elements unlimited' right
away rather than showing a choice between `NUMBER' and `unlimited'.
Add `NUMBER' then as an available completion for such `set' commands:
(gdb) complete set print elements
set print elements NUMBER
set print elements unlimited
(gdb)
Adjust the testsuite accordingly. Also document the feature in the
Completion section of the manual in addition to the Command Options
section already there.
With errors given for bad commands such as `set annotate' or `set width'
we produce an extraneous full stop within parentheses:
(gdb) set annotate
Argument required (integer to set it to.).
(gdb) set width
Argument required (integer to set it to, or "unlimited".).
(gdb)
This is grammatically incorrect, so remove the full stop and adjust the
testsuite accordingly.
For historical reasons, the CLI and the TUI observers are basically
exact duplicates, except for the downcast:
cli:
struct cli_interp *cli = as_cli_interp (interp);
tui:
struct interp *tui = as_tui_interp (interp);
and how they get at the interpreter's ui_out:
cli:
cli->cli_uiout
tui:
tui->interp_ui_out ()
Since interp_ui_out() is a virtual method that also works for the CLI
interpreter, and, both the CLI and the TUI interpreters inherit from
the same base class (cli_interp_base), we can convert the CLI
observers to cast to cli_interp_base instead and use interp_ui_out()
too. With that, the CLI observers will work for the TUI interpreter
as well. This lets us completely eliminate the TUI observers. That's
what this commit does.
Change-Id: Iaf6cf12dfa200ed3ab203a895a72b69dfedbd6e0
cli_set_logging is declared but not defined. It's probably a leftover
from whenever interpreters were changed to use inheritance. This
patch removes the declaration. Tested by grep and rebuilding.
Currently, GDB internally uses the term "location" for both the
location specification the user input (linespec, explicit location, or
an address location), and for actual resolved locations, like the
breakpoint locations, or the result of decoding a location spec to
SaLs. This is expecially confusing in the breakpoints module, as
struct breakpoint has these two fields:
breakpoint::location;
breakpoint::loc;
"location" is the location spec, and "loc" is the resolved locations.
And then, we have a method called "locations()", which returns the
resolved locations as range...
The location spec type is presently called event_location:
/* Location we used to set the breakpoint. */
event_location_up location;
and it is described like this:
/* The base class for all an event locations used to set a stop event
in the inferior. */
struct event_location
{
and even that is incorrect... Location specs are used for finding
actual locations in the program in scenarios that have nothing to do
with stop events. E.g., "list" works with location specs.
To clean all this confusion up, this patch renames "event_location" to
"location_spec" throughout, and then all the variables that hold a
location spec, they are renamed to include "spec" in their name, like
e.g., "location" -> "locspec". Similarly, functions that work with
location specs, and currently have just "location" in their name are
renamed to include "spec" in their name too.
Change-Id: I5814124798aa2b2003e79496e78f95c74e5eddca
Same idea as previous patch, but for symtab::objfile. I find
it clearer without this wrapper, as it shows that the objfile is
common to all symtabs of a given compunit. Otherwise, you could think
that each symtab (of a given compunit) can have a specific objfile.
Change-Id: Ifc0dbc7ec31a06eefa2787c921196949d5a6fcc6
I think the symtab::dirname method is bogus, or at least very
misleading. It makes you think that it returns the directory that was
used to find that symtab's file during compilation (i.e. the directory
the file refers to in the DWARF line header file table), or the
directory part of the symtab's filename maybe. In fact, it returns the
compilation unit's directory, which is the CWD of the compiler, at
compilation time. At least for DWARF, if the symtab's filename is
relative, it will be relative to that directory. But if the symtab's
filename is absolute, then the directory returned by symtab::dirname has
nothing to do with the symtab's filename.
Remove symtab::dirname to avoid this confusion, change all users to
fetch the same information through the compunit. At least, it will be
clear that this is a compunit property, not a symtab property.
Change-Id: I2894c3bf3789d7359a676db3c58be2c10763f5f0
Various spots in gdb currently know about the wrap buffer, and so are
careful to call wrap_here to be certain that all output has been
flushed.
Now that the pager is just an ordinary stream, this isn't needed, and
a simple call to gdb_flush is enough.
Similarly, there are places where gdb prints to gdb_stderr, but first
flushes gdb_stdout. stderr_file already flushes gdb_stdout, so these
aren't needed.
Now that filtered and unfiltered output can be treated identically, we
can unify the printf family of functions. This is done under the name
"gdb_printf". Most of this patch was written by script.
Now that filtered and unfiltered output can be treated identically, we
can unify the puts family of functions. This is done under the name
"gdb_puts". Most of this patch was written by script.
Now that filtered and unfiltered output can be treated identically, we
can unify the vprintf family of functions: vprintf_filtered,
vprintf_unfiltered, vfprintf_filtered and vfprintf_unfiltered. (For
the gdb_stdout variants, recall that only printf_unfiltered gets truly
unfiltered output at this point.) This removes one such function and
renames the remaining two to "gdb_vprintf". All callers are updated.
Much of this patch was written by script.
This rewrites the output pager as a ui_file implementation.
A new header is introduced to declare the pager class. The
implementation remains in utils.c for the time being, because there
are some static globals there that must be used by this code. (This
could be cleaned up at some future date.)
I went through all the text output in gdb to ensure that this change
should be ok. There are a few cases:
* Any existing call to printf_unfiltered is required to be avoid the
pager. This is ensured directly in the implementation.
* All remaining calls to the f*_unfiltered functions -- the ones that
take an explicit ui_file -- either send to an unfiltered stream
(e.g., gdb_stderr), which is obviously ok; or conditionally send to
gdb_stdout
I investigated all such calls by searching for:
grep -e '\bf[a-z0-9_]*_unfiltered' *.[chyl] */*.[ch] | grep -v gdb_stdlog | grep -v gdb_stderr
This yields a number of candidates to check.
* The breakpoint _print_recreate family, and
save_trace_state_variables. These are used for "save" commands
and so are fine.
* Things printing to a temporary stream. Obviously ok.
* Disassembly selftests.
* print_gdb_help - this is non-obvious, but ok because paging isn't
yet enabled at this point during startup.
* serial.c - doens't use gdb_stdout
* The code in compile/. This is all printing to a file.
* DWARF DIE dumping - doesn't reference gdb_stdout.
* Calls to the _filtered form -- these are all clearly ok, because if
they are using gdb_stdout, then filtering will still apply; and if
not, then filtering never applied and still will not.
Therefore, at this point, there is no longer any distinction between
all the other _filtered and _unfiltered calls, and they can be
unified.
In this patch, take special note of the vfprintf_maybe_filtered and
ui_file::vprintf change. This is one instance of the above idea,
erasing the distinction between filtered and unfiltered -- in this
part of the change, the "unfiltered_output" flag is never passe to
cli_ui_out. Subsequent patches will go much further in this
direction.
Also note the can_emit_style_escape changes in ui-file.c. Checking
against gdb_stdout or gdb_stderr was always a bit of a hack; and now
it is no longer needed, because this is decision can be more fully
delegated to the particular ui_file implementation.
ui_file::can_page is removed, because this patch removed the only call
to it.
I think this is the main part of fixing PR cli/7234.
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=7234
Currently, timestamps for logging are done by looking for the use of
gdb_stdlog in vfprintf_unfiltered. This seems potentially buggy, in
that during logging or other redirects (like execute_fn_to_ui_file) we
might have gdb_stdout==gdb_stdlog and so, conceivably, wind up with
timestamps in a log when they were not desired.
It seems better, instead, for timestamps to be a property of the
ui_file itself.
This patch changes gdb to use the new timestamped_file for gdb_stdlog
where appropriate, and removes the special case from
vfprintf_unfiltered.
Note that this may somewhat change the output in some cases -- in
particular, when going through execute_fn_to_ui_file (or the _string
variant), timestamps won't be emitted. This could be fixed in those
functions, but it wasn't clear to me whether this is really desirable.
Note also that this changes the TUI to send gdb_stdlog to gdb_stderr.
I imagine that the previous use of gdb_stdout here was inadvertent.
(And in any case it probably doesn't matter.)
This changes the CLI logging code to avoid manual memory management
(to the extent possible) by using unique_ptr in a couple of spots.
This will come in handy in a later patch.
The CLI's set_logging logic seemed unnecessarily complicated to me.
This patch simplifies it, with an eye toward changing it to use RAII
objects in a subsequent patch.
I did not touch the corresponding MI code. That code seems incorrect
(nothing ever uses raw_stdlog, and nothing ever sets
saved_raw_stdlog). I didn't attempt to fix this, because I question
whether this is even useful for MI.
This commit adds styling support to the disassembler output, as such
two new commands are added to GDB:
set style disassembler enabled on|off
show style disassembler enabled
In this commit I make use of the Python Pygments package to provide
the styling. I did investigate making use of libsource-highlight,
however, I found the highlighting results to be inferior to those of
Pygments; only some mnemonics were highlighted, and highlighting of
register names such as r9d and r8d (on x86-64) was incorrect.
To enable disassembler highlighting via Pygments, I've added a new
extension language hook, which is then implemented for Python. This
hook is very similar to the existing hook for source code
colorization.
One possibly odd choice I made with the new hook is to pass a
gdb.Architecture through, even though this is currently unused. The
reason this argument is not used is that, currently, styling is
performed identically for all architectures.
However, even though the Python function used to perform styling of
disassembly output is not part of any documented API, I don't want
to close the door on a user overriding this function to provide
architecture specific styling. To do this, the user would inevitably
require access to the gdb.Architecture, and so I decided to add this
field now.
The styling is applied within gdb_disassembler::print_insn, to achieve
this, gdb_disassembler now writes its output into a temporary buffer,
styling is then applied to the contents of this buffer. Finally the
gdb_disassembler buffer is copied out to its final destination stream.
There's a new test to check that the disassembler output includes some
escape sequences, though I don't check for specific colours; the
precise colors will depend on which instructions are in the
disassembler output, and, I guess, how pygments is configured.
The only negative change with this commit is how we currently style
addresses in GDB.
Currently, when the disassembler wants to print an address, we call
back into GDB, and GDB prints the address value using the `address`
styling, and the symbol name using `function` styling. After this
commit, if pygments is used, then all disassembler styling is done
through pygments, and this include the address and symbol name parts
of the disassembler output.
I don't know how much of an issue this will be for people. There's
already some precedent for this in GDB when we look at source styling.
For example, function names in styled source listings are not styled
using the `function` style, but instead, either GNU Source Highlight,
or pygments gets to decide how the function name should be styled.
If the Python pygments library is not present then GDB will continue
to behave as it always has, the disassembler output is mostly
unstyled, but the address and symbols are styled using the `address`
and `function` styles, as they are today.
However, if the user does `set style disassembler enabled off`, then
all disassembler styling is switched off. This obviously covers the
use of pygments, but also includes the minimal styling done by GDB
when pygments is not available.
While working on function calls, I realized that the thread_fsm member
of struct thread_info is a raw pointer to a resource it owns. This
commit changes the type of the thread_fsm member to a std::unique_ptr in
order to signify this ownership relationship and slightly ease resource
management (no need to manually call delete).
To ensure consistent use, the field is made a private member
(m_thread_fsm). The setter method (set_thread_fsm) can then check
that it is incorrect to associate a FSM to a thread_info object if
another one is already in place. This is ensured by an assertion.
The function run_inferior_call takes an argument as a pointer to a
call_thread_fsm and installs it in it in a thread_info instance. Also
change this function's signature to accept a unique_ptr in order to
signify that the ownership of the call_thread_fsm is transferred during
the call.
No user visible change expected after this commit.
Tested on x86_64-linux with no regression observed.
Change-Id: Ia1224f72a4afa247801ce6650ce82f90224a9ae8
GDB already has a flag to suppress printing notification events, such
as thread and inferior context switches, on the CLI. This is used
internally when executing commands. Make the flag available to the
user via a new command. This is expected to be useful in scripts.
For instance, suppose that when Inferior 1 gets to a certain state,
you want to add and set up a new inferior using the commands below,
but you also want to have a reduced/clean output.
define do-setup
printf "Setting up Inferior 2...\n"
add-inferior -exec a.out
inferior 2
break file.c:3
run
inferior 1
printf "Done\n"
end
Currently, GDB prints
(gdb) do-setup
Setting up Inferior 2...
[New inferior 2]
Added inferior 2 on connection 1 (native)
[Switching to inferior 2 [<null>] (/tmp/a.out)]
Breakpoint 2 at 0x1155: file file.c, line 3.
Thread 2.1 "a.out" hit Breakpoint 2, main () at file.c:3
3 return 0;
[Switching to inferior 1 [process 7670] (/tmp/test)]
[Switching to thread 1.1 (process 7670)]
#0 main () at test.c:2
2 int a = 1;
Done
GDB's Python API make it possible to capture and return GDB's output,
but this does not work for all the streams. In particular, CLI
notification events are not captured:
(gdb) python gdb.execute("do-setup", False, True)
[Switching to inferior 2 [<null>] (/tmp/a.out)]
Thread 2.1 "a.out" hit Breakpoint 2, main () at file.c:3
3 return 0;
[Switching to inferior 1 [process 8263] (/tmp/test)]
[Switching to thread 1.1 (process 8263)]
#0 main () at test.c:2
2 int a = 1;
You can use the new "set suppress-cli-notifications" command to
suppress the output:
(gdb) set suppress-cli-notifications on
(gdb) do-setup
Setting up Inferior 2...
[New inferior 2]
Added inferior 2 on connection 1 (native)
Breakpoint 2 at 0x1155: file file.c, line 3.
Done
Extend the 'cli_suppress_notification' struct with a new field,
'normal_stop', that can be used for checking if printing normal stop
events on the CLI should be suppressed.
This patch only introduces the flag. The subsequent patch adds a user
command to turn the flag off/on.
While working on the previous commit to fix PR cli/28665, I noticed
that the 'edit' command would suffer from the same problem. That is,
something like:
(gdb) edit task 123
would cause GDB to break. For a full explanation of what's going on
here, see the commit message for the previous commit.
As with the previous commit, this issue can be prevented by detecting,
and throwing, a junk at the end of the line error earlier, before
calling decode_line_1.
So, that's what this commit does. I've also added some tests for this
issue.
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=28665
In PR cli/28665, it was reported that GDB would crash when given a
command like:
(gdb) list task 123
The problem here is that in cli/cli-cmd.c:list_command, the string
'task 123' is passed to string_to_event_location in find a location
specification. However, this location parsing understands about
breakpoint conditions, and so, will stop parsing when it sees
something that looks like a condition, in this case, the 'task 123'
looks like a breakpoint condition.
As a result, the location we get back from string_to_event_location
has no actual location specification attached to it. The actual call
path is:
list_command
string_to_event_location
string_to_event_location_basic
new_linespec_location
In new_linespec_location we call linespec_lex_to_end, which looks at
'task 123' and decides that there's nothing there that describes a
location. As such, in new_linespec_location, the spec_string field of
the location is left as nullptr.
Back in list_command we then call decode_line_1, which calls
event_location_to_sals, which calls parse_linespec, which takes the
spec_string we found earlier, and tries to converts this into a list
of sals.
However, parse_linespec is not intended to be passed a nullptr, for
example, calling is_ada_operator will try to access through the
nullptr, causing undefined behaviour. But there are other cases
within parse_linespec which don't expect to see a nullptr.
When looking at how to fix this issue, I first considered having
linespec_lex_to_end detect the problem. That function understands
when the first thing in the linespec is a condition keyword, and so,
could throw an error saying something like: "no linespec before
condition keyword", however, this is not going to work, at least, not
without additional changes to GDB, it is valid to place a breakpoint
like:
(gdb) break task 123
This will place a breakpoint at the current location with the
condition 'task 123', and changing linespec_lex_to_end breaks this
behaviour.
So, next, I considered what would happen if I added a condition to an
otherwise valid list command, this is what I see:
(gdb) list file.c:1 task 123
Junk at end of line specification.
(gdb)
So, then I wondered, could we just pull the "Junk" detection forward,
so that we throw the error earlier, before we call decode_line_1?
It turns out that yes we can. Well, sort of.
It is simpler, I think, to add a separate check into the list_command
function, after calling string_to_event_location, but before calling
decode_line_1. We know when we call string_to_event_location that the
string in question is not empty, so, after calling
string_to_event_location, if non of the string has been consumed, then
the content of the string must be junk - it clearly doesn't look like
a location specification.
I've reused the same "Junk at end of line specification." error for
consistency, and added a few tests to cover this issue.
While the first version of this patch was on the mailing list, a
second bug PR gdb/28797 was raised. This was for a very similar
issue, but this time the problem command was:
(gdb) list ,,
Here the list command understands about the first comma, list can have
two arguments separated by a comma, and the first argument can be
missing. So we end up trying to parse the second command "," as a
linespec.
However, in linespec_lex_to_end, we will stop parsing a linespec at a
comma, so, in the above case we end up with an empty linespec (between
the two commas), and, like above, this results in the spec_string
being nullptr.
As with the previous case, I've resolved this issue by adding an extra
check for junk at the end of the line - after parsing (or failing to
parse) the nothing between the two commas, we still have the "," left
at the end of the list command line - when we see this we can throw
the same "junk at the end of the line" error, and all is good.
I've added tests for this case too.
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=28665
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=28797
