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

[Philip Pfaffe via Phabricator via llvm-commits]

philip.pfaffe accepted this revision.
philip.pfaffe added a comment.
Herald added a project: LLVM.

Great! Just three more nits. Feel free to land after adressing them!



================
Comment at: llvm/include/llvm/Transforms/Instrumentation/AddressSanitizer.h:74
+
+/// The ASanGlobalsMetadataAnalysis initializes and returns a Globalsmetadata
+/// object. More specifically, ASan requires looking at all globals registered
----------------
Globals**M**etadata


================
Comment at: llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp:529
 
-/// AddressSanitizer: instrument the code in module to find memory bugs.
-struct AddressSanitizer : public FunctionPass {
-  // Pass identification, replacement for typeid
+bool ShouldCompileKernel(bool CompileKernel) {
+  return ClEnableKasan.getNumOccurrences() > 0 ? ClEnableKasan : CompileKernel;
----------------
leonardchan wrote:
> philip.pfaffe wrote:
> > No need to pull this out!
> > 
> > As a followup, I want to add options handling just like we did for loop unrolling. Then this will get unified in a much nicer way!
> Done. I assume also rL350808 is the commit you are referring to? Should that be a patch separate from this or added on top of this also? 
Correct. I just submitted D57640 against msan to show how this might look.


================
Comment at: llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp:1202
 
-ModulePass *llvm::createAddressSanitizerModulePass(bool CompileKernel,
+ModulePass *llvm::createModuleAddressSanitizerPass(bool CompileKernel,
                                                    bool Recover,
----------------
The full name should be `createModuleAddressSanitizerLegacyPassPass`


================
Comment at: llvm/test/Instrumentation/AddressSanitizer/basic.ll:4
 ; RUN: opt < %s -asan -asan-module -S | FileCheck --check-prefixes=CHECK,CHECK-S3 %s
+; RUN: opt < %s -passes='require<asan-globals-md>,function(asan),module(asan-module)' -S | FileCheck --check-prefixes=CHECK,CHECK-S3 %s
 ; RUN: opt < %s -asan -asan-module -asan-mapping-scale=5 -S | FileCheck --check-prefixes=CHECK,CHECK-S5 %s
----------------
Could you drop a comment here //why// the `require<>` is required?


Repository:
  rL LLVM

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

https://reviews.llvm.org/D56470