Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(16)

Issue 6643048: Add a known bounds optimization to ASan.

Can't Edit
Can't Publish+Mail
Start Review
Created:
6 years, 3 months ago by otinn
Modified:
2 weeks, 2 days ago
Visibility:
Public.

Description

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.

Patch Set 1 #

Patch Set 2 : New version after ASan was converted to a function pass. #

Total comments: 4

Patch Set 3 : Fix the problems noted so far. #

Patch Set 4 : Check that the optimization is enabled if and only if both -asan-opt and -asan-opt-known-bounds areā€¦ #

Total comments: 2

Patch Set 5 : Remove the offset type conversion. #

Patch Set 6 : Add a comment about weak_odr in isSimpleMemoryObject. #

Patch Set 7 : Match the DataLayout getIntPtrType change. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+1418 lines, -1 line) Patch
M lib/Transforms/Instrumentation/AddressSanitizer.cpp View 1 2 3 4 5 6 8 chunks +78 lines, -0 lines 0 comments Download
A test/Instrumentation/AddressSanitizer/OptKnownBounds/double-loop-invalid-end.ll View 1 2 1 chunk +68 lines, -0 lines 0 comments Download
A test/Instrumentation/AddressSanitizer/OptKnownBounds/double-loop-invalid-end-manual.ll View 1 2 1 chunk +71 lines, -0 lines 0 comments Download
A test/Instrumentation/AddressSanitizer/OptKnownBounds/double-loop-invalid-single-end.ll View 1 2 1 chunk +68 lines, -0 lines 0 comments Download
A test/Instrumentation/AddressSanitizer/OptKnownBounds/double-loop-invalid-single-end-manual.ll View 1 2 1 chunk +71 lines, -0 lines 0 comments Download
A test/Instrumentation/AddressSanitizer/OptKnownBounds/double-loop-manual-valid.ll View 1 2 1 chunk +62 lines, -0 lines 0 comments Download
A test/Instrumentation/AddressSanitizer/OptKnownBounds/double-loop-valid.ll View 1 2 1 chunk +59 lines, -0 lines 0 comments Download
A test/Instrumentation/AddressSanitizer/OptKnownBounds/heap-free.ll View 1 2 1 chunk +42 lines, -0 lines 0 comments Download
A test/Instrumentation/AddressSanitizer/OptKnownBounds/heap-realloc.ll View 1 2 1 chunk +49 lines, -0 lines 0 comments Download
A test/Instrumentation/AddressSanitizer/OptKnownBounds/heap-unknown-function.ll View 1 2 1 chunk +51 lines, -0 lines 0 comments Download
A test/Instrumentation/AddressSanitizer/OptKnownBounds/loop-invalid-end-discountinuous.ll View 1 2 1 chunk +46 lines, -0 lines 0 comments Download
A test/Instrumentation/AddressSanitizer/OptKnownBounds/loop-invalid-end-heap.ll View 1 2 1 chunk +55 lines, -0 lines 0 comments Download
A test/Instrumentation/AddressSanitizer/OptKnownBounds/loop-invalid-end-linear-heap.ll View 1 2 1 chunk +57 lines, -0 lines 0 comments Download
A test/Instrumentation/AddressSanitizer/OptKnownBounds/loop-invalid-end-reverse.ll View 1 2 1 chunk +45 lines, -0 lines 0 comments Download
A test/Instrumentation/AddressSanitizer/OptKnownBounds/loop-invalid-start-heap.ll View 1 2 1 chunk +54 lines, -0 lines 0 comments Download
A test/Instrumentation/AddressSanitizer/OptKnownBounds/loop-invalid-start-linear-heap.ll View 1 2 1 chunk +57 lines, -0 lines 0 comments Download
A test/Instrumentation/AddressSanitizer/OptKnownBounds/loop-invalid-start-reverse.ll View 1 2 1 chunk +45 lines, -0 lines 0 comments Download
A test/Instrumentation/AddressSanitizer/OptKnownBounds/loop-valid-discountinuous.ll View 1 2 1 chunk +45 lines, -0 lines 0 comments Download
A test/Instrumentation/AddressSanitizer/OptKnownBounds/loop-valid-reverse.ll View 1 2 1 chunk +44 lines, -0 lines 0 comments Download
A test/Instrumentation/AddressSanitizer/OptKnownBounds/multi-obj-phi-select-invalid.ll View 1 2 1 chunk +48 lines, -0 lines 0 comments Download
A test/Instrumentation/AddressSanitizer/OptKnownBounds/multi-obj-select-invalid.ll View 1 2 1 chunk +43 lines, -0 lines 0 comments Download
A test/Instrumentation/AddressSanitizer/OptKnownBounds/single-after-end.ll View 1 2 1 chunk +30 lines, -0 lines 0 comments Download
A test/Instrumentation/AddressSanitizer/OptKnownBounds/single-before-start.ll View 1 2 1 chunk +30 lines, -0 lines 0 comments Download
A test/Instrumentation/AddressSanitizer/OptKnownBounds/single-first-valid.ll View 1 2 3 1 chunk +33 lines, -0 lines 0 comments Download
A test/Instrumentation/AddressSanitizer/OptKnownBounds/single-invalid-off-by-two.ll View 1 2 1 chunk +31 lines, -0 lines 0 comments Download
A test/Instrumentation/AddressSanitizer/OptKnownBounds/single-last-valid.ll View 1 2 1 chunk +29 lines, -0 lines 0 comments Download
A test/Instrumentation/AddressSanitizer/OptKnownBounds/single-very-large-valid.ll View 1 2 1 chunk +30 lines, -0 lines 0 comments Download
A test/Instrumentation/AddressSanitizer/OptKnownBounds/struct-byval.ll View 1 2 1 chunk +37 lines, -0 lines 0 comments Download
A test/Instrumentation/AddressSanitizer/OptKnownBounds/struct-ptr.ll View 1 2 1 chunk +39 lines, -0 lines 0 comments Download
M test/Instrumentation/AddressSanitizer/asan-vs-gvn.ll View 1 chunk +1 line, -1 line 0 comments Download

Messages

Total messages: 12
kcc2
https://codereview.appspot.com/6643048/diff/2001/lib/Transforms/Instrumentation/AddressSanitizer.cpp File lib/Transforms/Instrumentation/AddressSanitizer.cpp (right): https://codereview.appspot.com/6643048/diff/2001/lib/Transforms/Instrumentation/AddressSanitizer.cpp#newcode142 lib/Transforms/Instrumentation/AddressSanitizer.cpp:142: cl::Hidden, cl::init(true)); make it false by default for now, ...
6 years, 3 months ago (2012-10-18 07:49:47 UTC) #1
glider
https://codereview.appspot.com/6643048/diff/2001/test/Instrumentation/AddressSanitizer/OptKnownBounds/double-loop-invalid-end-manual.ll File test/Instrumentation/AddressSanitizer/OptKnownBounds/double-loop-invalid-end-manual.ll (right): https://codereview.appspot.com/6643048/diff/2001/test/Instrumentation/AddressSanitizer/OptKnownBounds/double-loop-invalid-end-manual.ll#newcode3 test/Instrumentation/AddressSanitizer/OptKnownBounds/double-loop-invalid-end-manual.ll:3: ; RUN: opt < %s -asan -S -asan-opt=1 | ...
6 years, 3 months ago (2012-10-18 09:10:20 UTC) #2
otinn
On 2012/10/18 07:49:47, kcc2 wrote: > https://codereview.appspot.com/6643048/diff/2001/lib/Transforms/Instrumentation/AddressSanitizer.cpp > File lib/Transforms/Instrumentation/AddressSanitizer.cpp (right): > > https://codereview.appspot.com/6643048/diff/2001/lib/Transforms/Instrumentation/AddressSanitizer.cpp#newcode142 > ...
6 years, 3 months ago (2012-10-18 10:05:09 UTC) #3
otinn
On 2012/10/18 09:10:20, glider wrote: > https://codereview.appspot.com/6643048/diff/2001/test/Instrumentation/AddressSanitizer/OptKnownBounds/double-loop-invalid-end-manual.ll > File > test/Instrumentation/AddressSanitizer/OptKnownBounds/double-loop-invalid-end-manual.ll > (right): > > ...
6 years, 3 months ago (2012-10-18 10:06:07 UTC) #4
samsonov
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/AddressSanitizer/OptKnownBounds/double-loop-invalid-end-manual.ll ...
6 years, 3 months ago (2012-10-18 12:06:04 UTC) #5
otinn
On 2012/10/18 12:06:04, samsonov wrote: > On 2012/10/18 10:06:07, otinn wrote: > > On 2012/10/18 ...
6 years, 3 months ago (2012-10-18 14:25:21 UTC) #6
dan433584
If you can limit your interest to constant offsets, GetPointerBaseWithConstantOffset would be a lot simpler ...
6 years, 3 months ago (2012-10-18 17:34:32 UTC) #7
otinn
On 2012/10/18 17:34:32, dan433584 wrote: > If you can limit your interest to constant offsets, ...
6 years, 3 months ago (2012-10-19 10:46:17 UTC) #8
dan433584
On Fri, Oct 19, 2012 at 3:46 AM, <llvm@otinn.com> wrote: > On 2012/10/18 17:34:32, dan433584 ...
6 years, 3 months ago (2012-10-19 12:55:20 UTC) #9
kcc1
On Fri, Oct 19, 2012 at 4:55 PM, Dan Gohman <dan433584@gmail.com> wrote: > > > ...
6 years, 3 months ago (2012-10-19 14:01:56 UTC) #10
otinn
On 2012/10/19 12:55:20, dan433584 wrote: > On Fri, Oct 19, 2012 at 3:46 AM, <mailto:llvm@otinn.com> ...
6 years, 3 months ago (2012-10-19 14:28:00 UTC) #11
christianity.webb
2 weeks, 2 days ago (2019-01-05 23:12:26 UTC) #12
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
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b