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

[Ehsan Akhgari]

================
Comment at: include/clang/AST/Decl.h:312
@@ -310,1 +311,3 @@
   LabelStmt *TheStmt;
+  SmallString<16> MSAsmName;
+  bool MSAsmNameResolved;
----------------
rnk wrote:
> 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.
Oh, I didn't know that!  What's this 'new (Contex, 1) char[Len]' trick?  I'm not familiar with it, I don't think...

================
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);
----------------
rnk wrote:
> I think you can sink this into translateLocation if you make it take a SourceMgr and SMLoc.
Yeah looks like it.  Will do.

================
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)
----------------
rnk wrote:
> 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.
I cannot issue the diagnostic there because I will not have a GotoStmt pointer when processing `foo:`.  That's really the main issue.

What I would ideally do would be to not issue any diagnostics in ActOnGotoStmt, and wait until the scope is fully processed, then visit all of the GotoStmts in the scope, check their LabelDecls to see if they're asm labels and issue the diagnostic there.  But I don't know where I would hook into in order to be able to visit all of the GotoStmts at the end of a scope.

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

================
Comment at: lib/Sema/SemaStmtAsm.cpp:534
@@ +533,3 @@
+    // name.
+    OS << llvm::format("__MSASMLABEL_.%" PRIu32 "__", counter++);
+    Label->setMSAsmLabel(OS.str());
----------------
rnk wrote:
> 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.
I think you brought this issue up before.  :)  I am turning these names into local symbol names in http://reviews.llvm.org/D4587 by using getPrivateGlobalPrefix() when rewriting `AOK_Label`.  I don't think that we need to store that prefix in the AST, do we?

Your suggestion to add the user-written label in the name is a good one, it just needs me to change all of these tests again!  ;)  But I'll do it in the next iteration.

On the issue of counter uniqueness, the current global counter obviously ensures that.  If we have one Sema object per each compilation, then making it a Sema member won't change things, so we should be good there.

================
Comment at: test/Sema/ms-inline-asm.c:145
@@ +144,3 @@
+void t10() {
+// TODO: The diagnostic below has the wrong location, because we don't have a better location in CheckPoppedLabel
+foo: // expected-error {{use of undeclared label 'foo'}}
----------------
Any ideas how I can fix this?  Or is the current misplaced diagnostic acceptable?

================
Comment at: test/Sema/ms-inline-asm.c:148
@@ +147,2 @@
+  __asm mov eax, foo
+}
----------------
rnk wrote:
> 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
>       }
>     };
>   }
Certainly!

http://reviews.llvm.org/D4589