[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 <https://reviews.llvm.org/rG006c49d890da633d1ce502117fc2a49863cd65b7>



================
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