llvm dev - May 2020 - Sanitizers + New Pass Manager

On 5/14/20 5:33 AM, Arthur Eubanks via llvm-dev wrote:
> > Is it the case that with the legacy PM there is no inlining at 
> either -O2 or -O3 and with newPM there is? Or is there something else 
> going on?
> Legacy PM inlines at -O2/-O3, new PM inlines at -O1/-O2/-O3. These 
> cases where inlining occurs also coincide with the test failure. I 
> agree that inlining itself isn't the issue, but it seems to contribute 
> to the stripping of lifetime intrinsics and thus the test failure.
>
> ASan needs lifetime intrinsics to implement use-after-scope. It may 
> just be luck that the legacy PM's -O1 happens to not strip lifetime 
> intrinsics before ASan gets to them?
>
> I tried moving the *San passes before inlining and now the tests that 
> I was having trouble with pass, but new tests that specifically test 
> inlining fail, so I think it's probably fairly important to run *San 
> passes after inlining. But I tried moving them right after inlining 
> (before SROA/something gets to optimize things out) and some TSan 
> tests now fail. I found out TSan expects to be run after pretty much 
> all other passes so that it can change any memset/memcpy intrinsics 
> into just the normal function call to memset()/memcpy(). But if we 
> move TSan to be run before InstCombine, then InstCombine will change 
> calls to memset()/memcpy() back to the intrinsics and the tests don't 
> like that. I think it's reasonable to treat all *San passes the same 
> way, so I don't think putting ASan somewhere different from TSan makes 
> sense. But that means there's no good place to put these sanitizers.
>
> I feel like a lot of these sanitizer tests just happened to work at 
> -O1 under the legacy PM. I think I'll try having the new PM not inline 
> when any sanitizers are enabled and see if I can keep the house of 
> cards standing. Or is it a bad idea to change the PM behavior based on 
> the detection of sanitizers, since sanitizers won't be testing the 
> same code you'd get without sanitizing (e.g. a lot of the inlining 
> tests would lose a lot of their value)?
While trying newPM with inliner turned off on O1 is fine and probably a 
right approach on digging into the cause of the failure,
actually changing Ox to be that different depending on sanitizer mode 
seems to be a bad idea to me.

regards,
   Fedor.
>
> On Wed, May 13, 2020 at 3:14 PM Johannes Doerfert 
> <johannesdoerfert at gmail.com <mailto:johannesdoerfert at
gmail.com>> wrote:
>
>
>     On 5/13/20 3:31 PM, David Blaikie via llvm-dev wrote:
>>     I believe it's meant to run after /some/ optimizations to make
it a bit
>>     more efficient, while not so optimized that it misses opportunities
to
>>     detect bugs - but I could be wrong there. I'll leave it up to
other folks
>>     to chime in.
>
>     I think that is right. The more transformations you run the more
>     UB you can also "loose" as it is defined to something by the
>     transformation.
>
>     Lifetime markers are an example. Once removed, which is generally
>     legal in IR, you cannot argue accesses after the end are UB.
>
>
>>     On Wed, May 13, 2020 at 1:04 PM Arthur Eubanks<aeubanks at
google.com>  <mailto:aeubanks at google.com>  wrote:
>>
>>>     Just tested it out, that test does indeed fail under the old PM
at -O3 and
>>>     even at -O2.
>>>
>>>     If the ASan pass runs after optimizations and is designed to
detect
>>>     undefined behavior at runtime, I don't see how it can be
super reliable at
>>>     higher optimization levels.
>>>
>>>     On Wed, May 13, 2020 at 12:39 PM David Blaikie<dblaikie at
gmail.com>  <mailto:dblaikie at gmail.com>  wrote:
>>>
>>>>     +some sanitizer/new pass manager folks
>>>>
>>>>
>>>>
>>>>     On Wed, May 13, 2020 at 12:22 PM Arthur Eubanks via
llvm-dev <
>>>>     llvm-dev at lists.llvm.org  <mailto:llvm-dev at
lists.llvm.org>> wrote:
>>>>
>>>>>     Hi,
>>>>>
>>>>>     I've been trying to burn down the remaining
sanitizer failures under the
>>>>>     new pass manager. Right now I'm stuck on some ASan
tests.
>>>>>
>>>>>     Some ASan tests run under -O1. There are a couple
differences between
>>>>>     the old and new pass managers under -O1, e.g. the old
PM doesn't inline
>>>>>     whereas the new PM does. The differences seem to cause
some lifetime
>>>>>     intrinsics to get stripped out (e.g. via SROA,
InstCombine). It might be
>>>>>     due to ASan specifically testing undefined behavior,
and different
>>>>>     optimizations run means different behavior. For a
specific example,
>>>>>     use-after-scope-dtor-order.cpp runs under -O1 and fails
under the new PM
>>>>>     because SROA strips out the lifetime intrinsics and by
the time the ASan
>>>>>     pass runs it doesn't find the lifetime intrinsics
to add its own
>>>>>     instrumentation.
>>>>>
>>>>     That, to me, sounds like a real bug in the
optimizations/asan
>>>>     implementation if this choice fo optimizations makes the
diagnosis go away.
>>>>     (is ASan able to diagnose the problem at -O3 (where I guess
SROA and other
>>>>     things run) with the legacy pass manager?)
>>>>
>>>>
>>>>>     What's the proper way to resolve this? Run the
tests under -O0? Change
>>>>>     the passes pipeline under the new PM when ASan (and
maybe other sanitizers)
>>>>>     is detected?
>>>>>     _______________________________________________
>>>>>     LLVM Developers mailing list
>>>>>     llvm-dev at lists.llvm.org  <mailto:llvm-dev at
lists.llvm.org>
>>>>>    
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>>>>>
>>
>>     _______________________________________________
>>     LLVM Developers mailing list
>>     llvm-dev at lists.llvm.org  <mailto:llvm-dev at
lists.llvm.org>
>>     https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>
>
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20200514/b5c310c4/attachment.html>

On Thu, May 14, 2020 at 3:13 AM Fedor Sergeev via llvm-dev <
llvm-dev at lists.llvm.org> wrote:
>
>
> On 5/14/20 5:33 AM, Arthur Eubanks via llvm-dev wrote:
>
> > Is it the case that with the legacy PM there is no inlining at either
> -O2 or -O3 and with newPM there is? Or is there something else going on?
> Legacy PM inlines at -O2/-O3, new PM inlines at -O1/-O2/-O3. These cases
> where inlining occurs also coincide with the test failure. I agree that
> inlining itself isn't the issue, but it seems to contribute to the
> stripping of lifetime intrinsics and thus the test failure.
>
> ASan needs lifetime intrinsics to implement use-after-scope. It may just
> be luck that the legacy PM's -O1 happens to not strip lifetime
intrinsics
> before ASan gets to them?
>
> I tried moving the *San passes before inlining and now the tests that I
> was having trouble with pass, but new tests that specifically test inlining
> fail, so I think it's probably fairly important to run *San passes
after
> inlining. But I tried moving them right after inlining (before
> SROA/something gets to optimize things out) and some TSan tests now fail. I
> found out TSan expects to be run after pretty much all other passes so that
> it can change any memset/memcpy intrinsics into just the normal function
> call to memset()/memcpy(). But if we move TSan to be run before
> InstCombine, then InstCombine will change calls to memset()/memcpy() back
> to the intrinsics and the tests don't like that. I think it's
reasonable to
> treat all *San passes the same way, so I don't think putting ASan
somewhere
> different from TSan makes sense. But that means there's no good place
to
> put these sanitizers.
>
> I feel like a lot of these sanitizer tests just happened to work at -O1
> under the legacy PM. I think I'll try having the new PM not inline when
any
> sanitizers are enabled and see if I can keep the house of cards standing.
> Or is it a bad idea to change the PM behavior based on the detection of
> sanitizers, since sanitizers won't be testing the same code you'd
get
> without sanitizing (e.g. a lot of the inlining tests would lose a lot of
> their value)?
>
> While trying newPM with inliner turned off on O1 is fine and probably a
> right approach on digging into the cause of the failure,
> actually changing Ox to be that different depending on sanitizer mode
> seems to be a bad idea to me.
>
>
Agreed. I don't want to turn off the inliner at O1. Reduce it down for
sure, but not turn it off. If you need help investigating these let me know
and we can try to help.

-eric

> regards,
>   Fedor.
>
>
> On Wed, May 13, 2020 at 3:14 PM Johannes Doerfert <
> johannesdoerfert at gmail.com> wrote:
>
>>
>> On 5/13/20 3:31 PM, David Blaikie via llvm-dev wrote:
>>
>> I believe it's meant to run after /some/ optimizations to make it a
bit
>> more efficient, while not so optimized that it misses opportunities to
>> detect bugs - but I could be wrong there. I'll leave it up to other
folks
>> to chime in.
>>
>> I think that is right. The more transformations you run the more UB you
>> can also "loose" as it is defined to something by the
transformation.
>>
>> Lifetime markers are an example. Once removed, which is generally legal
>> in IR, you cannot argue accesses after the end are UB.
>>
>>
>> On Wed, May 13, 2020 at 1:04 PM Arthur Eubanks <aeubanks at
google.com> <aeubanks at google.com> wrote:
>>
>>
>> Just tested it out, that test does indeed fail under the old PM at -O3
and
>> even at -O2.
>>
>> If the ASan pass runs after optimizations and is designed to detect
>> undefined behavior at runtime, I don't see how it can be super
reliable at
>> higher optimization levels.
>>
>> On Wed, May 13, 2020 at 12:39 PM David Blaikie <dblaikie at
gmail.com> <dblaikie at gmail.com> wrote:
>>
>>
>> +some sanitizer/new pass manager folks
>>
>>
>>
>> On Wed, May 13, 2020 at 12:22 PM Arthur Eubanks via llvm-dev
<llvm-dev at lists.llvm.org> wrote:
>>
>>
>> Hi,
>>
>> I've been trying to burn down the remaining sanitizer failures
under the
>> new pass manager. Right now I'm stuck on some ASan tests.
>>
>> Some ASan tests run under -O1. There are a couple differences between
>> the old and new pass managers under -O1, e.g. the old PM doesn't
inline
>> whereas the new PM does. The differences seem to cause some lifetime
>> intrinsics to get stripped out (e.g. via SROA, InstCombine). It might
be
>> due to ASan specifically testing undefined behavior, and different
>> optimizations run means different behavior. For a specific example,
>> use-after-scope-dtor-order.cpp runs under -O1 and fails under the new
PM
>> because SROA strips out the lifetime intrinsics and by the time the
ASan
>> pass runs it doesn't find the lifetime intrinsics to add its own
>> instrumentation.
>>
>>
>> That, to me, sounds like a real bug in the optimizations/asan
>> implementation if this choice fo optimizations makes the diagnosis go
away.
>> (is ASan able to diagnose the problem at -O3 (where I guess SROA and
other
>> things run) with the legacy pass manager?)
>>
>>
>>
>> What's the proper way to resolve this? Run the tests under -O0?
Change
>> the passes pipeline under the new PM when ASan (and maybe other
sanitizers)
>> is detected?
>> _______________________________________________
>> LLVM Developers mailing listllvm-dev at
lists.llvm.orghttps://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>>
>>
>> _______________________________________________
>> LLVM Developers mailing listllvm-dev at
lists.llvm.orghttps://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>>
>>
> _______________________________________________
> LLVM Developers mailing listllvm-dev at
lists.llvm.orghttps://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>
>
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20200514/4d26df26/attachment.html>

Sanitizer passes really should not run before the inliner. For example,
ASan moves all allocas into a "mega-alloca" to obtain fixed frame
layout
for reporting purposes. It also inserts a fake stack check in the function
prologue which will get duplicated (but will probably still work) after
inlining.

MSan removes readnone/readonly from all functions because they all update
shadow which is a memory write. Doing this early will suppress all kinds of
IPO.

I think ASan (and probably MSan) does the same thing as TSan regarding
memory intrinsics because it needs the runtime library to be able to tell
which memcpy/memset calls are application logic (and therefore need to
check/update shadow) and which are compiler-inserted after sanitizer
instrumentation. Doing this too early suppresses some optimizations.

It's probably OK to run the passes a little earlier (but not before
inlining), but I'd be careful to weigh the benefits of that against the
loss of performance. If the goal is to catch more bugs (specifically, any
undefined behavior that gets optimized out before instrumentation), I'd
rather do something in the frontend and in the IR passes suppress those
optimizations in sanitized functions.


On Thu, May 14, 2020 at 12:56 PM Eric Christopher via llvm-dev <
llvm-dev at lists.llvm.org> wrote:
>
>
> On Thu, May 14, 2020 at 3:13 AM Fedor Sergeev via llvm-dev <
> llvm-dev at lists.llvm.org> wrote:
>
>>
>>
>> On 5/14/20 5:33 AM, Arthur Eubanks via llvm-dev wrote:
>>
>> > Is it the case that with the legacy PM there is no inlining at
either
>> -O2 or -O3 and with newPM there is? Or is there something else going
on?
>> Legacy PM inlines at -O2/-O3, new PM inlines at -O1/-O2/-O3. These
cases
>> where inlining occurs also coincide with the test failure. I agree that
>> inlining itself isn't the issue, but it seems to contribute to the
>> stripping of lifetime intrinsics and thus the test failure.
>>
>> ASan needs lifetime intrinsics to implement use-after-scope. It may
just
>> be luck that the legacy PM's -O1 happens to not strip lifetime
intrinsics
>> before ASan gets to them?
>>
>> I tried moving the *San passes before inlining and now the tests that I
>> was having trouble with pass, but new tests that specifically test
inlining
>> fail, so I think it's probably fairly important to run *San passes
after
>> inlining. But I tried moving them right after inlining (before
>> SROA/something gets to optimize things out) and some TSan tests now
fail. I
>> found out TSan expects to be run after pretty much all other passes so
that
>> it can change any memset/memcpy intrinsics into just the normal
function
>> call to memset()/memcpy(). But if we move TSan to be run before
>> InstCombine, then InstCombine will change calls to memset()/memcpy()
back
>> to the intrinsics and the tests don't like that. I think it's
reasonable to
>> treat all *San passes the same way, so I don't think putting ASan
somewhere
>> different from TSan makes sense. But that means there's no good
place to
>> put these sanitizers.
>>
>> I feel like a lot of these sanitizer tests just happened to work at -O1
>> under the legacy PM. I think I'll try having the new PM not inline
when any
>> sanitizers are enabled and see if I can keep the house of cards
standing.
>> Or is it a bad idea to change the PM behavior based on the detection of
>> sanitizers, since sanitizers won't be testing the same code
you'd get
>> without sanitizing (e.g. a lot of the inlining tests would lose a lot
of
>> their value)?
>>
>> While trying newPM with inliner turned off on O1 is fine and probably a
>> right approach on digging into the cause of the failure,
>> actually changing Ox to be that different depending on sanitizer mode
>> seems to be a bad idea to me.
>>
>>
> Agreed. I don't want to turn off the inliner at O1. Reduce it down for
> sure, but not turn it off. If you need help investigating these let me know
> and we can try to help.
>
> -eric
>
>
>> regards,
>>   Fedor.
>>
>>
>> On Wed, May 13, 2020 at 3:14 PM Johannes Doerfert <
>> johannesdoerfert at gmail.com> wrote:
>>
>>>
>>> On 5/13/20 3:31 PM, David Blaikie via llvm-dev wrote:
>>>
>>> I believe it's meant to run after /some/ optimizations to make
it a bit
>>> more efficient, while not so optimized that it misses opportunities
to
>>> detect bugs - but I could be wrong there. I'll leave it up to
other folks
>>> to chime in.
>>>
>>> I think that is right. The more transformations you run the more UB
you
>>> can also "loose" as it is defined to something by the
transformation.
>>>
>>> Lifetime markers are an example. Once removed, which is generally
legal
>>> in IR, you cannot argue accesses after the end are UB.
>>>
>>>
>>> On Wed, May 13, 2020 at 1:04 PM Arthur Eubanks <aeubanks at
google.com> <aeubanks at google.com> wrote:
>>>
>>>
>>> Just tested it out, that test does indeed fail under the old PM at
-O3 and
>>> even at -O2.
>>>
>>> If the ASan pass runs after optimizations and is designed to detect
>>> undefined behavior at runtime, I don't see how it can be super
reliable at
>>> higher optimization levels.
>>>
>>> On Wed, May 13, 2020 at 12:39 PM David Blaikie <dblaikie at
gmail.com> <dblaikie at gmail.com> wrote:
>>>
>>>
>>> +some sanitizer/new pass manager folks
>>>
>>>
>>>
>>> On Wed, May 13, 2020 at 12:22 PM Arthur Eubanks via llvm-dev
<llvm-dev at lists.llvm.org> wrote:
>>>
>>>
>>> Hi,
>>>
>>> I've been trying to burn down the remaining sanitizer failures
under the
>>> new pass manager. Right now I'm stuck on some ASan tests.
>>>
>>> Some ASan tests run under -O1. There are a couple differences
between
>>> the old and new pass managers under -O1, e.g. the old PM
doesn't inline
>>> whereas the new PM does. The differences seem to cause some
lifetime
>>> intrinsics to get stripped out (e.g. via SROA, InstCombine). It
might be
>>> due to ASan specifically testing undefined behavior, and different
>>> optimizations run means different behavior. For a specific example,
>>> use-after-scope-dtor-order.cpp runs under -O1 and fails under the
new PM
>>> because SROA strips out the lifetime intrinsics and by the time the
ASan
>>> pass runs it doesn't find the lifetime intrinsics to add its
own
>>> instrumentation.
>>>
>>>
>>> That, to me, sounds like a real bug in the optimizations/asan
>>> implementation if this choice fo optimizations makes the diagnosis
go away.
>>> (is ASan able to diagnose the problem at -O3 (where I guess SROA
and other
>>> things run) with the legacy pass manager?)
>>>
>>>
>>>
>>> What's the proper way to resolve this? Run the tests under -O0?
Change
>>> the passes pipeline under the new PM when ASan (and maybe other
sanitizers)
>>> is detected?
>>> _______________________________________________
>>> LLVM Developers mailing listllvm-dev at
lists.llvm.orghttps://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>>>
>>>
>>> _______________________________________________
>>> LLVM Developers mailing listllvm-dev at
lists.llvm.orghttps://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>>>
>>>
>> _______________________________________________
>> LLVM Developers mailing listllvm-dev at
lists.llvm.orghttps://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>>
>>
>> _______________________________________________
>> LLVM Developers mailing list
>> llvm-dev at lists.llvm.org
>> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>>
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20200514/98aa247d/attachment.html>