This revision was automatically updated to reflect the committed changes.
Closed by commit rL332463: [analyzer] Improve the modeling of memset().
(authored by henrywong, committed by ).
Herald added a subscriber: llvm-commits.
Changed prior to commit:
https://reviews.llvm.org/D44934?vs=146816&i
NoQ accepted this revision.
NoQ added a comment.
This revision is now accepted and ready to land.
Thanks! This looks great now.
Comment at: lib/StaticAnalyzer/Checkers/CStringChecker.cpp:1055
+std::tie(StateNullChar, StateNonNullChar) =
+assumeZero(C, State, CharVal
MTC updated this revision to Diff 146816.
MTC added a comment.
- According to NoQ's suggestion, use `assumeZero()` instead of
`isZeroConstant()` to determine whether the value is 0.
- Add test `memset26_upper_UCHAR_MAX()` and `memset27_symbol()`
- Since `void *memset( void *dest, int ch, size_t c
MTC added inline comments.
Comment at: lib/StaticAnalyzer/Checkers/CStringChecker.cpp:1037
+
+if (StateWholeReg && !StateNotWholeReg && CharVal.isZeroConstant()) {
+ // If the 'memset()' acts on the whole region of destination buffer and
NoQ wrote:
> I t
NoQ added a comment.
Thank you! Looks good overall.
Comment at: lib/StaticAnalyzer/Checkers/CStringChecker.cpp:1013-1014
+
+ // Get the offset and the base region to figure out whether the offset of
+ // buffer is 0.
+ RegionOffset Offset = MR->getAsOffset();
---
MTC added a comment.
ping.
Repository:
rC Clang
https://reviews.llvm.org/D44934
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
MTC updated this revision to Diff 145361.
MTC added a comment.
- Since there is no perfect way to handle the default binding of non-zero
character, remove the default binding of non-zero character. Use
`bindDefaulrZero()` instead of `overwriteRegion()` to bind the zero character.
- Reuse `assume
MTC added a comment.
In https://reviews.llvm.org/D44934#1088771, @NoQ wrote:
> Hmm, ok, it seems that i've just changed the API in
> https://reviews.llvm.org/D46368, and i should have thought about this use
> case. Well, at least i have some background understanding of these problems
> now. So
NoQ added a comment.
Hmm, ok, it seems that i've just changed the API in
https://reviews.llvm.org/D46368, and i should have thought about this use case.
Well, at least i have some background understanding of these problems now.
Sorry for not keeping eye on this problem.
In https://reviews.llvm
MTC updated this revision to Diff 145019.
MTC added a comment.
- fix typos
- code refactoring, add auxiliary method `memsetAux()`
- according to a.sidorin's suggestions, remove the useless state splitting.
- make `StoreManager::overwriteRegion()` pure virtual
Repository:
rC Clang
https://revi
MTC added a comment.
Sorry for the long delay, I have just finished my holiday.
Thanks a lot for the review, I will fix the typo in next update.
Comment at: lib/StaticAnalyzer/Checkers/CStringChecker.cpp:2100
+std::tie(StateNullChar, StateNonNullChar) =
+assumeZero
a.sidorin added a comment.
Hi Henry. I had a quick look at the patch, here are some remarks.
Comment at: lib/StaticAnalyzer/Checkers/CStringChecker.cpp:2092
+ // 'invalidateRegions()', which will remove the pair
+ // in CStringLength map. So calls 'InvalidateBuffer()
MTC added a comment.
ping^2
Repository:
rC Clang
https://reviews.llvm.org/D44934
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
MTC added a comment.
Kindly ping!
Repository:
rC Clang
https://reviews.llvm.org/D44934
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
MTC updated this revision to Diff 140629.
MTC added a comment.
> Thank you for your reminding, I overlooked this point. However for
> non-concrete character, the symbol value, if we just invalidate the region,
> the constraint information of the non-concrete character will be lost. Do we
> need
MTC updated this revision to Diff 140405.
MTC added a comment.
According to @NoQ's suggestion, remove the duplicated code.
Repository:
rC Clang
https://reviews.llvm.org/D44934
Files:
include/clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h
include/clang/StaticAnalyzer/Core/PathSens
MTC added a comment.
Thanks for your review, NoQ!
> Why do you need separate code for null and non-null character? The function's
> semantics doesn't seem to care.
Thank you for your advice, I will remove the duplicate codeļ¼
> I'd rather consider the case of non-concrete character separately.
NoQ added a comment.
> In addition, `memset` can bind anything to the region, so
> `getBindingForDerivedDefaultValue()`'s logic needs some adjustment. **The
> solution in this patch is certainly not correct.**
Yeah, i guess you meant here the thing that i was saying about concrete
bindings. If
NoQ added a comment.
Why do you need separate code for null and non-null character? The function's
semantics doesn't seem to care.
I'd rather consider the case of non-concrete character separately. Because
wiping a region with a symbol is not something we currently support; a symbolic
default
MTC created this revision.
MTC added reviewers: dcoughlin, NoQ, xazax.hun, a.sidorin.
Herald added subscribers: cfe-commits, rnkovacs, szepet.
Herald added a reviewer: george.karpenkov.
This patch originates from https://reviews.llvm.org/D31868. There are two key
points in this
patch:
- Add `Ov
20 matches
Mail list logo
