[PATCH] D58665: [analyzer] Handle comparison between non-default AS symbol and constant

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

Hi, thanks!, i think this is correct. As in, `LocAsInteger` was clearly a mistake to begin with, but this change shouldn't make it worse.

You should be able to get away with not supporting comparisons between regions without symbolic base [as integers] and concrete integers because they usually yield fairly dumb values anyway. Eg., it's //impossible// for an address of a variable to be equal to a null pointer, and whether a variable's address is currently equal to `0xbadf00d` is //undefined// anyway - it will cause problems when we try to symbolicate our undefined values, but for now we prefer to interrupt the analysis whenever they actually end up being used (which causes the problem to disappear because symbolication only matters when we use the symbol more than once).



================
Comment at: lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp:574
           llvm::APSInt i = rhs.castAs<nonloc::ConcreteInt>().getValue();
-          BasicVals.getAPSIntType(Context.VoidPtrTy).apply(i);
+          // FIXME: Handle non-default address spaces for non-symbolic regions.
+          if (SymbolRef lSym = lhs.getAsLocSymbol(true))
----------------
Pls be more specific in this comment that we're making this extra check in order to support non-default address spaces. Eg., "If the region has a symbolic base, pay attention to the type; it might be coming from a non-default address space. For non-symbolic regions it doesn't matter that much because such comparisons would most likely evaluate to concrete false anyway. FIXME: We might still need to handle the non-comparison case."


================
Comment at: test/Analysis/ptr-cmp-const-trunc.cl:10
+  if ((uint64_t)p <= 1UL << 32)
+    bar(p);
+}
----------------
Pls add a `// no-warning` here, so that it was clear what the test is about.


Repository:
  rC Clang

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

https://reviews.llvm.org/D58665