xazax.hun added a comment.
In http://reviews.llvm.org/D15007#298086, @dcoughlin wrote:
> Gabor, thanks for taking a look at this!
>
> I'm not sure `RegionStore::getBinding()` is the right place to put this logic.
>
> First, not all dereferences of a `std::nullptr_t` value go through
> `getBinding()`, so, for example, the following snippet triggers the assertion
> even with your patch:
>
> decltype(nullptr) returnsNullPtrType();
> void fromReturnType() {
> ((X *)returnsNullPtrType())->f(); // Crash in analysis!
> }
>
Thank you for spotting this, I will definitely try the approach you suggested.
> Second, not all locations of type `std::nullptr_t` contain `0`. For example,
> the following prints "1" on my machine:
>
> #include <iostream>
>
> int main() {
> decltype(nullptr) x;
> std::cout << (bool)x << "\n";
> }
>
>
> That is, a default-initialized (garbage) nullptr_t may not be '0'. Prior to
> your patch the analyzer would warn here about `x` being uninitialized (when
> the analyzer is compiled without asserts) -- but with your patch we lose that
> warning.
I am not entirely sure what the correct behavior is here. According to the
standard (2.14.7): "std::nullptr_t is a distinct type that is neither a pointer
type nor a pointer to member type; rather, a prvalue of this type is a null
pointer constant and can be converted to a null pointer value or null member
pointer value." My interpretation is that, the std::nullptr_t does not have a
state, it just converts to a null pointer value regardless what value is at the
location of the nullptr_t. Regardless what the standard says it looks like the
value of std::nullptr_t can be (ab)used in the current clang implementation.
> What do you think about moving your new logic to the symbol value creation
> methods in SValBuilder? More specifically, you could add the check for
> nullptr_t to `getRegionValueSymbolVal()`, `conjureSymbolVal()` (both
> variants), `getConjuredHeapSymbolVal()`, and
> `getDerivedRegionValueSymbolVal()`. This would let the analyzer warn when it
> knows there is a garbage value in a location of type nullptr_t but still
> optimistically assume that the value is is '0' when the value would otherwise
> be symbolic.
>
> With this approach we would retain uninitialization warnings about accesses
> via definitely uninitialized storage:
>
> void fromUninitializedLocal() {
> decltype(nullptr) p;
> ((X *)p)->f(); // Warn about p being uninitialized
> }
>
> void fromUninitializedLocalStruct() {
> Type t;
> ((X *)t.x)->f(); // Warn about t.x being uninitialized.
> }
>
>
> but would warn about about a null access in `returnsNullPtrType()` and would
> keep the behavior you specify in your `f()` test because `p` is a parameter
> (and thus its value is symbolic).
I will look into this thank you.
http://reviews.llvm.org/D15007
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
