On Sun, 17 Nov 2024 06:13:46 GMT, Phil Race <p...@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
