8303166: Disable RTTI for Hotspot when building with MSVC #12743
Conversation
Signed-off-by: Justin King <[email protected]>
Signed-off-by: Justin King <[email protected]>
|
👋 Welcome back jcking! A progress list of the required criteria for merging this PR into |
|
/reviewers 2 |
Webrevs
|
| // JavaThread and NotificationThread. This results in the vtable symbols for both classes pointing | ||
| // to the same address, and Serviceability Agent thinking all JavaThreads are NotificationThreads. So | ||
| // while this method is not directly used anywhere, it must exist. | ||
| bool is_Notification_thread() const override { return true; } |
There was a problem hiding this comment.
Rather than adding this method, would it be sufficient to explicitly define ~NotificationThread()?
Might require making it non-trivial. The comment would still be needed.
|
The workaround should have been done under JDK-8302817. This issue should have simply disabled RTTI in the build system. |
I'll split the RTTI disable to a separate PR Monday, leave this one as the workaround, and rename to the other issue. |
|
@jcking This pull request has been inactive for more than 8 weeks and will be automatically closed if another 8 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! |
|
@jcking this pull request can not be integrated into git checkout msvc-rtti
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
|
@jcking This pull request has been inactive for more than 8 weeks and will be automatically closed if another 8 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! |
|
@jcking This pull request has been inactive for more than 16 weeks and will now be automatically closed. If you would like to continue working on this pull request in the future, feel free to reopen it! This can be done using the |
|
/label serviceability |
|
@plummercj |
|
❗ This change is not yet ready to be integrated. |
openjdk
bot
changed the title
|
Looks like it was closed because it depends on
[JDK-8302817](https://bugs.openjdk.org/browse/JDK-8302817), which was
resolved as "Won't Fix". The reason given there is:
[Alex
Menkov](https://bugs.openjdk.org/secure/ViewProfile.jspa?name=amenkov)
added a comment - [2023-05-18
16:52](https://bugs.openjdk.org/browse/JDK-8302817?focusedId=14582748&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14582748)
Test for unique vtables was integrated, but unfortunately it cannot
protect against null vtable cases (as in some cases it's ok).
I tend to the opinion that reducing the size of jvm.dll is not worth
the risk of breaking SA.
…On 10/30/24 1:21 AM, Julian Waters wrote:
Why was this never gotten around to? Seems like a beneficial change to
me. @jcking
<https://urldefense.com/v3/__https://github.com/jcking__;!!ACWV5N9M2RV99hQ!JbvLoyZy47CtR1EowaSlJWP7GC2kng1zEx83mx3kR53XWBZbwp8BgCr6cTG87TElXHDYOx3xHSYXyPPRkf8Pt7ONPR0$>
would you mind if I took over?
|
5 participants
Disable RTTI
-GR-for Hotspot when building with MSVC. This drops the size ofjvm.dllby roughly 1 MB. Hotspot does not rely on RTTI and it is disabled for both GCC/Clang and Open XL C/C++ already. This change disables it for MSVC, ensuring we do not accidently rely on RTTI for Windows-specific code while also decreasing the resulting binary size.Cheers to @alexmenkov for finding the tricky root cause of why disabling RTTI was causing a subset of serviceability agent tests to fail.
Progress
Warnings
Issue
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/12743/head:pull/12743$ git checkout pull/12743Update a local copy of the PR:
$ git checkout pull/12743$ git pull https://git.openjdk.org/jdk.git pull/12743/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 12743View PR using the GUI difftool:
$ git pr show -t 12743Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/12743.diff
Using Webrev
Link to Webrev Comment