[Support] CommandLine.cpp - Fix thread race condition in addOption

[wenju]

Summary

If multiple threads executes addOption concurrently, there is a race condition between find and insert operation to SC->OptionsMap.

It is fixed by adding a mutex.

[aaron.ballman]

Thanks for the fix! This LGTM as far as the change goes, though I wonder if there are other race conditions hiding in this file. I'm not an expert in this particular area, so you should wait a few days before landing in case any of the other reviewers have comments (I added a few more folks who have touched this file in recent history).

[rnk]

I think the problem is larger than this fix implies. I don't think this spot fix is the right approach. In the long run, it will make it harder to implement a comprehensive design for thread safety. Today, one cannot safely concurrently construct cl::opts, because they modify the global options registry.

Even if synchronization is added at this point, aren't there still logical races between the thread registering new command line options and another thread parsing a command line?

To address your issue, I recommend that you register all cl::opts prior to launching threads, or add synchronization between the threads while registering options. Alternatively, I'd like to see more comprehensive synchronization.

llvm/lib/Support/CommandLine.cpp

182

Here is another modification of OptionsMap that would need to be protected if we want to convince ourselves that the new code is thread safe. I doubt this is the only one.

283

Here is another mutation which would need to be locked.

[rnk]

+ folks who were discussing command line parsing

[mehdi_amini]

I think the problem is larger than this fix implies. I don't think this spot fix is the right approach. In the long run, it will make it harder to implement a comprehensive design for thread safety.

That's my sentiment as well, I'm wary of just patching over without stepping back and looking at this a bit more globally.

[MaskRay]

Adding/deleting options is typically done in a separate initialization stage, and just once.
It is easier to ensure a single thread by application but pretty hard by the library.

https://lists.llvm.org/pipermail/llvm-dev/2018-October/127039.html ([RFC] Enable thread specific cl::opt values for multi-threaded support) is a related previous thread.
The patch D53424 was abandoned. Folks had concerns like these:

@mehdi_amini : The consensus at the time was that the cl::opt were only here for debugging / overriding default, but should not be the way any user of "LLVM as a library" set the options.
zturner: I can't deny that it solves a practical problem, but I'm mildly concerned that this is making a bad problem even worse.

cl::opt (and its friends like cl::list) does have some problems, but we need to figure out the needs and look at the big picture.

[wenju]

The issue is that DefaultOptions is special and delay-processed in CommandLineParser::ParseCommandLineOptions. Suppose the main thread calls llvm:🆑:ParseCommandLineOptions, then DefaultOptions is processed but it is not cleared after processing.
Multithreads calling setCommandLineOpts in clang/lib/CodeGen/BackendUtil.cpp will have race condition on repeated and redundant processing of DefaultOptions. This race condition (SC->OptionsMap) is also explained in https://reviews.llvm.org/D99652

I find this issue is fixed (workaround) by https://reviews.llvm.org/D99740

If we can assume that ParseCommandLineOptions will never be called from setCommandLineOpts (CodeGen/BackendUtil.cpp) simultaneously by multiple threads, then we don't need to fix DefaultOptions.

llvm/lib/Support/CommandLine.cpp

 #include "llvm/Support/raw_ostream.h"
 #include <cstdlib>
 #include <map>
+#include <mutex>
 #include <string>
 using namespace llvm;
 using namespace cl;

   void addOption(Option *O, SubCommand *SC) {
     bool HadErrors = false;
     if (O->hasArgStr()) {
+      // Guard find/insert operations to SC->OptionsMap.
+      std::lock_guard<std::mutex> Lock(OptionsMapMutex);
+
       // 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())

 
 private:
   SubCommand *ActiveSubCommand;
+  std::mutex OptionsMapMutex;
 
   Option *LookupOption(SubCommand &Sub, StringRef &Arg, StringRef &Value);
   Option *LookupLongOption(SubCommand &Sub, StringRef &Arg, StringRef &Value,