[clang] Introduce -Warray-parameter

[serge-sans-paille]

Summary

This warning exist in GCC[0] and warns about re-declarations of functions
involving arguments of array or pointer types of inconsistent kinds or forms.

This is not the exact same implementation as GCC's : there's no warning level
and that flag has no effect on -Warray-bounds.

[0] https://gcc.gnu.org/onlinedocs/gcc-12.1.0/gcc/Warning-Options.html#index-Wno-array-parameter

[serge-sans-paille]

Activate -Warray-parameter on -Wmost but disable it by default

[aaron.ballman]

Thanks for working on this!

clang/lib/Sema/SemaDecl.cpp

49

Is this new include necessary?

3270–3273

clang/test/Misc/warning-wall.c

101

Something went sideways here (perhaps saved with the wrong line endings?).

clang/test/Sema/array-parameter.c

3

I'd like to see some additional test cases:

void func(int *p);
void func(int a[2]); // Diagnose this one, I presume?
void func(int a[]); // But also diagnose this one as well, yes?
void func(int a[2]) {} // Do we then diagnose this as well, or is this matching a previous declaration and thus fine?

void other(int n, int m, int a[n]);
void other(int n, int m, int a[m]); // Hopefully we diagnose this set!

void another(int n, int array[n]);
void another(int n, int array[*]); // I *think* this should be warned, but I'm still a bit on the fence about it

Also, if this is expected to work in C++ as well, we should probably have cases like:

template <int N>
void func(int i[10]);

template <int N>
void func(int i[N]); // Should we diagnose this before instantiation or wait until we see the instantiation and only diagnose if differs?

static constexpr int Extent = 10;
void other(int i[10]);
void other(int i[Extent]); // Should not be diagnosed

13–20

I don't think we should diagnose any of these -- the array bounds do match. GCC doesn't diagnose them either.

22–23

I don't think we should diagnose this case either -- the [*] case is denoting a parameter of variable-length array type with no size information, and [] is the same except not a variable-length array type. So it's hard to argue that the bounds are not matched properly.

[serge-sans-paille]

Take review into account : rework indentation, style cleaning and be more accurate about bounds reporting

[aaron.ballman]

clang/lib/Sema/SemaDecl.cpp

3220

3224

3234

3277–3281

The diagnostics engine knows how to format NamedDecl objects as well as types. Can you do this instead and still get reasonable diagnostics?

clang/test/Sema/array-parameter.c

3

Do you plan to add the C++ tests?

[serge-sans-paille]

Take review into account + add C++ test file.

clang/test/Sema/array-parameter.c

3

template <int N>
void func(int i[N]); // Should we diagnose this before instantiation or wait until we see the instantiation and only diagnose if differs?

GCC never warns about this. Current patch always warn and this looks fine to me.

[shafik]

clang/test/Sema/array-parameter.c

18

Since we are covering static, const and restict we should also cover volatile for completeness.

[serge-sans-paille]

Extra test

[aaron.ballman]

clang/lib/Sema/SemaDecl.cpp

3216

I forgot about this gotcha -- arrays are special in that you shouldn't be using cast<> and friends on them, you need to ask the ASTContext to go from the QualType to the correct array type. e.g., ASTContext::getAsConstantArrayType() and ASTContext::getAsVariableArrayType() -- I think this is the cause of the failed assertions we're seeing in precommit CI.

[serge-sans-paille]

Fix handling of ConstantArrayType, thanks @aaron.ballman for the hint.

[aaron.ballman]

clang/docs/ReleaseNotes.rst

324–326

clang/lib/Sema/SemaDecl.cpp

3216

I'm still not comfortable with the way this is written -- please go through the ASTContext instead, with something like:

if (Ty->isIncompleteArrayType() || Ty->isPointerType())
  return true;

if (const auto *VAT = Ctx.getAsVariableArrayType(Ty))
  return VAT->getSizeModifier() == ArrayType::ArraySizeModifier::Star;

return false;

3226–3227

Same here.

clang/test/Sema/array-parameter.c

18

+1, might as well round out the set.

clang/test/Sema/array-parameter.cpp

4–8

One more test, now that I'm thinking about explicit specializations:

template <int N>
void func(int (&Val)[N]);

template <>
void func<10>(int (&Val)[10]) {
}

I don't think this should get any diagnostics.

[serge-sans-paille]

Take review into account.

[aaron.ballman]

LGTM, thank you for the new diagnostic!

clang/lib/Sema/SemaDecl.cpp

3213

East const? MONSTEROUS! ;-) (We tend to use west const mostly in the code base.)

[serge-sans-paille]

clang/lib/Sema/SemaDecl.cpp

3213

Whenever I don't have strict focus on my fingers, they go east!

[alexfh]

Serge, this diagnostic doesn't handle non-type template parameters correctly in some cases. Here's an example derived from a real code: https://gcc.godbolt.org/z/cvP8od5c6

template<int K>
struct T {
  static void F(int a[8 * K]);
};
template<int K>
void T<K>::F(int a[8 * K]) {}

<source>:6:18: warning: argument 'a' of type 'int[8 * K]' with mismatched bound [-Warray-parameter]
void T<K>::F(int a[8 * K]) {}
                 ^
<source>:3:21: note: previously declared as 'int[8 * K]' here
  static void F(int a[8 * K]);
                    ^

Do you see an obvious fix? If not, please revert while investigating.

[serge-sans-paille]

Thanks for the extra test case!

Do you see an obvious fix? If not, please revert while investigating.

Obvious fix landed in 66fa2847a775dda27ddcac3832769441727db42f

clang/docs/ReleaseNotes.rst

- A new ARM pass to workaround Cortex-A57 Erratum 1742098 and Cortex-A72 Erratum
   ``-mno-fix-cortex-a57-aes-1742098`` or ``-mno-fix-cortex-a72-aes-1655431``.
 - Added the ``-fexperimental-max-bitint-width=`` option to increase the maximum
   allowed bit width of ``_BitInt`` types beyond the default of 128 bits. Some
   operations, such as division or float-to-integer conversion, on ``_BitInt``
   types with more than 128 bits currently crash clang. This option will be
   removed in the future once clang supports all such operations.
 - Added the ``-print-diagnostic-options`` option, which prints a list of
   warnings the compiler supports.
+- Added the ``-Warray-parameter`` warning. It diagnoses differences between
+  array parameters between function redeclarations (arrays of different extents,
+  etc). This flag is related to the same flag in GCC, but is different in that
+  it does not accept an explicitly- specified warning level and use of this flag
+  has no effect on ``-Warray-bounds``.
 
 Deprecated Compiler Flags
 -------------------------
 
 Modified Compiler Flags
 -----------------------
 
 Removed Compiler Flags

clang/include/clang/Basic/DiagnosticGroups.td

 def AddressOfTemporary : DiagGroup<"address-of-temporary">;
 def : DiagGroup<"aggregate-return">;
 def GNUAlignofExpression : DiagGroup<"gnu-alignof-expression">;
 def AmbigMemberTemplate : DiagGroup<"ambiguous-member-template">;
 def GNUAnonymousStruct : DiagGroup<"gnu-anonymous-struct">;
 def GNUAutoType : DiagGroup<"gnu-auto-type">;
 def ArrayBounds : DiagGroup<"array-bounds">;
 def ArrayBoundsPointerArithmetic : DiagGroup<"array-bounds-pointer-arithmetic">;
+def ArrayParameter : DiagGroup<"array-parameter">;
 def AutoDisableVptrSanitizer : DiagGroup<"auto-disable-vptr-sanitizer">;
 def Availability : DiagGroup<"availability">;
 def Section : DiagGroup<"section">;
 def AutoImport : DiagGroup<"auto-import">;
 def FrameworkHdrQuotedInclude : DiagGroup<"quoted-include-in-framework-header">;
 def FrameworkIncludePrivateFromPublic :
   DiagGroup<"framework-include-private-from-public">;
 def FrameworkHdrAtImport : DiagGroup<"atimport-in-framework-header">;
def Extra : DiagGroup<"extra", [
     NullPointerArithmetic,
     NullPointerSubtraction,
     EmptyInitStatement,
     StringConcatation,
     FUseLdPath,
   ]>;
 
 def Most : DiagGroup<"most", [
+    ArrayParameter,
     BoolOperation,
     CharSubscript,
     Comment,
     DeleteNonVirtualDtor,
     Format,
     ForLoopAnalysis,
     FrameAddress,
     Implicit,

clang/include/clang/Basic/DiagnosticSemaKinds.td

def warn_ptr_arith_exceeds_max_addressable_bounds : Warning<
   InGroup<ArrayBounds>;
 def warn_array_index_exceeds_max_addressable_bounds : Warning<
   "array index %0 refers past the last possible element for an array in %1-bit "
   "address space containing %2-bit (%3-byte) elements (max possible %4 element%s5)">,
   InGroup<ArrayBounds>;
 def note_array_declared_here : Note<
   "array %0 declared here">;
 
+def warn_inconsistent_array_form : Warning<
+  "argument %0 of type %1 with mismatched bound">,
+  InGroup<ArrayParameter>, DefaultIgnore;
+def note_previous_declaration_as : Note<
+  "previously declared as %0 here">;
+
 def warn_printf_insufficient_data_args : Warning<
   "more '%%' conversions than data arguments">, InGroup<FormatInsufficientArgs>;
 def warn_printf_data_arg_not_used : Warning<
   "data argument not used by format string">, InGroup<FormatExtraArgs>;
 def warn_format_invalid_conversion : Warning<
   "invalid conversion specifier '%0'">, InGroup<FormatInvalidSpecifier>;
 def warn_printf_incomplete_specifier : Warning<
   "incomplete format specifier">, InGroup<Format>;

clang/lib/Sema/SemaDecl.cpp

 #include "clang/Sema/Lookup.h"
 #include "clang/Sema/ParsedTemplate.h"
 #include "clang/Sema/Scope.h"
 #include "clang/Sema/ScopeInfo.h"
 #include "clang/Sema/SemaInternal.h"
 #include "clang/Sema/Template.h"
 #include "llvm/ADT/SmallString.h"
 #include "llvm/ADT/Triple.h"
 #include <algorithm>
 #include <cstring>
 #include <functional>
 #include <unordered_map>
 
 using namespace clang;
 using namespace sema;
 
 Sema::DeclGroupPtrTy Sema::ConvertDeclToDeclGroup(Decl *Ptr, Decl *OwnedType) {
if (!DeclHasAttr(newDecl, I)) {
       newDecl->addAttr(newAttr);
       foundAny = true;
     }
   }
 
   if (!foundAny) newDecl->dropAttrs();
 }
 
+static bool EquivalentArrayTypes(QualType Old, QualType New,
+                                 ASTContext const &Ctx) {
+
+  auto NoSizeInfo = [&Ctx](QualType Ty) {
+    if (Ty->isIncompleteArrayType() || Ty->isPointerType())
+      return true;
+    if (const auto *VAT = Ctx.getAsVariableArrayType(Ty))
+      return VAT->getSizeModifier() == ArrayType::ArraySizeModifier::Star;
+    return false;
+  };
+
+  // `type[]` is equivalent to `type *` and `type[*]`.
+  if (NoSizeInfo(Old) && NoSizeInfo(New))
+    return true;
+
+  // Don't try to compare VLA sizes, unless one of them has the star modifier.
+  if (Old->isVariableArrayType() && New->isVariableArrayType()) {
+    const auto *OldVAT = Ctx.getAsVariableArrayType(Old);
+    const auto *NewVAT = Ctx.getAsVariableArrayType(New);
+    if ((OldVAT->getSizeModifier() == ArrayType::ArraySizeModifier::Star) ^
+        (NewVAT->getSizeModifier() == ArrayType::ArraySizeModifier::Star))
+      return false;
+    return true;
+  }
+
+  // Only compare size, ignore Size modifiers and CVR.
+  if (Old->isConstantArrayType() && New->isConstantArrayType())
+    return Ctx.getAsConstantArrayType(Old)->getSize() ==
+           Ctx.getAsConstantArrayType(New)->getSize();
+
+  return Old == New;
+}
+
 static void mergeParamDeclTypes(ParmVarDecl *NewParam,
                                 const ParmVarDecl *OldParam,
                                 Sema &S) {
   if (auto Oldnullability = OldParam->getType()->getNullability(S.Context)) {
     if (auto Newnullability = NewParam->getType()->getNullability(S.Context)) {
       if (*Oldnullability != *Newnullability) {
         S.Diag(NewParam->getLocation(), diag::warn_mismatched_nullability_attr)
           << DiagNullabilityKind(
if (auto Oldnullability = OldParam->getType()->getNullability(S.Context)) {
     } else {
       QualType NewT = NewParam->getType();
       NewT = S.Context.getAttributedType(
                          AttributedType::getNullabilityAttrKind(*Oldnullability),
                          NewT, NewT);
       NewParam->setType(NewT);
     }
   }
+  const auto *OldParamDT = dyn_cast<DecayedType>(OldParam->getType());
+  const auto *NewParamDT = dyn_cast<DecayedType>(NewParam->getType());
+  if (OldParamDT && NewParamDT &&
+      OldParamDT->getPointeeType() == NewParamDT->getPointeeType()) {
+    QualType OldParamOT = OldParamDT->getOriginalType();
+    QualType NewParamOT = NewParamDT->getOriginalType();
+    if (!EquivalentArrayTypes(OldParamOT, NewParamOT, S.getASTContext())) {
+      S.Diag(NewParam->getLocation(), diag::warn_inconsistent_array_form)
+          << NewParam << NewParamOT;
+      S.Diag(OldParam->getLocation(), diag::note_previous_declaration_as)
+          << OldParamOT;
+    }
+  }
 }
 
 namespace {
 
 /// Used in MergeFunctionDecl to keep track of function parameters in
 /// C.
 struct GNUCompatibleParamWarning {
   ParmVarDecl *OldParm;

clang/test/Misc/warning-wall.c

 RUN: diagtool tree -Wall > %t 2>&1
 RUN: FileCheck --input-file=%t %s
 
      CHECK:-Wall
 CHECK-NEXT:  -Wmost
+CHECK-NEXT:    -Warray-parameter
 CHECK-NEXT:    -Wbool-operation
 CHECK-NEXT:    -Wbitwise-instead-of-logical
 CHECK-NEXT:    -Wchar-subscripts
 CHECK-NEXT:    -Wcomment
 CHECK-NEXT:    -Wdelete-non-virtual-dtor
 CHECK-NEXT:      -Wdelete-non-abstract-non-virtual-dtor
 CHECK-NEXT:      -Wdelete-abstract-non-virtual-dtor
 CHECK-NEXT:    -Wformat

 CHECK-NEXT:    -Wbitwise-conditional-parentheses
 CHECK-NEXT:    -Wbitwise-op-parentheses
 CHECK-NEXT:    -Wshift-op-parentheses
 CHECK-NEXT:    -Woverloaded-shift-op-parentheses
 CHECK-NEXT:    -Wparentheses-equality
 CHECK-NEXT:    -Wdangling-else
 CHECK-NEXT:  -Wswitch
 CHECK-NEXT:  -Wswitch-bool
 CHECK-NEXT:  -Wmisleading-indentation
 
 
 CHECK-NOT:-W

clang/test/Sema/array-parameter.c

+// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -fsyntax-only -Warray-parameter -verify %s
+void f0(int a[]);
+void f0(int *a); // no warning
+
+void f1(int a[]);  // expected-note {{previously declared as 'int[]' here}}
+void f1(int a[2]); // expected-warning {{argument 'a' of type 'int[2]' with mismatched bound}}
+
+void f2(int a[3]); // expected-note {{previously declared as 'int[3]' here}}
+void f2(int a[2]); // expected-warning {{argument 'a' of type 'int[2]' with mismatched bound}}
+
+void f3(int a[const 2]);
+void f3(int a[2]); // no warning
+
+void f4(int a[static 2]);
+void f4(int a[2]); // no warning
+
+void f5(int a[restrict 2]);
+void f5(int a[2]); // no warning
+
+void f6(int a[volatile 2]);
+void f6(int a[2]); // no warning
+
+void f7(int a[*]);
+void f7(int a[]); // no warning
+
+void f8(int n, int a[*]); // expected-note {{previously declared as 'int[*]' here}}
+void f8(int n, int a[n]); // expected-warning {{argument 'a' of type 'int[n]' with mismatched bound}}
+
+void f9(int *a);
+void f9(int a[2]);
+void f9(int a[]); // expected-warning {{argument 'a' of type 'int[]' with mismatched bound}}
+                  // expected-note@-2 {{previously declared as 'int[2]' here}}
+void f9(int a[2]) // expected-warning {{argument 'a' of type 'int[2]' with mismatched bound}}
+                  // expected-note@-3 {{previously declared as 'int[]' here}}
+{}

clang/test/Sema/array-parameter.cpp

+// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -fsyntax-only -Warray-parameter -verify %s
+
+template <int N>
+void func(int i[10]); // expected-note {{previously declared as 'int[10]' here}}
+
+template <int N>
+void func(int i[N]); // expected-warning {{argument 'i' of type 'int[N]' with mismatched bound}}
+
+template <int N>
+void func(int (&Val)[N]);
+
+template <>
+void func<10>(int (&Val)[10]) {
+}
+
+static constexpr int Extent = 10;
+void funk(int i[10]);
+void funk(int i[Extent]); // no-warning