Re: RFR: 8288396: Always create reproducible builds [v2]

[Magnus Ihse Bursie]

On Tue, 14 Jun 2022 11:54:40 GMT, Erik Joelsson  wrote:

>> Magnus Ihse Bursie has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   Fix exitTransportWithError signature
>
> make/autoconf/flags-ldflags.m4 line 132:
> 
>> 130: 
>> 131:   if test "x$TOOLCHAIN_TYPE" = xmicrosoft; then
>> 132: REPRODUCIBLE_LDFLAGS="-experimental:deterministic"
> 
> For the cflag, we check that the compiler supports it, but for the linker 
> flag you are just setting it without a check. Before this patch, if we got 
> here and ENABLE_REPRODUCIBLE_BUILD was true, it meant that the test had 
> passed for the compiler, from which we could assume it would also work for 
> the linker, but that is no longer the case.

Good point.

-

PR: https://git.openjdk.org/jdk/pull/9152

Re: RFR: 8288396: Always create reproducible builds [v2]

[Magnus Ihse Bursie]

On Tue, 14 Jun 2022 10:09:37 GMT, Magnus Ihse Bursie  wrote:

>> When we started introducing some possibly more intrusive compiler flags and 
>> functionality for reproducible builds, we also introduced a flag to turn 
>> this off  out of an abundance of caution. But we have been been using this 
>> configuration for a year or so internally within Oracle, with no issues. So 
>> there's really no reason to be able to turn this off. (If you were to ask 
>> me, the fact that compilers and build tools ever started to produce 
>> non-deterministic output has been a bug from day one.)
>> 
>> With this fix, all randomness should be gone from our builds, at least on 
>> linux and windows. There are no more `__DATE__` and `__TIME__` macros in the 
>> source code.
>
> Magnus Ihse Bursie has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Fix exitTransportWithError signature

I made some additional cleanup associated with shmem. I think that `sysAssert` 
can (and probably should) be replaced with `SHMEM_ASSERT`. I can fix that as 
well, if someone from serviceability says that I should do it.

-

PR: https://git.openjdk.org/jdk/pull/9152

Re: RFR: 8288396: Always create reproducible builds [v2]

[Magnus Ihse Bursie]

> When we started introducing some possibly more intrusive compiler flags and 
> functionality for reproducible builds, we also introduced a flag to turn this 
> off  out of an abundance of caution. But we have been been using this 
> configuration for a year or so internally within Oracle, with no issues. So 
> there's really no reason to be able to turn this off. (If you were to ask me, 
> the fact that compilers and build tools ever started to produce 
> non-deterministic output has been a bug from day one.)
> 
> With this fix, all randomness should be gone from our builds, at least on 
> linux and windows. There are no more `__DATE__` and `__TIME__` macros in the 
> source code.

Magnus Ihse Bursie has updated the pull request incrementally with one 
additional commit since the last revision:

  Fix exitTransportWithError signature

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/9152/files
  - new: https://git.openjdk.org/jdk/pull/9152/files/8bc40ddb..e2f5fc05

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

  Stats: 33 lines in 5 files changed: 0 ins; 27 del; 6 mod
  Patch: https://git.openjdk.org/jdk/pull/9152.diff
  Fetch: git fetch https://git.openjdk.org/jdk pull/9152/head:pull/9152

PR: https://git.openjdk.org/jdk/pull/9152

Re: RFR: 8288396: Always create reproducible builds

[Magnus Ihse Bursie]

On Tue, 14 Jun 2022 09:48:25 GMT, Magnus Ihse Bursie  wrote:

> When we started introducing some possibly more intrusive compiler flags and 
> functionality for reproducible builds, we also introduced a flag to turn this 
> off  out of an abundance of caution. But we have been been using this 
> configuration for a year or so internally within Oracle, with no issues. So 
> there's really no reason to be able to turn this off. (If you were to ask me, 
> the fact that compilers and build tools ever started to produce 
> non-deterministic output has been a bug from day one.)
> 
> With this fix, all randomness should be gone from our builds, at least on 
> linux and windows. There are no more `__DATE__` and `__TIME__` macros in the 
> source code.

This PR also include a more "radical" version of JDK-8287894, which probably 
should have been adopted by JDK-8287894 in the first place. There is no need to 
include the build date in the assert strings for shmem on Windows.

-

PR: https://git.openjdk.org/jdk/pull/9152

RFR: 8288396: Always create reproducible builds

[Magnus Ihse Bursie]

When we started introducing some possibly more intrusive compiler flags and 
functionality for reproducible builds, we also introduced a flag to turn this 
off  out of an abundance of caution. But we have been been using this 
configuration for a year or so internally within Oracle, with no issues. So 
there's really no reason to be able to turn this off. (If you were to ask me, 
the fact that compilers and build tools ever started to produce 
non-deterministic output has been a bug from day one.)

With this fix, all randomness should be gone from our builds, at least on linux 
and windows. There are no more `__DATE__` and `__TIME__` macros in the source 
code.

-

Commit messages:
 - 8288396: Always create reproducible builds

Changes: https://git.openjdk.org/jdk/pull/9152/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk=9152=00
  Issue: https://bugs.openjdk.org/browse/JDK-8288396
  Stats: 94 lines in 15 files changed: 11 ins; 63 del; 20 mod
  Patch: https://git.openjdk.org/jdk/pull/9152.diff
  Fetch: git fetch https://git.openjdk.org/jdk pull/9152/head:pull/9152

PR: https://git.openjdk.org/jdk/pull/9152

Re: RFR: 8287894: Use fixed timestamp as an alternative of __DATE__ macro in jdk.jdi to make Windows build reproducible

[Magnus Ihse Bursie]

On Tue, 7 Jun 2022 19:57:48 GMT, Alexey Pavlyutkin  
wrote:

> Hi!
> 
> Here is a fix to jdk.jdi that fixes reproducible build for Windows. The idea 
> of the fix is to re-use value of --with-hotspot-build-time option to generate 
> deterministic timestamp exactly like it's done to hotspot component.
> 
> Verification (Windows-10/MSVS-2019): ```bash ./configure 
> --with-boot-jdk=c:/work/boot-jdk/jdk-18 --with-debug-level=fastdebug 
> --with-hotspot-build-time="6/7/2022 2:35pm" 
> --with-extra-cflags="/experimental:deterministic" 
> --with-extra-cxxflags="/experimental:deterministic"```
> 
> Regression (Windows-10/MSVS-2019): ```bash ./configure 
> --with-boot-jdk=c:/work/boot-jdk/jdk-18 --with-debug-level=fastdebug```

Looks good. Thanks for fixing this!

-

Marked as reviewed by ihse (Reviewer).

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

Integrated: 8287254: Clean up Xcode sysroot logic

[Magnus Ihse Bursie]

On Tue, 24 May 2022 17:09:10 GMT, Magnus Ihse Bursie  wrote:

> The logic in BASIC_SETUP_DEVKIT for setting a correct sysroot for Xcode is 
> hard to follow. This should be straightened out. We also expose variables 
> that are no longer used. So there's a bit of related cleanup. 
> 
> The new code is more or less functionally equivalent to the old. There were 
> some corner cases which the old code did not handle well; this has now been 
> improved. I've also added yet another method of trying to get the SDK root 
> before falling back to the system library, by using `xcrun --show-sdk-path`.
> 
> In an ideal world, the sysroot flags to `mig` should be set in configure, 
> e.g. as `MIG_FLAGS` or `MIG_SYSROOT_FLAGS`. I can't be bothered to do that 
> for a single call to `mig`, though.
> 
> As always, changes like this that depend on the environment is tricky to 
> test. I've tried running it on a couple of different macs, with and without a 
> devkit.

This pull request has now been integrated.

Changeset: d889319a
Author:Magnus Ihse Bursie 
URL:   
https://git.openjdk.java.net/jdk/commit/d889319a86101e944aefd4ad7f300505abbe5b30
Stats: 200 lines in 4 files changed: 91 ins; 98 del; 11 mod

8287254: Clean up Xcode sysroot logic

Reviewed-by: erikj

-

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

RFR: 8287254: Clean up Xcode sysroot logic

[Magnus Ihse Bursie]

The logic in BASIC_SETUP_DEVKIT for setting a correct sysroot for Xcode is hard 
to follow. This should be straightened out. We also expose variables that are 
no longer used. So there's a bit of related cleanup. 

The new code is more or less functionally equivalent to the old. There were 
some corner cases which the old code did not handle well; this has now been 
improved. I've also added yet another method of trying to get the SDK root 
before falling back to the system library, by using `xcrun --show-sdk-path`.

In an ideal world, the sysroot flags to `mig` should be set in configure, e.g. 
as `MIG_FLAGS` or `MIG_SYSROOT_FLAGS`. I can't be bothered to do that for a 
single call to `mig`, though.

As always, changes like this that depend on the environment is tricky to test. 
I've tried running it on a couple of different macs, with and without a devkit.

-

Commit messages:
 - Make Xcode SDK locating more robust
 - * Clean up BASIC_SETUP_XCODE_SYSROOT
 - Don't export SDKROOT
 - Move sdk setup to BASIC_SETUP_XCODE_SYSROOT

Changes: https://git.openjdk.java.net/jdk/pull/8872/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk=8872=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8287254
  Stats: 200 lines in 4 files changed: 91 ins; 98 del; 11 mod
  Patch: https://git.openjdk.java.net/jdk/pull/8872.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/8872/head:pull/8872

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

Re: RFR: 8284209: Replace remaining usages of 'a the' in source code

[Magnus Ihse Bursie]

On Wed, 18 May 2022 14:46:42 GMT, Alexey Ivanov  wrote:

> Replaces usages of articles that follow each other in all combinations: 
> a/the, an?/an?, the/the…
> 
> It's the last issue in the series, and it still touches different areas of 
> the code.

Build changes look good. Thanks for the cleanup!

-

Marked as reviewed by ihse (Reviewer).

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

Integrated: 8285366: Fix typos in serviceability

[Magnus Ihse Bursie]

On Thu, 21 Apr 2022 11:22:48 GMT, Magnus Ihse Bursie  wrote:

> I ran `codespell` on modules owned by the serviceability team 
> (`java.instrument java.management.rmi java.management jdk.attach 
> jdk.hotspot.agent jdk.internal.jvmstat jdk.jcmd jdk.jconsole jdk.jdi 
> jdk.jdwp.agent jdk.jstatd jdk.management.agent jdk.management`), and accepted 
> those changes where it indeed discovered real typos.
> 
> 
> I will update copyright years using a script before pushing (otherwise like 
> every second change would be a copyright update, making reviewing much 
> harder).
> 
> The long term goal here is to make tooling support for running `codespell`. 
> The trouble with automating this is of course all false positives. But before 
> even trying to solve that issue, all true positives must be fixed. Hence this 
> PR.

This pull request has now been integrated.

Changeset: 76caeed4
Author:Magnus Ihse Bursie 
URL:   
https://git.openjdk.java.net/jdk/commit/76caeed498d868c7923461fb481349c0a2cbd99d
Stats: 303 lines in 136 files changed: 0 ins; 0 del; 303 mod

8285366: Fix typos in serviceability

Reviewed-by: kevinw, sspitsyn

-

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

Re: RFR: 8285366: Fix typos in serviceability [v3]

[Magnus Ihse Bursie]

> I ran `codespell` on modules owned by the serviceability team 
> (`java.instrument java.management.rmi java.management jdk.attach 
> jdk.hotspot.agent jdk.internal.jvmstat jdk.jcmd jdk.jconsole jdk.jdi 
> jdk.jdwp.agent jdk.jstatd jdk.management.agent jdk.management`), and accepted 
> those changes where it indeed discovered real typos.
> 
> 
> I will update copyright years using a script before pushing (otherwise like 
> every second change would be a copyright update, making reviewing much 
> harder).
> 
> The long term goal here is to make tooling support for running `codespell`. 
> The trouble with automating this is of course all false positives. But before 
> even trying to solve that issue, all true positives must be fixed. Hence this 
> PR.

Magnus Ihse Bursie has updated the pull request incrementally with one 
additional commit since the last revision:

  Update copyright year

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/8334/files
  - new: https://git.openjdk.java.net/jdk/pull/8334/files/ad36a59a..4490ef75

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

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

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

Re: RFR: 8285366: Fix typos in serviceability [v2]

[Magnus Ihse Bursie]

> I ran `codespell` on modules owned by the serviceability team 
> (`java.instrument java.management.rmi java.management jdk.attach 
> jdk.hotspot.agent jdk.internal.jvmstat jdk.jcmd jdk.jconsole jdk.jdi 
> jdk.jdwp.agent jdk.jstatd jdk.management.agent jdk.management`), and accepted 
> those changes where it indeed discovered real typos.
> 
> 
> I will update copyright years using a script before pushing (otherwise like 
> every second change would be a copyright update, making reviewing much 
> harder).
> 
> The long term goal here is to make tooling support for running `codespell`. 
> The trouble with automating this is of course all false positives. But before 
> even trying to solve that issue, all true positives must be fixed. Hence this 
> PR.

Magnus Ihse Bursie has updated the pull request incrementally with one 
additional commit since the last revision:

  Revert "invocable"

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/8334/files
  - new: https://git.openjdk.java.net/jdk/pull/8334/files/4b26668d..ad36a59a

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

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

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

Re: RFR: 8244681: Add a warning for possibly lossy conversion in compound assignments [v3]

[Magnus Ihse Bursie]

On Wed, 11 May 2022 13:05:45 GMT, Erik Gahlin  wrote:

>> make/modules/jdk.jfr/Java.gmk line 26:
>> 
>>> 24: #
>>> 25: 
>>> 26: DISABLED_WARNINGS_java += exports lossy-conversions
>> 
>> Note that with the fix of JDK-8286392 (and JDK-8286396) the 
>> `lossy-conversions` warning should not be disabled for the JFR code. 
>> 
>> In general, you need to check which of the subtasks of JDK-8286374 that has 
>> been fixed, and adjust the makefiles accordingly, before pushing this fix.
>> 
>> (In the future, it might be easier to push the fix which disables the 
>> warnings first, and then file follow-up bugs on aa per-component basis, and 
>> remind them to remove the disabling in the makefile. That way there won't be 
>> a race between individual fixes and a "master" bug like this.)
>
> I agree, but if it doesn't happen, I can follow up with a separate PR where I 
> remove the disablement.

That's good to know. I think the tricky part is mostly about keeping track of 
all these disabled warnings, so they are not kept around longer than necessary. 
And that needs coordination with all the subtasks of the umbrella issue.

-

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

Re: RFR: 8244681: Add a warning for possibly lossy conversion in compound assignments [v3]

[Magnus Ihse Bursie]

On Wed, 11 May 2022 07:45:39 GMT, Adam Sotona  wrote:

>> Please review this patch adding new lint option, **lossy-conversions**, to 
>> javac to warn about type casts in compound assignments with possible lossy 
>> conversions.
>> 
>> The new lint warning is shown if the type of the right-hand operand of a 
>> compound assignment is not assignment compatible with the type of the 
>> variable.
>> 
>> The implementation of the warning is based on similar check performed to 
>> emit "possible lossy conversion" compilation error for simple assignments. 
>> 
>> Proposed patch also include complex matrix-style test with positive and 
>> negative test cases of lossy conversions in compound assignments.
>> 
>> Proposed patch also disables this new lint option in all affected JDK 
>> modules and libraries to allow smooth JDK build. Individual cases to address 
>> possibly lossy conversions warnings in JDK are already addressed in a 
>> separate umbrella issue and its sub-tasks.
>> 
>> Thanks for your review,
>> Adam
>
> Adam Sotona has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   8244681: Add a warning for possibly lossy conversion in compound assignments
>   recommended correction of the warning description

Check updates on JDK-8286374 subtasks.

make/modules/jdk.jfr/Java.gmk line 26:

> 24: #
> 25: 
> 26: DISABLED_WARNINGS_java += exports lossy-conversions

Note that with the fix of JDK-8286392 (and JDK-8286396) the `lossy-conversions` 
warning should not be disabled for the JFR code. 

In general, you need to check which of the subtasks of JDK-8286374 that has 
been fixed, and adjust the makefiles accordingly, before pushing this fix.

(In the future, it might be easier to push the fix which disables the warnings 
first, and then file follow-up bugs on aa per-component basis, and remind them 
to remove the disabling in the makefile. That way there won't be a race between 
individual fixes and a "master" bug like this.)

-

Changes requested by ihse (Reviewer).

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

Re: RFR: 8285366: Fix typos in serviceability

[Magnus Ihse Bursie]

On Thu, 21 Apr 2022 16:17:20 GMT, Kevin Walls  wrote:

>> I ran `codespell` on modules owned by the serviceability team 
>> (`java.instrument java.management.rmi java.management jdk.attach 
>> jdk.hotspot.agent jdk.internal.jvmstat jdk.jcmd jdk.jconsole jdk.jdi 
>> jdk.jdwp.agent jdk.jstatd jdk.management.agent jdk.management`), and 
>> accepted those changes where it indeed discovered real typos.
>> 
>> 
>> I will update copyright years using a script before pushing (otherwise like 
>> every second change would be a copyright update, making reviewing much 
>> harder).
>> 
>> The long term goal here is to make tooling support for running `codespell`. 
>> The trouble with automating this is of course all false positives. But 
>> before even trying to solve that issue, all true positives must be fixed. 
>> Hence this PR.
>
> src/jdk.jdwp.agent/share/native/libjdwp/invoker.h line 38:
> 
>> 36: jboolean pending;  /* Is an invoke requested? */
>> 37: jboolean started;  /* Is an invoke happening? */
>> 38: jboolean available;/* Is the thread in an invocable state? */
> 
> invocable could perhaps stay as invokable ?
> Elsewhere we have a filename com/sun/tools/jdi/InvokableTypeImpl.java which 
> we clearly don't want to change,  and I see Internet dictionary evidence of 
> invokable being acceptable.

But on the other hand we have `javax.script.Invocable`. :-) 

Codespell suggested this change, and I based my decision to keep it based on 
[Merriam-Webster](https://www.merriam-webster.com/dictionary/invocable) not 
even listing "invokable" as an alternate spelling, and that "invocable" has 
about 3x the number of Google hits than "invokable". 

But sure, there is perhaps reason to consider "invokable" an acceptable 
alternative and keep it. I guess it depends on if you consider the word to be 
based on "invoke" with a suffix, or a latinized form, like "invocation". 

I'll wait a while for others to chime in, otherwise I'll revert the "invokable" 
changes.

-

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

Re: RFR: 8285366: Fix typos in serviceability

[Magnus Ihse Bursie]

On Thu, 21 Apr 2022 14:03:39 GMT, Daniel Fuchs  wrote:

>> I ran `codespell` on modules owned by the serviceability team 
>> (`java.instrument java.management.rmi java.management jdk.attach 
>> jdk.hotspot.agent jdk.internal.jvmstat jdk.jcmd jdk.jconsole jdk.jdi 
>> jdk.jdwp.agent jdk.jstatd jdk.management.agent jdk.management`), and 
>> accepted those changes where it indeed discovered real typos.
>> 
>> 
>> I will update copyright years using a script before pushing (otherwise like 
>> every second change would be a copyright update, making reviewing much 
>> harder).
>> 
>> The long term goal here is to make tooling support for running `codespell`. 
>> The trouble with automating this is of course all false positives. But 
>> before even trying to solve that issue, all true positives must be fixed. 
>> Hence this PR.
>
> LGTM. I spotted one fix in a exception message. I don't expect that there 
> will be tests depending on that, but it might still be a good idea to run the 
> serviceability tests to double check. Although I guess the test would have 
> had the same typo and would have been fixed too were it the case :-)

@dfuch I have only updated files in `src`, so if the incorrect spelling is 
tested, that test will now fail. I'm unfortunately not well versed in what 
tests cover serviceability code. Can you suggest a suitable set of tests to run?

And yes, ideally the tests should be spell checked as well. It's just that:
1) the product source is (imho) more important to begin with,
2) test comments are generally of a lower quality and more likely to contain 
more typos (imho), meaning even more work for me to publish a PR i believe is 
correct, and
3) the tests in the JDK are so intertwined and messy that I'm having a hard 
time understanding what groups to post reviews to. I could make one mega-PR 
touching the entire test code base, but I doubt it would be very popular. :-)

-

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

RFR: 8285366: Fix typos in serviceability

[Magnus Ihse Bursie]

I ran `codespell` on modules owned by the serviceability team (`java.instrument 
java.management.rmi java.management jdk.attach jdk.hotspot.agent 
jdk.internal.jvmstat jdk.jcmd jdk.jconsole jdk.jdi jdk.jdwp.agent jdk.jstatd 
jdk.management.agent jdk.management`), and accepted those changes where it 
indeed discovered real typos.


I will update copyright years using a script before pushing (otherwise like 
every second change would be a copyright update, making reviewing much harder).

The long term goal here is to make tooling support for running `codespell`. The 
trouble with automating this is of course all false positives. But before even 
trying to solve that issue, all true positives must be fixed. Hence this PR.

-

Commit messages:
 - Pass #2
 - Pass #1

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

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

Integrated: 8284903: Fix typos in hotspot

[Magnus Ihse Bursie]

On Fri, 15 Apr 2022 07:40:04 GMT, Magnus Ihse Bursie  wrote:

> I ran `codespell` on hotspot, and accepted those changes where it indeed 
> discovered real typos. 
> 
> You'd be surprised over the many implementions of instrinsics and other 
> intructions accross all archtectures I've encounted, so for the preceding 
> reason it's neccesery to sucessfully seach for exisiting typos...

This pull request has now been integrated.

Changeset: 4594696f
Author:Magnus Ihse Bursie 
URL:   
https://git.openjdk.java.net/jdk/commit/4594696f5462995ec58ca1d2c1bde7cc857c5caf
Stats: 1197 lines in 446 files changed: 0 ins; 1 del; 1196 mod

8284903: Fix typos in hotspot

Reviewed-by: cjplummer, coleenp, kvn, lucy, stefank

-

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

Re: RFR: 8284903: Fix typos in hotspot [v3]

[Magnus Ihse Bursie]

> I ran `codespell` on hotspot, and accepted those changes where it indeed 
> discovered real typos. 
> 
> You'd be surprised over the many implementions of instrinsics and other 
> intructions accross all archtectures I've encounted, so for the preceding 
> reason it's neccesery to sucessfully seach for exisiting typos...

Magnus Ihse Bursie has updated the pull request incrementally with one 
additional commit since the last revision:

  Update copyright headers

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/8260/files
  - new: https://git.openjdk.java.net/jdk/pull/8260/files/28bb365f..4fc36d6b

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

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

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

Re: RFR: 8284903: Fix typos in hotspot [v2]

[Magnus Ihse Bursie]

> I ran `codespell` on hotspot, and accepted those changes where it indeed 
> discovered real typos. 
> 
> You'd be surprised over the many implementions of instrinsics and other 
> intructions accross all archtectures I've encounted, so for the preceding 
> reason it's neccesery to sucessfully seach for exisiting typos...

Magnus Ihse Bursie has updated the pull request incrementally with one 
additional commit since the last revision:

  Fix some places according to code reviews

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/8260/files
  - new: https://git.openjdk.java.net/jdk/pull/8260/files/449d8a23..28bb365f

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

  Stats: 9 lines in 8 files changed: 0 ins; 1 del; 8 mod
  Patch: https://git.openjdk.java.net/jdk/pull/8260.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/8260/head:pull/8260

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

Re: RFR: 8284903: Fix typos in hotspot

[Magnus Ihse Bursie]

On Fri, 15 Apr 2022 07:40:04 GMT, Magnus Ihse Bursie  wrote:

> I ran `codespell` on hotspot, and accepted those changes where it indeed 
> discovered real typos. 
> 
> You'd be surprised over the many implementions of instrinsics and other 
> intructions accross all archtectures I've encounted, so for the preceding 
> reason it's neccesery to sucessfully seach for exisiting typos...

Thank you for all your input! I will update the PR according to all 
suggestions. But let me answer a few more general comments first.

The spelling corrections in this PR was generated by codespell. I manually 
removed all false positives (that was the most tedious part of this fix). 
Codespell works by checking a list of commonly misspelled words, not by 
verifying that all words are present in a list of accepted spellings. The 
latter approach might work for normal prose, but is utterly worthless for 
source code. In practice, codespell does automatically what several users have 
been doing manually lately -- discover a common misspelling, searching the code 
base for more instances of that misspelling. So, there are certainly lots of 
typos left. But this should reasonably cut down on the number of PRs of the 
type "Fix typos for supercalifragilisticexpialidocious" that updates a lot of 
files for just a single misspelled word.

For some cases where I were not sure (like mismatch or re-enable), I googled, 
read spelling recommendations and/or checked how common alternative spellings 
where. When the ratio exceeded 10:1 or so, I deemed one alternative as 
"correct"; in less common cases I let the original authors choice of spelling 
remain.

I have not read the complete sentences, just verified that the actual word 
fixes seemed to make sense. Hence the odd results in some places. 

Ideally, I would like to find some way of keeping the code "codespell clean" 
using tooling. Due to the amount of false positives, this is not a trivial 
task. I don't think we'll ever be able to have it as part of jcheck, but we 
might have a script that someone runs once per release or so. I'm not yet sure 
how to achieve this, but my two main ideas is to either just run codespell on 
added or modified code (think "+" lines in git diff), or to have a list of 
acknowledged false positives that should be skipped.

Before I can even start on this, the huge number of *true* positives needs to 
be addressed, though! Hotspot is by no means worst, it's just a huge chunk of 
code which I addressed all at once.

-

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

RFR: 8284903: Fix typos in hotspot

[Magnus Ihse Bursie]

I ran `codespell` on hotspot, and accepted those changes where it indeed 
discovered real typos. 

You'd be surprised over the many implementions of instrinsics and other 
intructions accross all archtectures I've encounted, so for the preceding 
reason it's neccesery to sucessfully seach for exisiting typos...

-

Commit messages:
 - Pass #2
 - Pass #1 in shared code
 - Pass #1 in platform-specific code

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

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

Integrated: 8284891: Fix typos in build system files

[Magnus Ihse Bursie]

On Thu, 14 Apr 2022 16:05:48 GMT, Magnus Ihse Bursie  wrote:

> I ran `codespell` on the `make` directory, and accepted those changes where 
> it indeed discovered real typos.
> 
> (Due to false positives this can unfortunately not be run automatically) 
> 
> Most of the fixes are in comments. A few are in messages aimed at the user.

This pull request has now been integrated.

Changeset: 160eb2bd
Author:Magnus Ihse Bursie 
URL:   
https://git.openjdk.java.net/jdk/commit/160eb2bd392fea29dd690ee9781174d14dc0b659
Stats: 72 lines in 46 files changed: 0 ins; 0 del; 72 mod

8284891: Fix typos in build system files

Reviewed-by: erikj

-

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

RFR: 8284891: Fix typos in build system files

[Magnus Ihse Bursie]

I ran `codespell` on the `make` directory, and accepted those changes where it 
indeed discovered real typos.

(Due to false positives this can unfortunately not be run automatically) 

Most of the fixes are in comments. A few are in messages aimed at the user.

-

Commit messages:
 - 8284891: Fix typos in build system files

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

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

Re: RFR: 8284853: Fix various 'expected' typo [v2]

[Magnus Ihse Bursie]

On Thu, 14 Apr 2022 09:28:17 GMT, Andrey Turbanov  wrote:

>> Found various typos of expected: `exepected`, `exept`, `epectedly`, 
>> `expeced`, `Unexpeted`, etc.
>
> Andrey Turbanov has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   8284853: Fix various 'expected' typo
>   improve test log

Build changes look good.

-

Marked as reviewed by ihse (Reviewer).

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

Re: RFR: 8284161: Implementation of Virtual Threads (Preview)

[Magnus Ihse Bursie]

On Fri, 8 Apr 2022 13:43:39 GMT, Alan Bateman  wrote:

> This is the implementation of JEP 425: Virtual Threads (Preview); TBD which 
> JDK version to target.
> 
> We will refresh this PR periodically to pick up changes and fixes from the 
> loom repo.
> 
> Most of the new mechanisms in the HotSpot VM are disabled by default and 
> require running with `--enable-preview` to enable.
> 
> The patch has support for x64 and aarch64 on the usual operating systems 
> (Linux, macOS, and Windows). There are stubs (calling Unimplemented) for zero 
> and some of the other ports. Additional ports can be contributed via PRs 
> against the fibers branch in the loom repo.
> 
> There are changes in many areas. To reduce notifications/mails, the labels 
> have been trimmed down for now to hotspot, serviceability and core-libs. 
> We'll add the complete set of labels when the PR is further along.
> 
> The changes include a refresh of java.util.concurrent and ForkJoinPool from 
> Doug Lea's CVS. These changes will probably be proposed and integrated in 
> advance of this PR.
> 
> The changes include some non-exposed and low-level infrastructure to support 
> the (in draft) JEPs for Structured Concurrency and Scope Locals. This is to 
> make life a bit easier and avoid having to separate VM changes and juggle 
> branches at this time.

src/java.base/share/classes/jdk/internal/misc/StructureViolationExceptions.java 
line 81:

> 79: Constructor ctor;
> 80: try {
> 81: Class exClass = 
> Class.forName("jdk.incubator.concurrent.StructureViolationException");

Should this not be `jdk.internal.misc`? (Also, if this is the case, maybe 
there's a test missing that could have discovered this...)

-

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

Re: RFR: 8276799: Implementation of JEP 422: Linux/RISC-V Port

[Magnus Ihse Bursie]

On Mon, 8 Nov 2021 11:17:47 GMT, Fei Yang  wrote:

> This PR implements JEP 422: Linux/RISC-V Port [1].
> The PR starts as a squashed merge of the 
> https://openjdk.java.net/projects/riscv-port branch.
> 
> This has been tested with jtreg tier{1,2,3,4} and jcstress on HiFive 
> Unmatched board. Dacapo, SPECjbb2015 and SPECjvm2008 benchmark tests are also 
> carried out regularly. So it should be good enough to run most Java programs.
> 
> [1] https://openjdk.java.net/jeps/422

Build changes look good. I can't say anything about the rest of the code.

-

Marked as reviewed by ihse (Reviewer).

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

Re: RFR: 8282657: Code cleanup: removing double semicolons at the end of lines

[Magnus Ihse Bursie]

On Mon, 7 Mar 2022 16:40:15 GMT, Lance Andersen  wrote:

>> Hi
>> 
>> I have reviewed the code for removing double semicolons at the end of lines
>> 
>> all the best
>> matteo
>
> What problem are you having editing the PR header?   You should be able to do 
> so as the author of the PR

@LanceAndersen @kevinrushforth TheShermanTanker is not the author of this PR, 
he's just assisting the author by creating the JBS issue.

-

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

Re: RFR: 8282657: Code cleanup: removing double semicolons at the end of lines

[Magnus Ihse Bursie]

On Fri, 4 Mar 2022 17:17:12 GMT, Roger Riggs  wrote:

>> Hi
>> 
>> I have reviewed the code for removing double semicolons at the end of lines
>> 
>> all the best
>> matteo
>
> We usually request that these be be broken up by area to attract the 
> appropriate reviewers and avoid eye-strain.  The client modules are usually 
> separated out, as are hotspot.
> And corresponding tests.
> This kind of change is pretty low value for the code base and requires the 
> involvement of quite a few reviewers.
> If you had ask ahead of time, I would have suggested finding something with 
> more value.

@RogerRiggs Otoh, this change is really trivial. I have verified that all 
changes are replacing trailing ";;" in Java code. These are all clearly typos. 
I think it's nice that we try to strive for a high quality, and while you are 
correct this is maybe not the most pressing issue, I think it's nice to get a 
cleanup like this done. 

I'd argue that this is a trivial change. If you are worried, let's get a couple 
of more reviewers. I can't see the need to split this up per area.

-

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

Re: RFR: JDK-8278273: Remove unnecessary exclusion of doclint accessibility checks

[Magnus Ihse Bursie]

On Sun, 5 Dec 2021 23:45:32 GMT, Joe Darcy  wrote:

> Exploratory builds indicate it is not currently necessary to exclude the 
> doclint accessibility checks; this patch enables them.
> 
> (Enabling the reference checks is left for future work.)



-

Marked as reviewed by ihse (Reviewer).

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

Integrated: 8276628: Use blessed modifier order in serviceability code

[Magnus Ihse Bursie]

On Thu, 4 Nov 2021 10:44:41 GMT, Magnus Ihse Bursie  wrote:

> I ran bin/blessed-modifier-order.sh on source owned by serviceability. This 
> scripts verifies that modifiers are in the "blessed" order, and fixes it 
> otherwise. I have manually checked the changes made by the script to make 
> sure they are sound.

This pull request has now been integrated.

Changeset: 7023b3f8
Author:Magnus Ihse Bursie 
URL:   
https://git.openjdk.java.net/jdk/commit/7023b3f8a120dda97bc27fdf130c05c1bd7a730b
Stats: 284 lines in 89 files changed: 0 ins; 0 del; 284 mod

8276628: Use blessed modifier order in serviceability code

Reviewed-by: dholmes, lmesnik, cjplummer

-

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

Re: RFR: 8276628: Use blessed modifier order in serviceability code [v2]

[Magnus Ihse Bursie]

> I ran bin/blessed-modifier-order.sh on source owned by serviceability. This 
> scripts verifies that modifiers are in the "blessed" order, and fixes it 
> otherwise. I have manually checked the changes made by the script to make 
> sure they are sound.

Magnus Ihse Bursie has updated the pull request incrementally with one 
additional commit since the last revision:

  Also update copyright year

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/6249/files
  - new: https://git.openjdk.java.net/jdk/pull/6249/files/cf5db105..8a5ff8a5

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

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

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

Re: RFR: 8276628: Use blessed modifier order in serviceability code

[Magnus Ihse Bursie]

On Thu, 4 Nov 2021 15:59:48 GMT, Chris Plummer  wrote:

>> I ran bin/blessed-modifier-order.sh on source owned by serviceability. This 
>> scripts verifies that modifiers are in the "blessed" order, and fixes it 
>> otherwise. I have manually checked the changes made by the script to make 
>> sure they are sound.
>
> Copyright updates?

@plummercj That's really kind of an edge case, considering the triviality of 
these changes. But yes, the norm in the JDK is to update the copyright year for 
all changes, so you're right, I should do that.

(And we *really* ought to wrangle free some resources to write tooling for us 
to do all the copyright dances...)

-

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

RFR: 8276628: Use blessed modifier order in serviceability code

[Magnus Ihse Bursie]

I ran bin/blessed-modifier-order.sh on source owned by serviceability. This 
scripts verifies that modifiers are in the "blessed" order, and fixes it 
otherwise. I have manually checked the changes made by the script to make sure 
they are sound.

-

Commit messages:
 - 8276628: Use blessed modifier order in serviceability code

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

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

kludgeMantis is no longer needed?

[Magnus Ihse Bursie]

I noticed the following piece of code in 
src/jdk.internal.jvmstat/share/classes/sun/jvmstat/perfdata/monitor/v1_0/PerfDataBuffer.java:


if(jvm_name.stringValue().indexOf("HotSpot") >= 0) {
if(jvm_version.stringValue().startsWith("1.4.2")) {
kludgeMantis(map, args);
}
}

It looks like there's some room for cleaning out old cruft here...

/Magnus

Re: Overriding the locale for JDB

[Magnus Ihse Bursie]

On 2021-09-02 07:01, Jakob Cornell wrote:

Hi all,

While working on changes for JBS 8271356 I spent some time testing on 
locales other than en-US and found something a bit odd.  I can 
override the system default locale for JDB by setting the `LANG' 
environment variable (on Linux), and I can override the system locale 
for the debuggee by using `-Duser.language', but unless handling of 
`LANG' is implemented cross-platform in OpenJDK there doesn't seem to 
be a portable way of overriding the locale for JDB itself.


The reason I bring this up is that I'm adding jtreg tests for my 
changes, and I'm trying to have them not depend on the system default 
locale (even though it seems the existing tests do). Wondering if I'm 
missing any better way to set the locale, in particular one that can 
be integrated into the `JdbTest' system. Looks like maybe all that's 
missing is a way to set system properties in JDB's VM, thus enabling 
use of `user.language'.


You can use -J to send options to the JVM, as in -J-Duser.language.

/Magnus



Jakob

Re: RFR: 8253795: Implementation of JEP 391: macOS/AArch64 Port [v29]

[Magnus Ihse Bursie]

On Mon, 22 Mar 2021 12:50:14 GMT, Anton Kozlov  wrote:

>> Please review the implementation of JEP 391: macOS/AArch64 Port.
>> 
>> It's heavily based on existing ports to linux/aarch64, macos/x86_64, and 
>> windows/aarch64. 
>> 
>> Major changes are in:
>> * src/hotspot/cpu/aarch64: support of the new calling convention (subtasks 
>> JDK-8253817, JDK-8253818)
>> * src/hotspot/os_cpu/bsd_aarch64: copy of os_cpu/linux_aarch64 with 
>> necessary adjustments (JDK-8253819)
>> * src/hotspot/share, test/hotspot/gtest: support of write-xor-execute (W^X), 
>> required on macOS/AArch64 platform. It's implemented with 
>> pthread_jit_write_protect_np provided by Apple. The W^X mode is local to a 
>> thread, so W^X mode change relates to the java thread state change (for java 
>> threads). In most cases, JVM executes in write-only mode, except when 
>> calling a generated stub like SafeFetch, which requires a temporary switch 
>> to execute-only mode. The same execute-only mode is enabled when a java 
>> thread executes in java or native states. This approach of managing W^X mode 
>> turned out to be simple and efficient enough.
>> * src/jdk.hotspot.agent: serviceability agent implementation (JDK-8254941)
>
> Anton Kozlov has updated the pull request with a new target base due to a 
> merge or a rebase. The pull request now contains 115 commits:
> 
>  - Merge branch 'master' into jdk-macos
>  - JDK-8262491: bsd_aarch64 part
>  - JDK-8263002: bsd_aarch64 part
>  - Merge remote-tracking branch 'upstream/jdk/master' into jdk-macos
>  - Wider #ifdef block
>  - Fix most of issues in java/foreign/ tests
>
>Failures related to va_args are tracked in JDK-8263512.
>  - Add Azul copyright
>  - Update Oracle copyright years
>  - Use Thread::current_or_null_safe in SafeFetch
>  - 8262903: [macos_aarch64] Thread::current() called on detached thread
>  - ... and 105 more: 
> https://git.openjdk.java.net/jdk/compare/a9d2267f...5add9269

Build changes still look good. Hope you can get this done now! :)

-

Marked as reviewed by ihse (Reviewer).

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

Re: RFR: 8253795: Implementation of JEP 391: macOS/AArch64 Port [v10]

[Magnus Ihse Bursie]

On Fri, 5 Feb 2021 09:49:11 GMT, Andrew Haley  wrote:

>> I reviewed bsd_aarch64.cpp digging bit deeper and left some comments.
>
>> > Umm, so how does patching work? We don't even know if other threads are 
>> > executing the code we need to patch.
>> 
>> I thought java can handle that scenario in usual (non W^X systems) just 
>> fine, so we just believe jvm did everything right and it's safe to rewrite 
>> some code at specific moment.
> 
> Got it, OK.

> This doesn't seem to be an issue anymore, After P.Race have finished with 
> JDK-8257852, Macarm port can be build without extra ld flags.

@VladimirKempik I agree. That concludes the build issues with this PR. So from 
a build perspective, this is now good to go.

-

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

Re: RFR: 8253795: Implementation of JEP 391: macOS/AArch64 Port [v10]

[Magnus Ihse Bursie]

On Wed, 3 Feb 2021 20:01:15 GMT, Anton Kozlov  wrote:

>> Please review the implementation of JEP 391: macOS/AArch64 Port.
>> 
>> It's heavily based on existing ports to linux/aarch64, macos/x86_64, and 
>> windows/aarch64. 
>> 
>> Major changes are in:
>> * src/hotspot/cpu/aarch64: support of the new calling convention (subtasks 
>> JDK-8253817, JDK-8253818)
>> * src/hotspot/os_cpu/bsd_aarch64: copy of os_cpu/linux_aarch64 with 
>> necessary adjustments (JDK-8253819)
>> * src/hotspot/share, test/hotspot/gtest: support of write-xor-execute (W^X), 
>> required on macOS/AArch64 platform. It's implemented with 
>> pthread_jit_write_protect_np provided by Apple. The W^X mode is local to a 
>> thread, so W^X mode change relates to the java thread state change (for java 
>> threads). In most cases, JVM executes in write-only mode, except when 
>> calling a generated stub like SafeFetch, which requires a temporary switch 
>> to execute-only mode. The same execute-only mode is enabled when a java 
>> thread executes in java or native states. This approach of managing W^X mode 
>> turned out to be simple and efficient enough.
>> * src/jdk.hotspot.agent: serviceability agent implementation (JDK-8254941)
>
> Anton Kozlov has updated the pull request incrementally with six additional 
> commits since the last revision:
> 
>  - Merge remote-tracking branch 'origin/jdk/jdk-macos' into jdk-macos
>  - Add comments to WX transitions
>
>+ minor change of placements
>  - Use macro conditionals instead of empty functions
>  - Add W^X to tests
>  - Do not require known W^X state
>  - Revert w^x in gtests

Marked as reviewed by ihse (Reviewer).

-

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

Re: RFR: 8253795: Implementation of JEP 391: macOS/AArch64 Port [v9]

[Magnus Ihse Bursie]

On Tue, 2 Feb 2021 21:20:52 GMT, Daniel D. Daugherty  wrote:

>> Anton Kozlov has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   support macos_aarch64 in hsdis
>
> make/autoconf/flags.m4 line 140:
> 
>> 138: else
>> 139:   MACOSX_VERSION_MIN=10.12.0
>> 140: fi
> 
> Not something that needs to be addressed here, but these changes
> illustrate that our collective use of macOSX/MACOSX/MacOSX names
> are tied to the fact that the macOS major version number was at 10
> for a very long time.
> 
> @magicus - Do we have an RFE to rename MACOSX or are we sticking
> with it and evolving our interpretation of the 'X' from '10' to 
> */splat/asterik?

@dcubed-ojdk There is no RFE to renaming "macosx" to "macos". I'm not sure it 
should be done. We can't follow all marketing trends (Apple recently renamed 
iOS to iPadOS for the iPad; we can't keep adapting to such schemes). 
Personally, I like the new name without the "x", but we had already spent some 
time trying to find and fix all (or at least, most) instances of "osx" in the 
code, that I don't really think it's worth the effort. 

If you can drill up enough enthusiasm for such a project, and get any 
objections down to minimum, I can help implementing it. But I won't be 
spearheading it.

> make/common/NativeCompilation.gmk line 1178:
> 
>> 1176: endif
>> 1177: # This only works if the openjdk_codesign identity is 
>> present on the system. Let
>> 1178: # silently fail otherwise.
> 
> Might want to add a comment here:
> #  The '-f' option will replace an existing signature if one exists.

We're not really in the habit of adding comments for various command line 
options. Normally, you can check these with "man" if you are uncertain. If they 
do something surprising, sure, but here it's more of a "it's needed on aarch64 
to work at all", so I don't think a comment will be anything but added clutter.

-

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

RFR: 8261149: Initial nroff manpage update for JDK 17

[Magnus Ihse Bursie]

We need to regenerate the exported nroff man pages to include the proper 
version string. This will also bring in some recent textual updates to the man 
pages.

-

Commit messages:
 - 8261149: Initial nroff manpage update for JDK 17

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

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

Re: RFR: 8257988: Remove JNF dependency from libsaproc/MacosxDebuggerLocal.m

[Magnus Ihse Bursie]

On 2021-02-02 21:47, Chris Plummer wrote:

Is there a reason why `java.desktop` continues to use `JNFJavaToNSString`? I 
was looking to see how this was handled in other places, but I couldn't find an 
example where `JNFJavaToNSString` was converted to call a newly implemented 
`JavaStringToNSString`.

That conversion is handled in https://github.com/openjdk/jdk/pull/2305.

/Magnus

Re: RFR: 8253795: Implementation of JEP 391: macOS/AArch64 Port [v6]

[Magnus Ihse Bursie]

On Mon, 1 Feb 2021 12:35:05 GMT, Alan Hayward 
 wrote:

> > Hello, hsdis is a separate out-of-tree project and is not part of this jep.
> 
> Unless there's something I'm missing it only requires a few lines of change 
> to src/utils/hsdis/makefile (it already has support for macos x86_64)

I agree with Alan that it makes sense to add this trivial change as part of 
this PR, if it allows the current hsdis solution to continue working on 
mac/aarch64.

> 
> > support for looking for proper hsdis dylib library was added as part of 
> > this jep.
> 
> I'm a little confused. Are you planning on adding a new disassembler?

@a74nh I think Vladimir is referring to 
https://github.com/openjdk/jdk/pull/392. The hsdis "component" has been left 
behind for a long time, and there are several requests to add new backends, 
which require a modernized build of hsdis. This is undfortunately not a 
high-priority project, and has been postponed several times already. :(

-

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

[jdk16] RFR: 8258378: Final nroff manpage update for JDK 16

[Magnus Ihse Bursie]

Before RC phase we need to ensure we have the final set of manpage updates 
published in OpenJDK.

-

Commit messages:
 - 8258378: Final nroff manpage update for JDK 16

Changes: https://git.openjdk.java.net/jdk16/pull/142/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk16=142=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8258378
  Stats: 238 lines in 27 files changed: 98 ins; 111 del; 29 mod
  Patch: https://git.openjdk.java.net/jdk16/pull/142.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk16 pull/142/head:pull/142

PR: https://git.openjdk.java.net/jdk16/pull/142

Re: RFR: 8257988: Remove JNF dependency from libsaproc/MacosxDebuggerLocal.m

[Magnus Ihse Bursie]

On Thu, 28 Jan 2021 22:40:57 GMT, Phil Race  wrote:

> This removes the JNF dependency from the jdk.hotspot.agent module.
> The macro expansions are the same as already used in the Java desktop module 
> - which actually uses a macro
> still but there there are hundreds of uses.
> The function of this macro code is to prevent NSExceptions escaping to Java 
> and also to drain the auto-release pool.
> What test group would be good for verifying this change ?

Build changes look good.

-

Marked as reviewed by ihse (Reviewer).

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

Re: RFR: 8253795: Implementation of JEP 391: macOS/AArch64 Port [v6]

[Magnus Ihse Bursie]

On Tue, 26 Jan 2021 21:59:03 GMT, Anton Kozlov  wrote:

>> Please review the implementation of JEP 391: macOS/AArch64 Port.
>> 
>> It's heavily based on existing ports to linux/aarch64, macos/x86_64, and 
>> windows/aarch64. 
>> 
>> Major changes are in:
>> * src/hotspot/cpu/aarch64: support of the new calling convention (subtasks 
>> JDK-8253817, JDK-8253818)
>> * src/hotspot/os_cpu/bsd_aarch64: copy of os_cpu/linux_aarch64 with 
>> necessary adjustments (JDK-8253819)
>> * src/hotspot/share, test/hotspot/gtest: support of write-xor-execute (W^X), 
>> required on macOS/AArch64 platform. It's implemented with 
>> pthread_jit_write_protect_np provided by Apple. The W^X mode is local to a 
>> thread, so W^X mode change relates to the java thread state change (for java 
>> threads). In most cases, JVM executes in write-only mode, except when 
>> calling a generated stub like SafeFetch, which requires a temporary switch 
>> to execute-only mode. The same execute-only mode is enabled when a java 
>> thread executes in java or native states. This approach of managing W^X mode 
>> turned out to be simple and efficient enough.
>> * src/jdk.hotspot.agent: serviceability agent implementation (JDK-8254941)
>
> Anton Kozlov has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Redo buildsys fix

Build changes per se now looks okay. However, I agree with Erik that unless 
this PR can wait for the JNF removal, at the very least the build docs needs to 
be updated to explain how to successfully build for this platform. (I can live 
with the configure command line hack, since it's temporary -- otherwise I'd 
have requested a new configure argument.) This can be done in this PR or a 
follow-up PR.

-

Marked as reviewed by ihse (Reviewer).

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

Re: RFR: 8253795: Implementation of JEP 391: macOS/AArch64 Port [v3]

[Magnus Ihse Bursie]

On Tue, 26 Jan 2021 22:04:48 GMT, Vladimir Kempik  wrote:

>> make/autoconf/build-aux/autoconf-config.guess line 1275:
>> 
>>> 1273: UNAME_PROCESSOR="aarch64"
>>> 1274: fi
>>> 1275:   fi ;;
>> 
>> Almost, but not quite, correct. We cannot change the autoconf-config.guess 
>> file due to license restrictions (the license allows redistribution, not 
>> modifications). Instead we have the config.guess file which "wraps" 
>> autoconf-config.guess and makes pre-/post-call modifications to work around 
>> limitations in the autoconf original file. So you need to check there if you 
>> are getting incorrect results back and adjust it in that case. See the 
>> already existing clauses in that file.
>
> Hello
> I have updated PR and moved this logic to make/autoconf/build-aux/config.guess
> It's pretty similar to i386 -> x86_64 fix-up on macos_intel

Thanks. That looks better.

-

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

Re: RFR: 8253795: Implementation of JEP 391: macOS/AArch64 Port [v6]

[Magnus Ihse Bursie]

On Mon, 25 Jan 2021 16:00:23 GMT, Bernhard Urban-Forster  
wrote:

>> @luhenry , could you please check this comment, I think SA-agent was MS's 
>> job here.
>
> The target is identified by the header file now:
> https://github.com/openjdk/jdk/pull/2200/files#diff-51442e74eeef2163f0f0643df0ae995083f666367e25fba2b527a9a5bc8743a6R35-R41
> 
> Do you think there is any problem with this approach?

@lewurm No, that's okay. I just wanted to know that this had not been lost.

-

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

Re: RFR: 8253795: Implementation of JEP 391: macOS/AArch64 Port [v3]

[Magnus Ihse Bursie]

On 2021-01-26 13:09, Vladimir Kempik wrote:

On Tue, 26 Jan 2021 12:02:02 GMT, Alan Hayward 
 wrote:


AIUI, the configure line needs passing a prebuilt JavaNativeFoundation framework
ie:
`--with-extra-ldflags='-F 
/Applications/Xcode.app/Contents/SharedFrameworks/ContentDeliveryServices.framework/Versions/A/itms/java/Frameworks/'`

Otherwise there will be missing _JNFNative* functions.

Is this the long term plan? Or will eventually the required code be moved into 
JDK and/or the xcode one automatically get picked up by the configure scripts?

There is ongoing work by P. Race to eliminate dependence on JNF at all
How far has that work come? Otherwise the logic should be added to 
configure to look for this framework automatically, and provide a way to 
override it/set it if not found.


I don't think it's OK to publish a new port that cannot build 
out-of-the-box without hacks like this.


/Magnus


-

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

Re: RFR: 8253795: Implementation of JEP 391: macOS/AArch64 Port [v3]

[Magnus Ihse Bursie]

On Mon, 25 Jan 2021 19:38:16 GMT, Anton Kozlov  wrote:

>> Please review the implementation of JEP 391: macOS/AArch64 Port.
>> 
>> It's heavily based on existing ports to linux/aarch64, macos/x86_64, and 
>> windows/aarch64. 
>> 
>> Major changes are in:
>> * src/hotspot/cpu/aarch64: support of the new calling convention (subtasks 
>> JDK-8253817, JDK-8253818)
>> * src/hotspot/os_cpu/bsd_aarch64: copy of os_cpu/linux_aarch64 with 
>> necessary adjustments (JDK-8253819)
>> * src/hotspot/share, test/hotspot/gtest: support of write-xor-execute (W^X), 
>> required on macOS/AArch64 platform. It's implemented with 
>> pthread_jit_write_protect_np provided by Apple. The W^X mode is local to a 
>> thread, so W^X mode change relates to the java thread state change (for java 
>> threads). In most cases, JVM executes in write-only mode, except when 
>> calling a generated stub like SafeFetch, which requires a temporary switch 
>> to execute-only mode. The same execute-only mode is enabled when a java 
>> thread executes in java or native states. This approach of managing W^X mode 
>> turned out to be simple and efficient enough.
>> * src/jdk.hotspot.agent: serviceability agent implementation (JDK-8254941)
>
> Anton Kozlov has updated the pull request incrementally with two additional 
> commits since the last revision:
> 
>  - Refactor CDS disabling
>  - Redo builsys support for aarch64-darwin

Changes requested by ihse (Reviewer).

make/autoconf/build-aux/autoconf-config.guess line 1275:

> 1273:   UNAME_PROCESSOR="aarch64"
> 1274:   fi
> 1275: fi ;;

Almost, but not quite, correct. We cannot change the autoconf-config.guess file 
due to license restrictions (the license allows redistribution, not 
modifications). Instead we have the config.guess file which "wraps" 
autoconf-config.guess and makes pre-/post-call modifications to work around 
limitations in the autoconf original file. So you need to check there if you 
are getting incorrect results back and adjust it in that case. See the already 
existing clauses in that file.

-

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

Re: RFR: 8253795: Implementation of JEP 391: macOS/AArch64 Port [v2]

[Magnus Ihse Bursie]

On Sun, 24 Jan 2021 15:32:59 GMT, Anton Kozlov  wrote:

>> Please review the implementation of JEP 391: macOS/AArch64 Port.
>> 
>> It's heavily based on existing ports to linux/aarch64, macos/x86_64, and 
>> windows/aarch64. 
>> 
>> Major changes are in:
>> * src/hotspot/cpu/aarch64: support of the new calling convention (subtasks 
>> JDK-8253817, JDK-8253818)
>> * src/hotspot/os_cpu/bsd_aarch64: copy of os_cpu/linux_aarch64 with 
>> necessary adjustments (JDK-8253819)
>> * src/hotspot/share, test/hotspot/gtest: support of write-xor-execute (W^X), 
>> required on macOS/AArch64 platform. It's implemented with 
>> pthread_jit_write_protect_np provided by Apple. The W^X mode is local to a 
>> thread, so W^X mode change relates to the java thread state change (for java 
>> threads). In most cases, JVM executes in write-only mode, except when 
>> calling a generated stub like SafeFetch, which requires a temporary switch 
>> to execute-only mode. The same execute-only mode is enabled when a java 
>> thread executes in java or native states. This approach of managing W^X mode 
>> turned out to be simple and efficient enough.
>> * src/jdk.hotspot.agent: serviceability agent implementation (JDK-8254941)
>
> Anton Kozlov has updated the pull request incrementally with two additional 
> commits since the last revision:
> 
>  - Address feedback for signature generators
>  - Enable -Wformat-nonliteral back

Changes requested by ihse (Reviewer).

make/autoconf/jvm-features.m4 line 269:

> 267: if test "x$OPENJDK_TARGET_OS" != xaix && \
> 268: !( test "x$OPENJDK_TARGET_OS" = "xmacosx" && \
> 269: test "x$OPENJDK_TARGET_CPU" = "xaarch64" ) ; then

This test is making my head spin. Can't you just invert it to this structure:

if OS=aix || OS+CPU = mac+aarch64; then
  no
else
 yes
fi

make/autoconf/platform.m4 line 75:

> 73: ;;
> 74:   esac
> 75:   ;;

This is solved in the wrong place. This code should just use the result from 
`config.guess/config.sub`. These files are imported from the autoconf project. 
Unfortunately, they have changed the license to one incompatible with OpenJDK, 
so we are stuck with an old version. Instead, we have created a "bugfix 
wrapper" that calls the original `autoconf-config.guess/sub` and fixes up the 
result, with stuff like this.

make/autoconf/platform.m4 line 273:

> 271:   # Convert the autoconf OS/CPU value to our own data, into the 
> VAR_OS/CPU/LIBC variables.
> 272:   PLATFORM_EXTRACT_VARS_FROM_OS($build_os)
> 273:   PLATFORM_EXTRACT_VARS_FROM_CPU($build_cpu, $build_os)

Fixing the comment above means this change, and the one below, can be reverted.

make/common/NativeCompilation.gmk line 1180:

> 1178: # silently fail otherwise.
> 1179: ifneq ($(CODESIGN), )
> 1180:   $(CODESIGN) -f -s "$(MACOSX_CODESIGN_IDENTITY)" 
> --timestamp --options runtime \

What does -f do, and is it needed for macos-x64 as well?

make/modules/jdk.hotspot.agent/Lib.gmk line 34:

> 32: 
> 33: else ifeq ($(call isTargetOs, macosx), true)
> 34:   SA_CFLAGS := -D_GNU_SOURCE -mno-omit-leaf-frame-pointer \

Is this really proper for macos-x64? I thought we used -Damd64 as a marker for 
all macos-x64 C/C++ files. (Most files get their flags from the common setup in 
configure, but SA is a different beast due to historical reasons).

-

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

Re: RFR: 8257733: Move module-specific data from make to respective module

[Magnus Ihse Bursie]

On 2021-01-15 19:27, mark.reinh...@oracle.com wrote:

Feature JEPs are living documents until such time as they are delivered.
In this case it would not be appropriate to update JEP 201, which is as
much about the transition from the old source-code layout as it is about
the new layout as of 2014.

At this point, and given that we’d already gone beyond JEP 201 prior to
this change (with `man` and `lib` subdirectories), what’d make the most
sense is a new informational JEP that documents the source-code layout.
Informational JEPs can, within reason, be updated over time.
So I take it I need to create a new informational JEP. :-) Fine, I can 
do that. I assume I mostly need to extract the parts of JEP 201 that 
describes the (then "new") layout, update wording to show that this is a 
description of the current layout, and add the new additions.


It'll be a very short JEP, but in this case, that's probably a virtue.

/Magnus

Re: RFR: 8253497: Core Libs Terminology Refresh [v2]

[Magnus Ihse Bursie]

On Tue, 15 Dec 2020 01:41:07 GMT, Joe Wang  wrote:

>> I agree that there is room for improvement here.  How about:
>> "...an allow-list too restrictively, or a reject-list too broadly, may..."
>> ?
>
> Your call, I'm not a native English speaker :-)  It felt to me it's 
> 'restrictive' than 'restrictively', an adj placed after the noun, e.g. a 
> restrictive allow-list.

It's an adverb, since it's the act of 'defining' that is being done too 
restrictively or broadly. If you want to have an adjective you would need to 
rephrase it so it applies to the noun, like 'defining a too restrictive 
accept-list'. My non-native English vibes would still prefer the former.

-

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

Re: RFR: 8257733: Move module-specific data from make to respective module

[Magnus Ihse Bursie]

On Fri, 4 Dec 2020 14:05:54 GMT, Erik Joelsson  wrote:

>> My understanding of JEPs is that they should be viewed as living documents. 
>> In this case, I think it's perfectly valid to update JEP 201 with additional 
>> source code layout. Both for this and for the other missing dirs.
>
> Regarding the chosen layout. Did you consider following the existing pattern 
> of src//{share,}/data?

@erikj79 Good point, that makes much more sense.

-

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

Re: RFR: 8257733: Move module-specific data from make to respective module

[Magnus Ihse Bursie]

On Fri, 4 Dec 2020 11:14:49 GMT, Alan Bateman  wrote:

>> To facilitate review, here is a list on how the different directories under 
>> make/data has moved.
>> 
>> **To java.base:**
>> * blacklistedcertsconverter
>> * cacerts
>> * characterdata
>> * charsetmapping
>> * cldr
>> * currency
>> * lsrdata
>> * publicsuffixlist
>> * tzdata
>> * unicodedata
>> 
>> **To java.desktop:**
>> * dtdbuilder
>> * fontconfig
>> * macosxicons
>> * x11wrappergen
>> 
>> **To jdk.compiler:**
>> * symbols
>> 
>> **To jdk.jdi:**
>> * jdwp
>> 
>> **Remaining in make:**
>> * bundle
>> * docs-resources
>> * macosxsigning
>> * mainmanifest
>
> Are you proposing any text or guidelines to be added to JEP 201 as part of 
> this?
> 
> I think the location of jdwp.spec and its location in the build tree may need 
> to be looked at again. It was convenient to have it in the make tree, from 
> which the protocol spec, and source code for the front end (module jdk.jdi) 
> and a header file for the back end (module jdk.jdwp.agent) are created. Given 
> that the JDWP protocol is standard (not JDK-specific) then there may be an 
> argument to put it in src/java.se instead of a JDK-specific module.

@AlanBateman Well, I don't know about updating JEP 201. Do you mean that `data` 
should be added to the list `classes`, `native`, `conf`, `legal` as presented 
under the heading "New scheme"? That list does not seem to have been kept up to 
date anyway. A quick glance also shows that we now have at least `man` and 
`lib` as well in this place. So either we say there's precedence for not 
updating the list, in which case I will do nothing. Or we should bring JEP 201 
up-to-date with current practices, which then of course should include `data` 
but also all other new directories that has been added since JEP 201 was 
written.

I really don't care either way, but my personal opinion is that JEP 201 
presented a view on how the plan was to re-organize things, given the knowledge 
and state of affairs at that time, but how we keep the source code organized 
and structured from there on, is a living, day-to-day engineering effort that 
is just hampered by having to update a formal document, that serves little 
purpose now that it has been implemented.

-

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

Re: RFR: 8257733: Move module-specific data from make to respective module

[Magnus Ihse Bursie]

On Fri, 4 Dec 2020 11:37:41 GMT, Magnus Ihse Bursie  wrote:

>> Are you proposing any text or guidelines to be added to JEP 201 as part of 
>> this?
>> 
>> I think the location of jdwp.spec and its location in the build tree may 
>> need to be looked at again. It was convenient to have it in the make tree, 
>> from which the protocol spec, and source code for the front end (module 
>> jdk.jdi) and a header file for the back end (module jdk.jdwp.agent) are 
>> created. Given that the JDWP protocol is standard (not JDK-specific) then 
>> there may be an argument to put it in src/java.se instead of a JDK-specific 
>> module.
>
> @AlanBateman Well, I don't know about updating JEP 201. Do you mean that 
> `data` should be added to the list `classes`, `native`, `conf`, `legal` as 
> presented under the heading "New scheme"? That list does not seem to have 
> been kept up to date anyway. A quick glance also shows that we now have at 
> least `man` and `lib` as well in this place. So either we say there's 
> precedence for not updating the list, in which case I will do nothing. Or we 
> should bring JEP 201 up-to-date with current practices, which then of course 
> should include `data` but also all other new directories that has been added 
> since JEP 201 was written.
> 
> I really don't care either way, but my personal opinion is that JEP 201 
> presented a view on how the plan was to re-organize things, given the 
> knowledge and state of affairs at that time, but how we keep the source code 
> organized and structured from there on, is a living, day-to-day engineering 
> effort that is just hampered by having to update a formal document, that 
> serves little purpose now that it has been implemented.

And I can certainly move jdwp.spec to java.base instead. That's the reason I 
need input on this: All I know is that is definitely not the responsibility of 
the Build Group to maintain that document, and I made my best guess at where to 
place it.

-

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

Re: RFR: 8257733: Move module-specific data from make to respective module

[Magnus Ihse Bursie]

On Thu, 3 Dec 2020 23:44:20 GMT, Magnus Ihse Bursie  wrote:

> A lot (but not all) of the data in make/data is tied to a specific module. 
> For instance, the publicsuffixlist is used by java.base, and fontconfig by 
> java.desktop. (A few directories, like mainmanifest, is *actually* used by 
> make for the whole build.) 
> 
> These data files should move to the module they belong to. The are, after 
> all, "source code" for that module that is "compiler" into resulting 
> deliverables, for that module. (But the "source code" language is not Java or 
> C, but typically a highly domain specific language or data format, and the 
> "compilation" is, often, a specialized transformation.) 
> 
> This misplacement of the data directory is most visible at code review time. 
> When such data is changed, most of the time build-dev (or the new build 
> label) is involved, even though this has nothing to do with the build. While 
> this is annoying, a worse problem is if the actual team that needs to review 
> the patch (i.e., the team owning the module) is missed in the review.

To facilitate review, here is a list on how the different directories under 
make/data has moved.

**To java.base:**
* blacklistedcertsconverter
* cacerts
* characterdata
* charsetmapping
* cldr
* currency
* lsrdata
* publicsuffixlist
* tzdata
* unicodedata

**To java.desktop:**
* dtdbuilder
* fontconfig
* macosxicons
* x11wrappergen

**To jdk.compiler:**
* symbols

**To jdk.jdi:**
* jdwp

**Remaining in make:**
* bundle
* docs-resources
* macosxsigning
* mainmanifest

-

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

RFR: 8257733: Move module-specific data from make to respective module

[Magnus Ihse Bursie]

A lot (but not all) of the data in make/data is tied to a specific module. For 
instance, the publicsuffixlist is used by java.base, and fontconfig by 
java.desktop. (A few directories, like mainmanifest, is *actually* used by make 
for the whole build.) 

These data files should move to the module they belong to. The are, after all, 
"source code" for that module that is "compiler" into resulting deliverables, 
for that module. (But the "source code" language is not Java or C, but 
typically a highly domain specific language or data format, and the 
"compilation" is, often, a specialized transformation.) 

This misplacement of the data directory is most visible at code review time. 
When such data is changed, most of the time build-dev (or the new build label) 
is involved, even though this has nothing to do with the build. While this is 
annoying, a worse problem is if the actual team that needs to review the patch 
(i.e., the team owning the module) is missed in the review.

-

Commit messages:
 - Update references in test
 - Step 2: Update references
 - First stage, move actual data files

Changes: https://git.openjdk.java.net/jdk/pull/1611/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk=1611=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8257733
  Stats: 78 lines in 1575 files changed: 3 ins; 1 del; 74 mod
  Patch: https://git.openjdk.java.net/jdk/pull/1611.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/1611/head:pull/1611

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

Re: RFR: 8255616: Disable AOT and Graal in Oracle OpenJDK

[Magnus Ihse Bursie]

On Fri, 30 Oct 2020 17:40:51 GMT, Vladimir Kozlov  wrote:

> We shipped Ahead-of-Time compilation (the jaotc tool) in JDK 9, as an 
> experimental feature. We shipped Graal as an experimental JIT compiler in JDK 
> 10. We haven't seen much use of these features, and the effort required to 
> support and enhance them is significant. We therefore intend to disable these 
> features in Oracle builds as of JDK 16. 
> 
> We'll leave the sources for these features in the repository, in case any one 
> else is interested in building them. But we will not update or test them.
> 
> We'll continue to build and ship JVMCI as an experimental feature in Oracle 
> builds.
> 
> Tested changes in all tiers.
> 
> I verified that with these changes I still able to build Graal in open repo 
> and run graalunit testing: 
> 
> `open$ bash test/hotspot/jtreg/compiler/graalunit/downloadLibs.sh 
> /mydir/graalunit_lib/`
> `open$ bash configure --with-debug-level=fastdebug 
> --with-graalunit-lib=/mydir/graalunit_lib/ --with-jtreg=/mydir/jtreg`
> `open$ make jdk-image`
> `open$ make test-image`
> `open$ make run-test TEST=compiler/graalunit/HotspotTest.java`

Build changes look good.

-

Marked as reviewed by ihse (Reviewer).

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

Re: RFR: 8212879: Make JVMTI TagMap table not hash on oop address

[Magnus Ihse Bursie]

On Fri, 30 Oct 2020 20:23:04 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.

Build changes look good. Not reviewed hotspot changes.

-

Marked as reviewed by ihse (Reviewer).

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

Re: RFR: 8248238: Implementation of JEP: Windows AArch64 Support [v11]

[Magnus Ihse Bursie]

On Mon, 28 Sep 2020 19:53:37 GMT, Monica Beckwith  wrote:

>> This is a continuation of 
>> https://mail.openjdk.java.net/pipermail/aarch64-port-dev/2020-August/009566.html
>>  
>> Changes since then:
>> * We've improved the write barrier as suggested by Andrew [1]
>> * The define-guards around R18 have been changed to `R18_RESERVED`. This 
>> will be enabled for Windows only for now but
>>   will be required for the upcoming macOS+Aarch64 [2] port as well.
>> * We've incorporated https://github.com/openjdk/jdk/pull/154 by @AntonKozlov 
>> in our PR for now and built the
>>   Windows-specific CPU feature detection on top of it.
>> 
>> [1] 
>> https://mail.openjdk.java.net/pipermail/aarch64-port-dev/2020-August/009597.html
>> [2] https://openjdk.java.net/jeps/8251280
>
> Monica Beckwith has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   r18 only on Linux

Build changes look good.

-

Marked as reviewed by ihse (Reviewer).

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

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

[Magnus Ihse Bursie]

On 2020-05-06 12:40, coleen.phillim...@oracle.com wrote:



On 5/6/20 2:09 AM, serguei.spit...@oracle.com wrote:

On 5/5/20 17:04, Mikael Vidstedt wrote:

On May 5, 2020, at 4:48 PM,serguei.spit...@oracle.com  wrote:

Hi Mikael,

The fixes in webrev look good to me.

I've just noticed a couple of more serviceability related things can be missed.
(Not sure if they are included into different chunk of fixes.)

One is libjvm_db.so which is for Solaris Pstack support:
   make/hotspot/lib/CompileDtraceLibraries.gmk:# Note that libjvm_db.c has 
tests for COMPILER2, but this was never set by
   make/hotspot/lib/CompileDtraceLibraries.gmk: LIBJVM_DB_OUTPUTDIR := 
$(JVM_VARIANT_OUTPUTDIR)/libjvm_db
   make/hotspot/lib/CompileDtraceLibraries.gmk:NAME := jvm_db, \
   make/hotspot/lib/CompileDtraceLibraries.gmk:SRC := 
$(TOPDIR)/src/java.base/solaris/native/libjvm_db, \

Another is DTrace support which also includes jhelper.d (support for DTrace 
jstack action which is for Solaris only):
   make/hotspot/gensrc/GensrcDtrace.gmk
   make/hotspot/lib/JvmDtraceObjects.gmk
It also impacts some other make files.

I believe you’ll find that these changes were included in the build system 
patch:

https://mail.openjdk.java.net/pipermail/build-dev/2020-May/027342.html
http://cr.openjdk.java.net/~mikael/webrevs/8244224/webrev.00/build/open/webrev/

Let me know if I missed something.


The file || make/hotspot/src/native/dtrace/generateJvmOffsets.cpp is 
for Solaris only, and so, can be removed.
It is for libjvm_db.so (provider for Solaris Pstack) and jhelper.d 
(provider for Solaris DTrace jstack action).
The jstack action (prints mixed java+native stack traces) was never 
implemented other than for Solaris.


I wonder if this can be used to implement the same thing on linux and 
if we can keep this?  Thoughts?
Is anyone seriously using dtrace to get stack traces? Or more cynically, 
is anyone seriouslly using dtrace with Java at all?


/Magnus


thanks,
Coleen


I do not see other problems but looked only at the Serviceability 
related files.


Thanks,
Serguei


Also, these lines can be just removed:
   open/src/jdk.jdwp.agent/unix/native/libjdwp/path_md.h:#ifndef 
_JAVASOFT_SOLARIS_PATH_MD_H_
   open/src/jdk.jdwp.agent/unix/native/libjdwp/path_md.h:#define 
_JAVASOFT_SOLARIS_PATH_MD_H_
   open/src/jdk.jdwp.agent/unix/native/libjdwp/path_md.h:#endif /* 
!_JAVASOFT_SOLARIS_PATH_MD_H_ */

Remove or rather rename? The corresponding Windows header also has the guard, 
any particular reason why it should be *removed*?

Cheers,
Mikael


Thanks,
Serguei


On 5/5/20 14:34, Mikael Vidstedt wrote:

All good points! I deliberately chose *not* to update comments where it wasn’t 
immediately 100% obvious exactly how to update them. For example, in many cases 
I found that the comments are already incomplete or stale, and for each such 
case we’ll want to consider how exactly to update the comment (remove/switch to 
Unix/etc.). I would prefer to handle this as follow-up(s), separate from the 
JEP. Does that sounds reasonable?

Apart from the comments - do the changes look good? If so, can I count you as a 
reviewer?

Cheers,
Mikael



On May 4, 2020, at 12:20 AM,serguei.spit...@oracle.com  wrote:

HI Mikael,

Some quick comments.

Some extra references to Solaris/solaris, SunOS or SPARC are listed below:

src/java.instrument/unix/native/libinstrument/FileSystemSupport_md.c (2 refs to 
Solaris/solaris)
src/java.management/share/classes/javax/management/loading/MLet.java (refs to 
Solaris, SPARC/sparc and SunOS)
src/jdk.management.agent/unix/classes/jdk/internal/agent/FileSystemImpl.java 
(ref to Solaris)

src/hotspot/share/prims/forte.cpp:// Solaris SPARC Compiler1 needs an 
additional check on the grandparent
src/hotspot/share/prims/forte.cpp:// on Linux X86, Solaris SPARC and Solaris 
X86.
src/hotspot/share/prims/jvmti.xml:On the Solaris Operating 
Environment, an agent library is a shared
src/hotspot/share/prims/jvmti.xml: LD_LIBRARY_PATH under the 
Solaris operating
src/hotspot/share/prims/jvmti.xml:  example, in the Java 2 SDK a CTRL-Break 
on Win32 and a CTRL-\ on Solaris
src/hotspot/share/prims/methodHandles.cpp:#undef CS  // Solaris builds complain


Thanks,
Serguei


On 5/3/20 22:12, 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/serviceability/open/webrev/
JEP:https://bugs.openjdk.java.net/browse/JDK-8241787


Note: When reviewing this, please be aware that this exercise was *extremely* 
mind-numbing, so I appreciate your help reviewing all the individual changes 
carefully. You may want to get that coffee cup filled up (or whatever keeps you 
awake)!


Background:

Because of the size of the total patch and wide range of areas touched, this 
patch is one out of in total six partial patches which together make up the 
necessary changes to remove 

Re: RFR: JDK-8242943 Fix all remaining unchecked warnings in jdk.hotspot.agent

[Magnus Ihse Bursie]

On 2020-04-20 21:19, Joe Darcy wrote:

Hello,

On 4/16/2020 5:47 AM, Magnus Ihse Bursie wrote:
[snip]
The tricky one here is the helper TableModelComparator. This one took 
me a while to wrap my head around. It implements Comparator, but the 
compare() method takes "rows" from the model, which are just 
expressed as Objects, and left to subclasses to define differently. 
For one of the subclasses it uses the type T that the TableModel is 
created around, but in other it uses an independent domain model. :-/ 
Anyway. The compare() method then extracts data for the individual 
columns of each row using the method getValueForColumn(), and compare 
them pairwise. This data from the rows are supposed to implement 
Comparable.


In the end, I think I got it pretty OK, but I'm still an apprentice 
when it comes to generics black magic. The subclasses of 
TableModelComparator want to return different objects from 
getValueForColumn() for different columns in the row, like Long or 
String. They are all Comparable, but String is Comparable and 
Long is Comparable. So I needed to specify the abstract method 
as returning Comparable, since Comparable is not 
Comparable.


But then, for reasons that I do not fully fathom, I could not specify

Comparable o1 = getValueForColumn(row1, columns[i]);
Comparable o2 = getValueForColumn(row2, columns[i]);
int result = o1.compareTo(o2);

because the compiler did not accept the call to compareTo().

I did try to sacrifice a black rooster at midnight and walking 
backwards in a circle three time, to no avail. Maybe the problem was 
that it was not full moon or that I have no cat. In the end, I casted 
o1 and o2 to Comparable and suppressed the warning for that 
cast.


If anyone knows what rituals I need to perform to make this work, or 
-- even better -- how to fix the code properly, please let me know!


A brief discussion of wildcards, ?. The meaning of a wildcard is some 
particular unknown type, meeting the various  constraints from "? 
extends" or "? super".  If there is no explicit constraint listed, it 
is equivalent to "? extends Object".


So the meaning of the code above is something

Comparable o1 = getValueForColumn(row1, columns[i]);
Comparable o2 = getValueForColumn(row2, columns[i]);

where S and T are two fresh, unrelated type variables. Concretely, 
this means S and T could be, say, String and Integer so that is why a 
call to o1.compareTo(o2) would not be accepted by the compiler since 
Strings and Integer aren't comparable to each other.


Ah, right, and compareTo -- in contrast with equals() -- only compares 
to objects of the same type. I think that's part of what confused me here.


While two instances of "Comparable" are syntactically the same, 
they don't represent the same type inside the compiler.


In some cases, you can get around this issue by using a separate 
method to capture the type variable and then use it more than once. A 
technique we've used in the JDK is to have a pubic method with a 
wildcards calling a private method using a type variable. I've looked 
around for a few minutes and didn't find a concrete example in the 
JDK, but they're in there.


I haven't looked over the specific code here in much detail; the type 
Comparable might to be the best you can do, but it isn't very 
expressive since Object's aren't necessarily comparable.

Thank you for taking time to explain this!

/Magnus


HTH,

-Joe

Re: RFR (T) 8242896: typo #ifdef INCLUDE_JVMTI in codeCache.cpp

[Magnus Ihse Bursie]

On 2020-04-16 04:37, coleen.phillim...@oracle.com wrote:



On 4/15/20 9:37 PM, David Holmes wrote:

Hi Coleen,

On 16/04/2020 10:59 am, coleen.phillim...@oracle.com wrote:
open webrev at 
http://cr.openjdk.java.net/~coleenp/2020/8242896.01/webrev

bug link https://bugs.openjdk.java.net/browse/JDK-8242896


Looks good but ...


Built and ran vmTestbase RedefineTests which use the protected code.


... you need to ensure that builds that don't INCLUDE_JVMTI still 
work okay as they will now actually be excluding this code for the 
first time.


Last time I tried to build minimal, it failed for some odd problem on 
my system.  I'll wait until someone from the openjdk can build it then.

thanks,
Building minimal should work.  Try something like this: "jib configure 
-- --with-jvm-variants=minimal". It should work.


I just tried with your patch, and it does not work.

/localhome/hg/jdk-BAR/open/src/hotspot/share/classfile/metadataOnStackMark.cpp:70: 
error: undefined reference to 'CodeCache::old_nmethods_do(MetadataClosure*)'
/localhome/hg/jdk-BAR/open/src/hotspot/share/code/nmethod.cpp:1495: 
error: undefined reference to 
'CodeCache::unregister_old_nmethod(CompiledMethod*)'


I made a quick check but it was not clear to me if the call sites in 
metadataOnStackMark.cpp and nmethod.cpp should be excluded if missing 
jvmti, or if the code in codeCache.cpp should really be present even 
with jvmti.


/Magnus



Coleen


Thanks,
David


thanks,
Coleen

RFR: JDK-8242943 Fix all remaining unchecked warnings in jdk.hotspot.agent

[Magnus Ihse Bursie]

This is the final part of removing all warnings from the build of 
jdk.hotspot.agent. This patch includes a number of non-trivial fixes for 
the few remaining unchecked warnings.


The good news is that with this fix (and after the recent removal of 
Nashorn and rmic), the JDK build is finally complete free from warnings 
for the first time ever! EPIC!1!!! :-)


The bad news is that this patch is probably the most complex of all I've 
posted so far. :-/


I'll break down the changes in four groups:

* ConstMethod superclass issue

ConstMethod current extends VMObject. However, it is treated as a 
Metadata (Metadata is a subtype of VMObject). For instance, this code 
appears in Metadata.java:

  metadataConstructor.addMapping("ConstMethod", ConstMethod.class);

I believe it is just an oversight that ConstMethod does not also extend 
Metadata like the rest of the classes added to the metadataConstructor 
mapping, one which has not been caught without the extra type safety 
given by generics.


* ProfileData casting

In several places, a ProfileData item is extracted from a collection, 
its type checked with instanceof, and then acted on accordingly. (Yeah, 
nice and clean OOP abstraction at work! :-))


This works well most of the time, but when casting to ReceiverTypeData 
or CallTypeDataInterface, the type include generic parametrisation, so 
it needs to be cast to e.g. ReceiverTypeData. 
Unfortunately, this cannot be verified using instanceof, due to generic 
type erasure, and thus the compiler considers this an unchecked cast. 
(At least, this is my understanding of what is happening.) As far as I 
can tell, this is legitimate code, and the only way to really handle 
this (barring a more extensive rewrite) is to disable the warning for 
these cases.


* Generification of the InstanceConstructor hierarchy

I have properly generified the InstanceConstructor hierarchy, with the 
VirtualConstructor, StaticBaseConstructor and VirtualBaseConstructor 
subclasses. (And also the related helper class VMObjectFactory.)


Most of it turned out OK (and with improved type safety), but there was 
one piece of code that is a bit unfortunate. In VirtualBaseConstructor, 
we have the following:

    @SuppressWarnings("unchecked")
    Class lookedUpClass = (ClassT>)Class.forName(packageName + "." + superType.getName());

    c = lookedUpClass;

That is, we send in a name for a class and assumes that it will resolve 
to a subtype of T, but there is no way to enforce this with type safety. 
I have verified all current callers to the constructor so that they all 
abide to this rule (of course, otherwise the code would not have worked, 
but I checked it anyway). The alternative to suppressing the warning is 
to redesign the entire API, so we do not send in a class name, but 
instead a Class, and use that to extract the name instead. 
At this point, I don't find it worth it. This is, after all, no worse 
than it was before.


* SortableTableModel and TableModelComparator

This one was quite messy. The tables are being used in quite different 
ways in different places, which made it hard to generify in a good way. 
A lot of it revolves around keeping the data as Objects and casting it 
to a known type. (Some of this behavior comes from the fact that they 
extend old Swing classes which does this.) I've tried to tighten it up 
as much as possible by introducing increased type safety by 
generification, but in the end, it was not fully possible to do without 
a major surgery of the entire class structure. As above, most of it 
turned out OK, but not all.


The tricky one here is the helper TableModelComparator. This one took me 
a while to wrap my head around. It implements Comparator, but the 
compare() method takes "rows" from the model, which are just expressed 
as Objects, and left to subclasses to define differently. For one of the 
subclasses it uses the type T that the TableModel is created around, but 
in other it uses an independent domain model. :-/ Anyway. The compare() 
method then extracts data for the individual columns of each row using 
the method getValueForColumn(), and compare them pairwise. This data 
from the rows are supposed to implement Comparable.


In the end, I think I got it pretty OK, but I'm still an apprentice when 
it comes to generics black magic. The subclasses of TableModelComparator 
want to return different objects from getValueForColumn() for different 
columns in the row, like Long or String. They are all Comparable, but 
String is Comparable and Long is Comparable. So I needed 
to specify the abstract method as returning Comparable, since 
Comparable is not Comparable.


But then, for reasons that I do not fully fathom, I could not specify

Comparable o1 = getValueForColumn(row1, columns[i]);
Comparable o2 = getValueForColumn(row2, columns[i]);
int result = o1.compareTo(o2);

because the compiler did not accept the call to compareTo().

I did try to sacrifice a 

Re: RFR: JDK-8242808 Fix all remaining deprecation warnings in jdk.hotspot.agent

[Magnus Ihse Bursie]

Here is an updated version, that avoids the SuppressWarnings for 
modelToView:


http://cr.openjdk.java.net/~ihse/JDK-8242808-fix-all-SA-deprecation/webrev.02

(Only change is in SourceCodePanel.java compared to v01)

/Magnus

On 2020-04-15 14:30, Magnus Ihse Bursie wrote:



On 2020-04-15 12:59, Prasanta Sadhukhan wrote:

Hi Magnus,

Why can't we just use modelToView2D() to get Rectangle2D and then 
cast to Rectangle tobe used in scrollRectToVisible() or else use 
(int)Rectangle2D.getX(), (int)getY(), getWidth(), getHeight() to 
construct a new Rectangle()?
Casting a Rectangle2D to a Rectangle does not seem to me to be an 
improvement. That presupposes even more knowledge about implementation 
details.


However, I'll look into constructing a Rectangle using the getters 
from the Rectangle2D, as you suggested. Thanks!


/Magnus


Regard
Prasanta
On 15-Apr-20 3:35 PM, Magnus Ihse Bursie wrote:

Hi swing-dev,

Do you have any other suggestions for how to resolve the deprecation 
of modelToView() in this code?


Basically, the code does this:

   Rectangle rect = source.modelToView(offset);
   source.scrollRectToVisible(rect);

but scrollRectToVisible() requires a Rectangle (a subtype of 
Rectangle2D), and modelToView() is deprecated with reference to 
modelToView2D(), which returns a Rectangle2D, which is thus unusable 
by scrollRectToVisible(). I also cannot find a way to create a new 
Rectangle, given a Rectangle2D.


In practice, under the hood, it's still just Rectangles (not 
Rectangle2D).


/Magnus

On 2020-04-15 11:37, David Holmes wrote:

Hi Magnus,

This one sounds like it needs a Swing/Java2D developer to review it :)

Cheers,
David

On 15/04/2020 7:13 pm, Magnus Ihse Bursie wrote:
After JDK-8242804, a few places remain which are using deprecated 
methods. They too should be fixed, and the deprecation warning 
should no longer be disabled.


This patch presupposes the fix for JDK-8242804 has been applied 
(otherwise we cannot turn the deprecation warning back on).


Some brief comments about each fix:

* In ConstantPool.java, there was a boxing deprecation that I 
missed in JDK-8242804 (sorry!)


* In HighPrecisionJScrollBar.java, there is a trivial replacement 
to use BigDecimal.divide with RoundingMode semantics.


* In SourceCodePanel.java, I settled for suppressing the warning. 
The issue here is that modelToView (which returns a Rectangle) is 
deprecated in favor of modelToView2D, which returns a Rectangle2D 
(which is a supertype of Rectangle). But we use the result in 
scrollRectToVisible, and there exist no version of that which 
accepts a Rectangle2D instead of a Rectangle, nor a way to created 
a Rectangle from a Rectangle2D (that I could find). In practice, 
this is just a game of types -- under the hood, modelToView2D 
still returns a Rectangle (even though it only promises a 
Rectangle2D). The alternative here would be to cast the result of 
modelToView2D to a Rectangle, but I found that less attractive.


* In JTreeTable.java, I've replaced the use of the old-style 
modifier mask with the new-style extended modifier mask. To the 
best of my understanding, this will just work the same for the 
code here (and for the MouseEvent constructor, using the extended 
mask is actually prescribed).


Bug: https://bugs.openjdk.java.net/browse/JDK-8242808
WebRev: 
http://cr.openjdk.java.net/~ihse/JDK-8242808-fix-all-SA-deprecation/webrev.01 

Re: RFR: JDK-8242808 Fix all remaining deprecation warnings in jdk.hotspot.agent

[Magnus Ihse Bursie]

On 2020-04-15 12:59, Prasanta Sadhukhan wrote:

Hi Magnus,

Why can't we just use modelToView2D() to get Rectangle2D and then cast 
to Rectangle tobe used in scrollRectToVisible() or else use 
(int)Rectangle2D.getX(), (int)getY(), getWidth(), getHeight() to 
construct a new Rectangle()?
Casting a Rectangle2D to a Rectangle does not seem to me to be an 
improvement. That presupposes even more knowledge about implementation 
details.


However, I'll look into constructing a Rectangle using the getters from 
the Rectangle2D, as you suggested. Thanks!


/Magnus


Regard
Prasanta
On 15-Apr-20 3:35 PM, Magnus Ihse Bursie wrote:

Hi swing-dev,

Do you have any other suggestions for how to resolve the deprecation 
of modelToView() in this code?


Basically, the code does this:

   Rectangle rect = source.modelToView(offset);
   source.scrollRectToVisible(rect);

but scrollRectToVisible() requires a Rectangle (a subtype of 
Rectangle2D), and modelToView() is deprecated with reference to 
modelToView2D(), which returns a Rectangle2D, which is thus unusable 
by scrollRectToVisible(). I also cannot find a way to create a new 
Rectangle, given a Rectangle2D.


In practice, under the hood, it's still just Rectangles (not 
Rectangle2D).


/Magnus

On 2020-04-15 11:37, David Holmes wrote:

Hi Magnus,

This one sounds like it needs a Swing/Java2D developer to review it :)

Cheers,
David

On 15/04/2020 7:13 pm, Magnus Ihse Bursie wrote:
After JDK-8242804, a few places remain which are using deprecated 
methods. They too should be fixed, and the deprecation warning 
should no longer be disabled.


This patch presupposes the fix for JDK-8242804 has been applied 
(otherwise we cannot turn the deprecation warning back on).


Some brief comments about each fix:

* In ConstantPool.java, there was a boxing deprecation that I 
missed in JDK-8242804 (sorry!)


* In HighPrecisionJScrollBar.java, there is a trivial replacement 
to use BigDecimal.divide with RoundingMode semantics.


* In SourceCodePanel.java, I settled for suppressing the warning. 
The issue here is that modelToView (which returns a Rectangle) is 
deprecated in favor of modelToView2D, which returns a Rectangle2D 
(which is a supertype of Rectangle). But we use the result in 
scrollRectToVisible, and there exist no version of that which 
accepts a Rectangle2D instead of a Rectangle, nor a way to created 
a Rectangle from a Rectangle2D (that I could find). In practice, 
this is just a game of types -- under the hood, modelToView2D still 
returns a Rectangle (even though it only promises a Rectangle2D). 
The alternative here would be to cast the result of modelToView2D 
to a Rectangle, but I found that less attractive.


* In JTreeTable.java, I've replaced the use of the old-style 
modifier mask with the new-style extended modifier mask. To the 
best of my understanding, this will just work the same for the code 
here (and for the MouseEvent constructor, using the extended mask 
is actually prescribed).


Bug: https://bugs.openjdk.java.net/browse/JDK-8242808
WebRev: 
http://cr.openjdk.java.net/~ihse/JDK-8242808-fix-all-SA-deprecation/webrev.01 

Re: RFR: JDK-8242808 Fix all remaining deprecation warnings in jdk.hotspot.agent

[Magnus Ihse Bursie]

Hi swing-dev,

Do you have any other suggestions for how to resolve the deprecation of 
modelToView() in this code?


Basically, the code does this:

   Rectangle rect = source.modelToView(offset);
   source.scrollRectToVisible(rect);

but scrollRectToVisible() requires a Rectangle (a subtype of 
Rectangle2D), and modelToView() is deprecated with reference to 
modelToView2D(), which returns a Rectangle2D, which is thus unusable by 
scrollRectToVisible(). I also cannot find a way to create a new 
Rectangle, given a Rectangle2D.


In practice, under the hood, it's still just Rectangles (not Rectangle2D).

/Magnus

On 2020-04-15 11:37, David Holmes wrote:

Hi Magnus,

This one sounds like it needs a Swing/Java2D developer to review it :)

Cheers,
David

On 15/04/2020 7:13 pm, Magnus Ihse Bursie wrote:
After JDK-8242804, a few places remain which are using deprecated 
methods. They too should be fixed, and the deprecation warning should 
no longer be disabled.


This patch presupposes the fix for JDK-8242804 has been applied 
(otherwise we cannot turn the deprecation warning back on).


Some brief comments about each fix:

* In ConstantPool.java, there was a boxing deprecation that I missed 
in JDK-8242804 (sorry!)


* In HighPrecisionJScrollBar.java, there is a trivial replacement to 
use BigDecimal.divide with RoundingMode semantics.


* In SourceCodePanel.java, I settled for suppressing the warning. The 
issue here is that modelToView (which returns a Rectangle) is 
deprecated in favor of modelToView2D, which returns a Rectangle2D 
(which is a supertype of Rectangle). But we use the result in 
scrollRectToVisible, and there exist no version of that which accepts 
a Rectangle2D instead of a Rectangle, nor a way to created a 
Rectangle from a Rectangle2D (that I could find). In practice, this 
is just a game of types -- under the hood, modelToView2D still 
returns a Rectangle (even though it only promises a Rectangle2D). The 
alternative here would be to cast the result of modelToView2D to a 
Rectangle, but I found that less attractive.


* In JTreeTable.java, I've replaced the use of the old-style modifier 
mask with the new-style extended modifier mask. To the best of my 
understanding, this will just work the same for the code here (and 
for the MouseEvent constructor, using the extended mask is actually 
prescribed).


Bug: https://bugs.openjdk.java.net/browse/JDK-8242808
WebRev: 
http://cr.openjdk.java.net/~ihse/JDK-8242808-fix-all-SA-deprecation/webrev.01 

RFR: JDK-8242808 Fix all remaining deprecation warnings in jdk.hotspot.agent

[Magnus Ihse Bursie]

After JDK-8242804, a few places remain which are using deprecated 
methods. They too should be fixed, and the deprecation warning should no 
longer be disabled.


This patch presupposes the fix for JDK-8242804 has been applied 
(otherwise we cannot turn the deprecation warning back on).


Some brief comments about each fix:

* In ConstantPool.java, there was a boxing deprecation that I missed in 
JDK-8242804 (sorry!)


* In HighPrecisionJScrollBar.java, there is a trivial replacement to use 
BigDecimal.divide with RoundingMode semantics.


* In SourceCodePanel.java, I settled for suppressing the warning. The 
issue here is that modelToView (which returns a Rectangle) is deprecated 
in favor of modelToView2D, which returns a Rectangle2D (which is a 
supertype of Rectangle). But we use the result in scrollRectToVisible, 
and there exist no version of that which accepts a Rectangle2D instead 
of a Rectangle, nor a way to created a Rectangle from a Rectangle2D 
(that I could find). In practice, this is just a game of types -- under 
the hood, modelToView2D still returns a Rectangle (even though it only 
promises a Rectangle2D). The alternative here would be to cast the 
result of modelToView2D to a Rectangle, but I found that less attractive.


* In JTreeTable.java, I've replaced the use of the old-style modifier 
mask with the new-style extended modifier mask. To the best of my 
understanding, this will just work the same for the code here (and for 
the MouseEvent constructor, using the extended mask is actually prescribed).


Bug: https://bugs.openjdk.java.net/browse/JDK-8242808
WebRev: 
http://cr.openjdk.java.net/~ihse/JDK-8242808-fix-all-SA-deprecation/webrev.01

RFR: JDK-8242804 Fix trivial deprecation issues in jdk.hotspot.agent

[Magnus Ihse Bursie]

In the quest for getting rid of warning messages in jdk.hotspot.agent, 
the time has now come for another major source of deprecation messages, 
that are trivial to fix and might hide more tricky issues.


This patch handles the "new Integer(42)" pattern of explicit boxing, 
which is deprecated. I have fixed this using the improved autoboxing 
functionality in modern Java (e.g. just "42"), wherever possible (which 
was most places). For some cases, a modern factory method was preferable.


This patch also fixes newInstance() deprecation.

As with the previous patches, it might be easier to just read the patch 
file, 
http://cr.openjdk.java.net/~ihse/JDK-8242804-SA-boxing-deprecations/webrev.01/open.patch. 
(Fortunately, there are a log less changes in this patch than the 
previous ones.)


Bug: https://bugs.openjdk.java.net/browse/JDK-8242804
WebRev: 
http://cr.openjdk.java.net/~ihse/JDK-8242804-SA-boxing-deprecations/webrev.01


/Magnus

Re: Ping: Re: RFR: JDK-8241618 Fix unchecked warning for jdk.hotspot.agent

[Magnus Ihse Bursie]

On 2020-04-15 02:34, serguei.spit...@oracle.com wrote:

Hi Magnus,

It looks good to me.

Thanks for the review, Serguei!

/Magnus


Thanks,
Serguei


On 4/14/20 14:23, Chris Plummer wrote:

Hi Magnus,

The changes look good. Just one minor issue:

http://cr.openjdk.java.net/~ihse/JDK-8241618-fix-unchecked-warnings-for-agent/webrev.02/src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/oops/Metadata.java.frames.html

Copyright was updated, but no other changes in this file.

BTW, do you know why there were cases in the old code of having the 
right generic type in a comment. For example:


1177 private static List/**/ 
getInstanceFields(InstanceKlass ik) {


and

1276 private Map classDataCache = new HashMap(); // 



It's almost as if some of the code was initially properly written for 
generics, but then backported to work with a pre-generics version of jdk.


thanks,

Chris

On 4/14/20 2:24 AM, Magnus Ihse Bursie wrote:

Hi,

Can I please get a review for this, simplified version of the patch? 
This only contain trivial changes, like this:

-  private Listobjects; // ArrayList
+  private List objects;
Basically all changes are to the container types List or Map (and a 
few changes from Class to Class).


If the list of all the files look horrible in the webrev, have a 
look at the patch file instead, it is easier to scroll through and 
check the changes:

http://cr.openjdk.java.net/~ihse/JDK-8241618-fix-unchecked-warnings-for-agent/webrev.02/open.patch

/Magnus

On 2020-03-30 16:25, Magnus Ihse Bursie wrote:



On 2020-03-25 20:52, Chris Plummer wrote:

Hi Magus,

I haven't looked at the changes yet, other to see that there are 
many files touched, but after reading below (and only partly 
understanding since I don't know this area well), I was wondering 
if this issue wouldn't be better served with multiple passes made 
to fix the warnings. Start with a straight forward one where you 
are maybe only making one or two types of changes, but that affect 
a large number of files and don't cascade into other more 
complicated changes. This will get a lot of the noise out of the 
way, and then we can focus on some of the harder issues you bring 
up below.
Ok, I did just this. Here is an updated webrev. It contain the bulk 
of the changes, but all changes are -- I dare not say trivially 
obvious, but at least no-brainers. Hopefully it should be easier to 
review so I can get this pushed and out of the way.


This also means that it is not possible to turn on the warning just 
yet.


http://cr.openjdk.java.net/~ihse/JDK-8241618-fix-unchecked-warnings-for-agent/webrev.02 



/Magnus


As for testing, I think the following list will capture all of 
them, but can't say for sure:


open/test/hotspot/jtreg/serviceability/sa
open/test/hotspot/jtreg/resourcehogs/serviceability/sa
open/test/jdk/sun/tools/jhsdb
open/test/jdk/sun/tools/jstack
open/test/jdk/sun/tools/jmap
open/test/hotspot/jtreg/gc/metaspace/CompressedClassSpaceSizeInJmapHeap.java 

open/test/hotspot/jtreg/compiler/ciReplay/TestSAClient.java 
open/test/hotspot/jtreg/compiler/ciReplay/TestSAServer.java


Chris

On 3/25/20 12:29 PM, Magnus Ihse Bursie wrote:
With the recent fixes in JDK-8241310, JDK-8237746 and 
JDK-8241073, and the upcoming fixes to remove the deprecated 
nashorn and jdk.rmi, the JDK build is very close to producing no 
warnings when compiling the Java classes.


The one remaining sinner is jdk.hotspot.agent. Most of the 
warnings here are turned off, but unchecked and deprecation 
cannot be completely silenced.


Since the poor agent does not seem to receive much love nowadays, 
I took it upon myself to fix these warnings, so we can finally 
get a quiet build.


I started to address the unchecked warnings. Unfortunately, this 
was a much bigger task than I anticipated. I had to generify most 
of the module. On the plus side, the code is so much better now. 
And most of the changes were trivial, just tedious.


There are a few places were I'm not entirely happy with the 
current solution, and that at least merits some discussion.


I have resorted to @SuppressWarnings in four classes: 
ciMethodData, MethodData, TableModelComparator and 
VirtualBaseConstructor. All of them has in common that they are 
doing slightly fishy things with classes in collections. I'm not 
entirely sure they are bug-free, but this patch leaves the 
behavior untouched. I did some efforts to sort out the logic, but 
it turned out to be too hairy for me to fix, and it will probably 
require more substantial changes to the workings of the code.


To make the code valid, I have moved ConstMethod to extend 
Metadata instead of VMObject. My understanding is that this is 
benign (and likely intended), but I really need for someone who 
knows the code to confirm this. I have also added a FIXME to 
signal this. I'll remove the FIXME as soon as I get confirmation 
that this is OK.
(The reason for this is the following piece of code from 
Metadata.java

Re: Ping: Re: RFR: JDK-8241618 Fix unchecked warning for jdk.hotspot.agent

[Magnus Ihse Bursie]

On 2020-04-14 23:23, Chris Plummer wrote:

Hi Magnus,

The changes look good. Just one minor issue:

http://cr.openjdk.java.net/~ihse/JDK-8241618-fix-unchecked-warnings-for-agent/webrev.02/src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/oops/Metadata.java.frames.html

Copyright was updated, but no other changes in this file.

Thanks for noticing! I reverted that change before pushing.



BTW, do you know why there were cases in the old code of having the 
right generic type in a comment. For example:


1177 private static List/**/ 
getInstanceFields(InstanceKlass ik) {


and

1276 private Map classDataCache = new HashMap(); // 



It's almost as if some of the code was initially properly written for 
generics, but then backported to work with a pre-generics version of jdk.
Yeah, isn't it? I have no clue, though. Most of the code is written 
completely oblivious to generics. Some parts have the typical 
pre-generics style of documenting in comments what types were expected, 
but most said nothing about the issue. And a few places has something 
that look like proper generics commented out. Maybe that's just the 
original programmer using C++ generics-inspired syntax as a way to 
comment the expected types?


/Magnus


thanks,

Chris

On 4/14/20 2:24 AM, Magnus Ihse Bursie wrote:

Hi,

Can I please get a review for this, simplified version of the patch? 
This only contain trivial changes, like this:

-  private Listobjects; // ArrayList
+  private List objects;
Basically all changes are to the container types List or Map (and a 
few changes from Class to Class).


If the list of all the files look horrible in the webrev, have a look 
at the patch file instead, it is easier to scroll through and check 
the changes:

http://cr.openjdk.java.net/~ihse/JDK-8241618-fix-unchecked-warnings-for-agent/webrev.02/open.patch

/Magnus

On 2020-03-30 16:25, Magnus Ihse Bursie wrote:



On 2020-03-25 20:52, Chris Plummer wrote:

Hi Magus,

I haven't looked at the changes yet, other to see that there are 
many files touched, but after reading below (and only partly 
understanding since I don't know this area well), I was wondering 
if this issue wouldn't be better served with multiple passes made 
to fix the warnings. Start with a straight forward one where you 
are maybe only making one or two types of changes, but that affect 
a large number of files and don't cascade into other more 
complicated changes. This will get a lot of the noise out of the 
way, and then we can focus on some of the harder issues you bring 
up below.
Ok, I did just this. Here is an updated webrev. It contain the bulk 
of the changes, but all changes are -- I dare not say trivially 
obvious, but at least no-brainers. Hopefully it should be easier to 
review so I can get this pushed and out of the way.


This also means that it is not possible to turn on the warning just 
yet.


http://cr.openjdk.java.net/~ihse/JDK-8241618-fix-unchecked-warnings-for-agent/webrev.02 



/Magnus


As for testing, I think the following list will capture all of 
them, but can't say for sure:


open/test/hotspot/jtreg/serviceability/sa
open/test/hotspot/jtreg/resourcehogs/serviceability/sa
open/test/jdk/sun/tools/jhsdb
open/test/jdk/sun/tools/jstack
open/test/jdk/sun/tools/jmap
open/test/hotspot/jtreg/gc/metaspace/CompressedClassSpaceSizeInJmapHeap.java 

open/test/hotspot/jtreg/compiler/ciReplay/TestSAClient.java 
open/test/hotspot/jtreg/compiler/ciReplay/TestSAServer.java


Chris

On 3/25/20 12:29 PM, Magnus Ihse Bursie wrote:
With the recent fixes in JDK-8241310, JDK-8237746 and JDK-8241073, 
and the upcoming fixes to remove the deprecated nashorn and 
jdk.rmi, the JDK build is very close to producing no warnings when 
compiling the Java classes.


The one remaining sinner is jdk.hotspot.agent. Most of the 
warnings here are turned off, but unchecked and deprecation cannot 
be completely silenced.


Since the poor agent does not seem to receive much love nowadays, 
I took it upon myself to fix these warnings, so we can finally get 
a quiet build.


I started to address the unchecked warnings. Unfortunately, this 
was a much bigger task than I anticipated. I had to generify most 
of the module. On the plus side, the code is so much better now. 
And most of the changes were trivial, just tedious.


There are a few places were I'm not entirely happy with the 
current solution, and that at least merits some discussion.


I have resorted to @SuppressWarnings in four classes: 
ciMethodData, MethodData, TableModelComparator and 
VirtualBaseConstructor. All of them has in common that they are 
doing slightly fishy things with classes in collections. I'm not 
entirely sure they are bug-free, but this patch leaves the 
behavior untouched. I did some efforts to sort out the logic, but 
it turned out to be too hairy for me to fix, and it will probably 
require more substantial changes to the workings of the code.


To make the code valid, I have moved ConstMethod

Re: RFR: JDK-8242629 Remove references to deprecated java.util.Observer and Observable

[Magnus Ihse Bursie]

On 2020-04-14 14:44, coleen.phillim...@oracle.com wrote:


This looks good to me since we can't remove this. 

Thanks for your review!

Does jdk.hotspot.agent follow the Hotspot review rules requiring two 
Reviewers?

The patch complains that for your Observer.java:

\ No newline at end of file

Yeah, I just noticed that. :-( I'll fix that before pushing.

/Magnus



Thanks,
Coleen

On 4/14/20 7:04 AM, Magnus Ihse Bursie wrote:
As a first step towards fixing deprecation warnings in SA, all the 
references (200+) to the deprecated java.util.Observer and Observable 
needs to be fixed, otherwise all other changes will drown in this one.


This solution is the result of the preceding discussions in 
serviceability-dev. That means that most of the change consists of 
adding an explicit import like this:


import sun.jvm.hotspot.utilities.Observable;
import sun.jvm.hotspot.utilities.Observer;

to override the general java.util.* import that was already present 
in all (or almost all) files, and make 
sun.jvm.hotspot.utilities.Observable and Observer extend the 
java.util versions but with deprecation warnings disabled.


It turned out however, that this simplest approach did not work 
fully. Since the interface java.util.Observer had the single method 
"void update(java.util.Observable o, Object arg)" it did not help to 
create a new interface sun.jvm.hotspot.utilities.Observer that 
extended java.util.Observer. I did not observe this issue in my PoC 
webrev that I posted during the discussion. :-(


Instead, for Observer, I had just created a new interface with the 
same method, but that uses sun.jvm.hotspot.utilities.Observable 
instead of java.util.Observable.


The end effect is the same -- the only change needed to most files is 
an added import, we get rid of the deprecation warnings, and we did 
not have to copy any significant amount of code from java.util.


I now hope this is acceptable by all.

Bug: https://bugs.openjdk.java.net/browse/JDK-8242629
WebRev: 
http://cr.openjdk.java.net/~ihse/JDK-8242629-fix-SA-Observer/webrev.01


(When reading the patch, I recommend looking at the patch file 
http://cr.openjdk.java.net/~ihse/JDK-8242629-fix-SA-Observer/webrev.01/open.patch 
instead of individually checking the files in the webrev.)


/Magnus

RFR: JDK-8242629 Remove references to deprecated java.util.Observer and Observable

[Magnus Ihse Bursie]

As a first step towards fixing deprecation warnings in SA, all the 
references (200+) to the deprecated java.util.Observer and Observable 
needs to be fixed, otherwise all other changes will drown in this one.


This solution is the result of the preceding discussions in 
serviceability-dev. That means that most of the change consists of 
adding an explicit import like this:


import sun.jvm.hotspot.utilities.Observable;
import sun.jvm.hotspot.utilities.Observer;

to override the general java.util.* import that was already present in 
all (or almost all) files, and make sun.jvm.hotspot.utilities.Observable 
and Observer extend the java.util versions but with deprecation warnings 
disabled.


It turned out however, that this simplest approach did not work fully. 
Since the interface java.util.Observer had the single method "void 
update(java.util.Observable o, Object arg)" it did not help to create a 
new interface sun.jvm.hotspot.utilities.Observer that extended 
java.util.Observer. I did not observe this issue in my PoC webrev that I 
posted during the discussion. :-(


Instead, for Observer, I had just created a new interface with the same 
method, but that uses sun.jvm.hotspot.utilities.Observable instead of 
java.util.Observable.


The end effect is the same -- the only change needed to most files is an 
added import, we get rid of the deprecation warnings, and we did not 
have to copy any significant amount of code from java.util.


I now hope this is acceptable by all.

Bug: https://bugs.openjdk.java.net/browse/JDK-8242629
WebRev: 
http://cr.openjdk.java.net/~ihse/JDK-8242629-fix-SA-Observer/webrev.01


(When reading the patch, I recommend looking at the patch file 
http://cr.openjdk.java.net/~ihse/JDK-8242629-fix-SA-Observer/webrev.01/open.patch 
instead of individually checking the files in the webrev.)


/Magnus

Ping: Re: RFR: JDK-8241618 Fix unchecked warning for jdk.hotspot.agent

[Magnus Ihse Bursie]

Hi,

Can I please get a review for this, simplified version of the patch? 
This only contain trivial changes, like this:


-  private Listobjects; // ArrayList
+  private List objects;

Basically all changes are to the container types List or Map (and a few 
changes from Class to Class).


If the list of all the files look horrible in the webrev, have a look at 
the patch file instead, it is easier to scroll through and check the 
changes:

http://cr.openjdk.java.net/~ihse/JDK-8241618-fix-unchecked-warnings-for-agent/webrev.02/open.patch

/Magnus

On 2020-03-30 16:25, Magnus Ihse Bursie wrote:



On 2020-03-25 20:52, Chris Plummer wrote:

Hi Magus,

I haven't looked at the changes yet, other to see that there are many 
files touched, but after reading below (and only partly understanding 
since I don't know this area well), I was wondering if this issue 
wouldn't be better served with multiple passes made to fix the 
warnings. Start with a straight forward one where you are maybe only 
making one or two types of changes, but that affect a large number of 
files and don't cascade into other more complicated changes. This 
will get a lot of the noise out of the way, and then we can focus on 
some of the harder issues you bring up below.
Ok, I did just this. Here is an updated webrev. It contain the bulk of 
the changes, but all changes are -- I dare not say trivially obvious, 
but at least no-brainers. Hopefully it should be easier to review so I 
can get this pushed and out of the way.


This also means that it is not possible to turn on the warning just yet.

http://cr.openjdk.java.net/~ihse/JDK-8241618-fix-unchecked-warnings-for-agent/webrev.02 



/Magnus


As for testing, I think the following list will capture all of them, 
but can't say for sure:


open/test/hotspot/jtreg/serviceability/sa
open/test/hotspot/jtreg/resourcehogs/serviceability/sa
open/test/jdk/sun/tools/jhsdb
open/test/jdk/sun/tools/jstack
open/test/jdk/sun/tools/jmap
open/test/hotspot/jtreg/gc/metaspace/CompressedClassSpaceSizeInJmapHeap.java 

open/test/hotspot/jtreg/compiler/ciReplay/TestSAClient.java 
open/test/hotspot/jtreg/compiler/ciReplay/TestSAServer.java


Chris

On 3/25/20 12:29 PM, Magnus Ihse Bursie wrote:
With the recent fixes in JDK-8241310, JDK-8237746 and JDK-8241073, 
and the upcoming fixes to remove the deprecated nashorn and jdk.rmi, 
the JDK build is very close to producing no warnings when compiling 
the Java classes.


The one remaining sinner is jdk.hotspot.agent. Most of the warnings 
here are turned off, but unchecked and deprecation cannot be 
completely silenced.


Since the poor agent does not seem to receive much love nowadays, I 
took it upon myself to fix these warnings, so we can finally get a 
quiet build.


I started to address the unchecked warnings. Unfortunately, this was 
a much bigger task than I anticipated. I had to generify most of the 
module. On the plus side, the code is so much better now. And most 
of the changes were trivial, just tedious.


There are a few places were I'm not entirely happy with the current 
solution, and that at least merits some discussion.


I have resorted to @SuppressWarnings in four classes: ciMethodData, 
MethodData, TableModelComparator and VirtualBaseConstructor. All of 
them has in common that they are doing slightly fishy things with 
classes in collections. I'm not entirely sure they are bug-free, but 
this patch leaves the behavior untouched. I did some efforts to sort 
out the logic, but it turned out to be too hairy for me to fix, and 
it will probably require more substantial changes to the workings of 
the code.


To make the code valid, I have moved ConstMethod to extend Metadata 
instead of VMObject. My understanding is that this is benign (and 
likely intended), but I really need for someone who knows the code 
to confirm this. I have also added a FIXME to signal this. I'll 
remove the FIXME as soon as I get confirmation that this is OK.
(The reason for this is the following piece of code from 
Metadata.java: metadataConstructor.addMapping("ConstMethod", 
ConstMethod.class))


In ObjectListPanel, there is some code that screams "dead" with this 
change. I added a FIXME to point this out:

    for (Iterator iter = elements.iterator(); iter.hasNext(); ) {
  if (iter.next() instanceof Array) {
    // FIXME: Does not seem possible to happen
    hasArrays = true;
    return;
  }
It seems that if you start pulling this thread, even more dead code 
will unravel, so I'm not so eager to touch this in the current 
patch. But I can remove the FIXME if you want.


My first iteration of this patch tried to generify the IntervalTree 
and related class hierarchy. However, this turned out to be 
impossible due to some weird usage in AnnotatedMemoryPanel, where 
there seemed to be confusion as to whether the tree stored 
Annotations or Addresses. I'm not entirely convinced the code is 
correct, it certainly looked an

Re: Discussion about fixing deprecation in jdk.hotspot.agent

[Magnus Ihse Bursie]

On 2020-04-01 12:22, Kevin Walls wrote:

Hi Coleen and all -

Given the choice I'd ask that we don't remove attach/detach because it 
limits the scope of what a clhsdb can do in future. Commands like 
jstack are a one-shot operation.  A Tool like clhsdb is ideally more 
flexible than that.


The SA is (I suggest) "too static" in its "there is one VM" approach, 
so we can't write a Tool that attaches to multiple VMs. If we remove 
attach/detach, it could not even gather information in a series of 
requests.  This is not going to be in the product any time soon, and 
maybe never, but it doesn't look right that we cut off such experiments.


Removing the Observer, yes would imply making detach into detach and 
exit.  I think the clhsdb "attach" command would still work (once 
only!) but is odd without detach (so do we make "bin/jhsdb clhsdb" 
require parameters...).


It looks like this changes the direction of the Tools in order to 
remove the deprecation warnings.


Magnus' webrev 
http://cr.openjdk.java.net/~ihse/hotspot-agent-observer/webrev.01/ 
does add/duplicate some code, but I like it for keeping things working 8-)


Here's an updated version of that approach that minimizes the amount of 
new code:


http://cr.openjdk.java.net/~ihse/hotspot-agent-observer/webrev.03

The difference is that I do not duplicate the classes themselves, I just 
subclass them to get a single point where the @SuppressWarnings can be 
put. The only change needed in the rest of the files is to make sure we 
import these classes instead of the ones in java.util.


/Magnus




Thanks
Kevin


On 31/03/2020 22:20, coleen.phillim...@oracle.com wrote:



On 3/31/20 4:55 PM, Chris Plummer wrote:

On 3/31/20 1:32 PM, coleen.phillim...@oracle.com wrote:



On 3/31/20 12:19 PM, Poonam Parhar wrote:

Hello Coleen,

Does the removal of this code only impact the 'reattach' 
functionality, and it does not affect any commands available in 
'clhsdb' once it is attached to a core file? If that's true, then 
I think it should be okay to remove this code.


Hi Poonam,  Thank you for answering. Yes, this patch only removes 
the reattach functionality.  I tried out the other clhsdb commands 
from your wiki page, and they worked fine, including object and 
heap inspection.
I'm trying to understand exactly when all these static initializes 
are triggered. Is it only after you do an attach?


The implementation of clhsdb reattach is exactly the same as doing a 
detach followed by an attach to the same process. I'm not sure how 
much value it has, but I think in general the removal of this code 
means you can't detach and then attach to anything, even a different 
pid. So "detach" might as well become "detach-and-exit", because 
your clhsdb session is dead once you detach. Do we really want to do 
this?


Well, that was my question. It seems like you could just exit and 
start up jhsdb again and that's more like something someone would do 
just as easily.  Given the use cases that we've seen from sustaining, 
this appears to be unneeded functionality.


The original mail was proposing adding more code to work around the 
deprecation messages.  It seems like more code should not be added 
for something that is unused.


thanks,
Coleen



Chris


Thanks,
Coleen


Thanks,
Poonam

On 3/31/20 5:34 AM, coleen.phillim...@oracle.com wrote:


To answer my own question, this functionality is used to allow 
detach/reattach from {cl}hsdb.  Which seems to work on linux but 
not windows with this code removed.


The next question is whether this is useful functionality to 
justify all this code (900+ and this new code that Magnus has 
added).  Can't you just exit and restart the clhsdb process on 
the core file or process?


For the record, this is me playing with python to remove this code.

http://cr.openjdk.java.net/~coleenp/2020/01/webrev/index.html

Thanks,
Coleen

On 3/30/20 3:04 PM, coleen.phillim...@oracle.com wrote:


I was wondering why this is needed when debugging a core file, 
which is the key thing we need the SA for:


  /** This is used by both the debugger and any runtime system. 
It is
  the basic mechanism by which classes which mimic 
underlying VM

  functionality cause themselves to be initialized. The given
  observer will be notified (with arguments (null, null)) 
when the
  VM is re-initialized, as well as when it registers itself 
with

  the VM. */
  public static void registerVMInitializedObserver(Observer o) {
    vmInitializedObservers.add(o);
    o.update(null, null);
  }

It seems like if it isn't needed, we shouldn't add these classes 
and remove their use.


Coleen

On 3/30/20 8:14 AM, Magnus Ihse Bursie wrote:

No opinions on this?

/Magnus

On 2020-03-25 23:34, Magnus Ihse Bursie wrote:

Hi everyone,

As a follow-up to the ongoing review for JDK-8241618, I have 
also looked at fixing the deprecation warnings in 
jdk.hotspot.agent. These fall i

Re: Discussion about fixing deprecation in jdk.hotspot.agent

[Magnus Ihse Bursie]

On 2020-03-31 14:34, coleen.phillim...@oracle.com wrote:


To answer my own question, this functionality is used to allow 
detach/reattach from {cl}hsdb.  Which seems to work on linux but not 
windows with this code removed.


The next question is whether this is useful functionality to justify 
all this code (900+ and this new code that Magnus has added).  Can't 
you just exit and restart the clhsdb process on the core file or process?


Personally, I'm happy for any solution. All I want to see is that SA 
stops polluting the build log with warnings that cannot be disabled. My 
approach was to minimize the amount of code changes that'd allow for 
this, but if you all can agree that this code is better off removed, 
then I'm completely OK with it. (And as a rule of thumb, dead and 
removed code is good code!)


/Magnus


For the record, this is me playing with python to remove this code.

http://cr.openjdk.java.net/~coleenp/2020/01/webrev/index.html

Thanks,
Coleen

On 3/30/20 3:04 PM, coleen.phillim...@oracle.com wrote:


I was wondering why this is needed when debugging a core file, which 
is the key thing we need the SA for:


  /** This is used by both the debugger and any runtime system. It is
  the basic mechanism by which classes which mimic underlying VM
  functionality cause themselves to be initialized. The given
  observer will be notified (with arguments (null, null)) when the
  VM is re-initialized, as well as when it registers itself with
  the VM. */
  public static void registerVMInitializedObserver(Observer o) {
    vmInitializedObservers.add(o);
    o.update(null, null);
  }

It seems like if it isn't needed, we shouldn't add these classes and 
remove their use.


Coleen

On 3/30/20 8:14 AM, Magnus Ihse Bursie wrote:

No opinions on this?

/Magnus

On 2020-03-25 23:34, Magnus Ihse Bursie wrote:

Hi everyone,

As a follow-up to the ongoing review for JDK-8241618, I have also 
looked at fixing the deprecation warnings in jdk.hotspot.agent. 
These fall in three broad categories:


* Deprecation of the boxing type constructors (e.g. "new 
Integer(42)").


* Deprecation of java.util.Observer and Observable.

* The rest (mostly Class.newInstance(), and a few number of other 
odd deprecations)


The first category is trivial to fix. The last category need some 
special discussion. But the overwhelming majority of deprecation 
warnings come from the use of Observer and Observable. This really 
dwarfs anything else, and needs to be handled first, otherwise it's 
hard to even spot the other issues.


My analysis of the situation is that the deprecation of Observer 
and Observable seems a bit harsh, from the PoV of 
jdk.hotspot.agent. Sure, it might be limited, but I think it does 
exactly what is needed here. So the migration suggested in 
Observable (java.beans or java.util.concurrent) seems overkill. If 
there are genuine threading issues at play here, this assumption 
might be wrong, and then maybe going the j.u.c. route is correct.


But if that's not, the main goal should be to stay with the current 
implementation. One way to do this is to sprinkle the code with 
@SuppressWarning. But I think a better way would be to just 
implement our own Observer and Observable. After all, the classes 
are trivial.


I've made a mock-up of this solution, were I just copied the 
java.util.Observer and Observable, and removed the deprecation 
annotations. The only thing needed for the rest of the code is to 
make sure we import these; I've done this for three arbitrarily 
selected classes just to show what the change would typically look 
like. Here's the mock-up:


http://cr.openjdk.java.net/~ihse/hotspot-agent-observer/webrev.01

Let me know what you think.

/Magnus

Re: RFR: JDK-8241618 Fix unchecked warning for jdk.hotspot.agent

[Magnus Ihse Bursie]

On 2020-03-25 20:52, Chris Plummer wrote:

Hi Magus,

I haven't looked at the changes yet, other to see that there are many 
files touched, but after reading below (and only partly understanding 
since I don't know this area well), I was wondering if this issue 
wouldn't be better served with multiple passes made to fix the 
warnings. Start with a straight forward one where you are maybe only 
making one or two types of changes, but that affect a large number of 
files and don't cascade into other more complicated changes. This will 
get a lot of the noise out of the way, and then we can focus on some 
of the harder issues you bring up below.
Ok, I did just this. Here is an updated webrev. It contain the bulk of 
the changes, but all changes are -- I dare not say trivially obvious, 
but at least no-brainers. Hopefully it should be easier to review so I 
can get this pushed and out of the way.


This also means that it is not possible to turn on the warning just yet.

http://cr.openjdk.java.net/~ihse/JDK-8241618-fix-unchecked-warnings-for-agent/webrev.02

/Magnus


As for testing, I think the following list will capture all of them, 
but can't say for sure:


open/test/hotspot/jtreg/serviceability/sa
open/test/hotspot/jtreg/resourcehogs/serviceability/sa
open/test/jdk/sun/tools/jhsdb
open/test/jdk/sun/tools/jstack
open/test/jdk/sun/tools/jmap
open/test/hotspot/jtreg/gc/metaspace/CompressedClassSpaceSizeInJmapHeap.java 

open/test/hotspot/jtreg/compiler/ciReplay/TestSAClient.java 
open/test/hotspot/jtreg/compiler/ciReplay/TestSAServer.java


Chris

On 3/25/20 12:29 PM, Magnus Ihse Bursie wrote:
With the recent fixes in JDK-8241310, JDK-8237746 and JDK-8241073, 
and the upcoming fixes to remove the deprecated nashorn and jdk.rmi, 
the JDK build is very close to producing no warnings when compiling 
the Java classes.


The one remaining sinner is jdk.hotspot.agent. Most of the warnings 
here are turned off, but unchecked and deprecation cannot be 
completely silenced.


Since the poor agent does not seem to receive much love nowadays, I 
took it upon myself to fix these warnings, so we can finally get a 
quiet build.


I started to address the unchecked warnings. Unfortunately, this was 
a much bigger task than I anticipated. I had to generify most of the 
module. On the plus side, the code is so much better now. And most of 
the changes were trivial, just tedious.


There are a few places were I'm not entirely happy with the current 
solution, and that at least merits some discussion.


I have resorted to @SuppressWarnings in four classes: ciMethodData, 
MethodData, TableModelComparator and VirtualBaseConstructor. All of 
them has in common that they are doing slightly fishy things with 
classes in collections. I'm not entirely sure they are bug-free, but 
this patch leaves the behavior untouched. I did some efforts to sort 
out the logic, but it turned out to be too hairy for me to fix, and 
it will probably require more substantial changes to the workings of 
the code.


To make the code valid, I have moved ConstMethod to extend Metadata 
instead of VMObject. My understanding is that this is benign (and 
likely intended), but I really need for someone who knows the code to 
confirm this. I have also added a FIXME to signal this. I'll remove 
the FIXME as soon as I get confirmation that this is OK.
(The reason for this is the following piece of code from 
Metadata.java: metadataConstructor.addMapping("ConstMethod", 
ConstMethod.class))


In ObjectListPanel, there is some code that screams "dead" with this 
change. I added a FIXME to point this out:

    for (Iterator iter = elements.iterator(); iter.hasNext(); ) {
  if (iter.next() instanceof Array) {
    // FIXME: Does not seem possible to happen
    hasArrays = true;
    return;
  }
It seems that if you start pulling this thread, even more dead code 
will unravel, so I'm not so eager to touch this in the current patch. 
But I can remove the FIXME if you want.


My first iteration of this patch tried to generify the IntervalTree 
and related class hierarchy. However, this turned out to be 
impossible due to some weird usage in AnnotatedMemoryPanel, where 
there seemed to be confusion as to whether the tree stored 
Annotations or Addresses. I'm not entirely convinced the code is 
correct, it certainly looked and smelled very fishy. However, I 
reverted these changes since I could not get them to work due to 
this, and it was not needed for the goal of just getting rid of the 
warning.


Finally, I have done no testing apart from verifying that it builds. 
Please advice on suitable tests to run.


Bug: https://bugs.openjdk.java.net/browse/JDK-8241618
WebRev: 
http://cr.openjdk.java.net/~ihse/JDK-8241618-fix-unchecked-warnings-for-agent/webrev.01


/Magnus

Re: Discussion about fixing deprecation in jdk.hotspot.agent

[Magnus Ihse Bursie]

No opinions on this?

/Magnus

On 2020-03-25 23:34, Magnus Ihse Bursie wrote:

Hi everyone,

As a follow-up to the ongoing review for JDK-8241618, I have also 
looked at fixing the deprecation warnings in jdk.hotspot.agent. These 
fall in three broad categories:


* Deprecation of the boxing type constructors (e.g. "new Integer(42)").

* Deprecation of java.util.Observer and Observable.

* The rest (mostly Class.newInstance(), and a few number of other odd 
deprecations)


The first category is trivial to fix. The last category need some 
special discussion. But the overwhelming majority of deprecation 
warnings come from the use of Observer and Observable. This really 
dwarfs anything else, and needs to be handled first, otherwise it's 
hard to even spot the other issues.


My analysis of the situation is that the deprecation of Observer and 
Observable seems a bit harsh, from the PoV of jdk.hotspot.agent. Sure, 
it might be limited, but I think it does exactly what is needed here. 
So the migration suggested in Observable (java.beans or 
java.util.concurrent) seems overkill. If there are genuine threading 
issues at play here, this assumption might be wrong, and then maybe 
going the j.u.c. route is correct.


But if that's not, the main goal should be to stay with the current 
implementation. One way to do this is to sprinkle the code with 
@SuppressWarning. But I think a better way would be to just implement 
our own Observer and Observable. After all, the classes are trivial.


I've made a mock-up of this solution, were I just copied the 
java.util.Observer and Observable, and removed the deprecation 
annotations. The only thing needed for the rest of the code is to make 
sure we import these; I've done this for three arbitrarily selected 
classes just to show what the change would typically look like. Here's 
the mock-up:


http://cr.openjdk.java.net/~ihse/hotspot-agent-observer/webrev.01

Let me know what you think.

/Magnus

Discussion about fixing deprecation in jdk.hotspot.agent

[Magnus Ihse Bursie]

Hi everyone,

As a follow-up to the ongoing review for JDK-8241618, I have also looked 
at fixing the deprecation warnings in jdk.hotspot.agent. These fall in 
three broad categories:


* Deprecation of the boxing type constructors (e.g. "new Integer(42)").

* Deprecation of java.util.Observer and Observable.

* The rest (mostly Class.newInstance(), and a few number of other odd 
deprecations)


The first category is trivial to fix. The last category need some 
special discussion. But the overwhelming majority of deprecation 
warnings come from the use of Observer and Observable. This really 
dwarfs anything else, and needs to be handled first, otherwise it's hard 
to even spot the other issues.


My analysis of the situation is that the deprecation of Observer and 
Observable seems a bit harsh, from the PoV of jdk.hotspot.agent. Sure, 
it might be limited, but I think it does exactly what is needed here. So 
the migration suggested in Observable (java.beans or 
java.util.concurrent) seems overkill. If there are genuine threading 
issues at play here, this assumption might be wrong, and then maybe 
going the j.u.c. route is correct.


But if that's not, the main goal should be to stay with the current 
implementation. One way to do this is to sprinkle the code with 
@SuppressWarning. But I think a better way would be to just implement 
our own Observer and Observable. After all, the classes are trivial.


I've made a mock-up of this solution, were I just copied the 
java.util.Observer and Observable, and removed the deprecation 
annotations. The only thing needed for the rest of the code is to make 
sure we import these; I've done this for three arbitrarily selected 
classes just to show what the change would typically look like. Here's 
the mock-up:


http://cr.openjdk.java.net/~ihse/hotspot-agent-observer/webrev.01

Let me know what you think.

/Magnus

Re: RFR: JDK-8241618 Fix unchecked warning for jdk.hotspot.agent

[Magnus Ihse Bursie]

On 2020-03-25 20:52, Chris Plummer wrote:

Hi Magus,

I haven't looked at the changes yet, other to see that there are many 
files touched, but after reading below (and only partly understanding 
since I don't know this area well), I was wondering if this issue 
wouldn't be better served with multiple passes made to fix the 
warnings. Start with a straight forward one where you are maybe only 
making one or two types of changes, but that affect a large number of 
files and don't cascade into other more complicated changes. 
Unfortunately, many changes tends to cling together -- for instance, 
class Foo has a List fooList of say Integer. If I change that to 
List, then also the constructor needs to change, and the 
getFooList() method, and that in turn propagate to users of getFooList() 
etc. I tried to do this piecewise but for every line that I fixed I just 
ended up getting more and more places that needed fixing.


On the other hand, the patch I present *is* indeed mostly trivial. Apart 
from the places I mentioned below, the fixes are straightforward. And I 
opted out of fixing the tricky ones by disabling the warnings. My 
intention is to file a follow-up bug for these @SuppressWarnings to be 
fixed properly. However, doing that is unfortunately beyond the scope of 
what I'm able to do, since I do not have enough domain knowledge. The 
fixes in this patch is more or less "stupid" applications of adding 
generics with the correct type. (Basically, what I've done is to locate 
a problematic type, like fooList, and check the type of elements 
inserted and extracted of it, and created it as a generic of that type. 
Boring, but not really difficult.)


I realize the webrev can look daunting. Perhaps start by looking at the 
patch file, that will quickly show what kind of changes this is about. 
Also, 1/3 of the patch is just about updating those darned copyright 
years. :-(


This will get a lot of the noise out of the way, and then we can focus 
on some of the harder issues you bring up below.


As for testing, I think the following list will capture all of them, 
but can't say for sure:


open/test/hotspot/jtreg/serviceability/sa
open/test/hotspot/jtreg/resourcehogs/serviceability/sa
open/test/jdk/sun/tools/jhsdb
open/test/jdk/sun/tools/jstack
open/test/jdk/sun/tools/jmap
open/test/hotspot/jtreg/gc/metaspace/CompressedClassSpaceSizeInJmapHeap.java 

open/test/hotspot/jtreg/compiler/ciReplay/TestSAClient.java 
open/test/hotspot/jtreg/compiler/ciReplay/TestSAServer.java

Thank you! I'll run these through our test system.

/Magnus


Chris

On 3/25/20 12:29 PM, Magnus Ihse Bursie wrote:
With the recent fixes in JDK-8241310, JDK-8237746 and JDK-8241073, 
and the upcoming fixes to remove the deprecated nashorn and jdk.rmi, 
the JDK build is very close to producing no warnings when compiling 
the Java classes.


The one remaining sinner is jdk.hotspot.agent. Most of the warnings 
here are turned off, but unchecked and deprecation cannot be 
completely silenced.


Since the poor agent does not seem to receive much love nowadays, I 
took it upon myself to fix these warnings, so we can finally get a 
quiet build.


I started to address the unchecked warnings. Unfortunately, this was 
a much bigger task than I anticipated. I had to generify most of the 
module. On the plus side, the code is so much better now. And most of 
the changes were trivial, just tedious.


There are a few places were I'm not entirely happy with the current 
solution, and that at least merits some discussion.


I have resorted to @SuppressWarnings in four classes: ciMethodData, 
MethodData, TableModelComparator and VirtualBaseConstructor. All of 
them has in common that they are doing slightly fishy things with 
classes in collections. I'm not entirely sure they are bug-free, but 
this patch leaves the behavior untouched. I did some efforts to sort 
out the logic, but it turned out to be too hairy for me to fix, and 
it will probably require more substantial changes to the workings of 
the code.


To make the code valid, I have moved ConstMethod to extend Metadata 
instead of VMObject. My understanding is that this is benign (and 
likely intended), but I really need for someone who knows the code to 
confirm this. I have also added a FIXME to signal this. I'll remove 
the FIXME as soon as I get confirmation that this is OK.
(The reason for this is the following piece of code from 
Metadata.java: metadataConstructor.addMapping("ConstMethod", 
ConstMethod.class))


In ObjectListPanel, there is some code that screams "dead" with this 
change. I added a FIXME to point this out:

    for (Iterator iter = elements.iterator(); iter.hasNext(); ) {
  if (iter.next() instanceof Array) {
    // FIXME: Does not seem possible to happen
    hasArrays = true;
    return;
  }
It seems that if you start pulling this thread, even more dead code 
will unravel, so I'm not so eager to touch this in the cu

RFR: JDK-8241618 Fix unchecked warning for jdk.hotspot.agent

[Magnus Ihse Bursie]

With the recent fixes in JDK-8241310, JDK-8237746 and JDK-8241073, and 
the upcoming fixes to remove the deprecated nashorn and jdk.rmi, the JDK 
build is very close to producing no warnings when compiling the Java 
classes.


The one remaining sinner is jdk.hotspot.agent. Most of the warnings here 
are turned off, but unchecked and deprecation cannot be completely silenced.


Since the poor agent does not seem to receive much love nowadays, I took 
it upon myself to fix these warnings, so we can finally get a quiet build.


I started to address the unchecked warnings. Unfortunately, this was a 
much bigger task than I anticipated. I had to generify most of the 
module. On the plus side, the code is so much better now. And most of 
the changes were trivial, just tedious.


There are a few places were I'm not entirely happy with the current 
solution, and that at least merits some discussion.


I have resorted to @SuppressWarnings in four classes: ciMethodData, 
MethodData, TableModelComparator and VirtualBaseConstructor. All of them 
has in common that they are doing slightly fishy things with classes in 
collections. I'm not entirely sure they are bug-free, but this patch 
leaves the behavior untouched. I did some efforts to sort out the logic, 
but it turned out to be too hairy for me to fix, and it will probably 
require more substantial changes to the workings of the code.


To make the code valid, I have moved ConstMethod to extend Metadata 
instead of VMObject. My understanding is that this is benign (and likely 
intended), but I really need for someone who knows the code to confirm 
this. I have also added a FIXME to signal this. I'll remove the FIXME as 
soon as I get confirmation that this is OK.
(The reason for this is the following piece of code from Metadata.java: 
metadataConstructor.addMapping("ConstMethod", ConstMethod.class))


In ObjectListPanel, there is some code that screams "dead" with this 
change. I added a FIXME to point this out:

    for (Iterator iter = elements.iterator(); iter.hasNext(); ) {
  if (iter.next() instanceof Array) {
    // FIXME: Does not seem possible to happen
    hasArrays = true;
    return;
  }
It seems that if you start pulling this thread, even more dead code will 
unravel, so I'm not so eager to touch this in the current patch. But I 
can remove the FIXME if you want.


My first iteration of this patch tried to generify the IntervalTree and 
related class hierarchy. However, this turned out to be impossible due 
to some weird usage in AnnotatedMemoryPanel, where there seemed to be 
confusion as to whether the tree stored Annotations or Addresses. I'm 
not entirely convinced the code is correct, it certainly looked and 
smelled very fishy. However, I reverted these changes since I could not 
get them to work due to this, and it was not needed for the goal of just 
getting rid of the warning.


Finally, I have done no testing apart from verifying that it builds. 
Please advice on suitable tests to run.


Bug: https://bugs.openjdk.java.net/browse/JDK-8241618
WebRev: 
http://cr.openjdk.java.net/~ihse/JDK-8241618-fix-unchecked-warnings-for-agent/webrev.01


/Magnus

Re: RFR: JDK-8241463 Move build tools to respective modules

[Magnus Ihse Bursie]

On 2020-03-23 23:15, naoto.s...@oracle.com wrote:

Hi Magnus,

I looked at i18n related changes:

make/CopyInterimTZDB.gmk
make/ToolsJdk.gmk
make/gendata/Gendata-java.base.gmk
make/gendata/GendataBreakIterator.gmk
make/gendata/GendataTZDB.gmk
make/gensrc/GensrcCharacterData.gmk
make/gensrc/GensrcEmojiData.gmk

They look ok to me.

Thank you!


The *.java changes should have copyright year update.

Ok, I'll update them.


As to charsetmapping and cldrconverter, I believe they can reside in 
java.base, as jdk.charsets and jdk.localedata modules depend on it.

Okay. It's not ideal, but I think you're right. I'll move them as well.

I'll publish an updated webrev with these changes when there's agreement 
on where in the source code tree to move the files.


/Magnus


Naoto

On 3/23/20 12:03 PM, Magnus Ihse Bursie wrote:
The build tools (small java tools that are run during the build to 
generate source code, or data, needed in the JDK) have historically 
been placed in the "make" directory. This maybe made sense long time 
ago, but does not do so anymore.


Instead, the build tools source code should move the the module that 
needs them. For instance, compilefontconfig should move to 
java.desktop, etc.


There are multiple reasons for this:

* Currently we build *all* build tools at once, which mean that we 
cannot compile java.base until e.g. the compilefontconfig tool is 
compiled, even though it is not needed.


* If a build tool, e.g. compilefontconfig is modified, all build 
tools are recompiled, which triggers a rebuild of more or less the 
entire JDK. This makes development of the build tools unnecessary 
tedious.


* When the build tools are modified, the group owning the 
corresponding module is the proper review instance, not the build 
team. But since they reside under "make", the review mails often 
include build-dev, but this is mostly noise for us. With this move, 
the ownership is made clear.


In this patch, I have not modified how and when the build tools are 
compiled, but this shuffle is the prerequisite for continuing with 
that in a follow-up patch.


I have also moved the build tools to the org.openjdk.buildtools.* 
package name space (inspired by Skara), instead of the strangely 
named build.tools.* name space.


A few build tools are not moved in this patch. Two of them, 
charsetmapping and cldrconverter, are shared between two modules. (I 
think they should move to modules nevertheless, but they need some 
more thought to make sure I do this right.) The rest are tools that 
are needed for the build in general, like linking or javadoc support. 
I'll move this to a better location too, but in a separate patch.


Bug: https://bugs.openjdk.java.net/browse/JDK-8241463
WebRev: 
http://cr.openjdk.java.net/~ihse/JDK-8241463-move-build-tools-to-modules/webrev.01 



/Magnus

RFR: JDK-8241463 Move build tools to respective modules

[Magnus Ihse Bursie]

The build tools (small java tools that are run during the build to 
generate source code, or data, needed in the JDK) have historically been 
placed in the "make" directory. This maybe made sense long time ago, but 
does not do so anymore.


Instead, the build tools source code should move the the module that 
needs them. For instance, compilefontconfig should move to java.desktop, 
etc.


There are multiple reasons for this:

* Currently we build *all* build tools at once, which mean that we 
cannot compile java.base until e.g. the compilefontconfig tool is 
compiled, even though it is not needed.


* If a build tool, e.g. compilefontconfig is modified, all build tools 
are recompiled, which triggers a rebuild of more or less the entire JDK. 
This makes development of the build tools unnecessary tedious.


* When the build tools are modified, the group owning the corresponding 
module is the proper review instance, not the build team. But since they 
reside under "make", the review mails often include build-dev, but this 
is mostly noise for us. With this move, the ownership is made clear.


In this patch, I have not modified how and when the build tools are 
compiled, but this shuffle is the prerequisite for continuing with that 
in a follow-up patch.


I have also moved the build tools to the org.openjdk.buildtools.* 
package name space (inspired by Skara), instead of the strangely named 
build.tools.* name space.


A few build tools are not moved in this patch. Two of them, 
charsetmapping and cldrconverter, are shared between two modules. (I 
think they should move to modules nevertheless, but they need some more 
thought to make sure I do this right.) The rest are tools that are 
needed for the build in general, like linking or javadoc support. I'll 
move this to a better location too, but in a separate patch.


Bug: https://bugs.openjdk.java.net/browse/JDK-8241463
WebRev: 
http://cr.openjdk.java.net/~ihse/JDK-8241463-move-build-tools-to-modules/webrev.01


/Magnus

Re: serviceability agent : problems when using gcc LTO (link time optimization)

[Magnus Ihse Bursie]

On 2020-01-14 13:49, Baesken, Matthias wrote:


Hi Magnus, thanks for the info , I already noticed yesterday the 
setting for arm-32 in the minimal build .


Do you think  we could set it too for the other Linux platforms  in 
the minimal build  ( serviceability agent is not supported there as 
well so the  observed issue wouldn’t be a problem).




You mean if you could enable it on your builds without any issues? I'd 
guess so, but I don't know. Just try it: 
--with-jvm-features="link-time-opt".


If you mean that it should be turned on by default on minimal builds for 
all platforms? No, I don't think that's a good idea. The link time is 
really a killer. I remember arm-32 going from like a couple of minutes 
to half an hour for linking libjvm.so.


Things might be different with gold, though. I know they have done work 
with at least some kind of "lightweight" LTO, that might be worth at 
least looking into.


/Magnus



Best regards, Matthias

On 2020-01-10 11:01, Baesken, Matthias wrote:

Hello,   I recently looked into  the  gcc  lto  optimization mode (see for 
some detailshttps://gcc.gnu.org/onlinedocs/gccint/LTO-Overview.html   
andhttp://hubicka.blogspot.com/2019/05/gcc-9-link-time-and-inter-procedural.html
   ).

This mode can lead to more compact binaries (~10% smaller)  , it also might 
bring  small performance improvements  but that wasn't my (main)  goal  .

The changes for this are rather small , one needs to use a recent gcc  , 
add  -flto   to the compile flags  , for example

--- a/make/autoconf/flags-cflags.m4  Wed Jan 01 03:08:45 2020 +0100

+++ b/make/autoconf/flags-cflags.m4   Wed Jan 08 17:39:10 2020 +0100

@@ -530,8 +530,13 @@

    fi

    if test "x$TOOLCHAIN_TYPE" = xgcc; then

-    TOOLCHAIN_CFLAGS_JVM="$TOOLCHAIN_CFLAGS_JVM -fcheck-new 
-fstack-protector"

-    TOOLCHAIN_CFLAGS_JDK="-pipe -fstack-protector"

+    TOOLCHAIN_CFLAGS_JVM="$TOOLCHAIN_CFLAGS_JVM -fcheck-new -fstack-protector 
-flto"

+    TOOLCHAIN_CFLAGS_JDK="-pipe -fstack-protector -flto"

    and you have to make sure  to use  gcc-ar  and  gcc-nm instead   of 
 ar / nm .

Build and test(s)  work,  however with  one exception.

The  serviceability   tests like  serviceability/sa   seems to rely   heavily  on the 
"normal"   structure  of   libjvm.so   (from what I   understand  e.g. in  
LinuxVtblAccess  it is attempted to access  internal symbols  like  _ZTV ).

Errors in the sa  tests look like :

java.lang.InternalError: Metadata does not appear to be polymorphic

  at 
jdk.hotspot.agent/sun.jvm.hotspot.types.basic.BasicTypeDataBase.findDynamicTypeForAddress(BasicTypeDataBase.java:279)

  at 
jdk.hotspot.agent/sun.jvm.hotspot.runtime.VirtualBaseConstructor.instantiateWrapperFor(VirtualBaseConstructor.java:102)

  at 
jdk.hotspot.agent/sun.jvm.hotspot.oops.Metadata.instantiateWrapperFor(Metadata.java:74)

  at 
jdk.hotspot.agent/sun.jvm.hotspot.memory.SystemDictionary.getClassLoaderKlass(SystemDictionary.java:96)

  at 
jdk.hotspot.agent/sun.jvm.hotspot.tools.ClassLoaderStats.printClassLoaderStatistics(ClassLoaderStats.java:93)

  at 
jdk.hotspot.agent/sun.jvm.hotspot.tools.ClassLoaderStats.run(ClassLoaderStats.java:78)

  at jdk.hotspot.agent/sun.jvm.hotspot.tools.JMap.run(JMap.java:115)

      at 
jdk.hotspot.agent/sun.jvm.hotspot.tools.Tool.startInternal(Tool.java:262)

  at 
jdk.hotspot.agent/sun.jvm.hotspot.tools.Tool.start(Tool.java:225)

  at 
jdk.hotspot.agent/sun.jvm.hotspot.tools.Tool.execute(Tool.java:118)

  at 
jdk.hotspot.agent/sun.jvm.hotspot.tools.JMap.main(JMap.java:176)

  at 
jdk.hotspot.agent/sun.jvm.hotspot.SALauncher.runJMAP(SALauncher.java:321)

  at 
jdk.hotspot.agent/sun.jvm.hotspot.SALauncher.main(SALauncher.java:406)

Has anyone experimented with LTO optimization ?


Hi Matthias,

We used to have LTO enabled on the old, closed-source Oracle arm-32 
builds. There is still a "link-time-opt" JVM feature present; afaik it 
still works and adds the -flto flag. The main drawback of this is the 
*extremely* long link times of libjvm.so.


I don't think servicability was ever supported for that platform, so 
I'm not surprised this does not work.


/Magnus


And to the  serviceability   agent experts -  any idea  how to make the  
jdk.hotspot.agent   more independent from  optimization settings ?

Best regards, Matthias

Re: serviceability agent : problems when using gcc LTO (link time optimization)

[Magnus Ihse Bursie]

On 2020-01-10 11:01, Baesken, Matthias wrote:

Hello,   I recently looked into  the  gcc  lto  optimization mode (see for some 
details https://gcc.gnu.org/onlinedocs/gccint/LTO-Overview.html  and  
http://hubicka.blogspot.com/2019/05/gcc-9-link-time-and-inter-procedural.html  
).
This mode can lead to more compact binaries (~10% smaller)  , it also might 
bring  small performance improvements  but that wasn't my (main)  goal  .

The changes for this are rather small , one needs to use a recent gcc  , add  
-flto   to the compile flags  , for example

--- a/make/autoconf/flags-cflags.m4  Wed Jan 01 03:08:45 2020 +0100
+++ b/make/autoconf/flags-cflags.m4   Wed Jan 08 17:39:10 2020 +0100
@@ -530,8 +530,13 @@
fi
if test "x$TOOLCHAIN_TYPE" = xgcc; then
-TOOLCHAIN_CFLAGS_JVM="$TOOLCHAIN_CFLAGS_JVM -fcheck-new -fstack-protector"
-TOOLCHAIN_CFLAGS_JDK="-pipe -fstack-protector"
+TOOLCHAIN_CFLAGS_JVM="$TOOLCHAIN_CFLAGS_JVM -fcheck-new -fstack-protector 
-flto"
+TOOLCHAIN_CFLAGS_JDK="-pipe -fstack-protector -flto"

    and you have to make sure  to use  gcc-ar  and  gcc-nm instead   of  ar 
/ nm .
Build and test(s)  work,  however with  one exception.
The  serviceability   tests like  serviceability/sa   seems to rely   heavily  on the 
"normal"   structure  of   libjvm.so   (from what I   understand  e.g. in  
LinuxVtblAccess  it is attempted to access  internal symbols  like  _ZTV ).

Errors in the sa  tests look like :


java.lang.InternalError: Metadata does not appear to be polymorphic
  at 
jdk.hotspot.agent/sun.jvm.hotspot.types.basic.BasicTypeDataBase.findDynamicTypeForAddress(BasicTypeDataBase.java:279)
  at 
jdk.hotspot.agent/sun.jvm.hotspot.runtime.VirtualBaseConstructor.instantiateWrapperFor(VirtualBaseConstructor.java:102)
  at 
jdk.hotspot.agent/sun.jvm.hotspot.oops.Metadata.instantiateWrapperFor(Metadata.java:74)
  at 
jdk.hotspot.agent/sun.jvm.hotspot.memory.SystemDictionary.getClassLoaderKlass(SystemDictionary.java:96)
  at 
jdk.hotspot.agent/sun.jvm.hotspot.tools.ClassLoaderStats.printClassLoaderStatistics(ClassLoaderStats.java:93)
  at 
jdk.hotspot.agent/sun.jvm.hotspot.tools.ClassLoaderStats.run(ClassLoaderStats.java:78)
  at jdk.hotspot.agent/sun.jvm.hotspot.tools.JMap.run(JMap.java:115)
  at 
jdk.hotspot.agent/sun.jvm.hotspot.tools.Tool.startInternal(Tool.java:262)
  at jdk.hotspot.agent/sun.jvm.hotspot.tools.Tool.start(Tool.java:225)
  at jdk.hotspot.agent/sun.jvm.hotspot.tools.Tool.execute(Tool.java:118)
  at jdk.hotspot.agent/sun.jvm.hotspot.tools.JMap.main(JMap.java:176)
  at 
jdk.hotspot.agent/sun.jvm.hotspot.SALauncher.runJMAP(SALauncher.java:321)
  at 
jdk.hotspot.agent/sun.jvm.hotspot.SALauncher.main(SALauncher.java:406)

Has anyone experimented with LTO optimization ?


Hi Matthias,

We used to have LTO enabled on the old, closed-source Oracle arm-32 
builds. There is still a "link-time-opt" JVM feature present; afaik it 
still works and adds the -flto flag. The main drawback of this is the 
*extremely* long link times of libjvm.so.


I don't think servicability was ever supported for that platform, so I'm 
not surprised this does not work.


/Magnus



And to the  serviceability   agent experts -  any idea  how to make the  
jdk.hotspot.agent   more independent from  optimization settings ?


Best regards, Matthias

Re: RFR: 8233285: Demangling C++ symbols in jhsdb jstack --mixed

[Magnus Ihse Bursie]

On 2019-11-02 13:43, Daniel D. Daugherty wrote:

Since this review contains build changes, I've added build-dev@...

Thanks Dan for noticing this and cc:ing us.

Yasumasa: build changes look fine. Thanks.

/Magnus


Dan


On 11/1/19 4:56 AM, Yasumasa Suenaga wrote:

(Changed subject to review request)

Hi,

I converted LinuxDebuggerLocal.c to C++ code, and it works fine on 
submit repo.

(mach5-one-ysuenaga-JDK-8233285-1-20191101-0746-6336009)

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

Could you review it?


Thanks,

Yasumasa


On 2019/11/01 8:54, Yasumasa Suenaga wrote:

Hi David,

On 2019/11/01 7:55, David Holmes wrote:

Hi Yasumasa,

New build dependencies cannot be added lightly. This impacts 
everyone who maintains build/test farms.


Ok, thanks for telling it.

We already use the C++ demangling capabilities in the VM. Is there 
some way to export that for use by libsaproc ?


Otherwise using C++ demangle may still be the better choice given 
we already have it as a dependency.


I found abi::__cxa_demangle() is used in ElfDecoder::demangle() at 
decoder_linux.cpp .

It is similar with my original proposal.


http://cr.openjdk.java.net/~ysuenaga/sa-demangle/


I agree with David to use C++ demangle way.
However we need to choice the fix from following:

   A. Convert LinuxDebuggerLocal.c to C++ code
   B. Add C++ code for libsaproc.so to demangle symbols.

I've discussed with Chris about it in [1].
Option A might be large change.


Thanks,

Yasumasa


[1] 
https://mail.openjdk.java.net/pipermail/serviceability-dev/2019-October/029716.html




David

On 1/11/2019 12:58 am, Chris Plummer wrote:

Hi Yasumasa,

Here's the failure during configure:

[2019-10-31T06:07:45,131Z] checking demangle.h usability... no
[2019-10-31T06:07:45,150Z] checking demangle.h presence... no
[2019-10-31T06:07:45,150Z] checking for demangle.h... no
[2019-10-31T06:07:45,151Z] configure: error: Could not find 
demangle.h! You might be able to fix this by running 'sudo yum 
install binutils-devel'.


Chris


On 10/31/19 1:08 AM, Yasumasa Suenaga wrote:

Hi,

I filed this enhancement to JBS:

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

Also I pushed the changes to submit repo, but it was failed.

  http://hg.openjdk.java.net/jdk/submit/rev/bfbc49233c26
  http://hg.openjdk.java.net/jdk/submit/rev/430e4f65ef25

According to the email from Mach 5, dependency errors were 
occurred in jib.

Can someone share the details?
I'm not familiar in jib, so I want help.

  mach5-one-ysuenaga-JDK-8233285-20191031-0606-6301426


Thanks,

Yasumasa


On 2019/10/31 11:23, Chris Plummer wrote:
You can change the configure script. I don't know if there's any 
concerns with using libiberty.a. That's possibly a legal 
question (GNU GPL). You might want to ask that on jdk-dev and/or 
build-dev.


Chris

On 10/30/19 7:14 PM, Yasumasa Suenaga wrote:

Hi Chris,

Thanks for quick reply!

If we convert LinuxDebuggerLocal.c to C++ code, we have to 
convert a lot of JNI calls to C++ style.

For example:

  (*env)->FindClass(env, "java/lang/String")
  to
  env->FindClass("java/lang/String")

Can it be accepted?

OTOH I said in my email, to use cplus_demangle(), we need to 
link libiberty.a which is provided by binutils. Thus I think we 
need to check libiberty.a in configure script. Is it ok?



I prefer to use cplus_demangle() if we can change configure 
script.



Yasumasa


On 2019/10/31 11:03, Chris Plummer wrote:

Hi Yasumasa,

I don't have concerns with adding C++ source to SA, but in 
order to do so you've put the new native code in its own file 
rather than in LinuxDebuggerLocal.c. I'd like to see that 
resolved. So either convert LinuxDebuggerLocal.c to C++, or 
use cplus_demangle().


thanks,

Chris

On 10/30/19 6:54 PM, Yasumasa Suenaga wrote:

Hi all,

I saw C++ frames in `jhsdb jstack --mixed`, and they were 
mangled as below:



0x7ff255a8fa4c 
_ZN9JavaCalls11call_helperEP9JavaValueRK12methodHandleP17JavaCallArgumentsP6Thread 
+ 0x6ac
0x7ff255a8cc1d 
_ZN9JavaCalls12call_virtualEP9JavaValueP5KlassP6SymbolS5_P17JavaCallArgumentsP6Thread 
+ 0x33d



We can demangle them via c++filt, but I think it is more 
convenience if jstack can show demangling symbols.
I think we can demangle in jstack with this patch. If it is 
accepted, I will file it to JBS and send review request.

What do you think?

http://cr.openjdk.java.net/~ysuenaga/sa-demangle/

We can get the stack as below after applying this patch:


0x7ff1aba20a4c JavaCalls::call_helper(JavaValue*, 
methodHandle const&, JavaCallArguments*, Thread*) + 0x6ac
0x7ff1aba1dc1d JavaCalls::call_virtual(JavaValue*, 
Klass*, Symbol*, Symbol*, JavaCallArguments*, Thread*) + 0x33d



I use abi::__cxa_demangle() for demangling, so this patch 
adds C++ source to SA.

If it is not comfortable, we can use cplus_demangle().
But this function is provided by libiberty.a, so we need to 
link it to libsaproc and need to check libiberty.a in 
configure script.



Thanks,

Re: RFR: 8232084: HotSpot build failed with GCC 9.2.1

[Magnus Ihse Bursie]

On 2019-10-11 15:38, Yasumasa Suenaga wrote:

Hi,

Christos commented to B-1. Thanks!
clang defines __GNU_C__ , but stringop-truncation is not supported.

So I updated Plan B-1. It works fine on my Fedora30 box.


  A. Use memcpy()
   http://cr.openjdk.java.net/~ysuenaga/JDK-8232084/webrev.02/

  B-1. Use #pragma
http://cr.openjdk.java.net/~ysuenaga/JDK-8232084/webrev.B1-pragma.02/

  C. Set -Wno-stringop-truncation in globally
   http://cr.openjdk.java.net/~ysuenaga/JDK-8232084/webrev.C/
Please note that if you chose to go with route C, you need to get this 
reviewed on build-dev (as all changes in the makefiles are). The patch 
as currently suggested is not acceptable; the built-in functions for 
testing compiler version needs to be used, as a minimum.


/Magnus



Of course I will push the change to submit repo before sending review 
request.



Thanks,

Yasumasa


On 2019/10/11 15:55, Yasumasa Suenaga wrote:

Hi,

Thanks for a lot advises!

We have following solutions for this issue.
I'd like to send RFR again with much consented patch in early next week.


   A. Use memcpy()
http://cr.openjdk.java.net/~ysuenaga/JDK-8232084/webrev.02/

   B-1. Use #pragma
http://cr.openjdk.java.net/~ysuenaga/JDK-8232084/webrev.B1-pragma/

   C. Set -Wno-stringop-truncation in globally
http://cr.openjdk.java.net/~ysuenaga/JDK-8232084/webrev.C/


If we fix with C, I will send RFR to hotspot-dev and build-dev.
Plan C also fixes other stringop-truncation problems such as 
JDK-8220074.
Thus it affects all of JDK code, but it would be useful if 
stringop-truncation

should be disabled in JDK build process.


Comments are welcome.

Yasumasa


On 2019/10/11 10:34, Yasumasa Suenaga wrote:

Hi,

I want to get conclusion of this discussion.

I understand the fix of macroAssembler_x86.hpp is ok, but we have 
not yet had conclusion

how we should fix diagnosticArgument.cpp .

I think we can fix diagnosticArgument.cpp as following:


   A. Use memcpy()
http://cr.openjdk.java.net/~ysuenaga/JDK-8232084/webrev.02/

   B. Add -Wno-stringop-truncation to 
make/hotspot/lib/JvmOverrideFiles.gmk

    This option will be added diagnosticArgument.cpp only.

   C. Set -Wno-stringop-truncation in globally
    make/hotspot/lib/CompileJvm.gmk


I prefer to fix like A because it affects minimally.
Some issues might be found out by stringop-truncation in future.


Thanks,

Yasumasa


On 2019/10/11 5:54, Kim Barrett wrote:
On Oct 10, 2019, at 3:03 AM, David Holmes 
 wrote:


On 10/10/2019 4:50 pm, Chris Plummer wrote:

 From JBS:
/home/ysuenaga/OpenJDK/jdk/src/hotspot/share/services/diagnosticArgument.cpp:154:14: 
warning: 'char* strncpy(char*, const char*, size_t)' output 
truncated before terminating nul copying as many bytes from a 
string as its length [-Wstringop-truncation]

   154 | strncpy(buf, str, len);
   | ~~~^~~
I assume this means that in all cases the "len" value is seen to 
be derived from strlen, and therefore strncpy is always copying 
one byte short of \0, and this is most likely not what the user 
wants. I seem to


Yes but we then explicitly set the NULL at buf[len] which is the 
expected/required pattern for this.


recall another recent similar fix that was done by switching to 
using memcpy instead.

Here's a discussion of interest, also suggesting memcpy:
https://stackoverflow.com/questions/50198319/gcc-8-wstringop-truncation-what-is-the-good-practice 



Seems to me that strncpy and memcpy are semantically equivalent 
here so all this does is avoid gcc's over zealous warnings. I'm 
inclined to use the:


#pragma GCC diagnostic push
#pragma GCC diagnostic ignored "-Wstringop-truncation"

solution.

YMMV.


We've run into and discussed problems with -Wstringop-truncation
before.  (See discussions of JDK-8214777 and JDK-8223186.) This is a
relatively recent warning option (introduced in gcc8, and included in
-Wall), and seems to have a considerable bug tail:

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=88781
A metabug for -Wstringop-truncation, currently with 16 open and 10
resolved associated bugs.

I'm not a fan of replacing correct and idiomatic uses of strncpy with
strcpy or memcpy.  I've suggested in the past that we should turn off
this warning while it is so buggy.

Re: RFR: 8230857: Avoid reflection in sun.tools.common.ProcessHelper

[Magnus Ihse Bursie]

On 2019-09-17 01:01, David Holmes wrote:

Hi Christoph,

Sorry for the delay getting back you.

cc'd build-dev to get some clarification on the below ...

On 12/09/2019 7:30 pm, Langer, Christoph wrote:

Hi David,


please review an enhancement which I've identified when working with
Processhelper for JDK-8230850.

I noticed that ProcessHelper is an interface in common code with a
static method that would lookup the actual platform implementation via
reflection. This seems a little cumbersome since we can have a common
dummy for ProcessHelper and override it with the platform specific
implementation, leveraging the build system.


I don't see you leveraging the build system. You have two source files
that compile to the same destination class file. What is ensuring the
platform specific version is compiled after the generic one?

Service-provider patterns use reflection to instantiate the service
implementation. I don't see any problem here that needs solving.


TL;DR:
There are two source files, one in share/classes and one in 
linux/classes. The build system overrides the share/classes 
implementation with the linux/classes implementation in the linux 
build. This is not by coincidence and only one class is contained in 
the generated jdk.jcmd module. Then there won't be a need for having 
a service interface and a service implementation that is looked up 
via reflection (which is not a bad pattern by itself). I agree that 
it's not a big problem to be solved but still not "no problem".
Here is some longer elaboration how the build system prefers specific 
implementations of classes and filters generic duplicates:
The SetupJavaCompilation function from JavaCompilation.gmk [0] is 
used to compile the java sources for JDK modules. In its 
documentation, for argument SRC [1], it claims: "one or more 
directories to search for sources. The order of the source roots is 
significant. The first found file of a certain name has priority". In 
its implementation the found files are first ordered [3] and 
duplicates filtered out [4].
The potential source files are handed to SetupJavaCompilation in 
CompileJavaModules.gmk [5] and were collected by a call to 
FindModuleSrcDirs [6].  FindModuleSrcDirs iterates over all potential 
source dirs for Java classes in the module [7]. The evaluated subdirs 
are (in that order) $(OPENJDK_TARGET_OS)/classes, 
$(OPENJDK_TARGET_OS_TYPE)/classes and share/classes, as per [8].

Hope that explains what I'm trying to leverage here.


I'm not 100% certain that what you describe actually ensures what you 
want it to ensure. I can't reconcile "the first found file ... has 
priority" with the fact found files are sorted and duplicates 
eliminated. It is the sorting that concerns me as it suggests 
linux/Foo.java might replace shared/Foo.java, but if we're on Windows 
then we have a problem! That said there is also this comment:


# Order src files according to the order of the src dirs. Correct 
odering is

# needed for correct overriding between different source roots.

I'd need the build team to clarify what "correct overriding" is 
actually defined as.

David,

Christoph is correct. linux/Foo.java will override share/Foo.java. I 
don't remember how the magic in JavaCompilation.gmk works anymore :-), 
but we have relied on this behavior in other places for a long time, so 
I'm pretty certain it is still working correctly. Presumably, the $(sort 
...) is there to remove (identical) duplicates, which is a side-effect 
of sort.


/Magnus



Thanks,
David
-

I've uploaded an updated webrev which contains some cleanup to the 
Test changes: http://cr.openjdk.java.net/~clanger/webrevs/8230857.1/


Thanks
Christoph

[0] 
http://hg.openjdk.java.net/jdk/jdk/file/ea93d6a9f720/make/common/JavaCompilation.gmk#l185
[1] 
http://hg.openjdk.java.net/jdk/jdk/file/ea93d6a9f720/make/common/JavaCompilation.gmk#l157
[3] 
http://hg.openjdk.java.net/jdk/jdk/file/ea93d6a9f720/make/common/JavaCompilation.gmk#l225
[4] 
http://hg.openjdk.java.net/jdk/jdk/file/ea93d6a9f720/make/common/JavaCompilation.gmk#l257
[5] 
http://hg.openjdk.java.net/jdk/jdk/file/ea93d6a9f720/make/CompileJavaModules.gmk#l603
[6] 
http://hg.openjdk.java.net/jdk/jdk/file/ea93d6a9f720/make/CompileJavaModules.gmk#l555
[7] 
http://hg.openjdk.java.net/jdk/jdk/file/ea93d6a9f720/make/common/Modules.gmk#l300
[8] 
http://hg.openjdk.java.net/jdk/jdk/file/ea93d6a9f720/make/common/Modules.gmk#l243

Re: [12] RFR for JDK-8214122: JDWP is broken on 32 bit Windows: transport library missing onLoad entry

[Magnus Ihse Bursie]

On 2018-12-12 17:38, Alexey Ivanov wrote:

Hi all,

I have updated the summary of JDK-8214122 and the subject accordingly.

Could you please review the following fix?

https://bugs.openjdk.java.net/browse/JDK-8214122
webrev: http://cr.openjdk.java.net/~aivanov/8214122/webrev.01/

Looks good to me.

/Magnus


*Root cause*
jdwpTransport_OnLoad is exported as _jdwpTransport_OnLoad@16 on 32 bit 
Windows according to the name decorations [1] for __stdcall [2] 
calling conventions.


*Fix*
On 32 bit Windows, look up the decorated name, 
_jdwpTransport_OnLoad@16, first; if not found, look up the undecorated 
name, jdwpTransport_OnLoad.



Regards,
Alexey

[1] 
https://docs.microsoft.com/en-us/cpp/build/reference/decorated-names?view=vs-2017#FormatC

[2] https://docs.microsoft.com/en-ie/cpp/cpp/stdcall?view=vs-2017

On 12/12/2018 11:19, Magnus Ihse Bursie wrote:



On 2018-12-11 18:16, Alexey Ivanov wrote:

Hi Simon,

Thank you for your comments.

The updated webrev:
http://cr.openjdk.java.net/~aivanov/8214122/webrev.01/

Indeed, it looks much cleaner.

Yes! This seems like the correct fix.

Ship it! :)

/Magnus



Regards,
Alexey

On 11/12/2018 16:43, Simon Tooke wrote:

On 2018-12-11 10:05 a.m., Alexey Ivanov wrote:

Hi everyone,

I came up with the following patch:
http://cr.openjdk.java.net/~aivanov/8214122/webrev.00/

It specifically addresses the problem in JDK-8214122 where on 32 bit
Windows jdwpTransport_OnLoad can exported with its plain and
__stdcall-mangled name. I used conditional compilation so that for
other platforms the code remains as it is now.

jshell starts successfully with this fix; an IDE debugger works as 
well.



I am not a reviewer, but this patch only works for the specific case
under discussion; the '@16' refers to the reserved stack size in the
Pascal calling convention.  So, the patch only works for 16 bytes of
parameters.  To be generic, the routine needs to have the stack size
passed in by the caller.  If a generic fix isn't desired (and I 
hope it

is), I'd prefer to see the caller simply pass the decorated or
undecorated name depending on the Win32/64 defines.

 #if defined(_WIN32) && !defined(_WIN64) onLoad =
 (jdwpTransport_OnLoad_t) dbgsysFindLibraryEntry(handle,
 "_jdwpTransport_OnLoad@16"); #else onLoad = 
(jdwpTransport_OnLoad_t)

 dbgsysFindLibraryEntry(handle, "jdwpTransport_OnLoad"); #endif


Thanks,
-Simon


Regards,
Alexey

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

On 10/12/2018 15:11, Magnus Ihse Bursie wrote:
Since removing JNICALL is not an option, there are only two 
options:


1. Add |/export| option to the Makefile or pragma-comment to the
source file;
2. Lookup the decorated name |_jdwpTransport_OnLoad@16| for Win32
with fallback to undecorated one.

Yes.

I think the correct solution here is 2.

Re: [PATCH] Windows 32-bit DLL name decoration

[Magnus Ihse Bursie]

On 2018-12-12 12:35, Alan Bateman wrote:

On 12/12/2018 11:14, Magnus Ihse Bursie wrote:

:

I searched the code for "jdwpTransport_On" to see of there was any 
corresponding OnUnload function (or other), but could not find any.
That's right, an unload wasn't needed when that SPI was originally 
added but could be added in the event that some future debugger agent 
need it. The SPI is a supported/documented JDK-specific mechanism, its 
"spec" is hosted here:


https://docs.oracle.com/en/java/javase/11/docs/specs/jdwp/jdwp-transport.html 

... yet in all that time, we have not fully supported the spec on 
Windows 32. :-( (We never discovered this because of lack of testing, I 
presume, and that our internal usage empoyed a questonable workaround.)




I see this thread is touching on the functions exported by libjli. 
This library is part of the launcher and shouldn't be used directly by 
anything outside of the JDK. However we have to be careful because 
JavaFX `javapackager` tool has been using it. There's a JEP to bring a 
similar tool, jpackage in the current proposal, that will supersede it 
but in the mean time we need to be careful not to break it. A second 
issue is that the libjli is listed in the property list (Info.plist) 
on macOS. This came from Apple in 7u4 and periodically things show up 
that are using it, e.g. that recent breakage with Eclipse that was 
never fully diagnosed but we think it was using Info.plist.
The latest patch, as suggested, will not modify JLI, but instead fix the 
broken behaviour of JDWP on Windows 32.


/Magnus

Re: [PATCH] Windows 32-bit DLL name decoration

[Magnus Ihse Bursie]

On 2018-12-11 18:16, Alexey Ivanov wrote:

Hi Simon,

Thank you for your comments.

The updated webrev:
http://cr.openjdk.java.net/~aivanov/8214122/webrev.01/

Indeed, it looks much cleaner.

Yes! This seems like the correct fix.

Ship it! :)

/Magnus



Regards,
Alexey

On 11/12/2018 16:43, Simon Tooke wrote:

On 2018-12-11 10:05 a.m., Alexey Ivanov wrote:

Hi everyone,

I came up with the following patch:
http://cr.openjdk.java.net/~aivanov/8214122/webrev.00/

It specifically addresses the problem in JDK-8214122 where on 32 bit
Windows jdwpTransport_OnLoad can exported with its plain and
__stdcall-mangled name. I used conditional compilation so that for
other platforms the code remains as it is now.

jshell starts successfully with this fix; an IDE debugger works as 
well.



I am not a reviewer, but this patch only works for the specific case
under discussion; the '@16' refers to the reserved stack size in the
Pascal calling convention.  So, the patch only works for 16 bytes of
parameters.  To be generic, the routine needs to have the stack size
passed in by the caller.  If a generic fix isn't desired (and I hope it
is), I'd prefer to see the caller simply pass the decorated or
undecorated name depending on the Win32/64 defines.

 #if defined(_WIN32) && !defined(_WIN64) onLoad =
 (jdwpTransport_OnLoad_t) dbgsysFindLibraryEntry(handle,
 "_jdwpTransport_OnLoad@16"); #else onLoad = 
(jdwpTransport_OnLoad_t)

 dbgsysFindLibraryEntry(handle, "jdwpTransport_OnLoad"); #endif


Thanks,
-Simon


Regards,
Alexey

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

On 10/12/2018 15:11, Magnus Ihse Bursie wrote:

Since removing JNICALL is not an option, there are only two options:

1. Add |/export| option to the Makefile or pragma-comment to the
source file;
2. Lookup the decorated name |_jdwpTransport_OnLoad@16| for Win32
with fallback to undecorated one.

Yes.

I think the correct solution here is 2.

Re: [PATCH] Windows 32-bit DLL name decoration

[Magnus Ihse Bursie]

On 2018-12-11 18:22, Bob Vandette wrote:

Hotspot has had support for decorated and non-decorated names for the JNI_OnLoad
functions.  Perhaps you should just follow the same procedure for the debug
library.

#define JNI_ONLOAD_SYMBOLS   {"_JNI_OnLoad@8", "JNI_OnLoad"}
#define JNI_ONUNLOAD_SYMBOLS {"_JNI_OnUnload@8", "JNI_OnUnload"}
#define JVM_ONLOAD_SYMBOLS  {"_JVM_OnLoad@12", "JVM_OnLoad"}
#define AGENT_ONLOAD_SYMBOLS{"_Agent_OnLoad@12", "Agent_OnLoad"}
#define AGENT_ONUNLOAD_SYMBOLS  {"_Agent_OnUnload@4", "Agent_OnUnload"}
#define AGENT_ONATTACH_SYMBOLS  {"_Agent_OnAttach@12", “Agent_OnAttach”}
Yes, that sounds mostly reasonable. The latest iteration of the patch is 
doing just this.


I searched the code for "jdwpTransport_On" to see of there was any 
corresponding OnUnload function (or other), but could not find any. But 
if there are other *_OnEvent() functions in the JDK code, they should be 
fixed too to handle decorated names. To the best of my knowledge, 
jdwpTransport_OnLoad was the only such *_OnEvent function that did not 
handle decorated names. I searched the code base for the pattern 
'[a-zA-Z]+_On[A-Z][a-zA-Z]*' and skimmed the result, but could not find 
anything else apart from those listed by you above, and the 
jdwpTransport_OnLoad. Any knowledge of additional such functions but 
with different names must come from the component teams.


/Magnus


Bob.



On Dec 11, 2018, at 11:43 AM, Simon Tooke  wrote:

On 2018-12-11 10:05 a.m., Alexey Ivanov wrote:

Hi everyone,

I came up with the following patch:
http://cr.openjdk.java.net/~aivanov/8214122/webrev.00/

It specifically addresses the problem in JDK-8214122 where on 32 bit
Windows jdwpTransport_OnLoad can exported with its plain and
__stdcall-mangled name. I used conditional compilation so that for
other platforms the code remains as it is now.

jshell starts successfully with this fix; an IDE debugger works as well.


I am not a reviewer, but this patch only works for the specific case
under discussion; the '@16' refers to the reserved stack size in the
Pascal calling convention.  So, the patch only works for 16 bytes of
parameters.  To be generic, the routine needs to have the stack size
passed in by the caller.  If a generic fix isn't desired (and I hope it
is), I'd prefer to see the caller simply pass the decorated or
undecorated name depending on the Win32/64 defines.

#if defined(_WIN32) && !defined(_WIN64) onLoad =
(jdwpTransport_OnLoad_t) dbgsysFindLibraryEntry(handle,
"_jdwpTransport_OnLoad@16"); #else onLoad = (jdwpTransport_OnLoad_t)
dbgsysFindLibraryEntry(handle, "jdwpTransport_OnLoad"); #endif


Thanks,
-Simon


Regards,
Alexey

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

On 10/12/2018 15:11, Magnus Ihse Bursie wrote:

Since removing JNICALL is not an option, there are only two options:

1. Add |/export| option to the Makefile or pragma-comment to the
source file;
2. Lookup the decorated name |_jdwpTransport_OnLoad@16| for Win32
with fallback to undecorated one.

Yes.

I think the correct solution here is 2.

Re: [PATCH] Windows 32-bit DLL name decoration

[Magnus Ihse Bursie]

On 2018-12-10 16:02, Alexey Ivanov wrote:



On 10/12/2018 10:41, Magnus Ihse Bursie wrote:



On 2018-12-07 22:18, Simon Tooke wrote:

Looking at the code, it seems (to me) the "correct" thing to do is to is
to create a Windows-specific version of dbgsysBuildFunName() in
linker_md.c, and have it decorate the function name as appropriate for
32-bit windows.  Note that in transport.c, you'd need to pass in the
parameter stack size (the bit after the @), but it could at least behave
properly on 32 vs 64 bit Windows.

Notice this approach is already used for the library name, via
dbgsysBuildLibName()

If the function is never intended to be called by Java via JNI, then it
should never have been declared JNICALL in the first place - but that
ship has sailed.

I don't think changing the decorated exported name to undecorated is a
good idea, as it obfuscates the calling convention used.

Good analysis, Simon. I agree.

I have now looked more into the situation. In fact, it looks a lot 
like JDWP has been broken on Windows 32-bit for a long time, but we 
have patched around the issue in the JDK by using /export. This is 
the spec we're dealing with: 
https://docs.oracle.com/javase/10/docs/specs/jdwp/jdwp-transport.html. 
I quote:


The transport library must export an/onload/function with the 
following prototype:


|JNIEXPORT jint JNICALL jdwpTransport_OnLoad(JavaVM *jvm, 
jdwpTransportCallback *callback, jint version, jdwpTransportEnv** env);|


This function will be called by the JDWP (or other agent) when the 
library is loaded.





Nothing here says anything that the function should be exported using 
anything other than the normal _stdcall (implied by JNICALL) name 
mangling rules. However, JDWP has not been working according to the 
spec and looking for the correct symbol, and while we could have 
noticed that in the JDK, we didn't, because someone (long ago) added 
/export:jdwpTransport_OnLoad as a workaround to the affected 
libraries, instead of fixing JDWP.


This means that we cannot change the calling convention: it must stay 
as it is now.


I think JDWP has always been working according to the spec. Using 
__stdcall is the recommended calling convention for Win32 and for DLLs 
in particular. Usually function names are exported undecorated in DLL. 
(If my memory serves me well, older Microsoft tools exported |extern 
"C" __stdcall| functions undecorated.)
So then the question becomes: what does the spec mean? That the 
__stdcall convention should be used, or that the function name should be 
explicitly exported under a non __stdcall-convention name? Neither 
behavior seems clearly written out, so this is left to an implicit 
interpretation of what that "usually" means..?


I believe this — exporting the undecorated function names — allows for 
interoperability between 32 bit and 64 bit in cases where you load a 
DLL and dynamically look up a function in it.


There were too many /export options to export Win32 functions 
undecorated for this one to be an accident rather than the intention.

How do you mean?


Now, given that we've shipped code that didn't adhere to the specs 
for so long, we can probably not break that behavior. Instead, I 
propose we amend dbgsysBuildFunName() so that on Windows 32-bit, it 
looks for both the "jdwpTransport_OnLoad", and the symbol as mangled 
by _stdcall rules. I also propose that if both symbols are present, 
the _stdcall named one should take precedence, since that's according 
to the spec (and the other is just a fallback to maintain backwards 
compatibility).


Since removing JNICALL is not an option, there are only two options:

1. Add |/export| option to the Makefile or pragma-comment to the 
source file;
2. Lookup the decorated name |_jdwpTransport_OnLoad@16| for Win32 with 
fallback to undecorated one.

Yes.

I think the correct solution here is 2.



I just wonder how it's possible to implement a generic 
|dbgsysBuildFunName| for an arbitrary function without knowing the 
size of function parameters. Could it be the reason why most DLLs 
export __stdcall functions undecorated?
(I assume you mean dbgsysFindLibraryEntry; there is a dbgsysBuildFunName 
as well but it appears to be dead code.)


The dbgsysFindLibraryEntry does not need to work with an arbitrary 
function. It's implemented in jdk.jdwp.agent, for the sole reason of 
locating the jdwpTransport_OnLoad function.


In general, I assume this to hold true. If you don't know the signature 
of the function you want to call, you're screwed anyway, regardless of 
calling convention.


/Magnus



Regards,
Alexey



/Magnus

-Simon Tooke


On 12/7/2018 2:09 PM, Alexey Ivanov wrote:

Hi Andrew,

Sorry for my belated reply.
Yes, it's also an option… however, this solution seems to be
overcomplicated to export one function or two. The calling convention
of exported functions for JVM cannot be changed, they can be called
from third-party software.

If the funct

Re: [PATCH] JDK-8214122 Prevent name mangling of jdwpTransport_OnLoad in Windows 32-bit DLL name decoration

[Magnus Ihse Bursie]

On 2018-12-07 21:24, Alexey Ivanov wrote:

Hi Ali,

On 06/12/2018 22:49, Ali İnce wrote:

Hi Magnus, Alexey,

I believe we won’t be able to get further opinions from 
serviceability-dev.


Unfortunately, no one has replied as of now.
Have you found any issues with jdwpTransport_OnLoad after removing 
JNICALL?


Andrew Luo suggested using a similar mechanism as is used for jvm.dll 
by using symbol name files mapped by platform (files are under 
make/hotspot/symbols and interestingly windows is almost the only 
platform for which a symbol file doesn’t exist).


Andrew says the .def files are auto-generated for Windows. There's a 
set of shared functions.
JVM cannot change the calling convention, jvm.dll is the public 
interface to JVM.


Another issue, again opened against AdoptOpenJDK 32-bit builds is 
somehow related with the same problem - but this time it is 
jimage.dll depending on zip.dll expecting the ZIP_InflateFully 
function to be decorated with JNICALL - whereas it was removed 
earlier from the implementation and the runtime image just fails with 
access violation just because jimage source code still declared it 
with JNICALL (https://github.com/AdoptOpenJDK/openjdk-build/issues/763).


The usage of ZIP_InflateFully from zip.dll by jimage.dll was 
overlooked during work on 
https://bugs.openjdk.java.net/browse/JDK-8201226.


I can take care of it.
It might be worthwhile to scan the entire code base for JNICALL and 
verify that we have no more mismatches. In general, JNICALL should be 
preserved on all officially supported, exported interfaces. It need not 
be present on interfaces used only internally, and my current thinking 
is that it would be better if it were removed on all internal 
interfaces. But at the very least, it should be consistently specificed 
where exported and imported. (Any misses here is probably due to 
duplicate declarations, instead of properly including header files, so 
that's the correct way to resolve any problems found...)


/Magnus

(I could not build 32 bit Windows. It seem to have overcome the 
problem by adding --disable-warnings-as-errors option to configure.)


However, the report says the resulting image crashes in 64 bit windows 
too which shouldn't be affected by JNICALL mismatch.


I’ve also searched for GetProcAddress calls across the source code - 
and I think it’s important to work on the consistency of JNICALL 
usages across the whole source code.


There are many places where libraries are loaded dynamically or a 
function may be unavailable on older versions of the platform.
Have you identified any other place where functions from JDK DLLs are 
looked up by names?


Another noteworthy thing I’ve noticed is there are still JNICALL 
modifiers for example in 
https://github.com/AdoptOpenJDK/openjdk-jdk11u/blob/master/src/jdk.crypto.cryptoki/windows/native/libj2pkcs11/j2secmod_md.c#L49 - 
which I guess will also be reported as broken since these functions 
are again name decorated.


This is a JNI method. It should use JNICALL modifier. If it wasn't 
found, the class sun.security.pkcs11.Secmod would fail to load. I 
guess JVM handles name mangling somehow for native method implementation.


Such functions were never explicitly exported using mapfiles or 
/export options on Windows, at least in the client area. For Linux and 
Solaris, adding or removing a native method required editing mapfiles.


If we’re still determined to remove JNICALL, what about just removing 
__stdcall from JNICALL declaration at 
https://github.com/AdoptOpenJDK/openjdk-jdk11u/blob/master/src/java.base/windows/native/include/jni_md.h#L31 ? Wouldn’t 
that be a more consistent move?


We can't do that.
Implementation of Java native methods must use __stdcall calling 
convention.




Regards,

Ali

On 22 Nov 2018, at 17:29, Magnus Ihse Bursie 
<mailto:magnus.ihse.bur...@oracle.com>> wrote:


I think we are in complete agreement. What's missing is some expert 
opinion from serviceability-dev if there is any problem with 
changing the signature. I'd preferably have that.


If no one knows, I'd say, change it, and see if we still pass the 
relevant tests, and if so, ship it.


/Magnus

22 nov. 2018 kl. 18:04 skrev Alexey Ivanov <mailto:alexey.iva...@oracle.com>>:




On 21/11/2018 12:16, Magnus Ihse Bursie wrote:


On 2018-11-20 16:49, Alexey Ivanov wrote:


Hi Ali, Magnus,

I absolutely agree this change has to be reviewed by someone from 
serviceability.


There are several options:

1. Add -export:jdwpTransport_OnLoad to LDFLAGS for Windows
http://mail.openjdk.java.net/pipermail/build-dev/2018-November/023935.html
so it partially reverts the changes from
https://bugs.openjdk.java.net/browse/JDK-8200178

2. Remove JNICALL (__stdcall) modifier, eventually changing the 
calling convention to __cdecl.

http://mail.openjdk.java.net/pipermail/build-dev/2018-November/023969.html

3. Use inline /export option via #pragma:
#pragma comment(linker, "/expo

Re: RFR (round 4), JDK-8214259: Implementation: JEP 189: Shenandoah: A Low-Pause Garbage Collector

[Magnus Ihse Bursie]

> 3 dec. 2018 kl. 20:27 skrev Roman Kennke :
> 
> Round 5 of Shenandoah review includes:
> - A fix for the @requires tag in TestFullGCCountTest.java. It should be
> correct now. We believe the CMS @requires was also not quite right and
> fixed it the same.
> 
> It reads now: Don't run this test if:
> - Actual GC set by harness is CMS *and* ExplicitGCInvokesConcurrent is
> true, as set by harness
> - Actual GC set by harness is Shenandoah *and*
> ExplicitGCInvokesConcurrent is not set false by harness (it's true by
> default in Shenandoah, so this needs to be double-inverteed).
> 
> The @requires for CMS was wrong before (we think), because it would also
> filter defaultGC + ExplicitGCInvokesConcurrent.
> 
> - Sorting of macros was fixed, as was pointed out by Per
> - Some stuff was added to SA, as suggested by Jini
> - Rebased on most current jdk/jdk code
> 
> Webrevs:
> http://cr.openjdk.java.net/~rkennke/shenandoah-upstream/05/
> 
> I also need reviews from GC reviewers for the CSR:
> https://bugs.openjdk.java.net/browse/JDK-8214349
> 
> I already got reviews for:
> [x] shared-runtime (coleenp)
> [x] shared-compiler (kvn)
> 
> I got reviews for shared-build, but an earlier version, so maybe makes
> sense to look over this again. Erik J, Magnus?

Build changes look good. 

/Magnus

> 
> I still need approvals for:
> [ ] shared-build  (kvn, erikj, ihse, pliden)
> [ ] shared-gc (pliden, kbarrett)
> [ ] shared-serviceability (jgeorge, pliden)
> [ ] shared-tests  (lmesnik, pliden)
> [ ] shenandoah-gc
> [ ] shenandoah-tests
> 
> 
> Thanks for your patience and ongoing support!
> 
> Cheers,
> Roman
> 
>> Hi all,
>> 
>> here comes round 4 of Shenandoah upstreaming review:
>> 
>> This includes fixes for the issues that Per brought up:
>> - Verify and gracefully reject dangerous flags combinations that
>> disables required barriers
>> - Revisited @requires filters in tests
>> - Trim unused code from Shenandoah's SA impl
>> - Move ShenandoahGCTracer to gc/shenandoah
>> - Fix ordering of GC names in various files
>> - Rename UINT64_FORMAT_HEX_W to UINT64_FORMAT_X_W
>> 
>> http://cr.openjdk.java.net/~rkennke/shenandoah-upstream/04/
>> 
>> Thanks everybody for taking time to review this!
>> Roman
>> 
>>> Hello all,
>>> 
>>> Thanks so far for all the reviews and support!
>>> 
>>> I forgot to update the 'round' yesterday. We are in round 3 now :-)
>>> Also, I fixed the numbering of my webrevs to match the review-round. ;-)
>>> 
>>> Things we've changed today:
>>> - We moved shenandoah-specific code out of .ad files into our own .ad
>>> files under gc/shenandoah (in shenandoah-gc), how cool is that? This
>>> requires an addition in build machinery though, see
>>> make/hotspot/gensrc/GensrcAdlc.gmk (in shared-build).
>>> - Improved zero-disabling and build-code-simplification as suggested by
>>> Magnus and Per
>>> - Cleaned up some leftovers in C2
>>> - Improved C2 loop opts code by introducing another APIs in
>>> BarrierSetC2. See the new APIs in shared-gc under BarrierSetC2.hpp.
>>> - I don't see where it makes sense to put INCLUDE_SHENANDOAHGC guards now.
>>> - We would all very much prefer to keep ShenandoahXYZNode names, as
>>> noted earlier. This stuff is Shenandoah-specific, so let's just call it
>>> that.
>>> - Rehashed Shenandoah tests (formatting, naming, directory layout, etc)
>>> - Rebased on jdk-12+22
>>> 
>>> - Question: let us know if you need separate RFE for the new BSC2 APIs?
>>> 
>>> http://cr.openjdk.java.net/~rkennke/shenandoah-upstream/03/
>>> 
>>> Thanks,
>>> Roman
>>> 
 Alright, we fixed:
 - The minor issues that Kim reported in shared-gc
 - A lot of fixes in shared-tests according to Leonid's review
 - Disabled SA heapdumping similar to ZGC as Per suggested
 
 Some notes:
 Leonid:  test/hotspot/jtreg/gc/TestFullGCCount.java was actually
 correct. The @requires there means to exclude runs with both CMS and
 ExplicitGCInvokesConcurrent at the same time, because that would be
 (expectedly) failing. It can run CMS, default GC and any other GC just
 fine. Adding the same clause for Shenandoah means the same, and filters
 the combination (+UseShenandoahGC)+(+ExplicitGCInvokesConcurrent). I
 made the condition a bit clearer by avoiding triple-negation.
 
 See:
 http://mail.openjdk.java.net/pipermail/shenandoah-dev/2018-November/008457.html
 
 Per: Disabling the SA part for heapdumping makes 2 tests fail:
 - test/hotspot/jtreg/serviceability/sa/ClhsdbJhisto.java
 - test/hotspot/jtreg/serviceability/sa/TestHeapDumpForLargeArray.java
 
 we filter them for Shenandoah now. I'm wondering: how do you get past
 those with ZGC?
 
 See:
 http://mail.openjdk.java.net/pipermail/shenandoah-dev/2018-November/008466.html
 
 (Note to Leonid and tests reviewers: I'll add those related filters in
 next round).
 
 Vladimir: Roland integrated a bunch of changes to make 

Re: RFR (round 1), JDK-8214259: Implementation: JEP 189: Shenandoah: A Low-Pause Garbage Collector

[Magnus Ihse Bursie]

On 2018-11-26 23:47, Erik Joelsson wrote:


Build changes look ok to me.
I agree, but I'd like to point out a (possible) style improvement. In 
hotspot.m4,


if test "x$OPENJDK_TARGET_CPU" = "xx86_64" || test 
"x$OPENJDK_TARGET_CPU" = "xaarch64" || test "x$OPENJDK_TARGET_CPU" == 
"xx86"; then


can be more succinctly expressed as

if test "x$OPENJDK_TARGET_CPU_ARCH" = "xx86" || test 
"x$OPENJDK_TARGET_CPU" = "xaarch64" ; then



But it's a matter of preference, if you do not want to change. Otherwise 
it looks good!


/Magnus


/Erik

On 2018-11-26 13:39, Roman Kennke wrote:

Hi,

This is the first round of changes for including Shenandoah GC into
mainline.
I divided the review into parts that roughly correspond to the 
mailing lists

that would normally review it, and I divided it into 'shared' code
changes and
'shenandoah' code changes (actually, mostly additions). The intend is to
eventually
push them as single 'combined' changeset, once reviewed.

JEP:
   https://openjdk.java.net/jeps/189
Bug entry:

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

Webrevs:
   http://cr.openjdk.java.net/~rkennke/shenandoah-upstream/00/

For those who want to see the full change, have a look at the
shenandoah-complete
 


directory,
it contains the full combined webrev. Alternatively, there is the file
shenandoah-master.patch
, 


which is what I intend to commit (and which should be equivalent to the
'shenandoah-complete' webrev).

Sections to review (at this point) are the following:
  *) shenandoah-gc
 


 - Actual Shenandoah implementation, almost completely residing in
gc/shenandoah

  *) shared-gc

 - This is mostly boilerplate that is common to any GC
 - referenceProcessor.cpp has a little change to make one assert not
fail (next to CMS and G1)
 - taskqueue.hpp has some small adjustments to enable subclassing

  *) shared-serviceability
 


 - The usual code to support another GC

  *) shared-runtime
 


 - A number of friends declarations to allow Shenandoah iterators to
hook up with,
   e.g. ClassLoaderData, CodeCache, etc
 - Warning and disabling JFR LeakProfiler
 - fieldDescriptor.hpp added is_stable() accessor, for use in
Shenandoah C2 optimizations
 - Locks initialization in mutexLocker.cpp as usual
 - VM operations defines for Shenandoah's VM ops
 - globalDefinitions.hpp added UINT64_FORMAT_HEX_W for use in
Shenandoah's logging
 - The usual macros in macro.hpp

  *) shared-build
 


 - Add shenandoah feature, enabled by default, as agreed with
Vladimir K. beforehand
 - Some flags for shenandoah-enabled compilation to get
SUPPORT_BARRIER_ON_PRIMITIVES
   and SUPPORT_NOT_TO_SPACE_INVARIANT which is required for
Shenandoah's barriers
 - --param inline-unit-growth=1000 settings for 2 shenandoah source
files, which is
   useful to get the whole marking loop inlined (observed 
significant

regression if we
   don't)

  *) shared-tests
 


 - Test infrastructure to support Shenandoah
 - Shenandoah test groups
 - Exclude Shenandoah in various tests that can be run with 
selected GC

 - Enable/add configure for Shenandoah for tests that make sense to
run with it

  *) shenandoah-tests
 

 - Shenandoah specific tests, most reside in gc/shenandoah 
subdirectory

 - A couple of tests configurations have been added, e.g.
TestGCBasherWithShenandoah.java

I intentionally left out shared-compiler for now, because we have some
work left to do
there, but if you click around you'll find the patch anyway, in case you
want to take
a peek at it.

We have regular builds on:
   - {Linux} x {x86_64, x86_32, armhf, aarch64, ppc64el, s390x}
   - {Windows} x {x86_64},
   - {MacOS X} x {x86_64}

This also routinely passes:
   - the new Shenandoah tests
   - jcstress with/without aggressive Shenandoah verification
   - specjvm2008 with/without aggressive Shenandoah verification


I'd like to thank my collegues at Red Hat: Christine Flood, she deserves
the credit for being the original inventor of Shenandoah, Aleksey
Shiplëv, Roland Westrelin & Zhengyu Gu for their countless
contributions, everybody else in Red Hat's OpenJDK team for testing,
advice and support, my collegues in Oracle's GC, runtime and compiler
teams for tirelessly helping with and 

Re: [PATCH] JDK-8214122 Prevent name mangling of jdwpTransport_OnLoad in Windows 32-bit DLL name decoration

[Magnus Ihse Bursie]

I think we are in complete agreement. What's missing is some expert opinion 
from serviceability-dev if there is any problem with changing the signature. 
I'd preferably have that. 

If no one knows, I'd say, change it, and see if we still pass the relevant 
tests, and if so, ship it. 

/Magnus

> 22 nov. 2018 kl. 18:04 skrev Alexey Ivanov :
> 
> 
>> On 21/11/2018 12:16, Magnus Ihse Bursie wrote:
>>> On 2018-11-20 16:49, Alexey Ivanov wrote:
>>> Hi Ali, Magnus,
>>> 
>>> I absolutely agree this change has to be reviewed by someone from 
>>> serviceability.
>>> 
>>> There are several options:
>>> 
>>> 1. Add -export:jdwpTransport_OnLoad to LDFLAGS for Windows
>>> http://mail.openjdk.java.net/pipermail/build-dev/2018-November/023935.html
>>> so it partially reverts the changes from
>>> https://bugs.openjdk.java.net/browse/JDK-8200178
>>> 
>>> 2. Remove JNICALL (__stdcall) modifier, eventually changing the calling 
>>> convention to __cdecl.
>>> http://mail.openjdk.java.net/pipermail/build-dev/2018-November/023969.html
>>> 
>>> 3. Use inline /export option via #pragma:
>>> #pragma comment(linker, "/export:jdwpTransport_OnLoad= 
>>> _jdwpTransport_OnLoad@16")
>>> as referenced in
>>> https://docs.microsoft.com/en-ie/cpp/cpp/dllexport-dllimport?view=vs-2017
>>> https://docs.microsoft.com/en-ie/cpp/build/reference/exports?view=vs-2017
>>> 
>>> The third options still needs to be tested to make sure it does not break 
>>> other platforms builds.
>> I'm not fond of either solution 1 or 3. I really think the signature should 
>> be made correctly at the point of definition in the source code.
> 
> That is why I proposed removing JNICALL (__stdcall) from the function 
> declaration as we had done for libzip, libjimage [1] and mlib_image [2].
> 
> Microsoft recommends using __stdcall for DLLs, at the same time it decorates 
> the function name making it harder to export it by its plain name.
> 
> 
> By removing JNICALL / __stdcall, we make the function use __cdecl calling 
> convention. The difference between them is that with __stdcall the callee 
> cleans the stack whereas with __cdecl the caller cleans the stack.
> 
> It shouldn't be a problem as long as all function declarations use the same 
> calling convention, which, I believe, is the case here.
> 
>> We've spent some considerable effort of getting rid of the /export flags for 
>> Windows (and map files for unix), and I don't want to see them creep back 
>> in. Note that option 3 is just option 1 in disguise. The only single good 
>> thing about it is that it allows you to put the compiler option next to the 
>> signature declaration in the source code.
> 
> At least in this case, the option will be near the function body definition. 
> It will be easier to update it if the signature changes.
> 
> If we are to use __stdcall, it seems to be the only way to export the 
> undecorated name.
> 
>> The same goes for def files, as suggested by Ali. It's just yet another 
>> version of option 1 in disguise/map files.
> 
> If option 2 is rejected, I'm for using option 3. If option 3 cannot be used 
> too, I'm for option 1.
> I think we should not fall back to def files in any case. And Makefile will 
> have to include the reference to the def file, so it's even worse than option 
> 1.
> 
> 
> Regards,
> Alexey
> 
> [1] https://bugs.openjdk.java.net/browse/JDK-8201226
> [2] https://bugs.openjdk.java.net/browse/JDK-8202476
>> 
>> /Magnus
>> 
>>> 
>>> 
>>> Regards,
>>> Alexey
>>> 
>>>> On 19/11/2018 12:19, Magnus Ihse Bursie wrote:
>>>> Hi Ali,
>>>> 
>>>> From a build perspective this change looks OK. I'm not aware of the finer 
>>>> details on the OnLoad mechanism, like what calling convention is to be 
>>>> used. So maybe this is a no-go from that view. 
>>>> 
>>>> I'm cc:ing servicability so they can have a look at it.
>>>> 
>>>> /Magnus
>>>> 
>>>>> On 2018-11-18 13:07, Ali İnce wrote:
>>>>> Hi Alexey,
>>>>> 
>>>>> Below is a new patch content that removes JNICALL modifiers from the 
>>>>> exported functions of interest. This results in exported functions not to 
>>>>> be name decorated and use __cdecl calling convention.
>>>>> 
>>>>> Do you have any more suggestions, or would you please guide me on next 
>>>>> steps?

Re: [PATCH] JDK-8214122 Prevent name mangling of jdwpTransport_OnLoad in Windows 32-bit DLL name decoration

[Magnus Ihse Bursie]

On 2018-11-20 16:49, Alexey Ivanov wrote:


Hi Ali, Magnus,

I absolutely agree this change has to be reviewed by someone from 
serviceability.


There are several options:

1. Add -export:jdwpTransport_OnLoad to LDFLAGS for Windows
http://mail.openjdk.java.net/pipermail/build-dev/2018-November/023935.html
so it partially reverts the changes from
https://bugs.openjdk.java.net/browse/JDK-8200178

2. Remove JNICALL (__stdcall) modifier, eventually changing the 
calling convention to __cdecl.

http://mail.openjdk.java.net/pipermail/build-dev/2018-November/023969.html

3. Use inline /export option via #pragma:
#pragma comment(linker, "/export:jdwpTransport_OnLoad= 
_jdwpTransport_OnLoad@16")

as referenced in
https://docs.microsoft.com/en-ie/cpp/cpp/dllexport-dllimport?view=vs-2017
https://docs.microsoft.com/en-ie/cpp/build/reference/exports?view=vs-2017

The third options still needs to be tested to make sure it does not 
break other platforms builds.
I'm not fond of either solution 1 or 3. I really think the signature 
should be made correctly at the point of definition in the source code. 
We've spent some considerable effort of getting rid of the /export flags 
for Windows (and map files for unix), and I don't want to see them creep 
back in. Note that option 3 is just option 1 in disguise. The only 
single good thing about it is that it allows you to put the compiler 
option next to the signature declaration in the source code.


The same goes for def files, as suggested by Ali. It's just yet another 
version of option 1 in disguise/map files.


/Magnus




Regards,
Alexey

On 19/11/2018 12:19, Magnus Ihse Bursie wrote:

Hi Ali,

From a build perspective this change looks OK. I'm not aware of the 
finer details on the OnLoad mechanism, like what calling convention 
is to be used. So maybe this is a no-go from that view.


I'm cc:ing servicability so they can have a look at it.

/Magnus

On 2018-11-18 13:07, Ali İnce wrote:

Hi Alexey,

Below is a new patch content that removes JNICALL modifiers from the exported 
functions of interest. This results in exported functions not to be name 
decorated and use __cdecl calling convention.

Do you have any more suggestions, or would you please guide me on next steps?

Thanks,

Ali

# HG changeset patch
# User Ali Ince
# Date 1542542415 0
#  Sun Nov 18 12:00:15 2018 +
# Node ID a41df44e13e1b761f4b3f05a17ed18953067ae39
# Parent  16d5ec7dbbb49ef3f969e34be870e3f917ea89ff
Remove JNICALL modifiers to prevent name decoration on 32-bit windows builds

diff -r 16d5ec7dbbb4 -r a41df44e13e1 
src/jdk.jdi/share/native/libdt_shmem/shmemBack.c
--- a/src/jdk.jdi/share/native/libdt_shmem/shmemBack.c  Tue Aug 14 14:28:23 
2018 +0200
+++ b/src/jdk.jdi/share/native/libdt_shmem/shmemBack.c  Sun Nov 18 12:00:15 
2018 +
@@ -339,7 +339,7 @@
  return JDWPTRANSPORT_ERROR_NONE;
  }
  
-JNIEXPORT jint JNICALL

+JNIEXPORT jint
  jdwpTransport_OnLoad(JavaVM *vm, jdwpTransportCallback* cbTablePtr,
   jint version, jdwpTransportEnv** result)
  {
diff -r 16d5ec7dbbb4 -r a41df44e13e1 
src/jdk.jdwp.agent/share/native/include/jdwpTransport.h
--- a/src/jdk.jdwp.agent/share/native/include/jdwpTransport.h   Tue Aug 14 
14:28:23 2018 +0200
+++ b/src/jdk.jdwp.agent/share/native/include/jdwpTransport.h   Sun Nov 18 
12:00:15 2018 +
@@ -140,7 +140,7 @@
  void (*free)(void *buffer);  /* Call this for all deallocations */
  } jdwpTransportCallback;
  
-typedef jint (JNICALL *jdwpTransport_OnLoad_t)(JavaVM *jvm,

+typedef jint (*jdwpTransport_OnLoad_t)(JavaVM *jvm,
 jdwpTransportCallback 
*callback,
 jint version,
 jdwpTransportEnv** env);
diff -r 16d5ec7dbbb4 -r a41df44e13e1 
src/jdk.jdwp.agent/share/native/libdt_socket/socketTransport.c
--- a/src/jdk.jdwp.agent/share/native/libdt_socket/socketTransport.cTue Aug 
14 14:28:23 2018 +0200
+++ b/src/jdk.jdwp.agent/share/native/libdt_socket/socketTransport.cSun Nov 
18 12:00:15 2018 +
@@ -1019,7 +1019,7 @@
  return JDWPTRANSPORT_ERROR_NONE;
  }
  
-JNIEXPORT jint JNICALL

+JNIEXPORT jint
  jdwpTransport_OnLoad(JavaVM *vm, jdwpTransportCallback* cbTablePtr,
   jint version, jdwpTransportEnv** env)
  {




On 16 Nov 2018, at 23:03, Alexey Ivanov  wrote:

Hi Ali,

It's exactly what I referred to.

Yes, it does change the calling convention.
Yet it's not a (big) problem because both parties will use the same calling 
convention.

Using a DLL from another build will not be possible. But it's not a good idea 
to do so anyway.


Mapfiles and export options have been removed 
byhttps://bugs.openjdk.java.net/browse/JDK-8200178  to simplify managing 
exported functions. So that to add or remove an exported function, you don't 
have modify makefiles and/or mapfiles. You just edit the source files.

Regards,
Alexey

On 16/11/2018 22:42

Re: [PATCH] Windows 32-bit DLL name decoration

[Magnus Ihse Bursie]

Hi Ali,

From a build perspective this change looks OK. I'm not aware of the 
finer details on the OnLoad mechanism, like what calling convention is 
to be used. So maybe this is a no-go from that view.


I'm cc:ing servicability so they can have a look at it.

/Magnus

On 2018-11-18 13:07, Ali İnce wrote:

Hi Alexey,

Below is a new patch content that removes JNICALL modifiers from the exported 
functions of interest. This results in exported functions not to be name 
decorated and use __cdecl calling convention.

Do you have any more suggestions, or would you please guide me on next steps?

Thanks,

Ali

# HG changeset patch
# User Ali Ince 
# Date 1542542415 0
#  Sun Nov 18 12:00:15 2018 +
# Node ID a41df44e13e1b761f4b3f05a17ed18953067ae39
# Parent  16d5ec7dbbb49ef3f969e34be870e3f917ea89ff
Remove JNICALL modifiers to prevent name decoration on 32-bit windows builds

diff -r 16d5ec7dbbb4 -r a41df44e13e1 
src/jdk.jdi/share/native/libdt_shmem/shmemBack.c
--- a/src/jdk.jdi/share/native/libdt_shmem/shmemBack.c  Tue Aug 14 14:28:23 
2018 +0200
+++ b/src/jdk.jdi/share/native/libdt_shmem/shmemBack.c  Sun Nov 18 12:00:15 
2018 +
@@ -339,7 +339,7 @@
  return JDWPTRANSPORT_ERROR_NONE;
  }
  
-JNIEXPORT jint JNICALL

+JNIEXPORT jint
  jdwpTransport_OnLoad(JavaVM *vm, jdwpTransportCallback* cbTablePtr,
   jint version, jdwpTransportEnv** result)
  {
diff -r 16d5ec7dbbb4 -r a41df44e13e1 
src/jdk.jdwp.agent/share/native/include/jdwpTransport.h
--- a/src/jdk.jdwp.agent/share/native/include/jdwpTransport.h   Tue Aug 14 
14:28:23 2018 +0200
+++ b/src/jdk.jdwp.agent/share/native/include/jdwpTransport.h   Sun Nov 18 
12:00:15 2018 +
@@ -140,7 +140,7 @@
  void (*free)(void *buffer);  /* Call this for all deallocations */
  } jdwpTransportCallback;
  
-typedef jint (JNICALL *jdwpTransport_OnLoad_t)(JavaVM *jvm,

+typedef jint (*jdwpTransport_OnLoad_t)(JavaVM *jvm,
 jdwpTransportCallback 
*callback,
 jint version,
 jdwpTransportEnv** env);
diff -r 16d5ec7dbbb4 -r a41df44e13e1 
src/jdk.jdwp.agent/share/native/libdt_socket/socketTransport.c
--- a/src/jdk.jdwp.agent/share/native/libdt_socket/socketTransport.cTue Aug 
14 14:28:23 2018 +0200
+++ b/src/jdk.jdwp.agent/share/native/libdt_socket/socketTransport.cSun Nov 
18 12:00:15 2018 +
@@ -1019,7 +1019,7 @@
  return JDWPTRANSPORT_ERROR_NONE;
  }
  
-JNIEXPORT jint JNICALL

+JNIEXPORT jint
  jdwpTransport_OnLoad(JavaVM *vm, jdwpTransportCallback* cbTablePtr,
   jint version, jdwpTransportEnv** env)
  {




On 16 Nov 2018, at 23:03, Alexey Ivanov  wrote:

Hi Ali,

It's exactly what I referred to.

Yes, it does change the calling convention.
Yet it's not a (big) problem because both parties will use the same calling 
convention.

Using a DLL from another build will not be possible. But it's not a good idea 
to do so anyway.


Mapfiles and export options have been removed by 
https://bugs.openjdk.java.net/browse/JDK-8200178 to simplify managing exported 
functions. So that to add or remove an exported function, you don't have modify 
makefiles and/or mapfiles. You just edit the source files.

Regards,
Alexey

On 16/11/2018 22:42, Ali İnce wrote:

Hi Alexey,

Thanks for your reply.

I will definitely give it a try though I’m not sure if that’ll be a breaking 
change. This is because JNICALL modifier is defined to be __stdcall on windows 
which is both specified on jdwpTransport_OnLoad exported functions both for 
dt_socket.dll and dt_shmem.dll.

The __stdcall is ignored on x64 platforms and also name decoration is not 
applied 
(https://docs.microsoft.com/en-gb/cpp/build/reference/decorated-names?view=vs-2017).
 I believe that’s why we don’t experience any problems on x64 builds.

Removing JNICALL (thus __stdcall) modifier (apparently only applies to x86 
builds) will result in the calling convention to be changed, and thus will 
change how parameters are ordered and also the stack cleanup rules. I think 
this may result in problems in programs that are designed to load shared 
libraries and its exported functions at runtime (not bound by link time which 
probably won’t cause any issues - assuming that they use the shared function 
signature) - as in 
https://github.com/AdoptOpenJDK/openjdk-jdk11u/blob/5f01925b80ed851b133ee26fbcb07026ac04149e/src/jdk.jdwp.agent/share/native/libjdwp/transport.c#L99.

Any thoughts?

Regards,

Ali


On 15 Nov 2018, at 22:14, Alexey Ivanov  wrote:

Hi Ali,

Can the issue be resolved by using different call modifiers on the affected 
functions?

Some build / link issues were resolved by adding / removing JNICALL modifier, 
see
https://bugs.openjdk.java.net/browse/JDK-8201226
http://mail.openjdk.java.net/pipermail/build-dev/2018-April/021553.html

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