[RFC] [PATCH][C][ADA] use function descriptors instead of trampolines in C

[RFC] [PATCH][C][ADA] use function descriptors instead of trampolines in C

[Uecker, Martin]

A while ago Eric Botcazou added support for function descriptors
as a replacement for trampolines. This is used in Ada, but not
in C where it would also imply a change of ABI. Still, as nested
functions are generally not used across interface boundaries,
I thought it would be really useful to have the option to
replace trampolines with function descriptors in C too.
This then avoids the need for an executable stack.
The main downside is that most calls through function pointers then
need to test a bit in the pointer to identify descriptors.

The following tiny patch (against recent git) is my initial attempt
to implement this idea. As most of the hard work was already done by
Eric for Ada, this turned out to be very simple. But as I am not
too familiar with the GCC code base, it might still be completely
wrong in some ways... In particular, I am not sure whether I mark
all the right tree nodes correctly in the C frontend.

The patch essentially just adds the marking of the tree nodes
at two places in C FE. The rest of the PATCH is moving the check
for the command-line flag in the front ends to be able to have
different defaults for different languages. 


I am not too deeply convinced about the security benefits of a
non-executable stack myself, but it some people like to
enforce this in general. So it might make sense to completely
transition to the use of function descriptors for nested functions.
A possible strategy for such a transition might be to first compile
all libraries with -fno-trampolines (but see downsides above).
Assuming these do not create trampolines themselves this should
cause no problems, but the libraries should then be compatible with
code compiled with trampolines and with code compiled with
descriptors.


The compiler passes the man or boy test by Donald Knuth with 
'-ftrampolines' (default for C) and '-fno-trampolines' and also
my internal project which uses nested function still passes its
complete test suite. I also verified that an executable stack
is not used anymore.

More testing is in progress.


Best,
Martin


diff --git a/gcc/ada/gcc-interface/trans.c b/gcc/ada/gcc-interface/trans.c
index 31e098a0c70..3eb2e71a2bd 100644
--- a/gcc/ada/gcc-interface/trans.c
+++ b/gcc/ada/gcc-interface/trans.c
@@ -1755,7 +1755,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
@@ -4327,7 +4328,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/c-objc-common.h b/gcc/c/c-objc-common.h
index 78e768c2366..ef039560eb9 100644
--- a/gcc/c/c-objc-common.h
+++ b/gcc/c/c-objc-common.h
@@ -110,4 +110,7 @@ along with GCC; see the file COPYING3.  If not see
 
 #undef LANG_HOOKS_TREE_INLINING_VAR_MOD_TYPE_P
 #define LANG_HOOKS_TREE_INLINING_VAR_MOD_TYPE_P c_vla_unspec_p
+
+#undef LANG_HOOKS_CUSTOM_FUNCTION_DESCRIPTORS
+#define LANG_HOOKS_CUSTOM_FUNCTION_DESCRIPTORS true
 #endif /* GCC_C_OBJC_COMMON */
diff --git a/gcc/c/c-typeck.c b/gcc/c/c-typeck.c
index 90ae306c99a..57f3eca320b 100644
--- a/gcc/c/c-typeck.c
+++ b/gcc/c/c-typeck.c
@@ -1912,7 +1912,13 @@ 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)
+      && (flag_trampolines == 0))
+     FUNC_ADDR_BY_DESCRIPTOR (r) = 1;
+
+  return r;
 }
 
 /* Mark EXP as read, not just set, for set but not used -Wunused
@@ -3130,6 +3136,11 @@ 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)
+      && (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 384c0238748..1367e0305a3 100644
--- a/gcc/calls.c
+++ b/gcc/calls.c
@@ -230,7 +230,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 5bb645291cf..9338edd7bb7 100644
--- a/gcc/common.opt
+++ b/gcc/common.opt
@@ -2471,7 +2471,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/tree-nested.c b/gcc/tree-nested.c
index 4c8eda94f14..4b49024b917 100644
--- a/gcc/tree-nested.c
+++ b/gcc/tree-nested.c
@@ -2497,7 +2497,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);

Re: [RFC] [PATCH][C][ADA] use function descriptors instead of trampolines in C

[Uecker, Martin]

When running the test suite with this patch applied and
"-fno-trampolines", there are some errors. Most of it is expected
(e.g. nested-6.c calls qsort which fails because it has not
itself been compiled with -fno-trampolines).

One test case for __builtin_call_with_static_chain
in gcc.dg/cwsc1.cfails in an assertion in the new code
at calls.c:288.  This seems unrelated to my patch though.

Eric, can you take a look? Maybe the assertion should be
removed to allow overriding the static chain?

Joseph, I mistyped your email address before. Could take a look
whether the C changes make sense to you?

Best,
Martin

$ gcc/xgcc -Bgcc -Wall -fno-trampolines -O3 cwsc1.c 
during RTL pass: expand
cwsc1.c: In function ‘main’:
cwsc1.c:39:46: internal compiler error: in prepare_call_address, at calls.c:288
   void *x = __builtin_call_with_static_chain(ptr(), &c);
             ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~
0x5bd37d prepare_call_address(tree_node*, rtx_def*, rtx_def*, rtx_def**, int, int)
        ../../gcc/gcc/calls.c:288
0x860947 expand_call(tree_node*, rtx_def*, int)
        ../../gcc/gcc/calls.c:4173
0x977358 expand_expr_real_1(tree_node*, rtx_def*, machine_mode, expand_modifier, rtx_def**, bool)
        ../../gcc/gcc/expr.c:10914
0x98375d store_expr(tree_node*, rtx_def*, int, bool, bool)
        ../../gcc/gcc/expr.c:5614
0x984b1c expand_assignment(tree_node*, tree_node*, bool)
        ../../gcc/gcc/expr.c:5398
0x872ac2 expand_call_stmt
        ../../gcc/gcc/cfgexpand.c:2685
0x872ac2 expand_gimple_stmt_1
        ../../gcc/gcc/cfgexpand.c:3575
0x872ac2 expand_gimple_stmt
        ../../gcc/gcc/cfgexpand.c:3734
0x873e3f expand_gimple_basic_block
        ../../gcc/gcc/cfgexpand.c:5769
0x878a17 execute
        ../../gcc/gcc/cfgexpand.c:6372
Please submit a full bug report,
with preprocessed source if appropriate.
Please include the complete backtrace with any bug report.
See <https://gcc.gnu.org/bugs/> for instructions.


Am Samstag, den 11.08.2018, 18:40 +0200 schrieb Martin Uecker:
> 
> 
> A while ago Eric Botcazou added support for function descriptors
> as a replacement for trampolines. This is used in Ada, but not
> in C where it would also imply a change of ABI. Still, as nested
> functions are generally not used across interface boundaries,
> I thought it would be really useful to have the option to
> replace trampolines with function descriptors in C too.
> This then avoids the need for an executable stack.
> The main downside is that most calls through function pointers then
> need to test a bit in the pointer to identify descriptors.
> 
> The following tiny patch (against recent git) is my initial attempt
> to implement this idea. As most of the hard work was already done by
> Eric for Ada, this turned out to be very simple. But as I am not
> too familiar with the GCC code base, it might still be completely
> wrong in some ways... In particular, I am not sure whether I mark
> all the right tree nodes correctly in the C frontend.
> 
> The patch essentially just adds the marking of the tree nodes
> at two places in C FE. The rest of the PATCH is moving the check
> for the command-line flag in the front ends to be able to have
> different defaults for different languages. 
> 
> 
> I am not too deeply convinced about the security benefits of a
> non-executable stack myself, but it some people like to
> enforce this in general. So it might make sense to completely
> transition to the use of function descriptors for nested functions.
> A possible strategy for such a transition might be to first compile
> all libraries with -fno-trampolines (but see downsides above).
> Assuming these do not create trampolines themselves this should
> cause no problems, but the libraries should then be compatible with
> code compiled with trampolines and with code compiled with
> descriptors.
> 
> 
> The compiler passes the man or boy test by Donald Knuth with 
> '-ftrampolines' (default for C) and '-fno-trampolines' and also
> my internal project which uses nested function still passes its
> complete test suite. I also verified that an executable stack
> is not used anymore.
> 
> More testing is in progress.
> 
> 
> Best,
> Martin
> 
> 
> diff --git a/gcc/ada/gcc-interface/trans.c b/gcc/ada/gcc-interface/trans.c
> index 31e098a0c70..3eb2e71a2bd 100644
> --- a/gcc/ada/gcc-interface/trans.c
> +++ b/gcc/ada/gcc-interface/trans.c
> @@ -1755,7 +1755,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
> @@ -4327,7 +4328,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/c-objc-common.h b/gcc/c/c-objc-common.h
> index 78e768c2366..ef039560eb9 100644
> --- a/gcc/c/c-objc-common.h
> +++ b/gcc/c/c-objc-common.h
> @@ -110,4 +110,7 @@ along with GCC; see the file COPYING3.  If not see
>  
>  #undef LANG_HOOKS_TREE_INLINING_VAR_MOD_TYPE_P
>  #define LANG_HOOKS_TREE_INLINING_VAR_MOD_TYPE_P c_vla_unspec_p
> +
> +#undef LANG_HOOKS_CUSTOM_FUNCTION_DESCRIPTORS
> +#define LANG_HOOKS_CUSTOM_FUNCTION_DESCRIPTORS true
>  #endif /* GCC_C_OBJC_COMMON */
> diff --git a/gcc/c/c-typeck.c b/gcc/c/c-typeck.c
> index 90ae306c99a..57f3eca320b 100644
> --- a/gcc/c/c-typeck.c
> +++ b/gcc/c/c-typeck.c
> @@ -1912,7 +1912,13 @@ 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)
> +      && (flag_trampolines == 0))
> +     FUNC_ADDR_BY_DESCRIPTOR (r) = 1;
> +
> +  return r;
>  }
>  
>  /* Mark EXP as read, not just set, for set but not used -Wunused
> @@ -3130,6 +3136,11 @@ 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)
> +      && (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 384c0238748..1367e0305a3 100644
> --- a/gcc/calls.c
> +++ b/gcc/calls.c
> @@ -230,7 +230,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 5bb645291cf..9338edd7bb7 100644
> --- a/gcc/common.opt
> +++ b/gcc/common.opt
> @@ -2471,7 +2471,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/tree-nested.c b/gcc/tree-nested.c
> index 4c8eda94f14..4b49024b917 100644
> --- a/gcc/tree-nested.c
> +++ b/gcc/tree-nested.c
> @@ -2497,7 +2497,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);

[PATCH v2][C][ADA] use function descriptors instead of trampolines in C

[Uecker, Martin]

This is a new version which adds proper changelog entries and
a test case (no actual code changes).

Bootstrapped an regression tested on x86_64.



    gcc/
            * common.opt (flag_trampolines): Change default.
            * calls.c (prepare_call_address): Remove check for
            flag_trampolines.  Decision is now made in FEs.
            * tree-nested.c (convert_tramp_reference_op): Likewise.
    gcc/ada/
            * gcc-interface/trans.c (Attribute_to_gnu): Add check for
            flag_trampolines.
    gcc/c/
            * c-objc-common.h: Define
LANG_HOOKS_CUSTOM_FUNCTION_DESCRIPTORS.
            * 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.



diff --git a/gcc/ChangeLog b/gcc/ChangeLog
index ec1fb242c23..e5f3844e8fd 100644
--- a/gcc/ChangeLog
+++ b/gcc/ChangeLog
@@ -1,3 +1,10 @@
+2018-08-20  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.
+	* tree-nested.c (convert_tramp_reference_op): Likewise.
+
 2018-08-19  Uros Bizjak  <ubizjak@gmail.com>
 
 	PR target/86994
diff --git a/gcc/ada/ChangeLog b/gcc/ada/ChangeLog
index 792811fb989..d906909774b 100644
--- a/gcc/ada/ChangeLog
+++ b/gcc/ada/ChangeLog
@@ -1,3 +1,8 @@
+2018-08-20  Martin Uecker  <martin.uecker@med.uni-goettingen.de>
+
+	* gcc-interface/trans.c (Attribute_to_gnu): Add check for
+	flag_trampolines.
+
 2018-08-03  Pierre-Marie de Rodat  <derodat@adacore.com>
 
 	Reverts
diff --git a/gcc/ada/gcc-interface/trans.c b/gcc/ada/gcc-
interface/trans.c
index 0371d00fce1..1b95f7070ac 100644
--- a/gcc/ada/gcc-interface/trans.c
+++ b/gcc/ada/gcc-interface/trans.c
@@ -1769,7 +1769,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
@@ -4341,7 +4342,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 7bde11c1f19..028a7284e41 100644
--- a/gcc/c/ChangeLog
+++ b/gcc/c/ChangeLog
@@ -1,3 +1,10 @@
+2018-08-20  Martin Uecker  <martin.uecker@med.uni-goettingen.de>
+
+	* c-objc-common.h: Define
LANG_HOOKS_CUSTOM_FUNCTION_DESCRIPTORS.
+	* 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.
+
 2018-08-15  David Malcolm  <dmalcolm@redhat.com>
 
 	* c-objc-common.c: Include "gcc-rich-location.h".
diff --git a/gcc/c/c-objc-common.h b/gcc/c/c-objc-common.h
index 78e768c2366..ef039560eb9 100644
--- a/gcc/c/c-objc-common.h
+++ b/gcc/c/c-objc-common.h
@@ -110,4 +110,7 @@ along with GCC; see the file COPYING3.  If not see
 
 #undef LANG_HOOKS_TREE_INLINING_VAR_MOD_TYPE_P
 #define LANG_HOOKS_TREE_INLINING_VAR_MOD_TYPE_P c_vla_unspec_p
+
+#undef LANG_HOOKS_CUSTOM_FUNCTION_DESCRIPTORS
+#define LANG_HOOKS_CUSTOM_FUNCTION_DESCRIPTORS true
 #endif /* GCC_C_OBJC_COMMON */
diff --git a/gcc/c/c-typeck.c b/gcc/c/c-typeck.c
index 726ea832ae1..a6a962d1e01 100644
--- a/gcc/c/c-typeck.c
+++ b/gcc/c/c-typeck.c
@@ -1912,7 +1912,13 @@ 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)
+      && (flag_trampolines == 0))
+     FUNC_ADDR_BY_DESCRIPTOR (r) = 1;
+
+  return r;
 }
 
 /* Mark EXP as read, not just set, for set but not used -Wunused
@@ -3135,6 +3141,11 @@ 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)
+      && (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 384c0238748..1367e0305a3 100644
--- a/gcc/calls.c
+++ b/gcc/calls.c
@@ -230,7 +230,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 ebc3ef43ce2..e56b134ba77 100644
--- a/gcc/common.opt
+++ b/gcc/common.opt
@@ -2479,7 +2479,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/testsuite/ChangeLog b/gcc/testsuite/ChangeLog
index e89f4a4c093..8469a57eac3 100644
--- a/gcc/testsuite/ChangeLog
+++ b/gcc/testsuite/ChangeLog
@@ -1,3 +1,7 @@
+2018-08-20  Martin Uecker  <martin.uecker@med.uni-goettingen.de>
+
+	* gcc.dg/trampoline-2.c: New test.
+
 2018-08-18  Iain Sandoe  <iain@sandoe.co.uk>
 
 	* gcc.dg/debug/dwarf2/pr80263.c: Suppress pubtypes output
diff --git a/gcc/testsuite/gcc.dg/trampoline-2.c
b/gcc/testsuite/gcc.dg/trampoline-2.c
new file mode 100644
index 00000000000..7b3a2b64462
--- /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-do run { target x86_64-*-* } } */
+/* { 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/tree-nested.c b/gcc/tree-nested.c
index 4c8eda94f14..4b49024b917 100644
--- a/gcc/tree-nested.c
+++ b/gcc/tree-nested.c
@@ -2497,7 +2497,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);

Re: [PATCH v2][C][ADA] use function descriptors instead of trampolines in C

[Joseph Myers]

On Mon, 20 Aug 2018, Uecker, Martin wrote:

> This is a new version which adds proper changelog entries and
> a test case (no actual code changes).

Please include the overall description of a change in every version 
submitted.  That is, the patch submission message should both include a 
description of the current version (as in a git-style commit message) and, 
if relevant, a description of what changed relative to the previous 
version of the patch (which would not go in the commit message).

A key thing I'm not clear on is what the user-visible difference in 
compiler behavior is supposed to be with this patch.  Whatever that 
user-visible difference is, I'd expect it to result in some change to the 
documentation of -ftrampolines in invoke.texi (describing the new feature, 
or changing a description of a limitation of an existing feature, or 
something like that).

> +/* { dg-do run { target x86_64-*-* } } */

It is always wrong for a test to use x86_64-*-* like that, because 
anything that should be tested for 64-bit code generation for an x86_64 
target should also be tested for i[34567]86-*-* -m64, and if you don't 
want to test for 32-bit code generation, you need to avoid testing for 
x86_64-*-* -m32, which that test would test for.  Anything genuinely 
x86-specific should go in gcc.target/i386 and then be conditioned on 
effective-target keywords such as lp64 if necessary.

I don't see why this is target-specific (if it is, the documentation for 
users in invoke.texi should explain what targets it works for and what it 
doesn't work for) anyway.  I'd expect it to be a target-independent 
feature with a target-independent test or tests.

Once there is sufficient user-level documentation showing what the 
intended semantics are, then it may be possible to evaluate how the 
implementation achieves that.

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [PATCH v2][C][ADA] use function descriptors instead of trampolines in C

[Uecker, Martin]

Am Montag, den 20.08.2018, 22:34 +0000 schrieb Joseph Myers:
> On Mon, 20 Aug 2018, Uecker, Martin wrote:
> 
> > This is a new version which adds proper changelog entries and
> > a test case (no actual code changes).
> 
> Please include the overall description of a change in every version 
> submitted.  That is, the patch submission message should both include a 
> description of the current version (as in a git-style commit message) and, 
> if relevant, a description of what changed relative to the previous 
> version of the patch (which would not go in the commit message).

Thank you. I will keep this in mind in the future.

> A key thing I'm not clear on is what the user-visible difference in 
> compiler behavior is supposed to be with this patch.  Whatever that 
> user-visible difference is, I'd expect it to result in some change to the 
> documentation of -ftrampolines in invoke.texi (describing the new feature, 
> or changing a description of a limitation of an existing feature, or 
> something like that).

The option -fno-trampolines already exists and is documented.
From the description one would think that using this option would
turn off generation of trampolines and replace them by descriptors.
This then eliminates the need for an executable stack for nested
functions.

The user visible change of my patch is that it now actually works for
the C language. So I don't think this patch needs to change the 
documentation as it makes the behavior match the existing documentation.  


Neverthless, I think the documentation of the existing option should
be improved to document the remaining limitations of this option
regarding languages and architectures. I am happy to add such
wording, and propose it as an independent patch.


> > +/* { dg-do run { target x86_64-*-* } } */
> 
> It is always wrong for a test to use x86_64-*-* like that, because 
> anything that should be tested for 64-bit code generation for an x86_64 
> target should also be tested for i[34567]86-*-* -m64, and if you don't 
> want to test for 32-bit code generation, you need to avoid testing for 
> x86_64-*-* -m32, which that test would test for.  Anything genuinely 
> x86-specific should go in gcc.target/i386 and then be conditioned on 
> effective-target keywords such as lp64 if necessary.

Thank you, I will fix this. In fact, it should be tested for all
targets which currently support custom function descriptors.

> I don't see why this is target-specific (if it is, the documentation for 
> users in invoke.texi should explain what targets it works for and what it 
> doesn't work for) anyway.  I'd expect it to be a target-independent 
> feature with a target-independent test or tests.

My understanding is that this is a target-independent feature which
has not yet been implemented for all targets. The existing
documentation does not reflect this.


> Once there is sufficient user-level documentation showing what the 
> intended semantics are, then it may be possible to evaluate how the 
> implementation achieves that.

Please refer to the existing documentation of -ftrampolines and
-fno-trampolines.

My original submission also gives some background information:

https://gcc.gnu.org/ml/gcc-patches/2018-08/msg00705.html

The original generic functionality was introduced with
this patch:

https://patchwork.ozlabs.org/patch/642247/


Best,
Martin

Re: [PATCH v2][C][ADA] use function descriptors instead of trampolines in C

[Joseph Myers]

[-- Attachment #1: Type: text/plain, Size: 1172 bytes --]

On Tue, 21 Aug 2018, Uecker, Martin wrote:

> > I don't see why this is target-specific (if it is, the documentation for 
> > users in invoke.texi should explain what targets it works for and what it 
> > doesn't work for) anyway.  I'd expect it to be a target-independent 
> > feature with a target-independent test or tests.
> 
> My understanding is that this is a target-independent feature which
> has not yet been implemented for all targets. The existing
> documentation does not reflect this.

How does one tell which targets do or do not support it?

For tests for features supported on some but not all targets, we use 
effective-target keywords.  Of course you need to be careful about how you 
implement those keywords: you don't want the implementation of the keyword 
to be essentially the same as the test being conditioned, to avoid a bug 
in the implementation quietly causing the test to be disabled.  But the 
implementation of the keyword might e.g. have a blacklist of targets that 
do not yet support the feature, with the expectation that the test should 
run and pass on all other targets.

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [PATCH v2][C][ADA] use function descriptors instead of trampolines in C

[Uecker, Martin]

Am Dienstag, den 21.08.2018, 21:34 +0000 schrieb Joseph Myers:
> On Tue, 21 Aug 2018, Uecker, Martin wrote:
> 
> > > I don't see why this is target-specific (if it is, the documentation for 
> > > users in invoke.texi should explain what targets it works for and what it 
> > > doesn't work for) anyway.  I'd expect it to be a target-independent 
> > > feature with a target-independent test or tests.
> > 
> > My understanding is that this is a target-independent feature which
> > has not yet been implemented for all targets. The existing
> > documentation does not reflect this.
> 
> How does one tell which targets do or do not support it?

There is a target hook

TARGET_CUSTOM_FUNCTION_DESCRIPTORS

But I am not sure how to get this information to the testsuite.

> For tests for features supported on some but not all targets, we use 
> effective-target keywords.  Of course you need to be careful about how you 
> implement those keywords: you don't want the implementation of the keyword 
> to be essentially the same as the test being conditioned, to avoid a bug 
> in the implementation quietly causing the test to be disabled.  But the 
> implementation of the keyword might e.g. have a blacklist of targets that 
> do not yet support the feature, with the expectation that the test should 
> run and pass on all other targets.

gcc/testsuite/lib/target-supports.exp

there seems to be infrastructure to implement this. The information seems
to come from a "target_info" structure (?) but I do not see where this
is populated.

Best,
Martin

Re: [PATCH v2][C][ADA] use function descriptors instead of trampolines in C

[Joseph Myers]

[-- Attachment #1: Type: text/plain, Size: 2804 bytes --]

On Wed, 22 Aug 2018, Uecker, Martin wrote:

> Am Dienstag, den 21.08.2018, 21:34 +0000 schrieb Joseph Myers:
> > On Tue, 21 Aug 2018, Uecker, Martin wrote:
> > 
> > > > I don't see why this is target-specific (if it is, the documentation for 
> > > > users in invoke.texi should explain what targets it works for and what it 
> > > > doesn't work for) anyway.  I'd expect it to be a target-independent 
> > > > feature with a target-independent test or tests.
> > > 
> > > My understanding is that this is a target-independent feature which
> > > has not yet been implemented for all targets. The existing
> > > documentation does not reflect this.
> > 
> > How does one tell which targets do or do not support it?
> 
> There is a target hook
> 
> TARGET_CUSTOM_FUNCTION_DESCRIPTORS
> 
> But I am not sure how to get this information to the testsuite.

Presumably through defining a check_* function in target-supports.exp that 
has a list of targets supporting the feature.  Together with 
cross-references in all the relevant places: the hook documentation in 
target.def (and thus the generated file tm.texi) should say that the list 
in target-supports.exp needs to be kept in sync if changing what targets 
implement the hook, and the list in target-supports.exp should similarly 
have a comment referencing the hook, and the user-level documentation in 
invoke.texi should list the relevant architectures, again with comments 
pointing in both directions between it and the other places needing 
updating when a change is made to the set of architectures supporting the 
feature.

Except: if the feature doesn't work on some targets, I'd expect an attempt 
to use -fno-trampolines on other targets to produce a sorry () message 
from the compiler, so it's immediately obvious to the user that the 
requested semantics are not being achieved.  And once you've got that 
sorry (), it should be something the check_* function can reliably test 
for (try compiling a trivial file with -fno-trampolines and see if it 
works, via check_no_compiler_messages*), so you don't need the list of 
targets in target-supports.exp in that case (but the documentation would 
need to say something about the option giving an error on targets not 
supporting it).

> there seems to be infrastructure to implement this. The information seems
> to come from a "target_info" structure (?) but I do not see where this
> is populated.

target_info comes from DejaGnu.  Sometimes a board file may set variables 
in target_info.  But for anything that is an architecture property of GCC, 
the board file should not need to set it; GCC should know the information 
itself.  The board file is more for describing board-specific information 
(e.g. memory available).

-- 
Joseph S. Myers
joseph@codesourcery.com

[PATCH v3][C][ADA] use function descriptors instead of trampolines in C

[Uecker, Martin]

Hi Joseph,

here is a new version of this patch which adds a warning
for targets which do not support -fno-trampolines  and
only runs the test case on architectures where this is
supported. It seems that documentation for this general
feature has improved in the meantime so I only mention
C as supported.


Best,
Martin

diff --git a/gcc/ChangeLog b/gcc/ChangeLog
index 5cf291da2d5..e75500c647a 100644
--- a/gcc/ChangeLog
+++ b/gcc/ChangeLog
@@ -1,3 +1,13 @@
+2018-11-03  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.
+	* 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.
+
 2018-11-02  Aaron Sawdey  <acsawdey@linux.ibm.com>
 
 	* config/rs6000/rs6000-string.c (expand_strncmp_gpr_sequence): Pay
diff --git a/gcc/ada/ChangeLog b/gcc/ada/ChangeLog
index 73666129f55..a7462edfc71 100644
--- a/gcc/ada/ChangeLog
+++ b/gcc/ada/ChangeLog
@@ -1,3 +1,8 @@
+2018-11-03  Martin Uecker  <martin.uecker@med.uni-goettingen.de>
+
+	* gcc-interface/trans.c (Attribute_to_gnu): Add check for
+	flag_trampolines.
+
 2018-10-22  Eric Botcazou  <ebotcazou@adacore.com>
 
 	* gcc-interface/utils.c (unchecked_convert): Use local variables for
diff --git a/gcc/ada/gcc-interface/trans.c b/gcc/ada/gcc-interface/trans.c
index ce2d43f989e..b79f2373c63 100644
--- a/gcc/ada/gcc-interface/trans.c
+++ b/gcc/ada/gcc-interface/trans.c
@@ -1753,7 +1753,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
@@ -4330,7 +4331,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 708ef5d7da2..62823ccf5c7 100644
--- a/gcc/c/ChangeLog
+++ b/gcc/c/ChangeLog
@@ -1,3 +1,10 @@
+2018-11-03  Martin Uecker  <martin.uecker@med.uni-goettingen.de>
+
+	* c-objc-common.h: Define LANG_HOOKS_CUSTOM_FUNCTION_DESCRIPTORS.
+	* 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.
+
 2018-10-29  David Malcolm  <dmalcolm@redhat.com>
 
 	* c-decl.c (implicit_decl_warning): Update "is there a suggestion"
diff --git a/gcc/c/c-objc-common.h b/gcc/c/c-objc-common.h
index 78e768c2366..ef039560eb9 100644
--- a/gcc/c/c-objc-common.h
+++ b/gcc/c/c-objc-common.h
@@ -110,4 +110,7 @@ along with GCC; see the file COPYING3.  If not see
 
 #undef LANG_HOOKS_TREE_INLINING_VAR_MOD_TYPE_P
 #define LANG_HOOKS_TREE_INLINING_VAR_MOD_TYPE_P c_vla_unspec_p
+
+#undef LANG_HOOKS_CUSTOM_FUNCTION_DESCRIPTORS
+#define LANG_HOOKS_CUSTOM_FUNCTION_DESCRIPTORS true
 #endif /* GCC_C_OBJC_COMMON */
diff --git a/gcc/c/c-typeck.c b/gcc/c/c-typeck.c
index 9d09b8d65fd..afae9de41e7 100644
--- a/gcc/c/c-typeck.c
+++ b/gcc/c/c-typeck.c
@@ -1912,7 +1912,13 @@ 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)
+      && (flag_trampolines == 0))
+     FUNC_ADDR_BY_DESCRIPTOR (r) = 1;
+
+  return r;
 }
 
 /* Mark EXP as read, not just set, for set but not used -Wunused
@@ -3134,6 +3140,11 @@ 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)
+      && (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 8978d3b42fd..95ab7d8405b 100644
--- a/gcc/calls.c
+++ b/gcc/calls.c
@@ -230,7 +230,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 2971dc21b1f..8457c93edab 100644
--- a/gcc/common.opt
+++ b/gcc/common.opt
@@ -2487,7 +2487,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/doc/invoke.texi b/gcc/doc/invoke.texi
index e290128f535..ccf651c1354 100644
--- a/gcc/doc/invoke.texi
+++ b/gcc/doc/invoke.texi
@@ -13570,7 +13570,8 @@ 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
diff --git a/gcc/testsuite/ChangeLog b/gcc/testsuite/ChangeLog
index a4d50614537..9c4bce69bd9 100644
--- a/gcc/testsuite/ChangeLog
+++ b/gcc/testsuite/ChangeLog
@@ -1,3 +1,9 @@
+2018-11-03  Martin Uecker  <martin.uecker@med.uni-goettingen.de>
+
+	* gcc.dg/trampoline-2.c: New test.
+	* lib/target-supports.exp 
+	(check_effective_target_notrampolines): New.
+
 2018-11-02  Rainer Orth  <ro@CeBiTec.Uni-Bielefeld.DE>
 
 	* gcc.dg/compat/pr83487-1_y.c: Move dg-skip-if ...
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 fd74c04d092..a34e966b7c4 100644
--- a/gcc/testsuite/lib/target-supports.exp
+++ b/gcc/testsuite/lib/target-supports.exp
@@ -916,6 +916,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 d7ea11abf53..33ded241a20 100644
--- a/gcc/toplev.c
+++ b/gcc/toplev.c
@@ -1697,6 +1697,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 4579b4c5839..e95c9aea051 100644
--- a/gcc/tree-nested.c
+++ b/gcc/tree-nested.c
@@ -2499,7 +2499,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);

Re: [PATCH v3][C][ADA] use function descriptors instead of trampolines in C

[Uecker, Martin]

Is there a change that we can move forward with this?

I think this is a very useful feature and might be especially
important if GCC is going to activate -Wtrampoline  with 
-Wall on some architectures.

Best,
Martin


Am Sonntag, den 04.11.2018, 21:48 +0100 schrieb Martin Uecker:
> Hi Joseph,
> 
> here is a new version of this patch which adds a warning
> for targets which do not support -fno-trampolines  and
> only runs the test case on architectures where this is
> supported. It seems that documentation for this general
> feature has improved in the meantime so I only mention
> C as supported.
> 
> 
> Best,
> Martin
> 
> diff --git a/gcc/ChangeLog b/gcc/ChangeLog
> index 5cf291da2d5..e75500c647a 100644
> --- a/gcc/ChangeLog
> +++ b/gcc/ChangeLog
> @@ -1,3 +1,13 @@
> +2018-11-03  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.
> +	* 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.
> +
>  2018-11-02  Aaron Sawdey  <acsawdey@linux.ibm.com>
>  
>  	* config/rs6000/rs6000-string.c
> (expand_strncmp_gpr_sequence): Pay
> diff --git a/gcc/ada/ChangeLog b/gcc/ada/ChangeLog
> index 73666129f55..a7462edfc71 100644
> --- a/gcc/ada/ChangeLog
> +++ b/gcc/ada/ChangeLog
> @@ -1,3 +1,8 @@
> +2018-11-03  Martin Uecker  <martin.uecker@med.uni-goettingen.de>
> +
> +	* gcc-interface/trans.c (Attribute_to_gnu): Add check for
> +	flag_trampolines.
> +
>  2018-10-22  Eric Botcazou  <ebotcazou@adacore.com>
>  
>  	* gcc-interface/utils.c (unchecked_convert): Use local
> variables for
> diff --git a/gcc/ada/gcc-interface/trans.c b/gcc/ada/gcc-
> interface/trans.c
> index ce2d43f989e..b79f2373c63 100644
> --- a/gcc/ada/gcc-interface/trans.c
> +++ b/gcc/ada/gcc-interface/trans.c
> @@ -1753,7 +1753,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
> @@ -4330,7 +4331,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 708ef5d7da2..62823ccf5c7 100644
> --- a/gcc/c/ChangeLog
> +++ b/gcc/c/ChangeLog
> @@ -1,3 +1,10 @@
> +2018-11-03  Martin Uecker  <martin.uecker@med.uni-goettingen.de>
> +
> +	* c-objc-common.h: Define
> LANG_HOOKS_CUSTOM_FUNCTION_DESCRIPTORS.
> +	* 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.
> +
>  2018-10-29  David Malcolm  <dmalcolm@redhat.com>
>  
>  	* c-decl.c (implicit_decl_warning): Update "is there a
> suggestion"
> diff --git a/gcc/c/c-objc-common.h b/gcc/c/c-objc-common.h
> index 78e768c2366..ef039560eb9 100644
> --- a/gcc/c/c-objc-common.h
> +++ b/gcc/c/c-objc-common.h
> @@ -110,4 +110,7 @@ along with GCC; see the file COPYING3.  If not
> see
>  
>  #undef LANG_HOOKS_TREE_INLINING_VAR_MOD_TYPE_P
>  #define LANG_HOOKS_TREE_INLINING_VAR_MOD_TYPE_P c_vla_unspec_p
> +
> +#undef LANG_HOOKS_CUSTOM_FUNCTION_DESCRIPTORS
> +#define LANG_HOOKS_CUSTOM_FUNCTION_DESCRIPTORS true
>  #endif /* GCC_C_OBJC_COMMON */
> diff --git a/gcc/c/c-typeck.c b/gcc/c/c-typeck.c
> index 9d09b8d65fd..afae9de41e7 100644
> --- a/gcc/c/c-typeck.c
> +++ b/gcc/c/c-typeck.c
> @@ -1912,7 +1912,13 @@ 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)
> +      && (flag_trampolines == 0))
> +     FUNC_ADDR_BY_DESCRIPTOR (r) = 1;
> +
> +  return r;
>  }
>  
>  /* Mark EXP as read, not just set, for set but not used -Wunused
> @@ -3134,6 +3140,11 @@ 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)
> +      && (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 8978d3b42fd..95ab7d8405b 100644
> --- a/gcc/calls.c
> +++ b/gcc/calls.c
> @@ -230,7 +230,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 2971dc21b1f..8457c93edab 100644
> --- a/gcc/common.opt
> +++ b/gcc/common.opt
> @@ -2487,7 +2487,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/doc/invoke.texi b/gcc/doc/invoke.texi
> index e290128f535..ccf651c1354 100644
> --- a/gcc/doc/invoke.texi
> +++ b/gcc/doc/invoke.texi
> @@ -13570,7 +13570,8 @@ 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
> diff --git a/gcc/testsuite/ChangeLog b/gcc/testsuite/ChangeLog
> index a4d50614537..9c4bce69bd9 100644
> --- a/gcc/testsuite/ChangeLog
> +++ b/gcc/testsuite/ChangeLog
> @@ -1,3 +1,9 @@
> +2018-11-03  Martin Uecker  <martin.uecker@med.uni-goettingen.de>
> +
> +	* gcc.dg/trampoline-2.c: New test.
> +	* lib/target-supports.exp 
> +	(check_effective_target_notrampolines): New.
> +
>  2018-11-02  Rainer Orth  <ro@CeBiTec.Uni-Bielefeld.DE>
>  
>  	* gcc.dg/compat/pr83487-1_y.c: Move dg-skip-if ...
> 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 fd74c04d092..a34e966b7c4 100644
> --- a/gcc/testsuite/lib/target-supports.exp
> +++ b/gcc/testsuite/lib/target-supports.exp
> @@ -916,6 +916,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 d7ea11abf53..33ded241a20 100644
> --- a/gcc/toplev.c
> +++ b/gcc/toplev.c
> @@ -1697,6 +1697,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 4579b4c5839..e95c9aea051 100644
> --- a/gcc/tree-nested.c
> +++ b/gcc/tree-nested.c
> @@ -2499,7 +2499,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);

Re: [PATCH v3][C][ADA] use function descriptors instead of trampolines in C

[Jeff Law]

On 11/4/18 1:48 PM, Uecker, Martin wrote:
> Hi Joseph,
> 
> here is a new version of this patch which adds a warning
> for targets which do not support -fno-trampolines  and
> only runs the test case on architectures where this is
> supported. It seems that documentation for this general
> feature has improved in the meantime so I only mention
> C as supported.
> 
> 
> Best,
> Martin
> 
> diff --git a/gcc/ada/gcc-interface/trans.c b/gcc/ada/gcc-interface/trans.c
> index ce2d43f989e..b79f2373c63 100644
> --- a/gcc/ada/gcc-interface/trans.c
> +++ b/gcc/ada/gcc-interface/trans.c
> @@ -1753,7 +1753,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;
You've got an extraneous set of parenthesis around your flag_trampolines
check.  Please remove them.


>  
>  	      /* Otherwise, we need to check that we are not violating the
> @@ -4330,7 +4331,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))
Similarly here.


> diff --git a/gcc/c/c-objc-common.h b/gcc/c/c-objc-common.h
> index 78e768c2366..ef039560eb9 100644
> --- a/gcc/c/c-objc-common.h
> +++ b/gcc/c/c-objc-common.h
> @@ -110,4 +110,7 @@ along with GCC; see the file COPYING3.  If not see
>  
>  #undef LANG_HOOKS_TREE_INLINING_VAR_MOD_TYPE_P
>  #define LANG_HOOKS_TREE_INLINING_VAR_MOD_TYPE_P c_vla_unspec_p
> +
> +#undef LANG_HOOKS_CUSTOM_FUNCTION_DESCRIPTORS
> +#define LANG_HOOKS_CUSTOM_FUNCTION_DESCRIPTORS true
>  #endif /* GCC_C_OBJC_COMMON */
I wonder if we even need the lang hook anymore.  ISTM that a front-end
that wants to use the function descriptors can just set
FUNC_ADDR_BY_DESCRIPTOR and we'd use the function descriptor, else we'll
use the trampoline.  Thoughts?


> diff --git a/gcc/c/c-typeck.c b/gcc/c/c-typeck.c
> index 9d09b8d65fd..afae9de41e7 100644
> --- a/gcc/c/c-typeck.c
> +++ b/gcc/c/c-typeck.c
> @@ -1912,7 +1912,13 @@ 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)
> +      && (flag_trampolines == 0))
> +     FUNC_ADDR_BY_DESCRIPTOR (r) = 1;
> +
> +  return r;
Extraneous parens here too.

>  }
>  
>  /* Mark EXP as read, not just set, for set but not used -Wunused
> @@ -3134,6 +3140,11 @@ 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)
> +      && (flag_trampolines == 0))
> +    CALL_EXPR_BY_DESCRIPTOR (result) = 1;
> +
And here too.


> diff --git a/gcc/testsuite/lib/target-supports.exp b/gcc/testsuite/lib/target-supports.exp
> index fd74c04d092..a34e966b7c4 100644
> --- a/gcc/testsuite/lib/target-supports.exp
> +++ b/gcc/testsuite/lib/target-supports.exp
> @@ -916,6 +916,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"]
> +}
> +
I think this needs documenting in sourcebuild.texi.   Look for
"Effective-target" to find the appropriate section.


Jeff

[PATCH v4][C][ADA] use function descriptors instead of trampolines in C

[Uecker, Martin]

Hi Jeff,

thank you. I fixed all the minor issues, but see below.


Am Montag, den 03.12.2018, 14:56 -0700 schrieb Jeff Law:
> On 11/4/18 1:48 PM, Uecker, Martin wrote:
> > Hi Joseph,
> > 
> > here is a new version of this patch which adds a warning
> > for targets which do not support -fno-trampolines  and
> > only runs the test case on architectures where this is
> > supported. It seems that documentation for this general
> > feature has improved in the meantime so I only mention
> > C as supported.
> > 
> > 
> > Best,
> > Martin
> > 
> > diff --git a/gcc/ada/gcc-interface/trans.c b/gcc/ada/gcc-interface/trans.c
> > index ce2d43f989e..b79f2373c63 100644
> > --- a/gcc/ada/gcc-interface/trans.c
> > +++ b/gcc/ada/gcc-interface/trans.c
> > @@ -1753,7 +1753,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;
> 
> You've got an extraneous set of parenthesis around your flag_trampolines
> check.  Please remove them.
> 
> 
> >  
> >  	      /* Otherwise, we need to check that we are not violating the
> > @@ -4330,7 +4331,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))
> 
> Similarly here.
> 
> 
> > diff --git a/gcc/c/c-objc-common.h b/gcc/c/c-objc-common.h
> > index 78e768c2366..ef039560eb9 100644
> > --- a/gcc/c/c-objc-common.h
> > +++ b/gcc/c/c-objc-common.h
> > @@ -110,4 +110,7 @@ along with GCC; see the file COPYING3.  If not see
> >  
> >  #undef LANG_HOOKS_TREE_INLINING_VAR_MOD_TYPE_P
> >  #define LANG_HOOKS_TREE_INLINING_VAR_MOD_TYPE_P c_vla_unspec_p
> > +
> > +#undef LANG_HOOKS_CUSTOM_FUNCTION_DESCRIPTORS
> > +#define LANG_HOOKS_CUSTOM_FUNCTION_DESCRIPTORS true
> >  #endif /* GCC_C_OBJC_COMMON */
> 
> I wonder if we even need the lang hook anymore.  ISTM that a front-end
> that wants to use the function descriptors can just set
> FUNC_ADDR_BY_DESCRIPTOR and we'd use the function descriptor, else we'll
> use the trampoline.  Thoughts?

The lang hook also affects the minimum alignment for function
pointers via the FUNCTION_ALIGNMENT macro (gcc/default.h). This does
not appear to change the default alignment on any architecture, but
it causes a failure in i386/gcc.target/i386/attr-aligned.c when
requesting a smaller alignment which is then silently ignored.

I am not sure what the best approach is, but my preference
would be to remove the lang hook and the FUNCTION_ALIGNMENT
logic which will also fix the test case (the requested
alignment will be applied).

I would then instead add a warning (or error?) which triggers
only with -fno-trampolines if the user requests an alignment
which is too small for this mechanism to work.

Does this sound reasonable?

Best,
Martin


> > diff --git a/gcc/c/c-typeck.c b/gcc/c/c-typeck.c
> > index 9d09b8d65fd..afae9de41e7 100644
> > --- a/gcc/c/c-typeck.c
> > +++ b/gcc/c/c-typeck.c
> > @@ -1912,7 +1912,13 @@ 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)
> > +      && (flag_trampolines == 0))
> > +     FUNC_ADDR_BY_DESCRIPTOR (r) = 1;
> > +
> > +  return r;
> 
> Extraneous parens here too.
> 
> >  }
> >  
> >  /* Mark EXP as read, not just set, for set but not used -Wunused
> > @@ -3134,6 +3140,11 @@ 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)
> > +      && (flag_trampolines == 0))
> > +    CALL_EXPR_BY_DESCRIPTOR (result) = 1;
> > +
> 
> And here too.
> 
> 
> > diff --git a/gcc/testsuite/lib/target-supports.exp b/gcc/testsuite/lib/target-supports.exp
> > index fd74c04d092..a34e966b7c4 100644
> > --- a/gcc/testsuite/lib/target-supports.exp
> > +++ b/gcc/testsuite/lib/target-supports.exp
> > @@ -916,6 +916,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"]
> > +}
> > +
> 
> I think this needs documenting in sourcebuild.texi.   Look for
> "Effective-target" to find the appropriate section.
> 
> 
> Jeff


diff --git a/gcc/ChangeLog b/gcc/ChangeLog
index 3c5e3ed4329..1702a9cd3cd 100644
--- a/gcc/ChangeLog
+++ b/gcc/ChangeLog
@@ -1,3 +1,15 @@
+2018-12-10  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.
+	* 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.
+
 2018-12-10  Uros Bizjak  <ubizjak@gmail.com>
 
 	PR target/88418
diff --git a/gcc/ada/ChangeLog b/gcc/ada/ChangeLog
index b48c757b816..eb14239dc62 100644
--- a/gcc/ada/ChangeLog
+++ b/gcc/ada/ChangeLog
@@ -1,3 +1,8 @@
+2018-12-10  Martin Uecker  <martin.uecker@med.uni-goettingen.de>
+
+	* gcc-interface/trans.c (Attribute_to_gnu): Add check for
+	flag_trampolines.
+
 2018-12-03  Gary Dismukes  <dismukes@adacore.com>
 
 	* sem_aux.adb (Object_Type_Has_Constrained_Partial_View): Return
diff --git a/gcc/ada/gcc-interface/trans.c b/gcc/ada/gcc-interface/trans.c
index 4c066c02421..97ce5ebd3d2 100644
--- a/gcc/ada/gcc-interface/trans.c
+++ b/gcc/ada/gcc-interface/trans.c
@@ -2239,7 +2239,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
@@ -5020,7 +5021,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 9bbfe76040b..4b4bd432304 100644
--- a/gcc/c/ChangeLog
+++ b/gcc/c/ChangeLog
@@ -1,3 +1,10 @@
+2018-12-10  Martin Uecker  <martin.uecker@med.uni-goettingen.de>
+
+	* c-objc-common.h: Define LANG_HOOKS_CUSTOM_FUNCTION_DESCRIPTORS.
+	* 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.
+
 2018-12-08  Segher Boessenkool  <segher@kernel.crashing.org>
 
 	* c-parser (c_parser_asm_statement) [RID_INLINE]: Delete stray line
diff --git a/gcc/c/c-objc-common.h b/gcc/c/c-objc-common.h
index 78e768c2366..ef039560eb9 100644
--- a/gcc/c/c-objc-common.h
+++ b/gcc/c/c-objc-common.h
@@ -110,4 +110,7 @@ along with GCC; see the file COPYING3.  If not see
 
 #undef LANG_HOOKS_TREE_INLINING_VAR_MOD_TYPE_P
 #define LANG_HOOKS_TREE_INLINING_VAR_MOD_TYPE_P c_vla_unspec_p
+
+#undef LANG_HOOKS_CUSTOM_FUNCTION_DESCRIPTORS
+#define LANG_HOOKS_CUSTOM_FUNCTION_DESCRIPTORS true
 #endif /* GCC_C_OBJC_COMMON */
diff --git a/gcc/c/c-typeck.c b/gcc/c/c-typeck.c
index 1a897273088..b14033064f3 100644
--- a/gcc/c/c-typeck.c
+++ b/gcc/c/c-typeck.c
@@ -1913,7 +1913,13 @@ 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
+      && flag_trampolines == 0)
+     FUNC_ADDR_BY_DESCRIPTOR (r) = 1;
+
+  return r;
 }
 
 /* Mark EXP as read, not just set, for set but not used -Wunused
@@ -3135,6 +3141,11 @@ 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
+      && 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 98c6377d78f..8b6742fd0ba 100644
--- a/gcc/calls.c
+++ b/gcc/calls.c
@@ -230,7 +230,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 08bffdf2c2d..5d441316ad2 100644
--- a/gcc/common.opt
+++ b/gcc/common.opt
@@ -2546,7 +2546,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/doc/invoke.texi b/gcc/doc/invoke.texi
index dd262b60d88..92f63ecc19d 100644
--- a/gcc/doc/invoke.texi
+++ b/gcc/doc/invoke.texi
@@ -14036,7 +14036,8 @@ 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
diff --git a/gcc/doc/sourcebuild.texi b/gcc/doc/sourcebuild.texi
index 1204a546c29..86ccd8ca16f 100644
--- a/gcc/doc/sourcebuild.texi
+++ b/gcc/doc/sourcebuild.texi
@@ -2162,6 +2162,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 35b4fe14c2e..8f205396e21 100644
--- a/gcc/testsuite/ChangeLog
+++ b/gcc/testsuite/ChangeLog
@@ -1,3 +1,9 @@
+2018-12-10  Martin Uecker  <martin.uecker@med.uni-goettingen.de>
+
+	* gcc.dg/trampoline-2.c: New test.
+	* lib/target-supports.exp 
+	(check_effective_target_notrampolines): New.
+
 2018-12-10  Steven G. Kargl  <kargl@gcc.gnu.org>
 
 	PR fortran/88269
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 5026c5906cd..44f3b0c1f37 100644
--- a/gcc/testsuite/lib/target-supports.exp
+++ b/gcc/testsuite/lib/target-supports.exp
@@ -924,6 +924,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 ab20cd98969..765ce347172 100644
--- a/gcc/toplev.c
+++ b/gcc/toplev.c
@@ -1698,6 +1698,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 0ad469ada49..eb9bccb7a9d 100644
--- a/gcc/tree-nested.c
+++ b/gcc/tree-nested.c
@@ -2544,7 +2544,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);

Re: [PATCH v4][C][ADA] use function descriptors instead of trampolines in C

[Jeff Law]

On 12/12/18 11:12 AM, Uecker, Martin wrote:
> 
> Hi Jeff,
> 
> thank you. I fixed all the minor issues, but see below.
> 
> 
> Am Montag, den 03.12.2018, 14:56 -0700 schrieb Jeff Law:
>> On 11/4/18 1:48 PM, Uecker, Martin wrote:
>>> Hi Joseph,
>>>
>>> here is a new version of this patch which adds a warning
>>> for targets which do not support -fno-trampolines  and
>>> only runs the test case on architectures where this is
>>> supported. It seems that documentation for this general
>>> feature has improved in the meantime so I only mention
>>> C as supported.
>>>
>>>
>>> Best,
>>> Martin
>>>
>>> diff --git a/gcc/ada/gcc-interface/trans.c b/gcc/ada/gcc-interface/trans.c
>>> index ce2d43f989e..b79f2373c63 100644
>>> --- a/gcc/ada/gcc-interface/trans.c
>>> +++ b/gcc/ada/gcc-interface/trans.c
>>> @@ -1753,7 +1753,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;
>>
>> You've got an extraneous set of parenthesis around your flag_trampolines
>> check.  Please remove them.
>>
>>
>>>  
>>>  	      /* Otherwise, we need to check that we are not violating the
>>> @@ -4330,7 +4331,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))
>>
>> Similarly here.
>>
>>
>>> diff --git a/gcc/c/c-objc-common.h b/gcc/c/c-objc-common.h
>>> index 78e768c2366..ef039560eb9 100644
>>> --- a/gcc/c/c-objc-common.h
>>> +++ b/gcc/c/c-objc-common.h
>>> @@ -110,4 +110,7 @@ along with GCC; see the file COPYING3.  If not see
>>>  
>>>  #undef LANG_HOOKS_TREE_INLINING_VAR_MOD_TYPE_P
>>>  #define LANG_HOOKS_TREE_INLINING_VAR_MOD_TYPE_P c_vla_unspec_p
>>> +
>>> +#undef LANG_HOOKS_CUSTOM_FUNCTION_DESCRIPTORS
>>> +#define LANG_HOOKS_CUSTOM_FUNCTION_DESCRIPTORS true
>>>  #endif /* GCC_C_OBJC_COMMON */
>>
>> I wonder if we even need the lang hook anymore.  ISTM that a front-end
>> that wants to use the function descriptors can just set
>> FUNC_ADDR_BY_DESCRIPTOR and we'd use the function descriptor, else we'll
>> use the trampoline.  Thoughts?
> 
> The lang hook also affects the minimum alignment for function
> pointers via the FUNCTION_ALIGNMENT macro (gcc/default.h). This does
> not appear to change the default alignment on any architecture, but
> it causes a failure in i386/gcc.target/i386/attr-aligned.c when
> requesting a smaller alignment which is then silently ignored.
Ugh.  I didn't see that.

> 
> I am not sure what the best approach is, but my preference
> would be to remove the lang hook and the FUNCTION_ALIGNMENT
> logic which will also fix the test case (the requested
> alignment will be applied).
> 
> I would then instead add a warning (or error?) which triggers
> only with -fno-trampolines if the user requests an alignment
> which is too small for this mechanism to work.
> 
> Does this sound reasonable?
So I'm thinking we should wrap the existing patch as-is for the trunk
(we're well into stage3 after all).  So leave the hook as-is for gcc-9.

We can then tackle removal of the hook, including twiddling
FUNCTION_ALIGNMENT for gcc-10.

Does that sound reasonable to you?

jeff

Re: [PATCH v4][C][ADA] use function descriptors instead of trampolines in C

[Uecker, Martin]

Am Donnerstag, den 13.12.2018, 16:35 -0700 schrieb Jeff Law:
> On 12/12/18 11:12 AM, Uecker, Martin wrote:

...
> > > > diff --git a/gcc/c/c-objc-common.h b/gcc/c/c-objc-common.h
> > > > index 78e768c2366..ef039560eb9 100644
> > > > --- a/gcc/c/c-objc-common.h
> > > > +++ b/gcc/c/c-objc-common.h
> > > > @@ -110,4 +110,7 @@ along with GCC; see the file COPYING3.  If
> > > > not see
> > > >  
> > > >  #undef LANG_HOOKS_TREE_INLINING_VAR_MOD_TYPE_P
> > > >  #define LANG_HOOKS_TREE_INLINING_VAR_MOD_TYPE_P c_vla_unspec_p
> > > > +
> > > > +#undef LANG_HOOKS_CUSTOM_FUNCTION_DESCRIPTORS
> > > > +#define LANG_HOOKS_CUSTOM_FUNCTION_DESCRIPTORS true
> > > >  #endif /* GCC_C_OBJC_COMMON */
> > > 
> > > I wonder if we even need the lang hook anymore.  ISTM that a
> > > front-end
> > > that wants to use the function descriptors can just set
> > > FUNC_ADDR_BY_DESCRIPTOR and we'd use the function descriptor,
> > > else we'll
> > > use the trampoline.  Thoughts?
> > 
> > The lang hook also affects the minimum alignment for function
> > pointers via the FUNCTION_ALIGNMENT macro (gcc/default.h). This
> > does
> > not appear to change the default alignment on any architecture, but
> > it causes a failure in i386/gcc.target/i386/attr-aligned.c when
> > requesting a smaller alignment which is then silently ignored.
> 
> Ugh.  I didn't see that.

The test is new (2019-11-29 Martin Sebor), but one could
argue that we could simply remove this specific test as 'aligned'
is only required to increase alignment. Martin?

> > I am not sure what the best approach is, but my preference
> > would be to remove the lang hook and the FUNCTION_ALIGNMENT
> > logic which will also fix the test case (the requested
> > alignment will be applied).
> > 
> > I would then instead add a warning (or error?) which triggers
> > only with -fno-trampolines if the user requests an alignment
> > which is too small for this mechanism to work.
>
> > Does this sound reasonable?
> 
> So I'm thinking we should wrap the existing patch as-is for the trunk
> (we're well into stage3 after all).  So leave the hook as-is for gcc-
> 9.
> 
> We can then tackle removal of the hook, including twiddling
> FUNCTION_ALIGNMENT for gcc-10.
> 
> Does that sound reasonable to you?

This is fine with me. So just confirm: I should install the 
patch despite the regression?

Best,
Martin

Re: [PATCH v4][C][ADA] use function descriptors instead of trampolines in C

[Jeff Law]

On 12/14/18 3:05 AM, Uecker, Martin wrote:
> 
> Am Donnerstag, den 13.12.2018, 16:35 -0700 schrieb Jeff Law:
>> On 12/12/18 11:12 AM, Uecker, Martin wrote:
> ...
>>>>> diff --git a/gcc/c/c-objc-common.h b/gcc/c/c-objc-common.h
>>>>> index 78e768c2366..ef039560eb9 100644
>>>>> --- a/gcc/c/c-objc-common.h
>>>>> +++ b/gcc/c/c-objc-common.h
>>>>> @@ -110,4 +110,7 @@ along with GCC; see the file COPYING3.  If
>>>>> not see
>>>>>  
>>>>>  #undef LANG_HOOKS_TREE_INLINING_VAR_MOD_TYPE_P
>>>>>  #define LANG_HOOKS_TREE_INLINING_VAR_MOD_TYPE_P c_vla_unspec_p
>>>>> +
>>>>> +#undef LANG_HOOKS_CUSTOM_FUNCTION_DESCRIPTORS
>>>>> +#define LANG_HOOKS_CUSTOM_FUNCTION_DESCRIPTORS true
>>>>>  #endif /* GCC_C_OBJC_COMMON */
>>>> I wonder if we even need the lang hook anymore.  ISTM that a
>>>> front-end
>>>> that wants to use the function descriptors can just set
>>>> FUNC_ADDR_BY_DESCRIPTOR and we'd use the function descriptor,
>>>> else we'll
>>>> use the trampoline.  Thoughts?
>>> The lang hook also affects the minimum alignment for function
>>> pointers via the FUNCTION_ALIGNMENT macro (gcc/default.h). This
>>> does
>>> not appear to change the default alignment on any architecture, but
>>> it causes a failure in i386/gcc.target/i386/attr-aligned.c when
>>> requesting a smaller alignment which is then silently ignored.
>> Ugh.  I didn't see that.
> The test is new (2019-11-29 Martin Sebor), but one could
> argue that we could simply remove this specific test as 'aligned'
> is only required to increase alignment. Martin?
The test is meant to test that we do the right thing consistently.  If
we're failing with your patch, then that needs to be addressed.

I read your note as the test would fail if you dropped the
CUSTOM_FUNCTION_DESCRIPTORS macro, not that it was failing with your
patch as-is.



> 
>>> I am not sure what the best approach is, but my preference
>>> would be to remove the lang hook and the FUNCTION_ALIGNMENT
>>> logic which will also fix the test case (the requested
>>> alignment will be applied).
>>>
>>> I would then instead add a warning (or error?) which triggers
>>> only with -fno-trampolines if the user requests an alignment
>>> which is too small for this mechanism to work.
>>> Does this sound reasonable?
>> So I'm thinking we should wrap the existing patch as-is for the trunk
>> (we're well into stage3 after all).  So leave the hook as-is for gcc-
>> 9.
>>
>> We can then tackle removal of the hook, including twiddling
>> FUNCTION_ALIGNMENT for gcc-10.
>>
>> Does that sound reasonable to you?
> This is fine with me. So just confirm: I should install the 
> patch despite the regression?
We need to address the regression.  Simply removing the test is probably
not the way to go.

jeff

Re: [PATCH v4][C][ADA] use function descriptors instead of trampolines in C

[Martin Sebor]

On 12/14/18 4:36 PM, Jeff Law wrote:
> On 12/14/18 3:05 AM, Uecker, Martin wrote:
>>
>> Am Donnerstag, den 13.12.2018, 16:35 -0700 schrieb Jeff Law:
>>> On 12/12/18 11:12 AM, Uecker, Martin wrote:
>> ...
>>>>>> diff --git a/gcc/c/c-objc-common.h b/gcc/c/c-objc-common.h
>>>>>> index 78e768c2366..ef039560eb9 100644
>>>>>> --- a/gcc/c/c-objc-common.h
>>>>>> +++ b/gcc/c/c-objc-common.h
>>>>>> @@ -110,4 +110,7 @@ along with GCC; see the file COPYING3.  If
>>>>>> not see
>>>>>>   
>>>>>>   #undef LANG_HOOKS_TREE_INLINING_VAR_MOD_TYPE_P
>>>>>>   #define LANG_HOOKS_TREE_INLINING_VAR_MOD_TYPE_P c_vla_unspec_p
>>>>>> +
>>>>>> +#undef LANG_HOOKS_CUSTOM_FUNCTION_DESCRIPTORS
>>>>>> +#define LANG_HOOKS_CUSTOM_FUNCTION_DESCRIPTORS true
>>>>>>   #endif /* GCC_C_OBJC_COMMON */
>>>>> I wonder if we even need the lang hook anymore.  ISTM that a
>>>>> front-end
>>>>> that wants to use the function descriptors can just set
>>>>> FUNC_ADDR_BY_DESCRIPTOR and we'd use the function descriptor,
>>>>> else we'll
>>>>> use the trampoline.  Thoughts?
>>>> The lang hook also affects the minimum alignment for function
>>>> pointers via the FUNCTION_ALIGNMENT macro (gcc/default.h). This
>>>> does
>>>> not appear to change the default alignment on any architecture, but
>>>> it causes a failure in i386/gcc.target/i386/attr-aligned.c when
>>>> requesting a smaller alignment which is then silently ignored.
>>> Ugh.  I didn't see that.
>> The test is new (2019-11-29 Martin Sebor), but one could
>> argue that we could simply remove this specific test as 'aligned'
>> is only required to increase alignment. Martin?
> The test is meant to test that we do the right thing consistently.  If
> we're failing with your patch, then that needs to be addressed.

I haven't been paying attention here and so I don't know how the test
fails after the change.  It's meant to verify that attribute aligned
successfully reduces the alignment of functions that have not been
previously declared with one all the way down to the supported minimum
(which is 1 on i386).  I agree with Jeff that removing the test would
not be right unless the failure is due to some bad assumptions on my
part.  If it's the built-in that fails that could be due to a bug in
it (it's very new).

Martin

> 
> I read your note as the test would fail if you dropped the
> CUSTOM_FUNCTION_DESCRIPTORS macro, not that it was failing with your
> patch as-is.
> 
> 
> 
>>
>>>> I am not sure what the best approach is, but my preference
>>>> would be to remove the lang hook and the FUNCTION_ALIGNMENT
>>>> logic which will also fix the test case (the requested
>>>> alignment will be applied).
>>>>
>>>> I would then instead add a warning (or error?) which triggers
>>>> only with -fno-trampolines if the user requests an alignment
>>>> which is too small for this mechanism to work.
>>>> Does this sound reasonable?
>>> So I'm thinking we should wrap the existing patch as-is for the trunk
>>> (we're well into stage3 after all).  So leave the hook as-is for gcc-
>>> 9.
>>>
>>> We can then tackle removal of the hook, including twiddling
>>> FUNCTION_ALIGNMENT for gcc-10.
>>>
>>> Does that sound reasonable to you?
>> This is fine with me. So just confirm: I should install the
>> patch despite the regression?
> We need to address the regression.  Simply removing the test is probably
> not the way to go.
> 
> jeff
> 

Re: [PATCH v4][C][ADA] use function descriptors instead of trampolines in C

[Uecker, Martin]

Am Freitag, den 14.12.2018, 18:20 -0700 schrieb Martin Sebor:
> On 12/14/18 4:36 PM, Jeff Law wrote:
> > On 12/14/18 3:05 AM, Uecker, Martin wrote:
> > > 
> > > Am Donnerstag, den 13.12.2018, 16:35 -0700 schrieb Jeff Law:
> > > > On 12/12/18 11:12 AM, Uecker, Martin wrote:
> > > 
> > > ...
> > > > > > > diff --git a/gcc/c/c-objc-common.h b/gcc/c/c-objc-common.h
> > > > > > > index 78e768c2366..ef039560eb9 100644
> > > > > > > --- a/gcc/c/c-objc-common.h
> > > > > > > +++ b/gcc/c/c-objc-common.h
> > > > > > > @@ -110,4 +110,7 @@ along with GCC; see the file COPYING3.  If
> > > > > > > not see
> > > > > > >   
> > > > > > >   #undef LANG_HOOKS_TREE_INLINING_VAR_MOD_TYPE_P
> > > > > > >   #define LANG_HOOKS_TREE_INLINING_VAR_MOD_TYPE_P c_vla_unspec_p
> > > > > > > +
> > > > > > > +#undef LANG_HOOKS_CUSTOM_FUNCTION_DESCRIPTORS
> > > > > > > +#define LANG_HOOKS_CUSTOM_FUNCTION_DESCRIPTORS true
> > > > > > >   #endif /* GCC_C_OBJC_COMMON */
> > > > > > 
> > > > > > I wonder if we even need the lang hook anymore.  ISTM that a
> > > > > > front-end
> > > > > > that wants to use the function descriptors can just set
> > > > > > FUNC_ADDR_BY_DESCRIPTOR and we'd use the function descriptor,
> > > > > > else we'll
> > > > > > use the trampoline.  Thoughts?
> > > > > 
> > > > > The lang hook also affects the minimum alignment for function
> > > > > pointers via the FUNCTION_ALIGNMENT macro (gcc/default.h). This
> > > > > does
> > > > > not appear to change the default alignment on any architecture, but
> > > > > it causes a failure in i386/gcc.target/i386/attr-aligned.c when
> > > > > requesting a smaller alignment which is then silently ignored.
> > > > 
> > > > Ugh.  I didn't see that.
> > > 
> > > The test is new (2019-11-29 Martin Sebor), but one could
> > > argue that we could simply remove this specific test as 'aligned'
> > > is only required to increase alignment. Martin?
> > 
> > The test is meant to test that we do the right thing consistently.  If
> > we're failing with your patch, then that needs to be addressed.
> 
> I haven't been paying attention here and so I don't know how the test
> fails after the change.  It's meant to verify that attribute aligned
> successfully reduces the alignment of functions that have not been
> previously declared with one all the way down to the supported minimum
> (which is 1 on i386).  I agree with Jeff that removing the test would
> not be right unless the failure is due to some bad assumptions on my
> part.  If it's the built-in that fails that could be due to a bug in
> it (it's very new).

There is a choice to be made: 

Either we activate the lang hook for C, then the minimum alignment for
functions on is not 1 anymore, because we need one bit to identify the 
descriptors.  From a correcntess point of view, this is OK as 'alignas'
is only required to increase alignment. It is also not really regression
in my opinion, as it is nowhere documented that one can reduce alignment
to '1'. The test also has just been added a couple of days ago (if I am
not mistaken). For these reasons, I think it would be OK to remove the test.

The other option is to decide that having an alignment of only '1'
is a valuable feature and should be preserved on those platforms which
support it. I have no idea what this could be good for, but maybe
there are use cases.  In this case, it makes of course sense to keep
the test.  We should then remove the lang hook (as it could never be
activated for most languages) and instead live with the fact that
'-fno-trampoline' and using alignof(1) on functions are simply
incompatible. A warning could be added if the compiler sees
alignof(1) when -fno-trampoline is active.


I am happy with both choices.


Best,
Martin

Re: [PATCH v4][C][ADA] use function descriptors instead of trampolines in C

[Jeff Law]

On 12/16/18 6:45 AM, Uecker, Martin wrote:
> Am Freitag, den 14.12.2018, 18:20 -0700 schrieb Martin Sebor:
>> On 12/14/18 4:36 PM, Jeff Law wrote:
>>> On 12/14/18 3:05 AM, Uecker, Martin wrote:
>>>>
>>>> Am Donnerstag, den 13.12.2018, 16:35 -0700 schrieb Jeff Law:
>>>>> On 12/12/18 11:12 AM, Uecker, Martin wrote:
>>>>
>>>> ...
>>>>>>>> diff --git a/gcc/c/c-objc-common.h b/gcc/c/c-objc-common.h
>>>>>>>> index 78e768c2366..ef039560eb9 100644
>>>>>>>> --- a/gcc/c/c-objc-common.h
>>>>>>>> +++ b/gcc/c/c-objc-common.h
>>>>>>>> @@ -110,4 +110,7 @@ along with GCC; see the file COPYING3.  If
>>>>>>>> not see
>>>>>>>>   
>>>>>>>>   #undef LANG_HOOKS_TREE_INLINING_VAR_MOD_TYPE_P
>>>>>>>>   #define LANG_HOOKS_TREE_INLINING_VAR_MOD_TYPE_P c_vla_unspec_p
>>>>>>>> +
>>>>>>>> +#undef LANG_HOOKS_CUSTOM_FUNCTION_DESCRIPTORS
>>>>>>>> +#define LANG_HOOKS_CUSTOM_FUNCTION_DESCRIPTORS true
>>>>>>>>   #endif /* GCC_C_OBJC_COMMON */
>>>>>>>
>>>>>>> I wonder if we even need the lang hook anymore.  ISTM that a
>>>>>>> front-end
>>>>>>> that wants to use the function descriptors can just set
>>>>>>> FUNC_ADDR_BY_DESCRIPTOR and we'd use the function descriptor,
>>>>>>> else we'll
>>>>>>> use the trampoline.  Thoughts?
>>>>>>
>>>>>> The lang hook also affects the minimum alignment for function
>>>>>> pointers via the FUNCTION_ALIGNMENT macro (gcc/default.h). This
>>>>>> does
>>>>>> not appear to change the default alignment on any architecture, but
>>>>>> it causes a failure in i386/gcc.target/i386/attr-aligned.c when
>>>>>> requesting a smaller alignment which is then silently ignored.
>>>>>
>>>>> Ugh.  I didn't see that.
>>>>
>>>> The test is new (2019-11-29 Martin Sebor), but one could
>>>> argue that we could simply remove this specific test as 'aligned'
>>>> is only required to increase alignment. Martin?
>>>
>>> The test is meant to test that we do the right thing consistently.  If
>>> we're failing with your patch, then that needs to be addressed.
>>
>> I haven't been paying attention here and so I don't know how the test
>> fails after the change.  It's meant to verify that attribute aligned
>> successfully reduces the alignment of functions that have not been
>> previously declared with one all the way down to the supported minimum
>> (which is 1 on i386).  I agree with Jeff that removing the test would
>> not be right unless the failure is due to some bad assumptions on my
>> part.  If it's the built-in that fails that could be due to a bug in
>> it (it's very new).
> 
> There is a choice to be made: 
> 
> Either we activate the lang hook for C, then the minimum alignment for
> functions on is not 1 anymore, because we need one bit to identify the 
> descriptors.  From a correcntess point of view, this is OK as 'alignas'
> is only required to increase alignment. It is also not really regression
> in my opinion, as it is nowhere documented that one can reduce alignment
> to '1'. The test also has just been added a couple of days ago (if I am
> not mistaken). For these reasons, I think it would be OK to remove the test.
> 
> The other option is to decide that having an alignment of only '1'
> is a valuable feature and should be preserved on those platforms which
> support it. I have no idea what this could be good for, but maybe
> there are use cases.  In this case, it makes of course sense to keep
> the test.  We should then remove the lang hook (as it could never be
> activated for most languages) and instead live with the fact that
> '-fno-trampoline' and using alignof(1) on functions are simply
> incompatible. A warning could be added if the compiler sees
> alignof(1) when -fno-trampoline is active.
There's certainly targets where 1 byte function alignment is important
from a code space perspective -- it's likely that function descriptors
will never be supported on those targets.

It's also important to remember that not every target which uses
function descriptors uses the LSB.  On some targets the LSB may switch
between modes (arm vs thumb for example).  So on those targets the use
of descriptors may imply an even larger minimum alignment.

Ultimately using function descriptors is an ABI breaking choice and we
might declare that function descriptors imply higher function
alignments.  The ABI implications also likely mean that function
descriptors aren't likely going to achieve widespread use.  Sigh.

jeff

Re: [PATCH v4][C][ADA] use function descriptors instead of trampolines in C

[Uecker, Martin]

Am Sonntag, den 16.12.2018, 09:13 -0700 schrieb Jeff Law:
> On 12/16/18 6:45 AM, Uecker, Martin wrote:
> > Am Freitag, den 14.12.2018, 18:20 -0700 schrieb Martin Sebor:
> > > On 12/14/18 4:36 PM, Jeff Law wrote:
> > > > On 12/14/18 3:05 AM, Uecker, Martin wrote:
> > > > > 
> > > > > Am Donnerstag, den 13.12.2018, 16:35 -0700 schrieb Jeff Law:
> > > > > > On 12/12/18 11:12 AM, Uecker, Martin wrote:
> > > > > 
> > > > > ...
> > > > > > > > > diff --git a/gcc/c/c-objc-common.h b/gcc/c/c-objc-common.h
> > > > > > > > > index 78e768c2366..ef039560eb9 100644
> > > > > > > > > --- a/gcc/c/c-objc-common.h
> > > > > > > > > +++ b/gcc/c/c-objc-common.h
> > > > > > > > > @@ -110,4 +110,7 @@ along with GCC; see the file COPYING3.  If
> > > > > > > > > not see
> > > > > > > > >   
> > > > > > > > >   #undef LANG_HOOKS_TREE_INLINING_VAR_MOD_TYPE_P
> > > > > > > > >   #define LANG_HOOKS_TREE_INLINING_VAR_MOD_TYPE_P c_vla_unspec_p
> > > > > > > > > +
> > > > > > > > > +#undef LANG_HOOKS_CUSTOM_FUNCTION_DESCRIPTORS
> > > > > > > > > +#define LANG_HOOKS_CUSTOM_FUNCTION_DESCRIPTORS true
> > > > > > > > >   #endif /* GCC_C_OBJC_COMMON */
> > > > > > > > 
> > > > > > > > I wonder if we even need the lang hook anymore.  ISTM that a
> > > > > > > > front-end
> > > > > > > > that wants to use the function descriptors can just set
> > > > > > > > FUNC_ADDR_BY_DESCRIPTOR and we'd use the function descriptor,
> > > > > > > > else we'll
> > > > > > > > use the trampoline.  Thoughts?
> > > > > > > 
> > > > > > > The lang hook also affects the minimum alignment for function
> > > > > > > pointers via the FUNCTION_ALIGNMENT macro (gcc/default.h). This
> > > > > > > does
> > > > > > > not appear to change the default alignment on any architecture, but
> > > > > > > it causes a failure in i386/gcc.target/i386/attr-aligned.c when
> > > > > > > requesting a smaller alignment which is then silently ignored.
> > > > > > 
> > > > > > Ugh.  I didn't see that.
> > > > > 
> > > > > The test is new (2019-11-29 Martin Sebor), but one could
> > > > > argue that we could simply remove this specific test as 'aligned'
> > > > > is only required to increase alignment. Martin?
> > > > 
> > > > The test is meant to test that we do the right thing consistently.  If
> > > > we're failing with your patch, then that needs to be addressed.
> > > 
> > > I haven't been paying attention here and so I don't know how the test
> > > fails after the change.  It's meant to verify that attribute aligned
> > > successfully reduces the alignment of functions that have not been
> > > previously declared with one all the way down to the supported minimum
> > > (which is 1 on i386).  I agree with Jeff that removing the test would
> > > not be right unless the failure is due to some bad assumptions on my
> > > part.  If it's the built-in that fails that could be due to a bug in
> > > it (it's very new).
> > 
> > There is a choice to be made: 
> > 
> > Either we activate the lang hook for C, then the minimum alignment for
> > functions on is not 1 anymore, because we need one bit to identify the 
> > descriptors.  From a correcntess point of view, this is OK as 'alignas'
> > is only required to increase alignment. It is also not really regression
> > in my opinion, as it is nowhere documented that one can reduce alignment
> > to '1'. The test also has just been added a couple of days ago (if I am
> > not mistaken). For these reasons, I think it would be OK to remove the test.
> > 
> > The other option is to decide that having an alignment of only '1'
> > is a valuable feature and should be preserved on those platforms which
> > support it. I have no idea what this could be good for, but maybe
> > there are use cases.  In this case, it makes of course sense to keep
> > the test.  We should then remove the lang hook (as it could never be
> > activated for most languages) and instead live with the fact that
> > '-fno-trampoline' and using alignof(1) on functions are simply
> > incompatible. A warning could be added if the compiler sees
> > alignof(1) when -fno-trampoline is active.
> 
> There's certainly targets where 1 byte function alignment is important
> from a code space perspective -- it's likely that function descriptors
> will never be supported on those targets.

But most architectures require a higher alignment anyway.
Here is a list of all targets where function alignment
is 1 byte:

gcc/config/avr/avr.h:#define FUNCTION_BOUNDARY 8
gcc/config/i386/i386.h:#define FUNCTION_BOUNDARY 8
gcc/config/m32c/m32c.h:#define FUNCTION_BOUNDARY 8
gcc/config/mn10300/mn10300.h:#define FUNCTION_BOUNDARY 8
gcc/config/pa/pa.h:#define FUNCTION_BOUNDARY BITS_PER_WORD
gcc/config/rl78/rl78.h:#define FUNCTION_BOUNDARY 		8
gcc/config/rx/rx.h:#define FUNCTION_BOUNDARY 		((rx_cpu_type == RX100 || rx_cpu_type == RX200) ? 4 : 8)


From this list only i386 currently adds the target hook and
would be affected by this patch. But arm is also affected:
 
> It's also important to remember that not every target which uses
> function descriptors uses the LSB.  On some targets the LSB may switch
> between modes (arm vs thumb for example).  So on those targets the use
> of descriptors may imply an even larger minimum alignment.

Yes, arm reserves the lowest 2 bits so since we require an additional
bit and this increases the minimum alignment from 4 to 8 bytes on 
aarch64.

Here are the architectures which currently support function
descriptors and the bit used:

$ grep TARGET_CUSTOM gcc/config/*/*.c | grep define
gcc/config/aarch64/aarch64.c:#define TARGET_CUSTOM_FUNCTION_DESCRIPTORS 4
gcc/config/arm/arm.c:#define TARGET_CUSTOM_FUNCTION_DESCRIPTORS 2
gcc/config/csky/csky.c:#define TARGET_CUSTOM_FUNCTION_DESCRIPTORS 1
gcc/config/i386/i386.c:#define TARGET_CUSTOM_FUNCTION_DESCRIPTORS 1
gcc/config/ia64/ia64.c:#define TARGET_CUSTOM_FUNCTION_DESCRIPTORS 0
gcc/config/mips/mips.c:#define TARGET_CUSTOM_FUNCTION_DESCRIPTORS 2
gcc/config/or1k/or1k.c:#define TARGET_CUSTOM_FUNCTION_DESCRIPTORS 1
gcc/config/powerpcspe/powerpcspe.c:#define TARGET_CUSTOM_FUNCTION_DESCRIPTORS 1
gcc/config/riscv/riscv.c:#define TARGET_CUSTOM_FUNCTION_DESCRIPTORS 1
gcc/config/rs6000/rs6000.c:#define TARGET_CUSTOM_FUNCTION_DESCRIPTORS 1
gcc/config/s390/s390.c:#define TARGET_CUSTOM_FUNCTION_DESCRIPTORS 1
gcc/config/sparc/sparc.c:#define TARGET_CUSTOM_FUNCTION_DESCRIPTORS 1

For mips minimum alignment is 4 bytes, so here alignment would
not change.

> Ultimately using function descriptors is an ABI breaking choice and we
> might declare that function descriptors imply higher function
> alignments.  

Increasing the alignment is not an ABI breaking change.

But the alignment increase itself on 'i386' and 'aarch64'
might be unacceptable. In this case, the only safe change
is to make the higher alignment also depend on
"-fno-trampolines".  Would this be acceptable?

 
(Code compiled with "-fno-trampolines" is of course
ABI-incompatible to existing code with respect to exported
descriptors and imported function pointers which happen
to be misaligned. Generally increasing alignment would
be nice as it would reduce this incompatibility in one
direction.)

> The ABI implications also likely mean that function
> descriptors aren't likely going to achieve widespread use.
> Sigh.

There might be other options for architectures
which are problematic: We could use unused higher
order bits or one could read the first bytes of the
function/descriptor and use a special tag for
descriptors which cannot occur in machine code.

Also on the language level, one could mark pointers which
point to designators and keep them separate. This is
what Apple's "blocks" language extension does.


Best,
Martin

Re: [PATCH v4][C][ADA] use function descriptors instead of trampolines in C

[Szabolcs Nagy]

On 16/12/2018 22:45, Uecker, Martin wrote:
> Am Sonntag, den 16.12.2018, 09:13 -0700 schrieb Jeff Law:
>> Ultimately using function descriptors is an ABI breaking choice and we
>> might declare that function descriptors imply higher function
>> alignments.  
> 
> Increasing the alignment is not an ABI breaking change.

increasing alignment _requirement_ is an abi breaking change.

and it's not clear who would benefit from the new abi:

- it affects everything that does indirect calls (if alignment
requirement is increased then in addition everything that has
functions whose address may be taken), so it can easily affect
existing handwritten asm and it definitely requires the rebuild
of the c runtime to support this abi (i think it even requires
asm changes there if you allow a thread or makecontext start
function to be a nested function).

- it makes indirect calls more expensive everywhere, even if
nested functions are not used.

i think to fix the executable stack problem in practice, the
new nested function mechanism should only require the rebuild
of code that actually contains nested functions and thus have
no abi or performance impact on code that never uses them.

i believe this can be achieved by some restrictions on nested
function usage in a way that covers most practical use-cases:
e.g. only allowing one active parent function call frame per
thread, no recursive calls to it, the nested function must be
invoked in the same thread as the parent using the same stack,
etc. (then the new mechanism can be used safely if nested
functions are known to follow the restrictions, the compiler
may even emit code to check the constraints at runtime.)

Re: [PATCH v4][C][ADA] use function descriptors instead of trampolines in C

[Jeff Law]

On 12/16/18 3:45 PM, Uecker, Martin wrote:
> Am Sonntag, den 16.12.2018, 09:13 -0700 schrieb Jeff Law:
>> On 12/16/18 6:45 AM, Uecker, Martin wrote:
>>> Am Freitag, den 14.12.2018, 18:20 -0700 schrieb Martin Sebor:
>>>> On 12/14/18 4:36 PM, Jeff Law wrote:
>>>>> On 12/14/18 3:05 AM, Uecker, Martin wrote:
>>>>>>
>>>>>> Am Donnerstag, den 13.12.2018, 16:35 -0700 schrieb Jeff Law:
>>>>>>> On 12/12/18 11:12 AM, Uecker, Martin wrote:
>>>>>>
>>>>>> ...
>>>>>>>>>> diff --git a/gcc/c/c-objc-common.h b/gcc/c/c-objc-common.h
>>>>>>>>>> index 78e768c2366..ef039560eb9 100644
>>>>>>>>>> --- a/gcc/c/c-objc-common.h
>>>>>>>>>> +++ b/gcc/c/c-objc-common.h
>>>>>>>>>> @@ -110,4 +110,7 @@ along with GCC; see the file COPYING3.  If
>>>>>>>>>> not see
>>>>>>>>>>   
>>>>>>>>>>   #undef LANG_HOOKS_TREE_INLINING_VAR_MOD_TYPE_P
>>>>>>>>>>   #define LANG_HOOKS_TREE_INLINING_VAR_MOD_TYPE_P c_vla_unspec_p
>>>>>>>>>> +
>>>>>>>>>> +#undef LANG_HOOKS_CUSTOM_FUNCTION_DESCRIPTORS
>>>>>>>>>> +#define LANG_HOOKS_CUSTOM_FUNCTION_DESCRIPTORS true
>>>>>>>>>>   #endif /* GCC_C_OBJC_COMMON */
>>>>>>>>>
>>>>>>>>> I wonder if we even need the lang hook anymore.  ISTM that a
>>>>>>>>> front-end
>>>>>>>>> that wants to use the function descriptors can just set
>>>>>>>>> FUNC_ADDR_BY_DESCRIPTOR and we'd use the function descriptor,
>>>>>>>>> else we'll
>>>>>>>>> use the trampoline.  Thoughts?
>>>>>>>>
>>>>>>>> The lang hook also affects the minimum alignment for function
>>>>>>>> pointers via the FUNCTION_ALIGNMENT macro (gcc/default.h). This
>>>>>>>> does
>>>>>>>> not appear to change the default alignment on any architecture, but
>>>>>>>> it causes a failure in i386/gcc.target/i386/attr-aligned.c when
>>>>>>>> requesting a smaller alignment which is then silently ignored.
>>>>>>>
>>>>>>> Ugh.  I didn't see that.
>>>>>>
>>>>>> The test is new (2019-11-29 Martin Sebor), but one could
>>>>>> argue that we could simply remove this specific test as 'aligned'
>>>>>> is only required to increase alignment. Martin?
>>>>>
>>>>> The test is meant to test that we do the right thing consistently.  If
>>>>> we're failing with your patch, then that needs to be addressed.
>>>>
>>>> I haven't been paying attention here and so I don't know how the test
>>>> fails after the change.  It's meant to verify that attribute aligned
>>>> successfully reduces the alignment of functions that have not been
>>>> previously declared with one all the way down to the supported minimum
>>>> (which is 1 on i386).  I agree with Jeff that removing the test would
>>>> not be right unless the failure is due to some bad assumptions on my
>>>> part.  If it's the built-in that fails that could be due to a bug in
>>>> it (it's very new).
>>>
>>> There is a choice to be made: 
>>>
>>> Either we activate the lang hook for C, then the minimum alignment for
>>> functions on is not 1 anymore, because we need one bit to identify the 
>>> descriptors.  From a correcntess point of view, this is OK as 'alignas'
>>> is only required to increase alignment. It is also not really regression
>>> in my opinion, as it is nowhere documented that one can reduce alignment
>>> to '1'. The test also has just been added a couple of days ago (if I am
>>> not mistaken). For these reasons, I think it would be OK to remove the test.
>>>
>>> The other option is to decide that having an alignment of only '1'
>>> is a valuable feature and should be preserved on those platforms which
>>> support it. I have no idea what this could be good for, but maybe
>>> there are use cases.  In this case, it makes of course sense to keep
>>> the test.  We should then remove the lang hook (as it could never be
>>> activated for most languages) and instead live with the fact that
>>> '-fno-trampoline' and using alignof(1) on functions are simply
>>> incompatible. A warning could be added if the compiler sees
>>> alignof(1) when -fno-trampoline is active.
>>
>> There's certainly targets where 1 byte function alignment is important
>> from a code space perspective -- it's likely that function descriptors
>> will never be supported on those targets.
> 
> But most architectures require a higher alignment anyway.
> Here is a list of all targets where function alignment
> is 1 byte:
> 
> gcc/config/avr/avr.h:#define FUNCTION_BOUNDARY 8
> gcc/config/i386/i386.h:#define FUNCTION_BOUNDARY 8
> gcc/config/m32c/m32c.h:#define FUNCTION_BOUNDARY 8
> gcc/config/mn10300/mn10300.h:#define FUNCTION_BOUNDARY 8
> gcc/config/pa/pa.h:#define FUNCTION_BOUNDARY BITS_PER_WORD
> gcc/config/rl78/rl78.h:#define FUNCTION_BOUNDARY 		8
> gcc/config/rx/rx.h:#define FUNCTION_BOUNDARY 		((rx_cpu_type == RX100 || rx_cpu_type == RX200) ? 4 : 8)


> 
> 
> From this list only i386 currently adds the target hook and
> would be affected by this patch. But arm is also affected:
>  
>> It's also important to remember that not every target which uses
>> function descriptors uses the LSB.  On some targets the LSB may switch
>> between modes (arm vs thumb for example).  So on those targets the use
>> of descriptors may imply an even larger minimum alignment.
> 
> Yes, arm reserves the lowest 2 bits so since we require an additional
> bit and this increases the minimum alignment from 4 to 8 bytes on 
> aarch64.
> 
> Here are the architectures which currently support function
> descriptors and the bit used:
> 
> $ grep TARGET_CUSTOM gcc/config/*/*.c | grep define
> gcc/config/aarch64/aarch64.c:#define TARGET_CUSTOM_FUNCTION_DESCRIPTORS 4
> gcc/config/arm/arm.c:#define TARGET_CUSTOM_FUNCTION_DESCRIPTORS 2
> gcc/config/csky/csky.c:#define TARGET_CUSTOM_FUNCTION_DESCRIPTORS 1
> gcc/config/i386/i386.c:#define TARGET_CUSTOM_FUNCTION_DESCRIPTORS 1
> gcc/config/ia64/ia64.c:#define TARGET_CUSTOM_FUNCTION_DESCRIPTORS 0
> gcc/config/mips/mips.c:#define TARGET_CUSTOM_FUNCTION_DESCRIPTORS 2
> gcc/config/or1k/or1k.c:#define TARGET_CUSTOM_FUNCTION_DESCRIPTORS 1
> gcc/config/powerpcspe/powerpcspe.c:#define TARGET_CUSTOM_FUNCTION_DESCRIPTORS 1
> gcc/config/riscv/riscv.c:#define TARGET_CUSTOM_FUNCTION_DESCRIPTORS 1
> gcc/config/rs6000/rs6000.c:#define TARGET_CUSTOM_FUNCTION_DESCRIPTORS 1
> gcc/config/s390/s390.c:#define TARGET_CUSTOM_FUNCTION_DESCRIPTORS 1
> gcc/config/sparc/sparc.c:#define TARGET_CUSTOM_FUNCTION_DESCRIPTORS 1
> 
> For mips minimum alignment is 4 bytes, so here alignment would
> not change.
> 
>> Ultimately using function descriptors is an ABI breaking choice and we
>> might declare that function descriptors imply higher function
>> alignments.  
> 
> Increasing the alignment is not an ABI breaking change.
Increasing alignment is also an ABI change if you make those low bits
have a new meaning.  Existing code won't know if the target address is
actually a function descriptor or just a minimally aligned real function.


It's not terribly important, but the HPPA bits above are wrong.  It's
minimum alignment is 4 bytes.  The low two bits are used for two
different style function descriptors that already exist on that
platform.  We may have lied about this to prevent combine from mucking
with anything that examined or modified the low bits in a function address.


> 
> But the alignment increase itself on 'i386' and 'aarch64'
> might be unacceptable. In this case, the only safe change
> is to make the higher alignment also depend on
> "-fno-trampolines".  Would this be acceptable?
Unclear at this point.


jeff

Re: [PATCH v4][C][ADA] use function descriptors instead of trampolines in C

[Martin Sebor]

On 12/16/18 6:45 AM, Uecker, Martin wrote:
> Am Freitag, den 14.12.2018, 18:20 -0700 schrieb Martin Sebor:
>> On 12/14/18 4:36 PM, Jeff Law wrote:
>>> On 12/14/18 3:05 AM, Uecker, Martin wrote:
>>>>
>>>> Am Donnerstag, den 13.12.2018, 16:35 -0700 schrieb Jeff Law:
>>>>> On 12/12/18 11:12 AM, Uecker, Martin wrote:
>>>>
>>>> ...
>>>>>>>> diff --git a/gcc/c/c-objc-common.h b/gcc/c/c-objc-common.h
>>>>>>>> index 78e768c2366..ef039560eb9 100644
>>>>>>>> --- a/gcc/c/c-objc-common.h
>>>>>>>> +++ b/gcc/c/c-objc-common.h
>>>>>>>> @@ -110,4 +110,7 @@ along with GCC; see the file COPYING3.  If
>>>>>>>> not see
>>>>>>>>    
>>>>>>>>    #undef LANG_HOOKS_TREE_INLINING_VAR_MOD_TYPE_P
>>>>>>>>    #define LANG_HOOKS_TREE_INLINING_VAR_MOD_TYPE_P c_vla_unspec_p
>>>>>>>> +
>>>>>>>> +#undef LANG_HOOKS_CUSTOM_FUNCTION_DESCRIPTORS
>>>>>>>> +#define LANG_HOOKS_CUSTOM_FUNCTION_DESCRIPTORS true
>>>>>>>>    #endif /* GCC_C_OBJC_COMMON */
>>>>>>>
>>>>>>> I wonder if we even need the lang hook anymore.  ISTM that a
>>>>>>> front-end
>>>>>>> that wants to use the function descriptors can just set
>>>>>>> FUNC_ADDR_BY_DESCRIPTOR and we'd use the function descriptor,
>>>>>>> else we'll
>>>>>>> use the trampoline.  Thoughts?
>>>>>>
>>>>>> The lang hook also affects the minimum alignment for function
>>>>>> pointers via the FUNCTION_ALIGNMENT macro (gcc/default.h). This
>>>>>> does
>>>>>> not appear to change the default alignment on any architecture, but
>>>>>> it causes a failure in i386/gcc.target/i386/attr-aligned.c when
>>>>>> requesting a smaller alignment which is then silently ignored.
>>>>>
>>>>> Ugh.  I didn't see that.
>>>>
>>>> The test is new (2019-11-29 Martin Sebor), but one could
>>>> argue that we could simply remove this specific test as 'aligned'
>>>> is only required to increase alignment. Martin?
>>>
>>> The test is meant to test that we do the right thing consistently.  If
>>> we're failing with your patch, then that needs to be addressed.
>>
>> I haven't been paying attention here and so I don't know how the test
>> fails after the change.  It's meant to verify that attribute aligned
>> successfully reduces the alignment of functions that have not been
>> previously declared with one all the way down to the supported minimum
>> (which is 1 on i386).  I agree with Jeff that removing the test would
>> not be right unless the failure is due to some bad assumptions on my
>> part.  If it's the built-in that fails that could be due to a bug in
>> it (it's very new).
> 
> There is a choice to be made:
> 
> Either we activate the lang hook for C, then the minimum alignment for
> functions on is not 1 anymore, because we need one bit to identify the
> descriptors.  From a correcntess point of view, this is OK as 'alignas'
> is only required to increase alignment.

alignas doesn't apply to functions.  Changing function alignment
is a feature unique to attribute aligned.  The attribute can be
used to decrease the alignment of functions that have not yet
been explicitly declared with one.  GCC has historically allowed
this, and based on my tests, the i386 target has always allowed
functions to be completely unaligned (either by using attribute
aligned (1) when supported or via -Os/-falign-functions=1).
The purpose of the test is to verify that this works.

> It is also not really regression
> in my opinion, as it is nowhere documented that one can reduce alignment
> to '1'. The test also has just been added a couple of days ago (if I am
> not mistaken). For these reasons, I think it would be OK to remove the test.

This wasn't clearly documented until recently.  The test was added
to verify that GCC behaves as intended and clarified in the manual.
The latest manual mentions that:

   The attribute cannot be used to decrease the alignment of
   a function previously declared with a more restrictive alignment;
   only to increase it.  Attempts to do otherwise are diagnosed.
   Some targets specify a minimum default alignment for functions
   that is greater than 1.  On such targets, specifying a less
   restrictive alignment is silently ignored.

The update to the text was discussed here:
   https://gcc.gnu.org/ml/gcc/2018-11/msg00127.html

If the i386 target should increase the minimum supported alignment
up from 1 under some conditions the test might need to be adjusted
(but it should not be removed).

Martin

> 
> The other option is to decide that having an alignment of only '1'
> is a valuable feature and should be preserved on those platforms which
> support it. I have no idea what this could be good for, but maybe
> there are use cases.  In this case, it makes of course sense to keep
> the test.  We should then remove the lang hook (as it could never be
> activated for most languages) and instead live with the fact that
> '-fno-trampoline' and using alignof(1) on functions are simply
> incompatible. A warning could be added if the compiler sees
> alignof(1) when -fno-trampoline is active.
> 
> 
> I am happy with both choices.
> 
> 
> Best,
> Martin
> 
> 

Re: [PATCH v4][C][ADA] use function descriptors instead of trampolines in C

[Uecker, Martin]

Am Montag, den 17.12.2018, 10:28 -0700 schrieb Jeff Law:
> On 12/16/18 3:45 PM, Uecker, Martin wrote:
> > Am Sonntag, den 16.12.2018, 09:13 -0700 schrieb Jeff Law:
> > > On 12/16/18 6:45 AM, Uecker, Martin wrote:
> > > > Am Freitag, den 14.12.2018, 18:20 -0700 schrieb Martin Sebor:
> > > > > On 12/14/18 4:36 PM, Jeff Law wrote:
> > > > > > On 12/14/18 3:05 AM, Uecker, Martin wrote:
> > > > > > > 
> > > > > > > Am Donnerstag, den 13.12.2018, 16:35 -0700 schrieb Jeff Law:
> > > > > > > > On 12/12/18 11:12 AM, Uecker, Martin wrote:
> > > > > > > 
...

> > > There's certainly targets where 1 byte function alignment is important
> > > from a code space perspective -- it's likely that function descriptors
> > > will never be supported on those targets.
> > 
> > But most architectures require a higher alignment anyway.
> > Here is a list of all targets where function alignment
> > is 1 byte:
> > 
> > gcc/config/avr/avr.h:#define FUNCTION_BOUNDARY 8
> > gcc/config/i386/i386.h:#define FUNCTION_BOUNDARY 8
> > gcc/config/m32c/m32c.h:#define FUNCTION_BOUNDARY 8
> > gcc/config/mn10300/mn10300.h:#define FUNCTION_BOUNDARY 8
> > gcc/config/pa/pa.h:#define FUNCTION_BOUNDARY BITS_PER_WORD
> > gcc/config/rl78/rl78.h:#define FUNCTION_BOUNDARY 		8
> > gcc/config/rx/rx.h:#define FUNCTION_BOUNDARY 		((rx_cpu_type == RX100 ||
> > rx_cpu_type == RX200) ? 4 : 8)

(BTW: pa was included here by mistake)

> > 
> > From this list only i386 currently adds the target hook and
> > would be affected by this patch. But arm is also affected:
> >  
> > > It's also important to remember that not every target which uses
> > > function descriptors uses the LSB.  On some targets the LSB may switch
> > > between modes (arm vs thumb for example).  So on those targets the use
> > > of descriptors may imply an even larger minimum alignment.
> > 
> > Yes, arm reserves the lowest 2 bits so since we require an additional
> > bit and this increases the minimum alignment from 4 to 8 bytes on 
> > aarch64.
> > 
> > Here are the architectures which currently support function
> > descriptors and the bit used:
> > 
> > $ grep TARGET_CUSTOM gcc/config/*/*.c | grep define
> > gcc/config/aarch64/aarch64.c:#define TARGET_CUSTOM_FUNCTION_DESCRIPTORS 4
> > gcc/config/arm/arm.c:#define TARGET_CUSTOM_FUNCTION_DESCRIPTORS 2
> > gcc/config/csky/csky.c:#define TARGET_CUSTOM_FUNCTION_DESCRIPTORS 1
> > gcc/config/i386/i386.c:#define TARGET_CUSTOM_FUNCTION_DESCRIPTORS 1
> > gcc/config/ia64/ia64.c:#define TARGET_CUSTOM_FUNCTION_DESCRIPTORS 0
> > gcc/config/mips/mips.c:#define TARGET_CUSTOM_FUNCTION_DESCRIPTORS 2
> > gcc/config/or1k/or1k.c:#define TARGET_CUSTOM_FUNCTION_DESCRIPTORS 1
> > gcc/config/powerpcspe/powerpcspe.c:#define TARGET_CUSTOM_FUNCTION_DESCRIPTORS 1
> > gcc/config/riscv/riscv.c:#define TARGET_CUSTOM_FUNCTION_DESCRIPTORS 1
> > gcc/config/rs6000/rs6000.c:#define TARGET_CUSTOM_FUNCTION_DESCRIPTORS 1
> > gcc/config/s390/s390.c:#define TARGET_CUSTOM_FUNCTION_DESCRIPTORS 1
> > gcc/config/sparc/sparc.c:#define TARGET_CUSTOM_FUNCTION_DESCRIPTORS 1
> > 
> > For mips minimum alignment is 4 bytes, so here alignment would
> > not change.
> > 
> > > Ultimately using function descriptors is an ABI breaking choice and we
> > > might declare that function descriptors imply higher function
> > > alignments.  
> > 
> > Increasing the alignment is not an ABI breaking change.
> 
> Increasing alignment is also an ABI change if you make those low bits
> have a new meaning.  

Absolutely, but this would only happen with the command-line
flag.

> Existing code won't know if the target address is
> actually a function descriptor or just a minimally aligned real function.

Let's think practically:

There are projects which use nested functions and typically these functions
and pointers to these functions are used only internally. The code interfaces
with existing code, which usually uses an alignment which is sufficient.
On most architectures this is the case, because the minimum alignment is
already big enough, and on others because the default alignment (atleast with
optimization) is big enough. If one is careful (actually checks that no
pointer to a nested function escapes and no pointer to functions with
smaller alignment are passed in) then using  '-fno-trampolines' is 
perfectly safe. This would then allow to compile these projects in a
way which does not require an executable stack. In my opinion this is
a very useful feature.

Admittedly, we should add the same warning we also have for other
ABI-changing options (e.g. for -freg-struct-return) to the description
and also only change the minimum alignment when the flag is given.
There would then be no impact without the flag and people who know
that it safe could still use it.
 

> It's not terribly important, but the HPPA bits above are wrong.  It's
> minimum alignment is 4 bytes.  The low two bits are used for two
> different style function descriptors that already exist on that
> platform.  We may have lied about this to prevent combine from mucking
> with anything that examined or modified the low bits in a function address.

It's defined to be BITS_PER_WORD so it be correct. I just shouldn't
have included in the list.

> > 
> > But the alignment increase itself on 'i386' and 'aarch64'
> > might be unacceptable. In this case, the only safe change
> > is to make the higher alignment also depend on
> > "-fno-trampolines".  Would this be acceptable?
> 
> Unclear at this point.

The feature is very useful to me, so if I can do something to get
this resolved, let me know.

Best,
Martin

Re: [PATCH v4][C][ADA] use function descriptors instead of trampolines in C

[Uecker, Martin]

Am Montag, den 17.12.2018, 10:31 -0700 schrieb Martin Sebor:
> On 12/16/18 6:45 AM, Uecker, Martin wrote:
> > Am Freitag, den 14.12.2018, 18:20 -0700 schrieb Martin Sebor:
> > > On 12/14/18 4:36 PM, Jeff Law wrote:
> > > > On 12/14/18 3:05 AM, Uecker, Martin wrote:
> > > > > 
> > > > > Am Donnerstag, den 13.12.2018, 16:35 -0700 schrieb Jeff Law:
> > > > > > On 12/12/18 11:12 AM, Uecker, Martin wrote:
> > > > > 
> > > > > ...
> > > > > > > > > diff --git a/gcc/c/c-objc-common.h b/gcc/c/c-objc-common.h
> > > > > > > > > index 78e768c2366..ef039560eb9 100644
> > > > > > > > > --- a/gcc/c/c-objc-common.h
> > > > > > > > > +++ b/gcc/c/c-objc-common.h
> > > > > > > > > @@ -110,4 +110,7 @@ along with GCC; see the file COPYING3.  If
> > > > > > > > > not see
> > > > > > > > >    
> > > > > > > > >    #undef LANG_HOOKS_TREE_INLINING_VAR_MOD_TYPE_P
> > > > > > > > >    #define LANG_HOOKS_TREE_INLINING_VAR_MOD_TYPE_P c_vla_unspec_p
> > > > > > > > > +
> > > > > > > > > +#undef LANG_HOOKS_CUSTOM_FUNCTION_DESCRIPTORS
> > > > > > > > > +#define LANG_HOOKS_CUSTOM_FUNCTION_DESCRIPTORS true
> > > > > > > > >    #endif /* GCC_C_OBJC_COMMON */
> > > > > > > > 
> > > > > > > > I wonder if we even need the lang hook anymore.  ISTM that a
> > > > > > > > front-end
> > > > > > > > that wants to use the function descriptors can just set
> > > > > > > > FUNC_ADDR_BY_DESCRIPTOR and we'd use the function descriptor,
> > > > > > > > else we'll
> > > > > > > > use the trampoline.  Thoughts?
> > > > > > > 
> > > > > > > The lang hook also affects the minimum alignment for function
> > > > > > > pointers via the FUNCTION_ALIGNMENT macro (gcc/default.h). This
> > > > > > > does
> > > > > > > not appear to change the default alignment on any architecture, but
> > > > > > > it causes a failure in i386/gcc.target/i386/attr-aligned.c when
> > > > > > > requesting a smaller alignment which is then silently ignored.
> > > > > > 
> > > > > > Ugh.  I didn't see that.
> > > > > 
> > > > > The test is new (2019-11-29 Martin Sebor), but one could
> > > > > argue that we could simply remove this specific test as 'aligned'
> > > > > is only required to increase alignment. Martin?
> > > > 
> > > > The test is meant to test that we do the right thing consistently.  If
> > > > we're failing with your patch, then that needs to be addressed.
> > > 
> > > I haven't been paying attention here and so I don't know how the test
> > > fails after the change.  It's meant to verify that attribute aligned
> > > successfully reduces the alignment of functions that have not been
> > > previously declared with one all the way down to the supported minimum
> > > (which is 1 on i386).  I agree with Jeff that removing the test would
> > > not be right unless the failure is due to some bad assumptions on my
> > > part.  If it's the built-in that fails that could be due to a bug in
> > > it (it's very new).
> > 
> > There is a choice to be made:
> > 
> > Either we activate the lang hook for C, then the minimum alignment for
> > functions on is not 1 anymore, because we need one bit to identify the
> > descriptors.  From a correcntess point of view, this is OK as 'alignas'
> > is only required to increase alignment.
> 
> alignas doesn't apply to functions.  Changing function alignment
> is a feature unique to attribute aligned.  The attribute can be
> used to decrease the alignment of functions that have not yet
> been explicitly declared with one.  GCC has historically allowed
> this, and based on my tests, the i386 target has always allowed
> functions to be completely unaligned (either by using attribute
> aligned (1) when supported or via -Os/-falign-functions=1).
> The purpose of the test is to verify that this works.
> 
> > It is also not really regression
> > in my opinion, as it is nowhere documented that one can reduce alignment
> > to '1'. The test also has just been added a couple of days ago (if I am
> > not mistaken). For these reasons, I think it would be OK to remove the test.
> 
> This wasn't clearly documented until recently.  The test was added
> to verify that GCC behaves as intended and clarified in the manual.
> The latest manual mentions that:
> 
>    The attribute cannot be used to decrease the alignment of
>    a function previously declared with a more restrictive alignment;
>    only to increase it.  Attempts to do otherwise are diagnosed.
>    Some targets specify a minimum default alignment for functions
>    that is greater than 1.  On such targets, specifying a less
>    restrictive alignment is silently ignored.
> 
> The update to the text was discussed here:
>    https://gcc.gnu.org/ml/gcc/2018-11/msg00127.html
> 
> If the i386 target should increase the minimum supported alignment
> up from 1 under some conditions the test might need to be adjusted
> (but it should not be removed).

Thank you for clarifying. Based on the discussion I think we can't
change the minimum alignment (at least without the command-line flag).

Best,
Martin

> > 
> > The other option is to decide that having an alignment of only '1'
> > is a valuable feature and should be preserved on those platforms which
> > support it. I have no idea what this could be good for, but maybe
> > there are use cases.  In this case, it makes of course sense to keep
> > the test.  We should then remove the lang hook (as it could never be
> > activated for most languages) and instead live with the fact that
> > '-fno-trampoline' and using alignof(1) on functions are simply
> > incompatible. A warning could be added if the compiler sees
> > alignof(1) when -fno-trampoline is active.
> > 
> > 
> > I am happy with both choices.
> > 
> > 
> > Best,
> > Martin
> > 
> > 
> 
> 

Re: [PATCH v4][C][ADA] use function descriptors instead of trampolines in C

[Uecker, Martin]

Am Montag, den 17.12.2018, 15:25 +0000 schrieb Szabolcs Nagy:
> On 16/12/2018 22:45, Uecker, Martin wrote:
> > Am Sonntag, den 16.12.2018, 09:13 -0700 schrieb Jeff Law:
> > > Ultimately using function descriptors is an ABI breaking choice and we
> > > might declare that function descriptors imply higher function
> > > alignments.  
> > 
> > Increasing the alignment is not an ABI breaking change.
> 
> increasing alignment _requirement_ is an abi breaking change.

You are right. The idea was to increase the minimum alignment
to always be compatible with code compiled with 
"-fno-trampolines" but without actually requiring the
alignment for other code as long as "-fno-trampolines"
is not given. 


> and it's not clear who would benefit from the new abi:
> 
> - it affects everything that does indirect calls (if alignment
> requirement is increased then in addition everything that has
> functions whose address may be taken), so it can easily affect
> existing handwritten asm and it definitely requires the rebuild
> of the c runtime to support this abi (i think it even requires
> asm changes there if you allow a thread or makecontext start
> function to be a nested function).
>
> - it makes indirect calls more expensive everywhere, even if
> nested functions are not used.

Yes, transition to "-fno-trampolines" by default would be a
major undertaking and the cost for
indirect calls might not
be acceptable. I was not proposing this.

> i think to fix the executable stack problem in practice, the
> new nested function mechanism should only require the rebuild
> of code that actually contains nested functions and thus have
> no abi or performance impact on code that never uses them.

My use case is to activate '-fno-trampolines' for some
project which use nested functions internally. This works
just fine with existing code because 1) no pointers to nested
functions escape 2) the default alignment on the existing
code is high enough.

This is a practical fix, but only when you are careful and
activate on a case by case. Of course, it is not a full solution
to the general problem. 


> i believe this can be achieved by some restrictions on nested
> function usage in a way that covers most practical use-cases:
> e.g. only allowing one active parent function call frame per
> thread, no recursive calls to it, the nested function must be
> invoked in the same thread as the parent using the same stack,
> etc. (then the new mechanism can be used safely if nested
> functions are known to follow the restrictions, the compiler
> may even emit code to check the constraints at runtime.)

So a thread_local static variable for storing the static
chain?

Best,
Martin

Re: [PATCH v4][C][ADA] use function descriptors instead of trampolines in C

[Andreas Schwab]

On Dez 17 2018, "Uecker, Martin" <Martin.Uecker@med.uni-goettingen.de> wrote:

> Am Montag, den 17.12.2018, 10:28 -0700 schrieb Jeff Law:
>> On 12/16/18 3:45 PM, Uecker, Martin wrote:
>> > But most architectures require a higher alignment anyway.
>> > Here is a list of all targets where function alignment
>> > is 1 byte:
>> > 
>> > gcc/config/avr/avr.h:#define FUNCTION_BOUNDARY 8
>> > gcc/config/i386/i386.h:#define FUNCTION_BOUNDARY 8
>> > gcc/config/m32c/m32c.h:#define FUNCTION_BOUNDARY 8
>> > gcc/config/mn10300/mn10300.h:#define FUNCTION_BOUNDARY 8
>> > gcc/config/pa/pa.h:#define FUNCTION_BOUNDARY BITS_PER_WORD
>> > gcc/config/rl78/rl78.h:#define FUNCTION_BOUNDARY 		8
>> > gcc/config/rx/rx.h:#define FUNCTION_BOUNDARY 		((rx_cpu_type == RX100 ||
>> > rx_cpu_type == RX200) ? 4 : 8)
>
> (BTW: pa was included here by mistake)

The rx config apparently confused bits and bytes.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

Re: [PATCH v4][C][ADA] use function descriptors instead of trampolines in C

[Szabolcs Nagy]

On 17/12/2018 18:22, Uecker, Martin wrote:
> Am Montag, den 17.12.2018, 15:25 +0000 schrieb Szabolcs Nagy:
>> On 16/12/2018 22:45, Uecker, Martin wrote:
>>> Am Sonntag, den 16.12.2018, 09:13 -0700 schrieb Jeff Law:
>>>> Ultimately using function descriptors is an ABI breaking choice and we
>>>> might declare that function descriptors imply higher function
>>>> alignments.  
>>>
>>> Increasing the alignment is not an ABI breaking change.
>>
>> increasing alignment _requirement_ is an abi breaking change.
> 
> You are right. The idea was to increase the minimum alignment
> to always be compatible with code compiled with 
> "-fno-trampolines" but without actually requiring the
> alignment for other code as long as "-fno-trampolines"
> is not given. 
> 
> 
>> and it's not clear who would benefit from the new abi:
>>
>> - it affects everything that does indirect calls (if alignment
>> requirement is increased then in addition everything that has
>> functions whose address may be taken), so it can easily affect
>> existing handwritten asm and it definitely requires the rebuild
>> of the c runtime to support this abi (i think it even requires
>> asm changes there if you allow a thread or makecontext start
>> function to be a nested function).
>>
>> - it makes indirect calls more expensive everywhere, even if
>> nested functions are not used.
> 
> Yes, transition to "-fno-trampolines" by default would be a
> major undertaking and the cost for
> indirect calls might not
> be acceptable. I was not proposing this.
> 
>> i think to fix the executable stack problem in practice, the
>> new nested function mechanism should only require the rebuild
>> of code that actually contains nested functions and thus have
>> no abi or performance impact on code that never uses them.
> 
> My use case is to activate '-fno-trampolines' for some
> project which use nested functions internally. This works
> just fine with existing code because 1) no pointers to nested
> functions escape 2) the default alignment on the existing
> code is high enough.
> 
> This is a practical fix, but only when you are careful and
> activate on a case by case. Of course, it is not a full solution
> to the general problem. 

i see.
i think that's not a common use-case.
i'd expect nested function pointers to often escape
(as callbacks to extern library function calls).

>> i believe this can be achieved by some restrictions on nested
>> function usage in a way that covers most practical use-cases:
>> e.g. only allowing one active parent function call frame per
>> thread, no recursive calls to it, the nested function must be
>> invoked in the same thread as the parent using the same stack,
>> etc. (then the new mechanism can be used safely if nested
>> functions are known to follow the restrictions, the compiler
>> may even emit code to check the constraints at runtime.)
> 
> So a thread_local static variable for storing the static
> chain?

something like that, but the more i think about it the
harder it seems: the call site of the nested function
may not be under control of the nested function writer,
in particular the nested function may be called on a
different thread, and extern library apis are unlikely
to provide guarantees about this, so in general if a
nested function escapes into an extern library then
this cannot be relied on, which limits my original
idea again to cases where there is no escape (which i
think is not that useful).

Re: [PATCH v4][C][ADA] use function descriptors instead of trampolines in C

[Paul Koning]

> On Dec 17, 2018, at 2:23 PM, Szabolcs Nagy <Szabolcs.Nagy@arm.com> wrote:
> 
> On 17/12/2018 18:22, Uecker, Martin wrote:
>>> 
>>> ...
>> 
>> So a thread_local static variable for storing the static
>> chain?
> 
> something like that, but the more i think about it the
> harder it seems: the call site of the nested function
> may not be under control of the nested function writer,
> in particular the nested function may be called on a
> different thread, and extern library apis are unlikely
> to provide guarantees about this, so in general if a
> nested function escapes into an extern library then
> this cannot be relied on, which limits my original
> idea again to cases where there is no escape (which i
> think is not that useful).

I'm not sure I understand "escape" of a nested function pointer. 

Your description makes it sound like you're talking about a function being called by someone who has been given the pointer, from outside the scope of the function.  That sounds like an illegal operation, exactly as it would be if you attempted to reference an automatic variable via a pointer from outside the scope of that variable.

Did I misunderstand?

	paul

Re: [PATCH v4][C][ADA] use function descriptors instead of trampolines in C

[Jakub Jelinek]

On Tue, Dec 18, 2018 at 10:23:46AM -0500, Paul Koning wrote:
> 
> 
> > On Dec 17, 2018, at 2:23 PM, Szabolcs Nagy <Szabolcs.Nagy@arm.com> wrote:
> > 
> > On 17/12/2018 18:22, Uecker, Martin wrote:
> >>> 
> >>> ...
> >> 
> >> So a thread_local static variable for storing the static
> >> chain?
> > 
> > something like that, but the more i think about it the
> > harder it seems: the call site of the nested function
> > may not be under control of the nested function writer,
> > in particular the nested function may be called on a
> > different thread, and extern library apis are unlikely
> > to provide guarantees about this, so in general if a
> > nested function escapes into an extern library then
> > this cannot be relied on, which limits my original
> > idea again to cases where there is no escape (which i
> > think is not that useful).
> 
> I'm not sure I understand "escape" of a nested function pointer. 
> 
> Your description makes it sound like you're talking about a function being called by someone who has been given the pointer, from outside the scope of the function.  That sounds like an illegal operation, exactly as it would be if you attempted to reference an automatic variable via a pointer from outside the scope of that variable.
> 
> Did I misunderstand?

The most common case is when you pass a call to a nested function
to some function that has a function pointer argument, e.g. qsort.
This is well defined with GNU nested functions, but the function that calls
the callback (qsort in this case) doesn't know it is a call to a nested
function.

#include <stdlib.h>
#include <stdbool.h>

int
main ()
{
  bool r = false;
  auto int cmp (const void *a, const void *b)
  {
    const signed char *c = (const signed char *) a;
    const signed char *d = (const signed char *) b;
    return r ? *c - *d : *d - *c;
  }

  signed char l[8] = { 10, 2, 11, 21, 0, 7, 18, 12 };
  qsort (l, 8, 1, cmp);
}

	Jakub

Re: [PATCH v4][C][ADA] use function descriptors instead of trampolines in C

[Jeff Law]

On 12/18/18 8:32 AM, Jakub Jelinek wrote:
> On Tue, Dec 18, 2018 at 10:23:46AM -0500, Paul Koning wrote:
>>
>>
>>> On Dec 17, 2018, at 2:23 PM, Szabolcs Nagy <Szabolcs.Nagy@arm.com> wrote:
>>>
>>> On 17/12/2018 18:22, Uecker, Martin wrote:
>>>>>
>>>>> ...
>>>>
>>>> So a thread_local static variable for storing the static
>>>> chain?
>>>
>>> something like that, but the more i think about it the
>>> harder it seems: the call site of the nested function
>>> may not be under control of the nested function writer,
>>> in particular the nested function may be called on a
>>> different thread, and extern library apis are unlikely
>>> to provide guarantees about this, so in general if a
>>> nested function escapes into an extern library then
>>> this cannot be relied on, which limits my original
>>> idea again to cases where there is no escape (which i
>>> think is not that useful).
>>
>> I'm not sure I understand "escape" of a nested function pointer. 
>>
>> Your description makes it sound like you're talking about a function being called by someone who has been given the pointer, from outside the scope of the function.  That sounds like an illegal operation, exactly as it would be if you attempted to reference an automatic variable via a pointer from outside the scope of that variable.
>>
>> Did I misunderstand?
> 
> The most common case is when you pass a call to a nested function
> to some function that has a function pointer argument, e.g. qsort.
> This is well defined with GNU nested functions, but the function that calls
> the callback (qsort in this case) doesn't know it is a call to a nested
> function.
Right.  This is the classic example and highlights the ABI concerns.  If
we use the low bit to distinguish between a normal function pointer and
a pointer to a descriptor and qsort doesn't know about it, then we lose.

One way around this is to make *all* function pointers be some kind of
descriptor and route all indirect calls through a resolver.  THen you
need either linker hackery or special code to compare function pointers
to preserve ISO C behavior.

Note that if you have a nested function and take its address, then go
out of scope of the containing function, then that function pointer is
no longer valid -- which makes perfect sense if you think about it.  THe
trampoline was on the stack and if you go out of scope of the containing
function, then that stack frame is invalid and you also don't have a
suitable frame chain to pass to the nested function either.


Jeff

Re: [PATCH v4][C][ADA] use function descriptors instead of trampolines in C

[Jakub Jelinek]

On Tue, Dec 18, 2018 at 09:03:41AM -0700, Jeff Law wrote:
> Right.  This is the classic example and highlights the ABI concerns.  If
> we use the low bit to distinguish between a normal function pointer and
> a pointer to a descriptor and qsort doesn't know about it, then we lose.
> 
> One way around this is to make *all* function pointers be some kind of
> descriptor and route all indirect calls through a resolver.  THen you

Either way, you are creating a new ABI for calling functions through
function pointers.  Because of how rarely GNU C nested functions are used
these days, if we want to do anything I'd think it might be better to use
trampolines, just don't place them on the stack, say have a mmaped page of
trampolines perhaps with some pointer encryption to where they jump to, so
it isn't a way to circumvent non-executable stack, and have some register
and unregister function you'd call to get or release the trampoline.
If more trampolines are needed than currently available, the library could
just mmap another such page.  A problem is how it should interact with
longjmp or similar APIs, because then we could leak some trampolines (no
"destructor" for the trampoline would be called.  The leaking could be
handled e.g. through remembering the thread and frame pointer for which it
has been allocated and if you ask for a new trampoline with a frame pointer
above the already allocated one, release those entries or reuse them,
instead of allocating a new one.  And somehow deal with thread exit.

	Jakub

Re: [PATCH v4][C][ADA] use function descriptors instead of trampolines in C

[Uecker, Martin]

Am Dienstag, den 18.12.2018, 09:03 -0700 schrieb Jeff Law:
> On 12/18/18 8:32 AM, Jakub Jelinek wrote:
> > On Tue, Dec 18, 2018 at 10:23:46AM -0500, Paul Koning wrote:
> > > 
> > > 
> > > > On Dec 17, 2018, at 2:23 PM, Szabolcs Nagy <Szabolcs.Nagy@arm.com> wrote:
> > > > 
> > > > On 17/12/2018 18:22, Uecker, Martin wrote:
> > > > > > 
> > > > > > ...
> > > > > 
> > > > > So a thread_local static variable for storing the static
> > > > > chain?
> > > > 
> > > > something like that, but the more i think about it the
> > > > harder it seems: the call site of the nested function
> > > > may not be under control of the nested function writer,
> > > > in particular the nested function may be called on a
> > > > different thread, and extern library apis are unlikely
> > > > to provide guarantees about this, so in general if a
> > > > nested function escapes into an extern library then
> > > > this cannot be relied on, which limits my original
> > > > idea again to cases where there is no escape (which i
> > > > think is not that useful).
> > > 
> > > I'm not sure I understand "escape" of a nested function pointer. 
> > > 
> > > Your description makes it sound like you're talking about a function being called by someone
> > > who has been given the pointer, from outside the scope of the function.  That sounds like an
> > > illegal operation, exactly as it would be if you attempted to reference an automatic variable
> > > via a pointer from outside the scope of that variable.
> > > 
> > > Did I misunderstand?
> > 
> > The most common case is when you pass a call to a nested function
> > to some function that has a function pointer argument, e.g. qsort.
> > This is well defined with GNU nested functions, but the function that calls
> > the callback (qsort in this case) doesn't know it is a call to a nested
> > function.
> 
> Right.  This is the classic example and highlights the ABI concerns.  If
> we use the low bit to distinguish between a normal function pointer and
> a pointer to a descriptor and qsort doesn't know about it, then we lose.
> 
> One way around this is to make *all* function pointers be some kind of
> descriptor and route all indirect calls through a resolver.  THen you
> need either linker hackery or special code to compare function pointers
> to preserve ISO C behavior.

If it has to work with existing code and without the restrictions
discussed above then you have to create a pointer to a new code address 
for each nested functions. One possibility which comes to mind would be a
shadow stack which is executable (and might contain pre-allocated
trampolines).


Best,
Martin


> Note that if you have a nested function and take its address, then go
> out of scope of the containing function, then that function pointer is
> no longer valid -- which makes perfect sense if you think about it.  THe
> trampoline was on the stack and if you go out of scope of the containing
> function, then that stack frame is invalid and you also don't have a
> suitable frame chain to pass to the nested function either.
> 
> 
> Jeff

Re: [PATCH v4][C][ADA] use function descriptors instead of trampolines in C

[Uecker, Martin]

Am Dienstag, den 18.12.2018, 17:24 +0100 schrieb Jakub Jelinek:
> On Tue, Dec 18, 2018 at 09:03:41AM -0700, Jeff Law wrote:
> > Right.  This is the classic example and highlights the ABI concerns.  If
> > we use the low bit to distinguish between a normal function pointer and
> > a pointer to a descriptor and qsort doesn't know about it, then we lose.
> > 
> > One way around this is to make *all* function pointers be some kind of
> > descriptor and route all indirect calls through a resolver.  THen you
> 
> Either way, you are creating a new ABI for calling functions through
> function pointers.  Because of how rarely GNU C nested functions are used
> these days, if we want to do anything I'd think it might be better to use
> trampolines, just don't place them on the stack, say have a mmaped page of
> trampolines perhaps with some pointer encryption to where they jump to, so
> it isn't a way to circumvent non-executable stack, and have some register
> and unregister function you'd call to get or release the trampoline.
> If more trampolines are needed than currently available, the library could
> just mmap another such page.  A problem is how it should interact with
> longjmp or similar APIs, because then we could leak some trampolines (no
> "destructor" for the trampoline would be called.  The leaking could be
> handled e.g. through remembering the thread and frame pointer for which it
> has been allocated and if you ask for a new trampoline with a frame pointer
> above the already allocated one, release those entries or reuse them,
> instead of allocating a new one.  And somehow deal with thread exit.

Yes, something like this. If the trampolines are pre-allocated, this could
even avoid the need to clear the cache on archs where this is needed.

Best,
Martin

Re: [PATCH v4][C][ADA] use function descriptors instead of trampolines in C

[Uecker, Martin]

Am Dienstag, den 18.12.2018, 17:29 +0100 schrieb Martin Uecker:
> Am Dienstag, den 18.12.2018, 17:24 +0100 schrieb Jakub Jelinek:
> > On Tue, Dec 18, 2018 at 09:03:41AM -0700, Jeff Law wrote:
> > > Right.  This is the classic example and highlights the ABI concerns.  If
> > > we use the low bit to distinguish between a normal function pointer and
> > > a pointer to a descriptor and qsort doesn't know about it, then we lose.
> > > 
> > > One way around this is to make *all* function pointers be some kind of
> > > descriptor and route all indirect calls through a resolver.  THen you
> > 
> > Either way, you are creating a new ABI for calling functions through
> > function pointers.  Because of how rarely GNU C nested functions are used
> > these days, if we want to do anything I'd think it might be better to use
> > trampolines, just don't place them on the stack, say have a mmaped page of
> > trampolines perhaps with some pointer encryption to where they jump to, so
> > it isn't a way to circumvent non-executable stack, and have some register
> > and unregister function you'd call to get or release the trampoline.
> > If more trampolines are needed than currently available, the library could
> > just mmap another such page.  A problem is how it should interact with
> > longjmp or similar APIs, because then we could leak some trampolines (no
> > "destructor" for the trampoline would be called.  The leaking could be
> > handled e.g. through remembering the thread and frame pointer for which it
> > has been allocated and if you ask for a new trampoline with a frame pointer
> > above the already allocated one, release those entries or reuse them,
> > instead of allocating a new one.  And somehow deal with thread exit.
> 
> Yes, something like this. If the trampolines are pre-allocated, this could
> even avoid the need to clear the cache on archs where this is needed.

And if we can make the trampolines be all the same (and it somehow derived
from the IP where it has to look for the static chain), we could map the
same page of pre-allocated trampolines and not use memory on platforms
with virtual memory.

Best,
Martin

Re: [PATCH v4][C][ADA] use function descriptors instead of trampolines in C

[Jakub Jelinek]

On Tue, Dec 18, 2018 at 04:33:48PM +0000, Uecker, Martin wrote:
> > Yes, something like this. If the trampolines are pre-allocated, this could
> > even avoid the need to clear the cache on archs where this is needed.
> 
> And if we can make the trampolines be all the same (and it somehow derived
> from the IP where it has to look for the static chain), we could map the
> same page of pre-allocated trampolines and not use memory on platforms
> with virtual memory.

Yeah, if it is e.g. a pair of executable page and data page right after it,
say for x86_64 page of:
pushq $0
jmp .L1
pushq $1
jmp .L1
...
push $NNN
jmp .L1
# Almost at the end of page
.L1:
decode the above pushed number
read + decrypt the data (both where to jump to and static chain)
set static chain reg to the static chain data
jmp *function pointer
it could just mmap both pages at once PROT_NONE, and then mmap one from the
file and fill in data in the other page.  Or perhaps one executable and two
data pages, depending on the exact sizes of needed data vs. code.

	Jakub

Re: [PATCH v4][C][ADA] use function descriptors instead of trampolines in C

[Uecker, Martin]

Am Sonntag, den 16.12.2018, 09:13 -0700 schrieb Jeff Law:

> It's also important to remember that not every target which uses
> function descriptors uses the LSB.  On some targets the LSB may switch
> between modes (arm vs thumb for example).  So on those targets the use
> of descriptors may imply an even larger minimum alignment.

There is a similar mechanism for pointer-to-member-functions
used by C++. Is this correct on aarch64?

/* By default, the C++ compiler will use the lowest bit of the pointer
   to function to indicate a pointer-to-member-function points to a
   virtual member function.  However, if FUNCTION_BOUNDARY indicates
   function addresses aren't always even, the lowest bit of the delta
   field will be used.  */
#ifndef TARGET_PTRMEMFUNC_VBIT_LOCATION
#define TARGET_PTRMEMFUNC_VBIT_LOCATION \
  (FUNCTION_BOUNDARY >= 2 * BITS_PER_UNIT \
   ? ptrmemfunc_vbit_in_pfn : ptrmemfunc_vbit_in_delta)
#endif


Best,
Martin

Re: [PATCH v4][C][ADA] use function descriptors instead of trampolines in C

[Uecker, Martin]

Am Dienstag, den 18.12.2018, 17:42 +0100 schrieb Jakub Jelinek:
> On Tue, Dec 18, 2018 at 04:33:48PM +0000, Uecker, Martin wrote:
> > > Yes, something like this. If the trampolines are pre-allocated, this could
> > > even avoid the need to clear the cache on archs where this is needed.
> > 
> > And if we can make the trampolines be all the same (and it somehow derived
> > from the IP where it has to look for the static chain), we could map the
> > same page of pre-allocated trampolines and not use memory on platforms
> > with virtual memory.
> 
> Yeah, if it is e.g. a pair of executable page and data page right after it,
> say for x86_64 page of:
> pushq $0
> jmp .L1
> pushq $1
> jmp .L1
> ...
> push $NNN
> jmp .L1
> # Almost at the end of page
> .L1:
> decode the above pushed number
> read + decrypt the data (both where to jump to and static chain)
> set static chain reg to the static chain data
> jmp *function pointer
> it could just mmap both pages at once PROT_NONE, and then mmap one from the
> file and fill in data in the other page.  Or perhaps one executable and two
> data pages, depending on the exact sizes of needed data vs. code.

What do you think about making the trampoline a single call
instruction and have a large memory region which is the same
page mapped many times?


call trampoline_handler
call trampoline_handler
call trampoline_handler
...
...
many identical read-only pages
...
...


The trampoline handler would pop the instruction pointer and use
this as an index into the real stack to read the static chain and
function pointer.


Creation of a trampoline would consist of storing
static chain and function on the stack (with
right alignment) and simply return the
corresponding address in the shadow stack.


Best,
Martin

Re: [PATCH v4][C][ADA] use function descriptors instead of trampolines in C

[Jakub Jelinek]

On Wed, Dec 19, 2018 at 07:53:48PM +0000, Uecker, Martin wrote:
> What do you think about making the trampoline a single call
> instruction and have a large memory region which is the same
> page mapped many times?
> 
> 
> call trampoline_handler
> call trampoline_handler
> call trampoline_handler
> ...
> ...
> many identical read-only pages
> ...
> ...
> 
> 
> The trampoline handler would pop the instruction pointer and use
> this as an index into the real stack to read the static chain and
> function pointer.

While you save a few bytes per trampoline that way, it is heavily call-ret
stack unfriendly, so it will not be very fast.

	Jakub

Re: [PATCH v4][C][ADA] use function descriptors instead of trampolines in C

[Wilco Dijkstra]

Hi,

Jakub Jelinek wrote:
> On Wed, Dec 19, 2018 at 07:53:48PM +0000, Uecker, Martin wrote:
>> What do you think about making the trampoline a single call
>> instruction and have a large memory region which is the same
>> page mapped many times?

This sounds like a good idea, but given a function descriptor is 8-16 bytes
it doesn't need to be 1 instruction. You can even go for larger sizes since
all it affects is minimum alignment of function descriptors.

>> The trampoline handler would pop the instruction pointer and use
>> this as an index into the real stack to read the static chain and
>> function pointer.
>
> While you save a few bytes per trampoline that way, it is heavily call-ret
> stack unfriendly, so it will not be very fast.

A repeated page adjacent to the stack is a good idea since it avoids adding
runtime support to push/pop nested function addresses. That would be
inefficient and likely very tricky for setjmp and exception handling
(or leak memory).

Since it can use several instructions we could load the static chain register
with the PC for example. On ISAs that don't support PC-relative addressing
you could do a call/ret sequence to get the PC and then tailcall the helper
to keep the return stack intact.

If computing the difference between the stack and trampoline region takes
just a few instructions (eg. thread local storage) then it could even be inlined.

Wilco 
    

Re: [PATCH v4][C][ADA] use function descriptors instead of trampolines in C

[Wilco Dijkstra]

Hi Martin,

> There is a similar mechanism for pointer-to-member-functions
> used by C++. Is this correct on aarch64?

/* By default, the C++ compiler will use the lowest bit of the pointer
   to function to indicate a pointer-to-member-function points to a
   virtual member function.  However, if FUNCTION_BOUNDARY indicates
   function addresses aren't always even, the lowest bit of the delta
   field will be used.  */
#ifndef TARGET_PTRMEMFUNC_VBIT_LOCATION
#define TARGET_PTRMEMFUNC_VBIT_LOCATION \
  (FUNCTION_BOUNDARY >= 2 * BITS_PER_UNIT \
   ? ptrmemfunc_vbit_in_pfn : ptrmemfunc_vbit_in_delta)
#endif

AArch64 overrides this like Arm to ptrmemfunc_vbit_in_delta.

But yes that's yet another example of GCC using the wrong default.
The assumption that FUNCTION_BOUNDARY implies no use of low
bits is simply false - a generic default should be safe for all targets
(and if that's not possible, there should not be a default).

Cheers,
Wilco

[PATCH v5][C][ADA] use function descriptors instead of trampolines in C

[Uecker, Martin]

Am Montag, den 17.12.2018, 10:28 -0700 schrieb Jeff Law:

> > But the alignment increase itself on 'i386' and 'aarch64'
> > might be unacceptable. In this case, the only safe change
> > is to make the higher alignment also depend on
> > "-fno-trampolines".  Would this be acceptable?
> 
> Unclear at this point.

Hi Jeff,

in any case, here is another revision of this patch for
consideration which implements just this.

The lang hook is not activated for C anymore which means
that this patch is a no-op when -fno-trampolines is not
given. So the test about achieving the minimum alignment on
i386 does not fail anymore. 

With -fno-trampolines the minimum alignment is increased
as needed on platforms which support descriptors. 
For C and Ada (where it is the default) function
descriptors are created instead of trampolines.

If -fno-trampolines is given on platforms which do not
support descriptors a warning is given (before the flag
was silently ignored.)

I also made the existing warnings about the ABI change
more visible (similar to -freg-struct-return or similar
flags). 

I consider the current behavior broken, where the flag
is a silent no-op for most languages and on some targets
while the documentation implies otherwise.

Bootstrapped and regression tested on x86.

I also tested -fno-trampolines on a project of mine which
uses nested functions and has a test suite and there it
works perfectly and removes the need for the executable stack.

Finally, this patch is a tiny and self-contained change which
could easily be reverted if there should be any unanticipated
failures.

Best,
Martin



diff --git a/gcc/ChangeLog b/gcc/ChangeLog
index 4be16c15f86..8825d87b44f 100644
--- a/gcc/ChangeLog
+++ b/gcc/ChangeLog
@@ -1,3 +1,16 @@
+2018-12-20  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.
+
 2018-12-20  Vladimir Makarov  <vmakarov@redhat.com>
 
 	PR target/88457
diff --git a/gcc/ada/ChangeLog b/gcc/ada/ChangeLog
index ba974cdcb03..c2f3d0db281 100644
--- a/gcc/ada/ChangeLog
+++ b/gcc/ada/ChangeLog
@@ -1,3 +1,8 @@
+2018-12-20  Martin Uecker  <martin.uecker@med.uni-goettingen.de>
+
+	* gcc-interface/trans.c (Attribute_to_gnu): Add check for
+	flag_trampolines.
+
 2018-12-14  Eric Botcazou  <ebotcazou@adacore.com>
 
 	* gcc-interface/decl.c (rm_size): Take into account the padding in
diff --git a/gcc/ada/gcc-interface/trans.c b/gcc/ada/gcc-interface/trans.c
index 620dbd3d36d..ae4139e9b84 100644
--- a/gcc/ada/gcc-interface/trans.c
+++ b/gcc/ada/gcc-interface/trans.c
@@ -2239,7 +2239,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
@@ -5050,7 +5051,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 6e12dda2331..71443846799 100644
--- a/gcc/c/ChangeLog
+++ b/gcc/c/ChangeLog
@@ -1,3 +1,9 @@
+2018-12-20  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.
+
 2018-12-19  Segher Boessenkool  <segher@kernel.crashing.org>
 
 	* c-parser.c (c_parser_asm_statement) <RID_CONST, RID_RESTRICT>: Give
diff --git a/gcc/c/c-typeck.c b/gcc/c/c-typeck.c
index 1ae5ede81e6..6363dfda3b8 100644
--- a/gcc/c/c-typeck.c
+++ b/gcc/c/c-typeck.c
@@ -1913,7 +1913,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
@@ -3135,6 +3142,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 e3b4ef80e51..1b977c231ea 100644
--- a/gcc/calls.c
+++ b/gcc/calls.c
@@ -230,7 +230,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 45d7f6189e5..d85d77b5b91 100644
--- a/gcc/common.opt
+++ b/gcc/common.opt
@@ -2546,7 +2546,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 9035b333be8..66de347edef 100644
--- a/gcc/defaults.h
+++ b/gcc/defaults.h
@@ -1050,7 +1050,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 ac2ee59d92c..5662abd1b97 100644
--- a/gcc/doc/invoke.texi
+++ b/gcc/doc/invoke.texi
@@ -14030,9 +14030,10 @@ 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
+@strong{Warning:} 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.
diff --git a/gcc/doc/sourcebuild.texi b/gcc/doc/sourcebuild.texi
index 29c693b9644..d645d1def92 100644
--- a/gcc/doc/sourcebuild.texi
+++ b/gcc/doc/sourcebuild.texi
@@ -2162,6 +2162,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 33a4776b921..24f443bb26f 100644
--- a/gcc/testsuite/ChangeLog
+++ b/gcc/testsuite/ChangeLog
@@ -1,3 +1,9 @@
+2018-12-20  Martin Uecker  <martin.uecker@med.uni-goettingen.de>
+
+	* gcc.dg/trampoline-2.c: New test.
+	* lib/target-supports.exp 
+	(check_effective_target_notrampolines): New.
+
 2018-12-20  Vladimir Makarov  <vmakarov@redhat.com>
 
 	PR target/88457
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 7dec43233e0..43f9bb3891f 100644
--- a/gcc/testsuite/lib/target-supports.exp
+++ b/gcc/testsuite/lib/target-supports.exp
@@ -924,6 +924,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 ab20cd98969..765ce347172 100644
--- a/gcc/toplev.c
+++ b/gcc/toplev.c
@@ -1698,6 +1698,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 0ad469ada49..eb9bccb7a9d 100644
--- a/gcc/tree-nested.c
+++ b/gcc/tree-nested.c
@@ -2544,7 +2544,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);

Re: [PATCH v4][C][ADA] use function descriptors instead of trampolines in C

[Hans-Peter Nilsson]

[-- Attachment #1: Type: text/plain, Size: 2581 bytes --]

On Tue, 18 Dec 2018, Uecker, Martin wrote:
> Am Dienstag, den 18.12.2018, 17:29 +0100 schrieb Martin Uecker:
> > Am Dienstag, den 18.12.2018, 17:24 +0100 schrieb Jakub Jelinek:
> > > On Tue, Dec 18, 2018 at 09:03:41AM -0700, Jeff Law wrote:
> > > > Right.  This is the classic example and highlights the ABI concerns.  If
> > > > we use the low bit to distinguish between a normal function pointer and
> > > > a pointer to a descriptor and qsort doesn't know about it, then we lose.
> > > >
> > > > One way around this is to make *all* function pointers be some kind of
> > > > descriptor and route all indirect calls through a resolver.  THen you
> > >
> > > Either way, you are creating a new ABI for calling functions through
> > > function pointers.  Because of how rarely GNU C nested functions are used
> > > these days, if we want to do anything I'd think it might be better to use
> > > trampolines, just don't place them on the stack, say have a mmaped page of
> > > trampolines perhaps with some pointer encryption to where they jump to, so
> > > it isn't a way to circumvent non-executable stack, and have some register
> > > and unregister function you'd call to get or release the trampoline.
> > > If more trampolines are needed than currently available, the library could
> > > just mmap another such page.  A problem is how it should interact with
> > > longjmp or similar APIs, because then we could leak some trampolines (no
> > > "destructor" for the trampoline would be called.  The leaking could be
> > > handled e.g. through remembering the thread and frame pointer for which it
> > > has been allocated and if you ask for a new trampoline with a frame pointer
> > > above the already allocated one, release those entries or reuse them,
> > > instead of allocating a new one.  And somehow deal with thread exit.
> >
> > Yes, something like this. If the trampolines are pre-allocated, this could
> > even avoid the need to clear the cache on archs where this is needed.
>
> And if we can make the trampolines be all the same (and it somehow derived
> from the IP where it has to look for the static chain), we could map the
> same page of pre-allocated trampolines and not use memory on platforms
> with virtual memory.

All fine with new ideas, but consider the case where the nested
functions are nested.  All mentioned ideas seem to fail for the
case where a caller (generating a trampoline to be called later)
is re-entered, i.e. need to generate another trampoline.  The
same location can't be re-used.  You need a sort of stack.

brgds, H-P

Re: [PATCH v4][C][ADA] use function descriptors instead of trampolines in C

[Uecker, Martin]

Am Freitag, den 21.12.2018, 16:13 -0500 schrieb Hans-Peter Nilsson:
> On Tue, 18 Dec 2018, Uecker, Martin wrote:
> > Am Dienstag, den 18.12.2018, 17:29 +0100 schrieb Martin Uecker:
> > > Am Dienstag, den 18.12.2018, 17:24 +0100 schrieb Jakub Jelinek:
> > > > On Tue, Dec 18, 2018 at 09:03:41AM -0700, Jeff Law wrote:
> > > > > Right.  This is the classic example and highlights the ABI concerns.  If
> > > > > we use the low bit to distinguish between a normal function pointer and
> > > > > a pointer to a descriptor and qsort doesn't know about it, then we lose.
> > > > > 
> > > > > One way around this is to make *all* function pointers be some kind of
> > > > > descriptor and route all indirect calls through a resolver.  THen you
> > > > 
> > > > Either way, you are creating a new ABI for calling functions through
> > > > function pointers.  Because of how rarely GNU C nested functions are used
> > > > these days, if we want to do anything I'd think it might be better to use
> > > > trampolines, just don't place them on the stack, say have a mmaped page of
> > > > trampolines perhaps with some pointer encryption to where they jump to, so
> > > > it isn't a way to circumvent non-executable stack, and have some register
> > > > and unregister function you'd call to get or release the trampoline.
> > > > If more trampolines are needed than currently available, the library could
> > > > just mmap another such page.  A problem is how it should interact with
> > > > longjmp or similar APIs, because then we could leak some trampolines (no
> > > > "destructor" for the trampoline would be called.  The leaking could be
> > > > handled e.g. through remembering the thread and frame pointer for which it
> > > > has been allocated and if you ask for a new trampoline with a frame pointer
> > > > above the already allocated one, release those entries or reuse them,
> > > > instead of allocating a new one.  And somehow deal with thread exit.
> > > 
> > > Yes, something like this. If the trampolines are pre-allocated, this could
> > > even avoid the need to clear the cache on archs where this is needed.
> > 
> > And if we can make the trampolines be all the same (and it somehow derived
> > from the IP where it has to look for the static chain), we could map the
> > same page of pre-allocated trampolines and not use memory on platforms
> > with virtual memory.
> 
> All fine with new ideas, but consider the case where the nested
> functions are nested.  All mentioned ideas seem to fail for the
> case where a caller (generating a trampoline to be called later)
> is re-entered, i.e. need to generate another trampoline.  The
> same location can't be re-used.  You need a sort of stack.

Yes, you need to be able to create arbitrary number of trampolines.

But this would work: One can use a second stack with pre-allocated
readonly trampolines. Everytime you would now create a trampoline on
the real stack you simply refer to an existing trampoline at the
same location on the parallel stack. And if these trampolines are
all identical, you only need a single real page which is mapped
many times.

Setting up a stack would be more complicated because you also
need to setup this parallel stack. Maybe simulating this second
stack with a global hash table which uses thread id and stack
pointer of the real stack as index is better...

Best,
Martin

[PING] [PATCH v5][C][ADA] use function descriptors instead of trampolines in C

[Uecker, Martin]

Does this patch have a change? This version seems risk-free and
is a clear improvement from simply doing nothing for 
'-fno-trampolines'. Also it is useful in situations where
one cannot have an executable stack.


I am currently thinking about working
around this problem by calling nested functions with the
following macro (x86_64 only):


#define NESTED_CALL(p, args) ({ \
_auto_type __p = (p); \
struct { unsigned short mov1; void* addr; unsigned short mov2; void* chain; unsigned int jmp; } \
__attribute__((packed))* __t = (void*)p; \
assert((0xbb49 == __t->mov1) && (0xba49 == __t->mov2) && (0x90e3ff49 == __t->jmp)); \
__builtin_call_with_static_chain(((__typeof__(__p))(__t->addr))args, __t->chain); \
})

(the 'assert' could be replaced with an 'if' to call
regular functions when pointers to nested functions
and regular functions are mixed)


But I would rather have a proper solution...

Best,
Martin


Am Freitag, den 21.12.2018, 07:46 +0100 schrieb Martin Uecker:
> Am Montag, den 17.12.2018, 10:28 -0700 schrieb Jeff Law:
> 
> > > But the alignment increase itself on 'i386' and 'aarch64'
> > > might be unacceptable. In this case, the only safe change
> > > is to make the higher alignment also depend on
> > > "-fno-trampolines".  Would this be acceptable?
> > 
> > Unclear at this point.
> 
> Hi Jeff,
> 
> in any case, here is another revision of this patch for
> consideration which implements just this.
> 
> The lang hook is not activated for C anymore which means
> that this patch is a no-op when -fno-trampolines is not
> given. So the test about achieving the minimum alignment on
> i386 does not fail anymore. 
> 
> With -fno-trampolines the minimum alignment is increased
> as needed on platforms which support descriptors. 
> For C and Ada (where it is the default) function
> descriptors are created instead of trampolines.
> 
> If -fno-trampolines is given on platforms which do not
> support descriptors a warning is given (before the flag
> was silently ignored.)
> 
> I also made the existing warnings about the ABI change
> more visible (similar to -freg-struct-return or similar
> flags). 
> 
> I consider the current behavior broken, where the flag
> is a silent no-op for most languages and on some targets
> while the documentation implies otherwise.
> 
> Bootstrapped and regression tested on x86.
> 
> I also tested -fno-trampolines on a project of mine which
> uses nested functions and has a test suite and there it
> works perfectly and removes the need for the executable stack.
> 
> Finally, this patch is a tiny and self-contained change which
> could easily be reverted if there should be any unanticipated
> failures.
> 
> Best,
> Martin
> 
> 
> 
> diff --git a/gcc/ChangeLog b/gcc/ChangeLog
> index 4be16c15f86..8825d87b44f 100644
> --- a/gcc/ChangeLog
> +++ b/gcc/ChangeLog
> @@ -1,3 +1,16 @@
> +2018-12-20  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.
> +
>  2018-12-20  Vladimir Makarov  <vmakarov@redhat.com>
>  
>  	PR target/88457
> diff --git a/gcc/ada/ChangeLog b/gcc/ada/ChangeLog
> index ba974cdcb03..c2f3d0db281 100644
> --- a/gcc/ada/ChangeLog
> +++ b/gcc/ada/ChangeLog
> @@ -1,3 +1,8 @@
> +2018-12-20  Martin Uecker  <martin.uecker@med.uni-goettingen.de>
> +
> +	* gcc-interface/trans.c (Attribute_to_gnu): Add check for
> +	flag_trampolines.
> +
>  2018-12-14  Eric Botcazou  <ebotcazou@adacore.com>
>  
>  	* gcc-interface/decl.c (rm_size): Take into account the padding in
> diff --git a/gcc/ada/gcc-interface/trans.c b/gcc/ada/gcc-interface/trans.c
> index 620dbd3d36d..ae4139e9b84 100644
> --- a/gcc/ada/gcc-interface/trans.c
> +++ b/gcc/ada/gcc-interface/trans.c
> @@ -2239,7 +2239,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
> @@ -5050,7 +5051,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 6e12dda2331..71443846799 100644
> --- a/gcc/c/ChangeLog
> +++ b/gcc/c/ChangeLog
> @@ -1,3 +1,9 @@
> +2018-12-20  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.
> +
>  2018-12-19  Segher Boessenkool  <segher@kernel.crashing.org>
>  
>  	* c-parser.c (c_parser_asm_statement) <RID_CONST, RID_RESTRICT>: Give
> diff --git a/gcc/c/c-typeck.c b/gcc/c/c-typeck.c
> index 1ae5ede81e6..6363dfda3b8 100644
> --- a/gcc/c/c-typeck.c
> +++ b/gcc/c/c-typeck.c
> @@ -1913,7 +1913,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
> @@ -3135,6 +3142,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 e3b4ef80e51..1b977c231ea 100644
> --- a/gcc/calls.c
> +++ b/gcc/calls.c
> @@ -230,7 +230,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 45d7f6189e5..d85d77b5b91 100644
> --- a/gcc/common.opt
> +++ b/gcc/common.opt
> @@ -2546,7 +2546,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 9035b333be8..66de347edef 100644
> --- a/gcc/defaults.h
> +++ b/gcc/defaults.h
> @@ -1050,7 +1050,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 ac2ee59d92c..5662abd1b97 100644
> --- a/gcc/doc/invoke.texi
> +++ b/gcc/doc/invoke.texi
> @@ -14030,9 +14030,10 @@ 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
> +@strong{Warning:} 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.
> diff --git a/gcc/doc/sourcebuild.texi b/gcc/doc/sourcebuild.texi
> index 29c693b9644..d645d1def92 100644
> --- a/gcc/doc/sourcebuild.texi
> +++ b/gcc/doc/sourcebuild.texi
> @@ -2162,6 +2162,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 33a4776b921..24f443bb26f 100644
> --- a/gcc/testsuite/ChangeLog
> +++ b/gcc/testsuite/ChangeLog
> @@ -1,3 +1,9 @@
> +2018-12-20  Martin Uecker  <martin.uecker@med.uni-goettingen.de>
> +
> +	* gcc.dg/trampoline-2.c: New test.
> +	* lib/target-supports.exp 
> +	(check_effective_target_notrampolines): New.
> +
>  2018-12-20  Vladimir Makarov  <vmakarov@redhat.com>
>  
>  	PR target/88457
> 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 7dec43233e0..43f9bb3891f 100644
> --- a/gcc/testsuite/lib/target-supports.exp
> +++ b/gcc/testsuite/lib/target-supports.exp
> @@ -924,6 +924,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 ab20cd98969..765ce347172 100644
> --- a/gcc/toplev.c
> +++ b/gcc/toplev.c
> @@ -1698,6 +1698,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 0ad469ada49..eb9bccb7a9d 100644
> --- a/gcc/tree-nested.c
> +++ b/gcc/tree-nested.c
> @@ -2544,7 +2544,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);

Re: [PING] [PATCH v5][C][ADA] use function descriptors instead of trampolines in C

[Jeff Law]

On 1/13/19 2:18 PM, Uecker, Martin wrote:
> 
> Does this patch have a change? This version seems risk-free and
> is a clear improvement from simply doing nothing for 
> '-fno-trampolines'. Also it is useful in situations where
> one cannot have an executable stack.
> 
> 
> I am currently thinking about working
> around this problem by calling nested functions with the
> following macro (x86_64 only):
I'm deferring to gcc-10.  We're well into stage4 and that's really where
my focus needs to be.  If another maintainer wants to push on these, I
won't object, but I won't be looking at this issue again until we're
into gcc-10 stage1 development.

Jeff

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

[Uecker, Martin]

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.

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

Re: [PATCH v6][C][ADA] use function descriptors instead of trampolines in C

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

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

Re: [PATCH v6][C][ADA] use function descriptors instead of trampolines in C

[Uecker, Martin]

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