request for review: 6436034: Instance filter doesn't filter event if it occurs in native method

[Mandy Chung]

  On 12/15/10 20:37, Keith McGuigan wrote:
>
> On Dec 15, 2010, at 4:44 AM, Alan Bateman wrote:
>
>> Keith McGuigan wrote:
>>>
>>> :
>>> Yeah I missed that check too.  I'm very much in favor of being able 
>>> to use an older (or newer) JVM in the JDK7 image.  Triage is one 
>>> important reason but another is the non-synchronous putback 
>>> schedules of the jdk and hotspot workspaces.  There will be a period 
>>> of QA time when the JDK uses an older VM (or vice-versa) just 
>>> because of the way our process works.
>>>
>>> I think the jdk code should probe the JVMTI version and use whatever 
>>> is available.  I'm in favor of modifying the code in debugInit.c to 
>>> verify only 1.1 and then dynamically probe before using any 1.2 
>>> features (and so on in the future).

Checking the JVMTI version would help when you bump the minor number in 
the JVMTI version from 1.1.x to 1.2.  If a new JVMTI function is added 
in JDK 7 in the future, it will bump the micro number but not the minor 
number.   I believe the isVersionGte12x() method will not work for that 
case.  The jdwp agent would get very complicated if it has to handle the 
micro edition to the JVMTI during development.  Or you still have to 
live with the synchronization putback schdedule - the jdk side has to 
wait until the hotspot change makes it into a promoted build; or another 
alternative is to check in a temporary check of the micro number  and 
remove that check in a later promoted build.

>> In the past I don't think we've done this. Instead we've usually just 
>> kept back the jdk changes until the hotspot changes made it into a 
>> promoted build. 
>>
>> One other thing is the regression test. The debugger tests are in 
>> jdk/test/com/sun/jdi and it is important to add a new test, or add to 
>> an existing test. If you are pushing the jdk changes before the VM 
>> changes have made it into a promoted build then I guess you'll need 
>> to hold off pushing the  test (or add it with the @ignore tag, 
>> removing it later via a different bugID).
>>
>> -Alan
>
> Ok, here's a new webrev: 
> http://cr.openjdk.java.net/~kamg/6436034/webrev.01/
>
> I added a regression test and modified the code in debugInit.cpp to 
> explicitly allow running with JVMTI 1.1 if that's what the JVM supports. 

This check looks okay but it doesn't help when new APIs are added in the 
same minor version.  It's not a problem for this fix to address.  
However, I wonder if the jdwp agent has to support older VM (older JVMTI 
version).

I understand the benefit of allowing an older VM to run on JDK 7 so that 
we can easily verify a hotspot fix and identify hotspot regression.  But 
I'm not sure if the jdwp agent should support that and wonder if it's 
useful in practice.  The use case requiring the jdwp agent to support an 
older JVMTI version is when a developer wants to debug a Java 
application on JDK 7 with a JDK 6 VM.   I would imagine that this is 
rarely needed.  Is there other use case that suggests this jvmti version 
check worth doing?

The current version check isn't too bad but I'm concerned down the road 
when more enhancements come in, it will complicate the JDWP 
implementation and various check of different versions are added in the 
implementation.

Would you consider keeping the jdwp agent in JDK 7 to require JVMTI 
version >= 1.2 (i.e. works with JDK 7 or newer VM)?

> The regression test is setup to pass when run with a JVM less than 
> version 20 build 05 (where the new JVMTI function is added), so it 
> should run and pass in either situation and won't require a later update.
>

When we do our engineering build of both jdk and hotspot repositories 
(or nightly testing), does the hotspot VM have an internal version 
string rather than 20.0-b03?  If that's the case, does that mean the 
test will just pass even if the hotspot supports JVMTI 1.2?  In that 
case, you would want the test to actually do the work.

FYI.  sun.misc.Version provides several internal methods to return the 
version number and build number of the VM that you can use in these 
tests to replace the regex.

Mandy

Mandy