This is an archive of the discontinued LLVM Phabricator instance.

[ItaniumMangle] Undeduced auto type doesn't belong in the substitution table
ClosedPublic

Authored by erik.pilkington on Apr 9 2018, 12:22 PM.

Details

Summary

Since "Da" and "Dc" (auto and decltype(auto)) are under <builtin-type> in the mangling grammer, they shouldn't have a substitution (https://itanium-cxx-abi.github.io/cxx-abi/abi.html#mangling-compression). I believe that a deduced AutoType shouldn't be able to reach mangleType(AutoType) because it's deduced type would have been canonicalized in mangleType(QualType) before, so I removed that branch and added an assert. FWIW, with this patch applied Clang and GCC agree on the manglings in the affected file.

Thanks for taking a look!
Erik

Diff Detail

Event Timeline

rjmccall added inline comments.Apr 13 2018, 12:11 AM
clang/lib/AST/ItaniumMangle.cpp
2343

I agree with your analysis that this shouldn't be a substitution candidate. However, I think this probably needs an ABI-compatibility guard.

erik.pilkington marked an inline comment as done.

Add an ABI compatibility guard. Thanks!

rjmccall added inline comments.Apr 21 2018, 5:55 AM
clang/lib/AST/ItaniumMangle.cpp
2343

You should probably add a comment like "Prior to LLVM v6, Clang accidentally treated deduced auto types as substitution candidates."

2343

Er, not "prior to", I guess. "through", maybe, or "up to and including".

Add a comment.

clang/lib/AST/ItaniumMangle.cpp
2343

Sure, I added that comment into the new patch. You meant to say "undeduced auto types", right? Deduced auto types should have been desugared to their underlying type before this function get called in mangleType().

rjmccall added inline comments.Apr 23 2018, 11:06 AM
clang/lib/AST/ItaniumMangle.cpp
2343

Yes, right, thanks. LGTM.

This revision was not accepted when it landed; it landed in state Needs Review.Apr 27 2018, 7:45 PM
This revision was not accepted when it landed; it landed in state Needs Review.
This revision was automatically updated to reflect the committed changes.
This revision was automatically updated to reflect the committed changes.