| Age | Commit message (Collapse) | Author | Files | Lines |
|---|
|
The patch to change say_where into a method introduced a bug. This
patch fixes it.
|
|
'say_where' is only useful (and only called for) code breakpoints, so
convert it to be a protected method on code_breakpoint.
|
|
This commit is the result of running the gdb/copyright.py script,
which automated the update of the copyright year range for all
source files managed by the GDB project to be updated to include
year 2023.
|
|
This changes bpstat to use 'bool' rather than 'char', and updates the
uses.
|
|
PR gdb/28947
The address_significant gdbarch setting was introduced as a way to remove
non-address bits from pointers, and it is specified by a constant. This
constant represents the number of address bits in a pointer.
Right now AArch64 is the only architecture that uses it, and 56 was a
correct option so far.
But if we are using Pointer Authentication (PAuth), we might use up to 2 bytes
from the address space to store the required information. We could also have
cases where we're using both PAuth and MTE.
We could adjust the constant to 48 to cover those cases, but this doesn't
cover the case where GDB needs to sign-extend kernel addresses after removal
of the non-address bits.
This has worked so far because bit 55 is used to select between kernel-space
and user-space addresses. But trying to clear a range of bits crossing the
bit 55 boundary requires the hook to be smarter.
The following patch renames the gdbarch hook from significant_addr_bit to
remove_non_address_bits and passes a pointer as opposed to the number of
bits. The hook is now responsible for removing the required non-address bits
and sign-extending the address if needed.
While at it, make GDB and GDBServer share some more code for aarch64 and add a
new arch-specific testcase gdb.arch/aarch64-non-address-bits.exp.
Bug-url: https://sourceware.org/bugzilla/show_bug.cgi?id=28947
Approved-By: Simon Marchi <simon.marchi@efficios.com>
|
|
[I sent this earlier today, but I don't see it in the archives.
Resending it through a different computer / SMTP.]
The use of the static buffer in command_line_input is becoming
problematic, as explained here [1]. In short, with this patch [2] that
attempt to fix a post-hook bug, when running gdb.base/commands.exp, we
hit a case where we read a "define" command line from a script file
using command_command_line_input. The command line is stored in
command_line_input's static buffer. Inside the define command's
execution, we read the lines inside the define using command_line_input,
which overwrites the define command, in command_line_input's static
buffer. After the execution of the define command, execute_command does
a command look up to see if a post-hook is registered. For that, it
uses a now stale pointer that used to point to the define command, in
the static buffer, causing a use-after-free. Note that the pointer in
execute_command points to the dynamically-allocated buffer help by the
static buffer in command_line_input, not to the static object itself,
hence why we see a use-after-free.
Fix that by removing the static buffer. I initially changed
command_line_input and other related functions to return an std::string,
which is the obvious but naive solution. The thing is that some callees
don't need to return an allocated string, so this this an unnecessary
pessimization. I changed it to passing in a reference to an std::string
buffer, which the callee can use if it needs to return
dynamically-allocated content. It fills the buffer and returns a
pointers to the C string inside. The callees that don't need to return
dynamically-allocated content simply don't use it.
So, it started with modifying command_line_input as described above, all
the other changes derive directly from that.
One slightly shady thing is in handle_line_of_input, where we now pass a
pointer to an std::string's internal buffer to readline's history_value
function, which takes a `char *`. I'm pretty sure that this function
does not modify the input string, because I was able to change it (with
enough massaging) to take a `const char *`.
A subtle change is that we now clear a UI's line buffer using a
SCOPE_EXIT in command_line_handler, after executing the command.
This was previously done by this line in handle_line_of_input:
/* We have a complete command line now. Prepare for the next
command, but leave ownership of memory to the buffer . */
cmd_line_buffer->used_size = 0;
I think the new way is clearer.
[1] https://inbox.sourceware.org/gdb-patches/becb8438-81ef-8ad8-cc42-fcbfaea8cddd@simark.ca/
[2] https://inbox.sourceware.org/gdb-patches/20221213112241.621889-1-jan.vrany@labware.com/
Change-Id: I8fc89b1c69870c7fc7ad9c1705724bd493596300
Reviewed-By: Tom Tromey <tom@tromey.com>
|
|
On powerpc64le-linux I ran into this FAIL:
...
(gdb) p exceptions.throw_function()^M
terminate called after throwing an instance of 'int'^M
^M
Program received signal SIGABRT, Aborted.^M
0x00007ffff7979838 in raise () from /lib64/libc.so.6^M
The program being debugged was signaled while in a function called from GDB.^M
GDB remains in the frame where the signal was received.^M
To change this behavior use "set unwindonsignal on".^M
Evaluation of the expression containing the function^M
(SimpleException::throw_function()) will be abandoned.^M
When the function is done executing, GDB will silently stop.^M
(gdb) FAIL: gdb.cp/gdb2495.exp: call a function that raises an exception \
without a handler.
...
The following happens:
- we start an inferior call
- an internal breakpoint is set on the global entry point of std::terminate
- the inferior call uses the local entry point
- the breakpoint is not triggered
- we run into std::terminate
We can fix this by simply adding the missing gdbarch_skip_entrypoint call in
create_std_terminate_master_breakpoint, but we try to do this a bit more
generic, by:
- adding a variant of function create_internal_breakpoint which takes a
minimal symbol instead of an address as argument
- in the new function:
- using both gdbarch_convert_from_func_ptr_addr and gdbarch_skip_entrypoint
- documenting why we don't need to use gdbarch_addr_bits_remove
- adding a note about possibly
needing gdbarch_deprecated_function_start_offset.
- using the new function in:
- create_std_terminate_master_breakpoint
- create_exception_master_breakpoint_hook, which currently uses only
gdbarch_convert_from_func_ptr_addr.
Note: we could use the new function in more locations in breakpoint.c, but
as we're not aware of any related failures, we declare this out of scope for
this patch.
Tested on x86_64-linux, powerpc64le-linux.
Co-Authored-By: Ulrich Weigand <uweigand@de.ibm.com>
Tested-by: Carl Love <cel@us.ibm.com>
PR tdep/29793
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=29793
|
|
The test py-objfile.exp unloads the current file while debugging the process.
This results in bpstat bs->b->loc to become nullptr.
Handle this case in breakpoint.c:bpstat_locno.
Note: GDB crashes on this problem with an internal error,
but the end of gdb summary shows:
...
=== gdb Summary ===
# of expected passes 36
The output also does not contain a 'FAIL:'.
After the fix, the nr of expected passes increased.
In the gdb.log output, one can see:
...
Fatal signal: Segmentation fault
----- Backtrace -----
0x55698905c5b9 gdb_internal_backtrace_1
../../binutils-gdb/gdb/bt-utils.c:122
0x55698905c5b9 _Z22gdb_internal_backtracev
...
ERROR: Couldn't send python print(objfile.filename) to GDB.
ERROR: : spawn id exp9 not open
while executing
"expect {
-i exp9 -timeout 10
-re ".*A problem internal to GDB has been detected" {
fail "$message (GDB internal error)"
gdb_internal_error..."
("uplevel" body line 1)
invoked from within
....
Wondering if it might be possible to improve gdb_test to have
gdb_test "python print(objfile.filename)" "None" \
"objfile.filename after objfile is unloaded"
reporting a failed result instead of just producing the internal error.
|
|
If the commands of the bpstat bs contain commands such as step or next or
continue, the BS and its commands are freed by execute_control_command.
So, we cannot remember the BS that was printed. Instead, remember
the bpnum and locno.
Regtested on debian/amd64 and re-run a few tests under valgrind.
|
|
$_hit_locno PR breakpoints/12464
This implements the request given in PR breakpoints/12464.
Before this patch, when a breakpoint that has multiple locations is reached,
GDB printed:
Thread 1 "zeoes" hit Breakpoint 1, some_func () at somefunc1.c:5
This patch changes the message so that bkpt_print_id prints the precise
encountered breakpoint:
Thread 1 "zeoes" hit Breakpoint 1.2, some_func () at somefunc1.c:5
In mi mode, bkpt_print_id also (optionally) prints a new table field "locno":
locno is printed when the breakpoint hit has more than one location.
Note that according to the GDB user manual node 'GDB/MI Development and Front
Ends', it is ok to add new fields without changing the MI version.
Also, when a breakpoint is reached, the convenience variables
$_hit_bpnum and $_hit_locno are set to the encountered breakpoint number
and location number.
$_hit_bpnum and $_hit_locno can a.o. be used in the command list of a
breakpoint, to disable the specific encountered breakpoint, e.g.
disable $_hit_bpnum.$_hit_locno
In case the breakpoint has only one location, $_hit_locno is set to
the value 1, so as to allow a command such as:
disable $_hit_bpnum.$_hit_locno
to disable the breakpoint even when the breakpoint has only one location.
This also fixes a strange behaviour: when a breakpoint X has only
one location,
enable|disable X.1
is accepted but transforms the breakpoint in a multiple locations
breakpoint having only one location.
The changes in RFA v4 handle the comments of Tom Tromey:
- Changed convenience var names from $bkptno/$locno to
$_hit_bpnum/$_hit_locno.
- updated the tests and user manual accordingly.
User manual also explictly describes that $_hit_locno is set to 1
for a breakpoint with a single location.
- The variable values are now set in bpstat_do_actions_1 so that
they are set for silent breakpoints, and when several breakpoints
are hit at the same time, that the variables are set to the printed
breakpoint.
The changes in RFA v3 handle the additional comments of Eli:
GDB/NEW:
- Use max 80-column
- Use 'code location' instead of 'location'.
- Fix typo $bkpno
- Ensure that disable $bkptno and disable $bkptno.$locno have
each their explanation inthe example
- Reworded the 'breakpoint-hit' paragraph.
gdb.texinfo:
- Use 'code location' instead of 'location'.
- Add a note to clarify the distinction between $bkptno and $bpnum.
- Use @kbd instead of examples with only one command.
Compared to RFA v1, the changes in v2 handle the comments given by
Keith Seitz and Eli Zaretskii:
- Use %s for the result of paddress
- Use bkptno_numopt_re instead of 2 different -re cases
- use C@t{++}
- Add index entries for $bkptno and $locno
- Added an example for "locno" in the mi interface
- Added examples in the Break command manual.
|
|
watchpoints
Currently the resource checking is done if REPARSE is true. The hardware
watchpoint resource checking in update_watchpoint needs to be redone on
each call to function update_watchpoints as the value chain may have
changed. The number of hardware registers needed for a watchpoint can
change if the variable being watched changes. This situation occurs in
this test when watching variable **global_ptr_ptr. Initially when the
watch command is issued, only two addresses need to be watched as
**global_ptr_ptr has not yet been initialized. Once the value of
**global_ptr_ptr is initialized the locations to be tracked increase to
three addresses. However, update_watchpoints is not called again with
REPARSE set to 1 to force the resource checking to be redone. When the
test is run on Power 10, an internal gdb error occurs when the PowerPC
routine tries to setup the three hardware watchpoint address since the hw
only has two hardware watchpoint registers. The error occurs because the
resource checking was not redone in update_watchpoints after
**global_ptr_ptr changed.
The following descibes the situation in detail that occurs on Power 10 with
gdb running on the binary for gdb.base/watchpoint.c.
1 break func4
2 run
3 watch *global_ptr
4 next execute source code: buf[0] = 3;
5 next execute source code: global_ptr = buf;
6 next execute source code: buf[0] = 7;
7 delete 2 (delete watch *global_ptr)
8 watch **global_ptr_ptr
9 next execute source code: buf[1] = 5;
10 next global_ptr_ptr = &global_ptr;
11 next buf[0] = 9;
In step 8, the the watch **global_ptr_prt command calls update_watchpoint
in breakpoint.c with REPARSE set to 1. The function update_watchpoint
calls can_use_hardware_watchpoint to see if there are enough
resources available to add the watchpoint since REPARSE is set to 1. At
this point, **global_ptr_ptr has not been initialized so only two addresses
are watched. The val_chain contains the address for **global_ptr_ptr and 0
since **global_ptr_ptr has not been initialized. The update_watchpoint
updates the breakpoint list as follows:
breakpoint 0
loc 0: b->address = 0x100009c0
breakpoint 1
loc 1: b->address = 0x7ffff7f838a0
breakpoint 2
loc 2: b->address = 0x7ffff7b7fc54
breakpoint 3
loc 3: b->address = 0x7ffff7a5788c
breakpoint 4
loc 4: b->address = 0x0 <-- location pointed to by global_ptr_ptr
loc 5: b->address = 0x100200b8 <-- global_ptr_ptr watchpoint
breakpoint 5
loc 6: b->address = 0x7ffff7b7fc54
In step 10, the next command executes the source code
global_ptr_ptr = &global_ptr. This changes the set of locations to be
watched for the watchpoint **global_ptr_prt. The list of addresses for the
breakpoint consist of the address for global_ptr_prt, global_ptr and buf.
The breakpoint list gets updated by update_watchpoint as follows:
breakpoint 0
loc 0: b->address = 0x100009c0
breakpoint 1
loc 1: b->address = 0x7ffff7f838a0
breakpoint 2
loc 2: b->address = 0x7ffff7b7fc54
breakpoint 3
loc 3: b->address = 0x7ffff7a5788c
breakpoint 4
loc 4: b->address = 0x10020050 buf
loc 5: b->address = 0x100200b0 watch *global_ptr
loc 6: b->address = 0x100200b8 watch **global_ptr_ptr
breakpoint 5
loc 7: b->address = 0x7ffff7b7fc54
breakpoint 6
However, the hardware resource checking was not redone because
update_breakpoint was called with REPARSE equal to 0.
Step 11, execute the third next command. The function
ppc_linux_nat_target::low_prepare_to_resume() attempts a ptrace
call to setup each of the three address for breakpoint 4. The slot
value returned for the third ptrace call is -1 indicating an error
because there are only two hardware watchpoint registers available on
Power 10.
This patch removes just the statement "if (reparse)" in function
update_watchpoint to force the resources to be rechecked on every call to
the function. This ensures that any changes to the val_chain resulting
in needing more resources then available will be caught.
The patch has been tested on Power 8, Power 10 and X86-64. Note the patch
has no effect on Power 9 since hardware watchpoint support is disabled on
that processor.
|
|
Recent commit b2829fcf9b5 ("[gdb] Fix rethrow exception slicing in
insert_bp_location") introduced macro RETHROW_ON_TARGET_CLOSE_ERROR.
I wrote this as a macro in order to have the rethrowing throw be part of the
same function as the catch, but as it turns out that's not necessary.
Rewrite into a function.
Tested on x86_64-linux.
|
|
The preferred way of rethrowing an exception is by using throw without
expression, because it avoids object slicing of the exception [1].
Fix this in insert_bp_location.
Tested on x86_64-linux.
[1] https://en.cppreference.com/w/cpp/language/throw
Approved-By: Andrew Burgess <aburgess@redhat.com>
|
|
Some int to bool conversion in breakpoint.c. I've only updated the
function signatures of static functions, but I've updated some
function local variables throughout the file.
There should be no user visible changes after this commit.
Approved-By: Simon Marchi <simon.marchi@efficios.com>
|
|
I noticed that we could make use of a scoped_restore in the function
unduplicated_should_be_inserted. I've also converted the function
return type from int to bool.
This change shouldn't make any difference, as I don't think anything
within should_be_inserted could throw an exception, but the change
doesn't hurt, and will help keep us safe if anything ever changes in
the future.
Approved-By: Simon Marchi <simon.marchi@efficios.com>
|
|
I was doing some int to bool cleanup in update_watchpoint, and I
noticed a manual version of scoped_restore_selected_frame. As always
when these things are done manually, there is the chance that, in an
error case, we might leave the wrong frame selected.
This commit updates things to use scoped_restore_selected_frame, and
also converts a local variable from int to bool.
The only user visible change after this commit is in the case where
update_watchpoint throws an error - we should now correctly restore
the previously selected frame. Otherwise, this commit should be
invisible to the user.
Approved-By: Simon Marchi <simon.marchi@efficios.com>
|
|
I spotted a few places where I could make some 'bp_location *'
arguments constant in breakpoint.c.
There should be no user visible changes after this commit.
Approved-By: Simon Marchi <simon.marchi@efficios.com>
|
|
With the cc-with-tweaks.sh patch submitted here (
https://sourceware.org/pipermail/gdb-patches/2022-October/192586.html ) we run
with:
...
$ export STRIP_ARGS_STRIP_DEBUG=--strip-all
$ make check RUNTESTFLAGS="gdb.base/jit-reader.exp \
--target_board cc-with-gnu-debuglink"
...
into the following assert:
...
(gdb) run ^M
Starting program: jit-reader ^M
gdb/jit.c:1247: internal-error: jit_event_handler: \
Assertion `jiter->jiter_data != nullptr' failed.^M
...
Fix this by handling the
jit_bp_sym.objfile->separate_debug_objfile_backlink != nullptr case in
handle_jit_event.
Tested on x86_64-linux.
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=29277
|
|
Currently, every internal_error call must be passed __FILE__/__LINE__
explicitly, like:
internal_error (__FILE__, __LINE__, "foo %d", var);
The need to pass in explicit __FILE__/__LINE__ is there probably
because the function predates widespread and portable variadic macros
availability. We can use variadic macros nowadays, and in fact, we
already use them in several places, including the related
gdb_assert_not_reached.
So this patch renames the internal_error function to something else,
and then reimplements internal_error as a variadic macro that expands
__FILE__/__LINE__ itself.
The result is that we now should call internal_error like so:
internal_error ("foo %d", var);
Likewise for internal_warning.
The patch adjusts all calls sites. 99% of the adjustments were done
with a perl/sed script.
The non-mechanical changes are in gdbsupport/errors.h,
gdbsupport/gdb_assert.h, and gdb/gdbarch.py.
Approved-By: Simon Marchi <simon.marchi@efficios.com>
Change-Id: Ia6f372c11550ca876829e8fd85048f4502bdcf06
|
|
I looked through all the uses of static_cast<... *> in gdb and
converted many of them to checked_static_cast.
I couldn't test a few of these changes.
|
|
I looked at all the spots using value_mark, and converted all the
straightforward ones to use scoped_value_mark instead.
Regression tested on x86-64 Fedora 34.
|
|
This changes GDB to use frame_info_ptr instead of frame_info *
The substitution was done with multiple sequential `sed` commands:
sed 's/^struct frame_info;/class frame_info_ptr;/'
sed 's/struct frame_info \*/frame_info_ptr /g' - which left some
issues in a few files, that were manually fixed.
sed 's/\<frame_info \*/frame_info_ptr /g'
sed 's/frame_info_ptr $/frame_info_ptr/g' - used to remove whitespace
problems.
The changed files were then manually checked and some 'sed' changes
undone, some constructors and some gets were added, according to what
made sense, and what Tromey originally did
Co-Authored-By: Bruno Larsen <blarsen@redhat.com>
Approved-by: Tom Tomey <tom@tromey.com>
|
|
This replaces frame_id_eq with operator== and operator!=. I wrote
this for a version of this series that I later abandoned; but since it
simplifies the code, I left this patch in.
Approved-by: Tom Tomey <tom@tromey.com>
|
|
Add a couple of missing nullptr checks in the function
bpstat_check_breakpoint_conditions.
No user visible change after this commit.
|
|
This commit adds additional infrun debug from the breakpoint.c file.
The new debug output all relates to breakpoint condition evaluation.
There is already some infrun debug emitted from the breakpoint.c file,
so hopefully, adding more will not be contentious. I think the
functions being instrumented make sense as part of the infrun process,
the inferior stops, evaluates the condition, and then either stops or
continues. This new debug gives more insight into that process.
I had to make the bp_location* argument to find_loc_num_by_location
const, and add a declaration for find_loc_num_by_location.
There should be no user visible changes unless they turn on debug
output.
|
|
This removes decode_location_spec_default, inlining it into its sole
caller.
Regression tested on x86-64 Fedora 34.
|
|
Remove the macro, replace all uses with calls to type::length.
Change-Id: Ib9bdc954576860b21190886534c99103d6a47afb
|
|
This changes ui_out_redirect_pop to also perform the redirection, and
then updates several sites to use this, rather than explicit
redirects.
|
|
The print_one_detail_ranged_breakpoint has been renamed to
ranged_breakpoint::print_one_detail in this commit:
commit ec45bb676c9c69c30783bcf35ffdac8280f3b8bc
Date: Sat Jan 15 16:34:51 2022 -0700
Convert ranged breakpoints to vtable ops
So their comments should be updated as well.
|
|
Change from int to target_waitkind, which is really what is is. While
at it, remove some outdated doc. The return value is described by a
relatively self-describing enum, not a numerical value like the doc
says.
Change-Id: Id899c853a857c7891c45e5b1639024067d5b59cd
|
|
breakpoint::decode_location_spec just asserts if called. It turned
out to be relatively easy to remove this method from breakpoint and
instead move the base implementation to code_breakpoint.
|
|
location_spec_to_sals is only ever called for code breakpoints, so
make it a protected method there.
|
|
breakpoint_re_set_default is only ever called from breakpoint re_set
methods, so make it a protected method on code_breakpoint.
|
|
The "script" field, output whenever information about a breakpoint with
commands is output, uses wrong MI syntax.
$ ./gdb -nx -q --data-directory=data-directory -x script -i mi
=thread-group-added,id="i1"
=breakpoint-created,bkpt={number="1",type="breakpoint",disp="keep",enabled="y",addr="0x000000000000111d",func="main",file="test.c",fullname="/home/simark/build/binutils-gdb-one-target/gdb/test.c",line="3",thread-groups=["i1"],times="0",original-location="main"}
=breakpoint-modified,bkpt={number="1",type="breakpoint",disp="keep",enabled="y",addr="0x000000000000111d",func="main",file="test.c",fullname="/home/simark/build/binutils-gdb-one-target/gdb/test.c",line="3",thread-groups=["i1"],times="0",script={"aaa","bbb","ccc"},original-location="main"}
(gdb)
-break-info
^done,BreakpointTable={nr_rows="1",nr_cols="6",hdr=[{width="7",alignment="-1",col_name="number",colhdr="Num"},{width="14",alignment="-1",col_name="type",colhdr="Type"},{width="4",alignment="-1",col_name="disp",colhdr="Disp"},{width="3",alignment="-1",col_name="enabled",colhdr="Enb"},{width="18",alignment="-1",col_name="addr",colhdr="Address"},{width="40",alignment="2",col_name="what",colhdr="What"}],body=[bkpt={number="1",type="breakpoint",disp="keep",enabled="y",addr="0x000000000000111d",func="main",file="test.c",fullname="/home/simark/build/binutils-gdb-one-target/gdb/test.c",line="3",thread-groups=["i1"],times="0",script={"aaa","bbb","ccc"},original-location="main"}]}
(gdb)
In both the =breakpoint-modified and -break-info output, we have:
script={"aaa","bbb","ccc"}
According to the output syntax [1], curly braces means tuple, and a
tuple contains key=value pairs. This looks like it should be a list,
but uses curly braces by mistake. This would make more sense:
script=["aaa","bbb","ccc"]
Fix it, keeping the backwards compatibility by introducing a new MI
version (MI4), in exactly the same way as was done when fixing
multi-locations breakpoint output in [2].
- Add a fix_breakpoint_script_output uiout flag. MI uiouts will use
this flag if the version is >= 4.
- Add a fix_breakpoint_script_output_globally variable and the
-fix-breakpoint-script-output MI command to set it, if frontends want
to use the fixed output for this without using the newer MI version.
- When emitting the script field, use list instead of tuple, if we want
the fixed output (depending on the two criteria above)
-
[1] https://sourceware.org/gdb/onlinedocs/gdb/GDB_002fMI-Output-Syntax.html#GDB_002fMI-Output-Syntax
[2] https://gitlab.com/gnutools/binutils-gdb/-/commit/b4be1b0648608a2578bbed39841c8ee411773edd
Change-Id: I7113c6892832c8d6805badb06ce42496677e2242
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=24285
|
|
This rewrites registry.h, removing all the macros and replacing it
with relatively ordinary template classes. The result is less code
than the previous setup. It replaces large macros with a relatively
straightforward C++ class, and now manages its own cleanup.
The existing type-safe "key" class is replaced with the equivalent
template class. This approach ended up requiring relatively few
changes to the users of the registry code in gdb -- code using the key
system just required a small change to the key's declaration.
All existing users of the old C-like API are now converted to use the
type-safe API. This mostly involved changing explicit deletion
functions to be an operator() in a deleter class.
The old "save/free" two-phase process is removed, and replaced with a
single "free" phase. No existing code used both phases.
The old "free" callbacks took a parameter for the enclosing container
object. However, this wasn't truly needed and is removed here as
well.
|
|
PR python/18385
v7:
This version addresses the issues pointed out by Tom.
Added nullchecks for Python object creations.
Changed from using PyLong_FromLong to the gdb_py-versions.
Re-factored some code to make it look more cohesive.
Also added the more safe Python reference count decrement PY_XDECREF,
even though the BreakpointLocation type is never instantiated by the
user (explicitly documented in the docs) decrementing < 0 is made
impossible with the safe call.
Tom pointed out that using the policy class explicitly to decrement a
reference counted object was not the way to go, so this has instead been
wrapped in a ref_ptr that handles that for us in blocpy_dealloc.
Moved macro from py-internal to py-breakpoint.c.
Renamed section at the bottom of commit message "Patch Description".
v6:
This version addresses the points Pedro gave in review to this patch.
Added the attributes `function`, `fullname` and `thread_groups`
as per request by Pedro with the argument that it more resembles the
output of the MI-command "-break-list". Added documentation for these attributes.
Cleaned up left overs from copy+paste in test suite, removed hard coding
of line numbers where possible.
Refactored some code to use more c++-y style range for loops
wrt to breakpoint locations.
Changed terminology, naming was very inconsistent. Used a variety of "parent",
"owner". Now "owner" is the only term used, and the field in the
gdb_breakpoint_location_object now also called "owner".
v5:
Changes in response to review by Tom Tromey:
- Replaced manual INCREF/DECREF calls with
gdbpy_ref ptrs in places where possible.
- Fixed non-gdb style conforming formatting
- Get parent of bploc increases ref count of parent.
- moved bploc Python definition to py-breakpoint.c
The INCREF of self in bppy_get_locations is due
to the individual locations holding a reference to
it's owner. This is decremented at de-alloc time.
The reason why this needs to be here is, if the user writes
for instance;
py loc = gdb.breakpoints()[X].locations[Y]
The breakpoint owner object is immediately going
out of scope (GC'd/dealloced), and the location
object requires it to be alive for as long as it is alive.
Thanks for your review, Tom!
v4:
Fixed remaining doc issues as per request
by Eli.
v3:
Rewritten commit message, shortened + reworded,
added tests.
Patch Description
Currently, the Python API lacks the ability to
query breakpoints for their installed locations,
and subsequently, can't query any information about them, or
enable/disable individual locations.
This patch solves this by adding Python type gdb.BreakpointLocation.
The type is never instantiated by the user of the Python API directly,
but is produced by the gdb.Breakpoint.locations attribute returning
a list of gdb.BreakpointLocation.
gdb.Breakpoint.locations:
The attribute for retrieving the currently installed breakpoint
locations for gdb.Breakpoint. Matches behavior of
the "info breakpoints" command in that it only
returns the last known or currently inserted breakpoint locations.
BreakpointLocation contains 7 attributes
6 read-only attributes:
owner: location owner's Python companion object
source: file path and line number tuple: (string, long) / None
address: installed address of the location
function: function name where location was set
fullname: fullname where location was set
thread_groups: thread groups (inferiors) where location was set.
1 writeable attribute:
enabled: get/set enable/disable this location (bool)
Access/calls to these, can all throw Python exceptions (documented in
the online documentation), and that's due to the nature
of how breakpoint locations can be invalidated
"behind the scenes", either by them being removed
from the original breakpoint or changed,
like for instance when a new symbol file is loaded, at
which point all breakpoint locations are re-created by GDB.
Therefore this patch has chosen to be non-intrusive:
it's up to the Python user to re-request the locations if
they become invalid.
Also there's event handlers that handle new object files etc, if a Python
user is storing breakpoint locations in some larger state they've
built up, refreshing the locations is easy and it only comes
with runtime overhead when the Python user wants to use them.
gdb.BreakpointLocation Python type
struct "gdbpy_breakpoint_location_object" is found in python-internal.h
Its definition, layout, methods and functions
are found in the same file as gdb.Breakpoint (py-breakpoint.c)
1 change was also made to breakpoint.h/c to make it possible
to enable and disable a bp_location* specifically,
without having its LOC_NUM, as this number
also can change arbitrarily behind the scenes.
Updated docs & news file as per request.
Testsuite: tests the .source attribute and the disabling of
individual locations.
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=18385
Change-Id: I302c1c50a557ad59d5d18c88ca19014731d736b0
|
|
The three targets that implement gdbarch_adjust_breakpoint_address are
arm, frv, and mips. In each of these targets the adjust breakpoint
address function does some combination of reading the symbol table, or
reading memory at the location the breakpoint could be placed.
The problem is that performing these actions requires that the current
inferior and program space be the one in which the breakpoint will be
placed, and this is not currently always the case.
Consider a GDB session with multiple inferiors. One inferior might be
a native target while another could be a remote target of a completely
different architecture. Alternatively, if we consider ARM and
AArch64, one native inferior might be AArch64, while a second native
inferior could be ARM.
In these cases it is possible, and valid, for a user to have one
inferior selected, and place a breakpoint in the other inferior by
placing a breakpoint on a particular symbol.
If this happens, then currently, when
gdbarch_adjust_breakpoint_address is called, the wrong inferior (and
program space) will be selected, and memory reads, and symbol look
ups, will not return the expected results, this could lead to
breakpoints being placed in the wrong location.
There are currently two places where gdbarch_adjust_breakpoint_address
is called:
1. In infrun.c, in the function handle_step_into_function. In this
case, I believe that the correct inferior and program space will
already be selected as this is called as part of the stop event
handling, so I don't think we need to worry about this case, and
2. In breakpoint.c, in the function adjust_breakpoint_address, which
is itself called from code_breakpoint::add_location and
watch_command_1.
The watch_command_1 case I don't think we need to worry about, this
is for when a local watch expression is created, which can only be
in the currently selected inferior, so this case should be fine.
The code_breakpoint::add_location case is the one that needs fixing,
this is what allows a breakpoint to be created between inferiors.
To fix the code_breakpoint::add_location case, I propose that we pass
the "correct" program_space (i.e. the program space in which the
breakpoint will be created) to the adjust_breakpoint_address function.
Then in adjust_breakpoint_address we can make use of
switch_to_program_space_and_thread to switch program_space and
inferior before calling gdbarch_adjust_breakpoint_address.
I discovered this issue while working on a later patch in this
series. This later patch will detect when we cast the result of
gdbarch_tdep to the wrong type.
With this later patch in place I ran gdb.multi/multi-arch.exp on an
AArch64 target. In this situation, two inferiors are created, an
AArch64 inferior, and an ARM inferior. The test selected the AArch64
inferior and tries to create a breakpoint in the ARM inferior.
As a result of this we end up in arm_adjust_breakpoint_address, which
calls arm_pc_is_thumb. Before this commit the AArch64 inferior would
be current. As a result, all of the checks in arm_pc_is_thumb would
fail (they rely on reading symbols from the current program space),
and so, at the end of arm_pc_is_thumb we would call
arm_frame_is_thumb. However, remember, at this point the current
inferior is the AArch64 inferior, so the current frame is an AArch64
frame.
In arm_frame_is_thumb we call arm_psr_thumb_bit, which calls
gdbarch_tdep and casts the result to arm_gdbarch_tdep. This is wrong,
the tdep field is of type aarch64_gdbarch_tdep. After this we have
undefined behaviour.
With this patch in place, we will have switched to a thread in the ARM
program space before calling arm_adjust_breakpoint_address. As a
result, we now succeed in looking up the required symbols in
arm_pc_is_thumb, and so we never call arm_frame_is_thumb.
However, in the worst case scenario, if we did end up calling
arm_frame_is_thumb, as the current inferior should now be the ARM
inferior, the current frame should be an ARM frame, so we still should
not hit undefined behaviour.
I have added an assert to arm_frame_is_thumb.
|
|
With a test like this:
1 #include <dlfcn.h>
2 int
3 main ()
4 {
5 dlsym (RTLD_DEFAULT, "FOO");
6 return 0;
7 }
and then "start" followed by "until 6", GDB currently incorrectly
stops inside the runtime loader, instead of line 6. Vis:
...
Temporary breakpoint 1, main () at until.c:5
4 {
(gdb) until 6
0x00007ffff7f0a90d in __GI__dl_catch_exception (exception=exception@entry=0x7fffffffdb00, operate=<optimized out>, args=0x7ffff7f0a90d <__GI__dl_catch_exception+109>) at dl-error-skeleton.c:206
206 dl-error-skeleton.c: No such file or directory.
(gdb)
The problem is related to longjmp handling -- dlsym internally
longjmps on error. The testcase can be reduced to this:
1 #include <setjmp.h>
2 void func () {
3 jmp_buf buf;
4 if (setjmp (buf) == 0)
5 longjmp (buf, 1);
6 }
7
8 int main () {
9 func ();
10 return 0; /* until to here */
11 }
and then with "start" followed by "until 10", GDB currently
incorrectly stops at line 4 (returning from setjmp), instead of line
10.
The problem is that the BPSTAT_WHAT_CLEAR_LONGJMP_RESUME code in
infrun.c fails to find the initiating frame, and so infrun thinks that
the longjmp jumped somewhere outer to "until"'s originating frame.
Here:
case BPSTAT_WHAT_CLEAR_LONGJMP_RESUME:
{
struct frame_info *init_frame;
/* There are several cases to consider.
1. The initiating frame no longer exists. In this case we
must stop, because the exception or longjmp has gone too
far.
...
init_frame = frame_find_by_id (ecs->event_thread->initiating_frame);
if (init_frame) // this is NULL!
{
...
}
/* For Cases 1 and 2, remove the step-resume breakpoint, if it
exists. */
delete_step_resume_breakpoint (ecs->event_thread);
end_stepping_range (ecs); // case 1., so we stop.
}
The initiating frame is set by until_break_command ->
set_longjmp_breakpoint. The initiating frame is supposed to be the
frame that is selected when the command was issued, but
until_break_command instead passes the frame id of the _caller_ frame
by mistake. When the "until LINE" command is issued from main, the
caller frame is the caller of main. When later infrun tries to find
that frame by id, it fails to find it, because frame_find_by_id
doesn't unwind past main.
The bug is that we passed the caller frame's id to
set_longjmp_breakpoint. We should have passed the selected frame's id
instead.
Change-Id: Iaae1af7cdddf296b7c5af82c3b5b7d9b66755b1c
|
|
This converts location_spec_to_string to a method of location_spec,
simplifying the code using it, as it no longer has to use
std::unique_ptr::get().
Change-Id: I621bdad8ea084470a2724163f614578caf8f2dd5
|
|
This converts location_spec_type to location_spec::type().
Change-Id: Iff4cbfafb1cf3d22adfa142ff939b4a148e52273
|
|
This converts location_spec_empty_p to a method of location_spec,
simplifying users, as they no longer have to use
std::unique_ptr::get().
Change-Id: I83381a729896f12e1c5a1b4d6d4c2eb1eb6582ff
|
|
copy_location_spec is just a wrapper around location_spec::clone(), so
remove it and call clone() directly. This simplifies users, as they
no longer have to use std::unique_ptr::get().
Change-Id: I8ce8658589460b98888283b306b315a5b8f73976
|
|
Currently, there's the location_spec hierarchy, and then some
location_spec subclasses have their own struct type holding all their
data fields.
I.e., there is this:
location_spec
explicit_location_spec
linespec_location_spec
address_location_spec
probe_location_spec
and then these separate types:
explicit_location
linespec_location
where:
explicit_location_spec
has-a explicit_location
linespec_location_spec
has-a linespec_location
This patch eliminates explicit_location and linespec_location,
inlining their members in the corresponding location_spec type.
The location_spec subclasses were the ones currently defined in
location.c, so they are moved to the header. Since the definitions of
the classes are now visible, we no longer need location_spec_deleter.
Some constructors that are used for cloning location_specs, like:
explicit explicit_location_spec (const struct explicit_location *loc)
... were converted to proper copy ctors.
In the process, initialize_explicit_location is eliminated, and some
functions that returned the "data type behind a locspec", like
get_linespec_location are converted to downcast functions, like
as_linespec_location_spec.
Change-Id: Ia31ccef9382b25a52b00fa878c8df9b8cf2a6c5a
|
|
Currently, GDB internally uses the term "location" for both the
location specification the user input (linespec, explicit location, or
an address location), and for actual resolved locations, like the
breakpoint locations, or the result of decoding a location spec to
SaLs. This is expecially confusing in the breakpoints module, as
struct breakpoint has these two fields:
breakpoint::location;
breakpoint::loc;
"location" is the location spec, and "loc" is the resolved locations.
And then, we have a method called "locations()", which returns the
resolved locations as range...
The location spec type is presently called event_location:
/* Location we used to set the breakpoint. */
event_location_up location;
and it is described like this:
/* The base class for all an event locations used to set a stop event
in the inferior. */
struct event_location
{
and even that is incorrect... Location specs are used for finding
actual locations in the program in scenarios that have nothing to do
with stop events. E.g., "list" works with location specs.
To clean all this confusion up, this patch renames "event_location" to
"location_spec" throughout, and then all the variables that hold a
location spec, they are renamed to include "spec" in their name, like
e.g., "location" -> "locspec". Similarly, functions that work with
location specs, and currently have just "location" in their name are
renamed to include "spec" in their name too.
Change-Id: I5814124798aa2b2003e79496e78f95c74e5eddca
|
|
Currently, breakpoint locations that are enabled while their parent
breakpoint is disabled are displayed with "y" in the Enb colum of
"info breakpoints":
(gdb) info breakpoints
Num Type Disp Enb Address What
1 breakpoint keep n <MULTIPLE>
1.1 y 0x00000000000011b6 in ...
1.2 y 0x00000000000011c2 in ...
1.3 n 0x00000000000011ce in ...
Such locations won't trigger a break, so to avoid confusion, show "y-"
instead. For example:
(gdb) info breakpoints
Num Type Disp Enb Address What
1 breakpoint keep n <MULTIPLE>
1.1 y- 0x00000000000011b6 in ...
1.2 y- 0x00000000000011c2 in ...
1.3 n 0x00000000000011ce in ...
The "-" sign is inspired on how the TUI represents breakpoints on the
left side of the source window, with "b-" for a disabled breakpoint.
Change-Id: I9952313743c51bf21b4b380c72360ef7d4396a09
|
|
Even after the previous patches reworking the inheritance of several
breakpoint types, the present breakpoint hierarchy looks a bit
surprising, as we have "breakpoint" as the superclass, and then
"base_breakpoint" inherits from "breakpoint". Like so, simplified:
breakpoint
base_breakpoint
ordinary_breakpoint
internal_breakpoint
momentary_breakpoint
ada_catchpoint
exception_catchpoint
tracepoint
watchpoint
catchpoint
exec_catchpoint
...
The surprising part to me is having "base_breakpoint" being a subclass
of "breakpoint". I'm just refering to naming here -- I mean, you'd
expect that it would be the top level baseclass that would be called
"base".
Just flipping the names of breakpoint and base_breakpoint around
wouldn't be super great for us, IMO, given we think of every type of
*point as a breakpoint at the user visible level. E.g., "info
breakpoints" shows watchpoints, tracepoints, etc. So it makes to call
the top level class breakpoint.
Instead, I propose renaming base_breakpoint to code_breakpoint. The
previous patches made sure that all code breakpoints inherit from
base_breakpoint, so it's fitting. Also, "code breakpoint" contrasts
nicely with a watchpoint also being typically known as a "data
breakpoint".
After this commit, the resulting hierarchy looks like:
breakpoint
code_breakpoint
ordinary_breakpoint
internal_breakpoint
momentary_breakpoint
ada_catchpoint
exception_catchpoint
tracepoint
watchpoint
catchpoint
exec_catchpoint
...
... which makes a lot more sense to me.
I've left this patch as last in the series in case people want to
bikeshed on the naming.
"code" has a nice property that it's exactly as many letters as
"base", so this patch didn't require any reindentation. :-)
Change-Id: Id8dc06683a69fad80d88e674f65e826d6a4e3f66
|
|
This adds a new ctor to momentary_breakpoints with a few parameters
that are always necessary for momentary breakpoints.
In particular, I noticed that set_std_terminate_breakpoint doesn't
make the breakpoint be thread specific, which looks like a bug to me.
The point of that breakpoint is to intercept std::terminate calls that
happen as result of the called thread throwing an exception that won't
be caught by the dummy frame. If some other thread calls
std::terminate, IMO, it's no different from some other thread calling
exit/_exit, for example.
Change-Id: Ifc5ff4a6d6e58b8c4854d00b86725382d38a1a02
|
|
Momentary breakpoints have no breakpoint number, their breakpoint
number should be always 0, to avoid constantly incrementing (or
decrementing) the internal breakpoint count.
Indeed, set_momentary_breakpoint installs the created breakpoint
without a number.
However, momentary_breakpoint_from_master incorrectly gives an
internal breakpoint number to the new breakpoint. This commit fixes
that.
Change-Id: Iedcae5432cdf232db9e9a6e1a646d358abd34f95
|
|
This tweaks the intro comments of the following classes:
internal_breakpoint
momentary_breakpoint
breakpoint
base_breakpoint
watchpoint
catchpoint
Change-Id: If6b31f51ebbb81705fbe5b8435f60ab2c88a98c8
|
|
After the previous patches, only base_breakpoint subclasses use
add_location(sal), so we can move it to base_breakpoint (a.k.a. base
class for code breakpoints).
This requires a few casts here and there, but always at spots where
you can see from context what the breakpoint's type actually is.
I inlined new_single_step_breakpoint into its only caller exactly for
this reason.
I did try to propagate more use of base_breakpoint to avoid casts, but
that turned out unwieldy for this patch.
Change-Id: I49d959322b0fdce5a88a216bb44730fc5dd7c6f8
|
