I would like to use gdb::parallel_for_each to implement the parallelism
of the DWARF unit indexing. However, the existing implementation of
gdb::parallel_for_each is blocking, which doesn't work with the model
used by the DWARF indexer, which is asynchronous and callback-based.
Add an asynchronouys version of gdb::parallel_for_each that will be
suitable for this task.
This new version accepts a callback that is invoked when the parallel
for each is complete.
This function uses the same strategy as gdb::task_group to invoke the
"done" callback: worker threads have a shared_ptr reference to some
object. The last worker thread to drop its reference causes the object
to be deleted, which invokes the callback.
Unlike for the sync version of gdb::parallel_for_each, it's not possible
to keep any state in the calling thread's stack, because that disappears
immediately after starting the workers. So all the state is kept in
that same shared object.
There is a limitation that the sync version doesn't have, regarding the
arguments you can pass to the worker objects: it's not possibly to rely
on references. There are more details in a comment in the code.
It would be possible to implement the sync version of
gdb::parallel_for_each on top of the async version, but I decided not to
do it to avoid the unnecessary dynamic allocation of the shared object,
and to avoid adding the limitations on passing references I mentioned
just above. But if we judge that it would be an acceptable cost to
avoid the duplication, we could do it.
Add a self test for the new function.
Change-Id: I6173defb1e09856d137c1aa05ad51cbf521ea0b0
Approved-By: Tom Tromey <tom@tromey.com>
In preparation for a following patch that will re-use the shared work
queue algorithm, move it to a separate class.
Change-Id: Id05cf8898a5d162048fa8fa056fbf7e0441bfb78
Approved-By: Tom Tromey <tom@tromey.com>
I think it would be convenient for parallel_for_each to pass an
iterator_range to the worker function, instead of separate begin and end
parameters. This allows using a ranged for loop directly.
Change-Id: I8f9681da65b0eb00b738379dfd2f4dc6fb1ee612
Approved-By: Tom Tromey <tom@tromey.com>
Add iterator_range::empty, indicating if the range is empty. This is
used in the following patch.
Change-Id: I1e6c873e635c2bb0ce5aaea2a176470970f6d7ac
Approved-By: Tom Tromey <tom@tromey.com>
gdb::parallel_for_each uses static partitioning of the workload, meaning
that each worker thread receives a similar number of work items. Change
it to use dynamic partitioning, where worker threads pull work items
from a shared work queue when they need to.
Note that gdb::parallel_for_each is currently only used for processing
minimal symbols in GDB. I am looking at improving the startup
performance of GDB, where the minimal symbol process is one step.
With static partitioning, there is a risk of workload imbalance if some
threads receive "easier" work than others. Some threads sit still while
others finish working on their share of the work. This is not
desirable, because the gdb::parallel_for_each takes as long as the
slowest thread takes.
When loading a file with a lot of minimal symbols (~600k) in GDB, with
"maint set per-command time on", I observe some imbalance:
Time for "minsyms install worker": wall 0.732, user 0.550, sys 0.041, user+sys 0.591, 80.7 % CPU
Time for "minsyms install worker": wall 0.881, user 0.722, sys 0.071, user+sys 0.793, 90.0 % CPU
Time for "minsyms install worker": wall 2.107, user 1.804, sys 0.147, user+sys 1.951, 92.6 % CPU
Time for "minsyms install worker": wall 2.351, user 2.003, sys 0.151, user+sys 2.154, 91.6 % CPU
Time for "minsyms install worker": wall 2.611, user 2.322, sys 0.235, user+sys 2.557, 97.9 % CPU
Time for "minsyms install worker": wall 3.074, user 2.729, sys 0.203, user+sys 2.932, 95.4 % CPU
Time for "minsyms install worker": wall 3.486, user 3.074, sys 0.260, user+sys 3.334, 95.6 % CPU
Time for "minsyms install worker": wall 3.927, user 3.475, sys 0.336, user+sys 3.811, 97.0 % CPU
^
----´
The fastest thread took 0.732 seconds to complete its work (and then sat
still), while the slowest took 3.927 seconds. This means the
parallel_for_each took a bit less than 4 seconds.
Even if the number of minimal symbols assigned to each worker is the
same, I suppose that some symbols (e.g. those that need demangling) take
longer to process, which could explain the imbalance.
With this patch, things are much more balanced:
Time for "minsym install worker": wall 2.807, user 2.222, sys 0.144, user+sys 2.366, 84.3 % CPU
Time for "minsym install worker": wall 2.808, user 2.073, sys 0.131, user+sys 2.204, 78.5 % CPU
Time for "minsym install worker": wall 2.804, user 1.994, sys 0.151, user+sys 2.145, 76.5 % CPU
Time for "minsym install worker": wall 2.808, user 1.977, sys 0.135, user+sys 2.112, 75.2 % CPU
Time for "minsym install worker": wall 2.808, user 2.061, sys 0.142, user+sys 2.203, 78.5 % CPU
Time for "minsym install worker": wall 2.809, user 2.012, sys 0.146, user+sys 2.158, 76.8 % CPU
Time for "minsym install worker": wall 2.809, user 2.178, sys 0.137, user+sys 2.315, 82.4 % CPU
Time for "minsym install worker": wall 2.820, user 2.141, sys 0.170, user+sys 2.311, 82.0 % CPU
^
----´
In this version, the parallel_for_each took about 2.8 seconds,
representing a reduction of ~1.2 seconds for this step. Not
life-changing, but it's still good I think.
Note that this patch helps when loading big programs. My go-to test
program for this is telegram-desktop that I built from source. For
small programs (including loading gdb itself), it makes no perceptible
difference.
Now the technical bits:
- One impact that this change has on the minimal symbol processing
specifically is that not all calls to compute_and_set_names (a
critical region guarded by a mutex) are done at the end of each
worker thread's task anymore.
Before this patch, each thread would compute the names and hash values for
all the minimal symbols it has been assigned, and then would call
compute_and_set_names for all of them, while holding the mutex (thus
preventing other threads from doing this same step).
With the shared work queue approach, each thread grabs a batch of of
minimal symbols, computes the names and hash values for them, and
then calls compute_and_set_names (with the mutex held) for this batch
only. It then repeats that until the work queue is empty.
There are therefore more small and spread out compute_and_set_names
critical sections, instead of just one per worker thread at the end.
Given that before this patch the work was not well balanced among worker
threads, I guess that threads would enter that critical region at
roughly different times, causing little contention.
In the "with this patch" results, the CPU utilization numbers are not
as good, suggesting that there is some contention. But I don't know
if it's contention due to the compute_and_set_names critical section
or the shared work queue critical section. That can be investigated
later. In any case, what ultimately counts is the wall time, which
improves.
- One choice I had to make was to decide how many work items (in this
case minimal symbols) each worker should pop when getting work from
the shared queue. The general wisdom is that:
- popping too few items, and the synchronization overhead becomes
significant, and the total processing time increases
- popping too many items, and we get some imbalance back, and the
total processing time increases again
I experimented using a dynamic batch size proportional to the number
of remaining work items. It worked well in some cases but not
always. So I decided to keep it simple, with a fixed batch size.
That can always be tweaked later.
- I want to still be able to use scoped_time_it to measure the time
that each worker thread spent working on the task. I find it really
handy when measuring the performance impact of changes.
Unfortunately, the current interface of gdb::parallel_for_each, which
receives a simple callback, is not well-suited for that, once I
introduce the dynamic partitioning. The callback would get called
once for each work item batch (multiple time for each worker thread),
so it's not possible to maintain a per-worker thread object for the
duration of the parallel for.
To allow this, I changed gdb::parallel_for_each to receive a worker
type as a template parameter. Each worker thread creates one local
instance of that type, and calls operator() on it for each work item
batch. By having a scoped_time_it object as a field of that worker,
we can get the timings per worker thread.
The drawbacks of this approach is that we must now define the
parallel task in a separate class and manually capture any context we
need as fields of that class.
Change-Id: Ibf1fea65c91f76a95b9ed8f706fd6fa5ef52d9cf
Approved-By: Tom Tromey <tom@tromey.com>
I started working on this patch because I noticed that this
parallel_for_each test:
/* Check that if there are fewer tasks than threads, then we won't
end up with a null result. */
is not really checking anything. And then, this patch ended with
several changes, leading to general refactor of the whole file.
This test verifies, using std::all_of, that no entry in the intresults
vector is nullptr. However, nothing ever adds anything to intresults.
Since the vector is always empty, std::all_of is always true. This
state probably dates back to afdd136635 ("Back out some
parallel_for_each features"), which removed the ability for
parallel_for_each to return a vector of results. That commit removed
some tests, but left this one in, I'm guessing as an oversight.
One good idea in this test is to check that the worker never receives
empty ranges. I think we should always test for that. I think it's
also a good idea to test with exactly one item, that's a good edge case.
To achieve this without adding some more code duplication, factor out
the core functionality of the test in yet another test_one function (I'm
running out of ideas for names). In there, check that the range
received by the worker is not empty. Doing this pointed out that the
worker is actually called with empty ranges in some cases, necessitating
some minor changes in parallel-for.h.
Then, instead of only checking that the sum of the ranges received by
worker functions is the right count, save the elements received as part
of those ranges (in a vector), and check that this vector contains each
expected element exactly once. This should make the test a bit more
robust (otherwise we could have the right number of calls, but on the
wrong items).
Then, a subsequent patch in this series changes the interface or
parallel_for_each to use iterator_range. The only hiccup is that it
doesn't really work if the "RandomIt" type of the parallel_for_each is
"int". iterator_range<int>::size wouldn't work, as std::distance
doesn't work on two ints. Fix this in the test right away by building
an std::vector<int> to use as input.
Finally, run the test with the default thread pool thread count in
addition to counts 0, 1 an 3, currently tested. I'm thinking that it
doesn't hurt to test parallel_for_each in the configuration that it's
actually used with.
Change-Id: I5adf3d61e6ffe3bc249996660f0a34b281490d54
Approved-By: Tom Tromey <tom@tromey.com>
Add a new maintenance command 'maint test-remote-args', this command
takes an argument string and splits it using gdb::remote_args::split
and then joins the result using gdb::remote_args::join and prints all
of the results. This is useful for diagnosing problems with remote
argument passing.
This new command is identical to what the remote argument self-tests
do, but while I was working on improving remote argument passing it
was far easier to have a command that I could just throw example
strings at, rather than having to add new selftests and recompile
GDB.
I ended up adding a couple of additional helper functions to the
gdb::argv_vec class.
Reviewed-By: Eli Zaretskii <eliz@gnu.org>
Tested-By: Guinevere Larsen <guinevere@redhat.com>
Approved-By: Kevin Buettner <kevinb@redhat.com>
Remove the use of xmalloc (and the arbitrary allocation size) in
format_pieces. This turned out a bit more involved than expected, but
not too bad.
format_pieces::m_storage is a buffer with multiple concatenated
null-terminated strings, referenced by format_piece::string. Change
this to an std::string, while keeping its purpose (use the std::string
as a buffer with embedded null characters).
However, because the std::string's internal buffer can be reallocated as
it grows, and I do not want to hardcode a big reserved size like we have
now, it's not possible to store the direct pointer to the string in
format_piece::string. Those pointers would become stale as the buffer
gets reallocated. Therefore, change format_piece to hold an index into
the storage instead. Add format_pieces::piece_str for the callers to be
able to access the piece's string. This requires changing the few
callers, but in a trivial way.
The selftest also needs to be updated. I want to keep the test cases
as-is, where the expected pieces contain the expected string, and not
hard-code an expected index. To achieve this, add the
expected_format_piece structure. Note that the previous
format_piece::operator== didn't compare the n_int_args fields, while the
test provides expected values for that field. I guess that was a
mistake. The new code checks it, and the test still passes.
Change-Id: I80630ff60e01c8caaa800ae22f69a9a7660bc9e9
Reviewed-By: Keith Seitz <keiths@redhat.com>
Remove the three remaining uses of alloca in gdbsupport.
I only built-tested the Windows-only portion in pathstuff.cc.
Change-Id: Ie588fa57f43de900d5f42e93a8875a7da462404b
Reviewed-By: Keith Seitz <keiths@redhat.com>
I think it makes the code slightly easier to understand.
Change-Id: I49056728e43fbf37c2af8f3904a543c10e987bba
Reviewed-By: Keith Seitz <keiths@redhat.com>
PR ada/33217 points out that gdb incorrectly calls the <ctype.h>
functions. In particular, gdb feels free to pass a 'char' like:
char *str = ...;
... isdigit (*str)
This is incorrect as isdigit only accepts EOF and values that can be
represented as 'unsigned char' -- that is, a cast is needed here to
avoid undefined behavior when 'char' is signed and a character in the
string might be sign-extended. (As an aside, I think this API seems
obviously bad, but unfortunately this is what the standard says, and
some systems check this.)
Rather than adding casts everywhere, this changes all the code in gdb
that uses any <ctype.h> API to instead call the corresponding c-ctype
function.
Now, c-ctype has some limitations compared to <ctype.h>. It works as
if the C locale is in effect, so in theory some non-ASCII characters
may be misclassified. This would only affect a subset of character
sets, though, and in most places I think ASCII is sufficient -- for
example the many places in gdb that check for whitespace.
Furthermore, in practice most users are using UTF-8-based locales,
where these functions aren't really informative for non-ASCII
characters anyway; see the existing workarounds in gdb/c-support.h.
Note that safe-ctype.h cannot be used because it causes conflicts with
readline.h. And, we canot poison the <ctype.h> identifiers as this
provokes errors from some libstdc++ headers.
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=33217
Approved-By: Simon Marchi <simon.marchi@efficios.com>
This changes gdb and related programs to use the gnulib c-ctype code
rather than safe-ctype.h. The gdb-safe-ctype.h header is removed.
This changes common-defs.h to include the c-ctype header, making it
available everywhere in gdb.
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=33217
Approved-By: Simon Marchi <simon.marchi@efficios.com>
It's currently not possible to use filtered_iterator with a pointer as
the base iterator type. This patch makes it possible. The indended
usage is:
Foo array[12];
Foo *begin = array;
Foo *end = array + ARRAY_SIZE (array);
filtered_iterator<Foo *, FooFilter> (begin, end);
Here are the things that needed changing:
- Give filtered_iterator a constructor where the caller provides
already constructed begin and end iterators. filtered_iterator
currently assumes that default-constructing a BaseIterator will
produce a valid "end" iterator. This is not the case if BaseIterator
is a pointer. The caller needs to pass in the end of the array /
region to iterate on as the end.
- Typedefs of member types like wouldn't work:
typedef typename BaseIterator::value_type value_type;
The compiler would complain that it's not possible to apply `::` to
type `BaseIterator` (aka `Foo *`). Use std::iterator_traits to fix
it [1].
- Similarly, the compiler would complain about the use of
`BaseIterator::operator*` in the return type of
`filtered_iterator::operator*`. Fix this by using `decltype(auto)`
as the return type. This lets the compiler deduce the return type
from the return statement. Unlike `auto`, `decltype(auto)` perfectly
preserves the "cvref-ness" of the deduced return type. If the return
expression yields a `Foo &`, then the function will return a `Foo &`
(which is what we want), whereas it would return a `Foo` if we used
just `auto`.
Improve the filtered_iterator unit tests to run the same tests but with
pointers as iterators. Because the filtered_iterator objects are
initialized differently in the two scenarios, I chose to copy the
existing code and adapt it. It would probably be possible to add a
layer of abstraction to avoid code duplication, but it would end up more
complicated and messy. If we ever add a third scenario, we can revisit
that.
[1] https://en.cppreference.com/w/cpp/iterator/iterator_traits.html
Change-Id: Id962ffbcd960a705a82bc5eb4808b4fe118a2761
Approved-By: Tom Tromey <tom@tromey.com>
This patch adds the user mode register PL3_SSP which is part of the
Intel(R) Control-Flow Enforcement Technology (CET) feature for support
of shadow stack.
For now, only native and remote debugging support for shadow stack
userspace on amd64 linux are covered by this patch including 64 bit and
x32 support. 32 bit support is not covered due to missing Linux kernel
support.
This patch requires fixing the test gdb.base/inline-frame-cycle-unwind
which is failing in case the shadow stack pointer is unavailable.
Such a state is possible if shadow stack is disabled for the current thread
but supported by HW.
This test uses the Python unwinder inline-frame-cycle-unwind.py which fakes
the cyclic stack cycle by reading the pending frame's registers and adding
them to the unwinder:
~~~
for reg in pending_frame.architecture().registers("general"):
val = pending_frame.read_register(reg)
unwinder.add_saved_register(reg, val)
return unwinder
~~~
However, in case the python unwinder is used we add a register (pl3_ssp) that is
unavailable. This leads to a NOT_AVAILABLE_ERROR caught in
gdb/frame-unwind.c:frame_unwind_try_unwinder and it is continued with standard
unwinders. This destroys the faked cyclic behavior and the stack is
further unwinded after frame 5.
In the working scenario an error should be triggered:
~~~
bt
0 inline_func () at /tmp/gdb.base/inline-frame-cycle-unwind.c:49^M
1 normal_func () at /tmp/gdb.base/inline-frame-cycle-unwind.c:32^M
2 0x000055555555516e in inline_func () at /tmp/gdb.base/inline-frame-cycle-unwind.c:45^M
3 normal_func () at /tmp/gdb.base/inline-frame-cycle-unwind.c:32^M
4 0x000055555555516e in inline_func () at /tmp/gdb.base/inline-frame-cycle-unwind.c:45^M
5 normal_func () at /tmp/gdb.base/inline-frame-cycle-unwind.c:32^M
Backtrace stopped: previous frame identical to this frame (corrupt stack?)
(gdb) PASS: gdb.base/inline-frame-cycle-unwind.exp: cycle at level 5: backtrace when the unwind is broken at frame 5
~~~
To fix the Python unwinder, we simply skip the unavailable registers.
Also it makes the test gdb.dap/scopes.exp fail. The shadow stack feature is
disabled by default, so the pl3_ssp register which is added with my CET
shadow stack series will be shown as unavailable and we see a TCL error:
~~
>>> {"seq": 12, "type": "request", "command": "variables", "arguments": {"variablesReference": 2, "count": 85}}
Content-Length: 129^M
^M
{"request_seq": 12, "type": "response", "command": "variables", "success": false, "message": "value is not available", "seq": 25}FAIL: gdb.dap/scopes.exp: fetch all registers success
ERROR: tcl error sourcing /tmp/gdb/testsuite/gdb.dap/scopes.exp.
ERROR: tcl error code TCL LOOKUP DICT body
ERROR: key "body" not known in dictionary
while executing
"dict get $val body variables"
(file "/tmp/gdb/testsuite/gdb.dap/scopes.exp" line 152)
invoked from within
"source /tmp/gdb/testsuite/gdb.dap/scopes.exp"
("uplevel" body line 1)
invoked from within
"uplevel #0 source /tmp/gdb/testsuite/gdb.dap/scopes.exp"
invoked from within
"catch "uplevel #0 source $test_file_name" msg"
UNRESOLVED: gdb.dap/scopes.exp: testcase '/tmp/gdb/testsuite/gdb.dap/scopes.exp' aborted due to Tcl error
~~
I am fixing this by enabling the test for CET shadow stack, in case we
detect that the HW supports it:
~~~
# If x86 shadow stack is supported we need to configure GLIBC_TUNABLES
# such that the feature is enabled and the register pl3_ssp is
# available. Otherwise the reqeust to fetch all registers will fail
# with "message": "value is not available".
if { [allow_ssp_tests] } {
append_environment GLIBC_TUNABLES "glibc.cpu.hwcaps" "SHSTK"
}
~~~
Reviewed-by: Thiago Jung Bauermann <thiago.bauermann@linaro.org>
Reviewed-By: Eli Zaretskii <eliz@gnu.org>
Approved-By: Luis Machado <luis.machado@arm.com>
Approved-By: Andrew Burgess <aburgess@redhat.com>
The XSAVE function set is organized in state components, which are a set of
registers or parts of registers. So-called XSAVE-supported features are
organized using state-component bitmaps, each bit corresponding to a
single state component.
The Intel Software Developer's Manual uses the term xstate_bv for a
state-component bitmap, which is defined as XCR0 | IA32_XSS. The control
register XCR0 only contains a state-component bitmap that specifies user state
components, while IA32_XSS contains a state-component bitmap that specifies
supervisor state components.
Until now, XCR0 is used as input for target description creation in GDB.
However, a following patch will add userspace support for the CET shadow
stack feature by Intel. The CET state is configured in IA32_XSS and consists
of 2 state components:
- State component 11 used for the 2 MSRs controlling user-mode
functionality for CET (CET_U state)
- State component 12 used for the 3 MSRs containing shadow-stack pointers
for privilege levels 0-2 (CET_S state).
Reading the CET shadow stack pointer register on linux requires a separate
ptrace call using NT_X86_SHSTK. To pass the CET shadow stack enablement
state we would like to pass the xstate_bv value instead of xcr0 for target
description creation. To prepare for that, we rename the xcr0 mask
values for target description creation to xstate_bv. However, this
patch doesn't add any functional changes in GDB.
Future states specified in IA32_XSS such as CET will create a combined
xstate_bv_mask including xcr0 register value and its corresponding bit in
the state component bitmap. This combined mask will then be used to create
the target descriptions.
Reviewed-By: Thiago Jung Bauermann <thiago.bauermann@linaro.org>
Approved-By: Luis Machado <luis.machado@arm.com>
Stop using AC_HEADER_STDC since it is no longer supported in autoconf
2.72+. We require a C++ compiler that supports c++17, it's probably
safe to assume that the C compiler fully supports C89.
Approved-By: Simon Marchi <simon.marchi@efficios.com>
Most code doesn't use cleanups any more, so remove the include of
cleanups.h from common-defs.h, and then only include that file where
it is truly needed.
Approved-By: Simon Marchi <simon.marchi@efficios.com>
PR mi/32571 reports the following problem:
...
$ gdb -q -batch -ex "b bla.c:100"
<random output>
Make breakpoint pending on future shared library load? (y or [n]) \
[answered N; input not from terminal]
...
while this is expected:
...
$ gdb -q -batch -ex "b bla.c:100"
No symbol table is loaded. Use the "file" command.
Make breakpoint pending on future shared library load? (y or [n]) \
[answered N; input not from terminal]
...
A few factors in reproducing this are building gdb using gcc 14,
"-O2 -flto=auto" and --disable-nls. For more details, see the PR.
This turns out to be caused by a GCC PR [1], more specifically a problem in
ipa-modref.
Work around this by disabling ipa-modref for GCC versions 12-15 and 16.0,
assuming the GCC 16.1 release will contain a fix.
Tested on aarch64-linux and x86_64-linux.
Approved-By: Andrew Burgess <aburgess@redhat.com>
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=32571
[1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=120987
Theoretically, in functions core_addr_to_string_nz() and
core_addr_to_string(), strcat() can overflow, so use a safe
approach using xsnprintf().
Change-Id: Ib9437450b3634dc35077234f462a03a8640242d4
This value will likely never change at runtime, so we might as well make
it a template parameter. This has the "advantage" of being able to
remove the unnecessary param from gdb::sequential_for_each.
Change-Id: Ia172ab8e08964e30d4e3378a95ccfa782abce674
Approved-By: Tom Tromey <tom@tromey.com>
I forgot to fix a `;;` typo when pushing a previous patch. Fix it, and
fix all the other instances I could find in the code base.
Change-Id: I298f9ffb1a5157925076ef67b439579b1aeeaa2b
Building current GDB on Cygwin, fails like so:
/home/pedro/gdb/src/gdbsupport/run-time-clock.cc: In function ‘void get_run_time(user_cpu_time_clock::time_point&, system_cpu_time_clock::time_point&, run_time_scope ’:
/home/pedro/gdb/src/gdbsupport/run-time-clock.cc:52:13: error: ‘RUSAGE_THREAD’ was not declared in this scope; did you mean ‘SIGEV_THREAD’?
52 | who = RUSAGE_THREAD;
| ^~~~~~~~~~~~~
| SIGEV_THREAD
Cygwin does not implement RUSAGE_THREAD. Googling around, I see
Cygwin is not alone, other platforms don't support it either. For
example, here is someone suggesting an alternative for darwin/macos:
https://stackoverflow.com/questions/5652463/equivalent-to-rusage-thread-darwin
Fix this by falling back to process scope if thread scope can't be
supported. I chose this instead of returning zero usage or some other
constant, because if gdb is built without threading support, then
process-scope run time usage is the right info to return.
But instead of falling back silently, print a warning (just once),
like so:
(gdb) maint set per-command time on
⚠️ warning: per-thread run time information not available on this platform
... so that developers on other platforms at least have a hint
upfront.
This new warning also shows on platforms that don't have getrusage in
the first place, but does not show if the build doesn't support
threading at all.
New tests are added to gdb.base/maint.exp, to expect the warning, and
also to ensure other "mt per-command" sub commands don't trigger the
new warning.
Change-Id: Ie01b916b62f87006f855e31594a5ac7cf09e4c02
Approved-By: Simon Marchi <simon.marchi@efficios.com>
Approved-By: Tom Tromey <tom@tromey.com>
In update_wait_timeout function, microseconds were not taken into account
in poll timeout computation, resulting in 100% cpu time consumption in
the event loop while waiting for a sub-second timeout.
The bug has been introduced in commit c2c6d25.
This patch adds the microseconds converted to milliseconds in poll
timeout computation. Conversion by excess (ceil) is performed to
avoid the same problem with sub-millisecond timeouts too.
Gdbsupport functions phex and phex_nz have a parameter sizeof_l:
...
extern const char *phex (ULONGEST l, int sizeof_l);
extern const char *phex_nz (ULONGEST l, int sizeof_l);
...
and a lot of calls use:
...
phex (l, sizeof (l))
...
Make this easier by reimplementing the functions as a template, allowing us to
simply write:
...
phex (l)
...
Simplify existing code using:
...
$ find gdb* -type f \
| xargs sed -i 's/phex (\([^,]*\), sizeof (\1))/phex (\1)/'
$ find gdb* -type f \
| xargs sed -i 's/phex_nz (\([^,]*\), sizeof (\1))/phex_nz (\1)/'
...
and manually review:
...
$ find gdb* -type f | xargs grep "phex (.*, sizeof.*)"
$ find gdb* -type f | xargs grep "phex_nz (.*, sizeof.*)"
...
Tested on x86_64-linux.
Approved-By: Tom Tromey <tom@tromey.com>
New in v2:
- actually use m_enabled in the constructor and destructor
- output using gdb_stdlog->write_async_safe instead of gdb_printf
scoped_time_it is a small utility that measures and prints how much time
a given thread spent in a given scope. Similar to the time(1) command,
it prints the time spent in user mode, system mode, and the wall clock
time. It also prints the CPU utilization percentage, which is:
(user + sys) / wall
This can help spot cases where the workload is not well balanced between
workers, or the CPU utilization is not optimal (perhaps due to
contention around a lock for example).
To use it, just add it in some scope. For instance, a subsequent patch
adds it here:
workers.add_task ([this, task_count, iter, last] ()
{
scoped_time_it time_it ("DWARF indexing worker");
process_cus (task_count, iter, last);
});
On destruction, if enabled, it prints a line showing the time spent by
that thread, similar to what time(1) prints.
The example above prints this (one line for each worker thread):
Time for "DWARF indexing worker": wall 0.173, user 0.120, sys 0.034, user+sys 0.154, 89.0 % CPU
Time for "DWARF indexing worker": wall 0.211, user 0.144, sys 0.047, user+sys 0.191, 90.5 % CPU
Time for "DWARF indexing worker": wall 0.368, user 0.295, sys 0.057, user+sys 0.352, 95.7 % CPU
Time for "DWARF indexing worker": wall 0.445, user 0.361, sys 0.072, user+sys 0.433, 97.3 % CPU
Time for "DWARF indexing worker": wall 0.592, user 0.459, sys 0.113, user+sys 0.572, 96.6 % CPU
Time for "DWARF indexing worker": wall 0.739, user 0.608, sys 0.115, user+sys 0.723, 97.8 % CPU
Time for "DWARF indexing worker": wall 0.831, user 0.677, sys 0.140, user+sys 0.817, 98.3 % CPU
Time for "DWARF indexing worker": wall 0.949, user 0.789, sys 0.144, user+sys 0.933, 98.3 % CPU
The object is only enabled if per_command_time (controlled by "maint set
per-command time") is true at construction time. I wanted to avoid
adding a new command for now, but eventually if there are too many
scoped_time_it around the code base and we want to be able to enabled
them selectively (e.g. just the ones in the DWARF reader, or in the
symbol searching functions, etc), we could have a dedicated command for
that.
I added this functionality to GDB because it relies on gdb_printf and
per_command_time, but if we ever need it in gdbsupport, I'm sure we
could find a way to put it there.
Change-Id: I5416ac1448f960f44d85f8449943d994198a271e
Approved-By: Tom Tromey <tom@tromey.com>
It is completely unrelated to run_time_clock, so I don't think it makes
sense to have it as a static function there.
Move it to be a free function named "get_run_time".
Change-Id: I0c3e4d3cc44ca37e523c94d72f7cd66add95645e
Approved-By: Tom Tromey <tom@tromey.com>
This introduces a new method, attribute::signed_constant. This should
be used wherever DWARF specifies a signed integer constant, or where
this is implied by the context. It properly handles sign-extension
for DW_FORM_data*.
To my surprise, there doesn't seem to be a pre-existing sign-extension
function. I've added one to common-utils.h alongside the align
functions.
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=32680
This is a refactoring commit. When passing inferior arguments to
gdbserver we have two actions that need to be performed, splitting and
joining.
On the GDB side, we take the inferior arguments, a single string, and
split the string into a list of individual arguments. These are then
sent to gdbserver over the remote protocol.
On the gdbserver side we receive the list of individual arguments and
join these back together into a single inferior argument string.
In the next commit I plan to add some unit testing for this remote
argument passing process. Ideally, for unit testing, we need the code
being tested to be located in some easily callable function, rather
than being inline at the site of use.
So in this commit I propose to move the splitting and joining logic
out into a separate file, we can then use this within GDB and
gdbserver when passing arguments between GDB and gdbserver, but we can
also call the same functions for some unit testing.
In this commit I'm not adding the unit tests, they will be added next,
so for now there should be no user visible changes after this commit.
Tested-By: Guinevere Larsen <guinevere@redhat.com>
Commit d01e823438 ("Update copyright dates to include 2025") incorrectly
changed the dates in Makefile.in. Re-run `autoreconf` in the gdbsupport
directory to fix that up.
Change-Id: Ifcdb6f2257e7a456439dfc3e7848db934a4b16b4
This updates the copyright headers to include 2025. I did this by
running gdb/copyright.py and then manually modifying a few files as
noted by the script.
Approved-By: Eli Zaretskii <eliz@gnu.org>
A recent change (512ca2fca4 "gdb: split up
construct_inferior_arguments") introduced a build failure for mingw:
CXX common-inferior.o
.../gdb/gdbsupport/common-inferior.cc: In function ‘std::string escape_characters(const char*, const char*)’:
.../gdb/gdbsupport/common-inferior.cc:62:20: error: ‘argv’ was not declared in this scope; did you mean ‘arg’?
62 | if (strpbrk (argv[i], special))
| ^~~~
| arg
.../gdb/gdbsupport/common-inferior.cc:62:25: error: ‘i’ was not declared in this scope
62 | if (strpbrk (argv[i], special))
| ^
This patch fixes that.
Change-Id: I07ade607bc4516627b433085b07d9d198d8e4b4a
Approved-By: Tom de Vries <tdevries@suse.de>
Add a pre-commit codespell hook for directories gdbsupport and gdbserver,
which are codespell-clean:
...
$ pre-commit run codespell --all-files
codespell................................................................Passed
...
A non-trivial question is where the codespell configuration goes.
Currently we have codespell sections in gdbsupport/setup.cfg and
gdbserver/setup.cfg, but codespell doesn't automatically use those because the
pre-commit hook runs codespell at the root of the repository.
A solution would be to replace those 2 setup.cfg files with a setup.cfg in the
root of the repository. Not ideal because generally we try to avoid adding
files related to subdirectories at the root.
Another solution would be to add two codespell hooks, one using
--config gdbsupport/setup.cfg and one using --config gdbserver/setup.cfg, and
add a third one once we start supporting gdb. Not ideal because it creates
duplication, but certainly possible.
I went with the following solution: a setup.cfg file in gdb/contrib (alongside
codespell-ignore-words.txt) which is used for both gdbserver and gdbsupport.
So, what can this new setup do for us? Let's demonstrate by simulating a typo:
...
$ echo "/* aways */" >> gdbsupport/agent.cc
...
We can check unstaged changes before committing:
...
$ pre-commit run codespell --all-files
codespell................................................................Failed
- hook id: codespell
- exit code: 65
gdbsupport/agent.cc:282: aways ==> always, away
...
Likewise, staged changes (no need for the --all-files):
...
$ git add gdbsupport/agent.cc
$ pre-commit run codespell
codespell................................................................Failed
- hook id: codespell
- exit code: 65
gdbsupport/agent.cc:282: aways ==> always, away
...
Or we can try to commit, and run into the codespell failure:
...
$ git commit -a
black................................................(no files to check)Skipped
flake8...............................................(no files to check)Skipped
isort................................................(no files to check)Skipped
codespell................................................................Failed
- hook id: codespell
- exit code: 65
gdbsupport/agent.cc:282: aways ==> always, away
check-include-guards.................................(no files to check)Skipped
...
which makes the commit fail.
Approved-By: Tom Tromey <tom@tromey.com>
Tom Tromey mentioned [1] that the words "invokable" and "useable"
present in codespell-ignore-words.txt should be dropped.
Do so and fix the following typos:
...
$ codespell --config gdbsupport/setup.cfg gdbsupport
gdbsupport/common-debug.h:218: invokable ==> invocable
gdbsupport/event-loop.cc:84: useable ==> usable
...
Approved-By: Tom Tromey <tom@tromey.com>
[1] https://sourceware.org/pipermail/gdb-patches/2025-March/216584.html
The function construct_inferior_arguments (gdbsupport/common-inferior.cc)
currently escapes all special shell characters. After this commit
there will be two "levels" of quoting:
1. The current "full" quoting, where all posix shell special
characters are quoted, and
2. a new "reduced" quoting, where only the characters that GDB sees
as special (quotes and whitespace) are quoted.
After this, almost all construct_inferior_arguments calls will use the
"full" quoting, which is the current quoting. The "reduced" quoting
will be used in this commit to restore the behaviour that was lost in
the previous commit (more details below).
In the future, the reduced quoting will be useful for some additional
inferior argument that I have planned. I already posted my full
inferior argument work here:
https://inbox.sourceware.org/gdb-patches/cover.1730731085.git.aburgess@redhat.com
But that series is pretty long, and wasn't getting reviewed, so I'm
posted the series in parts now.
Before the previous commit, GDB behaved like this:
$ gdb -eiex 'set startup-with-shell off' --args /tmp/exec '$FOO'
(gdb) show args
Argument list to give program being debugged when it is started is "$FOO".
Notice that with 'startup-with-shell' off, the argument was left as
just '$FOO'. But after the previous commit, this changed to:
$ gdb -eiex 'set startup-with-shell off' --args /tmp/exec '$FOO'
(gdb) show args
Argument list to give program being debugged when it is started is "\$FOO".
Now the '$' is escaped with a backslash. This commit restores the
original behaviour, as this is (currently) the only way to unquoted
shell special characters into arguments from the GDB command line.
The series that I listed above includes a new command line option for
GDB which provides a better approach for controlling the quoting of
special shell characters, but that work requires these patches to be
merged first.
I've split out the core of construct_inferior_arguments into the new
function escape_characters, which takes a set of characters to escape.
Then the two functions escape_shell_characters and
escape_gdb_characters call escape_characters with the appropriate
character sets.
Finally, construct_inferior_arguments, now takes a boolean which
indicates if we should perform full shell escaping, or just perform
the reduced escaping.
I've updated all uses of construct_inferior_arguments to pass a
suitable value to indicate what escaping to perform (mostly just
'true', but one case in main.c is different), also I've updated
inferior::set_args to take the same boolean flag, and pass it through
to construct_inferior_arguments.
Tested-By: Guinevere Larsen <guinevere@redhat.com>
In the commit:
commit 0df62bf09e
Date: Fri Oct 22 07:19:33 2021 +0000
gdb: Support some escaping of args with startup-with-shell being off
nat/fork-inferior.c was updated such that when we are starting an
inferior without a shell we now remove escape characters. The
benefits of this are explained in that commit, but having made this
change we can now make an additional change.
Currently, in construct_inferior_arguments, when startup_with_shell is
false we construct the inferior argument string differently than when
startup_with_shell is true; when true we apply some escaping to
special shell character, when false we don't.
This commit simplifies construct_inferior_arguments by removing the
!startup_with_shell case, and instead we now apply escaping in all
cases. This is fine because, thanks to the above commit the escaping
will be correctly removed again when we call into nat/fork-inferior.c.
We should think of construct_inferior_arguments and
nat/fork-inferior.c as needing to cooperate in order for argument
handling to work correctly.
construct_inferior_arguments converts a list of separate arguments
into a single string, and nat/fork-inferior.c splits that single
string back into a list of arguments. It is critical that, if
nat/fork-inferior.c is expecting to remove a "layer" of escapes, then
construct_inferior_arguments must add that expected "layer",
otherwise, we end up stripping more escapes than expected.
The great thing (I think) about the new configuration, is that GDB no
longer cares about startup_with_shell at the point the arguments are
being setup. We only care about startup_with_shell at the point that
the inferior is started. This means that a user can set the inferior
arguments, and then change the startup-with-shell setting, and GDB
will do what they expect.
Under the previous system, where construct_inferior_arguments changed
its behaviour based on startup_with_shell, the user had to change the
setting, and then set the arguments, otherwise, GDB might not do what
they expect.
There is one slight issue with this commit though, which will be
addressed by the next commit.
For GDB's native targets construct_inferior_arguments is reached via
two code paths; first when GDB starts and we combine arguments from
the command line, and second when the Python API is used to set the
arguments from a sequence. It's the command line argument handling
which we are interested in.
Consider this:
$ gdb --args /tmp/exec '$FOO'
(gdb) show args
Argument list to give program being debugged when it is started is "\$FOO".
Notice that the argument has become \$FOO, the '$' is now quoted.
This is because, by quoting the argument in the shell command that
started GDB, GDB was passed a literal $FOO with no quotes. In order
to ensure that the inferior sees this same value, GDB added the extra
escape character. When GDB starts with a shell we pass \$FOO, which
results in the inferior seeing a literal $FOO.
But what if the user _actually_ wanted to have the shell GDB uses to
start the inferior expand $FOO? Well, it appears this can't be done
from the command line, but from the GDB prompt we can just do:
(gdb) set args $FOO
(gdb) show args
Argument list to give program being debugged when it is started is "$FOO".
And now the inferior will see the shell expanded version of $FOO.
It might seem like we cannot achieve the same result from the GDB
command line, however, it is possible with this trick:
$ gdb -eiex 'set startup-with-shell off' --args /tmp/exec '$FOO'
(gdb) show args
Argument list to give program being debugged when it is started is "$FOO".
(gdb) show startup-with-shell
Use of shell to start subprocesses is off.
And now the $FOO is not escaped, but GDB is no longer using a shell to
start the inferior, however, we can extend our command line like this:
$ gdb -eiex 'set startup-with-shell off' \
-ex 'set startup-with-shell on' \
--args /tmp/exec '$FOO'
(gdb) show args
Argument list to give program being debugged when it is started is "$FOO".
(gdb) show startup-with-shell
Use of shell to start subprocesses is on.
Use an early-initialisation option to disable startup-with-shell, this
is done before command line argument processing, then a normal
initialisation option turns startup-with-shell back on after GDB has
processed the command line arguments!
Is this useful? Yes, absolutely. Is this a good user experience?
Absolutely not. And I plan to add a new command line option to
GDB (and gdbserver) that will allow users to achieve the same
result (this trick doesn't work in gdbserver as there's no
early-initialisation there) without having to toggle the
startup-with-shell option. The new option can be found in the series
here:
https://inbox.sourceware.org/gdb-patches/cover.1730731085.git.aburgess@redhat.com
The problem is that, that series is pretty long, and getting it
reviewed is just not possible. So instead I'm posting the individual
patches in smaller blocks, to make reviews easier.
So, what's the problem? Well, by removing the !startup_with_shell
code path from GDB, there is no longer a construct_inferior_arguments
code path that doesn't quote inferior arguments, and so there's no
longer a way, from the command line, to set an unquoted '$FOO' as an
inferior argument. Obviously, this can still be done from GDB's CLI
prompt.
The trick above is completely untested, so this regression isn't going
to show up in the testsuite.
And the breakage is only temporary. In the next commit I'll add a fix
which restores the above trick.
Of course, I hope that this fix will itself, only be temporary. Once
the new command line options that I mentioned above are added, then
the fix I add in the next commit can be removed, and user should start
using the new command line option.
After this commit a whole set of tests that were added as xfail in the
above commit are now passing.
A change similar to this one can be found in this series:
https://inbox.sourceware.org/gdb-patches/20211022071933.3478427-1-m.weghorn@posteo.de/
which I reviewed before writing this patch. I don't think there's any
one patch in that series that exactly corresponds with this patch
though, so I've listed the author of the original series as co-author
on this patch.
Co-Authored-By: Michael Weghorn <m.weghorn@posteo.de>
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=28392
Tested-By: Guinevere Larsen <guinevere@redhat.com>
Add a few -Wunused-* diagnostic flags that look useful. Some are known
to gcc, some to clang, some to both. Fix the fallouts.
-Wunused-const-variable=1 is understood by gcc, but not clang.
-Wunused-const-variable would be undertsood by both, but for gcc at
least it would flag the unused const variables in headers. This doesn't
make sense to me, because as soon as one source file includes a header
but doesn't use a const variable defined in that header, it's an error.
With `=1`, gcc only warns about unused const variable in the main source
file. It's not a big deal that clang doesn't understand it though: any
instance of that problem will be flagged by any gcc build.
Change-Id: Ie20d99524b3054693f1ac5b53115bb46c89a5156
Approved-By: Tom Tromey <tom@tromey.com>
The cooked index needs to allocate names in some cases -- when
canonicalizing or when synthesizing Ada package names. This process
currently uses a vector of unique_ptrs to manage the memory.
Another series I'm writing adds another spot where this allocation
must be done, and examining the result showed that certain names were
allocated multiple times.
To clean this up, this patch introduces a string cache object and
changes the cooked indexer to use it. I considered using bcache here,
but bcache doesn't work as nicely with string_view -- because bcache
is fundamentally memory-based, a temporary copy of the contents must
be made to ensure that bcache can see the trailing \0. Furthermore,
writing a custom class lets us avoid another copy when canonicalizing
C++ names.
Approved-By: Simon Marchi <simon.marchi@efficios.com>
I noticed that check-include-guards.py doesn't error in certain
situations -- but in situations where the --update flag would cause a
file to be changed.
This patch changes the script to issue an error for any discrepancy.
It also fixes the headers that weren't correct.
Approved-By: Simon Marchi <simon.marchi@efficios.com>
When running codespell on gdbsupport, we get:
...
$ codespell gdbsupport
gdbsupport/common-debug.h:218: invokable ==> invocable
gdbsupport/osabi.h:51: configury ==> configurable
gdbsupport/ChangeLog-2020-2021:344: ro ==> to, row, rob, rod, roe, rot
gdbsupport/ChangeLog-2020-2021:356: contaning ==> containing
gdbsupport/common.m4:19: configury ==> configurable
gdbsupport/Makefile.am:97: configury ==> configurable
gdbsupport/Makefile.in:811: configury ==> configurable
gdbsupport/event-loop.cc:84: useable ==> usable
gdbsupport/configure:15904: assigment ==> assignment
...
Some of these files we want to skip in a spell check, because they're
generated. We also want to skip ChangeLogs, we don't actively maintain those.
Add a file gdbsupport/setup.cfg with a codespell section, that skips those
files. The choice for setup.cfg (rather than say .codespellrc) comes from the
presence of gdb/setup.cfg.
That leaves invokable, configury and useable. I think configury is a common
expression in our context, and for invokable and useable I don't manage to
find out whether they really need rewriting, so I'd rather leave them alone
for now.
Add these to a file gdb/contrib/codespell-ignore-words.txt, and use the file in
gdbsupport/setup.cfg.
This makes the directory codespell clean:
...
$ codespell --config gdbsupport/setup.cfg gdbsupport
$
...
Because codespell seems to interpret filenames relative to the working
directory rather than relative to the config file, and the filename used in
gdbsupport/setup.cfg is gdb/contrib/codespell-ignore-words.txt, this simple
invocation doesn't work:
...
$ cd gdbsupport
$ codespell
...
because codespell can't find gdbsupport/gdb/contrib/codespell-ignore-words.txt.
We could fix this by using ../gdb/contrib/codespell-ignore-words.txt instead, but
likewise that breaks this invocation:
...
$ codespell --config gdbsupport/setup.cfg gdbsupport
...
I can't decide which one is worse, so I'm sticking with
gdb/contrib/codespell-ignore-words.txt for now.
Approved-By: Simon Marchi <simon.marchi@efficios.com>
I noticed a
// namespace selftests
comment, which doesn't follow our comment formatting convention. I did
a find & replace to fix all the offenders.
Change-Id: Idf8fe9833caf1c3d99e15330db000e4bab4ec66c
On windows, when creating a directory with an absolute path,
mkdir_recursive would start by trying to make 'C:'.
On windows, this has a special meaning, which is "the current
directory on the C drive". So the first thing it tries to do
is create the current directory.
Most of the time, this fails with EEXIST, so the function
continues as expected. However if the current directory is
C:/, trying to create that causes EPERM, which causes the
function to prematurely terminate.
(The same applies for any drive letter.)
This patch resolves this issue, by skipping the drive letter
so that it is never sent to the mkdir call.
Approved-By: Tom Tromey <tom@tromey.com>
