<div dir="ltr">I started to look into this again, and I hit an issue that I'm not sure how to solve. Creating a LabelDecl requires an IdentifierInfo corresponding to the identifier token for the label name, but for these asm labels, we use a different tokenizer on the LLVM side that doesn't know about IdentifierInfo's at all. It looks like the IdentifierInfo argument is used to construct a DeclarationName. I suppose one option that I have here is to add a new NameKind to DeclarationName and make it possible to construct a DeclarationName from that, but is there an easier option here?<br></div><div class="gmail_extra"><br><br><div class="gmail_quote">On Thu, Jul 31, 2014 at 3:50 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:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><div class="h5">On Thu, Jul 24, 2014 at 4:26 PM, Ehsan Akhgari <span dir="ltr"><<a href="mailto:ehsan.akhgari@gmail.com" target="_blank">ehsan.akhgari@gmail.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><div>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></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>
</div></div></div></div></blockquote><div><br></div></div></div><div>I'm don't want to support going to labels in asm or jumping out: we shouldn't break up __asm blobs and introduce new LabelStmts. Instead, we should synthesize a LabelDecl with no associated LabelStmt and use the rest of the usual label machinery to insert it into the correct function scope. However, when we see 'goto label_from_asm', we'll have to add code to reject that. Presumably we can tell the two kinds of LabelDecl apart by the presence or absence of a LabelStmt.</div><span class="">
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div></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.</div>
</div></div></div></blockquote><div><br></div></span><div>That would work too, but I think you'll end up duplicating lots of the label processing logic if you go that way.</div><div><br></div><div>It's probably also worth thinking about how to handle these kinds of test cases:</div>
<div><br></div><div>void foo() {</div><div> __asm jmp bar ; is 'bar' a function name or label?<br>}</div><div>void bar();</div></div></div></div>
</blockquote></div><br><br clear="all"><br>-- <br><div dir="ltr">Ehsan<br></div>
</div>