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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
