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();
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
