[analyzer] Get direct binding for specific punned case

[vabridgers]

Summary

Region store was not able to see through this case to the actual
initialized value of STRUCT ff. This change addresses this case by
getting the direct binding. This was found and debugged in a downstream
compiler, with debug guidance from @steakhal. A positive and negative
test case is added.

The specific case where this issue was exposed.

typedef struct {
  int a:1;
  int b[2];
} STRUCT;

int main() {
  STRUCT ff = {0};
  STRUCT* pff = &ff;
  int a = ((int)pff + 1);
  return a;
}

[vabridgers]

clang-format

[steakhal]

Looks good.
Wait for an other accept.

clang/test/Analysis/array-struct-region.c

362–366 ↗

(On Diff #424796)

void array_struct_bitfield_1() {
  BITFIELD_CAST ff = {0};
  BITFIELD_CAST *pff = &ff;
  clang_analyzer_eval(*((int *)pff + 1) == 0); // expected-warning{{TRUE}}
  ff.b[0] = 3;
  clang_analyzer_eval(*((int *)pff + 1) == 3); // expected-warning{{TRUE}}
}

[vabridgers]

clang/test/Analysis/array-struct-region.c

362–366 ↗

(On Diff #424796)

Good suggestion, thanks. I'll add this and wait for another accept.

[vabridgers]

update LIT per comments from @steakhal

[vabridgers]

polite ping to @NoQ and/or @martong. @steakhal is ok with this change, are you ok if I land this? Thanks!

[martong]

Can we have a test for this, got idea from here (https://stackoverflow.com/questions/4129961/how-is-the-size-of-a-struct-with-bit-fields-determined-measured)

typedef struct
{
        unsigned int a:1;
        unsigned int x:31;
        unsigned int c:1;
        int b[2];
} mystruct;
...
ff.b[0] = 3;
clang_analyzer_eval(*((int *)pff + 2) == 3); // expected-warning{{TRUE}}  // Or should this be `pff + 3` ???

clang/test/Analysis/array-struct-region.c

1 ↗

(On Diff #424844)

Should we pin the target, shouldn't we?

365 ↗

(On Diff #424844)

My gut feeling is that we should pin the target for these arithmetics.

[steakhal]

In D124349#3490524, @martong wrote:

Can we have a test for this, got idea from here (https://stackoverflow.com/questions/4129961/how-is-the-size-of-a-struct-with-bit-fields-determined-measured)

typedef struct
{
        unsigned int a:1;
        unsigned int x:31;
        unsigned int c:1;
        int b[2];
} mystruct;
...
ff.b[0] = 3;
clang_analyzer_eval(*((int *)pff + 2) == 3); // expected-warning{{TRUE}}  // Or should this be `pff + 3` ???

Generally, you are right. But in this case, we are talking about a *single bit* bitfield.
That bitfield cannot span across multiple unsigned objects. And int is supposed to be at least one byte large, hence there is plenty of room for an additional CHAR_BIT - 1 bits along with this one and we would be still portable.

clang/test/Analysis/array-struct-region.c

1 ↗

(On Diff #424844)

There is no need for that.
The sizeof(int) might change, but the operator+ will accommodate for that in the pointer arithmetic. And the field after bitfields is by default aligned to its preferred alignment.

[vabridgers]

Thanks for the comments! Per our discussion, I'll create a different test case with a pinned target and add the additional test case to be sure we see expected behavior. Best!

[vabridgers]

Address comments from @martong

[martong]

LGTM! Thanks Vince!

clang/test/Analysis/array-punned-region.c

38

[vabridgers]

fix a stupid leftover cut/paste mistake in the test case :/

[vabridgers]

Thanks @martong. I fixed the "typo" :/, landing. Best!

[isuckatcs]

clang/test/Analysis/array-punned-region.c

24

@steakhal @martong @NoQ

Isn't this actually a false positive here?

(int *)pff + 2 points to ff.b[1], which is initialized to 0.

https://godbolt.org/z/Gh8a4aMe8

clang/lib/StaticAnalyzer/Core/RegionStore.cpp

if (R->hasStackNonParametersStorage()) {
     // FIXME: We also need to take ElementRegions with symbolic indexes into
     // account.  This case handles both directly accessing an ElementRegion
     // with a symbolic offset, but also fields within an element with
     // a symbolic offset.
     if (hasSymbolicIndex)
       return UnknownVal();
 
     // Additionally allow introspection of a block's internal layout.
-    if (!hasPartialLazyBinding && !isa<BlockDataRegion>(R->getBaseRegion()))
+    // Try to get direct binding if all other attempts failed thus far.
+    // Else, return UndefinedVal()
+    if (!hasPartialLazyBinding && !isa<BlockDataRegion>(R->getBaseRegion())) {
+      if (const Optional<SVal> &V = B.getDefaultBinding(R))
+        return *V;
       return UndefinedVal();
     }
+  }
 
   // All other values are symbolic.
   return svalBuilder.getRegionValueSymbolVal(R);
 }
 
 SVal RegionStoreManager::getBindingForObjCIvar(RegionBindingsConstRef B,
                                                const ObjCIvarRegion* R) {
   // Check if the region has a binding.

clang/test/Analysis/array-punned-region.c

+// RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.core,debug.ExprInspection -verify -analyzer-config eagerly-assume=false -triple x86_64-pc-linux-gnu %s
+
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.core,debug.ExprInspection -verify -analyzer-config eagerly-assume=false -triple i386-pc-linux-gnu  %s
+
+int clang_analyzer_eval(int);
+
+typedef struct {
+  int a : 1;
+  int b[2];
+} BITFIELD_CAST;
+
+void array_struct_bitfield_1() {
+  BITFIELD_CAST ff = {0};
+  BITFIELD_CAST *pff = &ff;
+  clang_analyzer_eval(*((int *)pff + 1) == 0); // expected-warning{{TRUE}}
+  ff.b[0] = 3;
+  clang_analyzer_eval(*((int *)pff + 1) == 3); // expected-warning{{TRUE}}
+}
+
+int array_struct_bitfield_2() {
+  BITFIELD_CAST ff = {0};
+  BITFIELD_CAST *pff = &ff;
+  int a = *((int *)pff + 2); // expected-warning{{Assigned value is garbage or undefined [core.uninitialized.Assign]}}
+  return a;
+}
+
+typedef struct {
+  unsigned int a : 1;
+  unsigned int x : 31;
+  unsigned int c : 1;
+  int b[2];
+} mystruct;
+
+void array_struct_bitfield_3() {
+  mystruct ff;
+  mystruct *pff = &ff;
+  ff.b[0] = 3;
+  clang_analyzer_eval(*((int *)pff + 2) == 3); // expected-warning{{TRUE}}  // Or should this be `pff + 3` ???
+}