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
