[modules] Add a flag for TagDecl if it was a definition demoted to a declaration.

[vsapsai]

Summary

For redeclaration chains we maintain an invariant of having only a
single definition in the chain. In a single translation unit we make
sure not to create duplicates. But modules are separate translation
units and they can contain definitions for the same symbol
independently. When we load such modules together, we need to demote
duplicate definitions to keep the AST invariants.

Some AST clients are interested in distinguishing
declaration-that-was-demoted-from-definition and
declaration-that-was-never-a-definition. For that purpose introducing
IsThisDeclarationADemotedDefinition. No functional change intended.

rdar://84677782

[vsapsai]

Relevant discussion on how this is used in Swift is in https://github.com/apple/swift/pull/41144 Also it contains links to various [successful] test runs.

[vsapsai]

libomptarget :: x86_64-pc-linux-gnu :: mapping/delete_inf_refcount.c has failed due to

/usr/bin/ld: final link failed: bad value

As far as I can tell, it's not related to my change.

[vsapsai]

Ping. Swift PR testing is happy with this change and I've tested it successfully on >100 swift projects. So looks like this change is safe and does what it intends to do.

[Bigcheese]

lgtm.

[delcypher]

Change seems reasonable but I don't have expertise on this code. I've left a few minor nits.

clang/include/clang/AST/Decl.h

3494

This name feels a little clunky. How about isDemotedDefinition()?

3500

Given the name of this function (suggesting it always demotes) it probably should

assert(!isThisDeclarationADemotedDefinition() && "decl already demoted");

alternatively comments saying the operation is idempotent is probably fine too.

3500

The demoteThisDefinitionToDeclaration() name feels a little bit clunky. How about demoteDefinitionToDecl()?

[vsapsai]

Thanks for the review, Dan!

clang/include/clang/AST/Decl.h

1383–1398

Naming for my change is mimicking this API.

3494

I agree the name is awkward but I am using the same name as NonParmVarDecl does. Based on similarities in implementation I think the similarities in the name are useful. Also we have isThisDeclarationADefinition in multiple places, so it is good to follow the same naming pattern, even if it is clunky.

3500

Interesting suggestion, made me think about what exactly I'm trying to do here. The strict requirement is not to set the flag on forward declarations (aka non-definitions). To achieve that the alternative would be

if (!isCompleteDefinition())
  return;
setCompleteDefinition(false);
TagDeclBits.IsThisDeclarationADemotedDefinition = true;

Which is similar to the current code with the assertion. Demoting already demoted definition again is acceptable but there is really no need for that.

I agree the assertion makes the code less robust but its purpose is to enforce expected API usage, so being picky about the way it is called is intended. Though let me change the assertion message to something more actionable rather than state-of-the-world-observation.

[vsapsai]

Update assertion message wording to be more actionable.

[vsapsai]

In D118855#3320064, @Bigcheese wrote:

lgtm.

Thanks for the review, Michael! I'll give some time to the pre-merge checks to run, then will land the change.

clang/include/clang/AST/Decl.h

InitializationStyle getInitStyle() const {
     return static_cast<InitializationStyle>(VarDeclBits.InitStyle);
   }
 
   /// Whether the initializer is a direct-initializer (list or call).
   bool isDirectInit() const {
     return getInitStyle() != CInit;
   }
 
   /// If this definition should pretend to be a declaration.
   bool isThisDeclarationADemotedDefinition() const {
     return isa<ParmVarDecl>(this) ? false :
       NonParmVarDeclBits.IsThisDeclarationADemotedDefinition;
   }
 
   /// This is a definition which should be demoted to a declaration.
   ///
   /// In some cases (mostly module merging) we can end up with two visible
   /// definitions one of which needs to be demoted to a declaration to keep
   /// the AST invariants.
   void demoteThisDefinitionToDeclaration() {
     assert(isThisDeclarationADefinition() && "Not a definition!");
     assert(!isa<ParmVarDecl>(this) && "Cannot demote ParmVarDecls!");
     NonParmVarDeclBits.IsThisDeclarationADemotedDefinition = 1;
   }
 
   /// Determine whether this variable is the exception variable in a
   /// C++ catch statememt or an Objective-C \@catch statement.
   bool isExceptionVariable() const {
     return isa<ParmVarDecl>(this) ? false : NonParmVarDeclBits.ExceptionVar;
   }
   void setExceptionVariable(bool EV) {
     assert(!isa<ParmVarDecl>(this));
public:
   /// This option is only enabled when modules are enabled.
   bool mayHaveOutOfDateDef() const { return TagDeclBits.MayHaveOutOfDateDef; }
 
   /// Whether this declaration declares a type that is
   /// dependent, i.e., a type that somehow depends on template
   /// parameters.
   bool isDependentType() const { return isDependentContext(); }
 
+  /// Whether this declaration was a definition in some module but was forced
+  /// to be a declaration.
+  ///
+  /// Useful for clients checking if a module has a definition of a specific
+  /// symbol and not interested in the final AST with deduplicated definitions.
+  bool isThisDeclarationADemotedDefinition() const {
+    return TagDeclBits.IsThisDeclarationADemotedDefinition;
+  }
+
+  /// Mark a definition as a declaration and maintain information it _was_
+  /// a definition.
+  void demoteThisDefinitionToDeclaration() {
+    assert(isCompleteDefinition() &&
+           "Should demote definitions only, not forward declarations");
+    setCompleteDefinition(false);
+    TagDeclBits.IsThisDeclarationADemotedDefinition = true;
+  }
+
   /// Starts the definition of this tag declaration.
   ///
   /// This method should be invoked at the beginning of the definition
   /// of this tag declaration. It will set the tag type into a state
   /// where it is in the process of being defined.
   void startDefinition();
 
   /// Returns the TagDecl that actually defines this

clang/include/clang/AST/DeclBase.h

class TagDeclBitfields {
     /// to have an out-of-date definition.
     ///
     /// This option is only enabled when modules are enabled.
     uint64_t MayHaveOutOfDateDef : 1;
 
     /// Has the full definition of this type been required by a use somewhere in
     /// the TU.
     uint64_t IsCompleteDefinitionRequired : 1;
+
+    /// Whether this tag is a definition which was demoted due to
+    /// a module merge.
+    uint64_t IsThisDeclarationADemotedDefinition : 1;
   };
 
   /// Number of non-inherited bits in TagDeclBitfields.
-  enum { NumTagDeclBits = 9 };
+  enum { NumTagDeclBits = 10 };
 
   /// Stores the bits used by EnumDecl.
   /// If modified NumEnumDeclBit and the accessor
   /// methods in EnumDecl should be updated appropriately.
   class EnumDeclBitfields {
     friend class EnumDecl;
     /// For the bits in DeclContextBitfields.
     uint64_t : NumDeclContextBits;

clang/lib/AST/Decl.cpp

assert((DK != Enum || TK == TTK_Enum) &&
          "EnumDecl not matched with TTK_Enum");
   setPreviousDecl(PrevDecl);
   setTagKind(TK);
   setCompleteDefinition(false);
   setBeingDefined(false);
   setEmbeddedInDeclarator(false);
   setFreeStanding(false);
   setCompleteDefinitionRequired(false);
+  TagDeclBits.IsThisDeclarationADemotedDefinition = false;
 }
 
 SourceLocation TagDecl::getOuterLocStart() const {
   return getTemplateOrInnerLocStart(this);
 }
 
 SourceRange TagDecl::getSourceRange() const {
   SourceLocation RBraceLoc = BraceRange.getEnd();

clang/lib/Serialization/ASTReaderDecl.cpp

if (!OldDef) {
         if (!D->isFromASTFile() && D->isCompleteDefinition()) {
           OldDef = D;
           break;
         }
       }
     }
     if (OldDef) {
       Reader.MergedDeclContexts.insert(std::make_pair(ED, OldDef));
-      ED->setCompleteDefinition(false);
+      ED->demoteThisDefinitionToDeclaration();
       Reader.mergeDefinitionVisibility(OldDef, ED);
       if (OldDef->getODRHash() != ED->getODRHash())
         Reader.PendingEnumOdrMergeFailures[OldDef].push_back(ED);
     } else {
       OldDef = ED;
     }
   }
 
if (!OldDef) {
         if (!D->isFromASTFile() && D->isCompleteDefinition()) {
           OldDef = D;
           break;
         }
       }
     }
     if (OldDef) {
       Reader.MergedDeclContexts.insert(std::make_pair(RD, OldDef));
-      RD->setCompleteDefinition(false);
+      RD->demoteThisDefinitionToDeclaration();
       Reader.mergeDefinitionVisibility(OldDef, RD);
     } else {
       OldDef = RD;
     }
   }
 }
 
 void ASTDeclReader::VisitValueDecl(ValueDecl *VD) {