[GH54588]Fix ItaniumMangler for NTTP unnamed unions w/ unnamed structs

[erichkeane]

Summary

As reported in https://github.com/llvm/llvm-project/issues/54588
and discussed in https://github.com/itanium-cxx-abi/cxx-abi/issues/139

We are supposed to do a DFS, pre-order, decl-order search for a name for
the union in this case. Prevoiusly we crashed because the IdentiferInfo
pointer was nullptr, so this makes sure we have a name in the cases
described by the ABI.

I added an llvm-unreachable to cover an unexpected case at the end of
the new function with information/reference to the ABI in case we come
up with some way to get back to here.

[rjmccall]

Unfortunately, this initialization case does allow a situation where

clang/lib/AST/ItaniumMangle.cpp

5561

Alas, bit-fields are also allowed to be anonymous.

5576

Unfortunately, I think class value mangling has a counter-example to this: you can have an anonymous struct or union which doesn't declare any named members but will of course nonetheless show up in the list of members in the enclosing class. Code can even give it a non-zero initializer using list-initialization; that value can never be read, but it's there, and presumably it's part of uniquely determining a different template argument value and therefore needs to be mangled.

I don't know what we should do about this in the ABI, but we shouldn't crash in the compiler.

[erichkeane]

clang/lib/AST/ItaniumMangle.cpp

5561

Ah, good point, i'll update the comment and add a test.

5576

I'm not really understanding what you mean? Can you provide an example? All of the cases I could come up with like this ended up getting caught by the 'zero -init' case.

That said, I considered this 'unreachable' to be a distinct improvement over our current behavior which is to crash at effectively line 5558.

Would you prefer some sort of 'fatal-error' emit here?

[erichkeane]

Handle the unnamed bitfield case.

I'm still not sure how to reproduce the 'unreachable' example I added that @rjmccall suggested was possible, nor how we can deal with that, so waiting on his feedback for how he thinks we should handle that case.

Presumably I could replace it with a 'don't know how to handle that yet' fatal error like we do in the MS Mangler?

[rjmccall]

clang/lib/AST/ItaniumMangle.cpp

5576

I think the test case would be something like:

struct A {
  union { unsigned: 10; };
  int x;
  constexpr A(int x) : x(x) {}
};

template <A a> void foo() {}
template void foo<A(5)>();

But also consider:

struct B {
  struct Nested {
    union { unsigned: 10; };
    int x;
  } nested;
  constexpr B(int x) : nested{5, x} {}
};

template <B b> void bar() {}
template void bar<B(10)>();

[erichkeane]

Update to 'diag' instead of crash, as implied/requested by @rjmccall

clang/lib/AST/ItaniumMangle.cpp

5576

With my current patch, that first example mangles to:
@_Z3fooIXtl1ALNS0_Ut_EELi5EEEEvv (that is, it doesn't touch this crash). Unfortunately, that doesn't 'demangle' with llvm-cxxfilt.

For the 2nd example, i had to modify it to be:

struct B {
  struct Nested {
    union {
      unsigned : 10;
    };
    int x;
  } nested;
  constexpr B(int x) : nested{{}, x} {}
};
  
  
template <B b> void bar() {}
template void bar<B(10)>();

As you wrote it, we get the error:
temp.cpp:20:31: error: initializer for aggregate with no elements requires explicit braces

I tried putting a '5' in those brackets, and get:
temp.cpp:20:32: error: excess elements in union initializer

HOWEVER, I think it makes sense to have a Diag here rather than an llvm-unreachable, so I've done that instead.

[rjmccall]

clang/lib/AST/ItaniumMangle.cpp

5576

Hmm, alright, works for me.

[thakis]

Looks like this breaks tests on macOS: http://45.33.8.238/macm1/31733/step_7.txt

Please take a look, and revert for now if it takes a while to fix.

[thakis]

Reverted in 50186b63d1807d389f31c515377d94185795ab44 for now.

[erichkeane]

In D122820#3423712, @thakis wrote:

Looks like this breaks tests on macOS: http://45.33.8.238/macm1/31733/step_7.txt

Please take a look, and revert for now if it takes a while to fix.

That is sad, it seems that llvm-cxxfilt isn't working right on some of the machines :/

[thakis]

In D122820#3426132, @erichkeane wrote:

In D122820#3423712, @thakis wrote:

Looks like this breaks tests on macOS: http://45.33.8.238/macm1/31733/step_7.txt

Please take a look, and revert for now if it takes a while to fix.

That is sad, it seems that llvm-cxxfilt isn't working right on some of the machines :/

Oh hm, if it's due to the wrong llvm-cxxfilt being called, maybe we just need to update clang/test/lit.cfg.py to add llvm-cxxfilt to tools to make sure it uses the just-built one instead of the one on PATH (? not sure).

[thakis]

Oh hm, if it's due to the wrong llvm-cxxfilt being called, maybe we just need to update clang/test/lit.cfg.py to add llvm-cxxfilt to tools to make sure it uses the just-built one instead of the one on PATH (? not sure).

Actually, I consistently saw this failing on all our mac bots, and as far as I can tell they _don't_ have llvm-cxxfilt on PATH. Maybe it's due to macOS prepending a _ by default and llvm-cxxfilt insisting on host underscoriness?

% out/gn/bin/llvm-cxxfilt _Z4funcPci
_Z4funcPci
% out/gn/bin/llvm-cxxfilt __Z4funcPcia
func(char*, int, signed char)

Yes, looks like it. Since you're always passing a linux triple (which doesn't add the extra underscore), I think it'll work if you pass -n to llvm-cxxfilt to force that mode:

% out/gn/bin/llvm-cxxfilt -n _Z4funcPcia
func(char*, int, signed char)

[erichkeane]

In D122820#3426211, @thakis wrote:

Oh hm, if it's due to the wrong llvm-cxxfilt being called, maybe we just need to update clang/test/lit.cfg.py to add llvm-cxxfilt to tools to make sure it uses the just-built one instead of the one on PATH (? not sure).

Actually, I consistently saw this failing on all our mac bots, and as far as I can tell they _don't_ have llvm-cxxfilt on PATH. Maybe it's due to macOS prepending a _ by default and llvm-cxxfilt insisting on host underscoriness?

% out/gn/bin/llvm-cxxfilt _Z4funcPci
_Z4funcPci
% out/gn/bin/llvm-cxxfilt __Z4funcPcia
func(char*, int, signed char)

Yes, looks like it. Since you're always passing a linux triple (which doesn't add the extra underscore), I think it'll work if you pass -n to llvm-cxxfilt to force that mode:

% out/gn/bin/llvm-cxxfilt -n _Z4funcPcia
func(char*, int, signed char)

Ah, great, thank you for looking into that! I already re-committed with the test line disabled and a FIXME(since we're testing the 'mangled' names already!), but I'll try a commit with this -n to re-enable the line to see if this fixes it (and be quick about reverting it myself if I see the errors come across.

Thank you again for your help!

clang/lib/AST/ItaniumMangle.cpp

else if (const FieldDecl *FD =
       T = FD->getType();
     else
       T = Ctx.getRecordType(
           cast<CXXRecordDecl>(E.getAsBaseOrMember().getPointer()));
   }
   return T;
 }
 
+static IdentifierInfo *getUnionInitName(SourceLocation UnionLoc,
+                                        DiagnosticsEngine &Diags,
+                                        const FieldDecl *FD) {
+  // According to:
+  // http://itanium-cxx-abi.github.io/cxx-abi/abi.html#mangling.anonymous
+  // For the purposes of mangling, the name of an anonymous union is considered
+  // to be the name of the first named data member found by a pre-order,
+  // depth-first, declaration-order walk of the data members of the anonymous
+  // union.
+
+  if (FD->getIdentifier())
+    return FD->getIdentifier();
+
+  // The only cases where the identifer of a FieldDecl would be blank is if the
+  // field represents an anonymous record type or if it is an unnamed bitfield.
+  // There is no type to descend into in the case of a bitfield, so we can just
+  // return nullptr in that case.
+  if (FD->isBitField())
+    return nullptr;
+  const CXXRecordDecl *RD = FD->getType()->getAsCXXRecordDecl();
+
+  // Consider only the fields in declaration order, searched depth-first.  We
+  // don't care about the active member of the union, as all we are doing is
+  // looking for a valid name. We also don't check bases, due to guidance from
+  // the Itanium ABI folks.
+  for (const FieldDecl *RDField : RD->fields()) {
+    if (IdentifierInfo *II = getUnionInitName(UnionLoc, Diags, RDField))
+      return II;
+  }
+
+  // According to the Itanium ABI: If there is no such data member (i.e., if all
+  // of the data members in the union are unnamed), then there is no way for a
+  // program to refer to the anonymous union, and there is therefore no need to
+  // mangle its name. However, we should diagnose this anyway.
+  unsigned DiagID = Diags.getCustomDiagID(
+      DiagnosticsEngine::Error, "cannot mangle this unnamed union NTTP yet");
+  Diags.Report(UnionLoc, DiagID);
+
+  return nullptr;
+}
+
 void CXXNameMangler::mangleValueInTemplateArg(QualType T, const APValue &V,
                                               bool TopLevel,
                                               bool NeedExactType) {
   // Ignore all top-level cv-qualifiers, to match GCC.
   Qualifiers Quals;
   T = getASTContext().getUnqualifiedArrayType(T, Quals);
 
   // A top-level expression that's not a primary expression is wrapped in X...E.
case APValue::Union: {
     }
 
     // <braced-expression> ::= di <field source-name> <braced-expression>
     NotPrimaryExpr();
     Out << "tl";
     mangleType(T);
     if (!isZeroInitialized(T, V)) {
       Out << "di";
-      mangleSourceName(FD->getIdentifier());
+      IdentifierInfo *II = (getUnionInitName(
+          T->getAsCXXRecordDecl()->getLocation(), Context.getDiags(), FD));
+      if (II)
+        mangleSourceName(II);
       mangleValueInTemplateArg(FD->getType(), V.getUnionValue(), false);
     }
     Out << 'E';
     break;
   }
 
   case APValue::Array: {
     QualType ElemT(T->getArrayElementTypeNoTypeQual(), 0);

clang/test/CodeGenCXX/mangle-nttp-anon-union.cpp

+// RUN: %clang_cc1 -std=c++20 -emit-llvm %s -o - -triple=x86_64-linux-gnu | FileCheck %s
+// RUN: %clang_cc1 -std=c++20 -emit-llvm %s -o - -triple=x86_64-linux-gnu | llvm-cxxfilt | FileCheck %s --check-prefix DEMANGLED
+
+template<typename T>
+struct wrapper1 {
+  union {
+    struct {
+      T RightName;
+    };
+  };
+};
+
+template<typename T>
+struct wrapper2 {
+  union {
+    struct {
+      T RightName;
+    };
+    T WrongName;
+  };
+};
+
+struct Base {
+  int WrongName;
+};
+
+template <typename T>
+struct wrapper3 {
+  union {
+    struct : Base {
+      T RightName; };
+    T WrongName;
+  };
+};
+
+template <typename T>
+struct wrapper4 {
+  union {
+    int RightName;
+    struct {
+      T WrongName;
+    };
+    T AlsoWrongName;
+  };
+};
+
+template <typename T>
+struct wrapper5 {
+  union {
+    struct {
+      struct {
+        T RightName;
+      };
+      T WrongName;
+    };
+  };
+};
+
+template<typename T>
+struct wrapper6 {
+  union {
+    union{
+    int : 5;
+    T RightName;
+    };
+  };
+};
+
+
+
+template<auto tparam> void dummy(){}
+
+
+void uses() {
+  // Zero init'ed cases.
+  dummy<wrapper1<int>{}>();
+  // CHECK: call void @_Z5dummyIXtl8wrapper1IiEEEEvv
+  // DEMANGLED: call void @void dummy<wrapper1<int>{}>()()
+  dummy<wrapper2<float>{}>();
+  // CHECK: call void @_Z5dummyIXtl8wrapper2IfEEEEvv
+  // DEMANGLED: call void @void dummy<wrapper2<float>{}>()()
+  dummy<wrapper3<short>{}>();
+  // CHECK: call void @_Z5dummyIXtl8wrapper3IsEEEEvv
+  // DEMANGLED: call void @void dummy<wrapper3<short>{}>()()
+  dummy<wrapper4<double>{}>();
+  // CHECK: call void @_Z5dummyIXtl8wrapper4IdEEEEvv
+  // DEMANGLED: call void @void dummy<wrapper4<double>{}>()()
+  dummy<wrapper5<long long>{}>();
+  // CHECK: call void @_Z5dummyIXtl8wrapper5IxEEEEvv
+  // DEMANGLED: call void @void dummy<wrapper5<long long>{}>()()
+  dummy<wrapper6<int>{}>();
+  // CHECK: call void @_Z5dummyIXtl8wrapper6IiEEEEvv
+  // DEMANGLED: call void @void dummy<wrapper6<int>{}>()()
+
+  dummy<wrapper1<double>{123.0}>();
+  // CHECK: call void @_Z5dummyIXtl8wrapper1IdEtlNS1_Ut_Edi9RightNametlNS2_Ut_ELd405ec00000000000EEEEEEvv
+  // DEMANGLED: call void @void dummy<wrapper1<double>{wrapper1<double>::'unnamed'{.RightName = wrapper1<double>::'unnamed'::'unnamed'{0x1.ecp+6}}}>()()
+  dummy<wrapper2<double>{123.0}>();
+  // CHECK: call void @_Z5dummyIXtl8wrapper2IdEtlNS1_Ut_Edi9RightNametlNS2_Ut_ELd405ec00000000000EEEEEEvv
+  // DEMANGLED: call void @void dummy<wrapper2<double>{wrapper2<double>::'unnamed'{.RightName = wrapper2<double>::'unnamed'::'unnamed'{0x1.ecp+6}}}>()()
+  dummy<wrapper3<double>{123, 456}>();
+  // CHECK: call void @_Z5dummyIXtl8wrapper3IdEtlNS1_Ut_Edi9RightNametlNS2_Ut_Etl4BaseLi123EELd407c800000000000EEEEEEvv
+  // DEMANGLED: call void @void dummy<wrapper3<double>{wrapper3<double>::'unnamed'{.RightName = wrapper3<double>::'unnamed'::'unnamed'{Base{123}, 0x1.c8p+8}}}>()()
+  dummy<wrapper4<double>{123}>();
+  // CHECK: call void @_Z5dummyIXtl8wrapper4IdEtlNS1_Ut_Edi9RightNameLi123EEEEEvv
+  // DEMANGLED: call void @void dummy<wrapper4<double>{wrapper4<double>::'unnamed'{.RightName = 123}}>()()
+  dummy<wrapper5<double>{123.0, 456.0}>();
+  // CHECK: call void @_Z5dummyIXtl8wrapper5IdEtlNS1_Ut_Edi9RightNametlNS2_Ut_EtlNS3_Ut_ELd405ec00000000000EELd407c800000000000EEEEEEvv
+  // DEMANGLED: call void @void dummy<wrapper5<double>{wrapper5<double>::'unnamed'{.RightName = wrapper5<double>::'unnamed'::'unnamed'{wrapper5<double>::'unnamed'::'unnamed'::'unnamed'{0x1.ecp+6}, 0x1.c8p+8}}}>()()
+  dummy<wrapper6<double>{1}>();
+  // CHECK: call void @_Z5dummyIXtl8wrapper6IdEtlNS1_Ut_Edi9RightNametlNS2_Ut_Edi9RightNameLd3ff0000000000000EEEEEEvv
+  // DEMANGELD: call void @void dummy<wrapper6<double>{wrapper6<double>::'unnamed'{.RightName = wrapper6<double>::'unnamed'::'unnamed'{.RightName = 0x1p+0}}}>()()   
+}