RFR: 8297385: Remove duplicated null typos in javadoc
8297385: Remove duplicated null typos in javadoc - Commit messages: - 8297385: Remove duplicated null typo in javadoc Changes: https://git.openjdk.org/jdk/pull/11311/files Webrev: https://webrevs.openjdk.org/?repo=jdk=11311=00 Issue: https://bugs.openjdk.org/browse/JDK-8297385 Stats: 3 lines in 1 file changed: 0 ins; 0 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/11311.diff Fetch: git fetch https://git.openjdk.org/jdk pull/11311/head:pull/11311 PR: https://git.openjdk.org/jdk/pull/11311
Withdrawn: 8297385: Remove duplicated null typos in javadoc
On Tue, 15 Nov 2022 15:05:45 GMT, Dongxu Wang wrote: > 8297385: Remove duplicated null typos in javadoc This pull request has been closed without being integrated. - PR: https://git.openjdk.org/jdk/pull/11169
Re: RFR: 8297385: Remove duplicated null typos in javadoc [v2]
On Wed, 23 Nov 2022 06:49:56 GMT, Dongxu Wang wrote: >> 8297385: Remove duplicated null typos in javadoc > > Dongxu Wang has updated the pull request with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains two additional > commits since the last revision: > > - Merge branch 'openjdk:master' into master > - Minor remove duplicate null typo Use #11311 instead, close this pr. - PR: https://git.openjdk.org/jdk/pull/11169
Re: RFR: 8297385: Remove duplicated null typos in javadoc [v2]
On Wed, 23 Nov 2022 06:43:56 GMT, Yi Yang wrote: > This looks good, but I'm not a Reviewer, you still need an approval from > Reviewer. Thanks - PR: https://git.openjdk.org/jdk/pull/11169
Re: RFR: 8297385: Remove duplicated null typos in javadoc [v2]
> 8297385: Remove duplicated null typos in javadoc Dongxu Wang has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains two additional commits since the last revision: - Merge branch 'openjdk:master' into master - Minor remove duplicate null typo - Changes: - all: https://git.openjdk.org/jdk/pull/11169/files - new: https://git.openjdk.org/jdk/pull/11169/files/65327c89..f731384b Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=11169=01 - incr: https://webrevs.openjdk.org/?repo=jdk=11169=00-01 Stats: 44876 lines in 726 files changed: 16653 ins; 16626 del; 11597 mod Patch: https://git.openjdk.org/jdk/pull/11169.diff Fetch: git fetch https://git.openjdk.org/jdk pull/11169/head:pull/11169 PR: https://git.openjdk.org/jdk/pull/11169
Re: RFR: 8297385: Remove duplicated null typos in javadoc [v2]
On Wed, 23 Nov 2022 06:45:26 GMT, Dongxu Wang wrote: >> 8297385: Remove duplicated null typos in javadoc > > Dongxu Wang has updated the pull request with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains two additional > commits since the last revision: > > - Merge branch 'openjdk:master' into master > - Minor remove duplicate null typo This looks good, but I'm not a Reviewer, you still need an approval from Reviewer. - Marked as reviewed by yyang (Committer). PR: https://git.openjdk.org/jdk/pull/11169
Re: RFR: 8295146: Clean up native code with newer C/C++ language features [v3]
On Mon, 14 Nov 2022 12:20:54 GMT, Julian Waters wrote: >> Sorry my eyes must be playing tricks on me. ?? >> >> Why did you need to add this here? > > It's to avoid redefining the linkage as static in os_windows.cpp (where it's > implemented) after an extern declaration (inside the class), which is > forbidden by C++11: > >> The linkages implied by successive declarations for a given entity shall >> agree. That is, within a given scope, each declaration declaring the same >> variable name or the same overloading of a function name shall imply the >> same linkage. > > While 2019 by default seems to ignore this rule and accepts the conflicting > linkage as a language extension, this can cause issues with newer and > stricter versions of the Visual C++ compiler (especially with -permissive- > passed during compilation, which Magnus and Daniel have pointed out in > another discussion will become the default mode of compilation in the > future). It's not possible to declare a static friend inside a class, so the > addition above takes advantage of another C++ feature instead: > >> §11.3/4 [class.friend] > A function first declared in a friend declaration has external linkage (3.5). > Otherwise, the function retains its previous linkage (7.1.1). I think the problem here is the friend declaration, which doesn't look like it's needed and could be deleted. - PR: https://git.openjdk.org/jdk/pull/11081
RFR: 8297451: ProcessHandleImpl should assert privilege when modifying reaper thread
This commit guards thread modifications for the process reaper thread with doPrivileged. - Commit messages: - 8297451: ProcessHandleImpl should assert privilege when modifying reaper thread Changes: https://git.openjdk.org/jdk/pull/11309/files Webrev: https://webrevs.openjdk.org/?repo=jdk=11309=00 Issue: https://bugs.openjdk.org/browse/JDK-8297451 Stats: 31 lines in 2 files changed: 19 ins; 1 del; 11 mod Patch: https://git.openjdk.org/jdk/pull/11309.diff Fetch: git fetch https://git.openjdk.org/jdk pull/11309/head:pull/11309 PR: https://git.openjdk.org/jdk/pull/11309
Re: RFR: 8295146: Clean up native code with newer C/C++ language features [v3]
On Mon, 21 Nov 2022 02:43:12 GMT, Julian Waters wrote: > Out of curiosity, is there a way to get the discussion on approving the use > of alignas back up? [...] A PR to address JDK-8252584 would be welcomed by me. Just do the process for Style Guide changes (see the Style Guide or previous PRs for such). I don't expect it would be very controversial. I think the only reason it hasn't already happened is because nobody has gotten around to it, or felt the need for it. JDK-8250269 touches a bit more code (mostly in stubGenerator_x86_64 and macroAssembler_x86_32), but also seems like it should be straightforward. > > The various MSVC-conditional direct uses of __declspec(align(N)) should > > probably currently be using ATTRIBUTE_ALIGNED. > > The instances of `__declspec(align())` changed here are in the native > libraries written in C, not within HotSpot itself. From what I can see at > least HotSpot never uses compiler alignment attributes directly and always > strictly sticks to `ATTRIBUTE_ALIGNED` (which is probably a good thing) You are right that the Windows-conditionalized uses are in non-HotSpot code. I missed that context when skimming through the changes. Since Visual Studio is always C++ (even though the shared files are written as C), using alignas with appropriate conditionalization in those files should be fine. - PR: https://git.openjdk.org/jdk/pull/11081
Integrated: 8296329: jar validator doesn't account for minor class file version
On Tue, 15 Nov 2022 01:52:14 GMT, Bo Zhang wrote: > As described in [JDK-8296329](https://bugs.openjdk.org/browse/JDK-8296329), > previously, the jar validator compare the "version" to validate a > multi-release jar. The "version" is a mix of the major and minor version > fused into a single int, which might be a negative number with > `--enable-preview` - this result in wrong comparison. > > This PR fixes it by only comparing major versions. This pull request has now been integrated. Changeset: faf48e61 Author:Bo Zhang Committer: Jorn Vernee URL: https://git.openjdk.org/jdk/commit/faf48e61be4f97f725b053aa351d3c64638546bf Stats: 127 lines in 3 files changed: 123 ins; 1 del; 3 mod 8296329: jar validator doesn't account for minor class file version Reviewed-by: jvernee - PR: https://git.openjdk.org/jdk/pull/11153
Re: RFR: 8296329: jar validator doesn't account for minor class file version [v2]
On Wed, 23 Nov 2022 03:01:51 GMT, Jorn Vernee wrote: >> @JornVernee can you please sponsor this PR? > > @blindpirate Yes. If you `/integrate` it, I can then `/sponsor`. Thanks @JornVernee ! - PR: https://git.openjdk.org/jdk/pull/11153
Re: RFR: 8296329: jar validator doesn't account for minor class file version [v2]
On Wed, 23 Nov 2022 02:50:26 GMT, Bo Zhang wrote: >> Marked as reviewed by jvernee (Reviewer). > > @JornVernee can you please sponsor this PR? @blindpirate Yes. If you `/integrate` it, I can then `/sponsor`. - PR: https://git.openjdk.org/jdk/pull/11153
Re: RFR: 8296329: jar validator doesn't account for minor class file version [v2]
On Wed, 16 Nov 2022 13:23:03 GMT, Jorn Vernee wrote: >> Bo Zhang has refreshed the contents of this pull request, and previous >> commits have been removed. The incremental views will show differences >> compared to the previous content of the PR. The pull request contains one >> new commit since the last revision: >> >> 8296329: Only compare major versions in jar validator > > Marked as reviewed by jvernee (Reviewer). @JornVernee can you please sponsor this PR? - PR: https://git.openjdk.org/jdk/pull/11153
Re: RFR: 8297286: runtime/vthread tests crashing after JDK-8296324
On Wed, 23 Nov 2022 00:24:28 GMT, Serguei Spitsyn wrote: > This problem has two sides. > One is that the `VirtualThread::run() `cashes the field `notifyJvmtiEvents` > value. > It caused the native method `notifyJvmtiUnmountBegin()` not called after the > field `notifyJvmtiEvents` > value has been set to `true` when an agent library is loaded into running VM. > The fix is to get rid of this cashing. > Another is that enabling `notifyJvmtiEvents` notifications needs a > synchronization. > Otherwise, a VTMS transition start can be missed which will cause some > asserts to fire. > The fix is to use a JvmtiVTMSTransitionDisabler helper for sync. > > Testing: > The originally failed tests are passed now: > > runtime/vthread/RedefineClass.java > runtime/vthread/TestObjectAllocationSampleEvent.java > > In progress: > Run the tiers 1-6 to make sure there are no regression. Needed to check yieldContinuation() method for same issue. - Changes requested by lmesnik (Reviewer). PR: https://git.openjdk.org/jdk/pull/11304
Re: RFR: 8297286: runtime/vthread tests crashing after JDK-8296324
On Wed, 23 Nov 2022 00:24:28 GMT, Serguei Spitsyn wrote:
> This problem has two sides.
> One is that the `VirtualThread::run() `cashes the field `notifyJvmtiEvents`
> value.
> It caused the native method `notifyJvmtiUnmountBegin()` not called after the
> field `notifyJvmtiEvents`
> value has been set to `true` when an agent library is loaded into running VM.
> The fix is to get rid of this cashing.
> Another is that enabling `notifyJvmtiEvents` notifications needs a
> synchronization.
> Otherwise, a VTMS transition start can be missed which will cause some
> asserts to fire.
> The fix is to use a JvmtiVTMSTransitionDisabler helper for sync.
>
> Testing:
> The originally failed tests are passed now:
>
> runtime/vthread/RedefineClass.java
> runtime/vthread/TestObjectAllocationSampleEvent.java
>
> In progress:
> Run the tiers 1-6 to make sure there are no regression.
Marked as reviewed by lmesnik (Reviewer).
src/java.base/share/classes/java/lang/VirtualThread.java line 273:
> 271: private void run(Runnable task) {
> 272: assert state == RUNNING;
> 273: boolean notifyJvmti = notifyJvmtiEvents;
Don't we have same issue in yieldContinuation() method? (line 396)
-
PR: https://git.openjdk.org/jdk/pull/11304
RFR: 8297286: runtime/vthread tests crashing after JDK-8296324
This problem has two sides. One is that the `VirtualThread::run() `cashes the field `notifyJvmtiEvents` value. It caused the native method `notifyJvmtiUnmountBegin()` not called after the field `notifyJvmtiEvents` value has been set to `true` when an agent library is loaded into running VM. The fix is to get rid of this cashing. Another is that enabling `notifyJvmtiEvents` notifications needs a synchronization. Otherwise, a VTMS transition start can be missed which will cause some asserts to fire. The fix is to use a JvmtiVTMSTransitionDisabler helper for sync. Testing: The originally failed tests are passed now: runtime/vthread/RedefineClass.java runtime/vthread/TestObjectAllocationSampleEvent.java In progress: Run the tiers 1-6 to make sure there are no regression. - Commit messages: - 8297286: runtime/vthread tests crashing after JDK-8296324 Changes: https://git.openjdk.org/jdk/pull/11304/files Webrev: https://webrevs.openjdk.org/?repo=jdk=11304=00 Issue: https://bugs.openjdk.org/browse/JDK-8297286 Stats: 6 lines in 3 files changed: 1 ins; 3 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/11304.diff Fetch: git fetch https://git.openjdk.org/jdk pull/11304/head:pull/11304 PR: https://git.openjdk.org/jdk/pull/11304
Re: RFR: 8295857: Clarify that cleanup code can be skipped when the JVM terminates (e.g. when calling halt()) [v4]
On Tue, 22 Nov 2022 00:50:51 GMT, Brent Christian wrote:
>> [JDK-8290036](https://bugs.openjdk.org/browse/JDK-8290036) documented the
>> shutdown sequence, noting that calling Runtime.halt() skips the shutdown
>> sequence and immediately terminates the VM. Thus, "threads' current methods
>> do not complete normally or abruptly; no finally clause of any method is
>> executed".
>>
>> One ramification of this is that resources within try-with-resource blocks
>> will not be released. It would be good to state this explicitly.
>
> Brent Christian has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Update Runtime class doc re: other unexpected behaviors
src/java.base/share/classes/java/lang/Runtime.java line 97:
> 95: * no {@code finally} clause of any method is executed, and
> try-with-resources
> 96: * blocks do not {@linkplain AutoCloseable close} their resources.
> 97: * Other unexpected behaviors may also result.
No sorry that doesn't work for me. It sounds like you might get unexpected
things happening, when in fact it is things not happening that might surprise
someone (who doesn't understand what "immediately prevented from executing any
further Java code" means). The list of examples is getting too long to handle
in sentence structure so I suggest an actual list (not sure of list syntax).
Suggestion:
* Java code. This includes shutdown hooks as well as daemon and non-daemon
threads. This
* means, for example, that:
* - threads' current methods do not complete normally or abruptly
* - no {@code finally} clause of any method is executed
* - no {@linkplain Thread.UncaughtExceptionHandler uncaught exception
handler} is executed
* - no try-with-resources blocks {@linkplain AutoCloseable close} their
resources
-
PR: https://git.openjdk.org/jdk/pull/11218
Re: RFR: 8296546: Add @spec tags to API [v2]
> Please review a "somewhat automated" change to insert `@spec` tags into doc > comments, as appropriate, to leverage the recent new javadoc feature to > generate a new page listing the references to all external specifications > listed in the `@spec` tags. > > "Somewhat automated" means that I wrote and used a temporary utility to scan > doc comments looking for HTML links to selected sites, such as `ietf.org`, > `unicode.org`, `w3.org`. These links may be in the main description of a doc > comment, or in `@see` tags. For each link, the URL is examined, and > "normalized", and inserted into the doc comment with a new `@spec` tag, > giving the link and tile for the spec. > > "Normalized" means... > * Use `https:` where possible (includes pretty much all cases) > * Use a single consistent host name for all URLs coming from the same spec > site (i.e. don't use different aliases for the same site) > * Point to the root page of a multi-page spec > * Use a consistent form of the spec, preferring HTML over plain text where > both are available (this mostly applies to IETF specs) > > In addition, a "standard" title is determined for all specs, determined > either from the content of the (main) spec page or from site index pages. > > The net effect is (or should be) that **all** the changes are to just **add** > new `@spec` tags, based on the links found in each doc comment. There should > be no other changes to the doc comments, or to the implementation of any > classes and interfaces. > > That being said, the utility I wrote does have additional abilities, to > update the links that it finds (e.g. changing to use `https:` etc,) but those > features are _not_ being used here, but could be used in followup PRs if > component teams so desired. I did notice while working on this overall > feature that many of our links do point to "outdated" pages, some with > eye-catching notices declaring that the spec has been superseded. Determining > how, when and where to update such links is beyond the scope of this PR. > > Going forward, it is to be hoped that component teams will maintain the > underlying links, and the URLs in `@spec` tags, such that if references to > external specifications are updated, this will include updating the `@spec` > tags. > > To see the effect of all these new `@spec` tags, see > http://cr.openjdk.java.net/~jjg/8296546/api.00/ > > In particular, see the new [External > Specifications](http://cr.openjdk.java.net/~jjg/8296546/api.00/external-specs.html) > page, which you can also find via the new link near the top of the > [Index](http://cr.openjdk.java.net/~jjg/8296546/api.00/index-files/index-1.html) > pages. Jonathan Gibbons has updated the pull request incrementally with one additional commit since the last revision: Prefix RFC titles with `RFC :` - Changes: - all: https://git.openjdk.org/jdk/pull/11073/files - new: https://git.openjdk.org/jdk/pull/11073/files/30ce235f..c29092d8 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=11073=01 - incr: https://webrevs.openjdk.org/?repo=jdk=11073=00-01 Stats: 325 lines in 165 files changed: 4 ins; 4 del; 317 mod Patch: https://git.openjdk.org/jdk/pull/11073.diff Fetch: git fetch https://git.openjdk.org/jdk pull/11073/head:pull/11073 PR: https://git.openjdk.org/jdk/pull/11073
Re: JDK 19 innocuous reaper threads
Thanks you Alan and Roger. I filed the following issue to track this: https://bugs.openjdk.org/browse/JDK-8297451 There are likely many more usages of innocuous thread that could be improved by ensuring setDaemon is invoked while asserting privileges, but I'd like to leave those to a later issue, since the usage in process is causing us issues right now, and it would be great to get this fixed (and eventually backported to 19.0.2). -Chris. On 22/11/2022 17:01, Roger Riggs wrote: Hi Chris, Yes, adding a doPriv for setDaemon and setName in a couple of places makes sense. Thanks, Roger On 11/22/22 11:12 AM, Chris Hegarty wrote: Hi Alan, On 22/11/2022 16:08, Alan Bateman wrote: On 22/11/2022 15:21, Chris Hegarty wrote: .. Just to double check, does the ES security manager override checkAccess(Thread)? Yes. :-( That is usually a no-op but if overridden then it will expose an issue with the thread factory for the "process reaper" where it attempts changes the daemon status outside of a doPriv block. Right. That's exactly what we're running into. If there are no objections, then I'm happy to file an issue and PR to add narrow doPriv blocks around these calls. -Chris [1] https://github.com/elastic/elasticsearch/blob/main/libs/secure-sm/src/main/java/org/elasticsearch/secure_sm/SecureSM.java#L118
Re: RFR: JDK-8285932 Implementation of JEP 430 String Templates (Preview) [v28]
On Tue, 22 Nov 2022 18:43:53 GMT, Roger Riggs wrote:
>> Jim Laskey has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Seal Digits
>
> src/java.base/share/classes/java/lang/template/StringProcessor.java line 58:
>
>> 56: /**
>> 57: * Constructs a {@link String} based on the template fragments and
>> values in the
>> 58: * supplied {@link StringTemplate stringTemplate} object.
>
> When reading the javadoc, I expected the type name, not the variable name.
> Suggestion:
>
> * supplied {@link StringTemplate StringTemplate} object.
Changing.
-
PR: https://git.openjdk.org/jdk/pull/10889
Re: RFR: JDK-8285932 Implementation of JEP 430 String Templates (Preview) [v28]
On Tue, 22 Nov 2022 18:33:23 GMT, Roger Riggs wrote:
>> Jim Laskey has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Seal Digits
>
> src/java.base/share/classes/java/lang/template/StringTemplate.java line 52:
>
>> 50: * given by the template expression.
>> 51: *
>> 52: * For example, the following code contains a template expression that
>> uses the template
>
> Though this is trying to explain the general mechanism, it might be more
> useful to readers to start with the most common use case, that of using a
> string processor. Swapping the order of the examples possibly.
The issue is that most users will not see a StringTemplate object. If they've
come here then they want to see the inner workings.
> src/java.base/share/classes/java/lang/template/StringTemplate.java line 63:
>
>> 61: * {@code fragments} will be equivalent to {@code List.of("", " + ", " =
>> ", "")},
>> 62: * which includes the empty first and last fragments. {@code values}
>> will be the
>> 63: * equivalent of {@code List.of(10, 20, 30)}.
>
> Find a way to capitalize the first word of the sentence.
> Suggestion:
>
> * The value of {@code fragments} will be equivalent to {@code List.of("", "
> + ", " = ", "")},
> * which includes the empty first and last fragments. The {@code values} will
> be the
> * equivalent of {@code List.of(10, 20, 30)}.
Changing.
> src/java.base/share/classes/java/lang/template/StringTemplate.java line 66:
>
>> 64: *
>> 65: * The following code contains a template expression with the same
>> template but a
>> 66: * different template processor:
>
> Suggestion:
>
> * The following code contains a template expression with the same template
> but a
> * string template processor:
Changing. Missing "with" as well.
> src/java.base/share/classes/java/lang/template/StringTemplate.java line 75:
>
>> 73: * produced that returns the same lists from {@link
>> StringTemplate#fragments()} and
>> 74: * {@link StringTemplate#values()} as shown above. The {@link
>> StringTemplate#STR} template
>> 75: * processor uses these lists to yield an interpolated string. {@code s}
>> will be equivalent to
>
> Suggestion:
>
> * processor uses these lists to yield an interpolated string. The value of
> {@code s} will be equivalent to
Changing.
> src/java.base/share/classes/java/util/FormatProcessor.java line 196:
>
>> 194: * format specifier.
>> 195: * StringTemplate expressions without a preceeding specifier, use
>> "%s" by
>> 196:
>
> Its worth specifying the locale that is used by FMT, is it `Locale.ROOT`?
> If it is `Locale.default`, the results may vary from run to run.
Okay.
-
PR: https://git.openjdk.org/jdk/pull/10889
Re: RFR: JDK-8285932 Implementation of JEP 430 String Templates (Preview) [v28]
On Tue, 22 Nov 2022 18:17:28 GMT, Roger Riggs wrote:
>> Jim Laskey has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Seal Digits
>
> src/java.base/share/classes/java/lang/template/package-info.java line 57:
>
>> 55: *{@link java.lang.template.StringTemplate}. The end result of the
>> process template
>> 56: * expression is the value that is produced by invoking the processor's
>> 57: *{@link java.lang.template.ValidatingProcessor#process(StringTemplate)}
>
> I would reduce the use of "proper", it should be sufficient to describe them
> without the qualifier.
> The single use of "improper" is fine to warn of compilation errors.
>
> Also, add a space between "*" and "{".
Changing.
> src/java.base/share/classes/java/lang/template/package-info.java line 61:
>
>> 59: * improper processor arguments result in compilation errors.
>> 60: *
>> 61: * In the example, {@code STR."The result of adding \{x} and \{y} is \{x
>> + y}."},
>
> Maybe avoid the repetition of the example.
> Suggestion:
>
> * In the example above,
Changing.
> src/java.base/share/classes/java/lang/template/package-info.java line 76:
>
>> 74: * String literals and text blocks can be used as proper processor
>> arguments as
>> 75: * well. This is automatically facilitated by the Java compiler
>> converted the
>> 76: * strings to {@link java.lang.template.StringTemplate StringTemplate}
>> using the
>
> Suggestion:
>
> * well. This is automatically facilitated by the Java compiler converting the
> * strings to {@link java.lang.template.StringTemplate StringTemplate} using
> the
Changing.
> src/java.base/share/classes/java/lang/template/package-info.java line 79:
>
>> 77: * {@link java.lang.template.StringTemplate#of(String)} method.
>> 78: *
>> 79: * Users can create their own template processors by implementing either
>
> Suggestion:
>
> * Users can create their own template processors by implementing one of
Changing.
> src/java.base/share/classes/java/lang/template/package-info.java line 83:
>
>> 81: * {@link java.lang.template.TemplateProcessor} or
>> 82: * {@link java.lang.template.StringProcessor} interfaces.
>> 83: * See {@link java.lang.template.ValidatingProcessor} for examples and
>> details.
>
> I would give the reader a firm what to read next suggestion.
> It may be useful to mention and link to the `FMT` processor as a way to
> control the formatting of the values.
> Suggestion:
>
> * For more examples and details see {@link
> java.lang.template.StringTemplate} and
> * {@link java.lang.template.ValidatingProcessor}.
Changing.
-
PR: https://git.openjdk.org/jdk/pull/10889
Re: RFR: JDK-8285932 Implementation of JEP 430 String Templates (Preview) [v28]
On Mon, 21 Nov 2022 17:43:16 GMT, Jim Laskey wrote:
>> Enhance the Java programming language with string templates, which are
>> similar to string literals but contain embedded expressions. A string
>> template is interpreted at run time by replacing each expression with the
>> result of evaluating that expression, possibly after further validation and
>> transformation. This is a [preview language feature and
>> API](http://openjdk.java.net/jeps/12).
>
> Jim Laskey has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Seal Digits
Javadoc suggestions around readability, linking.
src/java.base/share/classes/java/lang/template/StringProcessor.java line 58:
> 56: /**
> 57: * Constructs a {@link String} based on the template fragments and
> values in the
> 58: * supplied {@link StringTemplate stringTemplate} object.
When reading the javadoc, I expected the type name, not the variable name.
Suggestion:
* supplied {@link StringTemplate StringTemplate} object.
src/java.base/share/classes/java/lang/template/StringTemplate.java line 52:
> 50: * given by the template expression.
> 51: *
> 52: * For example, the following code contains a template expression that
> uses the template
Though this is trying to explain the general mechanism, it might be more useful
to readers to start with the most common use case, that of using a string
processor. Swapping the order of the examples possibly.
src/java.base/share/classes/java/lang/template/StringTemplate.java line 63:
> 61: * {@code fragments} will be equivalent to {@code List.of("", " + ", " =
> ", "")},
> 62: * which includes the empty first and last fragments. {@code values} will
> be the
> 63: * equivalent of {@code List.of(10, 20, 30)}.
Find a way to capitalize the first word of the sentence.
Suggestion:
* The value of {@code fragments} will be equivalent to {@code List.of("", " +
", " = ", "")},
* which includes the empty first and last fragments. The {@code values} will
be the
* equivalent of {@code List.of(10, 20, 30)}.
src/java.base/share/classes/java/lang/template/StringTemplate.java line 66:
> 64: *
> 65: * The following code contains a template expression with the same
> template but a
> 66: * different template processor:
Suggestion:
* The following code contains a template expression with the same template but
a
* string template processor:
src/java.base/share/classes/java/lang/template/StringTemplate.java line 75:
> 73: * produced that returns the same lists from {@link
> StringTemplate#fragments()} and
> 74: * {@link StringTemplate#values()} as shown above. The {@link
> StringTemplate#STR} template
> 75: * processor uses these lists to yield an interpolated string. {@code s}
> will be equivalent to
Suggestion:
* processor uses these lists to yield an interpolated string. The value of
{@code s} will be equivalent to
src/java.base/share/classes/java/lang/template/StringTemplate.java line 319:
> 317: * This {@link StringProcessor} instance is conventionally used for
> the string interpolation
> 318: * of a supplied {@link StringTemplate}.
> 319: *
It should be mentioned that the string representations are created as if
invoking {@link String#valueOf}.
An perhaps a link/@see to `FMT` for control over formatting.
Also include it in the javadoc for `interpolate`.
src/java.base/share/classes/java/lang/template/package-info.java line 57:
> 55: *{@link java.lang.template.StringTemplate}. The end result of the
> process template
> 56: * expression is the value that is produced by invoking the processor's
> 57: *{@link java.lang.template.ValidatingProcessor#process(StringTemplate)}
I would reduce the use of "proper", it should be sufficient to describe them
without the qualifier.
The single use of "improper" is fine to warn of compilation errors.
Also, add a space between "*" and "{".
src/java.base/share/classes/java/lang/template/package-info.java line 61:
> 59: * improper processor arguments result in compilation errors.
> 60: *
> 61: * In the example, {@code STR."The result of adding \{x} and \{y} is \{x
> + y}."},
Maybe avoid the repetition of the example.
Suggestion:
* In the example above,
src/java.base/share/classes/java/lang/template/package-info.java line 76:
> 74: * String literals and text blocks can be used as proper processor
> arguments as
> 75: * well. This is automatically facilitated by the Java compiler converted
> the
> 76: * strings to {@link java.lang.template.StringTemplate StringTemplate}
> using the
Suggestion:
* well. This is automatically facilitated by the Java compiler converting the
* strings to {@link java.lang.template.StringTemplate StringTemplate} using the
src/java.base/share/classes/java/lang/template/package-info.java line 79:
> 77: * {@link java.lang.template.StringTemplate#of(String)} method.
> 78: *
> 79: * Users can create their own
Re: RFR: 8296024: Usage of DIrectBuffer::address should be guarded [v9]
On Tue, 22 Nov 2022 13:49:45 GMT, Per Minborg wrote:
>> src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java
>> line 914:
>>
>>> 912: * If so, make a copy to put the dst data in.
>>> 913: */
>>> 914: @SuppressWarnings("try")
>>
>> After looking at the implementation some more, I'm not sure this need
>> fixing? E.g. this method is just using the address to compute some overlap -
>> and return a buffer sliced accordingly. There's no access to the buffer data
>> (except for the last part which does a `put`). The access will fail if the
>> session is closed from underneath. I don't think this can crash the VM (in
>> fact this code did not have a reachability fence to begin with).
>
> Well spotted. I will remove the guarding here.
This `@SuppressWarnings` annotation is no longer needed:
Suggestion:
-
PR: https://git.openjdk.org/jdk/pull/11260
Re: RFR: JDK-8286666: JEP 429: Implementation of Scoped Values (Incubator) [v21]
> JEP 429 implementation. Andrew Haley has updated the pull request incrementally with three additional commits since the last revision: - Merge branch 'JDK-828' of https://github.com/theRealAph/jdk into JDK-828 - Update src/java.base/share/classes/java/lang/VirtualThread.java Co-authored-by: Alan Bateman - Feedback from reviewers - Changes: - all: https://git.openjdk.org/jdk/pull/10952/files - new: https://git.openjdk.org/jdk/pull/10952/files/b06ea927..dc577736 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=10952=20 - incr: https://webrevs.openjdk.org/?repo=jdk=10952=19-20 Stats: 2 lines in 2 files changed: 2 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/10952.diff Fetch: git fetch https://git.openjdk.org/jdk pull/10952/head:pull/10952 PR: https://git.openjdk.org/jdk/pull/10952
Re: RFR: JDK-8286666: JEP 429: Implementation of Scoped Values (Incubator) [v20]
> JEP 429 implementation. Andrew Haley has updated the pull request incrementally with one additional commit since the last revision: Update src/java.base/share/classes/java/lang/Thread.java Co-authored-by: ExE Boss <3889017+exe-b...@users.noreply.github.com> - Changes: - all: https://git.openjdk.org/jdk/pull/10952/files - new: https://git.openjdk.org/jdk/pull/10952/files/7c49e676..b06ea927 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=10952=19 - incr: https://webrevs.openjdk.org/?repo=jdk=10952=18-19 Stats: 8 lines in 1 file changed: 5 ins; 3 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/10952.diff Fetch: git fetch https://git.openjdk.org/jdk pull/10952/head:pull/10952 PR: https://git.openjdk.org/jdk/pull/10952
Re: RFR: JDK-8286666: JEP 429: Implementation of Scoped Values (Incubator) [v19]
> JEP 429 implementation. Andrew Haley has updated the pull request incrementally with one additional commit since the last revision: Update src/hotspot/cpu/aarch64/aarch64.ad Co-authored-by: ExE Boss <3889017+exe-b...@users.noreply.github.com> - Changes: - all: https://git.openjdk.org/jdk/pull/10952/files - new: https://git.openjdk.org/jdk/pull/10952/files/afad922c..7c49e676 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=10952=18 - incr: https://webrevs.openjdk.org/?repo=jdk=10952=17-18 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/10952.diff Fetch: git fetch https://git.openjdk.org/jdk pull/10952/head:pull/10952 PR: https://git.openjdk.org/jdk/pull/10952
Re: RFR: JDK-8286666: JEP 429: Implementation of Scoped Values (Incubator) [v18]
> JEP 429 implementation. Andrew Haley has updated the pull request incrementally with one additional commit since the last revision: Remove incorrect assertion. - Changes: - all: https://git.openjdk.org/jdk/pull/10952/files - new: https://git.openjdk.org/jdk/pull/10952/files/04320c7b..afad922c Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=10952=17 - incr: https://webrevs.openjdk.org/?repo=jdk=10952=16-17 Stats: 3 lines in 1 file changed: 0 ins; 2 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/10952.diff Fetch: git fetch https://git.openjdk.org/jdk pull/10952/head:pull/10952 PR: https://git.openjdk.org/jdk/pull/10952
Re: RFR: 8236919: Refactor com.sun.tools.javac.main.CommandLine into a reusable module for other JDK tools [v2]
On Tue, 22 Nov 2022 15:57:57 GMT, Christian Stein wrote: >> This PR copies the `CommandLine.java` file from module `jdk.compiler` >> (package `com.sun.tools.javac.main`) into the `jdk.internal.opt` module, >> creating a new package with name `jdk.internal.opt`. That new >> `jdk.internal.opt` package is then exported to the following modules: >> - `jdk.jartool` >> - `jdk.jlink` >> - `jdk.jpackage` >> >> Now, `jar`, `jlink`, and `jpackage` use a shared `CommandLine` class. In a >> future commit (presumable for JDK 21) the original `CommandLine.java` file >> in `jdk.compiler` can and will be replaced with this new one in >> `jdk.internal.opt`. Same goes for the `jdk.javadoc` module. >> >> - [x] Keep `CommandLine.java` in `jdk.compiler` module for the time being >> due to "JDK N-1 rule". >> - [x] Keep `CommandLine.java` in `jdk.javadoc` module for the time being due >> to "JDK N-1 rule". >> - [x] Remove `CommandLine.java` from `jdk.jartool` module >> - [x] Remove `CommandLine.java` from `jdk.jlink` module >> - [x] Remove `CommandLine.java` from `jdk.jpackage` module >> - [x] Check for related but renamed(?) usages of `CommandLine.java` in other >> JDK tools: `jshell`, `jdeps`, `jfr`, ... > > Christian Stein has updated the pull request incrementally with one > additional commit since the last revision: > > Remove superseded comment Marked as reviewed by jjg (Reviewer). src/jdk.internal.opt/share/classes/jdk/internal/opt/CommandLine.java line 49: > 47: * > 48: * This is NOT part of any supported API. > 49: * If you write code that depends on this, you do so at your own > risk. I think this entire paragraph can be removed. It is "just" the "scary warning" meant to warn people about using not-really-public classes in javac, before we had a module system for the encapsulation. - PR: https://git.openjdk.org/jdk/pull/11272
Re: JDK 19 innocuous reaper threads
Hi Chris, Yes, adding a doPriv for setDaemon and setName in a couple of places makes sense. Thanks, Roger On 11/22/22 11:12 AM, Chris Hegarty wrote: Hi Alan, On 22/11/2022 16:08, Alan Bateman wrote: On 22/11/2022 15:21, Chris Hegarty wrote: .. Just to double check, does the ES security manager override checkAccess(Thread)? Yes. :-( That is usually a no-op but if overridden then it will expose an issue with the thread factory for the "process reaper" where it attempts changes the daemon status outside of a doPriv block. Right. That's exactly what we're running into. If there are no objections, then I'm happy to file an issue and PR to add narrow doPriv blocks around these calls. -Chris [1] https://github.com/elastic/elasticsearch/blob/main/libs/secure-sm/src/main/java/org/elasticsearch/secure_sm/SecureSM.java#L118
Re: RFR: JDK-8286666: JEP 429: Implementation of Scoped Values (Incubator) [v17]
> JEP 429 implementation. Andrew Haley has updated the pull request incrementally with two additional commits since the last revision: - Merge branch 'JDK-828' of https://github.com/theRealAph/jdk into JDK-828 - Fix bad merge. - Changes: - all: https://git.openjdk.org/jdk/pull/10952/files - new: https://git.openjdk.org/jdk/pull/10952/files/86ce5bbd..04320c7b Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=10952=16 - incr: https://webrevs.openjdk.org/?repo=jdk=10952=15-16 Stats: 1 line in 1 file changed: 1 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/10952.diff Fetch: git fetch https://git.openjdk.org/jdk pull/10952/head:pull/10952 PR: https://git.openjdk.org/jdk/pull/10952
Re: JDK 19 innocuous reaper threads
Hi Alan, On 22/11/2022 16:08, Alan Bateman wrote: On 22/11/2022 15:21, Chris Hegarty wrote: .. Just to double check, does the ES security manager override checkAccess(Thread)? Yes. :-( That is usually a no-op but if overridden then it will expose an issue with the thread factory for the "process reaper" where it attempts changes the daemon status outside of a doPriv block. Right. That's exactly what we're running into. If there are no objections, then I'm happy to file an issue and PR to add narrow doPriv blocks around these calls. -Chris [1] https://github.com/elastic/elasticsearch/blob/main/libs/secure-sm/src/main/java/org/elasticsearch/secure_sm/SecureSM.java#L118
Re: JDK 19 innocuous reaper threads
On 22/11/2022 15:21, Chris Hegarty wrote: Hi, A change in JDK 19, that changed process reaper threads to be innocuous [1], has had an adverse affect when terminating the Elasticsearch server [2]. I agree with changing the process reapers to be innocuous, but just wonder if we're missing a few doPriv blocks. Additionally, and also in JDK 19, is the welcome improvement of the pid in the reaper thread name [3], but again I wonder if we're missing a few doPriv blocks here too. The issues we're seeing arise from the calls to `setDaemon` and `setName` outside of doPriv blocks, which can (depending on your security manager implementation) result in checking the callers permissions, and its callers permissions, etc, all the way to the thread's inherited access control context - which is effectively empty for these threads, since they are innocuous. I don't remember where we ended up with these kinda calls for innocuous threads, I'm thinking specifically about `setDaemon` (but `setName` seems similar) - whether they should be in doPriv or not, given the default security manager implementation. My feeling is that they should, since the caller should never be required to hold any specific permissions for these specific operations [4][5][6] to succeed. Just to double check, does the ES security manager override checkAccess(Thread)? That is usually a no-op but if overridden then it will expose an issue with the thread factory for the "process reaper" where it attempts changes the daemon status outside of a doPriv block. -Alan
Re: RFR: 8236919: Refactor com.sun.tools.javac.main.CommandLine into a reusable module for other JDK tools [v2]
> This PR copies the `CommandLine.java` file from module `jdk.compiler` > (package `com.sun.tools.javac.main`) into the `jdk.internal.opt` module, > creating a new package with name `jdk.internal.opt`. That new > `jdk.internal.opt` package is then exported to the following modules: > - `jdk.jartool` > - `jdk.jlink` > - `jdk.jpackage` > > Now, `jar`, `jlink`, and `jpackage` use a shared `CommandLine` class. In a > future commit (presumable for JDK 21) the original `CommandLine.java` file > in `jdk.compiler` can and will be replaced with this new one in > `jdk.internal.opt`. Same goes for the `jdk.javadoc` module. > > - [x] Keep `CommandLine.java` in `jdk.compiler` module for the time being due > to "JDK N-1 rule". > - [x] Keep `CommandLine.java` in `jdk.javadoc` module for the time being due > to "JDK N-1 rule". > - [x] Remove `CommandLine.java` from `jdk.jartool` module > - [x] Remove `CommandLine.java` from `jdk.jlink` module > - [x] Remove `CommandLine.java` from `jdk.jpackage` module > - [x] Check for related but renamed(?) usages of `CommandLine.java` in other > JDK tools: `jshell`, `jdeps`, `jfr`, ... Christian Stein has updated the pull request incrementally with one additional commit since the last revision: Remove superseded comment - Changes: - all: https://git.openjdk.org/jdk/pull/11272/files - new: https://git.openjdk.org/jdk/pull/11272/files/ebdcbde7..7854e14c Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=11272=01 - incr: https://webrevs.openjdk.org/?repo=jdk=11272=00-01 Stats: 3 lines in 1 file changed: 0 ins; 2 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/11272.diff Fetch: git fetch https://git.openjdk.org/jdk pull/11272/head:pull/11272 PR: https://git.openjdk.org/jdk/pull/11272
Re: RFR: 8236919: Refactor com.sun.tools.javac.main.CommandLine into a reusable module for other JDK tools
On Mon, 21 Nov 2022 15:40:19 GMT, Christian Stein wrote:
> This PR copies the `CommandLine.java` file from module `jdk.compiler`
> (package `com.sun.tools.javac.main`) into the `jdk.internal.opt` module,
> creating a new package with name `jdk.internal.opt`. That new
> `jdk.internal.opt` package is then exported to the following modules:
> - `jdk.jartool`
> - `jdk.jlink`
> - `jdk.jpackage`
>
> Now, `jar`, `jlink`, and `jpackage` use a shared `CommandLine` class. In a
> future commit (presumable for JDK 21) the original `CommandLine.java` file
> in `jdk.compiler` can and will be replaced with this new one in
> `jdk.internal.opt`. Same goes for the `jdk.javadoc` module.
>
> - [x] Keep `CommandLine.java` in `jdk.compiler` module for the time being due
> to "JDK N-1 rule".
> - [x] Keep `CommandLine.java` in `jdk.javadoc` module for the time being due
> to "JDK N-1 rule".
> - [x] Remove `CommandLine.java` from `jdk.jartool` module
> - [x] Remove `CommandLine.java` from `jdk.jlink` module
> - [x] Remove `CommandLine.java` from `jdk.jpackage` module
> - [x] Check for related but renamed(?) usages of `CommandLine.java` in other
> JDK tools: `jshell`, `jdeps`, `jfr`, ...
Yes. I copied `CommandLine` class from module `jdk.compiler`. In addition to
update of the copyright year and the name of the package, I also added small
API changes that other tools developed in their copies of the class. Find a
diff attached below:
diff --git
a/open/src/jdk.compiler/share/classes/com/sun/tools/javac/main/CommandLine.java
b/open/src/jdk.internal.opt/share/classes/jdk/internal/opt/CommandLine.java
index ec6f711f9b..9e4b0c6fe2 100644
---
a/open/src/jdk.compiler/share/classes/com/sun/tools/javac/main/CommandLine.java
+++ b/open/src/jdk.internal.opt/share/classes/jdk/internal/opt/CommandLine.java
@@ -1,5 +1,5 @@
/*
- * Copyright (c) 1999, 2018, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 1999, 2022, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
@@ -23,17 +23,25 @@
* questions.
*/
-package com.sun.tools.javac.main;
+package jdk.internal.opt;
import java.io.IOException;
+import java.io.InputStream;
+import java.io.InputStreamReader;
import java.io.Reader;
import java.nio.charset.Charset;
import java.nio.file.Files;
import java.nio.file.Paths;
import java.util.ArrayList;
-import java.util.Arrays;
import java.util.List;
+/*
+ * This file was originally a copy of CommandLine.java in
+ * com.sun.tools.javac.main -- and it will be the last.
+ *
+ * Find details at https://bugs.openjdk.org/browse/JDK-8236919
+ */
+
/**
* Various utility methods for processing Java tool command line arguments.
*
@@ -42,7 +50,16 @@ import java.util.List;
* This code and its internal interfaces are subject to change or
* deletion without notice.
*/
-public class CommandLine {
+public final class CommandLine {
+/**
+ * Convenient wrapper for the {@code List}-based parse method.
+ *
+ * @see #parse(List)
+ */
+public static String[] parse(String... args) throws IOException {
+return parse(List.of(args)).toArray(String[]::new);
+}
+
/**
* Process Win32-style command files for the specified command line
* arguments and return the resulting arguments. A command file argument
@@ -91,7 +108,7 @@ public class CommandLine {
* @param args the arguments that may contain @files
* @return the arguments, with environment variable's content and
expansion of @files
* @throws IOException if there is a problem reading any of the @files
- * @throws com.sun.tools.javac.main.CommandLine.UnmatchedQuote
+ * @throws CommandLine.UnmatchedQuote
*/
public static List parse(String envVariable, List args)
throws IOException, UnmatchedQuote {
@@ -104,8 +121,18 @@ public class CommandLine {
return newArgs;
}
+public static void loadCmdFile(InputStream in, List args) throws
IOException {
+Reader reader = new InputStreamReader(in);
+loadCmdFileAndCloseReader(reader, args);
+}
+
private static void loadCmdFile(String name, List args) throws
IOException {
-try (Reader r = Files.newBufferedReader(Paths.get(name),
Charset.defaultCharset())) {
+Reader reader = Files.newBufferedReader(Paths.get(name),
Charset.defaultCharset());
+loadCmdFileAndCloseReader(reader, args);
+}
+
+private static void loadCmdFileAndCloseReader(Reader r, List args)
throws IOException {
+try (r) {
Tokenizer t = new Tokenizer(r);
String s;
while ((s = t.nextToken()) != null) {
-
PR: https://git.openjdk.org/jdk/pull/11272
JDK 19 innocuous reaper threads
Hi, A change in JDK 19, that changed process reaper threads to be innocuous [1], has had an adverse affect when terminating the Elasticsearch server [2]. I agree with changing the process reapers to be innocuous, but just wonder if we're missing a few doPriv blocks. Additionally, and also in JDK 19, is the welcome improvement of the pid in the reaper thread name [3], but again I wonder if we're missing a few doPriv blocks here too. The issues we're seeing arise from the calls to `setDaemon` and `setName` outside of doPriv blocks, which can (depending on your security manager implementation) result in checking the callers permissions, and its callers permissions, etc, all the way to the thread's inherited access control context - which is effectively empty for these threads, since they are innocuous. I don't remember where we ended up with these kinda calls for innocuous threads, I'm thinking specifically about `setDaemon` (but `setName` seems similar) - whether they should be in doPriv or not, given the default security manager implementation. My feeling is that they should, since the caller should never be required to hold any specific permissions for these specific operations [4][5][6] to succeed. -Chris. [1] https://bugs.openjdk.org/browse/JDK-8279488 [2] https://github.com/elastic/elasticsearch/issues/91650 [3] https://bugs.openjdk.org/browse/JDK-8284165 [4] https://github.com/openjdk/jdk/blob/master/src/java.base/share/classes/java/lang/ProcessHandleImpl.java#L103 [5] https://github.com/openjdk/jdk/blob/master/src/java.base/share/classes/java/lang/ProcessHandleImpl.java#LL144 [6] https://github.com/openjdk/jdk/blob/master/src/java.base/share/classes/java/lang/ProcessHandleImpl.java#LL175
Re: RFR: 8297385: Remove duplicated null typos in javadoc
On Tue, 15 Nov 2022 15:05:45 GMT, Dongxu Wang wrote: > 8297385: Remove duplicated null typos in javadoc The source of this PR is the "master" branch of your fork. Note the Comment from the bot at the top. The conventional usage is to create a branch specific to the change you are making and create the PR from that. You will run into trouble with git due to any commits in the master branch other than are pulled from the mainline. You can create the new branch from your current master and then reset the HEAD of the master back to match the mainline head. - PR: https://git.openjdk.org/jdk/pull/11169
Re: RFR: 8295044: Implementation of Foreign Function and Memory API (Second Preview) [v29]
> This PR contains the API and implementation changes for JEP-434 [1]. A more > detailed description of such changes, to avoid repetitions during the review > process, is included as a separate comment. > > [1] - https://openjdk.org/jeps/434 Maurizio Cimadamore has updated the pull request incrementally with one additional commit since the last revision: Fix wrong check in MemorySegment::spliterator/elements (The check which ensures that the segment size is multiple of spliterator element size is bogus) - Changes: - all: https://git.openjdk.org/jdk/pull/10872/files - new: https://git.openjdk.org/jdk/pull/10872/files/a0cee7b0..66dd888d Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=10872=28 - incr: https://webrevs.openjdk.org/?repo=jdk=10872=27-28 Stats: 29 lines in 2 files changed: 21 ins; 0 del; 8 mod Patch: https://git.openjdk.org/jdk/pull/10872.diff Fetch: git fetch https://git.openjdk.org/jdk pull/10872/head:pull/10872 PR: https://git.openjdk.org/jdk/pull/10872
Re: RFR: 8296024: Usage of DIrectBuffer::address should be guarded [v10]
> This PR proposes the introduction of **guarding** of the use of
> `DirectBuffer::address` within the JDK. With the introduction of the Foreign
> Function and Memory access, it is possible to derive Buffer instances that
> are backed by native memory that, in turn, can be closed asynchronously by
> the user (and not only via a `Cleaner` when there is no other reference to
> the `Buffer` object). If another thread is invoking `MemorySession::close`
> while a thread is doing work using raw addresses, the outcome is undefined.
> This means the JVM might crash or even worse, silent modification of
> unrelated memory might occur.
>
> Design choices in this PR:
>
> There is already a method `MemorySession::whileAlive` that takes a runnable
> and that will perform the provided action while acquiring the underlying`
> MemorySession` (if any). However, using this method entailed relatively large
> changes while converting larger/nested code segments into lambdas. This would
> also risk introducing lambda capturing. So, instead, a try-with-resources
> friendly access method was added. This made is more easy to add guarding and
> did not risk lambda capturing. Also, introducing lambdas in certain
> fundamental JDK classes might incur bootstrap problems.
>
> The aforementioned TwR is using a "session acquisition" that is not used
> explicitly in the try block itself. This raises a warning that is suppressed
> using `@SuppressWarnings("try")`. In the future, we might be able to remove
> these suppressions by using the reserved variable name `_`. Another
> alternative was evaluated where a non-autocloseable resource was used.
> However, it became relatively complicated to guarantee that the session was
> always released and, at the same time, the existing 'final` block was always
> executed properly (as they both might throw exceptions). In the end, the
> proposed solution was much simpler and robust despite requiring a
> non-referenced TwR variable.
>
> In some cases, where is is guaranteed that the backing memory session is
> non-closeable, we do not have to guard the use of `DirectBuffer::address`.
> ~~These cases have been documented in the code.~~
>
> On some occasions, a plurality of acquisitions are made. This would never
> lead to deadlocks as acquisitions are fundamentally concurrent counters and
> not resources that only one thread can "own".
>
> I have added comments (and not javadocs) before the declaration of the
> non-public-api `DirectBuffer::address` method, that the use of the returned
> address needs to be guarded. It can be discussed if this is preferable or not.
>
> This PR spawns several areas of responsibility and so, I expect more than one
> reviewer before promoting the PR.
Per Minborg has updated the pull request incrementally with one additional
commit since the last revision:
Remove redundant guard and fix comment permanently
-
Changes:
- all: https://git.openjdk.org/jdk/pull/11260/files
- new: https://git.openjdk.org/jdk/pull/11260/files/0526e3dc..06c764ca
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk=11260=09
- incr: https://webrevs.openjdk.org/?repo=jdk=11260=08-09
Stats: 59 lines in 2 files changed: 19 ins; 26 del; 14 mod
Patch: https://git.openjdk.org/jdk/pull/11260.diff
Fetch: git fetch https://git.openjdk.org/jdk pull/11260/head:pull/11260
PR: https://git.openjdk.org/jdk/pull/11260
Re: RFR: 8296024: Usage of DIrectBuffer::address should be guarded [v9]
On Tue, 22 Nov 2022 09:23:40 GMT, Maurizio Cimadamore
wrote:
>> Per Minborg has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Rework Acquisition
>
> src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java
> line 914:
>
>> 912: * If so, make a copy to put the dst data in.
>> 913: */
>> 914: @SuppressWarnings("try")
>
> After looking at the implementation some more, I'm not sure this need fixing?
> E.g. this method is just using the address to compute some overlap - and
> return a buffer sliced accordingly. There's no access to the buffer data
> (except for the last part which does a `put`). The access will fail if the
> session is closed from underneath. I don't think this can crash the VM (in
> fact this code did not have a reachability fence to begin with).
Well spotted. I will remove the guarding here.
-
PR: https://git.openjdk.org/jdk/pull/11260
Re: RFR: 8296024: Usage of DIrectBuffer::address should be guarded [v9]
On Tue, 22 Nov 2022 09:29:14 GMT, Maurizio Cimadamore
wrote:
>> Per Minborg has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Rework Acquisition
>
> src/java.base/share/classes/java/nio/Buffer.java line 827:
>
>> 825:
>> 826: @Override
>> 827: public Runnable acquireSessionOrNull(Buffer buffer,
>> boolean async) {
>
> If allocation/performance is an issue, a relatively straightforward way to
> adjust the code would be to let go of the TWR entirely. E.g. we have code
> that does this:
>
>
> Buffer b = ...
> try {
> // use buffer.address();
> } finally {
> Reference.reachabilityFence(b);
> }
>
>
> We could transform to this:
>
>
> Buffer b = ...
> acquire(b); // calls MemorySessionImpl.acquire0 if session is not null
> try {
> // use buffer.address();
> } finally {
> release(b); // calls MemorySessionImpl.release0 if session is not null
> (and maybe adds a reachability fence, just in case)
> }
>
> This leads to a simpler patch that is probably easier to validate. The
> remaining IOUtil code will be using a different idiom, and perhaps we can, at
> some point, go back and make that consistent too.
The AutoCloseable experiment was interesting to try but I think you are right,
acquire try { .. } finally release would be simpler and also remove the
concerns about the performance critical code paths.
-
PR: https://git.openjdk.org/jdk/pull/11260
Re: RFR: 8296024: Usage of DIrectBuffer::address should be guarded [v9]
On Tue, 22 Nov 2022 09:38:35 GMT, Maurizio Cimadamore
wrote:
>> src/jdk.sctp/unix/classes/sun/nio/ch/sctp/SctpMultiChannelImpl.java line 590:
>>
>>> 588: int pos)
>>> 589: throws IOException {
>>> 590: try (var guard = NIO_ACCESS.acquireScope(bb)) {
>>
>> Why was the old code not using reachability fences? Bug or feature?
>
> I see that there's a subsequent buffer call if `n > 0`, so that's probably
> why the fence was skipped? (I also assume that the code calling this method
> will access the buffer before/after, so reachability is never truly an issue
> - but for session-backed buffers this needs fixing).
>
> Also, stepping back, I note how, if `receive0` was a native call using
> Linker, perhaps we wouldn't need all this manual address computation - we'd
> just get a memory segment slice from the buffer and pass that to the handle
> (which will perform the correct liveness check). E.g. maybe a better long
> term solution would be to panama-ize this code?
Yes, once the memory/linker APIs are permanent then the SCTP implementation
would be a good candidate to redo.
-
PR: https://git.openjdk.org/jdk/pull/11260
Re: RFR: 8295857: Clarify that cleanup code can be skipped when the JVM terminates (e.g. when calling halt()) [v4]
On Tue, 22 Nov 2022 00:50:51 GMT, Brent Christian wrote: >> [JDK-8290036](https://bugs.openjdk.org/browse/JDK-8290036) documented the >> shutdown sequence, noting that calling Runtime.halt() skips the shutdown >> sequence and immediately terminates the VM. Thus, "threads' current methods >> do not complete normally or abruptly; no finally clause of any method is >> executed". >> >> One ramification of this is that resources within try-with-resource blocks >> will not be released. It would be good to state this explicitly. > > Brent Christian has updated the pull request incrementally with one > additional commit since the last revision: > > Update Runtime class doc re: other unexpected behaviors Marked as reviewed by lancea (Reviewer). - PR: https://git.openjdk.org/jdk/pull/11218
Re: RFR: 8296024: Usage of DIrectBuffer::address should be guarded [v9]
On Tue, 22 Nov 2022 09:11:44 GMT, Per Minborg wrote:
>> This PR proposes the introduction of **guarding** of the use of
>> `DirectBuffer::address` within the JDK. With the introduction of the Foreign
>> Function and Memory access, it is possible to derive Buffer instances that
>> are backed by native memory that, in turn, can be closed asynchronously by
>> the user (and not only via a `Cleaner` when there is no other reference to
>> the `Buffer` object). If another thread is invoking `MemorySession::close`
>> while a thread is doing work using raw addresses, the outcome is undefined.
>> This means the JVM might crash or even worse, silent modification of
>> unrelated memory might occur.
>>
>> Design choices in this PR:
>>
>> There is already a method `MemorySession::whileAlive` that takes a runnable
>> and that will perform the provided action while acquiring the underlying`
>> MemorySession` (if any). However, using this method entailed relatively
>> large changes while converting larger/nested code segments into lambdas.
>> This would also risk introducing lambda capturing. So, instead, a
>> try-with-resources friendly access method was added. This made is more easy
>> to add guarding and did not risk lambda capturing. Also, introducing lambdas
>> in certain fundamental JDK classes might incur bootstrap problems.
>>
>> The aforementioned TwR is using a "session acquisition" that is not used
>> explicitly in the try block itself. This raises a warning that is suppressed
>> using `@SuppressWarnings("try")`. In the future, we might be able to remove
>> these suppressions by using the reserved variable name `_`. Another
>> alternative was evaluated where a non-autocloseable resource was used.
>> However, it became relatively complicated to guarantee that the session was
>> always released and, at the same time, the existing 'final` block was always
>> executed properly (as they both might throw exceptions). In the end, the
>> proposed solution was much simpler and robust despite requiring a
>> non-referenced TwR variable.
>>
>> In some cases, where is is guaranteed that the backing memory session is
>> non-closeable, we do not have to guard the use of `DirectBuffer::address`.
>> ~~These cases have been documented in the code.~~
>>
>> On some occasions, a plurality of acquisitions are made. This would never
>> lead to deadlocks as acquisitions are fundamentally concurrent counters and
>> not resources that only one thread can "own".
>>
>> I have added comments (and not javadocs) before the declaration of the
>> non-public-api `DirectBuffer::address` method, that the use of the returned
>> address needs to be guarded. It can be discussed if this is preferable or
>> not.
>>
>> This PR spawns several areas of responsibility and so, I expect more than
>> one reviewer before promoting the PR.
>
> Per Minborg has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Rework Acquisition
src/java.base/share/classes/sun/nio/ch/DirectBuffer.java line 43:
> 41: // try (var guard = NIO_ACCESS.acquireSession(bb)) {
> 42: // performOperation(bb.address());
> 43: // }
Again, updating this comment was missed:
Suggestion:
// try (var guard = NIO_ACCESS.acquireScope(bb)) {
// performOperation(guard.address());
// }
-
PR: https://git.openjdk.org/jdk/pull/11260
Re: RFR: 8296024: Usage of DIrectBuffer::address should be guarded [v9]
On Tue, 22 Nov 2022 09:32:32 GMT, Maurizio Cimadamore
wrote:
>> Per Minborg has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Rework Acquisition
>
> src/jdk.sctp/unix/classes/sun/nio/ch/sctp/SctpMultiChannelImpl.java line 590:
>
>> 588: int pos)
>> 589: throws IOException {
>> 590: try (var guard = NIO_ACCESS.acquireScope(bb)) {
>
> Why was the old code not using reachability fences? Bug or feature?
I see that there's a subsequent buffer call if `n > 0`, so that's probably why
the fence was skipped? (I also assume that the code calling this method will
access the buffer before/after, so reachability is never truly an issue - but
for session-backed buffers this needs fixing).
Also, stepping back, I note how, if `receive0` was a native call using Linker,
perhaps we wouldn't need all this manual address computation - we'd just get a
memory segment slice from the buffer and pass that to the handle (which will
perform the correct liveness check). E.g. maybe a better long term solution
would be to panama-ize this code?
-
PR: https://git.openjdk.org/jdk/pull/11260
Re: RFR: 8296024: Usage of DIrectBuffer::address should be guarded [v9]
On Tue, 22 Nov 2022 09:11:44 GMT, Per Minborg wrote:
>> This PR proposes the introduction of **guarding** of the use of
>> `DirectBuffer::address` within the JDK. With the introduction of the Foreign
>> Function and Memory access, it is possible to derive Buffer instances that
>> are backed by native memory that, in turn, can be closed asynchronously by
>> the user (and not only via a `Cleaner` when there is no other reference to
>> the `Buffer` object). If another thread is invoking `MemorySession::close`
>> while a thread is doing work using raw addresses, the outcome is undefined.
>> This means the JVM might crash or even worse, silent modification of
>> unrelated memory might occur.
>>
>> Design choices in this PR:
>>
>> There is already a method `MemorySession::whileAlive` that takes a runnable
>> and that will perform the provided action while acquiring the underlying`
>> MemorySession` (if any). However, using this method entailed relatively
>> large changes while converting larger/nested code segments into lambdas.
>> This would also risk introducing lambda capturing. So, instead, a
>> try-with-resources friendly access method was added. This made is more easy
>> to add guarding and did not risk lambda capturing. Also, introducing lambdas
>> in certain fundamental JDK classes might incur bootstrap problems.
>>
>> The aforementioned TwR is using a "session acquisition" that is not used
>> explicitly in the try block itself. This raises a warning that is suppressed
>> using `@SuppressWarnings("try")`. In the future, we might be able to remove
>> these suppressions by using the reserved variable name `_`. Another
>> alternative was evaluated where a non-autocloseable resource was used.
>> However, it became relatively complicated to guarantee that the session was
>> always released and, at the same time, the existing 'final` block was always
>> executed properly (as they both might throw exceptions). In the end, the
>> proposed solution was much simpler and robust despite requiring a
>> non-referenced TwR variable.
>>
>> In some cases, where is is guaranteed that the backing memory session is
>> non-closeable, we do not have to guard the use of `DirectBuffer::address`.
>> ~~These cases have been documented in the code.~~
>>
>> On some occasions, a plurality of acquisitions are made. This would never
>> lead to deadlocks as acquisitions are fundamentally concurrent counters and
>> not resources that only one thread can "own".
>>
>> I have added comments (and not javadocs) before the declaration of the
>> non-public-api `DirectBuffer::address` method, that the use of the returned
>> address needs to be guarded. It can be discussed if this is preferable or
>> not.
>>
>> This PR spawns several areas of responsibility and so, I expect more than
>> one reviewer before promoting the PR.
>
> Per Minborg has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Rework Acquisition
src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java line
914:
> 912: * If so, make a copy to put the dst data in.
> 913: */
> 914: @SuppressWarnings("try")
After looking at the implementation some more, I'm not sure this need fixing?
E.g. this method is just using the address to compute some overlap - and return
a buffer sliced accordingly. There's no access to the buffer data (except for
the last part which does a `put`). The access will fail if the session is
closed from underneath. I don't think this can crash the VM (in fact this code
did not have a reachability fence to begin with).
src/java.base/share/classes/java/nio/Buffer.java line 827:
> 825:
> 826: @Override
> 827: public Runnable acquireSessionOrNull(Buffer buffer,
> boolean async) {
If allocation/performance is an issue, a relatively straightforward way to
adjust the code would be to let go of the TWR entirely. E.g. we have code that
does this:
Buffer b = ...
try {
// use buffer.address();
} finally {
Reference.reachabilityFence(b);
}
We could transform to this:
Buffer b = ...
acquire(b); // calls MemorySessionImpl.acquire0 if session is not null
try {
// use buffer.address();
} finally {
release(b); // calls MemorySessionImpl.release0 if session is not null (and
maybe adds a reachability fence, just in case)
}
This leads to a simpler patch that is probably easier to validate. The
remaining IOUtil code will be using a different idiom, and perhaps we can, at
some point, go back and make that consistent too.
src/jdk.sctp/unix/classes/sun/nio/ch/sctp/SctpMultiChannelImpl.java line 590:
> 588: int pos)
> 589: throws IOException {
> 590: try (var guard = NIO_ACCESS.acquireScope(bb)) {
Why was the old code not using reachability fences? Bug or feature?
-
PR:
Re: RFR: 8296024: Usage of DIrectBuffer::address should be guarded [v6]
On Mon, 21 Nov 2022 15:29:11 GMT, Per Minborg wrote: > That is clear to me but I am trying to prevent future redundant guarding. > Anyway, I will remove the comments. The faraway use sites look much better now. The performance sensitive usages need close attention. Can you summarise the changes to DatagramChannel, are you creating a NoOpScopeAcquisition for every send/receive? This is low level code where we do do something different to avoid the try-with-resources. - PR: https://git.openjdk.org/jdk/pull/11260
Re: RFR: 8297385: Remove duplicated null typos in javadoc
On Tue, 22 Nov 2022 03:33:55 GMT, Yi Yang wrote: > > > good catch, do you need a JBS issue for this? > > > > > > Thank you if you can help with that. > > I filed https://bugs.openjdk.org/browse/JDK-8297385 for this, you can change > your PR title and commit message to [8297385: Remove duplicated null typo in > javadoc](https://bugs.openjdk.org/browse/JDK-8297385), OpenJDK robot will > guide you remaining processes. Thank you, can you also help review - PR: https://git.openjdk.org/jdk/pull/11169
Re: RFR: 8296024: Usage of DIrectBuffer::address should be guarded [v4]
On Mon, 21 Nov 2022 20:06:20 GMT, Alan Bateman wrote: >>> > Although this looks much simpler and concise, it means "a new object is >>> > created for each invocation" >>> >>> My comment was actually to see if DirectBuffer could extend AutoCloseable >>> so that the acquire returns "this" for the non-session case. Doing the >>> equivalent for the session case might leak into MemorySessionImpl >>> implementing DirectBuffer which you probably don't want to do. If >>> NOP_CLOSE.close can do the Reference.reachabilityFence(this) then it would >>> at least improve some of the use-sites. >> >> Not sure that is simpler. ByteBuffer <: AutoCloseable doesn't seem to make >> sense to me. I'm also not sure how much object allocation (all this stuff >> will become value types) should be the driving factor in these code paths. > > Right, I didn't mean BB <: AC but to see if we avoid needing to wrap it > because this PR is touching several low level and performance critical code > paths. For the faraway places then having the close do a > Reference.reachabilityFence(this) should avoid the surprise that the buffer > has to kept alive even though it appears that the try-with-resources is doing > it already. I have reworked Acquisition. I think we could merge the old and new way in a separate PR. - PR: https://git.openjdk.org/jdk/pull/11260
Re: RFR: 8296024: Usage of DIrectBuffer::address should be guarded [v9]
> This PR proposes the introduction of **guarding** of the use of
> `DirectBuffer::address` within the JDK. With the introduction of the Foreign
> Function and Memory access, it is possible to derive Buffer instances that
> are backed by native memory that, in turn, can be closed asynchronously by
> the user (and not only via a `Cleaner` when there is no other reference to
> the `Buffer` object). If another thread is invoking `MemorySession::close`
> while a thread is doing work using raw addresses, the outcome is undefined.
> This means the JVM might crash or even worse, silent modification of
> unrelated memory might occur.
>
> Design choices in this PR:
>
> There is already a method `MemorySession::whileAlive` that takes a runnable
> and that will perform the provided action while acquiring the underlying`
> MemorySession` (if any). However, using this method entailed relatively large
> changes while converting larger/nested code segments into lambdas. This would
> also risk introducing lambda capturing. So, instead, a try-with-resources
> friendly access method was added. This made is more easy to add guarding and
> did not risk lambda capturing. Also, introducing lambdas in certain
> fundamental JDK classes might incur bootstrap problems.
>
> The aforementioned TwR is using a "session acquisition" that is not used
> explicitly in the try block itself. This raises a warning that is suppressed
> using `@SuppressWarnings("try")`. In the future, we might be able to remove
> these suppressions by using the reserved variable name `_`. Another
> alternative was evaluated where a non-autocloseable resource was used.
> However, it became relatively complicated to guarantee that the session was
> always released and, at the same time, the existing 'final` block was always
> executed properly (as they both might throw exceptions). In the end, the
> proposed solution was much simpler and robust despite requiring a
> non-referenced TwR variable.
>
> In some cases, where is is guaranteed that the backing memory session is
> non-closeable, we do not have to guard the use of `DirectBuffer::address`.
> ~~These cases have been documented in the code.~~
>
> On some occasions, a plurality of acquisitions are made. This would never
> lead to deadlocks as acquisitions are fundamentally concurrent counters and
> not resources that only one thread can "own".
>
> I have added comments (and not javadocs) before the declaration of the
> non-public-api `DirectBuffer::address` method, that the use of the returned
> address needs to be guarded. It can be discussed if this is preferable or not.
>
> This PR spawns several areas of responsibility and so, I expect more than one
> reviewer before promoting the PR.
Per Minborg has updated the pull request incrementally with one additional
commit since the last revision:
Rework Acquisition
-
Changes:
- all: https://git.openjdk.org/jdk/pull/11260/files
- new: https://git.openjdk.org/jdk/pull/11260/files/88ed3aa2..0526e3dc
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk=11260=08
- incr: https://webrevs.openjdk.org/?repo=jdk=11260=07-08
Stats: 249 lines in 19 files changed: 66 ins; 91 del; 92 mod
Patch: https://git.openjdk.org/jdk/pull/11260.diff
Fetch: git fetch https://git.openjdk.org/jdk pull/11260/head:pull/11260
PR: https://git.openjdk.org/jdk/pull/11260
