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

[Jennifer Yu via Phabricator via cfe-commits]

jyu2 marked an inline comment as done.
jyu2 added inline comments.


================
Comment at: lib/Sema/SemaStmtAsm.cpp:470
+    if (NS->isGCCAsmGoto() &&
+        Exprs[ConstraintIdx]->getStmtClass() == Stmt::AddrLabelExprClass)
+      break;
----------------
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;
      }



CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56571/new/

https://reviews.llvm.org/D56571