Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
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.
Reverted in rG1484ad4137b5d627573672bad48b03785f8fdefd; pre-existing incorrect attribute logic for plain operator new removed in rGab243efb261ba7e27f4b14e1a6fbbff15a79c0bf.
I think there are a few attributes we can meaningfully and correctly add to some calls to operator new / operator delete:
- nonnull can be added for all operator new functions that have a throwing exception specification, but only the frontend knows this, so this should be done by Clang.
- noundef can be added to all functions that return a pointer, in C++, but the frontend should do that.
- The nothrow_t variants can be marked does_not_throw, but they're declared noexcept so the frontend will do that itself anyway.
- For builtin calls to operator delete, we might be able to justify adding inaccessiblemem_or_argmemonly and/or nocapture, but I don't think either of those is justified by the standard rules. It's also unclear to me if nocapture is at all meaningful for a pointer that LLVM considers to have been deallocated by the callee. But this can't be done by the mechanism in this patch, as we can only add those attributes on builtin call sites, not to the nobuiltin declaration of the function.
So there might be some changes we can make in the frontend to add more attributes on operator new / operator delete, but it doesn't seem like there are any we can / should add here.