[PATCH] D156312: [analyzer] Upstream BitwiseShiftChecker

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

steakhal added a comment.

Great progress.
Now, moving to the docs.
Have you checked that the docs compile without errors and look as you would expect? Please post how it looks.

Please make a note about this new checker in the clang's release docs, as we usually announce new checkers and major changes there.



================
Comment at: clang/docs/analyzer/checkers.rst:58-64
+ int basic_examples(int a, int b) {
+   if (b < 0) {
+     return a >> b; // warn: right argument is negative
+   } else if (b >= 32) {
+     return a << b; // warn: right argument >= capacity
+   }
+ }
----------------
else after return


================
Comment at: clang/lib/StaticAnalyzer/Checkers/BitwiseShiftChecker.cpp:272
+  if (const auto ConcreteRight = Right.getAs<nonloc::ConcreteInt>()) {
+    const std::int64_t RHS = ConcreteRight->getValue().getExtValue();
+    assert(RHS > MaximalAllowedShift);
----------------
`getExtValue()`, I believe, asserts that it "fits", thus the concrete int is at most 64 bits wide. Does it work for `_BigInt`s? (or whatever we have like that :D)


================
Comment at: clang/lib/StaticAnalyzer/Checkers/BitwiseShiftChecker.cpp:299-303
+      if (Side == OperandSide::Left) {
+        NonNegOperands |= NonNegLeft;
+      } else {
+        NonNegOperands |= NonNegRight;
+      }
----------------
When I see something like this I always think of using a ternary.
Have you also considered?


================
Comment at: clang/lib/StaticAnalyzer/Checkers/BitwiseShiftChecker.cpp:335-339
+    auto R =
+        std::make_unique<PathSensitiveBugReport>(BT, ShortMsg, Msg, ErrNode);
+    bugreporter::trackExpressionValue(ErrNode, Op->getLHS(), *R);
+    bugreporter::trackExpressionValue(ErrNode, Op->getRHS(), *R);
+    return R;
----------------
AHA! Now I got you. You also call this `R`.
Just use `R` at the callsite as well. Job done.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/BitwiseShiftChecker.cpp:355-357
+    if (!BTPtr)
+      BTPtr = std::make_unique<BugType>(this, "Bitwise shift",
+                                        "Suspicious operation");
----------------
How about eagerly inline initializing this field? And avoiding `mutable` as well.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/BitwiseShiftChecker.cpp:367-369
+
+  const AnalyzerOptions &Opts = Mgr.getAnalyzerOptions();
+
----------------



================
Comment at: clang/lib/StaticAnalyzer/Checkers/BitwiseShiftChecker.cpp:53
+class BitwiseShiftValidator {
+  // Primary mutable state:
+  CheckerContext &Ctx;
----------------
donat.nagy wrote:
> steakhal wrote:
> > Where does this comment corresponds to?
> The two data members (`Ctx` and `State`) that are directly below the comment.
> 
> I tried to split the list of data members into three groups (primary mutable state, secondary mutable state, `const`  members), but now I reconsider this I feel that these header comments are actually superfluous.
> 
> I'll return to a variant of the earlier layout where I put `NonNegFlags`/`UpperBound` to the end and comment that they are only used for note tag creation; but don't emphasize the (immediately visible) `const`ness / non-`const`-ness of other members with comments.
One way to emphasize this is by choosing names like `FoldedState`, or anything else that has something to do with "motion" or "change".
Given that if we have `ProgramStateRefs` in helper classes they usually remain still. Think of BugReportVisitors for example. Consequently,it's important to raise awareness when it's not like that. A different name would achieve this.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/BitwiseShiftChecker.cpp:57-59
+  // Secondary mutable state, used only for note tag creation:
+  enum { NonNegLeft = 1, NonNegRight = 2 };
+  unsigned NonNegFlags = 0;
----------------
donat.nagy wrote:
> donat.nagy wrote:
> > steakhal wrote:
> > > How about using the same enum type for these two? It would signify that these are used together.
> > > Also, by only looking at the possible values, it's not apparent that these are bitflag values. A suffix might help the reader.
> > Using the enum type is a good idea, however I'd probably put the `Flag` suffix into the name of the type:
> > ```
> > enum SignednessFlags : unsigned { NonNegLeft = 1, NonNegRight = 2 };
> > SignednessFlags NonNegOperands = 0;
> > ```
> I rename the member `NonNegFlags` to `NonNegOperands` but its type remains a plain `unsigned` because otherwise using the or-assign operator like `NonNegOperands |= NonNegLeft` produces the error
> > Assigning to 'enum SignednessFlags' from incompatible type 'unsigned int' (clang typecheck_convert_incompatible)
> (note that both operands were from the same enum type).
Okay, indeed one would need to define the `operator|(SignednessStatus, SignednessStatus)` and `operato&|(SignednessStatus, SignednessStatus)` as well.
It probably doesn't worth it. Such a pity that we can't have nice things in C++.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/BitwiseShiftChecker.cpp:60-63
+  // If there is an upper bound, it is <= the size of a primitive type
+  // (expressed in bits), so this is a very safe sentinel value:
+  enum { NoUpperBound = 999 };
+  unsigned UpperBound = NoUpperBound;
----------------
donat.nagy wrote:
> steakhal wrote:
> > Have you considered using `std::optional`?
> > Given that the dimension of this variable is "bits", I think it would be great to have that in its name.
> I considered using `std::optional`, but using this "old-school" solution ensures that `NoUpperBound` is comparable to and larger than the "real" upper bound values, which lets me avoid some ugly boolean logic. I'll update the comment and probably also the variable name.
What "ugly boolean logic" are you referring to?
I thought one can always use `value_or(999)` anyway. To me, that means that the option approach is a refinement of yours. But depending on the situation, it might be possible to avoid the extrema value 999 altogether.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/BitwiseShiftChecker.cpp:106-109
+  if (auto BRP = checkOvershift()) {
+    Ctx.emitReport(std::move(BRP));
+    return;
+  }
----------------
donat.nagy wrote:
> steakhal wrote:
> > I'd prefer `BR` or `Report`. Since we know already that we are in the path-sensitive domain, `P` has less value there.
> > I'd apply this concept everywhere in the patch.
> The `P` stands for the "Ptr" in the name of the type alias `BugReportPtr`; but your comment clearly highlights that this variable name is confusing ;).
> 
> I'll probably avoid the `auto` and use `BugReportPtr Bug = ...` to remind the reader that this is a pointer (that can be null) and if it's non-null, then we found a bug.
Oh I see. To me, it's fairly obvious now that bugreports are just smart-pointers.
So I implicitly looked for some other reason. But just look at other checker, and call it the same way how they do, for consistency. That's all I wanted.
There are two/three common options I believe: `B` or `BR`, and maybe `Report`.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/BitwiseShiftChecker.cpp:190-191
+  std::string Msg =
+      llvm::formatv("The result of {0} shift is undefined because the right "
+                    "operand{1} is not smaller than {2}, the capacity of '{3}'",
+                    isLeftShift() ? "left" : "right", RightOpStr, LHSBitWidth,
----------------
donat.nagy wrote:
> steakhal wrote:
> > 
> This is an important distinction, I use "not smaller than {2}" as a shorter synonym of "larger than or equal to {2}". I can choose some other wording if you feel that this is not clear enough.
No, I have not strong feelings. Feel free to use the one you like more.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/BitwiseShiftChecker.cpp:218
+                                       Side == OperandSide::Left ? "Left" : "Right",
+                                       isLeftShift() ? "left" : "right")
+                             .str();
----------------
donat.nagy wrote:
> steakhal wrote:
> > The `isLeftShift() ? "left" : "right"` expression appears 4 times in total.
> > I'd also recommend to not distinguish the capitalized "left" and "right" strings, and just post-process the messages to ensure that the first character is capitalized inside the `createBugReport()`.
> > 
> > Also note that you can reuse the same placeholder, so you would only need to pass the `side()` only once.
> > ```lang=C++
> > static void capitalizeBeginning(std::string &Str) {
> >   if (Str.empty())
> >     return;
> >   Str[0] = llvm::toUpper(Str[0]);
> > }
> > 
> > StringRef side() const {
> >   return isLeftShift() ? "left" : "right";
> > }
> > ```
> I introduced a method for `isLeftShift() ? "left" : "right"` (which is indeed repeated several times), but I didn't define `capitalizeBeginning` because that would've been used only once.
> 
> Your remark that [with the capitalization function] "you can reuse the same placeholder, so you would only need to pass the side() only once" seems to be incorrect, because `Side == OperandSide::Left ? "Left" : "Right"` (which operand are we talking about?) and `isLeftShift() ? "left" : "right"` (what kind of shift operator are we talking about?) are two independent values.
Makes sense. It was so tempting though.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/BitwiseShiftChecker.cpp:315
+    default:
+      llvm_unreachable("this checker does not use other comparison operators");
+  }
----------------
donat.nagy wrote:
> steakhal wrote:
> > 
> I'd say that the current wording is correct because the the checker "use"s the comparison operators (in its implementation) instead of "accept"ing them from outside; but I can replace the message with e.g. "this //function// does not accept other comparison operators" if you'd prefer that.
No strong feelings :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156312