The compunit symtab backlink of a symtab is never null (the constructor
asserts it), so make symtab::compunit return a reference instead of a
pointer, and have the symtab constructor take the compunit_symtab as a
reference too. Update all callers accordingly.
This came up earlier in review, where a caller would check the result of
`symtab->compunit ()` for nullptr, and I pointed out that it was
unnecessary. Returning a reference makes this clear.
Change-Id: Idf3a6b5fb07a10cd161826ae8a7b826d95bd96c6
Reviewed-By: Tom de Vries <tdevries@suse.de>
After commit 1eed06ae51 ("[gdbsupport] Use using instead of typedef in
next_iterator") I wondered if I could do something similar using sed.
Result of:
...
$ find gdb* -type f -name "*.[ch]" -o -name "*.cc" \
| egrep -v /testsuite/ \
| xargs sed -i \
's/^\([ \t]*\)typedef \([a-zA-Z_0-9:<>,.() ]*\) \([\*&][\*&]*\)\([a-zA-Z_0-9]*\);/\1using \4 = \2 \3;/'
$ find gdb* -type f -name "*.[ch]" -o -name "*.cc" \
| egrep -v /testsuite/ \
| xargs sed -i \
's/^\([ \t]*\)typedef \([a-zA-Z_0-9:<>,.() ]*\) \([a-zA-Z_0-9]*\);/\1using \3 = \2;/'
...
Tested on x86_64-linux.
gdbpy_print_stack() already clears the error indicator.
Thus remove the following call to PyErr_Clear().
Approved-By: Andrew Burgess <aburgess@redhat.com>
I build gdb with clang and ran into a build breaker:
...
gdb/python/py-breakpoint.c:1074:52: error: cannot pass object of non-trivial \
type 'const std::string' (aka 'const basic_string<char>') through variadic \
function; call will abort at runtime [-Wnon-pod-varargs]
1074 | return PyUnicode_FromFormat ("<%s (invalid)>", tp_name);
| ^
...
This looks like fallout from commit dafd73bcda ("gdb/python: fix memory leak
in gdb_py_tp_name"), which changed the return type of gdbpy_py_obj_tp_name
from const char * to std::string.
We could fix this using tp_name.c_str (), but instead fix this by simplifying
the code using gdb_py_invalid_object_repr.
Tested on x86_64-linux.
Approved-By: Tom Tromey <tom@tromey.com>
The current implementation of gdb_py_tp_name() leaks a reference to the
object returned by PyType_GetFullyQualifiedName() for Python >= 3.13, and
PyType_GetQualName() for Python >= 3.11.
Managing the strong reference on the returned PyObject* with a gdbpy_ref<>
fixes the reference count, but also causes the temporary object to be
deallocated on function exit. As a consequence, the 'const char *' returned
by PyUnicode_AsUTF8AndSize() becomes dangling and can no longer safely be
returned.
The proposed approach consists in changing gdb_py_tp_name() to return a
std::string, and forcing a copy of the 'const char *' value, statically
or dynamically allocated, stored in a temporary or non-temporary PyObject,
depending on the version of Python it was compiled with.
An unfortunate side effect of this fix is that every call sites where the
tp_name is printed, must now use `.c_str()' because PyErr_Format()
and its siblings cannot handle std::string.
Approved-By: Andrew Burgess <aburgess@redhat.com>
I noticed that while gdb_python_initialized is an int, there's also an
assignment using false:
...
gdb_python_initialized = false;
...
Fix this by converting gdb_python_initialized to bool type.
Approved-by: Kevin Buettner <kevinb@redhat.com>
The previous commit added a type trait which identifies types that
should not be used within Python objects, that is, types that are not
trivially default constructible. As a result of this, it was
discovered that pending_frame_object includes a frame_info_ptr field.
The problem with frame_info_ptr is that its constructor registers the
new frame_info_ptr with the global frame_list. It is by this
registration that invalidation of frame_info_ptr objects is performed.
As Python is written in C, C++ constructors are not called, so when a
pending_frame_object is created the constructor for the nested
frame_info_ptr field is never run, and the frame_info_ptr is never
registered with the global frame_list. As a result the frame_info_ptr
will never be invalidated if the frame cache is flushed, this can then
lead to problems where we make use of the 'frame_info *' within the
frame_info_ptr, even though it is no longer valid.
In this commit I change the frame_info_ptr within pending_frame_object
to a 'frame_info_ptr *' and allocate the frame_info_ptr object on the
heap, releasing the object, and resetting the point to NULL, when we
are done with it. As the pending_frame_object only needs to remain
valid for the duration of frame_unwind_python::sniff, the 'new' and
'delete' both performed within the function.
We can now check that a pending_frame_object is valid by checking if
the 'frame_info_ptr *' is NULL or not. As the frame_info_ptr is
created in a valid state, and the point is set back to NULL when we
are done with it, we no longer need to compare the frame_info_ptr
object itself against NULL.
The remaining changes in this patch are to dereference the
'frame_info_ptr *' in places where we need the actual object. In some
cases I need to move the dereference later within a function, after a
validity check, in order to avoid dereferencing a NULL pointer.
Finally, I can add the static_assert that guarantees that
pending_frame_object is now safe for allocation by Python.
I discovered this bug while looking at PR gdb/32120. That bug is
about a user's custom frame unwinder that triggers a flush of the
frame cache during the sniffer phase (the
RemoteTargetConnection.send_packet call switches thread, which
triggers the frame cache flush). While looking at that bug I noticed
that the frame_info_ptr within the pending_frame_object wasn't being
reset when the frame cache was flushed. Fixing this does not resolve
the user's issue, but I thought it was still worth tagging this commit
with the bug link.
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=32120
Approved-By: Tom Tromey <tom@tromey.com>
All of our custom Python types are created as structs, like this:
struct some_new_type : public PyObject
{
... various fields ...
};
Then instances of this struct are created by calling PyObject_New,
either directly within GDB's C++ code, or within Python when a user's
Python script creates an instance of that class.
The problem is that Python is written in C, and PyObject_New doesn't
call any constructors for `some_new_type`, nor for any of the fields
within `some_new_type`.
If `some_new_type` is Plain Old Data (POD), then this is fine. Or, to
be more C++ specific, if `some_new_type` is trivially default
constructable, then we're fine.
But if a field within `some_new_type` has a non-trivial constructor,
then we're in trouble as that constructor will never be run.
An example of a problematic field type is frame_info_ptr. The
constructor for this type registers the new object with a central
management object, recording the `this` pointer, using this type within
`some_new_type` will not work as expected; frame invalidation will not
show up within the frame_info_ptr as you might expect.
And so, this type trait exists. Whenever a struct is created to define
a new Python type we should add a line like:
static_assert (gdb::is_python_allocatable_v<some_new_type>);
This will fail if any field of `some_new_type` are unsuitable for this
use.
We don't actually check is_trivially_default_constructible here. Some
types, e.g. ui_file_style::color, have non-trivial (or no default)
constructors, but are still safe to use within `some_new_type` because
their constructors just initialise data fields; there's nothing
"special" that the constructor does that cannot be achieved by
assigning the fields after creation with PyObject_New.
What actually matters is that the type is trivially destructible
(Python won't call C++ destructors, so destructors with side effects,
like deregistering from a list, would be skipped) and trivially
copyable (Python may copy objects with memcpy). Types like
frame_info_ptr, whose constructors and destructors have side effects
such as registering with a central management object, will be caught
because they are neither trivially destructible nor trivially copyable.
Simple POD types like ui_file_style are trivially destructible and
copyable, so pass this trait.
This commit adds the new type trait, and makes use of it in all cases
but one, pending_frame_object in python/py-unwind.c, has a field of
type frame_info_ptr, which is currently broken. This will be fixed,
and the static_assert added, in the next commit.
Approved-By: Tom Tromey <tom@tromey.com>
Now that we've removed all uses of Py_RETURN_{NONE,TRUE,FALSE,NOTIMPLEMENTED},
undefine them to enforce using the refcount-safe wrappers instead.
Approved-By: Tom Tromey <tom@tromey.com>
Result of:
...
$ sed -i 's/Py_RETURN_NONE/return py_none ().release ()/' gdb/python/*.{c,h}
...
with the changes in py_none reverted.
Likewise for Py_RETURN_TRUE,Py_RETURN_FALSE and Py_RETURN_NOTIMPLEMENTED.
Approved-By: Tom Tromey <tom@tromey.com>
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=34145
I came across this code:
...
if (c)
return v;
else
Py_RETURN_NONE;
...
and realized I couldn't easily rewrite it into "return c ? v : ...".
Probably something like this would work:
...
return c ? v : ([] { Py_RETURN_NONE; } ());
...
but it made me wonder if we can get rid of Py_RETURN_NONE.
The only point of it seems to be to increase the reference count on the
Py_None object for older python versions.
Add a refcount-safe wrapper py_none (returning a gdbpy_ref), with the aim of
replacing all uses of Py_RETURN_NONE with "return py_none ().release ()".
Likewise for Py_RETURN_TRUE/Py_RETURN_FALSE/Py_RETURN_NOTIMPLEMENTED.
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=34145
The check-include-guards check fails for
gdb/python/python-limited-api-missing.h:
...
check-include-guards....................................................Failed
- hook id: check-include-guards
- exit code: 1
gdb/python/python-limited-api-missing.h:22: wrong symbol in ifndef
...
Fix this by using GDB_PYTHON_PYTHON_LIMITED_API_MISSING_H instead of
GDB_PYTHON_LIMITED_API_MISSING_H.
Most Python API usages in GDB can be migrated to the limited API, except
the following:
- PEP-741's configuration structures and functions, which use opaque
types. They were originally intended to be part of the Python limited
API, but some Python core maintainers opposed their inclusion at the
time.
- PyOS_ReadlineFunctionPointer, a global variable storing a function
used to override PyOS_StdioReadline(). The signature has remained
unchanged for a long time.
- PyRun_InteractiveLoop, used to read and execute Python statements when
embedding an interactive interpreter. Its signature has also remained
stable for a long time.
Since no limited API alternatives exist for these, and given their long
history of ABI stability, one approach is to expose them in a GDB header
and rely on their continued stability. While this is not without risk,
it seems acceptable given the arguments above. This would remove the
remaining obstacles preventing GDB from being agnostic to the Python
version available at runtime.
That said, issues should be opened on CPython issue tracker to request
that these functions be included in the limited API in future versions.
Last but not least, GDB does not need to officially support the Python
limited API. The '--enable-py-limited-api' option can remain experimental,
with appropriate forewarnings about its limitations and guarantees.
This patch adds a new header, python-limited-api-missing.h, which
exposes symbols not yet part of the Python limited API.
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=23830
Approved-By: Tom Tromey <tom@tromey.com>
GDB currently initializes CPython using the PyConfig C API introduced in
Python 3.8 (PEP 597). From an ABI stability perspective, this API has a
major drawback: it exposes a plain structure whose fields may be added or
removed between Python versions. As a result, it was excluded from the
Python limited API.
Python 3.14 introduced a new configuration API (PEP 741) that avoids
exposing plain structures and instead, operates via opaque pointers.
This design makes it much more suitable for inclusion in the Python
limited API. Indeed, this was the original intent of the PEP-741 author.
However, CPython maintainers ultimately decided otherwise.
Since GDB aims at using the CPython stable API to avoid rebuilding for
each Python version, the absence of a configuration API in the limited
C API constitutes a blocker. Nevertheless, this can be worked around by
using PEP-741 configuration API, whose design is compatible with the
limited C API. It is relatively safe to assume that this API will stay
around for some time.
In this perspective, this patch adds support for using the PEP-741 config
API starting from Python 3.14. When Py_LIMITED_API is defined, the
required functions are exposed as external symbols via a workaround header.
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=23830
Approved-By: Tom Tromey <tom@tromey.com>
This patch adds core file support to GDB's DAP interface.
Core files are supported as a GDB specific argument to 'attach', the
new argument is 'coreFile', the name of the core file to debug.
I think handling core files via attach makes the most sense; attach is
for connecting to existing processes, but these targets are (usually)
stopped as soon as GDB attaches, and that's what a core file looks
like, a target that was running, but is now stopped. It just happens
that core file targets are special in that the target cannot be
resumed again, nor can the user modify the program state (e.g. write
to memory or registers).
Prior to starting this work I took a look at what lldb does. The
documentation is not super clear, but this page seems to indicate that
lldb might also use the 'coreFile' argument to 'attach':
https://lldb.llvm.org/use/lldbdap.html#configuration-settings-reference
Like I said, it's not very clear, but search for "coreFile" and you'll
see it mentioned, just once, under the "attach" header. In order to
be compatible with lldb I used the same argument name with the same
capitalisation.
The new argument is added to the documentation and mentioned in NEWS.
I had to make some changes to testsuite/lib/dap-support.exp to support
this new feature. There's a new dap_corefile proc to handle setting
up the initial connection. This seemed cleaner that overloading
dap_attach, even though under the hood it is still an 'attach' request
that gets sent.
The new test tries to write to memory and registers with the core file
target in place, neither of these requests succeed, which is what we
want, but the exceptions are logged into the dap log file. The
dap_shutdown proc calls dap_check_log_file to check the log for
exceptions, and these two exceptions are spotted and trigger a FAIL.
To avoid this I've added a new "expected_exception_count" argument
for dap_shutdown. Now we check that we see the expected number of
exceptions. We don't check for the specific exception types right
now, but as the test is already checking that the expected requests
fail, I think we're OK.
Approved-By: Tom Tromey <tom@tromey.com>
Add a new Python event registry, events.corefile_changed. This event
is emitted each time the corefile within an inferior changes.
The event object has a single 'inferior' attribute which is the
gdb.Inferior object for which the core file changed. The user can
then inspect Inferior.corefile to see details about the new core file,
or this will be None if the core file was removed from the inferior.
I've updated the existing test to cover this new event.
The new test covers both the corefile_changed event, but also monitors
the exited event. This ties into the work done in the previous
commit where we use whether the inferior has exited or not as a guard
for whether core_target::exit_core_file_inferior should be called.
Unloading a core file should result in a single corefile_changed event
and a single exited event.
Reviewed-By: Eli Zaretskii <eliz@gnu.org>
There are a bunch of iteration functions that take a callback returning
true or false to indicate whether to continue or stop iterating. These
functions then return the same value, indicate whether the iteration was
done until the end of interrupted. I think this is confusing and
error-prone, as I never know which value means what. It is especially
confusing when two opposite conventions collide, such as in
objfile::map_symtabs_matching_filename.
I propose to make that more obvious by introducing a new
iteration_status enum with self-documenting values.
I started to change the callback type
compunit_symtab_iteration_callback, taken by
quick_symbol_functions::search, and then followed that path to update a
bunch of other functions.
I chose the name to be kind of generic, so that it can be used for other
similar iteration patterns. I also put it in gdbsupport, in case we
want to use it in gdbserver too.
Change-Id: I55d84d0c1af8ac0b82cc9f49ccf0d6b60e1769e0
Approved-By: Andrew Burgess <aburgess@redhat.com>
This changes a number of error messages in gdb to use command_style.
In some places I've added double quotes around the command name for
consistency with other messages.
Approved-By: Andrew Burgess <aburgess@redhat.com>
Py_TYPE (self)->tp_name is the traditional idiomatic way to get a Python
type's fully qualified name. However, in the context of the Python
limited API, PyTypeObject is opaque, so the 'tp_name' attribute is no
longer accessible. Additionally, retrieving the type of a Python object
requires Py_TYPE, which is only available as part of the stable API
starting with Python 3.14.
This patch increases minimal Python limited API version from 3.11 to 3.14.
It also introduces two new helpers to retrieve a type's fully qualified
name: gdb_py_tp_name() and gdbpy_py_obj_tp_name(), which extract the fully
qualified name from a PyTypeObject and a PyObject, respectively. Ifdefery
allows these wrappers to select the appropriate API depending on the Python
version and whether the Python limited API is enabled. For any Python
version less than 3.13, gdb_py_tp_name() fallbacks using __qualname__
instead. However, the result may differ slightly in some cases, e.g. the
module name may be missing.
Finally, this patch adapts the existing code to use these wrappers, and
adjusts some test expectations to use the fully qualified name (or
__qualname__ for versions <= 3.13) where it was not previously used.
Note that the corner case where the module name would be missing does not
appear in the testsuite.
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=23830
Approved-By: Tom Tromey <tom@tromey.com>
While working on Windows non-stop support, I ran into a
very-hard-to-track-down bug.
The problem turned out to be that
infrun.c:proceed_resume_thread_checked resumed an already-executing
thread because the thread was marked as "executing=true,
resumed=false", and that function only skips resuming threads that are
marked resumed=true. The consequence was that GDB corrupted the
registers of the Windows DLL loader threads, eventually leading to a
GDB+inferior deadlock.
Originally, the "resumed" flag was only ever set when infrun decided
is was ready to process a thread's pending wait status. infrun has
since evolved to set the resumed flag when we set a thread's executing
flag too. We are not always consistent throughout in guaranteeing
that a thread is marked resumed=true whenever it is marked
executing=true, though. For instance, no target code that supports
non-stop mode (linux-nat, remote, and windows-nat with this series) is
making sure that new threads are marked resumed=true when they are
added to the thread list. They are only marked as {state=running,
executing=true}, the "resumed" flag is not touched.
Making proceed_resume_thread_checked check thr->executing() in
addition to thr->resumed(), feels like papering over a combination of
states that shouldn't happen nowadays.
OTOH, having to have the target backends mark new threads as
resumed=true just feels like too many different states (three) to set:
add_thread (...);
set_running (...);
set_executing (...);
set_resumed (...);
Yuck. I think we can do better.
We really have too many "state tracking" flags in a thread.
Basically:
- whether a thread is "running/stopped/exited" (from the user's
perspective). This is the thread_info::state field.
- whether a thread is "executing" (infrun asked the target to set the
thread executing). This is thread_info::executing().
- whether a thread is "resumed" (infrun wants the thread to be
resumed, but maybe can't yet because the thread has a pending wait
status). This is thread_info::resumed()
"running", "executing", and "resumed" are almost synonyms, so this can
be highly confusing English-wise too.
For "running" vs "executing", in comments, we tipically need to
explain that "running/stopped/exited" is for the user/frontend
perspective, while "executing true/false" is for gdb's internal run
control.
(Also, "executing or not" can also mean something else in GDB's
codebase -- "target has execution" does not mean that threads are
actually running right now -- it's a test for whether we have a live
process vs a core dump!)
One simplification we can do that avoids this running vs executing
ambiguity is to replace the "executing" field with an "internal_state"
field, similar to the thread_info::state field, and make that new
internal_state field reuse the same enum thread_state type that is
used by thread_info::state. Like:
struct thread_info
{
...
/* Frontend/public/external/user view of the thread state. */
enum thread_state m_state = THREAD_STOPPED;
/* The thread's internal state. When the thread is stopped
internally while handling an internal event, like a software
single-step breakpoint, the internal state will be
THREAD_STOPPED, but the external state will still be
THREAD_RUNNING. */
enum thread_state m_internal_state = THREAD_STOPPED;
};
(Assume we'd add state() and internal_state() getters.)
With that, every check for thr->executing() is replaced with a
'thr->internal_state() == THREAD_RUNNING' check, and the code is
clearer by design. There is no confusion between "running" vs
"executing" any more, because they now mean the exact same thing.
Instead, we say e.g., 'thread has (user) state "running", and internal
state "stopped"'. Or simpler, 'thread is running (from the user's
perspective), but internally stopped'. That is after all what we
would way in comments today already.
That still leaves the 'resumed' flag, though. That's the least
obvious one. Turns out we can get rid of it, and make it a new state
tracked by thread_info::internal_state. That is, we make
internal_state have its own enumeration type (decoupled from
thread_info::state's type), and convert the resumed true/false flag to
a new enumerator of this new enumeration. Like so:
enum thread_int_state
{
THREAD_INT_STOPPED,
THREAD_INT_RUNNING,
+ THREAD_INT_RESUMED_PENDING_STATUS,
THREAD_INT_EXITED,
};
That is what this patch does. So in summary, we go from:
thread_info::state {THREAD_STOPPED, THREAD_RUNNING, THREAD_EXITED}
thread_info::executing {false, true}
thread_info::resumed {false, true}
to:
thread_info::state {THREAD_STOPPED, THREAD_RUNNING, THREAD_EXITED}
thread_info::internal_state {THREAD_INT_STOPPED, THREAD_INT_RUNNING,
THREAD_INT_RESUMED_PENDING_STATUS,
THREAD_INT_EXITED}
The patch adds getters/setters for both (user) state and
internal_state, and adds assertions around state transitions, ensuring
that internal_state doesn't get out of sync with
thread::have_pending_wait_status().
The code that adds/removes threads from the proc_target's
resumed_with_pending_wait_status list is all centralized within
thread_info::set_internal_state, when we switch to/from the
resumed-pending-status state. With the assertions in place, it should
be impossible to end up with a THREAD_INT_RUNNING thread with a
pending status.
The thread.c:set_running, thread.c:set_executing, thread.c:set_resumed
global functions are all gone, replaced with new thread.c:set_state
and thread.c:set_internal_state functions.
Tested on x86_64-linux-gnu, native and gdbserver.
Change-Id: I4f5097d68f4694d44e1ae23fea3e9bce45fb078c
commit-id:42ba97d4
Tom de Vries pointed out that a certain Python test left a core file.
Looking into this, I think the problem is that the GIL is not held
when gdbpy_reggroup_object_map is destroyed.
This patch changes the code to explicitly clear the global while
holding the GIL. This fixed the crash for me.
This also adds a comment to gdbpy_initialize_file to explicitly
mention that the GIL is held while the finalizers are run.
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=34013
Reviewed-By: Tom de Vries <tdevries@suse.de>
This commit adds a new property - ranges - to gdb.Block object. It holds
a tuple of ranges for that block. Each range is a tuple of (start, end)
address. For contiguous blocks it contains only one range.
Reviewed-By: Eli Zaretskii <eliz@gnu.org>
Approved-By: Tom Tromey <tom@tromey.com>
Extend the Architecture.disassemble API to allow the user to request
styled disassembler output via a new styling argument. A user can now
write:
insn = arch.disassemble(address, styling = True)
The instruction strings returned within INSN will contain ANSI escape
sequences so long as 'set style enabled on' is in effect. This means
that the user's personal settings (disabling styling) will override a
GDB extension that requests styled disassembler output. I think this
makes sense.
The default for the styling argument is False, this maintains the
current unstyled output as default.
Reviewed-By: Eli Zaretskii <eliz@gnu.org>
Approved-By: Tom Tromey <tom@tromey.com>
I was looking at the new_objfile observer and noticed a comment in
reread_symbols (symfile.c) that seemed out of date, it talked about
calling the new objfile observer with a NULL objfile argument.
A little digging indicates that the comment is indeed out of date, and
that we never call the new_objfile observer with a NULL argument any
more.
So in this commit I have:
1. Updated the out of date comment in reread_symbols.
2. Changed the argument type for the new_objfile observer from
'objfile *' to 'objfile &'. Passing a NULL pointer is no longer
an option.
3. Updated the existing new_objfile observers to take 'objfile &'
and updated their implementations as needed.
There should be no user visible changes after this commit.
Approved-By: Simon Marchi <simon.marchi@efficios.com>
Replace all occurrences of:
strcmp (...) == 0
strcmp (...) != 0
!strcmp (...)
0 == strcmp (...)
strcmp (...) directly used as a boolean predicate
with the equivalent expression using streq.
This is for consistency (we already use streq as some places in the
testsuite) but also for clarity. I think that streq is clearer on the
intent than strcmp. It's also a bit shorter.
Change-Id: Ibbf5261b1872c240bc0c982c147f6a5477275a91
Approved-By: Andrew Burgess <aburgess@redhat.com>
Python extension objects that support __dict__ must inherit from
gdbpy_dict_wrapper, a wrapper class that stores the PyObject
corresponding to the __dict__ attribute. Because this dictionary
is not managed directly by Python, each extension objects must
provide appropriate accessors so that attributes stored in __dict__
are looked up correctly.
Currently, management of this dictionary is not centralized, and
each Python extension object implements its own logic to create,
access, and destroy it.
A previous patch centralized the creation logic; this patch focuses
on centralizing the access to the dictionary. It introduces two new
macros:
- gdbpy_dict_wrapper_cfg_dict_getter, which defines a getter for
__dict__.
- gdbpy_dict_wrapper_getsetattro, which sets tp_getattro and
tp_setattro in PyTypeObject to gdb_py_generic_getattro and
gdb_py_generic_setattro, respectively. These helpers already
centralizes attribute access for Python extension objects having
a __dict__.
Note: this macro will eventually be removed once Python extension
types are migrated to heap-allocated types.
Approved-By: Tom Tromey <tom@tromey.com>
I noticed that many callbacks passed to
iterate_over_objfiles_in_search_order were still returning 0/1 rather
than false/true.
Approved-By: Simon Marchi <simon.marchi@efficios.com>
Python extension objects that support __dict__ must inherit from
gdbpy_dict_wrapper, a wrapper class that stores the PyObject
corresponding to the __dict__ attribute.
Currently, management of this dictionary is not centralized, and
each Python extension object implements its own logic to create,
access, and destroy it.
This patch focuses on the allocation of the dictionary, introduces
a new method, gdbpy_dict_wrapper::allocate_dict(), and
adapts the existing code to use this method.
Approved-By: Tom Tromey <tom@tromey.com>
This patch aims at systematically using gdbpy_ref<> at all call sites
of PyObject_New(). This prepares for future patches that expect
gdbby_ref<> parameters and affect return handling.
As part of this change, flattening the affected functions so that the
return logic becomes clearer and more flexible to adjust.
Approved-By: Tom Tromey <tom@tromey.com>
When ref_ptr<T,Policy>::new_reference() is specialized for 'PyObject'
(i.e. gdbpy_ref<>), it currently requires the argument type to be exactly
'PyObject *'. As a result, pointers to subclasses of 'PyObject' must be
explicitly cast before being passed, making call sites unnecessarily
verbose.
This patch makes ref_ptr<T,Policy>::new_reference() a template method
that accepts both T and subclasses of T, performing the cast to 'T *'
internally when needed. This removes redundant casts at call sites
without changing behavior.
Approved-By: Tom Tromey <tom@tromey.com>
This patch replaces the raw uint8[3] buffer used to represent RGB values
with a more convenient wrapper, rgb_color, around std::array<uint8_t, 3>.
It also changes the return type of ui_file_style::color::get_rgb to
rgb_color instead of filling a caller-provided buffer, and updates all
callers accordingly.
This expected benefit of this change consists in:
- removing the manual size handling.
- proving accessors without using hard-coded indexes.
- making the API safer.
- simplifying call sites.
This refactoring does not introduce any functional change.
Approved-By: Tom Tromey <tom@tromey.com>
Passing 'gdbpy_ref<> &' instead of raw 'PyObject *' to init helpers
makes ownership of PyObject clearer at call sites, and removes
unnecessary '.get()' calls.
Changing the return type from 'int' to 'bool' improves readability
and better expresses the success/failure semantics.
Approved-By: Tom Tromey <tom@tromey.com>
I noticed that gdb.FinishBreakpoint doesn't work if the parent
function is a tail call function. In bpfinishpy_init we use
get_frame_pc to find the address at which the finish breakpoint should
be placed within the previous frame.
However, if the previous frame is a tail call frame, then get_frame_pc
will return an address outside of the tail call function, an address
which will not be reached on the return path.
Unlike other recent tail call fixes I've made, we cannot switch to
using something like get_frame_address_in_block here as in the tail
call case this will return an address within the function, but not an
address that will be executed when we return.
What we need to do in the tail call case is create the finish
breakpoint in the frame that called the tail call function. Or if
that frame is itself a tail call, then we should walk back up the call
stack until we find a non-tail call function.
This can be achieved by adding a call to skip_tailcall_frames into
bpfinishpy_init after our existing call to get_prev_frame.
I've extended the existing test case to cover this additional
situation.
Approved-By: Tom Tromey <tom@tromey.com>
Creating a Python gdb.FinishBreakpoint for an inline frame doesn't
work.
If we look at the 'finish' command, in the finish_command
function (infcmd.c) then we see that GDB handles inline frames very
different to non-inline frames.
For non-inline frames GDB creates a temporary breakpoint and then
resumes the inferior until the breakpoint is hit.
But for inline frames, GDB steps forward until we have left the inline
frame.
When it comes to gdb.FinishBreakpoint we only have the "create a
temporary breakpoint" mechanism; that is, after all, what the
FinishBreakpoint is, it's a temporary breakpoint placed at the
return address in the caller.
Currently, when a FinishBreakpoint is created within an inline frame,
GDB ends up creating the breakpoint at the current $pc. As a result
the breakpoint will not be hit before the current function
exits (unless there's a loop going on, but that's not the point).
We could imagine what a solution to this problem would look like, GDB
would need to figure out the set of addresses for all possible exit
points from the inline function, and place a breakpoint at each of
these locations. I don't propose doing that in this commit.
Instead, I plan to update the docs to note that creating a
FinishBreakpoint within an inline frame is not allowed, and I will
catch this case within bpfinishpy_init (python/py-finishbreakpoint.c)
and throw an error.
Though the error is new, all I'm doing is raising an error for a case
that never worked.
There's a new test to cover this case.
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=18339
Reviewed-By: Eli Zaretskii <eliz@gnu.org>
Approved-By: Tom Tromey <tom@tromey.com>
The FinishBreakpoint.return_value attribute will not be populated
correctly for tail call functions.
In bpfinishpy_init (python/py-finishbreakpoint.c) we use the function
get_frame_pc_if_available to return an address, and then use this
address to lookup a function symbol.
The problem is that, for tail call functions, the address returned by
get_frame_pc_if_available can be outside the bounds of the function,
as a result GDB might find no function symbol at all, or might find
the wrong function symbol, if the tail call function is immediately
adjacent to the next function.
Fix this by using get_frame_address_in_block_if_available instead.
For tail call functions this will return an address within the bounds
of the function, which means that GDB should find the correct function
symbol, and from this the correct return type.
I've extended the existing FinishBreakpoint with tail call test case
to include printing the return value, this test fails without this
patch, but now works.
Approved-By: Tom Tromey <tom@tromey.com>
This commit adds a new method gdb.Symtab.source_lines. This method
can be used to read the lines from a symtab's source file. This is
similar to GDB's internal source_cache::get_source_lines function.
Currently using the Python API, if a user wants to display source
lines then they need to use Symtab.fullname() to get the source file
name, then open this file and parse out the lines themselves.
This isn't too much effort, but the problem is that these lines will
not be styled. The user could style the source content themselves,
but will this be styled exactly as GDB would style it?
The new Symtab.source_lines() method returns source lines with styling
included (as ANSI terminal escape sequences), assuming of course, that
styling is currently enabled.
Of course, in some cases, a user of the Python API might want source
code without styling. That's supported too, the new method has an
'unstyled' argument. If this is True then the output is forced to be
unstyled. The argument is named 'unstyled' rather than 'styled'
because the API call cannot force styling on. If 'set style enabled
off' is in effect then making the API call will never return styled
source lines.
The new API call allows for a range of lines to be requested if
desired.
As part of this commit I've updated the host_string_to_python_string
utility function to take a std::string_view.
Reviewed-By: Eli Zaretskii <eliz@gnu.org>
Approved-By: Tom Tromey <tom@tromey.com>
This commit introduces a new Python event, selected_context. This
event is attached to the user_selected_context_changed observer, which
triggers when the user changes the currently selected inferior,
thread, or frame.
Adding this event allows a Python extension to update in response to
user driven changes without having to poll the state from a
before_prompt hook, which is what I currently do to achieve the same
results.
I did consider splitting the user_selected_context_changed observer
into 3 separate Python events, inferior_changed, thread_changed, and
frame_changed, but I couldn't see any significant advantage to doing
this, so in the end I went with just a single event, and the event
object contains the inferior, thread, and frame.
Additionally, the user isn't informed about which aspect of the
context changed. That is, every event carries the inferior, thread,
and frame, so an event triggered when switching frames will looks
identical to an event triggered when switching inferiors. If the user
wants to know what changed then they will have to track the current
state themselves, and then compare the event state to the stored
current state. In many cases though I suspect that just being told
something changed, and then updating everything will be sufficient,
which is why I've not bothered trying to inform the user what changed.
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=24482
Reviewed-By: Eli Zaretskii <eliz@gnu.org>
Approved-By: Tom Tromey <tom@tromey.com>
In some spots the Python code will do an explicit upcast of gdbpy_ref,
like:
return gdbpy_ref<> ((PyObject *) color_obj.release ());
Now that ref_ptr allows upcasts, and because gdb's Python objects
derive from PyObject, these reshufflings can be expressed more simply.
Approved-By: Andrew Burgess <aburgess@redhat.com>
Now that gdb's Python types derive from PyObject, there's no need to
use a template for gdbpy_ref_policy. This simplifies the code a tiny
bit.
Approved-By: Andrew Burgess <aburgess@redhat.com>
* PyTuple_GET_ITEM -> PyTuple_GetItem
* PyTuple_SET_ITEM -> PyTuple_SetItem
* PyTuple_GET_SIZE -> PyTuple_Size
Unlike PyTuple_SET_ITEM(), PyTuple_SetItem() returns an integer: 0 on
success and -1 on error (e.g. IndexError). The existing code must therefore
be updated to handle this new behaviour.
Since processing now stops when PyTuple_SetItem() returns an error, some
resources (such as newly allocated tuples) must be properly deallocated in
error paths. To address this, this patch replaces the use of raw 'PyObject *'
pointers with gdbpy_ref<>, a reference-counted wrapper around 'PyObject *',
which automatically decrements the reference count on early exit.
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=23830
Approved-By: Tom Tromey <tom@tromey.com>
I noticed a few spots in the Python code that were decoding method
arguments and then immediately type-checking an argument. This can be
done more concisely using the "O!" format.
Reviewed-By: Tom de Vries <tdevries@suse.de>
They are unreachable because there is a return statement in all possible code
paths before.
Change-Id: I9db2f41f8017380c0c79f97af9bbf8734038ba9a
Approved-By: Tom Tromey <tom@tromey.com>
This changes gdbpy_registry::lookup to return a gdbpy_ref<>, using the
type system to convey that a new reference is always returned.
Approved-By: Simon Marchi <simon.marchi@efficios.com>
This changes gdbarch_to_arch_object to return a gdbpy_ref<>,
using the type system to convey that a new reference is always
returned.
Approved-By: Simon Marchi <simon.marchi@efficios.com>
This changes symtab_to_linetable_object to return a gdbpy_ref<>,
using the type system to convey that a new reference is always
returned.
Approved-By: Simon Marchi <simon.marchi@efficios.com>
