[PATCH] D73380: [clang] Annotating C++'s `operator new` with more attributes

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri marked 2 inline comments as done.
lebedev.ri added a comment.

In D73380#1839603 <https://reviews.llvm.org/D73380#1839603>, @xbolva00 wrote:

> LLVM already infers noalias nonnull for eg. _Znwm so noalias and nonnull info added by clang will not increase power of LLVM. Or?


To be honest, i don't really believe it is LLVM place to deduce attributes on standard library functions like these, frontend should be doing that.
Because as noted in patch description, if `noalias` is blindly "deduced" by LLVM,
that is a miscompile if `-fno-assume-sane-operator-new` was specificed.

> Alignment info is useful I think.

That is the main motivation, yes.



================
Comment at: clang/test/CodeGenCXX/builtin-operator-new-delete.cpp:54
 }
-// CHECK: declare noalias i8* @_ZnwmSt11align_val_t(i64, i64) [[ATTR_NOBUILTIN:#[^ ]*]]
+// CHECK: declare nonnull i8* @_ZnwmSt11align_val_t(i64, i64) [[ATTR_NOBUILTIN:#[^ ]*]]
 // CHECK: declare void @_ZdlPvSt11align_val_t(i8*, i64) [[ATTR_NOBUILTIN_NOUNWIND:#[^ ]*]]
----------------
xbolva00 wrote:
> We lost noalias here?
See two lines above, it migrated to the callsite.

I'm going to argue that the previous behavior was erroneous:
despite what `-fno-assume-sane-operator-new` is supposed to be doing,
i **suspect** it will magically break with LTO.
(we'd have `declare noalias nonnull i8* @_ZnwmSt11align_val_t(i64, i64)` in one TU
and `declare nonnull i8* @_ZnwmSt11align_val_t(i64, i64)` in another - one with `noalias`
and one without - how would they be merged? by dropping `noalias`, or keeping it?)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73380