2021-10-06 Philipp Tomsich <philipp.tomsich@vrull.eu>
bfd/
* elfxx-riscv.c (riscv_supported_std_z_ext): Update the version
number for zba, zbb and zbc to 1.0.0
Signed-off-by: Philipp Tomsich <philipp.tomsich@vrull.eu>
Version-changes: 3
- Updated version numbers for zba, zbb and zbc to 1.0.0
The Zb[abc] opcodes are bundled just below the Privileged opcodes in
riscv_opcodes, possibly giving the appearance that they are part of
the Privileged spec for an uninitiated reader. This separates them
out and adds comments above each section to clearly identify them as
Zba, Zbb or Zbc opcodes.
2021-10-04 Philipp Tomsich <philipp.tomsich@vrull.eu>
opcodes/
* riscv-opc.c: Split of Zb[abc] instructions and add comments.
Signed-off-by: Philipp Tomsich <philipp.tomsich@vrull.eu>
XCOFF archives use a bi-directional linked list for file members. So
one member points to both the previous member and the next member.
Members may not be sequentially ordered in the file. This of course
is over-engineered nonsense and an attractive target for fuzzers.
(There is even a free list of members!) The testcase in PR28423 is an
XCOFF archive with one member pointing to itself, which results in
lots of bad behaviour. For example, "ar t" never terminates.
The use-after-free with "objdump -r" happens like this: The first
archive element is opened, its symbols are read and "canonicalized"
for objdump, then relocations are read and printed. Those relocations
use the canonicalized symbols, and also happen to be cached by the
coff bfd backend support. objdump frees the symbols. The next
archive element is then opened. This must be done before the first
element is closed, because finding the next element uses data held in
the currect element. Unfortunately the next element happens to be the
original, so we aren't opening, we're reopening a bfd which has cached
data. When the relocations are printed they use the cached copy
containing references to the freed canonical symbols.
This patch adds a little sanity checking to the XCOFF "open next
archive file" support, so that it rejects archive members pointing at
themselves. That is sufficient to cure this problem. Anything more
is overkill. If someone deliberately fuzzes an XCOFF archive with an
element loop then reports an "ar" bug when it runs forever, they will
find their bug report closed WONTFIX.
PR 28423
* coff-rs6000.c (_bfd_xcoff_read_ar_hdr): Save size occupied
by member name in areltdata.extra_size.
(_bfd_xcoff_openr_next_archived_file): Sanity check nextoff.
* coff64-rs6000.c (xcoff64_openr_next_archived_file): Call
_bfd_xcoff_openr_next_archived_file.
This fixes a bug in commit 5d9bbb73c1. All fields preserved from a
bfd in struct bfd_preserve need to be cleared in bfd_reinit.
PR 28422
* format.c (bfd_reinit): Clear build_id.
Rather than reporting "memory exhausted", report "file truncated".
You can hit this error on small fuzzed object files, or on files that
are actually truncated. In either case sizes can be such that an out
of memory error is a little confusing.
* compress.c (bfd_get_full_section_contents): Set
bfd_error_file_truncated rather than bfd_error_no_memory when
section size exceeds file size.
On openSUSE tumbleweed I run into:
...
FAIL: gdb.base/annota1.exp: run until main breakpoint (timeout)
...
due to a message related to libthread_db:
...
^Z^Zstarting^M
[Thread debugging using libthread_db enabled]^M
Using host libthread_db library "/lib64/libthread_db.so.1".^M
^M
^Z^Zframes-invalid^M
...
which is not matched by the regexp.
Fix this by updating the regexp.
Tested on x86_64-linux.
The documentation for 'show print elements' contains the line:
If the number is 0, then the printing is unlimited.
However, this line is now out of date as can be seen by this GDB
session:
(gdb) set print elements 0
(gdb) show print elements
Limit on string chars or array elements to print is unlimited.
The value 0 does indeed mean unlimited, and this is described in the
'set print elements' section, however, for 'show print elements' the
user will never see the value 0, so lets just remove that bit from the
docs.
When running test-case gdb.tui/corefile-run.exp on openSUSE Tumbleweed,
I run into:
...
PASS: gdb.tui/corefile-run.exp: load corefile
FAIL: gdb.tui/corefile-run.exp: run until the end
...
What's going on is easier to see when also doing dump_screen if
check_contents passes, and inspecting state at the preceding PASS:
...
+-------------------------------------------------------------------------+
exec No process In: L?? PC: ??
[New LWP 16629]
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/lib64/libthread_db.so.1".
Core was generated by `/data/gdb_versions/devel/build/gdb/testsuite/output
s/gdb.tui/corefile-run/corefi'.
Program terminated with signal SIGTRAP, Trace/breakpoint trap.
#0 main ()
--Type <RET> for more, q to quit, c to continue without paging--
...
The problem is that we're getting a pagination prompt, and the subsequent run
command is interpreted as an answer to that prompt.
Fix this by:
- detecting the gdb prompt in response to "load corefile", such that
we detect the failure earlier, and
- doing a "set pagination off" in Term::clean_restart.
Tested on x86_64-linux.
This fixes a situation where the COFF code allocated memory for
internal representaion arrays before reading the external file data.
That meant the allocation didn't have any sanity check against file
size.
PR 28402
* coffcode.h (buy_and_read): Malloc rather than alloc memory.
(coff_slurp_line_table): Read native line number info before
allocating memory for internal line number array. Adjust error
paths to suit. Remove now unnecessary line number count check.
(coff_slurp_reloc_table): Adjust to suit buy_and_read change.
The PR28401 testcase has a section named "", ie. an empty string.
This results in some silly behaviour in load_debug_section, and
dump_dwarf_section. Fix that. Note that this patch doesn't correct
the main complaint in PR28401, "failed to allocate", since malloc
failures on sections having huge bogus sizes are to be expected. We
can't safely catch all such cases by comparing with file size, for
example, where sections contain compressed data.
PR 28401
* objdump.c (load_debug_section): Don't attempt to retrieve
empty name sections.
(dump_dwarf_section): Likewise.
Currently, tui testing is rather verbose. When using these RUNTESTFLAGS to
pick up all tui tests (17 in total):
...
rtf=$(echo $(cd src/gdb/testsuite/; find gdb.* -type f -name *.exp* \
| xargs grep -l tuiterm_env) )
...
we have:
...
$ wc -l gdb.log
120592 gdb.log
...
Most of the output is related to controlling the tui screen, but that does
not give a top-level sense of how the test-case progresses.
Put differently: a lot of bandwith is used to describe how we arrive at a
certain tui screen state. But we don't actually always show the state we
arrive at, unless there's a FAIL.
And if there's say, a PASS that should actually be FAILing, it's hard to
detect.
Fix this by:
- dropping the -log on the call to verbose in _log. We still can get the
same info back using runtest -v.
- dumping the screen or box that we're checking, also when the test passes.
Brings down verbosity to something more reasonable:
...
$ wc -l gdb.log
3221 gdb.log
...
Tested on x86_64-linux.
The commit 702991711a (gdb: Have setter
and getter callbacks for settings) makes it possible for a setting not
to be backed by a memory buffer but use callback functions instead to
retrieve or set the setting's value.
An assertion was not properly updated to take into account that the
m_var member (which points to a memory buffer, if used) might be nullptr
if the setting uses callback functions. If the setting is backed by a
memory buffer, the m_var has to be non nullptr, which is already checked
before the pointer is dereferenced.
This commit removes this assertion as it is not valid anymore.
This makes the Ada-specific "varsize-limit" a synonym for
"max-value-size", and removes the Ada-specific checks of the limit.
I am not certain of the history here, but it seems to me that this
code is fully obsolete now. And, removing this makes it possible to
index large Ada arrays without triggering an error. A new test case
is included to demonstrate this.
This changes value_zero to create a lazy value. In many cases,
value_zero is called in expression evaluation to wrap a type in a
non-eval context. It seems senseless to allocate a buffer in these
cases.
A new 'is_zero' flag is added so we can preserve the existing
assertions in value_fetch_lazy.
A subsequent patch will add a test where creating a zero value would
fail, due to the variable size check. However, the contents of this
value are never needed, and so creating a lazy value avoids the error
case.
This adds an is_optimized_out function pointer to lval_funcs, and
changes value_optimized_out to call it. This new function lets gdb
determine if a value is optimized out without necessarily fetching the
value. This is needed for a subsequent patch, where an attempt to
access a lazy value would fail due to the value size limit -- however,
the access was only needed to determine the optimized-out state.
Since commit e36788d135 "[gdb/testsuite] Fix handling of nr_args < 3 in
mi_gdb_test" we run into:
...
PASS: gdb.mi/mi-nsmoribund.exp: print done = 1
Expecting: ^(.*[^M
]+)?([^
]*^M
\*running,thread-id="[0-9]+"^M
\*running,thread-id="[0-9]+"^M
\*running,thread-id="[0-9]+"^M
\*running,thread-id="[0-9]+"^M
\*running,thread-id="[0-9]+"^M
\*running,thread-id="[0-9]+"^M
\*running,thread-id="[0-9]+"^M
\*running,thread-id="[0-9]+"^M
\*running,thread-id="[0-9]+"^M
\*running,thread-id="[0-9]+"[^M
]+[(]gdb[)] ^M
[ ]*)
103-exec-continue --all^M
=library-loaded,id="/lib64/libgcc_s.so.1",target-name="/lib64/libgcc_s.so.1",\
host-name="/lib64/libgcc_s.so.1",symbols-loaded="0",thread-group="i1",\
ranges=[{from="0x00007ffff22a5010",to="0x00007ffff22b6365"}]^M
103^running^M
*running,thread-id="5"^M
(gdb) ^M
FAIL: gdb.mi/mi-nsmoribund.exp: 103-exec-continue --all (unexpected output)
...
The regexp expect running messages for all threads, but we only get one for
thread 5.
The test-case uses non-stop mode, and when the exec-continue --all command is
issued, thread 5 is stopped and all other threads are running. Consequently,
only thread 5 is resumed, and reported as running.
Fix this by updating the regexp.
Tested on x86_64-linux.
When a user creates a gdb.Inferior object for the first time a new
Python object is created. This object is then cached within GDB's
inferior object using the registry mechanism (see
inferior_to_inferior_object in py-inferior.c, specifically the calls
to inferior_data and set_inferior_data).
The Python Reference to the gdb.Inferior object held within the real
inferior object ensures that the reference count on the Python
gdb.Inferior object never reaches zero while the GDB inferior object
continues to exist.
At the same time, the gdb.Inferior object maintains a C++ pointer back
to GDB's real inferior object. We therefore end up with a system that
looks like this:
Python Reference
|
|
.----------. | .--------------.
| |------------------->| |
| inferior | | gdb.Inferior |
| |<-------------------| |
'----------' | '--------------'
|
|
C++ Pointer
When GDB's inferior object is deleted (say the inferior exits) then
py_free_inferior is called (thanks to the registry system), this
function looks up the Python gdb.Inferior object and sets the C++
pointer to nullptr and finally reduces the reference count on the
Python gdb.Inferior object.
If at this point the user still holds a reference to the Python
gdb.Inferior object then nothing happens. However, the gdb.Inferior
object is now in the non-valid state (see infpy_is_valid in
py-inferior.c), but otherwise, everything is fine.
However, if there are no further references to the Python gdb.Inferior
object, or, once the user has given up all their references to the
gdb.Inferior object, then infpy_dealloc is called.
This function currently checks to see if the inferior pointer within
the gdb.Inferior object is nullptr or not. If the pointer is nullptr
then infpy_dealloc immediately returns.
Only when the inferior point in the gdb.Inferior is not nullptr do
we (a) set the gdb.Inferior reference inside GDB's inferior to
nullptr, and (b) call the underlying Python tp_free function.
There are a number things wrong here:
1. The Python gdb.Inferior reference within GDB's inferior object
holds a reference count, thus, setting this reference to nullptr
without first decrementing the reference count would leak a
reference, however...
2. As GDB's inferior holds a reference then infpy_dealloc will never
be called until GDB's inferior object is deleted. Deleting a GDB
inferior ohject calls py_free_inferior, and so gives up the
reference. At this point there is no longer a need to call
set_inferior_data to set the field back to NULL, that field must
have been cleared in order to get the reference count to zero, which
means...
3. If we know that py_free_inferior must be called before
infpy_dealloc, then we know that the inferior pointer in
gdb.Inferior will always be nullptr when infpy_dealloc is called,
this means that the call to the underlying tp_free function will
always be skipped. Skipping this call will cause Python to leak the
memory associated with the gdb.Inferior object, which is what we
currently always do.
Given all of the above, I assert that the C++ pointer within
gdb.Inferior will always be nullptr when infpy_dealloc is called.
That's what this patch does.
I wrote a test for this issue making use of Pythons tracemalloc
module, which allows us to spot this memory leak.
Following 2 test points are failing with clang compiler
(gdb) FAIL: gdb.dwarf2/dw2-ref-missing-frame.exp: func_nofb print
(gdb) FAIL: gdb.dwarf2/dw2-ref-missing-frame.exp: func_loopfb print
As in commit f677852bbd "[gdb/testsuite] Use function_range in
gdb.dwarf2/dw2-abs-hi-pc.exp", the problem is that the CU and functions
have an empty address range, due to using asm labels in global scope,
which is a known source of problems, as explained in the comment of proc
function_range in gdb/testsuite/lib/dwarf.exp. Hence fix this also by
using function_range.
Tested on x86_64-linux with gcc and clang.
Add a new event, gdb.events.gdb_exiting, which is called once GDB
decides it is going to exit.
This event is not triggered in the case that GDB performs a hard
abort, for example, when handling an internal error and the user
decides to quit the debug session, or if GDB hits an unexpected,
fatal, signal.
This event is triggered if the user just types 'quit' at the command
prompt, or if GDB is run with '-batch' and has processed all of the
required commands.
The new event type is gdb.GdbExitingEvent, and it has a single
attribute exit_code, which is the value that GDB is about to exit
with.
The event is triggered before GDB starts dismantling any of its own
internal state, so, my expectation is that most Python calls should
work just fine at this point.
When considering this functionality I wondered about using the
'atexit' Python module. However, this is triggered when the Python
environment is shut down, which is done from a final cleanup. At
this point we don't know for sure what other GDB state has already
been cleaned up.
The test gdb.python/py-events.exp sets up a handler for the gdb.exited
event. Unfortunately the handler is slightly broken, it assumes that
the exit_code attribute will always be present. This is not always
the case.
In a later commit I am going to add more tests to py-events.exp test
script, and in so doing I expose the bug in our handling of gdb.exited
events.
Just to be clear, GDB itself is fine, it is the test that is not
written correctly according to the Python Events API.
So, in this commit I fix the Python code in the test, and extend the
test case to exercise more paths through the Python code.
Additionally, I noticed that the gdb.exited event is used as an
example in the documentation for how to write an event handler.
Unfortunately the same bug that we had in our test was also present in
the example code in the manual.
So I've fixed that too.
After this commit there is no functional change to GDB.
I noticed that some methods in language_defn could use
unique_xmalloc_ptr<char> rather than a plain 'char *'. This patch
implements this change, fixing up the fallout and changing
gdb_demangle to also return this type. In one spot, std::string is
used to simplify some related code, and in another, an auto_obstack is
used to avoid manual management.
Regression tested on x86-64 Fedora 34.
Say we use a gcc version that (while supporting c++11) does not support c++11
by default, and needs an -std setting to enable it.
If gdb would use the default AX_CXX_COMPILE_STDCXX from autoconf-archive, then
we'd have:
...
CXX="g++ -std=gnu++11"
...
That mechanism however has the following problem (quoting from commit
0bcda68539):
...
the top level Makefile passes CXX down to subdirs, and that overrides whatever
gdb/Makefile may set CXX to. The result would be that a make invocation from
the build/gdb/ directory would use "g++ -std=gnu++11" as expected, while a
make invocation at the top level would not.
...
Commit 0bcda68539 fixes this by using a custom AX_CXX_COMPILE_STDCXX which
does:
...
CXX=g++
CXX_DIALECT=-std=gnu++11
...
The problem reported in PR28318 is that using the custom instead of the
default AX_CXX_COMPILE_STDCXX makes the configure test for std::thread
support fail.
We could simply add $CXX_DIALECT to the test for std::thread support, but
that would have to be repeated for each added c++ support test.
Instead, fix this by doing:
...
CXX="g++ -std=gnu++11"
CXX_DIALECT=-std=gnu++11
...
This is somewhat awkward, since it results in -std=gnu++11 occuring twice in
some situations:
...
$ touch src/gdb/dwarf2/read.c
$ ( cd build/gdb; make V=1 dwarf2/read.o )
g++-4.8 -std=gnu++11 -x c++ -std=gnu++11 ...
...
However, both settings are needed:
- the switch in CXX for the std::thread tests (and other tests)
- the switch in CXX_DIALECT so it can be appended in Makefiles, to
counteract the fact that the top-level Makefile overrides CXX
The code added in gdb/ax_cxx_compile_stdcxx.m4 is copied from the default
AX_CXX_COMPILE_STDCXX from autoconf-archive.
Tested on x86_64-linux.
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=28318
Consider test-case gdb.trace/entry-values.exp with target board
unix/-fPIE/-pie.
Using this command we have an abbreviated version, and can see the correct
@entry values for foo:
...
$ gdb -q -batch outputs/gdb.trace/entry-values/entry-values \
-ex start \
-ex "break foo" \
-ex "set print entry-values both" \
-ex continue
Temporary breakpoint 1 at 0x679
Temporary breakpoint 1, 0x0000555555554679 in main ()
Breakpoint 2 at 0x55555555463e
Breakpoint 2, 0x000055555555463e in foo (i=0, i@entry=2, j=2, j@entry=3)
...
Now, let's try the same again, but run directly to foo rather than stopping at
main:
...
$ gdb -q -batch outputs/gdb.trace/entry-values/entry-values \
-ex "break foo" \
-ex "set print entry-values both" \
-ex run
Breakpoint 1 at 0x63e
Breakpoint 1, 0x000055555555463e in foo (i=0, i@entry=<optimized out>, \
j=2, j@entry=<optimized out>)
...
So, what explains the difference? Noteworthy, this is a dwarf assembly
test-case, with debug info for foo and bar, but not for main.
In the first case:
- we run to main
- this does not trigger expanding debug info, because there's none for main
- we set a breakpoint at foo
- this triggers expanding debug info. Relocated addresses are used in
call_site info (because the exec is started)
- we continue to foo, and manage to find the call_site info
In the second case:
- we set a breakpoint at foo
- this triggers expanding debug info. Unrelocated addresses are used in
call_site info (because the exec is not started)
- we run to foo
- this triggers objfile_relocate1, but it doesn't update the call_site
info addresses
- we don't manage to find the call_site info
We could fix this by adding the missing call_site relocation in
objfile_relocate1.
This solution however is counter-trend in the sense that we're trying to
work towards the situation where when starting two instances of an executable,
we need only one instance of debug information, implying the use of
unrelocated addresses.
So, fix this instead by using unrelocated addresses in call_site info.
Tested on x86_64-linux.
This fixes all remaining unix/-fno-PIE/-no-pie vs unix/-fPIE/-pie
regressions, like f.i. PR24892.
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=24892
Co-Authored-By: Tom de Vries <tdevries@suse.de>
In commit b4c919f752 "[gdb/symtab] Fix htab_find_slot call in
read_call_site_scope" , I removed the comment:
...
It must be the first field as we overload core_addr_hash and core_addr_eq for
it.
...
for field pc of struct call_site.
However, this was not tested, and when indeed moving field pc to the second
location, we run into a testsuite failure in gdb.trace/entry-values.exp.
This is caused by core_addr_eq (the eq_f function for the htab) being
called with a pointer to the pc field (as passed into htab_find_slot) and a
pointer to a hash table element. Now that pc is no longer the first field,
the pointer to hash table element no longer points to the pc field.
This could be fixed by simply reinstating the comment, but we're trying to
get rid of this kind of tricks that make refactoring more difficult.
Instead, fix this by:
- reverting commit b4c919f752, apart from the comment removal, such that
we're passing a pointer to element to htab_find_slot
- updating the htab_find_slot call in compunit_symtab::find_call_site
in a similar manner
- adding a call_site_eq and call_site_hash, and using these in the hash table
instead of core_addr_eq and core_addr_hash.
Tested on x86_64-linux, both with and without a trigger patch that moves pc to
the second location in struct call_site.
The change "make string-like set show commands use std::string
variable" caused remote-sim.c to fail to build. The issue is that the
code does:
const std::string &sysroot = gdb_sysroot;
if (is_target_filename (sysroot))
sysroot += strlen (TARGET_SYSROOT_PREFIX);
... which isn't valid.
This patch changes this code to use a 'const char *' again, fixing the
build.
In the gdbsupport configure.ac file, there is an attempt to define
TARGET_WORD_SIZE. This is done by running grep on the file
../bfd/bfd-in3.h.
The problem with this is, the file bfd-in3.h is generated into the bfd
build directory when bfd is configured, and there is no dependency
between the gdbsupport module and the bfd module, so, for example, if
I do:
$ ../src/configure
$ make all-gdbsupport
Then bfd will neither be configured, or built. In this case
TARGET_WORD_SIZE ends up being defined, but with no value because the
grep on bfd-in3.h fails.
However, it turns out that this doesn't matter; we don't actually use
TARGET_WORD_SIZE anywhere.
My proposal in this commit is to just remove the definition of
TARGET_WORD_SIZE, the alternative would be to add a dependency between
configure-gdbsupport and configure-bfd into Makefile.def, but adding a
dependency for something we don't need seems pretty pointless.
It can be difficult to guess the exact bfd name, so add an option to
list all the targets that the current build supports. This aligns with
other simulator options like --info-architecture.
GDB can notify observers when a parameter is changed.
To do that, do_set_command (in gdb/cli/cli-setshow.c) compares the new
value against the old one before updating it, and based on that notifies
observers. This looks like something like:
int valuechanged = 0;
switch (cmd->var.type ())
{
case var_integer:
{
LONGEST new_val = parse_and_eval_long (arg)
if (new_val != cmd->var.get<int> ())
{
cmd->var.get<int> (new_val);
value_changes = 1;
}
}
case var_uinteger:
case var_zuinteger:
{
unsigned int val
= parse_cli_var_uinteger (c->var->type (), &arg, true);
if (c->var->get<unsigned int> () != val)
{
c->var->set<unsigned int> (val);
option_changed = true;
}
}
case...
/* And so on for all possible var_types. */
}
This comparison is done for each possible var_type, which leads to
unnecessary logic duplication.
In this patch I propose to move all those checks in one place within the
setting setter method. This limits the code duplication and simplifies
the do_set_command implementation.
This patch also changes slightly the way a value change is detected.
Instead of comparing the user provided value against the current value
of the setting, we compare the value of the setting before and after the
set operation. This is meant to handle edge cases where trying to set
an unrecognized value would be equivalent to a noop (the actual value
remains unchanged). Doing this requires that the original value needs
to be copied before the update, which can be non trivial for
std::string.
There should be no user visible change introduced by this commit.
Tested on x86_64 GNU/Linux.
[1] https://review.lttng.org/c/binutils-gdb/+/5831/41
Change-Id: If064b9cede3eb56275aacd2b286f74eceb1aed11
The main motivation behind this improvement is to help the
implementation of a patch Simon Marchi is preparing to fix a bug when
MI or Python try to access parameters that are inferior dependent (see
PR/28085).
This commit extends the previous ones, which introduces the setting
object to represent a static variable whose value can be set or shown
with the appropriate commands. This patch proposes that a setting can
either contain a pointer to a static variable holding a setting, or
pointers to a pair of setter and getter callback functions.
The callbacks functions can be used to retrieve or change the value with
custom logic. This is useful when the source of truth for a given
setting is not contained in the variable pointed to by the setting
instance.
Given that the callback function call is hidden within the setting
abstraction introduced earlier, none of the sites accessing the setting
needs to be updated. The registered getter or setter is used whatever
the way to access it is (through MI, Python, Guile, the "with" command
and the $_gdb_setting / $_gdb_setting_str convenience functions).
All the add_setshow_*_cmd are given a new overload that will accept the
pair of function pointers (set / get functions) instead of the pointer
to a global variable.
Tested on GNU/Linux x86_64 with no regression observed.
Change-Id: Ieb81fef57550632ff66e6aa85f637372a226be8c
Co-authored-by: Simon Marchi <simon.marchi@polymtl.ca>
String-like settings (var_string, var_filename, var_optional_filename,
var_string_noescape) currently take a pointer to a `char *` storage
variable (typically global) that holds the setting's value. I'd like to
"mordernize" this by changing them to use an std::string for storage.
An obvious reason is that string operations on std::string are often
easier to write than with C strings. And they avoid having to do any
manual memory management.
Another interesting reason is that, with `char *`, nullptr and an empty
string often both have the same meaning of "no value". String settings
are initially nullptr (unless initialized otherwise). But when doing
"set foo" (where `foo` is a string setting), the setting now points to
an empty string. For example, solib_search_path is nullptr at startup,
but points to an empty string after doing "set solib-search-path". This
leads to some code that needs to check for both to check for "no value".
Or some code that converts back and forth between NULL and "" when
getting or setting the value. I find this very error-prone, because it
is very easy to forget one or the other. With std::string, we at least
know that the variable is not "NULL". There is only one way of
representing an empty string setting, that is with an empty string.
I was wondering whether the distinction between NULL and "" would be
important for some setting, but it doesn't seem so. If that ever
happens, it would be more C++-y and self-descriptive to use
optional<string> anyway.
Actually, there's one spot where this distinction mattered, it's in
init_history, for the test gdb.base/gdbinit-history.exp. init_history
sets the history filename to the default ".gdb_history" if it sees that
the setting was never set - if history_filename is nullptr. If
history_filename is an empty string, it means the setting was explicitly
cleared, so it leaves it as-is. With the change to std::string, this
distinction doesn't exist anymore. This can be fixed by moving the code
that chooses a good default value for history_filename to
_initialize_top. This is ran before -ex commands are processed, so an
-ex command can then clear that value if needed (what
gdb.base/gdbinit-history.exp tests).
Another small improvement, in my opinion is that we can now easily
give string parameters initial values, by simply initializing the global
variables, instead of xstrdup-ing it in the _initialize function.
In Python and Guile, when registering a string-like parameter, we
allocate (with new) an std::string that is owned by the param_smob (in
Guile) and the parmpy_object (in Python) objects.
This patch started by changing all relevant add_setshow_* commands to
take an `std::string *` instead of a `char **` and fixing everything
that failed to build. That includes of course all string setting
variable and their uses.
string_option_def now uses an std::string also, because there's a
connection between options and settings (see
add_setshow_cmds_for_options).
The add_path function in source.c is really complex and twisted, I'd
rather not try to change it to work on an std::string right now.
Instead, I added an overload that copies the std:string to a `char *`
and back. This means more copying, but this is not used in a hot path
at all, so I think it is acceptable.
Change-Id: I92c50a1bdd8307141cdbacb388248e4e4fc08c93
Co-authored-by: Lancelot SIX <lsix@lancelotsix.com>
cmd_list_element can contain a pointer to data that can be set and / or
shown. This is achieved with the void* VAR member which points to the
data that can be accessed, while the VAR_TYPE member (of type enum
var_types) indicates how to interpret the data pointed to.
With this pattern, the user of the cmd_list_element needs to know what
is the storage type associated with a given VAR_TYPES in order to do
the proper casting. No automatic safeguard is available to prevent
miss-use of the pointer. Client code typically looks something like:
switch (c->var_type)
{
case var_zuinteger:
unsigned int v = *(unsigned int*) c->var;
...
break;
case var_boolean:
bool v = *(bool *) c->var;
...
break;
...
}
This patch proposes to add an abstraction around the var_types and void*
pointer pair. The abstraction is meant to prevent the user from having
to handle the cast and verify that the data is read or written as a type
that is coherent with the setting's var_type. This is achieved by
introducing the struct setting which exposes a set of templated get /
set member functions. The template parameter is the type of the
variable that holds the referred variable.
Using those accessors allows runtime checks to be inserted in order to
ensure that the data pointed to has the expected type. For example,
instantiating the member functions with bool will yield something
similar to:
const bool &get<bool> () const
{
gdb_assert (m_var_type == var_boolean);
gdb_assert (m_var != nullptr);
return *static_cast<bool *> (m_var);
}
void set<bool> (const bool &var)
{
gdb_assert (m_var_type == var_boolean);
gdb_assert (m_var != nullptr);
*static_cast<bool *> (m_var) = var;
}
Using the new abstraction, our initial example becomes:
switch (c->var_type)
{
case var_zuinteger:
unsigned int v = c->var->get<unsigned int> ();
...
break;
case var_boolean:
bool v = c->var->get<bool> ();
...
break;
...
}
While the call site is still similar, the introduction of runtime checks
help ensure correct usage of the data.
In order to avoid turning the bulk of add_setshow_cmd_full into a
templated function, and following a suggestion from Pedro Alves, a
setting can be constructed from a pre validated type erased reference to
a variable. This is what setting::erased_args is used for.
Introducing an opaque abstraction to describe a setting will also make
it possible to use callbacks to retrieve or set the value of the setting
on the fly instead of pointing to a static chunk of memory. This will
be done added in a later commit.
Given that a cmd_list_element may or may not reference a setting, the
VAR and VAR_TYPES members of the struct are replaced with a
gdb::optional<setting> named VAR.
Few internal function signatures have been modified to take into account
this new abstraction:
-The functions value_from_setting, str_value_from_setting and
get_setshow_command_value_string used to have a 'cmd_list_element *'
parameter but only used it for the VAR and VAR_TYPE member. They now
take a 'const setting &' parameter instead.
- Similarly, the 'void *' and a 'enum var_types' parameters of
pascm_param_value and gdbpy_parameter_value have been replaced with a
'const setting &' parameter.
No user visible change is expected after this patch.
Tested on GNU/Linux x86_64, with no regression noticed.
Co-authored-by: Simon Marchi <simon.marchi@polymtl.ca>
Change-Id: Ie1d08c3ceb8b30b3d7bf1efe036eb8acffcd2f34
We map target signals to host signals so we can propagate signals
between the host & simulated worlds. That means we need to know
the symbolic names & values of all signals that might be sent.
The tools that generate that list use signal.h and include all
symbols that start with "SIG" so as to automatically include any
new symbols that the C library might add. Unfortunately, this
also picks up "SIGSTKSZ" which is not actually a signal itself,
but a signal related setting -- it's the size of the stack when
a signal is handled.
By itself this doesn't super matter as we will never see a signal
with that same value (since the range of valid signals tend to be
way less than 1024, and the size of the default signal stack will
never be that small). But with recent glibc changes that make this
into a dynamic value instead of a compile-time constant, some users
see build failures when building the sim.
As suggested by Adam Sampson, update our scripts to ignore this
symbol to simplify everything and avoid the build failure.
Bug: https://sourceware.org/PR28302
Not all systems have easy access to hard links or symlinks, so add
fallback logic to the run->psim build code to handle those.
Bug: https://sourceware.org/PR18864
While working on other problems, I encountered situations where GDB
fails to properly unwind the stack because some functions use the C.MV
instruction in the prologue. The prologue scanner stops when it hits
this instruction assuming its job is done at this point. Unfortunately
the prologue is not necessarily finished yet, preventing GDB to properly
unwind.
This commit adds support for handling such instruction in
riscv_scan_prologue.
Note that C.MV is part of the compressed instruction set. The MV
counterpart from the base ISA is a pseudo instruction that expands to
'ADDI RD,RS1,0' which is already supported.
Tested on riscv64-linux-gnu.
All feedback are welcome.
Remove macro COMPUNIT_CALL_SITE_HTAB, and provide access to the htab using
member functions:
- compunit_symtab::find_call_site
- compunit_symtab::set_call_site_htab
Tested on x86_64-linux.
Co-Authored-By: Tom de Vries <tdevries@suse.de>
Fix these rather obvious warnings reported by flake8:
./lib/gdb/FrameIterator.py:16:1: F401 'gdb' imported but unused
./lib/gdb/FrameIterator.py:17:1: F401 'itertools' imported but unused
./lib/gdb/command/prompt.py:55:26: E712 comparison to False should be 'if cond is False:' or 'if not cond:'
./lib/gdb/command/explore.py:526:9: F841 local variable 'has_explorable_fields' is assigned to but never used
./lib/gdb/command/explore.py:697:56: E712 comparison to False should be 'if cond is False:' or 'if not cond:'
./lib/gdb/command/explore.py:736:62: E712 comparison to False should be 'if cond is False:' or 'if not cond:'
./lib/gdb/command/explore.py:767:61: E712 comparison to False should be 'if cond is False:' or 'if not cond:'
./lib/gdb/command/frame_filters.py:21:1: F401 'copy' imported but unused
./lib/gdb/command/frame_filters.py:22:1: F401 'gdb.FrameIterator.FrameIterator' imported but unused
./lib/gdb/command/frame_filters.py:23:1: F401 'gdb.FrameDecorator.FrameDecorator' imported but unused
./lib/gdb/command/frame_filters.py:25:1: F401 'itertools' imported but unused
./lib/gdb/command/frame_filters.py:179:17: E712 comparison to True should be 'if cond is True:' or 'if cond:'
Change-Id: I4f49c0cb430359ee872222600c61d9c5283b09ab
