Integrated: 8287363: null pointer should use NULL instead of 0

[Yasumasa Suenaga]

On Sat, 28 May 2022 03:31:30 GMT, Yasumasa Suenaga  wrote:

> We found using `0` as `NULL` in java_md_common.c . `0` is not a pointer, so 
> we should use `NULL` where we want to handle it.
> 
> https://github.com/openjdk/jdk/pull/8646#discussion_r882294076
> 
> Also I found using `0` as NUL char in java_md_common.c , so I replaced it to 
> `\0` in this PR.

This pull request has now been integrated.

Changeset: a27ba1a3
Author:Yasumasa Suenaga 
URL:   
https://git.openjdk.java.net/jdk/commit/a27ba1a3db5f0b4eb75b6cca94f33398e7b695cc
Stats: 5 lines in 1 file changed: 0 ins; 0 del; 5 mod

8287363: null pointer should use NULL instead of 0

Reviewed-by: kbarrett, stuefe, alanb

-

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

RFR: 8287363: null pointer should use NULL instead of 0

[Yasumasa Suenaga]

We found using `0` as `NULL` in java_md_common.c . `0` is not a pointer, so we 
should use `NULL` where we want to handle it.

https://github.com/openjdk/jdk/pull/8646#discussion_r882294076

Also I found using `0` as NUL char in java_md_common.c , so I replaced it to 
`\0` in this PR.

-

Commit messages:
 - Merge remote-tracking branch 'upstream/master' into JDK-8287363
 - 8287363: null pointer should use NULL instead of 0

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

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

Integrated: 8286562: GCC 12 reports some compiler warnings

[Yasumasa Suenaga]

On Wed, 11 May 2022 05:58:31 GMT, Yasumasa Suenaga  wrote:

> I saw some compiler warnings when I tried to build OpenJDK with GCC 12.0.1 on 
> Fedora 36.
> As you can see, the warnings spreads several areas. Let me know if I should 
> separate them by area.
> 
> * -Wstringop-overflow
> * src/hotspot/share/oops/array.hpp
> * 
> src/hotspot/share/jfr/recorder/checkpoint/types/traceid/jfrTraceIdBits.inline.hpp
> 
> In member function 'void Array::at_put(int, const T&) [with T = unsigned 
> char]',
> inlined from 'void ConstantPool::tag_at_put(int, jbyte)' at 
> /home/ysuenaga/github-forked/jdk/src/hotspot/share/oops/constantPool.hpp:126:64,
> inlined from 'void ConstantPool::method_at_put(int, int, int)' at 
> /home/ysuenaga/github-forked/jdk/src/hotspot/share/oops/constantPool.hpp:380:15,
> inlined from 'ConstantPool* 
> BytecodeConstantPool::create_constant_pool(JavaThread*) const' at 
> /home/ysuenaga/github-forked/jdk/src/hotspot/share/classfile/bytecodeAssembler.cpp:85:26:

This pull request has now been integrated.

Changeset: 410a25d5
Author:Yasumasa Suenaga 
URL:   
https://git.openjdk.java.net/jdk/commit/410a25d59a11b6a627bbb0a2c405c2c2be19f464
Stats: 41 lines in 5 files changed: 26 ins; 1 del; 14 mod

8286562: GCC 12 reports some compiler warnings

Reviewed-by: ihse, kbarrett, prr

-

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

Re: RFR: 8286562: GCC 12 reports some compiler warnings [v9]

[Yasumasa Suenaga]

On Thu, 26 May 2022 03:48:31 GMT, Kim Barrett  wrote:

>> Yasumasa Suenaga has updated the pull request incrementally with two 
>> additional commits since the last revision:
>> 
>>  - Change Array::data() implementation
>>  - Avoid stringop-overflow warning in jfrTraceIdBits.inline.hpp
>
> src/java.base/unix/native/libjli/java_md_common.c line 133:
> 
>> 131: 
>> 132: snprintf_result = JLI_Snprintf(name, sizeof(name), "%s%c%s", indir, 
>> FILE_SEPARATOR, cmd);
>> 133: if ((snprintf_result < 0) && (snprintf_result >= 
>> (int)sizeof(name))) {
> 
> That should be `||` rather than `&&`.

Good catch! I fixed it in new commit.

> src/java.base/unix/native/libjli/java_md_common.c line 135:
> 
>> 133: if ((snprintf_result < 0) && (snprintf_result >= 
>> (int)sizeof(name))) {
>> 134:   return 0;
>> 135: }
> 
> Pre-existing: It seems odd that this returns `0` above and below, rather than 
> returning `NULL`.  I think there are one or two other places in this file 
> that are similarly using literal `0` for a null pointer, though others are 
> using `NULL`.  Something to report and clean up separately from this change.

I was wondering about that too.
I use `NULL` at L134, and have filed it as 
[JDK-8287363](https://bugs.openjdk.java.net/browse/JDK-8287363). I will work 
for it after this issue.

-

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

Re: RFR: 8286562: GCC 12 reports some compiler warnings [v10]

[Yasumasa Suenaga]

> I saw some compiler warnings when I tried to build OpenJDK with GCC 12.0.1 on 
> Fedora 36.
> As you can see, the warnings spreads several areas. Let me know if I should 
> separate them by area.
> 
> * -Wstringop-overflow
> * src/hotspot/share/oops/array.hpp
> * 
> src/hotspot/share/jfr/recorder/checkpoint/types/traceid/jfrTraceIdBits.inline.hpp
> 
> In member function 'void Array::at_put(int, const T&) [with T = unsigned 
> char]',
> inlined from 'void ConstantPool::tag_at_put(int, jbyte)' at 
> /home/ysuenaga/github-forked/jdk/src/hotspot/share/oops/constantPool.hpp:126:64,
> inlined from 'void ConstantPool::method_at_put(int, int, int)' at 
> /home/ysuenaga/github-forked/jdk/src/hotspot/share/oops/constantPool.hpp:380:15,
> inlined from 'ConstantPool* 
> BytecodeConstantPool::create_constant_pool(JavaThread*) const' at 
> /home/ysuenaga/github-forked/jdk/src/hotspot/share/classfile/bytecodeAssembler.cpp:85:26:

Yasumasa Suenaga has updated the pull request incrementally with one additional 
commit since the last revision:

  Fix comments

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/8646/files
  - new: https://git.openjdk.java.net/jdk/pull/8646/files/17cda224..851279ae

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=8646=09
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8646=08-09

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

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

Re: RFR: 8286562: GCC 12 reports some compiler warnings [v8]

[Yasumasa Suenaga]

On Wed, 25 May 2022 01:50:57 GMT, Kim Barrett  wrote:

>> Yasumasa Suenaga has updated the pull request with a new target base due to 
>> a merge or a rebase. The pull request now contains 11 commits:
>> 
>>  - Merge remote-tracking branch 'upstream/master' into gcc12-warnings
>>  - Use getter function to access "_data"
>>  - Revert changes for bytecodeAssembler.cpp, classFileParser.cpp, 
>> symbolTable.cpp
>>  - revert changes for memnode.cpp and type.cpp
>>  - Add assert to check the range of BasicType
>>  - Merge remote-tracking branch 'upstream/master' into HEAD
>>  - Revert change for java.c , parse_manifest.c , LinuxPackage.c
>>  - Calculate char offset before realloc()
>>  - Use return value from JLI_Snprintf
>>  - Avoid pragma error in before GCC 12
>>  - ... and 1 more: 
>> https://git.openjdk.java.net/jdk/compare/c156bcc5...042c1c70
>
> src/hotspot/share/oops/array.hpp line 102:
> 
>> 100:   // standard operations
>> 101:   int  length() const { return _length; }
>> 102:   T* data() const { return 
>> reinterpret_cast(reinterpret_cast(this) + 
>> base_offset_in_bytes()); }
> 
> Adding the const-qualifier to the `data()` function and then implicitly
> casting it away (by casting through intptr_t) is wrong.  Either don't
> const-qualify (and leave it to some future use that needs such to address
> appropriately), or have two functions.  Also, the line length is excessive.
> So this:
> 
> 
> T* data() {
>   return reinterpret_cast(
> reinterpret_cast(this) + base_offset_in_bytes());
> }
> 
> and optionally add this:
> 
> const T* data() const {
>   return reinterpret_cast(
> reinterpret_cast(this) + base_offset_in_bytes());
> }

Thanks a lot @kimbarrett !

I updated around stringop-overflow warning in jfrTraceIdBits.inline.hpp , and 
added two `data()` in `Array` class. They works fine on my GCC 12 on Fedora 36. 
Could you review again?

-

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

Re: RFR: 8286562: GCC 12 reports some compiler warnings [v9]

[Yasumasa Suenaga]

> I saw some compiler warnings when I tried to build OpenJDK with GCC 12.0.1 on 
> Fedora 36.
> As you can see, the warnings spreads several areas. Let me know if I should 
> separate them by area.
> 
> * -Wstringop-overflow
> * src/hotspot/share/oops/array.hpp
> * 
> src/hotspot/share/jfr/recorder/checkpoint/types/traceid/jfrTraceIdBits.inline.hpp
> 
> In member function 'void Array::at_put(int, const T&) [with T = unsigned 
> char]',
> inlined from 'void ConstantPool::tag_at_put(int, jbyte)' at 
> /home/ysuenaga/github-forked/jdk/src/hotspot/share/oops/constantPool.hpp:126:64,
> inlined from 'void ConstantPool::method_at_put(int, int, int)' at 
> /home/ysuenaga/github-forked/jdk/src/hotspot/share/oops/constantPool.hpp:380:15,
> inlined from 'ConstantPool* 
> BytecodeConstantPool::create_constant_pool(JavaThread*) const' at 
> /home/ysuenaga/github-forked/jdk/src/hotspot/share/classfile/bytecodeAssembler.cpp:85:26:

Yasumasa Suenaga has updated the pull request incrementally with two additional 
commits since the last revision:

 - Change Array::data() implementation
 - Avoid stringop-overflow warning in jfrTraceIdBits.inline.hpp

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/8646/files
  - new: https://git.openjdk.java.net/jdk/pull/8646/files/042c1c70..17cda224

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=8646=08
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8646=07-08

  Stats: 39 lines in 4 files changed: 18 ins; 18 del; 3 mod
  Patch: https://git.openjdk.java.net/jdk/pull/8646.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/8646/head:pull/8646

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

Re: RFR: 8286562: GCC 12 reports some compiler warnings [v5]

[Yasumasa Suenaga]

On Sun, 22 May 2022 05:00:21 GMT, Kim Barrett  wrote:

>> I guess GCC cannot understand `assert(dest != NULL` immediately preceding it.
>> 
>> 
>> In file included from 
>> /home/ysuenaga/github-forked/jdk/src/hotspot/share/jfr/recorder/checkpoint/types/traceid/jfrTraceIdLoadBarrier.inline.hpp:33,
>>  from 
>> /home/ysuenaga/github-forked/jdk/src/hotspot/share/jfr/recorder/checkpoint/types/traceid/jfrTraceId.inline.hpp:30,
>>  from 
>> /home/ysuenaga/github-forked/jdk/src/hotspot/share/jfr/support/jfrJdkJfrEvent.cpp:30:
>> In function 'void set_form(jbyte, jbyte*) [with jbyte (* op)(jbyte, jbyte) = 
>> traceid_or]',
>> inlined from 'void set(jbyte, jbyte*)' at 
>> /home/ysuenaga/github-forked/jdk/src/hotspot/share/jfr/recorder/checkpoint/types/traceid/jfrTraceIdBits.inline.hpp:129:23,
>> inlined from 'static void JfrTraceIdBits::store(jbyte, const T*) [with T 
>> = Klass]' at 
>> /home/ysuenaga/github-forked/jdk/src/hotspot/share/jfr/recorder/checkpoint/types/traceid/jfrTraceIdBits.inline.hpp:135:6,
>> inlined from 'static void JfrTraceId::tag_as_jdk_jfr_event(const 
>> Klass*)' at 
>> /home/ysuenaga/github-forked/jdk/src/hotspot/share/jfr/recorder/checkpoint/types/traceid/jfrTraceId.inline.hpp:106:3,
>> inlined from 'static void JdkJfrEvent::tag_as(const Klass*)' at 
>> /home/ysuenaga/github-forked/jdk/src/hotspot/share/jfr/support/jfrJdkJfrEvent.cpp:176:35:
>
> I don't think this warning has anything to do with that NULL check. But I'm
> still not understanding what it is warning about. The "region of size 0" part
> of the warning message seems important, but I'm not (yet?) seeing how it could
> be coming up with that.  The code involved here is pretty contorted...

It might be GCC bug...

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99578

This issue is for integer literal, but [Comment 
29](https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99578#c29) mentions address 
calculation (e.g. `NULL + offset`) - it is similar the problem in 
jfrTraceIdBits.inline.hp because `dest` comes from `low_addr()` (`addr + 
low_offset`).

-

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

Re: RFR: 8286562: GCC 12 reports some compiler warnings [v5]

[Yasumasa Suenaga]

On Sun, 22 May 2022 03:15:20 GMT, Kim Barrett  wrote:

>> Like the others, it is caused by `Array::at_put()`.
>> 
>> 
>> In file included from 
>> /home/ysuenaga/github-forked/jdk/src/hotspot/share/oops/annotations.hpp:28,
>>  from 
>> /home/ysuenaga/github-forked/jdk/src/hotspot/share/oops/instanceKlass.hpp:29,
>>  from 
>> /home/ysuenaga/github-forked/jdk/src/hotspot/share/classfile/javaClasses.hpp:30,
>>  from 
>> /home/ysuenaga/github-forked/jdk/src/hotspot/share/precompiled/precompiled.hpp:35:
>> In member function 'void Array::at_put(int, const T&) [with T = unsigned 
>> char]',
>> inlined from 'void ConstantPool::tag_at_put(int, jbyte)' at 
>> /home/ysuenaga/github-forked/jdk/src/hotspot/share/oops/constantPool.hpp:126:64,
>> inlined from 'void ConstantPool::symbol_at_put(int, Symbol*)' at 
>> /home/ysuenaga/github-forked/jdk/src/hotspot/share/oops/constantPool.hpp:362:15,
>> inlined from 'void 
>> ClassFileParser::mangle_hidden_class_name(InstanceKlass*)' at 
>> /home/ysuenaga/github-forked/jdk/src/hotspot/share/classfile/classFileParser.cpp:5966:21:
>
> `Array::_data` is a pseudo flexible array member. "Pseudo" because C++
> doesn't have flexible array members. The compiler is completely justified in
> complaining about the apparently out-of-bounds accesses.
> 
> There is a "well-known" (though moderately ugly) approach to doing flexible
> array members in C++. Something like this:
> 
> 
> T* data() {
>   return reinterpret_cast(
> reinterpret_cast(this) + data_offset());
> }
> 
> 
> where `data_offset()` is new and private:
> 
> 
> static size_t data_offset() {
>   return offset_of(Array, _data);
> }
> 
> 
> Use `data()` everywhere instead of using `_data` directly.
> 
> There are other places in HotSpot that use this kind of approach.

Thanks @kimbarrett for your advice! Warnings from array.hpp have gone with your 
suggestion.

-

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

Re: RFR: 8286562: GCC 12 reports some compiler warnings [v8]

[Yasumasa Suenaga]

> I saw some compiler warnings when I tried to build OpenJDK with GCC 12.0.1 on 
> Fedora 36.
> As you can see, the warnings spreads several areas. Let me know if I should 
> separate them by area.
> 
> * -Wstringop-overflow
> * src/hotspot/share/oops/array.hpp
> * 
> src/hotspot/share/jfr/recorder/checkpoint/types/traceid/jfrTraceIdBits.inline.hpp
> 
> In member function 'void Array::at_put(int, const T&) [with T = unsigned 
> char]',
> inlined from 'void ConstantPool::tag_at_put(int, jbyte)' at 
> /home/ysuenaga/github-forked/jdk/src/hotspot/share/oops/constantPool.hpp:126:64,
> inlined from 'void ConstantPool::method_at_put(int, int, int)' at 
> /home/ysuenaga/github-forked/jdk/src/hotspot/share/oops/constantPool.hpp:380:15,
> inlined from 'ConstantPool* 
> BytecodeConstantPool::create_constant_pool(JavaThread*) const' at 
> /home/ysuenaga/github-forked/jdk/src/hotspot/share/classfile/bytecodeAssembler.cpp:85:26:

Yasumasa Suenaga has updated the pull request with a new target base due to a 
merge or a rebase. The pull request now contains 11 commits:

 - Merge remote-tracking branch 'upstream/master' into gcc12-warnings
 - Use getter function to access "_data"
 - Revert changes for bytecodeAssembler.cpp, classFileParser.cpp, 
symbolTable.cpp
 - revert changes for memnode.cpp and type.cpp
 - Add assert to check the range of BasicType
 - Merge remote-tracking branch 'upstream/master' into HEAD
 - Revert change for java.c , parse_manifest.c , LinuxPackage.c
 - Calculate char offset before realloc()
 - Use return value from JLI_Snprintf
 - Avoid pragma error in before GCC 12
 - ... and 1 more: https://git.openjdk.java.net/jdk/compare/c156bcc5...042c1c70

-

Changes: https://git.openjdk.java.net/jdk/pull/8646/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk=8646=07
  Stats: 42 lines in 7 files changed: 26 ins; 1 del; 15 mod
  Patch: https://git.openjdk.java.net/jdk/pull/8646.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/8646/head:pull/8646

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

Re: RFR: 8286562: GCC 12 reports some compiler warnings [v7]

[Yasumasa Suenaga]

On Tue, 17 May 2022 13:32:42 GMT, Yasumasa Suenaga  wrote:

>> I saw some compiler warnings when I tried to build OpenJDK with GCC 12.0.1 
>> on Fedora 36.
>> As you can see, the warnings spreads several areas. Let me know if I should 
>> separate them by area.
>> 
>> * -Wstringop-overflow
>> * src/hotspot/share/oops/array.hpp
>> * 
>> src/hotspot/share/jfr/recorder/checkpoint/types/traceid/jfrTraceIdBits.inline.hpp
>> 
>> In member function 'void Array::at_put(int, const T&) [with T = unsigned 
>> char]',
>> inlined from 'void ConstantPool::tag_at_put(int, jbyte)' at 
>> /home/ysuenaga/github-forked/jdk/src/hotspot/share/oops/constantPool.hpp:126:64,
>> inlined from 'void ConstantPool::method_at_put(int, int, int)' at 
>> /home/ysuenaga/github-forked/jdk/src/hotspot/share/oops/constantPool.hpp:380:15,
>> inlined from 'ConstantPool* 
>> BytecodeConstantPool::create_constant_pool(JavaThread*) const' at 
>> /home/ysuenaga/github-forked/jdk/src/hotspot/share/classfile/bytecodeAssembler.cpp:85:26:
>
> Yasumasa Suenaga has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   revert changes for memnode.cpp and type.cpp

In case of stringop-overflow errors (bytecodeAssembler.cpp, 
classFileParser.cpp, symbolTable.cpp) noted that offset of `Array::_data` 
might be  [-2147483648, -1].

-

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

Integrated: 8286694: Incorrect argument processing in java launcher

[Yasumasa Suenaga]

On Fri, 13 May 2022 08:56:59 GMT, Yasumasa Suenaga  wrote:

> GCC 12 reports as following:

This pull request has now been integrated.

Changeset: 26c7c92b
Author:    Yasumasa Suenaga 
URL:   
https://git.openjdk.java.net/jdk/commit/26c7c92bc93f3eecf7ce69c69f1999ba879d1d60
Stats: 5 lines in 1 file changed: 3 ins; 0 del; 2 mod

8286694: Incorrect argument processing in java launcher

Reviewed-by: dholmes

-

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

Re: RFR: 8286694: Incorrect argument processing in java launcher

[Yasumasa Suenaga]

On Tue, 17 May 2022 04:39:06 GMT, David Holmes  wrote:

>> GCC 12 reports as following:
>
> You forced me to look at this in depth. The values are set via the build 
> system. In the build system it is impossible to get just -J because the -J is 
> only added as a prefix for an existing string. So basically this is 
> impossible code to reach unless you manually override the build system 
> definition. So as far as I can see this is a classic case where we want an 
> assert.
> 
> 
> if (arg[0] == '-' && arg[1] == 'J') {
> assert(arg[2] != '\0', "Invalid JAVA_ARGS or EXTRA_JAVA_ARGS defined by 
> build");
> *nargv++ = JLI_StringDup(arg + 2);
> }

@dholmes-ora Thanks for your comment!

We cannot use `assert(cond, message)` at here because it is not HotSpot code. 
In imageFile.cpp for libjimage. It uses `assert(cond && message)`, so I use 
this style in new commit, and the warning has gone.

I can change to "normal" `assert` usage at here if it is strange.

-

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

Re: RFR: 8286694: Incorrect argument processing in java launcher [v2]

[Yasumasa Suenaga]

> GCC 12 reports as following:

Yasumasa Suenaga 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 three additional 
commits since the last revision:

 - Use assert() to check jargv
 - Merge remote-tracking branch 'upstream/master' into JDK-8286694
 - 8286694: Incorrect argument processing in java launcher

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/8694/files
  - new: https://git.openjdk.java.net/jdk/pull/8694/files/fbde50b2..339dc135

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

  Stats: 6334 lines in 527 files changed: 3166 ins; 1506 del; 1662 mod
  Patch: https://git.openjdk.java.net/jdk/pull/8694.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/8694/head:pull/8694

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

Re: RFR: 8286562: GCC 12 reports some compiler warnings [v5]

[Yasumasa Suenaga]

On Tue, 17 May 2022 04:52:44 GMT, Kim Barrett  wrote:

>> src/hotspot/share/opto/memnode.cpp line 1413:
>> 
>>> 1411:bt == T_BYTE|| bt == T_SHORT ||
>>> 1412:bt == T_INT || bt == T_LONG, "wrong type = 
>>> %s", type2name(bt));
>>> 1413: PRAGMA_DIAG_POP
>> 
>> The problem here is the definition of type2name, which returns NULL for an 
>> unknown BasicType.  It would probably be better if it returned a 
>> recognizable string for that situation, eliminating this warning at it's 
>> source.
>
> While looking at type2name, I noticed the comment for the immediately 
> preceding type2name_tab is wrong.

The warning has gone in new commit, and I fixed the comment for `type2name_tab` 
in it.

-

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

Re: RFR: 8286562: GCC 12 reports some compiler warnings [v5]

[Yasumasa Suenaga]

On Tue, 17 May 2022 03:06:49 GMT, Kim Barrett  wrote:

>> Yasumasa Suenaga has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   Revert change for java.c , parse_manifest.c , LinuxPackage.c
>
> src/hotspot/share/opto/type.cpp line 4300:
> 
>> 4298: PRAGMA_FORMAT_OVERFLOW_IGNORED
>> 4299:   fatal("not an element type: %s", type2name(etype));
>> 4300: PRAGMA_DIAG_POP
> 
> Another occurrence of type2name returning NULL for unknown BasicType.

The warning has gone in new commit due to change of `type2name`.

-

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

Re: RFR: 8286562: GCC 12 reports some compiler warnings [v7]

[Yasumasa Suenaga]

> I saw some compiler warnings when I tried to build OpenJDK with GCC 12.0.1 on 
> Fedora 36.
> As you can see, the warnings spreads several areas. Let me know if I should 
> separate them by area.
> 
> * -Wstringop-overflow
> * src/hotspot/share/oops/array.hpp
> * 
> src/hotspot/share/jfr/recorder/checkpoint/types/traceid/jfrTraceIdBits.inline.hpp
> 
> In member function 'void Array::at_put(int, const T&) [with T = unsigned 
> char]',
> inlined from 'void ConstantPool::tag_at_put(int, jbyte)' at 
> /home/ysuenaga/github-forked/jdk/src/hotspot/share/oops/constantPool.hpp:126:64,
> inlined from 'void ConstantPool::method_at_put(int, int, int)' at 
> /home/ysuenaga/github-forked/jdk/src/hotspot/share/oops/constantPool.hpp:380:15,
> inlined from 'ConstantPool* 
> BytecodeConstantPool::create_constant_pool(JavaThread*) const' at 
> /home/ysuenaga/github-forked/jdk/src/hotspot/share/classfile/bytecodeAssembler.cpp:85:26:

Yasumasa Suenaga has updated the pull request incrementally with one additional 
commit since the last revision:

  revert changes for memnode.cpp and type.cpp

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/8646/files
  - new: https://git.openjdk.java.net/jdk/pull/8646/files/73c306d7..88cbf46d

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

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

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

Re: RFR: 8286562: GCC 12 reports some compiler warnings [v5]

[Yasumasa Suenaga]

On Tue, 17 May 2022 03:14:05 GMT, Kim Barrett  wrote:

>> Yasumasa Suenaga has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   Revert change for java.c , parse_manifest.c , LinuxPackage.c
>
> src/hotspot/share/classfile/bytecodeAssembler.cpp line 95:
> 
>> 93: ShouldNotReachHere();
>> 94: }
>> 95: PRAGMA_DIAG_POP
> 
> One of these is another of the symbol_at_put cases that I don't understand.  
> Are there other cases in the switch that warn?  And if so, which ones and 
> how?  There are no details of this one in the PR cover description.

Most of switch cases are warned. Please see 
[logs](https://github.com/openjdk/jdk/files/8708578/hotspot_variant-server_libjvm_objs_bytecodeAssembler.o.log)

-

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

Re: RFR: 8286562: GCC 12 reports some compiler warnings [v6]

[Yasumasa Suenaga]

> I saw some compiler warnings when I tried to build OpenJDK with GCC 12.0.1 on 
> Fedora 36.
> As you can see, the warnings spreads several areas. Let me know if I should 
> separate them by area.
> 
> * -Wstringop-overflow
> * src/hotspot/share/oops/array.hpp
> * 
> src/hotspot/share/jfr/recorder/checkpoint/types/traceid/jfrTraceIdBits.inline.hpp
> 
> In member function 'void Array::at_put(int, const T&) [with T = unsigned 
> char]',
> inlined from 'void ConstantPool::tag_at_put(int, jbyte)' at 
> /home/ysuenaga/github-forked/jdk/src/hotspot/share/oops/constantPool.hpp:126:64,
> inlined from 'void ConstantPool::method_at_put(int, int, int)' at 
> /home/ysuenaga/github-forked/jdk/src/hotspot/share/oops/constantPool.hpp:380:15,
> inlined from 'ConstantPool* 
> BytecodeConstantPool::create_constant_pool(JavaThread*) const' at 
> /home/ysuenaga/github-forked/jdk/src/hotspot/share/classfile/bytecodeAssembler.cpp:85:26:

Yasumasa Suenaga has updated the pull request with a new target base due to a 
merge or a rebase. The pull request now contains seven commits:

 - Add assert to check the range of BasicType
 - Merge remote-tracking branch 'upstream/master' into HEAD
 - Revert change for java.c , parse_manifest.c , LinuxPackage.c
 - Calculate char offset before realloc()
 - Use return value from JLI_Snprintf
 - Avoid pragma error in before GCC 12
 - 8286562: GCC 12 reports some compiler warnings

-

Changes: https://git.openjdk.java.net/jdk/pull/8646/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk=8646=05
  Stats: 56 lines in 11 files changed: 46 ins; 1 del; 9 mod
  Patch: https://git.openjdk.java.net/jdk/pull/8646.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/8646/head:pull/8646

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

Re: RFR: 8286562: GCC 12 reports some compiler warnings [v5]

[Yasumasa Suenaga]

On Tue, 17 May 2022 03:02:55 GMT, Kim Barrett  wrote:

>> Yasumasa Suenaga has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   Revert change for java.c , parse_manifest.c , LinuxPackage.c
>
> src/hotspot/share/jfr/recorder/checkpoint/types/traceid/jfrTraceIdBits.inline.hpp
>  line 103:
> 
>> 101: PRAGMA_STRINGOP_OVERFLOW_IGNORED
>> 102:   *dest = op(bits, *dest);
>> 103: PRAGMA_DIAG_POP
> 
> I see no stringop here.  I'm still trying to understand what the compiler is 
> complaining about.

I guess GCC cannot understand `assert(dest != NULL` immediately preceding it.


In file included from 
/home/ysuenaga/github-forked/jdk/src/hotspot/share/jfr/recorder/checkpoint/types/traceid/jfrTraceIdLoadBarrier.inline.hpp:33,
 from 
/home/ysuenaga/github-forked/jdk/src/hotspot/share/jfr/recorder/checkpoint/types/traceid/jfrTraceId.inline.hpp:30,
 from 
/home/ysuenaga/github-forked/jdk/src/hotspot/share/jfr/support/jfrJdkJfrEvent.cpp:30:
In function 'void set_form(jbyte, jbyte*) [with jbyte (* op)(jbyte, jbyte) = 
traceid_or]',
inlined from 'void set(jbyte, jbyte*)' at 
/home/ysuenaga/github-forked/jdk/src/hotspot/share/jfr/recorder/checkpoint/types/traceid/jfrTraceIdBits.inline.hpp:129:23,
inlined from 'static void JfrTraceIdBits::store(jbyte, const T*) [with T = 
Klass]' at 
/home/ysuenaga/github-forked/jdk/src/hotspot/share/jfr/recorder/checkpoint/types/traceid/jfrTraceIdBits.inline.hpp:135:6,
inlined from 'static void JfrTraceId::tag_as_jdk_jfr_event(const Klass*)' 
at 
/home/ysuenaga/github-forked/jdk/src/hotspot/share/jfr/recorder/checkpoint/types/traceid/jfrTraceId.inline.hpp:106:3,
inlined from 'static void JdkJfrEvent::tag_as(const Klass*)' at 
/home/ysuenaga/github-forked/jdk/src/hotspot/share/jfr/support/jfrJdkJfrEvent.cpp:176:35:

-

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

Re: RFR: 8286562: GCC 12 reports some compiler warnings [v5]

[Yasumasa Suenaga]

On Tue, 17 May 2022 01:43:25 GMT, Kim Barrett  wrote:

>> Yasumasa Suenaga has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   Revert change for java.c , parse_manifest.c , LinuxPackage.c
>
> src/hotspot/share/classfile/classFileParser.cpp line 5970:
> 
>> 5968: PRAGMA_STRINGOP_OVERFLOW_IGNORED
>> 5969:   _cp->symbol_at_put(hidden_index, _class_name);
>> 5970: PRAGMA_DIAG_POP
> 
> I don't understand these warning suppressions for symbol_at_put (here and 
> elsewhere).  I don't see any stringops here.  What is the compiler 
> complaining about?  (There's no mention of classfile stuff in the review 
> cover message.)

Like the others, it is caused by `Array::at_put()`.


In file included from 
/home/ysuenaga/github-forked/jdk/src/hotspot/share/oops/annotations.hpp:28,
 from 
/home/ysuenaga/github-forked/jdk/src/hotspot/share/oops/instanceKlass.hpp:29,
 from 
/home/ysuenaga/github-forked/jdk/src/hotspot/share/classfile/javaClasses.hpp:30,
 from 
/home/ysuenaga/github-forked/jdk/src/hotspot/share/precompiled/precompiled.hpp:35:
In member function 'void Array::at_put(int, const T&) [with T = unsigned 
char]',
inlined from 'void ConstantPool::tag_at_put(int, jbyte)' at 
/home/ysuenaga/github-forked/jdk/src/hotspot/share/oops/constantPool.hpp:126:64,
inlined from 'void ConstantPool::symbol_at_put(int, Symbol*)' at 
/home/ysuenaga/github-forked/jdk/src/hotspot/share/oops/constantPool.hpp:362:15,
inlined from 'void 
ClassFileParser::mangle_hidden_class_name(InstanceKlass*)' at 
/home/ysuenaga/github-forked/jdk/src/hotspot/share/classfile/classFileParser.cpp:5966:21:

-

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

Re: RFR: 8286562: GCC 12 reports some compiler warnings [v4]

[Yasumasa Suenaga]

On Thu, 12 May 2022 11:02:02 GMT, Magnus Ihse Bursie  wrote:

>> Thanks for all to review this PR! I think we should separate this issue as 
>> following:
>> 
>> * Suppress warnings
>> * make/modules/java.desktop/lib/Awt2dLibraries.gmk
>> * src/hotspot/share/classfile/bytecodeAssembler.cpp
>> * src/hotspot/share/classfile/classFileParser.cpp
>> * 
>> src/hotspot/share/jfr/recorder/checkpoint/types/traceid/jfrTraceIdBits.inline.hpp
>> * src/hotspot/share/opto/memnode.cpp
>> * src/hotspot/share/opto/type.cpp
>> * src/hotspot/share/utilities/compilerWarnings.hpp
>> * src/hotspot/share/utilities/compilerWarnings_gcc.hpp
>> * src/java.base/unix/native/libjli/java_md_common.c
>> * Bug fixes
>> * src/java.base/share/native/libjli/java.c
>> * src/java.base/share/native/libjli/parse_manifest.c
>> * src/jdk.jpackage/linux/native/applauncher/LinuxPackage.c
>> 
>> I want to include the change of Awt2dLibraries.gmk (HarfBuzz) in this PR 
>> because it is 3rd party library.
>> 
>> I will separate in above if I do not hear any objections, and this issue 
>> (PR) handles "suppress warnings" only.
>
> @YaSuenag From my PoV this sounds like a good suggestion.

@magicus @prrace Thanks for your review!

Can I get the review from HotSpot folks? @kimbarrett

-

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

Re: RFR: 8286694: Incorrect argument processing in java launcher

[Yasumasa Suenaga]

On Mon, 16 May 2022 13:20:49 GMT, David Holmes  wrote:

>> GCC 12 reports as following:
>
> I don't recall what "builtin arguments" means. Can you tell me how we would 
> actually hit this conditions such that we string dup and  the null string? Or 
> are you saying this is actually an unreachable case: the check for NULL is 
> always false and the actual  string is never null anyway?

@dholmes-ora 
As I wrote in the description, `jargv` comes from `JAVA_ARGS` or 
`EXTRA_JAVA_ARGS` - end user cannot modify them. So I think they should be 
reported as developer error.
OTOH JDK developer cannot know invalid options if they do not set trace mode to 
launcher. So I want to discuss how we should handle invalid parameter(s) in 
`jargv`.

-

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

Re: RFR: 8286694: Incorrect argument processing in java launcher

[Yasumasa Suenaga]

On Mon, 16 May 2022 06:58:22 GMT, David Holmes  wrote:

> Simply considering the compiler warning shouldn't `(arg+2) == NULL` actually 
> be `arg[2] == '\0'` ? as we are looking to see if this arg is only three 
> characters: '-', 'J' and '\0'

Yeah, I thought that first, but I think better of keeping similar code because 
the result (element of `nargv`) will be used as loop condition (see description 
of this PR).

In addition, subsequent codes consider this case (`-J` only)  to be an error as 
following:


1633 for (i = 0; i < argc; i++) {
1634 char *arg = argv[i];
1635 if (arg[0] == '-' && arg[1] == 'J') {
1636 if (arg[2] == '\0') {
1637 JLI_ReportErrorMessage(ARG_ERROR3);
1638 exit(1);
1639 }
1640 *nargv++ = arg + 2;
1641 }
1642 }


However `jargv` is builtin arguments, not user specified arguments as I wrote 
the description of this PR. So I think we can ignore it, but it would be nice 
if we can see it in trace logs.

-

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

Integrated: 8286705: GCC 12 reports use-after-free potential bugs

[Yasumasa Suenaga]

On Fri, 13 May 2022 09:14:28 GMT, Yasumasa Suenaga  wrote:

> GCC 12 reports use-after-free potential bugs in below:
> 
> 
> In function 'find_positions',
> inlined from 'find_file' at 
> /home/ysuenaga/github-forked/jdk/src/java.base/share/native/libjli/parse_manifest.c:364:9:

This pull request has now been integrated.

Changeset: 0e4bece5
Author:    Yasumasa Suenaga 
URL:   
https://git.openjdk.java.net/jdk/commit/0e4bece5b5143b8505496ea7430bbfa11e151aff
Stats: 8 lines in 2 files changed: 4 ins; 1 del; 3 mod

8286705: GCC 12 reports use-after-free potential bugs

Reviewed-by: kbarrett

-

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

Re: RFR: 8286562: GCC 12 reports some compiler warnings [v5]

[Yasumasa Suenaga]

On Fri, 13 May 2022 10:02:30 GMT, Yasumasa Suenaga  wrote:

>> I saw some compiler warnings when I tried to build OpenJDK with GCC 12.0.1 
>> on Fedora 36.
>> As you can see, the warnings spreads several areas. Let me know if I should 
>> separate them by area.
>> 
>> * -Wstringop-overflow
>> * src/hotspot/share/oops/array.hpp
>> * 
>> src/hotspot/share/jfr/recorder/checkpoint/types/traceid/jfrTraceIdBits.inline.hpp
>> 
>> In member function 'void Array::at_put(int, const T&) [with T = unsigned 
>> char]',
>> inlined from 'void ConstantPool::tag_at_put(int, jbyte)' at 
>> /home/ysuenaga/github-forked/jdk/src/hotspot/share/oops/constantPool.hpp:126:64,
>> inlined from 'void ConstantPool::method_at_put(int, int, int)' at 
>> /home/ysuenaga/github-forked/jdk/src/hotspot/share/oops/constantPool.hpp:380:15,
>> inlined from 'ConstantPool* 
>> BytecodeConstantPool::create_constant_pool(JavaThread*) const' at 
>> /home/ysuenaga/github-forked/jdk/src/hotspot/share/classfile/bytecodeAssembler.cpp:85:26:
>
> Yasumasa Suenaga has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Revert change for java.c , parse_manifest.c , LinuxPackage.c

I've sent another review request for bug fixes as #8694 and #8696 , and I 
reverted change for them from this PR.
Could you review this PR to suppress warnings which we can ignore?

-

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

RFR: 8286694: Incorrect argument processing in java launcher

[Yasumasa Suenaga]

GCC 12 reports as following:

-

Commit messages:
 - 8286694: Incorrect argument processing in java launcher

Changes: https://git.openjdk.java.net/jdk/pull/8694/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk=8694=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8286694
  Stats: 8 lines in 1 file changed: 6 ins; 0 del; 2 mod
  Patch: https://git.openjdk.java.net/jdk/pull/8694.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/8694/head:pull/8694

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

RFR: 8286705: GCC 12 reports use-after-free potential bugs

[Yasumasa Suenaga]

GCC 12 reports use-after-free potential bugs in below:


In function 'find_positions',
inlined from 'find_file' at 
/home/ysuenaga/github-forked/jdk/src/java.base/share/native/libjli/parse_manifest.c:364:9:

-

Commit messages:
 - 8286705: GCC 12 reports use-after-free potential bugs

Changes: https://git.openjdk.java.net/jdk/pull/8696/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk=8696=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8286705
  Stats: 8 lines in 2 files changed: 4 ins; 1 del; 3 mod
  Patch: https://git.openjdk.java.net/jdk/pull/8696.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/8696/head:pull/8696

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

Re: RFR: 8286562: GCC 12 reports some compiler warnings [v5]

[Yasumasa Suenaga]

> I saw some compiler warnings when I tried to build OpenJDK with GCC 12.0.1 on 
> Fedora 36.
> As you can see, the warnings spreads several areas. Let me know if I should 
> separate them by area.
> 
> * -Wstringop-overflow
> * src/hotspot/share/oops/array.hpp
> * 
> src/hotspot/share/jfr/recorder/checkpoint/types/traceid/jfrTraceIdBits.inline.hpp
> 
> In member function 'void Array::at_put(int, const T&) [with T = unsigned 
> char]',
> inlined from 'void ConstantPool::tag_at_put(int, jbyte)' at 
> /home/ysuenaga/github-forked/jdk/src/hotspot/share/oops/constantPool.hpp:126:64,
> inlined from 'void ConstantPool::method_at_put(int, int, int)' at 
> /home/ysuenaga/github-forked/jdk/src/hotspot/share/oops/constantPool.hpp:380:15,
> inlined from 'ConstantPool* 
> BytecodeConstantPool::create_constant_pool(JavaThread*) const' at 
> /home/ysuenaga/github-forked/jdk/src/hotspot/share/classfile/bytecodeAssembler.cpp:85:26:

Yasumasa Suenaga has updated the pull request incrementally with one additional 
commit since the last revision:

  Revert change for java.c , parse_manifest.c , LinuxPackage.c

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/8646/files
  - new: https://git.openjdk.java.net/jdk/pull/8646/files/b3afa3e0..d5ef2c63

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

  Stats: 10 lines in 3 files changed: 1 ins; 4 del; 5 mod
  Patch: https://git.openjdk.java.net/jdk/pull/8646.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/8646/head:pull/8646

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

Re: RFR: 8286562: GCC 12 reports some compiler warnings [v4]

[Yasumasa Suenaga]

On Thu, 12 May 2022 01:27:30 GMT, Yasumasa Suenaga  wrote:

>> I saw some compiler warnings when I tried to build OpenJDK with GCC 12.0.1 
>> on Fedora 36.
>> As you can see, the warnings spreads several areas. Let me know if I should 
>> separate them by area.
>> 
>> * -Wstringop-overflow
>> * src/hotspot/share/oops/array.hpp
>> * 
>> src/hotspot/share/jfr/recorder/checkpoint/types/traceid/jfrTraceIdBits.inline.hpp
>> 
>> In member function 'void Array::at_put(int, const T&) [with T = unsigned 
>> char]',
>> inlined from 'void ConstantPool::tag_at_put(int, jbyte)' at 
>> /home/ysuenaga/github-forked/jdk/src/hotspot/share/oops/constantPool.hpp:126:64,
>> inlined from 'void ConstantPool::method_at_put(int, int, int)' at 
>> /home/ysuenaga/github-forked/jdk/src/hotspot/share/oops/constantPool.hpp:380:15,
>> inlined from 'ConstantPool* 
>> BytecodeConstantPool::create_constant_pool(JavaThread*) const' at 
>> /home/ysuenaga/github-forked/jdk/src/hotspot/share/classfile/bytecodeAssembler.cpp:85:26:
>
> Yasumasa Suenaga has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Calculate char offset before realloc()

Thanks for all to review this PR! I think we should separate this issue as 
following:

* Suppress warnings
* make/modules/java.desktop/lib/Awt2dLibraries.gmk
* src/hotspot/share/classfile/bytecodeAssembler.cpp
* src/hotspot/share/classfile/classFileParser.cpp
* 
src/hotspot/share/jfr/recorder/checkpoint/types/traceid/jfrTraceIdBits.inline.hpp
* src/hotspot/share/opto/memnode.cpp
* src/hotspot/share/opto/type.cpp
* src/hotspot/share/utilities/compilerWarnings.hpp
* src/hotspot/share/utilities/compilerWarnings_gcc.hpp
* src/java.base/unix/native/libjli/java_md_common.c
* Bug fixes
* src/java.base/share/native/libjli/java.c
* src/java.base/share/native/libjli/parse_manifest.c
* src/jdk.jpackage/linux/native/applauncher/LinuxPackage.c

I want to include the change of Awt2dLibraries.gmk (HarfBuzz) in this PR 
because it is 3rd party library.

I will separate in above if I do not hear any objections, and this issue (PR) 
handles "suppress warnings" only.

-

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

Re: RFR: 8286562: GCC 12 reports some compiler warnings [v2]

[Yasumasa Suenaga]

On Wed, 11 May 2022 13:47:43 GMT, Kim Barrett  wrote:

>> Yasumasa Suenaga has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   Avoid pragma error in before GCC 12
>
> src/jdk.jpackage/linux/native/applauncher/LinuxPackage.c line 193:
> 
>> 191: #if defined(__GNUC__) && __GNUC__ >= 12
>> 192: #pragma GCC diagnostic pop
>> 193: #endif
> 
> Rather than all this warning suppression cruft and the comment explaining why
> it's okay, just compute the `(strBufNextChar - strBufBegin)` offset a few
> lines earlier, before the realloc.

I did do that in new commit, and the warning has gone!

-

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

Re: RFR: 8286562: GCC 12 reports some compiler warnings [v4]

[Yasumasa Suenaga]

> I saw some compiler warnings when I tried to build OpenJDK with GCC 12.0.1 on 
> Fedora 36.
> As you can see, the warnings spreads several areas. Let me know if I should 
> separate them by area.
> 
> * -Wstringop-overflow
> * src/hotspot/share/oops/array.hpp
> * 
> src/hotspot/share/jfr/recorder/checkpoint/types/traceid/jfrTraceIdBits.inline.hpp
> 
> In member function 'void Array::at_put(int, const T&) [with T = unsigned 
> char]',
> inlined from 'void ConstantPool::tag_at_put(int, jbyte)' at 
> /home/ysuenaga/github-forked/jdk/src/hotspot/share/oops/constantPool.hpp:126:64,
> inlined from 'void ConstantPool::method_at_put(int, int, int)' at 
> /home/ysuenaga/github-forked/jdk/src/hotspot/share/oops/constantPool.hpp:380:15,
> inlined from 'ConstantPool* 
> BytecodeConstantPool::create_constant_pool(JavaThread*) const' at 
> /home/ysuenaga/github-forked/jdk/src/hotspot/share/classfile/bytecodeAssembler.cpp:85:26:

Yasumasa Suenaga has updated the pull request incrementally with one additional 
commit since the last revision:

  Calculate char offset before realloc()

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/8646/files
  - new: https://git.openjdk.java.net/jdk/pull/8646/files/8d608414..b3afa3e0

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

  Stats: 18 lines in 1 file changed: 3 ins; 14 del; 1 mod
  Patch: https://git.openjdk.java.net/jdk/pull/8646.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/8646/head:pull/8646

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

Re: RFR: 8286562: GCC 12 reports some compiler warnings [v2]

[Yasumasa Suenaga]

On Wed, 11 May 2022 13:43:55 GMT, Kim Barrett  wrote:

>> Yasumasa Suenaga has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   Avoid pragma error in before GCC 12
>
> src/java.base/unix/native/libjli/java_md_common.c line 135:
> 
>> 133: if ((JLI_StrLen(indir) + JLI_StrLen(cmd) + 2) > sizeof(name)) 
>> return 0;
>> 134: JLI_Snprintf(name, sizeof(name), "%s%c%s", indir, FILE_SEPARATOR, 
>> cmd);
>> 135: #pragma GCC diagnostic pop
> 
> Wouldn't it be better to just call JLI_Snprintf without the precheck and 
> check the result to determine whether it was successful or was truncated?  I 
> think that kind of check is supposed to make gcc's truncation checker happy.

The warning has gone when using return value from `JLI_Snprintf()` in new 
commit!

-

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

Re: RFR: 8286562: GCC 12 reports some compiler warnings [v3]

[Yasumasa Suenaga]

> I saw some compiler warnings when I tried to build OpenJDK with GCC 12.0.1 on 
> Fedora 36.
> As you can see, the warnings spreads several areas. Let me know if I should 
> separate them by area.
> 
> * -Wstringop-overflow
> * src/hotspot/share/oops/array.hpp
> * 
> src/hotspot/share/jfr/recorder/checkpoint/types/traceid/jfrTraceIdBits.inline.hpp
> 
> In member function 'void Array::at_put(int, const T&) [with T = unsigned 
> char]',
> inlined from 'void ConstantPool::tag_at_put(int, jbyte)' at 
> /home/ysuenaga/github-forked/jdk/src/hotspot/share/oops/constantPool.hpp:126:64,
> inlined from 'void ConstantPool::method_at_put(int, int, int)' at 
> /home/ysuenaga/github-forked/jdk/src/hotspot/share/oops/constantPool.hpp:380:15,
> inlined from 'ConstantPool* 
> BytecodeConstantPool::create_constant_pool(JavaThread*) const' at 
> /home/ysuenaga/github-forked/jdk/src/hotspot/share/classfile/bytecodeAssembler.cpp:85:26:

Yasumasa Suenaga has updated the pull request incrementally with one additional 
commit since the last revision:

  Use return value from JLI_Snprintf

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/8646/files
  - new: https://git.openjdk.java.net/jdk/pull/8646/files/7f155256..8d608414

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

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

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

Re: RFR: 8286562: GCC 12 reports some compiler warnings [v2]

[Yasumasa Suenaga]

On Wed, 11 May 2022 14:27:27 GMT, Kim Barrett  wrote:

>> src/java.base/share/native/libjli/java.c line 1629:
>> 
>>> 1627: const char *arg = jargv[i];
>>> 1628: if (arg[0] == '-' && arg[1] == 'J') {
>>> 1629: *nargv++ = (arg[2] == '\0') ? NULL : JLI_StringDup(arg + 
>>> 2);
>> 
>> Wow!
>
> I wonder if the client expects NULL strings in the result, or if the NULL 
> value should be an empty string?  If empty strings are okay, this would be 
> simpler without the conditional, just dup from arg + 2 to the terminating 
> byte (which might be immediate).

`NULL` affects as a loop stopper in `ParseArguments()` as following:


static jboolean
ParseArguments(int *pargc, char ***pargv,
   int *pmode, char **pwhat,
   int *pret, const char *jrepath)
{
int argc = *pargc;
char **argv = *pargv;
int mode = LM_UNKNOWN;
char *arg;

*pret = 0;

while ((arg = *argv) != 0 && *arg == '-') {


But I'm not sure it is valid, I think it might be discussed as another issue.

-

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

Re: RFR: 8286562: GCC 12 reports some compiler warnings [v2]

[Yasumasa Suenaga]

On Wed, 11 May 2022 13:35:43 GMT, Kim Barrett  wrote:

>> Yasumasa Suenaga has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   Avoid pragma error in before GCC 12
>
> src/hotspot/share/utilities/compilerWarnings_gcc.hpp line 51:
> 
>> 49: 
>> 50: #define PRAGMA_STRINGOP_OVERFLOW_IGNORED \
>> 51:   PRAGMA_DISABLE_GCC_WARNING("-Wstringop-overflow")
> 
> Are the reported problems with format/stringop-overflow real? Or are they
> false positives? If they are real then I'm not going to approve ignoring them,
> unless there is some urgent reason and there are followup bug reports for
> fixing them.

I think all of warnings in HotSpot are false-positives, so I added new pragmas 
to compilerWarnings.hpp, and use it in HotSpot code.

-

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

Re: RFR: 8286562: GCC 12 reports some compiler warnings [v2]

[Yasumasa Suenaga]

On Wed, 11 May 2022 19:11:16 GMT, Phil Race  wrote:

>> make/modules/java.desktop/lib/Awt2dLibraries.gmk line 462:
>> 
>>> 460:HARFBUZZ_DISABLED_WARNINGS_gcc := type-limits 
>>> missing-field-initializers strict-aliasing
>>> 461:HARFBUZZ_DISABLED_WARNINGS_CXX_gcc := reorder 
>>> delete-non-virtual-dtor strict-overflow \
>>> 462: maybe-uninitialized class-memaccess unused-result extra 
>>> use-after-free
>> 
>> Globally disabling use-after-free warnings for this package seems really
>> questionable. If these are problems in the code, just suppressing the warning
>> and leaving the problems to bite us seems like a bad idea.  And the problems
>> ought to be reported upstream to the HarfBuzz folks.
>
> I don't understand what the actual warning is getting at .. can anyone 
> explain it ?
> 
> FWIW the code is still the same in upstream harfbuzz
> https://github.com/harfbuzz/harfbuzz/blob/main/src/hb-font.cc

I've pasted a part of warning messages to description of this PR, all of 
messages are here:


In function 'void trampoline_reference(hb_trampoline_closure_t*)',
inlined from 'void hb_font_funcs_set_glyph_func(hb_font_funcs_t*, 
hb_font_get_glyph_func_t, void*, hb_destroy_func_t)' at 
/home/ysuenaga/github-forked/jdk/src/java.desktop/share/native/libharfbuzz/hb-font.cc:2368:24:

-

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

Re: RFR: 8286562: GCC 12 reports some compiler warnings [v2]

[Yasumasa Suenaga]

On Wed, 11 May 2022 12:48:38 GMT, Alan Bateman  wrote:

>> Yasumasa Suenaga has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   Avoid pragma error in before GCC 12
>
> src/java.base/unix/native/libjli/java_md_common.c line 135:
> 
>> 133: if ((JLI_StrLen(indir) + JLI_StrLen(cmd) + 2) > sizeof(name)) 
>> return 0;
>> 134: JLI_Snprintf(name, sizeof(name), "%s%c%s", indir, FILE_SEPARATOR, 
>> cmd);
>> 135: #pragma GCC diagnostic pop
> 
> Can we just replace this code rather than putting pragmas here?

I tried several patterns, but I couldn't find out a solution other than 
pragmas. Do you have any ideas?

For example:

if ((JLI_StrLen(indir) + JLI_StrLen(cmd) + 2) < sizeof(name)) {
  JLI_Snprintf(name, sizeof(name), "%s%c%s", indir, FILE_SEPARATOR, cmd);
} else {
  return 0;
}


Compiler warnings:

-

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

Re: RFR: 8286562: GCC 12 reports some compiler warnings [v2]

[Yasumasa Suenaga]

On Wed, 11 May 2022 08:40:21 GMT, Yasumasa Suenaga  wrote:

>> I saw some compiler warnings when I tried to build OpenJDK with GCC 12.0.1 
>> on Fedora 36.
>> As you can see, the warnings spreads several areas. Let me know if I should 
>> separate them by area.
>> 
>> * -Wstringop-overflow
>> * src/hotspot/share/oops/array.hpp
>> * 
>> src/hotspot/share/jfr/recorder/checkpoint/types/traceid/jfrTraceIdBits.inline.hpp
>> 
>> In member function 'void Array::at_put(int, const T&) [with T = unsigned 
>> char]',
>> inlined from 'void ConstantPool::tag_at_put(int, jbyte)' at 
>> /home/ysuenaga/github-forked/jdk/src/hotspot/share/oops/constantPool.hpp:126:64,
>> inlined from 'void ConstantPool::method_at_put(int, int, int)' at 
>> /home/ysuenaga/github-forked/jdk/src/hotspot/share/oops/constantPool.hpp:380:15,
>> inlined from 'ConstantPool* 
>> BytecodeConstantPool::create_constant_pool(JavaThread*) const' at 
>> /home/ysuenaga/github-forked/jdk/src/hotspot/share/classfile/bytecodeAssembler.cpp:85:26:
>
> Yasumasa Suenaga has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Avoid pragma error in before GCC 12

It is better to add pragma to `Array::at_put()` in 
src/hotspot/share/oops/array.hpp , but I couldn't suppress warnings. So I added 
pragmas to its caller - bytecodeAssembler.cpp, classFileParser.cpp, 
symbolTable.cpp .

-

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

Re: RFR: 8286562: GCC 12 reports some compiler warnings [v2]

[Yasumasa Suenaga]

On Wed, 11 May 2022 11:48:00 GMT, Magnus Ihse Bursie  wrote:

>> Yasumasa Suenaga has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   Avoid pragma error in before GCC 12
>
> The harfbuzz disabled warning looks good, so build changes are approved. 
> You'll still need approval for the rest of the changes.
> 
> While it's not my place really to say about the code changes, I think hiding 
> the warnings with pragmas like this is the least attractive option. But if 
> the code owners are okay with it...

Thanks @magicus for your review!

> While it's not my place really to say about the code changes, I think hiding 
> the warnings with pragmas like this is the least attractive option. But if 
> the code owners are okay with it...

Agree, so I fixed bugs which were found out by compiler warnings in this PR - 
they are in libjli.
I think we can ignore the others because they are already checked in other 
methods (e.g. `assert`), or due to structure of `Array` class which has payload 
in `_data[1]` (and it is also checked in `assert`).

-

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

Re: RFR: 8286562: GCC 12 reports some compiler warnings [v2]

[Yasumasa Suenaga]

> I saw some compiler warnings when I tried to build OpenJDK with GCC 12.0.1 on 
> Fedora 36.
> As you can see, the warnings spreads several areas. Let me know if I should 
> separate them by area.
> 
> * -Wstringop-overflow
> * src/hotspot/share/oops/array.hpp
> * 
> src/hotspot/share/jfr/recorder/checkpoint/types/traceid/jfrTraceIdBits.inline.hpp
> 
> In member function 'void Array::at_put(int, const T&) [with T = unsigned 
> char]',
> inlined from 'void ConstantPool::tag_at_put(int, jbyte)' at 
> /home/ysuenaga/github-forked/jdk/src/hotspot/share/oops/constantPool.hpp:126:64,
> inlined from 'void ConstantPool::method_at_put(int, int, int)' at 
> /home/ysuenaga/github-forked/jdk/src/hotspot/share/oops/constantPool.hpp:380:15,
> inlined from 'ConstantPool* 
> BytecodeConstantPool::create_constant_pool(JavaThread*) const' at 
> /home/ysuenaga/github-forked/jdk/src/hotspot/share/classfile/bytecodeAssembler.cpp:85:26:

Yasumasa Suenaga has updated the pull request incrementally with one additional 
commit since the last revision:

  Avoid pragma error in before GCC 12

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/8646/files
  - new: https://git.openjdk.java.net/jdk/pull/8646/files/8154f6ea..7f155256

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

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

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

RFR: 8286562: GCC 12 reports some compiler warnings

[Yasumasa Suenaga]

I saw some compiler warnings when I tried to build OpenJDK with GCC 12.0.1 on 
Fedora 36.
As you can see, the warnings spreads several areas. Let me know if I should 
separate them by area.

* -Wstringop-overflow
* src/hotspot/share/oops/array.hpp
* 
src/hotspot/share/jfr/recorder/checkpoint/types/traceid/jfrTraceIdBits.inline.hpp

In member function 'void Array::at_put(int, const T&) [with T = unsigned 
char]',
inlined from 'void ConstantPool::tag_at_put(int, jbyte)' at 
/home/ysuenaga/github-forked/jdk/src/hotspot/share/oops/constantPool.hpp:126:64,
inlined from 'void ConstantPool::method_at_put(int, int, int)' at 
/home/ysuenaga/github-forked/jdk/src/hotspot/share/oops/constantPool.hpp:380:15,
inlined from 'ConstantPool* 
BytecodeConstantPool::create_constant_pool(JavaThread*) const' at 
/home/ysuenaga/github-forked/jdk/src/hotspot/share/classfile/bytecodeAssembler.cpp:85:26:

-

Commit messages:
 - 8286562: GCC 12 reports some compiler warnings

Changes: https://git.openjdk.java.net/jdk/pull/8646/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk=8646=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8286562
  Stats: 63 lines in 13 files changed: 51 ins; 1 del; 11 mod
  Patch: https://git.openjdk.java.net/jdk/pull/8646.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/8646/head:pull/8646

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

Integrated: 8266168: -Wmaybe-uninitialized happens in check_code.c

[Yasumasa Suenaga]

On Thu, 29 Apr 2021 06:54:33 GMT, Yasumasa Suenaga  wrote:

> We can see following compiler warning in check_code.c on GCC 11.

This pull request has now been integrated.

Changeset: 0f9852c6
Author:    Yasumasa Suenaga 
URL:   
https://git.openjdk.java.net/jdk/commit/0f9852c63b12c43b52615ea003a4fc1d69ad3ada
Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod

8266168: -Wmaybe-uninitialized happens in check_code.c

Reviewed-by: stuefe

-

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

Re: RFR: 8266168: -Wmaybe-uninitialized happens in check_code.c

[Yasumasa Suenaga]

On Thu, 29 Apr 2021 06:54:33 GMT, Yasumasa Suenaga  wrote:

> We can see following compiler warning in check_code.c on GCC 11.

Ping: Could you review this PR? We still see this warning with GCC 11.1.

-

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

RFR: 8266168: -Wmaybe-uninitialized happens in check_code.c

[Yasumasa Suenaga]

We can see following compiler warning in check_code.c on GCC 11.

-

Commit messages:
 - 8266168: -Wmaybe-uninitialized happens in check_code.c

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

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

Integrated: 8266173: -Wmaybe-uninitialized happens in jni_util.c

[Yasumasa Suenaga]

On Wed, 28 Apr 2021 01:20:24 GMT, Yasumasa Suenaga  wrote:

> We can see compiler warnings in jni_util.c as following on GCC 11. `buf` 
> should be initialized.

This pull request has now been integrated.

Changeset: 4a9f2319
Author:Yasumasa Suenaga 
URL:   
https://git.openjdk.java.net/jdk/commit/4a9f2319c9cec5c2cc69aafe6abdf93aff120084
Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod

8266173: -Wmaybe-uninitialized happens in jni_util.c

Reviewed-by: dholmes

-

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

RFR: 8266173: -Wmaybe-uninitialized happens in jni_util.c

[Yasumasa Suenaga]

We can see compiler warnings in jni_util.c as following on GCC 11. `buf` should 
be initialized.

-

Commit messages:
 - 8266173: -Wmaybe-uninitialized happens in jni_util.c

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

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

Integrated: 8265152: jpackage cleanup fails on Windows with IOException deleting msi

[Yasumasa Suenaga]

On Sun, 18 Apr 2021 12:44:17 GMT, Yasumasa Suenaga  wrote:

> When creating an "exe" installer on Windows, `AbstractBundler.cleanup()` 
> calls `IOUtils.deleteRecursive()` to delete the tmp directory. It can 
> intermittently fail trying to delete the msi file.
> 
> [JDK-8263135](https://bugs.openjdk.java.net/browse/JDK-8263135) removed 
> `unique_ptr` from jpackage sources due to compiler warnings in VS 2019 
> (16.9.0), so we need to release the resource manually. However `DatabaseView` 
> didn't do that.

This pull request has now been integrated.

Changeset: 142edd3a
Author:Yasumasa Suenaga 
URL:   https://git.openjdk.java.net/jdk/commit/142edd3a
Stats: 6 lines in 1 file changed: 6 ins; 0 del; 0 mod

8265152: jpackage cleanup fails on Windows with IOException deleting msi

Reviewed-by: herrick, asemenyuk

-

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

RFR: 8265152: jpackage cleanup fails on Windows with IOException deleting msi

[Yasumasa Suenaga]

When creating an "exe" installer on Windows, `AbstractBundler.cleanup()` calls 
`IOUtils.deleteRecursive()` to delete the tmp directory. It can intermittently 
fail trying to delete the msi file.

[JDK-8263135](https://bugs.openjdk.java.net/browse/JDK-8263135) removed 
`unique_ptr` from jpackage sources due to compiler warnings in VS 2019 
(16.9.0), so we need to release the resource manually. However `DatabaseView` 
didn't do that.

-

Commit messages:
 - 8265152: jpackage cleanup fails on Windows with IOException deleting msi

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

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

Integrated: 8264551: Unexpected warning when jpackage creates an exe

[Yasumasa Suenaga]

On Wed, 7 Apr 2021 01:09:37 GMT, Yasumasa Suenaga  wrote:

> Since fix to [JDK-8263135](https://bugs.openjdk.java.net/browse/JDK-8263135), 
> we've seen following warning message on the console when we run jpackage to 
> create exe.
> 
> WARNING: MsiCloseHandle(3174034504) failed with error=6 
> 
> It is caused by lack of initializer in `DatabaseRecord(DatabaseView& view)`. 
> We need to add it.

This pull request has now been integrated.

Changeset: a863ab69
Author:Yasumasa Suenaga 
URL:   https://git.openjdk.java.net/jdk/commit/a863ab69
Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod

8264551: Unexpected warning when jpackage creates an exe

Reviewed-by: asemenyuk, herrick

-

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

RFR: 8264551: Unexpected warning when jpackage creates an exe

[Yasumasa Suenaga]

Since fix to [JDK-8263135](https://bugs.openjdk.java.net/browse/JDK-8263135), 
we've seen following warning message on the console when we run jpackage to 
create exe.

WARNING: MsiCloseHandle(3174034504) failed with error=6 

It is caused by lack of initializer in `DatabaseRecord(DatabaseView& view)`. We 
need to add it.

-

Commit messages:
 - Add initializer to DatabaseRecord(DatabaseView& view)

Changes: https://git.openjdk.java.net/jdk/pull/3364/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk=3364=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8264551
  Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod
  Patch: https://git.openjdk.java.net/jdk/pull/3364.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/3364/head:pull/3364

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

Integrated: 8263135: unique_ptr should not be used for types that are not pointers

[Yasumasa Suenaga]

On Sun, 7 Mar 2021 03:15:46 GMT, Yasumasa Suenaga  wrote:

> I saw error during jpackage compilation with VS 2019 (16.9.0) as following 
> (on Japanese locale):
> 
> c:\progra~2\micros~2\2019\commun~1\vc\tools\msvc\1428~1.299\include\utility(604):
>  error C2440: '=': '_Other' から '_Ty' に変換できません。
> with
> [
> _Other=nullptr
> ]
> and
> [
> _Ty=unsigned long
> ]
> c:\progra~2\micros~2\2019\commun~1\vc\tools\msvc\1428~1.299\include\utility(604):
>  note: ネイティブの nullptr はブールに変換するか、または reinterpret_cast を使用して整数型に変換することのみが可能です
> c:\progra~2\micros~2\2019\commun~1\vc\tools\msvc\1428~1.299\include\memory(3423):
>  note: コンパイル対象の関数 テンプレート インスタンス化 '_Ty std::exchange<_Ty2,nullptr>(_Ty 
> &,_Other &&) noexcept(false)' のリファレンスを確認してください
> with
> [
> _Ty=unsigned long,
> _Ty2=unsigned long,
> _Other=nullptr
> ]
> c:\progra~2\micros~2\2019\commun~1\vc\tools\msvc\1428~1.299\include\memory(3422):
>  note: クラス テンプ レート メンバー関数 'unsigned long 
> std::unique_ptr::release(void) noexcept' 
> のコンパイル中
> d:\github-forked\jdk\src\jdk.jpackage\windows\native\common\MsiDb.cpp(237): 
> note: コンパイル対象の関数 テンプレート インスタンス化 'unsigned long 
> std::unique_ptr::release(void) noexcept' 
> のリファレンスを確認してください
> d:\github-forked\jdk\src\jdk.jpackage\windows\native\common\MsiDb.h(119): 
> note: コンパイル対象の クラ ス テンプレート インスタンス化 
> 'std::unique_ptr' のリファレンスを確認してください
> 
> `UniqueMSIHANDLE` is declared in MsiUtils.h as `unique_ptr` for `MSIHANDLE`. 
> `MSIHANDLE` seems to be declared as synonym for `unsigned long`, not a 
> pointer type.
> I think `MSIHANDLE` does not need to handle as `unique_ptr` if it releases at 
> d'tor of the class which holds it (`Database`, `DatabaseRecord`).

This pull request has now been integrated.

Changeset: 4e947607
Author:Yasumasa Suenaga 
URL:   https://git.openjdk.java.net/jdk/commit/4e947607
Stats: 77 lines in 4 files changed: 14 ins; 36 del; 27 mod

8263135: unique_ptr should not be used for types that are not pointers

Reviewed-by: asemenyuk, herrick

-

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

RFR: 8263135: unique_ptr should not be used for types that are not pointers

[Yasumasa Suenaga]

I saw error during jpackage compilation with VS 2019 (16.9.0) as following (on 
Japanese locale):

c:\progra~2\micros~2\2019\commun~1\vc\tools\msvc\1428~1.299\include\utility(604):
 error C2440: '=': '_Other' から '_Ty' に変換できません。
with
[
_Other=nullptr
]
and
[
_Ty=unsigned long
]
c:\progra~2\micros~2\2019\commun~1\vc\tools\msvc\1428~1.299\include\utility(604):
 note: ネイティブの nullptr はブールに変換するか、または reinterpret_cast を使用して整数型に変換することのみが可能です
c:\progra~2\micros~2\2019\commun~1\vc\tools\msvc\1428~1.299\include\memory(3423):
 note: コンパイル対象の関数 テンプレート インスタンス化 '_Ty std::exchange<_Ty2,nullptr>(_Ty &,_Other 
&&) noexcept(false)' のリファレンスを確認してください
with
[
_Ty=unsigned long,
_Ty2=unsigned long,
_Other=nullptr
]
c:\progra~2\micros~2\2019\commun~1\vc\tools\msvc\1428~1.299\include\memory(3422):
 note: クラス テンプ レート メンバー関数 'unsigned long 
std::unique_ptr::release(void) noexcept' 
のコンパイル中
d:\github-forked\jdk\src\jdk.jpackage\windows\native\common\MsiDb.cpp(237): 
note: コンパイル対象の関数 テンプレート インスタンス化 'unsigned long 
std::unique_ptr::release(void) noexcept' 
のリファレンスを確認してください
d:\github-forked\jdk\src\jdk.jpackage\windows\native\common\MsiDb.h(119): note: 
コンパイル対象の クラ ス テンプレート インスタンス化 'std::unique_ptr' 
のリファレンスを確認してください

`UniqueMSIHANDLE` is declared in MsiUtils.h as `unique_ptr` for `MSIHANDLE`. 
`MSIHANDLE` seems to be declared as synonym for `unsigned long`, not a pointer 
type.
I think `MSIHANDLE` does not need to handle as `unique_ptr` if it releases at 
d'tor of the class which holds it (`Database`, `DatabaseRecord`).

-

Commit messages:
 - 8263135: unique_ptr should not be used for types that are not pointers

Changes: https://git.openjdk.java.net/jdk/pull/2858/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk=2858=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8263135
  Stats: 77 lines in 4 files changed: 14 ins; 36 del; 27 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2858.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2858/head:pull/2858

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

Re: Build error with GCC 10 in NetworkInterface.c and k_standard.c

[Yasumasa Suenaga]

Hi Koichi,

Looks good your change.

BTW have you got a sponsor?
I will sponsor you if you do not have yet.


Thanks,

Yasumasa


On 2020/07/30 18:45, Andrew Haley wrote:

On 7/30/20 6:58 AM, Koichi Sakata wrote:

I just formatted the patch to be same as the other code.

Moreover, I saw the following code in k_standard.c.

if (_LIB_VERSION == _SVID_)
exc.retval = zero;
else
exc.retval = zero/zero;

Do we need to do like that in this patch? It seems to be related to the
old matherr(), but I'm not sure about that.


We don't need it; this is ancient history. Nonetheless, feel free to
be consistent with the old style. I'll approve it.

Re: Build error with GCC 10 in NetworkInterface.c and k_standard.c

[Yasumasa Suenaga]

Hi Koichi,

On 2020/07/17 20:26, Koichi Sakata wrote:

Hi Daniel,

 > The changes to NetworkInterface.c look good to me.

Thank you.

 > You'll need to find a reviewer that understands what that
 > method is supposed to do in that case, that's not me ;-)

I understand. This ML is suitable for finding a reviewer, isn't it?
Or, there is another way. We can avoid the error by the accepting 
maybe-uninitialized warning in libfdlibm instead of fixing k_standard.c.

diff --git a/make/modules/java.base/lib/CoreLibraries.gmk 
b/make/modules/java.base/lib/CoreLibraries.gmk
--- a/make/modules/java.base/lib/CoreLibraries.gmk
+++ b/make/modules/java.base/lib/CoreLibraries.gmk
@@ -48,7 +48,7 @@
  CFLAGS := $(CFLAGS_JDKLIB) $(LIBFDLIBM_CFLAGS), \
  CFLAGS_windows_debug := -DLOGGING, \
  CFLAGS_aix := -qfloat=nomaf, \
-    DISABLED_WARNINGS_gcc := sign-compare misleading-indentation array-bounds, 
\
+    DISABLED_WARNINGS_gcc := sign-compare misleading-indentation array-bounds 
maybe-uninitialized, \
  DISABLED_WARNINGS_clang := sign-compare, \
  DISABLED_WARNINGS_microsoft := 4146 4244 4018, \
  ARFLAGS := $(ARFLAGS), \


This change affects all of C sources in core-libs.
So I think it is better affect to k_standard.c only as below:

```
diff --git a/make/modules/java.base/lib/CoreLibraries.gmk 
b/make/modules/java.base/lib/CoreLibraries.gmk
--- a/make/modules/java.base/lib/CoreLibraries.gmk
+++ b/make/modules/java.base/lib/CoreLibraries.gmk
@@ -39,6 +39,10 @@
 LIBFDLIBM_SRC := $(TOPDIR)/src/java.base/share/native/libfdlibm
 LIBFDLIBM_CFLAGS := -I$(LIBFDLIBM_SRC) $(FDLIBM_CFLAGS)

+ifeq ($(call isTargetOs, linux), true)
+  BUILD_LIBFDLIBM_k_standard.c_CFLAGS += -Wno-maybe-uninitialized
+endif
+
 $(eval $(call SetupNativeCompilation, BUILD_LIBFDLIBM, \
 NAME := fdlibm, \
 TYPE := STATIC_LIBRARY, \
```

You can pass compiler option to specified file.


Thanks,

Yasumasa



Thanks,
Koichi

On 2020/07/15 3:36, Daniel Fuchs wrote:

Hi Koichi,

On 13/07/2020 08:03, Koichi Sakata wrote:
 > I understand that. I respect your idea.
 > I fixed the patch as follows.
The changes to NetworkInterface.c look good to me.

 >
 > By the way, k_standard.c still remains. Is there a way to proceed with it?

You'll need to find a reviewer that understands what that
method is supposed to do in that case, that's not me ;-)

best regards,

-- daniel


Thanks, > Koichi

= PATCH =
diff --git a/src/java.base/unix/native/libnet/NetworkInterface.c 
b/src/java.base/unix/native/libnet/NetworkInterface.c
--- a/src/java.base/unix/native/libnet/NetworkInterface.c
+++ b/src/java.base/unix/native/libnet/NetworkInterface.c
@@ -1296,7 +1296,8 @@
  static int getIndex(int sock, const char *name) {
  struct ifreq if2;
  memset((char *), 0, sizeof(if2));
-    strncpy(if2.ifr_name, name, sizeof(if2.ifr_name) - 1);
+    strncpy(if2.ifr_name, name, sizeof(if2.ifr_name));
+    if2.ifr_name[sizeof(if2.ifr_name) - 1] = 0;

  if (ioctl(sock, SIOCGIFINDEX, (char *)) < 0) {
  return -1;
@@ -1359,7 +1360,8 @@
  static int getFlags(int sock, const char *ifname, int *flags) {
  struct ifreq if2;
  memset((char *), 0, sizeof(if2));
-    strncpy(if2.ifr_name, ifname, sizeof(if2.ifr_name) - 1);
+    strncpy(if2.ifr_name, ifname, sizeof(if2.ifr_name));
+    if2.ifr_name[sizeof(if2.ifr_name) - 1] = 0;

  if (ioctl(sock, SIOCGIFFLAGS, (char *)) < 0) {
  return -1;
diff --git a/src/java.base/share/native/libfdlibm/k_standard.c 
b/src/java.base/share/native/libfdlibm/k_standard.c
--- a/src/java.base/share/native/libfdlibm/k_standard.c
+++ b/src/java.base/share/native/libfdlibm/k_standard.c
@@ -739,6 +739,10 @@
  errno = EDOM;
  }
  break;
+    default:
+    exc.retval = zero;
+    errno = EINVAL;
+    break;
  }
  return exc.retval;
  }

Re: RFR: 8242283: Can't start JVM when java home path includes non-ASCII character

[Yasumasa Suenaga]

Thanks Sato-san!

I added noreg-hard to JBS.

Can I get reviewer(s) from HotSpot folks?
This webrev changes HotSpot code (os_windows.cpp).


Yasumasa


On 2020/04/11 2:17, naoto.s...@oracle.com wrote:

Suenaga-san,

Looks good to me. Please add noreg-hard if you are not planning to provide test 
cases. Thank you for fixing this issue.

Naoto

On 4/10/20 7:29 AM, Yasumasa Suenaga wrote:

Hi all,

Please review this change:

   JBS: https://bugs.openjdk.java.net/browse/JDK-8242283
   webrev: http://cr.openjdk.java.net/~ysuenaga/JDK-8242283/webrev.00/

After JDK-8240197 and JDK-8240725, java cannot start when java home path 
includes non-ASCII character e.g. Japanese Kanji.
We've discussed about this issue on JBS, and we decide to replace CP_THREAD_ACP 
to CP_ACP in MultiByteToWideChar() because CP_ACP works nicely rather than 
CP_THREAD_ACP.

But this change would not resolve in all cases. For example, Japanese encoded 
pathname cannot be recognized under the English system locale. It is 
(implicitly) described in the spec for JNI_CreateJavaVM().

https://docs.oracle.com/en/java/javase/14/docs/specs/jni/invocation.html#jni_createjavavm:
```
 char *optionString; /* the option as a string in the default platform 
encoding */
```


Thanks,

Yasumasa

RFR: 8242283: Can't start JVM when java home path includes non-ASCII character

[Yasumasa Suenaga]

Hi all,

Please review this change:

  JBS: https://bugs.openjdk.java.net/browse/JDK-8242283
  webrev: http://cr.openjdk.java.net/~ysuenaga/JDK-8242283/webrev.00/

After JDK-8240197 and JDK-8240725, java cannot start when java home path 
includes non-ASCII character e.g. Japanese Kanji.
We've discussed about this issue on JBS, and we decide to replace CP_THREAD_ACP 
to CP_ACP in MultiByteToWideChar() because CP_ACP works nicely rather than 
CP_THREAD_ACP.

But this change would not resolve in all cases. For example, Japanese encoded 
pathname cannot be recognized under the English system locale. It is 
(implicitly) described in the spec for JNI_CreateJavaVM().

https://docs.oracle.com/en/java/javase/14/docs/specs/jni/invocation.html#jni_createjavavm:
```
char *optionString; /* the option as a string in the default platform 
encoding */
```


Thanks,

Yasumasa

Re: RFR: 8232846: ProcessHandle.Info command with non-English shows question marks

[Yasumasa Suenaga]

Ok, your change looks good.


Thanks,

Yasumasa


On 2020/03/27 18:10, Toshio 5 Nakamura wrote:

Hi Suenaga-san,
Thank you for the comment. I updated the limit to 32768.
webrev.02: http://cr.openjdk.java.net/~tnakamura/8232846/webrev.02
Well, I believe the new logic works as same as the current one.
I added NULL initialization to commandObj, and the value will be changed only 
if QueryFullProcessImageNameW() was success.
Then, CHECK_NULL(commandObj) works as "return" if commandObj is NULL.
src/java.base/share/native/libjava/jni_util.h
 > #define CHECK_NULL(x)   \
 > do {    \
 > if ((x) == NULL) {  \
 > return; \
 > }   \
 > } while (0) \
I hope this solves your concern.
Best regards,
Toshio Nakamura

- Original message -
Nakamura-san,

I think location of CHECK_NULL and SetObjectField() are incorrect.
Currently they are called when QueryFullProcessImageName() succeed only.

In addition, you need to allocate 32768 chars (32767 + 1 ('\0')) via malloc.
I understand "32767" is max length, not includes null-terminator.


Thanks,

Yasumasa


On 2020/03/27 14:06, Toshio 5 Nakamura wrote:
 >
 > Hi Roger, Suenaga-san,
 >
 > Thank you for your comments and discussion for the issue.
 > I've updated webrev. Could you review it again?
 > tier1 tests passed.
 >
 > webrev.01: http://cr.openjdk.java.net/~tnakamura/8232846/webrev.01
     >
     > Best regards,
 > Toshio Nakamura
 >
 > Yasumasa Suenaga  wrote on 2020/03/27 09:10:15:
 >
 >> On 2020/03/27 9:01, Roger Riggs wrote:
 >>> Hi,
 >>>
 >>> Please don't throw an exception.
 >>> It would abort being able to provide any information.
 >>> And who would expect or handle such an exception?
 >>>
 >>> Degrading the info or omitting it provides better service overall.
 >>> There is no point to throwing an exception.
 >>
 >> hProcess in QueryFullProcessImageNameW needs
 >> PROCESS_QUERY_INFORMATION or PROCESS_QUERY_LIMITED_INFORMATION
 >> access right, so I thought it is reasonable to throw runtime
 >> exception if the call failed.
 >>
 >> Anyway, I agree with you if throwing exception is not comfortable in
 >> this case for Java API.
 >>
 >>
 >> Thanks,
 >>
 >> Yasumasa
 >>
 >>
 >>> Regards, Roger
 >>>
 >>>
 >>> On 3/26/20 7:55 PM, Yasumasa Suenaga wrote:
 >>>> On 2020/03/27 0:35, Roger Riggs wrote:
 >>>>> Hi,
 >>>>>
 >>>>> Reading further down the reference to the section:
 >>>>> "Enable Long Paths in Windows 10, Version 1607, and Later"
 >>>>> might suggest the code be more resilient to long paths.
 >>>>>
 >>>>> The stack allocated buffer (1024) is fine, but I'd suggest
 >> adding code to retry in the case
 >>>>> of INSUFFICIENT BUFFER with a malloc'd buffer to make it more
 >> robust since the enabling
 >>>>> of long paths is environment and application specific.
 >>>>
 >>>> In addition, it's better to throw an exception if the call failed
 >> due to other error code.
 >>>>
 >>>> Thanks,
 >>>>
 >>>> Yasumasa
 >>>>
 >>>>
 >>>>> Thanks, Roger
 >>>>>
 >>>>>
 >>>>>
 >>>>> On 3/26/20 10:40 AM, Toshio 5 Nakamura wrote:
 >>>>>> Hi Thomas, Suenaga-san, Roger,
 >>>>>>
 >>>>>> Thank you for reviewing and comments.
 >>>>>>
 >>>>>> I checked behaviors by a small program and debugger.
 >>>>>> If QueryFullProcessImageNameW failed by not enough buffer,
 >>>>>> the API didn't change the buffer.
 >>>>>> It just set ERROR_INSUFFICIENT_BUFFER(0x7a) to LastError.
 >>>>>>
 >>>>>> The buffer size becomes 1024 characters (2048 bytes) by this patch.
 >>>>>> Should it use bigger size with malloc? 32,767 characters can belimit
 > [2].
 >>>>>> I feel 1024 is reasonable value for command's location

Re: RFR: 8232846: ProcessHandle.Info command with non-English shows question marks

[Yasumasa Suenaga]

Nakamura-san,

I think location of CHECK_NULL and SetObjectField() are incorrect.
Currently they are called when QueryFullProcessImageName() succeed only.

In addition, you need to allocate 32768 chars (32767 + 1 ('\0')) via malloc.
I understand "32767" is max length, not includes null-terminator.


Thanks,

Yasumasa


On 2020/03/27 14:06, Toshio 5 Nakamura wrote:


Hi Roger, Suenaga-san,

Thank you for your comments and discussion for the issue.
I've updated webrev. Could you review it again?
tier1 tests passed.

webrev.01: http://cr.openjdk.java.net/~tnakamura/8232846/webrev.01

Best regards,
Toshio Nakamura

Yasumasa Suenaga  wrote on 2020/03/27 09:10:15:


On 2020/03/27 9:01, Roger Riggs wrote:

Hi,

Please don't throw an exception.
It would abort being able to provide any information.
And who would expect or handle such an exception?

Degrading the info or omitting it provides better service overall.
There is no point to throwing an exception.


hProcess in QueryFullProcessImageNameW needs
PROCESS_QUERY_INFORMATION or PROCESS_QUERY_LIMITED_INFORMATION
access right, so I thought it is reasonable to throw runtime
exception if the call failed.

Anyway, I agree with you if throwing exception is not comfortable in
this case for Java API.


Thanks,

Yasumasa



Regards, Roger


On 3/26/20 7:55 PM, Yasumasa Suenaga wrote:

On 2020/03/27 0:35, Roger Riggs wrote:

Hi,

Reading further down the reference to the section:
"Enable Long Paths in Windows 10, Version 1607, and Later"
might suggest the code be more resilient to long paths.

The stack allocated buffer (1024) is fine, but I'd suggest

adding code to retry in the case

of INSUFFICIENT BUFFER with a malloc'd buffer to make it more

robust since the enabling

of long paths is environment and application specific.


In addition, it's better to throw an exception if the call failed

due to other error code.


Thanks,

Yasumasa



Thanks, Roger



On 3/26/20 10:40 AM, Toshio 5 Nakamura wrote:

Hi Thomas, Suenaga-san, Roger,

Thank you for reviewing and comments.

I checked behaviors by a small program and debugger.
If QueryFullProcessImageNameW failed by not enough buffer,
the API didn't change the buffer.
It just set ERROR_INSUFFICIENT_BUFFER(0x7a) to LastError.

The buffer size becomes 1024 characters (2048 bytes) by this patch.
Should it use bigger size with malloc? 32,767 characters can belimit

[2].

I feel 1024 is reasonable value for command's location.

[2]


https://urldefense.com/v3/__https://docs.microsoft.com/en-us/windows/win32/fileio/naming-a-file*maximum-path-length-limitation__;Iw!!GqivPVa7Brio!PgzCcUQOjCNpcVeetU_drS4DFFVLaj0ceJBvipX7iDc_RlPtcEkdH8AzelGtzqXS
$

Best regards,
Toshio Nakamura


Hi,

If the call fails, the command field in the info will not be set

(and

therefore null).

I agree that a length of 512 (characters) is probably too small.

Roger


On 3/26/20 6:37 AM, Yasumasa Suenaga wrote:

Hi Nakamura-san,

Your change almost looks good, but I have one question.
Length of exeName (1024) is enough?

According to Microsoft Doc [1], exeName might not be

null-terminated

if it failed.
I concern buffer overrun might occur when

QueryFullProcessImageNameW

failed.


Thanks,

Yasumasa


[1]



https://urldefense.proofpoint.com/v2/url?u=https-3A__docs.microsoft.com_en-2Dus_windows_win32_api_winbase_nf-2Dwinbase-2Dqueryfullprocessimagenamew=DwICaQ=jf_iaSHvJObTbx-siA1ZOg=EVbFABcgo-X99_TGI2-qsMtyulHUruf8lAzMlVpVRqw=noFyLWDG1dMjO5TlaqM_zvW_f-8fvveS2EoZylT5DMQ=oc_KTGmJUjsfnLf3tjWr9DO1dwWp1TqIZgd2EiHVW14=





On 2020/03/26 17:58, Toshio 5 Nakamura wrote:

Hi All,

Could you review this change? Additionally, I'd like to ask a

sponsor

for
it, since I'm not a committer.

Bug:



https://urldefense.proofpoint.com/v2/url?u=https-3A__bugs.openjdk.java.net_browse_JDK-2D8232846=DwICaQ=jf_iaSHvJObTbx-siA1ZOg=EVbFABcgo-X99_TGI2-qsMtyulHUruf8lAzMlVpVRqw=noFyLWDG1dMjO5TlaqM_zvW_f-8fvveS2EoZylT5DMQ=pn_uS0DojBk6-R6R5QwtYFJvJpaabhia-eiOtIrJO9Q=




Webrev:



https://urldefense.proofpoint.com/v2/url?u=http-3A__cr.openjdk.java.net_-7Etnakamura_8232846_webrev.00=DwICaQ=jf_iaSHvJObTbx-siA1ZOg=EVbFABcgo-X99_TGI2-qsMtyulHUruf8lAzMlVpVRqw=noFyLWDG1dMjO5TlaqM_zvW_f-8fvveS2EoZylT5DMQ=NylYK0Q3nFFZRVIaeUna6iaClaJm1CWJ2Zkwy-ysvv4=




Under Windows environments, ProcessHandle.Info.command shows

question

marks
if the command has non-English characters. I'd like to change the
underlying API to Unicode version.
Tier1 tests passed.

Best regards,
Toshio Nakamura

Re: RFR: 8232846: ProcessHandle.Info command with non-English shows question marks

[Yasumasa Suenaga]

On 2020/03/27 9:01, Roger Riggs wrote:

Hi,

Please don't throw an exception.
It would abort being able to provide any information.
And who would expect or handle such an exception?

Degrading the info or omitting it provides better service overall.
There is no point to throwing an exception.


hProcess in QueryFullProcessImageNameW needs PROCESS_QUERY_INFORMATION or 
PROCESS_QUERY_LIMITED_INFORMATION access right, so I thought it is reasonable 
to throw runtime exception if the call failed.

Anyway, I agree with you if throwing exception is not comfortable in this case 
for Java API.


Thanks,

Yasumasa



Regards, Roger


On 3/26/20 7:55 PM, Yasumasa Suenaga wrote:

On 2020/03/27 0:35, Roger Riggs wrote:

Hi,

Reading further down the reference to the section:
"Enable Long Paths in Windows 10, Version 1607, and Later"
might suggest the code be more resilient to long paths.

The stack allocated buffer (1024) is fine, but I'd suggest adding code to retry 
in the case
of INSUFFICIENT BUFFER with a malloc'd buffer to make it more robust since the 
enabling
of long paths is environment and application specific.


In addition, it's better to throw an exception if the call failed due to other 
error code.

Thanks,

Yasumasa



Thanks, Roger



On 3/26/20 10:40 AM, Toshio 5 Nakamura wrote:

Hi Thomas, Suenaga-san, Roger,

Thank you for reviewing and comments.

I checked behaviors by a small program and debugger.
If QueryFullProcessImageNameW failed by not enough buffer,
the API didn't change the buffer.
It just set ERROR_INSUFFICIENT_BUFFER(0x7a) to LastError.

The buffer size becomes 1024 characters (2048 bytes) by this patch.
Should it use bigger size with malloc? 32,767 characters can be limit [2].
I feel 1024 is reasonable value for command's location.

[2]
https://urldefense.com/v3/__https://docs.microsoft.com/en-us/windows/win32/fileio/naming-a-file*maximum-path-length-limitation__;Iw!!GqivPVa7Brio!PgzCcUQOjCNpcVeetU_drS4DFFVLaj0ceJBvipX7iDc_RlPtcEkdH8AzelGtzqXS$
Best regards,
Toshio Nakamura


Hi,

If the call fails, the command field in the info will not be set (and
therefore null).

I agree that a length of 512 (characters) is probably too small.

Roger


On 3/26/20 6:37 AM, Yasumasa Suenaga wrote:

Hi Nakamura-san,

Your change almost looks good, but I have one question.
Length of exeName (1024) is enough?

According to Microsoft Doc [1], exeName might not be null-terminated
if it failed.
I concern buffer overrun might occur when QueryFullProcessImageNameW
failed.


Thanks,

Yasumasa


[1]

https://urldefense.proofpoint.com/v2/url?u=https-3A__docs.microsoft.com_en-2Dus_windows_win32_api_winbase_nf-2Dwinbase-2Dqueryfullprocessimagenamew=DwICaQ=jf_iaSHvJObTbx-siA1ZOg=EVbFABcgo-X99_TGI2-qsMtyulHUruf8lAzMlVpVRqw=noFyLWDG1dMjO5TlaqM_zvW_f-8fvveS2EoZylT5DMQ=oc_KTGmJUjsfnLf3tjWr9DO1dwWp1TqIZgd2EiHVW14=



On 2020/03/26 17:58, Toshio 5 Nakamura wrote:

Hi All,

Could you review this change? Additionally, I'd like to ask a sponsor
for
it, since I'm not a committer.

Bug:

https://urldefense.proofpoint.com/v2/url?u=https-3A__bugs.openjdk.java.net_browse_JDK-2D8232846=DwICaQ=jf_iaSHvJObTbx-siA1ZOg=EVbFABcgo-X99_TGI2-qsMtyulHUruf8lAzMlVpVRqw=noFyLWDG1dMjO5TlaqM_zvW_f-8fvveS2EoZylT5DMQ=pn_uS0DojBk6-R6R5QwtYFJvJpaabhia-eiOtIrJO9Q=


Webrev:

https://urldefense.proofpoint.com/v2/url?u=http-3A__cr.openjdk.java.net_-7Etnakamura_8232846_webrev.00=DwICaQ=jf_iaSHvJObTbx-siA1ZOg=EVbFABcgo-X99_TGI2-qsMtyulHUruf8lAzMlVpVRqw=noFyLWDG1dMjO5TlaqM_zvW_f-8fvveS2EoZylT5DMQ=NylYK0Q3nFFZRVIaeUna6iaClaJm1CWJ2Zkwy-ysvv4=


Under Windows environments, ProcessHandle.Info.command shows question
marks
if the command has non-English characters. I'd like to change the
underlying API to Unicode version.
Tier1 tests passed.

Best regards,
Toshio Nakamura

Re: RFR: 8232846: ProcessHandle.Info command with non-English shows question marks

[Yasumasa Suenaga]

On 2020/03/27 0:35, Roger Riggs wrote:

Hi,

Reading further down the reference to the section:
"Enable Long Paths in Windows 10, Version 1607, and Later"
might suggest the code be more resilient to long paths.

The stack allocated buffer (1024) is fine, but I'd suggest adding code to retry 
in the case
of INSUFFICIENT BUFFER with a malloc'd buffer to make it more robust since the 
enabling
of long paths is environment and application specific.


In addition, it's better to throw an exception if the call failed due to other 
error code.

Thanks,

Yasumasa



Thanks, Roger



On 3/26/20 10:40 AM, Toshio 5 Nakamura wrote:

Hi Thomas, Suenaga-san, Roger,

Thank you for reviewing and comments.

I checked behaviors by a small program and debugger.
If QueryFullProcessImageNameW failed by not enough buffer,
the API didn't change the buffer.
It just set ERROR_INSUFFICIENT_BUFFER(0x7a) to LastError.

The buffer size becomes 1024 characters (2048 bytes) by this patch.
Should it use bigger size with malloc? 32,767 characters can be limit [2].
I feel 1024 is reasonable value for command's location.

[2]
https://docs.microsoft.com/en-us/windows/win32/fileio/naming-a-file#maximum-path-length-limitation

Best regards,
Toshio Nakamura


Hi,

If the call fails, the command field in the info will not be set (and
therefore null).

I agree that a length of 512 (characters) is probably too small.

Roger


On 3/26/20 6:37 AM, Yasumasa Suenaga wrote:

Hi Nakamura-san,

Your change almost looks good, but I have one question.
Length of exeName (1024) is enough?

According to Microsoft Doc [1], exeName might not be null-terminated
if it failed.
I concern buffer overrun might occur when QueryFullProcessImageNameW
failed.


Thanks,

Yasumasa


[1]

https://urldefense.proofpoint.com/v2/url?u=https-3A__docs.microsoft.com_en-2Dus_windows_win32_api_winbase_nf-2Dwinbase-2Dqueryfullprocessimagenamew=DwICaQ=jf_iaSHvJObTbx-siA1ZOg=EVbFABcgo-X99_TGI2-qsMtyulHUruf8lAzMlVpVRqw=noFyLWDG1dMjO5TlaqM_zvW_f-8fvveS2EoZylT5DMQ=oc_KTGmJUjsfnLf3tjWr9DO1dwWp1TqIZgd2EiHVW14=



On 2020/03/26 17:58, Toshio 5 Nakamura wrote:

Hi All,

Could you review this change? Additionally, I'd like to ask a sponsor
for
it, since I'm not a committer.

Bug:

https://urldefense.proofpoint.com/v2/url?u=https-3A__bugs.openjdk.java.net_browse_JDK-2D8232846=DwICaQ=jf_iaSHvJObTbx-siA1ZOg=EVbFABcgo-X99_TGI2-qsMtyulHUruf8lAzMlVpVRqw=noFyLWDG1dMjO5TlaqM_zvW_f-8fvveS2EoZylT5DMQ=pn_uS0DojBk6-R6R5QwtYFJvJpaabhia-eiOtIrJO9Q=


Webrev:

https://urldefense.proofpoint.com/v2/url?u=http-3A__cr.openjdk.java.net_-7Etnakamura_8232846_webrev.00=DwICaQ=jf_iaSHvJObTbx-siA1ZOg=EVbFABcgo-X99_TGI2-qsMtyulHUruf8lAzMlVpVRqw=noFyLWDG1dMjO5TlaqM_zvW_f-8fvveS2EoZylT5DMQ=NylYK0Q3nFFZRVIaeUna6iaClaJm1CWJ2Zkwy-ysvv4=


Under Windows environments, ProcessHandle.Info.command shows question
marks
if the command has non-English characters. I'd like to change the
underlying API to Unicode version.
Tier1 tests passed.

Best regards,
Toshio Nakamura

Re: RFR: 8232846: ProcessHandle.Info command with non-English shows question marks

[Yasumasa Suenaga]

Hi Nakamura-san,

Your change almost looks good, but I have one question.
Length of exeName (1024) is enough?

According to Microsoft Doc [1], exeName might not be null-terminated if it 
failed.
I concern buffer overrun might occur when QueryFullProcessImageNameW failed.


Thanks,

Yasumasa


[1] 
https://docs.microsoft.com/en-us/windows/win32/api/winbase/nf-winbase-queryfullprocessimagenamew


On 2020/03/26 17:58, Toshio 5 Nakamura wrote:


Hi All,

Could you review this change? Additionally, I'd like to ask a sponsor for
it, since I'm not a committer.

Bug: https://bugs.openjdk.java.net/browse/JDK-8232846
Webrev: http://cr.openjdk.java.net/~tnakamura/8232846/webrev.00

Under Windows environments, ProcessHandle.Info.command shows question marks
if the command has non-English characters. I'd like to change the
underlying API to Unicode version.
Tier1 tests passed.

Best regards,
Toshio Nakamura

Re: RFR: 8240725: Some functions might not work with CJK character

[Yasumasa Suenaga]

Thanks Sato-san!

Yasumasa


On 2020/03/11 12:27, naoto.s...@oracle.com wrote:

Looks good to me. Thanks for the fix!

Naoto

On 3/10/20 5:39 PM, Yasumasa Suenaga wrote:

Hi Sato-san,

Thanks for your comment.
I uploaded new webrev:

   http://cr.openjdk.java.net/~ysuenaga/JDK-8240725/webrev.03/

On 2020/03/11 2:08, naoto.s...@oracle.com wrote:

Hi Suenaga-san,

Thank you for the update. I think your changes to the return values of 
MultiByteToWideChar look good.

Then I noticed (should have noticed in the first place, sorry) that the error handling in canonicalize_md.c is 
incorrect (unrelated to your change). All error cases "goto finish" which would end up "free(NULL)" 
in some cases, e.g., line 340 (in the new file) should not end up calling "free(wresult)" and 
"free(wpath)"

java_md.c

- "convert_to_unicode()" has different indentation than others (2 spaces which 
should be 4).


I fixed it.



- Should "wpath" be set to NULL before returning EINVAL at line 524? I don't know the 
contract of "create_unc_path", but the caller would receive a garbage pointer as a return 
value.


create_unc_path() is static function, and it would be called by just JLI_Open().
If create_unc_path() would be failed (it means `err` is set excepting 
ERROR_SUCCESS), wpath would call free() when it is not NULL.

Thus we should set related values as below:

   A: wpath = NULL, err != ERROR_SUCCESS
   B: wpath != NULL (not free'ed), err != ERROR_SUCCESS

I implemented A in this webrev because it is simple.


Thanks,

Yasumasa



Nit: please add "noreg-hard" tag to the JIRA issue.

Naoto


On 3/9/20 7:52 PM, Yasumasa Suenaga wrote:

I forgot to free buffer if MultiByteToWideChar() failed in zip_util.c .
So I updated webrev:

   http://cr.openjdk.java.net/~ysuenaga/JDK-8240725/webrev.02/


Yasumasa


On 2020/03/10 9:12, Yasumasa Suenaga wrote:

Hi Sato-san,

Thanks for your comment!

I updated webrev. Could you review again?

   http://cr.openjdk.java.net/~ysuenaga/JDK-8240725/webrev.01/


Yasumasa


On 2020/03/10 2:24, naoto.s...@oracle.com wrote:

Hi Suenaga-san,

I think the return value from the second MultiByteToWideChar invocation should 
be examined (non-zero), and if it was not successful, appropriate action should 
be taken.

Naoto

On 3/9/20 3:50 AM, Yasumasa Suenaga wrote:

Hi all,

Please review this change:

   JBS: https://bugs.openjdk.java.net/browse/JDK-8240725
   webrev: http://cr.openjdk.java.net/~ysuenaga/JDK-8240725/webrev.00/

We found the issue that HotSpot does not start when it is deployed on the path 
which contains CJK character(s), and it has been fixed in JDK-8240197.

On the review of JDK-8240197 [1], we concern similar issue might occur in other 
place, and I found potentially problem in below:

- ZFILE_Open() @ zip_util.c
- JDK_Canonicalize() @ canonicalize_md.c (for Windows)
- create_unc_path() @ java_md.c (for Windows)
- Platform::MultibyteStringToWideString() @ WindowsPlatform.cpp

This change passed tests on submit repo 
(mach5-one-ysuenaga-JDK-8240725-20200309-0811-9304139).


Thanks,

Yasumasa


[1] 
https://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/2020-March/038397.html

Re: RFR: 8240725: Some functions might not work with CJK character

[Yasumasa Suenaga]

Hi Sato-san,

Thanks for your comment.
I uploaded new webrev:

  http://cr.openjdk.java.net/~ysuenaga/JDK-8240725/webrev.03/

On 2020/03/11 2:08, naoto.s...@oracle.com wrote:

Hi Suenaga-san,

Thank you for the update. I think your changes to the return values of 
MultiByteToWideChar look good.

Then I noticed (should have noticed in the first place, sorry) that the error handling in canonicalize_md.c is 
incorrect (unrelated to your change). All error cases "goto finish" which would end up "free(NULL)" 
in some cases, e.g., line 340 (in the new file) should not end up calling "free(wresult)" and 
"free(wpath)"

java_md.c

- "convert_to_unicode()" has different indentation than others (2 spaces which 
should be 4).


I fixed it.



- Should "wpath" be set to NULL before returning EINVAL at line 524? I don't know the 
contract of "create_unc_path", but the caller would receive a garbage pointer as a return 
value.


create_unc_path() is static function, and it would be called by just JLI_Open().
If create_unc_path() would be failed (it means `err` is set excepting 
ERROR_SUCCESS), wpath would call free() when it is not NULL.

Thus we should set related values as below:

  A: wpath = NULL, err != ERROR_SUCCESS
  B: wpath != NULL (not free'ed), err != ERROR_SUCCESS

I implemented A in this webrev because it is simple.


Thanks,

Yasumasa



Nit: please add "noreg-hard" tag to the JIRA issue.

Naoto


On 3/9/20 7:52 PM, Yasumasa Suenaga wrote:

I forgot to free buffer if MultiByteToWideChar() failed in zip_util.c .
So I updated webrev:

   http://cr.openjdk.java.net/~ysuenaga/JDK-8240725/webrev.02/


Yasumasa


On 2020/03/10 9:12, Yasumasa Suenaga wrote:

Hi Sato-san,

Thanks for your comment!

I updated webrev. Could you review again?

   http://cr.openjdk.java.net/~ysuenaga/JDK-8240725/webrev.01/


Yasumasa


On 2020/03/10 2:24, naoto.s...@oracle.com wrote:

Hi Suenaga-san,

I think the return value from the second MultiByteToWideChar invocation should 
be examined (non-zero), and if it was not successful, appropriate action should 
be taken.

Naoto

On 3/9/20 3:50 AM, Yasumasa Suenaga wrote:

Hi all,

Please review this change:

   JBS: https://bugs.openjdk.java.net/browse/JDK-8240725
   webrev: http://cr.openjdk.java.net/~ysuenaga/JDK-8240725/webrev.00/

We found the issue that HotSpot does not start when it is deployed on the path 
which contains CJK character(s), and it has been fixed in JDK-8240197.

On the review of JDK-8240197 [1], we concern similar issue might occur in other 
place, and I found potentially problem in below:

- ZFILE_Open() @ zip_util.c
- JDK_Canonicalize() @ canonicalize_md.c (for Windows)
- create_unc_path() @ java_md.c (for Windows)
- Platform::MultibyteStringToWideString() @ WindowsPlatform.cpp

This change passed tests on submit repo 
(mach5-one-ysuenaga-JDK-8240725-20200309-0811-9304139).


Thanks,

Yasumasa


[1] 
https://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/2020-March/038397.html

Re: RFR: 8240725: Some functions might not work with CJK character

[Yasumasa Suenaga]

I forgot to free buffer if MultiByteToWideChar() failed in zip_util.c .
So I updated webrev:

  http://cr.openjdk.java.net/~ysuenaga/JDK-8240725/webrev.02/


Yasumasa


On 2020/03/10 9:12, Yasumasa Suenaga wrote:

Hi Sato-san,

Thanks for your comment!

I updated webrev. Could you review again?

   http://cr.openjdk.java.net/~ysuenaga/JDK-8240725/webrev.01/


Yasumasa


On 2020/03/10 2:24, naoto.s...@oracle.com wrote:

Hi Suenaga-san,

I think the return value from the second MultiByteToWideChar invocation should 
be examined (non-zero), and if it was not successful, appropriate action should 
be taken.

Naoto

On 3/9/20 3:50 AM, Yasumasa Suenaga wrote:

Hi all,

Please review this change:

   JBS: https://bugs.openjdk.java.net/browse/JDK-8240725
   webrev: http://cr.openjdk.java.net/~ysuenaga/JDK-8240725/webrev.00/

We found the issue that HotSpot does not start when it is deployed on the path 
which contains CJK character(s), and it has been fixed in JDK-8240197.

On the review of JDK-8240197 [1], we concern similar issue might occur in other 
place, and I found potentially problem in below:

- ZFILE_Open() @ zip_util.c
- JDK_Canonicalize() @ canonicalize_md.c (for Windows)
- create_unc_path() @ java_md.c (for Windows)
- Platform::MultibyteStringToWideString() @ WindowsPlatform.cpp

This change passed tests on submit repo 
(mach5-one-ysuenaga-JDK-8240725-20200309-0811-9304139).


Thanks,

Yasumasa


[1] 
https://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/2020-March/038397.html

Re: RFR: 8240725: Some functions might not work with CJK character

[Yasumasa Suenaga]

Hi Sato-san,

Thanks for your comment!

I updated webrev. Could you review again?

  http://cr.openjdk.java.net/~ysuenaga/JDK-8240725/webrev.01/


Yasumasa


On 2020/03/10 2:24, naoto.s...@oracle.com wrote:

Hi Suenaga-san,

I think the return value from the second MultiByteToWideChar invocation should 
be examined (non-zero), and if it was not successful, appropriate action should 
be taken.

Naoto

On 3/9/20 3:50 AM, Yasumasa Suenaga wrote:

Hi all,

Please review this change:

   JBS: https://bugs.openjdk.java.net/browse/JDK-8240725
   webrev: http://cr.openjdk.java.net/~ysuenaga/JDK-8240725/webrev.00/

We found the issue that HotSpot does not start when it is deployed on the path 
which contains CJK character(s), and it has been fixed in JDK-8240197.

On the review of JDK-8240197 [1], we concern similar issue might occur in other 
place, and I found potentially problem in below:

- ZFILE_Open() @ zip_util.c
- JDK_Canonicalize() @ canonicalize_md.c (for Windows)
- create_unc_path() @ java_md.c (for Windows)
- Platform::MultibyteStringToWideString() @ WindowsPlatform.cpp

This change passed tests on submit repo 
(mach5-one-ysuenaga-JDK-8240725-20200309-0811-9304139).


Thanks,

Yasumasa


[1] 
https://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/2020-March/038397.html

RFR: 8240725: Some functions might not work with CJK character

[Yasumasa Suenaga]

Hi all,

Please review this change:

  JBS: https://bugs.openjdk.java.net/browse/JDK-8240725
  webrev: http://cr.openjdk.java.net/~ysuenaga/JDK-8240725/webrev.00/

We found the issue that HotSpot does not start when it is deployed on the path 
which contains CJK character(s), and it has been fixed in JDK-8240197.

On the review of JDK-8240197 [1], we concern similar issue might occur in other 
place, and I found potentially problem in below:

- ZFILE_Open() @ zip_util.c
- JDK_Canonicalize() @ canonicalize_md.c (for Windows)
- create_unc_path() @ java_md.c (for Windows)
- Platform::MultibyteStringToWideString() @ WindowsPlatform.cpp

This change passed tests on submit repo 
(mach5-one-ysuenaga-JDK-8240725-20200309-0811-9304139).


Thanks,

Yasumasa


[1] 
https://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/2020-March/038397.html

Re: JDK-8237818: Typo in Unsafe: resposibility

[Yasumasa Suenaga]

On 2020/02/17 11:21, David Holmes wrote:

Hi Yasumasa,

On 17/02/2020 11:38 am, Yasumasa Suenaga wrote:

Hi David,

On 2020/02/17 8:10, David Holmes wrote:

On 17/02/2020 12:40 am, Aya Ebata wrote:

Hi,

I sent OCA today. So it hasn't been approved yet. I'm waiting for approval.


For this trivial contribution an OCA is a not required, so this can be 
sponsored before the OCA goes through processing.


I didn't know that!
I hope it is described in "How to contribute" page [1].


Unfortunately no, it is covered in the by-laws [1], when distinguishing a 
Participant from a Contributor:

"A Participant may post messages to a list, submit simple patches, and make other 
kinds of small contributions."

"A Contributor is a Participant who has signed the Oracle Contributor Agreement 
(OCA),  ..."


Thanks for your information!


Cheers,

Yasumasa



Cheers,
David

[1] https://openjdk.java.net/bylaws




Anyway, I will push Aya's change to jdk/jdk.


Thanks,

Yasumasa


[1] https://openjdk.java.net/contribute/



Cheers,
David


regards,
Aya Ebata


2020年2月16日(日) 16:49 Yasumasa Suenaga :


Hi Aya,

It looks good to me.
BTW, have you signed to OCA? If so, I will sponsor you.


Thanks,

Yasumasa


On 2020/02/16 16:42, 江畑 彩 wrote:

Hi,

I fixed typo in s.m.Unsafe and j.i.Unsafe. The changes are below.
Could you sponsor this, please.
https://bugs.openjdk.java.net/browse/JDK-8237818

regards,
Aya Ebata



diff --git a/src/java.base/share/classes/jdk/internal/misc/Unsafe.java
b/src/java.base/share/classes/jdk/internal/misc/Unsafe.java
--- a/src/java.base/share/classes/jdk/internal/misc/Unsafe.java
+++ b/src/java.base/share/classes/jdk/internal/misc/Unsafe.java
@@ -40,7 +40,7 @@
    * Although the class and all methods are public, use of this class is
    * limited because only trusted code can obtain instances of it.
    *
- * Note: It is the resposibility of the caller to make sure
+ * Note: It is the responsibility of the caller to make sure
    * arguments are checked before methods of this class are
    * called. While some rudimentary checks are performed on the input,
    * the checks are best effort and when performance is an overriding
@@ -425,7 +425,7 @@
   /**
    * Create an exception reflecting that some of the input was

invalid

    *
- * Note: It is the resposibility of the caller to make
+ * Note: It is the responsibility of the caller to make
    * sure arguments are checked before the methods are called. While
    * some rudimentary checks are performed on the input, the checks
    * are best effort and when performance is an overriding priority,
@@ -601,7 +601,7 @@
    * aligned for all value types.  Dispose of this memory by calling
{@link
    * #freeMemory}, or resize it with {@link #reallocateMemory}.
    *
- * Note: It is the resposibility of the caller to make
+ * Note: It is the responsibility of the caller to make
    * sure arguments are checked before the methods are called. While
    * some rudimentary checks are performed on the input, the checks
    * are best effort and when performance is an overriding priority,
@@ -657,7 +657,7 @@
    * #reallocateMemory}.  The address passed to this method may be

null,

in
    * which case an allocation will be performed.
    *
- * Note: It is the resposibility of the caller to make
+ * Note: It is the responsibility of the caller to make
    * sure arguments are checked before the methods are called. While
    * some rudimentary checks are performed on the input, the checks
    * are best effort and when performance is an overriding priority,
@@ -719,7 +719,7 @@
    * If the effective address and length are (resp.) even modulo 4

or 2,

    * the stores take place in units of 'int' or 'short'.
    *
- * Note: It is the resposibility of the caller to make
+ * Note: It is the responsibility of the caller to make
    * sure arguments are checked before the methods are called. While
    * some rudimentary checks are performed on the input, the checks
    * are best effort and when performance is an overriding priority,
@@ -781,7 +781,7 @@
    * If the effective addresses and length are (resp.) even modulo 4

or

2,
    * the transfer takes place in units of 'int' or 'short'.
    *
- * Note: It is the resposibility of the caller to make
+ * Note: It is the responsibility of the caller to make
    * sure arguments are checked before the methods are called. While
    * some rudimentary checks are performed on the input, the checks
    * are best effort and when performance is an overriding priority,
@@ -842,7 +842,7 @@
    * as discussed in {@link #getInt(Object,long)}.  When the object
reference is null,
    * the offset supplies an absolute base address.
    *
- * Note: It is the resposibility of the caller to make
+ * Note: It is the respon

Re: JDK-8237818: Typo in Unsafe: resposibility

[Yasumasa Suenaga]

Pushed: http://hg.openjdk.java.net/jdk/jdk/rev/f3f66f9e98ee

Yasumasa


On 2020/02/17 10:38, Yasumasa Suenaga wrote:

Hi David,

On 2020/02/17 8:10, David Holmes wrote:

On 17/02/2020 12:40 am, Aya Ebata wrote:

Hi,

I sent OCA today. So it hasn't been approved yet. I'm waiting for approval.


For this trivial contribution an OCA is a not required, so this can be 
sponsored before the OCA goes through processing.


I didn't know that!
I hope it is described in "How to contribute" page [1].

Anyway, I will push Aya's change to jdk/jdk.


Thanks,

Yasumasa


[1] https://openjdk.java.net/contribute/



Cheers,
David


regards,
Aya Ebata


2020年2月16日(日) 16:49 Yasumasa Suenaga :


Hi Aya,

It looks good to me.
BTW, have you signed to OCA? If so, I will sponsor you.


Thanks,

Yasumasa


On 2020/02/16 16:42, 江畑 彩 wrote:

Hi,

I fixed typo in s.m.Unsafe and j.i.Unsafe. The changes are below.
Could you sponsor this, please.
https://bugs.openjdk.java.net/browse/JDK-8237818

regards,
Aya Ebata



diff --git a/src/java.base/share/classes/jdk/internal/misc/Unsafe.java
b/src/java.base/share/classes/jdk/internal/misc/Unsafe.java
--- a/src/java.base/share/classes/jdk/internal/misc/Unsafe.java
+++ b/src/java.base/share/classes/jdk/internal/misc/Unsafe.java
@@ -40,7 +40,7 @@
    * Although the class and all methods are public, use of this class is
    * limited because only trusted code can obtain instances of it.
    *
- * Note: It is the resposibility of the caller to make sure
+ * Note: It is the responsibility of the caller to make sure
    * arguments are checked before methods of this class are
    * called. While some rudimentary checks are performed on the input,
    * the checks are best effort and when performance is an overriding
@@ -425,7 +425,7 @@
   /**
    * Create an exception reflecting that some of the input was

invalid

    *
- * Note: It is the resposibility of the caller to make
+ * Note: It is the responsibility of the caller to make
    * sure arguments are checked before the methods are called. While
    * some rudimentary checks are performed on the input, the checks
    * are best effort and when performance is an overriding priority,
@@ -601,7 +601,7 @@
    * aligned for all value types.  Dispose of this memory by calling
{@link
    * #freeMemory}, or resize it with {@link #reallocateMemory}.
    *
- * Note: It is the resposibility of the caller to make
+ * Note: It is the responsibility of the caller to make
    * sure arguments are checked before the methods are called. While
    * some rudimentary checks are performed on the input, the checks
    * are best effort and when performance is an overriding priority,
@@ -657,7 +657,7 @@
    * #reallocateMemory}.  The address passed to this method may be

null,

in
    * which case an allocation will be performed.
    *
- * Note: It is the resposibility of the caller to make
+ * Note: It is the responsibility of the caller to make
    * sure arguments are checked before the methods are called. While
    * some rudimentary checks are performed on the input, the checks
    * are best effort and when performance is an overriding priority,
@@ -719,7 +719,7 @@
    * If the effective address and length are (resp.) even modulo 4

or 2,

    * the stores take place in units of 'int' or 'short'.
    *
- * Note: It is the resposibility of the caller to make
+ * Note: It is the responsibility of the caller to make
    * sure arguments are checked before the methods are called. While
    * some rudimentary checks are performed on the input, the checks
    * are best effort and when performance is an overriding priority,
@@ -781,7 +781,7 @@
    * If the effective addresses and length are (resp.) even modulo 4

or

2,
    * the transfer takes place in units of 'int' or 'short'.
    *
- * Note: It is the resposibility of the caller to make
+ * Note: It is the responsibility of the caller to make
    * sure arguments are checked before the methods are called. While
    * some rudimentary checks are performed on the input, the checks
    * are best effort and when performance is an overriding priority,
@@ -842,7 +842,7 @@
    * as discussed in {@link #getInt(Object,long)}.  When the object
reference is null,
    * the offset supplies an absolute base address.
    *
- * Note: It is the resposibility of the caller to make
+ * Note: It is the responsibility of the caller to make
    * sure arguments are checked before the methods are called. While
    * some rudimentary checks are performed on the input, the checks
    * are best effort and when performance is an overriding priority,
@@ -901,7 +901,7 @@
    * #allocateMemory} or {@link #reallocateMemory}.  The address

passed

to
    * this method may be null, in which case no action is taken.
    *
- 

Re: JDK-8237818: Typo in Unsafe: resposibility

[Yasumasa Suenaga]

Hi David,

On 2020/02/17 8:10, David Holmes wrote:

On 17/02/2020 12:40 am, Aya Ebata wrote:

Hi,

I sent OCA today. So it hasn't been approved yet. I'm waiting for approval.


For this trivial contribution an OCA is a not required, so this can be 
sponsored before the OCA goes through processing.


I didn't know that!
I hope it is described in "How to contribute" page [1].

Anyway, I will push Aya's change to jdk/jdk.


Thanks,

Yasumasa


[1] https://openjdk.java.net/contribute/



Cheers,
David


regards,
Aya Ebata


2020年2月16日(日) 16:49 Yasumasa Suenaga :


Hi Aya,

It looks good to me.
BTW, have you signed to OCA? If so, I will sponsor you.


Thanks,

Yasumasa


On 2020/02/16 16:42, 江畑 彩 wrote:

Hi,

I fixed typo in s.m.Unsafe and j.i.Unsafe. The changes are below.
Could you sponsor this, please.
https://bugs.openjdk.java.net/browse/JDK-8237818

regards,
Aya Ebata



diff --git a/src/java.base/share/classes/jdk/internal/misc/Unsafe.java
b/src/java.base/share/classes/jdk/internal/misc/Unsafe.java
--- a/src/java.base/share/classes/jdk/internal/misc/Unsafe.java
+++ b/src/java.base/share/classes/jdk/internal/misc/Unsafe.java
@@ -40,7 +40,7 @@
    * Although the class and all methods are public, use of this class is
    * limited because only trusted code can obtain instances of it.
    *
- * Note: It is the resposibility of the caller to make sure
+ * Note: It is the responsibility of the caller to make sure
    * arguments are checked before methods of this class are
    * called. While some rudimentary checks are performed on the input,
    * the checks are best effort and when performance is an overriding
@@ -425,7 +425,7 @@
   /**
    * Create an exception reflecting that some of the input was

invalid

    *
- * Note: It is the resposibility of the caller to make
+ * Note: It is the responsibility of the caller to make
    * sure arguments are checked before the methods are called. While
    * some rudimentary checks are performed on the input, the checks
    * are best effort and when performance is an overriding priority,
@@ -601,7 +601,7 @@
    * aligned for all value types.  Dispose of this memory by calling
{@link
    * #freeMemory}, or resize it with {@link #reallocateMemory}.
    *
- * Note: It is the resposibility of the caller to make
+ * Note: It is the responsibility of the caller to make
    * sure arguments are checked before the methods are called. While
    * some rudimentary checks are performed on the input, the checks
    * are best effort and when performance is an overriding priority,
@@ -657,7 +657,7 @@
    * #reallocateMemory}.  The address passed to this method may be

null,

in
    * which case an allocation will be performed.
    *
- * Note: It is the resposibility of the caller to make
+ * Note: It is the responsibility of the caller to make
    * sure arguments are checked before the methods are called. While
    * some rudimentary checks are performed on the input, the checks
    * are best effort and when performance is an overriding priority,
@@ -719,7 +719,7 @@
    * If the effective address and length are (resp.) even modulo 4

or 2,

    * the stores take place in units of 'int' or 'short'.
    *
- * Note: It is the resposibility of the caller to make
+ * Note: It is the responsibility of the caller to make
    * sure arguments are checked before the methods are called. While
    * some rudimentary checks are performed on the input, the checks
    * are best effort and when performance is an overriding priority,
@@ -781,7 +781,7 @@
    * If the effective addresses and length are (resp.) even modulo 4

or

2,
    * the transfer takes place in units of 'int' or 'short'.
    *
- * Note: It is the resposibility of the caller to make
+ * Note: It is the responsibility of the caller to make
    * sure arguments are checked before the methods are called. While
    * some rudimentary checks are performed on the input, the checks
    * are best effort and when performance is an overriding priority,
@@ -842,7 +842,7 @@
    * as discussed in {@link #getInt(Object,long)}.  When the object
reference is null,
    * the offset supplies an absolute base address.
    *
- * Note: It is the resposibility of the caller to make
+ * Note: It is the responsibility of the caller to make
    * sure arguments are checked before the methods are called. While
    * some rudimentary checks are performed on the input, the checks
    * are best effort and when performance is an overriding priority,
@@ -901,7 +901,7 @@
    * #allocateMemory} or {@link #reallocateMemory}.  The address

passed

to
    * this method may be null, in which case no action is taken.
    *
- * Note: It is the resposibility of the caller to make
+ * Note: It is the responsibility of the caller to make

Re: JDK-8237818: Typo in Unsafe: resposibility

[Yasumasa Suenaga]

Hi Aya,

It looks good to me.
BTW, have you signed to OCA? If so, I will sponsor you.


Thanks,

Yasumasa


On 2020/02/16 16:42, 江畑 彩 wrote:

Hi,

I fixed typo in s.m.Unsafe and j.i.Unsafe. The changes are below.
Could you sponsor this, please.
https://bugs.openjdk.java.net/browse/JDK-8237818

regards,
Aya Ebata



diff --git a/src/java.base/share/classes/jdk/internal/misc/Unsafe.java
b/src/java.base/share/classes/jdk/internal/misc/Unsafe.java
--- a/src/java.base/share/classes/jdk/internal/misc/Unsafe.java
+++ b/src/java.base/share/classes/jdk/internal/misc/Unsafe.java
@@ -40,7 +40,7 @@
   * Although the class and all methods are public, use of this class is
   * limited because only trusted code can obtain instances of it.
   *
- * Note: It is the resposibility of the caller to make sure
+ * Note: It is the responsibility of the caller to make sure
   * arguments are checked before methods of this class are
   * called. While some rudimentary checks are performed on the input,
   * the checks are best effort and when performance is an overriding
@@ -425,7 +425,7 @@
  /**
   * Create an exception reflecting that some of the input was invalid
   *
- * Note: It is the resposibility of the caller to make
+ * Note: It is the responsibility of the caller to make
   * sure arguments are checked before the methods are called. While
   * some rudimentary checks are performed on the input, the checks
   * are best effort and when performance is an overriding priority,
@@ -601,7 +601,7 @@
   * aligned for all value types.  Dispose of this memory by calling
{@link
   * #freeMemory}, or resize it with {@link #reallocateMemory}.
   *
- * Note: It is the resposibility of the caller to make
+ * Note: It is the responsibility of the caller to make
   * sure arguments are checked before the methods are called. While
   * some rudimentary checks are performed on the input, the checks
   * are best effort and when performance is an overriding priority,
@@ -657,7 +657,7 @@
   * #reallocateMemory}.  The address passed to this method may be null,
in
   * which case an allocation will be performed.
   *
- * Note: It is the resposibility of the caller to make
+ * Note: It is the responsibility of the caller to make
   * sure arguments are checked before the methods are called. While
   * some rudimentary checks are performed on the input, the checks
   * are best effort and when performance is an overriding priority,
@@ -719,7 +719,7 @@
   * If the effective address and length are (resp.) even modulo 4 or 2,
   * the stores take place in units of 'int' or 'short'.
   *
- * Note: It is the resposibility of the caller to make
+ * Note: It is the responsibility of the caller to make
   * sure arguments are checked before the methods are called. While
   * some rudimentary checks are performed on the input, the checks
   * are best effort and when performance is an overriding priority,
@@ -781,7 +781,7 @@
   * If the effective addresses and length are (resp.) even modulo 4 or
2,
   * the transfer takes place in units of 'int' or 'short'.
   *
- * Note: It is the resposibility of the caller to make
+ * Note: It is the responsibility of the caller to make
   * sure arguments are checked before the methods are called. While
   * some rudimentary checks are performed on the input, the checks
   * are best effort and when performance is an overriding priority,
@@ -842,7 +842,7 @@
   * as discussed in {@link #getInt(Object,long)}.  When the object
reference is null,
   * the offset supplies an absolute base address.
   *
- * Note: It is the resposibility of the caller to make
+ * Note: It is the responsibility of the caller to make
   * sure arguments are checked before the methods are called. While
   * some rudimentary checks are performed on the input, the checks
   * are best effort and when performance is an overriding priority,
@@ -901,7 +901,7 @@
   * #allocateMemory} or {@link #reallocateMemory}.  The address passed
to
   * this method may be null, in which case no action is taken.
   *
- * Note: It is the resposibility of the caller to make
+ * Note: It is the responsibility of the caller to make
   * sure arguments are checked before the methods are called. While
   * some rudimentary checks are performed on the input, the checks
   * are best effort and when performance is an overriding priority,
diff --git a/src/jdk.unsupported/share/classes/sun/misc/Unsafe.java
b/src/jdk.unsupported/share/classes/sun/misc/Unsafe.java
--- a/src/jdk.unsupported/share/classes/sun/misc/Unsafe.java
+++ b/src/jdk.unsupported/share/classes/sun/misc/Unsafe.java
@@ -39,7 +39,7 @@
   * Although the class and all methods are public, use of this class is
   * limited because only trusted code can obtain instances of it.
   *
- * 

Re: RFR (trivial): 8235833: PosixPlatform.cpp should not include sysctl.h

[Yasumasa Suenaga]

Thanks Andrew!

Yasumasa


On 2019/12/19 18:07, Andrew Haley wrote:

On 12/17/19 11:32 PM, Yasumasa Suenaga wrote:

PING: Could you review this trivial change?


JBS: https://bugs.openjdk.java.net/browse/JDK-8235833
webrev: http://cr.openjdk.java.net/~ysuenaga/JDK-8235833/webrev.00/


It's OK.

Re: RFR (trivial): 8235833: PosixPlatform.cpp should not include sysctl.h

[Yasumasa Suenaga]

PING: Could you review this trivial change?


   JBS: https://bugs.openjdk.java.net/browse/JDK-8235833
   webrev: http://cr.openjdk.java.net/~ysuenaga/JDK-8235833/webrev.00/



Yasumasa


On 2019/12/12 22:23, Yasumasa Suenaga wrote:

Hi Andrew,

I can see same error whether --disable-precompiled-headers or not.
So I filed it to JBS and uploaded webrev.
Could you review it?

   JBS: https://bugs.openjdk.java.net/browse/JDK-8235833
   webrev: http://cr.openjdk.java.net/~ysuenaga/JDK-8235833/webrev.00/

It works fine on Fedora 31.
I've pushed it to submit repo. I believe it passes all tests.


Thanks,

Yasumasa


On 2019/12/12 19:25, Andrew Haley wrote:

On 12/12/19 9:31 AM, Yasumasa Suenaga wrote:

AFAICS sysctl.h provides `sysctl()` and some macros, but they are not used in 
PosixPlatform.cpp .
In fact, I did not see any error when I remove `#include ` from 
it.


You probably won't see an error unless you configure with
--disable-precompiled-headers. Please try that, and we can then
consider removing that #include.

Re: RFR (trivial): 8235833: PosixPlatform.cpp should not include sysctl.h

[Yasumasa Suenaga]

Thanks Alexey! and I wait Reviewer(s).

Yasumasa


On 2019/12/13 1:42, Alexey Semenyuk wrote:

Looks good.

- Alexey

On 12/12/2019 8:23 AM, Yasumasa Suenaga wrote:

Hi Andrew,

I can see same error whether --disable-precompiled-headers or not.
So I filed it to JBS and uploaded webrev.
Could you review it?

  JBS: https://bugs.openjdk.java.net/browse/JDK-8235833
  webrev: http://cr.openjdk.java.net/~ysuenaga/JDK-8235833/webrev.00/

It works fine on Fedora 31.
I've pushed it to submit repo. I believe it passes all tests.


Thanks,

Yasumasa


On 2019/12/12 19:25, Andrew Haley wrote:

On 12/12/19 9:31 AM, Yasumasa Suenaga wrote:

AFAICS sysctl.h provides `sysctl()` and some macros, but they are not used in 
PosixPlatform.cpp .
In fact, I did not see any error when I remove `#include ` from 
it.


You probably won't see an error unless you configure with
--disable-precompiled-headers. Please try that, and we can then
consider removing that #include.

RFR (trivial): 8235833: PosixPlatform.cpp should not include sysctl.h

[Yasumasa Suenaga]

Hi Andrew,

I can see same error whether --disable-precompiled-headers or not.
So I filed it to JBS and uploaded webrev.
Could you review it?

  JBS: https://bugs.openjdk.java.net/browse/JDK-8235833
  webrev: http://cr.openjdk.java.net/~ysuenaga/JDK-8235833/webrev.00/

It works fine on Fedora 31.
I've pushed it to submit repo. I believe it passes all tests.


Thanks,

Yasumasa


On 2019/12/12 19:25, Andrew Haley wrote:

On 12/12/19 9:31 AM, Yasumasa Suenaga wrote:

AFAICS sysctl.h provides `sysctl()` and some macros, but they are not used in 
PosixPlatform.cpp .
In fact, I did not see any error when I remove `#include ` from 
it.


You probably won't see an error unless you configure with
--disable-precompiled-headers. Please try that, and we can then
consider removing that #include.

Build warning in jpackage

[Yasumasa Suenaga]

Hi all,

I tried to build jdk/jdk on Fedora 31 x64, but I saw C++ compiler error as 
below:

```
In file included from 
/home/ysuenaga/OpenJDK/jdk/src/jdk.incubator.jpackage/unix/native/libapplauncher/PosixPlatform.cpp:36:
/usr/include/sys/sysctl.h:21:2: error: #warning "The  header is 
deprecated and will be removed." [-Werror=cpp]
   21 | #warning "The  header is deprecated and will be removed."
  |
```


AFAICS sysctl.h provides `sysctl()` and some macros, but they are not used in 
PosixPlatform.cpp .
In fact, I did not see any error when I remove `#include ` from 
it.

Is it bug?
If so, I will file it to JBS and will send review request.


Thanks,

Yasumasa

RFR: JDK-8164326: jrtfsviewer.js and jrtls.js does not work

[Yasumasa Suenaga]

Hi all,

This review request is related to [1].

jrtfsviewer.js and jrtls.js are not contained JDK/JRE binaries.
However, these tool might be used by JDK developers. So I think we should fix 
this issue.

I uploaded a webrev. Could you review it?

  http://cr.openjdk.java.net/~ysuenaga/JDK-8164326/webrev.00/


Thanks,

Yasumasa


[1] http://mail.openjdk.java.net/pipermail/core-libs-dev/2016-August/042995.html

Error at jrt*.js

[Yasumasa Suenaga]

Hi all,

I tried to run jrtfsviewer.js and jrtls.js . But they did not work as below:

$ jjs -fx jrtfsviewer.js
Exception in Application start method
Exception in thread "main" java.lang.RuntimeException: Exception in Application 
start method
at 
com.sun.javafx.application.LauncherImpl.launchApplication1(javafx.graphics@9-ea/LauncherImpl.java:897)
at 
com.sun.javafx.application.LauncherImpl.lambda$launchApplication$2(javafx.graphics@9-ea/LauncherImpl.java:188)
at java.lang.Thread.run(java.base@9-ea/Thread.java:843)
Caused by: java.lang.NullPointerException
at java.util.Objects.requireNonNull(java.base@9-ea/Objects.java:221)
at 
jdk.internal.jrtfs.JrtFileSystemProvider.newFileSystem(java.base@9-ea/JrtFileSystemProvider.java:104)
at 
java.nio.file.FileSystems.newFileSystem(java.base@9-ea/FileSystems.java:342)
at 
jdk.nashorn.internal.scripts.Script$Recompilation$22$3116$jrtfsviewer$cu1$restOf.getJrtFileSystem(jdk.scripting.nashorn.scripts/jrtfsviewer.js:103)
at 
jdk.nashorn.internal.scripts.Script$Recompilation$17$3748A$jrtfsviewer.start(jdk.scripting.nashorn.scripts/jrtfsviewer.js:109)
at 
jdk.nashorn.internal.scripts.Script$Recompilation$6$839A$\=fx\!bootstrap$cu1$restOf.start(jdk.scripting.nashorn.scripts/fx:bootstrap.js:57)
at 
jdk.nashorn.javaadapters.javafx_application_Application.start(Unknown Source)
at 
com.sun.javafx.application.LauncherImpl.lambda$launchApplication1$9(javafx.graphics@9-ea/LauncherImpl.java:843)
at 
com.sun.javafx.application.PlatformImpl.lambda$runAndWait$12(javafx.graphics@9-ea/PlatformImpl.java:452)
at 
com.sun.javafx.application.PlatformImpl.lambda$runLater$10(javafx.graphics@9-ea/PlatformImpl.java:421)
at java.security.AccessController.doPrivileged(java.base@9-ea/Native 
Method)
at 
com.sun.javafx.application.PlatformImpl.lambda$runLater$11(javafx.graphics@9-ea/PlatformImpl.java:420)
at 
com.sun.glass.ui.InvokeLaterDispatcher$Future.run(javafx.graphics@9-ea/InvokeLaterDispatcher.java:96)
at 
com.sun.glass.ui.win.WinApplication._runLoop(javafx.graphics@9-ea/Native Method)
at 
com.sun.glass.ui.win.WinApplication.lambda$runLoop$3(javafx.graphics@9-ea/WinApplication.java:189)
... 1 more

$ jjs jrtls.js
jrtls.js:37:1 Expected an operand but found *
 */
 ^


These scripts are not in JDK. I guess they are used for checking JDK/JRE 
modules by JDK developers, and they are no longer used.
So I'm not sure whether they should be fixed.

I think they can be fixed as below:

diff -r b60dcba6b4f9 
src/java.base/share/classes/jdk/internal/jrtfs/jrtfsviewer.js
--- a/src/java.base/share/classes/jdk/internal/jrtfs/jrtfsviewer.js Tue Aug 
16 09:57:50 2016 +0200
+++ b/src/java.base/share/classes/jdk/internal/jrtfs/jrtfsviewer.js Wed Aug 
18 20:41:25 2016 +0900
@@ -53,6 +53,7 @@
 var Files = Java.type("java.nio.file.Files");
 var System = Java.type("java.lang.System");
 var URI = Java.type("java.net.URI");
+var Collections = Java.type("java.util.Collections");
 
 // JavaFX classes used

 var StackPane = Java.type("javafx.scene.layout.StackPane");
@@ -100,7 +101,7 @@
 print("did you miss specifying jrt-fs.jar with -cp option?");
 usage();
 }
-return FileSystems.newFileSystem(uri, null, cls.classLoader);
+return FileSystems.newFileSystem(uri, Collections.emptyMap(), 
cls.classLoader);
 }
 }
 
diff -r b60dcba6b4f9 src/java.base/share/classes/jdk/internal/jrtfs/jrtls.js

--- a/src/java.base/share/classes/jdk/internal/jrtfs/jrtls.js   Tue Aug 16 
09:57:50 2016 +0200
+++ b/src/java.base/share/classes/jdk/internal/jrtfs/jrtls.js   Wed Aug 18 
20:41:25 2016 +0900
@@ -34,7 +34,6 @@
  * but also compiled and delivered as part of the jrtfs.jar to support access
  * to the jimage file provided by the shipped JDK by tools running on JDK 8.
  */
- */
 
 // classes used

 var Files = Java.type("java.nio.file.Files");


If this fix should be merged, I'll file it to JBS and upload webrev.
What do you think about it?


Thanks,

Yasumasa

Re: PING: RFR: JDK-4347142: Need method to set Password protection to Zip entries

[Yasumasa Suenaga]

Hi Andrew,

Thank you for your comment.

We will not modify ZIP file format, so I think we do not need to negotiate
for ZIP file format.

We want to propose to JEP as below:


  1. We do not need to modify current java.util.zip implementation for
 unencrypted data.

  2. We think that we can apply AE-1 and AE-2 format from WinZip [1].
 This format uses extra data field in standard ZIP file format for
 encryption header.
 Thus we can handle encrypted/unencrypted ZIP archive with keeping
 compatibility.

  3. For extensibility, the encryption engine should be pluggable like
 Java Cryptography Architecture (JCA).
 If so, any Java developer can add custom encryption engine.
 (e.g. traditional ZIP encryption engine)


Thanks,

Yasumasa


[1] http://www.winzip.com/aes_info.htm


On 2016/05/31 5:17, Andrew Haley wrote:

On 30/05/16 13:44, Yasumasa Suenaga wrote:

We had aimed to handle *traditional* zip encryption at first.
However, we also want to handle *strong* zip encryption. (e.g. AES)


OpenJDK does not control the format of a zip file.  To make
such a change we'd have to negotiate with the other users of
the .ZIP format.

Andrew.

Re: PING: RFR: JDK-4347142: Need method to set Password protection to Zip entries

[Yasumasa Suenaga]

Hi Mark,

I also agree to you.
We had aimed to handle *traditional* zip encryption at first.
However, we also want to handle *strong* zip encryption. (e.g. AES)

As discussion in this mail thread, I'm sure that Java developers
need this feature. So I want to move forward with this proposal.

For example, I hope that JDK supports plugins to add encryption
engines for zip archive. If JDK provides this enhancement, we are
happy because we can focus to implement the zip encryption engine
when we want another encryption algorithms.

For that reason, I would like to propose a new JEP if it is appropriate.

According to JEP 1 [1], OpenJDK committer can propose new JEP.
If so, I will make a JEP in accordance with JEP 2 [2], and will send it
to core-libs-dev for review.
(I'm OpenJDK committer (ysuenaga), so I think I can send JEP request.)


If you have better idea, I'm glad to hear your advice.


Thanks,

Yasumasa


[1] http://openjdk.java.net/jeps/1
[2] http://openjdk.java.net/jeps/2


On 2016/04/25 17:49, KUBOTA Yuji wrote:

2016-04-20 7:57 GMT+09:00  :

I have to agree.  I don't think it makes sense to add a known-vulnerable
encryption algorithm to the JDK.  It might work perfectly well for one
use case but it will eventually be used by someone who doesn't take the
time to understand it, assumes that it provides strong encryption when
it doesn't, gets burned, and then blames Java.


Yes, Mark. We never hope to make OpenJDK be blamed with our proposal.

Mark, I am glad if you are interested in this proposal if using any strong
encryption algorithm like Sherman's implementation (AES encryption).
If so, we hope to continue discussion the better way to fill our needs
and given comments for improving OpenJDK.

Thanks,
Yuji

Re: RFR: JDK-8152690: main thread does not have native thread name

[Yasumasa Suenaga]

Hi Kumar, David,


simply to set a name which seems to be useful only for folks trying to debug
the VM ???  besides ps and few other OS related utils it
has no visibility of value to the JRE or the JDK APIs.


I want to check CPU time through ps command on Linux.
Usually, I get "ps -eLf" and nid in threaddump to check
which thread spends much CPU time.

If I can get thread name from output of ps command,
I can check it rapidly.

So I want to set "main" name at least.


Thanks,

Yasumasa


On 2016/04/26 23:21, Kumar Srinivasan wrote:


On 4/26/2016 6:08 AM, David Holmes wrote:

On 26/04/2016 10:54 PM, Yasumasa Suenaga wrote:

Hi David,

For this issue, I think we can approach as following:


   1. Export new JVM function to set native thread name
It can avoid JNI call and can handle char* value.
However this plan has been rejected.

   2. Call uncaught exception handler from Launcher
We have to clear (process) any pending exception before
DetachCurrentThread() call. (not documented?)
--

so note that we are potentially calling DetachCurrentThread with an
exception pending - which is prohibited by JNI**



**JNI spec needs to be modified here.

--
So we can process pending exception through uncaught
exception handler.
However, this plan has been rejected.

   3. Do not set DestroyJavaVM name when exception is occurred
It disrupts consistency for native thread name.

   4. Attach to JVM to set native thread name for DestroyJavaVM
It does not look good.


If all of them are not accepted, I guess that it is difficult.
Do you have any other idea?


Sorry I don't. The basic idea of having the launcher set the native thread name is fine, 
but the mechanism to do that has been problematic. The "real" solution (ie one 
that other applications hosting the jvm would need to use) is for the launcher to 
duplicate the per-platform logic for os::set_native_thread_name - but that was 
undesirable. Instead we have tried to avoid that by finding a way to use whatever is 
already in the JVM - but adding a new JVM interface to expose it directly is not ideal; 
and hooking into the java.lang.Thread code to avoid that has run into these other 
problems. I really don't want to take the logic for uncaught exception handling that is 
in Thread::exit and duplicate it in the launcher.

The effort and disruption here really does not justify the "it would be nice to set 
the native thread name for the main thread" objective.

I never expected this to be as problematic as it has turned out.


I agree with Holmes-san, I think this needlessly complicates
the launcher, more than I would like!, ie  simply to set a name
which seems to be useful only for folks trying to debug
the VM ???  besides ps and few other OS related utils it
has no visibility of value to the JRE or the JDK APIs.

Thanks

Kumar



Sorry.

David
-




Thanks,

Yasumasa


On 2016/04/26 18:35, David Holmes wrote:

On 26/04/2016 7:22 PM, Yasumasa Suenaga wrote:

Hi David,


I thought about being able to save/restore the original pending
exception but could not see a simple way to restore it - ie just by
poking it back into the thread's pending exception field. The problem
with using env->Throw is that it acts like the initial throwing of the
exception and will have a number of side-effects that then get doubled
up:
- logging statements (UL and Event logging)
- OOM counting


Thanks, I understood.


so note that we are potentially calling DetachCurrentThread with an
exception pending - which is prohibited by JNI**, but which we
actually rely on for desired operation as DetachCurrentThread calls
thread->exit() which performs uncaught exception handling (ie it
prints the exception message and stacktrace) and then clears the
exception! (Hence DestroyJavaVM is not called with any pending
exceptions.)


I think we can call uncaught exception handler before calling
DestroyJavaVM().
I added it in new webrev for jdk:

http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.08/hotspot/
http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.08/jdk/

DispatchUncaughtException() in java.c emulates uncaught exception
handler
call in JavaThread::exit().
I think this patch can provide the solution for our issue.

Could you check it?


Sorry - this is getting far too disruptive. I do not support changing
the way the main thread behaves upon completion, just because we want
to set a native thread name.

David
-



Thanks,

Yasumasa


On 2016/04/26 14:16, David Holmes wrote:

On 26/04/2016 1:11 PM, Yasumasa Suenaga wrote:

Hi David, Kumar,

I think that we should evacuate original exception before
DestroyJavaVM
thread name is set.

http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.07/hotspot/
http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.07/jdk/

I added it to LEAVE macro in new webrev.


BTW: in the LEAVE macro, stylistically all the code

Re: RFR: JDK-8152690: main thread does not have native thread name

[Yasumasa Suenaga]

Hi David,

For this issue, I think we can approach as following:


  1. Export new JVM function to set native thread name
   It can avoid JNI call and can handle char* value.
   However this plan has been rejected.

  2. Call uncaught exception handler from Launcher
   We have to clear (process) any pending exception before
   DetachCurrentThread() call. (not documented?)
--

so note that we are potentially calling DetachCurrentThread with an
exception pending - which is prohibited by JNI**



**JNI spec needs to be modified here.

--
   So we can process pending exception through uncaught
   exception handler.
   However, this plan has been rejected.

  3. Do not set DestroyJavaVM name when exception is occurred
   It disrupts consistency for native thread name.

  4. Attach to JVM to set native thread name for DestroyJavaVM
   It does not look good.


If all of them are not accepted, I guess that it is difficult.
Do you have any other idea?


Thanks,

Yasumasa


On 2016/04/26 18:35, David Holmes wrote:

On 26/04/2016 7:22 PM, Yasumasa Suenaga wrote:

Hi David,


I thought about being able to save/restore the original pending
exception but could not see a simple way to restore it - ie just by
poking it back into the thread's pending exception field. The problem
with using env->Throw is that it acts like the initial throwing of the
exception and will have a number of side-effects that then get doubled
up:
- logging statements (UL and Event logging)
- OOM counting


Thanks, I understood.


so note that we are potentially calling DetachCurrentThread with an
exception pending - which is prohibited by JNI**, but which we
actually rely on for desired operation as DetachCurrentThread calls
thread->exit() which performs uncaught exception handling (ie it
prints the exception message and stacktrace) and then clears the
exception! (Hence DestroyJavaVM is not called with any pending
exceptions.)


I think we can call uncaught exception handler before calling
DestroyJavaVM().
I added it in new webrev for jdk:

   http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.08/hotspot/
   http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.08/jdk/

DispatchUncaughtException() in java.c emulates uncaught exception handler
call in JavaThread::exit().
I think this patch can provide the solution for our issue.

Could you check it?


Sorry - this is getting far too disruptive. I do not support changing the way 
the main thread behaves upon completion, just because we want to set a native 
thread name.

David
-



Thanks,

Yasumasa


On 2016/04/26 14:16, David Holmes wrote:

On 26/04/2016 1:11 PM, Yasumasa Suenaga wrote:

Hi David, Kumar,

I think that we should evacuate original exception before DestroyJavaVM
thread name is set.

   http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.07/hotspot/
   http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.07/jdk/

I added it to LEAVE macro in new webrev.


BTW: in the LEAVE macro, stylistically all the code should be in the
do { } while(false); loop. I overlooked that initially.


I tested it with following code. It works fine.

-
public void main(String[] args){
   throw new RuntimeException("test");
}
-

What do you think about it?


I thought about being able to save/restore the original pending
exception but could not see a simple way to restore it - ie just by
poking it back into the thread's pending exception field. The problem
with using env->Throw is that it acts like the initial throwing of the
exception and will have a number of side-effects that then get doubled
up:
- logging statements (UL and Event logging)
- OOM counting

I'm not sure whether that is acceptable or not

That aside you should check if orig_throwable is non-null before
clearing to avoid an unnecessary JNI call.

Also "Resume original exception" -> "Restore any original exception"

Thanks,
David
-



Thanks,

Yasumasa


On 2016/04/26 11:16, David Holmes wrote:

Hi Yasumasa, Kumar,

Turns out this change breaks the behaviour of the launcher in the case
that main() completes by throwing an exception.

What we have in the launcher is:

(*env)->CallStaticVoidMethod(env, mainClass, mainID, mainArgs);
ret = (*env)->ExceptionOccurred(env) == NULL ? 0 : 1;
LEAVE();

where LEAVE would have expanded to:

if ((*vm)->DetachCurrentThread(vm) != JNI_OK) { \
JLI_ReportErrorMessage(JVM_ERROR2); \
ret = 1; \
} \
if (JNI_TRUE) { \
(*vm)->DestroyJavaVM(vm); \
return ret; \
} \

so note that we are potentially calling DetachCurrentThread with an
exception pending - which is prohibited by JNI**, but which we
actually rely on for desired operation as DetachCurrentThread calls
thread->exit() which performs uncaught exception handling (ie it
prints the exception message and stacktrace) and then

Re: RFR: JDK-8152690: main thread does not have native thread name

[Yasumasa Suenaga]

Hi David,


I thought about being able to save/restore the original pending exception but 
could not see a simple way to restore it - ie just by poking it back into the 
thread's pending exception field. The problem with using env->Throw is that it 
acts like the initial throwing of the exception and will have a number of 
side-effects that then get doubled up:
- logging statements (UL and Event logging)
- OOM counting


Thanks, I understood.


so note that we are potentially calling DetachCurrentThread with an
exception pending - which is prohibited by JNI**, but which we
actually rely on for desired operation as DetachCurrentThread calls
thread->exit() which performs uncaught exception handling (ie it
prints the exception message and stacktrace) and then clears the
exception! (Hence DestroyJavaVM is not called with any pending
exceptions.)


I think we can call uncaught exception handler before calling DestroyJavaVM().
I added it in new webrev for jdk:

  http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.08/hotspot/
  http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.08/jdk/

DispatchUncaughtException() in java.c emulates uncaught exception handler
call in JavaThread::exit().
I think this patch can provide the solution for our issue.

Could you check it?


Thanks,

Yasumasa


On 2016/04/26 14:16, David Holmes wrote:

On 26/04/2016 1:11 PM, Yasumasa Suenaga wrote:

Hi David, Kumar,

I think that we should evacuate original exception before DestroyJavaVM
thread name is set.

   http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.07/hotspot/
   http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.07/jdk/

I added it to LEAVE macro in new webrev.


BTW: in the LEAVE macro, stylistically all the code should be in the do { } 
while(false); loop. I overlooked that initially.


I tested it with following code. It works fine.

-
public void main(String[] args){
   throw new RuntimeException("test");
}
-

What do you think about it?


I thought about being able to save/restore the original pending exception but 
could not see a simple way to restore it - ie just by poking it back into the 
thread's pending exception field. The problem with using env->Throw is that it 
acts like the initial throwing of the exception and will have a number of 
side-effects that then get doubled up:
- logging statements (UL and Event logging)
- OOM counting

I'm not sure whether that is acceptable or not

That aside you should check if orig_throwable is non-null before clearing to 
avoid an unnecessary JNI call.

Also "Resume original exception" -> "Restore any original exception"

Thanks,
David
-



Thanks,

Yasumasa


On 2016/04/26 11:16, David Holmes wrote:

Hi Yasumasa, Kumar,

Turns out this change breaks the behaviour of the launcher in the case
that main() completes by throwing an exception.

What we have in the launcher is:

(*env)->CallStaticVoidMethod(env, mainClass, mainID, mainArgs);
ret = (*env)->ExceptionOccurred(env) == NULL ? 0 : 1;
LEAVE();

where LEAVE would have expanded to:

if ((*vm)->DetachCurrentThread(vm) != JNI_OK) { \
JLI_ReportErrorMessage(JVM_ERROR2); \
ret = 1; \
} \
if (JNI_TRUE) { \
(*vm)->DestroyJavaVM(vm); \
return ret; \
} \

so note that we are potentially calling DetachCurrentThread with an
exception pending - which is prohibited by JNI**, but which we
actually rely on for desired operation as DetachCurrentThread calls
thread->exit() which performs uncaught exception handling (ie it
prints the exception message and stacktrace) and then clears the
exception! (Hence DestroyJavaVM is not called with any pending
exceptions.)

**JNI spec needs to be modified here.

With the current change we have now inserted the following at the
start of LEAVE:

SetNativeThreadName(env, "DestroyJavaVM");  \
if ((*env)->ExceptionOccurred(env)) { \
(*env)->ExceptionClear(env);   \
} \

this has two unintended consequences:

1. SetNativeThreadName itself calls a number of JNI methods, with the
exception pending - which is not permitted. So straight away where we
have:

   NULL_CHECK(cls = FindBootStrapClass(env, "java/lang/Thread"));

FindBootStrapClass calls JVM_FindClassFromBootLoader, which make calls
using the VM's CHECK_NULL macro - which checks for a pending exception
(which it finds) and returns NULL. So the jli NULL_CHECK macro then
reports a JNI error:

Error: A JNI error has occurred, please check your installation and
try again


2. There is no longer an exception from main() for DetachCurrentThread
to report, so we exit with a return code of 1 as required, but don't
report the exception message/stacktrace.

I don't see a reasonable solution for this other than abandoning the
attempt to change the name from "main" to &q

Re: RFR: JDK-8152690: main thread does not have native thread name

[Yasumasa Suenaga]

Hi David, Kumar,

I think that we should evacuate original exception before DestroyJavaVM
thread name is set.

  http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.07/hotspot/
  http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.07/jdk/

I added it to LEAVE macro in new webrev.
I tested it with following code. It works fine.

-
public void main(String[] args){
  throw new RuntimeException("test");
}
-

What do you think about it?


Thanks,

Yasumasa


On 2016/04/26 11:16, David Holmes wrote:

Hi Yasumasa, Kumar,

Turns out this change breaks the behaviour of the launcher in the case that 
main() completes by throwing an exception.

What we have in the launcher is:

(*env)->CallStaticVoidMethod(env, mainClass, mainID, mainArgs);
ret = (*env)->ExceptionOccurred(env) == NULL ? 0 : 1;
LEAVE();

where LEAVE would have expanded to:

if ((*vm)->DetachCurrentThread(vm) != JNI_OK) { \
JLI_ReportErrorMessage(JVM_ERROR2); \
ret = 1; \
} \
if (JNI_TRUE) { \
(*vm)->DestroyJavaVM(vm); \
return ret; \
} \

so note that we are potentially calling DetachCurrentThread with an exception 
pending - which is prohibited by JNI**, but which we actually rely on for desired 
operation as DetachCurrentThread calls thread->exit() which performs uncaught 
exception handling (ie it prints the exception message and stacktrace) and then 
clears the exception! (Hence DestroyJavaVM is not called with any pending 
exceptions.)

**JNI spec needs to be modified here.

With the current change we have now inserted the following at the start of 
LEAVE:

SetNativeThreadName(env, "DestroyJavaVM");  \
if ((*env)->ExceptionOccurred(env)) { \
(*env)->ExceptionClear(env);   \
} \

this has two unintended consequences:

1. SetNativeThreadName itself calls a number of JNI methods, with the exception 
pending - which is not permitted. So straight away where we have:

   NULL_CHECK(cls = FindBootStrapClass(env, "java/lang/Thread"));

FindBootStrapClass calls JVM_FindClassFromBootLoader, which make calls using 
the VM's CHECK_NULL macro - which checks for a pending exception (which it 
finds) and returns NULL. So the jli NULL_CHECK macro then reports a JNI error:

Error: A JNI error has occurred, please check your installation and try again


2. There is no longer an exception from main() for DetachCurrentThread to 
report, so we exit with a return code of 1 as required, but don't report the 
exception message/stacktrace.

I don't see a reasonable solution for this other than abandoning the attempt to change the name 
from "main" to "DestroyJavaVM" - which if we can't do, I question the utility 
of setting the name in the first place (while it might be useful in some circumstances [when main() 
is running] it will cause confusion in others [when main() has exited]).

David
-

On 25/04/2016 3:47 PM, David Holmes wrote:

Looks good to me.

I'll sponsor this "tomorrow".

Thanks,
David

On 23/04/2016 11:24 PM, Yasumasa Suenaga wrote:

Hi Kumar,

Thank you for your comment!
I've fixed them and uploaded new webrev. Could you review again?

   http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.06/hotspot/
   http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.06/jdk/


Thanks,

Yasumasa


On 2016/04/23 1:14, Kumar Srinivasan wrote:


Also a couple of minor suggestions:

- * Set native thread name as possible.
+ * Set native thread name if possible.


+  /*
-   * We can clear pending exception because exception at this point
-   * is not critical.
+   */

+  /*
+   * Clear non critical pending exceptions at this point.
+   */

Thanks
Kumar


Hi,

This is in the wrong place:

1715 if ((*env)->ExceptionOccurred(env)) {
1716   /*
1717* We can clear pending exception because exception at
this point
1718* is not critical.
1719*/
1720   (*env)->ExceptionClear(env);
1721 }

This above needs to be after
 391 SetNativeThreadName(env, "main");
 392

Here is why, supposing 1704 through 1711, returns a NULL,
but have also encountered an exception. In which case
the method SetNativeThreadName will return to the caller, as
if nothing has happened, because NULL_CHECK simply checks for NULL
and returns to the caller. This will cause the caller to enter
the VM with exceptions.

Kumar


On 4/22/2016 5:11 AM, Yasumasa Suenaga wrote:

Hi David,


I don't think we need to report the exception, but can just ignore
it. Either way we have to clear the exception before continuing.


I've fixed it in new webrev:

http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.05/hotspot/
http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.05/jdk/


Thanks,

Yasumasa


On 2016/04/22 15:33, David Holmes wrote:

On 22/04/2016 1:36

Re: RFR: JDK-8152690: main thread does not have native thread name

[Yasumasa Suenaga]

Hi Kumar,

Thank you for your comment!
I've fixed them and uploaded new webrev. Could you review again?

  http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.06/hotspot/
  http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.06/jdk/


Thanks,

Yasumasa


On 2016/04/23 1:14, Kumar Srinivasan wrote:


Also a couple of minor suggestions:

- * Set native thread name as possible.
+ * Set native thread name if possible.


+  /*
-   * We can clear pending exception because exception at this point
-   * is not critical.
+   */

+  /*
+   * Clear non critical pending exceptions at this point.
+   */

Thanks
Kumar


Hi,

This is in the wrong place:

1715 if ((*env)->ExceptionOccurred(env)) {
1716   /*
1717* We can clear pending exception because exception at this point
1718* is not critical.
1719*/
1720   (*env)->ExceptionClear(env);
1721 }

This above needs to be after
 391 SetNativeThreadName(env, "main");
 392

Here is why, supposing 1704 through 1711, returns a NULL,
but have also encountered an exception. In which case
the method SetNativeThreadName will return to the caller, as
if nothing has happened, because NULL_CHECK simply checks for NULL
and returns to the caller. This will cause the caller to enter
the VM with exceptions.

Kumar


On 4/22/2016 5:11 AM, Yasumasa Suenaga wrote:

Hi David,


I don't think we need to report the exception, but can just ignore it. Either 
way we have to clear the exception before continuing.


I've fixed it in new webrev:

http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.05/hotspot/
http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.05/jdk/


Thanks,

Yasumasa


On 2016/04/22 15:33, David Holmes wrote:

On 22/04/2016 1:36 PM, Yasumasa Suenaga wrote:

Hi all,

I have uploaded webrev.04 as below.
Could you review again?

 >  - hotspot:
 > http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.04/hotspot/


Looks fine.


 >
 >  - jdk:
 > http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.04/jdk/


As per private email (but repeated here on the record) in java.c:

715 if ((*env)->ExceptionOccurred(env)) {
1716   JLI_ReportExceptionDescription(env);
1717 }

I don't think we need to report the exception, but can just ignore it. Either 
way we have to clear the exception before continuing.

Thanks,
David


Thanks,

Yasumasa

2016/04/19 22:43 "Yasumasa Suenaga" <yasue...@gmail.com
<mailto:yasue...@gmail.com>>:
 >
 > Hi David,
 >
 > Thank you for your comment.
 > I uploaded new webrev. Could you review again?
 >
 >  - hotspot:
 > http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.04/hotspot/
 >
 >  - jdk:
 > http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.04/jdk/
 >
 >
 >> That aside I'm not sure why you do this so late in the process, I
would have done it immediately after here:
 >
 >
 > I think that native thread name ("main") should be set just before
 > main method call.
 > However, main thread is already started, so I moved it as you suggested.
 >
 >
 >> One thing I dislike about the current structure is that we have to
go from char* to java.lang.String to call setNativeName which then calls
JVM_SetNativeThreadName which converts the j.l.String back to a char* !
 >
 >
 > SoI proposed to export new JVM function to set native thread name with
 > const char *.
 >
 >
 > Thanks,
 >
 > Yasumasa
 >
 >
 >
 > On 2016/04/19 14:04, David Holmes wrote:
 >>
 >> Hi Yasumasa,
 >>
 >> Thanks for persevering with this to get it into the current form.
Sorry I haven't been able to do a detailed review until now.
 >>
 >> On 19/04/2016 9:28 AM, Yasumasa Suenaga wrote:
 >>>
 >>> Hi Gerard,
 >>>
 >>> 2016/04/19 3:14 "Gerard Ziemski" <gerard.ziem...@oracle.com
<mailto:gerard.ziem...@oracle.com>
 >>> <mailto:gerard.ziem...@oracle.com <mailto:gerard.ziem...@oracle.com>>>:
 >>>  >
 >>>  > hi Yasumasa,
 >>>  >
 >>>  > Nice work. I have 2 questions:
 >>>  >
 >>>  > 
 >>>  > File: java.c
 >>>  >
 >>>  > #1 Shouldn’t we be checking for “(*env)->ExceptionOccurred(env)”
 >>> after every single JNI call? In this example instead of NULL_CHECK,
 >>> should we be using CHECK_EXCEPTION_NULL_LEAVE macro?
 >>>
 >>> It is not critical if we encounter error at JNI function call because
 >>> we cannot set native thread name only.
 >>> So I think that we do not need to leave from launcher process.
 >>
 >>
 >> I agree we do not need to abort if an exception occurs (and in fact
I don't think an exception is even possible fro

Re: RFR: JDK-8152690: main thread does not have native thread name

[Yasumasa Suenaga]

Hi David,


I don't think we need to report the exception, but can just ignore it. Either 
way we have to clear the exception before continuing.


I've fixed it in new webrev:

  http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.05/hotspot/
  http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.05/jdk/


Thanks,

Yasumasa


On 2016/04/22 15:33, David Holmes wrote:

On 22/04/2016 1:36 PM, Yasumasa Suenaga wrote:

Hi all,

I have uploaded webrev.04 as below.
Could you review again?

 >  - hotspot:
 > http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.04/hotspot/


Looks fine.


 >
 >  - jdk:
 > http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.04/jdk/


As per private email (but repeated here on the record) in java.c:

715 if ((*env)->ExceptionOccurred(env)) {
1716   JLI_ReportExceptionDescription(env);
1717 }

I don't think we need to report the exception, but can just ignore it. Either 
way we have to clear the exception before continuing.

Thanks,
David


Thanks,

Yasumasa

2016/04/19 22:43 "Yasumasa Suenaga" <yasue...@gmail.com
<mailto:yasue...@gmail.com>>:
 >
 > Hi David,
 >
 > Thank you for your comment.
 > I uploaded new webrev. Could you review again?
 >
 >  - hotspot:
 > http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.04/hotspot/
 >
 >  - jdk:
 > http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.04/jdk/
 >
 >
 >> That aside I'm not sure why you do this so late in the process, I
would have done it immediately after here:
 >
 >
 > I think that native thread name ("main") should be set just before
 > main method call.
 > However, main thread is already started, so I moved it as you suggested.
 >
 >
 >> One thing I dislike about the current structure is that we have to
go from char* to java.lang.String to call setNativeName which then calls
JVM_SetNativeThreadName which converts the j.l.String back to a char* !
 >
 >
 > SoI proposed to export new JVM function to set native thread name with
 > const char *.
 >
 >
 > Thanks,
 >
 > Yasumasa
 >
 >
 >
 > On 2016/04/19 14:04, David Holmes wrote:
 >>
 >> Hi Yasumasa,
 >>
 >> Thanks for persevering with this to get it into the current form.
Sorry I haven't been able to do a detailed review until now.
 >>
 >> On 19/04/2016 9:28 AM, Yasumasa Suenaga wrote:
 >>>
 >>> Hi Gerard,
 >>>
 >>> 2016/04/19 3:14 "Gerard Ziemski" <gerard.ziem...@oracle.com
<mailto:gerard.ziem...@oracle.com>
 >>> <mailto:gerard.ziem...@oracle.com <mailto:gerard.ziem...@oracle.com>>>:
 >>>  >
 >>>  > hi Yasumasa,
 >>>  >
 >>>  > Nice work. I have 2 questions:
 >>>  >
 >>>  > 
 >>>  > File: java.c
 >>>  >
 >>>  > #1 Shouldn’t we be checking for “(*env)->ExceptionOccurred(env)”
 >>> after every single JNI call? In this example instead of NULL_CHECK,
 >>> should we be using CHECK_EXCEPTION_NULL_LEAVE macro?
 >>>
 >>> It is not critical if we encounter error at JNI function call  because
 >>> we cannot set native thread name only.
 >>> So I think that we do not need to leave from launcher process.
 >>
 >>
 >> I agree we do not need to abort if an exception occurs (and in fact
I don't think an exception is even possible from this code), but we
should ensure any pending exception is cleared before any futher JNI
calls might be made. Note that NULL_CHECK is already used extensively
throughout the launcher code - so if this usage is wrong then it is all
wrong! More on this code below ...
 >>
 >> Other comments:
 >>
 >> hotspot/src/share/vm/prims/jvm.cpp
 >>
 >> Please add a comment to the method now that you removed the internal
comment:
 >>
 >> // Sets the native thread name for a JavaThread. If specifically
 >> // requested JNI-attached threads can also have their native name set;
 >> // otherwise we do not modify JNI-attached threads as it may interfere
 >> // with the application that created them.
 >>
 >> ---
 >>
 >> jdk/src/java.base/share/classes/java/lang/Thread.java
 >>
 >> Please add the following comments:
 >>
 >> +// Don't modify JNI-attached threads
 >>   setNativeName(name, false);
 >>
 >> + // May be called directly via JNI or reflection (when permitted) to
 >> + // allow JNI-attached threads to set their native name
 >>   private native void setNativeName(String name, boolean
allowAttachedThread);
 >>
 >> ---
 >>
 >> jd/src/java.base/share/native/libjli/ja

Re: RFR: JDK-8152690: main thread does not have native thread name

[Yasumasa Suenaga]

Hi all,

I have uploaded webrev.04 as below.
Could you review again?

>  - hotspot:
> http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.04/hotspot/
>
>  - jdk:
> http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.04/jdk/

Thanks,

Yasumasa

2016/04/19 22:43 "Yasumasa Suenaga" <yasue...@gmail.com>:
>
> Hi David,
>
> Thank you for your comment.
> I uploaded new webrev. Could you review again?
>
>  - hotspot:
> http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.04/hotspot/
>
>  - jdk:
> http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.04/jdk/
>
>
>> That aside I'm not sure why you do this so late in the process, I would
have done it immediately after here:
>
>
> I think that native thread name ("main") should be set just before
> main method call.
> However, main thread is already started, so I moved it as you suggested.
>
>
>> One thing I dislike about the current structure is that we have to go
from char* to java.lang.String to call setNativeName which then calls
JVM_SetNativeThreadName which converts the j.l.String back to a char* !
>
>
> SoI proposed to export new JVM function to set native thread name with
> const char *.
>
>
> Thanks,
>
> Yasumasa
>
>
>
> On 2016/04/19 14:04, David Holmes wrote:
>>
>> Hi Yasumasa,
>>
>> Thanks for persevering with this to get it into the current form. Sorry
I haven't been able to do a detailed review until now.
>>
>> On 19/04/2016 9:28 AM, Yasumasa Suenaga wrote:
>>>
>>> Hi Gerard,
>>>
>>> 2016/04/19 3:14 "Gerard Ziemski" <gerard.ziem...@oracle.com
>>> <mailto:gerard.ziem...@oracle.com>>:
>>>  >
>>>  > hi Yasumasa,
>>>  >
>>>  > Nice work. I have 2 questions:
>>>  >
>>>  > 
>>>  > File: java.c
>>>  >
>>>  > #1 Shouldn’t we be checking for “(*env)->ExceptionOccurred(env)”
>>> after every single JNI call? In this example instead of NULL_CHECK,
>>> should we be using CHECK_EXCEPTION_NULL_LEAVE macro?
>>>
>>> It is not critical if we encounter error at JNI function call  because
>>> we cannot set native thread name only.
>>> So I think that we do not need to leave from launcher process.
>>
>>
>> I agree we do not need to abort if an exception occurs (and in fact I
don't think an exception is even possible from this code), but we should
ensure any pending exception is cleared before any futher JNI calls might
be made. Note that NULL_CHECK is already used extensively throughout the
launcher code - so if this usage is wrong then it is all wrong! More on
this code below ...
>>
>> Other comments:
>>
>> hotspot/src/share/vm/prims/jvm.cpp
>>
>> Please add a comment to the method now that you removed the internal
comment:
>>
>> // Sets the native thread name for a JavaThread. If specifically
>> // requested JNI-attached threads can also have their native name set;
>> // otherwise we do not modify JNI-attached threads as it may interfere
>> // with the application that created them.
>>
>> ---
>>
>> jdk/src/java.base/share/classes/java/lang/Thread.java
>>
>> Please add the following comments:
>>
>> +// Don't modify JNI-attached threads
>>   setNativeName(name, false);
>>
>> + // May be called directly via JNI or reflection (when permitted) to
>> + // allow JNI-attached threads to set their native name
>>   private native void setNativeName(String name, boolean
allowAttachedThread);
>>
>> ---
>>
>> jd/src/java.base/share/native/libjli/java.c
>>
>> 328 #define LEAVE() \
>> 329 SetNativeThreadName(env, "DestroyJavaVM"); \
>>
>> I was going to suggest this be set later, but realized we have to be
attached to do this and that happens inside DestroyJavaVM. :)
>>
>> + /* Set native thread name. */
>> + SetNativeThreadName(env, "main");
>>
>> The comment is redundant given the name of the method. That aside I'm
not sure why you do this so late in the process, I would have done it
immediately after here:
>>
>>   386 if (!InitializeJVM(, , )) {
>>   387 JLI_ReportErrorMessage(JVM_ERROR1);
>>   388 exit(1);
>>   389 }
>>   +   SetNativeThreadName(env, "main");
>>
>>
>> + /**
>> +  * Set native thread name as possible.
>> +  */
>>
>> Other than the as->if change I'm unclear where the "possible" bit comes
int

Re: RFR: JDK-8152690: main thread does not have native thread name

[Yasumasa Suenaga]

Well if it is hard then the jbs must be labelled so, noreg-hard.


I agree to label noreg-hard.


I concur.


I added noreg-hard to JBS.


Yasumasa


On 2016/04/21 16:01, David Holmes wrote:

On 21/04/2016 12:59 AM, Yasumasa Suenaga wrote:

Hi Kumar,


Isn't there a management API or something to enumerate the threads ?


Not that shows native thread names.

Existing launcher tests should of course be unaffected by this change.


On Linux, user apps can get native thread name through
pthread_getname_np(3).
However, this function is not called in hotspot and jdk.

So I think it is difficult to get native thread name in all platforms.



Well if it is hard then the jbs must be labelled so, noreg-hard.


I agree to label noreg-hard.


I concur.

Thanks,
David



Thanks,

Yasumasa


On 2016/04/20 23:26, Kumar Srinivasan wrote:


On 4/20/2016 6:06 AM, David Holmes wrote:

On 20/04/2016 10:58 PM, Kumar Srinivasan wrote:


Hello,

One more thing, what about a launcher regression test ?


What kind of regression test? I've manually visually inspected the
desired behaviour but a test for it is very problematic. AFAIK there
are no tests for setting the native thread name.


Isn't there a management API or something to enumerate the threads ?
I am more worried about some seemingly innocuous launcher change
causing a regression.

Well if it is hard then the jbs must be labelled so, noreg-hard.

Kumar



David


Thanks
Kumar



On 4/19/2016 1:32 PM, David Holmes wrote:

On 20/04/2016 3:00 AM, Kumar Srinivasan wrote:

Hi David,

  493 /* Set native thread name. */
  494 SetNativeThreadName(env, "main");
  495
  496 /* Invoke main method. */
  497 (*env)->CallStaticVoidMethod(env, mainClass, mainID,
mainArgs);

Supposing an exception is thrown in 494 (a very remote case),
will we not re-enter the VM in 497 with an exception ?


Yes as I said:


1712 NULL_CHECK(nameString = (*env)->NewStringUTF(env, name));
1713 (*env)->CallVoidMethod(env, currentThread, setNativeNameID,
1714nameString, JNI_TRUE);

As above NULL_CHECK is fine here, but we should check for and clear
any pending exception after CallVoidMethod.



Agreed.

Kumar


Thanks,
David


Kumar


On 19/04/2016 10:54 PM, Gerard Ziemski wrote:



On Apr 19, 2016, at 12:04 AM, David Holmes
<david.hol...@oracle.com>
wrote:

On 19/04/2016 9:28 AM, Yasumasa Suenaga wrote:

Hi Gerard,

2016/04/19 3:14 "Gerard Ziemski" <gerard.ziem...@oracle.com
<mailto:gerard.ziem...@oracle.com>>:


hi Yasumasa,

Nice work. I have 2 questions:


File: java.c

#1 Shouldn’t we be checking for
“(*env)->ExceptionOccurred(env)”

after every single JNI call? In this example instead of
NULL_CHECK,
should we be using CHECK_EXCEPTION_NULL_LEAVE macro?

It is not critical if we encounter error at JNI function call
because
we cannot set native thread name only.
So I think that we do not need to leave from launcher process.


I agree we do not need to abort if an exception occurs (and in
fact
I don't think an exception is even possible from this code),
but we
should ensure any pending exception is cleared before any
futher JNI
calls might be made. Note that NULL_CHECK is already used
extensively throughout the launcher code - so if this usage is
wrong
then it is all wrong! More on this code below ...


Isn’t there a risk that:

(*env)->CallVoidMethod(env, currentThread, setNativeNameID,
+   nameString, JNI_TRUE);

will raise an exception?

In the least, shouldn’t we clear any possible pending
exceptions at
the beginning of “SetNativeThreadName“, as you say, but also at
the
very end?


I was actually saying at the end, after the call (even though I
don't
think any exceptions are possible - except for "async"
exceptions of
course). We shouldn't be able to get to the call with any exception
pending as in that case the JNI calls will be returning NULL and we
will exit - again this pattern is used extensively in the launcher.

David



cheers

Re: RFR: JDK-8152690: main thread does not have native thread name

[Yasumasa Suenaga]

Hi Kumar,


Isn't there a management API or something to enumerate the threads ?


On Linux, user apps can get native thread name through pthread_getname_np(3).
However, this function is not called in hotspot and jdk.

So I think it is difficult to get native thread name in all platforms.



Well if it is hard then the jbs must be labelled so, noreg-hard.


I agree to label noreg-hard.


Thanks,

Yasumasa


On 2016/04/20 23:26, Kumar Srinivasan wrote:


On 4/20/2016 6:06 AM, David Holmes wrote:

On 20/04/2016 10:58 PM, Kumar Srinivasan wrote:


Hello,

One more thing, what about a launcher regression test ?


What kind of regression test? I've manually visually inspected the desired 
behaviour but a test for it is very problematic. AFAIK there are no tests for 
setting the native thread name.


Isn't there a management API or something to enumerate the threads ?
I am more worried about some seemingly innocuous launcher change
causing a regression.

Well if it is hard then the jbs must be labelled so, noreg-hard.

Kumar



David


Thanks
Kumar



On 4/19/2016 1:32 PM, David Holmes wrote:

On 20/04/2016 3:00 AM, Kumar Srinivasan wrote:

Hi David,

  493 /* Set native thread name. */
  494 SetNativeThreadName(env, "main");
  495
  496 /* Invoke main method. */
  497 (*env)->CallStaticVoidMethod(env, mainClass, mainID,
mainArgs);

Supposing an exception is thrown in 494 (a very remote case),
will we not re-enter the VM in 497 with an exception ?


Yes as I said:


1712 NULL_CHECK(nameString = (*env)->NewStringUTF(env, name));
1713 (*env)->CallVoidMethod(env, currentThread, setNativeNameID,
1714nameString, JNI_TRUE);

As above NULL_CHECK is fine here, but we should check for and clear
any pending exception after CallVoidMethod.



Agreed.

Kumar


Thanks,
David


Kumar


On 19/04/2016 10:54 PM, Gerard Ziemski wrote:



On Apr 19, 2016, at 12:04 AM, David Holmes <david.hol...@oracle.com>
wrote:

On 19/04/2016 9:28 AM, Yasumasa Suenaga wrote:

Hi Gerard,

2016/04/19 3:14 "Gerard Ziemski" <gerard.ziem...@oracle.com
<mailto:gerard.ziem...@oracle.com>>:


hi Yasumasa,

Nice work. I have 2 questions:


File: java.c

#1 Shouldn’t we be checking for “(*env)->ExceptionOccurred(env)”

after every single JNI call? In this example instead of NULL_CHECK,
should we be using CHECK_EXCEPTION_NULL_LEAVE macro?

It is not critical if we encounter error at JNI function call
because
we cannot set native thread name only.
So I think that we do not need to leave from launcher process.


I agree we do not need to abort if an exception occurs (and in fact
I don't think an exception is even possible from this code), but we
should ensure any pending exception is cleared before any futher JNI
calls might be made. Note that NULL_CHECK is already used
extensively throughout the launcher code - so if this usage is wrong
then it is all wrong! More on this code below ...


Isn’t there a risk that:

(*env)->CallVoidMethod(env, currentThread, setNativeNameID,
+   nameString, JNI_TRUE);

will raise an exception?

In the least, shouldn’t we clear any possible pending exceptions at
the beginning of “SetNativeThreadName“, as you say, but also at the
very end?


I was actually saying at the end, after the call (even though I don't
think any exceptions are possible - except for "async" exceptions of
course). We shouldn't be able to get to the call with any exception
pending as in that case the JNI calls will be returning NULL and we
will exit - again this pattern is used extensively in the launcher.

David



cheers

Re: RFR: JDK-8152690: main thread does not have native thread name

[Yasumasa Suenaga]

Hi Gerard,

2016/04/19 3:14 "Gerard Ziemski" <gerard.ziem...@oracle.com>:
>
> hi Yasumasa,
>
> Nice work. I have 2 questions:
>
> 
> File: java.c
>
> #1 Shouldn’t we be checking for “(*env)->ExceptionOccurred(env)” after
every single JNI call? In this example instead of NULL_CHECK, should we be
using CHECK_EXCEPTION_NULL_LEAVE macro?

It is not critical if we encounter error at JNI function call  because we
cannot set native thread name only.
So I think that we do not need to leave from launcher process.

> #2 Should the comment for “SetNativeThreadName” be “Set native thread
name if possible.” not "Set native thread name as possible.”?

Sorry for my bad English :-)

Thanks,

Yasumasa

> cheers
>
> > On Apr 16, 2016, at 4:29 AM, Yasumasa Suenaga <yasue...@gmail.com>
wrote:
> >
> > Hi David,
> >
> > I uploaded new webrev:
> >
> > - hotspot:
> >http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.03/hotspot/
> >
> > - jdk:
> >http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.03/jdk/
> >
> >
> >> it won't work unless you change the semantics of setName so I'm not
sure what you were thinking here. To take advantage of an arg taking
JVM_SetNativThreadName you would need to call it directly as no Java code
will call it . ???
> >
> > I added a flag for setting native thread name to JNI-attached thread.
> > This change can set native thread name if main thread changes to
JNI-attached thread.
> >
> >
> > Thanks,
> >
> > Yasumasa
> >
> >
> > On 2016/04/16 16:11, David Holmes wrote:
> >> On 16/04/2016 3:27 PM, Yasumasa Suenaga wrote:
> >>> Hi David,
> >>>
> >>>> That change in behaviour may be a problem, it could be considered a
> >>>> regression that setName stops setting the native thread main, even
> >>>> though we never really intended it to work in the first place. :(
Such
> >>>> a change needs to go through CCC.
> >>>
> >>> I understood.
> >>> Can I send CCC request?
> >>> (I'm jdk 9 commiter, but I'm not employee at Oracle.)
> >>
> >> Sorry you can't file a CCC request, you would need a sponsor for that.
But at this stage I don't think I agree with the proposed change because of
the change in behaviour - there's no way to restore the "broken" behaviour.
> >>
> >>> I want to continue to discuss about it on JDK-8154331 [1].
> >>
> >> Okay we can do that.
> >>
> >>>
> >>>> Further, we expect the launcher to use the supported JNI interface
(as
> >>>> other processes would), not the internal JVM interface that exists
for
> >>>> the JDK sources to communicate with the JVM.
> >>>
> >>> I think that we do not use JVM interface if we add new method in
> >>> LauncherHelper as below:
> >>> 
> >>> diff -r f02139a1ac84
> >>> src/java.base/share/classes/sun/launcher/LauncherHelper.java
> >>> --- a/src/java.base/share/classes/sun/launcher/LauncherHelper.java
> >>> Wed Apr 13 14:19:30 2016 +
> >>> +++ b/src/java.base/share/classes/sun/launcher/LauncherHelper.java
> >>> Sat Apr 16 11:25:53 2016 +0900
> >>> @@ -960,4 +960,8 @@
> >>>  else
> >>>  return md.toNameAndVersion() + " (" + loc + ")";
> >>>  }
> >>> +
> >>> +static void setNativeThreadName(String name) {
> >>> +Thread.currentThread().setName(name);
> >>> +}
> >>
> >> You could also make that call via JNI directly, so not sure the helper
adds much here. But it won't work unless you change the semantics of
setName so I'm not sure what you were thinking here. To take advantage of
an arg taking JVM_SetNativThreadName you would need to call it directly as
no Java code will call it . ???
> >>
> >> David
> >> -
> >>
> >>>  }
> >>> diff -r f02139a1ac84 src/java.base/share/native/libjli/java.c
> >>> --- a/src/java.base/share/native/libjli/java.cWed Apr 13 14:19:30
> >>> 2016 +
> >>> +++ b/src/java.base/share/native/libjli/java.cSat Apr 16 11:25:53
> >>> 2016 +0900
> >>> @@ -125,6 +125,7 @@
> >>>  static void PrintUsage(JNIEnv* env, jboolean doXUsage);
> >>>  static void ShowSettings(JNIEnv* env, char *optString);
> >>>  static void ListModules(JNIEnv* env, char *optString);
> >>

Re: RFR: JDK-8152690: main thread does not have native thread name

[Yasumasa Suenaga]

2016/04/18 13:41 "David Holmes" <david.hol...@oracle.com>:
>
> On 17/04/2016 8:38 PM, David Holmes wrote:
>>
>> On 17/04/2016 1:38 PM, Yasumasa Suenaga wrote:
>>>
>>> Hi David,
>>>
>>>> Need to hear from core-libs and/or launcher folk as this touches a
>>>> number of pieces of code.
>>>
>>>
>>> I'm waiting more reviewer(s) .
>>>
>>> BTW, Should I make patches which are based on jdk9/dev repos?
>>> My proposal includes hotspot changes.
>>> So I want to know whether I can push all changes jdk9/dev/{jdk,hotspot}
>>> after reviewing.
>>
>>
>> No, jdk9/hs please.
>
>
> And it will need to go via JPRT so I will sponsor it for you.

Thanks!
I will send changesets to you after reviewing.

Yasumasa

> Thanks,
> David
>
>
>> Thanks,
>> David
>>
>>> I can update my webrev to adapt to jdk9/dev repos if need.
>>>
>>>
>>> Thanks,
>>>
>>> Yasumasa
>>>
>>>
>>> On 2016/04/17 7:20, David Holmes wrote:
>>>>
>>>> Hi Yasumasa,
>>>>
>>>> On 16/04/2016 7:29 PM, Yasumasa Suenaga wrote:
>>>>>
>>>>> Hi David,
>>>>>
>>>>> I uploaded new webrev:
>>>>>
>>>>>   - hotspot:
>>>>>
>>>>> http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.03/hotspot/
>>>>>
>>>>>   - jdk:
>>>>>  http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.03/jdk/
>>>>
>>>>
>>>> Ah sneaky! :) Using JNI to by-pass access control so we can set up a
>>>> private method to do the name setting, yet avoid any API change that
>>>> would require a CCC request. I think I like it. :)
>>>>
>>>> Need to hear from core-libs and/or launcher folk as this touches a
>>>> number of pieces of code.
>>>>
>>>> Thanks,
>>>> David
>>>> -
>>>>
>>>>>
>>>>>> it won't work unless you change the semantics of setName so I'm not
>>>>>> sure what you were thinking here. To take advantage of an arg taking
>>>>>> JVM_SetNativThreadName you would need to call it directly as no Java
>>>>>> code will call it . ???
>>>>>
>>>>>
>>>>> I added a flag for setting native thread name to JNI-attached thread.
>>>>> This change can set native thread name if main thread changes to
>>>>> JNI-attached thread.
>>>>>
>>>>>
>>>>> Thanks,
>>>>>
>>>>> Yasumasa
>>>>>
>>>>>
>>>>> On 2016/04/16 16:11, David Holmes wrote:
>>>>>>
>>>>>> On 16/04/2016 3:27 PM, Yasumasa Suenaga wrote:
>>>>>>>
>>>>>>> Hi David,
>>>>>>>
>>>>>>>> That change in behaviour may be a problem, it could be considered a
>>>>>>>> regression that setName stops setting the native thread main, even
>>>>>>>> though we never really intended it to work in the first place. :(
>>>>>>>> Such
>>>>>>>> a change needs to go through CCC.
>>>>>>>
>>>>>>>
>>>>>>> I understood.
>>>>>>> Can I send CCC request?
>>>>>>> (I'm jdk 9 commiter, but I'm not employee at Oracle.)
>>>>>>
>>>>>>
>>>>>> Sorry you can't file a CCC request, you would need a sponsor for
that.
>>>>>> But at this stage I don't think I agree with the proposed change
>>>>>> because of the change in behaviour - there's no way to restore the
>>>>>> "broken" behaviour.
>>>>>>
>>>>>>> I want to continue to discuss about it on JDK-8154331 [1].
>>>>>>
>>>>>>
>>>>>> Okay we can do that.
>>>>>>
>>>>>>>
>>>>>>>> Further, we expect the launcher to use the supported JNI interface
>>>>>>>> (as
>>>>>>>> other processes would), not the internal JVM interface that exists
>>>>>>>> for
>>>>>>>> the JDK sources to communicate with the JVM.
>>>>>>>
>>>&

Re: RFR: JDK-8152690: main thread does not have native thread name

[Yasumasa Suenaga]

Hi David,


Need to hear from core-libs and/or launcher folk as this touches a number of 
pieces of code.


I'm waiting more reviewer(s) .

BTW, Should I make patches which are based on jdk9/dev repos?
My proposal includes hotspot changes.
So I want to know whether I can push all changes jdk9/dev/{jdk,hotspot} after 
reviewing.

I can update my webrev to adapt to jdk9/dev repos if need.


Thanks,

Yasumasa


On 2016/04/17 7:20, David Holmes wrote:

Hi Yasumasa,

On 16/04/2016 7:29 PM, Yasumasa Suenaga wrote:

Hi David,

I uploaded new webrev:

  - hotspot:
 http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.03/hotspot/

  - jdk:
 http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.03/jdk/


Ah sneaky! :) Using JNI to by-pass access control so we can set up a private 
method to do the name setting, yet avoid any API change that would require a 
CCC request. I think I like it. :)

Need to hear from core-libs and/or launcher folk as this touches a number of 
pieces of code.

Thanks,
David
-




it won't work unless you change the semantics of setName so I'm not
sure what you were thinking here. To take advantage of an arg taking
JVM_SetNativThreadName you would need to call it directly as no Java
code will call it . ???


I added a flag for setting native thread name to JNI-attached thread.
This change can set native thread name if main thread changes to
JNI-attached thread.


Thanks,

Yasumasa


On 2016/04/16 16:11, David Holmes wrote:

On 16/04/2016 3:27 PM, Yasumasa Suenaga wrote:

Hi David,


That change in behaviour may be a problem, it could be considered a
regression that setName stops setting the native thread main, even
though we never really intended it to work in the first place. :( Such
a change needs to go through CCC.


I understood.
Can I send CCC request?
(I'm jdk 9 commiter, but I'm not employee at Oracle.)


Sorry you can't file a CCC request, you would need a sponsor for that.
But at this stage I don't think I agree with the proposed change
because of the change in behaviour - there's no way to restore the
"broken" behaviour.


I want to continue to discuss about it on JDK-8154331 [1].


Okay we can do that.




Further, we expect the launcher to use the supported JNI interface (as
other processes would), not the internal JVM interface that exists for
the JDK sources to communicate with the JVM.


I think that we do not use JVM interface if we add new method in
LauncherHelper as below:

diff -r f02139a1ac84
src/java.base/share/classes/sun/launcher/LauncherHelper.java
--- a/src/java.base/share/classes/sun/launcher/LauncherHelper.java
Wed Apr 13 14:19:30 2016 +
+++ b/src/java.base/share/classes/sun/launcher/LauncherHelper.java
Sat Apr 16 11:25:53 2016 +0900
@@ -960,4 +960,8 @@
  else
  return md.toNameAndVersion() + " (" + loc + ")";
  }
+
+static void setNativeThreadName(String name) {
+Thread.currentThread().setName(name);
+}


You could also make that call via JNI directly, so not sure the helper
adds much here. But it won't work unless you change the semantics of
setName so I'm not sure what you were thinking here. To take advantage
of an arg taking JVM_SetNativThreadName you would need to call it
directly as no Java code will call it . ???

David
-


  }
diff -r f02139a1ac84 src/java.base/share/native/libjli/java.c
--- a/src/java.base/share/native/libjli/java.cWed Apr 13 14:19:30
2016 +
+++ b/src/java.base/share/native/libjli/java.cSat Apr 16 11:25:53
2016 +0900
@@ -125,6 +125,7 @@
  static void PrintUsage(JNIEnv* env, jboolean doXUsage);
  static void ShowSettings(JNIEnv* env, char *optString);
  static void ListModules(JNIEnv* env, char *optString);
+static void SetNativeThreadName(JNIEnv* env, char *name);

  static void SetPaths(int argc, char **argv);

@@ -325,6 +326,7 @@
   * mainThread.isAlive() to work as expected.
   */
  #define LEAVE() \
+SetNativeThreadName(env, "DestroyJavaVM"); \
  do { \
  if ((*vm)->DetachCurrentThread(vm) != JNI_OK) { \
  JLI_ReportErrorMessage(JVM_ERROR2); \
@@ -488,6 +490,9 @@
  mainArgs = CreateApplicationArgs(env, argv, argc);
  CHECK_EXCEPTION_NULL_LEAVE(mainArgs);

+/* Set native thread name. */
+SetNativeThreadName(env, "main");
+
  /* Invoke main method. */
  (*env)->CallStaticVoidMethod(env, mainClass, mainID, mainArgs);

@@ -1686,6 +1691,22 @@
   joptString);
  }

+/**
+ * Set native thread name as possible.
+ */
+static void
+SetNativeThreadName(JNIEnv *env, char *name)
+{
+jmethodID setNativeThreadNameID;
+jstring nameString;
+jclass cls = GetLauncherHelperClass(env);
+NULL_CHECK(cls);
+NULL_CHECK(setNativeThreadNameID =
(*env)->GetStaticMethodID(env, cls,
+"setNativeThreadName", "(Ljava/lang/String;)V"));
+NULL_CHECK(nameString

Re: RFR: JDK-8152690: main thread does not have native thread name

[Yasumasa Suenaga]

Hi David,

I uploaded new webrev:

 - hotspot:
http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.03/hotspot/

 - jdk:
http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.03/jdk/



it won't work unless you change the semantics of setName so I'm not sure what 
you were thinking here. To take advantage of an arg taking 
JVM_SetNativThreadName you would need to call it directly as no Java code will 
call it . ???


I added a flag for setting native thread name to JNI-attached thread.
This change can set native thread name if main thread changes to JNI-attached 
thread.


Thanks,

Yasumasa


On 2016/04/16 16:11, David Holmes wrote:

On 16/04/2016 3:27 PM, Yasumasa Suenaga wrote:

Hi David,


That change in behaviour may be a problem, it could be considered a
regression that setName stops setting the native thread main, even
though we never really intended it to work in the first place. :( Such
a change needs to go through CCC.


I understood.
Can I send CCC request?
(I'm jdk 9 commiter, but I'm not employee at Oracle.)


Sorry you can't file a CCC request, you would need a sponsor for that. But at this stage 
I don't think I agree with the proposed change because of the change in behaviour - 
there's no way to restore the "broken" behaviour.


I want to continue to discuss about it on JDK-8154331 [1].


Okay we can do that.




Further, we expect the launcher to use the supported JNI interface (as
other processes would), not the internal JVM interface that exists for
the JDK sources to communicate with the JVM.


I think that we do not use JVM interface if we add new method in
LauncherHelper as below:

diff -r f02139a1ac84
src/java.base/share/classes/sun/launcher/LauncherHelper.java
--- a/src/java.base/share/classes/sun/launcher/LauncherHelper.java
Wed Apr 13 14:19:30 2016 +
+++ b/src/java.base/share/classes/sun/launcher/LauncherHelper.java
Sat Apr 16 11:25:53 2016 +0900
@@ -960,4 +960,8 @@
  else
  return md.toNameAndVersion() + " (" + loc + ")";
  }
+
+static void setNativeThreadName(String name) {
+Thread.currentThread().setName(name);
+}


You could also make that call via JNI directly, so not sure the helper adds 
much here. But it won't work unless you change the semantics of setName so I'm 
not sure what you were thinking here. To take advantage of an arg taking 
JVM_SetNativThreadName you would need to call it directly as no Java code will 
call it . ???

David
-


  }
diff -r f02139a1ac84 src/java.base/share/native/libjli/java.c
--- a/src/java.base/share/native/libjli/java.cWed Apr 13 14:19:30
2016 +
+++ b/src/java.base/share/native/libjli/java.cSat Apr 16 11:25:53
2016 +0900
@@ -125,6 +125,7 @@
  static void PrintUsage(JNIEnv* env, jboolean doXUsage);
  static void ShowSettings(JNIEnv* env, char *optString);
  static void ListModules(JNIEnv* env, char *optString);
+static void SetNativeThreadName(JNIEnv* env, char *name);

  static void SetPaths(int argc, char **argv);

@@ -325,6 +326,7 @@
   * mainThread.isAlive() to work as expected.
   */
  #define LEAVE() \
+SetNativeThreadName(env, "DestroyJavaVM"); \
  do { \
  if ((*vm)->DetachCurrentThread(vm) != JNI_OK) { \
  JLI_ReportErrorMessage(JVM_ERROR2); \
@@ -488,6 +490,9 @@
  mainArgs = CreateApplicationArgs(env, argv, argc);
  CHECK_EXCEPTION_NULL_LEAVE(mainArgs);

+/* Set native thread name. */
+SetNativeThreadName(env, "main");
+
  /* Invoke main method. */
  (*env)->CallStaticVoidMethod(env, mainClass, mainID, mainArgs);

@@ -1686,6 +1691,22 @@
   joptString);
  }

+/**
+ * Set native thread name as possible.
+ */
+static void
+SetNativeThreadName(JNIEnv *env, char *name)
+{
+jmethodID setNativeThreadNameID;
+jstring nameString;
+jclass cls = GetLauncherHelperClass(env);
+NULL_CHECK(cls);
+NULL_CHECK(setNativeThreadNameID = (*env)->GetStaticMethodID(env, cls,
+"setNativeThreadName", "(Ljava/lang/String;)V"));
+NULL_CHECK(nameString = (*env)->NewStringUTF(env, name));
+(*env)->CallStaticVoidMethod(env, cls, setNativeThreadNameID,
nameString);
+}
+
  /*
   * Prints default usage or the Xusage message, see
sun.launcher.LauncherHelper.java
   */


So I want to add new arg to JVM_SetNativeThreadName().


However this is still a change to the exported JVM interface and so
has to be approved.


Do you mean that this change needs CCC?


Thanks,

Yasumasa


[1]
http://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/2016-April/019034.html



On 2016/04/16 7:26, David Holmes wrote:

On 15/04/2016 11:20 PM, Yasumasa Suenaga wrote:

Hi David,


I think it is a bug based on the comment here:

JavaThread(bool is_attaching_via_jni = false); // for main thread and
JNI attached threads


I filed it to JBS as JDK-8154331.
I will send revie

Re: RFR: JDK-8152690: main thread does not have native thread name

[Yasumasa Suenaga]

Hi David,


That change in behaviour may be a problem, it could be considered a regression 
that setName stops setting the native thread main, even though we never really 
intended it to work in the first place. :( Such a change needs to go through 
CCC.


I understood.
Can I send CCC request?
(I'm jdk 9 commiter, but I'm not employee at Oracle.)

I want to continue to discuss about it on JDK-8154331 [1].



Further, we expect the launcher to use the supported JNI interface (as other 
processes would), not the internal JVM interface that exists for the JDK 
sources to communicate with the JVM.


I think that we do not use JVM interface if we add new method in LauncherHelper 
as below:

diff -r f02139a1ac84 
src/java.base/share/classes/sun/launcher/LauncherHelper.java
--- a/src/java.base/share/classes/sun/launcher/LauncherHelper.java  Wed Apr 
13 14:19:30 2016 +
+++ b/src/java.base/share/classes/sun/launcher/LauncherHelper.java  Sat Apr 
16 11:25:53 2016 +0900
@@ -960,4 +960,8 @@
 else
 return md.toNameAndVersion() + " (" + loc + ")";
 }
+
+static void setNativeThreadName(String name) {
+Thread.currentThread().setName(name);
+}
 }
diff -r f02139a1ac84 src/java.base/share/native/libjli/java.c
--- a/src/java.base/share/native/libjli/java.c  Wed Apr 13 14:19:30 2016 +
+++ b/src/java.base/share/native/libjli/java.c  Sat Apr 16 11:25:53 2016 +0900
@@ -125,6 +125,7 @@
 static void PrintUsage(JNIEnv* env, jboolean doXUsage);
 static void ShowSettings(JNIEnv* env, char *optString);
 static void ListModules(JNIEnv* env, char *optString);
+static void SetNativeThreadName(JNIEnv* env, char *name);
 
 static void SetPaths(int argc, char **argv);
 
@@ -325,6 +326,7 @@

  * mainThread.isAlive() to work as expected.
  */
 #define LEAVE() \
+SetNativeThreadName(env, "DestroyJavaVM"); \
 do { \
 if ((*vm)->DetachCurrentThread(vm) != JNI_OK) { \
 JLI_ReportErrorMessage(JVM_ERROR2); \
@@ -488,6 +490,9 @@
 mainArgs = CreateApplicationArgs(env, argv, argc);
 CHECK_EXCEPTION_NULL_LEAVE(mainArgs);
 
+/* Set native thread name. */

+SetNativeThreadName(env, "main");
+
 /* Invoke main method. */
 (*env)->CallStaticVoidMethod(env, mainClass, mainID, mainArgs);
 
@@ -1686,6 +1691,22 @@

  joptString);
 }
 
+/**

+ * Set native thread name as possible.
+ */
+static void
+SetNativeThreadName(JNIEnv *env, char *name)
+{
+jmethodID setNativeThreadNameID;
+jstring nameString;
+jclass cls = GetLauncherHelperClass(env);
+NULL_CHECK(cls);
+NULL_CHECK(setNativeThreadNameID = (*env)->GetStaticMethodID(env, cls,
+"setNativeThreadName", "(Ljava/lang/String;)V"));
+NULL_CHECK(nameString = (*env)->NewStringUTF(env, name));
+(*env)->CallStaticVoidMethod(env, cls, setNativeThreadNameID, nameString);
+}
+
 /*
  * Prints default usage or the Xusage message, see 
sun.launcher.LauncherHelper.java
  */


So I want to add new arg to JVM_SetNativeThreadName().


However this is still a change to the exported JVM interface and so has to be 
approved.


Do you mean that this change needs CCC?


Thanks,

Yasumasa
 


[1] 
http://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/2016-April/019034.html


On 2016/04/16 7:26, David Holmes wrote:

On 15/04/2016 11:20 PM, Yasumasa Suenaga wrote:

Hi David,


I think it is a bug based on the comment here:

JavaThread(bool is_attaching_via_jni = false); // for main thread and
JNI attached threads


I filed it to JBS as JDK-8154331.
I will send review request to hotspot-runtime-dev.



Though that will introduce a change in behaviour by itself as setName
will no longer set the native name for the main thread.


I know.


That change in behaviour may be a problem, it could be considered a regression 
that setName stops setting the native thread main, even though we never really 
intended it to work in the first place. :( Such a change needs to go through 
CCC.


I checked changeset history.
JVM_SetNativeThreadName() was introduced in JDK-7098194, and it is
backported JDK 8.


Yes this all came in as part of the OSX port in 7u2.


However, this function seems to be called from Thread#setNativeName() only.
In addition, Thread#setNativeName() is private method.

Thus I think that we can add an argument to JVM_SetNativeThreadName()
for force setting.
(e.g. "bool forced")

It makes a change of JVM API.
However, this function is NOT public, so I think we can add one more
argument.

What do you think about this?
If it is accepted, we can set native thread name from Java launcher.


The private/public aspect of the Java API is not really at issue. Yes we would add 
another arg to the JVM function to allow it to apply to JNI-attached threads as well (I'd 
prefer the arg reflect that not just "force"). However this

Re: RFR: JDK-8152690: main thread does not have native thread name

[Yasumasa Suenaga]

Hi David,


I think it is a bug based on the comment here:

JavaThread(bool is_attaching_via_jni = false); // for main thread and JNI 
attached threads


I filed it to JBS as JDK-8154331.
I will send review request to hotspot-runtime-dev.



Though that will introduce a change in behaviour by itself as setName will no 
longer set the native name for the main thread.


I know.

I checked changeset history.
JVM_SetNativeThreadName() was introduced in JDK-7098194, and it is backported 
JDK 8.
However, this function seems to be called from Thread#setNativeName() only.
In addition, Thread#setNativeName() is private method.

Thus I think that we can add an argument to JVM_SetNativeThreadName() for force 
setting.
(e.g. "bool forced")

It makes a change of JVM API.
However, this function is NOT public, so I think we can add one more argument.

What do you think about this?
If it is accepted, we can set native thread name from Java launcher.


Thanks,

Yasumasa


On 2016/04/15 19:16, David Holmes wrote:

Hi Yasumasa,

On 15/04/2016 6:53 PM, Yasumasa Suenaga wrote:

Hi David,


The fact that the "main" thread is not tagged as being a JNI-attached
thread seems accidental to me


Should I file it to JBS?


I think it is a bug based on the comment here:

JavaThread(bool is_attaching_via_jni = false); // for main thread and JNI 
attached threads

Though that will introduce a change in behaviour by itself as setName will no 
longer set the native name for the main thread.


I think that we can fix as below:
---
diff -r 52aa0ee93b32 src/share/vm/runtime/thread.cpp
--- a/src/share/vm/runtime/thread.cpp   Thu Apr 14 13:31:11 2016 +0200
+++ b/src/share/vm/runtime/thread.cpp   Fri Apr 15 17:50:10 2016 +0900
@@ -3592,7 +3592,7 @@
  #endif // INCLUDE_JVMCI

// Attach the main thread to this os thread
-  JavaThread* main_thread = new JavaThread();
+  JavaThread* main_thread = new JavaThread(true);
main_thread->set_thread_state(_thread_in_vm);
main_thread->initialize_thread_current();
// must do this before set_active_handles
@@ -3776,6 +3776,9 @@
// Notify JVMTI agents that VM initialization is complete - nop if
no agents.
JvmtiExport::post_vm_initialized();

+  // Change attach status to "attached"
+  main_thread->set_done_attaching_via_jni();
+


I think we can do this straight after the JavaThread constructor.


if (TRACE_START() != JNI_OK) {
  vm_exit_during_initialization("Failed to start tracing backend.");
}
---



If it wants to name its native threads then it is free to do so,


Currently, JVM_SetNativeThreadName() cannot change native thread name
when the caller thread is JNI-attached thread.
However, I think that it should be changed if Java developer calls
Thread#setName() explicitly.
It is not the same of changing native thread name at Threads::create_vm().

If it is allowed, I want to fix SetNativeThreadName() as below.
What do you think about this?


The decision to not change the name of JNI-attached threads was a deliberate one** - this 
functionality originated with the OSX port and it was reported that the initial feedback 
with this feature was to ensure it didn't mess with thread names that had been set by the 
host process. If we do as you propose then we will just have an inconsistency for people 
to complain about: "why does my native thread only have a name if I call 
cur.setName(cur.getName()) ?"

** If you follow the bugs and related discussions on this, the semantics and 
limitations (truncation, current thread only, non-JNI threads only) of setting 
the native thread name were supposed to be documented in the release notes - 
but as far as I can see that never happened. :(

My position on this remains that if it is desirable for the main thread (and 
DestroyJavaVM thread) to have native names then the launcher needs to be 
setting them using the available platform APIs. Unfortunately this is 
complicated - as evidenced by the VM code for this - due to the need to verify 
API availability.

Any change in behaviour in relation to Thread.setName would have to go through 
our CCC process I think. But a change in the launcher would not.

Sorry.

David
-


---
--- a/src/share/vm/prims/jvm.cppThu Apr 14 13:31:11 2016 +0200
+++ b/src/share/vm/prims/jvm.cppFri Apr 15 17:50:10 2016 +0900
@@ -3187,7 +3187,7 @@
JavaThread* thr = java_lang_Thread::thread(java_thread);
// Thread naming only supported for the current thread, doesn't work
for
// target threads.
-  if (Thread::current() == thr && !thr->has_attached_via_jni()) {
+  if (Thread::current() == thr) {
  // we don't set the name of an attached thread to avoid stepping
  // on other programs
  const char *thread_name =
java_lang_String::as_utf8_string(JNIHandles::resolve_non_null(name));
---


Thanks,

Yasumasa


On 2016/04/15 13:32, David Holmes wrote:

Re: RFR: JDK-8152690: main thread does not have native thread name

[Yasumasa Suenaga]

Hi David,


The fact that the "main" thread is not tagged as being a JNI-attached thread 
seems accidental to me


Should I file it to JBS?
I think that we can fix as below:
---
diff -r 52aa0ee93b32 src/share/vm/runtime/thread.cpp
--- a/src/share/vm/runtime/thread.cpp   Thu Apr 14 13:31:11 2016 +0200
+++ b/src/share/vm/runtime/thread.cpp   Fri Apr 15 17:50:10 2016 +0900
@@ -3592,7 +3592,7 @@
 #endif // INCLUDE_JVMCI

   // Attach the main thread to this os thread
-  JavaThread* main_thread = new JavaThread();
+  JavaThread* main_thread = new JavaThread(true);
   main_thread->set_thread_state(_thread_in_vm);
   main_thread->initialize_thread_current();
   // must do this before set_active_handles
@@ -3776,6 +3776,9 @@
   // Notify JVMTI agents that VM initialization is complete - nop if no agents.
   JvmtiExport::post_vm_initialized();

+  // Change attach status to "attached"
+  main_thread->set_done_attaching_via_jni();
+
   if (TRACE_START() != JNI_OK) {
 vm_exit_during_initialization("Failed to start tracing backend.");
   }
---



If it wants to name its native threads then it is free to do so,


Currently, JVM_SetNativeThreadName() cannot change native thread name
when the caller thread is JNI-attached thread.
However, I think that it should be changed if Java developer calls
Thread#setName() explicitly.
It is not the same of changing native thread name at Threads::create_vm().

If it is allowed, I want to fix SetNativeThreadName() as below.
What do you think about this?
---
--- a/src/share/vm/prims/jvm.cppThu Apr 14 13:31:11 2016 +0200
+++ b/src/share/vm/prims/jvm.cppFri Apr 15 17:50:10 2016 +0900
@@ -3187,7 +3187,7 @@
   JavaThread* thr = java_lang_Thread::thread(java_thread);
   // Thread naming only supported for the current thread, doesn't work for
   // target threads.
-  if (Thread::current() == thr && !thr->has_attached_via_jni()) {
+  if (Thread::current() == thr) {
 // we don't set the name of an attached thread to avoid stepping
 // on other programs
 const char *thread_name = 
java_lang_String::as_utf8_string(JNIHandles::resolve_non_null(name));
---


Thanks,

Yasumasa


On 2016/04/15 13:32, David Holmes wrote:



On 15/04/2016 1:11 AM, Yasumasa Suenaga wrote:

Roger,
Thanks for your comment!

David,


I'll wait to see what Kumar thinks about this. I don't like exposing
a new JVM function this way.


I tried to call Thread#setName() after initializing VM (before calling
main method),
I could set native thread name.
However, DestroyJavaVM() calls AttachCurrentThread(). So we can't set
native thread name for DestroyJavaVM.


Right - I came to the same realization earlier this morning. Which, unfortunately, takes 
me back to the basic premise here that we don't set the name of threads not created by 
the JVM. The fact that the "main" thread is not tagged as being a JNI-attached 
thread seems accidental to me - so JVM_SetNativeThreadName is only working by accident 
for the initial attach, and can't be used for the DestroyJavaVM part - which leaves the 
thread inconsistently named at the native level.

I'm afraid my view here is that the launcher has to be treated like any other 
process that might host a JVM. If it wants to name its native threads then it 
is free to do so, but I would not be exporting a function from the JVM to do 
that - it would have to use the OS specific API's for that on a 
platform-by-platform basis.

Sorry.

David
-





Thanks,

Yasumasa


On 2016/04/14 23:24, Roger Riggs wrote:

Hi,

Comments:

jvm.h:  The function names are too similar but perform different
functions:

-JVM_SetNativeThreadName0 vs JVM_SetNativeThreadName

-  The first function applies to the current thread, the second one a
specific java thread.
  It would seem useful for there to be a comment somewhere about what
the new function does.

windows/native/libjli/java_md.c: line 408 casts to (void*) instead of
(SetNativeThreadName0_t)
as is done on unix and mac.

- macosx/native/libjli/java_md_macosx.c:
   - 737: looks wrong to overwriteifn->GetCreatedJavaVMs used at line 730
   - 738  Incorrect indentation; if possible keep the cast on the same
line as dlsym...

$.02, Roger


On 4/14/2016 9:32 AM, Yasumasa Suenaga wrote:

That is an interesting question which I haven't had time to check -
sorry. If the main thread is considered a JNI-attached thread then
my suggestion wont work. If it isn't then my suggestion should work
(but it means we have an inconsistency in our treatment of
JNI-attached threads :( )


I ran following program on JDK 9 EA b112, and I confirmed native
thread name (test) was set.
-
public class Sleep{
  public static void main(String[] args) throws Exception{
Thread.currentThread().setName("test");
Thread.sleep(360);
  }
}
-



I'll wait to see what Kumar thinks about this. I don't like exposing

Re: RFR: JDK-8152690: main thread does not have native thread name

[Yasumasa Suenaga]

Roger,
Thanks for your comment!

David,


I'll wait to see what Kumar thinks about this. I don't like exposing a new JVM 
function this way.


I tried to call Thread#setName() after initializing VM (before calling main 
method),
I could set native thread name.
However, DestroyJavaVM() calls AttachCurrentThread(). So we can't set native 
thread name
for DestroyJavaVM.


Thanks,

Yasumasa


On 2016/04/14 23:24, Roger Riggs wrote:

Hi,

Comments:

jvm.h:  The function names are too similar but perform different functions:

-JVM_SetNativeThreadName0 vs JVM_SetNativeThreadName

-  The first function applies to the current thread, the second one a specific 
java thread.
  It would seem useful for there to be a comment somewhere about what the new 
function does.

windows/native/libjli/java_md.c: line 408 casts to (void*) instead of 
(SetNativeThreadName0_t)
as is done on unix and mac.

- macosx/native/libjli/java_md_macosx.c:
   - 737: looks wrong to overwriteifn->GetCreatedJavaVMs used at line 730
   - 738  Incorrect indentation; if possible keep the cast on the same line as 
dlsym...

$.02, Roger


On 4/14/2016 9:32 AM, Yasumasa Suenaga wrote:

That is an interesting question which I haven't had time to check - sorry. If 
the main thread is considered a JNI-attached thread then my suggestion wont 
work. If it isn't then my suggestion should work (but it means we have an 
inconsistency in our treatment of JNI-attached threads :( )


I ran following program on JDK 9 EA b112, and I confirmed native thread name 
(test) was set.
-
public class Sleep{
  public static void main(String[] args) throws Exception{
Thread.currentThread().setName("test");
Thread.sleep(360);
  }
}
-



I'll wait to see what Kumar thinks about this. I don't like exposing a new JVM 
function this way.


I will update webrev after hearing Kumar's comment.


Thanks,

Yasumasa


On 2016/04/14 21:32, David Holmes wrote:

On 14/04/2016 1:52 PM, Yasumasa Suenaga wrote:

Hi,

On 2016/04/14 9:34, David Holmes wrote:

Hi,

On 14/04/2016 1:28 AM, Yasumasa Suenaga wrote:

Hi David,

Thanks for your comment.

I exported new JVM function to set native thread name, and JLI uses it
in new webrev.


First the launcher belongs to another team so core-libs will need to
review and approve this (in particular Kumar) - now cc'd.


Thanks!
I'm waiting to review :-)



Personally I would have used a Java upcall to Thread.setName rather
than exporting JVM_SetNativeThreadName. No hotspot changes needed in
that case.


As I wrote [1] in JBS, I changed to use Thread#setName() in Thread#init(),
but I could not change native thread name.


At Thread.init time the thread is not alive, which is why the native name is 
not set.


I guess that caller of main() is JNI attached thread.


That is an interesting question which I haven't had time to check - sorry. If 
the main thread is considered a JNI-attached thread then my suggestion wont 
work. If it isn't then my suggestion should work (but it means we have an 
inconsistency in our treatment of JNI-attached threads :( )

I'll wait to see what Kumar thinks about this. I don't like exposing a new JVM 
function this way.

Thanks,
David
-


Thus I think that we have to provide a function to set native thread name.


Thanks,

Yasumasa


[1]
https://bugs.openjdk.java.net/browse/JDK-8152690?focusedCommentId=13926851=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-13926851



Thanks,
David


Could you review again?

   - hotspot:

http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.02/hotspot/

   - jdk:
http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.02/jdk/


Thanks,

Yasumasa


On 2016/04/13 22:00, David Holmes wrote:

I'll answer on this original thread as well ...

Hi Yasumasa,

Please see my updates to the bug (sorry have been on vacation). This
needs to be done in the launcher to be correct as we do not set the
name of threads that attach via JNI, which includes the "main" thread.

David

On 31/03/2016 9:49 AM, Yasumasa Suenaga wrote:

Thanks Robbin,

I'm waiting a sponsor and more reviewer :-)

Yasumasa
2016/03/31 5:58 "Robbin Ehn" <robbin@oracle.com>:


FYI: I'm not a Reviewer.

/Robbin

On 03/30/2016 10:55 PM, Robbin Ehn wrote:


Thanks, looks good.

/Robbin

On 03/30/2016 03:47 PM, Yasumasa Suenaga wrote:


Hi,

I uploaded new webrev.
Could you review it?

http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.01/


Thanks,

Yasumasa


On 2016/03/30 19:10, Robbin Ehn wrote:


Hi,

On 03/30/2016 11:41 AM, Yasumasa Suenaga wrote:


Hi Robbin,

2016/03/30 18:22 "Robbin Ehn" <robbin@oracle.com
<mailto:robbin@oracle.com>>:
  >
  > Hi Yasumasa,
  >
  >
  > On 03/25/2016 12:48 AM, Yasumasa Suenaga wrote:
  >>
  >> Hi Robbin,
  >> 2016/03/25 1:51 "Robbin Ehn" <robbin@oracle.com
<mailto:robbin@oracle.com>
  >> <mai

Re: RFR: JDK-8152690: main thread does not have native thread name

[Yasumasa Suenaga]

That is an interesting question which I haven't had time to check - sorry. If 
the main thread is considered a JNI-attached thread then my suggestion wont 
work. If it isn't then my suggestion should work (but it means we have an 
inconsistency in our treatment of JNI-attached threads :( )


I ran following program on JDK 9 EA b112, and I confirmed native thread name 
(test) was set.
-
public class Sleep{
  public static void main(String[] args) throws Exception{
Thread.currentThread().setName("test");
Thread.sleep(360);
  }
}
-



I'll wait to see what Kumar thinks about this. I don't like exposing a new JVM 
function this way.


I will update webrev after hearing Kumar's comment.


Thanks,

Yasumasa


On 2016/04/14 21:32, David Holmes wrote:

On 14/04/2016 1:52 PM, Yasumasa Suenaga wrote:

Hi,

On 2016/04/14 9:34, David Holmes wrote:

Hi,

On 14/04/2016 1:28 AM, Yasumasa Suenaga wrote:

Hi David,

Thanks for your comment.

I exported new JVM function to set native thread name, and JLI uses it
in new webrev.


First the launcher belongs to another team so core-libs will need to
review and approve this (in particular Kumar) - now cc'd.


Thanks!
I'm waiting to review :-)



Personally I would have used a Java upcall to Thread.setName rather
than exporting JVM_SetNativeThreadName. No hotspot changes needed in
that case.


As I wrote [1] in JBS, I changed to use Thread#setName() in Thread#init(),
but I could not change native thread name.


At Thread.init time the thread is not alive, which is why the native name is 
not set.


I guess that caller of main() is JNI attached thread.


That is an interesting question which I haven't had time to check - sorry. If 
the main thread is considered a JNI-attached thread then my suggestion wont 
work. If it isn't then my suggestion should work (but it means we have an 
inconsistency in our treatment of JNI-attached threads :( )

I'll wait to see what Kumar thinks about this. I don't like exposing a new JVM 
function this way.

Thanks,
David
-


Thus I think that we have to provide a function to set native thread name.


Thanks,

Yasumasa


[1]
https://bugs.openjdk.java.net/browse/JDK-8152690?focusedCommentId=13926851=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-13926851



Thanks,
David


Could you review again?

   - hotspot:

http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.02/hotspot/

   - jdk:
   http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.02/jdk/


Thanks,

Yasumasa


On 2016/04/13 22:00, David Holmes wrote:

I'll answer on this original thread as well ...

Hi Yasumasa,

Please see my updates to the bug (sorry have been on vacation). This
needs to be done in the launcher to be correct as we do not set the
name of threads that attach via JNI, which includes the "main" thread.

David

On 31/03/2016 9:49 AM, Yasumasa Suenaga wrote:

Thanks Robbin,

I'm waiting a sponsor and more reviewer :-)

Yasumasa
2016/03/31 5:58 "Robbin Ehn" <robbin@oracle.com>:


FYI: I'm not a Reviewer.

/Robbin

On 03/30/2016 10:55 PM, Robbin Ehn wrote:


Thanks, looks good.

/Robbin

On 03/30/2016 03:47 PM, Yasumasa Suenaga wrote:


Hi,

I uploaded new webrev.
Could you review it?

http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.01/


Thanks,

Yasumasa


On 2016/03/30 19:10, Robbin Ehn wrote:


Hi,

On 03/30/2016 11:41 AM, Yasumasa Suenaga wrote:


Hi Robbin,

2016/03/30 18:22 "Robbin Ehn" <robbin@oracle.com
<mailto:robbin@oracle.com>>:
  >
  > Hi Yasumasa,
  >
  >
  > On 03/25/2016 12:48 AM, Yasumasa Suenaga wrote:
  >>
  >> Hi Robbin,
  >> 2016/03/25 1:51 "Robbin Ehn" <robbin@oracle.com
<mailto:robbin@oracle.com>
  >> <mailto:robbin@oracle.com
<mailto:robbin@oracle.com>>>:
  >>
  >>  >
  >>  > Hi Yasumasa,
  >>  >
  >>  > I'm not sure why you don't set it:
  >>  >
  >>  > diff -r ded6ef79c770 src/share/vm/runtime/thread.cpp
  >>  > --- a/src/share/vm/runtime/thread.cpp   Thu Mar 24
13:09:16 2016
+
  >>  > +++ b/src/share/vm/runtime/thread.cpp   Thu Mar 24
17:40:09 2016
+0100
  >>  > @@ -3584,6 +3584,7 @@
  >>  >JavaThread* main_thread = new JavaThread();
  >>  >main_thread->set_thread_state(_thread_in_vm);
  >>  >main_thread->initialize_thread_current();
  >>  > +  main_thread->set_native_thread_name("main");
  >>  >// must do this before set_active_handles
  >>  >main_thread->record_stack_base_and_size();
  >>  >
main_thread->set_active_handles(JNIHandleBlock::allocate_block());

  >>  >
  >>  > here instead? Am I missing something?
  >>
  >> Native thread name is the same to thread name in Thread
class.
  >

Re: RFR: JDK-8152690: main thread does not have native thread name

[Yasumasa Suenaga]

Hi,

On 2016/04/14 9:34, David Holmes wrote:

Hi,

On 14/04/2016 1:28 AM, Yasumasa Suenaga wrote:

Hi David,

Thanks for your comment.

I exported new JVM function to set native thread name, and JLI uses it
in new webrev.


First the launcher belongs to another team so core-libs will need to review and 
approve this (in particular Kumar) - now cc'd.


Thanks!
I'm waiting to review :-)



Personally I would have used a Java upcall to Thread.setName rather than 
exporting JVM_SetNativeThreadName. No hotspot changes needed in that case.


As I wrote [1] in JBS, I changed to use Thread#setName() in Thread#init(),
but I could not change native thread name.
I guess that caller of main() is JNI attached thread.

Thus I think that we have to provide a function to set native thread name.


Thanks,

Yasumasa


[1] 
https://bugs.openjdk.java.net/browse/JDK-8152690?focusedCommentId=13926851=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-13926851


Thanks,
David


Could you review again?

   - hotspot:
   http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.02/hotspot/

   - jdk:
   http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.02/jdk/


Thanks,

Yasumasa


On 2016/04/13 22:00, David Holmes wrote:

I'll answer on this original thread as well ...

Hi Yasumasa,

Please see my updates to the bug (sorry have been on vacation). This
needs to be done in the launcher to be correct as we do not set the
name of threads that attach via JNI, which includes the "main" thread.

David

On 31/03/2016 9:49 AM, Yasumasa Suenaga wrote:

Thanks Robbin,

I'm waiting a sponsor and more reviewer :-)

Yasumasa
2016/03/31 5:58 "Robbin Ehn" <robbin@oracle.com>:


FYI: I'm not a Reviewer.

/Robbin

On 03/30/2016 10:55 PM, Robbin Ehn wrote:


Thanks, looks good.

/Robbin

On 03/30/2016 03:47 PM, Yasumasa Suenaga wrote:


Hi,

I uploaded new webrev.
Could you review it?

http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.01/


Thanks,

Yasumasa


On 2016/03/30 19:10, Robbin Ehn wrote:


Hi,

On 03/30/2016 11:41 AM, Yasumasa Suenaga wrote:


Hi Robbin,

2016/03/30 18:22 "Robbin Ehn" <robbin@oracle.com
<mailto:robbin@oracle.com>>:
  >
  > Hi Yasumasa,
  >
  >
  > On 03/25/2016 12:48 AM, Yasumasa Suenaga wrote:
  >>
  >> Hi Robbin,
  >> 2016/03/25 1:51 "Robbin Ehn" <robbin@oracle.com
<mailto:robbin@oracle.com>
  >> <mailto:robbin@oracle.com
<mailto:robbin@oracle.com>>>:
  >>
  >>  >
  >>  > Hi Yasumasa,
  >>  >
  >>  > I'm not sure why you don't set it:
  >>  >
  >>  > diff -r ded6ef79c770 src/share/vm/runtime/thread.cpp
  >>  > --- a/src/share/vm/runtime/thread.cpp   Thu Mar 24
13:09:16 2016
+
  >>  > +++ b/src/share/vm/runtime/thread.cpp   Thu Mar 24
17:40:09 2016
+0100
  >>  > @@ -3584,6 +3584,7 @@
  >>  >JavaThread* main_thread = new JavaThread();
  >>  >main_thread->set_thread_state(_thread_in_vm);
  >>  >main_thread->initialize_thread_current();
  >>  > +  main_thread->set_native_thread_name("main");
  >>  >// must do this before set_active_handles
  >>  >main_thread->record_stack_base_and_size();
  >>  >
main_thread->set_active_handles(JNIHandleBlock::allocate_block());
  >>  >
  >>  > here instead? Am I missing something?
  >>
  >> Native thread name is the same to thread name in Thread class.
  >> It is set in c'tor in Thread or setName().
  >> If you create new thread in Java app, native thread name
will be
set at
  >> startup. However, main thread is already starte in VM.
  >> Thread name for "main" is set in create_initial_thread().
  >> I think that the place of setting thrrad name should be the
same.
  >
  >
  > Yes, I see your point. But then something like this is
nicer, no?
  >
  > --- a/src/share/vm/runtime/thread.cpp   Tue Mar 29 09:43:05
2016
+0200
  > +++ b/src/share/vm/runtime/thread.cpp   Wed Mar 30 10:51:12
2016
+0200
  > @@ -981,6 +981,7 @@
  >  // Creates the initial Thread
  >  static oop create_initial_thread(Handle thread_group,
JavaThread*
thread,
  >   TRAPS) {
  > +  static const char* initial_thread_name = "main";
  >Klass* k =
SystemDictionary::resolve_or_fail(vmSymbols::java_lang_Thread(),
true,
CHECK_NULL);
  >instanceKlassHandle klass (THREAD, k);
  >instanceHandle thread_oop =
klass->allocate_instance_handle(CHECK_NULL);
  > @@ -988,8 +989,10 @@
  >java_lang_Thread::set_thread(thread_oop(), thread);
  >java_lang_Thread::set_priority(thread_oop(), NormPriority);
  >thread->set_threadObj(thread_oop());
  > -
  > -  Ha

PING: RFR: JDK-4347142: Need method to set Password protection to Zip entries

[Yasumasa Suenaga]

PING: Could you review it?
We want to move forward this enhancement.

Thanks,

Yasumasa
2016/03/19 22:01 "Yasumasa Suenaga" <yasue...@gmail.com>:

> Hi Alan,
>
> > I think the main issue here is to decide whether the API
> > should be extended for encryption or not.
>
> We've discussed on the premise that we add the API for supporting ZIP
> encryption.
> In this context, Sherman tried to implement AES encryption extending
> current API. [1]
>
> IMHO, ZIP encryption should be implemented as extention of current ZIP API
> because encryption/decryption will process for each ZIP entries.
>
> According to Developers' Guide, guideline for adding new API is TBD. [2]
> What should I do next?
>
>
> Thanks,
>
> Yasumsa
>
>
> [1]
> http://mail.openjdk.java.net/pipermail/core-libs-dev/2016-January/037903.html
> [2] http://openjdk.java.net/guide/changePlanning.html#api
>
>
> On 2016/03/19 0:25, Alan Bateman wrote:
> >
> > On 18/03/2016 15:02, Yasumasa Suenaga wrote:
> >> Hi all,
> >>
> >> We (including me and Yuji Kubota (ykubota: OpenJDK jdk9 Author))
> discussed
> >> about this issue from Nov. 2015. [1]
> >> We heard several comments and we applied them to our patch.
> >>
> >> I have not heard new comment for our latest patch.
> >> So I send review request for it. Could you review it?
> >>
> >>webrev:
> >>  http://cr.openjdk.java.net/~ysuenaga/JDK-4347142/webrev.04/
> >>
> >>Usage of new API:
> >>
> http://cr.openjdk.java.net/~ysuenaga/JDK-4347142/webrev.04/Test.java
> >>
> > Yasumasa - I think the main issue here is to decide whether the API
> > should be extended for encryption or not. If exposing it in the API make
> > sense then I assume that alternative names to java.util.zip.ZipCryption
> > needs to be explored.
> >
> > -Alan.
> >
>