[analysis] Fix value tracking for pointers to qualified types

[vabridgers]

Summary

This change fixes part 1 described by Artem in the Bugzilla report 43364. The comparison done was on a canonical, but should have been done on an unqualified type. Without using the unqualified type, the type comparison in this specific case is for "const Type * const" against "Type * const", which for the purposes of static analysis can be done not considering the Type's const qualifier. This is best done using a nonqualified type comparison.
Test cases were added to cover this change.

[NoQ]

Thanks!! This definitely doesn't sort out all the problems of this kind, but that's a strict improvement.

Do you have commit access? Or i could commit for you.

clang/test/Analysis/uninit-val-const-likeness.c

22

I suggest adding // no-warning markers on the lines on which warnings were previously emitted. It doesn't have any physical meaning, just makes it easier to understand what the test is about when you accidentally break it.

[NoQ]

Hmm, it was supposed to get a green tick mark. Let me re-accept.

[xazax.hun]

Great! I have one question though. Will this also work as intended with sugared types? (e.g. typedefs)
I believe this might be one of the main reason why the original author used canonical types in the first place.

[NoQ]

In D70836#1789059, @xazax.hun wrote:

Will this also work as intended with sugared types? (e.g. typedefs)
I believe this might be one of the main reason why the original author used canonical types in the first place.

Whoops, indeed. Fxd in rGf0ced2ddb44e.

clang/lib/StaticAnalyzer/Core/Store.cpp

if (const auto *SR = dyn_cast<SymbolicRegion>(MR)) {
     return loc::MemRegionVal(GetElementZeroRegion(SR, TargetType));
   }
 
   // We failed if the region we ended up with has perfect type info.
   Failed = isa<TypedValueRegion>(MR);
   return UnknownVal();
 }
 
+static bool hasSameUnqualifiedPointeeType(QualType ty1, QualType ty2) {
+  return ty1->getPointeeType().getTypePtr() == 
+    ty2->getPointeeType().getTypePtr();
+}
+
 /// 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;
 
SVal StoreManager::CastRetrievedVal(SVal V, const TypedValueRegion *R,
   // 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.
   if (castTy->isPointerType() && !castTy->isVoidPointerType())
-    if (const auto *SR = dyn_cast_or_null<SymbolicRegion>(V.getAsRegion()))
-      if (SR->getSymbol()->getType().getCanonicalType() !=
-          castTy.getCanonicalType())
+    if (const auto *SR = dyn_cast_or_null<SymbolicRegion>(V.getAsRegion())) {
+      QualType sr = SR->getSymbol()->getType(); 
+      if (!hasSameUnqualifiedPointeeType(sr, castTy))
           return loc::MemRegionVal(castRegion(SR, castTy));
+    }
 
   return svalBuilder.dispatchCast(V, castTy);
 }
 
 SVal StoreManager::getLValueFieldOrIvar(const Decl *D, SVal Base) {
   if (Base.isUnknownOrUndef())
     return Base;
 

clang/test/Analysis/uninit-val-const-likeness.c

+// RUN: %clang_analyze_cc1 -analyzer-checker=core %s -verify 
+// expected-no-diagnostics
+
+#define SIZE 2
+
+typedef struct {
+  int noOfSymbols;
+} Params;
+
+static void create(const Params * const params, int fooList[]) {
+  int tmpList[SIZE] = {0};
+  for (int i = 0; i < params->noOfSymbols; i++)
+    fooList[i] = tmpList[i];
+}
+
+int work(Params * const params) {
+  int fooList[SIZE];
+  create(params, fooList);
+  int sum = 0;
+  for (int i = 0; i < params->noOfSymbols; i++)
+    sum += fooList[i]; // no-warning
+  return sum;
+}
+
+static void create2(const Params * const * pparams, int fooList[]) {
+  const Params * params = *pparams;
+  int tmpList[SIZE] = {0};
+  for (int i = 0; i < params->noOfSymbols; i++)
+    fooList[i] = tmpList[i];
+}
+
+int work2(const Params * const params) {
+  int fooList[SIZE];
+  create2(&params, fooList);
+  int sum = 0;
+  for (int i = 0; i < params->noOfSymbols; i++)
+    sum += fooList[i]; // no-warning
+  return sum;
+}
+
+static void create3(Params * const * pparams, int fooList[]) {
+  const Params * params = *pparams;
+  int tmpList[SIZE] = {0};
+  for (int i = 0; i < params->noOfSymbols; i++)
+    fooList[i] = tmpList[i];
+}
+
+int work3(const Params * const params) {
+  int fooList[SIZE];
+  Params *const *ptr = (Params *const*)&params;
+  create3(ptr, fooList);
+  int sum = 0;
+  for (int i = 0; i < params->noOfSymbols; i++)
+    sum += fooList[i]; // no-warning
+  return sum;
+}