Fixing array-to-pointer decay induced issue with conversion of the multidimentional arrays in C language.

[earnol]

This PR addresses issue #159603 and stops emitting diagnostics when multidimentional array attempted to be implicitly cast to the pointer type of the same base type.

[llvmbot]

@llvm/pr-subscribers-clang

Author: None (earnol)

Changes

This PR addresses issue #159603 and stops emitting diagnostics when multidimentional array attempted to be implicitly cast to the pointer type of the same base type.

---

Full diff: https://github.com/llvm/llvm-project/pull/159896.diff

2 Files Affected:

• (modified) clang/lib/Sema/SemaExpr.cpp (+135-65)
• (added) clang/test/Sema/multi-dim-array.c (+52)

diff --git a/clang/lib/Sema/SemaExpr.cpp b/clang/lib/Sema/SemaExpr.cpp
index 3b267c1b1693d..8ad05e56e3496 100644
--- a/clang/lib/Sema/SemaExpr.cpp
+++ b/clang/lib/Sema/SemaExpr.cpp
@@ -9032,15 +9032,107 @@ static bool IsInvalidCmseNSCallConversion(Sema &S, QualType FromType,
   return false;
 }
 
+/// This helper function returns element type if QT is a multidimentional array
+static std::optional<QualType> getMultiDimentionalArrayType(Sema const *S,
+                                                            QualType QT) {
+  const ArrayType *AT = S->Context.getAsArrayType(QT);
+  if (AT) {
+    const ConstantArrayType *CAT = llvm::dyn_cast<ConstantArrayType>(AT);
+    if (CAT) {
+      QualType ElementType = CAT->getElementType();
+      while (CAT) {
+        llvm::APInt Size = CAT->getSize();
+        // Zero sized array are no good.
+        if (Size == 0)
+          return std::optional<QualType>();
+        ElementType = CAT->getElementType();
+        CAT = llvm::dyn_cast<ConstantArrayType>(ElementType);
+      }
+      return std::optional<QualType>(
+          QualType(std::get<const Type *>(ElementType.split().asPair()), 0));
+    }
+  }
+  return std::optional<QualType>();
+}
+
+/// This helper function return AssignConvertType if pointers are not
+/// compatible
+static std::optional<AssignConvertType> checkPointerAssignmentCompatibility(
+    Sema &S, const Type *lhptee, const Type *rhptee, QualType ltrans,
+    QualType rtrans, QualType LHSType, QualType RHSType,
+    AssignConvertType ConvTy) {
+  if (!S.Context.typesAreCompatible(ltrans, rtrans)) {
+    // Check if the pointee types are compatible ignoring the sign.
+    // We explicitly check for char so that we catch "char" vs
+    // "unsigned char" on systems where "char" is unsigned.
+    if (lhptee->isCharType())
+      ltrans = S.Context.UnsignedCharTy;
+    else if (lhptee->hasSignedIntegerRepresentation())
+      ltrans = S.Context.getCorrespondingUnsignedType(ltrans);
+
+    if (rhptee->isCharType())
+      rtrans = S.Context.UnsignedCharTy;
+    else if (rhptee->hasSignedIntegerRepresentation())
+      rtrans = S.Context.getCorrespondingUnsignedType(rtrans);
+
+    if (ltrans == rtrans) {
+      // Types are compatible ignoring the sign. Qualifier incompatibility
+      // takes priority over sign incompatibility because the sign
+      // warning can be disabled.
+      if (!S.IsAssignConvertCompatible(ConvTy))
+        return ConvTy;
+
+      return AssignConvertType::IncompatiblePointerSign;
+    }
+
+    // If we are a multi-level pointer, it's possible that our issue is simply
+    // one of qualification - e.g. char ** -> const char ** is not allowed. If
+    // the eventual target type is the same and the pointers have the same
+    // level of indirection, this must be the issue.
+    if (isa<PointerType>(lhptee) && isa<PointerType>(rhptee)) {
+      Qualifiers lhq, rhq;
+      do {
+        std::tie(lhptee, lhq) =
+            cast<PointerType>(lhptee)->getPointeeType().split().asPair();
+        std::tie(rhptee, rhq) =
+            cast<PointerType>(rhptee)->getPointeeType().split().asPair();
+
+        // Inconsistent address spaces at this point is invalid, even if the
+        // address spaces would be compatible.
+        // FIXME: This doesn't catch address space mismatches for pointers of
+        // different nesting levels, like:
+        //   __local int *** a;
+        //   int ** b = a;
+        // It's not clear how to actually determine when such pointers are
+        // invalidly incompatible.
+        if (lhq.getAddressSpace() != rhq.getAddressSpace())
+          return AssignConvertType::
+              IncompatibleNestedPointerAddressSpaceMismatch;
+
+      } while (isa<PointerType>(lhptee) && isa<PointerType>(rhptee));
+
+      if (lhptee == rhptee)
+        return AssignConvertType::IncompatibleNestedPointerQualifiers;
+    }
+
+    // General pointer incompatibility takes priority over qualifiers.
+    if (RHSType->isFunctionPointerType() && LHSType->isFunctionPointerType())
+      return AssignConvertType::IncompatibleFunctionPointer;
+
+    // Return pointers as incompatible
+    return AssignConvertType::IncompatiblePointer;
+  }
+  return std::optional<AssignConvertType>();
+}
+
 // checkPointerTypesForAssignment - This is a very tricky routine (despite
 // being closely modeled after the C99 spec:-). The odd characteristic of this
 // routine is it effectively iqnores the qualifiers on the top level pointee.
 // This circumvents the usual type rules specified in 6.2.7p1 & 6.7.5.[1-3].
 // FIXME: add a couple examples in this comment.
-static AssignConvertType checkPointerTypesForAssignment(Sema &S,
-                                                        QualType LHSType,
-                                                        QualType RHSType,
-                                                        SourceLocation Loc) {
+static AssignConvertType
+checkPointerTypesForAssignment(Sema &S, QualType LHSType, QualType RHSType,
+                               QualType OrigRHSType, SourceLocation Loc) {
   assert(LHSType.isCanonical() && "LHS not canonicalized!");
   assert(RHSType.isCanonical() && "RHS not canonicalized!");
 
@@ -9128,65 +9220,26 @@ static AssignConvertType checkPointerTypesForAssignment(Sema &S,
 
   // C99 6.5.16.1p1 (constraint 3): both operands are pointers to qualified or
   // unqualified versions of compatible types, ...
-  QualType ltrans = QualType(lhptee, 0), rtrans = QualType(rhptee, 0);
-  if (!S.Context.typesAreCompatible(ltrans, rtrans)) {
-    // Check if the pointee types are compatible ignoring the sign.
-    // We explicitly check for char so that we catch "char" vs
-    // "unsigned char" on systems where "char" is unsigned.
-    if (lhptee->isCharType())
-      ltrans = S.Context.UnsignedCharTy;
-    else if (lhptee->hasSignedIntegerRepresentation())
-      ltrans = S.Context.getCorrespondingUnsignedType(ltrans);
-
-    if (rhptee->isCharType())
-      rtrans = S.Context.UnsignedCharTy;
-    else if (rhptee->hasSignedIntegerRepresentation())
-      rtrans = S.Context.getCorrespondingUnsignedType(rtrans);
-
-    if (ltrans == rtrans) {
-      // Types are compatible ignoring the sign. Qualifier incompatibility
-      // takes priority over sign incompatibility because the sign
-      // warning can be disabled.
-      if (!S.IsAssignConvertCompatible(ConvTy))
-        return ConvTy;
-
-      return AssignConvertType::IncompatiblePointerSign;
-    }
-
-    // If we are a multi-level pointer, it's possible that our issue is simply
-    // one of qualification - e.g. char ** -> const char ** is not allowed. If
-    // the eventual target type is the same and the pointers have the same
-    // level of indirection, this must be the issue.
-    if (isa<PointerType>(lhptee) && isa<PointerType>(rhptee)) {
-      do {
-        std::tie(lhptee, lhq) =
-          cast<PointerType>(lhptee)->getPointeeType().split().asPair();
-        std::tie(rhptee, rhq) =
-          cast<PointerType>(rhptee)->getPointeeType().split().asPair();
-
-        // Inconsistent address spaces at this point is invalid, even if the
-        // address spaces would be compatible.
-        // FIXME: This doesn't catch address space mismatches for pointers of
-        // different nesting levels, like:
-        //   __local int *** a;
-        //   int ** b = a;
-        // It's not clear how to actually determine when such pointers are
-        // invalidly incompatible.
-        if (lhq.getAddressSpace() != rhq.getAddressSpace())
-          return AssignConvertType::
-              IncompatibleNestedPointerAddressSpaceMismatch;
-
-      } while (isa<PointerType>(lhptee) && isa<PointerType>(rhptee));
-
-      if (lhptee == rhptee)
-        return AssignConvertType::IncompatibleNestedPointerQualifiers;
-    }
-
-    // General pointer incompatibility takes priority over qualifiers.
-    if (RHSType->isFunctionPointerType() && LHSType->isFunctionPointerType())
-      return AssignConvertType::IncompatibleFunctionPointer;
-    return AssignConvertType::IncompatiblePointer;
+  QualType ltrans = QualType(lhptee, 0);
+  QualType rtrans;
+  std::optional<AssignConvertType> CvtT;
+  std::optional<QualType> ArrET = getMultiDimentionalArrayType(&S, OrigRHSType);
+  if (ArrET) {
+    rtrans = ArrET.value();
+    CvtT = checkPointerAssignmentCompatibility(
+        S, lhptee, rhptee, ltrans, rtrans, LHSType, RHSType, ConvTy);
+    if (CvtT && CvtT.value() != AssignConvertType::IncompatiblePointer)
+      return CvtT.value();
+  }
+  if (!ArrET ||
+      (CvtT && CvtT.value() == AssignConvertType::IncompatiblePointer)) {
+    rtrans = QualType(rhptee, 0);
+    CvtT = checkPointerAssignmentCompatibility(
+        S, lhptee, rhptee, ltrans, rtrans, LHSType, RHSType, ConvTy);
+    if (CvtT)
+      return CvtT.value();
   }
+
   bool DiscardingCFIUncheckedCallee, AddingCFIUncheckedCallee;
   if (!S.getLangOpts().CPlusPlus &&
       S.IsFunctionConversion(ltrans, rtrans, &DiscardingCFIUncheckedCallee,
@@ -9313,6 +9366,22 @@ static bool isVector(QualType QT, QualType ElementType) {
   return false;
 }
 
+/// This helper function returns pre array-to-pointer decay type if possible
+static QualType getPreDecayType(ExprResult &RHS) {
+  QualType OrigRHSType = RHS.get()->getType();
+  ImplicitCastExpr *ICE = dyn_cast<ImplicitCastExpr>(RHS.get());
+  if (ICE) {
+    Expr *E = ICE->getSubExpr();
+    if (E) {
+      DeclRefExpr *DRE = dyn_cast<DeclRefExpr>(E);
+      if (DRE) {
+        OrigRHSType = DRE->getType();
+      }
+    }
+  }
+  return OrigRHSType;
+}
+
 /// CheckAssignmentConstraints (C99 6.5.16) - This routine currently
 /// has code to accommodate several GCC extensions when type checking
 /// pointers. Here are some objectionable examples that GCC considers warnings:
@@ -9510,7 +9579,9 @@ AssignConvertType Sema::CheckAssignmentConstraints(QualType LHSType,
         Kind = CK_NoOp;
       else
         Kind = CK_BitCast;
-      return checkPointerTypesForAssignment(*this, LHSType, RHSType,
+
+      QualType OrigType = getPreDecayType(RHS);
+      return checkPointerTypesForAssignment(*this, LHSType, RHSType, OrigType,
                                             RHS.get()->getBeginLoc());
     }
 
@@ -9629,7 +9700,6 @@ AssignConvertType Sema::CheckAssignmentConstraints(QualType LHSType,
                               Context.getObjCClassRedefinitionType())) {
         return AssignConvertType::Compatible;
       }
-
       return AssignConvertType::IncompatiblePointer;
     }
 
@@ -9902,7 +9972,7 @@ AssignConvertType Sema::CheckSingleAssignmentConstraints(QualType LHSType,
             RHS.get()->getType().getCanonicalType().getUnqualifiedType();
         QualType CanLHS = LHSType.getCanonicalType().getUnqualifiedType();
         if (CanRHS->isVoidPointerType() && CanLHS->isPointerType()) {
-          Ret = checkPointerTypesForAssignment(*this, CanLHS, CanRHS,
+          Ret = checkPointerTypesForAssignment(*this, CanLHS, CanRHS, CanRHS,
                                                RHS.get()->getExprLoc());
           // Anything that's not considered perfectly compatible would be
           // incompatible in C++.
diff --git a/clang/test/Sema/multi-dim-array.c b/clang/test/Sema/multi-dim-array.c
new file mode 100644
index 0000000000000..febd13a21d35e
--- /dev/null
+++ b/clang/test/Sema/multi-dim-array.c
@@ -0,0 +1,52 @@
+// RUN: %clang_cc1 %s -fsyntax-only -verify -verify=c2x -pedantic -Wno-strict-prototypes -Wno-zero-length-array
+
+int array_acceptor_good(unsigned long * par1)
+{
+  return par1 != (unsigned long *)0;
+}
+
+int array_acceptor_bad(unsigned long * par1) // expected-note {{passing argument to parameter 'par1' here}}
+{
+  return par1 != (unsigned long *)0;
+}
+
+int array_acceptor_bad2(unsigned long * par1) // expected-note {{passing argument to parameter 'par1' here}}
+{
+  return par1 != (unsigned long *)0;
+}
+
+struct S
+{
+  int a;
+};
+
+int array_acceptor_bad1(struct S * par1) // expected-note {{passing argument to parameter 'par1' here}}
+{
+  return par1 != (struct S *)0;
+}
+
+int array_struct_acceptor(struct S * par1)
+{
+  return par1 != (struct S *)0;
+}
+
+
+int array_tester()
+{
+  unsigned long mdarr[5][6];
+  double mddarr[5][6];
+  unsigned long sdarr[30];
+  unsigned long mdarr3d[5][6][2];
+  unsigned long mdarr4d[5][6][2][1];
+  unsigned long mdarrz4d[5][6][0][1];
+  struct S mdsarr[5][6][2];
+
+  array_acceptor_good(sdarr);
+  array_acceptor_good(mdarr);
+  array_acceptor_good(mdarr3d);
+  array_acceptor_good(mdarr4d);
+  array_acceptor_bad(mddarr); // expected-error {{incompatible pointer types passing 'double[5][6]' to parameter of type 'unsigned long *'}}
+  array_acceptor_bad1(mddarr); // expected-error {{incompatible pointer types passing 'double[5][6]' to parameter of type 'struct S *'}}
+  array_acceptor_bad2(mdarrz4d); // expected-error {{incompatible pointer types passing 'unsigned long[5][6][0][1]' to parameter of type 'unsigned long *'}}
+  array_struct_acceptor(mdsarr);
+}
\ No newline at end of file

[Sirraide]

I’m not entirely convinced Clang is wrong here? (See #159603 (comment) but basically every C compiler I could find diagnoses this)

[earnol]

First of all, thank you for looking into this PR.
Yes. I also checked with other compilers and noticed the same with gcc, however what others do is not a point of authority here for me - the C standard and practical considerations are.
And the practical considerations are the multidimensional array[m][n] is guaranteed to be stored as a whole block of size n × m, thus making pointers actually compatible.
Also this specific condition was not tested at all. I had to change zero existing lit tests when making my PR. This is the second factor which convinced me my change is correct.

Edit: Responded in #159603 thread in more details.

[cor3ntin]

This is more a language design level feature than a fix - lets have the discussion on the linked issue (I did not look at the specifics of the code)

[earnol]

@cor3ntin I agree. Let's table it until we come to the conclusion and have an action plan in: #159603

[earnol]

@AaronBallman Please check this PR and associated discussion #159603.
Summary of the goal here is not have an error (warning is ok) when multidimentional array being cast to the pointer of the same base type.