Re: RFR: 8284161: Implementation of Virtual Threads (Preview) [v8]

[Dean Long]

On Wed, 27 Apr 2022 14:24:20 GMT, Alan Bateman  wrote:

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

The compiler changes look good to me.

Marked as reviewed by dlong (Reviewer).

-

Marked as reviewed by dlong (Reviewer).

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

Re: RFR: 8284161: Implementation of Virtual Threads (Preview) [v8]

[Dean Long]

On Wed, 27 Apr 2022 14:24:20 GMT, Alan Bateman  wrote:

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

Marked as reviewed by dlong (Reviewer).

-

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

Re: [jdk18] RFR: 8281713: [BACKOUT] AArch64: Implement string_compare intrinsic in SVE

[Dean Long]

On Mon, 14 Feb 2022 10:30:09 GMT, Tobias Hartmann  wrote:

> We observed several failures on macosx aarch64 after integration of 
> [JDK-8275448](https://bugs.openjdk.java.net/browse/JDK-8275448). I could 
> reliably reproduce 
> [JDK-8281512](https://bugs.openjdk.java.net/browse/JDK-8281512) with JDK 18 
> b25-1665 (the first build with 
> [JDK-8275448](https://bugs.openjdk.java.net/browse/JDK-8275448) containing no 
> other changes) but **not** with JDK 18 b25-1664 (the build just before 
> integration). Also, I could reliably reproduce the issue with mainline but 
> **not** with the string compare intrinsic disabled. I think this is enough 
> evidence to prove that 
> [JDK-8275448](https://bugs.openjdk.java.net/browse/JDK-8275448) is the root 
> cause of the failures.
> 
> Given that we don't fully understand which part of this fix is causing the 
> issue and how (it might be a side effect that triggers a bug in the build 
> toolchain or adlc), I propose to backout the change. The backout applies 
> cleanly, approval is pending.
> 
> Thanks,
> Tobias

Marked as reviewed by dlong (Reviewer).

-

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

Re: RFR: 8278518: String(byte[], int, int, Charset) constructor and String.translateEscapes() miss bounds check elimination

[Dean Long]

On Mon, 13 Dec 2021 09:39:55 GMT, Сергей Цыпанов  wrote:

> Originally this was spotted by by Amir Hadadi in 
> https://stackoverflow.com/questions/70272651/missing-bounds-checking-elimination-in-string-constructor
> 
> It looks like in the following code in `String(byte[], int, int, Charset)`
> 
> while (offset < sl) {
> int b1 = bytes[offset];
> if (b1 >= 0) {
> dst[dp++] = (byte)b1;
> offset++;  // <---
> continue;
> }
> if ((b1 == (byte)0xc2 || b1 == (byte)0xc3) &&
> offset + 1 < sl) {
> int b2 = bytes[offset + 1];
> if (!isNotContinuation(b2)) {
> dst[dp++] = (byte)decode2(b1, b2);
> offset += 2;
> continue;
> }
> }
> // anything not a latin1, including the repl
> // we have to go with the utf16
> break;
> }
> 
> bounds check elimination is not executed when accessing byte array via 
> `bytes[offset].
> 
> The reason, I guess, is that offset variable is modified within the loop 
> (marked with arrow).
> 
> Possible fix for this could be changing:
> 
> `while (offset < sl)` ---> `while (offset >= 0 && offset < sl)`
> 
> However the best is to invest in C2 optimization to handle all such cases.
> 
> The following benchmark demonstrates good improvement:
> 
> @State(Scope.Thread)
> @BenchmarkMode(Mode.AverageTime)
> @OutputTimeUnit(TimeUnit.NANOSECONDS)
> public class StringConstructorBenchmark {
>   private byte[] array;
>   private String str;
> 
>   @Setup
>   public void setup() {
> str = "Quizdeltagerne spiste jordbær med fløde, mens cirkusklovnen. 
> Я";//Latin1 ending with Russian
> array = str.getBytes(StandardCharsets.UTF_8);
>   }
> 
>   @Benchmark
>   public String newString()  {
>   return new String(array, 0, array.length, StandardCharsets.UTF_8);
>   }
> 
>   @Benchmark
>   public String translateEscapes()  {
>   return str.translateEscapes();
>   }
> }
> 
> Results:
> 
> //baseline
> Benchmark Mode Cnt Score Error Units
> StringConstructorBenchmark.newString avgt 50 173,092 ± 3,048 ns/op
> 
> //patched
> Benchmark Mode Cnt Score Error Units
> StringConstructorBenchmark.newString avgt 50 126,908 ± 2,355 ns/op
> 
> The same is observed in String.translateEscapes() for the same String as in 
> the benchmark above:
> 
> //baseline
> Benchmark Mode Cnt Score Error Units
> StringConstructorBenchmark.translateEscapes avgt 100 53,627 ± 0,850 ns/op
> 
> //patched
> Benchmark Mode Cnt Score Error Units
> StringConstructorBenchmark.translateEscapes avgt 100 48,087 ± 1,129 ns/op
> 
> Also I've looked into this with `LinuxPerfAsmProfiler`, full output for 
> baseline is available here 
> https://gist.github.com/stsypanov/d2524f98477d633fb1d4a2510fedeea6 and for 
> patched code here 
> https://gist.github.com/stsypanov/16c787e4f9fa3dd122522f16331b68b7.
> 
> Here's the part of baseline assembly responsible for `while` loop: 
> 
>   3.62%   │││  0x7fed70eb4c1c:   mov%ebx,%ecx
>   2.29%   │││  0x7fed70eb4c1e:   mov%edx,%r9d
>   2.22%   │││  0x7fed70eb4c21:   mov(%rsp),%r8
>;*iload_2 {reexecute=0 rethrow=0 return_oop=0}
>   │││ 
>; - java.lang.String::init@107 (line 537)
>   2.32%   ↘││  0x7fed70eb4c25:   cmp%r13d,%ecx
>││  0x7fed70eb4c28:   jge
> 0x7fed70eb5388   ;*if_icmpge {reexecute=0 rethrow=0 return_oop=0}
>││ 
>; - java.lang.String::init@110 (line 537)
>   3.05%││  0x7fed70eb4c2e:   cmp0x8(%rsp),%ecx
>││  0x7fed70eb4c32:   jae
> 0x7fed70eb5319
>   2.38%││  0x7fed70eb4c38:   mov%r8,(%rsp)
>   2.64%││  0x7fed70eb4c3c:   movslq %ecx,%r8
>   2.46%││  0x7fed70eb4c3f:   mov%rax,%rbx
>   3.44%││  0x7fed70eb4c42:   sub%r8,%rbx
>   2.62%││  0x7fed70eb4c45:   add$0x1,%rbx
>   2.64%││  0x7fed70eb4c49:   and
> $0xfffe,%rbx
>   2.30%││  0x7fed70eb4c4d:   mov%ebx,%r8d
>   3.08%││  0x7fed70eb4c50:   add%ecx,%r8d
>   2.55%││  0x7fed70eb4c53:   movslq %r8d,%r8
>   2.45%││  0x7fed70eb4c56:   add
> $0xfffe,%r8
>   2.13%││  0x7fed70eb4c5a:   cmp(%rsp),%r8
>││  0x7fed70eb4c5e:   jae
> 0x7fed70eb5319
>   3.36%││  0x7fed70eb4c64:   mov%ecx,%edi 
>;*aload_1 

Re: [jdk18] RFR: 8277964: ClassCastException with no stack trace is thrown with -Xcomp in method handle invocation

[Dean Long]

On Wed, 15 Dec 2021 05:59:45 GMT, Vladimir Kozlov  wrote:

> A proper fix for this is to use the catchException combination. However, that 
> introduces significant cold startup performance regression. JDK-8278447 
> tracks the work to address the performance regression using catchException 
> and asSpreader combinator. It may require significant work and refactoring 
> which is risky for JDK 18. 
> 
> It is proposed to implement a workaround in C2 to white list the relevant 
> methods (all methods in sun.invoke.util.ValueConversions class) not to omit 
> stack trace when exception is thrown in them.
> 
> Added new regression test. Tested tier1-3.

Marked as reviewed by dlong (Reviewer).

-

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

Re: RFR: 8264805: Remove the experimental Ahead-of-Time Compiler [v4]

[Dean Long]

On Fri, 9 Apr 2021 16:54:51 GMT, Ioi Lam  wrote:

>> Vladimir Kozlov has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   Remove exports from Graal module to jdk.aot
>
> LGTM. Just a small nit.

@iklam
I thought the fingerprint code was also used by CDS.

-

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

Re: RFR: 8255150: Add utility methods to check long indexes and ranges [v7]

[Dean Long]

On Tue, 17 Nov 2020 08:33:20 GMT, Roland Westrelin  wrote:

>> This change add 3 new methods in Objects:
>> 
>> public static long checkIndex(long index, long length)
>> public static long checkFromToIndex(long fromIndex, long toIndex, long 
>> length)
>> public static long checkFromIndexSize(long fromIndex, long size, long length)
>> 
>> This mirrors the int utility methods that were added by JDK-8135248
>> with the same motivations.
>> 
>> As is the case with the int checkIndex(), the long checkIndex() method
>> is JIT compiled as an intrinsic. It allows the JIT to compile
>> checkIndex to an unsigned comparison and properly recognize it as
>> a range check that then becomes a candidate for the existing range check
>> optimizations. This has proven to be important for panama's
>> MemorySegment API and a prototype of this change (with some extra c2
>> improvements) showed that panama micro benchmark results improve
>> significantly.
>> 
>> This change includes:
>> 
>> - the API change
>> - the C2 intrinsic
>> - tests for the API and the C2 intrinsic
>> 
>> This is a joint work with Paul who reviewed and reworked the API change
>> and filled the CSR.
>
> Roland Westrelin has updated the pull request with a new target base due to a 
> merge or a rebase. The pull request now contains 27 commits:
> 
>  - Merge branch 'master' of https://git.openjdk.java.net/jdk into JDK-8255150
>  - Merge branch 'master' into JDK-8255150
>  - Merge branch 'master' into JDK-8255150
>  - Merge branch 'master' of https://git.openjdk.java.net/jdk into JDK-8255150
>  - exclude compiler test when run with -Xcomp
>  - CastLL should define carry_depency
>  - intrinsic comments
>  - Jorn's comments
>  - Update headers and add intrinsic to Graal test ignore list
>  - move compiler test and add bug to test
>  - ... and 17 more: 
> https://git.openjdk.java.net/jdk/compare/4553fa0b...feb32943

Marked as reviewed by dlong (Reviewer).

-

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

Re: RFR: 8255150: Add utility methods to check long indexes and ranges [v2]

[Dean Long]

On Thu, 5 Nov 2020 23:58:21 GMT, Dean Long  wrote:

>> Roland Westrelin has updated the pull request with a new target base due to 
>> a merge or a rebase. The pull request now contains ten commits:
>> 
>>  - Jorn's comments
>>  - Update headers and add intrinsic to Graal test ignore list
>>  - move compiler test and add bug to test
>>  - non x86_64 arch support
>>  - c2 test case
>>  - intrinsic
>>  - Use overloads of method names.
>>
>>Simplify internally to avoid overload resolution
>>issues, leverging List for the exception
>>mapper.
>>  - Vladimir's comments
>>  - checkLongIndex
>
> src/hotspot/share/opto/library_call.cpp line 1015:
> 
>> 1013:   Deoptimization::Action_make_not_entrant);
>> 1014:   }
>> 1015: 
> 
> A comment here explaining what the code below is doing would be helpful.

This code wasn't here before, so I'm guessing it's needed for T_LONG.  For 
T_INT is it just wasted work?

-

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

Re: RFR: 8255150: Add utility methods to check long indexes and ranges [v2]

[Dean Long]

On Thu, 5 Nov 2020 15:47:31 GMT, Roland Westrelin  wrote:

>> This change add 3 new methods in Objects:
>> 
>> public static long checkIndex(long index, long length)
>> public static long checkFromToIndex(long fromIndex, long toIndex, long 
>> length)
>> public static long checkFromIndexSize(long fromIndex, long size, long length)
>> 
>> This mirrors the int utility methods that were added by JDK-8135248
>> with the same motivations.
>> 
>> As is the case with the int checkIndex(), the long checkIndex() method
>> is JIT compiled as an intrinsic. It allows the JIT to compile
>> checkIndex to an unsigned comparison and properly recognize it as
>> a range check that then becomes a candidate for the existing range check
>> optimizations. This has proven to be important for panama's
>> MemorySegment API and a prototype of this change (with some extra c2
>> improvements) showed that panama micro benchmark results improve
>> significantly.
>> 
>> This change includes:
>> 
>> - the API change
>> - the C2 intrinsic
>> - tests for the API and the C2 intrinsic
>> 
>> This is a joint work with Paul who reviewed and reworked the API change
>> and filled the CSR.
>
> Roland Westrelin has updated the pull request with a new target base due to a 
> merge or a rebase. The pull request now contains ten commits:
> 
>  - Jorn's comments
>  - Update headers and add intrinsic to Graal test ignore list
>  - move compiler test and add bug to test
>  - non x86_64 arch support
>  - c2 test case
>  - intrinsic
>  - Use overloads of method names.
>
>Simplify internally to avoid overload resolution
>issues, leverging List for the exception
>mapper.
>  - Vladimir's comments
>  - checkLongIndex

C2 changes look good, except for new code block in 
inline_preconditions_checkIndex could use a comment.

src/hotspot/share/opto/library_call.cpp line 1015:

> 1013:   Deoptimization::Action_make_not_entrant);
> 1014:   }
> 1015: 

A comment here explaining what the code below is doing would be helpful.

-

Changes requested by dlong (Reviewer).

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

Re: Review Request: 8238358: Implementation of JEP 371: Hidden Classes

[Dean Long]

I looked at the AOT, C2, and JVMCI changes and I didn't find any issues.

dl

On 3/26/20 4:57 PM, Mandy Chung wrote:
Please review the implementation of JEP 371: Hidden Classes. The main 
changes are in core-libs and hotspot runtime area. Small changes are 
made in javac, VM compiler (intrinsification of Class::isHiddenClass), 
JFR, JDI, and jcmd.  CSR [1]has been reviewed and is in the finalized 
state (see specdiff and javadoc below for reference).


Webrev:
http://cr.openjdk.java.net/~mchung/valhalla/webrevs/hidden-classes/webrev.03

Hidden class is created via `Lookup::defineHiddenClass`. From JVM's point
of view, a hidden class is a normal class except the following:

- A hidden class has no initiating class loader and is not registered 
in any dictionary.
- A hidden class has a name containing an illegal character 
`Class::getName` returns `p.Foo/0x1234` whereas `GetClassSignature` 
returns "Lp/Foo.0x1234;".
- A hidden class is not modifiable, i.e. cannot be redefined or 
retransformed. JVM TI IsModifableClass returns false on a hidden.
- Final fields in a hidden class is "final".  The value of final 
fields cannot be overriden via reflection.  setAccessible(true) can 
still be called on reflected objects representing final fields in a 
hidden class and its access check will be suppressed but only have 
read-access (i.e. can do Field::getXXX but not setXXX).


Brief summary of this patch:

1. A new Lookup::defineHiddenClass method is the API to create a 
hidden class.
2. A new Lookup.ClassOption enum class defines NESTMATE and STRONG 
option that

   can be specified when creating a hidden class.
3. A new Class::isHiddenClass method tests if a class is a hidden class.
4. Field::setXXX method will throw IAE on a final field of a hidden class
   regardless of the value of the accessible flag.
5. JVM_LookupDefineClass is the new JVM entry point for 
Lookup::defineClass

   and defineHiddenClass to create a class from the given bytes.
6. ClassLoaderData implementation is not changed.  There is one 
primary CLD
   that holds the classes strongly referenced by its defining loader.  
There

   can be zero or more additional CLDs - one per weak class.
7. Nest host determination is updated per revised JVMS 5.4.4. Access 
control

   check no longer throws LinkageError but instead it will throw IAE with
   a clear message if a class fails to resolve/validate the nest host 
declared

   in NestHost/NestMembers attribute.
8. JFR, jcmd, JDI are updated to support hidden classes.
9. update javac LambdaToMethod as lambda proxy starts using nestmates
   and generate a bridge method to desuger a method reference to a 
protected

   method in its supertype in a different package

This patch also updates StringConcatFactory, LambdaMetaFactory, and 
LambdaForms
to use hidden classes.  The webrev includes changes in nashorn to 
hidden class

and I will update the webrev if JEP 372 removes it any time soon.

We uncovered a bug in Lookup::defineClass spec throws LinkageError and 
intends

to have the newly created class linked.  However, the implementation in 14
does not link the class.  A separate CSR [2] proposes to update the
implementation to match the spec.  This patch fixes the implementation.

The spec update on JVM TI, JDI and Instrumentation will be done as
a separate RFE [3].  This patch includes new tests for JVM TI and
java.instrument that validates how the existing APIs work for hidden 
classes.


javadoc/specdiff
http://cr.openjdk.java.net/~mchung/valhalla/webrevs/hidden-classes/api/
http://cr.openjdk.java.net/~mchung/valhalla/webrevs/hidden-classes/specdiff/

JVMS 5.4.4 change:
http://cr.openjdk.java.net/~mchung/valhalla/webrevs/hidden-classes/Draft-JVMS-HiddenClasses.pdf

CSR:
https://bugs.openjdk.java.net/browse/JDK-8238359

Thanks
Mandy
[1] https://bugs.openjdk.java.net/browse/JDK-8238359
[2] https://bugs.openjdk.java.net/browse/JDK-8240338
[3] https://bugs.openjdk.java.net/browse/JDK-8230502

Re: RFR: 8221723: Avoid storing zero to String.hash

[dean . long]

OK, I guess there's no ideal solution.  Adding a separate "not-computed" 
boolean adds space, and using a different sentinel value for 
"not-computed" would probably be slower on CPUs that have a fast 
compare-and-branch-against-zero instruction.


dl

On 4/1/19 12:55 PM, Martin Buchholz wrote:

The spec says that "".hashCode() must be 0.
https://docs.oracle.com/en/java/javase/11/docs/api/java.base/java/lang/String.html#hashCode()


On Mon, Apr 1, 2019 at 12:51 PM > wrote:


Wouldn't it be better to write a non-0 value when the computed
hash code
is 0, so we don't have to recompute it?  Is there some advantage to
writing 0 instead of any other value, such as 1?

dl

On 4/1/19 4:57 AM, Claes Redestad wrote:
> Hi,
>
> when a String has a calculated hash code value of 0, we
recalculate and
> store a 0 to the String.hash field every time (except for the empty
> String, which is special cased). To make String objects more
amenable to
> storage in shared read-only memory, e.g., CDS archives, we
should avoid
> this redundant store.
>
> Bug: https://bugs.openjdk.java.net/browse/JDK-8221723
> Webrev: http://cr.openjdk.java.net/~redestad/8221723/
>
> Testing: tier1-3, no regression on existing and new StringHashCode
> micros
>
> /Claes
>

Re: RFR: 8221723: Avoid storing zero to String.hash

[dean . long]

Wouldn't it be better to write a non-0 value when the computed hash code 
is 0, so we don't have to recompute it?  Is there some advantage to 
writing 0 instead of any other value, such as 1?


dl

On 4/1/19 4:57 AM, Claes Redestad wrote:

Hi,

when a String has a calculated hash code value of 0, we recalculate and
store a 0 to the String.hash field every time (except for the empty
String, which is special cased). To make String objects more amenable to
storage in shared read-only memory, e.g., CDS archives, we should avoid
this redundant store.

Bug:    https://bugs.openjdk.java.net/browse/JDK-8221723
Webrev: http://cr.openjdk.java.net/~redestad/8221723/

Testing: tier1-3, no regression on existing and new StringHashCode
micros

/Claes

Re: Proposal: Use new JDK_EXPORT decorator instead of JNIEXPORT

[dean . long]

If we are adding JDK_EXPORT, does it make sense to add JDK_CALL as well?

dl

On 12/16/18 4:57 PM, David Holmes wrote:

Hi Magnus,

Thanks for explaining how addition of JNIEXPORT may have started this 
problem.


One follow up:

This will also need a CSR request due to the change in linking 
behaviour.
I'm not sure what you mean by this..? My entire intention here is 
that we should make no changes to documented interfaces; this is 
strictly an internal change, so no CSR should be needed. Also, I 
don't understand what you mean by "linking behavior"? 


A CSR request is also required for behavioural changes - which this 
is. Someone linking an "internal only" function today may get an error 
if JNICALL is removed tomorrow. This may be acceptable but that is 
what the CSR request establishes.


Thanks,
David


On 13/12/2018 8:37 pm, Magnus Ihse Bursie wrote:

On 2018-12-12 13:17, David Holmes wrote:

Okay I went away and did some homework ...

Let me back up a bit and see if I have the evolution of this correctly.

The native implementation of Java methods should be declared and 
defined with JNIEXPORT and JNICALL.


JNIEXPORT controls the export visibility so they can be linked.

JNICALL controls the calling convention and is needed so that the 
JVM's calling convention matches the native code. [This part was 
unclear to me.]
Yes. And JNICALL is empty on all platforms except Windows 32, that's 
why we're only seeing issues about mismatch there.


Other native methods exported from the same (or different) native 
libraries may also be decorated with JNIEXPORT and JNICALL. But in 
places there is a mismatch between the declaration in the header and 
the definition leading to errors.
Yes; this mismatch has typically happened when the linking has not 
been done by simply including the relevant header file, but either 
duplicating the definition, or looking up the symbol dynamically. 
Otherwise things should basically work out.


There are two main types of such native functions:

a) those publicly defined in the various native APIs: JNI itself 
(jni.h), JVM TI (jvmti.h), AWT (jawt.h) ...


b) those intended only for use by other native code within the JDK 
libraries (JLI_* - though I note Alan's comment re javafxpackager, 
others??)


and a possible third type are callback functions like the JPLISAgent 
event handlers (e.g. eventHandlerVMInit).


I'm not sure I understand what the third type is, but if it is a 
publically defined API (which, at a first look, the JPLISAgent API 
seems to be), then it's part of catagory a, otherwise it's part of 
category b.


I must also state that my initial proposal didn't separate these two 
cases. I had in mind only the b cases -- I have no intention of 
changing public specifications, but I did not express this in my 
proposal. That might have been one source of confusion. I apologize 
for this omission.


There is a view that no native method that isn't the native-half of 
a Java method should use JNICALL. [Okay I can see how that makes 
sense - regardless of the actual calling convention used marking 
such methods as "must use the same calling convention as the VM 
native method call" is somewhat meaningless. Yet obviously the 
public native APIs do specify JNICALL so this is not a hard and fast 
rule. Further the callbacks must also follow a convention known to 
the VM doing the calling!]
Yes, or rather the rule is "only native half Java methods should use 
JNICALL, and also all public APIs where so is specified".




Where we have (introduced?) a discrepancy between the definition and 
declaration the approach has been (because of the previous view) to 
remove JNICALL. [This should only relate to methods of kind (b) above.]
Actually, it's not so much as we have *removed* JNICALL, as that we 
have *introduced* JNIEXPORT. When I removed the map files, I also 
removed the .def files and /export command lines for Windows. As a 
result, I needed to add JNIEXPORT to a lot of places. Initially, I 
followed the rule of adding JNICALL to those calls -- but I can 
surely have missed a couple of places, since things will work fine 
anyway, as long as caller and callee agree on the calling convention; 
and a mismatch can only happen on Windows 32, which we do not build 
and test. So there is a risk that even with the initial patch, not 
all newly added JNIEXPORTs had JNICALL.


Then, it turned out, that a lot of this code did not compile with 
Windows 32. With no deeper analysis of the flaw, the blame was put on 
the absence or presence of JNICALL, and a series of patches were made 
where JNICALL was more or less arbitrarily added or removed, until 
things "worked". This should have been a warning sign, and I was 
increasingly uneasy about all these changes, but I hadn't spent 
enough time to realize what the core issue was or how to resolve it 
properly.




That leaves those methods with JNIEXPORT only.

That seems wrong to you because they are not "JNI methods", so you 
want 

Re: RFR(M) 8212605: Pure-Java implementation of AccessController.doPrivileged

[dean . long]

Fixed.  Thanks.

dl

On 11/5/18 8:00 AM, Sean Mullan wrote:

726 * The VM recoginizes this method as special, so any changes to the

s/recoginizes/recognizes/

--Sean

On 11/3/18 4:00 PM, dean.l...@oracle.com wrote:
I made a pass at improving the comments based on feedback I've 
received.  I updated webrev.4 in place, along with an incremental diff:


http://cr.openjdk.java.net/~dlong/8212605/webrev.4.update/

dl

On 10/31/18 9:39 PM, Bernd Eckenfels wrote:
I find the tail call optimization comment in wrapException adds only 
confusion to an otherwise clear helper. But maybe it’s just me who 
does not understand it.

Re: RFR(M) 8212605: Pure-Java implementation of AccessController.doPrivileged

[dean . long]

Thanks Alan.

dl

On 11/4/18 1:03 AM, Alan Bateman wrote:

On 03/11/2018 20:00, dean.l...@oracle.com wrote:
I made a pass at improving the comments based on feedback I've 
received.  I updated webrev.4 in place, along with an incremental diff:
I looked through the updated webrev.4, mostly studying the changes in 
AccessController.java and jvm.cpp and the changes look good to me.


-Alan.

Re: RFR(M) 8212605: Pure-Java implementation of AccessController.doPrivileged

[dean . long]

I made a pass at improving the comments based on feedback I've 
received.  I updated webrev.4 in place, along with an incremental diff:


http://cr.openjdk.java.net/~dlong/8212605/webrev.4.update/

dl

On 10/31/18 9:39 PM, Bernd Eckenfels wrote:
I find the tail call optimization comment in wrapException adds only 
confusion to an otherwise clear helper. But maybe it’s just me who 
does not understand it.

Re: RFR(M) 8212605: Pure-Java implementation of AccessController.doPrivileged

[dean . long]

Thanks Mandy.  I also appreciate you noticing (off-list) that I can 
remove the extra space in "Class " in several places.  I have updated 
webrev.4 in place.


dl

On 11/2/18 1:55 PM, Mandy Chung wrote:

Hi Dean,

I reviewed webrev.4 version.  It looks good.  Happy to see moving the 
doPrivileged support to Java and the performance improvement.


On 10/31/18 3:23 PM, dean.l...@oracle.com wrote:

https://bugs.openjdk.java.net/browse/JDK-8212605
http://cr.openjdk.java.net/~dlong/8212605/webrev.1

This change implements AccessController.doPrivileged in Java. This 
gives a performance improvement while also being useful to Project 
Loom by removing the Java --> native --> Java transition.  One reason 
doPrivileged has historically been in native is because of the need 
to guarantee the cleanup of the privileged context when doPrivileged 
returns.  To do that in Java, the information that 
AccessController.getContext needs is pushed onto the Java stack as 
arguments to a method that getContext will recognize during its stack 
walk.  This allows us to remove the native privileged stack while 
guaranteeing that the privileged context goes away when the method 
returns.
Tested with tier1-tier3 hotspot and jdk tests and JCK 
api/java_security tests.  For the first few rounds of testing, I kept 
the old native privileged stack and compared the results of the old 
and new implementations for each getContext call, which did catch 
some early bugs.  The changes were also examined by internal security 
experts and run through additional internal security tests.


The improvement on this [1] doPrivileged microbenchmark is 
approximate 50x.


There is no attempt to optimize getContext() or security permission 
checks in this change, however, this is intended to be a first step 
towards other possible improvements, for example those proposed here 
[2].




FYI.  Sean and I also did some experiment to replace 
JVM_GetStackAccessControlContext with StackWalker some time ago. 
Another potential area to move the code from VM to Java for the future 
as David explored and probably involves  performance work in the stack 
walker.


Mandy

Re: RFR(M) 8212605: Pure-Java implementation of AccessController.doPrivileged

[dean . long]

On 11/1/18 12:39 PM, Sean Mullan wrote:
I also replaced getCallerPD with the faster getProtectionDomain and 
removed a stale comment about impliesCreateAccessControlContext being 
called by the VM. It should be safe to remove now, but I left it in 
to minimize changes.


I would just remove it, so we don't forget about it later. 


OK, I removed it:

http://cr.openjdk.java.net/~dlong/8212605/webrev.4.incr/

dl

Re: RFR(M) 8212605: Pure-Java implementation of AccessController.doPrivileged

[dean . long]

On 11/1/18 1:45 PM, Mandy Chung wrote:



On 11/1/18 1:18 AM, Vladimir Ivanov wrote:


I think it's a good idea, but I believe it would require a CSR 
request. Do you mind if I file a separate issue for 
jdk.internal.vm.annotation.Hidden?


Sure.

Most of the annotations in jdk.internal.vm.annotation were originally 
located in java.lang.invoke. Not sure CSR will be required in this 
particular case.




@Hidden is internal and no CSR is needed.

FYI.  JDK-8212620 is the RFE to consider providing a standard 
mechanism to hide frames from stack trace.




OK, I already filed JDK-8213234 but I think I should just close it as a 
duplicate of JDK-8212620.


dl


Mandy


Best regards,
Vladimir Ivanov


On 10/31/18 6:11 PM, Vladimir Ivanov wrote:

Dean,

src/java.base/share/classes/java/security/AccessController.java:
+    /**
+ * Internal marker for hidden implementation frames.
+ */
+    /*non-public*/
+    @Target(ElementType.METHOD)
+    @Retention(RetentionPolicy.RUNTIME)
+    @interface Hidden {
+    }

You declare @Hidden, but then map it to _method_Hidden along with 
@Hidden from java.lang.invoke.LambdaForm.


What do you think about moving LambdaForm.Hidden to 
jdk.internal.vm.annotation instead and share among all usages?


Best regards,
Vladimir Ivanov

On 31/10/2018 15:23, dean.l...@oracle.com wrote:

https://bugs.openjdk.java.net/browse/JDK-8212605
http://cr.openjdk.java.net/~dlong/8212605/webrev.1

This change implements AccessController.doPrivileged in Java. This 
gives a performance improvement while also being useful to Project 
Loom by removing the Java --> native --> Java transition.  One 
reason doPrivileged has historically been in native is because of 
the need to guarantee the cleanup of the privileged context when 
doPrivileged returns.  To do that in Java, the information that 
AccessController.getContext needs is pushed onto the Java stack as 
arguments to a method that getContext will recognize during its 
stack walk.  This allows us to remove the native privileged stack 
while guaranteeing that the privileged context goes away when the 
method returns.


Tested with tier1-tier3 hotspot and jdk tests and JCK 
api/java_security tests.  For the first few rounds of testing, I 
kept the old native privileged stack and compared the results of 
the old and new implementations for each getContext call, which 
did catch some early bugs.  The changes were also examined by 
internal security experts and run through additional internal 
security tests.


The improvement on this [1] doPrivileged microbenchmark is 
approximate 50x.


There is no attempt to optimize getContext() or security 
permission checks in this change, however, this is intended to be 
a first step towards other possible improvements, for example 
those proposed here [2].


dl

[1] 
http://hg.openjdk.java.net/code-tools/jmh-jdk-microbenchmarks/file/fc4783360f58/src/main/java/org/openjdk/bench/java/security/DoPrivileged.java 

[2] 
http://mail.openjdk.java.net/pipermail/security-dev/2017-December/016627.html 

Re: RFR(M) 8212605: Pure-Java implementation of AccessController.doPrivileged

[dean . long]

On 11/1/18 9:48 AM, Sean Mullan wrote:

On 11/1/18 1:29 AM, dean.l...@oracle.com wrote:

On 10/31/18 9:39 PM, Bernd Eckenfels wrote:
http://cr.openjdk.java.net/~dlong/8212605/webrev.1/src/java.base/share/classes/java/security/AccessController.java.udiff.html 



In checkContext should the security manager be null checked first 
instead of last to optimize for the typical case? (If the side 
effects in that expression are desired it should be documented)


I was following the example of createWrapper.  The side-effects of 
getInnocuousAcc() will only happen once, so the order shouldn't 
matter here, except for performance reasons.  I don't have a strong 
opinion about the order, but it looks like the typical case for 
createWrapper would also be the typical case for checkContext, so 
maybe they both should be changed, if indeed a null security manager 
is the more typical case.


A null SM should be the more common case. I am ok with changing the 
order so the SM is checked first, but it should be done in both the 
createWrapper and checkContext methods. Alternatively, we could see if 
they could be combined to eliminate the duplicate code but that might 
not be practical from looking at them.




I don't see a way to do it without using a lambda or doing extra work in 
one version, so I just changed the order for now.  I also replaced 
getCallerPD with the faster getProtectionDomain and removed a stale 
comment about impliesCreateAccessControlContext being called by the VM.  
It should be safe to remove now, but I left it in to minimize changes.


http://cr.openjdk.java.net/~dlong/8212605/webrev.3.incr/
http://cr.openjdk.java.net/~dlong/8212605/webrev.3/

dl


--Sean

Re: RFR(M) 8212605: Pure-Java implementation of AccessController.doPrivileged

[dean . long]

On 11/1/18 10:01 AM, Sean Mullan wrote:

Some of the copyrights need to be updated to 2018.



Fixed.

All else looks good to me as I had reviewed an earlier version of this 
before. We have talked about doing this for a while now, so I am 
finally glad we and are able to pretty much eliminate one of the more 
common SecurityManager related hot-spots and give a performance boost 
to applications that don't use the SM as well.




Thanks Sean.

dl


--Sean

On 10/31/18 6:23 PM, dean.l...@oracle.com wrote:

https://bugs.openjdk.java.net/browse/JDK-8212605
http://cr.openjdk.java.net/~dlong/8212605/webrev.1

This change implements AccessController.doPrivileged in Java. This 
gives a performance improvement while also being useful to Project 
Loom by removing the Java --> native --> Java transition.  One reason 
doPrivileged has historically been in native is because of the need 
to guarantee the cleanup of the privileged context when doPrivileged 
returns.  To do that in Java, the information that 
AccessController.getContext needs is pushed onto the Java stack as 
arguments to a method that getContext will recognize during its stack 
walk.  This allows us to remove the native privileged stack while 
guaranteeing that the privileged context goes away when the method 
returns.


Tested with tier1-tier3 hotspot and jdk tests and JCK 
api/java_security tests.  For the first few rounds of testing, I kept 
the old native privileged stack and compared the results of the old 
and new implementations for each getContext call, which did catch 
some early bugs.  The changes were also examined by internal security 
experts and run through additional internal security tests.


The improvement on this [1] doPrivileged microbenchmark is 
approximate 50x.


There is no attempt to optimize getContext() or security permission 
checks in this change, however, this is intended to be a first step 
towards other possible improvements, for example those proposed here 
[2].


dl

[1] 
http://hg.openjdk.java.net/code-tools/jmh-jdk-microbenchmarks/file/fc4783360f58/src/main/java/org/openjdk/bench/java/security/DoPrivileged.java 

[2] 
http://mail.openjdk.java.net/pipermail/security-dev/2017-December/016627.html 

Re: RFR(M) 8212605: Pure-Java implementation of AccessController.doPrivileged

[dean . long]

Hi Bernd,

On 10/31/18 9:39 PM, Bernd Eckenfels wrote:

http://cr.openjdk.java.net/~dlong/8212605/webrev.1/src/java.base/share/classes/java/security/AccessController.java.udiff.html

In checkContext should the security manager be null checked first instead of 
last to optimize for the typical case? (If the side effects in that expression 
are desired it should be documented)


I was following the example of createWrapper.  The side-effects of 
getInnocuousAcc() will only happen once, so the order shouldn't matter 
here, except for performance reasons.  I don't have a strong opinion 
about the order, but it looks like the typical case for createWrapper 
would also be the typical case for checkContext, so maybe they both 
should be changed, if indeed a null security manager is the more typical 
case.



I find the tail call optimization comment in wrapException adds only confusion 
to an otherwise clear helper. But maybe it’s just me who does not understand it.


OK, I'm happy to remove it if it's confusing or not helpful.  The reason 
for the other @ForceInline and @ReservedStackAccess annotations is to 
reduce that chance of getting a StackOverflowError, and tail call 
elimination would also help with that.  Let's see if anyone else finds 
the comment helpful or confusing.


dl


Gruss
Bernd
--
http://bernd.eckenfels.net


Von: core-libs-dev  im Auftrag von Vladimir 
Ivanov 
Gesendet: Donnerstag, November 1, 2018 5:11 AM
An: dean.l...@oracle.com; security-...@openjdk.java.net; core-libs-dev Libs; 
hotspot-dev developers
Betreff: Re: RFR(M) 8212605: Pure-Java implementation of 
AccessController.doPrivileged

Dean,

src/java.base/share/classes/java/security/AccessController.java:
+ /**
+ * Internal marker for hidden implementation frames.
+ */
+ /*non-public*/
+ @Target(ElementType.METHOD)
+ @Retention(RetentionPolicy.RUNTIME)
+ @interface Hidden {
+ }

You declare @Hidden, but then map it to _method_Hidden along with
@Hidden from java.lang.invoke.LambdaForm.

What do you think about moving LambdaForm.Hidden to
jdk.internal.vm.annotation instead and share among all usages?

Best regards,
Vladimir Ivanov

On 31/10/2018 15:23, dean.l...@oracle.com wrote:

https://bugs.openjdk.java.net/browse/JDK-8212605
http://cr.openjdk.java.net/~dlong/8212605/webrev.1

This change implements AccessController.doPrivileged in Java.  This
gives a performance improvement while also being useful to Project Loom
by removing the Java --> native --> Java transition.  One reason
doPrivileged has historically been in native is because of the need to
guarantee the cleanup of the privileged context when doPrivileged
returns.  To do that in Java, the information that
AccessController.getContext needs is pushed onto the Java stack as
arguments to a method that getContext will recognize during its stack
walk.  This allows us to remove the native privileged stack while
guaranteeing that the privileged context goes away when the method returns.

Tested with tier1-tier3 hotspot and jdk tests and JCK api/java_security
tests.  For the first few rounds of testing, I kept the old native
privileged stack and compared the results of the old and new
implementations for each getContext call, which did catch some early
bugs.  The changes were also examined by internal security experts and
run through additional internal security tests.

The improvement on this [1] doPrivileged microbenchmark is approximate 50x.

There is no attempt to optimize getContext() or security permission
checks in this change, however, this is intended to be a first step
towards other possible improvements, for example those proposed here [2].

dl

[1]
http://hg.openjdk.java.net/code-tools/jmh-jdk-microbenchmarks/file/fc4783360f58/src/main/java/org/openjdk/bench/java/security/DoPrivileged.java

[2]
http://mail.openjdk.java.net/pipermail/security-dev/2017-December/016627.html

Re: RFR(M) 8212605: Pure-Java implementation of AccessController.doPrivileged

[dean . long]

I think it's a good idea, but I believe it would require a CSR request.  
Do you mind if I file a separate issue for 
jdk.internal.vm.annotation.Hidden?


dl

On 10/31/18 6:11 PM, Vladimir Ivanov wrote:

Dean,

src/java.base/share/classes/java/security/AccessController.java:
+    /**
+ * Internal marker for hidden implementation frames.
+ */
+    /*non-public*/
+    @Target(ElementType.METHOD)
+    @Retention(RetentionPolicy.RUNTIME)
+    @interface Hidden {
+    }

You declare @Hidden, but then map it to _method_Hidden along with 
@Hidden from java.lang.invoke.LambdaForm.


What do you think about moving LambdaForm.Hidden to 
jdk.internal.vm.annotation instead and share among all usages?


Best regards,
Vladimir Ivanov

On 31/10/2018 15:23, dean.l...@oracle.com wrote:

https://bugs.openjdk.java.net/browse/JDK-8212605
http://cr.openjdk.java.net/~dlong/8212605/webrev.1

This change implements AccessController.doPrivileged in Java. This 
gives a performance improvement while also being useful to Project 
Loom by removing the Java --> native --> Java transition.  One reason 
doPrivileged has historically been in native is because of the need 
to guarantee the cleanup of the privileged context when doPrivileged 
returns.  To do that in Java, the information that 
AccessController.getContext needs is pushed onto the Java stack as 
arguments to a method that getContext will recognize during its stack 
walk.  This allows us to remove the native privileged stack while 
guaranteeing that the privileged context goes away when the method 
returns.


Tested with tier1-tier3 hotspot and jdk tests and JCK 
api/java_security tests.  For the first few rounds of testing, I kept 
the old native privileged stack and compared the results of the old 
and new implementations for each getContext call, which did catch 
some early bugs.  The changes were also examined by internal security 
experts and run through additional internal security tests.


The improvement on this [1] doPrivileged microbenchmark is 
approximate 50x.


There is no attempt to optimize getContext() or security permission 
checks in this change, however, this is intended to be a first step 
towards other possible improvements, for example those proposed here 
[2].


dl

[1] 
http://hg.openjdk.java.net/code-tools/jmh-jdk-microbenchmarks/file/fc4783360f58/src/main/java/org/openjdk/bench/java/security/DoPrivileged.java 

[2] 
http://mail.openjdk.java.net/pipermail/security-dev/2017-December/016627.html 

Re: RFR(M) 8212605: Pure-Java implementation of AccessController.doPrivileged

[dean . long]

Thanks Ioi.

dl

On 10/31/18 6:01 PM, Ioi Lam wrote:



On 10/31/18 5:13 PM, dean.l...@oracle.com wrote:

On 10/31/18 4:06 PM, David Holmes wrote:

Hi Dean,

Looking only at the hotspot changes. The removal of the DoPrivileged 
and related privileged_stack code seems okay. I have a few related 
comments:


src/hotspot/share/classfile/systemDictionary.hpp

You added the java_security_AccessController class after 
java_security_AccessControlContext. Did you actually verify where in 
the load/initialization order the AccessController class appears 
today, and where it appears after your change? (Note the comments at 
the start of WK_KLASSES_DO). Changes to the initialization order 
would be my biggest concern with this patch.




No, I did not notice that comment and assumed alphabetical order was 
OK here.  However, these classes appear to be only resolved, not 
initialized (and AccessController does not have a static 
initializer), so could you explain how the order in this list can 
affect initialization order?


I only need this in JVM_GetStackAccessControlContext, which is a 
static JNI method, so I could get the klass* from the incoming jclass 
instead of using a well-known class entry.


Hi Dean,

You're correct that those classes are only resolved, and not 
initialized. So the order shouldn't matter too much. However, the 
order of the first few classes is important, as the creation of 
Object, Serializable, Cloneable, String, etc, has to be done in a 
certain order.


I'm not sure whether the order of the reference classes, 292 classes, 
and box classes are important. I'll experiment of getting rid of the 
separate phases after the call to Universe::fixup_mirrors(). This 
might be relics from old days where the classes were once indeed 
initialized in SystemDictionary::initialize_well_known_classes, which 
was the old name for SystemDictionary::resolve_well_known_classes.


(-XX:+TraceBytecodes shows no Java code is executed before 
resolve_well_known_classes returns).


I filed https://bugs.openjdk.java.net/browse/JDK-8213230

For the time being, I think as long as you put the new 
AccessController class near the existing class AccessControlContext, 
you should be OK.


Thanks
- Ioi



---

I'm unclear about the change to the test:

test/hotspot/jtreg/runtime/JVMDoPrivileged/DoPrivRunAbstract.jasm

as it still refers to the now non-existent JVM_DoPrivileged in the 
summary. Does this test still make sense? Should it be moved to the 
Java side now it doesn't actually test anything in the VM?




I think these tests still make sense, unless we have similar coverage 
somewhere else.  How about if I fix the reference to JVM_DoPrivileged 
for now and file a separate bug about moving or removing these tests?



---

There may be further dead code to remove now:

vmSymbols.hpp: codesource_permissioncollection_signature is now 
unreferenced and can be removed.


javaClasses.*:
  - java_lang_System::has_security_manager
  - java_security_AccessControlContext::is_authorized

./share/memory/universe.hpp:  static Method* 
protection_domain_implies_method()




Good catches!  I have uploaded a new webrev:

http://cr.openjdk.java.net/~dlong/8212605/webrev.2/
http://cr.openjdk.java.net/~dlong/8212605/webrev.2.incr/ (incremental 
diff)


dl


---

Thanks,
David


On 1/11/2018 8:23 AM, dean.l...@oracle.com wrote:

https://bugs.openjdk.java.net/browse/JDK-8212605
http://cr.openjdk.java.net/~dlong/8212605/webrev.1

This change implements AccessController.doPrivileged in Java. This 
gives a performance improvement while also being useful to Project 
Loom by removing the Java --> native --> Java transition.  One 
reason doPrivileged has historically been in native is because of 
the need to guarantee the cleanup of the privileged context when 
doPrivileged returns.  To do that in Java, the information that 
AccessController.getContext needs is pushed onto the Java stack as 
arguments to a method that getContext will recognize during its 
stack walk.  This allows us to remove the native privileged stack 
while guaranteeing that the privileged context goes away when the 
method returns.


Tested with tier1-tier3 hotspot and jdk tests and JCK 
api/java_security tests.  For the first few rounds of testing, I 
kept the old native privileged stack and compared the results of 
the old and new implementations for each getContext call, which did 
catch some early bugs.  The changes were also examined by internal 
security experts and run through additional internal security tests.


The improvement on this [1] doPrivileged microbenchmark is 
approximate 50x.


There is no attempt to optimize getContext() or security permission 
checks in this change, however, this is intended to be a first step 
towards other possible improvements, for example those proposed 
here [2].


dl

[1] 
http://hg.openjdk.java.net/code-tools/jmh-jdk-microbenchmarks/file/fc4783360f58/src/main/java/org/openjdk/bench/java/security/DoPrivileged.java 

[2] 

Re: RFR(M) 8212605: Pure-Java implementation of AccessController.doPrivileged

[dean . long]

Thanks David.

dl

On 10/31/18 5:54 PM, David Holmes wrote:

Hi Dean,

On 1/11/2018 10:13 AM, dean.l...@oracle.com wrote:

On 10/31/18 4:06 PM, David Holmes wrote:

Hi Dean,

Looking only at the hotspot changes. The removal of the DoPrivileged 
and related privileged_stack code seems okay. I have a few related 
comments:


src/hotspot/share/classfile/systemDictionary.hpp

You added the java_security_AccessController class after 
java_security_AccessControlContext. Did you actually verify where in 
the load/initialization order the AccessController class appears 
today, and where it appears after your change? (Note the comments at 
the start of WK_KLASSES_DO). Changes to the initialization order 
would be my biggest concern with this patch.




No, I did not notice that comment and assumed alphabetical order was 
OK here.  However, these classes appear to be only resolved, not 
initialized (and AccessController does not have a static 
initializer), so could you explain how the order in this list can 
affect initialization order?


You're right it doesn't. There are a couple of comments that refer to 
"initialization" but it's not static initialization of these classes.


I'm unclear how the resolution order in that list may interact with 
other parts of the startup sequence.


I only need this in JVM_GetStackAccessControlContext, which is a 
static JNI method, so I could get the klass* from the incoming jclass 
instead of using a well-known class entry.


I think it's okay given we have AccessControlContext there anyway.


---

I'm unclear about the change to the test:

test/hotspot/jtreg/runtime/JVMDoPrivileged/DoPrivRunAbstract.jasm

as it still refers to the now non-existent JVM_DoPrivileged in the 
summary. Does this test still make sense? Should it be moved to the 
Java side now it doesn't actually test anything in the VM?




I think these tests still make sense, unless we have similar coverage 
somewhere else.  How about if I fix the reference to JVM_DoPrivileged 
for now and file a separate bug about moving or removing these tests?


Yep that's fine.


---

There may be further dead code to remove now:

vmSymbols.hpp: codesource_permissioncollection_signature is now 
unreferenced and can be removed.


javaClasses.*:
  - java_lang_System::has_security_manager
  - java_security_AccessControlContext::is_authorized

./share/memory/universe.hpp:  static Method* 
protection_domain_implies_method()




Good catches!  I have uploaded a new webrev:

http://cr.openjdk.java.net/~dlong/8212605/webrev.2/
http://cr.openjdk.java.net/~dlong/8212605/webrev.2.incr/ (incremental 
diff)


Updates all look fine.

Thanks,
David
-


dl


---

Thanks,
David


On 1/11/2018 8:23 AM, dean.l...@oracle.com wrote:

https://bugs.openjdk.java.net/browse/JDK-8212605
http://cr.openjdk.java.net/~dlong/8212605/webrev.1

This change implements AccessController.doPrivileged in Java. This 
gives a performance improvement while also being useful to Project 
Loom by removing the Java --> native --> Java transition.  One 
reason doPrivileged has historically been in native is because of 
the need to guarantee the cleanup of the privileged context when 
doPrivileged returns.  To do that in Java, the information that 
AccessController.getContext needs is pushed onto the Java stack as 
arguments to a method that getContext will recognize during its 
stack walk.  This allows us to remove the native privileged stack 
while guaranteeing that the privileged context goes away when the 
method returns.


Tested with tier1-tier3 hotspot and jdk tests and JCK 
api/java_security tests.  For the first few rounds of testing, I 
kept the old native privileged stack and compared the results of 
the old and new implementations for each getContext call, which did 
catch some early bugs.  The changes were also examined by internal 
security experts and run through additional internal security tests.


The improvement on this [1] doPrivileged microbenchmark is 
approximate 50x.


There is no attempt to optimize getContext() or security permission 
checks in this change, however, this is intended to be a first step 
towards other possible improvements, for example those proposed 
here [2].


dl

[1] 
http://hg.openjdk.java.net/code-tools/jmh-jdk-microbenchmarks/file/fc4783360f58/src/main/java/org/openjdk/bench/java/security/DoPrivileged.java 

[2] 
http://mail.openjdk.java.net/pipermail/security-dev/2017-December/016627.html 

Re: RFR(M) 8212605: Pure-Java implementation of AccessController.doPrivileged

[dean . long]

On 10/31/18 4:06 PM, David Holmes wrote:

Hi Dean,

Looking only at the hotspot changes. The removal of the DoPrivileged 
and related privileged_stack code seems okay. I have a few related 
comments:


src/hotspot/share/classfile/systemDictionary.hpp

You added the java_security_AccessController class after 
java_security_AccessControlContext. Did you actually verify where in 
the load/initialization order the AccessController class appears 
today, and where it appears after your change? (Note the comments at 
the start of WK_KLASSES_DO). Changes to the initialization order would 
be my biggest concern with this patch.




No, I did not notice that comment and assumed alphabetical order was OK 
here.  However, these classes appear to be only resolved, not 
initialized (and AccessController does not have a static initializer), 
so could you explain how the order in this list can affect 
initialization order?


I only need this in JVM_GetStackAccessControlContext, which is a static 
JNI method, so I could get the klass* from the incoming jclass instead 
of using a well-known class entry.



---

I'm unclear about the change to the test:

test/hotspot/jtreg/runtime/JVMDoPrivileged/DoPrivRunAbstract.jasm

as it still refers to the now non-existent JVM_DoPrivileged in the 
summary. Does this test still make sense? Should it be moved to the 
Java side now it doesn't actually test anything in the VM?




I think these tests still make sense, unless we have similar coverage 
somewhere else.  How about if I fix the reference to JVM_DoPrivileged 
for now and file a separate bug about moving or removing these tests?



---

There may be further dead code to remove now:

vmSymbols.hpp: codesource_permissioncollection_signature is now 
unreferenced and can be removed.


javaClasses.*:
  - java_lang_System::has_security_manager
  - java_security_AccessControlContext::is_authorized

./share/memory/universe.hpp:  static Method* 
protection_domain_implies_method()




Good catches!  I have uploaded a new webrev:

http://cr.openjdk.java.net/~dlong/8212605/webrev.2/
http://cr.openjdk.java.net/~dlong/8212605/webrev.2.incr/ (incremental diff)

dl


---

Thanks,
David


On 1/11/2018 8:23 AM, dean.l...@oracle.com wrote:

https://bugs.openjdk.java.net/browse/JDK-8212605
http://cr.openjdk.java.net/~dlong/8212605/webrev.1

This change implements AccessController.doPrivileged in Java. This 
gives a performance improvement while also being useful to Project 
Loom by removing the Java --> native --> Java transition.  One reason 
doPrivileged has historically been in native is because of the need 
to guarantee the cleanup of the privileged context when doPrivileged 
returns.  To do that in Java, the information that 
AccessController.getContext needs is pushed onto the Java stack as 
arguments to a method that getContext will recognize during its stack 
walk.  This allows us to remove the native privileged stack while 
guaranteeing that the privileged context goes away when the method 
returns.


Tested with tier1-tier3 hotspot and jdk tests and JCK 
api/java_security tests.  For the first few rounds of testing, I kept 
the old native privileged stack and compared the results of the old 
and new implementations for each getContext call, which did catch 
some early bugs.  The changes were also examined by internal security 
experts and run through additional internal security tests.


The improvement on this [1] doPrivileged microbenchmark is 
approximate 50x.


There is no attempt to optimize getContext() or security permission 
checks in this change, however, this is intended to be a first step 
towards other possible improvements, for example those proposed here 
[2].


dl

[1] 
http://hg.openjdk.java.net/code-tools/jmh-jdk-microbenchmarks/file/fc4783360f58/src/main/java/org/openjdk/bench/java/security/DoPrivileged.java 

[2] 
http://mail.openjdk.java.net/pipermail/security-dev/2017-December/016627.html 

RFR(M) 8212605: Pure-Java implementation of AccessController.doPrivileged

[dean . long]

https://bugs.openjdk.java.net/browse/JDK-8212605
http://cr.openjdk.java.net/~dlong/8212605/webrev.1

This change implements AccessController.doPrivileged in Java.  This 
gives a performance improvement while also being useful to Project Loom 
by removing the Java --> native --> Java transition.  One reason 
doPrivileged has historically been in native is because of the need to 
guarantee the cleanup of the privileged context when doPrivileged 
returns.  To do that in Java, the information that 
AccessController.getContext needs is pushed onto the Java stack as 
arguments to a method that getContext will recognize during its stack 
walk.  This allows us to remove the native privileged stack while 
guaranteeing that the privileged context goes away when the method returns.


Tested with tier1-tier3 hotspot and jdk tests and JCK api/java_security 
tests.  For the first few rounds of testing, I kept the old native 
privileged stack and compared the results of the old and new 
implementations for each getContext call, which did catch some early 
bugs.  The changes were also examined by internal security experts and 
run through additional internal security tests.


The improvement on this [1] doPrivileged microbenchmark is approximate 50x.

There is no attempt to optimize getContext() or security permission 
checks in this change, however, this is intended to be a first step 
towards other possible improvements, for example those proposed here [2].


dl

[1] 
http://hg.openjdk.java.net/code-tools/jmh-jdk-microbenchmarks/file/fc4783360f58/src/main/java/org/openjdk/bench/java/security/DoPrivileged.java
[2] 
http://mail.openjdk.java.net/pipermail/security-dev/2017-December/016627.html

Re: [9] RFR(L) 8158168: SIGSEGV: CollectedHeap::fill_with_objects(HeapWord*, unsigned long, bool)+0xa8

[dean . long]

Actually, the assert inside getChar/putChar shouldn't affect inlining, 
because C1 and C2 will use the intrinsic instead, so I'd like to use 
webrev.2 as is, so I don't have to re-run all the tests.


dl


On 3/30/17 11:24 AM, dean.l...@oracle.com wrote:


I would like to go with the webrev.2 version, with asserts removed.  
All the reviewers have said they are OK with that version, and it 
allows more complete testing than the minimal version.


dl


On 3/23/17 12:03 PM, dean.l...@oracle.com wrote:


On 3/23/17 11:25 AM, dean.l...@oracle.com wrote:


On 3/22/17 1:49 PM, Vladimir Ivanov wrote:

Also, it looks like the changes I made to ASB.appendChars(char[] 
s, int

off, int end) are not needed.


Agree.


Vladimir, don't you need to replace checkIndex with checkOffset in
indexOf and lastIndexOf, so that we allow count == length?


Yes, my bad. Good catch. Updated webrev in place.

FTR I haven't done any extensive testing of the minimized fix.

If we agree to proceed with it, the regression test should be 
updated as well. I think the viable solution would be to construct 
broken SBs (using reflection) and invoke affected methods on them.




We can construct broken SBs using the Helper class that gets patched 
into java.lang.  I'll work on that.




Nevermind.  I forgot that some problems can only happen when the SB 
is changed half-way through the method.  For example,
in append(), we can't force an overflow unless we change the SB after 
ensureCapacityInternal() is called.  I could do something like:


  760 public AbstractStringBuilder append(int i) {
761 int count = this.count;
  762 int spaceNeeded = count + Integer.stringSize(i);
  763 ensureCapacityInternal(spaceNeeded);
  764 if (isLatin1()) {
  >>  Helper.fuzzValue(this);
  765 Integer.getChars(i, spaceNeeded, value);
  766 } else {
  767 byte[] val = this.value;
  >>  Helper.fuzzValue(this);
  768 checkBoundsBeginEnd(count, spaceNeeded, val.length >> 1);
  769 Integer.getCharsUTF16(i, spaceNeeded, val);
  770 }
771 this.count = spaceNeeded;
  772 return this;
  773 }

where the default Helper.fuzzValue() is an empty method, but the test 
would patch in its own version of Helper that changes the ASB field 
values.  I like this less than refactoring the checks to StringUTF16.


dl


dl


Best regards,
Vladimir Ivanov


On 3/22/17 8:35 AM, Vladimir Ivanov wrote:
So are we convinced that the proposed changes will never lead 
to a

crash due to a missing or incorrect bounds check, due to a racy
use of
an unsynchronized ASB instance e.g. StringBuilder?


If only we had a static analysis tool that could tell us if the
code is
safe.  Because we don't, in my initial changeset, we always 
take a

snapshot of the ASB fields by passing those field values to
StringUTF16
before doing checks on them.  And I wrote a test to make sure 
that

those
StringUTF16 interfaces are catching all the underflows and 
overflows I
could imagine, and I added verification code to detect when a 
check

was
missed.

However, all the reviewers have requested to minimize the 
amount of

changes.  In Vladimir's version, if there is a missing check
somewhere,
then yes it could lead to a crash.


I'd like to point out that asserts and verification code are 
disabled

by default. They are invaluable during problem diagnosis, but don't
help at all from defence-in-depth perspective.

But I agree that it's easier to reason about and test the initial
version of the fix.


I wonder if the reviewers have fully realized the potential impact
here?
This has exposed a flaw in the way intrinsics are used from core
classes.


FTR here are the checks I omitted in the minimized version (modulo
separation of indexOf/lastIndexOf for trusted/non-trusted callers):

http://cr.openjdk.java.net/~vlivanov/dlong/8158168/redundant_checks/ 



Other than that, the difference is mainly about undoing 
refactorings

and removing verification logic (asserts + checks in the JVM).

There are still unsafe accesses which are considered safe in both
versions (see StringUTF16.Trusted usages in the initial version 
[1]).


We used to provide safe wrappers for unsafe intrinsics which 
makes it
much easier to reason about code correctness. I'd like to see 
compact

string code refactored that way and IMO the initial version by Dean
is a big step in the right direction.

I still prefer to see a point fix in 9 and major refactoring
happening in 10, but I'll leave the decision on how to proceed with
the fix to core-libs folks. After finishing the exercise minimizing
the fix, I'm much more comfortable with the initial fix [1] (though
there are changes I consider excessive).

Best regards,
Vladimir Ivanov

[1] http://cr.openjdk.java.net/~dlong/8158168/webrev.0


Some clarifications:


src/java.base/share/classes/java/lang/String.java:

The bounds check is needed only in String.nonSyncContentEquals

Re: [9] RFR(L) 8158168: SIGSEGV: CollectedHeap::fill_with_objects(HeapWord*, unsigned long, bool)+0xa8

[dean . long]

I would like to go with the webrev.2 version, with asserts removed.  All 
the reviewers have said they are OK with that version, and it allows 
more complete testing than the minimal version.


dl


On 3/23/17 12:03 PM, dean.l...@oracle.com wrote:


On 3/23/17 11:25 AM, dean.l...@oracle.com wrote:


On 3/22/17 1:49 PM, Vladimir Ivanov wrote:

Also, it looks like the changes I made to ASB.appendChars(char[] s, 
int

off, int end) are not needed.


Agree.


Vladimir, don't you need to replace checkIndex with checkOffset in
indexOf and lastIndexOf, so that we allow count == length?


Yes, my bad. Good catch. Updated webrev in place.

FTR I haven't done any extensive testing of the minimized fix.

If we agree to proceed with it, the regression test should be 
updated as well. I think the viable solution would be to construct 
broken SBs (using reflection) and invoke affected methods on them.




We can construct broken SBs using the Helper class that gets patched 
into java.lang.  I'll work on that.




Nevermind.  I forgot that some problems can only happen when the SB is 
changed half-way through the method.  For example,
in append(), we can't force an overflow unless we change the SB after 
ensureCapacityInternal() is called.  I could do something like:


  760 public AbstractStringBuilder append(int i) {
761 int count = this.count;
  762 int spaceNeeded = count + Integer.stringSize(i);
  763 ensureCapacityInternal(spaceNeeded);
  764 if (isLatin1()) {
  >>  Helper.fuzzValue(this);
  765 Integer.getChars(i, spaceNeeded, value);
  766 } else {
  767 byte[] val = this.value;
  >>  Helper.fuzzValue(this);
  768 checkBoundsBeginEnd(count, spaceNeeded, val.length >> 1);
  769 Integer.getCharsUTF16(i, spaceNeeded, val);
  770 }
771 this.count = spaceNeeded;
  772 return this;
  773 }

where the default Helper.fuzzValue() is an empty method, but the test 
would patch in its own version of Helper that changes the ASB field 
values.  I like this less than refactoring the checks to StringUTF16.


dl


dl


Best regards,
Vladimir Ivanov


On 3/22/17 8:35 AM, Vladimir Ivanov wrote:
So are we convinced that the proposed changes will never lead 
to a

crash due to a missing or incorrect bounds check, due to a racy
use of
an unsynchronized ASB instance e.g. StringBuilder?


If only we had a static analysis tool that could tell us if the
code is
safe.  Because we don't, in my initial changeset, we always take a
snapshot of the ASB fields by passing those field values to
StringUTF16
before doing checks on them.  And I wrote a test to make sure that
those
StringUTF16 interfaces are catching all the underflows and 
overflows I
could imagine, and I added verification code to detect when a 
check

was
missed.

However, all the reviewers have requested to minimize the 
amount of

changes.  In Vladimir's version, if there is a missing check
somewhere,
then yes it could lead to a crash.


I'd like to point out that asserts and verification code are 
disabled

by default. They are invaluable during problem diagnosis, but don't
help at all from defence-in-depth perspective.

But I agree that it's easier to reason about and test the initial
version of the fix.


I wonder if the reviewers have fully realized the potential impact
here?
This has exposed a flaw in the way intrinsics are used from core
classes.


FTR here are the checks I omitted in the minimized version (modulo
separation of indexOf/lastIndexOf for trusted/non-trusted callers):

http://cr.openjdk.java.net/~vlivanov/dlong/8158168/redundant_checks/

Other than that, the difference is mainly about undoing refactorings
and removing verification logic (asserts + checks in the JVM).

There are still unsafe accesses which are considered safe in both
versions (see StringUTF16.Trusted usages in the initial version 
[1]).


We used to provide safe wrappers for unsafe intrinsics which 
makes it
much easier to reason about code correctness. I'd like to see 
compact

string code refactored that way and IMO the initial version by Dean
is a big step in the right direction.

I still prefer to see a point fix in 9 and major refactoring
happening in 10, but I'll leave the decision on how to proceed with
the fix to core-libs folks. After finishing the exercise minimizing
the fix, I'm much more comfortable with the initial fix [1] (though
there are changes I consider excessive).

Best regards,
Vladimir Ivanov

[1] http://cr.openjdk.java.net/~dlong/8158168/webrev.0


Some clarifications:


src/java.base/share/classes/java/lang/String.java:

The bounds check is needed only in String.nonSyncContentEquals
when it
extracts info from AbstractStringBuilder. I don't see how 
out of

bounds access can happen in String.contentEquals:
 if (n != length()) {
 return false;
 }
...
 for (int i = 0; i < n; i++) {
 if 

Re: [9] RFR(L) 8158168: SIGSEGV: CollectedHeap::fill_with_objects(HeapWord*, unsigned long, bool)+0xa8

[dean . long]

On 3/23/17 11:25 AM, dean.l...@oracle.com wrote:


On 3/22/17 1:49 PM, Vladimir Ivanov wrote:


Also, it looks like the changes I made to ASB.appendChars(char[] s, int
off, int end) are not needed.


Agree.


Vladimir, don't you need to replace checkIndex with checkOffset in
indexOf and lastIndexOf, so that we allow count == length?


Yes, my bad. Good catch. Updated webrev in place.

FTR I haven't done any extensive testing of the minimized fix.

If we agree to proceed with it, the regression test should be updated 
as well. I think the viable solution would be to construct broken SBs 
(using reflection) and invoke affected methods on them.




We can construct broken SBs using the Helper class that gets patched 
into java.lang.  I'll work on that.




Nevermind.  I forgot that some problems can only happen when the SB is 
changed half-way through the method.  For example,
in append(), we can't force an overflow unless we change the SB after 
ensureCapacityInternal() is called.  I could do something like:


 760 public AbstractStringBuilder append(int i) {
761 int count = this.count;
 762 int spaceNeeded = count + Integer.stringSize(i);
 763 ensureCapacityInternal(spaceNeeded);
 764 if (isLatin1()) {
 >>  Helper.fuzzValue(this);
 765 Integer.getChars(i, spaceNeeded, value);
 766 } else {
 767 byte[] val = this.value;
 >>  Helper.fuzzValue(this);
 768 checkBoundsBeginEnd(count, spaceNeeded, val.length >> 1);
 769 Integer.getCharsUTF16(i, spaceNeeded, val);
 770 }
771 this.count = spaceNeeded;
 772 return this;
 773 }


where the default Helper.fuzzValue() is an empty method, but the test 
would patch in its own version of Helper that changes the ASB field 
values.  I like this less than refactoring the checks to StringUTF16.


dl


dl


Best regards,
Vladimir Ivanov


On 3/22/17 8:35 AM, Vladimir Ivanov wrote:

So are we convinced that the proposed changes will never lead to a
crash due to a missing or incorrect bounds check, due to a racy
use of
an unsynchronized ASB instance e.g. StringBuilder?


If only we had a static analysis tool that could tell us if the
code is
safe.  Because we don't, in my initial changeset, we always take a
snapshot of the ASB fields by passing those field values to
StringUTF16
before doing checks on them.  And I wrote a test to make sure that
those
StringUTF16 interfaces are catching all the underflows and 
overflows I

could imagine, and I added verification code to detect when a check
was
missed.

However, all the reviewers have requested to minimize the amount of
changes.  In Vladimir's version, if there is a missing check
somewhere,
then yes it could lead to a crash.


I'd like to point out that asserts and verification code are disabled
by default. They are invaluable during problem diagnosis, but don't
help at all from defence-in-depth perspective.

But I agree that it's easier to reason about and test the initial
version of the fix.


I wonder if the reviewers have fully realized the potential impact
here?
This has exposed a flaw in the way intrinsics are used from core
classes.


FTR here are the checks I omitted in the minimized version (modulo
separation of indexOf/lastIndexOf for trusted/non-trusted callers):

http://cr.openjdk.java.net/~vlivanov/dlong/8158168/redundant_checks/

Other than that, the difference is mainly about undoing refactorings
and removing verification logic (asserts + checks in the JVM).

There are still unsafe accesses which are considered safe in both
versions (see StringUTF16.Trusted usages in the initial version [1]).

We used to provide safe wrappers for unsafe intrinsics which makes it
much easier to reason about code correctness. I'd like to see compact
string code refactored that way and IMO the initial version by Dean
is a big step in the right direction.

I still prefer to see a point fix in 9 and major refactoring
happening in 10, but I'll leave the decision on how to proceed with
the fix to core-libs folks. After finishing the exercise minimizing
the fix, I'm much more comfortable with the initial fix [1] (though
there are changes I consider excessive).

Best regards,
Vladimir Ivanov

[1] http://cr.openjdk.java.net/~dlong/8158168/webrev.0


Some clarifications:


src/java.base/share/classes/java/lang/String.java:

The bounds check is needed only in String.nonSyncContentEquals
when it
extracts info from AbstractStringBuilder. I don't see how out of
bounds access can happen in String.contentEquals:
 if (n != length()) {
 return false;
 }
...
 for (int i = 0; i < n; i++) {
 if (StringUTF16.getChar(val, i) != 
cs.charAt(i)) {




OK.



src/java.base/share/classes/java/lang/StringConcatHelper.java:

I think bounds checks in StringConcatHelper.prepend() are 
skipped

intentionally, since java.lang.invoke.StringConcatFactory
constructs
method 

Re: [9] RFR(L) 8158168: SIGSEGV: CollectedHeap::fill_with_objects(HeapWord*, unsigned long, bool)+0xa8

[dean . long]

On 3/22/17 1:49 PM, Vladimir Ivanov wrote:


Also, it looks like the changes I made to ASB.appendChars(char[] s, int
off, int end) are not needed.


Agree.


Vladimir, don't you need to replace checkIndex with checkOffset in
indexOf and lastIndexOf, so that we allow count == length?


Yes, my bad. Good catch. Updated webrev in place.

FTR I haven't done any extensive testing of the minimized fix.

If we agree to proceed with it, the regression test should be updated 
as well. I think the viable solution would be to construct broken SBs 
(using reflection) and invoke affected methods on them.




We can construct broken SBs using the Helper class that gets patched 
into java.lang.  I'll work on that.


dl


Best regards,
Vladimir Ivanov


On 3/22/17 8:35 AM, Vladimir Ivanov wrote:

So are we convinced that the proposed changes will never lead to a
crash due to a missing or incorrect bounds check, due to a racy
use of
an unsynchronized ASB instance e.g. StringBuilder?


If only we had a static analysis tool that could tell us if the
code is
safe.  Because we don't, in my initial changeset, we always take a
snapshot of the ASB fields by passing those field values to
StringUTF16
before doing checks on them.  And I wrote a test to make sure that
those
StringUTF16 interfaces are catching all the underflows and 
overflows I

could imagine, and I added verification code to detect when a check
was
missed.

However, all the reviewers have requested to minimize the amount of
changes.  In Vladimir's version, if there is a missing check
somewhere,
then yes it could lead to a crash.


I'd like to point out that asserts and verification code are disabled
by default. They are invaluable during problem diagnosis, but don't
help at all from defence-in-depth perspective.

But I agree that it's easier to reason about and test the initial
version of the fix.


I wonder if the reviewers have fully realized the potential impact
here?
This has exposed a flaw in the way intrinsics are used from core
classes.


FTR here are the checks I omitted in the minimized version (modulo
separation of indexOf/lastIndexOf for trusted/non-trusted callers):

http://cr.openjdk.java.net/~vlivanov/dlong/8158168/redundant_checks/

Other than that, the difference is mainly about undoing refactorings
and removing verification logic (asserts + checks in the JVM).

There are still unsafe accesses which are considered safe in both
versions (see StringUTF16.Trusted usages in the initial version [1]).

We used to provide safe wrappers for unsafe intrinsics which makes it
much easier to reason about code correctness. I'd like to see compact
string code refactored that way and IMO the initial version by Dean
is a big step in the right direction.

I still prefer to see a point fix in 9 and major refactoring
happening in 10, but I'll leave the decision on how to proceed with
the fix to core-libs folks. After finishing the exercise minimizing
the fix, I'm much more comfortable with the initial fix [1] (though
there are changes I consider excessive).

Best regards,
Vladimir Ivanov

[1] http://cr.openjdk.java.net/~dlong/8158168/webrev.0


Some clarifications:


src/java.base/share/classes/java/lang/String.java:

The bounds check is needed only in String.nonSyncContentEquals
when it
extracts info from AbstractStringBuilder. I don't see how out of
bounds access can happen in String.contentEquals:
 if (n != length()) {
 return false;
 }
...
 for (int i = 0; i < n; i++) {
 if (StringUTF16.getChar(val, i) != 
cs.charAt(i)) {




OK.



src/java.base/share/classes/java/lang/StringConcatHelper.java:

I think bounds checks in StringConcatHelper.prepend() are skipped
intentionally, since java.lang.invoke.StringConcatFactory
constructs
method handle chains which already contain bounds checks: array
length
is precomputed based on argument values and all accesses are
guaranteed to be in bounds.



This is calling the trusted version of getChars() with no bounds
checks.  It was a little more obvious when I had the Trusted inner
class.



src/java.base/share/classes/java/lang/StringUTF16.java:

+static void putChar(byte[] val, int index, int c) {
+assert index >= 0 && index < length(val) : "Trusted 
caller

missed bounds check";

Unfortunately, asserts can affect inlining decisions (since they
increase bytecode size). In order to minimize possible 
performance

impact, I suggest to remove them from the fix targeting 9.



Sure.



 private static int indexOfSupplementary(byte[] value, int
ch, int
fromIndex, int max) {
 if (Character.isValidCodePoint(ch)) {
 final char hi = Character.highSurrogate(ch);
 final char lo = Character.lowSurrogate(ch);
+checkBoundsBeginEnd(fromIndex, max, value);

The check is redundant here. fromIndex & max are always 
inbounds by

construction:

public static int indexOf(byte[] 

Re: [9] RFR(L) 8158168: SIGSEGV: CollectedHeap::fill_with_objects(HeapWord*, unsigned long, bool)+0xa8

[dean . long]

Also, it looks like the changes I made to ASB.appendChars(char[] s, int 
off, int end) are not needed.


dl


On 3/22/17 11:01 AM, dean.l...@oracle.com wrote:
Vladimir, don't you need to replace checkIndex with checkOffset in 
indexOf and lastIndexOf, so that we allow count == length?


dl


On 3/22/17 8:35 AM, Vladimir Ivanov wrote:

So are we convinced that the proposed changes will never lead to a
crash due to a missing or incorrect bounds check, due to a racy 
use of

an unsynchronized ASB instance e.g. StringBuilder?


If only we had a static analysis tool that could tell us if the 
code is

safe.  Because we don't, in my initial changeset, we always take a
snapshot of the ASB fields by passing those field values to 
StringUTF16
before doing checks on them.  And I wrote a test to make sure that 
those

StringUTF16 interfaces are catching all the underflows and overflows I
could imagine, and I added verification code to detect when a check 
was

missed.

However, all the reviewers have requested to minimize the amount of
changes.  In Vladimir's version, if there is a missing check 
somewhere,

then yes it could lead to a crash.


I'd like to point out that asserts and verification code are disabled 
by default. They are invaluable during problem diagnosis, but don't 
help at all from defence-in-depth perspective.


But I agree that it's easier to reason about and test the initial 
version of the fix.


I wonder if the reviewers have fully realized the potential impact 
here?
This has exposed a flaw in the way intrinsics are used from core 
classes.


FTR here are the checks I omitted in the minimized version (modulo 
separation of indexOf/lastIndexOf for trusted/non-trusted callers):


http://cr.openjdk.java.net/~vlivanov/dlong/8158168/redundant_checks/

Other than that, the difference is mainly about undoing refactorings 
and removing verification logic (asserts + checks in the JVM).


There are still unsafe accesses which are considered safe in both 
versions (see StringUTF16.Trusted usages in the initial version [1]).


We used to provide safe wrappers for unsafe intrinsics which makes it 
much easier to reason about code correctness. I'd like to see compact 
string code refactored that way and IMO the initial version by Dean 
is a big step in the right direction.


I still prefer to see a point fix in 9 and major refactoring 
happening in 10, but I'll leave the decision on how to proceed with 
the fix to core-libs folks. After finishing the exercise minimizing 
the fix, I'm much more comfortable with the initial fix [1] (though 
there are changes I consider excessive).


Best regards,
Vladimir Ivanov

[1] http://cr.openjdk.java.net/~dlong/8158168/webrev.0


Some clarifications:


src/java.base/share/classes/java/lang/String.java:

The bounds check is needed only in String.nonSyncContentEquals 
when it

extracts info from AbstractStringBuilder. I don't see how out of
bounds access can happen in String.contentEquals:
 if (n != length()) {
 return false;
 }
...
 for (int i = 0; i < n; i++) {
 if (StringUTF16.getChar(val, i) != cs.charAt(i)) {



OK.



src/java.base/share/classes/java/lang/StringConcatHelper.java:

I think bounds checks in StringConcatHelper.prepend() are skipped
intentionally, since java.lang.invoke.StringConcatFactory 
constructs
method handle chains which already contain bounds checks: array 
length

is precomputed based on argument values and all accesses are
guaranteed to be in bounds.



This is calling the trusted version of getChars() with no bounds
checks.  It was a little more obvious when I had the Trusted inner
class.



src/java.base/share/classes/java/lang/StringUTF16.java:

+static void putChar(byte[] val, int index, int c) {
+assert index >= 0 && index < length(val) : "Trusted caller
missed bounds check";

Unfortunately, asserts can affect inlining decisions (since they
increase bytecode size). In order to minimize possible performance
impact, I suggest to remove them from the fix targeting 9.



Sure.



 private static int indexOfSupplementary(byte[] value, int 
ch, int

fromIndex, int max) {
 if (Character.isValidCodePoint(ch)) {
 final char hi = Character.highSurrogate(ch);
 final char lo = Character.lowSurrogate(ch);
+checkBoundsBeginEnd(fromIndex, max, value);

The check is redundant here. fromIndex & max are always inbounds by
construction:

public static int indexOf(byte[] value, int ch, int 
fromIndex) {

int max = value.length >> 1;
if (fromIndex < 0) {
fromIndex = 0;
} else if (fromIndex >= max) {
// Note: fromIndex might be near -1>>>1.
return -1;
}
...
return indexOfSupplementary(value, ch, fromIndex, max);



OK.



I moved bounds checks from StringUTF16.lastIndexOf/indexOf to

Re: [9] RFR(L) 8158168: SIGSEGV: CollectedHeap::fill_with_objects(HeapWord*, unsigned long, bool)+0xa8

[dean . long]

Vladimir, don't you need to replace checkIndex with checkOffset in 
indexOf and lastIndexOf, so that we allow count == length?


dl


On 3/22/17 8:35 AM, Vladimir Ivanov wrote:

So are we convinced that the proposed changes will never lead to a
crash due to a missing or incorrect bounds check, due to a racy use of
an unsynchronized ASB instance e.g. StringBuilder?


If only we had a static analysis tool that could tell us if the code is
safe.  Because we don't, in my initial changeset, we always take a
snapshot of the ASB fields by passing those field values to StringUTF16
before doing checks on them.  And I wrote a test to make sure that 
those

StringUTF16 interfaces are catching all the underflows and overflows I
could imagine, and I added verification code to detect when a check was
missed.

However, all the reviewers have requested to minimize the amount of
changes.  In Vladimir's version, if there is a missing check somewhere,
then yes it could lead to a crash.


I'd like to point out that asserts and verification code are disabled 
by default. They are invaluable during problem diagnosis, but don't 
help at all from defence-in-depth perspective.


But I agree that it's easier to reason about and test the initial 
version of the fix.



I wonder if the reviewers have fully realized the potential impact here?
This has exposed a flaw in the way intrinsics are used from core 
classes.


FTR here are the checks I omitted in the minimized version (modulo 
separation of indexOf/lastIndexOf for trusted/non-trusted callers):


http://cr.openjdk.java.net/~vlivanov/dlong/8158168/redundant_checks/

Other than that, the difference is mainly about undoing refactorings 
and removing verification logic (asserts + checks in the JVM).


There are still unsafe accesses which are considered safe in both 
versions (see StringUTF16.Trusted usages in the initial version [1]).


We used to provide safe wrappers for unsafe intrinsics which makes it 
much easier to reason about code correctness. I'd like to see compact 
string code refactored that way and IMO the initial version by Dean is 
a big step in the right direction.


I still prefer to see a point fix in 9 and major refactoring happening 
in 10, but I'll leave the decision on how to proceed with the fix to 
core-libs folks. After finishing the exercise minimizing the fix, I'm 
much more comfortable with the initial fix [1] (though there are 
changes I consider excessive).


Best regards,
Vladimir Ivanov

[1] http://cr.openjdk.java.net/~dlong/8158168/webrev.0


Some clarifications:


src/java.base/share/classes/java/lang/String.java:

The bounds check is needed only in String.nonSyncContentEquals 
when it

extracts info from AbstractStringBuilder. I don't see how out of
bounds access can happen in String.contentEquals:
 if (n != length()) {
 return false;
 }
...
 for (int i = 0; i < n; i++) {
 if (StringUTF16.getChar(val, i) != cs.charAt(i)) {



OK.



src/java.base/share/classes/java/lang/StringConcatHelper.java:

I think bounds checks in StringConcatHelper.prepend() are skipped
intentionally, since java.lang.invoke.StringConcatFactory constructs
method handle chains which already contain bounds checks: array 
length

is precomputed based on argument values and all accesses are
guaranteed to be in bounds.



This is calling the trusted version of getChars() with no bounds
checks.  It was a little more obvious when I had the Trusted inner
class.



src/java.base/share/classes/java/lang/StringUTF16.java:

+static void putChar(byte[] val, int index, int c) {
+assert index >= 0 && index < length(val) : "Trusted caller
missed bounds check";

Unfortunately, asserts can affect inlining decisions (since they
increase bytecode size). In order to minimize possible performance
impact, I suggest to remove them from the fix targeting 9.



Sure.



 private static int indexOfSupplementary(byte[] value, int 
ch, int

fromIndex, int max) {
 if (Character.isValidCodePoint(ch)) {
 final char hi = Character.highSurrogate(ch);
 final char lo = Character.lowSurrogate(ch);
+checkBoundsBeginEnd(fromIndex, max, value);

The check is redundant here. fromIndex & max are always inbounds by
construction:

public static int indexOf(byte[] value, int ch, int fromIndex) {
int max = value.length >> 1;
if (fromIndex < 0) {
fromIndex = 0;
} else if (fromIndex >= max) {
// Note: fromIndex might be near -1>>>1.
return -1;
}
...
return indexOfSupplementary(value, ch, fromIndex, max);



OK.



I moved bounds checks from StringUTF16.lastIndexOf/indexOf to
ABS.indexOf/lastIndexOf. I think it's enough to do range check on
ABS.value & ABS.count. After that, all accesses should be 
inbounds by

construction (in String.indexOf/lastIndexOf):

Re: [9] RFR(L) 8158168: SIGSEGV: CollectedHeap::fill_with_objects(HeapWord*, unsigned long, bool)+0xa8

[dean . long]

On 3/21/17 5:02 PM, David Holmes wrote:

I haven't been looking at the details of this but have been watching 
from afar. As per my comments in the bug report (now public) I'm quite 
concerned about the thread-non-safety issue here ...


On 22/03/2017 4:47 AM, dean.l...@oracle.com wrote:

On 3/21/17 9:37 AM, Vladimir Ivanov wrote:


and webrev.2 with it removed:

http://cr.openjdk.java.net/~dlong/8158168/webrev.2/


Thanks, Dean. I started with webrev.2 and tried to minimize the
changes. I ended up with the following version:

http://cr.openjdk.java.net/~vlivanov/dlong/8158168/webrev.00/



Thanks.  The reason I didn't go with that approach from the beginning is
because I couldn't convince myself that I could find all the missing
bounds checks, and I wanted an interface to test against.  With the
bounds checks in AbstractStringBuilder, it is very hard to test all the
possible race conditions, because some of the race conditions only
happen when an ASB field changes half-way through the method.


So are we convinced that the proposed changes will never lead to a 
crash due to a missing or incorrect bounds check, due to a racy use of 
an unsynchronized ASB instance e.g. StringBuilder?




If only we had a static analysis tool that could tell us if the code is 
safe.  Because we don't, in my initial changeset, we always take a 
snapshot of the ASB fields by passing those field values to StringUTF16 
before doing checks on them.  And I wrote a test to make sure that those 
StringUTF16 interfaces are catching all the underflows and overflows I 
could imagine, and I added verification code to detect when a check was 
missed.


However, all the reviewers have requested to minimize the amount of 
changes.  In Vladimir's version, if there is a missing check somewhere, 
then yes it could lead to a crash.



dl


Thanks,
David
-


Some clarifications:


src/java.base/share/classes/java/lang/String.java:

The bounds check is needed only in String.nonSyncContentEquals when it
extracts info from AbstractStringBuilder. I don't see how out of
bounds access can happen in String.contentEquals:
 if (n != length()) {
 return false;
 }
...
 for (int i = 0; i < n; i++) {
 if (StringUTF16.getChar(val, i) != cs.charAt(i)) {



OK.



src/java.base/share/classes/java/lang/StringConcatHelper.java:

I think bounds checks in StringConcatHelper.prepend() are skipped
intentionally, since java.lang.invoke.StringConcatFactory constructs
method handle chains which already contain bounds checks: array length
is precomputed based on argument values and all accesses are
guaranteed to be in bounds.



This is calling the trusted version of getChars() with no bounds
checks.  It was a little more obvious when I had the Trusted inner 
class.




src/java.base/share/classes/java/lang/StringUTF16.java:

+static void putChar(byte[] val, int index, int c) {
+assert index >= 0 && index < length(val) : "Trusted caller
missed bounds check";

Unfortunately, asserts can affect inlining decisions (since they
increase bytecode size). In order to minimize possible performance
impact, I suggest to remove them from the fix targeting 9.



Sure.



 private static int indexOfSupplementary(byte[] value, int ch, int
fromIndex, int max) {
 if (Character.isValidCodePoint(ch)) {
 final char hi = Character.highSurrogate(ch);
 final char lo = Character.lowSurrogate(ch);
+checkBoundsBeginEnd(fromIndex, max, value);

The check is redundant here. fromIndex & max are always inbounds by
construction:

public static int indexOf(byte[] value, int ch, int fromIndex) {
int max = value.length >> 1;
if (fromIndex < 0) {
fromIndex = 0;
} else if (fromIndex >= max) {
// Note: fromIndex might be near -1>>>1.
return -1;
}
...
return indexOfSupplementary(value, ch, fromIndex, max);



OK.



I moved bounds checks from StringUTF16.lastIndexOf/indexOf to
ABS.indexOf/lastIndexOf. I think it's enough to do range check on
ABS.value & ABS.count. After that, all accesses should be inbounds by
construction (in String.indexOf/lastIndexOf):

jdk/src/java.base/share/classes/java/lang/StringUTF16.java:
static int lastIndexOf(byte[] src, byte srcCoder, int srcCount,
   String tgtStr, int fromIndex) {

int rightIndex = srcCount - tgtCount;
if (fromIndex > rightIndex) {
fromIndex = rightIndex;
}
if (fromIndex < 0) {
return -1;
}

jdk/src/java.base/share/classes/java/lang/StringUTF16.java:
public static int lastIndexOf(byte[] src, int srcCount,
  byte[] tgt, int tgtCount, int
fromIndex) {
int min = tgtCount - 1;
int i = min + fromIndex;
int strLastIndex = tgtCount - 1;
char 

Re: [9] RFR(L) 8158168: SIGSEGV: CollectedHeap::fill_with_objects(HeapWord*, unsigned long, bool)+0xa8

[dean . long]

On 3/21/17 9:37 AM, Vladimir Ivanov wrote:


and webrev.2 with it removed:

http://cr.openjdk.java.net/~dlong/8158168/webrev.2/


Thanks, Dean. I started with webrev.2 and tried to minimize the 
changes. I ended up with the following version:


  http://cr.openjdk.java.net/~vlivanov/dlong/8158168/webrev.00/



Thanks.  The reason I didn't go with that approach from the beginning is 
because I couldn't convince myself that I could find all the missing 
bounds checks, and I wanted an interface to test against.  With the 
bounds checks in AbstractStringBuilder, it is very hard to test all the 
possible race conditions, because some of the race conditions only 
happen when an ASB field changes half-way through the method.



Some clarifications:


src/java.base/share/classes/java/lang/String.java:

The bounds check is needed only in String.nonSyncContentEquals when it 
extracts info from AbstractStringBuilder. I don't see how out of 
bounds access can happen in String.contentEquals:

 if (n != length()) {
 return false;
 }
...
 for (int i = 0; i < n; i++) {
 if (StringUTF16.getChar(val, i) != cs.charAt(i)) {



OK.



src/java.base/share/classes/java/lang/StringConcatHelper.java:

I think bounds checks in StringConcatHelper.prepend() are skipped 
intentionally, since java.lang.invoke.StringConcatFactory constructs 
method handle chains which already contain bounds checks: array length 
is precomputed based on argument values and all accesses are 
guaranteed to be in bounds.




This is calling the trusted version of getChars() with no bounds 
checks.  It was a little more obvious when I had the Trusted inner class.




src/java.base/share/classes/java/lang/StringUTF16.java:

+static void putChar(byte[] val, int index, int c) {
+assert index >= 0 && index < length(val) : "Trusted caller 
missed bounds check";


Unfortunately, asserts can affect inlining decisions (since they 
increase bytecode size). In order to minimize possible performance 
impact, I suggest to remove them from the fix targeting 9.




Sure.



 private static int indexOfSupplementary(byte[] value, int ch, int 
fromIndex, int max) {

 if (Character.isValidCodePoint(ch)) {
 final char hi = Character.highSurrogate(ch);
 final char lo = Character.lowSurrogate(ch);
+checkBoundsBeginEnd(fromIndex, max, value);

The check is redundant here. fromIndex & max are always inbounds by 
construction:


public static int indexOf(byte[] value, int ch, int fromIndex) {
int max = value.length >> 1;
if (fromIndex < 0) {
fromIndex = 0;
} else if (fromIndex >= max) {
// Note: fromIndex might be near -1>>>1.
return -1;
}
...
return indexOfSupplementary(value, ch, fromIndex, max);



OK.



I moved bounds checks from StringUTF16.lastIndexOf/indexOf to 
ABS.indexOf/lastIndexOf. I think it's enough to do range check on 
ABS.value & ABS.count. After that, all accesses should be inbounds by 
construction (in String.indexOf/lastIndexOf):


jdk/src/java.base/share/classes/java/lang/StringUTF16.java:
static int lastIndexOf(byte[] src, byte srcCoder, int srcCount,
   String tgtStr, int fromIndex) {

int rightIndex = srcCount - tgtCount;
if (fromIndex > rightIndex) {
fromIndex = rightIndex;
}
if (fromIndex < 0) {
return -1;
}

jdk/src/java.base/share/classes/java/lang/StringUTF16.java:
public static int lastIndexOf(byte[] src, int srcCount,
  byte[] tgt, int tgtCount, int 
fromIndex) {

int min = tgtCount - 1;
int i = min + fromIndex;
int strLastIndex = tgtCount - 1;
char strLastChar = getChar(tgt, strLastIndex);

startSearchForLastChar:
while (true) {
while (i >= min && getChar(src, i) != strLastChar) {

There are 2 places:
  * getChar(tgt, strLastIndex) => getChar(tgt, tgtCount-1) - inbound

  * getChar(src, i); i in [ min; min+fromIndex ]
min = tgtCount - 1
rightIndex = srcCount - tgtCount
fromIndex <= rightIndex

   0 <= min + fromIndex <= min + rightIndex == (tgtCount - 1) 
+ (srcCount - tgtCount) == srcCount - 1


Hence, should be covered by the check on count & value:
  public int lastIndexOf(String str, int fromIndex) {
+ byte[] value = this.value;
+ int count = this.count;
+ byte coder = this.coder;
+ checkIndex(count, value.length >> coder);
  return String.lastIndexOf(value, coder, count, str, fromIndex);
  }



OK, I will go with your version if it's OK with Sherman.

dl


Best regards,
Vladimir Ivanov


On 3/17/17 5:58 AM, Vladimir Ivanov wrote:



I have the same concern. Can we fix the immediate problem in 9 and
integrate verification logic in 10?

Re: [9] RFR(L) 8158168: SIGSEGV: CollectedHeap::fill_with_objects(HeapWord*, unsigned long, bool)+0xa8

[dean . long]

On 3/20/17 11:10 PM, Xueming Shen wrote:


Hi Dean,

Thanks for doing this.

Though as I suggested last time personally I prefer to make minimum 
change to simply
seal those holes in ASB at this late stage of JDK9. I'm fine with the 
webrev.2 and it looks

better and reasonable to pull all UTF16 operations into StringUTF16.java.

Just for my curiosity, does the change in String#1810 make difference?



Yes, it's in the spirit of the comment "return immediately where 
possible", and allows me to have


474 assert fromIndex >= 0;

in StringUTF16.lastIndexOf(). This is because fromIndex can go negative 
at String#1808.


Thanks for the review.

dl


Thanks!
Sherman



On 3/17/17, 3:07 PM, dean.l...@oracle.com wrote:

I posted two new versions, webrev.1 keeping the Trusted inner class:

http://cr.openjdk.java.net/~dlong/8158168/webrev.1/

and webrev.2 with it removed:

http://cr.openjdk.java.net/~dlong/8158168/webrev.2/

dl

On 3/17/17 5:58 AM, Vladimir Ivanov wrote:



I have the same concern. Can we fix the immediate problem in 9 and
integrate verification logic in 10?



OK, Tobias is suggesting having verification logic only inside the
intrinsics.  Are you suggesting removing that as well?


Yes and put them back in 10.


I'm OK with removing all the verification, but that won't reduce the
library changes much.  I could undo the renaming to 
Trusted.getChar, but

we would still have the bounds checks moved into StringUTF16.


I suggest to go with a point fix for 9: just add missing range checks. 

Re: [9] RFR(L) 8158168: SIGSEGV: CollectedHeap::fill_with_objects(HeapWord*, unsigned long, bool)+0xa8

[dean . long]

I posted two new versions, webrev.1 keeping the Trusted inner class:

http://cr.openjdk.java.net/~dlong/8158168/webrev.1/

and webrev.2 with it removed:

http://cr.openjdk.java.net/~dlong/8158168/webrev.2/

dl

On 3/17/17 5:58 AM, Vladimir Ivanov wrote:



I have the same concern. Can we fix the immediate problem in 9 and
integrate verification logic in 10?



OK, Tobias is suggesting having verification logic only inside the
intrinsics.  Are you suggesting removing that as well?


Yes and put them back in 10.


I'm OK with removing all the verification, but that won't reduce the
library changes much.  I could undo the renaming to Trusted.getChar, but
we would still have the bounds checks moved into StringUTF16.


I suggest to go with a point fix for 9: just add missing range checks. 

Re: [9] RFR(L) 8158168: SIGSEGV: CollectedHeap::fill_with_objects(HeapWord*, unsigned long, bool)+0xa8

[dean . long]

On 3/17/17 5:58 AM, Vladimir Ivanov wrote:




I have the same concern. Can we fix the immediate problem in 9 and
integrate verification logic in 10?



OK, Tobias is suggesting having verification logic only inside the
intrinsics.  Are you suggesting removing that as well?


Yes and put them back in 10.


OK.




I'm OK with removing all the verification, but that won't reduce the
library changes much.  I could undo the renaming to Trusted.getChar, but
we would still have the bounds checks moved into StringUTF16.


I suggest to go with a point fix for 9: just add missing range checks.

Is AbstractStringBuilder.append() the only affected method? (Sorry, 
it's hard to say exactly where the problem is by looking at the diff.)




In the failing test, yes, it was append, but when I went to fix the 
problem I found that it was much more wide spread, and there were 
several methods that were affected.


I really like the refactoring you propose on jdk side, but there are 
pieces I'm not sure about. For example, I spotted a repeated range check:


jdk/src/java.base/share/classes/java/lang/AbstractStringBuilder.java:
public void setCharAt(int index, char ch) {
checkIndex(index, count);
if (isLatin1() && StringLatin1.canEncode(ch)) {
value[index] = (byte)ch;
} else {
if (isLatin1()) {
inflate();
}
StringUTF16.putCharSB(value, index, ch);
}
}



OK, I did not look for redundant checks.  This check is actually not 
redundant.  The "value" array may be oversized, so "count" is supposed 
to contain the current maximum.  For the safety of the intrinsic array 
access, we check against the array length, but for the API we need to be 
stricter and check against the character count.


However, the checkIndex() here is a good example of what is wrong. Let's 
say we were checking against value.length instead of "count". Even if 
checkIndex() succeeds here, based on the current length of "value", we 
can't trust it because the object is mutable and "value" can change 
between the checkIndex() and putCharSB().


dl


Best regards,
Vladimir Ivanov

Re: [9] RFR(L) 8158168: SIGSEGV: CollectedHeap::fill_with_objects(HeapWord*, unsigned long, bool)+0xa8

[dean . long]

On 3/16/17 2:52 AM, Tobias Hartmann wrote:

As a safety net, I added asserts around the intrinsic calls, and a try/catch 
that so any out of bounds exception turns into an assert error as well.

So the assert and try/catch are only necessary to catch invalid offsets passed 
to the C1 intrinsic, right? Interpreted code is safe and the C2 intrinsics have 
additional guards in debug builds.


I'm fine with that but an alternative would be to also have such guards in the 
C1 intrinsic. For example, one could enable the normal bound checks and add a 
simple check to Runtime1::throw_range_check_exception() that fails if the 
throwing method is the C1 intrinsic. Like this, we could avoid the assert, 
try-catch and DEBUG_INTRINSICS code.



On 3/16/17 11:09 AM, Vladimir Ivanov wrote:




The changes to the JDK core classes are quite extensive. This will need
rigorous functional and performance testing and it is very late in the
release cycle to make these kinds of changes. But I'll leave that to the
core-libs folk to comment on.


I have the same concern. Can we fix the immediate problem in 9 and 
integrate verification logic in 10?




OK, Tobias is suggesting having verification logic only inside the 
intrinsics.  Are you suggesting removing that as well?


I'm OK with removing all the verification, but that won't reduce the 
library changes much.  I could undo the renaming to Trusted.getChar, but 
we would still have the bounds checks moved into StringUTF16.


dl


Best regards,
Vladimir Ivanov

Re: [9] RFR(L) 8158168: SIGSEGV: CollectedHeap::fill_with_objects(HeapWord*, unsigned long, bool)+0xa8

[dean . long]

On 3/15/17 6:19 PM, David Holmes wrote:


src/share/vm/classfile/javaClasses.hpp

Given the the only call to java_lang_String::set_debug_intrinsics is 
within an ifdef, shouldn't the declaration and definition of the 
method also be guarded the same way? 


OK I'll change it.

dl

Re: RFR: 8073093: AARCH64: C2 generates poor code for ByteBuffer accesses

[Dean Long]

On 2/14/2015 12:07 AM, Andrew Haley wrote:

On 02/13/2015 10:52 PM, Dean Long wrote:


My understanding is that whether or not aarch64 allows unaligned
accesses is based on a system setting, so this change is too
simplistic.

Disabling unaligned access would be a really perverse thing to do, and
I suspect that GCC and glibc already assume that unaligned accesses
work so it would require a recompilation of libjvm (and probably the
whole OS) to make it work.  However, if you really think there's a
point to making this a runtime flag I won't resist.

Andrew.


Even if linux-aarch64 always allows unaligned, checking only for 
aarch64 is not future-proof
because it doesn't take the OS into account.  However, I really don't 
like having to enumerate
all relevant platforms in multiple places in shared code, so I disagree 
with the existing code
and with perpetuating the pattern.  As long as the decision is in 
platform-specific code, a build-time

decision may be entirely appropriate.

dl

Re: RFR: 8073093: AARCH64: C2 generates poor code for ByteBuffer accesses

[Dean Long]

My understanding is that whether or not aarch64 allows unaligned 
accesses is based on a
system setting, so this change is too simplistic.  I would prefer that 
this was controlled with

something more flexible, like sun.cpu.unaligned.

dl

On 2/13/2015 5:38 AM, Andrew Haley wrote:

java.​nio.​DirectByteBuffer.getXXX is slow for types larger than byte
because the runtime does not know that AArch64 can perform unaligned
memory accesses.

The problem is due to this code in java.nio.Bits.unaligned():

 unaligned = arch.equals(i386) || arch.equals(x86)
 || arch.equals(amd64) || arch.equals(x86_64);

If we add AArch64 to this list code quality is very much improved.

http://cr.openjdk.java.net/~aph/8073093/

Thanks,
Andrew.

Re: RFR: 8073093: AARCH64: C2 generates poor code for ByteBuffer accesses

[Dean Long]

There is a system register bit to read, but I don't think it can be 
accessed by an application, only the kernel.
If the OS won't provide this information, you could do something similar 
to safeFetchN and catch the

resulting SIGBUS.

dl

On 2/13/2015 4:05 PM, Vladimir Kozlov wrote:
x86 has flag UseUnalignedLoadStores which is set to true depending on 
which version of CPU VM runs. The CPU version is determined based on 
CPUID instruction results.


Does AARCH64 has something similar?

Regards,
Vladimir

On 2/13/15 3:41 PM, Dean Long wrote:

On 2/13/2015 3:04 PM, chris...@zoulas.com wrote:

On Feb 13,  2:52pm, dean.l...@oracle.com (Dean Long) wrote:
-- Subject: Re: RFR: 8073093: AARCH64: C2 generates poor code for
ByteBuffer

| My understanding is that whether or not aarch64 allows unaligned=20
| accesses is based on a
| system setting, so this change is too simplistic.  I would prefer
that=20
| this was controlled with
| something more flexible, like sun.cpu.unaligned.

So does x86_64 and you can ask the CPU if it is enabled... I am not 
sure

if a variable setting makes sense because if alignment is required you
get a signal (BUS error -- hi linux, SEGV), or incorrect results.

christos


So it sounds like we need to determine if unaligned accesses are
supported during startup,
in a platform-specific way.  This could be exposed through a property
like I suggested,
or perhaps a new Unsafe method.

Regarding x86_64, there may be places in the JVM that already assume
unaligned accesses
are allowed, so disabling them may completely break the JVM until those
assumptions
are fixed.

dl

Re: RFR: 8073093: AARCH64: C2 generates poor code for ByteBuffer accesses

[Dean Long]

On 2/13/2015 3:04 PM, chris...@zoulas.com wrote:

On Feb 13,  2:52pm, dean.l...@oracle.com (Dean Long) wrote:
-- Subject: Re: RFR: 8073093: AARCH64: C2 generates poor code for ByteBuffer

| My understanding is that whether or not aarch64 allows unaligned=20
| accesses is based on a
| system setting, so this change is too simplistic.  I would prefer that=20
| this was controlled with
| something more flexible, like sun.cpu.unaligned.

So does x86_64 and you can ask the CPU if it is enabled... I am not sure
if a variable setting makes sense because if alignment is required you
get a signal (BUS error -- hi linux, SEGV), or incorrect results.

christos


So it sounds like we need to determine if unaligned accesses are 
supported during startup,
in a platform-specific way.  This could be exposed through a property 
like I suggested,

or perhaps a new Unsafe method.

Regarding x86_64, there may be places in the JVM that already assume 
unaligned accesses
are allowed, so disabling them may completely break the JVM until those 
assumptions

are fixed.

dl

Re: RFC 7038914: VM could throw uncaught OOME in ReferenceHandler thread

[Dean Long]

If you really want to bullet-proof ReferenceHandler (and other system 
threads) against OOME caused by native allocations,
then don't forget about monitor inflation if there is contention for 
lock :-)


dl

On 5/10/2013 6:14 AM, David Holmes wrote:

Hi Peter,

So it would appear that it is not in fact the new that causes the 
OOME but the classloading of InterruptedException ?


I'm not sure I can quite get my head around this late on a Friday 
night :)


David

On 10/05/2013 9:21 PM, Peter Levart wrote:

On 05/10/2013 12:52 PM, Peter Levart wrote:

While executing the above test with the patch to ReferenceHandler
applied, I noticed a strange behaviour. I can reproduce this behaviour
reliably on both JDK7 and JDK8. When the patch is applied as proposed:

try {
lock.wait();
} catch (InterruptedException |
OutOfMemoryError  x) { }

... I still get the following output from the test (reliably, always):

Exception: java.lang.OutOfMemoryError thrown from the
UncaughtExceptionHandler in thread Reference Handler
Exception in thread main java.lang.Exception: Reference Handler
thread died.
at OOMEInReferenceHandler.main(OOMEInReferenceHandler.java:80)

But when i change the patch to the following:

try {
lock.wait();
} catch (OutOfMemoryError |
InterruptedException  x) { }

...the test reliably and always passes.

My explanation to his behaviour is that the order of exception
handlers changes the order of class referencing. In the former
variation (that still throws OOME) the following seems to be happening:

wait() is interrupted and InterruptedException instance creation is
attempted. Because this is the 1st reference to InterruptedException
class in the lifetime of the JVM, loading of InterruptedException
class is attempted which fails because of OOME. This OOME is caught by
handler and ignored. But after handling of this OOME, another
reference to InterruptedException class is attempted by exception
handlers themselves (I don't know how exception handlers work exactly,
but I have a feeling this is happening). Because InterruptedException
class was not successfully loaded the 1st time tried, every reference
to this class must throw NoClassDefFoundError, so this is attempted,
but creation of NoClassDefFoundError fails because there's no heap
space and another OOME is thrown - this time out of exception handling
block, which is propagated and kills the thread.

If the order of exception handlers is reversed, this second OOME is
caught and ignored.


Hi,

This really seems to be happening (at least approximately, see below)
because if InterruptedException class is preloaded at start of test, the
order of exception handling does not have any impact on test.

By disassembling the class-files of both variants, I found the only
difference is the order of OutOfMemoryError  InterruptedException
entries found in exception table:

catch (InterruptedException | OutOfMemoryError  x) variant has:

   public void run();
 Code:
0: invokestatic  #2  // Method
java/lang/ref/Reference.access$100:()Ljava/lang/ref/Reference$Lock;
3: dup
4: astore_2
5: monitorenter
6: invokestatic  #3  // Method
java/lang/ref/Reference.access$200:()Ljava/lang/ref/Reference;
9: ifnull33
   12: invokestatic  #3  // Method
java/lang/ref/Reference.access$200:()Ljava/lang/ref/Reference;
   15: astore_1
   16: aload_1
   17: invokestatic  #4  // Method
java/lang/ref/Reference.access$300:(Ljava/lang/ref/Reference;)Ljava/lang/ref/Reference; 


   20: invokestatic  #5  // Method
java/lang/ref/Reference.access$202:(Ljava/lang/ref/Reference;)Ljava/lang/ref/Reference; 


   23: pop
   24: aload_1
   25: aconst_null
   26: invokestatic  #6  // Method
java/lang/ref/Reference.access$302:(Ljava/lang/ref/Reference;Ljava/lang/ref/Reference;)Ljava/lang/ref/Reference; 


   29: pop
   30: goto  48
*  33: invokestatic  #2  // Method
java/lang/ref/Reference.access$100:()Ljava/lang/ref/Reference$Lock;**
**  36: invokevirtual #7  // Method
java/lang/Object.wait:()V**
**  39: goto  43*
   42: astore_3
   43: aload_2
   44: monitorexit
   45: goto  0
   48: aload_2
   49: monitorexit
   50: goto  60
   53: astore4
   55: aload_2
   56: monitorexit
   57: aload 4
   59: athrow
   60: aload_1
   61: instanceof#10 // class sun/misc/Cleaner
   64: ifeq  77
   67: aload_1
   68: checkcast #10 // class sun/misc/Cleaner
   71: invokevirtual #11 // Method
sun/misc/Cleaner.clean:()V
   74: goto 

Re: RFC 7038914: VM could throw uncaught OOME in ReferenceHandler thread

[Dean Long]

On 5/10/2013 1:22 PM, Peter Levart wrote:


On 05/10/2013 10:08 PM, Peter Levart wrote:


On 05/10/2013 08:10 PM, Dean Long wrote:
If you really want to bullet-proof ReferenceHandler (and other 
system threads) against OOME caused by native allocations,
then don't forget about monitor inflation if there is contention for 
lock :-)


Aren't monitors C++ objects? Are they allocated from java heap?

Regards, Peter


Hi Dean,

Ah, I see you're suggesting that native allocations can also trigger 
OOME in Java program. Is this true?




That's what I was suggesting, but in the case of object monitors, it 
looks like the VM will exit if it runs out.
Other places where we could fail to allocate native memory, such as 
class loading or adding to the system dictionary,
might not exit the VM, but instead result in some other exception such 
as NoSuchMethod or ClassNotFound.
I haven't investigated all the possibilities, but I just wanted to bring 
it up as something to consider.


dl


Regards, Peter





dl

On 5/10/2013 6:14 AM, David Holmes wrote:

Hi Peter,

So it would appear that it is not in fact the new that causes the 
OOME but the classloading of InterruptedException ?


I'm not sure I can quite get my head around this late on a Friday 
night :)


David

On 10/05/2013 9:21 PM, Peter Levart wrote:

On 05/10/2013 12:52 PM, Peter Levart wrote:

While executing the above test with the patch to ReferenceHandler
applied, I noticed a strange behaviour. I can reproduce this 
behaviour
reliably on both JDK7 and JDK8. When the patch is applied as 
proposed:


try {
lock.wait();
} catch (InterruptedException |
OutOfMemoryError  x) { }

... I still get the following output from the test (reliably, 
always):


Exception: java.lang.OutOfMemoryError thrown from the
UncaughtExceptionHandler in thread Reference Handler
Exception in thread main java.lang.Exception: Reference Handler
thread died.
at 
OOMEInReferenceHandler.main(OOMEInReferenceHandler.java:80)


But when i change the patch to the following:

try {
lock.wait();
} catch (OutOfMemoryError |
InterruptedException  x) { }

...the test reliably and always passes.

My explanation to his behaviour is that the order of exception
handlers changes the order of class referencing. In the former
variation (that still throws OOME) the following seems to be 
happening:


wait() is interrupted and InterruptedException instance creation is
attempted. Because this is the 1st reference to InterruptedException
class in the lifetime of the JVM, loading of InterruptedException
class is attempted which fails because of OOME. This OOME is 
caught by

handler and ignored. But after handling of this OOME, another
reference to InterruptedException class is attempted by exception
handlers themselves (I don't know how exception handlers work 
exactly,
but I have a feeling this is happening). Because 
InterruptedException
class was not successfully loaded the 1st time tried, every 
reference

to this class must throw NoClassDefFoundError, so this is attempted,
but creation of NoClassDefFoundError fails because there's no heap
space and another OOME is thrown - this time out of exception 
handling

block, which is propagated and kills the thread.

If the order of exception handlers is reversed, this second OOME is
caught and ignored.


Hi,

This really seems to be happening (at least approximately, see below)
because if InterruptedException class is preloaded at start of 
test, the

order of exception handling does not have any impact on test.

By disassembling the class-files of both variants, I found the only
difference is the order of OutOfMemoryError  InterruptedException
entries found in exception table:

catch (InterruptedException | OutOfMemoryError  x) variant has:

   public void run();
 Code:
0: invokestatic  #2  // Method
java/lang/ref/Reference.access$100:()Ljava/lang/ref/Reference$Lock;
3: dup
4: astore_2
5: monitorenter
6: invokestatic  #3  // Method
java/lang/ref/Reference.access$200:()Ljava/lang/ref/Reference;
9: ifnull33
   12: invokestatic  #3  // Method
java/lang/ref/Reference.access$200:()Ljava/lang/ref/Reference;
   15: astore_1
   16: aload_1
   17: invokestatic  #4  // Method
java/lang/ref/Reference.access$300:(Ljava/lang/ref/Reference;)Ljava/lang/ref/Reference; 


   20: invokestatic  #5  // Method
java/lang/ref/Reference.access$202:(Ljava/lang/ref/Reference;)Ljava/lang/ref/Reference; 


   23: pop
   24: aload_1
   25: aconst_null
   26: invokestatic  #6  // Method
java/lang/ref/Reference.access$302:(Ljava/lang/ref/Reference;Ljava/lang/ref/Reference;)Ljava/lang/ref/Reference; 


   29: pop
   30: goto  48

Re: Request for review- RFE 8005716

[Dean Long]

On 3/7/2013 10:18 AM, Jeremy Manson wrote:

Hi guys,

I'm really glad to see you are doing this.  We've done something 
similar to good effect within Google (and we'll probably drop our 
similar, internal patch and pick up this patch once it is pushed).


Have you thought about support for having a JNI_OnLoad inside of a 
main executable that *doesn't* have a postfixed _lib?  Our 
Google-internal patch treats the main executable as a regular JNI 
library if you execute a special System.loadFromLauncher() function. 
 We also do the same thing with JVMTI.


Would that require additional changes to the JNI spec?  Is there an 
advantage to treating it as an unnamed library rather than

a named library (i.e. JNI_OnLoad_main)?

dl


Nit: The comments that read like this:
  * for example, L, and a native library called L is statically linked
  * with the VM, then the JNI_OnLoad_L function exported by the library
Should these read linked with the VM, or linked with the native 
application loading the VM?


Jeremy


On Wed, Mar 6, 2013 at 8:21 AM, Bob Vandette bob.vande...@oracle.com 
mailto:bob.vande...@oracle.com wrote:



On Mar 5, 2013, at 7:36 PM, Dean Long wrote:

 If JNI_ONLOAD_SYMBOLS contains something like _JNI_OnLoad@8 on
Windows, you can't just
 turn that into _JNI_OnLoad@8_ + libname.  I think it needs to be
 _JNI_OnLoad_  + libname + @8

Good catch Dean.


 Looks like onLoadSymbols[] is unused in
Java_java_lang_ClassLoader_00024NativeLibrary_findBuiltinLib().

 Instead of adding getProcessHandle(), why not add
JVM_FindBuiltinLibraryEntry() instead?
 This would make it easier to support table-lookup when runtime
symbol information is missing or not
 supported by the platform.

Bill has already factored out the implementation of
getProcessHandle.  Although your
approach is cleaner, I'm concerned about creating a VM version
dependency.   For a traditional
JRE that doesn't even require static library support, we'd have to
make sure to run on a VM that
support JNI_VERSION_1_8.  It looks like the JDK maintains their
own copy of jni.h.  If they didn't
do that, we would have already gone down that path.   The jdk
sources already contain several
instances of dlopen, dlysym and the Windows equivalents so I don't
see a need to add a new
VM function.

Bob.


 dl

 On 3/5/2013 3:05 PM, bill.pitt...@oracle.com
mailto:bill.pitt...@oracle.com wrote:
 This request is tied to bugid 8005716 that deals with adding
support for statically linked JNI libraries.

 The JEP is here: http://openjdk.java.net/jeps/178

 The bug is here:http://bugs.sun.com/view_bug.do?bug_id=8005716

 The webrevs are here:

 http://cr.openjdk.java.net/~bpittore/8005716/jdk-webrev.00/
http://cr.openjdk.java.net/%7Ebpittore/8005716/jdk-webrev.00/
 http://cr.openjdk.java.net/~bpittore/8005716/hs-webrev.00/
http://cr.openjdk.java.net/%7Ebpittore/8005716/hs-webrev.00/

 The main piece of functionality is to check for the presence of
JNI_OnLoad_libname to determine if the library specified by
'libname' has been statically linked into the VM. If the symbol is
found, it is assumed that the library is linked in and will not be
dynamically loaded.

 thanks,
 bill

Re: Request for review- RFE 8005716

[Dean Long]

On 3/5/2013 8:13 PM, BILL PITTORE wrote:

On 3/5/2013 7:36 PM, Dean Long wrote:
If JNI_ONLOAD_SYMBOLS contains something like _JNI_OnLoad@8 on 
Windows, you can't just

turn that into _JNI_OnLoad@8_ + libname.  I think it needs to be
_JNI_OnLoad_  + libname + @8
I'll look into that. When I built for windows and ran our test, the 
JNI_OnLoad_TestStaticLib was exported without the decoration just 
based on the JNIEXPORT JNICALL specifiers on the function. I didn't do 
anything special to export it. But I recall this problem from another 
project.
10 1014 JNI_OnLoad_TestStaticLib = 
@ILT+15(?JNI_OnLoad_TestStaticLib@@YGHPAUJavaVM_@@PAX@Z)


Looks like onLoadSymbols[] is unused in 
Java_java_lang_ClassLoader_00024NativeLibrary_findBuiltinLib().
I'll remove that. I redid the call to findBuiltinJniFunction but 
forgot to remove that.




Instead of adding getProcessHandle(), why not add 
JVM_FindBuiltinLibraryEntry() instead?
This would make it easier to support table-lookup when runtime symbol 
information is missing or not

supported by the platform.
Not sure I'm following you on this. Make JVM_FindBuiltinLibraryEntry() 
an exported function in the VM? How does it find JNI_OnLoad_L? 


For Windows and Linux, it would use basically your same code, just 
refactored a little.
By the way, did you consider using the special RTLD_DEFAULT handle 
instead of

dlopen(NULL)?

Via a table setup by the developer/build system when the library is 
linked in?




Yes, in previous projects we did exactly that for more exotic OSes.

dl


bill



dl

On 3/5/2013 3:05 PM, bill.pitt...@oracle.com wrote:
This request is tied to bugid 8005716 that deals with adding support 
for statically linked JNI libraries.


The JEP is here: http://openjdk.java.net/jeps/178

The bug is here:http://bugs.sun.com/view_bug.do?bug_id=8005716

The webrevs are here:

http://cr.openjdk.java.net/~bpittore/8005716/jdk-webrev.00/
http://cr.openjdk.java.net/~bpittore/8005716/hs-webrev.00/

The main piece of functionality is to check for the presence of 
JNI_OnLoad_libname to determine if the library specified by 
'libname' has been statically linked into the VM. If the symbol is 
found, it is assumed that the library is linked in and will not be 
dynamically loaded.


thanks,
bill

Re: Request for review- RFE 8005716

[Dean Long]

If JNI_ONLOAD_SYMBOLS contains something like _JNI_OnLoad@8 on 
Windows, you can't just

turn that into _JNI_OnLoad@8_ + libname.  I think it needs to be
_JNI_OnLoad_  + libname + @8

Looks like onLoadSymbols[] is unused in 
Java_java_lang_ClassLoader_00024NativeLibrary_findBuiltinLib().


Instead of adding getProcessHandle(), why not add 
JVM_FindBuiltinLibraryEntry() instead?
This would make it easier to support table-lookup when runtime symbol 
information is missing or not

supported by the platform.

dl

On 3/5/2013 3:05 PM, bill.pitt...@oracle.com wrote:
This request is tied to bugid 8005716 that deals with adding support 
for statically linked JNI libraries.


The JEP is here: http://openjdk.java.net/jeps/178

The bug is here:http://bugs.sun.com/view_bug.do?bug_id=8005716

The webrevs are here:

http://cr.openjdk.java.net/~bpittore/8005716/jdk-webrev.00/
http://cr.openjdk.java.net/~bpittore/8005716/hs-webrev.00/

The main piece of functionality is to check for the presence of 
JNI_OnLoad_libname to determine if the library specified by 'libname' 
has been statically linked into the VM. If the symbol is found, it is 
assumed that the library is linked in and will not be dynamically loaded.


thanks,
bill