Message ID | 20200127174923.GA21375@localhost.localdomain |
---|---|
State | New |
Headers | show |
Series | Return slot optimization for stack protector strong | expand |
On Mon, Jan 27, 2020 at 06:49:23PM +0100, Stefan Schulze Frielinghaus wrote: > some function calls trigger the stack-protector-strong although such > calls are later on implemented via calls to internal functions. > Consider the following example: > > long double > rintl_wrapper (long double x) > { > return rintl (x); > } > > On s390x a return value of type `long double` is passed via a return > slot. Thus according to function `stack_protect_return_slot_p` a > function call like `rintl (x)` triggers the stack-protector-strong since > rintl is not an internal function. However, in a later stage, during > `expand_call_stmt`, such a call is implemented via a call to an internal > function. This means in the example, the call `rintl (x)` is expanded > into an assembler instruction with register operands only. Thus this > late time decision renders the usage of the stack protector superfluous. I doubt your predicate gives any guarantees that the builtin will be expanded inline rather than a library call. Some builtins might be expanded inline or as a library call depending on various options, or depending on particular arguments etc. Jakub
On Mon, Jan 27, 2020 at 06:53:51PM +0100, Jakub Jelinek wrote: > On Mon, Jan 27, 2020 at 06:49:23PM +0100, Stefan Schulze Frielinghaus wrote: > > some function calls trigger the stack-protector-strong although such > > calls are later on implemented via calls to internal functions. > > Consider the following example: > > > > long double > > rintl_wrapper (long double x) > > { > > return rintl (x); > > } > > > > On s390x a return value of type `long double` is passed via a return > > slot. Thus according to function `stack_protect_return_slot_p` a > > function call like `rintl (x)` triggers the stack-protector-strong since > > rintl is not an internal function. However, in a later stage, during > > `expand_call_stmt`, such a call is implemented via a call to an internal > > function. This means in the example, the call `rintl (x)` is expanded > > into an assembler instruction with register operands only. Thus this > > late time decision renders the usage of the stack protector superfluous. > > I doubt your predicate gives any guarantees that the builtin will be > expanded inline rather than a library call. Some builtins might be expanded > inline or as a library call depending on various options, or depending on > particular arguments etc. My predicate is more or less just a copy of what happens in `expand_call_stmt` where we have decl = gimple_call_fndecl (stmt); if (gimple_call_lhs (stmt) && !gimple_has_side_effects (stmt) && (optimize || (decl && called_as_built_in (decl)))) { internal_fn ifn = replacement_internal_fn (stmt); if (ifn != IFN_LAST) { expand_internal_call (ifn, stmt); return; } } There a decision is made whether a call is implemented as a call to an internal function or not. Thus using the very same logic it should be possible to decide at an earlier stage that a call will be implemented as a call to an internal function. Since an internal function has no linkage, it is therefore guaranteed that it will be inlined. Do I miss something? Cheers, Stefan
On Tue, Jan 28, 2020 at 06:18:41PM +0100, Stefan Schulze Frielinghaus wrote: > On Mon, Jan 27, 2020 at 06:53:51PM +0100, Jakub Jelinek wrote: > > On Mon, Jan 27, 2020 at 06:49:23PM +0100, Stefan Schulze Frielinghaus wrote: > > > some function calls trigger the stack-protector-strong although such > > > calls are later on implemented via calls to internal functions. > > > Consider the following example: > > > > > > long double > > > rintl_wrapper (long double x) > > > { > > > return rintl (x); > > > } > > > > > > On s390x a return value of type `long double` is passed via a return > > > slot. Thus according to function `stack_protect_return_slot_p` a > > > function call like `rintl (x)` triggers the stack-protector-strong since > > > rintl is not an internal function. However, in a later stage, during > > > `expand_call_stmt`, such a call is implemented via a call to an internal > > > function. This means in the example, the call `rintl (x)` is expanded > > > into an assembler instruction with register operands only. Thus this > > > late time decision renders the usage of the stack protector superfluous. > > > > I doubt your predicate gives any guarantees that the builtin will be > > expanded inline rather than a library call. Some builtins might be expanded > > inline or as a library call depending on various options, or depending on > > particular arguments etc. > > My predicate is more or less just a copy of what happens in > `expand_call_stmt` where we have > > decl = gimple_call_fndecl (stmt); > if (gimple_call_lhs (stmt) > && !gimple_has_side_effects (stmt) > && (optimize || (decl && called_as_built_in (decl)))) > { > internal_fn ifn = replacement_internal_fn (stmt); > if (ifn != IFN_LAST) > { > expand_internal_call (ifn, stmt); > return; > } > } > > There a decision is made whether a call is implemented as a call to an > internal function or not. Thus using the very same logic it should be > possible to decide at an earlier stage that a call will be implemented > as a call to an internal function. Since an internal function has no > linkage, it is therefore guaranteed that it will be inlined. Ping. Any chance we can have a second look at this? I just outsourced the logic used in `expand_call_stmt` in order to determine whether a call is realized as a call to an internal function or not, into a predicate. This predicate I'm then using to decide whether a function call should trigger the stack protector or not. I would have thought that this is fine since internal functions are guaranteed to be inlined. Am I missing something?
On Mon, Jan 27, 2020 at 06:53:51PM +0100, Jakub Jelinek wrote: > On Mon, Jan 27, 2020 at 06:49:23PM +0100, Stefan Schulze Frielinghaus wrote: > > some function calls trigger the stack-protector-strong although such > > calls are later on implemented via calls to internal functions. > > Consider the following example: > > > > long double > > rintl_wrapper (long double x) > > { > > return rintl (x); > > } > > > > On s390x a return value of type `long double` is passed via a return > > slot. Thus according to function `stack_protect_return_slot_p` a > > function call like `rintl (x)` triggers the stack-protector-strong since > > rintl is not an internal function. However, in a later stage, during > > `expand_call_stmt`, such a call is implemented via a call to an internal > > function. This means in the example, the call `rintl (x)` is expanded > > into an assembler instruction with register operands only. Thus this > > late time decision renders the usage of the stack protector superfluous. > > I doubt your predicate gives any guarantees that the builtin will be > expanded inline rather than a library call. Some builtins might be expanded > inline or as a library call depending on various options, or depending on > particular arguments etc. I'm performing the very same check in stack_protect_return_slot_p as done in expand_call_stmt. In the latter we check whether a call to a builtin function can be realized as a call to an internal function. My understanding of internal functions is that they have no linkage and are therefore guaranteed to be inlined. Do I miss something, or where do you see a potential problem which I could try to investigate? Cheers, Stefan
diff --git a/gcc/cfgexpand.c b/gcc/cfgexpand.c index 9864e4344d2..452813efef1 100644 --- a/gcc/cfgexpand.c +++ b/gcc/cfgexpand.c @@ -2003,6 +2003,20 @@ estimated_stack_frame_size (struct cgraph_node *node) return estimated_poly_value (size); } +/* Return true, if CALL is a call to a BUILT_IN_NORMAL function which has no + * other effect than setting the lhs and which could be replaced on the current + * target by a call to an internal function. Otherwise, return false. */ + +static bool +builtin_as_internal_p (gcall *call) +{ + tree decl = gimple_call_fndecl (call); + return (gimple_call_lhs (call) + && !gimple_has_side_effects (call) + && (optimize || (decl && called_as_built_in (decl))) + && replacement_internal_fn (call) != IFN_LAST); +} + /* Check if the current function has calls that use a return slot. */ static bool @@ -2018,7 +2032,8 @@ stack_protect_return_slot_p () /* This assumes that calls to internal-only functions never use a return slot. */ if (is_gimple_call (stmt) - && !gimple_call_internal_p (stmt) + && !(gimple_call_internal_p (stmt) + || builtin_as_internal_p (as_a <gcall *> (stmt))) && aggregate_value_p (TREE_TYPE (gimple_call_fntype (stmt)), gimple_call_fndecl (stmt))) return true; @@ -2613,25 +2628,19 @@ expand_call_stmt (gcall *stmt) return; } - /* If this is a call to a built-in function and it has no effect other - than setting the lhs, try to implement it using an internal function - instead. */ - decl = gimple_call_fndecl (stmt); - if (gimple_call_lhs (stmt) - && !gimple_has_side_effects (stmt) - && (optimize || (decl && called_as_built_in (decl)))) + /* If this is a call to a built-in function which could be implemented as a + * call to an internal function for the current target, then do so. */ + if (builtin_as_internal_p (stmt)) { internal_fn ifn = replacement_internal_fn (stmt); - if (ifn != IFN_LAST) - { - expand_internal_call (ifn, stmt); - return; - } + expand_internal_call (ifn, stmt); + return; } exp = build_vl_exp (CALL_EXPR, gimple_call_num_args (stmt) + 3); CALL_EXPR_FN (exp) = gimple_call_fn (stmt); + decl = gimple_call_fndecl (stmt); builtin_p = decl && fndecl_built_in_p (decl); /* If this is not a builtin function, the function type through which the diff --git a/gcc/testsuite/gcc.target/s390/ssp-builtin-return-slot.c b/gcc/testsuite/gcc.target/s390/ssp-builtin-return-slot.c new file mode 100644 index 00000000000..a3c5eac7065 --- /dev/null +++ b/gcc/testsuite/gcc.target/s390/ssp-builtin-return-slot.c @@ -0,0 +1,39 @@ +/* Check that we do not trigger the Stack Smashing Protector unnecessarily. + * Some built-ins as e.g. rintl make use of a return slot and are therefore + * subject to stack protection. However, some of those built-ins (including + * rintl) are finally implemented via internal functions which should not + * trigger the Stack Smashing Protector Strong. */ + +/* { dg-do compile } */ +/* { dg-options "-O1 -march=zEC12 -fstack-protector-strong" } */ +/* { dg-final { scan-assembler-not "brasl\t%r14,__stack_chk_fail" } } */ + +long double ceill(long double x); +long double copysignl(long double x, long double y); +long double floorl(long double x); +long double nearbyintl(long double x); +long double rintl(long double x); +long double roundl(long double x); +long double truncl(long double x); + +#define UNARY(F) \ + long double F ## _wrapper(long double x) \ + { return F(x); } \ + long double F ## _wrapper_builtin(long double x) \ + { return __builtin_ ## F(x); } + +#define BINARY(F) \ + long double F ## _wrapper(long double x, long double y) \ + { return F(x, y); } \ + long double F ## _wrapper_builtin(long double x, long double y) \ + { return __builtin_ ## F(x, y); } + +UNARY(ceill) +UNARY(floorl) +UNARY(nearbyintl) +UNARY(rintl) +UNARY(roundl) +UNARY(truncl) + +BINARY(copysignl) +