Re: RFR: JDK-8194750,Console.readPassword does not save/restore tty settings

[Xueming Shen]

Thanks!

webrev has been updated accordingly as suggested.
http://cr.openjdk.java.net/~sherman/8194750/webrev

sherman


On 4/16/18, 7:20 PM, Martin Buchholz wrote:

Thanks, this looks good.

But I have my usual nitpicky comments

  376 // sets the console echo on/off
  377 private static native boolean echo(boolean on) throws IOException;

I would document the return value.
  @returnsthe previous console echo on/off status

  314 boolean echoWasOn = true;
I much prefer leaving it blank (in Java, not C !)

boolean echoWasOn;

relying on definite assignment in Java for safety.

Re: RFR: JDK-8194750, Console.readPassword does not save/restore tty settings

[Martin Buchholz]

Thanks, this looks good.

But I have my usual nitpicky comments

 376 // sets the console echo on/off
 377 private static native boolean echo(boolean on) throws IOException;


I would document the return value.

 @returns the previous console echo on/off status


 314 boolean echoWasOn = true;

I much prefer leaving it blank (in Java, not C !)

boolean echoWasOn;

relying on definite assignment in Java for safety.


On Mon, Apr 16, 2018 at 12:18 PM, Xueming Shen 
wrote:

> On 4/13/18, 8:04 PM, Martin Buchholz wrote:
>
> This is trickier than I expected, since you have to manage
> saving/restoring around each call to readPassword AND have an exit hook to
> restore in case the user never gets around to responding to the prompt.
>
> I see in the old code echo returns a boolean that you would think could be
> used to restore.  You want to save/restore around each readPassword AND
> around the entire java program.  It still seems right to restore after
> every readpassword, using the returned value from echo (which the suggested
> fix ignores!).  But also, the very first time readPassword is called, you
> create an exit hook to restore the value known at that time.  So you need
> echo0 as well ... or do you ... maybe you just need to keep track of a
> single state variable for you to do first-time setup.
>
> Updated to use a local flag for each "echo-off" setting for
> readPassword(), so the
> echo will not be turned on after readpw if its initial status is off
> before readpw.
>
> Still keep the global one & exit-hook for use scenario that someone ctrl-D
> to exit
> or the vm gets -kill signal during passwd reading.
>
> webrev has been updated accordingly (with the updates based on other
> comments).
>
> http://cr.openjdk.java.net/~sherman/8194750/webrev
>
> Thanks!
> Sherman
>
>
> +return tio.c_lflag & ECHO;
>
> Pedantically, functions returning jboolean should return only JNI_TRUE and
> JNI_FALSE.  I would do:
>
> return (tio.c_lflag & ECHO) != 0;
>
> although there's the even more pedantic
>
> return ((tio.c_lflag & ECHO) != 0) ? JNI_TRUE : JNI_FALSE;
>
> typo: chosole
>
>
>
>
>

Re: RFR: 8184693: (opt) add Optional.isEmpty

[Stuart Marks]

Hi Vivek,

Please add "@since 11" tags to the doc comments of the four Optional*.isEmpty() 
methods.


Regarding the tests, I don't think the various newly added testIsEmpty() tests 
are useful. The setup in those test files already generates a fairly 
comprehensive set of Optional values from a variety of sources. All the 
assertion checking is performed in the checkEmpty() and checkPresent() methods. 
It should be sufficient to add an assertTrue(isEmpty()) call to checkEmpty() -- 
as you've done -- and also to add an assertFalse(isEmpty()) call to 
checkPresent(). If these assertions are added, the additional testIsEmpty() test 
is unnecessary.


This applies to all four of the regression test files.

Thanks,

s'marks

On 4/16/18 11:08 AM, Vivek Theeyarath wrote:

Hi All,
Please find the updated webrev 
http://cr.openjdk.java.net/~vtheeyarath/8184693/webrev.02/ .
Here is the csr which I have raised for this change 
https://bugs.openjdk.java.net/browse/JDK-8201606

Regards
Vivek
-Original Message-
From: Chris Hegarty
Sent: Sunday, April 15, 2018 6:48 PM
To: Vivek Theeyarath 
Cc: Remi Forax ; core-libs-dev 

Subject: Re: RFR: 8184693: (opt) add Optional.isEmpty




On 15 Apr 2018, at 11:25, Vivek Theeyarath  wrote:

Hi All,
Please review http://cr.openjdk.java.net/~vtheeyarath/8184693/webrev.01/


This looks ok to me.

For consistency, can you please update the copyright header year range in 
OptionalInt.

-Chris.


Regards
Vivek
-Original Message-
From: Vivek Theeyarath
Sent: Saturday, April 14, 2018 6:24 PM
To: Remi Forax 
Cc: core-libs-dev 
Subject: RE: RFR: 8184693: (opt) add Optional.isEmpty

I missed that Remi. Thanks for pointing it out. Will address those and get back.

Regards
Vivek
-Original Message-
From: Remi Forax [mailto:fo...@univ-mlv.fr]
Sent: Saturday, April 14, 2018 2:58 PM
To: Vivek Theeyarath 
Cc: core-libs-dev 
Subject: Re: RFR: 8184693: (opt) add Optional.isEmpty

Hi Vivek,
OptionalInt, OptionalLong and OptionalDouble should be changed too.

Rémi

- Mail original -

De: "Vivek Theeyarath" 
À: "core-libs-dev" 
Envoyé: Samedi 14 Avril 2018 08:22:50
Objet: RFR: 8184693: (opt) add Optional.isEmpty



Hi All,

  Please review.

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

Webrev : http://cr.openjdk.java.net/~vtheeyarath/8184693/webrev.00/



The related jtreg test was run and the test passed .



Regards

Vivek

Re: Clean-room implementation of Double::toString(double) and Float::toString(float)

[mark . reinhold]

2018/4/12 1:12:36 -0700, raffaello.giulie...@gmail.com:
> my code is now ready to be uploaded to the OpenJDK reps. Currently it 
> resides on GitHub and is under the "GPLv2 + Classpath Exception" 
> license, with myself as the copyright holder.
> 
> I asked Oracle about how the copyright notice should be adapted to meet 
> the OCA requirements but, as of today, I got no answer.
> 
> Is there any official reference about the copyright notice on OpenJDK 
> contributions?

For library code, the template is in $JDK/make/templates/gpl-cp-header [1].
It begins:

  Copyright (c) %YEARS% Oracle and/or its affiliates. All rights reserved.
  DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.

  This code is free software; you can redistribute it and/or modify it
  under the terms of the GNU General Public License version 2 only, as
  ...

You can just use the Oracle copyright line or, at your option, replace
it with your own.  In any case %YEARS% should be replaced with the year
in which the file was created, and if it was modified in any later year
then the creation year should be followed by the latest year in which
the file was modified, separated by a comma.

(We can better advise you on the details once we see the actual code.)

- Mark


[1] http://hg.openjdk.java.net/jdk/jdk/file/tip/make/templates/gpl-cp-header

Re: Clean-room implementation of Double::toString(double) and Float::toString(float)

[David Holmes]

On 16/04/2018 11:22 PM, Mario Torre wrote:

2018-04-12 10:12 GMT+02:00 Raffaello Giulietti :


I asked Oracle about how the copyright notice should be adapted to meet the
OCA requirements but, as of today, I got no answer.


All the headers need to have a valid copyright text, you can just copy
the headers from any other file in the OpenJDK sources, as they are
all the same (just keep attention to the year, as this may not always
be up to date).


That's not true Mario. Files from different sources have different 
copyrights. Those that originated outside Oracle have non-Oracle 
copyrights. Some have dual copyrights.



The license and other useful things are explained here:

http://openjdk.java.net/legal/


None of that explains how to formulate the initial copyright header for 
sources added to OpenJDK.


Cheers,
David


Cheers,
Mario

Re: RFR: 8201609 - Split test/jdk/:tier2 to enable better parallel execution

[Christian Tornqvist]

Hi Alan

> On Apr 16, 2018, at 4:14 05PM, Alan Bateman  wrote:
> 
> On 16/04/2018 19:15, Christian Tornqvist wrote:
>> Please review this small change that splits the tier2 test group into three 
>> smaller test groups that can be executed in parallel.
>> 
>> Webrev: http://cr.openjdk.java.net/~ctornqvi/webrev/8201609/webrev.00/ 
>> 
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8201609 
>> 
>> 
> tier_part2 is an odd mix that I couldn't imagine anyone using directly. It's 
> okay for CI systems of course so no objection to the change.
> 

Yeah, my intention here was to split the existing group into somewhat balanced 
smaller parts. I expect normal users to keep running the tier2 group rather 
than using the tier2_part* ones.

> Do you know what sun/nio/cs/ISO8859x.java and com/sun/crypto/provider/Cipher 
> are special cased? Just wondering if there is a historical or current reason 
> to not run those.
> 

No idea.

Thanks,
Christian

> -Alan

Re: RFR(S): 8201540: [AIX] Extend the set of supported charsets in java.base

[Alan Bateman]

On 16/04/2018 18:43, Xueming Shen wrote:

It looks good to me.
I agree, the main thing is that it's not adding charsets to java.base 
for the other builds.


-Alan

Re: RFR: 8201609 - Split test/jdk/:tier2 to enable better parallel execution

[Alan Bateman]

On 16/04/2018 19:15, Christian Tornqvist wrote:

Please review this small change that splits the tier2 test group into three 
smaller test groups that can be executed in parallel.

Webrev: http://cr.openjdk.java.net/~ctornqvi/webrev/8201609/webrev.00/ 

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


tier_part2 is an odd mix that I couldn't imagine anyone using directly. 
It's okay for CI systems of course so no objection to the change.


Do you know what sun/nio/cs/ISO8859x.java and 
com/sun/crypto/provider/Cipher are special cased? Just wondering if 
there is a historical or current reason to not run those.


-Alan

Re: RFR: JDK-8194750,Console.readPassword does not save/restore tty settings

[Xueming Shen]

On 4/13/18, 8:04 PM, Martin Buchholz wrote:
This is trickier than I expected, since you have to manage 
saving/restoring around each call to readPassword AND have an exit 
hook to restore in case the user never gets around to responding to 
the prompt.


I see in the old code echo returns a boolean that you would think 
could be used to restore.  You want to save/restore around each 
readPassword AND around the entire java program.  It still seems right 
to restore after every readpassword, using the returned value from 
echo (which the suggested fix ignores!).  But also, the very first 
time readPassword is called, you create an exit hook to restore the 
value known at that time.  So you need echo0 as well ... or do you ... 
maybe you just need to keep track of a single state variable for you 
to do first-time setup.


Updated to use a local flag for each "echo-off" setting for 
readPassword(), so the
echo will not be turned on after readpw if its initial status is off 
before readpw.


Still keep the global one & exit-hook for use scenario that someone 
ctrl-D to exit

or the vm gets -kill signal during passwd reading.

webrev has been updated accordingly (with the updates based on other 
comments).


http://cr.openjdk.java.net/~sherman/8194750/webrev

Thanks!
Sherman



+return tio.c_lflag&  ECHO;
Pedantically, functions returning jboolean should return only JNI_TRUE 
and JNI_FALSE.  I would do:

return (tio.c_lflag&  ECHO) != 0;
although there's the even more pedantic

return ((tio.c_lflag & ECHO) != 0) ? JNI_TRUE : JNI_FALSE;

typo: chosole

RFR: 8201609 - Split test/jdk/:tier2 to enable better parallel execution

[Christian Tornqvist]

Please review this small change that splits the tier2 test group into three 
smaller test groups that can be executed in parallel.

Webrev: http://cr.openjdk.java.net/~ctornqvi/webrev/8201609/webrev.00/ 

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


Thanks,
Christian

RE: RFR: 8184693: (opt) add Optional.isEmpty

[Vivek Theeyarath]

Hi All,
Please find the updated webrev 
http://cr.openjdk.java.net/~vtheeyarath/8184693/webrev.02/ .
Here is the csr which I have raised for this change 
https://bugs.openjdk.java.net/browse/JDK-8201606 

Regards
Vivek
-Original Message-
From: Chris Hegarty 
Sent: Sunday, April 15, 2018 6:48 PM
To: Vivek Theeyarath 
Cc: Remi Forax ; core-libs-dev 

Subject: Re: RFR: 8184693: (opt) add Optional.isEmpty



> On 15 Apr 2018, at 11:25, Vivek Theeyarath  
> wrote:
> 
> Hi All,
>Please review http://cr.openjdk.java.net/~vtheeyarath/8184693/webrev.01/ 

This looks ok to me.

For consistency, can you please update the copyright header year range in 
OptionalInt.

-Chris.

> Regards
> Vivek
> -Original Message-
> From: Vivek Theeyarath 
> Sent: Saturday, April 14, 2018 6:24 PM
> To: Remi Forax 
> Cc: core-libs-dev 
> Subject: RE: RFR: 8184693: (opt) add Optional.isEmpty
> 
> I missed that Remi. Thanks for pointing it out. Will address those and get 
> back.
> 
> Regards
> Vivek
> -Original Message-
> From: Remi Forax [mailto:fo...@univ-mlv.fr] 
> Sent: Saturday, April 14, 2018 2:58 PM
> To: Vivek Theeyarath 
> Cc: core-libs-dev 
> Subject: Re: RFR: 8184693: (opt) add Optional.isEmpty
> 
> Hi Vivek,
> OptionalInt, OptionalLong and OptionalDouble should be changed too.
> 
> Rémi
> 
> - Mail original -
>> De: "Vivek Theeyarath" 
>> À: "core-libs-dev" 
>> Envoyé: Samedi 14 Avril 2018 08:22:50
>> Objet: RFR: 8184693: (opt) add Optional.isEmpty
> 
>> Hi All,
>> 
>>  Please review.
>> 
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8184693
>> 
>> Webrev : http://cr.openjdk.java.net/~vtheeyarath/8184693/webrev.00/
>> 
>> 
>> 
>> The related jtreg test was run and the test passed .
>> 
>> 
>> 
>> Regards
>> 
>> Vivek

Re: RFR(S): 8201540: [AIX] Extend the set of supported charsets in java.base

[Xueming Shen]

It looks good to me.

-Sherman

On 4/16/18, 4:10 AM, Bhaktavatsal R Maram wrote:

Hi All,

I've regenerated webrev using "hg rename" to create template files. webrev 
looks much neat now.. Thanks Alan for suggestion.

webrev - http://cr.openjdk.java.net/~gromero/8201540/v2/

Thanks,
Bhaktavatsal Reddy


-"core-libs-dev"  wrote: -
To: Alan Bateman
From: "Bhaktavatsal R Maram"
Sent by: "core-libs-dev"
Date: 04/16/2018 02:38PM
Cc: Tim Ellison, ppc-aix-port-...@openjdk.java.net, Java Core 
Libs
Subject: Re: RFR(S): 8201540: [AIX] Extend the set of supported charsets in 
java.base

Hi Alan,

I deleted IBM943C.java (using hg remove) and added new file IBM943C.java.template (using 
hg add). I now understand that using "hg rename" is giving more meaningful 
representation in webrev/index.html.

I will re-generate webrev by renaming source files to templates using "hg 
rename"

Thanks,
Bhaktavatsal Reddy



-Alan Bateman  wrote: -
To: Bhaktavatsal R Maram, Volker 
Simonis
From: Alan Bateman
Date: 04/16/2018 02:16PM
Cc: Java Core Libs, Tim 
Ellison, ppc-aix-port-...@openjdk.java.net
Subject: Re: RFR(S): 8201540: [AIX] Extend the set of supported charsets in 
java.base


On 16/04/2018 09:22, Bhaktavatsal R Maram wrote:

3. Source files for IBM-942C and IBM-943C are changed to template to support #1


You might want to double check the webrev as it looks like you've added
templates where as I assume you mean to use "hg rename" to rename
IBM942C.java and IBM943C.java.

-Alan

Re: InetAddress.getByName/getAllByName for empty host string

[Chris Hegarty]

Jaikiran,

On 13/04/18 16:29, Jaikiran Pai wrote:

Hi Chris,

Thank you creating that JIRA.

If the fix involves just updating the javadoc, is this something that 
youwould like me to contribute as a patch? I have a signed and approved 
OCA, but I will need a sponsor if I do come up with the patch.


A patch containing the specification clarification and an
update to an existing test to cover said specification
clarification would be helpful.

The change will require a CSR, but I can do that on your
behalf.

-Chris.


-Jaikiran


On 13/04/18 8:41 PM, Chris Hegarty wrote:

Hi Jaikiran,

On 13/04/18 15:29, Jaikiran Pai wrote:
The javadoc of InetAddress.getByName(host) and getAllByName(host) 
states that:


If the host is null then an InetAddress representing an address of 
the loopback interface is returned.


For non-null values the javadoc explains what the implementation 
does. However, there seems to be no mention of what it does with an 
empty string (length == 0) value for the host param. Right now, the 
implementation seems to treat an empty value the same way it treats 
the host == null case and returns an InetAddress representing the 
loopback address.


Consider the following example:

public class InetAddressTest {
 public static void main(final String[] args) throws Exception {
 System.out.println("InetAddress.getByName() for empty string 
returns " + java.net.InetAddress.getByName(""));
 System.out.println("InetAddress.getAllByName() for empty 
string returns "
 + 
java.util.Arrays.toString(java.net.InetAddress.getAllByName("")));


 }
}

This outputs:

InetAddress.getByName() for empty string returns localhost/127.0.0.1
InetAddress.getAllByName() for empty string returns 
[localhost/127.0.0.1]



Is it intentional for these APIs to behave this way for empty string?


Yes.


If so, should the javadoc be updated to explicitly state this behaviour?


Yeah, probably.

The following JIRA issue has been filed to track this:
  https://bugs.openjdk.java.net/browse/JDK-8201545

-Chris.

Re: Clean-room implementation of Double::toString(double) and Float::toString(float)

[Mario Torre]

2018-04-12 10:12 GMT+02:00 Raffaello Giulietti :

> I asked Oracle about how the copyright notice should be adapted to meet the
> OCA requirements but, as of today, I got no answer.

All the headers need to have a valid copyright text, you can just copy
the headers from any other file in the OpenJDK sources, as they are
all the same (just keep attention to the year, as this may not always
be up to date).

The license and other useful things are explained here:

http://openjdk.java.net/legal/

Cheers,
Mario
-- 
pgp key: http://subkeys.pgp.net/ PGP Key ID: 80F240CF
Fingerprint: BA39 9666 94EC 8B73 27FA  FC7C 4086 63E3 80F2 40CF

Java Champion - Blog: http://neugens.wordpress.com - Twitter: @neugens
Proud GNU Classpath developer: http://www.classpath.org/
OpenJDK: http://openjdk.java.net/projects/caciocavallo/

Please, support open standards:
http://endsoftpatents.org/

Re: 8201226 missing JNIEXPORT / JNICALL at some places in function declarations/implementations - was : RE: missing JNIEXPORT / JNICALL at some places in function declarations/implementations

[Magnus Ihse Bursie]

On 2018-04-16 14:59, Alexey Ivanov wrote:

Hi Matthias, Phil,

The build of 32 bit Windows is broken because of mlib_image.dll. As 
JNICALL modifier has been added to function declarations, they're 
exported with a decorated name, for example _j2d_mlib_ImageCreate@16. 
The functions in this library are looked up by their name [1] and 
therefore none can be found.
You should most likely just be able to remove the JNICALL modifiers for 
libmlib_image.


/Magnus



If you run tests in test/jdk/java/awt/image, for example 
test/jdk/java/awt/image/mlib/MlibOpsTest.java, some of them fail 
because ImagingLib is not available.


I'm working on a patch to fix it.


Regards,
Alexey

[1] 
http://hg.openjdk.java.net/jdk/jdk/file/bc1c7e41e285/src/java.desktop/windows/native/libawt/windows/awt_Mlib.cpp#l60


On 13/04/2018 06:48, Baesken, Matthias wrote:

Hi Phil/Alexey,  thanks for  adding the other lists .

Is this the current version of the change : 
http://cr.openjdk.java.net/~mbaesken/webrevs/8201226.2/ ?

Yes.

Best regards, Matthias



-Original Message-
From: Alexey Ivanov [mailto:alexey.iva...@oracle.com]
Sent: Donnerstag, 12. April 2018 23:53
To: Phil Race ; Baesken, Matthias
; Alan Bateman ;
Magnus Ihse Bursie 
Cc: build-...@openjdk.java.net; core-libs-dev@openjdk.java.net; Doerr,
Martin ; 2d-dev <2d-...@openjdk.java.net>;
hotspot-dev 
Subject: Re: 8201226 missing JNIEXPORT / JNICALL at some places in 
function

declarations/implementations - was : RE: missing JNIEXPORT / JNICALL at
some places in function declarations/implementations


On 12/04/2018 21:42, Phil Race wrote:

How can JNIEXPORT be different between 32 bit & 64 bit ?
I'm sure you saw compilation errors but I don't get why it failed for
32 only.

JNICALL (_stdcall) may be unnecessary on 64 bit Windows but that 
doesn't

explain why the 32 bit compiler would complain about inconsistent
application
of __declspec(dllexport) - ie JNIEXPORT.

Or is that part (adding JNIEXPORT) pure clean up and the compilation
errors were all down to JNICALL ?

Adding missing JNIEXPORT is for cleanup only.

The compiler complained about mismatched JNICALL / non-JNICALL
declarations as the macro changes calling convention from the default
__cdecl  to __stdcall on 32 bit Windows.

Another issue is that __stdcall decorates the functions: prefixes with
underscore and postfixes with @ + size of parameters. Because of the
decorations, classLoader.cpp can't lookup the required functions by 
name

from zip.dll and jimage.dll. The functions are exported but with
different name.

I hope this information adds more details to the picture.


I was a bit puzzled at the removals I saw here :

http://cr.openjdk.java.net/~mbaesken/webrevs/8201226.2/src/java.deskto
p/share/native/libsplashscreen/splashscreen_impl.h.udiff.html

.. I needed to look at the whole file to realise that you were
removing a duplicate
declaration.

That was tricky. I could have been mentioned in the review.


Regards,
Alexey


-phil.

On 04/12/2018 04:04 AM, Baesken, Matthias wrote:

Hi  Alan , this is the up to date webrev .
However we want to add   Alexey   Ivanov  as additional author .

As I read it, this changes the calling convention of these 
functions on

32-bit Windows but it will have no impact on 64-bit Windows (as
__stdcall is ignored) or other platforms, is that correct?


The  change adds  JNIEXPORT   at some places  where it is ben
forgotten , for example :


http://cr.openjdk.java.net/~mbaesken/webrevs/8201226.2/src/java.desktop/share/native/libmlib_image/mlib_c_ImageLookUp.c.udiff.html 



This might have  potential  impact  on other platforms (fixes the
mismatches) .

Best regards, Matthias





-Original Message-
From: Alan Bateman [mailto:alan.bate...@oracle.com]
Sent: Donnerstag, 12. April 2018 12:54
To: Baesken, Matthias ; Magnus Ihse 
Bursie 
Cc: build-...@openjdk.java.net; Doerr, Martin 
; core-libs-dev@openjdk.java.net

Subject: Re: 8201226 missing JNIEXPORT / JNICALL at some places in
function
declarations/implementations - was : RE: missing JNIEXPORT / 
JNICALL at

some places in function declarations/implementations

Adding core-libs-dev as this is change code in libjava, libzip,
libjimage, ...

Can you confirm that this is the up to date webrev:
http://cr.openjdk.java.net/~mbaesken/webrevs/8201226.2/

As I read it, this changes the calling convention of these 
functions on

32-bit Windows but it will have no impact on 64-bit Windows (as
__stdcall is ignored) or other platforms, is that correct?

-Alan

Re: 8201226 missing JNIEXPORT / JNICALL at some places in function declarations/implementations - was : RE: missing JNIEXPORT / JNICALL at some places in function declarations/implementations

[Alexey Ivanov]

Hi Matthias, Phil,

The build of 32 bit Windows is broken because of mlib_image.dll. As 
JNICALL modifier has been added to function declarations, they're 
exported with a decorated name, for example _j2d_mlib_ImageCreate@16. 
The functions in this library are looked up by their name [1] and 
therefore none can be found.


If you run tests in test/jdk/java/awt/image, for example 
test/jdk/java/awt/image/mlib/MlibOpsTest.java, some of them fail because 
ImagingLib is not available.


I'm working on a patch to fix it.


Regards,
Alexey

[1] 
http://hg.openjdk.java.net/jdk/jdk/file/bc1c7e41e285/src/java.desktop/windows/native/libawt/windows/awt_Mlib.cpp#l60


On 13/04/2018 06:48, Baesken, Matthias wrote:

Hi Phil/Alexey,  thanks for  adding the other lists .


Is this the current version of the change :  
http://cr.openjdk.java.net/~mbaesken/webrevs/8201226.2/ ?

Yes.

Best regards, Matthias



-Original Message-
From: Alexey Ivanov [mailto:alexey.iva...@oracle.com]
Sent: Donnerstag, 12. April 2018 23:53
To: Phil Race ; Baesken, Matthias
; Alan Bateman ;
Magnus Ihse Bursie 
Cc: build-...@openjdk.java.net; core-libs-dev@openjdk.java.net; Doerr,
Martin ; 2d-dev <2d-...@openjdk.java.net>;
hotspot-dev 
Subject: Re: 8201226 missing JNIEXPORT / JNICALL at some places in function
declarations/implementations - was : RE: missing JNIEXPORT / JNICALL at
some places in function declarations/implementations


On 12/04/2018 21:42, Phil Race wrote:

How can JNIEXPORT be different between 32 bit & 64 bit ?
I'm sure you saw compilation errors but I don't get why it failed for
32 only.

JNICALL (_stdcall) may be unnecessary on 64 bit Windows but that doesn't
explain why the 32 bit compiler would complain about inconsistent
application
of __declspec(dllexport) - ie JNIEXPORT.

Or is that part (adding JNIEXPORT) pure clean up and the compilation
errors were all down to JNICALL ?

Adding missing JNIEXPORT is for cleanup only.

The compiler complained about mismatched JNICALL / non-JNICALL
declarations as the macro changes calling convention from the default
__cdecl  to __stdcall on 32 bit Windows.

Another issue is that __stdcall decorates the functions: prefixes with
underscore and postfixes with @ + size of parameters. Because of the
decorations, classLoader.cpp can't lookup the required functions by name
from zip.dll and jimage.dll. The functions are exported but with
different name.

I hope this information adds more details to the picture.


I was a bit puzzled at the removals I saw here :

http://cr.openjdk.java.net/~mbaesken/webrevs/8201226.2/src/java.deskto
p/share/native/libsplashscreen/splashscreen_impl.h.udiff.html

.. I needed to look at the whole file to realise that you were
removing a duplicate
declaration.

That was tricky. I could have been mentioned in the review.


Regards,
Alexey


-phil.

On 04/12/2018 04:04 AM, Baesken, Matthias wrote:

Hi  Alan , this is the up to date webrev .
However we want to add   Alexey   Ivanov  as additional  author .


As I read it, this changes the calling convention of these functions on
32-bit Windows but it will have no impact on 64-bit Windows (as
__stdcall is ignored) or other platforms, is that correct?


The  change adds  JNIEXPORT   at some places  where it is  ben
forgotten , for example :



http://cr.openjdk.java.net/~mbaesken/webrevs/8201226.2/src/java.desktop/share/native/libmlib_image/mlib_c_ImageLookUp.c.udiff.html


This might have  potential  impact  on other platforms   (fixes the
mismatches) .

Best regards, Matthias





-Original Message-
From: Alan Bateman [mailto:alan.bate...@oracle.com]
Sent: Donnerstag, 12. April 2018 12:54
To: Baesken, Matthias ; Magnus Ihse Bursie 

Cc: build-...@openjdk.java.net; Doerr, Martin ; 
core-libs-dev@openjdk.java.net
Subject: Re: 8201226 missing JNIEXPORT / JNICALL at some places in
function
declarations/implementations - was : RE: missing JNIEXPORT / JNICALL at
some places in function declarations/implementations

Adding core-libs-dev as this is change code in libjava, libzip,
libjimage, ...

Can you confirm that this is the up to date webrev:
  http://cr.openjdk.java.net/~mbaesken/webrevs/8201226.2/

As I read it, this changes the calling convention of these functions on
32-bit Windows but it will have no impact on 64-bit Windows (as
__stdcall is ignored) or other platforms, is that correct?

-Alan

Re: RFR: JDK-8190187: C++ code calling JNI_CreateJavaVM can be killed by Java

[Adam Farley8]

Hi All, 

Here is the code to resolve JDK-8190187:

http://cr.openjdk.java.net/~mhorie/8190187/hg_diff.txt

Could a committer please take the fix and: 

1) Complete the CSR process for the new JNI Return code. 
2) Commit the changes that contain the Return code.

And, optionally, 3) Perform 1 and 2 for the jdwp agent changes as well, so 
it can use this new functionality.

Let me know if you need any help. :)

Best Regards

Adam Farley


Unless stated otherwise above:
IBM United Kingdom Limited - Registered in England and Wales with number 
741598. 
Registered office: PO Box 41, North Harbour, Portsmouth, Hampshire PO6 3AU

Re: RFR(S): 8201540: [AIX] Extend the set of supported charsets in java.base

[Bhaktavatsal R Maram]

Hi All,

I've regenerated webrev using "hg rename" to create template files. webrev 
looks much neat now.. Thanks Alan for suggestion.

webrev - http://cr.openjdk.java.net/~gromero/8201540/v2/

Thanks,
Bhaktavatsal Reddy


-"core-libs-dev"  wrote: -
To: Alan Bateman 
From: "Bhaktavatsal R Maram" 
Sent by: "core-libs-dev" 
Date: 04/16/2018 02:38PM
Cc: Tim Ellison , ppc-aix-port-...@openjdk.java.net, 
Java Core Libs 
Subject: Re: RFR(S): 8201540: [AIX] Extend the set of supported charsets in 
java.base

Hi Alan,

I deleted IBM943C.java (using hg remove) and added new file 
IBM943C.java.template (using hg add). I now understand that using "hg rename" 
is giving more meaningful representation in webrev/index.html.

I will re-generate webrev by renaming source files to templates using "hg 
rename"

Thanks,
Bhaktavatsal Reddy

  

-Alan Bateman  wrote: -
To: Bhaktavatsal R Maram , Volker Simonis 

From: Alan Bateman 
Date: 04/16/2018 02:16PM
Cc: Java Core Libs , Tim Ellison 
, ppc-aix-port-...@openjdk.java.net
Subject: Re: RFR(S): 8201540: [AIX] Extend the set of supported charsets in 
java.base


On 16/04/2018 09:22, Bhaktavatsal R Maram wrote:
>
> 3. Source files for IBM-942C and IBM-943C are changed to template to support 
> #1
>
You might want to double check the webrev as it looks like you've added 
templates where as I assume you mean to use "hg rename" to rename 
IBM942C.java and IBM943C.java.

-Alan

RFR: JDK-8190187: C++ code calling JNI_CreateJavaVM can be killed by Java

[Adam Farley8]

Hi All, 

I've attached the code to resolve JDK-8190187

Could a committer please take the fix (amended code attached, and 
available on request) and: 

1) Complete the CSR process for the new JNI Return code. 
2) Commit the changes that contain the Return code.

And, optionally, 3) Perform 1 and 2 for the jdwp agent changes as well, so 
it can use this new functionality.



Let me know if you need any help. :)

Best Regards

Adam Farley

Unless stated otherwise above:
IBM United Kingdom Limited - Registered in England and Wales with number 
741598. 
Registered office: PO Box 41, North Harbour, Portsmouth, Hampshire PO6 3AU
diff --git a/src/hotspot/share/prims/jni.cpp b/src/hotspot/share/prims/jni.cpp
--- a/src/hotspot/share/prims/jni.cpp
+++ b/src/hotspot/share/prims/jni.cpp
@@ -3909,7 +3909,7 @@
   bool can_try_again = true;
 
   result = Threads::create_vm((JavaVMInitArgs*) args, _try_again);
-  if (result == JNI_OK) {
+  if ((result == JNI_OK) || (result == JNI_SILENT_EXIT)) {
 JavaThread *thread = JavaThread::current();
 assert(!thread->has_pending_exception(), "should have returned not OK");
 /* thread is thread_in_vm here */
diff --git a/src/hotspot/share/prims/jvmti.xml 
b/src/hotspot/share/prims/jvmti.xml
--- a/src/hotspot/share/prims/jvmti.xml
+++ b/src/hotspot/share/prims/jvmti.xml
@@ -631,8 +631,10 @@
 
 
 The return value from Agent_OnLoad or
-Agent_OnLoad_agent-lib-name is used to indicate an 
error.
-Any value other than zero indicates an error and causes termination of the 
VM.
+Agent_OnLoad_agent-lib-name is used to indicate 
whether
+the agent successfully initialised or not. JNI_OK indicates success, 
JNI_SILENT_EXIT
+indicates the agent did not fully initialize but that no error occurred, 
and any
+other value indicates an error. Non-JNI_OK return codes cause termination 
of the VM.
   
 
   
@@ -14811,6 +14813,12 @@
- The function may return NULL in the start phase if the
  can_generate_early_vmstart capability is enabled.
   
+  
+  Added JNI_SILENT_EXIT return code for early exits without errors.
+  java.c's ParseArguments function now sets pret value to 2 for a NULL 
pwhat. Addendums:
+ - This allows us to clearly identify when no class was set, but no 
other error has occurred.
+ - This is undone in java.c's ContinueInNewThread method, so the 
surface behaviour does not change.  
+  
 
 
 
diff --git a/src/hotspot/share/runtime/thread.cpp 
b/src/hotspot/share/runtime/thread.cpp
--- a/src/hotspot/share/runtime/thread.cpp
+++ b/src/hotspot/share/runtime/thread.cpp
@@ -3637,6 +3637,9 @@
 }
 
 jint Threads::create_vm(JavaVMInitArgs* args, bool* canTryAgain) {
+  /* This gets returned at the end of the method. */
+  /* It allows us to complete vm initialisation and still return an error code 
if we want. */
+  jint jniReturnCode = JNI_OK;
   extern void JDK_Version_init();
 
   // Preinitialize version info.
@@ -3725,7 +3728,9 @@
 
   // Launch -agentlib/-agentpath and converted -Xrun agents
   if (Arguments::init_agents_at_startup()) {
-create_vm_init_agents();
+if (create_vm_init_agents() == JNI_SILENT_EXIT) {
+  jniReturnCode = JNI_SILENT_EXIT;
+}
   }
 
   // Initialize Threads state
@@ -3991,7 +3996,7 @@
 ShouldNotReachHere();
   }
 
-  return JNI_OK;
+  return jniReturnCode;
 }
 
 // type for the Agent_OnLoad and JVM_OnLoad entry points
@@ -4110,9 +4115,10 @@
 // Create agents for -agentlib:  -agentpath:  and converted -Xrun
 // Invokes Agent_OnLoad
 // Called very early -- before JavaThreads exist
-void Threads::create_vm_init_agents() {
+jint Threads::create_vm_init_agents() {
   extern struct JavaVM_ main_vm;
   AgentLibrary* agent;
+  jint jniReturnCode = JNI_OK;
 
   JvmtiExport::enter_onload_phase();
 
@@ -4123,13 +4129,18 @@
   // Invoke the Agent_OnLoad function
   jint err = (*on_load_entry)(_vm, agent->options(), NULL);
   if (err != JNI_OK) {
-vm_exit_during_initialization("agent library failed to init", 
agent->name());
+if (err == JNI_SILENT_EXIT) {
+  jniReturnCode = JNI_SILENT_EXIT;
+} else {
+  vm_exit_during_initialization("agent library failed to init", 
agent->name());
+}
   }
 } else {
   vm_exit_during_initialization("Could not find Agent_OnLoad function in 
the agent library", agent->name());
 }
   }
   JvmtiExport::enter_primordial_phase();
+  return jniReturnCode;
 }
 
 extern "C" {
diff --git a/src/hotspot/share/runtime/thread.hpp 
b/src/hotspot/share/runtime/thread.hpp
--- a/src/hotspot/share/runtime/thread.hpp
+++ b/src/hotspot/share/runtime/thread.hpp
@@ -2153,7 +2153,7 @@
   static jint create_vm(JavaVMInitArgs* args, bool* canTryAgain);
   static void convert_vm_init_libraries_to_agents();
   static void create_vm_init_libraries();
-  static void create_vm_init_agents();
+  static jint create_vm_init_agents();
   static void shutdown_vm_agents();
   static bool 

Re: [PATCH] RFR Bug-pending: Enable Hotspot to Track Native Memory Usage for Direct Byte Buffers

[Adam Farley8]

> On 13/04/2018 15:14, Adam Farley8 wrote:
>> Hi Alan, Peter, 
>> 
>> I see that native memory is tracked in java.nio.Bits, but that only 
includes what the user thinks they are allocating. 
>> 
>> When the VM adds extra memory to the allocation amount this extra bit 
is not represented in the Bits total. 
>> A cursory glance shows, minimum, that we round the requested memory 
quantity up to the heap word size in 
>> the Unsafe.allocateMemory code, and something to do with 
nmt_header_size in os:malloc() (os.cpp) too.
> Is the align_up(sz, HeapWordSize) really causing an issue?

Coupled with the nmt_header_size business, it makes the Bits values wrong. 
The more DBB allocations, the more 
inaccurate those numbers will be.

> 
> We could change Bits to align with HotSpot. The BufferPoolMXBean API 
allows the capacity and memory usage to differ (when we originally added 
this, direct buffers were page aligned) so doing this would mean it more 
accurately reflects the memory allocated to direct buffers.
> 
> -Alan

I agree with you that the "+x" information should be added to only one of 
the two atomic longs.

To get "X", it seems to me that the best option would be to introduce an 
native method in Bits that fetches
"X" directly from Hotspot, using the same code that Hotspot uses (so we'd 
have to abstract-out the Hotspot logic
that adds X to the memory quantity).

This way, anyone modifying the Hotspot logic won't risk rendering the Bits 
logic wrong again.

Best Regards

Adam Farley

Unless stated otherwise above:
IBM United Kingdom Limited - Registered in England and Wales with number 
741598. 
Registered office: PO Box 41, North Harbour, Portsmouth, Hampshire PO6 3AU

Re: RFR(S): 8201540: [AIX] Extend the set of supported charsets in java.base

[Bhaktavatsal R Maram]

Hi Alan,

I deleted IBM943C.java (using hg remove) and added new file 
IBM943C.java.template (using hg add). I now understand that using "hg rename" 
is giving more meaningful representation in webrev/index.html.

I will re-generate webrev by renaming source files to templates using "hg 
rename"

Thanks,
Bhaktavatsal Reddy

  

-Alan Bateman  wrote: -
To: Bhaktavatsal R Maram , Volker Simonis 

From: Alan Bateman 
Date: 04/16/2018 02:16PM
Cc: Java Core Libs , Tim Ellison 
, ppc-aix-port-...@openjdk.java.net
Subject: Re: RFR(S): 8201540: [AIX] Extend the set of supported charsets in 
java.base


On 16/04/2018 09:22, Bhaktavatsal R Maram wrote:
>
> 3. Source files for IBM-942C and IBM-943C are changed to template to support 
> #1
>
You might want to double check the webrev as it looks like you've added 
templates where as I assume you mean to use "hg rename" to rename 
IBM942C.java and IBM943C.java.

-Alan

Re: RFR(S): 8201540: [AIX] Extend the set of supported charsets in java.base

[Alan Bateman]

On 16/04/2018 09:22, Bhaktavatsal R Maram wrote:


3. Source files for IBM-942C and IBM-943C are changed to template to support #1

You might want to double check the webrev as it looks like you've added 
templates where as I assume you mean to use "hg rename" to rename 
IBM942C.java and IBM943C.java.


-Alan

RFR(S): 8201540: [AIX] Extend the set of supported charsets in java.base

[Bhaktavatsal R Maram]

Hi All,

Please review.

Bug: https://bugs.openjdk.java.net/browse/JDK-8201540
webrev: http://cr.openjdk.java.net/~gromero/8201540/v1/webrev/

In this patch,

1. Default charsets Big5, Big5_Solaris, Big5_HKSCS, GB18030, IBM856, IBM921, 
IBM922, IBM942, IBM942C, IBM943, IBM943C, IBM950, IBM970, IBM1046, IBM1124, 
IBM1383, ISO_8859_6, ISO_8859_8, MS1252, TIS_620 for different locales 
supported on AIX are added to standard charsets in java.base module

2. More aliases are added to existing charsets.

3. Source files for IBM-942C and IBM-943C are changed to template to support #1

4. Modified file make/jdk/src/classes/build/tools/charsetmapping/SPI.java to 
increase the Hashtable capacity that holds aliases of standard charsets. As the 
no.of charsets we include in standard charsets are more in this patch, existing 
capacity of Hashtable used to hold aliases is not efficient. Without change to 
this file, JDK won't get built and following error is seen

Exception in thread "main" java.lang.RuntimeException: Cannot find a suitable 
size within given constraints
at build.tools.charsetmapping.Hasher.build(Hasher.java:122)
at build.tools.charsetmapping.Hasher.genClass(Hasher.java:261)
at build.tools.charsetmapping.SPI.genClass(SPI.java:130)
at build.tools.charsetmapping.Main.main(Main.java:99)

Please note that webrev/index.html is not showing below 2 files I deleted. 
However, webrev/jdk11.changeset file show them deleted properly.
  src/jdk.charsets/share/classes/sun/nio/cs/ext/IBM942C.java
  src/jdk.charsets/share/classes/sun/nio/cs/ext/IBM943C.java

I've built the JDK with this patch on both Linux and Aix.


Thanks,
Bhaktavatsal Reddy


  

-Volker Simonis  wrote: -
To: Bhaktavatsal R Maram 
From: Volker Simonis 
Date: 04/13/2018 08:51PM
Cc: Alan Bateman , Java Core Libs 
, Tim Ellison , 
ppc-aix-port-...@openjdk.java.net
Subject: Re: Missing many locales support on AIX platform

Hi Bhaktavatsal Reddy,

thanks for addressing this long standing issue. I've opened "8201540:
[AIX] Extend the set of supported charsets in java.base" [1] to track
this issue.

As I wrote in the bug report, this problem is the consequence of an
emergency fix (JDK-8081332) I did back in 2015 to fix the build on AIX
after the integration of the modularity support change (see
discussion: [2]). At that time I only added the minimal set of
charsets which were required to fix the build.

It would be great if you can get in touch with your IBM colleagues to
find out which are the default extended charsets supported by IBM J9
on AIX. I think we should try to use a similar set here in our OpenJDK
port which is also used by the OpenJ9 for building OpenJ9 on AIX.

Also, your IBM colleagues can help you to host a webrev which will
make the further review of these changes much easier. I've but Tim
from IBM on CC which should have an overview of all the IBM people
involved in the OpenJDK project. Besides that I know at least the
following people who might help you:

Michihiro Horie: ho...@jp.ibm.com
Hiroshi H Horii: ho...@jp.ibm.com
Gustavo Romero: grom...@linux.vnet.ibm.com
Matthew Brandyberyy: mbra...@linux.vnet.ibm.com

Thank you and best regards,
Volker

[1] 
https://urldefense.proofpoint.com/v2/url?u=https-3A__bugs.openjdk.java.net_browse_JDK-2D8201540=DwIFaQ=jf_iaSHvJObTbx-siA1ZOg=KUVGEwJiRVpNtQ9wUhGP6BKqzSTV1OWX31WWPdQMmqg=cieXGANE8bD3liEC2gJzl5hfZorR2qIfggL6U9t-Et8=hC4gcA6uomYgY26uR74VelobSIK1ReDsjRZGmI46UBY=
[2] 
https://urldefense.proofpoint.com/v2/url?u=http-3A__mail.openjdk.java.net_pipermail_2d-2Ddev_2015-2DMay_005431.html=DwIFaQ=jf_iaSHvJObTbx-siA1ZOg=KUVGEwJiRVpNtQ9wUhGP6BKqzSTV1OWX31WWPdQMmqg=cieXGANE8bD3liEC2gJzl5hfZorR2qIfggL6U9t-Et8=qfYLaVVn7tapNDhv5bY6yE72nhngaWqte4wtRhQ3wB8=

On Fri, Apr 13, 2018 at 2:42 PM, Bhaktavatsal R Maram
 wrote:
> Hi Alan,
>
> Thank you for your response. I'm happy that my patch was attached. But, I 
> don't see attachment. So, I inlined patch which contain diffs from 2 
> changesets in mail text. If a Jira bug is opened for this issue, probably I 
> can attach complete and consolidated patch there.
>
> At high level, I'm adding following charsets to standard charset in 
> java.base. For this, I have to change IBM943C and IBM942C from source to 
> template to handle java package and aliases. It is also required to add 
> codepage 932 as alias for IBM942C because both are one and same.
>
> Big5, Big5_HKSCS, GB18030, IBM942, IBM942C, IBM943, IBM943C, IBM950, IBM970, 
> IBM1124, TIS_620
>
>
> These are default charsets for some of locales supported by Operating System 
> (AIX). Since these are not available in standard charset, JDK can't be used 
> in those locale even if they are available in jdk.charset module (java 
> -version fails).
>
> I've followed some of the discussions around this in