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

[Kim Barrett]

On Thu, 26 May 2022 10:55:28 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:
> 
>   Fix comments

Marked as reviewed by kbarrett (Reviewer).

-

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

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

[Kim Barrett]

On Wed, 25 May 2022 09:16:43 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 two 
> additional commits since the last revision:
> 
>  - Change Array::data() implementation
>  - Avoid stringop-overflow warning in jfrTraceIdBits.inline.hpp

Mostly good, but I missed a problem with an earlier part of the change.  Sorry 
I didn't notice sooner.

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 `&&`.

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.

-

Changes requested by kbarrett (Reviewer).

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

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

[Kim Barrett]

On Sun, 22 May 2022 16:45:11 GMT, Kim Barrett  wrote:

>> 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`).
>
> I don't see the similarity.  That gcc bug is about literals used as addresses,
> which get treated (suggested inappropriately) as NULL+offset, with NULL+offset
> being a cause of warnings.  I don't see that happening in our case.  That is,
> in our `addr + low_offset`, `addr` doesn't seem to be either a literal or NULL
> that I can see.
> 
> It's hard for me to investigate this any further just by perusing the code, so
> I'm in the process of getting set up to build with gcc12.x.  That will let me
> do some experiments. It may take me a few days to get to that point though.

I spent some time looking into this one. I agree there is a false positive
here, and there doesn't seem to be a better solution than suppressing the
warning. I would prefer the change below, rather than what's proposed. Also
eliminate the changes to utilities/compilerWarnings files. This is a very
gcc-specific issue; there's no reason to generalize the solution.  The reason
for relocating the suppression is to be able to describe the issue in more
detail in a context where that description makes sense.

template 
inline void JfrTraceIdBits::store(jbyte bits, const T* ptr) {
  assert(ptr != NULL, "invariant");
  // gcc12 warns "writing 1 byte into a region of size 0" when T == Klass.
  // The warning seems to be a false positive.  And there is no warning for
  // other types that use the same mechanisms.  The warning also sometimes
  // goes away with minor code perturbations, such as replacing function calls
  // with equivalent code directly inlined.
  PRAGMA_DIAG_PUSH
  PRAGMA_DISABLE_GCC_WARNING("-Wstringop-overflow")
  set(bits, traceid_tag_byte(ptr));
  PRAGMA_DIAG_POP
}

-

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

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

[Kim Barrett]

On Sun, 22 May 2022 08:40:28 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 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 requested by kbarrett (Reviewer).

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());
}

-

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

Integrated: 8286262: Windows: Cleanup deprecation warning suppression

[Kim Barrett]

On Sun, 15 May 2022 20:50:25 GMT, Kim Barrett  wrote:

> Please review this cleanup of deprecation warning suppression when building
> for Windows.
> 
> This change consists of several parts.
> 
> (1) Remove the global deprecation warning suppression when building HotSpot
> for Windows.
> 
> (2) Add macro definitions requesting suppression of selected sets of
> deprecation warnings when building HotSpot for Windows.
> 
> (3) Remove unnecessary forwarding macros for various POSIX functions in
> globalDefinitions_visCPP.hpp.  These were provided to avoid deprecation
> warnings (that were previously also being suppressed by the global request).
> They are now covered by the new macros provided by change (2) above.
> 
> An alternative to item (3) is to not define _CRT_NONSTDC_NO_DEPRECATE (in item
> (2)) and either retain the forwarding macros or define os:: wrapper functions
> for all of the affected functions.  We might eventually do the latter because
> of other reasons for avoiding some of these functions, but the approach being
> taken here is simpler.
> 
> For documentation of _CRT_NONSTDC_NO_DEPRECATE, see:
> https://docs.microsoft.com/en-us/cpp/c-runtime-library/compatibility
> https://docs.microsoft.com/en-us/cpp/error-messages/compiler-warnings/compiler-warning-level-3-c4996
> 
> Similarly for _CRT_SECURE_NO_WARNINGS.
> 
> Perhaps similarly for _WINSOCK_DEPRECATED_NO_WARNINGS (though I didn't find
> any documentation for the latter).  But it might be better to not supress the
> warnings and instead use the alternatives (JDK-8286781).
> 
> Testing:
> mach5 tier1

This pull request has now been integrated.

Changeset: 782ae380
Author:Kim Barrett 
URL:   
https://git.openjdk.java.net/jdk/commit/782ae3801c63945ed977782fe15e8e911f7f9656
Stats: 21 lines in 4 files changed: 2 ins; 13 del; 6 mod

8286262: Windows: Cleanup deprecation warning suppression

Reviewed-by: ihse, dholmes

-

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

Re: RFR: 8286262: Windows: Cleanup deprecation warning suppression [v2]

[Kim Barrett]

> Please review this cleanup of deprecation warning suppression when building
> for Windows.
> 
> This change consists of several parts.
> 
> (1) Remove the global deprecation warning suppression when building HotSpot
> for Windows.
> 
> (2) Add macro definitions requesting suppression of selected sets of
> deprecation warnings when building HotSpot for Windows.
> 
> (3) Remove unnecessary forwarding macros for various POSIX functions in
> globalDefinitions_visCPP.hpp.  These were provided to avoid deprecation
> warnings (that were previously also being suppressed by the global request).
> They are now covered by the new macros provided by change (2) above.
> 
> An alternative to item (3) is to not define _CRT_NONSTDC_NO_DEPRECATE (in item
> (2)) and either retain the forwarding macros or define os:: wrapper functions
> for all of the affected functions.  We might eventually do the latter because
> of other reasons for avoiding some of these functions, but the approach being
> taken here is simpler.
> 
> For documentation of _CRT_NONSTDC_NO_DEPRECATE, see:
> https://docs.microsoft.com/en-us/cpp/c-runtime-library/compatibility
> https://docs.microsoft.com/en-us/cpp/error-messages/compiler-warnings/compiler-warning-level-3-c4996
> 
> Similarly for _CRT_SECURE_NO_WARNINGS.
> 
> Perhaps similarly for _WINSOCK_DEPRECATED_NO_WARNINGS (though I didn't find
> any documentation for the latter).  But it might be better to not supress the
> warnings and instead use the alternatives (JDK-8286781).
> 
> Testing:
> mach5 tier1

Kim Barrett 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 two additional commits since the 
last revision:

 - Merge branch 'master' into no-deprecate
 - cleanup Windows deprecation warning suppression

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/8718/files
  - new: https://git.openjdk.java.net/jdk/pull/8718/files/dd39fb83..cbee140c

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

  Stats: 27751 lines in 853 files changed: 14074 ins; 9711 del; 3966 mod
  Patch: https://git.openjdk.java.net/jdk/pull/8718.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/8718/head:pull/8718

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

Re: RFR: 8286262: Windows: Cleanup deprecation warning suppression

[Kim Barrett]

On Tue, 17 May 2022 06:30:03 GMT, David Holmes  wrote:

>> Please review this cleanup of deprecation warning suppression when building
>> for Windows.
>> 
>> This change consists of several parts.
>> 
>> (1) Remove the global deprecation warning suppression when building HotSpot
>> for Windows.
>> 
>> (2) Add macro definitions requesting suppression of selected sets of
>> deprecation warnings when building HotSpot for Windows.
>> 
>> (3) Remove unnecessary forwarding macros for various POSIX functions in
>> globalDefinitions_visCPP.hpp.  These were provided to avoid deprecation
>> warnings (that were previously also being suppressed by the global request).
>> They are now covered by the new macros provided by change (2) above.
>> 
>> An alternative to item (3) is to not define _CRT_NONSTDC_NO_DEPRECATE (in 
>> item
>> (2)) and either retain the forwarding macros or define os:: wrapper functions
>> for all of the affected functions.  We might eventually do the latter because
>> of other reasons for avoiding some of these functions, but the approach being
>> taken here is simpler.
>> 
>> For documentation of _CRT_NONSTDC_NO_DEPRECATE, see:
>> https://docs.microsoft.com/en-us/cpp/c-runtime-library/compatibility
>> https://docs.microsoft.com/en-us/cpp/error-messages/compiler-warnings/compiler-warning-level-3-c4996
>> 
>> Similarly for _CRT_SECURE_NO_WARNINGS.
>> 
>> Perhaps similarly for _WINSOCK_DEPRECATED_NO_WARNINGS (though I didn't find
>> any documentation for the latter).  But it might be better to not supress the
>> warnings and instead use the alternatives (JDK-8286781).
>> 
>> Testing:
>> mach5 tier1
>
> Sorry Kim I'm having trouble seeing what change corresponds to (1) ??
> 
> Also the PR talks only about hotspot, but you're changing the JDK flags as 
> well. ??

Thanks @dholmes-ora and @magicus for reviews.

-

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

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

[Kim Barrett]

On Sun, 22 May 2022 08:45:48 GMT, Yasumasa Suenaga  wrote:

>> 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`).

I don't see the similarity.  That gcc bug is about literals used as addresses,
which get treated (suggested inappropriately) as NULL+offset, with NULL+offset
being a cause of warnings.  I don't see that happening in our case.  That is,
in our `addr + low_offset`, `addr` doesn't seem to be either a literal or NULL
that I can see.

It's hard for me to investigate this any further just by perusing the code, so
I'm in the process of getting set up to build with gcc12.x.  That will let me
do some experiments. It may take me a few days to get to that point though.

-

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

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

[Kim Barrett]

On Sun, 22 May 2022 08:35:54 GMT, Yasumasa Suenaga  wrote:

>> `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.

Good.  I see you found and used the existing `base_offset_in_bytes` (which I'd 
overlooked), rather than using my suggested `data_offset`.

-

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

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

[Kim Barrett]

On Tue, 17 May 2022 12:43:02 GMT, Yasumasa Suenaga  wrote:

>> 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:

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...

-

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

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

[Kim Barrett]

On Tue, 17 May 2022 12:38:55 GMT, Yasumasa Suenaga  wrote:

>> 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:

`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.

-

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

Re: RFR: 8286262: Windows: Cleanup deprecation warning suppression

[Kim Barrett]

On Tue, 17 May 2022 06:30:03 GMT, David Holmes  wrote:

> Sorry Kim I'm having trouble seeing what change corresponds to (1) ??

The change to CompileJvm.gmk, removing 4996 (deprecation warnings)
from the list of disabled warnings.

> Also the PR talks only about hotspot, but you're changing the JDK flags as 
> well. ??

I forgot to mention that in addition to reformatting the JDK flags a
little bit to be consistent with the JVM flags changes, I also changed
-D_CRT_SECURE_NO_DEPRECATE => -D_CRT_SECURE_NO_WARNINGS. Both are
(still) supported and have the same meaning, but the latter is the more
"modern" (circa VS2005-ish?) spelling.

-

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

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

[Kim Barrett]

On Tue, 17 May 2022 03:05:09 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/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.

-

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

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

[Kim Barrett]

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

Some suggestions for code changes instead of warning suppression.  And some 
warnings that I don't yet understand and don't think should be suppressed 
without more details or investigation.

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.

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.)

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.

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.

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.

-

Changes requested by kbarrett (Reviewer).

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

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

[Kim Barrett]

On Thu, 12 May 2022 18:28:02 GMT, Kim Barrett  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.
> 
> +1

> Can I get the review from HotSpot folks? @kimbarrett

Already working on it.  There are some I don't understand yet.

-

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

RFR: 8286262: Windows: Cleanup deprecation warning suppression

[Kim Barrett]

Please review this cleanup of deprecation warning suppression when building
for Windows.

This change consists of several parts.

(1) Remove the global deprecation warning suppression when building HotSpot
for Windows.

(2) Add macro definitions requesting suppression of selected sets of
deprecation warnings when building HotSpot for Windows.

(3) Remove unnecessary forwarding macros for various POSIX functions in
globalDefinitions_visCPP.hpp.  These were provided to avoid deprecation
warnings (that were previously also being suppressed by the global request).
They are now covered by the new macros provided by change (2) above.

An alternative to item (3) is to not define _CRT_NONSTDC_NO_DEPRECATE (in item
(2)) and either retain the forwarding macros or define os:: wrapper functions
for all of the affected functions.  We might eventually do the latter because
of other reasons for avoiding some of these functions, but the approach being
taken here is simpler.

For documentation of _CRT_NONSTDC_NO_DEPRECATE, see:
https://docs.microsoft.com/en-us/cpp/c-runtime-library/compatibility
https://docs.microsoft.com/en-us/cpp/error-messages/compiler-warnings/compiler-warning-level-3-c4996

Similarly for _CRT_SECURE_NO_WARNINGS.

Perhaps similarly for _WINSOCK_DEPRECATED_NO_WARNINGS (though I didn't find
any documentation for the latter).  But it might be better to not supress the
warnings and instead use the alternatives (JDK-8286781).

Testing:
mach5 tier1

-

Commit messages:
 - cleanup Windows deprecation warning suppression

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

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

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

[Kim Barrett]

On Thu, 12 May 2022 01:29:13 GMT, Yasumasa Suenaga  wrote:

>> 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.

> @YaSuenag From my PoV this sounds like a good suggestion.

+1

-

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

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

[Kim Barrett]

On Wed, 11 May 2022 13:56:44 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/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).

-

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

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

[Kim Barrett]

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

Changes requested by kbarrett (Reviewer).

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.

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.

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!

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.

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.

-

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

Re: RFR: 8204541: Correctly support AIX xlC 16.1 symbol visibility flags [v2]

[Kim Barrett]

> On Mar 18, 2022, at 4:11 AM, Thomas Stuefe  wrote:
> 
> On Fri, 18 Mar 2022 08:03:11 GMT, Ichiroh Takiguchi  
> wrote:
> 
>>> This change is just for AIX platform with IBM XL C/C++ 16.1.
>>> By this change, lib's entry points are reduced by compilation and linkage 
>>> option changes.
>>> 
>>> Without changes:
>>> 
>>> $ dump -X32_64 -Tv lib/libawt.so | grep EXP | wc
>>> 914
>>> 
>>> With changes
>>> 
>>> $ dump -X32_64 -Tv lib/libawt.so | grep EXP | wc
>>> 142
>> 
>> Ichiroh Takiguchi has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>  Remove unexpected comment
> 
> So, does this mean we cannot build with older xlC versions anymore? Just 
> curious.

XLC16 (xlclang/xlclang++) is needed for C++11/14 support; the older versions
of XLC don't support the newer language standards.  So XLC16 is required for
for JDK16+.

> Pinging @backwaterred .
> 
> -
> 
> PR: https://git.openjdk.java.net/jdk/pull/7862

Re: RFR: 8253495: CDS generates non-deterministic output [v6]

[Kim Barrett]

On Fri, 11 Mar 2022 06:55:23 GMT, Ioi Lam  wrote:

>> This patch makes the result of "java -Xshare:dump" deterministic:
>> - Disabled new Java threads from launching. This is harmless. See comments 
>> in jvm.cpp
>> - Fixed a problem in hashtable ordering in heapShared.cpp
>> - BasicHashtableEntry has a gap on 64-bit platforms that may contain random 
>> bits. Added code to zero it.
>> - Enabled checking of $JAVA_HOME/lib/server/classes.jsa in 
>> make/scripts/compare.sh
>> 
>> Note:  $JAVA_HOME/lib/server/classes_ncoops.jsa is still non-deterministic. 
>> This will be fixed in 
>> [JDK-8282828](https://bugs.openjdk.java.net/browse/JDK-8282828).
>> 
>> Testing under way:
>> - tier1~tier5
>> - Run all *-cmp-baseline jobs 20 times each (linux-aarch64-cmp-baseline, 
>> windows-x86-cmp-baseline,  etc).
>
> Ioi Lam has updated the pull request incrementally with one additional commit 
> since the last revision:
> 
>   Added helper function CollectedHeap::zap_filler_array_with

GC changes look good.

-

Marked as reviewed by kbarrett (Reviewer).

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

Re: RFR: 8253495: CDS generates non-deterministic output [v4]

[Kim Barrett]

On Fri, 11 Mar 2022 04:56:23 GMT, Ioi Lam  wrote:

>> This patch makes the result of "java -Xshare:dump" deterministic:
>> - Disabled new Java threads from launching. This is harmless. See comments 
>> in jvm.cpp
>> - Fixed a problem in hashtable ordering in heapShared.cpp
>> - BasicHashtableEntry has a gap on 64-bit platforms that may contain random 
>> bits. Added code to zero it.
>> - Enabled checking of $JAVA_HOME/lib/server/classes.jsa in 
>> make/scripts/compare.sh
>> 
>> Note:  $JAVA_HOME/lib/server/classes_ncoops.jsa is still non-deterministic. 
>> This will be fixed in 
>> [JDK-8282828](https://bugs.openjdk.java.net/browse/JDK-8282828).
>> 
>> Testing under way:
>> - tier1~tier5
>> - Run all *-cmp-baseline jobs 20 times each (linux-aarch64-cmp-baseline, 
>> windows-x86-cmp-baseline,  etc).
>
> Ioi Lam has updated the pull request incrementally with one additional commit 
> since the last revision:
> 
>   zero GC heap filler arrays

Changes requested by kbarrett (Reviewer).

src/hotspot/share/gc/shared/collectedHeap.cpp line 449:

> 447:   allocator.initialize(start);
> 448:   DEBUG_ONLY(zap_filler_array(start, words, zap);)
> 449:   if (DumpSharedSpaces) {

Probably shouldn't both zap and clear for dumping, to avoid wasting time.

-

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

RFR: 8257589: HotSpot Style Guide should link to rfc7282

[Kim Barrett]

Please review this change to the link for the definition of "rough consensus".
The current link is to a Wikipedia article that references rfc7282.  We should
instead link directly the the RFC.  This change was requested during the
review of JDK-8247976, but not made at that time. (I'm not sure whether it was
intentionally deferred or missed/forgotten.)

As this is a HotSpot Style Guide change, it requires reviewers who are HotSpot
Group Members, though comments from others are welcome.

-

Commit messages:
 - update html
 - update rough consensus link

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

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

RFR: 8252577: HotSpot Style Guide should link to One-True-Brace-Style description

[Kim Barrett]

Please review this change to provide a link to the Wikipedia description of
One-True-Brace-Style.

As this is a HotSpot Style Guide change, it requires reviewers who are HotSpot
Group Members, though comments from others are welcome.

-

Commit messages:
 - update html
 - add OTBS link

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

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

RFR: 8280916: Simplify HotSpot Style Guide editorial changes

[Kim Barrett]

Please review this change to the HotSpot Style Guide change process.

The current process involves gathering consensus among the HotSpot Group
Members.  That's fine for changes of substance.  But it seems overly weighty
for editorial changes that don't affect the substance of the guide, but only
it's clarity or accuracy.

The proposed change would permit the normal PR process to be used for such
changes, but require the requisite reviewers to additionally be HotSpot Group
Members.

Note that there have already been a couple of changes that effectively
followed the proposed new process.
https://bugs.openjdk.java.net/browse/JDK-8274169
https://bugs.openjdk.java.net/browse/JDK-8280182

This is a modification of the Style Guide, so rough consensus among the
HotSpot Group members is required to make this change. Only Group members
should vote for approval (via the github PR), though reasoned objections or
comments from anyone will be considered. A decision on this proposal will not
be made before Monday 14-Feb-2022 at 12h00 UTC.

Since we're piggybacking on github PRs here, please use the PR review process
to approve (click on Review Changes > Approve), rather than sending a "vote:
yes" email reply that would be normal for a CFV.

-

Commit messages:
 - update generated html
 - editorial change process

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

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

Withdrawn: 8280916: Simplify HotSpot Style Guide editorial changes

[Kim Barrett]

On Sun, 30 Jan 2022 00:28:59 GMT, Kim Barrett  wrote:

> Please review this change to the HotSpot Style Guide change process.
> 
> The current process involves gathering consensus among the HotSpot Group
> Members.  That's fine for changes of substance.  But it seems overly weighty
> for editorial changes that don't affect the substance of the guide, but only
> it's clarity or accuracy.
> 
> The proposed change would permit the normal PR process to be used for such
> changes, but require the requisite reviewers to additionally be HotSpot Group
> Members.
> 
> Note that there have already been a couple of changes that effectively
> followed the proposed new process.
> https://bugs.openjdk.java.net/browse/JDK-8274169
> https://bugs.openjdk.java.net/browse/JDK-8280182

This pull request has been closed without being integrated.

-

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

Re: RFR: 8280916: Simplify HotSpot Style Guide editorial changes

[Kim Barrett]

On Sun, 30 Jan 2022 00:28:59 GMT, Kim Barrett  wrote:

> Please review this change to the HotSpot Style Guide change process.
> 
> The current process involves gathering consensus among the HotSpot Group
> Members.  That's fine for changes of substance.  But it seems overly weighty
> for editorial changes that don't affect the substance of the guide, but only
> it's clarity or accuracy.
> 
> The proposed change would permit the normal PR process to be used for such
> changes, but require the requisite reviewers to additionally be HotSpot Group
> Members.
> 
> Note that there have already been a couple of changes that effectively
> followed the proposed new process.
> https://bugs.openjdk.java.net/browse/JDK-8274169
> https://bugs.openjdk.java.net/browse/JDK-8280182

I messed up the PR submission for this.  Closing and will open a new one.

-

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

RFR: 8280916: Simplify HotSpot Style Guide editorial changes

[Kim Barrett]

Please review this change to the HotSpot Style Guide change process.

The current process involves gathering consensus among the HotSpot Group
Members.  That's fine for changes of substance.  But it seems overly weighty
for editorial changes that don't affect the substance of the guide, but only
it's clarity or accuracy.

The proposed change would permit the normal PR process to be used for such
changes, but require the requisite reviewers to additionally be HotSpot Group
Members.

Note that there have already been a couple of changes that effectively
followed the proposed new process.
https://bugs.openjdk.java.net/browse/JDK-8274169
https://bugs.openjdk.java.net/browse/JDK-8280182

-

Commit messages:
 - update generated html
 - editorial change process

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

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

Re: RFR: 8280182: HotSpot Style Guide has stale link to chromium style guide

[Kim Barrett]

On Tue, 18 Jan 2022 23:09:12 GMT, Liam Miller-Cushon  wrote:

> Update links to the chromium style guide in the HotSpot Style Guide.

The current change process for the style guide requires voting by HotSpot
group members and a decision by the group lead (Vladimir). That seems overly
heavyweight for small editorial changes like this. I've been meaning to
propose a change to the change process, but haven't gotten around to it.  (And
I have several similar editorial changes pending.)

-

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

Re: RFR: JDK-8276422 Add command-line option to disable finalization

[Kim Barrett]

On Thu, 18 Nov 2021 01:34:36 GMT, Stuart Marks  wrote:

> Pretty much what it says. The new option controls a static member in 
> InstanceKlass that's consulted to determine whether the finalization 
> machinery is activated for instances when a class is loaded. A new native 
> method is added so that this state can be queried from Java. This is used to 
> control whether a finalizer thread is created and to disable the `System` and 
> `Runtime::runFinalization` methods. Includes tests for the above.

I only really reviewed the hotspot changes.

There is nothing here to make the various GCs take advantage of finalization 
being disabled.  Is the plan to leave that to followup changes?

src/hotspot/share/oops/instanceKlass.hpp line 338:

> 336: 
> 337:   // Queries finalization state
> 338:   static bool finalization_enabled() { return _finalization_enabled; }

Predicate functions like this are often named "is_xxx"; that idiom is common in 
this class.

src/hotspot/share/prims/jvm.cpp line 694:

> 692: 
> 693: JVM_ENTRY(jboolean, JVM_IsFinalizationEnabled(JNIEnv * env))
> 694: return InstanceKlass::finalization_enabled() ? JNI_TRUE : JNI_FALSE;

missing indentation

-

Changes requested by kbarrett (Reviewer).

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

Re: RFR: JDK-8276422 Add command-line option to disable finalization

[Kim Barrett]

On Thu, 18 Nov 2021 06:43:01 GMT, Kim Barrett  wrote:

>> Pretty much what it says. The new option controls a static member in 
>> InstanceKlass that's consulted to determine whether the finalization 
>> machinery is activated for instances when a class is loaded. A new native 
>> method is added so that this state can be queried from Java. This is used to 
>> control whether a finalizer thread is created and to disable the `System` 
>> and `Runtime::runFinalization` methods. Includes tests for the above.
>
> src/hotspot/share/prims/jvm.cpp line 694:
> 
>> 692: 
>> 693: JVM_ENTRY(jboolean, JVM_IsFinalizationEnabled(JNIEnv * env))
>> 694: return InstanceKlass::finalization_enabled() ? JNI_TRUE : JNI_FALSE;
> 
> missing indentation

I think this could just be `return InstanceKlass::finalization_enabled();`.  
There is lots of code in this file and elsewhere that assumes C++ `bool` 
converts to `jboolean` appropriately.

-

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

Re: RFR: 8270083: -Wnonnull errors happen with GCC 11.1.1

[Kim Barrett]

On Fri, 9 Jul 2021 00:28:38 GMT, Yasumasa Suenaga  wrote:

> It is better if we can treat registers as a class **normally**. Similar 
> change happens for markWord in 
> [JDK-8229258](https://bugs.openjdk.java.net/browse/JDK-8229258). Should we 
> change like now?

The potential difficulty with doing that is the same as for markWord - see 
[JDK-8235362](https://bugs.openjdk.java.net/browse/JDK-8235362).  A different 
option might be to make the various categories of Register be enum classes 
without any defined enumerators.  That wouldn't support the existing 
XXXRegisterImpl derivation from AbstractRegisterImpl.  I'm not sure that 
relationship is needed; it may not even be good.  The only use I can find is in 
the various assert_different_registers, and that could be done differently.  
(The numerous overloads could also probably be simplified using variadic 
templates.)

-

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

Re: RFR: 8270083: -Wnonnull errors happen with GCC 11.1.1

[Kim Barrett]

On Thu, 8 Jul 2021 09:42:43 GMT, Yasumasa Suenaga  wrote:

> I attempted to build OpenJDK on Fedora 34 with gcc-11.1.1-3.fc34.x86_64, but 
> I saw following errors:
> 
> 
> In file included from 
> /home/ysuenaga/github-forked/jdk/src/hotspot/share/runtime/frame.inline.hpp:42,
>  from 
> /home/ysuenaga/github-forked/jdk/src/hotspot/cpu/x86/abstractInterpreter_x86.cpp:29:

I'm conditionally approving this as a temporary workaround.  A followup RFE to 
improve things should be filed.

-

Marked as reviewed by kbarrett (Reviewer).

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

Re: RFR: 8270083: -Wnonnull errors happen with GCC 11.1.1

[Kim Barrett]

On Thu, 8 Jul 2021 09:42:43 GMT, Yasumasa Suenaga  wrote:

> I attempted to build OpenJDK on Fedora 34 with gcc-11.1.1-3.fc34.x86_64, but 
> I saw following errors:
> 
> 
> In file included from 
> /home/ysuenaga/github-forked/jdk/src/hotspot/share/runtime/frame.inline.hpp:42,
>  from 
> /home/ysuenaga/github-forked/jdk/src/hotspot/cpu/x86/abstractInterpreter_x86.cpp:29:

This _might_ be good enough as a short-term workaround, but it should not be 
considered a long-term fix.  The underlying problem really needs to be 
addressed properly.

-

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

Re: RFR: 8258010: Debug build failure with clang-10 due to -Wdeprecated-copy [v5]

[Kim Barrett]

On Mon, 11 Jan 2021 02:14:17 GMT, Hao Sun 
 wrote:

>> 1. '-Wdeprecated-copy'
>> As specified in C++11 [1], "the generation of the implicitly-defined
>> copy constructor is deprecated if T has a user-defined destructor or
>> user-defined copy assignment operator". The rationale behind is the
>> well-known Rule of Three [2].
>> 
>> Introduced since gcc-9 [3] and clang-10 [4], flag '-Wdeprecated-copy'
>> warns about the C++11 deprecation of implicitly declared copy
>> constructor and assignment operator if one of them is user-provided.
>> Defining an explicit copy constructor would suppress this warning.
>> 
>> The main reason why debug build with gcc-9 or higher succeeds lies in
>> the inconsistent warning behaviors between gcc and clang. See the
>> reduced code example [5]. We suspect it might be return value
>> optimization/copy elision [6] that drives gcc not to declare implicit
>> copy constructor for this case.
>> 
>> Note that flag '-Wdeprecated' in clang-8 and clang-9 would also raise
>> warnings for deprecated defintions of copy constructors. However,
>> '-Wdeprecated' is not enabled by '-Wall' or '-Wextra'. Hence, clang-8
>> and clang-9 are not affected.
>> 
>> ~~2. '-Wimplicit-int-float-conversion'~~
>> ~~Making the conversion explicit would fix it.~~
>> 
>> ~~Flag '-Wimplicit-int-float-conversion' is first introduced in clang-10.~~
>> ~~Therefore clang-8 and clang-9 are not affected. The flag with similar~~
>> ~~functionality in gcc is '-Wfloat-conversion', but it is not enabled by~~
>> ~~'-Wall' or '-Wextra'. That's why this warning does not apprear when~~
>> ~~building with gcc.~~
>> 
>> [1] https://en.cppreference.com/w/cpp/language/copy_constructor
>> [2] https://en.cppreference.com/w/cpp/language/rule_of_three
>> [3] https://www.gnu.org/software/gcc/gcc-9/changes.html
>> [4] https://releases.llvm.org/10.0.0/tools/clang/docs/ReleaseNotes.html
>> [5] https://godbolt.org/z/err4jM
>> [6] https://en.wikipedia.org/wiki/Copy_elision#Return_value_optimization
>> 
>> 
>> Note that we have tested with this patch, debug build succeeded with 
>> clang-10 on Linux X86-64/AArch64 machines.
>> Note that '--with-extra-cxxflags=-Wno-implicit-int-float-conversion' should 
>> be added when configuration. It's another issue (See JDK-8259288)
>
> Hao Sun has updated the pull request incrementally with one additional commit 
> since the last revision:
> 
>   Define the copy assign operator of class DUIterator_Last as defaulted
>   
>   The copy assignment operator of class DUIterator_Last should also be
>   defined as defaulted, i.e. =default, keeping consistent with the copy
>   constructor.
>   
>   Besides, fix the NIT for the copy ctor definition of class
>   DUIterator_Last.
>   
>   Change-Id: I2f9502f023443163910eea9469b72df5bf1e25e0
>   CustomizedGitHooks: yes

Marked as reviewed by kbarrett (Reviewer).

-

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

Re: RFR: 8258010: Debug build failure with clang-10 due to -Wdeprecated-copy [v4]

[Kim Barrett]

On Wed, 6 Jan 2021 06:14:11 GMT, Hao Sun 
 wrote:

>> 1. '-Wdeprecated-copy'
>> As specified in C++11 [1], "the generation of the implicitly-defined
>> copy constructor is deprecated if T has a user-defined destructor or
>> user-defined copy assignment operator". The rationale behind is the
>> well-known Rule of Three [2].
>> 
>> Introduced since gcc-9 [3] and clang-10 [4], flag '-Wdeprecated-copy'
>> warns about the C++11 deprecation of implicitly declared copy
>> constructor and assignment operator if one of them is user-provided.
>> Defining an explicit copy constructor would suppress this warning.
>> 
>> The main reason why debug build with gcc-9 or higher succeeds lies in
>> the inconsistent warning behaviors between gcc and clang. See the
>> reduced code example [5]. We suspect it might be return value
>> optimization/copy elision [6] that drives gcc not to declare implicit
>> copy constructor for this case.
>> 
>> Note that flag '-Wdeprecated' in clang-8 and clang-9 would also raise
>> warnings for deprecated defintions of copy constructors. However,
>> '-Wdeprecated' is not enabled by '-Wall' or '-Wextra'. Hence, clang-8
>> and clang-9 are not affected.
>> 
>> ~~2. '-Wimplicit-int-float-conversion'~~
>> ~~Making the conversion explicit would fix it.~~
>> 
>> ~~Flag '-Wimplicit-int-float-conversion' is first introduced in clang-10.~~
>> ~~Therefore clang-8 and clang-9 are not affected. The flag with similar~~
>> ~~functionality in gcc is '-Wfloat-conversion', but it is not enabled by~~
>> ~~'-Wall' or '-Wextra'. That's why this warning does not apprear when~~
>> ~~building with gcc.~~
>> 
>> [1] https://en.cppreference.com/w/cpp/language/copy_constructor
>> [2] https://en.cppreference.com/w/cpp/language/rule_of_three
>> [3] https://www.gnu.org/software/gcc/gcc-9/changes.html
>> [4] https://releases.llvm.org/10.0.0/tools/clang/docs/ReleaseNotes.html
>> [5] https://godbolt.org/z/err4jM
>> [6] https://en.wikipedia.org/wiki/Copy_elision#Return_value_optimization
>> 
>> 
>> Note that we have tested with this patch, debug build succeeded with 
>> clang-10 on Linux X86-64/AArch64 machines.
>> Note that '--with-extra-cxxflags=-Wno-implicit-int-float-conversion' should 
>> be added when configuration. It's another issue (See JDK-8259288)
>
> Hao Sun has updated the pull request incrementally with one additional commit 
> since the last revision:
> 
>   Split the PR, addressing -Wdeprecated-copy only
>   
>   As suggested by kimbarrett, we should focus on warnings produced by
>   '-Wdeprecated-copy' in this PR. Because JDK-8259288 is a very different
>   problem and might be reviewed by folks from different teams.
>   
>   Will create a new PR to address JDK-8259288.
>   
>   Change-Id: I1b9f434ab6fcdf2763a46870eaed91641984fd76
>   CustomizedGitHooks: yes

[Can't comment on this inline.] I'd prefer DUIterator_Last::operator= be 
changed to =default, for consistency with the copy constructor.  That would 
require fixing the return type too.

src/hotspot/share/opto/node.hpp line 1461:

> 1459:   // initialize to garbage
> 1460: 
> 1461:   DUIterator_Last (const DUIterator_Last& that) = default;

Nit - remove space before parameter list.

-

Changes requested by kbarrett (Reviewer).

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

Re: RFR: 8258010: Debug build failure with clang-10 due to -Wdeprecated-copy and -Wimplicit-int-float-conversion [v2]

[Kim Barrett]

On Mon, 4 Jan 2021 06:59:54 GMT, Hao Sun 
 wrote:

>> src/hotspot/share/opto/node.hpp line 1396:
>> 
>>> 1394: 
>>> 1395:   DUIterator_Fast(const DUIterator_Fast& that)
>>> 1396: { _outp = that._outp;   debug_only(reset(that)); }
>> 
>> `reset` tests `_vdui`, but nothing here has set it, so it's uninitialized 
>> and that test wil be UB.
>> 
>> I'm also not sure why it's okay for `operator=` to use whatever the current 
>> value of `_vdui` might be; that could leave `_last` as the old value rather 
>> than refreshing from `that`, which seems wrong.  This is aabout pre-existing 
>> code that looks questionable to me.
>
> I suppose the constructor would be invoked before the copy assignment 
> operator. That is `_vdui` gets initialized already in the ctor 
> `DUIterator_Fast()` for `operator=` case. Right?
> Take the following code snippet as an example.
> DUIterator_Fast t1, t2;
> t2 = t1;// assignment operator
> DUIterator_Fast t3 = t1;  // copy constructor. same as "t3(t1)"
> My point is that, the ctor for `t2` has already invoked, i.e. initializing 
> `_vdui` as false. That's why `operator=` works well.
> 
> 
> Yes. For our newly-added copy constructor for `DUIterator_Fast`, we should 
> initialize `_vdui` as `false`. It may be defined as below.
> DUIterator_Fast(const DUIterator_Fast& that)
> { _outp = that._outp;   debug_only(_vdui = false; 
> reset(that)); }

That's true on the first assignment of `t2`. But what if `t2` is reassigned
to some other iterator. That assignment sees `_vdui` true, and keeps the old
value of `_last` rather than updating the value from that other iterator. Is
that really correct? It certainly seems strange. I'm trying to find someone
who understands this code to get involved, but holidays.

-

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

Re: RFR: 8258010: Debug build failure with clang-10 due to -Wdeprecated-copy and -Wimplicit-int-float-conversion [v2]

[Kim Barrett]

> On Jan 3, 2021, at 11:33 PM, Hao Sun 
>  wrote:
> 
> On Mon, 4 Jan 2021 01:18:47 GMT, Hao Sun 
>  wrote:
> 
>>>> _Mailing list message from [Kim Barrett](mailto:kim.barr...@oracle.com) on 
>>>> [build-dev](mailto:build-dev@openjdk.java.net):_
>> 
>> It appears that either clang is different from gcc for -felide-constructors
>> (which seems unlikely), or clang (clang-10) is doing the deprecation warning
>> at a different point from gcc (quite plausible). That is, clang could be
>> checking for the deprecated case before eliding the call, while gcc is
>> checking for the deprecated case after copy elision has been applied.
> 
> Thanks for your reply.
> I checked the source code of clang-10 and gcc-9 and found that:
> 
> 1) for clang-10,
> 'Wdeprecated-copy' is implemented at the 'Sema' module of clang. See 
> https://github.com/llvm/llvm-project/blob/release/10.x/clang/lib/Sema/SemaDeclCXX.cpp#L13585
> 
> Flag 'felide-constructors' would enable 'felide_constructors' and flag 
> 'fno-elide-constructors' would enables 'fno_elide_constructors'. (See 
> https://github.com/llvm/llvm-project/blob/release/10.x/clang/include/clang/Driver/Options.td).
> Then 'ElideConstructors' will be set (See 
> https://github.com/llvm/llvm-project/blob/release/10.x/clang/lib/Frontend/CompilerInvocation.cpp#L2863)
> Finally, constructors might be elided in several spots in 'CodeGen' module. 
> See: 
> https://github.com/llvm/llvm-project/blob/release/10.x/clang/lib/CodeGen/CGStmt.cpp#L1094
> https://github.com/llvm/llvm-project/blob/release/10.x/clang/lib/CodeGen/CGExprCXX.cpp#L611
> https://github.com/llvm/llvm-project/blob/release/10.x/clang/lib/CodeGen/CGDecl.cpp#L1405
> 
> As far as I know, 'Sema' module is conducted to check semantics errors before 
> 'CodeGen' module.
> That verified your conjecture, i.e. 'clang could be checking for the 
> derepcated case before eliding the call'.
> 
> 2) for gcc-9,
> 'felide-constructors' and 'Wdeprecated-copy' are implemented in a number of 
> spots in gcc. I currently didn't figure out their execution order clearly.
> 
> But in one of the use points at function build_over_call(), 
> 'flag_elide_constructors' (See 
> https://github.com/gcc-mirror/gcc/blob/releases/gcc-9/gcc/cp/call.c#L8538) is 
> handled **before** 'warn_deprecated_copy' (See 
> https://github.com/gcc-mirror/gcc/blob/releases/gcc-9/gcc/cp/call.c#L8608 and 
> https://github.com/gcc-mirror/gcc/blob/releases/gcc-9/gcc/cp/call.c#L8679)
> 
> Hope that my finding is helpful. Thanks.

Yes, that would explain the different warning behaviors.  Thanks for digging 
through that.

Re: RFR: 8258010: Debug build failure with clang-10 due to -Wdeprecated-copy and -Wimplicit-int-float-conversion [v2]

[Kim Barrett]

On Mon, 28 Dec 2020 10:28:09 GMT, Hao Sun 
 wrote:

>> 1. '-Wdeprecated-copy'
>> As specified in C++11 [1], "the generation of the implicitly-defined
>> copy constructor is deprecated if T has a user-defined destructor or
>> user-defined copy assignment operator". The rationale behind is the
>> well-known Rule of Three [2].
>> 
>> Introduced since gcc-9 [3] and clang-10 [4], flag '-Wdeprecated-copy'
>> warns about the C++11 deprecation of implicitly declared copy
>> constructor and assignment operator if one of them is user-provided.
>> Defining an explicit copy constructor would suppress this warning.
>> 
>> The main reason why debug build with gcc-9 or higher succeeds lies in
>> the inconsistent warning behaviors between gcc and clang. See the
>> reduced code example [5]. We suspect it might be return value
>> optimization/copy elision [6] that drives gcc not to declare implicit
>> copy constructor for this case.
>> 
>> Note that flag '-Wdeprecated' in clang-8 and clang-9 would also raise
>> warnings for deprecated defintions of copy constructors. However,
>> '-Wdeprecated' is not enabled by '-Wall' or '-Wextra'. Hence, clang-8
>> and clang-9 are not affected.
>> 
>> 2. '-Wimplicit-int-float-conversion'
>> Making the conversion explicit would fix it.
>> 
>> Flag '-Wimplicit-int-float-conversion' is first introduced in clang-10.
>> Therefore clang-8 and clang-9 are not affected. The flag with similar
>> functionality in gcc is '-Wfloat-conversion', but it is not enabled by
>> '-Wall' or '-Wextra'. That's why this warning does not apprear when
>> building with gcc.
>> 
>> [1] https://en.cppreference.com/w/cpp/language/copy_constructor
>> [2] https://en.cppreference.com/w/cpp/language/rule_of_three
>> [3] https://www.gnu.org/software/gcc/gcc-9/changes.html
>> [4] https://releases.llvm.org/10.0.0/tools/clang/docs/ReleaseNotes.html
>> [5] https://godbolt.org/z/err4jM
>> [6] https://en.wikipedia.org/wiki/Copy_elision#Return_value_optimization
>> 
>> 
>> Note that we have tested with this patch, debug build succeeded with 
>> clang-10 on Linux X86/AArch64 machines.
>
> Hao Sun has updated the pull request incrementally with one additional commit 
> since the last revision:
> 
>   Remove the unused assignment operator for DUIterator_Last
>   
>   Instead of adding the copy constructor, remove the assignment operator
>   of DUIterator_Last since it's never used.
>   
>   Change-Id: Idf5658e38861eb2b0e724b064d17e9ab4e93905f
>   CustomizedGitHooks: yes

src/hotspot/share/opto/node.hpp line 1396:

> 1394: 
> 1395:   DUIterator_Fast(const DUIterator_Fast& that)
> 1396: { _outp = that._outp;   debug_only(reset(that)); }

`reset` tests `_vdui`, but nothing here has set it, so it's uninitialized and 
that test wil be UB.

I'm also not sure why it's okay for `operator=` to use whatever the current 
value of `_vdui` might be; that could leave `_last` as the old value rather 
than refreshing from `that`, which seems wrong.  This is aabout pre-existing 
code that looks questionable to me.

-

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

Re: RFR: 8258010: Debug build failure with clang-10 due to -Wdeprecated-copy and -Wimplicit-int-float-conversion [v2]

[Kim Barrett]

> On Jan 2, 2021, at 11:17 PM, Kim Barrett  wrote:
> 
>> On Dec 29, 2020, at 10:33 PM, Hao Sun 
>>  wrote:
>> 
>>> _Mailing list message from [Kim Barrett](mailto:kim.barr...@oracle.com) on 
>>> [build-dev](mailto:build-dev@openjdk.java.net):_
>>> But I discovered something alarming while experimenting. Building
>>> with gcc10.2 with -fno-elide-constructors doesn't seem to be possible.
>>> I get different kinds of failures depending on how DUIterator is
>>> defined:
>>> 
>>> - implict: deprecation warning (as expected)
>>> - =delete: error, deleted function used
>>> - =default: assert in os::free
>>> - _idx and reset from that: assert in reset
>>> 
>>> Without -fno-elide-constructors, all of the variants seem to work
>>> except =delete, which still fails because the deleted function is
>>> used. (I didn't test the "working" cases extensively though.)
>>> 
>>> So there's something problematic, though I don't understand the code
>>> well enough to understand what.
>> 
>> Thanks for your tests. 
>> But I have no idea how to fix it right now either.
>> Do you know anyone who is familiar with these code and maybe we can invite 
>> him/her to help take a look at this issue?
>> Thanks.
> 
> I have a suspicion that the assert failure when building with
> -fno-elide-constructors has nothing to do with DUIterator stuff, and is
> instead a problem elsewhere.  But it certainly makes it hard to feel
> confident that the additional constructors being added are correct.
> 
> I'm going to try to do some investigating of that assert failure, and see if
> I can figure out what's going on. Anyone else should feel free to join in.
> The failure is
> 
> #  Internal Error (../../src/hotspot/share/runtime/thread.hpp:850), 
> pid=29939, tid=29939
> #  assert(current != __null) failed: Thread::current() called on detached 
> thread

The assert failure when building with -fno-elide-constructors is a known issue:
https://bugs.openjdk.java.net/browse/JDK-8234773

I thought it looked vaguely familiar, but it took a while to track down.  I
think I'm going to deal with it so nobody runs into it again.

Re: RFR: 8258010: Debug build failure with clang-10 due to -Wdeprecated-copy and -Wimplicit-int-float-conversion [v2]

[Kim Barrett]

> On Dec 29, 2020, at 10:33 PM, Hao Sun 
>  wrote:
> 
>> _Mailing list message from [Kim Barrett](mailto:kim.barr...@oracle.com) on 
>> [build-dev](mailto:build-dev@openjdk.java.net):_
>> 
>> 
>> C++17 "guaranteed copy elision" is implemented in gcc7.
>> http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2016/p0135r1.html
>> 
> 
> Thanks for your comment.
> Initially we only suspected it might be copy elision that made gcc and clang 
> to behave differently on this warning, and we were not aware of this flag 
> '-fno-elide-constructors'. 
> Thank you for pointing it out.
> 
>> Using -fno-elide-constructors makes it obvious that the deprecated
>> implicit copy constructors are indeed being elided, so no deprecation
>> warning.
>> 
> 
> I suppose you want to express 'Using -**felide-constructors**' here.
> gcc with '-fno-elide-constructos' would produce derepcated warnings for this 
> issue as clang-10 does.

I really did mean "-fno-elide-constructors". The idea is that having the
normally working build fail with "-fno-elide-constructors" provides evidence
for the "working because of copy elision" conjecture.

clang has supported -f[no-]elide-constructors for a while. 

It appears that either clang is different from gcc for -felide-constructors
(which seems unlikely), or clang (clang-10) is doing the deprecation warning
at a different point from gcc (quite plausible). That is, clang could be
checking for the deprecated case before eliding the call, while gcc is
checking for the deprecated case after copy elision has been applied.

>> Why doesn't this patch similarly define DUIterator copy constructor?
>> It seems to have the same issue, and I'm surprised clang-10 didn't
>> complain about it too. Certainly gcc with -fno-elide-constructors
>> complains about the defaulted implicit constructor.
>> 
> 
> I'm afraid we have noticed the same issue for 'DUIterator' before.
> Yes. It's weird that clang-10 didn't raise warning here. ( verified it on my 
> local machine.)

Yes, that’s weird.

>> But I discovered something alarming while experimenting. Building
>> with gcc10.2 with -fno-elide-constructors doesn't seem to be possible.
>> I get different kinds of failures depending on how DUIterator is
>> defined:
>> 
>> - implict: deprecation warning (as expected)
>> - =delete: error, deleted function used
>> - =default: assert in os::free
>> - _idx and reset from that: assert in reset
>> 
>> Without -fno-elide-constructors, all of the variants seem to work
>> except =delete, which still fails because the deleted function is
>> used. (I didn't test the "working" cases extensively though.)
>> 
>> So there's something problematic, though I don't understand the code
>> well enough to understand what.
> 
> Thanks for your tests. 
> But I have no idea how to fix it right now either.
> Do you know anyone who is familiar with these code and maybe we can invite 
> him/her to help take a look at this issue?
> Thanks.

I have a suspicion that the assert failure when building with
-fno-elide-constructors has nothing to do with DUIterator stuff, and is
instead a problem elsewhere.  But it certainly makes it hard to feel
confident that the additional constructors being added are correct.

I'm going to try to do some investigating of that assert failure, and see if
I can figure out what's going on. Anyone else should feel free to join in.
The failure is

#  Internal Error (../../src/hotspot/share/runtime/thread.hpp:850), pid=29939, 
tid=29939
#  assert(current != __null) failed: Thread::current() called on detached thread

Re: RFR: 8258010: Debug build failure with clang-10 due to -Wdeprecated-copy and -Wimplicit-int-float-conversion

[Kim Barrett]

> On Dec 24, 2020, at 3:44 PM, Xin Liu  wrote:
> 
> On Thu, 24 Dec 2020 17:27:39 GMT, Kim Barrett  wrote:
>> […]
>> Since DUIterator_Last is just delegating both the copy constructor and
>> assignment operator to the base class, their copy constructor and
>> assignment operator would be better as the default (either implicit or
>> explicit using `=default`) rather than explicit code.
> 
> Agree.  c2 actually never uses the assignment operator of DUIterator_Last.  
> It needs the copy constructor in this line.
> https://github.com/openjdk/jdk/blob/master/src/hotspot/share/opto/node.hpp#L1499
> 
> you can delete the following code or leave it =default.
> -  void operator=(const DUIterator_Last& that)
> -{ DUIterator_Fast::operator=(that); }

DUIterator_Last::operator= *is* used, for example:
https://github.com/openjdk/jdk/blob/master/src/hotspot/share/opto/node.hpp#L1473

That doesn't change whether defaulting DUIIterator_Last's copy constructor
and copy assign works though (whether implicit or explicit).  So making it
implicit does work.

I think making it explicitly defaulted might be better though, to make it
clear the default behavior is intentional and it wasn't accidentally left as
the implicit default. This is because the default isn't right for the other
related classes.

Re: RFR: 8258010: Debug build failure with clang-10 due to -Wdeprecated-copy and -Wimplicit-int-float-conversion

[Kim Barrett]

> On Dec 22, 2020, at 8:52 PM, Hao Sun 
>  wrote:
> 
> 1. '-Wdeprecated-copy'
> As specified in C++11 [1], "the generation of the implicitly-defined
> copy constructor is deprecated if T has a user-defined destructor or
> user-defined copy assignment operator". The rationale behind is the
> well-known Rule of Three [2].
> 
> Introduced since gcc-9 [3] and clang-10 [4], flag '-Wdeprecated-copy'
> warns about the C++11 deprecation of implicitly declared copy
> constructor and assignment operator if one of them is user-provided.
> Defining an explicit copy constructor would suppress this warning.
> 
> The main reason why debug build with gcc-9 or higher succeeds lies in
> the inconsistent warning behaviors between gcc and clang. See the
> reduced code example [5]. We suspect it might be return value
> optimization/copy elision [6] that drives gcc not to declare implicit
> copy constructor for this case.

C++17 "guaranteed copy elision" is implemented in gcc7.
http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2016/p0135r1.html

Using -fno-elide-constructors makes it obvious that the deprecated
implicit copy constructors are indeed being elided, so no deprecation
warning.

Why doesn't this patch similarly define DUIterator copy constructor?
It seems to have the same issue, and I'm surprised clang-10 didn't
complain about it too. Certainly gcc with -fno-elide-constructors
complains about the defaulted implicit constructor.

But I discovered something alarming while experimenting.  Building
with gcc10.2 with -fno-elide-constructors doesn't seem to be possible.
I get different kinds of failures depending on how DUIterator is
defined:

- implict: deprecation warning (as expected)
- =delete: error, deleted function used
- =default: assert in os::free
- _idx and reset from that: assert in reset

Without -fno-elide-constructors, all of the variants seem to work
except =delete, which still fails because the deleted function is
used.  (I didn't test the "working" cases extensively though.)

So there's something problematic, though I don't understand the code
well enough to understand what.

Interestingly, some of the complexity and weirdness around copy/assign
for these classes may be an attempt at providing something like move
semantics in a C++98 code base. I've noticed a number of places in
HotSpot that are doing that.

Defining DUIterator_Fast and DUIterator_Last as movable but not
copyable (explicitly delete the copy constructor and copy assign
operator, and define the move constructor and move assign operator
with the reset) works, even with -fno-elide-constructors.

Re: RFR: 8258010: Debug build failure with clang-10 due to -Wdeprecated-copy and -Wimplicit-int-float-conversion

[Kim Barrett]

On Wed, 23 Dec 2020 01:45:58 GMT, Hao Sun 
 wrote:

> 1. '-Wdeprecated-copy'
> As specified in C++11 [1], "the generation of the implicitly-defined
> copy constructor is deprecated if T has a user-defined destructor or
> user-defined copy assignment operator". The rationale behind is the
> well-known Rule of Three [2].
> 
> Introduced since gcc-9 [3] and clang-10 [4], flag '-Wdeprecated-copy'
> warns about the C++11 deprecation of implicitly declared copy
> constructor and assignment operator if one of them is user-provided.
> Defining an explicit copy constructor would suppress this warning.
> 
> The main reason why debug build with gcc-9 or higher succeeds lies in
> the inconsistent warning behaviors between gcc and clang. See the
> reduced code example [5]. We suspect it might be return value
> optimization/copy elision [6] that drives gcc not to declare implicit
> copy constructor for this case.
> 
> Note that flag '-Wdeprecated' in clang-8 and clang-9 would also raise
> warnings for deprecated defintions of copy constructors. However,
> '-Wdeprecated' is not enabled by '-Wall' or '-Wextra'. Hence, clang-8
> and clang-9 are not affected.
> 
> 2. '-Wimplicit-int-float-conversion'
> Making the conversion explicit would fix it.
> 
> Flag '-Wimplicit-int-float-conversion' is first introduced in clang-10.
> Therefore clang-8 and clang-9 are not affected. The flag with similar
> functionality in gcc is '-Wfloat-conversion', but it is not enabled by
> '-Wall' or '-Wextra'. That's why this warning does not apprear when
> building with gcc.
> 
> [1] https://en.cppreference.com/w/cpp/language/copy_constructor
> [2] https://en.cppreference.com/w/cpp/language/rule_of_three
> [3] https://www.gnu.org/software/gcc/gcc-9/changes.html
> [4] https://releases.llvm.org/10.0.0/tools/clang/docs/ReleaseNotes.html
> [5] https://godbolt.org/z/err4jM
> [6] https://en.wikipedia.org/wiki/Copy_elision#Return_value_optimization
> 
> 
> Note that we have tested with this patch, debug build succeeded with clang-10 
> on Linux X86/AArch64 machines.

src/hotspot/share/opto/node.hpp line 1458:

> 1456:   // initialize to garbage
> 1457: 
> 1458:   DUIterator_Last (const DUIterator_Last& that) : DUIterator_Fast(that) 
> {}

Since DUIterator_Last is just delegating both the copy constructor and
assignment operator to the base class, their copy constructor and
assignment operator would be better as the default (either implicit or
explicit using `=default`) rather than explicit code.

-

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

Re: RFR: 8258010: Debug build failure with clang-10 due to -Wdeprecated-copy and -Wimplicit-int-float-conversion

[Kim Barrett]

On Wed, 23 Dec 2020 01:45:58 GMT, Hao Sun 
 wrote:

> 1. '-Wdeprecated-copy'
> As specified in C++11 [1], "the generation of the implicitly-defined
> copy constructor is deprecated if T has a user-defined destructor or
> user-defined copy assignment operator". The rationale behind is the
> well-known Rule of Three [2].
> 
> Introduced since gcc-9 [3] and clang-10 [4], flag '-Wdeprecated-copy'
> warns about the C++11 deprecation of implicitly declared copy
> constructor and assignment operator if one of them is user-provided.
> Defining an explicit copy constructor would suppress this warning.
> 
> The main reason why debug build with gcc-9 or higher succeeds lies in
> the inconsistent warning behaviors between gcc and clang. See the
> reduced code example [5]. We suspect it might be return value
> optimization/copy elision [6] that drives gcc not to declare implicit
> copy constructor for this case.
> 
> Note that flag '-Wdeprecated' in clang-8 and clang-9 would also raise
> warnings for deprecated defintions of copy constructors. However,
> '-Wdeprecated' is not enabled by '-Wall' or '-Wextra'. Hence, clang-8
> and clang-9 are not affected.
> 
> 2. '-Wimplicit-int-float-conversion'
> Making the conversion explicit would fix it.
> 
> Flag '-Wimplicit-int-float-conversion' is first introduced in clang-10.
> Therefore clang-8 and clang-9 are not affected. The flag with similar
> functionality in gcc is '-Wfloat-conversion', but it is not enabled by
> '-Wall' or '-Wextra'. That's why this warning does not apprear when
> building with gcc.
> 
> [1] https://en.cppreference.com/w/cpp/language/copy_constructor
> [2] https://en.cppreference.com/w/cpp/language/rule_of_three
> [3] https://www.gnu.org/software/gcc/gcc-9/changes.html
> [4] https://releases.llvm.org/10.0.0/tools/clang/docs/ReleaseNotes.html
> [5] https://godbolt.org/z/err4jM
> [6] https://en.wikipedia.org/wiki/Copy_elision#Return_value_optimization
> 
> 
> Note that we have tested with this patch, debug build succeeded with clang-10 
> on Linux X86/AArch64 machines.

I think the two issues described here are distinct and should be dealt
with in separate bugs and PRs.  Their only relation is that both arise
with using clang-10.  But they are very different problems, in very
different parts of the code, and probably ought to be reviewed by
folks from different teams.

-

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

Re: RFR: 8212879: Make JVMTI TagMap table concurrent [v11]

[Kim Barrett]

On Wed, 18 Nov 2020 19:31:44 GMT, Serguei Spitsyn  wrote:

> test/hotspot/jtreg/vmTestbase/nsk/jvmti/AttachOnDemand/attach021/attach021Agent00.cpp:
> 
> The change below is not needed as the call to 
> nsk_jvmti_aod_disableEventAndFinish() does exactly the same:
> 
> ```
> -nsk_jvmti_aod_disableEventAndFinish(agentName, JVMTI_EVENT_OBJECT_FREE, 
> success, jvmti, jni);
> +
> +/* Flush any pending ObjectFree events, which may set success to 1 */
> +if (jvmti->SetEventNotificationMode(JVMTI_DISABLE,
> +JVMTI_EVENT_OBJECT_FREE,
> +NULL) != JVMTI_ERROR_NONE) {
> +success = 0;
> +}
> +
> +nsk_aod_agentFinished(jni, agentName, success);
>  }
> ```

This change really is needed.

The success variable in the test is a global, initially 0, set to 1 by the
ObjectFree handler.

In the old code, if the ObjectFree event hasn't been posted yet, we pass the
initial 0 value of success to nsk_jvmti_aod_disabledEventAndFinish, where
it's a local variable (so unaffected by any changes to the variable in the
test), so stays 0 through to the call to nsk_aod_agentFinished.  So the test
fails. 

The split in the change causes the updated post-ObjectFree event success
value of 1 to be passed to agentFinished.  So the test passes.

That required some head scratching to find at the time.  That's the point of
the comment about flushing pending events.  Maybe the comment should be
improved.

-

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

Re: RFR: 8212879: Make JVMTI TagMap table concurrent [v11]

[Kim Barrett]

On Mon, 16 Nov 2020 23:30:25 GMT, Coleen Phillimore  wrote:

>> This change turns the HashTable that JVMTI uses for object tagging into a 
>> regular Hotspot hashtable - the one in hashtable.hpp with resizing and 
>> rehashing.   Instead of pointing directly to oops so that GC has to walk the 
>> table to follow oops and then to rehash the table, this table points to 
>> WeakHandle.  GC walks the backing OopStorages concurrently.
>> 
>> The hash function for the table is a hash of the lower 32 bits of the 
>> address.  A flag is set during GC (gc_notification if in a safepoint, and 
>> through a call to JvmtiTagMap::needs_processing()) so that the table is 
>> rehashed at the next use.
>> 
>> The gc_notification mechanism of weak oop processing is used to notify Jvmti 
>> to post ObjectFree events.  In concurrent GCs there can be a window of time 
>> between weak oop marking where the oop is unmarked, so dead (the phantom 
>> load in peek returns NULL) but the gc_notification hasn't been done yet.  In 
>> this window, a heap walk or GetObjectsWithTags call would not find an object 
>> before the ObjectFree event is posted.  This is dealt with in two ways:
>> 
>> 1. In the Heap walk, there's an unconditional table walk to post events if 
>> events are needed to post.
>> 2. For GetObjectWithTags, if a dead oop is found in the table and posting is 
>> required, we use the VM thread to post the event.
>> 
>> Event posting cannot be done in a JavaThread because the posting needs to be 
>> done while holding the table lock, so that the JvmtiEnv state doesn't change 
>> before posting is done.  ObjectFree callbacks are limited in what they can 
>> do as per the JVMTI Specification.  The allowed callbacks to the VM already 
>> have code to allow NonJava threads.
>> 
>> To avoid rehashing, I also tried to use object->identity_hash() but this 
>> breaks because entries can be added to the table during heapwalk, where the 
>> objects use marking.  The starting markWord is saved and restored.  Adding a 
>> hashcode during this operation makes restoring the former markWord (locked, 
>> inflated, etc) too complicated.  Plus we don't want all these objects to 
>> have hashcodes because locking operations after tagging would have to always 
>> use inflated locks.
>> 
>> Much of this change is to remove serial weak oop processing for the 
>> weakProcessor, ZGC and Shenandoah.  The GCs have been stress tested with 
>> jvmti code.
>> 
>> It has also been tested with tier1-6.
>> 
>> Thank you to Stefan, Erik and Kim for their help with this change.
>
> Coleen Phillimore has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Fix minimal build.

Marked as reviewed by kbarrett (Reviewer).

-

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

Re: RFR: 8188055: (ref) Add Reference::refersTo predicate [v6]

[Kim Barrett]

On Wed, 4 Nov 2020 09:31:13 GMT, Tagir F. Valeev  wrote:

> Hello!
> 
> As an IDE developer, I'm thinking about IDE inspection that may suggest the 
> new method. My idea is to suggest replacing every `ref.get() == obj` with 
> `ref.refersTo(obj)`. Is this a good idea or there are cases when `ref.get() 
> == obj` could be preferred? What do you think?

Those have different behaviors when ref's class overrides get.  Sometimes that 
might be intentional (PhantomReference, where get blocks access to the 
referent, and SoftReference, where get may update heuristics for recent 
accesses delaying GC clearing).  But if some further subclass overrides get for 
some reason, such a change might not be appropriate.

-

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

Re: RFR: 8212879: Make JVMTI TagMap table concurrent [v4]

[Kim Barrett]

On Tue, 3 Nov 2020 21:40:39 GMT, Coleen Phillimore  wrote:

>> src/hotspot/share/prims/jvmtiTagMap.cpp line 127:
>> 
>>> 125:   // The table cleaning, posting and rehashing can race for
>>> 126:   // concurrent GCs. So fix it here once we have a lock or are
>>> 127:   // at a safepoint.
>> 
>> I think this comment and the one below about locking are confused, at least 
>> about rehashing.  I _think_ this is referring to concurrent num-dead 
>> notification?  I've already commented there about it being a problem to do 
>> the unlink  in the GC pause (see later comment).  It also seems like a 
>> bad idea to be doing this here and block progress by a concurrent GC because 
>> we're holding the tagmap lock for a long time, which is another reason to 
>> not have the num-dead notification do very much (and not require a lock that 
>> might be held here for a long time).
>
> The comment is trying to describe the situation like:
> 1. mark-end pause (WeakHandle.peek() returns NULL because object A is 
> unmarked)
> 2. safepoint for heap walk
>2a. Need to post ObjectFree event for object A before the heap walk 
> doesn't find object A.
> 3. gc_notification - would have posted an ObjectFree event for object A if 
> the heapwalk hadn't intervened
> 
> The check_hashmap() function also checks whether the hash table needs to be 
> rehashed before the next operation that uses the hashtable.
> 
> Both operations require the table to be locked.
> 
> The unlink and post needs to be in a GC pause for reasons that I stated 
> above.  The unlink and post were done in a GC pause so this isn't worse for 
> any GCs.  The lock can be held for concurrent GC while the number of entries 
> are processed and this would be a delay for some applications that have 
> requested a lot of tags, but these applications have asked for this and it's 
> not worse than what we had with GC walking this table in safepoints.

For the GCs that call the num_dead notification in a pause it is much worse 
than what we had. As I pointed out elsewhere, it used to be that tagmap 
processing was all-in-one, as a single serial subtask taken by the first thread 
that reached it in WeakProcessor processing. Other threads would find that 
subtask taken and move on to processing oopstores in parallel with the tagmap 
processing. Now everything except the oopstorage-based clearing of dead entries 
is a single threaded serial task done by the VMThread, after all the parallel 
WeakProcessor work is done, because that's where the num-dead callbacks are 
invoked. WeakProcessor's parallel oopstorage processing doesn't have a way to 
do the num-dead callbacks by the last thread out of each parallel oopstorage 
processing. Instead it's left to the end, on the assumption that the callbacks 
are relatively cheap.  But that could still be much worse than the old code, 
since the tagmap oopstorage could be late in the order of processing, an
 d so still effectively be a serial subtask after all the parallel subtasks are 
done or mostly done.

-

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

Re: RFR: 8212879: Make JVMTI TagMap table concurrent [v4]

[Kim Barrett]

On Wed, 4 Nov 2020 07:52:12 GMT, Kim Barrett  wrote:

>> So the design is that when the oops have new addresses, we set a flag in the 
>> table to rehash it.  Not sure why this is wrong and why wouldn't it work for 
>> shenandoah? @zhengyu123 ?  When we call WeakHandle.peek()/resolve() after 
>> the call, the new/moved oop address should be returned.  Why wouldn't this 
>> be the case?
>
> I didn't say it "doesn't work for shenandoah", I said it wouldn't have worked 
> with the old shenandoah barriers without additional work, like adding calls 
> to resolve.  I understand the design intent of notifying the table management 
> that its hash codes are out of date.  And the num-dead callback isn't the 
> right place, since there are num-dead callback invocations that aren't 
> associated with hash code invalidation.  (It's not a correctness wrong, it's 
> a "these things are unrelated and this causes unnecessary work" wrong.)

It used to be that jvmti tagmap processing was all-in-one (in GC weak reference 
processing, with the weak clearing, dead table entry removal, and rehashing all 
done in one pass. This change has split that up, with the weak clearing 
happening in a different place (still as part of the GC's weak reference 
processing) than the others (which I claim can now be part of the mutator, 
whether further separated or not).

"Concurrent GC" has nothing to do with whether tagmaps need rehashing.  Any 
copying collector needs to do so. A non-copying collector (whether concurrent 
or not) would not. (We don't have any of those in HotSpot today.)  And weak 
reference clearing (whether concurrent or not) has nothing to do with whether 
objects have been moved and so the hashing has been invalidated.

There's also a "well known" issue with address-based hashing and generational 
or similar collectors, where a simple boolean "objects have moved" flag can be 
problematic, and which tagmaps seem likely to be prone to.  The old 
do_weak_oops tries to mitigate it by recognizing when the object didn't move.

-

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

Re: RFR: 8212879: Make JVMTI TagMap table concurrent [v5]

[Kim Barrett]

On Wed, 4 Nov 2020 00:08:10 GMT, Coleen Phillimore  wrote:

>> This change turns the HashTable that JVMTI uses for object tagging into a 
>> regular Hotspot hashtable - the one in hashtable.hpp with resizing and 
>> rehashing.   Instead of pointing directly to oops so that GC has to walk the 
>> table to follow oops and then to rehash the table, this table points to 
>> WeakHandle.  GC walks the backing OopStorages concurrently.
>> 
>> The hash function for the table is a hash of the lower 32 bits of the 
>> address.  A flag is set during GC (gc_notification if in a safepoint, and 
>> through a call to JvmtiTagMap::needs_processing()) so that the table is 
>> rehashed at the next use.
>> 
>> The gc_notification mechanism of weak oop processing is used to notify Jvmti 
>> to post ObjectFree events.  In concurrent GCs there can be a window of time 
>> between weak oop marking where the oop is unmarked, so dead (the phantom 
>> load in peek returns NULL) but the gc_notification hasn't been done yet.  In 
>> this window, a heap walk or GetObjectsWithTags call would not find an object 
>> before the ObjectFree event is posted.  This is dealt with in two ways:
>> 
>> 1. In the Heap walk, there's an unconditional table walk to post events if 
>> events are needed to post.
>> 2. For GetObjectWithTags, if a dead oop is found in the table and posting is 
>> required, we use the VM thread to post the event.
>> 
>> Event posting cannot be done in a JavaThread because the posting needs to be 
>> done while holding the table lock, so that the JvmtiEnv state doesn't change 
>> before posting is done.  ObjectFree callbacks are limited in what they can 
>> do as per the JVMTI Specification.  The allowed callbacks to the VM already 
>> have code to allow NonJava threads.
>> 
>> To avoid rehashing, I also tried to use object->identity_hash() but this 
>> breaks because entries can be added to the table during heapwalk, where the 
>> objects use marking.  The starting markWord is saved and restored.  Adding a 
>> hashcode during this operation makes restoring the former markWord (locked, 
>> inflated, etc) too complicated.  Plus we don't want all these objects to 
>> have hashcodes because locking operations after tagging would have to always 
>> use inflated locks.
>> 
>> Much of this change is to remove serial weak oop processing for the 
>> weakProcessor, ZGC and Shenandoah.  The GCs have been stress tested with 
>> jvmti code.
>> 
>> It has also been tested with tier1-6.
>> 
>> Thank you to Stefan, Erik and Kim for their help with this change.
>
> Coleen Phillimore has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Code review comments from Kim and Albert.

src/hotspot/share/prims/jvmtiTagMapTable.hpp line 36:

> 34: class JvmtiTagMapEntryClosure;
> 35: 
> 36: class JvmtiTagMapEntry : public HashtableEntry mtServiceability> {

By using utilities/hashtable this buys into having to use HashtableEntry, which 
includes the _hash member, even though that value is trivially computed from 
the key (since we're using address-based hashing here). This costs an 
additional 8 bytes (_LP64) per entry (a 25% increase) compared to the old 
JvmtiTagHashmapEntry.  (I think it doesn't currently make a difference on 
!_LP64 because of poorly chosen layout in the old code, but fixing that would 
make the difference 33%).

It seems like it should not have been hard to replace the oop _object member in 
the old code with a WeakHandle while otherwise maintaining the Entry interface, 
allowing much of the rest of the code to remain the same or similar and not 
incurring this additional space cost.

-

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

Re: RFR: 8188055: (ref) Add Reference::refersTo predicate [v6]

[Kim Barrett]

On Wed, 4 Nov 2020 09:31:13 GMT, Tagir F. Valeev  wrote:

>> The API looks good, thanks for getting this in.
>
> Hello!
> 
> As an IDE developer, I'm thinking about IDE inspection that may suggest the 
> new method. My idea is to suggest replacing every `ref.get() == obj` with 
> `ref.refersTo(obj)`. Is this a good idea or there are cases when `ref.get() 
> == obj` could be preferred? What do you think?

Thanks to a whole host of folks for reviews and comments.

-

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

Re: RFR: 8212879: Make JVMTI TagMap table concurrent [v3]

[Kim Barrett]

On Wed, 4 Nov 2020 07:41:39 GMT, Kim Barrett  wrote:

>>>Though it might be possible to go even further and eliminate 
>>>WeakProcessorPhases as a thing separate from OopStorageSet.
>> 
>> This makes sense.  Can we file another RFE for this? I was sort of surprised 
>> by how much code was involved so I tried to find a place to stop deleting.
>
>> Ok, so I'm not sure what to do with this:
>> 
>> enum Phase {
>> // Serial phase.
>> JVMTI_ONLY(jvmti)
>> // Additional implicit phase values follow for oopstorages.
>> `};`
>> 
>> I've removed the only thing in this enum.
> 
> Enums without any named enumerators are still meaningful types.  More so with 
> scoped enums, but still with unscoped enums.

> > Though it might be possible to go even further and eliminate 
> > WeakProcessorPhases as a thing separate from OopStorageSet.
> 
> This makes sense. Can we file another RFE for this? I was sort of surprised 
> by how much code was involved so I tried to find a place to stop deleting.

I think the deletion stopped at the wrong place; it either went too far, or not 
far enough.

-

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

Re: RFR: 8212879: Make JVMTI TagMap table concurrent [v4]

[Kim Barrett]

On Tue, 3 Nov 2020 21:31:35 GMT, Coleen Phillimore  wrote:

>> src/hotspot/share/prims/jvmtiTagMap.cpp line 2979:
>> 
>>> 2977: 
>>> 2978: // Concurrent GC needs to call this in relocation pause, so after the 
>>> objects are moved
>>> 2979: // and have their new addresses, the table can be rehashed.
>> 
>> I think the comment is confusing and wrong. The requirement is that the 
>> collector must call this before exposing moved objects to the mutator, and 
>> must provide the to-space invariant. (This whole design would not work with 
>> the old Shenandoah barriers without additional work. I don't know if tagmaps 
>> ever worked at all for them? Maybe they added calls to Access<>::resolve 
>> (since happily deceased) to deal with that?)  I also think there are a bunch 
>> of missing calls; piggybacking on the num-dead callback isn't correct (see 
>> later comment about that).
>
> So the design is that when the oops have new addresses, we set a flag in the 
> table to rehash it.  Not sure why this is wrong and why wouldn't it work for 
> shenandoah? @zhengyu123 ?  When we call WeakHandle.peek()/resolve() after the 
> call, the new/moved oop address should be returned.  Why wouldn't this be the 
> case?

I didn't say it "doesn't work for shenandoah", I said it wouldn't have worked 
with the old shenandoah barriers without additional work, like adding calls to 
resolve.  I understand the design intent of notifying the table management that 
its hash codes are out of date.  And the num-dead callback isn't the right 
place, since there are num-dead callback invocations that aren't associated 
with hash code invalidation.  (It's not a correctness wrong, it's a "these 
things are unrelated and this causes unnecessary work" wrong.)

>> src/hotspot/share/prims/jvmtiTagMap.cpp line 3015:
>> 
>>> 3013:   if (tag_map != NULL && !tag_map->is_empty()) {
>>> 3014: if (num_dead_entries != 0) {
>>> 3015:   tag_map->hashmap()->unlink_and_post(tag_map->env());
>> 
>> Why are we doing this in the callback, rather than just setting a flag?  I 
>> thought part of the point of this change was to get tagmap processing out of 
>> GC pauses.  The same question applies to the non-safepoint side.  The idea 
>> was to be lazy about updating the tagmap, waiting until someone actually 
>> needed to use it.  Or if more prompt ObjectFree notifications are needed 
>> then signal some thread (maybe the service thread?) for followup.
>
> The JVMTI code expects the posting to be done quite eagerly presumably during 
> GC, before it has a chance to disable the event or some other such operation. 
>  So the posting is done during the notification because it's as soon as 
> possible.  Deferring to the ServiceThread had two problems.  1. the event 
> came later than the caller is expecting it, and in at least one test the 
> event was disabled before posting, and 2. there's a comment in the code why 
> we can't post events with a JavaThread.  We'd have to transition into native 
> while holding a no safepoint lock (or else deadlock).  The point of making 
> this change was so that the JVMTI table does not need GC code to serially 
> process the table.

I know of nothing that leads to "presumably during GC" being a requirement. 
Having all pending events of some type occur before that type of event is 
disabled seems like a reasonable requirement, but just means that event 
disabling also requires the table to be "up to date", in the sense that any 
GC-cleared entries need to be dealt with. That can be handled just like other 
operations that use the table contents, rather than during the GC.  That is, 
use post_dead_object_on_vm_thread if there are or might be any pending dead 
objects, before disabling the event.

-

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

Re: RFR: 8212879: Make JVMTI TagMap table concurrent [v3]

[Kim Barrett]

On Tue, 3 Nov 2020 23:38:08 GMT, Coleen Phillimore  wrote:

>> Ok, so I'm not sure what to do with this:
>> 
>>  enum Phase { 
>> // Serial phase.
>> JVMTI_ONLY(jvmti)
>> // Additional implicit phase values follow for oopstorages.
>>   `};`
>> 
>> I've removed the only thing in this enum.
>
>>Though it might be possible to go even further and eliminate 
>>WeakProcessorPhases as a thing separate from OopStorageSet.
> 
> This makes sense.  Can we file another RFE for this? I was sort of surprised 
> by how much code was involved so I tried to find a place to stop deleting.

> Ok, so I'm not sure what to do with this:
> 
> enum Phase {
> // Serial phase.
> JVMTI_ONLY(jvmti)
> // Additional implicit phase values follow for oopstorages.
> `};`
> 
> I've removed the only thing in this enum.

Enums without any named enumerators are still meaningful types.  More so with 
scoped enums, but still with unscoped enums.

-

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

Integrated: 8188055: (ref) Add Reference::refersTo predicate

[Kim Barrett]

On Sun, 4 Oct 2020 03:59:59 GMT, Kim Barrett  wrote:

> Finally returning to this review that was started in April 2020.  I've
> recast it as a github PR.  I think the security concern raised by Gil
> has been adequately answered.
> https://mail.openjdk.java.net/pipermail/hotspot-gc-dev/2020-April/029203.html
> https://mail.openjdk.java.net/pipermail/hotspot-gc-dev/2020-July/030401.html
> https://mail.openjdk.java.net/pipermail/hotspot-gc-dev/2020-August/030677.html
> https://mail.openjdk.java.net/pipermail/hotspot-gc-dev/2020-September/030793.html
> 
> Please review a new function: java.lang.ref.Reference.refersTo.
> 
> This function is needed to test the referent of a Reference object without
> artificially extending the lifetime of the referent object, as may happen
> when calling Reference.get.  Some garbage collectors require extending the
> lifetime of a weak referent when accessed, in order to maintain collector
> invariants.  Lifetime extension may occur with any collector when the
> Reference is a SoftReference, as calling get indicates recent access.  This
> new function also allows testing the referent of a PhantomReference, which
> can't be accessed by calling get.
> 
> The new function uses native methods whose implementations are in the VM so
> they can use the Access API.  It is the intent that these methods will be
> intrinsified by optimizing compilers like C2 or graal, but that hasn't been
> implemented yet.  Bear that in mind before rushing off to change existing
> uses of Reference.get.
> 
> There are two native methods involved, one in Reference and an override in
> PhantomReference, both package private in java.lang.ref. The reason for this
> split is to simplify the intrinsification. This is a change from the version
> from April 2020; that version had a single native method in Reference,
> implemented using the ON_UNKNOWN_OOP_REF Access reference strength category.
> However, adding support for that category in the compilers adds significant
> implementation effort and complexity. Splitting avoids that complexity.
> 
> Testing:
> mach5 tier1
> Locally (linux-x64) verified the new test passes with various garbage 
> collectors.

This pull request has now been integrated.

Changeset: 6023f6b1
Author:Kim Barrett 
URL:   https://git.openjdk.java.net/jdk/commit/6023f6b1
Stats: 501 lines in 13 files changed: 488 ins; 0 del; 13 mod

8188055: (ref) Add Reference::refersTo predicate

Reviewed-by: mchung, pliden, rriggs, dholmes, ihse, smarks, alanb

-

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

Re: RFR: 8188055: (ref) Add Reference::refersTo predicate [v7]

[Kim Barrett]

> Finally returning to this review that was started in April 2020.  I've
> recast it as a github PR.  I think the security concern raised by Gil
> has been adequately answered.
> https://mail.openjdk.java.net/pipermail/hotspot-gc-dev/2020-April/029203.html
> https://mail.openjdk.java.net/pipermail/hotspot-gc-dev/2020-July/030401.html
> https://mail.openjdk.java.net/pipermail/hotspot-gc-dev/2020-August/030677.html
> https://mail.openjdk.java.net/pipermail/hotspot-gc-dev/2020-September/030793.html
> 
> Please review a new function: java.lang.ref.Reference.refersTo.
> 
> This function is needed to test the referent of a Reference object without
> artificially extending the lifetime of the referent object, as may happen
> when calling Reference.get.  Some garbage collectors require extending the
> lifetime of a weak referent when accessed, in order to maintain collector
> invariants.  Lifetime extension may occur with any collector when the
> Reference is a SoftReference, as calling get indicates recent access.  This
> new function also allows testing the referent of a PhantomReference, which
> can't be accessed by calling get.
> 
> The new function uses native methods whose implementations are in the VM so
> they can use the Access API.  It is the intent that these methods will be
> intrinsified by optimizing compilers like C2 or graal, but that hasn't been
> implemented yet.  Bear that in mind before rushing off to change existing
> uses of Reference.get.
> 
> There are two native methods involved, one in Reference and an override in
> PhantomReference, both package private in java.lang.ref. The reason for this
> split is to simplify the intrinsification. This is a change from the version
> from April 2020; that version had a single native method in Reference,
> implemented using the ON_UNKNOWN_OOP_REF Access reference strength category.
> However, adding support for that category in the compilers adds significant
> implementation effort and complexity. Splitting avoids that complexity.
> 
> Testing:
> mach5 tier1
> Locally (linux-x64) verified the new test passes with various garbage 
> collectors.

Kim Barrett 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 13 additional commits since the 
last revision:

 - Merge branch 'master' into refersto
 - improve wording in refersTo javadoc
 - Merge branch 'master' into refersto
 - More explicit refersTo0 comment.
 - simplify test
 - cleanup nits from Mandy
 - use Object instead of TestObject
 - improve refersTo0 descriptions
 - basic functional test
 - change referent access
 - ... and 3 more: https://git.openjdk.java.net/jdk/compare/f06d7348...79277ff3

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/498/files
  - new: https://git.openjdk.java.net/jdk/pull/498/files/3a15b6a9..79277ff3

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

  Stats: 90837 lines in 1555 files changed: 63919 ins; 19502 del; 7416 mod
  Patch: https://git.openjdk.java.net/jdk/pull/498.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/498/head:pull/498

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

Re: RFR: 8212879: Make JVMTI TagMap table concurrent [v4]

[Kim Barrett]

On Tue, 3 Nov 2020 14:53:12 GMT, Coleen Phillimore  wrote:

>> This change turns the HashTable that JVMTI uses for object tagging into a 
>> regular Hotspot hashtable - the one in hashtable.hpp with resizing and 
>> rehashing.   Instead of pointing directly to oops so that GC has to walk the 
>> table to follow oops and then to rehash the table, this table points to 
>> WeakHandle.  GC walks the backing OopStorages concurrently.
>> 
>> The hash function for the table is a hash of the lower 32 bits of the 
>> address.  A flag is set during GC (gc_notification if in a safepoint, and 
>> through a call to JvmtiTagMap::needs_processing()) so that the table is 
>> rehashed at the next use.
>> 
>> The gc_notification mechanism of weak oop processing is used to notify Jvmti 
>> to post ObjectFree events.  In concurrent GCs there can be a window of time 
>> between weak oop marking where the oop is unmarked, so dead (the phantom 
>> load in peek returns NULL) but the gc_notification hasn't been done yet.  In 
>> this window, a heap walk or GetObjectsWithTags call would not find an object 
>> before the ObjectFree event is posted.  This is dealt with in two ways:
>> 
>> 1. In the Heap walk, there's an unconditional table walk to post events if 
>> events are needed to post.
>> 2. For GetObjectWithTags, if a dead oop is found in the table and posting is 
>> required, we use the VM thread to post the event.
>> 
>> Event posting cannot be done in a JavaThread because the posting needs to be 
>> done while holding the table lock, so that the JvmtiEnv state doesn't change 
>> before posting is done.  ObjectFree callbacks are limited in what they can 
>> do as per the JVMTI Specification.  The allowed callbacks to the VM already 
>> have code to allow NonJava threads.
>> 
>> To avoid rehashing, I also tried to use object->identity_hash() but this 
>> breaks because entries can be added to the table during heapwalk, where the 
>> objects use marking.  The starting markWord is saved and restored.  Adding a 
>> hashcode during this operation makes restoring the former markWord (locked, 
>> inflated, etc) too complicated.  Plus we don't want all these objects to 
>> have hashcodes because locking operations after tagging would have to always 
>> use inflated locks.
>> 
>> Much of this change is to remove serial weak oop processing for the 
>> weakProcessor, ZGC and Shenandoah.  The GCs have been stress tested with 
>> jvmti code.
>> 
>> It has also been tested with tier1-6.
>> 
>> Thank you to Stefan, Erik and Kim for their help with this change.
>
> Coleen Phillimore has updated the pull request with a new target base due to 
> a merge or a rebase. The pull request now contains five commits:
> 
>  - Merge branch 'master' into jvmti-table
>  - Merge branch 'master' into jvmti-table
>  - More review comments from Stefan and ErikO
>  - Code review comments from StefanK.
>  - 8212879: Make JVMTI TagMap table not hash on oop address

src/hotspot/share/prims/jvmtiTagMap.cpp line 3018:

> 3016: }
> 3017: // Later GC code will relocate the oops, so defer rehashing 
> until then.
> 3018: tag_map->_needs_rehashing = true;

This is wrong for some collectors. I think all collectors ought to be calling 
set_needs_rehashing in appropriate places, and it can't be be correctly 
piggybacked on the num-dead callback. (See discussion above for that function.)

For example, G1 remark pause does weak processing (including weak oopstorage) 
and will call the num-dead callback, but does not move objects, so does not 
require tagmap rehashing.

(I think CMS oldgen remark may have been similar, for what that's worth.)

src/hotspot/share/prims/jvmtiTagMap.cpp line 3015:

> 3013:   if (tag_map != NULL && !tag_map->is_empty()) {
> 3014: if (num_dead_entries != 0) {
> 3015:   tag_map->hashmap()->unlink_and_post(tag_map->env());

Why are we doing this in the callback, rather than just setting a flag?  I 
thought part of the point of this change was to get tagmap processing out of GC 
pauses.  The same question applies to the non-safepoint side.  The idea was to 
be lazy about updating the tagmap, waiting until someone actually needed to use 
it.  Or if more prompt ObjectFree notifications are needed then signal some 
thread (maybe the service thread?) for followup.

src/hotspot/share/prims/jvmtiTagMap.cpp line 2979:

> 2977: 
> 2978: // Concurrent GC needs to call this in relocation pause, so after the 
> objects are moved
> 2979: // and have their new addresses, the table can be rehashed.

I think the comment is confusing and wrong. The requirement is that the 
collector must call this before exposing moved objects to the mutator, and must 
provide the to-space invariant. (This whole design would not work with the old 
Shenandoah barriers without additional work. I don't know if tagmaps ever 
worked at all for them? Maybe they added calls to Access<>::resolve (since 
happily deceased) to deal with that?)  I 

Re: RFR: 8212879: Make JVMTI TagMap table concurrent [v3]

[Kim Barrett]

On Tue, 3 Nov 2020 12:58:22 GMT, Coleen Phillimore  wrote:

>> This change turns the HashTable that JVMTI uses for object tagging into a 
>> regular Hotspot hashtable - the one in hashtable.hpp with resizing and 
>> rehashing.   Instead of pointing directly to oops so that GC has to walk the 
>> table to follow oops and then to rehash the table, this table points to 
>> WeakHandle.  GC walks the backing OopStorages concurrently.
>> 
>> The hash function for the table is a hash of the lower 32 bits of the 
>> address.  A flag is set during GC (gc_notification if in a safepoint, and 
>> through a call to JvmtiTagMap::needs_processing()) so that the table is 
>> rehashed at the next use.
>> 
>> The gc_notification mechanism of weak oop processing is used to notify Jvmti 
>> to post ObjectFree events.  In concurrent GCs there can be a window of time 
>> between weak oop marking where the oop is unmarked, so dead (the phantom 
>> load in peek returns NULL) but the gc_notification hasn't been done yet.  In 
>> this window, a heap walk or GetObjectsWithTags call would not find an object 
>> before the ObjectFree event is posted.  This is dealt with in two ways:
>> 
>> 1. In the Heap walk, there's an unconditional table walk to post events if 
>> events are needed to post.
>> 2. For GetObjectWithTags, if a dead oop is found in the table and posting is 
>> required, we use the VM thread to post the event.
>> 
>> Event posting cannot be done in a JavaThread because the posting needs to be 
>> done while holding the table lock, so that the JvmtiEnv state doesn't change 
>> before posting is done.  ObjectFree callbacks are limited in what they can 
>> do as per the JVMTI Specification.  The allowed callbacks to the VM already 
>> have code to allow NonJava threads.
>> 
>> To avoid rehashing, I also tried to use object->identity_hash() but this 
>> breaks because entries can be added to the table during heapwalk, where the 
>> objects use marking.  The starting markWord is saved and restored.  Adding a 
>> hashcode during this operation makes restoring the former markWord (locked, 
>> inflated, etc) too complicated.  Plus we don't want all these objects to 
>> have hashcodes because locking operations after tagging would have to always 
>> use inflated locks.
>> 
>> Much of this change is to remove serial weak oop processing for the 
>> weakProcessor, ZGC and Shenandoah.  The GCs have been stress tested with 
>> jvmti code.
>> 
>> It has also been tested with tier1-6.
>> 
>> Thank you to Stefan, Erik and Kim for their help with this change.
>
> Coleen Phillimore has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   More review comments from Stefan and ErikO

src/hotspot/share/gc/shared/weakProcessorPhases.hpp line 41:

> 39:   class Iterator;
> 40: 
> 41:   typedef void (*Processor)(BoolObjectClosure*, OopClosure*);

I think this typedef is to support serial phases and that it is probably no 
longer used.

src/hotspot/share/gc/shared/weakProcessorPhases.hpp line 50:

> 48: };
> 49: 
> 50: typedef uint WeakProcessorPhase;

This was originally written with the idea that WeakProcessorPhases::Phase (and 
WeakProcessorPhase) should be a scoped enum (but we didn't have that feature 
yet). It's possible there are places that don't cope with a scoped enum, since 
that feature wasn't available when the code was written, so there might have be 
mistakes.

But because of that, I'd prefer to keep the WeakProcessorPhases::Phase type and 
the existing definition of WeakProcessorPhase. Except this proposed change is 
breaking that at least here:

src/hotspot/share/gc/shared/weakProcessor.inline.hpp
116 uint oopstorage_index = WeakProcessorPhases::oopstorage_index(phase);
117 StorageState* cur_state = _storage_states.par_state(oopstorage_index);
=>
103 StorageState* cur_state = _storage_states.par_state(phase);

I think eventually (as in some future RFE) this could all be collapsed to 
something provided by OopStorageSet.
enum class : uint WeakProcessorPhase {}; 

ENUMERATOR_RANGE(WeakProcessorPhase,
 static_cast(0), 
 static_cast(OopStorageSet::weak_count));
and replacing all uses of WeakProcessorPhases::Iterator with 
EnumIterator (which involves more than a type alias).

Though it might be possible to go even further and eliminate 
WeakProcessorPhases as a thing separate from OopStorageSet.

-

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

Re: RFR: 8188055: (ref) Add Reference::refersTo predicate [v6]

[Kim Barrett]

On Fri, 16 Oct 2020 19:22:16 GMT, Mandy Chung  wrote:

>> I just want to note that if you have a `Reference ref` at hand, 
>> you can not just do:
>> Referemce r = (Reference) ref;
>> ...since those generic types are not related. You have to do something like:
>> 
>> @SuppressWarnings({"unchecked", "rawtypes"})
>> Referemce r = (Reference) ref;
>> which is very unfortunate. Comparing this method with for example 
>> `Collection.contains(Object element)`, you can see that Collection API has 
>> made a decision to not bother with T here. That was also due to keeping old 
>> code compatible when migrating from pre-generics Java to generified 
>> Collection, but as @dfuch noted, we have a migration story here too. We will 
>> be migrating `obj == ref.get()` to `ref.refersTo(obj)` ... Mind you that 
>> this is a boolean expression fragment which might be written inline 
>> surrounded with other parts of expression. So you'll be forced to split that 
>> into assignment with @SuppressWarnings and an expression or you will have to 
>> force the whole expression or method to @SuppressWarnings. I don't know if 
>> type "safety" is forth it here.
>
> Reference instances should not be leaked and so I don't see very common that 
> caller of `Reference::get` does not know the referent's type.   It also 
> depends on the `refersTo` check against `null` vs an object.  Any known use 
> case would be helpful if any (some existing code that wants to call 
> `refersTo` to compare a `Reference` of raw type with an object of unknown 
> type).
> 
> FWIW, when converting a few use of `Reference::get` to `refersTo` in JDK, 
> there is only one case (`equals(Object o)` method that needs the cast.
> 
> http://cr.openjdk.java.net/~mchung/jdk15/webrevs/8188055/jdk-use-refersTo/index.html

Is there a consensus on this issue?  It's not clear whether Daniel and Peter 
have agreed with Mandy's responses or have just not yet responded with further 
discussion.

-

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

Re: RFR: 8188055: (ref) Add Reference::refersTo predicate [v6]

[Kim Barrett]

> Finally returning to this review that was started in April 2020.  I've
> recast it as a github PR.  I think the security concern raised by Gil
> has been adequately answered.
> https://mail.openjdk.java.net/pipermail/hotspot-gc-dev/2020-April/029203.html
> https://mail.openjdk.java.net/pipermail/hotspot-gc-dev/2020-July/030401.html
> https://mail.openjdk.java.net/pipermail/hotspot-gc-dev/2020-August/030677.html
> https://mail.openjdk.java.net/pipermail/hotspot-gc-dev/2020-September/030793.html
> 
> Please review a new function: java.lang.ref.Reference.refersTo.
> 
> This function is needed to test the referent of a Reference object without
> artificially extending the lifetime of the referent object, as may happen
> when calling Reference.get.  Some garbage collectors require extending the
> lifetime of a weak referent when accessed, in order to maintain collector
> invariants.  Lifetime extension may occur with any collector when the
> Reference is a SoftReference, as calling get indicates recent access.  This
> new function also allows testing the referent of a PhantomReference, which
> can't be accessed by calling get.
> 
> The new function uses native methods whose implementations are in the VM so
> they can use the Access API.  It is the intent that these methods will be
> intrinsified by optimizing compilers like C2 or graal, but that hasn't been
> implemented yet.  Bear that in mind before rushing off to change existing
> uses of Reference.get.
> 
> There are two native methods involved, one in Reference and an override in
> PhantomReference, both package private in java.lang.ref. The reason for this
> split is to simplify the intrinsification. This is a change from the version
> from April 2020; that version had a single native method in Reference,
> implemented using the ON_UNKNOWN_OOP_REF Access reference strength category.
> However, adding support for that category in the compilers adds significant
> implementation effort and complexity. Splitting avoids that complexity.
> 
> Testing:
> mach5 tier1
> Locally (linux-x64) verified the new test passes with various garbage 
> collectors.

Kim Barrett has updated the pull request incrementally with one additional 
commit since the last revision:

  improve wording in refersTo javadoc

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/498/files
  - new: https://git.openjdk.java.net/jdk/pull/498/files/ab4e519b..3a15b6a9

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

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

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

Re: RFR: 8188055: (ref) Add Reference::refersTo predicate [v5]

[Kim Barrett]

On Tue, 20 Oct 2020 16:35:02 GMT, Mandy Chung  wrote:

>>> @kimbarrett your reworded text is okay. I think "if it initially had some 
>>> other referent value" can be dropped.
>>> 
>>> For a `Reference` constructed with a `null` referent, we can clarify in the 
>>> spec that such reference object will never
>>> get cleared and enqueued. I suggest to file a separate issue to follow up.
>> 
>> I don't think that clause can be dropped, because of explicit clearing (by 
>> clear() or enqueue()) rather than by the
>> GC.  If the reference was constructed with a null referent, 
>> ref.refersTo(null) cannot tell whether ref.clear() has been
>> called.
>
>> Mandy's comment implied that references with a null referent never get 
>> enqueued. Otherwise when would they get
>> enqueued? There would be nothing to trigger it.
> 
> Sorry I should have been clearer.What I try to say is that 
> `Reference(null)`
> will never be cleared and enqueued by GC since its referent is `null`.
> 
> Kim is right that `Reference(null)` can be explicitly cleared and enqueued
> via `Reference::enqueue`.`Reference::clear` on such an "empty" reference
> object is essentially a no-op.Whoever creates an "empty" reference would
> not intend to be cleared.
> 
>> > But the more we discuss this the more I think allowing an initial null 
>> > referent was a mistake in the first place. :(
>> 
>> I agree, but here we are. Very hard to know what the compatibility impact of 
>> changing that would be.
> 
> There are existing use cases depending on `Reference(null)` for example as a 
> special
> instance be an empty reference or the head of a doubly-linked list of 
> references.
> This was discussed two years ago [1].
> 
> [1] 
> https://mail.openjdk.java.net/pipermail/core-libs-dev/2018-July/054325.html

David, Mandy, and I discussed the wording in refersTo javadoc and reached a 
consensus that is reflected in 3a15b6a.

-

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

Re: RFR: 8252407: Build failure with gcc-8+ and asan.

[Kim Barrett]

On Mon, 12 Oct 2020 11:36:17 GMT, eric.1iu 
 wrote:

> Summary: Fix stringop-truncation warnings casused by gcc.
> 
> The newer gcc(gcc-8 or higher) would warn for calls to bounded string
> manipulation functions such as 'strncpy' that may either truncate the
> copied string or leave the destination unchanged.
> 
> The reason is that -fsanitize=address might simply incompatible with
> -Wstringop-truncation. This patch disables stringop-truncation warning
> when asan enabled, also disables the defining of _FORTIFY_SOURCE in
> fastdebug builds for some potential risks.

Looks good to me.  This is what I was expecting to see, based on the previous 
discussion.

-

Marked as reviewed by kbarrett (Reviewer).

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

Re: RFR: 8253375: OSX build fails with Xcode 12.0 (12A7209) [v2]

[Kim Barrett]

> On Sep 29, 2020, at 4:05 PM, Lutz Schmidt  wrote:
> 
> On Tue, 29 Sep 2020 19:33:48 GMT, Paul Hohensee  wrote:
> 
>>> Please review this small patch to enable the OSX build using Xcode 12.0.
>>> 
>>> Thanks,
>>> Paul
>> 
>> Paul Hohensee 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:
>> - 8253375: Reverted CSystemColors.m patch, replaced sharedRuntime.cpp patch
>> - Merge branch 'master' into JDK-8253375
>> - JDK-8253375
> 
> The proposed (updated) change does _NOT_ compile on my machine (MacOS 
> 10.15.6, Xcode 12.0):
> 
> sharedRuntime.cpp:2860:46: error: cannot cast from type 'struct (anonymous 
> struct at sharedRuntime.cpp:2859:7)' to
> pointer type 'relocInfo *’

Did you use the change from the PR, or apply it manually.  That looks like the 
error one would get if
only the type of locs_buf were changed, without changing to take the address in 
the reference.  That
is, without changing `(relocInfo*)locs_buf` to `(relocInfo*)_buf`.  That 
second change is in the PR.

> You will have to use a union (option (2) as proposed by Kim Barrett far 
> above. I proved that variant compiles on my
> machine.
> 
> -
> 
> Changes requested by lucy (Reviewer).
> 
> PR: https://git.openjdk.java.net/jdk/pull/348

Re: RFR: 8253375: OSX build fails with Xcode 12.0 (12A7209) [v2]

[Kim Barrett]

On Tue, 29 Sep 2020 19:33:48 GMT, Paul Hohensee  wrote:

>> Please review this small patch to enable the OSX build using Xcode 12.0.
>> 
>> Thanks,
>> Paul
>
> Paul Hohensee 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:
>  - 8253375: Reverted CSystemColors.m patch, replaced sharedRuntime.cpp patch
>  - Merge branch 'master' into JDK-8253375
>  - JDK-8253375

Marked as reviewed by kbarrett (Reviewer).

-

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

Re: [OpenJDK 2D-Dev] RFR: 8253375: OSX build fails with Xcode 12.0 (12A7209)

[Kim Barrett]

> On Sep 29, 2020, at 10:18 AM, Hohensee, Paul  wrote:
> Code that calls initialize_shared_locs is inconsistent even within itself. 
> E.g., in c1_Compilation.cpp, we have

Agreed there seems to be a bit of a mess around that function.

> Anyway, I just wanted to make the compiler warning go away, not figure out 
> why the code is the way it is. Matthias and Kim would like to separate out 
> the CSystemColors.m patch into a separate bug, which is fine by me (see 
> separate email).
> 
> So, for the sharedRuntime patch, would it be ok to just use
> 
> short locs_buf[84];
> 
> to account for possible alignment in initialize_shared_locs? I'm using 84 
> because 80 is exactly 5 records plus 5 sizeof(relocInfo)s, allowing for 
> alignment adds 3, and rounding the total array size up to a multiple of 8 
> adds 1.

Your analysis looks plausible.  But consider instead

struct { double data[20]; } locs_buf;
and change next line to use _buf

This doesn't change the size or alignment from pre-existing code. I can't
test whether this will suppress the warning, but I'm guessing it will.  And the 
rest
of below is off if that’s wrong.

Changing the size and type and worrying about alignment changes seems beyond
what's needed "to make the compiler warning go away, not figure out why the
code is the way it is.”  I dislike making weird changes to suppress compiler 
warnings,
but changing the type and size seems more weird to me here.  I mean, 84 looks 
like
a number pulled out of a hat, unless you are going to add a bunch of commentary
that probably really belongs in a bug report to look at this stuff more 
carefully.

And file an RFE to look at initialize_shared_locs and its callers more 
carefully. 

Re: RFR: 8253375: OSX build fails with Xcode 12.0 (12A7209)

[Kim Barrett]

> On Sep 29, 2020, at 3:59 AM, Matthias Baesken  
> wrote:
> 
> On Fri, 25 Sep 2020 02:23:07 GMT, David Holmes  wrote:
> 
>>> Please review this small patch to enable the OSX build using Xcode 12.0.
>>> 
>>> Thanks,
>>> Paul
>> 
>> src/hotspot/share/runtime/sharedRuntime.cpp line 2851:
>> 
>>> 2849: if (buf != NULL) {
>>> 2850:   CodeBuffer buffer(buf);
>>> 2851:   short locs_buf[80];
>> 
>> This code is just weird. Why is the locs_buf array not declared to be the 
>> desired type? If the compiler rejects double
>> because it isn't relocInfo* then why does it accept short? And if it accepts 
>> short will it accept int or long long or
>> int64_t? Using int64_t would be a clearer change. Though semantically this 
>> code is awful. :( Should it be intptr_t ?
> 
> Currently we have in the existing code  various kind of buffers passed into 
> initialize_shared_locs that compile nicely
> on all supported compilers and on Xcode 12 as well ,see
> 
> c1_Compilation.cpp
> 
> 326  char* locs_buffer = NEW_RESOURCE_ARRAY(char, locs_buffer_size);
> 327  code->insts()->initialize_shared_locs((relocInfo*)locs_buffer,
> 
> sharedRuntime.cpp
> 
> 2681  CodeBuffer buffer(buf);
> 2682  short buffer_locs[20];
> 2683  buffer.insts()->initialize_shared_locs((relocInfo*)buffer_locs,
> 2684 
> sizeof(buffer_locs)/sizeof(relocInfo));
> 
> So probably using short would be consistent to what we do already in the 
> coding at another place (looking into
> relocInfo it would be probably better  to use unsigned short  or to   typedef 
>  unsigned short in the relocInfo class
> and use the typedef).

That’s *not* consistent, because of buffer alignment.  The call to 
NEW_RESOURCE_ARRAY is going
to return a pointer to memory that is 2*word aligned.  (Interesting, possibly a 
32/64 bit “bug” here.)
The existing code in sharedRuntime.cpp is allocating double-aligned.  
iniitalize_shared_locs wants a
HeapWordSize-aligned buffer, and adjusts the pointer it uses accordingly.  (I 
think with existing code
it could just make the alignment of the buffer a precondition, but I haven’t 
looked at all callers.)
Changing the declaration in sharedRuntime.cpp to short[] reduces the alignment 
requirement for the
array, possibly requiring alignment adjustment (and hence size reduction) by 
initialize_shared_locs.

Since initialize_shared_locs specifically adjusts the alignment, some 
downstream use of the buffer
probably actually cares.


> 
> -
> 
> PR: https://git.openjdk.java.net/jdk/pull/348

Re: RFR: 8253375: OSX build fails with Xcode 12.0 (12A7209)

[Kim Barrett]

> On Sep 29, 2020, at 3:14 AM, Matthias Baesken  
> wrote:
> 
> On Fri, 25 Sep 2020 06:06:31 GMT, Kim Barrett  wrote:
> 
>>> Please review this small patch to enable the OSX build using Xcode 12.0.
>>> 
>>> Thanks,
>>> Paul
>> 
>> src/java.desktop/macosx/native/libawt_lwawt/awt/CSystemColors.m line 129:
>> 
>>> 127: NSColor* result = nil;
>>> 128:
>>> 129: if (colorIndex < ((useAppleColor) ? 
>>> sun_lwawt_macosx_LWCToolkit_NUM_APPLE_COLORS :
>>> java_awt_SystemColor_NUM_COLORS)) {
>> 
>> This looks like a plain old bug fix, nothing really to do with the compiler 
>> upgrade.  The new compiler presumably just
>> has a new warning that brought attention to the problem.  As such, I don't 
>> think it belongs in a PR that's about issues
>> related to the compiler upgrade.  Someone might want to backport just this 
>> fix, for example.
> 
> Hello  Kim, Paul -  so should we open a separate bug for the
> src/java.desktop/macosx/native/libawt_lwawt/awt/CSystemColors.m issue because 
> it might be a general  problem just
> uncovered by the new compiler ? Paul , do you want to do this ? Or should I 
> open a bug and post a separate change for
> the useAppleColor check in CSystemColors.m ?

I think so.

> 
> -
> 
> PR: https://git.openjdk.java.net/jdk/pull/348

Re: RFR: 8253375: OSX build fails with Xcode 12.0 (12A7209)

[Kim Barrett]

> On Sep 25, 2020, at 6:22 AM, Lutz Schmidt  wrote:
> 
> On Fri, 25 Sep 2020 05:46:08 GMT, Kim Barrett  wrote:
> 
> Another note, actually, it's more like a question:
> Has anyone had an eye on what happens in initialize_shared_locs(relocInfo* 
> buf, int length)? To my understanding,
> "buf", which is passed in as "locs_buf", is stored into CodeBuffer fields. Is 
> that a good idea? "locs_buf" points into
> the stack. This pointer becomes invalid at the end of the "locs_buf" scope. 
> Did I get something wrong here?

It’s “odd” but I think it’s more or less okay.  Here and in other uses we seem 
to be allocating
dynamically scoped storage for temporary use by the CodeBuffer.  I think a more 
normal
formulation might be to allocate the buffer, then pass it to the CodeBuffer 
constructor as a
non-transfer of ownership.  But I haven’t looked at all the places where this 
is used, and that’s
perhaps out of scope for the problem at hand.

> -
> 
> PR: https://git.openjdk.java.net/jdk/pull/348

Re: RFR: 8253375: OSX build fails with Xcode 12.0 (12A7209)

[Kim Barrett]

> On Sep 25, 2020, at 1:49 AM, Kim Barrett  wrote:
> 
> On Thu, 24 Sep 2020 21:28:01 GMT, Paul Hohensee  wrote:
> 
>> Please review this small patch to enable the OSX build using Xcode 12.0.
>> 
>> Thanks,
>> Paul
> 
> […]
> 
> I think changing the declaration for locs_buf to any of the following gives
> equivalent behavior to the current code. […]
> 
> […]

Another variant that probably avoids both the warning and avoids any C++14 
features:

(4)  union { char data[20 * sizeof(double)]; double align; } locs_buf;
and change (relocInfo*)locs_buf => (relocInfo*)_buf.

> -
> 
> Changes requested by kbarrett (Reviewer).
> 
> PR: https://git.openjdk.java.net/jdk/pull/348

Re: RFR: 8253375: OSX build fails with Xcode 12.0 (12A7209)

[Kim Barrett]

> On Sep 25, 2020, at 1:49 AM, Kim Barrett  wrote:
> 
> On Thu, 24 Sep 2020 21:28:01 GMT, Paul Hohensee  wrote:
> 
>> Please review this small patch to enable the OSX build using Xcode 12.0.
>> 
>> Thanks,
>> Paul

[I tried submitting this comment through the github UI and somehow managed to 
lose
all the context in the process.  In case it’s not (eventually) obvious, this is 
about the
change to src/hotspot/share/runtime/sharedRuntime.cpp.  Still figuring out 
github…]

> No, don't do this.  In the original, double is used to obtain the desired
> alignmnent.  Changing the element type to short reduces the alignment
> requirement for the variable.  initialize_shared_locs will then adjust the
> alignment, potentially shrinking the effective array size.  So this is a
> real change that I think shouldn't be made.
> 
> I think changing the declaration for locs_buf to any of the following gives
> equivalent behavior to the current code. I don't know whether any of them
> will trigger the new clang warning though. I don't have access to that
> version right now, and the documentation for the warning is useless (like so
> much of clang's warning options documentation).
> 
> (1) alignas(double) char locs_buf[20 * sizeof(double)];
> (but alignas is not yet in the "permitted features" list in the Style Guide:
> https://bugs.openjdk.java.net/browse/JDK-8252584)
> 
> (2) union { char locs_buf[20 * sizeof(double)]; double align; };
> 
> (3) std::aligned_storage_t<20 * sizeof(double)> locs_buf;
> and change (relocInfo*)locs_buf => (relocInfo*)_buf
> and #include 
> This has the benefit of being explicit that we're just stack allocating a
> block of storage.
> 
> I'll make a wild guess that (1) and (2) will still warn, though char arrays
> are somewhat special in the language so maybe they won't.  I'm pretty sure
> (3) should do the trick.
> 
> -
> 
> Changes requested by kbarrett (Reviewer).
> 
> PR: https://git.openjdk.java.net/jdk/pull/348

Re: RFR: 8253375: OSX build fails with Xcode 12.0 (12A7209)

[Kim Barrett]

On Thu, 24 Sep 2020 21:28:01 GMT, Paul Hohensee  wrote:

> Please review this small patch to enable the OSX build using Xcode 12.0.
> 
> Thanks,
> Paul

src/java.desktop/macosx/native/libawt_lwawt/awt/CSystemColors.m line 129:

> 127: NSColor* result = nil;
> 128:
> 129: if (colorIndex < ((useAppleColor) ? 
> sun_lwawt_macosx_LWCToolkit_NUM_APPLE_COLORS :
> java_awt_SystemColor_NUM_COLORS)) {

This looks like a plain old bug fix, nothing really to do with the compiler 
upgrade.  The new compiler presumably just
has a new warning that brought attention to the problem.  As such, I don't 
think it belongs in a PR that's about issues
related to the compiler upgrade.  Someone might want to backport just this fix, 
for example.

-

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

Re: RFR: 8253375: OSX build fails with Xcode 12.0 (12A7209)

[Kim Barrett]

On Thu, 24 Sep 2020 21:28:01 GMT, Paul Hohensee  wrote:

> Please review this small patch to enable the OSX build using Xcode 12.0.
> 
> Thanks,
> Paul

No, don't do this.  In the original, double is used to obtain the desired
alignmnent.  Changing the element type to short reduces the alignment
requirement for the variable.  initialize_shared_locs will then adjust the
alignment, potentially shrinking the effective array size.  So this is a
real change that I think shouldn't be made.

I think changing the declaration for locs_buf to any of the following gives
equivalent behavior to the current code. I don't know whether any of them
will trigger the new clang warning though. I don't have access to that
version right now, and the documentation for the warning is useless (like so
much of clang's warning options documentation).

(1) alignas(double) char locs_buf[20 * sizeof(double)];
(but alignas is not yet in the "permitted features" list in the Style Guide:
https://bugs.openjdk.java.net/browse/JDK-8252584)

(2) union { char locs_buf[20 * sizeof(double)]; double align; };

(3) std::aligned_storage_t<20 * sizeof(double)> locs_buf;
and change (relocInfo*)locs_buf => (relocInfo*)_buf
and #include 
This has the benefit of being explicit that we're just stack allocating a
block of storage.

I'll make a wild guess that (1) and (2) will still warn, though char arrays
are somewhat special in the language so maybe they won't.  I'm pretty sure
(3) should do the trick.

-

Changes requested by kbarrett (Reviewer).

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

Re: RFR: 8250876: Build system preparation to macos on aarch64

[Kim Barrett]

> On Aug 13, 2020, at 6:54 AM, Magnus Ihse Bursie 
>  wrote:
> 
> On 2020-08-13 09:15, Kim Barrett wrote:
>> I'm similarly surprised by this pre-existing bit in CoreLibraries.gmk:
>> 
>> $(eval $(call SetupJdkLibrary, BUILD_LIBJIMAGE, \
>> ...
>> LIBS_unix := -ljvm -ldl $(LIBCXX), \
>> LIBS_macosx := -lc++, \
>> 
>> And that suggests a better place for all this (assuming its needed at
>> all) is in lib-std.m4, setting LIBCXX for macosx and using it where
>> needed.
> Yes, that looks weird. Probably some left-over from the initial integration 
> of libjimage. I can build fine locally without it; I'm just verifying that it 
> also works in the CI. Thank you for noticing this!

I just filed https://bugs.openjdk.java.net/browse/JDK-8251533 for that.

Re: RFR: 8250876: Build system preparation to macos on aarch64

[Kim Barrett]

> On Aug 13, 2020, at 3:15 AM, Kim Barrett  wrote:
> 
>> On Aug 1, 2020, at 3:24 AM, Vladimir Kempik  wrote:
>> 
>> Hello
>> 
>> Please review this change for JDK-8250876
>> 
>> This changeset adds support for macos/aarch64 into build system.
>> It will allow to crosscompile for macos/aarch64 using intel mac as well.
>> 
>> This changeset does NOT address some arm specific issues in the macos 
>> related code, we plan to do that in s separate commit.
>> 
>> An example of configure to cross-compile for macos/arm64:
>> 
>> --with-boot-jdk=/path/to/java/ 
>> --with-build-jdk=/path/to/same/java/as/compiled  
>> --disable-warnings-as-errors --with-jvm-variants=zero 
>> --openjdk-target=aarch64-apple-darwin --with-extra-cflags='-arch arm64' 
>> --with-extra-ldflags='-arch arm64 
>> -F/Path/To/Folder/Containing/JNF_framework/' —with-extra-cxxflags='-arch 
>> arm64’
>> 
>> JNF.framework is missing arm64 part as of next macos release, but Apple has 
>> opensourced it. 
>> 
>> Fix to adlc were needed due to it using symbols from stdc++ and not linking 
>> to it, so it fails when doing make images.
>> 
>> The webrev: http://cr.openjdk.java.net/~vkempik/8250876/webrev.00/
>> The bug: https://bugs.openjdk.java.net/browse/JDK-8250876
>> 
>> Testing: jdk/submit.
>> 
>> Thanks, Vladimir.
> 
> Coming late to the party, as I see this has already been pushed.  But
> one thing caught my eye.

I should have read further ahead in the thread.  Looks like
this got dealt with in a different way that seems much better.

> 
> --
>  42   else ifeq ($(call isBuildOs, macosx), true)
>  43 ADLC_LDFLAGS := -lc++
> 
> I'm surprised this is needed. I expected the C++ toolchain to
> implicitly include that, the way g++ does.
> 
> If something like this really is needed, then it seems like it should
> be ADLC_LIBS that should be modified, rather than ADLC_LDFLAGS.
> Though I noticed there are currently no assignments to ADLC_LIBS.
> 
> I'm similarly surprised by this pre-existing bit in CoreLibraries.gmk:
> 
> $(eval $(call SetupJdkLibrary, BUILD_LIBJIMAGE, \
> ...
>LIBS_unix := -ljvm -ldl $(LIBCXX), \
>LIBS_macosx := -lc++, \
> 
> And that suggests a better place for all this (assuming its needed at
> all) is in lib-std.m4, setting LIBCXX for macosx and using it where
> needed.
> 
> --

Re: RFR: 8250876: Build system preparation to macos on aarch64

[Kim Barrett]

> On Aug 1, 2020, at 3:24 AM, Vladimir Kempik  wrote:
> 
> Hello
> 
> Please review this change for JDK-8250876
> 
> This changeset adds support for macos/aarch64 into build system.
> It will allow to crosscompile for macos/aarch64 using intel mac as well.
> 
> This changeset does NOT address some arm specific issues in the macos related 
> code, we plan to do that in s separate commit.
> 
> An example of configure to cross-compile for macos/arm64:
> 
> --with-boot-jdk=/path/to/java/ 
> --with-build-jdk=/path/to/same/java/as/compiled  --disable-warnings-as-errors 
> --with-jvm-variants=zero --openjdk-target=aarch64-apple-darwin 
> --with-extra-cflags='-arch arm64' --with-extra-ldflags='-arch arm64 
> -F/Path/To/Folder/Containing/JNF_framework/' —with-extra-cxxflags='-arch 
> arm64’
> 
> JNF.framework is missing arm64 part as of next macos release, but Apple has 
> opensourced it. 
> 
> Fix to adlc were needed due to it using symbols from stdc++ and not linking 
> to it, so it fails when doing make images.
> 
> The webrev: http://cr.openjdk.java.net/~vkempik/8250876/webrev.00/
> The bug: https://bugs.openjdk.java.net/browse/JDK-8250876
> 
> Testing: jdk/submit.
> 
> Thanks, Vladimir.

Coming late to the party, as I see this has already been pushed.  But
one thing caught my eye.

--
  42   else ifeq ($(call isBuildOs, macosx), true)
  43 ADLC_LDFLAGS := -lc++

I'm surprised this is needed. I expected the C++ toolchain to
implicitly include that, the way g++ does.

If something like this really is needed, then it seems like it should
be ADLC_LIBS that should be modified, rather than ADLC_LDFLAGS.
Though I noticed there are currently no assignments to ADLC_LIBS.

I'm similarly surprised by this pre-existing bit in CoreLibraries.gmk:

$(eval $(call SetupJdkLibrary, BUILD_LIBJIMAGE, \
...
LIBS_unix := -ljvm -ldl $(LIBCXX), \
LIBS_macosx := -lc++, \

And that suggests a better place for all this (assuming its needed at
all) is in lib-std.m4, setting LIBCXX for macosx and using it where
needed.

--

Re: RFR: 8246032: Implementation of JEP 347: Adopt C++14 Language Features in HotSpot

[Kim Barrett]

> On Jul 7, 2020, at 9:59 AM, Erik Joelsson  wrote:
> 
> Doc changes look good.
> 
> /Erik
> 
> On 2020-07-06 18:10, Kim Barrett wrote:
>> I just noticed that doc/building.{md,html} describes minimum toolchain 
>> versions,
>> so also needs to be updated.  Here’s the update, matching what’s now in the 
>> JEP:
>> 
>> full: https://cr.openjdk.java.net/~kbarrett/8246032/open.05/
>> incr: https://cr.openjdk.java.net/~kbarrett/8246032/open.05.inc/
>> 
>> I also deleted a discussion of a problem one might run into when building 
>> with
>> Visual Studio 2010, since that version is no longer relevant.

Thanks.

Re: RFR: 8246032: Implementation of JEP 347: Adopt C++14 Language Features in HotSpot

[Kim Barrett]

I just noticed that doc/building.{md,html} describes minimum toolchain versions,
so also needs to be updated.  Here’s the update, matching what’s now in the JEP:

full: https://cr.openjdk.java.net/~kbarrett/8246032/open.05/
incr: https://cr.openjdk.java.net/~kbarrett/8246032/open.05.inc/

I also deleted a discussion of a problem one might run into when building with
Visual Studio 2010, since that version is no longer relevant.

Re: RFR: JDK-8248548 Use DISABLED_WARNINGS for globally disabled warnings on Visual Studio in Hotspot

[Kim Barrett]

> On Jun 30, 2020, at 6:08 AM, Magnus Ihse Bursie 
>  wrote:
> 
> Currently hotspot/share/utilities/globalDefinitions_visCPP.hpp contains a lot 
> of #pragma warning( disable : ...).
> 
> All these globally disabled warnings should move to the make files instead.
> 
> I also cleaned out some versions checks that are no longer relevant for the 
> range of supported versions of Microsoft Visual Studio.
> 
> Bug: https://bugs.openjdk.java.net/browse/JDK-8248548
> WebRev: 
> http://cr.openjdk.java.net/~ihse/JDK-8248548-use-DISABLED_WARNINGS_microsoft-for-hotspot/webrev.01
> 
> /Magnus

I think it would be nice to have a comment block describing each of the 
disabled warnings.
But thanks for sorting them, unlike the globalDefinitions code.

(Even better would be to have comments describing why we’re disabling them, but 
we don’t do
that for any other platform either.)

Re: RFR: JDK-8248548 Use DISABLED_WARNINGS for globally disabled warnings on Visual Studio in Hotspot

[Kim Barrett]

> On Jun 30, 2020, at 6:48 AM, Magnus Ihse Bursie 
>  wrote:
> 
> On 2020-06-30 12:32, Kim Barrett wrote:
>>> On Jun 30, 2020, at 6:08 AM, Magnus Ihse Bursie 
>>>  wrote:
>>> 
>>> Currently hotspot/share/utilities/globalDefinitions_visCPP.hpp contains a 
>>> lot of #pragma warning( disable : ...).
>>> 
>>> All these globally disabled warnings should move to the make files instead.
>>> 
>>> I also cleaned out some versions checks that are no longer relevant for the 
>>> range of supported versions of Microsoft Visual Studio.
>>> 
>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8248548
>>> WebRev: 
>>> http://cr.openjdk.java.net/~ihse/JDK-8248548-use-DISABLED_WARNINGS_microsoft-for-hotspot/webrev.01
>>> 
>>> /Magnus
>> I think it would be nice to have a comment block describing each of the 
>> disabled warnings.
>> But thanks for sorting them, unlike the globalDefinitions code.
>> 
>> (Even better would be to have comments describing why we’re disabling them, 
>> but we don’t do
>> that for any other platform either.)
>> 
> We used to have that for solaris. It ended up as a long document inside the 
> code, hiding the actual code. I agree that it would be good to have a 
> rationale on why we disable warnings, and that it should be documented. But I 
> think the source code is the wrong location for that. Sure, non-descriptive 
> warnings designations like microsoft has is typically more in need of 
> documentation than the gcc-style, but the important thing is *why* it is 
> disabled, not *what* is disabled.
> 
> So I'd argue that we should not pollute the source code with lines upon lines 
> of warning messages, but if anything, we should instead point to an external 
> location  where we can provide not only an explanation of what the warnings 
> does, but a rationale for disabling it.
> 
> /Magnus

I can understand that.  Maybe a job for the HotSpot Style Guide. Oh wait, what 
am I saying.

Change looks good.

Re: [OpenJDK 2D-Dev] RFR: 8246032: Implementation of JEP 347: Adopt C++14 Language Features in HotSpot

[Kim Barrett]

> On Jun 24, 2020, at 7:01 AM, Magnus Ihse Bursie 
>  wrote:
> 
> On 2020-06-18 08:34, Kim Barrett wrote:
>>> On Jun 15, 2020, at 7:41 AM, Kim Barrett  wrote:
>>> 
>>>> On Jun 14, 2020, at 12:45 AM, Philip Race  wrote:
>>>> 
>>>> Kim,
>>>> 
>>>> 
>>>> Until it says in "the JDK" and not "in HotSpot" you have not addressed my 
>>>> main point.
>>>> Please rename the JEP.
>> After some off-list discussions I have a better idea of what Phil was
>> asking for and why. In response I've changed the JEP, replacing a few
>> occurrences of "HotSpot" with "the JDK", as described below.  All
>> other occurrences of "HotSpot" have been left as-is.
>> 
>> Title:
>> JEP 347: Adopt C++14 Language Features in HotSpot
>> =>
>> JEP 347: Adopt C++14 Language Features in the JDK
>> 
>> Summary:
>> Allow the use of selected C++14 language features in the HotSpot C++ source 
>> code.
>> =>
>> Allow the use of selected C++14 language features in the JDK C++ source code.
>> 
>> Goals:
>> The purpose of this JEP is to formally allow C++ source code changes within 
>> HotSpot to take advantage of some C++14 standard language features.
>> =>
>> The purpose of this JEP is to formally allow C++ source code changes within 
>> the JDK to take advantage of some C++14 standard language features.
>> 
> This all looks good to me.
> 
> /Magnus

Thanks.

Re: [OpenJDK 2D-Dev] RFR: 8246032: Implementation of JEP 347: Adopt C++14 Language Features in HotSpot

[Kim Barrett]

> On Jun 15, 2020, at 7:41 AM, Kim Barrett  wrote:
> 
>> On Jun 14, 2020, at 12:45 AM, Philip Race  wrote:
>> 
>> Kim,
>> 
>> 
>> Until it says in "the JDK" and not "in HotSpot" you have not addressed my 
>> main point.
>> Please rename the JEP.

After some off-list discussions I have a better idea of what Phil was
asking for and why. In response I've changed the JEP, replacing a few
occurrences of "HotSpot" with "the JDK", as described below.  All
other occurrences of "HotSpot" have been left as-is.

Title:
JEP 347: Adopt C++14 Language Features in HotSpot
=>
JEP 347: Adopt C++14 Language Features in the JDK

Summary:
Allow the use of selected C++14 language features in the HotSpot C++ source 
code.
=>
Allow the use of selected C++14 language features in the JDK C++ source code.

Goals:
The purpose of this JEP is to formally allow C++ source code changes within 
HotSpot to take advantage of some C++14 standard language features.
=>
The purpose of this JEP is to formally allow C++ source code changes within the 
JDK to take advantage of some C++14 standard language features.

Re: [OpenJDK 2D-Dev] RFR: 8246032: Implementation of JEP 347: Adopt C++14 Language Features in HotSpot

[Kim Barrett]

> On Jun 14, 2020, at 12:45 AM, Philip Race  wrote:
> 
> Kim,
> 
> 
> Until it says in "the JDK" and not "in HotSpot" you have not addressed my 
> main point.
> Please rename the JEP.

I think this JEP is primarily about updating the HotSpot-specific
subset of C++ and usage guidance to include some C++14 features.
HotSpot usage is already different in some ways from C++ code in other
parts of the JDK.  For example, HotSpot code doesn't use global
operator new; that isn't likely to change, and non-HotSpot code
doesn't have access to the alternatives defined and used but not
exported by HotSpot.

As a necessary prerequisite for that, the JDK build system is being
updated to enable the use of C++14 features.  It's probably possible
to limit the scope of the language change to HotSpot, but that doesn't
seem useful.  It also probably wasn't possible while Solaris was still
in the mix.

It is expected that groups responsible for other parts of the JDK will
also want to take advantage of that build change, but I think it's up
to those other groups to decide on an adoption strategy.  I have no
reason to think the choices being suggested here as appropriate for
HotSpot are appropriate JDK-wide.

I can't say whether a JEP is generally needed; maybe it's simply
"C++14 - yes" in some areas.  Also, some of the reasons for this being
a JEP no longer apply.  For example, Solaris was going to require at
least a major ABI change, and at the time the JEP was created there
was no path to C++14 for the AIX/PPC port; both of those needed
visibility and discussion that seemed best provided via the JEP
process.

> -phil.
> 
> On 6/13/20, 8:48 PM, Kim Barrett wrote:
>>> On Jun 10, 2020, at 2:26 AM, Kim Barrett  wrote:
>>> 
>>>> On Jun 8, 2020, at 4:07 PM, Philip Race  wrote:
>>>> BTW the JEP (https://bugs.openjdk.java.net/browse/JDK-8208089) still says
>>>>> For Solaris: Add the -std=c++14 compiler option. .
>>>> So I am sure there is still some room to update the JEP :-)
>>> Yeah, I have some updates to make.  I also need to do a bit of work on the 
>>> feature lists.
>> I think the JEP updates are done now.

Re: [OpenJDK 2D-Dev] RFR: 8246032: Implementation of JEP 347: Adopt C++14 Language Features in HotSpot

[Kim Barrett]

> On Jun 10, 2020, at 2:26 AM, Kim Barrett  wrote:
> 
>> On Jun 8, 2020, at 4:07 PM, Philip Race  wrote:
> 
>> BTW the JEP (https://bugs.openjdk.java.net/browse/JDK-8208089) still says
>>> For Solaris: Add the -std=c++14 compiler option. .
>> 
>> So I am sure there is still some room to update the JEP :-)
> 
> Yeah, I have some updates to make.  I also need to do a bit of work on the 
> feature lists.

I think the JEP updates are done now.

Re: [OpenJDK 2D-Dev] RFR: 8246032: Implementation of JEP 347: Adopt C++14 Language Features in HotSpot

[Kim Barrett]

> On Jun 8, 2020, at 4:07 PM, Philip Race  wrote:
> 
> Hi Kim,
> 
> Can we amend the JEP itself to be not solely about hotspot.
> Since it affects other areas I think that
> 1) They may need to be compiled with C++14 enabled whether they use new 
> features or not
> 2) They may want - or need - to adopt C++11 or C++14 features too.
> 
> You already know that soon we will upgrade the harfbuzz 3rd party library 
> used by 2D
> and it will bring along the need to use C++11 features and I had no plan to 
> propose a JEP for that.
> So I don't want to be asked why we didn't do one when hotspot did !
> So this really ought to be a JDK JEP whilst of course explaining that hotspot 
> is expected
> to be the primary adopter.

I think this is at least mostly covered by the following paragraph from the JEP:

(This JEP does not propose any usage or style changes for C++
code in the JDK that is outside of HotSpot.  The rules are already different
for HotSpot and non-HotSpot code.  For example, C++ exceptions are
used in some non-HotSpot code, but are disallowed in HotSpot by
build-time options.  However, build consistency requirements will make
the newer language features available for use in all C++ code in the JDK.)

That’s why I cc’d the RFR to several mailing lists in addition to hotspot-dev.

Is there something additional you would like to add?

> BTW the JEP (https://bugs.openjdk.java.net/browse/JDK-8208089) still says
> > For Solaris: Add the -std=c++14 compiler option. .
> 
> So I am sure there is still some room to update the JEP :-)

Yeah, I have some updates to make.  I also need to do a bit of work on the 
feature lists.

Re: RFR: 8246032: Implementation of JEP 347: Adopt C++14 Language Features in HotSpot

[Kim Barrett]

> On Jun 5, 2020, at 7:57 PM, Magnus Ihse Bursie 
>  wrote:
> 
> On 2020-06-05 09:52, Kim Barrett wrote:
>> [Changes are only to the build system, but since the changes have jdk-wide
>> effect I’ve cc’d what I think are the relevant dev lists.]
>> 
>> This change is part of JEP 347; the intent is to target JDK 16.
>> 
>> Please review this change to the building of C++ code in the JDK to
>> enable the use of C++14 language features.  This change does not make
>> any code changes to use new features provided by C++11 or C++14.
>> 
>> This requires trimming the supported compiler versions, moving the
>> minimum supported versions forward (significantly, in some cases).
>> The new minimums are based on compiler documentation rather than
>> testing.  It may be that more recent versions are actually required.
>> 
>> This change needs to be tested on platforms not supported by Oracle.
>> The JEP test plan includes additional Oracle testing beyond what I’ve done.
>> 
>> CR:
>> https://bugs.openjdk.java.net/browse/JDK-8246032
>> 
>> Webrev:
>> https://cr.openjdk.java.net/~kbarrett/8246032/open.02/
> Looks good to me.

Thanks.

> 
> /Magnus
> 
>> 
>> Testing:
>> mach5 tier1-5 on Oracle supported platforms.
>> 
>> Performance testing showed no significant changes in either direction.
>> 
>> Build-only (no tests) for linux-arm32, linux-s390x, linux-ppc64le

Re: RFR: 8246032: Implementation of JEP 347: Adopt C++14 Language Features in HotSpot

[Kim Barrett]

> On Jun 5, 2020, at 7:57 PM, Magnus Ihse Bursie 
>  wrote:
> 
> On 2020-06-05 13:59, Jim Laskey wrote:
>> I know there was a discussion about this elsewhere but I would like to take 
>> the opportunity to correct this now
>> 
>> make//autoconf/flags-cflags.m4:241
>> 
>> […]
>> MacOSX has been paying a historic and significant performance penalty for no 
>> valid reason.
> This might be a valid change, but it has nothing to do with C++14, and 
> changing it at the same time will increase risk for unrelated strange errors. 
> Please open a separate JBS issue for this requested change.

I was going to say much the same thing.  I don’t want to add that to the C++14 
mix.  I don’t even
know for sure that it’s a desirable change.  I’ve worked on projects that got 
better (measured)
performance from -Os than from other global alternatives. One would need to do 
a bunch of
performance testing and might need to go more deeply into the options to really 
optimize things.  

Re: RFR: 8246032: Implementation of JEP 347: Adopt C++14 Language Features in HotSpot

[Kim Barrett]

> On Jun 5, 2020, at 9:34 AM, Doerr, Martin  wrote:
> 
> Hi Kim,
> 
> builds out of the box on AIX with IBM XL C/C++ for AIX, V16.1.0 (We already 
> use clang frontend by default.)
> Very cool!

Thanks for taking it for a spin.

RFR: 8246032: Implementation of JEP 347: Adopt C++14 Language Features in HotSpot

[Kim Barrett]

[Changes are only to the build system, but since the changes have jdk-wide
effect I’ve cc’d what I think are the relevant dev lists.]

This change is part of JEP 347; the intent is to target JDK 16.

Please review this change to the building of C++ code in the JDK to
enable the use of C++14 language features.  This change does not make
any code changes to use new features provided by C++11 or C++14.

This requires trimming the supported compiler versions, moving the
minimum supported versions forward (significantly, in some cases).
The new minimums are based on compiler documentation rather than
testing.  It may be that more recent versions are actually required.

This change needs to be tested on platforms not supported by Oracle.
The JEP test plan includes additional Oracle testing beyond what I’ve done.

CR:
https://bugs.openjdk.java.net/browse/JDK-8246032

Webrev:
https://cr.openjdk.java.net/~kbarrett/8246032/open.02/

Testing:
mach5 tier1-5 on Oracle supported platforms.

Performance testing showed no significant changes in either direction.

Build-only (no tests) for linux-arm32, linux-s390x, linux-ppc64le

Re: RFR: 8240259: Disable -Wshift-negative-value warnings

[Kim Barrett]

> On May 22, 2020, at 3:38 AM, Magnus Ihse Bursie 
>  wrote:
> 
> Looks good to me.

Thanks.

> 
> /Magnus
> 
> On 2020-05-22 03:54, Kim Barrett wrote:
>> Please review this change which disables warnings for left shift of a
>> negative value when compiling HotSpot with gcc or clang.  This warning
>> isn't helpful, given that all compilers seem to do the obvious thing,
>> and that obvious thing will soon-ish (C++20) be the specified thing.
>> See the CR for more details.
>> 
>> CR:
>> https://bugs.openjdk.java.net/browse/JDK-8240259
>> 
>> Webrev:
>> https://cr.openjdk.java.net/~kbarrett/8240259/open.00/
>> 
>> Testing:
>> mach5 tier1.  Also tier1-5 with C++14 enabled.
>> 
>> Note that there are already occurrences of left shift of negative
>> values in our code, with no apparent consequences, even when testing
>> with C++11/14 enabled.

RFR: 8240259: Disable -Wshift-negative-value warnings

[Kim Barrett]

Please review this change which disables warnings for left shift of a
negative value when compiling HotSpot with gcc or clang.  This warning
isn't helpful, given that all compilers seem to do the obvious thing,
and that obvious thing will soon-ish (C++20) be the specified thing.
See the CR for more details.

CR:
https://bugs.openjdk.java.net/browse/JDK-8240259

Webrev:
https://cr.openjdk.java.net/~kbarrett/8240259/open.00/

Testing:
mach5 tier1.  Also tier1-5 with C++14 enabled.

Note that there are already occurrences of left shift of negative
values in our code, with no apparent consequences, even when testing
with C++11/14 enabled.

Re: RFR: JDK-8244653 Suppress gcc 9.1 ABI change notes on aarch64

[Kim Barrett]

> On May 8, 2020, at 7:12 AM, Magnus Ihse Bursie 
>  wrote:
> 
> When building HotSpot with gcc9.x for aarch64, there are a couple of places 
> that trigger a "warning" (technically a "note") about an ABI change from 
> earlier versions.  The message is
> 
> : note: parameter passing for argument of type ' changed in 
> GCC 9.1
> 
> This is mentioned prominently in the gcc 9 release notes:
> 
> "On Arm targets (arm*-*-*), a bug in the implementation of the procedure call 
> standard (AAPCS) in the GCC 6, 7 and 8 releases has been fixed: a structure 
> containing a bit-field based on a 64-bit integral type and where no other 
> element in a structure required 64-bit alignment could be passed incorrectly 
> to functions. This is an ABI change. If the option -Wpsabi is enabled (on by 
> default) the compiler will emit a diagnostic note for code that might be 
> affected."
> 
> As the only HotSpot types being warned about are internal and do not cross 
> library boundaries, and we compile the entire jdk with the same compiler, 
> these "warnings" are not interesting and just clutter build logs and cause 
> unnecessary worry.
> 
> To suppress these notes, -Wno-psabi should be added to the compiler options 
> for HotSpot when building for any flavor of ARM, i.e. when the $VAR_CPU build 
> variable is "arm" or "aarch64". That option is already present for "arm" 
> because of a similar issue with gcc 4.4. We should add it for "aarch64".
> 
> Bug: https://bugs.openjdk.java.net/browse/JDK-8244653
> WebRev: 
> http://cr.openjdk.java.net/~ihse/JDK-8244653-no-psabi-for-aarch64/webrev.01
> 
> /Magnus

Looks good.

Re: RFR: 8244224: Implementation of JEP 381: Remove the Solaris and SPARC Ports (build system)

[Kim Barrett]

> On May 6, 2020, at 8:47 PM, Mikael Vidstedt  
> wrote:
> 
> 
> New webrev available here:
> 
> webrev: 
> http://cr.openjdk.java.net/~mikael/webrevs/8244224/webrev.01/build/open/webrev/
>  
> 
> incremental: 
> http://cr.openjdk.java.net/~mikael/webrevs/8244224/webrev.01/build.incr/open/webrev/
>  
> 
> 
> I believe I have addressed all the review comments apart from:
> 
> * GNM - Magnus to file a follow-up RFE
> 
> * Zero - Waiting for somebody (Adrian?) to provide more information about 
> what needs to be preserved

Looks good.

Re: RFR: 8244224: Implementation of JEP 381: Remove the Solaris and SPARC Ports (build system)

[Kim Barrett]

> On May 4, 2020, at 1:12 AM, Mikael Vidstedt  
> wrote:
> 
> 
> Please review this change which implements part of JEP 381:
> 
> JBS: https://bugs.openjdk.java.net/browse/JDK-8244224
> webrev: 
> http://cr.openjdk.java.net/~mikael/webrevs/8244224/webrev.00/build/open/webrev/
> JEP: https://bugs.openjdk.java.net/browse/JDK-8241787

Looks good to me.

One comment:

--
build/open/webrev/make/autoconf/libraries.m4
 107   if test "x$TOOLCHAIN_TYPE" = xsolstudio; then
 108 GLOBAL_LIBS="-lc"
 109   else
 110 GLOBAL_LIBS=""
 111   fi
=>
 106   GLOBAL_LIBS=""

With this change, GLOBAL_LIBS appears to never have a non-empty value.
Maybe it's no longer needed at all.

--

Re: RFR: 8239357: Revert gcc implementation of offset_of

[Kim Barrett]

> On Apr 22, 2020, at 9:28 AM, Erik Joelsson  wrote:
> 
> Build change looks good.

Thanks.

> 
> /Erik
> 
> On 2020-04-22 06:23, Kim Barrett wrote:
>> Please review this change to undo the (small) portion of JDK-8238281
>> [1] related to the HotSpot offset_of macro for gcc/clang.  That change
>> should not have been made.  See CR for details.
>> 
>> I did not reinstate the redefinition of offsetof; we shouldn't be
>> using that macro, and having the warning enabled will catch
>> questionable cases.  (I also considered but didn’t poison it.)
>> 
>> CR:
>> https://bugs.openjdk.java.net/browse/JDK-8239357
>> 
>> [1] https://bugs.openjdk.java.net/browse/JDK-8238281
>> 
>> Webrev:
>> https://cr.openjdk.java.net/~kbarrett/8239357/open.00/
>> 
>> Testing:
>> mach5 tier1
>> 
>> With a temporary change to use offsetof with a non-standard-layout class,
>> verified the expected build failures occur with gcc and clang.

Re: RFR: 8239357: Revert gcc implementation of offset_of

[Kim Barrett]

> On Apr 22, 2020, at 9:57 AM, Magnus Ihse Bursie 
>  wrote:
> 
> On 2020-04-22 15:23, Kim Barrett wrote:
>> Please review this change to undo the (small) portion of JDK-8238281
>> [1] related to the HotSpot offset_of macro for gcc/clang.  That change
>> should not have been made.  See CR for details.
>> 
>> I did not reinstate the redefinition of offsetof; we shouldn't be
>> using that macro, and having the warning enabled will catch
>> questionable cases.  (I also considered but didn’t poison it.)
>> 
>> CR:
>> https://bugs.openjdk.java.net/browse/JDK-8239357
>> 
>> [1] https://bugs.openjdk.java.net/browse/JDK-8238281
>> 
>> Webrev:
>> https://cr.openjdk.java.net/~kbarrett/8239357/open.00/
> Looks good to me.
> 
> /Magnus

Thanks.

>> 
>> Testing:
>> mach5 tier1
>> 
>> With a temporary change to use offsetof with a non-standard-layout class,
>> verified the expected build failures occur with gcc and clang.

Re: RFR: 8239357: Revert gcc implementation of offset_of

[Kim Barrett]

> On Apr 22, 2020, at 10:33 PM, David Holmes  wrote:
> 
> On 22/04/2020 11:23 pm, Kim Barrett wrote:
>> Please review this change to undo the (small) portion of JDK-8238281
>> [1] related to the HotSpot offset_of macro for gcc/clang.  That change
>> should not have been made.  See CR for details.
>> I did not reinstate the redefinition of offsetof; we shouldn't be
>> using that macro, and having the warning enabled will catch
>> questionable cases.  (I also considered but didn’t poison it.)
> 
> Okay. This seems reasonable.
> 
> Thanks,
> David

Thanks.

> 
>> CR:
>> https://bugs.openjdk.java.net/browse/JDK-8239357
>> [1] https://bugs.openjdk.java.net/browse/JDK-8238281
>> Webrev:
>> https://cr.openjdk.java.net/~kbarrett/8239357/open.00/
>> Testing:
>> mach5 tier1
>> With a temporary change to use offsetof with a non-standard-layout class,
>> verified the expected build failures occur with gcc and clang.

RFR: 8239357: Revert gcc implementation of offset_of

[Kim Barrett]

Please review this change to undo the (small) portion of JDK-8238281
[1] related to the HotSpot offset_of macro for gcc/clang.  That change
should not have been made.  See CR for details.

I did not reinstate the redefinition of offsetof; we shouldn't be
using that macro, and having the warning enabled will catch
questionable cases.  (I also considered but didn’t poison it.)

CR:
https://bugs.openjdk.java.net/browse/JDK-8239357

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

Webrev:
https://cr.openjdk.java.net/~kbarrett/8239357/open.00/

Testing:
mach5 tier1

With a temporary change to use offsetof with a non-standard-layout class,
verified the expected build failures occur with gcc and clang.

Re: RFR: JDK-8240224 Allow building hotspot without the serial gc

[Kim Barrett]

> On Mar 10, 2020, at 10:51 AM, Magnus Ihse Bursie 
>  wrote:
>>> Nits:
>>> 
>>> *) src/hotspot/share/gc/shared/gcConfig.cpp changes are a bit strange:
>>>   - Epsilon should not ever be selected by ergonomics
>>>   - Why ZGC is selected before Shenandoah? [Oh, what a can of worms that 
>>> one is ;)]
>> 
>> This fallback list is clearly just meant to allow for any combination of GCs 
>> being compiled into the JVM. If the only one you picked was epsilon, then 
>> what other default would you expect? It's last in the list so any other GC 
>> will still be prioritized before it if present. For the same reason, the 
>> order of ZGC and Shenandoah is irrelevant and could just as well be the 
>> other way. It will never have any real consequence. This code is only there 
>> to keep things from falling apart when a non standard combination of jvm 
>> features is picked.
> Exactly. For good measure, I can surely put Shenandoah before ZGC. :)

Whichever one is placed first will likely annoy the folks behind the competing 
second.  There’s
no way to win this one.

> The idea behind the added defaults as fallback is just to allow the JVM to 
> even start if Serial GC is not present. If you configure with SerialGC 
> (which, as you note, is the typical case), this change will not affect 
> anything. Without this, it is not even possible to complete the build without 
> SerialGC, since jlink cannot run.

The is_server_class_machine() test is intended to filter out collectors that 
(probably)
don’t make sense to run on “small” machines.  (Admittedly, it’s not so easy to 
buy a
computer that doesn’t qualify for is_server_class_machine() anymore, outside of 
the
embedded space, and even there…)  But we let one insist by allowing the default 
to
be overridden by an explicit selection.

Re: RFR: JDK-8240224 Allow building hotspot without the serial gc

[Kim Barrett]

> On Mar 10, 2020, at 7:22 PM, David Holmes  wrote:
> Also I'm not at all clear what happens if the only GC configured is one of 
> the experimental GCs for which we would normally have to set 
> -XX:+UnlockExperimentalVMOptions ??

Yes, it seems wrong to ever select an experimental GC by default.