[PATCH] D117229: [Analyzer] Produce SymbolCast for pointer to integer cast

[Balázs Benics via Phabricator via cfe-commits]

steakhal added a comment.

Looks correct to me.



================
Comment at: clang/include/clang/StaticAnalyzer/Core/PathSensitive/SValBuilder.h:110-112
 
+  SVal simplifySymbolCast(SymbolRef SE, QualType CastTy);
+
----------------
You don't need to have gap between these two. They belong in the same overload set.


================
Comment at: clang/lib/StaticAnalyzer/Core/SValBuilder.cpp:776-785
+
+    AnalyzerOptions &Opts =
+        StateMgr.getOwningEngine().getAnalysisManager().getAnalyzerOptions();
+    if (Opts.ShouldSupportSymbolicIntegerCasts) {
+      const MemRegion *R = V.getRegion();
+      if (R && !OriginalTy.isNull())
+        if (const auto *SR = dyn_cast<SymbolicRegion>(R))
----------------



================
Comment at: clang/test/Analysis/produce-ptr-to-integer-symbolcast.cpp:24-27
+  // FIXME, below we should produce a SymbolCast instead of the LocAsInteger.
+  // However, for that we'd need to be able to store a loc::GotoLabel SVal
+  // inside the SymbolCast. But currently we can put only another SymExpr
+  // inside a SymbolCast.
----------------
Is this the only obstacle to eradicate `LocAsInteger`?


================
Comment at: clang/test/Analysis/symbol-simplification-mem-region-to-int-cast.cpp:1-6
+// RUN: %clang_analyze_cc1 %s \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-checker=debug.ExprInspection \
+// RUN:   -analyzer-config eagerly-assume=false \
+// RUN:   -analyzer-config support-symbolic-integer-casts=true \
+// RUN:   -verify
----------------
I think you need to pin the target. You assume a specific pointer bitwidth in the test.


================
Comment at: clang/test/Analysis/symbol-simplification-mem-region-to-int-cast.cpp:23
+  clang_analyzer_eval(p == 0);            // expected-warning{{TRUE}}
+  clang_analyzer_eval(p_as_integer == 0); // expected-warning{{UNKNOWN}}
+
----------------
Why is this ``UNKNOWN`? I would expect `TRUE` here as well.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D117229/new/

https://reviews.llvm.org/D117229