Page MenuHomePhabricator

MSan: handle llvm.lifetime.start intrinsic
ClosedPublic

Authored by glider on Apr 12 2019, 7:23 AM.

Details

Summary

When a variable goes into scope several times within a single function
or when two variables from different scopes share a stack slot it may
be incorrect to poison such scoped locals at the beginning of the
function.
In the former case it may lead to false negatives (see
https://github.com/google/sanitizers/issues/590), in the latter - to
incorrect reports (because only one origin remains on the stack).

If Clang emits lifetime intrinsics with known size for such scoped
variables we insert code poisoning them after each call to
llvm.lifetime.start(). This is done on a best-effort basis.
If for a certain intrinsic we fail to find a corresponding alloca, we
fall back to poisoning allocas for the whole function, as it's now
impossible to tell which alloca was missed.

The new instrumentation may slow down hot loops containing local
variables with lifetime intrinsics, so we allow disabling it with
-mllvm -msan-handle-lifetime-intrinsics=false.

Diff Detail

Repository
rL LLVM

Event Timeline

glider created this revision.Apr 12 2019, 7:23 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 12 2019, 7:23 AM
glider updated this revision to Diff 194898.Apr 12 2019, 8:35 AM

Removed errs()

Please test the case when alloca can not be found.
See https://bugs.llvm.org/show_bug.cgi?id=41481 for the reference.

With this change msan will start detecting new bugs, ex.:
for (int i = 0; ...; ++i) {

int x;
if (i == 0) x=0;
read(x);

}

It can also slow down some programs a lot.

Do you plan to remove SanitizeMemory condition from shouldEmitLifetimeMarkers ?

llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp
2556 ↗(On Diff #194898)

IMHO failure to find the underlying alloca must result in poisoning of all allocas in the prologue - exactly because we don't know which one we've missed.
Alternatively, since we know the size, we could instrument this lifetime.start anyway using an invalid origin id.

3446 ↗(On Diff #194898)

Let's call this function InstrumentAlloca, and rename instrumentAllocaUserspace to poisonAllocaUserspace.

Please test the case when alloca can not be found.
See https://bugs.llvm.org/show_bug.cgi?id=41481 for the reference.

Ok, will do. The repro in the bug doesn't produce IR containing a select statement for me, but the idea is clear.

With this change msan will start detecting new bugs, ex.:
for (int i = 0; ...; ++i) {

int x;
if (i == 0) x=0;
read(x);

}

It can also slow down some programs a lot.

Agreed. This also makes less sense without origins - do you think we shall hide it behind track-origins>0 ?

Do you plan to remove SanitizeMemory condition from shouldEmitLifetimeMarkers ?

Sorry, I don't get it. We won't see any lifetime markers with that condition removed, right?
What's the point of this fix then?

glider marked 2 inline comments as done.Apr 15 2019, 10:04 AM
glider added inline comments.
llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp
2556 ↗(On Diff #194898)

I like your first proposal better.
Using an invalid origin is impractical, as we won't be able to even find the function this poisoned local belongs to.

3446 ↗(On Diff #194898)

same for instrumentAllocaKmsan?

Please test the case when alloca can not be found.
See https://bugs.llvm.org/show_bug.cgi?id=41481 for the reference.

Ok, will do. The repro in the bug doesn't produce IR containing a select statement for me, but the idea is clear.

With this change msan will start detecting new bugs, ex.:
for (int i = 0; ...; ++i) {

int x;
if (i == 0) x=0;
read(x);

}

It can also slow down some programs a lot.

Agreed. This also makes less sense without origins - do you think we shall hide it behind track-origins>0 ?

No, I think it is a good change. But it would be nice to have an llvm flag to back out just in case.

Do you plan to remove SanitizeMemory condition from shouldEmitLifetimeMarkers ?

Sorry, I don't get it. We won't see any lifetime markers with that condition removed, right?
What's the point of this fix then?

We will see a lot more lifetime markers.
I'm talking about this:

// Disable lifetime markers in msan builds.
// FIXME: Remove this when msan works with lifetime markers.
if (LangOpts.Sanitize.has(SanitizerKind::Memory))
  return false;

http://llvm.org/viewvc/llvm-project?view=revision&revision=235613

glider updated this revision to Diff 195348.Apr 16 2019, 3:52 AM

Added a test containing an intrinsic without a known alloca.
Handled this case by falling back to instrumenting only allocas.
Added a flag to disable llvm.lifetime.start handling.

glider updated this revision to Diff 195350.Apr 16 2019, 3:59 AM

s/instrumentAllocaKmsan/poisonAllocaKmsan
s/instrumentAllocaUserspace/poisonAllocaUserspace

glider edited the summary of this revision. (Show Details)Apr 16 2019, 4:01 AM

Updated the patch, PTAL.

Re: shouldEmitLifetimeMarkers() - thanks, I wasn't aware we disable lifetime markers with -O0.
I don't need that for the kernel (which is built with -O2), but I can sure revert that CL if you think it's ok for the userspace.

FWIW those bits are in clang/lib/CodeGen/CodeGenFunction.cpp now, and removing just them doesn't break any tests.

eugenis added inline comments.Apr 16 2019, 6:02 PM
llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp
1037 ↗(On Diff #195350)

This does not have to be so complicated.
Get rid of FallbackAllocaSet; instead remove instructions from AllocaSet as they are being poisoned at lifetime calls, then poison remaining alloca instructions.

This way if untraceable lifetime was seen, you can simply skip the lifetime poisoning step.

Please rename InstrumentOnlyAllocas to something that does not suggest the we are instrumenting _only_ allocas (and nothing else).

1310 ↗(On Diff #195350)

Move this logic to a separate function.

2574 ↗(On Diff #195350)

What's wrong with variable length allocas?

glider updated this revision to Diff 195526.Apr 17 2019, 2:40 AM

Fixed comments.

glider marked 4 inline comments as done.Apr 17 2019, 2:44 AM
glider added inline comments.
llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp
1037 ↗(On Diff #195350)

Agreed, we don't need FallbackAllocaSet, we can just poison allocas from LifetimeStartList as if they were in AllocaSet (see the updated patch).

2574 ↗(On Diff #195350)

You're right, fixed this.

1300 ↗(On Diff #195526)

Reused instrumentAlloca() here instead of calling KMSAN/userspace-specific functions.

eugenis added inline comments.Apr 19 2019, 10:25 AM
llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp
1299 ↗(On Diff #195526)

What if InstrumentLifetimeStart == false, but there is an alloca with multiple lifetime.start ? It looks like you'll poison it several times in a row.

glider updated this revision to Diff 196450.Apr 24 2019, 7:38 AM
glider marked an inline comment as done.

Fixed the case when an alloca was initialized twice.

glider marked 2 inline comments as done.Apr 24 2019, 7:40 AM
glider added inline comments.
llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp
1299 ↗(On Diff #195526)

Fixed this by adding alloca to AllocaSet instead of calling instrumentAlloca(*Item.second, nullptr).

glider marked an inline comment as done.Apr 25 2019, 2:14 AM

ptal

eugenis added inline comments.Apr 25 2019, 5:35 PM
llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp
2573 ↗(On Diff #196450)

What if AllocaInst is visited after all of its lifetime.start calls? I'm not 100% that this is impossible, and then this code will poison both allocainst and lifetime.start.

IMHO, the visitor should simply collect allocas and lifetimes, and then the finalization code can erase allocas as it instruments their lifetime.starts.

glider updated this revision to Diff 196826.Apr 26 2019, 3:40 AM

Simplified the logic per Evgeniy's request.
No test changes.

glider marked 2 inline comments as done.Apr 26 2019, 3:43 AM
glider added inline comments.
llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp
2573 ↗(On Diff #196450)

You're right, we'd better not over-complicate the collection logic.
I however doubt a lifetime intrinsic can occur before an alloca, because it's using that alloca's result.

glider marked an inline comment as done.Apr 29 2019, 1:03 AM

PTAL

eugenis accepted this revision.Apr 29 2019, 3:29 PM

LGTM
Thank you!

Please remove the condition in clang as well in a separate patch. This thing that you've just fixed is the only reason for MSan condition in shouldEmitLifetimeMarkers() to exist, so I'm afraid it's on you now to clean that up.

This revision is now accepted and ready to land.Apr 29 2019, 3:29 PM
This revision was automatically updated to reflect the committed changes.