[cfe-commits] r166655 - in /cfe/trunk: include/clang/Basic/DiagnosticSemaKinds.td include/clang/Basic/TargetInfo.h lib/Basic/Targets.cpp lib/Sema/SemaStmtAsm.cpp test/Misc/warning-flags.c

[John McCall]

On Oct 24, 2012, at 10:47 PM, Bill Wendling wrote:
> On Oct 24, 2012, at 7:37 PM, John McCall <rjmccall at apple.com> wrote:
>> On Oct 24, 2012, at 7:06 PM, Bill Wendling wrote:
>>> Yeah. I realize the text of the warning wasn't super. I'm rethinking the whole patch though. As it turns out, people write bad ASM all the time, like
>>> 
>>> asm("foo %0", "=r" :);
>>> 
>>> I don't know if it's profitable to warn in this situation or the like. 
>> 
>> We actually reject that as syntactically ill-formed.  :)
>> 
> As we should. But it's used a lot, for instance in our tests: tools/clang/test/CodeGen/mult-alt-x86.c. :-)

I don't mean that the assembly is ill-formed, I mean that the asm statement itself is ill-formed.  It's got commas where it's supposed to have colons, and it's totally missing an argument expression.  That made it somewhat challenging to decide what it was an example of. :)

>> I'm not totally sure what your example is getting at.  Trying to guess, if you mean that people use asm constraints that aren't consistent with how the assembly is used — e.g. if they use an =r constraint and then obviously rely on the existing value in that register in the assembly — then by all means warn about that.  It needs to be under a warning flag, of course, so that people can suppress it if they're really sure they know what they're doing;  and of course it's going to be even more false-positive-prone than a normal warning, because assembly is not semantically rich, so you'd need to watch out for (say) idioms that technically read the value but actually don't depend on it in any way (like xor %0, %0).  But if you're seriously interested in putting the time into making a high-quality warning there, I think that could be very valuable for users who do rely on inline asm.
>> 
> That's basically what I'm getting at, yes. Why I reverted the patch was because it needs to be thought through a bit more before I and I assume others are happy with it. :)

Sure.  FWIW, I think your original warning should be a lot simpler to make satisfactory than the inadequate-constraint one that your later example's about.

Regardless, you'll need a warning group, and you probably ought to make two — one for this specific warning (-Wasm-operand-widths?), and an umbrella group for all warnings about asm statements (-Wasm?)

John.