<div dir="ltr">I am in favor of trying this out. <div>The worst that may happen is that a program with two UBs in the same place will have less readable msan report. </div><div>In parallel, we should hook our user to UBSan's alignment check :) </div><div><br></div><div>--kcc </div></div><div class="gmail_extra"><br><div class="gmail_quote">On Mon, Dec 8, 2014 at 12:57 AM, 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">I'm having second thoughts about this. By relying on pointer alignment<br>
information, we are turning something that's technically undefined<br>
behavior (in case the declared alignment is wrong) but normally works<br>
correctly anyway, at least on X86, into corrupted origin data without<br>
diagnostic.<br>
<div class="HOEnZb"><div class="h5"><br>
On Fri, Dec 5, 2014 at 5:34 PM, Evgeniy Stepanov<br>
<<a href="mailto:eugeni.stepanov@gmail.com">eugeni.stepanov@gmail.com</a>> wrote:<br>
> Author: eugenis<br>
> Date: Fri Dec 5 08:34:03 2014<br>
> New Revision: 223464<br>
><br>
> URL: <a href="http://llvm.org/viewvc/llvm-project?rev=223464&view=rev" target="_blank">http://llvm.org/viewvc/llvm-project?rev=223464&view=rev</a><br>
> Log:<br>
> [msan] Avoid extra origin address realignment.<br>
><br>
> Do not realign origin address if the corresponding application<br>
> address is at least 4-byte-aligned.<br>
><br>
> Saves 2.5% code size in track-origins mode.<br>
><br>
> Added:<br>
> llvm/trunk/test/Instrumentation/MemorySanitizer/origin-alignment.ll<br>
> Modified:<br>
> llvm/trunk/lib/Transforms/Instrumentation/MemorySanitizer.cpp<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=223464&r1=223463&r2=223464&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Instrumentation/MemorySanitizer.cpp?rev=223464&r1=223463&r2=223464&view=diff</a><br>
> ==============================================================================<br>
> --- llvm/trunk/lib/Transforms/Instrumentation/MemorySanitizer.cpp (original)<br>
> +++ llvm/trunk/lib/Transforms/Instrumentation/MemorySanitizer.cpp Fri Dec 5 08:34:03 2014<br>
> @@ -522,9 +522,11 @@ struct MemorySanitizerVisitor : public I<br>
><br>
> void storeOrigin(IRBuilder<> &IRB, Value *Addr, Value *Shadow, Value *Origin,<br>
> unsigned Alignment, bool AsCall) {<br>
> + unsigned OriginAlignment = std::max(kMinOriginAlignment, Alignment);<br>
> if (isa<StructType>(Shadow->getType())) {<br>
> - IRB.CreateAlignedStore(updateOrigin(Origin, IRB), getOriginPtr(Addr, IRB),<br>
> - Alignment);<br>
> + IRB.CreateAlignedStore(updateOrigin(Origin, IRB),<br>
> + getOriginPtr(Addr, IRB, Alignment),<br>
> + OriginAlignment);<br>
> } else {<br>
> Value *ConvertedShadow = convertToShadowTyNoVec(Shadow, IRB);<br>
> // TODO(eugenis): handle non-zero constant shadow by inserting an<br>
> @@ -549,7 +551,8 @@ struct MemorySanitizerVisitor : public I<br>
> Cmp, IRB.GetInsertPoint(), false, MS.OriginStoreWeights);<br>
> IRBuilder<> IRBNew(CheckTerm);<br>
> IRBNew.CreateAlignedStore(updateOrigin(Origin, IRBNew),<br>
> - getOriginPtr(Addr, IRBNew), Alignment);<br>
> + getOriginPtr(Addr, IRBNew, Alignment),<br>
> + OriginAlignment);<br>
> }<br>
> }<br>
> }<br>
> @@ -573,11 +576,9 @@ struct MemorySanitizerVisitor : public I<br>
><br>
> if (SI.isAtomic()) SI.setOrdering(addReleaseOrdering(SI.getOrdering()));<br>
><br>
> - if (MS.TrackOrigins) {<br>
> - unsigned Alignment = std::max(kMinOriginAlignment, SI.getAlignment());<br>
> - storeOrigin(IRB, Addr, Shadow, getOrigin(Val), Alignment,<br>
> + if (MS.TrackOrigins)<br>
> + storeOrigin(IRB, Addr, Shadow, getOrigin(Val), SI.getAlignment(),<br>
> InstrumentWithCalls);<br>
> - }<br>
> }<br>
> }<br>
><br>
> @@ -739,16 +740,17 @@ struct MemorySanitizerVisitor : public I<br>
> /// address.<br>
> ///<br>
> /// OriginAddr = (ShadowAddr + OriginOffset) & ~3ULL<br>
> - Value *getOriginPtr(Value *Addr, IRBuilder<> &IRB) {<br>
> + Value *getOriginPtr(Value *Addr, IRBuilder<> &IRB, unsigned Alignment) {<br>
> Value *ShadowLong =<br>
> - IRB.CreateAnd(IRB.CreatePointerCast(Addr, MS.IntptrTy),<br>
> - ConstantInt::get(MS.IntptrTy, ~MS.ShadowMask));<br>
> - Value *Add =<br>
> - IRB.CreateAdd(ShadowLong,<br>
> - ConstantInt::get(MS.IntptrTy, MS.OriginOffset));<br>
> - Value *SecondAnd =<br>
> - IRB.CreateAnd(Add, ConstantInt::get(MS.IntptrTy, ~3ULL));<br>
> - return IRB.CreateIntToPtr(SecondAnd, PointerType::get(IRB.getInt32Ty(), 0));<br>
> + IRB.CreateAnd(IRB.CreatePointerCast(Addr, MS.IntptrTy),<br>
> + ConstantInt::get(MS.IntptrTy, ~MS.ShadowMask));<br>
> + Value *Origin = IRB.CreateAdd(<br>
> + ShadowLong, ConstantInt::get(MS.IntptrTy, MS.OriginOffset));<br>
> + if (Alignment < kMinOriginAlignment) {<br>
> + uint64_t Mask = kMinOriginAlignment - 1;<br>
> + Origin = IRB.CreateAnd(Origin, ConstantInt::get(MS.IntptrTy, ~Mask));<br>
> + }<br>
> + return IRB.CreateIntToPtr(Origin, PointerType::get(IRB.getInt32Ty(), 0));<br>
> }<br>
><br>
> /// \brief Compute the shadow address for a given function argument.<br>
> @@ -1052,9 +1054,10 @@ struct MemorySanitizerVisitor : public I<br>
><br>
> if (MS.TrackOrigins) {<br>
> if (PropagateShadow) {<br>
> - unsigned Alignment = std::max(kMinOriginAlignment, I.getAlignment());<br>
> - setOrigin(&I,<br>
> - IRB.CreateAlignedLoad(getOriginPtr(Addr, IRB), Alignment));<br>
> + unsigned Alignment = I.getAlignment();<br>
> + unsigned OriginAlignment = std::max(kMinOriginAlignment, Alignment);<br>
> + setOrigin(&I, IRB.CreateAlignedLoad(getOriginPtr(Addr, IRB, Alignment),<br>
> + OriginAlignment));<br>
> } else {<br>
> setOrigin(&I, getCleanOrigin());<br>
> }<br>
> @@ -1706,7 +1709,7 @@ struct MemorySanitizerVisitor : public I<br>
> // FIXME: use ClStoreCleanOrigin<br>
> // FIXME: factor out common code from materializeStores<br>
> if (MS.TrackOrigins)<br>
> - IRB.CreateStore(getOrigin(&I, 1), getOriginPtr(Addr, IRB));<br>
> + IRB.CreateStore(getOrigin(&I, 1), getOriginPtr(Addr, IRB, 1));<br>
> return true;<br>
> }<br>
><br>
> @@ -1733,7 +1736,7 @@ struct MemorySanitizerVisitor : public I<br>
><br>
> if (MS.TrackOrigins) {<br>
> if (PropagateShadow)<br>
> - setOrigin(&I, IRB.CreateLoad(getOriginPtr(Addr, IRB)));<br>
> + setOrigin(&I, IRB.CreateLoad(getOriginPtr(Addr, IRB, 1)));<br>
> else<br>
> setOrigin(&I, getCleanOrigin());<br>
> }<br>
><br>
> Added: llvm/trunk/test/Instrumentation/MemorySanitizer/origin-alignment.ll<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Instrumentation/MemorySanitizer/origin-alignment.ll?rev=223464&view=auto" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Instrumentation/MemorySanitizer/origin-alignment.ll?rev=223464&view=auto</a><br>
> ==============================================================================<br>
> --- llvm/trunk/test/Instrumentation/MemorySanitizer/origin-alignment.ll (added)<br>
> +++ llvm/trunk/test/Instrumentation/MemorySanitizer/origin-alignment.ll Fri Dec 5 08:34:03 2014<br>
> @@ -0,0 +1,73 @@<br>
> +; RUN: opt < %s -msan -msan-check-access-address=0 -msan-track-origins=1 -S | FileCheck -check-prefix=CHECK -check-prefix=CHECK-ORIGINS1 %s<br>
> +; RUN: opt < %s -msan -msan-check-access-address=0 -msan-track-origins=2 -S | FileCheck -check-prefix=CHECK -check-prefix=CHECK-ORIGINS2 %s<br>
> +<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>
> +target triple = "x86_64-unknown-linux-gnu"<br>
> +<br>
> +<br>
> +; Check origin instrumentation of stores.<br>
> +; Check that debug info for origin propagation code is set correctly.<br>
> +<br>
> +@a8 = global i8 0, align 8<br>
> +@a4 = global i8 0, align 4<br>
> +@a2 = global i8 0, align 2<br>
> +@a1 = global i8 0, align 1<br>
> +<br>
> +; 8-aligned store => 8-aligned origin store, origin address is not realigned<br>
> +define void @Store8(i8 %x) sanitize_memory {<br>
> +entry:<br>
> + store i8 %x, i8* @a8, align 8<br>
> + ret void<br>
> +}<br>
> +<br>
> +; CHECK-LABEL: @Store8<br>
> +; CHECK-ORIGINS1: [[ORIGIN:%[01-9a-z]+]] = load {{.*}} @__msan_param_origin_tls<br>
> +; CHECK-ORIGINS2: [[ORIGIN0:%[01-9a-z]+]] = load {{.*}} @__msan_param_origin_tls<br>
> +; CHECK-ORIGINS2: [[ORIGIN:%[01-9a-z]+]] = call i32 @__msan_chain_origin(i32 [[ORIGIN0]])<br>
> +; CHECK: store i32 [[ORIGIN]], i32* inttoptr (i64 add (i64 and (i64 ptrtoint {{.*}} to i32*), align 8<br>
> +; CHECK: ret void<br>
> +<br>
> +<br>
> +; 4-aligned store => 4-aligned origin store, origin address is not realigned<br>
> +define void @Store4(i8 %x) sanitize_memory {<br>
> +entry:<br>
> + store i8 %x, i8* @a4, align 4<br>
> + ret void<br>
> +}<br>
> +<br>
> +; CHECK-LABEL: @Store4<br>
> +; CHECK-ORIGINS1: [[ORIGIN:%[01-9a-z]+]] = load {{.*}} @__msan_param_origin_tls<br>
> +; CHECK-ORIGINS2: [[ORIGIN0:%[01-9a-z]+]] = load {{.*}} @__msan_param_origin_tls<br>
> +; CHECK-ORIGINS2: [[ORIGIN:%[01-9a-z]+]] = call i32 @__msan_chain_origin(i32 [[ORIGIN0]])<br>
> +; CHECK: store i32 [[ORIGIN]], i32* inttoptr (i64 add (i64 and (i64 ptrtoint {{.*}} to i32*), align 4<br>
> +; CHECK: ret void<br>
> +<br>
> +<br>
> +; 2-aligned store => 4-aligned origin store, origin address is realigned<br>
> +define void @Store2(i8 %x) sanitize_memory {<br>
> +entry:<br>
> + store i8 %x, i8* @a2, align 2<br>
> + ret void<br>
> +}<br>
> +<br>
> +; CHECK-LABEL: @Store2<br>
> +; CHECK-ORIGINS1: [[ORIGIN:%[01-9a-z]+]] = load {{.*}} @__msan_param_origin_tls<br>
> +; CHECK-ORIGINS2: [[ORIGIN0:%[01-9a-z]+]] = load {{.*}} @__msan_param_origin_tls<br>
> +; CHECK-ORIGINS2: [[ORIGIN:%[01-9a-z]+]] = call i32 @__msan_chain_origin(i32 [[ORIGIN0]])<br>
> +; CHECK: store i32 [[ORIGIN]], i32* inttoptr (i64 and (i64 add (i64 and (i64 ptrtoint {{.*}} i64 -4) to i32*), align 4<br>
> +; CHECK: ret void<br>
> +<br>
> +<br>
> +; 1-aligned store => 4-aligned origin store, origin address is realigned<br>
> +define void @Store1(i8 %x) sanitize_memory {<br>
> +entry:<br>
> + store i8 %x, i8* @a1, align 1<br>
> + ret void<br>
> +}<br>
> +<br>
> +; CHECK-LABEL: @Store1<br>
> +; CHECK-ORIGINS1: [[ORIGIN:%[01-9a-z]+]] = load {{.*}} @__msan_param_origin_tls<br>
> +; CHECK-ORIGINS2: [[ORIGIN0:%[01-9a-z]+]] = load {{.*}} @__msan_param_origin_tls<br>
> +; CHECK-ORIGINS2: [[ORIGIN:%[01-9a-z]+]] = call i32 @__msan_chain_origin(i32 [[ORIGIN0]])<br>
> +; CHECK: store i32 [[ORIGIN]], i32* inttoptr (i64 and (i64 add (i64 and (i64 ptrtoint {{.*}} i64 -4) to i32*), align 4<br>
> +; CHECK: ret void<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>
</div></div></blockquote></div><br></div>