Page MenuHomePhabricator

Teach clang how to merge typedef over anonymous structs in C mode.
ClosedPublic

Authored by v.g.vassilev on Jun 22 2017, 5:56 AM.

Details

Summary

empty.h makes module A available. textual.h brings the definition of fsid_t and a.h imports again the definition of fsid_t.

In C mode clang fails to merge the textually included definition with the one imported from a module. The C lookup rules fail to find the imported definition because its linkage is internal in non C++ mode.

This patch reinstates some of the ODR merging rules for typedefs of anonymous tags for languages other than C++.

Patch by Raphael Isemann and I.

Diff Detail

Repository
rL LLVM

Event Timeline

v.g.vassilev created this revision.Jun 22 2017, 5:56 AM
rsmith edited edge metadata.Jun 26 2017, 1:42 PM

https://reviews.llvm.org/D31778 is related. We need to have a design for how ODR-like issues in C will be resolved before we'll know what the right fix is here. Prior to D31778 my intention had been to implement C's structural typing ("compatible type") rules and keep multiple definitions of RecordDecls in C as separate entities, but D31778 is suggesting that we use something more ODR-like instead, which is also more in line with the direction that this patch takes.

It's notable that C has no notion of "typedef name for linkage", so this patch does not address all the typedef-related issues that might show up in C code. For example:

typedef struct { ... } *ptr;

in two different modules would not be merged into a single type with this approach (you'd need to actually implement the C compatible type rules to get that to work). But maybe that's OK -- the above also does not (formally) work in C++, and it doesn't seem to cause major problems.

bruno edited edge metadata.Jun 30 2017, 10:00 AM

Hi @v.g.vassilev, sorry for the delay.

Just updated D31778. I agree with Richard's observations, do you think you can extend it to work in the same way as D31778 does? The structural checking is already abstracted there (should be straightforward to use).

bruno accepted this revision.Jun 30 2017, 5:26 PM

Tested it locally, we actually need all these changes on top of D31778.

LGTM

This revision is now accepted and ready to land.Jun 30 2017, 5:26 PM
v.g.vassilev closed this revision.Jul 1 2017, 1:45 PM

Landed in r306964.