<div class="gmail_quote">On Fri, Mar 2, 2012 at 9:53 AM, Nico Weber <span dir="ltr"><<a href="mailto:thakis@chromium.org">thakis@chromium.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="HOEnZb"><div class="h5">On Thu, Mar 1, 2012 at 11:12 PM, Richard Smith <<a href="mailto:richard@metafoo.co.uk">richard@metafoo.co.uk</a>> wrote:<br>
> On Thu, Mar 1, 2012 at 9:26 PM, Nico Weber <<a href="mailto:thakis@chromium.org">thakis@chromium.org</a>> wrote:<br>
>><br>
>> >> > On Tue, Feb 21, 2012 at 7:17 PM, Nico Weber <<a href="mailto:thakis@chromium.org">thakis@chromium.org</a>><br>
>> >> > wrote:<br>
>> >> >><br>
>> >> >> the attached patch adds -Wstring-plus-int (I'm open to more creative<br>
>> >> >> names :-) ), which warns on code like |"error code: " + err|. The<br>
>> >> >> warning found 4 bugs in 2 files in chromium. The bugs it found look<br>
>> >> >> like this:<br>
>> >> >><br>
>> >> >> File 1:<br>
>> >> >> if (!read_node.InitByIdLookup(it->id)) {<br>
>> >> >> error_handler()->OnUnrecoverableError(FROM_HERE, "Failed to<br>
>> >> >> look<br>
>> >> >> up "<br>
>> >> >> " data for received change with id " + it->id);<br>
>> >> >> return;<br>
>> >> >> }<br>
>> >> >><br>
>> >> >> File 2:<br>
>> >> >> ResetStream(stream_id, spdy::INVALID_ASSOCIATED_STREAM,<br>
>> >> >> "Received OnSyn with inactive associated stream " +<br>
>> >> >> associated_stream_id);<br>
>> >> >> (Fix:<br>
>> >> >><br>
>> >> >> <a href="http://codereview.chromium.org/9372076/diff/1/net/spdy/spdy_session.cc" target="_blank">http://codereview.chromium.org/9372076/diff/1/net/spdy/spdy_session.cc</a><br>
>> >> >> )<br>
>> >> >><br>
>> >> >> (A coworker found the bug in the one file, which prompted me writing<br>
>> >> >> the warning. The warning then found the bug in the other file.)<br>
>> >> >><br>
>> >> >><br>
>> >> >> When building all of chromium and its many dependencies, the warning<br>
>> >> >> did also find 3 false positives, but they all look alike: Ffmepg<br>
>> >> >> contains three different functions that all look like this:<br>
>> >> ><br>
>> >> ><br>
>> >> > 4 bugs and 3 false positives doesn't sound great to me.<br>
>> >><br>
>> >> True. All of the the 3 false positives are really the same snippet<br>
>> >> though. (On the other hand, 3 of the 4 bugs are very similar as well.)<br>
>> >> The bugs this did find are crash bugs, and they were in rarely run<br>
>> >> error-logging code, so it does find interesting bugs. Maybe you guys<br>
>> >> can run it on your codebase after it's in, and we can move it out of<br>
>> >> -Wmost if you consider it too noisy in practice?<br>
>> ><br>
>> ><br>
>> > Sounds reasonable.<br>
>> ><br>
>> >><br>
>> >> > Have you considered<br>
>> >> > relaxing the warning in cases where the integral summand is a<br>
>> >> > constant<br>
>> >> > expression and is in-bounds? Would this pattern have matched any of<br>
>> >> > your<br>
>> >> > real bugs?<br>
>> >><br>
>> >> That's a good idea (for my bugs, the int was always a declrefexpr,<br>
>> >> never a literal). The ffmpeg example roughly looks like this:<br>
>> >><br>
>> >> #define A "foo"<br>
>> >> #define B "bar"<br>
>> >> consume(A B + sizeof(A) - 1);<br>
>> >><br>
>> >> The RHS is just "sizeof(A)" without the "- 1", but since A B has a<br>
>> >> length of 6, this still makes this warning go away. I added this to<br>
>> >> the patch. With this change, it's 4 bugs / 0 false positives.<br>
>> >><br>
>> >> Note that this suppresses the warning for most enums which start at 0.<br>
>> >> Or did you mean to do this only for non-enum constant expressions?<br>
>> ><br>
>> ><br>
>> > I could imagine code legitimately wanting to do something like this:<br>
>> ><br>
>> > template</*...*/><br>
>> > struct S {<br>
>> > enum { Offset = /* ... */ };<br>
>> > static const char *str = "some string" + Offset;<br>
>> > };<br>
>> ><br>
>> > That said, I'm happy for us to warn on such code. Whatever you prefer<br>
>> > seems<br>
>> > fine to me; we can refine this later, if a need arises.<br>
>><br>
>> Enums are handled like ints: They warn if the offset is out of bounds,<br>
>> but don't if it's they are in bounds.<br>
>><br>
>> >> > Onto the patch...<br>
>> >> ><br>
>> >> > Index: include/clang/Basic/DiagnosticGroups.td<br>
>> >> > ===================================================================<br>
>> >> > --- include/clang/Basic/DiagnosticGroups.td (revision 150418)<br>
>> >> > +++ include/clang/Basic/DiagnosticGroups.td (working copy)<br>
>> >> > @@ -320,6 +321,7 @@<br>
>> >> > ReturnType,<br>
>> >> > SelfAssignment,<br>
>> >> > SizeofArrayArgument,<br>
>> >> > + StringPlusInt,<br>
>> >> > Trigraphs,<br>
>> >> > Uninitialized,<br>
>> >> > UnknownPragmas,<br>
>> >> ><br>
>> >> > Given the current level of false positives, I'm not completely<br>
>> >> > convinced<br>
>> >> > this should go into -Wmost. I imagine we'll know more once we've run<br>
>> >> > this on<br>
>> >> > more code.<br>
>> >> ><br>
>> >> > Index: include/clang/Basic/DiagnosticSemaKinds.td<br>
>> >> > ===================================================================<br>
>> >> > --- include/clang/Basic/DiagnosticSemaKinds.td (revision 150418)<br>
>> >> > +++ include/clang/Basic/DiagnosticSemaKinds.td (working copy)<br>
>> >> > @@ -3414,6 +3414,12 @@<br>
>> >> > "explicitly assigning a variable of type %0 to itself">,<br>
>> >> > InGroup<SelfAssignment>, DefaultIgnore;<br>
>> >> ><br>
>> >> > +def warn_string_plus_int : Warning<<br>
>> >> > + "Adding an integral to a string does not append to the string">,<br>
>> >> ><br>
>> >> > Diagnostics outside the static analyzer should not be capitalized.<br>
>> >><br>
>> >> Done.<br>
>> >><br>
>> >> > + InGroup<StringPlusInt>;<br>
>> >> > +def note_string_plus_int_silence : Note<<br>
>> >> > + "use &(str[index]) if pointer arithmetic is intended">;<br>
>> >> ><br>
>> >> > The parentheses here are redundant. I think I'd prefer for the<br>
>> >> > suggestion to<br>
>> >> > be provided via fixits (and to reword this as something like "use<br>
>> >> > array<br>
>> >> > indexing to silence this warning").<br>
>> >><br>
>> >> Clang tends to suggest very explicit parentheses (see<br>
>> >> -Wparentheses-logical-op for example).<br>
>> ><br>
>> ><br>
>> > Usually only in cases where there is a potential bug. If we saw<br>
>> > &"foo"[2],<br>
>> > we wouldn't be likely to suspect that the user might have really meant<br>
>> > (&"foo")[2], so my (weak) preference is to suggest the simpler fix.<br>
>><br>
>> Removed the parens.<br>
>><br>
>> > (Incidentally, -Warray-bounds should be taught to complain about<br>
>> > (&"foo")[2]!)<br>
>><br>
>> Patches accepted! :-D<br>
>><br>
>> >> I did change it to a fixit and<br>
>> >> reworded, it now looks like so:<br>
>> >><br>
>> >> test/SemaCXX/string-plus-int.cpp:12:17: warning: adding an integral to<br>
>> >> a string does not append to the string [-Wstring-plus-int]<br>
>> >> consume("foo" + 5);<br>
>> >> ~~~~~~^~~<br>
>> >> test/SemaCXX/string-plus-int.cpp:12:17: note: use array indexing to<br>
>> >> silence this warning<br>
>> >> consume("foo" + 5);<br>
>> >> ^<br>
>> >> &( )[ ]<br>
>> ><br>
>> ><br>
>> > This looks nice ... though if we're going to have parens in the fixit,<br>
>> > they<br>
>> > should contain the [] :-)<br>
>> ><br>
>> >><br>
>> >> > Index: lib/Sema/SemaExpr.cpp<br>
>> >> > ===================================================================<br>
>> >> > --- lib/Sema/SemaExpr.cpp (revision 150418)<br>
>> >> > +++ lib/Sema/SemaExpr.cpp (working copy)<br>
>> >> > @@ -7798,6 +7798,20 @@<br>
>> >> > ParensRange);<br>
>> >> > }<br>
>> >> ><br>
>> >> > +/// DiagnoseStringPlusInt - Emit a warning when adding an integer to<br>
>> >> > a<br>
>> >> > string<br>
>> >> > +/// literal.<br>
>> >> > +static void DiagnoseStringPlusInt(Sema &Self, SourceLocation OpLoc,<br>
>> >> > + Expr *LHSExpr, Expr *RHSExpr) {<br>
>> >> > + bool IsStringPlusInt = dyn_cast<StringLiteral>(LHSExpr) &&<br>
>> >> > + RHSExpr->getType()->isIntegralType(Self.getASTContext());<br>
>> >> ><br>
>> >> > We should probably also check for (unscoped) enumeration types, and<br>
>> >> > for<br>
>> >> > class types with implicit conversion to int. Doing so will require<br>
>> >> > moving<br>
>> >> > this check to checkArithmeticOpPointerOperand (after we've checked<br>
>> >> > for<br>
>> >> > an<br>
>> >> > overloaded operator+).<br>
>> >><br>
>> >> Added a check for (unscoped) enumeration types. class types with<br>
>> >> implicit conversion to int feel like overkill to me.<br>
>> ><br>
>> ><br>
>> > OK. You should still move the check to after we check for an overloaded<br>
>> > operator+, since operator+ can be overloaded for enums. (Once that's<br>
>> > done,<br>
>> > you may find it's easier to warn on such implicitly-convertible types<br>
>> > than<br>
>> > to not do so.)<br>
>><br>
>> Done, added an enum-overloaded operator+ to the test.<br>
>><br>
>> >> > + if (!IsStringPlusInt)<br>
>> >> > + return;<br>
>> >> > +<br>
>> >> > + SourceRange DiagRange(LHSExpr->getLocStart(),<br>
>> >> > RHSExpr->getLocEnd());<br>
>> >> > + Self.Diag(OpLoc, diag::warn_string_plus_int) << DiagRange;<br>
>> >> > + Self.Diag(OpLoc, diag::note_string_plus_int_silence);<br>
>> >> > +}<br>
>> >> ><br>
>> >> > We should also catch the case where the integer is on the LHS (and<br>
>> >> > skip<br>
>> >> > the<br>
>> >> > fixit in that case).<br>
>> >><br>
>> >> I tried that. It found neither additional bugs nor additional false<br>
>> >> positives, so it's probably not worth it?<br>
>> ><br>
>> ><br>
>> > Fine, there's no point adding this if it doesn't actually catch any<br>
>> > bugs. I<br>
>> > have only empirical evidence that it might -- I once saw[*] the moral<br>
>> > equivalent of:<br>
>> ><br>
>> > int n = /*...*/;<br>
>> > std::string s = n + " widgets found";<br>
>> ><br>
>> > [*] a long time ago, in a codebase far away...<br>
>><br>
>> Ok, I added the warning for the int on the left as well (without fixit<br>
>> in that case, like you suggested upthread).<br>
><br>
><br>
> Just a few more things:<br>
><br>
> +def warn_string_plus_int : Warning<<br>
> + "adding an integral to a string does not append to the string">,<br>
> + InGroup<StringPlusInt>;<br>
><br>
> Rather than "an integral", can we say something like:<br>
><br>
> adding %0 to a string literal does not append to the string<br>
><br>
> (where %0 is the type)?<br>
<br>
</div></div>Done.<br>
<div class="im"><br>
> + if (IndexExpr->EvaluateAsInt(index, Self.getASTContext())) {<br>
> + if (index.isNonNegative() &&<br>
> + index < llvm::APSInt(llvm::APInt(index.getBitWidth(),<br>
> + StrExpr->getByteLength()),<br>
><br>
> This should be <=, not <, I think: we want to allow "foo"+4 (a pointer past<br>
> the null) but warn on "foo"+5 (which has undefined behavior). And<br>
> getByteLength() should be getLength(), to properly handle wide string<br>
> literals. We'll get duplicate warnings for the same issue if<br>
> -Warray-bounds-pointer-arithmetic is enabled and we carry on here, but such<br>
> code is rare enough that I don't think that's a pressing concern.<br>
<br>
</div>Done (-ish: getLength() didn't include the trailing \0, so I'm doing<br>
<= getLength() + 1), added a test.</blockquote><div><br></div><div>Thanks, this looks fine to commit.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="im">
> Other than that, a test for the fix-it would be great (see test/FixIt for<br>
> examples).<br>
<br>
</div>Wouldn't that require the fixit to be on the warning instead of the<br>
note? (If so, maybe there's no need for the test yet?)<br></blockquote></div><br><div>You can test fixits on notes with -fdiagnostics-parseable-fixits + FileCheck. See test/FixIt/fixit-vexing-parse.cpp for an example of that. I'm happy for that to be a subsequent commit.</div>