Updated patch according to Jeff Law's comments ( http://gcc.gnu.org/ml/gcc-patches/2013-05/msg00038.html )
Thanks,
H.
On Wed, Apr 17, 2013 at 11:40 AM, Han Shen(沈涵) <shen...@google.com> wrote:
> Thanks.
>
> On Wed, Apr 17, 2013 at 2:26 AM, Florian Weimer <fwei...@redhat.com> wrote:
>> On 04/17/2013 02:49 AM, Han Shen wrote:
>>>> Indentation is off (unless both mail clients I tried are clobbering your
>>>> patch). I think the GNU coding style prohibits the braces around the
>>>> single-statement body of the outer 'for".
>>>>
>>> Done with indentation properly on and removed the braces. (GMail
>>> composing window drops all the tabs when pasting... I have to use
>>> Thunderbird to paste the patch. Hope it is right this time)
>>
>>
>> Thunderbird mangles patches as well, but I was able to repair the damage.
>> When using Thunderbird, please send the patch as a text file attachment.
>> You can put the changelog snippets at the beginning of the file as well.
>> This way, everything is sent out unchanged.
>>
> Patch sent as attachment.
>>
>>
>>>> Can you make the conditional more similar to the comment, perhaps using a
>>>> switch statement on the value of the flag_stack_protect variable? That's
>>>> going to be much easier to read.
>>>>
>>> Re-coded. Now using 'switch-case'.
>>
>>
>> Thanks. I think the comment is now redundant because it matches the code
>> almost word-for-word. 8-)
>>
> Comment deleted.
>>
>>> No for 'struct-returning' functions. But I regard this not an issue ---
>>> at the programming level, there is no way to get one's hand on the
>>> address of a returned structure ---
>>> struct Node foo();
>>> struct Node *p = &foo(); // compiler error - lvalue required as
>>> unary '&' operand.
>>
>>
>> C++ const references can bind to rvalues.
>> 2013-04-11 Jakub Jelinek <ja...@redhat.com>
>> But I'm more worried about the interaction with the return value
>> optimization. Consider this C++ code:
>>
>> struct S {
>> S();
>> int a;
>> int b;
>> int c;
>> int d;
>> int e;
>> };
>>
>> void f1(int *);
>>
>> S f2()
>> {
>> S s;
>> f1(&s.a);
>> return s;
>> }
>>
>> S g2();
>>
>> void g3()
>> {
>> S s = g2();
>> }
>>
>> void g3b(const S&);
>>
>> void g3b()
>> {
>> g3b(g2());
>> }
>>
>> With your patch and -O2 -fstack-protector-strong, this generates the
>> following assembly:
>>
>> .globl _Z2f2v
>> .type _Z2f2v, @function
>> _Z2f2v:
>> .LFB0:
>> .cfi_startproc
>> pushq %rbx
>> .cfi_def_cfa_offset 16
>> .cfi_offset 3, -16
>> movq %rdi, %rbx
>> call _ZN1SC1Ev
>> movq %rbx, %rdi
>> call _Z2f1Pi
>> movq %rbx, %rax
>> popq %rbx
>> .cfi_def_cfa_offset 8
>> ret
>> .cfi_endproc
>> .LFE0:
>> .size _Z2f2v, .-_Z2f2v
>> .p2align 4,,15
>> .globl _Z2g3v
>> .type _Z2g3v, @function
>> _Z2g3v:
>> .LFB1:
>> .cfi_startproc
>> subq $40, %rsp
>> .cfi_def_cfa_offset 48
>> movq %rsp, %rdi
>> call _Z2g2v
>> addq $40, %rsp
>> .cfi_def_cfa_offset 8
>> ret
>> .cfi_endproc
>> .LFE1:
>> .size _Z2g3v, .-_Z2g3v
>> .p2align 4,,15
>> .globl _Z3g3bv
>> .type _Z3g3bv, @function
>> _Z3g3bv:
>> .LFB2:
>> .cfi_startproc
>> subq $40, %rsp
>> .cfi_def_cfa_offset 48
>> movq %rsp, %rdi
>> movq %fs:40, %rax
>> movq %rax, 24(%rsp)
>> xorl %eax, %eax
>> call _Z2g2v
>> movq %rsp, %rdi
>> call _Z3g3bRK1S
>> movq 24(%rsp), %rax
>> xorq %fs:40, %rax
>> jne .L9
>> addq $40, %rsp
>> .cfi_remember_state
>> .cfi_def_cfa_offset 8
>> ret
>> .L9:
>> .cfi_restore_state
>> .p2align 4,,6
>> call __stack_chk_fail
>> .cfi_endproc
>> .LFE2:
>> .size _Z3g3bv, .-_Z3g3bv
>>
>> Here, g3b() is correctly instrumented, and f2() does not need
>> instrumentation (because space for the returned object is not part of the
>> local frame). But an address on the stack escapes in g3() and is used for
>> the return value of the call to g2(). This requires instrumentation, which
>> is missing in this example.
>>
>> I suppose this can be handled in a follow-up patch if necessary.
>>
> Thanks for the case. Yeah, I see the problem here - in cfgexpand phase
> - where functions being scanned for stack protection - the tree
> representation has no indication that a local address is computed,
> just as listed below -
> void g3() ()
> {
> struct S s;
> <bb 2>:
> s = g2 (); [return slot optimization]
> s ={v} {CLOBBER};
> return;
> }
> One solution might be to scan for the "[return slot optimization]" tag
> in the tree. I'll post later another patch to address this.
>>
>>> ChangeLog and patch below --
>>>
>>> gcc/ChangeLog
>>> 2013-04-16 Han Shen <shen...@google.com>
>>> * cfgexpand.c (record_or_union_type_has_array_p): Helper function
>>> to check if a record or union contains an array field.
>>
>>
>> I think the GNU convention is to write only this:
>>
>> * cfgexpand.c (record_or_union_type_has_array_p): New function.
> Done.
>>
>>
>>> (expand_used_vars): Add logic handling '-fstack-protector-strong'.
>>> * common.opt (fstack-protector-all): New option.
>>
>>
>> Should be "fstack-protector-strong".
> Done.
>>
>>
>> --
>> Florian Weimer / Red Hat Product Security Team
>
> Patch attached as plain txt.
>
> Thanks,
> H.
fstack-protector-strong.patch
Description: Binary data
