This is an archive of the discontinued LLVM Phabricator instance.

[lldb] Prevent that LLDB randomly crashes in CommandLineParser::addOption by initializing LLVM's command line parser
ClosedPublic

Authored by teemperor on Mar 31 2021, 4:44 AM.

Details

Summary

Since quite a while Apple's LLDB fork (that contains the Swift debugging support) is randomly crashing in CommandLineParser::addOption
with an error such as CommandLine Error: Option 'h' registered more than once!

The backtrace of the crashing thread is shown below. There are also usually many other threads also performing similar clang::FrontendActions
which are all trying to generate (usually outdated) Clang modules which are used by Swift for various reasons.

[  6] LLDB`(anonymous namespace)::CommandLineParser::addOption(llvm::cl::Option*, llvm::cl::SubCommand*) + 856
[  7] LLDB`(anonymous namespace)::CommandLineParser::addOption(llvm::cl::Option*, llvm::cl::SubCommand*) + 733
[  8] LLDB`(anonymous namespace)::CommandLineParser::addOption(llvm::cl::Option*, bool) + 184
[  9] LLDB`llvm::cl::ParseCommandLineOptions(...) [inlined] (anonymous namespace)::CommandLineParser::ParseCommandLineOptions(... + 1279
[  9] LLDB`llvm::cl::ParseCommandLineOptions(...) + 497
[ 10] LLDB`setCommandLineOpts(clang::CodeGenOptions const&) + 416
[ 11] LLDB`(anonymous namespace)::EmitAssemblyHelper::EmitAssemblyWithNewPassManager(...) + 98
[ 12] LLDB`clang::EmitBackendOutput(...) + 4580
[ 13] LLDB`(anonymous namespace)::PCHContainerGenerator::HandleTranslationUnit(clang::ASTContext&) + 871
[ 14] LLDB`clang::MultiplexConsumer::HandleTranslationUnit(clang::ASTContext&) + 43
[ 15] LLDB`clang::ParseAST(clang::Sema&, bool, bool) + 579
[ 16] LLDB`clang::FrontendAction::Execute() + 74
[ 17] LLDB`clang::CompilerInstance::ExecuteAction(clang::FrontendAction&) + 1808

The underlying reason for the crash is that the CommandLine code in LLVM isn't thread-safe and will never be thread-safe with its current
architecture. The way LLVM's CommandLine logic works is that all parts of the LLVM can provide command line arguments by defining
cl::opt global variables and their constructors (which are invoked during static initialisation) register the variable in LLVM's
CommandLineParser (which is also just a global variable). At some later point after static initialization we actually try to parse
command line arguments and we ask the CommandLineParser to parse our argv. The CommandLineParser then
lazily constructs it's internal parsing state in a non-thread-safe way (this is where the crash happens), parses the provided command line
and then goes back to the respective cl::opt global variables and sets their values according to the parse result.

As all of this is based on global state, this whole mechanism isn't thread-safe so the only time to ever use it is when we know we
only have one active thread dealing with LLVM logic. That's why nearly all callers of llvm::cl::ParseCommandLineOptions are at
the top of the main function of the some LLVM-based tool. One of the few exceptions to this rule is in the setCommandLineOpts
function in BackendUtil.cpp which is in our backtrace:

static void setCommandLineOpts(const CodeGenOptions &CodeGenOpts) {
  SmallVector<const char *, 16> BackendArgs;
  BackendArgs.push_back("clang"); // Fake program name.
  if (!CodeGenOpts.DebugPass.empty()) {
    BackendArgs.push_back("-debug-pass");
    BackendArgs.push_back(CodeGenOpts.DebugPass.c_str());
  }
  if (!CodeGenOpts.LimitFloatPrecision.empty()) {
    BackendArgs.push_back("-limit-float-precision");
    BackendArgs.push_back(CodeGenOpts.LimitFloatPrecision.c_str());
  }
  BackendArgs.push_back(nullptr);
  llvm::cl::ParseCommandLineOptions(BackendArgs.size() - 1,
                                    BackendArgs.data());
}

This is trying to set cl::opt variables in the LLVM backend to their right value as the passed via CodeGenOptions by invoking
the CommandLine parser. As this is just in some generic Clang CodeGen code (where we allow having multiple threads) this
is code is clearly wrong. If we're unlucky it either overwrites the value of the global variables or it causes the CommandLine
parser to crash.

So the next question is why is this only crashing in LLDB? The main reason seems to be that easiest way to crash this code is
to concurrently enter the initial CommandLineParser construction where it tries to collect all the registered cl::opt options
and checks for sanity:

// If it's a DefaultOption, check to make sure it isn't already there.
if (O->isDefaultOption() &&
    SC->OptionsMap.find(O->ArgStr) != SC->OptionsMap.end())
  return;

// Add argument to the argument map!
if (!SC->OptionsMap.insert(std::make_pair(O->ArgStr, O)).second) {
  errs() << ProgramName << ": CommandLine Error: Option '" << O->ArgStr
         << "' registered more than once!\n";
  HadErrors = true;
}

The OptionsMap here is global variable and if we end up in this code with two threads at once then two threads at the same
time can register an option (such as 'h') when they pass the first if and then we fail with the sanity check in the second if.

After this sanity check and initial setup code the only remaining work is just parsing the provided CommandLine which isn't
thread-safe but at least doesn't crash in all my attempts at breaking it (as it's usually just reading from the already generated
parser state but not further modifying it). The exception to this is probably that once people actually specify the options
in the code snippet above we might run into some new interesting ways to crash everything.

To go back to why it's only affecting LLDB: Nearly all LLVM tools I could find (even if they are using threads) seem to call the
CommandLine parser at the start so they all execute the initial parser setup at a point where there is only one thread. So
once the code above is executed they are mostly safe from the sanity check crashes. We even have some shady code for
the gtest main in TestMain.cpp which is why this also doesn't affect unit tests.

The only exception to this rule is ... *drum roll* ... LLDB! it's not using that CommandLine library for parsing options so it
also never ends up calling it in main. So when we end up in the FrontendAction code from the backtrace we are already
very deep in some LLDB logic and usually already have several threads. In a situation where Swift decides to compile a large
amount of Clang modules in parallel we then end up entering this code via several threads. If several threads reach this code
at the same time we end up in the situation where the sanity-checking code of CommandLine crashes. I have a very reliable
way of demonstrating the whole thing in D99650 (just run the unit test several times, it usually crashes after 3-4 attempts).

We have several ways to fix this:

  1. Make the whole CommandLine mechanism in LLVM thread-safe.
  2. Get rid of setCommandLineOpts in BackendUtil.cpp and other callers of the command line parsing in generic Clang code.
  3. Initialise the CommandLine library in a safe point in LLDB.

Option 1 is just a lot of work and I'm not even sure where to start. The whole mechanism is based on global variables and global state
and this seems like a humongous task.

Option 2 is probably the best thing we can do in the near future. There are only two callers of the command line parser in generic Clang
code. The one in BackendUtils.cpp looks like it can be replaced with some reasonable refactoring (as it only deals with two specific
options). There is another one in ExecuteCompilerInvocation which deals with forwarding the generic -mllvm options to the backend
which seems like it will just end up requiring us to do Option 1.

Option 3 is what this patch is doing. We just parse some dummy command line invocation in a point of the LLDB execution where we
only have one thread that is dealing with LLVM/Clang stuff. This way we are at least prevent the frequent crashes for users as parsing
the dummy command line invocation will set up the initial parser state safely.

Fixes rdar://70989856

Diff Detail

Event Timeline

teemperor requested review of this revision.Mar 31 2021, 4:44 AM
teemperor created this revision.
mib accepted this revision.Mar 31 2021, 10:51 AM

It's unfortunate the first 2 options are not feasible at the moment. Your implementation looks like a good compromise. Thanks for the thorough explanation.
LGTM!

This revision is now accepted and ready to land.Mar 31 2021, 10:51 AM
JDevlieghere accepted this revision.Mar 31 2021, 10:57 AM

Neat, thanks for getting to the bottom of this.

This revision was landed with ongoing or failed builds.Apr 1 2021, 11:19 AM
This revision was automatically updated to reflect the committed changes.