xazax.hun added inline comments.
================
Comment at:
clang/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h:312
+ const NoteTag *getNoteTag(
+ std::function<void(PathSensitiveBugReport &BR, llvm::raw_ostream &OS)>
Cb,
+ bool IsPrunable = false) {
----------------
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?
================
Comment at: clang/lib/StaticAnalyzer/Checkers/NullDereference.h:23
+
+/// Returns NullDereferenceBugType.
+const BugType *getNullDereferenceBugType();
----------------
I think this comment does not really add much value.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrChecker.cpp:110
+ SmartPtrChecker *Checker = Mgr.registerChecker<SmartPtrChecker>();
+ NullDereferenceBugTypePtr = &Checker->NullDereferenceBugType;
}
----------------
NoQ wrote:
> vrnithinkumar wrote:
> > NoQ wrote:
> > > 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?
> > > 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`
> 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.
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.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:134
+
+static std::string getSmartPtrName(const MemRegion *Region) {
+ std::string SmartPtrName = "";
----------------
I wonder whether `MemRegion::printPretty` is what we want here.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:199
+ } else {
+ const auto *TrackingExpr = getFirstArgExpr(Call);
+ C.addTransition(State, C.getNoteTag([ThisRegion, TrackingExpr](
----------------
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.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:352
+// Gets the SVal to track for a smart pointer memory region
+SVal SmartPtrModeling::getSValForRegion(const CallEvent &Call,
+ CheckerContext &C) const {
----------------
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.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D84600/new/
https://reviews.llvm.org/D84600
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
