[clang] Update NumFunctionDeclBits for FunctionDeclBitfields

[danix800]

Summary

FunctionDeclBitfields.NumFunctionDeclBits is not updated when DeductionCandidateKind is incremented.
CXXConstructorDeclBitfields.NumCtorInitializers is shrunk as needed.

Fixes https://github.com/llvm/llvm-project/issues/64171

[danix800]

Add alternative testcase.

[cor3ntin]

For context this seems to be the patch that did not increment the bit field https://reviews.llvm.org/D139837

[cor3ntin]

Thanks for working on this!
Looks good to me except for a couple typos.
Can you an an entry in clang/docs/ReleaseNotes.rst (mentioning the github issue)

clang/unittests/AST/ASTImporterTest.cpp

7836

ImportFunctionDeclBitShouldNotOverwriteCtorDeclBits

clang/unittests/AST/DeclTest.cpp

356 ↗

(On Diff #551016)

[danix800]

Can you an an entry in clang/docs/ReleaseNotes.rst (mentioning the github issue)

No problem!

[danix800]

clang/unittests/AST/ASTImporterTest.cpp

7836

I added the case into DeclTest.cpp because i think it can cover the root cause for both testcases.

How about remove the somewhat repeated one in ASTImporterTest.cpp?

[danix800]

I also investigated whether we could count those bits at compile time and statically assert on them,
because a small typo or missed update could spend us a lot of time to dig for the cause.

My first step is trying to count number of bits for a single bitfield, this is promising based on this but
with a restriction, it only works on struct (default public fields), not class (default to private fields).

If we can implement this bitsizeof then we could have:

enum { NumFunctionDeclBits = offsetof(FunctionDeclBitfields, SClass)
                           + offsetof(FunctionDeclBitfields, IsInline)
                           + ... };

This can automatically update total number of bits if any of the existing one is updated.

The second step is trying to enumerate all bit fields at compile time so that we can totally fix this kind
of issue, but it seems not possible.

Any suggestions or advices? Is it even worth it to do it like this?

[cor3ntin]

In D158145#4594549, @danix800 wrote:

I also investigated whether we could count those bits at compile time and statically assert on them,
because a small typo or missed update could spend us a lot of time to dig for the cause.

My first step is trying to count number of bits for a single bitfield, this is promising based on this but
with a restriction, it only works on struct (default public fields), not class (default to private fields).

If we can implement this bitsizeof then we could have:

enum { NumFunctionDeclBits = offsetof(FunctionDeclBitfields, SClass)
                           + offsetof(FunctionDeclBitfields, IsInline)
                           + ... };

This can automatically update total number of bits if any of the existing one is updated.

The second step is trying to enumerate all bit fields at compile time so that we can totally fix this kind
of issue, but it seems not possible.

Any suggestions or advices? Is it even worth it to do it like this?

It would be great to be able to stattic_assert these sort of bugs. I think @aaron did some research in this area, it might be worth asking him how far he got!

clang/unittests/AST/ASTImporterTest.cpp

7836

Now that you have written it, we can leave both tests i think.
Afaik one test serialization so they complement each other

[danix800]

clang/unittests/AST/ASTImporterTest.cpp

7836

OK! :-)

[danix800]

1. Update ReleaseNotes.rst
2. Fix typos pointed out by @cor3ntin (thanks!)

[balazske]

clang/unittests/AST/DeclTest.cpp

368 ↗

(On Diff #551037)

This dump is not needed?

[danix800]

clang/unittests/AST/DeclTest.cpp

368 ↗

(On Diff #551037)

Yeah it's for debug only, I'll remove it.

[danix800]

Remove debug code.

[aaron.ballman]

In D158145#4594569, @cor3ntin wrote:

In D158145#4594549, @danix800 wrote:

I also investigated whether we could count those bits at compile time and statically assert on them,
because a small typo or missed update could spend us a lot of time to dig for the cause.

My first step is trying to count number of bits for a single bitfield, this is promising based on this but
with a restriction, it only works on struct (default public fields), not class (default to private fields).

If we can implement this bitsizeof then we could have:

enum { NumFunctionDeclBits = offsetof(FunctionDeclBitfields, SClass)
                           + offsetof(FunctionDeclBitfields, IsInline)
                           + ... };

This can automatically update total number of bits if any of the existing one is updated.

The second step is trying to enumerate all bit fields at compile time so that we can totally fix this kind
of issue, but it seems not possible.

Any suggestions or advices? Is it even worth it to do it like this?

It would be great to be able to stattic_assert these sort of bugs. I think @aaron did some research in this area, it might be worth asking him how far he got!

I was never able to find a conforming way to do it without implementing something like a "bitwidth of" operator, which is something I was planning to propose to WG14 anyway for use with _BitInt. I've not implemented the extension yet though, and even if I did, it wouldn't help us with non-Clang host compilers, so I eventually gave up trying to automatically calculate the values.

Changes LGTM!

clang/include/clang/AST/DeclBase.h

     uint64_t UsesFPIntrin : 1;
 
     // Indicates this function is a constrained friend, where the constraint
     // refers to an enclosing template for hte purposes of [temp.friend]p9.
     uint64_t FriendConstraintRefersToEnclosingTemplate : 1;
   };
 
   /// Number of non-inherited bits in FunctionDeclBitfields.
-  enum { NumFunctionDeclBits = 30 };
+  enum { NumFunctionDeclBits = 31 };
 
   /// Stores the bits used by CXXConstructorDecl. If modified
   /// NumCXXConstructorDeclBits and the accessor
   /// methods in CXXConstructorDecl should be updated appropriately.
   class CXXConstructorDeclBitfields {
     friend class CXXConstructorDecl;
     /// For the bits in DeclContextBitfields.
     uint64_t : NumDeclContextBits;
     /// For the bits in FunctionDeclBitfields.
     uint64_t : NumFunctionDeclBits;
 
-    /// 21 bits to fit in the remaining available space.
+    /// 20 bits to fit in the remaining available space.
     /// Note that this makes CXXConstructorDeclBitfields take
     /// exactly 64 bits and thus the width of NumCtorInitializers
     /// will need to be shrunk if some bit is added to NumDeclContextBitfields,
     /// NumFunctionDeclBitfields or CXXConstructorDeclBitfields.
-    uint64_t NumCtorInitializers : 18;
+    uint64_t NumCtorInitializers : 17;
     uint64_t IsInheritingConstructor : 1;
 
     /// Whether this constructor has a trail-allocated explicit specifier.
     uint64_t HasTrailingExplicitSpecifier : 1;
     /// If this constructor does't have a trail-allocated explicit specifier.
     /// Whether this constructor is explicit specified.
     uint64_t IsSimpleExplicit : 1;
   };

clang/lib/Serialization/ASTWriterDecl.cpp

     Record.AddStmt(Info->TrailingRequiresClause);
   }
   // The location information is deferred until the end of the record.
   Record.AddTypeRef(D->getTypeSourceInfo() ? D->getTypeSourceInfo()->getType()
                                            : QualType());
 }
 
 void ASTDeclWriter::VisitFunctionDecl(FunctionDecl *D) {
-  static_assert(DeclContext::NumFunctionDeclBits == 30,
+  static_assert(DeclContext::NumFunctionDeclBits == 31,
                 "You need to update the serializer after you change the "
                 "FunctionDeclBits");
 
   VisitRedeclarable(D);
 
   Record.push_back(D->getTemplatedKind());
   switch (D->getTemplatedKind()) {
   case FunctionDecl::TK_NonTemplate:

       D->hasWrittenPrototype() &&
       D->getTemplatedKind() == FunctionDecl::TK_NonTemplate)
     AbbrevToUse = Writer.getDeclCXXMethodAbbrev();
 
   Code = serialization::DECL_CXX_METHOD;
 }
 
 void ASTDeclWriter::VisitCXXConstructorDecl(CXXConstructorDecl *D) {
-  static_assert(DeclContext::NumCXXConstructorDeclBits == 21,
+  static_assert(DeclContext::NumCXXConstructorDeclBits == 20,
                 "You need to update the serializer after you change the "
                 "CXXConstructorDeclBits");
 
   Record.push_back(D->getTrailingAllocKind());
   addExplicitSpecifier(D->getExplicitSpecifier(), Record);
   if (auto Inherited = D->getInheritedConstructor()) {
     Record.AddDeclRef(Inherited.getShadowDecl());
     Record.AddDeclRef(Inherited.getConstructor());

clang/unittests/AST/ASTImporterTest.cpp

 
   auto *ToC = Import(FromC, Lang_CXX11);
   TranslationUnitDecl *ToTU = ToC->getTranslationUnitDecl();
 
   CheckAST(FromTU, FromC);
   CheckAST(ToTU, ToC);
 }
 
+TEST_P(ASTImporterOptionSpecificTestBase,
+    ImportFunctionDeclBitShouldNotStampingOnCtorDeclBits) {
+  Decl *From, *To;
+  std::tie(From, To) = getImportedDecl(
+      R"s(
+        struct A {
+          A() : m() {}
+          int m;
+        };
+
+        A foo() { A a; return a; }
+        A bar() { return {}; }
+      )s",
+      Lang_CXX17,
+      R"s(
+        struct A {
+          A() : m() {}
+          int m;
+        };
+        A baz() { return {}; }
+      )s",
+      Lang_CXX17, "A");
+
+  auto HasCtorInit =
+      hasAnyConstructorInitializer(cxxCtorInitializer(isMemberInitializer()));
+  auto ImpMoveCtor =
+      cxxConstructorDecl(isMoveConstructor(), isImplicit(), HasCtorInit);
+
+  auto *FromImpMoveCtor = FirstDeclMatcher<CXXConstructorDecl>().match(
+      From, ImpMoveCtor);
+  auto *ToImpMoveCtor = FirstDeclMatcher<CXXConstructorDecl>().match(
+      To, ImpMoveCtor);
+
+  EXPECT_TRUE(FromImpMoveCtor->getNumCtorInitializers() == 1);
+  EXPECT_FALSE(FromImpMoveCtor->FriendConstraintRefersToEnclosingTemplate());
+
+  EXPECT_TRUE(ToImpMoveCtor->getNumCtorInitializers() == 1);
+  EXPECT_FALSE(ToImpMoveCtor->FriendConstraintRefersToEnclosingTemplate());
+  EXPECT_TRUE(*ToImpMoveCtor->init_begin());
+}
+
 AST_MATCHER_P(UsingShadowDecl, hasIntroducerDecl, internal::Matcher<NamedDecl>,
               InnerMatcher) {
   return InnerMatcher.matches(*Node.getIntroducer(), Finder, Builder);
 }
 
 TEST_P(ASTImporterOptionSpecificTestBase,
        ImportConstructorUsingShadowVirtualBase) {
   TranslationUnitDecl *FromTU = getTuDecl(