<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Thu, Jul 24, 2014 at 2:52 PM, Reid Kleckner <span dir="ltr"><<a href="mailto:rnk@google.com" target="_blank">rnk@google.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">

================<br>
Comment at: lib/Parse/ParseStmtAsm.cpp:96<br>
@@ -95,1 +95,3 @@<br>
<br>
+  void LookupInlineAsmLabel(StringRef &Identifier, llvm::SourceMgr &LSM,<br>
+                            llvm::SMLoc Location, bool Create) {<br>
----------------<br>
You can return a StringRef directly.<br>
<br>
================<br>
Comment at: lib/Sema/SemaDecl.cpp:1492<br>
@@ -1491,1 +1491,3 @@<br>
<br>
+  HandleMSAsmLabelsOnScopePop();<br>
+<br>
----------------<br>
I believe ActOnPopScope happens on any closing curly brace, so this won't do the "right" thing:<br>
  void f() {<br>
    bool failed;<br>
    __asm retry:<br>
    {<br>
      failed = g();<br>
    } // The label is cleared here.<br>
    if (failed)<br>
      __asm jmp retry<br>
  }<br>
<br>
Thinking more carefully, I don't think having the label map right on Sema works at all in the general case of nested functions.  Consider this C++ example with inner classes:<br>
  void f() {<br>
    __asm some_label:<br>
    struct A {<br>
      static void g() {<br>
        __asm jmp some_label ; This should jump forwards<br>
        __asm some_label:<br>
        __asm nop<br>
      }<br>
    };<br>
  }<br>
<br>
You can build similar test cases from C++11 lambdas and Obj-C blocks, and now OpenMP captured statements.<br>
<br>
We need to associate this label map with the function we're currently processing.  Now that I'm considering the actual implementation difficulties, I'm going back to your "proposal #3" on PR20023, where we model these labels as regular label statements.  That way, we reuse all the right name lookup machinery.  We won't be able to 'goto' a label from an inline asm statement or 'jmp' to a non-asm label, so if you go this way, please diagnose both cases.  You can probably add a bit to LabelDecl to track whether the label was from MS inline asm or regular source code.<br>


<br>
Apologies for the runaround, but I think it's probably better to rewrite along these lines.  What do you think?<br></blockquote><div><br></div><div>Hmm, there are a number of problems with that approach.  One is that we need to break up __asm blocks and perhaps remember part of the parser state until we hit the end of the function (such as the number of braces we have seen, their source locations, etc.) which complicates the parsing code, and actually needs us to do something at the end of the function too.  It also means that we won't be able to reuse LookupInlineAsmIdentifier, since AFAICT labels are not Expr's.  Also, it will mean that the __asm blocks will be disjoint across labels, even after my __asm block joining patch.<br>

<br></div><div>It seems like your main objection to this approach is to storing the label map on Sema, and that is quite fair (and I should add tests for that!).  But isn't a much easier solution to that to store the label map at the right place?  I've been looking at the code that handles LabelDecl's, and I think we can get by with storing the map on Scope objects, and do something similar to Sema::LookupOrCreateLabel to make sure we are accessing the right function scope.  I think with that change, everything else in my patch will work properly.<br>

</div><div><br>What do you think about that?<br><br clear="all"><div>--<br>Ehsan<br><<a href="http://ehsanakhgari.org/">http://ehsanakhgari.org/</a>></div> 
<br></div></div></div></div>