[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