Re: RFR(XS): 8245126 Kitchensink fails with: assert(!method->is_old()) failed: Should not be installing old methods
On 5/31/20 23:16,
serguei.spit...@oracle.com wrote:
Hi Dean,
To check the is_old as you suggest the target method has to be
passed
to the cache_jvmti_state() as argument. Is it what you are
suggesting?
Just want to make sure I understand you correctly.
The cache_jvmti_state() and cache_dtrace_flags() are called in
the
CompileBroker::init_compiler_runtime() for a ciEnv with the NULL
CompileTask
which looks unnecessary (or I don't understand it):
bool CompileBroker::init_compiler_runtime() {
CompilerThread* thread = CompilerThread::current();
. . .
ciEnv ci_env((CompileTask*)NULL);
// Cache Jvmti state
ci_env.cache_jvmti_state();
// Cache DTrace flags
ci_env.cache_dtrace_flags();
The JVMCI has a separate implementation for ciEnv which is
jvmciEnv and
its own set of cache_jvmti_state() and jvmti_state_changed()
functions.
Both are not called in the JVMCI case.
So, these checks look as broken in JVMCI now.
Not sure, I have enough compiler knowledge to fix this at this
stage of release.
Would it better to file a separate hotspot/compiler RFE targeted
to 16?
It can be assigned to me if it helps.
I forgot to tell that this RFE should probably include some
refactoring
to make all this more clear and elegant.
Thanks,
Serguei
Thanks,
Serguei
On 5/28/20 10:54, Dean Long wrote:
Sure, you could just have cache_jvmti_state() return a boolean
to bail out immediately for is_old.
dl
On 5/28/20 7:23 AM, serguei.spit...@oracle.com wrote:
Hi Dean,
Thank you for looking at this!
Okay. Let me check what cab be done in this direction.
There is no point to cache is_old. The compilation has to
bail out if it is discovered to be true.
Thanks,
Serguei
On 5/28/20 00:59, Dean Long wrote:
This seems OK as long as the memory barriers in the thread
state transitions prevent the C++ compiler from doing
something like reading is_old before reading
redefinition_count. I would feel better if both JVMCI and
C1/C2 cached is_old and redefinition_count at the same time
(making sure to be in the _thread_in_vm state), then bail
out based on the cached value of is_old.
dl
On 5/26/20 12:04 AM, serguei.spit...@oracle.com
wrote:
On 5/25/20 23:39, serguei.spit...@oracle.com
wrote:
Please, review a fix for:
https://bugs.openjdk.java.net/browse/JDK-8245126
Webrev:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/kitchensink-comp.1/
Summary:
The Kitchensink stress test with the Instrumentation
module enabled does
a lot of class retransformations in parallel with all
other stressing.
It provokes the assert at the compiled code
installation time:
assert(!method->is_old()) failed: Should not be
installing old methods
The problem is that the
CompileBroker::invoke_compiler_on_method in C2 version
(non-JVMCI tiered compilation) is missing the check
that exists in the JVMCI
part of implementation:
2148 // Skip redefined methods
2149 if (target_handle->is_old()) {
2150 failure_reason = "redefined method";
2151 retry_message = "not retryable";
2152 compilable = ciEnv::MethodCompilable_never;
2153 } else {
. . .
2168 }
The fix is to add this check.
Sorry, forgot to explain one thing.
Compiler code has a special mechanism to ensure the JVMTI
class redefinition did
not happen while the method was compiled, so all the
assumptions remain correct.
2190 // Cache Jvmti state
2191 ci_env.cache_jvmti_state();
Part of this is a check that the value of
JvmtiExport::redefinition_count() is
cached in ciEnv variable: _jvmti_redefinition_count.
The JvmtiExport::redefinition_count() val
Re: RFR(XS): 8245126 Kitchensink fails with: assert(!method->is_old()) failed: Should not be installing old methods
Hi Dean,
To check the is_old as you suggest the target method has to be
passed
to the cache_jvmti_state() as argument. Is it what you are
suggesting?
Just want to make sure I understand you correctly.
The cache_jvmti_state() and cache_dtrace_flags() are called in the
CompileBroker::init_compiler_runtime() for a ciEnv with the NULL
CompileTask
which looks unnecessary (or I don't understand it):
bool CompileBroker::init_compiler_runtime() {
CompilerThread* thread = CompilerThread::current();
. . .
ciEnv ci_env((CompileTask*)NULL);
// Cache Jvmti state
ci_env.cache_jvmti_state();
// Cache DTrace flags
ci_env.cache_dtrace_flags();
The JVMCI has a separate implementation for ciEnv which is
jvmciEnv and
its own set of cache_jvmti_state() and jvmti_state_changed()
functions.
Both are not called in the JVMCI case.
So, these checks look as broken in JVMCI now.
Not sure, I have enough compiler knowledge to fix this at this
stage of release.
Would it better to file a separate hotspot/compiler RFE targeted
to 16?
It can be assigned to me if it helps.
Thanks,
Serguei
On 5/28/20 10:54, Dean Long wrote:
Sure,
you could just have cache_jvmti_state() return a boolean to bail
out immediately for is_old.
dl
On 5/28/20 7:23 AM, serguei.spit...@oracle.com wrote:
Hi Dean,
Thank you for looking at this!
Okay. Let me check what cab be done in this direction.
There is no point to cache is_old. The compilation has to bail
out if it is discovered to be true.
Thanks,
Serguei
On 5/28/20 00:59, Dean Long wrote:
This seems OK as long as the memory barriers in the thread
state transitions prevent the C++ compiler from doing
something like reading is_old before reading
redefinition_count. I would feel better if both JVMCI and
C1/C2 cached is_old and redefinition_count at the same time
(making sure to be in the _thread_in_vm state), then bail out
based on the cached value of is_old.
dl
On 5/26/20 12:04 AM, serguei.spit...@oracle.com
wrote:
On 5/25/20 23:39, serguei.spit...@oracle.com
wrote:
Please, review a fix for:
https://bugs.openjdk.java.net/browse/JDK-8245126
Webrev:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/kitchensink-comp.1/
Summary:
The Kitchensink stress test with the Instrumentation
module enabled does
a lot of class retransformations in parallel with all
other stressing.
It provokes the assert at the compiled code installation
time:
assert(!method->is_old()) failed: Should not be
installing old methods
The problem is that the
CompileBroker::invoke_compiler_on_method in C2 version
(non-JVMCI tiered compilation) is missing the check that
exists in the JVMCI
part of implementation:
2148 // Skip redefined methods
2149 if (target_handle->is_old()) {
2150 failure_reason = "redefined method";
2151 retry_message = "not retryable";
2152 compilable = ciEnv::MethodCompilable_never;
2153 } else {
. . .
2168 }
The fix is to add this check.
Sorry, forgot to explain one thing.
Compiler code has a special mechanism to ensure the JVMTI
class redefinition did
not happen while the method was compiled, so all the
assumptions remain correct.
2190 // Cache Jvmti state
2191 ci_env.cache_jvmti_state();
Part of this is a check that the value of
JvmtiExport::redefinition_count() is
cached in ciEnv variable: _jvmti_redefinition_count.
The JvmtiExport::redefinition_count() value change means a
class redefinition
happened which also implies some of methods may become old.
However, the method being compiled can be already old at the
point where the
redefinition counter is cached, so the redefinition counter
check does not help much.
Thanks,
Serguei
Testing:
Ran Kitchensink test with the Instrumentation mod
Re: RFR JDK-8232222: Set state to 'linked' when an archived class is restored at runtime
Hi Jiangli,
On 29/05/2020 9:02 am, Jiangli Zhou wrote:
(Looping in serviceability-dev@openjdk.java.net ...)
Hi David and Ioi,
On Wed, May 27, 2020 at 11:15 PM David Holmes wrote:
Hi Jiangli,
On 28/05/2020 11:35 am, Ioi Lam wrote:
On 5/27/20 6:17 PM, Jiangli Zhou wrote:
On Wed, May 27, 2020 at 1:56 PM Ioi Lam wrote:
On 5/26/20 6:21 PM, Jiangli Zhou wrote:
Focusing on the link state for archived classes in this thread, I
updated the webrev to only set archived boot classes to 'linked' state
at restore time. More investigations can be done for archived classes
for other builtin loaders.
https://bugs.openjdk.java.net/browse/JDK-823
http://cr.openjdk.java.net/~jiangli/823/webrev.02/
Please let me know if there is any additional concerns to the change.
Best regards,
Jiangli
Hi Jiangli,
I think the change is fine. I am wondering if this
2530 if (!BytecodeVerificationLocal &&
2531loader_data->is_the_null_class_loader_data()) {
2532 _init_state = linked;
2533 }
can be changed to
if (!BytecodeVerificationLocal &&
loader_data->is_the_null_class_loader_data() &&
!JvmtiExport::should_post_class_prepare())
That way, there's no need to change systemDictionary.cpp.
I was going to take the suggestion, but realized that it would add
unnecessary complications for archived boot classes with class
pre-initialization support. Some agents may set
JvmtiExport::should_post_class_prepare(). It's worthwhile to support
class pre-init uniformly for archived boot classes with
JvmtiExport::should_post_class_prepare() enabled or disabled.
This would introduce behavioral changes when JVMTI is enabled:
+ The order of JvmtiExport::post_class_prepare is different than before
+ JvmtiExport::post_class_prepare may be called for a class that was not
called before (if the class is never linked during run time)
+ JvmtiExport::post_class_prepare was called inside the init_lock, now
it's called outside of the init_lock
I have to say I share Ioi's concerns here. This change will impact JVM
TI agents in a way we can't be sure of. From a specification perspective
I think we are fine as linking can be lazy or eager, so there's no
implied order either. But this would be a behavioural change that will
be observable by agents. (I'm less concerned about the init_lock
situation as it seems potentially buggy to me to call out to an agent
with the init_lock held in the first place! I find it hard to imagine an
agent only working correctly if the init_lock is held.)
Totally agree that we need to be very careful here (that's also part
of the reason why I separated this into an individual RFE for the
dedicated discussion). David, thanks for the analysis from the spec
perspective! Agreed with the init_lock comment also. In the future, I
think we can even get rid of the needs for init_lock completely for
some of the pre-initialized classes.
This change has gone through extensive testing since the later part of
last year and has been in use (with the default CDS) with agents that
do post_class_prepare. Hopefully that would ease some of the concerns.
That is good to know, but that is just one sample of a set of agents.
This would need a CSR request and involvement of the serviceabilty folk,
to work through any potential issues.
I've looped in serviceability-dev@openjdk.java.net for this
discussion. Chris or Serguei could you please take a look of the
change, http://cr.openjdk.java.net/~jiangli/823/webrev.02/,
specifically the JvmtiExport::post_class_prepare change in
systemDictionary.cpp.
Filing a CSR request sounds good to me. The CSR looks after source,
binary, and behavioral compatibility. From a behavior point of view,
the change most likely does not cause any visible effects to a JVMTI
agent (based on what's observed in testing and usages). What should be
included in the CSR?
The CSR request should explain the behavioural change that will be
observable by agents, and all of the potential compatibility issues that
might arise from that - pointing out of course that as the spec (JVMS
5.4**) allows for eager or lazy linking, agents shouldn't be relying on
the exact timing or order of events.
** I note this section has some additional constraints regarding
dynamically computed constants that might also come into play with this
pre-linking for CDS classes.
Cheers,
David
-
Ioi's suggestion avoids this problem, but, as you note, at the expense
of disabling this optimisation if an agent is attached and wants class
prepare events.
Right, if we handle that case conditionally, we would alway need to
store the cached static field values separately since the dump time
cannot foresee if the runtime can set boot classes in 'linked' state
(and 'fully_initialized' state with the planned changes) at restore
time. As a result, we need to handle all pre-initialized static fields
like what we are doing today, which is storing them in the archived
cl
Re: RFR: JDK-8225056 VM support for sealed classes
Hi Harold,
On 1/06/2020 8:57 am, Harold Seigel wrote:
Thanks for the comments.
Here's version 3 of the JDK and VM changes for sealed classes.
full webrev:
http://cr.openjdk.java.net/~hseigel/sealedClasses.8225056.3/webrev/
The new webrev contains just the following three changes:
1. The sealed classes API's in Class.java (permittedSubclasses() and
isSealed()) were revised and, in particular, API
permittedSubclasses() no longer uses reflection.
For those following along we have presently abandoned the attempt to
cache the array in ReflectionData.
Current changes look okay. But I note from the CSR there appears to be a
further minor update to the javadoc coming.
2. An unneeded 'if' statement was removed from
JVM_GetPermittedSubclasses() (pointed out by David.)
Looks good.
3. VM runtime test files SealedUnnamedModuleIntfTest.java and
Permitted.java were changed to add a test case for a non-public
permitted subclass and its sealed superclass being in the same
module and package.
Looks good.
Additionally, two follow on RFE's will be filed. One to add additional
VM sealed classes tests
Thanks. I think there is a more mechanical approach to testing here that
will allow the complete matrix to be easily covered with minimal
duplication between testing for named and unnamed modules.
and one to improve the implementations of the
sealed classes API's in Class.java.
Thanks.
David
-
Thanks, Harold
On 5/28/2020 8:30 PM, David Holmes wrote:
Hi Harold,
Sorry Mandy's comment raised a couple of issues ...
On 29/05/2020 7:12 am, Mandy Chung wrote:
Hi Harold,
On 5/27/20 1:35 PM, Harold Seigel wrote:
Incremental webrev:
http://cr.openjdk.java.net/~hseigel/sealedClasses.8225056.incr.2/
full webrev:
http://cr.openjdk.java.net/~hseigel/sealedClasses.8225056.2/webrev/
Class.java
4406 ReflectionData rd = reflectionData();
4407 ClassDesc[] tmp = rd.permittedSubclasses;
4408 if (tmp != null) {
4409 return tmp;
4410 }
4411
4412 if (isArray() || isPrimitive()) {
4413 rd.permittedSubclasses = new ClassDesc[0];
4414 return rd.permittedSubclasses;
4415 }
This causes an array class or primitive type to create a
ReflectionData. It should first check if this is non-sealed class
and returns a constant empty array.
It can't check if this is a non-sealed class as the isSealed() check
calls the above code! But for arrays and primitives which can't be
sealed we should just do:
4412 if (isArray() || isPrimitive()) {
4413 return new ClassDesc[0];
4414 }
But this then made me realize that we need to be creating defensive
copies of the returned arrays, as happens with other APIs that use
ReflectionData.
Backing up a bit I complained that:
public boolean isSealed() {
return permittedSubclasses().length != 0;
}
is a very inefficient way to answer the question as to whether a class
is sealed, so I suggested that the result of permittedSubclasses() be
cached. Caching is not without its own issues as we are discovering,
and when you add in defensive copies this seems to be trading one
inefficiency for another. For nestmates we don't cache
getNestMembers() because we don;t think it will be called often - it
is there to complete the introspection API of Class rather than being
anticipated as used in a regular programmatic sense. I expect the same
is true for permittedSubclasses(). Do we expect isSealed() to be used
often or is it too just there for completeness? If just for
completeness then perhaps a VM query would be a better compromise on
the efficiency front? Otherwise I can accept the current
implementation of isSealed(), and a non-caching permittedClasses() for
this initial implementation of sealed classes. If efficiency turns out
to be a problem for isSealed() then we can revisit it then.
Thanks,
David
In fact, ReflectionData caches the derived names and reflected
members for performance and also they may be invalidated when the
class is redefined. It might be okay to add
ReflectionData::permittedSubclasses while `PermittedSubclasses`
attribute can't be redefined and getting this attribute is not
performance sensitive. For example, the result of `getNestMembers`
is not cached in ReflectionData. It may be better not to add it in
ReflectionData for modifiable and performance-sensitive data.
4421 tmp = new ClassDesc[subclassNames.length];
4422 int i = 0;
4423 for (String subclassName : subclassNames) {
4424 try {
4425 tmp[i++] = ClassDesc.of(subclassName.replace('/', '.'));
4426 } catch (IllegalArgumentException iae) {
4427 throw new InternalError("Invalid type in permitted subclasses
information: " + subclassName, iae);
4428 }
4429 }
Nit: rename tmp to some other name e.g. descs
I read the JVMS but it isn't clear to me that the VM will validate
the names in `PermittedSubclasses`attribute are valid class
descriptors. I see ConstantPool::is_klass_or_reference check but
can't find where it validates the name is
Re: RFR: JDK-8225056 VM support for sealed classes
Thanks for the comments.
Here's version 3 of the JDK and VM changes for sealed classes.
full webrev:
http://cr.openjdk.java.net/~hseigel/sealedClasses.8225056.3/webrev/
The new webrev contains just the following three changes:
1. The sealed classes API's in Class.java (permittedSubclasses() and
isSealed()) were revised and, in particular, API
permittedSubclasses() no longer uses reflection.
2. An unneeded 'if' statement was removed from
JVM_GetPermittedSubclasses() (pointed out by David.)
3. VM runtime test files SealedUnnamedModuleIntfTest.java and
Permitted.java were changed to add a test case for a non-public
permitted subclass and its sealed superclass being in the same
module and package.
Additionally, two follow on RFE's will be filed. One to add additional
VM sealed classes tests and one to improve the implementations of the
sealed classes API's in Class.java.
Thanks, Harold
On 5/28/2020 8:30 PM, David Holmes wrote:
Hi Harold,
Sorry Mandy's comment raised a couple of issues ...
On 29/05/2020 7:12 am, Mandy Chung wrote:
Hi Harold,
On 5/27/20 1:35 PM, Harold Seigel wrote:
Incremental webrev:
http://cr.openjdk.java.net/~hseigel/sealedClasses.8225056.incr.2/
full webrev:
http://cr.openjdk.java.net/~hseigel/sealedClasses.8225056.2/webrev/
Class.java
4406 ReflectionData rd = reflectionData();
4407 ClassDesc[] tmp = rd.permittedSubclasses;
4408 if (tmp != null) {
4409 return tmp;
4410 }
4411
4412 if (isArray() || isPrimitive()) {
4413 rd.permittedSubclasses = new ClassDesc[0];
4414 return rd.permittedSubclasses;
4415 }
This causes an array class or primitive type to create a
ReflectionData. It should first check if this is non-sealed class
and returns a constant empty array.
It can't check if this is a non-sealed class as the isSealed() check
calls the above code! But for arrays and primitives which can't be
sealed we should just do:
4412 if (isArray() || isPrimitive()) {
4413 return new ClassDesc[0];
4414 }
But this then made me realize that we need to be creating defensive
copies of the returned arrays, as happens with other APIs that use
ReflectionData.
Backing up a bit I complained that:
public boolean isSealed() {
return permittedSubclasses().length != 0;
}
is a very inefficient way to answer the question as to whether a class
is sealed, so I suggested that the result of permittedSubclasses() be
cached. Caching is not without its own issues as we are discovering,
and when you add in defensive copies this seems to be trading one
inefficiency for another. For nestmates we don't cache
getNestMembers() because we don;t think it will be called often - it
is there to complete the introspection API of Class rather than being
anticipated as used in a regular programmatic sense. I expect the same
is true for permittedSubclasses(). Do we expect isSealed() to be used
often or is it too just there for completeness? If just for
completeness then perhaps a VM query would be a better compromise on
the efficiency front? Otherwise I can accept the current
implementation of isSealed(), and a non-caching permittedClasses() for
this initial implementation of sealed classes. If efficiency turns out
to be a problem for isSealed() then we can revisit it then.
Thanks,
David
In fact, ReflectionData caches the derived names and reflected
members for performance and also they may be invalidated when the
class is redefined. It might be okay to add
ReflectionData::permittedSubclasses while `PermittedSubclasses`
attribute can't be redefined and getting this attribute is not
performance sensitive. For example, the result of `getNestMembers`
is not cached in ReflectionData. It may be better not to add it in
ReflectionData for modifiable and performance-sensitive data.
4421 tmp = new ClassDesc[subclassNames.length];
4422 int i = 0;
4423 for (String subclassName : subclassNames) {
4424 try {
4425 tmp[i++] = ClassDesc.of(subclassName.replace('/', '.'));
4426 } catch (IllegalArgumentException iae) {
4427 throw new InternalError("Invalid type in permitted subclasses
information: " + subclassName, iae);
4428 }
4429 }
Nit: rename tmp to some other name e.g. descs
I read the JVMS but it isn't clear to me that the VM will validate
the names in `PermittedSubclasses`attribute are valid class
descriptors. I see ConstantPool::is_klass_or_reference check but
can't find where it validates the name is a valid class descriptor -
can you point me there? (otherwise, maybe define it to be unspecified?)
W.r.t. the APIs. I don't want to delay this review. I see that you
renamed the method to new API style as I brought up. OTOH, I expect
more discussion is needed. Below is a recent comment from John on
this topic [1]
One comment, really for the future, regarding the shape of the Java
API here: It uses Optional and omits the "get" prefix on accessors.
This is the New Style, as opposed to the Classic Style using null
(for "empty" resu
RFR(XS): 8221306: JVMTI spec for FramePop(), MethodExit(), and MethodEnter() could use some cleanup
Please, review a fix for small spec bug: https://bugs.openjdk.java.net/browse/JDK-8221306 Webrev: http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/jvmt-funcs-cleanup.1/src/ Updated JVM TI spec for the FramePop, MethodEntry and MethodExit events: http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/jvmt-funcs-cleanup.1/docs/specs/jvmti.html#FramePop Summary: It is a minor spec cleanup for JVM TI events FramePop/MethodEntry/MethodExit: - added small clarification that GetFrameLocation needs to be asked for frame at depth 0 - removed partly unneeded and partly incorrect statements about MethodExit event argument Testing: Manually verified the generated jvmti.html. I think, there is no need to file a CSR for this spec update as it is just minor cleanup. Thanks, Serguei
Re: RFR(XS): 8234882: JVM TI StopThread should only allow ThreadDeath
Hi David,
Also jumping to end.
On 5/30/20 06:50, David Holmes wrote:
Hi Serguei,
Jumping to the end for now ...
On 30/05/2020 5:50 am, serguei.spit...@oracle.com wrote:
Hi David and reviewers,
The updated webrev version is:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/jvmti-stop-thread.2/src/
This update adds testing that StopThread can return
JVMTI_ERROR_INVALID_OBJECT error code.
The JVM TI StopThread spec is:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/jvmti-stop-thread.2/docs/specs/jvmti.html#StopThread
There is a couple of comments below.
On 5/29/20 06:18, David Holmes wrote:
On 29/05/2020 6:24 pm, serguei.spit...@oracle.com wrote:
On 5/29/20 00:56, serguei.spit...@oracle.com wrote:
On 5/29/20 00:42, serguei.spit...@oracle.com wrote:
Hi David,
Thank you for reviewing this!
On 5/28/20 23:57, David Holmes wrote:
Hi Serguei,
On 28/05/2020 3:12 pm, serguei.spit...@oracle.com wrote:
Hi David,
I've updated the CSR and webrev in place.
The changes are:
- addressed David's suggestion to rephrase StopThread
description change
- replaced JVMTI_ERROR_INVALID_OBJECT with
JVMTI_ERROR_ILLEGAL_ARGUMENT
- updated the implementation in jvmtiEnv.cpp to return
JVMTI_ERROR_ILLEGAL_ARGUMENT
- updated one of the nsk.jvmti StopThread tests to check
error case with the JVMTI_ERROR_ILLEGAL_ARGUMENT
I'm reposting the links for convenience.
Enhancement:
https://bugs.openjdk.java.net/browse/JDK-8234882
CSR draft:
https://bugs.openjdk.java.net/browse/JDK-8245853
Spec updates are good - thanks.
Thank you for the CSR review.
Webrev:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/jvmti-stop-thread.1/src/
src/hotspot/share/prims/jvmtiEnv.cpp
The ThreadDeath check is fine but I'm a bit confused about the
additional null check that leads to JVMTI_ERROR_INVALID_OBJECT.
I can't see how resolve_external_guard can return NULL when not
passed in NULL. Nor why that would result in
JVMTI_ERROR_INVALID_OBJECT rather than JVMTI_ERROR_NULL_POINTER.
And I note JVMTI_ERROR_NULL_POINTER is not even a listed error
for StopThread! This part of the change seems unrelated to this
issue.
I was also surprised with the JVMTI_ERROR_NULL_POINTER and
JVMTI_ERROR_INVALID_OBJECT error codes.
The JVM TI spec automatic generation adds these two error codes
for a jobject parameter.
Also, they both are from the Universal Errors section:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/jvmti-stop-thread.1/docs/specs/jvmti.html#universal-error
You can find a link to this section at the start of the Error
section:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/jvmti-stop-thread.1/docs/specs/jvmti.html#StopThread
My understanding (not sure, it is right) is that NULL has to be
reported with JVMTI_ERROR_NULL_POINTER and a bad
jobject (for instance, a WeakReference with a GC-ed target) has
to be reported with JVMTI_ERROR_INVALID_OBJECT.
At least, I was not able to construct a test case to get this
error code returned.
So, I'm puzzled with this. I'll try to find some examples with
JVMTI_ERROR_NULL_POINTER errors.
Found the explanation.
The JDI file:
src/jdk.jdi/share/classes/com/sun/tools/jdi/JDWPException.java
has a fragment that translate the INVALID_OBJECT error to the
ObjectCollectedException:
RuntimeException toJDIException() {
switch (errorCode) {
case JDWP.Error.INVALID_OBJECT:
return new ObjectCollectedException();
So, the INVALID_OBJECT is for a jobject handle that is referencing
a collected object.
It means that previous implementation incorrectly returned
JVMTI_ERROR_NULL_POINTER error code.
I should create and delete local or global ref to construct a test
case for this.
Interesting that the JDWPException::toJDIException() does not
convert the ILLEGAL_ARGUMENT error code to an
IllegalArgumentException.
I've just added this conversion.
Given the definition of JDWP INVALID_OBJECT then obviously JDI
converts it to ObjectCollectedException.
So reading further in JNI spec:
"Weak global references are a special kind of global reference.
Unlike normal global references, a weak global reference allows the
underlying Java object to be garbage collected. Weak global
references may be used in any situation where global or local
references are used."
So it seems that any function that takes a jobject cxould in fact
accept a jweak, in which case JVMTI_ERROR_INVALID_OBJECT is a
possibility in all cases. So IIUC JNIHandles::resolve_external_guard
can return NULL if a weak reference has been collected. So the new
code you propose seems correct.
You are right about weak global references.
I was able to construct a test case for JVMTI_ERROR_INVALID_OBJECT.
The JNI NewGlobalRef and DeleteGlobalRef are used for it.
You can find it in the updated webrev version.
However, this still is unrelated to the current issue and I do not
see other JVM TI doing checks for this case. So this seems to be a
much bro
