[llvm-commits] [PATCH] Disable an ASan-unsafe optimization with address_safety attribute

[Evgeniy Stepanov]

On Thu, Mar 1, 2012 at 12:46 PM, Nick Lewycky <nicholas at mxc.ca> wrote:
> Evgeniy Stepanov wrote:
>>
>> Right. Thanks for noticing :)
>
>
> Index: test/Transforms/InstCombine/address-safety.ll
> ===================================================================
> --- test/Transforms/InstCombine/address-safety.ll       (revision 0)
> +++ test/Transforms/InstCombine/address-safety.ll       (revision 0)
> @@ -0,0 +1,32 @@
> +; RUN: opt -instcombine -S < %s | FileCheck %s
> +; http://llvm.org/bugs/show_bug.cgi?id=12047
>
> The format for the bug line is "; PR12047" and the test runner will (used
> to?) detect that and print that bug number when the test failed. Note that
> llvm.org/PR12047 gets you to the bug quickly.
>
> +%struct.B = type { i32 }
> +%struct.A = type { i8*, i8*, i8* }
> +%struct.S = type { %struct.A, %struct.B }
>
> I just want to make sure I understand this. Your test indicates a machine
> with 32-bit pointers. %struct.A will be padded out to 4x ptrsize, but inside
> %struct.S, %struct.B could fit inside struct.A's padding. Is that what's
> going on?
>
> Your comment is straight-forward:
>
> +    // If the cast type has alloc size larger than store size, uses of the
> new
> +    // alloc may end up accessing the alignment padding. This is ok in a
> regular
> +    // build, but Address Safety analysis tools may report false warnings.
>
> but I don't understand how it matches the test. Instcombine transforms the
> alloca %struct.S into an alloca i96, which is only 3x ptrsize. Is that
> transform *ever* legal? Wasn't the code accessing the i32 inside %struct.B
> legally before, and the illegally afterwards?

In this case, i96 has getTypeStoreSize() of 12 and getTypeAllocSize() of 16.

According to getTypeAllocSize docs, this is the guaranteed memory
block size that alloca will provide. Therefore, it seems that
accessing i32 in struct.B is legal, because it is still inside the
allocated area.

http://llvm.org/docs/doxygen/html/classllvm_1_1TargetData.html#abda1a3caac513012f2466fa50958e1e6

>
> (A related question for me is how does -scalarrepl handle the similar case,
> though it's hard to produce a testcase it doesn't just reduce away
> entirely.)
>
> Nick
>
>
>>
>> On Wed, Feb 29, 2012 at 4:27 PM, Jia Liu<proljc at gmail.com>  wrote:
>>>
>>> Hi Evgeniy,
>>>
>>> On Wed, Feb 29, 2012 at 8:15 PM, Evgeniy Stepanov
>>> <eugeni.stepanov at gmail.com>  wrote:
>>>>
>>>> Hi,
>>>>
>>>> please review this fix for bug 12047.
>>>>
>>>> One of InstCombine optimizations can replace an allocation of type T
>>>> with an allocation of type S, where S has a smaller storage size, but
>>>> the same allocation size. This is ok for regular builds, but
>>>> incompatible with address safety analysis tools like AddressSanitizer.
>>>>
>>>> This patch disables this optimization in functions with address_safety
>>>> attribute.
>>>
>>>
>>> Did you forgot patch in your mail?
>>>
>>>> _______________________________________________
>>>> llvm-commits mailing list
>>>> llvm-commits at cs.uiuc.edu
>>>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>>>>
>>>>
>>>> _______________________________________________
>>>> llvm-commits mailing list
>>>> llvm-commits at cs.uiuc.edu
>>>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
>