[PATCH] D83502: Change behavior with zero-sized static array extents

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman closed this revision.
aaron.ballman marked an inline comment as done.
aaron.ballman added a comment.

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





Comment at: clang/lib/CodeGen/CGCall.cpp:2515
+} else {
+  AI->addAttr(llvm::Attribute::NonNull);
+}

rjmccall wrote:
> aaron.ballman wrote:
> > rjmccall wrote:
> > > aaron.ballman wrote:
> > > > rjmccall wrote:
> > > > > 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.
> > > > 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?
> > > 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.
> > 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.
> (the zero representation, not null)
Agreed -- sorry for being imprecise when it mattered.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D83502/new/

https://reviews.llvm.org/D83502



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D83502: Change behavior with zero-sized static array extents

[John McCall via Phabricator via cfe-commits]

rjmccall accepted this revision.
rjmccall added a comment.
This revision is now accepted and ready to land.

Thanks, LGTM.




Comment at: clang/lib/CodeGen/CGCall.cpp:2515
+} else {
+  AI->addAttr(llvm::Attribute::NonNull);
+}

aaron.ballman wrote:
> rjmccall wrote:
> > aaron.ballman wrote:
> > > rjmccall wrote:
> > > > 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.
> > > 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?
> > 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.
> 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.
(the zero representation, not null)


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D83502/new/

https://reviews.llvm.org/D83502



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D83502: Change behavior with zero-sized static array extents

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman updated this revision to Diff 277120.
aaron.ballman marked an inline comment as done.
aaron.ballman added a comment.

Updated based on review feedback.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D83502/new/

https://reviews.llvm.org/D83502

Files:
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/CodeGen/CGCall.cpp
  clang/lib/Sema/SemaType.cpp
  clang/test/CodeGen/vla.c
  clang/test/Sema/static-array.c


Index: clang/test/Sema/static-array.c
===
--- clang/test/Sema/static-array.c
+++ clang/test/Sema/static-array.c
@@ -1,6 +1,7 @@
-// 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}}
 
@@ -9,7 +10,7 @@
 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}}
Index: clang/test/CodeGen/vla.c
===
--- clang/test/CodeGen/vla.c
+++ clang/test/CodeGen/vla.c
@@ -206,3 +206,7 @@
 // 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)
Index: clang/lib/Sema/SemaType.cpp
===
--- clang/lib/Sema/SemaType.cpp
+++ clang/lib/Sema/SemaType.cpp
@@ -2393,13 +2393,6 @@
  ? 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?
Index: clang/lib/CodeGen/CGCall.cpp
===
--- clang/lib/CodeGen/CGCall.cpp
+++ clang/lib/CodeGen/CGCall.cpp
@@ -2504,9 +2504,11 @@
   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);
   }
Index: clang/include/clang/Basic/DiagnosticSemaKinds.td
===
--- clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -5395,9 +5395,6 @@
   "zero size arrays are an extension">, InGroup;
 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;
 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">;


Index: clang/test/Sema/static-array.c
===
--- clang/test/Sema/static-array.c
+++ clang/test/Sema/static-array.c
@@ -1,6 +1,7 @@
-// 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 

[PATCH] D83502: Change behavior with zero-sized static array extents

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang/lib/CodeGen/CGCall.cpp:2515
+} else {
+  AI->addAttr(llvm::Attribute::NonNull);
+}

rjmccall wrote:
> aaron.ballman wrote:
> > rjmccall wrote:
> > > 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.
> > 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?
> 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.
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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D83502/new/

https://reviews.llvm.org/D83502



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D83502: Change behavior with zero-sized static array extents

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.



Comment at: clang/lib/CodeGen/CGCall.cpp:2515
+} else {
+  AI->addAttr(llvm::Attribute::NonNull);
+}

aaron.ballman wrote:
> rjmccall wrote:
> > 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.
> 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?
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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D83502/new/

https://reviews.llvm.org/D83502



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D83502: Change behavior with zero-sized static array extents

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman marked an inline comment as done.
aaron.ballman added inline comments.



Comment at: clang/lib/CodeGen/CGCall.cpp:2515
+} else {
+  AI->addAttr(llvm::Attribute::NonNull);
+}

rjmccall wrote:
> 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.
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?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D83502/new/

https://reviews.llvm.org/D83502



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D83502: Change behavior with zero-sized static array extents

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.



Comment at: clang/lib/CodeGen/CGCall.cpp:2515
+} else {
+  AI->addAttr(llvm::Attribute::NonNull);
+}

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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D83502/new/

https://reviews.llvm.org/D83502



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D83502: Change behavior with zero-sized static array extents

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman created this revision.
aaron.ballman added reviewers: rsmith, echristo, dblaikie, rjmccall.

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(+1, 0); //ok
  my_bzero(t+k,n-k); //ok, pattern from actual code


https://reviews.llvm.org/D83502

Files:
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/CodeGen/CGCall.cpp
  clang/lib/Sema/SemaType.cpp
  clang/test/CodeGen/vla.c
  clang/test/Sema/static-array.c


Index: clang/test/Sema/static-array.c
===
--- clang/test/Sema/static-array.c
+++ clang/test/Sema/static-array.c
@@ -1,6 +1,7 @@
-// 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}}
 
@@ -9,7 +10,7 @@
 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}}
Index: clang/test/CodeGen/vla.c
===
--- clang/test/CodeGen/vla.c
+++ clang/test/CodeGen/vla.c
@@ -206,3 +206,6 @@
 // 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]) {}
+// CHECK: define void @test10(i32* nonnull %a)
Index: clang/lib/Sema/SemaType.cpp
===
--- clang/lib/Sema/SemaType.cpp
+++ clang/lib/Sema/SemaType.cpp
@@ -2393,13 +2393,6 @@
  ? 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?
Index: clang/lib/CodeGen/CGCall.cpp
===
--- clang/lib/CodeGen/CGCall.cpp
+++ clang/lib/CodeGen/CGCall.cpp
@@ -2500,12 +2500,20 @@
 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);
-AI->addAttrs(Attrs);
+  if (!ETy->isIncompleteType() && ETy->isConstantSizeType()) {
+// If the type is complete and we know the array size is non-
+// zero, then we know the dereferencable range of memory.
+// Otherwise, if the array size is zero, we only know that the
+// memory must be nonnull.
+if (ArrSize) {
+  llvm::AttrBuilder Attrs;
+  Attrs.addDereferenceableAttr(
+  getContext().getTypeSizeInChars(ETy).getQuantity() *
+  ArrSize);
+  AI->addAttrs(Attrs);
+} else {
+