[asan] In llvm.asan.globals, allow entries to be non-GlobalVariable and skip over them

[kubamracek]

Summary

Looks like there are valid reasons why we need to allow bitcasts in llvm.asan.globals, see discussion at https://github.com/apple/swift-llvm/pull/133. Let's skip over them in the llvm.asan.globals list.

[rjmccall]

Thanks! I think the only real question here is whether the entry should be *skipped* if it isn't directly a global variable or if you should stripPointerCasts() to try to find the global.

For the IR-linker example I gave you, it's probably reasonable to assume that if the global is in llvm.asan.globals in one module, it's there in all of them, just possibly with a different type. Assuming the linker picks a type for the global that corresponds to its type from at least one of the input modules (I.e. it doesn't just invent an arbitrarily different type from whole cloth), that means there should be at least one uncasted entry in llvm.asan.globals.

On the other hand, it's probably more resilient to strip casts. This would also handle the case where e.g. a global-initialization optimization sees something weird, like a pointer-typed global being initialized with a float, and decides that its best option is to rewrite the global's type. That shouldn't result in the global being dropped from llvm.asan.globals.

[kubamracek]

I see, you're probably right. The reason why I suggested to skip entries with bitcasts was because the cases where I hit this crash were all such that the global was getting re-inserted into llvm.asan.globals anyway, so it didn't matter if we skipped them. But I guess that doesn't need to always be the case. I'll change the patch to look through bitcasts, and I believe the code should already be handling the case of repeated values in llvm.asan.globals.

[rjmccall]

lib/Transforms/Instrumentation/AddressSanitizer.cpp

445 ↗

(On Diff #178751)

This will crash if V is actually null.

[rjmccall]

lib/Transforms/Instrumentation/AddressSanitizer.cpp

448 ↗

(On Diff #178759)

This can now just be dyn_cast. But LGTM with that.

llvm/trunk/lib/Transforms/Instrumentation/AddressSanitizer.cpp

public:
   void init(Module &M) {
     assert(!inited_);
     inited_ = true;
     NamedMDNode *Globals = M.getNamedMetadata("llvm.asan.globals");
     if (!Globals) return;
     for (auto MDN : Globals->operands()) {
       // Metadata node contains the global and the fields of "Entry".
       assert(MDN->getNumOperands() == 5);
-      auto *GV = mdconst::extract_or_null<GlobalVariable>(MDN->getOperand(0));
+      auto *V = mdconst::extract_or_null<Constant>(MDN->getOperand(0));
       // The optimizer may optimize away a global entirely.
+      if (!V) continue;
+      auto *StrippedV = V->stripPointerCasts();
+      auto *GV = dyn_cast<GlobalVariable>(StrippedV);
       if (!GV) continue;
       // We can already have an entry for GV if it was merged with another
       // global.
       Entry &E = Entries[GV];
       if (auto *Loc = cast_or_null<MDNode>(MDN->getOperand(1)))
         E.SourceLoc.parse(Loc);
       if (auto *Name = cast_or_null<MDString>(MDN->getOperand(2)))
         E.Name = Name->getString();

llvm/trunk/test/Instrumentation/AddressSanitizer/global_metadata_bitcasts.ll

+; Test that the compiler doesn't crash when the llvm.asan.globals containts
+; an entry that points to a BitCast instruction.
+
+; RUN: opt < %s -asan -asan-module -asan-globals-live-support=1 -S
+
+target datalayout = "e-m:o-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-apple-macosx10.11.0"
+
+@g = global [1 x i32] zeroinitializer, align 4
+
+!llvm.asan.globals = !{!0, !1}
+!0 = !{[1 x i32]* @g, null, !"name", i1 false, i1 false}
+!1 = !{i8* bitcast ([1 x i32]* @g to i8*), null, !"name", i1 false, i1 false}