[PATCH] D105974: [analyzer] Fix assertion in state split code path

[Vince Bridgers via Phabricator via cfe-commits]

vabridgers created this revision.
vabridgers added reviewers: martong, steakhal.
Herald added subscribers: manas, ASDenysPetrov, dkrupp, donat.nagy, Szelethus, mikhail.ramalho, a.sidorin, rnkovacs, szepet, baloghadamsoftware, xazax.hun, whisperity.
vabridgers requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

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) {}
    (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 evaluting 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 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.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D105974

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


Index: clang/lib/StaticAnalyzer/Core/SValBuilder.cpp
===================================================================
--- clang/lib/StaticAnalyzer/Core/SValBuilder.cpp
+++ clang/lib/StaticAnalyzer/Core/SValBuilder.cpp
@@ -712,9 +712,18 @@
           // 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();
+      // FIXME: Why did we have references at this point?
+      // 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);
   }


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D105974.358572.patch
Type: text/x-patch
Size: 1263 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20210714/0393a963/attachment.bin>