Change behavior with zero-sized static array extents

[aaron.ballman]

Summary

Currently, Clang diagnoses this code by default: void f(int a[static 0]); saying that "static has no effect on zero-length arrays" and this diagnostic is accurate for our implementation.

However, static array extents require that the caller of the function pass a nonnull pointer to an array of *at least* that number of elements, but it can pass more (see C17 6.7.6.3p6). Given that we allow zero-sized arrays as a GNU extension and that it's valid to pass more elements than specified by the static array extent, I think we should support zero-sized static array extents with the usual semantics because it can be useful, as pointed out to me on Twitter (https://twitter.com/rep_stosq_void/status/1280892279998291973):

void my_bzero(char p[static 0], int n);
my_bzero(&c+1, 0); //ok
my_bzero(t+k,n-k); //ok, pattern from actual code

[rjmccall]

clang/lib/CodeGen/CGCall.cpp

2521

Isn't the old logic still correct? If the element size is static and the element count is positive, the argument is dereferenceable out to their product; otherwise it's nonnull if null is the zero value and we aren't semantically allowing that to be a valid pointer.

[aaron.ballman]

clang/lib/CodeGen/CGCall.cpp

2521

I was questioning this -- I didn't think the old logic was correct because it checks that the array is in address space 0, but the nonnull-ness should apply regardless of address space (I think). The point about valid null pointers still stands, though. Am I misunderstanding the intended address space behavior?

[rjmccall]

clang/lib/CodeGen/CGCall.cpp

2521

I believe LLVM's nonnull actually always means nonzero. static just tells us that the address is valid, so (1) we always have to suppress the attribute under NullPointerIsValid and (2) we have the suppress the attribute if the null address is nonzero, because the zero address could still be valid in that case. The way the existing code is implementing the latter seems excessively conservative about non-standard address spaces, since we might know that they still use a zero null pointer; more importantly, it seems wrong in the face of an address space that lowers to LLVM's address space 0 but doesn't use a zero null pointer. You can call getTargetInfo().getNullPointerValue(ETy.getAddressSpace()) == 0 to answer this more correctly.

[aaron.ballman]

Updated based on review feedback.

clang/lib/CodeGen/CGCall.cpp

2521

Ah, I see! Thank you for the explanation -- I wasn't aware that null could be a valid address in other address spaces, but that makes sense.

[rjmccall]

Thanks, LGTM.

clang/lib/CodeGen/CGCall.cpp

2521

(the zero representation, not null)

[aaron.ballman]

Thanks for the review, I've gone ahead and committed in 006c49d890da633d1ce502117fc2a49863cd65b7

clang/lib/CodeGen/CGCall.cpp

2521

Agreed -- sorry for being imprecise when it mattered.

clang/include/clang/Basic/DiagnosticSemaKinds.td

 def err_typecheck_invalid_restrict_not_pointer_noarg : Error<
   "restrict requires a pointer or reference">;
 def err_typecheck_invalid_restrict_invalid_pointee : Error<
   "pointer to function type %0 may not be 'restrict' qualified">;
 def ext_typecheck_zero_array_size : Extension<
   "zero size arrays are an extension">, InGroup<ZeroLengthArray>;
 def err_typecheck_zero_array_size : Error<
   "zero-length arrays are not permitted in C++">;
-def warn_typecheck_zero_static_array_size : Warning<
-  "'static' has no effect on zero-length arrays">,
-  InGroup<ArrayBounds>;
 def err_array_size_non_int : Error<"size of array has non-integer type %0">;
 def err_init_element_not_constant : Error<
   "initializer element is not a compile-time constant">;
 def ext_aggregate_init_not_constant : Extension<
   "initializer for aggregate is not a compile-time constant">, InGroup<C99>;
 def err_local_cant_init : Error<
   "'__local' variable cannot have an initializer">;
 def err_loader_uninitialized_cant_init

clang/lib/CodeGen/CGCall.cpp

case ABIArgInfo::Direct: {
             // bytes).
             if (ArrTy->getSizeModifier() == ArrayType::Static) {
               QualType ETy = ArrTy->getElementType();
               uint64_t ArrSize = ArrTy->getSize().getZExtValue();
               if (!ETy->isIncompleteType() && ETy->isConstantSizeType() &&
                   ArrSize) {
                 llvm::AttrBuilder Attrs;
                 Attrs.addDereferenceableAttr(
-                  getContext().getTypeSizeInChars(ETy).getQuantity()*ArrSize);
+                    getContext().getTypeSizeInChars(ETy).getQuantity() *
+                    ArrSize);
                 AI->addAttrs(Attrs);
-              } else if (getContext().getTargetAddressSpace(ETy) == 0 &&
+              } else if (getContext().getTargetInfo().getNullPointerValue(
+                             ETy.getAddressSpace()) == 0 &&
                          !CGM.getCodeGenOpts().NullPointerIsValid) {
                 AI->addAttr(llvm::Attribute::NonNull);
               }
             }
           } else if (const auto *ArrTy =
                      getContext().getAsVariableArrayType(OTy)) {
             // For C99 VLAs with the static keyword, we don't know the size so
             // we can't use the dereferenceable attribute, but in addrspace(0)
             // we know that it must be nonnull.
             if (ArrTy->getSizeModifier() == VariableArrayType::Static &&
                 !getContext().getTargetAddressSpace(ArrTy->getElementType()) &&
                 !CGM.getCodeGenOpts().NullPointerIsValid)
               AI->addAttr(llvm::Attribute::NonNull);
           }
 
           // Set `align` attribute if any.
           const auto *AVAttr = PVD->getAttr<AlignValueAttr>();
           if (!AVAttr)

clang/lib/Sema/SemaType.cpp

if (!ArraySize) {
     }
     if (ConstVal == 0) {
       // GCC accepts zero sized static arrays. We allow them when
       // we're not in a SFINAE context.
       Diag(ArraySize->getBeginLoc(), isSFINAEContext()
                                          ? diag::err_typecheck_zero_array_size
                                          : diag::ext_typecheck_zero_array_size)
           << ArraySize->getSourceRange();
-
-      if (ASM == ArrayType::Static) {
-        Diag(ArraySize->getBeginLoc(),
-             diag::warn_typecheck_zero_static_array_size)
-            << ArraySize->getSourceRange();
-        ASM = ArrayType::Normal;
-      }
     } else if (!T->isDependentType() && !T->isVariablyModifiedType() &&
                !T->isIncompleteType() && !T->isUndeducedType()) {
       // Is the array too large?
       unsigned ActiveSizeBits
         = ConstantArrayType::getNumAddressingBits(Context, T, ConstVal);
       if (ActiveSizeBits > ConstantArrayType::getMaxSizeBits(Context)) {
         Diag(ArraySize->getBeginLoc(), diag::err_array_too_large)
             << ConstVal.toString(10) << ArraySize->getSourceRange();

clang/test/CodeGen/vla.c

 // provided.
 void test8(int a[static 3]) { }
 // CHECK: define void @test8(i32* dereferenceable(12) %a)
 
 void test9(int n, int a[static n]) { }
 // NULL-INVALID: define void @test9(i32 %n, i32* nonnull %a)
 // NULL-VALID: define void @test9(i32 %n, i32* %a)
 
+// Make sure a zero-sized static array extent is still required to be nonnull.
+void test10(int a[static 0]) {}
+// NULL-INVALID: define void @test10(i32* nonnull %a)
+// NULL-VALID: define void @test10(i32* %a)

clang/test/Sema/static-array.c

-// RUN: %clang_cc1 -triple x86_64-apple-macosx10.14.0 -fsyntax-only -fblocks -verify %s
+// RUN: %clang_cc1 -triple x86_64-apple-macosx10.14.0 -fsyntax-only -fblocks -pedantic -verify %s
 
-void cat0(int a[static 0]) {} // expected-warning {{'static' has no effect on zero-length arrays}}
+void cat0(int a[static 0]) {} // expected-warning {{zero size arrays are an extension}} \
+                              // expected-note {{callee declares array parameter as static here}}
 
 void cat(int a[static 3]) {} // expected-note 4 {{callee declares array parameter as static here}} expected-note 2 {{passing argument to parameter 'a' here}}
 
 void vat(int i, int a[static i]) {} // expected-note {{callee declares array parameter as static here}}
 
 void f(int *p) {
   int a[2], b[3], c[4];
 
-  cat0(0);
+  cat0(0); // expected-warning {{null passed to a callee that requires a non-null argument}}
 
   cat(0); // expected-warning {{null passed to a callee that requires a non-null argument}}
   cat(a); // expected-warning {{array argument is too small; contains 2 elements, callee requires at least 3}}
   cat(b);
   cat(c);
   cat(p);
 
   vat(1, 0); // expected-warning {{null passed to a callee that requires a non-null argument}}