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

[Ehsan Akhgari]

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?


On Thu, Jul 31, 2014 at 3:50 PM, Reid Kleckner <rnk at google.com> wrote:

> 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();
>



-- 
Ehsan
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140906/955403ab/attachment.html>