* Patch for 96948
@ 2020-09-07 6:12 Kirill Müller
2020-09-08 12:08 ` Martin Storsjö
0 siblings, 1 reply; 8+ messages in thread
From: Kirill Müller @ 2020-09-07 6:12 UTC (permalink / raw)
To: gcc-patches
[-- Attachment #1: Type: text/plain, Size: 184 bytes --]
Hi
As requested, attaching a patch for
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96948. This solves a
problem with _Unwind_Backtrace() on mingw64 + SEH.
Best regards
Kirill
[-- Attachment #2: 48.patch --]
[-- Type: text/x-patch, Size: 1503 bytes --]
From bbc163476cab9bcaaa044d8aa9ecee824f7284f5 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Kirill=20M=C3=BCller?= <krlmlr@mailbox.org>
Date: Sun, 6 Sep 2020 10:01:46 +0200
Subject: [PATCH] Fix _Unwind_Backtrace() for SEH
---
libgcc/unwind-seh.c | 14 +++++++++++---
1 file changed, 11 insertions(+), 3 deletions(-)
diff --git a/libgcc/unwind-seh.c b/libgcc/unwind-seh.c
index 8c6aade9a3b3..a2d51b6df02b 100644
--- a/libgcc/unwind-seh.c
+++ b/libgcc/unwind-seh.c
@@ -450,8 +450,11 @@ _Unwind_Backtrace(_Unwind_Trace_Fn trace,
gcc_context.disp->ContextRecord = &ms_context;
gcc_context.disp->HistoryTable = &ms_history;
+ int first = 1;
while (1)
{
+ gcc_context.cfa = ms_context.Rsp;
+ gcc_context.ra = ms_context.Rip;
gcc_context.disp->ControlPc = ms_context.Rip;
gcc_context.disp->FunctionEntry
= RtlLookupFunctionEntry (ms_context.Rip, &gcc_context.disp->ImageBase,
@@ -466,9 +469,14 @@ _Unwind_Backtrace(_Unwind_Trace_Fn trace,
&gcc_context.disp->HandlerData,
&gcc_context.disp->EstablisherFrame, NULL);
- /* Call trace function. */
- if (trace (&gcc_context, trace_argument) != _URC_NO_REASON)
- return _URC_FATAL_PHASE1_ERROR;
+ /* Call trace function, skip first call. */
+ if (first) {
+ first = 0;
+ }
+ else {
+ if (trace (&gcc_context, trace_argument) != _URC_NO_REASON)
+ return _URC_FATAL_PHASE1_ERROR;
+ }
/* ??? Check for invalid stack pointer. */
if (ms_context.Rip == 0)
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: Patch for 96948
2020-09-07 6:12 Patch for 96948 Kirill Müller
@ 2020-09-08 12:08 ` Martin Storsjö
2020-09-08 13:29 ` Kirill Müller
0 siblings, 1 reply; 8+ messages in thread
From: Martin Storsjö @ 2020-09-08 12:08 UTC (permalink / raw)
To: Kirill Müller; +Cc: gcc-patches
Hi,
On Mon, 7 Sep 2020, Kirill Müller via Gcc-patches wrote:
> As requested, attaching a patch for
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96948. This solves a problem
> with _Unwind_Backtrace() on mingw64 + SEH.
What a coincidence - I actually sent a patch for the exact same thing
last week, see
https://gcc.gnu.org/pipermail/gcc-patches/2020-September/553082.html.
My version doesn't set gcc_context.cfa though, but is simpler by avoiding
the whole "first" flag logic.
I can send an updated patch that also sets gcc_context.cfa in a smilar
manner to the previous one.
// Martin
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: Patch for 96948
2020-09-08 12:08 ` Martin Storsjö
@ 2020-09-08 13:29 ` Kirill Müller
2020-09-08 15:34 ` Martin Storsjö
0 siblings, 1 reply; 8+ messages in thread
From: Kirill Müller @ 2020-09-08 13:29 UTC (permalink / raw)
To: Martin Storsjö; +Cc: gcc-patches
Hi
Thanks for the heads up. The coincidence is funny -- a file that hasn't
been touched for years.
I do believe that we need the logic around the `first` flag for
consistency with the other unwind-*.c implementations. This can be
verified by running the tests in the included libbacktrace library (e.g.
via make check), in particular btest.c . See
https://github.com/ianlancetaylor/libbacktrace/issues/43 for the
downstream ticket.
Best regards
Kirill
On 08.09.20 14:08, Martin Storsjö wrote:
> Hi,
>
> On Mon, 7 Sep 2020, Kirill Müller via Gcc-patches wrote:
>
>> As requested, attaching a patch for
>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96948. This solves a
>> problem with _Unwind_Backtrace() on mingw64 + SEH.
>
> What a coincidence - I actually sent a patch for the exact same thing
> last week, see
> https://gcc.gnu.org/pipermail/gcc-patches/2020-September/553082.html.
>
> My version doesn't set gcc_context.cfa though, but is simpler by
> avoiding the whole "first" flag logic.
>
> I can send an updated patch that also sets gcc_context.cfa in a smilar
> manner to the previous one.
>
> // Martin
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: Patch for 96948
2020-09-08 13:29 ` Kirill Müller
@ 2020-09-08 15:34 ` Martin Storsjö
2020-09-08 15:50 ` Kirill Müller
2020-11-06 16:42 ` Jeff Law
0 siblings, 2 replies; 8+ messages in thread
From: Martin Storsjö @ 2020-09-08 15:34 UTC (permalink / raw)
To: Kirill Müller; +Cc: gcc-patches
Hi,
On Tue, 8 Sep 2020, Kirill Müller wrote:
> Thanks for the heads up. The coincidence is funny -- a file
> that hasn't been touched for years.
I think we both may originally be triggered from the same guy asking
around in different places about implementations of _Unwind_Backtrace for
windows, actually.
> I do believe that we need the logic around the `first` flag
> for consistency with the other unwind-*.c implementations.
Yes, if you store ms_context.Rip/Rsp before the RtlVirtualUnwind step -
but my patch stores them afterwards; after RtlVirtualUnwind, before
calling the callback.
The result should be the same, except if using the first flag approach, I
believe you're missing the last frame that is printed if using my patch.
// Martin
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: Patch for 96948
2020-09-08 15:34 ` Martin Storsjö
@ 2020-09-08 15:50 ` Kirill Müller
2020-09-08 19:34 ` Martin Storsjö
2020-11-06 16:42 ` Jeff Law
1 sibling, 1 reply; 8+ messages in thread
From: Kirill Müller @ 2020-09-08 15:50 UTC (permalink / raw)
To: Martin Storsjö; +Cc: gcc-patches
Hi
Thanks for the explanation, this makes sense now.
I haven't actually tested if the cfa value (ms_context.Rsp) is valid, I
also see no reason why it shouldn't be.
What does it take for your patch to be accepted? What's the minimum gcc
version where it will be available?
Best regards
Kirill
On 08.09.20 17:34, Martin Storsjö wrote:
> Hi,
>
> On Tue, 8 Sep 2020, Kirill Müller wrote:
>
>> Thanks for the heads up. The coincidence is funny -- a file that
>> hasn't been touched for years.
>
> I think we both may originally be triggered from the same guy asking
> around in different places about implementations of _Unwind_Backtrace
> for windows, actually.
>
>> I do believe that we need the logic around the `first` flag for
>> consistency with the other unwind-*.c implementations.
>
> Yes, if you store ms_context.Rip/Rsp before the RtlVirtualUnwind step
> - but my patch stores them afterwards; after RtlVirtualUnwind, before
> calling the callback.
>
> The result should be the same, except if using the first flag
> approach, I believe you're missing the last frame that is printed if
> using my patch.
>
> // Martin
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: Patch for 96948
2020-09-08 15:50 ` Kirill Müller
@ 2020-09-08 19:34 ` Martin Storsjö
0 siblings, 0 replies; 8+ messages in thread
From: Martin Storsjö @ 2020-09-08 19:34 UTC (permalink / raw)
To: Kirill Müller; +Cc: gcc-patches
Hi,
On Tue, 8 Sep 2020, Kirill Müller wrote:
> I haven't actually tested if the cfa value (ms_context.Rsp) is valid, I also
> see no reason why it shouldn't be.
>
> What does it take for your patch to be accepted? What's the minimum gcc
> version where it will be available?
I'm not a maintainer nor a committer here, so it takes someone with such a
role/privileges to review and commit it, and presumably it'd be part of
the upcoming GCC 11 then, unless someone chooses to backport it to current
release branches.
// Martin
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: Patch for 96948
2020-09-08 15:34 ` Martin Storsjö
2020-09-08 15:50 ` Kirill Müller
@ 2020-11-06 16:42 ` Jeff Law
2020-11-06 20:53 ` Martin Storsjö
1 sibling, 1 reply; 8+ messages in thread
From: Jeff Law @ 2020-11-06 16:42 UTC (permalink / raw)
To: Martin Storsjö, Kirill Müller; +Cc: gcc-patches
On 9/8/20 9:34 AM, Martin Storsjö wrote:
> Hi,
>
> On Tue, 8 Sep 2020, Kirill Müller wrote:
>
>> Thanks for the heads up. The coincidence is funny -- a file that
>> hasn't been touched for years.
>
> I think we both may originally be triggered from the same guy asking
> around in different places about implementations of _Unwind_Backtrace
> for windows, actually.
>
>> I do believe that we need the logic around the `first` flag for
>> consistency with the other unwind-*.c implementations.
>
> Yes, if you store ms_context.Rip/Rsp before the RtlVirtualUnwind step
> - but my patch stores them afterwards; after RtlVirtualUnwind, before
> calling the callback.
>
> The result should be the same, except if using the first flag
> approach, I believe you're missing the last frame that is printed if
> using my patch.
Presumably with your patch installed, the patch from Kirill is
unnecessary, right?
jeff
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: Patch for 96948
2020-11-06 16:42 ` Jeff Law
@ 2020-11-06 20:53 ` Martin Storsjö
0 siblings, 0 replies; 8+ messages in thread
From: Martin Storsjö @ 2020-11-06 20:53 UTC (permalink / raw)
To: Jeff Law; +Cc: Kirill Müller, gcc-patches
On Fri, 6 Nov 2020, Jeff Law wrote:
>
> On 9/8/20 9:34 AM, Martin Storsjö wrote:
>> Hi,
>>
>> On Tue, 8 Sep 2020, Kirill Müller wrote:
>>
>>> Thanks for the heads up. The coincidence is funny -- a file that
>>> hasn't been touched for years.
>>
>> I think we both may originally be triggered from the same guy asking
>> around in different places about implementations of _Unwind_Backtrace
>> for windows, actually.
>>
>>> I do believe that we need the logic around the `first` flag for
>>> consistency with the other unwind-*.c implementations.
>>
>> Yes, if you store ms_context.Rip/Rsp before the RtlVirtualUnwind step
>> - but my patch stores them afterwards; after RtlVirtualUnwind, before
>> calling the callback.
>>
>> The result should be the same, except if using the first flag
>> approach, I believe you're missing the last frame that is printed if
>> using my patch.
>
> Presumably with your patch installed, the patch from Kirill is
> unnecessary, right?
Indeed, as far as I know, this issue should be fixed now (but I'd
appreciate if Kirill can retest things as well). Thanks for your time!
// Martin
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2020-11-06 20:53 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-09-07 6:12 Patch for 96948 Kirill Müller
2020-09-08 12:08 ` Martin Storsjö
2020-09-08 13:29 ` Kirill Müller
2020-09-08 15:34 ` Martin Storsjö
2020-09-08 15:50 ` Kirill Müller
2020-09-08 19:34 ` Martin Storsjö
2020-11-06 16:42 ` Jeff Law
2020-11-06 20:53 ` Martin Storsjö
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).