[PATCH] D53424: Enable thread specific cl::opt values for multi-threaded support

[Dan Liew via Phabricator via llvm-commits]

delcypher added a comment.

In https://reviews.llvm.org/D53424#1273737, @yrouban wrote:

> In https://reviews.llvm.org/D53424#1273728, @jfb wrote:
>
> > Was there a commitment from the community, with some time horizon for moving away from this patch?
>
>
> Frankly speaking, I do not think that this kind of transition can be done easily. Every single option needs a big change. That is the biggest disadvantage of the feature https://reviews.llvm.org/D5389. It needs some improvements to allow enough flexibility. And we cannot know what improvements are needed until we implement enough options in the new way. I do not see any good way to force the transition to finish in a fixed time frame. If https://reviews.llvm.org/D5389 were easy to use and transit to, it would have been already adopted and we would not have to came up with another idea like this https://reviews.llvm.org/D53424.
>  In other words, I would give the new idea a try (at least under a preprocessor flag).


While I agree that a transition away from global `cl::opt`s is going to be tough I think we need to get agreement from the community on some sort of plan before we land this. Otherwise we'll end up with this patch becoming a permanent part of LLVM's internal API which makes our technical debt even worse.
My suggestion would be to create a new RFC on llvm-dev that proposes a plan to move away from `cl::opt`s in LLVM's codebase (and probably other sub-projects). Given that you're looking at this issue you're probably in good position to discuss the technical problems with the available replacement APIs in LLVM's codebase.
You could sketch a simple patch for an existing pass that shows where the existing APIs work well and then discuss an example where the existing API is insufficient.

On a related note I think it's worth mentioning how I handled the global `cl::opt` problem in my own project. The strategy I used might be applicable to some parts of LLVM's codebase too. The strategy is as follows.

- For the class/struct of interest, a corresponding options struct is created (e.g. https://github.com/delcypher/jfs/blob/master/include/jfs/CXXFuzzingBackend/ClangOptions.h). The users of these options use a pointer to these options and never use `cl::opts` directly (apart from occasional internal debugging stuff which is not meant to be part of the API). Here's the user in my project https://github.com/delcypher/jfs/blob/master/include/jfs/CXXFuzzingBackend/CXXFuzzingSolverOptions.h .
- The library containing the class/struct of interest (in this case `JFSCXXFuzzingBackend`) doesn't use `cl::opt` at all. Instead there is a corresponding library (in this case `JFSCXXFuzzingBackendCmdLine`) that declares the `cl::opt` for command line use and contains a helper function (`buildClangOptionsFromCmdLine(...)` ) that constructs the option struct from the command line options (see https://github.com/delcypher/jfs/blob/master/lib/CXXFuzzingBackend/CmdLine/ClangOptionsBuilder.cpp ). This separation means that if clients of my libraries don't want to use the command line to initialise `ClangOptions` they just link against `JFSCXXFuzzingBackend` and initialise an instance of `CXXFuzzingSolverOptions` directly using the API (no global `cl::opts` are pulled into clients code). If a client of the libraries **does want to initialise `ClangOptions` from the command line** they link against JFSCXXFuzzingBackend` and  `JFSCXXFuzzingBackendCmdLine`. The latter library adds the global `cl::opt`s and the helper function to initialise the options struct from the `cl::opt`s.

This approach does have some disadvantages:

- It still uses global `cl::opt`s. This could be fixed by encapsulating these `cl::opts` into a class that is ensured to live long enough to do command line option parsing.
- The separation of libraries only works for static libraries. If you build a dynamic library everything has to be included which means the global `cl::opt`s get pulled in.
- The options struct and the global `cl::opts`s duplicate option default values and storage. This duplication then means that the values read on the command line need to be copied over to the options struct which is wasteful and maintaining two copies of the default values is a pain.



================
Comment at: unittests/Support/CommandLineTest.cpp:25
 
+#ifdef HAVE_PTHREAD_H
+#include <chrono>
----------------
This unit test needs to work on Windows. I see no good reason to use pthread here. Can't you just use `std::thread` instead?
Also guarding `chrono` and `thread` headers on pthread being available seems wrong given that those headers will be available on platforms without pthreads.


Repository:
  rL LLVM

https://reviews.llvm.org/D53424