[PATCH] D15007: [analyzer] Improve modelling of nullptr_t in the analyzer. Fix PR25414.

[Gábor Horváth via cfe-commits]

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