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

[Leonard Chan via Phabricator via llvm-commits]

leonardchan marked an inline comment as done.
leonardchan added a comment.

In D56470#1350722 <https://reviews.llvm.org/D56470#1350722>, @chandlerc wrote:

> I think a key thing that this is currently missing (or I'm missing it when looking at hte patch) the core shift that we made working through how to do sanitizer stuff with MSan.
>
> Specifically, I suspect that we don't need any analysis layer, and the module and function layer can operate entirely independently. This would require roughly the following changes to the current ASan infrastructure:


Replied to a comment below with reasoning for the analysis, but it could also be that there's another way to perform initialization without the analysis.

> 1. Change how the per-module initialization of the `AddressSanitizerFunction` pass works to resemble hwat was done for MSan and TSan, factoring it into a helper class that is used by a legacy function pass an a new function pass. This will require the get-or-create pattern for all the things touched, and registering things only when creation is necessary.

I believe I did this in this patch. `AddressSanitizer` was separated into the helper class `AddressSanitizer` and the pass `AddressSanitizerLegacyPass`. The helper class is also used by the new PM function pass `AddressSanitizerPass`. Similar actions were done with `AddressSanitizerModule` (which I believe is independent from `AddressSanitizer`) and created `AddressSanitizerModuleLegacyPass` and `AddressSanitizerModulePass`. I have not yet used `getOrCreateInitFunction`, but I think it only needs to be used with `AddressSanitizerModule` since `AddressSanitizer` does not seem to require changing `llvm.global_ctors`.

> 2. Make a few things in the current function pass lazy such as buildig the inline assembly needed, etc.

Would this be more appropriate in a separate patch since this involves changing some ASan internals? The primary focus of this patch was to be able to use ASan with the new PM.

> 3. Directly port the `AddressSanitizerModule` to new PM.

Already made a new PM equivalent to this (`AddressSanitizerModulePass`), or did you mean that the legacy `AddressSanitizerModule` should be deleted?

> 4. (maybe) decouple the ctor insertion in the module pass from the ctor insertion in the function pass. but maybe no need, just the first one to need it does it.

This could be done with the module pass, but I don't think the `AddressSanitizer` function pass does any constructor insertion.

> I don't think any analysis should be required at this point?
> 
> Let me know if this isn't making sense and I can try to explain more.

I think I understand where you're coming from, but do correct me if I'm wrong. Essentially follow the same pattern as MSan and TSan, which I think is just creating a sanitizer specific initialization function that gets added to `llvm.global_ctors`. I could be wrong on this, but I think this would only apply to `AddressSanitizerModule` and not `AddressSanitizer` specifically, where the main problem encountered last time with `AddressSanitizer` was that initializing `GlobalsMD` on each function pass was too expensive. Perhaps the initialization could also be moved to a function that could be added to `llvm.global_ctors`, although I would need to dig deeper into ASan internals and how it depends on `GlobalsMetadata` to see how this could work.



================
Comment at: llvm/include/llvm/Transforms/Instrumentation/AddressSanitizerPass.h:108-123
+class AddressSanitizerParamsAnalysis
+    : public AnalysisInfoMixin<AddressSanitizerParamsAnalysis> {
+public:
+  using Result = AddressSanitizerParamsWrapper;
+
+  AddressSanitizerParamsAnalysis(bool CompileKernel = false)
+      : Params(CompileKernel) {}
----------------
chandlerc wrote:
> Why is this analysis needed? It seems like it could just be directly computed as part of the pass?
> 
> Is there some logic shared between this and some other pass? If so, documenting what it is and how it works seems really important.
The main reason for the analysis is to just initialize some variables that were initialized in `AddressSanitizer`'s `doInitialization`, namely:

```
  GlobalsMetadata GlobalsMD;
  LLVMContext *C;
  int LongSize;
  Type *IntptrTy;
  Triple TargetTriple;
  ShadowMapping Mapping;
```

These members only need to be initialized once per module instead of per function and can be retrieved from the module. We initially did what was done with MSan and TSan by moving this initialization to the sanitizer constructor (in D52739), but this initialization turned out to be expensive and timely since we would be performing this initialization every time we passed over a function. This resulted in a timeout for an internal ASan build. These parameters can be technically computed during construction of each sanitizer but would be expensive.

>From what we could tell, it was initializing `GlobalsMD` which seemed to be the most expensive part, and neither MSan nor TSan require initializing something like `GlobalsMetadata`. A bunch of alternative approaches were discussed in D54337 and a recommended one was to create a class that just contained these required members and initializing them in an analysis that the function pass can depend on. This should work since the analysis wouldn't be changing any IR, the passes can depend on it and just get the variables it needs from it, and it can be used by both the independent `AddressSanitizer` and `AddressSanitizerModule` passes.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D56470