[PATCH] D47603: [analyzer] fix bug with 1-bit APSInt types in Z3ConstraintManager

[Dominic Chen via Phabricator via cfe-commits]

ddcc added a comment.

In https://reviews.llvm.org/D47603#1118138, @vlad.tsyrklevich wrote:

> In https://reviews.llvm.org/D47603#1118106, @george.karpenkov wrote:
>
> > Would it be possible to add tests? I know we have very few unit tests, but I assume you could actually use an integration test to exercise this path?
>
>
> I tested this change and it fixes PR37622. There's a simple crash reproducer included there.


Cool, thanks for the repro! It's been long enough since I've touched this code that I don't recall the original failing testcase. I'll add the test to this revision.



================
Comment at: lib/StaticAnalyzer/Core/Z3ConstraintManager.cpp:1044
   Z3Expr Exp = getZ3Expr(Sym, &RetTy);
-
-  assert((getAPSIntType(From) == getAPSIntType(To)) &&
-         "Range values have different types!");
-  QualType RTy = getAPSIntType(From);
-  bool isSignedTy = RetTy->isSignedIntegerOrEnumerationType();
-  Z3Expr FromExp = Z3Expr::fromAPSInt(From);
-  Z3Expr ToExp = Z3Expr::fromAPSInt(To);
+  QualType LTy;
+  llvm::APSInt NewFromInt;
----------------
george.karpenkov wrote:
> What does `L` stand for here? It's confusing because `L/R` usually stand for left/right-hand-side in this context.
They correspond to the inferred left/right hand-side inferred types, but inside the subsequent Z3Expr `LHS` and `RHS` variables. This is confusing, so I'll rename them to `FromTy` and `ToTy`.


================
Comment at: lib/StaticAnalyzer/Core/Z3ConstraintManager.cpp:1154
+  BasicValueFactory &BVF = getBasicVals();
+  ASTContext &Ctx = BVF.getContext();
 
----------------
george.karpenkov wrote:
> that's a separate change, but OK
Yeah, this is one of several small miscellaneous changes that didn't make the original commit. It seemed a bit excessive to open separate revisions for each, so I've just been merging them into the next patch. I'm not sure which is preferable?


================
Comment at: lib/StaticAnalyzer/Core/Z3ConstraintManager.cpp:1426
+    *Ty = getAPSIntType(Int);
+  return Int;
+}
----------------
george.karpenkov wrote:
> It's redundant to mutate the argument passed by reference and also return it.
> Could we take a single `APSInt` parameter by value and return `std::pair<APSInt, QualType>` ?
Sure.


Repository:
  rC Clang

https://reviews.llvm.org/D47603