[analyzer] Improve loads from reinterpret-cast fields

[steakhal]

Summary

Consider this example:

struct header {
  unsigned a : 1;
  unsigned b : 1;
};
struct parse_t {
  unsigned bits0 : 1;
  unsigned bits2 : 2; // <-- header
  unsigned bits4 : 4;
};
int parse(parse_t *p) {
  unsigned copy = p->bits2;
  clang_analyzer_dump(copy);
  // expected-warning@-1 {{reg_$1<unsigned int SymRegion{reg_$0<struct Bug_55934::parse_t * p>}.bits2>}}

  header *bits = (header *)&copy;
  clang_analyzer_dump(bits->b); // <--- Was UndefinedVal previously.
  // expected-warning@-1 {{derived_$2{reg_$1<unsigned int SymRegion{reg_$0<struct Bug_55934::parse_t * p>}.bits2>,Element{copy,0 S64b,struct Bug_55934::header}.b}}}
  return bits->b; // no-warning: it's not UndefinedVal
}

bits->b should have the same content as the second bit of p->bits2
(assuming that the bitfields are in spelling order).

---

The Store has the correct bindings. The problem is with the load of bits->b.
It will eventually reach RegionStoreManager::getBindingForField() with
Element{copy,0 S64b,struct header}.b, which is a FieldRegion.
It did not find any direct bindings, so the getBindingForFieldOrElementCommon()
gets called. That won't find any bindings, but it sees that the variable
is on the stack, thus it must be an uninitialized local variable;
thus it returns UndefinedVal.

Instead of doing this, it should have created a derived symbol
representing the slice of the region corresponding to the member.
So, if the value of copy is reg1, then the value of bits->b should
be derived{reg1, elem{copy,0, header}.b}.

Actually, the getBindingForElement() already does exactly this for reinterpret-casts, so I decided to hoist that and reuse the logic.

Fixes #55934

[steakhal]

No runtime diff. No crashes.
8 disappeared:

• 4 core.CallAndMessage (1st arg is uninitialized)
• 2 alpha.deadcode.UnreachableCode (sinked before reaching it)
• 2 core.UndefinedBinaryOperatorResult

2 new reports:

• 1 core.UndefinedBinaryOperatorResult
• 1 alpha.security.ArrayBound

15k+ reports preserved.

[martong]

I like it, but I'd like to know more about the nature of the applied "hack" and if there is a way to solve it properly in the longer term.

clang/lib/StaticAnalyzer/Core/RegionStore.cpp

2014–2017

Could you please elaborate why this is a hack? What should be done and where to solve it properly? (Disclaimer, I did not look into getBindingForFieldOrElementCommon.)

[steakhal]

clang/lib/StaticAnalyzer/Core/RegionStore.cpp

1897

BTW I'm not sure why this check is here. I would expect this to work eve without this.
OOh, I think I know why. getTypeSizeInChars can only be called on scalars?

2014–2017

I'm not sure what Jordan was actually referring to. IMO hardcoding to look for a highly specific scenario (a typed memregion, and a symbol to be exact) could be thought about implementing a 'hack'. IMO this fits in the current ecosystem, so for me it doesn't look like a hack; hence IMO this is the way of implementing this.

To make it clear, I simply hoisted the nested dyn_casts and checks into a function, while preserved the 'hack' comment at the callsite; so it's not added by me.

One additional note. We relay on the type information embedded into the memregion - which we know might not always be present and even accurate. That could be another reason why this should be considered as a 'hack'. IDK

[martong]

LGTM (given that you remove the vague and disturbing FIXME comments).

clang/lib/StaticAnalyzer/Core/RegionStore.cpp

2014–2017

Okay, thanks for the detailed explanation. So, seems like these FIXME comments are vague and they do not add any meaningful information, they are just disturbing noise. Could you please remove them?

Other than that, your change makes perfect sense to me.

clang/lib/StaticAnalyzer/Core/RegionStore.cpp

SVal RegionStoreManager::getSValFromStringLiteral(const StringLiteral *SL,
   assert(SL && "StringLiteral should not be null");
   // C++20 [dcl.init.string] 9.4.2.3:
   //   If there are fewer initializers than there are array elements, each
   //   element not explicitly initialized shall be zero-initialized [dcl.init].
   uint32_t Code = (Offset >= SL->getLength()) ? 0 : SL->getCodeUnit(Offset);
   return svalBuilder.makeIntVal(Code, ElemT);
 }
 
+static Optional<SVal> getDerivedSymbolForBinding(
+    RegionBindingsConstRef B, const TypedValueRegion *BaseRegion,
+    const TypedValueRegion *SubReg, const ASTContext &Ctx, SValBuilder &SVB) {
+  assert(BaseRegion);
+  QualType BaseTy = BaseRegion->getValueType();
+  QualType Ty = SubReg->getValueType();
+  if (BaseTy->isScalarType() && Ty->isScalarType()) {
+    if (Ctx.getTypeSizeInChars(BaseTy) >= Ctx.getTypeSizeInChars(Ty)) {
+      if (const Optional<SVal> &ParentValue = B.getDirectBinding(BaseRegion)) {
+        if (SymbolRef ParentValueAsSym = ParentValue->getAsSymbol())
+          return SVB.getDerivedRegionValueSymbolVal(ParentValueAsSym, SubReg);
+
+        if (ParentValue->isUndef())
+          return UndefinedVal();
+
+        // Other cases: give up.  We are indexing into a larger object
+        // that has some value, but we don't know how to handle that yet.
+        return UnknownVal();
+      }
+    }
+  }
+  return None;
+}
+
 SVal RegionStoreManager::getBindingForElement(RegionBindingsConstRef B,
                                               const ElementRegion* R) {
   // Check if the region has a binding.
   if (const Optional<SVal> &V = B.getDirectBinding(R))
     return *V;
 
   const MemRegion* superR = R->getSuperRegion();
 
SVal RegionStoreManager::getBindingForElement(RegionBindingsConstRef B,
   //   return *y;
   // FIXME: This is a hack, and doesn't do anything really intelligent yet.
   const RegionRawOffset &O = R->getAsArrayOffset();
 
   // If we cannot reason about the offset, return an unknown value.
   if (!O.getRegion())
     return UnknownVal();
 
-  if (const TypedValueRegion *baseR =
-        dyn_cast_or_null<TypedValueRegion>(O.getRegion())) {
-    QualType baseT = baseR->getValueType();
-    if (baseT->isScalarType()) {
-      QualType elemT = R->getElementType();
-      if (elemT->isScalarType()) {
-        if (Ctx.getTypeSizeInChars(baseT) >= Ctx.getTypeSizeInChars(elemT)) {
-          if (const Optional<SVal> &V = B.getDirectBinding(superR)) {
-            if (SymbolRef parentSym = V->getAsSymbol())
-              return svalBuilder.getDerivedRegionValueSymbolVal(parentSym, R);
-
-            if (V->isUnknownOrUndef())
-              return *V;
-            // Other cases: give up.  We are indexing into a larger object
-            // that has some value, but we don't know how to handle that yet.
-            return UnknownVal();
-          }
-        }
-      }
-    }
-  }
+  if (const TypedValueRegion *baseR = dyn_cast<TypedValueRegion>(O.getRegion()))
+    if (auto V = getDerivedSymbolForBinding(B, baseR, R, Ctx, svalBuilder))
+
   return getBindingForFieldOrElementCommon(B, R, R->getElementType());
 }
 
 SVal RegionStoreManager::getBindingForField(RegionBindingsConstRef B,
                                             const FieldRegion* R) {
 
   // Check if the region has a binding.
   if (const Optional<SVal> &V = B.getDirectBinding(R))
if (RecordVarTy.isConstQualified() || Ty.isConstQualified() ||
               if (Optional<SVal> V = svalBuilder.getConstantVal(FieldInit))
                 return *V;
           } else {
             return svalBuilder.makeZeroVal(Ty);
           }
         }
   }
 
+  // Handle the case where we are accessing into a larger scalar object.
+  // For example, this handles:
+  //   struct header {
+  //     unsigned a : 1;
+  //     unsigned b : 1;
+  //   };
+  //   struct parse_t {
+  //     unsigned bits0 : 1;
+  //     unsigned bits2 : 2; // <-- header
+  //     unsigned bits4 : 4;
+  //   };
+  //   int parse(parse_t *p) {
+  //     unsigned copy = p->bits2;
+  //     header *bits = (header *)&copy;
+  //     return bits->b;  <-- here
+  //   }
+  // FIXME: This is a hack, and doesn't do anything really intelligent yet.
+  // FIXME: This hack is analogous with the one present in
+  // `getBindingForElement`, maybe we should handle both in
+  // `getBindingForFieldOrElementCommon`.
+  if (const auto *Base = dyn_cast<TypedValueRegion>(R->getBaseRegion()))
+    if (auto V = getDerivedSymbolForBinding(B, Base, R, Ctx, svalBuilder))
+      return *V;
+
   return getBindingForFieldOrElementCommon(B, R, Ty);
 }
 
 Optional<SVal>
 RegionStoreManager::getBindingForDerivedDefaultValue(RegionBindingsConstRef B,
                                                      const MemRegion *superR,
                                                      const TypedValueRegion *R,
                                                      QualType Ty) {

clang/test/Analysis/ptr-arith.cpp

 // RUN: %clang_analyze_cc1 -Wno-unused-value -std=c++14 -analyzer-checker=core,debug.ExprInspection,alpha.core.PointerArithm -verify %s
+
+template <typename T> void clang_analyzer_dump(T);
+
 struct X {
   int *p;
   int zero;
   void foo () {
     reset(p - 1);
   }
   void reset(int *in) {
     while (in != p) // Loop must be entered.
bool ptrAsIntegerSubtractionNoCrash(__UINTPTR_TYPE__ x, char *p) {
   return y == x;
 }
 
 // Bug 34374
 bool integerAsPtrSubtractionNoCrash(char *p, __UINTPTR_TYPE__ m) {
   auto n = p - reinterpret_cast<char*>((__UINTPTR_TYPE__)1);
   return n == m;
 }
+
+namespace Bug_55934 {
+struct header {
+  unsigned a : 1;
+  unsigned b : 1;
+};
+struct parse_t {
+  unsigned bits0 : 1;
+  unsigned bits2 : 2; // <-- header
+  unsigned bits4 : 4;
+};
+int parse(parse_t *p) {
+  unsigned copy = p->bits2;
+  clang_analyzer_dump(copy);
+  // expected-warning@-1 {{reg_$1<unsigned int SymRegion{reg_$0<struct Bug_55934::parse_t * p>}.bits2>}}
+  header *bits = (header *)&copy;
+  clang_analyzer_dump(bits->b);
+  // expected-warning@-1 {{derived_$2{reg_$1<unsigned int SymRegion{reg_$0<struct Bug_55934::parse_t * p>}.bits2>,Element{copy,0 S64b,struct Bug_55934::header}.b}}}
+  return bits->b; // no-warning
+}
+} // namespace Bug_55934