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

[John McCall via Phabricator via cfe-commits]

rjmccall accepted this revision.
rjmccall added inline comments.
This revision is now accepted and ready to land.


================
Comment at: clang/lib/AST/ItaniumMangle.cpp:5576
+      "program to refer to the anonymous union, and there is therefore no need "
+      "to mangle its name. '");
+}
----------------
erichkeane wrote:
> rjmccall wrote:
> > erichkeane wrote:
> > > rjmccall wrote:
> > > > 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.
> > > 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?  
> > 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)>();
> > ```
> 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.
Hmm, alright, works for me.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D122820/new/

https://reviews.llvm.org/D122820