Re: RFR JDK-8233527: Update Lookup::hasPrivateAccess and Lookup::defineClass spec w.r.t. full power lookup

[Mandy Chung]

On 11/13/19 11:45 AM, Johannes Kuhn wrote:
The two bullets about the caller lookup object must have MODULE and 
PRIVATE are important explanation for it to require such access.


Maybe I can add a bullet to say "The caller lookup object must have 
full privilege access" and then move those two bullets as sub-bullets 
under it.


What do you think?
This is a good idea, because full privilege access is a new concept, 
and should be mentioned at the places where it is used. 


Updated in place:

http://cr.openjdk.java.net/~mchung/jdk14/8233527/webrev.02/
http://cr.openjdk.java.net/~mchung/jdk14/8233527/specdiff/

Mandy

Re: RFR (M) 8223261 "JDK-8189208 followup - remove JDK_GetVersionInfo0 and the supporting code"

[Sergey Bylokhov]

Looks fine.

On 11/13/19 10:50 am, gerard ziemski wrote:

Hi all,

Please review this cleanup, where we remove JDK_GetVersionInfo0 and related 
code, since we can get build versions directly from within the VM itself:

I'm including core-libs and awt in this review because the proposed fix touches 
their corresponding files.


bug: https://bugs.openjdk.java.net/browse/JDK-8223261
webrev: http://cr.openjdk.java.net/~gziemski/8223261_rev1
tests: passes Mach5 tier1,2,3,4,5,6


cheers




--
Best regards, Sergey.

Re: RFR (M) 8223261 "JDK-8189208 followup - remove JDK_GetVersionInfo0 and the supporting code"

[David Holmes]

Hi Gerard,

On 14/11/2019 6:05 am, Langer, Christoph wrote:

Hi Gerard,

generally it looks like a nice cleanup.

I've got a patch prepared though, which I was planning on posting tomorrow. It 
is about cleanup for the canonicalize function in libjava. I wanted to use 
jdk_util.h for the function prototype. I had not yet filed a bug but here is 
what I have:
http://cr.openjdk.java.net/~clanger/webrevs/cleanup-canonicalize/

So maybe you could refrain from removing jdk_util.h or maybe you can hold off 
submitting your change until my cleanup is reviewed?


I'd also suggest not deleting jdk_util.h. It seems very odd to me to 
have jdk_util_md.h with no shared jdk_util.h. If you keep jdk_util.h 
then you don't need to touch a number of the files.


Otherwise this looks like a good cleanup.

Thanks,
David
-


I'll create a bug and post an official review thread tomorrow...

Thanks
Christoph


-Original Message-
From: hotspot-dev  On Behalf Of
Mandy Chung
Sent: Mittwoch, 13. November 2019 20:30
To: gerard ziemski 
Cc: awt-...@openjdk.java.net; hotspot-dev developers ; core-libs-dev@openjdk.java.net
Subject: Re: RFR (M) 8223261 "JDK-8189208 followup - remove
JDK_GetVersionInfo0 and the supporting code"



On 11/13/19 10:50 AM, gerard ziemski wrote:

Hi all,

Please review this cleanup, where we remove JDK_GetVersionInfo0 and
related code, since we can get build versions directly from within the
VM itself:

I'm including core-libs and awt in this review because the proposed
fix touches their corresponding files.


bug: https://bugs.openjdk.java.net/browse/JDK-8223261
webrev: http://cr.openjdk.java.net/~gziemski/8223261_rev1
tests: passes Mach5 tier1,2,3,4,5,6



This is a good clean up.  JDK_GetVersionInfo0 was needed long time ago
in particular in HS express support that is no longer applicable.

One leftover comment should also be removed.

src/hotspot/share/runtime/vm_version.hpp
    // Gets the jvm_version_info.jvm_version defined in jvm.h

otherwise looks good.

Mandy

RFR: JDK-8212780: JEP 343: Packaging Tool Implementation

[Andy Herrick]

Please review  changes for [1] which is the implementation bug for JEP-343.

The webrev at [2] is the total cumulative webrev of changes for the 
jpackage tool, currently in the JDK-8200758-branch branch of the open 
sandbox repository.


The webrev at [3] shows the changes since EA-06 (Build 13-jpackage+1-49 
) up to the current point.


The latest EA (Build 14-jpackage+1-49 ) is posted at [4].

Please send feedback to core-libs-dev@openjdk.java.net


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

[2] http://cr.openjdk.java.net/~herrick/8212780/webrev.EA-10/

[3] http://cr.openjdk.java.net/~herrick/8212780/webrev.06-10/

[4] http://jdk.java.net/jpackage/

Re: [PATCH] Enhancement proposal for usage of Method::getParameterTypes

[Сергей Цыпанов]

Hello,

thanks for quick review!

Tagir Valeev from JetBrains comes with suggestion to improve compiler 
optimizations here https://youtrack.jetbrains.com/issue/IDEA-226474.

Rationale: in fact the cloned array could be a subject of allocation erasure as 
any property dereferenced from it can be dereferenced from original array:

Consider this test:

@Test
void join() {
  final Object[] objects = new Object[3];
  objects[0] = "azaza";
  objects[1] = 365;
  objects[2] = 9876L;

  final Object[] clone = objects.clone();

  assertEquals(objects.length, clone.length);
  assertSame(objects[0], clone[0]);
  assertSame(objects[1], clone[1]);
  assertSame(objects[2], clone[2]);
}

Optimizing compiler could drop allocation of clone and substitute its usage 
with original array, but as previous benchmark demonstrates it doesn't do that 
at least on JDK 11.

Is there a way I can develop a patch for compiler and is this mailing list is 
appropriate place to discuss compiler issues?

Regards,
Sergey Tsypanov


09.11.2019, 01:15, "Claes Redestad" :
> Hi,
>
> some or all of these were pointed out as part of
> https://bugs.openjdk.java.net/browse/JDK-8029019
>
> There was a patch out for review early 2017. I'm not sure what happened
> to that?
>
> Either way I think it might make sense to get this small and trivial
> enhancement separated out and fixed.
>
> Thanks!
>
> /Claes
>
> On 2019-11-08 23:04, Сергей Цыпанов wrote:
>>  Hello,
>>
>>  it seems like Method.getParameterTypes() is often misused in JDK (and 
>> beyond): array returned from the method is used only to acquire number of 
>> method params by retrieving array.length.
>>  The problem here is that Method.getPatameterTypes() clones underlying array 
>> and returns the copy.
>>  Instead we can use Method.getParameterCount() which doesn't allocate any 
>> additional memory but returns directly the length of underlying array.
>>
>>  To measure probable performance difference I've created a benchmark for the 
>> most simple case when tested method has no parameters:
>>
>>  @State(Scope.Thread)
>>  @BenchmarkMode(Mode.AverageTime)
>>  @OutputTimeUnit(TimeUnit.NANOSECONDS)
>>  public class MethodToStringBenchmark {
>> private Method method;
>>
>> @Setup
>> public void setup() throws Exception { method = 
>> getClass().getMethod("toString"); }
>>
>> @Benchmark
>> public int getParameterCount() { return method.getParameterCount(); }
>>
>> @Benchmark
>> public int getParameterTypes() { return 
>> method.getParameterTypes().length; }
>>  }
>>
>>  on my i7-7700 with JDK 11 it produces these results:
>>
>>  Benchmark Mode Cnt Score Error Units
>>
>>  MethodToStringBenchmark.getParameterCount avgt 25 2,528 ± 0,085 ns/op
>>  MethodToStringBenchmark.getParameterCount:·gc.alloc.rate avgt 25 ≈ 10⁻⁴ 
>> MB/sec
>>  MethodToStringBenchmark.getParameterCount:·gc.alloc.rate.norm avgt 25 ≈ 
>> 10⁻⁷ B/op
>>  MethodToStringBenchmark.getParameterCount:·gc.count avgt 25 ≈ 0 counts
>>
>>  MethodToStringBenchmark.getParameterTypes avgt 25 7,299 ± 0,410 ns/op
>>  MethodToStringBenchmark.getParameterTypes:·gc.alloc.rate avgt 25 1999,454 ± 
>> 89,929 MB/sec
>>  MethodToStringBenchmark.getParameterTypes:·gc.alloc.rate.norm avgt 25 
>> 16,000 ± 0,001 B/op
>>  MethodToStringBenchmark.getParameterTypes:·gc.churn.G1_Eden_Space avgt 25 
>> 2003,360 ± 91,537 MB/sec
>>  MethodToStringBenchmark.getParameterTypes:·gc.churn.G1_Eden_Space.norm avgt 
>> 25 16,030 ± 0,045 B/op
>>  MethodToStringBenchmark.getParameterTypes:·gc.churn.G1_Old_Gen avgt 25 
>> 0,004 ± 0,001 MB/sec
>>  MethodToStringBenchmark.getParameterTypes:·gc.churn.G1_Old_Gen.norm avgt 25 
>> ≈ 10⁻⁵ B/op
>>  MethodToStringBenchmark.getParameterTypes:·gc.count avgt 25 2380,000 counts
>>  MethodToStringBenchmark.getParameterTypes:·gc.time avgt 25 1325,000 ms
>>
>>  I've prepared a small patch to replace usage of getParameterTypes() with 
>> getParameterCount() where appropriate in java.base module.
>>
>>  The patch is attached.
>>
>>  Regards,
>>  Sergey Tsypanov

Re: JDK 14 RFR of JDK-8233940: Preview API tests for String methods should use ${jdk.version} as -source arg

[Jonathan Gibbons]

+1


On 11/12/2019 09:50 AM, Jan Lahoda wrote:

Looks good to me.

Jan

On 12. 11. 19 7:13, Joe Darcy wrote:

Hello,

To remove the need to modify tests when the JDK version is updated, 
the tests of the preview API in String should use "${jdk.version}" as 
an argument to -source rather than a fixed version string:


 JDK-8233940: Preview API tests for String methods should use 
${jdk.version} as -source arg

 http://cr.openjdk.java.net/~darcy/8233940.0/

The new TEST.properties file only affects tests in the 
java/lang/String directory. Some additional work is needed before 
jtreg "smart actions" can be enabled throughout the tests in the 
"jdk" directory as they are already in "langtools".


Patch below.

Thanks,

-Joe

--- old/test/jdk/java/lang/String/Formatted.java    2019-11-11 
22:09:42.692005000 -0800
+++ new/test/jdk/java/lang/String/Formatted.java    2019-11-11 
22:09:42.476005000 -0800

@@ -25,7 +25,7 @@
   * @test
   * bug 8203444
   * @summary Unit tests for instance versions of String#format
- * @compile --enable-preview -source 14 Formatted.java
+ * @compile --enable-preview -source ${jdk.version} Formatted.java
   * @run main/othervm --enable-preview Formatted
   */

--- old/test/jdk/java/lang/String/StripIndent.java    2019-11-11 
22:09:43.376005000 -0800
+++ new/test/jdk/java/lang/String/StripIndent.java    2019-11-11 
22:09:43.120005000 -0800

@@ -25,7 +25,7 @@
   * @test
   * @bug 8223775
   * @summary This exercises String#stripIndent patterns and limits.
- * @compile --enable-preview -source 14 StripIndent.java
+ * @compile --enable-preview -source ${jdk.version} StripIndent.java
   * @run main/othervm --enable-preview StripIndent
   */

--- old/test/jdk/java/lang/String/TranslateEscapes.java  2019-11-11 
22:09:44.020005000 -0800
+++ new/test/jdk/java/lang/String/TranslateEscapes.java  2019-11-11 
22:09:43.776005000 -0800

@@ -25,7 +25,7 @@
   * @test
   * @bug 8223780
   * @summary This exercises String#translateEscapes patterns and 
limits.

- * @compile --enable-preview -source 14 TranslateEscapes.java
+ * @compile --enable-preview -source ${jdk.version} 
TranslateEscapes.java

   * @run main/othervm --enable-preview TranslateEscapes
   */

--- /dev/null    2019-09-13 10:19:35.76800 -0700
+++ new/test/jdk/java/lang/String/TEST.properties    2019-11-11 
22:09:44.396005000 -0800

@@ -0,0 +1 @@
+allowSmartActionArgs=true

Re: RFR JDK-8233527: Update Lookup::hasPrivateAccess and Lookup::defineClass spec w.r.t. full power lookup

[Mandy Chung]

On 11/13/19 11:45 AM, Johannes Kuhn wrote:
This is a good idea, because full privilege access is a new concept, 
and should be mentioned at the places where it is used. 


To clarify, "full privilege access" (aka "full power") is not really a 
new concept.


Module access was introduced in Java SE 9.  A full privilege access 
(despite the name of `hasPrivateAccess`) necessarily includes module 
privilege.   Before JDK-8173978, a Lookup with private access always 
includes module privilege.  After JDK-8173978, the module privilege has 
become independent of private privilege and hence this new 
`hasFullPrivilegeAcess` method, a correctly named method, is proposed.


Mandy

Re: RFR 8233920: MethodHandles::tryFinally generates illegal bytecode for long/double return types

[Claes Redestad]

On 2019-11-13 21:49, John Rose wrote:

On Nov 13, 2019, at 12:39 PM, Claes Redestad  wrote:

Similar to other promising candidates like constant folding I think it
opens up some more general questions about observability. For example:
do we retain the original bytecode and mappings everything back to it
so that a debugger would be none the wiser about any inlining that may
have happened? Do we "simply" deoptimize and redefine everything back to
the stashed original when someone hooks in a debugger..? (Do we go so
far as to allow opting out of debuggers entirely?)

Yep.  These questions are why link-time optimization is not a slam dunk.
If it were, I suppose we’d have it already.

Debugging is especially hard.  We don’t want to give it up, but any
program transformation is going to make debugging harder to implement.
You should see what the JITs do to support deopt; like exact GC, it’s
something that makes them different animals from the usual llvm/gcc
compilers.

That said, a bytecode-to-bytecode “deoptimization” is easier to specify
and think about than a machine-code-to-bytecode deopt, which is what
the JITs support.  Juicy problems everywhere!


Yes, not trivial, but likely much easier.



Also I wonder if stashing classifies for debugging only will motivate a partial
resurrection of pack200, for storing said things “cold” on disk.


AFAIU, pack200 comes with a maintenance burden that we'd like to
permanently get rid of. For a stashed-away class file archive only used
when debugging then plain old zip at a high compression level might be
sufficient.

/Claes

Re: RFR 8233920: MethodHandles::tryFinally generates illegal bytecode for long/double return types

[John Rose]

On Nov 13, 2019, at 12:39 PM, Claes Redestad  wrote:
> Similar to other promising candidates like constant folding I think it
> opens up some more general questions about observability. For example:
> do we retain the original bytecode and mappings everything back to it
> so that a debugger would be none the wiser about any inlining that may
> have happened? Do we "simply" deoptimize and redefine everything back to
> the stashed original when someone hooks in a debugger..? (Do we go so
> far as to allow opting out of debuggers entirely?)

Yep.  These questions are why link-time optimization is not a slam dunk.
If it were, I suppose we’d have it already.

Debugging is especially hard.  We don’t want to give it up, but any
program transformation is going to make debugging harder to implement.
You should see what the JITs do to support deopt; like exact GC, it’s
something that makes them different animals from the usual llvm/gcc
compilers.

That said, a bytecode-to-bytecode “deoptimization” is easier to specify
and think about than a machine-code-to-bytecode deopt, which is what
the JITs support.  Juicy problems everywhere!

Also I wonder if stashing classifies for debugging only will motivate a partial
resurrection of pack200, for storing said things “cold” on disk.

Re: RFR 8233920: MethodHandles::tryFinally generates illegal bytecode for long/double return types

[Claes Redestad]

On 2019-11-13 21:04, John Rose wrote:
On Nov 13, 2019, at 11:24 AM, Claes Redestad > wrote:


Thanks for suggesting that Vladimir.  That indeed is how the existing 
code is organized.


Yes, not very startup friendly... ;-)


No, but it’s maintainer friendly.  We can walk and chew gum at the same 
time >

Is it time to think about a jlink plugin which inlines simple method calls?
Perhaps we need to wait until we understand (semi-)closed-world packaging.

Long, long ago, a very early version of javac did inlining of simple 
method calls.
That functionality was removed when we realized that this was placing 
“getfield”
instructions in classes which didn’t have access to the referenced 
private fields,
causing code to break.  It also messed up the binary compatibility story 
required

to make sense of independently recompiled class files.  How innocent we were
back then.  We won’t make those same mistakes again, but we could try the
same trick again, more carefully, in jlink or some other static 
packaging workflow.


As a startup optimization, inlining small (private?) methods at link-
time seems like a promising candidate, yes.

Similar to other promising candidates like constant folding I think it
opens up some more general questions about observability. For example:
do we retain the original bytecode and mappings everything back to it
so that a debugger would be none the wiser about any inlining that may
have happened? Do we "simply" deoptimize and redefine everything back to
the stashed original when someone hooks in a debugger..? (Do we go so
far as to allow opting out of debuggers entirely?)

/Claes

Re: RFR 8233920: MethodHandles::tryFinally generates illegal bytecode for long/double return types

[John Rose]

On Nov 13, 2019, at 11:24 AM, Claes Redestad  wrote:
> 
>> Thanks for suggesting that Vladimir.  That indeed is how the existing code 
>> is organized.
> 
> Yes, not very startup friendly... ;-)

No, but it’s maintainer friendly.  We can walk and chew gum at the same time.

Is it time to think about a jlink plugin which inlines simple method calls?
Perhaps we need to wait until we understand (semi-)closed-world packaging.

Long, long ago, a very early version of javac did inlining of simple method 
calls.
That functionality was removed when we realized that this was placing “getfield”
instructions in classes which didn’t have access to the referenced private 
fields,
causing code to break.  It also messed up the binary compatibility story 
required
to make sense of independently recompiled class files.  How innocent we were
back then.  We won’t make those same mistakes again, but we could try the
same trick again, more carefully, in jlink or some other static packaging 
workflow.

— John

RE: RFR (M) 8223261 "JDK-8189208 followup - remove JDK_GetVersionInfo0 and the supporting code"

[Langer, Christoph]

Hi Gerard,

generally it looks like a nice cleanup.

I've got a patch prepared though, which I was planning on posting tomorrow. It 
is about cleanup for the canonicalize function in libjava. I wanted to use 
jdk_util.h for the function prototype. I had not yet filed a bug but here is 
what I have:
http://cr.openjdk.java.net/~clanger/webrevs/cleanup-canonicalize/

So maybe you could refrain from removing jdk_util.h or maybe you can hold off 
submitting your change until my cleanup is reviewed?

I'll create a bug and post an official review thread tomorrow...

Thanks
Christoph

> -Original Message-
> From: hotspot-dev  On Behalf Of
> Mandy Chung
> Sent: Mittwoch, 13. November 2019 20:30
> To: gerard ziemski 
> Cc: awt-...@openjdk.java.net; hotspot-dev developers  d...@openjdk.java.net>; core-libs-dev@openjdk.java.net
> Subject: Re: RFR (M) 8223261 "JDK-8189208 followup - remove
> JDK_GetVersionInfo0 and the supporting code"
> 
> 
> 
> On 11/13/19 10:50 AM, gerard ziemski wrote:
> > Hi all,
> >
> > Please review this cleanup, where we remove JDK_GetVersionInfo0 and
> > related code, since we can get build versions directly from within the
> > VM itself:
> >
> > I'm including core-libs and awt in this review because the proposed
> > fix touches their corresponding files.
> >
> >
> > bug: https://bugs.openjdk.java.net/browse/JDK-8223261
> > webrev: http://cr.openjdk.java.net/~gziemski/8223261_rev1
> > tests: passes Mach5 tier1,2,3,4,5,6
> >
> 
> This is a good clean up.  JDK_GetVersionInfo0 was needed long time ago
> in particular in HS express support that is no longer applicable.
> 
> One leftover comment should also be removed.
> 
> src/hotspot/share/runtime/vm_version.hpp
>    // Gets the jvm_version_info.jvm_version defined in jvm.h
> 
> otherwise looks good.
> 
> Mandy

Re: RFR JDK-8233527: Update Lookup::hasPrivateAccess and Lookup::defineClass spec w.r.t. full power lookup

[Johannes Kuhn]

On 13.11.2019 20:07, Mandy Chung wrote:
Thank you for all those changes. It fixed two of my reported bugs 
(JDK-8209005, JDK-8209078).


Thanks for filing these good reports.   JDK-8173978 resolved the 
issues reported by JDK-8209005 and JDK-8209078.

I'm happy I could help.
It also makes my suggested workaround for JDK-8230636 not longer 
possible. Thanks for picking that up too.


I just have a question about the requirement of 
MethodHandles.privateLookupIn: As it requires that the originating 
lookup has both PRIVATE and MODULE access, it sounds a lot like full 
privilege access. Should this be reworded?




The two bullets about the caller lookup object must have MODULE and 
PRIVATE are important explanation for it to require such access.


Maybe I can add a bullet to say "The caller lookup object must have 
full privilege access" and then move those two bullets as sub-bullets 
under it.


What do you think?
This is a good idea, because full privilege access is a new concept, and 
should be mentioned at the places where it is used.
Also, with the current specification, I don't see a way to abuse 
jdk.unsupported's privileged access into java.base (which IMHO caused 
the entire rework).


Thanks for looking at this.

Mandy


Shameless plug: I once asked on Stackoverflow if untrusted code running 
with a SecurityManager and -illegal-access=deny but granting 
|ReflectPermission("supressAccessChecks") |could escape the sandbox[1]. 
I found a way to escape the sandbox, which relies on the power of the 
Lookup returned by privateLookupIn.
As this has been changed, I wonder if this escape was ever intended in 
the first place. Are there any resources out there that can help answer 
this question?


Thanks,
Johannes

[1]: 
https://stackoverflow.com/questions/51712903/is-it-safe-to-grant-untrusted-code-supressaccesschecks-when-illegal-access-de

Re: RFR (M) 8223261 "JDK-8189208 followup - remove JDK_GetVersionInfo0 and the supporting code"

[Mandy Chung]

On 11/13/19 10:50 AM, gerard ziemski wrote:

Hi all,

Please review this cleanup, where we remove JDK_GetVersionInfo0 and 
related code, since we can get build versions directly from within the 
VM itself:


I'm including core-libs and awt in this review because the proposed 
fix touches their corresponding files.



bug: https://bugs.openjdk.java.net/browse/JDK-8223261
webrev: http://cr.openjdk.java.net/~gziemski/8223261_rev1
tests: passes Mach5 tier1,2,3,4,5,6



This is a good clean up.  JDK_GetVersionInfo0 was needed long time ago 
in particular in HS express support that is no longer applicable.


One leftover comment should also be removed.

src/hotspot/share/runtime/vm_version.hpp
  // Gets the jvm_version_info.jvm_version defined in jvm.h

otherwise looks good.

Mandy

Re: RFR 8233920: MethodHandles::tryFinally generates illegal bytecode for long/double return types

[Claes Redestad]

On 2019-11-13 20:05, John Rose wrote:
On Nov 13, 2019, at 2:28 AM, Vladimir Ivanov 
mailto:vladimir.x.iva...@oracle.com>> wrote:


   private void emitPopInsn(BasicType type) {
 mv.visitVarInsn(popInsnOpcode(type));
   }



Thanks for suggesting that Vladimir.  That indeed is how the existing 
code is organized.


Yes, not very startup friendly... ;-)

I'll do some sanity testing before push.

/Claes

Re: RFR JDK-8233527: Update Lookup::hasPrivateAccess and Lookup::defineClass spec w.r.t. full power lookup

[Mandy Chung]

On 11/13/19 6:10 AM, Johannes Kuhn wrote:

On 11.11.2019 22:23, Mandy Chung wrote:
This is a follow-up of JDK-8226916. Lookup::hasPrivateAccess intends 
to test if this lookup is a full-power lookup; that is created by the 
original caller class calling MethodHandles::lookup. The current 
specification for Lookup::hasPrivateAccess returns true if the lookup 
modes contain PRIVATE but it does not check MODULE bit.


This patch also clarifies the capabilities w.r.t PRIVATE access and 
full-power access. The security permission check is performed if the 
Lookup is not a full power lookup and therefore Lookup::defineClass 
spec should be updated to perform security permission check if the 
security manager is present and this lookup refuses access, 
consistent with other Lookup operations.


http://cr.openjdk.java.net/~mchung/jdk14/8233527/webrev.02/
http://cr.openjdk.java.net/~mchung/jdk14/8233527/specdiff/

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

thanks
Mandy


Thank you for all those changes. It fixed two of my reported bugs 
(JDK-8209005, JDK-8209078).


Thanks for filing these good reports.   JDK-8173978 resolved the issues 
reported by JDK-8209005 and JDK-8209078.


It also makes my suggested workaround for JDK-8230636 not longer 
possible. Thanks for picking that up too.


I just have a question about the requirement of 
MethodHandles.privateLookupIn: As it requires that the originating 
lookup has both PRIVATE and MODULE access, it sounds a lot like full 
privilege access. Should this be reworded?




The two bullets about the caller lookup object must have MODULE and 
PRIVATE are important explanation for it to require such access.


Maybe I can add a bullet to say "The caller lookup object must have full 
privilege access" and then move those two bullets as sub-bullets under it.


What do you think?

Also, with the current specification, I don't see a way to abuse 
jdk.unsupported's privileged access into java.base (which IMHO caused 
the entire rework).


Thanks for looking at this.

Mandy

Re: RFR 8233920: MethodHandles::tryFinally generates illegal bytecode for long/double return types

[John Rose]

On Nov 13, 2019, at 2:28 AM, Vladimir Ivanov mailto:vladimir.x.iva...@oracle.com>> wrote:
> 
>private void emitPopInsn(BasicType type) {
>  mv.visitVarInsn(popInsnOpcode(type));
>}
> 

Thanks for suggesting that Vladimir.  That indeed is how the existing code is 
organized.

Re: RFR (M) 8223261 "JDK-8189208 followup - remove JDK_GetVersionInfo0 and the supporting code"

[Claes Redestad]

On 2019-11-13 19:50, gerard ziemski wrote:

webrev: http://cr.openjdk.java.net/~gziemski/8223261_rev1


Nice cleanup! Looks good to me.

/Claes

RFR (M) 8223261 "JDK-8189208 followup - remove JDK_GetVersionInfo0 and the supporting code"

[gerard ziemski]

Hi all,

Please review this cleanup, where we remove JDK_GetVersionInfo0 and 
related code, since we can get build versions directly from within the 
VM itself:


I'm including core-libs and awt in this review because the proposed fix 
touches their corresponding files.



bug: https://bugs.openjdk.java.net/browse/JDK-8223261
webrev: http://cr.openjdk.java.net/~gziemski/8223261_rev1
tests: passes Mach5 tier1,2,3,4,5,6


cheers

RFR 8233272 : The Class.forName specification should be updated to match the long-standing implementation with respect to class linking

[Brent Christian]

Hi,

Recently, the 2-arg and 3-arg Class.forName() methods were updated[1] to 
perform class linking, per the specification.  However this change had 
to be reverted[2].


Instead, let's clarify the Class.forName() spec not to guarantee linking 
(outside the case of also performing initialization, of course).  This 
is the long-standing behavior.


I also have a test of the non-linking behavior; it's based on the test 
case[3] for JDK-8231924.  It fails as of 14b14 (8212117) and passes as 
of 14b22 (8233091).


Please review my webrev:
http://cr.openjdk.java.net/~bchristi/8233272/webrev-02/

If the wording looks good, I'll fill in the Specification for the CSR[4] 
I've started.


Thanks,
-Brent


1. https://bugs.openjdk.java.net/browse/JDK-8212117

2. https://bugs.openjdk.java.net/browse/JDK-8233091

3. 
https://mail.openjdk.java.net/pipermail/core-libs-dev/2019-October/062747.html


4. https://bugs.openjdk.java.net/browse/JDK-8233554

Re: RFR(XS): 8234011: (zipfs) Memory leak in ZipFileSystem.releaseDeflater()

[Lance Andersen]

Hi Volker,

Thank you for doing this.

As Christoph mentioned,  you can just do Path.of() and create the file in the 
work directory for the test.

If possible, I would use TestNG as that is consistent with the vast majority of 
the tests.

I also believe the rest of the comments below are worth addressing.

Thank you again for the fix

Best
Lance
> On Nov 13, 2019, at 11:26 AM, Langer, Christoph  
> wrote:
> 
> Hi Volker,
> 
> good catch in ZipFileSystem  The fix is the right thing to do.
> 
> I have a few remarks to the test, though:
> 
> Line 52, initialization of the File object: I think you should just do Path 
> zipFile = Paths.get("file.zip"); When the test is run in the jtreg framework, 
> it's running in its own scratch directory, so no need to use java.io.tmpdir. 
> You can also leave cleanup to jtreg and don't need to delete the file in the 
> end (in the finally block). However, you should probably check whether the 
> file exists in the beginning and delete it in that case.
> 
> Line 55ff: You don't need to use this URI thing any more. You can simply do: 
> try (FileSystem fs = FileSystems.newFileSystem(zipFile, Map.of("create", 
> true))) { (line 58).
> 
> Line 61/62: You're using a Vector, wow  You should rather use ArrayList, I 
> think...
> 
> Line 85: This should rather be:
>@SuppressWarnings("unchecked")
>int inflater_count = 
> ((List)inflaters.get(fs)).size();
> Same for line 89.
> 
> Line 93 (Catch clause): I think we should fail in that case, too. Otherwise, 
> if the implementation would change, the test could run unnoticed for years, 
> testing basically nothing...
> 
> Best regards,
> Christoph
> 
> 
>> -Original Message-
>> From: core-libs-dev  On Behalf
>> Of Simonis, Volker
>> Sent: Mittwoch, 13. November 2019 16:23
>> To: core-libs-dev@openjdk.java.net
>> Subject: RFR(XS): 8234011: (zipfs) Memory leak in
>> ZipFileSystem.releaseDeflater()
>> 
>> Hi,
>> 
>> can I please get a review for this trivial fix of an old copy-and-paste 
>> error:
>> 
>> http://cr.openjdk.java.net/~simonis/webrevs/2019/8234011/
>> https://bugs.openjdk.java.net/browse/JDK-8234011
>> 
>> ZipFileSystem caches MAX_FLATER (currently 20) Inflater/Deflater objects.
>> However the logic for reusing Deflaters is wrong because it references the
>> Inflater List when checking against the number of already cached objects.
>> This seems like a day-one copy and paste error.
>> 
>> Notice that this issue is not as critical as it appears, because retaining of
>> additional Deflaters only happens when more than MAX_FLATER are used
>> and released concurrently. I.e. the maximum number of cached Deflaters is
>> the maximal number of Deflaters that are released while no new Deflater is
>> requested. In practice this is usually still a small number, less than
>> MAX_FLATERS. Nevertheless we can easily construct an example which
>> demonstrates the memory leak (see the JTRegtest in the patch) and because
>> the fix is trivial we should really fix this.
>> 
>> Thank you and best regards,
>> Volker
>> 
>> 
>> 
>> Amazon Development Center Germany GmbH
>> Krausenstr. 38
>> 10117 Berlin
>> Geschaeftsfuehrung: Christian Schlaeger, Ralf Herbrich
>> Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
>> Sitz: Berlin
>> Ust-ID: DE 289 237 879
>> 
>> 
> 

 
  

 Lance Andersen| 
Principal Member of Technical Staff | +1.781.442.2037
Oracle Java Engineering 
1 Network Drive 
Burlington, MA 01803
lance.ander...@oracle.com 

RFR: 8234089: (zipfs) Remove classes JarFileSystemProvider and JarFileSystem

[Langer, Christoph]

Hi,

can I please get reviews for this cleanup change in zipfs.

Bug: https://bugs.openjdk.java.net/browse/JDK-8234089
Webrev: http://cr.openjdk.java.net/~clanger/webrevs/8234089.0/

I figured that JarFileSystemProvider is completely obsolete (please correct me 
if I'm wrong) and the reasons for having a class JarFileSystem that extends 
ZipFileSystems are very little in my opinion. I think maintainability is better 
when the few implementation details of multi release jars live in ZipFileSystem 
as well. It saves some code. The only possible drawback is that ZipFileSystem:: 
getInode would have an additional call to a lookup function, that usually is an 
identity transformation. I would hope however, that runtime impact is 
negligible.

I also fix a small bug when property "releaseVersion" is set to null in the env 
map and multi-release contains a value. In the current implementation it would 
not consider the "multi-release" value and treat the mr jar as the current 
runtime version. I had to do a small fix in MultiReleaseJarTest.java to make 
sure the properties list is cleared in every case.

The jdk/nio/zipfs tests run well after my change.

Thanks
Christoph

Re: RFR 8233920: MethodHandles::tryFinally generates illegal bytecode for long/double return types

[Vladimir Ivanov]

http://cr.openjdk.java.net/~jvernee/8233920/webrev.04/


Looks good.

Best regards,
Vladimir Ivanov


On 13/11/2019 11:28, Vladimir Ivanov wrote:



http://cr.openjdk.java.net/~jvernee/8233920/webrev.03/


I like how the patch shapes. Looks good.

In addition, you can encapsulate the following code into a helper method:

  if (isNonVoid) {
- mv.visitInsn(Opcodes.POP);
+ if (isDoubleWord) {
+ mv.visitInsn(Opcodes.POP2);
+ } else {
+ mv.visitInsn(Opcodes.POP);
+ }
  }


    if (isNonVoid) {
  BasicType basicReturnType = BasicType.basicType(returnType);
  emitPopInsn(basicReturnType);
    }

    private void emitPopInsn(BasicType type) {
  mv.visitVarInsn(popInsnOpcode(type));
    }

    private int popInsnOpcode(BasicType type) {
   switch (type) {
 case I_TYPE: return Opcodes.POP;
 case J_TYPE: return Opcodes.POP2;
 case F_TYPE: return Opcodes.POP;
 case D_TYPE: return Opcodes.POP2;
 case L_TYPE: return Opcodes.POP;
 default:
   throw new InternalError("unknown type: " + type);
    }

Best regards,
Vladimir Ivanov

As long as Claes is okay with it, I think we should go for that one 
since the code/doc is simpler. I can file an RFE for examining the 
performance of different code shapes.


Thanks,
Jorn

On 12/11/2019 16:35, Claes Redestad wrote:

Interesting, and you might be right. However I think it'd be better to
file a follow-up to examine this (pre-existing) behavior than to 
expand the scope of this bug fix.


/Claes

On 2019-11-12 16:08, Remi Forax wrote:

Hi Jorn, Claes,
Is it not better to always store in a local variable, like javac 
does, instead of doing the double SWAP if there is a return value 
of size 1 ?


Rémi

- Mail original -

De: "Jorn Vernee" 
À: "Claes Redestad" , "core-libs-dev" 


Envoyé: Mardi 12 Novembre 2019 15:56:06
Objet: Re: RFR 8233920: MethodHandles::tryFinally generates 
illegal bytecode for long/double return types



Hi Claes,

Thanks for the comments!

Updated webrev: 
http://cr.openjdk.java.net/~jvernee/8233920/webrev.02/


Also, thanks for sponsoring.

Jorn

On 12/11/2019 15:30, Claes Redestad wrote:

Hi Jorn,

good catch!

Some minor stylistic concerns:

"* = depends on whether the return type is a category 2 type, which
takes up 2 stack slots."

While long/double are known to use two stack slots, "category 2 
type"

isn't used much outside the specification. I'd either simplify to
"* = depends on whether the return type takes up 1 or 2 stack 
slots."

or add a reference to the definition of type categories.

Similarly I think:

boolean returnsCat2Type = returnType == long.class || returnType ==
double.class;

could be simplified as:

boolean isDoubleWord = basicReturnType.isDoubleWord(); //
returnsDoubleWord?

In the test, is it possible to narrow the expected exception to the
exact type being thrown (T1?) rather than Throwable?

I can sponsor the patch.

/Claes

On 2019-11-12 12:09, Jorn Vernee wrote:

Hi,

Please review the following patch that fixes handling of 
long/double

returns for the tryFinally MethodHandle combinator.

Bug: https://bugs.openjdk.java.net/browse/JDK-8233920
Webrev: http://cr.openjdk.java.net/~jvernee/8233920/webrev.01/
(Testing=tier1)

FWIW, I also added some missing close tags to the javadoc in the
InvokerBytecodeGenerator class (IntelliJ was warning about this).

As a heads-up; I'm not a committer on the jdk project, so a sponsor
would be needed to push this.

Thanks,
Jorn

RE: RFR(XS): 8234011: (zipfs) Memory leak in ZipFileSystem.releaseDeflater()

[Langer, Christoph]

Hi Volker,

good catch in ZipFileSystem  The fix is the right thing to do.

I have a few remarks to the test, though:

Line 52, initialization of the File object: I think you should just do Path 
zipFile = Paths.get("file.zip"); When the test is run in the jtreg framework, 
it's running in its own scratch directory, so no need to use java.io.tmpdir. 
You can also leave cleanup to jtreg and don't need to delete the file in the 
end (in the finally block). However, you should probably check whether the file 
exists in the beginning and delete it in that case.

Line 55ff: You don't need to use this URI thing any more. You can simply do: 
try (FileSystem fs = FileSystems.newFileSystem(zipFile, Map.of("create", 
true))) { (line 58).

Line 61/62: You're using a Vector, wow  You should rather use ArrayList, I 
think...

Line 85: This should rather be:
@SuppressWarnings("unchecked")
int inflater_count = 
((List)inflaters.get(fs)).size();
Same for line 89.

Line 93 (Catch clause): I think we should fail in that case, too. Otherwise, if 
the implementation would change, the test could run unnoticed for years, 
testing basically nothing...

Best regards,
Christoph


> -Original Message-
> From: core-libs-dev  On Behalf
> Of Simonis, Volker
> Sent: Mittwoch, 13. November 2019 16:23
> To: core-libs-dev@openjdk.java.net
> Subject: RFR(XS): 8234011: (zipfs) Memory leak in
> ZipFileSystem.releaseDeflater()
> 
> Hi,
> 
> can I please get a review for this trivial fix of an old copy-and-paste error:
> 
> http://cr.openjdk.java.net/~simonis/webrevs/2019/8234011/
> https://bugs.openjdk.java.net/browse/JDK-8234011
> 
> ZipFileSystem caches MAX_FLATER (currently 20) Inflater/Deflater objects.
> However the logic for reusing Deflaters is wrong because it references the
> Inflater List when checking against the number of already cached objects.
> This seems like a day-one copy and paste error.
> 
> Notice that this issue is not as critical as it appears, because retaining of
> additional Deflaters only happens when more than MAX_FLATER are used
> and released concurrently. I.e. the maximum number of cached Deflaters is
> the maximal number of Deflaters that are released while no new Deflater is
> requested. In practice this is usually still a small number, less than
> MAX_FLATERS. Nevertheless we can easily construct an example which
> demonstrates the memory leak (see the JTRegtest in the patch) and because
> the fix is trivial we should really fix this.
> 
> Thank you and best regards,
> Volker
> 
> 
> 
> Amazon Development Center Germany GmbH
> Krausenstr. 38
> 10117 Berlin
> Geschaeftsfuehrung: Christian Schlaeger, Ralf Herbrich
> Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
> Sitz: Berlin
> Ust-ID: DE 289 237 879
> 
> 

RFR(XS): 8234011: (zipfs) Memory leak in ZipFileSystem.releaseDeflater()

[Simonis, Volker]

Hi,

can I please get a review for this trivial fix of an old copy-and-paste error:

http://cr.openjdk.java.net/~simonis/webrevs/2019/8234011/
https://bugs.openjdk.java.net/browse/JDK-8234011

ZipFileSystem caches MAX_FLATER (currently 20) Inflater/Deflater objects. 
However the logic for reusing Deflaters is wrong because it references the 
Inflater List when checking against the number of already cached objects. This 
seems like a day-one copy and paste error.

Notice that this issue is not as critical as it appears, because retaining of 
additional Deflaters only happens when more than MAX_FLATER are used and 
released concurrently. I.e. the maximum number of cached Deflaters is the 
maximal number of Deflaters that are released while no new Deflater is 
requested. In practice this is usually still a small number, less than 
MAX_FLATERS. Nevertheless we can easily construct an example which demonstrates 
the memory leak (see the JTRegtest in the patch) and because the fix is trivial 
we should really fix this.

Thank you and best regards,
Volker



Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Ralf Herbrich
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879

Re: RFR 8233920: MethodHandles::tryFinally generates illegal bytecode for long/double return types

[Jorn Vernee]

Hi Vladimir,

Thanks for the suggestion, I think it sounds good.

Here is the updated webrev: 
http://cr.openjdk.java.net/~jvernee/8233920/webrev.04/


Thanks,
Jorn

On 13/11/2019 11:28, Vladimir Ivanov wrote:



http://cr.openjdk.java.net/~jvernee/8233920/webrev.03/


I like how the patch shapes. Looks good.

In addition, you can encapsulate the following code into a helper method:

  if (isNonVoid) {
- mv.visitInsn(Opcodes.POP);
+ if (isDoubleWord) {
+ mv.visitInsn(Opcodes.POP2);
+ } else {
+ mv.visitInsn(Opcodes.POP);
+ }
  }


    if (isNonVoid) {
  BasicType basicReturnType = BasicType.basicType(returnType);
  emitPopInsn(basicReturnType);
    }

    private void emitPopInsn(BasicType type) {
  mv.visitVarInsn(popInsnOpcode(type));
    }

    private int popInsnOpcode(BasicType type) {
   switch (type) {
 case I_TYPE: return Opcodes.POP;
 case J_TYPE: return Opcodes.POP2;
 case F_TYPE: return Opcodes.POP;
 case D_TYPE: return Opcodes.POP2;
 case L_TYPE: return Opcodes.POP;
 default:
   throw new InternalError("unknown type: " + type);
    }

Best regards,
Vladimir Ivanov

As long as Claes is okay with it, I think we should go for that one 
since the code/doc is simpler. I can file an RFE for examining the 
performance of different code shapes.


Thanks,
Jorn

On 12/11/2019 16:35, Claes Redestad wrote:

Interesting, and you might be right. However I think it'd be better to
file a follow-up to examine this (pre-existing) behavior than to 
expand the scope of this bug fix.


/Claes

On 2019-11-12 16:08, Remi Forax wrote:

Hi Jorn, Claes,
Is it not better to always store in a local variable, like javac 
does, instead of doing the double SWAP if there is a return value 
of size 1 ?


Rémi

- Mail original -

De: "Jorn Vernee" 
À: "Claes Redestad" , "core-libs-dev" 


Envoyé: Mardi 12 Novembre 2019 15:56:06
Objet: Re: RFR 8233920: MethodHandles::tryFinally generates 
illegal bytecode for long/double return types



Hi Claes,

Thanks for the comments!

Updated webrev: 
http://cr.openjdk.java.net/~jvernee/8233920/webrev.02/


Also, thanks for sponsoring.

Jorn

On 12/11/2019 15:30, Claes Redestad wrote:

Hi Jorn,

good catch!

Some minor stylistic concerns:

"* = depends on whether the return type is a category 2 type, which
takes up 2 stack slots."

While long/double are known to use two stack slots, "category 2 
type"

isn't used much outside the specification. I'd either simplify to
"* = depends on whether the return type takes up 1 or 2 stack 
slots."

or add a reference to the definition of type categories.

Similarly I think:

boolean returnsCat2Type = returnType == long.class || returnType ==
double.class;

could be simplified as:

boolean isDoubleWord = basicReturnType.isDoubleWord(); //
returnsDoubleWord?

In the test, is it possible to narrow the expected exception to the
exact type being thrown (T1?) rather than Throwable?

I can sponsor the patch.

/Claes

On 2019-11-12 12:09, Jorn Vernee wrote:

Hi,

Please review the following patch that fixes handling of 
long/double

returns for the tryFinally MethodHandle combinator.

Bug: https://bugs.openjdk.java.net/browse/JDK-8233920
Webrev: http://cr.openjdk.java.net/~jvernee/8233920/webrev.01/
(Testing=tier1)

FWIW, I also added some missing close tags to the javadoc in the
InvokerBytecodeGenerator class (IntelliJ was warning about this).

As a heads-up; I'm not a committer on the jdk project, so a sponsor
would be needed to push this.

Thanks,
Jorn

Jpackage EA build available

[Andy Herrick]

The next EA build of JPackage is available at 
https://jdk.java.net/jpackage/


Build 14-jpackage+1-70 (2019/11/12) contains the following changes 
(since Build Build 14-jpackage+1-64 on 2019/10/28):


JDK-8233594 create a new option --bind-services to pass on to jlink
JDK-8233636 Make jpackage an incubator and remove tool provider 
implementation

JDK-8233591 Reorder jpackage help text to focus on package creation
JDK-8233592 change --package-type option name to --type and allow -t 
short form
JDK-8232919 If user installs msi and exe, two installations are 
found in Add/Remove

JDK-8232186 Add verification for pkg and dmg tests
JDK-8233265 jpackage --add-modules cannot find additional modules 
with non-modular app
JDK-823 Incorrect comparison of number version strings in 
ToolValidator

JDK-8233143 RPM errors: rpmbuild: no spec files given for build
JDK-8233138 Error 2343 when using --win-dir-chooser
JDK-8233218 rpm uninstall errors (xdg-icon-resource: the icon size 
must be specified with --size)

JDK-8233114 uninstalling app removes shortcut directory

please send feedback to core-libs-dev@openjdk.java.net

/Andy Herrick

Re: RFR JDK-8233527: Update Lookup::hasPrivateAccess and Lookup::defineClass spec w.r.t. full power lookup

[Johannes Kuhn]

On 11.11.2019 22:23, Mandy Chung wrote:
This is a follow-up of JDK-8226916. Lookup::hasPrivateAccess intends 
to test if this lookup is a full-power lookup; that is created by the 
original caller class calling MethodHandles::lookup. The current 
specification for Lookup::hasPrivateAccess returns true if the lookup 
modes contain PRIVATE but it does not check MODULE bit.


This patch also clarifies the capabilities w.r.t PRIVATE access and 
full-power access. The security permission check is performed if the 
Lookup is not a full power lookup and therefore Lookup::defineClass 
spec should be updated to perform security permission check if the 
security manager is present and this lookup refuses access, consistent 
with other Lookup operations.


http://cr.openjdk.java.net/~mchung/jdk14/8233527/webrev.02/
http://cr.openjdk.java.net/~mchung/jdk14/8233527/specdiff/

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

thanks
Mandy


Thank you for all those changes. It fixed two of my reported bugs 
(JDK-8209005, JDK-8209078).
It also makes my suggested workaround for JDK-8230636 not longer 
possible. Thanks for picking that up too.


I just have a question about the requirement of 
MethodHandles.privateLookupIn: As it requires that the originating 
lookup has both PRIVATE and MODULE access, it sounds a lot like full 
privilege access. Should this be reworded?


Also, with the current specification, I don't see a way to abuse 
jdk.unsupported's privileged access into java.base (which IMHO caused 
the entire rework).


thanks,
Johannes

Re: [JDK 14] RFR 8234079: ZipFileInputStreamSkipTest.java runs zero test

[Lance Andersen]

Looks fine

> On Nov 13, 2019, at 6:11 AM, Amy Lu  wrote:
> 
> java/util/zip/ZipFile/ZipFileInputStreamSkipTest.java
> 
> Test runs zero test. Please review the fix.
> 
> bug: https://bugs.openjdk.java.net/browse/JDK-8234079
> webrev: http://cr.openjdk.java.net/~amlu/8234079/webrev.00
> 
> Thanks,
> Amy
> 
> --- old/test/jdk/java/util/zip/ZipFile/ZipFileInputStreamSkipTest.java
> 2019-11-13 19:01:44.0 +0800
> +++ new/test/jdk/java/util/zip/ZipFile/ZipFileInputStreamSkipTest.java
> 2019-11-13 19:01:44.0 +0800
> @@ -105,7 +105,7 @@
>  * @throws Exception If an error occurs during the test
>  */
> @Test
> -private void testStoredSkip() throws Exception {
> +public void testStoredSkip() throws Exception {
>  try (ZipFile zf = new ZipFile(STORED_ZIPFILE.toFile())) {
> var entries = zf.entries();
> @@ -153,7 +153,7 @@
>  * @throws Exception If an error occurs during the test
>  */
> @Test
> -private void testStoredNegativeSkip() throws Exception {
> +public void testStoredNegativeSkip() throws Exception {
>  try (ZipFile zf = new ZipFile(STORED_ZIPFILE.toFile())) {
> var entries = zf.entries();
> @@ -198,7 +198,7 @@
>  * @throws Exception If an error occurs during the test
>  */
> @Test
> -private void testDeflatedSkip() throws Exception {
> +public void testDeflatedSkip() throws Exception {
> try (ZipFile zf = new ZipFile(DEFLATED_ZIPFILE.toFile())) {
> var toSkip = 5; // Bytes to Skip
> var entries = zf.entries();
> @@ -225,7 +225,7 @@
>  * @throws Exception If an unexpected error occurs during the test
>  */
> @Test
> -private void testDeflatedIOException() throws Exception {
> +public void testDeflatedIOException() throws Exception {
> try (ZipFile zf = new ZipFile(DEFLATED_ZIPFILE.toFile())) {
> var entries = zf.entries();
> while (entries.hasMoreElements()) {
> 
> 

 
  

 Lance Andersen| 
Principal Member of Technical Staff | +1.781.442.2037
Oracle Java Engineering 
1 Network Drive 
Burlington, MA 01803
lance.ander...@oracle.com 

[JDK 14] RFR 8234079: ZipFileInputStreamSkipTest.java runs zero test

[Amy Lu]

java/util/zip/ZipFile/ZipFileInputStreamSkipTest.java

Test runs zero test. Please review the fix.

bug: https://bugs.openjdk.java.net/browse/JDK-8234079
webrev: http://cr.openjdk.java.net/~amlu/8234079/webrev.00

Thanks,
Amy

--- old/test/jdk/java/util/zip/ZipFile/ZipFileInputStreamSkipTest.java  
2019-11-13 19:01:44.0 +0800
+++ new/test/jdk/java/util/zip/ZipFile/ZipFileInputStreamSkipTest.java  
2019-11-13 19:01:44.0 +0800
@@ -105,7 +105,7 @@
  * @throws Exception If an error occurs during the test
  */
 @Test
-private void testStoredSkip() throws Exception {
+public void testStoredSkip() throws Exception {
 
 try (ZipFile zf = new ZipFile(STORED_ZIPFILE.toFile())) {

 var entries = zf.entries();
@@ -153,7 +153,7 @@
  * @throws Exception If an error occurs during the test
  */
 @Test
-private void testStoredNegativeSkip() throws Exception {
+public void testStoredNegativeSkip() throws Exception {
 
 try (ZipFile zf = new ZipFile(STORED_ZIPFILE.toFile())) {

 var entries = zf.entries();
@@ -198,7 +198,7 @@
  * @throws Exception If an error occurs during the test
  */
 @Test
-private void testDeflatedSkip() throws Exception {
+public void testDeflatedSkip() throws Exception {
 try (ZipFile zf = new ZipFile(DEFLATED_ZIPFILE.toFile())) {
 var toSkip = 5; // Bytes to Skip
 var entries = zf.entries();
@@ -225,7 +225,7 @@
  * @throws Exception If an unexpected error occurs during the test
  */
 @Test
-private void testDeflatedIOException() throws Exception {
+public void testDeflatedIOException() throws Exception {
 try (ZipFile zf = new ZipFile(DEFLATED_ZIPFILE.toFile())) {
 var entries = zf.entries();
 while (entries.hasMoreElements()) {

Re: RFR 8233731: repeated typo "fro" for "for"

[Daniel Fuchs]

Hi Kiran,

Looks good to me.

best regards,

-- daniel

On 13/11/2019 10:13, Kiran Ravikumar wrote:

Hey Guys,

Please review the correction of a typo from 'fro' to 'for' in 
java.util.Arrays class.



Thanks,

Kiran

*Patch :*

@@ -2599,7 +2599,7 @@
   *   first array to be tested
   * @param aToIndex the index (exclusive) of the last element in the
   * first array to be tested
- * @param b the second array to be tested fro equality
+ * @param b the second array to be tested for equality
   * @param bFromIndex the index (inclusive) of the first element in 
the

   *   second array to be tested
   * @param bToIndex the index (exclusive) of the last element in the
@@ -2671,7 +2671,7 @@
   *   first array to be tested
   * @param aToIndex the index (exclusive) of the last element in the
   * first array to be tested
- * @param b the second array to be tested fro equality
+ * @param b the second array to be tested for equality
   * @param bFromIndex the index (inclusive) of the first element in 
the

   *   second array to be tested
   * @param bToIndex the index (exclusive) of the last element in the
@@ -2743,7 +2743,7 @@
   *   first array to be tested
   * @param aToIndex the index (exclusive) of the last element in the
   * first array to be tested
- * @param b the second array to be tested fro equality
+ * @param b the second array to be tested for equality
   * @param bFromIndex the index (inclusive) of the first element in 
the

   *   second array to be tested
   * @param bToIndex the index (exclusive) of the last element in the
@@ -2816,7 +2816,7 @@
   *   first array to be tested
   * @param aToIndex the index (exclusive) of the last element in the
   * first array to be tested
- * @param b the second array to be tested fro equality
+ * @param b the second array to be tested for equality
   * @param bFromIndex the index (inclusive) of the first element in 
the

   *   second array to be tested
   * @param bToIndex the index (exclusive) of the last element in the
@@ -2889,7 +2889,7 @@
   *   first array to be tested
   * @param aToIndex the index (exclusive) of the last element in the
   * first array to be tested
- * @param b the second array to be tested fro equality
+ * @param b the second array to be tested for equality
   * @param bFromIndex the index (inclusive) of the first element in 
the

   *   second array to be tested
   * @param bToIndex the index (exclusive) of the last element in the
@@ -2961,7 +2961,7 @@
   *   first array to be tested
   * @param aToIndex the index (exclusive) of the last element in the
   * first array to be tested
- * @param b the second array to be tested fro equality
+ * @param b the second array to be tested for equality
   * @param bFromIndex the index (inclusive) of the first element in 
the

   *   second array to be tested
   * @param bToIndex the index (exclusive) of the last element in the
@@ -3044,7 +3044,7 @@
   *   first array to be tested
   * @param aToIndex the index (exclusive) of the last element in the
   * first array to be tested
- * @param b the second array to be tested fro equality
+ * @param b the second array to be tested for equality
   * @param bFromIndex the index (inclusive) of the first element in 
the

   *   second array to be tested
   * @param bToIndex the index (exclusive) of the last element in the
@@ -3127,7 +3127,7 @@
   *   first array to be tested
   * @param aToIndex the index (exclusive) of the last element in the
   * first array to be tested
- * @param b the second array to be tested fro equality
+ * @param b the second array to be tested for equality
   * @param bFromIndex the index (inclusive) of the first element in 
the

   *   second array to be tested
   * @param bToIndex the index (exclusive) of the last element in the
@@ -3210,7 +3210,7 @@
   *   first array to be tested
   * @param aToIndex the index (exclusive) of the last element in the
   * first array to be tested
- * @param b the second array to be tested fro equality
+ * @param b the second array to be tested for equality
   * @param bFromIndex the index (inclusive) of the first element in 
the

   *   second array to be tested
   * @param bToIndex the index (exclusive) of the last element in the
@@ -3303,7 +3303,7 @@
   *   

Re: RFR 8233920: MethodHandles::tryFinally generates illegal bytecode for long/double return types

[Vladimir Ivanov]

http://cr.openjdk.java.net/~jvernee/8233920/webrev.03/


I like how the patch shapes. Looks good.

In addition, you can encapsulate the following code into a helper method:

  if (isNonVoid) {
- mv.visitInsn(Opcodes.POP);
+ if (isDoubleWord) {
+ mv.visitInsn(Opcodes.POP2);
+ } else {
+ mv.visitInsn(Opcodes.POP);
+ }
  }


if (isNonVoid) {
  BasicType basicReturnType = BasicType.basicType(returnType);
  emitPopInsn(basicReturnType);
}

private void emitPopInsn(BasicType type) {
  mv.visitVarInsn(popInsnOpcode(type));
}

private int popInsnOpcode(BasicType type) {
   switch (type) {
 case I_TYPE: return Opcodes.POP;
 case J_TYPE: return Opcodes.POP2;
 case F_TYPE: return Opcodes.POP;
 case D_TYPE: return Opcodes.POP2;
 case L_TYPE: return Opcodes.POP;
 default:
   throw new InternalError("unknown type: " + type);
}

Best regards,
Vladimir Ivanov

As long as Claes is okay with it, I think we should go for that one 
since the code/doc is simpler. I can file an RFE for examining the 
performance of different code shapes.


Thanks,
Jorn

On 12/11/2019 16:35, Claes Redestad wrote:

Interesting, and you might be right. However I think it'd be better to
file a follow-up to examine this (pre-existing) behavior than to 
expand the scope of this bug fix.


/Claes

On 2019-11-12 16:08, Remi Forax wrote:

Hi Jorn, Claes,
Is it not better to always store in a local variable, like javac 
does, instead of doing the double SWAP if there is a return value of 
size 1 ?


Rémi

- Mail original -

De: "Jorn Vernee" 
À: "Claes Redestad" , "core-libs-dev" 


Envoyé: Mardi 12 Novembre 2019 15:56:06
Objet: Re: RFR 8233920: MethodHandles::tryFinally generates illegal 
bytecode for long/double return types



Hi Claes,

Thanks for the comments!

Updated webrev: http://cr.openjdk.java.net/~jvernee/8233920/webrev.02/

Also, thanks for sponsoring.

Jorn

On 12/11/2019 15:30, Claes Redestad wrote:

Hi Jorn,

good catch!

Some minor stylistic concerns:

"* = depends on whether the return type is a category 2 type, which
takes up 2 stack slots."

While long/double are known to use two stack slots, "category 2 type"
isn't used much outside the specification. I'd either simplify to
"* = depends on whether the return type takes up 1 or 2 stack slots."
or add a reference to the definition of type categories.

Similarly I think:

boolean returnsCat2Type = returnType == long.class || returnType ==
double.class;

could be simplified as:

boolean isDoubleWord = basicReturnType.isDoubleWord(); //
returnsDoubleWord?

In the test, is it possible to narrow the expected exception to the
exact type being thrown (T1?) rather than Throwable?

I can sponsor the patch.

/Claes

On 2019-11-12 12:09, Jorn Vernee wrote:

Hi,

Please review the following patch that fixes handling of long/double
returns for the tryFinally MethodHandle combinator.

Bug: https://bugs.openjdk.java.net/browse/JDK-8233920
Webrev: http://cr.openjdk.java.net/~jvernee/8233920/webrev.01/
(Testing=tier1)

FWIW, I also added some missing close tags to the javadoc in the
InvokerBytecodeGenerator class (IntelliJ was warning about this).

As a heads-up; I'm not a committer on the jdk project, so a sponsor
would be needed to push this.

Thanks,
Jorn

RFR 8233731: repeated typo "fro" for "for"

[Kiran Ravikumar]

Hey Guys,

Please review the correction of a typo from 'fro' to 'for' in 
java.util.Arrays class.



Thanks,

Kiran

*Patch :*

@@ -2599,7 +2599,7 @@
  *   first array to be tested
  * @param aToIndex the index (exclusive) of the last element in the
  * first array to be tested
- * @param b the second array to be tested fro equality
+ * @param b the second array to be tested for equality
  * @param bFromIndex the index (inclusive) of the first element in the
  *   second array to be tested
  * @param bToIndex the index (exclusive) of the last element in the
@@ -2671,7 +2671,7 @@
  *   first array to be tested
  * @param aToIndex the index (exclusive) of the last element in the
  * first array to be tested
- * @param b the second array to be tested fro equality
+ * @param b the second array to be tested for equality
  * @param bFromIndex the index (inclusive) of the first element in the
  *   second array to be tested
  * @param bToIndex the index (exclusive) of the last element in the
@@ -2743,7 +2743,7 @@
  *   first array to be tested
  * @param aToIndex the index (exclusive) of the last element in the
  * first array to be tested
- * @param b the second array to be tested fro equality
+ * @param b the second array to be tested for equality
  * @param bFromIndex the index (inclusive) of the first element in the
  *   second array to be tested
  * @param bToIndex the index (exclusive) of the last element in the
@@ -2816,7 +2816,7 @@
  *   first array to be tested
  * @param aToIndex the index (exclusive) of the last element in the
  * first array to be tested
- * @param b the second array to be tested fro equality
+ * @param b the second array to be tested for equality
  * @param bFromIndex the index (inclusive) of the first element in the
  *   second array to be tested
  * @param bToIndex the index (exclusive) of the last element in the
@@ -2889,7 +2889,7 @@
  *   first array to be tested
  * @param aToIndex the index (exclusive) of the last element in the
  * first array to be tested
- * @param b the second array to be tested fro equality
+ * @param b the second array to be tested for equality
  * @param bFromIndex the index (inclusive) of the first element in the
  *   second array to be tested
  * @param bToIndex the index (exclusive) of the last element in the
@@ -2961,7 +2961,7 @@
  *   first array to be tested
  * @param aToIndex the index (exclusive) of the last element in the
  * first array to be tested
- * @param b the second array to be tested fro equality
+ * @param b the second array to be tested for equality
  * @param bFromIndex the index (inclusive) of the first element in the
  *   second array to be tested
  * @param bToIndex the index (exclusive) of the last element in the
@@ -3044,7 +3044,7 @@
  *   first array to be tested
  * @param aToIndex the index (exclusive) of the last element in the
  * first array to be tested
- * @param b the second array to be tested fro equality
+ * @param b the second array to be tested for equality
  * @param bFromIndex the index (inclusive) of the first element in the
  *   second array to be tested
  * @param bToIndex the index (exclusive) of the last element in the
@@ -3127,7 +3127,7 @@
  *   first array to be tested
  * @param aToIndex the index (exclusive) of the last element in the
  * first array to be tested
- * @param b the second array to be tested fro equality
+ * @param b the second array to be tested for equality
  * @param bFromIndex the index (inclusive) of the first element in the
  *   second array to be tested
  * @param bToIndex the index (exclusive) of the last element in the
@@ -3210,7 +3210,7 @@
  *   first array to be tested
  * @param aToIndex the index (exclusive) of the last element in the
  * first array to be tested
- * @param b the second array to be tested fro equality
+ * @param b the second array to be tested for equality
  * @param bFromIndex the index (inclusive) of the first element in the
  *   second array to be tested
  * @param bToIndex the index (exclusive) of the last element in the
@@ -3303,7 +3303,7 @@
  *   first array to be tested
  * @param aToIndex the index (exclusive) of the last element in the
  * first array to be tested
- * @param b the second