This is an archive of the discontinued LLVM Phabricator instance.

Add additional mangling for struct members of non trivial structs
ClosedPublic

Authored by danzimm on Mar 27 2019, 5:46 AM.

Details

Summary

In https://bugs.llvm.org/show_bug.cgi?id=41206 we observe bad codegen when embedding a non-trivial C struct within a C struct. This is due to the fact that name mangling for non-trivial structs marks the two structs as identical. This diff contains a potential fix for this issue. Please comment if there're any untested scenarios I should include.

Test Plan: I've included a unit test & updated other tests

Event Timeline

danzimm created this revision.Mar 27 2019, 5:46 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 27 2019, 5:46 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript

There is no test here.
Tests should probably go into clang/test/CodeGen.

@lebedev.ri right, sorry about that- I prematurely diff'd (got a few terminals crossed and thought I was finished with the test already). I will be amending with a test and a few other test fixes shortly. Sorry about the miscommunication :/

danzimm edited the summary of this revision. (Show Details)Mar 27 2019, 6:05 AM
danzimm updated this revision to Diff 192436.Mar 27 2019, 6:33 AM

I forgot to add the test originally, this update contains a test and updates to old tests to make them pass

danzimm added a subscriber: smeenai.

@smeenai please feel free add any reviewers that I might've missed

This revision is now accepted and ready to land.Mar 27 2019, 10:09 AM

Do you need someone to commit this for you?

clang/test/CodeGenObjC/nontrivial-c-struct-within-struct-name.m
2

I think you can get rid of -fblocks, -fobjc-runtime=ios-11.0, -DUSESTRUCT, and -I %S/Inputs

Also, it might be worth adding a third level of struct to the test, to show that it handles arbitrary nesting correctly (which it does).

danzimm updated this revision to Diff 192652.Mar 28 2019, 9:05 AM

Add a third level to ensure nontrivial structs within structs within structs works (this suggests that N embeddings works too). Also change the invocation of the test

@smeenai good idea on the third level!

Yep, I'll need somebody to commit this for me, thanks!

Looks good. I'll commit this.

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMar 28 2019, 10:02 AM