RFR: 8262881: port JVM/DI tests from JDK-4413752 to JVM/TI [v4]

[Serguei Spitsyn]

On Wed, 7 Apr 2021 19:17:33 GMT, Daniel D. Daugherty <dcubed at openjdk.org> wrote:

>> Hi Dan,
>> It looks good in general.
>> But I think moving JVMTI error code checks to Java is a step in a wrong direction.
>> First, it makes it inconsistent with all existing JVMTI tests. In this particular case, all the complexity is moved to Java side making it unbalanced. Also, we are working with Leonid toward creating relevant native testing lib for serviceability/jvmti tests (created it for Loom first) which has a support to print symbolic names (for error codes as well) if necessary.
>> Not sure, I understood what wrong with the native check_jvmti_status(). It seems the id argument is not needed much and can be removed anyway.
>> Thanks,
>> Serguei
>
> Hi @sspitsyn,
> 
> I rather like the much simpler agent code and it puts the majority of logic
> in the .java file so the reader/reviewer doesn't have to jump back and forth
> between the two files nearly as much.
> 
> The `id` argument had to be passed to the old version of the function calls
> in order for the logging messages generated by the agent code to (easily)
> have the same thread names as the Java side logging. Once all the error
> checking was moved to the Java side, that made the `id` argument no longer
> necessary and also allowed the native `check_jvmti_status()` function (along
> with other functions) to be removed. So there was nothing wrong with the
> native `check_jvmti_status()` function; it was just made obsolete by the code
> migration to the Java side.
> 
> I consider @robehn's idea here to be good evolution for these kinds of tests to
> move code from the native side, where the code is more platform dependent,
> to the Java side, where the code is more platform independent.
> 
> I hope I have convinced you that this is a good changeset for moving forward.

Dan,
I'm sorry, I do not see much value in this approach going forward. One line with a call to check_jvmti_status() does not add much to the platform dependency no to complexity in native code. Also, more sophisticated analysis of the returned error code frequently is needed. Also, I consider it not feasible to move a thousand of JVMTI tests in this direction. So that we will end up with inconsistency over all tests.
I've already marked this as reviewed, so you can push it. But I'm against following this pattern going forward. Or at least, we need to consult with the SQE engineers first.
Thanks,
Serguei

-------------

PR: https://git.openjdk.java.net/jdk/pull/2899