vrnithinkumar 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) {
----------------
xazax.hun wrote:
> 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?
Changed to rvalue reference.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/NullDereference.h:21
+namespace ento {
+namespace nullDereference {
+
----------------
NoQ wrote:
> Namespaces are traditionally snake_case rather than camelCase.
removed the new namespace
================
Comment at: clang/lib/StaticAnalyzer/Checkers/NullDereference.h:23
+
+/// Returns NullDereferenceBugType.
+const BugType *getNullDereferenceBugType();
----------------
xazax.hun wrote:
> I think this comment does not really add much value.
Yeah.
removed it.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrChecker.cpp:110
+ SmartPtrChecker *Checker = Mgr.registerChecker<SmartPtrChecker>();
+ NullDereferenceBugTypePtr = &Checker->NullDereferenceBugType;
}
----------------
NoQ wrote:
> vrnithinkumar wrote:
> > xazax.hun wrote:
> > > 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.
> > > 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?
> > 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.
Removed the new header file and added to `SmartPtr.h`
================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:131
+ }
+ return nullptr;
+}
----------------
NoQ wrote:
> You never ever check for this case. Therefore this function entirely boils
> down to `Call.getArgExpr(0)` which is shorter than `getFirstArgExpr(Call)`
> anyway.
removed it and inlined.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:134
+
+static std::string getSmartPtrName(const MemRegion *Region) {
+ std::string SmartPtrName = "";
----------------
xazax.hun wrote:
> I wonder whether `MemRegion::printPretty` is what we want here.
Yeah Thanks.
This is what we exactly needed.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:199
+ } else {
+ const auto *TrackingExpr = getFirstArgExpr(Call);
+ C.addTransition(State, C.getNoteTag([ThisRegion, TrackingExpr](
----------------
NoQ wrote:
> xazax.hun wrote:
> > 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.
> 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.
Added a check if the value is null or not and add message based on 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 {
----------------
vrnithinkumar wrote:
> xazax.hun wrote:
> > 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.
> Also we have to assert that we are adding a pointer value to TrackedRegionMap.
Removed the function and inlined it
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D84600/new/
https://reviews.llvm.org/D84600
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
