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

[Reid Kleckner via Phabricator via llvm-commits]

rnk requested changes to this revision.
rnk added a comment.
This revision now requires changes to proceed.

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.



================
Comment at: llvm/lib/Support/CommandLine.cpp:182
       return;
     if (!SC->OptionsMap.insert(std::make_pair(Name, &Opt)).second) {
       errs() << ProgramName << ": CommandLine Error: Option '" << Name
----------------
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.


================
Comment at: llvm/lib/Support/CommandLine.cpp:283
       if (I != End && I->getValue() == O)
         Sub.OptionsMap.erase(I);
     }
----------------
Here is another mutation which would need to be locked.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D105461/new/

https://reviews.llvm.org/D105461