Fix for PR gdb/20684. When invoking MI commands with --thread and/or
--frame, the user selected thread and frame was not preserved:
(gdb)
info thread
&"info thread\n"
~" Id Target Id Frame \n"
~"* 1 Thread 0x7ffff7c30740 (LWP 19302) \"user-selected-c\" main () at /home/uuu/gdb/gdb/testsuite/gdb.mi/user-selected-context-sync.c:60\n"
~" 2 Thread 0x7ffff7c2f700 (LWP 19306) \"user-selected-c\" child_sub_function () at /home/uuu/gdb/gdb/testsuite/gdb.mi/user-selected-context-sync.c:30\n"
~" 3 Thread 0x7ffff742e700 (LWP 19307) \"user-selected-c\" child_sub_function () at /home/uuu/gdb/gdb/testsuite/gdb.mi/user-selected-context-sync.c:30\n"
^done
(gdb)
info frame
&"info frame\n"
~"Stack level 0, frame at 0x7fffffffdf90:\n"
~" rip = 0x555555555207 in main (/home/uuu/gdb/gdb/testsuite/gdb.mi/user-selected-context-sync.c:60); saved rip = 0x7ffff7c5709b\n"
~" source language c.\n"
~" Arglist at 0x7fffffffdf80, args: \n"
~" Locals at 0x7fffffffdf80, Previous frame's sp is 0x7fffffffdf90\n"
~" Saved registers:\n "
~" rbp at 0x7fffffffdf80, rip at 0x7fffffffdf88\n"
^done
(gdb)
-stack-info-depth --thread 3
^done,depth="4"
(gdb)
info thread
&"info thread\n"
~" Id Target Id Frame \n"
~" 1 Thread 0x7ffff7c30740 (LWP 19302) \"user-selected-c\" main () at /home/uuu/gdb/gdb/testsuite/gdb.mi/user-selected-context-sync.c:60\n"
~" 2 Thread 0x7ffff7c2f700 (LWP 19306) \"user-selected-c\" child_sub_function () at /home/uuu/gdb/gdb/testsuite/gdb.mi/user-selected-context-sync.c:30\n"
~"* 3 Thread 0x7ffff742e700 (LWP 19307) \"user-selected-c\" child_sub_function () at /home/uuu/gdb/gdb/testsuite/gdb.mi/user-selected-context-sync.c:30\n"
^done
(gdb)
info frame
&"info frame\n"
~"Stack level 0, frame at 0x7ffff742dee0:\n"
~" rip = 0x555555555169 in child_sub_function (/home/uuu/gdb/gdb/testsuite/gdb.mi/user-selected-context-sync.c:30); saved rip = 0x555555555188\n"
~" called by frame at 0x7ffff742df00\n"
~" source language c.\n"
~" Arglist at 0x7ffff742ded0, args: \n"
~" Locals at 0x7ffff742ded0, Previous frame's sp is 0x7ffff742dee0\n"
~" Saved registers:\n "
~" rbp at 0x7ffff742ded0, rip at 0x7ffff742ded8\n"
^done
(gdb)
This caused problems for frontends that provide access to CLI because UI
may silently change the context for CLI commands (as demonstrated above).
This commit fixes the problem by restoring thread and frame in
mi_cmd_execute (). With this change, there are only two GDB/MI commands
that can change user selected context: -thread-select and -stack-select-frame.
This allows us to remove all and rather complicated logic of notifying
about user selected context change from mi_execute_command (), leaving it
to these two commands themselves to notify.
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=20684
Following on from the previous commit, where the -add-inferior command
now uses the same connection as the current inferior, this commit adds
a --no-connection option to -add-inferior.
This new option matches the existing option of the same name for the
CLI version of add-inferior; the new inferior is created with no
connection.
I've added a new 'connection' field to the MI output of -add-inferior,
which includes the connection number and short name. I haven't
included the longer description field, this is the MI after all. My
expectation would be that if the frontend wanted to display all the
connection details then this would be looked up from 'info
connection' (or the MI equivalent if/when such a command is added).
The existing -add-inferior tests are updated, as are the docs.
Prior to the multi-target support commit:
commit 5b6d1e4fa4
Date: Fri Jan 10 20:06:08 2020 +0000
Multi-target support
When a new inferior was added using the MI -add-inferior command, the
new inferior would be using the same target as all the other
inferiors. This makes sense, GDB only supported a single target stack
at a time.
After the above commit, each inferior has its own target stack.
To maintain backward compatibility, for the CLI add-inferior command,
when a new inferior is added the above commit has the new inferior
inherit a copy of the target stack from the current inferior.
Unfortunately, this same backward compatibility is missing for the MI.
This commit fixes this oversight.
Now, when the -add-inferior MI command is used, the new inferior will
inherit a copy of the target stack from the current inferior.
Joel noticed that if the remote dies unexpectedly during a command --
you can simulate this by using "continue" and then killing gdbserver
-- then the CLI will print a new prompt, but MI will not. Later, we
found out that this was also filed in bugzilla as PR mi/23820.
The output looks something like this:
| (gdb)
| cont
| &"cont\n"
| ~"Continuing.\n"
| ^running
| *running,thread-id="all"
| (gdb)
| [... some output from GDB during program startup...]
| =thread-exited,id="1",group-id="i1"
| =thread-group-exited,id="i1"
| &"Remote connection closed\n"
Now, what about that "(gdb)" in the middle?
That prompt comes from this questionable code in
mi-interp.c:mi_on_resume_1:
/* This is what gdb used to do historically -- printing prompt
even if it cannot actually accept any input. This will be
surely removed for MI3, and may be removed even earlier. */
if (current_ui->prompt_state == PROMPT_BLOCKED)
fputs_unfiltered ("(gdb) \n", mi->raw_stdout);
... which seems like something to remove. But maybe the intent here
is that this prompt is sufficient, and MI clients must be ready to
handle output coming after a prompt. On the other hand, if this code
*is* removed, then nothing would print a prompt in this scenario.
Anyway, the CLI and the TUI handle emitting the prompt here by hooking
into gdb::observers::command_error, but MI doesn't install an observer
here.
This patch adds the missing observer and arranges to show the MI
prompt. Regression tested on x86-64 Fedora 34.
It seems like this area could be improved a bit, by having
start_event_loop call the prompt-displaying code directly, rather than
indirecting through an observer. However, I haven't done this.
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=23820
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
Add a getter and a setter for a symbol's line. Remove the corresponding macro
and adjust all callers.
Change-Id: I229f2b8fcf938c07975f641361313a8761fad9a5
Add a getter and a setter for a symbol's type. Remove the corresponding
macro and adjust all callers.
Change-Id: Ie1a137744c5bfe1df4d4f9ae5541c5299577c8de
Add a getter and a setter for whether a symbol is an argument. Remove
the corresponding macro and adjust all callers.
Change-Id: I71b4f0465f3dfd2ed8b9e140bd3f7d5eb8d9ee81
Add a getter and a setter for a symtab's linetable. Remove the
corresponding macro and adjust all callers.
Change-Id: I159183fc0ccd8e18ab937b3c2f09ef2244ec6e9c
Add a getter and a setter for a symtab's compunit_symtab. Remove the
corresponding macro and adjust all callers.
For brevity, I chose the name "compunit" instead of "compunit_symtab"
the the field, getter and setter names. Since we are already in symtab
context, the _symtab suffix seems redundant.
Change-Id: I4b9b731c96e3594f7733e75af1e3d01bc0e4fe92
Add a getter and a setter for a compunit_symtab's macro table. Remove the
corresponding macro and adjust all callers.
Change-Id: I00615ea72d5ac43d9a865e941cb2de0a979c173a
I noticed that host_hex_value is redundant, because gdbsupport already
has fromhex. This patch removes the former in favor of the latter.
Regression tested on x86-64 Fedora 34.
Right now, wrap_here is a global function. In the long run, we'd like
output streams to be relatively self-contained objects, and having a
global function like this is counter to that goal. Also, existing
code freely mixes writes to some parameterized stream with calls to
wrap_here -- but wrap_here only really affects gdb_stdout, so this is
also incoherent.
This step is a patch toward making wrap_here more sane. It adds a
wrap_here method to ui_file and changes ui_out implementations to use
it.
I think it only really makes sense to call wrap_here with an argument
consisting solely of spaces. Given this, it seemed better to me that
the argument be an int, rather than a string. This patch is the
result. Much of it was written by a script.
In my tour of the ui_file subsystem, I found that fputstr and fputstrn
can be simplified. The _filtered forms are never used (and IMO
unlikely to ever be used) and so can be removed. And, the interface
can be simplified by removing a callback function and moving the
implementation directly to ui_file.
A new self-test is included. Previously, I think nothing was testing
this code.
Regression tested on x86-64 Fedora 34.
This commit brings all the changes made by running gdb/copyright.py
as per GDB's Start of New Year Procedure.
For the avoidance of doubt, all changes in this commits were
performed by the script.
Just give this class a new name, more inline with the name of the
sub-classes. I've also updated mi_cmd_up to mi_command_up in
mi-cmds.c inline with this new naming scheme.
There should be no user visible changes after this commit.
This commit changes the infrastructure in mi-cmds.{c,h} to add new
sub-classes for the different types of MI command. Instances of these
sub-classes are then created and added into mi_cmd_table.
The existing mi_cmd class becomes the abstract base class, this has an
invoke method and takes care of the suppress notifications handling,
before calling a do_invoke virtual method which is implemented by all
of the sub-classes.
There's currently two different sub-classes, one of pure MI commands,
and a second for MI commands that delegate to CLI commands.
There should be no user visible changes after this commit.
Change an argument of mi_execute_cli_command from int to bool. Update
the callers to take this into account. Within mi_execute_cli_command,
update a comparison of a pointer to 0 with a comparison to nullptr,
and add an assert, if we are not using the argument string then the
string should be nullptr. Also removed a cryptic 'gdb_????' comment,
which isn't really helpful.
There should be no user visible changes after this commit.
This changes the hashmap used in mi-cmds.c from a custom structure to
std::map. Not only is replacing a custom container with a standard
one an improvement, but using std::map will make it easier to
dynamically add commands; which is something that is planned for a
later series, where we will allow MI commands to be implemented in
Python.
There should be no user visible changes after this commit.
Lets give this function a more descriptive name. I've also improved
the comments in the header and source files.
There should be no user visible changes after this commit.
There are a few places where we call the target_ops::can_async_p
member function directly, instead of using the target_can_async_p
wrapper.
In some of these places this is because we need to ask before the
target has been pushed, and in another location (in target.c) it seems
unnecessary to go through the wrapper when we are already in target.c
code.
However, in the next commit I'd like to hoist some common checks out
of target specific code into target.c. To achieve this, in this
commit, I introduce a new overload of target_can_async_p which takes a
target_ops pointer, and calls the ::can_async_p method directly. I
then make use of the new overload where appropriate.
There should be no user visible changes after this commit.
The motivation is to reduce the number of places where unmanaged
pointers are returned from allocation type routines. All of the
callers are updated.
There should be no user visible changes after this commit.
When doing "continue -a" in non-stop mode, each thread is individually
resumed while the commit resumed state is enabled. This forces the
target to commit each resumption immediately, instead of being able to
batch things.
The reason is that there is no scoped_disable_commit_resumed around the
loop over threads in continue_1, when "non_stop && all_threads" is true.
Since the proceed function is called once for each thread, the
scoped_disable_commit_resumed in proceed therefore forces commit-resumed
between each thread resumption. Add the necessary
scoped_disable_commit_resumed in continue_1 to avoid that.
I looked at the MI side of things, the function exec_continue, and found
that it was correct. There is a similar iteration over threads, and
there is a scoped_disable_commit_resumed at the function scope. This is
not wrong, but a bit more than we need. The branches that just call
continue_1 do not need it, as continue_1 takes care of disabling commit
resumed. So, move the scoped_disable_commit_resumed to the inner scope
where we iterate on threads and proceed them individually.
Here's an example debugging a multi-threaded program attached by
gdbserver (debug output trimmed for brevity):
$ ./gdb -nx -q --data-directory=data-directory -ex "set non-stop" -ex "tar rem :1234"
(gdb) set debug remote
(gdb) set debug infrun
(gdb) c -a
Continuing.
[infrun] proceed: enter
[infrun] scoped_disable_commit_resumed: reason=proceeding
[remote] Sending packet: $vCont;c:p14388.14388#90
[infrun] reset: reason=proceeding
[infrun] maybe_set_commit_resumed_all_targets: enabling commit-resumed for target remote
[infrun] maybe_call_commit_resumed_all_targets: calling commit_resumed for target remote
[infrun] proceed: exit
[infrun] proceed: enter
[infrun] scoped_disable_commit_resumed: reason=proceeding
[remote] Sending packet: $vCont;c:p14388.1438a#b9
[infrun] reset: reason=proceeding
[infrun] maybe_set_commit_resumed_all_targets: enabling commit-resumed for target remote
[infrun] maybe_call_commit_resumed_all_targets: calling commit_resumed for target remote
[infrun] proceed: exit
... and so on for each thread ...
Notice how we send one vCont;c for each thread. With the patch applied, we
send a single vCont;c at the end:
[infrun] scoped_disable_commit_resumed: reason=continue all threads in non-stop
[infrun] proceed: enter
[infrun] scoped_disable_commit_resumed: reason=proceeding
[infrun] reset: reason=proceeding
[infrun] proceed: exit
[infrun] clear_proceed_status_thread: Thread 85790.85792
[infrun] proceed: enter
[infrun] scoped_disable_commit_resumed: reason=proceeding
[infrun] reset: reason=proceeding
[infrun] proceed: exit
... proceeding threads individually ...
[infrun] reset: reason=continue all threads in non-stop
[infrun] maybe_set_commit_resumed_all_targets: enabling commit-resumed for target remote
[infrun] maybe_call_commit_resumed_all_targets: calling commit_resumed for target remote
[remote] Sending packet: $vCont;c#a8
Change-Id: I331dd2473c5aa5114f89854196fed2a8fdd122bb
I don't find that the bpstat typedef, which hides a pointer, is
particularly useful. In fact, it confused me many times, and I just see
it as something to remember that adds cognitive load. Also, with C++,
we might want to be able to pass bpstats objects by const-reference, not
necessarily by pointer.
So, remove the bpstat typedef and rename struct bpstats to bpstat (since
it represents one bpstat, it makes sense that it is singular).
Change-Id: I52e763b6e54ee666a9e045785f686d37b4f5f849
String-like settings (var_string, var_filename, var_optional_filename,
var_string_noescape) currently take a pointer to a `char *` storage
variable (typically global) that holds the setting's value. I'd like to
"mordernize" this by changing them to use an std::string for storage.
An obvious reason is that string operations on std::string are often
easier to write than with C strings. And they avoid having to do any
manual memory management.
Another interesting reason is that, with `char *`, nullptr and an empty
string often both have the same meaning of "no value". String settings
are initially nullptr (unless initialized otherwise). But when doing
"set foo" (where `foo` is a string setting), the setting now points to
an empty string. For example, solib_search_path is nullptr at startup,
but points to an empty string after doing "set solib-search-path". This
leads to some code that needs to check for both to check for "no value".
Or some code that converts back and forth between NULL and "" when
getting or setting the value. I find this very error-prone, because it
is very easy to forget one or the other. With std::string, we at least
know that the variable is not "NULL". There is only one way of
representing an empty string setting, that is with an empty string.
I was wondering whether the distinction between NULL and "" would be
important for some setting, but it doesn't seem so. If that ever
happens, it would be more C++-y and self-descriptive to use
optional<string> anyway.
Actually, there's one spot where this distinction mattered, it's in
init_history, for the test gdb.base/gdbinit-history.exp. init_history
sets the history filename to the default ".gdb_history" if it sees that
the setting was never set - if history_filename is nullptr. If
history_filename is an empty string, it means the setting was explicitly
cleared, so it leaves it as-is. With the change to std::string, this
distinction doesn't exist anymore. This can be fixed by moving the code
that chooses a good default value for history_filename to
_initialize_top. This is ran before -ex commands are processed, so an
-ex command can then clear that value if needed (what
gdb.base/gdbinit-history.exp tests).
Another small improvement, in my opinion is that we can now easily
give string parameters initial values, by simply initializing the global
variables, instead of xstrdup-ing it in the _initialize function.
In Python and Guile, when registering a string-like parameter, we
allocate (with new) an std::string that is owned by the param_smob (in
Guile) and the parmpy_object (in Python) objects.
This patch started by changing all relevant add_setshow_* commands to
take an `std::string *` instead of a `char **` and fixing everything
that failed to build. That includes of course all string setting
variable and their uses.
string_option_def now uses an std::string also, because there's a
connection between options and settings (see
add_setshow_cmds_for_options).
The add_path function in source.c is really complex and twisted, I'd
rather not try to change it to work on an std::string right now.
Instead, I added an overload that copies the std:string to a `char *`
and back. This means more copying, but this is not used in a hot path
at all, so I think it is acceptable.
Change-Id: I92c50a1bdd8307141cdbacb388248e4e4fc08c93
Co-authored-by: Lancelot SIX <lsix@lancelotsix.com>
As reported in PR gdb/28076 [1], passing no condition argument to the
-break-condition command (e.g.: "-break-condition 2") should clear the
condition for breakpoint 2, just like CLI's "condition 2", but instead
an error message is returned:
^error,msg="-break-condition: Missing the <number> and/or <expr> argument"
The current implementation of the -break-condition command's argument
handling (79aabb7308 "gdb/mi: add a '--force' flag to the
'-break-condition' command") was done according to the documentation,
where the condition argument seemed mandatory. However, the
-break-condition command originally (i.e. before the 79aabb7308
patch) used the CLI's "cond" command, and back then not passing a
condition argument was clearing out the condition. So, this is a
regression in terms of the behavior.
Fix the argument handling of the -break-condition command to allow not
having a condition argument, and also update the document to make the
behavior clear. Also add test cases to test the scenarios which were
previously not covered.
[1] https://sourceware.org/bugzilla/show_bug.cgi?id=28076
As reported in PR gdb/28077, we hit an internal error when using
-exec-interrupt with --thread-group:
info threads
&"info threads\n"
~" Id Target Id Frame \n"
~"* 1 process 403312 \"loop\" (running)\n"
^done
(gdb)
-exec-interrupt --thread-group i1
~"/home/simark/src/binutils-gdb/gdb/target.c:3768: internal-error: void target_stop(ptid_t): Assertion `!proc_target->commit_resumed_state' failed.\nA problem internal to GDB has been detected,\nfurther debugging may prove unreliable.\nQuit this debugging session? (y or n) "
This is because this code path never disables commit-resumed (a
requirement for calling target_stop, as documented in
process_stratum_target::»commit_resumed_state) before calling
target_stop.
The other 3 code paths in mi_cmd_exec_interrupt use interrupt_target_1,
which does it. But the --thread-group code path uses its own thing
which doesn't do it. Fix this by adding a scoped_disable_commit_resumed
in this code path.
Calling -exec-interrupt with --thread-group is apparently not tested at
the moment (which is why this bug could creep in). Add a new test for
that. The test runs two inferiors and tries to interrupt them with
"-exec-interrupt --thread-group X".
This will need to be merged in the gdb-11-branch, so here are ChangeLog
entries:
gdb/ChangeLog:
* mi/mi-main.c (mi_cmd_exec_interrupt): Use
scoped_disable_commit_resumed in the --thread-group case.
gdb/testsuite/ChangeLog:
* gdb.mi/interrupt-thread-group.c: New.
* gdb.mi/interrupt-thread-group.exp: New.
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=28077
Change-Id: I615efefcbcaf2c15d47caf5e4b9d82854b2a2fcb
This commit adds a new option '--group-by-objfile' to the MI command
-file-list-exec-source-files. With this option the output format is
changed; instead of a single list of source files the results are now
a list of objfiles. For each objfile all of the source files
associated with that objfile are listed.
Here is an example of the new output format taken from the
documentation (the newlines are added just for readability):
-file-list-exec-source-files --group-by-objfile
^done,files=[{filename="/tmp/info-sources/test.x",
debug-info="fully-read",
sources=[{file="test.c",
fullname="/tmp/info-sources/test.c",
debug-fully-read="true"},
{file="/usr/include/stdc-predef.h",
fullname="/usr/include/stdc-predef.h",
debug-fully-read="true"},
{file="header.h",
fullname="/tmp/info-sources/header.h",
debug-fully-read="true"}]},
{filename="/lib64/ld-linux-x86-64.so.2",
debug-info="none",
sources=[]},
{filename="system-supplied DSO at 0x7ffff7fcf000",
debug-info="none",
sources=[]},
{filename="/tmp/info-sources/libhelper.so",
debug-info="fully-read",
sources=[{file="helper.c",
fullname="/tmp/info-sources/helper.c",
debug-fully-read="true"},
{file="/usr/include/stdc-predef.h",
fullname="/usr/include/stdc-predef.h",
debug-fully-read="true"},
{file="header.h",
fullname="/tmp/info-sources/header.h",
debug-fully-read="true"}]},
{filename="/lib64/libc.so.6",
debug-info="none",
sources=[]}]
In the above output the 'debug-info' field associated with each
objfile will have one of the values 'none', 'partially-read', or
'fully-read'. For example, /lib64/libc.so.6 has the value 'none',
this indicates that this object file has no debug information
associated with it, unsurprisingly then, the sources list of this
object file is empty.
An object file that was compiled with debug, for example
/tmp/info-sources/libhelper.so, has the value 'fully-read' above
indicating that this object file does have debug information, and the
information is fully read into GDB. At different times this field
might have the value 'partially-read' indicating that that the object
file has debug information, but it has not been fully read into GDB
yet.
Source files can appear at most once for any single objfile, but can
appear multiple times in total, if the same source file is part of
multiple objfiles, for example /tmp/info-sources/header.h in the above
output.
The new output format is hidden behind a command option to ensure that
the default output is unchanged, this ensures backward compatibility.
The behaviour of the CLI "info sources" command is unchanged after
this commit.
gdb/ChangeLog:
* NEWS: Mention additions to -file-list-exec-source-files.
* mi/mi-cmd-file.c (mi_cmd_file_list_exec_source_files): Add
--group-by-objfile option.
* symtab.c (isrc_flag_option_def): Rename to...
(isrc_match_flag_option_def): ...this.
(info_sources_option_defs): Rename to...
(info_sources_match_option_defs): ...this, and update to rename of
isrc_flag_option_def.
(struct filename_grouping_opts): New struct.
(isrc_grouping_flag_option_def): New type.
(info_sources_grouping_option_defs): New static global.
(make_info_sources_options_def_group): Update to return two option
groups.
(info_sources_command_completer): Update for changes to
make_info_sources_options_def_group.
(info_sources_worker): Add extra parameter, use this to display
alternative output format.
(info_sources_command): Pass extra parameter to
info_sources_worker.
(_initialize_symtab): Update for changes to
make_info_sources_options_def_group.
* symtab.h (info_sources_worker): Add extra parameter.
gdb/doc/ChangeLog:
* gdb.texinfo (GDB/MI File Commands): Document --group-by-objfile
extension for -file-list-exec-source-files.
gdb/testsuite/ChangeLog:
* gdb.mi/mi-info-sources.exp: Add additional tests.
This commit extends the existing MI command
-file-list-exec-source-files to provide the same regular expression
based filtering that the equivalent CLI command "info sources"
provides.
The new command syntax is:
-file-list-exec-source-files [--basename | --dirname] [--] [REGEXP]
All options are optional, which ensures the command is backward
compatible.
As part of this work I have unified the CLI and MI code.
As a result of the unified code I now provide additional information
in the MI command output, there is now a new field 'debug-fully-read'
included with each source file. This field which has the values
'true' or 'false', indicates if the source file is from a compilation
unit that has had its debug information fully read. However, as this
is additional information, a well written front-end should just ignore
this field if it doesn't understand it, so things should still be
backward compatible.
gdb/ChangeLog:
* NEWS: Mention additions to -file-list-exec-source-files.
* mi/mi-cmd-file.c (print_partial_file_name): Delete.
(mi_cmd_file_list_exec_source_files): Rewrite to handle command
options, and make use of info_sources_worker.
* symtab.c (struct info_sources_filter): Moved to symtab.h.
(info_sources_filter::print): Take uiout argument, produce output
through uiout.
(struct output_source_filename_data)
<output_source_filename_data>: Take uiout argument, store into
m_uiout. <output>: Rewrite comment, add additional arguments to
declaration. <operator()>: Send more arguments to
output. <m_uiout>: New member variable.
(output_source_filename_data::output): Take extra arguments,
produce output through m_uiout, and structure for MI.
(output_source_filename_data::print_header): Produce output
through m_uiout.
(info_sources_worker): New function, the implementation is taken
from info_sources_command, but modified so produce output through
a ui_out.
(info_sources_command): The second half of this function has gone
to become info_sources_worker.
* symtab.h (struct info_sources_filter): Moved from symtab.c, add
extra parameter to print member function.
(info_sources_worker): Declare.
gdb/doc/ChangeLog:
* gdb.texinfo (GDB/MI File Commands): Document extensions to
-file-list-exec-source-files.
gdb/testsuite/ChangeLog:
* gdb.dwarf2/dw2-filename.exp: Update expected results.
* gdb.mi/mi-file.exp: Likewise.
* gdb.mi/mi-info-sources-base.c: New file.
* gdb.mi/mi-info-sources.c: New file.
* gdb.mi/mi-info-sources.exp: New file.
I spotted some indentation issues where we had some spaces followed by
tabs at beginning of line, that I wanted to fix. So while at it, I did
a quick grep to find and fix all I could find.
gdb/ChangeLog:
* Fix tab after space indentation issues throughout.
Change-Id: I1acb414dd9c593b474ae2b8667496584df4316fd
I wrote a small script to spot a pattern of indentation mistakes I saw
happened in breakpoint.c. And while at it I ran it on all files and
fixed what I found. No behavior changes intended, just indentation and
addition / removal of curly braces.
gdb/ChangeLog:
* Fix some indentation mistakes throughout.
gdbserver/ChangeLog:
* Fix some indentation mistakes throughout.
Change-Id: Ia01990c26c38e83a243d8f33da1d494f16315c6e
Remove a few instances where we look up a command by name, but could
just use the return value of a previous "add command" function call
instead.
gdb/ChangeLog:
* mi/mi-main.c (_initialize_mi_main):
* python/py-auto-load.c (gdbpy_initialize_auto_load):
* remote.c (_initialize_remote):
Change-Id: I6d06f9ca636e340c88c1064ae870483ad392607d
Add a '--force' flag to the '-break-condition' command to be
able to force conditions.
gdb/ChangeLog:
2021-05-06 Tankut Baris Aktemur <tankut.baris.aktemur@intel.com>
* mi/mi-cmd-break.c (mi_cmd_break_condition): New function.
* mi/mi-cmds.c: Change the binding of "-break-condition" to
mi_cmd_break_condition.
* mi/mi-cmds.h (mi_cmd_break_condition): Declare.
* breakpoint.h (set_breakpoint_condition): Declare a new
overload.
* breakpoint.c (set_breakpoint_condition): New overloaded function
extracted out from ...
(condition_command): ... this.
* NEWS: Mention the change.
gdb/testsuite/ChangeLog:
2021-05-06 Tankut Baris Aktemur <tankut.baris.aktemur@intel.com>
* gdb.mi/mi-break.exp (test_forced_conditions): Add a test
for the -break-condition command's "--force" flag.
gdb/doc/ChangeLog:
2021-05-06 Tankut Baris Aktemur <tankut.baris.aktemur@intel.com>
* gdb.texinfo (GDB/MI Breakpoint Commands): Mention the
'--force' flag of the '-break-condition' command.
Add a '--force-condition' flag to the '-break-insert' command to be
able to force conditions. Because the '-dprintf-insert' command uses
the same mechanism as the '-break-insert' command, it obtains the
'--force-condition' flag, too.
gdb/ChangeLog:
2021-05-06 Tankut Baris Aktemur <tankut.baris.aktemur@intel.com>
* mi/mi-cmd-break.c (mi_cmd_break_insert_1): Recognize the
'--force-condition' flag to force the condition in the
'-break-insert' and '-dprintf-insert' commands.
* NEWS: Mention the change.
gdb/testsuite/ChangeLog:
2021-05-06 Tankut Baris Aktemur <tankut.baris.aktemur@intel.com>
* gdb.mi/mi-break.exp (test_forced_conditions): New proc that
is called by the test.
gdb/doc/ChangeLog:
2021-05-06 Tankut Baris Aktemur <tankut.baris.aktemur@intel.com>
* gdb.texinfo (GDB/MI Breakpoint Commands): Mention the
'--force-condition' flag of the '-break-insert' and
'-dprintf-insert' commands.
Give a name to each observer, this will help produce more meaningful
debug message.
gdbsupport/ChangeLog:
* observable.h (class observable) <struct observer> <observer>:
Add name parameter.
<name>: New field.
<attach>: Add name parameter, update all callers.
Change-Id: Ie0cc4664925215b8d2b09e026011b7803549fba0
The 'create_breakpoint' function takes a 'parse_extra' argument that
determines whether the condition, thread, and force-condition
specifiers should be parsed from the extra string or be used from the
function arguments. However, for the case when 'parse_extra' is
false, there is no way to pass the force-condition specifier. This
patch adds it as a new argument.
Also, in the case when parse_extra is false, the current behavior is
as if the condition is being forced. This is a bug. The default
behavior should reject the breakpoint. See below for a demo of this
incorrect behavior. (The MI command '-break-insert' uses the
'create_breakpoint' function with parse_extra=0.)
$ gdb -q --interpreter=mi3 /tmp/simple
=thread-group-added,id="i1"
=cmd-param-changed,param="history save",value="on"
=cmd-param-changed,param="auto-load safe-path",value="/"
~"Reading symbols from /tmp/simple...\n"
(gdb)
-break-insert -c junk -f main
&"warning: failed to validate condition at location 1, disabling:\n "
&"No symbol \"junk\" in current context.\n"
^done,bkpt={number="1",type="breakpoint",disp="keep",enabled="y",addr="<MULTIPLE>",cond="junk",times="0",original-location="main",locations=[{number="1.1",enabled="N",addr="0x000000000000114e",func="main",file="/tmp/simple.c",fullname="/tmp/simple.c",line="2",thread-groups=["i1"]}]}
(gdb)
break main if junk
&"break main if junk\n"
&"No symbol \"junk\" in current context.\n"
^error,msg="No symbol \"junk\" in current context."
(gdb)
break main -force-condition if junk
&"break main -force-condition if junk\n"
~"Note: breakpoint 1 also set at pc 0x114e.\n"
&"warning: failed to validate condition at location 1, disabling:\n "
&"No symbol \"junk\" in current context.\n"
~"Breakpoint 2 at 0x114e: file /tmp/simple.c, line 2.\n"
=breakpoint-created,bkpt={number="2",type="breakpoint",disp="keep",enabled="y",addr="<MULTIPLE>",cond="junk",times="0",original-location="main",locations=[{number="2.1",enabled="N",addr="0x000000000000114e",func="main",file="/tmp/simple.c",fullname="/tmp/simple.c",line="2",thread-groups=["i1"]}]}
^done
(gdb)
After applying this patch, we get the behavior below:
(gdb)
-break-insert -c junk -f main
^error,msg="No symbol \"junk\" in current context."
This restores the behavior that is present in the existing releases.
gdb/ChangeLog:
2021-04-21 Tankut Baris Aktemur <tankut.baris.aktemur@intel.com>
* breakpoint.h (create_breakpoint): Add a new parameter,
'force_condition'.
* breakpoint.c (create_breakpoint): Use the 'force_condition'
argument when 'parse_extra' is false to check if the condition
is invalid at all of the breakpoint locations.
Update the users below.
(break_command_1)
(dprintf_command)
(trace_command)
(ftrace_command)
(strace_command)
(create_tracepoint_from_upload): Update.
* guile/scm-breakpoint.c (gdbscm_register_breakpoint_x): Update.
* mi/mi-cmd-break.c (mi_cmd_break_insert_1): Update.
* python/py-breakpoint.c (bppy_init): Update.
* python/py-finishbreakpoint.c (bpfinishpy_init): Update.
gdb/testsuite/ChangeLog:
2021-04-21 Tankut Baris Aktemur <tankut.baris.aktemur@intel.com>
* gdb.mi/mi-break.exp: Extend with checks for invalid breakpoint
conditions.
This changes quick_symbol_functions::map_symbol_filenames to use a
function_view, and updates all the uses. It also changes the final
parameter to 'bool'. A couple of spots are further updated to use
operator() rather than a lambda.
gdb/ChangeLog
2021-03-26 Tom Tromey <tom@tromey.com>
* symtab.c (struct output_source_filename_data): Add 'output'
method and operator().
(output_source_filename_data::output): Rename from
output_source_filename.
(output_partial_symbol_filename): Remove.
(info_sources_command): Update.
(struct add_partial_filename_data): Add operator().
(add_partial_filename_data::operator()): Rename from
maybe_add_partial_symtab_filename.
(make_source_files_completion_list): Update.
* symfile.c (quick_symbol_functions): Update.
* symfile-debug.c (objfile::map_symbol_filenames): Update.
* quick-symbol.h (symbol_filename_ftype): Change type of 'fun' and
'need_fullname'. Remove 'data' parameter.
(struct quick_symbol_functions) <map_symbol_filenames>: Likewise.
* psymtab.c (psymbol_functions::map_symbol_filenames): Update.
* psympriv.h (struct psymbol_functions) <map_symbol_filenames>:
Change type of 'fun' and 'need_fullname'. Remove 'data'
parameter.
* objfiles.h (struct objfile) <map_symbol_filenames>: Change type
of 'fun' and 'need_fullname'. Remove 'data' parameter.
* mi/mi-cmd-file.c (print_partial_file_name): Remove 'ignore'
parameter.
(mi_cmd_file_list_exec_source_files): Update.
* dwarf2/read.c
(dwarf2_base_index_functions::map_symbol_filenames): Update.
The rationale for this patch comes from the ROCm port [1], the goal
being to reduce the number of back and forths between GDB and the
target when doing successive operations. I'll start with explaining
the rationale and then go over the implementation. In the ROCm / GPU
world, the term "wave" is somewhat equivalent to a "thread" in GDB.
So if you read if from a GPU stand point, just s/thread/wave/.
ROCdbgapi, the library used by GDB [2] to communicate with the GPU
target, gives the illusion that it's possible for the debugger to
control (start and stop) individual threads. But in reality, this is
not how it works. Under the hood, all threads of a queue are
controlled as a group. To stop one thread in a group of running ones,
the state of all threads is retrieved from the GPU, all threads are
destroyed, and all threads but the one we want to stop are re-created
from the saved state. The net result, from the point of view of GDB,
is that the library stopped one thread. The same thing goes if we
want to resume one thread while others are running: the state of all
running threads is retrieved from the GPU, they are all destroyed, and
they are all re-created, including the thread we want to resume.
This leads to some inefficiencies when combined with how GDB works,
here are two examples:
- Stopping all threads: because the target operates in non-stop mode,
when the user interface mode is all-stop, GDB must stop all threads
individually when presenting a stop. Let's suppose we have 1000
threads and the user does ^C. GDB asks the target to stop one
thread. Behind the scenes, the library retrieves 1000 thread
states and restores the 999 others still running ones. GDB asks
the target to stop another one. The target retrieves 999 thread
states and restores the 998 remaining ones. That means that to
stop 1000 threads, we did 1000 back and forths with the GPU. It
would have been much better to just retrieve the states once and
stop there.
- Resuming with pending events: suppose the 1000 threads hit a
breakpoint at the same time. The breakpoint is conditional and
evaluates to true for the first thread, to false for all others.
GDB pulls one event (for the first thread) from the target, decides
that it should present a stop, so stops all threads using
stop_all_threads. All these other threads have a breakpoint event
to report, which is saved in `thread_info::suspend::waitstatus` for
later. When the user does "continue", GDB resumes that one thread
that did hit the breakpoint. It then processes the pending events
one by one as if they just arrived. It picks one, evaluates the
condition to false, and resumes the thread. It picks another one,
evaluates the condition to false, and resumes the thread. And so
on. In between each resumption, there is a full state retrieval
and re-creation. It would be much nicer if we could wait a little
bit before sending those threads on the GPU, until it processed all
those pending events.
To address this kind of performance issue, ROCdbgapi has a concept
called "forward progress required", which is a boolean state that
allows its user (i.e. GDB) to say "I'm doing a bunch of operations,
you can hold off putting the threads on the GPU until I'm done" (the
"forward progress not required" state). Turning forward progress back
on indicates to the library that all threads that are supposed to be
running should now be really running on the GPU.
It turns out that GDB has a similar concept, though not as general,
commit_resume. One difference is that commit_resume is not stateful:
the target can't look up "does the core need me to schedule resumed
threads for execution right now". It is also specifically linked to
the resume method, it is not used in other contexts. The target
accumulates resumption requests through target_ops::resume calls, and
then commits those resumptions when target_ops::commit_resume is
called. The target has no way to check if it's ok to leave resumed
threads stopped in other target methods.
To bridge the gap, this patch generalizes the commit_resume concept in
GDB to match the forward progress concept of ROCdbgapi. The current
name (commit_resume) can be interpreted as "commit the previous resume
calls". I renamed the concept to "commit_resumed", as in "commit the
threads that are resumed".
In the new version, we have two things:
- the commit_resumed_state field in process_stratum_target: indicates
whether GDB requires target stacks using this target to have
resumed threads committed to the execution target/device. If
false, an execution target is allowed to leave resumed threads
un-committed at the end of whatever method it is executing.
- the commit_resumed target method: called when commit_resumed_state
transitions from false to true. While commit_resumed_state was
false, the target may have left some resumed threads un-committed.
This method being called tells it that it should commit them back
to the execution device.
Let's take the "Stopping all threads" scenario from above and see how
it would work with the ROCm target with this change. Before stopping
all threads, GDB would set the target's commit_resumed_state field to
false. It would then ask the target to stop the first thread. The
target would retrieve all threads' state from the GPU and mark that
one as stopped. Since commit_resumed_state is false, it leaves all
the other threads (still resumed) stopped. GDB would then proceed to
call target_stop for all the other threads. Since resumed threads are
not committed, this doesn't do any back and forth with the GPU.
To simplify the implementation of targets, this patch makes it so that
when calling certain target methods, the contract between the core and
the targets guarantees that commit_resumed_state is false. This way,
the target doesn't need two paths, one for commit_resumed_state ==
true and one for commit_resumed_state == false. It can just assert
that commit_resumed_state is false and work with that assumption.
This also helps catch places where we forgot to disable
commit_resumed_state before calling the method, which represents a
probable optimization opportunity. The commit adds assertions in the
target method wrappers (target_resume and friends) to have some
confidence that this contract between the core and the targets is
respected.
The scoped_disable_commit_resumed type is used to disable the commit
resumed state of all process targets on construction, and selectively
re-enable it on destruction (see below for criteria). Note that it
only sets the process_stratum_target::commit_resumed_state flag. A
subsequent call to maybe_call_commit_resumed_all_targets is necessary
to call the commit_resumed method on all target stacks with process
targets that got their commit_resumed_state flag turned back on. This
separation is because we don't want to call the commit_resumed methods
in scoped_disable_commit_resumed's destructor, as they may throw.
On destruction, commit-resumed is not re-enabled for a given target
if:
1. this target has no threads resumed, or
2. this target has at least one resumed thread with a pending status
known to the core (saved in thread_info::suspend::waitstatus).
The first point is not technically necessary, because a proper
commit_resumed implementation would be a no-op if the target has no
resumed threads. But since we have a flag do to a quick check, it
shouldn't hurt.
The second point is more important: together with the
scoped_disable_commit_resumed instance added in fetch_inferior_event,
it makes it so the "Resuming with pending events" described above is
handled efficiently. Here's what happens in that case:
1. The user types "continue".
2. Upon destruction, the scoped_disable_commit_resumed in the
`proceed` function does not enable commit-resumed, as it sees some
threads have pending statuses.
3. fetch_inferior_event is called to handle another event, the
breakpoint hit evaluates to false, and that thread is resumed.
Because there are still more threads with pending statuses, the
destructor of scoped_disable_commit_resumed in
fetch_inferior_event still doesn't enable commit-resumed.
4. Rinse and repeat step 3, until the last pending status is handled
by fetch_inferior_event. In that case,
scoped_disable_commit_resumed's destructor sees there are no more
threads with pending statues, so it asks the target to commit
resumed threads.
This allows us to avoid all unnecessary back and forths, there is a
single commit_resumed call once all pending statuses are processed.
This change required remote_target::remote_stop_ns to learn how to
handle stopping threads that were resumed but pending vCont. The
simplest example where that happens is when using the remote target in
all-stop, but with "maint set target-non-stop on", to force it to
operate in non-stop mode under the hood. If two threads hit a
breakpoint at the same time, GDB will receive two stop replies. It
will present the stop for one thread and save the other one in
thread_info::suspend::waitstatus.
Before this patch, when doing "continue", GDB first resumes the thread
without a pending status:
Sending packet: $vCont;c:p172651.172676#f3
It then consumes the pending status in the next fetch_inferior_event
call:
[infrun] do_target_wait_1: Using pending wait status status->kind = stopped, signal = GDB_SIGNAL_TRAP for Thread 1517137.1517137.
[infrun] target_wait (-1.0.0, status) =
[infrun] 1517137.1517137.0 [Thread 1517137.1517137],
[infrun] status->kind = stopped, signal = GDB_SIGNAL_TRAP
It then realizes it needs to stop all threads to present the stop, so
stops the thread it just resumed:
[infrun] stop_all_threads: Thread 1517137.1517137 not executing
[infrun] stop_all_threads: Thread 1517137.1517174 executing, need stop
remote_stop called
Sending packet: $vCont;t:p172651.172676#04
This is an unnecessary resume/stop. With this patch, we don't commit
resumed threads after proceeding, because of the pending status:
[infrun] maybe_commit_resumed_all_process_targets: not requesting commit-resumed for target extended-remote, a thread has a pending waitstatus
When GDB handles the pending status and stop_all_threads runs, we stop a
resumed but pending vCont thread:
remote_stop_ns: Enqueueing phony stop reply for thread pending vCont-resume (1520940, 1520976, 0)
That thread was never actually resumed on the remote stub / gdbserver,
so we shouldn't send a packet to the remote side asking to stop the
thread.
Note that there are paths that resume the target and then do a
synchronous blocking wait, in sort of nested event loop, via
wait_sync_command_done. For example, inferior function calls, or any
run control command issued from a breakpoint command list. We handle
that making wait_sync_command_one a "sync" point -- force forward
progress, or IOW, force-enable commit-resumed state.
gdb/ChangeLog:
yyyy-mm-dd Simon Marchi <simon.marchi@efficios.com>
Pedro Alves <pedro@palves.net>
* infcmd.c (run_command_1, attach_command, detach_command)
(interrupt_target_1): Use scoped_disable_commit_resumed.
* infrun.c (do_target_resume): Remove
target_commit_resume call.
(commit_resume_all_targets): Remove.
(maybe_set_commit_resumed_all_targets): New.
(maybe_call_commit_resumed_all_targets): New.
(enable_commit_resumed): New.
(scoped_disable_commit_resumed::scoped_disable_commit_resumed)
(scoped_disable_commit_resumed::~scoped_disable_commit_resumed)
(scoped_disable_commit_resumed::reset)
(scoped_disable_commit_resumed::reset_and_commit)
(scoped_enable_commit_resumed::scoped_enable_commit_resumed)
(scoped_enable_commit_resumed::~scoped_enable_commit_resumed):
New.
(proceed): Use scoped_disable_commit_resumed and
maybe_call_commit_resumed_all_targets.
(fetch_inferior_event): Use scoped_disable_commit_resumed.
* infrun.h (struct scoped_disable_commit_resumed): New.
(maybe_call_commit_resumed_all_process_targets): New.
(struct scoped_enable_commit_resumed): New.
* mi/mi-main.c (exec_continue): Use scoped_disable_commit_resumed.
* process-stratum-target.h (class process_stratum_target):
<commit_resumed_state>: New.
* record-full.c (record_full_wait_1): Change commit_resumed_state
around calling commit_resumed.
* remote.c (class remote_target) <commit_resume>: Rename to...
<commit_resumed>: ... this.
(struct stop_reply): Move up.
(remote_target::commit_resume): Rename to...
(remote_target::commit_resumed): ... this. Check if there is any
thread pending vCont resume.
(remote_target::remote_stop_ns): Generate stop replies for resumed
but pending vCont threads.
(remote_target::wait_ns): Add gdb_assert.
* target-delegates.c: Regenerate.
* target.c (target_wait, target_resume): Assert that the current
process_stratum target isn't in commit-resumed state.
(defer_target_commit_resume): Remove.
(target_commit_resume): Remove.
(target_commit_resumed): New.
(make_scoped_defer_target_commit_resume): Remove.
(target_stop): Assert that the current process_stratum target
isn't in commit-resumed state.
* target.h (struct target_ops) <commit_resume>: Rename to ...
<commit_resumed>: ... this.
(target_commit_resume): Remove.
(target_commit_resumed): New.
(make_scoped_defer_target_commit_resume): Remove.
* top.c (wait_sync_command_done): Use
scoped_enable_commit_resumed.
[1] https://github.com/ROCm-Developer-Tools/ROCgdb/
[2] https://github.com/ROCm-Developer-Tools/ROCdbgapi
Change-Id: I836135531a29214b21695736deb0a81acf8cf566
The current_top_target function is a hidden dependency on the current
inferior. Since I'd like to slowly move towards reducing our dependency
on the global current state, remove this function and make callers use
current_inferior ()->top_target ()
There is no expected change in behavior, but this one step towards
making those callers use the inferior from their context, rather than
refer to the global current inferior.
gdb/ChangeLog:
* target.h (current_top_target): Remove, make callers use the
current inferior instead.
* target.c (current_top_target): Remove.
Change-Id: Iccd457036f84466cdaa3865aa3f9339a24ea001d
