[PATCH] D56470: [NewPM] Second attempt at porting ASan

[Philip Pfaffe via Phabricator via llvm-commits]

philip.pfaffe added a comment.

Nice, we're getting somewhere! A lot of comments inline. Most importantly, your analysis is a function analysis, which it mustn't be, otherwise there's no gain from it at all.

In addition to basic.ll, you should also test the code paths using GlobalsMD now.



================
Comment at: llvm/include/llvm/Transforms/Instrumentation/AddressSanitizerPass.h:14
+//===----------------------------------------------------------------------===//
+#ifndef LLVM_TRANSFORMS_INSTRUMENTATION_ADDRESSSANITIZERPASS_H
+#define LLVM_TRANSFORMS_INSTRUMENTATION_ADDRESSSANITIZERPASS_H
----------------
The filename should be just AddressSanitizer.h, like for the other sanitizers.


================
Comment at: llvm/include/llvm/Transforms/Instrumentation/AddressSanitizerPass.h:49
+
+  void reset() {
+    inited_ = false;
----------------
This isn't needed.


================
Comment at: llvm/include/llvm/Transforms/Instrumentation/AddressSanitizerPass.h:54
+
+  void init(Module &M);
+
----------------
You can drop this and init on construction.


================
Comment at: llvm/include/llvm/Transforms/Instrumentation/AddressSanitizerPass.h:69
+/// so we do not need to copy it so many times.
+class GlobalsMetadataWrapper {
+public:
----------------
No need for an extra class. Just make GlobalsMetadata the result of the analysis.


================
Comment at: llvm/include/llvm/Transforms/Instrumentation/AddressSanitizerPass.h:90
+
+class GlobalsMetadataAnalysis
+    : public AnalysisInfoMixin<GlobalsMetadataAnalysis> {
----------------
Name it `ASanGlobalsMetadataAnalysis` to make more clear what it does. Since it is an analysis now that people can use, this needs comments!


================
Comment at: llvm/include/llvm/Transforms/Instrumentation/AddressSanitizerPass.h:96
+  Result run(Module &, ModuleAnalysisManager &);
+  Result run(Function &, FunctionAnalysisManager &);
+
----------------
This can't be a function analysis, or it would still run for every function.


================
Comment at: llvm/include/llvm/Transforms/Instrumentation/AddressSanitizerPass.h:102
+
+  GlobalsMetadata GlobalsMD;
+};
----------------
Make GlobalsMetadata the direct result of the analysis.


================
Comment at: llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp:535
+/// Module analysis for getting various metadata about the module.
+class GlobalsMetadataLegacyPass : public ModulePass {
+public:
----------------
The convention is to name this  `ASanGlobalsMetadataWrapperPass`.


================
Comment at: llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp:564
+struct AddressSanitizer {
+  explicit AddressSanitizer(Module &M, GlobalsMetadata &GlobalsMD,
+                            bool CompileKernel = false, bool Recover = false,
----------------
explicit is superfluous now.


================
Comment at: llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp:621
   Value *memToShadow(Value *Shadow, IRBuilder<> &IRB);
-  bool runOnFunction(Function &F) override;
+  bool instrument(Function &F, const TargetLibraryInfo *TLI);
   bool maybeInsertAsanInitAtFunctionEntry(Function &F);
----------------
It's called  instrumentFunction in the other sanitizers.


================
Comment at: llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp:716
+
+class AddressSanitizerModule {
+public:
----------------
I'd prefer ModuleAddressSanitizer, because it's not a module, it's a sanitizer.


================
Comment at: llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp:745
 
-  bool runOnModule(Module &M) override;
-  StringRef getPassName() const override { return "AddressSanitizerModule"; }
+  bool instrument(Module &);
 
----------------
Should be named instrumentModule.


================
Comment at: llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp:1148
+  GlobalsMetadata &GlobalsMD =
+      AM.getResult<GlobalsMetadataAnalysis>(F).getGlobalsMD();
+  const TargetLibraryInfo *TLI = &AM.getResult<TargetLibraryAnalysis>(F);
----------------
When GlobalsMD stops being a function analysis, this won't work anymore. It might then happen that the result is not available, so you need to handle that case. IMHO that should be a hard error, but alternatively asan can either not run or compute the MD itself. In any case it should not be silent!


================
Comment at: llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp:1176
+INITIALIZE_PASS(
+    GlobalsMetadataLegacyPass, "asan-params",
+    "AddressSanitizer: detects use-after-free and out-of-bounds bugs.", false,
----------------
Use the same name as for the new PM


================
Comment at: llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp:1177
+    GlobalsMetadataLegacyPass, "asan-params",
+    "AddressSanitizer: detects use-after-free and out-of-bounds bugs.", false,
+    true);
----------------
This description is wrong.


================
Comment at: llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp:1208
 
 ModulePass *llvm::createAddressSanitizerModulePass(bool CompileKernel,
                                                    bool Recover,
----------------
Needs a name update. At the same time, it's declaration should go into AddressSanitizer.h


Repository:
  rL LLVM

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

https://reviews.llvm.org/D56470