[PATCH] [-cxx-abi microsoft] Implement local manglings accurately

[Charles Davis]

  A few minor comments...


================
Comment at: lib/AST/MicrosoftMangle.cpp:621
@@ -599,2 +620,3 @@
+void MicrosoftCXXNameMangler::mangleNestedName(const NamedDecl *ND) {
   // <postfix> ::= <unqualified-name> [<postfix>]
   //           ::= <substitution> [<postfix>]
----------------
You should update the BNF here, since we agreed that `manglePostfix` was a bad name for this function ;).

================
Comment at: lib/AST/MicrosoftMangle.cpp:620-659
@@ -596,43 +619,42 @@
 
-void MicrosoftCXXNameMangler::manglePostfix(const DeclContext *DC,
-                                            bool NoFunction) {
+void MicrosoftCXXNameMangler::mangleNestedName(const NamedDecl *ND) {
   // <postfix> ::= <unqualified-name> [<postfix>]
   //           ::= <substitution> [<postfix>]
+  const DeclContext *DC = ND->getDeclContext();
 
-  if (!DC) return;
+  while (!DC->isTranslationUnit()) {
+    if (isa<TagDecl>(ND) || isa<VarDecl>(ND)) {
+      uint64_t Disc;
+      if (Context.getNextDiscriminator(ND, Disc)) {
+        Out << '?';
+        mangleNumber(Disc);
+        Out << '?';
+      }
+    }
 
-  while (isa<LinkageSpecDecl>(DC))
+    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);
+      Out << '@';
+      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
+        mangleUnqualifiedName(ND);
+    }
     DC = DC->getParent();
-
-  if (DC->isTranslationUnit())
-    return;
-
-  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);
-    Out << '@';
-    return manglePostfix(DC->getParent(), NoFunction);
-  } else if (isa<CapturedDecl>(DC)) {
-    // Skip CapturedDecl context.
-    manglePostfix(DC->getParent(), NoFunction);
-    return;
-  }
-
-  if (NoFunction && (isa<FunctionDecl>(DC) || isa<ObjCMethodDecl>(DC)))
-    return;
-  else if (const ObjCMethodDecl *Method = dyn_cast<ObjCMethodDecl>(DC))
-    mangleObjCMethodName(Method);
-  else if (const FunctionDecl *Func = dyn_cast<FunctionDecl>(DC))
-    mangleLocalName(Func);
-  else {
-    mangleUnqualifiedName(cast<NamedDecl>(DC));
-    manglePostfix(DC->getParent(), NoFunction);
   }
 }
 
----------------
Except for the part about the nested scope discriminator (gosh, I wish I'd thought of that name when I first wrote the nested scope mangling stuff), I think you can just pull out this rewrite here (that we discussed the other day on IRC) and commit it on its own.

================
Comment at: include/clang/Sema/Scope.h:20
@@ -19,2 +19,3 @@
 #include "llvm/ADT/SmallVector.h"
+#include "llvm/Support/raw_ostream.h"
 
----------------
Reid Kleckner wrote:
> You can get by with a forward decl for raw_ostream.
I agree, but don't forget...

================
Comment at: lib/Sema/Scope.cpp:15-17
@@ -14,4 +14,5 @@
 
 #include "clang/Sema/Scope.h"
+#include "llvm/Support/MathExtras.h"
 
 using namespace clang;
----------------
...to make sure this file includes (one way or another) `"llvm/Support/raw_ostream.h"`.

================
Comment at: test/CodeGenCXX/microsoft-abi-static-initializers.cpp:27
@@ -26,3 +26,1 @@
 // CHECK-LABEL: define void @"\01?StaticLocal@@YAXXZ"()
-// CHECK: load i32* @"\01?$S1@?1??StaticLocal@@YAXXZ at 4IA"
-// CHECK: store i32 {{.*}}, i32* @"\01?$S1@?1??StaticLocal@@YAXXZ at 4IA"
----------------
Reid Kleckner wrote:
> Were these names wrong?
I recall David saying (in the summary above) that his algorithm won't mangle certain nested scopes to avoid certain pathological cases. I think this is one of them.

================
Comment at: include/clang/Sema/Scope.h:113-116
@@ -111,2 +112,6 @@
 
+  /// \brief Declarations with static linkage are mangled with the number of
+  /// scopes seen as a component.
+  unsigned short MSLocalManglingNumber;
+
   /// PrototypeDepth - This is the number of function prototype scopes
----------------
Are you sure there isn't a better way to keep track of the number we need to put inside a local name than having `Sema` do it?


http://llvm-reviews.chandlerc.com/D2204