Further develop the buffer size argumentum constraint so it can handle sizes
that we can get by multiplying two variables.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
clang/test/Analysis/std-c-library-functions-arg-constraints.c | ||
---|---|---|
163 | s <= 3 is better here? We know that the buffer has space for 3 short's. But it is better to assume nothing about sizeof(short). |
clang/test/Analysis/std-c-library-functions-arg-constraints.c | ||
---|---|---|
163 | That would have been better if the range based constraint solver was able to solve the in-equation to s. So, even though the analyzer knows that s * 2 <= 6 it cannot reason about s itself. This is the limitation of the solver. I believe @baloghadamsoftware has a few patches that directs this deficiency: D49074 and D50256 |
clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp | ||
---|---|---|
231–239 | I don't know, these constructors don't look overly talkative. | |
986 | So this is supposed to mean that the 1st argument has the size of the argument of 2nd argument times the 3rd argument? Unlike the other summaries, this feels a bit cryptic. Something like this might look better: .ArgConstraint(0, BufferSize(/*BufSize*/1, /*BufSizeMultiplier*/)) .ArgConstraint(BufferSize(/*Buffer*/0, /*BufSize*/1, /*BufSizeMultiplier*/2)) .ArgConstraint(0, BufferSizeMul(1, 2)) WDYT? |
Overall looks good to me, I have one question inline.
clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp | ||
---|---|---|
1008 | Why do we need these test functions? Above I saw fread as an example that requires this capability. Wouldn't it be better to make its summary utilize the new feature and use fread in tests? Do I miss something? |
clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp | ||
---|---|---|
1008 | Yeah, we could test that with fread. However, in fread there are multiple argument constraints for the different args. I wanted a test function which has this arg constraint in an isolation. In my opinion it is good to have small isolated unit tests that test only one functionality. Also if we decide to remove or modify freads summary for any reason, then we should modify this test too, on the other hand, with this test function it is not a problem. |
clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp | ||
---|---|---|
1008 | I see. The plan in the future is to split this checker up a bit. In this case I'd like to have these test functions in a separate test checker. Until then I'm fine with a FIXME/TODO. |
clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp | ||
---|---|---|
1008 | They are already enabled only if a test checker is enabled, check out the enclosing if of this block: if (ChecksEnabled[CK_StdCLibraryFunctionsTesterChecker]) { |
clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp | ||
---|---|---|
1008 | Oh, sorry, my bad :) |
fread-like