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 <[email protected]> wrote: > On Thu, Jul 24, 2014 at 4:26 PM, Ehsan Akhgari <[email protected]> > wrote: > >> On Thu, Jul 24, 2014 at 2:52 PM, Reid Kleckner <[email protected]> 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
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
