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

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

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

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