Make the static counters in ASTContext non-static

[alexfh]

Summary

Fixes a data race and makes it possible to run clang-based tools in multithreaded environment with TSan.

[riccibruno]

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

[ilya-biryukov]

In D58612#1408831, @riccibruno wrote:

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

+1, the only reason to use atomics here instead is if getting hold of an actual ASTContext is hard where the statistics are used (for legacy reasons?).

[riccibruno]

In D58612#1408836, @ilya-biryukov wrote:

In D58612#1408831, @riccibruno wrote:

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

+1, the only reason to use atomics here instead is if getting hold of an actual ASTContext is hard where the statistics are used (for legacy reasons?).

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.

[ilya-biryukov]

In D58612#1408839, @riccibruno wrote:

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.

[riccibruno]

In D58612#1408843, @ilya-biryukov wrote:

In D58612#1408839, @riccibruno wrote:

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.

Fair enough. Is that urgent ? I actually would be willing to look at this since I wanted to add some stats to track the space used for trailing objects anyway.

[alexfh]

Make the counters non-static members of ASTContext.

[alexfh]

In D58612#1408845, @riccibruno wrote:

In D58612#1408843, @ilya-biryukov wrote:

In D58612#1408839, @riccibruno wrote:

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.

Fair enough. Is that urgent ? I actually would be willing to look at this since I wanted to add some stats to track the space used for trailing objects anyway.

It turned out to be a trivial change. Thanks for the idea, btw ;)

[riccibruno]

Okay, but what about the other similar uses of static members which have the same problem ?

[alexfh]

In D58612#1408942, @riccibruno wrote:

Okay, but what about the other similar uses of static members which have the same problem ?

Do you have any example in mind? I've only seen TSan warnings for these counters, nothing else so far.

[riccibruno]

In D58612#1408991, @alexfh wrote:

In D58612#1408942, @riccibruno wrote:

Okay, but what about the other similar uses of static members which have the same problem ?

Do you have any example in mind? I've only seen TSan warnings for these counters, nothing else so far.

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.

[alexfh]

In D58612#1409024, @riccibruno wrote:

In D58612#1408991, @alexfh wrote:

In D58612#1408942, @riccibruno wrote:

Okay, but what about the other similar uses of static members which have the same problem ?

Do you have any example in mind? I've only seen TSan warnings for these counters, nothing else so far.

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.

In D58612#1409024, @riccibruno wrote:

In D58612#1408991, @alexfh wrote:

In D58612#1408942, @riccibruno wrote:

Okay, but what about the other similar uses of static members which have the same problem ?

Do you have any example in mind? I've only seen TSan warnings for these counters, nothing else so far.

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.

[riccibruno]

Sounds good. LGTM.

[ilya-biryukov]

LGTM!

[ilya-biryukov]

Awesome, I was also surprised it's so easy to convert them.

[riccibruno]

(With a commit message which actually reflect the change of course)

[alexfh]

In D58612#1409024, @riccibruno wrote:

...
For example in DeclBase.cpp

#define DECL(DERIVED, BASE) static int n##DERIVED##s = 0;
#define ABSTRACT_DECL(DECL)
#include "clang/AST/DeclNodes.inc"

...

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.

[riccibruno]

In D58612#1409215, @alexfh wrote:

In D58612#1409024, @riccibruno wrote:

...
For example in DeclBase.cpp

#define DECL(DERIVED, BASE) static int n##DERIVED##s = 0;
#define ABSTRACT_DECL(DECL)
#include "clang/AST/DeclNodes.inc"

...

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.

[alexfh]

In D58612#1409216, @riccibruno wrote:

In D58612#1409215, @alexfh wrote:

In D58612#1409024, @riccibruno wrote:

...
For example in DeclBase.cpp

#define DECL(DERIVED, BASE) static int n##DERIVED##s = 0;
#define ABSTRACT_DECL(DECL)
#include "clang/AST/DeclNodes.inc"

...

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.

cfe/trunk/include/clang/AST/ASTContext.h

public:
   APValue *getMaterializedTemporaryValue(const MaterializeTemporaryExpr *E,
                                          bool MayCreate);
 
   //===--------------------------------------------------------------------===//
   //                    Statistics
   //===--------------------------------------------------------------------===//
 
   /// The number of implicitly-declared default constructors.
-  static unsigned NumImplicitDefaultConstructors;
+  unsigned NumImplicitDefaultConstructors;
 
   /// The number of implicitly-declared default constructors for
   /// which declarations were built.
-  static unsigned NumImplicitDefaultConstructorsDeclared;
+  unsigned NumImplicitDefaultConstructorsDeclared;
 
   /// The number of implicitly-declared copy constructors.
-  static unsigned NumImplicitCopyConstructors;
+  unsigned NumImplicitCopyConstructors;
 
   /// The number of implicitly-declared copy constructors for
   /// which declarations were built.
-  static unsigned NumImplicitCopyConstructorsDeclared;
+  unsigned NumImplicitCopyConstructorsDeclared;
 
   /// The number of implicitly-declared move constructors.
-  static unsigned NumImplicitMoveConstructors;
+  unsigned NumImplicitMoveConstructors;
 
   /// The number of implicitly-declared move constructors for
   /// which declarations were built.
-  static unsigned NumImplicitMoveConstructorsDeclared;
+  unsigned NumImplicitMoveConstructorsDeclared;
 
   /// The number of implicitly-declared copy assignment operators.
-  static unsigned NumImplicitCopyAssignmentOperators;
+  unsigned NumImplicitCopyAssignmentOperators;
 
   /// The number of implicitly-declared copy assignment operators for
   /// which declarations were built.
-  static unsigned NumImplicitCopyAssignmentOperatorsDeclared;
+  unsigned NumImplicitCopyAssignmentOperatorsDeclared;
 
   /// The number of implicitly-declared move assignment operators.
-  static unsigned NumImplicitMoveAssignmentOperators;
+  unsigned NumImplicitMoveAssignmentOperators;
 
   /// The number of implicitly-declared move assignment operators for
   /// which declarations were built.
-  static unsigned NumImplicitMoveAssignmentOperatorsDeclared;
+  unsigned NumImplicitMoveAssignmentOperatorsDeclared;
 
   /// The number of implicitly-declared destructors.
-  static unsigned NumImplicitDestructors;
+  unsigned NumImplicitDestructors;
 
   /// The number of implicitly-declared destructors for which
   /// declarations were built.
-  static unsigned NumImplicitDestructorsDeclared;
+  unsigned NumImplicitDestructorsDeclared;
 
 public:
   /// Initialize built-in types.
   ///
   /// This routine may only be invoked once for a given ASTContext object.
   /// It is normally invoked after ASTContext construction.
   ///
   /// \param Target The target

cfe/trunk/lib/AST/ASTContext.cpp

 #include <map>
 #include <memory>
 #include <string>
 #include <tuple>
 #include <utility>
 
 using namespace clang;
 
-unsigned ASTContext::NumImplicitDefaultConstructors;
-unsigned ASTContext::NumImplicitDefaultConstructorsDeclared;
-unsigned ASTContext::NumImplicitCopyConstructors;
-unsigned ASTContext::NumImplicitCopyConstructorsDeclared;
-unsigned ASTContext::NumImplicitMoveConstructors;
-unsigned ASTContext::NumImplicitMoveConstructorsDeclared;
-unsigned ASTContext::NumImplicitCopyAssignmentOperators;
-unsigned ASTContext::NumImplicitCopyAssignmentOperatorsDeclared;
-unsigned ASTContext::NumImplicitMoveAssignmentOperators;
-unsigned ASTContext::NumImplicitMoveAssignmentOperatorsDeclared;
-unsigned ASTContext::NumImplicitDestructors;
-unsigned ASTContext::NumImplicitDestructorsDeclared;
-
 enum FloatingRank {
   Float16Rank, HalfRank, FloatRank, DoubleRank, LongDoubleRank, Float128Rank
 };
 
 RawComment *ASTContext::getRawCommentForDeclNoCache(const Decl *D) const {
   if (!CommentsLoaded && ExternalSource) {
     ExternalSource->ReadComments();
 

cfe/trunk/lib/Sema/SemaDeclCXX.cpp

 
 /// AddImplicitlyDeclaredMembersToClass - Adds any implicitly-declared
 /// special functions, such as the default constructor, copy
 /// constructor, or destructor, to the given C++ class (C++
 /// [special]p1).  This routine can only be executed just before the
 /// definition of the class is complete.
 void Sema::AddImplicitlyDeclaredMembersToClass(CXXRecordDecl *ClassDecl) {
   if (ClassDecl->needsImplicitDefaultConstructor()) {
-    ++ASTContext::NumImplicitDefaultConstructors;
+    ++getASTContext().NumImplicitDefaultConstructors;
 
     if (ClassDecl->hasInheritedConstructor())
       DeclareImplicitDefaultConstructor(ClassDecl);
   }
 
   if (ClassDecl->needsImplicitCopyConstructor()) {
-    ++ASTContext::NumImplicitCopyConstructors;
+    ++getASTContext().NumImplicitCopyConstructors;
 
     // If the properties or semantics of the copy constructor couldn't be
     // determined while the class was being declared, force a declaration
     // of it now.
     if (ClassDecl->needsOverloadResolutionForCopyConstructor() ||
         ClassDecl->hasInheritedConstructor())
       DeclareImplicitCopyConstructor(ClassDecl);
     // For the MS ABI we need to know whether the copy ctor is deleted. A
     // prerequisite for deleting the implicit copy ctor is that the class has a
     // move ctor or move assignment that is either user-declared or whose
     // semantics are inherited from a subobject. FIXME: We should provide a more
     // direct way for CodeGen to ask whether the constructor was deleted.
     else if (Context.getTargetInfo().getCXXABI().isMicrosoft() &&
              (ClassDecl->hasUserDeclaredMoveConstructor() ||
               ClassDecl->needsOverloadResolutionForMoveConstructor() ||
               ClassDecl->hasUserDeclaredMoveAssignment() ||
               ClassDecl->needsOverloadResolutionForMoveAssignment()))
       DeclareImplicitCopyConstructor(ClassDecl);
   }
 
   if (getLangOpts().CPlusPlus11 && ClassDecl->needsImplicitMoveConstructor()) {
-    ++ASTContext::NumImplicitMoveConstructors;
+    ++getASTContext().NumImplicitMoveConstructors;
 
     if (ClassDecl->needsOverloadResolutionForMoveConstructor() ||
         ClassDecl->hasInheritedConstructor())
       DeclareImplicitMoveConstructor(ClassDecl);
   }
 
   if (ClassDecl->needsImplicitCopyAssignment()) {
-    ++ASTContext::NumImplicitCopyAssignmentOperators;
+    ++getASTContext().NumImplicitCopyAssignmentOperators;
 
     // If we have a dynamic class, then the copy assignment operator may be
     // virtual, so we have to declare it immediately. This ensures that, e.g.,
     // it shows up in the right place in the vtable and that we diagnose
     // problems with the implicit exception specification.
     if (ClassDecl->isDynamicClass() ||
         ClassDecl->needsOverloadResolutionForCopyAssignment() ||
         ClassDecl->hasInheritedAssignment())
       DeclareImplicitCopyAssignment(ClassDecl);
   }
 
   if (getLangOpts().CPlusPlus11 && ClassDecl->needsImplicitMoveAssignment()) {
-    ++ASTContext::NumImplicitMoveAssignmentOperators;
+    ++getASTContext().NumImplicitMoveAssignmentOperators;
 
     // Likewise for the move assignment operator.
     if (ClassDecl->isDynamicClass() ||
         ClassDecl->needsOverloadResolutionForMoveAssignment() ||
         ClassDecl->hasInheritedAssignment())
       DeclareImplicitMoveAssignment(ClassDecl);
   }
 
   if (ClassDecl->needsImplicitDestructor()) {
-    ++ASTContext::NumImplicitDestructors;
+    ++getASTContext().NumImplicitDestructors;
 
     // If we have a dynamic class, then the destructor may be virtual, so we
     // have to declare the destructor immediately. This ensures that, e.g., it
     // shows up in the right place in the vtable and that we diagnose problems
     // with the implicit exception specification.
     if (ClassDecl->isDynamicClass() ||
         ClassDecl->needsOverloadResolutionForDestructor())
       DeclareImplicitDestructor(ClassDecl);
CXXConstructorDecl *Sema::DeclareImplicitDefaultConstructor(
 
   setupImplicitSpecialMemberType(DefaultCon, Context.VoidTy, None);
 
   // We don't need to use SpecialMemberIsTrivial here; triviality for default
   // constructors is easy to compute.
   DefaultCon->setTrivial(ClassDecl->hasTrivialDefaultConstructor());
 
   // Note that we have declared this constructor.
-  ++ASTContext::NumImplicitDefaultConstructorsDeclared;
+  ++getASTContext().NumImplicitDefaultConstructorsDeclared;
 
   Scope *S = getScopeForContext(ClassDecl);
   CheckImplicitSpecialMemberDeclaration(S, DefaultCon);
 
   if (ShouldDeleteSpecialMember(DefaultCon, CXXDefaultConstructor))
     SetDeclDeleted(DefaultCon, ClassLoc);
 
   if (S)
CXXDestructorDecl *Sema::DeclareImplicitDestructor(CXXRecordDecl *ClassDecl) {
 
   // We don't need to use SpecialMemberIsTrivial here; triviality for
   // destructors is easy to compute.
   Destructor->setTrivial(ClassDecl->hasTrivialDestructor());
   Destructor->setTrivialForCall(ClassDecl->hasAttr<TrivialABIAttr>() ||
                                 ClassDecl->hasTrivialDestructorForCall());
 
   // Note that we have declared this destructor.
-  ++ASTContext::NumImplicitDestructorsDeclared;
+  ++getASTContext().NumImplicitDestructorsDeclared;
 
   Scope *S = getScopeForContext(ClassDecl);
   CheckImplicitSpecialMemberDeclaration(S, Destructor);
 
   // We can't check whether an implicit destructor is deleted before we complete
   // the definition of the class, because its validity depends on the alignment
   // of the class. We'll check this from ActOnFields once the class is complete.
   if (ClassDecl->isCompleteDefinition() &&
CXXMethodDecl *Sema::DeclareImplicitCopyAssignment(CXXRecordDecl *ClassDecl) {
   CopyAssignment->setParams(FromParam);
 
   CopyAssignment->setTrivial(
     ClassDecl->needsOverloadResolutionForCopyAssignment()
       ? SpecialMemberIsTrivial(CopyAssignment, CXXCopyAssignment)
       : ClassDecl->hasTrivialCopyAssignment());
 
   // Note that we have added this copy-assignment operator.
-  ++ASTContext::NumImplicitCopyAssignmentOperatorsDeclared;
+  ++getASTContext().NumImplicitCopyAssignmentOperatorsDeclared;
 
   Scope *S = getScopeForContext(ClassDecl);
   CheckImplicitSpecialMemberDeclaration(S, CopyAssignment);
 
   if (ShouldDeleteSpecialMember(CopyAssignment, CXXCopyAssignment))
     SetDeclDeleted(CopyAssignment, ClassLoc);
 
   if (S)
CXXMethodDecl *Sema::DeclareImplicitMoveAssignment(CXXRecordDecl *ClassDecl) {
   MoveAssignment->setParams(FromParam);
 
   MoveAssignment->setTrivial(
     ClassDecl->needsOverloadResolutionForMoveAssignment()
       ? SpecialMemberIsTrivial(MoveAssignment, CXXMoveAssignment)
       : ClassDecl->hasTrivialMoveAssignment());
 
   // Note that we have added this copy-assignment operator.
-  ++ASTContext::NumImplicitMoveAssignmentOperatorsDeclared;
+  ++getASTContext().NumImplicitMoveAssignmentOperatorsDeclared;
 
   Scope *S = getScopeForContext(ClassDecl);
   CheckImplicitSpecialMemberDeclaration(S, MoveAssignment);
 
   if (ShouldDeleteSpecialMember(MoveAssignment, CXXMoveAssignment)) {
     ClassDecl->setImplicitMoveAssignmentIsDeleted();
     SetDeclDeleted(MoveAssignment, ClassLoc);
   }
CXXConstructorDecl *Sema::DeclareImplicitCopyConstructor(
   CopyConstructor->setTrivialForCall(
       ClassDecl->hasAttr<TrivialABIAttr>() ||
       (ClassDecl->needsOverloadResolutionForCopyConstructor()
            ? SpecialMemberIsTrivial(CopyConstructor, CXXCopyConstructor,
              TAH_ConsiderTrivialABI)
            : ClassDecl->hasTrivialCopyConstructorForCall()));
 
   // Note that we have declared this constructor.
-  ++ASTContext::NumImplicitCopyConstructorsDeclared;
+  ++getASTContext().NumImplicitCopyConstructorsDeclared;
 
   Scope *S = getScopeForContext(ClassDecl);
   CheckImplicitSpecialMemberDeclaration(S, CopyConstructor);
 
   if (ShouldDeleteSpecialMember(CopyConstructor, CXXCopyConstructor)) {
     ClassDecl->setImplicitCopyConstructorIsDeleted();
     SetDeclDeleted(CopyConstructor, ClassLoc);
   }
CXXConstructorDecl *Sema::DeclareImplicitMoveConstructor(
   MoveConstructor->setTrivialForCall(
       ClassDecl->hasAttr<TrivialABIAttr>() ||
       (ClassDecl->needsOverloadResolutionForMoveConstructor()
            ? SpecialMemberIsTrivial(MoveConstructor, CXXMoveConstructor,
                                     TAH_ConsiderTrivialABI)
            : ClassDecl->hasTrivialMoveConstructorForCall()));
 
   // Note that we have declared this constructor.
-  ++ASTContext::NumImplicitMoveConstructorsDeclared;
+  ++getASTContext().NumImplicitMoveConstructorsDeclared;
 
   Scope *S = getScopeForContext(ClassDecl);
   CheckImplicitSpecialMemberDeclaration(S, MoveConstructor);
 
   if (ShouldDeleteSpecialMember(MoveConstructor, CXXMoveConstructor)) {
     ClassDecl->setImplicitMoveConstructorIsDeleted();
     SetDeclDeleted(MoveConstructor, ClassLoc);
   }