[clang] 918bda1 - [analyzer] Do not assume that all pointers have the same bitwidth as void*

[via cfe-commits]

Author: Vince Bridgers
Date: 2021-07-16T03:22:57-05:00
New Revision: 918bda1241202d0480c6d94ec8f72c483d77a06c

URL: https://github.com/llvm/llvm-project/commit/918bda1241202d0480c6d94ec8f72c483d77a06c
DIFF: https://github.com/llvm/llvm-project/commit/918bda1241202d0480c6d94ec8f72c483d77a06c.diff

LOG: [analyzer] Do not assume that all pointers have the same bitwidth as void*

This change addresses this assertion that occurs in a downstream
compiler with a custom target.

```APInt.h:1151: bool llvm::APInt::operator==(const llvm::APInt &) const: Assertion `BitWidth == RHS.BitWidth && "Comparison requires equal bit widths"'```

No covering test case is susbmitted with this change since this crash
cannot be reproduced using any upstream supported target. The test case
that exposes this issue is as simple as:

```lang=c++
  void test(int * p) {
    int * q = p-1;
    if (q) {}
    if (q) {} // crash
    (void)q;
  }
```

The custom target that exposes this problem supports two address spaces,
16-bit `char`s, and a `_Bool` type that maps to 16-bits. There are no upstream
supported targets with similar attributes.

The assertion appears to be happening as a result of evaluating the
`SymIntExpr` `(reg_$0<int * p>) != 0U` in `VisitSymIntExpr` located in
`SimpleSValBuilder.cpp`. The `LHS` is evaluated to `32b` and the `RHS` is
evaluated to `16b`. This eventually leads to the assertion in `APInt.h`.

While this change addresses the crash and passes LITs, two follow-ups
are required:
  1) The remainder of `getZeroWithPtrWidth()` and `getIntWithPtrWidth()`
     should be cleaned up following this model to prevent future
     confusion.
  2) We're not sure why references are found along with the modified
     code path, that should not be the case. A more principled
     fix may be found after some further comprehension of why this
     is the case.

Acks: Thanks to @steakhal and @martong for the discussions leading to this
fix.

Reviewed By: NoQ

Differential Revision: https://reviews.llvm.org/D105974

Added: 
    clang/test/Analysis/solver-sym-simplification-ptr-bool.cl

Modified: 
    clang/lib/StaticAnalyzer/Core/SValBuilder.cpp

Removed: 
    


################################################################################
diff  --git a/clang/lib/StaticAnalyzer/Core/SValBuilder.cpp b/clang/lib/StaticAnalyzer/Core/SValBuilder.cpp
index b7ca8e8ca9cd..a5e99ce72d6b 100644
--- a/clang/lib/StaticAnalyzer/Core/SValBuilder.cpp
+++ b/clang/lib/StaticAnalyzer/Core/SValBuilder.cpp
@@ -712,9 +712,23 @@ SVal SValBuilder::evalCastSubKind(loc::MemRegionVal V, QualType CastTy,
           // symbols to use, only content metadata.
           return nonloc::SymbolVal(SymMgr.getExtentSymbol(FTR));
 
-    if (const SymbolicRegion *SymR = R->getSymbolicBase())
-      return makeNonLoc(SymR->getSymbol(), BO_NE,
-                        BasicVals.getZeroWithPtrWidth(), CastTy);
+    if (const SymbolicRegion *SymR = R->getSymbolicBase()) {
+      SymbolRef Sym = SymR->getSymbol();
+      QualType Ty = Sym->getType();
+      // This change is needed for architectures with varying
+      // pointer widths. See the amdgcn opencl reproducer with
+      // this change as an example: solver-sym-simplification-ptr-bool.cl
+      // FIXME: We could encounter a reference here,
+      //        try returning a concrete 'true' since it might
+      //        be easier on the solver.
+      // FIXME: Cleanup remainder of `getZeroWithPtrWidth ()`
+      //        and `getIntWithPtrWidth()` functions to prevent future
+      //        confusion
+      const llvm::APSInt &Zero = Ty->isReferenceType()
+                                     ? BasicVals.getZeroWithPtrWidth()
+                                     : BasicVals.getZeroWithTypeSize(Ty);
+      return makeNonLoc(Sym, BO_NE, Zero, CastTy);
+    }
     // Non-symbolic memory regions are always true.
     return makeTruthVal(true, CastTy);
   }

diff  --git a/clang/test/Analysis/solver-sym-simplification-ptr-bool.cl b/clang/test/Analysis/solver-sym-simplification-ptr-bool.cl
new file mode 100644
index 000000000000..be8edbf51eba
--- /dev/null
+++ b/clang/test/Analysis/solver-sym-simplification-ptr-bool.cl
@@ -0,0 +1,30 @@
+// RUN: %clang_analyze_cc1 -triple amdgcn-unknown-unknown -analyze -analyzer-checker=core %s
+
+// expected-no-diagnostics
+
+// This test case covers an issue found in the static analyzer
+// solver where pointer sizes were assumed. Pointer sizes may vary on other
+// architectures. This issue was originally discovered on a downstream,
+// custom target, this assert occurs on the custom target and this one
+// without the fix, and is fixed with this change.
+//
+// The assertion appears to be happening as a result of evaluating the
+// SymIntExpr (reg_$0<int * p>) != 0U in VisitSymIntExpr located in
+// SimpleSValBuilder.cpp. The LHS is evaluated to 32b and the RHS is
+// evaluated to 16b. This eventually leads to the assertion in APInt.h.
+//
+// APInt.h:1151: bool llvm::APInt::operator==(const llvm::APInt &) const: Assertion `BitWidth == RHS.BitWidth && "Comparison requires equal bit widths"'
+// 
+void test1(__attribute__((address_space(256))) int * p) {
+  __attribute__((address_space(256))) int * q = p-1;
+  if (q) {}
+  if (q) {}
+  (void)q;
+}
+ 
+void test2(__attribute__((address_space(256))) int * p) {
+  __attribute__((address_space(256))) int * q = p-1;
+  q && q; 
+  q && q; 
+  (void)q;
+}