AST: mangle BlockDecls under MS ABI

[compnerd]

Summary

When generating the decorated name for a static variable inside a BlockDecl, construct a scope for the block invocation function that homes the parameter. This allows for arbitrary nesting of the blocks even if the variables are shadowed. Furthermore, using this for the name allows for undname to properly undecorated the name for us. It shows up as the synthetic __block_invocation function that the compiler emitted in the local scope.

[majnemer]

Can you please have a test where you define blocks w/ static variables in several default arguments of the same function? Also would be good to have this in NSDMIs in class definitions.

[efriedma]

Patch is missing context.

You have to use getBlockManglingNumber() for blocks which are externally visible; otherwise, the numbers won't be consistent in other translation units.

[compnerd]

@efriedma hmm...using getBlockManglingNumber causes the name to be duplicated. Ill look into that. However, wouldn't all the block invocation functions be defined and COMDAT'ed?

@majnemer Sure, will add more tests.

[efriedma]

@efriedma hmm...using getBlockManglingNumber causes the name to be duplicated. Ill look into that.

Have you looked at the Itanium mangling implementation?

However, wouldn't all the block invocation functions be defined and COMDAT'ed?

IIRC, we always emit blocks with internal linkage, not linkonce. But even if we did use linkonce linkage, the block could get inlined.

[compnerd]

Add additional test cases, improve coverage and mangling.

[compnerd]

@efriedma which bit of the Itanium mangling should I be looking at? A BlockDecl does not have visibility associated with them, so Im not sure what I should be checking to see if the block is visible or not. What is the best way forward for finishing this up?

[efriedma]

I think we just use "0" as a sentinel value to indicate the block doesn't need a mangling number. getLambdaManglingNumber works the same way? See CXXNameMangler::mangleUnqualifiedBlock in the Itanium mangler.

[compnerd]

@efriedma I think that Im still not understanding the case that you are trying to point out. How do we end up in a state where the block invocation function is referenced external to the TU? The block would be referenced to by name of the block, no? AFAICT, this is for local storage in a block, which is scoped to the block invocation function, which is TU local, and will be referenced by the block_literal (which contains the block invocation function and is dispatched via the BlocksRuntime).

[efriedma]

How do we end up in a state where the block invocation function is referenced external to the TU?

ODR allows certain definitions, like class definitions and inline function definitions, to be written in multiple translation units. See http://itanium-cxx-abi.github.io/cxx-abi/abi.html#closure-types , specifically the list of cases where lambda types are required to correspond.

[compnerd]

This is a step in the right direction. Although the NSDMI cases and default parameter value cases are not yet handled, they break due to tracking of the global mangling number tracking, not due to the scheme.

[majnemer]

lib/AST/MicrosoftMangle.cpp

974–977

I think you want to use mangleArtificalTagType here.

[compnerd]

Ah, thanks for the explanation @efriedma.

lib/AST/MicrosoftMangle.cpp

974–977

I was considering that. The addition of the Discriminator makes that harder. I can create a local buffer and create the name and then mangle that though if you feel strongly about that.

[majnemer]

lib/AST/MicrosoftMangle.cpp

974–977

I do, it will ensure it correctly backreferences.

[compnerd]

Address feedback. Also fix the case that was previously not working. This now covers all the various cases that have been discussed.

[majnemer]

lib/AST/MicrosoftMangle.cpp

985

Should this be Out << '?' << mangleSourceName(Discriminate("_block_invoke", Discriminator));

990–994

You should probably add comments explaining these bits.

[compnerd]

Use mangleSourceName

[majnemer]

We need tests that show that it does the right thing in blocks defined in classes in classes and other nested concepts.

lib/AST/MicrosoftMangle.cpp

980–981

Why isn't it local?

991–992

Can blocks not be given a specific calling convention?

993–994

Shouldn't we also mangle an 'E' in here on 64-bit platforms?

999–1001

This logic should be explained.

1003–1005

This logic should be explained.

[compnerd]

I can add the nested/nested classes. What were other nested concepts you thinking of?

lib/AST/MicrosoftMangle.cpp

980–981

This code path is global blocks only AFAIK.

991–992

Not AFAIK. The synthetic function here is dispatched through the BlocksRuntime and is assumed to be cdecl.

993–994

I suppose that the default handling in x86_64 would give that. I don't have a strong enough opinion on that. I can add that if you think it makes a difference.

[compnerd]

Some more comments, add test case.

[majnemer]

lib/AST/MicrosoftMangle.cpp

993–994

It is consistent with other pointer manglings; I think it'd be best to mangle pointer types as being 64-bit consistently.

[compnerd]

__ptr64 mangling, add tests for 32-bit.

[majnemer]

Looks good to me but I definitely want to hear what @efriedma has to say.

[efriedma]

lib/AST/MicrosoftMangle.cpp

988

The call to mangleName() looks a little weird... I would have expected a call to mangleUnqualifiedName or something like that. (If there's some reason for the asymmetry, a brief comment explaining it would be fine.)

1008

This isn't quite the same thing the Itanium mangling does...

Itanium has special cases for in-class initializers and default arguments. Using the name is fine for in-class initalizers, but it doesn't really work for default arguments: arguments aren't required to have names. Consider, for example:

extern int e(void);
class C {
  void m(int = ^{ static int i = e(); return ++i; }(),
         int = ^{ static int i = e(); return ++i; }());
};

test/CodeGenCXX/msabi-blocks.cpp

91

The Itanium ABI document lists five cases where the mangling is externally visible. I think this is missing a testcase for the "initializers of nonspecialized static members of template classes" case. (Something like "template<typename T> class X { static T foo; }; template<typename T> T X<T>::foo = ^{static int i = e(); return ++i;}();".)

[compnerd]

lib/AST/MicrosoftMangle.cpp

988

Accounting for the parameter like we do for lambdas makes this unnecessary.

1008

Added a test case and handled it.

test/CodeGenCXX/msabi-blocks.cpp

91

This is currently broken even under the itanium scheme. I think that doing that in a follow up is reasonable.

template <typename T>
struct s {
  static T i;
};

template <typename T>
T s<T>::i = ^{ static T i = T(); return i; }();

template class s<int>;
template class s<short>;

[efriedma]

test/CodeGenCXX/msabi-blocks.cpp

91

Fine; please file a bug.

[compnerd]

lib/AST/MicrosoftMangle.cpp

988

Err, still need it for the fields; added a comment.

[compnerd]

Handle unnamed parameters, improve mangling for NSDMis. The one case that we dont handle currently causes an assertion even in itanium mode.

[efriedma]

LGTM (with the caveat that I don't know anything about Microsoft mangling).

[compnerd]

SVN r306347

Thanks @efriedma and @majnemer!

lib/AST/MicrosoftMangle.cpp

if (isa<TagDecl>(ND) || isa<VarDecl>(ND)) {
       if (Context.getNextDiscriminator(ND, Disc)) {
         Out << '?';
         mangleNumber(Disc);
         Out << '?';
       }
     }
 
     if (const BlockDecl *BD = dyn_cast<BlockDecl>(DC)) {
-      DiagnosticsEngine &Diags = Context.getDiags();
-      unsigned DiagID =
-          Diags.getCustomDiagID(DiagnosticsEngine::Error,
-                                "cannot mangle a local inside this block yet");
-      Diags.Report(BD->getLocation(), DiagID);
-
-      // FIXME: This is completely, utterly, wrong; see ItaniumMangle
-      // for how this should be done.
-      Out << "__block_invoke" << Context.getBlockId(BD, false);
+      auto Discriminate = [](StringRef Name,
+                             const unsigned Discriminator) -> std::string {
+        std::string Buffer;
+        llvm::raw_string_ostream Stream(Buffer);
+        Stream << Name;
+        if (Discriminator)
+          Stream << '_' << Discriminator;
+        return Stream.str();
+      };
+
+      unsigned Discriminator = BD->getBlockManglingNumber();
+      if (!Discriminator)
+        Discriminator = Context.getBlockId(BD, /*Local=*/false);
+
+      DC = getEffectiveDeclContext(BD);
+
+      Out << '?';
+      mangleSourceName(Discriminate("_block_invoke", Discriminator));
+      if (const auto *RD = dyn_cast<RecordDecl>(DC))
+        mangleName(RD);
+      else
         Out << '@';
+      // void __cdecl
+      Out << "YAX";
+      // struct __block_literal *
+      Out << "PA";
+      mangleArtificalTagType(TTK_Struct,
+                             Discriminate("__block_literal", Discriminator));
+      Out << "@Z";
+
+      // If we have a block mangling context, encode that now.  Although the
+      // mangling is not perfect, it allows us to discriminate between multiple
+      // instances in the same scope.
+      if (const auto *MC = BD->getBlockManglingContextDecl())
+        if (const auto *ND = dyn_cast<NamedDecl>(MC))
+          mangleUnqualifiedName(ND);
+
+      // If the effective context was a Record, we have fully mangled the
+      // qualified name and do not need to continue.
+      if (isa<RecordDecl>(DC))
+        break;
+      continue;
     } else if (const ObjCMethodDecl *Method = dyn_cast<ObjCMethodDecl>(DC)) {
       mangleObjCMethodName(Method);
     } else if (isa<NamedDecl>(DC)) {
       ND = cast<NamedDecl>(DC);
       if (const FunctionDecl *FD = dyn_cast<FunctionDecl>(ND)) {
         mangle(FD, "?");
         break;
       } else {

test/CodeGenCXX/msabi-blocks.cpp

+// RUN: %clang_cc1 -triple x86_64-unknown-windows-msvc -std=c++11 -fblocks -S -o - -emit-llvm %s | FileCheck %s
+
+extern int e(void);
+
+void (^b)() = ^{
+  static int i = 0;
+};
+
+// CHECK-DAG: @"\01?i@?1??_block_invoke@@YAXPAU__block_literal@@@Z@4HA" ={{.*}} global i32 0
+
+void f(void) {
+  static int i = 0;
+  ^{ static int i = e(); }();
+
+// CHECK-DAG: @"\01?i@?1??_block_invoke_1@@YAXPAU__block_literal_1@@@Z?1??f@@YAXXZ@4HA" ={{.*}} global i32 0
+
+  ^{ static int i = e(); }();
+
+// CHECK-DAG: @"\01?i@?1??_block_invoke_2@@YAXPAU__block_literal_2@@@Z?1??f@@YAXXZ@4HA" ={{.*}} global i32 0
+
+  ^{ ^{ static int i = e(); }(); }();
+
+// CHECK-DAG: @"\01?i@?1??_block_invoke_3@@YAXPAU__block_literal_3@@@Z?1??_block_invoke_4@@YAXPAU__block_literal_4@@@Z?1??f@@YAXXZ@4HA" ={{.*}} global i32 0
+}
+
+
+template <typename>
+void g(void) {
+  ^{ static int i = e(); }();
+}
+
+template void g<char>(void);
+
+// CHECK-DAG: @"\01?i@?2??_block_invoke_1@@YAXPAU__block_literal_1@@@Z?2???$g@D@@YAXXZ@4HA" ={{.*}} global i32 0
+
+template void g<int>(void);
+
+// CHECK-DAG: @"\01?i@?2??_block_invoke_1@@YAXPAU__block_literal_1@@@Z?2???$g@H@@YAXXZ@4HA" ={{.*}} global i32 0
+
+inline void h(void) {
+  ^{ static int i = e(); }();
+}
+
+// CHECK-DAG: @"\01?i@?2??_block_invoke_1@@YAXPAU__block_literal_1@@@Z?2??h@@YAXXZ@4HA" ={{.*}} global i32 0
+
+struct s {
+  int i = ^{ static int i = e(); return ++i; }();
+
+// CHECK-DAG: @"\01?i@?0??_block_invoke_1@s@@YAXPAU__block_literal_1@@@Z0@4HA" ={{.*}} global i32 0
+
+  int j = ^{ static int i = e(); return ++i; }();
+
+// CHECK-DAG: @"\01?i@?0??_block_invoke_1@s@@YAXPAU__block_literal_1@@@Zj@@4HA" ={{.*}} global i32 0
+
+  void m(int i = ^{ static int i = e(); return ++i; }(),
+         int j = ^{ static int i = e(); return ++i; }()) {}
+
+// CHECK-DAG: @"\01?i@?0??_block_invoke_1@@YAXPAU__block_literal_1@@@Zj@?0??m@s@@QEAAXHH@Z@4HA" ={{.*}} global i32 0
+// CHECK-DAG: @"\01?i@?0??_block_invoke_1@@YAXPAU__block_literal_1@@@Z0?0??m@s@@QEAAXHH@Z@4HA" ={{.*}} global i32 0
+
+};
+
+struct t {
+  struct u {
+    int i = ^{ static int i = e(); return ++i; }();
+
+// CHECK-DAG: @"\01?i@?0??_block_invoke_1@u@t@@YAXPAU__block_literal_1@@@Z0@4HA" ={{.*}} global i32 0
+
+  };
+};
+
+void j(void) {
+  h();
+  struct s s;
+  s.m();
+  struct t::u t;
+}
+