Mangling of anonymous unions

[eastig]

Summary

Anonymous union issues:

A name in the following code is mangled incorrectly:

inline int f1 () {
  static union {
    int a;
    long int b;
  };

  static union {
    int c;
    double d;
  };

  return a+c;
}

int f2 (int a) {
  return a+f1();
}

The name of the second union is mangled as _ZZ2f1vE1c_0 instead of _ZZ2f1vE1c. According to the Itanium C++ ABI '5.1.6 Scope Encoding':
The discriminator is used only for the second and later occurrences of the same "top-level" name within a single function (since "unnamed types" are distinctly numbered, they never include a discriminator).
In the program above 'int c' is the first occurrence of the name 'c'. So no discriminator must be used.

[rnk]

I don't think we should change our behavior w.r.t. static locals of non-inline functions. This behavior should stay the same:

void f() {
  if (0) {
    static int a = 0;
  }
  static int a = 0; // _ZZ1fvE1a
}

inline void f() {
  if (0) {
    static int a = 0;
  }
  static int a = 0; // _ZZ1fvE1a_0
}

The reason is that in the non-inline case, we don't want to waste time during Sema building up tables on the ASTContext. The name is not ABI relevant, and either name is as good as the next.

I agree with your assessment of the static local union case, though. It's not clear to me how your fix works, but maybe -ast-dump isn't complete enough for me to see the IndirectFieldDecl that you're finding.

[eastig]

In D6295#3, @rnk wrote:

I don't think we should change our behavior w.r.t. static locals of non-inline functions. This behavior should stay the same:

void f() {
  if (0) {
    static int a = 0;
  }
  static int a = 0; // _ZZ1fvE1a
}

inline void f() {
  if (0) {
    static int a = 0;
  }
  static int a = 0; // _ZZ1fvE1a_0
}

The reason is that in the non-inline case, we don't want to waste time during Sema building up tables on the ASTContext. The name is not ABI relevant, and either name is as good as the next.

I agree with your assessment of the static local union case, though. It's not clear to me how your fix works, but maybe -ast-dump isn't complete enough for me to see the IndirectFieldDecl that you're finding.

You mean that this will hurt performance, don't you?

The code that takes into account declarations in dead code was added in revision 185986 by an engineer from Apple as a fix of the issue rdar://problem/14204721. I do not have access to Apple's bug tracking system to see what was the issue.

The current issue is that different enumeration schemas are used by getNextDiscriminator. And they produce different names depending on the function specifier 'inline'.
For example:

f.h

inline int f(int v) {
  static union {
    int a;
    int b;
  };

  static union {
    union {
      union {
        int c;                                                                                                                                                                                                                                
      };
      float d;
    };
  };

  int r = a+c;

  a = v;
  c = v+1;

  return r;
}

m_a.cpp

#include "f.h"                                                                                                                                                                                                                                

int f_a(int a) {
  return a+f(a);
}

m_b.cpp

#include "f.h"                                                                                                                                                                                                                                

int f_b(int b) {
  return b+f(b);
}

main.cpp

extern int f_a(int);
extern int f_b(int);

int main(int argc, const char* argv[]) {
  int r1 = f_a(argc);
  int r2 = f_b(argc+1);
  return r1 + r2;                                                                                                                                                                                                                             
}

According to the C++ standard [dcl.fct.spec]: A static local variable in an extern inline function always refers to the same object.
This works when everything is compile by one compiler but it does not work when m_a.cpp is compiled with clang and other files are compiled with g++:

$ /tools/collections/clang/3.5.0/3/linux_3.2-ubuntu_12.04-x86_64/bin/clang++ -c -O3 m_a.cpp
$ /tools/collections/clang/3.5.0/3/linux_3.2-ubuntu_12.04-x86_64/bin/clang++ -c -O3 m_b.cpp
$ /tools/collections/clang/3.5.0/3/linux_3.2-ubuntu_12.04-x86_64/bin/clang++ -O3 main.cpp m_a.o m_b.o
$ ./a.out 
$ echo $?
6

$ g++ -c -O3 m_a.cpp
$ g++ -c -O3 m_b.cpp
$ g++ -O3 main.cpp m_b.o m_a.o
$ ./a.out 
$ echo $?
6

$ /tools/collections/clang/3.5.0/3/linux_3.2-ubuntu_12.04-x86_64/bin/clang++ -c -O3 m_a.cpp
$ g++ -O3 main.cpp m_b.o m_a.o
$ ./a.out 
$ echo $?
4

[rnk]

In D6295#5, @eastig wrote:

You mean that this will hurt performance, don't you?

Yes, there is an (admittedly minor) performance cost to numbering static locals of non-inline functions when no numbering is required.

The code that takes into account declarations in dead code was added in revision 185986 by an engineer from Apple as a fix of the issue rdar://problem/14204721. I do not have access to Apple's bug tracking system to see what was the issue.

Yep, I'm familiar with the work Eli did here because I helped make the Microsoft ABI version of this work.

The current issue is that different enumeration schemas are used by getNextDiscriminator. And they produce different names depending on the function specifier 'inline'.

They do. This is by design. Can you elaborate on why this is a problem? The names of static locals in non-inline functions are not ABI-relevant.

According to the C++ standard [dcl.fct.spec]: A static local variable in an extern inline function always refers to the same object.

Note that the standard is explicitly talking about an "extern inline function" here.

This works when everything is compile by one compiler but it does not work when m_a.cpp is compiled with clang and other files are compiled with g++:

Yes, the example you provided is the static local anonymous union case, which I agree needs to be fixed. Can you reupload a new patch which is limited to handling static local anonymous unions in inline functions, and avoid changes to non-ABI relevant names?

[eastig]

Yes, the example you provided is the static local anonymous union case, which I agree needs to be fixed. Can you reupload a new patch which is limited to handling static local anonymous unions in inline functions, and avoid changes to non-ABI relevant names?

Yes. I have implemented such fix. I am testing it.

[eastig]

Updated patch according to comments

[eastig]

Ping.

[rsmith]

lib/AST/ItaniumMangle.cpp

3913–3925 ↗

(On Diff #16563)

Why do you need to do this? Since this code is only reached for non-externally-visible entities, a single counter for the context (like we used to use) seems like it should be sufficient.

[rsmith]

lib/AST/ItaniumCXXABI.cpp

65–67 ↗

(On Diff #16636)

How is it possible to get here with a VarDecl with no identifier, but where it's not an anonymous union? Please either add a comment explaining or change this to an assert/castAs.

[eastig]

Cleaned up code:

• no need of 'if' checks as VarDecl without an identifier should be an anonymous union declaration.
• created function 'findAnonymousUnionVarDeclName'

[eastig]

This comment was removed by eastig.

lib/AST/ItaniumMangle.cpp

3913–3925 ↗

(On Diff #16563)

You are right. Reverted to the old code.

[eastig]

Ping

[rsmith]

LGTM

lib/AST/ItaniumCXXABI.cpp

50 ↗

(On Diff #16676)

Can this happen?

cfe/trunk/include/clang/AST/Decl.h

public:
   /// commandline option.
   bool isMsStruct(const ASTContext &C) const;
 
   /// \brief Whether we are allowed to insert extra padding between fields.
   /// These padding are added to help AddressSanitizer detect
   /// intra-object-overflow bugs.
   bool mayInsertExtraPadding(bool EmitRemark = false) const;
 
+  /// Finds the first data member which has a name.
+  /// nullptr is returned if no named data member exists.
+  const FieldDecl *findFirstNamedDataMember() const;  
+
 private:
   /// \brief Deserialize just the fields.
   void LoadFieldsFromExternalStorage() const;
 };
 
 class FileScopeAsmDecl : public Decl {
   virtual void anchor();
   StringLiteral *AsmString;

cfe/trunk/lib/AST/Decl.cpp

else
       Context.getDiagnostics().Report(
           getLocation(),
           diag::remark_sanitize_address_insert_extra_padding_accepted)
           << getQualifiedNameAsString();
   }
   return ReasonToReject < 0;
 }
 
+const FieldDecl *RecordDecl::findFirstNamedDataMember() const {
+  for (const auto *I : fields()) {
+    if (I->getIdentifier())
+      return I;
+
+    if (const RecordType *RT = I->getType()->getAs<RecordType>())
+      if (const FieldDecl *NamedDataMember =
+          RT->getDecl()->findFirstNamedDataMember())
+        return NamedDataMember;
+  }
+
+  // We didn't find a named data member.
+  return nullptr;
+}
+
+
 //===----------------------------------------------------------------------===//
 // BlockDecl Implementation
 //===----------------------------------------------------------------------===//
 
 void BlockDecl::setParams(ArrayRef<ParmVarDecl *> NewParamInfo) {
   assert(!ParamInfo && "Already has param info!");
 
   // Zero params -> null pointer.

cfe/trunk/lib/AST/ItaniumCXXABI.cpp

 #include "clang/AST/RecordLayout.h"
 #include "clang/AST/Type.h"
 #include "clang/Basic/TargetInfo.h"
 
 using namespace clang;
 
 namespace {
 
+/// According to Itanium C++ ABI 5.1.2:
+/// 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 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.
+///
+/// Returns the name of anonymous union VarDecl or nullptr if it is not found.
+static const IdentifierInfo *findAnonymousUnionVarDeclName(const VarDecl& VD) {
+  const RecordType *RT = VD.getType()->getAs<RecordType>();
+  assert(RT && "type of VarDecl is expected to be RecordType.");
+  assert(RT->getDecl()->isUnion() && "RecordType is expected to be a union.");
+  if (const FieldDecl *FD = RT->getDecl()->findFirstNamedDataMember()) {
+    return FD->getIdentifier();
+  }
+
+  return nullptr;
+}
+
 /// \brief Keeps track of the mangled names of lambda expressions and block
 /// literals within a particular context.
 class ItaniumNumberingContext : public MangleNumberingContext {
   llvm::DenseMap<const Type *, unsigned> ManglingNumbers;
-  llvm::DenseMap<IdentifierInfo *, unsigned> VarManglingNumbers;
-  llvm::DenseMap<IdentifierInfo *, unsigned> TagManglingNumbers;
+  llvm::DenseMap<const IdentifierInfo *, unsigned> VarManglingNumbers;
+  llvm::DenseMap<const IdentifierInfo *, unsigned> TagManglingNumbers;
 
 public:
   unsigned getManglingNumber(const CXXMethodDecl *CallOperator) override {
     const FunctionProtoType *Proto =
         CallOperator->getType()->getAs<FunctionProtoType>();
     ASTContext &Context = CallOperator->getASTContext();
 
     QualType Key =
public:
   }
 
   unsigned getStaticLocalNumber(const VarDecl *VD) override {
     return 0;
   }
 
   /// Variable decls are numbered by identifier.
   unsigned getManglingNumber(const VarDecl *VD, unsigned) override {
-    return ++VarManglingNumbers[VD->getIdentifier()];
+    const IdentifierInfo *Identifier = VD->getIdentifier();
+    if (!Identifier) {
+      // VarDecl without an identifier represents an anonymous union declaration.
+      Identifier = findAnonymousUnionVarDeclName(*VD);
+    }
+    return ++VarManglingNumbers[Identifier];
   }
 
   unsigned getManglingNumber(const TagDecl *TD, unsigned) override {
     return ++TagManglingNumbers[TD->getIdentifier()];
   }
 };
 
 class ItaniumCXXABI : public CXXABI {

cfe/trunk/lib/AST/ItaniumMangle.cpp

 }
 
 static const unsigned UnknownArity = ~0U;
 
 class ItaniumMangleContextImpl : public ItaniumMangleContext {
   typedef std::pair<const DeclContext*, IdentifierInfo*> DiscriminatorKeyTy;
   llvm::DenseMap<DiscriminatorKeyTy, unsigned> Discriminator;
   llvm::DenseMap<const NamedDecl*, unsigned> Uniquifier;
-  
 public:
   explicit ItaniumMangleContextImpl(ASTContext &Context,
                                     DiagnosticsEngine &Diags)
       : ItaniumMangleContext(Context, Diags) {}
 
   /// @name Mangler Entry Points
   /// @{
 

 void CXXNameMangler::mangleUnresolvedName(NestedNameSpecifier *qualifier,
                                           NamedDecl *firstQualifierLookup,
                                           DeclarationName name,
                                           unsigned knownArity) {
   if (qualifier) mangleUnresolvedPrefix(qualifier, firstQualifierLookup);
   mangleUnqualifiedName(nullptr, name, knownArity);
 }
 
-static const FieldDecl *FindFirstNamedDataMember(const RecordDecl *RD) {
-  assert(RD->isAnonymousStructOrUnion() &&
-         "Expected anonymous struct or union!");
-  
-  for (const auto *I : RD->fields()) {
-    if (I->getIdentifier())
-      return I;
-    
-    if (const RecordType *RT = I->getType()->getAs<RecordType>())
-      if (const FieldDecl *NamedDataMember = 
-          FindFirstNamedDataMember(RT->getDecl()))
-        return NamedDataMember;
-  }
-
-  // We didn't find a named data member.
-  return nullptr;
-}
-
 void CXXNameMangler::mangleUnqualifiedName(const NamedDecl *ND,
                                            DeclarationName Name,
                                            unsigned KnownArity) {
   //  <unqualified-name> ::= <operator-name>
   //                     ::= <ctor-dtor-name>
   //                     ::= <source-name>
   switch (Name.getNameKind()) {
   case DeclarationName::Identifier: {
if (const NamespaceDecl *NS = dyn_cast<NamespaceDecl>(ND)) {
         // This is how gcc mangles these names.
         Out << "12_GLOBAL__N_1";
         break;
       }
     }
 
     if (const VarDecl *VD = dyn_cast<VarDecl>(ND)) {
       // We must have an anonymous union or struct declaration.
-      const RecordDecl *RD = 
+      const RecordDecl *RD =
         cast<RecordDecl>(VD->getType()->getAs<RecordType>()->getDecl());
-      
       // Itanium C++ ABI 5.1.2:
       //
       //   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 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.
-      const FieldDecl *FD = FindFirstNamedDataMember(RD);
+      assert(RD->isAnonymousStructOrUnion()
+             && "Expected anonymous struct or union!");
+      const FieldDecl *FD = RD->findFirstNamedDataMember();
 
       // It's actually possible for various reasons for us to get here
       // with an empty anonymous struct / union.  Fortunately, it
       // doesn't really matter what name we generate.
       if (!FD) break;
       assert(FD->getIdentifier() && "Data member name isn't an identifier!");
-      
       mangleSourceName(FD->getIdentifier());
       break;
     }
 
     // Class extensions have no name as a category, and it's possible
     // for them to be the semantic parent of certain declarations
     // (primarily, tag decls defined within declarations).  Such
     // declarations will always have internal linkage, so the name
void ItaniumMangleContextImpl::mangleCXXName(const NamedDecl *D,
   assert(!isa<CXXConstructorDecl>(D) && !isa<CXXDestructorDecl>(D) &&
          "Invalid mangleName() call on 'structor decl!");
 
   PrettyStackTraceDecl CrashInfo(D, SourceLocation(),
                                  getASTContext().getSourceManager(),
                                  "Mangling declaration");
 
   CXXNameMangler Mangler(*this, Out, D);
-  return Mangler.mangle(D);
+  Mangler.mangle(D);
 }
 
 void ItaniumMangleContextImpl::mangleCXXCtor(const CXXConstructorDecl *D,
                                              CXXCtorType Type,
                                              raw_ostream &Out) {
   CXXNameMangler Mangler(*this, Out, D, Type);
   Mangler.mangle(D);
 }

 void ItaniumMangleContextImpl::mangleStringLiteral(const StringLiteral *, raw_ostream &) {
   llvm_unreachable("Can't mangle string literals");
 }
 
 ItaniumMangleContext *
 ItaniumMangleContext::create(ASTContext &Context, DiagnosticsEngine &Diags) {
   return new ItaniumMangleContextImpl(Context, Diags);
 }
+

cfe/trunk/test/CodeGenCXX/mangle-local-anonymous-unions.cpp

+// RUN: %clang_cc1 %s -emit-llvm -triple %itanium_abi_triple -o - | FileCheck %s
+
+// CHECK-DAG: @_ZZ2f0vE1a
+// CHECK-DAG: @_ZZ2f0vE1c
+// CHECK-DAG: @_ZZ2f0vE1e_0
+inline int f0() {
+  static union {
+    int a;
+    long int b;
+  };
+
+  static union {
+    int c;
+    double d;
+  };
+
+  if (0) {
+    static union {
+      int e;
+      int f;
+    };
+  }
+  static union {
+    int e;
+    int f;
+  };
+
+  return a+c;
+}
+
+inline void nop() {
+  static union {
+    union {
+    };
+  };
+}
+
+int f1 (int a, int c) {
+  nop();
+  return a+c+f0();
+}
+