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

["Uecker, Martin" <Martin.Uecker@med.uni-goettingen.de>]

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