| Age | Commit message (Collapse) | Author | Files | Lines |
|---|
|
Same as previous patches, but for sync_execution_done. Except that
here, we only want to notify the interpreter that is executing the
command, not all interpreters.
Change-Id: I729c719447b5c5f29af65dbf6fed9132e2cd308b
|
|
Same as previous patches, but for no_history.
Change-Id: I06930fe7cb4082138c6c5496c5118fe4951c10da
|
|
Same as previous patch, but for exited. Remove the exited observable,
since nothing uses it anymore, and we don't have anything coming that
will use it.
Change-Id: I358cbea0159af56752dfee7510d6a86191e722bb
|
|
Same as previous patch, but for signal_exited. Remove the signal_exited
observable, since nothing uses it anymore, and we don't have anything
coming that will use it.
Change-Id: I0dca1eab76338bf27be755786e3dad3241698b10
|
|
Same idea as the previous patch, but for the normal_stop event.
Change-Id: I4fc8ca8a51c63829dea390a2b6ce30b77f9fb863
|
|
Instead of having the interpreter code registering observers for the
signal_received observable, add a "signal_received" virtual method to
struct interp. Add a interps_notify_signal_received function that loops
over all UIs and calls the signal_received method on the interpreter.
Finally, add a notify_signal_received function that calls
interps_notify_signal_received and then notifies the observers. Replace
all existing notifications to the signal_received observers with calls
to notify_signal_received.
Before this patch, the CLI and MI code both register a signal_received
observer. These observer go over all UIs, and, for those that have a
interpreter of the right kind, print the stop notifiation.
After this patch, we have just one "loop over all UIs", inside
interps_notify_signal_received. Since the interp::on_signal_received
method gets called once for each interpreter, the implementations only
need to deal with the current interpreter (the "this" pointer).
The motivation for this patch comes from a future patch, that makes the
amdgpu code register an observer to print a warning after the CLI's
signal stop message. Since the amdgpu and the CLI code both use
observers, the order of the two messages is not stable, unless we define
the priority using the observer dependency system. However, the
approach of using virtual methods on the interpreters seems like a good
change anyway, I think it's more straightforward and simple to
understand than the current solution that uses observers. We are sure
that the amdgpu message gets printed after the CLI message, since
observers are notified after interpreters.
Keep the signal_received, even if nothing uses if, because we will be
using it in the upcoming amdgpu patch implementing the warning described
above.
Change-Id: I4d8614bb8f6e0717f4bfc2a59abded3702f23ac4
|
|
I stumbled on the mi_proceeded and running_result_record_printed
globals, which are shared by all MI interpreter instances (it's unlikely
that people use multiple MI interpreter instances, but it's possible).
After poking at it, I found this bug:
1. Start GDB in MI mode
2. Add a second MI interpreter with the new-ui command
3. Use -exec-run on the second interpreter
This is the output I get on the first interpreter:
=thread-group-added,id="i1"
~"Reading symbols from a.out...\n"
~"New UI allocated\n"
(gdb)
=thread-group-started,id="i1",pid="94718"
=thread-created,id="1",group-id="i1"
^running
*running,thread-id="all"
And this is the output I get on the second intepreter:
=thread-group-added,id="i1"
(gdb)
-exec-run
=thread-group-started,id="i1",pid="94718"
=thread-created,id="1",group-id="i1"
*running,thread-id="all"
The problem here is that the `^running` reply to the -exec-run command
is printed on the wrong UI. It is printed on the first one, it should
be printed on the second (the one on which we sent the -exec-run).
What happens under the hood is that captured_mi_execute_command, while
executing a command for the second intepreter, clears the
running_result_record_printed and mi_proceeded globals.
mi_about_to_proceed then sets mi_proceeded. Then, mi_on_resume_1 gets
called for the first intepreter first. Since the
!running_result_record_printed && mi_proceeded
condition is true, it prints a ^running, and sets
running_result_record_printed. When mi_on_resume_1 gets called for the
second interpreter, running_result_record_printed is already set, so
^running is not printed there.
It took me a while to understand the relationship between these two
variables. I think that in the end, this is what we want to track:
1. When executing an MI command, take note if that command causes a
"proceed". This is done in mi_about_to_proceed.
2. In mi_on_resume_1, if the command indeed caused a "proceed", we want
to output a ^running record. And we want to remember that we did,
because...
3. Back in captured_mi_execute_command, if we did not output a
^running, we want to output a ^done.
Moving those two variables to the mi_interp struture appears to fix it.
Only for the interpreter doing the -exec-run command does the
running_result_record_printed flag get cleared, and therefore only or
that one does the ^running record get printed.
Add a new test for this, that does pretty much what the reproducer above
shows. Without the fix, the test fails because
mi_send_resuming_command_raw never sees the ^running record.
Change-Id: I63ea30e6cb61a8e1dd5ef03377e6003381a9209b
Tested-By: Alexandra Petlanova Hajkova <ahajkova@redhat.com>
|
|
I've had this patch for a while now and figured I'd update it and send
it. It changes MI commands to use a "const char * const" for their
argv parameter.
Regression tested on x86-64 Fedora 36.
Acked-By: Simon Marchi <simon.marchi@efficios.com>
|
|
This adds a new Python function, gdb.execute_mi, that can be used to
invoke an MI command but get the output as a Python object, rather
than a string. This is done by implementing a new ui_out subclass
that builds a Python object.
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=11688
Reviewed-By: Eli Zaretskii <eliz@gnu.org>
|
|
This adds a second mi_parse constructor. This constructor takes a
command name and vector of arguments, and does not do any escape
processing. This also changes mi_parse::args to handle parse objects
created this new way.
|
|
This introduces some helper methods for mi_parse that handle some of
the details of parsing. This approach lets us reuse them later.
|
|
Change the mi_parse function to be a static method of mi_parse. This
lets us remove the 'set_args' setter function.
|
|
This changes mi_parse_argv to be a method of mi_parse. This is just a
minor cleanup.
|
|
This changes mi_parse::args to be a private member, retrieved via
accessor. It also changes this member to be a std::string. This
makes it simpler for a subsequent patch to implement different
behavior for argument parsing.
|
|
This changes mi_parse to use member initializers rather than a
constructor. This is easier to follow.
|
|
SUMMARY
The '--simple-values' argument to '-stack-list-arguments' and similar
GDB/MI commands does not take reference types into account, so that
references to arbitrarily large structures are considered "simple" and
printed. This means that the '--simple-values' argument cannot be used
by IDEs when tracing the stack due to the time taken to print large
structures passed by reference.
DETAILS
Various GDB/MI commands ('-stack-list-arguments', '-stack-list-locals',
'-stack-list-variables' and so on) take a PRINT-VALUES argument which
may be '--no-values' (0), '--all-values' (1) or '--simple-values' (2).
In the '--simple-values' case, the command is supposed to print the
name, type, and value of variables with simple types, and print only the
name and type of variables with compound types.
The '--simple-values' argument ought to be suitable for IDEs that need
to update their user interface with the program's call stack every time
the program stops. However, it does not take C++ reference types into
account, and this makes the argument unsuitable for this purpose.
For example, consider the following C++ program:
struct s {
int v[10];
};
int
sum(const struct s &s)
{
int total = 0;
for (int i = 0; i < 10; ++i) total += s.v[i];
return total;
}
int
main(void)
{
struct s s = { { 1, 2, 3, 4, 5, 6, 7, 8, 9, 10 } };
return sum(s);
}
If we start GDB in MI mode and continue to 'sum', the behaviour of
'-stack-list-arguments' is as follows:
(gdb)
-stack-list-arguments --simple-values
^done,stack-args=[frame={level="0",args=[{name="s",type="const s &",value="@0x7fffffffe310: {v = {1, 2, 3, 4, 5, 6, 7, 8, 9, 10}}"}]},frame={level="1",args=[]}]
Note that the value of the argument 's' was printed, even though 's' is
a reference to a structure, which is not a simple value.
See https://github.com/microsoft/MIEngine/pull/673 for a case where this
behaviour caused Microsoft to avoid the use of '--simple-values' in
their MIEngine debug adapter, because it caused Visual Studio Code to
take too long to refresh the call stack in the user interface.
SOLUTIONS
There are two ways we could fix this problem, depending on whether we
consider the current behaviour to be a bug.
1. If the current behaviour is a bug, then we can update the behaviour
of '--simple-values' so that it takes reference types into account:
that is, a value is simple if it is neither an array, struct, or
union, nor a reference to an array, struct or union.
In this case we must add a feature to the '-list-features' command so
that IDEs can detect that it is safe to use the '--simple-values'
argument when refreshing the call stack.
2. If the current behaviour is not a bug, then we can add a new option
for the PRINT-VALUES argument, for example, '--scalar-values' (3),
that would be suitable for use by IDEs.
In this case we must add a feature to the '-list-features' command
so that IDEs can detect that the '--scalar-values' argument is
available for use when refreshing the call stack.
PATCH
This patch implements solution (1) as I think the current behaviour of
not printing structures, but printing references to structures, is
contrary to reasonable expectation.
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=29554
|
|
I'd like to move some things so they become methods on struct ui. But
first, I think that struct ui and the related things are big enough to
deserve their own file, instead of being scattered through top.{c,h} and
event-top.c.
Change-Id: I15594269ace61fd76ef80a7b58f51ff3ab6979bc
|
|
Like evaluate_expression, evaluate_type is also just a simple wrapper.
Removing it makes the code a little nicer.
|
|
evaluate_expression is just a little wrapper for a method on
expression. Removing it also removes a lot of ugly (IMO) calls to
get().
|
|
Latest libc++[1] causes transitive include to <locale> when
<mutex> or <thread> header is included. This causes
gdb to not build[2] since <locale> defines isupper/islower etc.
functions that are explicitly macroed-out in safe-ctype.h to
prevent their use.
Use the suggestion from libc++ to include <locale> internally when
building in C++ mode to avoid build errors.
Use safe-gdb-ctype.h as the include instead of "safe-ctype.h"
to keep this isolated to gdb since rest of binutils
does not seem to use much C++.
[1]: https://reviews.llvm.org/D144331
[2]: https://issuetracker.google.com/issues/277967395
|
|
I noticed the following behaviour:
$ gdb -q -i=mi /tmp/hello.x
=thread-group-added,id="i1"
=cmd-param-changed,param="print pretty",value="on"
~"Reading symbols from /tmp/hello.x...\n"
(gdb)
-break-insert -p 99 main
^done,bkpt={number="1",type="breakpoint",disp="keep",enabled="y",addr="0x0000000000401198",func="main",file="/tmp/hello.c",fullname="/tmp/hello.c",line="18",thread-groups=["i1"],thread="99",times="0",original-location="main"}
(gdb)
info breakpoints
&"info breakpoints\n"
~"Num Type Disp Enb Address What\n"
~"1 breakpoint keep y 0x0000000000401198 in main at /tmp/hello.c:18\n"
&"../../src/gdb/thread.c:1434: internal-error: print_thread_id: Assertion `thr != nullptr' failed.\nA problem internal to GDB has been detected,\nfurther debugging may prove unreliable."
&"\n"
&"----- Backtrace -----\n"
&"Backtrace unavailable\n"
&"---------------------\n"
&"\nThis is a bug, please report it."
&" For instructions, see:\n"
&"<https://www.gnu.org/software/gdb/bugs/>.\n\n"
Aborted (core dumped)
What we see here is that when using the MI a user can create
thread-specific breakpoints for non-existent threads. Then if we try
to use the CLI 'info breakpoints' command GDB throws an assertion.
The assert is a result of the print_thread_id call when trying to
build the 'stop only in thread xx.yy' line; print_thread_id requires a
valid thread_info pointer, which we can't have for a non-existent
thread.
In contrast, when using the CLI we see this behaviour:
$ gdb -q /tmp/hello.x
Reading symbols from /tmp/hello.x...
(gdb) break main thread 99
Unknown thread 99.
(gdb)
The CLI doesn't allow a breakpoint to be created for a non-existent
thread. So the 'info breakpoints' command is always fine.
Interestingly, the MI -break-info command doesn't crash, this is
because the MI uses global thread-ids, and so never calls
print_thread_id. However, GDB does support using CLI and MI in
parallel, so we need to solve this problem.
One option would be to change the CLI behaviour to allow printing
breakpoints for non-existent threads. This would preserve the current
MI behaviour.
The other option is to pull the MI into line with the CLI and prevent
breakpoints being created for non-existent threads. This is good for
consistency, but is a breaking change for the MI.
In the end I figured that it was probably better to retain the
consistent CLI behaviour, and just made the MI reject requests to
place a breakpoint on a non-existent thread. The only test we had
that depended on the old behaviour was
gdb.mi/mi-thread-specific-bp.exp, which was added by me in commit:
commit 2fd9a436c8d24eb0af85ccb3a2fbdf9a9c679a6c
Date: Fri Feb 17 10:48:06 2023 +0000
gdb: don't duplicate 'thread' field in MI breakpoint output
I certainly didn't intend for this test to rely on this feature of the
MI, so I propose to update this test to only create breakpoints for
threads that exist.
Actually, I've added a new test that checks the MI rejects creating a
breakpoint for a non-existent thread, and I've also extended the test
to run with the separate MI/CLI UIs, and then tested 'info
breakpoints' to ensure this command doesn't crash.
I've extended the documentation of the `-p` flag to explain the
constraints better.
I have also added a NEWS entry just in case someone runs into this
issue, at least then they'll know this change in behaviour was
intentional.
One thing that I did wonder about while writing this patch, is whether
we should treat requests like this, on both the MI and CLI, as another
form of pending breakpoint, something like:
(gdb) break foo thread 9
Thread 9 does not exist.
Make breakpoint pending on future thread creation? (y or [n]) y
Breakpoint 1 (foo thread 9) pending.
(gdb) info breakpoints
Num Type Disp Enb Address What
1 breakpoint keep y <PENDING> foo thread 9
Don't know if folk think that would be a useful idea or not? Either
way, I think that would be a separate patch from this one.
Reviewed-By: Eli Zaretskii <eliz@gnu.org>
|
|
I noticed that this observable was never notified, which means we can
probably safely remove it. The notification was removed in:
commit 243a925328f8e3184b2356bee497181049c0174f
Author: Pedro Alves <palves@redhat.com>
Date: Wed Sep 9 18:23:24 2015 +0100
Replace "struct continuation" mechanism by something more extensible
print_end_stepping_range_reason in turn becomes unused, so remote it as
well.
Change-Id: If5da5149276c282d2540097c8c4327ce0f70431a
|
|
I think that the language_auto enumerator and the auto_language class
can be removed. There isn't really an "auto" language, it's only a
construct of the "set language" command to say "pick the appropriate
language automatically". But "auto" is never the current language. The
`current_language` points to the current effective language, and the
fact that we're in "auto language" mode is noted by the language_mode
global.
- Change set_language to handle the "auto" (and "local", which is a
synonym) early, instead of in the for loop. I think it makes the two
cases (auto vs explicit language) more clearly separated anyway.
- Adjust add_set_language_command to hard-code the "auto" string,
instead of using the "auto" language definition.
- Remove auto_language, rename auto_or_unknown_language to
unknown_language and move the bits of the existing unknown_language
in there.
- Remove the set_language at the end of _initialize_language. I think
it's not needed, because we call set_language in gdb_init, after all
_initialize functions are called. There is some chance that an
_initialize function that runs after _initialize_language implicitly
depends on current_language being set, but my testsuite runs haven't
found anything like that.
- Use language_unknown instead of language_auto when creating a minimal
symbol (minimal_symbol_reader::record_full). I think that this value
is used to indicate that we don't know the symbol of the minimal
symbol (yet), so language_unknown makes sense to me. Update a
condition accordingly in ada-lang.c. symbol_find_demangled_name also
appears to "normalize" this value from "unknown" to "auto", remove
that part and update the condition to just check for
language_unknown.
Change-Id: I47bcd6c15f607d9818f2e6e413053c2dc8ec5034
Reviewed-By: Tom Tromey <tom@tromey.com>
|
|
Make find_thread_ptid (the overload that takes a process_stratum_target)
a method of process_stratum_target.
Change-Id: Ib190a925a83c6b93e9c585dc7c6ab65efbdd8629
Reviewed-By: Tom Tromey <tom@tromey.com>
|
|
The mi_version function is unused, and I think it's better overall if
it is never used. This patch removes it. Tested by rebuilding.
Approved-By: Simon Marchi <simon.marchi@efficios.com>
|
|
This changes linetables to not add the text offset to the addresses
they contain. I did this in a few steps, necessarily combined
together in one patch: I renamed the 'pc' member to 'm_pc', added the
appropriate accessors, and then recompiled. Then I fixed all the
errors. Where possible I generally chose to use the raw_pc accessor,
as it is less expensive.
Note that this patch discounts the possibility that the text section
offset might cause wraparound in the addresses in the line table.
However, this was already discounted -- in particular,
objfile_relocate1 did not re-sort the table in this scenario. (There
was a bug open about this, but as far as I can tell this has never
happened, it's not even clear what inspired that bug.)
Approved-By: Simon Marchi <simon.marchi@efficios.com>
|
|
Replace spaces with tabs in a bunch of places.
Change-Id: If0f87180f1d13028dc178e5a8af7882a067868b0
|
|
This commit contains changes which have an explicit throw for
gdb_exception_forced_quit, or, in a couple of cases for gdb_exception,
but with a throw following a check to see if 'reason' is
RETURN_FORCED_QUIT.
Most of these are straightforward - it made sense to continue to allow
an existing catch of gdb_exception to also catch gdb_exception_quit;
in these cases, a catch/throw for gdb_exception_forced_quit was added.
There are two cases, however, which deserve a more detailed
explanation.
1) remote_fileio_request in gdb/remote-fileio.c:
The try block calls do_remote_fileio_request which can (in turn)
call one of the functions in remote_fio_func_map[]. Taking the
first one, remote_fileio_func_open(), we have the following call
path to maybe_quit():
remote_fileio_func_open(remote_target*, char*)
-> target_read_memory(unsigned long, unsigned char*, long)
-> target_read(target_ops*, target_object, char const*, unsigned char*, unsigned long, long)
-> maybe_quit()
Since there is a path to maybe_quit(), we must ensure that the
catch block is not permitted to swallow a QUIT representing a
SIGTERM.
However, for this case, we must take care not to change the way that
Ctrl-C / SIGINT is handled; we want to send a suitable EINTR reply to
the remote target should that happen. That being the case, I added a
catch/throw for gdb_exception_forced_quit. I also did a bit of
rewriting here, adding a catch for gdb_exception_quit in favor of
checking the 'reason' code in the catch block for gdb_exception.
2) mi_execute_command in gdb/mi/mi-main.c:
The try block calls captured_mi_execute_command(); there exists
a call path to maybe_quit():
captured_mi_execute_command(ui_out*, mi_parse*)
-> mi_cmd_execute(mi_parse*)
-> get_current_frame()
-> get_prev_frame_always_1(frame_info*)
-> frame_register_unwind_location(frame_info*, int, int*, lval_type*, unsigned long*, int*)
-> frame_register_unwind(frame_info*, int, int*, int*, lval_type*, unsigned long*, int*, unsigned char*)
-> value_entirely_available(value*)
-> value_fetch_lazy(value*)
-> value_fetch_lazy_memory(value*)
-> read_value_memory(value*, long, int, unsigned long, unsigned char*, unsigned long)
-> maybe_quit()
That being the case, we can't allow the exception handler (catch block)
to swallow a gdb_exception_quit for SIGTERM. However, it does seem
reasonable to output the exception via the mi interface so that some
suitable message regarding SIGTERM might be printed; therefore, I
check the exception's 'reason' field for RETURN_FORCED_QUIT and
do a throw for this case.
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=26761
Tested-by: Tom de Vries <tdevries@suse.de>
Approved-By: Pedro Alves <pedro@palves.net>
|
|
As described in the previous commit for this series, I became
concerned that there might be instances in which a QUIT (due to either
a SIGINT or SIGTERM) might not cause execution to return to the top
level. In some (though very few) instances, it is okay to not
propagate the exception for a Ctrl-C / SIGINT, but I don't think that
it is ever okay to swallow the exception caused by a SIGTERM.
Allowing that to happen would definitely be a deviation from the
current behavior in which GDB exits upon receipt of a SIGTERM.
I looked at all cases where an exception handler catches a
gdb_exception. Handlers which did NOT need modification were those
which satisifed one or more of the following conditions:
1) There is no call path to maybe_quit() in the try block. I used a
static analysis tool to help make this determination. In
instances where the tool didn't provide an answer of "yes, this
call path can result in maybe_quit() being called", I reviewed it
by hand.
2) The catch block contains a throw for conditions that it
doesn't want to handle; these "not handled" conditions
must include the quit exception and the new "forced quit" exception.
3) There was (also) a catch for gdb_exception_quit.
Any try/catch blocks not meeting the above conditions could
potentially swallow a QUIT exception.
My first thought was to add catch blocks for gdb_exception_quit and
then rethrow the exception. But Pedro pointed out that this can be
handled without adding additional code by simply catching
gdb_exception_error instead. That's what this patch series does.
There are some oddball cases which needed to be handled differently,
plus the extension languages, but those are handled in later patches.
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=26761
Tested-by: Tom de Vries <tdevries@suse.de>
Approved-by: Pedro Alves <pedro@palves.net>
|
|
This removes ALL_BLOCK_SYMBOLS in favor of foreach.
|
|
This turns many functions that are related to optimized-out or
availability-checking to be methods of value. The static function
value_entirely_covered_by_range_vector is also converted to be a
private method.
Approved-By: Simon Marchi <simon.marchi@efficios.com>
|
|
This changes value_contents_eq to be a method of value. It also
converts the static function value_contents_bits_eq into a private
method.
Approved-By: Simon Marchi <simon.marchi@efficios.com>
|
|
This changes various offset-related functions to be methods of value.
Much of this patch was written by script.
Approved-By: Simon Marchi <simon.marchi@efficios.com>
|
|
This changes value_type to be a method of value. Much of this patch
was written by script.
Approved-By: Simon Marchi <simon.marchi@efficios.com>
|
|
This patch implements a simplication that I suggested here:
https://sourceware.org/pipermail/gdb-patches/2022-March/186320.html
Currently, the interp::exec virtual method interface is such that
subclass implementations must catch exceptions and then return them
via normal function return.
However, higher up the in chain, for the CLI we get to
interpreter_exec_cmd, which does:
for (i = 1; i < nrules; i++)
{
struct gdb_exception e = interp_exec (interp_to_use, prules[i]);
if (e.reason < 0)
{
interp_set (old_interp, 0);
error (_("error in command: \"%s\"."), prules[i]);
}
}
and for MI we get to mi_cmd_interpreter_exec, which has:
void
mi_cmd_interpreter_exec (const char *command, char **argv, int argc)
{
...
for (i = 1; i < argc; i++)
{
struct gdb_exception e = interp_exec (interp_to_use, argv[i]);
if (e.reason < 0)
error ("%s", e.what ());
}
}
Note that if those errors are reached, we lose the original
exception's error code. I can't see why we'd want that.
And, I can't see why we need to have interp_exec catch the exception
and return it via the normal return path. That's normally needed when
we need to handle propagating exceptions across C code, like across
readline or ncurses, but that's not the case here.
It seems to me that we can simplify things by removing some
try/catch-ing and just letting exceptions propagate normally.
Note, the "error in command" error shown above, which only exists in
the CLI interpreter-exec command, is only ever printed AFAICS if you
run "interpreter-exec console" when the top level interpreter is
already the console/tui. Like:
(gdb) interpreter-exec console "foobar"
Undefined command: "foobar". Try "help".
error in command: "foobar".
You won't see it with MI's "-interpreter-exec console" from a top
level MI interpreter:
(gdb)
-interpreter-exec console "foobar"
&"Undefined command: \"foobar\". Try \"help\".\n"
^error,msg="Undefined command: \"foobar\". Try \"help\"."
(gdb)
nor with MI's "-interpreter-exec mi" from a top level MI interpreter:
(gdb)
-interpreter-exec mi "-foobar"
^error,msg="Undefined MI command: foobar",code="undefined-command"
^done
(gdb)
in both these cases because MI's -interpreter-exec just does:
error ("%s", e.what ());
You won't see it either when running an MI command with the CLI's
"interpreter-exec mi":
(gdb) interpreter-exec mi "-foobar"
^error,msg="Undefined MI command: foobar",code="undefined-command"
(gdb)
This last case is because MI's interp::exec implementation never
returns an error:
gdb_exception
mi_interp::exec (const char *command)
{
mi_execute_command_wrapper (command);
return gdb_exception ();
}
Thus I think that "error in command" error is pretty pointless, and
since it simplifies things to not have it, the patch just removes it.
The patch also ends up addressing an old FIXME.
Change-Id: I5a6432a80496934ac7127594c53bf5221622e393
Approved-By: Tom Tromey <tromey@adacore.com>
Approved-By: Kevin Buettner <kevinb@redhat.com>
|
|
I noticed some unusual code in mi-cmd-stack.c. This code is a switch,
where one of the cases appears in the middle of another block. It
seemed cleaner to me to have the earlier case just conditionally fall
through.
|
|
This is the second step of making frame_info_ptr automatic, reinflate on
demand whenever trying to obtain the wrapper frame_info pointer, either
through the get method or operator->. Make the reinflate method
private, it is used as a convenience method in those two.
Add an "is_null" method, because it is often needed to know whether the
frame_info_ptr wraps an frame_info or is empty.
Make m_ptr mutable, so that it's possible to reinflate const
frame_info_ptr objects. Whether m_ptr is nullptr or not does not change
the logical state of the object, because we re-create it on demand. I
believe this is the right use case for mutable.
Change-Id: Icb0552d0035e227f81eb3c121d8a9bb2f9d25794
Reviewed-By: Bruno Larsen <blarsen@redhat.com>
|
|
This is the first step of making frame_info_ptr automatic. Remove the
frame_info_ptr::prepare_reinflate method, move that code to the
constructor.
Change-Id: I85cdae3ab1c043c70e2702e7fb38e9a4a8a675d8
Reviewed-By: Bruno Larsen <blarsen@redhat.com>
|
|
When executing in reverse and runs out of recorded history, GDB prints
a warning to the user, but does not add a reason in the stopped record,
for example:
*stopped,frame={addr="0x000000000040113e",func="main",args=[],file="/home/blarsen/Documents/fsf_build/gdb/testsuite/../../../binutils-gdb/gdb/testsuite/gdb.reverse/solib-reverse.c",fullname="/home/blarsen/Documents/binutils-gdb/gdb/testsuite/gdb.reverse/solib-reverse.c",line="27",arch="i386:x86-64"},thread-id="1",stopped-threads="all",core="1"
This problem was reported as record/29260.
This commit adds the reason no-history to the record, making it easier
for interfaces using the mi interpreter to report the result. It also
changes the test gdb.mi/mi-reverse.exp to test that the reason shows up
correctly.
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=29260
|
|
This commit is the result of running the gdb/copyright.py script,
which automated the update of the copyright year range for all
source files managed by the GDB project to be updated to include
year 2023.
|
|
This changes the uses of value_print_options to use 'true' and 'false'
rather than integers.
|
|
MI version 1 is long since obsolete. Several years ago, I filed
PR mi/23170 for this. I think it's finally time to remove this.
Any users of MI 1 can and should upgrade to a newer version.
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=23170
|
|
[I sent this earlier today, but I don't see it in the archives.
Resending it through a different computer / SMTP.]
The use of the static buffer in command_line_input is becoming
problematic, as explained here [1]. In short, with this patch [2] that
attempt to fix a post-hook bug, when running gdb.base/commands.exp, we
hit a case where we read a "define" command line from a script file
using command_command_line_input. The command line is stored in
command_line_input's static buffer. Inside the define command's
execution, we read the lines inside the define using command_line_input,
which overwrites the define command, in command_line_input's static
buffer. After the execution of the define command, execute_command does
a command look up to see if a post-hook is registered. For that, it
uses a now stale pointer that used to point to the define command, in
the static buffer, causing a use-after-free. Note that the pointer in
execute_command points to the dynamically-allocated buffer help by the
static buffer in command_line_input, not to the static object itself,
hence why we see a use-after-free.
Fix that by removing the static buffer. I initially changed
command_line_input and other related functions to return an std::string,
which is the obvious but naive solution. The thing is that some callees
don't need to return an allocated string, so this this an unnecessary
pessimization. I changed it to passing in a reference to an std::string
buffer, which the callee can use if it needs to return
dynamically-allocated content. It fills the buffer and returns a
pointers to the C string inside. The callees that don't need to return
dynamically-allocated content simply don't use it.
So, it started with modifying command_line_input as described above, all
the other changes derive directly from that.
One slightly shady thing is in handle_line_of_input, where we now pass a
pointer to an std::string's internal buffer to readline's history_value
function, which takes a `char *`. I'm pretty sure that this function
does not modify the input string, because I was able to change it (with
enough massaging) to take a `const char *`.
A subtle change is that we now clear a UI's line buffer using a
SCOPE_EXIT in command_line_handler, after executing the command.
This was previously done by this line in handle_line_of_input:
/* We have a complete command line now. Prepare for the next
command, but leave ownership of memory to the buffer . */
cmd_line_buffer->used_size = 0;
I think the new way is clearer.
[1] https://inbox.sourceware.org/gdb-patches/becb8438-81ef-8ad8-cc42-fcbfaea8cddd@simark.ca/
[2] https://inbox.sourceware.org/gdb-patches/20221213112241.621889-1-jan.vrany@labware.com/
Change-Id: I8fc89b1c69870c7fc7ad9c1705724bd493596300
Reviewed-By: Tom Tromey <tom@tromey.com>
|
|
This changes mi_interp to use ui_file_up rather than explicit
management.
Approved-By: Simon Marchi <simon.marchi@efficios.com>
|
|
Right now, tee_file owns the second stream it writes to. This is done
for the convenience of the users. In a subsequent patch, this will no
longer be convenient, so this patch moves the responsibility for
ownership to the users of tee_file.
|
|
If the download size is known, a progress bar is displayed along with
the percentage of completion and the total download size.
Downloading separate debug info for /lib/libxyz.so
[############ ] 25% (10.01 M)
If the download size is not known, a progress indicator is displayed
with a ticker ("###") that moves across the screen at a rate of 1 tick
every 0.5 seconds.
Downloading separate debug info for /lib/libxyz.so
[ ### ]
If the output stream is not a tty, batch mode is enabled, the screen is
too narrow or width has been set to 'unlimited', then only a static
description of the download is printed. No bar or ticker is displayed.
Downloading separate debug info for /lib/libxyz.so...
In any case, if the size of the download is known at the time the
description is printed then it will be included in the description.
Downloading 10.01 MB separate debug info for /lib/libxyz.so...
|
|
Fix some whitespace issues introduced with the frame_info_ptr patch.
Change-Id: I158d30d8108c97564276c647fc98283ff7b12163
|
|
Currently, every internal_error call must be passed __FILE__/__LINE__
explicitly, like:
internal_error (__FILE__, __LINE__, "foo %d", var);
The need to pass in explicit __FILE__/__LINE__ is there probably
because the function predates widespread and portable variadic macros
availability. We can use variadic macros nowadays, and in fact, we
already use them in several places, including the related
gdb_assert_not_reached.
So this patch renames the internal_error function to something else,
and then reimplements internal_error as a variadic macro that expands
__FILE__/__LINE__ itself.
The result is that we now should call internal_error like so:
internal_error ("foo %d", var);
Likewise for internal_warning.
The patch adjusts all calls sites. 99% of the adjustments were done
with a perl/sed script.
The non-mechanical changes are in gdbsupport/errors.h,
gdbsupport/gdb_assert.h, and gdb/gdbarch.py.
Approved-By: Simon Marchi <simon.marchi@efficios.com>
Change-Id: Ia6f372c11550ca876829e8fd85048f4502bdcf06
|
|
Currently, despite having a smart pointer for frame_infos, GDB may
attempt to use an invalidated frame_info_ptr, which would cause internal
errors to happen. One such example has been documented as PR
python/28856, that happened when printing frame arguments calls an
inferior function.
To avoid failures, the smart wrapper was changed to also cache the frame
id, so the pointer can be reinflated later. For this to work, the
frame-id stuff had to be moved to their own .h file, which is included
by frame-info.h.
Frame_id caching is done explicitly using the prepare_reinflate method.
Caching is done manually so that only the pointers that need to be saved
will be, and reinflating has to be done manually using the reinflate
method because the get method and the -> operator must not change
the internals of the class. Finally, attempting to reinflate when the
pointer is being invalidated causes the following assertion errors:
check_ptrace_stopped_lwp_gone: assertion `lp->stopped` failed.
get_frame_pc: Assertion `frame->next != NULL` failed.
As for performance concerns, my personal testing with `time make
chec-perf GDB_PERFTEST_MODE=run` showed an actual reduction of around
10% of time running.
This commit also adds a testcase that exercises the python/28856 bug with
7 different triggers, run, continue, step, backtrace, finish, up and down.
Some of them can seem to be testing the same thing twice, but since this
test relies on stale pointers, there is always a chance that GDB got lucky
when testing, so better to test extra.
Regression tested on x86_64, using both gcc and clang.
Approved-by: Tom Tomey <tom@tromey.com>
|
|
This changes GDB to use frame_info_ptr instead of frame_info *
The substitution was done with multiple sequential `sed` commands:
sed 's/^struct frame_info;/class frame_info_ptr;/'
sed 's/struct frame_info \*/frame_info_ptr /g' - which left some
issues in a few files, that were manually fixed.
sed 's/\<frame_info \*/frame_info_ptr /g'
sed 's/frame_info_ptr $/frame_info_ptr/g' - used to remove whitespace
problems.
The changed files were then manually checked and some 'sed' changes
undone, some constructors and some gets were added, according to what
made sense, and what Tromey originally did
Co-Authored-By: Bruno Larsen <blarsen@redhat.com>
Approved-by: Tom Tomey <tom@tromey.com>
|
