Thanks for the quick review, and sorry for the somewhat slow follow-up :-) On Tue, Feb 21, 2012 at 8:07 PM, Richard Smith <[email protected]> wrote: > Hi Nico, > > On Tue, Feb 21, 2012 at 7:17 PM, Nico Weber <[email protected]> wrote: >> >> the attached patch adds -Wstring-plus-int (I'm open to more creative >> names :-) ), which warns on code like |"error code: " + err|. The >> warning found 4 bugs in 2 files in chromium. The bugs it found look >> like this: >> >> File 1: >> if (!read_node.InitByIdLookup(it->id)) { >> error_handler()->OnUnrecoverableError(FROM_HERE, "Failed to look up " >> " data for received change with id " + it->id); >> return; >> } >> >> File 2: >> ResetStream(stream_id, spdy::INVALID_ASSOCIATED_STREAM, >> "Received OnSyn with inactive associated stream " + >> associated_stream_id); >> (Fix: >> http://codereview.chromium.org/9372076/diff/1/net/spdy/spdy_session.cc ) >> >> (A coworker found the bug in the one file, which prompted me writing >> the warning. The warning then found the bug in the other file.) >> >> >> When building all of chromium and its many dependencies, the warning >> did also find 3 false positives, but they all look alike: Ffmepg >> contains three different functions that all look like this: > > > 4 bugs and 3 false positives doesn't sound great to me.
True. All of the the 3 false positives are really the same snippet
though. (On the other hand, 3 of the 4 bugs are very similar as well.)
The bugs this did find are crash bugs, and they were in rarely run
error-logging code, so it does find interesting bugs. Maybe you guys
can run it on your codebase after it's in, and we can move it out of
-Wmost if you consider it too noisy in practice?
> Have you considered
> relaxing the warning in cases where the integral summand is a constant
> expression and is in-bounds? Would this pattern have matched any of your
> real bugs?
That's a good idea (for my bugs, the int was always a declrefexpr,
never a literal). The ffmpeg example roughly looks like this:
#define A "foo"
#define B "bar"
consume(A B + sizeof(A) - 1);
The RHS is just "sizeof(A)" without the "- 1", but since A B has a
length of 6, this still makes this warning go away. I added this to
the patch. With this change, it's 4 bugs / 0 false positives.
Note that this suppresses the warning for most enums which start at 0.
Or did you mean to do this only for non-enum constant expressions?
> Onto the patch...
>
> Index: include/clang/Basic/DiagnosticGroups.td
> ===================================================================
> --- include/clang/Basic/DiagnosticGroups.td (revision 150418)
> +++ include/clang/Basic/DiagnosticGroups.td (working copy)
> @@ -320,6 +321,7 @@
> ReturnType,
> SelfAssignment,
> SizeofArrayArgument,
> + StringPlusInt,
> Trigraphs,
> Uninitialized,
> UnknownPragmas,
>
> Given the current level of false positives, I'm not completely convinced
> this should go into -Wmost. I imagine we'll know more once we've run this on
> more code.
>
> Index: include/clang/Basic/DiagnosticSemaKinds.td
> ===================================================================
> --- include/clang/Basic/DiagnosticSemaKinds.td (revision 150418)
> +++ include/clang/Basic/DiagnosticSemaKinds.td (working copy)
> @@ -3414,6 +3414,12 @@
> "explicitly assigning a variable of type %0 to itself">,
> InGroup<SelfAssignment>, DefaultIgnore;
>
> +def warn_string_plus_int : Warning<
> + "Adding an integral to a string does not append to the string">,
>
> Diagnostics outside the static analyzer should not be capitalized.
Done.
> + InGroup<StringPlusInt>;
> +def note_string_plus_int_silence : Note<
> + "use &(str[index]) if pointer arithmetic is intended">;
>
> The parentheses here are redundant. I think I'd prefer for the suggestion to
> be provided via fixits (and to reword this as something like "use array
> indexing to silence this warning").
Clang tends to suggest very explicit parentheses (see
-Wparentheses-logical-op for example). I did change it to a fixit and
reworded, it now looks like so:
test/SemaCXX/string-plus-int.cpp:12:17: warning: adding an integral to
a string does not append to the string [-Wstring-plus-int]
consume("foo" + 5);
~~~~~~^~~
test/SemaCXX/string-plus-int.cpp:12:17: note: use array indexing to
silence this warning
consume("foo" + 5);
^
&( )[ ]
> Index: lib/Sema/SemaExpr.cpp
> ===================================================================
> --- lib/Sema/SemaExpr.cpp (revision 150418)
> +++ lib/Sema/SemaExpr.cpp (working copy)
> @@ -7798,6 +7798,20 @@
> ParensRange);
> }
>
> +/// DiagnoseStringPlusInt - Emit a warning when adding an integer to a
> string
> +/// literal.
> +static void DiagnoseStringPlusInt(Sema &Self, SourceLocation OpLoc,
> + Expr *LHSExpr, Expr *RHSExpr) {
> + bool IsStringPlusInt = dyn_cast<StringLiteral>(LHSExpr) &&
> + RHSExpr->getType()->isIntegralType(Self.getASTContext());
>
> We should probably also check for (unscoped) enumeration types, and for
> class types with implicit conversion to int. Doing so will require moving
> this check to checkArithmeticOpPointerOperand (after we've checked for an
> overloaded operator+).
Added a check for (unscoped) enumeration types. class types with
implicit conversion to int feel like overkill to me.
>
> + if (!IsStringPlusInt)
> + return;
> +
> + SourceRange DiagRange(LHSExpr->getLocStart(), RHSExpr->getLocEnd());
> + Self.Diag(OpLoc, diag::warn_string_plus_int) << DiagRange;
> + Self.Diag(OpLoc, diag::note_string_plus_int_silence);
> +}
>
> We should also catch the case where the integer is on the LHS (and skip the
> fixit in that case).
I tried that. It found neither additional bugs nor additional false
positives, so it's probably not worth it?
Nico
clang-str-plus-int-v2.patch
Description: Binary data
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
