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

[dstenb]

Summary

When comparing a symbolic region and a constant, the constant would be
widened or truncated to the width of a void pointer, meaning that the
constant could be incorrectly truncated when handling symbols for
non-default address spaces. In the attached test case this resulted in a
false positive since the constant was truncated to zero. To fix this,
widen/truncate the constant to the width of the symbol expression's
type.

This commit does not consider non-symbolic regions as I'm not sure how
to generalize getting the type there.

This fixes PR40814.

[NoQ]

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).

lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp

574–582

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."

test/Analysis/ptr-cmp-const-trunc.cl

11

Pls add a // no-warning here, so that it was clear what the test is about.

[dstenb]

Address comments.

[NoQ]

Thanks again! Please commit.

[dstenb]

Thanks for the review! I'll submit this shortly then.

lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp

case nonloc::LocAsIntegerKind: {
           // of modeling "pointers as integers" is not complete.
           if (!BinaryOperator::isComparisonOp(op))
             return UnknownVal();
           // Transform the integer into a location and compare.
           // FIXME: This only makes sense for comparisons. If we want to, say,
           // add 1 to a LocAsInteger, we'd better unpack the Loc and add to it,
           // then pack it back into a LocAsInteger.
           llvm::APSInt i = rhs.castAs<nonloc::ConcreteInt>().getValue();
+          // 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.
+          if (SymbolRef lSym = lhs.getAsLocSymbol(true))
+            BasicVals.getAPSIntType(lSym->getType()).apply(i);
+          else
             BasicVals.getAPSIntType(Context.VoidPtrTy).apply(i);
           return evalBinOpLL(state, op, lhsL, makeLoc(i), resultTy);
         }
         default:
           switch (op) {
             case BO_EQ:
               return makeTruthVal(false, resultTy);
             case BO_NE:
               return makeTruthVal(true, resultTy);

test/Analysis/ptr-cmp-const-trunc.cl

+//RUN: %clang_analyze_cc1 -triple amdgcn-unknown-unknown -analyze -analyzer-checker=core -verify %s
+// expected-no-diagnostics
+
+#include <stdint.h>
+
+void bar(__global int *p) __attribute__((nonnull(1)));
+
+void foo(__global int *p) {
+  if ((uint64_t)p <= 1UL << 32)
+    bar(p); // no-warning
+}