Currently, the overload handling in Ada assumes that any two array
types are compatible. However, this is obviously untrue, and a user
reported an oddity where comparing two Ada strings resulted in a call
to the "=" function for packed boolean arrays.
This patch improves the situation somewhat, by requiring that the two
arrays have the same arity and compatible base element types. This is
still over-broad, but it seems safe and is better than the status quo.
The previous patch fixed the immediate performance problem with Ada
name matching, by having a subset of matches call
expand_symtabs_matching rather than expand_matching_symbols. However,
it seemed to me that expand_matching_symbols should not be needed at
all.
To achieve this, this patch changes ada_lookup_name_info::split_name
to use the decoded name, rather than the encoded name. In order to
make this work correctly, a new decoded form is used: one that does
not decode operators (this is already done) and also does not decode
wide characters. The latter change is done so that changes to the Ada
source charset don't affect the DWARF index.
With this in place, we can change ada-lang.c to always use
expand_symtabs_matching rather than expand_matching_symbols.
A user reported that certain operations -- like printing a large
structure -- could be slow. I tracked this down to
ada-lang.c:map_matching_symbols taking an inordinate amount of time.
Specifically, calls like the one to look for a parallel "__XVZ"
variable, in ada_to_fixed_type_1, could result in gdb walking over all
the entries in the cooked index over and over.
Looking into this reveals that
cooked_index_functions::expand_matching_symbols is not written
efficiently -- it ignores its "ordered_compare" parameter. While
fixing this would be good, it turns out that this entire method isn't
needed; so this series removes it.
However, the deletion is not done in this patch. This one, instead,
fixes the immediate cause of the slowdown, by using
objfile::expand_symtabs_matching when possible. This approach is
faster because it is more selective about which index entries to
examine.
This changes gdb to use the C++17 [[fallthrough]] attribute rather
than special comments.
This was mostly done by script, but I neglected a few spellings and so
also fixed it up by hand.
I suspect this fixes the bug mentioned below, by switching to a
standard approach that, presumably, clang supports.
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=23159
Approved-By: John Baldwin <jhb@FreeBSD.org>
Approved-By: Luis Machado <luis.machado@arm.com>
Approved-By: Pedro Alves <pedro@palves.net>
This patch removes all uses of to_string(const std::string_view&) and
use the std::string ctor or implicit conversion from std::string_view to
std::string instead.
A later patch will remove this gdb::to_string while removing
gdbsupport/gdb_string_view.h.
Change-Id: I877cde557a0727be7b0435107e3c7a2aac165895
Approved-By: Tom Tromey <tom@tromey.com>
Approved-By: Pedro Alves <pedro@palves.net>
Given that GDB now requires a C++17, replace all uses of
gdb::string_view with std::string_view.
This change has mostly been done automatically:
- gdb::string_view -> std::string_view
- #include "gdbsupport/gdb_string_view.h" -> #include <string_view>
One things which got brought up during review is that gdb::stging_view
does support being built from "nullptr" while std::sting_view does not.
Two places are manually adjusted to account for this difference:
gdb/tui/tui-io.c:tui_getc_1 and
gdbsupport/format.h:format_piece::format_piece.
The above automatic change transformed
"gdb::to_string (const gdb::string_view &)" into
"gdb::to_string (const std::string_view &)". The various direct users
of this function are now explicitly including
"gdbsupport/gdb_string_view.h". A later patch will remove the users of
gdb::to_string.
The implementation and tests of gdb::string_view are unchanged, they will
be removed in a following patch.
Change-Id: Ibb806a7e9c79eb16a55c87c6e41ad396fecf0207
Approved-By: Tom Tromey <tom@tromey.com>
Approved-By: Pedro Alves <pedro@palves.net>
Since GDB now requires C++17, we don't need the internally maintained
gdb::optional implementation. This patch does the following replacing:
- gdb::optional -> std::optional
- gdb::in_place -> std::in_place
- #include "gdbsupport/gdb_optional.h" -> #include <optional>
This change has mostly been done automatically. One exception is
gdbsupport/thread-pool.* which did not use the gdb:: prefix as it
already lives in the gdb namespace.
Change-Id: I19a92fa03e89637bab136c72e34fd351524f65e9
Approved-By: Tom Tromey <tom@tromey.com>
Approved-By: Pedro Alves <pedro@palves.net>
This function is just a wrapper around the current inferior's gdbarch.
I find that having that wrapper just obscures where the arch is coming
from, and that it's often used as "I don't know which arch to use so
I'll use this magical target_gdbarch function that gets me an arch" when
the arch should in fact come from something in the context (a thread,
objfile, symbol, etc). I think that removing it and inlining
`current_inferior ()->arch ()` everywhere will make it a bit clearer
where that arch comes from and will trigger people into reflecting
whether this is the right place to get the arch or not.
Change-Id: I79f14b4e4934c88f91ca3a3155f5fc3ea2fadf6b
Reviewed-By: John Baldwin <jhb@FreeBSD.org>
Approved-By: Andrew Burgess <aburgess@redhat.com>
The new_objfile observer is currently used to indicate both when a new
objfile is added to program space (when passed non-nullptr) and when all
objfiles of a program space were just removed (when passed nullptr).
I think this is confusing (and Andrew apparently thinks so too [1]).
Add a new "all_objfiles_removed" observer to remove the second role from
"new_objfile".
Some existing users of new_objfile do nothing if the passed objfile is
nullptr. For them, we can simply drop the nullptr check. For others,
add a new all_objfiles_removed callback, and refactor things a bit to
keep the existing behavior as much as possible.
Some callbacks relied on current_program_space, and following
the refactoring now use either objfile->pspace or the pspace passed to
all_objfiles_removed. I think this should be relatively safe, and in
general a step in the right direction.
On the notify side, I found only one call site to change from
new_objfile to all_objfiles_removed, in clear_symtab_users. It is not
entirely clear to me that this is entirely correct. clear_symtab_users
appears to be called in spots that don't remove all objfiles
(functions finish_new_objfile, remove_symbol_file_command, reread_symbols,
do_module_cleanups). But I think that this patch at least makes the
current code clearer.
[1] a0a031bce0
Change-Id: Icb648f72862e056267f30f44dd439bd4ec766f13
Approved-By: Tom Tromey <tom@tromey.com>
Make the references to current_program_space bubble up one level.
Change-Id: I82acab5628c30f6535d52aa32ce2c1d0375cbeed
Approved-By: Tom Tromey <tom@tromey.com>
This adds new is_array_like and to_array methods to language_defn.
This will be used in a subsequent patch that generalizes the new
Python array- and string-handling code.
When running test-case gdb.ada/local-enum.exp with target board debug-types, I
run into:
...
(gdb) print v1(three)^M
No name 'three' in enumeration type 'local__e1'^M
(gdb) FAIL: gdb.ada/local-enum.exp: print v1 element
...
The array V1 is of type A1 which is an array with index type E1, containing
"three" as enumerator:
...
type E1 is (one, two, three);
type A1 is array (E1) of Integer;
V1 : A1 := (0, 1, 2);
...
There's also a type E2 that contains three as enumerator:
...
type E2 is (three, four, five);
...
When doing "print v1(three)", it's the job of ada_resolve_enum to resolve
"three" to type E1 rather than type E2.
When using target board debug-types, the enums E1 and E2 are replicated in the
.debug_types section, and consequently in ada_resolve_enum the type
equivalence check using a pointer comparison fails:
...
for (int i = 0; i < syms.size (); ++i)
{
/* We already know the name matches, so we're just looking for
an element of the correct enum type. */
if (ada_check_typedef (syms[i].symbol->type ()) == context_type)
return i;
}
...
Fix this by also trying a structural comparison using
ada_identical_enum_types_p.
Tested on x86_64-linux.
Approved-By: Tom Tromey <tom@tromey.com>
PR ada/29335
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=29335
When running test-case gdb.ada/arr_acc_idx_w_gap.exp with target board
cc-with-dwz, I run into:
...
(gdb) print enum_with_gaps'enum_rep(lit3)^M
'Enum_Rep requires argument to have same type as enum^M
(gdb) FAIL: gdb.ada/arr_acc_idx_w_gap.exp: enum_rep
...
With target_board unix, we have instead:
...
(gdb) print enum_with_gaps'enum_rep(lit3)^M
$16 = 13^M
(gdb) PASS: gdb.ada/arr_acc_idx_w_gap.exp: enum_rep
...
Conversely, when I add this test to the test-case:
...
gdb_test "print enum_with_gaps'enum_rep(lit3)" " = 13" \
"enum_rep"
+ gdb_test "print enum_subrange'enum_rep(lit3)" " = 13" \
+ "other enum_rep"
...
the extra test passes with target board cc-with-dwz, but fails with target
board unix.
The problem is here in remove_extra_symbols:
...
if (symbols_are_identical_enums (syms))
syms.resize (1);
...
where one of the two identical enums is picked before the enum_rep handling
can resolve lit3 to one of the two.
Fix this by moving the code to ada_resolve_variable.
Tested on x86_64-linux.
Approved-By: Tom Tromey <tom@tromey.com>
PR ada/30726
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=30726
Ada has a few complexities when it comes to array handling. Currently
these are all handled in Ada-specific code -- but unfortunately that
means they aren't really accessible to Python.
This patch changes the Python code to defer to Ada when given an Ada
array. In order to make this work, one spot in ada-lang.c had to be
updated to set the "GNAT-specific" flag on an array type.
The test case for this will come in a later patch.
An upstream bug report points out this bug: if the user switches from
one Ada executable to another without "kill"ing the inferior, then the
"start" command will fail.
What happens here is that the Ada "main" name is found in a constant
string in the executable. But, if the inferior is running, then the
process_stratum target reads from the inferior memory.
This patch fixes the problem by changing the main name code to set
trust-readonly-sections, causing the target stack to read from the
executable instead.
I looked briefly at changing GNAT to emit DW_AT_main_subprogram
instead, but this looks to be pretty involved.
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=25811
This adds symbol::matches, a wrapper for symbol_matches_domain. Most
places calling symbol_matches_domain can call this method instead,
which is a bit less wordy and also (IMO) clearer.
Approved-By: Simon Marchi <simon.marchi@efficios.com>
Add these two methods, rename the field to m_bitsize to make it pseudo
private.
Change-Id: Ief95e5cf106e72f2c22ae47b033d0fa47202b413
Approved-By: Tom Tromey <tom@tromey.com>
After finding this code in buildsym_compunit::finish_block_internal:
...
ftype->set_fields
((struct field *)
TYPE_ALLOC (ftype, nparams * sizeof (struct field)));
...
and fixing PR30810 by using TYPE_ZALLOC, I wondered if there were more
locations that needed fixing.
I decided to make things easier to spot by factoring out a new function
alloc_fields:
...
/* Allocate the fields array of this type, with NFIELDS elements. If INIT,
zero-initialize the allocated memory. */
void
type::alloc_fields (unsigned int nfields, bool init = true);
...
where:
- a regular use would be "alloc_fields (nfields)", and
- an exceptional use that needed no initialization would be
"alloc_fields (nfields, false)".
Pretty soon I discovered that most of the latter cases are due to
initialization by memcpy, so I added two variants of copy_fields as well.
After this rewrite there are 8 uses of set_fields left:
...
gdb/coffread.c: type->set_fields (nullptr);
gdb/coffread.c: type->set_fields (nullptr);
gdb/coffread.c: type->set_fields (nullptr);
gdb/eval.c: type->set_fields
gdb/gdbtypes.c: type->set_fields (args);
gdb/gdbtypes.c: t->set_fields (XRESIZEVEC (struct field, t->fields (),
gdb/dwarf2/read.c: type->set_fields (new_fields);
gdb/dwarf2/read.c: sub_type->set_fields (sub_type->fields () + 1);
...
These fall into the following categories:
- set to nullptr (coffread.c),
- type not owned by objfile or gdbarch (eval.c), and
- modifying an existing fields array, like adding an element at the end or
dropping an element at the start (the rest).
Tested on x86_64-linux.
When building gdb with -std=c++20 I run into:
...
gdb/ada-lang.c:10713:16: error: implicit capture of ‘this’ via ‘[=]’ is \
deprecated in C++20 [-Werror=deprecated]
10713 | auto do_op = [=] (LONGEST x, LONGEST y)
| ^
gdb/ada-lang.c:10713:16: note: add explicit ‘this’ or ‘*this’ capture
...
Fix this by using "[this]".
Likewise in two more spots.
Tested on x86_64-linux.
A bug report pointed out a buffer overflow in ada_decode, which Keith
helpfully analyzed. ada_decode had a logic error when the input was
all digits. While this isn't valid -- and would probably only appear
in fuzzer tests -- it still should be handled properly.
This patch adds a missing bounds check. Tested with the self-tests in
an asan build.
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=30639
Reviewed-by: Keith Seitz <keiths@redhat.com>
I found an obvious bug in Ada aggregate expression handling:
if (vvo != nullptr)
error (_("Invalid record component association."));
name = vvo->get_symbol ()->natural_name ();
Here the code errors when vvo is not null -- and then proceeds to use
vvo.
This hasn't caused a crash because, I believe, there's currently no
way to reach this code in the null case. However, I'm not really
willing to assert this...
Fixing this shows another bug, which is that due to the way the parser
works, a field name in an aggregate expression might erroneously be
fully qualified if some global variable with the same base name
exists.
The included test case triggers both bugs. Note that the test
includes a confounding case for array aggregates as well, but as these
are harder to fix, I've left it as kfail.
As this is Ada-specific, and has already been tested internally at
AdaCore, I am checking it in.
Ada 2022 adds the "target name symbol", which can be used on the right
hand side of an assignment to refer to the left hand side. This
allows for convenient updates. This patch implements this for gdb's
Ada expression parser.
Reviewed-By: Eli Zaretskii <eliz@gnu.org>
I ran across this very old code in gdb's Ada support. After a bit of
archaeology, we couldn't determine what bug this might have been
working around. It is no longer needed, so this patch removes it.
As this is entirely Ada-specific and was reviewed and tested at
AdaCore, I'm checking it in.
ada_attribute_name uses an array that must be kept in sync with an
enum -- but the comment here refers to an enum that no longer exists.
Looking at the sole caller, I see this can only be called for two
opcodes. So, remove this entirely and inline it.
Currently, Ada catchpoints require that the inferior be running.
However, there's no deep reason for this -- for example, C++ exception
catchpoints do not have this requirement. Instead, those work like
ordinary breakpoints: they are pending until the needed runtime
locations are seen.
This patch changes Ada catchpoints to work the same way.
This patch merges create_excep_cond_exprs into ada_catchpoint::re_set.
This is less verbose and is also a step toward making ada_catchpoint
work more like the other code_breakpoint-based exception catchpoints.
This patch simplifies ada_lookup_struct_elt_type by changing it to
call find_struct_field. The two functions were substantially similar,
even to the point of having identical comments.
I tested this using both the gdb test suite and the internal AdaCore
test suite. Given this and the fact that it is Ada-specific, I am
checking it in.
History Of This Patch
=====================
This commit aims to address PR gdb/21699. There have now been a
couple of attempts to fix this issue. Simon originally posted two
patches back in 2021:
https://sourceware.org/pipermail/gdb-patches/2021-July/180894.htmlhttps://sourceware.org/pipermail/gdb-patches/2021-July/180896.html
Before Pedro then posted a version of his own:
https://sourceware.org/pipermail/gdb-patches/2021-July/180970.html
After this the conversation halted. Then in 2023 I (Andrew) also took
a look at this bug and posted two versions:
https://sourceware.org/pipermail/gdb-patches/2023-April/198570.htmlhttps://sourceware.org/pipermail/gdb-patches/2023-April/198680.html
The approach taken in my first patch was pretty similar to what Simon
originally posted back in 2021. My second attempt was only a slight
variation on the first.
Pedro then pointed out his older patch, and so we arrive at this
patch. The GDB changes here are mostly Pedro's work, but updated by
me (Andrew), any mistakes are mine.
The tests here are a combinations of everyone's work, and the commit
message is new, but copies bits from everyone's earlier work.
Problem Description
===================
Bug PR gdb/21699 makes the observation that using $_as_string with
GDB's printf can cause GDB to print unexpected data from the
inferior. The reproducer is pretty simple:
#include <stddef.h>
static char arena[100];
/* Override malloc() so value_coerce_to_target() gets a known
pointer, and we know we"ll see an error if $_as_string() gives
a string that isn't null terminated. */
void
*malloc (size_t size)
{
memset (arena, 'x', sizeof (arena));
if (size > sizeof (arena))
return NULL;
return arena;
}
int
main ()
{
return 0;
}
And then in a GDB session:
$ gdb -q test
Reading symbols from /tmp/test...
(gdb) start
Temporary breakpoint 1 at 0x4004c8: file test.c, line 17.
Starting program: /tmp/test
Temporary breakpoint 1, main () at test.c:17
17 return 0;
(gdb) printf "%s\n", $_as_string("hello")
"hello"xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx
(gdb) quit
The problem above is caused by how value_cstring is used within
py-value.c, but once we understand the issue then it turns out that
value_cstring is used in an unexpected way in many places within GDB.
Within py-value.c we have a null-terminated C-style string. We then
pass a pointer to this string, along with the length of this
string (so not including the null-character) to value_cstring.
In value_cstring GDB allocates an array value of the given character
type, and copies in requested number of characters. However
value_cstring does not add a null-character of its own. This means
that the value created by calling value_cstring is only
null-terminated if the null-character is included in the passed in
length. In py-value.c this is not the case, and indeed, in most uses
of value_cstring, this is not the case.
When GDB tries to print one of these strings the value contents are
pushed to the inferior, and then read back as a C-style string, that
is, GDB reads inferior memory until it finds a null-terminator. For
the py-value.c case, no null-terminator is pushed into the inferior,
so GDB will continue reading inferior memory until a null-terminator
is found, with unpredictable results.
Patch Description
=================
The first thing this patch does is better define what the arguments
for the two function value_cstring and value_string should represent.
The comments in the header file are updated to describe whether the
length argument should, or should not, include a null-character.
Also, the data argument is changed to type gdb_byte. The functions as
they currently exist will handle wide-characters, in which case more
than one 'char' would be needed for each character. As such using
gdb_byte seems to make more sense.
To avoid adding casts throughout GDB, I've also added an overload that
still takes a 'char *', but asserts that the character type being used
is of size '1'.
The value_cstring function is now responsible for adding a null
character at the end of the string value it creates.
However, once we start looking at how value_cstring is used, we
realise there's another, related, problem. Not every language's
strings are null terminated. Fortran and Ada strings, for example,
are just an array of characters, GDB already has the function
value_string which can be used to create such values.
Consider this example using current GDB:
(gdb) set language ada
(gdb) p $_gdb_setting("arch")
$1 = (97, 117, 116, 111)
(gdb) ptype $
type = array (1 .. 4) of char
(gdb) p $_gdb_maint_setting("test-settings string")
$2 = (0)
(gdb) ptype $
type = array (1 .. 1) of char
This shows two problems, first, the $_gdb_setting and
$_gdb_maint_setting functions are calling value_cstring using the
builtin_char character, rather than a language appropriate type. In
the first call, the 'arch' case, the value_cstring call doesn't
include the null character, so the returned array only contains the
expected characters. But, in the $_gdb_maint_setting example we do
end up including the null-character, even though this is not expected
for Ada strings.
This commit adds a new language method language_defn::value_string,
this function takes a pointer and length and creates a language
appropriate value that represents the string. For C, C++, etc this
will be a null-terminated string (by calling value_cstring), and for
Fortran and Ada this can be a bounded array of characters with no null
terminator. Additionally, this new language_defn::value_string
function is responsible for selecting a language appropriate character
type.
After this commit the only calls to value_cstring are from the C
expression evaluator and from the default language_defn::value_string.
And the only calls to value_string are from Fortan, Ada, and ObjectC
related code.
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=21699
Co-Authored-By: Simon Marchi <simon.marchi@efficios.com>
Co-Authored-By: Andrew Burgess <aburgess@redhat.com>
Co-Authored-By: Pedro Alves <pedro@palves.net>
Approved-By: Simon Marchi <simon.marchi@efficios.com>
Remove the bp_location_pointer_iterator layer. Adjust all users of
breakpoint::locations to use references instead of pointers.
Change-Id: Iceed34f5e0f5790a9cf44736aa658be6d1ba1afa
Reviewed-By: Andrew Burgess <aburgess@redhat.com>
Add three convenience methods to struct breakpoint:
- has_locations: returns true if the breakpoint has at least one
location
- has_single_location: returns true if the breakpoint has exactly one
location
- has_multiple_locations: returns true if the breakpoint has more than
one location
A subsequent patch changes the list of breakpoints to be an
intrusive_list, so all these spots would need to change. But in any
case, I think that this:
if (b->has_multiple_locations ())
conveys the intention better than:
if (b->loc != nullptr && b->loc->next != nullptr)
Change-Id: Ib18c3605fd35d425ef9df82cb7aacff1606c6747
Reviewed-By: Andrew Burgess <aburgess@redhat.com>
The print_it method itself is const. In a subsequent patch, the
locations that come out of a const breakpoint will be const as well. It
will therefore be needed to make the last_loc output parameter const as
well. Make that change now to reduce the size of the following patches.
Change-Id: I7ed962950bc9582646e31e2e42beca2a1c9c5105
Reviewed-By: Andrew Burgess <aburgess@redhat.com>
A co-worker here at AdaCore discovered that the Pragma Import series
caused a rgression. When debugging gnat1, gdb started asking for
overload resolution like:
(gdb) call pp(n)
Multiple matches for pp
[0] cancel
[1] pp (types.union_id) at ../../gcc/gcc/ada/treepr.adb:511
[2] treepr.pp (types.union_id) at ../../gcc/gcc/ada/treepr.adb:511
This worked before the series, and is strange anyway, because the
matches refer to the same function.
This patch adds a test case for this situation and fixes the bug by
pruning identical functions in remove_extra_symbols.
I noticed many spots checking whether a dynamic property's kind is
PROP_CONST. Some spots, I think, are doing a slightly incorrect check
-- checking for != PROP_UNDEFINED where == PROP_CONST is actually
required, the key thing being that const_val may only be called for
PROP_CONST properties.
This patch adds dynamic::is_constant and then updates these checks to
use it.
Regression tested on x86-64 Fedora 36.
A co-worker at AdaCore noticed that calling a function without
debuginfo yields:
(gdb) print plus_one(23)
'pck.plus_one' has unknown return type; cast the call to its declared return type
However, this also happens if you follow the directions and add the
cast.
This patch fixes the problem and adds a regression test.
I think that the language_auto enumerator and the auto_language class
can be removed. There isn't really an "auto" language, it's only a
construct of the "set language" command to say "pick the appropriate
language automatically". But "auto" is never the current language. The
`current_language` points to the current effective language, and the
fact that we're in "auto language" mode is noted by the language_mode
global.
- Change set_language to handle the "auto" (and "local", which is a
synonym) early, instead of in the for loop. I think it makes the two
cases (auto vs explicit language) more clearly separated anyway.
- Adjust add_set_language_command to hard-code the "auto" string,
instead of using the "auto" language definition.
- Remove auto_language, rename auto_or_unknown_language to
unknown_language and move the bits of the existing unknown_language
in there.
- Remove the set_language at the end of _initialize_language. I think
it's not needed, because we call set_language in gdb_init, after all
_initialize functions are called. There is some chance that an
_initialize function that runs after _initialize_language implicitly
depends on current_language being set, but my testsuite runs haven't
found anything like that.
- Use language_unknown instead of language_auto when creating a minimal
symbol (minimal_symbol_reader::record_full). I think that this value
is used to indicate that we don't know the symbol of the minimal
symbol (yet), so language_unknown makes sense to me. Update a
condition accordingly in ada-lang.c. symbol_find_demangled_name also
appears to "normalize" this value from "unknown" to "auto", remove
that part and update the condition to just check for
language_unknown.
Change-Id: I47bcd6c15f607d9818f2e6e413053c2dc8ec5034
Reviewed-By: Tom Tromey <tom@tromey.com>
I noticed some unnecessary casts to LONGEST in ada-lang.c. This patch
removes the ones I think are very clearly not needed. I'm checking
this in as obvious.
