Fixes a data race and makes it possible to run clang-based tools in multithreaded environment with TSan.
Hmm. These are not the only static variables which are used for statistics (eg: in DeclBase.cpp). Would it make sense instead to keep all of the statistics in the AST context (without making them atomic) ?
I don't know how hard it would be to do this, but I would like to argue that this should be done even if it require some refactoring. These static variables used for stats are kind of ugly imho; they conceptually belong to the AST context.
I agree, OTOH if refactoring would take more time than one can spare, moving them to atomics is better than what we have now.
For example, AFAIK that's the only thing that keeps clang from being tsan-clean.
For example in DeclBase.cpp
#define DECL(DERIVED, BASE) static int n##DERIVED##s = 0; #define ABSTRACT_DECL(DECL) #include "clang/AST/DeclNodes.inc"
which count the number of declaration node of each kind.
These are probably easier to convert to std::atomic<int>, but I'd do this separately.
Just converting these to std::atomic<> will mute TSan, but won't fix the underlying issue: the stats should be collected per-compiler instance. Initialization will require a bit more locking as well. Not sure what would be a good solution here.
I agree that making them atomic is not a good solution. I am not sure that the stats should be stored in the compiler instance (is that what you are suggesting ?). The number of declaration nodes of each kind seems to be something that should be stored in the AST context.
Well, I wasn't talking about storing the metrics in the CompilerInstance class directly. Maybe in ASTContext. But after looking a bit closer I see that the problem is that an instance of ASTContext is not readily available where these stats are gathered. I'm not sure I'll get to change this any time soon though.