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

[Leonard Chan via Phabricator via llvm-commits]

leonardchan added inline comments.


================
Comment at: clang/lib/CodeGen/BackendUtil.cpp:1056
+
+    if (LangOpts.Sanitize.has(SanitizerKind::Address)) {
+      bool Recover = CodeGenOpts.SanitizeRecover.has(SanitizerKind::Address);
----------------
philip.pfaffe wrote:
> This is wrong. Check out how this is done for the other sanitizers above! On top of what's happening there, you'll also have to add a requires wrapper for the GlobalsMD analysis at the last module EP before asan is scheduled.
Done. Although I think placing them here will require compiling with `-O1` in order to enter this block.


================
Comment at: llvm/include/llvm/Transforms/Instrumentation/AddressSanitizerPass.h:69
+/// so we do not need to copy it so many times.
+class GlobalsMetadataWrapper {
+public:
----------------
philip.pfaffe wrote:
> No need for an extra class. Just make GlobalsMetadata the result of the analysis.
Done. But if we don't have a wrapper that holds a reference to GlobalsMetadata, wouldn't that mean copying it's internal map every time we return it as a result from the analysis?

Although I'm not sure how big this map can actually get, so it may not be worth caring about. I also couldn't find examples of analyses that return references either.


================
Comment at: llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp:716
+
+class AddressSanitizerModule {
+public:
----------------
philip.pfaffe wrote:
> I'd prefer ModuleAddressSanitizer, because it's not a module, it's a sanitizer.
Done. Also renamed other functions + classes with `AddressSanitizerModule` to `ModuleAddressSanitizer`.


================
Comment at: llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp:1148
+  GlobalsMetadata &GlobalsMD =
+      AM.getResult<GlobalsMetadataAnalysis>(F).getGlobalsMD();
+  const TargetLibraryInfo *TLI = &AM.getResult<TargetLibraryAnalysis>(F);
----------------
philip.pfaffe wrote:
> leonardchan wrote:
> > philip.pfaffe wrote:
> > > 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!
> > One question before continuing: so how can this function pass use the results of this module analysis if it's not a function pass? When turning on the debug pass manager with opt, the analysis does not actually get run. Wrapping it in a require wrapper with "-passed='require<asan-globals-md>'" allows it to run on the module pass manager, but still crashes when the function pass manager runs.
> > 
> > The crash is that `"This analysis pass was not registered prior to being queried"` (the GlobalsMetadataAnalysis).
> You need to get it through the ModuleAnalysisManager proxy.
Ah. I did not know about this proxy. This is very useful. Although this requires adding `require<analysis>` to the tests.

Also chose to make it a hard error if the analysis result was not found beforehand, but if there's a better alternative I don't mind changing it.


================
Comment at: llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp:1208
 
 ModulePass *llvm::createAddressSanitizerModulePass(bool CompileKernel,
                                                    bool Recover,
----------------
philip.pfaffe wrote:
> Needs a name update. At the same time, it's declaration should go into AddressSanitizer.h
Done. Also moved the one for the function address sanitizer.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D56470