steakhal added a comment.
In D152435#4433147 <https://reviews.llvm.org/D152435#4433147>, @OikawaKirie
wrote:
> BTW, what does the `Done` checkbox mean in the code comments?
"Done" means "Resolved" on GitHub - which is basically that some action was
taken or the person who raised the issue withdrew.
In general, no revision/pull request should be merged with having
open/unresolved discussions.
Usually, the patch author marks comments as "Done" and on the next update (by
pressing the "Submit" button) all those threads will be closed automatically.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp:286-287
+ SVal V,
+ std::function<bool(RegionAndSymbolInvalidationTraits &,
+ const MemRegion *)>);
----------------
OikawaKirie wrote:
> steakhal wrote:
> > I'd highly suggest making this a template taking the functor that way.
> > Given that we modify these functions, we should make them comply with the
> > lower camel case naming convention.
> > And their variables with upper camel case.
> > I'd highly suggest making this a template taking the functor that way.
>
> Any examples?
> Do you mean
> ```
> template <typename T>
> static ProgramStateRef
> InvalidateBufferAux(CheckerContext &C, ProgramStateRef state, const Expr *Ex,
> SVal V, std::function<T> CallBack);
> ```
My problem is that `std::function` owns the callback, and really likely to
allocate for doing so.
And in this context a more lightweight approach would be also good.
What I had in mind was:
```lang=C++
template <typename Callback>
static ProgramStateRef
InvalidateBufferAux(CheckerContext &C, ProgramStateRef state, const Expr *Ex,
SVal V, Callback F);
...
bool asd = F(a, b);
```
But actually, `llvm::function_ref` would be probably even better, so use that
instead.
Also, add some comments on what the callback's return type means etc.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp:1054
+ SVal SizeV, QualType SizeTy) {
+ return InvalidateBufferAux(
+ C, S, BufE, BufV,
----------------
OikawaKirie wrote:
> steakhal wrote:
> > Shouldn't we assert that `SizeV` is some known value or at least smaller
> > than (or equal to) the extent of the buffer?
> The calls in `CStringChecker::evalCopyCommon` and
> `CStringChecker::evalStrcpyCommon` seem to have chances able to pass an
> unknown value. But I am not very sure about this.
>
> If the `SizeV` is sure to be smaller or equal to the extent of the buffer,
> the `IsFirstBufInBound` check seems useless.
> Maybe I need to dig deeper into the callers.
Indeed.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp:1075-1077
+ [](RegionAndSymbolInvalidationTraits &, const MemRegion *R) {
+ return MemRegion::FieldRegionKind == R->getKind();
+ });
----------------
OikawaKirie wrote:
> steakhal wrote:
> > The lambdas inline in the call look a bit messy. I would prefer declaring
> > it and passing it as an argument separately.
> > This applies to the rest of the lambdas as well.
> > And their variables with upper camel case.
>
> What about the variable names for the lambdas? Lower camel or upper camel?
> I cannot find the rules for named lambdas from the guidance
> <https://llvm.org/docs/CodingStandards.html>.
I think I asked the same question some time ago.
https://discourse.llvm.org/t/naming-lambdas-in-llvm-coding-standards/62689/3
================
Comment at: clang/test/Analysis/issue-55019.cpp:80-87
+ std::copy(p, p + 1, x.arr);
+
+ // FIXME: As we currently cannot know whether the copy overflows, the checker
+ // invalidates the entire `x` object. When the copy size through iterators
+ // can be correctly modeled, we can then update the verify direction from
+ // SymRegion to HeapSymRegion as this std::copy call never overflows and
+ // hence the pointer `x.ptr` shall not be invalidated.
----------------
OikawaKirie wrote:
> > So, you say that It should be `HeapSymRegion` instead because `x.arr`
> > shouldn't be invalidated because the `std::copy` won't modify it. Right?
>
> Yes, but just in this test case.
>
> The size of `x.arr` is 4 and the copy length is 1, which means the call to
> `std::copy` here never overflows.
> Therefore, we should not invalidate the super region `x` and keep `x.ptr`
> still pointing to the original `HeapSymRegion` unchanged.
> However, the copy length computed through iterators is not modeled currently
> in `CStringChecker::evalStdCopyCommon`, where
> `InvalidateDestinationBufferAlwaysEscapeSuperRegion` is called.
> When it is modeled in the future, the function should call
> `InvalidateDestinationBufferBySize` instead to make the invalidation of the
> super region determined by the copy length to report potential leaks.
Makes sense. Thanks.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D152435/new/
https://reviews.llvm.org/D152435
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
