[PATCH] D81390: [KernelAddressSanitizer] Make globals constructors compatible with kernel [v2]

[Marco Elver via Phabricator via cfe-commits]

melver added a comment.

Thanks! PTAL.



================
Comment at: llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp:759
         // do globals-gc.
-        UseCtorComdat(UseGlobalsGC && ClWithComdat) {
-    this->Recover = ClRecover.getNumOccurrences() > 0 ? ClRecover : Recover;
-    this->CompileKernel =
-        ClEnableKasan.getNumOccurrences() > 0 ? ClEnableKasan : CompileKernel;
-
+        UseCtorComdat(UseGlobalsGC && ClWithComdat && !this->CompileKernel) {
     C = &(M.getContext());
----------------
glider wrote:
> `UseGlobalsGC` assumes `!this->CompileKernel` already, doesn't it?
I experimented a bit and found that even if one of them is enabled things break (say if you remove UseGlobalsGC here). This is more explicit and less error-prone in case somebody removes UseGlobalsGC from UseCtorComdat. And there is no real penalty here for having it on both.


================
Comment at: llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp:1796
+    const GlobalAlias &GA) const {
+  if (CompileKernel) {
+    // When compiling the kernel, globals that are aliased by symbols prefixed
----------------
glider wrote:
> Maybe it's better to call this function only if CompileKernel is true?
> I don't think we need it except for the kernel.
Done, though the previous version was cleaner. I guarded the call but now we need an assert and a comment to make sure the check is moved when people decide to use this for more than the kernel.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81390