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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
