diff mbox

[BZ,#18508] S390: Fix "backtrace() returns infinitely deep stack frames with makecontext()".

Message ID mlm3f3$bbu$1@ger.gmane.org
State Committed
Headers show

Commit Message

Stefan Liebler June 15, 2015, 8:47 a.m. UTC
On 06/12/2015 05:40 PM, Andreas Schwab wrote:
> Stefan Liebler <stli@linux.vnet.ibm.com> writes:
>
>> Is there a way to load the correct libgcc_s.so on all architectures?
>
> #include <gnu/lib-names.h>
> dlopen(LIBGCC_S_SO, ...)
>
> Andreas.
>
Thanks.
Here is the patch with the enhanced tst-makecontext testcase.
It calls _Unwind_Backtrace within cf() function, which is called in a 
makecontext context and check if the callback function is not called 
infinitely times.

Tested on s390/s390x before and after this patch.
Before: Test fails; After: Test succeeds.

Ok to commit?

Bye Stefan

---
2015-06-15  Stefan Liebler  <stli@linux.vnet.ibm.com>

	[BZ #18508]
	* stdlib/Makefile ($(objpfx)tst-makecontext3):
	Depend on $(libdl).
	* stdlib/tst-makecontext.c (cf): Test if _Unwind_Backtrace
	is not called infinitely times.
	(backtrace_helper): New function.
	(trace_arg): New struct.
	(st1): Enlarge stack size.
	* sysdeps/unix/sysv/linux/s390/s390-32/__makecontext_ret.S:
	(__makecontext_ret): Omit cfi_startproc and cfi_endproc.
	* sysdeps/unix/sysv/linux/s390/s390-64/__makecontext_ret.S:
	Likewise.

Comments

Stefan Liebler June 19, 2015, 10:44 a.m. UTC | #1
PING

On 06/15/2015 10:47 AM, Stefan Liebler wrote:
> On 06/12/2015 05:40 PM, Andreas Schwab wrote:
>> Stefan Liebler <stli@linux.vnet.ibm.com> writes:
>>
>>> Is there a way to load the correct libgcc_s.so on all architectures?
>>
>> #include <gnu/lib-names.h>
>> dlopen(LIBGCC_S_SO, ...)
>>
>> Andreas.
>>
> Thanks.
> Here is the patch with the enhanced tst-makecontext testcase.
> It calls _Unwind_Backtrace within cf() function, which is called in a
> makecontext context and check if the callback function is not called
> infinitely times.
>
> Tested on s390/s390x before and after this patch.
> Before: Test fails; After: Test succeeds.
>
> Ok to commit?
>
> Bye Stefan
>
> ---
> 2015-06-15  Stefan Liebler  <stli@linux.vnet.ibm.com>
>
>      [BZ #18508]
>      * stdlib/Makefile ($(objpfx)tst-makecontext3):
>      Depend on $(libdl).
>      * stdlib/tst-makecontext.c (cf): Test if _Unwind_Backtrace
>      is not called infinitely times.
>      (backtrace_helper): New function.
>      (trace_arg): New struct.
>      (st1): Enlarge stack size.
>      * sysdeps/unix/sysv/linux/s390/s390-32/__makecontext_ret.S:
>      (__makecontext_ret): Omit cfi_startproc and cfi_endproc.
>      * sysdeps/unix/sysv/linux/s390/s390-64/__makecontext_ret.S:
>      Likewise.
Stefan Liebler June 26, 2015, 11:11 a.m. UTC | #2
ping

On 06/19/2015 12:44 PM, Stefan Liebler wrote:
> PING
>
> On 06/15/2015 10:47 AM, Stefan Liebler wrote:
>> On 06/12/2015 05:40 PM, Andreas Schwab wrote:
>>> Stefan Liebler <stli@linux.vnet.ibm.com> writes:
>>>
>>>> Is there a way to load the correct libgcc_s.so on all architectures?
>>>
>>> #include <gnu/lib-names.h>
>>> dlopen(LIBGCC_S_SO, ...)
>>>
>>> Andreas.
>>>
>> Thanks.
>> Here is the patch with the enhanced tst-makecontext testcase.
>> It calls _Unwind_Backtrace within cf() function, which is called in a
>> makecontext context and check if the callback function is not called
>> infinitely times.
>>
>> Tested on s390/s390x before and after this patch.
>> Before: Test fails; After: Test succeeds.
>>
>> Ok to commit?
>>
>> Bye Stefan
>>
>> ---
>> 2015-06-15  Stefan Liebler  <stli@linux.vnet.ibm.com>
>>
>>      [BZ #18508]
>>      * stdlib/Makefile ($(objpfx)tst-makecontext3):
>>      Depend on $(libdl).
>>      * stdlib/tst-makecontext.c (cf): Test if _Unwind_Backtrace
>>      is not called infinitely times.
>>      (backtrace_helper): New function.
>>      (trace_arg): New struct.
>>      (st1): Enlarge stack size.
>>      * sysdeps/unix/sysv/linux/s390/s390-32/__makecontext_ret.S:
>>      (__makecontext_ret): Omit cfi_startproc and cfi_endproc.
>>      * sysdeps/unix/sysv/linux/s390/s390-64/__makecontext_ret.S:
>>      Likewise.
>
>
Stefan Liebler July 3, 2015, 7:58 a.m. UTC | #3
ping

On 06/26/2015 01:11 PM, Stefan Liebler wrote:
> ping
>
> On 06/19/2015 12:44 PM, Stefan Liebler wrote:
>> PING
>>
>> On 06/15/2015 10:47 AM, Stefan Liebler wrote:
>>> On 06/12/2015 05:40 PM, Andreas Schwab wrote:
>>>> Stefan Liebler <stli@linux.vnet.ibm.com> writes:
>>>>
>>>>> Is there a way to load the correct libgcc_s.so on all architectures?
>>>>
>>>> #include <gnu/lib-names.h>
>>>> dlopen(LIBGCC_S_SO, ...)
>>>>
>>>> Andreas.
>>>>
>>> Thanks.
>>> Here is the patch with the enhanced tst-makecontext testcase.
>>> It calls _Unwind_Backtrace within cf() function, which is called in a
>>> makecontext context and check if the callback function is not called
>>> infinitely times.
>>>
>>> Tested on s390/s390x before and after this patch.
>>> Before: Test fails; After: Test succeeds.
>>>
>>> Ok to commit?
>>>
>>> Bye Stefan
>>>
>>> ---
>>> 2015-06-15  Stefan Liebler  <stli@linux.vnet.ibm.com>
>>>
>>>      [BZ #18508]
>>>      * stdlib/Makefile ($(objpfx)tst-makecontext3):
>>>      Depend on $(libdl).
>>>      * stdlib/tst-makecontext.c (cf): Test if _Unwind_Backtrace
>>>      is not called infinitely times.
>>>      (backtrace_helper): New function.
>>>      (trace_arg): New struct.
>>>      (st1): Enlarge stack size.
>>>      * sysdeps/unix/sysv/linux/s390/s390-32/__makecontext_ret.S:
>>>      (__makecontext_ret): Omit cfi_startproc and cfi_endproc.
>>>      * sysdeps/unix/sysv/linux/s390/s390-64/__makecontext_ret.S:
>>>      Likewise.
>>
>>
>
>
Florian Weimer July 3, 2015, 9:11 a.m. UTC | #4
On 06/15/2015 10:47 AM, Stefan Liebler wrote:
> +/* We do not want .eh_frame info for __makecontext_ret to stop unwinding
> +   if backtrace() was called within a context created by makecontext.
> +   (there is also no .eh_frame info for _start or thread_start)  */

> +/* We do not want .eh_frame info for __makecontext_ret to stop unwinding
> +   if backtrace() was called within a context created by makecontext.
> +   (there is also no .eh_frame info for _start or thread_start)  */

I think this should read:

“We do not want .eh_frame ifno so that __makecontext_ret stops unwinding
if backtrace was called within a context created by makecontext.  (There
is also no .eh_frame info for _start or thread_start.)”

Could a native speaker review this, please?

Rest of the patch seems okay to me.
H.J. Lu July 7, 2015, 5:31 p.m. UTC | #5
On Fri, Jul 3, 2015 at 2:11 AM, Florian Weimer <fweimer@redhat.com> wrote:
> On 06/15/2015 10:47 AM, Stefan Liebler wrote:
>> +/* We do not want .eh_frame info for __makecontext_ret to stop unwinding
>> +   if backtrace() was called within a context created by makecontext.
>> +   (there is also no .eh_frame info for _start or thread_start)  */
>
>> +/* We do not want .eh_frame info for __makecontext_ret to stop unwinding
>> +   if backtrace() was called within a context created by makecontext.
>> +   (there is also no .eh_frame info for _start or thread_start)  */
>
> I think this should read:
>
> “We do not want .eh_frame ifno so that __makecontext_ret stops unwinding
> if backtrace was called within a context created by makecontext.  (There
> is also no .eh_frame info for _start or thread_start.)”
>
> Could a native speaker review this, please?
>
> Rest of the patch seems okay to me.
>

The new test failed on Fedora/22/ i686:

cf (i=-78) at tst-makecontext.c:50
50 {
(gdb)
55  if (i != othervar || thr != 94)
(gdb)
51  struct trace_arg arg = { .size = 100, .cnt = -1 };
(gdb)
55  if (i != othervar || thr != 94)
(gdb)
66  handle = dlopen (LIBGCC_S_SO, RTLD_LAZY);
(gdb)
67  if (handle != NULL)
(gdb)
66  handle = dlopen (LIBGCC_S_SO, RTLD_LAZY);
(gdb)
67  if (handle != NULL)
(gdb)
69      unwind_backtrace = dlsym (handle, "_Unwind_Backtrace");
(gdb)
70      if (unwind_backtrace != NULL)
(gdb)
72  unwind_backtrace (backtrace_helper, &arg);
(gdb)

Program received signal SIGSEGV, Segmentation fault.
0xf7de14a1 in ?? () from /lib/libgcc_s.so.1
H.J. Lu July 7, 2015, 5:47 p.m. UTC | #6
On Tue, Jul 7, 2015 at 10:31 AM, H.J. Lu <hjl.tools@gmail.com> wrote:
> On Fri, Jul 3, 2015 at 2:11 AM, Florian Weimer <fweimer@redhat.com> wrote:
>> On 06/15/2015 10:47 AM, Stefan Liebler wrote:
>>> +/* We do not want .eh_frame info for __makecontext_ret to stop unwinding
>>> +   if backtrace() was called within a context created by makecontext.
>>> +   (there is also no .eh_frame info for _start or thread_start)  */
>>
>>> +/* We do not want .eh_frame info for __makecontext_ret to stop unwinding
>>> +   if backtrace() was called within a context created by makecontext.
>>> +   (there is also no .eh_frame info for _start or thread_start)  */
>>
>> I think this should read:
>>
>> “We do not want .eh_frame ifno so that __makecontext_ret stops unwinding
>> if backtrace was called within a context created by makecontext.  (There
>> is also no .eh_frame info for _start or thread_start.)”
>>
>> Could a native speaker review this, please?
>>
>> Rest of the patch seems okay to me.
>>
>
> The new test failed on Fedora/22/ i686:
>
> cf (i=-78) at tst-makecontext.c:50
> 50 {
> (gdb)
> 55  if (i != othervar || thr != 94)
> (gdb)
> 51  struct trace_arg arg = { .size = 100, .cnt = -1 };
> (gdb)
> 55  if (i != othervar || thr != 94)
> (gdb)
> 66  handle = dlopen (LIBGCC_S_SO, RTLD_LAZY);
> (gdb)
> 67  if (handle != NULL)
> (gdb)
> 66  handle = dlopen (LIBGCC_S_SO, RTLD_LAZY);
> (gdb)
> 67  if (handle != NULL)
> (gdb)
> 69      unwind_backtrace = dlsym (handle, "_Unwind_Backtrace");
> (gdb)
> 70      if (unwind_backtrace != NULL)
> (gdb)
> 72  unwind_backtrace (backtrace_helper, &arg);
> (gdb)
>
> Program received signal SIGSEGV, Segmentation fault.
> 0xf7de14a1 in ?? () from /lib/libgcc_s.so.1
>

I opened:

https://sourceware.org/bugzilla/show_bug.cgi?id=18635
Carlos O'Donell July 7, 2015, 7:02 p.m. UTC | #7
On 07/07/2015 01:47 PM, H.J. Lu wrote:
> I opened:
> https://sourceware.org/bugzilla/show_bug.cgi?id=18635

Stefan,

Please fix this promptly as glibc 2.22 will be released
shortly and the test results should be clean. If we can't
fix it promptly, then we should revert the test changes.

Cheers,
Carlos.
diff mbox

Patch

diff --git a/stdlib/Makefile b/stdlib/Makefile
index 3300dd2..7fc5a80 100644
--- a/stdlib/Makefile
+++ b/stdlib/Makefile
@@ -164,3 +164,5 @@  $(objpfx)tst-setcontext3.out: tst-setcontext3.sh $(objpfx)tst-setcontext3
 		 '$(run-program-env)' '$(test-program-prefix-after-env)' \
 		 $(common-objpfx)stdlib/; \
 	$(evaluate-test)
+
+$(objpfx)tst-makecontext: $(libdl)
diff --git a/stdlib/tst-makecontext.c b/stdlib/tst-makecontext.c
index 29a588e..8170e8a 100644
--- a/stdlib/tst-makecontext.c
+++ b/stdlib/tst-makecontext.c
@@ -19,23 +19,62 @@ 
 #include <stdlib.h>
 #include <stdio.h>
 #include <ucontext.h>
+#include <assert.h>
+#include <unwind.h>
+#include <dlfcn.h>
+#include <gnu/lib-names.h>
 
 ucontext_t ucp;
-char st1[8192];
+char st1[16384];
 __thread int thr;
 
 int somevar = -76;
 long othervar = -78L;
 
+struct trace_arg
+{
+  int cnt, size;
+};
+
+static _Unwind_Reason_Code
+backtrace_helper (struct _Unwind_Context *ctx, void *a)
+{
+  struct trace_arg *arg = a;
+  if (++arg->cnt == arg->size)
+    return _URC_END_OF_STACK;
+  return _URC_NO_REASON;
+}
+
 void
 cf (int i)
 {
+  struct trace_arg arg = { .size = 100, .cnt = -1 };
+  void *handle;
+  _Unwind_Reason_Code (*unwind_backtrace) (_Unwind_Trace_Fn, void *);
+
   if (i != othervar || thr != 94)
     {
       printf ("i %d thr %d\n", i, thr);
       exit (1);
     }
 
+  /* Test if callback function of _Unwind_Backtrace is not called infinitely
+     times. See Bug 18508 or gcc bug "Bug 66303 - runtime.Caller() returns
+     infinitely deep stack frames on s390x.".
+     The go runtime calls backtrace_full() in
+     <gcc-src>/libbacktrace/backtrace.c, which uses _Unwind_Backtrace().  */
+  handle = dlopen (LIBGCC_S_SO, RTLD_LAZY);
+  if (handle != NULL)
+    {
+      unwind_backtrace = dlsym (handle, "_Unwind_Backtrace");
+      if (unwind_backtrace != NULL)
+	{
+	  unwind_backtrace (backtrace_helper, &arg);
+	  assert (arg.cnt != -1 && arg.cnt < 100);
+	}
+      dlclose (handle);
+    }
+
   /* Since uc_link below has been set to NULL, setcontext is supposed to
      terminate the process normally after this function returns.  */
 }
diff --git a/sysdeps/unix/sysv/linux/s390/s390-32/__makecontext_ret.S b/sysdeps/unix/sysv/linux/s390/s390-32/__makecontext_ret.S
index e1f9347..8d36ea2 100644
--- a/sysdeps/unix/sysv/linux/s390/s390-32/__makecontext_ret.S
+++ b/sysdeps/unix/sysv/linux/s390/s390-32/__makecontext_ret.S
@@ -17,6 +17,14 @@ 
 
 #include <sysdep.h>
 
+/* We do not want .eh_frame info for __makecontext_ret to stop unwinding
+   if backtrace() was called within a context created by makecontext.
+   (there is also no .eh_frame info for _start or thread_start)  */
+#undef cfi_startproc
+#define cfi_startproc
+#undef cfi_endproc
+#define cfi_endproc
+
 ENTRY(__makecontext_ret)
 	basr  %r14,%r7
 	ltr   %r8,%r8			/* Check whether uc_link is 0.  */
diff --git a/sysdeps/unix/sysv/linux/s390/s390-64/__makecontext_ret.S b/sysdeps/unix/sysv/linux/s390/s390-64/__makecontext_ret.S
index 11a3cd3..64619f1 100644
--- a/sysdeps/unix/sysv/linux/s390/s390-64/__makecontext_ret.S
+++ b/sysdeps/unix/sysv/linux/s390/s390-64/__makecontext_ret.S
@@ -17,6 +17,14 @@ 
 
 #include <sysdep.h>
 
+/* We do not want .eh_frame info for __makecontext_ret to stop unwinding
+   if backtrace() was called within a context created by makecontext.
+   (there is also no .eh_frame info for _start or thread_start)  */
+#undef cfi_startproc
+#define cfi_startproc
+#undef cfi_endproc
+#define cfi_endproc
+
 ENTRY(__makecontext_ret)
 	basr	%r14,%r7
 	ltgr	%r8,%r8			/* Check whether uc_link is 0.  */