[PATCH] D81699: MemorySanitizer: Add option to insert init checks at call site

[Vitaly Buka via Phabricator via llvm-commits]

vitalybuka added inline comments.


================
Comment at: llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp:889
+  static const llvm::StringRef ExpectUninitOperands[] = {
+    "__sanitizer_unaligned_store16", "__sanitizer_unaligned_store32", "__sanitizer_unaligned_store64",
+  };
----------------
this line is to long. please clang-format the patch
I usually do that by:
git clang-format -f --style=file HEAD^


================
Comment at: llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp:1122
     // must always return fully initialized values. For now, we hardcode "main".
-    CheckReturnValue = SanitizeFunction && (F.getName() == "main");
+    CheckReturnValue = F.getName() == "main";
+    CheckReturnValue &= SanitizeFunction;
----------------
Please undo such irrelevant changes or move them into separate patches


================
Comment at: llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp:1268
+    
+    if (StructType *strct = dyn_cast<StructType>(Shadow->getType())) {
+      unsigned i = 0;
----------------
var names are inconsistent
same for i and it
llvm style is to start with uppercase


================
Comment at: llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp:1270
+      unsigned i = 0;
+      for (auto it = strct->element_begin(); it != strct->element_end(); i++, it++) {
+        Value *ShadowItem;
----------------
so if we need integer index, then just iterate with getNumElements() getElementType(unsigned N) ?
range base loop and separate var is fine as well

BTW. the first "i" is 1, is it expected?


================
Comment at: llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp:1271
+      for (auto it = strct->element_begin(); it != strct->element_end(); i++, it++) {
+        Value *ShadowItem;
+        {
----------------
declare and intialize variable at the same statement if possible.
I assume it's to destroy IRB before recursive call
maybe then extract lines in {} into a separate function?


================
Comment at: llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp:1690
+        bool FArgPartialInit = FArg.hasAttribute(Attribute::PartialInit);
+        bool FArgCheck = ClEagerChecks && !FArgByVal && !FArgPartialInit;
         unsigned Size =
----------------
FArgCheck -> FArgEagerCheck?


================
Comment at: llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp:1749
+
+        if (!FArgCheck)
+          ArgOffset += alignTo(Size, kShadowTLSAlignment);
----------------
why do we need this?


================
Comment at: llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp:3442
+      bool PartialInit = CB.paramHasAttr(i, Attribute::PartialInit);
+      bool Check = ClEagerChecks && !ByVal && !PartialInit;
+
----------------
Check -> EagerCheck?


================
Comment at: llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp:3497
+
+    if (ClEagerChecks && !CB.hasRetAttr(Attribute::PartialInit)) {
+      setShadow(&CB, getCleanShadow(&CB));
----------------
ClEagerChecks && !PartialInit;


================
Comment at: llvm/test/Instrumentation/MemorySanitizer/msan_eager.ll:1
+; RUN: opt < %s -msan-check-access-address=0 -msan-track-origins=1 -msan-eager-checks -S -passes='module(msan-module),function(msan)' 2>&1 | \
+; RUN:   FileCheck -allow-deprecated-dag-overlap -check-prefixes=CHECK,CHECK-ORIGINS %s
----------------
would you like to try go generate test with llvm/utils/update_analyze_test_checks.py


================
Comment at: llvm/test/Instrumentation/MemorySanitizer/msan_eager.ll:9-20
+; Check the presence and the linkage type of __msan_track_origins and
+; other interface symbols.
+; CHECK-NOT: @__msan_track_origins
+; CHECK-ORIGINS: @__msan_track_origins = weak_odr constant i32 1
+; CHECK-NOT: @__msan_keep_going = weak_odr constant i32 0
+; CHECK: @__msan_retval_tls = external thread_local(initialexec) global [{{.*}}]
+; CHECK: @__msan_retval_origin_tls = external thread_local(initialexec) global i32
----------------
llvm/utils/update_analyze_test_checks.py will likely drop this, but they are not important for this check


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D81699/new/

https://reviews.llvm.org/D81699