Re: RFR: 8236569: -Xss not multiple of 4K does not work for the main thread on macOS [v5]

[Henry Jen]

On Tue, 8 Jun 2021 13:37:10 GMT, Thomas Stuefe  wrote:

>> Henry Jen has updated the pull request with a new target base due to a merge 
>> or a rebase. The incremental webrev excludes the unrelated changes brought 
>> in by the merge/rebase. The pull request contains seven additional commits 
>> since the last revision:
>> 
>>  - Cast type
>>  - Merge
>>  - Change java -X output for -Xss
>>  - Merge
>>  - Only try to round-up when current value failed
>>  - Avoid overflow on page size
>>  - JDK-8236569: -Xss not multiple of 4K does not work for the main thread on 
>> macOS
>
> Please make sure the failing tests have nothing to do with your patch. 
> `gc/shenandoah/compiler/TestLinkToNativeRBP.java`
>  sounds at least suggestive.

Still pending CSR, also considering adapt hotspot align up as suggested by 
@tstuefe.

-

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

Re: RFR: 8236569: -Xss not multiple of 4K does not work for the main thread on macOS [v6]

[Henry Jen]

> …d on macOS
> 
> This patch simply round up the specified stack size to multiple of the system 
> page size. 
> 
> Test is trivial, simply run java with -Xss option against following code. On 
> MacOS, before the fix, running with `-Xss159k` and `-Xss160k` would get 
> `7183` and `649` respectively. After fix, both would output `649`, while 
> `-Xss161k` would be same as `-Xss164k` and see 691 as the output.
> 
> ```code:java
> public class StackLeak {
> public int depth = 0;
> public void stackLeak() {
> depth++;
> stackLeak();
> }
> 
> public static void main(String[] args) {
> var test = new StackLeak();
> try {
> test.stackLeak();
> } catch (Throwable e) {
>         System.out.println(test.depth);
> }
> }
> }

Henry Jen has updated the pull request incrementally with one additional commit 
since the last revision:

  Update help text

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/4256/files
  - new: https://git.openjdk.java.net/jdk/pull/4256/files/5a8d4a10..a3966612

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=4256=05
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=4256=04-05

  Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod
  Patch: https://git.openjdk.java.net/jdk/pull/4256.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/4256/head:pull/4256

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

Re: RFR: 8236569: -Xss not multiple of 4K does not work for the main thread on macOS [v5]

[Henry Jen]

On Tue, 8 Jun 2021 08:17:54 GMT, Thomas Stuefe  wrote:

>> Henry Jen has updated the pull request with a new target base due to a merge 
>> or a rebase. The incremental webrev excludes the unrelated changes brought 
>> in by the merge/rebase. The pull request contains seven additional commits 
>> since the last revision:
>> 
>>  - Cast type
>>  - Merge
>>  - Change java -X output for -Xss
>>  - Merge
>>  - Only try to round-up when current value failed
>>  - Avoid overflow on page size
>>  - JDK-8236569: -Xss not multiple of 4K does not work for the main thread on 
>> macOS
>
> src/java.base/unix/native/libjli/java_md.c line 666:
> 
>> 664: return page_size * pages;
>> 665: }
>> 666: }
> 
> Could probably be shortened to something like this:
> 
> 
> size_t pagesize = (size_t)sysconf(_SC_PAGESIZE);
> return (stack_size + (pagesize - 1)) & ~(pagesize - 1);
> 
> 
> or, if you insist on checking for SIZE_MAX:
> 
> 
> size_t pagesize = (size_t)sysconf(_SC_PAGESIZE);
> size_t max = SIZE_MAX - pagesize;
> return stack_size <= max ? (stack_size + (pagesize - 1)) & ~(pagesize - 1) : 
> max;
> 
> 
> (I see David requested this, so this is fine, though passing SIZE_MAX to this 
> function will quite likely fail anyway :)

While sysconf(_SC_PAGESIZE) is most likely(if not always) be power of 2, it's 
not a constant we know for sure here and this is not critical path for 
performance, thus I didn't take that approach.

-

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

Re: RFR: 8236569: -Xss not multiple of 4K does not work for the main thread on macOS [v5]

[Henry Jen]

On Tue, 8 Jun 2021 02:36:26 GMT, David Holmes  wrote:

>> Henry Jen has updated the pull request with a new target base due to a merge 
>> or a rebase. The incremental webrev excludes the unrelated changes brought 
>> in by the merge/rebase. The pull request contains seven additional commits 
>> since the last revision:
>> 
>>  - Cast type
>>  - Merge
>>  - Change java -X output for -Xss
>>  - Merge
>>  - Only try to round-up when current value failed
>>  - Avoid overflow on page size
>>  - JDK-8236569: -Xss not multiple of 4K does not work for the main thread on 
>> macOS
>
> src/java.base/share/classes/sun/launcher/resources/launcher.properties line 
> 175:
> 
>> 173: \  configuration and continue\n\
>> 174: \-Xssset java thread stack size\n\
>> 175: \  The actual size may be round up to multiple of 
>> system\n\
> 
> See updated help text in the CSR request.

Updated, thanks

-

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

Re: RFR: 8236569: -Xss not multiple of 4K does not work for the main thread on macOS [v5]

[Henry Jen]

On Mon, 7 Jun 2021 03:18:32 GMT, Henry Jen  wrote:

>> …d on macOS
>> 
>> This patch simply round up the specified stack size to multiple of the 
>> system page size. 
>> 
>> Test is trivial, simply run java with -Xss option against following code. On 
>> MacOS, before the fix, running with `-Xss159k` and `-Xss160k` would get 
>> `7183` and `649` respectively. After fix, both would output `649`, while 
>> `-Xss161k` would be same as `-Xss164k` and see 691 as the output.
>> 
>> ```code:java
>> public class StackLeak {
>> public int depth = 0;
>> public void stackLeak() {
>> depth++;
>> stackLeak();
>> }
>> 
>> public static void main(String[] args) {
>> var test = new StackLeak();
>> try {
>> test.stackLeak();
>> } catch (Throwable e) {
>> System.out.println(test.depth);
>> }
>> }
>> }
>
> Henry Jen has updated the pull request with a new target base due to a merge 
> or a rebase. The incremental webrev excludes the unrelated changes brought in 
> by the merge/rebase. The pull request contains seven additional commits since 
> the last revision:
> 
>  - Cast type
>  - Merge
>  - Change java -X output for -Xss
>  - Merge
>  - Only try to round-up when current value failed
>  - Avoid overflow on page size
>  - JDK-8236569: -Xss not multiple of 4K does not work for the main thread on 
> macOS

Linux man page for pthread_attr_setstacksize() states that,

   On some systems, pthread_attr_setstacksize() can fail with the
   error EINVAL if stacksize is not a multiple of the system page
   size.

-

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

Re: RFR: 8236569: -Xss not multiple of 4K does not work for the main thread on macOS [v5]

[Henry Jen]

On Mon, 7 Jun 2021 03:18:32 GMT, Henry Jen  wrote:

>> …d on macOS
>> 
>> This patch simply round up the specified stack size to multiple of the 
>> system page size. 
>> 
>> Test is trivial, simply run java with -Xss option against following code. On 
>> MacOS, before the fix, running with `-Xss159k` and `-Xss160k` would get 
>> `7183` and `649` respectively. After fix, both would output `649`, while 
>> `-Xss161k` would be same as `-Xss164k` and see 691 as the output.
>> 
>> ```code:java
>> public class StackLeak {
>> public int depth = 0;
>> public void stackLeak() {
>> depth++;
>> stackLeak();
>> }
>> 
>> public static void main(String[] args) {
>> var test = new StackLeak();
>> try {
>> test.stackLeak();
>> } catch (Throwable e) {
>> System.out.println(test.depth);
>> }
>> }
>> }
>
> Henry Jen has updated the pull request with a new target base due to a merge 
> or a rebase. The incremental webrev excludes the unrelated changes brought in 
> by the merge/rebase. The pull request contains seven additional commits since 
> the last revision:
> 
>  - Cast type
>  - Merge
>  - Change java -X output for -Xss
>  - Merge
>  - Only try to round-up when current value failed
>  - Avoid overflow on page size
>  - JDK-8236569: -Xss not multiple of 4K does not work for the main thread on 
> macOS

Planned to close JDK-8236569 as 'Won't Fix', as the issue was re-opened, we 
give it another shot. As explained in the CSR review, we will only round-up the 
stack size as required by the operating system. Test on Ubuntu shows that there 
is no need to round-up, while some other Posix system might as explained in the 
man page.
We round-up for MacOS as the document explicitly said that the size need to be 
multiple of system page size. I also changed to use getpagesize() as you 
suggested, although that's not needed.

-

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

Re: RFR: 8236569: -Xss not multiple of 4K does not work for the main thread on macOS [v5]

[Henry Jen]

> …d on macOS
> 
> This patch simply round up the specified stack size to multiple of the system 
> page size. 
> 
> Test is trivial, simply run java with -Xss option against following code. On 
> MacOS, before the fix, running with `-Xss159k` and `-Xss160k` would get 
> `7183` and `649` respectively. After fix, both would output `649`, while 
> `-Xss161k` would be same as `-Xss164k` and see 691 as the output.
> 
> ```code:java
> public class StackLeak {
> public int depth = 0;
> public void stackLeak() {
> depth++;
> stackLeak();
> }
> 
> public static void main(String[] args) {
> var test = new StackLeak();
> try {
> test.stackLeak();
> } catch (Throwable e) {
>         System.out.println(test.depth);
> }
> }
> }

Henry Jen has updated the pull request with a new target base due to a merge or 
a rebase. The incremental webrev excludes the unrelated changes brought in by 
the merge/rebase. The pull request contains seven additional commits since the 
last revision:

 - Cast type
 - Merge
 - Change java -X output for -Xss
 - Merge
 - Only try to round-up when current value failed
 - Avoid overflow on page size
 - JDK-8236569: -Xss not multiple of 4K does not work for the main thread on 
macOS

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/4256/files
  - new: https://git.openjdk.java.net/jdk/pull/4256/files/764a1f93..5a8d4a10

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=4256=04
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=4256=03-04

  Stats: 1679 lines in 41 files changed: 1388 ins; 168 del; 123 mod
  Patch: https://git.openjdk.java.net/jdk/pull/4256.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/4256/head:pull/4256

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

Re: RFR: 8236569: -Xss not multiple of 4K does not work for the main thread on macOS [v4]

[Henry Jen]

> …d on macOS
> 
> This patch simply round up the specified stack size to multiple of the system 
> page size. 
> 
> Test is trivial, simply run java with -Xss option against following code. On 
> MacOS, before the fix, running with `-Xss159k` and `-Xss160k` would get 
> `7183` and `649` respectively. After fix, both would output `649`, while 
> `-Xss161k` would be same as `-Xss164k` and see 691 as the output.
> 
> ```code:java
> public class StackLeak {
> public int depth = 0;
> public void stackLeak() {
> depth++;
> stackLeak();
> }
> 
> public static void main(String[] args) {
> var test = new StackLeak();
> try {
> test.stackLeak();
> } catch (Throwable e) {
>         System.out.println(test.depth);
> }
> }
> }

Henry Jen has updated the pull request incrementally with one additional commit 
since the last revision:

  Change java -X output for -Xss

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/4256/files
  - new: https://git.openjdk.java.net/jdk/pull/4256/files/1e96be94..764a1f93

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=4256=03
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=4256=02-03

  Stats: 2 lines in 1 file changed: 2 ins; 0 del; 0 mod
  Patch: https://git.openjdk.java.net/jdk/pull/4256.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/4256/head:pull/4256

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

Re: RFR: 8236569: -Xss not multiple of 4K does not work for the main thread on macOS [v3]

[Henry Jen]

> …d on macOS
> 
> This patch simply round up the specified stack size to multiple of the system 
> page size. 
> 
> Test is trivial, simply run java with -Xss option against following code. On 
> MacOS, before the fix, running with `-Xss159k` and `-Xss160k` would get 
> `7183` and `649` respectively. After fix, both would output `649`, while 
> `-Xss161k` would be same as `-Xss164k` and see 691 as the output.
> 
> ```code:java
> public class StackLeak {
> public int depth = 0;
> public void stackLeak() {
> depth++;
> stackLeak();
> }
> 
> public static void main(String[] args) {
> var test = new StackLeak();
> try {
> test.stackLeak();
> } catch (Throwable e) {
>         System.out.println(test.depth);
> }
> }
> }

Henry Jen has updated the pull request with a new target base due to a merge or 
a rebase. The incremental webrev excludes the unrelated changes brought in by 
the merge/rebase. The pull request contains four additional commits since the 
last revision:

 - Merge
 - Only try to round-up when current value failed
 - Avoid overflow on page size
 - JDK-8236569: -Xss not multiple of 4K does not work for the main thread on 
macOS

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/4256/files
  - new: https://git.openjdk.java.net/jdk/pull/4256/files/39b041d7..1e96be94

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=4256=02
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=4256=01-02

  Stats: 485003 lines in 2409 files changed: 450413 ins; 28438 del; 6152 mod
  Patch: https://git.openjdk.java.net/jdk/pull/4256.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/4256/head:pull/4256

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

Re: RFR: 8236569: -Xss not multiple of 4K does not work for the main thread on macOS [v2]

[Henry Jen]

> …d on macOS
> 
> This patch simply round up the specified stack size to multiple of the system 
> page size. 
> 
> Test is trivial, simply run java with -Xss option against following code. On 
> MacOS, before the fix, running with `-Xss159k` and `-Xss160k` would get 
> `7183` and `649` respectively. After fix, both would output `649`, while 
> `-Xss161k` would be same as `-Xss164k` and see 691 as the output.
> 
> ```code:java
> public class StackLeak {
> public int depth = 0;
> public void stackLeak() {
> depth++;
> stackLeak();
> }
> 
> public static void main(String[] args) {
> var test = new StackLeak();
> try {
> test.stackLeak();
> } catch (Throwable e) {
>         System.out.println(test.depth);
> }
> }
> }

Henry Jen has updated the pull request incrementally with one additional commit 
since the last revision:

  Avoid overflow on page size

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/4256/files
  - new: https://git.openjdk.java.net/jdk/pull/4256/files/fb9b22d5..39b041d7

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=4256=01
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=4256=00-01

  Stats: 15 lines in 2 files changed: 8 ins; 0 del; 7 mod
  Patch: https://git.openjdk.java.net/jdk/pull/4256.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/4256/head:pull/4256

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

Withdrawn: 8236569: -Xss not multiple of 4K does not work for the main thread on macOS

[Henry Jen]

On Fri, 28 May 2021 21:55:24 GMT, Henry Jen  wrote:

> …d on macOS
> 
> This patch simply round up the specified stack size to multiple of the system 
> page size. 
> 
> Test is trivial, simply run java with -Xss option against following code. On 
> MacOS, before the fix, running with `-Xss159k` and `-Xss160k` would get 
> `7183` and `649` respectively. After fix, both would output `649`, while 
> `-Xss161k` would be same as `-Xss164k` and see 691 as the output.
> 
> ```code:java
> public class StackLeak {
> public int depth = 0;
> public void stackLeak() {
> depth++;
> stackLeak();
> }
> 
> public static void main(String[] args) {
> var test = new StackLeak();
> try {
> test.stackLeak();
> } catch (Throwable e) {
> System.out.println(test.depth);
> }
> }
> }

This pull request has been closed without being integrated.

-

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

Re: RFR: 8236569: -Xss not multiple of 4K does not work for the main thread on macOS

[Henry Jen]

On Fri, 28 May 2021 21:55:24 GMT, Henry Jen  wrote:

> …d on macOS
> 
> This patch simply round up the specified stack size to multiple of the system 
> page size. 
> 
> Test is trivial, simply run java with -Xss option against following code. On 
> MacOS, before the fix, running with `-Xss159k` and `-Xss160k` would get 
> `7183` and `649` respectively. After fix, both would output `649`, while 
> `-Xss161k` would be same as `-Xss164k` and see 691 as the output.
> 
> ```code:java
> public class StackLeak {
> public int depth = 0;
> public void stackLeak() {
> depth++;
> stackLeak();
> }
> 
> public static void main(String[] args) {
> var test = new StackLeak();
> try {
> test.stackLeak();
> } catch (Throwable e) {
> System.out.println(test.depth);
> }
> }
> }

Withdraw for keep current behavior for compatibility. It would be preferred for 
user to specify proper value than we change the value on user's behalf.

-

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

Re: RFR: 8236569: -Xss not multiple of 4K does not work for the main thread on macOS

[Henry Jen]

On Mon, 31 May 2021 20:23:53 GMT, Vladimir Kempik  wrote:

>> src/java.base/macosx/native/libjli/java_md_macosx.m line 727:
>> 
>>> 725: 
>>> 726: static size_t alignUp(size_t stack_size) {
>>> 727: long page_size = sysconf(_SC_PAGESIZE);
>> 
>> In hotspot we use `getpagesize()`. There is also a guard for a very large 
>> stack (within a page of SIZE_MAX) so that rounding up does not produce zero.
>
> sounds like that (getpagesize) should work with m1 mac as well, as they have 
> 16k pages. will it ?

sysconf is the portable way based on POSIX, we can use getpagesize give this is 
macOS specific code, which is BSD based.

-

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

RFR: 8236569: -Xss not multiple of 4K does not work for the main thread on macOS

[Henry Jen]

…d on macOS

This patch simply round up the specified stack size to multiple of the system 
page size. 

Test is trivial, simply run java with -Xss option against following code. On 
MacOS, before the fix, running with `-Xss159k` and `-Xss160k` would get `7183` 
and `649` respectively. After fix, both would output `649`, while `-Xss161k` 
would be same as `-Xss164k` and see 691 as the output.

```code:java
public class StackLeak {
public int depth = 0;
public void stackLeak() {
depth++;
stackLeak();
}

public static void main(String[] args) {
var test = new StackLeak();
try {
test.stackLeak();
} catch (Throwable e) {
System.out.println(test.depth);
}
}
}

-

Commit messages:
 - JDK-8236569: -Xss not multiple of 4K does not work for the main thread on 
macOS

Changes: https://git.openjdk.java.net/jdk/pull/4256/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk=4256=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8236569
  Stats: 23 lines in 2 files changed: 21 ins; 0 del; 2 mod
  Patch: https://git.openjdk.java.net/jdk/pull/4256.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/4256/head:pull/4256

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

Integrated: 8261785: Calling "main" method in anonymous nested class crashes the JVM

[Henry Jen]

On Sun, 14 Mar 2021 23:34:55 GMT, Henry Jen  wrote:

> This patch ensure launcher won't crash JVM for the new static Methods from 
> local/anonymous class on MacOS.
> 
> As @dholmes-ora pointed out in the analysis, this is a MacOS specific bug 
> when the launcher trying to grab class name to be displayed as the 
> Application name on the menu.
> 
> The fix is to not setting name, test shows that GUI java application shows 
> 'bin' as the application name. It's possible for us to set the name to 
> something more friendly, for example, "Java", but I am not sure that should 
> be launcher's responsibility to choose such a default name. It seems to me 
> the consumer of the JAVA_MAIN_CLASS_%d environment variable should be 
> responsible to pick such name in case the environment variable is not set.

This pull request has now been integrated.

Changeset: b2df5137
Author:Henry Jen 
URL:   https://git.openjdk.java.net/jdk/commit/b2df5137
Stats: 143 lines in 3 files changed: 142 ins; 0 del; 1 mod

8261785: Calling "main" method in anonymous nested class crashes the JVM

Reviewed-by: serb

-

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

Re: RFR: 8261785: Calling "main" method in anonymous nested class crashes the JVM [v3]

[Henry Jen]

On Wed, 17 Mar 2021 08:55:54 GMT, Alan Bateman  wrote:

> > > Using an anonymous class for the main class looks strange and hard to 
> > > believe anyone is relying on this. I wonder if we should do more checking 
> > > LauncherHelper.validateMainClass to reject cases like this.
> > 
> > 
> > I raised that same question, and people tends to agree launcher could 
> > reject anonymous/local classes. But pointed out that should require a CSR 
> > review. Therefore I chose to fix crash first, and we can file another 
> > ticket to address main class requirements.
> 
> The requirement for a CSR and release note should not be a concern here. I 
> don't object to the fix but I think it would be very useful to document the 
> main class and whether local or anonymous classes can be used, its 
> accessibility, and the accessibility of the main method. We had to do 
> something similar recently with the premain method (java.lang.instrument).

Absolutely we need to clarify that, however, the discussion of what should or 
should not be allowed may take some time. For example, if we limit such usage 
in launcher, should it be possible for custom launcher to start such a main 
method? Thus the recommendation to have a separate ticket for that.

The current java document mostly only say to load the specify the class name, 
however there is a sentence `By default, the first argument that isn't an 
option of the java command is the fully qualified name of the class to be 
called. If -jar is specified, then its argument is the name of the JAR file 
containing class and resource files for the application. The startup class must 
be indicated by the Main-Class manifest header in its manifest file.`. Not that 
it says `fully qualified name of the class`.

Filed [JDK-8263735](https://bugs.openjdk.java.net/browse/JDK-8263735) for the 
follow up, in the mean time, we should get this crash resolved.

-

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

Re: RFR: 8261785: Calling "main" method in anonymous nested class crashes the JVM [v3]

[Henry Jen]

> This patch ensure launcher won't crash JVM for the new static Methods from 
> local/anonymous class on MacOS.
> 
> As @dholmes-ora pointed out in the analysis, this is a MacOS specific bug 
> when the launcher trying to grab class name to be displayed as the 
> Application name on the menu.
> 
> The fix is to not setting name, test shows that GUI java application shows 
> 'bin' as the application name. It's possible for us to set the name to 
> something more friendly, for example, "Java", but I am not sure that should 
> be launcher's responsibility to choose such a default name. It seems to me 
> the consumer of the JAVA_MAIN_CLASS_%d environment variable should be 
> responsible to pick such name in case the environment variable is not set.

Henry Jen has updated the pull request incrementally with one additional commit 
since the last revision:

  Add copyright and another test case

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/2999/files
  - new: https://git.openjdk.java.net/jdk/pull/2999/files/58f197f4..f68b0919

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=2999=02
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=2999=01-02

  Stats: 30 lines in 2 files changed: 29 ins; 0 del; 1 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2999.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2999/head:pull/2999

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

Re: RFR: 8261785: Calling "main" method in anonymous nested class crashes the JVM [v2]

[Henry Jen]

> This patch ensure launcher won't crash JVM for the new static Methods from 
> local/anonymous class on MacOS.
> 
> As @dholmes-ora pointed out in the analysis, this is a MacOS specific bug 
> when the launcher trying to grab class name to be displayed as the 
> Application name on the menu.
> 
> The fix is to not setting name, test shows that GUI java application shows 
> 'bin' as the application name. It's possible for us to set the name to 
> something more friendly, for example, "Java", but I am not sure that should 
> be launcher's responsibility to choose such a default name. It seems to me 
> the consumer of the JAVA_MAIN_CLASS_%d environment variable should be 
> responsible to pick such name in case the environment variable is not set.

Henry Jen has updated the pull request incrementally with one additional commit 
since the last revision:

  Adjust width used for Copyright

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/2999/files
  - new: https://git.openjdk.java.net/jdk/pull/2999/files/0fdea41c..58f197f4

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=2999=01
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=2999=00-01

  Stats: 15 lines in 1 file changed: 3 ins; 0 del; 12 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2999.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2999/head:pull/2999

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

Re: RFR: 8261785: Calling "main" method in anonymous nested class crashes the JVM

[Henry Jen]

On Tue, 16 Mar 2021 15:33:37 GMT, Alan Bateman  wrote:

> Using an anonymous class for the main class looks strange and hard to believe 
> anyone is relying on this. I wonder if we should do more checking 
> LauncherHelper.validateMainClass to reject cases like this.

I raised that same question, and people tends to agree launcher could reject 
anonymous/local classes. But pointed out that should require a CSR review. 
Therefore I chose to  fix crash first, and we can file another ticket to 
address main class requirements.

> This is not the last attempt to set the name, the JAVA_MAIN_CLASS_ variable 
> is used in the middle of the name selection, there are some others. And the 
> "bin" is selected by some of the next step, I agree it is not a friendly name 
> that could be improved.

I tried to do a quick search on JAVA_MAIN_CLASS_%pid variable, didn't find 
other code to set this. I had a version that would set the variable to "Java", 
I can extend that to cover exception case as well.

-

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

Re: RFR: 8261785: Calling "main" method in anonymous nested class crashes the JVM

[Henry Jen]

On Tue, 16 Mar 2021 01:59:33 GMT, Sergey Bylokhov  wrote:

>> This patch ensure launcher won't crash JVM for the new static Methods from 
>> local/anonymous class on MacOS.
>> 
>> As @dholmes-ora pointed out in the analysis, this is a MacOS specific bug 
>> when the launcher trying to grab class name to be displayed as the 
>> Application name on the menu.
>> 
>> The fix is to not setting name, test shows that GUI java application shows 
>> 'bin' as the application name. It's possible for us to set the name to 
>> something more friendly, for example, "Java", but I am not sure that should 
>> be launcher's responsibility to choose such a default name. It seems to me 
>> the consumer of the JAVA_MAIN_CLASS_%d environment variable should be 
>> responsible to pick such name in case the environment variable is not set.
>
> test/jdk/tools/launcher/8261785/CrashTheJVM.java line 1:
> 
>> 1: import java.io.IOException;
> 
> Copyright?

This file is mostly based on the bug report as I just adjust static keyword to 
make sure we cover different cases, thus I am not sure whether to add copyright 
or not.

-

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

RFR: 8261785: Calling "main" method in anonymous nested class crashes the JVM

[Henry Jen]

This patch ensure launcher won't crash JVM for the new static Methods from 
local/anonymous class on MacOS.

As @dholmes-ora pointed out in the analysis, this is a MacOS specific bug when 
the launcher trying to grab class name to be displayed as the Application name 
on the menu.

The fix is to not setting name, test shows that GUI java application shows 
'bin' as the application name. It's possible for us to set the name to 
something more friendly, for example, "Java", but I am not sure that should be 
launcher's responsibility to choose such a default name. It seems to me the 
consumer of the JAVA_MAIN_CLASS_%d environment variable should be responsible 
to pick such name in case the environment variable is not set.

-

Commit messages:
 - 8261785: Calling "main" method in anonymous nested class crashes the JVM

Changes: https://git.openjdk.java.net/jdk/pull/2999/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk=2999=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8261785
  Stats: 111 lines in 3 files changed: 110 ins; 0 del; 1 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2999.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2999/head:pull/2999

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

Re: RFR(S): 8241638: launcher time metrics alway report 1 on Linux when _JAVA_LAUNCHER_DEBUG set(Internet mail)

[Henry Jen]

Sure, will change the before I push. I considered that but didn’t run with it.

Cheers,
Henry


> On Apr 6, 2020, at 7:43 PM, David Holmes  wrote:
> 
> Hi Henry,
> 
> On 7/04/2020 3:36 am, Henry Jen wrote:
>> Here is the combined webrev[1] which I think is what we should have. By 
>> using __solaris__, we make sure no extra define from build and assuming that 
>> all solaris build will have gethrtime() and use that.
> 
> This looks good to me and means the Solaris code should start working 
> correctly again, as well as providing an implementation on all other 
> platforms.
> 
> Only minor suggestion is to move
> 
> 33 #include 
> 
> outside of the ifdef in the header file as all platforms will include it 
> anyway. You can then also remove the include in the .c file
> 
> 819 #include 
> 
> Thanks,
> David
> -
> 
>> The current build only use that for static build launcher, not custom 
>> launcher use libjli.
>> Cheers,
>> Henry
>> [1] http://cr.openjdk.java.net/~henryjen/jdk/8241638.2/webrev/
>>> On Apr 5, 2020, at 9:21 PM, David Holmes  wrote:
>>> 
>>> On 6/04/2020 12:37 pm, David Holmes wrote:
>>>> On 6/04/2020 12:20 pm, Henry Jen wrote:
>>>>> On Apr 5, 2020, at 7:15 PM, David Holmes  wrote:
>>>>>> 
>>>>>> On 6/04/2020 11:50 am, David Holmes wrote:
>>>>>>> There is something not right here ...
>>>>>>> On 4/04/2020 3:13 pm, Henry Jen wrote:
>>>>>>>> Internal test shows that inline implementation is not working for some 
>>>>>>>> Solaris artifacts, because the -DHAVE_GETHRTIME is not consistently 
>>>>>>>> defined, so it is actually broken. :)
>>>>>>>> 
>>>>>>>>> [2020-04-03T15:59:26,981Z] Creating 
>>>>>>>>> support/test/hotspot/jtreg/native/bin/jvm-test-launcher from 1 file(s)
>>>>>>> Are you sure that line actually pertains to any error? The test defines 
>>>>>>> a custom launcher which doesn't use libjli so should never be including 
>>>>>>> the header file we are discussing.
>>>>>>>>> [2020-04-03T16:02:10,984Z]
>>>>>>>>> [2020-04-03T16:02:10,984Z] ERROR: Build failed for target 'default 
>>>>>>>>> (product-bundles test-bundles static-libs-bundles)' in configuration 
>>>>>>>>> 'solaris-sparcv9-open' (exit code 2)
>>>>>>>>> [2020-04-03T16:02:11,051Z]
>>>>>>>>> [2020-04-03T16:02:11,051Z] === Output from failing command(s) 
>>>>>>>>> repeated here ===
>>>>>>>>> [2020-04-03T16:02:11,055Z] * For target 
>>>>>>>>> support_native_java.base_libjli_BUILD_LIBJLI_link:
>>>>>>>>> [2020-04-03T16:02:11,061Z] Undefinedfirst referenced
>>>>>>>>> [2020-04-03T16:02:11,061Z]  symbol  in file
>>>>>>>>> [2020-04-03T16:02:11,061Z] getTimeMicros   
>>>>>>>>> /export/home/opt/mach5/mesos/work_dir/e101deec-9613-4d46-a64d-559e689496aa/workspace/build/solaris-sparcv9-open/support/native/java.base/libjli/java.o
>>>>>>>>> [2020-04-03T16:02:11,061Z] ld: fatal: symbol referencing errors
>>>>>>> This looks like a direct linkage error. AFAICS 
>>>>>>> ./launcher/LauncherCommon.gmk only defines -DHAVE_GETHRTIME for 
>>>>>>> launcher executables. But if we are building libjli it is not an 
>>>>>>> executable. I'm suspecting there is actually a long standing build bug 
>>>>>>> here from when libjli was introduced. Possibly only evident on an 
>>>>>>> incremental build.
>>>>>> 
>>>>>> I can confirm that the flags set in LauncherCommon.gmk are not passed to 
>>>>>> the compilation of java_md_solinux.c (I added a custom -DDAVIDH to the 
>>>>>> linux flags and checked the build). So I have no idea how this has been 
>>>>>> working, if indeed it actually has.
>>>>>> 
>>>>> 
>>>>> I should say it’s the inconsistency for building java.c for launcher 
>>>>> executable and libjli.so, thus cause libjli.so failed to build. It wasn’t 
>>>>> detected before because CounterGet is defined as no-op, so nothing to 
>>>>> link with.
>>>> My point is that this seem

Re: RFR(S): 8241638: launcher time metrics alway report 1 on Linux when _JAVA_LAUNCHER_DEBUG set(Internet mail)

[Henry Jen]

Here is the combined webrev[1] which I think is what we should have. By using 
__solaris__, we make sure no extra define from build and assuming that all 
solaris build will have gethrtime() and use that.

The current build only use that for static build launcher, not custom launcher 
use libjli.

Cheers,
Henry

[1] http://cr.openjdk.java.net/~henryjen/jdk/8241638.2/webrev/

> On Apr 5, 2020, at 9:21 PM, David Holmes  wrote:
> 
> On 6/04/2020 12:37 pm, David Holmes wrote:
>> On 6/04/2020 12:20 pm, Henry Jen wrote:
>>> On Apr 5, 2020, at 7:15 PM, David Holmes  wrote:
>>>> 
>>>> On 6/04/2020 11:50 am, David Holmes wrote:
>>>>> There is something not right here ...
>>>>> On 4/04/2020 3:13 pm, Henry Jen wrote:
>>>>>> Internal test shows that inline implementation is not working for some 
>>>>>> Solaris artifacts, because the -DHAVE_GETHRTIME is not consistently 
>>>>>> defined, so it is actually broken. :)
>>>>>> 
>>>>>>> [2020-04-03T15:59:26,981Z] Creating 
>>>>>>> support/test/hotspot/jtreg/native/bin/jvm-test-launcher from 1 file(s)
>>>>> Are you sure that line actually pertains to any error? The test defines a 
>>>>> custom launcher which doesn't use libjli so should never be including the 
>>>>> header file we are discussing.
>>>>>>> [2020-04-03T16:02:10,984Z]
>>>>>>> [2020-04-03T16:02:10,984Z] ERROR: Build failed for target 'default 
>>>>>>> (product-bundles test-bundles static-libs-bundles)' in configuration 
>>>>>>> 'solaris-sparcv9-open' (exit code 2)
>>>>>>> [2020-04-03T16:02:11,051Z]
>>>>>>> [2020-04-03T16:02:11,051Z] === Output from failing command(s) repeated 
>>>>>>> here ===
>>>>>>> [2020-04-03T16:02:11,055Z] * For target 
>>>>>>> support_native_java.base_libjli_BUILD_LIBJLI_link:
>>>>>>> [2020-04-03T16:02:11,061Z] Undefinedfirst referenced
>>>>>>> [2020-04-03T16:02:11,061Z]  symbol  in file
>>>>>>> [2020-04-03T16:02:11,061Z] getTimeMicros   
>>>>>>> /export/home/opt/mach5/mesos/work_dir/e101deec-9613-4d46-a64d-559e689496aa/workspace/build/solaris-sparcv9-open/support/native/java.base/libjli/java.o
>>>>>>>  
>>>>>>> [2020-04-03T16:02:11,061Z] ld: fatal: symbol referencing errors
>>>>> This looks like a direct linkage error. AFAICS 
>>>>> ./launcher/LauncherCommon.gmk only defines -DHAVE_GETHRTIME for launcher 
>>>>> executables. But if we are building libjli it is not an executable. I'm 
>>>>> suspecting there is actually a long standing build bug here from when 
>>>>> libjli was introduced. Possibly only evident on an incremental build.
>>>> 
>>>> I can confirm that the flags set in LauncherCommon.gmk are not passed to 
>>>> the compilation of java_md_solinux.c (I added a custom -DDAVIDH to the 
>>>> linux flags and checked the build). So I have no idea how this has been 
>>>> working, if indeed it actually has.
>>>> 
>>> 
>>> I should say it’s the inconsistency for building java.c for launcher 
>>> executable and libjli.so, thus cause libjli.so failed to build. It wasn’t 
>>> detected before because CounterGet is defined as no-op, so nothing to link 
>>> with.
>> My point is that this seems completely broken. HAVE_GETHRTIME is never 
>> defined when building libjli, only when building the launcher executables, 
>> and the launcher executable code never calls CounterGet(). This suggests 
>> that the launcher debug code on Solaris has been using the same code as 
>> linux and thus should be seen to be reporting the same thing ... which it is 
>> ...
>> _JAVA_LAUNCHER_DEBUG=true 
>> /java/re/jdk/9/promoted/latest/binaries/solaris-x64/bin/java -version
>> _JAVA_LAUNCHER_DEBUG
>> Launcher state:
>> First application arg index: -1
>> debug:on
>> javargs:off
>> program name:java
>> launcher name:java
>> javaw:off
>> fullversion:9+181
>> Command line args:
>> argv[0] = /java/re/jdk/9/promoted/latest/binaries/solaris-x64/bin/java
>> argv[1] = -version
>> JRE path is .../java_re/jdk/9/fcs/181/binaries/solaris-x64
>> jvm.cfg[0] = ->-server<-
>> jvm.cfg[1] = ->-client<-
>> 1 micro seconds to parse jvm.cfg

Re: RFR(S): 8241638: launcher time metrics alway report 1 on Linux when _JAVA_LAUNCHER_DEBUG set(Internet mail)

[Henry Jen]

On Apr 5, 2020, at 7:15 PM, David Holmes  wrote:
> 
> On 6/04/2020 11:50 am, David Holmes wrote:
>> There is something not right here ...
>> On 4/04/2020 3:13 pm, Henry Jen wrote:
>>> Internal test shows that inline implementation is not working for some 
>>> Solaris artifacts, because the -DHAVE_GETHRTIME is not consistently 
>>> defined, so it is actually broken. :)
>>> 
>>>> [2020-04-03T15:59:26,981Z] Creating 
>>>> support/test/hotspot/jtreg/native/bin/jvm-test-launcher from 1 file(s)
>> Are you sure that line actually pertains to any error? The test defines a 
>> custom launcher which doesn't use libjli so should never be including the 
>> header file we are discussing.
>>>> [2020-04-03T16:02:10,984Z]
>>>> [2020-04-03T16:02:10,984Z] ERROR: Build failed for target 'default 
>>>> (product-bundles test-bundles static-libs-bundles)' in configuration 
>>>> 'solaris-sparcv9-open' (exit code 2)
>>>> [2020-04-03T16:02:11,051Z]
>>>> [2020-04-03T16:02:11,051Z] === Output from failing command(s) repeated 
>>>> here ===
>>>> [2020-04-03T16:02:11,055Z] * For target 
>>>> support_native_java.base_libjli_BUILD_LIBJLI_link:
>>>> [2020-04-03T16:02:11,061Z] Undefinedfirst referenced
>>>> [2020-04-03T16:02:11,061Z]  symbol  in file
>>>> [2020-04-03T16:02:11,061Z] getTimeMicros   
>>>> /export/home/opt/mach5/mesos/work_dir/e101deec-9613-4d46-a64d-559e689496aa/workspace/build/solaris-sparcv9-open/support/native/java.base/libjli/java.o
>>>>  
>>>> [2020-04-03T16:02:11,061Z] ld: fatal: symbol referencing errors
>> This looks like a direct linkage error. AFAICS ./launcher/LauncherCommon.gmk 
>> only defines -DHAVE_GETHRTIME for launcher executables. But if we are 
>> building libjli it is not an executable. I'm suspecting there is actually a 
>> long standing build bug here from when libjli was introduced. Possibly only 
>> evident on an incremental build.
> 
> I can confirm that the flags set in LauncherCommon.gmk are not passed to the 
> compilation of java_md_solinux.c (I added a custom -DDAVIDH to the linux 
> flags and checked the build). So I have no idea how this has been working, if 
> indeed it actually has.
> 

I should say it’s the inconsistency for building java.c for launcher executable 
and libjli.so, thus cause libjli.so failed to build. It wasn’t detected before 
because CounterGet is defined as no-op, so nothing to link with.

Cheers,
Henry


> David
> 
>> David
>> -
>>>> [2020-04-03T16:02:11,082Z]
>>>> [2020-04-03T16:02:11,082Z] * All command lines available in 
>>>> /export/home/opt/mach5/mesos/work_dir/e101deec-9613-4d46-a64d-559e689496aa/workspace/build/solaris-sparcv9-open/make-support/failure-logs.
>>>>  
>>>> [2020-04-03T16:02:11,086Z] === End of repeated output ===
>>>> [2020-04-03T16:02:11,094Z]
>>>> [2020-04-03T16:02:11,094Z] === Make failed targets repeated here ===
>>>> [2020-04-03T16:02:11,736Z] CoreLibraries.gmk:206: recipe for target 
>>>> '/export/home/opt/mach5/mesos/work_dir/e101deec-9613-4d46-a64d-559e689496aa/workspace/build/solaris-sparcv9-open/support/modules_libs/java.base/libjli.so'
>>>>  failed
>>>> [2020-04-03T16:02:11,737Z] make/Main.gmk:195: recipe for target 
>>>> 'java.base-libs' failed
>>>> [2020-04-03T16:02:11,739Z] === End of repeated output ===
>>>> [2020-04-03T16:02:11,741Z]
>>> 
>>> 
>>> I verified that either move implementation into .c as a function body[1] or 
>>> change to #ifdef __solaris__[2] will fix that. So I think we will change to 
>>> detect __solaris__ as webrev[2] rather than have an extra #define. If some 
>>> other build want to have that, they can be modify that #ifdef easily.
>>> 
>>> As I look into it, I found Mac have similar implementation with minor 
>>> mistake, so I fixed that as well. Please review following based on 
>>> zhanglin’s patch.
>>> 
>>> I’ll push [2] once I got a +1.
>>> 
>>> [1] http://cr.openjdk.java.net/~henryjen/jdk/8241638.0/webrev/
>>> [2] http://cr.openjdk.java.net/~henryjen/jdk/8241638.1/webrev/
>>> 
>>> Cheers,
>>> Henry
>>> 
>>>> On Apr 2, 2020, at 6:17 AM, Alan Bateman  wrote:
>>>> 
>>>> On 02/04/2020 11:26, linzang(臧琳) wrote:
>>>>> :
>>>>> Here is the updated webrev : 
>>>>> http://cr.openjdk.java.net/~lzang/8241638/webrev04/
>>>>> Thanks for your help!
>>>>> 
>>>> webrev04 looks good. My preference was to was to replace #ifdef 
>>>> HAVE_GETHRTIME with #ifdef __solaris__ but there doesn't seem to be 
>>>> appetite to do this now. I think Henry has offered to help sponsor.
>>>> 
>>>> -Alan.

Re: RFR(S): 8241638: launcher time metrics alway report 1 on Linux when _JAVA_LAUNCHER_DEBUG set(Internet mail)

[Henry Jen]

> On Apr 5, 2020, at 6:52 AM, Alan Bateman  wrote:
> 
> 
> On 05/04/2020 14:17, David Holmes wrote:
>> On 4/04/2020 3:13 pm, Henry Jen wrote:
>>> Internal test shows that inline implementation is not working for some 
>>> Solaris artifacts, because the -DHAVE_GETHRTIME is not consistently 
>>> defined, so it is actually broken. :)
>> 
>> The problem is in defining that function as inline rather than the 
>> -DHAVE_GETHRTIME.
>> 
>>>> [2020-04-03T15:59:26,981Z] Creating 
>>>> support/test/hotspot/jtreg/native/bin/jvm-test-launcher from 1 file(s)
>> 
>> The build rules for this special test launcher need to be examined as 
>> something seems wrong to me.
> I assume it's just that it's just compiled differently to the regular 
> launchers and just not noticed that it was missing -DHAVE_GETHRTIME (unless 
> for anyone to use it with _JAVA_LAUNCHER_DEBUG set).

This is my understanding as well, we built something without define 
-DHAVA_GETHRTIME but linked with the library that was built with that defined 
and use inline function. We won’t notice this before because CounterGet is 
no-op before.

> If we go with Henry's second webrev then I assume -DHAVE_GETHRTIME can be 
> dropped from LauncherCommon.gmk to avoid confusing any further maintainers in 
> this area.

Right, I can drop that as well.

> Also is there a strong need for the non-Solaris getTimeMicros to have be 
> inline?
> 

Certainly we can change to not inline by combining both fixes. I don’t think 
inline is necessary causing problem but that means header and the binary does 
need to match.

Cheers,
Henry

Re: RFR(S): 8241638: launcher time metrics alway report 1 on Linux when _JAVA_LAUNCHER_DEBUG set(Internet mail)

[Henry Jen]

Internal test shows that inline implementation is not working for some Solaris 
artifacts, because the -DHAVE_GETHRTIME is not consistently defined, so it is 
actually broken. :)

> [2020-04-03T15:59:26,981Z] Creating 
> support/test/hotspot/jtreg/native/bin/jvm-test-launcher from 1 file(s)
> [2020-04-03T16:02:10,984Z] 
> [2020-04-03T16:02:10,984Z] ERROR: Build failed for target 'default 
> (product-bundles test-bundles static-libs-bundles)' in configuration 
> 'solaris-sparcv9-open' (exit code 2) 
> [2020-04-03T16:02:11,051Z] 
> [2020-04-03T16:02:11,051Z] === Output from failing command(s) repeated here 
> ===
> [2020-04-03T16:02:11,055Z] * For target 
> support_native_java.base_libjli_BUILD_LIBJLI_link:
> [2020-04-03T16:02:11,061Z] Undefined  first referenced
> [2020-04-03T16:02:11,061Z]  symbolin file
> [2020-04-03T16:02:11,061Z] getTimeMicros   
> /export/home/opt/mach5/mesos/work_dir/e101deec-9613-4d46-a64d-559e689496aa/workspace/build/solaris-sparcv9-open/support/native/java.base/libjli/java.o
> [2020-04-03T16:02:11,061Z] ld: fatal: symbol referencing errors
> [2020-04-03T16:02:11,082Z] 
> [2020-04-03T16:02:11,082Z] * All command lines available in 
> /export/home/opt/mach5/mesos/work_dir/e101deec-9613-4d46-a64d-559e689496aa/workspace/build/solaris-sparcv9-open/make-support/failure-logs.
> [2020-04-03T16:02:11,086Z] === End of repeated output ===
> [2020-04-03T16:02:11,094Z] 
> [2020-04-03T16:02:11,094Z] === Make failed targets repeated here ===
> [2020-04-03T16:02:11,736Z] CoreLibraries.gmk:206: recipe for target 
> '/export/home/opt/mach5/mesos/work_dir/e101deec-9613-4d46-a64d-559e689496aa/workspace/build/solaris-sparcv9-open/support/modules_libs/java.base/libjli.so'
>  failed
> [2020-04-03T16:02:11,737Z] make/Main.gmk:195: recipe for target 
> 'java.base-libs' failed
> [2020-04-03T16:02:11,739Z] === End of repeated output ===
> [2020-04-03T16:02:11,741Z] 


I verified that either move implementation into .c as a function body[1] or 
change to #ifdef __solaris__[2] will fix that. So I think we will change to 
detect __solaris__ as webrev[2] rather than have an extra #define. If some 
other build want to have that, they can be modify that #ifdef easily.

As I look into it, I found Mac have similar implementation with minor mistake, 
so I fixed that as well. Please review following based on zhanglin’s patch.

I’ll push [2] once I got a +1.

[1] http://cr.openjdk.java.net/~henryjen/jdk/8241638.0/webrev/
[2] http://cr.openjdk.java.net/~henryjen/jdk/8241638.1/webrev/

Cheers,
Henry

> On Apr 2, 2020, at 6:17 AM, Alan Bateman  wrote:
> 
> On 02/04/2020 11:26, linzang(臧琳) wrote:
>> :
>>Here is the updated webrev : 
>> http://cr.openjdk.java.net/~lzang/8241638/webrev04/
>>Thanks for your help!
>> 
> webrev04 looks good. My preference was to was to replace #ifdef 
> HAVE_GETHRTIME with #ifdef __solaris__ but there doesn't seem to be appetite 
> to do this now. I think Henry has offered to help sponsor.
> 
> -Alan.

Re: RFR(S): 8241638: launcher time metrics alway report 1 on Linux when _JAVA_LAUNCHER_DEBUG set(Internet mail)

[Henry Jen]

Yes, we need an official Reviewer to approve. I can help to push the change 
after that.

Cheers,
Henry



> On Mar 31, 2020, at 9:55 PM, linzang(臧琳)  wrote:
> 
> Thanks Henry, 
> 
> I agree to leave Solaris as it is, it seems to me there is no strong reason 
> to remove it.
> 
> So, Do I need more review before push this patch? 
> 
> And may I ask your help to push it if a go decision is made.
> 
> Thanks.
> 
> 
> BRs,
> Lin
> 
> On 2020/3/31, 12:30 PM, "Henry Jen"  wrote:
> 
>OK, I agree with David gettimeofday is an improvement than 1, so we can go 
> ahead with the patch. Not sure if the caveat should be disclosed or not, I 
> don’t think it’s important enough, just a known fact probably worth to leave 
> a trace(perhaps a comment in code or the bug entry). As whether to change the 
> ifdef to simply detect Solaris, I prefer just to leave it as is for two 
> reasons:
> 
>1. It’s not broken, why change it?
>2. I checked the code, it’s just a simply macro defined by the make file. 
> Realtime Linux extension would have that function and special custom build 
> can still use that, even though that probably is not happening.
> 
>Cheers,
>Henry
> 
> 
>> On Mar 30, 2020, at 7:39 PM, linzang(臧琳)  wrote:
>> 
>> Hi David, Henry, Alan,
>>   Thanks a lot for your review. 
>>   I have considered use clock_gettime() first, but I seached the code and 
>> found there is a marco SUPPORTS_CLOCK_MONOTONIC guard the usage of it. Which 
>> makes me think it may not be available under specific situation. So I 
>> choosed gettimeofday.  
>>  Do you think the patch need to be refined to remove HAVE_GETHRTIME as Alan 
>> suggested?  Thanks. 
>> 
>> BRs,
>> Lin
>> 
>>> On 2020/3/31, 8:05 AM, "David Holmes"  wrote:
>>> 
>>>  On 31/03/2020 4:08 am, Henry Jen wrote:
>>>> Based on my understanding to gethrtime(), the main benefit is not to be 
>>>> affected by settimeofday or adjtime. I think it is probably better to use
>>>> 
>>>> clock_gettime(CLOCK_MONOTONIC_RAW, ts);
>>>> 
>>>> which I checked seems to be available on both Linux and Mac. Haven’t test 
>>>> it though.
>>> 
>>>  Not guaranteed to be available - either clock_gettime function or that 
>>>  particular clock - at build time or runtime. We use a check in the build 
>>>  system to determine build-time availability for hotspot, and then use 
>>>  dl_lookup etc at runtime to determine if actually available. We should 
>>>  be able to get rid of this one day but we checked fairly recently and 
>>>  there were still some issues.
>> 
>>>  gettimeofday is a lot better than returning 1. Otherwise call into the 
>>>  VM and use JVM_NanoTime.
>>> 
>>>  Cheers,
>>>  David
>>>  -
>>> 
>>>> Cheers,
>>>> Henry
>>>> 
>>>>> On Mar 30, 2020, at 1:37 AM, Alan Bateman  wrote:
>>>>> 
>>>>> On 30/03/2020 03:41, linzang(臧琳) wrote:
>>>>>> Dear All,
>>>>>> May I ask your help to reivew this tiny patch? Thanks.
>>>>>> 
>>>>>> 
>>>>>> 
>>>>>> BRs,
>>>>>> Lin
>>>>>> 
>>>>>> From: "linzang(臧琳)" 
>>>>>> Date: Thursday, March 26, 2020 at 3:13 PM
>>>>>> To: "core-libs-dev@openjdk.java.net" 
>>>>>> Subject: RFR(S): 8241638: launcher time metrics alway report 1 on Linux 
>>>>>> when _JAVA_LAUNCHER_DEBUG set
>>>>>> 
>>>>>> Dear All,
>>>>>> May I ask your help to review this tiny fix?
>>>>>>Bug: https://bugs.openjdk.java.net/browse/JDK-8241638
>>>>>>Webrev: http://cr.openjdk.java.net/~lzang/8241638/webrev01/
>>>>>> Thanks!
>>>>>> 
>>>>> Using gettimeofday on non-Solaris platforms seems reasonable here. The 
>>>>> comment in the patch suggests Linux but it's other Unix builds too. Also 
>>>>> just a minor nit that the code in java.base uses 4-space indent, not 2. 
>>>>> Looking at the patch makes me wondering if we should remove 
>>>>> HAVE_GETHRTIME as it seems to be only used on Solaris >and the launcher 
>>>>> is already using #ifdef __solaris__ in several places. Henry, do you have 
>>>>> any comments on this?
>>>>> 
>>>>> -Alan
>>>> 
>> 
>> 
>> 
> 
> 
> 
> 

Re: RFR(S): 8241638: launcher time metrics alway report 1 on Linux when _JAVA_LAUNCHER_DEBUG set(Internet mail)

[Henry Jen]

OK, I agree with David gettimeofday is an improvement than 1, so we can go 
ahead with the patch. Not sure if the caveat should be disclosed or not, I 
don’t think it’s important enough, just a known fact probably worth to leave a 
trace(perhaps a comment in code or the bug entry). As whether to change the 
ifdef to simply detect Solaris, I prefer just to leave it as is for two reasons:

1. It’s not broken, why change it?
2. I checked the code, it’s just a simply macro defined by the make file. 
Realtime Linux extension would have that function and special custom build can 
still use that, even though that probably is not happening.

Cheers,
Henry


> On Mar 30, 2020, at 7:39 PM, linzang(臧琳)  wrote:
> 
> Hi David, Henry, Alan,
>Thanks a lot for your review. 
>I have considered use clock_gettime() first, but I seached the code and 
> found there is a marco SUPPORTS_CLOCK_MONOTONIC guard the usage of it. Which 
> makes me think it may not be available under specific situation. So I choosed 
> gettimeofday.  
>   Do you think the patch need to be refined to remove HAVE_GETHRTIME as Alan 
> suggested?  Thanks. 
> 
> BRs,
> Lin
> 
> >On 2020/3/31, 8:05 AM, "David Holmes"  wrote:
>> 
>>   On 31/03/2020 4:08 am, Henry Jen wrote:
>>> Based on my understanding to gethrtime(), the main benefit is not to be 
>>> affected by settimeofday or adjtime. I think it is probably better to use
>>> 
>>> clock_gettime(CLOCK_MONOTONIC_RAW, ts);
>>> 
>>> which I checked seems to be available on both Linux and Mac. Haven’t test 
>>> it though.
>> 
>>   Not guaranteed to be available - either clock_gettime function or that 
>>   particular clock - at build time or runtime. We use a check in the build 
>>   system to determine build-time availability for hotspot, and then use 
>>   dl_lookup etc at runtime to determine if actually available. We should 
>>   be able to get rid of this one day but we checked fairly recently and 
>>   there were still some issues.
> 
>>   gettimeofday is a lot better than returning 1. Otherwise call into the 
>>   VM and use JVM_NanoTime.
>> 
>>   Cheers,
>>   David
>>   -
>> 
>>> Cheers,
>>> Henry
>>> 
>>>> On Mar 30, 2020, at 1:37 AM, Alan Bateman  wrote:
>>>> 
>>>> On 30/03/2020 03:41, linzang(臧琳) wrote:
>>>>> Dear All,
>>>>>  May I ask your help to reivew this tiny patch? Thanks.
>>>>> 
>>>>> 
>>>>> 
>>>>> BRs,
>>>>> Lin
>>>>> 
>>>>> From: "linzang(臧琳)" 
>>>>> Date: Thursday, March 26, 2020 at 3:13 PM
>>>>> To: "core-libs-dev@openjdk.java.net" 
>>>>> Subject: RFR(S): 8241638: launcher time metrics alway report 1 on Linux 
>>>>> when _JAVA_LAUNCHER_DEBUG set
>>>>> 
>>>>> Dear All,
>>>>> May I ask your help to review this tiny fix?
>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8241638
>>>>> Webrev: http://cr.openjdk.java.net/~lzang/8241638/webrev01/
>>>>> Thanks!
>>>>> 
>>>> Using gettimeofday on non-Solaris platforms seems reasonable here. The 
>>>> comment in the patch suggests Linux but it's other Unix builds too. Also 
>>>> just a minor nit that the code in java.base uses 4-space indent, not 2. 
>>>> Looking at the patch makes me wondering if we should remove HAVE_GETHRTIME 
>>>> as it seems to be only used on Solaris >and the launcher is already using 
>>>> #ifdef __solaris__ in several places. Henry, do you have any comments on 
>>>> this?
>>>> 
>>>> -Alan
>>> 
> 
> 
> 

Re: RFR(S): 8241638: launcher time metrics alway report 1 on Linux when _JAVA_LAUNCHER_DEBUG set

[Henry Jen]

Based on my understanding to gethrtime(), the main benefit is not to be 
affected by settimeofday or adjtime. I think it is probably better to use

clock_gettime(CLOCK_MONOTONIC_RAW, ts);

which I checked seems to be available on both Linux and Mac. Haven’t test it 
though.

Cheers,
Henry

> On Mar 30, 2020, at 1:37 AM, Alan Bateman  wrote:
> 
> On 30/03/2020 03:41, linzang(臧琳) wrote:
>> Dear All,
>>  May I ask your help to reivew this tiny patch? Thanks.
>> 
>> 
>> 
>> BRs,
>> Lin
>> 
>> From: "linzang(臧琳)" 
>> Date: Thursday, March 26, 2020 at 3:13 PM
>> To: "core-libs-dev@openjdk.java.net" 
>> Subject: RFR(S): 8241638: launcher time metrics alway report 1 on Linux when 
>> _JAVA_LAUNCHER_DEBUG set
>> 
>> Dear All,
>> May I ask your help to review this tiny fix?
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8241638
>> Webrev: http://cr.openjdk.java.net/~lzang/8241638/webrev01/
>> Thanks!
>> 
> Using gettimeofday on non-Solaris platforms seems reasonable here. The 
> comment in the patch suggests Linux but it's other Unix builds too. Also just 
> a minor nit that the code in java.base uses 4-space indent, not 2. Looking at 
> the patch makes me wondering if we should remove HAVE_GETHRTIME as it seems 
> to be only used on Solaris and the launcher is already using #ifdef 
> __solaris__ in several places. Henry, do you have any comments on this?
> 
> -Alan

Re: RFR: 8240629: argfiles parsing broken for argfiles with comment cross 4096 bytes chunk

[Henry Jen]

Thanks for the review, I updated the webrev[1] with simplified test and ensure 
other cases in boundary won’t be causing trouble by only take meaningful tokens.

This fix is more defensive and allow anchor to be ignored when it’s meaningless.

[1] http://cr.openjdk.java.net/~henryjen/jdk/8240629.1/webrev/

Cheers,
Henry


> On Mar 8, 2020, at 1:54 AM, Alan Bateman  wrote:
> 
> On 06/03/2020 22:40, Henry Jen wrote:
>> Hi,
>> 
>> Please review the webrev[1] fix JDK-8240629 reported earlier by Robert.
>> 
>> http://cr.openjdk.java.net/~henryjen/jdk/8240629.0/webrev/
>> 
> The changes and test look okay.
> 
> -Alan

RFR: 8240629: argfiles parsing broken for argfiles with comment cross 4096 bytes chunk

[Henry Jen]

Hi,

Please review the webrev[1] fix JDK-8240629 reported earlier by Robert.

http://cr.openjdk.java.net/~henryjen/jdk/8240629.0/webrev/

Cheers,
Henry

Re: argfiles parsing broken for argfiles > 4096 bytes

[Henry Jen]

Thanks for the report and investigation, bug confirmed, JDK-8240629 is filed to 
tacking the issue.

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

Cheers,
Henry


> On Mar 5, 2020, at 11:43 AM, Robert Stupp  wrote:
> 
> Another note: I've also seen it appending a comment-line in
> http://hg.openjdk.java.net/jdk-updates/jdk11u/annotate/20e49753c388/src/java.base/share/native/libjli/args.c#l296
> with pctx->state==IN_COMMENT, but wasn't able to reconstruct the
> behavior with a "clean" argfile. But that behavior was also fixed with
> the condition ('&& pctx->state == IN_TOKEN') added by the patch.
> 
> On Thu, 2020-03-05 at 20:34 +0100, Robert Stupp wrote:
>> Sorry for the broken formatting...
>> 
>> I recently came across a bug in the java executable with an argfile
>> that's larger than 4096 bytes, if a 4096-byte-chunk ends in a comment
>> line.
>> 
>> The bug happens when the last character of a comment line is the
>> 4096th
>> byte and the trailing newline is the first byte in the next chunk. In
>> that case, nextToken() in src/java.base/share/native/libjli/args.c
>> calls JLI_List_addSubstring (in the if-block at the end of
>> nextToken())
>> with the contents of the current comment line. The next "valid"
>> option
>> gets appended to that comment line and java errors out.
>> 
>> I've added some debugging output to the last if-block and pctx->
>> state==FIND_NEXT and the the substring being added is the comment
>> line.
>> 
>> I've also tried and changed this line
>> http://hg.openjdk.java.net/jdk-updates/jdk11u/annotate/20e49753c388/src/java.base/share/native/libjli/args.c#l294
>> to 'if (anchor != nextc && pctx->state == IN_TOKEN)' and the argfile
>> is
>> parsed correctly.
>> 
>> Steps to reproduce:
>> 1. save the attached my-argfile
>> 2. run 'java @my-argfile my.className'
>> 3. java errors out with
>> Error: Could not find or
>> load main class #
>> X
>> XX
>> X
>> -Dfoo=bar
>> Caused by: java.lang.ClassNotFoundException: #
>> X
>> XX
>> XX
>> -Dfoo=bar
>> 
>> With the above change (the attached patch in argfile-parse-bug.txt),
>> argfile parsing works as expected.Also updated the condition in the
>> loop looking for a comment's newline, as it looks inconsistent to the
>> condition in the loop for FIND_NEXT/SKIP_LEAD_WS and the condition of
>> the outer for-loop.
>> 
>> Robert
>> 
>> 
>> On Thu, 2020-03-05 at 20:00 +0100, Robert Stupp wrote:
>>> Hi,
>> ...
> 

Re: [EXTERNAL] JDK-8234076 bug fix candidate

[Henry Jen]

The quote is causing test failure on Mac, I’ll apply following fix
diff --git a/test/jdk/tools/launcher/ArgsEnvVar.java 
b/test/jdk/tools/launcher/ArgsEnvVar.java
--- a/test/jdk/tools/launcher/ArgsEnvVar.java
+++ b/test/jdk/tools/launcher/ArgsEnvVar.java
@@ -285,7 +285,7 @@
 }
 
 // check that specifying --module and --module-path with file works
-tr = doExec(javaCmd, "-Dfile.encoding=UTF-8", "\"@cmdargs\"");
+tr = doExec(javaCmd, "-Dfile.encoding=UTF-8", "@cmdargs");
 tr.contains("[--hello]");
 if (!tr.testStatus) {
 System.out.println(tr);
@@ -294,7 +294,7 @@
 
 // check with reversed --module-path and --module in the arguments 
file, this will fail, --module= is terminating
 File argFile1 = createArgFile("cmdargs1", List.of(MODULE_OPTION, 
"--module-path", MODS_DIR.toString(), "--hello"));
-tr = doExec(javaCmd, "-Dfile.encoding=UTF-8", "\"@cmdargs1\"");
+tr = doExec(javaCmd, "-Dfile.encoding=UTF-8", "@cmdargs1");
 tr.checkNegative();
 if (!tr.testStatus) {
     System.out.println(tr);


Cheers,
Henry



> On Dec 11, 2019, at 10:29 PM, Henry Jen  wrote:
> 
> Nikola,
> 
> The change looks fine to me as well, I’ll run the test and push it before 
> RDP1 if everything is good with Kumar as reviewer.
> 
> Cheers,
> Henry
> 
> 
>> On Dec 11, 2019, at 2:26 PM, Kumar Srinivasan  wrote:
>> 
>> Hi Nikola,
>> 
>> Thank you for making the changes. Looks good to me. 
>> 
>> Kumar Srinivasan
>> PS:  my new and official email address: kusriniva...@vmware.com
>> 
>> 
>> On Wed, Dec 11, 2019 at 10:04 AM Nikola Grcevski 
>>  wrote:
>> Thank you again for reviewing my code Kumar!
>> 
>> I have applied your refactoring suggestions. Using the array approach as you 
>> suggested made the test code a lot more tidier. I’m attaching the updated 
>> patch after my signature and the external webrev is here for reference:
>> 
>> https://grcevski.github.io/JDK-8234076/webrev/ 
>> 
>> Cheers,
>> Nikola
>> 
>> diff -r bd436284147d src/java.base/share/native/libjli/args.c
>> --- a/src/java.base/share/native/libjli/args.c  Wed Nov 20 08:12:14 2019 
>> +0800
>> +++ b/src/java.base/share/native/libjli/args.c  Wed Dec 11 12:09:29 2019 
>> -0500
>> @@ -130,6 +130,8 @@
>> }
>> } else if (JLI_StrCmp(arg, "--disable-@files") == 0) {
>> stopExpansion = JNI_TRUE;
>> +} else if (JLI_StrCCmp(arg, "--module=") == 0) {
>> +idx = argsCount;
>> }
>> } else {
>> if (!expectingNoDashArg) {
>> @@ -449,6 +451,7 @@
>> return JLI_StrCmp(arg, "-jar") == 0 ||
>>JLI_StrCmp(arg, "-m") == 0 ||
>>JLI_StrCmp(arg, "--module") == 0 ||
>> +   JLI_StrCCmp(arg, "--module=") == 0 ||
>>JLI_StrCmp(arg, "--dry-run") == 0 ||
>>JLI_StrCmp(arg, "-h") == 0 ||
>>JLI_StrCmp(arg, "-?") == 0 ||
>> diff -r bd436284147d src/java.base/windows/native/libjli/java_md.c
>> --- a/src/java.base/windows/native/libjli/java_md.c Wed Nov 20 08:12:14 
>> 2019 +0800
>> +++ b/src/java.base/windows/native/libjli/java_md.c Wed Dec 11 12:09:29 
>> 2019 -0500
>> @@ -34,6 +34,7 @@
>> #include 
>> #include 
>> #include 
>> +#include 
>> 
>> #include 
>> #include "java.h"
>> @@ -1015,6 +1016,17 @@
>> 
>> // sanity check, match the args we have, to the holy grail
>> idx = JLI_GetAppArgIndex();
>> +
>> +// First arg index is NOT_FOUND
>> +if (idx < 0) {
>> +// The only allowed value should be NOT_FOUND (-1) unless another 
>> change introduces
>> +// a different negative index
>> +assert (idx == -1);
>> +JLI_TraceLauncher("Warning: first app arg index not found, %d\n", 
>> idx);
>> +JLI_TraceLauncher("passing arguments as-is.\n");
>> +return NewPlatformStringArray(env, strv, argc);
>> +}
>> +
>> isTool = (idx == 0);
>> if (isTool) { idx++; } // skip tool name
>> JLI_TraceLauncher("AppArgIndex: %d points to %s\n", idx, 
>> stdargs[idx].arg);
>> diff -r bd436284147d test/jdk/tools/launcher/ArgsEnvVar.java
>> --- a/test/jdk/tools/launc

Re: [EXTERNAL] JDK-8234076 bug fix candidate

[Henry Jen]

9:29 
> 2019 -0500
> @@ -246,6 +246,10 @@
>  if (!tr.contains("Error: Could not find or load main class 
> AbsentClass")) {
>  throw new RuntimeException("Test Fails");
>  }
> +
> +// Make sure we handle correctly the module long form (--module=)
> +tr = doExec(javaCmd, "-XX:NativeMemoryTracking=summary", 
> "--module=jdk.compiler/com.sun.tools.javac.Main", "--help");
> +ensureNoWarnings(tr);
>  }
> 
>  void ensureNoWarnings(TestResult tr) {
> diff -r bd436284147d test/jdk/tools/launcher/modules/basic/BasicTest.java
> --- a/test/jdk/tools/launcher/modules/basic/BasicTest.java  Wed Nov 20 
> 08:12:14 2019 +0800
> +++ b/test/jdk/tools/launcher/modules/basic/BasicTest.java  Wed Dec 11 
> 12:09:29 2019 -0500
> @@ -29,6 +29,7 @@
>   *  jdk.jlink
>   * @build BasicTest jdk.test.lib.compiler.CompilerUtils
>   * @run testng BasicTest
> + * @bug 8234076
>   * @summary Basic test of starting an application as a module
>   */
> 
> @@ -40,6 +41,8 @@
> 
>  import jdk.test.lib.compiler.CompilerUtils;
>  import jdk.test.lib.process.ProcessTools;
> +import jdk.test.lib.process.OutputAnalyzer;
> +import jdk.test.lib.Utils;
> 
>  import org.testng.annotations.BeforeTest;
>  import org.testng.annotations.Test;
> @@ -70,6 +73,8 @@
>  // the module main class
>  private static final String MAIN_CLASS = "jdk.test.Main";
> 
> +// for Windows specific launcher tests
> +static final boolean IS_WINDOWS = System.getProperty("os.name", 
> "unknown").startsWith("Windows");
> 
>  @BeforeTest
>  public void compileTestModule() throws Exception {
> @@ -259,4 +264,87 @@
>  assertTrue(exitValue != 0);
>  }
> 
> +
> +/**
> + * Helper method that creates a ProcessBuilder with command line 
> arguments
> + * while setting the _JAVA_LAUNCHER_DEBUG environment variable.
> + */
> +private ProcessBuilder createProcessWithLauncherDebugging(String... 
> cmds) {
> +ProcessBuilder pb = 
> ProcessTools.createJavaProcessBuilder(Utils.addTestJavaOpts(cmds));
> +pb.environment().put("_JAVA_LAUNCHER_DEBUG", "true");
> +
> +return pb;
> +}
> +
> + /**
> + * Test the ability for the Windows launcher to do proper application 
> argument
> + * detection and expansion, when using the long form module option and 
> all passed in
> + * command line arguments are prefixed with a dash.
> + *
> + * These tests are not expected to work on *nixes, and are ignored.
> + */
> +public void testWindowsWithLongFormModuleOption() throws Exception {
> +if (!IS_WINDOWS) {
> +return;
> +}
> +
> +String dir = MODS_DIR.toString();
> +String mid = TEST_MODULE + "/" + MAIN_CLASS;
> +
> +// java --module-path=mods --module=$TESTMODULE/$MAINCLASS --help
> +// We should be able to find the argument --help as an application 
> argument
> +ProcessTools.executeProcess(
> +createProcessWithLauncherDebugging(
> +"--module-path=" + dir,
> +"--module=" + mid,
> +"--help"))
> +.outputTo(System.out)
> +.errorTo(System.out)
> +.shouldContain("F--help");
> +
> +// java --module-path=mods --module=$TESTMODULE/$MAINCLASS 
> <...src/test>/*.java --help
> +// We should be able to see argument expansion happen
> +ProcessTools.executeProcess(
> +createProcessWithLauncherDebugging(
> +"--module-path=" + dir,
> +"--module=" + mid,
> +SRC_DIR.resolve(TEST_MODULE).toString() + "\\*.java",
> +"--help"))
> +.outputTo(System.out)
> +.errorTo(System.out)
> +.shouldContain("F--help")
> +.shouldContain("module-info.java");
> +}
> +
> +
> +/**
> + * Test that --module= is terminating for VM argument processing just 
> like --module
> + */
> +public void testLongFormModuleOptionTermination() throws Exception {
> +String dir = MODS_DIR.toString();
> +String mid = TEST_MODULE + "/" + MAIN_CLASS;
> +
> +// java --module-path=mods --module=$TESTMODULE/$MAINCLASS 
> --module-path=mods --module=$TESTMODULE/$MAINCLASS
> +// The first --module= will terminate the VM arguments processing. 
> The second 

Re: [EXTERNAL] JDK-8234076 bug fix candidate

[Henry Jen]

dash.
> + *
> + * These tests are not expected to work on *nixes, and are ignored.
> + */
> +public void testWindowsWithLongFormModuleOption() throws Exception {
> +if (!IS_WINDOWS) {
> +return;
> +}
> +
> +String dir = MODS_DIR.toString();
> +String mid = TEST_MODULE + "/" + MAIN_CLASS;
> +
> +// java --module-path=mods --module=$TESTMODULE/$MAINCLASS --help
> +// We should be able to find the argument --help as an application 
> argument
> +ProcessTools.executeProcess(
> +createProcessWithLauncherDebugging(
> +"--module-path=" + dir,
> +"--module=" + mid,
> +"--help"))
> +.outputTo(System.out)
> +.errorTo(System.out)
> +.shouldContain("F--help");
> +
> +// java --module-path=mods --module=$TESTMODULE/$MAINCLASS 
> <...src/test>/*.java --help
> +// We should be able to see argument expansion happen
> +ProcessTools.executeProcess(
> +createProcessWithLauncherDebugging(
> +"--module-path=" + dir,
> +"--module=" + mid,
> +SRC_DIR.resolve(TEST_MODULE).toString() + "\\*.java",
> +"--help"))
> +.outputTo(System.out)
> +.errorTo(System.out)
> +.shouldContain("F--help")
> +.shouldContain("module-info.java");
> +}
> +
> +
> +/**
> + * Test that --module= is terminating for VM argument processing just 
> like --module
> + */
> +public void testLongFormModuleOptionTermination() throws Exception {
> +String dir = MODS_DIR.toString();
> +String mid = TEST_MODULE + "/" + MAIN_CLASS;
> +
> +// java --module-path=mods --module=$TESTMODULE/$MAINCLASS 
> --module-path=mods --module=$TESTMODULE/$MAINCLASS
> +// The first --module= will terminate the VM arguments processing. 
> The second pair of module-path and module will be
> +// deemed as application arguments
> +OutputAnalyzer output = ProcessTools.executeProcess(
> +createProcessWithLauncherDebugging(
> +"--module-path=" + dir,
> +"--module=" + mid,
> +"--module-path=" + dir,
> +"--module=" + mid))
> +.outputTo(System.out)
> +.errorTo(System.out)
> +.shouldContain("argv[ 0] = '--module-path=" + dir)
> +.shouldContain("argv[ 1] = '--module=" + mid);
> +
> +if (IS_WINDOWS) {
> +output.shouldContain("F--module-path=" + 
> dir).shouldContain("F--module=" + mid);
> +}
> +
> +// java --module=$TESTMODULE/$MAINCLASS --module-path=mods
> +// This command line will not work as --module= is terminating and 
> the module will be not found
> +int exitValue = exec("--module=" + mid, "--module-path" + dir);
> +assertTrue(exitValue != 0);
> +}
> }
> 
> 
> -Original Message-
> From: Henry Jen  
> Sent: December 6, 2019 12:03 AM
> To: Nikola Grcevski 
> Cc: Kumar Srinivasan ; Alan Bateman 
> ; core-libs-dev@openjdk.java.net
> Subject: Re: [EXTERNAL] JDK-8234076 bug fix candidate
> 
> 
>> On Dec 5, 2019, at 5:57 PM, Nikola Grcevski  
>> wrote:
>> 
>> Hello all again! 
>> 
>> Based on the suggestion by Kumar I made the following small patch to 
>> checkArg src/java.base/share/native/libjli/args.c:
>> 
>> --- a/src/java.base/share/native/libjli/args.c
>> +++ b/src/java.base/share/native/libjli/args.c
>> @@ -130,6 +130,8 @@ static void checkArg(const char *arg) {
>>}
>>} else if (JLI_StrCmp(arg, "--disable-@files") == 0) {
>>stopExpansion = JNI_TRUE;
>> +} else if (JLI_StrNCmp(arg, "--module=", 9) == 0) {
> 
> I would suggest use JLI_StrCCmp as in java.c. I think combine this fix with 
> origin crash prevention for -1 is a safe approach and most compatible to 
> current behavior.
> 
> BTW, we need the patch to be hosted on cr.openjdk.java.net or you can attach 
> the patch to the review thread if you are not yet an author.
> 
> Cheers,
> Henry
> 
> 
>> +idx = argsCount;
>>}
>>} else {
>>if (!expectingNoDashArg) {
>> 
>> The change is in common code and simply checks for the usage of --modul

Re: [EXTERNAL] JDK-8234076 bug fix candidate

[Henry Jen]

> On Dec 5, 2019, at 5:57 PM, Nikola Grcevski  
> wrote:
> 
> Hello all again! 
> 
> Based on the suggestion by Kumar I made the following small patch to checkArg 
> src/java.base/share/native/libjli/args.c:
> 
> --- a/src/java.base/share/native/libjli/args.c
> +++ b/src/java.base/share/native/libjli/args.c
> @@ -130,6 +130,8 @@ static void checkArg(const char *arg) {
> }
> } else if (JLI_StrCmp(arg, "--disable-@files") == 0) {
> stopExpansion = JNI_TRUE;
> +} else if (JLI_StrNCmp(arg, "--module=", 9) == 0) {

I would suggest use JLI_StrCCmp as in java.c. I think combine this fix with 
origin crash prevention for -1 is a safe approach and most compatible to 
current behavior.

BTW, we need the patch to be hosted on cr.openjdk.java.net or you can attach 
the patch to the review thread if you are not yet an author.

Cheers,
Henry


> +idx = argsCount;
> }
> } else {
> if (!expectingNoDashArg) {
> 
> The change is in common code and simply checks for the usage of --module= and 
> deems the next argument as the start of the application arguments. I can 
> confirm that using -m instead of --module doesn't allow for the = separator 
> to be used, so we only need to check for --module= if this is a desired 
> change.
> 
> I tested with various combinations on the command line and I couldn't find a 
> set of arguments ordering that breaks the terminating quality of --module.
> 
> I also performed series of tests to try to break the argument expansion on 
> Windows with this change, but it worked in all instances. The testcase is 
> working correctly with this change, as well as the javac launcher command as 
> proposed by Kumar (java --module-path=mods 
> --module=jdk.compiler/com.sun.tools.javac.Main *.java).
> 
> I ran all the launcher tests on both Windows and Linux and all tests pass.
> 
> Please let me know if this is a preferred approach to address this bug or if 
> you think there's a potential problem with the change. If this is an 
> acceptable fix I can create new webrev with set of tests for the various edge 
> cases I tried, and new launcher specific tests that ensure argument expansion 
> is performed on Windows with this module usage.
> 
> Thank you,
> Nikola
> 
> -Original Message-
> From: Henry Jen  
> Sent: December 4, 2019 8:26 PM
> To: Kumar Srinivasan ; Alan Bateman 
> ; Nikola Grcevski 
> Cc: core-libs-dev@openjdk.java.net
> Subject: Re: [EXTERNAL] JDK-8234076 bug fix candidate
> 
>> On Dec 4, 2019, at 1:15 PM, Kumar Srinivasan  wrote:
>> 
>> Hi Nikola,
>> 
>> It looks ok to me, I will leave it to Henry and Alan to bless this.
>> 
>> IMHO: I think we should fix it correctly now than later, I don't think 
>> it is all that difficult AFAICT all the launcher has  to do is 
>> identify "--module==", then the next argument is the first index.
>> 
> 
> I don’t disagree, if we can decide whether —module= is allowed. Based on my 
> understanding and the document for java[1], the —module= is not necessarily 
> correct.
> 
> If we decided to accept it, the fix would be make sure the index set 
> correctly as Kumar suggested, and the fix can be relatively simple.
> 
> FWIW, it’s still possible the index is NOT_FOUND if there is no main class 
> specified, but that should never cause crash as if no main class is found, 
> the launcher should either execute other terminal argument or display help.
> 
> I agree the fix is not complete as it only make sure no crash can happen. It 
> doesn’t actually make —module= illegal and show help in case of that. At this 
> point, there is a discrepancy in launcher code regard —module usage, and we 
> need to fix that.
> 
> I’ll let Alan to comment about the —module option usage.
> 
> The webrev looks good to me, although I would like to see the test be more 
> close to other arguments processing test, but since this can only be 
> reproduced with —module= usage, I assume this is not bad.
> 
> [1] 
> https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fdocs.oracle.com%2Fen%2Fjava%2Fjavase%2F13%2Fdocs%2Fspecs%2Fman%2Fjava.htmldata=02%7C01%7CNikola.Grcevski%40microsoft.com%7C6158f8460dcd4c39518708d7792228c5%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637111061023797544sdata=uO2tgi1QNvXgI0wT%2FxOxB%2Bh10YpCbq37uzkKKlByUYg%3Dreserved=0
> 
>> Kumar
>> 
>> On Tue, Dec 3, 2019 at 5:29 PM Nikola Grcevski 
>>  wrote:
>> Hi Henry and Kumar,
>> 
>> Thanks again for your comments! I have updated the test to be part of 
>> test/jdk/tools/launcher/modules/basic, it took a lot less code to achieve 
>> the same a

Re: RFR: JEP 367: Remove the Pack200 Tools and API

[Henry Jen]

OK, so I created an issue[1] for follow up for Windows build and reverted the 
change in flags-cflags.m4, if nothing else, I’ll push without another webrev 
pinging that I get an +1 from someone in build-de, Erik?

[1] https://bugs.openjdk.java.net/browse/JDK-8235461

Cheers,
Henry

> On Dec 5, 2019, at 12:21 PM, Mandy Chung  wrote:
> 
> 
> 
> On 12/5/19 12:41 AM, Alan Bateman wrote:
>> On 05/12/2019 08:16, Henry Jen wrote:
>>> Hi,
>>> 
>>> Updated webrev[1] reflect comments since last webrev. Vicente had done all 
>>> the heavy-lifting and hand over to me to finish up.
>>> 
>>> Changes to symbols is reverted, as we expect that only need to be updated 
>>> in next release by running make/scripts/generate-symbol-data.sh.
>>> 
>>> The jar files are confusing in the webrev, but those files are removed. The 
>>> whole test/jdk/tools/pack200 is removed.
>>> 
>>> Cheers,
>>> Henry
>>> 
>>> [1] http://cr.openjdk.java.net/~henryjen/jdk14/8234542/0/webrev/
>>> 
>> The update webrev looks okay to me, except this part of the comment in 
>> flags-cflags.m4
>> 
>> "Now that unpack200 has been removed we should consider setting it for 
>> windows too. but this could be done as a follow-up effort. It could be that 
>> other other clients are relying on the current configuration for windows".
>> 
>> I think it would be best to create an infrastructure/build issue and leave 
>> most of this  confusing comment out.
>> 
> 
> I also think keeping flags-cflags.m4 as is and file a new build issue as a 
> follow-up would be better.
> 
> Otherwise, this updated webrev looks okay to me.
> 
> Mandy

Re: RFR: JEP 367: Remove the Pack200 Tools and API

[Henry Jen]

Hi,

Updated webrev[1] reflect comments since last webrev. Vicente had done all the 
heavy-lifting and hand over to me to finish up.

Changes to symbols is reverted, as we expect that only need to be updated in 
next release by running make/scripts/generate-symbol-data.sh. 

The jar files are confusing in the webrev, but those files are removed. The 
whole test/jdk/tools/pack200 is removed.

Cheers,
Henry

[1] http://cr.openjdk.java.net/~henryjen/jdk14/8234542/0/webrev/


> On Dec 2, 2019, at 6:50 PM, Joe Darcy  wrote:
> 
> Hi Vicente,
> 
> It looks like the update to make/data/symbols/symbols removes the jdk.pack 
> module from the history JDKs 9, 10, and 11 when --release is used.
> 
> If that is the case, it would be incorrect since historically the jdk.pack 
> module was present in those releases.
> 
> Thanks,
> 
> -Joe
> 
> On 11/22/2019 1:30 PM, Vicente Romero wrote:
>> Hi all,
>> 
>> On 11/22/19 3:21 AM, Alan Bateman wrote:
>>> 
>>> 
>>> On 21/11/2019 19:53, Vicente Romero wrote:
 Hi,
 
 I think I have covered all the proposed fixes so far. This is the last 
 iteration of the webrev [1], all the current changes are in this one, the 
 code hasn't been split into different webrevs. I'm also forwarding to 
 build-dev as there are some build related changes too. The CSR for this 
 change is at [2]
>>> Would it be possible to summarize what will remain in 
>>> test/jdk/tools/pack200 after this removal? The webrev makes it looks like 
>>> badattr.jar is being added but since it already exists then I'm not sure 
>>> whether to believe it. pack200-verifier/data/golden.jar is another one as 
>>> it looks like JAR file that is generated by the tests today is being 
>>> checked in, maybe `hg add` in error?
>> 
>> I don't know why it is shown as added in the webrev, they have been removed. 
>> I have published another iteration of the webrev at [1]
>>> 
>>> The change to flags-cflag.m4 to add LP64=1 on Windows will need eyes, it's 
>>> not immediately obvious to me which shared code compiled on Windows is 
>>> impacted by this.
>> 
>> yes probably this change is risky. I did it as the comment suggested that 
>> the only reason the treatment for Windows was different was because of 
>> pack200. But not sure if we can trust that comment. Should I restore this 
>> code to its original state?
>>> 
>>> -Alan
>> 
>> 
>> Thanks,
>> Vicente
>> 
>> [1] http://cr.openjdk.java.net/~vromero/8234542/webrev.03/

Re: [EXTERNAL] JDK-8234076 bug fix candidate

[Henry Jen]

> On Dec 4, 2019, at 1:15 PM, Kumar Srinivasan  wrote:
> 
> Hi Nikola,
> 
> It looks ok to me, I will leave it to Henry and Alan to bless this.
> 
> IMHO: I think we should fix it correctly now than later, I don't think it is 
> all that 
> difficult AFAICT all the launcher has  to do is identify "--module==", then
> the next argument is the first index.
> 

I don’t disagree, if we can decide whether —module= is allowed. Based on my 
understanding and the document for java[1], the —module= is not necessarily 
correct.

If we decided to accept it, the fix would be make sure the index set correctly 
as Kumar suggested, and the fix can be relatively simple.

FWIW, it’s still possible the index is NOT_FOUND if there is no main class 
specified, but that should never cause crash as if no main class is found, the 
launcher should either execute other terminal argument or display help.

I agree the fix is not complete as it only make sure no crash can happen. It 
doesn’t actually make —module= illegal and show help in case of that. At this 
point, there is a discrepancy in launcher code regard —module usage, and we 
need to fix that.

I’ll let Alan to comment about the —module option usage.

The webrev looks good to me, although I would like to see the test be more 
close to other arguments processing test, but since this can only be reproduced 
with —module= usage, I assume this is not bad.

[1] https://docs.oracle.com/en/java/javase/13/docs/specs/man/java.html

> Kumar
> 
> On Tue, Dec 3, 2019 at 5:29 PM Nikola Grcevski 
>  wrote:
> Hi Henry and Kumar,
> 
> Thanks again for your comments! I have updated the test to be part of 
> test/jdk/tools/launcher/modules/basic, it took a lot less code to achieve the 
> same amount of testing. I added a new test method inside BasicTest.java and 
> tested on both Windows and Linux.
> 
> Please find my updated webrev here for your review: 
> https://grcevski.github.io/JDK-8234076/webrev/
> 
> Cheers,
> 
> Nikola
> 
> -Original Message-
> From: Henry Jen  
> Sent: December 3, 2019 11:39 AM
> To: Kumar Srinivasan 
> Cc: Nikola Grcevski ; Alan Bateman 
> ; core-libs-dev@openjdk.java.net
> Subject: Re: [EXTERNAL] JDK-8234076 bug fix candidate
> 
> Kumar,
> 
> Great to have you look at this, you are correct, this patch doesn’t address 
> the wildcard expansion issue, but only to address the potential crash if a 
> main class is not specified as Nikola pointed out. 
> 
> We definitely need a follow up to fix wildcard expansion. The pointer to 
> simplify the test is helpful, it would make the test more obvious.
> 
> Cheers,
> Henry
> 
> > On Dec 3, 2019, at 7:14 AM, Kumar Srinivasan  wrote:
> > 
> > Hi,
> > 
> > Sorry for chiming in  late in the review process, for what it's worth
> > 
> > 1. It is not at all clear to me if this solution is correct, yes it averts 
> > the problem of not finding the main-class
> > and subsequent crash,  but it does not address  wildcard arguments 
> > expansion.
> > 
> > What if we have
> > % java --module-path=mods 
> > --module=jdk.compiler/com.sun.tools.javac.Main *.java
> > Where jdk.compiler is a java compiler implementation (javac).
> > The user would expect the above compiler module to build all the .java 
> > files in that directory, 
> > and this fix will not address that.
> > 
> > Some background:
> > https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugs.openjdk.java.net%2Fbrowse%2FJDK-7146424data=02%7C01%7CNikola.Grcevski%40microsoft.com%7C7b1b46aa46024285881108d7780f452c%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637109879293524293sdata=9KSksL8%2BCmXSscF8oGGn5piLz2wApQ9paJUyZWbKWCw%3Dreserved=0
> > Please see all the related bugs in the above JIRA issue.
> > 
> > Paragraph #6 in this interview surmises the wild card behavior on  Windows:
> > https://nam06.safelinks.protection.outlook.com/?url=https:%2F%2Fwww.princeton.edu%2F~hos%2Fmike%2Ftranscripts%2Fkernighan.htmdata=02%7C01%7CNikola.Grcevski%40microsoft.com%7C7b1b46aa46024285881108d7780f452c%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637109879293524293sdata=l20J1AN4vBmT19gzBxLOktBsdv260F2rMWRvCLeVb84%3Dreserved=0
> > 
> > 2. Though the arguments related tests are in Aaarghs.java the modules 
> > related tests for the launcher are at:
> > https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fhg.openjdk.java.net%2Fjdk%2Fjdk13%2Ffile%2F0368f3a073a9%2Ftest%2Fjdk%2Ftools%2Flauncher%2Fmodules%2Fbasicdata=02%7C01%7CNikola.Grcevski%40microsoft.com%7C7b1b46aa46024285881108d7780f452c%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637109879293524293sdata=jsOjS1rgX4tfzJwE8Xif3NARZPRHb39Y64LvSdz1Jic%3Dre

Re: [EXTERNAL] JDK-8234076 bug fix candidate

[Henry Jen]

Kumar,

Great to have you look at this, you are correct, this patch doesn’t address the 
wildcard expansion issue, but only to address the potential crash if a main 
class is not specified as Nikola pointed out. 

We definitely need a follow up to fix wildcard expansion. The pointer to 
simplify the test is helpful, it would make the test more obvious.

Cheers,
Henry

> On Dec 3, 2019, at 7:14 AM, Kumar Srinivasan  wrote:
> 
> Hi,
> 
> Sorry for chiming in  late in the review process, for what it's worth
> 
> 1. It is not at all clear to me if this solution is correct, yes it averts 
> the problem of not finding the main-class
> and subsequent crash,  but it does not address  wildcard arguments 
> expansion.
> 
> What if we have
> % java --module-path=mods --module=jdk.compiler/com.sun.tools.javac.Main 
> *.java
> Where jdk.compiler is a java compiler implementation (javac).
> The user would expect the above compiler module to build all the .java 
> files in that directory, 
> and this fix will not address that.
> 
> Some background:
> https://bugs.openjdk.java.net/browse/JDK-7146424
> Please see all the related bugs in the above JIRA issue.
> 
> Paragraph #6 in this interview surmises the wild card behavior on  Windows:
> https://www.princeton.edu/~hos/mike/transcripts/kernighan.htm
> 
> 2. Though the arguments related tests are in Aaarghs.java the modules related 
> tests for the launcher are at:
> https://hg.openjdk.java.net/jdk/jdk13/file/0368f3a073a9/test/jdk/tools/launcher/modules/basic
> Using the modules test framework may make the test simpler.
> 
> Kumar Srinivasan
> 
> 
> On Mon, Dec 2, 2019 at 11:34 AM Nikola Grcevski 
>  wrote:
> Hi Alan and Henry,
> 
> Thank you for reviewing my email! Henry's observation matches mine, the 
> shared common code for all platforms in checkArg 
> (src/java.base/share/native/libjli/args.c) can potentially leave the 
> firstAppArgIndex static set to -1, if all passed command line arguments are 
> prefixed with a dash. Later on the arguments are validated, however we might 
> crash before then on Windows because of the negative index access. In this 
> case, the customer command was valid (modules usage) and the program runs 
> successfully.
> 
> I created a webrev here for the change, including a new test in Arrrghs.java:
> 
> https://grcevski.github.io/JDK-8234076/webrev/
> 
> I copied the test validation and assertion style of other code inside the 
> test class.
> 
> Please let me know if you have any comments or suggestions.
> 
> Thanks again!
> 
> -Original Message-
> From: Henry Jen  
> Sent: December 2, 2019 12:26 PM
> To: Alan Bateman 
> Cc: Nikola Grcevski ; 
> core-libs-dev@openjdk.java.net
> Subject: [EXTERNAL] Re: JDK-8234076 bug fix candidate
> 
> The fix looks reasonable to me, basically, if the command argument format is 
> not legal, it’s possible we won’t find the main class when doing argument 
> file expansion, and the index is -1 which could cause crash on Windows 
> platform for the wildcard processing.
> 
> I think we should add a test case for this, probably in 
> test/jdk/tools/launcher/Arrrghs.java.
> 
> As I understand it, the argument validation is done in a later stage after 
> calling JLI_Launch.
> 
> Cheers,
> Henry
> 
> 
> > On Dec 2, 2019, at 2:12 AM, Alan Bateman  wrote:
> > 
> > On 20/11/2019 19:42, Nikola Grcevski wrote:
> >> :
> >> 
> >> Please let me know if this approach is acceptable for the current bug 
> >> report and I'll make a webrev and include appropriate launcher tests. I 
> >> was thinking the tests should do extra validation on the output from 
> >> _JAVA_LAUNCHER_DEBUG on Windows.
> >> 
> > I think you're in the right area but I would have expected the arg index to 
> > 0 here. Henry Jen (cc'ed) might have some comments on this.
> > 
> > -Alan
> 

Re: JDK-8234076 bug fix candidate

[Henry Jen]

The fix looks reasonable to me, basically, if the command argument format is 
not legal, it’s possible we won’t find the main class when doing argument file 
expansion, and the index is -1 which could cause crash on Windows platform for 
the wildcard processing.

I think we should add a test case for this, probably in 
test/jdk/tools/launcher/Arrrghs.java.

As I understand it, the argument validation is done in a later stage after 
calling JLI_Launch.

Cheers,
Henry


> On Dec 2, 2019, at 2:12 AM, Alan Bateman  wrote:
> 
> On 20/11/2019 19:42, Nikola Grcevski wrote:
>> :
>> 
>> Please let me know if this approach is acceptable for the current bug report 
>> and I'll make a webrev and include appropriate launcher tests. I was 
>> thinking the tests should do extra validation on the output from 
>> _JAVA_LAUNCHER_DEBUG on Windows.
>> 
> I think you're in the right area but I would have expected the arg index to 0 
> here. Henry Jen (cc'ed) might have some comments on this.
> 
> -Alan

Re: RFR: 8231863: Crash if classpath is read from @argument file and the main gets option argument

[Henry Jen]

Thanks Alan and Mandy for the review.

I am guessing the Alan’s preference to use Files.write(aFilePath, lines) is to 
avoid extra String.join operation, which I would too. The current version with 
byte[] is more flexible but we most likely won't need it in launcher. Anyway, I 
don’t think the difference would be noticeable with launcher tests.

The updated webrev[1] takes a boolean to decide which Files.write to use but 
gives up the byte[] version.

Cheers,
Henry

[1] http://cr.openjdk.java.net/~henryjen/jdk/8231863.1/webrev/

> On Nov 9, 2019, at 11:33 AM, Mandy Chung  wrote:
> 
> The patch looks fine to me as well.
> 
> As for the test, perhaps adding a new createAFile(File aFile, List 
> lines, boolean hasTrailingBlankLine) in TestHelper instead may help avoid any 
> confusion.
> 
> 
> Mandy
> 
> On 11/8/19 7:43 AM, Mat Carter wrote:
>> Hi Alan
>> 
>> The method you propose: [nio.]Files[.write](aFile.toPath, lines) adds a 
>> trailing blank line to the file; the regression test needs to generate a 
>> file without a trailing blank line as this is the condition in which the bug 
>> occurs.  This is why it now writes out an array of bytes
>> 
>> Cheers
>> Mat
>> 
>> 
>> From: core-libs-dev  on behalf of 
>> Alan Bateman 
>> Sent: Friday, November 8, 2019 2:56 AM
>> To: Henry Jen ; core-libs-dev@openjdk.java.net 
>> 
>> Subject: Re: RFR: 8231863: Crash if classpath is read from @argument file 
>> and the main gets option argument
>> 
>> On 07/11/2019 22:55, Henry Jen wrote:
>>> Hi,
>>> 
>>> Please review the webrev[1], contributed by Mat Carter. You can find the 
>>> bug details at JBS[2]. I have reviewed and tested the fix, I still need an 
>>> official review before I can push this.
>>> 
>> Looks okay although in the test, the createAFile helper method could be
>> replaced with Files(aFile.toPath, lines) and that would avoid the need
>> to concatenate all the lines. You can specify the defaultCharset to that
>> method if you need really it.
>> 
>> -Alan
> 

RFR: 8231863: Crash if classpath is read from @argument file and the main gets option argument

[Henry Jen]

Hi,

Please review the webrev[1], contributed by Mat Carter. You can find the bug 
details at JBS[2]. I have reviewed and tested the fix, I still need an official 
review before I can push this.

Cheers,
Henry


[1] http://cr.openjdk.java.net/~henryjen/jdk/8231863.0/webrev/
[2] https://bugs.openjdk.java.net/browse/JDK-8231863


> On Nov 4, 2019, at 3:40 PM, Mat Carter  wrote:
> 
> Thanks Henry
> 
> I'm working on a test, do you want me to share directly with you (incl. 
> questions) or continue with comms through the mailing list
> 
> Cheers
> Mat
> 
> From: Henry Jen 
> Sent: Monday, November 4, 2019 12:47 PM
> To: Mat Carter 
> Cc: core-libs-dev@openjdk.java.net 
> Subject: Re: JDK-8231863 bug fix candidate
>  
> Hi Mat,
> 
> I’ll sponsor this fix, let me know if you are coming up with test and I’ll 
> work with you to get your patch committed.
> 
> Cheers,
> Henry
> 
> 
> > On Nov 4, 2019, at 9:21 AM, Henry Jen  wrote:
> > 
> > The fix is on the spot, good catch.
> > 
> > As the test, test/jdk/tools/launcher/ArgsFileTest.java is testing the file 
> > expansions, we can add a test case there to have an argument file not have 
> > newline at the end, and check that we get correct arguments.
> > 
> > Cheers,
> > Henry
> > 
> > 
> >> On Nov 1, 2019, at 7:06 AM, Mat Carter  
> >> wrote:
> >> 
> >> Hello core-libs-dev
> >> 
> >> I'm a VM engineer at Microsoft, now that we've signed the OCA (news of 
> >> which was shared by Bruno Borges in the discuss mailing list) I wanted to 
> >> pick up an issue in order to gain familiarity with the change process
> >> 
> >> After reproducing the error reported in "JDK-8231863: Crash if classpath 
> >> is read from @argument file and the main gets option argument" 
> >> (https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugs.openjdk.java.net%2Fbrowse%2FJDK-8231863data=02%7C01%7CMatthew.Carter%40microsoft.com%7C657f0afad82b4d31670a08d761682b0b%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637085026486748197sdata=19S%2BLGCPaoy0h2YmbKrKfsC21d6B43rITkZhnAKyuHw%3Dreserved=0),
> >>  I've identified the root cause and a candidate fix.
> >> 
> >> As such I'd like to pick this issue up, assuming its current state of 
> >> unassigned still holds what's the process of having it assigned to me or 
> >> having it somehow reserved as I don't want to cause unnecessary duplicated 
> >> work.
> >> 
> >> As well as validating the fix in the debugger on WIndows and Linux, I've 
> >> run the AdoptOpenJDK sanity tests on Mac, Windows and Linux for both tip 
> >> and 11u.
> >> 
> >> I'm currently figuring out where/how to write regression tests for this 
> >> case; input from this mailing list would be welcomed (examples of tests 
> >> for similar bugs etc)
> >> 
> >> This error occurs on all platforms (tip + 11u) but only results in a crash 
> >> on Windows in java_md.c due to that platforms dependency on 
> >> JLI_GetAppArgIndex().  Note that while the error is 100% reproducible, the 
> >> crash only occurs <10%.  The following change fixes that problem by 
> >> passing in the final token fragment from the argument file into the state 
> >> machine via checkArg()
> >> 
> >> --- a/src/java.base/share/native/libjli/args.c
> >> +++ b/src/java.base/share/native/libjli/args.c
> >> @@ -337,7 +337,9 @@ static JLI_List readArgFile(FILE *file) {
> >> // remaining partial token
> >> if (ctx.state == IN_TOKEN || ctx.state == IN_QUOTE) {
> >> if (ctx.parts->size != 0) {
> >> -JLI_List_add(rv, JLI_List_combine(ctx.parts));
> >> +token = JLI_List_combine(ctx.parts);
> >> +checkArg(token);
> >> +JLI_List_add(rv, token);
> >> }
> >> }
> >> JLI_List_free(ctx.parts);
> >> 
> >> The fix tackles the memory corruption indirectly.  Further preventative 
> >> changes could be made in java_md.c/CreateApplicationArgs to avoid future 
> >> memory corruptions (caused by JLI_GetAppArgIndex() returning -1 in this 
> >> case) ; but I felt that that handling those error cases ran secondary to 
> >> addressing the bug in the argument file parsing code.
> >> 
> >> Cheers
> >> Mat
> > 
> 

Re: JDK-8231863 bug fix candidate

[Henry Jen]

Whatever you are comfortable with, we will have the final patch reviewed on the 
public list though.

Please keep me in the direct email recipient to make sure I read the email in 
time.

Cheers,
Henry


> On Nov 4, 2019, at 3:40 PM, Mat Carter  wrote:
> 
> Thanks Henry
> 
> I'm working on a test, do you want me to share directly with you (incl. 
> questions) or continue with comms through the mailing list
> 
> Cheers
> Mat
> 
> From: Henry Jen 
> Sent: Monday, November 4, 2019 12:47 PM
> To: Mat Carter 
> Cc: core-libs-dev@openjdk.java.net 
> Subject: Re: JDK-8231863 bug fix candidate
>  
> Hi Mat,
> 
> I’ll sponsor this fix, let me know if you are coming up with test and I’ll 
> work with you to get your patch committed.
> 
> Cheers,
> Henry
> 
> 
> > On Nov 4, 2019, at 9:21 AM, Henry Jen  wrote:
> > 
> > The fix is on the spot, good catch.
> > 
> > As the test, test/jdk/tools/launcher/ArgsFileTest.java is testing the file 
> > expansions, we can add a test case there to have an argument file not have 
> > newline at the end, and check that we get correct arguments.
> > 
> > Cheers,
> > Henry
> > 
> > 
> >> On Nov 1, 2019, at 7:06 AM, Mat Carter  
> >> wrote:
> >> 
> >> Hello core-libs-dev
> >> 
> >> I'm a VM engineer at Microsoft, now that we've signed the OCA (news of 
> >> which was shared by Bruno Borges in the discuss mailing list) I wanted to 
> >> pick up an issue in order to gain familiarity with the change process
> >> 
> >> After reproducing the error reported in "JDK-8231863: Crash if classpath 
> >> is read from @argument file and the main gets option argument" 
> >> (https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugs.openjdk.java.net%2Fbrowse%2FJDK-8231863data=02%7C01%7CMatthew.Carter%40microsoft.com%7C657f0afad82b4d31670a08d761682b0b%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637085026486748197sdata=19S%2BLGCPaoy0h2YmbKrKfsC21d6B43rITkZhnAKyuHw%3Dreserved=0),
> >>  I've identified the root cause and a candidate fix.
> >> 
> >> As such I'd like to pick this issue up, assuming its current state of 
> >> unassigned still holds what's the process of having it assigned to me or 
> >> having it somehow reserved as I don't want to cause unnecessary duplicated 
> >> work.
> >> 
> >> As well as validating the fix in the debugger on WIndows and Linux, I've 
> >> run the AdoptOpenJDK sanity tests on Mac, Windows and Linux for both tip 
> >> and 11u.
> >> 
> >> I'm currently figuring out where/how to write regression tests for this 
> >> case; input from this mailing list would be welcomed (examples of tests 
> >> for similar bugs etc)
> >> 
> >> This error occurs on all platforms (tip + 11u) but only results in a crash 
> >> on Windows in java_md.c due to that platforms dependency on 
> >> JLI_GetAppArgIndex().  Note that while the error is 100% reproducible, the 
> >> crash only occurs <10%.  The following change fixes that problem by 
> >> passing in the final token fragment from the argument file into the state 
> >> machine via checkArg()
> >> 
> >> --- a/src/java.base/share/native/libjli/args.c
> >> +++ b/src/java.base/share/native/libjli/args.c
> >> @@ -337,7 +337,9 @@ static JLI_List readArgFile(FILE *file) {
> >> // remaining partial token
> >> if (ctx.state == IN_TOKEN || ctx.state == IN_QUOTE) {
> >> if (ctx.parts->size != 0) {
> >> -JLI_List_add(rv, JLI_List_combine(ctx.parts));
> >> +token = JLI_List_combine(ctx.parts);
> >> +checkArg(token);
> >> +JLI_List_add(rv, token);
> >> }
> >> }
> >> JLI_List_free(ctx.parts);
> >> 
> >> The fix tackles the memory corruption indirectly.  Further preventative 
> >> changes could be made in java_md.c/CreateApplicationArgs to avoid future 
> >> memory corruptions (caused by JLI_GetAppArgIndex() returning -1 in this 
> >> case) ; but I felt that that handling those error cases ran secondary to 
> >> addressing the bug in the argument file parsing code.
> >> 
> >> Cheers
> >> Mat
> > 
> 

Re: JDK-8231863 bug fix candidate

[Henry Jen]

Hi Mat,

I’ll sponsor this fix, let me know if you are coming up with test and I’ll work 
with you to get your patch committed.

Cheers,
Henry


> On Nov 4, 2019, at 9:21 AM, Henry Jen  wrote:
> 
> The fix is on the spot, good catch.
> 
> As the test, test/jdk/tools/launcher/ArgsFileTest.java is testing the file 
> expansions, we can add a test case there to have an argument file not have 
> newline at the end, and check that we get correct arguments.
> 
> Cheers,
> Henry
> 
> 
>> On Nov 1, 2019, at 7:06 AM, Mat Carter  wrote:
>> 
>> Hello core-libs-dev
>> 
>> I'm a VM engineer at Microsoft, now that we've signed the OCA (news of which 
>> was shared by Bruno Borges in the discuss mailing list) I wanted to pick up 
>> an issue in order to gain familiarity with the change process
>> 
>> After reproducing the error reported in "JDK-8231863: Crash if classpath is 
>> read from @argument file and the main gets option argument" 
>> (https://bugs.openjdk.java.net/browse/JDK-8231863), I've identified the root 
>> cause and a candidate fix.
>> 
>> As such I'd like to pick this issue up, assuming its current state of 
>> unassigned still holds what's the process of having it assigned to me or 
>> having it somehow reserved as I don't want to cause unnecessary duplicated 
>> work.
>> 
>> As well as validating the fix in the debugger on WIndows and Linux, I've run 
>> the AdoptOpenJDK sanity tests on Mac, Windows and Linux for both tip and 11u.
>> 
>> I'm currently figuring out where/how to write regression tests for this 
>> case; input from this mailing list would be welcomed (examples of tests for 
>> similar bugs etc)
>> 
>> This error occurs on all platforms (tip + 11u) but only results in a crash 
>> on Windows in java_md.c due to that platforms dependency on 
>> JLI_GetAppArgIndex().  Note that while the error is 100% reproducible, the 
>> crash only occurs <10%.  The following change fixes that problem by passing 
>> in the final token fragment from the argument file into the state machine 
>> via checkArg()
>> 
>> --- a/src/java.base/share/native/libjli/args.c
>> +++ b/src/java.base/share/native/libjli/args.c
>> @@ -337,7 +337,9 @@ static JLI_List readArgFile(FILE *file) {
>> // remaining partial token
>> if (ctx.state == IN_TOKEN || ctx.state == IN_QUOTE) {
>> if (ctx.parts->size != 0) {
>> -JLI_List_add(rv, JLI_List_combine(ctx.parts));
>> +token = JLI_List_combine(ctx.parts);
>> +checkArg(token);
>> +JLI_List_add(rv, token);
>> }
>> }
>> JLI_List_free(ctx.parts);
>> 
>> The fix tackles the memory corruption indirectly.  Further preventative 
>> changes could be made in java_md.c/CreateApplicationArgs to avoid future 
>> memory corruptions (caused by JLI_GetAppArgIndex() returning -1 in this 
>> case) ; but I felt that that handling those error cases ran secondary to 
>> addressing the bug in the argument file parsing code.
>> 
>> Cheers
>> Mat
> 

Re: JDK-8231863 bug fix candidate

[Henry Jen]

The fix is on the spot, good catch.

As the test, test/jdk/tools/launcher/ArgsFileTest.java is testing the file 
expansions, we can add a test case there to have an argument file not have 
newline at the end, and check that we get correct arguments.

Cheers,
Henry


> On Nov 1, 2019, at 7:06 AM, Mat Carter  wrote:
> 
> Hello core-libs-dev
> 
> I'm a VM engineer at Microsoft, now that we've signed the OCA (news of which 
> was shared by Bruno Borges in the discuss mailing list) I wanted to pick up 
> an issue in order to gain familiarity with the change process
> 
> After reproducing the error reported in "JDK-8231863: Crash if classpath is 
> read from @argument file and the main gets option argument" 
> (https://bugs.openjdk.java.net/browse/JDK-8231863), I've identified the root 
> cause and a candidate fix.
> 
> As such I'd like to pick this issue up, assuming its current state of 
> unassigned still holds what's the process of having it assigned to me or 
> having it somehow reserved as I don't want to cause unnecessary duplicated 
> work.
> 
> As well as validating the fix in the debugger on WIndows and Linux, I've run 
> the AdoptOpenJDK sanity tests on Mac, Windows and Linux for both tip and 11u.
> 
> I'm currently figuring out where/how to write regression tests for this case; 
> input from this mailing list would be welcomed (examples of tests for similar 
> bugs etc)
> 
> This error occurs on all platforms (tip + 11u) but only results in a crash on 
> Windows in java_md.c due to that platforms dependency on 
> JLI_GetAppArgIndex().  Note that while the error is 100% reproducible, the 
> crash only occurs <10%.  The following change fixes that problem by passing 
> in the final token fragment from the argument file into the state machine via 
> checkArg()
> 
> --- a/src/java.base/share/native/libjli/args.c
> +++ b/src/java.base/share/native/libjli/args.c
> @@ -337,7 +337,9 @@ static JLI_List readArgFile(FILE *file) {
>  // remaining partial token
>  if (ctx.state == IN_TOKEN || ctx.state == IN_QUOTE) {
>  if (ctx.parts->size != 0) {
> -JLI_List_add(rv, JLI_List_combine(ctx.parts));
> +token = JLI_List_combine(ctx.parts);
> +checkArg(token);
> +JLI_List_add(rv, token);
>  }
>  }
>  JLI_List_free(ctx.parts);
> 
> The fix tackles the memory corruption indirectly.  Further preventative 
> changes could be made in java_md.c/CreateApplicationArgs to avoid future 
> memory corruptions (caused by JLI_GetAppArgIndex() returning -1 in this case) 
> ; but I felt that that handling those error cases ran secondary to addressing 
> the bug in the argument file parsing code.
> 
> Cheers
> Mat

Re: RFR: 8215156: Deprecate the -Xfuture option

[Henry Jen]

> On May 22, 2019, at 7:04 PM, David Holmes  wrote:
> 
> On 23/05/2019 11:56 am, Henry Jen wrote:
>> Thanks for the quick review,
>>> On May 22, 2019, at 6:34 PM, David Holmes  wrote:
>>> 
>>> Hi Henry,
>>> 
>>> On 23/05/2019 11:07 am, Henry Jen wrote:
>>>> Hi,
>>>> Please review a webrev[1] to deprecate the -Xfuture option per 
>>>> CSR-8224524[2].
>>> 
>>> src/hotspot/share/Xusage.txt
>>> 
>>> Ignoring the usefulness, or otherwise of this file, the entry for Xfuture 
>>> should not be deleted (that happens when an option is actually removed) but 
>>> just have "(deprecated)" added to it.
>>> 
>> I was wondering if I should prefix (deprecated) or append to the content. It 
>> make the line over the 80 so I decided just remove it since this document is 
>> not official we don’t want to advocate that option any more.
>> So, we have couple options,
>>  -Xfuture  enable strictest checks, anticipating future default
>>(deprecated)
>>  -Xrs  reduce use of OS signals by Java/VM (see 
>> documentation)
> 
> I prefer option 1.
> 

OK, updated[1].

[1] https://cr.openjdk.java.net/~henryjen/jdk13/8215156/open/1/webrev/

Cheers,
Henry

Re: RFR: 8215156: Deprecate the -Xfuture option

[Henry Jen]

Thanks for the quick review,

> On May 22, 2019, at 6:34 PM, David Holmes  wrote:
> 
> Hi Henry,
> 
> On 23/05/2019 11:07 am, Henry Jen wrote:
>> Hi,
>> Please review a webrev[1] to deprecate the -Xfuture option per 
>> CSR-8224524[2].
> 
> src/hotspot/share/Xusage.txt
> 
> Ignoring the usefulness, or otherwise of this file, the entry for Xfuture 
> should not be deleted (that happens when an option is actually removed) but 
> just have "(deprecated)" added to it.
> 

I was wondering if I should prefix (deprecated) or append to the content. It 
make the line over the 80 so I decided just remove it since this document is 
not official we don’t want to advocate that option any more.

So, we have couple options,


 -Xfuture  enable strictest checks, anticipating future default
   (deprecated)
 -Xrs  reduce use of OS signals by Java/VM (see documentation)

or 

-Xfuture  enable strictest checks (deprecated)

or 

-Xfuture  (deprecated)
  enable strictest checks, anticipating future default

or keep in one line don’t mind the 80 width.

Let me know which do we prefer.

> ---
> 
> In hotspot we include the version of the JDK that deprecated the option:
> 
> "Option %s was deprecated in version %s and will likely be removed in a 
> future release."
> 
> Perhaps you could do the same for the launcher?
> 

During the CSR discussion, we asked Dr Deprecator’s opinion and decided on 
“XXX is deprecated and may be removed in a future release.”

It was suggested to change JVM message to match.

Cheers,
Henry

> Otherwise seems fine.
> 
> Thanks,
> David
> -
> 
>> Cheers,
>> Henry
>>  [1] https://cr.openjdk.java.net/~henryjen/jdk13/8215156/open/webrev/
>> [2] https://bugs.openjdk.java.net/browse/JDK-8224524

RFR: 8215156: Deprecate the -Xfuture option

[Henry Jen]

Hi,

Please review a webrev[1] to deprecate the -Xfuture option per CSR-8224524[2].

Cheers,
Henry
 
[1] https://cr.openjdk.java.net/~henryjen/jdk13/8215156/open/webrev/
[2] https://bugs.openjdk.java.net/browse/JDK-8224524

Re: RFR: 8218997: Xusage text, man help, etc doesn't mention -Xlog option.

[Henry Jen]

It was brought to my attention that the java.1 page is out of date and labeled 
for jdk 8, thus I will withdraw the change for java.1. Only keep the change to 
launcher.properties.

Thanks for the review.

Cheers,
Henry

> On May 21, 2019, at 5:26 PM, David Holmes  wrote:
> 
> That works for me. :)
> 
> Thanks,
> David
> 
> On 22/05/2019 9:40 am, Henry Jen wrote:
>> Good suggestion, didn’t know about that page. I took what’s in Xusage.txt. 
>> Updated as following,
>> diff -r cd3c74c0 
>> src/java.base/share/classes/sun/launcher/resources/launcher.properties
>> --- a/src/java.base/share/classes/sun/launcher/resources/launcher.properties 
>>Tue May 21 15:08:50 2019 -0700
>> +++ b/src/java.base/share/classes/sun/launcher/resources/launcher.properties 
>>Tue May 21 16:38:42 2019 -0700
>> @@ -138,6 +138,9 @@
>>  \-Xinternalversion\n\
>>  \  displays more detailed JVM version information than 
>> the\n\
>>  \  -version option\n\
>> +\-Xlog:  Configure or enable logging with the Java Virtual\n\
>> +\  Machine (JVM) unified logging framework. Use 
>> -Xlog:help\n\
>> +\  for details.\n\
>>  \-Xloggc:log GC status to a file with time stamps\n\
>>  \-Xmixed   mixed mode execution (default)\n\
>>  \-Xmnsets the initial and maximum size (in bytes) of the 
>> heap\n\
>> diff -r cd3c74c0 src/java.base/share/man/java.1
>> --- a/src/java.base/share/man/java.1Tue May 21 15:08:50 2019 -0700
>> +++ b/src/java.base/share/man/java.1Tue May 21 16:38:42 2019 -0700
>> @@ -732,6 +732,11 @@
>>  option, and then exits\&.
>>  .RE
>>  .PP
>> +\-Xlog:\fIopts\fR
>> +.RS 4
>> +Configure or enable logging with the Java Virtual Machine (JVM) unified 
>> logging framework. Use \fB\-Xlog:help\fR for details.
>> +.RE
>> +.PP
>>  \-Xloggc:\fIfilename\fR
>>  .RS 4
>>  Sets the file to which verbose GC events information should be redirected 
>> for logging\&. The information written to this file is similar to the output 
>> of
>> Cheers,
>> Henry
>>> On May 21, 2019, at 4:11 PM, David Holmes  wrote:
>>> 
>>> Hi Henry,
>>> 
>>> On 22/05/2019 8:41 am, Henry Jen wrote:
>>>> Hi,
>>>> Please review a trivial patch that add some hints about how to use -Xlog 
>>>> in java help and man page.
>>>> diff -r cd3c74c0 
>>>> src/java.base/share/classes/sun/launcher/resources/launcher.properties
>>>> --- 
>>>> a/src/java.base/share/classes/sun/launcher/resources/launcher.properties   
>>>>  Tue May 21 15:08:50 2019 -0700
>>>> +++ 
>>>> b/src/java.base/share/classes/sun/launcher/resources/launcher.properties   
>>>>  Tue May 21 15:31:52 2019 -0700
>>>> @@ -138,6 +138,7 @@
>>>>  \-Xinternalversion\n\
>>>>  \  displays more detailed JVM version information 
>>>> than the\n\
>>>>  \  -version option\n\
>>>> +\-Xlog:  control JVM logging, use -Xlog:help for details
>>> 
>>> I would suggest using the same words as used in the online tool reference 
>>> page [1]:
>>> 
>>> Configure or enable logging with the Java Virtual Machine (JVM) unified 
>>> logging framework. Use -Xlog:help for details.
>>> 
>>> Ditto for the man page.
>>> 
>>> Cheers,
>>> David
>>> 
>>> [1] 
>>> https://docs.oracle.com/en/java/javase/12/tools/java.html#GUID-3B1CE181-CD30-4178-9602-230B800D4FAE
>>> 
>>>>  \-Xloggc:log GC status to a file with time stamps\n\
>>>>  \-Xmixed   mixed mode execution (default)\n\
>>>>  \-Xmnsets the initial and maximum size (in bytes) of 
>>>> the heap\n\
>>>> diff -r cd3c74c0 src/java.base/share/man/java.1
>>>> --- a/src/java.base/share/man/java.1Tue May 21 15:08:50 2019 -0700
>>>> +++ b/src/java.base/share/man/java.1Tue May 21 15:31:52 2019 -0700
>>>> @@ -732,6 +732,11 @@
>>>>  option, and then exits\&.
>>>>  .RE
>>>>  .PP
>>>> +\-Xlog:\fIopts\fR
>>>> +.RS 4
>>>> +Control JVM logging, use \fB\-Xlog:help\fR for details.
>>>> +.RE
>>>> +.PP
>>>>  \-Xloggc:\fIfilename\fR
>>>>  .RS 4
>>>>  Sets the file to which verbose GC events information should be redirected 
>>>> for logging\&. The information written to this file is similar to the 
>>>> output of
>>>> Cheers,
>>>> Henry

Re: RFR: 8218997: Xusage text, man help, etc doesn't mention -Xlog option.

[Henry Jen]

Good suggestion, didn’t know about that page. I took what’s in Xusage.txt. 
Updated as following,

diff -r cd3c74c0 
src/java.base/share/classes/sun/launcher/resources/launcher.properties
--- a/src/java.base/share/classes/sun/launcher/resources/launcher.properties
Tue May 21 15:08:50 2019 -0700
+++ b/src/java.base/share/classes/sun/launcher/resources/launcher.properties
Tue May 21 16:38:42 2019 -0700
@@ -138,6 +138,9 @@
 \-Xinternalversion\n\
 \  displays more detailed JVM version information than 
the\n\
 \  -version option\n\
+\-Xlog:  Configure or enable logging with the Java Virtual\n\
+\  Machine (JVM) unified logging framework. Use 
-Xlog:help\n\
+\  for details.\n\
 \-Xloggc:log GC status to a file with time stamps\n\
 \-Xmixed   mixed mode execution (default)\n\
 \-Xmnsets the initial and maximum size (in bytes) of the 
heap\n\
diff -r cd3c74c0 src/java.base/share/man/java.1
--- a/src/java.base/share/man/java.1Tue May 21 15:08:50 2019 -0700
+++ b/src/java.base/share/man/java.1Tue May 21 16:38:42 2019 -0700
@@ -732,6 +732,11 @@
 option, and then exits\&.
 .RE
 .PP
+\-Xlog:\fIopts\fR
+.RS 4
+Configure or enable logging with the Java Virtual Machine (JVM) unified 
logging framework. Use \fB\-Xlog:help\fR for details.
+.RE
+.PP
 \-Xloggc:\fIfilename\fR
 .RS 4
 Sets the file to which verbose GC events information should be redirected for 
logging\&. The information written to this file is similar to the output of


Cheers,
Henry


> On May 21, 2019, at 4:11 PM, David Holmes  wrote:
> 
> Hi Henry,
> 
> On 22/05/2019 8:41 am, Henry Jen wrote:
>> Hi,
>> Please review a trivial patch that add some hints about how to use -Xlog in 
>> java help and man page.
>> diff -r cd3c74c0 
>> src/java.base/share/classes/sun/launcher/resources/launcher.properties
>> --- a/src/java.base/share/classes/sun/launcher/resources/launcher.properties 
>>Tue May 21 15:08:50 2019 -0700
>> +++ b/src/java.base/share/classes/sun/launcher/resources/launcher.properties 
>>Tue May 21 15:31:52 2019 -0700
>> @@ -138,6 +138,7 @@
>>  \-Xinternalversion\n\
>>  \  displays more detailed JVM version information than 
>> the\n\
>>  \  -version option\n\
>> +\-Xlog:  control JVM logging, use -Xlog:help for details
> 
> I would suggest using the same words as used in the online tool reference 
> page [1]:
> 
> Configure or enable logging with the Java Virtual Machine (JVM) unified 
> logging framework. Use -Xlog:help for details.
> 
> Ditto for the man page.
> 
> Cheers,
> David
> 
> [1] 
> https://docs.oracle.com/en/java/javase/12/tools/java.html#GUID-3B1CE181-CD30-4178-9602-230B800D4FAE
> 
>>  \-Xloggc:log GC status to a file with time stamps\n\
>>  \-Xmixed   mixed mode execution (default)\n\
>>  \-Xmnsets the initial and maximum size (in bytes) of the 
>> heap\n\
>> diff -r cd3c74c0 src/java.base/share/man/java.1
>> --- a/src/java.base/share/man/java.1Tue May 21 15:08:50 2019 -0700
>> +++ b/src/java.base/share/man/java.1Tue May 21 15:31:52 2019 -0700
>> @@ -732,6 +732,11 @@
>>  option, and then exits\&.
>>  .RE
>>  .PP
>> +\-Xlog:\fIopts\fR
>> +.RS 4
>> +Control JVM logging, use \fB\-Xlog:help\fR for details.
>> +.RE
>> +.PP
>>  \-Xloggc:\fIfilename\fR
>>  .RS 4
>>  Sets the file to which verbose GC events information should be redirected 
>> for logging\&. The information written to this file is similar to the output 
>> of
>> Cheers,
>> Henry

RFR: 8218997: Xusage text, man help, etc doesn't mention -Xlog option.

[Henry Jen]

Hi,

Please review a trivial patch that add some hints about how to use -Xlog in 
java help and man page.

diff -r cd3c74c0 
src/java.base/share/classes/sun/launcher/resources/launcher.properties
--- a/src/java.base/share/classes/sun/launcher/resources/launcher.properties
Tue May 21 15:08:50 2019 -0700
+++ b/src/java.base/share/classes/sun/launcher/resources/launcher.properties
Tue May 21 15:31:52 2019 -0700
@@ -138,6 +138,7 @@
 \-Xinternalversion\n\
 \  displays more detailed JVM version information than 
the\n\
 \  -version option\n\
+\-Xlog:  control JVM logging, use -Xlog:help for details
 \-Xloggc:log GC status to a file with time stamps\n\
 \-Xmixed   mixed mode execution (default)\n\
 \-Xmnsets the initial and maximum size (in bytes) of the 
heap\n\
diff -r cd3c74c0 src/java.base/share/man/java.1
--- a/src/java.base/share/man/java.1Tue May 21 15:08:50 2019 -0700
+++ b/src/java.base/share/man/java.1Tue May 21 15:31:52 2019 -0700
@@ -732,6 +732,11 @@
 option, and then exits\&.
 .RE
 .PP
+\-Xlog:\fIopts\fR
+.RS 4
+Control JVM logging, use \fB\-Xlog:help\fR for details.
+.RE
+.PP
 \-Xloggc:\fIfilename\fR
 .RS 4
 Sets the file to which verbose GC events information should be redirected for 
logging\&. The information written to this file is similar to the output of

Cheers,
Henry

Re: RFR: 8217216: Launcher does not defend itself against LD_LIBRARY_PATH_64 (Solaris)

[Henry Jen]

Thanks, Roger.

Cheers,
Henry


> On Mar 1, 2019, at 10:27 AM, Roger Riggs  wrote:
> 
> Hi Henry,
> 
> The change looks ok, Reviewed.
> 
> (I'm not that familiar with Solaris).
> 
> Thanks, Roger
> 
> 
> 
> On 3/1/19 10:45 AM, Henry Jen wrote:
>> Ping!
>> 
>> Any thought on this webrev restore the way for Solaris LD_LIBRARY_PATH_64 
>> handling?
>> 
>> Cheers,
>> Henry
>> 
>> 
>>> On Feb 13, 2019, at 9:37 AM, Henry Jen  wrote:
>>> 
>>> Hi,
>>> 
>>> Please review the webrev[1] for 8217216. The fix makes sure on Solaris, 
>>> when LD_LIBRARY_PATH_64 is set, we setup LD_LIBRARY_PATH based on that 
>>> value and unset LD_LIBRARY_PATH_64 in the relaunched process.
>>> 
>>> Same approach was used before JDK-6367077, and the override is expected 
>>> behavior on Solaris 64-bit as stated in Solaris 64-bit Developer's Guide[2],
>>> "The 64-bit dynamic linker's search path can be completely overridden using 
>>> the LD_LIBRARY_PATH_64 environment variable."
>>> 
>>> Cheers,
>>> Henry
>>> 
>>> [1] http://cr.openjdk.java.net/~henryjen/8217216/0/webrev/
>>> [2] https://docs.oracle.com/cd/E19455-01/806-0477/dev-env-7/index.html
>>> 
> 

Re: RFR: 8217216: Launcher does not defend itself against LD_LIBRARY_PATH_64 (Solaris)

[Henry Jen]

Ping! 

Any thought on this webrev restore the way for Solaris LD_LIBRARY_PATH_64 
handling? 

Cheers,
Henry


> On Feb 13, 2019, at 9:37 AM, Henry Jen  wrote:
> 
> Hi,
> 
> Please review the webrev[1] for 8217216. The fix makes sure on Solaris, when 
> LD_LIBRARY_PATH_64 is set, we setup LD_LIBRARY_PATH based on that value and 
> unset LD_LIBRARY_PATH_64 in the relaunched process.
> 
> Same approach was used before JDK-6367077, and the override is expected 
> behavior on Solaris 64-bit as stated in Solaris 64-bit Developer's Guide[2],
> "The 64-bit dynamic linker's search path can be completely overridden using 
> the LD_LIBRARY_PATH_64 environment variable."
> 
> Cheers,
> Henry
> 
> [1] http://cr.openjdk.java.net/~henryjen/8217216/0/webrev/
> [2] https://docs.oracle.com/cd/E19455-01/806-0477/dev-env-7/index.html
> 

RFR: 8217216: Launcher does not defend itself against LD_LIBRARY_PATH_64 (Solaris)

[Henry Jen]

Hi,

Please review the webrev[1] for 8217216. The fix makes sure on Solaris, when 
LD_LIBRARY_PATH_64 is set, we setup LD_LIBRARY_PATH based on that value and 
unset LD_LIBRARY_PATH_64 in the relaunched process.

Same approach was used before JDK-6367077, and the override is expected 
behavior on Solaris 64-bit as stated in Solaris 64-bit Developer's Guide[2],
"The 64-bit dynamic linker's search path can be completely overridden using the 
LD_LIBRARY_PATH_64 environment variable."

Cheers,
Henry

[1] http://cr.openjdk.java.net/~henryjen/8217216/0/webrev/
[2] https://docs.oracle.com/cd/E19455-01/806-0477/dev-env-7/index.html

Re: RFR: 8215000: tools/launcher/JliLaunchTest.java fails on Windows

[Henry Jen]

Duh, should be v==null. Thanks for catching it.

Cheers,
Henry

> On Dec 12, 2018, at 10:50 AM, Mandy Chung  wrote:
> 
> Brent is right since k is the given key and non-null.  Although it does not 
> cause any issue as it only adds an empty element in the path, we should fix 
> it in this patch.
> 
> Mandy
> 
> On 12/12/18 10:30 AM, Brent Christian wrote:
>> Hi, 
>> 
>> Shouldn't the lambdas be checking for v == null, rather than k == null? 
>> 
>> -Brent 
>> 
>> On 12/12/18 9:36 AM, Henry Jen wrote: 
>>> Hi, 
>>> 
>>> Can I get a review of following patch? 
>>> 
>>> Looks like the assumption test jdk will be in PATH is simply not true, 
>>> jtreg doesn’t do that. 
>>> Also, this patch make sure the JDK to be tested is first in the search 
>>> path. 
>>> 
>>> Cheers, 
>>> Henry 
>>> 
>>> 
>>> diff -r 241b8151b6b6 test/jdk/tools/launcher/JliLaunchTest.java 
>>> --- a/test/jdk/tools/launcher/JliLaunchTest.javaFri Nov 30 13:42:49 
>>> 2018 -0800 
>>> +++ b/test/jdk/tools/launcher/JliLaunchTest.javaWed Dec 12 09:31:53 
>>> 2018 -0800 
>>> @@ -49,10 +49,12 @@ 
>>>   Map env = pb.environment(); 
>>>   if (Platform.isWindows()) { 
>>>   // The DLL should be in JDK/bin 
>>> +String libdir = 
>>> Paths.get(Utils.TEST_JDK).resolve("bin").toAbsolutePath().toString(); 
>>> +env.compute("PATH", (k, v) -> (k == null) ? libdir : libdir + 
>>> ";" + v); 
>>>   } else { 
>>>   String libdir = 
>>> Paths.get(Utils.TEST_JDK).resolve("lib").toAbsolutePath().toString(); 
>>>   String LD_LIBRARY_PATH = Platform.isOSX() ? 
>>> "DYLD_LIBRARY_PATH" : "LD_LIBRARY_PATH"; 
>>> -env.compute(LD_LIBRARY_PATH, (k, v) -> (k == null) ? libdir : 
>>> v + ":" + libdir); 
>>> +env.compute(LD_LIBRARY_PATH, (k, v) -> (k == null) ? libdir : 
>>> libdir + ":" + v); 
>>>   } 
>>> 
>>>   OutputAnalyzer outputf = new OutputAnalyzer(pb.start()); 
>>> 
> 

RFR: 8215000: tools/launcher/JliLaunchTest.java fails on Windows

[Henry Jen]

Hi,

Can I get a review of following patch?

Looks like the assumption test jdk will be in PATH is simply not true, jtreg 
doesn’t do that.
Also, this patch make sure the JDK to be tested is first in the search path.

Cheers,
Henry


diff -r 241b8151b6b6 test/jdk/tools/launcher/JliLaunchTest.java
--- a/test/jdk/tools/launcher/JliLaunchTest.javaFri Nov 30 13:42:49 
2018 -0800
+++ b/test/jdk/tools/launcher/JliLaunchTest.javaWed Dec 12 09:31:53 
2018 -0800
@@ -49,10 +49,12 @@
 Map env = pb.environment();
 if (Platform.isWindows()) {
 // The DLL should be in JDK/bin
+String libdir = 
Paths.get(Utils.TEST_JDK).resolve("bin").toAbsolutePath().toString();
+env.compute("PATH", (k, v) -> (k == null) ? libdir : libdir + ";" 
+ v);
 } else {
 String libdir = 
Paths.get(Utils.TEST_JDK).resolve("lib").toAbsolutePath().toString();
 String LD_LIBRARY_PATH = Platform.isOSX() ? "DYLD_LIBRARY_PATH" : 
"LD_LIBRARY_PATH";
-env.compute(LD_LIBRARY_PATH, (k, v) -> (k == null) ? libdir : v + 
":" + libdir);
+env.compute(LD_LIBRARY_PATH, (k, v) -> (k == null) ? libdir : 
libdir + ":" + v);
 }

 OutputAnalyzer outputf = new OutputAnalyzer(pb.start());

Re: RFR: JDK-8213362 : Could not find libjava.dylib error when initializing JVM via JNI_CreateJavaVM

[Henry Jen]

Thanks for the review, Alan and Magnus.

Cheers,
Henry

> On Nov 30, 2018, at 7:20 AM, Alan Bateman  wrote:
> 
> 
> 
> On 28/11/2018 22:49, Henry Jen wrote:
>> Since there is no header file in JDK provides the function prototypes, I 
>> don’t think this is considered public(supported) APIs.
>> 
>> Anyway, in case a tools is build with launcher code, and shipped separately 
>> from JDK, that would be impacted by this bug. So I added a test call 
>> JLI_Launch as well. Please review the updated webrev[1].
>> 
>> Note that, JLI_Launch should not be used directly as it does’t handle the 
>> argument processing which is done in launcher/main.c.
>> 
>> Cheers,
>> Henry
>> 
>> [1] http://cr.openjdk.java.net/~henryjen/jdk12/8213362.1/webrev/
>> 
> Updated webrev looks okay. I do find the exeXXX convention for the launchers 
> created in the test make files very strange but this is something we can 
> chase up with another issue.
> 
> -Alan

Re: RFR: JDK-8213362 : Could not find libjava.dylib error when initializing JVM via JNI_CreateJavaVM

[Henry Jen]

Since there is no header file in JDK provides the function prototypes, I don’t 
think this is considered public(supported) APIs.

Anyway, in case a tools is build with launcher code, and shipped separately 
from JDK, that would be impacted by this bug. So I added a test call JLI_Launch 
as well. Please review the updated webrev[1].

Note that, JLI_Launch should not be used directly as it does’t handle the 
argument processing which is done in launcher/main.c.

Cheers,
Henry

[1] http://cr.openjdk.java.net/~henryjen/jdk12/8213362.1/webrev/


> On Nov 28, 2018, at 5:28 AM, Kevin Rushforth  
> wrote:
> 
> 
> 
> On 11/28/2018 5:19 AM, Alan Bateman wrote:
>> On 28/11/2018 13:13, Kevin Rushforth wrote:
>>> The jpackage tool calls JLI_Launch.
>> I remember that from Andy's webrev but it's not integrated yet. Does the 
>> JavaFX packager tool do the same?
> 
> Yes, the javapackager tool (delivered via JavaFX) in JDK 8/9/10 calls 
> JLI_Launch.
> 
> -- Kevin
> 

Re: RFR: JDK-8213362 : Could not find libjava.dylib error when initializing JVM via JNI_CreateJavaVM

[Henry Jen]

> On Nov 28, 2018, at 12:46 AM, Alan Bateman  wrote:
> 
> On 27/11/2018 23:05, Henry Jen wrote:
>> Hi,
>> 
>> Please review a follow up webrev[1] based on Priyanka’s patch, it simply 
>> added a test case for Mac only that will link with libjli.
>> Note that, to use invoke API, one should probably link with libjvm, which 
>> works for all supported platforms, not just Mac.
>> 
>> Cheers,
>> Henry
>> 
>> [1] http://cr.openjdk.java.net/~henryjen/jdk12/8213362.0/webrev/
>> 
> The changes to java_md_macosx.m looks okay although the issue as to how 
> Eclipse runs into this have not been established. If Eclipse dlopen's libjvm 
> then it should have no issue locating and calling JNI's CreateJavaVM. It may 
> be that Eclipse is using libjli directly but it shouldn't do that because 
> it's not a documented/supported interface. It might be that it's using the 
> CFBundleExecutable key in Info.plist. Henry, Priyanka and I discussed this a 
> bit and were not able to able to establish what it is doing.
> 
> On the test: it's clear whether you've moved or copied 
> test/hotspot/jtreg/runtime/jni/CalleeSavedRegisters/FPRegs.java. The webrev 
> suggests you've moved it but it's doesn't handle hg copy. Either way, it 
> needs clean up, e.g. it shouldn't need @modules java.base/jdk.internal.misc, 
> the really long lines make it difficult to look at side-by-side changes, why 
> does it check for Windows when the @requires means it runs on Mac only. I 
> assume we can find a better name for exeJNILauncher.c. It will also need a 
> copyright header.
> 

It’s hg copy. I’ll clean up the module and windows line, also add a copyright 
for exeJNILauncher.c. The exe is prefix for build, if it’s JNILauncher not 
desired, I am open for suggestions.

Cheers,
Henry

RFR: JDK-8213362 : Could not find libjava.dylib error when initializing JVM via JNI_CreateJavaVM

[Henry Jen]

Hi,

Please review a follow up webrev[1] based on Priyanka’s patch, it simply added 
a test case for Mac only that will link with libjli.
Note that, to use invoke API, one should probably link with libjvm, which works 
for all supported platforms, not just Mac.

Cheers,
Henry

[1] http://cr.openjdk.java.net/~henryjen/jdk12/8213362.0/webrev/

> On Nov 6, 2018, at 12:18 AM, Magnus Ihse Bursie 
>  wrote:
> 
> I now noticed that this was only sent to build-dev. This is not really a 
> build question. Cc:ing core-libs-dev. 
> 
> /Magnus
> 
>> 5 nov. 2018 kl. 15:21 skrev Magnus Ihse Bursie 
>> :
>> 
>> Hi,
>> 
>> Fix looks good, but maybe we should have a regression test of GetJREPath()?
>> 
>> /Magnus
>> 
>>> 5 nov. 2018 kl. 12:09 skrev Priyanka Mangal :
>>> 
>>> Hi,
>>> 
>>> Please review the minor fix for :
>>> https://bugs.openjdk.java.net/browse/JDK-8213362
>>> http://cr.openjdk.java.net/~pmangal/8213362/webrev.00/
>>> 
>>> With the JDK-8210931  
>>> libjli has moved from "jli" subdirectory into standard lib directory.
>>> However `GetJREPath` method in 
>>> `src/java.base/macosx/native/libjli/java_md_macosx.m` still assumes that 
>>> `libjli.dylib` should be on `lib/jli/libjli.dylib` path.
>>> So corrected the same.
>>> 
>>> Testing:
>>> mach5 : tier1-3
>>> http://java.se.oracle.com:10065/mdash/jobs/fmatte-jdk-20181105-0941-8721?search=result.status:*
>>> 
>>> Thanks
>>> Regards
>>> Priyanka
>> 
>> 
> 

Re: [RFR] 8210810: Escaped character at specific position in argument file is not handled properly

[Henry Jen]

I’ll create back port requests.

Cheers,
Henry


> On Sep 27, 2018, at 11:06 PM, Bo Zhang  wrote:
> 
> Thanks Henry, this change looks good to me. I assume this patch will be 
> backported to 11GA, will it be back ported to 10 as well?
> 
> Regards,
> Bo
> 
>> On 28 Sep 2018, at 00:49, Henry Jen  wrote:
>> 
>> Agree, please find updated webrev[1].
>> 
>> [1] http://cr.openjdk.java.net/~henryjen/jdk/8210810/1/webrev/
>> 
>> Cheers,
>> Henry
>> 
>> 
>>> On Sep 27, 2018, at 1:45 AM, Alan Bateman  wrote:
>>> 
>>> On 27/09/2018 03:58, Henry Jen wrote:
>>>> Hi,
>>>> 
>>>> Need a quick review of the webrev[1] for JDK-8210810[2], it’s pretty much 
>>>> what Bo contributed, just add some trailing text “aaa\” to verify the 
>>>> integrity of escape sequence is handled properly.
>>>> 
>>>> I had reviewed the change and tested, but we need a “R”eviewer.
>>>> 
>>>> Cheers,
>>>> Henry
>>>> 
>>>> [1] http://cr.openjdk.java.net/~henryjen/jdk/8210810/webrev/
>>>> [2] https://bugs.openjdk.java.net/browse/JDK-8210810
>>>> 
>>>> 
>>> The change to args.c looks okay but for the test then I assume it would be 
>>> better to just add another test case to the existing ArgFileSyntax.java.
>>> 
>>> -Alan
>> 
> 

Re: [RFR] 8210810: Escaped character at specific position in argument file is not handled properly

[Henry Jen]

Will do.

Cheers,
Henry


> On Sep 28, 2018, at 3:23 AM, Alan Bateman  wrote:
> 
> 
> 
> On 27/09/2018 17:49, Henry Jen wrote:
>> Agree, please find updated webrev[1].
>> 
>> [1] http://cr.openjdk.java.net/~henryjen/jdk/8210810/1/webrev/
>> 
>> 
> This looks okay to me, you may want to consider adding 8210810 to the @bug 
> tag before pushing.
> 
> -Alan

Re: [RFR] 8210810: Escaped character at specific position in argument file is not handled properly

[Henry Jen]

Agree, please find updated webrev[1].

[1] http://cr.openjdk.java.net/~henryjen/jdk/8210810/1/webrev/

Cheers,
Henry


> On Sep 27, 2018, at 1:45 AM, Alan Bateman  wrote:
> 
> On 27/09/2018 03:58, Henry Jen wrote:
>> Hi,
>> 
>> Need a quick review of the webrev[1] for JDK-8210810[2], it’s pretty much 
>> what Bo contributed, just add some trailing text “aaa\” to verify the 
>> integrity of escape sequence is handled properly.
>> 
>> I had reviewed the change and tested, but we need a “R”eviewer.
>> 
>> Cheers,
>> Henry
>> 
>> [1] http://cr.openjdk.java.net/~henryjen/jdk/8210810/webrev/
>> [2] https://bugs.openjdk.java.net/browse/JDK-8210810
>> 
>> 
> The change to args.c looks okay but for the test then I assume it would be 
> better to just add another test case to the existing ArgFileSyntax.java.
> 
> -Alan

[RFR] 8210810: Escaped character at specific position in argument file is not handled properly

[Henry Jen]

Hi,

Need a quick review of the webrev[1] for JDK-8210810[2], it’s pretty much what 
Bo contributed, just add some trailing text “aaa\” to verify the integrity of 
escape sequence is handled properly.

I had reviewed the change and tested, but we need a “R”eviewer.

Cheers,
Henry

[1] http://cr.openjdk.java.net/~henryjen/jdk/8210810/webrev/
[2] https://bugs.openjdk.java.net/browse/JDK-8210810

> On Sep 26, 2018, at 6:45 PM, Bo Zhang  wrote:
> 
> Thank you Henry. Feel free to modify it.
> 
> Regards,
> Bo
> 
>> On 27 Sep 2018, at 09:39, Henry Jen  wrote:
>> 
>> Thanks for the fix. I had looked into it, it’s a nice catch, glad you find 
>> it.
>> 
>> The patch looks good, and test is sufficient, I have tested it with jdk.jdk 
>> repo. I will help you to push the fix. The only minor improvement I like to 
>> see is to verify content after the escape character, which I took the 
>> liberty to do it. Webrev coming soon.
>> 
>> Cheers,
>> Henry
>> 
>>> On Sep 26, 2018, at 4:03 PM, Bo Zhang  wrote:
>>> 
>>> Can anyone please take a look?
>>> 
>>> Thank you.
>>> 
>>>> On 25 Sep 2018, at 14:27, Bo Zhang  wrote:
>>>> 
>>>> Hi all,
>>>> 
>>>> This is my first time to try to submit a patch. Please forgive me if I did 
>>>> anything inappropriate. I’ve signed the OCA with the name “Bo Zhang”.
>>>> 
>>>> Bug description:
>>>> 
>>>> 8210810 <https://bugs.openjdk.java.net/browse/JDK-8210810>: Incorrect 
>>>> argument file parser behavior in certain cases
>>>> 
>>>> Previously, if a quoted backslash character appears at argument file
>>>> parser buffer boundary (i.e. 4096th/8192nd/... character of an argument
>>>> file is backslash '\'), the segment before the backslash character will
>>>> be added into argument twice due to a missing anchor position reset
>>>> operation.
>>>> 
>>>> For example, you have an argument file:
>>>> 
>>>> -cp "...a\\b"
>>>> 
>>>> and the 4096th and 4097th characters happen to be both '\', the result
>>>> generated by the argument file parser would be:
>>>> 
>>>> -cp "...aa\\b”
>>>> 
>>>> This patch fixes the issue by resetting anchor position correctly
>>>> in this case.
>>>> 
>>>> I’m a little surprised that this issue has existed for several years 
>>>> unnoticed.
>>>> 
>>>> 
>>>> Patch:
>>>> 
>>>> # HG changeset patch
>>>> # User Bo Zhang mailto:zhangbo...@gmail.com>>
>>>> # Date 1537835887 -28800
>>>> #  Tue Sep 25 08:38:07 2018 +0800
>>>> # Node ID 34f2803ad62cde8c82e3dfcf0b66e80276bd7352
>>>> # Parent  16f0deae8fa6ef85f7230308e1fe7556bd007c39
>>>> 8210810: Incorrect argument file parser behavior in certain cases
>>>> 
>>>> Previously, if a quoted backslash character appears at argument file
>>>> parser buffer boundary (i.e. 4096th/8192nd/... character of an argument
>>>> file is backslash '\'), the segment before the backslash character will
>>>> be added into argument twice due to a missing anchor position reset
>>>> operation.
>>>> 
>>>> For example, you have an argument file:
>>>> 
>>>> -cp "...a\\b"
>>>> 
>>>> and the 4096th and 4097th characters happen to be both '\', the result
>>>> generated by the argument file parser would be:
>>>> 
>>>> -cp "...aa\\b"
>>>> 
>>>> This patch fixes the issue by resetting anchor position correctly
>>>> in this case.
>>>> 
>>>> diff --git a/src/java.base/share/native/libjli/args.c 
>>>> b/src/java.base/share/native/libjli/args.c
>>>> --- a/src/java.base/share/native/libjli/args.c
>>>> +++ b/src/java.base/share/native/libjli/args.c
>>>> @@ -262,6 +262,8 @@
>>>>continue;
>>>>}
>>>>JLI_List_addSubstring(pctx->parts, anchor, nextc - anchor);
>>>> +// anchor after backslash character
>>>> +anchor = nextc + 1;
>>>>pctx->state = IN_ESCAPE;
>>>>break;
>>>>case '\'':
>>>> diff --git a/test/jdk/tools/launcher/Test8210810.java 
>

Re: [PATCH] 8210810: Incorrect argument file parser behavior in certain cases

[Henry Jen]

Thanks for the fix. I had looked into it, it’s a nice catch, glad you find it.

The patch looks good, and test is sufficient, I have tested it with jdk.jdk 
repo. I will help you to push the fix. The only minor improvement I like to see 
is to verify content after the escape character, which I took the liberty to do 
it. Webrev coming soon.

Cheers,
Henry

> On Sep 26, 2018, at 4:03 PM, Bo Zhang  wrote:
> 
> Can anyone please take a look?
> 
> Thank you.
> 
>> On 25 Sep 2018, at 14:27, Bo Zhang  wrote:
>> 
>> Hi all,
>> 
>> This is my first time to try to submit a patch. Please forgive me if I did 
>> anything inappropriate. I’ve signed the OCA with the name “Bo Zhang”.
>> 
>> Bug description:
>> 
>> 8210810 : Incorrect 
>> argument file parser behavior in certain cases
>> 
>> Previously, if a quoted backslash character appears at argument file
>> parser buffer boundary (i.e. 4096th/8192nd/... character of an argument
>> file is backslash '\'), the segment before the backslash character will
>> be added into argument twice due to a missing anchor position reset
>> operation.
>> 
>> For example, you have an argument file:
>> 
>> -cp "...a\\b"
>> 
>> and the 4096th and 4097th characters happen to be both '\', the result
>> generated by the argument file parser would be:
>> 
>> -cp "...aa\\b”
>> 
>> This patch fixes the issue by resetting anchor position correctly
>> in this case.
>> 
>> I’m a little surprised that this issue has existed for several years 
>> unnoticed.
>> 
>> 
>> Patch:
>> 
>> # HG changeset patch
>> # User Bo Zhang mailto:zhangbo...@gmail.com>>
>> # Date 1537835887 -28800
>> #  Tue Sep 25 08:38:07 2018 +0800
>> # Node ID 34f2803ad62cde8c82e3dfcf0b66e80276bd7352
>> # Parent  16f0deae8fa6ef85f7230308e1fe7556bd007c39
>> 8210810: Incorrect argument file parser behavior in certain cases
>> 
>> Previously, if a quoted backslash character appears at argument file
>> parser buffer boundary (i.e. 4096th/8192nd/... character of an argument
>> file is backslash '\'), the segment before the backslash character will
>> be added into argument twice due to a missing anchor position reset
>> operation.
>> 
>> For example, you have an argument file:
>> 
>> -cp "...a\\b"
>> 
>> and the 4096th and 4097th characters happen to be both '\', the result
>> generated by the argument file parser would be:
>> 
>> -cp "...aa\\b"
>> 
>> This patch fixes the issue by resetting anchor position correctly
>> in this case.
>> 
>> diff --git a/src/java.base/share/native/libjli/args.c 
>> b/src/java.base/share/native/libjli/args.c
>> --- a/src/java.base/share/native/libjli/args.c
>> +++ b/src/java.base/share/native/libjli/args.c
>> @@ -262,6 +262,8 @@
>> continue;
>> }
>> JLI_List_addSubstring(pctx->parts, anchor, nextc - anchor);
>> +// anchor after backslash character
>> +anchor = nextc + 1;
>> pctx->state = IN_ESCAPE;
>> break;
>> case '\'':
>> diff --git a/test/jdk/tools/launcher/Test8210810.java 
>> b/test/jdk/tools/launcher/Test8210810.java
>> new file mode 100644
>> --- /dev/null
>> +++ b/test/jdk/tools/launcher/Test8210810.java
>> @@ -0,0 +1,107 @@
>> +/*
>> + * Copyright (c) 2018, Oracle and/or its affiliates. All rights reserved.
>> + * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
>> + *
>> + * This code is free software; you can redistribute it and/or modify it
>> + * under the terms of the GNU General Public License version 2 only, as
>> + * published by the Free Software Foundation.
>> + *
>> + * This code is distributed in the hope that it will be useful, but WITHOUT
>> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
>> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
>> + * version 2 for more details (a copy is included in the LICENSE file that
>> + * accompanied this code).
>> + *
>> + * You should have received a copy of the GNU General Public License version
>> + * 2 along with this work; if not, write to the Free Software Foundation,
>> + * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
>> + *
>> + * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
>> + * or visit www.oracle.com  if you need additional 
>> information or have any
>> + * questions.
>> + */
>> +
>> +/*
>> + * @test
>> + * @bug 8210810
>> + * @summary Incorrect argument file parser behavior when backslash appears 
>> at buffer boundary
>> + * @modules java.base
>> + * @author Bo Zhang
>> + * @build TestHelper
>> + * @run main Test8210810
>> + */
>> +
>> +/*
>> + * 8210810: Previously, if a quoted backslash character appears at argument 
>> file
>> + * parser buffer boundary (i.e. 4096th/8192nd/... character of an argument
>> + * file is backslash '\'), the segment before the backslash character will
>> + * be added into argument twice 

Re: RFR: 8199871: Deprecate pack200 and unpack200 tools

[Henry Jen]

I revised the webrev to have warning only executable name for unpack200 instead 
of full path on Windows, which I believe is what was intended all along, the 
test is also revised to take unpack200.exe on the Windows platform.

The updated version is at 
http://cr.openjdk.java.net/~henryjen/jdk11/8199871/1/webrev/

Cheers,
Henry


> On Jun 8, 2018, at 6:56 PM, Henry Jen  wrote:
> 
> Hi,
> 
> Please review this webrev[1] in which we mark Pack200 related API and tools 
> deprecate, and print a warning about tools to be remove in a future JDK 
> release. To avoid interrupt existing tools depends on the output, an option 
> is provided to suppress the warning message.
> 
> The option name is following the convention of javah, not the GNU style as we 
> should. The rationale is that this is a temporary option and an existing 
> convention would make it more consistent for users. Anyhow, if we have a 
> strong feeling this should be changed, we can do that.
> 
> For jar tool, the normalize option use Pack200 is also deprecated, the 
> suppress option only available to the new GNU style option mode. As if user 
> need to provide the option, the command line change is necessary, so without 
> compatible mode option should not be an issue.
> 
> Cheers,
> Henry
> 
> [1] http://cr.openjdk.java.net/~henryjen/jdk11/8199871/0/webrev/

RFR: JDK-8180447: Trailing space in JDK_JAVA_OPTIONS causes an application fail to launch

[Henry Jen]

Hi,

Please review a trivial fix for JDK-8180447[1], which we also makes the output 
of options picked up by the environment variable in one line instead of two for 
easier extraction from output.

Cheers,
Henry

[1] http://cr.openjdk.java.net/~henryjen/jdk9/8180447/webrev/

Re: RFR [XS] : 8175000 : jexec fails to execute simple helloworld.jar

[Henry Jen]

> On Mar 1, 2017, at 3:47 AM, Thomas Stüfe  wrote:
> 
> Hi Matthias,
> 
> the fix makes sense, this is very clearly a bug.
> 
> I'd suggest a simpler fix though:
> 
>  end -= 4; // make sure there are 4 bytes to read at
> start
> - while (start < end) {
> +while (start <= end) {
> 

+1.

Cheers,
Henry


> Note that the code has a diffent bug too, very unlikely but not impossible
> to hit:
> 
> 321 ssize_t count = read(fd, buf, CHUNK_SIZE);
> 322 if (count >= MIN_SIZE) {
> 
> We attempt to read CHUNK_SIZE bytes and require the read to have returned
> at least MIN_SIZE (something like 30ish bytes). If not, jexec fails.
> 
> read may have been interrupted (EINTR) or may have returned less bytes than
> MIN_SIZE, so it should read in a loop til eof or CHUNK_SIZE bytes are read.
> 
> Kind Regards, Thomas
> 
> 
> On Wed, Mar 1, 2017 at 10:23 AM, Baesken, Matthias > wrote:
> 
>> Ping ...
>> 
>> Can I get a review please for the change ?
>> 
>> 
>> Thanks, Matthias
>> 
>> From: Baesken, Matthias
>> Sent: Donnerstag, 23. Februar 2017 12:28
>> To: 'core-libs-dev@openjdk.java.net' 
>> Cc: Langer, Christoph ; Erik Joelsson (
>> erik.joels...@oracle.com) ; 'Michel Trudeau' <
>> michel.trud...@oracle.com>
>> Subject: RE: RFR [XS] : 8175000 : jexec fails to execute simple
>> helloworld.jar
>> 
>> Here is  the webrev for jdk9 :
>> 
>> http://cr.openjdk.java.net/~mbaesken/webrevs/8175000/
>> 
>> 
>> ?  And btw I really wonder  - is  jexec still needed in future jdk's like
>> jdk10  ? Seems it is not used much .
>> 
>> ?
>> 
>> In case  jexec will stay in  the jdk  I might add a test for the tool as
>> well, if there is interest  ( could not really find one that tests
>> execution of a simple jar-file with jexec).
>> 
>> Best regards, Matthias
>> 
>> 
>> From: Baesken, Matthias
>> Sent: Donnerstag, 23. Februar 2017 07:39
>> To: 'core-libs-dev@openjdk.java.net' > >
>> Cc: Langer, Christoph  christoph.lan...@sap.com>>; Erik Joelsson (erik.joels...@oracle.com<
>> mailto:erik.joels...@oracle.com>)  erik.joels...@oracle.com>>
>> Subject: RE: RFR [XS] : 8175000 : jexec fails to execute simple
>> helloworld.jar
>> 
>> Hello,  probably I should add the info that the fix is needed  in jdk9 as
>> well , not only jdk10 .
>> 
>> Without the fix jdk9/10show this error  when executing a small
>> example jar :
>> 
>> /myjdk9/images/jdk/lib/jexec  /java_test/hellojar/helloworld.jar
>> invalid file (bad magic number): Exec format error
>> 
>> with the fix :
>> 
>> jdk/lib/jexec/java_test/hellojar/helloworld.jar
>> Hello world from a jar file
>> 
>> 
>> And btw I really wonder  - is  jexec still needed in future jdk's like
>> jdk10  ? Seems it is not used much .
>> 
>> Best regards, Matthias
>> 
>> 
>> From: Baesken, Matthias
>> Sent: Mittwoch, 22. Februar 2017 18:16
>> To: core-libs-dev@openjdk.java.net
>> Cc: Langer, Christoph  christoph.lan...@sap.com>>; Erik Joelsson (erik.joels...@oracle.com<
>> mailto:erik.joels...@oracle.com>)  erik.joels...@oracle.com>>
>> Subject: RFR [XS] : 8175000 : jexec fails to execute simple helloworld.jar
>> 
>> Hello , when looking into  the jexec build I noticed   that  execution of
>> a simple helloworld.jar   with jexec does not work any more.
>> 
>> I did a little patch for this which adjusted the addition done with  CR
>> 8156478: 3 Buffer overrun defect groups in jexec.c> oracle.com/mproxy/repository/technology/java2/jdk9/jdk/rev/4f96129b45ee>
>> .
>> 
>> Could I have a review ( just a diff this time is provided because of
>> infrastructure issues) for it ?
>> 
>> 
>> Thanks, Matthias
>> 
>> Bug :
>> https://bugs.openjdk.java.net/browse/JDK-8175000
>> 
>> 
>> Diff for jdk10  :
>> 
>> # HG changeset patch
>> # User mbaesken
>> # Date 1487782485 -3600
>> #  Wed Feb 22 17:54:45 2017 +0100
>> # Node ID 93d55a711f3b1c3f282e6889c24d13f16d4a4548
>> # Parent  884872263accabd4ab68d005abd4e5393144aa4f
>> 8175000: jexec fails to execute simple helloworld.jar
>> 
>> diff --git a/src/java.base/unix/native/launcher/jexec.c
>> b/src/java.base/unix/native/launcher/jexec.c
>> --- a/src/java.base/unix/native/launcher/jexec.c
>> +++ b/src/java.base/unix/native/launcher/jexec.c
>> @@ -331,8 +331,9 @@
>> off_t end   = start  + xlen;
>> 
>> if (end <= count) {
>> -end -= 4; // make sure there are 4 bytes to read at
>> start
>> -while (start < end) {
>> +// make sure there are 4 bytes to read at start
>> +end -= 3;
>> +while ((start < 

Re: Review Request: JDK-8173712: Rename JAVA_OPTIONS environment variable to JDK_JAVA_OPTIONS

[Henry Jen]

+1.

Cheers,
Henry

> On Feb 7, 2017, at 1:16 PM, Mandy Chung  wrote:
> 
> Henry, Kumar,
> 
> Can you please review this patch:
>  http://cr.openjdk.java.net/~mchung/jdk9/webrevs/8173712/webrev.00/
> 
> JAVA_OPTIONS was introduced by JDK-8170832 but it turns out that
> environment variable name conflicts with existing uses.  It might 
> not be uncommon for scripts to set JAVA_OPTIONS. To mitigate the
> compatibility concern, we decide to rename JAVA_OPTIONS to 
> JDK_JAVA_OPTIONS.  With multi-word name and `JDK_` prefix, that
> should reduce the chance of name clash.
> 
> thanks
> Mandy

Re: RFR: 8171522: Jar prints error message with old (non gnu-style options).

[Henry Jen]

Sorry, please ignore previous email, this one is correct.

diff -r 0e2935453091 
src/jdk.jartool/share/classes/sun/tools/jar/resources/jar.properties
--- a/src/jdk.jartool/share/classes/sun/tools/jar/resources/jar.properties  
Wed Feb 01 11:05:33 2017 -0800
+++ b/src/jdk.jartool/share/classes/sun/tools/jar/resources/jar.properties  
Thu Feb 02 11:00:36 2017 -0800
@@ -36,7 +36,7 @@
 error.bad.file.arg=\
  Error parsing file arguments
 error.bad.option=\
-One of options -{ctxu} must be specified.
+One of options -{ctxuid} must be specified.
 error.bad.cflag=\
 'c' flag requires manifest or input files to be specified!
 error.bad.uflag=\

Cheers,
Henry


> On Feb 2, 2017, at 10:58 AM, Henry Jen <henry@oracle.com> wrote:
> 
> Hi,
> 
> Please review a trivial patch for JDK-8171522, basically we just add two 
> missing main operations into the message. This message is appropriate for 
> both compatible style or new gnu style.
> 
> Cheers,
> Henry
> 
> diff -r 0e2935453091 
> src/jdk.jartool/share/classes/sun/tools/jar/resources/jar.properties
> --- a/src/jdk.jartool/share/classes/sun/tools/jar/resources/jar.properties
>   Wed Feb 01 11:05:33 2017 -0800
> +++ b/src/jdk.jartool/share/classes/sun/tools/jar/resources/jar.properties
>   Thu Feb 02 10:48:11 2017 -0800
> @@ -36,7 +36,7 @@
> error.bad.file.arg=\
>  Error parsing file arguments
> error.bad.option=\
> -One of options -{ctxu} must be specified.
> +One of the option -{ctxuid} must be specified.
> error.bad.cflag=\
> 'c' flag requires manifest or input files to be specified!
> error.bad.uflag=\

RFR: 8171522: Jar prints error message with old (non gnu-style options).

[Henry Jen]

Hi,

Please review a trivial patch for JDK-8171522, basically we just add two 
missing main operations into the message. This message is appropriate for both 
compatible style or new gnu style.

Cheers,
Henry

diff -r 0e2935453091 
src/jdk.jartool/share/classes/sun/tools/jar/resources/jar.properties
--- a/src/jdk.jartool/share/classes/sun/tools/jar/resources/jar.properties  
Wed Feb 01 11:05:33 2017 -0800
+++ b/src/jdk.jartool/share/classes/sun/tools/jar/resources/jar.properties  
Thu Feb 02 10:48:11 2017 -0800
@@ -36,7 +36,7 @@
 error.bad.file.arg=\
  Error parsing file arguments
 error.bad.option=\
-One of options -{ctxu} must be specified.
+One of the option -{ctxuid} must be specified.
 error.bad.cflag=\
 'c' flag requires manifest or input files to be specified!
 error.bad.uflag=\

RFR: 8171524: jar --help doesn't provide information that stdout and stdin can be used as output and input for tool

[Henry Jen]

Hi,

Please review a trivial fix[1] for JDK-8171524.

The fix added a short paragraph about @argfile. Clarify stdin or stdout is used 
if —file is omitted.

It also correct that -h, not -? is for the short option for —help.

Cheers,
Henry

[1] http://cr.openjdk.java.net/~henryjen/jdk9/8171524/0/webrev/

Re: RFR: 8172309: classpath wildcards code does not support --class-path

[Henry Jen]

How about following patch instead for langtools repo?

diff -r ef142ac9824e test/tools/javac/Paths/wcMineField.sh
--- a/test/tools/javac/Paths/wcMineField.sh Thu Jan 26 16:53:56 2017 -0800
+++ b/test/tools/javac/Paths/wcMineField.sh Fri Jan 27 13:10:12 2017 -0800
@@ -26,7 +26,7 @@
 #
 # @test
 # @summary Test classpath wildcards for javac and java -classpath option.
-# @bug 6268383
+# @bug 6268383 8172309
 # @run shell/timeout=600 wcMineField.sh

 # To run this test manually, simply do ./wcMineField.sh
@@ -186,6 +186,8 @@
 Failure "$javac" ${TESTTOOLVMOPTS} -classpath "GooJar/*${PS}." Main1.java
 Success "$javac" ${TESTTOOLVMOPTS} -cp "GooJar/SubDir/*" Main1.java
 Success "$javac" ${TESTTOOLVMOPTS} -classpath "GooJar/SubDir/*" Main1.java
+Success "$javac" ${TESTTOOLVMOPTS} --class-path "GooJar/SubDir/*" Main1.java
+Success "$javac" ${TESTTOOLVMOPTS} --class-path="GooJar/SubDir/*" Main1.java
 #Same with launcher. Should not load jar in subdirectories unless specified
 Failure "$java" ${TESTVMOPTS}  -classpath "GooJar/*${PS}." Main1
 Success "$java" ${TESTVMOPTS}  -classpath "GooJar/SubDir/*${PS}." Main1

Cheers,
Henry


> On Jan 26, 2017, at 6:01 PM, Henry Jen <henry@oracle.com> wrote:
> 
>> On Jan 26, 2017, at 4:09 PM, Jonathan Gibbons <jonathan.gibb...@oracle.com> 
>> wrote:
>> 
>> Henry,
>> 
>> You've changed old test cases into new test cases, thereby eliminating the 
>> old cases, which is not so good.
>> 
>> You should be adding new test cases, but changing old ones.
>> 
> 
> I am not sure, I believe all wild-card cases still tested, just that we have 
> different variety of calling -cp. Of course, it would be nice to test all 
> cases with all 4 flavors of -cp, but I don’t think that’s necessary as I 
> think the test coverage is the same.
> 
> Anyway, I could be wrong. If you feel strong about this, I can redo it. But 
> that may takes more time to digest what the test cases are really for.
> 
> Cheers,
> Henry
> 
> 
>> -- Jon
>> 
>> 
>> On 01/26/2017 02:31 PM, Kumar Srinivasan wrote:
>>> 
>>> Hi Henry,
>>> 
>>> Looks ok to me. Thanks for making this change.
>>> 
>>> Kumar
>>> 
>>>> Hi,
>>>> 
>>>> Please review the webrev[1], the fix is to ensure —class-path and 
>>>> —class-path= is processed correctly to expand wildcard. Changes are made 
>>>> in jdk repo. However, test case to verify the bug fix is in langtool repo.
>>>> 
>>>> Cheers,
>>>> Henry
>>>> 
>>>> [1] http://cr.openjdk.java.net/~henryjen/jdk9/8172309/0/
>>>> [2] https://bugs.openjdk.java.net/browse/JDK-8172309
>>> 
>> 
> 

Re: RFR: 8172309: classpath wildcards code does not support --class-path

[Henry Jen]

> On Jan 26, 2017, at 4:09 PM, Jonathan Gibbons  
> wrote:
> 
> Henry,
> 
> You've changed old test cases into new test cases, thereby eliminating the 
> old cases, which is not so good.
> 
> You should be adding new test cases, but changing old ones.
> 

I am not sure, I believe all wild-card cases still tested, just that we have 
different variety of calling -cp. Of course, it would be nice to test all cases 
with all 4 flavors of -cp, but I don’t think that’s necessary as I think the 
test coverage is the same.

Anyway, I could be wrong. If you feel strong about this, I can redo it. But 
that may takes more time to digest what the test cases are really for.

Cheers,
Henry


> -- Jon
> 
> 
> On 01/26/2017 02:31 PM, Kumar Srinivasan wrote:
>> 
>> Hi Henry,
>> 
>> Looks ok to me. Thanks for making this change.
>> 
>> Kumar
>> 
>>> Hi,
>>> 
>>> Please review the webrev[1], the fix is to ensure —class-path and 
>>> —class-path= is processed correctly to expand wildcard. Changes are made in 
>>> jdk repo. However, test case to verify the bug fix is in langtool repo.
>>> 
>>> Cheers,
>>> Henry
>>> 
>>> [1] http://cr.openjdk.java.net/~henryjen/jdk9/8172309/0/
>>> [2] https://bugs.openjdk.java.net/browse/JDK-8172309
>> 
> 

RFR: 8172309: classpath wildcards code does not support --class-path

[Henry Jen]

Hi,

Please review the webrev[1], the fix is to ensure —class-path and —class-path= 
is processed correctly to expand wildcard. Changes are made in jdk repo. 
However, test case to verify the bug fix is in langtool repo.

Cheers,
Henry

[1] http://cr.openjdk.java.net/~henryjen/jdk9/8172309/0/
[2] https://bugs.openjdk.java.net/browse/JDK-8172309

Updated RFR: 8170832: Add a new launcher environment variable JAVA_OPTIONS

[Henry Jen]

> On Jan 25, 2017, at 12:14 PM, Henry Jen <henry@oracle.com> wrote:
> 
> 
>> On Jan 25, 2017, at 11:32 AM, Kumar Srinivasan 
>> <kumar.x.sriniva...@oracle.com> wrote:
>> 
>> 
>> Hi Henry,
>> 
>> I was somewhat surprised to see changes to launcher_LANG.properties, I 
>> usually
>> make the change in the english/default locale and allow the L1ON team to 
>> make the
>> locale specific changes, but if you are confident of the changes, that is 
>> fine.

I just ordering the sentence, it has slim chance to screw up. :)
Anyhow, if L10N team is monitoring the chance, they should double check.

Updated webrev address feedbacks so far can be found at 
http://cr.openjdk.java.net/~henryjen/jdk9/8170832/5/webrev/

Cheers,
Henry

Re: RFR: 8170832: Add a new launcher environment variable JAVA_OPTIONS

[Henry Jen]

> On Jan 25, 2017, at 11:32 AM, Kumar Srinivasan 
>  wrote:
> 
> 
> Hi Henry,
> 
> I was somewhat surprised to see changes to launcher_LANG.properties, I usually
> make the change in the english/default locale and allow the L1ON team to make 
> the
> locale specific changes, but if you are confident of the changes, that is 
> fine.
> 
> src/java.base/share/native/libjli/args.c
> 
> +// Check if Main class specified after argument checked
> +// Must be after expansion so we can caught if main class specified 
> @argfile
> 
> 
> Is my interpretation accurate ? If so please reword it accordingly.
> /*
> * Check if main-class is specified after argument being checked, it
> * must always appear after expansion,  as a  main-class specified in
> * an @argfile is not allowed, and it must be caught now.
> */

Not exactly, main-class or other terminal operation specified in the 
environment variable is not allowed, even if it is from @argfile.
However, @argfile directly on command line has no such limitations. I’ll update 
accordingly.

> 
> +assert (*env == '\0' || isspace(*env));
> 
> asserts are not enable in product builds, is the intention only
> for debug builds ? Should this be flagged as a warning or something
> under tracing ?
> 

Yes, that’s what should happen if the code if implemented correctly regardless 
the input.

> src/java.base/windows/native/libjli/cmdtoargs.c
> 
> -// iterate through rest of coammand line
> 
> +// iterate through rest of command line
> 

Fixed.

Cheers,
Henry

Re: RFR: 8170832: Add a new launcher environment variable JAVA_OPTIONS

[Henry Jen]

> On Jan 24, 2017, at 12:41 PM, Mandy Chung <mandy.ch...@oracle.com> wrote:
> 
> 
>> On Jan 24, 2017, at 10:20 AM, Henry Jen <henry@oracle.com> wrote:
>> 
>> Hi,
>> 
>> Please review the webrev[1] that add support for JAVA_OPTIONS environment 
>> variable. The bug[2] describes how JAVA_OPTIONS works.
>> 
>> [1] http://cr.openjdk.java.net/~henryjen/jdk9/8170832/4/webrev/
> 
> This looks quite good.  A couple of minor comments:
> 
> 503 // Must be after expansion so we can caught if main class 
> specified @argfile
> 
> typo: s/caught/catch.  It’d be clear to simply say:
> // Check if Main class specified after argument checked.
> // This check must be done after expansion.
> 

Fixed.

>  42 #define JAVA_OPTIONS “JAVA_OPTIONS"
> 
> I think java.h is the appropriate file to declare this instead of jli_util.h.
> 

Well, no disagreement. I moved it and add a separate entry in args.c.
The reason it was in jli_util.h is follows the _JAVA_LAUNCHER_DEBUG. and 
JAVA_OPTIONS is for launcher. It is used by args.c which include jli_util.h but 
not full java.h in standalone mode. 

> emessages.h
>  Would it be clearer to rename ARG_INFO to ARG_INFO_ENVVAR?
> 

Again, try o be consistent with existing style. Changed.

Cheers,
Henry

RFR: 8170832: Add a new launcher environment variable JAVA_OPTIONS

[Henry Jen]

Hi,

Please review the webrev[1] that add support for JAVA_OPTIONS environment 
variable. The bug[2] describes how JAVA_OPTIONS works.

[1] http://cr.openjdk.java.net/~henryjen/jdk9/8170832/4/webrev/
[2] https://bugs.openjdk.java.net/browse/JDK-8170832

Cheers,
Henry

Re: JEP 293: Guidelines for JDK Command-Line Tool Options - @-files

[Henry Jen]

OS-specific encoding, but has to be ASCII friendly, modern system with UTF-8 as 
system encoding should work just fine.

Space in quote should work just fine, for example, “c:\\Program Files” should 
be correct. Can you post messages from JLink? Also if you can verify java is 
working OK with space, that would be helpful to tell if there is something 
different in JLink.

Cheers,
Henry

On October 27, 2016 at 2:27:44 PM, Robert Scholte (rfscho...@apache.org) wrote:
> Hi,
>  
> I'm facing some troubles with the content of the @-files and the
> documentation[1] isn't helping me yet.
>  
> First of all it doesn't mention the encoding, I assume it is the OS
> specific encoding.
>  
> I'm facing issues with a long --module-path on Windows.
> I noticed I can use the "normal" filename (not a URI or URL), but I need
> to escape the \ with an \, resulting in
> "E:\\java-workspace\\sandbox\\mvnexbook-examples-1.0\\ch-multi-spring\\simple-parent\\simple-command\\target\\classes;E:\\java-workspace\\sandbox\\mvnexbook-examples-1.0\\ch-multi-spring\\simple-parent\\simple-weather\\target\\simple-weather-0.8-SNAPSHOT.jar;"
>   
> (and much more entries)
>  
> You can use a space or a newline as separator between arguments, but if an
> argument contains spaces it must be surrounded with double quotes.
> However, the path to my local (Maven) repository contains a space and
> JLink is complaining it cannot find these dependencies. It seems like I
> need to escape spaces too, though I haven't figured out how to do that.
> Changing my local repo to a space-less path works. There should be a way
> to handle spaces, but how?
>  
> I'm not sure if there are other specific things to keep in mind when
> writing @-files, but it would be nice if that would be explained it the
> jep too.
>  
> thanks,
> Robert
>  
> [1] http://openjdk.java.net/jeps/293
>  

Re: RFR: 8042148: Ensure that the java launcher help is consistent with the manpage where they report common information

[Henry Jen]

Well, comments about -Xcomp and -Xusealtsigs doesn’t seem to require a change, 
are we good on this webrev?

Let me know if there is anything need to change, otherwise, I would need 
official +1 from reviewers. :)

Cheers,
Henry

On September 8, 2016 at 10:39:13 PM, Alan Bateman (alan.bate...@oracle.com) 
wrote:
>  
>  
> On 08/09/2016 20:52, Henry Jen wrote:
> > Hi,
> >
> > Please review a trivial fix for bug 8042148 at following URL:
> > Webrev: http://cr.openjdk.java.net/~henryjen/jdk9/8042148/webrev/
> > Bug: https://bugs.openjdk.java.net/browse/JDK-8042148
> >
> >
> The ordering/re-wording looks okay, just surprised to see -Xcomp added
> (I realize the `java -X` output has listed -Xint and -Xmixed for a long
> time).
>  
> -Alan
>  

Re: RFR: 8042148: Ensure that the java launcher help is consistent with the manpage where they report common information

[Henry Jen]

According to David in the comments, -Xusealtsigs is no longer an option, is it?

Cheers,
Henry

On September 8, 2016 at 3:27:21 PM, Kumar Srinivasan 
(kumar.x.sriniva...@oracle.com) wrote:
> Hi Henry,
> 
> Looks a lot nicer with the alpha ordering, but it seems to be missing
> -Xusealtsigs use alternative signals instead of SIGUSR1 and SIGUSR2 for
> JVM internal signals
> 
> Kumar
> 
> 
> On 9/8/2016 12:52 PM, Henry Jen wrote:
> > Hi,
> >
> > Please review a trivial fix for bug 8042148 at following URL:
> > Webrev: http://cr.openjdk.java.net/~henryjen/jdk9/8042148/webrev/
> > Bug: https://bugs.openjdk.java.net/browse/JDK-8042148
> >
> >
> > The fix added options asked for as discussed in the bug comments, and sort 
> > those options 
> in alphabetical order like the web page does.
> >
> > Cheers,
> > Henry
> >
> 
> 

RFR: 8042148: Ensure that the java launcher help is consistent with the manpage where they report common information

[Henry Jen]

Hi,

Please review a trivial fix for bug 8042148 at following URL:
Webrev: http://cr.openjdk.java.net/~henryjen/jdk9/8042148/webrev/
Bug: https://bugs.openjdk.java.net/browse/JDK-8042148


The fix added options asked for as discussed in the bug comments, and sort 
those options in alphabetical order like the web page does.

Cheers,
Henry

Re: RFR: 8081388: JNI exception pending in jdk/src/windows/bin/java_md.c

[Henry Jen]

That’s what I think too, this is just to silent parfait.
I don’t know for sure that we always get NULL with exception pending though.

Cheers,
Henry

On September 1, 2016 at 12:34:02 AM, David Holmes (david.hol...@oracle.com) 
wrote:
> On 1/09/2016 5:51 AM, Henry Jen wrote:
> > Hi,
> >
> > Please review a trivial fix for 8081388, in a nutshell,
> >
> > - Return NULL from NewPlatformStringArray if an exception occurred
> > - All other places call this function already handled return value NULL
> > - Launcher handles exception in JavaMain, report error and exit.
> >
> > Cheers,
> > Henry
> >
> > diff --git a/src/java.base/share/native/libjli/java.c 
> > b/src/java.base/share/native/libjli/java.c  
> > --- a/src/java.base/share/native/libjli/java.c
> > +++ b/src/java.base/share/native/libjli/java.c
> > @@ -1497,6 +1497,7 @@
> >
> > NULL_CHECK0(cls = FindBootStrapClass(env, "java/lang/String"));
> > NULL_CHECK0(ary = (*env)->NewObjectArray(env, strc, cls, 0));
> > + CHECK_EXCEPTION_RETURN_VALUE(0);
>  
> You will only get a NULL if an exception is pending; conversely you will
> only have an exception pending if the return value is NULL. The new
> check will never execute in a "positive way" and is unnecessary.
>  
> David
> -
>  
> > for (i = 0; i < strc; i++) {
> > jstring str = NewPlatformString(env, *strv++);
> > NULL_CHECK0(str);
> > diff --git a/src/java.base/share/native/libjli/java.h 
> > b/src/java.base/share/native/libjli/java.h  
> > --- a/src/java.base/share/native/libjli/java.h
> > +++ b/src/java.base/share/native/libjli/java.h
> > @@ -253,6 +253,13 @@
> > #define NULL_CHECK(NC_check_pointer) \
> > NULL_CHECK_RETURN_VALUE(NC_check_pointer, )
> >
> > +#define CHECK_EXCEPTION_RETURN_VALUE(CER_value) \
> > + do { \
> > + if ((*env)->ExceptionOccurred(env)) { \
> > + return CER_value; \
> > + } \
> > + } while (JNI_FALSE)
> > +
> > #define CHECK_EXCEPTION_RETURN() \
> > do { \
> > if ((*env)->ExceptionOccurred(env)) { \
> >
>  

RFR: 8081388: JNI exception pending in jdk/src/windows/bin/java_md.c

[Henry Jen]

Hi,

Please review a trivial fix for 8081388, in a nutshell,

- Return NULL from NewPlatformStringArray if an exception occurred
- All other places call this function already handled return value NULL
- Launcher handles exception in JavaMain, report error and exit.

Cheers,
Henry

diff --git a/src/java.base/share/native/libjli/java.c 
b/src/java.base/share/native/libjli/java.c
--- a/src/java.base/share/native/libjli/java.c
+++ b/src/java.base/share/native/libjli/java.c
@@ -1497,6 +1497,7 @@

     NULL_CHECK0(cls = FindBootStrapClass(env, "java/lang/String"));
     NULL_CHECK0(ary = (*env)->NewObjectArray(env, strc, cls, 0));
+    CHECK_EXCEPTION_RETURN_VALUE(0);
     for (i = 0; i < strc; i++) {
         jstring str = NewPlatformString(env, *strv++);
         NULL_CHECK0(str);
diff --git a/src/java.base/share/native/libjli/java.h 
b/src/java.base/share/native/libjli/java.h
--- a/src/java.base/share/native/libjli/java.h
+++ b/src/java.base/share/native/libjli/java.h
@@ -253,6 +253,13 @@
 #define NULL_CHECK(NC_check_pointer) \
     NULL_CHECK_RETURN_VALUE(NC_check_pointer, )

+#define CHECK_EXCEPTION_RETURN_VALUE(CER_value) \
+    do { \
+        if ((*env)->ExceptionOccurred(env)) { \
+            return CER_value; \
+        } \
+    } while (JNI_FALSE)
+
 #define CHECK_EXCEPTION_RETURN() \
     do { \
         if ((*env)->ExceptionOccurred(env)) { \

Re: [9] RFR: 8162343: non-ASCII characters in source code comments (.hpp)

[Henry Jen]

Looks good to me, but I am not a Reviewer.

Cheers,
Henry


On July 26, 2016 at 10:16:36 AM, Naoto Sato (naoto.s...@oracle.com) wrote:
> Ping.
> 
> On 7/21/16 3:44 PM, Naoto Sato wrote:
> > Hello,
> >
> > Please review the change to the following issue:
> >
> > https://bugs.openjdk.java.net/browse/JDK-8162343
> >
> > The proposed fix is located at:
> >
> > http://cr.openjdk.java.net/~naoto/8162343/webrev.00/
> >
> > It's a trivial fix following the bug 8161937.
> >
> > Naoto
> 

Re: RFR: 8132379: -J options can cause crash or "Warning: app args parsing error passing arguments as-is"

[Henry Jen]

Kumar,

Thanks for the review.

Cheers,
Henry

On July 13, 2016 at 6:43:09 AM, Kumar Srinivasan 
(kumar.x.sriniva...@oracle.com) wrote:
> 
> Henry,
> 
> Thanks for fixing this. Looks good.
> 
> Kumar
> 
> > Hi,
> >
> > Please review a fix for JDK-8132379, the fix is to build matching index to 
> > the original 
> arguments for the application arguments, used later for the sanity check and 
> wildcard 
> expansion. The fix is specific to Windows platform.
> >
> > -J prefix used by launcher-based tools such as javac need to be filtered 
> > out, but not 
> for java itself, where -J prefix could be used by the main class.
> >
> > Use index table avoid copy of arguments and use less memory. The fix is 
> > tested on Windows 
> 10.
> >
> > Bug: https://bugs.openjdk.java.net/browse/JDK-8132379
> > Webrev: http://cr.openjdk.java.net/~henryjen/jdk9/8132379.0/webrev/
> >
> > Cheers,
> > Henry
> >
> 
> 

RFR: 8132379: -J options can cause crash or "Warning: app args parsing error passing arguments as-is"

[Henry Jen]

Hi,

Please review a fix for JDK-8132379, the fix is to build matching index to the 
original arguments for the application arguments, used later for the sanity 
check and wildcard expansion. The fix is specific to Windows platform.

-J prefix used by launcher-based tools such as javac need to be filtered out, 
but not for java itself, where -J prefix could be used by the main class.

Use index table avoid copy of arguments and use less memory. The fix is tested 
on Windows 10.

Bug: https://bugs.openjdk.java.net/browse/JDK-8132379
Webrev: http://cr.openjdk.java.net/~henryjen/jdk9/8132379.0/webrev/

Cheers,
Henry

Re: Fwd: Files.walk() is unusable because of AccessDeniedException

[Henry Jen]

I think there is a work-around, use list() and flatMap() should get you what 
you needed.

The API is designed to walk a tree where you suppose to have access with. If OS 
level cause an IOException, that need to be dealt with. Acknowledged that 
exception handling is not a strong suite in Stream API, developer will need to 
do some work.

Files.find() also allows you to get entries and filter out by permission. What 
you can do is make sure you have permission on the top level, then call find 
with maxDepth 1 to only get entries on that directory.

Combined with flatMap(), you should be able to get what you want. Try the 
following code to see if it works for you.

import java.nio.file.Files;
import java.nio.file.Path;
import java.nio.file.Paths;
import java.util.stream.Stream;
import java.io.IOException;

public class ListCanRead {
    static Stream walkReadable(Path p) {
        if (Files.isReadable(p)) {
            if (Files.isDirectory(p)) {
                try {
                    return Stream.concat(Stream.of(p), Files.list(p));
                } catch (IOException ioe) {
                    return Stream.of(p);
                }
            } else {
                return Stream.of(p);
            }
        }
        return Stream.of(p);
    }

    public static void main(String[] args) throws IOException {
        System.out.println("List directory: " + args[0]);
        walkReadable(Paths.get(args[0])).flatMap(ListCanRead::walkReadable)
            .forEach(System.out::println);

        Files.walk(Paths.get(args[0]))
            .forEach(System.out::println); // Could throw AccessDeniedException
    }
}

Cheers,
Henry

On May 24, 2016 at 4:48:30 PM, Gilles Habran (gilleshab...@gmail.com) wrote:
> Good morning,
>  
> well I would like to be able to manage the outcome of the processing myself
> and not get an exception in a part where I have no control.
>  
> For example, I would expect to get an exception when I tried to read a file
> where I don't have the permission. I would not expect to get an exception
> when Java creates the Stream.
>  
> Maybe I am the only one to have a problem with this ? I don't know but it
> feels strange to be forced to execute a software with root permissions
> where I don't even plan to read file I cannot read because of lack of
> permissions.
>  
> For me, we should be able to test the attributes of a file and depending on
> the result, read the file or not. If we read the file without permission,
> we get an exception, if not, we can go to the next file without error.
>  
> If that's unclear, please let me know, I'll try to give more informations
> or examples.
>  
> Thank you.
>  
> On 24 May 2016 at 10:19, Andrew Haley wrote:
>  
> > On 05/20/2016 10:38 AM, Gilles Habran wrote:
> > > why is my message still waiting for approval after a month ?
> >
> > What is it you want Java to do? You can't walk the directory
> > because you don't have permission. sudo should work.
> >
> > Andrew.
> >
> >
>  

Re: RFR: JDK-8115868: 32-bit JVM failed to start from a large network filesystem

[Henry Jen]

Changes looks reasonable to me.

Cheers,
Henry


> On Dec 15, 2015, at 7:54 AM, Kumar Srinivasan  
> wrote:
> 
> Hello,
> 
> Please review fix for: JDK-8115868
> 
> The webrev is here:
> http://cr.openjdk.java.net/~ksrini/8115868/webrev.0/
> 
> The background:
> The launcher uses stat(2) to check for the existence of a file, unfortunately
> on 32-bit system with large file systems causes the inode storage to overflow
> causing the syscall to return EOVERFLOW.
> 
> Solution:
> * stat(2)  replaced with access(3), in most cases.
> * jbs is marked noreg-hard hard to replicate the problem
> 
> Thanks
> Kumar
> 
> 
> 
> 
> 

Re: RFR 8144675: Add a filtering collector

[Henry Jen]

Uh, you did mention you need entry with value zero. I knew I miss something. :)

Cheers,
Henry


> On Dec 4, 2015, at 12:53 PM, ShinyaYoshida <bitterf...@gmail.com> wrote:
> 
> Thank you for your comment.
> 
> 2015-12-05 3:28 GMT+09:00 Henry Jen <henry@oracle.com>:
> My first thought is what’s wrong with
> 
> stream.filter(predicate).collect(...)?
> 
> In your case, that would be,
> 
> amps.stream().filter(e -> e.getSalary() > 2000)
> .collect(groupingBy(Employee::getDepartment), Collectors.counting())
> 
> That should work just fine?
> Unfortunately, not, Stream#filter could not satisfy us in this case.
> Because when I filter before collecting, the entry which the value is 0 will 
> not be appeared in the result map.
> Ex)
> filtering collector(Collectors#filtering): http://ideone.com/83Lzb7
> filtering before collecting(Stream#filter): http://ideone.com/EmTrXE
> 
> Regards,
> shinyafox(ShinyaYoshida)
>  
> 
> Cheers,
> Henry
> 
> 
> 
> > On Dec 3, 2015, at 10:57 PM, ShinyaYoshida <bitterf...@gmail.com> wrote:
> >
> > Hi, core-libs-dev and Brian, Paul,
> > I'd like to propose adding filtering method to Collectors.
> >
> > When I consider the operation what is "grouping the number of employees
> > whose income is over 2000 by the depertment from employees", we have to
> > write following because there is no way to filter for Collector:
> > (Note: In this case, we need the entry which the value is 0)
> >
> > Map<Department, Long> map = emps.stream()
> >.collect(groupingBy(Employee::getDepartment,
> >collectingAndThen(toList(),
> >es -> es.stream().filter(e -> e.getSalary() > 2000).count(;
> >
> > When I add filtering like following to Collectors, we can write it easy,
> > and it would be more efficient.
> >public static <T, A, R>
> >Collector<T, ?, R> filtering(Predicate filter,
> >   Collector downstream) {
> >BiConsumer<A, ? super T> downstreamAccumulator =
> > downstream.accumulator();
> >return new CollectorImpl<>(downstream.supplier(),
> >   (r, t) -> {
> >   if (filter.test(t)) {
> >   downstreamAccumulator.accept(r,
> > t);
> >   }
> >   },
> >   downstream.combiner(),
> > downstream.finisher(),
> >   downstream.characteristics());
> >}
> >
> > Map<Department, Long> map = emps.stream()
> >.collect(groupingBy(Employee::getDepartment,
> >filtering(e -> e.getSalary() > 2000, counting(;
> >
> > Here is patch:
> > webrev: http://cr.openjdk.java.net/~shinyafox/8144675/webrev.00/
> > bugs: https://bugs.openjdk.java.net/browse/JDK-8144675
> >
> > Could you consider this patch and proposal?
> >
> > Regards,
> > shinyafox(Shinya Yoshida)
> 
> 

Re: RFR 8144675: Add a filtering collector

[Henry Jen]

My first thought is what’s wrong with

stream.filter(predicate).collect(...)?

In your case, that would be,

amps.stream().filter(e -> e.getSalary() > 2000)
.collect(groupingBy(Employee::getDepartment), Collectors.counting())

That should work just fine?

Cheers,
Henry



> On Dec 3, 2015, at 10:57 PM, ShinyaYoshida  wrote:
> 
> Hi, core-libs-dev and Brian, Paul,
> I'd like to propose adding filtering method to Collectors.
> 
> When I consider the operation what is "grouping the number of employees
> whose income is over 2000 by the depertment from employees", we have to
> write following because there is no way to filter for Collector:
> (Note: In this case, we need the entry which the value is 0)
> 
> Map map = emps.stream()
>.collect(groupingBy(Employee::getDepartment,
>collectingAndThen(toList(),
>es -> es.stream().filter(e -> e.getSalary() > 2000).count(;
> 
> When I add filtering like following to Collectors, we can write it easy,
> and it would be more efficient.
>public static 
>Collector filtering(Predicate filter,
>   Collector downstream) {
>BiConsumer downstreamAccumulator =
> downstream.accumulator();
>return new CollectorImpl<>(downstream.supplier(),
>   (r, t) -> {
>   if (filter.test(t)) {
>   downstreamAccumulator.accept(r,
> t);
>   }
>   },
>   downstream.combiner(),
> downstream.finisher(),
>   downstream.characteristics());
>}
> 
> Map map = emps.stream()
>.collect(groupingBy(Employee::getDepartment,
>filtering(e -> e.getSalary() > 2000, counting(;
> 
> Here is patch:
> webrev: http://cr.openjdk.java.net/~shinyafox/8144675/webrev.00/
> bugs: https://bugs.openjdk.java.net/browse/JDK-8144675
> 
> Could you consider this patch and proposal?
> 
> Regards,
> shinyafox(Shinya Yoshida)

Re: RFR v6 - 8027634: Support @argfiles for java command-line tool

[Henry Jen]

Kumar, Mandy,

Thanks for reviewing.

Cheers,
Henry


 On Aug 22, 2015, at 12:42 PM, Kumar Srinivasan 
 kumar.x.sriniva...@oracle.com wrote:
 
 Henry,
 
 Looks good, and thank you for taking this on.
 
 Kumar
 
 On 8/21/2015 3:46 PM, Henry Jen wrote:
 v7 is up, changes are
 
 - Add formfeed character(\u000c) as while space character
 - Support escape \f for formfeed character in quote
 - Update java help output to include @filepath and -Xdisable-@files
 
 http://cr.openjdk.java.net/~henryjen/jdk9/8027634/v7/webrev/
 
 Cheers,
 Henry
 
 
 On Aug 18, 2015, at 11:00 AM, Mandy Chung mandy.ch...@oracle.com wrote:
 
 
 
 On 08/17/2015 09:15 PM, Henry Jen wrote:
 v6 is available at
 
 http://cr.openjdk.java.net/~henryjen/jdk9/8027634/v6/webrev/
 Thanks for the update.  Looks fine.
 
 Mandy
 

Re: RFR v6 - 8027634: Support @argfiles for java command-line tool

[Henry Jen]

v7 is up, changes are

- Add formfeed character(\u000c) as while space character
- Support escape \f for formfeed character in quote
- Update java help output to include @filepath and -Xdisable-@files

http://cr.openjdk.java.net/~henryjen/jdk9/8027634/v7/webrev/

Cheers,
Henry


 On Aug 18, 2015, at 11:00 AM, Mandy Chung mandy.ch...@oracle.com wrote:
 
 
 
 On 08/17/2015 09:15 PM, Henry Jen wrote:
 v6 is available at
 
 http://cr.openjdk.java.net/~henryjen/jdk9/8027634/v6/webrev/
 
 Thanks for the update.  Looks fine.
 
 Mandy

Re: RFR v5 - 8027634: Support @argfiles for java command-line tool

[Henry Jen]

 On Aug 16, 2015, at 4:51 PM, Henry Jen henry@oracle.com wrote:
 
 Thanks for reviewing, comment inline below,
 
 On Aug 14, 2015, at 4:07 PM, Mandy Chung mandy.ch...@oracle.com wrote:
 
 
 On Aug 14, 2015, at 1:10 PM, Henry Jen henry@oracle.com wrote:
 
 Hi,
 
 Another minor revision address comments, no real behavior changes except 
 use JLI_StrCmp instead of JLI_StrCCmp in checkArg().
 
 http://cr.openjdk.java.net/~henryjen/jdk9/8027634/v5/webrev/
 
 main.c
  JLI_PreprocessingArg returns NULL if not @argfile
 
 Would it be better to return JLI_List containing one element as argv[i]?  We 
 want to avoid new/free JLI_List for every argument and maybe a preallocated 
 reusable copy for single element list to use (non-growable)?
 
 Only argument with @prefix will be expanded, for any other cases, the 
 function return NULL. This avoid any copy when not necessary. Regular 
 argument is left alone, so that caller will just use the original value.

I think I may be talking at a different thing, guess you meant the make 
JLI_List JLI_PreprocessArg(const char*) to void JLI_PreprocessArg(JLI_List, 
const char*) and pass in a list to hold all the arguments.

That is reasonable. Considered that but didn’t do it eventually because not 
much benefits and hide the “expansion” fact. Since now you mentioned this, it 
probably worth to do it that way.

Cheers,
Henry

RFR v6 - 8027634: Support @argfiles for java command-line tool

[Henry Jen]

 On Aug 17, 2015, at 8:45 PM, Mandy Chung mandy.ch...@oracle.com wrote:
 
 OK.   firstAppArgIndex is not used in the parsing.

It is set in args.c:127, and stop expansion on line 369.

v6 is available at 

http://cr.openjdk.java.net/~henryjen/jdk9/8027634/v6/webrev/

Changes based on Mandy’s comment,

- add a new test, basCases, which not escaping backslash properly and cause an 
open quote and failed java in ArgFileSyntac.java
- rename killSwitchOn to stopExpansion in args.c
- change to use NOT_FOUND at args.c:87
- fix indentation at  ArgFileSyntax.java line 167-170

Cheers,
Henry

Re: RFR v5 - 8027634: Support @argfiles for java command-line tool

[Henry Jen]

On Aug 17, 2015, at 7:10 AM, Henry Jen henry@oracle.com wrote:
 
 
 On Aug 16, 2015, at 4:51 PM, Henry Jen henry@oracle.com wrote:
 
 Thanks for reviewing, comment inline below,
 
 On Aug 14, 2015, at 4:07 PM, Mandy Chung mandy.ch...@oracle.com wrote:
 
 
 On Aug 14, 2015, at 1:10 PM, Henry Jen henry@oracle.com wrote:
 
 Hi,
 
 Another minor revision address comments, no real behavior changes except 
 use JLI_StrCmp instead of JLI_StrCCmp in checkArg().
 
 http://cr.openjdk.java.net/~henryjen/jdk9/8027634/v5/webrev/
 
 main.c
 JLI_PreprocessingArg returns NULL if not @argfile
 
 Would it be better to return JLI_List containing one element as argv[i]?  
 We want to avoid new/free JLI_List for every argument and maybe a 
 preallocated reusable copy for single element list to use (non-growable)?
 
 Only argument with @prefix will be expanded, for any other cases, the 
 function return NULL. This avoid any copy when not necessary. Regular 
 argument is left alone, so that caller will just use the original value.
 
 I think I may be talking at a different thing, guess you meant the make 
 JLI_List JLI_PreprocessArg(const char*) to void JLI_PreprocessArg(JLI_List, 
 const char*) and pass in a list to hold all the arguments.
 
 That is reasonable. Considered that but didn’t do it eventually because not 
 much benefits and hide the “expansion” fact. Since now you mentioned this, it 
 probably worth to do it that way.
 

Now I remember another reason I didn’t do it, because of the wildcard 
processing in cmdtoargs.c. I don’t want to convert every platform to use StdArg 
or maintain different version of PreprocesArgs.

I am going to leave it as it.

Cheers,
Henry

Re: RFR v5 - 8027634: Support @argfiles for java command-line tool

[Henry Jen]

Thanks for reviewing, comment inline below,

 On Aug 14, 2015, at 4:07 PM, Mandy Chung mandy.ch...@oracle.com wrote:
 
 
 On Aug 14, 2015, at 1:10 PM, Henry Jen henry@oracle.com wrote:
 
 Hi,
 
 Another minor revision address comments, no real behavior changes except use 
 JLI_StrCmp instead of JLI_StrCCmp in checkArg().
 
 http://cr.openjdk.java.net/~henryjen/jdk9/8027634/v5/webrev/
 
 main.c
   JLI_PreprocessingArg returns NULL if not @argfile
 
 Would it be better to return JLI_List containing one element as argv[i]?  We 
 want to avoid new/free JLI_List for every argument and maybe a preallocated 
 reusable copy for single element list to use (non-growable)?

Only argument with @prefix will be expanded, for any other cases, the function 
return NULL. This avoid any copy when not necessary. Regular argument is left 
alone, so that caller will just use the original value.

 
 140 // Shallow free, we reuse the string to avoid copy
 141 JLI_MemFree(argsInFile-elements);
 142 JLI_MemFree(argsInFile);
 
 What about adding JLI_List_free(JLI_List sl, boolean shadow)?  This would be 
 useful for the reusable single element list. Same comment applied to 
 cmdtoargs.c
 

Thought of that, but decided to keep in place for clarity. When move into a 
function, then we want to check for NULL, and ask  how shallow the free is, do 
we keep the array but free the list or both?

 args.c
  You have #define NOT_FOUND -1 but NOT_FOUND is not used else where.
 

Used right under to initialize firstAppArgIndex, and should be used in 
following statement you shown.

  void JLI_InitArgProcessing(jboolean isJava, jboolean disableArgFile) {
  // for tools, this value remains 0 all the time.
  firstAppArgIndex = isJava ? -1 : 0;
 
 If I understand correctly, firstAppArgIndex set to 0 means that @argfile is 
 disabled.  On the other hand, ENABLE_ARG_FILES is also the flag to enable JDK 
 tools to enable @argfile (disabled by default).  It seems to me that you no 
 longer needs isJava parameter here.

firstAppArgIndex is the first index of user’s application argument, has nothing 
to do with disable @argfile. This value remains 0 for launcher-based tools, and 
returned by JLI_GetAppArgIndex().

A tools can have ENABLE_ARG_FILES to enable argument expansion, but we still 
need to know it’s a launcher-based tool.

  Might be killSwitchOn can be replaced with firstAppArgIndex == 0 case (not 
 sure - will let you think about that.)   killSwitchOn is unclear what it 
 means;  maybe renaming it to disableArgFile?
 

As explained earlier, firstAppArgIndex is different. We can rename 
killSwitchOn, it was clear as we discussed kill switch, the -X:disable-@files 
option. disableArgFile is used as the function argument, thus I left them as is.

 You may have to try building one tool with ENABLE_ARG_FILES to verify the 
 change.

java is built with the flag. Others are not. I tested with javac before the 
flag is reversed, so we know the flag is working.

 
 JLI_PreprocessArg - as mentioned in the comment above, I suggest to have 
 JLI_PreprocessArg to always return a non-null JLI_List and perhaps renaming 
 the method to JLI_ExpandArgumentList
 

NULL is better than non-NULL copy so that we don’t need to copy the argument or 
check content when it’s not @ prefixed.
The function was named JLI_ExpandArgumentList, later renamed to current form as 
it also check each argument to determine the location of first user argument 
for main class.

// We can not just update the idx here because if -jar @file
// still need expansion of @file to get the argument for -jar
 
 I only skimmed on the tests.
ArgFileSyntax.java line 167-170: nit: indentation should be 4-space
 

You are right, thanks. When I was making those changes, I noted the editor 
changed something, but didn’t realize it’s the indentation.

 It might be useful to add a few negative tests to sanity check especially on 
 the quotation.
 

Make sense. Do you mean test cases that will fail launch of java?

 TestHelper.java has no change - should be reverted.
 

It has two minor indentation clean up, I can revert them, but think since we 
were here, perhaps just fix it.

Cheers,
Henry

RFR v5 - 8027634: Support @argfiles for java command-line tool

[Henry Jen]

Hi,

Another minor revision address comments, no real behavior changes except use 
JLI_StrCmp instead of JLI_StrCCmp in checkArg().

http://cr.openjdk.java.net/~henryjen/jdk9/8027634/v5/webrev/

Cheers,
Henry