| Age | Commit message (Collapse) | Author | Files | Lines |
|---|
|
Test-case gdb.dwarf2/forward-spec.exp contains a non-trivial gdb_test_multiple
to parse this cooked_index_entry:
...
[5] ((cooked_index_entry *) 0x7f01f0004040)^M
name: v^M
canonical: v^M
qualified: ns::v^M
DWARF tag: DW_TAG_variable^M
flags: 0x2 [IS_STATIC]^M
DIE offset: 0xcb^M
parent: ((cooked_index_entry *) 0x7f01f00040a0) [ns]^M
...
which allows us to verify that the entry has a parent.
After commit 8f258a6c979 ("[gdb/symtab] Dump qualified name of
cooked_index_entry") that's no longer necessary.
Simplify this by checking for ns::v instead.
While we're at it, also fix the test-case for target boards readnow,
cc-with-gdb-index and cc-with-debug-names.
Tested on x86_64-linux.
|
|
Small typo in some example code inside a comment; the arguments were
in the wrong order.
There's no functional change after this commit.
|
|
In gdb.base/corefile-buildid.exp, in the function
do_corefile_buildid_tests, if we fail to find the build-id for the
test binary then we call 'untested', but then push on with the test,
which inevitably fails as the rest of the test depends on having found
the build-id.
I think we're missing a 'return' after the call to 'untested' which
I've now added.
Also I noticed that we call build_id_debug_filename_get and then
manually remove '.debug' from the end. This is no longer necessary,
we can just ask build_id_debug_filename_get to not add the suffix.
|
|
gdb.python/py-missing-debug.exp"
This reverts commit 29c70787112e01cd52b53bf14bdcacb0a11e0725.
After the previous commit 29c70787112e01cd52 should no longer be
needed as the curses dependency has been removed.
|
|
When running test-case gdb.xml/tdesc-regs.exp on riscv64-linux, I get:
...
(gdb) set tdesc file single-reg.xml^M
warning: Architecture rejected target-supplied description^M
(gdb) FAIL: gdb.xml/tdesc-regs.exp: set tdesc file single-reg.xml
UNSUPPORTED: gdb.xml/tdesc-regs.exp: register tests
...
The FAIL and UNSUPPORTED are produced here:
...
# If no core registers were specified, assume this target does not
# support target-defined registers. Verify that we get a warning if
# we try to use them. This not only tests the warning, but also
# reminds maintainers to add test support when they add the feature.
if {[string equal ${core-regs} ""]} {
gdb_test "set tdesc file $single_reg_xml" \
"warning: Target-supplied registers are not supported.*" \
"set tdesc file single-reg.xml"
unsupported "register tests"
return 0
}
...
The test-case contains target-specific setting of the core-regs variable, and
adding this for riscv64 bypasses this code and makes the test-case pass.
However, without that change, the test-case shouldn't produce a FAIL since
gdb isn't doing anything wrong.
Fix this by producing instead:
...
PASS: $exp: set tdesc file single-reg.xml
UNSUPPORTED: $exp: register tests (missing architecture-specific core-regs setting)
...
Tested on riscv64-linux.
|
|
On a system with python 3.6, module gdb.missing_debug imports module curses,
so when running test-case gdb.python/py-missing-debug.exp on a system without
that module installed, we run into:
...
(gdb) source py-missing-debug.py^M
Python Exception <class 'ImportError'>: Module 'curses' is not installed.^M
Use:^M
sudo zypper install python36-curses^M
to install it.^M
Error occurred in Python: Module 'curses' is not installed.^M
Use:^M
sudo zypper install python36-curses^M
to install it.^M
(gdb) FAIL: gdb.python/py-missing-debug.exp: source python script
...
Fix this by issuing UNSUPPORTED instead, and bailing out.
Tested on x86_64-linux.
Approved-by: Kevin Buettner <kevinb@redhat.com>
PR testsuite/31576
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=31576
|
|
This commit updates GDB so that thread or inferior specific
breakpoints are only inserted into the program space in which the
specific thread or inferior is running.
In terms of implementation, getting this basically working is easy
enough, now that a breakpoint's thread or inferior field is setup
prior to GDB looking for locations, we can easily use this information
to find a suitable program_space and pass this to as a filter when
creating the sals.
Or we could if breakpoint_ops::create_sals_from_location_spec allowed
us to pass in a filter program_space.
So, this commit extends breakpoint_ops::create_sals_from_location_spec
to take a program_space argument, and uses this to filter the set of
returned sals. This accounts for about half the change in this patch.
The second set of changes starts from breakpoint_set_thread and
breakpoint_set_inferior, this is called when the thread or inferior
for a breakpoint changes, e.g. from the Python API.
Previously this call would never result in the locations of a
breakpoint changing, after all, locations were inserted in every
program space, and we just use the thread or inferior variable to
decide when we should stop. Now though, changing a breakpoint's
thread or inferior can mean we need to figure out a new set of
breakpoint locations.
To support this I've added a new breakpoint_re_set_one function, which
is like breakpoint_re_set, but takes a single breakpoint, and just
updates the locations for that one breakpoint. We only need to call
this function if the program_space in which a breakpoint's thread (or
inferior) is running actually changes. If the program_space does
change then we call the new breakpoint_re_set_one function passing in
the program_space which should be used to filter the new locations (or
nullptr to indicate we should set locations in all program spaces).
This filter program_space needs to propagate down to all the re_set
methods, this accounts for the remaining half of the changes in this
patch.
There were a couple of existing tests that created thread or inferior
specific breakpoints and then checked the 'info breakpoints' output,
these needed updating. These were:
gdb.mi/user-selected-context-sync.exp
gdb.multi/bp-thread-specific.exp
gdb.multi/multi-target-continue.exp
gdb.multi/multi-target-ping-pong-next.exp
gdb.multi/tids.exp
gdb.mi/new-ui-bp-deleted.exp
gdb.multi/inferior-specific-bp.exp
gdb.multi/pending-bp-del-inferior.exp
I've also added some additional tests to:
gdb.multi/pending-bp.exp
I've updated the documentation and added a NEWS entry.
Reviewed-By: Eli Zaretskii <eliz@gnu.org>
|
|
I spotted this code within create_breakpoint:
if ((type_wanted != bp_breakpoint
&& type_wanted != bp_hardware_breakpoint) || thread != -1)
b->pspace = current_program_space;
this code is only executed when creating a pending breakpoint, and
sets the breakpoint::pspace member variable.
The above code gained the 'thread != -1' clause with this commit:
commit cc72b2a2da6d6372cbdb1d14639a5fce84e1a325
Date: Fri Dec 23 17:06:16 2011 +0000
Introduce gdb.FinishBreakpoint in Python
While the type_wanted checks were added with this commit:
commit f8eba3c61629b3c03ac1f33853eab4d8507adb9c
Date: Tue Dec 6 18:54:43 2011 +0000
the "ambiguous linespec" series
Before this breakpoint::pspace was set unconditionally.
If we look at how breakpoint::pspace is used today, some breakpoint
types specifically set this field, either in their constructors, or in
a wrapper function that calls the constructor. So, the watchpoint
type and its sub-class set this variable, as does the catchpoint type,
and all it's sub-classes.
However, code_breakpoint doesn't specifically set this field within
its constructor, though some sub-classes of
code_breakpoint (ada_catchpoint, exception_catchpoint,
internal_breakpoint, and momentary_breakpoint) do set this field.
When I examine all the places that breakpoint::pspace is used, I
believe that in every place where it is expected that this field is
set, the breakpoint type will be one that specifically sets this
field.
Next, I observe two problems with the existing code.
First, the above code is only hit for pending breakpoints, there's no
equivalent code for non-pending breakpoints. This opens up the
possibility of GDB entering non-consistent states; if a breakpoint is
first created pending and then later gets a location, the pspace field
will be set, while if the breakpoint is immediately non-pending, then
the pspace field will never be set.
Second, if we look at how breakpoint::pspace is used in the function
breakpoint_program_space_exit, we see that when a program space is
removed, any breakpoint with breakpoint::pspace set to the removed
program space, will be deleted. This makes sense, but does mean we
need to ensure breakpoint::pspace is only set for breakpoints that
apply to a single program space.
So, if I create a pending dprintf breakpoint (type bp_dprintf) then
the breakpoint::pspace variable will be set even though the dprintf is
not really tied to that one program space. As a result, when the
matching program space is removed the dprintf is incorrectly removed.
Also, if I create a thread specific breakpoint, then, thanks to the
'thread != -1' clause the wrong program space will be stored in
breakpoint::pspace (the current program space is always used, which
might not be the program space that corresponds to the selected
thread), as a result, the thread specific breakpoint will be deleted
when the matching program space is removed.
If we look at commit cc72b2a2da6d which added the 'thread != -1'
clause, we can see this change was entirely redundant, the
breakpoint::pspace is also set in bpfinishpy_init after
create_breakpoint has been called. As such, I think we can safely
drop the 'thread != -1' clause.
For the other problems, I'm proposing to be pretty aggressive - I'd
like to drop the breakpoint::pspace assignment completely from
create_breakpoint. Having looked at how this variable is used, I
believe that it is already set elsewhere in all the cases that it is
needed. Maybe this code was needed at one time, but I can't see how
it's needed any more.
There's tests to expose the issues I've spotted with this code, and
there's no regressions in testing.
|
|
The initial motivation for this commit was to allow thread or inferior
specific breakpoints to only be inserted within the appropriate
inferior's program-space. The benefit of this is that inferiors for
which the breakpoint does not apply will no longer need to stop, and
then resume, for such breakpoints. This commit does not make this
change, but is a refactor to allow this to happen in a later commit.
The problem we currently have is that when a thread-specific (or
inferior-specific) breakpoint is created, the thread (or inferior)
number is only parsed by calling find_condition_and_thread_for_sals.
This function is only called for non-pending breakpoints, and requires
that we know the locations at which the breakpoint will be placed (for
expression checking in case the breakpoint is also conditional).
A consequence of this is that by the time we figure out the breakpoint
is thread-specific we have already looked up locations in all program
spaces. This feels wasteful -- if we knew the thread-id earlier then
we could reduce the work GDB does by only looking up locations within
the program space for which the breakpoint applies.
Another consequence of how find_condition_and_thread_for_sals is
called is that pending breakpoints don't currently know they are
thread-specific, nor even that they are conditional! Additionally, by
delaying parsing the thread-id, pending breakpoints can be created for
non-existent threads, this is different to how non-pending
breakpoints are handled, so I can do this:
$ gdb -q ./gdb/testsuite/outputs/gdb.multi/pending-bp/pending-bp
Reading symbols from ./gdb/testsuite/outputs/gdb.multi/pending-bp/pending-bp...
(gdb) break foo thread 99
Function "foo" not defined.
Make breakpoint pending on future shared library load? (y or [n]) y
Breakpoint 1 (foo thread 99) pending.
(gdb) r
Starting program: /tmp/gdb/testsuite/outputs/gdb.multi/pending-bp/pending-bp
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/lib64/libthread_db.so.1".
Error in re-setting breakpoint 1: Unknown thread 99.
[Inferior 1 (process 3329749) exited normally]
(gdb)
GDB only checked the validity of 'thread 99' at the point the 'foo'
location became non-pending. In contrast, if I try this:
$ gdb -q ./gdb/testsuite/outputs/gdb.multi/pending-bp/pending-bp
Reading symbols from ./gdb/testsuite/outputs/gdb.multi/pending-bp/pending-bp...
(gdb) break main thread 99
Unknown thread 99.
(gdb)
GDB immediately checks if 'thread 99' exists. I think inconsistencies
like this are confusing, and should be fixed if possible.
In this commit the create_breakpoint function is updated so that the
extra_string, which contains the thread, inferior, task, and/or
condition information, is parsed immediately, even for pending
breakpoints.
Obviously, the condition still can't be validated until the breakpoint
becomes non-pending, but the thread, inferior, and task information
can be pulled from the extra-string, and can be validated early on,
even for pending breakpoints. The -force-condition flag is also
parsed as part of this early parsing change.
There are a couple of benefits to doing this:
1. Printing of breakpoints is more consistent now. Consider creating
a conditional breakpoint before this commit:
(gdb) set breakpoint pending on
(gdb) break pendingfunc if (0)
Function "pendingfunc" not defined.
Breakpoint 1 (pendingfunc if (0)) pending.
(gdb) break main if (0)
Breakpoint 2 at 0x401198: file /tmp/hello.c, line 18.
(gdb) info breakpoints
Num Type Disp Enb Address What
1 breakpoint keep y <PENDING> pendingfunc if (0)
2 breakpoint keep y 0x0000000000401198 in main at /tmp/hello.c:18
stop only if (0)
(gdb)
And after this commit:
(gdb) set breakpoint pending on
(gdb) break pendingfunc if (0)
Function "pendingfunc" not defined.
Breakpoint 1 (pendingfunc) pending.
(gdb) break main if (0)
Breakpoint 2 at 0x401198: file /home/andrew/tmp/hello.c, line 18.
(gdb) info breakpoints
Num Type Disp Enb Address What
1 breakpoint keep y <PENDING> pendingfunc
stop only if (0)
2 breakpoint keep y 0x0000000000401198 in main at /home/andrew/tmp/hello.c:18
stop only if (0)
(gdb)
Notice that the display of the condition is now the same for the
pending and non-pending breakpoints.
The same is true for the thread, inferior, or task information in
thread, inferior, or task specific breakpoints; this information is
displayed on its own line rather than being part of the 'What'
field.
2. We can check that the thread exists as soon as the pending
breakpoint is created. Currently there is a weird difference
between pending and non-pending breakpoints when creating a
thread-specific breakpoint.
A pending thread-specific breakpoint only checks its thread when it
becomes non-pending, at which point the thread the breakpoint was
intended for might have exited. Here's the behaviour before this
commit:
(gdb) set breakpoint pending on
(gdb) break foo thread 2
Function "foo" not defined.
Breakpoint 2 (foo thread 2) pending.
(gdb) c
Continuing.
[Thread 0x7ffff7c56700 (LWP 2948835) exited]
Error in re-setting breakpoint 2: Unknown thread 2.
[Inferior 1 (process 2948832) exited normally]
(gdb)
Notice the 'Error in re-setting breakpoint 2: Unknown thread 2.'
line, this was triggered when GDB tried to make the breakpoint
non-pending, and GDB discovers that the thread no longer exists.
Compare that to the behaviour after this commit:
(gdb) set breakpoint pending on
(gdb) break foo thread 2
Function "foo" not defined.
Breakpoint 2 (foo) pending.
(gdb) c
Continuing.
[Thread 0x7ffff7c56700 (LWP 2949243) exited]
Thread-specific breakpoint 2 deleted - thread 2 no longer in the thread list.
[Inferior 1 (process 2949240) exited normally]
(gdb)
Now the behaviour for pending breakpoints is identical to
non-pending breakpoints, the thread specific breakpoint is removed
as soon as the thread the breakpoint is associated with exits.
There is an additional change; when the pending breakpoint is
created prior to this patch we see this line:
Breakpoint 2 (foo thread 2) pending.
While after this patch we get this line:
Breakpoint 2 (foo) pending.
Notice that 'thread 2' has disappeared. This might look like a
regression, but I don't think it is. That we said 'thread 2'
before was just a consequence of the lazy parsing of the breakpoint
specification, while with this patch GDB understands, and has
parsed away the 'thread 2' bit of the spec. If folk think the old
information was useful then this would be trivial to add back in
code_breakpoint::say_where.
As a result of this commit the breakpoints 'extra_string' field is now
only used by bp_dprintf type breakpoints to hold the printf format and
arguments. This string should always be empty for other breakpoint
types. This allows some cleanup in print_breakpoint_location.
In code_breakpoint::code_breakpoint I've changed an error case into an
assert. This is because the error is now handled earlier in
create_breakpoint. As a result we know that by this point, the
extra_string will always be nullptr for anything other than a
bp_dprintf style breakpoint.
The find_condition_and_thread_for_sals function is now no longer
needed, this was previously doing the delayed splitting of the extra
string into thread, task, and condition, but this is now all done in
create_breakpoint, so find_condition_and_thread_for_sals can be
deleted, and the code that calls this in
code_breakpoint::location_spec_to_sals can be removed. With this
update this code would only ever be reached for bp_dprintf style
breakpoints, and in these cases the extra_string should not contain
anything other than format and args.
The most interesting changes are all in create_breakpoint and in the
new file break-cond-parse.c. We have a new block of code early on in
create_breakpoint that is responsible for splitting the extra_string
into its component parts by calling create_breakpoint_parse_arg_string
a function in the new break-cond-parse.c file. This means that some
of the later code can be simplified a little.
The new break-cond-parse.c file implements the splitting up the
extra_string and finding all the parts, as well as some self-tests for
the new function.
Finally, now we know all the breakpoint details, these can be stored
within the breakpoint object if we end up creating a deferred
breakpoint. Additionally, if we are creating a deferred bp_dprintf we
can parse the extra_string to build the printf command.
The implementation here aims to maintain backwards compatibility as
much as possible, this means that:
1. We support abbreviations of 'thread', 'task', and 'inferior' in
some places on the breakpoint line. The handling of abbreviations
has (before this patch) been a little weird, so this works:
(gdb) break *main th 1
And creates a breakpoint at '*main' for thread 1 only, while this
does not work:
(gdb) break main th 1
In this case GDB will try to find the symbol 'main th 1'. This
weirdness exists before and after this patch.
2. The handling of '-force-condition' is odd, if this flag appears
immediately after a condition then it will be treated as part of the
condition, e.g.:
(gdb) break main if 0 -force-condition
No symbol "force" in current context.
But we are fine with these alternatives:
(gdb) break main if 0 thread 1 -force-condition
(gdb) break main -force-condition if 0
Again, this is just a quirk of how the breakpoint line used to be
parsed, but I've maintained this for backward compatibility. During
review it was suggested that -force-condition should become an
actual breakpoint flag (i.e. only valid after the 'break' command
but before the function name), and I don't think that would be a
terrible idea, however, that's not currently a trivial change, and I
think should be done as a separate piece of work. For now, this
patch just maintains the current behaviour.
The implementation works by first splitting the breakpoint condition
string (everything after the location specification) into a list of
tokens, each token has a type and a value. (e.g. we have a THREAD
token where the value is the thread-id string). The list of tokens is
validated, and in some cases, tokens are merged. Then the values are
extracted from the remaining token list.
Consider this breakpoint command:
(gdb) break main thread 1 if argc == 2
The condition string passed to create_breakpoint_parse_arg_string is
going to be 'thread 1 if argc == 2', which is then split into the
tokens:
{ THREAD: "1" } { CONDITION: "argc == 2" }
The thread-id (1) and the condition string 'argc == 2' are extracted
from these tokens and returns back to create_breakpoint.
Now consider this breakpoint command:
(gdb) break some_function if ( some_var == thread )
Here the user wants a breakpoint if 'some_var' is equal to the
variable 'thread'. However, when this is initially parsed we will
find these tokens:
{ CONDITION: "( some_var == " } { THREAD: ")" }
This is a consequence of how we have to try and figure out the
contents of the 'if' condition without actually parsing the
expression; parsing the expression requires that we know the location
in order to lookup the variables by name, and this can't be done for
pending breakpoints (their location isn't known yet), and one of the
points of this work is that we extract things like thread-id for
pending breakpoints.
And so, it is in this case that token merging takes place. We check
if the value of a token appearing immediately after the CONDITION
token looks valid. In this case, does ')' look like a valid
thread-id. Clearly, in this case ')' does not, and so me merge the
THREAD token into the condition token, giving:
{ CONDITION: "( some_var == thread )" }
Which is what we want.
I'm sure that we might still be able to come up with some edge cases
where the parser makes the wrong choice. I think long term the best
way to work around these would be to move the thread, inferior, task,
and -force-condition flags to be "real" command options for the break
command. I am looking into doing this, but can't guarantee if/when
that work would be completed, so this patch should be reviewed assume
that the work will never arrive (though I hope it will).
Reviewed-By: Eli Zaretskii <eliz@gnu.org>
|
|
This commit changes the 'target ...' commands that accept a filename
to take a quoted or escaped filename rather than a literal filename.
What this means in practice is that if you are specifying a filename
that contains no white space or quote characters, then nothing should
change, e.g.:
target exec /path/to/some/file
works both before and after this commit.
However, if a user wishes to specify a file containing white space
then either the entire filename needs to be quoted, or the special
white space needs to be escaped. Before this patch a user could
write:
target exec /path/to a file/containing spaces
But after this commit the user would have to choose one of:
target exec "/path/to a file/containing spaces"
or
target exec /path/to\ a\ file/containing\ spaces
Obviously this is a potentially breaking change. The benefit of
making this change is consistency. Commands that take multiple
arguments (one of which is a filename) or in the future, commands that
take filename options, will always need to use quoted/escaped
filenames, so converting all unquoted filename commands to use quoting
or escaping makes the UI more consistent.
Additionally (though this is probably not a common problem), GDB
strips trailing white space from commands that the user enters. As
such it is not possible to reference any file that ends in white space
unless the quoting / escaping style is used. Though I suspect very
few users run into this problem!
The downside obviously is that this is a UI breaking change.
Reviewed-By: Eli Zaretskii <eliz@gnu.org>
|
|
This commit changes how GDB processes command arguments for the
following commands:
compile file
maint print c-tdesc
save gdb-index
After this commit these commands will now expect their single filename
argument to be (optionally) quoted if it contains any special
characters (e.g. whit space or quotes).
If the filename does not contain any special characters then nothing
changes. As an example:
(gdb) save gdb-index /path/to/some/directory/
will work before and after this patch. However, if the directory
name contains a white space then before this patch a user would write:
(gdb) save gdb-index /path/to some/directory/
But this will now fail as GDB will consider this as two arguments,
'/path/to' and 'some/directory/'. To pass this single directory name
a user must now do one of these:
(gdb) save gdb-index "/path/to some/directory/"
(gdb) save gdb-index '/path/to some/directory/'
(gdb) save gdb-index /path/to\ some/directory/
This brings these commands into line with commands like 'file' and
'symbol-file', which have supported quoted filenames for a while.
The motivation for this change is to make handling of filename
arguments consistent throughout GDB. We can't move to all commands
taking non-quoted filenames as the non-quoted style only allows for a
single argument. Additionally, the non-quoted style doesn't allow for
filenames that end in white space (though this is probably pretty
rare). So, if we want to have consistency the only choice is to move
towards supporting quote filenames.
Reviewed-By: Eli Zaretskii <eliz@gnu.org>
|
|
The 'remove-symbol-file' command doesn't currently offer command
completion. This commit addresses this.
The 'remove-symbol-file' uses gdb_argv to split its command arguments,
this means that the filename the command expects can be quoted.
However, the 'remove-symbol-file' command is a little weird in that it
also has a '-a' option, if this option is passed then the command
expects not a filename, but an address.
Currently the remove_symbol_file_command function splits the command
args using gdb_argv, checks for a '-a' flag by looking at the first
argument value, and then expects the filename or address to occupy a
single entry in the gdb_argv array.
The first thing I do is handle the '-a' flag using GDB's option
system. I model this option as a flag_option_def (a boolean option).
I've dropped the use of gdb_argv and instead use the new(ish) function
extract_single_filename_arg, which was added a couple of commits back,
to parse the filename argument (when '-a' is not given).
If '-a' is given the the remove-symbol-file command expects an address
rather than a filename. As we previously split the arguments using
gdb_argv this meant the address needed to appear as a single
argument. So a user could write:
(gdb) remove-symbol-file 0x1234
Or they could write:
(gdb) remove-symbol-file some_function
Both of these would work fine. But a user could not write:
(gdb) remove-symbol-file some_function + 0x1000
As only the 'some_function' part would be processed. Now the user
could do this:
(gdb) remove-symbol-file "some_function + 0x1000"
By enclosing the address expression in quotes this would be handled as
a single argument. However, this is a little weird, that's not how
commands like 'print' or 'x' work. Also this functionality was
neither documented, or tested.
And so, in this commit, by removing the use of gdb_argv I bring the
'remove-symbol-file' command inline with GDB's other commands that
take an expression, the quotes are no longer needed.
Usually in a completer we call 'complete_options', but don't actually
capture the option values. But for remove-symbol-file I do. This
allows me to spot when the '-a' option has been given, I can then
complete the rest of the command line as either a filename or an
expression.
Reviewed-By: Eli Zaretskii <eliz@gnu.org>
|
|
Implement the readline rl_directory_rewrite_hook callback function,
this is used when readline needs to offer completions from within a
directory. The important thing is that this function should remove
any escaping, this allows GDB to correctly offer completions in
situations like this:
(gdb) file /tmp/directory\ with\ spaces/<TAB><TAB>
Note the escaping in 'directory\ with\ spaces'. Without the
rl_directory_rewrite_hook callback readline will try to open a
directory literally called '/tmp/directory\ with\ spaces' which
obviously doesn't exist.
There are tests added to cover this new functionality.
|
|
The function gdb_rl_find_completion_word is very similar to the
readline function _rl_find_completion_word, but was either an older
version of that function, or was trimmed when copying to remove code
which was considered unnecessary.
We maintain this copy because the _rl_find_completion_word function is
not part of the public readline API, and we need to replicate the
functionality of that function as part of the 'complete' command.
Within gdb_rl_find_completion_word when looking for the completion
word, if we don't find a unclosed quoted string (which would become
the completion word) then we scan backwards looking for a word break
character. For example, given:
(gdb) complete file /tmp/foo
There is no unclosed quoted string so we end up scanning backwards
from the end looking for a word break character. In this case the
space after 'file' and before '/tmp/foo' is found, so '/tmp/foo'
becomes the completion word.
However, given this:
(gdb) complete file /tmp/foo\"
There is still no unclosed quoted string, however, when we can
backwards the '"' (double quotes) are treated as a word break
character, and so we end up using the empty string as the completion
word.
The readline function _rl_find_completion_word avoids this mistake by
using the rl_char_is_quoted_p hook. This function will return true
for the double quote character as it is preceded by a backslash. An
earlier commit in this series supplied a rl_char_is_quoted_p function
for the filename completion case, however, gdb_rl_find_completion_word
doesn't call rl_char_is_quoted_p so this doesn't help for the
'complete' case.
In this commit I've copied the code to call rl_char_is_quoted_p from
_rl_find_completion_word into gdb_rl_find_completion_word.
This half solves the problem. In the case:
(gdb) complete file /tmp/foo\"
We do now try to complete on the string '/tmp/foo\"', however, when we
reach filename_completer we call back into readline to actually
perform filename completion. However, at this point the WORD variable
points to a string that still contains the backslash. The backslash
isn't part of the actual filename, that's just an escape character.
Our expectation is that readline will remove the backslash when
looking for matching filenames. However, readline contains an
optimisation to avoid unnecessary work trying to remove escape
characters.
The readline variable rl_completion_found_quote is set in the readline
function gen_completion_matches before the generation of completion
matches. This variable is set to true (non-zero) if there is (or
might be) escape characters within the completion word.
The function rl_filename_completion_function, which generates the
filename matches, only removes escape characters when
rl_completion_found_quote is true. When GDB generates completions
through readline (e.g. tab completion) then rl_completion_found_quote
is set correctly.
But when we use the 'complete' command we don't pass through readline,
and so gen_completion_matches is never called and
rl_completion_found_quote is not set. In this case when we call
rl_filename_completion_function readline doesn't remove the escapes
from the completion word, and so in our case above, readline looks for
completions of the exact filename '/tmp/foo\"', that is, the filename
including the backslash.
To work around this problem I've added a new flag to our function
gdb_rl_find_completion_word which is set true when we find any quoting
or escaping. This matches what readline does.
Then in the 'complete' function we can set rl_completion_found_quote
prior to generating completion matches.
With this done the 'complete' command now works correctly when trying
to complete filenames that contain escaped word break characters. The
tests have been updated accordingly.
|
|
Building on the mechanism added in the previous commit(s), this commit
applies escaping to filenames in the 'complete' command output.
Consider a file: /tmp/xxx/aa"bb -- that is a filename that contains a
double quote, currently the 'complete' command output looks like this:
(gdb) complete file /tmp/xxx/a
file /tmp/xxx/aa"bb
Notice that the double quote in the output is not escaped. If we
passed this same output back to GDB then the double quote will be
treated as the start of a string.
After this commit then the output looks like this:
(gdb) complete file /tmp/xxx/a
file /tmp/xxx/aa\"bb
The double quote is now escaped. If we feed this output back to GDB
then GDB will treat this as a single filename that contains a double
quote, exactly what we want.
To achieve this I've done a little refactoring, splitting out the core
of gdb_completer_file_name_quote, and then added a new call from the
filename_match_formatter function.
There are updates to the tests to cover this new functionality.
|
|
This commit solves a problem that existed prior to the previous
commit, but the previous commit made more common.
When completing a filename with the 'complete' command GDB will always
add a trailing quote character, even if the completion is a directory
name, in which case it would be better if the trailing quote was not
added. Consider:
(gdb) complete file '/tmp/xx
file '/tmp/xxx/'
The completion offered here is really only a partial completion, we've
completed up to the end of the next directory name, but, until we have
a filename then the completion is not finished and the trailing quote
should not be added.
This would match the readline behaviour, e.g.:
(gdb) file '/tmp/xx<TAB>
(gdb) file '/tmp/xxx/
In this case readline completes the directory name, but doesn't add
the trailing quote character.
Remember that the 'complete' command is intended for tools like
e.g. emacs in order that they can emulate GDB's standard readline
completion when implementing a CLI of their own. As such, not adding
the trailing quote in this case matches the readline behaviour, and
seems like the right way to go.
To achieve this, I've added a new function pointer member variable
completion_result::m_match_formatter. This contains a pointer to a
callback function which is used by the 'complete' command to format
each result.
The default behaviour of this callback function is to just append the
quote character (the character from before the completion string) to
the end of the completion result. This matches the current behaviour.
However, for filename completion we override the default value of
m_match_formatter, this new function checks if the completion result
is a directory or not. If the completion result is a directory then
the closing quote is not added, instead we add a trailing '/'
character.
The code to add a trailing '/' character already exists within the
filename_completer function. This is no longer needed in this
location, instead this code is moved into the formatter callback.
Tests are updated to handle the changes in functionality, this removes
an xfail added in the previous commit.
|
|
Simplify completion_result::print_matches by removing one of the code
paths. Now, every time we call ::print_matches we always add the
trailing quote.
Previously, when using the 'complete' command, if there was only one
result then trailing quote was added in ::build_completion_result, but
when we had multiple results the trailing quote was added in
::print_matches. As a consequence, ::print_matches had to understand
not to add the trailing quote for the single result case.
After this commit we don't add the trailing quote in
::build_completion_result, instead ::print_matches always adds the
trailing quote, which makes ::print_matches simpler.
However, there is a slight problem. When completion is being driven
by readline, and not by the 'complete' command, we still need to
manually add the trailing quote in the single result case, and as the
printing is done by readline we can't add the quote at the time of
printing, and so, in ::build_completion_result, we still add the
trailing quote, but only when completion is being done for readline.
And this does cause a small problem. When completing a filename, if
the completion results in a directory name then, when using the
'complete' command, GDB should not be adding a trailing quote. For
example, if we have the file /tmp/xxx/foo.c, then what we should see
is this:
(gdb) complete file '/tmp/xx
file 'tmp/xxx/
But what we actually see after this commit is this:
(gdb) complete file '/tmp/xx
file 'tmp/xxx/'
Previously we didn't get the trailing quote in this case, as when
there is only a single result, the quote was added in
::build_completion_result, and for filename completion, GDB didn't
know what the quote character was in ::build_completion_result, so no
quote was added. Now that the trailing quote is always added in
::print_matches, and GDB does know the quote character at this point,
so we are now getting the trailing quote, which is not correct.
This is a regression, but really, GDB is now broken in a consistent
way, if we create the file /tmp/xxa/bar.c, then previously if we did
this:
(gdb) complete file '/tmp/xx
file '/tmp/xxa/'
file '/tmp/xxx/'
Notice how we get the trailing quote in this case, this is the before
patch behaviour, and is also wrong.
A later commit will fix things so that the trailing quote is not added
in this filename completion case, but for now I'm going to accept this
small regression.
This change in behaviour caused some failures in one of the completion
tests, I've tweaked the test case to expect the trailing quote as part
of this commit, but will revert this in a later commit in this series.
I've also added an extra test for when the 'complete' command does
complete to a single complete filename, in which case the trailing
quote is expected.
|
|
This improves quoting and escaping when completing filenames for
commands that allow filenames to be quoted and escaped.
I've struggled a bit trying to split this series into chunks. There's
a lot of dependencies between different parts of the completion
system, and trying to get this working correctly is pretty messy.
This first step is really about implementing 3 readline hooks:
rl_char_is_quoted_p
- Is a particular character quoted within readline's input buffer?
rl_filename_dequoting_function
- Remove quoting characters from a filename.
rl_filename_quoting_function
- Add quoting characters to a filename.
See 'info readline' for full details, but with these hooks connected
up, readline (on behalf of GDB) should do a better job inserting
backslash escapes when completing filenames.
There's still a bunch of stuff that doesn't work after this commit,
mostly around the 'complete' command which of course doesn't go
through readline, so doesn't benefit from all of these new functions
yet, I'll add some of this in a later commit.
Tab completion is now slightly improved though, it is possible to
tab-complete a filename that includes a double or single quote, either
in an unquoted string or within a string surrounded by single or
double quotes, backslash escaping is used when necessary.
There are some additional tests to cover the new functionality.
|
|
Unfortunately we have two different types of filename completion in
GDB.
The majority of commands have what I call unquoted filename
completion, this is for commands like 'set logging file ...', 'target
core ...', and 'add-auto-load-safe-path ...'. For these commands
everything after the command name (that is not a command option) is
treated as a single filename. If the filename contains white space
then this does not need to be escaped, nor does the filename need to
be quoted. In fact, the filename argument is not de-quoted, and does
not have any escaping removed, so if a user does try to add such
things, they will be treated as part of the filename. As an example:
(gdb) target core "/path/that contains/some white space"
Will look for a directory calls '"' (double quotes) in the local
directory.
A small number of commands do de-quote and remove escapes from
filename arguments. These command accept what I call quoted and
escaped filenames. Right now these are the commands that specify the
file for GDB to debug, so:
file
exec-file
symbol-file
add-symbol-file
remove-symbol-file
As an example of this in action:
(gdb) file "/path/that contains/some white space"
In this case GDB would load the file:
/path/that contains/some white space
Current filename completion always assumes that filenames can be
quoted, though escaping doesn't work in completion right now. But the
assumption that quoting is allowed is clearly wrong.
This commit splits filename completion into two. The existing
filename_completer is retained, and is used for unquoted filenames. A
second filename_maybe_quoted_completer is added which can be used for
completing quoted filenames.
The filename completion test has been extended to cover more cases.
As part of the extended testing I need to know the character that
should be used to separate filenames within a path. For this TCL 8.6+
has $::tcl_platform(pathSeparator). To support older versions of TCL
I've added some code to testsuite/lib/gdb.exp.
You might notice that after this commit the completion for unquoted
files is all done in the brkchars phase, that is the function
filename_completer_handle_brkchars calculates the completions and
marks the completion_tracker as using a custom word point. The reason
for this is that we don't want to break on white space for this
completion, but if we rely on readline to find the completion word,
readline will consider the entire command line, and with no white
space in the word break character set, readline will end up using the
entire command line as the word to complete.
For now at least, the completer for quoted filenames does generate its
completions during the completion phase, though this is going to
change in a later commit.
|
|
There are 3 places where we currently call debuginfod_exec_query to
lookup an objfile for a given build-id.
In one of these places we first call build_id_to_exec_bfd which also
looks up an objfile given a build-id, but this function looks on disk
for a symlink in the .build-id/ sub-directory (within the
debug-file-directory).
I can't think of any reason why we shouldn't call build_id_to_exec_bfd
before every call to debuginfod_exec_query.
So, in this commit I have added a new function in build-id.c,
find_objfile_by_build_id, this function calls build_id_to_exec_bfd,
and if that fails, then calls debuginfod_exec_query.
Everywhere we call debuginfod_exec_query is updated to call the new
function, and in locate_exec_from_corefile_build_id, the existing call
to build_id_to_exec_bfd is removed as calling find_objfile_by_build_id
does this for us.
One slight weird thing is in core_target::build_file_mappings, here we
call find_objfile_by_build_id which returns a gdb_bfd_ref_ptr for the
opened file, however we immediately reopen the file as "binary". The
reason for this is that all the bfds opened in ::build_file_mappings
need to be opened as "binary" (see the function comments for why).
I did consider passing a target type into find_objfile_by_build_id,
which could then be forwarded to build_id_to_exec_bfd and used to open
the BFD as "binary", however, if you follow the call chain you'll end
up in build_id_to_debug_bfd_1, where we actually open the bfd. Notice
in here that we call build_id_verify to double check the build-id of
the file we found, this requires that the bfd not be opened as
"binary".
What this means is that we always have to first open the bfd using the
gnutarget target type (for the build-id check), and then we would have
to reopen it as "binary". There seems little point pushing the reopen
logic into find_objfile_by_build_id, so we just do this in the
::build_file_mappings function.
I've extended the tests to cover the two cases which actually changed
in this commit.
|
|
When GDB opens a core file, in 'core_target::build_file_mappings ()',
we collection information about the files that are mapped into the
core file, specifically, the build-id and the DT_SONAME attribute for
the file, which will be set for some shared libraries.
We then cache the DT_SONAME to build-id information on the core file
bfd object in the function set_cbfd_soname_build_id.
Later, when we are loading the shared libraries for the core file, we
can use the library's file name to look in the DT_SONAME to build-id
map, and, if we find a matching entry, we can use the build-id to
validate that we are loading the correct shared library.
This works OK, but has some limitations: not every shared library will
have a DT_SONAME attribute. Though it is good practice to add such an
attribute, it's not required. A library without this attribute will
not have its build-id checked, which can lead to GDB loading the wrong
shared library.
What I want to do in this commit is to improve GDB's ability to use
the build-ids extracted in core_target::build_file_mappings to both
validate the shared libraries being loaded, and then to use these
build-ids to potentially find (via debuginfod) the shared library.
To do this I propose making the following changes to GDB:
(1) Rather than just recording the DT_SONAME to build-id mapping in
set_cbfd_soname_build_id, we should also record, the full filename to
build-id mapping, and also the memory ranges to build-id mapping for
every memory range covered by every mapped file.
(2) Add a new callback solib_ops::find_solib_addr. This callback
takes a solib object and returns an (optional) address within the
inferior that is part of this library. We can use this address to
find a mapped file using the stored memory ranges which will increase
the cases in which a match can be found.
(3) Move the mapped file record keeping out of solib.c and into
corelow.c. Future commits will make use of this information from
other parts of GDB. This information was never solib specific, it
lived in the solib.c file because that was the only user of the data,
but really, the data is all about the core file, and should be stored
in core_target, other parts of GDB can then query this data as needed.
Now, when we load a shared library for a core file, we do the
following lookups:
1. Is the exact filename of the shared library found in the filename
to build-id map? If so then use this build-id for validation.
2. Find an address within the shared library using ::find_solib_addr
and then look for an entry in the mapped address to build-id map.
If an entry is found then use this build-id.
3. Finally, look in the soname to build-id map. If an entry is
found then use this build-id.
The addition of step #2 here means that GDB is now far more likely to
find a suitable build-id for a shared library. Having acquired a
build-id the existing code for using debuginfod to lookup a shared
library object can trigger more often.
On top of this, we also create a build-id to filename map. This is
useful as often a shared library is implemented as a symbolic link to
the actual shared library file. The mapped file information is stored
based on the actual, real file name, while the shared library
information holds the original symbolic link file name.
If when loading the shared library, we find the symbolic link has
disappeared, we can use the build-id to file name map to check if the
actual file is still around, if it is (and if the build-id matches)
then we can fall back to use that file. This is another way in which
we can slightly increase the chances that GDB will find the required
files when loading a core file.
Adding all of the above required pretty much a full rewrite of the
existing set_cbfd_soname_build_id function and the corresponding
get_cbfd_soname_build_id function, so I have taken the opportunity to
move the information caching out of solib.c and into corelow.c where
it is now accessed through the function core_target_find_mapped_file.
At this point the benefit of this move is not entirely obvious, though
I don't think the new location is significantly worse than where it
was originally. The benefit though is that the cached information is
no longer tied to the shared library loading code.
I already have a second set of patches (not in this series) that make
use of this caching from elsewhere in GDB. I've not included those
patches in this series as this series is already pretty big, but even
if those follow up patches don't arrive, I think the new location is
just as good as the original location.
Rather that caching the information within the core file BFD via the
registry mechanism, the information used for the mapped file lookup is
now stored within the core_file target directly.
|
|
This commit improves how GDB handles file backed mappings within a
core file, specifically, this is a restructuring of the function
core_target::build_file_mapping.
The primary motivation for this commit was to put in place the
infrastructure to support the next commit in this series, but this
commit does itself make some improvements.
Currently in core_target::build_file_mapping we use
gdbarch_read_core_file_mappings to iterate over the mapped regions
within a core file.
For each region a callback is invoked which is passed details of the
mapping; the file the mapping is from, the offset into the file, and
the address range at which the mapping exists. We are also passed the
build-id for the mapped file in some cases.
We are only told the build-id for the mapped region which actually
contains the ELF header of the mapped file. Other regions of the same
mapped ELF will not have the build-id passed to the callback.
Within core_target::build_file_mapping, in the per-region callback, we
try to find the mapped file based on its filename. If the file can't
be found, and if we have a build-id then we'll ask debuginfod to
download the file.
However we find the file, we cache the opened bfd object, which is
good. Subsequent mappings from the same file will not have a build-id
set, but by that point we already have a cached open bfd object, so
the lack of build-id is irrelevant.
The problem with the above is that if we find a matching file based on
the filename, then we accept that file, even if we have a build-id,
and the build-id doesn't match.
Currently, the mapped region processing is done in a single pass, we
call gdbarch_read_core_file_mappings, and for each mapping, as we see
it, we create the data structures needed to represent that mapping.
In this commit, I will change this to a two phase process. In the
first phase the mappings are grouped together based on the name of the
mapped file. At the end of phase one we have a 'struct mapped_file',
a new struct, for each mapped file. This struct associates an
optional build-id with a list of mapped regions.
In the second phase we try to find the file using its filename. If
the file is found, and the 'struct mapped_file' has a build-id, then
we'll compare the build-id with the file we found. This allows us to
reject on-disk files which have changed since the core file was
created.
If no suitable file was found (either no file found, or a build-id
mismatch) then we can use debuginfod to potentially download a
suitable file.
NOTE: In the future we could potentially add additional sanity
checks here, for example, if a data-file is mapped, and has no
build-id, we can estimate a minimum file size based on the expected
mappings. If the file we find is not big enough then we can reject
the on-disk file. But I don't know how useful this would actually
be, so I've not done that for now.
Having found (or not) a suitable file then we can create the data
structures for each mapped region just as we did before.
The new functionality here is the extra build-id check, and the
possibility of rejecting an on-disk file if the build-id doesn't
match.
This change could have been done within the existing single phase
approach I think, however, in the next approach I need to have all the
mapped regions associated with the expected build-id, and the new two
phase structure allows me to do that, this is the reason for such an
extensive rewrite in this commit.
There's a new test that exercises GDB's ability to find mapped files
via the build-id, and this downloading from debuginfod.
|
|
When GDB opens a core file the bfd library processes the core file and
creates sections within the bfd object to represent each of the
segments within the core file.
GDB then creates two target_section lists, m_core_section_table and
m_core_file_mappings, these, along with m_core_unavailable_mappings,
are used by GDB to implement core_target::xfer_partial; this is the
function used when GDB tries to read memory from a core file inferior.
The m_core_section_table list represents sections within the core file
itself. The sections in this list can be split into two groups based
on whether the section has the SEC_HAS_CONTENTS flag set or not.
Sections (from the core file) that have the SEC_HAS_CONTENTS flag had
their contents copied into the core file when the core file was
created. These correspond to writable sections within the original
inferior (the inferior for which the core file was created).
Sections (from the core file) that do not have the SEC_HAS_CONTENTS
flag will not have had their contents copied into the core file when
it was created. These sections correspond to read-only sections
mapped from a file (possibly the initial executable, or possibly some
other file) in the original inferior. The expectation is that the
contents of these sections can still be found by looking in the file
that was originally mapped.
The m_core_file_mappings list is created when GDB parses the mapped
file list in the core file. Every mapped region will be covered by
entries in the m_core_section_table list (see above), but for
read-only mappings the entry in m_core_section_table will not have the
SEC_HAS_CONTENTS flag set. As GDB parses the mapped file list, if the
file that was originally mapped can be found, then GDB creates an
entry in the m_core_file_mappings list which represents the region
of the file that was mapped into the original inferior.
However, GDB only creates entries in m_core_file_mappings if it is
able to find the correct on-disk file to open. If the file can't be
found then an entry is added to m_core_unavailable_mappings instead.
If is the handling m_core_unavailable_mappings which I think is
currently not completely correct.
When a read lands within an m_core_unavailable_mappings region we
currently forward the read to the exec file stratum. The reason for
this is this: when GDB read the mapped file list, if the executable
file could not be found at the expected path then mappings within the
executable will end up in the m_core_unavailable_mappings list.
However, the user might provide the executable to GDB from a different
location. If this happens then forwarding the read to the exec file
stratum might give a result.
But, if the exec file stratum does not resolve the access then
currently we continue through ::xfer_partial, the next step of which
is to handle m_core_section_table entries that don't have the
SEC_HAS_CONTENTS flag set. Every m_core_unavailable_mappings entry
will naturally have an m_core_section_table without the
SEC_HAS_CONTENTS flag set, and so we treat the unavailable mapping as
zero initialised memory and return all zeros.
It is this fall through behaviour that I think is wrong. If a read
falls in an unavailable region, and the exec file stratum cannot help,
then I think the access should fail.
To achieve this goal I have removed the xfer_memory_via_mappings
helper function and moved its content inline into ::xfer_partial.
Now, if an access is within an m_core_unavailable_mappings region, and
the exec file stratum doesn't help, we immediately return with an
error.
The reset of ::xfer_partial is unchanged, I've extended some comments
in the area that I have changed to (I hope) explain better what's
going on.
There's a new test that covers the new functionality, an inferior maps
a file and generates a core file. We then remove the mapped file,
load the core file and try to read from the mapped region. The
expectation is that GDB should give an error rather than claiming that
the region is full of zeros.
|
|
A user noticed that when an Ada program (including the runtime) is
compiled with -flto, then "catch exception" does not work -- even
though setting the equivalent breakpoint by hand does work.
Looking into this, it turns out that GCC puts the exception functions
from the Ada runtime into a CU that uses the C language, not Ada.
Then, when trying to look up the relevant symbol,
lookup_name_info::search_name_hash uses the "verbatim" form of the
symbol name (like "<__gnat_debug_raise_exception>") rather than the
"<>"-less form, causing the symbol not to be found.
This patch fixes the problem in two steps.
First, lookup_name_info::search_name_hash is changed to use the same
hack that language_defn::get_symbol_name_matcher uses. That is, when
the current language is Ada, verbatim-mode lookups are special-cased.
(This is a bit unfortunate; perhaps a better long term approach would
be to promote verbatim mode to a fundamental mode of
lookup_name_info.)
Second, although the above fixes the problem in the Ada language mode,
the code still fails in other languages. However, due to the way
these lookups are coded in ada-lang.c, I think it makes sense to
temporarily set the current language to Ada in
create_ada_exception_catchpoint.
Tested on x86-64 Fedora 38.
A new test case that mimics the -flto scenario is included.
Reviewed-By: Alexandra Petlanova Hajkova <ahajkova@redhat.com>
|
|
While working on a longer series, I needed to make sure this
particular test kept working with -fgnat-encodings=all, so this patch
adds it to the test.
|
|
gnat-llvm does not support the -fgnat-encodings flag. This patch
prepares gdb's Ada tests to handle this situation by introducing a new
foreach_gnat_encoding. A subsequent patch may change this to support
gnat-llvm; meanwhile this is a little cleaner anyway.
|
|
It is possible that the compiler is configured to do
so automatically, but at least for GCC the configure option
--enable-linker-build-id is not enabled by default.
So the option -Wl,--build-id should be used regardless
of which compiler is used.
Approved-By: Tom de Vries <tdevries@suse.de>
|
|
Fix formatting of a Python file added in commit:
commit a92e943014f5e8d6a2eaccaf8a725941ac47a121
Date: Wed Aug 14 15:16:46 2024 +0100
gdb: implement ::re_set method for catchpoint class
No functional change after this commit.
|
|
It is possible to attach a condition to a catchpoint. This can't be
done when the catchpoint is created, but can be done with the
'condition' command, this is documented in the GDB manual:
You can also use the 'if' keyword with the 'watch' command. The
'catch' command does not recognize the 'if' keyword; 'condition' is the
only way to impose a further condition on a catchpoint.
A GDB crash was reported against Fedora GDB where a user had attached
a condition to a catchpoint and then restarted the inferior. When the
catchpoint was hit GDB would immediately segfault. I was able to
reproduce the failure on upstream GDB:
(gdb) file ./some/binary
(gdb) catch syscall write
(gdb) run
...
Catchpoint 1 (returned from syscall write), 0x00007ffff7b594a7 in write () from /lib64/libc.so.6
(gdb) condition 1 $_streq((char *) $rsi, "foobar") == 0
(gdb) run
...
Fatal signal: Segmentation fault
...
What happened here is that on the system in question we had debug
information available for both the main application and also for
libc.
When the condition was attached GDB was stopped inside libc and as the
debug information was available GDB found a reference to the 'char'
type (for the cast) inside libc's debug information.
When the inferior is restarted GDB discards all of the objfiles
associated with shared libraries, and this includes libc. As such the
'char' type, which is objfile owned, is discarded and the reference to
it from the catchpoint's condition expression becomes invalid.
Now, if it were a breakpoint instead of a catchpoint, what would
happen is that after the shared library objfiles had been discarded
we'd call the virtual breakpoint::re_set method on the breakpoint, and
this would update the breakpoint's condition expression. This is
because user breakpoints are actually instances of the code_breakpoint
class and the code_breakpoint::re_set method contains the code to
recompute the breakpoint's condition expression.
However, catchpoints are instances of the catchpoint class which
inherits from the base breakpoint class. The catchpoint class does
not override breakpoint::re_set, and breakpoint::re_set is empty!
The consequence of this is that catchpoint condition expressions are
never recomputed, and the dangling pointer to the now deleted, objfile
owned type 'char' is left around, and, when the catchpoint is hit, the
invalid pointer is used when GDB tries to evaluate the condition
expression.
In this commit I have implemented catchpoint::re_set. This is pretty
simple and just recomputes the condition expression as you'd expect.
If the condition doesn't evaluate then the catchpoint is marked as
disabled_by_cond.
I have also made breakpoint::re_set pure virtual. With the addition
of catchpoint::re_set every sub-class of breakpoint now implements the
::re_set method, and if new sub-classes are added in the future I
think that they _must_ implement ::re_set in order to avoid this
problem. As such falling back to an empty breakpoint::re_set doesn't
seem helpful.
For testing I have not relied on stopping in libc and having libc
debug information available, this doesn't seem like a good idea for
the GDB testsuite. Instead I create a (rather pointless) condition
check that uses a type defined only within a shared library. When the
inferior is restarted the catchpoint will temporarily be marked as
disabled_by_cond (due to the type not being available), but once the
shared library is loaded again the catchpoint will be re-enabled.
Without the fixes above then the same crashing behaviour can be
observed.
One point of note: the dangling pointer of course exposes undefined
behaviour, with no guarantee of a crash. Though a crash is what I
usually see I have see GDB throw random errors from the expression
evaluation code, and once, I saw no problem at all! If you recompile
GDB with the address sanitizer, or run under valgrind, then the bug
will be exposed every time.
After fixing this bug I checked bugzilla and found PR gdb/29960 which
is the same bug. I was able to reproduce the bug before this commit,
and after this commit GDB is no longer crashing.
Before:
(gdb) file /tmp/hello.x
Reading symbols from /tmp/hello.x...
(gdb) run
Starting program: /tmp/hello.x
Hello World
[Inferior 1 (process 1101855) exited normally]
(gdb) catch syscall 1
Catchpoint 1 (syscall 'write' [1])
(gdb) condition 1 write.fd == 1
(gdb) run
Starting program: /tmp/hello.x
Fatal signal: Segmentation fault
...
And after:
(gdb) file /tmp/hello.x
Reading symbols from /tmp/hello.x...
(gdb) run
Starting program: /tmp/hello.x
Hello World
Args: ( 0 , 1 , 2 , 3 , 4 , 5 , 6 , 7 )
[Inferior 1 (process 1102373) exited normally]
(gdb) catch syscall 1
Catchpoint 1 (syscall 'write' [1])
(gdb) condition 1 write.fd == 1
(gdb) r
Starting program: /tmp/hello.x
Error in testing condition for breakpoint 1:
Attempt to extract a component of a value that is not a structure.
Catchpoint 1 (call to syscall write), 0x00007ffff7eb94a7 in write ()
from /lib64/libc.so.6
(gdb) ptype write
type = <unknown return type> ()
(gdb)
Notice we get the error now when the condition fails to evaluate.
This seems reasonable given that 'write' will be a function, and
indeed the final 'ptype' shows that it's a function, not a struct.
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=29960
Reviewed-By: Tom de Vries <tdevries@suse.de>
|
|
On riscv64-linux, with test-case gdb.arch/riscv-tdesc-regs.exp I get:
...
(gdb) info registers fflags^M
fflags 0x0 NV:0 DZ:0 OF:0 UF:0 NX:0^M
(gdb) FAIL: gdb.arch/riscv-tdesc-regs.exp: info registers fflags
info registers frm^M
frm 0x0 FRM:0 [RNE (round to nearest; ties to even)]^M
(gdb) FAIL: gdb.arch/riscv-tdesc-regs.exp: info registers frm
...
The FAILs are produced by:
...
foreach reg {fflags frm} {
gdb_test_multiple "info registers $reg" "" {
-re "^info registers $reg\r\n" {
exp_continue
}
-wrap -re "^Invalid register `$reg`" {
fail $gdb_test_name
}
-wrap -re "^$reg\\s+\[^\r\n\]+" {
pass $gdb_test_name
}
}
}
...
The first clause is meant to consume the command.
The '^' char was updated to mean "consume command", so that clause no longer
works since it now attempts to consume the command twice.
Also, it's unnecessary because the following clauses start with ^.
Then, the second clause is unnecessary because there's a default clause
producing the FAIL.
Fix this by simplifying to:
...
foreach reg {fflags frm} {
gdb_test "info registers $reg" "^$reg\\s+\[^\r\n\]+"
}
...
Tested on riscv64-linux.
Approved-By: Andrew Burgess <aburgess@redhat.com>
|
|
With test-case gdb.dwarf2/dw2-lines.exp on arm-linux, I run into:
...
(gdb) break bar_label^M
Breakpoint 2 at 0x4004f6: file dw2-lines.c, line 29.^M
(gdb) continue^M
Continuing.^M
^M
Breakpoint 2, bar () at dw2-lines.c:29^M
29 foo (2);^M
(gdb) PASS: $exp: cv=2: cdw=32: lv=2: ldw=32: continue to breakpoint: foo \(1\)
...
The pass is incorrect because the continue lands at line 29 with "foo (2)"
instead of line line 27 with "foo (1)".
A minimal version is:
...
$ gdb -q -batch dw2-lines.cv-2-cdw-32-lv-2-ldw-32 -ex "b bar_label"
Breakpoint 1 at 0x4f6: file dw2-lines.c, line 29.
...
where:
...
000004ec <bar>:
4ec: b580 push {r7, lr}
4ee: af00 add r7, sp, #0
000004f0 <bar_label>:
4f0: 2001 movs r0, #1
4f2: f7ff fff1 bl 4d8 <foo>
000004f6 <bar_label_2>:
4f6: 2002 movs r0, #2
4f8: f7ff ffee bl 4d8 <foo>
...
So, how does this happen? In short:
- skip_prologue_sal calls arm_skip_prologue with pc == 0x4ec,
- thumb_analyze_prologue returns 0x4f2
(overshooting by 1 insn, PR tdep/31981), and
- skip_prologue_sal decides that we're mid-line, and updates to 0x4f6.
However, this is a test-case about .debug_line info, so why didn't arm_skip_prologue
use the line info to skip the prologue?
The answer is that the line info starts at bar_label, not at bar.
Fixing that allows us to work around PR tdep/31981.
Likewise in gdb.dwarf2/dw2-line-number-zero.exp.
Instead, add a new test-case gdb.arch/skip-prologue.exp that is dedicated to
checking quality of architecture-specific prologue analysis, without being
written in an architecture-specific way.
If fails on arm-linux for both marm and mthumb:
...
FAIL: gdb.arch/skip-prologue.exp: f2: $bp_addr == $prologue_end_addr (skipped too much)
FAIL: gdb.arch/skip-prologue.exp: f4: $bp_addr == $prologue_end_addr (skipped too much)
...
and passes for:
- x86_64-linux for {m64,m32}x{-fno-PIE/-no-pie,-fPIE/-pie}
- aarch64-linux.
Tested on arm-linux.
|
|
Fix a few typos.
unconditionaly -> unconditionally
gratuitiously -> gratuitously
configureable -> configurable
represention -> representation
distiguished -> distinguished
breakpointer -> breakpoint
asssignments -> assignments
architectual -> architectural
compatibity -> compatibility
adjustement -> adjustment
unexcepted -> unexpected
propogated -> propagated
consistant -> consistent
succeding -> succeeding
higlight -> highlight
detachs -> detach
Tested by rebuilding on x86_64-linux.
Approved-By: Simon Marchi <simon.marchi@efficios.com>
|
|
On riscv64-linux, I run into:
...
Expecting: ^(catch syscall[^M
]+)?((&.*)*.*~"Catchpoint 5 .*\\n".*=breakpoint-created,bkpt=\{number="5",type="catchpoint".*\}.*\n\^done[^M
]+[(]gdb[)] ^M
[ ]*)
catch syscall^M
&"catch syscall\n"^M
&"The feature 'catch syscall' is not supported on this architecture yet.\n"^M
^error,msg="The feature 'catch syscall' is not supported on this architecture yet."^M
(gdb) ^M
FAIL: gdb.mi/mi-breakpoint-changed.exp: test_insert_delete_modify: catch syscall (unexpected output)
...
Fix this by:
- factoring out proc supports_catch_syscall out of gdb.base/catch-syscall.exp,
and
- using it in gdb.mi/mi-breakpoint-changed.exp.
Tested on x86_64-linux and riscv64-linux.
Approved-By: Andrew Burgess <aburgess@redhat.com>
|
|
I noticed that some gdb.ada tests used regular expressions like:
"Continuing\..*$inferior_exited_re.*" \
Here, the "\." should either be "." or "\\." -- "\." is not really
meaningful.
This patch fixes all the cases of this I could find in gdb.ada. In
one test (fun_renaming.exp), using "\\." would result in failures, and
here I rewrote the tests to use -wrap.
Approved-By: Andrew Burgess <aburgess@redhat.com>
|
|
When debugging ROCm code, you might have something like this:
__global__ void kernel ()
{
...
// break here
...
}
int main ()
{
// Code to call `kernel`
}
... where kernel is a function compiled to execute on the GPU. It does
not exist in the host x86-64 program that runs the main function, and
GDB doesn't know about that function until it is called, at which point
the runtime loads the corresponding code object and GDB learns about the
code of the "kernel" function. Before the GPU code object is loaded,
from the point of view of GDB, you might as well have blank lines
instead of the "kernel" function. The DWARF in the host program doesn't
describe anything at these lines.
So, a common problem that users face is:
- Start GDB with the host binary
- Place a breakpoint by line number at the "break here" line
- At this point, GDB only knows about the host code, the lines of the
`kernel` function are a big void.
- GDB finds no code mapped to the "break here" line and searches for
the first following line that has code mapped to it.
- GDB finds that the line with the opening bracket of the `main`
function (or around there) has code mapped to it, places breakpoint
there.
- User runs the program.
- The programs hits the breakpoint at the start of main.
- User is confused, because they didn't ask for a breakpoint in main.
If they continue, the code object eventually gets loaded, GDB reads the
debug info from it, re-evaluates the breakpoint locations, and at this
point the breakpoint is placed at the expected location.
The goal of this patch is to get rid of this annoyance.
A case similar to the one shown above can actually be simulated without
GPU-specific code: using a single source file to generate a library and
an executable loading that library (see the new test
gdb.linespec/line-breakpoint-outside-function.c for an example). Before
the library is loaded, trying to place a breakpoint in the library code
results in the breakpoint "drifting" down to the main function.
To address this problem, make it so that when a user requests a
breakpoint outside a function, GDB makes a pending breakpoint, rather
than placing a breakpoint at the next line with code, which happens to
be in the next function. When the GPU kernel or shared library gets
loaded, the breakpoint resolves to a location in the kernel or library.
Note that we still want breakpoints placed inside a function to
"drift" down to the next line with code. For example, here:
9
10 void foo()
11 {
12 int x;
13
14 x++;
There is probably no code associated to lines 10, 12 and 13, but the
user can still reasonably expect to be able to put a breakpoint there.
In my experience, GCC maps the function prologue to the line with the
opening curly bracket, so the user will be able to place a breakpoint
there anyway (line 11 in the example). But I don't really see a use
case to put a breakpoint above line 10 and expect to get a breakpoint in
foo. So I think that is a reasonable behavior change for GDB.
This is implemented using the following heuristic:
- If a breakpoint is requested at line L but there is no code mapped to
L, search for a following line with associated code (this already
exists today).
- However, if:
1. the found location falls in a function symbol's block
2. the found location's address is equal the entry PC of that
function
3. the found location's line is greater that the requested line
... then we don't place a breakpoint at the found location, we will
end up with a pending breakpoint.
Change the message "No line X in file..." to "No compiled code for line
X in file...". There is clearly a line 9 in the example above, so it
would be weird to say "No line 9 in file...". What we mean is that
there is no code associated to line 9.
All the regressions that I found this patch to cause were:
1. tests specifically this behavior where placing a breakpoint before
a function results in a breakpoint on that function, in which case I
removed the tests or changed them to expect a pending breakpoint
2. linespec tests expecting things like "break -line N garbage" to
error out because of the following garbage, but we now got a
different error because line N now doesn't resolve to something
anymore. For example, before:
(gdb) break -line 3 if foofoofoo == 1
No symbol "foofoofoo" in current context.
became
(gdb) break -line 3 if foofoofoo == 1
No line 3 in the current file.
These tests were modified to refer to a valid line with code, so
that we can still test what we intended to test.
Notes:
- The CUDA compiler "solves" this problem by adding dummy function
symbols between functions, that are never called. So when you try to
insert a breakpoint in the not-yet-loaded kernel, the breakpoint
still drifts, but is placed on some dummy symbol. For reasons that
would be too long to explain here, the ROCm compiler does not do
that, and it is not a desirable option.
- You can have constructs like this:
void host_function()
{
struct foo
{
static void __global__ kernel ()
{
// Place breakpoint here
}
};
// Host code that calls `kernel`
}
The heuristic won't work then, as the breakpoint will drift somewhere
inside the enclosing function, but won't be at the start of that
function. So a bogus breakpoint location will be created on the host
side. I don't think that people are going to use this kind of
construct often though, so we can probably ignore it (or at least it
shouldn't prevent making the more common case better).
ROCm doesn't support passing a lambda kernel function to
hipLaunchKernelGGL (the function used to launch kernels on the
device), but if it eventually does, there will be the same
problem.
I think that to properly support this, we will need some DWARF
improvements to be able to say "there is really nothing at these
lines" in the line table.
Co-Authored-By: Simon Marchi <simon.marchi@efficios.com>
Change-Id: I3cc12cfa823dc7d8e24dd4d35bced8e8baf7f9b6
|
|
On openSUSE Tumbleweed, I run into:
...
(gdb) PASS: gdb.threads/stepi-over-clone.exp: catch process syscalls
continue^M
Continuing.^M
^M
Catchpoint 2 (call to syscall clone3), __clone3 () at clone3.S:62^M
(gdb) FAIL: gdb.threads/stepi-over-clone.exp: continue
...
Fix this by updating another (see commit 8fbf220321d) regexp to also recognize
__clone3.
Tested on x86_64-linux.
|
|
Usually, with test-case gdb.arch/i386-disp-step-self-call.exp I get:
...
(gdb) x/1wx 0xffffc4f8^M
0xffffc4f8: 0x08048472^M
(gdb) PASS: $exp: check return address was updated correctly
...
but sometimes I run into:
...
(gdb) x/1wx 0xffffc5c8^M
0xffffc5c8: 0x0804917e^M
(gdb) FAIL: $exp: check return address was updated correctly
...
The problem is that here:
...
set next_insn_addr 0x[format %08X $next_insn_addr]
gdb_test "x/1wx 0x[format %x $sp]" "$hex:\\s+$next_insn_addr" \
"check return address was updated correctly"
...
we're trying to match string 0x0804917e against regexp 0x0804917E due to using
"%08X" as format string.
We only run into this problem if the address contains letters, which apparently
usually isn't the case.
Fix this by using "%08x" instead as format string.
Likewise in test-case gdb.arch/amd64-disp-step-self-call.exp.
Tested on x86_64-linux.
PR testsuite/32121
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=32121
|
|
The recent commit 089197010993b3a5dc50bf882470bab2de696d92 changed the
warnings when GDB reaches the end of the recorded history, and updated
tests to expect the new messages. The pattern used for
gdb.btrace/non-stop.exp, however, was too broad and could cause the
following test result:
...
(gdb) PASS: gdb.btrace/non-stop.exp: no progress: all: thread apply all continue: prompt
^M
Reached end of recorded history; stopping.^M
Following forward execution will be added to history.^M
test (arg=0x0) at /data/vries/gdb/src/gdb/testsuite/gdb.btrace/non-stop.c:30^M
30 return arg; /* bp.2 */^M
^M
Reached end of recorded history; stopping.^M
Following forward execution will be added to history.^M
test (arg=0x0) at /data/vries/gdb/src/gdb/testsuite/gdb.btrace/non-stop.c:30^M
30 return arg; /* bp.2 */^M
PASS: gdb.btrace/non-stop.exp: no progress: all: thread apply all continue: thread 0
FAIL: gdb.btrace/non-stop.exp: no progress: all: thread apply all continue: thread 1 (timeout)
...
This happens because the pattern looks like one of these 2:
"Reached end of recorded.*Backwards execution.*"
"Reached end of recorded.*Following forward.*"
What seems to have happened is that all the output came at once, and
most of it was consumed by the first '.*' pattern when checking for
thread 0, so there was no output left for checking thread 1. This commit
fixes that by making the expected outputs more exact.
I also fixed the whitespace errors in gdb_cont_to_no_history_backwards
that pre-dated the commit above, since I was already touching that proc.
Approved-By: Tom de Vries <tdevries@suse.de>
|
|
New 'no-delete-breakpoints' option for the 'runto' proc. This option
disables the delete_breakpoints call early on in this proc.
There are a couple of places in the testsuite where I have used:
proc no_delete_breakpoints {} {}
with_override delete_breakpoints no_delete_breakpoints {
if {![runto_main]} {
return
}
}
In order to avoid the deleting all breakpoints when I call
runto_main. I was about to add yet another instance of this pattern
and I figured that it's time to do this properly.
This commit adds the new option to 'runto' which causes the
delete_breakpoints call to be skipped.
And, we now forward any arguments from 'runto_main' through to
'runto', this means I can now just do:
if {![runto_main no-delete-breakpoints]} {
return
}
which I think is cleaner and easier to understand.
I've updated the two tests I found that use the old with_override
approach.
There should be no change in what is tested after this commit.
Approved-By: Tom Tromey <tom@tromey.com>
|
|
While reviewing a patch I wanted to understand which blocks existed at
a given address.
The 'maint print symbols' command does provide some of this
information, but that command displays all blocks within a given
symtab. If I want to know which blocks are at a given address I have
to figure that out for myself based on the output of 'maint print
symbols' ... and I'm too lazy for that!
So this command lists just those blocks at a given address, along with
information about the blocks type. This new command doesn't list the
symbols within each block, for that my expectation is that you'd cross
reference the output with that of 'maint print symbols'.
The new command format is:
maintenance info blocks
maintenance info blocks ADDRESS
This lists the blocks at ADDRESS, or at the current $pc if ADDRESS is
not given. Blocks are listed starting at the global block, then the
static block, and then the progressively narrower scoped blocks.
For each block we list the internal block pointer (which allows easy
cross referencing with 'maint print symbols'), the inferior address
range, along with other useful information.
Reviewed-By: Eli Zaretskii <eliz@gnu.org>
Approved-By: Simon Marchi <simon.marchi@efficios.com>
|
|
While reviewing a patch I wanted to view GDB's inline frame state. I
don't believe there's currently a maintenance command to view this
information, so in this commit I've added one.
The new command is:
maintenance info inline-frames
maintenance info inline-frames ADDRESS
The command lists the inline frames that start at ADDRESS, or at the
current $pc if no ADDRESS is given. The command also displays the
"outer" function in which the inline functions are present.
An example of the command output:
(gdb) maintenance info inline-frames
Cached inline state information for thread 1.
program counter = 0x401137
skipped frames = 1
bar
> foo
main
(gdb)
This tells us that function 'main' called 'foo' which called 'bar'.
The functions 'foo' and 'bar' are both inline and both start at the
address 0x401137. Currently GDB considers the inferior to be stopped
in frame 'foo' (note the '>' marker), this means that there is 1
skipped frame (function 'bar').
The function 'main' is the outer function. The outer function might
not start at 0x401137, it is simply the function that contains the
inline functions.
If the user does a 'step' then GDB will not actually move the inferior
forward, but will instead simply tell the user that the inferior
entered 'bar'. The output of 'maint info inline-frames' will change
like this:
(gdb) step
bar () at inline.c:6
6 ++global_counter;
(gdb) maintenance info inline-frames
Cached inline state information for thread 1.
program counter = 0x401137
skipped frames = 0
> bar
foo
main
(gdb)
Now GDB is in function 'bar' and there are no skipped frames.
I have renamed skipped_symbols to function symbols within the
inline_state class. We are now going to carry the "outer"
function (the function that contains all the inlined functions) within
this list (as the last entry), so the old name didn't really make
sense. As a consequence of this rename I've updated some comments.
I've changed stopped_by_user_bp_inline_frame to take a symbol rather
than a block. Previously we just used the block to access the
associated function symbol. After this commit we can just pass in the
function symbol directly, so lets do that.
New function gather_inline_frames contains some of the logic pulled
from skip_inline_frames. This new function builds the list of all
symbols of inlined functions that start at a given $pc value and also
the "outer" function that contains all of the inlined functions.
In skip_inline_frames I've split the loop logic into two. The loop to
build the function symbol list has moved to gather_inline_frames. The
loop to figure out how many of the inlined functions we are skipping
remains in skip_inline_frames and uses the result of calling
gather_inline_frames.
In inline_skipped_symbol there are some minor updates to the comment,
and I've tweaked one of the asserts now that the function symbols list
also contains the "outer" function (a <= becomes <).
The maintenance_info_inline_frames function is now and implements the
new maintenance command.
And _initialize_inline_frame is updated to register the new command.
I've added a basic test for the new command. Please excuse the file
name for the new test, in the next commit I'll be adding additional
tests and at that point the file name will make sense.
Reviewed-By: Eli Zaretskii <eliz@gnu.org>
Approved-By: Simon Marchi <simon.marchi@efficios.com>
|
|
In commit b5070480d74 ("[gdb/symtab] Change DWARF_ERROR from Dwarf Error to
DWARF Error") I changed the dwarf error prefix, but failed to update test-case
gdb.dwarf2/dw2-inter-cu-error.exp.
Fix this by updating the corresponding regexp in the test-case.
Tested on x86_64-linux.
|
|
It was suggested here [1] that the canonical prefix for dwarf errors
should not be "Dwarf Error: ", given that the canonical spelling is DWARF
instead of Dwarf.
Fix this by using "DWARF Error: " instead.
Given the use of DWARF_ERROR_PREFIX, that needs to be changed only in a single
location.
Tested on x86_64-linux.
Suggested-By: Tom Tromey <tom@tromey.com>
Approved-By: Tom Tromey <tom@tromey.com>
[1] https://sourceware.org/pipermail/gdb-patches/2024-August/211258.html
|
|
Currently, gdbserver hangs after stdin is closed while it tries to
write: "Remote side has terminated connection. GDBserver will reopen
the connection." This hang disappears if --once is also given. Since
the stdin connection won't ever reopen if it's closed, it's safe to
assume --once is desired.
The gdb.server/server-pipe.exp test was also updated to reflect this
change. There is now a second disconnect at the end of the proc,
with a tighter-than-normal timeout to catch if the command hangs as
it used to.
Co-Authored-By: Guinevere Larsen <blarsen@redhat.com>
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=29796
Approved-By: Andrew Burgess <aburgess@redhat.com>
|
|
In a record session, when we move backward, GDB switches from normal
execution to simulation. Moving forward again, the emulation continues
until the end of the reverse history. When the end is reached, the
execution stops, and a warning message is shown. This message has been
modified to indicate that the forward emulation has reached the end, but
the execution can continue as normal, and the recording will also continue.
Before this patch, the warning message shown in that case was the same as
in the reverse case. This meant that when the end of history was reached in
either backward or forward emulation, the same message was displayed:
"No more reverse-execution history."
This message has changed for these two cases. Backward emulation:
"Reached end of recorded history; stopping.
Backward execution from here not possible."
Forward emulation:
"Reached end of recorded history; stopping.
Following forward execution will be added to history."
The reason for this change is that the initial message was deceiving, for
the forward case, making the user believe that forward debugging could not
continue.
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=31224
Reviewed-By: Markus T. Metzger <markus.t.metzger@intel.com> (btrace)
Approved-By: Guinevere Larsen <blarsen@redhat.com>
|
|
A corrupt debuginfo file can result in a null abbrev_info pointer
being passed to cooked_indexer::scan_attributes. This pointer
is set to nullptr by peek_die_abbrev when an abbrev of 0 is found.
There is no check for whether the abbrev pointer is null and
SIGSEGV occurs when attempting to dereference the pointer.
An abbrev of 0 normally indicates that the corresponding DIE is a
null entry, but scan_attributes expects a non-null DIE.
Fix this by throwing an error in cooked_indexer::scan_attributes
when peek_die_abbrev returns a nullptr in order to avoid
scan_attributes calling itself with a null abbrev.
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=31478
Co-authored-by: Tom de Vries <tdevries@suse.de>
Approved-By: Tom Tromey <tom@tromey.com>
|
|
cooked_indexer::ensure_cu_exists
With the test-case included in this patch, we run into:
...
$ gdb -q -batch $exec
Dwarf Error: Could not find abbrev number 3 in CU at offset 0xdb \
[in module $exec]
...
The debug info consists of two CUs:
...
Compilation Unit @ offset 0xb2:
Length: 0x25 (32-bit)
Version: 4
Abbrev Offset: 0x6c
Pointer Size: 8
<0><bd>: Abbrev Number: 1 (DW_TAG_compile_unit)
<be> DW_AT_language : 2 (non-ANSI C)
<1><bf>: Abbrev Number: 2 (DW_TAG_subprogram)
<c0> DW_AT_low_pc : 0x4004a7
<c8> DW_AT_high_pc : 0x4004b2
<d0> DW_AT_specification: <0xe8>
<1><d4>: Abbrev Number: 3 (DW_TAG_subprogram)
<d5> DW_AT_name : main
<1><da>: Abbrev Number: 0
Compilation Unit @ offset 0xdb:
Length: 0xf (32-bit)
Version: 4
Abbrev Offset: 0x86
Pointer Size: 8
<0><e6>: Abbrev Number: 1 (DW_TAG_compile_unit)
<e7> DW_AT_language : 2 (non-ANSI C)
<1><e8>: Abbrev Number: 2 (DW_TAG_subprogram)
<e9> DW_AT_specification: <0xd4>
<1><ed>: Abbrev Number: 0
...
where:
- DIE 0xbf in CU@0xb2 contains an inter-CU reference to
- DIE 0xe8 in CU@0xdb, which contains an inter-CU reference to
- DIE 0xd4 back in CU@0xb2.
The dwarf error is caused by this bit of code in
cooked_indexer::ensure_cu_exists:
...
if (per_cu == m_per_cu)
return reader;
...
The dwarf error happens as follows:
- a cutu_reader A is created for CU@0xb2
- using cutu_reader A, the cooked index reader starts indexing dies, with
m_per_cu set to CU@0xb2
- while indexing it scans the attributes of DIE 0xbf and encounters the
inter-CU reference to DIE 0xe8
- it calls cooked_indexer::ensure_cu_exists, which creates a cutu_reader B for
CU@0xdb and returns it
- using cutu_reader B, it continues scanning attributes of DIE 0xe8 and
encounters the inter-CU reference to DIE 0xd4
- it calls cooked_indexer::ensure_cu_exists, the problematic bit is triggered
and cutu_reader B is returned
- using cutu_reader B, it continues scanning attributes of DIE 0xd4
- this goes wrong because:
- the attributes of the DIE are encoded using the abbreviation table at
offset 0x6c, while
- the decoding is done using cutu_reader B which uses the abbreviation table
at offset 0x86.
Fix this by removing the problematic if clause.
Since cutu_reader A is not preserved in m_index_storage,
cooked_indexer::ensure_cu_exists cannot find it there and creates a duplicate
cutu_reader C for CU@0xb2. Fix this in process_psymtab_comp_unit by preserving
the cutu_reader A as well in m_index_storage.
Tested on x86_64-linux and aarch64-linux.
PR symtab/32081
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=32081
Approved-By: Tom Tromey <tom@tromey.com>
Reported-By: Andreas Schwab <schwab@linux-m68k.org>
|
|
It was pointed out in this email:
https://inbox.sourceware.org/gdb-patches/97973506-79f4-4216-9c0b-57401b3933f5@arm.com
that this commit:
commit 0726729d344fecf98f8d138e688e77201cc3cece
Date: Mon Jun 3 13:56:54 2024 +0100
gdb/testsuite: track if a caching proc calls gdb_exit or not
had broken some AArch64 tests.
What is going on is that there are two caching procs:
allow_aarch64_sme_tests
aarch64_initialize_sme_information
the allow_aarch64_sme_tests proc makes a call to
aarch64_initialize_sme_information, but
aarch64_initialize_sme_information is also called from other
non-caching procs, like aarch64_supports_sme_svl.
Both of the caching procs mentioned above compile and run a helper
program, and both of them call gdb_exit.
After the above commit, the first call to any caching proc, the body
of which calls gdb_exit, will result in a gdb_exit call even if the
body is not executed and the result is fetched from the cache.
What was observed is that in the first test script
allow_aarch64_sme_tests is called, the body of this caching proc is
run which calls gdb_exit. Then allow_aarch64_sme_tests calls
aarch64_initialize_sme_information, the body of which is run and
gdb_exit is called again. The results from both procs are added to
the cache.
In the next test script allow_aarch64_sme_tests is called. This
results in a cache hit, but gdb_exit is also called as this is the
first call in this second test script.
Later in the test script aarch64_supports_sme_svl is called which
calls aarch64_initialize_sme_information. As this is the first call
to aarch64_initialize_sme_information in this second test
script (remember the body of allow_aarch64_sme_tests was never run)
then gdb_exit is called. This call to gdb_exit is new after the above
commit and is unexpected.
I think the idea behind the above commit is still sound. If the call
to allow_aarch64_sme_tests was removed from the second test script
then we would want the extra gdb_exit call as this would expose a real
bug in the test. The problem is that after the above commit the
nested nature of the caching proc calls becomes important: a call to
allow_aarch64_sme_tests should mean that we've also called
aarch64_initialize_sme_information, and that relationship isn't
currently captured.
So in this commit I'm adding another field to the global
gdb_data_cache (in lib/cache.exp). This new field is 'also_called'.
For every caching proc we populate this field with a list of names,
these are the names of any nested caching procs that are called when
the body of a caching proc is executed.
Now when we get a cache hit in gdb_data_cache we mark every proc in
the 'also_called' list as having been called. This means that further
calls to these procs will no longer trigger a gdb_exit call.
Approved-By: Luis Machado <luis.machado@arm.com>
Tested-By: Luis Machado <luis.machado@arm.com>
|
|
When running test-case gdb.python/py-mi-cmd.exp with python 3.13, I run into:
...
Expecting: ^(-pycmd exp[^M
]+)?(.*&"Traceback \(most recent call last\):.."^M
&"[^^M
]+py-mi-cmd.py[^^M
]+"^M
&"[^^M
]+raise gdb.GdbError\(\).."^M
&"gdb.GdbError.."^M
\^error,msg="Error occurred in Python\."[^M
]+[(]gdb[)] ^M
[ ]*)
-pycmd exp^M
&"Traceback (most recent call last):\n"^M
&" File \"py-mi-cmd.py\", line 76, in invoke\n raise gdb.GdbError()\n"^M
&"gdb.GdbError\n"^M
^error,msg="Error occurred in Python."^M
(gdb) ^M
FAIL: gdb.python/py-mi-cmd.exp: -pycmd exp (unexpected output)
...
In contrast, with python 3.12 I have:
...
Expecting: ^(-pycmd exp[^M
]+)?(.*&"Traceback \(most recent call last\):.."^M
&"[^^M
]+py-mi-cmd.py[^^M
]+"^M
&"[^^M
]+raise gdb.GdbError\(\).."^M
&"gdb.GdbError.."^M
\^error,msg="Error occurred in Python\."[^M
]+[(]gdb[)] ^M
[ ]*)
-pycmd exp^M
&"Traceback (most recent call last):\n"^M
&" File \"py-mi-cmd.py\", line 76, in invoke\n"^M
&" raise gdb.GdbError()\n"^M
&"gdb.GdbError\n"^M
^error,msg="Error occurred in Python."^M
(gdb) ^M
PASS: gdb.python/py-mi-cmd.exp: -pycmd exp
...
To make it easier to understand what we're looking at, let's take this out of
the mi interpreter context and use the cli interpreter:
...
$ gdb -q -batch -ex "set trace-commands on" -x gdb.in
+set python print-stack full
+source py-mi-cmd.py
+python pycmd1('-pycmd')
+python pycmd1.invoke (pycmd1, ["exp"])
Traceback (most recent call last):
File "<string>", line 1, in <module>
File "py-mi-cmd.py", line 76, in invoke
raise gdb.GdbError()
gdb.GdbError
gdb.in:4: Error in sourced command file:
Error occurred in Python.
...
Interestingly, this is what we're seeing with both python 3.12 and 3.13.
The difference between the python versions is that:
- with python 3.12 each line is printed by itself, and
- with python 3.13 two particular lines are printed toghether.
With the cli interpreter, that makes no difference, because the '\n' is
interpreted.
But with the mi interpreter, that causes a difference in output because the
'\n' is not interpreted, but rather printed literally.
Fix this by accepting the new output in addition to the old one.
Tested on aarch64-linux.
Reviewed-by: Thiago Jung Bauermann <thiago.bauermann@linaro.org>
PR testsuite/31913
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=31913
|
|
gdb.ada/multiarray.exp has a loop that looks like it should run the
test in both 'all' and 'minimal' encodings mode. However, the body of
the loop doesn't actually use the 'flags' variable. This was an
oversight in the original commit.
|
