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

[Florian Weimer]

On 01/03/2014 07:57 PM, Jakub Jelinek wrote:

>> +/* Check if the basic block has a call which uses a return slot.  */
>> +
>> +static bool
>> +call_with_return_slot_opt_p (basic_block bb)
>> +{
>> +  for (gimple_stmt_iterator gsi = gsi_start_bb (bb);
>> +       !gsi_end_p (gsi); gsi_next (&gsi))
>> +    {
>> +      gimple stmt = gsi_stmt (gsi);
>> +      if (gimple_code (stmt) == GIMPLE_CALL
>
> That would be is_gimple_call (stmt) instead.

Ah, it's not used consistently everywhere, and I got it from of the 
leftover places.

> Also, I'd think the function is misnamed, given that it checks if there
> are any calls with return_slot_opt_p in a bb.  I think it would be
> better to move FOR_ALL_BB_FN (bb, cfun) loop also into the
> function and call it any_call_...

I should probably move both loops (the one for declarations and the one 
for basic blocks) into its own function.

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

foo11 ()
{
   int D.2265;
   struct B D.2266;

   D.2266 = global3 (); [return slot optimization]
   D.2265 = D.2266.a1;
   return D.2265;
}

In both cases, SSP instrumentation is applied:

         .type   foo11, @function
foo11:
.LFB24:
         .cfi_startproc
         subq    $56, %rsp
         .cfi_def_cfa_offset 64
         movq    %rsp, %rdi
         movq    %fs:40, %rax
         movq    %rax, 40(%rsp)
         xorl    %eax, %eax
         call    global3
         movq    40(%rsp), %rdx
         xorq    %fs:40, %rdx
         movl    (%rsp), %eax
         jne     .L50
         addq    $56, %rsp
         .cfi_remember_state
         .cfi_def_cfa_offset 8
         ret
.L50:
         .cfi_restore_state
         call    __stack_chk_fail
         .cfi_endproc

> Isn't what you are looking for instead
> whether the called function returns value through invisible reference,
> because then you'll always have some (aggregate) addressable object
> in the caller's frame and supposedly you are after making sure that the
> callee doesn't overflow the return object.
>
> So, looking at tree-nrv.c, that check would be roughly:
>            if (is_gimple_call (stmt)
>                && gimple_call_lhs (stmt)
>                && aggregate_value_p (TREE_TYPE (gimple_call_lhs (stmt)),
>                                      gimple_call_fndecl (stmt)))

When I do that, I get SSP instrumentation even when the struct is small 
enough to be returned in registers.  gimple_call_return_slot_opt_p 
returns false in this case.  So gimple_call_return_slot_opt_p appears to 
be misnamed (it's an ABI thing, not really an optimization), but it's 
closer to what I want.

-- 
Florian Weimer / Red Hat Product Security Team