This is an archive of the discontinued LLVM Phabricator instance.

[Analyzer] Support note tags for smart ptr checker
ClosedPublic

Authored by vrnithinkumar on Jul 26 2020, 10:09 AM.

Details

Summary
  • Added support for note tags for null smart_ptr reporting
  • Updated the warning message with variable and smart_ptr type
  • Added tests for note tags

Diff Detail

Event Timeline

vrnithinkumar created this revision.Jul 26 2020, 10:09 AM
vrnithinkumar edited the summary of this revision. (Show Details)Jul 26 2020, 10:14 AM
NoQ added inline comments.Jul 26 2020, 5:43 PM
clang/lib/StaticAnalyzer/Checkers/SmartPtrChecker.cpp
100

Please getNameAsString() because getName() crashes on anonymous declarations (eg., lambda captures, anonymous nested structs/unions, etc.).

109–110

Aha, this time you checked for anonymous declarations! Good.

That said, i'm not sure type is actually useful for this warning, because they're roughly all the same.

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

I feel this is a bit over-engineered. There's no need to create an enum and later convert it into a string when you can capture a string directly. Like, this entire "details" structure of your, it should be just captures instead, and your strings would make it immediately obvious what kind of note is emitted:

C.addTransition(State, getNoteTag([R](PathSensitiveBugReport &BR) {
  if (&BR.getBugType() != &NullDereferenceBugType || !R->isInteresting())
    return "";
 
  return Twine("Default constructed smart pointer '") + getSmartPtrName(R) + "' is null";
}));

Hmm, maybe we should also provide an overload with lambdas of signature`void(PathSensitiveBugReport &BR, llvm::raw_ostream OS)` so that to make the same one-liners possible with streams? Something like this:

class CheckerContext {
// ...
  NoteTag *getNoteTag(std::function<void(PathSensitiveBugReport &BR, llvm::raw_ostream OS)> f) {
    return getNoteTag([](PathSensitiveBugReport &BR) -> std::string {
      llvm::SmallString<128> Str;
      llvm::raw_svector_ostream OS(Str);
      f(BR, OS);
      return OS.str();
    });
  }
// ...
};

Then you could do:

C.addTransition(State, getNoteTag([R](PathSensitiveBugReport &BR) {
  if (&BR.getBugType() != &NullDereferenceBugType || !R->isInteresting())
    return;

  OS << "Default constructed smart pointer '" << getSmartPtrName(R) << "' is null";
}));
344–348

Ok, note that note tags are attached to nodes independently of bug reports; when the report is thrown, only then we know what are the smart pointers that should be explained.

So there are two checks that you should do here:

  1. Check that the bug report is emitted by your checker (eg., by comparing bug types). If not, don't add notes.
  1. Check that the region about which the note speaks is related to your report (i.e., it's not a completely unrelated smart pointer). You can do that by marking the smart pointer as "interesting" (i.e., PathSensitiveBugReport::markIntersting()) when you emit the report, and then in the lambda you check whether the smart pointer is interesting before you emit a note. Additionally, you can carry over interestingness when smart pointers are copied.

This is what i was trying to accomplish with this code snippet that i included in the examples in the other comment:

if (&BR.getBugType() != &NullDereferenceBugType || !R->isInteresting())
  return "";
NoQ added inline comments.Jul 26 2020, 5:58 PM
clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp
149

(forgot , llvm::raw_ostream OS in the last snippet; probably forgot a bunch of other stuff because i didn't try to actually compile these snippets)

344–348

(i strongly recommend having test cases for both of these issues)

vrnithinkumar added inline comments.Jul 31 2020, 5:09 PM
clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp
344–348

I was stuck on how to check the 2 cases from SmartPtrModeling.

  1. I was not able to figure out how to access NullDereferenceBugType defined in the SmartPtrChecker in SmartPtrModeling to check &BR.getBugType() != &NullDereferenceBugType. Since NullDereferenceBugType is part of the SmartPtrChecker I could not access it from PathSensitiveBugReport. One way I figured out is to use getCheckerName() on BugType and compare the string. I feel this one as little hacky.
  1. I got stuck on how will we implement the !R->isInteresting() in case of the bug report is added by some other checker on some other region. For below test case, bug report is added on a raw pointer by CallAndMessageChecker and the !R->isInteresting() will not satisfy and we will not be adding note tags where unique_ptr is released. I tried getting the LHS region for A *AP = P.release(); assignment operation and check if the region is interesting but not sure whether its gonna work for some complex assignment cases.
void derefOnReleasedNullRawPtr() {
  std::unique_ptr<A> P;
  A *AP = P.release(); // expected-note {{'AP' initialized to a null pointer value}}
  // expected-note@-1 {{Smart pointer 'P' is released and set to null}}
  AP->foo(); // expected-warning {{Called C++ object pointer is null [core.CallAndMessage]}}
  // expected-note@-1{{Called C++ object pointer is null}}
}
NoQ added inline comments.Jul 31 2020, 5:23 PM
clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp
344–348

Since NullDereferenceBugType is part of the SmartPtrChecker I could not access it from PathSensitiveBugReport.

You shouldn't be accessing it from the bug report, you should be accessing it from the lambda. See the example code snippets in D84600#inline-779418

For below test case, bug report is added on a raw pointer by CallAndMessageChecker and the !R->isInteresting() will not satisfy and we will not be adding note tags where unique_ptr is released.

That's an interesting question (no pun intended). The way i imagine this working is: the note tag for .release() should try to figure out whether the raw pointer is tracked and mark the smart pointer as interesting based on that. If the raw pointer was a symbol that would have been easy (either null dereference checker or trackExpressionValue() could mark it as interesting). But for concrete null pointer this won't work.

Maybe we should consider introducing interesting expressions. I.e., when trackExpressionValue() reaches the call-expression P.release(), it has to stop there. But if it also marked the call-expression as interesting, the note tag provided by the checker could read that interestingness information and act upon it by marking the smart pointer region as interesting.

NoQ added inline comments.Jul 31 2020, 5:27 PM
clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp
344–348

That's an interesting question

I'd rather make a separate commit for this endeavor because it sounds pretty nasty.

vrnithinkumar added inline comments.Jul 31 2020, 5:31 PM
clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp
344–348

You shouldn't be accessing it from the bug report, you should be accessing it from the lambda. See the example code snippets in D84600#inline-779418

Sorry, I am still confused how will the lambda defined in the SmartPtrModeling can capture the NullDereferenceBugType from SmartPtrChecker?

NoQ added inline comments.Aug 1 2020, 1:37 PM
clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp
344–348

Lambda can capture anything that's available in its lexical context. For capturing a field of the surrounding class, i believe you should capture this. See how other checkers do that.

NoQ added inline comments.Aug 2 2020, 8:56 PM
clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp
344–348

Wait, i see what you mean. We're in the wrong checker!

Let's turn the null dereference bug type into an inter-checker API then? Make a header, like Move.h but say NullDereference.h, and introduce a const BugType *getNullDereferenceBugType() that we could invoke in the lambda.

Because the bug type is created with checker registration, it might make sense to maintain a static global pointer to the bug type that'll initially be null but initialized during registration.

vrnithinkumar marked 10 inline comments as done.
vrnithinkumar edited the summary of this revision. (Show Details)
  • Addressing review comments
clang/lib/StaticAnalyzer/Checkers/SmartPtrChecker.cpp
100

Changed to use getNameAsString()

109–110

Okay. I was trying to be consistent with MoveChecker.
Just removed smart pointer type.

clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp
149
  • Added overload for getNoteTag with lambda accepting a llvm::raw_ostream
  • Used the overload instead of SmartPtrTagDetails structure to add tags.
  • Removed the SmartPtrTagDetails and changed related code.
344–348

(i strongly recommend having test cases for both of these issues)

Added the tests

344–348

That's an interesting question

I'd rather make a separate commit for this endeavor because it sounds pretty nasty.

Will make a separate commit

344–348
  • Created NullDereference.h and added const BugType *getNullDereferenceBugType()
  • Added a static global pointer to the bug type.
  • Added the check if the region is interesting and bug type is matching.
NoQ added inline comments.Aug 3 2020, 8:40 PM
clang/lib/StaticAnalyzer/Checkers/NullDereference.h
21 ↗(On Diff #282771)

Namespaces are traditionally snake_case rather than camelCase.

clang/lib/StaticAnalyzer/Checkers/SmartPtrChecker.cpp
102

Wait, i don't understand again. You're taking the bug type from that checker and using it in that checker. Why did you need to make it global? I thought your problem was about capturing the bug type from the raw null dereference checker?

109–110

These were important in MoveChecker because use-after-move rules for smart pointers are different from use-after-move rules of any other class in the standard library and the checker has to behave differently (https://wiki.sei.cmu.edu/confluence/x/O3s-BQ).

vrnithinkumar added inline comments.Aug 4 2020, 9:33 AM
clang/lib/StaticAnalyzer/Checkers/SmartPtrChecker.cpp
102

Wait, i don't understand again. You're taking the bug type from that checker and using it in that checker. Why did you need to make it global? I thought your problem was about capturing the bug type from the raw null dereference checker?

The check for bug type is in SmartPtrModeling but the bug type is defined in SmartPtrChecker

109–110

Oh
That makes sense.

NoQ added inline comments.Aug 4 2020, 11:54 AM
clang/lib/StaticAnalyzer/Checkers/SmartPtrChecker.cpp
102

Aha, ok, i misunderstood again. So i guess we didn't need a new header then. That said, it's an interesting layering violation that we encounter in this design: it used to be up to the checker to attach a visitor and so should be activating a note tag, but note tags are part of modeling, not checking.

I guess that's just how it is and it's the responsibility of the checkers to inform the modeling about bug types. I guess the ultimate API could look like BugReport->activateNoteTag(NoteTagTag TT) (where TT is a crying smiley that cries about "tag tags" T_T). But that's a story for another time.

NoQ added a comment.Aug 4 2020, 12:01 PM

Generally i think this is shaping up nicely, with the couple of minor nits addressed i'm happy to land this. Like, there are a few cornercases that we could address but we could do that in follow-up patches. Folks, do you see anything else here?

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

You never ever check for this case. Therefore this function entirely boils down to Call.getArgExpr(0) which is shorter than getFirstArgExpr(Call) anyway.

xazax.hun added inline comments.Aug 4 2020, 10:51 PM
clang/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h
312

The callback is taken is an rvalue reference in other getNoteTag APIs. I think these overloads should be consistent.
Also, I wonder if the caller should be able to manipulate the buffer size of the small string (as a template parameter), but I do not have strong feelings about this.

As a side note, since Clang is using C++14, maybe the lambda captures in the getNoteTag overloads above should utilize it and capture the callback by move. This is more of a note to ourselves independent of this patch.

Side note 2: maybe a modernize tidy check would be useful to discover where rvalue references are captured by value in lambdas?

clang/lib/StaticAnalyzer/Checkers/NullDereference.h
23 ↗(On Diff #282771)

I think this comment does not really add much value.

clang/lib/StaticAnalyzer/Checkers/SmartPtrChecker.cpp
102

I hope we will be able to figure out a nicer solution at some point. But I do not have a better alternative now, so I am ok with committing as is.

The API Artem is suggesting looks good to me (although it is out of scope for the GSoC). But I think that might not solve the problem of how multiple checkers should share the information about those tags. (Or at least I do not see how.)

Note that if we had both checks in the same file we might be able to work this problem around using CheckerManager::getChecker. I am not suggesting merging them, only wanted to point out.

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

I wonder whether MemRegion::printPretty is what we want here.

181

I am a bit confused. As far as I understand we ALWAYS add the note tag below, regardless of the first argument being null.
I think I do understand that we only trigger this note tag when there is a null dereference but consider the code below:

void f(int *p) {
  std::unique_ptr<int> u(p); // p is not always null here.
  if (!p) {
    *u; // p is always null here
  }
}

Do we ant to output the message that the smart pointer is constructed using a null value? While this is technically true, there is a later event in the path that uncovers the fact that p is null.

@NoQ what do you think? I do not have any strong feelings about this, I only wonder how users would react to a bug report like that.

334

I am a bit unhappy with this function.
I feel like it does not really have a purpose. It returns the null constant for default constructor calls, for every other case it just returns the first argument. The branch we have in this function would completely go away if we would inline this to all the call sites and the result would be a single line each place. I'd rather just remove this function.

NoQ added inline comments.Aug 5 2020, 12:26 PM
clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp
181

Indeed, nice!! We can only proclaim the pointer to be null if it's known to be null in the current state. I.e., if previous steps of the report indicated that it's null. If it's discovered to be null later, we cannot call it null yet. I think we should check the current state and say "is constructed using a null value" if it's already null or simply "is constructed" if it's not yet known to be null.

vrnithinkumar added inline comments.Aug 5 2020, 2:51 PM
clang/lib/StaticAnalyzer/Checkers/SmartPtrChecker.cpp
102

So i guess we didn't need a new header then.

So we should remove the NullDereference.h and add the inter checker api to get the BugType to SmartPtr.h?

I guess the ultimate API could look like BugReport->activateNoteTag(NoteTagTag TT)

Do we have to make these api changes on this review? Or some follow up changes?

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

Also we have to assert that we are adding a pointer value to TrackedRegionMap.

NoQ added inline comments.Aug 5 2020, 2:55 PM
clang/lib/StaticAnalyzer/Checkers/SmartPtrChecker.cpp
102

So we should remove the NullDereference.h and add the inter checker api to get the BugType to SmartPtr.h?

Yes.

Do we have to make these api changes on this review? Or some follow up changes?

You don't have to do this at all; i think you have enough stuff on your plate. But if you like you can put up another patch later.

vrnithinkumar marked 13 inline comments as done.
  • Changes from review comments
clang/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h
312

Changed to rvalue reference.

clang/lib/StaticAnalyzer/Checkers/NullDereference.h
21 ↗(On Diff #282771)

removed the new namespace

23 ↗(On Diff #282771)

Yeah.
removed it.

clang/lib/StaticAnalyzer/Checkers/SmartPtrChecker.cpp
102

Removed the new header file and added to SmartPtr.h

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

removed it and inlined.

132

Yeah Thanks.
This is what we exactly needed.

181

Added a check if the value is null or not and add message based on that.

334

Removed the function and inlined it

xazax.hun accepted this revision.Aug 7 2020, 1:26 AM

LGTM, thanks!

I think all the problems that left should be solved in a separate patch or even out of the scope for the GSoC.

clang/lib/StaticAnalyzer/Checkers/SmartPtrChecker.cpp
33

I find this comment redundant as well. It just repeats what is already evident from the code.

This revision is now accepted and ready to land.Aug 7 2020, 1:26 AM
NoQ added inline comments.Aug 7 2020, 11:06 AM
clang/test/Analysis/smart-ptr-text-output.cpp
66

Such note is unnecessary. We don't care what happens to P after it's released; we only care about its old value.

80

Instead of commenting out tests, i recommend testing the incorrect behavior (with a FIXME comment telling us why it's incorrect). This way we'll be notified when the test is fixed, accidentally or intentionally, and also generally that's more testing for everybody.

NoQ added inline comments.Aug 7 2020, 11:08 AM
clang/test/Analysis/smart-ptr-text-output.cpp
64

Unlike the next line, this line deserves a note that P holds a null value.

Layering violations are a running theme in the analyzer -- CheckerRegistry and the entire MallocChecker fiasco are two glaring examples. Luckily, this isn't a severe case so I wouldn't worry about it much.

I've been following your patches, but them mentors answer way ahead of me and are very thorough :^) Best of luck with the finale!

vrnithinkumar marked 4 inline comments as done.
  • Review comment changes
clang/lib/StaticAnalyzer/Checkers/SmartPtrChecker.cpp
33

removed

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

Added a FIXEM to add note "Default constructed smart pointer 'P' is null"

66

Removed this.

80

I have commented out this since right now std::swap is using unique_ptr.swap.
So note tag for std::swap is added in header file system-header-simulator-cxx.h
eg:
system-header-simulator-cxx.h Line 978: Swapped null smart pointer 'PNull' with smart pointer 'P'

Layering violations are a running theme in the analyzer -- CheckerRegistry and the entire MallocChecker fiasco are two glaring examples. Luckily, this isn't a severe case so I wouldn't worry about it much.

I've been following your patches, but them mentors answer way ahead of me and are very thorough :^) Best of luck with the finale!

Thanks for sharing checker dependency talk.
Also fo suggesting to separate checker and modeling from beginning. :)

vrnithinkumar added inline comments.Aug 8 2020, 10:30 AM
clang/test/Analysis/smart-ptr-text-output.cpp
80
  • Is it okay to add a expected-note on system-header-simulator-cxx.h?
NoQ added inline comments.Aug 9 2020, 11:34 AM
clang/test/Analysis/smart-ptr-text-output.cpp
80

Aha, i see.

It's obviously not ok because the header is included by multiple tests but only some tests will have the note show up.

The usual solution is to put the expected-note comment in the current test file but make it refer to the header. Here's doc from https://clang.llvm.org/doxygen/classclang_1_1VerifyDiagnosticConsumer.html:

If the diagnostic is generated in a separate file, for example in a shared header file, it may be beneficial to be able to declare the file in which the diagnostic will appear, rather than placing the expected-* directive in the actual file itself. This can be done using the following syntax:

// expected-error@path/include.h:15 {{error message}}

It still is a bit of a problem that you have to hardcode the line number (so everybody who modifies the header above your note will have to update your test) but that's not a big deal.

vrnithinkumar marked an inline comment as done.
  • Updating test with tags from header file
clang/test/Analysis/smart-ptr-text-output.cpp
80

Thanks!
Added the tags for the line from header file.

NoQ accepted this revision.Aug 10 2020, 8:29 PM

Let's land this! Damn, we've learned a lot from this patch.

This revision was landed with ongoing or failed builds.Aug 11 2020, 2:27 PM
This revision was automatically updated to reflect the committed changes.