Extend -fstack-protector-strong to cover calls with return slot

On 01/03/2014 10:43 PM, Florian Weimer wrote:

>> Lastly, I wonder if gimple_call_return_slot_opt_p is really what you are
>> after, why does NRV matter here?
>
> The C code we generate does not construct the returned value in place
> (presumably because the partial write would be visible with threads,
> longjmp etc.), unlike the C++ code.
>
> That's why I'm interested in instrumenting NRV-able calls only.  But
> gimple_call_return_slot_opt_p doesn't actually give me that.  The GIMPLE
> from the C test case looks like this (before and after applying your
> proposal):

I thought about this some more and I think it makes sense to add the 
instrumentation each time the return slot is used, both for C and C++. 
We don't if the called function is implemented in C or C++, so 
language-specific instrumentation is not entirely accurate.

I'm attaching a second version of the patch, splitting out the decl and 
bb analysis and using is_gimple_call.  Bootstrapped and 
regression-tested on x86_64-redhat-linux-gnu.

On Tue, Jan 07, 2014 at 01:51:00PM +0100, Florian Weimer wrote:
> +static bool
> +stack_protect_return_slot_p ()
> +{
> +  basic_block bb;
> +  
> +  FOR_ALL_BB_FN (bb, cfun)
> +    for (gimple_stmt_iterator gsi = gsi_start_bb (bb);
> +	 !gsi_end_p (gsi); gsi_next (&gsi))
> +      {
> +	gimple stmt = gsi_stmt (gsi);
> +	if (is_gimple_call (stmt) && gimple_call_return_slot_opt_p (stmt))

I have to repeat, this is not the right test, it really is just an
optimization hint, nothing else.  Just look where it is set (unless it is
C++ where some NRV is performed in the FE as mandated by the languages)
- in pass_return_slot, which is an optimization pass, not run with -O0 or
-Og at all.  So, you'd stack protect something only if not -O0 or -Og, that
is just wrong, and only if it was suitable for NRV.  You really want to
protect all functions that call something that is returned by hidden
reference.

	Jakub

On 01/07/2014 02:07 PM, Jakub Jelinek wrote:
> On Tue, Jan 07, 2014 at 01:51:00PM +0100, Florian Weimer wrote:
>> +static bool
>> +stack_protect_return_slot_p ()
>> +{
>> +  basic_block bb;
>> +
>> +  FOR_ALL_BB_FN (bb, cfun)
>> +    for (gimple_stmt_iterator gsi = gsi_start_bb (bb);
>> +	 !gsi_end_p (gsi); gsi_next (&gsi))
>> +      {
>> +	gimple stmt = gsi_stmt (gsi);
>> +	if (is_gimple_call (stmt) && gimple_call_return_slot_opt_p (stmt))
>
> I have to repeat, this is not the right test, it really is just an
> optimization hint, nothing else.  Just look where it is set (unless it is
> C++ where some NRV is performed in the FE as mandated by the languages)
> - in pass_return_slot, which is an optimization pass, not run with -O0 or
> -Og at all.

I don't understand.  The corresponding tree flag is set by 
gimplify_modify_expr_rhs, in the CALL_EXPR case:

	      if (use_target)
		{
		  CALL_EXPR_RETURN_SLOT_OPT (*from_p) = 1;
		  mark_addressable (*to_p);
		}

Isn't this an integral part of the gimplifier which also runs at -O0?  I 
certainly see the flag in the generated gimple at -O0, with the amended 
C test case:

foo11 ()
{
   int D.2037;
   struct B D.2038;

   D.2038 = global3 (); [return slot optimization]
   D.2037 = D.2038.a1;
   return D.2037;
}

As I said, I think the function gimple_call_return_slot_opt_p is 
misnamed because it's not actually an optimization.

On Tue, Jan 07, 2014 at 02:27:04PM +0100, Florian Weimer wrote:
> gimplify_modify_expr_rhs, in the CALL_EXPR case:
> 
> 	      if (use_target)
> 		{
> 		  CALL_EXPR_RETURN_SLOT_OPT (*from_p) = 1;
> 		  mark_addressable (*to_p);
> 		}

Yeah, that sets it in some cases too, not in other testcases.

Just look at how the flag is used when actually expanding it:

        if (target && MEM_P (target) && CALL_EXPR_RETURN_SLOT_OPT (exp))
          structure_value_addr = XEXP (target, 0);
        else
          {
            /* For variable-sized objects, we must be called with a target
               specified.  If we were to allocate space on the stack here,
               we would have no way of knowing when to free it.  */
            rtx d = assign_temp (rettype, 1, 1);
            structure_value_addr = XEXP (d, 0);
            target = 0;
          }

so, if it is set, the address of the var on the LHS is passed to the
function as hidden argument, if it is not set, we pass address of
a stack temporary instead.  Both the automatic var and the stack temporary
can overflow, if the callee does something wrong.

	Jakub