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

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

rsmith added a comment.

In D106994#2911903 <https://reviews.llvm.org/D106994#2911903>, @vsapsai wrote:

> In D106994#2911508 <https://reviews.llvm.org/D106994#2911508>, @rsmith wrote:
>
>> For what it's worth, I think the right way to handle this in C is to properly implement the "compatible types" rule instead of trying to invent something ODR-ish: in C, struct definitions in different translation units are different types, but if they're structurally equivalent then they're compatible and you can implicitly pass an object of some type to a function accepting a compatible type. This would mean that we could have multiple, different types with the same name, and we'd need name lookup to deduplicate compatible types, but we wouldn't need to do any cross-module ODR-like struct merging.
>
> I agree that implementing the "compatible types" looks better as it models the language more faithfully. And in the long run we might need to do that anyway. Is there any work done for "compatible types" already? Or I can start by creating a new type for a new definition with the same name and see how it breaks the lookup?
>
> From pragmatic perspective we are pretty invested into this ODR-ish approach and it's not clear how much work switching to "compatible types" would take. So I'd like to continue with the definition merging and evaluate the effort for "compatible types". That's why I'm curious what work is done already.

I don't think there's been any real work done on a cross-TU implementation of compatible types. I also don't want that idea to get in the way of this patch, which seems like a clear improvement following our current approach.

>> But assuming we want to keep the current ODR-in-C approach, this looks OK. There might be some places that assume the lexical and semantic `DeclContext` for a C `FieldDecl` are the same (etc) but I don't think there's a good way to find such things other than by testing this patch broadly.
>
> Are there any known signs for mixing lexical and semantic `DeclContext`? I plan to test the change on our internal codebase, hopefully it'll help to catch any remaining issues.

The kinds of things I saw go wrong when we were bringing this up on the C++ side were generally in code that would walk the list of (say) fields of a record building up some information, and then attempt to look up a given `FieldDecl*` in that data structure. That can fail if fields get merged, because the lookup key may be a different redeclaration of the same field than the one found by walking the class's members.  The fix is usually to add `getCanonicalDecl` calls in the right places. The sign of this kind of bug happening was usually a crash or assert, usually pretty close to where the problem was.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106994