On Sat, Dec 21, 2013 at 8:55 PM, Nico Weber <[email protected]> wrote:
> Thanks for taking a look! > > On Sat, Dec 21, 2013 at 2:26 PM, Alp Toker <[email protected]> wrote: > >> On 21/12/2013 07:31, Nico Weber wrote: >> >>> Hi, >>> >>> the attached patch adds a new warning that makes memcmp & co warn on >>> misplaced parentheses such as >>> >>> if (memcmp(a, b, sizeof(a) != 0)) >>> >>> like so: >>> >>> test4.cc:5:30: warning: size argument in 'memcmp' call is a comparison >>> [-Wmemsize-comparison] >>> if (memcmp(a, b, sizeof(a) != 0)) >>> ~~~~~~~~~~^~~~ >>> >> >> >> Hi Nico, >> >> Agree we'd do well to diagnose here, but it seems can get a lot more >> mileage by simplifying and generalizing the check. >> >> This doesn't appear to be a memsize-specific problem, nor is it >> necessarily limited to binop argument expressions. >> >> 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? > > > 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? > > I think a more promising generalization is to do this for the last > parameter of calls whose last parameter type is unsigned. > 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 http://llvm.org/bugs/show_bug.cgi?id=18297 , comments 6 and later. 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: http://llvm.org/bugs/attachment.cgi?id=11839 > > 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, but I feel this is a good start". > > >> >> >> >> test4.cc:5:7: note: did you mean to compare the result of 'memcmp' >>> instead? >>> if (memcmp(a, b, sizeof(a) != 0)) >>> ^ ~ >>> ) >>> test4.cc:5:20: note: explicitly cast the argument to size_t to silence >>> this warning >>> if (memcmp(a, b, sizeof(a) != 0)) >>> ^ >>> (size_t)( ) >>> >> >> >> 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. >> > > 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.) > > >> 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. >> > > 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 > http://clang.llvm.org/docs/InternalsManual.html#fix-it-hints). > > Nico > > >> >> Alp. >> >> >> >> 1 warning generated. >>> >>> >>> 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. >>> >>> The idea of triggering this warning on a binop in the size argument is >>> due to rnk. >>> >>> This warning can possibly be extended later on, but I feel this is a >>> good start. >>> >>> Thoughts? >>> >>> Nico >>> >>> [1]: https://codereview.chromium.org/99423002/diff/1/net/third_ >>> party/nss/ssl/ssl3con.c >>> [2]: https://codereview.chromium.org/8431007/#msg12 >>> >>> >>> _______________________________________________ >>> cfe-commits mailing list >>> [email protected] >>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits >>> >> >> -- >> http://www.nuanti.com >> the browser experts >> >> >
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
