diff mbox

Go patch committed: Functions that call defer_retaddr not inlinable

Message ID mcr1tq87x3e.fsf@iant-glaptop.roam.corp.google.com
State New
Headers show

Commit Message

Ian Lance Taylor Oct. 16, 2014, 5:17 p.m. UTC
It does not work to split functions that call __go_set_defer_retaddr,
because those functions are designed to work as a single unit.  They
always look like this:
    if (__go_set_defer_retaddr (&&L))
      goto L;
    real_function (real_args);
  L:
The function __go_set_defer_retaddr always returns false, but the
middle-end doesn't know that.  Unfortunately this look like a candidate
for splitting, but it really isn't because we want the label to appear
immediately after the all to the real function, so that the real
function can use it to determine whether is permitted to call recover.
In general nothing goes wrong, because the function is split and then
recombined.  But we shouldn't let it be split in the first place.

This is PR 63560, and the proposed solution is to not split functions
marked as not inlinable.  The point of splitting a function is so that
the split off part can be inlined, but that is inappropriate if the
function is marked non-inlinable.

This patch implements this on the Go side by marking these functions as
non-inlinable.  This has no effect other than on splitting
optimizations, as these functions are never called directly; they are
only ever passed as function pointers to __go_defer, so there is never
an inlining opportunity in any case.

Bootstrapped and ran Go testsuite on x86_64-unknown-linux-gnu.
Committed to mainline.

Ian
diff mbox

Patch

diff -r 551d7176fbd7 go/gogo.cc
--- a/go/gogo.cc	Mon Oct 13 14:02:46 2014 -0700
+++ b/go/gogo.cc	Thu Oct 16 10:07:52 2014 -0700
@@ -4945,6 +4945,14 @@ 
       // our return address comparison.
       bool is_inlinable = !(this->calls_recover_ || this->is_recover_thunk_);
 
+      // If a function calls __go_set_defer_retaddr, then mark it as
+      // uninlinable.  This prevents the GCC backend from splitting
+      // the function; splitting the function is a bad idea because we
+      // want the return address label to be in the same function as
+      // the call.
+      if (this->calls_defer_retaddr_)
+	is_inlinable = false;
+
       // If this is a thunk created to call a function which calls
       // the predeclared recover function, we need to disable
       // stack splitting for the thunk.
diff -r 551d7176fbd7 go/gogo.h
--- a/go/gogo.h	Mon Oct 13 14:02:46 2014 -0700
+++ b/go/gogo.h	Thu Oct 16 10:07:52 2014 -0700
@@ -1065,6 +1065,12 @@ 
   set_has_recover_thunk()
   { this->has_recover_thunk_ = true; }
 
+  // Record that this function is a thunk created for a defer
+  // statement that calls the __go_set_defer_retaddr runtime function.
+  void
+  set_calls_defer_retaddr()
+  { this->calls_defer_retaddr_ = true; }
+
   // Mark the function as going into a unique section.
   void
   set_in_unique_section()
@@ -1190,6 +1196,9 @@ 
   bool is_recover_thunk_ : 1;
   // True if this function already has a recover thunk.
   bool has_recover_thunk_ : 1;
+  // True if this is a thunk built for a defer statement that calls
+  // the __go_set_defer_retaddr runtime function.
+  bool calls_defer_retaddr_ : 1;
   // True if this function should be put in a unique section.  This is
   // turned on for field tracking.
   bool in_unique_section_ : 1;
diff -r 551d7176fbd7 go/statements.cc
--- a/go/statements.cc	Mon Oct 13 14:02:46 2014 -0700
+++ b/go/statements.cc	Thu Oct 16 10:07:52 2014 -0700
@@ -2376,6 +2376,8 @@ 
 						  location);
       s->determine_types();
       gogo->add_statement(s);
+
+      function->func_value()->set_calls_defer_retaddr();
     }
 
   // Get a reference to the parameter.