[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

[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

This dump is not needed?

[danix800]

clang/unittests/AST/DeclTest.cpp

368

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/docs/ReleaseNotes.rst

- Fix cases where we ignore ambiguous name lookup when looking up memebers.
   (`#29942 <https://github.com/llvm/llvm-project/issues/29942>`_),
   (`#35574 <https://github.com/llvm/llvm-project/issues/35574>`_) and
   (`#27224 <https://github.com/llvm/llvm-project/issues/27224>`_).
 
 - Clang emits an error on substitution failure within lambda body inside a
   requires-expression. This fixes:
   (`#64138 <https://github.com/llvm/llvm-project/issues/64138>`_).
 
+- Update ``FunctionDeclBitfields.NumFunctionDeclBits``. This fixes:
+  (`#64171 <https://github.com/llvm/llvm-project/issues/64171>`_).
+
 Bug Fixes to AST Handling
 ^^^^^^^^^^^^^^^^^^^^^^^^^
 - Fixed an import failure of recursive friend class template.
   `Issue 64169 <https://github.com/llvm/llvm-project/issues/64169>`_
 - Remove unnecessary RecordLayout computation when importing UnaryOperator. The
   computed RecordLayout is incorrect if fields are not completely imported and
   should not be cached.
   `Issue 64170 <https://github.com/llvm/llvm-project/issues/64170>`_

clang/include/clang/AST/DeclBase.h

class FunctionDeclBitfields {
     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

if (D->hasExtInfo()) {
     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:
if (D->getDeclContext() == D->getLexicalDeclContext() &&
       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

TEST_P(ASTImporterOptionSpecificTestBase, ImportConstructorUsingShadow) {
 
   auto *ToC = Import(FromC, Lang_CXX11);
   TranslationUnitDecl *ToTU = ToC->getTranslationUnitDecl();
 
   CheckAST(FromTU, FromC);
   CheckAST(ToTU, ToC);
 }
 
+TEST_P(ASTImporterOptionSpecificTestBase,
+       ImportFunctionDeclBitShouldNotOverwriteCtorDeclBits) {
+  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(

clang/unittests/AST/DeclTest.cpp

TEST(Decl, FriendFunctionWithinClassInHeaderUnit) {
 
   auto *getFooValue = selectFirst<FunctionDecl>(
       "getFooValue",
       match(functionDecl(hasName("getFooValue")).bind("getFooValue"), Ctx));
 
   EXPECT_TRUE(getFooValue->isInlined());
 }
 
+TEST(Decl, FunctionDeclBitsShouldNotOverlapWithCXXConstructorDeclBits) {
+  llvm::Annotations Code(R"(
+    struct A {
+      A() : m() {}
+      int m;
+    };
+
+    A f() { return A(); }
+    )");
+
+  auto AST = tooling::buildASTFromCodeWithArgs(Code.code(), {"-std=c++14"});
+  ASTContext &Ctx = AST->getASTContext();
+
+  auto HasCtorInit =
+      hasAnyConstructorInitializer(cxxCtorInitializer(isMemberInitializer()));
+  auto ImpMoveCtor =
+      cxxConstructorDecl(isMoveConstructor(), isImplicit(), HasCtorInit)
+          .bind("MoveCtor");
+
+  auto *ToImpMoveCtor =
+      selectFirst<CXXConstructorDecl>("MoveCtor", match(ImpMoveCtor, Ctx));
+
+  EXPECT_TRUE(ToImpMoveCtor->getNumCtorInitializers() == 1);
+  EXPECT_FALSE(ToImpMoveCtor->FriendConstraintRefersToEnclosingTemplate());
+}
+
 TEST(Decl, NoProtoFunctionDeclAttributes) {
   llvm::Annotations Code(R"(
     void f();
     )");
 
   auto AST = tooling::buildASTFromCodeWithArgs(
       Code.code(),
       /*Args=*/{"-target", "i386-apple-darwin", "-x", "objective-c",