8253899: Make IsClassUnloadingEnabled signature match specification

[VladimirKempik]

Please review this change for hotspot and one test.
There is few JVMTI callback/event functions in jdk which signature doesn't match specification.
for example:
static jvmtiError JNICALL IsClassUnloadingEnabled(const jvmtiEnv* env, jboolean* enabled, ...)
but according to jvmti specs it should be:
static jvmtiError JNICALL IsClassUnloadingEnabled(const jvmtiEnv* env, ...)
same with ClassUnload(jvmtiEnv* jvmti_env, JNIEnv* jni_env, const char* name, ...) in tests
for many years that didn't matter but with coming JEP-391 it becomes important to make it match the spec
https://developer.apple.com/documentation/apple_silicon/addressing_architectural_differences_in_your_macos_code
This commit makes the above mentioned functions to have signature matching jvmti specification

---

Progress

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

Testing

	Linux x64	Windows x64	macOS x64
Build	✔️ (3/3 passed)	✔️ (2/2 passed)	✔️ (2/2 passed)
Test (tier1)	⏳ (2/9 running)	⏳ (7/9 running)	⏳ (1/9 running)

Issue

• JDK-8253899: Make IsClassUnloadingEnabled signature match specification

Reviewers

• Serguei Spitsyn (@sspitsyn - Reviewer) ⚠️ Review applies to 1ef832d
• David Holmes (@dholmes-ora - Reviewer)

Download

$ git fetch https://git.openjdk.java.net/jdk pull/466/head:pull/466
$ git checkout pull/466

[bridgekeeper]

👋 Welcome back vkempik! 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]

@VladimirKempik The following labels will be automatically applied to this pull request:

• hotspot
• serviceability

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.

[mlbridge]

Webrevs

• 01: Full - Incremental (cd7c4d5)
• 00: Full (1ef832d)

[mlbridge]

Mailing list message from David Holmes on hotspot-dev:

Hi Vladimir,

On 2/10/2020 1:09 am, Vladimir Kempik wrote:

Please review this change for hotspot and one test.
There is few JVMTI callback/event functions in jdk which signature doesn't match specification.
for example:
static jvmtiError JNICALL IsClassUnloadingEnabled(const jvmtiEnv* env, jboolean* enabled, ...)
but according to jvmti specs it should be:
static jvmtiError JNICALL IsClassUnloadingEnabled(const jvmtiEnv* env, ...)
same with ClassUnload(jvmtiEnv* jvmti_env, JNIEnv* jni_env, const char* name, ...) in tests

Sorry I'm missing something - where in the specification is this? This
is an extension event and I don't see it documented.

Thanks,
David

[sspitsyn]

David,
I think, Vladimir is referring to the JVMTI extension mechanism spec:
https://docs.oracle.com/en/java/javase/15/docs/specs/jvmti.html#jvmtiExtensionFunction
https://docs.oracle.com/en/java/javase/15/docs/specs/jvmti.html#jvmtiExtensionEvent

[sspitsyn]

Vladimir, it looks good to me.

[openjdk]

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

8253899: Make IsClassUnloadingEnabled signature match specification

Reviewed-by: sspitsyn, dholmes

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

• aad3cf4: 8254234: Add test library stream object builder
• 4184959: 8252374: Add a new factory method to concatenate a sequence of BodyPublisher instances into a single publisher.
• 05459df: 8253765: C2: Control randomization in StressLCM and StressGCM
• 6620b61: 8254573: Shenandoah: Streamline/inline native-LRB entry point
• a6c23b7: 8253923: C2 doesn't always run loop opts for compilations that include loops
• dfe8ba6: 8254320: Shenandoah: C2 native LRB should activate for non-cset objects
• 295a44a: 8254558: Remove unimplemented Arguments::do_pd_flag_adjustments
• 0fab73e: 8254560: Shenandoah: Concurrent Strong Roots logging is incorrect
• 638f910: 8254559: Remove unimplemented JVMFlag::get_locked_message_ext
• 0ec1d63: 8253117: Replace HTML tables in javadoc summaries with CSS grid elements
• ... and 129 more: https://git.openjdk.java.net/jdk/compare/44e6820c377dd6f9a11d66957ccd0e71fa9be2fb...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.

[VladimirKempik]

David,
I think, Vladimir is referring to the JVMTI extension mechanism spec:
https://docs.oracle.com/en/java/javase/15/docs/specs/jvmti.html#jvmtiExtensionFunction
https://docs.oracle.com/en/java/javase/15/docs/specs/jvmti.html#jvmtiExtensionEvent

Hello Serguei, you are right, I was talking about this documents.
Thank you.

[dholmes-ora]

Okay but look at the example that documentation gives:

For example, if the jvmtiParamInfo returned by GetExtensionEvents indicates that there is a jint parameter, the event handler should be declared:

    void JNICALL myHandler(jvmtiEnv* jvmti_env, jint myInt, ...)

The myInt is explicit, just as our "jboolean* enabled" is explicit. I think they key point is that the signature must end with "..." which it does.

I don't see anything here that needs to be fixed.

[VladimirKempik]

Okay but look at the example that documentation gives:

For example, if the jvmtiParamInfo returned by GetExtensionEvents indicates that there is a jint parameter, the event handler should be declared:

    void JNICALL myHandler(jvmtiEnv* jvmti_env, jint myInt, ...)

The myInt is explicit, just as our "jboolean* enabled" is explicit. I think they key point is that the signature must end with "..." which it does.

I don't see anything here that needs to be fixed.

Hello David. On majority of platforms this would be fine.

But on some platforms, variadic arguments and non variadic arguments are passed differently ( for example on macos-aarch64, variadic args are passed always on stack, non variadic on registers (and on stack for 9th+ arg) , that causes issues.

If you still see no issues here we can delay and make this changeset part of JEP-391.
But since this changeset isn't much macos-aarch64 specific, I thought it would be good to integrate it separately from jep-391.

Regards, Vladimir

[mlbridge]

Mailing list message from David Holmes on hotspot-dev:

Hi Vladimir,

On 2/10/2020 5:37 pm, Vladimir Kempik wrote:

On Fri, 2 Oct 2020 07:27:17 GMT, David Holmes <dholmes at openjdk.org> wrote:

Okay but look at the example that documentation gives:

For example, if the jvmtiParamInfo returned by GetExtensionEvents indicates that there is a jint parameter, the event
handler should be declared: ```
void JNICALL myHandler(jvmtiEnv* jvmti_env, jint myInt, ...)
```

The myInt is explicit, just as our "jboolean* enabled" is explicit. I think they key point is that the signature must
end with "..." which it does.
I don't see anything here that needs to be fixed.

Hello David. On majority of platforms this would be fine.

But on some platforms, variadic arguments and non variadic arguments are passed differently ( for example on
macos-aarch64, variadic args are passed always on stack, non variadic on registers (and on stack for 9th+ arg) , that
causes issues.

Okay - I see the potential for a problme here but ...

If you still see no issues here we can delay and make this changeset part of JEP-391.
But since this changeset isn't much macos-aarch64 specific, I thought it would be good to integrate it separately from
jep-391.

... this change actually goes against the example in the spec, so if you
make this change it indicates the spec needs to be updated too.

Cheers,
David
-----

[VladimirKempik]

Mailing list message from David Holmes on hotspot-dev:

Hi Vladimir,

On 2/10/2020 5:37 pm, Vladimir Kempik wrote:

On Fri, 2 Oct 2020 07:27:17 GMT, David Holmes wrote:

Okay but look at the example that documentation gives:

For example, if the jvmtiParamInfo returned by GetExtensionEvents indicates that there is a jint parameter, the event
handler should be declared: ```
void JNICALL myHandler(jvmtiEnv* jvmti_env, jint myInt, ...)

The myInt is explicit, just as our "jboolean* enabled" is explicit. I think they key point is that the signature must
end with "..." which it does.
I don't see anything here that needs to be fixed.

Hello David. On majority of platforms this would be fine.
But on some platforms, variadic arguments and non variadic arguments are passed differently ( for example on
macos-aarch64, variadic args are passed always on stack, non variadic on registers (and on stack for 9th+ arg) , that
causes issues.

Okay - I see the potential for a problme here but ...

If you still see no issues here we can delay and make this changeset part of JEP-391.
But since this changeset isn't much macos-aarch64 specific, I thought it would be good to integrate it separately from
jep-391.

... this change actually goes against the example in the spec, so if you
make this change it indicates the spec needs to be updated too.

Cheers,
David

Hello David

I really believe the problem is in document here ( in examples)
first, the doc clearly specify the type

typedef jvmtiError (JNICALL jvmtiExtensionFunction)
(jvmtiEnv jvmti_env,
...);

then in examples it declares the function not matching this spec.

Is it a good idea to update the docs in a separate bug ?

Thanks, Vladimir

[mlbridge]

Mailing list message from David Holmes on hotspot-dev:

Hi Vladimir,

On 3/10/2020 1:29 am, Vladimir Kempik wrote:

On Fri, 2 Oct 2020 07:34:45 GMT, Vladimir Kempik <vkempik at openjdk.org> wrote:

If you still see no issues here we can delay and make this changeset part of JEP-391.
But since this changeset isn't much macos-aarch64 specific, I thought it would be good to integrate it separately from
jep-391.

... this change actually goes against the example in the spec, so if you
make this change it indicates the spec needs to be updated too.

Cheers,
David
-----

Hello David

I really believe the problem is in document here ( in examples)
first, the doc clearly specify the type

typedef jvmtiError (JNICALL *jvmtiExtensionFunction)
(jvmtiEnv* jvmti_env,
...);

then in examples it declares the function not matching this spec.

Is it a good idea to update the docs in a separate bug ?

I don't think it really matters one way or the other as long as any new
bug is promptly fixed. At the moment the code matches the example in the
spec but they are both "wrong". I'd like to see them both "right" asap.

Thanks,
David

[VladimirKempik]

Hello David
I have created CSR draft
https://bugs.openjdk.java.net/browse/JDK-8254014

Regards, Vladimir

[dholmes-ora]

/csr needed

[openjdk]

@dholmes-ora this pull request will not be integrated until the CSR request JDK-8254014 for issue JDK-8253899 has been approved.

[mlbridge]

Mailing list message from David Holmes on hotspot-dev:

On 5/10/2020 10:47 pm, Vladimir Kempik wrote:

On Fri, 2 Oct 2020 15:26:30 GMT, Vladimir Kempik <vkempik at openjdk.org> wrote:

Okay but look at the example that documentation gives:

For example, if the jvmtiParamInfo returned by GetExtensionEvents indicates that there is a jint parameter, the event
handler should be declared: ```
void JNICALL myHandler(jvmtiEnv* jvmti_env, jint myInt, ...)
```

The myInt is explicit, just as our "jboolean* enabled" is explicit. I think they key point is that the signature must
end with "..." which it does.
I don't see anything here that needs to be fixed.

Hello David. On majority of platforms this would be fine.

But on some platforms, variadic arguments and non variadic arguments are passed differently ( for example on
macos-aarch64, variadic args are passed always on stack, non variadic on registers (and on stack for 9th+ arg) , that
causes issues. If you still see no issues here we can delay and make this changeset part of JEP-391.
But since this changeset isn't much macos-aarch64 specific, I thought it would be good to integrate it separately from
jep-391.
Regards, Vladimir

_Mailing list message from [David Holmes](mailto:david.holmes at oracle.com) on
[hotspot-dev](mailto:hotspot-dev at openjdk.java.net):_
Hi Vladimir,

On 2/10/2020 5:37 pm, Vladimir Kempik wrote:

On Fri, 2 Oct 2020 07:27:17 GMT, David Holmes <dholmes at openjdk.org> wrote:

Okay but look at the example that documentation gives:

For example, if the jvmtiParamInfo returned by GetExtensionEvents indicates that there is a jint parameter, the event
handler should be declared: ```
void JNICALL myHandler(jvmtiEnv* jvmti_env, jint myInt, ...)
```

The myInt is explicit, just as our "jboolean* enabled" is explicit. I think they key point is that the signature must
end with "..." which it does.
I don't see anything here that needs to be fixed.

Hello David. On majority of platforms this would be fine.
But on some platforms, variadic arguments and non variadic arguments are passed differently ( for example on
macos-aarch64, variadic args are passed always on stack, non variadic on registers (and on stack for 9th+ arg) , that
causes issues.

Okay - I see the potential for a problme here but ...

If you still see no issues here we can delay and make this changeset part of JEP-391.
But since this changeset isn't much macos-aarch64 specific, I thought it would be good to integrate it separately from
jep-391.

... this change actually goes against the example in the spec, so if you
make this change it indicates the spec needs to be updated too.

Cheers,
David
-----

Hello David

I really believe the problem is in document here ( in examples)
first, the doc clearly specify the type

typedef jvmtiError (JNICALL *jvmtiExtensionFunction)
(jvmtiEnv* jvmti_env,
...);

then in examples it declares the function not matching this spec.

Is it a good idea to update the docs in a separate bug ?

Thanks, Vladimir

Hello David
I have created CSR draft
https://bugs.openjdk.java.net/browse/JDK-8254014

Thanks. I have updated and reviewed the CSR request.

David

[VladimirKempik]

Hello
I have updated the PR with changes from spec of CSR

Regards, Vladimir

[dholmes-ora]

Looks good.
Thanks,
David

[VladimirKempik]

/integrate

[openjdk]

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

• aad3cf4: 8254234: Add test library stream object builder
• 4184959: 8252374: Add a new factory method to concatenate a sequence of BodyPublisher instances into a single publisher.
• 05459df: 8253765: C2: Control randomization in StressLCM and StressGCM
• 6620b61: 8254573: Shenandoah: Streamline/inline native-LRB entry point
• a6c23b7: 8253923: C2 doesn't always run loop opts for compilations that include loops
• dfe8ba6: 8254320: Shenandoah: C2 native LRB should activate for non-cset objects
• 295a44a: 8254558: Remove unimplemented Arguments::do_pd_flag_adjustments
• 0fab73e: 8254560: Shenandoah: Concurrent Strong Roots logging is incorrect
• 638f910: 8254559: Remove unimplemented JVMFlag::get_locked_message_ext
• 0ec1d63: 8253117: Replace HTML tables in javadoc summaries with CSS grid elements
• ... and 129 more: https://git.openjdk.java.net/jdk/compare/44e6820c377dd6f9a11d66957ccd0e71fa9be2fb...master

Your commit was automatically rebased without conflicts.

Pushed as commit c7f0064.

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