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

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

You are right. Reverted to the old code.

[eastig]

Ping

[rsmith]

LGTM

lib/AST/ItaniumCXXABI.cpp

50 ↗

(On Diff #16676)

Can this happen?

include/clang/AST/Mangle.h

   DiagnosticsEngine &getDiags() const { return Diags; }
 
   virtual void startNewFunction() { LocalBlockIds.clear(); }
-  
+  virtual void processNewFunction(const FunctionDecl &FD) {}
+
   unsigned getBlockId(const BlockDecl *BD, bool Local) {
     llvm::DenseMap<const BlockDecl *, unsigned> &BlockIds
       = Local? LocalBlockIds : GlobalBlockIds;

lib/AST/ItaniumMangle.cpp

 
 class ItaniumMangleContextImpl : public ItaniumMangleContext {
   typedef std::pair<const DeclContext*, IdentifierInfo*> DiscriminatorKeyTy;
-  llvm::DenseMap<DiscriminatorKeyTy, unsigned> Discriminator;
+  llvm::DenseMap<DiscriminatorKeyTy, unsigned> Discriminators;
   llvm::DenseMap<const NamedDecl*, unsigned> Uniquifier;
-  
+private:
+  unsigned uniquifyNamedDecl(const NamedDecl *ND);
+
 public:
   explicit ItaniumMangleContextImpl(ASTContext &Context,
                                     DiagnosticsEngine &Diags)

 
   void mangleStringLiteral(const StringLiteral *, raw_ostream &) override;
 
-  bool getNextDiscriminator(const NamedDecl *ND, unsigned &disc) {
-    // Lambda closure types are already numbered.
-    if (isLambda(ND))
-      return false;
-
-    // Anonymous tags are already numbered.
-    if (const TagDecl *Tag = dyn_cast<TagDecl>(ND)) {
-      if (Tag->getName().empty() && !Tag->getTypedefNameForAnonDecl())
-        return false;
-    }
-
-    // Use the canonical number for externally visible decls.
-    if (ND->isExternallyVisible()) {
-      unsigned discriminator = getASTContext().getManglingNumber(ND);
-      if (discriminator == 1)
-        return false;
-      disc = discriminator - 2;
-      return true;
-    }
-
-    // Make up a reasonable number for internal decls.
-    unsigned &discriminator = Uniquifier[ND];
-    if (!discriminator) {
-      const DeclContext *DC = getEffectiveDeclContext(ND);
-      discriminator = ++Discriminator[std::make_pair(DC, ND->getIdentifier())];
-    }
-    if (discriminator == 1)
-      return false;
-    disc = discriminator-2;
-    return true;
-  }
+  bool getNextDiscriminator(const NamedDecl *ND, unsigned &disc);
+  void processNewFunction(const FunctionDecl &FD) override;
   /// @}
 };
 

                                  "Mangling declaration");
 
   CXXNameMangler Mangler(*this, Out, D);
-  return Mangler.mangle(D);
+  Mangler.mangle(D);
 }
 
 void ItaniumMangleContextImpl::mangleCXXCtor(const CXXConstructorDecl *D,

 ItaniumMangleContext::create(ASTContext &Context, DiagnosticsEngine &Diags) {
   return new ItaniumMangleContextImpl(Context, Diags);
 }
+
+static bool isDiscriminatorNeeded(const NamedDecl *ND) {
+  // Lambda closure types are already numbered.
+  if (isLambda(ND))
+    return false;
+
+  // Anonymous tags are already numbered.
+  if (const TagDecl *Tag = dyn_cast<TagDecl>(ND)) {
+    if (Tag->getName().empty() && !Tag->getTypedefNameForAnonDecl())
+      return false;
+  }
+
+  if (const VarDecl *VD = dyn_cast<VarDecl>(ND)) {
+      return VD->isStaticLocal() || VD->isExternallyVisible();
+  }
+
+  return true;
+}
+
+unsigned ItaniumMangleContextImpl::uniquifyNamedDecl(const NamedDecl *ND) {
+ if (IdentifierInfo *Identifier = ND->getIdentifier()) {
+    const NamedDecl *D = ND;
+    if (const IndirectFieldDecl *IFD = dyn_cast<IndirectFieldDecl>(ND)) {
+      D = IFD->getVarDecl();
+    }
+    unsigned &Discriminator = Uniquifier[D];
+    if (!Discriminator) {
+      const DeclContext *DC = getEffectiveDeclContext(D);
+      Discriminator = ++Discriminators[std::make_pair(DC, Identifier)];
+    }
+    return Discriminator;
+  } else
+    return 0;
+}
+
+void ItaniumMangleContextImpl::processNewFunction(const FunctionDecl &FD) {
+  for (auto D: FD.decls()) {
+    if (const NamedDecl * ND = dyn_cast<NamedDecl>(D)) {
+      if (isDiscriminatorNeeded(ND))
+        uniquifyNamedDecl(ND);
+    }
+  }
+}
+
+bool ItaniumMangleContextImpl::getNextDiscriminator(const NamedDecl *ND,
+                                                unsigned &disc) {
+  if (!isDiscriminatorNeeded(ND))
+    return false;
+
+  unsigned Discriminator = Uniquifier[ND];
+  if (!Discriminator) {
+    Discriminator = uniquifyNamedDecl(ND);
+  }
+  if (Discriminator <= 1)
+    return false;
+  disc = Discriminator-2;
+  return true;
+}
+

lib/CodeGen/CodeGenFunction.cpp

 void CodeGenFunction::GenerateCode(GlobalDecl GD, llvm::Function *Fn,
                                    const CGFunctionInfo &FnInfo) {
   const FunctionDecl *FD = cast<FunctionDecl>(GD.getDecl());
+  CGM.getCXXABI().getMangleContext().processNewFunction(*FD);
 
   // Check if we should generate debug info for this function.
   if (FD->hasAttr<NoDebugAttr>())

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;
+}
+
+// CHECK-DAG: @_ZZ2f1vE1a
+// CHECK-DAG: @_ZZ2f1vE1c
+// CHECK-DAG: @_ZZ2f1vE1c_0
+// CHECK-DAG: @_ZZ2f1vE1c_1
+// CHECK-DAG: @_ZZ2f1vE1e_0
+int f1 () {
+  static union {
+    int a;
+    long int b;
+  };
+
+  static union {
+    int c;
+    double d;
+  };
+
+  { static int c; }
+  { static int c; }
+
+  if (0) {
+    static union {
+      int e;
+      int f;
+    };
+  }
+  static union {
+    int e;
+    int f;
+  };
+
+  return a+c+f0();
+}
+

test/CodeGenCXX/mangle.cpp

 }
 
 namespace test40 {
-  // CHECK: i32* {{.*}} @_ZZN6test401fEvE1a_0
+  // CHECK-DAG: i32* {{.*}} @_ZZN6test401fEvE1a_0
   void h(int&);
   inline void f() {
     if (0) {

     static int a;
     h(a);
   };
-  void g() { f(); }
+  // CHECK-DAG: i32* {{.*}} @_ZZN6test402ffEvE1a_0
+  void ff() {
+    if (0) {
+      static int a;
+    }
+    static int a;
+    h(a);
+  }
+  void g() { f(); ff(); }
 }
 
 namespace test41 {