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

[Reid Kleckner]

IdentifierInfo is basically a uniqued string class, you can call
Preprocessor::getIdentifierInfo() to look one up. The parser should have
access to the preprocessor.

On Sat, Sep 6, 2014 at 2:03 PM, Ehsan Akhgari <ehsan.akhgari at gmail.com>
wrote:

> 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/20140915/9ab345db/attachment.html>