Re: [asan] Protection of stack vars

[Xinliang David Li <davidxl at google dot com>]

On Fri, Oct 12, 2012 at 11:10 AM, Jakub Jelinek <jakub@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