[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<CXXRecordDecl>(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