efriedma added inline comments.
================ Comment at: lib/Sema/SemaStmtAsm.cpp:470 + if (NS->isGCCAsmGoto() && + Exprs[ConstraintIdx]->getStmtClass() == Stmt::AddrLabelExprClass) + break; ---------------- jyu2 wrote: > efriedma wrote: > > jyu2 wrote: > > > jyu2 wrote: > > > > efriedma wrote: > > > > > jyu2 wrote: > > > > > > efriedma wrote: > > > > > > > jyu2 wrote: > > > > > > > > efriedma wrote: > > > > > > > > > This looks suspicious; an AddrLabelExpr could be an input or > > > > > > > > > output, e.g. `"r"(&&foo)`. > > > > > > > > Syntax for asm goto: > > > > > > > > Syntax: > > > > > > > > asm [volatile] goto ( AssemblerTemplate > > > > > > > > : > > > > > > > > : InputOperands > > > > > > > > : Clobbers > > > > > > > > : GotoLabels) > > > > > > > > > > > > > > > > Only input is allowed. Output is not allowed > > > > > > > > > > > > > > > That doesn't really address my point here... ignore the "or > > > > > > > output" part of the comment. > > > > > > Sorry did not realize that. Thank you so much for catching that. > > > > > > Need to add other condition "ConstraintIdx > NS->getNumInputs() - > > > > > > 1", change to : > > > > > > > > > > > > if (NS->isGCCAsmGoto() && ConstraintIdx > NS->getNumInputs() - 1 && > > > > > > Exprs[ConstraintIdx]->getStmtClass() == > > > > > > Stmt::AddrLabelExprClass) > > > > > > break; > > > > > > > > > > > > Is this ok with you? Thanks > > > > > That's the right idea. But I still see a few issues at that point: > > > > > > > > > > 1. The AddrLabelExprClass check is redundant. > > > > > 2. "NS->getNumInputs() - 1" can overflow; probably should use > > > > > "ConstraintIdx >= NS->getNumInputs()". > > > > > 3. "break" exits the loop completely (so it skips validating all > > > > > constraints written after the label). > > > > > 4. The code needs to verify that the user correctly specified the "l" > > > > > constraint modifier. > > > > Sorry not done yet. > > > For you comment 4: > > > > > > The code needs to verify that the user correctly specified the "l" > > > constraint modifier. We already emit error like following? > > > > > > Do you mean, we need more checking here? Thanks. > > > > > > n.c:4:35: error: unknown symbolic operand name in inline assembly string > > > asm goto ("frob %%r5, %1; jc %l[error]; mov (%2), %%r5" > > > ~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~ > > > n.c:8:15: error: use of undeclared label 'error1' > > > : error1); > > > > > > Test is: > > > int frob(int x) > > > { > > > int y; > > > asm goto ("frob %%r5, %1; jc %l[error]; mov (%2), %%r5" > > > : /* No outputs. */ > > > : "r"(x), "r"(&y) > > > : "memory" > > > : error1); > > > return y; > > > error: > > > return -1; > > > } > > > > > > > > I mean, there needs to be a diagnostic for the following: > > > > ``` > > asm goto ("jne %h0"::::x); > > ``` > > > > On a related note, there should also be a diagnostic for the following > > somewhere: > > > > ``` > > asm ("jne %l0"::"r"(0)); > > ``` > Hi Eli, > > Thanks for your review. > > For case: > asm goto ("jne %h0"::::x); > > Without define label x, both clang and my current implementation give error > of "use of undeclared label" > > if x is defined: gcc give error > asm_goto>!gcc > gcc n.c > n.c: Assembler messages: > n.c:4: Error: operand type mismatch for `jne' > > My current implementation don't emit error. I think this is need to be done > in LLVM. Am I right here? > > For the case: > asm ("jne %l0"::"r"(0)); > > gcc don't allow any modifier 'l' with asm stmt but it allows with asm goto. > Is that something you are look for? Thanks. > > So I add code in AST/Stmt.cpp to emit error. > ..... > return diag::err_asm_invalid_escape; > } else if (!this->isGCCAsmGoto() && EscapedChar == 'l' && > isDigit(*CurPtr)) { > DiagOffs = CurPtr-StrStart; > return diag::err_asm_invalid_operand_number; > } > For the first one, I was trying with Aarch64 gcc; I guess x86 doesn't emit the same error? `void f() { x: asm goto ("jne %i0"::::x);}` should be the same for both. > gcc don't allow any modifier 'l' with asm stmt but it allows with asm goto. > Is that something you are look for? Thanks. We should reject any use of the "l" modifier that does not point to a label in the label list. So we should also reject `void f(){x:asm goto ("jne %l0"::"r"(&&x)::x);}`. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D56571/new/ https://reviews.llvm.org/D56571 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits