[PATCH] D93922: Mangle `__alignof__` differently than `alignof`.

James Y Knight via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Thu Jan 21 12:00:13 PST 2021


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



More information about the cfe-commits mailing list