This is an archive of the discontinued LLVM Phabricator instance.

[analyzer] Add bool operator modeling for unque_ptr
ClosedPublic

Authored by vrnithinkumar on Aug 15 2020, 2:44 PM.

Details

Summary

Added modeling for unique_ptr::operator bool method

Diff Detail

Event Timeline

vrnithinkumar created this revision.Aug 15 2020, 2:44 PM
vrnithinkumar requested review of this revision.Aug 15 2020, 2:44 PM
vrnithinkumar edited the summary of this revision. (Show Details)Aug 15 2020, 2:46 PM
vrnithinkumar added inline comments.Aug 15 2020, 2:49 PM
clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp
147

This seems little messy here.
I guess once we model the std::move for smart ptr it will be less messy

NoQ added inline comments.Aug 15 2020, 3:44 PM
clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp
400

It's always UnknownVal because the call has not been evaluated yet. This assume() does nothing.

vrnithinkumar added inline comments.Aug 15 2020, 4:15 PM
clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp
411

Since the inner pointer value can be any non-null value, I am not sure what should be the value to be added to the map for tracking.

vrnithinkumar added inline comments.Aug 15 2020, 4:20 PM
clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp
400

Okay.
So in that case the NotNullState and NullState will be same as the original state?

NoQ added inline comments.Aug 15 2020, 6:36 PM
clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp
400

Yup.

411

Don't add values, constrain existing values.

xazax.hun added inline comments.Aug 17 2020, 4:18 AM
clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp
147

I agree. isNullAfterMoveMethod is especially confusing as it does not do what the name of the function says. It checks if the Call is a boolean conversion operator. I'd suggest renaming that method to make this code a bit more sensible.

Moreover, when ModelSmartPtrDereference is true, we no longer model moves below right? I think a comment that this logic is duplicated handleBoolOperation might make the code cleaner.

But yeah, this needs to be cleaned up, hopefully really soon.

389

This looks fine for now, but we often prefer adding a return after each case to avoid executing multiple addTransitions accidentally after refactoring.

vrnithinkumar marked 5 inline comments as done.

Changes to use conjureSymbolVal if the inner pointer value is missing

clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp
147
  • Renamed the method to isBooleanOpMethod
  • Added comments and FIXME

Hopefully we can clean up once std::move is modeled.

389

Added return

411

Made changes to create the conjured symbol and use that to constrain.

vrnithinkumar added inline comments.Aug 19 2020, 12:32 PM
clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp
129

It seems like a long shot to me.
I am not sure is there any direct or easy way to get inner pointer type from a smart pointer

NoQ added inline comments.Aug 22 2020, 12:12 AM
clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp
129

That's about right. You're doing exactly what you're asked: grab the template parameter of the class. The problem is indeed that complicated!

144

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.

418

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.

428–430

Wait, what happens when the region can't be pretty-printed? Does it print two spaces between "pointer" and "is"?

xazax.hun requested changes to this revision.Aug 22 2020, 9:01 AM

Please add a test case, where the unique_ptr is initialized from a pointer parameter that has no assumptions. I think that case is not handled correctly.

clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp
136

I'd rather use Decl::isInStdNamespace instead of querying the DeclContext of the decl. The former is more robust with inline namespaces.

140

Inverting this condition would reduce the indentation in the rest of the function.

146

You could return the real inner type here and replace all other returns with return {}; making the code a bit cleaner.

383

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.

402

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

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.

This revision now requires changes to proceed.Aug 22 2020, 9:01 AM
vsavchenko added inline comments.Aug 24 2020, 1:09 AM
clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp
374

I suggest BoolConversion

vrnithinkumar marked 11 inline comments as done.
  • Adding checkLiveSymbols and review comments changes
clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp
136

Changed to use Decl::isInStdNamespace

140

inverted the if condition

144

added a check for TemplateArgs.size() == 0 before accessing the first Template argument.

146

Updated to return with {}

374

yeah thats sounds better.
Changed.

383

Fixed as you suggested in the below comments

402

Made the changes as suggested above

418

Refactored the method as suggested.

428–430

Yes.
Created checkAndPrettyPrintRegion to check if the region can be pretty printed or not to avoid two spaces.

NoQ added inline comments.Aug 25 2020, 12:08 AM
clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp
275–278

Yes, this looks correct, thanks!

In the ideal world we'd have checked that I->first is live before marking I->second live. Unfortunately, currently checkLiveSymbols is invoked before RegionStore's live symbols detection, so we can't rely on that and it's a bug; in order to be correct, RegionStore's live symbols detection and checker's live symbols callback should engage in fixpoint iteration.

415–417

Because these note tags aren't specific to the bug report at hand, they have to be marked as prunable (i.e., the optional getNoteTag()'s parameter "IsPrunable" should be set to true). That'd reduce bug report clutter by not bringing in stack frames that aren't otherwise interesting for the bug report.

Something like this may act as a test (i didn't try):

struct S {
  std::unique_ptr<int> P;

  void foo() {
    if (!P) { // no-note because foo() is pruned
      return;
    }
  }

  int bar() {
    P = new int(0);
    foo();
    return 1 / *P; // warning: division by zero
  }

}
  • Making the note tags prunable
vrnithinkumar marked an inline comment as done.Aug 25 2020, 5:47 AM
vrnithinkumar added inline comments.
clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp
415–417

Marked these tags as prunable and added the tests.

NoQ added inline comments.Aug 25 2020, 1:10 PM
clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp
404

Wait, why are we marking the region interesting here? Not every null smart pointer is interesting, only the ones that are actually dereferenced.

418–420

Also i'm starting to suspect that these notes aren't actually needed when there's no "Assuming..."; in particular, we don't emit such notes for raw pointers, so we probably shouldn't emit them for smart pointers either. There's anyway going to be a note about "Taking true branch..." or something like that, which is sufficient to understand what was the smart pointer.

I.e., this note is unnecessary:

void foo() {
  std::unique_ptr<int> P = nullptr;
  if (P) {        // Smart pointer 'P' is null
                  // Taking false branch
  }
}

The second note, "Taking false branch", is sufficient for explaining to the user that the smart pointer is known to be null. The user may ask why the smart pointer is null, but the note isn't explaining it. The way you marked the region as interesting (as i noticed in the above inline comment) would have indeed explained why it's null, but at this point we draw the line and believe that if the region isn't already interesting then the user most likely doesn't need to know why it's null (and if it's already interesting then there's no need to mark it again).

But this note is necessary:

void foo(std::unique_ptr<int> P) {
  if (P) {        // Assuming smart pointer 'P' is null
                  // Taking false branch
  }
}

This note conveys the extra information that we don't know for a fact that the smart pointer is null on the current path based on previous analysis, but instead the analyzer is *assuming* that it's possible that it's null due to the presence of the check in the code. That's a big deal.

vrnithinkumar marked an inline comment as done.
  • Removing unnecessary notetags.
vrnithinkumar marked 2 inline comments as done.Aug 26 2020, 11:14 AM
vrnithinkumar added inline comments.
clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp
404

This was added to show how the smart pointer, even though it is not dereferenced. But why did we take true/false branch based on this.

418–420

Since the "Taking false branch" is enough to implies the smart pointer in null, removing the notes which are unnecessary.

vrnithinkumar marked 2 inline comments as done.Aug 26 2020, 11:17 AM
NoQ accepted this revision.Aug 27 2020, 3:57 PM

I have no other concerns, i think this is good to go!

xazax.hun added inline comments.Aug 31 2020, 3:09 AM
clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp
393

Don't we want to actually add InnerPointerVal to TrackedRegionMap in this case?

I might be wrong but I cannot find where do we actually record the fact that this freshly conjured symbol belongs to the unique_ptr we are modeling.

  • Addressing review comments
  • Fixing minor spacing issue
vrnithinkumar marked an inline comment as done.Aug 31 2020, 10:02 AM
vrnithinkumar added inline comments.
clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp
393

Thanks for catching that.
We have to update the TrackedRegionMap to track the created conjureSymbolVal InnerPointerVal.
Updated to add it to the TrackedRegionMap

vrnithinkumar marked 2 inline comments as done.Aug 31 2020, 10:02 AM
xazax.hun accepted this revision.Aug 31 2020, 10:05 AM

LG! Thanks!

This revision is now accepted and ready to land.Aug 31 2020, 10:05 AM
This revision was landed with ongoing or failed builds.Aug 31 2020, 10:41 AM
This revision was automatically updated to reflect the committed changes.