| 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) +
Hi, 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 am wondering if we can prevent this at least for calls to builtin functions which are finally implemented via calls to internal functions. The attached patch solves this for me. Any thoughts? Successfully bootstrapped and regtested on s390x. Cheers, Stefan