Page MenuHomePhabricator

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

Authored by kubamracek on Dec 17 2018, 2:50 PM.

Details

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.

Diff Detail

Repository
rL LLVM

Event Timeline

kubamracek created this revision.Dec 17 2018, 2:50 PM

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.

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 added inline comments.Dec 18 2018, 12:26 PM
lib/Transforms/Instrumentation/AddressSanitizer.cpp
445 ↗(On Diff #178751)

This will crash if V is actually null.

kubamracek marked an inline comment as done.
rjmccall accepted this revision.Dec 18 2018, 1:03 PM
rjmccall added inline comments.
lib/Transforms/Instrumentation/AddressSanitizer.cpp
448 ↗(On Diff #178759)

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

This revision is now accepted and ready to land.Dec 18 2018, 1:03 PM
This revision was automatically updated to reflect the committed changes.