[PATCH] D86027: [analyzer] Add bool operator modeling for unque_ptr

[Nithin VR via Phabricator via cfe-commits]

vrnithinkumar added inline comments.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:137
+  const auto *RecordDecl = MethodDecl->getParent();
+  if (!RecordDecl || !RecordDecl->getDeclContext()->isStdNamespace())
+    return InnerType;
----------------
xazax.hun wrote:
> I'd rather use `Decl::isInStdNamespace` instead of querying the DeclContext of the decl. The former is more robust with inline namespaces. 
Changed to use `Decl::isInStdNamespace`


================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:141
+  const auto *TSD = dyn_cast<ClassTemplateSpecializationDecl>(RecordDecl);
+  if (TSD) {
+    auto TemplateArgs = TSD->getTemplateArgs().asArray();
----------------
xazax.hun wrote:
> Inverting this condition would reduce the indentation in the rest of the function.
inverted the if condition


================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:145
+        TemplateArgs.size() > 0 &&
+        "Smart pointer should have specialized with atleast one template type");
+    auto InnerValueType = TemplateArgs[0].getAsType();
----------------
NoQ wrote:
> That's pretty fundamental, right? If it's a specialization, it must have something specialized. It isn't specific to unique pointers, right?
> 
> Because unique pointers aren't special; technically anybody can define an arbitrary class with name `std::unique_ptr` and any properties they'd like. It's going to be undefined behavior according to the standard (because namespace `std` is explicitly reserved for the standard library) but if the compiler *crashes* it'll still be our fault.
> 
> 
added a check for `TemplateArgs.size() == 0` before accessing the first Template argument.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:147
+    auto InnerValueType = TemplateArgs[0].getAsType();
+    InnerType =
+        C.getASTContext().getPointerType(InnerValueType.getCanonicalType());
----------------
xazax.hun wrote:
> You could return the real inner type here and replace all other returns with `return {};` making the code a bit cleaner.
Updated to return with {}


================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:404
 
+void SmartPtrModeling::handleBoolOperation(const CallEvent &Call,
+                                           CheckerContext &C) const {
----------------
vsavchenko wrote:
> I suggest `BoolConversion`
yeah thats sounds better.
Changed.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:413
+  if (InnerPointVal) {
+    bool IsInnerPtrNull = InnerPointVal->isZeroConstant();
+    State = State->BindExpr(CallExpr, C.getLocationContext(),
----------------
xazax.hun wrote:
> Is this actually correct? What if the InnerPtr is an unconstrained symbol. In that case, it is not a zero constant so we will assume that it is constrained to non-zero. As far as I understand.
Fixed as you suggested in the below comments


================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:432
+    return;
+  } else {
+    // In case of inner pointer SVal is not available we create
----------------
xazax.hun wrote:
> I'd do it the other way around as we discussed during the call.
> * Move the task of conjuring a new symbol to the beginning of the method.
> * Start by calling this function at the beginning of modeling operator bool.
> * The rest of the function could assume that there always is a symbol. It could be constrained to be non-null, it could be the zero constant, or it could be a completely unconstrained symbol. The latter will not work as expected with your current implementation, see my comment above.
Made the changes as suggested above


================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:448
+
+    if (NullState) {
+      auto NullVal = C.getSValBuilder().makeNull();
----------------
xazax.hun wrote:
> NoQ wrote:
> > There's no need to check. You just conjured a brand new symbol out of thin air; you can be sure that it's completely unconstrained and both states are feasible. You can instead `assert()` that they're both feasible.
> I think instead of removing this check, this method should be reworked. I think it might have some bugs, see my comment at the beginning of this method.
Refactored the method as suggested.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:458-460
+                        OS << "Assuming smart pointer ";
+                        ThisRegion->printPretty(OS);
+                        OS << " is null";
----------------
NoQ wrote:
> Wait, what happens when the region can't be pretty-printed? Does it print two spaces between "pointer" and "is"?
Yes.
Created `checkAndPrettyPrintRegion` to check if the region can be pretty printed or not to avoid two spaces.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86027