It occured to me that `intrusive_list<solib>`, as returned by
`solib_ops::current_sos`, for instance, is not very safe. The
current_sos method returns the ownership of the solib objects
(heap-allocated) to its caller, but the `intrusive_list<solib>` type
does not convey it. If a function is building an
`intrusive_list<solib>` and something throws, the solibs won't
automatically be deleted. Introduce owning_intrusive_list to fill this
gap.
Interface
---------
The interface of owning_intrusive_list is mostly equivalent to
intrusive_list, with the following differences:
- When destroyed, owning_intrusive_list deletes all element objects.
The clear method does so as well.
- The erase method destroys the removed object.
- The push_front, push_back and insert methods accept a `unique_ptr<T>`
(compared to `T &` for intrusive_list), taking ownership of the
object.
- owning_intrusive_list has emplace_front, emplace_back and emplace
methods, allowing to allocate and construct an object directly in the
list. This is really just a shorthand over std::make_unique and
insert (or push_back / push_front if you don't care about the return
value), but I think it is nicer to read:
list.emplace (pos, "hello", 2);
rather than
list.insert (pos, std::make_unique<Foo> ("hello", 2));
These methods are not `noexcept`, since the allocation or the
constructor could throw.
- owning_intrusive_list has a release method, allowing to remove an
element without destroying it. The release method returns a
pair-like struct with an iterator to the next element in the list
(like the erase method) and a unique pointer transferring the
ownership of the released element to the caller.
- owning_intrusive_list does not have a clear_and_dispose method, since
that is typically used to manually free items.
Implementation
--------------
owning_intrusive_list privately inherits from intrusive_list, in order
to re-use the linked list machinery. It adds ownership semantics around
it.
Testing
-------
Because of the subtle differences in the behavior in behavior and what
we want to test for each type of intrusive list, I didn't see how to
share the tests for the two implementations. I chose to copy the
intrusive_list tests and adjust them for owning_intrusive_list.
The verify_items function was made common though, and it tries to
dereference the items in the list, to make sure they have not been
deleted by mistake (which would be caught by Valgrind / ASan).
Change-Id: Idbde09c1417b79992a0a9534d6907433e706f760
Co-Authored-By: Pedro Alves <pedro@palves.net>
Reviewed-by: Keith Seitz <keiths@redhat.com>
Some methods of intrusive_list are marked noexcept. But really,
everything in that file could be noexcept. Add it everywhere.
The only one I had a doubt about is clear_and_dispose: what if the
disposer throws? The boost equivalent [1] is noexcept and requires the
disposer not to throw. The rationale is probably the same as for
destructors. What if the disposer throws for an element in the middle
of the list? Do you skip the remaining elements? Do you swallow the
exception and keep calling the disposer for the remaining elements?
It's simpler to say no exceptions allowed.
[1] https://www.boost.org/doc/libs/1_79_0/doc/html/boost/intrusive/list.html#idm33710-bb
Change-Id: I402646cb12c6b7906f4bdc2ad85203d8c8cdf2cc
Reviewed-by: Keith Seitz <keiths@redhat.com>
Several tests in gdb.trace trigger TCL errors if the In-Process Agent
library is not found, e.g.:
Running gdb/testsuite/gdb.trace/change-loc.exp ...
ERROR: tcl error sourcing gdb/testsuite/gdb.trace/change-loc.exp.
ERROR: error copying "gdb/gdb/testsuite/../../gdbserver/libinproctrace.so":
no such file or directory
while executing
"file copy -force $fromfile $tofile"
(procedure "gdb_remote_download" line 29)
invoked from within
"gdb_remote_download target $target_file"
(procedure "gdb_download_shlib" line 6)
invoked from within
"gdb_download_shlib $file"
(procedure "gdb_load_shlib" line 2)
invoked from within
"gdb_load_shlib $libipa"
(file "gdb/testsuite/gdb.trace/change-loc.exp" line 354)
invoked from within
"source gdb/testsuite/gdb.trace/change-loc.exp"
("uplevel" body line 1)
invoked from within
"uplevel #0 source gdb/testsuite/gdb.trace/change-loc.exp"
invoked from within
"catch "uplevel #0 source $test_file_name""
Protect against this error by checking if the library is available.
Distributions like Debian [0] and Gentoo are phasing out the use of
the non-portable `which` utility. Use POSIX's `command -v` instead.
[0] https://lwn.net/Articles/874049/
gprofng/ChangeLog
PR gprofng/32166
* testsuite/lib/Makefile.skel (JAVABIN): Replace use of which.
The binary provided with bug 32165 [1] has 36139 ELF sections. GDB
crashes on it with (note that my GDB is build with -D_GLIBCXX_DEBUG=1:
$ ./gdb -nx -q --data-directory=data-directory ./vmlinux
Reading symbols from ./vmlinux...
(No debugging symbols found in ./vmlinux)
(gdb) info func
/usr/include/c++/14.2.1/debug/vector:508:
In function:
std::debug::vector<_Tp, _Allocator>::reference std::debug::vector<_Tp,
_Allocator>::operator[](size_type) [with _Tp = long unsigned int;
_Allocator = std::allocator<long unsigned int>; reference = long
unsigned int&; size_type = long unsigned int]
Error: attempt to subscript container with out-of-bounds index -29445, but
container only holds 36110 elements.
Objects involved in the operation:
sequence "this" @ 0x514000007340 {
type = std::debug::vector<unsigned long, std::allocator<unsigned long> >;
}
The crash occurs here:
#3 0x00007ffff5e334c3 in __GI_abort () at abort.c:79
#4 0x00007ffff689afc4 in __gnu_debug::_Error_formatter::_M_error (this=<optimized out>) at /usr/src/debug/gcc/gcc/libstdc++-v3/src/c++11/debug.cc:1320
#5 0x0000555561119a16 in std::__debug::vector<unsigned long, std::allocator<unsigned long> >::operator[] (this=0x514000007340, __n=18446744073709522171)
at /usr/include/c++/14.2.1/debug/vector:508
#6 0x0000555562e288e8 in minimal_symbol::value_address (this=0x5190000bb698, objfile=0x514000007240) at /home/smarchi/src/binutils-gdb/gdb/symtab.c:517
#7 0x0000555562e5a131 in global_symbol_searcher::expand_symtabs (this=0x7ffff0f5c340, objfile=0x514000007240, preg=std::optional [no contained value])
at /home/smarchi/src/binutils-gdb/gdb/symtab.c:4983
#8 0x0000555562e5d2ed in global_symbol_searcher::search (this=0x7ffff0f5c340) at /home/smarchi/src/binutils-gdb/gdb/symtab.c:5189
#9 0x0000555562e5ffa4 in symtab_symbol_info (quiet=false, exclude_minsyms=false, regexp=0x0, kind=FUNCTION_DOMAIN, t_regexp=0x0, from_tty=1)
at /home/smarchi/src/binutils-gdb/gdb/symtab.c:5361
#10 0x0000555562e6131b in info_functions_command (args=0x0, from_tty=1) at /home/smarchi/src/binutils-gdb/gdb/symtab.c:5525
That is, at this line of `minimal_symbol::value_address`, where
`objfile->section_offsets` is an `std::vector`:
return (CORE_ADDR (this->unrelocated_address ())
+ objfile->section_offsets[this->section_index ()]);
A section index of -29445 is suspicious. The minimal_symbol at play
here is:
(top-gdb) p m_name
$1 = 0x521001de10af "_sinittext"
So I restarted debugging, breaking on:
(top-gdb) b general_symbol_info::set_section_index if $_streq("_sinittext", m_name)
And I see that weird -29445 value:
(top-gdb) frame
#0 general_symbol_info::set_section_index (this=0x525000082390, idx=-29445) at /home/smarchi/src/binutils-gdb/gdb/symtab.h:611
611 { m_section = idx; }
But going up one frame, the section index is 36091:
(top-gdb) frame
#1 0x0000555562426526 in minimal_symbol_reader::record_full (this=0x7ffff0ead560, name="_sinittext", copy_name=false,
address=-2111475712, ms_type=mst_text, section=36091) at /home/smarchi/src/binutils-gdb/gdb/minsyms.c:1228
1228 msymbol->set_section_index (section);
It seems like the problem is just that the type used for the section
index (short) is not big enough. Change from short to int. If somebody
insists, we could even go long long / int64_t, but I doubt it's
necessary.
With that fixed, I get:
(gdb) info func
All defined functions:
Non-debugging symbols:
0xffffffff81000000 _stext
0xffffffff82257000 _sinittext
0xffffffff822b4ebb _einittext
[1] https://sourceware.org/bugzilla/show_bug.cgi?id=32165
Change-Id: Icb1c3de9474ff5adef7e0bbbf5e0b67b279dee04
Reviewed-By: Tom de Vries <tdevries@suse.de>
Reviewed-by: Keith Seitz <keiths@redhat.com>
This leverages commit ("s390: Simplify (dis)assembly of insn operands
with const bits") to relax the operand constraints of the immediate
operand that contains the constant Z- or T-bit of the following extended
mnemonics:
risbgz, risbgnz, risbhgz, risblgz, rnsbgt, rosbgt, rxsbgt
Previously those instructions were the only ones where the assembler
on s390 restricted the specification of the subject I3/I4 operand values
exactly according to their specification to an unsigned 6- or 5-bit
unsigned integer. For any other instructions the assembler allows to
specify any operand value allowed by the instruction format, regardless
of whether the instruction specification is more restrictive.
Allow to specify the subject I3/I4 operand as unsigned 8-bit integer
with the constant operand bits being ORed during assembly.
Relax the instructions subject significant operand bit masks to only
consider the Z/T-bit as significant, so that the instructions get
disassembled as their *z or *t flavor regardless of whether any reserved
bits are set in addition to the Z/T-bit.
Adapt the rnsbg, rosbg, and rxsbg test cases not to inadvertently set
the T-bit in operand I3, as they otherwise get disassembled as their
rnsbgt, rosbgt, and rxsbgt counterpart.
This aligns GNU Assembler to LLVM Assembler.
opcodes/
* s390-opc.c (U6_18, U5_27, U6_26): Remove.
(INSTR_RIE_RRUUU2, INSTR_RIE_RRUUU3, INSTR_RIE_RRUUU4): Define
as INSTR_RIE_RRUUU while retaining insn fmt mask.
(MASK_RIE_RRUUU2, MASK_RIE_RRUUU3, MASK_RIE_RRUUU4): Treat only
Z/T-bit of I3/I4 operand as significant.
gas/testsuite/
* gas/s390/zarch-z10.s (rnsbg, rosbg, rxsbg): Do not set T-bit.
Reported-by: Dominik Steenken <dost@de.ibm.com>
Suggested-by: Ulrich Weigand <ulrich.weigand@de.ibm.com>
Signed-off-by: Jens Remus <jremus@linux.ibm.com>
Simplify assembly and disassembly of extended mnemonics with operands
with constant ORed bits:
Their instruction template already contains the respective constant
operand bits, as they are significant to distinguish the extended from
their base mnemonic. Operands are ORed into the instruction template.
Therefore it is not necessary to OR the constant bits into the operand
value during assembly in s390_insert_operand.
Additionally the constant operand bits from the instruction template
can be used to mask them from the operand value during disassembly in
s390_print_insn_with_opcode. For now do so for non-length unsigned
integer operands only.
The separate instruction formats need to be retained, as their masks
differ, which is relevant during disassembly to distinguish the base
and extended mnemonics from each other.
This affects the following extended mnemonics:
- vfaebs, vfaehs, vfaefs
- vfaezb, vfaezh, vfaezf
- vfaezbs, vfaezhs, vfaezfs
- vstrcbs, vstrchs, vstrcfs
- vstrczb, vstrczh, vstrczf
- vstrczbs, vstrczhs, vstrczfs
- wcefb, wcdgb
- wcelfb, wcdlgb
- wcfeb, wcgdb
- wclfeb, wclgdb
- wfisb, wfidb, wfixb
- wledb, wflrd, wflrx
include/
* opcode/s390.h (S390_OPERAND_OR1, S390_OPERAND_OR2,
S390_OPERAND_OR8): Remove.
opcodes/
* s390-opc.c (U4_OR1_24, U4_OR2_24, U4_OR8_28): Remove.
(INSTR_VRR_VVV0U1, INSTR_VRR_VVV0U2, INSTR_VRR_VVV0U3): Define
as INSTR_VRR_VVV0U0 while retaining respective insn fmt mask.
(INSTR_VRR_VV0UU8): Define as INSTR_VRR_VV0UU while retaining
respective insn fmt mask.
(INSTR_VRR_VVVU0VB1, INSTR_VRR_VVVU0VB2, INSTR_VRR_VVVU0VB3):
Define as INSTR_VRR_VVVU0VB while retaining respective insn fmt
mask.
* s390-dis.c (s390_print_insn_with_opcode): Mask constant
operand bits set in insn template of non-length unsigned
integer operands.
gas/
* config/tc-s390.c (s390_insert_operand): Do not OR constant
operand value bits.
Signed-off-by: Jens Remus <jremus@linux.ibm.com>
Fixed UBSAN runtime errors such as:
- load of value 4294967295, which is not a valid value for type 'Cmsg_warn'
- null pointer passed as argument 2, which is declared to never be null
- load of value 4294967295, which is not a valid value for type 'ProfData_type'
- reference binding to misaligned address 0x00000357583c for type 'long unsigned int', which requires 8 byte alignment
gprofng/ChangeLog
2024-09-09 Vladimir Mezentsev <vladimir.mezentsev@oracle.com>.
PR gprofng/32096
* src/BaseMetric.cc: Fix UBSAN runtime errors.
* src/BaseMetric.h: Likewise.
* src/Emsg.h: Likewise.
* src/Experiment.cc: Likewise.
* src/Table.h: Likewise.
Test-case gdb.dwarf2/forward-spec.exp contains a non-trivial gdb_test_multiple
to parse this cooked_index_entry:
...
[5] ((cooked_index_entry *) 0x7f01f0004040)^M
name: v^M
canonical: v^M
qualified: ns::v^M
DWARF tag: DW_TAG_variable^M
flags: 0x2 [IS_STATIC]^M
DIE offset: 0xcb^M
parent: ((cooked_index_entry *) 0x7f01f00040a0) [ns]^M
...
which allows us to verify that the entry has a parent.
After commit 8f258a6c97 ("[gdb/symtab] Dump qualified name of
cooked_index_entry") that's no longer necessary.
Simplify this by checking for ns::v instead.
While we're at it, also fix the test-case for target boards readnow,
cc-with-gdb-index and cc-with-debug-names.
Tested on x86_64-linux.
A previous patch made ld fail early on Thumb-only where branch_type is
ST_BRANCH_UNKNOWN.
However, this fails erroneously when the target is undefweak: in that
case the branch should be replaced by a branch to the next instruction
(or nop.w on thumb2). This patch accepts this case and restores the
previous behaviour in such cases.
This was reported by failures in the GCC testsuite, where we fail to
link executables because __deregister_frame_info is undefweak:
(__deregister_frame_info): Unknown destination type (ARM/Thumb) in ...crtbegin.o
crtbegin.o: in function `__do_global_dtors_aux':
crtstuff.c:(.text+0x52): dangerous relocation: unsupported relocation
What's past an active .end directive (when that has its default purpose)
is supposed to be entirely ignored. That should be true not just for
regular processing, but also for "pre-processing" (aka scrubbing). A
complication is that such a directive may of course occur inside a
(false) conditional or a macro definition. To deal with that make sure
we can continue as usual if called another time.
Note however that .end inside a macro will still have the full macro
body expanded; dealing with that would require further (perhaps
intrusive) adjustments in sb_scrub_and_add_sb() and/or callers thereof.
However, at least some of the warnings issued by do_scrub_chars() are
unlikely to occur when expanding a macro. (If we needed to go that far,
presumably .exitm would also want recognizing.)
In that mode the comment char is ; while @ has no special meaning.
Engaging the special logic in that case results in comments not being
respected on .symver lines.
EVEX.B4 is used only for GPR (or addressing of memory) operands. SIMD
registers encoded via ModR/M.rm (when ModR/M.mod == 3) have their top
bit in EVEX.X3. Supposedly (doc version 004) EVEX.B4 is ignored when
unused, hence also don't flag such encodings as invalid.
Error messages there would better not be followed by further "junk at
end of line" diagnostics. Arrange for this to be the case uniformly.
While there also replace a somewhat unhelpful open-coding of
restore_line_pointer().
A bug in ld meant that we were erroneously generating image relocations
for .secrel32 ops, which we then reflected in our PDB section
contributions because the linker was adding a .reloc section.
This was incidental to what we were testing for, so pass
--disable-reloc-section to ld in order to ensure a consistent output.
In gdb.base/corefile-buildid.exp, in the function
do_corefile_buildid_tests, if we fail to find the build-id for the
test binary then we call 'untested', but then push on with the test,
which inevitably fails as the rest of the test depends on having found
the build-id.
I think we're missing a 'return' after the call to 'untested' which
I've now added.
Also I noticed that we call build_id_debug_filename_get and then
manually remove '.debug' from the end. This is no longer necessary,
we can just ask build_id_debug_filename_get to not add the suffix.
The commit:
commit 29c7078711
Date: Sun Sep 8 07:46:09 2024 +0200
[gdb/testsuite] Handle missing curses in gdb.python/py-missing-debug.exp
Highlighted that in some cases we might be running on a system with an
older version of Python (earlier than 3.7), and on a system for which
the curses library has not been installed.
In these circumstances the gdb.missing_debug module will not load as
it uses curses to provide isalnum() and isascii() functions.
To avoid this problem I propose that we copy the isalnum() and
isascii() from the Python curses library. These functions are
basically trivial and removing the curses dependency means GDB will
work in more cases without increasing its dependencies.
I did consider keeping the uses of curses and only having the function
definitions be a fallback for when the curses library failed to load,
but this felt like overkill. The function definitions are both tiny
and I think "obvious" given their specifications, so I figure we might
as well just use our own definitions if they are not available as
builtin methods on the str class.
For testing I changed this line:
if sys.version_info >= (3, 7):
to
if sys.version_info >= (3, 7) and False:
then reran gdb.python/py-missing-debug.exp, there were no failures.
Approved-By: Tom de Vries <tdevries@suse.de>
When running test-case gdb.xml/tdesc-regs.exp on riscv64-linux, I get:
...
(gdb) set tdesc file single-reg.xml^M
warning: Architecture rejected target-supplied description^M
(gdb) FAIL: gdb.xml/tdesc-regs.exp: set tdesc file single-reg.xml
UNSUPPORTED: gdb.xml/tdesc-regs.exp: register tests
...
The FAIL and UNSUPPORTED are produced here:
...
# If no core registers were specified, assume this target does not
# support target-defined registers. Verify that we get a warning if
# we try to use them. This not only tests the warning, but also
# reminds maintainers to add test support when they add the feature.
if {[string equal ${core-regs} ""]} {
gdb_test "set tdesc file $single_reg_xml" \
"warning: Target-supplied registers are not supported.*" \
"set tdesc file single-reg.xml"
unsupported "register tests"
return 0
}
...
The test-case contains target-specific setting of the core-regs variable, and
adding this for riscv64 bypasses this code and makes the test-case pass.
However, without that change, the test-case shouldn't produce a FAIL since
gdb isn't doing anything wrong.
Fix this by producing instead:
...
PASS: $exp: set tdesc file single-reg.xml
UNSUPPORTED: $exp: register tests (missing architecture-specific core-regs setting)
...
Tested on riscv64-linux.
On x86_64-linux, with gcc 7.5.0 and CFLAGS/CXXFLAGS="-O0 -g -Wall" I ran into
a build breaker:
...
gdb/corelow.c: In member function ‘void mapped_file_info::add(const char*, const char*, const char*, std::vector<mem_range>&&, const bfd_build_id*)’:
gdb/corelow.c:1822:27: error: unused variable ‘it’ [-Werror=unused-variable]
const auto [it, inserted]
^
...
Fix this by dropping the variable it.
Tested on x86_64-linux.
Reviewed-By: Lancelot Six<lancelot.six@amd.com>
Since linker calls bfd_plugin_object_p, which calls ld_plugin_object_p,
only for command-line input objects, pass true to ld_plugin_object_p so
that the same input IR file won't be included twice if the new LTO hook,
LDPT_REGISTER_CLAIM_FILE_HOOK_V2 isn't used.
PR ld/32153
* plugin.c (bfd_plugin_object_p): Pass true to ld_plugin_object_p.
Signed-off-by: H.J. Lu <hjl.tools@gmail.com>
Currently, the callers of ada_identical_enum_types_p must check that
both enum types have the same number of members. In another series
I'm working on, it was convenient to move this check into the callee
instead; and I broke this patch out to make that series a little
simpler.
Approved-By: Tom de Vries <tdevries@suse.de>
This moves the declaration of 'i' into the 'for' loops in
ada_identical_enum_types_p. This is just a trivial cleanup.
Approved-By: Tom de Vries <tdevries@suse.de>
This fixes a couple of comments in dwarf2/cooked-index.h.
The comment by cooked_index_entry::canonical mentions C++, but this
field can also be different from 'name' in other situations. Rather
than enumerate the cases here (which doesn't seem important), make the
text a little less specific.
Also, cooked_index_entry::write_scope doesn't document its "for_main"
parameter -- and it is misnamed in the prototype as well.
Reviewed-By: Tom de Vries <tdevries@suse.de>
This changes cooked_index_shard::handle_gnat_encoded_entry to modify
the incoming entry itself, and to return void rather than a new name.
this simplifies the caller a little, which is convenient for a
different series I am working on.
Approved-By: Tom de Vries <tdevries@suse.de>
DW_TAG_padding isn't a real tag -- it doesn't appear in the DWARF
standard, only in include/dwarf2.def as a placeholder. So, remove it
from dwarf2/tag.h:tag_is_type.
Reviewed-By: Tom de Vries <tdevries@suse.de>
Document the s390-specific assembler syntax introduced by commit
aacf780bca ("s390: Allow to explicitly omit base register operand in
assembly") to omit the base register operand B in D(X,B) and D(L,B) by
coding D(X,) and D(L,).
While at it document the alternative syntax to omit the index register
operand X in D(X,B) by coding D(,B) instead of D(B).
gas/
* doc/c-s390.texi (s390 Operands): Document syntax to omit base
register operand.
Fixes: aacf780bca ("s390: Allow to explicitly omit base register operand in assembly")
Signed-off-by: Jens Remus <jremus@linux.ibm.com>
I noticed that the list of general NEWS items seemed to have gotten
mixed up a bit in the NEWS file. This commit just moves things around
so that the general items all appear at the start of the 'Changes
since GDB 15' section. I've not changed any of the actual content.
The precedence of the operators "+" and "-" in the current loongarch
instruction expression is higher than "<<" and ">>", which is different
from the explanation in the user guide.
We modified the precedence of "<<" and ">>" to be higher than "+" and "-".
The commit:
commit c6b486755e
Date: Thu Mar 30 19:21:22 2023 +0100
gdb: parse pending breakpoint thread/task immediately
Introduce a use bug where the value of a temporary variable was being
used after it had gone out of scope. This was picked up by the
address sanitizer and would result in this error:
(gdb) maintenance selftest create_breakpoint_parse_arg_string
Running selftest create_breakpoint_parse_arg_string.
=================================================================
==2265825==ERROR: AddressSanitizer: stack-use-after-scope on address 0x7fbb08046511 at pc 0x000001632230 bp 0x7fff7c2fb770 sp 0x7fff7c2fb768
READ of size 1 at 0x7fbb08046511 thread T0
#0 0x163222f in create_breakpoint_parse_arg_string(char const*, std::unique_ptr<char, gdb::xfree_deleter<char> >*, int*, int*, int*, std::unique_ptr<char, gdb::xfree_deleter<char> >*, bool*) ../../src/gdb/break-cond-parse.c:496
#1 0x1633026 in test ../../src/gdb/break-cond-parse.c:582
#2 0x163391b in create_breakpoint_parse_arg_string_tests ../../src/gdb/break-cond-parse.c:649
#3 0x12cfebc in void std::__invoke_impl<void, void (*&)()>(std::__invoke_other, void (*&)()) /usr/include/c++/13/bits/invoke.h:61
#4 0x12cc8ee in std::enable_if<is_invocable_r_v<void, void (*&)()>, void>::type std::__invoke_r<void, void (*&)()>(void (*&)()) /usr/include/c++/13/bits/invoke.h:111
#5 0x12c81e5 in std::_Function_handler<void (), void (*)()>::_M_invoke(std::_Any_data const&) /usr/include/c++/13/bits/std_function.h:290
#6 0x18bb51d in std::function<void ()>::operator()() const /usr/include/c++/13/bits/std_function.h:591
#7 0x4193ef9 in selftests::run_tests(gdb::array_view<char const* const>, bool) ../../src/gdbsupport/selftest.cc:100
#8 0x21c2206 in maintenance_selftest ../../src/gdb/maint.c:1172
... etc ...
The problem was caused by three lines like this one:
thread_info *thr
= parse_thread_id (std::string (t.get_value ()).c_str (), &tmptok);
After parsing the thread-id TMPTOK would be left pointing into the
temporary string which had been created on this line. When on the
next line we did this:
gdb_assert (*tmptok == '\0');
The value of *TMPTOK is undefined.
Fix this by creating the std::string earlier in the scope. Now the
contents of the string will remain valid when we check *TMPTOK. The
address sanitizer issue is now resolved.
On a system with python 3.6, module gdb.missing_debug imports module curses,
so when running test-case gdb.python/py-missing-debug.exp on a system without
that module installed, we run into:
...
(gdb) source py-missing-debug.py^M
Python Exception <class 'ImportError'>: Module 'curses' is not installed.^M
Use:^M
sudo zypper install python36-curses^M
to install it.^M
Error occurred in Python: Module 'curses' is not installed.^M
Use:^M
sudo zypper install python36-curses^M
to install it.^M
(gdb) FAIL: gdb.python/py-missing-debug.exp: source python script
...
Fix this by issuing UNSUPPORTED instead, and bailing out.
Tested on x86_64-linux.
Approved-by: Kevin Buettner <kevinb@redhat.com>
PR testsuite/31576
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=31576
This commit updates GDB so that thread or inferior specific
breakpoints are only inserted into the program space in which the
specific thread or inferior is running.
In terms of implementation, getting this basically working is easy
enough, now that a breakpoint's thread or inferior field is setup
prior to GDB looking for locations, we can easily use this information
to find a suitable program_space and pass this to as a filter when
creating the sals.
Or we could if breakpoint_ops::create_sals_from_location_spec allowed
us to pass in a filter program_space.
So, this commit extends breakpoint_ops::create_sals_from_location_spec
to take a program_space argument, and uses this to filter the set of
returned sals. This accounts for about half the change in this patch.
The second set of changes starts from breakpoint_set_thread and
breakpoint_set_inferior, this is called when the thread or inferior
for a breakpoint changes, e.g. from the Python API.
Previously this call would never result in the locations of a
breakpoint changing, after all, locations were inserted in every
program space, and we just use the thread or inferior variable to
decide when we should stop. Now though, changing a breakpoint's
thread or inferior can mean we need to figure out a new set of
breakpoint locations.
To support this I've added a new breakpoint_re_set_one function, which
is like breakpoint_re_set, but takes a single breakpoint, and just
updates the locations for that one breakpoint. We only need to call
this function if the program_space in which a breakpoint's thread (or
inferior) is running actually changes. If the program_space does
change then we call the new breakpoint_re_set_one function passing in
the program_space which should be used to filter the new locations (or
nullptr to indicate we should set locations in all program spaces).
This filter program_space needs to propagate down to all the re_set
methods, this accounts for the remaining half of the changes in this
patch.
There were a couple of existing tests that created thread or inferior
specific breakpoints and then checked the 'info breakpoints' output,
these needed updating. These were:
gdb.mi/user-selected-context-sync.exp
gdb.multi/bp-thread-specific.exp
gdb.multi/multi-target-continue.exp
gdb.multi/multi-target-ping-pong-next.exp
gdb.multi/tids.exp
gdb.mi/new-ui-bp-deleted.exp
gdb.multi/inferior-specific-bp.exp
gdb.multi/pending-bp-del-inferior.exp
I've also added some additional tests to:
gdb.multi/pending-bp.exp
I've updated the documentation and added a NEWS entry.
Reviewed-By: Eli Zaretskii <eliz@gnu.org>
I spotted this code within create_breakpoint:
if ((type_wanted != bp_breakpoint
&& type_wanted != bp_hardware_breakpoint) || thread != -1)
b->pspace = current_program_space;
this code is only executed when creating a pending breakpoint, and
sets the breakpoint::pspace member variable.
The above code gained the 'thread != -1' clause with this commit:
commit cc72b2a2da
Date: Fri Dec 23 17:06:16 2011 +0000
Introduce gdb.FinishBreakpoint in Python
While the type_wanted checks were added with this commit:
commit f8eba3c616
Date: Tue Dec 6 18:54:43 2011 +0000
the "ambiguous linespec" series
Before this breakpoint::pspace was set unconditionally.
If we look at how breakpoint::pspace is used today, some breakpoint
types specifically set this field, either in their constructors, or in
a wrapper function that calls the constructor. So, the watchpoint
type and its sub-class set this variable, as does the catchpoint type,
and all it's sub-classes.
However, code_breakpoint doesn't specifically set this field within
its constructor, though some sub-classes of
code_breakpoint (ada_catchpoint, exception_catchpoint,
internal_breakpoint, and momentary_breakpoint) do set this field.
When I examine all the places that breakpoint::pspace is used, I
believe that in every place where it is expected that this field is
set, the breakpoint type will be one that specifically sets this
field.
Next, I observe two problems with the existing code.
First, the above code is only hit for pending breakpoints, there's no
equivalent code for non-pending breakpoints. This opens up the
possibility of GDB entering non-consistent states; if a breakpoint is
first created pending and then later gets a location, the pspace field
will be set, while if the breakpoint is immediately non-pending, then
the pspace field will never be set.
Second, if we look at how breakpoint::pspace is used in the function
breakpoint_program_space_exit, we see that when a program space is
removed, any breakpoint with breakpoint::pspace set to the removed
program space, will be deleted. This makes sense, but does mean we
need to ensure breakpoint::pspace is only set for breakpoints that
apply to a single program space.
So, if I create a pending dprintf breakpoint (type bp_dprintf) then
the breakpoint::pspace variable will be set even though the dprintf is
not really tied to that one program space. As a result, when the
matching program space is removed the dprintf is incorrectly removed.
Also, if I create a thread specific breakpoint, then, thanks to the
'thread != -1' clause the wrong program space will be stored in
breakpoint::pspace (the current program space is always used, which
might not be the program space that corresponds to the selected
thread), as a result, the thread specific breakpoint will be deleted
when the matching program space is removed.
If we look at commit cc72b2a2da which added the 'thread != -1'
clause, we can see this change was entirely redundant, the
breakpoint::pspace is also set in bpfinishpy_init after
create_breakpoint has been called. As such, I think we can safely
drop the 'thread != -1' clause.
For the other problems, I'm proposing to be pretty aggressive - I'd
like to drop the breakpoint::pspace assignment completely from
create_breakpoint. Having looked at how this variable is used, I
believe that it is already set elsewhere in all the cases that it is
needed. Maybe this code was needed at one time, but I can't see how
it's needed any more.
There's tests to expose the issues I've spotted with this code, and
there's no regressions in testing.
The initial motivation for this commit was to allow thread or inferior
specific breakpoints to only be inserted within the appropriate
inferior's program-space. The benefit of this is that inferiors for
which the breakpoint does not apply will no longer need to stop, and
then resume, for such breakpoints. This commit does not make this
change, but is a refactor to allow this to happen in a later commit.
The problem we currently have is that when a thread-specific (or
inferior-specific) breakpoint is created, the thread (or inferior)
number is only parsed by calling find_condition_and_thread_for_sals.
This function is only called for non-pending breakpoints, and requires
that we know the locations at which the breakpoint will be placed (for
expression checking in case the breakpoint is also conditional).
A consequence of this is that by the time we figure out the breakpoint
is thread-specific we have already looked up locations in all program
spaces. This feels wasteful -- if we knew the thread-id earlier then
we could reduce the work GDB does by only looking up locations within
the program space for which the breakpoint applies.
Another consequence of how find_condition_and_thread_for_sals is
called is that pending breakpoints don't currently know they are
thread-specific, nor even that they are conditional! Additionally, by
delaying parsing the thread-id, pending breakpoints can be created for
non-existent threads, this is different to how non-pending
breakpoints are handled, so I can do this:
$ gdb -q ./gdb/testsuite/outputs/gdb.multi/pending-bp/pending-bp
Reading symbols from ./gdb/testsuite/outputs/gdb.multi/pending-bp/pending-bp...
(gdb) break foo thread 99
Function "foo" not defined.
Make breakpoint pending on future shared library load? (y or [n]) y
Breakpoint 1 (foo thread 99) pending.
(gdb) r
Starting program: /tmp/gdb/testsuite/outputs/gdb.multi/pending-bp/pending-bp
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/lib64/libthread_db.so.1".
Error in re-setting breakpoint 1: Unknown thread 99.
[Inferior 1 (process 3329749) exited normally]
(gdb)
GDB only checked the validity of 'thread 99' at the point the 'foo'
location became non-pending. In contrast, if I try this:
$ gdb -q ./gdb/testsuite/outputs/gdb.multi/pending-bp/pending-bp
Reading symbols from ./gdb/testsuite/outputs/gdb.multi/pending-bp/pending-bp...
(gdb) break main thread 99
Unknown thread 99.
(gdb)
GDB immediately checks if 'thread 99' exists. I think inconsistencies
like this are confusing, and should be fixed if possible.
In this commit the create_breakpoint function is updated so that the
extra_string, which contains the thread, inferior, task, and/or
condition information, is parsed immediately, even for pending
breakpoints.
Obviously, the condition still can't be validated until the breakpoint
becomes non-pending, but the thread, inferior, and task information
can be pulled from the extra-string, and can be validated early on,
even for pending breakpoints. The -force-condition flag is also
parsed as part of this early parsing change.
There are a couple of benefits to doing this:
1. Printing of breakpoints is more consistent now. Consider creating
a conditional breakpoint before this commit:
(gdb) set breakpoint pending on
(gdb) break pendingfunc if (0)
Function "pendingfunc" not defined.
Breakpoint 1 (pendingfunc if (0)) pending.
(gdb) break main if (0)
Breakpoint 2 at 0x401198: file /tmp/hello.c, line 18.
(gdb) info breakpoints
Num Type Disp Enb Address What
1 breakpoint keep y <PENDING> pendingfunc if (0)
2 breakpoint keep y 0x0000000000401198 in main at /tmp/hello.c:18
stop only if (0)
(gdb)
And after this commit:
(gdb) set breakpoint pending on
(gdb) break pendingfunc if (0)
Function "pendingfunc" not defined.
Breakpoint 1 (pendingfunc) pending.
(gdb) break main if (0)
Breakpoint 2 at 0x401198: file /home/andrew/tmp/hello.c, line 18.
(gdb) info breakpoints
Num Type Disp Enb Address What
1 breakpoint keep y <PENDING> pendingfunc
stop only if (0)
2 breakpoint keep y 0x0000000000401198 in main at /home/andrew/tmp/hello.c:18
stop only if (0)
(gdb)
Notice that the display of the condition is now the same for the
pending and non-pending breakpoints.
The same is true for the thread, inferior, or task information in
thread, inferior, or task specific breakpoints; this information is
displayed on its own line rather than being part of the 'What'
field.
2. We can check that the thread exists as soon as the pending
breakpoint is created. Currently there is a weird difference
between pending and non-pending breakpoints when creating a
thread-specific breakpoint.
A pending thread-specific breakpoint only checks its thread when it
becomes non-pending, at which point the thread the breakpoint was
intended for might have exited. Here's the behaviour before this
commit:
(gdb) set breakpoint pending on
(gdb) break foo thread 2
Function "foo" not defined.
Breakpoint 2 (foo thread 2) pending.
(gdb) c
Continuing.
[Thread 0x7ffff7c56700 (LWP 2948835) exited]
Error in re-setting breakpoint 2: Unknown thread 2.
[Inferior 1 (process 2948832) exited normally]
(gdb)
Notice the 'Error in re-setting breakpoint 2: Unknown thread 2.'
line, this was triggered when GDB tried to make the breakpoint
non-pending, and GDB discovers that the thread no longer exists.
Compare that to the behaviour after this commit:
(gdb) set breakpoint pending on
(gdb) break foo thread 2
Function "foo" not defined.
Breakpoint 2 (foo) pending.
(gdb) c
Continuing.
[Thread 0x7ffff7c56700 (LWP 2949243) exited]
Thread-specific breakpoint 2 deleted - thread 2 no longer in the thread list.
[Inferior 1 (process 2949240) exited normally]
(gdb)
Now the behaviour for pending breakpoints is identical to
non-pending breakpoints, the thread specific breakpoint is removed
as soon as the thread the breakpoint is associated with exits.
There is an additional change; when the pending breakpoint is
created prior to this patch we see this line:
Breakpoint 2 (foo thread 2) pending.
While after this patch we get this line:
Breakpoint 2 (foo) pending.
Notice that 'thread 2' has disappeared. This might look like a
regression, but I don't think it is. That we said 'thread 2'
before was just a consequence of the lazy parsing of the breakpoint
specification, while with this patch GDB understands, and has
parsed away the 'thread 2' bit of the spec. If folk think the old
information was useful then this would be trivial to add back in
code_breakpoint::say_where.
As a result of this commit the breakpoints 'extra_string' field is now
only used by bp_dprintf type breakpoints to hold the printf format and
arguments. This string should always be empty for other breakpoint
types. This allows some cleanup in print_breakpoint_location.
In code_breakpoint::code_breakpoint I've changed an error case into an
assert. This is because the error is now handled earlier in
create_breakpoint. As a result we know that by this point, the
extra_string will always be nullptr for anything other than a
bp_dprintf style breakpoint.
The find_condition_and_thread_for_sals function is now no longer
needed, this was previously doing the delayed splitting of the extra
string into thread, task, and condition, but this is now all done in
create_breakpoint, so find_condition_and_thread_for_sals can be
deleted, and the code that calls this in
code_breakpoint::location_spec_to_sals can be removed. With this
update this code would only ever be reached for bp_dprintf style
breakpoints, and in these cases the extra_string should not contain
anything other than format and args.
The most interesting changes are all in create_breakpoint and in the
new file break-cond-parse.c. We have a new block of code early on in
create_breakpoint that is responsible for splitting the extra_string
into its component parts by calling create_breakpoint_parse_arg_string
a function in the new break-cond-parse.c file. This means that some
of the later code can be simplified a little.
The new break-cond-parse.c file implements the splitting up the
extra_string and finding all the parts, as well as some self-tests for
the new function.
Finally, now we know all the breakpoint details, these can be stored
within the breakpoint object if we end up creating a deferred
breakpoint. Additionally, if we are creating a deferred bp_dprintf we
can parse the extra_string to build the printf command.
The implementation here aims to maintain backwards compatibility as
much as possible, this means that:
1. We support abbreviations of 'thread', 'task', and 'inferior' in
some places on the breakpoint line. The handling of abbreviations
has (before this patch) been a little weird, so this works:
(gdb) break *main th 1
And creates a breakpoint at '*main' for thread 1 only, while this
does not work:
(gdb) break main th 1
In this case GDB will try to find the symbol 'main th 1'. This
weirdness exists before and after this patch.
2. The handling of '-force-condition' is odd, if this flag appears
immediately after a condition then it will be treated as part of the
condition, e.g.:
(gdb) break main if 0 -force-condition
No symbol "force" in current context.
But we are fine with these alternatives:
(gdb) break main if 0 thread 1 -force-condition
(gdb) break main -force-condition if 0
Again, this is just a quirk of how the breakpoint line used to be
parsed, but I've maintained this for backward compatibility. During
review it was suggested that -force-condition should become an
actual breakpoint flag (i.e. only valid after the 'break' command
but before the function name), and I don't think that would be a
terrible idea, however, that's not currently a trivial change, and I
think should be done as a separate piece of work. For now, this
patch just maintains the current behaviour.
The implementation works by first splitting the breakpoint condition
string (everything after the location specification) into a list of
tokens, each token has a type and a value. (e.g. we have a THREAD
token where the value is the thread-id string). The list of tokens is
validated, and in some cases, tokens are merged. Then the values are
extracted from the remaining token list.
Consider this breakpoint command:
(gdb) break main thread 1 if argc == 2
The condition string passed to create_breakpoint_parse_arg_string is
going to be 'thread 1 if argc == 2', which is then split into the
tokens:
{ THREAD: "1" } { CONDITION: "argc == 2" }
The thread-id (1) and the condition string 'argc == 2' are extracted
from these tokens and returns back to create_breakpoint.
Now consider this breakpoint command:
(gdb) break some_function if ( some_var == thread )
Here the user wants a breakpoint if 'some_var' is equal to the
variable 'thread'. However, when this is initially parsed we will
find these tokens:
{ CONDITION: "( some_var == " } { THREAD: ")" }
This is a consequence of how we have to try and figure out the
contents of the 'if' condition without actually parsing the
expression; parsing the expression requires that we know the location
in order to lookup the variables by name, and this can't be done for
pending breakpoints (their location isn't known yet), and one of the
points of this work is that we extract things like thread-id for
pending breakpoints.
And so, it is in this case that token merging takes place. We check
if the value of a token appearing immediately after the CONDITION
token looks valid. In this case, does ')' look like a valid
thread-id. Clearly, in this case ')' does not, and so me merge the
THREAD token into the condition token, giving:
{ CONDITION: "( some_var == thread )" }
Which is what we want.
I'm sure that we might still be able to come up with some edge cases
where the parser makes the wrong choice. I think long term the best
way to work around these would be to move the thread, inferior, task,
and -force-condition flags to be "real" command options for the break
command. I am looking into doing this, but can't guarantee if/when
that work would be completed, so this patch should be reviewed assume
that the work will never arrive (though I hope it will).
Reviewed-By: Eli Zaretskii <eliz@gnu.org>
This is a refactoring commit that splits the existing parse_thread_id
function into two parts, and then adds a new is_thread_id function.
The core of parse_thread_id is split into parse_thread_id_1, which is
responsible for actually parsing a thread-id. Then parse_thread_id is
responsible for taking a parsed thread-id and validating that it
references an actually existing inferior thread.
The new is_thread_id function also uses parse_thread_id_1, but doesn't
actually check that the inferior thread exists, instead, this new
function simply checks that a string looks like a thread-id.
This commit does not add a use for is_thread_id, this will be added in
the next commit.
This is a refactoring commit, there should be no user visible changes
after this commit.
We already have one overload of the startswith function that takes a
std::string_view for both arguments. A later patch in this series is
going to be improved by having an overload that takes one argument as
a std::string_view and the other argument as a plain 'char *'.
This commit adds the new overload, but doesn't make use of it (yet).
There should be no user visible changes after this commit.
This commit makes breakpoint_debug_printf available outside of
breakpoint.c. In a later commit I'll want to use this macro from
another file.
This is just a refactor, there should be no user visible changes after
this commit.
