| Age | Commit message (Collapse) | Author | Files | Lines |
|---|
|
I saw this failure on a CI:
(gdb) add-inferior
[New inferior 2]
Added inferior 2
(gdb) PASS: gdb.threads/vfork-multi-inferior.exp: method=non-stop: add-inferior
inferior 2
[Switching to inferior 2 [<null>] (<noexec>)]
(gdb) PASS: gdb.threads/vfork-multi-inferior.exp: method=non-stop: inferior 2
kill
The program is not being run.
(gdb) file /home/jenkins/workspace/binutils-gdb_master_linuxbuild/platform/jammy-amd64/target_board/unix/tmp/tmp.GYATAXR8Ku/gdb/testsuite/outputs/gdb.threads/vfork-multi-inferior/vfork-multi-inferior-sleep
Reading symbols from /home/jenkins/workspace/binutils-gdb_master_linuxbuild/platform/jammy-amd64/target_board/unix/tmp/tmp.GYATAXR8Ku/gdb/testsuite/outputs/gdb.threads/vfork-multi-inferior/vfork-multi-inferior-sleep...
(gdb) run &
Starting program: /home/jenkins/workspace/binutils-gdb_master_linuxbuild/platform/jammy-amd64/target_board/unix/tmp/tmp.GYATAXR8Ku/gdb/testsuite/outputs/gdb.threads/vfork-multi-inferior/vfork-multi-inferior-sleep
(gdb) PASS: gdb.threads/vfork-multi-inferior.exp: method=non-stop: run inferior 2
inferior 1
[Switching to inferior 1 [<null>] (<noexec>)]
(gdb) PASS: gdb.threads/vfork-multi-inferior.exp: method=non-stop: inferior 1
kill
The program is not being run.
(gdb) file /home/jenkins/workspace/binutils-gdb_master_linuxbuild/platform/jammy-amd64/target_board/unix/tmp/tmp.GYATAXR8Ku/gdb/testsuite/outputs/gdb.threads/vfork-multi-inferior/vfork-multi-inferior
Reading symbols from /home/jenkins/workspace/binutils-gdb_master_linuxbuild/platform/jammy-amd64/target_board/unix/tmp/tmp.GYATAXR8Ku/gdb/testsuite/outputs/gdb.threads/vfork-multi-inferior/vfork-multi-inferior...
(gdb) break should_break_here
Breakpoint 1 at 0x11b1: file /home/jenkins/workspace/binutils-gdb_master_linuxbuild/platform/jammy-amd64/target_board/unix/src/binutils-gdb/gdb/testsuite/gdb.threads/vfork-multi-inferior.c, line 25.
(gdb) PASS: gdb.threads/vfork-multi-inferior.exp: method=non-stop: break should_break_here
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1".
start
Temporary breakpoint 2 at 0x11c0: -qualified main. (2 locations)
Starting program: /home/jenkins/workspace/binutils-gdb_master_linuxbuild/platform/jammy-amd64/target_board/unix/tmp/tmp.GYATAXR8Ku/gdb/testsuite/outputs/gdb.threads/vfork-multi-inferior/vfork-multi-inferior
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1".
Thread 2.1 "vfork-multi-inf" hit Temporary breakpoint 2, main () at /home/jenkins/workspace/binutils-gdb_master_linuxbuild/platform/jammy-amd64/target_board/unix/src/binutils-gdb/gdb/testsuite/gdb.threads/vfork-multi-inferior-sleep.c:23
23 sleep (30);
(gdb) FAIL: gdb.threads/vfork-multi-inferior.exp: method=non-stop: start inferior 1
What happens is:
1. We start inferior 2 with "run&", it runs very slowly, takes time to
get to main
2. We switch to inferior 1, and run "start"
3. The temporary breakpoint inserted by "start" applies to all inferiors
4. Inferior 2 hits that breakpoint and GDB reports that hit
To avoid this, breakpoints inserted by "start" should be
inferior-specific. However, we don't have a nice way to make
inferior-specific breakpoints yet. It's possible to make
pspace-specific breakpoints (for example how the internal_breakpoint
constructor does) by creating a symtab_and_line manually. However,
inferiors can share program spaces (usually on particular embedded
targets), so we could have a situation where two inferiors run the same
code in the same program space. In that case, it would just not be
possible to insert a breakpoint in one inferior but not the other.
A simple solution that should work all the time is to add a condition to
the breakpoint inserted by "start", to check the inferior reporting the
hit is the expected one. This is what this patch implements.
Add a test that does:
- start in background inferior 1 that sleeps before reaching its main
function (using a sleep in a global C++ object's constructor)
- start inferior 2 with the "start" command, which also sleeps before
reaching its main function
- validate that we hit the breakpoint in inferior 2
Without the fix, we hit the breakpoint in inferior 1 pretty much all the
time. There could be some unfortunate scheduling causing the test not
to catch the bug, for instance if the scheduler decides not to schedule
inferior 1 for a long time, but it would be really rare. If the bug is
re-introduced, the test will catch it much more often than not, so it
will be noticed.
Reviewed-By: Bruno Larsen <blarsen@redhat.com>
Approved-By: Pedro Alves <pedro@palves.net>
Change-Id: Ib0148498a476bfa634ed62353c95f163623c686a
|
|
Currently, every internal_error call must be passed __FILE__/__LINE__
explicitly, like:
internal_error (__FILE__, __LINE__, "foo %d", var);
The need to pass in explicit __FILE__/__LINE__ is there probably
because the function predates widespread and portable variadic macros
availability. We can use variadic macros nowadays, and in fact, we
already use them in several places, including the related
gdb_assert_not_reached.
So this patch renames the internal_error function to something else,
and then reimplements internal_error as a variadic macro that expands
__FILE__/__LINE__ itself.
The result is that we now should call internal_error like so:
internal_error ("foo %d", var);
Likewise for internal_warning.
The patch adjusts all calls sites. 99% of the adjustments were done
with a perl/sed script.
The non-mechanical changes are in gdbsupport/errors.h,
gdbsupport/gdb_assert.h, and gdb/gdbarch.py.
Approved-By: Simon Marchi <simon.marchi@efficios.com>
Change-Id: Ia6f372c11550ca876829e8fd85048f4502bdcf06
|
|
Currently, despite having a smart pointer for frame_infos, GDB may
attempt to use an invalidated frame_info_ptr, which would cause internal
errors to happen. One such example has been documented as PR
python/28856, that happened when printing frame arguments calls an
inferior function.
To avoid failures, the smart wrapper was changed to also cache the frame
id, so the pointer can be reinflated later. For this to work, the
frame-id stuff had to be moved to their own .h file, which is included
by frame-info.h.
Frame_id caching is done explicitly using the prepare_reinflate method.
Caching is done manually so that only the pointers that need to be saved
will be, and reinflating has to be done manually using the reinflate
method because the get method and the -> operator must not change
the internals of the class. Finally, attempting to reinflate when the
pointer is being invalidated causes the following assertion errors:
check_ptrace_stopped_lwp_gone: assertion `lp->stopped` failed.
get_frame_pc: Assertion `frame->next != NULL` failed.
As for performance concerns, my personal testing with `time make
chec-perf GDB_PERFTEST_MODE=run` showed an actual reduction of around
10% of time running.
This commit also adds a testcase that exercises the python/28856 bug with
7 different triggers, run, continue, step, backtrace, finish, up and down.
Some of them can seem to be testing the same thing twice, but since this
test relies on stale pointers, there is always a chance that GDB got lucky
when testing, so better to test extra.
Regression tested on x86_64, using both gcc and clang.
Approved-by: Tom Tomey <tom@tromey.com>
|
|
This changes GDB to use frame_info_ptr instead of frame_info *
The substitution was done with multiple sequential `sed` commands:
sed 's/^struct frame_info;/class frame_info_ptr;/'
sed 's/struct frame_info \*/frame_info_ptr /g' - which left some
issues in a few files, that were manually fixed.
sed 's/\<frame_info \*/frame_info_ptr /g'
sed 's/frame_info_ptr $/frame_info_ptr/g' - used to remove whitespace
problems.
The changed files were then manually checked and some 'sed' changes
undone, some constructors and some gets were added, according to what
made sense, and what Tromey originally did
Co-Authored-By: Bruno Larsen <blarsen@redhat.com>
Approved-by: Tom Tomey <tom@tromey.com>
|
|
After the previous few commit, gdbarch_register_name no longer returns
nullptr. This commit audits all the calls to gdbarch_register_name
and removes any code that checks the result against nullptr.
There should be no visible change after this commit.
|
|
Remove the macro, replace all uses with calls to type::length.
Change-Id: Ib9bdc954576860b21190886534c99103d6a47afb
|
|
Remove the macro, replace all uses by calls to type::target_type.
Change-Id: Ie51d3e1e22f94130176d6abd723255282bb6d1ed
|
|
This changes the parameter of target_ops::async from int to bool.
Regression tested on x86-64 Fedora 34.
|
|
'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.
|
|
PR gdb/17160 points out that "interrupt -a" errors in all-stop mode,
but there's no good reason for this. This patch removes the error.
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=17160
|
|
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.
|
|
Replace with equivalent method.
Change-Id: I0e033095e7358799930775e61028b48246971a7d
|
|
Replace with equivalent methods.
Change-Id: I10a6c8a2a86462d9d4a6a6409a3f07a6bea66310
|
|
This turns symbol_objfile into a method on symbol.
|
|
Remove all macros related to getting and setting some symbol value:
#define SYMBOL_VALUE(symbol) (symbol)->value.ivalue
#define SYMBOL_VALUE_ADDRESS(symbol) \
#define SET_SYMBOL_VALUE_ADDRESS(symbol, new_value) \
#define SYMBOL_VALUE_BYTES(symbol) (symbol)->value.bytes
#define SYMBOL_VALUE_COMMON_BLOCK(symbol) (symbol)->value.common_block
#define SYMBOL_BLOCK_VALUE(symbol) (symbol)->value.block
#define SYMBOL_VALUE_CHAIN(symbol) (symbol)->value.chain
#define MSYMBOL_VALUE(symbol) (symbol)->value.ivalue
#define MSYMBOL_VALUE_RAW_ADDRESS(symbol) ((symbol)->value.address + 0)
#define MSYMBOL_VALUE_ADDRESS(objfile, symbol) \
#define BMSYMBOL_VALUE_ADDRESS(symbol) \
#define SET_MSYMBOL_VALUE_ADDRESS(symbol, new_value) \
#define MSYMBOL_VALUE_BYTES(symbol) (symbol)->value.bytes
#define MSYMBOL_BLOCK_VALUE(symbol) (symbol)->value.block
Replace them with equivalent methods on the appropriate objects.
Change-Id: Iafdab3b8eefc6dc2fd895aa955bf64fafc59ed50
|
|
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.
|
|
I found a few spots where filename styling ought to be applied, but is
not.
|
|
If the attach target supports async mode, enable it after the
attach target's ::attach method returns.
|
|
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
|
|
Change-Id: I83211d5a47efc0564386e5b5ea4a29c00b1fd46a
|
|
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.
|
|
non-stop
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_terminal.
Change-Id: If9367d5db8c976a4336680adca4ea5bc31ab64d2
|
|
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
|
