Re: RFR: JDK-8286348: incorrect use of `@serial` [v3]

[Phil Race]

On Mon, 9 May 2022 20:19:45 GMT, Jonathan Gibbons  wrote:

>> Please review a fix to remove incorrect use of the `@serial` tag from the 
>> doc comments for methods such as `readObject` and `readResolve`. The tag has 
>> no effect in this position other than to trigger warnings from the standard 
>> doclet when running javadoc.
>> 
>> There is no change to the generated documentation as a result off this 
>> change. In particular, there is no change to the API docs for any of the 
>> modified files, or to the overall top-level serialized-form.html file.
>> 
>> Although most of the affected files are in the `java.desktop` module, there 
>> is one outlier, in `java.security.Provider`.
>
> Jonathan Gibbons has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Fix whitespace (blank lines) after merge

Marked as reviewed by prr (Reviewer).

-

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

Re: RFR: JDK-8286348: incorrect use of `@serial` [v2]

[Phil Race]

On Mon, 9 May 2022 18:14:35 GMT, Jonathan Gibbons  wrote:

>> Please review a fix to remove incorrect use of the `@serial` tag from the 
>> doc comments for methods such as `readObject` and `readResolve`. The tag has 
>> no effect in this position other than to trigger warnings from the standard 
>> doclet when running javadoc.
>> 
>> There is no change to the generated documentation as a result off this 
>> change. In particular, there is no change to the API docs for any of the 
>> modified files, or to the overall top-level serialized-form.html file.
>> 
>> Although most of the affected files are in the `java.desktop` module, there 
>> is one outlier, in `java.security.Provider`.
>
> Jonathan Gibbons has updated the pull request with a new target base due to a 
> merge or a rebase. The pull request now contains two commits:
> 
>  - Merge with upstream/master
>  - JDK-8286348: incorrect use of `@serial`

Marked as reviewed by prr (Reviewer).

-

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

Re: RFR: JDK-6725221 Standardize obtaining boolean properties with defaults

[Phil Race]

On Thu, 5 May 2022 16:49:07 GMT, Mark Powers  wrote:

> JDK-6725221 Standardize obtaining boolean properties with defaults

I did wonder why it has security-libs as the sub-category and if the intent was 
not what we see here.

-

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

Re: RFR: JDK-8286348: incorrect use of `@serial`

[Phil Race]

On Sat, 7 May 2022 01:04:03 GMT, Jonathan Gibbons  wrote:

> Please review a fix to remove incorrect use of the `@serial` tag from the doc 
> comments for methods such as `readObject` and `readResolve`. The tag has no 
> effect in this position other than to trigger warnings from the standard 
> doclet when running javadoc.
> 
> There is no change to the generated documentation as a result off this 
> change. In particular, there is no change to the API docs for any of the 
> modified files, or to the overall top-level serialized-form.html file.
> 
> Although most of the affected files are in the `java.desktop` module, there 
> is one outlier, in `java.security.Provider`.

Jon, all of the changes in java.desktop are already underway in 
https://github.com/openjdk/jdk/pull/8432/files and
have been approved and even have an approved CSR ..  just waiting for 
@alisenchung to type /integrate ..

-

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

Re: RFR: JDK-6725221 Standardize obtaining boolean properties with defaults

[Phil Race]

On Thu, 5 May 2022 16:49:07 GMT, Mark Powers  wrote:

> JDK-6725221 Standardize obtaining boolean properties with defaults

The xtoolkit change is fine. I've not looked at anything else
You'll clearly need multiple reviewers for this one.

-

Marked as reviewed by prr (Reviewer).

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

Re: RFR: JDK-8285676: Add missing @param tags for type parameters on classes and interfaces [v3]

[Phil Race]

On Thu, 28 Apr 2022 01:31:13 GMT, Joe Darcy  wrote:

>> To enable more complete doclint checking (courtesy @jonathan-gibbons), 
>> please review this PR to add type-level @param tags where they are missing.
>> 
>> To the maintainers of java.util.concurrent, those changes could be separated 
>> out in another bug if that would ease maintenance of that code.
>> 
>> Making these library fixes is a blocker for correcting and expanding the 
>> doclint checks (JDK-8285496).
>> 
>> I'll update copyright years before pushing.
>
> Joe Darcy has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Respond to more review feedback.

Marked as reviewed by prr (Reviewer).

-

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

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

[Phil Race]

On Fri, 18 Mar 2022 21:24:43 GMT, Phil Race  wrote:

>> Magnus Ihse Bursie has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   Fix typos
>
> This doesn't seem right to me to put x11wrappergen into share.
> 
> This is X11 specific. It should not be in share.
> 
> Same for all of the fontconfig files. In make/data it did not seem too weird 
> but it is very weird to put them all in share. If you were to go back and 
> look how it used to be before someone moved them to make I am sure you'd find 
> them in platform specific source directories.

> @prrace I have now moved the fontconfig files to `$OS/data`. (I also took the 
> opportunity to clean up the GenData file, which had a quite hairy logic for a 
> trivial task.) I have moved the x11wrappergen files to `unix/data`.
> 
> (Strictly speaking, "unix" and "x11" do not have the same "sense" (in the 
> Fregean meaning), even if it has the same "referent". But we've used that 
> convention before so I'm sticking to it.)

Indeed they do not but it is a better approximation than "shared".

-

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

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

[Phil Race]

On Thu, 17 Mar 2022 00:12:38 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.
>> 
>> ### Modules reviewed
>> 
>> - [x] java.base
>> - [x] java.desktop
>> - [x] jdk.compiler
>> - [x] java.se
>
> Magnus Ihse Bursie has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Fix typos

This doesn't seem right to me to put x11wrappergen into share.

This is X11 specific. It should not be in share.

Same for all of the fontconfig files. In make/data it did not seem too weird 
but it is very weird to put them all in share. If you were to go back and look 
how it used to be before someone moved them to make I am sure you'd find them 
in platform specific source directories.

-

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

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

[Phil Race]

On Thu, 17 Mar 2022 00:12:38 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.
>> 
>> ### Modules reviewed
>> 
>> - [x] java.base
>> - [x] java.desktop
>> - [x] jdk.compiler
>> - [x] java.se
>
> Magnus Ihse Bursie has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Fix typos

Changes requested by prr (Reviewer).

make/modules/java.desktop/gensrc/GensrcX11Wrappers.gmk line 27:

> 25: 
> 26: # Generate java sources using the X11 offsets that are precalculated in 
> files
> 27: # src/java.desktop/share/data/x11wrappergen/sizes-.txt.

This doesn't seem right to me. This is X11 specific. It should not be in share.
Same for related files.

-

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

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

[Phil Race]

On Tue, 15 Mar 2022 23:50: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.
>> 
>> ### Modules reviewed
>> 
>> - [x] java.base
>> - [x] java.desktop
>> - [x] jdk.compiler
>> - [x] java.se
>
> Magnus Ihse Bursie has updated the pull request with a new target base due to 
> a merge or a rebase. The pull request now contains 12 commits:
> 
>  - Merge branch 'master' into shuffle-data-reborn
>  - Fix merge
>  - Merge tag 'jdk-19+13' into shuffle-data-reborn
>
>Added tag jdk-19+13 for changeset 5df2a057
>  - Move characterdata templates to share/classes/java/lang.
>  - Update comment refering to "make" dir
>  - Move new symbols to jdk.compiler
>  - Merge branch 'master' into shuffle-data
>  - Move macosxicons from share to macosx
>  - Move to share/data, and move jdwp.spec to java.se
>  - Update references in test
>  - ... and 2 more: 
> https://git.openjdk.java.net/jdk/compare/83d77186...598f740f

How are folks reviewing this huge PR ?
My browser paints a few files and that's it, and the drop down list to "jump 
to" is too big to display - it says !

-

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

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

[Phil Race]

On Fri, 28 Jan 2022 14:39:31 GMT, Matteo Baccan  wrote:

> Hi
> 
> I have reviewed the code for removing double semicolons at the end of lines
> 
> all the best
> matteo

Marked as reviewed by prr (Reviewer).

Looks like  there's only one client source code file touched
Most of the client changes are only in jtreg tests - and this is very trivial, 
so I'm OK with them being included here.

-

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

Re: RFR: JDK-8276447 Deprecate finalization-related methods for removal [v3]

[Phil Race]

On Wed, 1 Dec 2021 19:23:59 GMT, Brent Christian  wrote:

>> Here are the code changes for the "Deprecate finalizers in the standard Java 
>> API" portion of JEP 421 ("Deprecate Finalization for Removal") for code 
>> review.
>> 
>> This change makes the indicated deprecations, and updates the API spec for 
>> JEP 421. It also updates the relevant @SuppressWarning annotations.
>> 
>> The CSR has been approved.
>> An automated test build+test run passes cleanly (FWIW :D ).
>
> Brent Christian has updated the pull request with a new target base due to a 
> merge or a rebase. The pull request now contains 33 commits:
> 
>  - Merge branch 'master' into 8276447
>  - Merge branch 'master' into 8276447
>  - merge
>  - @SuppressWarnings(removal) in Windows CKey.java
>  - Add jls tag to runFinalization methods
>  - Update wording of Object.finalize, new paragraph is now bold
>  - update Object.finalize javadoc
>  - update Object.finalize JavaDoc and @deprecated tag
>  - Update Object.finalize deprecation message
>  - Indicate that runFinalizers does nothing if finalization is disabled or 
> removed
>  - ... and 23 more: 
> https://git.openjdk.java.net/jdk/compare/0dfb3a70...8cde0674

Marked as reviewed by prr (Reviewer).

-

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

Re: RFR: 8276348: Use blessed modifier order in java.base

[Phil Race]

On Tue, 2 Nov 2021 16:30:56 GMT, Pavel Rappo  wrote:

> This PR follows up one of the recent PRs, where I used a non-canonical 
> modifier order. Since the problem was noticed [^1], why not to address it at 
> mass?
> 
> As far as I remember, the first mass-canonicalization of modifiers took place 
> in JDK-8136583 in 2015 [^2]. That change affected 1780 lines spanning 453 
> files. Since then modifiers have become a bit lose, and it makes sense to 
> re-bless (using the JDK-8136583 terminology) them.
> 
> This change was produced by running the below command followed by updating 
> the copyright years on the affected files where necessary:
> 
> $ sh ./bin/blessed-modifier-order.sh src/java.base
> 
> The resulting change is much smaller than that of 2015: 39 lines spanning 21 
> files.
> 
> [^1]: 
> https://mail.openjdk.java.net/pipermail/core-libs-dev/2021-November/082987.html
>  (or https://github.com/openjdk/jdk/pull/6191#pullrequestreview-79465)
> [^2]: 
> http://mail.openjdk.java.net/pipermail/core-libs-dev/2015-September/035217.html

JFYI a couple of times I've wondered if we regressed on this. I just ran the 
script on the desktop module and we havea  few instances there too, so I've 
filed a clean up bug on it.

-

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

Re: RFR: 8273101: Eliminate the usage of threadgroup sandboxing in the java.util.logging

[Phil Race]

On Wed, 1 Sep 2021 06:31:16 GMT, Sergey Bylokhov  wrote:

> The "java.util.logging.LogManager" class uses the "threadgroup sandboxing" 
> via an AppContext to support "applet logging isolation". The AppContext class 
> became useless since the plugin and webstart are no longer supported and 
> removed in jdk11.
> 
> This is the request to delete the usage of AppContext in the LogManager and 
> related tests.
> 
> Tested by tier1/tier2/tier3 and jdk_desktop tests.

@aivanov-jdk will help make sure the closed changes are pushed at exactly the 
same time as this gets pushed.

-

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

Re: RFR: 8273101: Eliminate the usage of threadgroup sandboxing in the java.util.logging

[Phil Race]

On Wed, 1 Sep 2021 06:31:16 GMT, Sergey Bylokhov  wrote:

> The "java.util.logging.LogManager" class uses the "threadgroup sandboxing" 
> via an AppContext to support "applet logging isolation". The AppContext class 
> became useless since the plugin and webstart are no longer supported and 
> removed in jdk11.
> 
> This is the request to delete the usage of AppContext in the LogManager and 
> related tests.
> 
> Tested by tier1/tier2/tier3 and jdk_desktop tests.

Hmm I was under the impression this was removing AppContext itself but it is 
just removing the backdoor needed by logging
Perhaps this isn't the change that requires the CSR but it then leaves an 
inconsistent state where desktop supports AppContext still but other modules 
don't ...

-

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

Re: RFR: 8273101: Eliminate the usage of threadgroup sandboxing in the java.util.logging

[Phil Race]

On Wed, 1 Sep 2021 06:31:16 GMT, Sergey Bylokhov  wrote:

> The "java.util.logging.LogManager" class uses the "threadgroup sandboxing" 
> via an AppContext to support "applet logging isolation". The AppContext class 
> became useless since the plugin and webstart are no longer supported and 
> removed in jdk11.
> 
> This is the request to delete the usage of AppContext in the LogManager and 
> related tests.
> 
> Tested by tier1/tier2/tier3 and jdk_desktop tests.

I believe we should have a CSR for this. It removes a long standing capability 
in the platform that was used by
components such as plugin and webstart.

-

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

Re: RFR: 8273101: Eliminate the usage of threadgroup sandboxing in the java.util.logging

[Phil Race]

On Wed, 1 Sep 2021 06:31:16 GMT, Sergey Bylokhov  wrote:

> The "java.util.logging.LogManager" class uses the "threadgroup sandboxing" 
> via an AppContext to support "applet logging isolation". The AppContext class 
> became useless since the plugin and webstart are no longer supported and 
> removed in jdk11.
> 
> This is the request to delete the usage of AppContext in the LogManager and 
> related tests.
> 
> Tested by tier1/tier2/tier3 and jdk_desktop tests.

This fix requires coordinated closed changes so needs an Oracle sponsor.
And actually is probably better handled entirely by an Oracle engineer because 
pushes need to be very co-ordinated.

-

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

Re: RFR: 8267521: Post JEP 411 refactoring: maximum covering > 50K [v3]

[Phil Race]

On Fri, 21 May 2021 20:37:30 GMT, Weijun Wang  wrote:

>> The code change refactors classes that have a `SuppressWarnings("removal")` 
>> annotation that covers more than 50KB of code. The big annotation is often 
>> quite faraway from the actual deprecated API usage it is suppressing, and 
>> with the annotation covering too big a portion it's easy to call other 
>> deprecated methods without being noticed.
>> 
>> The code change shows some common solutions to avoid such too wide 
>> annotations:
>> 
>> 1. Extract calls into a method and add annotation on that method
>> 2. Assign the return value of a deprecated method call to a new local 
>> variable and add annotation on the declaration, and then assign that value 
>> to the original l-value if not void. The local variable will be called `tmp` 
>> if later reassigned or `dummy` if it will be discarded.
>> 3. Put declaration and assignment into a single statement if possible.
>> 4. Rewrite code to achieve #3 above.
>> 
>> I'll add a copyright year update commit before integration.
>
> Weijun Wang has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   update FtpClient.java

Marked as reviewed by prr (Reviewer).

-

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

Re: RFR: 8267521: Post JEP 411 refactoring: maximum covering > 50K [v3]

[Phil Race]

On Fri, 28 May 2021 02:50:55 GMT, Weijun Wang  wrote:

>> src/java.desktop/share/classes/java/awt/Component.java line 630:
>> 
>>> 628: }
>>> 629: 
>>> 630: @SuppressWarnings("removal")
>> 
>> I'm confused. I thought the reason this wasn't done in the JEP 
>> implementation PR is because of refactoring
>> that was needed because of the usage in this static block and you could not 
>> apply the annotation here.
>> Yet it seems you are doing exactly what was supposed to be impossible with 
>> no refactoring.
>> Can you explain ?
>
> There *is* a tiny refactoring here: a new variable `s2` is introduced so the 
> 2nd `doPrivileged` call on line 636 is now also in a declaration statement 
> (for `s2`) and therefore annotatable. Without this I cannot add the 
> annotation on line 635.

Ok. But I will quote you
"This happens when a deprecated method is called inside a static block. The 
annotation can only be added to a declaration and here it must be the whole 
class"

So the way you explained this before made it sound like any time there was any 
SM API usage in a static block, the entire class needed to be annotated.

Why has the explanation changed ?

-

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

Re: RFR: 8267521: Post JEP 411 refactoring: maximum covering > 50K [v3]

[Phil Race]

On Fri, 21 May 2021 20:37:30 GMT, Weijun Wang  wrote:

>> The code change refactors classes that have a `SuppressWarnings("removal")` 
>> annotation that covers more than 50KB of code. The big annotation is often 
>> quite faraway from the actual deprecated API usage it is suppressing, and 
>> with the annotation covering too big a portion it's easy to call other 
>> deprecated methods without being noticed.
>> 
>> The code change shows some common solutions to avoid such too wide 
>> annotations:
>> 
>> 1. Extract calls into a method and add annotation on that method
>> 2. Assign the return value of a deprecated method call to a new local 
>> variable and add annotation on the declaration, and then assign that value 
>> to the original l-value if not void. The local variable will be called `tmp` 
>> if later reassigned or `dummy` if it will be discarded.
>> 3. Put declaration and assignment into a single statement if possible.
>> 4. Rewrite code to achieve #3 above.
>> 
>> I'll add a copyright year update commit before integration.
>
> Weijun Wang has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   update FtpClient.java

src/java.desktop/share/classes/java/awt/Component.java line 630:

> 628: }
> 629: 
> 630: @SuppressWarnings("removal")

I'm confused. I thought the reason this wasn't done in the JEP implementation 
PR is because of refactoring
that was needed because of the usage in this static block and you could not 
apply the annotation here.
Yet it seems you are doing exactly what was supposed to be impossible with no 
refactoring.
Can you explain ?

-

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

Re: RFR: 8266459: Implement JEP 411: Deprecate the Security Manager for Removal [v4]

[Phil Race]

On Mon, 24 May 2021 13:53:34 GMT, Weijun Wang  wrote:

>> Please review this implementation of [JEP 
>> 411](https://openjdk.java.net/jeps/411).
>> 
>> The code change is divided into 3 commits. Please review them one by one.
>> 
>> 1. 
>> https://github.com/openjdk/jdk/commit/576161d15423f58281e384174d28c9f9be7941a1
>>  The essential change for this JEP, including the `@Deprecate` annotations 
>> and spec change. It also update the default value of the 
>> `java.security.manager` system property to "disallow", and necessary test 
>> change following this update.
>> 2. 
>> https://github.com/openjdk/jdk/commit/26a54a835e9f84aa528740a7c5c35d07355a8a66
>>  Manual changes to several files so that the next commit can be generated 
>> programatically.
>> 3. 
>> https://github.com/openjdk/jdk/commit/eb6c566ff9207974a03a53335e0e697cffcf0950
>>  Automatic changes to other source files to avoid javac warnings on 
>> deprecation for removal
>> 
>> The 1st and 2nd commits should be reviewed carefully. The 3rd one is 
>> generated programmatically, see the comment below for more details. If you 
>> are only interested in a portion of the 3rd commit and would like to review 
>> it as a separate file, please comment here and I'll generate an individual 
>> webrev.
>> 
>> Due to the size of this PR, no attempt is made to update copyright years for 
>> any file to minimize unnecessary merge conflict.
>> 
>> Furthermore, since the default value of `java.security.manager` system 
>> property is now "disallow", most of the tests calling 
>> `System.setSecurityManager()` need to launched with 
>> `-Djava.security.manager=allow`. This is covered in a different PR at 
>> https://github.com/openjdk/jdk/pull/4071.
>> 
>> Update: the deprecation annotations and javadoc tags, build, compiler, 
>> core-libs, hotspot, i18n, jmx, net, nio, security, and serviceability are 
>> reviewed. Rest are 2d, awt, beans, sound, and swing.
>
> Weijun Wang has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   keep only one systemProperty tag

Marked as reviewed by prr (Reviewer).

I'm OK with this now given that the refactoring is already underway at 
https://github.com/openjdk/jdk/pull/4138

-

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

Re: RFR: 8266459: Implement JEP 411: Deprecate the Security Manager for Removal [v3]

[Phil Race]

On Wed, 19 May 2021 13:47:53 GMT, Weijun Wang  wrote:

>> Please review this implementation of [JEP 
>> 411](https://openjdk.java.net/jeps/411).
>> 
>> The code change is divided into 3 commits. Please review them one by one.
>> 
>> 1. 
>> https://github.com/openjdk/jdk/commit/576161d15423f58281e384174d28c9f9be7941a1
>>  The essential change for this JEP, including the `@Deprecate` annotations 
>> and spec change. It also update the default value of the 
>> `java.security.manager` system property to "disallow", and necessary test 
>> change following this update.
>> 2. 
>> https://github.com/openjdk/jdk/commit/26a54a835e9f84aa528740a7c5c35d07355a8a66
>>  Manual changes to several files so that the next commit can be generated 
>> programatically.
>> 3. 
>> https://github.com/openjdk/jdk/commit/eb6c566ff9207974a03a53335e0e697cffcf0950
>>  Automatic changes to other source files to avoid javac warnings on 
>> deprecation for removal
>> 
>> The 1st and 2nd commits should be reviewed carefully. The 3rd one is 
>> generated programmatically, see the comment below for more details. If you 
>> are only interested in a portion of the 3rd commit and would like to review 
>> it as a separate file, please comment here and I'll generate an individual 
>> webrev.
>> 
>> Due to the size of this PR, no attempt is made to update copyright years for 
>> any file to minimize unnecessary merge conflict.
>> 
>> Furthermore, since the default value of `java.security.manager` system 
>> property is now "disallow", most of the tests calling 
>> `System.setSecurityManager()` need to launched with 
>> `-Djava.security.manager=allow`. This is covered in a different PR at 
>> https://github.com/openjdk/jdk/pull/4071.
>> 
>> Update: the deprecation annotations and javadoc tags, build, compiler, 
>> core-libs, hotspot, i18n, jmx, net, nio, security, and serviceability are 
>> reviewed. Rest are 2d, awt, beans, sound, and swing.
>
> Weijun Wang has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   fixing awt/datatransfer/DataFlavor/DataFlavorRemoteTest.java

I haven't seen any response to this comment I made a couple of days ago and I 
suspect it got missed
>
> Weijun Wang has updated the pull request incrementally with one additional 
> commit since the last revision:
>
>   fixing awt/datatransfer/DataFlavor/DataFlavorRemoteTest.java

test/jdk/java/awt/datatransfer/DataFlavor/DataFlavorRemoteTest.java line 59:

> 57: ProcessCommunicator
> 58: .executeChildProcess(Consumer.class, new 
> String[0]);
> 59: if (!"Hello".equals(processResults.getStdOut())) {

Who or what prompted this change ?

-

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

Re: RFR: 8267184: JEP 411: Add -Djava.security.manager=allow to tests calling System.setSecurityManager [v2]

[Phil Race]

On Tue, 18 May 2021 21:44:43 GMT, Weijun Wang  wrote:

>> Please review the test changes for [JEP 
>> 411](https://openjdk.java.net/jeps/411).
>> 
>> With JEP 411 and the default value of `-Djava.security.manager` becoming 
>> `disallow`, tests calling `System.setSecurityManager()`  need 
>> `-Djava.security.manager=allow` when launched. This PR covers such changes 
>> for tier1 to tier3 (except for the JCK tests).
>> 
>> To make it easier to focus your review on the tests in your area, this PR is 
>> divided into multiple commits for different areas (~~serviceability~~, 
>> ~~hotspot-compiler~~, ~~i18n~~, ~~rmi~~, ~~javadoc~~, swing, 2d, 
>> ~~security~~, ~~hotspot-runtime~~, ~~nio~~, ~~xml~~, ~~beans~~, 
>> ~~core-libs~~, ~~net~~, ~~compiler~~, ~~hotspot-gc~~). Mostly the rule is 
>> the same as how Skara adds labels, but there are several small tweaks:
>> 
>> 1. When a file is covered by multiple labels, only one is chosen. I make my 
>> best to avoid putting too many tests into `core-libs`. If a file is covered 
>> by `core-libs` and another label, I categorized it into the other label.
>> 2. If a file is in `core-libs` but contains `/xml/` in its name, it's in the 
>> `xml` commit.
>> 3. If a file is in `core-libs` but contains `/rmi/` in its name, it's in the 
>> `rmi` commit.
>> 4. One file not covered by any label -- 
>> `test/jdk/com/sun/java/accessibility/util/8051626/Bug8051626.java` -- is in 
>> the `swing` commit.
>> 
>> Due to the size of this PR, no attempt is made to update copyright years for 
>> all files to minimize unnecessary merge conflict.
>> 
>> Please note that this PR can be integrated before the source changes for JEP 
>> 411, as the possible values of this system property was already defined long 
>> time ago in JDK 9.
>> 
>> Most of the change in this PR is a simple adding of 
>> `-Djava.security.manager=allow` to the `@run main/othervm` line. Sometimes 
>> it was not `othervm` and we add one. Sometimes there's no `@run` at all and 
>> we add the line.
>> 
>> There are several tests that launch another Java process that needs to call 
>> the `System.setSecurityManager()` method, and the system property is added 
>> to `ProcessBuilder`, `ProcessTools`, or the java command line (if the test 
>> is a shell test).
>> 
>> 3 langtools tests are added into problem list due to 
>> [JDK-8265611](https://bugs.openjdk.java.net/browse/JDK-8265611).
>> 
>> 2 SQL tests are moved because they need different options on the `@run` line 
>> but they are inside a directory that has a `TEST.properties`:
>> 
>> rename test/jdk/java/sql/{testng/test/sql/othervm => 
>> permissionTests}/DriverManagerPermissionsTests.java (93%)
>> rename test/jdk/javax/sql/{testng/test/rowset/spi => 
>> permissionTests}/SyncFactoryPermissionsTests.java (95%)
>>  ```
>> 
>> The source change for JEP 411 is at https://github.com/openjdk/jdk/pull/4073.
>
> Weijun Wang has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   adjust order of VM options

The client changes are fine except for the one misplaced (c)

test/jdk/java/awt/FontClass/CreateFont/fileaccess/FontFile.java line 3:

> 1: /*
> 2:  * Copyright (c) 2008, 2021, Oracle and/or its affiliates. All rights 
> reserved.
> 3:  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.

Probably the (c) update was meant to be on the .sh file that is actually 
modified.

-

Marked as reviewed by prr (Reviewer).

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

Re: RFR: 8266459: Implement JEP 411: Deprecate the Security Manager for Removal [v3]

[Phil Race]

On Thu, 20 May 2021 07:06:00 GMT, Alan Bateman  wrote:

>> The JEP isn't PTT for 17 so there's plenty of time isn't there ?
>
> There are 827 files in this patch. Phil is right that adding SW at the class 
> level is introducing technical debt but if addressing that requires 
> refactoring and re-testing of AWT or font code then it would be better to 
> have someone from that area working on. Is there any reason why this can't be 
> going on now on awt-dev/2d-dev and integrated immediately after JEP 411? I 
> don't think we should put Max through the wringer here as there are too many 
> areas to cover.

Are you suggesting that the patch doesn't need testing as it is ? It should be 
the same either way.
It is very straight forward to run the automated tests across all platforms 
these days.
I get the impression that no one is guaranteeing to do this straight after 
integration.
It sounds like it is up for deferral if time runs out.

The amount of follow-on work that I am hearing about here, and may be for 
tests, does not  make it sound
like this JEP is nearly as done as first presented.

If there was some expectation that groups responsible for an area would get 
involved with this
issue which I am assured was already known about, then why was it not raised 
before and made
part of the plan ?

-

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

Re: RFR: 8266459: Implement JEP 411: Deprecate the Security Manager for Removal [v3]

[Phil Race]

On Thu, 20 May 2021 04:05:23 GMT, Phil Race  wrote:

>> By converting JDK-8267432 to a bug, there is an extra benefit that we can 
>> fix it after RDP. So I'll convert it now.
>
> That is unfortunate, but nonetheless I think required to be done.
> We have acknowledeged this can't reasonably be called an RFE, so the JEP is 
> introducing bugs/technical debt/call it what you will. This should generally 
> be part of a sandbox for the JEP and fixed before integration of the JEP.
> From my point of view it is a blocker.

The JEP isn't PTT for 17 so there's plenty of time isn't there ?

-

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

Re: RFR: 8266459: Implement JEP 411: Deprecate the Security Manager for Removal [v3]

[Phil Race]

On Thu, 20 May 2021 02:09:57 GMT, Weijun Wang  wrote:

>> I can make it a bug.
>> 
>> I don't want to do it here is because it involves indefinite amount of 
>> manual work and we will see everyone having their preferences. The more time 
>> we spend on this PR the more likely there will be merge conflict. There are 
>> just too many files here.
>> 
>> And no matter if we include that change in this PR or after it, it will be 
>> after the automatic conversion. In fact, the data about which cases are more 
>> worth fixing come from the automatic conversion itself. Also, as the manual 
>> work will be done part by part, if the automatic conversion is after it, 
>> there will be rounds and rounds of history rewriting and force push. This is 
>> quite unfriendly to the reviewers.
>> 
>> Altogether, there are 117 class-level annotations. Unfortunately, 
>> `java.awt.Component` is the one with the biggest class -- estimated number 
>> of bytes that the annotation covers is over 380K. In the client area, the 
>> 2nd place is `java.awt.Container`, and then we have 
>> `sun.font.SunFontManager`, `java.awt.Window`, and `java.awt.Toolkit` which 
>> are over 100KB, and other 25 classes over 10KB, and other 11 classes smaller 
>> than 10KB.
>
> By converting JDK-8267432 to a bug, there is an extra benefit that we can fix 
> it after RDP. So I'll convert it now.

That is unfortunate, but nonetheless I think required to be done.
We have acknowledeged this can't reasonably be called an RFE, so the JEP is 
introducing bugs/technical debt/call it what you will. This should generally be 
part of a sandbox for the JEP and fixed before integration of the JEP.
>From my point of view it is a blocker.

-

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

Re: RFR: 8266459: Implement JEP 411: Deprecate the Security Manager for Removal [v3]

[Phil Race]

On Wed, 19 May 2021 22:14:20 GMT, Weijun Wang  wrote:

>> I don't think it is a separate P4 enhancement (?) that someone will (maybe) 
>> do next release.
>> I think it should all be taken care of as part of this proposed change.
>
> I just made it P3 (P4 was the default value), and I will target it to 17 once 
> JEP 411 is targeted 17. But I think it's probably not a good idea to include 
> it inside *this* PR. There are some middle ground where it's debatable if a 
> change is worth doing (Ex: which is uglier between an 
> a-liitle-faraway-annotation and a temporary variable?) so it's not obvious 
> what the scope of the refactoring can be. Thus it will be divided into 
> smaller sub-tasks. It's not totally impossible that part of the work will be 
> delayed to next release.

Well .. as an enhancement (P3 or otherwise) I can see it being dropped and 
definitely if it misses the fork,
and I don't get why you can't do it here. And if it isn't done the JEP isn't 
really ready.
I already pasted the patch for Component.java and it took about 60 seconds to 
do that.
Then yes, you would have to deal with the fact that now you need to reapply 
your automated tool to the file but this is just work you'd have to have done 
anyway if it was already refactored.

I only *noticed* Component and Container. And stopped there to raise the 
question. How many more cases are there ?

-

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

Re: RFR: 8266459: Implement JEP 411: Deprecate the Security Manager for Removal [v3]

[Phil Race]

On Wed, 19 May 2021 21:53:35 GMT, Weijun Wang  wrote:

>> That's a sad limitation of the annotation stuff then, but I don't think that 
>> it is insurmountable.
>> You can define a static private method to contain this and call it from the 
>> static initializer block.
>> Much better than applying the annotation to an entire class.
>> 
>> --- a/src/java.desktop/share/classes/java/awt/Component.java
>> +++ b/src/java.desktop/share/classes/java/awt/Component.java
>> @@ -618,6 +618,17 @@ public abstract class Component implements 
>> ImageObserver, MenuContainer,
>>   */
>>  static boolean isInc;
>>  static int incRate;
>> +
>> +private static void initIncRate() {
>> +String s = java.security.AccessController.doPrivileged(
>> + new 
>> GetPropertyAction("awt.image.incrementaldraw"));
>> +isInc = (s == null || s.equals("true"));
>> +
>> +s = java.security.AccessController.doPrivileged(
>> +  new GetPropertyAction("awt.image.redrawrate"));
>> +incRate = (s != null) ? Integer.parseInt(s) : 100;
>> +}
>> +
>>  static {
>>  /* ensure that the necessary native libraries are loaded */
>>  Toolkit.loadLibraries();
>> @@ -625,14 +636,7 @@ public abstract class Component implements 
>> ImageObserver, MenuContainer,
>>  if (!GraphicsEnvironment.isHeadless()) {
>>  initIDs();
>>  }
>> -
>> -String s = java.security.AccessController.doPrivileged(
>> -   new 
>> GetPropertyAction("awt.image.incrementaldraw"));
>> -isInc = (s == null || s.equals("true"));
>> -
>> -s = java.security.AccessController.doPrivileged(
>> -new 
>> GetPropertyAction("awt.image.redrawrate"));
>> -incRate = (s != null) ? Integer.parseInt(s) : 100;
>> +initIncRate();
>>  }
>
> Correct, there are ways to modify the code to make it more 
> annotation-friendly. We thought about whether it's good to do it before 
> adding the annotations or after it. Our decision now is to do it after 
> because it will be more easy to see why it's necessary and we can take time 
> to do them little by little. A new enhancement at 
> https://bugs.openjdk.java.net/browse/JDK-8267432 is filed.

I don't think it is a separate P4 enhancement (?) that someone will (maybe) do 
next release.
I think it should all be taken care of as part of this proposed change.

-

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

Re: RFR: 8266459: Implement JEP 411: Deprecate the Security Manager for Removal [v3]

[Phil Race]

On Wed, 19 May 2021 18:38:39 GMT, Weijun Wang  wrote:

>> src/java.desktop/share/classes/java/awt/Component.java line 217:
>> 
>>> 215:  * @author  Sami Shaio
>>> 216:  */
>>> 217: @SuppressWarnings("removal")
>> 
>> Why is this placed on the *entire class* ??
>> This class is 10535 lines long and mentions AccessControl something like 8 
>> places it uses AccessController or AcessControlContext.
>
> This happens when a deprecated method is called inside a static block. The 
> annotation can only be added to a declaration and here it must be the whole 
> class. The call in this file is
> 
> s = java.security.AccessController.doPrivileged(
> new 
> GetPropertyAction("awt.image.redrawrate"));

That's a sad limitation of the annotation stuff then, but I don't think that it 
is insurmountable.
You can define a static private method to contain this and call it from the 
static initializer block.
Much better than applying the annotation to an entire class.

--- a/src/java.desktop/share/classes/java/awt/Component.java
+++ b/src/java.desktop/share/classes/java/awt/Component.java
@@ -618,6 +618,17 @@ public abstract class Component implements ImageObserver, 
MenuContainer,
  */
 static boolean isInc;
 static int incRate;
+
+private static void initIncRate() {
+String s = java.security.AccessController.doPrivileged(
+ new 
GetPropertyAction("awt.image.incrementaldraw"));
+isInc = (s == null || s.equals("true"));
+
+s = java.security.AccessController.doPrivileged(
+  new GetPropertyAction("awt.image.redrawrate"));
+incRate = (s != null) ? Integer.parseInt(s) : 100;
+}
+
 static {
 /* ensure that the necessary native libraries are loaded */
 Toolkit.loadLibraries();
@@ -625,14 +636,7 @@ public abstract class Component implements ImageObserver, 
MenuContainer,
 if (!GraphicsEnvironment.isHeadless()) {
 initIDs();
 }
-
-String s = java.security.AccessController.doPrivileged(
-   new 
GetPropertyAction("awt.image.incrementaldraw"));
-isInc = (s == null || s.equals("true"));
-
-s = java.security.AccessController.doPrivileged(
-new 
GetPropertyAction("awt.image.redrawrate"));
-incRate = (s != null) ? Integer.parseInt(s) : 100;
+initIncRate();
 }

-

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

Re: RFR: 8266459: Implement JEP 411: Deprecate the Security Manager for Removal [v3]

[Phil Race]

On Wed, 19 May 2021 13:47:53 GMT, Weijun Wang  wrote:

>> Please review this implementation of [JEP 
>> 411](https://openjdk.java.net/jeps/411).
>> 
>> The code change is divided into 3 commits. Please review them one by one.
>> 
>> 1. 
>> https://github.com/openjdk/jdk/commit/576161d15423f58281e384174d28c9f9be7941a1
>>  The essential change for this JEP, including the `@Deprecate` annotations 
>> and spec change. It also update the default value of the 
>> `java.security.manager` system property to "disallow", and necessary test 
>> change following this update.
>> 2. 
>> https://github.com/openjdk/jdk/commit/26a54a835e9f84aa528740a7c5c35d07355a8a66
>>  Manual changes to several files so that the next commit can be generated 
>> programatically.
>> 3. 
>> https://github.com/openjdk/jdk/commit/eb6c566ff9207974a03a53335e0e697cffcf0950
>>  Automatic changes to other source files to avoid javac warnings on 
>> deprecation for removal
>> 
>> The 1st and 2nd commits should be reviewed carefully. The 3rd one is 
>> generated programmatically, see the comment below for more details. If you 
>> are only interested in a portion of the 3rd commit and would like to review 
>> it as a separate file, please comment here and I'll generate an individual 
>> webrev.
>> 
>> Due to the size of this PR, no attempt is made to update copyright years for 
>> any file to minimize unnecessary merge conflict.
>> 
>> Furthermore, since the default value of `java.security.manager` system 
>> property is now "disallow", most of the tests calling 
>> `System.setSecurityManager()` need to launched with 
>> `-Djava.security.manager=allow`. This is covered in a different PR at 
>> https://github.com/openjdk/jdk/pull/4071.
>> 
>> Update: the deprecation annotations and javadoc tags, build, compiler, 
>> core-libs, hotspot, i18n, jmx, net, nio, security, and serviceability are 
>> reviewed. Rest are 2d, awt, beans, sound, and swing.
>
> Weijun Wang has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   fixing awt/datatransfer/DataFlavor/DataFlavorRemoteTest.java

src/java.desktop/share/classes/java/awt/Component.java line 217:

> 215:  * @author  Sami Shaio
> 216:  */
> 217: @SuppressWarnings("removal")

Why is this placed on the *entire class* ??
This class is 10535 lines long and mentions AccessControl something like 8 
places it uses AccessController or AcessControlContext.

src/java.desktop/share/classes/java/awt/Container.java line 97:

> 95:  * @since 1.0
> 96:  */
> 97: @SuppressWarnings("removal")

Same issue as with Component. a > 5,000 line file that uses AccessController in 
just 4 places.

Where else are you adding this to entire classes instead of the specific site ?

-

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

Re: RFR: 8266459: Implement JEP 411: Deprecate the Security Manager for Removal [v3]

[Phil Race]

On Wed, 19 May 2021 13:47:53 GMT, Weijun Wang  wrote:

>> Please review this implementation of [JEP 
>> 411](https://openjdk.java.net/jeps/411).
>> 
>> The code change is divided into 3 commits. Please review them one by one.
>> 
>> 1. 
>> https://github.com/openjdk/jdk/commit/576161d15423f58281e384174d28c9f9be7941a1
>>  The essential change for this JEP, including the `@Deprecate` annotations 
>> and spec change. It also update the default value of the 
>> `java.security.manager` system property to "disallow", and necessary test 
>> change following this update.
>> 2. 
>> https://github.com/openjdk/jdk/commit/26a54a835e9f84aa528740a7c5c35d07355a8a66
>>  Manual changes to several files so that the next commit can be generated 
>> programatically.
>> 3. 
>> https://github.com/openjdk/jdk/commit/eb6c566ff9207974a03a53335e0e697cffcf0950
>>  Automatic changes to other source files to avoid javac warnings on 
>> deprecation for removal
>> 
>> The 1st and 2nd commits should be reviewed carefully. The 3rd one is 
>> generated programmatically, see the comment below for more details. If you 
>> are only interested in a portion of the 3rd commit and would like to review 
>> it as a separate file, please comment here and I'll generate an individual 
>> webrev.
>> 
>> Due to the size of this PR, no attempt is made to update copyright years for 
>> any file to minimize unnecessary merge conflict.
>> 
>> Furthermore, since the default value of `java.security.manager` system 
>> property is now "disallow", most of the tests calling 
>> `System.setSecurityManager()` need to launched with 
>> `-Djava.security.manager=allow`. This is covered in a different PR at 
>> https://github.com/openjdk/jdk/pull/4071.
>> 
>> Update: the deprecation annotations and javadoc tags, build, compiler, 
>> core-libs, hotspot, i18n, jmx, net, nio, security, and serviceability are 
>> reviewed. Rest are 2d, awt, beans, sound, and swing.
>
> Weijun Wang has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   fixing awt/datatransfer/DataFlavor/DataFlavorRemoteTest.java

This change is so large that github won't even display the list of files so I 
can't jump to where I want to go !
And when I try to scroll I get just a blank page.

-

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

Re: RFR: 8266459: Implement JEP 411: Deprecate the Security Manager for Removal [v3]

[Phil Race]

On Wed, 19 May 2021 13:47:53 GMT, Weijun Wang  wrote:

>> Please review this implementation of [JEP 
>> 411](https://openjdk.java.net/jeps/411).
>> 
>> The code change is divided into 3 commits. Please review them one by one.
>> 
>> 1. 
>> https://github.com/openjdk/jdk/commit/576161d15423f58281e384174d28c9f9be7941a1
>>  The essential change for this JEP, including the `@Deprecate` annotations 
>> and spec change. It also update the default value of the 
>> `java.security.manager` system property to "disallow", and necessary test 
>> change following this update.
>> 2. 
>> https://github.com/openjdk/jdk/commit/26a54a835e9f84aa528740a7c5c35d07355a8a66
>>  Manual changes to several files so that the next commit can be generated 
>> programatically.
>> 3. 
>> https://github.com/openjdk/jdk/commit/eb6c566ff9207974a03a53335e0e697cffcf0950
>>  Automatic changes to other source files to avoid javac warnings on 
>> deprecation for removal
>> 
>> The 1st and 2nd commits should be reviewed carefully. The 3rd one is 
>> generated programmatically, see the comment below for more details. If you 
>> are only interested in a portion of the 3rd commit and would like to review 
>> it as a separate file, please comment here and I'll generate an individual 
>> webrev.
>> 
>> Due to the size of this PR, no attempt is made to update copyright years for 
>> any file to minimize unnecessary merge conflict.
>> 
>> Furthermore, since the default value of `java.security.manager` system 
>> property is now "disallow", most of the tests calling 
>> `System.setSecurityManager()` need to launched with 
>> `-Djava.security.manager=allow`. This is covered in a different PR at 
>> https://github.com/openjdk/jdk/pull/4071.
>> 
>> Update: the deprecation annotations and javadoc tags, build, compiler, 
>> core-libs, hotspot, i18n, jmx, net, nio, security, and serviceability are 
>> reviewed. Rest are 2d, awt, beans, sound, and swing.
>
> Weijun Wang has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   fixing awt/datatransfer/DataFlavor/DataFlavorRemoteTest.java

test/jdk/java/awt/datatransfer/DataFlavor/DataFlavorRemoteTest.java line 59:

> 57: ProcessCommunicator
> 58: .executeChildProcess(Consumer.class, new 
> String[0]);
> 59: if (!"Hello".equals(processResults.getStdOut())) {

Who or what prompted this change ?

-

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

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

[Phil Race]

On Wed, 17 Feb 2021 12:36:10 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 88 commits:
> 
>  - Merge remote-tracking branch 'upstream/jdk/master' into jdk-macos
>  - Re-do safefetch.hpp
>  - Merge remote-tracking branch 'origin/jdk/8261075-stubroutines-inline' into 
> jdk-macos
>  - stubRoutines.inline.hpp -> safefetch.hpp
>  - Update copyrights
>  - Merge remote-tracking branch 'upstream/jdk/master' into 
> 8261075-stubroutines-inline
>  - Merge remote-tracking branch 'upstream/jdk/master' into 
> 8261075-stubroutines-inline
>  - Extract SafeFetch32/N to stubRoutines.inline.hpp
>  - Revert "Extract SafeFetch32/N to stubRoutines.inline.hpp"
>
>This reverts commit b873c25f31dd21349d140b790713cc9ccb5f2dc0.
>  - Merge pull request #9 from VladimirKempik/pull/2200
>
>Removed unused variables
>  - ... and 78 more: 
> https://git.openjdk.java.net/jdk/compare/b955f85e...ab72613c

Looks like the compiler warning changess are now the only desktop changes.
That is fine by me.

-

Marked as reviewed by prr (Reviewer).

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

Re: RFR: 8257858: [macOS]: Remove JNF dependency from libosxsecurity/KeystoreImpl.m [v3]

[Phil Race]

On Fri, 29 Jan 2021 22:47:52 GMT, Weijun Wang  wrote:

>> This fix covers both
>> 
>> - [[macOS]: Remove JNF dependency from 
>> libosxsecurity/KeystoreImpl.m](https://bugs.openjdk.java.net/browse/JDK-8257858)
>> - [[macOS]: Remove JNF dependency from 
>> libosxkrb5/SCDynamicStoreConfig.m](https://bugs.openjdk.java.net/browse/JDK-8257860)
>
> Weijun Wang has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   phil comment

Marked as reviewed by prr (Reviewer).

-

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

Re: RFR: 8257858: [macOS]: Remove JNF dependency from libosxsecurity/KeystoreImpl.m [v2]

[Phil Race]

On Fri, 29 Jan 2021 21:47:32 GMT, Weijun Wang  wrote:

>> src/java.base/macosx/native/libosxsecurity/KeystoreImpl.m line 619:
>> 
>>> 617: (*env)->ReleaseCharArrayElements(env, passwordObj, 
>>> passwordChars,
>>> 618: JNI_ABORT);
>>> 619: }
>> 
>> Although you have it in the later code, here you are missing
>>  @catch (NSException *e) { 
>>  NSLog(@"%@", [e callStackSymbols]); 
>>  }
>
> Will add.
> 
> BTW, will these be shown on stdout or stderr? If so, is this a good idea?

should be stderr. Whether to log is up to you. You could wrap it in something 
that checks for a debug build.
The idea is that if this ever happens there is a serious problem. NSException 
is only thrown (by the OS frameworks) in supposedly fatal scenarios.

>> src/java.security.jgss/macosx/native/libosxkrb5/SCDynamicStoreConfig.m line 
>> 57:
>> 
>>> 55: }
>>> 56: }
>>> 57: (*localVM)->DetachCurrentThread(localVM);
>> 
>> I think you only want to detach if you actually attached ! you don't want to 
>> be detaching VM threads.
>
> So I should remember how env was retrieved and only detach when it's from 
> `AttachCurrentThreadAsDaemon`? In fact, in my test the plain `GetEnv` has 
> never succeeded and it was always attached.

Yes that is the idea. BTW which thread was it attached to in what you saw ?

-

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

Re: RFR: 8257858: [macOS]: Remove JNF dependency from libosxsecurity/KeystoreImpl.m [v2]

[Phil Race]

On Fri, 29 Jan 2021 14:57:56 GMT, Weijun Wang  wrote:

>> This fix covers both
>> 
>> - [[macOS]: Remove JNF dependency from 
>> libosxsecurity/KeystoreImpl.m](https://bugs.openjdk.java.net/browse/JDK-8257858)
>> - [[macOS]: Remove JNF dependency from 
>> libosxkrb5/SCDynamicStoreConfig.m](https://bugs.openjdk.java.net/browse/JDK-8257860)
>
> Weijun Wang has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   same behavior as before -- empty realm map

make/modules/java.security.jgss/Lib.gmk line 84:

> 82: $(call SET_SHARED_LIBRARY_ORIGIN), \
> 83: LIBS := -framework Cocoa -framework SystemConfiguration \
> 84: -framework Kerberos -ljava, \

The need to add -ljava is interesting. It implies we were getting something 
from the platform that usually we'd expect to come from the JDK itself ??

src/java.base/macosx/classes/apple/security/KeychainStore.java line 820:

> 818: private void createKeyEntry(String alias, long creationDate, long 
> secKeyRef,
> 819: long[] secCertificateRefs, byte[][] 
> rawCertData) {
> 820: KeyEntry ke = new KeyEntry();

removing these exceptions is presumably just clean up - not directly related ??

src/java.base/macosx/native/libosxsecurity/KeystoreImpl.m line 28:

> 26: #import "apple_security_KeychainStore.h"
> 27: #include "jni.h"
> 28: #include "jni_util.h"

jni_util.h includes jni.h so I don't understand the need for this change.
Also why did you change import to include ? import is the Obj-C norm ...

src/java.base/macosx/native/libosxsecurity/KeystoreImpl.m line 619:

> 617: (*env)->ReleaseCharArrayElements(env, passwordObj, 
> passwordChars,
> 618: JNI_ABORT);
> 619: }

Although you have it in the later code, here you are missing
 @catch (NSException *e) { 
 NSLog(@"%@", [e callStackSymbols]); 
 }

src/java.security.jgss/macosx/native/libosxkrb5/SCDynamicStoreConfig.m line 41:

> 39: if ([keys count] == 0) return;
> 40: if (![keys containsObject:KERBEROS_DEFAULT_REALMS] && ![keys 
> containsObject:KERBEROS_DEFAULT_REALM_MAPPINGS]) return;
> 41: //JNFPerformEnvBlock(JNFThreadDetachOnThreadDeath | 
> JNFThreadSetSystemClassLoaderOnAttach | JNFThreadAttachAsDaemon, ^(JNIEnv 
> *env) {

remove commented out code

src/java.security.jgss/macosx/native/libosxkrb5/SCDynamicStoreConfig.m line 57:

> 55: }
> 56: }
> 57: (*localVM)->DetachCurrentThread(localVM);

I think you only want to detach if you actually attached ! you don't want to be 
detaching VM threads.

-

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

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

[Phil Race]

On Mon, 25 Jan 2021 23:34:04 GMT, Phil Race  wrote:

>> that sounds good to me, I am just afraid to break intel mac on older macos 
>> versions with this change.
>
> That may actually be a valid concern. Both say macOS 10.12+ ...  which might 
> conflict with the 10.9 target.

Maybe you should just file a bug after all for this to be dealt with separately.
Certainly if it is NOT fixed now such a bug needs to be filed.

-

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

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

[Phil Race]

On Mon, 25 Jan 2021 22:47:33 GMT, Vladimir Kempik  wrote:

>> 1) I meant change to NSWindowStyleMaskBorderless from NSBorderlessWindowMask
>> 2) So maybe rather than the deprecation suppression  you could change both 
>> constants to the new ones.
>> Ordinarily I'd say let someone else do that but this looks like a simple 
>> obvious change and is dwarfed by all the other changes going on for Mac ARM 
>> ...
>
> that sounds good to me, I am just afraid to break intel mac on older macos 
> versions with this change.

That may actually be a valid concern. Both say macOS 10.12+ ...  which might 
conflict with the 10.9 target.

-

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

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

[Phil Race]

On Mon, 25 Jan 2021 22:25:48 GMT, Vladimir Kempik  wrote:

>> Are you doing something somewhere to change the target version of macOS or 
>> SDK ? I had no such problem.
>> I think we currently target a macOS 10.9 and if you are changing that it 
>> would need discussion.
>> If you are changing it only for Mac ARM that may make more sense .. 
>> 
>> And these appear to be just API churn by Apple
>> NSAlphaFirstBitmapFormat is replaced by NSBitmapFormatAlphaFirst
>> 
>> https://developer.apple.com/documentation/appkit/nsbitmapformat/nsbitmapformatalphafirst?language=objc
>> 
>> NSBorderlessWindowMask is replaced by NSWindowStyleMask
>> 
>> https://developer.apple.com/documentation/appkit/nswindowstylemask?language=occ
>
> Min_macos version is changed to 11.0 for macos_aarch64
> 
> https://github.com/openjdk/jdk/pull/2200/files/0c2cb0a372bf1a8607810d773b53d6959616a816#diff-7cd97cdbeb3053597e5d6659016cdf0f60b2c412bd39934a43817ee0b717b7a7R136

1) I meant change to NSWindowStyleMaskBorderless from NSBorderlessWindowMask
2) So maybe rather than the deprecation suppression  you could change both 
constants to the new ones.
Ordinarily I'd say let someone else do that but this looks like a simple 
obvious change and is dwarfed by all the other changes going on for Mac ARM ...

-

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

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

[Phil Race]

On Mon, 25 Jan 2021 21:18:59 GMT, Vladimir Kempik  wrote:

>> Hello
>> I believe it was a workaround for issues with xcode 12.2 in early beta days.
>> Those issues were fixed later in upstream jdk, so most likely we need to 
>> remove these workarounds.
>
> It seems these workarounds are still needed:
> 
> jdk/src/java.desktop/macosx/native/libsplashscreen/splashscreen_sys.m:300:39: 
> error: 'NSAlphaFirstBitmapFormat' is deprecated: first deprecated in macOS 
> 10.14 [-Werror,-Wdeprecated-declarations]
> bitmapFormat: NSAlphaFirstBitmapFormat | 
> NSAlphaNonpremultipliedBitmapFormat
>   ^~~~
>   NSBitmapFormatAlphaFirst
> 
> jdk/src/java.desktop/macosx/native/libsplashscreen/splashscreen_sys.m:438:34: 
> error: 'NSBorderlessWindowMask' is deprecated: first deprecated in macOS 
> 10.12 [-Werror,-Wdeprecated-declarations]
>   styleMask: NSBorderlessWindowMask
>  ^~
>  NSWindowStyleMaskBorderless

Are you doing something somewhere to change the target version of macOS or SDK 
? I had no such problem.
I think we currently target a macOS 10.9 and if you are changing that it would 
need discussion.
If you are changing it only for Mac ARM that may make more sense .. 

And these appear to be just API churn by Apple
NSAlphaFirstBitmapFormat is replaced by NSBitmapFormatAlphaFirst

https://developer.apple.com/documentation/appkit/nsbitmapformat/nsbitmapformatalphafirst?language=objc

NSBorderlessWindowMask is replaced by NSWindowStyleMask

https://developer.apple.com/documentation/appkit/nswindowstylemask?language=occ

-

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

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

[Phil Race]

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

make/modules/java.desktop/lib/Awt2dLibraries.gmk line 573:

> 571: EXTRA_HEADER_DIRS := $(LIBFONTMANAGER_EXTRA_HEADER_DIRS), \
> 572: WARNINGS_AS_ERRORS_xlc := false, \
> 573: DISABLED_WARNINGS_clang := deprecated-declarations, \

I see this added here and in another location. What is deprecated ? You really 
need to explain these sorts of things.
I've built JDK using Xcode 12.3 and not had any need for this.

-

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

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

[Phil Race]

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 prr (Reviewer).

src/java.desktop/share/native/libharfbuzz/hb-common.h line 113:

> 111: 
> 112: #define HB_TAG(c1,c2,c3,c4) 
> ((hb_tag_t)uint32_t)(c1)&0xFF)<<24)|(((uint32_t)(c2)&0xFF)<<16)|(((uint32_t)(c3)&0xFF)<<8)|((uint32_t)(c4)&0xFF)))
> 113: #define HB_UNTAG(tag)   (char)(((tag)>>24)&0xFF), 
> (char)(((tag)>>16)&0xFF), (char)(((tag)>>8)&0xFF), (char)((tag)&0xFF)

I need a robust explanation of these changes.
They are not mentioned, nor are they in upstream harfbuzz.
Likely these should be pushed to upstream first if there is a reason for them.

src/java.desktop/share/native/libharfbuzz/hb-coretext.cc line 193:

> 191:* crbug.com/549610 */
> 192:   // 0x0007 stands for "kCTVersionNumber10_10", see CoreText.h
> 193: #if TARGET_OS_OSX && MAC_OS_X_VERSION_MIN_REQUIRED < __MAC_10_10

I need a robust explanation of these changes.
They are not mentioned, nor are they in upstream harfbuzz.
Likely these should be pushed to upstream first if there is a reason for them.

-

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

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

[Phil Race]

On Tue, 15 Dec 2020 22:56:15 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.
>> 
>> ### Modules reviewed
>> 
>> - [x] java.base
>> - [ ] java.desktop
>> - [x] jdk.compiler
>> - [x] java.se
>
> Magnus Ihse Bursie has updated the pull request with a new target base due to 
> a merge or a rebase. The incremental webrev excludes the unrelated changes 
> brought in by the merge/rebase. The pull request contains eight additional 
> commits since the last revision:
> 
>  - Update comment refering to "make" dir
>  - Move new symbols to jdk.compiler
>  - Merge branch 'master' into shuffle-data
>  - Move macosxicons from share to macosx
>  - Move to share/data, and move jdwp.spec to java.se
>  - Update references in test
>  - Step 2: Update references
>  - First stage, move actual data files

Marked as reviewed by prr (Reviewer).

-

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

Re: RFR: 8080272 Refactor I/O stream copying to use java.io.InputStream.transferTo

[Phil Race]

On Sun, 20 Dec 2020 20:22:48 GMT, Andrey Turbanov 
 wrote:

>> jrtfs is compiled twice, the second is to --release 8 so it can be packaged 
>> into jrt-fs.jar for use by IDEs/tools running on older JDK releases. So need 
>> to be careful with the changes here as it will likely causing build breakage.
>> 
>> We try to keep the changes to ASM to a minimum, might be better to leave 
>> this change out of the patch.
>> 
>> One or two of the sources changes should probably uses Files.copy, e.g. 
>> ZipPath, sjavac/CopyFile.java.
>
>> One or two of the sources changes should probably uses Files.copy, e.g. 
>> ZipPath, sjavac/CopyFile.java.
> 
> Good idea! Replaced in few places. But not in ZipPath: it's actually 
> implementation of underlying call from Files.copy: 
> `jdk.nio.zipfs.ZipFileSystemProvider#copy`. So, `Files.copy` call will be 
> recursive.

So these changes are all over the place which means no one person knows how to 
test all of it.
Have you run the sound, swing tests, and the  printing tests on unix and 
windows ?
For printing for sure you'll need to do some manual testing.

-

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

Re: RFR: 8255299: Drop explicit zeroing at instantiation of Atomic* objects

[Phil Race]

On Thu, 22 Oct 2020 20:46:15 GMT, Сергей Цыпанов 
 wrote:

> As discussed in https://github.com/openjdk/jdk/pull/510 there is never a 
> reason to explicitly instantiate any instance of `Atomic*` class with its 
> default value, i.e. `new AtomicInteger(0)` could be replaced with `new 
> AtomicInteger()` which is faster:
> @State(Scope.Thread)
> @OutputTimeUnit(TimeUnit.NANOSECONDS)
> @BenchmarkMode(value = Mode.AverageTime)
> public class AtomicBenchmark {
>   @Benchmark
>   public Object defaultValue() {
> return new AtomicInteger();
>   }
>   @Benchmark
>   public Object explicitValue() {
> return new AtomicInteger(0);
>   }
> }
> THis benchmark demonstrates that `explicitValue()` is much slower:
> Benchmark  Mode  Cnt   Score   Error  Units
> AtomicBenchmark.defaultValue   avgt   30   4.778 ± 0.403  ns/op
> AtomicBenchmark.explicitValue  avgt   30  11.846 ± 0.273  ns/op
> So meanwhile https://bugs.openjdk.java.net/browse/JDK-8145948 is still in 
> progress we could trivially replace explicit zeroing with default 
> constructors gaining some performance benefit with no risk.
> 
> I've tested the changes locally, both tier1 and tier 2 are ok. 
> 
> Could one create an issue for tracking this?

client changes are fine

-

Marked as reviewed by prr (Reviewer).

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

Re: RFR: 8252999: Cleanup: replace .equals("") with .isEmpty() within all codebase

[Phil Race]

On Sun, 6 Sep 2020 13:57:19 GMT, Dmitriy Dumanskiy 
 wrote:

> **isEmpty** is faster + has less byte code + easier to read. Benchmarks could 
> be found
>   
> [here](https://medium.com/javarevisited/micro-optimizations-in-java-string-equals-22be19fd8416).

1) This is un-necessary churn.
2) I can't even be sure I am finding the ones in my area because there's so 
much here
3) The ones I can find have no need of whatever performance improvement this 
might bring.
I think this whole PR should be withdrawn, and there may an attempt at 
measuring the benefits of this for the various
cases before submitting in appropriate smaller chunks. But I'll tell you now I 
don't see a need for the test updates
you are making.

-

Changes requested by prr (Reviewer).

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

Re: RFR: JDK-8200178 Remove mapfiles for JDK native libraries

[Phil Race]

http://cr.openjdk.java.net/~ihse/JDK-8200178-remove-mapfiles/webrev.01/src/java.desktop/share/native/libmlib_image/mlib_image_proto.h.udiff.html

The variable definitions here are now misaligned.

..and added 2d-dev since many of these native changes are in 2d.

-phil.

On 03/23/2018 10:33 AM, Phil Race wrote:

There are a lot of changes in the desktop libraries.
Doing mach5 tier1/2/3 testing is not nearly sufficient to cover those 
since
only tier3 has any UI tests and it barely uses anything that's touched 
here.

So since testing seems to be wise, then I think you should do a
jtreg desktop group run on Linux & Windows.
You can probably skip Mac since it is unaffected and I think Linux 
will cover Solaris here.

You should also do some headless testing.

It could take some time to review this properly and decide what 
changes are OK,
what changes are something we should clean up later, and what changes 
are something

that ought to be addressed now ..

I think I'd be mainly concerned that something fails due to a missing 
symbol, or
that for newly exported symbols if we ended up with duplicate symbols 
as a result.


The results of a test run will add confidence here.

BTW I don't think you are right that
java.desktop:/libawt_headless: The following symbols are now also 
exported due to JNIEXPORT (they were previously

..
 X11SurfaceData_GetOps

It looks to me like it was previously exported.


-phil.



On 03/23/2018 06:56 AM, Magnus Ihse Bursie wrote:
With modern compilers, we can use compiler directives (such as 
_attribute__((visibility("default"))), or __declspec(dllexport)) to 
control symbol visibility, directly in the source code. This has 
historically not been present on all compilers, so we had to resort 
to using mapfiles (also known as linker scripts).


This is no longer the case. Now all compilers we use support symbol 
visibility directives, in one form or another. We should start using 
this. Since this has been the only way to control symbol visibility 
on Windows, for most of the shared code, we already have proper 
JNIEXPORT decorations in place.


If we fix the remaining platform-specific files to have proper 
JNIEXPORT tagging, then we can finally get rid of mapfiles.


This fix removed mapfiles for all JDK libraries. It does not touch 
hotspot libraries nor JDK executables; they will have to wait for a 
future fix -- this was complex enough. This change will not have any 
impact on macosx, since we do not use mapfiles there, but instead 
export all symbols. (This is not a good idea, but I'll address that 
separately.) This change will also have a minimal impact on Windows. 
The only reason Windows is impacted at all, is that some changes 
needed by Solaris and Linux were simpler to fix for all platforms.


I have strived for this change to have no impact on the actual 
generated code. Unfortunately, this was not possible to fully 
achieve. I do not believe that these changes will have any actual 
impact on the product, though. I will present the differences more in 
detail further down. Those who are not interested can probably skip 
that.


The patch has passed tier1 testing and is currently running tier2 and 
tier3. Since the running code is more or less (see caveat below) 
unmodified, I don't expect any testing issues.


Bug: https://bugs.openjdk.java.net/browse/JDK-8200178
WebRev: 
http://cr.openjdk.java.net/~ihse/JDK-8200178-remove-mapfiles/webrev.01


Details on changes:
Most of the source code changes are (unsurprisingly) in java.base and 
java.desktop. Remaining changes are in jdk.crypto.ucrypto, 
jdk.hotspot.agent, jdk.jdi and jdk.jdwp.agent.


Source code changes does almost to 100% consists in decorating an 
exported function with JNIEXPORT. I have also followed the 
long-standing convention of adding JNICALL. This is a no-op on 
non-Windows platforms, so for most of the changes this is purely 
cosmetic (and possibly adding in robustness, should the function ever 
be used on Windows in the future). I have also followed the stylistic 
convention of putting "JNIEXPORT  JNICALL" on a separate 
line. For some functions, however, this might cause a change in 
calling convention on Windows. Since this can not apply to exported 
functions on Windows (otherwise they would already have had 
JNIEXPORT), I do not think this matters anything.


A few libraries did not have a mapfile, on Linux and/or Solaris. This 
actually meant that all symbols were exported. It is highly unclear 
if this was known and intended by the original make rule writer. I 
have emulated this by adding the flag $(EXPORT_ALL_SYMBOLS) to these 
libraries. Hopefully, we can remove this flag and fix proper exported 
symbols in the future.


I have run the complete build using COMPARE_BUILD, and made a 
thourough analysis of the differences for Linux and Solaris. All 
native libraries have symbol differences, but most of them are 
trivial and/or harmless. As a result, most libraries 

Re: RFR: JDK-8200178 Remove mapfiles for JDK native libraries

[Phil Race]

There are a lot of changes in the desktop libraries.
Doing mach5 tier1/2/3 testing is not nearly sufficient to cover those since
only tier3 has any UI tests and it barely uses anything that's touched here.
So since testing seems to be wise, then I think you should do a
jtreg desktop group run on Linux & Windows.
You can probably skip Mac since it is unaffected and I think Linux will 
cover Solaris here.

You should also do some headless testing.

It could take some time to review this properly and decide what changes 
are OK,
what changes are something we should clean up later, and what changes 
are something

that ought to be addressed now ..

I think I'd be mainly concerned that something fails due to a missing 
symbol, or
that for newly exported symbols if we ended up with duplicate symbols as 
a result.


The results of a test run will add confidence here.

BTW I don't think you are right that
java.desktop:/libawt_headless: The following symbols are now also 
exported due to JNIEXPORT (they were previously

..
 X11SurfaceData_GetOps

It looks to me like it was previously exported.


-phil.



On 03/23/2018 06:56 AM, Magnus Ihse Bursie wrote:
With modern compilers, we can use compiler directives (such as 
_attribute__((visibility("default"))), or __declspec(dllexport)) to 
control symbol visibility, directly in the source code. This has 
historically not been present on all compilers, so we had to resort to 
using mapfiles (also known as linker scripts).


This is no longer the case. Now all compilers we use support symbol 
visibility directives, in one form or another. We should start using 
this. Since this has been the only way to control symbol visibility on 
Windows, for most of the shared code, we already have proper JNIEXPORT 
decorations in place.


If we fix the remaining platform-specific files to have proper 
JNIEXPORT tagging, then we can finally get rid of mapfiles.


This fix removed mapfiles for all JDK libraries. It does not touch 
hotspot libraries nor JDK executables; they will have to wait for a 
future fix -- this was complex enough. This change will not have any 
impact on macosx, since we do not use mapfiles there, but instead 
export all symbols. (This is not a good idea, but I'll address that 
separately.) This change will also have a minimal impact on Windows. 
The only reason Windows is impacted at all, is that some changes 
needed by Solaris and Linux were simpler to fix for all platforms.


I have strived for this change to have no impact on the actual 
generated code. Unfortunately, this was not possible to fully achieve. 
I do not believe that these changes will have any actual impact on the 
product, though. I will present the differences more in detail further 
down. Those who are not interested can probably skip that.


The patch has passed tier1 testing and is currently running tier2 and 
tier3. Since the running code is more or less (see caveat below) 
unmodified, I don't expect any testing issues.


Bug: https://bugs.openjdk.java.net/browse/JDK-8200178
WebRev: 
http://cr.openjdk.java.net/~ihse/JDK-8200178-remove-mapfiles/webrev.01


Details on changes:
Most of the source code changes are (unsurprisingly) in java.base and 
java.desktop. Remaining changes are in jdk.crypto.ucrypto, 
jdk.hotspot.agent, jdk.jdi and jdk.jdwp.agent.


Source code changes does almost to 100% consists in decorating an 
exported function with JNIEXPORT. I have also followed the 
long-standing convention of adding JNICALL. This is a no-op on 
non-Windows platforms, so for most of the changes this is purely 
cosmetic (and possibly adding in robustness, should the function ever 
be used on Windows in the future). I have also followed the stylistic 
convention of putting "JNIEXPORT  JNICALL" on a separate 
line. For some functions, however, this might cause a change in 
calling convention on Windows. Since this can not apply to exported 
functions on Windows (otherwise they would already have had 
JNIEXPORT), I do not think this matters anything.


A few libraries did not have a mapfile, on Linux and/or Solaris. This 
actually meant that all symbols were exported. It is highly unclear if 
this was known and intended by the original make rule writer. I have 
emulated this by adding the flag $(EXPORT_ALL_SYMBOLS) to these 
libraries. Hopefully, we can remove this flag and fix proper exported 
symbols in the future.


I have run the complete build using COMPARE_BUILD, and made a 
thourough analysis of the differences for Linux and Solaris. All 
native libraries have symbol differences, but most of them are trivial 
and/or harmless. As a result, most libraries have disasm differences 
as well, but these too seem trivial and harmless. The differences in 
symbols that are common to all libraries include:
 * Internal symbols such as __bss_start, _edata, _end and _fini are 
now global. (They are imported as such from the compiler 
libraries/archives, and we have no linker script to override this 

Re: [OpenJDK 2D-Dev] RFR(M): 8170525: Fix minor issues in awt coding

[Phil Race]

Sorry. it is
ops->GetRasInfo(env, ops, lockInfo);
that initialises it ..


That is still before the dereference
Anyway, what was the reason for updating one function, but not the other.
I don't mind the change, but the inconsistency looks odd.

-phil.

On 12/01/2016 03:03 AM, Lindenmaier, Goetz wrote:

Hi Phil,


Did you actually compile this ? The variable is called "rasBase", not
"resBase".

Yes, I compiled it, and I fixed the error, but that was another repo
I use for the coverity checks.  Somehow it did not find its way into
the webrev.

I don't see where ops->Lock() initializes this field.
ops->GetRasInfo(env, ops, lockInfo); does so if it resolves
to BufImg_GetRasInfo().  I can't look in our code scan results
because the issue is gone after fixing it, that lists the path
of execution that leads to the issue.
If you are sure this is correct I will remove the initialization,
else I will also put it into the other method.

DBN_GetPixelPointer checks lockInfo->rasBase for NULL and
does what looks like the error case if it's NULL. Therefore NULL
seemed a good initialization to me.

Best regards,
   Goetz.




-Original Message-
From: Phil Race [mailto:philip.r...@oracle.com]
Sent: Mittwoch, 30. November 2016 20:59
To: Sergey Bylokhov <sergey.bylok...@oracle.com>; Lindenmaier, Goetz
<goetz.lindenma...@sap.com>; Vincent Ryan <vincent.x.r...@oracle.com>
Cc: awt-...@openjdk.java.net; 2d-dev <2d-...@openjdk.java.net>; security-
d...@openjdk.java.net
Subject: Re: [OpenJDK 2D-Dev]  RFR(M): 8170525: Fix minor issues
in awt coding

Hi Goetz,


   DataBufferNative.c
Using uninitialized value lockInfo.rasBase when calling DBN_GetPixelPointer.

75 lockInfo.resBase = NULL;

Did you actually compile this ? The variable is called "rasBase", not
"resBase".

And strictly there is no problem since inside  DBN_GetPixelPointer
the code calls ops->Lock which should initialise this.
A "rasBase" of 0 isn't really any better than a random one ..

Also I don't see why there's a problem here and not in
the function immediately following since it is the exact same case.

-phil.

On 11/30/2016 10:00 AM, Sergey Bylokhov wrote:

cc 2d-dev.

On 30.11.16 18:41, Lindenmaier, Goetz wrote:

Hi Vincent,

thanks for the quit review!
Good catch that I lost the change to p11_mutex.c ... I had to change
it and it fell out of my patches.
I edited the Last Modified Date, and also updated the copyright
messages.
New webrev:
http://cr.openjdk.java.net/~goetz/wr16/8170525-awt-dev/

Best regards,
   Goetz.

(Am I correct that your openJdk name is Vinnie?)


-Original Message-
From: Vincent Ryan [mailto:vincent.x.r...@oracle.com]
Sent: Mittwoch, 30. November 2016 14:53
To: Lindenmaier, Goetz <goetz.lindenma...@sap.com>
Cc: awt-...@openjdk.java.net; security-dev@openjdk.java.net
Subject: Re: RFR(M): 8170525: Fix minor issues in awt coding

Hello Goetz,

Please modify the bug summary to reference ECC too.
Your ECC changes look fine but the ‘Last Modified Date’ line in the
4 source
code headers will need to be updated/added.

BTW p11_mutex.c is listed below but appears to be missing from the
webrev.

Thanks.




 On 30 Nov 2016, at 13:12, Lindenmaier, Goetz
<goetz.lindenma...@sap.com <mailto:goetz.lindenma...@sap.com> >

wrote:

 Hi,

 I’d like to propose a row of smaller fixes where code is noted
down a
bit questionable.
 SAP’s quality process requires that we fix these in our internal
delivery,
and I
 Would like to share my fixes with openJdk.  Some of these fixes
are of
more
 theoretical nature as how I understand the code paths never
allow the
 problematic situation, but fixing it nevertheless assures that
nothing is
 overseen if the code changes.  Most changes are in libawt_xawt,
some
 are in libsunec.

 I’d appreciate a review:
 http://cr.openjdk.java.net/~goetz/wr16/8170525-awt/webrev.01/

 Changes in detail:

 awt_InputMethod.c:

 One might overrun the 100 byte fixed-size string
statusWindow->status
by copying text->string.multi_byte without checking the length.

 gtk3_interface.c:

 This less-than-zero comparison of an unsigned value is never true.

 Using uninitialized value color. Field color.alpha is
uninitialized.
 E.g. used at gtk3_interface.c:2287.

 XToolkit.c

 Using uninitialized value ret_timeout.
 E.g. in XToolkit.c:6809.

 XWindow.c

 Argument is incompatible with corresponding format string
conversion.

 splashscreen_sys.c

 Overflowed or truncated value (or a value computed from an
overflowed or truncated value) (gdk_scale > 0) ? native_scale *
(double)gdk_scale : native_scale used as return value.

 ec.c

 Using uninitialized value k.dp when calling mp_clear.

 ecdecode.c

 You might overrun the 291 byte fixed-size string genenc by copying
curveParams->geny w

Re: [OpenJDK 2D-Dev] RFR(M): 8170525: Fix minor issues in awt coding

[Phil Race]

Hi Goetz,


  DataBufferNative.c
Using uninitialized value lockInfo.rasBase when calling DBN_GetPixelPointer.


  75 lockInfo.resBase = NULL;

Did you actually compile this ? The variable is called "rasBase", not 
"resBase".


And strictly there is no problem since inside  DBN_GetPixelPointer
the code calls ops->Lock which should initialise this.
A "rasBase" of 0 isn't really any better than a random one ..

Also I don't see why there's a problem here and not in
the function immediately following since it is the exact same case.

-phil.

On 11/30/2016 10:00 AM, Sergey Bylokhov wrote:

cc 2d-dev.

On 30.11.16 18:41, Lindenmaier, Goetz wrote:

Hi Vincent,

thanks for the quit review!
Good catch that I lost the change to p11_mutex.c ... I had to change
it and it fell out of my patches.
I edited the Last Modified Date, and also updated the copyright 
messages.

New webrev:
http://cr.openjdk.java.net/~goetz/wr16/8170525-awt-dev/

Best regards,
  Goetz.

(Am I correct that your openJdk name is Vinnie?)


-Original Message-
From: Vincent Ryan [mailto:vincent.x.r...@oracle.com]
Sent: Mittwoch, 30. November 2016 14:53
To: Lindenmaier, Goetz 
Cc: awt-...@openjdk.java.net; security-dev@openjdk.java.net
Subject: Re: RFR(M): 8170525: Fix minor issues in awt coding

Hello Goetz,

Please modify the bug summary to reference ECC too.
Your ECC changes look fine but the ‘Last Modified Date’ line in the 
4 source

code headers will need to be updated/added.

BTW p11_mutex.c is listed below but appears to be missing from the 
webrev.


Thanks.




On 30 Nov 2016, at 13:12, Lindenmaier, Goetz
 > wrote:

Hi,

I’d like to propose a row of smaller fixes where code is noted 
down a

bit questionable.
SAP’s quality process requires that we fix these in our internal 
delivery,

and I
Would like to share my fixes with openJdk.  Some of these fixes 
are of

more
theoretical nature as how I understand the code paths never 
allow the
problematic situation, but fixing it nevertheless assures that 
nothing is
overseen if the code changes.  Most changes are in libawt_xawt, 
some

are in libsunec.

I’d appreciate a review:
http://cr.openjdk.java.net/~goetz/wr16/8170525-awt/webrev.01/

Changes in detail:

awt_InputMethod.c:

One might overrun the 100 byte fixed-size string 
statusWindow->status

by copying text->string.multi_byte without checking the length.

gtk3_interface.c:

This less-than-zero comparison of an unsigned value is never true.

Using uninitialized value color. Field color.alpha is 
uninitialized.

E.g. used at gtk3_interface.c:2287.

XToolkit.c

Using uninitialized value ret_timeout.
E.g. in XToolkit.c:6809.

XWindow.c

Argument is incompatible with corresponding format string 
conversion.


splashscreen_sys.c

Overflowed or truncated value (or a value computed from an
overflowed or truncated value) (gdk_scale > 0) ? native_scale *
(double)gdk_scale : native_scale used as return value.

ec.c

Using uninitialized value k.dp when calling mp_clear.

ecdecode.c

You might overrun the 291 byte fixed-size string genenc by copying
curveParams->geny without checking the length.
Added sanity check before doing the string concatenation.

ecl_mult.c

Using uninitialized value kt.flag when calling 
*group->point_mul. (The

function pointer resolves to ec_GF2m_pt_mul_mont.)

mpi.c

Using uninitialized value s. Field s.flag is uninitialized when 
calling

s_mp_exch.
Using uninitialized value tmp. Field tmp.flag is uninitialized when
calling s_mp_exch
Using uninitialized value t.dp when calling mp_clear.

p11_mutex.c

Using uninitialized value *ckpInitArgs. Field ckpInitArgs->flags is
uninitialized when calling memcpy.


DataBufferNative.c

Using uninitialized value lockInfo.rasBase when calling
BN_GetPixelPointer.

fontpath.c

You might overrun the 512 byte fixed-size string fontDirPath by 
copying

DirP->name[index] without checking the length.

Re: AWT Dev RFR: JDK-8074096 Disable (most) native warnings in JDK on a per-library basis

[Phil Race]

I like the overall approach.
We can then attack the warnings lib by lib and/or platform by platform.

I do want to mention that some libs like liblcms are 3rd party open 
source libraries

and it may not always be possible to convince upstream to change their code.

-phil.


On 03/04/2015 08:30 AM, Alan Bateman wrote:

On 04/03/2015 15:03, Magnus Ihse Bursie wrote:
I believe all warnings are important to check! Unfortunately, this 
has not been done for a lot of warnings for a lot of time. :(
Right, although in the past we had some areas close to be cleared of 
warnings, it's just that we didn't keep up the effort and of course 
the compilers get more pedantic with each version.




With this framework, it is simple to enable a single warning, 
recompile and see the effect. Hopefully this lowers the threshold far 
enough so that all warnings are given proper attention. The 
incremental build functionality will come in very handy. Just by 
simply removing a warning from the DISABLED_WARNINGS_toolchain on a 
library and running make again, only the files affected will be 
recompiled.

Yes, it looks easy to maintain.




I can easily paste in what warning categories are disabled for a 
specific library, yes.


However, if you are asking me to build each library, individually, 
with warnings re-enabled, and pasting the output, I'd rather not. 
That would be a lot of work, to detangle the output of each 
individual library.
I'm not asking that, it would be too much work. Maybe it's worth 
saving the logs somewhere so that you can point the bugs at it. It 
would also be useful for the bugs to point to your change sets (when 
they are pushed) so that it's obvious which make files were changed to 
silence the warnings.


-Alan

Re: AWT Dev RFR(L) - 2nd round: 8024854: Basic changes and files to build the class library on AIX

[Phil Race]

Looking only at what you needed to change this time round, all seems 
fine now.


-phil.

On 11/26/13 8:23 AM, Volker Simonis wrote:

Hi,

thanks to everybody for the prompt and helpful reviews. Here comes the 
final webrev which incorporates all the corrections and suggestions 
from the second review round:


http://cr.openjdk.java.net/~simonis/webrevs/8024854.v3/ 
http://cr.openjdk.java.net/%7Esimonis/webrevs/8024854.v3/


I've successfully build (and run some smoke tests) with these changes 
on Linux (x86_32, x86_64, ppc64), Solaris/sparcv9, Windows/x86_64, 
MacOSX and AIX (5.3, 7.1).


So if nobody vetoes these changes are ready for push into our staging 
repository.


@Vladimir: can I push them or do you want to run them trough JPRT?

Thank you and best regards,
Volker

PS: compared to the last webrev 
(http://cr.openjdk.java.net/~simonis/webrevs/8024854.v2/ 
http://cr.openjdk.java.net/%7Esimonis/webrevs/8024854.v2/), I've 
slightly changed the following files:


makefiles/lib/Awt2dLibraries.gmk
makefiles/lib/NetworkingLibraries.gmk
makefiles/Setup.gmk
src/aix/classes/sun/awt/fontconfigs/aix.fontconfig.properties
src/aix/classes/sun/nio/ch/AixPollPort.java
src/aix/classes/sun/nio/fs/AixFileSystem.java
src/aix/native/java/net/aix_close.c
src/aix/porting/porting_aix.c
src/share/native/java/net/net_util.c
src/share/native/java/net/net_util.h
src/share/native/sun/awt/medialib/mlib_sys.c
src/solaris/bin/java_md_solinux.c
src/solaris/classes/sun/nio/ch/Port.java
src/solaris/native/java/net/net_util_md.c
src/solaris/native/sun/java2d/x11/XRBackendNative.c
src/solaris/native/sun/management/OperatingSystemImpl.c
src/solaris/native/sun/nio/fs/UnixNativeDispatcher.c
src/windows/native/java/net/net_util_md.c

Most of the changes are cosmetic, except the ones in:

src/share/native/java/net/net_util.c
src/share/native/java/net/net_util.h
src/solaris/native/java/net/net_util_md.c
src/windows/native/java/net/net_util_md.c

where I renamed 'initLocalAddrTable()' to 'platformInit()'. For AIX, 
this method will now call 'aix_close_init()' as suggested by Alan.


The changes to src/solaris/native/com/sun/ 
management/UnixOperatingSystem_md.c are now in 
src/solaris/native/sun/management/OperatingSystemImpl.c because that 
file was moved by an upstream change.




On Wed, Nov 20, 2013 at 7:26 PM, Volker Simonis 
volker.simo...@gmail.com mailto:volker.simo...@gmail.com wrote:


Hi,

this is the second review round for 8024854: Basic changes and
files to build the class library on AIX
https://bugs.openjdk.java.net/browse/JDK-8024854. The previous
reviews can be found at the end of this mail in the references
section.

I've tried to address all the comments and suggestions from the
first round and to further streamline the patch (it perfectly
builds on Linux/x86_64, Linux/PPC664, AIX, Solaris/SPARC and
Windows/x86_64). The biggest change compared to the first review
round is the new aix/ subdirectory which I've now created under
jdk/src and which contains AIX-only code.

The webrev is against our
http://hg.openjdk.java.net/ppc-aix-port/stage repository which has
been recently synchronised with the jdk8 master:

http://cr.openjdk.java.net/~simonis/webrevs/8024854.v2/
http://cr.openjdk.java.net/%7Esimonis/webrevs/8024854.v2/

Below (and in the webrev) you can find some comments which explain
the changes to each file. In order to be able to push this big
change, I need the approval of reviewers from the lib, net, svc,
2d, awt and sec groups. So please don't hesitate to jump at it -
there are enough changes for everybody:)

Thank you and best regards,
Volker

*References:*

The following reviews have been posted so far (thanks Iris for
collecting them :)

- Alan Bateman (lib): Initial comments (16 Sep [2])
- Chris Hegarty (lib/net): Initial comments (20 Sep [3])
- Michael McMahon (net): Initial comments (20 Sept [4])
- Steffan Larsen (svc): APPROVED (20 Sept [5])
- Phil Race (2d): Initial comments  (18 Sept [6]); Additional
comments (15 Oct [7])
- Sean Mullan (sec): Initial comments (26 Sept [8])

[2]:

http://mail.openjdk.java.net/pipermail/ppc-aix-port-dev/2013-September/001045.html
[3]:

http://mail.openjdk.java.net/pipermail/ppc-aix-port-dev/2013-September/001078.html
[4]:

http://mail.openjdk.java.net/pipermail/ppc-aix-port-dev/2013-September/001079.html
[5]:

http://mail.openjdk.java.net/pipermail/ppc-aix-port-dev/2013-September/001077.html
[6]:

http://mail.openjdk.java.net/pipermail/ppc-aix-port-dev/2013-September/001069.html
[7]:
http://mail.openjdk.java.net/pipermail/2d-dev/2013-October/003826.html
[8]:

http://mail.openjdk.java.net/pipermail/ppc-aix-port-dev/2013-September/001081.html


*Detailed change description:*

The new jdk/src/aix subdirectory contains the following new and
AIX-specific files for now

Re: [OpenJDK 2D-Dev] RFR(L) - 2nd round: 8024854: Basic changes and files to build the class library on AIX

[Phil Race]

Hi,
I see you've already received a ton of good feedback on this v2.

I have just a few  things to add.
I don't know what symlinks might exist on AIX but it seems odd to
me that you have :-

138 static char *fullAixFontPath[] = {
 139 /usr/lpp/X11/lib/X11/fonts/Type1,
..

but the paths in the new file aix.fontconfig.properties look like this :-

/usr/lib/X11/fonts/Type1/cour.pfa


Also for the build to pick up a file called
*src/aix/classes/sun/awt/fontconfigs/aix.fontconfig.properties

*it seems like you should have added a section to handle this path to
jdk/makefiles/gendata/GenDataFontConfig.gmk

That seems to be where the new build handles such files.

Are you seeing the .bfc and .src files created ?

At runtime it'll get picked up so so long as System.getProperty(os.name)
returns aix (all lower-case) so I think that's OK. Its the build time part
I'd expect to see but don't.

-phil.

On 11/20/2013 10:26 AM, Volker Simonis wrote:

Hi,

this is the second review round for 8024854: Basic changes and files 
to build the class library on AIX 
https://bugs.openjdk.java.net/browse/JDK-8024854. The previous 
reviews can be found at the end of this mail in the references section.


I've tried to address all the comments and suggestions from the first 
round and to further streamline the patch (it perfectly builds on 
Linux/x86_64, Linux/PPC664, AIX, Solaris/SPARC and Windows/x86_64). 
The biggest change compared to the first review round is the new 
aix/ subdirectory which I've now created under jdk/src and which 
contains AIX-only code.


The webrev is against our 
http://hg.openjdk.java.net/ppc-aix-port/stage repository which has 
been recently synchronised with the jdk8 master:


http://cr.openjdk.java.net/~simonis/webrevs/8024854.v2/ 
http://cr.openjdk.java.net/%7Esimonis/webrevs/8024854.v2/


Below (and in the webrev) you can find some comments which explain the 
changes to each file. In order to be able to push this big change, I 
need the approval of reviewers from the lib, net, svc, 2d, awt and sec 
groups. So please don't hesitate to jump at it - there are enough 
changes for everybody:)


Thank you and best regards,
Volker

*References:*

The following reviews have been posted so far (thanks Iris for 
collecting them :)


- Alan Bateman (lib): Initial comments (16 Sep [2])
- Chris Hegarty (lib/net): Initial comments (20 Sep [3])
- Michael McMahon (net): Initial comments (20 Sept [4])
- Steffan Larsen (svc): APPROVED (20 Sept [5])
- Phil Race (2d): Initial comments  (18 Sept [6]); Additional comments 
(15 Oct [7])

- Sean Mullan (sec): Initial comments (26 Sept [8])

[2]: 
http://mail.openjdk.java.net/pipermail/ppc-aix-port-dev/2013-September/001045.html
[3]: 
http://mail.openjdk.java.net/pipermail/ppc-aix-port-dev/2013-September/001078.html
[4]: 
http://mail.openjdk.java.net/pipermail/ppc-aix-port-dev/2013-September/001079.html
[5]: 
http://mail.openjdk.java.net/pipermail/ppc-aix-port-dev/2013-September/001077.html
[6]: 
http://mail.openjdk.java.net/pipermail/ppc-aix-port-dev/2013-September/001069.html
[7]: 
http://mail.openjdk.java.net/pipermail/2d-dev/2013-October/003826.html
[8]: 
http://mail.openjdk.java.net/pipermail/ppc-aix-port-dev/2013-September/001081.html



*Detailed change description:*

The new jdk/src/aix subdirectory contains the following new and 
AIX-specific files for now:

  src/aix/classes/sun/awt/fontconfigs/aix.fontconfig.properties
  src/aix/classes/sun/nio/ch/AixAsynchronousChannelProvider.java
  src/aix/classes/sun/nio/ch/AixPollPort.java
  src/aix/classes/sun/nio/fs/AixFileStore.java
  src/aix/classes/sun/nio/fs/AixFileSystem.java
  src/aix/classes/sun/nio/fs/AixFileSystemProvider.java
  src/aix/classes/sun/nio/fs/AixNativeDispatcher.java
  src/aix/classes/sun/tools/attach/AixAttachProvider.java
  src/aix/classes/sun/tools/attach/AixVirtualMachine.java
  src/aix/native/java/net/aix_close.c
  src/aix/native/sun/nio/ch/AixPollPort.c
  src/aix/native/sun/nio/fs/AixNativeDispatcher.c
  src/aix/native/sun/tools/attach/AixVirtualMachine.c
  src/aix/porting/porting_aix.c
  src/aix/porting/porting_aix.h


src/aix/porting/porting_aix.c
src/aix/porting/porting_aix.h

  * Added these two files for AIX relevant utility code.
  * Currently these files only contain an implementation of |dladdr|
which is not available on AIX.
  * In the first review round the existing |dladdr| users in the code
either called the version from the HotSpot on AIX
(|src/solaris/native/sun/awt/awt_LoadLibrary.c|) or had a private
copy of the whole implementation
(|src/solaris/demo/jvmti/hprof/hprof_md.c|). This is now not
necessary any more.

The new file layout required some small changes to the makefiles to 
make them aware of the new directory locations:



makefiles/CompileDemos.gmk

  * Add an extra argument to |SetupJVMTIDemo| which can be used to
pass additional source locations.
  * Currently this is only used on AIX for the AIX porting utilities

Re: Review Request: 7164376 Replace use of sun.security.action.LoadLibraryAction

[Phil Race]

All looks good to me. Compiler won't spot misspelled library names so I
did try to check all those are still the same.

-phil.

On 4/26/2012 2:20 PM, Mandy Chung wrote:

Thanks, Sean.  I have fixed the 3 files per your comment.

Mandy

On 4/26/2012 1:59 PM, Sean Mullan wrote:

Looks fine, just a couple of nits.

src/macosx/classes/com/apple/concurrent/LibDispatchNative.java,

  - the closing static brace is not indented the same as the open brace.

src/solaris/classes/sun/management/FileSystemImpl.java
src/windows/classes/sun/management/FileSystemImpl.java

  - line-break coding style is different from all others; probably 
better to be consistent


--Sean

On 04/26/2012 02:49 PM, Mandy Chung wrote:

7164376 Replace use of sun.security.action.LoadLibraryAction with
direct call of System.loadLibrary

Webrev:
http://cr.openjdk.java.net/~mchung/jdk8/webrevs/7164376/webrev.00/

This change is required for jdk modularization. High level summary:
it replaces the use of LoadLibraryAction:

FROM: java.security.AccessController.doPrivileged(new
LoadLibraryAction(net));

TO: AccessController.doPrivileged( new
java.security.PrivilegedActionVoid() { public Void run() {
System.loadLibrary(net); return null; } });

It touches files in awt, security and serviceability area (cc'ed).
For this type of simple change, I think 1-2 reviewers can review all
files (simpler to review jdk.patch) and no need for all teams to do
the reviews.

System.loadLibrary and Runtime.loadLibrary loads a system library of
the given library name that requires
RuntimePermission(loadLibrary.+lib) permission. Many places in the
JDK code loading a system native library is using the
sun.security.action.LoadLibraryAction convenient class that will load
the system library as a privileged action:
java.security.AccessController.doPrivileged(new
LoadLibraryAction(net));

The search path of native libraries are coupled with an associated
class loader. For example, the application class loader uses the path
specified in the java.library.path system property for native
library lookup. The loadLibrary implementation uses the caller's
class loader for finding the native library being requested. For
system libraries, the class loader is null and the system library
lookup is handled as a special case. When the
sun.security.action.LoadLibraryAction class is used that is the
caller of System.loadLibrary, the caller's class loader in this case
is null loader and thus it always finds the native library from the
system library path.

In a modular world, JDK modules may be loaded by multiple different
module class loader. The following code would not work if it is
expected to load a native library from a module which is not the
module where the sun.security.action.LoadLibraryAction lives.

For example, the management module is trying to load
libmanagement.so. Calling the following will fail to find
libmanagement.so because the caller of System.loadLibrary is the
LoadLibraryAction which is in the base module and search the library
from the base module only. To prepare for jdk modularization, the use
of LoadLibraryAction should be replaced with a direct call of
System.loadLibrary.

This patch also removes sun.security.action.LoadLibraryAction class
to avoid regression.

Thanks Mandy

Re: [OpenJDK 2D-Dev] Remove including of link.h to improve portability

[Phil Race]

I added two of those includes myself I believe and I doubt I did it 
unless needed
and others apparently found it necessary too. So we need to be sure this 
is OK.
However at least one of those I added dates back to Solaris 8 being the 
build platform

so maybe its no longer needed.

I ran the patch through our internal jprt build system on all platforms 
which
for Solaris uses a recent Solaris 10 update and it built fine. I didn't 
notice

any new warnings on the files I know anything about.

 7152519
It was incorrectly submitted as awt, I moved it to 2D.

But I think you should split this into 2 patches.
The above bug can be used for all the 2D ones and push to 2d.

The other one for the nio and security patches can go to the tl repo.

-phil.




On 3/11/2012 8:29 PM, Jonathan Lu wrote:

Bug 7152519 has been created for this patch.

- Jonathan

On 03/09/2012 07:49 PM, Jonathan Lu wrote:

Hello 2d-dev,

I find that link.h is included in several place of OpenJDK code, 
mostly together with dlfcn.h, but this caused portability problem in 
my testing on some Unix platforms, such as AIX.
So far as I see OpenJDK only makes use of basic POSIX.1-2001 
compatible dynamic library manipulation functions, such as dlopen, 
dlclose, dlsym, dlerr functions, no other extensions found, so is 
link.h still neccessary for current implementation? because link.h is 
not found in the c-POSIX standard headers 
(http://en.wikipedia.org/wiki/C_POSIX_library) and I think this 
removal will be an enhancement for portability, does that make sense?


Here's the proposed patch, since most parts of it are from Java2d so 
I post it here for discussion.
http://cr.openjdk.java.net/~luchsh/remove_link_h/ 
http://cr.openjdk.java.net/%7Eluchsh/remove_link_h/


And one more question, in 
src/solaris/native/sun/java2d/x11/XRBackendNative.c I found following 
comments

 #ifdef __solaris__
 /* Solaris 10 will not have these symbols at runtime */
 #include link.h

And in src/solaris/native/sun/awt/fontpath.c,
 #include dlfcn.h
 #ifndef __linux__ /* i.e. is solaris */
 #include link.h
 #endif

I've built successfully on Ubuntu 11.10 32bit and OpenSolaris Express 
2010.11 x86, the patch seems to be OK, but does anybody know the 
situation on Solaris (e.g. Solaris 10) of this problem?
I assume it will also comply with POSIX.1-2001 standard, and provide 
all the required functions in dlfcn.h, right?


Cheers!
- Jonathan