<div dir="ltr">On Sat, Dec 21, 2013 at 8:55 PM, Nico Weber <span dir="ltr"><<a href="mailto:thakis@chromium.org" target="_blank">thakis@chromium.org</a>></span> wrote:<br><div class="gmail_extra"><div class="gmail_quote">
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">
Thanks for taking a look!</div><div class="gmail_quote"><br></div><div class="gmail_quote"><div class="im">On Sat, Dec 21, 2013 at 2:26 PM, Alp Toker <span dir="ltr"><<a href="mailto:alp@nuanti.com" target="_blank">alp@nuanti.com</a>></span> wrote:<br>

<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div>On 21/12/2013 07:31, Nico Weber wrote:<br>

<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
Hi,<br>
<br>
the attached patch adds a new warning that makes memcmp & co warn on misplaced parentheses such as<br>
<br>
  if (memcmp(a, b, sizeof(a) != 0))<br>
<br>
like so:<br>
<br>
test4.cc:5:30: warning: size argument in 'memcmp' call is a comparison [-Wmemsize-comparison]<br>
  if (memcmp(a, b, sizeof(a) != 0))<br>
                   ~~~~~~~~~~^~~~<br>
</blockquote>
<br>
<br></div>
Hi Nico,<br>
<br>
Agree we'd do well to diagnose here, but it seems can get a lot more mileage by simplifying and generalizing the check.<br>
<br>
This doesn't appear to be a memsize-specific problem, nor is it necessarily limited to binop argument expressions.<br>
<br>
Surely it's rather the case that any implicit bool-to-size_t call argument conversion is at best dubious, and most likely a bug?</blockquote><div><br></div></div><div>I think it's not obvious which direction the generalization should go: You suggest implicit bool-to-size_t – there's a much higher chance for false positives there. If experiments show that this is in fact viable, should this be done for bool-to-unsigned? What about bool-to-int – we already know that this has a noise level that's too high to be bearable. Would you do it everywhere, or only in calls?</div>

<div><br></div><div>I think a more promising generalization is to do this for the last parameter of calls whose last parameter type is unsigned.</div></div></div></div></blockquote><div><br></div><div>I did experiment with this extension a bit. It finds another 1.5 bugs in chromium (a real bug in NSS, and somewhat questionable code in chromium itself), with 0.5 false positives (it's actually 0, but in 1 case it's fairly easy to accidentally enable the warning) – see <a href="http://llvm.org/bugs/show_bug.cgi?id=18297">http://llvm.org/bugs/show_bug.cgi?id=18297</a> , comments 6 and later.</div>
<div><br></div><div>Does someone feel like running this expanded warning on another large codebase, to get a better idea how useful it is? A prototype patch that's good enough for evaluating the warning is attached to the bug: <a href="http://llvm.org/bugs/attachment.cgi?id=11839">http://llvm.org/bugs/attachment.cgi?id=11839</a></div>
<div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">
<div><br></div><div>But since it's not clear, I figured this is a generally agreeable and useful subset, so I wanted to get this in first; this is what I meant with "this can possibly be extended later on<span style="font-family:arial,sans-serif;font-size:13px">, but I feel this is a good start".</span></div>
<div class="im">
<div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div><br>
<br>
<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
test4.cc:5:7: note: did you mean to compare the result of 'memcmp' instead?<br>
  if (memcmp(a, b, sizeof(a) != 0))<br>
      ^                          ~<br>
                            )<br>
test4.cc:5:20: note: explicitly cast the argument to size_t to silence this warning<br>
  if (memcmp(a, b, sizeof(a) != 0))<br>
                   ^<br>
                   (size_t)(     )<br>
</blockquote>
<br>
<br></div>
A cast is the wrong FixIt to suggest. "sizeof(a) ? 1 : 0" is closer to the natural way to write this construct if it was indeed their true intention.<br></blockquote><div><br></div></div><div>This is mostly a style thing, no? Is there any technical reason to prefer your suggestion? I went with a cast since several other warnings have a "explicitly cast to suppress" note, and I liked the consistency in diagnostics. (?1:0 will suppress the warning too, so people can use that if they prefer this.)</div>
<div class="im">
<div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">However it's best not to generate a FixIt in this context given that we have a high degree of confidence that the user's code is bogus, especially since the FixIt hides away the bug into a form that's harder to detect.<br>

</blockquote><div><br></div></div><div>Note that it's a fixit on a note, not on the diagnostic itself, so it's never applied automatically. We have several diagnostics with fixits on the "to suppress" note. (See also the bit on fixits on <a href="http://clang.llvm.org/docs/InternalsManual.html#fix-it-hints" target="_blank">http://clang.llvm.org/docs/InternalsManual.html#fix-it-hints</a>).</div>
<span class=""><font color="#888888">
<div><br></div><div>Nico</div></font></span><div class="im"><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">

<br>
Alp.<br>
<br>
<br>
<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div>
1 warning generated.<br>
<br>
<br>
This would have found one bug in NSS that we recently fixed [1] and found one more bug in chromium we didn't know about before [2]. It had 0 false positives on all of chromium.<br>
<br>
The idea of triggering this warning on a binop in the size argument is due to rnk.<br>
<br>
This warning can possibly be extended later on, but I feel this is a good start.<br>
<br>
Thoughts?<br>
<br>
Nico<br>
<br>
[1]: <a href="https://codereview.chromium.org/99423002/diff/1/net/third_party/nss/ssl/ssl3con.c" target="_blank">https://codereview.chromium.<u></u>org/99423002/diff/1/net/third_<u></u>party/nss/ssl/ssl3con.c</a><br>
[2]: <a href="https://codereview.chromium.org/8431007/#msg12" target="_blank">https://codereview.chromium.<u></u>org/8431007/#msg12</a><br>
<br>
<br></div>
______________________________<u></u>_________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@cs.uiuc.edu" target="_blank">cfe-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank">http://lists.cs.uiuc.edu/<u></u>mailman/listinfo/cfe-commits</a><span><font color="#888888"><br>
</font></span></blockquote><span><font color="#888888">
<br>
-- <br>
<a href="http://www.nuanti.com" target="_blank">http://www.nuanti.com</a><br>
the browser experts<br>
<br>
</font></span></blockquote></div></div><br></div></div>
</blockquote></div><br></div></div>