<div class="gmail_quote">On Mon, Feb 27, 2012 at 11:11 PM, 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">
Thanks for the quick review, and sorry for the somewhat slow follow-up :-)<br>
<div class="im"><br>
On Tue, Feb 21, 2012 at 8:07 PM, Richard Smith <<a href="mailto:richard@metafoo.co.uk">richard@metafoo.co.uk</a>> wrote:<br>
> Hi Nico,<br>
><br>
> On Tue, Feb 21, 2012 at 7:17 PM, Nico Weber <<a href="mailto:thakis@chromium.org">thakis@chromium.org</a>> 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 look 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>
>> <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>
>> (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>
</div>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?</blockquote><div><br></div><div>Sounds reasonable.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="im">
> Have you considered<br>
> relaxing the warning in cases where the integral summand is a constant<br>
> expression and is in-bounds? Would this pattern have matched any of your<br>
> real bugs?<br>
<br>
</div>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?</blockquote><div><br></div><div>I could imagine code legitimately wanting to do something like this:</div><div><br></div><div>template</*...*/></div>
<div>struct S {</div><div> enum { Offset = /* ... */ };</div><div> static const char *str = "some string" + Offset;</div><div>};</div><div><br></div><div>That said, I'm happy for us to warn on such code. Whatever you prefer seems fine to me; we can refine this later, if a need arises.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><div class="h5">
> 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 convinced<br>
> this should go into -Wmost. I imagine we'll know more once we've run 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>
</div></div>Done.<br>
<div class="im"><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 suggestion to<br>
> be provided via fixits (and to reword this as something like "use array<br>
> indexing to silence this warning").<br>
<br>
</div>Clang tends to suggest very explicit parentheses (see<br>
-Wparentheses-logical-op for example).</blockquote><div><br></div><div>Usually only in cases where there is a potential bug. If we saw &"foo"[2], we wouldn't be likely to suspect that the user might have really meant (&"foo")[2], so my (weak) preference is to suggest the simpler fix.</div>
<div><br></div><div>(Incidentally, -Warray-bounds should be taught to complain about (&"foo")[2]!)</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
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>
&( )[ ]</blockquote><div><br></div><div>This looks nice ... though if we're going to have parens in the fixit, they should contain the [] :-)</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="im">
> 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 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 for<br>
> class types with implicit conversion to int. Doing so will require moving<br>
> this check to checkArithmeticOpPointerOperand (after we've checked for an<br>
> overloaded operator+).<br>
<br>
</div>Added a check for (unscoped) enumeration types. class types with<br>
implicit conversion to int feel like overkill to me.</blockquote><div><br></div><div>OK. You should still move the check to after we check for an overloaded operator+, since operator+ can be overloaded for enums. (Once that's done, you may find it's easier to warn on such implicitly-convertible types than to not do so.)</div>
<div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="im">
><br>
> + if (!IsStringPlusInt)<br>
> + return;<br>
> +<br>
> + SourceRange DiagRange(LHSExpr->getLocStart(), 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 skip the<br>
> fixit in that case).<br>
<br>
</div>I tried that. It found neither additional bugs nor additional false<br>
positives, so it's probably not worth it?<br></blockquote></div><div><br></div><div>Fine, there's no point adding this if it doesn't actually catch any bugs. I have only empirical evidence that it might -- I once saw[*] the moral equivalent of:</div>
<div><br></div><div> int n = /*...*/;</div><div> std::string s = n + " widgets found";</div><div><br></div><div>[*] a long time ago, in a codebase far away...</div>