OikawaKirie added a comment. BTW, what does the `Done` checkbox mean in the code comments?
================ Comment at: clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp:286-287 + SVal V, + std::function<bool(RegionAndSymbolInvalidationTraits &, + const MemRegion *)>); ---------------- 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); ``` ================ Comment at: clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp:1054 + SVal SizeV, QualType SizeTy) { + return InvalidateBufferAux( + C, S, BufE, BufV, ---------------- 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. ================ Comment at: clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp:1075-1077 + [](RegionAndSymbolInvalidationTraits &, const MemRegion *R) { + return MemRegion::FieldRegionKind == R->getKind(); + }); ---------------- 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>. ================ 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. ---------------- > 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. ================ Comment at: clang/test/Analysis/pr22954.c:560 clang_analyzer_eval(x263.s1[0] == 0); // expected-warning{{UNKNOWN}} + // expected-warning@-1{{Potential leak of memory pointed to by 'x263.s2'}} clang_analyzer_eval(x263.s1[1] == 0); // expected-warning{{UNKNOWN}} ---------------- steakhal wrote: > Ah, the leak should have been reported at the end of the full-expression of > `memcpy`. > If we have this expect line here it could mislead the reader that it has > anything to do with the `eval` call before. > > In fact, any statement after `memcpy` would have this diagnostic. > I have seen cases where we have a `next_line()` opaque call after such places > to anchor the leak diagnostic. > I think we can do the same here. > Its not advised to have different, but similar looking hunks out of which > some are the consequence of the semantic change we made and also have others > where they are just refactors. Do you mean I only need to update the verification direction here as you suggested above and leave others unchanged? 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