[ASTImporter][AST] Fix structural equivalency crash on dependent FieldDecl

[martong]

The added test causes the below assertion in the baseline (w/o the fix):

ASTTests: ../../git/llvm-project/clang/lib/AST/ExprConstant.cpp:14543: llvm::APSInt clang::Expr::EvaluateKnownConstInt(const clang::ASTContext&, llvm::SmallVectorImpl<std::pair<clang::SourceLocation, clang::PartialDiagnostic> >*) const: Assertion `!isValueDependent() && "Expression evaluator can't be called on a dependent expression."' failed.

[shafik]

So was the bug we were saying there were falsely equivalent or falsely not equivalent?

[teemperor]

Shouldn't this compare the actual width expressions? In other words, this patch wouldn't pass the test below:

TEST_F(StructuralEquivalenceTemplateTest, DependentFieldDeclDifferentVal) {
  const char *Code1 = "template <class A, class B> class foo { int a : sizeof(A); };";
  const char *Code2 = "template <class A, class B> class foo { int a : sizeof(B); };";
  auto t = makeNamedDecls(Code1, Code2, Lang_CXX03);
  EXPECT_FALSE(testStructuralMatch(t));
}

I don't want to derail the simple crash fix here, but I'm tempted to say that we might as well just simplify all this to return IsStructurallyEquivalent(Context, Field1->getBitWidth(), Field2->getBitWidth());. The nice diagnostic could live behind behind a check that ensures that both widths are not value-dependent.

If you want to keep this simple because you want to backport it I would also be fine with the current patch (not crashing is clearly an improvement)

In D88665#2307562, @shafik wrote:

So was the bug we were saying there were falsely equivalent or falsely not equivalent?

I think the bug is the crash :)

[martong]

In D88665#2307562, @shafik wrote:

So was the bug we were saying there were falsely equivalent or falsely not equivalent?

I think the bug is the crash :)

Yes. More specifically, the call of getBitWidthValue() caused a segfault with release builds and with assertions enabled it caused the mentioned assert on the given test code.

In D88665#2307945, @teemperor wrote:

Shouldn't this compare the actual width expressions? In other words, this patch wouldn't pass the test below:

TEST_F(StructuralEquivalenceTemplateTest, DependentFieldDeclDifferentVal) {
  const char *Code1 = "template <class A, class B> class foo { int a : sizeof(A); };";
  const char *Code2 = "template <class A, class B> class foo { int a : sizeof(B); };";
  auto t = makeNamedDecls(Code1, Code2, Lang_CXX03);
  EXPECT_FALSE(testStructuralMatch(t));
}

Yes, absolutely, it should. I have updated the patch this way, thanks! However, I was struggling to pass the existing lit test (ASTMerge/struct/test.c). Finally, I decided to remove all the BitField diagnostic code because it was already flawed - we called getBitWidthValue unchecked.

Currently, we are totally inconsistent about the diagnostics. Either we should emit a diagnostic before all return false or we should not ever emit any diags. The diagnostics in their current form are misleading, because there could be many notes missing. I am not sure how much do you guys value these diags in LLDB, but in CTU we simply don't care about them. I'd rather remove these diags from the equivalency check code, because they are causing only confusion. (Or we should properly implement and test all aspects of the diags.)

[martong]

• Delegate to BitWidth Expr matching in case of BitFields

[shafik]

In D88665#2308366, @martong wrote:

In D88665#2307562, @shafik wrote:

So was the bug we were saying there were falsely equivalent or falsely not equivalent?

I think the bug is the crash :)

Yes. More specifically, the call of getBitWidthValue() caused a segfault with release builds and with assertions enabled it caused the mentioned assert on the given test code.

Thank you! From the test it was not obvious what was the actual cause of the crash so evaluating the correctness was difficult.

[teemperor]

LGTM, thanks!

[teemperor]

I am not sure how much do you guys value these diags in LLDB

I think we don't even display them most of the time (IIUC they go to the diagnostic engine which is not always hooked up for the ASTs in LLDB as only a few actually come from Clang).

[martong]

Currently, we are totally inconsistent about the diagnostics. Either we should emit a diagnostic before all return false or we should not ever emit any diags. The diagnostics in their current form are misleading, because there could be many notes missing. I am not sure how much do you guys value these diags in LLDB, but in CTU we simply don't care about them. I'd rather remove these diags from the equivalency check code, because they are causing only confusion. (Or we should properly implement and test all aspects of the diags.)

In D88665#2311344, @teemperor wrote:

LGTM, thanks!

Thanks for the review!

clang/lib/AST/ASTStructuralEquivalence.cpp

if (Context.Complain) {
       Context.Diag2(Field2->getLocation(), diag::note_odr_field)
           << Field2->getDeclName() << Field2->getType();
       Context.Diag1(Field1->getLocation(), diag::note_odr_field)
           << Field1->getDeclName() << Field1->getType();
     }
     return false;
   }
 
-  if (Field1->isBitField() != Field2->isBitField()) {
-    if (Context.Complain) {
-      Context.Diag2(
-          Owner2->getLocation(),
-          Context.getApplicableDiagnostic(diag::err_odr_tag_type_inconsistent))
-          << Context.ToCtx.getTypeDeclType(Owner2);
-      if (Field1->isBitField()) {
-        Context.Diag1(Field1->getLocation(), diag::note_odr_bit_field)
-            << Field1->getDeclName() << Field1->getType()
-            << Field1->getBitWidthValue(Context.FromCtx);
-        Context.Diag2(Field2->getLocation(), diag::note_odr_not_bit_field)
-            << Field2->getDeclName();
-      } else {
-        Context.Diag2(Field2->getLocation(), diag::note_odr_bit_field)
-            << Field2->getDeclName() << Field2->getType()
-            << Field2->getBitWidthValue(Context.ToCtx);
-        Context.Diag1(Field1->getLocation(), diag::note_odr_not_bit_field)
-            << Field1->getDeclName();
-      }
-    }
-    return false;
-  }
-
-  if (Field1->isBitField()) {
-    // Make sure that the bit-fields are the same length.
-    unsigned Bits1 = Field1->getBitWidthValue(Context.FromCtx);
-    unsigned Bits2 = Field2->getBitWidthValue(Context.ToCtx);
-
-    if (Bits1 != Bits2) {
-      if (Context.Complain) {
-        Context.Diag2(Owner2->getLocation(),
-                      Context.getApplicableDiagnostic(
-                          diag::err_odr_tag_type_inconsistent))
-            << Context.ToCtx.getTypeDeclType(Owner2);
-        Context.Diag2(Field2->getLocation(), diag::note_odr_bit_field)
-            << Field2->getDeclName() << Field2->getType() << Bits2;
-        Context.Diag1(Field1->getLocation(), diag::note_odr_bit_field)
-            << Field1->getDeclName() << Field1->getType() << Bits1;
-      }
-      return false;
-    }
-  }
+  if (Field1->isBitField())
+    return IsStructurallyEquivalent(Context, Field1->getBitWidth(),
+                                    Field2->getBitWidth());
 
   return true;
 }
 
 /// Determine structural equivalence of two methods.
 static bool IsStructurallyEquivalent(StructuralEquivalenceContext &Context,
                                      CXXMethodDecl *Method1,
                                      CXXMethodDecl *Method2) {

clang/test/ASTMerge/struct/test.c

 // CHECK: struct2.c:21:8: note: no corresponding field here
 // CHECK: struct2.c:21:31: warning: external variable 'x3' declared with incompatible types in different translation units ('struct S3' vs. 'struct S3')
 // CHECK: struct1.c:24:41: note: declared here with type 'struct S3'
 // CHECK: struct1.c:27:8: warning: type 'struct S4' has incompatible definitions in different translation units
 // CHECK: struct2.c:24:26: note: field 'f' has type 'float' here
 // CHECK: struct1.c:27:8: note: no corresponding field here
 // CHECK: struct2.c:24:31: warning: external variable 'x4' declared with incompatible types in different translation units ('struct S4' vs. 'struct S4')
 // CHECK: struct1.c:27:22: note: declared here with type 'struct S4'
-// CHECK: struct1.c:33:8: warning: type 'struct S6' has incompatible definitions in different translation units
-// CHECK: struct1.c:33:33: note: bit-field 'j' with type 'unsigned int' and length 8 here
-// CHECK: struct2.c:30:33: note: field 'j' is not a bit-field
-// CHECK: struct2.c:30:38: warning: external variable 'x6' declared with incompatible types in different translation units ('struct S6' vs. 'struct S6')
-// CHECK: struct1.c:33:42: note: declared here with type 'struct S6'
-// CHECK: struct1.c:36:8: warning: type 'struct S7' has incompatible definitions in different translation units
-// CHECK: struct1.c:36:33: note: bit-field 'j' with type 'unsigned int' and length 8 here
-// CHECK: struct2.c:33:33: note: bit-field 'j' with type 'unsigned int' and length 16 here
-// CHECK: struct2.c:33:43: warning: external variable 'x7' declared with incompatible types in different translation units ('struct S7' vs. 'struct S7')
-// CHECK: struct1.c:36:42: note: declared here with type 'struct S7'
 // CHECK: struct1.c:56:10: warning: type 'struct DeeperError' has incompatible definitions in different translation units
 // CHECK: struct1.c:56:35: note: field 'f' has type 'int' here
 // CHECK: struct2.c:53:37: note: field 'f' has type 'float' here
 // CHECK: struct1.c:54:8: warning: type 'struct DeepError' has incompatible definitions in different translation units
 // CHECK: struct1.c:56:41: note: field 'Deeper' has type 'struct DeeperError *' here
 // CHECK: struct2.c:53:43: note: field 'Deeper' has type 'struct DeeperError *' here
 // CHECK: struct2.c:54:3: warning: external variable 'xDeep' declared with incompatible types in different translation units ('struct DeepError' vs. 'struct DeepError')
 // CHECK: struct1.c:57:3: note: declared here with type 'struct DeepError'
 // CHECK: struct1.c:74:9: warning: type 'S13' has incompatible definitions in different translation units
 // CHECK: struct1.c:75:9: note: field 'i' has type 'Float' (aka 'float') here
 // CHECK: struct2.c:72:7: note: field 'i' has type 'int' here
 // CHECK: struct2.c:76:5: warning: external variable 'x13' declared with incompatible types in different translation units ('S13' vs. 'S13')
 // CHECK: struct1.c:79:5: note: declared here with type 'S13'
 // CHECK: struct1.c:130:7: warning: type 'struct DeepUnnamedError::(anonymous at [[PATH_TO_INPUTS:.+]]struct1.c:130:7)' has incompatible definitions in different translation units
 // CHECK: struct1.c:131:14: note: field 'i' has type 'long' here
 // CHECK: struct2.c:128:15: note: field 'i' has type 'float' here
 // CHECK: struct1.c:129:5: warning: type 'union DeepUnnamedError::(anonymous at [[PATH_TO_INPUTS]]struct1.c:129:5)' has incompatible definitions in different translation units
 // CHECK: struct1.c:132:9: note: field 'S' has type 'struct (anonymous struct at [[PATH_TO_INPUTS]]struct1.c:130:7)' here
 // CHECK: struct2.c:129:9: note: field 'S' has type 'struct (anonymous struct at [[PATH_TO_INPUTS]]struct2.c:127:7)' here
 // CHECK: struct2.c:138:3: warning: external variable 'x16' declared with incompatible types in different translation units ('struct DeepUnnamedError' vs. 'struct DeepUnnamedError')
 // CHECK: struct1.c:141:3: note: declared here with type 'struct DeepUnnamedError'
-// CHECK: 20 warnings generated
+// CHECK: 17 warnings generated

clang/unittests/AST/StructuralEquivalenceTest.cpp

 }
 
 TEST_F(StructuralEquivalenceTemplateTest, DifferentTemplateArgKind) {
   auto t = makeNamedDecls("template <class T> struct foo;",
                           "template <int T> struct foo;", Lang_CXX03);
   EXPECT_FALSE(testStructuralMatch(t));
 }
 
+TEST_F(StructuralEquivalenceTemplateTest, BitFieldDecl) {
+  const char *Code = "class foo { int a : 2; };";
+  auto t = makeNamedDecls(Code, Code, Lang_CXX03);
+  EXPECT_TRUE(testStructuralMatch(t));
+}
+
+TEST_F(StructuralEquivalenceTemplateTest, BitFieldDeclDifferentWidth) {
+  auto t = makeNamedDecls("class foo { int a : 2; };",
+                          "class foo { int a : 4; };", Lang_CXX03);
+  EXPECT_FALSE(testStructuralMatch(t));
+}
+
+TEST_F(StructuralEquivalenceTemplateTest, DependentBitFieldDecl) {
+  const char *Code = "template <class T> class foo { int a : sizeof(T); };";
+  auto t = makeNamedDecls(Code, Code, Lang_CXX03);
+  EXPECT_TRUE(testStructuralMatch(t));
+}
+
+TEST_F(StructuralEquivalenceTemplateTest, DependentBitFieldDeclDifferentVal) {
+  auto t = makeNamedDecls(
+      "template <class A, class B> class foo { int a : sizeof(A); };",
+      "template <class A, class B> class foo { int a : sizeof(B); };",
+      Lang_CXX03);
+  EXPECT_FALSE(testStructuralMatch(t));
+}
+
+TEST_F(StructuralEquivalenceTemplateTest, DependentBitFieldDeclDifferentVal2) {
+  auto t = makeNamedDecls(
+      "template <class A> class foo { int a : sizeof(A); };",
+      "template <class A> class foo { int a : sizeof(A) + 1; };", Lang_CXX03);
+  EXPECT_FALSE(testStructuralMatch(t));
+}
+
 TEST_F(StructuralEquivalenceTemplateTest, ExplicitBoolSame) {
   auto Decls = makeNamedDecls(
       "template <bool b> struct foo {explicit(b) foo(int);};",
       "template <bool b> struct foo {explicit(b) foo(int);};", Lang_CXX20);
   CXXConstructorDecl *First = FirstDeclMatcher<CXXConstructorDecl>().match(
       get<0>(Decls), cxxConstructorDecl(hasName("foo<b>")));
   CXXConstructorDecl *Second = FirstDeclMatcher<CXXConstructorDecl>().match(
       get<1>(Decls), cxxConstructorDecl(hasName("foo<b>")));