[PATCH] D156312: [analyzer] Upstream BitwiseShiftChecker

[Donát Nagy via Phabricator via cfe-commits]

donat.nagy added a comment.

In D156312#4589251 <https://reviews.llvm.org/D156312#4589251>, @steakhal wrote:

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

Minor clarification: these are not FPs, these are true positives with a bad error message. I was annoyed when I found this surprising bug on the almost-ready checker that I was working on; but I wouldn't say that this is an especially severe issue.

In D156312#4589251 <https://reviews.llvm.org/D156312#4589251>, @steakhal wrote:

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

I agree, I'll create a new revision which mentions this checker in the release notes, and I hope that I'll be able to merge that. Later I'll try to return to this question with either an engine improvement or a heuristic mitigation.



================
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}}
----------------
steakhal wrote:
> 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.
I already tried calling `clang_analyzer_value()` at that point when I was investigating, but unfortunately it won't say anything useful because the actual range corresponding to the symbolic expression is only calculated when the `evalBinOp()` calls examine it.

I'll change the "expected - warning" to a FIXME.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156312