diff mbox series

[v6,C,ADA] use function descriptors instead of trampolines in C

Message ID 1561412100.19181.1.camel@med.uni-goettingen.de
State New
Headers show
Series [v6,C,ADA] use function descriptors instead of trampolines in C | expand

Commit Message

Uecker, Martin June 24, 2019, 9:35 p.m. UTC 
Hi,

here is a new version of this patch. It makes "-fno-trampolines"
work for C which then makes it possible to use nested functions
without executable stack. The only change in this version is in
the documentation.

Maybe it could be reconsidered at this stage?


Bootstrapped and regression tested on x86.

Martin


    gcc/
            * common.opt (flag_trampolines): Change default.
            * calls.c (prepare_call_address): Remove check for
            flag_trampolines.  Decision is now made in FEs.
            * defaults.h (FUNCTION_ALIGNMENT): Add test for flag_trampolines.
            * tree-nested.c (convert_tramp_reference_op): Likewise.
            * toplev.c (process_options): Add warning for -fno-trampolines on
            unsupported targets.
            * doc/invoke.texi (-fno-trampolines): Document support for C.
    gcc/ada/
            * gcc-interface/trans.c (Attribute_to_gnu): Add check for
            flag_trampolines.
    gcc/c/
            * c-typeck.c (function_to_pointer_conversion): If using descriptors
            instead of trampolines, amend function address with
            FUNC_ADDR_BY_DESCRIPTOR and calls with ALL_EXPR_BY_DESCRIPTOR.
    gcc/testsuite/
            * gcc.dg/trampoline-2.c: New test.
            * lib/target-supports.exp
            (check_effective_target_notrampolines): New.

Comments

Jeff Law Aug. 9, 2019, 10:45 p.m. UTC | #1 
On 6/24/19 3:35 PM, Uecker, Martin wrote:
> 
> 
> Hi,
> 
> here is a new version of this patch. It makes "-fno-trampolines"
> work for C which then makes it possible to use nested functions
> without executable stack. The only change in this version is in
> the documentation.
> 
> Maybe it could be reconsidered at this stage?
> 
> 
> Bootstrapped and regression tested on x86.
> 
> Martin
> 
> 
>     gcc/
>             * common.opt (flag_trampolines): Change default.
>             * calls.c (prepare_call_address): Remove check for
>             flag_trampolines.  Decision is now made in FEs.
>             * defaults.h (FUNCTION_ALIGNMENT): Add test for flag_trampolines.
>             * tree-nested.c (convert_tramp_reference_op): Likewise.
>             * toplev.c (process_options): Add warning for -fno-trampolines on
>             unsupported targets.
>             * doc/invoke.texi (-fno-trampolines): Document support for C.
>     gcc/ada/
>             * gcc-interface/trans.c (Attribute_to_gnu): Add check for
>             flag_trampolines.
>     gcc/c/
>             * c-typeck.c (function_to_pointer_conversion): If using descriptors
>             instead of trampolines, amend function address with
>             FUNC_ADDR_BY_DESCRIPTOR and calls with ALL_EXPR_BY_DESCRIPTOR.
>     gcc/testsuite/
>             * gcc.dg/trampoline-2.c: New test.
>             * lib/target-supports.exp
>             (check_effective_target_notrampolines): New.
IIRC we got stuck last year on the requirement that there be some bit we
can use to distinguish that we have a function descriptor which is an
ABI change, even more so if we have to bump the function alignment
requirements to give us a bit we can use.

Which in my experience means the option won't really be used.  You have
to build the entire system with the new options and also ensure you
aren't ever running old code that was compiled without the option.

I'm not really in favor of adding the option.  But I won't stand in the
way if another maintainer wants to try and move forward with this.

jeff
Uecker, Martin Aug. 10, 2019, 7:05 a.m. UTC | #2 
Am Freitag, den 09.08.2019, 16:45 -0600 schrieb Jeff Law:
> On 6/24/19 3:35 PM, Uecker, Martin wrote:
> > 
> > 
> > Hi,
> > 
> > here is a new version of this patch. It makes "-fno-trampolines"
> > work for C which then makes it possible to use nested functions
> > without executable stack. The only change in this version is in
> > the documentation.
> > 
> > Maybe it could be reconsidered at this stage?
> > 
> > 
> > Bootstrapped and regression tested on x86.
> > 
> > Martin
> > 
> > 
> >     gcc/
> >             * common.opt (flag_trampolines): Change default.
> >             * calls.c (prepare_call_address): Remove check for
> >             flag_trampolines.  Decision is now made in FEs.
> >             * defaults.h (FUNCTION_ALIGNMENT): Add test for flag_trampolines.
> >             * tree-nested.c (convert_tramp_reference_op): Likewise.
> >             * toplev.c (process_options): Add warning for -fno-trampolines on
> >             unsupported targets.
> >             * doc/invoke.texi (-fno-trampolines): Document support for C.
> >     gcc/ada/
> >             * gcc-interface/trans.c (Attribute_to_gnu): Add check for
> >             flag_trampolines.
> >     gcc/c/
> >             * c-typeck.c (function_to_pointer_conversion): If using descriptors
> >             instead of trampolines, amend function address with
> >             FUNC_ADDR_BY_DESCRIPTOR and calls with ALL_EXPR_BY_DESCRIPTOR.
> >     gcc/testsuite/
> >             * gcc.dg/trampoline-2.c: New test.
> >             * lib/target-supports.exp
> >             (check_effective_target_notrampolines): New.
> 
> IIRC we got stuck last year on the requirement that there be some bit we
> can use to distinguish that we have a function descriptor which is an
> ABI change, even more so if we have to bump the function alignment
> requirements to give us a bit we can use.
> 
> Which in my experience means the option won't really be used.  You have
> to build the entire system with the new options and also ensure you
> aren't ever running old code that was compiled without the option.

A safe use case which does not require recompiling everything
is to use it to support localized use of nested functions, e.g.
code which uses nested functions locally but does not pass
or receive function pointers from/to library functions.

As this is the typical use case for nested functions this is
very useful to support them in cases where executable  stacks
are not an option.

The other thing to note is that the minimum function alignment
is usually always automatically high enough with optimization.

> I'm not really in favor of adding the option.  But I won't stand in the
> way if another maintainer wants to try and move forward with this.

The option already exists. It just does not work for
C which is surprising.

Best,
Martin
diff mbox series

Patch

diff --git a/gcc/ChangeLog b/gcc/ChangeLog
index d2dd391b39c..568e203bdcc 100644
--- a/gcc/ChangeLog
+++ b/gcc/ChangeLog
@@ -1,3 +1,16 @@ 
+2019-06-24  Martin Uecker  <martin.uecker@med.uni-goettingen.de>
+
+	* common.opt (flag_trampolines): Change default.
+	* calls.c (prepare_call_address): Remove check for
+	flag_trampolines.  Decision is now made in FEs.
+	* defaults.h (FUNCTION_ALIGNMENT): Add test for flag_trampolines.
+	* tree-nested.c (convert_tramp_reference_op): Likewise.
+	* toplev.c (process_options): Add warning for -fno-trampolines on
+	unsupported targets.
+	* doc/invoke.texi (-fno-trampolines): Document support for C.
+	* doc/sourcebuild.texi (target attributes): Document new
+	"notrampolines" effective target keyword.
+
 2019-06-22  Jeff Law  <law@redhat.com>
 
 	* config/avr/avr.c (TARGET_BUILTIN_SETJMP_FRAME_VALUE): Remove.
diff --git a/gcc/ada/ChangeLog b/gcc/ada/ChangeLog
index 1b6aa2fd11f..5b8d0ef44c2 100644
--- a/gcc/ada/ChangeLog
+++ b/gcc/ada/ChangeLog
@@ -1,3 +1,8 @@ 
+2019-06-24  Martin Uecker  <martin.uecker@med.uni-goettingen.de>
+
+	* gcc-interface/trans.c (Attribute_to_gnu): Add check for
+	flag_trampolines.
+
 2019-06-18  Arnaud Charlet  <charlet@adacore.com>
 
 PR ada/80590
diff --git a/gcc/ada/gcc-interface/trans.c b/gcc/ada/gcc-interface/trans.c
index e2d2ddae3fe..cd244808954 100644
--- a/gcc/ada/gcc-interface/trans.c
+++ b/gcc/ada/gcc-interface/trans.c
@@ -2267,7 +2267,8 @@  Attribute_to_gnu (Node_Id gnat_node, tree *gnu_result_type_p, int attribute)
 	      if ((attribute == Attr_Access
 		   || attribute == Attr_Unrestricted_Access)
 		  && targetm.calls.custom_function_descriptors > 0
-		  && Can_Use_Internal_Rep (Etype (gnat_node)))
+		  && Can_Use_Internal_Rep (Etype (gnat_node))
+                  && flag_trampolines != 1)
 		FUNC_ADDR_BY_DESCRIPTOR (gnu_expr) = 1;
 
 	      /* Otherwise, we need to check that we are not violating the
@@ -5106,7 +5107,8 @@  Call_to_gnu (Node_Id gnat_node, tree *gnu_result_type_p, tree gnu_target,
       /* If the access type doesn't require foreign-compatible representation,
 	 be prepared for descriptors.  */
       if (targetm.calls.custom_function_descriptors > 0
-	  && Can_Use_Internal_Rep (Etype (Prefix (Name (gnat_node)))))
+	  && Can_Use_Internal_Rep (Etype (Prefix (Name (gnat_node))))
+          && flag_trampolines != 1)
 	by_descriptor = true;
     }
   else if (Nkind (Name (gnat_node)) == N_Attribute_Reference)
diff --git a/gcc/c/ChangeLog b/gcc/c/ChangeLog
index 103634eff73..b724ed7d99c 100644
--- a/gcc/c/ChangeLog
+++ b/gcc/c/ChangeLog
@@ -1,3 +1,9 @@ 
+2019-06-24  Martin Uecker  <martin.uecker@med.uni-goettingen.de>
+
+	* c-typeck.c (function_to_pointer_conversion): If using descriptors
+	instead of trampolines, amend function address with
+	FUNC_ADDR_BY_DESCRIPTOR and calls with ALL_EXPR_BY_DESCRIPTOR.
+
 2019-06-10  Jakub Jelinek  <jakub@redhat.com>
 
 	* c-parser.c (c_parser_pragma): Reject PRAGMA_OMP_SCAN.
diff --git a/gcc/c/c-typeck.c b/gcc/c/c-typeck.c
index 6abfd101f30..2216518e024 100644
--- a/gcc/c/c-typeck.c
+++ b/gcc/c/c-typeck.c
@@ -1914,7 +1914,14 @@  function_to_pointer_conversion (location_t loc, tree exp)
   if (TREE_NO_WARNING (orig_exp))
     TREE_NO_WARNING (exp) = 1;
 
-  return build_unary_op (loc, ADDR_EXPR, exp, false);
+  tree r = build_unary_op (loc, ADDR_EXPR, exp, false);
+
+  if (TREE_CODE(r) == ADDR_EXPR
+      && targetm.calls.custom_function_descriptors > 0
+      && flag_trampolines == 0)
+     FUNC_ADDR_BY_DESCRIPTOR (r) = 1;
+
+  return r;
 }
 
 /* Mark EXP as read, not just set, for set but not used -Wunused
@@ -3136,6 +3143,12 @@  build_function_call_vec (location_t loc, vec<location_t> arg_loc,
   else
     result = build_call_array_loc (loc, TREE_TYPE (fntype),
 				   function, nargs, argarray);
+
+  if (TREE_CODE (result) == CALL_EXPR
+      && targetm.calls.custom_function_descriptors > 0
+      && flag_trampolines == 0)
+    CALL_EXPR_BY_DESCRIPTOR (result) = 1;
+
   /* If -Wnonnull warning has been diagnosed, avoid diagnosing it again
      later.  */
   if (warned_p && TREE_CODE (result) == CALL_EXPR)
diff --git a/gcc/calls.c b/gcc/calls.c
index 6ab138e7bb0..5b06be821da 100644
--- a/gcc/calls.c
+++ b/gcc/calls.c
@@ -221,7 +221,7 @@  prepare_call_address (tree fndecl_or_type, rtx funexp, rtx static_chain_value,
     {
       /* If it's an indirect call by descriptor, generate code to perform
 	 runtime identification of the pointer and load the descriptor.  */
-      if ((flags & ECF_BY_DESCRIPTOR) && !flag_trampolines)
+      if (flags & ECF_BY_DESCRIPTOR)
 	{
 	  const int bit_val = targetm.calls.custom_function_descriptors;
 	  rtx call_lab = gen_label_rtx ();
diff --git a/gcc/common.opt b/gcc/common.opt
index a1544d06824..33f05e74a99 100644
--- a/gcc/common.opt
+++ b/gcc/common.opt
@@ -2554,7 +2554,7 @@  Common Report Var(flag_tracer) Optimization
 Perform superblock formation via tail duplication.
 
 ftrampolines
-Common Report Var(flag_trampolines) Init(0)
+Common Report Var(flag_trampolines) Init(-1)
 For targets that normally need trampolines for nested functions, always
 generate them instead of using descriptors.
 
diff --git a/gcc/defaults.h b/gcc/defaults.h
index b7534256119..be5f933b7d8 100644
--- a/gcc/defaults.h
+++ b/gcc/defaults.h
@@ -1054,7 +1054,8 @@  see the files COPYING3 and COPYING.RUNTIME respectively.  If not, see
 /* Force minimum alignment to be able to use the least significant bits
    for distinguishing descriptor addresses from code addresses.  */
 #define FUNCTION_ALIGNMENT(ALIGN)					\
-  (lang_hooks.custom_function_descriptors				\
+  ((   lang_hooks.custom_function_descriptors				\
+    || flag_trampolines == 0)						\
    && targetm.calls.custom_function_descriptors > 0			\
    ? MAX ((ALIGN),						\
 	  2 * targetm.calls.custom_function_descriptors * BITS_PER_UNIT)\
diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
index 4f93c7e0de7..50d321a146f 100644
--- a/gcc/doc/invoke.texi
+++ b/gcc/doc/invoke.texi
@@ -14217,12 +14217,12 @@  made executable in order for the program to work properly.
 basis to let the compiler avoid generating them, if it computes that this
 is safe, and replace them with descriptors.  Descriptors are made up of data
 only, but the generated code must be prepared to deal with them.  As of this
-writing, @option{-fno-trampolines} is enabled by default only for Ada.
+writing, @option{-fno-trampolines} is supported only for C and Ada and
+enabled by default only for Ada.
 
-Moreover, code compiled with @option{-ftrampolines} and code compiled with
-@option{-fno-trampolines} are not binary compatible if nested functions are
-present.  This option must therefore be used on a program-wide basis and be
-manipulated with extreme care.
+@strong{Warning:} Code compiled with @option{-ftrampolines} and code compiled
+with @option{-fno-trampolines} are not binary compatible.  This option must
+therefore be used on a program-wide basis and be manipulated with extreme care.
 
 @item -fvisibility=@r{[}default@r{|}internal@r{|}hidden@r{|}protected@r{]}
 @opindex fvisibility
diff --git a/gcc/doc/sourcebuild.texi b/gcc/doc/sourcebuild.texi
index 85efadb3ca1..61d2a8b823a 100644
--- a/gcc/doc/sourcebuild.texi
+++ b/gcc/doc/sourcebuild.texi
@@ -2172,6 +2172,9 @@  Target supports Newlib.
 GCC was configured with @code{--enable-newlib-nano-formatted-io}, which reduces
 the code size of Newlib formatted I/O functions.
 
+@item notrampolines
+Target supports option @option{-fno-trampolines}.
+
 @item pow10
 Target provides @code{pow10} function.
 
diff --git a/gcc/testsuite/ChangeLog b/gcc/testsuite/ChangeLog
index 3063ea4831f..71ce075ffbd 100644
--- a/gcc/testsuite/ChangeLog
+++ b/gcc/testsuite/ChangeLog
@@ -1,3 +1,9 @@ 
+2019-06-24  Martin Uecker  <martin.uecker@med.uni-goettingen.de>
+
+	* gcc.dg/trampoline-2.c: New test.
+	* lib/target-supports.exp 
+	(check_effective_target_notrampolines): New.
+
 2019-06-22  Jerry DeLisle  <jvdelisle@gcc.gnu.org>
 
 	PR fortran/89782
diff --git a/gcc/testsuite/gcc.dg/trampoline-2.c b/gcc/testsuite/gcc.dg/trampoline-2.c
new file mode 100644
index 00000000000..06c1cf4f647
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/trampoline-2.c
@@ -0,0 +1,23 @@ 
+/* test that nested function work without trampolines for -fno-trampolines */
+/* Origin: Martin Uecker <martin.uecker@med.uni-goettingen.de> */
+/* { dg-require-effective-target notrampolines } */
+/* { dg-options "-std=gnu11 -O2 -Wtrampolines -fno-trampolines" } */
+
+static int p(void) { return +1; }
+static int m(void) { return -1; } 
+static int z(void) { return 0; }
+
+typedef int (*funptr_t)(void);
+
+static int A(int k, funptr_t a1, funptr_t a2, funptr_t a3, funptr_t a4, funptr_t a5)
+{
+	int B(void) { return A(--k, B, a1, a2, a3, a4); }
+
+	return (k <= 0) ? (a4() + a5()) : (B());
+}
+
+int main(void)
+{
+	return (0 == A(5, p, m, m, p, z)) ? 0 : 1;
+}
+
diff --git a/gcc/testsuite/lib/target-supports.exp b/gcc/testsuite/lib/target-supports.exp
index 1d4aaa2a87e..4cb30bb91a8 100644
--- a/gcc/testsuite/lib/target-supports.exp
+++ b/gcc/testsuite/lib/target-supports.exp
@@ -937,6 +937,14 @@  proc check_effective_target_scheduling {} {
     } "-fschedule-insns"]
 }
 
+# Return 1 if it is possible to use function descriptors instead of trampolines, 0 otherwise.
+
+proc check_effective_target_notrampolines {} {
+    return [check_no_compiler_messages notrampolines assembly {
+        void foo (void) { }
+    } "-fno-trampolines"]
+}
+
 # Return 1 if trapping arithmetic is available, 0 otherwise.
 
 proc check_effective_target_trapping {} {
diff --git a/gcc/toplev.c b/gcc/toplev.c
index 116be7be395..8f8830d90f3 100644
--- a/gcc/toplev.c
+++ b/gcc/toplev.c
@@ -1699,6 +1699,12 @@  process_options (void)
       flag_prefetch_loop_arrays = 0;
     }
 
+  if (flag_trampolines == 0 && targetm.calls.custom_function_descriptors == -1)
+   {
+     warning_at (UNKNOWN_LOCATION, 0,
+                 "-fno-trampolines not supported for this target");
+   }
+
   /* This combination of options isn't handled for i386 targets and doesn't
      make much sense anyway, so don't allow it.  */
   if (flag_prefetch_loop_arrays > 0 && optimize_size)
diff --git a/gcc/tree-nested.c b/gcc/tree-nested.c
index 22aa6423756..daa516e7ae6 100644
--- a/gcc/tree-nested.c
+++ b/gcc/tree-nested.c
@@ -2558,7 +2558,7 @@  convert_tramp_reference_op (tree *tp, int *walk_subtrees, void *data)
 	continue;
 
       /* Decide whether to generate a descriptor or a trampoline. */
-      descr = FUNC_ADDR_BY_DESCRIPTOR (t) && !flag_trampolines;
+      descr = FUNC_ADDR_BY_DESCRIPTOR (t);
 
       if (descr)
 	x = lookup_descr_for_decl (i, decl, INSERT);