Page MenuHomePhabricator

[analyzer] Handle std::swap for std::unique_ptr
ClosedPublic

Authored by RedDocMD on Jun 15 2021, 8:05 AM.

Details

Summary

This patch handles the std::swap function specialization
for std::unique_ptr. Implemented to be very similar to
how swap method is handled

Diff Detail

Event Timeline

RedDocMD created this revision.Jun 15 2021, 8:05 AM
RedDocMD requested review of this revision.Jun 15 2021, 8:05 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 15 2021, 8:05 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript

I am not entirely satisfied with the note that is being emitted currently from the std::swap handling (its ripped off from the note emitted for std::unique_ptr::swap).
Any suggestions?

vsavchenko added inline comments.Jun 15 2021, 8:18 AM
clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp
131–138

That's great!

254–292

Maybe a separate method then?

283

Wait, and what if the second argument is interesting?

clang/test/Analysis/smart-ptr-text-output.cpp
80

I know that this case existed before, but can we initialize P with non-null, so that the warning is a bit more life-like?

It looks like new functionality is VERY much like handleSwap, and we should definitely merge common parts.

Also, I think that commit title should be more specific than "Handle std::swap". It should at least mention the checker.

RedDocMD retitled this revision from [analyzer] Handle `std::swap` to [analyzer] Handle `std::swap` for std::unique_ptr.Jun 15 2021, 10:48 AM
RedDocMD retitled this revision from [analyzer] Handle `std::swap` for std::unique_ptr to [analyzer] Handle std::swap for std::unique_ptr.
RedDocMD added inline comments.Jun 15 2021, 11:21 AM
clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp
283

I know. That's why I said that the note is crappy.
The question I have in mind is - what if we swap null with null? Do we want a note in that case?
IMO, we should have a note for every case of swapping a confirmed null with something that is not confirmed to be null.

The current implementation of how notes are emitted in handleSwap is broken. Consider the following code:

#include <memory>

void foo() {
  auto ptr1 = std::unique_ptr<int>(new int(10));
  auto ptr2 = std::unique_ptr<int>(new int(13));
  ptr1.swap(ptr2);
  ptr1.reset();
  *ptr1;
}

This yields the following warning and notes:

swap-and-reset.cpp:8:3: warning: Dereference of null smart pointer 'ptr1' [alpha.cplusplus.SmartPtr]
  *ptr1;
  ^~~~~
swap-and-reset.cpp:4:36: note: Assigning 10
  auto ptr1 = std::unique_ptr<int>(new int(10));
                                   ^~~~~~~~~~~
swap-and-reset.cpp:4:15: note: Smart pointer 'ptr1' is constructed
  auto ptr1 = std::unique_ptr<int>(new int(10));
              ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
swap-and-reset.cpp:5:36: note: Assigning 13
  auto ptr2 = std::unique_ptr<int>(new int(13));
                                   ^~~~~~~~~~~
swap-and-reset.cpp:5:15: note: Smart pointer 'ptr2' is constructed
  auto ptr2 = std::unique_ptr<int>(new int(13));
              ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
swap-and-reset.cpp:6:3: note: Swapped null smart pointer 'ptr2' with smart pointer 'ptr1'
  ptr1.swap(ptr2);
  ^~~~~~~~~~~~~~~
swap-and-reset.cpp:7:3: note: Smart pointer 'ptr1' reset using a null value
  ptr1.reset();
  ^~~~~~~~~~~~
swap-and-reset.cpp:8:3: note: Dereference of null smart pointer 'ptr1'
  *ptr1;
  ^~~~~

But clearly, ptr2 is not null.

RedDocMD updated this revision to Diff 352201.Jun 15 2021, 11:34 AM

Refactored common code, removed note emission

I have gotten rid of notes for the time being.
The trouble is that, we were not figuring out whether the null-pointer de-reference was occurring due to the null value being swapped with the current pointer.
We just assumed that. So I am guessing we would need a visitor to figure that out? (Hooray for Valeriy!)
Or is there a simpler solution?

NoQ added a comment.Jun 16 2021, 12:37 AM

I think notes are fairly straightforward: if we're tracking one smart pointer below the swap, we should stop tracking it and start tracking the other smart pointer above the swap. If both were tracked, keep tracking both (but in swapped manners, assuming we're tracking them in different manners). It doesn't matter whether any of them is null, it only matters which ones were tracked. Though I guess we could mention if any of them are null or do other cosmetic improvements depending on how they participated in the bug. Trackedness is equivalent to interestingness because we're dealing with regions.

RedDocMD updated this revision to Diff 352627.Jun 17 2021, 12:14 AM

Marking and un-marking interestingness

I agree with @NoQ that notes are pretty much straightforward and we shouldn't abandon them altogether. Refinements about what is null or non-null pointer are purely cosmetic and we definitely can tweak this messaging.

clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp
260–266

I guess handleSwap can take SVals instead of MemRegion and we can mostly cut on this boilerplate as well.

return handleSwap(State, Call.getArgSVal(0), Call.getArgSVal(1), C);

and

return handleSwap(State, IC->getCXXThisVal(), Call.getArgSVal(0), C);
628–637

nit: these two pieces of code are very much the same

xazax.hun added inline comments.Jun 17 2021, 4:12 PM
clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h
435 ↗(On Diff #352627)

Bikeshedding: I wonder if we prefer Uninteresting to NotInteresting. Or alternatively, if we want to emphasize this is only the lack of interestingness, we could name it removeInterestingness. I do not have strong feelings about any of the options, I was just wondering if any of you has a preference.

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

I wonder about the value of this assertion. Shouldn`t Call.isCalled(StdSwapCall) already validate the number of arguments?

257

Nit: we usually omit braces for simple if statements.

628–637

I guess we could make markInteresting take an optional bool and swap the interestingness unconditionally.

NoQ accepted this revision.Jun 17 2021, 11:28 PM

I think this patch is good to go as long as other folks are happy. I'm glad that uninterestingness is becoming a thing!

This revision is now accepted and ready to land.Jun 17 2021, 11:28 PM
RedDocMD marked 5 inline comments as done.Jun 18 2021, 11:47 AM
RedDocMD added inline comments.
clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h
435 ↗(On Diff #352627)

I was actually planning to use something like unmarkInteresting, but that is grammatically incorrect. removeInterestingness is fair enough, but a mouthful I think. As for Uninteresting, I think we are better off avoiding clever uses of English and stick with simple, programmer jargon.

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

About as valuable as any assertion is ;)
My reasoning is that since we are calling Call.getArgSval(0) and Call.getArgSVal(1), we had better assert that there are two args.
As a side-note, I had a bug in my CallDescriptor, which was caught by this assertion

260–266

Yup, okay.

628–637

Then we would need to rename it to something like setInterestingness.
And renaming such a widely used function would be a real pain.

628–637

Yes, but I don't think refactoring them into a lambda (within a lambda) would be that nice either.

RedDocMD marked 3 inline comments as done.Jun 18 2021, 11:47 AM
RedDocMD updated this revision to Diff 353068.Jun 18 2021, 11:49 AM

Some more refactoring

This change is accepted already. Is it good to commit? I would need the change for symbol "uninterestingness" in D104925.

RedDocMD updated this revision to Diff 359600.Sun, Jul 18, 1:11 AM

Post rebase cleanup

This revision was landed with ongoing or failed builds.Sun, Jul 18, 2:10 AM
This revision was automatically updated to reflect the committed changes.