Skip to content

8306579: Consider building with /Zc:throwingNew #22039

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
TheShermanTanker wants to merge 3 commits into from
Closed

8306579: Consider building with /Zc:throwingNew #22039

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
72fe25c
8306579
TheShermanTanker Nov 12, 2024
b857c18
-Zc:throwingNew in flags-cflags.m4
TheShermanTanker Nov 12, 2024
248eacc
Merge branch 'master' into patch-15
TheShermanTanker Nov 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions make/autoconf/flags-cflags.m4
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -572,8 +572,8 @@ AC_DEFUN([FLAGS_SETUP_CFLAGS_HELPER],
elif test "x$TOOLCHAIN_TYPE" = xmicrosoft; then
# The -utf-8 option sets source and execution character sets to UTF-8 to enable correct
# compilation of all source files regardless of the active code page on Windows.
TOOLCHAIN_CFLAGS_JVM="-nologo -MD -Zc:preprocessor -Zc:inline -permissive- -utf-8 -MP"
TOOLCHAIN_CFLAGS_JDK="-nologo -MD -Zc:preprocessor -Zc:inline -permissive- -utf-8 -Zc:wchar_t-"
TOOLCHAIN_CFLAGS_JVM="-nologo -MD -Zc:preprocessor -Zc:inline -Zc:throwingNew -permissive- -utf-8 -MP"
TOOLCHAIN_CFLAGS_JDK="-nologo -MD -Zc:preprocessor -Zc:inline -Zc:throwingNew -permissive- -utf-8 -Zc:wchar_t-"
fi

# CFLAGS C language level for JDK sources (hotspot only uses C++)
Expand Down
11 changes: 0 additions & 11 deletions src/java.desktop/windows/native/libawt/windows/awt_new.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,17 +114,6 @@ void *safe_Realloc(void *memblock, size_t size) {
return ptr;
}

#if !defined(DEBUG)
// This function exists because VC++ 5.0 currently does not conform to the
// Standard C++ specification which requires that operator new throw
// std::bad_alloc in an out of memory situation. Instead, VC++ 5.0 returns 0.
//
// This function can be safely removed when the problem is corrected.
void * operator new(size_t size) {
return safe_Malloc(size);
}
#endif

Copy link
Contributor

@prrace prrace Nov 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

@kimbarrett kimbarrett Nov 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

@kimbarrett kimbarrett Nov 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

// This function is called at the beginning of an entry point.
// Entry points are functions which are declared:
// 1. CALLBACK,
Expand Down