8262092: vmTestbase/nsk/jvmti/scenarios/hotswap/HS102/hs102t001/TestDescription.java SIGSEGV in memmove_ssse3 #3889

alexmenkov · 2021-05-05T20:26:13Z

Class loading can happen on different threads, but HotSwap agent is not ready to this - classCount variable is used without any synchronization.
The fix adds synchronization for ClassFileLoadHook.

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed

Issue

JDK-8262092: vmTestbase/nsk/jvmti/scenarios/hotswap/HS102/hs102t001/TestDescription.java SIGSEGV in memmove_ssse3

Reviewers

Leonid Mesnik (@lmesnik - Committer)
Serguei Spitsyn (@sspitsyn - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/3889/head:pull/3889
$ git checkout pull/3889

Update a local copy of the PR:
$ git checkout pull/3889
$ git pull https://git.openjdk.java.net/jdk pull/3889/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 3889

View PR using the GUI difftool:
$ git pr show -t 3889

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/3889.diff

bridgekeeper · 2021-05-05T20:26:59Z

👋 Welcome back amenkov! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2021-05-05T20:29:17Z

@alexmenkov The following label will be automatically applied to this pull request:

serviceability

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.

mlbridge · 2021-05-05T20:33:05Z

Webrevs

01: Full - Incremental (6606d89b)
00: Full (63e85bc3)

lmesnik

Also, generic comment. There are other counters which might require synchronization, like CompiledMethodLoadEventsCount. Doesn't it makes sense to fix them also or file a new bug?

lmesnik · 2021-05-05T20:59:01Z

test/hotspot/jtreg/vmTestbase/nsk/share/jvmti/hotswap/HotSwap.cpp

@@ -67,6 +67,8 @@ static char** names = NULL;
 static jvmtiClassDefinition* old_class_def = NULL;
 static jvmtiClassDefinition* new_class_def = NULL;
 static int classCount = 0;
+/* lock to access classCount */
+static jrawMonitorID classLoadLock = NULL;


I think it would simpler to use an atomic counter rather than monitors for this fix.

Do you mean use atomic from STL?
I'm not sure it would be simpler as we need logic like "get the value, if the value is less than max_classes, increment it"

lmesnik · 2021-05-05T21:01:11Z

test/hotspot/jtreg/vmTestbase/nsk/share/jvmti/hotswap/HotSwap.cpp

-        if (!NSK_JVMTI_VERIFY(jvmti_env->Allocate(class_data_len, (unsigned char**)
-                &old_class_def[classCount].class_bytes))) {
+        // use while instead of if to exit the block on error
+        while (classCount < max_classes) {


Could you please explain why this while is executed only once. It is not obvious.

I tried to explain it in the comment :)
it should be "if (classCount < max_classes)",
but inside the if we have return statements and we need to do cleanup (exit monitor).
So I used well-known pattern with 1-pass cycle (note that "return"s are replaced with "break"s)

Offline discussion shown that I forgot to add break at the end of the cycle. Will fix

alexmenkov · 2021-05-05T22:00:33Z

Also, generic comment. There are other counters which might require synchronization, like CompiledMethodLoadEventsCount. Doesn't it makes sense to fix them also or file a new bug?

The problem with classCount is it's used as an index in the arrays.
CompiledMethodLoadEventsCount is only incremented (but formally it should be synchronized too).
And looks like CompiledMethodLoadEventsCount is actually not used (I mean the values is not checked)

sspitsyn

This looks good.
Thanks,
Serguei

openjdk · 2021-05-07T00:24:18Z

@alexmenkov 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:

8262092: vmTestbase/nsk/jvmti/scenarios/hotswap/HS102/hs102t001/TestDescription.java SIGSEGV in memmove_ssse3

Reviewed-by: lmesnik, sspitsyn

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 23 new commits pushed to the master branch:

36e5ad6: 8263236: runtime/os/TestTracePageSizes.java fails on old kernels
0ca86da: 8266002: vmTestbase/nsk/jvmti/ClassPrepare/classprep001 should skip events for unexpected classes
52f1db6: 8262002: java/lang/instrument/VerifyLocalVariableTableOnRetransformTest.sh failed with "TestCaseScaffoldException: DummyClassWithLVT did not match .class file"
04f7112: 8266293: Key protection using PBEWithMD5AndDES fails with "java.security.InvalidAlgorithmParameterException: Salt must be 8 bytes long"
a90b33a: 8266573: Make sure blackholes are tagged for all JVMCI paths
2dcbedf: 8266044: Nested class summary should show kind of class or interface
e840597: 8266460: java.io tests fail on null stream with upgraded jtreg/TestNG
fcedfc8: 8266579: Update test/jdk/java/lang/ProcessHandle/PermissionTest.java & test/jdk/java/sql/testng/util/TestPolicy.java
c665dba: 8266561: Remove Compile::_save_argument_registers
47d4438: 8266426: ZHeapIteratorOopClosure does not handle native access properly
... and 13 more: https://git.openjdk.java.net/jdk/compare/6018336fc5e93675482b92df76594712c238adda...master

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 master branch, type /integrate in a new comment.

alexmenkov · 2021-05-10T22:46:32Z

/integrate

openjdk · 2021-05-10T22:48:29Z

@alexmenkov Since your change was applied there have been 65 commits pushed to the master branch:

23446f1: 8265128: [REDO] Optimize Vector API slice and unslice operations
e5d3ee3: 8266802: Shenandoah: Round up region size to page size unconditionally
8851cb6: 8266779: Use instead of ZERO_WIDTH_SPACE
0cc7833: 8265208: [JEP-356] : SplittableRandom and SplittableGenerators - splits() methods does not throw NullPointerException when source is null
f78440a: 8266440: Shenandoah: TestReferenceShortcutCycle.java test failed on AArch64
de78431: 8241502: C2: Migrate x86_64.ad to MacroAssembler
c8b7447: 8266603: jpackage: Add missing copyright file in Java runtime .deb installers
c494efc: 8266774: System property values for stdout/err on Windows UTF-8
25d99e5: 8266330: itableMethodEntry::initialize() asserts with archived old classes
5d761fc: 8266796: Clean up the unnecessary code in the method UnsharedNameTable#fromUtf
... and 55 more: https://git.openjdk.java.net/jdk/compare/6018336fc5e93675482b92df76594712c238adda...master

Your commit was automatically rebased without conflicts.

Pushed as commit 9713152.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

Added synchronization for ClassFileLoadHook callback

63e85bc

openjdk bot added the rfr Pull request is ready for review label May 5, 2021

openjdk bot added the serviceability serviceability-dev@openjdk.org label May 5, 2021

lmesnik suggested changes May 5, 2021

View reviewed changes

added missed break; made all counters atomic

6606d89

lmesnik approved these changes May 6, 2021

View reviewed changes

sspitsyn approved these changes May 7, 2021

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label May 7, 2021

openjdk bot closed this May 10, 2021

openjdk bot added integrated Pull request has been integrated and removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels May 10, 2021

alexmenkov deleted the HotSwapSync branch May 10, 2021 22:49

8262092: vmTestbase/nsk/jvmti/scenarios/hotswap/HS102/hs102t001/TestDescription.java SIGSEGV in memmove_ssse3 #3889

8262092: vmTestbase/nsk/jvmti/scenarios/hotswap/HS102/hs102t001/TestDescription.java SIGSEGV in memmove_ssse3 #3889

alexmenkov commented May 5, 2021 •

edited by openjdk bot

bridgekeeper bot commented May 5, 2021

openjdk bot commented May 5, 2021

mlbridge bot commented May 5, 2021 •

edited

lmesnik left a comment

lmesnik May 5, 2021

alexmenkov May 5, 2021 •

edited

lmesnik May 5, 2021

alexmenkov May 5, 2021

alexmenkov May 5, 2021

alexmenkov commented May 5, 2021 •

edited

sspitsyn left a comment

openjdk bot commented May 7, 2021

alexmenkov commented May 10, 2021

openjdk bot commented May 10, 2021

8262092: vmTestbase/nsk/jvmti/scenarios/hotswap/HS102/hs102t001/TestDescription.java SIGSEGV in memmove_ssse3 #3889

8262092: vmTestbase/nsk/jvmti/scenarios/hotswap/HS102/hs102t001/TestDescription.java SIGSEGV in memmove_ssse3 #3889

Conversation

alexmenkov commented May 5, 2021 • edited by openjdk bot

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented May 5, 2021

openjdk bot commented May 5, 2021

mlbridge bot commented May 5, 2021 • edited

Webrevs

lmesnik left a comment

Choose a reason for hiding this comment

lmesnik May 5, 2021

Choose a reason for hiding this comment

alexmenkov May 5, 2021 • edited

Choose a reason for hiding this comment

lmesnik May 5, 2021

Choose a reason for hiding this comment

alexmenkov May 5, 2021

Choose a reason for hiding this comment

alexmenkov May 5, 2021

Choose a reason for hiding this comment

alexmenkov commented May 5, 2021 • edited

sspitsyn left a comment

Choose a reason for hiding this comment

openjdk bot commented May 7, 2021

alexmenkov commented May 10, 2021

openjdk bot commented May 10, 2021

alexmenkov commented May 5, 2021 •

edited by openjdk bot

mlbridge bot commented May 5, 2021 •

edited

alexmenkov May 5, 2021 •

edited

alexmenkov commented May 5, 2021 •

edited