This is an archive of the discontinued LLVM Phabricator instance.

[analyzer] Handle << operator for std::unique_ptr
ClosedPublic

Authored by RedDocMD on Jul 5 2021, 1:50 AM.

Details

Summary

This patch handles the << operator defined for std::unique_ptr in
the std namespace (ignores custom overloads of the operator).

Diff Detail

Event Timeline

RedDocMD created this revision.Jul 5 2021, 1:50 AM
RedDocMD requested review of this revision.Jul 5 2021, 1:50 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 5 2021, 1:50 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
NoQ added inline comments.Jul 5 2021, 10:52 PM
clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp
218–219

When you're doing evalCall you're responsible for modeling *all* aspects of the call. One does not simply say that they modeled all aspects of the call when they didn't even set the return value :)

Similarly to how make_unique delegates work to the constructor of the managed object and ~unique_ptr delegates work to the destructor of the managed object, I suspect this code could delegate work to basic_ostream::operator<<(T *). We don't yet have any facilities to implement such logic yet (i.e., entering function call from a checker callback).

Given that we don't do much modeling of basic_ostream yet, I think it's perfectly fine to invalidate the stream entirely (which would be as precise as whatever the default evaluation gave us) (see ProgramState::invalidateRegions) and return the reference to the stream (which is already better than what the default evaluation gave us); additionally, we get extra precision because we don't invalidate the rest of the heap. That's the bare minimum of what we have to do here if we are to do anything at all.

This also gives some ideas of how to write tests for this patch.

That said, I suspect that this patch is not critical to enabling the checker by default, because we probably already know that this method doesn't change the inner pointer value (simply through not doing anything special) (and accepting the smart pointer by const reference, so even if we implement invalidation it will probably still "just work") (?).

RedDocMD updated this revision to Diff 357031.Jul 7 2021, 11:49 AM

Invalidating regions

clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp
218–219

Does this work?

NoQ added a comment.Jul 7 2021, 12:26 PM

Yes, I think this totally works now!

Can you also add some tests?

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

This should be static right?

98

I think the new functions that you've added aren't yet used in an inter-checker manner. Maybe keep them as static too until we have an actual use case?

221–222

Might also be an UnknownVal which doesn't have a region. It's hard to test because there doesn't exist a test in which UnknownVal appears for a good reason (they only appear for bad reasons) but I'd still rather bail out.

Good job, great to see how you are going through the whole list of methods!

As for the patch, some tests and COMMENTS will be nice. Also, I think that for a checker that models quite a lot of functions and methods, we need to follow the pattern:

if (isMethodA(...)) {
  return handleMethodA(...);
}
if (isMethodB(...)) {
  return handleMethodB(...);
}
...

however small implementations for those methods are. It will give fast insight into which methods are actually supported.

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

When you change those vectors of names to global arrays, we can change it to ArrayRef to be more idiomatic LLVM code.

111

This is a compile-time constant, I don't think we should construct it every time in runtime. Global array of three names is totally fine.

194

Same goes here

RedDocMD marked 3 inline comments as done.Jul 8 2021, 11:23 AM
RedDocMD marked 3 inline comments as done.Jul 8 2021, 11:33 AM
RedDocMD updated this revision to Diff 357307.Jul 8 2021, 11:43 AM

Little refactors

I will be figuring out some tests tomorrow morning.

RedDocMD updated this revision to Diff 357422.Jul 9 2021, 12:11 AM

Tests implemented

Great, thanks for addressing my comments! I still have a couple of minor suggestions though.

clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp
98–99

If it's a compile-time constant, let's make sure it is.
nit: I prefer global scope constants to be CAPITALIZED, so it's easier to spot them in the code.

192

Same here + don't call it "Name" (singular). It is a) an array and b) in the future, we might add more things to it, so we shouldn't need to rename it everywhere.

NoQ accepted this revision.Jul 9 2021, 10:30 AM

Great, thanks!

clang/test/Analysis/smart-ptr.cpp
472–474 ↗(On Diff #357422)

Another thing to test could be that global variables are not invalidated. Eg.:

int glob;
void foo(std::unique_ptr<int> P) {
  int x = glob;
  std::cout << P;
  int y = glob;
  clang_analyzer_eval(x == y); // expected-warning{{TRUE}}
}
476 ↗(On Diff #357422)

This deserves a FIXME: because the expected result is still TRUE.

This revision is now accepted and ready to land.Jul 9 2021, 10:30 AM
RedDocMD marked 4 inline comments as done.Jul 10 2021, 7:50 AM
RedDocMD added inline comments.
clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp
192

Well, we won't have to rename but we will still have to change the array length everywhere if more names are added.

RedDocMD marked an inline comment as done.Jul 10 2021, 7:50 AM
RedDocMD updated this revision to Diff 357724.Jul 10 2021, 8:08 AM

Little refactors, one more test

vsavchenko added inline comments.Jul 12 2021, 6:30 AM
clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp
86

ArrayRef is already a ref (and it says it in its name), so you should remove it there.

102

ArrayRef has an implicit constructor from C-arrays, putting size here is unnecessary.
And the reason why you were not able to do this is type mismatch. STD_PTR_NAMES is array of llvm::StringLiteral, but you chose ArrayRef<StringRef> as the type of the argument.

192

No, we don't have to do that. See the reason in the other comment.
Whenever you need to explicitly hardcode size for a stack-allocated array, you are doing something wrong. Stack-allocated everything (VLAs aside) is required to have a fixed size.

RedDocMD updated this revision to Diff 358204.Jul 13 2021, 2:31 AM

Removed stupid mistakes

vsavchenko accepted this revision.Jul 13 2021, 2:36 AM

Removed stupid mistakes

No need for self deprecation here! Thanks for addressing these!

RedDocMD updated this revision to Diff 359211.Jul 15 2021, 9:43 PM

Post rebase cleanup

RedDocMD closed this revision.Jul 18 2021, 7:28 AM

For some reason this revision did not get automatically closed after commit, manually merging it.