This is an archive of the discontinued LLVM Phabricator instance.

Handle llvm.launder.invariant.group in msan.
ClosedPublic

Authored by TokarIP on Sep 30 2019, 11:52 AM.

Details

Summary

[MSan] handle llvm.launder.invariant.group

Msan used to give false-positives in

class Foo {
 public:
  virtual ~Foo() {};
};

// Return true iff *x is set.
bool f1(void **x, bool flag);

Foo* f() {
  void *p;
  bool found;
  found = f1(&p,flag);
  if (found) {
    // p is always set here.
    return static_cast<Foo*>(p); // False positive here.
  }
  return nullptr;
}

Diff Detail

Event Timeline

TokarIP created this revision.Sep 30 2019, 11:52 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 30 2019, 11:52 AM
TokarIP edited the summary of this revision. (Show Details)Sep 30 2019, 11:53 AM

Thank you.
Do you mind handling llvm.strip.invariant.group in the same change?

I'm not sure if llvm.invariant.start and llvm.invariant.end need any handling. It kind of makes sense to ignore the shadow of their pointer arguments, since (a) it will be used in an actual load/store where uninit would be detected and (b) we don't want to raise an alarm in the case the intrinsic call is speculated.

llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp
2567

Why bitcast? From the intrinsic definition, the types must match.

llvm/test/Instrumentation/MemorySanitizer/msan_llvm_launder_invariant.ll
26

The test could be a lot simpler, something like

i8* f(i8 *p) {
return @llvm.launder.invariant.group(p);
}

+1, strip.invariant.group probably needs the same handling. Thanks for fixing!

TokarIP updated this revision to Diff 222625.Oct 1 2019, 8:15 AM
TokarIP marked 2 inline comments as done.
TokarIP added inline comments.
llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp
2567

You are right, fixed.

llvm/test/Instrumentation/MemorySanitizer/msan_llvm_launder_invariant.ll
26

This is just a n IR from minimal reproducer for false-positive, so I wanted to keep it as a regression test.
I've tried your test case, but it generates same IR with and without this patch.

eugenis accepted this revision.Oct 1 2019, 1:09 PM

LGTM with one more test

llvm/test/Instrumentation/MemorySanitizer/msan_llvm_launder_invariant.ll
26

This does not sound right. Before this patch there should be a shadow check for the intrinsic argument, after the patch - only this:

%0 = load i64, i64* getelementptr inbounds ([100 x i64], [100 x i64]* @__msan_param_tls, i32 0, i32 0), align 8
%q = call i8* @llvm.launder.invariant.group.p0i8(i8* %p)
store i64 %0, i64* getelementptr inbounds ([100 x i64], [100 x i64]* @__msan_retval_tls, i32 0, i32 0), align 8
ret i8* %q

Sure, let's keep this test, it gives extra context about the problem, but please add a minimal one for llvm.strip.invariant.group.

This revision is now accepted and ready to land.Oct 1 2019, 1:09 PM
TokarIP updated this revision to Diff 222717.Oct 1 2019, 2:55 PM

Added llvm.strip.invariant.group test, checked that if fails without this patch.

This revision was automatically updated to reflect the committed changes.