Re: Review Request JDK-8200559: Java agents doing instrumentation need a means to define auxiliary classes

[mandy chung]

https://bugs.openjdk.java.net/browse/JDK-8201784

This is the correct JBS issue (Sorry I cut-n-paste the wrong link).

Mandy

On 4/18/18 3:46 PM, mandy chung wrote:

Hi Rafael,

I think it's best to separate the testing requirement from java agents 
doing instrumentation that may run in production environment.


I have created a JBS issue to track the testing mode idea that would 
require more discussion and investigation:

https://bugs.openjdk.java.net/browse/JDK-8201562

I understand it's not as efficient to inject a class in a package 
different than the package of the transformed class. With the 
principle of least privilege, I prefer not to provide an API to inject 
any class in any package and you can achieve by calling 
retransformClasses.


What do you think?

Mandy

On 4/18/18 5:23 AM, Rafael Winterhalter wrote:

Hei Mandy,
Lookup::defineClass would always be an alternative but it would 
require me to open the class first. If the instrumented type can read 
the module with the callback but its module was not opened, this 
would not help me much, unfortunately. Also, I could not resolve this 
lookup as the class in question is not necessarily loaded at this point.

Best regards, Rafael

2018-04-17 9:28 GMT+02:00 mandy chung >:


Hi Rafael,

I see that mocking/proxying/testing framework should be looked at
separately since its requirements and approaches can be different
than tool agents.

On 4/17/18 5:06 AM, Rafael Winterhalter wrote:

Hei Mandy,

I have looked into several Java agents that I have worked on and
for many of them, this API does unfortunately not supply
sufficient access. I would therefore still prefer a method
Instrumentation::defineClass.

The problem is that some agents need to define classes in other
packages then in that of the instrumented class. For example, I
might need to enhance a library that defines a set of callback
classes in package A. All these classes share a common super
class with a package-private constructor. I want to instrument
some class in package B to use a callback that the library does
not supply and need to add a new callback class to A. This is
not possible using the current API.



Are these callback classes made available statically?  or just
dynamically defining additional class as needed?  Is
Lookup::defineClass an alternative if you get a hold of common
super class in A?


I could however achieve do so by calling
Instrumentation::retransform on one of the classes in A after
registering a class file transformer. Once the retransformation
is triggered, I can now define a class in A. Of course this is
inefficient and I would rather open the jdk.internal.misc module
and use the "old" API instead.

For this reason, I argue that this rather restrained API is not
convenient while it does not add anything to security. Also, for
the use case of Mockito, this would neither be sufficient as
Mockito sometimes redefines classes and sometimes adds a
subclass without retransforming. We would rather have direct
access to class definition once we are already running with the
privileges of a Java agent.

I would therefore suggest to add a method:

interface Instrumentation {
  Class defineClass(byte[] bytes, ProtectionDomain pd);
}

which can be implemented simply by delegating to
jdk.internal.misc.Unsafe.

On a side note. Does JavaLangAccess::defineClass work with the
bootstrap class loader? I have not tried it but I always thought
it was just an access layer for the class loader API that cannot
access the null value.



The JVM entry point does allow null loader.

Mandy



Thanks for considering this use case!
Best regards, Rafael

2018-04-15 8:23 GMT+02:00 mandy chung >:

Background:

Java agents support both load time and dynamic
instrumentation.   At load time,
the agent's ClassFileTransformer is invoked to transform
class bytes.  There is
no Class objects at this time.  Dynamic instrumentation is
when redefineClasses
or retransformClasses is used to redefine an existing loaded
class.  The
ClassFileTransformer is invoked with class bytes where the
Class object is present.

Java agent doing instrumentation needs a means to define
auxiliary classes
that are visible and accessible to the instrumented class. 
Existing agents
have been using sun.misc.Unsafe::defineClass to define aux
classes directly
or accessing protected ClassLoader::defineClass method with
setAccessible to
suppress the language access check (see [1] where this issue
was brought up).

Re: RFR: 8196325: GarbageCollectionNotificationInfo has same information for before and after

[mandy chung]

On 4/19/18 4:52 AM, sangheon.kim wrote:



Webrev:
http://cr.openjdk.java.net/~sangheki/8196325/webrev.1 (full)
http://cr.openjdk.java.net/~sangheki/8196325/webrev.1_to_0/ (inc)


Looks good.

Mandy

Re: RFR: 8196325: GarbageCollectionNotificationInfo has same information for before and after

[sangheon.kim]

Hi Mandy,

On 04/18/2018 02:20 AM, mandy chung wrote:

Hi Sangheon,


On 4/18/18 12:41 PM, sangheon.kim wrote:


CR: https://bugs.openjdk.java.net/browse/JDK-8196325
webrev: http://cr.openjdk.java.net/~sangheki/8196325/webrev.0/


This is indeed a regression.  GcInfoBuilder depends on the order of 
the pool name array.


The change looks okay.  I would suggest to use stream in the new 
getAllMemoryPoolNames() like this:


    public static String[] getAllMemoryPoolNames() {
    return Arrays.stream(MemoryImpl.getMemoryPools())
          .map(MemoryPoolMXBean::getName)
 .toArray(String[]::new);
    }

Done.



Testing: 
jdk-tier1,jdk-tier2,jdk-tier3,hs-tier1,hs-tier2,builds-tier1, 
jdk_management, jdk_jmx




These test groups are good.

Okay.

Webrev:
http://cr.openjdk.java.net/~sangheki/8196325/webrev.1 (full)
http://cr.openjdk.java.net/~sangheki/8196325/webrev.1_to_0/ (inc)

Sangheon




Mandy


Thanks,
Sangheon

Re: Review Request JDK-8200559: Java agents doing instrumentation need a means to define auxiliary classes

[Rafael Winterhalter]

Hei Mandy,

Generally I agree with you that these two should be seperate but I think
that there is a very blurry border between the two. Java agents are
sometimes used to test something in a production environment and there is a
long history of agent development that relies on the easy and general way
of defining classes using sun.misc.Unsafe. I have looked through the agents
I worked on and for the most, classes are defined in the same package. In a
few cases, and this is true for most agents, there is however a need to
define classes in other packages, too.

In this context, things quickly get complex to manage. If I use a method
handle lookup, I need to find a loaded class and potentially open its
module. From within an agent, it is however way to easy to break class
loading orders and to trigger premature loading what makes this
inconvienent. The same goes for retransforming a class to get hold of a
suitable class definer instance for a given package.

For all these reasons, I believe that given this approach, most people will
simply fall back to jdk.internal.misc.Unsafe which can be opened too and
which is much easier. Therefore, given the tradition of using this class
and its more powerful and more performent approach to defining classes
given the multitude of use cases, I believe that this API would not be used
by most but people would continue to rely on jdk.internal.misc.Unsafe. At
least, I think that I would go for this solution for most of my agents,
also considering an easy and safe migration.

For this reason, I still recommend adding a method
Instrumentation::defineClass instead. Especially since a Java agent does
have this privilege, there is no security constraint to uphold here and it
offers the simplest solution that most agent developers hope for.
Therefore, I do not think that the principle of least privilege should
apply here. In contrast, if this API is not added I am afraid that many
tools opening jdk.internal.misc might lead to exploits if this package is
opened to an unnamed module, for example by APM or other production
monitoring tools that typically need to define classes in packages in which
no retransformation is applied. (The example I mentioned comes from a real
use case in a very widely used APM tool.)

Thank you again for considering my point of view on this!
Best regards, Rafael



2018-04-18 9:46 GMT+02:00 mandy chung :

> Hi Rafael,
>
> I think it's best to separate the testing requirement from java agents
> doing instrumentation that may run in production environment.
>
> I have created a JBS issue to track the testing mode idea that would
> require more discussion and investigation:
>   https://bugs.openjdk.java.net/browse/JDK-8201562
>
> I understand it's not as efficient to inject a class in a package
> different than the package of the transformed class.  With the principle of
> least privilege, I prefer not to provide an API to inject any class in any
> package and you can achieve by calling retransformClasses.
>
> What do you think?
>
> Mandy
>
> On 4/18/18 5:23 AM, Rafael Winterhalter wrote:
>
> Hei Mandy,
> Lookup::defineClass would always be an alternative but it would require me
> to open the class first. If the instrumented type can read the module with
> the callback but its module was not opened, this would not help me much,
> unfortunately. Also, I could not resolve this lookup as the class in
> question is not necessarily loaded at this point.
> Best regards, Rafael
>
> 2018-04-17 9:28 GMT+02:00 mandy chung :
>
>> Hi Rafael,
>>
>> I see that mocking/proxying/testing framework should be looked at
>> separately since its requirements and approaches can be different than tool
>> agents.
>>
>> On 4/17/18 5:06 AM, Rafael Winterhalter wrote:
>>
>> Hei Mandy,
>>
>> I have looked into several Java agents that I have worked on and for many
>> of them, this API does unfortunately not supply sufficient access. I would
>> therefore still prefer a method Instrumentation::defineClass.
>>
>> The problem is that some agents need to define classes in other packages
>> then in that of the instrumented class. For example, I might need to
>> enhance a library that defines a set of callback classes in package A. All
>> these classes share a common super class with a package-private
>> constructor. I want to instrument some class in package B to use a callback
>> that the library does not supply and need to add a new callback class to A.
>> This is not possible using the current API.
>>
>>
>> Are these callback classes made available statically?  or just
>> dynamically defining additional class as needed?  Is Lookup::defineClass an
>> alternative if you get a hold of common super class in A?
>>
>> I could however achieve do so by calling Instrumentation::retransform on
>> one of the classes in A after registering a class file transformer. Once
>> the retransformation is triggered, I can now define a class in A. Of course
>> 

Re: RFR: JDK-8174994: SA: clhsdb printmdo throws WrongTypeException when attached to a process with CDS

[Jini George]

Thank you very much for the reviews, Coleen and Ioi. I have filed the RFE.

-Jini.

On 4/17/2018 11:09 AM, Ioi Lam wrote:

The changes look good to me.

I agree with Coleen. Maybe FileMapHeader should be moved to a common 
file that can be included in all the ps_core files, as well as the VM's 
filemap.hpp?


Thanks

- Ioi


On 4/13/18 8:35 AM, coleen.phillim...@oracle.com wrote:


This change seems good.

http://cr.openjdk.java.net/~jgeorge/8174994/webrev.00/src/jdk.hotspot.agent/linux/native/libsaproc/ps_core.c.udiff.html 



It seems that you have three copies of this code.  Can you file an RFE 
to consolidate these?


thanks,
Coleen

On 4/12/18 12:21 AM, Jini George wrote:

Ping: Gentle reminder !

Thanks,
Jini.

On 4/6/2018 9:51 PM, Jini George wrote:

Hello!

Requesting reviews for: 
https://bugs.openjdk.java.net/browse/JDK-8174994

Webrev: http://cr.openjdk.java.net/~jgeorge/8174994/webrev.00/

While trying to identify the type given an address, a 
WrongTypeException was getting thrown with various clhsdb commands 
(like printmdo, jstack, etc). This was since SA tries to map an 
address to a hotspot C++ type by comparing the vtable address to the 
vtable address values of known types. With CDS, since the vtables 
are copied over for the Metadata classes, the vtable addresses 
themselves don't match (though, of course, the contents will), and 
SA errors out.


The fix has been implemented by making changes to read in the md 
region (consisting of the c++ vtables) of the CDS archive in SA, and 
mapping the vtable addresses to the corresponding metadata type 
(ConstantPool, InstanceKlass, InstanceClassLoaderKlass, 
InstanceMirrorKlass, InstanceRefKlass, Method, ObjArrayKlass, 
TypeArrayKlass).


For corefiles, an additional modification has been done to have the 
replicated FileMapHeader structure (from 
src/hotspot/share/memory/filemap.hpp, which is replicated in SA in 
ps_core.c), to be in sync with the corresponding definition in 
src/hotspot/share/memory/filemap.hpp.


Test cases to test both live and corefile debugging are being added 
with this. These and other SA tests pass on Mach5.


Thanks,
Jini.

Re: RFR: JDK-8174994: SA: clhsdb printmdo throws WrongTypeException when attached to a process with CDS

[Jini George]

I agree with the need of testing as much as we can. I could do something 
on the lines of how other debuggers like LLDB test: if we can't find the 
core file location, check for "|" at the beginning of a line in the 
/proc/sys/kernel/core_pattern file -- and fail with a message stating 
that the system is using a crash reporting tool.


Thank you,
Jini.

On 4/18/2018 12:40 PM, David Holmes wrote:

My 2c ...

We have to have tests that can test core file attaching capability - 
else we don't know it works. So we have to try and generate a core file.


But, we have to expect that in many cases no core file will be generated 
even if the hs-err file claims it was. For example my primary local 
testing system never generates core files even though it claims to:


# Core dump will be written. Default location: Core dumps may be 
processed with "/usr/share/apport/apport %p %s %c" (or dumping to /

export/users/dh198349/valhalla/repos/valhalla-dev/open/test/jdk/core.29848)

apport isn't even installed, even though core_pattern lists it.

Cheers,
David

On 18/04/2018 4:36 PM, Yasumasa Suenaga wrote:

2018-04-18 15:05 GMT+09:00 Jini George :
Thank you very much, Yasumasa, for pointing this out. You are right 
-- this

would fail in the Linux systems if systemd-coredump is enabled.

I plan to file an enhancement request to address this issue (wrt
systemd-coredump) separately since this would apply to other coredump
generating test cases also like:
test/hotspot/jtreg/compiler/ciReplay/TestSAServer.java.


I agree with you, but...


 From what i can gather, i think we might be able to at least partially
address this by using

coredumptl -o  dump 

in the test cases, provided the kernel.core_pattern variable is not 
set to

"|/bin/false".

Let me know if you are not OK with this.


IMHO it is not good.
Some Linux distros use other coredump collector. For example, RHEL 6
uses ABRT, Ubuntu uses Apport, Fedora uses systemd-coredump.
Hence I think we should disable all tests which requires core images
for Linux like a Windows platform.


Thanks,

Yasumasa



Thank you,
Jini.




On 4/14/2018 7:39 PM, Yasumasa Suenaga wrote:


Hi Jini,

ClhsdbCDSCore.java:
    Can this test work on modern Linux?
    AFAIK modern Linux contains systemd-coredump to gather core 
images. So

I concern ClhsdbCDSCore.java fails in the future.


Thanks,

Yasumasa


On 2018/04/12 13:21, Jini George wrote:


Ping: Gentle reminder !

Thanks,
Jini.

On 4/6/2018 9:51 PM, Jini George wrote:


Hello!

Requesting reviews for: 
https://bugs.openjdk.java.net/browse/JDK-8174994

Webrev: http://cr.openjdk.java.net/~jgeorge/8174994/webrev.00/

While trying to identify the type given an address, a 
WrongTypeException
was getting thrown with various clhsdb commands (like printmdo, 
jstack,
etc). This was since SA tries to map an address to a hotspot C++ 
type by

comparing the vtable address to the vtable address values of known
types. With CDS, since the vtables are copied over for the Metadata
classes, the vtable addresses themselves don't match (though, of 
course,

the contents will), and SA errors out.

The fix has been implemented by making changes to read in the md 
region

(consisting of the c++ vtables) of the CDS archive in SA, and mapping
the vtable addresses to the corresponding metadata type 
(ConstantPool,

InstanceKlass, InstanceClassLoaderKlass, InstanceMirrorKlass,
InstanceRefKlass, Method, ObjArrayKlass, TypeArrayKlass).

For corefiles, an additional modification has been done to have the
replicated FileMapHeader structure (from
src/hotspot/share/memory/filemap.hpp, which is replicated in SA in
ps_core.c), to be in sync with the corresponding definition in
src/hotspot/share/memory/filemap.hpp.

Test cases to test both live and corefile debugging are being 
added with

this. These and other SA tests pass on Mach5.

Thanks,
Jini.

Re: RFR: 8201409: JDWP debugger initialization hangs intermittently

[serguei.spit...@oracle.com]

  
  
Hi Andrew,
  
  Sorry, I did not reply earlier.
  The fix need more testing. We also have some important tests in
  closed.
  I'll run them but I'm a little bit busy at the moment.
  
  You have two reviews which is enough for push after testing.
  
  Thanks,
  Serguei
  
  
  On 4/18/18 08:23, Andrew Leonard wrote:


  
  Hi Serguei,
  
  Do you need me
to try anything else for this review?
hotspot/jtreg/serviceability suite
run successfully.
  
  Many Thanks
  
  Andrew
  
  
  Andrew Leonard
Java Runtimes Development
IBM Hursley
IBM United Kingdom Ltd
Phone internal: 245913, external: 01962 815913
internet email: andrew_m_leon...@uk.ibm.com 
  
  
  
  
  
  From:
       "serguei.spit...@oracle.com"

  
  To:
       daniel.daughe...@oracle.com,
Andrew Leonard 
  
  Cc:
       serviceability-dev@openjdk.java.net
  
  Date:
       16/04/2018
07:10
  
  Subject:
       Re:
RFR: 8201409: JDWP debugger initialization hangs intermittently
  
  
  
  
  
  On 4/15/18 10:01, Daniel D. Daugherty
wrote:
  
  On 4/13/18 3:07 PM, serguei.spit...@oracle.com
wrote:
  
  Andrew and reviewers,

I'm re-sending this RFR with a corrected subject that includes
the bug
number.

The issues is:
  https://bugs.openjdk.java.net/browse/JDK-8201409

Webrev:
  http://cr.openjdk.java.net/~sspitsyn/webrevs/2018/8201409-jdwp-initsync.ibm.1/
  
   
  src/jdk.jdwp.agent/share/native/libjdwp/debugInit.c
      No comments.
  
  src/jdk.jdwp.agent/share/native/libjdwp/debugInit.h
      No comments.
  
  src/jdk.jdwp.agent/share/native/libjdwp/debugLoop.c
      So now pauses in debugLoop_run() before the loop
      that reads cmds. Looks good.
  
  src/jdk.jdwp.agent/share/native/libjdwp/eventHelper.c
      So the VM_INIT event handler now signals that we have
      received the VM_INIT event so that allows debugLoop_run()
      to proceed.
  
  Serguei, this fix needs to have the most of the Serviceability
  stack of tests run against it (jdwp, JVM/TI, JDI and jdb
  tests).
  Based on the email thread, I can't tell which tests have been
  run with the fix in place.
  
  
Hi Dan,

I'm going to sponsor this fix and will run all the debugger
tests.
Sorry that I did not announce it yet.

Thanks,
Serguei
  
  
  
  Dan

  
  

The fix looks good to me.
Also, I've agreed to skip a unit test as creating it for this
issue is
not easy.

At least, one more review is needed before the fix can be
pushed.

Thanks,
Serguei


On 4/11/18 06:33, Andrew Leonard wrote:
  
  Hi Serguei, 
Thank you for raising the bug. 
I had a chat with one of my colleagues who could recreate it,
and it's
probably related to the handshaking that is done in the
particular scenario.
So with the JCK harness: 

com.sun.jck.lib.ExecJCKTestOtherJVMCmd LD_LIBRARY_PATH=/javatest/lib/jck/jck8b/natives/linux_x86-64
/projects/jck/jdwp/j2sdk-image/bin/java
-Xdump:system:none -Xdump:system:events=gpf+abort+traceassert+corruptcache
-Xdump:snap:none -Xdump:snap:events=gpf+abort+traceassert+corruptcache
-Xdump:java:none -Xdump:java:events=gpf+abort+traceassert+corruptcache
-Xdump:heap:none -Xdump:heap:events=gpf+abort+traceassert+corruptcache
-Xfuture
-agentlib:jdwp=server=y,transport=dt_socket,address=localhost:35000,suspend=y
-classpath /javatest/lib/jck/JCK8b-b03/JCK-runtime-8b/classes
-Djava.security.policy=/javatest/lib/jck/JCK8b-b03/JCK-runtime-8b/lib/jck.policy
javasoft.sqe.jck.lib.jpda.jdwp.DebuggeeLoader -waittime=600
-msgSwitch=ub1604x64vm10:38636
-componentName=ArrayReference.GetValues.getvalues002 

Note that the JCK test harness starts the target process,
attaches to it,
and sends the resume command 
in a very short time with no handshaking. 

That may not help..but hopefully helps explain things a bit?
It's the timing
of the resume command during the test that is crucial, resuming
before
the VM initialization is complete will trigger it. 

   

Re: RFR: 8201409: JDWP debugger initialization hangs intermittently

[Andrew Leonard]

Hi Serguei,
Do you need me to try anything else for this review? 
hotspot/jtreg/serviceability suite run successfully.
Many Thanks
Andrew

Andrew Leonard
Java Runtimes Development
IBM Hursley
IBM United Kingdom Ltd
Phone internal: 245913, external: 01962 815913
internet email: andrew_m_leon...@uk.ibm.com 




From:   "serguei.spit...@oracle.com" 
To: daniel.daughe...@oracle.com, Andrew Leonard 

Cc: serviceability-dev@openjdk.java.net
Date:   16/04/2018 07:10
Subject:Re: RFR: 8201409: JDWP debugger initialization hangs 
intermittently



On 4/15/18 10:01, Daniel D. Daugherty wrote:
On 4/13/18 3:07 PM, serguei.spit...@oracle.com wrote:
Andrew and reviewers,

I'm re-sending this RFR with a corrected subject that includes the bug 
number.

The issues is:
  https://bugs.openjdk.java.net/browse/JDK-8201409

Webrev:
  
http://cr.openjdk.java.net/~sspitsyn/webrevs/2018/8201409-jdwp-initsync.ibm.1/
 
src/jdk.jdwp.agent/share/native/libjdwp/debugInit.c
No comments.

src/jdk.jdwp.agent/share/native/libjdwp/debugInit.h
No comments.

src/jdk.jdwp.agent/share/native/libjdwp/debugLoop.c
So now pauses in debugLoop_run() before the loop
that reads cmds. Looks good.

src/jdk.jdwp.agent/share/native/libjdwp/eventHelper.c
So the VM_INIT event handler now signals that we have
received the VM_INIT event so that allows debugLoop_run()
to proceed.

Serguei, this fix needs to have the most of the Serviceability
stack of tests run against it (jdwp, JVM/TI, JDI and jdb tests).
Based on the email thread, I can't tell which tests have been
run with the fix in place.

Hi Dan,

I'm going to sponsor this fix and will run all the debugger tests.
Sorry that I did not announce it yet.

Thanks,
Serguei


Dan



The fix looks good to me.
Also, I've agreed to skip a unit test as creating it for this issue is not 
easy.

At least, one more review is needed before the fix can be pushed.

Thanks,
Serguei


On 4/11/18 06:33, Andrew Leonard wrote:
Hi Serguei, 
Thank you for raising the bug. 
I had a chat with one of my colleagues who could recreate it, and it's 
probably related to the handshaking that is done in the particular 
scenario. So with the JCK harness: 

com.sun.jck.lib.ExecJCKTestOtherJVMCmd LD_LIBRARY_PATH=/javatest/lib/jck
/jck8b/natives/linux_x86-64 /projects/jck/jdwp/j2sdk-image/bin/java 
-Xdump:system:none -Xdump:system:events=gpf+abort+traceassert+corruptcache 
-Xdump:snap:none -Xdump:snap:events=gpf+abort+traceassert+corruptcache 
-Xdump:java:none -Xdump:java:events=gpf+abort+traceassert+corruptcache 
-Xdump:heap:none -Xdump:heap:events=gpf+abort+traceassert+corruptcache -
Xfuture -agentlib:jdwp=server=y,transport=dt_socket,address=localhost
:35000,suspend=y -classpath /javatest/lib/jck
/JCK8b-b03/JCK-runtime-8b/classes -Djava.security.policy=/javatest/lib/jck
/JCK8b-b03/JCK-runtime-8b/lib/jck.policy 
javasoft.sqe.jck.lib.jpda.jdwp.DebuggeeLoader -waittime=600 
-msgSwitch=ub1604x64vm10:38636 -componentName=
ArrayReference.GetValues.getvalues002 

Note that the JCK test harness starts the target process, attaches to it, 
and sends the resume command 
in a very short time with no handshaking. 

That may not help..but hopefully helps explain things a bit? It's the 
timing of the resume command during the test that is crucial, resuming 
before the VM initialization is complete will trigger it. 

Thanks 
Andrew 

Andrew Leonard
Java Runtimes Development
IBM Hursley
IBM United Kingdom Ltd
Phone internal: 245913, external: 01962 815913
internet email: andrew_m_leon...@uk.ibm.com 




From:"serguei.spit...@oracle.com"  
To:Andrew Leonard  
Cc:serviceability-dev@openjdk.java.net 
Date:11/04/2018 09:57 
Subject:Re: RFR: Fix race condition in jdwp 



Hi Andrew,

I've filed the bug:
  https://bugs.openjdk.java.net/browse/JDK-8201409

Also, this is a webrev with your patch:
  
http://cr.openjdk.java.net/~sspitsyn/webrevs/2018/8201409-jdwp-initsync.ibm.1/


I agree that creating a standalone test is tricky here.

I've added usleep(1) into the eventHelper_reportVMInit()
and ran the JTreg com/sun/jdi tests with my JDK build.
However, none of the tests failed with the failure mode you described.
So that I'm puzzled a little bit.
I suspect that some specific debugLoop commands were used in your 
scenario.

It is still possible that I've missed something here.
Will try to double check everything.

Thanks,
Serguei


On 4/11/18 01:29, Andrew Leonard wrote: 
Thanks Serguei, 
I terms of a standalone testcase it is quite tricky, as due to the nature 
of the issue which took a lot of investigation to solve it's very timing 
dependent and will only occur randomly. It can be forced as I indicated 
below by adding a "sleep" in the VMInit report code but that's not a 
testcase, however the issue was originally found in our JCK testing for 
IBMJava8, 

Re: RFR: 8196325: GarbageCollectionNotificationInfo has same information for before and after

[mandy chung]

Hi Sangheon,


On 4/18/18 12:41 PM, sangheon.kim wrote:


CR: https://bugs.openjdk.java.net/browse/JDK-8196325
webrev: http://cr.openjdk.java.net/~sangheki/8196325/webrev.0/


This is indeed a regression.  GcInfoBuilder depends on the order of the 
pool name array.


The change looks okay.  I would suggest to use stream in the new 
getAllMemoryPoolNames() like this:


    public static String[] getAllMemoryPoolNames() {
    return Arrays.stream(MemoryImpl.getMemoryPools())
          .map(MemoryPoolMXBean::getName)
 .toArray(String[]::new);
    }

Testing: jdk-tier1,jdk-tier2,jdk-tier3,hs-tier1,hs-tier2,builds-tier1, 
jdk_management, jdk_jmx




These test groups are good.

Mandy


Thanks,
Sangheon

Re: Review Request JDK-8200559: Java agents doing instrumentation need a means to define auxiliary classes

[mandy chung]

Hi Rafael,

I think it's best to separate the testing requirement from java agents 
doing instrumentation that may run in production environment.


I have created a JBS issue to track the testing mode idea that would 
require more discussion and investigation:

  https://bugs.openjdk.java.net/browse/JDK-8201562

I understand it's not as efficient to inject a class in a package 
different than the package of the transformed class.  With the principle 
of least privilege, I prefer not to provide an API to inject any class 
in any package and you can achieve by calling retransformClasses.


What do you think?

Mandy

On 4/18/18 5:23 AM, Rafael Winterhalter wrote:

Hei Mandy,
Lookup::defineClass would always be an alternative but it would 
require me to open the class first. If the instrumented type can read 
the module with the callback but its module was not opened, this would 
not help me much, unfortunately. Also, I could not resolve this lookup 
as the class in question is not necessarily loaded at this point.

Best regards, Rafael

2018-04-17 9:28 GMT+02:00 mandy chung >:


Hi Rafael,

I see that mocking/proxying/testing framework should be looked at
separately since its requirements and approaches can be different
than tool agents.

On 4/17/18 5:06 AM, Rafael Winterhalter wrote:

Hei Mandy,

I have looked into several Java agents that I have worked on and
for many of them, this API does unfortunately not supply
sufficient access. I would therefore still prefer a method
Instrumentation::defineClass.

The problem is that some agents need to define classes in other
packages then in that of the instrumented class. For example, I
might need to enhance a library that defines a set of callback
classes in package A. All these classes share a common super
class with a package-private constructor. I want to instrument
some class in package B to use a callback that the library does
not supply and need to add a new callback class to A. This is not
possible using the current API.



Are these callback classes made available statically?  or just
dynamically defining additional class as needed?  Is
Lookup::defineClass an alternative if you get a hold of common
super class in A?


I could however achieve do so by calling
Instrumentation::retransform on one of the classes in A after
registering a class file transformer. Once the retransformation
is triggered, I can now define a class in A. Of course this is
inefficient and I would rather open the jdk.internal.misc module
and use the "old" API instead.

For this reason, I argue that this rather restrained API is not
convenient while it does not add anything to security. Also, for
the use case of Mockito, this would neither be sufficient as
Mockito sometimes redefines classes and sometimes adds a subclass
without retransforming. We would rather have direct access to
class definition once we are already running with the privileges
of a Java agent.

I would therefore suggest to add a method:

interface Instrumentation {
  Class defineClass(byte[] bytes, ProtectionDomain pd);
}

which can be implemented simply by delegating to
jdk.internal.misc.Unsafe.

On a side note. Does JavaLangAccess::defineClass work with the
bootstrap class loader? I have not tried it but I always thought
it was just an access layer for the class loader API that cannot
access the null value.



The JVM entry point does allow null loader.

Mandy



Thanks for considering this use case!
Best regards, Rafael

2018-04-15 8:23 GMT+02:00 mandy chung >:

Background:

Java agents support both load time and dynamic
instrumentation.   At load time,
the agent's ClassFileTransformer is invoked to transform
class bytes.  There is
no Class objects at this time.  Dynamic instrumentation is
when redefineClasses
or retransformClasses is used to redefine an existing loaded
class.  The
ClassFileTransformer is invoked with class bytes where the
Class object is present.

Java agent doing instrumentation needs a means to define
auxiliary classes
that are visible and accessible to the instrumented class. 
Existing agents
have been using sun.misc.Unsafe::defineClass to define aux
classes directly
or accessing protected ClassLoader::defineClass method with
setAccessible to
suppress the language access check (see [1] where this issue
was brought up).

Instrumentation::appendToBootstrapClassLoaderSearch and
appendToSystemClassLoaderSearch
APIs are existing means to supply additional classes.  It's
too limited
for 

Re: RFR: JDK-8174994: SA: clhsdb printmdo throws WrongTypeException when attached to a process with CDS

[David Holmes]

My 2c ...

We have to have tests that can test core file attaching capability - 
else we don't know it works. So we have to try and generate a core file.


But, we have to expect that in many cases no core file will be generated 
even if the hs-err file claims it was. For example my primary local 
testing system never generates core files even though it claims to:


# Core dump will be written. Default location: Core dumps may be 
processed with "/usr/share/apport/apport %p %s %c" (or dumping to /

export/users/dh198349/valhalla/repos/valhalla-dev/open/test/jdk/core.29848)

apport isn't even installed, even though core_pattern lists it.

Cheers,
David

On 18/04/2018 4:36 PM, Yasumasa Suenaga wrote:

2018-04-18 15:05 GMT+09:00 Jini George :

Thank you very much, Yasumasa, for pointing this out. You are right -- this
would fail in the Linux systems if systemd-coredump is enabled.

I plan to file an enhancement request to address this issue (wrt
systemd-coredump) separately since this would apply to other coredump
generating test cases also like:
test/hotspot/jtreg/compiler/ciReplay/TestSAServer.java.


I agree with you, but...


 From what i can gather, i think we might be able to at least partially
address this by using

coredumptl -o  dump 

in the test cases, provided the kernel.core_pattern variable is not set to
"|/bin/false".

Let me know if you are not OK with this.


IMHO it is not good.
Some Linux distros use other coredump collector. For example, RHEL 6
uses ABRT, Ubuntu uses Apport, Fedora uses systemd-coredump.
Hence I think we should disable all tests which requires core images
for Linux like a Windows platform.


Thanks,

Yasumasa



Thank you,
Jini.




On 4/14/2018 7:39 PM, Yasumasa Suenaga wrote:


Hi Jini,

ClhsdbCDSCore.java:
Can this test work on modern Linux?
AFAIK modern Linux contains systemd-coredump to gather core images. So
I concern ClhsdbCDSCore.java fails in the future.


Thanks,

Yasumasa


On 2018/04/12 13:21, Jini George wrote:


Ping: Gentle reminder !

Thanks,
Jini.

On 4/6/2018 9:51 PM, Jini George wrote:


Hello!

Requesting reviews for: https://bugs.openjdk.java.net/browse/JDK-8174994
Webrev: http://cr.openjdk.java.net/~jgeorge/8174994/webrev.00/

While trying to identify the type given an address, a WrongTypeException
was getting thrown with various clhsdb commands (like printmdo, jstack,
etc). This was since SA tries to map an address to a hotspot C++ type by
comparing the vtable address to the vtable address values of known
types. With CDS, since the vtables are copied over for the Metadata
classes, the vtable addresses themselves don't match (though, of course,
the contents will), and SA errors out.

The fix has been implemented by making changes to read in the md region
(consisting of the c++ vtables) of the CDS archive in SA, and mapping
the vtable addresses to the corresponding metadata type (ConstantPool,
InstanceKlass, InstanceClassLoaderKlass, InstanceMirrorKlass,
InstanceRefKlass, Method, ObjArrayKlass, TypeArrayKlass).

For corefiles, an additional modification has been done to have the
replicated FileMapHeader structure (from
src/hotspot/share/memory/filemap.hpp, which is replicated in SA in
ps_core.c), to be in sync with the corresponding definition in
src/hotspot/share/memory/filemap.hpp.

Test cases to test both live and corefile debugging are being added with
this. These and other SA tests pass on Mach5.

Thanks,
Jini.

Re: RFR: JDK-8174994: SA: clhsdb printmdo throws WrongTypeException when attached to a process with CDS

[Yasumasa Suenaga]

2018-04-18 15:05 GMT+09:00 Jini George :
> Thank you very much, Yasumasa, for pointing this out. You are right -- this
> would fail in the Linux systems if systemd-coredump is enabled.
>
> I plan to file an enhancement request to address this issue (wrt
> systemd-coredump) separately since this would apply to other coredump
> generating test cases also like:
> test/hotspot/jtreg/compiler/ciReplay/TestSAServer.java.

I agree with you, but...

> From what i can gather, i think we might be able to at least partially
> address this by using
>
> coredumptl -o  dump 
>
> in the test cases, provided the kernel.core_pattern variable is not set to
> "|/bin/false".
>
> Let me know if you are not OK with this.

IMHO it is not good.
Some Linux distros use other coredump collector. For example, RHEL 6
uses ABRT, Ubuntu uses Apport, Fedora uses systemd-coredump.
Hence I think we should disable all tests which requires core images
for Linux like a Windows platform.


Thanks,

Yasumasa


> Thank you,
> Jini.
>
>
>
>
> On 4/14/2018 7:39 PM, Yasumasa Suenaga wrote:
>>
>> Hi Jini,
>>
>> ClhsdbCDSCore.java:
>>Can this test work on modern Linux?
>>AFAIK modern Linux contains systemd-coredump to gather core images. So
>> I concern ClhsdbCDSCore.java fails in the future.
>>
>>
>> Thanks,
>>
>> Yasumasa
>>
>>
>> On 2018/04/12 13:21, Jini George wrote:
>>>
>>> Ping: Gentle reminder !
>>>
>>> Thanks,
>>> Jini.
>>>
>>> On 4/6/2018 9:51 PM, Jini George wrote:

 Hello!

 Requesting reviews for: https://bugs.openjdk.java.net/browse/JDK-8174994
 Webrev: http://cr.openjdk.java.net/~jgeorge/8174994/webrev.00/

 While trying to identify the type given an address, a WrongTypeException
 was getting thrown with various clhsdb commands (like printmdo, jstack,
 etc). This was since SA tries to map an address to a hotspot C++ type by
 comparing the vtable address to the vtable address values of known
 types. With CDS, since the vtables are copied over for the Metadata
 classes, the vtable addresses themselves don't match (though, of course,
 the contents will), and SA errors out.

 The fix has been implemented by making changes to read in the md region
 (consisting of the c++ vtables) of the CDS archive in SA, and mapping
 the vtable addresses to the corresponding metadata type (ConstantPool,
 InstanceKlass, InstanceClassLoaderKlass, InstanceMirrorKlass,
 InstanceRefKlass, Method, ObjArrayKlass, TypeArrayKlass).

 For corefiles, an additional modification has been done to have the
 replicated FileMapHeader structure (from
 src/hotspot/share/memory/filemap.hpp, which is replicated in SA in
 ps_core.c), to be in sync with the corresponding definition in
 src/hotspot/share/memory/filemap.hpp.

 Test cases to test both live and corefile debugging are being added with
 this. These and other SA tests pass on Mach5.

 Thanks,
 Jini.
>>>
>>>
>

Re: RFR: JDK-8174994: SA: clhsdb printmdo throws WrongTypeException when attached to a process with CDS

[Jini George]

Thank you very much, Yasumasa, for pointing this out. You are right -- 
this would fail in the Linux systems if systemd-coredump is enabled.


I plan to file an enhancement request to address this issue (wrt 
systemd-coredump) separately since this would apply to other coredump 
generating test cases also like:

test/hotspot/jtreg/compiler/ciReplay/TestSAServer.java.

From what i can gather, i think we might be able to at least partially 
address this by using


coredumptl -o  dump 

in the test cases, provided the kernel.core_pattern variable is not set 
to "|/bin/false".


Let me know if you are not OK with this.

Thank you,
Jini.



On 4/14/2018 7:39 PM, Yasumasa Suenaga wrote:

Hi Jini,

ClhsdbCDSCore.java:
   Can this test work on modern Linux?
   AFAIK modern Linux contains systemd-coredump to gather core images. 
So I concern ClhsdbCDSCore.java fails in the future.



Thanks,

Yasumasa


On 2018/04/12 13:21, Jini George wrote:

Ping: Gentle reminder !

Thanks,
Jini.

On 4/6/2018 9:51 PM, Jini George wrote:

Hello!

Requesting reviews for: https://bugs.openjdk.java.net/browse/JDK-8174994
Webrev: http://cr.openjdk.java.net/~jgeorge/8174994/webrev.00/

While trying to identify the type given an address, a WrongTypeException
was getting thrown with various clhsdb commands (like printmdo, jstack,
etc). This was since SA tries to map an address to a hotspot C++ type by
comparing the vtable address to the vtable address values of known
types. With CDS, since the vtables are copied over for the Metadata
classes, the vtable addresses themselves don't match (though, of course,
the contents will), and SA errors out.

The fix has been implemented by making changes to read in the md region
(consisting of the c++ vtables) of the CDS archive in SA, and mapping
the vtable addresses to the corresponding metadata type (ConstantPool,
InstanceKlass, InstanceClassLoaderKlass, InstanceMirrorKlass,
InstanceRefKlass, Method, ObjArrayKlass, TypeArrayKlass).

For corefiles, an additional modification has been done to have the
replicated FileMapHeader structure (from
src/hotspot/share/memory/filemap.hpp, which is replicated in SA in
ps_core.c), to be in sync with the corresponding definition in
src/hotspot/share/memory/filemap.hpp.

Test cases to test both live and corefile debugging are being added with
this. These and other SA tests pass on Mach5.

Thanks,
Jini.