Isn’t redefining (when you start with a fresh byte code copy) different from
retransforming where you are depending on the VM to provide you with the byte
code through the class_file_load_hook? I think what we have here is the latter
and that no class_file_load_hooks are executed for shared classes? I need to
dig deep into what capabilities the j.l.instrument native agent is actually
asking for.
/Staffan
On 4 dec 2013, at 19:57, Daniel D. Daugherty <daniel.daughe...@oracle.com>
wrote:
> > Could you please help us with this? Is it a legal scenario to redefine
> > shared classes?
>
> Sorry for the delay in responding. I was on vacation.
>
> Yes, redefining a shared class is supported via either JVM/TI or
> via java.lang.instrument. There should be existing tests that
> verify this behavior, but you may have discovered a corner case
> by accident.
>
> Dan
>
>
> On 11/27/13 8:13 AM, Leonid Mesnik wrote:
>> I think test should test instrumentation of custom, JDK, JDK shared classes
>> as a part of scenario.
>> This is how tools could use this mechanism. May be I am wrong.
>>
>> Also I found
>> JDK-5002268 Allow class sharing use with RedefineClasses
>> which allow to redefine of classes when class sharing is used. I don't
>> exactly know should it work
>> for j.l.instrument as well as for JVMTI
>> Dan
>> Could you please help us with this? Is it a legal scenario to redefine
>> shared classes?
>>
>> Leonid
>>
>> On 11/27/2013 03:46 PM, Mattias Tobiasson wrote:
>>> According to the test documentation and bug references the test verifies
>>> the manifest attribute "Can-Redefine-Classes".
>>> The test does not mention shared class archive or -Xshare.
>>> But maybe the test has found a problem accidentally...
>>>
>>> Mattias
>>>
>>> ----- Original Message -----
>>> From: david.hol...@oracle.com
>>> To: mattias.tobias...@oracle.com
>>> Cc: serviceability-dev@openjdk.java.net
>>> Sent: Wednesday, November 27, 2013 12:21:50 PM GMT +01:00 Amsterdam /
>>> Berlin / Bern / Rome / Stockholm / Vienna
>>> Subject: Re: RFR (S): 6461635: [TESTBUG] BasicTests.sh test fails
>>> intermittently
>>>
>>> On 27/11/2013 8:46 PM, Mattias Tobiasson wrote:
>>>> Hi, I now have a reproducer for this test that fails every time.
>>>> I just need to verify that it really is a test bug and not a product bug.
>>>>
>>>> This is a summary of the test:
>>>> 1. It loads a javaagent jar that implements ClassFileTransformer.
>>>> 2. The agent loads class java.math.BigInteger and expects a callback to
>>>> ClassFileTransformer.transform(). But it sometimes does not get that
>>>> callback.
>>>>
>>>> I believe the reason for the intermittent failures are that the jvm flag
>>>> -Xshare have different default values on different servers.
>>> -Xshare:auto will default to off for server compiler, but on for client
>>>
>>>> When the reproducer is run with flag "-Xshare:off" it passes every time.
>>>> When run with "-Xshare:auto" it fails every time.
>>>> When the reproducer is changed to use a dummy class (RedefineDummy)
>>>> instead of BigInteger, then the test passes every time no matter what
>>>> value -Xshare has.
>>>>
>>>> My proposed fix for the bug is to use a dummy class instead of BigInteger.
>>>> I just need to verify that it is ok that ClassFileTransformer.transform()
>>>> is NOT called for BigInteger when -Xshare is enabled.
>>> If the whole point of the test is to transform a file from the shared
>>> archive then changing to a class not in the archive will defeat that. So
>>> you need to verify what the intent of the test is.
>>>
>>> David
>>>
>>>> The reproducer is shown below. It loads the javaagent into its own vm so I
>>>> don't need to set up multiple jvms.
>>>>
>>>> public class TestRedefine implements ClassFileTransformer {
>>>>
>>>> public static void agentmain(String agentArgs, Instrumentation inst)
>>>> throws Throwable {
>>>> try {
>>>> inst.addTransformer(new TestRedefine());
>>>>
>>>> ClassLoader.getSystemClassLoader().loadClass("java.math.BigInteger");
>>>> if (!isTransformCalled) {
>>>> throw new Exception("transform() NOT called for " +
>>>> targetName);
>>>> }
>>>> } catch (Throwable t) {
>>>> t.printStackTrace();
>>>> throw t;
>>>> }
>>>> }
>>>>
>>>> public byte[] transform(ClassLoader loader,
>>>> String className,
>>>> Class<?> classBeingRedefined,
>>>> ProtectionDomain protectionDomain,
>>>> byte[] classfileBuffer) {
>>>> System.out.println("transform called: " + className);
>>>> if (className.equals("java/math/BigInteger")) {
>>>> isTransformCalled = true;
>>>> }
>>>> return null;
>>>> }
>>>>
>>>> public static void main(String args[]) throws Exception {
>>>> String pid = getProcessId();
>>>> try {
>>>> VirtualMachine vm = VirtualMachine.attach(pid);
>>>> vm.loadAgent("TestRedefine.jar");
>>>> } catch (Exception e) {
>>>> e.printStackTrace();
>>>> throw e;
>>>> }
>>>> }
>>>>
>>>> ... more code ...
>>>> }
>>>>
>>>>
>>>>
>>>>
>>>>
>>>> ----- Original Message -----
>>>> From: leonid.mes...@oracle.com
>>>> To: mattias.tobias...@oracle.com
>>>> Cc: serviceability-dev@openjdk.java.net
>>>> Sent: Wednesday, November 27, 2013 9:19:37 AM GMT +01:00 Amsterdam /
>>>> Berlin / Bern / Rome / Stockholm / Vienna
>>>> Subject: Re: RFR (S): 6461635: [TESTBUG] BasicTests.sh test fails
>>>> intermittently
>>>>
>>>> Hi
>>>>
>>>> Generally I am ok with your comments and fix. However you still needed
>>>> to get official review.
>>>>
>>>> The only question is about failure related from retransformation of
>>>> j.l.BigInteger with CDS.
>>>> Could it be JDK issue? In this case it would be better to file it as a
>>>> part of fix.
>>>> I understand that using of CDS and instrumentation of JDK classes is not
>>>> related with this test
>>>> and your fix make it cleaner. However could you please verify that we
>>>> don't have JDK issue here.
>>>>
>>>> see other comments inline
>>>>
>>>> On 11/26/2013 08:35 PM, Mattias Tobiasson wrote:
>>>>> Hi,
>>>>> Thanks for the review. I have summarized the questions and answers:
>>>>>
>>>>> Q: What is the reason of this intermittent failure?
>>>>> A: The test expects the callback ClassFileTransformer.transform() to be
>>>>> called when it loads a new class. That function does not seem to be
>>>>> called when class sharing is enabled and a previous test has called
>>>>> "-Xshare:dump". I am not really sure how ClassFileTransformer works
>>>>> together with class sharing.
>>>>> I got the idea for -Xshare:dump from this bug:
>>>>> https://bugs.openjdk.java.net/browse/JDK-6571866
>>>>> I have verified on jprt that it fails without that flag. I have not been
>>>>> able to reproduce a failure when -Xshare:off is set.
>>>>>
>>>>> Having thought more about the problem, I would like to use another fix.
>>>>> The test currently uses java.lang.BigInteger for the transform test. I
>>>>> want to change that and instead use my own class (RedefineDummy.java).
>>>>> Since loading my own class is not affected by -Xshare, I do not need to
>>>>> set the flag.
>>>>> Q: You used "output" in finally which is not initialized properly in the
>>>>> case of Exception. Could we get another uncaught exception in finally?
>>>>> A: output is only sent to getProcessLog() to get a log message. That
>>>>> function can handle null values, which it just logs as "null".
>>>> Thank you for explanation.
>>>>> Q: In getCommandLine(), should we also trim cmd to remove last " "?
>>>>> A: Yes. I will fix that. I will also add a check if ProcessBuilder is
>>>>> null.
>>>> Ok
>>>>> Q: waitForJvmPid(String key) throws Throwable. Why throw throwable?
>>>>> A: I think there are so many things that can go wrong when starting a
>>>>> separate java process, that I do not want to check for expected errors. I
>>>>> also don't know how a test writer should handle different kind of
>>>>> exceptions from this function. Any exception is logged in the sub
>>>>> function.
>>>>> Q: Should tryFindJvmPid(String key) be private? Are we going to use it in
>>>>> tests or only waitForJvmPid?
>>>>> A: Yes, that function may also be used by tests. It is currently not used
>>>>> by any test, but it might be useful.
>>>> Ok
>>>>> Q: Timeouts in function waitForJvmPid(String key)?
>>>>> A: I definitely agree of the problem. The reason for not adding a timeout
>>>>> parameter to the function is that I do not want the tests to be
>>>>> responsible for setting hard coded timeouts. Timeouts should be handled
>>>>> by the framework. I know there are plans of adding better process
>>>>> handling to jtreg (with ProcessHandler). After that jtreg should be
>>>>> better at handling timeouts.
>>>>> I will add a flush() to stdout to make sure we log before we wait.
>>>> Hope we will update jtreg before get into this issue :)
>>>>
>>>> Leonid
>>>>> Mattias
>>>>>
>>>>> ----- Original Message -----
>>>>> From: leonid.mes...@oracle.com
>>>>> To: mattias.tobias...@oracle.com, serviceability-dev@openjdk.java.net
>>>>> Sent: Monday, November 25, 2013 6:26:27 PM GMT +01:00 Amsterdam / Berlin
>>>>> / Bern / Rome / Stockholm / Vienna
>>>>> Subject: Re: RFR (S): 6461635: [TESTBUG] BasicTests.sh test fails
>>>>> intermittently
>>>>>
>>>>> Hi
>>>>>
>>>>> I have a couple of high-level questions.
>>>>>
>>>>> On 11/25/2013 08:28 PM, Mattias Tobiasson wrote:
>>>>>> Hi,
>>>>>> The test has been updated after the first review.
>>>>>> The two java files for each test has been merged to a single file.
>>>>>>
>>>>>>
>>>>>> Updated summary of changes:
>>>>>> 1. The real test bug fix is to add flag "-Xshare:off" when starting the
>>>>>> "Application" instance. Without that flag, the test for
>>>>>> ClassFileTransformer in RedefineAgent.java fails intermittently. The
>>>>>> flag is added in function startApplication() in RunnerUtil.java.
>>>>> What is the reason of this intermittent failure?
>>>>>> 2. Removed all bash scripts.
>>>>> Great!
>>>>>> 3. Merged the setup bash script and the java test code for each test to
>>>>>> a single java file. The old java test code is unchanged, it has just
>>>>>> been moved to static nested class in the new java setup file.
>>>>>>
>>>>>> 4. Added some utility functions to jdk/testlibrary.
>>>>> Here some comments about library code.
>>>>> http://cr.openjdk.java.net/~miauno/6461635/webrev.01/jdk/test/lib/testlibrary/jdk/testlibrary/ProcessThread.java.udiff.html
>>>>>
>>>>> + try {
>>>>> + output = new OutputAnalyzer(this.process);
>>>>> + } catch (Throwable t) {
>>>>> + String name = Thread.currentThread().getName();
>>>>> + System.out.println(String.format("ProcessThread[%s]
>>>>> failed: %s", name, t.toString()));
>>>>> + throw t;
>>>>> + } finally {
>>>>> + String logMsg =
>>>>> ProcessTools.getProcessLog(processBuilder, output);
>>>>> + System.out.println(logMsg);
>>>>> + }
>>>>>
>>>>>
>>>>> You used output in finally which is not initialized properly in the case
>>>>> of Exception.
>>>>> Could we get another uncaught exception in finally?
>>>>>
>>>>> http://cr.openjdk.java.net/~miauno/6461635/webrev.01/jdk/test/lib/testlibrary/jdk/testlibrary/ProcessTools.java.udiff.html
>>>>>
>>>>> 1) Same in as previous in executeProcess method.
>>>>>
>>>>> 2) getCommandLine (...)
>>>>>
>>>>> + /**
>>>>> + * @return The full command line for the ProcessBuilder.
>>>>> + */
>>>>> + public static String getCommandLine(ProcessBuilder pb) {
>>>>> + StringBuilder cmd = new StringBuilder();
>>>>> + for (String s : pb.command()) {
>>>>> + cmd.append(s).append(" ");
>>>>> + }
>>>>> + return cmd.toString();
>>>>> + }
>>>>>
>>>>>
>>>>> Should we also trim cmd to remove last " "?
>>>>>
>>>>> http://cr.openjdk.java.net/~miauno/6461635/webrev.01/jdk/test/lib/testlibrary/jdk/testlibrary/Utils.java.udiff.html
>>>>>
>>>>> public static int waitForJvmPid(String key) throws Throwable {
>>>>>
>>>>>
>>>>> Why it throw Trowable? Could we deal with exceptions in this method and
>>>>> re-throw some more meaningful exception here?
>>>>> Could you force flush for out here:
>>>>>
>>>>> System.out.println("waitForJvmPid: Waiting for key '" + key + "'");
>>>>>
>>>>> Hope it should be enough. It is scary to investigate such "timeouts".
>>>>> Would it be better to add some
>>>>> internal timeout in testlibrary? Really jtreg doesn't kill all processes
>>>>> and we have alive java processes in such case.
>>>>> For samevm tests timeouts are even worse. There is no good way to kill
>>>>> test in samevm.
>>>>>
>>>>> Should be
>>>>>
>>>>> public static int tryFindJvmPid(String key) throws Throwable {
>>>>>
>>>>> private? Are we going to use it in tests or only waitForJvmPid?
>>>>>
>>>>> Leonid
>>>>>> 5. Moved exit code check from the common utility function in
>>>>>> ProcessThread.java to the test JstatdTest.java. The check is moved to
>>>>>> the test because other tests in the future may have other expected exit
>>>>>> codes.
>>>>>>
>>>>>>
>>>>>> Webrev:
>>>>>> http://cr.openjdk.java.net/~miauno/6461635/webrev.01/
>>>>>>
>>>>>> Bug:
>>>>>> https://bugs.openjdk.java.net/browse/JDK-6461635
>>>>>>
>>>>>> Mattias
>>>>>>
>>>>>>
>>>>>> ----- Original Message -----
>>>>>> From: mattias.tobias...@oracle.com
>>>>>> To: mikael.a...@oracle.com
>>>>>> Cc: serviceability-dev@openjdk.java.net, alan.bate...@oracle.com
>>>>>> Sent: Thursday, November 21, 2013 9:22:11 AM GMT +01:00 Amsterdam /
>>>>>> Berlin / Bern / Rome / Stockholm / Vienna
>>>>>> Subject: Re: RFR (S): 6461635: [TESTBUG] BasicTests.sh test fails
>>>>>> intermittently
>>>>>>
>>>>>> I agree that merging both files in a single file may be good.
>>>>>> The main reason why I did not do that is that I wanted to keep the
>>>>>> history of the existing test. If I copy the test to a new file, all
>>>>>> history of the code is lost.
>>>>>>
>>>>>> But this test bug was reported 8 years ago, and I don't believe much has
>>>>>> changed in the code since then. So keeping the history may not be that
>>>>>> important.
>>>>>>
>>>>>> Mattias
>>>>>>
>>>>>> ----- Original Message -----
>>>>>> From: mikael.a...@oracle.com
>>>>>> To: mattias.tobias...@oracle.com, alan.bate...@oracle.com
>>>>>> Cc: serviceability-dev@openjdk.java.net
>>>>>> Sent: Wednesday, November 20, 2013 4:11:45 PM GMT +01:00 Amsterdam /
>>>>>> Berlin / Bern / Rome / Stockholm / Vienna
>>>>>> Subject: Re: RFR (S): 6461635: [TESTBUG] BasicTests.sh test fails
>>>>>> intermittently
>>>>>>
>>>>>> How about defining the class that you want to attach to as a static
>>>>>> inner class to the actual test? That would give you only one file, but
>>>>>> with two classes, each with its own main method and clear correlation
>>>>>> between them.
>>>>>>
>>>>>> Mikael
>>>>>>
>>>>>> On 2013-11-20 15:41, Mattias Tobiasson wrote:
>>>>>>> Hi,
>>>>>>> Each test requires 2 files, the actual test code and a helper file.
>>>>>>>
>>>>>>> The helper file will launch a separate java process (called
>>>>>>> Application), and then start the actual test. The actual test will then
>>>>>>> attach to the Application.
>>>>>>>
>>>>>>> For example:
>>>>>>> PermissionTests.sh: Helper file that will launch Application instance
>>>>>>> and then start PermissionTest.java
>>>>>>> PermissionTest.java: The actual test code that attaches to the
>>>>>>> Application.
>>>>>>>
>>>>>>> It is the PermissionTests.sh that is started by jtreg (contains the
>>>>>>> "@test"-tag).
>>>>>>>
>>>>>>> When I port PermissionTest.sh to java I would get 2 files called
>>>>>>> PermissionTest.java, so some name change is required.
>>>>>>>
>>>>>>> I could have kept the old PermissionTest.java unchanged, but then I
>>>>>>> would need another name for the prevoius PermissionTest.sh. And I
>>>>>>> wanted a "clean" Test name for the file containing the "@test"-tag.
>>>>>>>
>>>>>>> I used these names:
>>>>>>> TestPermission.java: Helper file.
>>>>>>> TestPermissionImpl.java: Actual test code.
>>>>>>>
>>>>>>> I am still new to adding tests for open jdk. I am happy to change the
>>>>>>> names if they do not follow the naming convention.
>>>>>>>
>>>>>>> Mattias
>>>>>>>
>>>>>>>
>>>>>>> ----- Original Message -----
>>>>>>> From: alan.bate...@oracle.com
>>>>>>> To: mattias.tobias...@oracle.com
>>>>>>> Cc: serviceability-dev@openjdk.java.net
>>>>>>> Sent: Wednesday, November 20, 2013 2:50:52 PM GMT +01:00 Amsterdam /
>>>>>>> Berlin / Bern / Rome / Stockholm / Vienna
>>>>>>> Subject: Re: RFR (S): 6461635: [TESTBUG] BasicTests.sh test fails
>>>>>>> intermittently
>>>>>>>
>>>>>>>
>>>>>>> Out of curiosity, what is the reason for the rename? I ask because we
>>>>>>> use Basic and similar names in many areas. Also anything in the test
>>>>>>> tree should be a test.
>>>>>>>
>>>>>>> -Alan.
>>>>>>>
>>>>>>> On 20/11/2013 13:47, Mattias Tobiasson wrote:
>>>>>>>> Hi,
>>>>>>>> Could you please review this fix.
>>>>>>>>
>>>>>>>> Summary of changes:
>>>>>>>>
>>>>>>>> 1. The real test bug fix is to add flag "-Xshare:off" when starting
>>>>>>>> the "Application" instance. Without that flag, the test for
>>>>>>>> ClassFileTransformer in RedefineAgent.java fails intermittently. The
>>>>>>>> flag is added in function startApplication() in RunnerUtil.java.
>>>>>>>>
>>>>>>>> 2. Ported the following bash scripts to java:
>>>>>>>> BasicTests.sh -> TestBasic.java
>>>>>>>> PermissionTests.sh -> TestPermission.java
>>>>>>>> ProviderTests.sh -> TestProvider.java
>>>>>>>>
>>>>>>>> 3. Renamed the java test code to avoid name clash with new java
>>>>>>>> classes ported from bash script:
>>>>>>>> BasicsTest.java -> TestBasicImpl.java
>>>>>>>> PermissionTest.java -> TestPermissionImpl.java
>>>>>>>> ProviderTest.java -> TestProviderImpl.java
>>>>>>>>
>>>>>>>> 4. Added some utility functions to jdk/testlibrary.
>>>>>>>>
>>>>>>>> 5. Moved exit code check from the common utility function in
>>>>>>>> ProcessThread.java to the test JstatdTest.java. The check is moved to
>>>>>>>> the test because other tests in the future may have other expected
>>>>>>>> exit codes.
>>>>>>>>
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> Mattias
>>>>>>>>
>>>>>>>> Webrev:
>>>>>>>> http://cr.openjdk.java.net/~miauno/6461635/webrev.00/
>>>>>>>>
>>>>>>>> Bug:
>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-6461635
>>
>>
>> --
>> Leonid Mesnik
>> JVM SQE
>
