jyknight added inline comments.
================
Comment at: clang/lib/AST/ItaniumMangle.cpp:4010
+ if (Context.getASTContext().getLangOpts().getClangABICompat() >=
+ LangOptions::ClangABI::Ver11) {
+ Out << "u8__uuidof";
----------------
rsmith wrote:
> Should this be `>= Ver12` / `> Ver11` (given that we already released version
> 11 and it didn't do this)?
Oops, indeed, done, and adjusted the test as well to test against ABI 11.
================
Comment at: clang/lib/AST/ItaniumMangle.cpp:4025
+ QualType UuidT = UE->getTypeOperand(Context.getASTContext());
+ Out << 't';
+ mangleType(UuidT);
----------------
rsmith wrote:
> It looks like we've lost the `u8__uuidof` prefix on this side. Did you intend
> to emit that unconditionally above?
Whoops, indeed.
================
Comment at: clang/lib/AST/ItaniumMangle.cpp:4032
+ }
}
break;
----------------
rjmccall wrote:
> 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.
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?
================
Comment at: clang/lib/AST/ItaniumMangle.cpp:4349
+ if (Context.getASTContext().getLangOpts().getClangABICompat() >=
+ LangOptions::ClangABI::Ver11) {
+ Out << "u11__alignof__";
----------------
rsmith wrote:
> Presumably this should be `> Ver11`, as above.
Done.
================
Comment at: clang/lib/AST/ItaniumMangle.cpp:4356
+ mangleExpression(SAE->getArgumentExpr());
+ Out << 'E';
+ }
----------------
rjmccall wrote:
> 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.
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`.
================
Comment at: clang/test/CodeGenCXX/microsoft-uuidof-mangling.cpp:54
+// CHECK: call void @_Z15test_uuidofTypeI10TestStructEvDTu8__uuidofT_EE(
+// CHECK: call void
@_Z15test_uuidofExprI9HasMemberEvDTu8__uuidofXsrT_6memberEEE(
// CHECK: define linkonce_odr void
@_ZN8UUIDTestI10TestStructL_Z42_GUID_eafa1952_66f8_438b_8fba_af1bbae42191EEC1Ev
----------------
rsmith wrote:
> Please consider adding test coverage for `-fclang-abi-compat` for `__uuidof`
> too.
Indeed, that would've caught the bug in my code...done.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D93922/new/
https://reviews.llvm.org/D93922
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
