Patch for 96948

Patch for 96948

[Kirill Müller]

[-- 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)

Re: Patch for 96948

[Martin Storsjö]

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

Re: Patch for 96948

[Kirill Müller]

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

Re: Patch for 96948

[Martin Storsjö]

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

Re: Patch for 96948

[Kirill Müller]

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

Re: Patch for 96948

[Martin Storsjö]

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

Re: Patch for 96948

[Jeff Law]

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

Re: Patch for 96948

[Martin Storsjö]

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