[PATCH] D106994: [modules] Fix miscompilation when using two RecordDecl definitions with the same name.

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

rsmith added inline comments.


================
Comment at: clang/lib/Serialization/ASTReaderDecl.cpp:832
+      RD->setCompleteDefinition(false);
+      Reader.mergeDefinitionVisibility(OldDef, RD);
+    } else {
----------------
vsapsai wrote:
> rsmith wrote:
> > vsapsai wrote:
> > > Here is the perfect place to compare if `RD` and `OldDef` are equivalent and emit diagnostic if they are not. I've tried `StructuralEquivalenceContext` for this purpose but it compares canonical decls which doesn't work in this case. I think the best approach for this task would be ODR hash comparison proposed in https://reviews.llvm.org/D71734 It will need some tweaks to work with the current patch but overall the plan is to use ODR hash instead of any other decl comparison.
> > Just a minor note: it's not safe to emit diagnostics from here in general; in particular, emitting a diagnostic that refers to a declaration can trigger deserialization, which can reenter the AST reader in unfortunate ways and crash. But we can make a note to do the structural equivalence check here and then actually perform the check when we finish deserialization (with the other merging checks).
> Thanks for pointing it out, I didn't realize diagnostic can trigger deserialization. Was planning to do something like
> 
> ```lang=c++
>     if (OldDef->getODRHash() != RD->getODRHash())
>       Reader.PendingRecordOdrMergeFailures[OldDef].push_back(RD);
> ```
That seems reasonable to me.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106994