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

[vabridgers]

Summary

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:

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 chars, 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.

[vabridgers]

update commit header

[bjope]

I think you can create an upstream test case by looking at clang/test/Analysis/ptr.cl.
It uses amdgcn and address space 256, which seems to have a different pointer size compared to address space zero for that specific target.

So the test case (named with a .cl extension) would look like this (I don't think you really need to specify verde as target cpu):

// RUN: %clang_analyze_cc1 -triple amdgcn-unknown-unknown -target-cpu verde -analyze -analyzer-checker=core %s

void test(__attribute__((address_space(256))) int * p) {
  __attribute__((address_space(256))) int * q = p-1;
  if (q) {}
  if (q) {} // crash
  (void)q;
}

[NoQ]

I think this is a plausible use case, previously I thought it's too much work but it seems reasonable last time I looked into it, so I'm glad to see it going on!

Can you add a comment explaining that the new code is necessary for architectures with varying pointer bit width? That's often unobvious.

clang/lib/StaticAnalyzer/Core/SValBuilder.cpp

718

I guess something like this?

void foo(int &x) {
  int *p = &x; // 'p' is the same SVal as 'x'
  bool b = p;
}

I think we should return a concrete true in the reference case. We already know that such symbol is true-ish but returning a concrete true might be easier on the solver.

[vabridgers]

Thanks all for the updates and comments. I'll address promptly and resubmit. Best!

[vabridgers]

address comments, add test case suggested by @bjope

[vabridgers]

Thanks for the updates, comments and the reproducer. I've added those in this update. If possible, I'd like to get this change accepted to avoid the crash we currently see, and I'll immediately follow up with the FIXMEs. Otherwise, I'll iterate quickly on the FIXMEs. - Vince

clang/lib/StaticAnalyzer/Core/SValBuilder.cpp

718

I think this comment is addressed by changing the comment, and deferring a subsequent follow up. I'd like to get this change submitted to at least avoid the crash if possible? Otherwise, if needed, I'll iterate quickly on trying to return a concrete 'true' for the reference case.

[vabridgers]

update test case

[vabridgers]

rebase

[NoQ]

Looks great, thanks!

clang/lib/StaticAnalyzer/Core/SValBuilder.cpp

718

Yes, it's absolutely fine to investigate this separately :)

clang/lib/StaticAnalyzer/Core/SValBuilder.cpp

if (CastTy->isBooleanType()) {
     if (const FunctionCodeRegion *FTR = dyn_cast<FunctionCodeRegion>(R))
       if (const FunctionDecl *FD = dyn_cast<FunctionDecl>(FTR->getDecl()))
         if (FD->isWeak())
           // FIXME: Currently we are using an extent symbol here,
           // because there are no generic region address metadata
           // 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);
   }
 
   const bool IsUnknownOriginalType = OriginalTy.isNull();
   // Try to cast to array
   const auto *ArrayTy =
       IsUnknownOriginalType

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

+// 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;
+}