Page MenuHomePhabricator

[Analyzer] Support note tags for smart ptr checker
Needs ReviewPublic

Authored by vrnithinkumar on Sun, Jul 26, 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

Unit TestsFailed

TimeTest
10,590 mslinux > libomp.env::kmp_set_dispatch_buf.c
Script: -- : 'RUN: at line 1'; /mnt/disks/ssd0/agent/llvm-project/build/./bin/clang -fopenmp -pthread -fno-experimental-isel -I /mnt/disks/ssd0/agent/llvm-project/openmp/runtime/test -I /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/runtime/src -L /mnt/disks/ssd0/agent/llvm-project/build/lib -I /mnt/disks/ssd0/agent/llvm-project/openmp/runtime/test/ompt /mnt/disks/ssd0/agent/llvm-project/openmp/runtime/test/env/kmp_set_dispatch_buf.c -o /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/runtime/test/env/Output/kmp_set_dispatch_buf.c.tmp -lm -latomic
1,680 mslinux > libomp.worksharing/for::kmp_set_dispatch_buf.c
Script: -- : 'RUN: at line 1'; /mnt/disks/ssd0/agent/llvm-project/build/./bin/clang -fopenmp -pthread -fno-experimental-isel -I /mnt/disks/ssd0/agent/llvm-project/openmp/runtime/test -I /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/runtime/src -L /mnt/disks/ssd0/agent/llvm-project/build/lib -I /mnt/disks/ssd0/agent/llvm-project/openmp/runtime/test/ompt /mnt/disks/ssd0/agent/llvm-project/openmp/runtime/test/worksharing/for/kmp_set_dispatch_buf.c -o /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/runtime/test/worksharing/for/Output/kmp_set_dispatch_buf.c.tmp -lm -latomic && /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/runtime/test/worksharing/for/Output/kmp_set_dispatch_buf.c.tmp 7

Event Timeline

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

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

112–113

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
151

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";
}));
370–374

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.Sun, Jul 26, 5:58 PM
clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp
151

(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)

370–374

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

vrnithinkumar added inline comments.Fri, Jul 31, 5:09 PM
clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp
370–374

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.Fri, Jul 31, 5:23 PM
clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp
370–374

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.Fri, Jul 31, 5:27 PM
clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp
370–374

That's an interesting question

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

vrnithinkumar added inline comments.Fri, Jul 31, 5:31 PM
clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp
370–374

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.Sat, Aug 1, 1:37 PM
clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp
370–374

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.Sun, Aug 2, 8:56 PM
clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp
370–374

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
103

Changed to use getNameAsString()

112–113

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

clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp
151
  • 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.
370–374

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

Added the tests

370–374

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

370–374
  • 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.Mon, Aug 3, 8:40 PM
clang/lib/StaticAnalyzer/Checkers/NullDereference.h
21

Namespaces are traditionally snake_case rather than camelCase.

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

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?

112–113

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.Tue, Aug 4, 9:33 AM
clang/lib/StaticAnalyzer/Checkers/SmartPtrChecker.cpp
110

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

112–113

Oh
That makes sense.

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

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.Tue, Aug 4, 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
131

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.Tue, Aug 4, 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

I think this comment does not really add much value.

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

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
134

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

199

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.

352

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.Wed, Aug 5, 12:26 PM
clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp
199

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.Wed, Aug 5, 2:51 PM
clang/lib/StaticAnalyzer/Checkers/SmartPtrChecker.cpp
110

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
352

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

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

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.