This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by wenju on Jul 5 2021, 11:57 PM.

Details

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.

Diff Detail

Event Timeline

wenju created this revision.Jul 5 2021, 11:57 PM
wenju requested review of this revision.Jul 5 2021, 11:57 PM
Herald added a project: Restricted Project. Β· View Herald TranscriptJul 5 2021, 11:57 PM
Herald added a subscriber: llvm-commits. Β· View Herald Transcript
wenju updated this revision to Diff 357132.Jul 7 2021, 10:21 PM
aaron.ballman accepted this revision.Jul 8 2021, 3:54 AM
aaron.ballman added reviewers: hokein, rnk.

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).

This revision is now accepted and ready to land.Jul 8 2021, 3:54 AM
rnk requested changes to this revision.Jul 8 2021, 8:40 AM

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.

This revision now requires changes to proceed.Jul 8 2021, 8:40 AM

+ folks who were discussing command line parsing

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.

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 added a comment.Jul 28 2021, 6:54 PM

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.

wenju abandoned this revision.Aug 9 2021, 6:13 PM