[clang] [clang][analyzer] Simplify code of StreamChecker (NFC). (PR #79312)
https://github.com/balazske closed https://github.com/llvm/llvm-project/pull/79312 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][analyzer] Simplify code of StreamChecker (NFC). (PR #79312)
=?utf-8?q?Bal=C3=A1zs_K=C3=A9ri?= Message-ID: In-Reply-To: https://github.com/NagyDonat approved this pull request. LGTM, this is a nice little cleanup patch. https://github.com/llvm/llvm-project/pull/79312 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][analyzer] Simplify code of StreamChecker (NFC). (PR #79312)
https://github.com/balazske updated
https://github.com/llvm/llvm-project/pull/79312
From 62dc7fdb2f86c81af501f7f1255a98d97ede303f Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Bal=C3=A1zs=20K=C3=A9ri?=
Date: Wed, 24 Jan 2024 16:28:57 +0100
Subject: [PATCH 1/2] [clang][analyzer] Simplify code of StreamChecker (NFC).
Common parts of some "eval" functions are moved into one function.
The non-common parts of the "eval" functions are passed through
lambda parameters to the new function.
---
.../StaticAnalyzer/Checkers/StreamChecker.cpp | 438 +-
1 file changed, 230 insertions(+), 208 deletions(-)
diff --git a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
index 07727b339d967..95e7e5d9f0912 100644
--- a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
@@ -400,6 +400,51 @@ class StreamChecker : public Checker;
+using GetSuccessStateFn = std::function;
+using GetFailureStateFn =
+std::function(
+SymbolRef, const CallExpr *, ASTContext &, SValBuilder &)>;
+
+EvalRWCommonFn(InitFn I, GetSuccessStateFn GSS, GetFailureStateFn GFS)
+: Init(I), GetSuccessState(GSS), GetFailureState(GFS) {}
+EvalRWCommonFn(GetSuccessStateFn GSS, GetFailureStateFn GFS)
+: Init([](SymbolRef, const CallExpr *, ASTContext &, SValBuilder &) {
+return nullptr;
+ }),
+ GetSuccessState(GSS), GetFailureState(GFS) {}
+
+// Called at start of the evaluation.
+// Should return 'nullptr' to continue evaluation, or a program state that
+// is added and evaluation stops here.
+// This is used to handle special cases when no success or failure state
+// is added.
+InitFn Init;
+// Create the state for the "success" case of the function and return it.
+// Can return 'nullptr' to not add a success state.
+GetSuccessStateFn GetSuccessState;
+// Create the state for the "failure" case of the function.
+// Return the state and the new stream error state (combination of possible
+// errors).
+GetFailureStateFn GetFailureState;
+ };
+
+ /// Common part of many eval functions in the checker.
+ /// This can be used for simple stream read and write functions.
+ /// This function handles full modeling of such functions, by passing a
+ /// 'GetSuccessState' and 'GetFailureState' function. These functions should
+ /// create the program state for the success and failure cases. 'IsRead'
+ /// indicates if the modeled function is a stream read or write operation.
+ /// If the evaluated function is a read operation and the stream is already
+ /// in EOF, the new error will always be EOF and no success state is added.
+ /// This case is handled here and 'GetFailureState' should not care about it.
+ void evalRWCommon(const FnDescription *Desc, const CallEvent &Call,
+CheckerContext &C, bool IsRead, EvalRWCommonFn Fn) const;
+
/// Check that the stream (in StreamVal) is not NULL.
/// If it can only be NULL a fatal error is emitted and nullptr returned.
/// Otherwise the return value is a new state where the stream is constrained
@@ -726,225 +771,140 @@ void StreamChecker::preReadWrite(const FnDescription
*Desc,
void StreamChecker::evalFreadFwrite(const FnDescription *Desc,
const CallEvent &Call, CheckerContext &C,
bool IsFread) const {
- ProgramStateRef State = C.getState();
- SymbolRef StreamSym = getStreamArg(Desc, Call).getAsSymbol();
- if (!StreamSym)
-return;
-
- const CallExpr *CE = dyn_cast_or_null(Call.getOriginExpr());
- if (!CE)
-return;
-
- std::optional SizeVal = Call.getArgSVal(1).getAs();
- if (!SizeVal)
-return;
- std::optional NMembVal = Call.getArgSVal(2).getAs();
- if (!NMembVal)
-return;
-
- const StreamState *OldSS = State->get(StreamSym);
- if (!OldSS)
-return;
-
- assertStreamStateOpened(OldSS);
-
- // C'99 standard, §7.19.8.1.3, the return value of fread:
- // The fread function returns the number of elements successfully read, which
- // may be less than nmemb if a read error or end-of-file is encountered. If
- // size or nmemb is zero, fread returns zero and the contents of the array
and
- // the state of the stream remain unchanged.
-
- if (State->isNull(*SizeVal).isConstrainedTrue() ||
- State->isNull(*NMembVal).isConstrainedTrue()) {
-// This is the "size or nmemb is zero" case.
-// Just return 0, do nothing more (not clear the error flags).
-State = bindInt(0, State, C, CE);
-C.addTransition(State);
-return;
- }
-
- // Generate a transition for the success state.
- // If we know the state to be FEOF at fread, do not add a success state.
- if (!IsFread || (OldSS->ErrorState != ErrorFEof)) {
-ProgramStateRef StateNotFailed =
-State->BindExpr(CE, C.getLocationContext(), *NMembVal);
-
[clang] [clang][analyzer] Simplify code of StreamChecker (NFC). (PR #79312)
benshi001 wrote: For the part in the end of most `evalXX` functions, ``` StateFailed = ... StateNotFailed = ... ``` They are quite similar but not identical, so we can generalize them with helper functions. https://github.com/llvm/llvm-project/pull/79312 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][analyzer] Simplify code of StreamChecker (NFC). (PR #79312)
benshi001 wrote: I do not like too long lambda either. In my opinion lambda should be short/compact. And I do not think the redundancy is serious, except the common part ``` ProgramStateRef State = C.getState(); SymbolRef StreamSym = getStreamArg(Desc, Call).getAsSymbol(); if (!StreamSym) return; const CallExpr *CE = dyn_cast_or_null(Call.getOriginExpr()); if (!CE) return; const StreamState *OldSS = State->get(StreamSym); if (!OldSS) return; assertStreamStateOpened(OldSS); ``` which appears in the beginning of most `evalXX` functions. And can we replace this piece with a macro ? https://github.com/llvm/llvm-project/pull/79312 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][analyzer] Simplify code of StreamChecker (NFC). (PR #79312)
balazske wrote: For clarification, the `evalRWCommon` new function is not planned to be used in the same way as other `eval*` calls, the name may be misleading. This function contains a "generic algorithm" that is called from the other `eval*` calls, but not used directly in `CallDescriptionMap`. The old `eval*` calls and the `evalRWCommon` are not meant to be called from each other, except `evalRWCommon` from other `eval*` calls. I agree that using lambdas makes debugging more difficult and lambdas are not ideal for big parts of code and code formatting is not nice either. https://github.com/llvm/llvm-project/pull/79312 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][analyzer] Simplify code of StreamChecker (NFC). (PR #79312)
NagyDonat wrote: Yes, the "common big set of arguments" is an annoying problem. Perhaps you could consider my solution in core.BitwiseShift where I introduced a "validator" class that holds all the common arguments as data members, so instead of helper functions you have helper methods that can all access the common arguments. (The checker callback constructs a validator, and calls its "main" method, which will call the others as needed) https://github.com/llvm/llvm-project/pull/79312 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][analyzer] Simplify code of StreamChecker (NFC). (PR #79312)
balazske wrote: My concern was the addition of many small functions that are used only from few other `eval*` calls and are relatively special. And all of these need a common big set of arguments like `StreamSym`, `CE`, `Call`. The build of states for success and failure cases in the thing that is really special for the calls. But I can make another solution with "helper" functions. https://github.com/llvm/llvm-project/pull/79312 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][analyzer] Simplify code of StreamChecker (NFC). (PR #79312)
steakhal wrote: > I'm seconding the suggestions of @steakhal, and in particular I agree with > > > I'd also advise against using more callables bundled with CallDescriptions. > > They make debugging code more difficult, as the control-flow would become > > also data-dependent. I'd suggest other ways to generalize. > > Instead of creating this shared framework that calls different callbacks > depending on the checked function, consider keeping separate top-level > evaluator functions that rely on a shared set of smaller tool-like helper > functions. One more not about composability. In general, `addTransition` calls don't compose, thus limits reusability. For example, `preReadWrite` should be reusable from other precondition handlers, such that the new one can just call `preReadWrite` and then do some other more specialized checks. However, once a handler calls `addTransition` we are done. We no longer can simply wrap it, aka. compose with other functionalities. Consequently, I'd suggest to have some common pattern, such as return state pointers, or NULL if an error was reported. And leave the `addTransition` to the caller - if they chose to call it. `eval*` handlers are more difficult to compose, given that they also need to bind the return value and apply their sideffects, but I believe we could achieve similar objectives there too (thus, have refined, composable handlers, combined in useful ways). https://github.com/llvm/llvm-project/pull/79312 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][analyzer] Simplify code of StreamChecker (NFC). (PR #79312)
NagyDonat wrote: I'm seconding the suggestions of @steakhal, and in particular I agree with > I'd also advise against using more callables bundled with CallDescriptions. > They make debugging code more difficult, as the control-flow would become > also data-dependent. I'd suggest other ways to generalize. Instead of creating this shared framework that calls different callbacks depending on the checked function, consider keeping separate top-level evaluator functions that rely on a shared set of smaller tool-like helper functions. https://github.com/llvm/llvm-project/pull/79312 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][analyzer] Simplify code of StreamChecker (NFC). (PR #79312)
benshi001 wrote: This change is a bit complex and I need more time to understand. https://github.com/llvm/llvm-project/pull/79312 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][analyzer] Simplify code of StreamChecker (NFC). (PR #79312)
steakhal wrote: I'm yet to review the PR, but I would express my opinion on the ergonomics of the StreamChecker, as I've spent the last couple of days around it. I find code duplication less harmful than unnatural generalization over small set of functions (I know, it's a hot take :D). `std::bind` is harmful, and specific handlers should be preferred. As a rule of thumb, if we need a flag on a top-level API, then that's likely a code-smell. E.g.: There presence of `IsRead` or `IsSingleChar` (that we need to bind), signals to me that there should be a specific API for the read, write, singleChar and buffer cases. Internally, those could dispatch to a generic call, which has this flag - but at least on the use-sites, we don't need to use `std::bind`, but naturally just taking the address of the member function without any tricks. I'd also advise against using more callables bundled with CallDescriptions. They make debugging code more difficult, as the control-flow would become also data-dependent. I'd suggest other ways to generalize. If I have some spare time in the next days, I'm going to also look deeper and come up with actual recommendations. https://github.com/llvm/llvm-project/pull/79312 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][analyzer] Simplify code of StreamChecker (NFC). (PR #79312)
balazske wrote: Later more functions can be simplified with `evalRWCommon`, probably in a next patch to make the review more simple. https://github.com/llvm/llvm-project/pull/79312 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][analyzer] Simplify code of StreamChecker (NFC). (PR #79312)
llvmbot wrote:
@llvm/pr-subscribers-clang-static-analyzer-1
Author: Balázs Kéri (balazske)
Changes
Common parts of some "eval" functions are moved into one function. The
non-common parts of the "eval" functions are passed through lambda parameters
to the new function.
---
Full diff: https://github.com/llvm/llvm-project/pull/79312.diff
1 Files Affected:
- (modified) clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp (+230-208)
``diff
diff --git a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
index 07727b339d967ae..95e7e5d9f0912cf 100644
--- a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
@@ -400,6 +400,51 @@ class StreamChecker : public Checker;
+using GetSuccessStateFn = std::function;
+using GetFailureStateFn =
+std::function(
+SymbolRef, const CallExpr *, ASTContext &, SValBuilder &)>;
+
+EvalRWCommonFn(InitFn I, GetSuccessStateFn GSS, GetFailureStateFn GFS)
+: Init(I), GetSuccessState(GSS), GetFailureState(GFS) {}
+EvalRWCommonFn(GetSuccessStateFn GSS, GetFailureStateFn GFS)
+: Init([](SymbolRef, const CallExpr *, ASTContext &, SValBuilder &) {
+return nullptr;
+ }),
+ GetSuccessState(GSS), GetFailureState(GFS) {}
+
+// Called at start of the evaluation.
+// Should return 'nullptr' to continue evaluation, or a program state that
+// is added and evaluation stops here.
+// This is used to handle special cases when no success or failure state
+// is added.
+InitFn Init;
+// Create the state for the "success" case of the function and return it.
+// Can return 'nullptr' to not add a success state.
+GetSuccessStateFn GetSuccessState;
+// Create the state for the "failure" case of the function.
+// Return the state and the new stream error state (combination of possible
+// errors).
+GetFailureStateFn GetFailureState;
+ };
+
+ /// Common part of many eval functions in the checker.
+ /// This can be used for simple stream read and write functions.
+ /// This function handles full modeling of such functions, by passing a
+ /// 'GetSuccessState' and 'GetFailureState' function. These functions should
+ /// create the program state for the success and failure cases. 'IsRead'
+ /// indicates if the modeled function is a stream read or write operation.
+ /// If the evaluated function is a read operation and the stream is already
+ /// in EOF, the new error will always be EOF and no success state is added.
+ /// This case is handled here and 'GetFailureState' should not care about it.
+ void evalRWCommon(const FnDescription *Desc, const CallEvent &Call,
+CheckerContext &C, bool IsRead, EvalRWCommonFn Fn) const;
+
/// Check that the stream (in StreamVal) is not NULL.
/// If it can only be NULL a fatal error is emitted and nullptr returned.
/// Otherwise the return value is a new state where the stream is constrained
@@ -726,225 +771,140 @@ void StreamChecker::preReadWrite(const FnDescription
*Desc,
void StreamChecker::evalFreadFwrite(const FnDescription *Desc,
const CallEvent &Call, CheckerContext &C,
bool IsFread) const {
- ProgramStateRef State = C.getState();
- SymbolRef StreamSym = getStreamArg(Desc, Call).getAsSymbol();
- if (!StreamSym)
-return;
-
- const CallExpr *CE = dyn_cast_or_null(Call.getOriginExpr());
- if (!CE)
-return;
-
- std::optional SizeVal = Call.getArgSVal(1).getAs();
- if (!SizeVal)
-return;
- std::optional NMembVal = Call.getArgSVal(2).getAs();
- if (!NMembVal)
-return;
-
- const StreamState *OldSS = State->get(StreamSym);
- if (!OldSS)
-return;
-
- assertStreamStateOpened(OldSS);
-
- // C'99 standard, §7.19.8.1.3, the return value of fread:
- // The fread function returns the number of elements successfully read, which
- // may be less than nmemb if a read error or end-of-file is encountered. If
- // size or nmemb is zero, fread returns zero and the contents of the array
and
- // the state of the stream remain unchanged.
-
- if (State->isNull(*SizeVal).isConstrainedTrue() ||
- State->isNull(*NMembVal).isConstrainedTrue()) {
-// This is the "size or nmemb is zero" case.
-// Just return 0, do nothing more (not clear the error flags).
-State = bindInt(0, State, C, CE);
-C.addTransition(State);
-return;
- }
-
- // Generate a transition for the success state.
- // If we know the state to be FEOF at fread, do not add a success state.
- if (!IsFread || (OldSS->ErrorState != ErrorFEof)) {
-ProgramStateRef StateNotFailed =
-State->BindExpr(CE, C.getLocationContext(), *NMembVal);
-StateNotFailed =
-StateNotFailed->set(StreamSym,
StreamState::getOpened(Desc));
-C.addTransition(StateNotFailed);
- }
-
- // Add tran
[clang] [clang][analyzer] Simplify code of StreamChecker (NFC). (PR #79312)
https://github.com/balazske created
https://github.com/llvm/llvm-project/pull/79312
Common parts of some "eval" functions are moved into one function. The
non-common parts of the "eval" functions are passed through lambda parameters
to the new function.
From 62dc7fdb2f86c81af501f7f1255a98d97ede303f Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Bal=C3=A1zs=20K=C3=A9ri?=
Date: Wed, 24 Jan 2024 16:28:57 +0100
Subject: [PATCH] [clang][analyzer] Simplify code of StreamChecker (NFC).
Common parts of some "eval" functions are moved into one function.
The non-common parts of the "eval" functions are passed through
lambda parameters to the new function.
---
.../StaticAnalyzer/Checkers/StreamChecker.cpp | 438 +-
1 file changed, 230 insertions(+), 208 deletions(-)
diff --git a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
index 07727b339d967a..95e7e5d9f0912c 100644
--- a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
@@ -400,6 +400,51 @@ class StreamChecker : public Checker;
+using GetSuccessStateFn = std::function;
+using GetFailureStateFn =
+std::function(
+SymbolRef, const CallExpr *, ASTContext &, SValBuilder &)>;
+
+EvalRWCommonFn(InitFn I, GetSuccessStateFn GSS, GetFailureStateFn GFS)
+: Init(I), GetSuccessState(GSS), GetFailureState(GFS) {}
+EvalRWCommonFn(GetSuccessStateFn GSS, GetFailureStateFn GFS)
+: Init([](SymbolRef, const CallExpr *, ASTContext &, SValBuilder &) {
+return nullptr;
+ }),
+ GetSuccessState(GSS), GetFailureState(GFS) {}
+
+// Called at start of the evaluation.
+// Should return 'nullptr' to continue evaluation, or a program state that
+// is added and evaluation stops here.
+// This is used to handle special cases when no success or failure state
+// is added.
+InitFn Init;
+// Create the state for the "success" case of the function and return it.
+// Can return 'nullptr' to not add a success state.
+GetSuccessStateFn GetSuccessState;
+// Create the state for the "failure" case of the function.
+// Return the state and the new stream error state (combination of possible
+// errors).
+GetFailureStateFn GetFailureState;
+ };
+
+ /// Common part of many eval functions in the checker.
+ /// This can be used for simple stream read and write functions.
+ /// This function handles full modeling of such functions, by passing a
+ /// 'GetSuccessState' and 'GetFailureState' function. These functions should
+ /// create the program state for the success and failure cases. 'IsRead'
+ /// indicates if the modeled function is a stream read or write operation.
+ /// If the evaluated function is a read operation and the stream is already
+ /// in EOF, the new error will always be EOF and no success state is added.
+ /// This case is handled here and 'GetFailureState' should not care about it.
+ void evalRWCommon(const FnDescription *Desc, const CallEvent &Call,
+CheckerContext &C, bool IsRead, EvalRWCommonFn Fn) const;
+
/// Check that the stream (in StreamVal) is not NULL.
/// If it can only be NULL a fatal error is emitted and nullptr returned.
/// Otherwise the return value is a new state where the stream is constrained
@@ -726,225 +771,140 @@ void StreamChecker::preReadWrite(const FnDescription
*Desc,
void StreamChecker::evalFreadFwrite(const FnDescription *Desc,
const CallEvent &Call, CheckerContext &C,
bool IsFread) const {
- ProgramStateRef State = C.getState();
- SymbolRef StreamSym = getStreamArg(Desc, Call).getAsSymbol();
- if (!StreamSym)
-return;
-
- const CallExpr *CE = dyn_cast_or_null(Call.getOriginExpr());
- if (!CE)
-return;
-
- std::optional SizeVal = Call.getArgSVal(1).getAs();
- if (!SizeVal)
-return;
- std::optional NMembVal = Call.getArgSVal(2).getAs();
- if (!NMembVal)
-return;
-
- const StreamState *OldSS = State->get(StreamSym);
- if (!OldSS)
-return;
-
- assertStreamStateOpened(OldSS);
-
- // C'99 standard, §7.19.8.1.3, the return value of fread:
- // The fread function returns the number of elements successfully read, which
- // may be less than nmemb if a read error or end-of-file is encountered. If
- // size or nmemb is zero, fread returns zero and the contents of the array
and
- // the state of the stream remain unchanged.
-
- if (State->isNull(*SizeVal).isConstrainedTrue() ||
- State->isNull(*NMembVal).isConstrainedTrue()) {
-// This is the "size or nmemb is zero" case.
-// Just return 0, do nothing more (not clear the error flags).
-State = bindInt(0, State, C, CE);
-C.addTransition(State);
-return;
- }
-
- // Generate a transition for the success state.
- // If we know the state to be FEOF at fread, do not add a succes
