[PATCH] D71734: [Modules] Handle tag types and complain about bad merges in C/Objective-C mode

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai updated this revision to Diff 482981.
vsapsai added a comment.
Herald added a project: All.

Rebase and use ODRDiagsEmitter. Add more tests.


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: [Modules] Handle tag types and complain about bad merges in C/Objective-C mode

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai planned changes to this revision.
vsapsai added a comment.

Need to detect mismatches in nested structs.


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: [Modules] Handle tag types and complain about bad merges in C/Objective-C mode

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai added a comment.

Found another case that doesn't emit an error

  #if defined(FIRST)
  struct Indirect {
int x;
  };
  struct Direct {
struct Indirect i;
  };
  #elif defined(SECOND)
  struct Indirect {
double a;
  };
  struct Direct {
struct Indirect i;
  };
  #else
  struct Direct d;
  #endif

According to my debugging there is no error because `Direct` fields aren't 
deserialized in -fsyntax-only mode and therefore `Indirect` definitions aren't 
compared. But during IRGen there is diagnostic and that's because calculating 
record layout triggers full deserialization. Also there is diagnostic in C++ 
because we are dealing with default initialization and 
`DeclareImplicitDefaultConstructor` iterates through all the fields 
deserializing them.

I believe the best user experience is consistent diagnostic, so we should emit 
the error even with -fsyntax-only. If anybody has any objections, please let me 
know, it would save time.


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: [Modules] Handle tag types and complain about bad merges in C/Objective-C mode

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai added inline comments.



Comment at: clang/lib/Serialization/ASTReaderDecl.cpp:818
+// performance.
+RecordDecl *Canon = static_cast(RD->getCanonicalDecl());
+if (RD == Canon || Canon->getODRHash() == RD->getODRHash())

During investigation of a different issue noticed there is no error for the 
following test case

```lang=c++
#if defined(FIRST)
typedef struct FW FW;
struct FW {
  int x;
};
#elif defined(SECOND)
struct FW {
  float a;
};
#else
struct FW fw;
#endif
```

And that is because `Canon` is a forward declaration from the typedef and we 
don't compare non-definition with definition.


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: [Modules] Handle tag types and complain about bad merges in C/Objective-C mode

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai added a comment.

Ping.


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: [Modules] Handle tag types and complain about bad merges in C/Objective-C mode

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai added a comment.

Ping.


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: [Modules] Handle tag types and complain about bad merges in C/Objective-C mode

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai added inline comments.



Comment at: clang/lib/AST/DeclCXX.cpp:487
   // Only calculate hash on first call of getODRHash per record.
-  ODRHash Hash;
+  class ODRHash Hash;
   Hash.AddCXXRecordDecl(getDefinition());

rsmith wrote:
> I think this change is no longer necessary.
Reverted the change.



Comment at: clang/lib/Serialization/ASTReader.cpp:9585-9587
+assert(getContext().hasSameType(FirstField->getType(),
+SecondField->getType()));
+

bruno wrote:
> rsmith wrote:
> > bruno wrote:
> > > rsmith wrote:
> > > > I don't understand why this assertion would be correct if the above 
> > > > branch can ever be taken. It's possible for two different types to have 
> > > > the same hash, and it looks like we'll assert here if that happens. 
> > > > Should we be branching on `hasSameType` above instead of comparing 
> > > > hashes?
> > > This is trying to cover two things. The first one is to handle nested 
> > > anonymous unions, which might have the same type, but underlying 
> > > mismatching fields:
> > > ```
> > > #if defined(FIRST)
> > > struct AU {
> > >   union {
> > > int a;
> > >   };
> > > };
> > > #elif defined(SECOND)
> > > struct AU {
> > >   union {
> > > char a;
> > >   };
> > > };
> > > #else
> > > struct AU au;
> > > ```
> > > 
> > > The second is to allow for @interfaces (downstream patches at the moment) 
> > > to reuse logic to diagnose fields in `ODRDiagField` without having to add 
> > > its own check for the underlying types. Does that makes sense?
> > I still don't understand.
> > 
> > Either the types are always the same before the previous `if` and can only 
> > differ in type sugar (in which case this change is unnecessary), or the 
> > types can be different in more than just sugar (in which case this assert 
> > is wrong because there's no guarantee that different types will have 
> > different hashes).
> > 
> > What am I missing?
> @vsapsai: just to follow up here a bit. I don't remember the full context 
> anymore, but it should be fine to reintroduce this in a later patch with a 
> better explanation to @rsmith, or change the approach if it makes sense. 
> Thanks for taking this over!
I think it makes the most sense to remove the assertion. Based on the local 
code, it doesn't make much sense, as equality of field names doesn't imply the 
equality of field types. On the higher level, we were checking field types in a 
different place, specifically we populated `PendingOdrMergeChecks` in 
`ASTDeclReader::findExisting` and diagnosed it earlier in this mega-method 
`ASTReader::diagnoseOdrViolations` (error constant 
`err_module_odr_violation_missing_decl`). Also you can notice that the 
diagnostic for

```lang=c++
struct S { int x; };

struct S { float x; };
```
is different, it is
> 'S::x' from module 'SecondModule' is not present in definition of 'S' in 
> module 'FirstModule'

compared to
> 'S' has different definitions in different modules; first difference is 
> definition in module 'FirstModule' found field 'x' with type 'int'

that we emit in ODRDiagField.

It is possible to try to make unions behave the same way as structs (and keep 
the assertion) but I'm not sure it is worth it. Looks like the major difference 
is that not all tag types are true Redeclerable and I don't know if it is 
something we should change.



Comment at: clang/lib/Serialization/ASTWriterDecl.cpp:489
+  // of CXXRecordDecl.
+  Record.push_back(Writer.getLangOpts().CPlusPlus ? 0UL : D->getODRHash());
 

rsmith wrote:
> It would be better to avoid writing this at all for a CXXRecordDecl, to avoid 
> adding 6 unused bits per class to the bitcode. (Please also look at 
> isa(D) not at the LangOpts here.)
Changed to write ODR hash for everything except CXXRecordDecl (right now 
everything is RecordDecl). Also updated ASTReaderDecl.cpp. Not 100% sure I 
understand your comment correctly, so if my change is way off, please let me 
know.


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: [Modules] Handle tag types and complain about bad merges in C/Objective-C mode

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai updated this revision to Diff 353081.
vsapsai marked an inline comment as done.
vsapsai added a comment.

Rebase on top of "main" and address review comments.


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/ODRHash.h
  clang/include/clang/Serialization/ASTReader.h
  clang/lib/AST/Decl.cpp
  clang/lib/AST/ODRHash.cpp
  clang/lib/Serialization/ASTReader.cpp
  clang/lib/Serialization/ASTReaderDecl.cpp
  clang/lib/Serialization/ASTWriterDecl.cpp
  clang/test/Modules/odr_hash-record.c

Index: clang/test/Modules/odr_hash-record.c
===
--- /dev/null
+++ clang/test/Modules/odr_hash-record.c
@@ -0,0 +1,391 @@
+// Clear and create directories
+// RUN: rm -rf %t
+// RUN: mkdir %t
+// RUN: mkdir %t/cache
+// RUN: mkdir %t/Inputs
+
+// Build first header file
+// RUN: echo "#define FIRST" >> %t/Inputs/first.h
+// RUN: cat %s   >> %t/Inputs/first.h
+
+// Build second header file
+// RUN: echo "#define SECOND" >> %t/Inputs/second.h
+// RUN: cat %s>> %t/Inputs/second.h
+
+// Test that each header can compile
+// RUN: %clang_cc1 -fsyntax-only -x c %t/Inputs/first.h
+// RUN: %clang_cc1 -fsyntax-only -x c %t/Inputs/second.h
+
+// Build module map file
+// RUN: echo "module FirstModule {" >> %t/Inputs/module.map
+// RUN: echo "header \"first.h\""   >> %t/Inputs/module.map
+// RUN: echo "}">> %t/Inputs/module.map
+// RUN: echo "module SecondModule {">> %t/Inputs/module.map
+// RUN: echo "header \"second.h\""  >> %t/Inputs/module.map
+// RUN: echo "}">> %t/Inputs/module.map
+
+// Run test
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -x c \
+// RUN:   -fmodules -fimplicit-module-maps -fmodules-cache-path=%t/cache \
+// RUN:   -I%t/Inputs -verify %s
+
+#if !defined(FIRST) && !defined(SECOND)
+#include "first.h"
+#include "second.h"
+#endif
+
+#if defined(FIRST)
+struct S1 {};
+struct S1 s1a;
+#elif defined(SECOND)
+struct S1 {};
+#else
+struct S1 s1;
+#endif
+
+#if defined(FIRST)
+struct S2 {
+  int x;
+  int y;
+};
+#elif defined(SECOND)
+struct S2 {
+  int y;
+  int x;
+};
+#else
+struct S2 s2;
+// expected-error@first.h:* {{'S2' has different definitions in different modules; first difference is definition in module 'FirstModule' found field 'x'}}
+// expected-note@second.h:* {{but in 'SecondModule' found field 'y'}}
+#endif
+
+#if defined(FIRST)
+struct S3 {
+  double x;
+};
+#elif defined(SECOND)
+struct S3 {
+  int x;
+};
+#else
+struct S3 s3;
+// expected-error@second.h:* {{'S3::x' from module 'SecondModule' is not present in definition of 'struct S3' in module 'FirstModule'}}
+// expected-note@first.h:* {{declaration of 'x' does not match}}
+#endif
+
+#if defined(FIRST)
+typedef int A;
+struct S4 {
+  A x;
+};
+
+struct S5 {
+  A x;
+};
+#elif defined(SECOND)
+typedef int B;
+struct S4 {
+  B x;
+};
+
+struct S5 {
+  int x;
+};
+#else
+struct S4 s4;
+// expected-error@first.h:* {{'S4' has different definitions in different modules; first difference is definition in module 'FirstModule' found field 'x' with type 'A' (aka 'int')}}
+// expected-note@second.h:* {{but in 'SecondModule' found field 'x' with type 'B' (aka 'int')}}
+
+struct S5 s5;
+// expected-error@first.h:* {{'S5' has different definitions in different modules; first difference is definition in module 'FirstModule' found field 'x' with type 'A' (aka 'int')}}
+// expected-note@second.h:* {{but in 'SecondModule' found field 'x' with type 'int'}}
+#endif
+
+#if defined(FIRST)
+struct S6 {
+  unsigned x;
+};
+#elif defined(SECOND)
+struct S6 {
+  unsigned x : 1;
+};
+#else
+struct S6 s6;
+// expected-error@first.h:* {{'S6' has different definitions in different modules; first difference is definition in module 'FirstModule' found non-bitfield 'x'}}
+// expected-note@second.h:* {{but in 'SecondModule' found bitfield 'x'}}
+#endif
+
+#if defined(FIRST)
+struct S7 {
+  unsigned x : 2;
+};
+#elif defined(SECOND)
+struct S7 {
+  unsigned x : 1;
+};
+#else
+struct S7 s7;
+// expected-error@first.h:* {{'S7' has different definitions in different modules; first difference is definition in module 'FirstModule' found bitfield 'x' with one width expression}}
+// expected-note@second.h:* {{but in 'SecondModule' found bitfield 'x' with different width expression}}
+#endif
+
+#if defined(FIRST)
+struct S8 {
+  unsigned x : 2;
+};
+#elif defined(SECOND)
+struct S8 {
+  unsigned x : 1 + 1;
+};
+#else
+struct S8 s8;
+// expected-error@first.h:* {{'S8' has different definitions in different modules; first difference is definition in module 'FirstModule' found bitfield 'x' with one width expression}}
+// expected-note@second.h:* {{but in 'SecondModule' found bitfield 'x' with different width expression}}
+#endif
+
+#if 

[PATCH] D71734: [Modules] Handle tag types and complain about bad merges in C/Objective-C mode

[Bruno Cardoso Lopes via Phabricator via cfe-commits]

bruno added inline comments.



Comment at: clang/lib/Serialization/ASTReader.cpp:9585-9587
+assert(getContext().hasSameType(FirstField->getType(),
+SecondField->getType()));
+

rsmith wrote:
> bruno wrote:
> > rsmith wrote:
> > > I don't understand why this assertion would be correct if the above 
> > > branch can ever be taken. It's possible for two different types to have 
> > > the same hash, and it looks like we'll assert here if that happens. 
> > > Should we be branching on `hasSameType` above instead of comparing hashes?
> > This is trying to cover two things. The first one is to handle nested 
> > anonymous unions, which might have the same type, but underlying 
> > mismatching fields:
> > ```
> > #if defined(FIRST)
> > struct AU {
> >   union {
> > int a;
> >   };
> > };
> > #elif defined(SECOND)
> > struct AU {
> >   union {
> > char a;
> >   };
> > };
> > #else
> > struct AU au;
> > ```
> > 
> > The second is to allow for @interfaces (downstream patches at the moment) 
> > to reuse logic to diagnose fields in `ODRDiagField` without having to add 
> > its own check for the underlying types. Does that makes sense?
> I still don't understand.
> 
> Either the types are always the same before the previous `if` and can only 
> differ in type sugar (in which case this change is unnecessary), or the types 
> can be different in more than just sugar (in which case this assert is wrong 
> because there's no guarantee that different types will have different hashes).
> 
> What am I missing?
@vsapsai: just to follow up here a bit. I don't remember the full context 
anymore, but it should be fine to reintroduce this in a later patch with a 
better explanation to @rsmith, or change the approach if it makes sense. Thanks 
for taking this over!


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: [Modules] Handle tag types and complain about bad merges in C/Objective-C mode

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.



Comment at: clang/lib/AST/DeclCXX.cpp:487
   // Only calculate hash on first call of getODRHash per record.
-  ODRHash Hash;
+  class ODRHash Hash;
   Hash.AddCXXRecordDecl(getDefinition());

I think this change is no longer necessary.



Comment at: clang/lib/Serialization/ASTReader.cpp:9585-9587
+assert(getContext().hasSameType(FirstField->getType(),
+SecondField->getType()));
+

bruno wrote:
> rsmith wrote:
> > I don't understand why this assertion would be correct if the above branch 
> > can ever be taken. It's possible for two different types to have the same 
> > hash, and it looks like we'll assert here if that happens. Should we be 
> > branching on `hasSameType` above instead of comparing hashes?
> This is trying to cover two things. The first one is to handle nested 
> anonymous unions, which might have the same type, but underlying mismatching 
> fields:
> ```
> #if defined(FIRST)
> struct AU {
>   union {
> int a;
>   };
> };
> #elif defined(SECOND)
> struct AU {
>   union {
> char a;
>   };
> };
> #else
> struct AU au;
> ```
> 
> The second is to allow for @interfaces (downstream patches at the moment) to 
> reuse logic to diagnose fields in `ODRDiagField` without having to add its 
> own check for the underlying types. Does that makes sense?
I still don't understand.

Either the types are always the same before the previous `if` and can only 
differ in type sugar (in which case this change is unnecessary), or the types 
can be different in more than just sugar (in which case this assert is wrong 
because there's no guarantee that different types will have different hashes).

What am I missing?



Comment at: clang/lib/Serialization/ASTWriterDecl.cpp:489
+  // of CXXRecordDecl.
+  Record.push_back(Writer.getLangOpts().CPlusPlus ? 0UL : D->getODRHash());
 

It would be better to avoid writing this at all for a CXXRecordDecl, to avoid 
adding 6 unused bits per class to the bitcode. (Please also look at 
isa(D) not at the LangOpts here.)


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: [Modules] Handle tag types and complain about bad merges in C/Objective-C mode

[Bruno Cardoso Lopes via Phabricator via cfe-commits]

bruno added a comment.

Ping!


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: [Modules] Handle tag types and complain about bad merges in C/Objective-C mode

[Bruno Cardoso Lopes via Phabricator via cfe-commits]

bruno updated this revision to Diff 244435.
bruno added a comment.

Address Richard's review.


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/ODRHash.h
  clang/include/clang/Serialization/ASTReader.h
  clang/lib/AST/Decl.cpp
  clang/lib/AST/DeclCXX.cpp
  clang/lib/AST/ODRHash.cpp
  clang/lib/Serialization/ASTReader.cpp
  clang/lib/Serialization/ASTReaderDecl.cpp
  clang/lib/Serialization/ASTWriterDecl.cpp
  clang/test/Modules/odr_hash-record.c

Index: clang/test/Modules/odr_hash-record.c
===
--- /dev/null
+++ clang/test/Modules/odr_hash-record.c
@@ -0,0 +1,391 @@
+// Clear and create directories
+// RUN: rm -rf %t
+// RUN: mkdir %t
+// RUN: mkdir %t/cache
+// RUN: mkdir %t/Inputs
+
+// Build first header file
+// RUN: echo "#define FIRST" >> %t/Inputs/first.h
+// RUN: cat %s   >> %t/Inputs/first.h
+
+// Build second header file
+// RUN: echo "#define SECOND" >> %t/Inputs/second.h
+// RUN: cat %s>> %t/Inputs/second.h
+
+// Test that each header can compile
+// RUN: %clang_cc1 -fsyntax-only -x c %t/Inputs/first.h
+// RUN: %clang_cc1 -fsyntax-only -x c %t/Inputs/second.h
+
+// Build module map file
+// RUN: echo "module FirstModule {" >> %t/Inputs/module.map
+// RUN: echo "header \"first.h\""   >> %t/Inputs/module.map
+// RUN: echo "}">> %t/Inputs/module.map
+// RUN: echo "module SecondModule {">> %t/Inputs/module.map
+// RUN: echo "header \"second.h\""  >> %t/Inputs/module.map
+// RUN: echo "}">> %t/Inputs/module.map
+
+// Run test
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -x c \
+// RUN:   -fmodules -fimplicit-module-maps -fmodules-cache-path=%t/cache \
+// RUN:   -I%t/Inputs -verify %s
+
+#if !defined(FIRST) && !defined(SECOND)
+#include "first.h"
+#include "second.h"
+#endif
+
+#if defined(FIRST)
+struct S1 {};
+struct S1 s1a;
+#elif defined(SECOND)
+struct S1 {};
+#else
+struct S1 s1;
+#endif
+
+#if defined(FIRST)
+struct S2 {
+  int x;
+  int y;
+};
+#elif defined(SECOND)
+struct S2 {
+  int y;
+  int x;
+};
+#else
+struct S2 s2;
+// expected-error@first.h:* {{'S2' has different definitions in different modules; first difference is definition in module 'FirstModule' found field 'x'}}
+// expected-note@second.h:* {{but in 'SecondModule' found field 'y'}}
+#endif
+
+#if defined(FIRST)
+struct S3 {
+  double x;
+};
+#elif defined(SECOND)
+struct S3 {
+  int x;
+};
+#else
+struct S3 s3;
+// expected-error@second.h:* {{'S3::x' from module 'SecondModule' is not present in definition of 'struct S3' in module 'FirstModule'}}
+// expected-note@first.h:* {{declaration of 'x' does not match}}
+#endif
+
+#if defined(FIRST)
+typedef int A;
+struct S4 {
+  A x;
+};
+
+struct S5 {
+  A x;
+};
+#elif defined(SECOND)
+typedef int B;
+struct S4 {
+  B x;
+};
+
+struct S5 {
+  int x;
+};
+#else
+struct S4 s4;
+// expected-error@first.h:* {{'S4' has different definitions in different modules; first difference is definition in module 'FirstModule' found field 'x' with type 'A' (aka 'int')}}
+// expected-note@second.h:* {{but in 'SecondModule' found field 'x' with type 'B' (aka 'int')}}
+
+struct S5 s5;
+// expected-error@first.h:* {{'S5' has different definitions in different modules; first difference is definition in module 'FirstModule' found field 'x' with type 'A' (aka 'int')}}
+// expected-note@second.h:* {{but in 'SecondModule' found field 'x' with type 'int'}}
+#endif
+
+#if defined(FIRST)
+struct S6 {
+  unsigned x;
+};
+#elif defined(SECOND)
+struct S6 {
+  unsigned x : 1;
+};
+#else
+struct S6 s6;
+// expected-error@first.h:* {{'S6' has different definitions in different modules; first difference is definition in module 'FirstModule' found non-bitfield 'x'}}
+// expected-note@second.h:* {{but in 'SecondModule' found bitfield 'x'}}
+#endif
+
+#if defined(FIRST)
+struct S7 {
+  unsigned x : 2;
+};
+#elif defined(SECOND)
+struct S7 {
+  unsigned x : 1;
+};
+#else
+struct S7 s7;
+// expected-error@first.h:* {{'S7' has different definitions in different modules; first difference is definition in module 'FirstModule' found bitfield 'x' with one width expression}}
+// expected-note@second.h:* {{but in 'SecondModule' found bitfield 'x' with different width expression}}
+#endif
+
+#if defined(FIRST)
+struct S8 {
+  unsigned x : 2;
+};
+#elif defined(SECOND)
+struct S8 {
+  unsigned x : 1 + 1;
+};
+#else
+struct S8 s8;
+// expected-error@first.h:* {{'S8' has different definitions in different modules; first difference is definition in module 'FirstModule' found bitfield 'x' with one width expression}}
+// expected-note@second.h:* {{but in 'SecondModule' found bitfield 'x' with different width expression}}
+#endif
+
+#if defined(FIRST)
+struct S12 {
+  unsigned x[5];
+};

[PATCH] D71734: [Modules] Handle tag types and complain about bad merges in C/Objective-C mode

[Bruno Cardoso Lopes via Phabricator via cfe-commits]

bruno marked 3 inline comments as done.
bruno added a comment.

Thanks for taking a look Richard, comments inline.




Comment at: clang/include/clang/AST/Decl.h:4009-4010
+
+  /// Store the ODR hash for this decl.
+  unsigned ODRHash;
 };

rsmith wrote:
> We shouldn't store this here; this will make all `CXXRecordDecl`s larger to 
> store a field they will never look at.
> 
> We have 27 spare trailing bits in `RecordDeclBitfields` (28 if you don't add 
> the `HasODRHash` bit and use 0 to mean "hash not computed"). Can we store 
> this there instead of here?
Makes sense!



Comment at: clang/lib/AST/ODRHash.cpp:480-481
+if (auto *SubRD = dyn_cast(SubDecl)) {
+  if (!SubRD->isCompleteDefinition())
+continue;
+  ID.AddInteger(SubRD->getODRHash());

rsmith wrote:
> This is, at least in principle, not reliable. After definition merging, we 
> could have picked a different definition of this record type as "the" 
> definition. (It's probably not straightforward to get this to happen in 
> practice, and maybe not even possible, but I'm not certain of that.)
> 
> Maybe we could add to `TagDecl` something like
> 
> ```
> bool isThisDeclarationADemotedDefinition() const {
>   return !isThisDeclarationADefinition() && BraceRange.isValid();
> }
> ```
> 
> and then check `isThisDeclarationADefinition() || 
> isThisDeclarationADemotedDefinition()` here? (I'm not sure we always update 
> `BraceRange` properly for demoted definitions either, so maybe that's not 
> quite right.)
There are two things I realized while looking at this:

- What I really intended here was to hash underlying anonymous structs/unions, 
not just any underlying RecordDecl. Will change the logic in 
`ODRHash::AddRecordDecl` do reflect that properly.
- This patch was skipping `RecordDecl`s without definitions during ODR 
diagnostics, I've changed to instead decide at merge time if we want to 
register those Decls for later diagnose, and the new rule is clang skips 
`RecordDecl`s that disagree on whether they have a definition. This should 
still allow clang to diagnose declarations without defintions that have 
different attributes (which I intend to add for specific attributes in future 
patches).

Will update the patch to reflect that. WDYT?



Comment at: clang/lib/Serialization/ASTReader.cpp:9585-9587
+assert(getContext().hasSameType(FirstField->getType(),
+SecondField->getType()));
+

rsmith wrote:
> I don't understand why this assertion would be correct if the above branch 
> can ever be taken. It's possible for two different types to have the same 
> hash, and it looks like we'll assert here if that happens. Should we be 
> branching on `hasSameType` above instead of comparing hashes?
This is trying to cover two things. The first one is to handle nested anonymous 
unions, which might have the same type, but underlying mismatching fields:
```
#if defined(FIRST)
struct AU {
  union {
int a;
  };
};
#elif defined(SECOND)
struct AU {
  union {
char a;
  };
};
#else
struct AU au;
```

The second is to allow for @interfaces (downstream patches at the moment) to 
reuse logic to diagnose fields in `ODRDiagField` without having to add its own 
check for the underlying types. Does that makes sense?


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: [Modules] Handle tag types and complain about bad merges in C/Objective-C mode

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.



Comment at: clang/include/clang/AST/Decl.h:4009-4010
+
+  /// Store the ODR hash for this decl.
+  unsigned ODRHash;
 };

We shouldn't store this here; this will make all `CXXRecordDecl`s larger to 
store a field they will never look at.

We have 27 spare trailing bits in `RecordDeclBitfields` (28 if you don't add 
the `HasODRHash` bit and use 0 to mean "hash not computed"). Can we store this 
there instead of here?



Comment at: clang/lib/AST/ODRHash.cpp:480-481
+if (auto *SubRD = dyn_cast(SubDecl)) {
+  if (!SubRD->isCompleteDefinition())
+continue;
+  ID.AddInteger(SubRD->getODRHash());

This is, at least in principle, not reliable. After definition merging, we 
could have picked a different definition of this record type as "the" 
definition. (It's probably not straightforward to get this to happen in 
practice, and maybe not even possible, but I'm not certain of that.)

Maybe we could add to `TagDecl` something like

```
bool isThisDeclarationADemotedDefinition() const {
  return !isThisDeclarationADefinition() && BraceRange.isValid();
}
```

and then check `isThisDeclarationADefinition() || 
isThisDeclarationADemotedDefinition()` here? (I'm not sure we always update 
`BraceRange` properly for demoted definitions either, so maybe that's not quite 
right.)



Comment at: clang/lib/Serialization/ASTReader.cpp:9585-9587
+assert(getContext().hasSameType(FirstField->getType(),
+SecondField->getType()));
+

I don't understand why this assertion would be correct if the above branch can 
ever be taken. It's possible for two different types to have the same hash, and 
it looks like we'll assert here if that happens. Should we be branching on 
`hasSameType` above instead of comparing hashes?


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: [Modules] Handle tag types and complain about bad merges in C/Objective-C mode

[Bruno Cardoso Lopes via Phabricator via cfe-commits]

bruno updated this revision to Diff 241855.
bruno added a comment.

- Update patch after Volodymyr's review.
- Refactor bits done as part of rG90f58eaeff5f 
, update 
it to remove that part.


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/ODRHash.h
  clang/include/clang/Serialization/ASTReader.h
  clang/lib/AST/Decl.cpp
  clang/lib/AST/DeclCXX.cpp
  clang/lib/AST/ODRHash.cpp
  clang/lib/Serialization/ASTReader.cpp
  clang/lib/Serialization/ASTReaderDecl.cpp
  clang/lib/Serialization/ASTWriterDecl.cpp
  clang/test/Modules/odr_hash-record.c

Index: clang/test/Modules/odr_hash-record.c
===
--- /dev/null
+++ clang/test/Modules/odr_hash-record.c
@@ -0,0 +1,381 @@
+// Clear and create directories
+// RUN: rm -rf %t
+// RUN: mkdir %t
+// RUN: mkdir %t/cache
+// RUN: mkdir %t/Inputs
+
+// Build first header file
+// RUN: echo "#define FIRST" >> %t/Inputs/first.h
+// RUN: cat %s   >> %t/Inputs/first.h
+
+// Build second header file
+// RUN: echo "#define SECOND" >> %t/Inputs/second.h
+// RUN: cat %s>> %t/Inputs/second.h
+
+// Test that each header can compile
+// RUN: %clang_cc1 -fsyntax-only -x c %t/Inputs/first.h
+// RUN: %clang_cc1 -fsyntax-only -x c %t/Inputs/second.h
+
+// Build module map file
+// RUN: echo "module FirstModule {" >> %t/Inputs/module.map
+// RUN: echo "header \"first.h\""   >> %t/Inputs/module.map
+// RUN: echo "}">> %t/Inputs/module.map
+// RUN: echo "module SecondModule {">> %t/Inputs/module.map
+// RUN: echo "header \"second.h\""  >> %t/Inputs/module.map
+// RUN: echo "}">> %t/Inputs/module.map
+
+// Run test
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -x c \
+// RUN:   -fmodules -fimplicit-module-maps -fmodules-cache-path=%t/cache \
+// RUN:   -I%t/Inputs -verify %s
+
+#if !defined(FIRST) && !defined(SECOND)
+#include "first.h"
+#include "second.h"
+#endif
+
+#if defined(FIRST)
+struct S1 {};
+struct S1 s1a;
+#elif defined(SECOND)
+struct S1 {};
+#else
+struct S1 s1;
+#endif
+
+#if defined(FIRST)
+struct S2 {
+  int x;
+  int y;
+};
+#elif defined(SECOND)
+struct S2 {
+  int y;
+  int x;
+};
+#else
+struct S2 s2;
+// expected-error@first.h:* {{'S2' has different definitions in different modules; first difference is definition in module 'FirstModule' found field 'x'}}
+// expected-note@second.h:* {{but in 'SecondModule' found field 'y'}}
+#endif
+
+#if defined(FIRST)
+struct S3 {
+  double x;
+};
+#elif defined(SECOND)
+struct S3 {
+  int x;
+};
+#else
+struct S3 s3;
+// expected-error@second.h:* {{'S3::x' from module 'SecondModule' is not present in definition of 'struct S3' in module 'FirstModule'}}
+// expected-note@first.h:* {{declaration of 'x' does not match}}
+#endif
+
+#if defined(FIRST)
+typedef int A;
+struct S4 {
+  A x;
+};
+
+struct S5 {
+  A x;
+};
+#elif defined(SECOND)
+typedef int B;
+struct S4 {
+  B x;
+};
+
+struct S5 {
+  int x;
+};
+#else
+struct S4 s4;
+// expected-error@first.h:* {{'S4' has different definitions in different modules; first difference is definition in module 'FirstModule' found field 'x' with type 'A' (aka 'int')}}
+// expected-note@second.h:* {{but in 'SecondModule' found field 'x' with type 'B' (aka 'int')}}
+
+struct S5 s5;
+// expected-error@first.h:* {{'S5' has different definitions in different modules; first difference is definition in module 'FirstModule' found field 'x' with type 'A' (aka 'int')}}
+// expected-note@second.h:* {{but in 'SecondModule' found field 'x' with type 'int'}}
+#endif
+
+#if defined(FIRST)
+struct S6 {
+  unsigned x;
+};
+#elif defined(SECOND)
+struct S6 {
+  unsigned x : 1;
+};
+#else
+struct S6 s6;
+// expected-error@first.h:* {{'S6' has different definitions in different modules; first difference is definition in module 'FirstModule' found non-bitfield 'x'}}
+// expected-note@second.h:* {{but in 'SecondModule' found bitfield 'x'}}
+#endif
+
+#if defined(FIRST)
+struct S7 {
+  unsigned x : 2;
+};
+#elif defined(SECOND)
+struct S7 {
+  unsigned x : 1;
+};
+#else
+struct S7 s7;
+// expected-error@first.h:* {{'S7' has different definitions in different modules; first difference is definition in module 'FirstModule' found bitfield 'x' with one width expression}}
+// expected-note@second.h:* {{but in 'SecondModule' found bitfield 'x' with different width expression}}
+#endif
+
+#if defined(FIRST)
+struct S8 {
+  unsigned x : 2;
+};
+#elif defined(SECOND)
+struct S8 {
+  unsigned x : 1 + 1;
+};
+#else
+struct S8 s8;
+// expected-error@first.h:* {{'S8' has different definitions in different modules; first difference is definition in module 'FirstModule' found bitfield 'x' with one width expression}}
+// 

[PATCH] D71734: [Modules] Handle tag types and complain about bad merges in C/Objective-C mode

[Bruno Cardoso Lopes via Phabricator via cfe-commits]

bruno added a comment.

> - Why are you adding ODR hash support for `RecordDecl` and not `TagDecl`? We 
> already have support for `EnumDecl`, so `TagDecl` seems like a good candidate 
> to cover both. Honestly, I don't know if it is possible or a good idea but it 
> looks plausible enough to consider.

The reason is that in C++ ODR diagnostics for TagDecl are handled as part of 
CXXRecordDecl, I didn't want add a path in TagDecl for C/ObjC only.

> - Are anonymous structs working? Worth adding test cases.

They are at the top level but not nested ones, I fixed that and should upload a 
new patch with that next.

> - Are unions working? Didn't notice any code specifically for them but 
> `RecordDecl` covers both structs and unions, so they should be working and we 
> need to test that.

Done in upcoming patch.

> - Few testing additions. These cases might be already covered or might be low 
> value, so take these suggestions with a grain of salt:
>   - test self-referential structs like `struct Node { struct Node *next; };`
>   - test comparing structs and forward declarations, e.g., `struct S;` and 
> `struct S { ... };`, and another couple `struct S { ... };` and `struct S; 
> struct S { ... };` The motivation is to make sure we aren't stumped when we 
> cannot find struct definition or when the definition is in unexpected place.

Ditto.

> Heads up in case it affects you refactoring work:
>  While looking through this code, I found a bug I previously made.  I fixed 
> it with a small change, but that lies in the middle of your refactoring 
> during FieldDecl handling.  The change is here:
> 
> https://reviews.llvm.org/rGa60e8927297005898b10a46300d929ba5cf7833c

Thanks for the heads up @rtrieu, I end up including that as part of 
rG90f58eaeff5f 



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: [Modules] Handle tag types and complain about bad merges in C/Objective-C mode

[Richard Trieu via Phabricator via cfe-commits]

rtrieu added a comment.

Heads up in case it affects you refactoring work:
While looking through this code, I found a bug I previously made.  I fixed it 
with a small change, but that lies in the middle of your refactoring during 
FieldDecl handling.  The change is here:

https://reviews.llvm.org/rGa60e8927297005898b10a46300d929ba5cf7833c


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: [Modules] Handle tag types and complain about bad merges in C/Objective-C mode

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai added a comment.

Think I'll need to make another review pass but here are my comments so far:

- Why are you adding ODR hash support for `RecordDecl` and not `TagDecl`? We 
already have support for `EnumDecl`, so `TagDecl` seems like a good candidate 
to cover both. Honestly, I don't know if it is possible or a good idea but it 
looks plausible enough to consider.
- Are anonymous structs working? Worth adding test cases.
- Are unions working? Didn't notice any code specifically for them but 
`RecordDecl` covers both structs and unions, so they should be working and we 
need to test that.
- Few testing additions. These cases might be already covered or might be low 
value, so take these suggestions with a grain of salt:
  - test self-referential structs like `struct Node { struct Node *next; };`
  - test comparing structs and forward declarations, e.g., `struct S;` and 
`struct S { ... };`, and another couple `struct S { ... };` and `struct S; 
struct S { ... };` The motivation is to make sure we aren't stumped when we 
cannot find struct definition or when the definition is in unexpected place.




Comment at: clang/lib/AST/Decl.cpp:4519
+  Hash.AddRecordDecl(this);
+  setHasODRHash();
+  ODRHash = Hash.CalculateHash();

To be consistent with the existing code, it is better to call it as 
`setHasODRHash(true)`


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: [Modules] Handle tag types and complain about bad merges in C/Objective-C mode

[Richard Trieu via Phabricator via cfe-commits]

rtrieu added inline comments.



Comment at: clang/lib/Serialization/ASTReader.cpp:11007
 
+  // Issue any pending ODR-failure diagnostics.
+  for (auto  : RecordOdrMergeFailures) {

bruno wrote:
> rtrieu wrote:
> > Is this just a copy of the other loop?  That's a lot of code duplication.
> There's a lot of copy but a lot of CXXRecord specific logic got trimmed out 
> and there are small differences in the approach, so a direct refactor of it 
> won't work, but there's certainly more small pieces that can be factored out, 
> will follow up with that.
I like this refactoring much better.  Probably more refactoring could be done 
to this function since it's so long, but that's an issue for another time.


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: [Modules] Handle tag types and complain about bad merges in C/Objective-C mode

[Bruno Cardoso Lopes via Phabricator via cfe-commits]

bruno marked an inline comment as done.
bruno added inline comments.



Comment at: clang/lib/Serialization/ASTReader.cpp:11007
 
+  // Issue any pending ODR-failure diagnostics.
+  for (auto  : RecordOdrMergeFailures) {

rtrieu wrote:
> Is this just a copy of the other loop?  That's a lot of code duplication.
There's a lot of copy but a lot of CXXRecord specific logic got trimmed out and 
there are small differences in the approach, so a direct refactor of it won't 
work, but there's certainly more small pieces that can be factored out, will 
follow up with that.


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: [Modules] Handle tag types and complain about bad merges in C/Objective-C mode

[Richard Trieu via Phabricator via cfe-commits]

rtrieu added inline comments.



Comment at: clang/lib/Serialization/ASTReader.cpp:11007
 
+  // Issue any pending ODR-failure diagnostics.
+  for (auto  : RecordOdrMergeFailures) {

Is this just a copy of the other loop?  That's a lot of code duplication.


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: [Modules] Handle tag types and complain about bad merges in C/Objective-C mode

[Bruno Cardoso Lopes via Phabricator via cfe-commits]

bruno updated this revision to Diff 236965.
bruno added a comment.

Remove some FIXMEs that are now done.


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/ODRHash.h
  clang/include/clang/Serialization/ASTReader.h
  clang/lib/AST/Decl.cpp
  clang/lib/AST/DeclCXX.cpp
  clang/lib/AST/ODRHash.cpp
  clang/lib/Serialization/ASTReader.cpp
  clang/lib/Serialization/ASTReaderDecl.cpp
  clang/lib/Serialization/ASTWriterDecl.cpp
  clang/test/Modules/odr_hash-record.c

Index: clang/test/Modules/odr_hash-record.c
===
--- /dev/null
+++ clang/test/Modules/odr_hash-record.c
@@ -0,0 +1,293 @@
+// Clear and create directories
+// RUN: rm -rf %t
+// RUN: mkdir %t
+// RUN: mkdir %t/cache
+// RUN: mkdir %t/Inputs
+
+// Build first header file
+// RUN: echo "#define FIRST" >> %t/Inputs/first.h
+// RUN: cat %s   >> %t/Inputs/first.h
+
+// Build second header file
+// RUN: echo "#define SECOND" >> %t/Inputs/second.h
+// RUN: cat %s>> %t/Inputs/second.h
+
+// Test that each header can compile
+// RUN: %clang_cc1 -fsyntax-only -x c %t/Inputs/first.h
+// RUN: %clang_cc1 -fsyntax-only -x c %t/Inputs/second.h
+
+// Build module map file
+// RUN: echo "module FirstModule {" >> %t/Inputs/module.map
+// RUN: echo "header \"first.h\""   >> %t/Inputs/module.map
+// RUN: echo "}">> %t/Inputs/module.map
+// RUN: echo "module SecondModule {">> %t/Inputs/module.map
+// RUN: echo "header \"second.h\""  >> %t/Inputs/module.map
+// RUN: echo "}">> %t/Inputs/module.map
+
+// Run test
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -x c \
+// RUN:   -fmodules -fimplicit-module-maps -fmodules-cache-path=%t/cache \
+// RUN:   -I%t/Inputs -verify %s
+
+#if !defined(FIRST) && !defined(SECOND)
+#include "first.h"
+#include "second.h"
+#endif
+
+#if defined(FIRST)
+struct S1 {};
+struct S1 s1a;
+#elif defined(SECOND)
+struct S1 {};
+#else
+struct S1 s1;
+#endif
+
+#if defined(FIRST)
+struct S2 {
+  int x;
+  int y;
+};
+#elif defined(SECOND)
+struct S2 {
+  int y;
+  int x;
+};
+#else
+struct S2 s2;
+// expected-error@first.h:* {{'S2' has different definitions in different modules; first difference is definition in module 'FirstModule' found field 'x'}}
+// expected-note@second.h:* {{but in 'SecondModule' found field 'y'}}
+#endif
+
+#if defined(FIRST)
+struct S3 {
+  double x;
+};
+#elif defined(SECOND)
+struct S3 {
+  int x;
+};
+#else
+struct S3 s3;
+// expected-error@second.h:* {{'S3::x' from module 'SecondModule' is not present in definition of 'struct S3' in module 'FirstModule'}}
+// expected-note@first.h:* {{declaration of 'x' does not match}}
+#endif
+
+#if defined(FIRST)
+typedef int A;
+struct S4 {
+  A x;
+};
+
+struct S5 {
+  A x;
+};
+#elif defined(SECOND)
+typedef int B;
+struct S4 {
+  B x;
+};
+
+struct S5 {
+  int x;
+};
+#else
+struct S4 s4;
+// expected-error@first.h:* {{'S4' has different definitions in different modules; first difference is definition in module 'FirstModule' found field 'x' with type 'A' (aka 'int')}}
+// expected-note@second.h:* {{but in 'SecondModule' found field 'x' with type 'B' (aka 'int')}}
+
+struct S5 s5;
+// expected-error@first.h:* {{'S5' has different definitions in different modules; first difference is definition in module 'FirstModule' found field 'x' with type 'A' (aka 'int')}}
+// expected-note@second.h:* {{but in 'SecondModule' found field 'x' with type 'int'}}
+#endif
+
+#if defined(FIRST)
+struct S6 {
+  unsigned x;
+};
+#elif defined(SECOND)
+struct S6 {
+  unsigned x : 1;
+};
+#else
+struct S6 s6;
+// expected-error@first.h:* {{'S6' has different definitions in different modules; first difference is definition in module 'FirstModule' found non-bitfield 'x'}}
+// expected-note@second.h:* {{but in 'SecondModule' found bitfield 'x'}}
+#endif
+
+#if defined(FIRST)
+struct S7 {
+  unsigned x : 2;
+};
+#elif defined(SECOND)
+struct S7 {
+  unsigned x : 1;
+};
+#else
+struct S7 s7;
+// expected-error@first.h:* {{'S7' has different definitions in different modules; first difference is definition in module 'FirstModule' found bitfield 'x' with one width expression}}
+// expected-note@second.h:* {{but in 'SecondModule' found bitfield 'x' with different width expression}}
+#endif
+
+#if defined(FIRST)
+struct S8 {
+  unsigned x : 2;
+};
+#elif defined(SECOND)
+struct S8 {
+  unsigned x : 1 + 1;
+};
+#else
+struct S8 s8;
+// expected-error@first.h:* {{'S8' has different definitions in different modules; first difference is definition in module 'FirstModule' found bitfield 'x' with one width expression}}
+// expected-note@second.h:* {{but in 'SecondModule' found bitfield 'x' with different width expression}}
+#endif
+
+#if defined(FIRST)
+struct S12 {
+  unsigned 

[PATCH] D71734: [Modules] Handle tag types and complain about bad merges in C/Objective-C mode

[Bruno Cardoso Lopes via Phabricator via cfe-commits]

bruno updated this revision to Diff 236964.
bruno added a comment.

Change the approach: handle type merging for tag types using the ODRHash 
mechanism


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/ODRHash.h
  clang/include/clang/Serialization/ASTReader.h
  clang/lib/AST/Decl.cpp
  clang/lib/AST/DeclCXX.cpp
  clang/lib/AST/ODRHash.cpp
  clang/lib/Serialization/ASTReader.cpp
  clang/lib/Serialization/ASTReaderDecl.cpp
  clang/lib/Serialization/ASTWriterDecl.cpp
  clang/test/Modules/odr_hash-record.c

Index: clang/test/Modules/odr_hash-record.c
===
--- /dev/null
+++ clang/test/Modules/odr_hash-record.c
@@ -0,0 +1,293 @@
+// Clear and create directories
+// RUN: rm -rf %t
+// RUN: mkdir %t
+// RUN: mkdir %t/cache
+// RUN: mkdir %t/Inputs
+
+// Build first header file
+// RUN: echo "#define FIRST" >> %t/Inputs/first.h
+// RUN: cat %s   >> %t/Inputs/first.h
+
+// Build second header file
+// RUN: echo "#define SECOND" >> %t/Inputs/second.h
+// RUN: cat %s>> %t/Inputs/second.h
+
+// Test that each header can compile
+// RUN: %clang_cc1 -fsyntax-only -x c %t/Inputs/first.h
+// RUN: %clang_cc1 -fsyntax-only -x c %t/Inputs/second.h
+
+// Build module map file
+// RUN: echo "module FirstModule {" >> %t/Inputs/module.map
+// RUN: echo "header \"first.h\""   >> %t/Inputs/module.map
+// RUN: echo "}">> %t/Inputs/module.map
+// RUN: echo "module SecondModule {">> %t/Inputs/module.map
+// RUN: echo "header \"second.h\""  >> %t/Inputs/module.map
+// RUN: echo "}">> %t/Inputs/module.map
+
+// Run test
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -x c \
+// RUN:   -fmodules -fimplicit-module-maps -fmodules-cache-path=%t/cache \
+// RUN:   -I%t/Inputs -verify %s
+
+#if !defined(FIRST) && !defined(SECOND)
+#include "first.h"
+#include "second.h"
+#endif
+
+#if defined(FIRST)
+struct S1 {};
+struct S1 s1a;
+#elif defined(SECOND)
+struct S1 {};
+#else
+struct S1 s1;
+#endif
+
+#if defined(FIRST)
+struct S2 {
+  int x;
+  int y;
+};
+#elif defined(SECOND)
+struct S2 {
+  int y;
+  int x;
+};
+#else
+struct S2 s2;
+// expected-error@first.h:* {{'S2' has different definitions in different modules; first difference is definition in module 'FirstModule' found field 'x'}}
+// expected-note@second.h:* {{but in 'SecondModule' found field 'y'}}
+#endif
+
+#if defined(FIRST)
+struct S3 {
+  double x;
+};
+#elif defined(SECOND)
+struct S3 {
+  int x;
+};
+#else
+struct S3 s3;
+// expected-error@second.h:* {{'S3::x' from module 'SecondModule' is not present in definition of 'struct S3' in module 'FirstModule'}}
+// expected-note@first.h:* {{declaration of 'x' does not match}}
+#endif
+
+#if defined(FIRST)
+typedef int A;
+struct S4 {
+  A x;
+};
+
+struct S5 {
+  A x;
+};
+#elif defined(SECOND)
+typedef int B;
+struct S4 {
+  B x;
+};
+
+struct S5 {
+  int x;
+};
+#else
+struct S4 s4;
+// expected-error@first.h:* {{'S4' has different definitions in different modules; first difference is definition in module 'FirstModule' found field 'x' with type 'A' (aka 'int')}}
+// expected-note@second.h:* {{but in 'SecondModule' found field 'x' with type 'B' (aka 'int')}}
+
+struct S5 s5;
+// expected-error@first.h:* {{'S5' has different definitions in different modules; first difference is definition in module 'FirstModule' found field 'x' with type 'A' (aka 'int')}}
+// expected-note@second.h:* {{but in 'SecondModule' found field 'x' with type 'int'}}
+#endif
+
+#if defined(FIRST)
+struct S6 {
+  unsigned x;
+};
+#elif defined(SECOND)
+struct S6 {
+  unsigned x : 1;
+};
+#else
+struct S6 s6;
+// expected-error@first.h:* {{'S6' has different definitions in different modules; first difference is definition in module 'FirstModule' found non-bitfield 'x'}}
+// expected-note@second.h:* {{but in 'SecondModule' found bitfield 'x'}}
+#endif
+
+#if defined(FIRST)
+struct S7 {
+  unsigned x : 2;
+};
+#elif defined(SECOND)
+struct S7 {
+  unsigned x : 1;
+};
+#else
+struct S7 s7;
+// expected-error@first.h:* {{'S7' has different definitions in different modules; first difference is definition in module 'FirstModule' found bitfield 'x' with one width expression}}
+// expected-note@second.h:* {{but in 'SecondModule' found bitfield 'x' with different width expression}}
+#endif
+
+#if defined(FIRST)
+struct S8 {
+  unsigned x : 2;
+};
+#elif defined(SECOND)
+struct S8 {
+  unsigned x : 1 + 1;
+};
+#else
+struct S8 s8;
+// expected-error@first.h:* {{'S8' has different definitions in different modules; first difference is definition in module 'FirstModule' found bitfield 'x' with one width expression}}
+// expected-note@second.h:* {{but in 'SecondModule' found bitfield 'x' with different width expression}}
+#endif
+

[PATCH] D71734: [Modules] Handle tag types and complain about bad merges in C/Objective-C mode

[Bruno Cardoso Lopes via Phabricator via cfe-commits]

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






Comment at: clang/lib/Serialization/ASTReader.cpp:9286
+  false /*UseCanonicalDecls*/);
+  (void)Ctx.IsEquivalent(D, Canon);
+}

martong wrote:
> Would it be possible to check the structural non-equivalency with `ODRHash`?
> I wonder if the `ODRHash` of the two Decls are different in this case or not.
> `ASTStructuralEquivalency` and `ODRHash` seems to serve a very similar use 
> case, however, AFAIK in the rest of the ASTReader code `ODRHash` is used 
> exclusively.
> 
> (On a long term, I am thinking about that perhaps `ASTImporter` could use 
> ODRHash instead of ASTStructuralEquivalency.)
Great idea. Updated the patch with another approach!


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: [Modules] Handle tag types and complain about bad merges in C/Objective-C mode

[Gabor Marton via Phabricator via cfe-commits]

martong added inline comments.



Comment at: clang/lib/Serialization/ASTReader.cpp:9286
+  false /*UseCanonicalDecls*/);
+  (void)Ctx.IsEquivalent(D, Canon);
+}

Would it be possible to check the structural non-equivalency with `ODRHash`?
I wonder if the `ODRHash` of the two Decls are different in this case or not.
`ASTStructuralEquivalency` and `ODRHash` seems to serve a very similar use 
case, however, AFAIK in the rest of the ASTReader code `ODRHash` is used 
exclusively.

(On a long term, I am thinking about that perhaps `ASTImporter` could use 
ODRHash instead of ASTStructuralEquivalency.)


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: [Modules] Handle tag types and complain about bad merges in C/Objective-C mode

[Bruno Cardoso Lopes via Phabricator via cfe-commits]

bruno created this revision.
bruno added reviewers: rsmith, arphaman, vsapsai, martong.
Herald added subscribers: cfe-commits, ributzka, dexonsmith, jkorous, rnkovacs.
Herald added a reviewer: jdoerfert.
Herald added a project: clang.

Take `struct Z {...}` defined differently and imported from both modules
X and Y. While in C/Objective-C mode, clang used to pick one of the
definitions and ignore the other even though they are not structurally
equivalent. Hook up a mechanism to check for it and reject such
conflicts. Instead of silently compiling, clang now emits:

In module 'Y' imported from t.m:2:
./y.h:2:8: error: type 'struct Z' has incompatible definitions in different 
translation units
struct Z {

  ^

./y.h:3:10: note: field 'm' has type 'double' here

  double m;
 ^

./x.h:3:7: note: field 'm' has type 'int' here

  int m;
  ^

rdar://problem/56764293


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D71734

Files:
  clang/include/clang/AST/ASTStructuralEquivalence.h
  clang/include/clang/Serialization/ASTReader.h
  clang/lib/AST/ASTStructuralEquivalence.cpp
  clang/lib/Serialization/ASTReader.cpp
  clang/lib/Serialization/ASTReaderDecl.cpp
  clang/test/Modules/Inputs/merge-in-c/tag-types/module.modulemap
  clang/test/Modules/Inputs/merge-in-c/tag-types/x.h
  clang/test/Modules/Inputs/merge-in-c/tag-types/y.h
  clang/test/Modules/merge-tag-types.c

Index: clang/test/Modules/merge-tag-types.c
===
--- /dev/null
+++ clang/test/Modules/merge-tag-types.c
@@ -0,0 +1,14 @@
+// RUN: rm -rf %t
+// RUN: %clang_cc1 -fmodules -fimplicit-module-maps -fmodules-cache-path=%t/cache -x objective-c -I%S/Inputs/merge-in-c/tag-types -verify %s
+
+@import Y;
+@import X;
+
+void foo() {
+  struct Z z;
+  z.m = 2.0;
+}
+
+// expected-error@y.h:2 {{type 'struct Z' has incompatible definitions in different translation units}}
+// expected-note@y.h:3 {{field 'm' has type 'double' here}}
+// expected-note@x.h:3 {{field 'm' has type 'int' here}}
Index: clang/test/Modules/Inputs/merge-in-c/tag-types/y.h
===
--- /dev/null
+++ clang/test/Modules/Inputs/merge-in-c/tag-types/y.h
@@ -0,0 +1,4 @@
+
+struct Z {
+  double m;
+};
Index: clang/test/Modules/Inputs/merge-in-c/tag-types/x.h
===
--- /dev/null
+++ clang/test/Modules/Inputs/merge-in-c/tag-types/x.h
@@ -0,0 +1,4 @@
+
+struct Z {
+  int m;
+};
Index: clang/test/Modules/Inputs/merge-in-c/tag-types/module.modulemap
===
--- /dev/null
+++ clang/test/Modules/Inputs/merge-in-c/tag-types/module.modulemap
@@ -0,0 +1,10 @@
+
+module X {
+  header "x.h"
+  export *
+}
+
+module Y {
+  header "y.h"
+  export *
+}
Index: clang/lib/Serialization/ASTReaderDecl.cpp
===
--- clang/lib/Serialization/ASTReaderDecl.cpp
+++ clang/lib/Serialization/ASTReaderDecl.cpp
@@ -14,6 +14,7 @@
 #include "ASTCommon.h"
 #include "ASTReaderInternals.h"
 #include "clang/AST/ASTContext.h"
+#include "clang/AST/ASTStructuralEquivalence.h"
 #include "clang/AST/Attr.h"
 #include "clang/AST/AttrIterator.h"
 #include "clang/AST/Decl.h"
@@ -723,8 +724,22 @@
 llvm_unreachable("unexpected tag info kind");
   }
 
-  if (!isa(TD))
-mergeRedeclarable(TD, Redecl);
+  if (isa(TD))
+return Redecl;
+
+  mergeRedeclarable(TD, Redecl);
+  // Handle merging in C/Objective-C
+  if (!Reader.getContext().getLangOpts().CPlusPlus) {
+auto *Canon = TD->getCanonicalDecl();
+if (TD == Canon)
+  return Redecl;
+
+// Only check decls loaded from different external sources and
+// delay the actual checking using PendingStructuralMismatches.
+if (TD->getGlobalID() != Canon->getGlobalID())
+  Reader.PendingStructuralMismatches[Canon].push_back(TD);
+  }
+
   return Redecl;
 }
 
Index: clang/lib/Serialization/ASTReader.cpp
===
--- clang/lib/Serialization/ASTReader.cpp
+++ clang/lib/Serialization/ASTReader.cpp
@@ -10,14 +10,14 @@
 //
 //===--===//
 
-#include "clang/Serialization/ASTRecordReader.h"
 #include "ASTCommon.h"
 #include "ASTReaderInternals.h"
-#include "clang/AST/AbstractTypeReader.h"
 #include "clang/AST/ASTConsumer.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/AST/ASTMutationListener.h"
+#include "clang/AST/ASTStructuralEquivalence.h"
 #include "clang/AST/ASTUnresolvedSet.h"
+#include "clang/AST/AbstractTypeReader.h"
 #include "clang/AST/Decl.h"
 #include "clang/AST/DeclBase.h"
 #include "clang/AST/DeclCXX.h"
@@ -30,8 +30,8 @@
 #include "clang/AST/ExprCXX.h"
 #include "clang/AST/ExternalASTSource.h"
 #include "clang/AST/NestedNameSpecifier.h"
-#include "clang/AST/OpenMPClause.h"
 #include "clang/AST/ODRHash.h"