Page MenuHomePhabricator

CommandLine: Cleanup options and remove use of ManagedStatic
AcceptedPublic

Authored by nhaehnle on Jul 5 2022, 2:19 AM.

Details

Reviewers
efriedma
lattner
Summary

Replace uses of ManagedStatics by function-local static variables.

Also fix a corruption issue when LLVM is used as a shared library in a
plugin setting.

Consider the case where A depends on B depends on LLVM, where B is a
shared compiler library built on LLVM that defines command-line options
because it can also be used in a non-plugin setting.

If A is loaded and unloaded multiple times, causing B to be loaded and
unloaded multiple times while LLVM *isn't* unloaded, then corruption
results without this change: as B is unloaded, its cl::opt globals
disappear but the GlobalParser still holds a (dangling) reference to
them. Furthermore, if A and B are loaded a second time, the cl::opt
globals are constructed again which fails because options of the
same name are still registered.

The straightforward fix is to remove options from the GlobalParser
in the cl::Option destructor (which was previously defaulted but
already non-trivial).

Diff Detail

Event Timeline

nhaehnle created this revision.Jul 5 2022, 2:19 AM
nhaehnle requested review of this revision.Jul 5 2022, 2:19 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 5 2022, 2:19 AM
Herald added a subscriber: jholewinski. · View Herald Transcript
efriedma added inline comments.Jul 29 2022, 1:43 PM
llvm/include/llvm/Support/CommandLine.h
354

"This option must have been the last option registered. For testing purposes only."

Is this still accurate? Is it an issue here?

nhaehnle updated this revision to Diff 449607.Aug 3 2022, 2:34 AM

Remove an outdated comment

llvm/include/llvm/Support/CommandLine.h
354

Thanks for spotting that. I did look into that when I wrote the code and the comment looks outdated to me. I've also been using these patches for a while now and this doesn't appear to be an issue in practice. I'm going to remove the comment.

This revision is now accepted and ready to land.Aug 3 2022, 11:15 AM