Bug 49243 - attribute((returns_twice)) doesn't work
attribute((returns_twice)) doesn't work
Status: RESOLVED FIXED
Product: gcc
Classification: Unclassified
Component: tree-optimization
4.7.0
: P3 normal
: 4.6.1
Assigned To: Not yet assigned to anyone
: wrong-code
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-05-31 16:21 UTC by Mikael Pettersson
Modified: 2011-06-06 15:03 UTC (History)
0 users

See Also:
Host:
Target: x86_64-pc-linux-gnu, i686-pc-linux-gnu
Build:
Known to work: 3.4.6, 4.6.1, 4.7.0
Known to fail: 4.0.4, 4.1.2, 4.2.4, 4.3.5, 4.4.6, 4.5.2, 4.6.0
Last reconfirmed:


Attachments
test case (2.29 KB, text/plain)
2011-05-31 16:22 UTC, Mikael Pettersson
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Mikael Pettersson 2011-05-31 16:21:52 UTC
According to the documentation, attribute((returns_twice)) is supposed to allow people to call alternate implementations of setjmp-like functions.

It doesn't work.

In my test case, calling "_setjmp" (glibc's name) works and prevents inlining and other optimizations that are invalid in the presence of setjmp and longjmp, but calling "my_setjmp" (declared with returns_twice) doesn't prevent those invalid optimizations.  This results in important side-effects disappearing if my longjmp replacement is called.

The test case is a little long to inline here (114 lines) so I'll summarize.

There's a decoder that reads and processes characters, and it's called in a loop to process strings of unknown length.

In the real application it's possible for the input to be followed by an inaccessible page.  This is handled by a wrapper function that calls setjmp() before calling the decoding loop, and having the read fault handler longjmp to the wrapper, which then returns a special value.  The test case simulates the read fault by calling longjmp() on the second input character.

In case of a read fault the toplevel needs to know the corresponding invalid input address.  To do this it passes down the address of a local variable that initially points to the start of the input.  The decoder loop increments this variable after each successful decoding step.  After a read fault, the toplevel uses the updated variable to see how much was successfully decoded.

The purpose of the intermediate wrapper function and passing down a pointer by reference is to limit the number of auto variables that become indeterminate after a longjmp, and to avoid having to mark auto variables as volatile, see the longjmp spec in C99 TC2 or C1X.

The real application is sort-of embedded and uses it's own mini-libc, including setjmp/longjmp replacements written in assembly code.  The test case simulates that by wrapping libc's setjmp behind new names, and declares the setjmp() replacement with __attribute__((returns_twice)) following gcc's documentation.  The test case can also be compiled to use libc's setjmp as-is.

First try with libc's setjmp as-is:
> gcc -O2 returns-twice-bug.c; ./a.out
(No abort == success.)

Now try with the setjmp replacement:
> gcc -O2 -DMYSETJMP returns-twice-bug.c ; ./a.out
Abort

What happened is that the read fault was detected but the toplevel variable that should have been updated to point to the invalid address wasn't.

Repeating the above two gcc calls with -S and comparing the .s files one sees that the wrapper function with the setjmp call is eliminated and inlined into the toplevel, and the decoding loop no longer writes to the in-memory pointer variable.

Now try again with the setjmp replacement but declare the wrapper function with __attribute__((noinline)):
> gcc -O2 -DMYSETJMP -DKLUDGE returns-twice-bug.c ; ./a.out
(No abort == success.)

Comparing the assembly code from the first and third steps we see that they are now essentially the same, differing only in the third step code having the setjmp/longjmp wrapper function definitions, the labels in subsequent code, and the name of the setjmp function called from the wrapper.

So the important effect of calling libc's setjmp is that it prevents inlining of the calling function, but declaring a function of a different name but with returns_twice doesn't have that required effect.  That's the bug.

A quick check shows that glibc's setjmp() isn't declared with returns_twice, and gcc recognizes it from its name alone.  Apparently the name-based checks cause more magic to happen than attribute((returns_twice)).

This bug occurs on x86_64-linux and i686-linux with every gcc-4.x including current 4.7 trunk.  With gcc-3.x it doesn't occur.  I haven't checked non-x86 targets.
Comment 1 Mikael Pettersson 2011-05-31 16:22:43 UTC
Created attachment 24404 [details]
test case
Comment 2 Mikael Pettersson 2011-05-31 22:49:36 UTC
Comparing tree dumps from gcc-4.7 on a reduced compile-only test case shows that the code that calls _setjmp (a) and the code that calls my_setjmp (b) start to differ after 019t.inline_param1:

--- a/pr49243.c.019t.inline_param1      2011-05-31 23:29:47.000000000 +0200
+++ b/pr49243.c.019t.inline_param1      2011-05-31 23:30:10.000000000 +0200
@@ -4,7 +4,7 @@
 
 Analyzing function body size: wrapper
 
-Inline summary for wrapper/0
+Inline summary for wrapper/0 inlinable
   self time:       33
   global time:     0
   self size:       16

Adding -Winline one finds that the code that calls _setjmp is marked non-inlinable because setjmp_call_p says so in inline_forbidden_p_stmt,
however it doesn't say so for the code that calls my_setjmp.  setjmp_call_p calls special_function_p, which only looks at the spelling of the function.  The closely related flags_from_decl_or_type does look at both attributes and spelling; the following crude patch to make setjmp_call_p call the latter instead fixes the reduced test case for me:

--- gcc-4.7-20110528/gcc/calls.c.~1~    2011-05-25 13:00:14.000000000 +0200
+++ gcc-4.7-20110528/gcc/calls.c        2011-06-01 00:02:13.000000000 +0200
@@ -554,7 +554,7 @@ special_function_p (const_tree fndecl, i
 int
 setjmp_call_p (const_tree fndecl)
 {
-  return special_function_p (fndecl, 0) & ECF_RETURNS_TWICE;
+  return flags_from_decl_or_type (fndecl) & ECF_RETURNS_TWICE;
 }
 
 
(This is a tad expensive and can be optimized.)

The other calls to special_function_p () look safe though.
Comment 3 Richard Biener 2011-06-01 10:30:42 UTC
Looks good.  Can you test the patch and post it to gcc-patches?
Comment 4 Mikael Pettersson 2011-06-02 15:33:40 UTC
Patch posted:
http://gcc.gnu.org/ml/gcc-patches/2011-06/msg00145.html
Comment 5 Richard Biener 2011-06-06 11:43:34 UTC
Author: rguenth
Date: Mon Jun  6 11:43:31 2011
New Revision: 174695

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=174695
Log:
2011-06-06  Mikael Pettersson  <mikpe@it.uu.se>

	PR tree-optimization/49243
	* calls.c (setjmp_call_p): Also check if fndecl has the
	returns_twice attribute.

	* gcc.dg/pr49243.c: New.

Added:
    trunk/gcc/testsuite/gcc.dg/pr49243.c
Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/calls.c
    trunk/gcc/testsuite/ChangeLog
Comment 6 Richard Biener 2011-06-06 11:46:16 UTC
Author: rguenth
Date: Mon Jun  6 11:46:14 2011
New Revision: 174696

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=174696
Log:
2011-06-06  Mikael Pettersson  <mikpe@it.uu.se>

	PR tree-optimization/49243
	* calls.c (setjmp_call_p): Also check if fndecl has the
	returns_twice attribute.

	* gcc.dg/pr49243.c: New.

Added:
    branches/gcc-4_6-branch/gcc/testsuite/gcc.dg/pr49243.c
Modified:
    branches/gcc-4_6-branch/gcc/ChangeLog
    branches/gcc-4_6-branch/gcc/calls.c
    branches/gcc-4_6-branch/gcc/testsuite/ChangeLog
Comment 7 Mikael Pettersson 2011-06-06 13:32:23 UTC
Fixed for 4.6.1/4.7.0.  The fix does work for 4.5 and 4.4 too, but given the age and relative obscurity of the bug I'm not pushing it to older branches.