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

[Evgenii Stepanov via Phabricator via llvm-commits]

eugenis added inline comments.


================
Comment at: compiler-rt/test/msan/cxa_atexit.cpp:20
 __attribute__((noinline))
-void f(long x) {
+void f(PartInit<long> x) {
 }
----------------
For tests that do not make sense with the eager checks, you could introduce a lit feature, and guard them with
// REQUIRES: !msan_eager_checks



================
Comment at: llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp:285
+static cl::opt<bool> ClEagerChecks("msan-eager-checks",
+       cl::desc("check arguments at function call boundaries"),
+       cl::Hidden, cl::init(false));
----------------
... and return values ...


================
Comment at: llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp:894
+      for (unsigned op = 0; op < 2; op++)
+        func->addAttribute(AttributeList::FirstArgIndex+op, Attribute::PartialInit);
+    }
----------------
There is only one argument that can be uninit - the value to be stored. No need to set the attribute on all of them.


================
Comment at: llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp:1021
 bool MemorySanitizerLegacyPass::doInitialization(Module &M) {
+  if (ClEagerChecks && !M.getModuleFlag("DisallowPoisonedCallArguments")) {
+    report_fatal_error("cannot use MSan eager checks; compiler does not support argument init analysis");
----------------
The module flag is gone, and in any case this should not be a fatal error - if the frontend can not promise correct attribute value, we always delay the check, that's all.


================
Comment at: llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp:1271
+      for (auto it = strct->element_begin(); it != strct->element_end(); i++, it++) {
+        Value *ShadowItem;
+        {
----------------
vitalybuka wrote:
> 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?
It's also common to reuse the builder with IRB.SetInsertPoint


================
Comment at: llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp:1281
+      return;
+    }
+
----------------
This change can be done in a separate patch.

It's more efficient to OR everything together and have a single branch. I'm not sure which would generate better code:
* ShadowCombiner and materializeOneCheck on the result, this might do bad things if there are very large members (like vectors)
* icmp NE 0 each members' shadow and materializeOneCheck of that.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81699