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