On Fri, Oct 12, 2012 at 11:10 AM, Jakub Jelinek <ja...@redhat.com> wrote:
> On Fri, Oct 12, 2012 at 10:52:04AM -0700, Xinliang David Li wrote:
>> This is related to the way how you implement it. Emitting the stack
>> shadow initialization code in GIMPLE would solve the problem. I think
>> that would be cleaner.
>
> You'd need to duplicate all the stack slot sharing code, or adjust it so
> that it would be usable for both RTL and GIMPLE. IMHO much uglier.
I don't think you can have stack slot sharing with tsan, can you?
Unless the shadow initialization/clearing code is inserted at the
entry and exit of each lexical scope.
>
> I don't see how it would solve the problem where to put the cleanup
> sequence. Either you'd put it before tail calls and allow them to be
> tail call optimized (but you don't know at the GIMPLE level whether
> they will be or won't be tail call optimized), or you'd just put them
> after the calls, but then they wouldn't be tail call optimized ever.
Yes -- that is more natural way to disable tail call other than
checking the tsan flag :).
> It is really not a big deal to put the cleanup sequence somewhere,
> it is easy to disable tail call optimizations if var_end_seq is non-NULL,
> if we decide that is the way to go, or it is easy to put it before the
> calls.
> The reason I haven't implemented it is because I run out of hacking time for
> today.
>
>> > +static rtx
>> > +asan_shadow_cst (unsigned char shadow_bytes[4])
>>
>> Missing comments.
>
> Will add.
>>
>> > +{
>> > + int i;
>> > + unsigned HOST_WIDE_INT val = 0;
>> > + gcc_assert (WORDS_BIG_ENDIAN == BYTES_BIG_ENDIAN);
>> > + for (i = 0; i < 4; i++)
>> > + val |= (unsigned HOST_WIDE_INT) shadow_bytes[BYTES_BIG_ENDIAN ? 3 - i
>> > : i]
>> > + << (BITS_PER_UNIT * i);
>> > + return GEN_INT (trunc_int_for_mode (val, SImode));
>> > +}
>> > +
>> > +rtx
>> > +asan_emit_stack_protection (rtx base, HOST_WIDE_INT *offsets, tree *decls,
>> > + int length)
>>
>> Missing comments.
>
> Likewise.
>
>> See my previous comment -- having this in rtl form makes the code less
>> readable and less maintainable.
>
> I disagree about that. I know in LLVM asan is implemented as a single pass
> transforming everything at once, but I don't think that is the best thing to
> do in GCC, actually I think it is better to let the instrumentation phase
> just emit an artificial builtin call instead of the shadow mem address
> computation, read, test, and conditional call (e.g. because then it won't
> prevent vectorization), and either lower it at some later point during
> GIMPLE, or just expand it to RTL directly.
The builtin idea sounds good. Without that, splitting the tsan
instrumentation code in two places sounds unreasonable to me --
especially for high level transformation like this.
thanks,
David
>
>> > /* A subroutine of expand_used_vars. Give each partition representative
>> > a unique location within the stack frame. Update each partition member
>> > with that location. */
>> >
>> > static void
>> > -expand_stack_vars (bool (*pred) (tree))
>> > +expand_stack_vars (bool (*pred) (tree), VEC(HOST_WIDE_INT, heap)
>> > **asan_vec,
>> > + VEC(tree, heap) **asan_decl_vec)
>>
>>
>> With the current implementation, the interface change can also be
>> avoided --- can the asan shadow var info be stashed into 'struct
>> stack_var' ?
>
> Yeah, sure, it can be groupped into a single struct. But it is just
> an interface between a helper function and the caller anyway, the reason
> for the separate expand_stack_vars function is that it needs to be called
> up to 3 times for -fstack-protector support.
>
> Jakub
