[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/AST/Stmt.cpp:628
+          DiagOffs = CurPtr-StrStart-1;
+          return diag::err_asm_invalid_operand_for_goto_labels;
+        }
----------------
jyu2 wrote:
> rsmith wrote:
> > jyu2 wrote:
> > > rsmith wrote:
> > > > I'm curious why we're checking this here, when all the other validation for the modifier letter happens in LLVM. Does this check need to be done differently from the others?
> > > This is to verify following in spec:
> > > =====
> > > To reference a label in the assembler template, prefix it with ‘%l’ (lowercase ‘L’) followed by its (zero-based) position in GotoLabels plus the number of input operands. For example, if the asm has three inputs and references two labels, refer to the first label as ‘%l3’ and the second as ‘%l4’).
> > > 
> > > Was request from Eli.
> > Sorry, I think my question was unclear. I understand why you need to do this check somewhere. My question is why this check needs to be *here*, given that all the other checks for the modifier letter are performed elsewhere. Splitting the checks up between multiple locations makes the system harder to maintain.
> > 
> > As it happens, there's a FIXME for this here: http://llvm.org/doxygen/AsmPrinterInlineAsm_8cpp_source.html#l00436 -- and a test case like `void f(void *a) { asm("mov %l0, %%eax" :: "r"(a)); }` currently causes LLVM to assert in a function that's supposed to be validating these modifier letters and producing a clean error if they're bad. So I think that's an LLVM bug, and fixing that LLVM bug would enforce the property we need here (that `%lN` only names labels).
> > 
> > 
> > [It's not clear to me whether we should be enforcing that `%lN` only names label //operands//, which would require the check to be in Clang rather than LLVM; GCC doesn't do that, and for instance accepts
> > 
> > ```
> > void f() { asm goto("jmp %l0" :: "i"(&&foo)::foo); foo:; }
> > ```
> > 
> > (at least for x86_64), though this construct doesn't seem hugely useful given that the input operand would need to be a literal address-of-label expression and you'd need to name the target label as a label operand anyway.]
> Hi Richard,
> 
> Thanks for the detail explanation. 
> I agree with you.  Our compiler FE don't emit error for this also.
> 
> I'd happy to remove this, If we all agree.  
> 
> Hi Eli, 
> what do you think?
> 
> Thanks.
> Jennifer  
> 
Yes, that's okay.


================
Comment at: lib/Analysis/CFG.cpp:3137
+
+  Block = createBlock(false);
+  Block->setTerminator(G);
----------------
efriedma wrote:
> Passing add_successor=false seems suspect; this could probably use more test coverage.
Did you ever look at this?


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

https://reviews.llvm.org/D56571