RE: Regression after April Java 17 Update (mime types)

[Langer, Christoph]

Hi Bernd,

I just noticed your report regarding the regression after JDK-8273655. I'm 
directing this communication to jdk-updates-dev now as it seems more 
appropriate.

I assume with release notes you're referring to the Oracle 17.0.3 release 
notes? This change, if you look closely at the backports, was only brought to 
OpenJDK 17.0.3 and it will hit the OpenJDK 11u update 11.0.16 in July. So, it 
would not be reflected in Oracle release notes. The best you can get for 
OpenJDK 17.0.3 release notes is [0], to my knowledge.

Do you have some more information regarding the incompatibility? What I'm 
understanding is that this backport unveils a problem with Apache VFS? Do you 
have a bug link for that one? If it's just uncovering a bug in another 3rd 
party software, I assume it doesn't merit a backout, though.

Best regards
Christoph

[0] https://builds.shipilev.net/backports-monitor/release-notes-17.0.3.html


> -Original Message-
> From: core-libs-dev  On Behalf Of Bernd
> Eckenfels
> Sent: Montag, 25. April 2022 19:39
> To: core-libs-dev 
> Subject: Regression after April Java 17 Update (mime types)
> 
> Hello,
> 
> We just tried to push out the Java 17 April Update but it failed with some
> incompatible behavior. We found out it is caused due to a new mime-type (and
> a bug in Apache VFS) that JAR files could no longer be opened in an overlay
> (technically a JAR URL suddenly had a mime-type and therefore VFS no longer
> looked at the file extension).
> 
> Just wanted to give a heads up in case anybody else has that problem. The
> change 8273655 (a backport 828109) was not mentioned in the 17.0.3 release
> notes (it is also in 11.0.16) from Oracle and Azul - will it show up in the
> OpenJDK announcement?
> 
> This specific case is a changed behavior (even when it has rather unexpected
> negative consequences), it would be therefore good to be called out
> specifically.
> 
> Gruss
> Bernd
> 
> 
> --
> https://eur03.safelinks.protection.outlook.com/?url=http%3A%2F%2Fbernd.eck
> enfels.net%2Fdata=05%7C01%7Cchristoph.langer%40sap.com%7C9ca8f
> 2471e24471c3d8308da26e286b1%7C42f7676cf455423c82f6dc2d99791af7%7
> C0%7C0%7C637865052069446373%7CUnknown%7CTWFpbGZsb3d8eyJWIjoi
> MC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000
> %7C%7C%7Csdata=J3kfVCvkDUH9UxiRqkoS9g9o85nB46ksbpUF5HihRP8
> %3Dreserved=0

RE: [11u] RFR: 8211825: ModuleLayer.defineModulesWithXXX does not setup delegation when module reads automatic module

[Langer, Christoph]

Hi Martin,

this makes sense. Thanks for doing the backport.

Best regards
Christoph

> -Original Message-
> From: core-libs-dev  On Behalf Of
> Doerr, Martin
> Sent: Mittwoch, 16. Dezember 2020 14:39
> To: core-libs-dev ; jdk-updates-
> d...@openjdk.java.net
> Subject: [CAUTION] [11u] RFR: 8211825:
> ModuleLayer.defineModulesWithXXX does not setup delegation when
> module reads automatic module
> 
> Hi,
> 
> JDK-8211825 is backported to 11.0.11-oracle. I'd like to backport it for 
> parity.
> The original change applies cleanly, by javac from 11u has a problem with the
> new AutomaticModulesTest:
> AutomaticModulesTest.java:70: error: reference to createJarFile is
> ambiguous
> JarUtils.createJarFile(LIB.resolve("alib.jar"), CLASSES);
> ^
>   both method createJarFile(Path,Path,Path...) in JarUtils and method
> createJarFile(Path,Path,String...) in JarUtils match
> 
> I suggest to resolve it this way:
> http://cr.openjdk.java.net/~mdoerr/8211825_module_layer_11u/test_fix.p
> atch
> 
> 
> Bug:
> https://bugs.openjdk.java.net/browse/JDK-8211825
> 
> Original change:
> http://hg.openjdk.java.net/jdk/jdk/rev/a42c378b6f01
> 
> 11u webrev:
> http://cr.openjdk.java.net/~mdoerr/8211825_module_layer_11u/webrev.0
> 0/
> 
> Please review.
> 
> Best regards,
> Martin

RE: RFR(S): 8242848: Improve performance of InflaterOutputStream.write()

[Langer, Christoph]

Hi Volker,

since it's not yet pushed, I also went over the change and I like it. +1 

One little style nit caught my eye, which you could correct before pushing: The 
style of the if/else blocks in test/jdk/java/util/zip/DeflateIn_InflateOut.java 
in lines 48/49 and 59/60 does not match the other if/else blocks in the file. 
You should probably have the else on the line of the closing bracket before...

Thanks
Christoph


> -Original Message-
> From: core-libs-dev  On Behalf
> Of Volker Simonis
> Sent: Mittwoch, 22. April 2020 22:09
> To: Lance Andersen 
> Cc: Java Core Libs ; Vyom Tewari
> 
> Subject: Re: RFR(S): 8242848: Improve performance of
> InflaterOutputStream.write()
> 
> On Tue, Apr 21, 2020 at 5:23 PM Lance Andersen
>  wrote:
> >
> > Hi Volker,
> >
> > I think overall this looks OK.  I went through the older SCCS histories to 
> > see
> if I could figure out why they were using 512 for the input length but could
> not find anything that might shed some light for me.
> >
> 
> Hi Lance,
> 
> thanks a lot for digging in the old sources to review my change. It's
> great that we stil have people who can use SCCS :)
> 
> > I am not sure you can guarantee that src.zip exists but others might be able
> to weigh in here.  What we have been trying to do going forward is to have
> the tests create  the zip files that it needs.  In some cases, we have 
> created a
> byte array within the test which represents the zip and just write it out
> before the test begins.
> >
> 
> Yes, the dependency on an external file was not nice, so I rewrote the
> benchmark to programmatically create a file which can be compressed by
> a factor of ~6:
> 
> http://cr.openjdk.java.net/~simonis/webrevs/2020/8242848.02/
> 
> Notice that this new version only changes the microbenchmark, all the
> other files are untouched.
> 
> As everybody seemed to be happy with the change itself and the
> regression test, I'm now waiting for your and Clae's final review of
> the microbenchmark before pushing. Please let me know hat you think?
> 
> Best regards,
> Volker
> 
> > I am hoping others with more history might also chime in case they are
> aware of the history here.
> >
> > Thank you for helping improve the performance.
> >
> > Best
> > Lance
> >
> > On Apr 17, 2020, at 6:49 AM, Volker Simonis 
> wrote:
> >
> > Thanks everybody for looking at this change!
> >
> > Please find an updated webrev (with a new test and micro-benchmark)
> > and my answers to your comments below:
> >
> > http://cr.openjdk.java.net/~simonis/webrevs/2020/8242848.01/
> >
> > On Thu, Apr 16, 2020 at 6:40 AM Vyom Tiwari 
> wrote:
> >
> > Thanks for doing this, i think the below code change is not required .
> >
> > Please do let me know if i am not reading it correctly.
> >
> > if (inf.finished() || (len == 0)/* no more input */) {
> >
> > If you check the native code "inf.finished() will be true only of the
> corresponding native call inflate(strm, Z_PARTIAL_FLUSH) returns
> "Z_STREAM_END".
> >
> > After your code change  write may return even if finished() is not true.
> >
> >
> > Yes, that's true, but that's what we must do if there's no more input
> > available. Before my change this break on "len < 1" was in the "if
> > (inf.needsInput())" branch.
> >
> > On Thu, Apr 16, 2020 at 8:22 AM Thomas Stüfe
>  wrote:
> >
> > 252 // Check the decompressor
> > 253 if (inf.finished() || (len == 0)/* no more input */) {
> > 254 break;
> > 255 }
> >
> > Not sure but I think this is wrong because now you bypass the followup
> handling of inf.needsDirectory.
> >
> > Inflater.inflate() returns 0 if either needsInput or needsDirectory. We have
> to ignore needsInput since we have no input anymore, but needsDirectory
> has to be handled, no?
> >
> >
> > You're absolutely right Thomas. Thanks for catching this! I've moved
> > the check for "needsDictionary" in front of the "finished() || len ==
> > 0" check.
> >
> > Unfortunately there is not very good test coverage for zip with preset
> > dictionaries (jtreg and submit repo passed without problems). So I
> > added a new test for this use case to "
> > test/jdk/java/util/zip/DeflateIn_InflateOut.java".
> >
> > On Thu, Apr 16, 2020 at 8:48 AM Thomas Stüfe
>  wrote:
> >
> >
> > As for increasing the buffer size, it makes sense but IMHO needs a CSR and
> a release note.
> >
> >
> > I don't think so. This is an internal, implementation specific setting
> > which has never been exposed or documented before so I don't see why
> > we should document it now. But let's discuss this separately in the
> > corresponding JBS issue (see below).
> >
> > On Thu, Apr 16, 2020 at 1:18 PM Claes Redestad
> >  wrote:
> >
> >
> > Hi Volker,
> >
> > On 2020-04-15 19:48, Volker Simonis wrote:
> >
> > While doing this change, I've also realized that all the streams in
> > java.util.zip (i.e. DeflaterInputStream, GZIPInputStream,
> > GZIPOutputStream, InflaterInputStream, 

RE: RFR (T): 8243117: Cleanups in Java code of module jdk.jlink

[Langer, Christoph]

Hi Alan,

> >> Looks okay to me too, except @SuppressWarnings("unused") looks
> strange,
> >> is that an Eclipse compiler warning name?
> > Yes, it comes from Eclipse, probably only used by its compiler. The private
> field EXIT_SYSERR doesn't seem to be used. Alternatively, I could comment it
> out? Or remove it?
> >
> The jimage tool went through a few iterations and I think it's just a
> left over and should be okay to remove.

OK, I'll remove it altogether then: 
http://cr.openjdk.java.net/~clanger/webrevs/8243117.1/

I'll push it tomorrow, after a final round of testing.

Cheers
Christoph

RE: RFR (T): 8243117: Cleanups in Java code of module jdk.jlink

[Langer, Christoph]

Hi Alan,

> Looks okay to me too, except @SuppressWarnings("unused") looks strange,
> is that an Eclipse compiler warning name?

Yes, it comes from Eclipse, probably only used by its compiler. The private 
field EXIT_SYSERR doesn't seem to be used. Alternatively, I could comment it 
out? Or remove it?

/Christoph

RE: RFR (T): 8243117: Cleanups in Java code of module jdk.jlink

[Langer, Christoph]

Hi Claes,

thanks for your quick review. I configured my IDE to go to wildcard at 10 
imports from the same package. But if there's a common style guide for OpenJDK, 
I can certainly reconfigure this.

Best regards
Christoph

> -Original Message-
> From: Claes Redestad 
> Sent: Sonntag, 19. April 2020 18:32
> To: Langer, Christoph ; jigsaw-
> d...@openjdk.java.net
> Cc: core-libs-dev@openjdk.java.net
> Subject: Re: RFR (T): 8243117: Cleanups in Java code of module jdk.jlink
> 
> Looks good to me, Christoph,
> 
> JmodTask.java: Not sure what the consensus is about wildcard imports,
> but I'm fine with it here.
> 
> /Claes
> 
> On 2020-04-19 17:32, Langer, Christoph wrote:
> > Hi,
> >
> > please help review this cleanup patch for the Java code in module jdk.jlink.
> It's mostly automatic IDE cleanups of unused imports plus a few removals for
> unused private methods, fields and annotations. I've also added some
> @Override annotations where such were missing.
> >
> > Bug: https://bugs.openjdk.java.net/browse/JDK-8243117
> > Webrev: http://cr.openjdk.java.net/~clanger/webrevs/8243117.0/
> >
> > Thanks
> > Christoph
> >

RFR (T): 8243117: Cleanups in Java code of module jdk.jlink

[Langer, Christoph]

Hi,

please help review this cleanup patch for the Java code in module jdk.jlink. 
It's mostly automatic IDE cleanups of unused imports plus a few removals for 
unused private methods, fields and annotations. I've also added some @Override 
annotations where such were missing.

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

Thanks
Christoph

RE: RFR: 8242039: Improve jlink VersionPropsPlugin

[Langer, Christoph]

Hi Claes and Mark,

thanks again for your inputs.

I'll push this with Claes' review then.

Best regards
Christoph

> -Original Message-
> From: core-libs-dev  On Behalf
> Of Claes Redestad
> Sent: Mittwoch, 8. April 2020 00:12
> To: core-libs-dev@openjdk.java.net
> Subject: Re: RFR: 8242039: Improve jlink VersionPropsPlugin
> 
> 
> 
> On 2020-04-03 15:36, Langer, Christoph wrote:
> > Eventually I came up with this result and then I also asked myself the
> question whether the new complexity was worth the benefit. I answered
> myself with a yes (though definitely not a clear one ), and that's why I
> proposed the change. After all, the new complexity isn't huge...
> 
> I don't mind the cleaned up patch[1].
> 
> It also gets rid of the constants being replaced, which I assume will
> otherwise be loaded and kept on the heap and in the string table
> forever. While unlikely to cause confusion, I'd argue that not finding
> the value replaced in heap dumps might be of some value.
> 
> /Claes
> 
> [1] http://cr.openjdk.java.net/~clanger/webrevs/8242039.1/

RE: RFR: 8242039: Improve jlink VersionPropsPlugin

[Langer, Christoph]

Thanks, Claes.

> -Original Message-
> From: Claes Redestad 
> Sent: Donnerstag, 2. April 2020 23:05
> To: Langer, Christoph ; jigsaw-
> d...@openjdk.java.net; core-libs-dev@openjdk.java.net
> Subject: Re: RFR: 8242039: Improve jlink VersionPropsPlugin
> 
> 
> 
> On 2020-04-02 20:21, Langer, Christoph wrote:
> > OK, I created an edition which doesn't change the formatting so that the
> added code becomes more obvious:
> > http://cr.openjdk.java.net/~clanger/webrevs/8242039.1/
> >
> > Does it look better?
> 
> I think so, yes.
> 
> /Claes

RE: RFR: 8242039: Improve jlink VersionPropsPlugin

[Langer, Christoph]

Hi Mark,

> -Original Message-
> From: mark.reinh...@oracle.com 
> Sent: Donnerstag, 2. April 2020 23:13
> To: Langer, Christoph 
> Cc: jigsaw-...@openjdk.java.net; core-libs-dev@openjdk.java.net
> Subject: Re: RFR: 8242039: Improve jlink VersionPropsPlugin
> 
> 2020/4/2 8:01:28 -0700, christoph.lan...@sap.com:
> > please review a small improvement for the jlink
> > VersionPropsPlugin. The Plugin modifies the bytecode of
> > java/lang/VersionProps.class to replace the initializion of certain
> > vendor specific system properties with custom values. This
> > modification currently adds two bytecodes per constant, a pop and
> > another ldc. I overhauled it to simply replace the original ldc of the
> > old value with another ldc, loading the custom value.
> 
> I thought about doing this when I originally wrote that plugin, but it’s
> so awkward to achieve with ASM -- as demonstrated by your patch -- that
> I concluded it wasn’t worth it.  Who will notice an extra pop in a basic
> block that’s only ever executed once?  Is the complexity of this new
> code worth the benefit?

Well, first I started playing with this and got a bit obsessed to find 
optimizations in that area. (I learned quite a bit about java asm.)
It would be of higher (micro-)benefit for common VM startup if the fields to be 
modified could be final but that's even more awkward to do and requires 
intricate knowledge and assumptions about how VersionProps.java is structured. 
So I decided against messing with that.

Eventually I came up with this result and then I also asked myself the question 
whether the new complexity was worth the benefit. I answered myself with a yes 
(though definitely not a clear one ), and that's why I proposed the change. 
After all, the new complexity isn't huge...

So, would that be your terminal veto or could you imagine accepting the change?

Best regards
Christoph

RE: RFR: 8242039: Improve jlink VersionPropsPlugin

[Langer, Christoph]

Hi Claes,

> fun little micro-optimization. :-)

Yep, I got a bit obsessed here.  But thanks for the review.

> My only nit is that I'd have preferred if you kept the indentation
> style, since the patch now shifts code around a bit (which makes it
> harder to review and see the history)

OK, I created an edition which doesn't change the formatting so that the added 
code becomes more obvious:
http://cr.openjdk.java.net/~clanger/webrevs/8242039.1/

Does it look better?

Cheers
Christoph

> 
> /Claes
> 
> 
> On 2020-04-02 17:01, Langer, Christoph wrote:
> > Hi,
> >
> > please review a small improvement for the jlink VersionPropsPlugin. The
> Plugin modifies the bytecode of java/lang/VersionProps.class to replace the
> initializion of certain vendor specific system properties with custom values.
> This modification currently adds two bytecodes per constant, a pop and
> another ldc. I overhauled it to simply replace the original ldc of the old 
> value
> with another ldc, loading the custom value.
> >
> > I was playing a bit with the plugin and tried to familiarize with the code 
> > – so
> that’s the outcome 
> >
> > I also added an @SuppressWarnings("unused") in
> VersionProps.java.template to quiesce an IDE warning.
> >
> > Bug: https://bugs.openjdk.java.net/browse/JDK-8242039
> > Webrev: http://cr.openjdk.java.net/~clanger/webrevs/8242039.0/
> >
> > Thanks
> > Christoph
> >

RFR: 8242039: Improve jlink VersionPropsPlugin

[Langer, Christoph]

Hi,

please review a small improvement for the jlink VersionPropsPlugin. The Plugin 
modifies the bytecode of java/lang/VersionProps.class to replace the 
initializion of certain vendor specific system properties with custom values. 
This modification currently adds two bytecodes per constant, a pop and another 
ldc. I overhauled it to simply replace the original ldc of the old value with 
another ldc, loading the custom value.

I was playing a bit with the plugin and tried to familiarize with the code – so 
that’s the outcome 

I also added an @SuppressWarnings("unused") in VersionProps.java.template to 
quiesce an IDE warning.

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

Thanks
Christoph

RE: RFR (S): 8241947: Minor comment fixes for system property handling

[Langer, Christoph]

Thanks for the review, Alan.

I'll push it then.

Best regards
Christoph

> -Original Message-
> From: Alan Bateman 
> Sent: Mittwoch, 1. April 2020 16:46
> To: Langer, Christoph ; Mandy Chung
> ; core-libs-dev@openjdk.java.net
> Cc: build-dev 
> Subject: Re: RFR (S): 8241947: Minor comment fixes for system property
> handling
> 
> On 31/03/2020 19:57, Langer, Christoph wrote:
> > Hi Mandy,
> >
> > this is a good suggestion. The listing of system properties at the props 
> > field
> declaration seems somewhat redundant, given that it already exists more
> exactly and with API normativity in the doc of System::getProperties().
> >
> > So what do you think of this version:
> http://cr.openjdk.java.net/~clanger/webrevs/8241947.1/ ?
> >
> Mandy's suggestion to link to getProperties instead is good. This
> version looks good to me.
> 
> -Alan.

RE: RFR (S): 8241947: Minor comment fixes for system property handling

[Langer, Christoph]

Hi Mandy,

this is a good suggestion. The listing of system properties at the props field 
declaration seems somewhat redundant, given that it already exists more exactly 
and with API normativity in the doc of System::getProperties().

So what do you think of this version: 
http://cr.openjdk.java.net/~clanger/webrevs/8241947.1/ ?

Thanks
Christoph

From: Mandy Chung 
Sent: Dienstag, 31. März 2020 19:34
To: Langer, Christoph ; core-libs-dev@openjdk.java.net
Cc: build-dev 
Subject: Re: RFR (S): 8241947: Minor comment fixes for system property handling


On 3/31/20 7:56 AM, Langer, Christoph wrote:

Hi,



please review a small fix that updates two comments.



The first one, in make/autoconf/spec.gmk.in, is probably quite old. It talks 
about handling of a property "vm.vendor" in VersionProps.java.template. 
However, there is no property "vm.vendor", it must rather be "java.vendor". I 
stumbled over that when looking at the change of JDK-4947890 (Minimize JNI 
upcalls in system-properties initialization).



The second one is the code comment attached to "private static Properties 
props;" in java.lang.System. After JDK-8197927 (Allow the system property 
`java.vendor.version` to be undefined), "java.vendor.version" can be undefined, 
so this should be reflected in the comment. I also took the liberty to remove 
an unneeded import statement.



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

Webrev: http://cr.openjdk.java.net/~clanger/webrevs/8241947.0/



I wonder if we still want to keep this block of comments listing a subset of 
system properties.  Anyway the minor comment looks okay.

Mandy

RE: RFR (S): 8241947: Minor comment fixes for system property handling

[Langer, Christoph]

Hi Magnus,

>  From a build perspective this looks fine.

Thanks for the review.

> But it seems you are changing the interface for java.lang.System. Don't
> you need a CSR for that? Or is your claim that the interface was indeed
> changed by JDK-8197927, and it is a bug that the documentation was not
> updated to match this?

Yes, the second is true. The interface/spec was already changed with 
JDK-8197927 which also has a CSR attached. The commit is this: 
http://hg.openjdk.java.net/jdk/jdk/rev/d80becbcd3c1.
What I'm modifying here is only the non API relevant doc of the private field 
props. The actual interface is documented in the Javadoc of method 
java.lang.System::getProperties().

Cheers
Christoph

RFR (S): 8241947: Minor comment fixes for system property handling

[Langer, Christoph]

Hi,

please review a small fix that updates two comments.

The first one, in make/autoconf/spec.gmk.in, is probably quite old. It talks 
about handling of a property "vm.vendor" in VersionProps.java.template. 
However, there is no property "vm.vendor", it must rather be "java.vendor". I 
stumbled over that when looking at the change of JDK-4947890 (Minimize JNI 
upcalls in system-properties initialization).

The second one is the code comment attached to "private static Properties 
props;" in java.lang.System. After JDK-8197927 (Allow the system property 
`java.vendor.version` to be undefined), "java.vendor.version" can be undefined, 
so this should be reflected in the comment. I also took the liberty to remove 
an unneeded import statement.

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

Thanks
Christoph

RE: [11u] RFR(S): 8223326: Regression introduced by CPU sync: java.security.AccessControlException: access denied ("java.net.NetPermission" "setSocketImpl")

[Langer, Christoph]

> > By spec part you mean the "@throws SecurityException" sections? Do you
> think those should not have been part of the 11u/13u change? Should these
> be even rolled back?
> >
> The spec changes to NetPermission and the protected Socket constructor
> should not be in the update releases. If a security fix involves a spec
> clarification then a good starting assumption is that the scope of the
> change for the update releases, if applicable, will be bit different.

Ok, makes sense. I wasn't involved in the security fix and its backport, 
though. But I assume should now leave the fix for JDK-8218573: "Better socket 
support" (http://hg.openjdk.java.net/jdk-updates/jdk11u-dev/rev/c7602effc480) 
alone now?

/Christoph

RE: [11u] RFR(S): 8223326: Regression introduced by CPU sync: java.security.AccessControlException: access denied ("java.net.NetPermission" "setSocketImpl")

[Langer, Christoph]

> >> One other thing on this. It looks like the spec changes that were part
> >> of JDK-8218573 have been backported to jdk-updates/jdk11u-dev by
> >> mistake. Is that part of the issue that you are trying to fix?
> > I can't find/access JDK-8218573 and I also struggle to find a changelist 
> > that
> would resolve it (should it be a private bug). Did you misspell the number?
> Please help me.
> >
> JDK-8218573 was a security fix (restricted in JBS). The spec update for
> Java SE 13+ that was part of that change have found their way into the
> jdk11u-dev repo, the change for JDK 12 and older should have not
> included the spec change.
> 
> https://hg.openjdk.java.net/jdk-updates/jdk11u-dev/rev/c7602effc480

Ah, I see... JDK-8218573 is JDK11u/JDK13u specific. Looks like it was derived 
from JDK-8217997 in jdk/jdk but pushed as a different bug. jdk/jdk was the only 
place where I was looking for JDK-8218573, so I couldn't find it.

By spec part you mean the "@throws SecurityException" sections? Do you think 
those should not have been part of the 11u/13u change? Should these be even 
rolled back?

Thanks
Christoph

RE: [11u] RFR(S): 8223326: Regression introduced by CPU sync: java.security.AccessControlException: access denied ("java.net.NetPermission" "setSocketImpl")

[Langer, Christoph]

Hi Alan,

> On 23/03/2020 18:07, Doerr, Martin wrote:
> > Hi,
> >
> > I'd like to backport JDK-8223326 from jdk/jdk.
> >
> One other thing on this. It looks like the spec changes that were part
> of JDK-8218573 have been backported to jdk-updates/jdk11u-dev by
> mistake. Is that part of the issue that you are trying to fix?

I can't find/access JDK-8218573 and I also struggle to find a changelist that 
would resolve it (should it be a private bug). Did you misspell the number? 
Please help me.

Thanks
Christoph

RE: [11u] RFR(S): 8223326: Regression introduced by CPU sync: java.security.AccessControlException: access denied ("java.net.NetPermission" "setSocketImpl")

[Langer, Christoph]

Hi Alan,

> -Original Message-
> From: jdk-updates-dev  On
> Behalf Of Alan Bateman
> Sent: Montag, 23. März 2020 20:19
> To: Doerr, Martin ; core-libs-
> d...@openjdk.java.net; jdk-updates-...@openjdk.java.net
> Subject: Re: [11u] RFR(S): 8223326: Regression introduced by CPU sync:
> java.security.AccessControlException: access denied
> ("java.net.NetPermission" "setSocketImpl")
> 
> On 23/03/2020 18:07, Doerr, Martin wrote:
> > Hi,
> >
> > I'd like to backport JDK-8223326 from jdk/jdk.
> >
> > 11u backport issue:
> > https://bugs.openjdk.java.net/browse/JDK-8241460
> >
> > Original change:
> > https://hg.openjdk.java.net/jdk/jdk/rev/29624901d8bc
> >
> > 11u backport webrev:
> > http://cr.openjdk.java.net/~mdoerr/8223326_nio_socket_11u/webrev.00/
> >
> > I had to integrate the change manually due to context changes. However,
> the new code fits to the 11u versions.
> >
> > Please review.
> >
> Socket(SocketImpl) is only specified to do a permission check when the
> impl is non-null. The socket adaptor in JDK 12 and older releases
> doesn't have a dummy impl so the change should not be needed. If there
> is a security exception thrownhere then it suggests something may be
> broken elsewhere, do you have a stack trace?

I think you are completely right, that change is not required for 11u as it 
stands.

ServerSocketAdaptor::create in 11u would never enter a path that does a 
permission check and for SocketAdaptor:create the Socket constructor is called 
with a null impl, so checkPermission(null) will immediately return null.

We also don't have an indication that something is broken. My team is just 
doing an exercise to go through private JBS bugs that have been resolved in 
jdk/jdk after JDK11 was released and tries to asses which ones should/need to 
go into OpenJDK 11 updates, too. That's because we wouldn't see when Oracle 
would backport such bugs for their Oracle 11 Updates and we fear we'd miss 
something important otherwise.

But I suggest to withdraw this backport of JDK-8223326 and set JDK-8241460 to 
"Won't Fix".

Best regards
Christoph

RE: [11u] RFR(S): 8223326: Regression introduced by CPU sync: java.security.AccessControlException: access denied ("java.net.NetPermission" "setSocketImpl")

[Langer, Christoph]

Hi Paul,

as Martin said, the original bug could not be opened by Oracle. I, however 
changed the bug created by Martin to a Backport item. Doing it that way would 
allow a "standard" backport of such type of issues while the original item that 
was pushed to head can remain invisible. We proceeded like that with other bugs 
already and it proved to work nicely.

Best regards
Christoph

> -Original Message-
> From: core-libs-dev  On Behalf
> Of Hohensee, Paul
> Sent: Montag, 23. März 2020 19:29
> To: Doerr, Martin ; core-libs-
> d...@openjdk.java.net; jdk-updates-...@openjdk.java.net
> Subject: RE: [11u] RFR(S): 8223326: Regression introduced by CPU sync:
> java.security.AccessControlException: access denied
> ("java.net.NetPermission" "setSocketImpl")
> 
> The changeset references JDK-8223326, which is private. If possible, ask
> Oracle to make it public so we can do a normal backport rather than file an
> 11u-specific issue. The backport itself looks fine.
> 
> Thanks,
> Paul
> 
> On 3/23/20, 11:08 AM, "jdk-updates-dev on behalf of Doerr, Martin"  updates-dev-boun...@openjdk.java.net on behalf of
> martin.do...@sap.com> wrote:
> 
> 
> Hi,
> 
> I'd like to backport JDK-8223326 from jdk/jdk.
> 
> 11u backport issue:
> https://bugs.openjdk.java.net/browse/JDK-8241460
> 
> Original change:
> https://hg.openjdk.java.net/jdk/jdk/rev/29624901d8bc
> 
> 11u backport webrev:
> 
> http://cr.openjdk.java.net/~mdoerr/8223326_nio_socket_11u/webrev.00/
> 
> I had to integrate the change manually due to context changes. However,
> the new code fits to the 11u versions.
> 
> Please review.
> 
> Best regards,
> Martin
> 
> 

RE: RFR: 8230117 removing dead code from jar tool

[Langer, Christoph]

Hi Adam,

thanks for opening up the bug and reposting. Looks good to me 

Cheers
Christoph

From: Adam Sotona 
Sent: Mittwoch, 11. März 2020 11:32
To: Langer, Christoph 
Cc: Lance Andersen ; Sean Mullan 
; core-libs-dev@openjdk.java.net
Subject: Re: RFR: 8230117 removing dead code from jar tool

Hi Christoph,
thank you for clarification, here it is:

JBS:  https://bugs.openjdk.java.net/browse/JDK-8230117
webrev: http://cr.openjdk.java.net/~asotona/8230117/webrev.00/

Mach5 Tier1, Tier2 and Tier3 tests passed.

Thanks,
Adam


On 1 Mar 2020, at 21:16, Langer, Christoph 
mailto:christoph.lan...@sap.com>> wrote:

Hi Adam, Lance,

as you've already figured out, attachments sent out to the mailing list will be 
removed. So it's important to generate webrevs and link to it or, for smaller 
patches, inline it. 

Also, the RFR subject is missing a bug ID. From the URL to the webrev I take 
the bug ID is JDK-8230117. Going to JBS it seems that the bug is confidential. 
Could you please either open up the bug before pushing or alternatively, create 
a new, public bug to push this against?

Thanks
Christoph




-Original Message-
From: core-libs-dev 
mailto:core-libs-dev-boun...@openjdk.java.net>>
 On Behalf
Of Lance Andersen
Sent: Freitag, 28. Februar 2020 18:04
To: Sean Mullan mailto:sean.mul...@oracle.com>>
Cc: core-libs-dev@openjdk.java.net<mailto:core-libs-dev@openjdk.java.net>
Subject: Re: RFR: removing dead code from jar tool

sigh it is being removed it appears by the mail server

I placed a copy here: http://cr.openjdk.java.net/~lancea/JDK-8230117.patch
<http://cr.openjdk.java.net/~lancea/JDK-8230117.patch>


On Feb 28, 2020, at 12:00 PM, Lance Andersen
mailto:lance.ander...@oracle.com>> wrote:


Here it is again


On Feb 28, 2020, at 11:07 AM, Sean Mullan 
mailto:sean.mul...@oracle.com>>
wrote:


I think you forgot to attach the patch.

--Sean

On 2/28/20 2:28 AM, Adam Sotona wrote:

Hi,
I would like to ask for review of the attached patch removing dead code
from jar tool

Class files sun.tools.jar.Manifest and sun.tools.jar.SignatureFile appear
to be dead code and should be removed.

Build with the patch passes all Tier1, Tier2 and Tier3 tests.
Thank you,
Adam

<http://oracle.com/us/design/oracle-email-sig-198324.gif>
<http://oracle.com/us/design/oracle-email-sig-198324.gif>
<http://oracle.com/us/design/oracle-email-sig-198324.gif>

<http://oracle.com/us/design/oracle-email-sig-198324.gif>Lance
Andersen| Principal Member of Technical Staff | +1.781.442.2037

Oracle Java Engineering
1 Network Drive
Burlington, MA 01803
lance.ander...@oracle.com<mailto:lance.ander...@oracle.com> 
<mailto:lance.ander...@oracle.com>


<http://oracle.com/us/design/oracle-email-sig-198324.gif>
<http://oracle.com/us/design/oracle-email-sig-198324.gif>
<http://oracle.com/us/design/oracle-email-sig-198324.gif>
<http://oracle.com/us/design/oracle-email-sig-198324.gif>Lance Andersen|
Principal Member of Technical Staff | +1.781.442.2037
Oracle Java Engineering
1 Network Drive
Burlington, MA 01803
lance.ander...@oracle.com<mailto:lance.ander...@oracle.com> 
<mailto:lance.ander...@oracle.com>

RE: RFR 8240524: Removed warnings from test classes

[Langer, Christoph]

Hi Vipin,

this all looks correct to me.

When changing the copyright headers, you have to keep the first (initial) year 
and only update the second year. If this doesn’t exist, you’ll have to add it, 
e.g.
Copyright (c) 1999, Oracle -> Copyright (c) 1999, 2020, Oracle

I can fix that for you though, when sponsoring, no need for new webrev.

Can I have a second review, please?

Thanks
Christoph



From: Vipin Sharma 
Sent: Donnerstag, 5. März 2020 18:27
To: Langer, Christoph ; core-libs-dev@openjdk.java.net
Subject: RFR 8240524: Removed warnings from test classes

Hi All,

Please review patch to remove warnings from test classes.

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


Change description:

Class: test/jdk/java/lang/Boolean/GetBoolean.java
Fixed following warning:
1. "Exception 'java.lang.Exception' is never thrown in the method”

Class: test/jdk/java/lang/Boolean/MakeBooleanComparable.java
Fixed following warnings:
1. Explicit type argument Boolean can be replaced with <>
2. C-style array declaration of parameter 'args'

Class: test/jdk/java/lang/Boolean/ParseBoolean.java
Fixed following warning:
1. Exception 'java.lang.Exception' is never thrown in the method


Regards,
Vipin

RE: RFR: 8211917: (zipfs) Creating or updating a JAR file system should put the MANIFEST.MF at the start

[Langer, Christoph]

Hi Lance,

The revised webrev is at 
http://cr.openjdk.java.net/~lancea/8211917/webrev.02/index.html

This looks good to me now. Ship it 

Cheers
Christoph

RE: RFR: 8211917: (zipfs) Creating or updating a JAR file system should put the MANIFEST.MF at the start

[Langer, Christoph]

Hi Lance,

Thanks for addressing my points. Here my answer to those things which weren't 
in full agreement yet:

> I dislike a bit the fact that we introduce a new testng subfolder in
> test/jdk/nio/zipfs. Wouldn’t it be possible to consolidate in the current test
> folder?
> 
> I am trying to add some organization separating the non-testng  tests from
> the  testng tests and other testng tests will be moved here.  I did the same
> for JDBC a few years back

ok, maybe it's a good idea to do this here and gradually move all testng tests 
over.

> - line 387: Manfiest -> Manifest

I think you missed that one


> - line 417: Parameter "final Map
> attributes" of ManifestOrderTest::verify should be renamed to
> "manifestAttributes" to make it easier to understand its purpose
> 
> 
> Prefer to leave as is as it gets wordy and I believe the description is clear 
> in
> the comments

Hm, I needed to look twice to grasp it. So, I'd still prefer something with 
"manifest" in the variable name. But I leave it to you since it's probably a 
personal taste thing 

However, two more things here:

The Javadoc of verify says (line 412):
* @param attributes  Is there a Manifest to check

You should rather go with the Javadoc of validateManifest here as well:
* @param attributes A Map containing the attributes expected in the Manifest;
*   otherwise empty

Also, I spotted in the Javadoc, line 413:
* @param entries Entries to validate are in the JAR

I guess the "are" is wrong here.


> test/jdk/jdk/nio/zipfs/testng/util/ZipFsBaseTest.java:
> - rename to ZipFSBaseTest (Capital ‘S’)??
> hmm  I had that originally but did not like it…  but don’t have a strong
> preference

Ok, leave it as you have it 

> - line 120: public static void verify - > this method is not used by
> ManifestOrderTest. I think we should only add it when having a test that
> really uses this verify method. But I generally agree that the verify method 
> is
> a candidate for communalization
> 
> This will be used by some tests I have created and some I will be moving so
> rather add it now on the initial push.  This is used by several tests that 
> will be
> migrated
> 
> - line 176: public void zip: dito, this method doesn’t seem used. So I suggest
> to remove it for this change
> Same comment as above

OK.

> The webrev for the above
> is http://cr.openjdk.java.net/~lancea/8211917/webrev.01/index.html

Looks good, apart from my comments above.

Thanks
Christoph

RE: Need bug id to remove the explicit type argument in test class

[Langer, Christoph]

Hi Vipin,

it's a really tiny thing but here you go: 
https://bugs.openjdk.java.net/browse/JDK-8240524

In the change you'll also need to update the copyright year.

Maybe you find other places in these tests you want to update?

Best regards
Christoph


> -Original Message-
> From: core-libs-dev  On Behalf
> Of Vipin Sharma
> Sent: Montag, 2. März 2020 20:34
> To: core-libs-dev@openjdk.java.net
> Subject: Need bug id to remove the explicit type argument in test class
> 
> Hi,
> 
> My OCA has been processed recently and I want to start Contributing to
> OpenJDK.
> 
> As a first fix, I would like to remove the explicit type argument in test
> class test/jdk/java/lang/Boolean/MakeBooleanComparable.java to fix one
> warning.
> 
> The first line given below is the existing code and the second line is what
> I am changing it to.
> 
> List list = new ArrayList();
> List list = new ArrayList<>();
> 
> If this is not too small change to start with, could you please help me to
> create a bug id against this so that I can create a patch and share.
> 
> Regards,
> Vipin

RE: RFR 8129841 Update comment for Java_java_net_Inet6AddressImpl_getHostByAddr

[Langer, Christoph]

Hi Vipin,

I'm forwarding this to net-dev where it should be reviewed.

Best regards
Christoph

> -Original Message-
> From: core-libs-dev  On Behalf
> Of Vipin Mv1
> Sent: Dienstag, 3. März 2020 11:54
> To: core-libs-dev@openjdk.java.net
> Subject: RFR 8129841 Update comment for
> Java_java_net_Inet6AddressImpl_getHostByAddr
> 
> Hi All,
> 
> Please find the below changes for the issue
> https://bugs.openjdk.java.net/browse/JDK-8129841. Apart from the
> requested changes, I have made additional changes to the Signature where
> ever I found it incorrect.
> 
> 
> Thanks
> Vipin M V
> 
> 
> diff -r 387369ff21a4 src/java.base/unix/native/libnet/Inet4AddressImpl.c
> --- a/src/java.base/unix/native/libnet/Inet4AddressImpl.c Wed Feb 05
> 12:20:36 2020 -0300
> +++ b/src/java.base/unix/native/libnet/Inet4AddressImpl.c Tue Mar 03
> 15:32:21 2020 +0530
> @@ -218,7 +218,7 @@
>  /*
>   * Class: java_net_Inet4AddressImpl
>   * Method:getHostByAddr
> - * Signature: (I)Ljava/lang/String;
> + * Signature: ([B)Ljava/lang/String;
>   *
>   * Theoretically the UnknownHostException could be enriched with gai error
>   * information. But as it is silently ignored anyway, there's no need for 
> this.
> diff -r 387369ff21a4 src/java.base/unix/native/libnet/Inet6AddressImpl.c
> --- a/src/java.base/unix/native/libnet/Inet6AddressImpl.c Wed Feb 05
> 12:20:36 2020 -0300
> +++ b/src/java.base/unix/native/libnet/Inet6AddressImpl.c Tue Mar 03
> 15:32:21 2020 +0530
> @@ -413,7 +413,7 @@
>  /*
>   * Class: java_net_Inet6AddressImpl
>   * Method:getHostByAddr
> - * Signature: (I)Ljava/lang/String;
> + * Signature: ([B)Ljava/lang/String;
>   *
>   * Theoretically the UnknownHostException could be enriched with gai error
>   * information. But as it is silently ignored anyway, there's no need for 
> this.
> @@ -668,7 +668,7 @@
>  /*
>   * Class: java_net_Inet6AddressImpl
>   * Method:isReachable0
> - * Signature: ([bII[bI)Z
> + * Signature: ([BII[BII)Z
>   */
>  JNIEXPORT jboolean JNICALL
>  Java_java_net_Inet6AddressImpl_isReachable0(JNIEnv *env, jobject this,
> diff -r 387369ff21a4
> src/java.base/windows/native/libnet/Inet4AddressImpl.c
> --- a/src/java.base/windows/native/libnet/Inet4AddressImpl.c  Wed
> Feb 05 12:20:36 2020 -0300
> +++ b/src/java.base/windows/native/libnet/Inet4AddressImpl.c  Tue
> Mar 03 15:32:21 2020 +0530
> @@ -171,7 +171,7 @@
>  /*
>   * Class: java_net_Inet4AddressImpl
>   * Method:getHostByAddr
> - * Signature: (I)Ljava/lang/String;
> + * Signature: ([B)Ljava/lang/String;
>   *
>   * Theoretically the UnknownHostException could be enriched with gai error
>   * information. But as it is silently ignored anyway, there's no need for 
> this.
> diff -r 387369ff21a4
> src/java.base/windows/native/libnet/Inet6AddressImpl.c
> --- a/src/java.base/windows/native/libnet/Inet6AddressImpl.c  Wed
> Feb 05 12:20:36 2020 -0300
> +++ b/src/java.base/windows/native/libnet/Inet6AddressImpl.c  Tue
> Mar 03 15:32:21 2020 +0530
> @@ -240,7 +240,7 @@
>  /*
>   * Class: java_net_Inet6AddressImpl
>   * Method:getHostByAddr
> - * Signature: (I)Ljava/lang/String;
> + * Signature: ([B)Ljava/lang/String;
>   *
>   * Theoretically the UnknownHostException could be enriched with gai error
>   * information. But as it is silently ignored anyway, there's no need for 
> this.
> @@ -435,7 +435,7 @@
>  /*
>   * Class: java_net_Inet6AddressImpl
>   * Method:isReachable0
> - * Signature: ([bII[bI)Z
> + * Signature: ([BII[BII)Z
>   */
>  JNIEXPORT jboolean JNICALL
>  Java_java_net_Inet6AddressImpl_isReachable0(JNIEnv *env, jobject this,
> 
> 

RE: RFR(s): 8240235: jdk.test.lib.util.JarUtils updates jar files incorrectly

[Langer, Christoph]

> On 03/03/20 12:45 pm, Langer, Christoph wrote:
> > Unfortunately I don't get the mails from Martin ☹
> 
> Same for me. They always end up in spam folder which I usually check
> once a month. I see that there's already
> https://bugs.openjdk.java.net/browse/JDK-8213225 which seems to be the
> same issue.
> 

You are lucky, that you at least get it delivered to the spam folder... 

/Christoph

RE: RFR: removing dead code from jar tool

[Langer, Christoph]

Hi Adam, Lance,

as you've already figured out, attachments sent out to the mailing list will be 
removed. So it's important to generate webrevs and link to it or, for smaller 
patches, inline it. 

Also, the RFR subject is missing a bug ID. From the URL to the webrev I take 
the bug ID is JDK-8230117. Going to JBS it seems that the bug is confidential. 
Could you please either open up the bug before pushing or alternatively, create 
a new, public bug to push this against?

Thanks
Christoph



> -Original Message-
> From: core-libs-dev  On Behalf
> Of Lance Andersen
> Sent: Freitag, 28. Februar 2020 18:04
> To: Sean Mullan 
> Cc: core-libs-dev@openjdk.java.net
> Subject: Re: RFR: removing dead code from jar tool
> 
> sigh it is being removed it appears by the mail server
> 
> I placed a copy here: http://cr.openjdk.java.net/~lancea/JDK-8230117.patch
> 
> 
> > On Feb 28, 2020, at 12:00 PM, Lance Andersen
>  wrote:
> >
> > Here it is again
> >
> >> On Feb 28, 2020, at 11:07 AM, Sean Mullan 
> wrote:
> >>
> >> I think you forgot to attach the patch.
> >>
> >> --Sean
> >>
> >> On 2/28/20 2:28 AM, Adam Sotona wrote:
> >>> Hi,
> >>> I would like to ask for review of the attached patch removing dead code
> from jar tool
> >>> Class files sun.tools.jar.Manifest and sun.tools.jar.SignatureFile appear
> to be dead code and should be removed.
> >>> Build with the patch passes all Tier1, Tier2 and Tier3 tests.
> >>> Thank you,
> >>> Adam
> >
> > 
> > 
> 
> > Lance
> Andersen| Principal Member of Technical Staff | +1.781.442.2037
> > Oracle Java Engineering
> > 1 Network Drive
> > Burlington, MA 01803
> > lance.ander...@oracle.com 
> >
> >
> 
>  
>  
> 
>  Lance Andersen|
> Principal Member of Technical Staff | +1.781.442.2037
> Oracle Java Engineering
> 1 Network Drive
> Burlington, MA 01803
> lance.ander...@oracle.com 
> 
> 

RE: RFR 8239556: (zipfs) remove ExistingChannelCloser facility in zipfs implementation

[Langer, Christoph]

Hi Lance,

thanks, I’ve pushed it now: http://hg.openjdk.java.net/jdk/jdk/rev/c22b369d40b2

I’ll be out next week, so I’ll probably get to JDK-8225507 the week after.

Cheers
Christoph

From: Lance Andersen 
Sent: Freitag, 21. Februar 2020 12:34
To: Langer, Christoph 
Cc: nio-...@openjdk.java.net; core-libs-dev@openjdk.java.net
Subject: Re: RFR 8239556: (zipfs) remove ExistingChannelCloser facility in 
zipfs implementation

Morning Christoph,



On Feb 21, 2020, at 3:11 AM, Langer, Christoph 
mailto:christoph.lan...@sap.com>> wrote:

Hi Lance,

thanks for reviewing. Any results from Mach5? I’ll push it after I get your ok.

It finished late last night and it ran clean


The manifest fix will still apply after this – but you wanted to update the 
webrev anyway with some test updates, right?
Yes, I wanted to clean up the test some.


When both of these fixes are in, I’ll look at augmenting Jai’s current proposal 
for JDK-8225507 to overhaul exception handling in close()().

Assume your last webrev on top of this fix is the lucky winner while you take a 
stab at this clean up :-)

Best
Lance


Cheers
Christoph



From: Lance Andersen 
mailto:lance.ander...@oracle.com>>
Sent: Donnerstag, 20. Februar 2020 20:00
To: Langer, Christoph 
mailto:christoph.lan...@sap.com>>
Cc: nio-...@openjdk.java.net<mailto:nio-...@openjdk.java.net>; 
core-libs-dev@openjdk.java.net<mailto:core-libs-dev@openjdk.java.net>
Subject: Re: RFR 8239556: (zipfs) remove ExistingChannelCloser facility in 
zipfs implementation

Hi Christoph,

This looks good and thank you for tackling. We will need to regenerate the 
web-rev for the Manifest fix after this gets pushed

I am running Mach5 tiers 1-3 now and will let you know when it completes





On Feb 20, 2020, at 8:33 AM, Langer, Christoph 
mailto:christoph.lan...@sap.com>> wrote:

Hi,

please review this cleanup change to remove the ExistingChannelCloser facility 
in zipfs.

Some technical details about why this existed can be found in this mailing list 
thread, where Sherman explained its original purpose: 
https://mail.openjdk.java.net/pipermail/core-libs-dev/2019-April/059839.html. 
At that time we didn't dare to remove it and deferred to further investigation.

Now, when looking at the close() implementation of ZipFileSystem during review 
of JDK-8225507, I had a closer look to exChannelCloser as well and found that 
it should definitely be removed. Its original purpose was to keep a backing 
file and channel for InputStreams that were still open while the sync() method 
was called to update a zip file on disk. However, in its current state, a zip 
file is only synced to disk by sync() during close(), at a time when all 
InputStreams should already be closed.

Closing of the streams is done at the beginning of method close(), in line 
466ff. Closing of any open InputStream would remove it from the list named 
"streams". After that, it must be assured that no InputStreams get created, of 
course. This is accomplished by calling ensureOpen() under readLock, whenever 
an InputStream shall be created. The first step of close() though is to set the 
ZipFileSystem to state "closed", so these checks should all fail. I have, 
however, found one location where this ensureOpen check was missing and added 
it.

jdk/nio/zipfs tests still pass.

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

Thanks
Christoph

<http://oracle.com/us/design/oracle-email-sig-198324.gif>

<http://oracle.com/us/design/oracle-email-sig-198324.gif>Lance Andersen| 
Principal Member of Technical Staff | +1.781.442.2037
Oracle Java Engineering
1 Network Drive
Burlington, MA 01803
lance.ander...@oracle.com<mailto:lance.ander...@oracle.com>






[cid:image001.gif@01D5E8E4.A340D420]<http://oracle.com/us/design/oracle-email-sig-198324.gif>

<http://oracle.com/us/design/oracle-email-sig-198324.gif>Lance Andersen| 
Principal Member of Technical Staff | +1.781.442.2037
Oracle Java Engineering
1 Network Drive
Burlington, MA 01803
lance.ander...@oracle.com<mailto:lance.ander...@oracle.com>

RE: RFR [XS]: 8238947: tools/jpackage tests fail with old rpmbuild versions

[Langer, Christoph]

Hi Matthias,

I think this is good.

Best regards
Christoph

> -Original Message-
> From: core-libs-dev  On Behalf
> Of Baesken, Matthias
> Sent: Donnerstag, 20. Februar 2020 14:15
> To: core-libs-dev@openjdk.java.net
> Subject: [CAUTION] RFR [XS]: 8238947: tools/jpackage tests fail with old
> rpmbuild versions
> 
> Hello, please review this small change adjusting the minimal rpmbuild version
> of tools/jpackage tests .
> 
> Currently the tools/jpackage tests, for example
> jdk/test/jdk/tools/jpackage/linux/AppCategoryTest.java fail on older distros
> with rpmbuild tools < 4.10 .
> This can be seen on Suse Linux (SLES11) with rpmbuild 4.4 and on RedHat 6
> with rpmbuild 4.8 .
> For example, rpmbuild returns an unexpected "rpmbuild: no spec files given
> for build" when calling :
> 
> > rpmbuild --eval=%{_target_cpu}
> > x86_64
> > rpmbuild: no spec files given for build
> 
> Probably we should check for at least rpmbuild version 4.10 when executing
> the tests (on Suse Linux 12 and on RedHat 7+ we have rpmbuild 4.11 or
> higher).
> 
> 
> Bug/webrev :
> 
> https://bugs.openjdk.java.net/browse/JDK-8238947
> 
> http://cr.openjdk.java.net/~mbaesken/webrevs/8238947.0/
> 
> Thanks, Matthias

RE: RFR 8239556: (zipfs) remove ExistingChannelCloser facility in zipfs implementation

[Langer, Christoph]

Hi Lance,

thanks for reviewing. Any results from Mach5? I'll push it after I get your ok.

The manifest fix will still apply after this - but you wanted to update the 
webrev anyway with some test updates, right?

When both of these fixes are in, I'll look at augmenting Jai's current proposal 
for JDK-8225507 to overhaul exception handling in close()().

Cheers
Christoph



From: Lance Andersen 
Sent: Donnerstag, 20. Februar 2020 20:00
To: Langer, Christoph 
Cc: nio-...@openjdk.java.net; core-libs-dev@openjdk.java.net
Subject: Re: RFR 8239556: (zipfs) remove ExistingChannelCloser facility in 
zipfs implementation

Hi Christoph,

This looks good and thank you for tackling. We will need to regenerate the 
web-rev for the Manifest fix after this gets pushed

I am running Mach5 tiers 1-3 now and will let you know when it completes




On Feb 20, 2020, at 8:33 AM, Langer, Christoph 
mailto:christoph.lan...@sap.com>> wrote:

Hi,

please review this cleanup change to remove the ExistingChannelCloser facility 
in zipfs.

Some technical details about why this existed can be found in this mailing list 
thread, where Sherman explained its original purpose: 
https://mail.openjdk.java.net/pipermail/core-libs-dev/2019-April/059839.html. 
At that time we didn't dare to remove it and deferred to further investigation.

Now, when looking at the close() implementation of ZipFileSystem during review 
of JDK-8225507, I had a closer look to exChannelCloser as well and found that 
it should definitely be removed. Its original purpose was to keep a backing 
file and channel for InputStreams that were still open while the sync() method 
was called to update a zip file on disk. However, in its current state, a zip 
file is only synced to disk by sync() during close(), at a time when all 
InputStreams should already be closed.

Closing of the streams is done at the beginning of method close(), in line 
466ff. Closing of any open InputStream would remove it from the list named 
"streams". After that, it must be assured that no InputStreams get created, of 
course. This is accomplished by calling ensureOpen() under readLock, whenever 
an InputStream shall be created. The first step of close() though is to set the 
ZipFileSystem to state "closed", so these checks should all fail. I have, 
however, found one location where this ensureOpen check was missing and added 
it.

jdk/nio/zipfs tests still pass.

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

Thanks
Christoph

[cid:image001.gif@01D5E896.DE3E9740]<http://oracle.com/us/design/oracle-email-sig-198324.gif>

<http://oracle.com/us/design/oracle-email-sig-198324.gif>Lance Andersen| 
Principal Member of Technical Staff | +1.781.442.2037
Oracle Java Engineering
1 Network Drive
Burlington, MA 01803
lance.ander...@oracle.com<mailto:lance.ander...@oracle.com>

RFR 8239556: (zipfs) remove ExistingChannelCloser facility in zipfs implementation

[Langer, Christoph]

Hi,

please review this cleanup change to remove the ExistingChannelCloser facility 
in zipfs.

Some technical details about why this existed can be found in this mailing list 
thread, where Sherman explained its original purpose: 
https://mail.openjdk.java.net/pipermail/core-libs-dev/2019-April/059839.html. 
At that time we didn't dare to remove it and deferred to further investigation.

Now, when looking at the close() implementation of ZipFileSystem during review 
of JDK-8225507, I had a closer look to exChannelCloser as well and found that 
it should definitely be removed. Its original purpose was to keep a backing 
file and channel for InputStreams that were still open while the sync() method 
was called to update a zip file on disk. However, in its current state, a zip 
file is only synced to disk by sync() during close(), at a time when all 
InputStreams should already be closed.

Closing of the streams is done at the beginning of method close(), in line 
466ff. Closing of any open InputStream would remove it from the list named 
"streams". After that, it must be assured that no InputStreams get created, of 
course. This is accomplished by calling ensureOpen() under readLock, whenever 
an InputStream shall be created. The first step of close() though is to set the 
ZipFileSystem to state "closed", so these checks should all fail. I have, 
however, found one location where this ensureOpen check was missing and added 
it.

jdk/nio/zipfs tests still pass.

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

Thanks
Christoph

RE: 8238953: tools/jpackage tests do not work on Ubuntu Linux

[Langer, Christoph]

Hi Matthias,

Looks good to me now.

Cheers
Christoph

> -Original Message-
> From: Baesken, Matthias 
> Sent: Dienstag, 18. Februar 2020 16:55
> To: Langer, Christoph ; core-libs-
> d...@openjdk.java.net; Alexey Semenyuk 
> Subject: RE: 8238953: tools/jpackage tests do not work on Ubuntu Linux
> 
> Ok why not,  new webrev :
> 
> http://cr.openjdk.java.net/~mbaesken/webrevs/8238953.2/
> 
> Thanks, Matthias
> 
> 
> 
> >
> > Hi Matthias,
> >
> > you could improve the patch a bit by starting TKit:: isUbuntu() with
> >
> > if (!isLinux()) {
> > return false;
> > }
> >
> > And then, in PackageType, you could simply do:
> >
> > private final static Set DISABLED_PACKAGERS =
> Optional.ofNullable(
> > TKit.tokenizeConfigProperty("disabledPackagers")).orElse(
> > TKit.isUbuntu() ? Set.of("rpm") : Collections.emptySet());
> >
> > Best regards
> > Christoph
> >
> > > -Original Message-
> > > From: core-libs-dev  On
> Behalf
> > > Of Baesken, Matthias
> > > Sent: Dienstag, 18. Februar 2020 09:14
> > > To: core-libs-dev@openjdk.java.net; Alexey Semenyuk
> > > 
> > > Subject: [CAUTION] RE: 8238953: tools/jpackage tests do not work on
> > > Ubuntu Linux
> > >
> > > Ping ...  are you fine with the latest version ?
> > >
> > > Best Regards, Matthias
> > >
> > > >
> > > > Hi Alexey  , I like your idea to do the handling in
> > > > test/jdk/tools/jpackage/helpers/jdk/jpackage/test/PackageType.java  .
> > > >
> > > > New webrev :
> > > >
> > > > http://cr.openjdk.java.net/~mbaesken/webrevs/8238953.1/
> > > >
> > > >
> > > >
> > > > Best regards, Matthias
> > > >
> > > >

RE: 8238953: tools/jpackage tests do not work on Ubuntu Linux

[Langer, Christoph]

Hi Matthias,

you could improve the patch a bit by starting TKit:: isUbuntu() with

if (!isLinux()) {
return false;
}

And then, in PackageType, you could simply do:

private final static Set DISABLED_PACKAGERS = Optional.ofNullable(
TKit.tokenizeConfigProperty("disabledPackagers")).orElse(
TKit.isUbuntu() ? Set.of("rpm") : Collections.emptySet());

Best regards
Christoph

> -Original Message-
> From: core-libs-dev  On Behalf
> Of Baesken, Matthias
> Sent: Dienstag, 18. Februar 2020 09:14
> To: core-libs-dev@openjdk.java.net; Alexey Semenyuk
> 
> Subject: [CAUTION] RE: 8238953: tools/jpackage tests do not work on
> Ubuntu Linux
> 
> Ping ...  are you fine with the latest version ?
> 
> Best Regards, Matthias
> 
> >
> > Hi Alexey  , I like your idea to do the handling in
> > test/jdk/tools/jpackage/helpers/jdk/jpackage/test/PackageType.java  .
> >
> > New webrev :
> >
> > http://cr.openjdk.java.net/~mbaesken/webrevs/8238953.1/
> >
> >
> >
> > Best regards, Matthias
> >
> >

RE: RFR: JDK-8238380: java.base/unix/native/libjava/childproc.c "multiple definition" link errors with GCC10

[Langer, Christoph]

As far as another reviewer is needed - looks good to me, too 

/Christoph

> -Original Message-
> From: core-libs-dev  On Behalf
> Of Thomas Stüfe
> Sent: Donnerstag, 6. Februar 2020 07:49
> To: Patrick Zhang OS 
> Cc: core-libs-dev 
> Subject: Re: RFR: JDK-8238380: java.base/unix/native/libjava/childproc.c
> "multiple definition" link errors with GCC10
> 
> I can sponsor this for you.
> 
> .. Thomas
> 
> On Thu, Feb 6, 2020, 04:35 Patrick Zhang OS
> 
> wrote:
> 
> > Does this require one more “yes” from any other reviewer?
> >
> >
> >
> > And may I ask for any committer’s help to sponsor and push it (once
> > approved)? Thanks in advance.
> >
> >
> >
> > Regards
> >
> > Patrick
> >
> >
> >
> > *From:* Thomas Stüfe 
> > *Sent:* Wednesday, February 5, 2020 11:16 PM
> > *To:* Patrick Zhang OS 
> > *Cc:* core-libs-dev 
> > *Subject:* Re: RFR: JDK-8238380:
> > java.base/unix/native/libjava/childproc.c "multiple definition" link errors
> > with GCC10
> >
> >
> >
> > All still good.
> >
> >
> >
> > ...Thomas
> >
> >
> >
> > On Wed, Feb 5, 2020 at 3:42 PM Patrick Zhang OS <
> > patr...@os.amperecomputing.com> wrote:
> >
> > Thanks for your comments, Thomas.
> >
> >
> >
> > I have no accurate knowledge regarding why parentPathv was initially
> > written as a global var instead of a member in ChildStuff struct
> > initialized in jspawnhelper.c. However the suggested change might not be
> > very straight-forward as there are other references such as:
> > Java_java_lang_ProcessImpl_init(), spawnChild() in
> > libjava\ProcessImpl_md.c, etc. And this goes beyond my original intention
> > to fix the build errors purely, and try best to avoid any side effect. So I
> > would leave the further improvement to others, and keep it as-is. Thanks.
> >
> >
> >
> > Updated the change a bit (the header comments):
> > http://cr.openjdk.java.net/~qpzhang/8238380/webrev.02/
> >
> >
> >
> > Regards
> >
> > Patrick
> >
> >
> >
> > *From:* Thomas Stüfe 
> > *Sent:* Wednesday, February 5, 2020 2:30 PM
> > *To:* Patrick Zhang OS 
> > *Cc:* core-libs-dev 
> > *Subject:* Re: RFR: JDK-8238380:
> > java.base/unix/native/libjava/childproc.c "multiple definition" link errors
> > with GCC10
> >
> >
> >
> > Hi Patrick,
> >
> >
> >
> > You patch looks alright.
> >
> >
> >
> > But I wonder whether a cleaner solution would not be to add a
> parentPathv
> > pointer to the ChildStuff structure and pass it down to JDK_execvpe that
> > way, like we do for all other input arguments of that function. If it is an
> > input argument, seems strange to pass it as global variable. I leave that
> > for others to decide though, your patch certainly works.
> >
> >
> >
> > Cheers, Thomas
> >
> >
> >
> > On Tue, Feb 4, 2020 at 3:39 PM Patrick Zhang OS <
> > patr...@os.amperecomputing.com> wrote:
> >
> > Hi
> >
> > I split the original patch into parts for corresponding function review,
> > here is the very simple change to java.base/unix/native/libjava/childproc.c
> > and .h.
> > Could core-libs-dev group help review and sponsor this? thanks in advance.
> >
> > JBS: https://bugs.openjdk.java.net/browse/JDK-8238380 (a sub-task of
> > JDK-8235903)
> > Webrev: http://cr.openjdk.java.net/~qpzhang/8238380/webrev.01/
> > Test: ran jtreg tier1, jdk built with GCC4.8.5, GCC9, and GCC10, no
> > regression found. I don’t have any specific function tests, so this is just
> > a smoke test.
> >
> > Regards
> > Patrick
> >
> > From: Martin Buchholz
> mailto:marti...@google.com>>
> > Sent: Monday, December 16, 2019 10:44 AM
> > To: Patrick Zhang OS  > patr...@os.amperecomputing.com>>; net-dev  d...@openjdk.java.net
> > >; OpenJDK  d...@openjdk.java.net
> > >
> > Cc: core-libs-dev  > core-libs-dev@openjdk.java.net>>
> > Subject: Re: RFR: JDK-8235903: GCC default -fno-common exposes
> "multiple
> > definition" link errors
> >
> > forwarded to other teams for review.
> >
> > On Fri, Dec 13, 2019 at 3:14 AM Patrick Zhang OS <
> >
> patr...@os.amperecomputing.com m>>
> > wrote:
> > Hi
> >
> > Please review this patch, if it should be reviewed by any group other than
> > core-libs, please help forward it. Thanks.
> >
> > JBS: https://bugs.openjdk.java.net/browse/JDK-8235903
> > Webrev: http://cr.openjdk.java.net/~qpzhang/8235903/webrev.01/
> >
> > A recent GCC patch (supposed to be in GCC 10) exposes a couple of
> > "multiple definition" link errors when building the jdk tip.
> >
> > [PATCH] PR85678: Change default to -fno-common
> > https://gcc.gnu.org/ml/gcc-patches/2019-10/msg01847.html
> >
> > For example, the error message looks like:
> > * For target support_native_java.base_libjava_BUILD_LIBJAVA_link:
> > build/support/native/java.base/libjava/childproc.o:(.bss+0x0): multiple
> > definition of `parentPathv'
> > build/support/native/java.base/libjava/ProcessImpl_md.o:(.bss+0x0): first
> > defined here
> > collect2: error: ld returned 1 exit status
> >
> > This 

RE: RFR: JDK-8238225: Issues reported after replacing symlink at Contents/MacOS/libjli.dylib with binary

[Langer, Christoph]

Hi Erik,

> Hello,
> 
> New webrev: http://cr.openjdk.java.net/~erikj/8238225/webrev.02/
> 
> On 2020-02-05 02:17, Langer, Christoph wrote:
> > Hi,
> >
> > we've tested the patch and all reported failure scenarios we're aware of
> are fixed with that, so basically, ship it 
> >
> > As for the code review part:
> > The patch I've tested had removed the "-1" in line 532, so that seems to be
> correct. As Magnus wrote, the pattern seems to be copied from the lines
> above. So I think in line 518, subtracting -1 from "sizeOfLastPathComponent"
> seems to be incorrect as well. So it could be corrected in the same fix, I
> guess.
> Yes, I did indeed just copy the pattern, but it also seems you are right
> about the -1, so I fixed it in both locations. I also figured reusing
> the variables wasn't very nice, so add the "Alt" prefix in all of them.

Looks good. Given there are other even longer lines, maybe you would want to 
join the comment lines 523 and 524?

> > And there's one other minor thing: I tried to execute JliLaunchTest for the
> bundle scenario and had to make some adaptions to the example call to
> "make test-only ..." in line 66 of 
> test/jdk/tools/launcher/JliLaunchTest.java. I
> guess the example could be more generic if it were changed to:
> >
> > 66 // $ make test-only
> TEST=test/jdk/tools/launcher/JliLaunchTest.java \
> > 67 // JDK_IMAGE_DIR=$PWD/images/jdk-bundle/jdk-
> 15.jdk/Contents/Home
> >
> > (e.g. use relative paths inside the build directory)
> 
> Right, the name of the output dir may change. I didn't intend the
> example to be verbatim correct for everyone (hence the "something like"
> wording) but I see your point. Changed it.

Yep, it wouldn't always be verbatim correct, e.g. if jdk/jdk advances to jdk16, 
it'll be wrong. However maybe you could still strip the "open" from the 
parameter "TEST=open/test/jdk/tools/launcher/JliLaunchTest.java" in line 66? I 
guess that would be most correct for OpenJDK...

But anyway, the new webrev looks good. No need for further update/discussion 
from my end. The sooner it's in the merrier I am 

Cheers
Christoph

RE: RFR: JDK-8238225: Issues reported after replacing symlink at Contents/MacOS/libjli.dylib with binary

[Langer, Christoph]

Hi,

we've tested the patch and all reported failure scenarios we're aware of are 
fixed with that, so basically, ship it 

As for the code review part:
The patch I've tested had removed the "-1" in line 532, so that seems to be 
correct. As Magnus wrote, the pattern seems to be copied from the lines above. 
So I think in line 518, subtracting -1 from "sizeOfLastPathComponent" seems to 
be incorrect as well. So it could be corrected in the same fix, I guess.

And there's one other minor thing: I tried to execute JliLaunchTest for the 
bundle scenario and had to make some adaptions to the example call to "make 
test-only ..." in line 66 of test/jdk/tools/launcher/JliLaunchTest.java. I 
guess the example could be more generic if it were changed to:

66 // $ make test-only 
TEST=test/jdk/tools/launcher/JliLaunchTest.java \
67 // 
JDK_IMAGE_DIR=$PWD/images/jdk-bundle/jdk-15.jdk/Contents/Home

(e.g. use relative paths inside the build directory)

Thanks
Christoph

> -Original Message-
> From: Magnus Ihse Bursie 
> Sent: Mittwoch, 5. Februar 2020 10:54
> To: Erik Joelsson ; core-libs-dev  d...@openjdk.java.net>; build-dev ; Langer,
> Christoph 
> Subject: Re: RFR: JDK-8238225: Issues reported after replacing symlink at
> Contents/MacOS/libjli.dylib with binary
> 
> On 2020-02-04 18:45, Erik Joelsson wrote:
> > Does anyone have an opinion on this?
> The solution seems sound to me. I'm having a hard time figuring out if
> the string manipulations in java_md_macosx.m are correct; as Christoph
> is saying, they might not be. I realize you have copied an existing
> pattern, and is likely subject to constraints, but that does not make it
> easier to read.
> 
> /Magnus
> >
> > /Erik
> >
> > On 2020-01-31 07:31, Erik Joelsson wrote:
> >> In JDK-8235687 the MacOS bundle distribution of the JDK was changed
> >> to conform to Apple requirements by changing
> >> Contents/MacOS/libjli.dylib from a symlink into
> >> ../Home/lib/libjli.dylib to a copy of that file. The problem with
> >> having a symlink there is that Contents/MacOS/libjli.dylib is the
> >> declared CFBundleExecutable of the bundle and that executable may not
> >> be a symlink. The history around why that particular library was put
> >> there seems lost in ancient history. All we know is that it was there
> >> when Apple donated the Mac port and according to this bug report,
> >> there are users out there relying on it.
> >>
> >> When changing Contents/MacOS/libjli.dylib to a copy, loading that
> >> dylib and using it to launch a JVM no longer works. This patch fixes
> >> that by making libjli.dylib aware of potentially being located there
> >> and if so, finding the JDK home dir in ../Home.
> >>
> >> I've also expanded the existing test for launching a JVM through
> >> libjli.dylib directly to also test this location when possible. In
> >> local testing, this will not be covered unless the user explicitly
> >> specifies that the JDK under test should be the bundle image on the
> >> command line like this:
> >>
> >> $ make test-only TEST=open/test/jdk/tools/launcher/JliLaunchTest.java
> >> JDK_IMAGE_DIR=$PWD/build/macosx-x64/images/jdk-bundle/jdk-
> 15.jdk/Contents/Home
> >>
> >> But, at least in Oracle's distributed testing, the JDK on MacOS is
> >> distributed in the bundle layout, so there this functionality will be
> >> tested.
> >>
> >> Bug: https://bugs.openjdk.java.net/browse/JDK-8238225
> >>
> >> Webrev: http://cr.openjdk.java.net/~erikj/8238225/webrev.01/
> >>
> >> /Erik
> >>

RE: RFR: JDK-8238225: Issues reported after replacing symlink at Contents/MacOS/libjli.dylib with binary

[Langer, Christoph]

Hi Erik,

 > Does anyone have an opinion on this?
Yep, first of all, thanks for doing this work. Sorry for not replying earlier 
but FOSDEM/openjdkcw has kept us a bit busy. We're in the process of testing 
your change in the reported cases where behavior is broken and shall supply you 
with results soon.

There's one nit in the coding I've spotted:
Line 532: if (0 == strncmp(realPathToSelf + indexOfLastPathComponent, 
altLastPathComponent, sizeOfLastPathComponent - 1)) {

I'm not sure if you want to subtract 1 from sizeOfLastPathComponent here again, 
as I believe this was done in line 526 already and sizeOfLastPathComponent 
should already reflect the strlen of the string you want to compare to.

Best regards
Christoph

RE: RFR 7143743: (zipfs) Potential memory leak with zip provider

[Langer, Christoph]

Hi,

Looks good to me 

Cheers
Christoph

From: Lance Andersen 
Sent: Montag, 13. Januar 2020 21:26
To: Alan Bateman 
Cc: Jaikiran Pai ; Langer, Christoph 
; nio-...@openjdk.java.net; 
core-libs-dev@openjdk.java.net
Subject: Re: RFR 7143743: (zipfs) Potential memory leak with zip provider




On Jan 13, 2020, at 1:53 PM, Alan Bateman 
mailto:alan.bate...@oracle.com>> wrote:

On 13/01/2020 10:31, Jaikiran Pai wrote:

Hello Christoph,

Setting inodes to null sounds fine to me and as you say since its usage
is already guarded by ensureOpen, IMO, it should be fine. I've now
updated the patch to set inodes to null in close() and the new updated
webrev is at https://cr.openjdk.java.net/~jpai/webrev/7143743/3/webrev/
This version looks good except it might be better if the comment just says that 
it clears the inodes map to allow the keys/values be GC’ed.

I revised the comment to:



$ hg diff
diff -r 9338d0f52b2e 
src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java
--- a/src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java  Mon Jan 
13 11:51:45 2020 -0500
+++ b/src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java  Mon Jan 
13 15:24:37 2020 -0500
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2009, 2019, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2009, 2020, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -490,6 +490,14 @@
 def.end();
 }

+beginWrite();// lock and sync
+try {
+// Clear the map so that its keys & values can be garbage collected
+inodes = null;
+} finally {
+endWrite();
+}
+
 IOException ioe = null;
 synchronized (tmppaths) {
 for (Path p : tmppaths) {
$
———

I will push the change tomorrow barring any hiccups with Mach5 or additional 
comments….

Best
lance


-Alan

[cid:image001.gif@01D5CACC.1E85C500]<http://oracle.com/us/design/oracle-email-sig-198324.gif>

<http://oracle.com/us/design/oracle-email-sig-198324.gif>Lance Andersen| 
Principal Member of Technical Staff | +1.781.442.2037
Oracle Java Engineering
1 Network Drive
Burlington, MA 01803
lance.ander...@oracle.com<mailto:lance.ander...@oracle.com>

RE: RFR 7143743: (zipfs) Potential memory leak with zip provider

[Langer, Christoph]

Hi,

I'm wondering whether we shouldn't just do "inodes = null;"? I guess this is 
cheaper and accesses to the inodes map on a closed filesystem object should not 
happen anyway (e.g. all is guarded by ensureOpen()). Other than that, the 
change looks fine.

Cheers
Christoph

> -Original Message-
> From: nio-dev  On Behalf Of Jaikiran
> Pai
> Sent: Samstag, 11. Januar 2020 11:24
> To: Alan Bateman ; nio-...@openjdk.java.net
> Cc: core-libs-dev@openjdk.java.net
> Subject: Re: RFR 7143743: (zipfs) Potential memory leak with zip provider
> 
> Hello Alan,
> 
> On 11/01/20 3:37 pm, Alan Bateman wrote:
> > On 11/01/2020 09:51, Jaikiran Pai wrote:
> >> :
> >>
> >> The commit here fixes that issue by simply clearing the "inodes" map in
> >> the jdk.nio.zipfs.ZipFileSystem.close() method. I have checked the usage
> >> of the "inodes" map and from what I see, it's usage in various places is
> >> guarded by "ensureOpen" checks, which means that once the
> ZipFileSystem
> >> instance is closed, the contents of these "inodes" map is no longer
> >> relevant and hence clearing it shouldn't cause any issues.
> >>
> > Clearing the inodes map should be okay for cases where something is
> > holding a reference to a closed zip file system. However, you should
> > look at beginWrite/endWrite so that all access to the map is
> > consistently synchronized.
> >
> Thank you very much for that input - I hadn't considered the concurrency
> aspect of it. Based on your input and after looking at the usage of the
> "inodes", I have now updated the patch to use proper locks during the
> clearing of the inodes. The updated webrev is available at
> https://cr.openjdk.java.net/~jpai/webrev/7143743/2/webrev/
> 
> -Jaikiran

RE: [14] RFR: 8236495, open/test/jdk/java/util/Locale/LocaleProvidersRun.java failed on mac 10.14 with de_DE locale.

[Langer, Christoph]

Hi Naoto,

The change looks good. Thanks for fixing this. We're seeing it regularly in our 
test infrastructure where we're running Mac systems with German locale.

Best regards
Christoph

> -Original Message-
> From: i18n-dev  On Behalf Of
> naoto.s...@oracle.com
> Sent: Montag, 6. Januar 2020 18:05
> To: i18n-...@openjdk.java.net; core-libs-dev  d...@openjdk.java.net>
> Subject:  [14] RFR: 8236495,
> open/test/jdk/java/util/Locale/LocaleProvidersRun.java failed on mac 10.14
> with de_DE locale.
> 
> Hi,
> 
> Please review the fix to the following issue:
> 
> https://bugs.openjdk.java.net/browse/JDK-8236495
> 
> The proposed changeset is located at:
> 
> https://cr.openjdk.java.net/~naoto/8236495/webrev.00/
> 
> The test case for the fix to 8232860 was only intended for the US
> locale. Simply adding the default locale check will fix the test case.
> 
> Naoto

RE: RFR (S): 8235750: [jpackage] Cleanup imports in WinMsiBundler.java

[Langer, Christoph]

Thanks, Andy and Alexey for the reviews. I've pushed it.

Cheers
Christoph

> -Original Message-
> From: core-libs-dev  On Behalf
> Of Andy Herrick
> Sent: Mittwoch, 11. Dezember 2019 15:25
> To: core-libs-dev@openjdk.java.net
> Subject: Re: RFR (S): 8235750: [jpackage] Cleanup imports in
> WinMsiBundler.java
> 
> looks good - thank you.
> 
> /Andy
> 
> On 12/11/2019 5:02 AM, Langer, Christoph wrote:
> > Hi,
> >
> > please review this import statements cleanup for
> src/jdk.incubator.jpackage/windows/classes/jdk/incubator/jpackage/interna
> l/WinMsiBundler.java. I stumbled over an issue when I imported the
> jpackage project into Eclipse.
> >
> > Due to importing both, static
> jdk.incubator.jpackage.internal.StandardBundlerParam.* and static
> jdk.incubator.jpackage.internal.WindowsBundlerParam.*, Eclipse thinks
> some symbols are ambiguous.
> >
> > I would also remove an unused list object.
> >
> > Webrev: http://cr.openjdk.java.net/~clanger/webrevs/8235750.0/
> > Bug: https://bugs.openjdk.java.net/browse/JDK-8235750
> >
> > Thanks
> > Christoph
> >

RFR (S): 8235750: [jpackage] Cleanup imports in WinMsiBundler.java

[Langer, Christoph]

Hi,

please review this import statements cleanup for 
src/jdk.incubator.jpackage/windows/classes/jdk/incubator/jpackage/internal/WinMsiBundler.java.
 I stumbled over an issue when I imported the jpackage project into Eclipse.

Due to importing both, static 
jdk.incubator.jpackage.internal.StandardBundlerParam.* and static 
jdk.incubator.jpackage.internal.WindowsBundlerParam.*, Eclipse thinks some 
symbols are ambiguous.

I would also remove an unused list object.

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

Thanks
Christoph

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

[Langer, Christoph]

Hi Gerard,

this looks good.

Cheers
Christoph

> -Original Message-
> From: gerard ziemski 
> Sent: Samstag, 7. Dezember 2019 14:34
> To: hotspot-dev developers ; core-libs-
> d...@openjdk.java.net; Claes Redestad ;
> Mandy Chung ; David Holmes
> ; Sergey Bylokhov
> ; Langer, Christoph
> 
> Subject: Re: RFR (M) 8223261 "JDK-8189208 followup - remove
> JDK_GetVersionInfo0 and the supporting code"
>
> Hi all,
>
> Please review this revision 2 of a cleanup, where we remove
> JDK_GetVersionInfo0 and related code, since we can get build versions
> directly from within the VM itself.
>
> The rev2 differs from rev1 in that it's simpler due to JDK-8234185,
> which was just checked in yesterday. Waiting for this fix to go in
> first, allowed us not to have to delete the jdk_util.h file, which in
> turn means that we don't have to touch all those files that used that
> include.
>
>
> bug: https://bugs.openjdk.java.net/browse/JDK-8223261
> webrev: http://cr.openjdk.java.net/~gziemski/8223261_rev2
> tests: passes Mach5 tier1,2,3 (another tier1,2,3,4,5,6 in progress...)
>
>
> cheers

RE: RFR(S): 8220348: [ntintel] asserts about copying unalinged array

[Langer, Christoph]

Hi Martin,

ok, you are the author – so I won’t insist. 

Best regards
Christoph

From: Doerr, Martin 
Sent: Freitag, 6. Dezember 2019 12:22
To: Langer, Christoph 
Cc: core-libs-dev@openjdk.java.net; security-dev 
; Lindenmaier, Goetz 
; Thomas Stüfe 
Subject: RE: RFR(S): 8220348: [ntintel] asserts about copying unalinged array

Hi Christoph,

that would work, but I don’t want to pollute this file with compiler specific 
defines. In addition, I don’t like introducing a macro which works on some 
platforms and does nothing on other ones (which is the case for hotspot’s 
ATTRIBUTE_ALIGNED).

Because Windows 32 bit is the only affected platform, I prefer not to touch 
other ones. Are you ok with webrev.00 as it is?

Best regards,
Martin



From: Langer, Christoph 
mailto:christoph.lan...@sap.com>>
Sent: Donnerstag, 5. Dezember 2019 12:16
To: Doerr, Martin mailto:martin.do...@sap.com>>
Cc: core-libs-dev@openjdk.java.net<mailto:core-libs-dev@openjdk.java.net>; 
security-dev 
mailto:security-...@openjdk.java.net>>; 
Lindenmaier, Goetz 
mailto:goetz.lindenma...@sap.com>>; Thomas Stüfe 
mailto:thomas.stu...@gmail.com>>
Subject: RE: RFR(S): 8220348: [ntintel] asserts about copying unalinged array

Hi Martin,

I can see that both places already include headers from 
src/java.base/share/native/libjava/. I suggest adding the define in jni_util.h. 
WindowsPreferences.c already includes it.

Best regards
Christoph

From: Doerr, Martin mailto:martin.do...@sap.com>>
Sent: Donnerstag, 5. Dezember 2019 12:00
To: Thomas Stüfe mailto:thomas.stu...@gmail.com>>; 
Langer, Christoph mailto:christoph.lan...@sap.com>>
Cc: core-libs-dev@openjdk.java.net<mailto:core-libs-dev@openjdk.java.net>; 
security-dev 
mailto:security-...@openjdk.java.net>>; 
Lindenmaier, Goetz mailto:goetz.lindenma...@sap.com>>
Subject: RE: RFR(S): 8220348: [ntintel] asserts about copying unalinged array

Hi Thomas and Christoph,

thanks for the reviews.

Other code in java.security.jgss also uses these #defined checks:
src/java.security.jgss/share/native/libj2gss/gssapi.h:#if defined (_WIN32) && 
defined (_MSC_VER)

I’d like to have it consistent with that.

@Christoph: I think having ATTRIBUTE_ALIGNED(x) would be nice. It could get 
defined easily for Visual Studio and GCC, but some other compilers may be more 
difficult. Note that this macro is only defined for a selected set of compilers 
in hotspot. If we wanted to add it, where should we define it?

Windows 32 bit seems to be the only platform which is affected by the problem 
that jlongs on stack are not 8 byte aligned.
(AFAIK, GCC uses -malign-double by default on 32 bit which should do the job 
for jlongs, too:
https://gcc.gnu.org/onlinedocs/gcc-4.5.3/gcc/i386-and-x86_002d64-Options.html)

Best regards,
Martin


From: Thomas Stüfe mailto:thomas.stu...@gmail.com>>
Sent: Mittwoch, 4. Dezember 2019 17:56
To: Doerr, Martin mailto:martin.do...@sap.com>>
Cc: core-libs-dev@openjdk.java.net<mailto:core-libs-dev@openjdk.java.net>; 
security-dev 
mailto:security-...@openjdk.java.net>>; 
Lindenmaier, Goetz mailto:goetz.lindenma...@sap.com>>
Subject: Re: RFR(S): 8220348: [ntintel] asserts about copying unalinged array

Hi Martin,

this makes sense. This is the right way to force alignment. I do not like the 
platform code in the shared file but do not think this is a big deal.

+#if defined (_WIN32) && defined (_MSC_VER)

Why do you think we need _MSC_VER too? Is OpenJDK on Windows even buildable 
with anything other than VC++?

Cheers, Thomas

On Mon, Dec 2, 2019 at 4:14 PM Doerr, Martin 
mailto:martin.do...@sap.com>> wrote:
Hi,

I'd like to propose a fix for an old issue on 32 bit Windows (also for an 11u 
backport):
https://bugs.openjdk.java.net/browse/JDK-8220348

Some jdk native methods use jni_SetLongArrayRegion with a stack allocated 
buffer.
jni_SetLongArrayRegion uses Copy::conjoint_jlongs_atomic which requires jlongs 
to be 8 byte aligned (asserted).
However, Windows 32 bit only uses 4 byte alignment for jlong arrays by default.
I found such issues in the following files:
src/java.prefs/windows/native/libprefs/WindowsPreferences.c
src/java.security.jgss/share/native/libj2gss/GSSLibStub.c
I suggest to use __declspec(align(8)) there.

Webrev:
http://cr.openjdk.java.net/~mdoerr/8220348_ntintel_stack_align/webrev.00/
Please review.

I think using 8 byte alignment is not a disadvantage for 64 bit.

I guess there are still people interested in this platform with jdk14. 
Otherwise I could contribute it as 11u only fix.

Is there a better way to force 8 byte alignment for jlongs or jlong arrays on 
stack?
Best regards,
Martin

RE: RFR: 8234185: Cleanup usage of canonicalize function between libjava, hotspot and libinstrument

[Langer, Christoph]

Thanks, Alan.

Then I'll push it now. A final jdk-submit run succeeded 
(mach5-one-clanger-JDK-8234185-4-20191206-0819-7283662).

> -Original Message-
> From: Alan Bateman 
> Sent: Freitag, 6. Dezember 2019 13:17
> To: Langer, Christoph ; David Holmes
> ; daniel.daughe...@oracle.com
> Cc: core-libs-dev@openjdk.java.net; hotspot-runtime-
> d...@openjdk.java.net; gerard ziemski 
> Subject: Re: RFR: 8234185: Cleanup usage of canonicalize function between
> libjava, hotspot and libinstrument
> 
> On 06/12/2019 08:04, Langer, Christoph wrote:
> > Thanks, David.
> >
> > I'll run the final change once again through jdk-submit befor pushing.
> >
> > Alan, Dan, may I consider this reviewed by either of you?
> >
> Yes, I think this looks okay.
> 
> -Alan

RE: RFR: 8234185: Cleanup usage of canonicalize function between libjava, hotspot and libinstrument

[Langer, Christoph]

Thanks, David.

I'll run the final change once again through jdk-submit befor pushing.

Alan, Dan, may I consider this reviewed by either of you?

Thanks
Christoph

> -Original Message-
> From: David Holmes 
> Sent: Freitag, 6. Dezember 2019 08:02
> To: Langer, Christoph ;
> daniel.daughe...@oracle.com
> Cc: core-libs-dev@openjdk.java.net; hotspot-runtime-
> d...@openjdk.java.net; Alan Bateman ; gerard
> ziemski 
> Subject: Re: RFR: 8234185: Cleanup usage of canonicalize function between
> libjava, hotspot and libinstrument
> 
> Hi Christoph,
> 
> On 6/12/2019 2:12 am, Langer, Christoph wrote:
> > Hi David,
> >
> > I prepared an updated webrev:
> http://cr.openjdk.java.net/~clanger/webrevs/8234185.3/
> >
> >> src/java.base/windows/native/libjava/canonicalize_md.c
> >>
> >> +// We can't include jdk_util.h here because the file is used in Oracle
> >> in another context
> >> +// #include "jdk_util.h"
> >>
> >> Seems to me clients of JDK_Canonicalize need to include jdk_util.h, not
> >> the files that implement it. So there is no reason to include it here
> >> and so no need for the comment.
> >
> > Well, it's actually not needed but I think it's good practice that the 
> > declaring
> header is included in the implementation file.
> 
> Yes I was forgetting the importance of ensuring the declaration in the
> header matches the definition. There is a typo in the comment "possibile".
> 
> >> Further, does:
> >>
> >> src/java.base/unix/native/libjava/canonicalize_md.c
> >>
> >> need to include jdk_util.h? I think not.
> >
> > Same as for the windows file - not necessary but good style.
> >
> >> +/* The appropriate location of getPrefixed() is io_util_md.c, but it is
> >> +   also used in a non-OpenJDK context within Oracle. There,
> >> canonicalize_md.c
> >> +   is already pulled in and compiled, so to avoid more complicate 
> >> solutions
> >> +   we keep this method here. */
> >>
> >> I don't like to have comments like this but I guess it is needed here.
> >> Please put the */ on a newline. Also s/complicate/complicates/
> >
> > I did as Dan pointed out.
> 
> :) Yes sorry about that slip.
> 
> Otherwise all looks good. No need for new webrev for the typo.
> 
> Thanks,
> David
> 
> >> src/java.base/windows/native/libjava/io_util_md.c
> >>
> >> should now be unchanged, but you've left in the copyright update.
> >>
> >
> > Right, thanks for the catch.
> >
> >> A second review is still needed for the final version of this.
> >
> > Dan, can I add you as reviewer then?
> >
> > Best regards
> > Christoph
> >

RE: RFR: 8234185: Cleanup usage of canonicalize function between libjava, hotspot and libinstrument

[Langer, Christoph]

Hi David,

I prepared an updated webrev: 
http://cr.openjdk.java.net/~clanger/webrevs/8234185.3/

> src/java.base/windows/native/libjava/canonicalize_md.c
> 
> +// We can't include jdk_util.h here because the file is used in Oracle
> in another context
> +// #include "jdk_util.h"
> 
> Seems to me clients of JDK_Canonicalize need to include jdk_util.h, not
> the files that implement it. So there is no reason to include it here
> and so no need for the comment.

Well, it's actually not needed but I think it's good practice that the 
declaring header is included in the implementation file.

> Further, does:
> 
> src/java.base/unix/native/libjava/canonicalize_md.c
> 
> need to include jdk_util.h? I think not.

Same as for the windows file - not necessary but good style.

> +/* The appropriate location of getPrefixed() is io_util_md.c, but it is
> +   also used in a non-OpenJDK context within Oracle. There,
> canonicalize_md.c
> +   is already pulled in and compiled, so to avoid more complicate solutions
> +   we keep this method here. */
> 
> I don't like to have comments like this but I guess it is needed here.
> Please put the */ on a newline. Also s/complicate/complicates/

I did as Dan pointed out.

> src/java.base/windows/native/libjava/io_util_md.c
> 
> should now be unchanged, but you've left in the copyright update.
> 

Right, thanks for the catch.

> A second review is still needed for the final version of this.

Dan, can I add you as reviewer then?

Best regards
Christoph

RE: RFR: 8234185: Cleanup usage of canonicalize function between libjava, hotspot and libinstrument

[Langer, Christoph]

Hi David,

thanks again for your efforts.

Here is a version that ran successfully through jdk-submit 
(mach5-one-clanger-JDK-8234185-3-20191205-1051-7247172): 
http://cr.openjdk.java.net/~clanger/webrevs/8234185.2/

I avoid the inclusion of jdk_util.h in 
src/java.base/windows/native/libjava/canonicalize_md.c. Do you think this is 
good to go?

Somebody in Oracle could then eventually clean up things by decoupling the 
installer from OpenJDK's canonicalize_md.c. I'd open a bug for this.

Thanks
Christoph

> -Original Message-
> From: David Holmes 
> Sent: Dienstag, 3. Dezember 2019 23:52
> To: Langer, Christoph 
> Cc: core-libs-dev@openjdk.java.net; hotspot-runtime-
> d...@openjdk.java.net; Alan Bateman ; gerard
> ziemski 
> Subject: Re: RFR: 8234185: Cleanup usage of canonicalize function between
> libjava, hotspot and libinstrument
> 
> Hi Christoph,
> 
> On 3/12/2019 7:26 pm, Langer, Christoph wrote:
> > Hi David,
> >
> > thanks for taking care of this.
> >
> > This would be my updated patch that could hopefully be enabled by just
> adding the include directory where "jdk_util.h" is located. It would be really
> great if that'd work. I think it would open up a path to automatically include
> io_util_md.h by including io_util.h.
> >
> > http://cr.openjdk.java.net/~clanger/webrevs/8234185.1/
> 
> I'm afraid I cannot get this to work at our end. I get the following errors:
> 
> t:\workspace\open\src\java.base\share\native\libjava\jdk_util.h(46):
> error C2143: syntax error: missing ')' before '*'
> t:\workspace\open\src\java.base\share\native\libjava\jdk_util.h(46):
> error C2143: syntax error: missing '{' before '*'
> t:\workspace\open\src\java.base\share\native\libjava\jdk_util.h(46):
> warning C4142: 'size_t': benign redefinition of type
> C:\ade\mesos\work_dir\jib-
> ma~1\install\jpg\infra\buildd~1\devkit~1\vs2017~3.0\devkit~1.gz\VC\includ
> e\vcruntime.h(184):
> note: see declaration of 'size_t'
> t:\workspace\open\src\java.base\share\native\libjava\jdk_util.h(46):
> error C2370: 'size_t': redefinition; different storage class
> C:\ade\mesos\work_dir\jib-
> ma~1\install\jpg\infra\buildd~1\devkit~1\vs2017~3.0\devkit~1.gz\VC\includ
> e\vcruntime.h(184):
> note: see declaration of 'size_t'
> t:\workspace\open\src\java.base\share\native\libjava\jdk_util.h(46):
> error C2146: syntax error: missing ';' before identifier 'info_size'
> t:\workspace\open\src\java.base\share\native\libjava\jdk_util.h(46):
> error C2059: syntax error: ')'
> 
> This pertains to the line:
> 
> JDK_GetVersionInfo0(jdk_version_info* info, size_t info_size);
> 
> There is also a second problem in our closed code that will take more
> effort from someone familiar with that code to resolve. I will file an
> issue for our install team to pick up.
> 
> I would ask that this not be pushed for the moment while others figure
> out how best to handle this.
> 
> Thanks,
> David
> -
> 
> 
> > Cheers
> > Christoph
> >
> >
> >> -Original Message-
> >> From: David Holmes 
> >> Sent: Dienstag, 3. Dezember 2019 03:13
> >> To: Langer, Christoph ; Alan Bateman
> >> ; gerard ziemski
> 
> >> Cc: core-libs-dev@openjdk.java.net; hotspot-runtime-
> >> d...@openjdk.java.net
> >> Subject: Re: RFR: 8234185: Cleanup usage of canonicalize function
> between
> >> libjava, hotspot and libinstrument
> >>
> >> Hi Christoph,
> >>
> >> Can you please post your updated patch for review and I will use it to
> >> check the fix for our internal build. Once you have your fix reviewed I
> >> will push both fixes together.
> >>
> >> Thanks,
> >> David
> >>
> >> On 25/11/2019 10:41 pm, David Holmes wrote:
> >>> Hi Christoph,
> >>>
> >>> On 25/11/2019 10:38 pm, Langer, Christoph wrote:
> >>>> Hi David,
> >>>>
> >>>> thanks for your investigation. I'll prepare a fix to move back
> >>>> getPrefixed into canonicalize_md.c. However, could you please still
> >>>> fix your internal build in terms that it would have 'jdk_util.h' in
> >>>> the include path?
> >>>
> >>> That should be simple enough to do.
> >>>
> >>> David
> >>>
> >>>> Thanks
> >>>> Christoph
> >>>>
> >>>>> -Original Message-
> >>>>> From: David Holmes 
> >>>>> Sent: Montag, 25. November 2019 01:02
> >>>>> To: Langer, Christoph ; Alan Bateman
> >>>>&g

RE: RFR(S): 8220348: [ntintel] asserts about copying unalinged array

[Langer, Christoph]

Hi Martin,

I can see that both places already include headers from 
src/java.base/share/native/libjava/. I suggest adding the define in jni_util.h. 
WindowsPreferences.c already includes it.

Best regards
Christoph

From: Doerr, Martin 
Sent: Donnerstag, 5. Dezember 2019 12:00
To: Thomas Stüfe ; Langer, Christoph 

Cc: core-libs-dev@openjdk.java.net; security-dev 
; Lindenmaier, Goetz 
Subject: RE: RFR(S): 8220348: [ntintel] asserts about copying unalinged array

Hi Thomas and Christoph,

thanks for the reviews.

Other code in java.security.jgss also uses these #defined checks:
src/java.security.jgss/share/native/libj2gss/gssapi.h:#if defined (_WIN32) && 
defined (_MSC_VER)

I’d like to have it consistent with that.

@Christoph: I think having ATTRIBUTE_ALIGNED(x) would be nice. It could get 
defined easily for Visual Studio and GCC, but some other compilers may be more 
difficult. Note that this macro is only defined for a selected set of compilers 
in hotspot. If we wanted to add it, where should we define it?

Windows 32 bit seems to be the only platform which is affected by the problem 
that jlongs on stack are not 8 byte aligned.
(AFAIK, GCC uses -malign-double by default on 32 bit which should do the job 
for jlongs, too:
https://gcc.gnu.org/onlinedocs/gcc-4.5.3/gcc/i386-and-x86_002d64-Options.html)

Best regards,
Martin


From: Thomas Stüfe mailto:thomas.stu...@gmail.com>>
Sent: Mittwoch, 4. Dezember 2019 17:56
To: Doerr, Martin mailto:martin.do...@sap.com>>
Cc: core-libs-dev@openjdk.java.net<mailto:core-libs-dev@openjdk.java.net>; 
security-dev 
mailto:security-...@openjdk.java.net>>; 
Lindenmaier, Goetz mailto:goetz.lindenma...@sap.com>>
Subject: Re: RFR(S): 8220348: [ntintel] asserts about copying unalinged array

Hi Martin,

this makes sense. This is the right way to force alignment. I do not like the 
platform code in the shared file but do not think this is a big deal.

+#if defined (_WIN32) && defined (_MSC_VER)

Why do you think we need _MSC_VER too? Is OpenJDK on Windows even buildable 
with anything other than VC++?

Cheers, Thomas

On Mon, Dec 2, 2019 at 4:14 PM Doerr, Martin 
mailto:martin.do...@sap.com>> wrote:
Hi,

I'd like to propose a fix for an old issue on 32 bit Windows (also for an 11u 
backport):
https://bugs.openjdk.java.net/browse/JDK-8220348

Some jdk native methods use jni_SetLongArrayRegion with a stack allocated 
buffer.
jni_SetLongArrayRegion uses Copy::conjoint_jlongs_atomic which requires jlongs 
to be 8 byte aligned (asserted).
However, Windows 32 bit only uses 4 byte alignment for jlong arrays by default.
I found such issues in the following files:
src/java.prefs/windows/native/libprefs/WindowsPreferences.c
src/java.security.jgss/share/native/libj2gss/GSSLibStub.c
I suggest to use __declspec(align(8)) there.

Webrev:
http://cr.openjdk.java.net/~mdoerr/8220348_ntintel_stack_align/webrev.00/
Please review.

I think using 8 byte alignment is not a disadvantage for 64 bit.

I guess there are still people interested in this platform with jdk14. 
Otherwise I could contribute it as 11u only fix.

Is there a better way to force 8 byte alignment for jlongs or jlong arrays on 
stack?
Best regards,
Martin

RE: native debug symbols support on Windows

[Langer, Christoph]

Hi,

thanks for your comments.

The reason why I want to have (at least basic) internal debugging information 
is to have helpful callstacks in hs_err files on crashes.

So, Bob, I don't think the executables can contain debug information, just the 
compiled .obj files. When it comes to linking, the linker either generates a 
pdb file or nothing. That's at least how I read the MSDN documentation (of 
linker options /debug [0] and /pdb [1]).

Maybe an idea to implement "internal" debug symbols for Windows would be to 
copy the pdb files to the release bundle as well. But actually, it's a strange 
interpretation of "internal" in that case...

Thanks
Christoph

[0] 
https://docs.microsoft.com/en-us/cpp/build/reference/debug-generate-debug-info?view=vs-2019
[1] 
https://docs.microsoft.com/en-us/cpp/build/reference/pdb-use-program-database?view=vs-2019

> -Original Message-
> From: Erik Joelsson 
> Sent: Mittwoch, 4. Dezember 2019 15:46
> To: Bob Vandette 
> Cc: Langer, Christoph ; build-
> d...@openjdk.java.net; core-libs-dev@openjdk.java.net; hotspot-dev
> developers 
> Subject: Re: native debug symbols support on Windows
> 
> Hello,
> 
> On 2019-12-04 06:26, Bob Vandette wrote:
> > There seems to be an option that will include debug information in
> generated .obj files.  Assuming this option is supported in the
> > versions of Visual Studio we use, it could be used to implement “internal”
> native debug symbols.
> >
> > /Z7
> 
> We already use this when compiling, but we still link to external pdb
> files. I was not aware of being able to link with the symbol information
> internal.
> 
> While this seems like it could be implemented, my question is, does
> anyone need it? The internal symbols on Linux was something the Linux
> distros wanted as they like to move it out in a uniform manner later. I
> can't really see a need for this on Windows, but I certainly wouldn't
> object if someone else do and wants to implement the support in the
> OpenJDK build.
> 
> /Erik
> 
> > The /Z7 option produces object files that also contain full symbolic
> debugging information for use with the debugger. These object files and the
> built executable can be substantially larger than files that have no debugging
> information. The symbolic debugging information includes the names and
> types of variables, as well as functions and line numbers. No PDB file is
> produced.
> >
> > Bob.
> >
> >
> >> On Dec 4, 2019, at 9:11 AM, Erik Joelsson 
> wrote:
> >>
> >> Correct, with the Microsoft toolchain there is no support for internal. I
> don't know what happens to the build if you try to configure it that way. Feel
> free to come up with a reasonable behavior.
> >>
> >> /Erik
> >>
> >> On 2019-12-04 00:06, Langer, Christoph wrote:
> >>> Hi,
> >>>
> >>> I'm currently looking into native debug symbols support for Windows.
> >>>
> >>> The OpenJDK build system supports these two configure flags --with-
> native-debug-symbols= (among a few other options
> which I don't want to discuss here).
> >>>
> >>> So, the name implies that for "internal", debug symbols should be
> contained in the binaries. And "external" should create separate files that
> contain the debug symbols. However, to my knowledge, Windows would
> always make use of external symbol files, named *.pdb. And there is no way
> to have the debug symbols included in the binaries. Is that correct or am I
> wrong in this assumption?
> >>>
> >>> If it's true, I guess --with-native-debug-symbols=internal would not
> make sense on Windows and should rather be rejected by configure.
> Otherwise, if we were to support --with-native-debug-symbols=internal, the
> build is broken for target "test-image" when it comes to building/copying the
> gtest image.
> >>>
> >>> I'd like to fix either the one way or the other. What do people think?
> >>>
> >>> Thanks for your help
> >>> Christoph
> >>>

RE: RFR(S): 8220348: [ntintel] asserts about copying unalinged array

[Langer, Christoph]

Hi Martin,

thanks for looking into this and coming up with this patch. The test failures 
were quite annoying 

In hotspot, there is coding to define a macro "ATTRIBUTE_ALIGNED(x)". I'd 
rather like to see that we define such a macro in the JDK code as well and use 
it. I think it would make the code more readable. Other than that, +1

Cheers
Christoph

> -Original Message-
> From: core-libs-dev  On Behalf
> Of Doerr, Martin
> Sent: Montag, 2. Dezember 2019 16:13
> To: core-libs-dev@openjdk.java.net; security-dev  d...@openjdk.java.net>
> Cc: Lindenmaier, Goetz 
> Subject: [CAUTION] RFR(S): 8220348: [ntintel] asserts about copying
> unalinged array
> 
> Hi,
> 
> I'd like to propose a fix for an old issue on 32 bit Windows (also for an 11u
> backport):
> https://bugs.openjdk.java.net/browse/JDK-8220348
> 
> Some jdk native methods use jni_SetLongArrayRegion with a stack allocated
> buffer.
> jni_SetLongArrayRegion uses Copy::conjoint_jlongs_atomic which requires
> jlongs to be 8 byte aligned (asserted).
> However, Windows 32 bit only uses 4 byte alignment for jlong arrays by
> default.
> I found such issues in the following files:
> src/java.prefs/windows/native/libprefs/WindowsPreferences.c
> src/java.security.jgss/share/native/libj2gss/GSSLibStub.c
> I suggest to use __declspec(align(8)) there.
> 
> Webrev:
> http://cr.openjdk.java.net/~mdoerr/8220348_ntintel_stack_align/webrev.0
> 0/
> Please review.
> 
> I think using 8 byte alignment is not a disadvantage for 64 bit.
> 
> I guess there are still people interested in this platform with jdk14. 
> Otherwise
> I could contribute it as 11u only fix.
> 
> Is there a better way to force 8 byte alignment for jlongs or jlong arrays on
> stack?
> Best regards,
> Martin

RE: RFR(XS): 8234696: tools/jlink/plugins/VendorInfoPluginsTest.java times out

[Langer, Christoph]

Hi Arno,

+1

I'll push it for you.

Cheers
Christoph

> -Original Message-
> From: core-libs-dev  On Behalf
> Of Mandy Chung
> Sent: Mittwoch, 4. Dezember 2019 00:39
> To: Zeller, Arno 
> Cc: core-libs-dev@openjdk.java.net
> Subject: Re: RFR(XS): 8234696:
> tools/jlink/plugins/VendorInfoPluginsTest.java times out
> 
> Hi Arno,
> 
> On 12/3/19 1:13 AM, Zeller, Arno wrote:
> > Hi all,
> >
> > could someone please review this small improvement for the test
> VendorInfoPluginsTest.java? The change avoids writing a core file and might
> reduce the memory footprint .
> >
> > JBS: https://bugs.openjdk.java.net/browse/JDK-8234696
> > Webrev: http://cr.openjdk.java.net/~azeller/webrevs/8234696.0/
> >
> 
> This looks okay.
> 
> Mandy

native debug symbols support on Windows

[Langer, Christoph]

Hi,

I'm currently looking into native debug symbols support for Windows.

The OpenJDK build system supports these two configure flags 
--with-native-debug-symbols= (among a few other options 
which I don't want to discuss here).

So, the name implies that for "internal", debug symbols should be contained in 
the binaries. And "external" should create separate files that contain the 
debug symbols. However, to my knowledge, Windows would always make use of 
external symbol files, named *.pdb. And there is no way to have the debug 
symbols included in the binaries. Is that correct or am I wrong in this 
assumption?

If it's true, I guess --with-native-debug-symbols=internal would not make sense 
on Windows and should rather be rejected by configure. Otherwise, if we were to 
support --with-native-debug-symbols=internal, the build is broken for target 
"test-image" when it comes to building/copying the gtest image.

I'd like to fix either the one way or the other. What do people think?

Thanks for your help
Christoph

RE: RFR: 8234185: Cleanup usage of canonicalize function between libjava, hotspot and libinstrument

[Langer, Christoph]

Hi David,

thanks for taking care of this.

This would be my updated patch that could hopefully be enabled by just adding 
the include directory where "jdk_util.h" is located. It would be really great 
if that'd work. I think it would open up a path to automatically include 
io_util_md.h by including io_util.h.

http://cr.openjdk.java.net/~clanger/webrevs/8234185.1/

Cheers
Christoph


> -Original Message-
> From: David Holmes 
> Sent: Dienstag, 3. Dezember 2019 03:13
> To: Langer, Christoph ; Alan Bateman
> ; gerard ziemski 
> Cc: core-libs-dev@openjdk.java.net; hotspot-runtime-
> d...@openjdk.java.net
> Subject: Re: RFR: 8234185: Cleanup usage of canonicalize function between
> libjava, hotspot and libinstrument
> 
> Hi Christoph,
> 
> Can you please post your updated patch for review and I will use it to
> check the fix for our internal build. Once you have your fix reviewed I
> will push both fixes together.
> 
> Thanks,
> David
> 
> On 25/11/2019 10:41 pm, David Holmes wrote:
> > Hi Christoph,
> >
> > On 25/11/2019 10:38 pm, Langer, Christoph wrote:
> >> Hi David,
> >>
> >> thanks for your investigation. I'll prepare a fix to move back
> >> getPrefixed into canonicalize_md.c. However, could you please still
> >> fix your internal build in terms that it would have 'jdk_util.h' in
> >> the include path?
> >
> > That should be simple enough to do.
> >
> > David
> >
> >> Thanks
> >> Christoph
> >>
> >>> -Original Message-
> >>> From: David Holmes 
> >>> Sent: Montag, 25. November 2019 01:02
> >>> To: Langer, Christoph ; Alan Bateman
> >>> ; gerard ziemski
> 
> >>> Cc: core-libs-dev@openjdk.java.net; hotspot-runtime-
> >>> d...@openjdk.java.net
> >>> Subject: Re: RFR: 8234185: Cleanup usage of canonicalize function
> >>> between
> >>> libjava, hotspot and libinstrument
> >>>
> >>>
> >>>
> >>> On 25/11/2019 8:45 am, David Holmes wrote:
> >>>> On 25/11/2019 7:49 am, David Holmes wrote:
> >>>>> On 25/11/2019 7:33 am, David Holmes wrote:
> >>>>>> Hi Christoph,
> >>>>>>
> >>>>>> On 23/11/2019 12:04 am, Langer, Christoph wrote:
> >>>>>>> Hi,
> >>>>>>>
> >>>>>>> I'd like to push this change. However, running it through jdk-submit
> >>>>>>> shows reproducible errors:
> >>>>>>>
> >>>>>>> Job: mach5-one-clanger-JDK-8234185-1-20191122-0927-6913189
> >>>>>>> BuildId: 2019-11-22-0926373.christoph.langer.source
> >>>>>>> No failed tests
> >>>>>>> Tasks Summary
> >>>>>>> •    NA: 0
> >>>>>>> •    NOTHING_TO_RUN: 0
> >>>>>>> •    KILLED: 0
> >>>>>>> •    PASSED: 76
> >>>>>>> •    UNABLE_TO_RUN: 0
> >>>>>>> •    EXECUTED_WITH_FAILURE: 1
> >>>>>>> •    FAILED: 0
> >>>>>>> •    HARNESS_ERROR: 0
> >>>>>>> Build
> >>>>>>> 1 Executed with failure
> >>>>>>> o    windows-x64-install-windows-x64-build-19 error while building,
> >>>>>>> return value: 2
> >>>>>>>
> >>>>>>>
> >>>>>>> Job: mach5-one-clanger-JDK-8234185-20191121-2313-6898791
> >>>>>>> BuildId: 2019-11-21-2311357.christoph.langer.source
> >>>>>>> No failed tests
> >>>>>>> Tasks Summary
> >>>>>>> •    NA: 0
> >>>>>>> •    NOTHING_TO_RUN: 0
> >>>>>>> •    KILLED: 0
> >>>>>>> •    PASSED: 76
> >>>>>>> •    UNABLE_TO_RUN: 0
> >>>>>>> •    EXECUTED_WITH_FAILURE: 1
> >>>>>>> •    FAILED: 0
> >>>>>>> •    HARNESS_ERROR: 0
> >>>>>>> Build
> >>>>>>> 1 Executed with failure
> >>>>>>> o    windows-x64-install-windows-x64-build-19 error while building,
> >>>>>>> return value: 2
> >>>>>>>
> >>>>>>>
> >>>>>>> David already had a look and let me know that the following was
> the
> >>>>>>> reason

RE: RFR: 8234821: remove unused functions from libjli

[Langer, Christoph]

Hi Matthias,

overall this looks good to me.

The only change to src/java.base/share/native/libjli/jli_util.c is the 
copyrights, so I guess this should be omitted...

Cheers
Christoph

> -Original Message-
> From: core-libs-dev  On Behalf
> Of Baesken, Matthias
> Sent: Mittwoch, 27. November 2019 09:56
> To: core-libs-dev@openjdk.java.net
> Subject: [CAUTION] RE: RFR: 8234821: remove unused functions from libjli
> 
> Hello,  I missed the  reference from windows only code to JLI_MemRealloc ,
> new webrev :
> 
> http://cr.openjdk.java.net/~mbaesken/webrevs/8234821.1/
> 
> Best regards, Matthias
> 
> 
> 
> Hello, please review  this small change .
> It removed  unneeded functions from libjli (shown  by  link-time-gc) .
> 
> libjli contains a few functions that are shown as unused when applying link-
> time-gc .
> Those functions fall into 2 categories :
> - totally unused/uncalled functions   (can be deleted )
> - functions that are always inlined, but still kept separate when linking
> (making them static often helps to remove the unneeded duplicate )
> 
> 
>  ( Similar approach has been done to libnet , see : 8234629: remove unused
> functions from libnet )
> 
> 
> 
> Thanks, Matthias
> 
> 
> Bug/webrev :
> 
> https://bugs.openjdk.java.net/browse/JDK-8234821
> 
> http://cr.openjdk.java.net/~mbaesken/webrevs/8234821.0/
> 

RE: RFR: 8234185: Cleanup usage of canonicalize function between libjava, hotspot and libinstrument

[Langer, Christoph]

Hi David,

thanks for your investigation. I'll prepare a fix to move back getPrefixed into 
canonicalize_md.c. However, could you please still fix your internal build in 
terms that it would have 'jdk_util.h' in the include path?

Thanks
Christoph

> -Original Message-
> From: David Holmes 
> Sent: Montag, 25. November 2019 01:02
> To: Langer, Christoph ; Alan Bateman
> ; gerard ziemski 
> Cc: core-libs-dev@openjdk.java.net; hotspot-runtime-
> d...@openjdk.java.net
> Subject: Re: RFR: 8234185: Cleanup usage of canonicalize function between
> libjava, hotspot and libinstrument
> 
> 
> 
> On 25/11/2019 8:45 am, David Holmes wrote:
> > On 25/11/2019 7:49 am, David Holmes wrote:
> >> On 25/11/2019 7:33 am, David Holmes wrote:
> >>> Hi Christoph,
> >>>
> >>> On 23/11/2019 12:04 am, Langer, Christoph wrote:
> >>>> Hi,
> >>>>
> >>>> I'd like to push this change. However, running it through jdk-submit
> >>>> shows reproducible errors:
> >>>>
> >>>> Job: mach5-one-clanger-JDK-8234185-1-20191122-0927-6913189
> >>>> BuildId: 2019-11-22-0926373.christoph.langer.source
> >>>> No failed tests
> >>>> Tasks Summary
> >>>> •    NA: 0
> >>>> •    NOTHING_TO_RUN: 0
> >>>> •    KILLED: 0
> >>>> •    PASSED: 76
> >>>> •    UNABLE_TO_RUN: 0
> >>>> •    EXECUTED_WITH_FAILURE: 1
> >>>> •    FAILED: 0
> >>>> •    HARNESS_ERROR: 0
> >>>> Build
> >>>> 1 Executed with failure
> >>>> o    windows-x64-install-windows-x64-build-19 error while building,
> >>>> return value: 2
> >>>>
> >>>>
> >>>> Job: mach5-one-clanger-JDK-8234185-20191121-2313-6898791
> >>>> BuildId: 2019-11-21-2311357.christoph.langer.source
> >>>> No failed tests
> >>>> Tasks Summary
> >>>> •    NA: 0
> >>>> •    NOTHING_TO_RUN: 0
> >>>> •    KILLED: 0
> >>>> •    PASSED: 76
> >>>> •    UNABLE_TO_RUN: 0
> >>>> •    EXECUTED_WITH_FAILURE: 1
> >>>> •    FAILED: 0
> >>>> •    HARNESS_ERROR: 0
> >>>> Build
> >>>> 1 Executed with failure
> >>>> o    windows-x64-install-windows-x64-build-19 error while building,
> >>>> return value: 2
> >>>>
> >>>>
> >>>> David already had a look and let me know that the following was the
> >>>> reason:
> >>>>
> >>>>
> t:/workspace/open/src/java.base/windows/native/libjava/canonicalize_md.
> c(41):
> >>>> fatal error C1083: Cannot open include file: 'jdk_util.h': No such
> >>>> file or directory
> >>>>
> >>>> This is not explainable to me as I see this running through my local
> >>>> build and our nightly builds without problems. I also can't explain
> >>>> jdk_util.h can't be opened at this place - it should be there and
> >>>> part of the include directories...
> >>>>
> >>>> I'd appreciate any help...
> >>>
> >>> I just dug a little deeper and this is failing in part of our closed
> >>> build for the install repo. There is a library there that is using
> >>> canonicalize_md.c directly - i.e. it adds that file to its source
> >>> files list. The build instructions don't include that directory on
> >>> the include directory list - hence the failure. But it will also fail
> >>> due to the name change you made.
> >>
> >> Actually it appears that the other source code doesn't actually refer
> >> to the canonicalize function at all, so a simple fix may be possible
> >> at the build level on our side. I'm testing that now.
> >
> > It isn't the canonicalize function that is used, it is getPrefixed,
> > which has now been moved to the io_util_md.c file. So a fix will be a
> > bit more involved.
> 
> I tried adding io_util_md.c to the library source list instead of
> canonicalize_md.c but that just caused a slew of other compilation
> failures, so I don't see any quick fix for us here.
> 
> David
> 
> >
> > David
> >
> >>
> >> David
> >> -
> >>
> >>> Someone will need to work with you to make the necessary changes to
> >>> our code.
> >>>
> >>> David
> >>>
> >&

RE: RFR: 8234185: Cleanup usage of canonicalize function between libjava, hotspot and libinstrument

[Langer, Christoph]

Hi,

I'd like to push this change. However, running it through jdk-submit shows 
reproducible errors:

Job: mach5-one-clanger-JDK-8234185-1-20191122-0927-6913189
BuildId: 2019-11-22-0926373.christoph.langer.source
No failed tests
Tasks Summary
•   NA: 0
•   NOTHING_TO_RUN: 0
•   KILLED: 0
•   PASSED: 76
•   UNABLE_TO_RUN: 0
•   EXECUTED_WITH_FAILURE: 1
•   FAILED: 0
•   HARNESS_ERROR: 0
Build
1 Executed with failure
o   windows-x64-install-windows-x64-build-19 error while building, return 
value: 2


Job: mach5-one-clanger-JDK-8234185-20191121-2313-6898791
BuildId: 2019-11-21-2311357.christoph.langer.source
No failed tests
Tasks Summary
•   NA: 0
•   NOTHING_TO_RUN: 0
•   KILLED: 0
•   PASSED: 76
•   UNABLE_TO_RUN: 0
•   EXECUTED_WITH_FAILURE: 1
•   FAILED: 0
•   HARNESS_ERROR: 0
Build
1 Executed with failure
o   windows-x64-install-windows-x64-build-19 error while building, return 
value: 2


David already had a look and let me know that the following was the reason:

t:/workspace/open/src/java.base/windows/native/libjava/canonicalize_md.c(41): 
fatal error C1083: Cannot open include file: 'jdk_util.h': No such file or 
directory

This is not explainable to me as I see this running through my local build and 
our nightly builds without problems. I also can't explain jdk_util.h can't be 
opened at this place - it should be there and part of the include directories...

I'd appreciate any help...

Thanks
Christoph


> -Original Message-
> From: Langer, Christoph
> Sent: Donnerstag, 21. November 2019 14:19
> To: Alan Bateman ; core-libs-
> d...@openjdk.java.net; hotspot-runtime-...@openjdk.java.net
> Subject: RE: RFR: 8234185: Cleanup usage of canonicalize function between
> libjava, hotspot and libinstrument
> 
> Hi Alan,
> 
> thanks for the review. I'll push it then after running through jdk-submit.
> 
> /Christoph
> 
> > -Original Message-
> > From: Alan Bateman 
> > Sent: Donnerstag, 21. November 2019 09:51
> > To: Langer, Christoph ; core-libs-
> > d...@openjdk.java.net; hotspot-runtime-...@openjdk.java.net
> > Subject: Re: RFR: 8234185: Cleanup usage of canonicalize function between
> > libjava, hotspot and libinstrument
> >
> > On 14/11/2019 15:37, Langer, Christoph wrote:
> > > Hi,
> > >
> > > please review this cleanup change regarding function "canonicalize" of
> > libjava.
> > >
> > > Bug: https://bugs.openjdk.java.net/browse/JDK-8234185
> > > Webrev: http://cr.openjdk.java.net/~clanger/webrevs/8234185.0/
> > >
> > >
> > > The goal is to cleanup how this function is defined and used. One thing 
> > > is,
> > that there was an unnecessary wrapper function "Canonicalize" in jni_util.c.
> > It wrapped the call to "canonicalize". We can get rid of this wrapper.
> > Unfortunately, it is not possible to just export "canonicalize" since this 
> > will
> > conflict with a method signature from the math library, at least on modern
> > Linuxes. So I decided to call the method JDK_Canonicalize and will correctly
> > define it in jdk_util.h which can be included everywhere.
> > >
> > I think this change is okay. My main concern when initially seeing this
> > go by was that it would leak the \\?\ or \\?\UNC\ prefix into the
> > canonical File when it wasn't there previously, this would of course
> > have several implications. But I think you have it right and this is, as
> > you position, just refactoring/cleanup.
> >
> > -Alan

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

[Langer, Christoph]

Hi Lance,

thanks again. Makes sense to keep comments consistent. So this is what I’m 
going to push tomorrow: http://cr.openjdk.java.net/~clanger/webrevs/8234089.5/

Cheers
Christoph

From: Lance Andersen 
Sent: Mittwoch, 20. November 2019 19:02
Cc: Langer, Christoph ; nio-dev 
; core-libs-dev@openjdk.java.net
Subject: Re: RFR: 8234089: (zipfs) Remove classes JarFileSystemProvider and 
JarFileSystem

Hi Christoph,

Again, thank you for your efforts here.

Overall I think your latest changes look fine.  I would like to suggest that 
for the methods that you added for MR support, that we make sure the 1st 
character in the comment is capitalized prior to your push of the changes.  I 
know this was not that way prior, but I think it helps with consistency.

BTW,  I am not sure that JarFileSystemProvider was actually ever 
used(JarFileSystem was though) as when I look at the installed providers on my 
Mac, I see:

[sun.nio.fs.MacOSXFileSystemProvider@5b2133b1, 
jdk.nio.zipfs.ZipFileSystemProvider@72ea2f77<mailto:jdk.nio.zipfs.ZipFileSystemProvider@72ea2f77>,
 
jdk.internal.jrtfs.JrtFileSystemProvider@33c7353a<mailto:jdk.internal.jrtfs.JrtFileSystemProvider@33c7353a>]


Best
Lance
On Nov 20, 2019, at 10:02 AM, Alan Bateman 
mailto:alan.bate...@oracle.com>> wrote:



On 20/11/2019 13:39, Langer, Christoph wrote:
Hi Alan,

makes sense. I’ve updated the patch: 
http://cr.openjdk.java.net/~clanger/webrevs/8234089.4/

The updated test looks okay.

-Alan

[cid:image001.gif@01D5A0C8.666731C0]<http://oracle.com/us/design/oracle-email-sig-198324.gif>

<http://oracle.com/us/design/oracle-email-sig-198324.gif>Lance Andersen| 
Principal Member of Technical Staff | +1.781.442.2037
Oracle Java Engineering
1 Network Drive
Burlington, MA 01803
lance.ander...@oracle.com<mailto:lance.ander...@oracle.com>

RE: RFR [XS] : 8234339: replace JLI_StrTok in java_md_solinux.c

[Langer, Christoph]

+1

> -Original Message-
> From: core-libs-dev  On Behalf
> Of Roger Riggs
> Sent: Mittwoch, 20. November 2019 16:08
> To: core-libs-dev@openjdk.java.net
> Subject: Re: RFR [XS] : 8234339: replace JLI_StrTok in java_md_solinux.c
> 
> Hi Matthias,
> 
> Good to see the switch to strtok_r.
> Looks fine.
> 
> Thanks, Roger
> 
> 
> On 11/19/19 4:23 AM, Baesken, Matthias wrote:
> > Hello, please review this small change .
> >
> > JLI_StrTok is only used in one function, so the define can be replaced,
> probably we should use the "safer" strtok_r (the MT safety might not be a
> big deal in the launcher however).
> >
> >
> > Bug/webrev :
> >
> > https://bugs.openjdk.java.net/browse/JDK-8234339
> >
> > http://cr.openjdk.java.net/~mbaesken/webrevs/8234339.0/
> >
> >
> > Thanks, Matthias

RE: RFR: 8234185: Cleanup usage of canonicalize function between libjava, hotspot and libinstrument

[Langer, Christoph]

Hi Alan,

thanks for the review. I'll push it then after running through jdk-submit.

/Christoph

> -Original Message-
> From: Alan Bateman 
> Sent: Donnerstag, 21. November 2019 09:51
> To: Langer, Christoph ; core-libs-
> d...@openjdk.java.net; hotspot-runtime-...@openjdk.java.net
> Subject: Re: RFR: 8234185: Cleanup usage of canonicalize function between
> libjava, hotspot and libinstrument
> 
> On 14/11/2019 15:37, Langer, Christoph wrote:
> > Hi,
> >
> > please review this cleanup change regarding function "canonicalize" of
> libjava.
> >
> > Bug: https://bugs.openjdk.java.net/browse/JDK-8234185
> > Webrev: http://cr.openjdk.java.net/~clanger/webrevs/8234185.0/
> >
> >
> > The goal is to cleanup how this function is defined and used. One thing is,
> that there was an unnecessary wrapper function "Canonicalize" in jni_util.c.
> It wrapped the call to "canonicalize". We can get rid of this wrapper.
> Unfortunately, it is not possible to just export "canonicalize" since this 
> will
> conflict with a method signature from the math library, at least on modern
> Linuxes. So I decided to call the method JDK_Canonicalize and will correctly
> define it in jdk_util.h which can be included everywhere.
> >
> I think this change is okay. My main concern when initially seeing this
> go by was that it would leak the \\?\ or \\?\UNC\ prefix into the
> canonical File when it wasn't there previously, this would of course
> have several implications. But I think you have it right and this is, as
> you position, just refactoring/cleanup.
> 
> -Alan

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

[Langer, Christoph]

Hi Alan,

makes sense. I’ve updated the patch: 
http://cr.openjdk.java.net/~clanger/webrevs/8234089.4/

Best regards
Christoph

From: Alan Bateman 
Sent: Mittwoch, 20. November 2019 09:33
To: Langer, Christoph ; Lance Andersen 

Cc: nio-dev ; core-libs-dev@openjdk.java.net
Subject: Re: RFR: 8234089: (zipfs) Remove classes JarFileSystemProvider and 
JarFileSystem


On 20/11/2019 08:15, Langer, Christoph wrote:
Hi Lance,

I’ve taken care of ModulesInCustomFileSystem then, too.

Updated webrev in place…

If the ModulesInCustomFileSystem test really needs to be changed then the 
private method that has been renamed to testZipFileSystem shoud have its 
parameter changed too. The parameter can be renamed to zipfile (or similar) as 
it's a file path to a zip file. While in the area, it should be changed to use 
the 1-arg newFileSystem method, no need to use the system class loader.

-Alan

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

[Langer, Christoph]

Hi Lance,

I’ve taken care of ModulesInCustomFileSystem then, too.

Updated webrev in place…

Cheers
Christoph

From: Lance Andersen 
Sent: Dienstag, 19. November 2019 23:42
To: Langer, Christoph 
Cc: core-libs-dev@openjdk.java.net; nio-dev 
Subject: Re: RFR: 8234089: (zipfs) Remove classes JarFileSystemProvider and 
JarFileSystem

Hi Christoph,

Thank you for the follow up.
On Nov 19, 2019, at 5:37 PM, Langer, Christoph 
mailto:christoph.lan...@sap.com>> wrote:

ModulesInCustomFileSystem

At line 71:
—
/**
 * Test exploded modules in a JAR file system.
 */
———

I will look at the rest of the changes tomorrow

Best
Lance

[cid:image001.gif@01D59F83.08ADFCD0]<http://oracle.com/us/design/oracle-email-sig-198324.gif>

<http://oracle.com/us/design/oracle-email-sig-198324.gif>Lance Andersen| 
Principal Member of Technical Staff | +1.781.442.2037
Oracle Java Engineering
1 Network Drive
Burlington, MA 01803
lance.ander...@oracle.com<mailto:lance.ander...@oracle.com>

RE: RFR: 8234185: Cleanup usage of canonicalize function between libjava, hotspot and libinstrument

[Langer, Christoph]

Hi Serguei,

thanks for the review.

The patch successfully ran through our nightly test system which covers all 
these tests on several platforms.

Best regards
Christoph

> -Original Message-
> From: serguei.spit...@oracle.com 
> Sent: Mittwoch, 20. November 2019 03:21
> To: Langer, Christoph ; core-libs-
> d...@openjdk.java.net; hotspot-runtime-...@openjdk.java.net; gerard
> ziemski 
> Subject: Re: RFR: 8234185: Cleanup usage of canonicalize function between
> libjava, hotspot and libinstrument
> 
> Hi Christoph,
> 
> The fix looks good to me.
> I'd recommend to run the jdk_instrument and vmTestbase_nsk_jvmti tests.
> Also, it can be safe to run:
>    open/test/hotspot/jtreg/serviceability/jvmti
>    open/test/hotspot/jtreg/runtime/cds/appcds
>    open/test/hotspot/jtreg/runtime/BootClassAppendProp
> 
> Thanks,
> Serguei
> 
> On 11/14/19 07:37, Langer, Christoph wrote:
> > Hi,
> >
> > please review this cleanup change regarding function "canonicalize" of
> libjava.
> >
> > Bug: https://bugs.openjdk.java.net/browse/JDK-8234185
> > Webrev: http://cr.openjdk.java.net/~clanger/webrevs/8234185.0/
> >
> >
> > The goal is to cleanup how this function is defined and used. One thing is,
> that there was an unnecessary wrapper function "Canonicalize" in jni_util.c.
> It wrapped the call to "canonicalize". We can get rid of this wrapper.
> Unfortunately, it is not possible to just export "canonicalize" since this 
> will
> conflict with a method signature from the math library, at least on modern
> Linuxes. So I decided to call the method JDK_Canonicalize and will correctly
> define it in jdk_util.h which can be included everywhere.
> >
> >
> >
> > Hotspot's classloader.cpp will dynamically resolve this method, so I add a
> local declaration of the function pointer in there.
> >
> >
> >
> > This change shall be predecessor of JDK-8223261, where a review was
> already started here: https://mail.openjdk.java.net/pipermail/core-libs-
> dev/2019-November/063398.html
> >
> > Thanks
> > Christoph
> >

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

[Langer, Christoph]

Hi Lance,

> If you look at MultiReleaseJarTest.java, it explicitly references JAR FS in a
> comment. Same for JFSTester.java in the @Summary.  These should
> definitely be updated.  There are comments
> in ModulesInCustomFileSystem.java as well.

Ok, agreed for MultiReleaseJarTest and JFSTester. I fixed the comments in 
there. In ModulesInCustomFileSystem I can't see comments that explicitly refer 
to JarFileSystem.

> I would suggesting moving the code added to the constructor for checking
> the releaseVersion/multi-release properties to a method and grouping it
> with the other methods added for supporting MR jars around line 1396.
> (keeps it easier to follow and maintain)
> 
> Done that.
> 
> Thank you.  I do think however we need to change the name of the method
> to perhaps “checkReleaseVersionProperty” as the current name
> “initializeMultiReleaseJar”  does not seem quite right to me.

Ok, I changed it to " initializeReleaseVersion". I didn't follow your 
suggestion because the method not only checks the release version property but 
also triggers some initialization if a version property is found and it's an 
mr-jar.

> I would also update the comment for the method to something like:
> 
> 
> Checks to see if the Zip File System property  “releaseVersion” has been
> specified.  If it has not been specified then check for the existence of the
> “multi-release” property.   If set and the file represents a multi-release 
> JAR,
> determine the runtime version to use.
> 

I updated the comment, trying to incorporate your suggestion. How do you like 
it?

> Could you also add a comment above the isMultiReleaseJar method to for
> clarity going forward (I know there was not one there before)
> 
> Done, too.
> 
> Thank you.
> 
> I think we can tweak the comment slightly to something like
> 
> ———
> Returns true if the Manifest main attribute “Multi-Release” is set to true;
> false otherwise
> 

Done.

> I might change the name of the lookup field in the method makeParentDirs
> with the addition of the Function lookup on line 126.
> 
> I chose to change the name of the global field in line 126 to be named
> mrlookup. Also updated the comment.
> 
> I am not sure “mrlookup” is the best name as this field is used with or
> without a multi-release jar.  Perhaps “IndexNodeNameLookup” or
> “entryLookup"

Ok, entryLookup seems the best fit to me. Changed that.

http://cr.openjdk.java.net/~clanger/webrevs/8234089.3/

Thanks again for reviewing.

Cheers
Christoph

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

[Langer, Christoph]

Hi Lance,

I’m actually not so sure about this. While my cleanup change would remove the 
implementation detail of zipfs to use a class named “JarFileSystem” for 
multi-release jar peculiarities, I think a user of a FileSystem object upon a 
jar file is not wrong if he names the variable like jarFileSystem or the like. 
So I don’t see that such cleanup is really worth the while. Would you be ok 
with that?

I’ll get back to you soon with an updated webrev regarding the other changes 
you suggested.

Best regards
Christoph

From: Lance Andersen 
Sent: Dienstag, 19. November 2019 19:57
To: Langer, Christoph 
Cc: core-libs-dev@openjdk.java.net; nio-dev 
Subject: Re: RFR: 8234089: (zipfs) Remove classes JarFileSystemProvider and 
JarFileSystem

Hi Christoph,

Something else that should probably be done as part of this proposed change is  
to clean up tests such as NodeCostDumpUtil.java and 
ModulesInCustomFileSystem.java and a few others as they have methods/fields 
etc... that make reference to the  Jar File System.  While it does not impact 
the tests as they are currently written, I think it would make sense to do this 
for clarity with the change you are making.

Best
Lance
On Nov 18, 2019, at 12:53 PM, Lance Andersen 
mailto:lance.ander...@oracle.com>> wrote:

Hi Christoph,

Sorry for the late reply but I was out of the office Friday
On Nov 15, 2019, at 3:40 AM, Langer, Christoph 
mailto:christoph.lan...@sap.com>> wrote:

Hi Lance,

thanks for reviewing. I've addressed your points:


I would suggesting moving the code added to the constructor for checking
the releaseVersion/multi-release properties to a method and grouping it
with the other methods added for supporting MR jars around line 1396.
(keeps it easier to follow and maintain)

Done that.

Thank you.  I do think however we need to change the name of the method to 
perhaps “checkReleaseVersionProperty” as the current name 
“initializeMultiReleaseJar”  does not seem quite right to me.

I would also update the comment for the method to something like:


Checks to see if the Zip File System property  “releaseVersion” has been 
specified.  If it has not been specified then check for the existence of the 
“multi-release” property.   If set and the file represents a multi-release JAR, 
determine the runtime version to use.




Could you also add a comment above the isMultiReleaseJar method to for
clarity going forward (I know there was not one there before)

Done, too.

Thank you.

I think we can tweak the comment slightly to something like

———
Returns true if the Manifest main attribute “Multi-Release” is set to true; 
false otherwise




I might change the name of the lookup field in the method makeParentDirs
with the addition of the Function lookup on line 126.

I chose to change the name of the global field in line 126 to be named 
mrlookup. Also updated the comment.

I am not sure “mrlookup” is the best name as this field is used with or without 
a multi-release jar.  Perhaps “IndexNodeNameLookup” or
“entryLookup"


This is the new webrev: http://cr.openjdk.java.net/~clanger/webrevs/8234089.2/

Cheers
Christoph

Thank you again!

Best
Lance



<http://oracle.com/us/design/oracle-email-sig-198324.gif>

<http://oracle.com/us/design/oracle-email-sig-198324.gif>Lance Andersen| 
Principal Member of Technical Staff | +1.781.442.2037
Oracle Java Engineering
1 Network Drive
Burlington, MA 01803
lance.ander...@oracle.com<mailto:lance.ander...@oracle.com>

[cid:image001.gif@01D59F25.2712D690]<http://oracle.com/us/design/oracle-email-sig-198324.gif>

<http://oracle.com/us/design/oracle-email-sig-198324.gif>Lance Andersen| 
Principal Member of Technical Staff | +1.781.442.2037
Oracle Java Engineering
1 Network Drive
Burlington, MA 01803
lance.ander...@oracle.com<mailto:lance.ander...@oracle.com>

RE: RFR: 8234185: Cleanup usage of canonicalize function between libjava, hotspot and libinstrument

[Langer, Christoph]

Hi Thorsten,

I saw your other mail already but didn't find time to reply.

I'm actually not convinced that it is a good idea to add ERROR_NO_MORE_FILES to 
lastErrorReportable. The error codes listed there are conditions on which 
canonicalization of a path is stopped but the result is deemed correct. E.g. if 
the path only exists up to a certain directory, one can assume the rest of the 
path is canonic. Or if there are conditions like network errors or access 
denied, then further canonicalization isn't possible, too.

However, your case, the sporadic ERROR_NO_MORE_FILES, needs to be understood 
first. I rather think if this happens, there's a real condition for an 
IOException. It should definitely be analyzed and understood what the reason is 
for ERROR_NO_MORE_FILES. Are you aware of other reports of this issue? Was this 
already analyzed by some Windows experts, e.g. Microsoft support?

Best regards
Christoph


> -Original Message-
> From: Thorsten Schöning 
> Sent: Montag, 18. November 2019 14:09
> To: core-libs-dev@openjdk.java.net
> Cc: Langer, Christoph ; hotspot-runtime-
> d...@openjdk.java.net; gerard ziemski 
> Subject: Re: RFR: 8234185: Cleanup usage of canonicalize function between
> libjava, hotspot and libinstrument
> 
> Guten Tag Langer, Christoph,
> am Donnerstag, 14. November 2019 um 16:37 schrieben Sie:
> 
> > please review this cleanup change regarding function "canonicalize" of
> libjava.
> [...]
> > The goal is to cleanup how this function is defined and used.[...]
> 
> If you are already changing "lastErrorReportable" for Windows, how
> about adding ERROR_NO_MORE_FILES there as well to not run into
> unnecessary exceptions under some circumstances?
> 
> https://mail.openjdk.java.net/pipermail/core-libs-dev/2019-
> November/063437.html
> https://stackoverflow.com/questions/58825588/does-java-need-to-
> support-error-no-more-files-when-canonicalizing-paths-on-windo
> https://stackoverflow.com/questions/58825963/when-does-findfirstfilew-
> set-last-error-to-be-error-no-more-files-instead-of-err?noredirect=1=1
> 
> Mit freundlichen Grüßen,
> 
> Thorsten Schöning
> 
> --
> Thorsten Schöning   E-Mail: thorsten.schoen...@am-soft.de
> AM-SoFT IT-Systeme  http://www.AM-SoFT.de/
> 
> Telefon...05151-  9468- 55
> Fax...05151-  9468- 88
> Mobil..0178-8 9468- 04
> 
> AM-SoFT GmbH IT-Systeme, Brandenburger Str. 7c, 31789 Hameln
> AG Hannover HRB 207 694 - Geschäftsführer: Andreas Muchow

RE: RFR: 8234185: Cleanup usage of canonicalize function between libjava, hotspot and libinstrument

[Langer, Christoph]

> I plan to review this change. We also need to figure out how to remove
> the dependency on this function from the JPLIS agent as that should not
> be there.

Agree. I'd hope, however, that this can be done with a different change (unless 
you have an idea for a very simple, straightforward way that could fit under 
the umbrella of JDK- 8234185).

/Christoph

RE: RFR: 8234185: Cleanup usage of canonicalize function between libjava, hotspot and libinstrument

[Langer, Christoph]

Hi David,

> This all seems fine to me. One clarification:

Thanks for the review.

> 
> - /* The appropriate location of getPrefixed() should be io_util_md.c, but
> -java.lang.instrument package has hardwired canonicalize_md.c into their
> -dll, to avoid complicate solution such as including io_util_md.c into
> -that package, as a workaround we put this method here.
> -  */
> 
> I assume this hardwired usage was removed some time ago?

AFAICS, yes. libinstrument builds/links against libjava. I cannot find any 
duplicates of canonicalize* in there.

Any other reviews (e.g. Gerard?)

Thanks & Best regards
Christoph

> 
> Thanks,
> David
> 
> On 15/11/2019 1:37 am, Langer, Christoph wrote:
> > Hi,
> >
> > please review this cleanup change regarding function "canonicalize" of
> libjava.
> >
> > Bug: https://bugs.openjdk.java.net/browse/JDK-8234185
> > Webrev: http://cr.openjdk.java.net/~clanger/webrevs/8234185.0/
> >
> >
> > The goal is to cleanup how this function is defined and used. One thing is,
> that there was an unnecessary wrapper function "Canonicalize" in jni_util.c.
> It wrapped the call to "canonicalize". We can get rid of this wrapper.
> Unfortunately, it is not possible to just export "canonicalize" since this 
> will
> conflict with a method signature from the math library, at least on modern
> Linuxes. So I decided to call the method JDK_Canonicalize and will correctly
> define it in jdk_util.h which can be included everywhere.
> >
> >
> >
> > Hotspot's classloader.cpp will dynamically resolve this method, so I add a
> local declaration of the function pointer in there.
> >
> >
> >
> > This change shall be predecessor of JDK-8223261, where a review was
> already started here: https://mail.openjdk.java.net/pipermail/core-libs-
> dev/2019-November/063398.html
> >
> > Thanks
> > Christoph
> >

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

[Langer, Christoph]

Hi Volker,

> > Please remove the "import java.nio.file.Files;" statement before pushing.
> >
> 
> It's needed for "Files.deleteIfExists(zipFile)" in the finally block..

Oops, you're right. It popped up in my IDE because I tested what happened when 
the file remained in the directory and therefore I commented that block out.

So, go for it 

Cheers
Christoph

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

[Langer, Christoph]

Hi Volker,

Looks awesome now 

Please remove the "import java.nio.file.Files;" statement before pushing.

Cheers
Christoph


> -Original Message-
> From: Volker Simonis 
> Sent: Freitag, 15. November 2019 12:30
> To: Langer, Christoph ; Volker Simonis
> 
> Cc: core-libs-dev@openjdk.java.net; Lance Andersen
> 
> Subject: Re: RFR(XS): 8234011: (zipfs) Memory leak in
> ZipFileSystem.releaseDeflater()
> 
> On 14.11.19 18:24, Langer, Christoph wrote:
> > Hi Volker,
> >
> > funny, I didn’t get aware of your mails until I now recognized that my
> > mail client moved your mail into the “Amazon-advertisement-spam” folder
> > of my mailbox.  I have to check and modify my filter rules…
> >
> > Ok, let’s continue the little bike-shed about the test then.
> >
> > First, thanks for making the adaptions. The test looks better already. I
> > still have a few points:
> >
> > 1. The imports of java.io.File and java.util.HashMap can be removed now.
> >
> 
> Done.
> 
> > 2. The two try statements in lines 54 and 55 look a bit awkward. I guess
> > you could just use the one try-with-resource from line 55 and put the
> > cleanup in its finally block.
> >
> 
> Done.
> 
> > 3. Maybe you also want to attempt a Files.deleteIfExists(zipFile);
> > before opening the Zip file system? Otherwise there is a remote
> > possibility that ReleaseDeflaterTest.zip already exists and the test
> > will fail because of this.
> >
> 
> That doesn't harm. The file will just be reused.
> 
> > 4. I’m also not a fan of the SkippedException here. I for myself would
> > probably not get aware of a skip. And if somebody changes the
> > implementation of ZipFileSystem, why shouldn’t he/she/… immediately
> > recognize this and adapt the test in the same change?
> >
> 
> OK, changed it to a RuntimeException now.
> 
> Here's the new webrev:
> 
> http://cr.openjdk.java.net/~simonis/webrevs/2019/8234011.v3
> 
> > Best regards
> >
> > Christoph
> >
> > *From:* Lance Andersen 
> > *Sent:* Donnerstag, 14. November 2019 17:38
> > *To:* Volker Simonis 
> > *Cc:* Langer, Christoph ; Simonis, Volker
> > ; core-libs-dev@openjdk.java.net
> > *Subject:* Re: RFR(XS): 8234011: (zipfs) Memory leak in
> > ZipFileSystem.releaseDeflater()
> >
> > On Nov 14, 2019, at 11:27 AM, Volker Simonis  > <mailto:simon...@amazon.com>> wrote:
> >
> > On 14.11.19 16:27, Lance Andersen wrote:
> >
> > Hi Volker,
> >
> > On Nov 14, 2019, at 8:53 AM, Volker Simonis
> >  > <mailto:simon...@amazon.com><mailto:simon...@amazon.com>>
> wrote:
> >
> > On 13.11.19 18:54, Lance Andersen wrote:
> >
> > 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.
> >
> >
> > Done.
> >
> >
> > If possible, I would use TestNG as that is consistent
> > with the vast majority of the tests.
> >
> >
> > I don't particularly like to nest one test harness within
> > another one :)
> > But seriously, I think using JUnit or TestNG makes sens if
> > you write a whole test suit which uses the features of such
> > a test harness (e.g. tear up, tear down, etc.)
> >
> > Well you could use @AfterMethod or @AfterClass to clean up files
> > etc… ;-)
> >
> > But for small trivial tests I really prefer to use plain
> > JTreg. This also has the big advantage that is becomes
> > trivial to run such a test stand-alone which may come handy
> > if you have to debug it.
> >
> > Easy enough to add a main method to call your test method (there
> > are some testng tests I have seen in the workspace that do that)
> >
> >
> > So if you don't insist, I'll prefer to leave the test as it is.
> >
> > While I would prefer it for new tests, I am not insisting you
> > need to change the test….. ;-)
> >
> >
> > OK, thanks. I might consider using it in the future :)
> >
> >
> >
> >
> > I also believe the rest of the comments below are worth
> > addressing.
> >
> >
> >  

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

[Langer, Christoph]

Hi Lance,

thanks for reviewing. I've addressed your points:

> I would suggesting moving the code added to the constructor for checking
> the releaseVersion/multi-release properties to a method and grouping it
> with the other methods added for supporting MR jars around line 1396.
> (keeps it easier to follow and maintain)

Done that.

> Could you also add a comment above the isMultiReleaseJar method to for
> clarity going forward (I know there was not one there before)

Done, too.

> I might change the name of the lookup field in the method makeParentDirs
> with the addition of the Function lookup on line 126.

I chose to change the name of the global field in line 126 to be named 
mrlookup. Also updated the comment.

This is the new webrev: http://cr.openjdk.java.net/~clanger/webrevs/8234089.2/

Cheers
Christoph

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

[Langer, Christoph]

Hi Volker,

funny, I didn’t get aware of your mails until I now recognized that my mail 
client moved your mail into the “Amazon-advertisement-spam” folder of my 
mailbox.  I have to check and modify my filter rules…

Ok, let’s continue the little bike-shed about the test then.

First, thanks for making the adaptions. The test looks better already. I still 
have a few points:

1. The imports of java.io.File and java.util.HashMap can be removed now.
2. The two try statements in lines 54 and 55 look a bit awkward. I guess you 
could just use the one try-with-resource from line 55 and put the cleanup in 
its finally block.
3. Maybe you also want to attempt a Files.deleteIfExists(zipFile); before 
opening the Zip file system? Otherwise there is a remote possibility that 
ReleaseDeflaterTest.zip already exists and the test will fail because of this.
4. I’m also not a fan of the SkippedException here. I for myself would probably 
not get aware of a skip. And if somebody changes the implementation of 
ZipFileSystem, why shouldn’t he/she/… immediately recognize this and adapt the 
test in the same change?

Best regards
Christoph




From: Lance Andersen 
Sent: Donnerstag, 14. November 2019 17:38
To: Volker Simonis 
Cc: Langer, Christoph ; Simonis, Volker 
; core-libs-dev@openjdk.java.net
Subject: Re: RFR(XS): 8234011: (zipfs) Memory leak in 
ZipFileSystem.releaseDeflater()


On Nov 14, 2019, at 11:27 AM, Volker Simonis 
mailto:simon...@amazon.com>> wrote:

On 14.11.19 16:27, Lance Andersen wrote:

Hi Volker,

On Nov 14, 2019, at 8:53 AM, Volker Simonis 
mailto:simon...@amazon.com> <mailto:simon...@amazon.com>> 
wrote:

On 13.11.19 18:54, Lance Andersen wrote:

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.

Done.


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

I don't particularly like to nest one test harness within another one :)
But seriously, I think using JUnit or TestNG makes sens if you write a whole 
test suit which uses the features of such a test harness (e.g. tear up, tear 
down, etc.)
Well you could use @AfterMethod or @AfterClass to clean up files etc… ;-)

But for small trivial tests I really prefer to use plain JTreg. This also has 
the big advantage that is becomes trivial to run such a test stand-alone which 
may come handy if you have to debug it.
Easy enough to add a main method to call your test method (there are some 
testng tests I have seen in the workspace that do that)


So if you don't insist, I'll prefer to leave the test as it is.
While I would prefer it for new tests, I am not insisting you need to change 
the test….. ;-)

OK, thanks. I might consider using it in the future :)




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

Besides that, I've addressed all the other points mentioned by Christoph. 
Please find the new webrev at:

http://cr.openjdk.java.net/~simonis/webrevs/2019/8234011.v1/
line 55 you can remove the creation of the HashMap

Good catch! Removed.


line 73, do you really need the equals check seeing you do not do anything?

Removed. It was a leftover of local testing.


I am not sure throwing a SkippedException is correct,  I would probably throw a 
RuntimeException

As I wrote in the answer to Christoph, this is a relatively new feature of 
JTreg [1] which I think was introduced for exactly such kind of situations 
where a tests detects at runtime only, that for some reasons he can't test the 
issue he was supposed to test. More and more tests are adapting it now [2]. The 
SkippedException will be handled specially by JTreg and let the test pass, but 
with status "Passed.Skipped" (plus exception message) instead of just "Passed" 
(plis "Execution successful").

Here's the next webrev:

http://cr.openjdk.java.net/~simonis/webrevs/2019/8234011.v2/

Thank you for the updates.  I am still a bit skeptical of the use of 
SkippedException here as you would never see the test is no longer passing due 
to an implementation change unless you are specifically looking for it.

That being said,  if others who have more experience with using this Exception 
in a similar scenario are good, then I am good.

So once we get a couple of other views on this from others with a thumbs up for 
using SkippedException here, we are good to go :-)

Best
Lance



Thanks,
Volker

[1] 
https://openjdk.java.net/jtreg/faq.html#what-if-a-test-does-not-apply-in-a-given-situation
[2] https://bugs.openjdk.java.net/browse/JDK-8208655


Best
Lance


Thank you and best regards,
Volker


Thank you again for the fix
Best
Lance

On Nov 13, 2019, at 11:26 AM, Langer, Christoph 
mailto:christoph.lan...@sap.com> 
<mailto:christoph.lan...@sap.com> <mailto:christoph.lan...@sap.com>> wrote:

Hi Volker,

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

I have a few

RFR: 8234185: Cleanup usage of canonicalize function between libjava, hotspot and libinstrument

[Langer, Christoph]

Hi,

please review this cleanup change regarding function "canonicalize" of libjava.

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


The goal is to cleanup how this function is defined and used. One thing is, 
that there was an unnecessary wrapper function "Canonicalize" in jni_util.c. It 
wrapped the call to "canonicalize". We can get rid of this wrapper. 
Unfortunately, it is not possible to just export "canonicalize" since this will 
conflict with a method signature from the math library, at least on modern 
Linuxes. So I decided to call the method JDK_Canonicalize and will correctly 
define it in jdk_util.h which can be included everywhere.



Hotspot's classloader.cpp will dynamically resolve this method, so I add a 
local declaration of the function pointer in there.



This change shall be predecessor of JDK-8223261, where a review was already 
started here: 
https://mail.openjdk.java.net/pipermail/core-libs-dev/2019-November/063398.html

Thanks
Christoph

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

[Langer, Christoph]

Hi,

after exchanging some direct mails with Lance, I came to the conclusion that 
the current behavior to ignore file system property "multi-release" when 
"releaseVersion" is explicitly set to null is the right thing to do. So I 
updated the webrev to reinstate this functionality and added explicit comments 
as well as augmenting MultiReleaseJarTest.java a little bit to test that 
"multi-release" is ignored in the right places.

This is the new webrev: http://cr.openjdk.java.net/~clanger/webrevs/8234089.1/

Best regards
Christoph


From: Langer, Christoph
Sent: Mittwoch, 13. November 2019 17:42
To: core-libs-dev@openjdk.java.net; nio-dev 
Subject: RFR: 8234089: (zipfs) Remove classes JarFileSystemProvider and 
JarFileSystem

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 (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

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(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
> 
> 

RE: RFR JDK-8232724: Remove indirection with calling JNU_NewStringPlatform

[Langer, Christoph]

Sure. Thanks, Alexey.

> -Original Message-
> From: Alexey Ivanov 
> Sent: Mittwoch, 30. Oktober 2019 14:22
> To: Langer, Christoph ; David Holmes
> ; core-libs ;
> hotspot-runtime 
> Cc: Claes Redestad 
> Subject: Re: RFR JDK-8232724: Remove indirection with calling
> JNU_NewStringPlatform
> 
> Thank you, David and Christoph, for your reviews.
> 
> I'll update the copyright year before the push if you don't mind.
> 
> Regards,
> Alexey
> 
> On 30/10/2019 09:05, Langer, Christoph wrote:
> > +1
> >
> > You'll have to update the copyright years in the jni_util files.
> >
> > Cheers
> > Christoph
> >
> >> -Original Message-
> >> From: David Holmes 
> >> Sent: Mittwoch, 30. Oktober 2019 05:29
> >> To: Alexey Ivanov ; core-libs  >> d...@openjdk.java.net>; hotspot-runtime  >> d...@openjdk.java.net>; Langer, Christoph 
> >> Cc: Claes Redestad 
> >> Subject: Re: RFR JDK-8232724: Remove indirection with calling
> >> JNU_NewStringPlatform
> >>
> >> LGTM.
> >>
> >> Thanks,
> >> David
> >>
> >> On 30/10/2019 1:04 am, Alexey Ivanov wrote:
> >>> Hi David, Christoph,
> >>>
> >>> Thank you for your reviews.
> >>>
> >>> On 29/10/2019 07:49, David Holmes wrote:
> >>>>> Shall I remove one of them? The one in jvm.h because it's unused?
> >>>> Yes please remove the unused one in jvm.h.
> >>> The updated webrev:
> >>> http://cr.openjdk.java.net/~aivanov/8232724/webrev.01/
> >>>
> >>> --
> >>> Regards,
> >>> Alexey

RE: RFR JDK-8232724: Remove indirection with calling JNU_NewStringPlatform

[Langer, Christoph]

+1

You'll have to update the copyright years in the jni_util files.

Cheers
Christoph

> -Original Message-
> From: David Holmes 
> Sent: Mittwoch, 30. Oktober 2019 05:29
> To: Alexey Ivanov ; core-libs  d...@openjdk.java.net>; hotspot-runtime  d...@openjdk.java.net>; Langer, Christoph 
> Cc: Claes Redestad 
> Subject: Re: RFR JDK-8232724: Remove indirection with calling
> JNU_NewStringPlatform
> 
> LGTM.
> 
> Thanks,
> David
> 
> On 30/10/2019 1:04 am, Alexey Ivanov wrote:
> > Hi David, Christoph,
> >
> > Thank you for your reviews.
> >
> > On 29/10/2019 07:49, David Holmes wrote:
> >>> Shall I remove one of them? The one in jvm.h because it's unused?
> >>
> >> Yes please remove the unused one in jvm.h.
> >
> > The updated webrev:
> > http://cr.openjdk.java.net/~aivanov/8232724/webrev.01/
> >
> > --
> > Regards,
> > Alexey

RE: RFR (S) 8232168: Fix non wide char canonicalization on Windows

[Langer, Christoph]

Thanks Alan,

I'll push this after running through jdk-submit. I'm preparing another patch 
for some further cleanup regarding canonicalize. But it won't get rid of the 
call from JPLIS.

Best regards
Christoph

> -Original Message-
> From: Alan Bateman 
> Sent: Dienstag, 29. Oktober 2019 15:33
> To: Langer, Christoph ; hotspot-runtime-
> d...@openjdk.java.net; Java Core Libs 
> Cc: Schmelter, Ralf 
> Subject: Re: RFR (S) 8232168: Fix non wide char canonicalization on Windows
> 
> On 23/10/2019 14:26, Langer, Christoph wrote:
> > Hi Alan,
> >
> >> I have this on my list to look at. I think we'll need to figure out a
> >> path to separate the usages (the JPLIS agent usage is technical debt, we
> >> should have addressed that issue a long time ago).
> > I agree that it's a good thing to explore how the different usages of calls 
> > to
> canonicalize could be disentangled. However, can't we defer that to another
> item? This one seems like a straightforward bugfix to me, with some cleanup
> attached. And I've prepared some more cleanup in that area already which is
> dependent on this fix - so I'd appreciate if this wouldn't be procrastinated 
> too
> much :)
> I went through the change and they look okay to me (much smaller than I
> was expecting). I think we will need to figure out a solution for the
> JPLIS agent at some point because it shouldn't be using functions in
> libjava (having the VM use the function isn't too bad I suppose).
> 
> -Alan

RE: RFR JDK-8232724: Remove indirection with calling JNU_NewStringPlatform

[Langer, Christoph]

Hi Alexey,

> Please review the following fix which removes indirection in calling
> JNU_NewStringPlatform via NewStringPlatform.
> 
> bug: https://bugs.openjdk.java.net/browse/JDK-8232724
> webrev: http://cr.openjdk.java.net/~aivanov/8232724/webrev.00/
> 
> It is a follow-up fix to JDK-8232624 and JDK-8231355. It removes
> NewStringPlatform alias as Claes did in JDK-8231355. To call
> JNU_NewStringPlatform, the function pointer to_java_string_fn_t is
> declared with JNICALL (__stdcall); the function is looked up by both the
> undecorated name and the __stdcall-decorated name for Win32.
> 
> Tier 1 and 2 tests are all green.

This looks good to me.

> There are two declarations of the to_java_string_fn_t function pointer:
> in jvm.h and locally in javaClasses. I searched the source code for it
> and didn't find any usages but in
> java_lang_String::create_from_platform_dependent_str. The declaration in
> jvm.h uses "char *" whereas javaClasses uses "const char *".
> 
> Shall I remove one of them? The one in jvm.h because it's unused?

Yes, I would suggest dropping the declaration in jvm.h

Cheers
Christoph

RE: RFR 8231451: ZipFileInputStream::skip handling of negative values with STORED entries

[Langer, Christoph]

Hi Lance,

I looked at this patch as well and it seems good to me.

Cheers
Christoph

> -Original Message-
> From: core-libs-dev  On Behalf
> Of Lance Andersen
> Sent: Freitag, 25. Oktober 2019 20:42
> To: Alan Bateman 
> Cc: core-libs-dev 
> Subject: Re: RFR 8231451: ZipFileInputStream::skip handling of negative
> values with STORED entries
> 
> 
> > On Oct 23, 2019, at 4:58 PM, Alan Bateman 
> wrote:
> >
> > On 23/10/2019 19:06, Lance Andersen wrote:
> >>
> >>> On Oct 22, 2019, at 2:37 PM, Alan Bateman  > wrote:
> >>>
> >>> I assume skip(Long.MAX_VALUE) will cause n+pos to overflow and it will
> skip backwards rather than to the end.
> >>
> >> That is correct.  If you prefer it to skip to the end, I will adjust 
> >> accordingly.
> > It would be surprising to specify a positive value and have it skip 
> > backwards
> so I think this should be fixed.
> 
> I have updated the patch per your suggestion.
> 
> The updated webrev can be found at:
> http://cr.openjdk.java.net/~lancea/8231451/webrev.02/index.html
> 
> 
> mach5 jdk-tier1 through jdk-tier3 are clean.
> 
> Best
> Lance
> >
> > -Alan
> 
>  
>  
> 
>  Lance Andersen|
> Principal Member of Technical Staff | +1.781.442.2037
> Oracle Java Engineering
> 1 Network Drive
> Burlington, MA 01803
> lance.ander...@oracle.com 
> 
> 

RE: RFR: JDK-8232879: (zipfs) Writing out data with ZipFileSystem leads to a CRC failure in the generated jar file

[Langer, Christoph]

Hi Lance,

thanks for reworking the test. That definitely improves coverage.

The comment for the test method (line 56/57) could be improved like: "Verify 
all write methods of an OutputStream that can be used to write to an entry of 
Zip Filesystem by exercising all of them when creating an archive and then 
traversing the archive with ZipFile, ZipInputStream and the Zip Filesystem".

Other than that, it's good to go for mw.

BTW, I think meanwhile all these zipfs tests have quite some duplicate 
infrastructure classes and methods (e.g. Entry) that could be shared. Maybe we 
should try to carve out some stuff into a library or some Auxillary helper 
class. What do you think?

Thanks
Christoph

> -Original Message-
> From: core-libs-dev  On Behalf
> Of Lance Andersen
> Sent: Samstag, 26. Oktober 2019 04:42
> To: Jaikiran Pai 
> Cc: core-libs-dev@openjdk.java.net
> Subject: Re: RFR: JDK-8232879: (zipfs) Writing out data with ZipFileSystem
> leads to a CRC failure in the generated jar file
> 
> 
> > On Oct 25, 2019, at 10:10 PM, Jaikiran Pai 
> wrote:
> >
> > Thank you for the review, Lance.
> >
> 
> You are very welcome Jaikiran.
> > On 26/10/19 4:25 AM, Lance Andersen wrote:
> >>
> >> The change to Zip FS looks good.  I re-worked the test so that it also runs
> against ZipFile (which uses the CEN vs the LOC) and Zip FS in addition to
> ZipInputStream for extra coverage.
> >>
> >> The webrev can be found at:
> http://cr.openjdk.java.net/~lancea/8232879/webrev.00/index.html
> Looks
> fine. A very minor note about
> http://cr.openjdk.java.net/~lancea/8232879/webrev.00/test/jdk/jdk/nio/zip
> fs/CRCWriteTest.java.html
>  pfs/CRCWriteTest.java.html>
> > 136 while ((ze = zis.getNextEntry()) != null) {
> >  137 assertNotNull(ze);
> >
> > Looks like an oversight - that assertNotNull(ze) on 137 isn't needed due to
> the != null check on 1
> 
> Yep, not needed.  Sometimes you do things on auto pilot ;-)
> 
> Will remove before I push the change.
> > 36.
> >
> > -Jaikiran
> >
> 
>  
>  
> 
>  Lance Andersen|
> Principal Member of Technical Staff | +1.781.442.2037
> Oracle Java Engineering
> 1 Network Drive
> Burlington, MA 01803
> lance.ander...@oracle.com 
> 
> 

RE: RFR (S) 8232168: Fix non wide char canonicalization on Windows

[Langer, Christoph]

> I would like to review this change. Would you mind holding off for a few
> days until I can find time?

OK, sure. Then I'll wait for you.

Cheers
Christoph

RE: RFR (S) 8232168: Fix non wide char canonicalization on Windows

[Langer, Christoph]

Thanks, Calvin for the review.

Alan, unless you have severe objections I'd push this tomorrow after running 
another night of tests with this patch and run doing jdk-submit. Is that ok 
with you?

Thanks
Christoph

> -Original Message-
> From: Calvin Cheung 
> Sent: Mittwoch, 23. Oktober 2019 20:21
> To: Langer, Christoph ; Alan Bateman
> ; hotspot-runtime-...@openjdk.java.net; Java
> Core Libs 
> Subject: Re: RFR (S) 8232168: Fix non wide char canonicalization on Windows
> 
> Hi Christoph,
> 
> The changes look good.
> 
> I agree that other mentioned items (the JPLIS agent usage and moving the
> canonicalize function into hotspot) could be done in separate RFEs.
> 
> thanks,
> 
> Calvin
> 
> On 10/23/19 5:57 AM, Langer, Christoph wrote:
> > Hi,
> >
> > I've picked up Ralf's patch and cleaned it up a little bit. But apart from 
> > some
> comment changes it should be the same as his original version. So, what
> happens is that the windows 'canonicalize" function will only delegate to
> 'wcanonicalize' from now on.  Furthermore,  'canonicalizeWithPrefix' along
> with some auxiliary stuff could be removed completely.
> >
> > Can I please get a review for this, as I'm planning on doing further cleanup
> around canonicalization.
> >
> > New webrev: http://cr.openjdk.java.net/~clanger/webrevs/8232168.0/
> >
> > Thanks
> > Christoph
> >
> >> -Original Message-
> >> From: hotspot-runtime-dev  >> boun...@openjdk.java.net> On Behalf Of Alan Bateman
> >> Sent: Freitag, 18. Oktober 2019 12:41
> >> To: Schmelter, Ralf ; David Holmes
> >> ; hotspot-runtime-...@openjdk.java.net;
> Java
> >> Core Libs 
> >> Subject: Re: RFR (S) 8232168: Fix non wide char canonicalization on
> Windows
> >>
> >> On 16/10/2019 09:28, Schmelter, Ralf wrote:
> >>> Hi David,
> >>>
> >>> the canonicalize() method is never used by java.io or any Java code.
> >> Currently it is used by the hotspot in classloader.cpp (which I use in the
> test)
> >> and in libinstrument in InvocationAdapter.c. There is no way to test it in
> core-
> >> libs. One can argue if the canonicalize method is in the right file, but 
> >> that
> >> should be a separate discussion.
> >> I plan to review this, I just haven't had time yet. You are right that
> >> there is technical debt here, include dependency in the JPLIS agent
> >> which crept in when java.lang.instrument was updated to support
> >> augmenting of the boot class path.
> >>
> >> -Alan

RE: RFR: JDK-8232879: (zipfs) Writing out data with ZipFileSystem leads to a CRC failure in the generated jar file

[Langer, Christoph]

Hi Jaikiran,

thanks for proposing this patch. I just had a look and think the fix in 
ZipFileSystem.java is the right thing to do.

As for the test: That could be simplified a bit.

For the path to the test file, you could simply do: final Path jarPath = 
Paths.get("zipfs-crc-test.jar");
The test is run in the scratch directory of jtreg, so no reason to go to 
java.io.tmpdir.

You can also skip deletion of this test file in the finally block, as the jtreg 
scratch directories will be wiped by jtreg eventually.

For the existence check of the test file in line 62, you can simply use 
Files.exists.

As for creating the zipfs Filesystem (line 68), you can simply use 
FileSystems.newFileSystem(Path, Map), no need to mess around with URIs.

Line 96, where construct the input stream and then in 97, the ZipInputStream, I 
suggest you either put both into the try statement or you use ZipInputStream 
zis = new ZipInputStream(Files.newInputStream(jarPath)) and then rely on 
ZipInputStream cascading the close.

And my last suggestion: you could statically import Assert.assertEquals to 
shorten lines 105 and 106.

Thanks
Christoph

> -Original Message-
> From: core-libs-dev  On Behalf
> Of Jaikiran Pai
> Sent: Mittwoch, 23. Oktober 2019 13:24
> To: core-libs-dev@openjdk.java.net
> Subject: RFR: JDK-8232879: (zipfs) Writing out data with ZipFileSystem leads
> to a CRC failure in the generated jar file
> 
> Can I please get a review and a sponsor for a potential fix for
> JDK-8232879[1]? The patch is available as a webrev at [2].
> 
> What's happening in there is that the
> jdk.nio.zipfs.ZipFileSystem.DeflatingEntryOutputStream is overriding the
> one arg write(byte b) method and calling the super.write(b) and then
> doing a crc.update. The super.write(b)
> (java.util.zip.DeflaterOutputStream in this case) actually internally
> calls the 3 arg write(b, offset, length) which is overriding by this
> jdk.nio.zipfs.ZipFileSystem.DeflatingEntryOutputStream. In its
> implementation of write(b, offset, length), in addition to (rightly)
> calling super.write(b, offset, length), this method also updates the CRC
> (again). So this ends up updating the CRC multiple times when the single
> arg write is invoked.
> 
> The patch now removes this overridden implementation of write(b) in the
> DeflatingEntryOutputStream so that the call is handled by the
> java.util.zip.DeflaterOutputStream. Although there's no @implNote on
> java.util.zip.DeflaterOutputStream#write(byte b) static that it's
> (always) going to call the 3 arg write(b, offset, length) method, the
> implementation as of now does indeed do that. So I guess, its probably
> OK to rely on that knowledge and get rid of this overridden write(b)
> method instead of coming up with a bit more complicated fix.
> 
> The patch also includes a jtreg testcase which reproduces this issues
> and verifies the fix.
> 
> [1] https://bugs.openjdk.java.net/browse/JDK-8232879
> 
> [2] https://cr.openjdk.java.net/~jpai/webrev/8232879/1/webrev/
> 
> -Jaikiran
> 

RE: RFR (S) 8232168: Fix non wide char canonicalization on Windows

[Langer, Christoph]

Hi Alan,

> I have this on my list to look at. I think we'll need to figure out a
> path to separate the usages (the JPLIS agent usage is technical debt, we
> should have addressed that issue a long time ago).

I agree that it's a good thing to explore how the different usages of calls to 
canonicalize could be disentangled. However, can't we defer that to another 
item? This one seems like a straightforward bugfix to me, with some cleanup 
attached. And I've prepared some more cleanup in that area already which is 
dependent on this fix - so I'd appreciate if this wouldn't be procrastinated 
too much :) 

Thanks
Christoph

RE: RFR (S) 8232168: Fix non wide char canonicalization on Windows

[Langer, Christoph]

Hi,

I've picked up Ralf's patch and cleaned it up a little bit. But apart from some 
comment changes it should be the same as his original version. So, what happens 
is that the windows 'canonicalize" function will only delegate to 
'wcanonicalize' from now on.  Furthermore,  'canonicalizeWithPrefix' along with 
some auxiliary stuff could be removed completely.

Can I please get a review for this, as I'm planning on doing further cleanup 
around canonicalization.

New webrev: http://cr.openjdk.java.net/~clanger/webrevs/8232168.0/

Thanks
Christoph

> -Original Message-
> From: hotspot-runtime-dev  boun...@openjdk.java.net> On Behalf Of Alan Bateman
> Sent: Freitag, 18. Oktober 2019 12:41
> To: Schmelter, Ralf ; David Holmes
> ; hotspot-runtime-...@openjdk.java.net; Java
> Core Libs 
> Subject: Re: RFR (S) 8232168: Fix non wide char canonicalization on Windows
> 
> On 16/10/2019 09:28, Schmelter, Ralf wrote:
> > Hi David,
> >
> > the canonicalize() method is never used by java.io or any Java code.
> Currently it is used by the hotspot in classloader.cpp (which I use in the 
> test)
> and in libinstrument in InvocationAdapter.c. There is no way to test it in 
> core-
> libs. One can argue if the canonicalize method is in the right file, but that
> should be a separate discussion.
> >
> I plan to review this, I just haven't had time yet. You are right that
> there is technical debt here, include dependency in the JPLIS agent
> which crept in when java.lang.instrument was updated to support
> augmenting of the boot class path.
> 
> -Alan

RE: RFR (S) 8232168: Fix non wide char canonicalization on Windows

[Langer, Christoph]

Hi,

this item is really on  the edge between core-libs and hotspot. So I'm 
including both lists now.

The canonicalize() method is implemented in libjava, for both Unix and Windows. 
For Unix, it is used from hotspot, libjava (the file system implementations) 
and libinstrument. But for Windows, canonicalize isn't actually used at all by 
libjava any more after JDK-8190737 [0] since the WinNTFileSystem implementation 
uses wcanonicalize now.

Maybe this calls out for some further refactoring. One could move canonicalize 
completely into hotspot and use it from there. That would appear sensible to me 
but I don't know how the policy is for exporting methods from hotspot and 
consuming them in libjava. Alternatively, they could be kept at libjava but one 
has to know/keep in mind, that it is not used by the Windows java classlib 
implementation. If you say that it's the right way to go to move canonicalize 
into hotspot, I'd volunteer to bring this refactoring forward. Let me know.

But, apart from that possible refactoring, Ralf's change seems completely right 
to me. Also, using the hotspot test to verify the fix appears right. This test 
is a place where the functionality of Windows canonicalize is stressed and you 
won't find so many of them...

This fix seems a logical followup for JDK-8190737 [0] and JDK-8191521 [1].

Best regards
Christoph

[0] https://bugs.openjdk.java.net/browse/JDK-8190737
[1] https://bugs.openjdk.java.net/browse/JDK-8191521


> On 15/10/2019 16:19, Schmelter, Ralf wrote:
> > Hello,
> >
> > this is a small change which fixes the some problems with the canonicalize()
> method in canonicalize_md.c.
> Can you split out the changes to the java.io into a separate issue and
> bring them to core-libs-dev to discuss? I suspect the changes will
> require a lot of review cycles.
> 
> -Alan
> 
> 
> > Currently it lets the wcanonicalize() method do the  work for path lengths >
> MAX_PATH. This change always calls the wcanonicalize() method, since the
> old implementation doesn't work for all path lengths <= MAX_PATH. In
> addition the canonicalizeWithPrefix() method has been removed, since it
> seems to be not used anymore.
> >
> > Bug report: https://bugs.openjdk.java.net/browse/JDK-8232168
> > Webrev:
> http://cr.openjdk.java.net/~rschmelter/webrevs/8232168/webrev.0/
> >
> > Best regards,
> > Ralf

RE: [11u] RFR 8229773: Resolve permissions for code source URLs lazily

[Langer, Christoph]

Thanks for the review, Claes.

> -Original Message-
> From: Claes Redestad 
> Sent: Mittwoch, 18. September 2019 19:16
> To: Langer, Christoph ; jdk-updates-
> d...@openjdk.java.net
> Cc: core-libs-dev 
> Subject: Re: [11u] RFR 8229773: Resolve permissions for code source URLs
> lazily
> 
> Looks ok to me!
> 
> /Claes
> 
> On 2019-09-18 12:01, Langer, Christoph wrote:
> > Hi,
> >
> > please review the backport for JDK-8229773: Resolve permissions for code
> source URLs lazily to OpenJDK11 updates.
> >
> > Bug: https://bugs.openjdk.java.net/browse/JDK-8229773
> > Webrev: http://cr.openjdk.java.net/~clanger/webrevs/8229773.11u-
> dev.0/
> > Original Change: https://hg.openjdk.java.net/jdk/jdk/rev/1b6806340400
> >
> > The patch did not apply cleanly and I had to resolve
> > src/java.base/share/classes/java/lang/System.java: imports and the hunk
> about "// ensure the default file system is initialized"
> > src/java.base/share/classes/jdk/internal/loader/BuiltinClassLoader.java:
> import statements
> >
> > With these modifications, the patch applies and regression testing does not
> show findings.
> >
> > Thanks
> > Christoph
> >

[11u] RFR 8229773: Resolve permissions for code source URLs lazily

[Langer, Christoph]

Hi,

please review the backport for JDK-8229773: Resolve permissions for code source 
URLs lazily to OpenJDK11 updates.

Bug: https://bugs.openjdk.java.net/browse/JDK-8229773
Webrev: http://cr.openjdk.java.net/~clanger/webrevs/8229773.11u-dev.0/
Original Change: https://hg.openjdk.java.net/jdk/jdk/rev/1b6806340400

The patch did not apply cleanly and I had to resolve
src/java.base/share/classes/java/lang/System.java: imports and the hunk about 
"// ensure the default file system is initialized"
src/java.base/share/classes/jdk/internal/loader/BuiltinClassLoader.java: import 
statements

With these modifications, the patch applies and regression testing does not 
show findings.

Thanks
Christoph

RE: RFR: 8228482: fix xlc16/xlclang comparison of distinct pointer types and string literal conversion warnings

[Langer, Christoph]

Hi Matthias,

this seems fine to me. The change in NetworkInterface.c is correct, too.

Best regards
Christoph

> -Original Message-
> From: core-libs-dev  On Behalf
> Of Baesken, Matthias
> Sent: Donnerstag, 25. Juli 2019 15:45
> To: Doerr, Martin ; 'hotspot-
> d...@openjdk.java.net' ; core-libs-
> d...@openjdk.java.net; net-...@openjdk.java.net
> Cc: 'ppc-aix-port-...@openjdk.java.net'  d...@openjdk.java.net>
> Subject: [CAUTION] RE: RFR: 8228482: fix xlc16/xlclang comparison of distinct
> pointer types and string literal conversion warnings
> 
> Thanks Martin .
> May I get a second review ?
> 
> Best regards, Matthias
> 
> 
> From: Doerr, Martin
> Sent: Mittwoch, 24. Juli 2019 12:14
> To: Baesken, Matthias ; 'hotspot-
> d...@openjdk.java.net' ; core-libs-
> d...@openjdk.java.net; net-...@openjdk.java.net
> Cc: 'ppc-aix-port-...@openjdk.java.net'  d...@openjdk.java.net>
> Subject: RE: RFR: 8228482: fix xlc16/xlclang comparison of distinct pointer
> types and string literal conversion warnings
> 
> Hi Matthias,
> 
> I wouldn’t say „looks good”, but I think it’s the right thing to do 
> The type casts look correct to fit to the AIX headers.
> 
> libodm_aix:
> Good. Maybe we should open a new issue for freeing what’s returned by
> odm_set_path and we could also remove the AIX 5 support.
> 
> NetworkInterface.c:
> Strange, but ok. Should be reviewed by somebody else in addition.
> 
> Other files:
> No comments.
> 
> Best regards,
> Martin
> 
> 
> From: ppc-aix-port-dev  boun...@openjdk.java.net boun...@openjdk.java.net>> On Behalf Of Baesken, Matthias
> Sent: Dienstag, 23. Juli 2019 17:15
> To: 'hotspot-...@openjdk.java.net'  d...@openjdk.java.net>; core-libs-
> d...@openjdk.java.net; net-
> d...@openjdk.java.net
> Cc: 'ppc-aix-port-...@openjdk.java.net'  d...@openjdk.java.net>
> Subject: RFR: 8228482: fix xlc16/xlclang comparison of distinct pointer types
> and string literal conversion warnings
> 
> Hello please review this patch .
> 
> It fixes a couple of  xlc16/xlclang warnings  , especially  comparison of 
> distinct
> pointer types and string literal conversion warnings  .
> 
> When building with xlc16/xlclang, we still have a couple of warnings that have
> to be fixed :
> warning: ISO C++11 does not allow conversion from string literal to 'char *' 
> [-
> Wwritable-strings]
> for example :
> /nightly/jdk/src/hotspot/os/aix/libodm_aix.cpp:81:18: warning: ISO C++11
> does not allow conversion from string literal to 'char *' [-Wwritable-strings]
>   odmWrapper odm("product", "/usr/lib/objrepos"); // could also use "lpp"
>  ^
> /nightly/jdk/src/hotspot/os/aix/libodm_aix.cpp:81:29: warning: ISO C++11
> does not allow conversion from string literal to 'char *' [-Wwritable-strings]
>   odmWrapper odm("product", "/usr/lib/objrepos"); // could also use "lpp"
> ^
> 
> warning: comparison of distinct pointer types, for example :
> 
> /nightly/jdk/src/java.desktop/aix/native/libawt/porting_aix.c:50:14:
> warning: comparison of distinct pointer types ('void *' and 'char *') [-
> Wcompare-distinct-pointer-types]
> addr < (((char*)p->ldinfo_textorg) + p->ldinfo_textsize)) {
>  ^ ~
> 
> 
> 
> Bug/webrev :
> 
> 
> https://bugs.openjdk.java.net/browse/JDK-8228482
> 
> http://cr.openjdk.java.net/~mbaesken/webrevs/8228482.1/
> 
> 
> Thanks, Matthias
> 

RE: RFR 8226530: ZipFile reads wrong entry size from ZIP64 entries

[Langer, Christoph]

Hi Lance,

this looks good to me.

Best regards
Christoph

> -Original Message-
> From: core-libs-dev  On Behalf
> Of Lance Andersen
> Sent: Dienstag, 6. August 2019 22:32
> To: core-libs-dev 
> Subject: RFR 8226530: ZipFile reads wrong entry size from ZIP64 entries
> 
> Hi
> 
> Please review the fix for https://bugs.openjdk.java.net/browse/JDK-
> 8226530 , where
> ZipFile does not return the correct size from the CEN when  the Zip64
> Extended Information Extra Field is used to store the entry size
> 
> The webrev can be found at
> http://cr.openjdk.java.net/~lancea/8226530/webrev.00/index.html
> 
> 
> Mach5 jdk tier1-tier3 all pass
> 
> Best
> Lance
> 
>  
>  
> 
>  Lance Andersen|
> Principal Member of Technical Staff | +1.781.442.2037
> Oracle Java Engineering
> 1 Network Drive
> Burlington, MA 01803
> lance.ander...@oracle.com 
> 
> 

RE: RFR 8213031: (zipfs) Add support for POSIX file permissions

[Langer, Christoph]

Hi,

the changeset for the zipfs POSIX permissions support seems to be ready now. 
I've spent some iterations with Lance to finalize it, mainly some formal things 
and additions/cleanups to the test. I've got reviews now from both, Alan and 
Lance.

So, that's the last call for review/feedback/objections from anybody else. If I 
don't get further feedback I'm going to push this within the next 2-3 days: 
http://cr.openjdk.java.net/~clanger/webrevs/8213031.17/

Thank you,
Christoph


From: Langer, Christoph
Sent: Mittwoch, 5. Juni 2019 16:45
To: Alan Bateman ; Lance Andersen 

Cc: nio-dev ; Java Core Libs 

Subject: RE: RFR 8213031: (zipfs) Add support for POSIX file permissions

Hi Lance, Alan, et al.,

as it seems we are getting close to CSR approval, the changeset should also be 
finally reviewed.

Here is the most current version: 
http://cr.openjdk.java.net/~clanger/webrevs/8213031.14/

I have made some updates to ZipConstants as suggested by Lance in a private 
mail.

Best regards
Christoph

RE: RFR : 8227737: avoid implicit-function-declaration on AIX

[Langer, Christoph]

Hi Matthias,



looks good, thanks for doing this.



How far are we then from enabling "warnings as errors" on AIX? :P



Best regards

Christoph



From: awt-dev  On Behalf Of Baesken, Matthias
Sent: Dienstag, 16. Juli 2019 17:04
To: Java Core Libs ; nio-...@openjdk.java.net; 
net-...@openjdk.java.net; awt-...@openjdk.java.net
Cc: 'ppc-aix-port-...@openjdk.java.net' 
Subject: [CAUTION]  RFR : 8227737: avoid implicit-function-declaration 
on AIX



Hello, please review the following  AIX related change .

It fixes a number of  missing inclusions  leading to  
implicit-function-declaration  warnings when compiling  with the recent xlc16 
/xlclang .





At various places in the native C coding in jdk, we miss header inclusions on 
AIX.
This leads to warnings like :

/nightly/jdk/src/java.base/unix/native/libjava/childproc.c:99:5: warning: 
implicitly declaring library function 'snprintf'
with type 'int (char *, unsigned long, const char *, ...)' 
[-Wimplicit-function-declaration]
snprintf(aix_fd_dir, 32, "/proc/%d/fd", getpid());
^
/nightly/jdk/src/java.base/unix/native/libjava/childproc.c:99:5: note: include 
the header  or explicitly provide a declaration for 'snprintf'

or

/nightly/jdk/src/java.base/aix/native/libjli/java_md_aix.c:38:5: warning: 
implicitly declaring library function 'memset'
with type 'void *(void *, int, unsigned long)' [-Wimplicit-function-declaration]
memset((void *)info, 0, sizeof(Dl_info));

/nightly/jdk/src/java.base/aix/native/libjli/java_md_aix.c:38:5: note: include 
the header  or explicitly provide a declaration for 'memset'







Bug/webrev :



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



http://cr.openjdk.java.net/~mbaesken/webrevs/8227737.0/





Thanks, Matthias

RE: RFR: JDK-8227831: Avoid using volatile for write-once, read-many class field

[Langer, Christoph]

Hi Ogata,

this seems to make sense. So, +1 from my end.

Can you please add a space after "if" in line "734 
if(langReflectAccess == null) {"?

Thanks
Christoph

> -Original Message-
> From: core-libs-dev  On Behalf
> Of Kazunori Ogata
> Sent: Mittwoch, 17. Juli 2019 08:49
> To: core-libs-dev@openjdk.java.net
> Subject: RFR: JDK-8227831: Avoid using volatile for write-once, read-many
> class field
>
> Hi,
>
> May I have a review for "JDK-8227831: Avoid using volatile for write-once,
> read-many class field"?
>
> In jdk.internal.reflect.ReflectionFactory, there is a private class field
> named "langReflectAccess", which is referenced every time when the library
> handles various reflective operations.  This field is initialized on the
> first access to the ReflectionFactory class.  This field is declared as
> volatile to avoid (or reduce) race condition between initialization and
> references to the field.
>
> On the platforms with weak memory model (i.e, POWER and ARM), reading a
> volatile variable requires memory fence and incurs overhead.  So it is
> preferable to avoid use of volatile for such a write-once, read-many
> variable.
>
> langReflectAccess can be modified only in setLangReflectAccess() method.
> So we can avoid using volatile by modifying setLangReflectAccess() to use
> a synchronized block to avoid race condition.  This change reduced elapsed
> time of a micro benchmark by 9%, which repeatedly invoke
> Class.getMethods().
>
>
> Bug: https://bugs.openjdk.java.net/browse/JDK-8227831
>
> Webrev: http://cr.openjdk.java.net/~ogatak/8227831/webrev/
>
>
> Regards,
> Ogata

RE: 8226863: [aix] jdk/java/lang/reflect/exeCallerAccessTest cannot launch on primordial thread

[Langer, Christoph]

Hi Thomas,

I looked at this.

One thing that catches my eye immediately: exeCallerAccessTest.c, line 149: 
Looks like this #error would always fire on Windows. So the test wouldn’t 
compile there…? Or am I wrong?

Furthermore, if we do this new thread dance only on AIX, shouldn’t all the 
extra coding be guarded by #ifdef _AIX? The only common part of the changes 
would be to rename main to main_inner and call this method for all non-AIX 
cases…

Best regards
Christoph

From: ppc-aix-port-dev  On Behalf Of 
Thomas Stüfe
Sent: Donnerstag, 27. Juni 2019 09:02
To: Java Core Libs ; ppc-aix-port-dev 

Subject: 8226863: [aix] jdk/java/lang/reflect/exeCallerAccessTest cannot launch 
on primordial thread

Hi all,

Issue:
https://bugs.openjdk.java.net/browse/JDK-8226863
webrev:
http://cr.openjdk.java.net/~stuefe/webrevs/8226863--jdk-java-lang-reflect-execalleraccesstest-cannot-launch-on-primordial-thread/webrev.00/webrev/

we have this annoying issue on AIX that the libjvm cannot be invoked on a 
primordial thread. Therefore we need to change launchers which create the VM to 
spawn own threads (we only do this where it is worth the effort - this, 
admittedly, is a corner case, we could alternatively just disable the test on 
AIX).

This is annoying and vaguely embarrassing :-/ Maybe IBM will step in some time 
and help us solve the underlying issues which have to do with the inability to 
create guard pages on the primordial thread stack (among other things IIRC).

Thanks for reviewing,

Thomas

RE: [13] RFR: 8226876: Assertion in sun/util/locale/provider/CalendarDataUtility on Windows after JDK-8218960

[Langer, Christoph]

Sounds great. Thank you.

> -Original Message-
> From: naoto.s...@oracle.com 
> Sent: Donnerstag, 27. Juni 2019 23:16
> To: Langer, Christoph ; i18n-
> d...@openjdk.java.net; core-libs-dev 
> Subject: Re:  [13] RFR: 8226876: Assertion in
> sun/util/locale/provider/CalendarDataUtility on Windows after JDK-8218960
> 
> Thanks for the review, Christoph.
> 
> I'll wait for a whole day so that everyone sees the fix. Planning to
> push it tomorrow.
> 
> Naoto
> 
> On 6/27/19 2:05 PM, Langer, Christoph wrote:
> >>> The change looks good to me. But I would say the assertion in
> >> CalendarDataUtility in line 266 is pointless now, isn't it?
> >>
> >> Yes, but would not hurt leaving it. It would catch error if yet another
> >> case is installed (and it forgot to provide a default value) in the
> >> switch statement. So I just left it.
> >
> > Ok, sounds reasonable.
> >
> > What's your target for pushing this? I'd like to backport it to OpenJDK 
> > 11.0.4
> to fix the regression there...
> >
> > /Christoph
> >