[PATCH] Add a truncation warning for values under a bitwise or

Sam Panzer espanz at gmail.com
Thu Mar 14 18:15:38 PDT 2013



================
Comment at: lib/Sema/SemaChecking.cpp:4822-4824
@@ +4821,5 @@
+    case BO_AndAssign:
+      return (EvalAsInt(Ctx, Value, BO->getLHS(), NarrowedExpr) ||
+              EvalAsInt(Ctx, Value, BO->getRHS(), NarrowedExpr)) &&
+             TruncationChangesValue(Value, TargetRange, true);
+
----------------
Richard Smith wrote:
> What happens if both LHS and RHS can be evaluated, and only RHS implicitly narrows?
Good call. Diagnostic is now issued for this and the next set of cases.

================
Comment at: lib/Sema/SemaChecking.cpp:4828-4830
@@ +4827,5 @@
+    case BO_OrAssign:
+    case BO_Add:
+    case BO_AddAssign:
+    case BO_Sub:
+    case BO_SubAssign:
----------------
Richard Smith wrote:
> The BO_Add and BO_Sub cases might have false positives, for instance:
> '''
> unsigned char X = 0x1000 - Y;
> '''
> is fine if Y is known to be between 0x0f01 and 0x1000.
> 
> I think the cases we can warn on here are BO_AddAssign, BO_SubAssign, and unsigned BO_Add. (Though even unsigned add may have false positives if it's intended to wrap.) Maybe just drop the non-Assign Add and Sub checks for now?
I disabled those checks, then realized that BO_Xor might cause some false positives too.

```char c = 0x1ff ^ Y;```

is also fine if Y is between 0x100 and 0x1ff. It's even worse with something like

```char c = 0x0f00 ^ Y```

where Y is between 0xf080 and 0xf0ff, since the second byte of Y is known to start with a 1. I //think// that truncation leaves the result the same for signed inputs, since the result should be between -1 and -256.

Should I also remove the non-Assign Xor check for now?



================
Comment at: lib/Sema/SemaChecking.cpp:5057-5058
@@ -4981,5 +5056,4 @@
 
-    // People want to build with -Wshorten-64-to-32 and not -Wconversion.
-    if (S.SourceMgr.isInSystemMacro(CC))
-      return;
+        if (S.SourceMgr.isInSystemMacro(CC))
+          return;
 
----------------
Richard Smith wrote:
> This same check occurs both within the 'if' and after it; please move it before 'EvaluateAsInt'.
Done.


http://llvm-reviews.chandlerc.com/D405



More information about the cfe-commits mailing list