jyu2 marked an inline comment as done. jyu2 added inline comments.
================ Comment at: lib/AST/Stmt.cpp:628 + DiagOffs = CurPtr-StrStart-1; + return diag::err_asm_invalid_operand_for_goto_labels; + } ---------------- 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 CHANGES SINCE LAST ACTION https://reviews.llvm.org/D56571/new/ https://reviews.llvm.org/D56571 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
