[PATCH] D55605: [NewPM] Port Msan: Alternative approach using a module to create the global constructor

[Alexander Potapenko via Phabricator via llvm-commits]

glider added a comment.

A couple of nits.
I also think the newly added classes should be marked as such, not as structs, but YMMV.



================
Comment at: llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp:576
+
+struct MemorySanitizerLegacyPass : public FunctionPass {
+  // Pass identification, replacement for typeid.
----------------
A comment explaining why there's "Legacy" in the name would be nice. We'll also need a precondition for removing this legacy from the code.


================
Comment at: llvm/test/Instrumentation/MemorySanitizer/PowerPC/vararg-ppc64le.ll:1
+; RUN: opt < %s -S -passes='msan-init,function(msan)' 2>&1 | FileCheck %s
 ; RUN: opt < %s -msan -S | FileCheck %s
----------------
This command line duplication is fragile. Can it be somehow done in lit?


================
Comment at: llvm/test/Instrumentation/MemorySanitizer/X86/vararg-too-large.ll:1
+; RUN: opt < %s -msan-check-access-address=0 -S 2>&1 -passes='msan-init,function(msan)' | FileCheck  \
+; RUN: %s
----------------
Nit: I think it's more readable if you break the lines before pipes and indent the continued lines, e.g.:

```
; RUN: opt < %s -msan-check-access-address=0 ... \
; RUN:   | FileCheck %s
```


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

https://reviews.llvm.org/D55605