[NFC][msan] Don't setOrigin for byval pointer

[vitalybuka]

Summary

It's NFC because shadow of pointer is clean so origins will not be
propagated anyway.

Depends on D117275

[vitalybuka]

update

[eugenis]

This does not look right. We may not always check that the shadow is constant zero before accessing origin. Note that getOrigin() wants origins to be available for all values.

a = b + c
Sa = Sb | Sc
Oa = Sb ? Ob : Oc;  // <= this will attempt getOrigin(c) without looking at Sc.

In any case, there are other places in the pass with the same pattern.

[vitalybuka]

In D117276#3244615, @eugenis wrote:

This does not look right. We may not always check that the shadow is constant zero before accessing origin. Note that getOrigin() wants origins to be available for all values.

a = b + c
Sa = Sb | Sc
Oa = Sb ? Ob : Oc;  // <= this will attempt getOrigin(c) without looking at Sc.

Yes, but after the patch I call more setOrigin(A, getCleanOrigin()); not less

To clarify:
the only changed case is (TrackOrigin && !Overflow && ByVal)
Before the patch: setOrigin(A, ... unrelated nonsense)
After the patch: setOrigin(A,getClean())

[eugenis]

Hmm I somehow managed to read your patch backwards :)

LGTM

llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp

if (Argument *A = dyn_cast<Argument>(V)) {
               Value *Base = getShadowPtrForArgument(&FArg, EntryIRB, ArgOffset);
               const Align CopyAlign = std::min(ArgAlign, kShadowTLSAlignment);
               Value *Cpy = EntryIRB.CreateMemCpy(CpShadowPtr, CopyAlign, Base,
                                                  CopyAlign, Size);
               LLVM_DEBUG(dbgs() << "  ByValCpy: " << *Cpy << "\n");
               (void)Cpy;
             }
             *ShadowPtr = getCleanShadow(V);
+            setOrigin(A, getCleanOrigin());
           } else if (MS.EagerChecks && FArg.hasAttribute(Attribute::NoUndef)) {
             *ShadowPtr = getCleanShadow(V);
             setOrigin(A, getCleanOrigin());
             break;
           } else {
             if (Overflow) {
               // ParamTLS overflow.
               *ShadowPtr = getCleanShadow(V);
+              setOrigin(A, getCleanOrigin());
             } else {
               // Shadow over TLS
               Value *Base = getShadowPtrForArgument(&FArg, EntryIRB, ArgOffset);
               *ShadowPtr = EntryIRB.CreateAlignedLoad(getShadowTy(&FArg), Base,
                                                       kShadowTLSAlignment);
-            }
-          }
-          LLVM_DEBUG(dbgs()
-                     << "  ARG:    " << FArg << " ==> " << **ShadowPtr << "\n");
-          if (MS.TrackOrigins && !Overflow) {
+              if (MS.TrackOrigins) {
                 Value *OriginPtr =
                     getOriginPtrForArgument(&FArg, EntryIRB, ArgOffset);
                 setOrigin(A, EntryIRB.CreateLoad(MS.OriginTy, OriginPtr));
-          } else {
-            setOrigin(A, getCleanOrigin());
               }
-
+            }
+          }
+          LLVM_DEBUG(dbgs()
+                     << "  ARG:    " << FArg << " ==> " << **ShadowPtr << "\n");
           break;
         }
 
         ArgOffset += alignTo(Size, kShadowTLSAlignment);
       }
       assert(*ShadowPtr && "Could not find shadow for an argument");
       return *ShadowPtr;
     }

llvm/test/Instrumentation/MemorySanitizer/byval.ll

 ; RUN: opt < %s -S -passes="msan<track-origins=1>" 2>&1 | FileCheck %s --implicit-check-not "call void @llvm.mem" --implicit-check-not " load" --implicit-check-not " store"
 ; RUN: opt < %s -S -msan -msan-track-origins=1 | FileCheck %s --implicit-check-not "call void @llvm.mem" --implicit-check-not " load" --implicit-check-not " store"
 
 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"
 target triple = "x86_64-unknown-linux-gnu"
 
 declare void @FnByVal(i128* byval(i128) %p);
 declare void @Fn(i128* %p);
 
 define i128 @ByValArgument(i32, i128* byval(i128) %p) sanitize_memory {
 ; CHECK-LABEL: @ByValArgument(
 ; CHECK-NEXT:  entry:
 ; CHECK:         call void @llvm.memcpy.p0i8.p0i8.i64({{.*}} @__msan_param_tls to i64), i64 8) to i8*), i64 16, i1 false)
-; CHECK:         load i32, i32* inttoptr (i64 add (i64 ptrtoint ([200 x i32]* @__msan_param_origin_tls to i64), i64 8) to i32*)
 ; CHECK:         %x = load i128, i128* %p
 ; CHECK:         load i128
 ; CHECK:         load i32
 ; CHECK:         store i128 {{.*}}, i128* bitcast ([100 x i64]* @__msan_retval_tls to i128*)
 ; CHECK-NEXT:    store i32 {{.*}}, i32* @__msan_retval_origin_tls
 ; CHECK-NEXT:    ret i128
 ;
 entry:
entry:
   ret i128 %x
 }
 
 ; FIXME: Origin of byval pointee is not propagated.
 define void @ByValForward(i32, i128* byval(i128) %p) sanitize_memory {
 ; CHECK-LABEL: @ByValForward(
 ; CHECK-NEXT:  entry:
 ; CHECK:         call void @llvm.memcpy.p0i8.p0i8.i64({{.*}} @__msan_param_tls to i64), i64 8) to i8*), i64 16, i1 false)
-; CHECK:         load i32, i32* inttoptr (i64 add (i64 ptrtoint ([200 x i32]* @__msan_param_origin_tls to i64), i64 8) to i32*)
 ; CHECK:         store i64 0, i64* getelementptr inbounds ([100 x i64], [100 x i64]* @__msan_param_tls, i32 0, i32 0)
 ; CHECK-NEXT:    call void @Fn(
 ; CHECK-NEXT:    ret void
 ;
 entry:
   call void @Fn(i128* %p)
   ret void
 }
entry:
   ret void
 }
 
 ; FIXME: Origin of %p byval pointee is not propagated.
 define void @ByValForwardByVal(i32, i128* byval(i128) %p) sanitize_memory {
 ; CHECK-LABEL: @ByValForwardByVal(
 ; CHECK-NEXT:  entry:
 ; CHECK:         call void @llvm.memcpy.p0i8.p0i8.i64({{.*}} @__msan_param_tls to i64), i64 8) to i8*), i64 16, i1 false)
-; CHECK:         load i32, i32* inttoptr (i64 add (i64 ptrtoint ([200 x i32]* @__msan_param_origin_tls to i64), i64 8) to i32*) 
 ; CHECK:         call void @llvm.memcpy.p0i8.p0i8.i64(i8* bitcast ([100 x i64]* @__msan_param_tls to i8*), i8* {{.*}}, i64 16, i1 false)
 ; CHECK:         store i32 {{.*}}, i32* getelementptr inbounds ([200 x i32], [200 x i32]* @__msan_param_origin_tls, i32 0, i32 0)
 ; CHECK-NEXT:    call void @FnByVal(
 ; CHECK-NEXT:    ret void
 ;
 entry:
   call void @FnByVal(i128* byval(i128) %p)
   ret void