efriedma added inline comments.
================
Comment at: include/clang/Basic/DiagnosticASTKinds.td:214
+ def err_asm_invalid_operand_number_for_goto_labels : Error <
+ "invalid operand number which isn't point to a goto label in asm string">;
}
----------------
Grammar.
Also, "invalid operand number" isn't really a good explanation. Maybe something
like "operand with 'l' modifier must refer to a label".
================
Comment at: lib/Sema/SemaStmtAsm.cpp:470
+ if (NS->isGCCAsmGoto() &&
+ Exprs[ConstraintIdx]->getStmtClass() == Stmt::AddrLabelExprClass)
+ break;
----------------
jyu2 wrote:
> efriedma wrote:
> > 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);}`.
> Hi Eli,
>
> Thank you so much to point this out. I add code for emit error for use of
> the "l" modifier that does not point to a label in the label list.
>
>
Please also add the `void f() { x: asm goto ("jne %i0"::::x);}` testcase.
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
