'finish_print' does not really belong in value_print_options -- this
is consulted only when deciding whether or not to print a value, and
never during the course of printing. This patch removes it from the
structure and makes it a static global in infcmd.c instead.
Tested on x86-64 Fedora 34.
While working on this patch:
https://sourceware.org/pipermail/gdb-patches/2022-January/185109.html
I found it really useful to print the executing/resumed status of all
threads (or all threads in a particular inferior) at various
places (e.g. when a new inferior is started, when GDB attaches, etc).
This debug was originally part of the above patch, but I wanted to
rewrite this as a separate patch and move the code into a new function
in infrun.h, which is what this patch does.
Unless 'set debug infrun on' is in effect, then there should be no
user visible changes after this commit.
Move 'struct reggroup' into the reggroups.h header. Remove the
reggroup_name and reggroup_type accessor functions, and just use the
name/type member functions within 'struct reggroup', update all uses
of these removed functions.
There should be no user visible changes after this commit.
Add a new function gdbarch_reggroups that returns a reference to a
vector containing all the reggroups for an architecture.
Make use of this function throughout GDB instead of the existing
reggroup_next and reggroup_prev functions.
Finally, delete the reggroup_next and reggroup_prev functions.
Most of these changes are pretty straight forward, using range based
for loops instead of the old style look using reggroup_next. There
are two places where the changes are less straight forward.
In gdb/python/py-registers.c, the register group iterator needed to
change slightly. As the iterator is tightly coupled to the gdbarch, I
just fetch the register group vector from the gdbarch when needed, and
use an index counter to find the next item from the vector when
needed.
In gdb/tui/tui-regs.c the tui_reg_next and tui_reg_prev functions are
just wrappers around reggroup_next and reggroup_prev respectively.
I've just inlined the logic of the old functions into the tui
functions. As the tui function had its own special twist (wrap around
behaviour) I think this is OK.
There should be no user visible changes after this commit.
Add a "reason" parameter, only used to show in debug messages what is
the reason for stopping all threads. This helped me understand the
debug logs while adding some new uses of stop_all_threads, so I am
proposing to merge it.
Change-Id: I66c8c335ebf41836a7bc3d5fe1db92c195f65e55
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.
A number of spots call printf_unfiltered only because they are in code
that should not be interrupted by the pager. However, I believe these
cases are all handled by infrun's blanket ban on paging, and so can be
converted to the default (_filtered) API.
After this patch, I think all the remaining _unfiltered calls are ones
that really ought to be. A few -- namely in complete_command -- could
be replaced by a scoped assignment to pagination_enabled, but for the
remainder, the code seems simple enough like this.
It is possible for a compiler to optimize a function in a such ways that
the function does not follow the calling convention of the target. In
such situation, the compiler can use the DW_AT_calling_convention
attribute with the value DW_CC_nocall to tell the debugger that it is
unsafe to call the function. The DWARF5 standard states, in 3.3.1.1:
> If the value of the calling convention attribute is the constant
> DW_CC_nocall, the subroutine does not obey standard calling
> conventions, and it may not be safe for the debugger to call this
> subroutine.
Non standard calling convention can affect GDB's assumptions in multiple
ways, including how arguments are passed to the function, how values are
returned, and so on. For this reason, it is unsafe for GDB to try to do
the following operations on a function with marked with DW_CC_nocall:
- call / print an expression requiring the function to be evaluated,
- inspect the value a function returns using the 'finish' command,
- force the value returned by a function using the 'return' command.
This patch ensures that if a command which relies on GDB's knowledge of
the target's calling convention is used on a function marked nocall, GDB
prints an appropriate message to the user and does not proceed with the
operation which is unreliable.
Note that it is still possible for someone to use a vendor specific
value for the DW_AT_calling_convention attribute for example to indicate
the use of an alternative calling convention. This commit does not
prevent this, and target dependent code can be adjusted if one wanted to
support multiple calling conventions.
Tested on x86_64-Linux, with no regression observed.
Change-Id: I72970dae68234cb83edbc0cf71aa3d6002a4a540
Add an argument to the get_return_value function to indicate the symbol
of the function the debuggee is returning from. This will be used by
the following patch.
Since the function return type can be deduced from the symbol remove the
value_type argument which becomes redundant.
No user visible change after this patch.
Tested on x86_64-linux.
Change-Id: Idf1279f1f7199f5022738a6679e0fa63fbd22edc
Co-authored-by: Simon Marchi <simon.marchi@polymtl.ca>
When using the command "until", it is expected that GDB will exit a
loop if the current instruction is the last one related to that loop.
However, if there were trailing non-statement instructions, "until"
would just behave as "next". This was noticeable in clang-compiled
code, but might happen with gcc-compiled as well. PR gdb/17315 relates
to this problem, as running gdb.base/watchpoint.exp with clang
would fail for this reason.
To better understand this issue, consider the following source code,
with line numbers marked on the left:
10: for (i = 0; i < 10; ++i)
11: loop_body ();
12: other_stuff ();
If we transform this to pseudo-assembler, and generate a line table,
we could end up with something like this:
Address | Pseudo-Assembler | Line | Is-Statement?
0x100 | i = 0 | 10 | Yes
0x104 | loop_body () | 11 | Yes
0x108 | i = i + 1 | 10 | Yes
0x10c | if (i < 10): | 10 | No
0x110 | goto 0x104 | 10 | No
0x114 | other_stuff () | 12 | Yes
Notice the two non-statement instructions at the end of the loop.
The problem is that when we reach address 0x108 and use 'until',
hoping to leave the loop, GDB sets up a stepping range that runs from
the start of the function (0x100 in our example) to the end of the
current line table entry, that is 0x10c in our example. GDB then
starts stepping forward.
When 0x10c is reached GDB spots that we have left the stepping range,
that the new location is not a statement, and that the new location is
associated with the same source line number as the previous stepping
range. GDB then sets up a new stepping range that runs from 0x10c to
0x114, and continues stepping forward.
Within that stepping range the inferior hits the goto (at 0x110) and
loops back to address 0x104.
At 0x104 GDB spots that we have left the previous stepping range, that
the new address is marked as a statement, and that the new address is
for a different source line. As a result, GDB stops and returns
control to the user. This is not what the user was expecting, they
expected GDB to exit the loop.
The fix proposed in this patch, is that, when the user issues the
'until' command, and GDB sets up the initial stepping range, GDB will
check subsequent SALs (symtab_and_lines) to see if they are
non-statements associated with the same line number. If they are then
the end of the initial stepping range is extended to the end of the
non-statement SALs.
In our example above, the user is at 0x108 and uses 'until', GDB now
sets up a stepping range from the start of the function 0x100 to
0x114, the first address associated with a different line.
Now as GDB steps around the loop it never leaves the initial stepping
range. It is only when GDB exits the loop that we leave the stepping
range, and the stepping finishes at address 0x114.
This patch also adds a test case that can be run with gcc to test that
this functionality is not broken in the future.
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=17315
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 type. Remove the corresponding
macro and adjust all callers.
Change-Id: Ie1a137744c5bfe1df4d4f9ae5541c5299577c8de
In an earlier version of the pager rewrite series, it was important to
audit unfiltered output calls to see which were truly necessary.
This is no longer necessary, but it still seems like a decent cleanup
to change calls to avoid explicitly passing gdb_stdout. That is,
rather than using something like fprintf_unfiltered with gdb_stdout,
the code ought to use plain printf_unfiltered instead.
This patch makes this change. I went ahead and converted all the
_filtered calls I could find, as well, for the same clarity.
While working on another patch I wanted to add some extra debug
information to the attach_command function. This required me to add a
new function to convert the thread_info::state variable to a string.
The new debug might be useful to others, and the state to string
function might be useful in other locations, so I thought I'd merge
it.
This changes the kill command to use filtered output. I split this
one into its own patch because, out of an abundance of caution, I
changed the function to call bfd_cache_close_all a bit earlier, in
case pagination caused an exception.
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.
I happened to notice that one "show" callback was printing to
gdb_stdout rather than to the passed-in ui_file parameter. I went
through all such callbacks and fixed them to consistently use the
ui_file.
Regression tested on x86-64 Fedora 34.
When running test-case gdb.base/cached-source-file.exp with target board
readnow, we run into:
...
FAIL: gdb.base/cached-source-file.exp: rerun program (the program exited)
...
The problem is that when rereading, the readnow is ignored.
Fix this by copying the readnow handling code from symbol_file_add_with_addrs
to reread_symbols.
Tested on x86_64-linux.
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=26800
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.
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
The bug fixed by this [1] patch was caused by an out-of-bounds access to
a value's content. The code gets the value's content (just a pointer)
and then indexes it with a non-sensical index.
This made me think of changing functions that return value contents to
return array_views instead of a plain pointer. This has the advantage
that when GDB is built with _GLIBCXX_DEBUG, accesses to the array_view
are checked, making bugs more apparent / easier to find.
This patch changes the return types of these functions, and updates
callers to call .data() on the result, meaning it's not changing
anything in practice. Additional work will be needed (which can be done
little by little) to make callers propagate the use of array_view and
reap the benefits.
[1] https://sourceware.org/pipermail/gdb-patches/2021-September/182306.html
Change-Id: I5151f888f169e1c36abe2cbc57620110673816f3
I stumbled on a bug caused by the fact that a code path read
target_waitstatus::value::sig (expecting it to contain a gdb_signal
value) while target_waitstatus::kind was TARGET_WAITKIND_FORKED. This
meant that the active union field was in fact
target_waitstatus::value::related_pid, and contained a ptid. The read
signal value was therefore garbage, and that caused GDB to crash soon
after. Or, since that GDB was built with ubsan, this nice error
message:
/home/simark/src/binutils-gdb/gdb/linux-nat.c:1271:12: runtime error: load of value 2686365, which is not a valid value for type 'gdb_signal'
Despite being a large-ish change, I think it would be nice to make
target_waitstatus safe against that kind of bug. As already done
elsewhere (e.g. dynamic_prop), validate that the type of value read from
the union matches what is supposed to be the active field.
- Make the kind and value of target_waitstatus private.
- Make the kind initialized to TARGET_WAITKIND_IGNORE on
target_waitstatus construction. This is what most users appear to do
explicitly.
- Add setters, one for each kind. Each setter takes as a parameter the
data associated to that kind, if any. This makes it impossible to
forget to attach the associated data.
- Add getters, one for each associated data type. Each getter
validates that the data type fetched by the user matches the wait
status kind.
- Change "integer" to "exit_status", "related_pid" to "child_ptid",
just because that's more precise terminology.
- Fix all users.
That last point is semi-mechanical. There are a lot of obvious changes,
but some less obvious ones. For example, it's not possible to set the
kind at some point and the associated data later, as some users did.
But in any case, the intent of the code should not change in this patch.
This was tested on x86-64 Linux (unix, native-gdbserver and
native-extended-gdbserver boards). It was built-tested on x86-64
FreeBSD, NetBSD, MinGW and macOS. The rest of the changes to native
files was done as a best effort. If I forgot any place to update in
these files, it should be easy to fix (unless the change happens to
reveal an actual bug).
Change-Id: I0ae967df1ff6e28de78abbe3ac9b4b2ff4ad03b7
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>
Currently the stop_pc field of thread_suspect_state is a CORE_ADDR and
when we want to indicate that there is no stop_pc available we set
this field back to a special value.
There are actually two special values used, in post_create_inferior
the stop_pc is set to 0. This is a little unfortunate, there are
plenty of embedded targets where 0 is a valid pc value. The more
common special value for stop_pc though, is set in
thread_info::set_executing, where the value (~(CORE_ADDR) 0) is used.
This commit changes things so that the stop_pc is instead a
gdb::optional. We can now explicitly reset the field to an
uninitialised state, we also have asserts that we don't read the
stop_pc when its in an uninitialised state (both in
gdbsupport/gdb_optional.h, when compiling with _GLIBCXX_DEBUG
defined, and in thread_info::stop_pc).
One situation where a thread will not have a stop_pc value is when the
thread is stopped as a consequence of GDB being in all stop mode, and
some other thread stopped at an interesting event. When GDB brings
all the other threads to a stop those other threads will not have a
stop_pc set (thus avoiding an unnecessary read of the pc register).
Previously, when GDB passed through handle_one (in infrun.c) the
threads executing flag was set to false and the stop_pc field was left
unchanged, i.e. it would (previous) have been left as ~0.
Now, handle_one leaves the stop_pc with no value.
This caused a problem when we later try to set these threads running
again, in proceed() we compare the current pc with the cached stop_pc.
If the thread was stopped via handle_one then the stop_pc would have
been left as ~0, and the compare (in proceed) would (likely) fail.
Now however, this compare tries to read the stop_pc when it has no
value and this would trigger an assert.
To resolve this I've added thread_info::stop_pc_p() which returns true
if the thread has a cached stop_pc. We should only ever call
thread_info::stop_pc() if we know that there is a cached stop_pc,
however, this doesn't mean that every call to thread_info::stop_pc()
needs to be guarded with a call to thread_info::stop_pc_p(), in most
cases we know that the thread we are looking at stopped due to some
interesting event in that thread, and so, we know that the stop_pc is
valid.
After running the testsuite I've seen no other situations where
stop_pc is read uninitialised.
There should be no user visible changes after this commit.
Rename thread_info::executing to thread_info::m_executing, and make it
private. Add a new get/set member functions, and convert GDB to make
use of these.
The only real change of interest in this patch is in thread.c where I
have deleted the helper function set_executing_thread, and now just
use the new set function thread_info::set_executing. However, the old
helper function set_executing_thread included some code to reset the
thread's stop_pc, so I moved this code into the new function
thread_info::set_executing. However, I don't believe there is
anywhere that this results in a change of behaviour, previously the
executing flag was always set true through a call to
set_executing_thread anyway.
Same idea as the previous patch, but for m_cwd.
To keep things consistent across the board, change get_inferior_cwd as
well, which is shared with GDBserver. So update the related GDBserver
code too.
Change-Id: Ia2c047fda738d45f3d18bc999eb67ceb8400ce4e
With the current code, both a NULL pointer and an empty string can mean
"no arguments". We don't need this distinction. Changing to a string
has the advantage that there is now a single state for that (an empty
string), which makes the code a bit simpler in my opinion.
Change-Id: Icdc622820f7869478791dbaa84b4a1c7fec21ced
Add cwd/set_cwd to the inferior class, remove set_inferior_args.
Keep get_inferior_args, because it is used from fork_inferior, in shared
code. The cwd could eventually be passed as a parameter eventually,
though, I think that would be cleaner.
Change-Id: Ifb72ea865d7e6f9a491308f0d5c1595579d8427e
Add args/set_args to the inferior class, remove the set_inferior_args
and get_inferior_args functions, that would just be wrappers around
them.
Change-Id: If87d52f3402ce08be26c32897ae8915d9f6d1ea3
There are currently two states that the inferior args can be stored.
The main one is the `args` field, where they are stored as a single
string. The other one is the `argc`/`argv` fields.
This last one is only used for arguments passed in GDB's
command line. And the only outcome is that when get_inferior_args is
called, `argc`/`argv` are serialized into `args`. So really,
`argc`/`argv` is just a staging area before moving the arguments in
`args`.
Simplify this by only keeping the `args` field. Change
set_inferior_args_vector to immediately serialize the arguments into
`args`, work that would be done in get_inferior_args later anyway.
The only time where this work would be "wasted" is when the user passes
some arguments on the command line, but does not end up running the
program. But that just seems unlikely. And it's not that much work.
Change-Id: Ica0b9859397c095f6530350c8fb3c36905f2044a
The declaration of set_inferior_cwd is currently shared between gdb and
gdbserver, in gdbsupport/common-inferior.h. It doesn't need to be, as
set_inferior_cwd is not called from common code. Only get_inferior_cwd
needs to.
The motivation for this is that a future patch will change the prototype
of set_inferior_cwd in gdb, and I don't want to change it for gdbserver
unnecessarily. I see this as a good cleanup in any case, to reduce to
just the essential what is shared between GDB and GDBserver.
Change-Id: I3127d27d078f0503ebf5ccc6fddf14f212426a73
A following patch will want to take some action when a pending wait
status is set on or removed from a thread. Add a getter and a setter on
thread_info for the pending waitstatus, so that we can add some code in
the setter later.
The thing is, the pending wait status field is in the
thread_suspend_state, along with other fields that we need to backup
before and restore after the thread does an inferior function call.
Therefore, make the thread_suspend_state member private
(thread_info::suspend becomes thread_info::m_suspend), and add getters /
setters for all of its fields:
- pending wait status
- stop signal
- stop reason
- stop pc
For the pending wait status, add the additional has_pending_waitstatus
and clear_pending_waitstatus methods.
I think this makes the thread_info interface a bit nicer, because we
now access the fields as:
thread->stop_pc ()
rather than
thread->suspend.stop_pc
The stop_pc field being in the `suspend` structure is an implementation
detail of thread_info that callers don't need to be aware of.
For the backup / restore of the thread_suspend_state structure, add
save_suspend_to and restore_suspend_from methods. You might wonder why
`save_suspend_to`, as opposed to a simple getter like
thread_suspend_state &suspend ();
I want to make it clear that this is to be used only for backing up and
restoring the suspend state, _not_ to access fields like:
thread->suspend ()->stop_pc
Adding some getters / setters allows adding some assertions. I find
that this helps understand how things are supposed to work. Add:
- When getting the pending status (pending_waitstatus method), ensure
that there is a pending status.
- When setting a pending status (set_pending_waitstatus method), ensure
there is no pending status.
There is one case I found where this wasn't true - in
remote_target::process_initial_stop_replies - which needed adjustments
to respect that contract. I think it's because
process_initial_stop_replies is kind of (ab)using the
thread_info::suspend::waitstatus to store some statuses temporarily, for
its internal use (statuses it doesn't intent on leaving pending).
process_initial_stop_replies pulls out stop replies received during the
initial connection using target_wait. It always stores the received
event in `evthread->suspend.waitstatus`. But it only sets
waitstatus_pending_p, if it deems the event interesting enough to leave
pending, to be reported to the core:
if (ws.kind != TARGET_WAITKIND_STOPPED
|| ws.value.sig != GDB_SIGNAL_0)
evthread->suspend.waitstatus_pending_p = 1;
It later uses this flag a bit below, to choose which thread to make the
"selected" one:
if (selected == NULL
&& thread->suspend.waitstatus_pending_p)
selected = thread;
And ultimately that's used if the user-visible mode is all-stop, so that
we print the stop for that interesting thread:
/* In all-stop, we only print the status of one thread, and leave
others with their status pending. */
if (!non_stop)
{
thread_info *thread = selected;
if (thread == NULL)
thread = lowest_stopped;
if (thread == NULL)
thread = first;
print_one_stopped_thread (thread);
}
But in any case (all-stop or non-stop), print_one_stopped_thread needs
to access the waitstatus value of these threads that don't have a
pending waitstatus (those that had TARGET_WAITKIND_STOPPED +
GDB_SIGNAL_0). This doesn't work with the assertions I've
put.
So, change the code to only set the thread's wait status if it is an
interesting one that we are going to leave pending. If the thread
stopped due to a non-interesting event (TARGET_WAITKIND_STOPPED +
GDB_SIGNAL_0), don't store it. Adjust print_one_stopped_thread to
understand that if a thread has no pending waitstatus, it's because it
stopped with TARGET_WAITKIND_STOPPED + GDB_SIGNAL_0.
The call to set_last_target_status also uses the pending waitstatus.
However, given that the pending waitstatus for the thread may have been
cleared in print_one_stopped_thread (and that there might not even be a
pending waitstatus in the first place, as explained above), it is no
longer possible to do it at this point. To fix that, move the call to
set_last_target_status in print_one_stopped_thread. I think this will
preserve the existing behavior, because set_last_target_status is
currently using the current thread's wait status. And the current
thread is the last one for which print_one_stopped_thread is called. So
by calling set_last_target_status in print_one_stopped_thread, we'll get
the same result. set_last_target_status will possibly be called
multiple times, but only the last call will matter. It just means
possibly more calls to set_last_target_status, but those are cheap.
Change-Id: Iedab9653238eaf8231abcf0baa20145acc8b77a7
When trying to attach to a pthread process on a Linux system with glibc 2.33,
we get:
$ ./gdb -q -nx --data-directory=data-directory -p 1472010
Attaching to process 1472010
[New LWP 1472013]
[New LWP 1472014]
[New LWP 1472015]
Error while reading shared library symbols for /usr/lib/libpthread.so.0:
Cannot find user-level thread for LWP 1472015: generic error
0x00007ffff6d3637f in poll () from /usr/lib/libc.so.6
(gdb)
When attaching to a process (or handling a fork child, an operation very
similar to attaching), GDB reads the shared library list from the
process. For each shared library (if "set auto-solib-add" is on), it
reads its symbols and calls the "new_objfile" observable.
The libthread-db code monitors this observable, and if it sees an
objfile named somewhat like "libpthread.so" go by, it tries to load
libthread_db.so in the GDB process itself. libthread_db knows how to
navigate libpthread's data structures to get information about the
existing threads.
To locate these data structures, libthread_db calls ps_pglobal_lookup
(implemented in proc-service.c), passing in a symbol name and expecting
an address in return.
Before glibc 2.33, libthread_db always asked for symbols found in
libpthread. There was no ordering problem: since we were always trying
to load libthread_db in reaction to processing libpthread (and reading
in its symbols) and libthread_db only asked symbols from libpthread, the
requested symbols could always be found. Starting with glibc 2.33,
libthread_db now asks for a symbol name that can be found in
/lib/ld-linux-x86-64.so.2 (_rtld_global). And the ordering in which GDB
reads the shared libraries from the inferior when attaching is
unfortunate, in that libpthread is processed before ld-linux. So when
loading libthread_db in reaction to processing libpthread, and
libthread_db requests the symbol that is from ld-linux, GDB is not yet
able to supply it.
That problematic symbol lookup happens in the thread_from_lwp function,
when we call td_ta_map_lwp2thr_p, and an exception is thrown at this
point:
#0 0x00007ffff6681012 in __cxxabiv1::__cxa_throw (obj=0x60e000006100, tinfo=0x555560033b50 <typeinfo for gdb_exception_error>, dest=0x55555d9404bc <gdb_exception_error::~gdb_exception_error()>) at /build/gcc/src/gcc/libstdc++-v3/libsupc++/eh_throw.cc:78
#1 0x000055555e5d3734 in throw_it(return_reason, errors, const char *, typedef __va_list_tag __va_list_tag *) (reason=RETURN_ERROR, error=GENERIC_ERROR, fmt=0x55555f0c5360 "Cannot find user-level thread for LWP %ld: %s", ap=0x7fffffffaae0) at /home/simark/src/binutils-gdb/gdbsupport/common-exceptions.cc:200
#2 0x000055555e5d37d4 in throw_verror (error=GENERIC_ERROR, fmt=0x55555f0c5360 "Cannot find user-level thread for LWP %ld: %s", ap=0x7fffffffaae0) at /home/simark/src/binutils-gdb/gdbsupport/common-exceptions.cc:208
#3 0x000055555e0b0ed2 in verror (string=0x55555f0c5360 "Cannot find user-level thread for LWP %ld: %s", args=0x7fffffffaae0) at /home/simark/src/binutils-gdb/gdb/utils.c:171
#4 0x000055555e5e898a in error (fmt=0x55555f0c5360 "Cannot find user-level thread for LWP %ld: %s") at /home/simark/src/binutils-gdb/gdbsupport/errors.cc:43
#5 0x000055555d06b4bc in thread_from_lwp (stopped=0x617000035d80, ptid=...) at /home/simark/src/binutils-gdb/gdb/linux-thread-db.c:418
#6 0x000055555d07040d in try_thread_db_load_1 (info=0x60c000011140) at /home/simark/src/binutils-gdb/gdb/linux-thread-db.c:912
#7 0x000055555d071103 in try_thread_db_load (library=0x55555f0c62a0 "libthread_db.so.1", check_auto_load_safe=false) at /home/simark/src/binutils-gdb/gdb/linux-thread-db.c:1014
#8 0x000055555d072168 in try_thread_db_load_from_sdir () at /home/simark/src/binutils-gdb/gdb/linux-thread-db.c:1091
#9 0x000055555d072d1c in thread_db_load_search () at /home/simark/src/binutils-gdb/gdb/linux-thread-db.c:1146
#10 0x000055555d07365c in thread_db_load () at /home/simark/src/binutils-gdb/gdb/linux-thread-db.c:1203
#11 0x000055555d07373e in check_for_thread_db () at /home/simark/src/binutils-gdb/gdb/linux-thread-db.c:1246
#12 0x000055555d0738ab in thread_db_new_objfile (objfile=0x61300000c0c0) at /home/simark/src/binutils-gdb/gdb/linux-thread-db.c:1275
#13 0x000055555bd10740 in std::__invoke_impl<void, void (*&)(objfile*), objfile*> (__f=@0x616000068d88: 0x55555d073745 <thread_db_new_objfile(objfile*)>) at /usr/include/c++/10.2.0/bits/invoke.h:60
#14 0x000055555bd02096 in std::__invoke_r<void, void (*&)(objfile*), objfile*> (__fn=@0x616000068d88: 0x55555d073745 <thread_db_new_objfile(objfile*)>) at /usr/include/c++/10.2.0/bits/invoke.h:153
#15 0x000055555bce0392 in std::_Function_handler<void (objfile*), void (*)(objfile*)>::_M_invoke(std::_Any_data const&, objfile*&&) (__functor=..., __args#0=@0x7fffffffb4a0: 0x61300000c0c0) at /usr/include/c++/10.2.0/bits/std_function.h:291
#16 0x000055555d3595c0 in std::function<void (objfile*)>::operator()(objfile*) const (this=0x616000068d88, __args#0=0x61300000c0c0) at /usr/include/c++/10.2.0/bits/std_function.h:622
#17 0x000055555d356b7f in gdb::observers::observable<objfile*>::notify (this=0x555566727020 <gdb::observers::new_objfile>, args#0=0x61300000c0c0) at /home/simark/src/binutils-gdb/gdb/../gdbsupport/observable.h:106
#18 0x000055555da3f228 in symbol_file_add_with_addrs (abfd=0x61200001ccc0, name=0x6190000d9090 "/usr/lib/libpthread.so.0", add_flags=..., addrs=0x7fffffffbc10, flags=..., parent=0x0) at /home/simark/src/binutils-gdb/gdb/symfile.c:1131
#19 0x000055555da3f763 in symbol_file_add_from_bfd (abfd=0x61200001ccc0, name=0x6190000d9090 "/usr/lib/libpthread.so.0", add_flags=<error reading variable: Cannot access memory at address 0xffffffffffffffb0>, addrs=0x7fffffffbc10, flags=<error reading variable: Cannot access memory at address 0xffffffffffffffc0>, parent=0x0) at /home/simark/src/binutils-gdb/gdb/symfile.c:1167
#20 0x000055555d95f9fa in solib_read_symbols (so=0x6190000d8e80, flags=...) at /home/simark/src/binutils-gdb/gdb/solib.c:681
#21 0x000055555d96233d in solib_add (pattern=0x0, from_tty=0, readsyms=1) at /home/simark/src/binutils-gdb/gdb/solib.c:987
#22 0x000055555d93646e in enable_break (info=0x608000008f20, from_tty=0) at /home/simark/src/binutils-gdb/gdb/solib-svr4.c:2238
#23 0x000055555d93cfc0 in svr4_solib_create_inferior_hook (from_tty=0) at /home/simark/src/binutils-gdb/gdb/solib-svr4.c:3049
#24 0x000055555d96610d in solib_create_inferior_hook (from_tty=0) at /home/simark/src/binutils-gdb/gdb/solib.c:1195
#25 0x000055555cdee318 in post_create_inferior (from_tty=0) at /home/simark/src/binutils-gdb/gdb/infcmd.c:318
#26 0x000055555ce00e6e in setup_inferior (from_tty=0) at /home/simark/src/binutils-gdb/gdb/infcmd.c:2439
#27 0x000055555ce59c34 in handle_one (event=...) at /home/simark/src/binutils-gdb/gdb/infrun.c:4887
#28 0x000055555ce5cd00 in stop_all_threads () at /home/simark/src/binutils-gdb/gdb/infrun.c:5064
#29 0x000055555ce7f0da in stop_waiting (ecs=0x7fffffffd170) at /home/simark/src/binutils-gdb/gdb/infrun.c:8006
#30 0x000055555ce67f5c in handle_signal_stop (ecs=0x7fffffffd170) at /home/simark/src/binutils-gdb/gdb/infrun.c:6062
#31 0x000055555ce63653 in handle_inferior_event (ecs=0x7fffffffd170) at /home/simark/src/binutils-gdb/gdb/infrun.c:5727
#32 0x000055555ce4f297 in fetch_inferior_event () at /home/simark/src/binutils-gdb/gdb/infrun.c:4105
#33 0x000055555cdbe3bf in inferior_event_handler (event_type=INF_REG_EVENT) at /home/simark/src/binutils-gdb/gdb/inf-loop.c:42
#34 0x000055555d018047 in handle_target_event (error=0, client_data=0x0) at /home/simark/src/binutils-gdb/gdb/linux-nat.c:4060
#35 0x000055555e5ea77e in handle_file_event (file_ptr=0x60600008b1c0, ready_mask=1) at /home/simark/src/binutils-gdb/gdbsupport/event-loop.cc:575
#36 0x000055555e5eb09c in gdb_wait_for_event (block=0) at /home/simark/src/binutils-gdb/gdbsupport/event-loop.cc:701
#37 0x000055555e5e8d19 in gdb_do_one_event () at /home/simark/src/binutils-gdb/gdbsupport/event-loop.cc:212
#38 0x000055555dd6e0d4 in wait_sync_command_done () at /home/simark/src/binutils-gdb/gdb/top.c:528
#39 0x000055555dd6e372 in maybe_wait_sync_command_done (was_sync=0) at /home/simark/src/binutils-gdb/gdb/top.c:545
#40 0x000055555d0ec7c8 in catch_command_errors (command=0x55555ce01bb8 <attach_command(char const*, int)>, arg=0x7fffffffe28d "1472010", from_tty=1, do_bp_actions=false) at /home/simark/src/binutils-gdb/gdb/main.c:452
#41 0x000055555d0f03ad in captured_main_1 (context=0x7fffffffdd10) at /home/simark/src/binutils-gdb/gdb/main.c:1149
#42 0x000055555d0f1239 in captured_main (data=0x7fffffffdd10) at /home/simark/src/binutils-gdb/gdb/main.c:1232
#43 0x000055555d0f1315 in gdb_main (args=0x7fffffffdd10) at /home/simark/src/binutils-gdb/gdb/main.c:1257
#44 0x000055555bb70cf9 in main (argc=7, argv=0x7fffffffde88) at /home/simark/src/binutils-gdb/gdb/gdb.c:32
The exception is caught here:
#0 __cxxabiv1::__cxa_begin_catch (exc_obj_in=0x60e0000060e0) at /build/gcc/src/gcc/libstdc++-v3/libsupc++/eh_catch.cc:84
#1 0x000055555d95fded in solib_read_symbols (so=0x6190000d8e80, flags=...) at /home/simark/src/binutils-gdb/gdb/solib.c:689
#2 0x000055555d96233d in solib_add (pattern=0x0, from_tty=0, readsyms=1) at /home/simark/src/binutils-gdb/gdb/solib.c:987
#3 0x000055555d93646e in enable_break (info=0x608000008f20, from_tty=0) at /home/simark/src/binutils-gdb/gdb/solib-svr4.c:2238
#4 0x000055555d93cfc0 in svr4_solib_create_inferior_hook (from_tty=0) at /home/simark/src/binutils-gdb/gdb/solib-svr4.c:3049
#5 0x000055555d96610d in solib_create_inferior_hook (from_tty=0) at /home/simark/src/binutils-gdb/gdb/solib.c:1195
#6 0x000055555cdee318 in post_create_inferior (from_tty=0) at /home/simark/src/binutils-gdb/gdb/infcmd.c:318
#7 0x000055555ce00e6e in setup_inferior (from_tty=0) at /home/simark/src/binutils-gdb/gdb/infcmd.c:2439
#8 0x000055555ce59c34 in handle_one (event=...) at /home/simark/src/binutils-gdb/gdb/infrun.c:4887
#9 0x000055555ce5cd00 in stop_all_threads () at /home/simark/src/binutils-gdb/gdb/infrun.c:5064
#10 0x000055555ce7f0da in stop_waiting (ecs=0x7fffffffd170) at /home/simark/src/binutils-gdb/gdb/infrun.c:8006
#11 0x000055555ce67f5c in handle_signal_stop (ecs=0x7fffffffd170) at /home/simark/src/binutils-gdb/gdb/infrun.c:6062
#12 0x000055555ce63653 in handle_inferior_event (ecs=0x7fffffffd170) at /home/simark/src/binutils-gdb/gdb/infrun.c:5727
#13 0x000055555ce4f297 in fetch_inferior_event () at /home/simark/src/binutils-gdb/gdb/infrun.c:4105
#14 0x000055555cdbe3bf in inferior_event_handler (event_type=INF_REG_EVENT) at /home/simark/src/binutils-gdb/gdb/inf-loop.c:42
#15 0x000055555d018047 in handle_target_event (error=0, client_data=0x0) at /home/simark/src/binutils-gdb/gdb/linux-nat.c:4060
#16 0x000055555e5ea77e in handle_file_event (file_ptr=0x60600008b1c0, ready_mask=1) at /home/simark/src/binutils-gdb/gdbsupport/event-loop.cc:575
#17 0x000055555e5eb09c in gdb_wait_for_event (block=0) at /home/simark/src/binutils-gdb/gdbsupport/event-loop.cc:701
#18 0x000055555e5e8d19 in gdb_do_one_event () at /home/simark/src/binutils-gdb/gdbsupport/event-loop.cc:212
#19 0x000055555dd6e0d4 in wait_sync_command_done () at /home/simark/src/binutils-gdb/gdb/top.c:528
#20 0x000055555dd6e372 in maybe_wait_sync_command_done (was_sync=0) at /home/simark/src/binutils-gdb/gdb/top.c:545
#21 0x000055555d0ec7c8 in catch_command_errors (command=0x55555ce01bb8 <attach_command(char const*, int)>, arg=0x7fffffffe28d "1472010", from_tty=1, do_bp_actions=false) at /home/simark/src/binutils-gdb/gdb/main.c:452
#22 0x000055555d0f03ad in captured_main_1 (context=0x7fffffffdd10) at /home/simark/src/binutils-gdb/gdb/main.c:1149
#23 0x000055555d0f1239 in captured_main (data=0x7fffffffdd10) at /home/simark/src/binutils-gdb/gdb/main.c:1232
#24 0x000055555d0f1315 in gdb_main (args=0x7fffffffdd10) at /home/simark/src/binutils-gdb/gdb/main.c:1257
#25 0x000055555bb70cf9 in main (argc=7, argv=0x7fffffffde88) at /home/simark/src/binutils-gdb/gdb/gdb.c:32
Catching the exception at this point means that the thread_db_info
object for this inferior will be left in place, despite the failure to
load libthread_db. This means that there won't be further attempts at
loading libthread_db, because thread_db_load will think that
libthread_db is already loaded for this inferior and will always exit
early. To fix this, add a try/catch around calling try_thread_db_load_1
in try_thread_db_load, such that if some exception is thrown while
trying to load libthread_db, we reset / delete the thread_db_info for
that inferior. That alone makes attach work fine again, because
check_for_thread_db is called again in the thread_db_inferior_created
observer (that happens after we learned about all shared libraries and
their symbols), and libthread_db is successfully loaded then.
When attaching, I think that the inferior_created observer is a good
place to try to load libthread_db: it is called once everything has
stabilized, when we learned about all shared libraries.
The only problem then is that when we first try (and fail) to load
libthread_db, in reaction to learning about libpthread, we show this
warning:
warning: Unable to find libthread_db matching inferior's thread library, thread debugging will not be available.
This is misleading, because we do succeed in loading it later. So when
attaching, I think we shouldn't try to load libthread_db in reaction to
the new_objfile events, we should wait until we have learned about all
shared libraries (using the inferior_created observable). To do so, add
an `in_initial_library_scan` flag to struct inferior. This flag is used
to postpone loading libthread_db if we are attaching or handling a fork
child.
When debugging remotely with GDBserver, the same problem happens, except
that the qSymbol mechanism (allowing the remote side to ask GDB for
symbols values) is involved. The fix there is the same idea, we make
GDB wait until all shared libraries and their symbols are known before
sending out a qSymbol packet. This way, we never present the remote
side a state where libpthread.so's symbols are known but ld-linux's
symbols aren't.
gdb/ChangeLog:
* inferior.h (class inferior) <in_initial_library_scan>: New.
* infcmd.c (post_create_inferior): Set in_initial_library_scan.
* infrun.c (follow_fork_inferior): Likewise.
* linux-thread-db.c (try_thread_db_load): Catch exception thrown
by try_thread_db_load_1
(thread_db_load): Return early if in_initial_library_scan is
set.
* remote.c (remote_new_objfile): Return early if
in_initial_library_scan is set.
Change-Id: I7a279836cfbb2b362b4fde11b196b4aab82f5efb
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
Same idea as previous patch, but for add_info_alias.
gdb/ChangeLog:
* command.h (add_info_alias): Accept target as
cmd_list_element. Update callers.
Change-Id: If830d423364bf42d7bea5ac4dd3a81adcfce6f7a
The alias creation functions currently accept a name to specify the
target command. They pass this to add_alias_cmd, which needs to lookup
the target command by name.
Given that:
- We don't support creating an alias for a command before that command
exists.
- We always use add_info_alias just after creating that target command,
and therefore have access to the target command's cmd_list_element.
... change add_com_alias to accept the target command as a
cmd_list_element (other functions are done in subsequent patches). This
ensures we don't create the alias before the target command, because you
need to get the cmd_list_element from somewhere when you call the alias
creation function. And it avoids an unecessary command lookup. So it
seems better to me in every aspect.
gdb/ChangeLog:
* command.h (add_com_alias): Accept target as
cmd_list_element. Update callers.
Change-Id: I24bed7da57221cc77606034de3023fedac015150
