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

[Reid Kleckner]

On Thu, Jul 24, 2014 at 4:26 PM, Ehsan Akhgari <ehsan.akhgari at gmail.com>
wrote:

> On Thu, Jul 24, 2014 at 2:52 PM, Reid Kleckner <rnk at google.com> wrote:
>
>> ================
>> Comment at: lib/Parse/ParseStmtAsm.cpp:96
>> @@ -95,1 +95,3 @@
>>
>> +  void LookupInlineAsmLabel(StringRef &Identifier, llvm::SourceMgr &LSM,
>> +                            llvm::SMLoc Location, bool Create) {
>> ----------------
>> You can return a StringRef directly.
>>
>> ================
>> Comment at: lib/Sema/SemaDecl.cpp:1492
>> @@ -1491,1 +1491,3 @@
>>
>> +  HandleMSAsmLabelsOnScopePop();
>> +
>> ----------------
>> I believe ActOnPopScope happens on any closing curly brace, so this won't
>> do the "right" thing:
>>   void f() {
>>     bool failed;
>>     __asm retry:
>>     {
>>       failed = g();
>>     } // The label is cleared here.
>>     if (failed)
>>       __asm jmp retry
>>   }
>>
>> 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:
>>   void f() {
>>     __asm some_label:
>>     struct A {
>>       static void g() {
>>         __asm jmp some_label ; This should jump forwards
>>         __asm some_label:
>>         __asm nop
>>       }
>>     };
>>   }
>>
>> You can build similar test cases from C++11 lambdas and Obj-C blocks, and
>> now OpenMP captured statements.
>>
>> 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.
>>
>> Apologies for the runaround, but I think it's probably better to rewrite
>> along these lines.  What do you think?
>>
>
> 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.
>

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.


> 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.
>

That would work too, but I think you'll end up duplicating lots of the
label processing logic if you go that way.

It's probably also worth thinking about how to handle these kinds of test
cases:

void foo() {
  __asm jmp bar ; is 'bar' a function name or label?
}
void bar();
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140731/626c9173/attachment.html>