Page MenuHomePhabricator

Mangle __unaligned in Itanium ABI
ClosedPublic

Authored by rogfer01 on May 22 2017, 2:39 AM.

Details

Summary

__unaligned is not currently mangled in any way in the Itanium ABI. This causes failures when using -fms-extensions and C++ in targets using Itanium ABI.

As suggested by @rsmith the simplest thing to do here is actually mangle the qualifier as a vendor extension.

As an alternative, we could continue not mangling this qualifier. But if I read the code right, in order to get substitutions correctly, we would have to rebuild some types and stripe the __unaligned qualifier prior any mangling and substitution.

This patch also removes the change done in D31976 and updates its test to the new reality.

Diff Detail

Repository
rL LLVM

Event Timeline

rogfer01 created this revision.May 22 2017, 2:39 AM
rsmith added inline comments.May 22 2017, 3:05 PM
lib/AST/ItaniumMangle.cpp
2329–2333 ↗(On Diff #99727)

I don't think this is the right place/way to handle this: given

void f(struct X __unaligned *p, struct X *q) {}

it looks like we'll mangle as _Z1fP1XP1X with this patch, which seems wrong: this should presumably instead be _Z1fP1XS0_.

But regardless, I think the right thing to do is to invent a mangling for __unaligned, since we support overloading on it; the most appropriate mangling would be U11__unaligned, per http://itanium-cxx-abi.github.io/cxx-abi/abi.html#mangling-type. (You'll need to extend mangleQualifiers to emit this and hasMangledSubstitutionQualifiers to ignore it.)

rogfer01 added inline comments.May 23 2017, 12:13 AM
lib/AST/ItaniumMangle.cpp
2329–2333 ↗(On Diff #99727)

Oh. I see. Thanks for the review.

I'll update this patch and then I'll post another one for the mangling of unaligned.

rogfer01 updated this revision to Diff 99876.May 23 2017, 3:41 AM
rogfer01 retitled this revision from Remove __unaligned preventively when mangling types in Itanium ABI to Mangle __unaligned in Itanium ABI.
rogfer01 edited the summary of this revision. (Show Details)
rogfer01 added a reviewer: majnemer.

Abandon the old approach which cannot possibly work and just opt to mangle __unaligned.

aaron.ballman added inline comments.May 23 2017, 5:36 AM
test/CodeGenCXX/pr33080.cpp
19 ↗(On Diff #99876)

Can we also get a test like struct A * __unaligned * __unaligned *? Also, perhaps a test case involving templates would be good.

rogfer01 updated this revision to Diff 100670.May 30 2017, 1:37 AM

Thanks @aaron.ballman for the review.

I have extended the test with your suggestions.

aaron.ballman accepted this revision.May 30 2017, 6:29 AM

Can you run clang-format over both the test files? Aside from that, looks good to me, but you should wait for @rsmith or @majnemer to sign off before committing.

This revision is now accepted and ready to land.May 30 2017, 6:29 AM
rogfer01 updated this revision to Diff 100868.May 31 2017, 8:46 AM

Fix formatting of tests.

Thanks for the review @aaron.ballman !

rnk added a reviewer: rnk.May 31 2017, 12:27 PM
rnk edited edge metadata.
rnk added a subscriber: Prazek.

This was filed as https://bugs.llvm.org/show_bug.cgi?id=33178

Isn't there a space in the mangling for vendor extensions? Can we use that here?

rsmith edited edge metadata.May 31 2017, 12:37 PM
In D33398#769152, @rnk wrote:

Isn't there a space in the mangling for vendor extensions? Can we use that here?

We are using that here: that's what mangleVendorQualifier does.

lib/AST/ItaniumMangle.cpp
2210 ↗(On Diff #100868)

Too much indentation here. Also, the ABI requires the "unordered" vendor qualifiers to be emitted in reverse alphabetical order, so this should be emitted after __weak and __strong but before __autoreleasing.

rogfer01 updated this revision to Diff 100974.Jun 1 2017, 12:56 AM

Changelog:

  • Fix formatting.
  • Emit __unaligned in the right order (as defined by the Itanium ABI) when there are Objective-C++ ARC vendor qualifiers.
  • New test for __unaligned and ARC's __weak, __strong and __autorelease
rogfer01 added inline comments.Jun 1 2017, 12:58 AM
lib/AST/ItaniumMangle.cpp
2210 ↗(On Diff #100868)

I think you meant after __weak but before __strong and __autoreleasing? Maybe I'm misinterpreting something here.

The current patch emits __weak, then __unaligned and then the remaining ARC ones.

rsmith accepted this revision.Jun 1 2017, 1:07 PM

Looks great, thanks!

lib/AST/ItaniumMangle.cpp
2184 ↗(On Diff #100974)

first __weak -> __weak first

2210 ↗(On Diff #100868)

The alphabet is hard, apparently :) Yes, thanks!

This revision was automatically updated to reflect the committed changes.