rsmith 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: > > 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.] 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
