RFR: 8306579: Consider building with /Zc:throwingNew

[Kim Barrett]

On Sun, 17 Nov 2024 09:22:40 GMT, Kim Barrett <kbarrett at openjdk.org> wrote:

>> 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.

There also isn't a corresponding `operator new[]`. Because of that, the
various places that are allocating arrays are already using the default array
allocation function, e.g. the C++ allocator, rather than directly using
malloc.  That also argues for the removal proposed here.

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

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