llvm dev - Dec 2011 - [LLVMdev] load widening conflicts with AddressSanitizer

On Dec 16, 2011, at 2:27 PM, Kostya Serebryany wrote:
> This is a good question.  Would it be possible for ASan to do its
instrumentation earlier?
> 
> It would be possible but undesirable. 
> First, asan blows up the IR and running asan early will increase the
compile-time.
> Second, asan greatly benefits from all optimizations running before it
because it needs to instrument less memory accesses.
> It actually benefits from load widening too: in the test case above asan
instruments only one load instead of two.
You certainly wouldn't want to run asan before mem2reg/SRoA, but after that,
the benefits are probably small.  I'd guess that there is some non-zero
value to exposing the code generated by asan to the optimizer.  Have you looked
at that at all?
> In this case, we have an array of 22 bytes which is 16-aligned. 
> I suspect that load widening changed the alignment of alloca instruction to
make the transformation legal. Right?
> Can we change the load widening algorithm to also change the size of alloca
instruction to be dividable by 16?
> This will solve the problem, at least the variant I observe now. 
I believe it is 16-byte aligned based on ABI requirements for x86-64, though
you're right that the optimizer will increase alignment in other cases.  In
any case, we don't want to increase the size of the object, because that
would prevent packing some other data in after it.  For example, a 2-byte
aligned 10 byte object can be placed after it in memory if we keep it 22-bytes
in size.

-Chris

-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20111216/801b6ac8/attachment.html>

On 12/16/11 4:35 PM, Chris Lattner wrote:
> On Dec 16, 2011, at 2:27 PM, Kostya Serebryany wrote:
>>
>>     This is a good question.  Would it be possible for ASan to do its
>>     instrumentation earlier?
>>
>>
>> It would be possible but undesirable.
>> First, asan blows up the IR and running asan early will increase the 
>> compile-time.
>> Second, asan greatly benefits from all optimizations running before 
>> it because it needs to instrument less memory accesses.
>> It actually benefits from load widening too: in the test case above 
>> asan instruments only one load instead of two.
>
> You certainly wouldn't want to run asan before mem2reg/SRoA, but after 
> that, the benefits are probably small.  I'd guess that there is some 
> non-zero value to exposing the code generated by asan to the 
> optimizer.  Have you looked at that at all?
>
>> In this case, we have an array of 22 bytes which is 16-aligned.
>> I suspect that load widening changed the alignment of alloca 
>> instruction to make the transformation legal. Right?
>> Can we change the load widening algorithm to also change the size of 
>> alloca instruction to be dividable by 16?
>> This will solve the problem, at least the variant I observe now.
>
> I believe it is 16-byte aligned based on ABI requirements for x86-64, 
> though you're right that the optimizer will increase alignment in 
> other cases.  In any case, we don't want to increase the size of the 
> object, because that would prevent packing some other data in after 
> it.  For example, a 2-byte aligned 10 byte object can be placed after 
> it in memory if we keep it 22-bytes in size.
It seems that another option would be to check that both the alignment 
*and* size of the memory object permit safe load-widening and to not 
perform it on memory objects such as this 22-byte object.

Do you think such unsafe cases occur often in practice?

-- John T.
>
> -Chris
>
>
>
> _______________________________________________
> LLVM Developers mailing list
> LLVMdev at cs.uiuc.edu         http://llvm.cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20111216/e6f74818/attachment.html>

On Dec 16, 2011, at 2:45 PM, John Criswell wrote:
>> I believe it is 16-byte aligned based on ABI requirements for x86-64,
though you're right that the optimizer will increase alignment in other
cases.  In any case, we don't want to increase the size of the object,
because that would prevent packing some other data in after it.  For example, a
2-byte aligned 10 byte object can be placed after it in memory if we keep it
22-bytes in size.
> 
> It seems that another option would be to check that both the alignment
*and* size of the memory object permit safe load-widening and to not perform it
on memory objects such as this 22-byte object.
> 
> Do you think such unsafe cases occur often in practice?
I'm not sure what "unsafe" means in this situation, it's
clearly safe.  It also clearly happens in practice if it's causing you guys
heartburn. :)

-Chris

On Fri, Dec 16, 2011 at 2:35 PM, Chris Lattner <clattner at apple.com>
wrote:
> On Dec 16, 2011, at 2:27 PM, Kostya Serebryany wrote:
>
> This is a good question.  Would it be possible for ASan to do its
>> instrumentation earlier?
>>
>
> It would be possible but undesirable.
> First, asan blows up the IR and running asan early will increase the
> compile-time.
> Second, asan greatly benefits from all optimizations running before it
> because it needs to instrument less memory accesses.
> It actually benefits from load widening too: in the test case above asan
> instruments only one load instead of two.
>
>
> You certainly wouldn't want to run asan before mem2reg/SRoA, but after
> that, the benefits are probably small.  I'd guess that there is some
> non-zero value to exposing the code generated by asan to the optimizer.
>  Have you looked at that at all?
>
This is the usual phase ordering problem.
If asan is done early, you get all of the optimizations cleaning up after
asan.
If you run asan late, you instrument a cleaner IR.
Asan should run after the loop invariant loads are hoisted up, common
subexpressions eliminated, loads widened/combined, dead stores eliminated,
memsets merged, etc.
Otherwise the optimizer will have a hard time optimizing the original code
*and* the instrumentation.

I have not looked into placing asan somewhere else in LLVM (this is in my
TODO list somewhere at the bottom).
But I had to place asan early in gcc (before the loop optimizations) and it
was a disaster (4x compile-time slowdown, 20% poorer run-time performance).
This is not necessary relevant to LLVM of course.


>
> In this case, we have an array of 22 bytes which is 16-aligned.
> I suspect that load widening changed the alignment of alloca instruction
> to make the transformation legal. Right?
> Can we change the load widening algorithm to also change the size of
> alloca instruction to be dividable by 16?
> This will solve the problem, at least the variant I observe now.
>
>
> I believe it is 16-byte aligned based on ABI requirements for x86-64,
> though you're right that the optimizer will increase alignment in other
> cases.  In any case, we don't want to increase the size of the object,
> because that would prevent packing some other data in after it.  For
> example, a 2-byte aligned 10 byte object can be placed after it in memory
> if we keep it 22-bytes in size.
>
ok.

I wonder if the load widening can attach some metadata to the stack
objects, accesses to which it has modified?
Then asan will increase the alloca size as appropriate (it does it anyway).

Why you don't like the idea to disable or restrict the widening when asan
is on?

--kcc


>
> -Chris
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20111216/ae0563a4/attachment.html>

On Fri, Dec 16, 2011 at 6:08 PM, Kostya Serebryany <kcc at google.com>
wrote:
>  In this case, we have an array of 22 bytes which is 16-aligned.
>> I suspect that load widening changed the alignment of alloca
instruction
>> to make the transformation legal. Right?
>> Can we change the load widening algorithm to also change the size of
>> alloca instruction to be dividable by 16?
>> This will solve the problem, at least the variant I observe now.
>>
>>
>> I believe it is 16-byte aligned based on ABI requirements for x86-64,
>> though you're right that the optimizer will increase alignment in
other
>> cases.  In any case, we don't want to increase the size of the
object,
>> because that would prevent packing some other data in after it.  For
>> example, a 2-byte aligned 10 byte object can be placed after it in
memory
>> if we keep it 22-bytes in size.
>>
>
> ok.
>
> I wonder if the load widening can attach some metadata to the stack
> objects, accesses to which it has modified?
> Then asan will increase the alloca size as appropriate (it does it anyway).
>
>
This doesn't seem like a general solution.  What if there is some heap
allocated buffer proven to be 16 byte aligned, but 22 bytes long?  Load
widening may occur, but you won't be able to fudge the size.

> Why you don't like the idea to disable or restrict the widening when
asan
> is on?
>
I agree this seems reasonable.

Reid
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20111216/61e3d25f/attachment.html>

I'm not opposed to disabling this transformation when asan is on, we just
need a clean way to express this in the IR.

-Chris

On Dec 16, 2011, at 3:08 PM, Kostya Serebryany <kcc at google.com> wrote:
> Why you don't like the idea to disable or restrict the widening when
asan is on?
> 
> --kcc