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

[David Holmes]

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: [EXTERNAL] JDK-8234076 bug fix candidate

[Henry Jen]

> On Dec 5, 2019, at 5:57 PM, Nikola Grcevski  
> wrote:
> 
> Hello all again! 
> 
> Based on the suggestion by Kumar I made the following small patch to checkArg 
> src/java.base/share/native/libjli/args.c:
> 
> --- a/src/java.base/share/native/libjli/args.c
> +++ b/src/java.base/share/native/libjli/args.c
> @@ -130,6 +130,8 @@ static void checkArg(const char *arg) {
> }
> } else if (JLI_StrCmp(arg, "--disable-@files") == 0) {
> stopExpansion = JNI_TRUE;
> +} else if (JLI_StrNCmp(arg, "--module=", 9) == 0) {

I would suggest use JLI_StrCCmp as in java.c. I think combine this fix with 
origin crash prevention for -1 is a safe approach and most compatible to 
current behavior.

BTW, we need the patch to be hosted on cr.openjdk.java.net or you can attach 
the patch to the review thread if you are not yet an author.

Cheers,
Henry


> +idx = argsCount;
> }
> } else {
> if (!expectingNoDashArg) {
> 
> The change is in common code and simply checks for the usage of --module= and 
> deems the next argument as the start of the application arguments. I can 
> confirm that using -m instead of --module doesn't allow for the = separator 
> to be used, so we only need to check for --module= if this is a desired 
> change.
> 
> I tested with various combinations on the command line and I couldn't find a 
> set of arguments ordering that breaks the terminating quality of --module.
> 
> I also performed series of tests to try to break the argument expansion on 
> Windows with this change, but it worked in all instances. The testcase is 
> working correctly with this change, as well as the javac launcher command as 
> proposed by Kumar (java --module-path=mods 
> --module=jdk.compiler/com.sun.tools.javac.Main *.java).
> 
> I ran all the launcher tests on both Windows and Linux and all tests pass.
> 
> Please let me know if this is a preferred approach to address this bug or if 
> you think there's a potential problem with the change. If this is an 
> acceptable fix I can create new webrev with set of tests for the various edge 
> cases I tried, and new launcher specific tests that ensure argument expansion 
> is performed on Windows with this module usage.
> 
> Thank you,
> Nikola
> 
> -Original Message-
> From: Henry Jen  
> Sent: December 4, 2019 8:26 PM
> To: Kumar Srinivasan ; Alan Bateman 
> ; Nikola Grcevski 
> Cc: core-libs-dev@openjdk.java.net
> Subject: Re: [EXTERNAL] JDK-8234076 bug fix candidate
> 
>> On Dec 4, 2019, at 1:15 PM, Kumar Srinivasan  wrote:
>> 
>> Hi Nikola,
>> 
>> It looks ok to me, I will leave it to Henry and Alan to bless this.
>> 
>> IMHO: I think we should fix it correctly now than later, I don't think 
>> it is all that difficult AFAICT all the launcher has  to do is 
>> identify "--module==", then the next argument is the first index.
>> 
> 
> I don’t disagree, if we can decide whether —module= is allowed. Based on my 
> understanding and the document for java[1], the —module= is not necessarily 
> correct.
> 
> If we decided to accept it, the fix would be make sure the index set 
> correctly as Kumar suggested, and the fix can be relatively simple.
> 
> FWIW, it’s still possible the index is NOT_FOUND if there is no main class 
> specified, but that should never cause crash as if no main class is found, 
> the launcher should either execute other terminal argument or display help.
> 
> I agree the fix is not complete as it only make sure no crash can happen. It 
> doesn’t actually make —module= illegal and show help in case of that. At this 
> point, there is a discrepancy in launcher code regard —module usage, and we 
> need to fix that.
> 
> I’ll let Alan to comment about the —module option usage.
> 
> The webrev looks good to me, although I would like to see the test be more 
> close to other arguments processing test, but since this can only be 
> reproduced with —module= usage, I assume this is not bad.
> 
> [1] 
> https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fdocs.oracle.com%2Fen%2Fjava%2Fjavase%2F13%2Fdocs%2Fspecs%2Fman%2Fjava.htmldata=02%7C01%7CNikola.Grcevski%40microsoft.com%7C6158f8460dcd4c39518708d7792228c5%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637111061023797544sdata=uO2tgi1QNvXgI0wT%2FxOxB%2Bh10YpCbq37uzkKKlByUYg%3Dreserved=0
> 
>> Kumar
>> 
>> On Tue, Dec 3, 2019 at 5:29 PM Nikola Grcevski 
>>  wrote:
>> Hi Henry and Kumar,
>> 
>> Thanks again for your comments! I have updated the test to be part of 
>> test/jdk/tools/launcher/modules/basic, it took a lot less code to achieve 
>> the same amount of testing. I added a new test method inside BasicTest.java 
>> and tested on both Windows and Linux.
>> 
>> Please find my updated webrev here for your review: 
>> https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgrce
>> vski.github.io%2FJDK-8234076%2Fwebrev%2Fdata=02%7C01%7CNikola.Grc
>> evski%40microsoft.com%7C6158f8460dcd4c39518708d7792228c5%7C72f988bf86f

Re: RFR: JEP 367: Remove the Pack200 Tools and API

[Henry Jen]

OK, so I created an issue[1] for follow up for Windows build and reverted the 
change in flags-cflags.m4, if nothing else, I’ll push without another webrev 
pinging that I get an +1 from someone in build-de, Erik?

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

Cheers,
Henry

> On Dec 5, 2019, at 12:21 PM, Mandy Chung  wrote:
> 
> 
> 
> On 12/5/19 12:41 AM, Alan Bateman wrote:
>> On 05/12/2019 08:16, Henry Jen wrote:
>>> Hi,
>>> 
>>> Updated webrev[1] reflect comments since last webrev. Vicente had done all 
>>> the heavy-lifting and hand over to me to finish up.
>>> 
>>> Changes to symbols is reverted, as we expect that only need to be updated 
>>> in next release by running make/scripts/generate-symbol-data.sh.
>>> 
>>> The jar files are confusing in the webrev, but those files are removed. The 
>>> whole test/jdk/tools/pack200 is removed.
>>> 
>>> Cheers,
>>> Henry
>>> 
>>> [1] http://cr.openjdk.java.net/~henryjen/jdk14/8234542/0/webrev/
>>> 
>> The update webrev looks okay to me, except this part of the comment in 
>> flags-cflags.m4
>> 
>> "Now that unpack200 has been removed we should consider setting it for 
>> windows too. but this could be done as a follow-up effort. It could be that 
>> other other clients are relying on the current configuration for windows".
>> 
>> I think it would be best to create an infrastructure/build issue and leave 
>> most of this  confusing comment out.
>> 
> 
> I also think keeping flags-cflags.m4 as is and file a new build issue as a 
> follow-up would be better.
> 
> Otherwise, this updated webrev looks okay to me.
> 
> Mandy

RFR JDK-8235351: Lookup::unreflect should bind with the original caller independent of Method's accessible flag

[Mandy Chung]

When unreflecting on a Method for caller-sensitive method, the produced
MethodHandle should bind with the original caller lookup independent
of the accessible flag.

Webrev:
  http://cr.openjdk.java.net/~mchung/jdk14/8235351/webrev.00/

thanks
Mandy

RE: [EXTERNAL] JDK-8234076 bug fix candidate

[Nikola Grcevski]

Hello all again! 

Based on the suggestion by Kumar I made the following small patch to checkArg 
src/java.base/share/native/libjli/args.c:

--- a/src/java.base/share/native/libjli/args.c
+++ b/src/java.base/share/native/libjli/args.c
@@ -130,6 +130,8 @@ static void checkArg(const char *arg) {
 }
 } else if (JLI_StrCmp(arg, "--disable-@files") == 0) {
 stopExpansion = JNI_TRUE;
+} else if (JLI_StrNCmp(arg, "--module=", 9) == 0) {
+idx = argsCount;
 }
 } else {
 if (!expectingNoDashArg) {

The change is in common code and simply checks for the usage of --module= and 
deems the next argument as the start of the application arguments. I can 
confirm that using -m instead of --module doesn't allow for the = separator to 
be used, so we only need to check for --module= if this is a desired change.

I tested with various combinations on the command line and I couldn't find a 
set of arguments ordering that breaks the terminating quality of --module.

I also performed series of tests to try to break the argument expansion on 
Windows with this change, but it worked in all instances. The testcase is 
working correctly with this change, as well as the javac launcher command as 
proposed by Kumar (java --module-path=mods 
--module=jdk.compiler/com.sun.tools.javac.Main *.java).

I ran all the launcher tests on both Windows and Linux and all tests pass.

Please let me know if this is a preferred approach to address this bug or if 
you think there's a potential problem with the change. If this is an acceptable 
fix I can create new webrev with set of tests for the various edge cases I 
tried, and new launcher specific tests that ensure argument expansion is 
performed on Windows with this module usage.

Thank you,
Nikola

-Original Message-
From: Henry Jen  
Sent: December 4, 2019 8:26 PM
To: Kumar Srinivasan ; Alan Bateman 
; Nikola Grcevski 
Cc: core-libs-dev@openjdk.java.net
Subject: Re: [EXTERNAL] JDK-8234076 bug fix candidate

> On Dec 4, 2019, at 1:15 PM, Kumar Srinivasan  wrote:
> 
> Hi Nikola,
> 
> It looks ok to me, I will leave it to Henry and Alan to bless this.
> 
> IMHO: I think we should fix it correctly now than later, I don't think 
> it is all that difficult AFAICT all the launcher has  to do is 
> identify "--module==", then the next argument is the first index.
> 

I don’t disagree, if we can decide whether —module= is allowed. Based on my 
understanding and the document for java[1], the —module= is not necessarily 
correct.

If we decided to accept it, the fix would be make sure the index set correctly 
as Kumar suggested, and the fix can be relatively simple.

FWIW, it’s still possible the index is NOT_FOUND if there is no main class 
specified, but that should never cause crash as if no main class is found, the 
launcher should either execute other terminal argument or display help.

I agree the fix is not complete as it only make sure no crash can happen. It 
doesn’t actually make —module= illegal and show help in case of that. At this 
point, there is a discrepancy in launcher code regard —module usage, and we 
need to fix that.

I’ll let Alan to comment about the —module option usage.

The webrev looks good to me, although I would like to see the test be more 
close to other arguments processing test, but since this can only be reproduced 
with —module= usage, I assume this is not bad.

[1] 
https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fdocs.oracle.com%2Fen%2Fjava%2Fjavase%2F13%2Fdocs%2Fspecs%2Fman%2Fjava.htmldata=02%7C01%7CNikola.Grcevski%40microsoft.com%7C6158f8460dcd4c39518708d7792228c5%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637111061023797544sdata=uO2tgi1QNvXgI0wT%2FxOxB%2Bh10YpCbq37uzkKKlByUYg%3Dreserved=0

> Kumar
> 
> On Tue, Dec 3, 2019 at 5:29 PM Nikola Grcevski 
>  wrote:
> Hi Henry and Kumar,
> 
> Thanks again for your comments! I have updated the test to be part of 
> test/jdk/tools/launcher/modules/basic, it took a lot less code to achieve the 
> same amount of testing. I added a new test method inside BasicTest.java and 
> tested on both Windows and Linux.
> 
> Please find my updated webrev here for your review: 
> https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgrce
> vski.github.io%2FJDK-8234076%2Fwebrev%2Fdata=02%7C01%7CNikola.Grc
> evski%40microsoft.com%7C6158f8460dcd4c39518708d7792228c5%7C72f988bf86f
> 141af91ab2d7cd011db47%7C1%7C0%7C637111061023797544sdata=ee0dSSSJ%
> 2BXZogFtgPl8xFcUZ0P%2BX%2FR2G7x%2F4jjqiRIE%3Dreserved=0
> 
> Cheers,
> 
> Nikola
> 
> -Original Message-
> From: Henry Jen 
> Sent: December 3, 2019 11:39 AM
> To: Kumar Srinivasan 
> Cc: Nikola Grcevski ; Alan Bateman 
> ; core-libs-dev@openjdk.java.net
> Subject: Re: [EXTERNAL] JDK-8234076 bug fix candidate
> 
> Kumar,
> 
> Great to have you look at this, you are correct, this patch doesn’t address 
> the wildcard expansion issue, but only to address the potential crash if a 
> 

Re: RFR: JDK-8233270: Add support to jtreg helpers to unpack packages

[Alexander Matveev]

Hi Alexey,

1) Remove this file:
test/jdk/tools/jpackage/helpers/jdk/jpackage/test/JavaTool.java.rej
2) Agree with Phil, they probably should be pushed as two separate bugs.
3) Do you know how to run installer tests with new changes? It is not 
clear from code.


Changes itself looks fine.

Thanks,
Alexander

On 12/5/2019 5:33 PM, Philip Race wrote:

I don't understand the relationship between these two bugs.

-phil

On 12/5/19, 2:47 PM, Alexey Semenyuk wrote:

Please review  fixes for [1] and [2]. Both target jpackage tool.

The webrev is at [3].

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

[2] https://bugs.openjdk.java.net/browse/JDK-8230933

[3] http://cr.openjdk.java.net/~asemenyuk/8233270/webrev.00/

Re: RFR: JDK-8233270: Add support to jtreg helpers to unpack packages

[Philip Race]

I don't understand the relationship between these two bugs.

-phil

On 12/5/19, 2:47 PM, Alexey Semenyuk wrote:

Please review  fixes for [1] and [2]. Both target jpackage tool.

The webrev is at [3].

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

[2] https://bugs.openjdk.java.net/browse/JDK-8230933

[3] http://cr.openjdk.java.net/~asemenyuk/8233270/webrev.00/

Re: RFR JDK-8234049: Implementation of Memory Access API (Incubator)

[Maurizio Cimadamore]

On 06/12/2019 00:13, Remi Forax wrote:

Hi Maurizio,
that's a huge work :)

So as a guy discovering the API,
i've not taken a deep look to the implementation because i've noob questions.

The first sentence of the overview of GroupLayout should say that there are two 
types of GroupLayout struct and union instead of talking about members layouts 
in italic.
Still in GroupLayout, there is a static method "optSize" with no documentation and you 
don"t use the prefix "opt" anywhere else so i think it should be dropped.
hasNaturalAlignment() is protected, but you don"t allow user to implement their 
own GroupLayout, so it's like the method was package private but visible in the 
javadoc.
Good catch - these two methods were never meant to be exposed - they are 
morally package private members of a shared implementation class - I 
will fix that


MemoryLayout and MemoryLayout.PathElement static methods have names that are ok 
to use in static imports, but it's not explicit written in the javadoc.
People will say that Java is verbose :)


To give some context here - we had an earlier variant of the API which 
was much more concise - e.g.


layout.varHandle(p -> p.groupElement("foo").sequenceElement())

This uses a builder-style idiom. The problem with this approach is that 
is less constant-folding friendly - which is the main reason as to why 
we went the current path.




I don't see the point of having MemoryLayouts separated from MemoryLayout.


Possibly - I found myself thinking that too - although, with the 
subsequent Panama step (ABI support) we'll be adding a ton of 
ABI-dependent layouts in here... (but we could address that in other 
ways also).


If there's enough consensus one way or another I'm happy to consolidate 
the two classes.




MemoryAddress should have a method that does raw bytes comparison like there is 
MemoryAddress.copy() so simulate struct comparison.
Uhm - it's not a bad idea - but I don't immediately see why it *has* to 
be in the API - it will be a loop - we can't do much more than that, in 
terms of making it fast.


I don't understand how MemorySegment.acquire() solve the fact that you have two 
threads that can access to the same underlying memory area with unprotected 
access.
When you own a memory segment, an other thread can acquire a new memory segment 
and then both thread can mutate the same memory location ?


With concurrent access you have two kinds of races:

- access vs. access (e.g. thread A reads while threads B write)

- access vs. close (e.g. thread A reads while thread B close the segment)

The first race is *not* what we want to protect you against here. The 
VarHandle API has plenty of primitives which allow clients to roll in 
their own synchronization. The really nasty issue is with the latter 
race - if you access a segment that has been closed, you don't just read 
a stale value, you can crash the VM.


When you acquire a segment you obtain a new view of the same memory 
block which is temporally bounded by the original segment. The original 
segment cannot be closed until all the acquired views have been closed. 
This guarantees that the memory will never go away behind your back.




MemorySegment.isAccessible() is a very overloaded term, isOwnByCurrentThread() 
is maybe better ?

Open to suggestion - I don't particularly like yours though :-)


In the implementation of MemoryScope, please don't use i++ or i-- to change the 
lambda parameter, what you want is i - 1 or i + 1.
And i wander if it's not more efficient to use getAndIncrement and if it's 
negative (overflow), to do a supplementary volatile write to Integer.MIN_VALUE 
before raising the IllegalStateException.
Goo points - note that, in these cases, we don't really care about 
performance much - because acquiring a segment is not on the hot path 
(which is memory access).


In AddressVarHandleGenerator.iConstInsn(),
ICONST_M1, ICONST_0, etc are subsequent bytecodes so you can group all the case 
to: mv.visitInsn(ICONST_0 + i);


will do thanks!

Maurizio



regards,
Rémi

- Mail original -

De: "Maurizio Cimadamore" 
À: "core-libs-dev" , "hotspot-dev" 

Envoyé: Jeudi 5 Décembre 2019 22:04:55
Objet: RFR JDK-8234049: Implementation of Memory Access API (Incubator)
Hi,
as part of the effort to upstream the changes related to JEP 370
(foreign memory access API) [1], I'd like to ask for a code review for
the corresponding core-libs and hotspot changes:

http://cr.openjdk.java.net/~mcimadamore/panama/8234049/

A javadoc for the memory access API is also available here:

http://cr.openjdk.java.net/~mcimadamore/panama/memaccess_javadoc/jdk/incubator/foreign/package-summary.html

Note: the patch passes tier1, tier2 and tier3 testing (**)


Here is a brief summary of the changes in java.base and hotspot (the
remaining new files are implementation classes and tests for the new API):

* ciField.cpp - this one is to trust final fields in the foreign memory
access implementation (otherwise VM 

Re: RFR JDK-8234049: Implementation of Memory Access API (Incubator)

[Remi Forax]

Hi Maurizio,
that's a huge work :)

So as a guy discovering the API,
i've not taken a deep look to the implementation because i've noob questions.

The first sentence of the overview of GroupLayout should say that there are two 
types of GroupLayout struct and union instead of talking about members layouts 
in italic.
Still in GroupLayout, there is a static method "optSize" with no documentation 
and you don"t use the prefix "opt" anywhere else so i think it should be 
dropped.
hasNaturalAlignment() is protected, but you don"t allow user to implement their 
own GroupLayout, so it's like the method was package private but visible in the 
javadoc.

MemoryLayout and MemoryLayout.PathElement static methods have names that are ok 
to use in static imports, but it's not explicit written in the javadoc.
People will say that Java is verbose :)

I don't see the point of having MemoryLayouts separated from MemoryLayout.

MemoryAddress should have a method that does raw bytes comparison like there is 
MemoryAddress.copy() so simulate struct comparison.

I don't understand how MemorySegment.acquire() solve the fact that you have two 
threads that can access to the same underlying memory area with unprotected 
access.
When you own a memory segment, an other thread can acquire a new memory segment 
and then both thread can mutate the same memory location ?

MemorySegment.isAccessible() is a very overloaded term, isOwnByCurrentThread() 
is maybe better ?

In the implementation of MemoryScope, please don't use i++ or i-- to change the 
lambda parameter, what you want is i - 1 or i + 1.
And i wander if it's not more efficient to use getAndIncrement and if it's 
negative (overflow), to do a supplementary volatile write to Integer.MIN_VALUE 
before raising the IllegalStateException.

In AddressVarHandleGenerator.iConstInsn(),
ICONST_M1, ICONST_0, etc are subsequent bytecodes so you can group all the case 
to: mv.visitInsn(ICONST_0 + i);

regards,
Rémi

- Mail original -
> De: "Maurizio Cimadamore" 
> À: "core-libs-dev" , "hotspot-dev" 
> 
> Envoyé: Jeudi 5 Décembre 2019 22:04:55
> Objet: RFR JDK-8234049: Implementation of Memory Access API (Incubator)

> Hi,
> as part of the effort to upstream the changes related to JEP 370
> (foreign memory access API) [1], I'd like to ask for a code review for
> the corresponding core-libs and hotspot changes:
> 
> http://cr.openjdk.java.net/~mcimadamore/panama/8234049/
> 
> A javadoc for the memory access API is also available here:
> 
> http://cr.openjdk.java.net/~mcimadamore/panama/memaccess_javadoc/jdk/incubator/foreign/package-summary.html
> 
> Note: the patch passes tier1, tier2 and tier3 testing (**)
> 
> 
> Here is a brief summary of the changes in java.base and hotspot (the
> remaining new files are implementation classes and tests for the new API):
> 
> * ciField.cpp - this one is to trust final fields in the foreign memory
> access implementation (otherwise VM doesn't trust memory segment bounds)
> 
> * Modules.gmk - these changes are needed to require that the incubating
> module is loaded by the boot loader (otherwise the above changes are
> useless)
> 
> * library_call.cpp - this one is a JIT compiler change to treat
> Thread.currentThread() as a well-known constant - which helps a lot in
> the confinement checks (thanks Vlad!)
> 
> * various Buffer-related changes; these changes are needed because the
> memory access API allows a memory segment to be projected into a byte
> buffer, for interop reasons. As such, we need to insert a liveness check
> in the various get/put methods. Previously we had an implementation
> strategy where a BB was 'decorated' by a subclass called ScopedBuffer -
> but doing so required some changes to the BB API (e.g. making certain
> methods non-final, so that we could decorate them). Here I use an
> approach (which I have discussed with Alan) which doesn't require any
> public API  changes, but needs to add a 'segment' field in Buffer - and
> then have constructors which keep track of this extra parameter.
> 
> * FileChannel changes - these changes are required so that we can reuse
> the Unmapper class from the MemorySegment implementation, to
> deterministically deallocate a mapped memory segment. This should be a
> 'straight' refactoring, no change in behavior should occur here. Please
> double check.
> 
> * VarHandles - this class now provides a factory to create memory access
> VarHandle - this is a bit tricky, since VarHandle cannot really be
> implemented outside java.base (e.g. VarForm is not public). So we do the
> usual trick where we define a bunch of proxy interfaces (see
> jdk/internal/access/foreign) have the classes in java.base refer to
> these - and then have the implementation classes of the memory access
> API implement these interfaces.
> 
> * JavaNIOAccess, JavaLangInvokeAccess - because of the above, we need to
> provide access to otherwise hidden functionalities - e.g. creating a new
> scoped buffer, or retrieving the 

Re: RFR JDK-8234049: Implementation of Memory Access API (Incubator)

[Maurizio Cimadamore]

On 05/12/2019 22:39, Vladimir Ivanov wrote:

Awesome work, Maurizio!

Regarding hotspot changes:

* ciField.cpp - this one is to trust final fields in the foreign 
memory access implementation (otherwise VM doesn't trust memory 
segment bounds)


New packages aren't part of java.base. Considering the implementation 
resides in an incubator module, is it enough to consider them as 
trusted and well-known to the JVM?
This gives same performance as with -XX:+TrustFinalNonStaticFields - if 
we remove these changes, then memory segment bounds are never inlined. 
I'm happy to change this if you have other suggestions on how to get 
there, of course (I can run some benchmarks w/ and w/o and post some 
numbers if that helps).


* library_call.cpp - this one is a JIT compiler change to treat 
Thread.currentThread() as a well-known constant - which helps a lot 
in the confinement checks (thanks Vlad!)


FTR it is tracked by JDK-8235143 [1] and the patch is under review [2].


Should I remove this from the patch then?

Thanks
Maurizio



Best regards,
Vladimir Ivanov

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

[2] 
https://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/2019-December/036295.html


* various Buffer-related changes; these changes are needed because 
the memory access API allows a memory segment to be projected into a 
byte buffer, for interop reasons. As such, we need to insert a 
liveness check in the various get/put methods. Previously we had an 
implementation strategy where a BB was 'decorated' by a subclass 
called ScopedBuffer - but doing so required some changes to the BB 
API (e.g. making certain methods non-final, so that we could decorate 
them). Here I use an approach (which I have discussed with Alan) 
which doesn't require any public API  changes, but needs to add a 
'segment' field in Buffer - and then have constructors which keep 
track of this extra parameter.


* FileChannel changes - these changes are required so that we can 
reuse the Unmapper class from the MemorySegment implementation, to 
deterministically deallocate a mapped memory segment. This should be 
a 'straight' refactoring, no change in behavior should occur here. 
Please double check.


* VarHandles - this class now provides a factory to create memory 
access VarHandle - this is a bit tricky, since VarHandle cannot 
really be implemented outside java.base (e.g. VarForm is not public). 
So we do the usual trick where we define a bunch of proxy interfaces 
(see jdk/internal/access/foreign) have the classes in java.base refer 
to these - and then have the implementation classes of the memory 
access API implement these interfaces.


* JavaNIOAccess, JavaLangInvokeAccess - because of the above, we need 
to provide access to otherwise hidden functionalities - e.g. creating 
a new scoped buffer, or retrieving the properties of a memory access 
handle (e.g. offset, stride etc.), so that we can implement the 
memory access API in its own separate module


* GensrcVarHandles.gmk - these changes are needed to enable the 
generation of the new memory address var handle implementations; 
there's an helper class per carrier (e.g. 
VarHandleMemoryAddressAsBytes, ...). At runtime, when a memory access 
var handle is needed, we dynamically spin a new VH implementation 
which makes use of the right carrier. We need to spin because the VH 
can have a variable number of access coordinates (e.g. depending on 
the dimensions of the array to be accessed). But, under the hood, all 
the generated implementation will be using the same helper class.


* tests - we've tried to add fairly robust tests, often checking all 
possible permutations of carriers/dimensions etc. Because of that, 
the tests might not be the easiest to look at, but they have proven 
to be pretty effective at shaking out issues.


I think that covers the main aspects of the implementation and where 
it differs from vanilla JDK.


P.S.

In the CSR review [2], Joe raised a fair point - which is 
MemoryAddress has both:


offset(long) --> move address of given offset
offset() --> return the offset of this address in its owning segment

And this was considered suboptimal, given both methods use the same 
name but do something quite different (one is an accessor, another is 
a 'wither'). one obvious option is to rename the first to 
'withOffset'. But I think that would lead to verbose code (since that 
is a very common operation). Other options are to:


* rename offset(long) to move(long), advance(long), or something else
* drop offset() - but then add an overload of MemorySegment::asSlice 
which takes an address instead of a plain long offset


I'll leave the choice to the reviewers :-)



Finally, I'd like to thank Mark, Brian, John, Alan, Paul, Vlad, 
Stuart, Roger, Joe and the Panama team for the feedback provided so 
far, which helped to get the API in the shape it is today.


Cheers
Maurizio

(**) There is one failure, for 

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

[David Holmes]

On 6/12/2019 2:06 am, Daniel D. Daugherty wrote:

On 12/5/19 8:00 AM, David Holmes wrote:

Hi Christoph,

On 5/12/2019 9:55 pm, Langer, Christoph wrote:

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?


Avoiding the include does seem to be the way to go. That seems so 
obvious now.


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. Further, does:


src/java.base/unix/native/libjava/canonicalize_md.c

need to include jdk_util.h? I think not.

+/* 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/


It should be:

s/complicate/complicated/


Thanks Dan - fat fingers :) I need a keyboard with bigger spaces between 
keys


David



Dan



src/java.base/windows/native/libjava/io_util_md.c

should now be unchanged, but you've left in the copyright update.

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

Thanks,
David


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 prepar

RFR: JDK-8233270: Add support to jtreg helpers to unpack packages

[Alexey Semenyuk]

Please review  fixes for [1] and [2]. Both target jpackage tool.

The webrev is at [3].

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

[2] https://bugs.openjdk.java.net/browse/JDK-8230933

[3] http://cr.openjdk.java.net/~asemenyuk/8233270/webrev.00/

Re: RFR JDK-8234049: Implementation of Memory Access API (Incubator)

[Vladimir Ivanov]

Awesome work, Maurizio!

Regarding hotspot changes:

* ciField.cpp - this one is to trust final fields in the foreign memory 
access implementation (otherwise VM doesn't trust memory segment bounds)


New packages aren't part of java.base. Considering the implementation 
resides in an incubator module, is it enough to consider them as trusted 
and well-known to the JVM?


* library_call.cpp - this one is a JIT compiler change to treat 
Thread.currentThread() as a well-known constant - which helps a lot in 
the confinement checks (thanks Vlad!)


FTR it is tracked by JDK-8235143 [1] and the patch is under review [2].

Best regards,
Vladimir Ivanov

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

[2] 
https://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/2019-December/036295.html


* various Buffer-related changes; these changes are needed because the 
memory access API allows a memory segment to be projected into a byte 
buffer, for interop reasons. As such, we need to insert a liveness check 
in the various get/put methods. Previously we had an implementation 
strategy where a BB was 'decorated' by a subclass called ScopedBuffer - 
but doing so required some changes to the BB API (e.g. making certain 
methods non-final, so that we could decorate them). Here I use an 
approach (which I have discussed with Alan) which doesn't require any 
public API  changes, but needs to add a 'segment' field in Buffer - and 
then have constructors which keep track of this extra parameter.


* FileChannel changes - these changes are required so that we can reuse 
the Unmapper class from the MemorySegment implementation, to 
deterministically deallocate a mapped memory segment. This should be a 
'straight' refactoring, no change in behavior should occur here. Please 
double check.


* VarHandles - this class now provides a factory to create memory access 
VarHandle - this is a bit tricky, since VarHandle cannot really be 
implemented outside java.base (e.g. VarForm is not public). So we do the 
usual trick where we define a bunch of proxy interfaces (see 
jdk/internal/access/foreign) have the classes in java.base refer to 
these - and then have the implementation classes of the memory access 
API implement these interfaces.


* JavaNIOAccess, JavaLangInvokeAccess - because of the above, we need to 
provide access to otherwise hidden functionalities - e.g. creating a new 
scoped buffer, or retrieving the properties of a memory access handle 
(e.g. offset, stride etc.), so that we can implement the memory access 
API in its own separate module


* GensrcVarHandles.gmk - these changes are needed to enable the 
generation of the new memory address var handle implementations; there's 
an helper class per carrier (e.g. VarHandleMemoryAddressAsBytes, ...). 
At runtime, when a memory access var handle is needed, we dynamically 
spin a new VH implementation which makes use of the right carrier. We 
need to spin because the VH can have a variable number of access 
coordinates (e.g. depending on the dimensions of the array to be 
accessed). But, under the hood, all the generated implementation will be 
using the same helper class.


* tests - we've tried to add fairly robust tests, often checking all 
possible permutations of carriers/dimensions etc. Because of that, the 
tests might not be the easiest to look at, but they have proven to be 
pretty effective at shaking out issues.


I think that covers the main aspects of the implementation and where it 
differs from vanilla JDK.


P.S.

In the CSR review [2], Joe raised a fair point - which is MemoryAddress 
has both:


offset(long) --> move address of given offset
offset() --> return the offset of this address in its owning segment

And this was considered suboptimal, given both methods use the same name 
but do something quite different (one is an accessor, another is a 
'wither'). one obvious option is to rename the first to 'withOffset'. 
But I think that would lead to verbose code (since that is a very common 
operation). Other options are to:


* rename offset(long) to move(long), advance(long), or something else
* drop offset() - but then add an overload of MemorySegment::asSlice 
which takes an address instead of a plain long offset


I'll leave the choice to the reviewers :-)



Finally, I'd like to thank Mark, Brian, John, Alan, Paul, Vlad, Stuart, 
Roger, Joe and the Panama team for the feedback provided so far, which 
helped to get the API in the shape it is today.


Cheers
Maurizio

(**) There is one failure, for "java/util/TimeZone/Bug6329116.java" - 
but that is unrelated to this patch, and it's a known failing test.


[1] - https://openjdk.java.net/jeps/370
[2] - https://bugs.openjdk.java.net/browse/JDK-8234050

Re: JDK 14 RFR of JDK-8235369: "Class.toGenericString need to be updated for records"

[Chris Hegarty]

LGTM.

-Chris

> On 5 Dec 2019, at 18:08, Joe Darcy  wrote:
> 
> Hello,
> 
> Please review this small cleanup to records support in java.lang.Class:
> 
> JDK-8235369: "Class.toGenericString need to be updated for records"
> CSR: https://bugs.openjdk.java.net/browse/JDK-8235428
> webrev: http://cr.openjdk.java.net/~darcy/8235369.0/
> 
> Patch below; thanks,
> 
> -Joe
> 
> diff -r 666fa504b60c src/java.base/share/classes/java/lang/Class.java
> --- a/src/java.base/share/classes/java/lang/Class.javaWed Dec 04 14:55:15 
> 2019 -0800
> +++ b/src/java.base/share/classes/java/lang/Class.javaThu Dec 05 10:04:26 
> 2019 -0800
> @@ -207,8 +207,8 @@
>   *
>   * The string is formatted as a list of type modifiers, if any,
>   * followed by the kind of type (empty string for primitive types
> - * and {@code class}, {@code enum}, {@code interface}, or
> - * {@code interface}, as appropriate), followed
> + * and {@code class}, {@code enum}, {@code interface},
> + * {@code interface}, or {@code record} as 
> appropriate), followed
>   * by the type's name, followed by an angle-bracketed
>   * comma-separated list of the type's type parameters, if any,
>   * including informative bounds on the type parameters, if any.
> @@ -234,6 +234,7 @@
>   *
>   * @since 1.8
>   */
> +@SuppressWarnings("preview")
>  public String toGenericString() {
>  if (isPrimitive()) {
>  return toString();
> @@ -264,6 +265,8 @@
>  } else {
>  if (isEnum())
>  sb.append("enum");
> +else if (isRecord())
> +sb.append("record");
>  else
>  sb.append("class");
>  }
> 
> diff -r 666fa504b60c 
> test/jdk/java/lang/reflect/records/RecordReflectionTest.java
> --- a/test/jdk/java/lang/reflect/records/RecordReflectionTest.java Wed Dec 04 
> 14:55:15 2019 -0800
> +++ b/test/jdk/java/lang/reflect/records/RecordReflectionTest.java Thu Dec 05 
> 10:05:01 2019 -0800
> @@ -23,6 +23,7 @@
> 
>  /*
>   * @test
> + * @bug 8235369
>   * @summary reflection test for records
>   * @compile --enable-preview -source ${jdk.version} RecordReflectionTest.java
>   * @run testng/othervm --enable-preview RecordReflectionTest
> @@ -51,8 +52,11 @@
>  public void testIsRecord() {
>  assertFalse(NoRecord.class.isRecord());
> 
> -for (Class c : List.of(R1.class, R2.class, R3.class))
> -assertTrue(c.isRecord());
> +for (Class c : List.of(R1.class, R2.class, R3.class)) {
> +String message = c.toGenericString();
> +assertTrue(c.isRecord(), message);
> +assertTrue(message.contains("record") , message);
> +}
>  }
> 
>  public void testGetComponentsNoRecord() {
> 
> 

RFR JDK-8234049: Implementation of Memory Access API (Incubator)

[Maurizio Cimadamore]

Hi,
as part of the effort to upstream the changes related to JEP 370 
(foreign memory access API) [1], I'd like to ask for a code review for 
the corresponding core-libs and hotspot changes:


http://cr.openjdk.java.net/~mcimadamore/panama/8234049/

A javadoc for the memory access API is also available here:

http://cr.openjdk.java.net/~mcimadamore/panama/memaccess_javadoc/jdk/incubator/foreign/package-summary.html

Note: the patch passes tier1, tier2 and tier3 testing (**)


Here is a brief summary of the changes in java.base and hotspot (the 
remaining new files are implementation classes and tests for the new API):


* ciField.cpp - this one is to trust final fields in the foreign memory 
access implementation (otherwise VM doesn't trust memory segment bounds)


* Modules.gmk - these changes are needed to require that the incubating 
module is loaded by the boot loader (otherwise the above changes are 
useless)


* library_call.cpp - this one is a JIT compiler change to treat 
Thread.currentThread() as a well-known constant - which helps a lot in 
the confinement checks (thanks Vlad!)


* various Buffer-related changes; these changes are needed because the 
memory access API allows a memory segment to be projected into a byte 
buffer, for interop reasons. As such, we need to insert a liveness check 
in the various get/put methods. Previously we had an implementation 
strategy where a BB was 'decorated' by a subclass called ScopedBuffer - 
but doing so required some changes to the BB API (e.g. making certain 
methods non-final, so that we could decorate them). Here I use an 
approach (which I have discussed with Alan) which doesn't require any 
public API  changes, but needs to add a 'segment' field in Buffer - and 
then have constructors which keep track of this extra parameter.


* FileChannel changes - these changes are required so that we can reuse 
the Unmapper class from the MemorySegment implementation, to 
deterministically deallocate a mapped memory segment. This should be a 
'straight' refactoring, no change in behavior should occur here. Please 
double check.


* VarHandles - this class now provides a factory to create memory access 
VarHandle - this is a bit tricky, since VarHandle cannot really be 
implemented outside java.base (e.g. VarForm is not public). So we do the 
usual trick where we define a bunch of proxy interfaces (see 
jdk/internal/access/foreign) have the classes in java.base refer to 
these - and then have the implementation classes of the memory access 
API implement these interfaces.


* JavaNIOAccess, JavaLangInvokeAccess - because of the above, we need to 
provide access to otherwise hidden functionalities - e.g. creating a new 
scoped buffer, or retrieving the properties of a memory access handle 
(e.g. offset, stride etc.), so that we can implement the memory access 
API in its own separate module


* GensrcVarHandles.gmk - these changes are needed to enable the 
generation of the new memory address var handle implementations; there's 
an helper class per carrier (e.g. VarHandleMemoryAddressAsBytes, ...). 
At runtime, when a memory access var handle is needed, we dynamically 
spin a new VH implementation which makes use of the right carrier. We 
need to spin because the VH can have a variable number of access 
coordinates (e.g. depending on the dimensions of the array to be 
accessed). But, under the hood, all the generated implementation will be 
using the same helper class.


* tests - we've tried to add fairly robust tests, often checking all 
possible permutations of carriers/dimensions etc. Because of that, the 
tests might not be the easiest to look at, but they have proven to be 
pretty effective at shaking out issues.


I think that covers the main aspects of the implementation and where it 
differs from vanilla JDK.


P.S.

In the CSR review [2], Joe raised a fair point - which is MemoryAddress 
has both:


offset(long) --> move address of given offset
offset() --> return the offset of this address in its owning segment

And this was considered suboptimal, given both methods use the same name 
but do something quite different (one is an accessor, another is a 
'wither'). one obvious option is to rename the first to 'withOffset'. 
But I think that would lead to verbose code (since that is a very common 
operation). Other options are to:


* rename offset(long) to move(long), advance(long), or something else
* drop offset() - but then add an overload of MemorySegment::asSlice 
which takes an address instead of a plain long offset


I'll leave the choice to the reviewers :-)



Finally, I'd like to thank Mark, Brian, John, Alan, Paul, Vlad, Stuart, 
Roger, Joe and the Panama team for the feedback provided so far, which 
helped to get the API in the shape it is today.


Cheers
Maurizio

(**) There is one failure, for "java/util/TimeZone/Bug6329116.java" - 
but that is unrelated to this patch, and it's a known failing test.


[1] - https://openjdk.java.net/jeps/370

Re: [14] RFR: 8222756: Plural support in CompactNumberFormat

[Joe Wang]

+1, looks good!


Best regards,
Joe


On 12/5/19 12:13 PM, Roger Riggs wrote:

Hi Naoto,

Thanks for the updates.

Looks fine.

Roger


On 12/5/19 1:49 PM, naoto.s...@oracle.com wrote:

Thanks, Joe and Roger, for the reviews. Here is the updated webrev:

http://cr.openjdk.java.net/~naoto/8222756/webrev.02/

These are the changes since v.01:

CompactNumberFormat.java:

- Reflected the CSR changes (thank you, JoeD, for the quick 
turnaround!), and misc typos in the spec.


- Added length limitation to the "pluralRules" argument in the new 
constructor. Throws IllegalArgumentException if the input is too long 
(>2,048 chars).


- Added validation to plural rules, and throws 
IllegalArgumentException if the given rules' syntax is incorrect.


- Consolidated the same pattern to get the affixes into a utility 
method (getAffix())


- Directly find NaN and Infinity in the number string, parse the 
number with regex otherwise.


TestEquality.java: Tidied the test array up.

CLDRConverter.java: Added String::trim so that the generated 
PluralRules.java will not have extra spaces.


TestPlurals.java: Added test cases for invalid cases, i.e., invalid 
syntax and length limit exceeding rules in the constructor.


Not addressed:

- ResourceBundleGenerator.java exists in the webrev as it does have a 
modification (only seen in "patch" link, as the mod is only the 
indentation)


- Using "==" for double values: As Roger mentioned, it is in fact 
comparing integers (the only reason to use double is to deal with NaN 
and Infinity)


- Possible performance improvement: Could be addressed later if it is 
regarded as an issue. But since the effective plural rules are 
simple/short enough, I would not expect it as a problem.


Naoto



On 11/26/19 1:35 PM, naoto.s...@oracle.com wrote:

I modified CompactNumberFormat.java to simplify the syntax parsing:

https://cr.openjdk.java.net/~naoto/8222756/webrev.01/

Please review this webrev instead.

Naoto

On 11/25/19 1:16 PM, naoto.s...@oracle.com wrote:

Hello,

CompactNumberFormat has been added since JDK 12 to support compact 
number formatting, such as 10_000 being formatted as "10K." [1] It 
works fine for English, however, not for other languages that take 
plural forms in formatted number prefixes/suffixes. In order to fix 
this, I filed the following CSR to extend the current 
CompactNumberFormat spec:


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

It basically accommodates the plural prefix/suffix forms into the 
existing compact patterns array, so that the existing compact 
number format works compatibly. The plural rules are solely based 
on the CLDR's plural language rules [2]


Here is the implementation of the CSR:

https://cr.openjdk.java.net/~naoto/8222756/webrev.00/

Please review the CSR as well as its implementation.

Naoto


[1] https://bugs.openjdk.java.net/browse/JDK-8177552
[2] 
https://unicode.org/reports/tr35/tr35-numbers.html#Language_Plural_Rules 

Re: JDK 14 RFR of JDK-8235369: "Class.toGenericString need to be updated for records"

[Mandy Chung]

+1

Mandy

On 12/5/19 10:08 AM, Joe Darcy wrote:

Hello,

Please review this small cleanup to records support in java.lang.Class:

    JDK-8235369: "Class.toGenericString need to be updated for records"
    CSR: https://bugs.openjdk.java.net/browse/JDK-8235428
    webrev: http://cr.openjdk.java.net/~darcy/8235369.0/

Patch below; thanks,

-Joe

diff -r 666fa504b60c src/java.base/share/classes/java/lang/Class.java
--- a/src/java.base/share/classes/java/lang/Class.java    Wed Dec 04 
14:55:15 2019 -0800
+++ b/src/java.base/share/classes/java/lang/Class.java    Thu Dec 05 
10:04:26 2019 -0800

@@ -207,8 +207,8 @@
  *
  * The string is formatted as a list of type modifiers, if any,
  * followed by the kind of type (empty string for primitive types
- * and {@code class}, {@code enum}, {@code interface}, or
- * {@code interface}, as appropriate), followed
+ * and {@code class}, {@code enum}, {@code interface},
+ * {@code interface}, or {@code record} as 
appropriate), followed

  * by the type's name, followed by an angle-bracketed
  * comma-separated list of the type's type parameters, if any,
  * including informative bounds on the type parameters, if any.
@@ -234,6 +234,7 @@
  *
  * @since 1.8
  */
+    @SuppressWarnings("preview")
 public String toGenericString() {
 if (isPrimitive()) {
 return toString();
@@ -264,6 +265,8 @@
 } else {
 if (isEnum())
 sb.append("enum");
+    else if (isRecord())
+    sb.append("record");
 else
 sb.append("class");
 }

diff -r 666fa504b60c 
test/jdk/java/lang/reflect/records/RecordReflectionTest.java
--- a/test/jdk/java/lang/reflect/records/RecordReflectionTest.java Wed 
Dec 04 14:55:15 2019 -0800
+++ b/test/jdk/java/lang/reflect/records/RecordReflectionTest.java Thu 
Dec 05 10:05:01 2019 -0800

@@ -23,6 +23,7 @@

 /*
  * @test
+ * @bug 8235369
  * @summary reflection test for records
  * @compile --enable-preview -source ${jdk.version} 
RecordReflectionTest.java

  * @run testng/othervm --enable-preview RecordReflectionTest
@@ -51,8 +52,11 @@
 public void testIsRecord() {
 assertFalse(NoRecord.class.isRecord());

-    for (Class c : List.of(R1.class, R2.class, R3.class))
-    assertTrue(c.isRecord());
+    for (Class c : List.of(R1.class, R2.class, R3.class)) {
+    String message = c.toGenericString();
+    assertTrue(c.isRecord(), message);
+    assertTrue(message.contains("record") , message);
+    }
 }

 public void testGetComponentsNoRecord() {

Re: JDK 14 RFR of JDK-8235369: "Class.toGenericString need to be updated for records"

[Vicente Romero]

looks good,
Vicente

On 12/5/19 1:08 PM, Joe Darcy wrote:

Hello,

Please review this small cleanup to records support in java.lang.Class:

    JDK-8235369: "Class.toGenericString need to be updated for records"
    CSR: https://bugs.openjdk.java.net/browse/JDK-8235428
    webrev: http://cr.openjdk.java.net/~darcy/8235369.0/

Patch below; thanks,

-Joe

diff -r 666fa504b60c src/java.base/share/classes/java/lang/Class.java
--- a/src/java.base/share/classes/java/lang/Class.java    Wed Dec 04 
14:55:15 2019 -0800
+++ b/src/java.base/share/classes/java/lang/Class.java    Thu Dec 05 
10:04:26 2019 -0800

@@ -207,8 +207,8 @@
  *
  * The string is formatted as a list of type modifiers, if any,
  * followed by the kind of type (empty string for primitive types
- * and {@code class}, {@code enum}, {@code interface}, or
- * {@code interface}, as appropriate), followed
+ * and {@code class}, {@code enum}, {@code interface},
+ * {@code interface}, or {@code record} as 
appropriate), followed

  * by the type's name, followed by an angle-bracketed
  * comma-separated list of the type's type parameters, if any,
  * including informative bounds on the type parameters, if any.
@@ -234,6 +234,7 @@
  *
  * @since 1.8
  */
+    @SuppressWarnings("preview")
 public String toGenericString() {
 if (isPrimitive()) {
 return toString();
@@ -264,6 +265,8 @@
 } else {
 if (isEnum())
 sb.append("enum");
+    else if (isRecord())
+    sb.append("record");
 else
 sb.append("class");
 }

diff -r 666fa504b60c 
test/jdk/java/lang/reflect/records/RecordReflectionTest.java
--- a/test/jdk/java/lang/reflect/records/RecordReflectionTest.java Wed 
Dec 04 14:55:15 2019 -0800
+++ b/test/jdk/java/lang/reflect/records/RecordReflectionTest.java Thu 
Dec 05 10:05:01 2019 -0800

@@ -23,6 +23,7 @@

 /*
  * @test
+ * @bug 8235369
  * @summary reflection test for records
  * @compile --enable-preview -source ${jdk.version} 
RecordReflectionTest.java

  * @run testng/othervm --enable-preview RecordReflectionTest
@@ -51,8 +52,11 @@
 public void testIsRecord() {
 assertFalse(NoRecord.class.isRecord());

-    for (Class c : List.of(R1.class, R2.class, R3.class))
-    assertTrue(c.isRecord());
+    for (Class c : List.of(R1.class, R2.class, R3.class)) {
+    String message = c.toGenericString();
+    assertTrue(c.isRecord(), message);
+    assertTrue(message.contains("record") , message);
+    }
 }

 public void testGetComponentsNoRecord() {

Re: RFR: JEP 367: Remove the Pack200 Tools and API

[Mandy Chung]

On 12/5/19 12:41 AM, Alan Bateman wrote:

On 05/12/2019 08:16, Henry Jen wrote:

Hi,

Updated webrev[1] reflect comments since last webrev. Vicente had 
done all the heavy-lifting and hand over to me to finish up.


Changes to symbols is reverted, as we expect that only need to be 
updated in next release by running make/scripts/generate-symbol-data.sh.


The jar files are confusing in the webrev, but those files are 
removed. The whole test/jdk/tools/pack200 is removed.


Cheers,
Henry

[1] http://cr.openjdk.java.net/~henryjen/jdk14/8234542/0/webrev/

The update webrev looks okay to me, except this part of the comment in 
flags-cflags.m4


"Now that unpack200 has been removed we should consider setting it for 
windows too. but this could be done as a follow-up effort. It could be 
that other other clients are relying on the current configuration for 
windows".


I think it would be best to create an infrastructure/build issue and 
leave most of this  confusing comment out.




I also think keeping flags-cflags.m4 as is and file a new build issue as 
a follow-up would be better.


Otherwise, this updated webrev looks okay to me.

Mandy

Re: [14] RFR: 8222756: Plural support in CompactNumberFormat

[Roger Riggs]

Hi Naoto,

Thanks for the updates.

Looks fine.

Roger


On 12/5/19 1:49 PM, naoto.s...@oracle.com wrote:

Thanks, Joe and Roger, for the reviews. Here is the updated webrev:

http://cr.openjdk.java.net/~naoto/8222756/webrev.02/

These are the changes since v.01:

CompactNumberFormat.java:

- Reflected the CSR changes (thank you, JoeD, for the quick 
turnaround!), and misc typos in the spec.


- Added length limitation to the "pluralRules" argument in the new 
constructor. Throws IllegalArgumentException if the input is too long 
(>2,048 chars).


- Added validation to plural rules, and throws 
IllegalArgumentException if the given rules' syntax is incorrect.


- Consolidated the same pattern to get the affixes into a utility 
method (getAffix())


- Directly find NaN and Infinity in the number string, parse the 
number with regex otherwise.


TestEquality.java: Tidied the test array up.

CLDRConverter.java: Added String::trim so that the generated 
PluralRules.java will not have extra spaces.


TestPlurals.java: Added test cases for invalid cases, i.e., invalid 
syntax and length limit exceeding rules in the constructor.


Not addressed:

- ResourceBundleGenerator.java exists in the webrev as it does have a 
modification (only seen in "patch" link, as the mod is only the 
indentation)


- Using "==" for double values: As Roger mentioned, it is in fact 
comparing integers (the only reason to use double is to deal with NaN 
and Infinity)


- Possible performance improvement: Could be addressed later if it is 
regarded as an issue. But since the effective plural rules are 
simple/short enough, I would not expect it as a problem.


Naoto



On 11/26/19 1:35 PM, naoto.s...@oracle.com wrote:

I modified CompactNumberFormat.java to simplify the syntax parsing:

https://cr.openjdk.java.net/~naoto/8222756/webrev.01/

Please review this webrev instead.

Naoto

On 11/25/19 1:16 PM, naoto.s...@oracle.com wrote:

Hello,

CompactNumberFormat has been added since JDK 12 to support compact 
number formatting, such as 10_000 being formatted as "10K." [1] It 
works fine for English, however, not for other languages that take 
plural forms in formatted number prefixes/suffixes. In order to fix 
this, I filed the following CSR to extend the current 
CompactNumberFormat spec:


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

It basically accommodates the plural prefix/suffix forms into the 
existing compact patterns array, so that the existing compact number 
format works compatibly. The plural rules are solely based on the 
CLDR's plural language rules [2]


Here is the implementation of the CSR:

https://cr.openjdk.java.net/~naoto/8222756/webrev.00/

Please review the CSR as well as its implementation.

Naoto


[1] https://bugs.openjdk.java.net/browse/JDK-8177552
[2] 
https://unicode.org/reports/tr35/tr35-numbers.html#Language_Plural_Rules 

Re: RFR 8235359: Simplify method Class.getRecordComponents()

[Harold Seigel]

Thanks Mandy!

Harold

On 12/5/2019 1:59 PM, Mandy Chung wrote:

Looks good.

Mandy

On 12/5/19 6:36 AM, Harold Seigel wrote:

Hi,

Please review this small change to simplify 
Class.getRecordComponents() by changing the return type of 
getRecordComponents0() to RecordComponent[].


Open Webrev: 
http://cr.openjdk.java.net/~hseigel/bug_8235359/webrev/index.html


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

The fix was regression tested by running Mach5 tiers 1 and 2 tests 
and builds on Linux-x64, Solaris, Windows, and Mac OS X, by running 
Mach5 tiers 3-5 tests on Linux-x64, and JCK lang and VM tests on 
Linux-x64.


Thanks, Harold

Re: RFR 8235359: Simplify method Class.getRecordComponents()

[Mandy Chung]

Looks good.

Mandy

On 12/5/19 6:36 AM, Harold Seigel wrote:

Hi,

Please review this small change to simplify 
Class.getRecordComponents() by changing the return type of 
getRecordComponents0() to RecordComponent[].


Open Webrev: 
http://cr.openjdk.java.net/~hseigel/bug_8235359/webrev/index.html


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

The fix was regression tested by running Mach5 tiers 1 and 2 tests and 
builds on Linux-x64, Solaris, Windows, and Mac OS X, by running Mach5 
tiers 3-5 tests on Linux-x64, and JCK lang and VM tests on Linux-x64.


Thanks, Harold

Re: [14] RFR: 8222756: Plural support in CompactNumberFormat

[naoto . sato]

Thanks, Joe and Roger, for the reviews. Here is the updated webrev:

http://cr.openjdk.java.net/~naoto/8222756/webrev.02/

These are the changes since v.01:

CompactNumberFormat.java:

- Reflected the CSR changes (thank you, JoeD, for the quick 
turnaround!), and misc typos in the spec.


- Added length limitation to the "pluralRules" argument in the new 
constructor. Throws IllegalArgumentException if the input is too long 
(>2,048 chars).


- Added validation to plural rules, and throws IllegalArgumentException 
if the given rules' syntax is incorrect.


- Consolidated the same pattern to get the affixes into a utility method 
(getAffix())


- Directly find NaN and Infinity in the number string, parse the number 
with regex otherwise.


TestEquality.java: Tidied the test array up.

CLDRConverter.java: Added String::trim so that the generated 
PluralRules.java will not have extra spaces.


TestPlurals.java: Added test cases for invalid cases, i.e., invalid 
syntax and length limit exceeding rules in the constructor.


Not addressed:

- ResourceBundleGenerator.java exists in the webrev as it does have a 
modification (only seen in "patch" link, as the mod is only the indentation)


- Using "==" for double values: As Roger mentioned, it is in fact 
comparing integers (the only reason to use double is to deal with NaN 
and Infinity)


- Possible performance improvement: Could be addressed later if it is 
regarded as an issue. But since the effective plural rules are 
simple/short enough, I would not expect it as a problem.


Naoto



On 11/26/19 1:35 PM, naoto.s...@oracle.com wrote:

I modified CompactNumberFormat.java to simplify the syntax parsing:

https://cr.openjdk.java.net/~naoto/8222756/webrev.01/

Please review this webrev instead.

Naoto

On 11/25/19 1:16 PM, naoto.s...@oracle.com wrote:

Hello,

CompactNumberFormat has been added since JDK 12 to support compact 
number formatting, such as 10_000 being formatted as "10K." [1] It 
works fine for English, however, not for other languages that take 
plural forms in formatted number prefixes/suffixes. In order to fix 
this, I filed the following CSR to extend the current 
CompactNumberFormat spec:


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

It basically accommodates the plural prefix/suffix forms into the 
existing compact patterns array, so that the existing compact number 
format works compatibly. The plural rules are solely based on the 
CLDR's plural language rules [2]


Here is the implementation of the CSR:

https://cr.openjdk.java.net/~naoto/8222756/webrev.00/

Please review the CSR as well as its implementation.

Naoto


[1] https://bugs.openjdk.java.net/browse/JDK-8177552
[2] 
https://unicode.org/reports/tr35/tr35-numbers.html#Language_Plural_Rules

Re: RFR 8235359: Simplify method Class.getRecordComponents()

[Harold Seigel]

Thanks Vicente!

Harold

On 12/5/2019 11:44 AM, Vicente Romero wrote:

looks good,
Vicente

On 12/5/19 9:36 AM, Harold Seigel wrote:

Hi,

Please review this small change to simplify 
Class.getRecordComponents() by changing the return type of 
getRecordComponents0() to RecordComponent[].


Open Webrev: 
http://cr.openjdk.java.net/~hseigel/bug_8235359/webrev/index.html


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

The fix was regression tested by running Mach5 tiers 1 and 2 tests 
and builds on Linux-x64, Solaris, Windows, and Mac OS X, by running 
Mach5 tiers 3-5 tests on Linux-x64, and JCK lang and VM tests on 
Linux-x64.


Thanks, Harold

JDK 14 RFR of JDK-8235369: "Class.toGenericString need to be updated for records"

[Joe Darcy]

Hello,

Please review this small cleanup to records support in java.lang.Class:

    JDK-8235369: "Class.toGenericString need to be updated for records"
    CSR: https://bugs.openjdk.java.net/browse/JDK-8235428
    webrev: http://cr.openjdk.java.net/~darcy/8235369.0/

Patch below; thanks,

-Joe

diff -r 666fa504b60c src/java.base/share/classes/java/lang/Class.java
--- a/src/java.base/share/classes/java/lang/Class.java    Wed Dec 04 
14:55:15 2019 -0800
+++ b/src/java.base/share/classes/java/lang/Class.java    Thu Dec 05 
10:04:26 2019 -0800

@@ -207,8 +207,8 @@
  *
  * The string is formatted as a list of type modifiers, if any,
  * followed by the kind of type (empty string for primitive types
- * and {@code class}, {@code enum}, {@code interface}, or
- * {@code interface}, as appropriate), followed
+ * and {@code class}, {@code enum}, {@code interface},
+ * {@code interface}, or {@code record} as 
appropriate), followed

  * by the type's name, followed by an angle-bracketed
  * comma-separated list of the type's type parameters, if any,
  * including informative bounds on the type parameters, if any.
@@ -234,6 +234,7 @@
  *
  * @since 1.8
  */
+    @SuppressWarnings("preview")
 public String toGenericString() {
 if (isPrimitive()) {
 return toString();
@@ -264,6 +265,8 @@
 } else {
 if (isEnum())
 sb.append("enum");
+    else if (isRecord())
+    sb.append("record");
 else
 sb.append("class");
 }

diff -r 666fa504b60c 
test/jdk/java/lang/reflect/records/RecordReflectionTest.java
--- a/test/jdk/java/lang/reflect/records/RecordReflectionTest.java Wed 
Dec 04 14:55:15 2019 -0800
+++ b/test/jdk/java/lang/reflect/records/RecordReflectionTest.java Thu 
Dec 05 10:05:01 2019 -0800

@@ -23,6 +23,7 @@

 /*
  * @test
+ * @bug 8235369
  * @summary reflection test for records
  * @compile --enable-preview -source ${jdk.version} 
RecordReflectionTest.java

  * @run testng/othervm --enable-preview RecordReflectionTest
@@ -51,8 +52,11 @@
 public void testIsRecord() {
 assertFalse(NoRecord.class.isRecord());

-    for (Class c : List.of(R1.class, R2.class, R3.class))
-    assertTrue(c.isRecord());
+    for (Class c : List.of(R1.class, R2.class, R3.class)) {
+    String message = c.toGenericString();
+    assertTrue(c.isRecord(), message);
+    assertTrue(message.contains("record") , message);
+    }
 }

 public void testGetComponentsNoRecord() {

Re: RFR 8235359: Simplify method Class.getRecordComponents()

[Vicente Romero]

looks good,
Vicente

On 12/5/19 9:36 AM, Harold Seigel wrote:

Hi,

Please review this small change to simplify 
Class.getRecordComponents() by changing the return type of 
getRecordComponents0() to RecordComponent[].


Open Webrev: 
http://cr.openjdk.java.net/~hseigel/bug_8235359/webrev/index.html


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

The fix was regression tested by running Mach5 tiers 1 and 2 tests and 
builds on Linux-x64, Solaris, Windows, and Mac OS X, by running Mach5 
tiers 3-5 tests on Linux-x64, and JCK lang and VM tests on Linux-x64.


Thanks, Harold

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

[Daniel D. Daugherty]

On 12/5/19 8:00 AM, David Holmes wrote:

Hi Christoph,

On 5/12/2019 9:55 pm, Langer, Christoph wrote:

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?


Avoiding the include does seem to be the way to go. That seems so 
obvious now.


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. Further, does:


src/java.base/unix/native/libjava/canonicalize_md.c

need to include jdk_util.h? I think not.

+/* 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/


It should be:

s/complicate/complicated/

Dan



src/java.base/windows/native/libjava/io_util_md.c

should now be unchanged, but you've left in the copyright update.

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

Thanks,
David


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'

Re: RFR 8235359: Simplify method Class.getRecordComponents()

[Harold Seigel]

Thanks Lois and Chris!

Harold

On 12/5/2019 9:56 AM, Chris Hegarty wrote:



On 5 Dec 2019, at 14:36, Harold Seigel > wrote:


Hi,

Please review this small change to simplify 
Class.getRecordComponents() by changing the return type of 
getRecordComponents0() to RecordComponent[].


Open Webrev: 
http://cr.openjdk.java.net/~hseigel/bug_8235359/webrev/index.html


LGTM.

-Chris.

P.S. Further changes in this area are being discussed on 
amber-spec-experts - 
https://mail.openjdk.java.net/pipermail/amber-spec-experts/2019-December/001840.html

Re: RFR 8235359: Simplify method Class.getRecordComponents()

[Chris Hegarty]

> On 5 Dec 2019, at 14:36, Harold Seigel  wrote:
> 
> Hi,
> 
> Please review this small change to simplify Class.getRecordComponents() by 
> changing the return type of getRecordComponents0() to RecordComponent[].
> 
> Open Webrev: 
> http://cr.openjdk.java.net/~hseigel/bug_8235359/webrev/index.html 
> 

LGTM.

-Chris.

P.S. Further changes in this area are being discussed on amber-spec-experts - 
https://mail.openjdk.java.net/pipermail/amber-spec-experts/2019-December/001840.html

Re: RFR 8235359: Simplify method Class.getRecordComponents()

[Lois Foltan]

Looks good.
Lois

On 12/5/2019 9:36 AM, Harold Seigel wrote:

Hi,

Please review this small change to simplify 
Class.getRecordComponents() by changing the return type of 
getRecordComponents0() to RecordComponent[].


Open Webrev: 
http://cr.openjdk.java.net/~hseigel/bug_8235359/webrev/index.html


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

The fix was regression tested by running Mach5 tiers 1 and 2 tests and 
builds on Linux-x64, Solaris, Windows, and Mac OS X, by running Mach5 
tiers 3-5 tests on Linux-x64, and JCK lang and VM tests on Linux-x64.


Thanks, Harold

Re: RFR 8235359: Simplify method Class.getRecordComponents()

[Frederic Parain]

Looks good to me.

Fred


> On Dec 5, 2019, at 09:36, Harold Seigel  wrote:
> 
> Hi,
> 
> Please review this small change to simplify Class.getRecordComponents() by 
> changing the return type of getRecordComponents0() to RecordComponent[].
> 
> Open Webrev: http://cr.openjdk.java.net/~hseigel/bug_8235359/webrev/index.html
> 
> JBS Bug: https://bugs.openjdk.java.net/browse/JDK-8235359
> 
> The fix was regression tested by running Mach5 tiers 1 and 2 tests and builds 
> on Linux-x64, Solaris, Windows, and Mac OS X, by running Mach5 tiers 3-5 
> tests on Linux-x64, and JCK lang and VM tests on Linux-x64.
> 
> Thanks, Harold
> 

Re: RFR 8235359: Simplify method Class.getRecordComponents()

[Harold Seigel]

Thanks Fred!

Harold

On 12/5/2019 9:50 AM, Frederic Parain wrote:

Looks good to me.

Fred



On Dec 5, 2019, at 09:36, Harold Seigel  wrote:

Hi,

Please review this small change to simplify Class.getRecordComponents() by 
changing the return type of getRecordComponents0() to RecordComponent[].

Open Webrev: http://cr.openjdk.java.net/~hseigel/bug_8235359/webrev/index.html

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

The fix was regression tested by running Mach5 tiers 1 and 2 tests and builds 
on Linux-x64, Solaris, Windows, and Mac OS X, by running Mach5 tiers 3-5 tests 
on Linux-x64, and JCK lang and VM tests on Linux-x64.

Thanks, Harold

RFR 8235359: Simplify method Class.getRecordComponents()

[Harold Seigel]

Hi,

Please review this small change to simplify Class.getRecordComponents() 
by changing the return type of getRecordComponents0() to RecordComponent[].


Open Webrev: 
http://cr.openjdk.java.net/~hseigel/bug_8235359/webrev/index.html


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

The fix was regression tested by running Mach5 tiers 1 and 2 tests and 
builds on Linux-x64, Solaris, Windows, and Mac OS X, by running Mach5 
tiers 3-5 tests on Linux-x64, and JCK lang and VM tests on Linux-x64.


Thanks, Harold

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

[David Holmes]

Hi Christoph,

On 5/12/2019 9:55 pm, Langer, Christoph wrote:

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?


Avoiding the include does seem to be the way to go. That seems so 
obvious now.


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. Further, does:


src/java.base/unix/native/libjava/canonicalize_md.c

need to include jdk_util.h? I think not.

+/* 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/


src/java.base/windows/native/libjava/io_util_md.c

should now be unchanged, but you've left in the copyright update.

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

Thanks,
David



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 Mess

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; 
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(S): 8220348: [ntintel] asserts about copying unalinged array

[Doerr, Martin]

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 
Sent: Mittwoch, 4. Dezember 2019 17:56
To: Doerr, Martin 
Cc: core-libs-dev@openjdk.java.net; security-dev 
; Lindenmaier, Goetz 
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: JEP 367: Remove the Pack200 Tools and API

[Alan Bateman]

On 05/12/2019 08:16, Henry Jen wrote:

Hi,

Updated webrev[1] reflect comments since last webrev. Vicente had done all the 
heavy-lifting and hand over to me to finish up.

Changes to symbols is reverted, as we expect that only need to be updated in 
next release by running make/scripts/generate-symbol-data.sh.

The jar files are confusing in the webrev, but those files are removed. The 
whole test/jdk/tools/pack200 is removed.

Cheers,
Henry

[1] http://cr.openjdk.java.net/~henryjen/jdk14/8234542/0/webrev/

The update webrev looks okay to me, except this part of the comment in 
flags-cflags.m4


"Now that unpack200 has been removed we should consider setting it for 
windows too. but this could be done as a follow-up effort. It could be 
that other other clients are relying on the current configuration for 
windows".


I think it would be best to create an infrastructure/build issue and 
leave most of this  confusing comment out.


-Alan.

Re: RFR: JEP 367: Remove the Pack200 Tools and API

[Henry Jen]

Hi,

Updated webrev[1] reflect comments since last webrev. Vicente had done all the 
heavy-lifting and hand over to me to finish up.

Changes to symbols is reverted, as we expect that only need to be updated in 
next release by running make/scripts/generate-symbol-data.sh. 

The jar files are confusing in the webrev, but those files are removed. The 
whole test/jdk/tools/pack200 is removed.

Cheers,
Henry

[1] http://cr.openjdk.java.net/~henryjen/jdk14/8234542/0/webrev/


> On Dec 2, 2019, at 6:50 PM, Joe Darcy  wrote:
> 
> Hi Vicente,
> 
> It looks like the update to make/data/symbols/symbols removes the jdk.pack 
> module from the history JDKs 9, 10, and 11 when --release is used.
> 
> If that is the case, it would be incorrect since historically the jdk.pack 
> module was present in those releases.
> 
> Thanks,
> 
> -Joe
> 
> On 11/22/2019 1:30 PM, Vicente Romero wrote:
>> Hi all,
>> 
>> On 11/22/19 3:21 AM, Alan Bateman wrote:
>>> 
>>> 
>>> On 21/11/2019 19:53, Vicente Romero wrote:
 Hi,
 
 I think I have covered all the proposed fixes so far. This is the last 
 iteration of the webrev [1], all the current changes are in this one, the 
 code hasn't been split into different webrevs. I'm also forwarding to 
 build-dev as there are some build related changes too. The CSR for this 
 change is at [2]
>>> Would it be possible to summarize what will remain in 
>>> test/jdk/tools/pack200 after this removal? The webrev makes it looks like 
>>> badattr.jar is being added but since it already exists then I'm not sure 
>>> whether to believe it. pack200-verifier/data/golden.jar is another one as 
>>> it looks like JAR file that is generated by the tests today is being 
>>> checked in, maybe `hg add` in error?
>> 
>> I don't know why it is shown as added in the webrev, they have been removed. 
>> I have published another iteration of the webrev at [1]
>>> 
>>> The change to flags-cflag.m4 to add LP64=1 on Windows will need eyes, it's 
>>> not immediately obvious to me which shared code compiled on Windows is 
>>> impacted by this.
>> 
>> yes probably this change is risky. I did it as the comment suggested that 
>> the only reason the treatment for Windows was different was because of 
>> pack200. But not sure if we can trust that comment. Should I restore this 
>> code to its original state?
>>> 
>>> -Alan
>> 
>> 
>> Thanks,
>> Vicente
>> 
>> [1] http://cr.openjdk.java.net/~vromero/8234542/webrev.03/