gdb/python: Allow SIGTRAMP_FRAME python unwinders to be created.
Checks
| Context |
Check |
Description |
| linaro-tcwg-bot/tcwg_gdb_build--master-aarch64 |
success
|
Testing passed
|
| linaro-tcwg-bot/tcwg_gdb_build--master-arm |
success
|
Testing passed
|
| linaro-tcwg-bot/tcwg_gdb_check--master-aarch64 |
success
|
Testing passed
|
| linaro-tcwg-bot/tcwg_gdb_check--master-arm |
success
|
Testing passed
|
Commit Message
Wine now executes Win32 code on a separate stack for its Unix code. It
switches from one stack to another on through specific functions, and
without any custom unwinders, debugging Wine in Gdb will only let you
see the frames of either the Win32 side, or the Unix side.
The Win32 and Unix call stacks are actually interleaved, with Unix code
sometimes calling back into Win32. Using a custom Python frame unwinder
we can provide Gdb with the information it needs to join both toghether
and show a complete interleaved call stack. However, Gdb will often stop
unwinding as it will see the frames from one stack as inner the frames
from the other stack.
This allows to write custom unwinders to produce SIGTRAMP_FRAME typed
frames, which bypasses this restriction and will show the Win32 / Unix
gate as a signal frame.
---
gdb/python/lib/gdb/__init__.py | 8 ++++----
gdb/python/lib/gdb/unwinder.py | 7 ++++++-
gdb/python/py-unwind.c | 16 +++++++++++++++-
3 files changed, 25 insertions(+), 6 deletions(-)
Comments
Ping,
Thanks.
On 2/14/24 22:58, Rémi Bernon wrote:
> Wine now executes Win32 code on a separate stack for its Unix code. It
> switches from one stack to another on through specific functions, and
> without any custom unwinders, debugging Wine in Gdb will only let you
> see the frames of either the Win32 side, or the Unix side.
>
> The Win32 and Unix call stacks are actually interleaved, with Unix code
> sometimes calling back into Win32. Using a custom Python frame unwinder
> we can provide Gdb with the information it needs to join both toghether
> and show a complete interleaved call stack. However, Gdb will often stop
> unwinding as it will see the frames from one stack as inner the frames
> from the other stack.
>
> This allows to write custom unwinders to produce SIGTRAMP_FRAME typed
> frames, which bypasses this restriction and will show the Win32 / Unix
> gate as a signal frame.
> ---
> gdb/python/lib/gdb/__init__.py | 8 ++++----
> gdb/python/lib/gdb/unwinder.py | 7 ++++++-
> gdb/python/py-unwind.c | 16 +++++++++++++++-
> 3 files changed, 25 insertions(+), 6 deletions(-)
>
> diff --git a/gdb/python/lib/gdb/__init__.py b/gdb/python/lib/gdb/__init__.py
> index b3124369fe8..d9226440cbc 100644
> --- a/gdb/python/lib/gdb/__init__.py
> +++ b/gdb/python/lib/gdb/__init__.py
> @@ -86,7 +86,7 @@ frame_filters = {}
> frame_unwinders = []
>
>
> -def _execute_unwinders(pending_frame):
> +def _execute_unwinders(pending_frame, frame_type):
> """Internal function called from GDB to execute all unwinders.
>
> Runs each currently enabled unwinder until it finds the one that
> @@ -105,19 +105,19 @@ def _execute_unwinders(pending_frame):
> """
> for objfile in objfiles():
> for unwinder in objfile.frame_unwinders:
> - if unwinder.enabled:
> + if unwinder.enabled and unwinder.frame_type == frame_type:
> unwind_info = unwinder(pending_frame)
> if unwind_info is not None:
> return (unwind_info, unwinder.name)
>
> for unwinder in current_progspace().frame_unwinders:
> - if unwinder.enabled:
> + if unwinder.enabled and unwinder.frame_type == frame_type:
> unwind_info = unwinder(pending_frame)
> if unwind_info is not None:
> return (unwind_info, unwinder.name)
>
> for unwinder in frame_unwinders:
> - if unwinder.enabled:
> + if unwinder.enabled and unwinder.frame_type == frame_type:
> unwind_info = unwinder(pending_frame)
> if unwind_info is not None:
> return (unwind_info, unwinder.name)
> diff --git a/gdb/python/lib/gdb/unwinder.py b/gdb/python/lib/gdb/unwinder.py
> index 140b84d3374..7e23a662a32 100644
> --- a/gdb/python/lib/gdb/unwinder.py
> +++ b/gdb/python/lib/gdb/unwinder.py
> @@ -29,7 +29,7 @@ class Unwinder(object):
> enabled: A boolean indicating whether the unwinder is enabled.
> """
>
> - def __init__(self, name):
> + def __init__(self, name, frame_type=gdb.NORMAL_FRAME):
> """Constructor.
>
> Args:
> @@ -39,9 +39,14 @@ class Unwinder(object):
> if not isinstance(name, str):
> raise TypeError("incorrect type for name: %s" % type(name))
>
> + self._frame_type = frame_type
> self._name = name
> self._enabled = True
>
> + @property
> + def frame_type(self):
> + return self._frame_type
> +
> @property
> def name(self):
> return self._name
> diff --git a/gdb/python/py-unwind.c b/gdb/python/py-unwind.c
> index 1856e41e2a1..2d36c43d342 100644
> --- a/gdb/python/py-unwind.c
> +++ b/gdb/python/py-unwind.c
> @@ -844,7 +844,8 @@ pyuw_sniffer (const struct frame_unwind *self, frame_info_ptr this_frame,
> /* A (gdb.UnwindInfo, str) tuple, or None. */
> gdbpy_ref<> pyo_execute_ret
> (PyObject_CallFunctionObjArgs (pyo_execute.get (),
> - pyo_pending_frame.get (), NULL));
> + pyo_pending_frame.get (),
> + PyLong_FromLong(self->type), NULL));
> if (pyo_execute_ret == nullptr)
> {
> /* If the unwinder is cancelled due to a Ctrl-C, then propagate
> @@ -965,6 +966,19 @@ pyuw_on_new_gdbarch (struct gdbarch *newarch)
> unwinder->sniffer = pyuw_sniffer;
> unwinder->dealloc_cache = pyuw_dealloc_cache;
> frame_unwind_prepend_unwinder (newarch, unwinder);
> +
> + struct frame_unwind *unwinder_signals
> + = GDBARCH_OBSTACK_ZALLOC (newarch, struct frame_unwind);
> +
> + unwinder_signals->name = "python-sigtramp";
> + unwinder_signals->type = SIGTRAMP_FRAME;
> + unwinder_signals->stop_reason = default_frame_unwind_stop_reason;
> + unwinder_signals->this_id = pyuw_this_id;
> + unwinder_signals->prev_register = pyuw_prev_register;
> + unwinder_signals->unwind_data = (const struct frame_data *) newarch;
> + unwinder_signals->sniffer = pyuw_sniffer;
> + unwinder_signals->dealloc_cache = pyuw_dealloc_cache;
> + frame_unwind_prepend_unwinder (newarch, unwinder_signals);
> data->unwinder_registered = 1;
> }
> }
Ping,
Thanks,
On 2/14/24 22:58, Rémi Bernon wrote:
> Wine now executes Win32 code on a separate stack for its Unix code. It
> switches from one stack to another on through specific functions, and
> without any custom unwinders, debugging Wine in Gdb will only let you
> see the frames of either the Win32 side, or the Unix side.
>
> The Win32 and Unix call stacks are actually interleaved, with Unix code
> sometimes calling back into Win32. Using a custom Python frame unwinder
> we can provide Gdb with the information it needs to join both toghether
> and show a complete interleaved call stack. However, Gdb will often stop
> unwinding as it will see the frames from one stack as inner the frames
> from the other stack.
>
> This allows to write custom unwinders to produce SIGTRAMP_FRAME typed
> frames, which bypasses this restriction and will show the Win32 / Unix
> gate as a signal frame.
> ---
> gdb/python/lib/gdb/__init__.py | 8 ++++----
> gdb/python/lib/gdb/unwinder.py | 7 ++++++-
> gdb/python/py-unwind.c | 16 +++++++++++++++-
> 3 files changed, 25 insertions(+), 6 deletions(-)
>
> diff --git a/gdb/python/lib/gdb/__init__.py b/gdb/python/lib/gdb/__init__.py
> index b3124369fe8..d9226440cbc 100644
> --- a/gdb/python/lib/gdb/__init__.py
> +++ b/gdb/python/lib/gdb/__init__.py
> @@ -86,7 +86,7 @@ frame_filters = {}
> frame_unwinders = []
>
>
> -def _execute_unwinders(pending_frame):
> +def _execute_unwinders(pending_frame, frame_type):
> """Internal function called from GDB to execute all unwinders.
>
> Runs each currently enabled unwinder until it finds the one that
> @@ -105,19 +105,19 @@ def _execute_unwinders(pending_frame):
> """
> for objfile in objfiles():
> for unwinder in objfile.frame_unwinders:
> - if unwinder.enabled:
> + if unwinder.enabled and unwinder.frame_type == frame_type:
> unwind_info = unwinder(pending_frame)
> if unwind_info is not None:
> return (unwind_info, unwinder.name)
>
> for unwinder in current_progspace().frame_unwinders:
> - if unwinder.enabled:
> + if unwinder.enabled and unwinder.frame_type == frame_type:
> unwind_info = unwinder(pending_frame)
> if unwind_info is not None:
> return (unwind_info, unwinder.name)
>
> for unwinder in frame_unwinders:
> - if unwinder.enabled:
> + if unwinder.enabled and unwinder.frame_type == frame_type:
> unwind_info = unwinder(pending_frame)
> if unwind_info is not None:
> return (unwind_info, unwinder.name)
> diff --git a/gdb/python/lib/gdb/unwinder.py b/gdb/python/lib/gdb/unwinder.py
> index 140b84d3374..7e23a662a32 100644
> --- a/gdb/python/lib/gdb/unwinder.py
> +++ b/gdb/python/lib/gdb/unwinder.py
> @@ -29,7 +29,7 @@ class Unwinder(object):
> enabled: A boolean indicating whether the unwinder is enabled.
> """
>
> - def __init__(self, name):
> + def __init__(self, name, frame_type=gdb.NORMAL_FRAME):
> """Constructor.
>
> Args:
> @@ -39,9 +39,14 @@ class Unwinder(object):
> if not isinstance(name, str):
> raise TypeError("incorrect type for name: %s" % type(name))
>
> + self._frame_type = frame_type
> self._name = name
> self._enabled = True
>
> + @property
> + def frame_type(self):
> + return self._frame_type
> +
> @property
> def name(self):
> return self._name
> diff --git a/gdb/python/py-unwind.c b/gdb/python/py-unwind.c
> index 1856e41e2a1..2d36c43d342 100644
> --- a/gdb/python/py-unwind.c
> +++ b/gdb/python/py-unwind.c
> @@ -844,7 +844,8 @@ pyuw_sniffer (const struct frame_unwind *self, frame_info_ptr this_frame,
> /* A (gdb.UnwindInfo, str) tuple, or None. */
> gdbpy_ref<> pyo_execute_ret
> (PyObject_CallFunctionObjArgs (pyo_execute.get (),
> - pyo_pending_frame.get (), NULL));
> + pyo_pending_frame.get (),
> + PyLong_FromLong(self->type), NULL));
> if (pyo_execute_ret == nullptr)
> {
> /* If the unwinder is cancelled due to a Ctrl-C, then propagate
> @@ -965,6 +966,19 @@ pyuw_on_new_gdbarch (struct gdbarch *newarch)
> unwinder->sniffer = pyuw_sniffer;
> unwinder->dealloc_cache = pyuw_dealloc_cache;
> frame_unwind_prepend_unwinder (newarch, unwinder);
> +
> + struct frame_unwind *unwinder_signals
> + = GDBARCH_OBSTACK_ZALLOC (newarch, struct frame_unwind);
> +
> + unwinder_signals->name = "python-sigtramp";
> + unwinder_signals->type = SIGTRAMP_FRAME;
> + unwinder_signals->stop_reason = default_frame_unwind_stop_reason;
> + unwinder_signals->this_id = pyuw_this_id;
> + unwinder_signals->prev_register = pyuw_prev_register;
> + unwinder_signals->unwind_data = (const struct frame_data *) newarch;
> + unwinder_signals->sniffer = pyuw_sniffer;
> + unwinder_signals->dealloc_cache = pyuw_dealloc_cache;
> + frame_unwind_prepend_unwinder (newarch, unwinder_signals);
> data->unwinder_registered = 1;
> }
> }
On 14/02/2024 22:58, Rémi Bernon wrote:
> Wine now executes Win32 code on a separate stack for its Unix code. It
> switches from one stack to another on through specific functions, and
> without any custom unwinders, debugging Wine in Gdb will only let you
> see the frames of either the Win32 side, or the Unix side.
>
> The Win32 and Unix call stacks are actually interleaved, with Unix code
> sometimes calling back into Win32. Using a custom Python frame unwinder
> we can provide Gdb with the information it needs to join both toghether
> and show a complete interleaved call stack. However, Gdb will often stop
> unwinding as it will see the frames from one stack as inner the frames
> from the other stack.
>
> This allows to write custom unwinders to produce SIGTRAMP_FRAME typed
> frames, which bypasses this restriction and will show the Win32 / Unix
> gate as a signal frame.
Hi!
Sorry for taking a long time for to review.
This looks like a good improvement. It would be nice if you could
provide a test, but I'm not sure if you are able to manufacture a
trampoline frame for a test. If it isn't possible to make a test patch
LGTM: Reviewed-By: Guinevere Larsen <blarsen@redhat.com>
I hope a responsible maintainer is able to approve this soon (or explain
how to make a faux trampoline frame to test).
On 3/14/24 14:00, Guinevere Larsen wrote:
> On 14/02/2024 22:58, Rémi Bernon wrote:
>> Wine now executes Win32 code on a separate stack for its Unix code. It
>> switches from one stack to another on through specific functions, and
>> without any custom unwinders, debugging Wine in Gdb will only let you
>> see the frames of either the Win32 side, or the Unix side.
>>
>> The Win32 and Unix call stacks are actually interleaved, with Unix code
>> sometimes calling back into Win32. Using a custom Python frame unwinder
>> we can provide Gdb with the information it needs to join both toghether
>> and show a complete interleaved call stack. However, Gdb will often stop
>> unwinding as it will see the frames from one stack as inner the frames
>> from the other stack.
>>
>> This allows to write custom unwinders to produce SIGTRAMP_FRAME typed
>> frames, which bypasses this restriction and will show the Win32 / Unix
>> gate as a signal frame.
>
> Hi!
>
> Sorry for taking a long time for to review.
>
> This looks like a good improvement. It would be nice if you could
> provide a test, but I'm not sure if you are able to manufacture a
> trampoline frame for a test. If it isn't possible to make a test patch
> LGTM: Reviewed-By: Guinevere Larsen <blarsen@redhat.com>
>
> I hope a responsible maintainer is able to approve this soon (or explain
> how to make a faux trampoline frame to test).
>
Thanks for the feedback! I have no idea how to add a test, but I'll try
to figure something up.
Rémi Bernon <rbernon@codeweavers.com> writes:
> Wine now executes Win32 code on a separate stack for its Unix code. It
> switches from one stack to another on through specific functions, and
> without any custom unwinders, debugging Wine in Gdb will only let you
> see the frames of either the Win32 side, or the Unix side.
>
> The Win32 and Unix call stacks are actually interleaved, with Unix code
> sometimes calling back into Win32. Using a custom Python frame unwinder
> we can provide Gdb with the information it needs to join both toghether
> and show a complete interleaved call stack. However, Gdb will often stop
> unwinding as it will see the frames from one stack as inner the frames
> from the other stack.
>
> This allows to write custom unwinders to produce SIGTRAMP_FRAME typed
> frames, which bypasses this restriction and will show the Win32 / Unix
> gate as a signal frame.
> ---
> gdb/python/lib/gdb/__init__.py | 8 ++++----
> gdb/python/lib/gdb/unwinder.py | 7 ++++++-
> gdb/python/py-unwind.c | 16 +++++++++++++++-
As Guinevere pointed out, this will really need a test case. But also
you'll need to update the documentation.
> 3 files changed, 25 insertions(+), 6 deletions(-)
>
> diff --git a/gdb/python/lib/gdb/__init__.py b/gdb/python/lib/gdb/__init__.py
> index b3124369fe8..d9226440cbc 100644
> --- a/gdb/python/lib/gdb/__init__.py
> +++ b/gdb/python/lib/gdb/__init__.py
> @@ -86,7 +86,7 @@ frame_filters = {}
> frame_unwinders = []
>
>
> -def _execute_unwinders(pending_frame):
> +def _execute_unwinders(pending_frame, frame_type):
There are lots of different frame types, but in the C++ code we only
invoke for NORMAL_FRAME and SIGTRAMP_FRAME, yet a user could register an
unwinder for e.g. INLINE_FRAME, and that'll never be used. So at a
minimum I think we need additional error checking when unwinders are
registered.
Also, there's the 'info unwinder' command which would need updating to
display the unwinder type I think.
I wonder if instead of giving frames a type, if we could have GDB ask
the unwinder if it should perform a more-inner check, and the Python
unwinders could just say no.
Guinevere has some patches that C++-ify the unwinders, so this might
make it easier to add new methods, which might make doing something like
this easier.
I think there's only two places where the more-inner check is performed,
so I'd be interested to see what drawbacks there might be to this
approach.
> """Internal function called from GDB to execute all unwinders.
>
> Runs each currently enabled unwinder until it finds the one that
> @@ -105,19 +105,19 @@ def _execute_unwinders(pending_frame):
> """
> for objfile in objfiles():
> for unwinder in objfile.frame_unwinders:
> - if unwinder.enabled:
> + if unwinder.enabled and unwinder.frame_type == frame_type:
> unwind_info = unwinder(pending_frame)
> if unwind_info is not None:
> return (unwind_info, unwinder.name)
>
> for unwinder in current_progspace().frame_unwinders:
> - if unwinder.enabled:
> + if unwinder.enabled and unwinder.frame_type == frame_type:
> unwind_info = unwinder(pending_frame)
> if unwind_info is not None:
> return (unwind_info, unwinder.name)
>
> for unwinder in frame_unwinders:
> - if unwinder.enabled:
> + if unwinder.enabled and unwinder.frame_type == frame_type:
> unwind_info = unwinder(pending_frame)
> if unwind_info is not None:
> return (unwind_info, unwinder.name)
> diff --git a/gdb/python/lib/gdb/unwinder.py b/gdb/python/lib/gdb/unwinder.py
> index 140b84d3374..7e23a662a32 100644
> --- a/gdb/python/lib/gdb/unwinder.py
> +++ b/gdb/python/lib/gdb/unwinder.py
> @@ -29,7 +29,7 @@ class Unwinder(object):
> enabled: A boolean indicating whether the unwinder is enabled.
> """
>
> - def __init__(self, name):
> + def __init__(self, name, frame_type=gdb.NORMAL_FRAME):
> """Constructor.
>
> Args:
> @@ -39,9 +39,14 @@ class Unwinder(object):
> if not isinstance(name, str):
> raise TypeError("incorrect type for name: %s" % type(name))
>
> + self._frame_type = frame_type
> self._name = name
> self._enabled = True
>
> + @property
> + def frame_type(self):
> + return self._frame_type
> +
> @property
> def name(self):
> return self._name
> diff --git a/gdb/python/py-unwind.c b/gdb/python/py-unwind.c
> index 1856e41e2a1..2d36c43d342 100644
> --- a/gdb/python/py-unwind.c
> +++ b/gdb/python/py-unwind.c
> @@ -844,7 +844,8 @@ pyuw_sniffer (const struct frame_unwind *self, frame_info_ptr this_frame,
> /* A (gdb.UnwindInfo, str) tuple, or None. */
> gdbpy_ref<> pyo_execute_ret
> (PyObject_CallFunctionObjArgs (pyo_execute.get (),
> - pyo_pending_frame.get (), NULL));
> + pyo_pending_frame.get (),
> + PyLong_FromLong(self->type), NULL));
Nit: missing a space after PyLong_FromLong before the '('.
Thanks,
Andrew
> if (pyo_execute_ret == nullptr)
> {
> /* If the unwinder is cancelled due to a Ctrl-C, then propagate
> @@ -965,6 +966,19 @@ pyuw_on_new_gdbarch (struct gdbarch *newarch)
> unwinder->sniffer = pyuw_sniffer;
> unwinder->dealloc_cache = pyuw_dealloc_cache;
> frame_unwind_prepend_unwinder (newarch, unwinder);
> +
> + struct frame_unwind *unwinder_signals
> + = GDBARCH_OBSTACK_ZALLOC (newarch, struct frame_unwind);
> +
> + unwinder_signals->name = "python-sigtramp";
> + unwinder_signals->type = SIGTRAMP_FRAME;
> + unwinder_signals->stop_reason = default_frame_unwind_stop_reason;
> + unwinder_signals->this_id = pyuw_this_id;
> + unwinder_signals->prev_register = pyuw_prev_register;
> + unwinder_signals->unwind_data = (const struct frame_data *) newarch;
> + unwinder_signals->sniffer = pyuw_sniffer;
> + unwinder_signals->dealloc_cache = pyuw_dealloc_cache;
> + frame_unwind_prepend_unwinder (newarch, unwinder_signals);
> data->unwinder_registered = 1;
> }
> }
> --
> 2.43.0
@@ -86,7 +86,7 @@ frame_filters = {}
frame_unwinders = []
-def _execute_unwinders(pending_frame):
+def _execute_unwinders(pending_frame, frame_type):
"""Internal function called from GDB to execute all unwinders.
Runs each currently enabled unwinder until it finds the one that
@@ -105,19 +105,19 @@ def _execute_unwinders(pending_frame):
"""
for objfile in objfiles():
for unwinder in objfile.frame_unwinders:
- if unwinder.enabled:
+ if unwinder.enabled and unwinder.frame_type == frame_type:
unwind_info = unwinder(pending_frame)
if unwind_info is not None:
return (unwind_info, unwinder.name)
for unwinder in current_progspace().frame_unwinders:
- if unwinder.enabled:
+ if unwinder.enabled and unwinder.frame_type == frame_type:
unwind_info = unwinder(pending_frame)
if unwind_info is not None:
return (unwind_info, unwinder.name)
for unwinder in frame_unwinders:
- if unwinder.enabled:
+ if unwinder.enabled and unwinder.frame_type == frame_type:
unwind_info = unwinder(pending_frame)
if unwind_info is not None:
return (unwind_info, unwinder.name)
@@ -29,7 +29,7 @@ class Unwinder(object):
enabled: A boolean indicating whether the unwinder is enabled.
"""
- def __init__(self, name):
+ def __init__(self, name, frame_type=gdb.NORMAL_FRAME):
"""Constructor.
Args:
@@ -39,9 +39,14 @@ class Unwinder(object):
if not isinstance(name, str):
raise TypeError("incorrect type for name: %s" % type(name))
+ self._frame_type = frame_type
self._name = name
self._enabled = True
+ @property
+ def frame_type(self):
+ return self._frame_type
+
@property
def name(self):
return self._name
@@ -844,7 +844,8 @@ pyuw_sniffer (const struct frame_unwind *self, frame_info_ptr this_frame,
/* A (gdb.UnwindInfo, str) tuple, or None. */
gdbpy_ref<> pyo_execute_ret
(PyObject_CallFunctionObjArgs (pyo_execute.get (),
- pyo_pending_frame.get (), NULL));
+ pyo_pending_frame.get (),
+ PyLong_FromLong(self->type), NULL));
if (pyo_execute_ret == nullptr)
{
/* If the unwinder is cancelled due to a Ctrl-C, then propagate
@@ -965,6 +966,19 @@ pyuw_on_new_gdbarch (struct gdbarch *newarch)
unwinder->sniffer = pyuw_sniffer;
unwinder->dealloc_cache = pyuw_dealloc_cache;
frame_unwind_prepend_unwinder (newarch, unwinder);
+
+ struct frame_unwind *unwinder_signals
+ = GDBARCH_OBSTACK_ZALLOC (newarch, struct frame_unwind);
+
+ unwinder_signals->name = "python-sigtramp";
+ unwinder_signals->type = SIGTRAMP_FRAME;
+ unwinder_signals->stop_reason = default_frame_unwind_stop_reason;
+ unwinder_signals->this_id = pyuw_this_id;
+ unwinder_signals->prev_register = pyuw_prev_register;
+ unwinder_signals->unwind_data = (const struct frame_data *) newarch;
+ unwinder_signals->sniffer = pyuw_sniffer;
+ unwinder_signals->dealloc_cache = pyuw_dealloc_cache;
+ frame_unwind_prepend_unwinder (newarch, unwinder_signals);
data->unwinder_registered = 1;
}
}