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>
Up to this point filename completion for possibly quoted filenames has
always been handled during the second (non-brkchars) phase of
completion. This works fine for commands that only want to complete
on a single filename argument.
In a later commit though I need to perform completion of a quoted
filename argument during the first (brkchars) phase of completion.
This will allow me to add a custom completer that completes both
command options and arguments for a command (remove-symbol-file) that
takes a possibly quoted filename argument.
This commit doesn't add the remove-symbol-file completer, this commit
is just about putting support for that in place.
To achieve this I've added the new function
advance_to_filename_maybe_quoted_complete_word_point, which is unused
in this commit. I've then had to extend some other functions in order
to extract the quoting state during the brkchars phase.
As this commit doesn't use the new functionality, the important thing
at this point is that I've not regressed the existing filename
completion (or any of the other completion). The next commit in this
series will make use of the new functionality, and will include
tests.
There should be no user visible changes after this commit.
This commit is in preparation for the next few commits, this commit
adds a new function extract_single_filename_arg.
This new function will be used to convert GDB commands that expect a
single filename argument to have these commands take a possibly quoted
or escaped string.
There's no use of the new function in this commit, for that see the
following commits.
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 commit moves the printing of the 'complete' command results out
of the 'complete_command' function. The printing is now done in a new
member function 'completion_result::print_matches'. At this point,
this is entirely a refactor.
The motivation for this refactor is how 'complete' should print the
completion of filename arguments. In some cases the filename results
need to have escaping added to the output. This escaping needs to be
done immediately prior to printing the result as adding too early will
result in multiple 'complete' results potentially being sorted
incorrectly. See the subsequent commits for more details.
There should be no user visible changes after this commit.
Approved-By: Tom Tromey <tom@tromey.com>
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.
Following on from the previous commit, this commit marks the old
unquoted filename completion related functions as deprecated.
The aim of doing this is to make it more obvious to someone adding a
new command that they should not be using the older unquoted style
filename argument handling.
I split this change from the previous to make for an easier review.
This commit touches more files, but is _just_ function renaming.
Check out gdb/completer.{c,h} for what has been renamed. All the
other files have just been updated to use the new names.
There should be no user visible changes after this commit.
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.
LoongArch: Not append rela for absolute symbol
Use la.global to get absolute symbol like la.abs.
la.global put address of a global symbol into a got
entry and append a rela for it, which will be used
to relocate by dynamic linker. Dynamic linker should
not relocate for got entry of absolute symbol as it
stores symval not symbol's address.
LoongArch: Add macros to get opcode and register of instructions appropriately
Currently, we get opcode of an instruction by manipulate the binary with
it's mask, it's a bit of a pain. Now a macro is defined to do this and a
macro to get the RD and RJ registers which is applicable to most instructions
of LoongArch are added.
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>
This patch changes "maint flush symbol-cache" to also flush the
Ada-specific symbol cache. This can be helpful when working on the
Ada code.
Approved-By: Tom de Vries <tdevries@suse.de>
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>
This patch initializes the "op" variable in skip_cfa_op() function
of bfd/elf-eh-frame.c to "0" at its declaration point to avoid the
"maybe-uninitialized" warning.
Building binutils on a system with GCC version 13.2.0 and a configure
command that sets the optimization level to "-Og" leads to a build
failure because of a warning being treated as an error:
---------------------------------------------------------------------
$ ./configure CFLAGS="-Og"
$ make
...
CC elf-eh-frame.lo
/src/gdb/bfd/elf-eh-frame.c: In function 'skip_cfa_op':
/src/gdb/bfd/elf-eh-frame.c:354:33: error: 'op' may be used
uninitialized [-Werror=maybe-uninitialized]
354 | switch (op & 0xc0 ? op & 0xc0 : op)
| ~~~~~~~~~~~~~~~~~~~~~~^~~~
/src/gdb/bfd/elf-eh-frame.c:348:12: note: 'op' was declared here
348 | bfd_byte op;
| ^~
cc1: all warnings being treated as errors
...
---------------------------------------------------------------------
The relevant code snippet related to this warning looks like:
---------------------------------------------------------------------
static inline bool
read_byte (bfd_byte **iter, bfd_byte *end, unsigned char *result)
{
if (*iter >= end)
return false;
*result = *((*iter)++);
return true;
}
static bool
skip_cfa_op (bfd_byte **iter, bfd_byte *end,...)
{
bfd_byte op;
if (!read_byte (iter, end, &op))
return false;
switch (op & 0xc0 ? op & 0xc0 : op)
...
}
---------------------------------------------------------------------
This warning probably happens because "-Og" results in GCC not
inlining the "read_byte()" function. Therefore, GCC treats its
invocation inside "skip_cfa_op()" like a black box and that ends
in the aforementioned warning.
Acknowledgement:
Lancelot Six -- for coming with the idea behind this fix.
Jan Beulich -- for reviewing.
bfd/ChangeLog:
* elf-eh-frame.c (skip_cfa_op): Initialize the "op" variable.
Along the lines of 2513312930 ("x86/APX: apply NDD-to-legacy
transformation to further CMOVcc forms") these can similarly be
converted to the shorter legacy-encoded CMOVcc.
We don't really want to align the last section's size to object
alignment (when that section may itself not be aligned as much), we want
image size to be a multiple thereof.
Since this commit:
commit a8651ef518
CommitDate: Fri Jun 14 14:47:38 2024 +0100
gdb/aarch64: prevent crash from in process agent
gdbserver isn't sending expedited registers with its stop reply packets
anymore. The problem is with how the constructor of the
expedited_registers std::vector is called:
The intent of the expedited_registers initialization in
aarch64_linux_read_description is to create a vector with capacity for 6
elements, but that's not how the std::vector constructor works.
Instead it creates a vector pre-populated with 6 elements initialized
with the default value for the type of the elements, and thus the first
6 elements are null pointers. The actual expedited registers are added
starting at the 7th element.
This causes init_target_desc to consider that the expedite_regs list is
empty, since it stops checking at the first nullptr element. The end
result is that gdbserver doesn't send any expedited registers to GDB in
its stop replies.
Fix by not specifying an element count when declaring the vector.
Tested for regressions on aarch64-linux-gnu native-extended-remote.
Approved-By: Andrew Burgess <aburgess@redhat.com>
Commit "b67a17aa7c0c478a" modified the logic of allocating dynamic
relocation space for TLS GD/IE, but only modified the logic of
generation dynamic relocations for TLS GD/IE in ABI v2.00. When
linking an object file of ABI v1.00 with bfd ld of ABI v2.00, it
will cause an assertion failure.
Modified the dynamic relocation generation logic of TLS GD/IE
in ABI v1.00 to be consistent with ABI v2.00.
I noticed that the comments for class parent_map aren't very clear.
This patch attempts to fix this, and also clarifies a point on
parent_map_map::add_map.
Approved-By: Simon Marchi <simon.marchi@efficios.com>
Fix formatting of a Python file added in commit:
commit a92e943014
Date: Wed Aug 14 15:16:46 2024 +0100
gdb: implement ::re_set method for catchpoint class
No functional change after this commit.
This syncs binutils-gdb/libiberty with gcc/libiberty up to GCC commit
64028d626a50410dbf29. This picks up the follow 3 GCC commits:
ea238096883 (gcc-delete-unused-func) libiberty/argv.c: remove only_whitespace
5e1d530da87 (gcc-buildargv) libiberty/buildargv: handle input consisting of only white space
a87954610f5 libiberty/buildargv: POSIX behaviour for backslash handling
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>
We recently fixed a bug in libgcc
(https://gcc.gnu.org/bugzilla/show_bug.cgi?id=115360)
where a symbol was missing a %function .type decoration.
This meant the linker would silently pick the wrong type of 'farcall
stub', involving Arm-mode instructions on Thumb-only CPUs.
This patch emits an error instead, and warns in some other cases, to
encourage users to add the missing '.type foo,%function' directive.
In practice: in arm_type_of_stub() we no longer try to infer which
stub to use if the destination is of unknown type and the CPU is
Thumb-only; so we won't lie to elf32_arm_size_stubs() which does not
check branch_type.
If branch_type is ST_BRANCH_TO_ARM but the CPU is Thumb-only, we now
convert it to ST_BRANCH_TO_THUMB only if the destination is of
absolute type. This is to support the case where the destination of
the branch is defined by the linker script (see thumb-b-lks-sym.s and
thumb-bl-lks-sym.s testcases for instance).
The motivating case is covered by the new farcall-missing-type
testcase, where we now emit an error message. We do not emit an error
when branch_type is ST_BRANCH_UNKNOWN and the CPU supports Arm-mode: a
lot of legacy code (e.g. newlib's crt0.S) lacks the corresponding
'.type foo, %function' directives and even a (too verbose) warning
could be perceived as a nuisance.
Existing testcases where such a warning would trigger:
arm-static-app.s (app_func, app_func2)
arm-rel32.s (foo)
arm-app.s (app_func)
rel32-reject.s () main)
fix-arm1176.s (func_to_branch_to)
but this list is not exhaustive.
For the sake of clarity, the patch replaces occurrences of
sym.st_target_internal = 0; with
sym.st_target_internal = ST_BRANCH_TO_ARM;
enum arm_st_branch_type is defined in include/elf/arm.h, and relies on
ST_BRANCH_TO_ARM==0, as sym.st_target_internal is also initialized to
0 in other target-independent parts of BFD code. (For instance,
swapping the ST_BRANCH_TO_ARM and ST_BRANCH_TO_THUMB entries in the
enum definition leads to 'interesting' results...)
Regarding the testsuite:
* new expected warning for thumb-b-lks-sym and thumb-bl-lks-sym
* new testcase farcall-missing-type to check the new error case
* attr-merge-arch-2b.s, branch-futures (and bfs-1.s) updated to avoid
a diagnostic
Tested on arm-eabi and arm-pe with no regression.
This script is a copy of the current script used by Sourceware's
autoregen buildbots.
It is intended as a helper to regenerate files managed by autotools
(autoconf, automake, aclocal, ....), as well as the toplevel
Makefile.in which is created by autogen.
Other files can be updated when using maintainer-mode, but this is not
covered by this script.
2024-04-19 Christophe Lyon <christophe.lyon@linaro.org>
contrib/
* autoregen.py: New script.
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.
MSVC 2022 is more pedantic than MSVC 2019 when it comes to loading PDB
files in readonly mode, and was rejecting PDB files generated by binutils
because of their invalid free-space bitmaps. It's unknown what would
have happened if you tried to use MS tools to modify a PDB created by
binutils, but it probably would have led to a corrupted file.
This patch fixes pdb_write_bitmap so we generate files that MSVC will accept.
Specifically there were three things we were doing wrong:
- We weren't including the superblock (block 0)
- We were setting bits in bytes backwards (MSB to LSB, rather than LSB to MSB)
- We should have been marking the contents of stream 0 as free. This is
because, as the comment says, it's intended to be used for the
directory for the previous write, to allow atomic updates.
