8306579: Consider building with /Zc:throwingNew #22039
Conversation
|
👋 Welcome back jwaters! A progress list of the required criteria for merging this PR into |
|
@TheShermanTanker This change now passes all automated pre-integration checks. ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details. After integration, the commit message for the final commit will be: You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been 570 new commits pushed to the
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details. ➡️ To integrate this PR with the above commit message to the |
|
@TheShermanTanker The following labels will be automatically applied to this pull request:
When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command. |
openjdk
bot
added
build
|
I don't plan on integrating this yet. My intention is to let it run in Actions while I also test it locally. This is also dependent on some cleanup that has to be done in ADLC, where an operator new is currently causing issues with experimental enabling of C++17 |
| return safe_Malloc(size); | ||
| } | ||
| #endif | ||
|
|
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 was a problem hiding this comment.
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.
|
@TheShermanTanker this pull request can not be integrated into git checkout patch-15
git fetch https://git.openjdk.org/jdk.git master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push |
openjdk
bot
added
the
merge-conflict
openjdk
bot
removed
the
merge-conflict
|
One thing I have a query on: Does this need the problem with the ADLC operator new to be fixed before it can be integrated? |
For those following along, the bug being referred to is this: I think that's not a blocker for this change. With this change, |
Ah, alright. For exceptions ADLC is a strange case. ADLC has no exceptions on Linux, but does on every other platform. I couldn't tell you why this is the case EDIT: Nevermind, I think I misunderstood "I don't think there's any exception handling in adlc", my bad |
Right, there's no uses of try/catch or anything related. I thought adlc had exceptions enabled, but you say not on linux, only on But I'm pretty sure it doesn't matter, because I think that function |
magicus
left a comment
magicus
left a comment
There was a problem hiding this comment.
Build changes look fine. I also believe the AWT change is sound, based on Kim's reasoning.
|
@TheShermanTanker This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration! |
|
Keep open please |
|
@TheShermanTanker What is holding you back from integrating? I think you are good to go at this point. |
|
Sorry, was bogged down on my end. I'm free enough to integrate this at the moment, so I'll do that |
|
/integrate |
|
Going to push as commit a289bcf.
Your commit was automatically rebased without conflicts. |
openjdk
bot
removed
ready
|
@TheShermanTanker Pushed as commit a289bcf. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
4 participants
JDK-8305590 removed
-fcheck-newwhen 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, 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.
Progress
Issue
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/22039/head:pull/22039$ git checkout pull/22039Update a local copy of the PR:
$ git checkout pull/22039$ git pull https://git.openjdk.org/jdk.git pull/22039/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 22039View PR using the GUI difftool:
$ git pr show -t 22039Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/22039.diff
Using Webrev
Link to Webrev Comment