[PATCH] D52739: [PassManager/Sanitizer] Port of AddresSanitizer pass from legacy to new PassManager

[Philip Pfaffe via Phabricator via llvm-commits]

philip.pfaffe added a comment.

Some comments inline. Does it make sense to merge the module and function passes into one? Is it really necessary to distinguish the passes?



================
Comment at: include/llvm/IR/AddressSanitizerPass.h:23
+
+class AddressSanitizerPass {
+public:
----------------
The PassInfoMixin takes care of the name function for you.


================
Comment at: include/llvm/IR/AddressSanitizerPass.h:37
+
+class AddressSanitizerModulePass {
+public:
----------------
Use the PassInfoMixin.


================
Comment at: lib/Transforms/Instrumentation/AddressSanitizer.cpp:610
 
-  StringRef getPassName() const override {
-    return "AddressSanitizerFunctionPass";
-  }
-
-  void getAnalysisUsage(AnalysisUsage &AU) const override {
-    AU.addRequired<DominatorTreeWrapperPass>();
-    AU.addRequired<TargetLibraryInfoWrapperPass>();
-  }
+  static StringRef getPassName() { return "AddressSanitizerFunctionPass"; }
 
----------------
This isn't a pass anymore, it doesn't need this function.


================
Comment at: lib/Transforms/Instrumentation/AddressSanitizer.cpp:739
+  bool instrument(Module &M);
+  static StringRef getPassName() { return "AddressSanitizerModule"; }
 
----------------
Unneeded function.


================
Comment at: lib/Transforms/Instrumentation/AddressSanitizer.cpp:1047
 
+class AddressSanitizerLegacy : public FunctionPass {
+public:
----------------
The convention is to name it AddressSanitizerLegacyPass


================
Comment at: lib/Transforms/Instrumentation/AddressSanitizer.cpp:1081
+
+class AddressSanitizerModuleLegacy : public ModulePass {
+public:
----------------
The convention is to name it AddressSanitizerModuleLegacyPass


================
Comment at: lib/Transforms/Instrumentation/AddressSanitizer.cpp:1124
+  AddressSanitizer Sanitizer(CompileKernel, Recover, UseAfterScope);
+  return Sanitizer.instrument(F, DT, TLI) ? PreservedAnalyses::none()
+                                          : PreservedAnalyses::all();
----------------
Just make this an `if`.


================
Comment at: lib/Transforms/Instrumentation/AddressSanitizer.cpp:1131
+  AddressSanitizerModule Sanitizer(CompileKernel, Recover, UseAfterScope);
+  return Sanitizer.instrument(M) ? PreservedAnalyses::none()
+                                 : PreservedAnalyses::all();
----------------
As above, just make this an `if`


================
Comment at: lib/Transforms/Instrumentation/AddressSanitizer.cpp:2538
+                                  const TargetLibraryInfo *TLI) {
+  if (!Inited) {
+    // Initialize the private fields. No one has accessed them before.
----------------
Instead of doing this kind of lazy initialization, just do it in the constructor.


Repository:
  rL LLVM

https://reviews.llvm.org/D52739