[analyzer] Symbolicate float values with integral casting

[ASDenysPetrov]

Summary

Create nonloc::SymbolVal with SymbolCast expression from integral type to float type.

[ASDenysPetrov]

Hi, folk! Please, review this tiny patch.

[ASDenysPetrov]

Updated. Moved comments as well.

[steakhal]

void clang_analyzer_dumpi(int);
void clang_analyzer_dumpf(float);
void clang_analyzer_dumpip(int*);
void clang_analyzer_dumpfp(float*);

void SymbolCast_of_float_type_aux(int *p) {
  *p += 1;
  clang_analyzer_dumpi(*p); // (Previously): Unknown  ->  (Now): (float) (conj_$1{int, LC2, S790, #1})
  clang_analyzer_dumpf(*p); // (Previously): Unknown  ->  (Now): (float) (conj_$1{int, LC2, S790, #1})
  clang_analyzer_dumpip(p); // &Element{x1,1 S64b,float}
  clang_analyzer_dumpfp(p); // &Element{x1,1 S64b,float}
}

void SymbolCast_of_float_type() {
  extern float x1;
  extern double x2;
  extern long double x3;

  void (*f)() = SymbolCast_of_float_type_aux;

  clang_analyzer_dumpi(*(&x1 + 1));  // Unknown
  clang_analyzer_dumpf(*(&x1 + 1));  // Unknown
  clang_analyzer_dumpip(&x1 + 1);    // &Element{x1,1 S64b,int}
  clang_analyzer_dumpfp(&x1 + 1);    // &Element{x1,1 S64b,float}

  f(&x1 + 1);
  // f(&x2 + 1);
  // f(&x3 + 1);
}

Only lines 8-9 have changed. But IMO, in a bad way.
We should have just (conj_$1{float, LC2, S790, #1}) there, because the object you read from that location is of type float.
Reading an int and casting it to float means a different thing.

Before the patch - saying Unknown, it was not accurate, but at least correct.

[ASDenysPetrov]

This is just a note for me to correctly rebase it in the future:
https://reviews.llvm.org/D96090?vs=321602&id=322422#toc

[NoQ]

tiny patch

IIUC this is roughly the first time ever when SValBuilder starts emitting symbols of float type. This is a huge change with an almost unlimited scope of unexpected side effects and very rigorous testing is required to understand the actual effect of this patch on programs that use floats. I think that generally some symbols are always better than no symbols, even if the constraint manager is completely unable to handle them. But we have to make sure that the entirety of SValBuilder and the entirety of RegionStore (and possibly the entirety of ExprEngine) are ready to deal with completely new kinds of values that they've never seen before.

clang/test/Analysis/svalbuilder-float-cast.c

11

You silently ignored and removed this comment. The author of the original code was pretty certain that your solution is incorrect and most likely added this comment there specifically to make sure your solution doesn't get implemented.

If you see potential problems with your patch, please don't try to sweep them under the rug. If you're sure your solution is correct, please explain why. If not, please ask about it in the summary or in the comments.

Generally, every commit should be obvious. If there are obvious questions that remain unanswered then the commit is not obvious and needs improvement.

[ASDenysPetrov]

@NoQ wrote:

IIUC this is roughly the first time ever when SValBuilder starts emitting symbols of float type. This is a huge change with an almost unlimited scope of unexpected side effects and very rigorous testing is required to understand the actual effect of this patch on programs that use floats. I think that generally some symbols are always better than no symbols, even if the constraint manager is completely unable to handle them. But we have to make sure that the entirety of SValBuilder and the entirety of RegionStore (and possibly the entirety of ExprEngine) are ready to deal with completely new kinds of values that they've never seen before.

I kind of abandoned this patch. I realized that too much circumstances appeared that I am not an expert in. It is more like an experimental patch which serves to make clear some information, raise a discussion of what another people know about handling floats. Thank you for your sharing. Now I see what potential problems could be. I'm going to proceed as soon as I feel more confident answering these questions.

clang/test/Analysis/svalbuilder-float-cast.c

11

Very good objection! Sorry, honestly I somehow missed that it should NOT. Of course we should have strong arguments introducing the opposite effect. This is my mistake.

clang/lib/StaticAnalyzer/Core/SValBuilder.cpp

 // At the moment, this function will redirect to evalCast, except when the range
 // of the original value is known to be greater than the max of the target type.
 SVal SValBuilder::evalIntegralCast(ProgramStateRef state, SVal val,
                                    QualType castTy, QualType originalTy) {
   // No truncations if target type is big enough.
   if (getContext().getTypeSize(castTy) >= getContext().getTypeSize(originalTy))
     return evalCast(val, castTy, originalTy);
 
+  // The code call below would convert the int into a float.
+  // What we want, however, is a bit-by-bit reinterpretation of the int
+  // as a float, which usually yields nothing garbage.
+  // TODO: What other combinations of types are affected?
+  // Let evalCast handle non symbolic expressions and float symbols.
   SymbolRef se = val.getAsSymbol();
-  if (!se) // Let evalCast handle non symbolic expressions.
+  if (!se || se->getType()->isFloatingType())
     return evalCast(val, castTy, originalTy);
 
   // Find the maximum value of the target type.
   APSIntType ToType(getContext().getTypeSize(castTy),
                     castTy->isUnsignedIntegerType());
   llvm::APSInt ToTypeMax = ToType.getMaxValue();
   NonLoc ToTypeMaxVal =
       makeIntVal(ToTypeMax.isUnsigned() ? ToTypeMax.getZExtValue()

clang/lib/StaticAnalyzer/Core/Store.cpp

 /// CastRetrievedVal - Used by subclasses of StoreManager to implement
 ///  implicit casts that arise from loads from regions that are reinterpreted
 ///  as another region.
 SVal StoreManager::CastRetrievedVal(SVal V, const TypedValueRegion *R,
                                     QualType castTy) {
   if (castTy.isNull() || V.isUnknownOrUndef())
     return V;
 
-  // The dispatchCast() call below would convert the int into a float.
-  // What we want, however, is a bit-by-bit reinterpretation of the int
-  // as a float, which usually yields nothing garbage. For now skip casts
-  // from ints to floats.
-  // TODO: What other combinations of types are affected?
-  if (castTy->isFloatingType()) {
-    SymbolRef Sym = V.getAsSymbol();
-    if (Sym && !Sym->getType()->isFloatingType())
-      return UnknownVal();
-  }
-
   // When retrieving symbolic pointer and expecting a non-void pointer,
   // wrap them into element regions of the expected type if necessary.
   // SValBuilder::dispatchCast() doesn't do that, but it is necessary to
   // make sure that the retrieved value makes sense, because there's no other
   // cast in the AST that would tell us to cast it to the correct pointer type.
   // We might need to do that for non-void pointers as well.
   // FIXME: We really need a single good function to perform casts for us
   // correctly every time we need it.

clang/test/Analysis/svalbuilder-float-cast.c

 // RUN: %clang_analyze_cc1 -analyzer-checker debug.ExprInspection -verify %s
 void clang_analyzer_denote(int, const char *);
 void clang_analyzer_express(int);
 
 void SymbolCast_of_float_type_aux(int *p) {
   *p += 0;
-  // FIXME: Ideally, all unknown values should be symbolicated.
-  clang_analyzer_denote(*p, "$x"); // expected-warning{{Not a symbol}}
+  clang_analyzer_denote(*p, "$x");
 
   *p += 1;
-  // This should NOT be (float)$x + 1. Symbol $x was never casted to float.
-  // FIXME: Ideally, this should be $x + 1.
-  clang_analyzer_express(*p); // expected-warning{{Not a symbol}}
+  // expected-warning@+3{{(float)$x + 1}}
+  // expected-warning@+2{{(double)$x + 1}}
+  // expected-warning@+1{{(long double)$x + 1}}
+  clang_analyzer_express(*p);
 }
 
 void SymbolCast_of_float_type() {
-  extern float x;
+  extern float x1;
+  extern double x2;
+  extern long double x3;
+
   void (*f)() = SymbolCast_of_float_type_aux;
-  f(&x);
+
+  f(&x1);
+  f(&x2);
+  f(&x3);
 }