[cfe-commits] [patch] Warn in string literal + integral

[Richard Smith]

Hi Nico,

On Tue, Feb 21, 2012 at 7:17 PM, Nico Weber <thakis at chromium.org> 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. 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?

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.

+  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").

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+).

+  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).

Richard
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120221/b7419492/attachment.html>