Add a known bounds optimization to ASan.

This patch adds an optimization that makes ASan avoid adding checks to memory accesses that are known to use valid memory objects and are always in bounds.

[kcc2, 2012-10-18 07:49:47]

https://codereview.appspot.com/6643048/diff/2001/lib/Transforms/Instrumentati...
File lib/Transforms/Instrumentation/AddressSanitizer.cpp (right):

https://codereview.appspot.com/6643048/diff/2001/lib/Transforms/Instrumentati...
lib/Transforms/Instrumentation/AddressSanitizer.cpp:142: cl::Hidden,
cl::init(true));
make it false by default for now, please

https://codereview.appspot.com/6643048/diff/2001/lib/Transforms/Instrumentati...
lib/Transforms/Instrumentation/AddressSanitizer.cpp:333:
assert(getObjectSize(ObjPtr, ObjSize, TD, NULL, /*RoundToAlign=*/false));
I don't think you want to call something important from assert (assert is no-op
in release build)

https://codereview.appspot.com/6643048/diff/2001/lib/Transforms/Instrumentati...
lib/Transforms/Instrumentation/AddressSanitizer.cpp:358: if (ClOpt &&
ClOptKnownBounds) {
you can make it one if statement instead of two nested.

[glider, 2012-10-18 09:10:20]

https://codereview.appspot.com/6643048/diff/2001/test/Instrumentation/Address...
File
test/Instrumentation/AddressSanitizer/OptKnownBounds/double-loop-invalid-end-manual.ll
(right):

https://codereview.appspot.com/6643048/diff/2001/test/Instrumentation/Address...
test/Instrumentation/AddressSanitizer/OptKnownBounds/double-loop-invalid-end-manual.ll:3:
; RUN: opt < %s -asan -S -asan-opt=1 | FileCheck %s -check-prefix=OPT1
I suggest to explicitly pass the necessary flags here (such as
-asan-opt-known-bounds), so that the tests won't depend on the default values.

[otinn, 2012-10-18 10:05:09]

On 2012/10/18 07:49:47, kcc2 wrote:
>
https://codereview.appspot.com/6643048/diff/2001/lib/Transforms/Instrumentati...
> File lib/Transforms/Instrumentation/AddressSanitizer.cpp (right):
> 
>
https://codereview.appspot.com/6643048/diff/2001/lib/Transforms/Instrumentati...
> lib/Transforms/Instrumentation/AddressSanitizer.cpp:142: cl::Hidden,
> cl::init(true));
> make it false by default for now, please
> 
>
https://codereview.appspot.com/6643048/diff/2001/lib/Transforms/Instrumentati...
> lib/Transforms/Instrumentation/AddressSanitizer.cpp:333:
> assert(getObjectSize(ObjPtr, ObjSize, TD, NULL, /*RoundToAlign=*/false));
> I don't think you want to call something important from assert (assert is
no-op
> in release build)
> 
>
https://codereview.appspot.com/6643048/diff/2001/lib/Transforms/Instrumentati...
> lib/Transforms/Instrumentation/AddressSanitizer.cpp:358: if (ClOpt &&
> ClOptKnownBounds) {
> you can make it one if statement instead of two nested.

These problems should be fixed in the newest version.

[otinn, 2012-10-18 10:06:07]

On 2012/10/18 09:10:20, glider wrote:
>
https://codereview.appspot.com/6643048/diff/2001/test/Instrumentation/Address...
> File
>
test/Instrumentation/AddressSanitizer/OptKnownBounds/double-loop-invalid-end-manual.ll
> (right):
> 
>
https://codereview.appspot.com/6643048/diff/2001/test/Instrumentation/Address...
>
test/Instrumentation/AddressSanitizer/OptKnownBounds/double-loop-invalid-end-manual.ll:3:
> ; RUN: opt < %s -asan -S -asan-opt=1 | FileCheck %s -check-prefix=OPT1
> I suggest to explicitly pass the necessary flags here (such as
> -asan-opt-known-bounds), so that the tests won't depend on the default values.

This is done for all new tests in the newest version.

[samsonov, 2012-10-18 12:06:04]

On 2012/10/18 10:06:07, otinn wrote:
> On 2012/10/18 09:10:20, glider wrote:
> >
>
https://codereview.appspot.com/6643048/diff/2001/test/Instrumentation/Address...
> > File
> >
>
test/Instrumentation/AddressSanitizer/OptKnownBounds/double-loop-invalid-end-manual.ll
> > (right):
> > 
> >
>
https://codereview.appspot.com/6643048/diff/2001/test/Instrumentation/Address...
> >
>
test/Instrumentation/AddressSanitizer/OptKnownBounds/double-loop-invalid-end-manual.ll:3:
> > ; RUN: opt < %s -asan -S -asan-opt=1 | FileCheck %s -check-prefix=OPT1
> > I suggest to explicitly pass the necessary flags here (such as
> > -asan-opt-known-bounds), so that the tests won't depend on the default
values.
> 
> This is done for all new tests in the newest version.

Optionally, you may want to add a test for -asan-opt -asan-opt-known-bounds
relation (your optimization turns on iff both flags are set). Otherwise, this
looks good to me.

[otinn, 2012-10-18 14:25:21]

On 2012/10/18 12:06:04, samsonov wrote:
> On 2012/10/18 10:06:07, otinn wrote:
> > On 2012/10/18 09:10:20, glider wrote:
> > >
> >
>
https://codereview.appspot.com/6643048/diff/2001/test/Instrumentation/Address...
> > > File
> > >
> >
>
test/Instrumentation/AddressSanitizer/OptKnownBounds/double-loop-invalid-end-manual.ll
> > > (right):
> > > 
> > >
> >
>
https://codereview.appspot.com/6643048/diff/2001/test/Instrumentation/Address...
> > >
> >
>
test/Instrumentation/AddressSanitizer/OptKnownBounds/double-loop-invalid-end-manual.ll:3:
> > > ; RUN: opt < %s -asan -S -asan-opt=1 | FileCheck %s -check-prefix=OPT1
> > > I suggest to explicitly pass the necessary flags here (such as
> > > -asan-opt-known-bounds), so that the tests won't depend on the default
> values.
> > 
> > This is done for all new tests in the newest version.
> 
> Optionally, you may want to add a test for -asan-opt -asan-opt-known-bounds
> relation (your optimization turns on iff both flags are set). Otherwise, this
> looks good to me.
Added this check to the single-first-valid test.

[dan433584, 2012-10-18 17:34:32]

If you can limit your interest to constant offsets,
GetPointerBaseWithConstantOffset would be a lot simpler than using
ScalarEvolution.

Other than that, with a few comments, looks good.

https://codereview.appspot.com/6643048/diff/13001/lib/Transforms/Instrumentat...
File lib/Transforms/Instrumentation/AddressSanitizer.cpp (right):

https://codereview.appspot.com/6643048/diff/13001/lib/Transforms/Instrumentat...
lib/Transforms/Instrumentation/AddressSanitizer.cpp:329: if
(!isSimpleMemoryObject(ObjPtr))
It's not clear why this check is needed. Would it be sufficient to just be
prepared for getObjectSize (below) to return false?

https://codereview.appspot.com/6643048/diff/13001/lib/Transforms/Instrumentat...
lib/Transforms/Instrumentation/AddressSanitizer.cpp:341: Offset =
SE->getTruncateOrZeroExtend(Offset, ObjSizeSCEV->getType());
Is zero-extension correct here? What if the offset is negative? That would be an
out-of-bounds access, a case where you don't want to elide the checking. Also,
is it intended that a truncate be silently possible here?

[otinn, 2012-10-19 10:46:17]

On 2012/10/18 17:34:32, dan433584 wrote:
> If you can limit your interest to constant offsets,
> GetPointerBaseWithConstantOffset would be a lot simpler than using
> ScalarEvolution.
> 
> Other than that, with a few comments, looks good.
> 
>
https://codereview.appspot.com/6643048/diff/13001/lib/Transforms/Instrumentat...
> File lib/Transforms/Instrumentation/AddressSanitizer.cpp (right):
> 
>
https://codereview.appspot.com/6643048/diff/13001/lib/Transforms/Instrumentat...
> lib/Transforms/Instrumentation/AddressSanitizer.cpp:329: if
> (!isSimpleMemoryObject(ObjPtr))
> It's not clear why this check is needed. Would it be sufficient to just be
> prepared for getObjectSize (below) to return false?
There are some special cases where getObjectSize will return true and a non-zero
size for something that the current isSimpleMemoryObject returns false.

For example linking two files with the same name for different sized weak_odr
globals produces different results depending on the order. isSimpleMemoryObject
returns false in that case but getObjectSize does not.



>
https://codereview.appspot.com/6643048/diff/13001/lib/Transforms/Instrumentat...
> lib/Transforms/Instrumentation/AddressSanitizer.cpp:341: Offset =
> SE->getTruncateOrZeroExtend(Offset, ObjSizeSCEV->getType());
> Is zero-extension correct here? What if the offset is negative? That would be
an
> out-of-bounds access, a case where you don't want to elide the checking. Also,
> is it intended that a truncate be silently possible here?
I removed this conversion in the latest version because the original reason for
adding it is no longer possible.

[dan433584, 2012-10-19 12:55:20]

On Fri, Oct 19, 2012 at 3:46 AM, <llvm@otinn.com> wrote:

> On 2012/10/18 17:34:32, dan433584 wrote:
>
> https://codereview.appspot.**com/6643048/diff/13001/lib/**
>
Transforms/Instrumentation/**AddressSanitizer.cpp#**newcode329<https://codereview.appspot.com/6643048/diff/13001/lib/Transforms/Instrumentation/AddressSanitizer.cpp#newcode329>
>
>> lib/Transforms/**Instrumentation/**AddressSanitizer.cpp:329: if
>> (!isSimpleMemoryObject(ObjPtr)**)
>> It's not clear why this check is needed. Would it be sufficient to
>>
> just be
>
>> prepared for getObjectSize (below) to return false?
>>
> There are some special cases where getObjectSize will return true and a
> non-zero size for something that the current isSimpleMemoryObject
> returns false.
>
> For example linking two files with the same name for different sized
> weak_odr globals produces different results depending on the order.
> isSimpleMemoryObject returns false in that case but getObjectSize does
> not.


Wouldn't that be an ODR violation then?  Or is the idea that
AddressSanitizer should be paranoid and perform checking even (and perhaps
especially) in the case of an ODR violation? If that's the case, please add
a comment to that effect.

 lib/Transforms/Instrumentation/AddressSanitizer.cpp:341: Offset =
> SE->getTruncateOrZeroExtend(Offset, ObjSizeSCEV->getType());
> Is zero-extension correct here? What if the offset is negative? That
>
would be an

> out-of-bounds access, a case where you don't want to elide the
>
checking. Also,

> is it intended that a truncate be silently possible here?
>
I removed this conversion in the latest version because the original
> reason for adding it is no longer possible.
>

Ok.

Dan

[kcc1, 2012-10-19 14:01:56]

On Fri, Oct 19, 2012 at 4:55 PM, Dan Gohman <dan433584@gmail.com> wrote:

>
>
> On Fri, Oct 19, 2012 at 3:46 AM, <llvm@otinn.com> wrote:
>
>> On 2012/10/18 17:34:32, dan433584 wrote:
>>
>> https://codereview.appspot.**com/6643048/diff/13001/lib/**
>>
Transforms/Instrumentation/**AddressSanitizer.cpp#**newcode329<https://codereview.appspot.com/6643048/diff/13001/lib/Transforms/Instrumentation/AddressSanitizer.cpp#newcode329>
>>
>>> lib/Transforms/**Instrumentation/**AddressSanitizer.cpp:329: if
>>> (!isSimpleMemoryObject(ObjPtr)**)
>>> It's not clear why this check is needed. Would it be sufficient to
>>>
>> just be
>>
>>> prepared for getObjectSize (below) to return false?
>>>
>> There are some special cases where getObjectSize will return true and a
>> non-zero size for something that the current isSimpleMemoryObject
>> returns false.
>>
>> For example linking two files with the same name for different sized
>> weak_odr globals produces different results depending on the order.
>> isSimpleMemoryObject returns false in that case but getObjectSize does
>> not.
>
>
> Wouldn't that be an ODR violation then?  Or is the idea that
> AddressSanitizer should be paranoid and perform checking even (and perhaps
> especially) in the case of an ODR violation? If that's the case, please add
> a comment to that effect.
>

Yes, asan detects ODR violations. (did find quite a few already)


>
>
>  lib/Transforms/Instrumentation/AddressSanitizer.cpp:341: Offset =
>> SE->getTruncateOrZeroExtend(Offset, ObjSizeSCEV->getType());
>> Is zero-extension correct here? What if the offset is negative? That
>>
> would be an
>
>> out-of-bounds access, a case where you don't want to elide the
>>
> checking. Also,
>
>> is it intended that a truncate be silently possible here?
>>
> I removed this conversion in the latest version because the original
>> reason for adding it is no longer possible.
>>
>
> Ok.
>
> Dan
>
>

[otinn, 2012-10-19 14:28:00]

On 2012/10/19 12:55:20, dan433584 wrote:
> On Fri, Oct 19, 2012 at 3:46 AM, <mailto:llvm@otinn.com> wrote:
> 
> > On 2012/10/18 17:34:32, dan433584 wrote:
> >
> > https://codereview.appspot.**com/6643048/diff/13001/lib/**
> >
>
Transforms/Instrumentation/**AddressSanitizer.cpp#**newcode329<https://codereview.appspot.com/6643048/diff/13001/lib/Transforms/Instrumentation/AddressSanitizer.cpp#newcode329>
> >
> >> lib/Transforms/**Instrumentation/**AddressSanitizer.cpp:329: if
> >> (!isSimpleMemoryObject(ObjPtr)**)
> >> It's not clear why this check is needed. Would it be sufficient to
> >>
> > just be
> >
> >> prepared for getObjectSize (below) to return false?
> >>
> > There are some special cases where getObjectSize will return true and a
> > non-zero size for something that the current isSimpleMemoryObject
> > returns false.
> >
> > For example linking two files with the same name for different sized
> > weak_odr globals produces different results depending on the order.
> > isSimpleMemoryObject returns false in that case but getObjectSize does
> > not.
> 
> 
> Wouldn't that be an ODR violation then?  Or is the idea that
> AddressSanitizer should be paranoid and perform checking even (and perhaps
> especially) in the case of an ODR violation? If that's the case, please add
> a comment to that effect.
Added a comment about it to isSimpleMemoryObject.



>  lib/Transforms/Instrumentation/AddressSanitizer.cpp:341: Offset =
> > SE->getTruncateOrZeroExtend(Offset, ObjSizeSCEV->getType());
> > Is zero-extension correct here? What if the offset is negative? That
> >
> would be an
> 
> > out-of-bounds access, a case where you don't want to elide the
> >
> checking. Also,
> 
> > is it intended that a truncate be silently possible here?
> >
> I removed this conversion in the latest version because the original
> > reason for adding it is no longer possible.
> >
> 
> Ok.
> 
> Dan

[christianity.webb, 2019-01-05 23:12:26]

On 2012/10/19 14:28:00, otinn wrote:
> On 2012/10/19 12:55:20, dan433584 wrote:
> > On Fri, Oct 19, 2012 at 3:46 AM, <mailto:llvm@otinn.com> wrote:
> > 
> > > On 2012/10/18 17:34:32, dan433584 wrote:
> > >
> > > https://codereview.appspot.**com/6643048/diff/13001/lib/**
> > >
> >
>
Transforms/Instrumentation/**AddressSanitizer.cpp#**newcode329<https://codereview.appspot.com/6643048/diff/13001/lib/Transforms/Instrumentation/AddressSanitizer.cpp#newcode329>
> > >
> > >> lib/Transforms/**Instrumentation/**AddressSanitizer.cpp:329: if
> > >> (!isSimpleMemoryObject(ObjPtr)**)
> > >> It's not clear why this check is needed. Would it be sufficient to
> > >>
> > > just be
> > >
> > >> prepared for getObjectSize (below) to return false?
> > >>
> > > There are some special cases where getObjectSize will return true and a
> > > non-zero size for something that the current isSimpleMemoryObject
> > > returns false.
> > >
> > > For example linking two files with the same name for different sized
> > > weak_odr globals produces different results depending on the order.
> > > isSimpleMemoryObject returns false in that case but getObjectSize does
> > > not.
> > 
> > 
> > Wouldn't that be an ODR violation then?  Or is the idea that
> > AddressSanitizer should be paranoid and perform checking even (and perhaps
> > especially) in the case of an ODR violation? If that's the case, please add
> > a comment to that effect.
> Added a comment about it to isSimpleMemoryObject.
> 
> 
> 
> >  lib/Transforms/Instrumentation/AddressSanitizer.cpp:341: Offset =
> > > SE->getTruncateOrZeroExtend(Offset, ObjSizeSCEV->getType());
> > > Is zero-extension correct here? What if the offset is negative? That
> > >
> > would be an
> > 
> > > out-of-bounds access, a case where you don't want to elide the
> > >
> > checking. Also,
> > 
> > > is it intended that a truncate be silently possible here?
> > >
> > I removed this conversion in the latest version because the original
> > > reason for adding it is no longer possible.
> > >
> > 
> > Ok.
> > 
> > Dan
Dan ODR does not handle paranoria and that is the cause of false flags.  When
one extreme reaches another it correspondes and that is reason you end up with
information to help you understand that the Internet is safer than it is and
there will never be aday that anything bothersome to a person doesn't make
history, even if it takes up your OS for you computer space thanks