Re: RFR: 8282191: Implementation of Foreign Function & Memory API (Preview) [v4]
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]
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)
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]
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]
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
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]
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]
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]
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
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)
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
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]
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]
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
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
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]
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]
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]
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]
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
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
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
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
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]
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
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
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]
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]
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
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
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
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]
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 >> `MemoryAdd
Re: RFR: 8254162: Implementation of Foreign-Memory Access API (Third Incubator) [v2]
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 >> `MemoryAdd
Re: RFR: 8254162: Implementation of Foreign-Memory Access API (Third Incubator)
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 implementat
Re: RFR: 8235710: Remove the legacy elliptic curves [v2]
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
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]
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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 rel
Re: RFR (XL): JDK-8204572 SetupJdkLibrary should setup SRC and -I flags automatically
Hello Magnus, Very nice refactoring! 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. 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 reason for the difference was the same as for the Solaris libs; the include paths had changes, which caused a binary diff. For libmlib_image.dylib, the link order had changed causing the noted binary difference. * Linux: On linux, the compare script noted differences for libextnet, libjava, libmlib_image, libprefs, libsaproc, libsplashscreen and libsunec. The differences for libextnet, libprefs, libsplashscreen and libsunec turned out
Re: RFR 8201815: Use Mozilla Public Suffix List
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 : On May 23, 2018, at 4:21 AM, Magnus Ihse Bursie 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 : 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
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
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
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 deco
Re: RFR: JDK-8199640 Split up BUILD_LIBKRB5 into the two, unrelated compilations it consists of
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
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
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
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
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
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
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
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
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
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
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
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: RFR 8133151: Preferred provider configuration for JCE
Makefile change looks ok. /Erik On 2015-10-02 19:08, Anthony Scarpino wrote: Hi all, I'm need a review of the last developement piece to JEP 246, the configuration changes. I've copied the build-dev in case there were any comments on the minor changes in the make directory related to the java.security file. http://cr.openjdk.java.net/~ascarpino/8133151/webrev/ thanks Tony
Re: [9] RFR 8086002: Move apple.security.AppleProvider to a proper module
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 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
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
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"
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
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 Peng 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,
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,
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 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
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
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
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
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
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
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
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
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
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 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 wrote: On Jul 25, 2014, at 22:30, Sean Mullan 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
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 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 wrote: On Jul 25, 2014, at 22:30, Sean Mullan 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
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 wrote: On Jul 25, 2014, at 22:30, Sean Mullan 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
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
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
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; ...
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)
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: 8030781: System.setProperties(null) drops all system properties (RELEASE not set)
Changeset: 53d74b77ee53 Author:erikj Date: 2014-01-08 14:02 +0100 URL: http://hg.openjdk.java.net/jdk8/tl/rev/53d74b77ee53 8030781: System.setProperties(null) drops all system properties (RELEASE not set) Reviewed-by: alanb, ihse, tbell ! common/autoconf/generated-configure.sh ! common/autoconf/toolchain.m4
hg: jdk8/tl/jdk: 8027963: Create unlimited policy jars.
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.
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/jdk: 2 new changesets
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
hg: jdk8/tl/corba: 6604021: RMIC is defaulting to BOOT jdk version, needs to be rmic.jar
Changeset: 438c54c148a6 Author:erikj Date: 2013-10-16 13:49 +0200 URL: http://hg.openjdk.java.net/jdk8/tl/corba/rev/438c54c148a6 6604021: RMIC is defaulting to BOOT jdk version, needs to be rmic.jar Reviewed-by: dholmes, chegar ! makefiles/BuildCorba.gmk
hg: jdk8/tl: 6604021: RMIC is defaulting to BOOT jdk version, needs to be rmic.jar
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
