[PATCH] D95095: [BuildLibcalls, Attrs] Support more variants of C++'s new, add attributes for C++'s delete

[Richard Smith - zygoloid via Phabricator via llvm-commits]

rsmith added a comment.

This change has introduced a miscompile on code such as:

  int num_allocs;
  bool f() {
    if (num_allocs == 1) return true;
    ::operator delete(::operator new(0));
    if (num_allocs == 1) return true;
    return false;
  }

... by eliding the second branch due to the incorrect `inaccessiblememonly` attribute. There are two different bugs here: (1) the calls to `operator new` and `operator delete` in this case are `nobuiltin` calls, so you cannot assume you know anything special about them, and (2) even for `delete new int;` (which results in `builtin` calls) I don't see how you can infer that the calls are `inaccessiblememonly` -- I don't see anything in the standard that prevents a replacement global `new` or `delete` from accessing memory that's visible to the caller, and likewise a custom `new_handler` could do so.

`willreturn` is also wrong, even for a `builtin` call; there's no guarantee that a custom new handler will terminate. And `noalias` on the return value should be controlled by the frontend's `-fassume-sane-operator-new` flag, not by LLVM.

I think some of the attributes here are right, but enough of this is inappropriate that I'm going to revert.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95095