RFR (XXS) 8026808: serviceability/sa/jmap-hprof/JMapHProfLargeHeapTest.java failed with unexpected exit value

[David Holmes]

Hi Fredrik,

After our email side-bar it seems to me that the bug here is in the 
JDKToolLauncher constructor:

     private JDKToolLauncher(String tool, boolean useCompilerJDK) {
         if (useCompilerJDK) {
             executable = JDKToolFinder.getJDKTool(tool);
         } else {
             executable = JDKToolFinder.getCurrentJDKTool(tool);
         }
 
vmArgs.addAll(Arrays.asList(ProcessTools.getPlatformSpecificVMArgs()));
     }

The last line does not add the -J where needed and could/should be:

    for (String vmArg : ProcessTools.getPlatformSpecificVMArgs()) {
       addVMArg(vmArg);
    }

But your change still ends up adding -J where needed. To me it seemed 
that it modified addVMArg in a way that was inconsistent with its 
specification, but on further reflection that is not the case.

My apologies for the erroneous commentary.

Reviewed.

Thanks,
David

On 19/10/2013 10:19 PM, David Holmes wrote:
> Hi Fredrik,
>
> On 18/10/2013 11:41 PM, Fredrik Arvidsson wrote:
>> Hi
>>
>> Please help me with this tiny review:
>>
>> Webrev:
>> http://cr.openjdk.java.net/~farvidsson/8026808/webrev.00/index.html
>> <http://cr.openjdk.java.net/%7Efarvidsson/8026808/webrev.00/index.html>
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8026808
>>
>> About this bug:
>> This bug was revealed by another test-fix:
>> https://bugs.openjdk.java.net/browse/JDK-8025638. The JDKToolLauncher
>> have probably never worked in Solaris 64 environments.
>
> I don't see the connection to 64-bit from the fix but I suspect it may
> be a confusion between launcher args (like -d64) and VM args (like
> -XX:+foo). AFAICS the tools will accept both -d64 and -J-d64.
>
> Your change to vmArgs is wrong. It explicitly states that it adds -J to
> the arg and you have removed that. When launching a tool (javac, javap,
> etc) you have to pass VM args via -J - and that is what this method was
> supposed to do. AFAICS based on the usage example given in the code:
>
> 40  * JDKToolLauncher jmap = JDKToolLauncher.create("jmap")
>    41  *                                       .addVMArg("-XX:+PrintGC");
>    42  * .addVMArg("-XX:+PrintGCDetails")
>    43  *                                       .addToolArg("-heap")
>    44  *                                       .addToolArg(pid);
>    45  * ProcessBuilder pb = new ProcessBuilder(jmap.getCommand());
>
> the existing code was correct.
>
> Based on the jtreg log in the bug report:
>
> Command line:
> [/export/local/aurora/sandbox/sca/vmsqe/jdk/nightly/fastdebug/rt_baseline/solaris-amd64/bin/java
> -d64 -Xmx1g JMapHProfLargeHeapProc 22528 ]
> Extracted pid: 18298
> jmap command:
> [/export/local/aurora/sandbox/sca/vmsqe/jdk/nightly/fastdebug/rt_baseline/solaris-amd64/bin/jmap,
> -d64, -dump:format=b,file=18298-heap.hprof, 18298]
>
> it would seem to me that the problem is the presence of commas in the
> jmap command (compare with the java command). Not that I can see where
> they come from. I can't find this test in our repo - where is it?
>
> David
> -----
>
>> JTReg tests run in JPRT:
>> http://sthjprt.se.oracle.com/archives/2013/10/2013-10-18-094943.farvidss.hotspot/
>>
>> verified successful.
>>
>> Cheers
>> /F