8288282: Zero-release build is broken after JDK-8279047 due to UseHeavyMonitors is read-only #9138
Conversation
…vyMonitors is read-only
|
👋 Welcome back jiefu! A progress list of the required criteria for merging this PR into |
|
@DamonFool The following label 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 list. If you would like to change these labels, use the /label pull request command. |
Webrevs
|
|
@DamonFool , sorry I missed this when expiring UseHeavyMonitors in product mode. The Zero code should have been fixed long before now. But why not make keep the diagnostics available in the debug VM? |
Updated. |
There was a problem hiding this comment.
Unfortunately, that would break:
CONF=linux-x86_64-zero-release make exploded-test TEST=runtime/Monitor/SyncOnValueBasedClassTest.java
java.lang.RuntimeException: 'fatal error: Synchronizing on object' missing from stdout/stderr
I think the patch should just be:
diff --git a/src/hotspot/cpu/zero/vm_version_zero.cpp b/src/hotspot/cpu/zero/vm_version_zero.cpp
index 5c2a0a3d5d2..df6c7f0eba2 100644
--- a/src/hotspot/cpu/zero/vm_version_zero.cpp
+++ b/src/hotspot/cpu/zero/vm_version_zero.cpp
@@ -47,7 +47,8 @@ void VM_Version::initialize() {
// If lock diagnostics is needed, always call to runtime
if (DiagnoseSyncOnValueBasedClasses != 0) {
- FLAG_SET_DEFAULT(UseHeavyMonitors, true);
+ warning("Lock diagnostics is not available on this CPU");
+ FLAG_SET_DEFAULT(DiagnoseSyncOnValueBasedClasses, 0);
}
if (UseAESIntrinsics) {
diff --git a/test/hotspot/jtreg/runtime/Monitor/SyncOnValueBasedClassTest.java b/test/hotspot/jtreg/runtime/Monitor/SyncOnValueBasedClassTest.java
index 394a92c0398..1793ba2e0d7 100644
--- a/test/hotspot/jtreg/runtime/Monitor/SyncOnValueBasedClassTest.java
+++ b/test/hotspot/jtreg/runtime/Monitor/SyncOnValueBasedClassTest.java
@@ -31,6 +31,7 @@ import java.util.stream.*;
* @bug 8242263
* @summary Exercise DiagnoseSyncOnValueBasedClasses diagnostic flag
* @requires vm.flagless
+ * @requires vm.flavor != "zero"
* @library /test/lib
* @run driver/timeout=180000 SyncOnValueBasedClassTest
*/
Good catch. I think it's fine to keep the diagnostics for zero debug VM in case someone might use it while debugging. |
Let's not force VM into different option settings in |
So what's the trouble would be? There are many VM flags which are only available for debug VMs. |
Those flags are usually having the same value for release and debug VMs. Your current patch changes the flag values depending on VM mode, which exposes potentially different behaviors depending on VM mode. Imagine chasing a bug in The block your patch is modifying is added by me in JDK-8273486, I say we just redo it without involving |
Okay. |
|
It made more sense to me to keep what was the existing debug behaviour, but I don't have any interest in Zero at all, so whatever you want to do @shipilev is fine by me. |
|
Thanks @dholmes-ora and @shipilev for the review. |
|
@DamonFool 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 3 new commits pushed to the
Please see this link for an up-to-date comparison between the source branch of this pull request and the ➡️ To integrate this PR with the above commit message to the |
|
/integrate |
|
Going to push as commit 4aede33.
Your commit was automatically rebased without conflicts. |
openjdk
bot
removed
ready
|
@DamonFool Pushed as commit 4aede33. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
|
@DamonFool The command |
Hi all,
Zero-release build is broken after JDK-8279047.
After JDK-8279047,
UseHeavyMonitorsbecomes read-only in PRODUCT VMs.But for Zero,
UseHeavyMonitorsneeds to be reset ifDiagnoseSyncOnValueBasedClasses != 0.I never hear that people would DiagnoseSyncOnValueBasedClasses with zero vms.
So in order to expire
UseHeavyMonitorsfor all PRODUCT VMs, I suggest disabling lock diagnostics for zero vms.Thanks.
Best regards,
Jie
Progress
Issue
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk pull/9138/head:pull/9138$ git checkout pull/9138Update a local copy of the PR:
$ git checkout pull/9138$ git pull https://git.openjdk.org/jdk pull/9138/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 9138View PR using the GUI difftool:
$ git pr show -t 9138Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/9138.diff