pgousseau added a comment.
In http://reviews.llvm.org/D11832#224929, @dcoughlin wrote:
> You should consider what should happen when the memcpy may write past the end
> of the fixed-size array and add tests that specify correct behavior for these
> cases. An important example is:
>
> struct Foo {
> char data[4];
> int i;
> };
>
> Foo f;
> f.i = 10;
>
> memcpy(f.data, someBuf, 100);
>
> clang_analyzer_eval(f.i == 10); // What should this yield?
>
>
> I think it is also important to add tests for regions at symbolic offsets,
> for bindings in the super region having keys with symbolic offsets, and for
> cases where there is potential aliasing and casting between regions with
> symbolic offsets.
Yes it seems I overlooked symbolic offsets in the test cases, will handle those.
> Also, Jordan wrote up a description of the region store in
> docs/analyzer/RegionStore.txt that you might find helpful if you haven't
> already seen it.
Very usefull thanks !
================
Comment at: lib/StaticAnalyzer/Core/RegionStore.cpp:746
@@ -739,1 +745,3 @@
+ return static_cast<DERIVED*>(this)->hasTrait(MR, IK);
+ }
};
----------------
ayartsev wrote:
> Hmm.. Either we completely encapsulate 'RegionAndSymbolInvalidationTraits' in
> the 'invalidateRegionsWorker' class or we move
> 'RegionAndSymbolInvalidationTraits' (maybe renamed to the more general name)
> to the base 'ClusterAnalysis' class.
> In the suggested solution you on the one hand make processing of
> 'RegionAndSymbolInvalidationTraits' specific to 'invalidateRegionsWorker'
> class but on the other hand explicitly refer to
> 'RegionAndSymbolInvalidationTraits::InvalidationKinds' and
> 'RegionAndSymbolInvalidationTraits::TK_DoNotInvalidateSuperRegion' in the
> 'ClusterAnalysis' class.
> It seems to me the proper way to encapsulate
> 'RegionAndSymbolInvalidationTraits' in the 'invalidateRegionsWorker' is to
> just override 'AddToWorkList()' in subclasses (as you done with 'hasTrait()').
I agree yes overriding AddToWorkList would fit better, will upload new patch.
Thanks !
================
Comment at: lib/StaticAnalyzer/Core/RegionStore.cpp:1110
@@ +1109,3 @@
+ assert(RO.getOffset() >= 0 && "Offset should not be negative");
+ uint64_t LowerOffset = RO.getOffset();
+ uint64_t UpperOffset = LowerOffset + *NumElements * ElemSize;
----------------
dcoughlin wrote:
> R0.getOffset() will assert if R0 is a symbolic region offset. This can happen
> if the invalidated array is itself in an array (e.g.,
> someOtherArray[i].array) or is in a union.
Yes that definitely needs fixing. Thanks !
================
Comment at: lib/StaticAnalyzer/Core/RegionStore.cpp:1118
@@ +1117,3 @@
+ ++I) {
+ uint64_t ROffset = I.getKey().getOffset();
+ if (ROffset >= LowerOffset && ROffset <= UpperOffset)
----------------
dcoughlin wrote:
> getOffset() here will assert also if there is any key with a symbolic offset
> in SuperR.
Will fix. Thanks !
================
Comment at: lib/StaticAnalyzer/Core/RegionStore.cpp:1119
@@ +1118,3 @@
+ uint64_t ROffset = I.getKey().getOffset();
+ if (ROffset >= LowerOffset && ROffset <= UpperOffset)
+ B = B.removeBinding(I.getKey());
----------------
dcoughlin wrote:
> Should this be ROffset < UpperOffset?
Yes, will change to (ROffset < UpperOffset || (LowerOffset == UpperOffset &&
ROffset == LowerOffset)).
To handle arrays of 0 elements and arrays of 0 sized elements.
Thanks !
http://reviews.llvm.org/D11832
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
