[PATCH] D97183: [analyzer] Add NoteTag for smart-ptr get()

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

No-no, `TrackControlDependencyCondBRVisitor`'s purpose is completely different. It tracks symbols that didn't necessarily participate in the report but participated in conditional statements that lexically surround the report. It's used for explaining how did we got ourselves into a given lexical spot but it doesn't explain why is this a problem.

We can get it out of the way:

  A *a = P.get(); // no note expected
  if (!P) {}
  P->foo();

vs.

  A *a = P.get(); // expected note
  if (!a) {}
  P->foo();

I suspect that it may still work and the reason this works is because we're not collapsing `.get()`'s return value to null when it's constrained to null. Given that the only interesting thing that could happen to the return value of `.get()` is getting constrained to null (because it's an rvalue, the programmer can't use it to overwrite the raw pointer value inside the pointer), it's either already null or you'd see the constraint tracked once you mark the symbol as interesting.

The reason i'd still not like this solution is because collapsing the symbol to null on `.get()` (if it's already constrained to null) is arguably the preferred behavior as it makes constraint solver's life easier. But that'd most likely break your tracking solution.

In D97183#2637341 <https://reviews.llvm.org/D97183#2637341>, @steakhal wrote:

> Do you think it is a related issue @NoQ?

I don't see any smart pointers or null dereferences so... no? But there's definitely //an// issue with tracking in your example. It's definitely correct that `div` is initialized with zero on the path on which `p0` is zero but the events that led to `p0` being zero are not explained which is a bug that needs to be fixed.



================
Comment at: clang/test/Analysis/smart-ptr-text-output.cpp:317
 
 void getShouldNotAlwaysLeaveANote() {
 	std::unique_ptr<A> P; // expected-note {{Default constructed smart pointer 'P' is null}}
----------------
Looks like your git history is acting up. Your patch adds this test right? Are there more proposed changes in the cpp files that aren't currently highlighted for a similar reason?

I'll try to play with your patch locally once this is fixed ^.^


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97183