<div style="font-family: arial, helvetica, sans-serif; font-size: 10pt"><br><br><div class="gmail_quote">On Thu, Dec 6, 2012 at 3:41 PM, Evgeniy Stepanov <span dir="ltr"><<a href="mailto:eugeni.stepanov@gmail.com" target="_blank">eugeni.stepanov@gmail.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Author: eugenis<br>
Date: Thu Dec 6 05:41:03 2012<br>
New Revision: 169490<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=169490&view=rev" target="_blank">http://llvm.org/viewvc/llvm-project?rev=169490&view=rev</a><br>
Log:<br>
[msan] Do not store origin for clean values.<br>
<br>
Instead of unconditionally storing origin with every application store,<br>
only do this when the shadow of the stored value is != 0.<br>
<br>
This change also delays instrumentation of stores until after the walk over<br>
function's instructions, because adding new basic blocks confuses InstVisitor.<br>
<br>
We only keep 1 origin value per 4 bytes of application memory. This change<br>
fixes the bug when a store of a single clean byte wiped the origin for the<br>
whole 4-byte area.<br>
<br>
Since stores of uninitialized values are relatively uncommon, this change<br>
improves performance of track-origins mode by 5% median and by up to 47% on<br>
specs.<br>
<br>
Modified:<br>
llvm/trunk/lib/Transforms/Instrumentation/MemorySanitizer.cpp<br>
llvm/trunk/test/Instrumentation/MemorySanitizer/msan_basic.ll<br>
<br>
Modified: llvm/trunk/lib/Transforms/Instrumentation/MemorySanitizer.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Instrumentation/MemorySanitizer.cpp?rev=169490&r1=169489&r2=169490&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Instrumentation/MemorySanitizer.cpp?rev=169490&r1=169489&r2=169490&view=diff</a><br>
==============================================================================<br>
--- llvm/trunk/lib/Transforms/Instrumentation/MemorySanitizer.cpp (original)<br>
+++ llvm/trunk/lib/Transforms/Instrumentation/MemorySanitizer.cpp Thu Dec 6 05:41:03 2012<br>
@@ -102,6 +102,10 @@<br>
cl::desc("propagate shadow through ICmpEQ and ICmpNE"),<br>
cl::Hidden, cl::init(true));<br>
<br>
+static cl::opt<bool> ClStoreCleanOrigin("msan-store-clean-origin",<br>
+ cl::desc("store origin for clean (fully initialized) values"),<br>
+ cl::Hidden, cl::init(false));<br>
+<br>
// This flag controls whether we check the shadow of the address<br>
// operand of load or store. Such bugs are very rare, since load from<br>
// a garbage address typically results in SEGV, but still happen<br>
@@ -180,6 +184,8 @@<br>
uint64_t OriginOffset;<br>
/// \brief Branch weights for error reporting.<br>
MDNode *ColdCallWeights;<br>
+ /// \brief Branch weights for origin store.<br>
+ MDNode *OriginStoreWeights;<br>
/// \brief The blacklist.<br>
OwningPtr<BlackList> BL;<br>
/// \brief An empty volatile inline asm that prevents callback merge.<br>
@@ -308,6 +314,7 @@<br>
OriginTy = IRB.getInt32Ty();<br>
<br>
ColdCallWeights = MDBuilder(*C).createBranchWeights(1, 1000);<br>
+ OriginStoreWeights = MDBuilder(*C).createBranchWeights(1, 1000);<br>
<br>
// Insert a call to __msan_init/__msan_track_origins into the module's CTORs.<br>
appendToGlobalCtors(M, cast<Function>(M.getOrInsertFunction(<br>
@@ -382,6 +389,7 @@<br>
ShadowOriginAndInsertPoint() : Shadow(0), Origin(0), OrigIns(0) { }<br>
};<br>
SmallVector<ShadowOriginAndInsertPoint, 16> InstrumentationList;<br>
+ SmallVector<Instruction*, 16> StoreList;<br>
<br>
MemorySanitizerVisitor(Function &F, MemorySanitizer &MS)<br>
: F(F), MS(MS), VAHelper(CreateVarArgHelper(F, MS, *this)) {<br>
@@ -391,6 +399,49 @@<br>
<< F.getName() << "'\n");<br>
}<br>
<br>
+ void materializeStores() {<br>
+ for (size_t i = 0, n = StoreList.size(); i < n; i++) {<br>
+ StoreInst& I = *dyn_cast<StoreInst>(StoreList[i]);<br>
+<br>
+ IRBuilder<> IRB(&I);<br>
+ Value *Val = I.getValueOperand();<br>
+ Value *Addr = I.getPointerOperand();<br>
+ Value *Shadow = getShadow(Val);<br>
+ Value *ShadowPtr = getShadowPtr(Addr, Shadow->getType(), IRB);<br>
+<br>
+ StoreInst *NewSI = IRB.CreateAlignedStore(Shadow, ShadowPtr, I.getAlignment());<br>
+ DEBUG(dbgs() << " STORE: " << *NewSI << "\n");<br>
+ // If the store is volatile, add a check.<br>
+ if (I.isVolatile())<br>
+ insertCheck(Val, &I);<br>
+ if (ClCheckAccessAddress)<br>
+ insertCheck(Addr, &I);<br>
+<br>
+ if (ClTrackOrigins) {<br>
+ if (ClStoreCleanOrigin || isa<StructType>(Shadow->getType())) {<br>
+ IRB.CreateAlignedStore(getOrigin(Val), getOriginPtr(Addr, IRB), I.getAlignment());<br>
+ } else {<br>
+ Value *ConvertedShadow = convertToShadowTyNoVec(Shadow, IRB);<br>
+<br>
+ Constant *Cst = dyn_cast_or_null<Constant>(ConvertedShadow);<br>
+ // TODO(eugenis): handle non-zero constant shadow by inserting an<br>
+ // unconditional check (can not simply fail compilation as this could<br>
+ // be in the dead code).<br>
+ if (Cst)<br>
+ continue;<br>
+<br>
+ Value *Cmp = IRB.CreateICmpNE(ConvertedShadow,<br>
+ getCleanShadow(ConvertedShadow), "_mscmp");<br>
+ Instruction *CheckTerm =<br>
+ SplitBlockAndInsertIfThen(cast<Instruction>(Cmp), false, MS.OriginStoreWeights);<br>
+ IRBuilder<> IRBNewBlock(CheckTerm);<br>
+ IRBNewBlock.CreateAlignedStore(getOrigin(Val),<br>
+ getOriginPtr(Addr, IRBNewBlock), I.getAlignment());<br>
+ }<br>
+ }<br>
+ }<br>
+ }<br>
+<br>
void materializeChecks() {<br>
for (size_t i = 0, n = InstrumentationList.size(); i < n; i++) {<br>
Instruction *Shadow = InstrumentationList[i].Shadow;<br>
@@ -448,6 +499,11 @@<br>
<br>
VAHelper->finalizeInstrumentation();<br>
<br>
+ // Delayed instrumentation of StoreInst.<br>
+ // This make add new checks to inserted later.<br></blockquote><div><br></div><div>Something is wrong in the comment? </div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+ materializeStores();<br>
+<br>
+ // Insert shadow value checks.<br>
materializeChecks();<br>
<br>
return true;<br>
@@ -738,23 +794,7 @@<br>
/// Optionally, checks that the store address is fully defined.<br>
/// Volatile stores check that the value being stored is fully defined.<br>
void visitStoreInst(StoreInst &I) {<br>
- IRBuilder<> IRB(&I);<br>
- Value *Val = I.getValueOperand();<br>
- Value *Addr = I.getPointerOperand();<br>
- Value *Shadow = getShadow(Val);<br>
- Value *ShadowPtr = getShadowPtr(Addr, Shadow->getType(), IRB);<br>
-<br>
- StoreInst *NewSI = IRB.CreateAlignedStore(Shadow, ShadowPtr, I.getAlignment());<br>
- DEBUG(dbgs() << " STORE: " << *NewSI << "\n");<br>
- (void)NewSI;<br>
- // If the store is volatile, add a check.<br>
- if (I.isVolatile())<br>
- insertCheck(Val, &I);<br>
- if (ClCheckAccessAddress)<br>
- insertCheck(Addr, &I);<br>
-<br>
- if (ClTrackOrigins)<br>
- IRB.CreateAlignedStore(getOrigin(Val), getOriginPtr(Addr, IRB), I.getAlignment());<br>
+ StoreList.push_back(&I);<br>
}<br>
<br>
// Vector manipulation.<br>
<br>
Modified: llvm/trunk/test/Instrumentation/MemorySanitizer/msan_basic.ll<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Instrumentation/MemorySanitizer/msan_basic.ll?rev=169490&r1=169489&r2=169490&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Instrumentation/MemorySanitizer/msan_basic.ll?rev=169490&r1=169489&r2=169490&view=diff</a><br>
==============================================================================<br>
--- llvm/trunk/test/Instrumentation/MemorySanitizer/msan_basic.ll (original)<br>
+++ llvm/trunk/test/Instrumentation/MemorySanitizer/msan_basic.ll Thu Dec 6 05:41:03 2012<br>
@@ -1,4 +1,5 @@<br>
; RUN: opt < %s -msan -S | FileCheck %s<br>
+; RUN: opt < %s -msan -msan-track-origins=1 -S | FileCheck -check-prefix=CHECK-ORIGINS %s<br>
target datalayout = "e-p:64:64:64-i1:8:8-i8:8:8-i16:16:16-i32:32:32-i64:64:64-f32:32:32-f64:64:64-v64:64:64-v128:128:128-a0:0:64-s0:64:64-f80:128:128-n8:16:32:64-S128"<br>
<br>
; Check the presence of __msan_init<br>
@@ -7,6 +8,31 @@<br>
; Check the presence and the linkage type of __msan_track_origins<br>
; CHECK: @__msan_track_origins = weak_odr constant i32 0<br>
<br>
+; Check instrumentation of stores<br>
+define void @Store(i32* nocapture %p, i32 %x) nounwind uwtable {<br>
+entry:<br>
+ store i32 %x, i32* %p, align 4<br>
+ ret void<br>
+}<br>
+<br>
+; CHECK: @Store<br>
+; CHECK: load {{.*}} @__msan_param_tls<br>
+; CHECK: store<br>
+; CHECK: store<br>
+; CHECK: ret void<br>
+; CHECK-ORIGINS: @Store<br>
+; CHECK-ORIGINS: load {{.*}} @__msan_param_tls<br>
+; CHECK-ORIGINS: store<br>
+; CHECK-ORIGINS: icmp<br>
+; CHECK-ORIGINS: br i1<br>
+; CHECK-ORIGINS: <label><br>
+; CHECK-ORIGINS: store<br>
+; CHECK-ORIGINS: br label<br>
+; CHECK-ORIGINS: <label><br>
+; CHECK-ORIGINS: store<br>
+; CHECK-ORIGINS: ret void<br>
+<br>
+<br>
; load followed by cmp: check that we load the shadow and call __msan_warning.<br>
define void @LoadAndCmp(i32* nocapture %a) nounwind uwtable {<br>
entry:<br>
<br>
<br>
_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
</blockquote></div><br></div>