RFR: 8306579: Consider building with /Zc:throwingNew

[Kim Barrett]

On Sun, 17 Nov 2024 06:13:46 GMT, Phil Race <prr at openjdk.org> wrote:

>> [JDK-8305590](https://bugs.openjdk.org/browse/JDK-8305590) removed `-fcheck-new` when building with gcc. It turns out Visual Studio has a similar option, though inverted in behavior and default.
>> 
>> It seems like /Zc:throwingNew- (the default) corresponds to gcc -fcheck-new, and /Zc:throwingNew corresponds to -fno-check-new (the default).
>> 
>> The Visual Studio documentation strongly recommends using /Zc:throwingNew if possible, as turning it off (the default) seriously bloats code and inhibits optimizations.
>> https://learn.microsoft.com/en-us/cpp/build/reference/zc-throwingnew-assume-operator-new-throws?view=msvc-170
>> 
>> As mentioned in [JDK-8305590](https://bugs.openjdk.org/browse/JDK-8305590), the standard says that an allocation function can report allocation failure either by returning null (when it must have a nothrow exception specification), or by throwing `std::bad_alloc` (so obviously must not be declared as non-throwing). HotSpot allocation functions terminate the program instead of throwing on allocation failure, so similarly don't need the result checked for null.
>> 
>> The documentation for /Zc:throwingNew is somewhat vague and confusing, so some investigation is probably needed to verify it really has the desired effect for us.
>
> src/java.desktop/windows/native/libawt/windows/awt_new.cpp line 127:
> 
>> 125: }
>> 126: #endif
>> 127: 
> 
> I don't see you commenting on this which is a "huge" deal as it seems like it changes memory allocation for a lot of the AWT Windows code.
> This needs careful and analysis and explanation - from you - so reviewers can ponder it.
> Also you need to run a lot of tests to verify it.

This is a "replacement function" for global `operator new`. As the comment
says, it exists to provide the semantics specified by the standard.
Specifically, throwing std::bad_alloc when the allocation fails, because old
versions of the MSVC-provided default implementation didn't do that. That's no
longer true; the default implementation has thrown on allocation failure for a
long time (at least since VS 2015).

https://learn.microsoft.com/en-us/cpp/cpp/new-and-delete-operators?view=msvc-170
https://learn.microsoft.com/en-us/cpp/cpp/new-and-delete-operators?view=msvc-140

VS documentation discusses replacing that behavior by linking in non-throwing
operator new, but we're not doing that.
https://learn.microsoft.com/en-us/cpp/c-runtime-library/link-options?view=msvc-170
see nothrownew.obj

This was also never a correct implementation, since there isn't a
corresponding replacement for `operator delete`. This implementation just
calls malloc and checks the result. Calling the default `operator delete` on
the result of malloc (or anything else that isn't the result of calling the
default `operator new`) is UB; C++14 3.7.4.2/3.  Presumably it's been working,
but that's presumably by accident of the MSVC implementation.

The effect of removing this definition is that the default `operator new` will
be used instead.  Doing that instead of calling malloc is potentially somewhat
of a change of behavior.  Whether it matters is hard for me to guess.

Either this replacement definiton should be removed, or a corresponding
`operator delete` must be added.

Also, can user code be linked into the application using this? If so, it is
generally wrong for a library to provide a replacement definition; the
application might be providing its own.

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/22039#discussion_r1845346809