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

[Reid Kleckner]

================
Comment at: include/clang/AST/Decl.h:312
@@ -310,1 +311,3 @@
   LabelStmt *TheStmt;
+  SmallString<16> MSAsmName;
+  bool MSAsmNameResolved;
----------------
ehsan wrote:
> 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...
It's an operator new overload declared in ASTContext.h. Basically, it's equivalent to 'new char[Len]', except it goes calls the 'void* operator new(size_t Bytes, ASTContext &Context, size_t Alignment)' overload.

================
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)
----------------
ehsan wrote:
> 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.
Looks like we have JumpDiagnostics.cpp for this purpose... See if you can work it in there?

================
Comment at: lib/Sema/SemaStmtAsm.cpp:534
@@ +533,3 @@
+    // name.
+    OS << llvm::format("__MSASMLABEL_.%" PRIu32 "__", counter++);
+    Label->setMSAsmLabel(OS.str());
----------------
ehsan wrote:
> 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.
OK, doing this in LLVM MC seems great.

================
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'}}
----------------
ehsan wrote:
> Any ideas how I can fix this?  Or is the current misplaced diagnostic acceptable?
Moving to JumpDiagnostics.cpp will probably fix it.

http://reviews.llvm.org/D4589