[PATCH] D88665: [ASTImporter][AST] Fix structural equivalency crash on dependent FieldDecl

[Gabor Marton via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG007dd12d5468: [ASTImporter][AST] Fix structural equivalency 
crash on dependent FieldDecl (authored by martong).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D88665/new/

https://reviews.llvm.org/D88665

Files:
  clang/lib/AST/ASTStructuralEquivalence.cpp
  clang/test/ASTMerge/struct/test.c
  clang/unittests/AST/StructuralEquivalenceTest.cpp

Index: clang/unittests/AST/StructuralEquivalenceTest.cpp
===
--- clang/unittests/AST/StructuralEquivalenceTest.cpp
+++ clang/unittests/AST/StructuralEquivalenceTest.cpp
@@ -976,6 +976,39 @@
   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 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 foo { int a : sizeof(A); };",
+  "template  class foo { int a : sizeof(B); };",
+  Lang_CXX03);
+  EXPECT_FALSE(testStructuralMatch(t));
+}
+
+TEST_F(StructuralEquivalenceTemplateTest, DependentBitFieldDeclDifferentVal2) {
+  auto t = makeNamedDecls(
+  "template  class foo { int a : sizeof(A); };",
+  "template  class foo { int a : sizeof(A) + 1; };", Lang_CXX03);
+  EXPECT_FALSE(testStructuralMatch(t));
+}
+
 TEST_F(StructuralEquivalenceTemplateTest, ExplicitBoolSame) {
   auto Decls = makeNamedDecls(
   "template  struct foo {explicit(b) foo(int);};",
Index: clang/test/ASTMerge/struct/test.c
===
--- clang/test/ASTMerge/struct/test.c
+++ clang/test/ASTMerge/struct/test.c
@@ -21,16 +21,6 @@
 // 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
@@ -52,4 +42,4 @@
 // 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
Index: clang/lib/AST/ASTStructuralEquivalence.cpp
===
--- clang/lib/AST/ASTStructuralEquivalence.cpp
+++ clang/lib/AST/ASTStructuralEquivalence.cpp
@@ -1256,48 +1256,9 @@
 return false;
   }
 
-  if (Field1->isBitField() != Field2->isBitField()) {
-if (Context.Complain) {
-  Context.Diag2(
-  Owner2->getLocation(),
-  Context.getApplicableDiagnostic(diag::err_odr_tag_type_inconsistent))
-  << 

[PATCH] D88665: [ASTImporter][AST] Fix structural equivalency crash on dependent FieldDecl

[Gabor Marton via Phabricator via cfe-commits]

martong added a comment.

> 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!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D88665/new/

https://reviews.llvm.org/D88665

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D88665: [ASTImporter][AST] Fix structural equivalency crash on dependent FieldDecl

[Raphael Isemann via Phabricator via cfe-commits]

teemperor added a comment.

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D88665/new/

https://reviews.llvm.org/D88665

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D88665: [ASTImporter][AST] Fix structural equivalency crash on dependent FieldDecl

[Raphael Isemann via Phabricator via cfe-commits]

teemperor accepted this revision.
teemperor added a comment.
This revision is now accepted and ready to land.

LGTM, thanks!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D88665/new/

https://reviews.llvm.org/D88665

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D88665: [ASTImporter][AST] Fix structural equivalency crash on dependent FieldDecl

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik added a comment.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D88665/new/

https://reviews.llvm.org/D88665

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D88665: [ASTImporter][AST] Fix structural equivalency crash on dependent FieldDecl

[Gabor Marton via Phabricator via cfe-commits]

martong updated this revision to Diff 295800.
martong added a comment.

- Delegate to BitWidth Expr matching in case of BitFields


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D88665/new/

https://reviews.llvm.org/D88665

Files:
  clang/lib/AST/ASTStructuralEquivalence.cpp
  clang/test/ASTMerge/struct/test.c
  clang/unittests/AST/StructuralEquivalenceTest.cpp

Index: clang/unittests/AST/StructuralEquivalenceTest.cpp
===
--- clang/unittests/AST/StructuralEquivalenceTest.cpp
+++ clang/unittests/AST/StructuralEquivalenceTest.cpp
@@ -976,6 +976,39 @@
   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 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 foo { int a : sizeof(A); };",
+  "template  class foo { int a : sizeof(B); };",
+  Lang_CXX03);
+  EXPECT_FALSE(testStructuralMatch(t));
+}
+
+TEST_F(StructuralEquivalenceTemplateTest, DependentBitFieldDeclDifferentVal2) {
+  auto t = makeNamedDecls(
+  "template  class foo { int a : sizeof(A); };",
+  "template  class foo { int a : sizeof(A) + 1; };", Lang_CXX03);
+  EXPECT_FALSE(testStructuralMatch(t));
+}
+
 TEST_F(StructuralEquivalenceTemplateTest, ExplicitBoolSame) {
   auto Decls = makeNamedDecls(
   "template  struct foo {explicit(b) foo(int);};",
Index: clang/test/ASTMerge/struct/test.c
===
--- clang/test/ASTMerge/struct/test.c
+++ clang/test/ASTMerge/struct/test.c
@@ -21,16 +21,6 @@
 // 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
@@ -52,4 +42,4 @@
 // 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
Index: clang/lib/AST/ASTStructuralEquivalence.cpp
===
--- clang/lib/AST/ASTStructuralEquivalence.cpp
+++ clang/lib/AST/ASTStructuralEquivalence.cpp
@@ -1256,48 +1256,9 @@
 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)
-<< 

[PATCH] D88665: [ASTImporter][AST] Fix structural equivalency crash on dependent FieldDecl

[Gabor Marton via Phabricator via cfe-commits]

martong added a comment.

> 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 foo { int a : 
> sizeof(A); };";
> const char *Code2 = "template  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.)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D88665/new/

https://reviews.llvm.org/D88665

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D88665: [ASTImporter][AST] Fix structural equivalency crash on dependent FieldDecl

[Raphael Isemann via Phabricator via cfe-commits]

teemperor requested changes to this revision.
teemperor added a comment.
This revision now requires changes to proceed.

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 foo { int a : 
sizeof(A); };";
const char *Code2 = "template  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 :)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D88665/new/

https://reviews.llvm.org/D88665

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D88665: [ASTImporter][AST] Fix structural equivalency crash on dependent FieldDecl

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik added a comment.

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D88665/new/

https://reviews.llvm.org/D88665

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D88665: [ASTImporter][AST] Fix structural equivalency crash on dependent FieldDecl

[Gabor Marton via Phabricator via cfe-commits]

martong added a comment.

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 >*) const: Assertion `!isValueDependent() && 
"Expression evaluator can't be called on a dependent expression."' failed.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D88665/new/

https://reviews.llvm.org/D88665

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D88665: [ASTImporter][AST] Fix structural equivalency crash on dependent FieldDecl

[Gabor Marton via Phabricator via cfe-commits]

martong created this revision.
martong added reviewers: balazske, teemperor, shafik, a_sidorin.
Herald added subscribers: cfe-commits, gamesh411, Szelethus, dkrupp, rnkovacs.
Herald added a reviewer: a.sidorin.
Herald added a project: clang.
martong requested review of this revision.

Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D88665

Files:
  clang/lib/AST/ASTStructuralEquivalence.cpp
  clang/unittests/AST/StructuralEquivalenceTest.cpp


Index: clang/unittests/AST/StructuralEquivalenceTest.cpp
===
--- clang/unittests/AST/StructuralEquivalenceTest.cpp
+++ clang/unittests/AST/StructuralEquivalenceTest.cpp
@@ -976,6 +976,12 @@
   EXPECT_FALSE(testStructuralMatch(t));
 }
 
+TEST_F(StructuralEquivalenceTemplateTest, DependentFieldDecl) {
+  const char *Code = "template  class foo { int a : sizeof(T); };";
+  auto t = makeNamedDecls(Code, Code, Lang_CXX03);
+  EXPECT_TRUE(testStructuralMatch(t));
+}
+
 TEST_F(StructuralEquivalenceTemplateTest, ExplicitBoolSame) {
   auto Decls = makeNamedDecls(
   "template  struct foo {explicit(b) foo(int);};",
Index: clang/lib/AST/ASTStructuralEquivalence.cpp
===
--- clang/lib/AST/ASTStructuralEquivalence.cpp
+++ clang/lib/AST/ASTStructuralEquivalence.cpp
@@ -1280,6 +1280,10 @@
   }
 
   if (Field1->isBitField()) {
+bool isVD1 = Field1->getBitWidth()->isValueDependent();
+bool isVD2 = Field2->getBitWidth()->isValueDependent();
+if (isVD1 || isVD2)
+  return isVD1 && isVD2;
 // Make sure that the bit-fields are the same length.
 unsigned Bits1 = Field1->getBitWidthValue(Context.FromCtx);
 unsigned Bits2 = Field2->getBitWidthValue(Context.ToCtx);


Index: clang/unittests/AST/StructuralEquivalenceTest.cpp
===
--- clang/unittests/AST/StructuralEquivalenceTest.cpp
+++ clang/unittests/AST/StructuralEquivalenceTest.cpp
@@ -976,6 +976,12 @@
   EXPECT_FALSE(testStructuralMatch(t));
 }
 
+TEST_F(StructuralEquivalenceTemplateTest, DependentFieldDecl) {
+  const char *Code = "template  class foo { int a : sizeof(T); };";
+  auto t = makeNamedDecls(Code, Code, Lang_CXX03);
+  EXPECT_TRUE(testStructuralMatch(t));
+}
+
 TEST_F(StructuralEquivalenceTemplateTest, ExplicitBoolSame) {
   auto Decls = makeNamedDecls(
   "template  struct foo {explicit(b) foo(int);};",
Index: clang/lib/AST/ASTStructuralEquivalence.cpp
===
--- clang/lib/AST/ASTStructuralEquivalence.cpp
+++ clang/lib/AST/ASTStructuralEquivalence.cpp
@@ -1280,6 +1280,10 @@
   }
 
   if (Field1->isBitField()) {
+bool isVD1 = Field1->getBitWidth()->isValueDependent();
+bool isVD2 = Field2->getBitWidth()->isValueDependent();
+if (isVD1 || isVD2)
+  return isVD1 && isVD2;
 // Make sure that the bit-fields are the same length.
 unsigned Bits1 = Field1->getBitWidthValue(Context.FromCtx);
 unsigned Bits2 = Field2->getBitWidthValue(Context.ToCtx);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits