[PATCH] D51057: [analyzer][UninitializedObjectChecker] Fixed dereferencing

[Domján Dániel via Phabricator via llvm-commits]

isuckatcs added inline comments.
Herald added subscribers: steakhal, manas, ASDenysPetrov, martong, gamesh411, dkrupp, donat.nagy, baloghadamsoftware.
Herald added a project: All.


================
Comment at: cfe/trunk/lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedPointee.cpp:222
 
-    if (isVoidPointer(DynT)) {
+  while (const MemRegion *Tmp = State->getSVal(R, DynT).getAsRegion()) {
+
----------------
@Szelethus @NoQ  @xazax.hun I ran into an issue related to this line.

Consider this test case:
```lang=c++
struct CyclicPointerTest1 {
  int *ptr; // expected-note{{object references itself 'this->ptr'}}
  int dontGetFilteredByNonPedanticMode = 0;

  CyclicPointerTest1() : ptr(reinterpret_cast<int *>(&ptr)) {} // expected-warning{{1 uninitialized field}}
};
```

At this point, with the example above, the store looks like this:
```
{"kind": "Direct", "offset": 0, "extent": 64, value: &Element{temp_object{CyclicPointerTest1, S915}.ptr,0 S64b,int}}
{"kind": "Direct", "offset": 64, "extent": 32, value: 0 S32b}
```

which means the values are the following:
```
V = &Element{temp_object{CyclicPointerTest1, S915}.ptr,0 S64b,int}
R = Element{temp_object{CyclicPointerTest1, S915}.ptr,0 S64b,int}
DynT = PointerType ... 'int *'
```
when `State->getSVal(R, DynT)` is called, eventually `RegionStoreManager::getBinding()` will also be invoked, where because `R` is an `ElementRegion` we reach these lines:
```lang=c++
  if (const ElementRegion *ER = dyn_cast<ElementRegion>(R)) {
    // FIXME: Here we actually perform an implicit conversion from the loaded
    // value to the element type.  Eventually we want to compose these values
    // more intelligently.  For example, an 'element' can encompass multiple
    // bound regions (e.g., several bound bytes), or could be a subset of
    // a larger value.
    return svalBuilder.evalCast(getBindingForElement(B, ER), T, QualType{});
  }
```
What happens here is that we call `getBindingForElement(B, ER)` and whatever value it returns, we cast to `T`, which is `int *` in this example. The type of the element inside the `ER` however is an `int`, so the following lookup will be performed:
```
Binding being looked up: 
  {"kind": "Direct", "offset": 0, "extent": 32}
------------------------------------------------------------------------------------------------------------------------
Store:
  {"kind": "Direct", "offset": 0, "extent": 64 value: &Element{temp_object{CyclicPointerTest1, S915}.ptr,0 S64b,int}}
  {"kind": "Direct", "offset": 64, "extent": 32 value: 0 S32b}
```
Since extents are ignored by default, the returned value will be `&Element{temp_object{CyclicPointerTest1, S915}.ptr,0 S64b,int}}`, which is indeed a pointer, however I'm not sure that the lookup here is actually correct. 

As far as I understand, we want to lookup an `int` element based on it's region and we receive a pointer value. Is it because I misunderstand something, or it's an actual issue that is hid by how region store works right now? 


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D51057/new/

https://reviews.llvm.org/D51057