[analyzer] Fix logical not for pointers with different bit width

[danielmarjamaki]

Summary

The Static Analyzer assumed that all pointers had the same bit width. Now pass the type to the 'makeNull' method, to construct a null
pointer of the appropiate bit width.

Example code that does not work well:

int main(void) {
  __cm void *cm_p = 0;
  if (cm_p == 0)
    (void)cm_p;
}

Unfortunately there is no proper testcase here. The problem is seen with a custom target.

[xazax.hun]

So there is no LLVM supported target with different bit width pointer types?

In case we have such a target upstreamed, you can add a test with the host-triple hardcoded into the test.

[danielmarjamaki]

I am not sure where to look. I heard somebody say OpenCL has different pointer widths.

[grandinj]

include/clang/StaticAnalyzer/Core/PathSensitive/SValBuilder.h

313–317

spelling and grammar and doxygen formatting:

/// \param type accommodate different pointer bit-widths of different address spaces.

[zaks.anna]

Are there other cases where makeNull would need to be replaced?

[danielmarjamaki]

Added a testcase that will crash without the fix. Used the amdgcn target as that happens to use different pointer bit widths for different address spaces.

Updated the comment. I am not good at english so I hope the grammar is ok.

[danielmarjamaki]

In D31029#703428, @zaks.anna wrote:

Are there other cases where makeNull would need to be replaced?

There might be. As I understand it, this is the only known case at the moment.

[danielmarjamaki]

In D31029#708567, @danielmarjamaki wrote:

In D31029#703428, @zaks.anna wrote:

Are there other cases where makeNull would need to be replaced?

There might be. As I understand it, this is the only known case at the moment.

To clarify. The static analyser runs fine on plenty of code. I ran CSA using this target on a 1000's of C-files project. I think it works well.. but I can't guarantee there won't be any more issues.

[danielmarjamaki]

Ping

[danielmarjamaki]

Ping

[danielmarjamaki]

Ping

[zaks.anna]

Sorry for the delay!!!

include/clang/StaticAnalyzer/Core/PathSensitive/BasicValueFactory.h

180

The isUnsigned parameter is not used and is not necessary as the type itself has this info.

Not clear if this function is expected to work for all types or only scalar types (maybe assert that the input is isScalarType()).

[danielmarjamaki]

Fix review comments

[danielmarjamaki]

ping

[zaks.anna]

Looks good with a nit!

include/clang/StaticAnalyzer/Core/PathSensitive/SValBuilder.h

320

Formatting changes here look inconsistent with the rest of the code around.

include/clang/StaticAnalyzer/Core/PathSensitive/BasicValueFactory.h

public:
   }
 
   inline const llvm::APSInt& Sub1(const llvm::APSInt& V) {
     llvm::APSInt X = V;
     --X;
     return getValue(X);
   }
 
-  inline const llvm::APSInt& getZeroWithPtrWidth(bool isUnsigned = true) {
+  inline const llvm::APSInt &getZeroWithTypeSize(QualType T,
+                                                 bool isUnsigned = true) {
+    return getValue(0, Ctx.getTypeSize(T), isUnsigned);
+  }
+
+  inline const llvm::APSInt &getZeroWithPtrWidth(bool isUnsigned = true) {
     return getValue(0, Ctx.getTypeSize(Ctx.VoidPtrTy), isUnsigned);
   }
 
   inline const llvm::APSInt &getIntWithPtrWidth(uint64_t X, bool isUnsigned) {
     return getValue(X, Ctx.getTypeSize(Ctx.VoidPtrTy), isUnsigned);
   }
 
   inline const llvm::APSInt& getTruthValue(bool b, QualType T) {

include/clang/StaticAnalyzer/Core/PathSensitive/SValBuilder.h

public:
   nonloc::ConcreteInt makeTruthVal(bool b, QualType type) {
     return nonloc::ConcreteInt(BasicVals.getTruthValue(b, type));
   }
 
   nonloc::ConcreteInt makeTruthVal(bool b) {
     return nonloc::ConcreteInt(BasicVals.getTruthValue(b));
   }
 
-  Loc makeNull() {
-    return loc::ConcreteInt(BasicVals.getZeroWithPtrWidth());
+  /// Create NULL pointer, with proper pointer bit-width for given address
+  /// space.
+  /// \param type pointer type.
+  Loc makeNullWithType(QualType type) {
+    return loc::ConcreteInt(BasicVals.getZeroWithTypeSize(type));
   }
 
+  Loc makeNull() { return loc::ConcreteInt(BasicVals.getZeroWithPtrWidth()); }
+
   Loc makeLoc(SymbolRef sym) {
     return loc::MemRegionVal(MemMgr.getSymbolicRegion(sym));
   }
 
   Loc makeLoc(const MemRegion* region) {
     return loc::MemRegionVal(region);
   }
 

lib/StaticAnalyzer/Core/ExprEngineC.cpp

case UO_Not: {
           break;
         case UO_LNot:
           // C99 6.5.3.3: "The expression !E is equivalent to (0==E)."
           //
           //  Note: technically we do "E == 0", but this is the same in the
           //    transfer functions as "0 == E".
           SVal Result;
           if (Optional<Loc> LV = V.getAs<Loc>()) {
-            Loc X = svalBuilder.makeNull();
+            Loc X = svalBuilder.makeNullWithType(Ex->getType());
             Result = evalBinOp(state, BO_EQ, *LV, X, U->getType());
-          }
-          else if (Ex->getType()->isFloatingType()) {
+          } else if (Ex->getType()->isFloatingType()) {
             // FIXME: handle floating point types.
             Result = UnknownVal();
           } else {
             nonloc::ConcreteInt X(getBasicVals().getValue(0, Ex->getType()));
             Result = evalBinOp(state, BO_EQ, V.castAs<NonLoc>(), X,
                                U->getType());
           }
 

test/Analysis/ptr.cl

+// RUN: %clang_analyze_cc1 -triple amdgcn-unknown-unknown -target-cpu verde -analyze -analyzer-checker=core %s
+
+#define __cm __attribute__((address_space(256)))
+
+// Don't crash when pointer bit-widths are different for different address spaces
+void dontCrash(void) {
+  __cm void *cm_p = 0;
+  if (!cm_p)
+    (void)cm_p;
+}