This is an archive of the discontinued LLVM Phabricator instance.

Registry initialization and linkage interactions
ClosedPublic

Authored by reames on Jan 13 2016, 6:02 PM.

Details

Summary

Hey C++ template/linkage experts out there, this patch is for you.

The Registry class constructs a linked list of nodes whose storage is inside static variables and nodes are added via static initializers. The trick is that those static initializers are in both the LLVM code base, and some random plugin that might get loaded in at runtime. The existing code tries to use C++ templates and their ODR rules to get a single definition of the registry for each type, but, experimentally, this doesn't quite work as designed. (Well, the entire structure doesn't. It might not actually be an ODR problem.)

Previously, when I tried moving the GCStrategy class (along with it's registry) from CodeGen to IR, I ran into a problem where asking the GCStrategyRegistry a question would return inconsistent results depending on whether you asked from CodeGen (where the static initializers still were) or Transforms. My best guess is that this is a result of either a) an order of initialization error, or b) we ended up with two copies of the registry being created. I remember at the time having convinced myself it was probably (b), but I don't have any of my notes around from that investigation any more.

See http://reviews.llvm.org/rL226311 for the original patch in question.

This patch tries to remove the possibility of (b) above. It still suffers from (a) since there are static initializers in several files within CodeGen that all modify this structure. I haven't fixed that, but hopefully, I haven't made it worse either. Once this is in, I plan to restructure code to put all of the static initializers used in the Registry in a single file to remove that possibility as well.

Diff Detail

Event Timeline

reames updated this revision to Diff 44819.Jan 13 2016, 6:02 PM
reames retitled this revision from to Registry initialization and linkage interactions.
reames updated this object.
reames added reviewers: chandlerc, rsmith.
reames added a subscriber: llvm-commits.

We currently produce a linkonce_odr for
_ZN4llvm8RegistryINS_10GCStrategyEE4HeadE, so short of some linker
script this should be working (on ELF).

In any case, IMHO giving a home to these symbols is an improvement, so LGTM.

This revision was automatically updated to reflect the committed changes.
cray added a subscriber: cray.Jan 19 2016, 11:54 AM

This commit seems to have caused a build break in clang. There are now undefined references to llvm::Registry<T>::Head, and llvm::Registry<T>::Tail

Output from buildbot:

lib/libclangFrontend.a(FrontendAction.cpp.o): In function `clang::FrontendAction::CreateWrappedASTConsumer(clang::CompilerInstance&, llvm::StringRef)':
FrontendAction.cpp:(.text._ZN5clang14FrontendAction24CreateWrappedASTConsumerERNS_16CompilerInstanceEN4llvm9StringRefE+0xfc): undefined reference to `llvm::Registry<clang::PluginASTAction>::Head'
FrontendAction.cpp:(.text._ZN5clang14FrontendAction24CreateWrappedASTConsumerERNS_16CompilerInstanceEN4llvm9StringRefE+0x100): undefined reference to `llvm::Registry<clang::PluginASTAction>::Head'
lib/libclangTooling.a(JSONCompilationDatabase.cpp.o): In function `_GLOBALsub_I_JSONCompilationDatabase.cpp':
JSONCompilationDatabase.cpp:(.text.startup._GLOBAL
sub_I_JSONCompilationDatabase.cpp+0x0): undefined reference to `llvm::Registry<clang::tooling::CompilationDatabasePlugin>::Tail'
JSONCompilationDatabase.cpp:(.text.startup._GLOBALsub_I_JSONCompilationDatabase.cpp+0x8): undefined reference to `llvm::Registry<clang::tooling::CompilationDatabasePlugin>::Tail'
JSONCompilationDatabase.cpp:(.text.startup._GLOBAL
sub_I_JSONCompilationDatabase.cpp+0x4c): undefined reference to `llvm::Registry<clang::tooling::CompilationDatabasePlugin>::Tail'
JSONCompilationDatabase.cpp:(.text.startup._GLOBALsub_I_JSONCompilationDatabase.cpp+0x60): undefined reference to `llvm::Registry<clang::tooling::CompilationDatabasePlugin>::Head'
JSONCompilationDatabase.cpp:(.text.startup._GLOBAL
sub_I_JSONCompilationDatabase.cpp+0x68): undefined reference to `llvm::Registry<clang::tooling::CompilationDatabasePlugin>::Head'
lib/libclangTooling.a(CompilationDatabase.cpp.o): In function `clang::tooling::CompilationDatabase::loadFromDirectory(llvm::StringRef, std::string&)':
CompilationDatabase.cpp:(.text._ZN5clang7tooling19CompilationDatabase17loadFromDirectoryEN4llvm9StringRefERSs+0x1a4): undefined reference to `llvm::Registry<clang::tooling::CompilationDatabasePlugin>::Head'
CompilationDatabase.cpp:(.text._ZN5clang7tooling19CompilationDatabase17loadFromDirectoryEN4llvm9StringRefERSs+0x1a8): undefined reference to `llvm::Registry<clang::tooling::CompilationDatabasePlugin>::Head'

This was already reverted due to those build failures.

Philip