[PATCH] D71734: [ODRHash] Hash `RecordDecl` and diagnose discovered mismatches.

[Volodymyr Sapsai via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
vsapsai marked an inline comment as done.
Closed by commit rG160bc160b9b1: [ODRHash] Hash `RecordDecl` and diagnose 
discovered mismatches. (authored by vsapsai).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71734

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/AST/Decl.h
  clang/include/clang/AST/DeclBase.h
  clang/include/clang/AST/ODRDiagsEmitter.h
  clang/include/clang/AST/ODRHash.h
  clang/include/clang/Serialization/ASTReader.h
  clang/lib/AST/Decl.cpp
  clang/lib/AST/ODRDiagsEmitter.cpp
  clang/lib/AST/ODRHash.cpp
  clang/lib/Serialization/ASTReader.cpp
  clang/lib/Serialization/ASTReaderDecl.cpp
  clang/lib/Serialization/ASTWriterDecl.cpp
  clang/test/Modules/compare-record.c

Index: clang/test/Modules/compare-record.c
===
--- /dev/null
+++ clang/test/Modules/compare-record.c
@@ -0,0 +1,418 @@
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+
+// Build first header file
+// RUN: echo "#define FIRST" >> %t/include/first.h
+// RUN: cat %t/test.c>> %t/include/first.h
+// RUN: echo "#undef FIRST"  >> %t/include/first.h
+
+// Build second header file
+// RUN: echo "#define SECOND" >> %t/include/second.h
+// RUN: cat %t/test.c >> %t/include/second.h
+// RUN: echo "#undef SECOND"  >> %t/include/second.h
+
+// Test that each header can compile
+// RUN: %clang_cc1 -fsyntax-only -x objective-c %t/include/first.h -fblocks -fobjc-arc
+// RUN: %clang_cc1 -fsyntax-only -x objective-c %t/include/second.h -fblocks -fobjc-arc
+
+// Run test
+// RUN: %clang_cc1 -I%t/include -verify %t/test.c -fblocks -fobjc-arc \
+// RUN:-fmodules -fimplicit-module-maps -fmodules-cache-path=%t/modules.cache
+
+// Run tests for nested structs
+// DEFINE: %{filename} = test-nested-struct.c
+// DEFINE: %{macro_flag} = -DCASE1=1
+// DEFINE: %{command} = %clang_cc1 -I%t/include -verify %t/%{filename} -fblocks -fobjc-arc \
+// DEFINE: -fmodules -fimplicit-module-maps -fmodules-cache-path=%t/modules.cache \
+// DEFINE: %{macro_flag} -emit-llvm -o %t/%{filename}.bc
+// RUN: %{command}
+// REDEFINE: %{macro_flag} = -DCASE2=1
+// RUN: %{command}
+// REDEFINE: %{macro_flag} = -DCASE3=1
+// RUN: %{command}
+
+// Test that we don't accept different structs and unions with the same name
+// from multiple modules but detect mismatches and provide actionable
+// diagnostic.
+
+//--- include/first-empty.h
+//--- include/module.modulemap
+module First {
+  module Empty {
+header "first-empty.h"
+  }
+  module Hidden {
+header "first.h"
+header "first-nested-struct.h"
+export *
+  }
+}
+module Second {
+  header "second.h"
+  header "second-nested-struct.h"
+  export *
+}
+
+//--- test.c
+#if !defined(FIRST) && !defined(SECOND)
+# include "first-empty.h"
+# include "second.h"
+#endif
+
+#if defined(FIRST)
+struct CompareForwardDeclaration1;
+struct CompareForwardDeclaration2 {};
+#elif defined(SECOND)
+struct CompareForwardDeclaration1 {};
+struct CompareForwardDeclaration2;
+#else
+struct CompareForwardDeclaration1 *compareForwardDeclaration1;
+struct CompareForwardDeclaration2 *compareForwardDeclaration2;
+#endif
+
+#if defined(FIRST)
+struct CompareMatchingFields {
+  int matchingFieldName;
+};
+
+struct CompareFieldPresence1 {
+  int fieldPresence1;
+};
+struct CompareFieldPresence2 {};
+
+struct CompareFieldName {
+  int fieldNameA;
+};
+
+struct CompareFieldOrder {
+  int fieldOrderX;
+  int fieldOrderY;
+};
+#elif defined(SECOND)
+struct CompareMatchingFields {
+  int matchingFieldName;
+};
+
+struct CompareFieldPresence1 {
+};
+struct CompareFieldPresence2 {
+  int fieldPresence2;
+};
+
+struct CompareFieldName {
+  int fieldNameB;
+};
+
+struct CompareFieldOrder {
+  int fieldOrderY;
+  int fieldOrderX;
+};
+#else
+struct CompareMatchingFields compareMatchingFields;
+struct CompareFieldPresence1 compareFieldPresence1;
+// expected-error@first.h:* {{'CompareFieldPresence1' has different definitions in different modules; first difference is definition in module 'First.Hidden' found field}}
+// expected-note@second.h:* {{but in 'Second' found end of class}}
+struct CompareFieldPresence2 compareFieldPresence2;
+// expected-error@second.h:* {{'CompareFieldPresence2::fieldPresence2' from module 'Second' is not present in definition of 'struct CompareFieldPresence2' in module 'First.Hidden'}}
+// expected-note@first.h:* {{definition has no member 'fieldPresence2'}}
+struct CompareFieldName compareFieldName;
+// expected-error@second.h:* {{'CompareFieldName::fieldNameB' from module 'Second' is not present in definition of 'struct CompareFieldName' in module 'First.Hidden'}}
+// expected-note@first.h:* {{definition has no member 'fieldNameB'}}
+struct CompareFieldOrder compareFieldOrder;
+// 

[PATCH] D71734: [ODRHash] Hash `RecordDecl` and diagnose discovered mismatches.

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai marked an inline comment as done.
vsapsai added a comment.

In D71734#4061228 , @ChuanqiXu wrote:

> LGTM generally. It'd better to mention this in the `Potentially Breaking 
> Changes` section of ReleaseNotes.

Thanks for all your efforts during the review! Mentioned the change in the 
release notes.




Comment at: clang/lib/AST/Decl.cpp:4714
   setIsRandomized(false);
+  RecordDeclBits.ODRHash = 0;
 }

ChuanqiXu wrote:
> nit: setODRHash(0) may be more consistent with above style.
You are right, changed it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71734

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

[PATCH] D71734: [ODRHash] Hash `RecordDecl` and diagnose discovered mismatches.

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai updated this revision to Diff 490354.
vsapsai added a comment.

`setODRHash(0)` for consistency, mention the change in Potentially Breaking 
Changes.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71734

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/AST/Decl.h
  clang/include/clang/AST/DeclBase.h
  clang/include/clang/AST/ODRDiagsEmitter.h
  clang/include/clang/AST/ODRHash.h
  clang/include/clang/Serialization/ASTReader.h
  clang/lib/AST/Decl.cpp
  clang/lib/AST/ODRDiagsEmitter.cpp
  clang/lib/AST/ODRHash.cpp
  clang/lib/Serialization/ASTReader.cpp
  clang/lib/Serialization/ASTReaderDecl.cpp
  clang/lib/Serialization/ASTWriterDecl.cpp
  clang/test/Modules/compare-record.c

Index: clang/test/Modules/compare-record.c
===
--- /dev/null
+++ clang/test/Modules/compare-record.c
@@ -0,0 +1,418 @@
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+
+// Build first header file
+// RUN: echo "#define FIRST" >> %t/include/first.h
+// RUN: cat %t/test.c>> %t/include/first.h
+// RUN: echo "#undef FIRST"  >> %t/include/first.h
+
+// Build second header file
+// RUN: echo "#define SECOND" >> %t/include/second.h
+// RUN: cat %t/test.c >> %t/include/second.h
+// RUN: echo "#undef SECOND"  >> %t/include/second.h
+
+// Test that each header can compile
+// RUN: %clang_cc1 -fsyntax-only -x objective-c %t/include/first.h -fblocks -fobjc-arc
+// RUN: %clang_cc1 -fsyntax-only -x objective-c %t/include/second.h -fblocks -fobjc-arc
+
+// Run test
+// RUN: %clang_cc1 -I%t/include -verify %t/test.c -fblocks -fobjc-arc \
+// RUN:-fmodules -fimplicit-module-maps -fmodules-cache-path=%t/modules.cache
+
+// Run tests for nested structs
+// DEFINE: %{filename} = test-nested-struct.c
+// DEFINE: %{macro_flag} = -DCASE1=1
+// DEFINE: %{command} = %clang_cc1 -I%t/include -verify %t/%{filename} -fblocks -fobjc-arc \
+// DEFINE: -fmodules -fimplicit-module-maps -fmodules-cache-path=%t/modules.cache \
+// DEFINE: %{macro_flag} -emit-llvm -o %t/%{filename}.bc
+// RUN: %{command}
+// REDEFINE: %{macro_flag} = -DCASE2=1
+// RUN: %{command}
+// REDEFINE: %{macro_flag} = -DCASE3=1
+// RUN: %{command}
+
+// Test that we don't accept different structs and unions with the same name
+// from multiple modules but detect mismatches and provide actionable
+// diagnostic.
+
+//--- include/first-empty.h
+//--- include/module.modulemap
+module First {
+  module Empty {
+header "first-empty.h"
+  }
+  module Hidden {
+header "first.h"
+header "first-nested-struct.h"
+export *
+  }
+}
+module Second {
+  header "second.h"
+  header "second-nested-struct.h"
+  export *
+}
+
+//--- test.c
+#if !defined(FIRST) && !defined(SECOND)
+# include "first-empty.h"
+# include "second.h"
+#endif
+
+#if defined(FIRST)
+struct CompareForwardDeclaration1;
+struct CompareForwardDeclaration2 {};
+#elif defined(SECOND)
+struct CompareForwardDeclaration1 {};
+struct CompareForwardDeclaration2;
+#else
+struct CompareForwardDeclaration1 *compareForwardDeclaration1;
+struct CompareForwardDeclaration2 *compareForwardDeclaration2;
+#endif
+
+#if defined(FIRST)
+struct CompareMatchingFields {
+  int matchingFieldName;
+};
+
+struct CompareFieldPresence1 {
+  int fieldPresence1;
+};
+struct CompareFieldPresence2 {};
+
+struct CompareFieldName {
+  int fieldNameA;
+};
+
+struct CompareFieldOrder {
+  int fieldOrderX;
+  int fieldOrderY;
+};
+#elif defined(SECOND)
+struct CompareMatchingFields {
+  int matchingFieldName;
+};
+
+struct CompareFieldPresence1 {
+};
+struct CompareFieldPresence2 {
+  int fieldPresence2;
+};
+
+struct CompareFieldName {
+  int fieldNameB;
+};
+
+struct CompareFieldOrder {
+  int fieldOrderY;
+  int fieldOrderX;
+};
+#else
+struct CompareMatchingFields compareMatchingFields;
+struct CompareFieldPresence1 compareFieldPresence1;
+// expected-error@first.h:* {{'CompareFieldPresence1' has different definitions in different modules; first difference is definition in module 'First.Hidden' found field}}
+// expected-note@second.h:* {{but in 'Second' found end of class}}
+struct CompareFieldPresence2 compareFieldPresence2;
+// expected-error@second.h:* {{'CompareFieldPresence2::fieldPresence2' from module 'Second' is not present in definition of 'struct CompareFieldPresence2' in module 'First.Hidden'}}
+// expected-note@first.h:* {{definition has no member 'fieldPresence2'}}
+struct CompareFieldName compareFieldName;
+// expected-error@second.h:* {{'CompareFieldName::fieldNameB' from module 'Second' is not present in definition of 'struct CompareFieldName' in module 'First.Hidden'}}
+// expected-note@first.h:* {{definition has no member 'fieldNameB'}}
+struct CompareFieldOrder compareFieldOrder;
+// expected-error@first.h:* {{'CompareFieldOrder' has different definitions in different modules; first difference is definition in module 

[PATCH] D71734: [ODRHash] Hash `RecordDecl` and diagnose discovered mismatches.

[Chuanqi Xu via Phabricator via cfe-commits]

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

LGTM generally. It'd better to mention this in the `Potentially Breaking 
Changes` section of ReleaseNotes.




Comment at: clang/lib/AST/Decl.cpp:4714
   setIsRandomized(false);
+  RecordDeclBits.ODRHash = 0;
 }

nit: setODRHash(0) may be more consistent with above style.



Comment at: clang/lib/AST/Decl.cpp:4881-4882
+  // For RecordDecl the ODRHash is stored in the remaining 26
+  // bit of RecordDeclBits, adjust the hash to accomodate.
+  setODRHash(Hash.CalculateHash() >> 6);
+  return RecordDeclBits.ODRHash;

vsapsai wrote:
> ChuanqiXu wrote:
> > I am curious for the operation. It looks dangerous. How can we be sure that 
> > the hash value is still meaningful after remove its lower 6 bits?
> Hash value itself has no meaning. The risk with truncation is that 
> `RecordDecl`s with different hashes can end up with equal truncated hashes. 
> It means we'd miss some mismatches. Currently we are missing //all// 
> mismatches, so some looks like an improvement. 
> 
> This applies only to C and Objective-C but not to C++ as CXXRecordDecl stores 
> its own ODR hash separately. So some imperfection in Objective-C seems more 
> desirable than adding 4 bytes in memory consumption per each struct and 
> class, including C++ classes.
It sounds not bad.



Comment at: clang/lib/AST/ODRDiagsEmitter.cpp:1550-1551
 
+bool ODRDiagsEmitter::diagnoseMismatch(const RecordDecl *FirstRecord,
+   const RecordDecl *SecondRecord) const {
+  if (FirstRecord == SecondRecord)

vsapsai wrote:
> ChuanqiXu wrote:
> > The implementation of this function looks redundant with other overloads. 
> > Of course this is not the problem of the patch. I think we can add a FIXME 
> > at least.
> Do you have any specific ideas to reduce redundancy? `PopulateHashes` 
> probably can be made the same for different entities but the rest has many 
> annoying differences. Diagnosing mismatches for different entities consists 
> of the same pieces (that are already put into reusable methods) but the 
> composition of such pieces is different for different entities.
> 
> I've tried to push complex logic into reusable methods and repeat the simple 
> stuff. For example, for `std::string FirstModule = 
> getOwningModuleNameForDiagnostic(FirstRecord);` obtaining a module name is 
> non-trivial and that's why it is in a reusable method. But the same 
> assignment doesn't seem to carry the problems of copy-pasted code.
> 
> By the way, Phabricator shows big [visually] contiguous chunks as being 
> copied. But in fact these big chunks consist of smaller pieces that are taken 
> from different places. So I think that reusing the same pieces multiple times 
> and composing them in different ways is actually useful.
No specific ideas really. I feel it is scary to see so many copied chunks and 
we should be able to extract them into smaller common functions. But this is 
not the problem of the patch. I don't want to block it for such reasons.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71734

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

[PATCH] D71734: [ODRHash] Hash `RecordDecl` and diagnose discovered mismatches.

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai updated this revision to Diff 490036.
vsapsai added a comment.

Rebase and add assertion in `ODRHash::AddRecordDecl`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71734

Files:
  clang/include/clang/AST/Decl.h
  clang/include/clang/AST/DeclBase.h
  clang/include/clang/AST/ODRDiagsEmitter.h
  clang/include/clang/AST/ODRHash.h
  clang/include/clang/Serialization/ASTReader.h
  clang/lib/AST/Decl.cpp
  clang/lib/AST/ODRDiagsEmitter.cpp
  clang/lib/AST/ODRHash.cpp
  clang/lib/Serialization/ASTReader.cpp
  clang/lib/Serialization/ASTReaderDecl.cpp
  clang/lib/Serialization/ASTWriterDecl.cpp
  clang/test/Modules/compare-record.c

Index: clang/test/Modules/compare-record.c
===
--- /dev/null
+++ clang/test/Modules/compare-record.c
@@ -0,0 +1,418 @@
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+
+// Build first header file
+// RUN: echo "#define FIRST" >> %t/include/first.h
+// RUN: cat %t/test.c>> %t/include/first.h
+// RUN: echo "#undef FIRST"  >> %t/include/first.h
+
+// Build second header file
+// RUN: echo "#define SECOND" >> %t/include/second.h
+// RUN: cat %t/test.c >> %t/include/second.h
+// RUN: echo "#undef SECOND"  >> %t/include/second.h
+
+// Test that each header can compile
+// RUN: %clang_cc1 -fsyntax-only -x objective-c %t/include/first.h -fblocks -fobjc-arc
+// RUN: %clang_cc1 -fsyntax-only -x objective-c %t/include/second.h -fblocks -fobjc-arc
+
+// Run test
+// RUN: %clang_cc1 -I%t/include -verify %t/test.c -fblocks -fobjc-arc \
+// RUN:-fmodules -fimplicit-module-maps -fmodules-cache-path=%t/modules.cache
+
+// Run tests for nested structs
+// DEFINE: %{filename} = test-nested-struct.c
+// DEFINE: %{macro_flag} = -DCASE1=1
+// DEFINE: %{command} = %clang_cc1 -I%t/include -verify %t/%{filename} -fblocks -fobjc-arc \
+// DEFINE: -fmodules -fimplicit-module-maps -fmodules-cache-path=%t/modules.cache \
+// DEFINE: %{macro_flag} -emit-llvm -o %t/%{filename}.bc
+// RUN: %{command}
+// REDEFINE: %{macro_flag} = -DCASE2=1
+// RUN: %{command}
+// REDEFINE: %{macro_flag} = -DCASE3=1
+// RUN: %{command}
+
+// Test that we don't accept different structs and unions with the same name
+// from multiple modules but detect mismatches and provide actionable
+// diagnostic.
+
+//--- include/first-empty.h
+//--- include/module.modulemap
+module First {
+  module Empty {
+header "first-empty.h"
+  }
+  module Hidden {
+header "first.h"
+header "first-nested-struct.h"
+export *
+  }
+}
+module Second {
+  header "second.h"
+  header "second-nested-struct.h"
+  export *
+}
+
+//--- test.c
+#if !defined(FIRST) && !defined(SECOND)
+# include "first-empty.h"
+# include "second.h"
+#endif
+
+#if defined(FIRST)
+struct CompareForwardDeclaration1;
+struct CompareForwardDeclaration2 {};
+#elif defined(SECOND)
+struct CompareForwardDeclaration1 {};
+struct CompareForwardDeclaration2;
+#else
+struct CompareForwardDeclaration1 *compareForwardDeclaration1;
+struct CompareForwardDeclaration2 *compareForwardDeclaration2;
+#endif
+
+#if defined(FIRST)
+struct CompareMatchingFields {
+  int matchingFieldName;
+};
+
+struct CompareFieldPresence1 {
+  int fieldPresence1;
+};
+struct CompareFieldPresence2 {};
+
+struct CompareFieldName {
+  int fieldNameA;
+};
+
+struct CompareFieldOrder {
+  int fieldOrderX;
+  int fieldOrderY;
+};
+#elif defined(SECOND)
+struct CompareMatchingFields {
+  int matchingFieldName;
+};
+
+struct CompareFieldPresence1 {
+};
+struct CompareFieldPresence2 {
+  int fieldPresence2;
+};
+
+struct CompareFieldName {
+  int fieldNameB;
+};
+
+struct CompareFieldOrder {
+  int fieldOrderY;
+  int fieldOrderX;
+};
+#else
+struct CompareMatchingFields compareMatchingFields;
+struct CompareFieldPresence1 compareFieldPresence1;
+// expected-error@first.h:* {{'CompareFieldPresence1' has different definitions in different modules; first difference is definition in module 'First.Hidden' found field}}
+// expected-note@second.h:* {{but in 'Second' found end of class}}
+struct CompareFieldPresence2 compareFieldPresence2;
+// expected-error@second.h:* {{'CompareFieldPresence2::fieldPresence2' from module 'Second' is not present in definition of 'struct CompareFieldPresence2' in module 'First.Hidden'}}
+// expected-note@first.h:* {{definition has no member 'fieldPresence2'}}
+struct CompareFieldName compareFieldName;
+// expected-error@second.h:* {{'CompareFieldName::fieldNameB' from module 'Second' is not present in definition of 'struct CompareFieldName' in module 'First.Hidden'}}
+// expected-note@first.h:* {{definition has no member 'fieldNameB'}}
+struct CompareFieldOrder compareFieldOrder;
+// expected-error@first.h:* {{'CompareFieldOrder' has different definitions in different modules; first difference is definition in module 'First.Hidden' found field 'fieldOrderX'}}
+// 

[PATCH] D71734: [ODRHash] Hash `RecordDecl` and diagnose discovered mismatches.

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai added inline comments.



Comment at: clang/lib/AST/Decl.cpp:4881-4882
+  // For RecordDecl the ODRHash is stored in the remaining 26
+  // bit of RecordDeclBits, adjust the hash to accomodate.
+  setODRHash(Hash.CalculateHash() >> 6);
+  return RecordDeclBits.ODRHash;

ChuanqiXu wrote:
> I am curious for the operation. It looks dangerous. How can we be sure that 
> the hash value is still meaningful after remove its lower 6 bits?
Hash value itself has no meaning. The risk with truncation is that 
`RecordDecl`s with different hashes can end up with equal truncated hashes. It 
means we'd miss some mismatches. Currently we are missing //all// mismatches, 
so some looks like an improvement. 

This applies only to C and Objective-C but not to C++ as CXXRecordDecl stores 
its own ODR hash separately. So some imperfection in Objective-C seems more 
desirable than adding 4 bytes in memory consumption per each struct and class, 
including C++ classes.



Comment at: clang/lib/AST/ODRDiagsEmitter.cpp:1550-1551
 
+bool ODRDiagsEmitter::diagnoseMismatch(const RecordDecl *FirstRecord,
+   const RecordDecl *SecondRecord) const {
+  if (FirstRecord == SecondRecord)

ChuanqiXu wrote:
> The implementation of this function looks redundant with other overloads. Of 
> course this is not the problem of the patch. I think we can add a FIXME at 
> least.
Do you have any specific ideas to reduce redundancy? `PopulateHashes` probably 
can be made the same for different entities but the rest has many annoying 
differences. Diagnosing mismatches for different entities consists of the same 
pieces (that are already put into reusable methods) but the composition of such 
pieces is different for different entities.

I've tried to push complex logic into reusable methods and repeat the simple 
stuff. For example, for `std::string FirstModule = 
getOwningModuleNameForDiagnostic(FirstRecord);` obtaining a module name is 
non-trivial and that's why it is in a reusable method. But the same assignment 
doesn't seem to carry the problems of copy-pasted code.

By the way, Phabricator shows big [visually] contiguous chunks as being copied. 
But in fact these big chunks consist of smaller pieces that are taken from 
different places. So I think that reusing the same pieces multiple times and 
composing them in different ways is actually useful.



Comment at: clang/lib/AST/ODRHash.cpp:592-593
 
+void ODRHash::AddRecordDecl(const RecordDecl *Record) {
+  AddDecl(Record);
+

ChuanqiXu wrote:
> For the current implementation, if it makes sense to add an assertion 
> `!isa(Record);`
Yes, that's a good idea as calling AddRecordDecl with CXXRecordDecl looks like 
an easy mistake to make.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71734

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

[PATCH] D71734: [ODRHash] Hash `RecordDecl` and diagnose discovered mismatches.

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added inline comments.



Comment at: clang/lib/AST/Decl.cpp:4881-4882
+  // For RecordDecl the ODRHash is stored in the remaining 26
+  // bit of RecordDeclBits, adjust the hash to accomodate.
+  setODRHash(Hash.CalculateHash() >> 6);
+  return RecordDeclBits.ODRHash;

I am curious for the operation. It looks dangerous. How can we be sure that the 
hash value is still meaningful after remove its lower 6 bits?



Comment at: clang/lib/AST/ODRDiagsEmitter.cpp:1550-1551
 
+bool ODRDiagsEmitter::diagnoseMismatch(const RecordDecl *FirstRecord,
+   const RecordDecl *SecondRecord) const {
+  if (FirstRecord == SecondRecord)

The implementation of this function looks redundant with other overloads. Of 
course this is not the problem of the patch. I think we can add a FIXME at 
least.



Comment at: clang/lib/AST/ODRHash.cpp:592-593
 
+void ODRHash::AddRecordDecl(const RecordDecl *Record) {
+  AddDecl(Record);
+

For the current implementation, if it makes sense to add an assertion 
`!isa(Record);`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71734

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

[PATCH] D71734: [ODRHash] Hash `RecordDecl` and diagnose discovered mismatches.

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai added a comment.

Ping. Also checked the change on a bunch of internal Objective-C/C code using 
modules - no new errors.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71734

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

[PATCH] D71734: [ODRHash] Hash `RecordDecl` and diagnose discovered mismatches.

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai added a comment.

If anybody is curious about anonymous structs/unions, the relevant change is 
D140055 .


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71734

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