[analyzer][NFC] Inline ExprEngine::handleLVectorSplat()

[steakhal]

Summary

It seems like ExprEngine::handleLVectorSplat() was used at only 2 places.
It might be better to directly inline them for readability.

It seems like these cases were not covered by tests according to my coverage measurement, so I'm adding tests as well.
Besides that, I'm handling CK_MatrixCast similarly to how the rest of the unhandled casts are evaluated.

Previously:

After covering all three labels:

[Szelethus]

It might be better to directly inline them for readability.

Using so many parameters like that is ugly indeed, but this is hardly better, and introduces code duplication. Maybe a fall through?

[NoQ]

In D105125#2847371, @Szelethus wrote:

It might be better to directly inline them for readability.

Using so many parameters like that is ugly indeed, but this is hardly better, and introduces code duplication. Maybe a fall through?

Yes, i guess a fallthrough would be appropriate, given that the first call site has nothing to do with vector splats anyway.

[vsavchenko]

+1 for fallthrough

[steakhal]

• Uses FALLTHROUGH
• Handles CK_MatrixCast similarly to how the rest of the unhandled casts are evaluated
• Added tests to cover the switch case

[steakhal]

clang/test/Analysis/casts.c

201

BTW I don't know why are these Unknown. I thought it should be some conjured value. The same applies to the other 2.

[vsavchenko]

clang/test/Analysis/casts.c

201

Oh, I was (and still am) very sour about this! The store actually has all the required information (we'll have compoundVal{ x } here, but we absolutely cannot read it from the store! The logic there simply doesn't make sense! And whenever we use union objects that we know everything about, we get Unknownalmost every time.

[steakhal]

clang/test/Analysis/casts.c

201

Okay, does the same reasoning apply for the rest of the added test-cases?
What do you propose for moving forward?

[vsavchenko]

clang/test/Analysis/casts.c

201

It's not about your patch at all! I just saw union and Unknown and decided to rant about it 😅

[steakhal]

clang/test/Analysis/casts.c

201

Oh, fine.
What do you think about the other two cases? Shall I debug them, or let them go?

[vsavchenko]

clang/test/Analysis/casts.c

201

How was it before the patch?

[steakhal]

clang/test/Analysis/casts.c

201

The testfile passes as-is, before and after the patch.
It simply increases the coverage.

[vsavchenko]

clang/test/Analysis/casts.c

201

So, I'd say that this particular patch is good to go. And if you want to look into why these are Unknowns, it should probably be another patch. At least this is how I see it.

[NoQ]

Great, thanks!

clang/lib/StaticAnalyzer/Core/ExprEngineC.cpp

523

Dunno, is this the intended formatting for fallthrough? Seems kinda off near those braces.

[steakhal]

Thank you @NoQ and @vsavchenko for the review!

clang/lib/StaticAnalyzer/Core/ExprEngineC.cpp

523

I agree.
I'm using clang-format-10 at the moment.
Formats the diff automatically on save, so I assume it's the 'recommended' place.

[steakhal]

clang/lib/StaticAnalyzer/Core/ExprEngineC.cpp

523

It seems like the most recent clang-format behaves the same.

clang/lib/StaticAnalyzer/Core/ExprEngineC.cpp

ProgramStateRef ExprEngine::handleLValueBitCast(
   if (V.isUnknown() && !OrigV.isUnknown()) {
     state = escapeValues(state, OrigV, PSK_EscapeOther);
   }
   Bldr.generateNode(CastE, Pred, state);
 
   return state;
 }
 
-ProgramStateRef ExprEngine::handleLVectorSplat(
-    ProgramStateRef state, const LocationContext* LCtx, const CastExpr* CastE,
-    StmtNodeBuilder &Bldr, ExplodedNode* Pred) {
-  // Recover some path sensitivity by conjuring a new value.
-  QualType resultType = CastE->getType();
-  if (CastE->isGLValue())
-    resultType = getContext().getPointerType(resultType);
-  SVal result = svalBuilder.conjureSymbolVal(nullptr, CastE, LCtx,
-                                             resultType,
-                                             currBldrCtx->blockCount());
-  state = state->BindExpr(CastE, LCtx, result);
-  Bldr.generateNode(CastE, Pred, state);
-
-  return state;
-}
-
 void ExprEngine::VisitCast(const CastExpr *CastE, const Expr *Ex,
                            ExplodedNode *Pred, ExplodedNodeSet &Dst) {
 
   ExplodedNodeSet dstPreStmt;
   getCheckerManager().runCheckersForPreStmt(dstPreStmt, Pred, CastE, *this);
 
   if (CastE->getCastKind() == CK_LValueToRValue ||
       CastE->getCastKind() == CK_LValueToRValueBitCast) {
switch (CastE->getCastKind()) {
           SVal CastedPTMSV =
               svalBuilder.makePointerToMember(getBasicVals().accumCXXBase(
                   CastE->path(), *PTMSV, CastE->getCastKind()));
           state = state->BindExpr(CastE, LCtx, CastedPTMSV);
           Bldr.generateNode(CastE, Pred, state);
           continue;
         }
         // Explicitly proceed with default handler for this case cascade.
-        state = handleLVectorSplat(state, LCtx, CastE, Bldr, Pred);
-        continue;
       }
+        LLVM_FALLTHROUGH;
       // Various C++ casts that are not handled yet.
       case CK_ToUnion:
+      case CK_MatrixCast:
       case CK_VectorSplat: {
-        state = handleLVectorSplat(state, LCtx, CastE, Bldr, Pred);
-        continue;
-      }
-      case CK_MatrixCast: {
-        // TODO: Handle MatrixCast here.
+        QualType resultType = CastE->getType();
+        if (CastE->isGLValue())
+          resultType = getContext().getPointerType(resultType);
+        SVal result = svalBuilder.conjureSymbolVal(
+            /*symbolTag=*/nullptr, CastE, LCtx, resultType,
+            currBldrCtx->blockCount());
+        state = state->BindExpr(CastE, LCtx, result);
+        Bldr.generateNode(CastE, Pred, state);
         continue;
       }
     }
   }
 }
 
 void ExprEngine::VisitCompoundLiteralExpr(const CompoundLiteralExpr *CL,
                                           ExplodedNode *Pred,

clang/test/Analysis/casts.c

-// RUN: %clang_analyze_cc1 -triple x86_64-apple-darwin9 -analyzer-checker=core,alpha.core,debug.ExprInspection -analyzer-store=region -Wno-pointer-to-int-cast -verify -analyzer-config eagerly-assume=false %s
-// RUN: %clang_analyze_cc1 -triple i386-apple-darwin9 -analyzer-checker=core,alpha.core,debug.ExprInspection -analyzer-store=region -Wno-pointer-to-int-cast -verify -analyzer-config eagerly-assume=false %s
-// RUN: %clang_analyze_cc1 -triple x86_64-apple-darwin9 -analyzer-checker=core,alpha.core,debug.ExprInspection -Wno-pointer-to-int-cast -verify -DEAGERLY_ASSUME=1 -w %s
-// RUN: %clang_analyze_cc1 -triple i386-apple-darwin9 -analyzer-checker=core,alpha.core,debug.ExprInspection -Wno-pointer-to-int-cast -verify -DEAGERLY_ASSUME=1 -DBIT32=1 -w %s
+// RUN: %clang_analyze_cc1 -triple x86_64-apple-darwin9 -fenable-matrix -analyzer-checker=core,alpha.core,debug.ExprInspection -analyzer-store=region -Wno-pointer-to-int-cast -verify -analyzer-config eagerly-assume=false %s
+// RUN: %clang_analyze_cc1 -triple i386-apple-darwin9 -fenable-matrix -analyzer-checker=core,alpha.core,debug.ExprInspection -analyzer-store=region -Wno-pointer-to-int-cast -verify -analyzer-config eagerly-assume=false %s
+// RUN: %clang_analyze_cc1 -triple x86_64-apple-darwin9 -fenable-matrix -analyzer-checker=core,alpha.core,debug.ExprInspection -Wno-pointer-to-int-cast -verify -DEAGERLY_ASSUME=1 -w %s
+// RUN: %clang_analyze_cc1 -triple i386-apple-darwin9 -fenable-matrix -analyzer-checker=core,alpha.core,debug.ExprInspection -Wno-pointer-to-int-cast -verify -DEAGERLY_ASSUME=1 -DBIT32=1 -w %s
 
 extern void clang_analyzer_eval(_Bool);
 
 // Test if the 'storage' region gets properly initialized after it is cast to
 // 'struct sockaddr *'. 
 
 typedef unsigned char __uint8_t;
 typedef unsigned int __uint32_t;

 }
 
 void testSwitchWithSizeofs() {
   switch (sizeof(char) == 1) { // expected-warning{{switch condition has boolean value}}
   case sizeof(char):; // no-crash
   }
 }
 
+void test_ToUnion_cast(unsigned long long x) {
+  union Key {
+    unsigned long long data;
+  };
+  void clang_analyzer_dump_union(union Key);
+  clang_analyzer_dump_union((union Key)x); // expected-warning {{Unknown}}
+}
+
+typedef char cx5x5 __attribute__((matrix_type(5, 5)));
+typedef int ix5x5 __attribute__((matrix_type(5, 5)));
+void test_MatrixCast_cast(cx5x5 c) {
+  void clang_analyzer_dump_ix5x5(ix5x5);
+  clang_analyzer_dump_ix5x5((ix5x5)c); // expected-warning {{Unknown}}
+}
+
+void test_VectorSplat_cast(long x) {
+  typedef int __attribute__((ext_vector_type(2))) V;
+  void clang_analyzer_dump_V(V);
+  clang_analyzer_dump_V((V)x); // expected-warning {{Unknown}}
+}
+
 #endif
 
 #ifdef EAGERLY_ASSUME
 
 int globalA;
 extern int globalFunc();
 void no_crash_on_symsym_cast_to_long() {
   char c = globalFunc() - 5;