Page MenuHomePhabricator

Itanium Mangling: Mangle `__alignof__` differently than `alignof`.
ClosedPublic

Authored by jyknight on Dec 29 2020, 2:29 PM.

Details

Summary

The two operations have acted differently since Clang 8, but were
unfortunately mangled the same. The new mangling uses new "vendor
extended expression" syntax proposed in
https://github.com/itanium-cxx-abi/cxx-abi/issues/112

GCC had the same mangling problem, https://gcc.gnu.org/PR88115, and
will hopefully be switching to the same mangling as implemented here.

Additionally, fix the mangling of __uuidof to use the new extension
syntax, instead of its previous nonstandard special-case.

Adjusts the demangler accordingly.

Diff Detail

Event Timeline

jyknight requested review of this revision.Dec 29 2020, 2:29 PM
jyknight created this revision.
Herald added projects: Restricted Project, Restricted Project, Restricted Project. · View Herald TranscriptDec 29 2020, 2:29 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
ldionne added inline comments.
clang/lib/AST/ItaniumMangle.cpp
4014

Please forgive my ignorance, but when left unspecified, what does the Clang ABI compatibility flag default to? I'm sure you've thought about it already, but I'm trying to understand whether that is an ABI break for people compiling without specifying the Clang ABI compatibility version (which is most users).

jyknight added inline comments.Jan 14 2021, 9:17 AM
clang/lib/AST/ItaniumMangle.cpp
4014

It defaults to the latest version of clang (usually; some vendors pin it to an old version). Yes, it is theoretically an ABI break -- but one which is extremely unlikely to cause trouble in any real code.

This is in line with other similar C++ ABI breaks we make in Clang.

ldionne accepted this revision as: Restricted Project.Jan 14 2021, 12:12 PM
ldionne added inline comments.
clang/lib/AST/ItaniumMangle.cpp
4014

Thanks for the information.

Yeah, I guess this is only an ABI break when a signature contains an expression that uses alignof. I think this is fine.

Was the "group" libc++abi accept intended to be an accept for the whole revision? Or should still ping for review from rsmith or someone else?

rsmith added inline comments.Jan 20 2021, 11:47 AM
clang/lib/AST/ItaniumMangle.cpp
4015

Should this be >= Ver12 / > Ver11 (given that we already released version 11 and it didn't do this)?

4025

It looks like we've lost the u8__uuidof prefix on this side. Did you intend to emit that unconditionally above?

4349

Presumably this should be > Ver11, as above.

clang/test/CodeGenCXX/microsoft-uuidof-mangling.cpp
59

Please consider adding test coverage for -fclang-abi-compat for __uuidof too.

rjmccall added inline comments.
clang/lib/AST/ItaniumMangle.cpp
4032

The old case doesn't seem to be adding the u8__uuidof prefix anymore.

Your patch description does not say anything about changing the mangling of __uuidof, and that should be treated separately, not just lumped in to try to make manglings more consistent.

4356

This isn't a correct mangling of template-arg for expressions; simple expressions should not be surrounded by X...E. Please call mangleTemplateArg.

Although... mangleTemplateArg doesn't look like it consistently does the right thing when you pass it a TemplateArgument constructed with an arbitrary expression, because it seems to be relying on certain expressions having been folded to their canonical representations during template argument checking. So really we need to have some code that mangles an expression as if it were a template argument, which is to say, recognizing simple expressions that fit into expr-primary.

This would break ABI for any existing place that calls mangleTemplateArg with an arbitrary expression, but it looks like the only place that does that is dependent matrix types, where I think wee can justify the break as a bug fix since matrix types are such a new feature. Pinging Florian.

Was the "group" libc++abi accept intended to be an accept for the whole revision? Or should still ping for review from rsmith or someone else?

Sorry, I should have been clearer. I just wanted to get libc++abi out of your way, but I still think it would be good for a more Clang person to review the patch.

jyknight updated this revision to Diff 318277.Jan 21 2021, 12:00 PM
jyknight marked 8 inline comments as done.

Address comments.

clang/lib/AST/ItaniumMangle.cpp
4015

Oops, indeed, done, and adjusted the test as well to test against ABI 11.

4025

Whoops, indeed.

4032

The original description didn't, but I updated it a while ago. ("Additionally, fix the mangling of __uuidof to use the new extension syntax, instead of its previous nonstandard special-case.")

Not sure if your comment predates that or is asking for something more?

4349

Done.

4356

Yep, made the first part of the change (although, annoyingly, using a const_cast -- TemplateArg's constructor requires a non-const Expr*, and that doesn't look easy to change, since it exposes it via a getter, and other callers require a non-const Expr* result from that getter).

I'll work on fixing mangleTemplateArg separately.

I'll note that non-instantiation-dependent __alignof__ gets mangled as a integer literal already, even when in a dependent expression. So I think it's not possible to get this bug to exhibit for alignof -- I don't see how you could end up with something that should mangle as expr-primary there.

For __uuidof, it can only be applied to expressions of struct type, so, most of the things that fit in expr-primary would be an error there. However, L _Z encoding E is possible here, e.g. template <class T> void test_uuidofExpr(decltype(T{}, __uuidof(HasMember::member)) arg) {} gets mangled with my patch as _Z15test_uuidofExprI11OtherStructEvDTcmtlT_Eu8__uuidofXL_ZN9HasMember6memberEEEEE when it should've omitted the X/E on the arg to uuidof, as: _Z15test_uuidofExprI11OtherStructEvDTcmtlT_Eu8__uuidofL_ZN9HasMember6memberEEEE.

clang/test/CodeGenCXX/microsoft-uuidof-mangling.cpp
59

Indeed, that would've caught the bug in my code...done.

jyknight updated this revision to Diff 319422.Jan 26 2021, 3:47 PM
jyknight retitled this revision from Mangle `__alignof__` differently than `alignof`. to Itanium Mangling: Mangle `__alignof__` differently than `alignof`..

Minor updates.

OK, I've posted two follow-up changes now.

https://reviews.llvm.org/D95487 fixes the issue with mangleTemplateArg given expressions -- this turned out to be a larged-scoped problem than I had thought, affecting manglings other than just that used the matrix extension.
https://reviews.llvm.org/D95488 fixes the mangling for enable_if, which had the same problem of hardcoding X/E.

rjmccall accepted this revision.Jan 27 2021, 1:24 AM

This change looks good to me given the follow-up commits.

This revision is now accepted and ready to land.Jan 27 2021, 1:24 AM
This revision was landed with ongoing or failed builds.Jan 27 2021, 1:52 PM
This revision was automatically updated to reflect the committed changes.