[PATCH] D56571: [RFC prototype] Implementation of asm-goto support in clang

[Eli Friedman via Phabricator via cfe-commits]

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