[PATCH] D156312: [analyzer] Upstream BitwiseShiftChecker

[Balázs Benics via Phabricator via cfe-commits]

steakhal accepted this revision.
steakhal added a comment.

In D156312#4588406 <https://reviews.llvm.org/D156312#4588406>, @donat.nagy wrote:

> After investigating this issue, I added the testcases `signed_aritmetic_{good,bad}` which document the current sub-optimal state. The root cause of this problem is a high-level property of the engine (that it assumes that signed overflows are always possible and acceptable) and I don't see a local workaround that would silence or fix these incorrect error messages.
>
> @steakhal @NoQ What do you know about these signed overflow issues (I presume that analogous issues also appear in other checkers)? How should we handle this limitation of this checker?

I don't think most checkers depend on value ranges explicitly, except for a handful of checkers maybe. So, I don't think it's such a huge deal, but I agree that it's a problem.
I would bet that the StdLibraryFunctionsChecker would suffer from the same issue, and the OOB checker(s) along with it.
I don't know of heuristics mitigating the damage.

I don't think we should do anything about it unless it's frequent enough.
Try to come up with a heuristic to be able to measure how often this happens, if you really care.
Once you have a semi-working heuristic for determining if that bugpath suffers from this, you can as well use it for marking the bugreport invalid if that's the case to lean towards suppressing those FPs.

I don't think it's completely necessary to fix those FPs to land this. I think of that as an improvement, on top of this one.
I hope I clarified my standpoint.



================
Comment at: clang/test/Analysis/bitwise-shift-sanity-checks.c:70-74
+  if (right < 0)
+    return 0;
+  return left << (right + 32);
+  // expected - warning at -1 {{Left shift overflows the capacity of 'int'}}
+  // expected-warning at -2 {{Right operand is negative in left shift}}
----------------
Let's be explicit about the actual assumed value range, and use `clang_analyzer_value()`.
I also recommend using an explicit FIXME for calling out what should be there, instead of abusing a non-matching `expected` pattern. I know I used that in the past, and probably seen it from me. I feel ashamed that I did that. I know I did that to have cleaner diffs, once fixed, but I honestly think it does more harm than good.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D156312/new/

https://reviews.llvm.org/D156312