Re: RFR: 8282191: Implementation of Foreign Function & Memory API (Preview) [v4]

[Erik Joelsson]

On Tue, 22 Mar 2022 19:07:12 GMT, Maurizio Cimadamore  
wrote:

>> This PR contains the API and implementation changes for JEP-424 [1]. A more 
>> detailed description of such changes, to avoid repetitions during the review 
>> process, is included as a separate comment.
>> 
>> [1] - https://openjdk.java.net/jeps/424
>
> Maurizio Cimadamore has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Fix indentation in Lib.gmk

Marked as reviewed by erikj (Reviewer).

-

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

Re: RFR: 8282191: Implementation of Foreign Function & Memory API (Preview) [v3]

[Erik Joelsson]

On Tue, 22 Mar 2022 14:04:07 GMT, Maurizio Cimadamore  
wrote:

>> This PR contains the API and implementation changes for JEP-424 [1]. A more 
>> detailed description of such changes, to avoid repetitions during the review 
>> process, is included as a separate comment.
>> 
>> [1] - https://openjdk.java.net/jeps/424
>
> Maurizio Cimadamore has updated the pull request incrementally with three 
> additional commits since the last revision:
> 
>  - rename syslookup lib on Windows
>  - Add missing LIBS flag
>  - Simplify syslookup build changes

make/modules/java.base/Lib.gmk line 217:

> 215:   LDFLAGS_linux := -Wl$(COMMA)--no-as-needed, \
> 216:   LIBS := $(LIBCXX), \
> 217:   LIBS_linux := -lc -lm -ldl, \

This looks much better, thanks! Now if you could just fix the indentation of 
the parameters to 4 spaces, it would be perfect.

-

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

Re: RFR: 8282191: Implementation of Foreign Function & Memory API (Preview)

[Erik Joelsson]

On Mon, 21 Mar 2022 10:45:27 GMT, Maurizio Cimadamore  
wrote:

> This PR contains the API and implementation changes for JEP-424 [1]. A more 
> detailed description of such changes, to avoid repetitions during the review 
> process, is included as a separate comment.
> 
> [1] - https://openjdk.java.net/jeps/424

Build changes look ok.

make/modules/java.base/Lib.gmk line 217:

> 215:   CXXFLAGS := $(CXXFLAGS_JDKLIB), \
> 216:   LDFLAGS := $(LDFLAGS_JDKLIB) -Wl$(COMMA)--no-as-needed, \
> 217:   LIBS := $(LIBCXX) -lc -lm -ldl, \

Instead of repeating this whole macro call for both Linux and non Linux, you 
can use parameters of the form LDFLAGS_linux and LIBS_linux to add the Linux 
specific flags. Something like this:


LDFLAGS := $(LDFLAGS_JDKLIB), \
LDFLAGS_linux := -Wl$(COMMA)--no-as-needed, \


For the NAME field, there is no such support, so the way we usually do that is 
through a variable and conditionals before the macro call. What's the reason to 
have a different lib name on Windows? If they were the same, and the source 
file in windows/native/... had the same name, it would just automatically 
override in the build.

I realize now that this is just moved code from jdk.incubator.foreign, and this 
patch is probably big enough as it is so no need to refactor the build logic at 
the same time.

-

Marked as reviewed by erikj (Reviewer).

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

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

[Erik Joelsson]

On Mon, 21 Mar 2022 16:29:25 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 20 commits:
> 
>  - Merge branch 'master' into shuffle-data
>  - Correct name for bfc files
>  - Merge tag 'jdk-19+14' into shuffle-data
>
>Added tag jdk-19+14 for changeset 08cadb47
>  - Move x11wrappergen from share/data to unix/data
>  - Fix fontconfig according to review request
>  - Fix typos
>  - Restore charsetmapping and cldr to make/data
>  - Update copyright year
>  - Merge branch 'master' into shuffle-data-reborn
>  - Fix merge
>  - ... and 10 more: 
> https://git.openjdk.java.net/jdk/compare/19d34bdf...1c47d82d

OS specific X11 and fontconfig looks good.

make/modules/java.desktop/gendata/GendataFontConfig.gmk line 57:

> 55:   TARGETS += $(FONTCONFIG_OUT_BIN_FILE)
> 56: 
> 57: endif

It says no newline at end of file, maybe add that if it's true.

-

Marked as reviewed by erikj (Reviewer).

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

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

[Erik Joelsson]

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

Marked as reviewed by erikj (Reviewer).

-

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

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

[Erik Joelsson]

On Sat, 5 Mar 2022 06:49:16 GMT, Julian Waters  wrote:

> Should I change the JBS issue title to match the PR title, or is it preferred 
> for the PR title to change?

They need to match. You can either do it manually, or change the title to just 
the bug number and the bot will change it for you.

-

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

Re: [jdk18] RFR: JDK-8278967 rmiregistry fails to start because SecurityManager is disabled [v2]

[Erik Joelsson]

On Wed, 22 Dec 2021 01:18:58 GMT, Stuart Marks  wrote:

>> Enable the security manager in rmiregistry's launcher arguments.
>
> Stuart Marks has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Change java.security.manager to "allow"; filter warning lines from 
> VersionCheck

Build change looks good.

-

Marked as reviewed by erikj (Reviewer).

PR: https://git.openjdk.java.net/jdk18/pull/45

Re: RFR: 8278080: Add --with-cacerts-src='user cacerts folder' to enable deterministic cacerts generation [v2]

[Erik Joelsson]

On Thu, 2 Dec 2021 18:03:50 GMT, Andrew Leonard  wrote:

>> my assumption was the recipe gets resolved later
>
> this was my understanding: 
> https://www.gnu.org/software/make/manual/html_node/Variables-in-Recipes.html
> 
> This occurs after make has finished reading all the makefiles and the target 
> is determined to be out of date; so, the recipes for targets which are not 
> rebuilt are never expanded. 
> 
> but i'm going to double check I was checking the resultant cacerts correctly 
> in my tests

Oh, I didn't expand the diff far enough to actually see the context correctly 
when I reviewed this as I would never have imagined the conditional to be 
placed after the rule. While this will work as so far as using the correct 
files, incremental builds will not be correct, because the rules are defined in 
the first pass.

I very much agree with Magnus that this conditional belongs around line 63.

-

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

Re: RFR: 8278080: Add --with-cacerts-src='user cacerts folder' to enable deterministic cacerts generation [v2]

[Erik Joelsson]

On Thu, 2 Dec 2021 12:13:03 GMT, Andrew Leonard  wrote:

>> Addition of a configure option --with-cacerts-src='user cacerts folder' to 
>> allow developers to specify their own cacerts PEM folder for generation of 
>> the cacerts store using the deterministic openjdk GenerateCacerts tool.
>> 
>> Signed-off-by: Andrew Leonard 
>
> Andrew Leonard has updated the pull request with a new target base due to a 
> merge or a rebase. The incremental webrev excludes the unrelated changes 
> brought in by the merge/rebase. The pull request contains four additional 
> commits since the last revision:
> 
>  - 8278080: Add --with-cacerts-src='user cacerts folder' to enable 
> deterministic cacerts generation
>
>Signed-off-by: Andrew Leonard 
>  - Merge branch 'master' of https://github.com/openjdk/jdk into cacertssrc
>  - 8278080: Add --with-cacerts-src='user cacerts folder' to enable 
> determinsitic cacerts generation
>
>Signed-off-by: Andrew Leonard 
>  - 8278080: Add --with-cacerts-src='user cacerts folder' to enable 
> determinsitic cacerts generation
>
>Signed-off-by: Andrew Leonard 

Looks good!

-

Marked as reviewed by erikj (Reviewer).

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

Re: RFR: 8275252: Migrate cacerts from JKS to password-less PKCS12

[Erik Joelsson]

On Thu, 14 Oct 2021 13:36:19 GMT, Weijun Wang  wrote:

> The cacerts file is now a password-less PKCS12 file. This make sure old code 
> that uses a JKS KeyStore object can continuously load it using a null 
> password (in fact, any password) and see all certificates inside.

Build change looks good.

-

Marked as reviewed by erikj (Reviewer).

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

Re: RFR: 8275063: Implementation of Memory Access API (Second incubator)

[Erik Joelsson]

On Tue, 12 Oct 2021 11:16:51 GMT, Maurizio Cimadamore  
wrote:

> This PR contains the API and implementation changes for JEP-419 [1]. A more 
> detailed description of such changes, to avoid repetitions during the review 
> process, is included as a separate comment.
> 
> [1] - https://openjdk.java.net/jeps/419

Build changes look good.

-

Marked as reviewed by erikj (Reviewer).

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

Re: RFR: 8202056: Expand serial warning to check for bad overloads of serial-related methods and ineffectual fields

[Erik Joelsson]

On Mon, 27 Sep 2021 01:00:18 GMT, Joe Darcy  wrote:

> This is an initial PR for expanded lint warnings done under two bugs:
> 
> 8202056: Expand serial warning to check for bad overloads of serial-related 
> methods and ineffectual fields
> 8160675: Issue lint warning for non-serializable non-transient instance 
> fields in serializable type
> 
> to get feedback on the general approach and test strategy before further 
> polishing the implementation.
> 
> The implementation initially started as an annotation processor I wrote 
> several years ago. The refined version being incorporated into Attr has been 
> refactored, had its checks expanded, and been partially ported to idiomatic 
> javac coding style rather than using the javax.lang.model API from annotation 
> processing.
> 
> Subsequent versions of this PR are expected to move the implementation closer 
> to idiomatic javac, in particular to use javac flags rather than 
> javax.lang.model.Modifier's. Additional resources keys will be defined for 
> the serialization-related fields and methods not having the expected 
> modifiers, types, etc. The resource keys for the existing checks related to 
> serialVersionUID and reused.
> 
> Please also review the corresponding CSRs:
> 
> https://bugs.openjdk.java.net/browse/JDK-8274335
> https://bugs.openjdk.java.net/browse/JDK-8274336
> 
> Informative serialization-related warning messages must take into account 
> whether a class, interface, annotation, record, and enum is being analyzed. 
> Enum classes and record classes have special handling in serialization. This 
> implementation under review has been augmented with checks for interface 
> types recommended by Chris Hegarty in an attachment on 8202056.
> 
> The JDK build has the Xlint:serial check enabled. The build did not pass with 
> the augmented checks. For most modules, this PR contains the library changes 
> necessary for the build to pass. I will start separate PRs in those library 
> areas to get the needed SuppressWarning("serial") or other changes in place. 
> For one module, I temporarily disabled the Xlint:serial check.
> 
> In terms of performance, I have not done benchmarks of the JDK build with and 
> without these changes, but informally the build seems to take about as long 
> as before.

Build change looks ok.

-

Marked as reviewed by erikj (Reviewer).

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

Re: RFR: 8272805: Avoid looking up standard charsets [v2]

[Erik Joelsson]

On Sun, 22 Aug 2021 23:02:06 GMT, Sergey Bylokhov  wrote:

>> This is the continuation of JDK-8233884, JDK-8271456, and JDK-8272120.
>> 
>> In many places standard charsets are looked up via their names, for example:
>> absolutePath.getBytes("UTF-8");
>> 
>> This could be done more efficiently(up to x20 time faster) with use of 
>> java.nio.charset.StandardCharsets:
>> absolutePath.getBytes(StandardCharsets.UTF_8);
>> 
>> The later variant also makes the code cleaner, as it is known not to throw 
>> UnsupportedEncodingException in contrary to the former variant.
>> 
>> This change includes:
>>  * demo/utils
>>  * jdk.xx packages
>>  * Some places were missed in the previous changes. I have found it by 
>> tracing the calls to the Charset.forName() by executing tier1,2,3 and 
>> desktop tests.
>> 
>> Some performance discussion: https://github.com/openjdk/jdk/pull/5063
>> 
>> Code excluded in this fix: the Xerces library(should be fixed upstream), 
>> J2DBench(should be compatible to 1.4), some code in the network(the change 
>> there are not straightforward, will do it later).
>> 
>> Tested by the tier1/tier2/tier3 tests on Linux/Windows/macOS.
>
> Sergey Bylokhov 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 14 additional 
> commits since the last revision:
> 
>  - Update the usage of Files.readAllLines()
>  - Update datatransfer
>  - Merge branch 'master' into standard-encodings-in-non-public-modules
>  - Merge branch 'master' into standard-encodings-in-non-public-modules
>  - Fix related imports
>  - Merge branch 'master' into standard-encodings-in-non-public-modules
>  - Cleanup UnsupportedEncodingException
>  - Update PacketStream.java
>  - Rollback TextTests, should be compatible with jdk1.4
>  - Rollback TextRenderTests, should be compatible with jdk1.4
>  - ... and 4 more: 
> https://git.openjdk.java.net/jdk/compare/6af90a19...e7127644

Build tool change looks good.

-

Marked as reviewed by erikj (Reviewer).

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

Re: RFR: 8268129: LibraryLookup::ofDefault leaks symbols from loaded libraries [v6]

[Erik Joelsson]

On Thu, 3 Jun 2021 16:43:51 GMT, Maurizio Cimadamore  
wrote:

>> This patch overhauls the library loading mechanism used by the Foreign 
>> Linker API. We realized that, while handy, the *default* lookup abstraction 
>> (`LibraryLookup::ofDefault`) was behaving inconsistentlt across platforms.
>> 
>> This patch replaces `LibraryLookup` with a simpler `SymbolLookup` 
>> abstraction, a functional interface. Crucially, `SymbolLookup` does not 
>> concern with library loading, only symbol lookup. For this reason, two 
>> factories are added:
>> 
>> * `SymbolLookup::loaderLookup` - which obtains a lookup that can be used to 
>> lookup symbols in libraries loaded by current loader
>> * `CLinker::systemLookup` - a more stable replacement for the *default* 
>> lookup, which looks for symbols in libc.so (or its equivalent in other 
>> platforms). The contents of this lookup are unspecified.
>> 
>> Both factories are *restricted*, so they can only be called when 
>> `--enable-native-access` is set.
>
> Maurizio Cimadamore has updated the pull request with a new target base due 
> to a merge or a rebase. The pull request now contains 16 commits:
> 
>  - Merge branch 'master' into symbolLookup
>  - Forgot to add makefile for building shim library
>  - Address review comments
>  - Update test/jdk/java/foreign/TestIntrinsics.java
>
>Co-authored-by: Paul Sandoz 
>  - Update test/jdk/java/foreign/valist/VaListTest.java
>
>Co-authored-by: Paul Sandoz 
>  - Update test/jdk/java/foreign/TestVarArgs.java
>
>Co-authored-by: Paul Sandoz 
>  - Update test/jdk/java/foreign/TestUpcall.java
>
>Co-authored-by: Paul Sandoz 
>  - Update test/jdk/java/foreign/TestIllegalLink.java
>
>Co-authored-by: Paul Sandoz 
>  - Update test/jdk/java/foreign/TestSymbolLookup.java
>
>Co-authored-by: Paul Sandoz 
>  - Update test/jdk/java/foreign/TestDowncall.java
>
>Co-authored-by: Paul Sandoz 
>  - ... and 6 more: 
> https://git.openjdk.java.net/jdk/compare/52d8215a...2545e2b6

Looks pretty good, just a few comments.

make/modules/jdk.incubator.foreign/Lib.gmk line 28:

> 26: include LibCommon.gmk
> 27: 
> 28: ifeq ($(call isTargetOs, linux), true)

Please indent everything inside the ifeq-block 2 spaces. (See 
http://openjdk.java.net/groups/build/doc/code-conventions.html)

make/modules/jdk.incubator.foreign/Lib.gmk line 34:

> 32: CFLAGS := $(CFLAGS_JDKLIB), \
> 33: CXXFLAGS := $(CXXFLAGS_JDKLIB), \
> 34: LDFLAGS := -Wl$(COMMA)--no-as-needed -lc -lm -ldl $(LDFLAGS_JDKLIB) 
> $(call SET_SHARED_LIBRARY_ORIGIN), \

Unless you link with any other library in the JDK (typically libjava and/or 
libjvm), I don't think there is a need for adding SET_SHARED_LIBRARY_ORIGIN.

Please put all the -l* flags in LIBS rather than LDFLAGS.

I also recommend putting any additional flags after the general LDFLAGS_JDKLIB. 
That way you are guaranteed that your flag takes precedence over anything that 
may be added to LDFLAGS_JDKLIB in the future.

-

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

Re: RFR: 8267123: Remove RMI Activation

[Erik Joelsson]

On Tue, 25 May 2021 18:04:45 GMT, Stuart Marks  wrote:

> This is the implementation of [JEP 407](https://openjdk.java.net/jeps/407).
> 
> This is a fairly straightforward removal of this component.
>  - Activation implementation classes removed
>  - Activation tests removed
>  - adjustments to various comments to remove references to Activation
>  - adjustments to some code to remove special-case support for Activation 
> from core RMI
>  - adjustments to several tests to remove testing and support for, and 
> mentions of Activation
>  - remove `rmid` tool
> 
> (Personally, I found that reviewing using the webrev was easier than 
> navigating github's diff viewer.)

Build changes look good.

-

Marked as reviewed by erikj (Reviewer).

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

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

[Erik Joelsson]

On Mon, 17 May 2021 18:23:41 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.

Makefile change looks ok.

-

Marked as reviewed by erikj (Reviewer).

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

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

[Erik Joelsson]

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

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

Build changes look good.

-

Marked as reviewed by erikj (Reviewer).

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

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

[erik . joelsson]

On 2021-02-26 06:37, daniel.daughe...@oracle.com wrote:

On 2/26/21 7:55 AM, Vladimir Kempik wrote:
On Tue, 2 Feb 2021 23:07:08 GMT, Daniel D. Daugherty 
 wrote:


Anton Kozlov has updated the pull request incrementally with one 
additional commit since the last revision:


   support macos_aarch64 in hsdis

src/java.base/macosx/native/libjli/java_md_macosx.m line 210:


208: if (preferredJVM == NULL) {
209: #if defined(__i386__)
210: preferredJVM = "client";

#if defined(__i386__)
 preferredJVM = "client";
Not your bug, but Oracle/OpenJDK never supported 32-bit __i386__ on 
macOS.

Hello
I thought the openjdk7 supported 32-bit macos at some point in time


The macOS porting project supported 32-bit macOS, but when the code
was integrated into OpenJDK7 I don't believe that 32-bit macOS was
supported. I could be wrong... it was quite a while ago...

AFAIK, OpenJDK never supported 32bit macos, but there are certainly 
leftovers here and there indicating that the original source did at some 
point. In the build system we cleaned that up a long time ago.


/Erik

Re: RFR: 8259886 : Improve SSL session cache performance and scalability [v2]

[Erik Joelsson]

On Mon, 1 Feb 2021 18:49:04 GMT, djelinski 
 wrote:

>> Under certain load, MemoryCache operations take a substantial fraction of 
>> the time needed to complete SSL handshakes. This series of patches improves 
>> performance characteristics of MemoryCache, at the cost of a functional 
>> change: expired entries are no longer guaranteed to be removed before live 
>> ones. Unused entries are still removed before used ones, and cache 
>> performance no longer depends on its capacity.
>> 
>> First patch in the series contains a benchmark that can be run with `make 
>> test TEST="micro:CacheBench"`.
>> Baseline results before any MemoryCache changes:
>> Benchmark   (size)  (timeout)  Mode  Cnt ScoreError  Units
>> CacheBench.put   20480  86400  avgt   2583.653 ?  6.269  us/op
>> CacheBench.put   20480  0  avgt   25 0.107 ?  0.001  us/op
>> CacheBench.put  204800  86400  avgt   25  2057.781 ? 35.942  us/op
>> CacheBench.put  204800  0  avgt   25 0.108 ?  0.001  us/op
>> there's a nonlinear performance drop between 20480 and 204800 entries, 
>> probably attributable to CPU cache thrashing. Beyond 204800 entries the 
>> cache scales more linearly.
>> 
>> Benchmark results after the 2nd and 3rd patches are pretty similar, so I'll 
>> only copy one:
>> Benchmark   (size)  (timeout)  Mode  Cnt  Score   Error  Units
>> CacheBench.put   20480  86400  avgt   25  0.146 ? 0.002  us/op
>> CacheBench.put   20480  0  avgt   25  0.108 ? 0.002  us/op
>> CacheBench.put  204800  86400  avgt   25  0.150 ? 0.001  us/op
>> CacheBench.put  204800  0  avgt   25  0.106 ? 0.001  us/op
>> The third patch improves worst-case times on a mostly idle cache by 
>> scattering removal of expired entries over multiple `put` calls. It does not 
>> affect performance of an overloaded cache.
>> 
>> The 4th patch removes all code that clears cached values before handing them 
>> over to the GC. [This 
>> comment](https://github.com/openjdk/jdk/commit/5859a0320334bfb6b46b62eb16b4c387641f4a2a#diff-c6bd583a97fbc4f471621fee7eab37c63718cdb6932ce357fa403cfda4b32b6fL346)
>>  stated that clearing values was supposed to be a GC performance 
>> optimization. It wasn't. Benchmark results after that commit:
>> Benchmark   (size)  (timeout)  Mode  Cnt  Score   Error  Units
>> CacheBench.put   20480  86400  avgt   25  0.113 ? 0.001  us/op
>> CacheBench.put   20480  0  avgt   25  0.075 ? 0.002  us/op
>> CacheBench.put  204800  86400  avgt   25  0.116 ? 0.001  us/op
>> CacheBench.put  204800  0  avgt   25  0.072 ? 0.001  us/op
>> I wasn't expecting that much of an improvement, and don't know how to 
>> explain it.
>> 
>> The 40ns difference between cache with and without a timeout can be 
>> attributed to 2 `System.currentTimeMillis()` calls; they were pretty slow on 
>> my VM.
>
> djelinski has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Simplify makefile

Build change looks good, but I would like to hear from @cl4es too.

-

Marked as reviewed by erikj (Reviewer).

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

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

[Erik Joelsson]

On Mon, 1 Feb 2021 16:51:12 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 two additional 
> commits since the last revision:
> 
>  - file attr error
>  - objc use .m

Build changes look good. Thanks for fixing the .m support in 
TestFilesCompilation.gmk!

-

Marked as reviewed by erikj (Reviewer).

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

Re: RFR: 8259886 : Improve SSL session cache performance and scalability

[Erik Joelsson]

On Mon, 1 Feb 2021 18:24:46 GMT, djelinski 
 wrote:

>> make/test/BuildMicrobenchmark.gmk line 97:
>> 
>>> 95: SRC := $(MICROBENCHMARK_SRC), \
>>> 96: BIN := $(MICROBENCHMARK_CLASSES), \
>>> 97: JAVAC_FLAGS := $(JAVAC_FLAGS) --add-exports 
>>> java.base/sun.security.util=ALL-UNNAMED, \
>> 
>> Why do you need to add $(JAVAC_FLAGS) here?
>
> I'm trying to benchmark a class that is in a non-exported package 
> `sun.security.util`. Without this line the benchmark doesn't compile. I 
> couldn't find any other benchmarks that access non-exported classes, so I 
> came up with my own implementation. 
> 
> Is there a better way to get the benchmark to compile?

Adding "--add-exports java.base/sun.security.util=ALL-UNNAMED" is fine, I'm 
asking about "$(JAVAC_FLAGS)".

-

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

Re: RFR: 8259886 : Improve SSL session cache performance and scalability

[Erik Joelsson]

On Mon, 1 Feb 2021 18:30:14 GMT, Erik Joelsson  wrote:

>> I'm trying to benchmark a class that is in a non-exported package 
>> `sun.security.util`. Without this line the benchmark doesn't compile. I 
>> couldn't find any other benchmarks that access non-exported classes, so I 
>> came up with my own implementation. 
>> 
>> Is there a better way to get the benchmark to compile?
>
> Adding "--add-exports java.base/sun.security.util=ALL-UNNAMED" is fine, I'm 
> asking about "$(JAVAC_FLAGS)".

Hm, maybe you just misunderstand how this makefile construct works. If you just 
want to add "--add-exports java.base/sun.security.util=ALL-UNNAMED", then 
that's all you should put in this assignment.

-

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

Re: RFR: 8259886 : Improve SSL session cache performance and scalability

[Erik Joelsson]

On Wed, 27 Jan 2021 11:32:08 GMT, djelinski 
 wrote:

> Under certain load, MemoryCache operations take a substantial fraction of the 
> time needed to complete SSL handshakes. This series of patches improves 
> performance characteristics of MemoryCache, at the cost of a functional 
> change: expired entries are no longer guaranteed to be removed before live 
> ones. Unused entries are still removed before used ones, and cache 
> performance no longer depends on its capacity.
> 
> First patch in the series contains a benchmark that can be run with `make 
> test TEST="micro:CacheBench"`.
> Baseline results before any MemoryCache changes:
> Benchmark   (size)  (timeout)  Mode  Cnt ScoreError  Units
> CacheBench.put   20480  86400  avgt   2583.653 ?  6.269  us/op
> CacheBench.put   20480  0  avgt   25 0.107 ?  0.001  us/op
> CacheBench.put  204800  86400  avgt   25  2057.781 ? 35.942  us/op
> CacheBench.put  204800  0  avgt   25 0.108 ?  0.001  us/op
> there's a nonlinear performance drop between 20480 and 204800 entries, 
> probably attributable to CPU cache thrashing. Beyond 204800 entries the cache 
> scales more linearly.
> 
> Benchmark results after the 2nd and 3rd patches are pretty similar, so I'll 
> only copy one:
> Benchmark   (size)  (timeout)  Mode  Cnt  Score   Error  Units
> CacheBench.put   20480  86400  avgt   25  0.146 ? 0.002  us/op
> CacheBench.put   20480  0  avgt   25  0.108 ? 0.002  us/op
> CacheBench.put  204800  86400  avgt   25  0.150 ? 0.001  us/op
> CacheBench.put  204800  0  avgt   25  0.106 ? 0.001  us/op
> The third patch improves worst-case times on a mostly idle cache by 
> scattering removal of expired entries over multiple `put` calls. It does not 
> affect performance of an overloaded cache.
> 
> The 4th patch removes all code that clears cached values before handing them 
> over to the GC. [This 
> comment](https://github.com/openjdk/jdk/commit/5859a0320334bfb6b46b62eb16b4c387641f4a2a#diff-c6bd583a97fbc4f471621fee7eab37c63718cdb6932ce357fa403cfda4b32b6fL346)
>  stated that clearing values was supposed to be a GC performance 
> optimization. It wasn't. Benchmark results after that commit:
> Benchmark   (size)  (timeout)  Mode  Cnt  Score   Error  Units
> CacheBench.put   20480  86400  avgt   25  0.113 ? 0.001  us/op
> CacheBench.put   20480  0  avgt   25  0.075 ? 0.002  us/op
> CacheBench.put  204800  86400  avgt   25  0.116 ? 0.001  us/op
> CacheBench.put  204800  0  avgt   25  0.072 ? 0.001  us/op
> I wasn't expecting that much of an improvement, and don't know how to explain 
> it.
> 
> The 40ns difference between cache with and without a timeout can be 
> attributed to 2 `System.currentTimeMillis()` calls; they were pretty slow on 
> my VM.

make/test/BuildMicrobenchmark.gmk line 97:

> 95: SRC := $(MICROBENCHMARK_SRC), \
> 96: BIN := $(MICROBENCHMARK_CLASSES), \
> 97: JAVAC_FLAGS := $(JAVAC_FLAGS) --add-exports 
> java.base/sun.security.util=ALL-UNNAMED, \

Why do you need to add $(JAVAC_FLAGS) here?

-

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

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

[Erik Joelsson]

On Fri, 18 Dec 2020 19:20:47 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)

Build changes look good.

-

Marked as reviewed by erikj (Reviewer).

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

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

[erik . joelsson]

On 2021-01-26 04:44, Magnus Ihse Bursie wrote:

On 2021-01-26 13:09, Vladimir Kempik wrote:
On Tue, 26 Jan 2021 12:02:02 GMT, Alan Hayward 
 wrote:


AIUI, the configure line needs passing a prebuilt 
JavaNativeFoundation framework

ie:
`--with-extra-ldflags='-F 
/Applications/Xcode.app/Contents/SharedFrameworks/ContentDeliveryServices.framework/Versions/A/itms/java/Frameworks/'`


Otherwise there will be missing _JNFNative* functions.

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

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


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


My understanding is that Apple chose to not provide JNF for aarch64, so 
if you want to build OpenJDK, you first need to build JNF yourself (it's 
available in github). Phil is working on removing this dependency 
completely, which will solve this issue [1].


In the meantime, I don't think we should rely on finding JNF in 
unsupported locations inside Xcode. Could we wait with integrating this 
port until it can be built without such hacks? If not, then adding 
something in the documentation on how to get a working JNF would at 
least be needed.


/Erik

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

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

[Erik Joelsson]

On Fri, 22 Jan 2021 18:49:42 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)

Build changes in general look good.

make/autoconf/flags-cflags.m4 line 169:

> 167:   WARNINGS_ENABLE_ALL="-Wall -Wextra -Wformat=2 
> $WARNINGS_ENABLE_ADDITIONAL"
> 168: 
> 169:   DISABLED_WARNINGS="unknown-warning-option unused-parameter unused 
> format-nonliteral"

Globally disabling a warning needs some motivation. Why is this needed? Does it 
really need to be applied everywhere or is there a specific binary or source 
file where this is triggered?

-

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

Re: RFR: 8253866: Security Libs Terminology Refresh

[Erik Joelsson]

On Thu, 14 Jan 2021 06:32:37 GMT, Jamil Nimeh  wrote:

> This is the security libs portion of the effort to replace 
> archaic/non-inclusive words with more neutral terms (see JDK-8253315 for 
> details).
> 
> Here are the changes covering core libraries code and tests. Terms were 
> changed as follows:
> 
> - blacklisted.certs -> blocked.certs (along with supporting make 
> infrastructure and tests)
> - master/slave in KRB5 -> primary/replica
> - blacklist in other files -> deny list
> - whitelist -> allow list
> 
> Addressing similar issues in upstream 3rd party code is out of scope of this 
> PR. Such changes will be picked up from their upstream sources.

Build changes look good.

-

Marked as reviewed by erikj (Reviewer).

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

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

[Erik Joelsson]

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

-

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

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

[Erik Joelsson]

On 2020-12-08 00:30, Magnus Ihse Bursie wrote:

On Tue, 8 Dec 2020 02:40:43 GMT, Mandy Chung  wrote:


I have reviewed all lines in the patch file with or near instances of 
`jdk.compiler`

Hi Magnus,

I see the motivation of moving these build files for better identification of 
ownership.   Placing them under
`src/$MODULE/{share,$OS}/data` is one option.  Given that skara will 
automatically determine appropriate mailing lists of a PR, it seems that 
`make/modules/$MODULE/data` can be another alternative that skara can include 
this pattern in the mailing list configuration??   I don't yet have a strong 
preference while I don't consider everything under `make` must be owned by the 
build team though.  Do you see one option is better than the other?

@mlchung If you don't have any strong preference, I implore you to accept this 
change. I **vastly** prefer to move the data out of `make`!

This is not just about Skara tooling. When working with make files, autoconf and shell scripts, 
there is no fancy IDE support, so you are stuck with simple text editors and tools like `grep`. 
I've lost count on how many times I've had my grep searches blow up, since I happened to find e.g. 
a string in `tzdata` and get hundreds or more of hits. :-( And I do believe we will get a better 
code structure if the build team "owns" `make`;  or at least has a vested interest in 
what's in that directory. We still suffer a lot of the old "I don't know where to put this 
file, so I'll just put it in make cause nobody cares about it anyway" mentality, but I've been 
working for quite some time to make that list of misplaced files shorter and shorter.

I strongly agree with Magnus for all the same reasons. To me, the data 
files are clearly a form of source code that should be considered owned 
by the component teams. I'm honestly perplexed over why this is being 
argued against.


/Erik


-

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

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

[Erik Joelsson]

On Fri, 4 Dec 2020 12:30:02 GMT, Alan Bateman  wrote:

>> And I can certainly move jdwp.spec to java.base instead. That's the reason I 
>> need input on this: All I know is that is definitely not the responsibility 
>> of the Build Group to maintain that document, and I made my best guess at 
>> where to place it.
>
>> And I can certainly move jdwp.spec to java.base instead. 
> 
> If jdwp.spec has to move to the src tree then src/java.se is probably the 
> most suitable home because Java SE specifies JDWP as an optional interface. 
> So nothing to do with java.base and the build will need to continue to 
> generate the sources for the front-end (jdk.jdi) and back-end 
> (jdk.jdwp.agent) as they implement the protocol.

My understanding of JEPs is that they should be viewed as living documents. In 
this case, I think it's perfectly valid to update JEP 201 with additional 
source code layout. Both for this and for the other missing dirs.

-

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

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

[Erik Joelsson]

On Fri, 4 Dec 2020 14:03:08 GMT, Erik Joelsson  wrote:

>>> And I can certainly move jdwp.spec to java.base instead. 
>> 
>> If jdwp.spec has to move to the src tree then src/java.se is probably the 
>> most suitable home because Java SE specifies JDWP as an optional interface. 
>> So nothing to do with java.base and the build will need to continue to 
>> generate the sources for the front-end (jdk.jdi) and back-end 
>> (jdk.jdwp.agent) as they implement the protocol.
>
> My understanding of JEPs is that they should be viewed as living documents. 
> In this case, I think it's perfectly valid to update JEP 201 with additional 
> source code layout. Both for this and for the other missing dirs.

Regarding the chosen layout. Did you consider following the existing pattern of 
src//{share,}/data?

-

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

Re: RFR: 8248862: Implement Enhanced Pseudo-Random Number Generators

[Erik Joelsson]

On Tue, 17 Nov 2020 19:58:47 GMT, Jim Laskey  wrote:

> This PR is to introduce a new random number API for the JDK. The primary API 
> is found in RandomGenerator and RandomGeneratorFactory. Further description 
> can be found in the JEP https://openjdk.java.net/jeps/356 .

It looks like you have deleted the doc directory. That can't be right?

-

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

Re: RFR: 8254162: Implementation of Foreign-Memory Access API (Third Incubator) [v3]

[Erik Joelsson]

On Thu, 8 Oct 2020 13:59:20 GMT, Maurizio Cimadamore  
wrote:

>> This patch contains the changes associated with the third incubation round 
>> of the foreign memory access API incubation
>> (see JEP 393 [1]). This iteration focus on improving the usability of the 
>> API in 3 main ways:
>> * first, by providing a way to obtain truly *shared* segments, which can be 
>> accessed and closed concurrently from
>>   multiple threads
>> * second, by providing a way to register a memory segment against a 
>> `Cleaner`, so as to have some (optional) guarantee
>>   that the memory will be deallocated, eventually
>> * third, by not requiring users to dive deep into var handles when they 
>> first pick up the API; a new `MemoryAccess` class
>>   has been added, which defines several useful dereference routines; these 
>> are really just thin wrappers around memory
>>   access var handles, but they make the barrier of entry for using this API 
>> somewhat lower.
>> 
>> A big conceptual shift that comes with this API refresh is that the role of 
>> `MemorySegment` and `MemoryAddress` is not
>> the same as it used to be; it used to be the case that a memory address 
>> could (sometimes, not always) have a back link
>> to the memory segment which originated it; additionally, memory access var 
>> handles used `MemoryAddress` as a basic unit
>> of dereference.  This has all changed as per this API refresh;  now a 
>> `MemoryAddress` is just a dumb carrier which
>> wraps a pair of object/long addressing coordinates; `MemorySegment` has 
>> become the star of the show, as far as
>> dereferencing memory is concerned. You cannot dereference memory if you 
>> don't have a segment. This improves usability
>> in a number of ways - first, it is a lot easier to wrap native addresses 
>> (`long`, essentially) into a `MemoryAddress`;
>> secondly, it is crystal clear what a client has to do in order to 
>> dereference memory: if a client has a segment, it can
>> use that; otherwise, if the client only has an address, it will have to 
>> create a segment *unsafely* (this can be done
>> by calling `MemoryAddress::asSegmentRestricted`).  A list of the API, 
>> implementation and test changes is provided
>> below. If  you have any questions, or need more detailed explanations, I 
>> (and the  rest of the Panama team) will be
>> happy to point at existing discussions,  and/or to provide the feedback 
>> required.   A big thank to Erik Osterlund,
>> Vladimir Ivanov and David Holmes, without whom the work on shared memory 
>> segment would not have been possible; also I'd
>> like to thank Paul Sandoz, whose insights on API design have been very 
>> helpful in this journey.  Thanks  Maurizio
>> Javadoc:   
>> http://cr.openjdk.java.net/~mcimadamore/8254162_v1/javadoc/jdk/incubator/foreign/package-summary.html
>> Specdiff:
>> 
>> http://cr.openjdk.java.net/~mcimadamore/8254162_v1/specdiff/jdk/incubator/foreign/package-summary.html
>> 
>> CSR:
>> 
>> https://bugs.openjdk.java.net/browse/JDK-8254163
>> 
>> 
>> 
>> ### API Changes
>> 
>> * `MemorySegment`
>>   * drop factory for restricted segment (this has been moved to 
>> `MemoryAddress`, see below)
>>   * added a no-arg factory for a native restricted segment representing 
>> entire native heap
>>   * rename `withOwnerThread` to `handoff`
>>   * add new `share` method, to create shared segments
>>   * add new `registerCleaner` method, to register a segment against a cleaner
>>   * add more helpers to create arrays from a segment e.g. `toIntArray`
>>   * add some `asSlice` overloads (to make up for the fact that now segments 
>> are more frequently used as cursors)
>>   * rename `baseAddress` to `address` (so that `MemorySegment` can implement 
>> `Addressable`)
>> * `MemoryAddress`
>>   * drop `segment` accessor
>>   * drop `rebase` method and replace it with `segmentOffset` which returns 
>> the offset (a `long`) of this address relative
>> to a given segment
>> * `MemoryAccess`
>>   * New class supporting several static dereference helpers; the helpers are 
>> organized by carrier and access mode, where a
>> carrier is one of the usual suspect (a Java primitive, minus `boolean`); 
>> the access mode can be simple (e.g. access
>> base address of given segment), or indexed, in which case the accessor 
>> takes a segment and either a low-level byte
>> offset,or a high level logical index. The classification is reflected in 
>> the naming scheme (e.g. `getByte` vs.
>> `getByteAtOffset` vs `getByteAtIndex`).
>> * `MemoryHandles`
>>   * drop `withOffset` combinator
>>   * drop `withStride` combinator
>>   * the basic memory access handle factory now returns a var handle which 
>> takes a `MemorySegment` and a `long` - from which
>> it is easy to derive all the other handles using plain var handle 
>> combinators.
>> * `Addressable`
>>   * This is a new interface which is attached to entities which can be 
>> projected to a `MemoryAddress`. For now, both
>> 

Re: RFR: 8254162: Implementation of Foreign-Memory Access API (Third Incubator) [v2]

[Erik Joelsson]

On Thu, 8 Oct 2020 10:29:24 GMT, Maurizio Cimadamore  
wrote:

>> This patch contains the changes associated with the third incubation round 
>> of the foreign memory access API incubation
>> (see JEP 393 [1]). This iteration focus on improving the usability of the 
>> API in 3 main ways:
>> * first, by providing a way to obtain truly *shared* segments, which can be 
>> accessed and closed concurrently from
>>   multiple threads
>> * second, by providing a way to register a memory segment against a 
>> `Cleaner`, so as to have some (optional) guarantee
>>   that the memory will be deallocated, eventually
>> * third, by not requiring users to dive deep into var handles when they 
>> first pick up the API; a new `MemoryAccess` class
>>   has been added, which defines several useful dereference routines; these 
>> are really just thin wrappers around memory
>>   access var handles, but they make the barrier of entry for using this API 
>> somewhat lower.
>> 
>> A big conceptual shift that comes with this API refresh is that the role of 
>> `MemorySegment` and `MemoryAddress` is not
>> the same as it used to be; it used to be the case that a memory address 
>> could (sometimes, not always) have a back link
>> to the memory segment which originated it; additionally, memory access var 
>> handles used `MemoryAddress` as a basic unit
>> of dereference.  This has all changed as per this API refresh;  now a 
>> `MemoryAddress` is just a dumb carrier which
>> wraps a pair of object/long addressing coordinates; `MemorySegment` has 
>> become the star of the show, as far as
>> dereferencing memory is concerned. You cannot dereference memory if you 
>> don't have a segment. This improves usability
>> in a number of ways - first, it is a lot easier to wrap native addresses 
>> (`long`, essentially) into a `MemoryAddress`;
>> secondly, it is crystal clear what a client has to do in order to 
>> dereference memory: if a client has a segment, it can
>> use that; otherwise, if the client only has an address, it will have to 
>> create a segment *unsafely* (this can be done
>> by calling `MemoryAddress::asSegmentRestricted`).  A list of the API, 
>> implementation and test changes is provided
>> below. If  you have any questions, or need more detailed explanations, I 
>> (and the  rest of the Panama team) will be
>> happy to point at existing discussions,  and/or to provide the feedback 
>> required.   A big thank to Erik Osterlund,
>> Vladimir Ivanov and David Holmes, without whom the work on shared memory 
>> segment would not have been possible; also I'd
>> like to thank Paul Sandoz, whose insights on API design have been very 
>> helpful in this journey.  Thanks  Maurizio
>> Javadoc:   
>> http://cr.openjdk.java.net/~mcimadamore/8254162_v1/javadoc/jdk/incubator/foreign/package-summary.html
>> Specdiff:
>> 
>> http://cr.openjdk.java.net/~mcimadamore/8254162_v1/specdiff/jdk/incubator/foreign/package-summary.html
>> 
>> CSR:
>> 
>> https://bugs.openjdk.java.net/browse/JDK-8254163
>> 
>> 
>> 
>> ### API Changes
>> 
>> * `MemorySegment`
>>   * drop factory for restricted segment (this has been moved to 
>> `MemoryAddress`, see below)
>>   * added a no-arg factory for a native restricted segment representing 
>> entire native heap
>>   * rename `withOwnerThread` to `handoff`
>>   * add new `share` method, to create shared segments
>>   * add new `registerCleaner` method, to register a segment against a cleaner
>>   * add more helpers to create arrays from a segment e.g. `toIntArray`
>>   * add some `asSlice` overloads (to make up for the fact that now segments 
>> are more frequently used as cursors)
>>   * rename `baseAddress` to `address` (so that `MemorySegment` can implement 
>> `Addressable`)
>> * `MemoryAddress`
>>   * drop `segment` accessor
>>   * drop `rebase` method and replace it with `segmentOffset` which returns 
>> the offset (a `long`) of this address relative
>> to a given segment
>> * `MemoryAccess`
>>   * New class supporting several static dereference helpers; the helpers are 
>> organized by carrier and access mode, where a
>> carrier is one of the usual suspect (a Java primitive, minus `boolean`); 
>> the access mode can be simple (e.g. access
>> base address of given segment), or indexed, in which case the accessor 
>> takes a segment and either a low-level byte
>> offset,or a high level logical index. The classification is reflected in 
>> the naming scheme (e.g. `getByte` vs.
>> `getByteAtOffset` vs `getByteAtIndex`).
>> * `MemoryHandles`
>>   * drop `withOffset` combinator
>>   * drop `withStride` combinator
>>   * the basic memory access handle factory now returns a var handle which 
>> takes a `MemorySegment` and a `long` - from which
>> it is easy to derive all the other handles using plain var handle 
>> combinators.
>> * `Addressable`
>>   * This is a new interface which is attached to entities which can be 
>> projected to a `MemoryAddress`. For now, both
>> 

Re: RFR: 8254162: Implementation of Foreign-Memory Access API (Third Incubator)

[Erik Joelsson]

On Wed, 7 Oct 2020 17:13:22 GMT, Maurizio Cimadamore  
wrote:

> This patch contains the changes associated with the third incubation round of 
> the foreign memory access API incubation
> (see JEP 393 [1]). This iteration focus on improving the usability of the API 
> in 3 main ways:
> * first, by providing a way to obtain truly *shared* segments, which can be 
> accessed and closed concurrently from
>   multiple threads
> * second, by providing a way to register a memory segment against a 
> `Cleaner`, so as to have some (optional) guarantee
>   that the memory will be deallocated, eventually
> * third, by not requiring users to dive deep into var handles when they first 
> pick up the API; a new `MemoryAccess` class
>   has been added, which defines several useful dereference routines; these 
> are really just thin wrappers around memory
>   access var handles, but they make the barrier of entry for using this API 
> somewhat lower.
> 
> A big conceptual shift that comes with this API refresh is that the role of 
> `MemorySegment` and `MemoryAddress` is not
> the same as it used to be; it used to be the case that a memory address could 
> (sometimes, not always) have a back link
> to the memory segment which originated it; additionally, memory access var 
> handles used `MemoryAddress` as a basic unit
> of dereference.  This has all changed as per this API refresh;  now a 
> `MemoryAddress` is just a dumb carrier which
> wraps a pair of object/long addressing coordinates; `MemorySegment` has 
> become the star of the show, as far as
> dereferencing memory is concerned. You cannot dereference memory if you don't 
> have a segment. This improves usability
> in a number of ways - first, it is a lot easier to wrap native addresses 
> (`long`, essentially) into a `MemoryAddress`;
> secondly, it is crystal clear what a client has to do in order to dereference 
> memory: if a client has a segment, it can
> use that; otherwise, if the client only has an address, it will have to 
> create a segment *unsafely* (this can be done
> by calling `MemoryAddress::asSegmentRestricted`).  A list of the API, 
> implementation and test changes is provided
> below. If  you have any questions, or need more detailed explanations, I (and 
> the  rest of the Panama team) will be
> happy to point at existing discussions,  and/or to provide the feedback 
> required.   A big thank to Erik Osterlund,
> Vladimir Ivanov and David Holmes, without whom the work on shared memory 
> segment would not have been possible; also I'd
> like to thank Paul Sandoz, whose insights on API design have been very 
> helpful in this journey.  Thanks  Maurizio
> Javadoc:   
> http://cr.openjdk.java.net/~mcimadamore/8254162_v1/javadoc/jdk/incubator/foreign/package-summary.html
> Specdiff:
> 
> http://cr.openjdk.java.net/~mcimadamore/8254162_v1/specdiff/jdk/incubator/foreign/package-summary.html
> 
> CSR:
> 
> https://bugs.openjdk.java.net/browse/JDK-8254163
> 
> 
> 
> ### API Changes
> 
> * `MemorySegment`
>   * drop factory for restricted segment (this has been moved to 
> `MemoryAddress`, see below)
>   * added a no-arg factory for a native restricted segment representing 
> entire native heap
>   * rename `withOwnerThread` to `handoff`
>   * add new `share` method, to create shared segments
>   * add new `registerCleaner` method, to register a segment against a cleaner
>   * add more helpers to create arrays from a segment e.g. `toIntArray`
>   * add some `asSlice` overloads (to make up for the fact that now segments 
> are more frequently used as cursors)
>   * rename `baseAddress` to `address` (so that `MemorySegment` can implement 
> `Addressable`)
> * `MemoryAddress`
>   * drop `segment` accessor
>   * drop `rebase` method and replace it with `segmentOffset` which returns 
> the offset (a `long`) of this address relative
> to a given segment
> * `MemoryAccess`
>   * New class supporting several static dereference helpers; the helpers are 
> organized by carrier and access mode, where a
> carrier is one of the usual suspect (a Java primitive, minus `boolean`); 
> the access mode can be simple (e.g. access
> base address of given segment), or indexed, in which case the accessor 
> takes a segment and either a low-level byte
> offset,or a high level logical index. The classification is reflected in 
> the naming scheme (e.g. `getByte` vs.
> `getByteAtOffset` vs `getByteAtIndex`).
> * `MemoryHandles`
>   * drop `withOffset` combinator
>   * drop `withStride` combinator
>   * the basic memory access handle factory now returns a var handle which 
> takes a `MemorySegment` and a `long` - from which
> it is easy to derive all the other handles using plain var handle 
> combinators.
> * `Addressable`
>   * This is a new interface which is attached to entities which can be 
> projected to a `MemoryAddress`. For now, both
> `MemoryAddress` and `MemorySegment` implement it; we have plans, with JEP 
> 389 [2] to add more 

Re: RFR: 8235710: Remove the legacy elliptic curves [v2]

[Erik Joelsson]

On Tue, 22 Sep 2020 00:18:07 GMT, Anthony Scarpino  
wrote:

>> This change removes the native elliptic curves library code; as well as, and 
>> calls to that code, tests, and files
>> associated with those libraries.  The makefiles have been changed to remove 
>> from all source builds of the ec code.  The
>> SunEC system property is removed and java.security configurations changed to 
>> reflect the removed curves.  This will
>> remove the following elliptic curves from SunEC:   secp112r1, secp112r2, 
>> secp128r1, secp128r2, secp160k1, secp160r1,
>> secp160r2, secp192k1, secp192r1, secp224k1, secp224r1, secp256k1, sect113r1, 
>> sect113r2, sect131r1, sect131r2,
>> sect163k1, sect163r1, sect163r2, sect193r1, sect193r2, sect233k1, sect233r1, 
>> sect239k1, sect283k1, sect283r1,
>> sect409k1, sect409r1, sect571k1, sect571r1, X9.62 c2tnb191v1, X9.62 
>> c2tnb191v2, X9.62 c2tnb191v3, X9.62 c2tnb239v1,
>> X9.62 c2tnb239v2, X9.62 c2tnb239v3, X9.62 c2tnb359v1, X9.62 c2tnb431r1, 
>> X9.62 prime192v2, X9.62 prime192v3, X9.62
>> prime239v1, X9.62 prime239v2, X9.62 prime239v3, brainpoolP256r1 
>> brainpoolP320r1, brainpoolP384r1, brainpoolP512r1
>
> Anthony Scarpino has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   remove JDKOPT_DETECT_INTREE_EC from configure.ac

Build changes look good.

Marked as reviewed by erikj (Reviewer).

-

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

Re: RFR: 8235710: Remove the legacy elliptic curves

[Erik Joelsson]

On Mon, 21 Sep 2020 21:10:58 GMT, Anthony Scarpino  
wrote:

> This change removes the native elliptic curves library code; as well as, and 
> calls to that code, tests, and files
> associated with those libraries.  The makefiles have been changed to remove 
> from all source builds of the ec code.  The
> SunEC system property is removed and java.security configurations changed to 
> reflect the removed curves.  This will
> remove the following elliptic curves from SunEC:   secp112r1, secp112r2, 
> secp128r1, secp128r2, secp160k1, secp160r1,
> secp160r2, secp192k1, secp192r1, secp224k1, secp224r1, secp256k1, sect113r1, 
> sect113r2, sect131r1, sect131r2,
> sect163k1, sect163r1, sect163r2, sect193r1, sect193r2, sect233k1, sect233r1, 
> sect239k1, sect283k1, sect283r1,
> sect409k1, sect409r1, sect571k1, sect571r1, X9.62 c2tnb191v1, X9.62 
> c2tnb191v2, X9.62 c2tnb191v3, X9.62 c2tnb239v1,
> X9.62 c2tnb239v2, X9.62 c2tnb239v3, X9.62 c2tnb359v1, X9.62 c2tnb431r1, X9.62 
> prime192v2, X9.62 prime192v3, X9.62
> prime239v1, X9.62 prime239v2, X9.62 prime239v3, brainpoolP256r1 
> brainpoolP320r1, brainpoolP384r1, brainpoolP512r1

make/autoconf/jdk-options.m4 line 234:

> 232: # Enable or disable the elliptic curve crypto implementation
> 233: #
> 234: AC_DEFUN_ONCE([JDKOPT_DETECT_INTREE_EC],

There should be a call to this macro from either configure.ac or this file that 
also needs to be removed.

-

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

Re: RFR: 8253208: Move CDS related code to a separate class [v2]

[Erik Joelsson]

On Mon, 21 Sep 2020 18:17:55 GMT, Yumin Qi  wrote:

>> With more CDS related code added to VM, it is time to move CDS code to a 
>> separate class. CDS is the new class which is
>> specific to CDS.
>> Tests: tier1-4
>
> Yumin Qi has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   8253208: Move CDS related code to a separate class

Build changes look good.

-

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

Re: RFR 8240261: Use make/templates/gpl-cp-header in FieldGen.java

[Erik Joelsson]

Build changes look ok.

/Erik

On 2020-02-29 05:02, Weijun Wang wrote:

Please take a review at

https://cr.openjdk.java.net/~weijun/8240261/webrev.00

Thanks,
Max

Re: RFR 8234697: Generate sun.security.util.math.intpoly classes during build

[Erik Joelsson]

On 2019-11-26 16:39, Weijun Wang wrote:



On Nov 27, 2019, at 12:14 AM, Erik Joelsson  wrote:

On 2019-11-25 16:42, Weijun Wang wrote:

On Nov 26, 2019, at 12:36 AM, Erik Joelsson  wrote:

Build change looks good.

Thanks.

One question: I see the output to stdout from FieldGen.java shown in build. Is 
it possible to hide it but still store the output in the log file?

No, we are not able to support different log levels to the main log file and console. What 
you can do is add $(LOG_INFO) or $(LOG_DEBUG) last on the command line. That macro resolves 
to "> /dev/null" when we don't want the output printed. How much output and how 
interesting is it to see? You can also wrap the whole command in a call to ExecuteWithLog to 
have it being logged to an individual file, which gets repeated at the end of the build in 
case it fails.

I think you should be able to combine the two with something like this:

$(call ExecuteWithLog, ...) $(LOG_DEBUG)

That should print everything to the special log file as well as letting it pass 
to stdout when LOG=debug, but I haven't tested it.

The log shows on the console with LOG=debug but whatever LOG level I choose, 
the output is always collected in the log file. This is exactly what I am 
looking for.


Note that it will not show up in build.log, but in a special log file 
just for this command, along with a .cmdline file for this command. The 
location of these files need to be provided to ExecuteWithLog in the 
first argument, as a basename for the files. In this case it would make 
sense to store them next to the touch file of the rule.


/Erik


Thanks,
Max


/Erik


I copied everything from CLDR_GEN_DONE but that one does not log anything 
actually.

I can remove all output too, not really important.

Thanks,
Max


/Erik

On 2019-11-22 18:59, Weijun Wang wrote:

Please review the change at

http://cr.openjdk.java.net/~weijun/8234697/webrev.00/

The new lines in Gensrc-java.base.gmk mimics the one for CLDR_GEN_DONE at the 
beginning of the same file.

I changed the BigInteger parameter in the FieldParams constructor (the one not 
reading terms) to its HEX string form and is now using the inverse. This makes 
sure the strings appeared here are exactly the same as the one used in 
CurveDB.java. The generated source code is identical to before.

Other smaller changes made to FieldGen.jsh:

1. Package name
2. No more jshell, but now Java
3. Input/output paths as args for main()
4. White spaces, wrapping and indentation

Thanks,
Max

Re: RFR 8234697: Generate sun.security.util.math.intpoly classes during build

[Erik Joelsson]

On 2019-11-25 16:42, Weijun Wang wrote:



On Nov 26, 2019, at 12:36 AM, Erik Joelsson  wrote:

Build change looks good.

Thanks.

One question: I see the output to stdout from FieldGen.java shown in build. Is 
it possible to hide it but still store the output in the log file?


No, we are not able to support different log levels to the main log file 
and console. What you can do is add $(LOG_INFO) or $(LOG_DEBUG) last on 
the command line. That macro resolves to "> /dev/null" when we don't 
want the output printed. How much output and how interesting is it to 
see? You can also wrap the whole command in a call to ExecuteWithLog to 
have it being logged to an individual file, which gets repeated at the 
end of the build in case it fails.


I think you should be able to combine the two with something like this:

$(call ExecuteWithLog, ...) $(LOG_DEBUG)

That should print everything to the special log file as well as letting 
it pass to stdout when LOG=debug, but I haven't tested it.


/Erik


I copied everything from CLDR_GEN_DONE but that one does not log anything 
actually.

I can remove all output too, not really important.

Thanks,
Max


/Erik

On 2019-11-22 18:59, Weijun Wang wrote:

Please review the change at

http://cr.openjdk.java.net/~weijun/8234697/webrev.00/

The new lines in Gensrc-java.base.gmk mimics the one for CLDR_GEN_DONE at the 
beginning of the same file.

I changed the BigInteger parameter in the FieldParams constructor (the one not 
reading terms) to its HEX string form and is now using the inverse. This makes 
sure the strings appeared here are exactly the same as the one used in 
CurveDB.java. The generated source code is identical to before.

Other smaller changes made to FieldGen.jsh:

1. Package name
2. No more jshell, but now Java
3. Input/output paths as args for main()
4. White spaces, wrapping and indentation

Thanks,
Max

Re: RFR 8234697: Generate sun.security.util.math.intpoly classes during build

[Erik Joelsson]

Build change looks good.

/Erik

On 2019-11-22 18:59, Weijun Wang wrote:

Please review the change at

http://cr.openjdk.java.net/~weijun/8234697/webrev.00/

The new lines in Gensrc-java.base.gmk mimics the one for CLDR_GEN_DONE at the 
beginning of the same file.

I changed the BigInteger parameter in the FieldParams constructor (the one not 
reading terms) to its HEX string form and is now using the inverse. This makes 
sure the strings appeared here are exactly the same as the one used in 
CurveDB.java. The generated source code is identical to before.

Other smaller changes made to FieldGen.jsh:

1. Package name
2. No more jshell, but now Java
3. Input/output paths as args for main()
4. White spaces, wrapping and indentation

Thanks,
Max

Re: RFR: 8231351: Add notes for PKCS11 tests in the test doc

[Erik Joelsson]

Sure, will do.

/Erik

On 2019-09-23 20:57, Jia Huang wrote:

Hi Erik,

Thank you for your review and valuable comments.

Updated: http://cr.openjdk.java.net/~jiefu/8231351-huangjia/webrev.01/
 - The reference to the pkcs11 README and the reviewers had been added.

Please note the user in the patch.
Hope you wouldn't mind it. Thanks.

Could you please sponsor it?

Thanks a lot.
Best regards,
Jia

在 2019年09月23日 23:18, Erik Joelsson 写道:
I think this type of comment fits well in the top level test doc. It 
just provides basic instructions for setting up these tests so that 
they pass without going into too much detail. Perhaps a reference to 
the pkcs11 README for more details would be a good idea.


Looks good to me.

/Erik

On 2019-09-23 05:54, sha.ji...@oracle.com wrote:

Hi Jia,
I think this isn't a general testing problem.
It may not worthy of highlighting this point in the JDK testing doc.
In fact, PKCS11 tests have their own doc at: 
test/jdk/sun/security/pkcs11/README


Best regards,
John Jiang

On 2019/9/23 18:04, Jia Huang wrote:

Hi all,

JBS:    https://bugs.openjdk.java.net/browse/JDK-8231351
Webrev: http://cr.openjdk.java.net/~jiefu/8231351-huangjia/webrev.00/

sun/security/pkcs11/Secmod/AddTrustedCert.java failed on Ubuntu 18.04.
According to the comments in JDK-8231338, it was caused by the 
improper NSS libs of the system.


These failures are confusing and hard to diagnose.
It might be better to add some notes for the pkcs11 tests.

Thanks a lot.

Best regards,
Jia

Re: RFR: 8231351: Add notes for PKCS11 tests in the test doc

[Erik Joelsson]

I think this type of comment fits well in the top level test doc. It 
just provides basic instructions for setting up these tests so that they 
pass without going into too much detail. Perhaps a reference to the 
pkcs11 README for more details would be a good idea.


Looks good to me.

/Erik

On 2019-09-23 05:54, sha.ji...@oracle.com wrote:

Hi Jia,
I think this isn't a general testing problem.
It may not worthy of highlighting this point in the JDK testing doc.
In fact, PKCS11 tests have their own doc at: 
test/jdk/sun/security/pkcs11/README


Best regards,
John Jiang

On 2019/9/23 18:04, Jia Huang wrote:

Hi all,

JBS:    https://bugs.openjdk.java.net/browse/JDK-8231351
Webrev: http://cr.openjdk.java.net/~jiefu/8231351-huangjia/webrev.00/

sun/security/pkcs11/Secmod/AddTrustedCert.java failed on Ubuntu 18.04.
According to the comments in JDK-8231338, it was caused by the 
improper NSS libs of the system.


These failures are confusing and hard to diagnose.
It might be better to add some notes for the pkcs11 tests.

Thanks a lot.

Best regards,
Jia

Re: [13] RFR: JDK-8225392: Comparison builds are failing due to cacerts file

[Erik Joelsson]

I'm happy with this.

/Erik

On 2019-06-14 08:33, Weijun Wang wrote:

Here is the updated webrev

   http://cr.openjdk.java.net/~weijun/8225392/webrev.01/

The only change is ordering in 'keytool -list' and its test.

Thanks,
Max


On Jun 14, 2019, at 7:55 PM, Sean Mullan  wrote:

On 6/14/19 1:49 AM, Weijun Wang wrote:

BTW, something not related but similar: Do you like me to also sort aliases 
alphabetically in the output of "keytool -list"?

Yes, I think that is useful.

--Sean

Re: RFR: JDK-8225392: Comparison builds are failing due to cacerts file

[Erik Joelsson]

Hello,

We cannot rely on querying mercurial at build time. The source must be 
buildable from a source distribution.


/Erik

On 2019-06-12 11:39, Sean Mullan wrote:
Using the certificate's notBefore date as the KeyStore entry creation 
date is misleading since many of these root certs were not integrated 
into the JDK until after they were created by the CA. Can we somehow 
extract the last revision time of each PEM file instead? That is more 
aligned with the previous creation date that we used.


--Sean

On 6/12/19 12:38 PM, Erik Joelsson wrote:

Hello Max,

Much appreciated! I will need to have this fixed one way or other in 
JDK 13, so depending on if you get your fix there in time, I will 
retract my proposal. If your fix only hits 14, I will push mine to 13.


/Erik

On 2019-06-12 08:41, Weijun Wang wrote:

This is my version of the fix:

    http://cr.openjdk.java.net/~weijun/8225392/webrev.00/

Now you can still compare cacerts bit by bit.

Thanks,
Max

On Jun 12, 2019, at 10:50 PM, Weijun Wang  
wrote:


Hi Erik,

Are you going to fix this bug soon?

I am inspired by Martin's words and would like to update 
GenerateCacerts.java so that as long as the certs and their aliases 
are unchanged, the output cacerts will always be the same. I can 
send out a code review today.


Thanks,
Max

On Jun 12, 2019, at 10:59 AM, Weijun Wang  
wrote:


Good idea about the creation time.

--Max

On Jun 12, 2019, at 10:53 AM, Martin Buchholz 
 wrote:


Google culture really likes build output determinism, and we 
recently built our own cacerts generator.


To get determinism, we are using cert digest as alias (must have 
a unique alias, but value doesn't seem to matter much), and using 
cert notBefore instead of current (build) timestamp.


On Mon, Jun 10, 2019 at 12:40 PM Erik Joelsson 
 wrote:
Since JDK-8193255, when we started generating the cacerts file in 
the
build, the build compare baseline builds have started failing. It 
seems
the cacerts binary file has some non determinism built in so it 
doesn't

get generated exactly the same given the same input. This patch adds
special handling when comparing that file by comparing the output of
"keytool -list" on the files instead.

Bug: https://bugs.openjdk.java.net/browse/JDK-8225392

Webrev: http://cr.openjdk.java.net/~erikj/8225392/webrev.01/

/Erik

Re: RFR: JDK-8225392: Comparison builds are failing due to cacerts file

[Erik Joelsson]

Hello Max,

Much appreciated! I will need to have this fixed one way or other in JDK 
13, so depending on if you get your fix there in time, I will retract my 
proposal. If your fix only hits 14, I will push mine to 13.


/Erik

On 2019-06-12 08:41, Weijun Wang wrote:

This is my version of the fix:

http://cr.openjdk.java.net/~weijun/8225392/webrev.00/

Now you can still compare cacerts bit by bit.

Thanks,
Max


On Jun 12, 2019, at 10:50 PM, Weijun Wang  wrote:

Hi Erik,

Are you going to fix this bug soon?

I am inspired by Martin's words and would like to update GenerateCacerts.java 
so that as long as the certs and their aliases are unchanged, the output 
cacerts will always be the same. I can send out a code review today.

Thanks,
Max


On Jun 12, 2019, at 10:59 AM, Weijun Wang  wrote:

Good idea about the creation time.

--Max


On Jun 12, 2019, at 10:53 AM, Martin Buchholz  wrote:

Google culture really likes build output determinism, and we recently built our 
own cacerts generator.

To get determinism, we are using cert digest as alias (must have a unique 
alias, but value doesn't seem to matter much), and using cert notBefore instead 
of current (build) timestamp.

On Mon, Jun 10, 2019 at 12:40 PM Erik Joelsson  wrote:
Since JDK-8193255, when we started generating the cacerts file in the
build, the build compare baseline builds have started failing. It seems
the cacerts binary file has some non determinism built in so it doesn't
get generated exactly the same given the same input. This patch adds
special handling when comparing that file by comparing the output of
"keytool -list" on the files instead.

Bug: https://bugs.openjdk.java.net/browse/JDK-8225392

Webrev: http://cr.openjdk.java.net/~erikj/8225392/webrev.01/

/Erik

Re: RFR 6722928: Support SSPI as a native GSS-API provider

[Erik Joelsson]

Hello Max,

I believe $(call FindSrcDirsForLib, $(MODULE), sspi_bridge) for SRC is 
the default so you shouldn't need to specify it explicitly, or did you 
experience some problems with the default that I'm missing?


/Erik

On 2019-06-09 19:30, Weijun Wang wrote:

Updated webrev at

http://cr.openjdk.java.net/~weijun/6722928/webrev.08/

@build-dev guys, I realize I've never included you in this code review. Please 
take a look.

@Valerie, All comments accepted except for one (see below). In fact, I think I 
found a bug in gss_release_context that it might release a cred handle passed 
in, so I add a isLocalCred flag. However, I test it on my local Windows and it 
seems the same handle can be FreeCredentialsHandle and then used and then freed 
again.


On Jun 7, 2019, at 10:45 AM, Valerie Peng  wrote:

Hi, Max,



- line 424: the "(used to be const)" comment can now be removed.



- line 396-403: on line 338, there is no need to go to err as no memory has 
been allocated. What happens when jumping to err but the variables, i.e. value 
and name, have not been declared? Line 400-401 seems not used as there is no 
more goto err after line 391.

- line 528: the size of buffer here is 4*len + 1, but then when calling 
WideCharToMultiByte, the 6th argument is len. Seems inconsistent? line 534: shouldn't we 
free "buffer" here?

- line 596: free cred allocated on line 588? line 610 and 617: free cred and 
cred->phCredK? line 638 and 644, 648 and 653: free cred, cred->phCredK and 
cred->phCredS?

- line 829: free the context handle allocated on line 807? line 879: free 
newCred? line 901: no memory de-allocation before returning error? line 921: 
seems redundant given line 918.

- line 1071: based on gss api doc, context_handle should be set to 
GSS_C_NO_CONTEXT after deletion.

- line 1333: what about secBuff[1].pvBuffer?

According to the DecryptMessage spec, "The encrypted message is decrypted in place, 
overwriting the original contents of its buffer". I've printed out both 
secBuff[0].pvBuffer and secBuff[1].pvBuffer after the decryption and the latter is indeed 
inside the former.


- line 1390, 1393, 1397: call gss_release_oid_set before returning failure?

- line 1471: should we return an error code here when FormatMessage() call 
failed?

Rest looks fine.

Thanks,

Valerie

Thanks,
Max

[1] 
https://docs.microsoft.com/en-us/windows/desktop/api/sspi/nf-sspi-decryptmessage


On 6/4/2019 2:52 AM, Weijun Wang wrote:

I uploaded an updated webrev in place. The only changes are:

1. s/SSPI_TRACE/SSPI_BRIDGE_TRACE/ in sspi.cpp
2. Several copyright year updates.
3. Remove one unused import.

Thanks,
Max


On May 30, 2019, at 11:18 AM, Weijun Wang  wrote:

Here is the latest webrev

   http://cr.openjdk.java.net/~weijun/6722928/webrev.07/

Re: RFR 8193255: Root Certificates should be stored in text format and assembled at build time

[Erik Joelsson]

This looks good to me, and thanks for fixing the other instances of 
BUILD_TOOLS_JDK!


/Erik

On 2019-05-30 20:00, Weijun Wang wrote:

New webrev at

https://cr.openjdk.java.net/~weijun/8193255/webrev.01

Changes:

1. Textual info at the beginning of each cert

2. Makefile: indentation, BUILD_TOOLS_JDK, MakeTargetDir, files instead of dir

Thanks,
Max


On May 31, 2019, at 1:34 AM, Erik Joelsson  wrote:

On 2019-05-30 08:32, Weijun Wang wrote:

On May 30, 2019, at 10:01 PM, Erik Joelsson  wrote:

In my experience, using directories for dependencies in make does not work 
well. Since all the files in make/data/cacerts are in a flat structure, I would 
recommend expressing the prerequisites as:

$(wildcard $(GENDATA_CACERTS_SRC)/*)

This will not cover the case where a file is removed, but that case is rarely 
handled well in make based build systems.

But in my experiment, using the directory name does detect the file removal.

It believe that worked well on your machine, but directory timestamp updates 
are file system dependent. I'm not sure we can count on all filesystems to 
accurately reflect time stamps based on file modification. It's also possible 
that an OS would touch directory timestamps for other reasons, which should not 
affect the build. I haven't tried having source directories as prerequisites 
before, so I simply don't know how reliable it is. My experience is rather with 
directories as targets, which certainly doesn't work. If you verified that it 
worked as expected on all supported OSes, I would be less worried.


Or, can I list *both* the files and the directory to get maximum awareness?

The directory modification time will usually not change when a file in it is 
modified, only when adding or removing files (and possibly some other 
operations), so adding the files is certainly a must. If you go with both, then 
I would just be worried about potential false positives.

/Erik

Re: RFR 8193255: Root Certificates should be stored in text format and assembled at build time

[Erik Joelsson]

On 2019-05-30 08:32, Weijun Wang wrote:



On May 30, 2019, at 10:01 PM, Erik Joelsson  wrote:

In my experience, using directories for dependencies in make does not work 
well. Since all the files in make/data/cacerts are in a flat structure, I would 
recommend expressing the prerequisites as:

$(wildcard $(GENDATA_CACERTS_SRC)/*)

This will not cover the case where a file is removed, but that case is rarely 
handled well in make based build systems.

But in my experiment, using the directory name does detect the file removal.


It believe that worked well on your machine, but directory timestamp 
updates are file system dependent. I'm not sure we can count on all 
filesystems to accurately reflect time stamps based on file 
modification. It's also possible that an OS would touch directory 
timestamps for other reasons, which should not affect the build. I 
haven't tried having source directories as prerequisites before, so I 
simply don't know how reliable it is. My experience is rather with 
directories as targets, which certainly doesn't work. If you verified 
that it worked as expected on all supported OSes, I would be less worried.



Or, can I list *both* the files and the directory to get maximum awareness?


The directory modification time will usually not change when a file in 
it is modified, only when adding or removing files (and possibly some 
other operations), so adding the files is certainly a must. If you go 
with both, then I would just be worried about potential false positives.


/Erik

Re: RFR 8193255: Root Certificates should be stored in text format and assembled at build time

[Erik Joelsson]

Hello Max,

Looking in ToolsJdk.gmk, I realize that the BUILD_TOOLS variable was 
renamed back when we unified the repositories and is now called 
BUILD_TOOLS_JDK. It seems like I missed updating the references to this 
variable in the gendata dir. If you use the new variable name in the 
prerequisites list, you get a rebuild. Feel free to update the other 
references in make/gendata if you want to.


In my experience, using directories for dependencies in make does not 
work well. Since all the files in make/data/cacerts are in a flat 
structure, I would recommend expressing the prerequisites as:


$(wildcard $(GENDATA_CACERTS_SRC)/*)

This will not cover the case where a file is removed, but that case is 
rarely handled well in make based build systems.


Some minor notes.

The current recommended macro for creating the target directory in a 
recipe is $(call MakeTargetDir).


For logical indent (Gendata-java.base.gmk:75 and Copy-java.base.gmk:173) 
please use 2 spaces [1].


I also think it would be good with a comment in Copy-java.base.gmk 
explaining that CACERTS_FILE is optionally set in configure to override 
the default cacerts which is otherwise generated in Gendata-java.base.gmk.


Thanks,

/Erik

[1] http://openjdk.java.net/groups/build/doc/code-conventions.html

On 2019-05-30 06:34, Weijun Wang wrote:

Since I need to track all added, removed, updated files in that directory, I 
thought depending on the directory itself is more correct. If I use the 
FindFiles function, then if some files are removed the cacerts will not be 
rebuilt.

--Max


On May 30, 2019, at 9:11 PM, David Holmes  wrote:

Hi Max,

Not a review :)

On 30/05/2019 11:01 pm, Weijun Wang wrote:

Please take a review at
http://cr.openjdk.java.net/~weijun/8193255/webrev.00/
Please pay attention to the 1st 3 and the last 2 files. Others are PEM files 
for all certs inside the original cacerts.
There is one thing I cannot get correct. If I update the GenerateCacerts.java 
file and rerun make, the cacerts file is unchanged. I thought the following line
$(GENDATA_CACERTS): $(BUILD_TOOLS) $(GENDATA_CACERTS_SRC)
means when when the tool is changed, GENDATA_CACERTS will be called.

I think you have set a dependency on the directory, not the files within it. If 
you look at make/gensrc/Gensrc-jdk.internal.vm.compiler.gmk for example you'll 
see this rule:

$(GENSRC_DIR)/_gensrc_proc_done: $(PROC_SRCS) $(PROCESSOR_JARS)

but PROC_SRCS is defined as

PROC_SRCS := $(filter %.java, $(call FindFiles, $(PROC_SRC_DIRS)))

which is all the .java files in the src directory.

David


Thanks,
Max

Re: RFR 8205445: Add RSASSA-PSS Signature support to SunMSCAPI

[Erik Joelsson]

Build change looks good.

/Erik


On 2018-06-21 04:12, Weijun Wang wrote:

Please take a review on this change

   http://cr.openjdk.java.net/~weijun/8205445/webrev.00/

and the release note at


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

The code change adds RSASSA-PSS signature support to the SunMSCAPI provider.

Several notes:

1. CryptoAPI (which SunMSCAPI is based on and now a deprecated technology) does 
not support RSASSA-PSS. In fact, CNG [1] is used to perform the signing and 
verification. This is certainly not a perfect solution and we are thinking of 
support CNG in a more sophisticated way in future releases of JDK.

2. For unknown reason, the newly added verification code for RSASSA-PSS does 
not work correctly (precisely, ::NCryptTranslateHandle returns 
NTE_INVALID_PARAMETER). A fallback mechanism is added into 
mscapi/RSASignature.java. A SunRsaSign Signature object is actually used when a 
SunMSCAPI Signature is initialized to verify an RSASSA-PSS signature.

3. It looks like CNG only supports PSSParamterSpec with the same message hash 
algorithm and MGF1 hash algorithm, because there is only one algorithm field in 
BCRYPT_PSS_PADDING_INFO [2]. This is checked when setting the parameter.

4. It looks like CNG only supports RSASSA-PSS using these hash algorithms: SHA-1, 
SHA-256, SHA-384, and SHA-512. This is not checked at parameter setting but sign() will 
throw a SignatureException saying "Unrecognised hash algorithm". Since the 
verify() side uses a fallback SunRsaSign signature, other hash algorithms are supported.

Thanks
Max

[1] 
https://msdn.microsoft.com/en-us/library/windows/desktop/aa376210(v=vs.85).aspx
[2] 
https://msdn.microsoft.com/en-us/library/windows/desktop/aa375529(v=vs.85).aspx
[3] 
https://msdn.microsoft.com/en-us/library/windows/desktop/aa375534(v=vs.85).aspx

Re: RFR (XL): JDK-8204572 SetupJdkLibrary should setup SRC and -I flags automatically

[Erik Joelsson]

Looks good.

/Erik


On 2018-06-08 01:50, Magnus Ihse Bursie wrote:

On 2018-06-07 23:20, Erik Joelsson wrote:

Hello Magnus,

Very nice refactoring!

Thanks!



JdkNativeCompilation.gmk
line 126-127 looks a bit long. There is an extra space on 126. Also, 
why not addprefix for adding -I instead of clunky foreach? Not that I 
care greatly, but I usually prefer that construct.


Yeah, I agree, addprefix is better. I just forgot about it; foreach is 
a nice allround tool.


Updated webrev: 
http://cr.openjdk.java.net/~ihse/JDK-8204572-autodetect-SRC-and-headers-dirs/webrev.02/

(Only changes is in JdkNativeCompilation.gmk, as per above comments).

/Magnus



Otherwise looks good.

/Erik


On 2018-06-07 13:22, Magnus Ihse Bursie wrote:

This change needs some background information.

I've been working on simplifying and streamlining the compilation of 
native libraries of the JDK. Previously, I introduced the 
SetupJdkLibrary function, and started to get a better control of 
compiler flags. This patch continues on both paths.


The original intent of this change, which I naively thought was 
going to be much simpler than it turned out :-) was to separate the 
-I flags (location of header files) from other flags, and to 
generate these automatically, wherever possible. Since we have a 
standard way of locating native code, and most libraries just want 
to have an -I path to their own source base and the generated Java 
header, we should be able to provide this in SetupJdkLibrary.


This turned out to be closely related to SetupJdkLibrary being able 
to discover the location of the SRC directories itself, using 
"convention over configuration" and assuming that the library 
"libfoo" for "java.module" would be located in 
java.module/*/native/libfoo.


While this sounds simple in theory, when actually trying to 
implement this I of course ran into all the places where some 
special handling was indeed needed. So even if like 90% of all 
libraries were simple to get to build using automated discovery of 
source and header directories, the 10% that did not caused me much 
more headaches than I had anticipated. On the other hand, now that 
I've sorted out all those places, the few remaining odd solutions is 
clearly documented and not just something that "just happens" due to 
strange configurations.


One file deserves mentioning specifically: Awt2dLibraries.gmk. The 
java.desktop libraries are unfortunately quite entangled with each 
other, and do not readily follow the patterns that are used 
elsewhere in the code base. So it might just look like the file has 
just gone from one state of messiness, to another, which would 
hardly be an improvement. :-( I would still argue that the new 
messiness is better: It is now much clearer in what ways the 
libraries diverge from our standard assumption, and what course of 
action needs to be taken to minimize these differences. (Which is 
something I believe should be done -- these issues are not just 
cosmetic but is the root of most of the issues we always see for 
these libraries, when upgrading compilers, etc.)


During this change, I noticed that not all native libraries include 
the proper generated header file. This is a dangerous coding 
practice, since a change in the Java part of the interface might not 
get picked up properly in the native part. I've added the missing 
includes that I've detected, and due to these changes, I'm also 
including the component teams in what is really only a build change. 
As can be seen for jdk.crypto.mscapi; there had indeed been changes 
that needed correcting.


Since this is (basically) a pure build change, my gold standard here 
has been the build compare script. In essence, the build output 
prior to my change and with this change are 100% identical. In 
truth, this is a bit too strong a claim. A few changes has occurred, 
but none of them should matter. Here's a breakdown of the compare.sh 
results:


* Windows-x64:

No differences at all.

* Solaris:

Two libraries are reported to differ: libsaproc.so and 
libfontmanager.so, both with a disass diff on ~700 bytes. Analyzing 
this, I found that the object files used to link these two libraries 
has no disass differences. They have a slight binary difference and 
a difference in size, due to the include paths being different (and 
this is stored in the .o file, which makes it different). Somehow 
this apparently triggers the linker to generate a slightly different 
code in a few places, using a different register or so. (Weird...)


* MacOS:

Two libraries are reported to differ: libjava.dylib and 
libmlib_image.dylib, both of them just reported as a binary diff (no 
symbol, disass or fulldump differences). This is not really 
unsuspected, but I analyzed it anyway.


I found that for libjava.dylib, a single .o file was different. This 
one was actually picked up from closed sources, and are not really 
relevant for OpenJDK. Anyway, the

Re: RFR 8201815: Use Mozilla Public Suffix List

[Erik Joelsson]

On 2018-05-23 03:54, Magnus Ihse Bursie wrote:

mv should not modify attributes. cp will, but mv should not.

Your solution might fail in the (admittedly unlikely) case that the build is 
interrupted before the chmod but after the mv. In that case, an incremental 
rebuild will not see that anything is missing.

I believe the other cases that you quote are also incorrect.

But I'd like to hear Erik's input on this as well.
We have the pseudo target .DELETE_ON_ERROR defined globally (in 
MakeBase.gmk) which causes make to delete the target of any failed or 
interrupted recipe. This should actually remove any need for using .tmp 
files and mv on the last line. We still have a lot of those constructs 
around since forever though. The recipe copied here (and the two other 
examples) are based on a template from very early build-infra makefile 
history and do not represent current best practices. If anything I would 
recommend getting rid of the .tmp and mv completely, but if you prefer 
both belt and suspenders, putting the move last should be the correct 
construct.


/Erik

/Magnus


23 maj 2018 kl. 02:01 skrev Weijun Wang <weijun.w...@oracle.com>:




On May 23, 2018, at 4:21 AM, Magnus Ihse Bursie <magnus.ihse.bur...@oracle.com> 
wrote:

... but you should switch order on the chmod and the mv in the new gensrc file, 
so the mv comes last.

I thought it's safer to call CHMOD last so MV won't change file mode back. (I'm 
not saying it will, just afraid.)

In below cases, CHMOD is called after MV/CP.

gendata/Gendata-java.base.gmk
59-$(MV) $@.tmp $@
60:$(CHMOD) 444 $@
61-

common/JavaCompilation.gmk
80-$(CP) $$< $$@
81:$(CHMOD) -f ug+w $$@

Thanks
Max


/Magnus


22 maj 2018 kl. 17:44 skrev Erik Joelsson <erik.joels...@oracle.com>:

Build changes look ok.

/Erik



On 2018-05-22 08:25, Weijun Wang wrote:
Please take a review at

  http://cr.openjdk.java.net/~weijun/8201815/webrev.00/

With this change, We switch from a home-grown public suffix list (implemented 
in sun/net/RegisteredDomain.java) to Mozilla's PSL. The PSL data was re-encoded 
as a zip file with entries for different TLDs.

There is no plan to update the data in a different channel other than a JDK 
release.

Thanks
Max

Re: RFR 8201815: Use Mozilla Public Suffix List

[Erik Joelsson]

Build changes look ok.

/Erik


On 2018-05-22 08:25, Weijun Wang wrote:

Please take a review at

http://cr.openjdk.java.net/~weijun/8201815/webrev.00/

With this change, We switch from a home-grown public suffix list (implemented 
in sun/net/RegisteredDomain.java) to Mozilla's PSL. The PSL data was re-encoded 
as a zip file with entries for different TLDs.

There is no plan to update the data in a different channel other than a JDK 
release.

Thanks
Max

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

[Erik Joelsson]

Build changes still look good to me.

/Erik


On 2018-03-28 03:31, Magnus Ihse Bursie wrote:

On 2018-03-28 01:52, Weijun Wang wrote:


On Mar 24, 2018, at 6:03 AM, Magnus Ihse Bursie 
 wrote:


https://bugs.openjdk.java.net/browse/JDK-8200193 -- for 
jdk.security.auth
There is only one function to export and it already has JNIEXPORT, so 
you can just remove the new $(LIBJAAS_CFLAGS) [1].

Ok, thanks Max!

Are you going to update your webrev?
Here is a new webrev. It includes your recommended change in 
Lib-jdk.security.auth.gmk.


It is also updated to keep track of changes in shared native libraries 
that has happend in the mainline since my first webrev. Most notably 
is the addition of libjsig. For now, I have just added the JNIEXPORT 
markers for the platforms that need it. Hopefully we can unify libjsig 
across all platforms, but that seems to be more complicated than I 
thought, so that'll have to wait.


I have also recieved word from Phil Race that there were no testing 
issues for client, so he's happy as well.


Updated webrev: 
http://cr.openjdk.java.net/~ihse/JDK-8200178-remove-mapfiles/webrev.03


/Magnus



Thanks
Max

[1] 
http://cr.openjdk.java.net/~ihse/JDK-8200178-remove-mapfiles/webrev.01/make/lib/Lib-jdk.security.auth.gmk.sdiff.html

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

[Erik Joelsson]

I have looked at the build changes and they look good.

Will you file followups for each component team to look over their 
exported symbols, at least for the libraries with $(EXPORT_ALL_SYMBOLS)? 
It sure looks like there is some technical debt laying around here.


/Erik


On 2018-03-23 06:56, 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 
behavior).
 * The versioning tag SUNWprivate_1.1 is not included, and thus 
neither the .gnu.version_d symbol.
 * There are a few differences in the symbol and/or mangling of some 
local functions. I'm not sure what's causing this,

but it's unlikely to have any effect on the product.

Another common source for change in symbols is due to previous 
platform differences. For instance, if we had "JNIEXPORT int JNICALL 
do_foo() { ... }", but do_foo was not in the mapfile, the symbol was 
exported on Windows but not on Linux and Solaris. (Presumable since it 
was not needed there, even though it was compiled for those platforms 
as well.) Now, with the mapfiles gone, do_foo() will be exported on 
all platforms. And contrary, functions that are compiled on all 
platforms, and were exported in mapfiles, but now have gotten an 
JNIEXPORT 

Re: RFR: JDK-8199640 Split up BUILD_LIBKRB5 into the two, unrelated compilations it consists of

[Erik Joelsson]

Looks good.

/Erik


On 2018-03-14 16:18, Magnus Ihse Bursie wrote:
BUILD_LIBKRB5 is currently a strange chimera between compiling 
w2k_lsa_auth on windows, and osxkrb5 on macos. They do not share 
source code, name or compilation options.


This patch separates them into two separate compilations, one for each 
platform, so that they then can follow the normal JDK pattern for 
compiling libs.


Bug: https://bugs.openjdk.java.net/browse/JDK-8199640
WebRev: 
http://cr.openjdk.java.net/~ihse/JDK-8199640-split-up-BUILD_LIBJRB5/webrev.01


/Magnus

Re: RFR: JDK-8199636 Unify naming for jaas_unix and jaas_nt

[Erik Joelsson]

Looks good.

/Erik


On 2018-03-14 14:50, Magnus Ihse Bursie wrote:
For some odd reason, the native library compiled for jdk.security.auth 
is called jaas_unix on unix and jaas_nt on windows. There's no good 
reason for this, and it breaks with the common practice in OpenJDK.


This patch renames both libraries to the basename "jaas" (that is, 
libjaas.so, libjaas.dylib or jaas.dll, depending on OS).


Bug: https://bugs.openjdk.java.net/browse/JDK-8199636
WebRev: 
http://cr.openjdk.java.net/~ihse/JDK-8199636-rename-libjaas/webrev.01


/Magnus

Re: RFR 8148371: Remove policytool

[Erik Joelsson]

From a build point of view this looks good.

/Erik


On 2017-09-06 06:17, Weijun Wang wrote:

Hi All

Please review the change, which spans to root, jdk and langtools repos.

   http://cr.openjdk.java.net/~weijun/8148371/

I've searched for the "policytool" word in the whole jdk10/jdk10 forests, 
removed all files having the word inside the path name, and remove almost all occurrences 
of the word in other places.

The exceptions are:

1. Two files with the jdk8 word in file name. I assume I should not touch them. 
Please advise me.

jdk/src/java.base/share/classes/jdk/internal/module/jdk8_packages.dat:
  1288  sun.security.tools.jarsigner
  1289  sun.security.tools.keytool
  1290: sun.security.tools.policytool
  1291  sun.security.util
  1292  sun.security.validator

langtools/src/jdk.jdeps/share/classes/com/sun/tools/jdeps/resources/jdk8_internals.txt:
   977  sun.security.tools.jarsigner
   978  sun.security.tools.keytool
   979: sun.security.tools.policytool
   980  sun.security.util
   981  sun.security.validator

2. A swing test containing a full JDK 1.4.2 README text. Keep unchanged.

jdk/test/javax/swing/JTextArea/4697612/bug4697612.txt:
   122bin/ktab and jre/bin/ktab
   123  Kerberos key table manager
   124:   bin/policytool and jre/bin/policytool
   125  Policy File Creation and Management Tool
   126bin/orbd and jre/bin/orbd

3. A manual test on what resource string are used. It is based on the 
pre-module source layout and needs to be updated anyway. Keep unchanged this 
time. https://bugs.openjdk.java.net/browse/JDK-8187265 filed.

jdk/test/sun/security/util/Resources/NewResourcesNames.java:
62  "sun/security/tools/jarsigner/Resources.java",
63  "sun/security/tools/keytool/Resources.java",
64: "sun/security/tools/policytool/Resources.java",
65  "sun/security/util/Resources.java",
66  "sun/security/util/AuthResources.java",
..
   103  //
   104  // which is mismatch. There are only two such special cases 
list above.
   105: // For KeyTool, there are 3 calls for showing help. For 
PolicyTool, 3
   106  // for name prefixed with POLICY. They are covered in the two 
special
   107  // cases above.

There are some Japanese man pages containing the word. I've filed another bug 
(https://bugs.openjdk.java.net/browse/JDK-8187262) on it.

Thanks
Max

Re: RFR: JDK-8178278 Move Standard Algorithm Names document to specs directory

[Erik Joelsson]

Nah, it's fine.

/Erik


On 2017-05-09 04:37, Magnus Ihse Bursie wrote:


On 2017-05-05 17:52, Erik Joelsson wrote:
What's the reason for adding the conditional around SetupCopyFiles in 
Javadoc.gmk? The SetupCopyFiles macro should be safe to call with an 
empty FILES list. It should evaluate to basically nothing in that case.


I don't think there's an explicit reason. The code evolved that way. 
The previous version had a check for $(SPECS_$m), since SetupCopyFiles 
does *not* cope with an empty SRC. (Trust me on this ;-))


However, the new foreach will take care of that, so $d will always be 
specified. The current pattern also mimicks the call to 
SetupProcessMarkdown further down. (Which, I believe, would not handle 
an empty value on FILES. Which it perhaps should.)


If nothing else, it would probably save us a few clock cycles. :-)

Do you want me to remove it?

/Magnus



/Erik


On 2017-05-05 06:17, Magnus Ihse Bursie wrote:
The Security Standard Names document will be moved to a new 
location, so all links needs to be updated. Also, a minor fix to the 
build system was needed.


Bug: https://bugs.openjdk.java.net/browse/JDK-8178278
WebRev: 
http://cr.openjdk.java.net/~ihse/JDK-8178278-standard-names-spec-as-markdown/webrev.01


/Magnus

Re: RFR: JDK-8178278 Move Standard Algorithm Names document to specs directory

[Erik Joelsson]

What's the reason for adding the conditional around SetupCopyFiles in 
Javadoc.gmk? The SetupCopyFiles macro should be safe to call with an 
empty FILES list. It should evaluate to basically nothing in that case.


/Erik


On 2017-05-05 06:17, Magnus Ihse Bursie wrote:
The Security Standard Names document will be moved to a new location, 
so all links needs to be updated. Also, a minor fix to the build 
system was needed.


Bug: https://bugs.openjdk.java.net/browse/JDK-8178278
WebRev: 
http://cr.openjdk.java.net/~ihse/JDK-8178278-standard-names-spec-as-markdown/webrev.01


/Magnus

Re: RFR: 8170157/8169335: Unlimited Cryptography Policy Changes

[Erik Joelsson]

Looks good.

/Erik


On 2016-12-03 00:50, Bradford Wetmore wrote:


Hi,

I need reviewers for these related bugs:

https://bugs.openjdk.java.net/browse/JDK-8170157
Enable unlimited cryptographic policy by default in OracleJDK

https://bugs.openjdk.java.net/browse/JDK-8169335
Add a crypto policy fallback in case Security Property
 'crypto.policy' does not exist

http://cr.openjdk.java.net/~wetmore/8170157

This change enables "unlimited" cryptographic policy by default in the 
OpenJDK/OracleJDK builds.


For those who still require the classic limited policy bundles, you 
will now need to use the following "configure" option:


--enable-unlimited-crypto

There are still distributions that require crypto policy, so simply 
removing that code is not an option, sorry!


Brad

Re: RFR: 8157561 :Ship the unlimited policy files in JDK Updates

[Erik Joelsson]

Build changes look ok to me.

/Erik


On 2016-11-04 14:42, Seán Coffey wrote:
Looking to push this enhancement to jdk8u. The change introduces the 
new Security property which was brought into JDK 9 via JDK-8061842.


The code differs in that jar files continue to be used and backwards 
compatibility is maintained. If the new security property is not 
defined and the policy jar files exist in the legacy locations, then 
those jar files will continue to be honoured.


https://bugs.openjdk.java.net/browse/JDK-8157561
webrev : 
http://cr.openjdk.java.net/~coffeys/webrev.8157561.8u.jdk.v4/webrev/

Re: RFR 8074838: Resolve disabled warnings for libj2pkcs11

[Erik Joelsson]

Looks ok to me.

/Erik

On 2016-08-25 23:20, Anthony Scarpino wrote:

Hi,

Can I get a review of this change to remove the warning suppression 
and fix the minor compiler issues that it was hiding in the pkcs11 
wrapper library.


http://cr.openjdk.java.net/~ascarpino/8074838/webrev/

thanks

Tony

Re: RFR: 8164071: Default.policy file missing content for solaris

[Erik Joelsson]

On 2016-08-17 18:43, Sean Mullan wrote:


On 8/17/2016 12:33 PM, Erik Joelsson wrote:

Hello Sean,

The change looks ok, but it could also be expressed like this to 
avoid duplication:


ifneq ($(filter $(OPENJDK_TARGET_OS), windows solaris), )
  DEF_POLICY_SRC_LIST += 
$(JDK_TOPDIR)/src/java.base/$(OPENJDK_TARGET_OS)/lib/security/default.policy

endif

Thanks. However, shouldn't that be "ifeq" and not "ifneq"?

Actually no, if OPENJDK_TARGET_OS is either windows or solaris, the 
filter function will return exactly that, otherwise it will return 
empty. The if(n)eq compares the result of filter to the empty string. 
This construct is a bit convoluted but is our standard construct for 
this situation.


/Erik

--Sean



/Erik

On 2016-08-17 18:18, Sean Mullan wrote:
Please review this simple fix to append the solaris default.policy 
file to the overall default.policy file.


Bug: https://bugs.openjdk.java.net/browse/JDK-8164071

diff -r 551f7617b2c0 make/copy/Copy-java.base.gmk
--- a/make/copy/Copy-java.base.gmkWed Aug 17 10:08:18 2016 +0800
+++ b/make/copy/Copy-java.base.gmkWed Aug 17 12:17:19 2016 -0400
@@ -185,6 +185,8 @@

 ifeq ($(OPENJDK_TARGET_OS), windows)
   DEF_POLICY_SRC_LIST += 
$(JDK_TOPDIR)/src/java.base/$(OPENJDK_TARGET_OS)/lib/security/default.policy

+else ifeq ($(OPENJDK_TARGET_OS), solaris)
+  DEF_POLICY_SRC_LIST += 
$(JDK_TOPDIR)/src/java.base/$(OPENJDK_TARGET_OS)/lib/security/default.policy 


 endif

 # Allow imported modules to modify the java.policy


Thanks,
Sean

Re: RFR: 8164071: Default.policy file missing content for solaris

[Erik Joelsson]

Hello Sean,

The change looks ok, but it could also be expressed like this to avoid 
duplication:


ifneq ($(filter $(OPENJDK_TARGET_OS), windows solaris), )
  DEF_POLICY_SRC_LIST += 
$(JDK_TOPDIR)/src/java.base/$(OPENJDK_TARGET_OS)/lib/security/default.policy
endif


/Erik

On 2016-08-17 18:18, Sean Mullan wrote:
Please review this simple fix to append the solaris default.policy 
file to the overall default.policy file.


Bug: https://bugs.openjdk.java.net/browse/JDK-8164071

diff -r 551f7617b2c0 make/copy/Copy-java.base.gmk
--- a/make/copy/Copy-java.base.gmkWed Aug 17 10:08:18 2016 +0800
+++ b/make/copy/Copy-java.base.gmkWed Aug 17 12:17:19 2016 -0400
@@ -185,6 +185,8 @@

 ifeq ($(OPENJDK_TARGET_OS), windows)
   DEF_POLICY_SRC_LIST += 
$(JDK_TOPDIR)/src/java.base/$(OPENJDK_TARGET_OS)/lib/security/default.policy

+else ifeq ($(OPENJDK_TARGET_OS), solaris)
+  DEF_POLICY_SRC_LIST += 
$(JDK_TOPDIR)/src/java.base/$(OPENJDK_TARGET_OS)/lib/security/default.policy 


 endif

 # Allow imported modules to modify the java.policy


Thanks,
Sean

Re: RFR 8162739: Create new keytool option to access cacerts file

[Erik Joelsson]

Build change looks fine.

/Erik


On 2016-08-08 15:46, Weijun Wang wrote:

Please review the code changes at

   http://cr.openjdk.java.net/~weijun/8162739/webrev.00/

A new -cacerts option is added to keytool so there is no need to 
provide the full cacerts path on the command line. 
lib/security/cacerts is considered an implementation detail and 
subject to change.


The conf/security/cacerts file in src/ is moved to 
lib/security/cacerts, which matches its destination path. A small 
change in make/ is needed.


Thanks
Max

Re: RFR 8140422: Add mechanism to allow non default root CAs to be not subject to algorithm restrictions

[Erik Joelsson]

Much better, and thank you for fixing the existing mkdir/echo lines too. 
Just one nit, for this continuation:


$(TOOL_CACERTSHASHER) -i $(GENDATA_CACERTSHASHER_IN) \
 -o $(GENDATA_CACERTSHASHER)

please use tab+4spaces for the second line. No need to resend webrev for 
that. See [1] for our build system code conventions.


[1] http://openjdk.java.net/groups/build/doc/code-conventions.html

/Erik

On 2016-03-18 18:09, Anthony Scarpino wrote:

I believe I got everyone's comments. I've updated the webrev.

http://cr.openjdk.java.net/~ascarpino/8140422/webrev.02/

Thanks

Tony


On 02/29/2016 08:55 AM, Anthony Scarpino wrote:

Currently CertPath algorithm restrictions allow or deny all
certificates.  This change adds the ability to reject certificate chains
that contain a restricted algorithm and the chain terminates at a root
CA; therefore, allowing a self-signed or chain that does not terminate
at a root CA.

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

Thanks

Tony

Re: [9] RFR 8086002: Move apple.security.AppleProvider to a proper module

[Erik Joelsson]

One is enough.

/Erik

On 2015-08-18 02:21, Valerie Peng wrote:

Thanks for the review.
Is one more reviewer from build team needed?
Valerie

On 8/14/2015 4:58 PM, Mandy Chung wrote:

Looks good.
Mandy

On Aug 14, 2015, at 4:30 PM, Valerie Pengvalerie.p...@oracle.com  
wrote:



Updated the webrev in place to use osxsecurity given peer feedbacks.
Thanks,
Valerie

On 8/13/2015 7:31 PM, Valerie Peng wrote:

Can someone please help reviewing this change?
This is to move Apple provider from jdk.deploy.osx module to 
java.base module.
The native library for Apple provider is separated out from the 
osx one generated in jdk.deploy.osx module and named osxapple 
(sort of following the convention of SunMSCAPI provider whose 
native library is named sunmscapi).


webrev: http://cr.openjdk.java.net/~valeriep/8086002/webrev.00/

Thanks,
Valerie

Re: [9] RFR 8086002: Move apple.security.AppleProvider to a proper module

[Erik Joelsson]

Build changes still look good.

/Erik

On 2015-08-15 01:30, Valerie Peng wrote:


Updated the webrev in place to use osxsecurity given peer feedbacks.
Thanks,
Valerie

On 8/13/2015 7:31 PM, Valerie Peng wrote:


Can someone please help reviewing this change?
This is to move Apple provider from jdk.deploy.osx module to 
java.base module.
The native library for Apple provider is separated out from the osx 
one generated in jdk.deploy.osx module and named osxapple (sort of 
following the convention of SunMSCAPI provider whose native library 
is named sunmscapi).


webrev: http://cr.openjdk.java.net/~valeriep/8086002/webrev.00/

Thanks,
Valerie

Re: [9] RFR 8086002: Move apple.security.AppleProvider to a proper module

[Erik Joelsson]

Build changes look good.

/Erik

On 2015-08-14 04:31, Valerie Peng wrote:


Can someone please help reviewing this change?
This is to move Apple provider from jdk.deploy.osx module to java.base 
module.
The native library for Apple provider is separated out from the osx 
one generated in jdk.deploy.osx module and named osxapple (sort of 
following the convention of SunMSCAPI provider whose native library is 
named sunmscapi).


webrev: http://cr.openjdk.java.net/~valeriep/8086002/webrev.00/

Thanks,
Valerie

Re: [9] RFR 8130665 java/lang/SecurityManager/CheckSecurityProvider.java failing in jake on OS X

[Erik Joelsson]

Hello Valerie,

To reduce duplication, I think something like this would be preferable. 
Remove the windows and macosx checks and just always run this line:


POLICY_SRC_LIST += \
$(wildcard 
$(JDK_TOPDIR)/src/java.base/$(OPENJDK_TARGET_OS)/conf/security/java.policy)


That way, I platform specific file is always picked up if found.

/Erik

On 2015-08-08 01:25, Valerie Peng wrote:

Hi, Sean and build experts,

Can you please review the fix for 8130665 
java/lang/SecurityManager/CheckSecurityProvider.java failing in jake 
on OS X?
The Apple provider fails to be instantiated in Jake due to the missing 
permission grants.
However, it currently only fails in Jake workspace and fix is verified 
by running JPRT against Jake.


Given that Apple provider only presents for Mac OSX, its policy file 
is under the macosx directory and I have to make minor modifications 
to pick up that file for MacOSX build.


Here is the webrev:
http://cr.openjdk.java.net/~valeriep/8130665/webrev.00/

Thanks,
Valerie

Re: RFR 7191662: JCE providers should be located via ServiceLoader

[Erik Joelsson]

Hello Valerie,

The merging seems ok, but I thought there was non determinism in the 
image builder regarding which provider would get picked up. Is that 
resolved or do you really need to override all of those providers with 
your generated file in gensrc? I can assist in writing that makefile 
logic if needed.


/Erik

On 2015-06-04 23:58, Valerie Peng wrote:

Build experts,

Can you please review the make file related change, i.e. the new file 
make/gensrc/Gensrc-java.naming.gmk, in the following webrev: 
http://cr.openjdk.java.net/~valeriep/7191662/webrev.03


This is for merging the java.security.Provider file from various 
providers and use the (merged) result for the final image build.


Thanks,
Valerie

On 6/3/2015 10:27 AM, Valerie (Yu-Ching) Peng wrote:


Correct, if the makefile related changes are removed then no need for 
build team to review 7191662 webrev anymore.
There are other discussions ongoing and we should be able to reach a 
decision in a day or two.

Will update the list again.
Thanks,
Valerie

On 06/01/15 16:39, Magnus Ihse Bursie wrote:

On 2015-05-29 00:10, Valerie Peng wrote:


Please find comments in line...

On 5/27/2015 3:42 PM, Mandy Chung wrote:

Valerie,

Did you see my comment yesterday?
http://mail.openjdk.java.net/pipermail/security-dev/2015-May/012254.html 

Yes, we exchanged emails after this above one. I will follow up 
your latest one later today.




Since you have reverted the java.security to keep the classname, 
to avoid causing merge conflict to jimage refresh, let’s remove 
the META-INF files in the first push and the build change.


The security providers will be loaded via the fallback mechanism 
(i.e. ProviderLoader.legacyLoad method).  You should keep the 
ProviderLoader.load method to take the provider name instead of 
classname.
Sure, I can remove the META-INF files so the providers are loaded 
through the legacyLoad().
Hmm, the ProviderLoader.load() method is used by java.security file 
provider loading. Since the current list still uses class name, it 
should take class name when checking for matches while iterating 
through the list returned by ServiceLoader.
This way, when changes are sync'ed into Jake, no extra change 
required and the providers will be loaded through 
ProviderLoader.load() automatically with the current list.


I’ll file a bug to follow up to change java.security to list the 
provider name.  This will wait after the jimage refresh goes into 
jdk9/dev

Ok.
Thanks,
Valerie


I'm not sure I followed completely here were this landed. Does this 
mean that there's currently no need for a build system code review 
on http://cr.openjdk.java.net/~valeriep/7191662/webrev.01/, and that 
a new webrev will be posted instead?


/Magnus




.

Mandy

On May 27, 2015, at 3:29 PM, Valerie 
Pengvalerie.p...@oracle.com  wrote:


Hi, build experts,

Can you please review the make file related change, i.e. the new 
file make/gensrc/Gensrc-java.naming.gmk, in the following webrev:

http://cr.openjdk.java.net/~valeriep/7191662/webrev.01/

This is for merging the java.security.Provider file from various 
providers and use the (merged) result for the final image build.


The rest of source code changes are reviewed by my team already.
Thanks,
Valerie
(Java Security Team)

Re: RFR 7191662: JCE providers should be located via ServiceLoader,

[Erik Joelsson]

On 2015-05-22 18:53, Mandy Chung wrote:



On 05/22/2015 08:09 AM, Alan Bateman wrote:


On 22/05/2015 13:55, Chris Hegarty wrote:

:
I think it could be done either way.


Valerie - have you considered not pushing the services configuration 
files with this change? With the change then the java.security 
configuration is still class names, not provider names, so the 
fallback should just work. This is what we've done in a few other 
areas (like JNDI for example).


I wasn't aware of the other areas that move to service provider but 
remain being loaded with the fallback Class.forName.


I would prefer java.security should convert to use the provider names 
as an example and also exercise the code path using service 
providers.  If this causes much work to workaround it temporarily, I 
won't object the security providers are not truly service providers 
(no META-INF/services and java.security lists class name instead)


Another option to workaround this:

we only need to merge the service config files for generating the 
image.  Can we have do the concatenation of 
jdk/modules/*/META-INF/services file and output to 
supports/image_gensrc before the images target and have the image 
builder to exclude all jdk/modules/*/META-INF/services files and take 
the supports/image_gensrc instead?


This will remove the process-provider logic from Gensrc-*.gmk files.  
Would this be a better alternative?


Maybe, I'm not sure. I still think solving this in java code in the 
ImageBuilder is the right thing to do.


/Erik

Re: RFR 7191662: JCE providers should be located via ServiceLoader,

[Erik Joelsson]

On 2015-05-22 02:46, Mandy Chung wrote:

I’m including build-dev and we need to ask for Erik and Magnus advice what’s 
the best way to work around this.

Erik, Magnus,
   Security providers now become service providers.   They are provided from 11 
different modules, 3 of them are os-specific.  The current image builder 
doesn’t merge duplicate resources and we currently work around it in the build 
doing the merge as it’s temporary until the module system is moving further.  
Gensrc-jdk.jdi.gmk is the example.

Valerie has a patch attempting to concatenate them and it turns out that it 
might not be straight forward I have thought.

http://cr.openjdk.java.net/~valeriep/7191662/webrev.00/make/gensrc/Gensrc-java.naming.gmk.html

As we can’t say which one image builder will pick, it needs to copy gensrc 
(merged version) to all 
modules/$MODULE/META-INF/services/java.security.Provider config files.

I wonder if it’s quicker to hack ImageBuilder to special case the service 
config file and merge them.

Any thought?
I could certainly make this work in the makefile, but it is getting a 
bit tedious to keep these hacks going there. How hard would it be to 
change the ImageBuilder, which is just a temporary solution anyway, to 
identify and merge all service provider files with the same name that it 
finds? With that solution, the makefiles do not need to change as much 
when the module system is introduced and taking care of this properly.


/Erik



On May 21, 2015, at 3:03 PM, Valerie Peng valerie.p...@oracle.com wrote:

Mandy,

Please find comments in line.

On 5/20/2015 10:39 PM, Mandy Chung wrote:

A quick comment on the META-INF/services config files and the makefile. Merging 
the service config files is temporary until the module system is moving further 
along.

src/jdk.crypto.ucrypto/solaris/classes/META-INF/services/java.security.Provider.html
and other os-specific service configuration:
   1 #[solaris]com.oracle.security.ucrypto.UcryptoProvider

- why is this commented out?  Does the makefile uncomment it?  It should be 
simple
concatenation with

In an example that I found through another makefile, it would uncomment the entry start 
with #[OS] (and process/remove this prefix) when the OS matches. We need 
OS-specific file processing when concatenate these files.


Ah… that’s from Gensrc-jdk.jdi.gmk.  The service config file contains a mixture 
of cross-neutral and os-specific providers.  That’s not the example to copy.

define process-provider
 $(MKDIR) -p $(@D)
 $(CAT) $^  $@
endef



What decides if it's appropriate or not? These are not just crypto providers 
that we are defining here, but all classes which extend from 
java.security.Provider. I recall using jdk.crypto.ec as the gensrc module as 
you suggested initially. But when testing it, it doesn't seem to work as 
expected. I ended up using java.naming as that's the one ended up in the final 
image instead of the concatenated one under jdk.crypto.ec. Could there be some 
alphabetic ordering when processing/building these modules?

Well, since this is really a hack and only temporary, does it really matter 
whether it's under java.naming or jdk.crypto.ec? Both contains providers for 
the java.security provider list. The key thing is that the resulting image works

Mandy

Re: RFR: JDK-8074859 Turn on warnings as error

[Erik Joelsson]

Looks good to me.

/Erik

On 2015-04-17 14:52, Magnus Ihse Bursie wrote:
With JDK-8074096, the number of warnings in the product was reduced to 
a minimum. This enables the next step, which is turning on the 
respective compiler flags that turns warnings into errors. In the long 
run, this is the only way to keep the warnings from creeping back.


Even with JDK-8074096, the product does not build 100% warning free. 
This is due to some warnings that cannot be disabled, or (in one case) 
where C and C++ code is mixed, and warnings for both languages cannot 
be used. A system similar to the one introduced in JDK-8074096 is 
therefore needed, in which individual libraries can be exempted from 
this flag, until such warnings are fixed. A library can thus disable 
warnings as errors with WARNINGS_AS_ERRORS := false, or (better) use a 
toolchain-specific version, e.g WARNINGS_AS_ERRORS_gcc := false. This 
is intended as a temporary measure, though. The long-term solution is 
reasonably to fix the warnings and remove that argument.


Also, different versions of compilers can generate a different set of 
warnings. It is therefore necessary to be able to turn off this 
globally. Therefor a new flag for configure is introduced: 
--disable-warnings-as-errors.


While the code compiles without errors on the build systems used 
internally at Oracle, this might not be the case on other combinations 
of operating system versions and toolchain versions. To facilitate for 
unexpecting developers, a help message is added if the build fails, 
that suggests using --disable-warnings-as-errors. This solution was 
chosen as a compromise between the hard core solution of turning on 
warnings as errors by default for anyone, and the cowar... erm, 
conservative solution of checking if the compiler versions exactly 
match what's used inside Oracle (and therefore regularly tested), and 
only turn it on in that case.


Similarly to JDK-8074096, I intend to file follow-up bugs for each 
individual library that got a WARNINGS_AS_ERRORS_* := false.


Bug: https://bugs.openjdk.java.net/browse/JDK-8074859
WebRev for top: 
http://cr.openjdk.java.net/~ihse/JDK-8074859-warnings-as-errors-top/webrev.01
WebRev for jdk: 
http://cr.openjdk.java.net/~ihse/JDK-8074859-warnings-as-errors-jdk/webrev.01


Some comments:

* I needed to add a few more DISABLED_WARNINGS. For windows, this is 
most likely due to the recent compiler change. For other libraries, 
I'm not sure, but it might well be the result of recent changes that 
has introduced new warnings. If so, it highlights the need of this 
patch to keep the build warning free.


* For a few libraries and toolchains, there is *both* 
WARNINGS_AS_ERRORS := false and a DISABLED_WARNINGS list. This is the 
case if not all warnings are possible to disable.


* I have removed some incorrect uses of SHARED_LIBRARY_FLAGS. This is 
included in our JDK LDFLAGS, so it should not be set separately, and 
definitely not as CFLAGS. (This caused compiler warnings, which now 
turned into errors.) However, a more suitable long-term solution is 
probably to move the knowledge of how to create shared libraries more 
specifically into SetupNativeCompilation, and not set it as part of 
the JDK flags.


/Magnus

Re: RFR 8074835/8074836: Resolve disabled warnings for libj2gss/libosxkrb5

[Erik Joelsson]

Looks good to me. Thanks for fixing warnings!

/Erik

On 2015-03-16 04:12, Wang Weijun wrote:

Hi All

Please review the change at

http://cr.openjdk.java.net/~weijun/8074836/webrev.00/

Thanks
Max

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

[Erik Joelsson]

Thanks, looks good!

/Erik

On 2015-03-06 17:14, Magnus Ihse Bursie wrote:

On 2015-03-04 14:31, Erik Joelsson wrote:

Hello,

Really nice to finally see this patch getting done!

Only one comment:

flags.m4:
In the grep expression, could you move the extra [] outside of the 
actual command line options to grep so that the command line could be 
copied to the shell for debugging in the future? Also, how hard would 
it be to instead do AC_COMPILE_IFELSE with a dummy warning to instead 
test for capability?


I have updated the fix to use the eminent 
FLAGS_COMPILER_CHECK_ARGUMENTS instead. :-)


http://cr.openjdk.java.net/~ihse/JDK-8074096-disable-native-warnings/webrev.02 



/Magnus

Re: Review Request: 8074428, 8074429, 8074430 jdk.pack200, jdk.jartool, jdk.policytool modules

[Erik Joelsson]

Hello Mandy,

The build changes look ok to me.

/Erik

On 2015-03-05 02:13, Mandy Chung wrote:

As listed in an open issue in JEP 200:

The jdk.dev and jdk.runtime modules contain miscellaneous tools that do
not obviously belong to any other module; these modules will eventually
be either renamed or refactored.

Currently there are jdk.javadoc, jdk.jconsole, jdk.jcmd modules in
the JDK that are organized around its primary tool.  Such organization
is easy to name, document and understand. This patch proposes to
move tools from jdk.runtime and jdk.dev to jdk.pack200, jdk.jartool,
jdk.policytool modules.

Overall Webrev that will be convenient to review the build change
and modules.xml change.
http://cr.openjdk.java.net/~mchung/jdk9/webrevs/8074428%2b8074429%2b8074430/webrev.00/

Separate webrevs for each issue:
1. pack200, unpack200 to jdk.pack200

http://cr.openjdk.java.net/~mchung/jdk9/webrevs/8074428%2b8074429%2b8074430/8074428/webrev.00/ 



2. jar, jarsigner to jdk.jartool

http://cr.openjdk.java.net/~mchung/jdk9/webrevs/8074428%2b8074429%2b8074430/8074429/webrev.00/ 



3. policytool to jdk.policytool

http://cr.openjdk.java.net/~mchung/jdk9/webrevs/8074428%2b8074429%2b8074430/8074430/webrev.00/ 



There are remaining tools in jdk.dev that will be handled separately.
jdk.dev will disappear when all of the remaining tools find its home.

Mandy

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

[Erik Joelsson]

Hello,

Really nice to finally see this patch getting done!

Only one comment:

flags.m4:
In the grep expression, could you move the extra [] outside of the 
actual command line options to grep so that the command line could be 
copied to the shell for debugging in the future? Also, how hard would it 
be to instead do AC_COMPILE_IFELSE with a dummy warning to instead test 
for capability?


/Erik

On 2015-03-04 14:17, Magnus Ihse Bursie wrote:
When building the native code in the jdk repo, a lot of warnings are 
generated. So many that it's hard to spot any new issues.


While the ultimate goal must be to address and fix these warnings 
individually, this bug is about disabling these warnings where they 
occur, so that it is easier to spot new warnings, and that the 
existing warnings can be addressed one at a time.


Disabling warnings instead of fixing them can be problematic. If you 
are too broad with disabling warnings, you might hide future problems. 
On the other hand, there have been a lot of warnings that indicate 
serious problems that has been hidden in plain sight for a very long 
time in the code base anyway.


I have opted to disable warnings at the library level. Disabling 
warnings globally would be too broad. Disabling per file would have 
been too tedious. Many warnings also tend to repeat across multiple 
files in the same library, due to e.g. less well-suited programming 
style or design choice. A library also seems like a suitable chunk of 
work to address later on, in trying to get rid of the source of the 
warnings.


This fix will not get rid of all warnings, but the bulk of them. It 
will make the remaining warnings stick out more. The few warnings that 
remain are either:

* Not possible to disable.
* Not resulting from native compilation (but from linking, or javadoc, 
etc).
* Not possible to disable with this framework (this goes for builds on 
pre-4.4 gcc, which Oracle currently does as default on macosx), and 
libfontmanager on Solaris, which mixes C and C++ code. (While the 
solstudio C compiler does not fail on requests to disable C++ 
warnings, the converse is unfortunately not true). It did not seem 
worth the trouble to build a more extensible framework to handle this, 
compared to actually fixing these warnings.


Note that the current JPRT build environment in Oracle uses a pre-4.4 
gcc for macosx by default, so the default macosx build will still 
contain warnings. When building with --with-toolchain-type=clang, the 
clang warning disabling kicks in. (This will be the default in JPRT 
later on this year.)


I intend to file individual bugs to handle these remaining warnings, 
so the net result will be a completely warning free build.


I also intend to file individual bugs on all the libraries that has 
had warnings disabled. I expect the outcome of these bugs to be either:


A) The code modified so it does not trigger warnings, and the warnings 
re-enabled, or


B) The warnings (or a subset of them) kept disabled, but a comment 
added to the makefile describing why this is the proper course of action.


Not all bugs might be worth fixing. For instance, the GCC warnings 
type-limits, sign-compare, pointer-to-int-cast, conversion-null, 
deprecated-declarations, clobbered, int-to-pointer-cast and 
type-limits are all more or less benign, and is possibly the result of 
a false positive.


On the other hands, warnings such as format-nonliteral, unused-result, 
maybe-uninitialized, format, format-security, int-to-pointer-cast, 
reorder and delete-non-virtual-dtor are more likely to actually point 
to real issues. I recommend anyone finding these warnings on a library 
they care about to try and fix them as soon as possible.


Here is a summary of the libraries and binaries that have gotten 
warnings disabled. I'm not sure about what group owns all these 
libraries; if I missed sending this mail to the correct list, please 
help me and forward it.


* libunpack
* libfdlibm
* libverify
* libjava
* libzip
* libjli/libjli_static
* libj2gss
* libsunec
* libj2pkcs11
* libnet
* libnio
* libosxkrb5
* libosxapp
* libosx
* libapplescriptengine
* libjsound
* libjsoundalsa
* libmlib_image
* libawt
* libawt_xawt
* libawt_lwawt
* liblcms
* libjavajpeg
* libawt_headless
* libfontmanager
* libsplashscreen
* unpack200
* The JVMTI demos compiledMethodLoad and waiters

Bug: https://bugs.openjdk.java.net/browse/JDK-8074096

WebRev: 
http://cr.openjdk.java.net/~ihse/JDK-8074096-disable-native-warnings/webrev.01


/Magnus

RFR: JDK-8068748: missing US_export_policy.jar in jdk9-b44 is causing compilation errors building jdk9 source code

[Erik Joelsson]

Hello,

Please review the open part of this patch, which changes the building of 
policy jars to happen even if BUILD_CRYPTO is false. Previously these 
weren't built as there were signed versions of these jars, but since we 
no longer sign them, there is no need to not build them.


Bug: https://bugs.openjdk.java.net/browse/JDK-8068748
Webrev: http://cr.openjdk.java.net/~erikj/8068748/webrev.jdk.01/

The webrev diffs look a bit weird. The only thing I did was to remove 
the ifneq ($(BUILD_CRYPTO), no) and adjust the indentation.


/Erik

Re: RFR 8039898: sunpkcs11-solaris.cfg should be in solaris specific directory

[Erik Joelsson]

Change looks good to me.

/Erik

On 2014-09-10 18:28, Valerie Peng wrote:


Could someone please review this build related change for moving 
sunpkcs11-solaris.cfg file to the pkcs11 module?


Webrev: http://cr.openjdk.java.net/~valeriep/8039898/webrev.00/

Thanks,
Valerie

Re: RFR 6997010: Consolidate java.security files into one file with modifications

[Erik Joelsson]

On 2014-08-05 23:24, Sean Mullan wrote:

On 07/28/2014 09:53 AM, Wang Weijun wrote:

Yes, you are right.

Webrev updated at 
http://cr.openjdk.java.net/~weijun/6997010/webrev.02. 
GendataJavaSecurity.gmk and MakeJavaSecurity.java updated.


There's an unnecessary indent at line 1 of GendataJavaSecurity.gmk

Speaking of indentation, please also change the ifdef bodies to 2 spaces.

/Erik


In CheckSecurityProvider.java, ucrypto is in the closed sources, so 
Security.getProviders() will not return it if you are testing an 
OpenJDK build. You need to adjust the test to not include this 
provider if testing an OpenJDK build.


--Sean



Thanks
Max

On Jul 28, 2014, at 19:43, Erik Joelsson erik.joels...@oracle.com 
wrote:



Hello Max,

Shouldn't the rule for $(GENDATA_JAVA_SECURITY) depend on 
$(RESTRICTED_PKGS_SRC) so that updates to the pkgs file triggers a 
rebuild? For that to work, the variable $(RESTRICTED_PKGS_SRC) needs 
to be empty for the OPENJDK case rather than have a dummy name and 
MakeJavaSecurity.java needs to handle missing the last argument.


/Erik

On 2014-07-28 03:44, Wang Weijun wrote:

Webrev updated at


http://cr.openjdk.java.net/~weijun/6997010/webrev.01/


New test CheckSecurityProvider.java, and updates to 
MakeJavaSecurity.addPackages().


Thanks
Max

On Jul 25, 2014, at 22:44, Wang Weijun
weijun.w...@oracle.com
  wrote:



On Jul 25, 2014, at 22:30, Sean Mullan sean.mul...@oracle.com
  wrote:



http://cr.openjdk.java.net/~weijun/6997010/webrev.00/


4. *IMPORTANT*: In order to easily maintain platform-related 
entries,

every line (including the last line) in package.access and
package.definition MUST end with ',\' now. A blank line MUST exist
after the last line. This avoid ugly lines like

#ifndef windows entry1. #endif #ifdef windows entry1.,\ entry2
#endif

What happens if someone (inevitably) adds a new package to the 
list and forgets to do either of these? Does it result in a build 
failure?


No build failure, but 
test/java/security/SecurityManager/CheckPackageAccess.java will fail.


I can add check in the build tool.


Otherwise looks good, although I think it would be useful to 
write an additional test to make sure the correct providers are 
installed and ordered correctly on the different platforms, 
something similar to the 
java/lang/SecurityManager/CheckPackageAccess.java test but 
specific to providers.



OK.

Thanks
Max

Re: RFR 6997010: Consolidate java.security files into one file with modifications

[Erik Joelsson]

On 2014-08-06 11:14, Weijun Wang wrote:

On 8/6/2014 17:04, Erik Joelsson wrote:
Speaking of indentation, please also change the ifdef bodies to 2 
spaces.


Sure. Whenever I edit a Makefile, I dare not use spaces and always use 
TABs. Obviously an ifdef does not need it.


The tab character unfortunately has significance in make, so we 
carefully differentiate between using space and tab when it's 
appropriate. A tab in the wrong place can also cause trouble.


/Erik

Thanks
Max

Re: RFR 6997010: Consolidate java.security files into one file with modifications

[Erik Joelsson]

Hello Max,

Shouldn't the rule for $(GENDATA_JAVA_SECURITY) depend on 
$(RESTRICTED_PKGS_SRC) so that updates to the pkgs file triggers a 
rebuild? For that to work, the variable $(RESTRICTED_PKGS_SRC) needs to 
be empty for the OPENJDK case rather than have a dummy name and 
MakeJavaSecurity.java needs to handle missing the last argument.


/Erik

On 2014-07-28 03:44, Wang Weijun wrote:

Webrev updated at

http://cr.openjdk.java.net/~weijun/6997010/webrev.01/

New test CheckSecurityProvider.java, and updates to 
MakeJavaSecurity.addPackages().

Thanks
Max

On Jul 25, 2014, at 22:44, Wang Weijun weijun.w...@oracle.com wrote:


On Jul 25, 2014, at 22:30, Sean Mullan sean.mul...@oracle.com wrote:


http://cr.openjdk.java.net/~weijun/6997010/webrev.00/

4. *IMPORTANT*: In order to easily maintain platform-related entries,
every line (including the last line) in package.access and
package.definition MUST end with ',\' now. A blank line MUST exist
after the last line. This avoid ugly lines like

#ifndef windows entry1. #endif #ifdef windows entry1.,\ entry2
#endif

What happens if someone (inevitably) adds a new package to the list and forgets 
to do either of these? Does it result in a build failure?

No build failure, but 
test/java/security/SecurityManager/CheckPackageAccess.java will fail.

I can add check in the build tool.


Otherwise looks good, although I think it would be useful to write an 
additional test to make sure the correct providers are installed and ordered 
correctly on the different platforms, something similar to the 
java/lang/SecurityManager/CheckPackageAccess.java test but specific to 
providers.

OK.

Thanks
Max

Re: RFR 6997010: Consolidate java.security files into one file with modifications

[Erik Joelsson]

Build change looks good to me now.

/Erik

On 2014-07-28 15:53, Wang Weijun wrote:

Yes, you are right.

Webrev updated at http://cr.openjdk.java.net/~weijun/6997010/webrev.02. 
GendataJavaSecurity.gmk and MakeJavaSecurity.java updated.

Thanks
Max

On Jul 28, 2014, at 19:43, Erik Joelsson erik.joels...@oracle.com wrote:


Hello Max,

Shouldn't the rule for $(GENDATA_JAVA_SECURITY) depend on 
$(RESTRICTED_PKGS_SRC) so that updates to the pkgs file triggers a rebuild? For 
that to work, the variable $(RESTRICTED_PKGS_SRC) needs to be empty for the 
OPENJDK case rather than have a dummy name and MakeJavaSecurity.java needs to 
handle missing the last argument.

/Erik

On 2014-07-28 03:44, Wang Weijun wrote:

Webrev updated at


http://cr.openjdk.java.net/~weijun/6997010/webrev.01/



New test CheckSecurityProvider.java, and updates to 
MakeJavaSecurity.addPackages().

Thanks
Max

On Jul 25, 2014, at 22:44, Wang Weijun
weijun.w...@oracle.com
  wrote:



On Jul 25, 2014, at 22:30, Sean Mullan sean.mul...@oracle.com
  wrote:



http://cr.openjdk.java.net/~weijun/6997010/webrev.00/


4. *IMPORTANT*: In order to easily maintain platform-related entries,
every line (including the last line) in package.access and
package.definition MUST end with ',\' now. A blank line MUST exist
after the last line. This avoid ugly lines like

#ifndef windows entry1. #endif #ifdef windows entry1.,\ entry2
#endif


What happens if someone (inevitably) adds a new package to the list and forgets 
to do either of these? Does it result in a build failure?


No build failure, but 
test/java/security/SecurityManager/CheckPackageAccess.java will fail.

I can add check in the build tool.



Otherwise looks good, although I think it would be useful to write an 
additional test to make sure the correct providers are installed and ordered 
correctly on the different platforms, something similar to the 
java/lang/SecurityManager/CheckPackageAccess.java test but specific to 
providers.


OK.

Thanks
Max

Re: Review request: 8040059 Change default policy for extensions to no permission

[Erik Joelsson]

Hello Mandy,

The logic looks fine. Just some style issues. I would like indentation 
for the conditionals to be 2 spaces as is currently the standard in the 
makefiles. I would also like to have POLICY_SRC_LIST to be declared 
empty with := instead of just =. We only use = assignment when 
explicitly needed.


/Erik

On 2014-04-25 01:07, Mandy Chung wrote:

Thanks Sean.

I have updated the webrev:
http://cr.openjdk.java.net/~mchung/jdk9/webrevs/8040059/webrev.01/

Erik - I'm including build-dev to review the build change for 
java.policy file.


Thanks
Mandy

On 4/24/14 11:32 AM, Sean Mullan wrote:

On 04/23/2014 05:29 PM, Mandy Chung wrote:


On 4/23/2014 1:10 PM, Sean Mullan wrote:

Just a few comments:

1. When you write a test that uses the jtreg /policy option, the
policy file overrides the system policy file. If the test depends on a
standard extension, then you may get SecurityExceptions unless
additional perms are granted. Thus, there are quite a few tests that
define their own policy files and duplicate the grant statement for
extensions from the system policy:

grant codeBase file:${{java.ext.dirs}}/* {
permission java.security.AllPermission;
}

These tests should be modified to only grant the necessary
permissions. However, ideally I think that a better solution would be
to add a jtreg /policy option that doesn't override the system policy
file, but rather appends to it, for example, using == :



I suspect most of the tests only want to grant the permissions for the
test itself rather than overriding the system policy file. Having a new
jtreg/policy option not to override the system policy file may be a
better solution.This would avoid updating the test's policy file
every time the system's policy is modified.   On the other hand, I 
think

the test policy may not need to grant permissions to the extensions
directory if it doesn't use the classes in extensions.  It's a good
opportunity to clean that up. This will require some closer look at the
tests.

If you are okay, I'd like to separate the test's custom policy 
update as

a follow-on work.


That's fine with me.


@run main/othervm/policy==test.policy

(this is the reverse behavior of the java.security.policy system
property, so it might be a little confusing, so maybe it is better to
add a new option)



== is a confusing syntax. -Djava.security.policy==test.policy
overrides the system policy file (= prefix) whereas jtreg uses the
reverse syntax? I think it would be better to make jtreg/policy
consistent with -Djava.security.policy (i.e. default is to extend the
system java.security).


Would be nice, but not sure if we can change it at this point. 
Anyway, one of us should file a jtreg RFE.



3. jdk/nio/zipfs/ZipFileSystem.java

If I understand the changes, the previous code would throw
SecurityExceptions when run under a SecurityManager? It's not
specifically related to this bug, is it?



It's a bug in the zipfs provider and I can file a new JBS issue for the
zipfs change.  I prefer to push them in the same changeset that reduces
zipfs's privileges and added tests to run with security manager.


Sure, it is fine with me to include them with this. I just wanted to 
make sure I understood the changes.


--Sean

hg: jdk8/tl: 8032632: Wrong version for the first jdk8 fcs build

[erik . joelsson]

Changeset: 7238a870ddb7
Author:erikj
Date:  2014-01-24 10:39 +0100
URL:   http://hg.openjdk.java.net/jdk8/tl/rev/7238a870ddb7

8032632: Wrong version for the first jdk8 fcs build
Reviewed-by: katleman

! common/autoconf/spec.gmk.in

hg: jdk8/tl/jdk: 8032217: failure in man page processing

[erik . joelsson]

Changeset: ff56039c4870
Author:erikj
Date:  2014-01-22 12:13 +0100
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/ff56039c4870

8032217: failure in man page processing
Reviewed-by: dholmes, tbell

! make/Images.gmk

hg: jdk8/tl/jdk: 8031300: No jdeps.1 and jjs.1 man pages in jdk8 b122 build and jvisualvm.1 and jcmd.1 missing on macosx; ...

[erik . joelsson]

Changeset: 6f3a3bd78c57
Author:erikj
Date:  2014-01-10 10:25 +0100
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/6f3a3bd78c57

8031300: No jdeps.1 and jjs.1 man pages in jdk8 b122 build and jvisualvm.1 and 
jcmd.1 missing on macosx
8030946: No jmc.1 for man page of JMC
Reviewed-by: ihse, tbell

! make/Images.gmk
+ src/bsd/doc/man/ja/jcmd.1
+ src/bsd/doc/man/ja/jdeps.1
+ src/bsd/doc/man/ja/jjs.1
- src/bsd/doc/man/ja/kinit.1
- src/bsd/doc/man/ja/klist.1
- src/bsd/doc/man/ja/ktab.1

hg: jdk8/tl/jdk: 8030781: System.setProperties(null) drops all system properties (RELEASE not set)

[erik . joelsson]

Changeset: 2437ccbf3504
Author:erikj
Date:  2014-01-08 14:04 +0100
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/2437ccbf3504

8030781: System.setProperties(null) drops all system properties (RELEASE not 
set)
Reviewed-by: alanb

+ test/java/lang/System/SetPropertiesNull.java

hg: jdk8/tl/jdk: 8027963: Create unlimited policy jars.

[erik . joelsson]

Changeset: 427c78c88229
Author:erikj
Date:  2013-12-05 09:25 +0100
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/427c78c88229

8027963: Create unlimited policy jars.
Reviewed-by: wetmore, ihse

! make/CreateSecurityJars.gmk
! make/SignJars.gmk
- make/data/cryptopolicy/limited/LIMITED
- make/data/cryptopolicy/unlimited/UNLIMITED

hg: jdk8/tl: 8027963: Create unlimited policy jars.

[erik . joelsson]

Changeset: c009462c1e92
Author:erikj
Date:  2013-12-04 12:45 +0100
URL:   http://hg.openjdk.java.net/jdk8/tl/rev/c009462c1e92

8027963: Create unlimited policy jars.
Reviewed-by: wetmore, ihse

! common/autoconf/spec.gmk.in

hg: jdk8/tl: 6604021: RMIC is defaulting to BOOT jdk version, needs to be rmic.jar

[erik . joelsson]

Changeset: af81988013b5
Author:erikj
Date:  2013-10-16 13:50 +0200
URL:   http://hg.openjdk.java.net/jdk8/tl/rev/af81988013b5

6604021: RMIC is defaulting to BOOT jdk version, needs to be rmic.jar
Reviewed-by: dholmes, chegar

! common/makefiles/JavaCompilation.gmk
! common/makefiles/RMICompilation.gmk

hg: jdk8/tl/jdk: 2 new changesets

[erik . joelsson]

Changeset: 76a7c0bc74fd
Author:erikj
Date:  2013-10-16 13:50 +0200
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/76a7c0bc74fd

6604021: RMIC is defaulting to BOOT jdk version, needs to be rmic.jar
Reviewed-by: dholmes, chegar

! makefiles/GendataBreakIterator.gmk
! makefiles/GenerateClasses.gmk
! makefiles/Setup.gmk
! src/share/classes/sun/tools/tree/Node.java

Changeset: e078fd8a78f6
Author:erikj
Date:  2013-10-16 15:53 +0200
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/e078fd8a78f6

Merge

Re: [8] Review Request for 8007292 : Add JavaFX internal packages to package.access

[Erik Joelsson]

On 2013-10-10 19:45, Sean Mullan wrote:

On 10/10/2013 05:57 AM, Erik Joelsson wrote:

Adding makefiles to make/tools is not needed for the new build. Either
remove those changes or make a complete port to the old build. I'm not
pushing for porting this to the old build at this point since missing
this will not cause the old build to stop working, it will just diverge
the resulting bits a bit more.


I have ported the changes to the old build, please review.

New webrev: http://cr.openjdk.java.net/~mullan/webrevs/8007292/webrev.02/

The only modified file is make/java/security/Makefile


Thanks, looks good now!
/Erik

Re: [8] Review Request for 8007292 : Add JavaFX internal packages to package.access

[Erik Joelsson]

Adding makefiles to make/tools is not needed for the new build. Either 
remove those changes or make a complete port to the old build. I'm not 
pushing for porting this to the old build at this point since missing 
this will not cause the old build to stop working, it will just diverge 
the resulting bits a bit more.


/Erik

On 2013-10-09 19:19, Sean Mullan wrote:
Updated webrev: 
http://cr.openjdk.java.net/~mullan/webrevs/8007292/webrev.01/


Let me know if there are any more comments, otherwise I will plan to 
push tomorrow.


Thanks,
Sean

On 10/09/2013 09:20 AM, Sean Mullan wrote:

On 10/09/2013 05:14 AM, Erik Joelsson wrote:

Hello Sean,

On 2013-10-09 06:33, David Holmes wrote:

Hi Sean,

Not a full review.

On 9/10/2013 5:52 AM, Sean Mullan wrote:

Please review the fix for the following bug:

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

This bug requires build changes and a new build tool to add 
additional

restricted packages to the java.security file which are not part of
OpenJDK. These packages are only added when doing a build 
including the

open and closed sources.

The restricted packages and new test are in the closed sources and 
will

be reviewed separately.

webrev: http://cr.openjdk.java.net/~mullan/webrevs/8007292/webrev.00/


Based on your description and the ifndef OPENJDK it sounds to me that
this doesn't belong in the OpenJDK makefiles.


I agree in principle, but the pattern on how to handle this isn't well
established yet and something we need to improve on. In this case we
would need to make changes on the open side anyway to provide hooks for
overrides of the behavior. I'm willing to accept this for now.


Thanks. Also, keep in mind that this hook allows each vendor,etc to add
additional proprietary or internal packages to the restricted packages
properties for their own build. So I think it is useful in general in
that respect.


That aside I would think the CP+RM could be changed to a MV.

Agreed.


Right. Will do.


I would prefer the TOOL_ADDTO... line to be moved to
jdk/makefiles/Tools.gmk with the other similar definitions, even if it
is only used here atm.


Ok.


In the tool this code doesn't show correct use of try-with-resources:

51 try (BufferedReader br = new BufferedReader(new
FileReader(args[0]));
  52  BufferedWriter bw = new BufferedWriter(new
FileWriter(args[1]))) {

The FileReader and FileWriter should also be covered by TWR:

  try (FileReader fr = new FileReader(args[0]);
   BufferedReader br = new BufferedReader(fr);
   FileWriter fw = new FileWriter(args[1]);
   BufferedWriter bw = new BufferedWriter(fw)) {


I'm not familiar with the try-with-resources, but calling close on a
BufferedReader/writer will close the underlying reader/writer so 
nothing

will be left open, will it not?


That's what I thought as well. David?


Finally do we still use make/tools/Makefile in the new build?


No, we don't. If you want to add old build support for this, you would
also need to add usage of the tool to the correct old makefile.


I don't think it's necessary to add this to the old build at this point.

I'll post another webrev later in the day with these updates.

Thanks,
Sean

Re: [8] Review Request for 8007292 : Add JavaFX internal packages to package.access

[Erik Joelsson]

Hello Sean,

On 2013-10-09 06:33, David Holmes wrote:

Hi Sean,

Not a full review.

On 9/10/2013 5:52 AM, Sean Mullan wrote:

Please review the fix for the following bug:

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

This bug requires build changes and a new build tool to add additional
restricted packages to the java.security file which are not part of
OpenJDK. These packages are only added when doing a build including the
open and closed sources.

The restricted packages and new test are in the closed sources and will
be reviewed separately.

webrev: http://cr.openjdk.java.net/~mullan/webrevs/8007292/webrev.00/


Based on your description and the ifndef OPENJDK it sounds to me that 
this doesn't belong in the OpenJDK makefiles.


I agree in principle, but the pattern on how to handle this isn't well 
established yet and something we need to improve on. In this case we 
would need to make changes on the open side anyway to provide hooks for 
overrides of the behavior. I'm willing to accept this for now.

That aside I would think the CP+RM could be changed to a MV.

Agreed.

I would prefer the TOOL_ADDTO... line to be moved to 
jdk/makefiles/Tools.gmk with the other similar definitions, even if it 
is only used here atm.




In the tool this code doesn't show correct use of try-with-resources:

51 try (BufferedReader br = new BufferedReader(new 
FileReader(args[0]));
  52  BufferedWriter bw = new BufferedWriter(new 
FileWriter(args[1]))) {


The FileReader and FileWriter should also be covered by TWR:

  try (FileReader fr = new FileReader(args[0]);
   BufferedReader br = new BufferedReader(fr);
   FileWriter fw = new FileWriter(args[1]);
   BufferedWriter bw = new BufferedWriter(fw)) {

I'm not familiar with the try-with-resources, but calling close on a 
BufferedReader/writer will close the underlying reader/writer so nothing 
will be left open, will it not?


Finally do we still use make/tools/Makefile in the new build?

No, we don't. If you want to add old build support for this, you would 
also need to add usage of the tool to the correct old makefile.


/Erik

Re: webrev.01 of 8011402: Move blacklisting certificate logic from hard code to data

[Erik Joelsson]

Build change looks ok.

/Erik

On 2013-09-17 13:07, Weijun Wang wrote:

Webrev updated to version 02 at

  http://cr.openjdk.java.net/~weijun/8011402/webrev.02/

Changes since webrev.01:

1. Makefiles:
   - new build logic outside ifndef OPENJDK
   - Add a sed check to make sure open and close list use same algorithm

2. Fingerprint calculation moved into X509CertImpl using a 
ConcurrrentHashMap, although we only use one algorithm now.


3. Certificate::hashCode is now 0 if it's not a X509Cert

4. Cleanup comments in blacklisted.certs.pem, only 
subject/issuer/serial remain


5. Test moved to lib/security and check more.

I didn't change Certificate's private hash field to volatile.

Thanks
Max


On 9/13/13 8:29 AM, Weijun Wang wrote:



On 9/13/13 5:15 AM, Sean Mullan wrote:


Ok, I suggested you use a WeakHashMap but now I'm a little concerned
this could become a bottleneck if every certificate check has to lock
the map. Hmm. Maybe we should go back to the previous code, that also
had some concurrency issues but it was only per certificate, and wasn't
too bad since the hash would always be the same (maybe we could just
mark the fingerprint variable volatile). I think this is worse because
the lock needs to be obtained when any certificate is checked. Also,
have you thought about computing the fingerprint as you read in the
bytes of the certificate? This means every certificate object (at least
our own implementation) would have the fingerprint calculated already,
but since you are calculating the hash as you are reading in the bytes,
the performance impact might not be much at all. However one issue is
that you don't know what algorithm to use, unless you read in the
blacklist file. We could just assume SHA-256 for now.


I'll think about it later. Some CPU bugs on the plate now.

Maybe I can keep a small MapAlgorithm,fingerprint in each certificate,
and as you suggested, pre-fill the SHA-256 value at read time.

ConcurrentMap should have a method called getOrCalculate that takes a
lambda that could calucalte the value lazily.



Certificate:

[70] I would mark this volatile.


Aha, I copied the logic from String and thought it must be the safest.



[134] the old code returned 0, seems we should preserve that even 
though

this is an odd error case


Do you think I should also revert the calculation from
Arrays.hashCode(X509CertImpl.getEncodedInternal(this)) to the old for
loop?



CheckAll:

I would move this test to test/lib/java/security and rename it to
CheckBlacklistedCerts.


Certainly.



You should also check that the files have the same number of entries.


In my assumption, open and closed could have dups. I'll compare the Set
size then.



UntrustedCertificates:

[84] nit: insert spaces around the = and 


OK.

Thanks
Max



--Sean


On 09/11/2013 09:32 AM, Weijun Wang wrote:

Slightly updated at the same location.

Added different algorithm check in the 2 makefiles.

Thanks
Max


On 9/11/13 3:57 PM, Weijun Wang wrote:

Hi Sean and Erik

An updated webrev is at

   http://cr.openjdk.java.net/~weijun/8011402/webrev.01/

Changes since the last webrev:

- Some makefile changes
   * wildcard on closed file
   * make sure the file's first line is always Algorithm=
- Move fingerprint cache for cert from X509CertImpl to
UntrustedCertificates
- Cache hash for Certificate
- log blacklist parsing error in UntrustedCertificates
- A new test

Thanks
Max

On 9/6/13 9:30 PM, Weijun Wang wrote:

Hi Sean

Please review the code changes at

   8011402: Move blacklisting certificate logic from hard code to 
data


http://cr.openjdk.java.net/~weijun/8011402/webrev.00/



Hard coded blacklisted certificates are moved out of the class file
and
now inside a data file. Furthermore, only their fingerprints are
released in the JRE. The makefile covers blacklist files in both 
open

and closed repo.

No regression test, cleanup.

*build-dev*, I am not an export of Makefile, and I have some
questions:

1. I create a new macro (or function?) called cat-files. Its only
difference from install-file is that it needs to deal with two 
inputs.

Do we already have a similar macro somewhere?

2. cat-files is defined inside CopyFiles.gmk right beside its
usage. Do
you think it's better to define it in a common file?

3. Most important: it only works if both 
$(BLACKLISTED_CERTS_SRC_OPEN)
and $(BLACKLISTED_CERTS_SRC_CLOSED) already exists. Currently 
there is

no closed blacklist, but I still have to create an empty file there.
Otherwise, there will be

make[2]: *** No rule to make target
`/space/repos/jdk8/tl/jdk/src/closed/share/lib/security/blacklisted.certs', 






needed by
`/space/repos/jdk8/tl/build/macosx-x86_64-normal-server-release/jdk/lib/security/blacklisted.certs'. 






  Stop.

Is there a way to make it work without adding that empty file?

Thanks
Max