Introducing a new argument constraint to confine buffer sizes. It is typical in
C APIs that a parameter represents a buffer and another param holds the size of
the buffer (or the size of the data we want to handle from the buffer).
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Please avoid to stuff in CheckerContext because this facility should be used by ExprEngine/Store as well.
Let us reword your API: getDynamicSizeWithOffset(ProgramStateRef, SVal, SValBuilder &). Of course we are trying to obtain some buffer-ish size, that is the purpose of the entire API.
I also could imagine something like getDynamicSizeMul(ProgramStateRef, const MemRegion &, const MemRegion &, SValBuilder &), as it is very common.
May it would make sense to use the API like:
getDynamicSizeWithOffset(State, MR, SVB) { Offset = State->getStoreManager().getStaticOffset(MR, SVB); ... }
This idea is similar to MemRegionManager::getStaticSize(MR, SVB). Hopefully no-one needs dynamic offsets.
Hm, the MemRegion's offset should be great. I was thinking about if we would store SVal offsets in the Store.
clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp | ||
---|---|---|
253 | Use BinaryOperator::negateComparisonOp |
- Rebase to master
- Use BinaryOperator::negateComparisonOp
- getBufferDynamicSize -> getDynamicSizeWithOffset
I'm not familiar enough with DynamicSize.cpp to judge the changes there, but aside from a few nits, this LGTM.
clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp | ||
---|---|---|
246–247 | Would an unreachable be appropriate here then? | |
1007 | In most places, where we refer to an argument number, we use ArgNo. Is there a reason we don't do that here? Can we enforce greater type safety? |
- REBASE to master
- Add assert
- Create the BufferSize arg constraints in a more readable way
The rebase to master makes a bit hard to see the exact changes in this update. I should have done the rebase in a separate update, sorry about that.
clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp | ||
---|---|---|
246–247 | Yes, good point, just added that. CallAndMessage is already a dependency, so this must not fire. | |
1007 | Yeah, good point, I am going with this: BufferSize(/*Buffer=*/ArgNo(0), /*BufSize=*/ArgNo(1)))); About type safety: I was thinking about a strong typedef, but we don't actually have a convenient template for that in LLVM. And most of the time here in LLVM people just apply the /*Arg=*/ pythonish form. |
LGTM! I think you could remove the extra parameter, but I don't think this warrants another round of review on my end. However, the dynamic size change seems very unfamiliar to me. @baloghadamsoftware, @balazske, could you take a peek?
clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp | ||
---|---|---|
234 | You can retrieve SValBuilder from ProgramState: State->getStateManager().getSValBuilder(). |
- Remove SValBuilder param
clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp | ||
---|---|---|
234 | Thanks! |
I failed to emphasize that the point was to remove the new CheckerContext parameters :)
You can retrieve SValBuilder from ProgramState: State->getStateManager().getSValBuilder().