[PATCH] ms-inline-asm: Scope inline asm labels to functions

[Reid Kleckner]

Nice, this completely eliminates the lookup difficulties in the previous approach.

================
Comment at: include/clang/AST/Decl.h:312
@@ -310,1 +311,3 @@
   LabelStmt *TheStmt;
+  SmallString<16> MSAsmName;
+  bool MSAsmNameResolved;
----------------
This will cause a memory leak for names > 16 chars. When the ASTContext is destroyed, it doesn't actually call destructors of individual AST nodes, it merely frees the memory. This should be a StringRef that points to memory from 'new (Context, 1) char[Len]', probably.

================
Comment at: lib/Parse/ParseStmtAsm.cpp:99-101
@@ +98,5 @@
+                                 bool Create) override {
+    const llvm::MemoryBuffer *LBuf =
+      LSM.getMemoryBuffer(LSM.FindBufferContainingLoc(Location));
+    unsigned Offset = Location.getPointer() - LBuf->getBufferStart();
+    SourceLocation Loc = translateLocation(Offset);
----------------
I think you can sink this into translateLocation if you make it take a SourceMgr and SMLoc.

================
Comment at: lib/Sema/SemaStmt.cpp:2357-2358
@@ +2356,4 @@
+  if (TheDecl->isMSAsmLabel()) {
+    // TODO Move this diagnostic elsewhere so that we get to examine the goto
+    // statements after we've seen all of the body of the current scope.
+    StmtResult error(StmtError(Diag(GotoLoc, diag::err_goto_ms_asm_label)
----------------
Does it help if you issue the same diagnostic when processing 'foo:' in this example?
  void f() {
    goto foo;
    __asm {
    foo:
      nop
    }
  }

After issuing the diagnostic, you can avoid the "undeclared label" knock-on errors by pretending that you did in fact define the label.

================
Comment at: lib/Sema/SemaStmtAsm.cpp:522
@@ +521,3 @@
+                                       bool AlwaysCreate) {
+  static uint32_t counter = 0;
+
----------------
This should be a member variable of Sema, rather than a global.

================
Comment at: lib/Sema/SemaStmtAsm.cpp:534
@@ +533,3 @@
+    // name.
+    OS << llvm::format("__MSASMLABEL_.%" PRIu32 "__", counter++);
+    Label->setMSAsmLabel(OS.str());
----------------
We should be able to use the 'L' assembler prefix to suppress the symbol table entry for the label. I'm happy as long as both 'nm' and 'dumpbin /symbols' agree that there are no MSASMLABEL symbols in the object file. I think it will with the code as written.

I'd also try to get the user-written label in there, so that the -S output is more intelligible, something like:
  OS << "L__MSASMLABEL_" << counter++ << "__" << ExternalLabelName;

Users can't make labels that start with 'L', so we should be safe from collisions, we just need a unique counter to make sure we don't collide with ourselves.

================
Comment at: test/Sema/ms-inline-asm.c:148
@@ +147,2 @@
+  __asm mov eax, foo
+}
----------------
Can you add a .cpp test case involving nested scopes, like:

  void f() {
    __asm some_label:
    struct A {
      static void g() {
        __asm jmp some_label ; This should jump forwards
        __asm some_label:
        __asm nop
      }
    };
  }

http://reviews.llvm.org/D4589