[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-serviceability-dev
Subject:    Re: RFR: 8269542: JDWP: EnableCollection support is no longer spec compliant after JDK-8255987 [v3]
From:       Chris Plummer <cjplummer () openjdk ! java ! net>
Date:       2022-01-25 19:29:41
Message-ID: TMqL6jvZQHx3r3o1OkeH4AL2leyVkRgav_EBIRefCks=.3df0e1e7-2b69-418f-9a9c-7d68ebfb8727 () github ! com
[Download RAW message or body]

On Thu, 20 Jan 2022 04:22:28 GMT, Chris Plummer <cjplummer@openjdk.org> wrote:

> > The JDWP spec mentions nothing about `DisableCollection` and `EnableCollection` \
> > tracking the depth or nesting of the commands. This means that `EnableCollection` \
> > should enable collection no matter how many `DisableCollection` commands were \
> > done before it. This is how the debug agent used to work before \
> > [JDK-8255987](https://bugs.openjdk.java.net/browse/JDK-8255987). Now the \
> > commands' nesting level is tracked, meaning that if there are two \
> > `DisableCollection` commands, you need two `EnableCollection` commands to \
> > re-enable collection. This is contrary to what the spec says. Only one \
> > `EnableCollection` command should be needed to undo any number of \
> > `DisableCollection` commands. 
> > Note, JDWP differs from JDI here. The JDI spec explicitly says you need an enable \
> > for each disable in order to enable collection again. JDI tracks the disable \
> > count. Also, JDI only does the JDWP `DisableCollection` for the first disable, \
> > and only does the JDWP `EnableCollection` when the count goes back to 0. For this \
> > reason this JDWP bug cannot be reproduced by using JDI. You have to use JDWP \
> > directly. Unfortunately we have very limited direct testing of JDWP. JDWP testing \
> > mostly is done via JDI. What little JDWP testing we do have pretty much just \
> > verifies that reply packets are as expected. They don't verify VM or application \
> > behavior. For example, we have no JDWP test that tests that `DisableCollection` \
> > actually disables the collecting of the object. For this reason I have not added \
> > a test for this CR. 
> > Another issue is that support for `DisableCollection/EnableCollection` nesting \
> > was intermixed with the support for having `VM.Suspend` disable collection on all \
> > objects (which was the main purpose of \
> > [JDK-8255987](https://bugs.openjdk.java.net/browse/JDK-8255987)). As a result, if \
> > you do a `VM.Suspend` and then do a `DisableCollection` on an `ObjectReference`, \
> > that object can now be collected, even though the spec says it should not be \
> > during a `VM.Suspend`. This issues is documented in \
> > [JDK-8258071](https://bugs.openjdk.java.net/browse/JDK-8258071), and is also \
> > fixed by this PR. 
> > To fix both of these issues, `node->strongCount` should go back to being a \
> > boolean (`node->isStrong`) like it was before \
> > [JDK-8255987](https://bugs.openjdk.java.net/browse/JDK-8255987). Also, separate \
> > flags should be maintained to indicate if the reference is strong due to a \
> > `DisableCollection` and/or due to `VM.Suspend`. We need a flag for each, because \
> > it's possible that both can be true at the same time. When `node->isStrong` is \
> > true, if there is an `EnableCollection` it only gets set false if there is no \
> > current `VM.Suspend`. Likewise was if `VM.Resume` is done, it only gets set false \
> > if there is no outstanding `DisableCollection`. 
> > There should be no need for maintaining a count anymore since we aren't suppose \
> > to for `DisableCollection/EnableCollection`, and there is no need to for \
> > `VM.Suspend/Resume`, since it only calls `commonRef_pinAll()` code when the \
> > `suspendAllCount` is changed to/from 0. 
> > Please also note [JDK-8269232](https://bugs.openjdk.java.net/browse/JDK-8269232), \
> > which was also introduced by this nesting level counting, but is being fixed in \
> > more direct manner to keep the changes simple. The fix for CR replaces the \
> > changes for [JDK-8269232](https://bugs.openjdk.java.net/browse/JDK-8269232).
> 
> Chris Plummer has updated the pull request incrementally with one additional commit \
> since the last revision: 
> Fix some error case bugs that were introduced.

Thanks for the reviews Per and David!

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

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


[prev in list] [next in list] [prev in thread] [next in thread]