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

[Ehsan Akhgari]

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.

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.

What do you think about that?

--
Ehsan
<http://ehsanakhgari.org/>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140724/64f85ef7/attachment.html>