Re: [12] RFR JDK-8211266: [TESTBUG] jdk/nio/zipfs/ZipFSTester.java failed intermittently in ZipFSTester.checkRead()

[Amy Lu]

Thank you Lance!

Pushed.

Thanks,
Amy

On 2018/11/22 2:34 AM, Lance Andersen wrote:

Hi Amy,

The changes seem reasonable.

Best
Lance
On Nov 20, 2018, at 3:37 AM, Amy Lu > wrote:


Please review.

Thanks,
Amy

On 2018/11/12 2:17 PM, Amy Lu wrote:

Please review this test fix for jdk/nio/zipfs/ZipFSTester.java

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



Testcase testStreamChannel fails when the given "bytes" length=0 
(expected.length=0, and in such case, also sbc.size=0), from checkRead:


504    static void testStreamChannel() throws Exception {
...
536 checkRead(path, bytes);
}

435    private static void checkRead(Path path, byte[] expected) 
throws IOException {

...
487   int pos = rdm.nextInt((int)sbc.size());
488   int len = rdm.nextInt(Math.min(buf.length, 
expected.length - pos));


This causing
java.lang.IllegalArgumentException: bound must be positive
at java.base/java.util.Random.nextInt(Random.java:388)
at ZipFSTester.checkRead(ZipFSTester.java:487)
at ZipFSTester.testStreamChannel(ZipFSTester.java:536)

It IS possible that (the given "bytes", byte[] expected) 
expected.length=0, which is generated by

    new byte[rdm.nextInt(8192)]

ZipFSTester::checkRead need to take this into account.

Thanks,
Amy








Lance 
Andersen| Principal Member of Technical Staff | +1.781.442.2037

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

Re: RFR JDK-8213909: jdeps --print-module-deps should report missing dependences

[Sundararajan Athijegannathan]

Hi Mandy,

Delta webrev looks good.

-Sundar

On 21/11/18, 2:12 AM, Mandy Chung wrote:

Hi Sundar,

Per Joe's feedback on this CSR, a new `--no-recursive` option
is added to restore the non-transitive behavior.  Here is the
delta webrev:

http://cr.openjdk.java.net/~mchung/jdk12/webrevs/8213909/webrev.01-delta

Thanks
Mandy

On 11/19/18 9:22 AM, Mandy Chung wrote:

Thanks Sundar.  Here is the CSR:
https://bugs.openjdk.java.net/browse/JDK-8213915

Mandy

On 11/19/18 6:48 AM, Sundararajan Athijegannathan wrote:

Looks good to me.

-Sundar

On 15/11/18, 5:46 AM, Mandy Chung wrote:

This patch improves `jdeps --print-module-deps`, `--list-deps` and
`--list-reduced-deps` to report missing dependences and also do 
transitive

dependence analysis as the default.

Webrev at:
http://cr.openjdk.java.net/~mchung/jdk12/webrevs/8213909/webrev.00/

$ jdeps --class-path libs --print-module-deps app.jar

`--print-module-deps` finds the modules required by the specified 
application.
Its result can be used to create a runtime image for such 
application to run.
The current behavior does not report missing dependences.  In 
addition,
`--print-module-deps` only reports module dependences required by 
app.jar.
To include the transitive module dependences required by libs, if 
referenced,

-R option can be used.

If -R option is not specified, app.jar will fail to run on the 
runtime image
created by the output from `jdeps --print-module-deps`.  The patch 
changes
the default behavior to do transitive analysis.  In addition, it 
will report

as an error if any dependence is missing and not found.
The --ignore-missing-deps option can be used to ignore missing deps.

This patch also includes a simple fix in 
JdepsTask.ResourceBundleHelper for:

   JDK-8168869: jdeps: localized messages don't use proper line breaks

I can separate it in its own changeset when I push.

Thanks
Mandy

8214195: Align stdout messages in test/jdk/java/math/BigInteger/PrimitiveConversionTests.java

[Brian Burkhalter]

https://bugs.openjdk.java.net/browse/JDK-8214195
http://cr.openjdk.java.net/~bpb/8214195/webrev.00/

Print statements are different for float and double but should be the same.

Thanks,

Brian

Re: RFR 8177552: Compact Number Formatting support

[naoto . sato]

Hi Nishit,

On 11/21/18 12:53 AM, Nishit Jain wrote:

Hi Naoto,

Updated the webrev based on suggestions

http://cr.openjdk.java.net/~nishjain/8177552/webrevs/webrev.01/

Changes made:
- Replaced List with String[] to be added to the the resource 
bundles


Good.

- refactored DecimalFormat.subparse() to be used by the CNF.parse(), to 
reduce code duplication.


I presume CNF is calling package-private methods in DF to share the same 
code. Some comments noting the sharing would be helpful.



- Also updated it with other changes as suggested in the comments


Sorry I missed your question the last time:
>>> Do you think this is an issue with DecimalFormat.parse() and CNF
>>> should avoid parsing exponential numbers? Or, should CNF.parse() be
>>> modified to be consistent with DecimalFormat.parse() in this aspect?

I think DecimalFormat and CNF should behave the same, ie. 'E' should be 
treated as the exponent without a quote.


Some more comments (all in CompactNumberFormat.java)

line 807: expandAffix() seems to treat localizable special pattern 
characters, but currently the implementation only cares for the minus 
sign. Should other localizable pattern chars be taken care of, such as 
percent sign?


line 869, 888: Define what -1 means as a ret value.

line 897: iterMultiplier be better all capitalized as it is a constant. 
And it could be statically defined in the class to be shared with other 
locations that use "10" for arithmetic operation.


line 1531: Any possibility this could lead to divide-by-zero?

Naoto



Regards,
Nishit Jain
On 20-11-2018 00:33, naoto.s...@oracle.com wrote:

Hi Nishit,

On 11/18/18 10:29 PM, Nishit Jain wrote:

Hi Naoto,

Please check my comments inline.

On 17-11-2018 04:52, naoto.s...@oracle.com wrote:

Hi Nishit,

Here are my comments:

- CLDRConverter: As the compact pattern no more employs 
List, can we eliminate stringListEntry/Element, and use 
Array equivalent instead?
Since the CNF design does not put any limit on the size of compact 
pattern, so at the time of parsing the CLDR xmls using SAX parser, it 
becomes difficult to identify the size of array when the parent 
element of compact pattern is encountered, so I think it is better to 
keep the List while extracting the resources.


OK. However I'd not keep the List format on generating the 
resource bundle, as there is no reason to introduce yet another bundle 
format other than the existing array of String.




- CompactNumberFormat.java

Multiple locations: Use StringBuilder instead of StringBuffer.

OK


line 268: The link points to NumberFormat.getNumberInstance(Locale) 
instead of DecimalFormat

OK. Changed it at line 165 also.


line 855: no need to do toString(). length() can detect whether it's 
empty or not.


line 884: "Overloaded method" reads odd here. I'd prefer 
specializing in the "given number" into either long or biginteger.

OK


line 1500: subparseNumber() pretty much shares the same code with 
DecimalFormat.subparse(). can they be merged?
The existing CNF.subParseNumber differs in the way parseIntegerOnly 
is handled, DecimalFormat.parse()/subparse() behaviour is 
unpredictable with parseIntegeronly = true when multipliers are 
involved (Please see JDK-8199223).


Also, I had thought that the CNF.parse()/subparseNumber() should *not 
*parse the exponential notation e.g. while parsing "1.05E4K" the 
parsing should break at 'E' and returns 1.05, because 'E' should be 
considered as unparseable character for general number format pattern 
or compact number pattern, but this is not the case with 
DecimalFormat.parse(). The below DecimalFormat general number format 
instance


NumberFormat nf =  NumberFormat.getNumberInstance();
nf.parse("1.05E4")

Successfully parse the string and returns 10500. The same behaviour 
is there with other DecimalFormat instances also e.g. currency instance.


Do you think this is an issue with DecimalFormat.parse() and CNF 
should avoid parsing exponential numbers? Or, should CNF.parse() be 
modified to be consistent with DecimalFormat.parse() in this aspect?


No, I understand there are differences. But I see a lot of duplicated 
piece of code which I would like to eliminate.






line 1913-1923, 1950-1960, 1987-1997, 2024-2034: It simply calls 
super. No need to override them.
Since setters are overridden, I think that it is better to override 
getters also (even if they are just calling super and have same 
javadoc) to keep them at same level. But, if you see no point in 
keeping them in CNF, I will remove them. Does that need CSR change?


I don't see any point for override. I don't think there needs a CSR, 
but better ask Joe about it.




line 2231: You need to test the type before cast. Otherwise 
ClassCastException may be thrown.
The type is checked in the superclass equals method getClass() != 
obj.getClass(), so I think there is no need to check the type here.


OK.

Naoto



Regards,
Nishit Jain


Naoto

On 11/16/18 9:54 AM, Nishit Jain wrote:

Hi,

Re: [12] RFR JDK-8211266: [TESTBUG] jdk/nio/zipfs/ZipFSTester.java failed intermittently in ZipFSTester.checkRead()

[Roger Riggs]

Hi Amy,

Looks fine,

Thanks, Roger

On 11/20/2018 03:37 AM, Amy Lu wrote:

Please review.

Thanks,
Amy

On 2018/11/12 2:17 PM, Amy Lu wrote:

Please review this test fix for jdk/nio/zipfs/ZipFSTester.java

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

Testcase testStreamChannel fails when the given "bytes" length=0 
(expected.length=0, and in such case, also sbc.size=0), from checkRead:


504    static void testStreamChannel() throws Exception {
...
536 checkRead(path, bytes);
}

435    private static void checkRead(Path path, byte[] expected) 
throws IOException {

...
487   int pos = rdm.nextInt((int)sbc.size());
488   int len = rdm.nextInt(Math.min(buf.length, 
expected.length - pos));


This causing
java.lang.IllegalArgumentException: bound must be positive
at java.base/java.util.Random.nextInt(Random.java:388)
at ZipFSTester.checkRead(ZipFSTester.java:487)
at ZipFSTester.testStreamChannel(ZipFSTester.java:536)

It IS possible that (the given "bytes", byte[] expected) 
expected.length=0, which is generated by

    new byte[rdm.nextInt(8192)]

ZipFSTester::checkRead need to take this into account.

Thanks,
Amy

Re: RFR 8177552: Compact Number Formatting support

[Roger Riggs]

Hi Nishit,

Comments on the tests:

- The tests looks to be quite complete.

- Have the locale specific data been independently verified?
   Or are they just assumed to be correct based on using CNF to 
generate the formatted strings?


- Is there any overlap between the format and parse patterns that can be 
removed;
   using the same dataprovider for both format and parse (and an extra 
provider for unique cases).


- Using TestNG consistently would improve the test suite.

- In comments, Capitalize the first word

- The names of the test files should be more consistent, some include 
Test at the beginning,
   some at the end and some not at all.  The utility classes 
(CompactFormatAndParse) name

   doesn't make it clear it is not a test itself.

Serialization Test: should be comparing the fields of the Format instances,
not only that it formats a value the same.

To setup for future revisions, several serialized CNF instances should 
be hardcoded
in the test and deserialized to be checked against the current CNF 
instances.


Using testng dataproviders would show a more regular structure.

CompactFormatAndParse.java:
 - The method don't need "public" since they are used only in the test.
 - unused import of BigInteger

EqualityCheck:
 - Its good form to always have an @run line, even if for default behavior.

 - The CNF.equals method includes both symbols and decimal pattern;
   are there tests for those being the different?

CompactPatternsValidity.java:
 -60:  Indentation of continued data array values would make it more 
readable.


 - Is there any overlap between the format and parse patterns that can 
be removed?
   Using the same dataprovider for both format and parse (and an extra 
provider for unique cases).


CNFRoundingTest.java:
 - Can the Rounding mode test methods be consolidated and pass in the 
desired rounding mode.

  It would save on some boilerplate.

Thanks, Roger


On 11/21/2018 03:53 AM, Nishit Jain wrote:

Hi Naoto,

Updated the webrev based on suggestions

http://cr.openjdk.java.net/~nishjain/8177552/webrevs/webrev.01/

Changes made:
- Replaced List with String[] to be added to the the resource 
bundles
- refactored DecimalFormat.subparse() to be used by the CNF.parse(), 
to reduce code duplication.

- Also updated it with other changes as suggested in the comments

Regards,
Nishit Jain
On 20-11-2018 00:33, naoto.s...@oracle.com wrote:

Hi Nishit,

On 11/18/18 10:29 PM, Nishit Jain wrote:

Hi Naoto,

Please check my comments inline.

On 17-11-2018 04:52, naoto.s...@oracle.com wrote:

Hi Nishit,

Here are my comments:

- CLDRConverter: As the compact pattern no more employs 
List, can we eliminate stringListEntry/Element, and use 
Array equivalent instead?
Since the CNF design does not put any limit on the size of compact 
pattern, so at the time of parsing the CLDR xmls using SAX parser, 
it becomes difficult to identify the size of array when the parent 
element of compact pattern is encountered, so I think it is better 
to keep the List while extracting the resources.


OK. However I'd not keep the List format on generating the 
resource bundle, as there is no reason to introduce yet another 
bundle format other than the existing array of String.




- CompactNumberFormat.java

Multiple locations: Use StringBuilder instead of StringBuffer.

OK


line 268: The link points to NumberFormat.getNumberInstance(Locale) 
instead of DecimalFormat

OK. Changed it at line 165 also.


line 855: no need to do toString(). length() can detect whether 
it's empty or not.


line 884: "Overloaded method" reads odd here. I'd prefer 
specializing in the "given number" into either long or biginteger.

OK


line 1500: subparseNumber() pretty much shares the same code with 
DecimalFormat.subparse(). can they be merged?
The existing CNF.subParseNumber differs in the way parseIntegerOnly 
is handled, DecimalFormat.parse()/subparse() behaviour is 
unpredictable with parseIntegeronly = true when multipliers are 
involved (Please see JDK-8199223).


Also, I had thought that the CNF.parse()/subparseNumber() should 
*not *parse the exponential notation e.g. while parsing "1.05E4K" 
the parsing should break at 'E' and returns 1.05, because 'E' should 
be considered as unparseable character for general number format 
pattern or compact number pattern, but this is not the case with 
DecimalFormat.parse(). The below DecimalFormat general number format 
instance


NumberFormat nf =  NumberFormat.getNumberInstance();
nf.parse("1.05E4")

Successfully parse the string and returns 10500. The same behaviour 
is there with other DecimalFormat instances also e.g. currency 
instance.


Do you think this is an issue with DecimalFormat.parse() and CNF 
should avoid parsing exponential numbers? Or, should CNF.parse() be 
modified to be consistent with DecimalFormat.parse() in this aspect?


No, I understand there are differences. But I see a lot of duplicated 
piece of code which I would like to eliminate.






line 

Re: [12] RFR JDK-8211266: [TESTBUG] jdk/nio/zipfs/ZipFSTester.java failed intermittently in ZipFSTester.checkRead()

[Lance Andersen]

Hi Amy,

The changes seem reasonable.

Best
Lance
> On Nov 20, 2018, at 3:37 AM, Amy Lu  wrote:
> 
> Please review.
> 
> Thanks,
> Amy
> 
> On 2018/11/12 2:17 PM, Amy Lu wrote:
>> Please review this test fix for jdk/nio/zipfs/ZipFSTester.java
>> 
>> bug: https://bugs.openjdk.java.net/browse/JDK-8211266
>> webrev: http://cr.openjdk.java.net/~amlu/8211266/webrev.00/
>> 
>> Testcase testStreamChannel fails when the given "bytes" length=0 
>> (expected.length=0, and in such case, also sbc.size=0), from checkRead:
>> 
>> 504static void testStreamChannel() throws Exception {
>> ...
>> 536 checkRead(path, bytes);
>> }
>> 
>> 435private static void checkRead(Path path, byte[] expected) throws 
>> IOException {
>> ...
>> 487   int pos = rdm.nextInt((int)sbc.size());
>> 488   int len = rdm.nextInt(Math.min(buf.length, expected.length 
>> - pos));
>> 
>> This causing
>> java.lang.IllegalArgumentException: bound must be positive
>> at java.base/java.util.Random.nextInt(Random.java:388)
>> at ZipFSTester.checkRead(ZipFSTester.java:487)
>> at ZipFSTester.testStreamChannel(ZipFSTester.java:536)
>> 
>> It IS possible that (the given "bytes", byte[] expected) expected.length=0, 
>> which is generated by
>> new byte[rdm.nextInt(8192)]
>> 
>> ZipFSTester::checkRead need to take this into account.
>> 
>> Thanks,
>> Amy
>> 
>> 
> 

 
  

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

Re: 6516099: InputStream.skipFully(int k) to skip exactly k bytes

[Roger Riggs]

Looks fine Brian!

On 11/21/2018 12:19 PM, Brian Burkhalter wrote:

Hello, again,

In response to comments on the CSR

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

the following change to the verbiage is proposed:

--- a/src/java.base/share/classes/java/io/InputStream.java
+++ b/src/java.base/share/classes/java/io/InputStream.java
@@ -562,7 +562,8 @@
   * in an inconsistent state. It is strongly recommended that the
   * stream be promptly closed if an I/O error occurs.
   *
- *  Subclasses are encouraged to provide a more efficient implementation
+ * @implNote
+ * Subclasses are encouraged to provide a more efficient implementation
   * of this method.
   *
   * @implSpec
@@ -572,7 +573,8 @@
   * then {@link #read()} is invoked repeatedly until the stream is {@code 
n}
   * bytes beyond its position when this method was invoked or end of stream
   * is reached.  If the return value of {@code skip(n)} is negative or
- * greater than {@code n}, then an {@code IOException} is thrown.
+ * greater than {@code n}, then an {@code IOException} is thrown.  Any
+ * exception thrown by {@code skip()} or {@code read()} will be propagated.
   *
   * @param  n   the number of bytes to be skipped.
   * @throws EOFException if end of stream is encountered before the

Thanks,

Brian


On Nov 16, 2018, at 5:25 PM, Brian Burkhalter  
wrote:

So updated in place.

http://cr.openjdk.java.net/~bpb/6516099/webrev.07/ 

Re: 6516099: InputStream.skipFully(int k) to skip exactly k bytes

[Brian Burkhalter]

Hello, again,

In response to comments on the CSR

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

the following change to the verbiage is proposed:

--- a/src/java.base/share/classes/java/io/InputStream.java
+++ b/src/java.base/share/classes/java/io/InputStream.java
@@ -562,7 +562,8 @@
  * in an inconsistent state. It is strongly recommended that the
  * stream be promptly closed if an I/O error occurs.
  *
- *  Subclasses are encouraged to provide a more efficient implementation
+ * @implNote
+ * Subclasses are encouraged to provide a more efficient implementation
  * of this method.
  *
  * @implSpec
@@ -572,7 +573,8 @@
  * then {@link #read()} is invoked repeatedly until the stream is {@code n}
  * bytes beyond its position when this method was invoked or end of stream
  * is reached.  If the return value of {@code skip(n)} is negative or
- * greater than {@code n}, then an {@code IOException} is thrown.
+ * greater than {@code n}, then an {@code IOException} is thrown.  Any
+ * exception thrown by {@code skip()} or {@code read()} will be propagated.
  *
  * @param  n   the number of bytes to be skipped.
  * @throws EOFException if end of stream is encountered before the

Thanks,

Brian

> On Nov 16, 2018, at 5:25 PM, Brian Burkhalter  
> wrote:
> 
> So updated in place.
> 
> http://cr.openjdk.java.net/~bpb/6516099/webrev.07/ 
> 

Re: RFR: JDK-8214063: OpenJDK will not build on AIX while using the xlc 13.1 compiler

[Adam Farley8]

Hi Volker,

Will do some digging and see if I can figure out the cause of the 
compilation problem.

Also, I don't have a record of the original error after we added the 
JNIEXPORT/JNIIMPORT fix in, so I've got a build running now to reproduce 
it.

Best Regards

Adam Farley 
IBM Runtimes


Volker Simonis  wrote on 21/11/2018 14:07:07:

> From: Volker Simonis 
> To: adam.far...@uk.ibm.com
> Cc: Java Core Libs , "Stuefe, 
> Thomas" 
> Date: 21/11/2018 14:07
> Subject: Re: RFR: JDK-8214063: OpenJDK will not build on AIX while 
> using the xlc 13.1 compiler
> 
> On Wed, Nov 21, 2018 at 1:46 PM Adam Farley8  
wrote:
> >
> > Hi Volker,
> >
> > The NativeImageBuffer.cpp changes are best explained by the full text 
of
> > the referenced GitHub Pull Request, copied here for simplicity:
> >
> > -
> > Define JNIEXPORT and JNIIMPORT for xlc version 13.1 or newer. Without 
this,
> > almost no symbols are exported from shared libraries due to use of
> > -qvisibility=hidden as specified in make/lib/LibCommon.gmk. The 
symptoms
> > are reported in eclipse/openj9#2468.
> >
> > Unfortunately, this encounters a bug in xlc: it fails to parse what 
seems
> > to be reasonable code.
> 
> Sorry, but I don't see how this answers my question.
> 
> 1. Which "reasonable code" does xlc fails to parse. A stand-alone
> example would be nice.
> 
> 2. Have you reported this as bug to the xlc developers? What did they 
say?
> 
> 3. "jdk_internal_jimage_NativeImageBuffer.h" doesn't seem to be
> special. It's a plain, generated JNI header file as generated by
> 'javah' or 'javac -h'. If XLC 13 has problems parsing it, there should
> be much more places which need fixing. So what's special about
> "jdk_internal_jimage_NativeImageBuffer.h".
> 
> In the referenced pull request
> (INVALID URI REMOVED
> 
u=https-3A__github.com_eclipse_openj9_issues_2468=DwIFaQ=jf_iaSHvJObTbx-
> siA1ZOg=P5m8KWUXJf-
> 
CeVJc0hDGD9AQ2LkcXDC0PMV9ntVw5Ho=sgfFd6mB1EYM4nOM89rgFFzUyX7B21XbckIY7L0kUNU=TJ-4nr8ikZKImwDygirRTxLybsnQWBN71nEZCwZ59NQ=
> ) I can only see linker
> errors (and no compiler errors). The linker errors are for both
> libjsig and libjava. They are related to the symbol ".sigaction" in
> jsig.o and I don't see how this should be related to
> NativeImageBuffer.cpp or "jdk_internal_jimage_NativeImageBuffer.h".
> NativeImageBuffer.cpp is only used to create libjimage and not related
> in any way to libjsig or libjava.
> 
> It seems wired to do the change to NativeImageBuffer.cpp which you've
> proposed without understanding the real cause of the problem.
> 
> Regards,
> Volker
> 
> > A workaround is required in just one place:
> > src/java.base/share/native/libjimage/NativeImageBuffer.cpp.
> > -
> >
> > Best Regards
> >
> > Adam Farley
> > IBM Runtimes
> >
> >
> > Volker Simonis  wrote on 20/11/2018 
17:50:41:
> >
> > > From: Volker Simonis 
> > > To: "Stuefe, Thomas" 
> > > Cc: adam.far...@uk.ibm.com, Java Core Libs  d...@openjdk.java.net>
> > > Date: 20/11/2018 17:59
> > > Subject: Re: RFR: JDK-8214063: OpenJDK will not build on AIX while
> > > using the xlc 13.1 compiler
> > >
> > > On Tue, Nov 20, 2018 at 6:15 PM Thomas Stüfe 
>  wrote:
> > > >
> > > > On Tue, Nov 20, 2018 at 6:12 PM Adam Farley8 
>  wrote:
> > > > >
> > > > > Heya Tom,
> > > > >
> > > > > "In JDK11 and JDK12, source files are compiled with -
> qvisibility=hidden
> > > > > when using xlc version other than 12.1. That doesn't seem toplay 
well
> > > > > with link option -bexpall. "
> > > > >
> > > > > Found that buried in one of the associated Git issues. It 
appears that
> > > > > it's OpenJDK's use of that option that's causing the problem, 
though
> > > > > I couldn't speculate as to why it was added in the first place.
> > > > >
> > > > > I see this has also been noted in https://
> > > urldefense.proofpoint.com/v2/url?
> > > 
> 
u=https-3A__bugs.openjdk.java.net_browse_JDK-2D8204541=DwIFaQ=jf_iaSHvJObTbx-
> > > siA1ZOg=P5m8KWUXJf-
> > > 
> 
CeVJc0hDGD9AQ2LkcXDC0PMV9ntVw5Ho=SD6UdjysISJRBlWUm8pEzF5lRZ5opfbrKzEh_jrOras=5qDEdIfg8qZ-
> > > vCglsZ9qNDTEPMnCkj-mVPVah6eEDLE=
> > > > >
> > > > > Does that answer your question?
> > > > >
> > > >
> > > > Yes, Thank you. Odd. Will have to do archeology on that one.
> > > >
> > >
> > > No I begin to understand the problem as well :)
> > >
> > > It was actually change "8202322: AIX: symbol visibility flags not
> > > support on xlc 12.1" [1] which introduced "-qvisibility=hidden" for
> > > XLC version not equal to 12.1. That's kind of a weak check and I
> > > suppose nobody has ever tested this change with an XLC version other
> > > than 12.1 (until you came along :). Maybe that check should be a 
more
> > > precisly check for >= 13.1 (but I know such version checks are hard 
to
> > > do in Makefile syntax)?
> > >
> > > The thing I don't understand about your patch (the changes in
> > > "jni_md.h" look good although I haven't tested them) is why you need
> > > the 

Re: Proposal: Add support for Process Groups to the JDK

[Roger Riggs]

Hi Thomas,

I'd be interested in hearing more about the use cases.
There seem to be many cases where containers are doing the management
of groups of processes.

The function will need to have an equivalent on Windows.

The expressed use case is taking advantage of Posix/Unix signal behavior.
But there are oh so many issues with signals, its likely to be a big can 
of worms.

You mention a desire for other Posix functions, please elaborate.

If there's a way to scope it reasonably then a JEP is the process to pursue.

Thanks, Roger


On 11/12/2018 12:29 PM, Thomas Stüfe wrote:

Dear all,

may I please hear your thoughts about the following proposal?

We would like to add support for process groups to the JDK: the
ability to put child processes into new or pre-existing process
groups. We added this feature to our proprietary port some time ago
and has been very useful in cases where the VM acts in a process
scheduling role.

With process groups we mean of course standard Unix process groups.
There exists a similar concept on Windows, Job Objects, so at least a
subset of what we propose could be done in a platform independent way.



Motivation:

Most importantly, the ability to safely terminate a group of processes.

The established way to do this is, since Java 9, to iterate over a
process tree, calling Process.children() or Process.descendants() on
the root Process, and killing them using Process.destroy().

In practice, that approach is not always a good fit. It leaves out any
orphaned processes; any deceased non-leaf process in the tree makes
its children unreachable. Worst case, if the root process dies, all
children are orphaned and cannot be reached. Another limitation is
that this only works for process trees - parent-child relationships -
but not for unrelated processes one might want to group together. It
also becomes a bit inefficient with many processes, requiring one JNI
call/system call per process to kill.

Process groups, OTOH, would allow us to group together any number of
unrelated processes. We can then send them bulk signals, eg
SIGTERM/SIGKILL with only one system call. And for that to work, the
parent relationships do not matter, so we also reach processes which
have been orphaned.

There are more things one could do with process groups besides killing
them: suspend/resume them together (SIGSTOP/CONT), or to send them to
the background of the controlling terminal.

In fact, one could write its own shell in Java :)



I drew up a tiny patch to demonstrate how this could look. This is
just an example, to have something to play with and talk about:

http://cr.openjdk.java.net/~stuefe/webrevs/processgroup-support/webrev.01/webrev/index.html

and here is a small usage example:

https://github.com/tstuefe/ojdk-repros/blob/master/src/other/RuntimeExecSimpleTestWithProcessGroup.java

The suggested API changes are small:

- A new class ProcessGroup as the platform's notion of a process
group. In this patch, it offers four functions:
   - destroy()/destroyForcibly() terminate or kill the whole process group
   - suspend()/resume() puts them to sleep and wakes them up.
   More functionality could be added if needed. This mostly depends on
how tightly we want to be bound by platform limitations on Windows,
where process groups cannot be translated 1:1 to Job Objects.

- ProcessBuilder has now two new attributes:
   - createProcessGroup() is a boolean flag directing the builder to
let sub processes create their own process group, with themselves
being the leader.
   - processGroup() is a reference to a ProcessGroup object; when not
null, subprocesses will join that process group.

- The Process class gets a new query method to retrieve a ProcessGroup
object linked to its process group id.

Using these building stones, a typical pattern could be:


 ProcessBuilder processBuilder = new ProcessBuilder(cmd);
 processBuilder.createProcessGroup(true);  <-- next process is pg leader

 Process leader = processBuilder.start();

 ProcessGroup pgr = leader.processGroup();  <-- retrieve newly
created process group
 processBuilder.processGroup(pgr); <-- next processes shall be
members of this process group too

 processBuilder.start();
 processBuilder.start();
 


and then call operations on the ProcessGroup object.



It is clear to me that this kind of change would require probably a
JEP, if it is desired at all. With this mail I just wanted to gauge
interest.

What do you think?

Thanks & Best Regards, Thomas

Re: RFR: JDK-8214063: OpenJDK will not build on AIX while using the xlc 13.1 compiler

[Volker Simonis]

On Wed, Nov 21, 2018 at 12:44 PM Magnus Ihse Bursie
 wrote:
>
>
>
> On 2018-11-20 18:50, Volker Simonis wrote:
> > On Tue, Nov 20, 2018 at 6:15 PM Thomas Stüfe  
> > wrote:
> >> On Tue, Nov 20, 2018 at 6:12 PM Adam Farley8  
> >> wrote:
> >>> Heya Tom,
> >>>
> >>> "In JDK11 and JDK12, source files are compiled with -qvisibility=hidden
> >>> when using xlc version other than 12.1. That doesn't seem to play well
> >>> with link option -bexpall. "
> >>>
> >>> Found that buried in one of the associated Git issues. It appears that
> >>> it's OpenJDK's use of that option that's causing the problem, though
> >>> I couldn't speculate as to why it was added in the first place.
> >>>
> >>> I see this has also been noted in 
> >>> https://bugs.openjdk.java.net/browse/JDK-8204541
> >>>
> >>> Does that answer your question?
> >>>
> >> Yes, Thank you. Odd. Will have to do archeology on that one.
> >>
> > No I begin to understand the problem as well :)
> >
> > It was actually change "8202322: AIX: symbol visibility flags not
> > support on xlc 12.1" [1] which introduced "-qvisibility=hidden" for
> > XLC version not equal to 12.1. That's kind of a weak check and I
> > suppose nobody has ever tested this change with an XLC version other
> > than 12.1 (until you came along :). Maybe that check should be a more
> > precisly check for >= 13.1 (but I know such version checks are hard to
> > do in Makefile syntax)?
> In configure (where, ideally, all version checks should be made),
> there's the TOOLCHAIN_CHECK_COMPILER_VERSION function, which supports
> #   IF_AT_LEAST:   block to run if the compiler is at least this version
> (>=)
> #   IF_OLDER_THAN:   block to run if the compiler is older than this
> version (<)
> for normal, dot-separated version number schemes.
>

Thanks for the pointer. I was looking for this functionality yesterday
but couldn't find it :)

> /Magnus
>
> >
> > The thing I don't understand about your patch (the changes in
> > "jni_md.h" look good although I haven't tested them) is why you need
> > the extra changes in NativeImageBuffer.cpp?
> > "jdk_internal_jimage_NativeImageBuffer.h" is a plain, generated JNI
> > header file. If XLC 13 has problems to parse it, there should be much
> > more places which need fixing. I think that part of your change needs
> > a closer evaluation.
> >
> > Thank you and best regards,
> > Volker
> >
> > [1] https://bugs.openjdk.java.net/browse/JDK-8202322
> >
> >> ..Thomas
> >>
> >>> Best Regards
> >>>
> >>> Adam Farley
> >>> IBM Runtimes
> >>>
> >>>
> >>> "Thomas Stüfe"  wrote on 20/11/2018 16:44:07:
> >>>
>  From: "Thomas Stüfe" 
>  To: Adam Farley8 
>  Cc: Java Core Libs 
>  Date: 20/11/2018 16:48
>  Subject: Re: RFR: JDK-8214063: OpenJDK will not build on AIX while
>  using the xlc 13.1 compiler
> 
>  Hi Adam,
> 
>  On Tue, Nov 20, 2018 at 5:12 PM Adam Farley8  
>  wrote:
> > Hi Tom,
> >
> > Sounds reasonable. I've added a webex to the bug, and here's a
>  link to the bug.
> > https://urldefense.proofpoint.com/v2/url?
>  u=https-3A__bugs.openjdk.java.net_browse_JDK-2D8214063=DwIFaQ=jf_iaSHvJObTbx-
>  siA1ZOg=P5m8KWUXJf-
>  CeVJc0hDGD9AQ2LkcXDC0PMV9ntVw5Ho=z8YYwBXEfN7UtX1suPjpp9CZSHf8v0GrIMK3XGIC9VY=81TP9mIjhYD2Hmt8g7p2EHWRZXgiep21hxKLYRU7zIQ=
> > This patch is required because otherwise, when building on AIX
>  using xlc 3.1,
> > the build fails with this error:
> >
> > "Visibility is not allowed on a reference to an imported symbol."
> >
> > We believe this is caused by JNIEXPORT and JNIIMPORT not being
>  defined. Without
> > this, almost no symbols are exported from shared libraries due to use of
> > -qvisibility=hidden as specified in make/lib/LibCommon.gmk.
>  Yes but what I try to understand is why does this happen now with
>  xlc13? Did xlc change the rules for -qvisibility from v12 to v13 ?
>  That would be quite a break in backward compatibility.
> 
> > For convenience, here's a summary of the diffs:
> >
> > --
> > File 1 of 2) src/java.base/share/native/libjimage/NativeImageBuffer.cpp
> >
> >   #include "osSupport.hpp"
> >
> > +#if defined(__xlC__) && (__xlC__ >= 0x0d01)
> > +/*
> > + * Version 13.1.3 of xlc seems to have trouble parsing the 
> > `__attribute__`
> > + * annotation in the generated header file we're about to
>  include. Repeating
> > + * the forward declaration (without the braces) here avoids the 
> > diagnostic:
> > + *   1540-0040 (S) The text "void" is unexpected.  "visibility"
>  may be undeclared or ambiguous.
> > + */
> > +extern "C" JNIEXPORT jobject JNICALL
>  Java_jdk_internal_jimage_NativeImageBuffer_getNativeMap(JNIEnv *,
>  jclass, jstring);
> > +#endif
> > +
> > #include "jdk_internal_jimage_NativeImageBuffer.h"
> > --
> > File 

Re: RFR: JDK-8214063: OpenJDK will not build on AIX while using the xlc 13.1 compiler

[Volker Simonis]

On Wed, Nov 21, 2018 at 1:46 PM Adam Farley8  wrote:
>
> Hi Volker,
>
> The NativeImageBuffer.cpp changes are best explained by the full text of
> the referenced GitHub Pull Request, copied here for simplicity:
>
> -
> Define JNIEXPORT and JNIIMPORT for xlc version 13.1 or newer. Without this,
> almost no symbols are exported from shared libraries due to use of
> -qvisibility=hidden as specified in make/lib/LibCommon.gmk. The symptoms
> are reported in eclipse/openj9#2468.
>
> Unfortunately, this encounters a bug in xlc: it fails to parse what seems
> to be reasonable code.

Sorry, but I don't see how this answers my question.

1. Which "reasonable code" does xlc fails to parse. A stand-alone
example would be nice.

2. Have you reported this as bug to the xlc developers? What did they say?

3. "jdk_internal_jimage_NativeImageBuffer.h" doesn't seem to be
special. It's a plain, generated JNI header file as generated by
'javah' or 'javac -h'. If XLC 13 has problems parsing it, there should
be much more places which need fixing. So what's special about
"jdk_internal_jimage_NativeImageBuffer.h".

In the referenced pull request
(https://github.com/eclipse/openj9/issues/2468) I can only see linker
errors (and no compiler errors). The linker errors are for both
libjsig and libjava. They are related to the symbol ".sigaction" in
jsig.o and I don't see how this should be related to
NativeImageBuffer.cpp or "jdk_internal_jimage_NativeImageBuffer.h".
NativeImageBuffer.cpp is only used to create libjimage and not related
in any way to libjsig or libjava.

It seems wired to do the change to NativeImageBuffer.cpp which you've
proposed without understanding the real cause of the problem.

Regards,
Volker

> A workaround is required in just one place:
> src/java.base/share/native/libjimage/NativeImageBuffer.cpp.
> -
>
> Best Regards
>
> Adam Farley
> IBM Runtimes
>
>
> Volker Simonis  wrote on 20/11/2018 17:50:41:
>
> > From: Volker Simonis 
> > To: "Stuefe, Thomas" 
> > Cc: adam.far...@uk.ibm.com, Java Core Libs 
> > Date: 20/11/2018 17:59
> > Subject: Re: RFR: JDK-8214063: OpenJDK will not build on AIX while
> > using the xlc 13.1 compiler
> >
> > On Tue, Nov 20, 2018 at 6:15 PM Thomas Stüfe  
> > wrote:
> > >
> > > On Tue, Nov 20, 2018 at 6:12 PM Adam Farley8  
> > > wrote:
> > > >
> > > > Heya Tom,
> > > >
> > > > "In JDK11 and JDK12, source files are compiled with -qvisibility=hidden
> > > > when using xlc version other than 12.1. That doesn't seem to play well
> > > > with link option -bexpall. "
> > > >
> > > > Found that buried in one of the associated Git issues. It appears that
> > > > it's OpenJDK's use of that option that's causing the problem, though
> > > > I couldn't speculate as to why it was added in the first place.
> > > >
> > > > I see this has also been noted in https://
> > urldefense.proofpoint.com/v2/url?
> > u=https-3A__bugs.openjdk.java.net_browse_JDK-2D8204541=DwIFaQ=jf_iaSHvJObTbx-
> > siA1ZOg=P5m8KWUXJf-
> > CeVJc0hDGD9AQ2LkcXDC0PMV9ntVw5Ho=SD6UdjysISJRBlWUm8pEzF5lRZ5opfbrKzEh_jrOras=5qDEdIfg8qZ-
> > vCglsZ9qNDTEPMnCkj-mVPVah6eEDLE=
> > > >
> > > > Does that answer your question?
> > > >
> > >
> > > Yes, Thank you. Odd. Will have to do archeology on that one.
> > >
> >
> > No I begin to understand the problem as well :)
> >
> > It was actually change "8202322: AIX: symbol visibility flags not
> > support on xlc 12.1" [1] which introduced "-qvisibility=hidden" for
> > XLC version not equal to 12.1. That's kind of a weak check and I
> > suppose nobody has ever tested this change with an XLC version other
> > than 12.1 (until you came along :). Maybe that check should be a more
> > precisly check for >= 13.1 (but I know such version checks are hard to
> > do in Makefile syntax)?
> >
> > The thing I don't understand about your patch (the changes in
> > "jni_md.h" look good although I haven't tested them) is why you need
> > the extra changes in NativeImageBuffer.cpp?
> > "jdk_internal_jimage_NativeImageBuffer.h" is a plain, generated JNI
> > header file. If XLC 13 has problems to parse it, there should be much
> > more places which need fixing. I think that part of your change needs
> > a closer evaluation.
> >
> > Thank you and best regards,
> > Volker
> >
> > [1] https://urldefense.proofpoint.com/v2/url?
> > u=https-3A__bugs.openjdk.java.net_browse_JDK-2D8202322=DwIFaQ=jf_iaSHvJObTbx-
> > siA1ZOg=P5m8KWUXJf-
> > CeVJc0hDGD9AQ2LkcXDC0PMV9ntVw5Ho=SD6UdjysISJRBlWUm8pEzF5lRZ5opfbrKzEh_jrOras=JAEK6rePGMPinZzOquHBzj5oc7vA3kaFt9x0WIIUzvk=
> >
> > > ..Thomas
> > >
> > > > Best Regards
> > > >
> > > > Adam Farley
> > > > IBM Runtimes
> > > >
> > > >
> > > > "Thomas Stüfe"  wrote on 20/11/2018 16:44:07:
> > > >
> > > > > From: "Thomas Stüfe" 
> > > > > To: Adam Farley8 
> > > > > Cc: Java Core Libs 
> > > > > Date: 20/11/2018 16:48
> > > > > Subject: Re: RFR: JDK-8214063: OpenJDK will not build on AIX while
> > > > > using the 

Re: Proposal: Add support for Process Groups to the JDK

[Thomas Stüfe]

Hi David,

On Wed, Nov 21, 2018 at 10:57 AM David Holmes  wrote:
>
> On 21/11/2018 6:14 pm, Thomas Stüfe wrote:
> > Ping.. no-one has any thoughts on this?
>
> I can see it being useful for test harnesses/frameworks.

Oh, very much so! We use this feature in the framework that runs our
nightlies. Since there the likelihood for hanging/crashing processes
is high, this feature is an ideal fit.

We also use it in other process scheduling roles, starting/observing
server processes etc.

>
> It's not something I've ever looked at so have no first hand knowledge.
> I wonder how the semantics of Windows Jobs compare to *NIX process
> groups? Do they map nicely?

Unfortunately no. They seem to hover more in cgroups territory - e.g.
limiting resource usage etc. But if all you want to do is to group
processes together and being able to kill them and their children in
one go, that would work with Windows Jobs. I am not sure about
suspending though, or sending to background, etc.

Personally I would much rather expose more Posix-like features,
because I want to see a shell written in Java at some point :) But the
group-killing ability is to us the most important aspect of this
proposal.

Cheers, Thomas

>
> Cheers,
> David
>
> > Thanks, Thomas
> > On Mon, Nov 12, 2018 at 6:29 PM Thomas Stüfe  
> > wrote:
> >>
> >> Dear all,
> >>
> >> may I please hear your thoughts about the following proposal?
> >>
> >> We would like to add support for process groups to the JDK: the
> >> ability to put child processes into new or pre-existing process
> >> groups. We added this feature to our proprietary port some time ago
> >> and has been very useful in cases where the VM acts in a process
> >> scheduling role.
> >>
> >> With process groups we mean of course standard Unix process groups.
> >> There exists a similar concept on Windows, Job Objects, so at least a
> >> subset of what we propose could be done in a platform independent way.
> >>
> >> 
> >>
> >> Motivation:
> >>
> >> Most importantly, the ability to safely terminate a group of processes.
> >>
> >> The established way to do this is, since Java 9, to iterate over a
> >> process tree, calling Process.children() or Process.descendants() on
> >> the root Process, and killing them using Process.destroy().
> >>
> >> In practice, that approach is not always a good fit. It leaves out any
> >> orphaned processes; any deceased non-leaf process in the tree makes
> >> its children unreachable. Worst case, if the root process dies, all
> >> children are orphaned and cannot be reached. Another limitation is
> >> that this only works for process trees - parent-child relationships -
> >> but not for unrelated processes one might want to group together. It
> >> also becomes a bit inefficient with many processes, requiring one JNI
> >> call/system call per process to kill.
> >>
> >> Process groups, OTOH, would allow us to group together any number of
> >> unrelated processes. We can then send them bulk signals, eg
> >> SIGTERM/SIGKILL with only one system call. And for that to work, the
> >> parent relationships do not matter, so we also reach processes which
> >> have been orphaned.
> >>
> >> There are more things one could do with process groups besides killing
> >> them: suspend/resume them together (SIGSTOP/CONT), or to send them to
> >> the background of the controlling terminal.
> >>
> >> In fact, one could write its own shell in Java :)
> >>
> >> 
> >>
> >> I drew up a tiny patch to demonstrate how this could look. This is
> >> just an example, to have something to play with and talk about:
> >>
> >> http://cr.openjdk.java.net/~stuefe/webrevs/processgroup-support/webrev.01/webrev/index.html
> >>
> >> and here is a small usage example:
> >>
> >> https://github.com/tstuefe/ojdk-repros/blob/master/src/other/RuntimeExecSimpleTestWithProcessGroup.java
> >>
> >> The suggested API changes are small:
> >>
> >> - A new class ProcessGroup as the platform's notion of a process
> >> group. In this patch, it offers four functions:
> >>- destroy()/destroyForcibly() terminate or kill the whole process group
> >>- suspend()/resume() puts them to sleep and wakes them up.
> >>More functionality could be added if needed. This mostly depends on
> >> how tightly we want to be bound by platform limitations on Windows,
> >> where process groups cannot be translated 1:1 to Job Objects.
> >>
> >> - ProcessBuilder has now two new attributes:
> >>- createProcessGroup() is a boolean flag directing the builder to
> >> let sub processes create their own process group, with themselves
> >> being the leader.
> >>- processGroup() is a reference to a ProcessGroup object; when not
> >> null, subprocesses will join that process group.
> >>
> >> - The Process class gets a new query method to retrieve a ProcessGroup
> >> object linked to its process group id.
> >>
> >> Using these building stones, a typical pattern could be:
> >>
> >> 
> >>  ProcessBuilder processBuilder = new 

Re: RFR: JDK-8214063: OpenJDK will not build on AIX while using the xlc 13.1 compiler

[Adam Farley8]

Hi Volker,

The NativeImageBuffer.cpp changes are best explained by the full text of 
the referenced GitHub Pull Request, copied here for simplicity:

-
Define JNIEXPORT and JNIIMPORT for xlc version 13.1 or newer. Without 
this, 
almost no symbols are exported from shared libraries due to use of 
-qvisibility=hidden as specified in make/lib/LibCommon.gmk. The symptoms 
are reported in eclipse/openj9#2468.

Unfortunately, this encounters a bug in xlc: it fails to parse what seems 
to be reasonable code. A workaround is required in just one place: 
src/java.base/share/native/libjimage/NativeImageBuffer.cpp.
-

Best Regards

Adam Farley 
IBM Runtimes


Volker Simonis  wrote on 20/11/2018 17:50:41:

> From: Volker Simonis 
> To: "Stuefe, Thomas" 
> Cc: adam.far...@uk.ibm.com, Java Core Libs 

> Date: 20/11/2018 17:59
> Subject: Re: RFR: JDK-8214063: OpenJDK will not build on AIX while 
> using the xlc 13.1 compiler
> 
> On Tue, Nov 20, 2018 at 6:15 PM Thomas Stüfe  
wrote:
> >
> > On Tue, Nov 20, 2018 at 6:12 PM Adam Farley8  
wrote:
> > >
> > > Heya Tom,
> > >
> > > "In JDK11 and JDK12, source files are compiled with 
-qvisibility=hidden
> > > when using xlc version other than 12.1. That doesn't seem to play 
well
> > > with link option -bexpall. "
> > >
> > > Found that buried in one of the associated Git issues. It appears 
that
> > > it's OpenJDK's use of that option that's causing the problem, though
> > > I couldn't speculate as to why it was added in the first place.
> > >
> > > I see this has also been noted in https://
> urldefense.proofpoint.com/v2/url?
> 
u=https-3A__bugs.openjdk.java.net_browse_JDK-2D8204541=DwIFaQ=jf_iaSHvJObTbx-
> siA1ZOg=P5m8KWUXJf-
> 
CeVJc0hDGD9AQ2LkcXDC0PMV9ntVw5Ho=SD6UdjysISJRBlWUm8pEzF5lRZ5opfbrKzEh_jrOras=5qDEdIfg8qZ-
> vCglsZ9qNDTEPMnCkj-mVPVah6eEDLE=
> > >
> > > Does that answer your question?
> > >
> >
> > Yes, Thank you. Odd. Will have to do archeology on that one.
> >
> 
> No I begin to understand the problem as well :)
> 
> It was actually change "8202322: AIX: symbol visibility flags not
> support on xlc 12.1" [1] which introduced "-qvisibility=hidden" for
> XLC version not equal to 12.1. That's kind of a weak check and I
> suppose nobody has ever tested this change with an XLC version other
> than 12.1 (until you came along :). Maybe that check should be a more
> precisly check for >= 13.1 (but I know such version checks are hard to
> do in Makefile syntax)?
> 
> The thing I don't understand about your patch (the changes in
> "jni_md.h" look good although I haven't tested them) is why you need
> the extra changes in NativeImageBuffer.cpp?
> "jdk_internal_jimage_NativeImageBuffer.h" is a plain, generated JNI
> header file. If XLC 13 has problems to parse it, there should be much
> more places which need fixing. I think that part of your change needs
> a closer evaluation.
> 
> Thank you and best regards,
> Volker
> 
> [1] INVALID URI REMOVED
> 
u=https-3A__bugs.openjdk.java.net_browse_JDK-2D8202322=DwIFaQ=jf_iaSHvJObTbx-
> siA1ZOg=P5m8KWUXJf-
> 
CeVJc0hDGD9AQ2LkcXDC0PMV9ntVw5Ho=SD6UdjysISJRBlWUm8pEzF5lRZ5opfbrKzEh_jrOras=JAEK6rePGMPinZzOquHBzj5oc7vA3kaFt9x0WIIUzvk=
> 
> > ..Thomas
> >
> > > Best Regards
> > >
> > > Adam Farley
> > > IBM Runtimes
> > >
> > >
> > > "Thomas Stüfe"  wrote on 20/11/2018 
16:44:07:
> > >
> > > > From: "Thomas Stüfe" 
> > > > To: Adam Farley8 
> > > > Cc: Java Core Libs 
> > > > Date: 20/11/2018 16:48
> > > > Subject: Re: RFR: JDK-8214063: OpenJDK will not build on AIX while
> > > > using the xlc 13.1 compiler
> > > >
> > > > Hi Adam,
> > > >
> > > > On Tue, Nov 20, 2018 at 5:12 PM Adam Farley8 
>  wrote:
> > > > >
> > > > > Hi Tom,
> > > > >
> > > > > Sounds reasonable. I've added a webex to the bug, and here's a
> > > > link to the bug.
> > > > >
> > > > > INVALID URI REMOVED
> > > > 
> 
u=https-3A__bugs.openjdk.java.net_browse_JDK-2D8214063=DwIFaQ=jf_iaSHvJObTbx-
> > > > siA1ZOg=P5m8KWUXJf-
> > > > 
> 
CeVJc0hDGD9AQ2LkcXDC0PMV9ntVw5Ho=z8YYwBXEfN7UtX1suPjpp9CZSHf8v0GrIMK3XGIC9VY=81TP9mIjhYD2Hmt8g7p2EHWRZXgiep21hxKLYRU7zIQ=
> > > > >
> > > > > This patch is required because otherwise, when building on AIX
> > > > using xlc 3.1,
> > > > > the build fails with this error:
> > > > >
> > > > > "Visibility is not allowed on a reference to an imported 
symbol."
> > > > >
> > > > > We believe this is caused by JNIEXPORT and JNIIMPORT not being
> > > > defined. Without
> > > > > this, almost no symbols are exported from shared libraries 
> due to use of
> > > > > -qvisibility=hidden as specified in make/lib/LibCommon.gmk.
> > > >
> > > > Yes but what I try to understand is why does this happen now with
> > > > xlc13? Did xlc change the rules for -qvisibility from v12 to v13 ?
> > > > That would be quite a break in backward compatibility.
> > > >
> > > > >
> > > > > For convenience, here's a summary of the diffs:
> > > > >
> > > > > 

Re: RFR: 8214077: test java/io/File/SetLastModified.java fails on ARM32

[Magnus Ihse Bursie]

On 2018-11-21 11:08, Nick Gasson wrote:


Hi Alan,


Have you looked at replacing the remaining usages of stat changed to
stat64 instead?

I've tried this just now and it also resolves the issue. I can
test on some more platforms and update the webrev if this is the
preferred solution?
I'd say this is preferred to adding compiler flags, yes. The code will 
make it unambiguously clear that it's correct.


/Magnus


Thanks,
Nick


-Original Message-
From: Alan Bateman 
Sent: Wednesday, November 21, 2018 4:55 PM
To: Nick Gasson ; build-dev ; core-libs-dev@openjdk.java.net
Cc: nd 
Subject: Re: RFR: 8214077: test java/io/File/SetLastModified.java fails on
ARM32

On 21/11/2018 02:46, Nick Gasson wrote:

Hi,

Can someone please help me review this small makefile patch to
fix an issue with java.io.File::setLastModified on 32-bit
systems?

https://bugs.openjdk.java.net/browse/JDK-8214077
http://cr.openjdk.java.net/~njian/8214077/webrev.0/

If the file size is > 2GB so that the size won't fit in a signed
32-bit off_t all stat() calls will fail with EOVERFLOW. This causes
the native method UnixFileSystem::setLastModifiedTime to fail as it
uses stat() to preserve the access time. It also causes other methods
like File::length and File::lastModified to return 0.


Have you looked at replacing the remaining usages of stat changed to
stat64 instead?

-Alan

Re: RFR: JDK-8214063: OpenJDK will not build on AIX while using the xlc 13.1 compiler

[Magnus Ihse Bursie]

On 2018-11-20 18:50, Volker Simonis wrote:

On Tue, Nov 20, 2018 at 6:15 PM Thomas Stüfe  wrote:

On Tue, Nov 20, 2018 at 6:12 PM Adam Farley8  wrote:

Heya Tom,

"In JDK11 and JDK12, source files are compiled with -qvisibility=hidden
when using xlc version other than 12.1. That doesn't seem to play well
with link option -bexpall. "

Found that buried in one of the associated Git issues. It appears that
it's OpenJDK's use of that option that's causing the problem, though
I couldn't speculate as to why it was added in the first place.

I see this has also been noted in 
https://bugs.openjdk.java.net/browse/JDK-8204541

Does that answer your question?


Yes, Thank you. Odd. Will have to do archeology on that one.


No I begin to understand the problem as well :)

It was actually change "8202322: AIX: symbol visibility flags not
support on xlc 12.1" [1] which introduced "-qvisibility=hidden" for
XLC version not equal to 12.1. That's kind of a weak check and I
suppose nobody has ever tested this change with an XLC version other
than 12.1 (until you came along :). Maybe that check should be a more
precisly check for >= 13.1 (but I know such version checks are hard to
do in Makefile syntax)?
In configure (where, ideally, all version checks should be made), 
there's the TOOLCHAIN_CHECK_COMPILER_VERSION function, which supports
#   IF_AT_LEAST:   block to run if the compiler is at least this version 
(>=)
#   IF_OLDER_THAN:   block to run if the compiler is older than this 
version (<)

for normal, dot-separated version number schemes.

/Magnus



The thing I don't understand about your patch (the changes in
"jni_md.h" look good although I haven't tested them) is why you need
the extra changes in NativeImageBuffer.cpp?
"jdk_internal_jimage_NativeImageBuffer.h" is a plain, generated JNI
header file. If XLC 13 has problems to parse it, there should be much
more places which need fixing. I think that part of your change needs
a closer evaluation.

Thank you and best regards,
Volker

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


..Thomas


Best Regards

Adam Farley
IBM Runtimes


"Thomas Stüfe"  wrote on 20/11/2018 16:44:07:


From: "Thomas Stüfe" 
To: Adam Farley8 
Cc: Java Core Libs 
Date: 20/11/2018 16:48
Subject: Re: RFR: JDK-8214063: OpenJDK will not build on AIX while
using the xlc 13.1 compiler

Hi Adam,

On Tue, Nov 20, 2018 at 5:12 PM Adam Farley8  wrote:

Hi Tom,

Sounds reasonable. I've added a webex to the bug, and here's a

link to the bug.

https://urldefense.proofpoint.com/v2/url?

u=https-3A__bugs.openjdk.java.net_browse_JDK-2D8214063=DwIFaQ=jf_iaSHvJObTbx-
siA1ZOg=P5m8KWUXJf-
CeVJc0hDGD9AQ2LkcXDC0PMV9ntVw5Ho=z8YYwBXEfN7UtX1suPjpp9CZSHf8v0GrIMK3XGIC9VY=81TP9mIjhYD2Hmt8g7p2EHWRZXgiep21hxKLYRU7zIQ=

This patch is required because otherwise, when building on AIX

using xlc 3.1,

the build fails with this error:

"Visibility is not allowed on a reference to an imported symbol."

We believe this is caused by JNIEXPORT and JNIIMPORT not being

defined. Without

this, almost no symbols are exported from shared libraries due to use of
-qvisibility=hidden as specified in make/lib/LibCommon.gmk.

Yes but what I try to understand is why does this happen now with
xlc13? Did xlc change the rules for -qvisibility from v12 to v13 ?
That would be quite a break in backward compatibility.


For convenience, here's a summary of the diffs:

--
File 1 of 2) src/java.base/share/native/libjimage/NativeImageBuffer.cpp

  #include "osSupport.hpp"

+#if defined(__xlC__) && (__xlC__ >= 0x0d01)
+/*
+ * Version 13.1.3 of xlc seems to have trouble parsing the `__attribute__`
+ * annotation in the generated header file we're about to

include. Repeating

+ * the forward declaration (without the braces) here avoids the diagnostic:
+ *   1540-0040 (S) The text "void" is unexpected.  "visibility"

may be undeclared or ambiguous.

+ */
+extern "C" JNIEXPORT jobject JNICALL

Java_jdk_internal_jimage_NativeImageBuffer_getNativeMap(JNIEnv *,
jclass, jstring);

+#endif
+
#include "jdk_internal_jimage_NativeImageBuffer.h"
--
File 2 of 2) src/java.base/unix/native/include/jni_md.h

  #define JNIIMPORT __attribute__((visibility("default")))
   #endif
+#elif defined(__xlC__) && (__xlC__ >= 0x0d01) /* xlc version 13.1

or better required */

+  #define JNIEXPORT   __attribute__((visibility("default")))
+  #define JNIIMPORT   __attribute__((visibility("default")))
#else
   #define JNIEXPORT
--


Thank you.

Cheers, Thomas


Best Regards

Adam Farley
IBM Runtimes


"Thomas Stüfe"  wrote on 19/11/2018 18:11:34:


From: "Thomas Stüfe" 
To: Adam Farley8 
Cc: Java Core Libs 
Date: 19/11/2018 18:12
Subject: Re: RFR: JDK-8214063: OpenJDK will not build on AIX while
using the xlc 13.1 compiler

Hi Adam,

could you please include link to the JBS issue and either link to the
patch/webrev or link to the webrev, or at the 

RE: RFR: 8214077: test java/io/File/SetLastModified.java fails on ARM32

[Nick Gasson]

Hi Alan,

> Have you looked at replacing the remaining usages of stat changed to
> stat64 instead?

I've tried this just now and it also resolves the issue. I can
test on some more platforms and update the webrev if this is the
preferred solution?

Thanks,
Nick

> -Original Message-
> From: Alan Bateman 
> Sent: Wednesday, November 21, 2018 4:55 PM
> To: Nick Gasson ; build-dev  d...@openjdk.java.net>; core-libs-dev@openjdk.java.net
> Cc: nd 
> Subject: Re: RFR: 8214077: test java/io/File/SetLastModified.java fails on
> ARM32
> 
> On 21/11/2018 02:46, Nick Gasson wrote:
> > Hi,
> >
> > Can someone please help me review this small makefile patch to
> > fix an issue with java.io.File::setLastModified on 32-bit
> > systems?
> >
> > https://bugs.openjdk.java.net/browse/JDK-8214077
> > http://cr.openjdk.java.net/~njian/8214077/webrev.0/
> >
> > If the file size is > 2GB so that the size won't fit in a signed
> > 32-bit off_t all stat() calls will fail with EOVERFLOW. This causes
> > the native method UnixFileSystem::setLastModifiedTime to fail as it
> > uses stat() to preserve the access time. It also causes other methods
> > like File::length and File::lastModified to return 0.
> >
> Have you looked at replacing the remaining usages of stat changed to
> stat64 instead?
> 
> -Alan

Re: Proposal: Add support for Process Groups to the JDK

[David Holmes]

On 21/11/2018 6:14 pm, Thomas Stüfe wrote:

Ping.. no-one has any thoughts on this?


I can see it being useful for test harnesses/frameworks.

It's not something I've ever looked at so have no first hand knowledge. 
I wonder how the semantics of Windows Jobs compare to *NIX process 
groups? Do they map nicely?


Cheers,
David


Thanks, Thomas
On Mon, Nov 12, 2018 at 6:29 PM Thomas Stüfe  wrote:


Dear all,

may I please hear your thoughts about the following proposal?

We would like to add support for process groups to the JDK: the
ability to put child processes into new or pre-existing process
groups. We added this feature to our proprietary port some time ago
and has been very useful in cases where the VM acts in a process
scheduling role.

With process groups we mean of course standard Unix process groups.
There exists a similar concept on Windows, Job Objects, so at least a
subset of what we propose could be done in a platform independent way.



Motivation:

Most importantly, the ability to safely terminate a group of processes.

The established way to do this is, since Java 9, to iterate over a
process tree, calling Process.children() or Process.descendants() on
the root Process, and killing them using Process.destroy().

In practice, that approach is not always a good fit. It leaves out any
orphaned processes; any deceased non-leaf process in the tree makes
its children unreachable. Worst case, if the root process dies, all
children are orphaned and cannot be reached. Another limitation is
that this only works for process trees - parent-child relationships -
but not for unrelated processes one might want to group together. It
also becomes a bit inefficient with many processes, requiring one JNI
call/system call per process to kill.

Process groups, OTOH, would allow us to group together any number of
unrelated processes. We can then send them bulk signals, eg
SIGTERM/SIGKILL with only one system call. And for that to work, the
parent relationships do not matter, so we also reach processes which
have been orphaned.

There are more things one could do with process groups besides killing
them: suspend/resume them together (SIGSTOP/CONT), or to send them to
the background of the controlling terminal.

In fact, one could write its own shell in Java :)



I drew up a tiny patch to demonstrate how this could look. This is
just an example, to have something to play with and talk about:

http://cr.openjdk.java.net/~stuefe/webrevs/processgroup-support/webrev.01/webrev/index.html

and here is a small usage example:

https://github.com/tstuefe/ojdk-repros/blob/master/src/other/RuntimeExecSimpleTestWithProcessGroup.java

The suggested API changes are small:

- A new class ProcessGroup as the platform's notion of a process
group. In this patch, it offers four functions:
   - destroy()/destroyForcibly() terminate or kill the whole process group
   - suspend()/resume() puts them to sleep and wakes them up.
   More functionality could be added if needed. This mostly depends on
how tightly we want to be bound by platform limitations on Windows,
where process groups cannot be translated 1:1 to Job Objects.

- ProcessBuilder has now two new attributes:
   - createProcessGroup() is a boolean flag directing the builder to
let sub processes create their own process group, with themselves
being the leader.
   - processGroup() is a reference to a ProcessGroup object; when not
null, subprocesses will join that process group.

- The Process class gets a new query method to retrieve a ProcessGroup
object linked to its process group id.

Using these building stones, a typical pattern could be:


 ProcessBuilder processBuilder = new ProcessBuilder(cmd);
 processBuilder.createProcessGroup(true);  <-- next process is pg leader

 Process leader = processBuilder.start();

 ProcessGroup pgr = leader.processGroup();  <-- retrieve newly
created process group
 processBuilder.processGroup(pgr); <-- next processes shall be
members of this process group too

 processBuilder.start();
 processBuilder.start();
 


and then call operations on the ProcessGroup object.



It is clear to me that this kind of change would require probably a
JEP, if it is desired at all. With this mail I just wanted to gauge
interest.

What do you think?

Thanks & Best Regards, Thomas

Re: RFR: 8214077: test java/io/File/SetLastModified.java fails on ARM32

[Alan Bateman]

On 21/11/2018 02:46, Nick Gasson wrote:

Hi,

Can someone please help me review this small makefile patch to
fix an issue with java.io.File::setLastModified on 32-bit
systems?

https://bugs.openjdk.java.net/browse/JDK-8214077
http://cr.openjdk.java.net/~njian/8214077/webrev.0/

If the file size is > 2GB so that the size won't fit in a signed
32-bit off_t all stat() calls will fail with EOVERFLOW. This causes
the native method UnixFileSystem::setLastModifiedTime to fail as it
uses stat() to preserve the access time. It also causes other methods
like File::length and File::lastModified to return 0.

Have you looked at replacing the remaining usages of stat changed to 
stat64 instead?


-Alan

Re: RFR 8177552: Compact Number Formatting support

[Nishit Jain]

Hi Naoto,

Updated the webrev based on suggestions

http://cr.openjdk.java.net/~nishjain/8177552/webrevs/webrev.01/

Changes made:
- Replaced List with String[] to be added to the the resource 
bundles
- refactored DecimalFormat.subparse() to be used by the CNF.parse(), to 
reduce code duplication.

- Also updated it with other changes as suggested in the comments

Regards,
Nishit Jain
On 20-11-2018 00:33, naoto.s...@oracle.com wrote:

Hi Nishit,

On 11/18/18 10:29 PM, Nishit Jain wrote:

Hi Naoto,

Please check my comments inline.

On 17-11-2018 04:52, naoto.s...@oracle.com wrote:

Hi Nishit,

Here are my comments:

- CLDRConverter: As the compact pattern no more employs 
List, can we eliminate stringListEntry/Element, and use 
Array equivalent instead?
Since the CNF design does not put any limit on the size of compact 
pattern, so at the time of parsing the CLDR xmls using SAX parser, it 
becomes difficult to identify the size of array when the parent 
element of compact pattern is encountered, so I think it is better to 
keep the List while extracting the resources.


OK. However I'd not keep the List format on generating the 
resource bundle, as there is no reason to introduce yet another bundle 
format other than the existing array of String.




- CompactNumberFormat.java

Multiple locations: Use StringBuilder instead of StringBuffer.

OK


line 268: The link points to NumberFormat.getNumberInstance(Locale) 
instead of DecimalFormat

OK. Changed it at line 165 also.


line 855: no need to do toString(). length() can detect whether it's 
empty or not.


line 884: "Overloaded method" reads odd here. I'd prefer 
specializing in the "given number" into either long or biginteger.

OK


line 1500: subparseNumber() pretty much shares the same code with 
DecimalFormat.subparse(). can they be merged?
The existing CNF.subParseNumber differs in the way parseIntegerOnly 
is handled, DecimalFormat.parse()/subparse() behaviour is 
unpredictable with parseIntegeronly = true when multipliers are 
involved (Please see JDK-8199223).


Also, I had thought that the CNF.parse()/subparseNumber() should *not 
*parse the exponential notation e.g. while parsing "1.05E4K" the 
parsing should break at 'E' and returns 1.05, because 'E' should be 
considered as unparseable character for general number format pattern 
or compact number pattern, but this is not the case with 
DecimalFormat.parse(). The below DecimalFormat general number format 
instance


NumberFormat nf =  NumberFormat.getNumberInstance();
nf.parse("1.05E4")

Successfully parse the string and returns 10500. The same behaviour 
is there with other DecimalFormat instances also e.g. currency instance.


Do you think this is an issue with DecimalFormat.parse() and CNF 
should avoid parsing exponential numbers? Or, should CNF.parse() be 
modified to be consistent with DecimalFormat.parse() in this aspect?


No, I understand there are differences. But I see a lot of duplicated 
piece of code which I would like to eliminate.






line 1913-1923, 1950-1960, 1987-1997, 2024-2034: It simply calls 
super. No need to override them.
Since setters are overridden, I think that it is better to override 
getters also (even if they are just calling super and have same 
javadoc) to keep them at same level. But, if you see no point in 
keeping them in CNF, I will remove them. Does that need CSR change?


I don't see any point for override. I don't think there needs a CSR, 
but better ask Joe about it.




line 2231: You need to test the type before cast. Otherwise 
ClassCastException may be thrown.
The type is checked in the superclass equals method getClass() != 
obj.getClass(), so I think there is no need to check the type here.


OK.

Naoto



Regards,
Nishit Jain


Naoto

On 11/16/18 9:54 AM, Nishit Jain wrote:

Hi,

Please review this non trivial feature addition to NumberFormat API.

The existing NumberFormat API provides locale based support for 
formatting and parsing numbers which includes formatting decimal, 
percent, currency etc, but the support for formatting a number into 
a human readable or compact form is missing. This RFE adds that 
feature to format a decimal number in a compact format (e.g. 1000 
-> 1K, 100 -> 1M in en_US locale) , which is useful for the 
environment where display space is limited, so that the formatted 
string can be displayed in that limited space. It is defined by 
LDML's specification for Compact Number Formats.


http://unicode.org/reports/tr35/tr35-numbers.html#Compact_Number_Formats 




RFE: https://bugs.openjdk.java.net/browse/JDK-8177552
Webrev: 
http://cr.openjdk.java.net/~nishjain/8177552/webrevs/webrev.00/

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

Request to please help review the the change.

Regards,
Nishit Jain

Re: Proposal: Add support for Process Groups to the JDK

[Thomas Stüfe]

Ping.. no-one has any thoughts on this?

Thanks, Thomas
On Mon, Nov 12, 2018 at 6:29 PM Thomas Stüfe  wrote:
>
> Dear all,
>
> may I please hear your thoughts about the following proposal?
>
> We would like to add support for process groups to the JDK: the
> ability to put child processes into new or pre-existing process
> groups. We added this feature to our proprietary port some time ago
> and has been very useful in cases where the VM acts in a process
> scheduling role.
>
> With process groups we mean of course standard Unix process groups.
> There exists a similar concept on Windows, Job Objects, so at least a
> subset of what we propose could be done in a platform independent way.
>
> 
>
> Motivation:
>
> Most importantly, the ability to safely terminate a group of processes.
>
> The established way to do this is, since Java 9, to iterate over a
> process tree, calling Process.children() or Process.descendants() on
> the root Process, and killing them using Process.destroy().
>
> In practice, that approach is not always a good fit. It leaves out any
> orphaned processes; any deceased non-leaf process in the tree makes
> its children unreachable. Worst case, if the root process dies, all
> children are orphaned and cannot be reached. Another limitation is
> that this only works for process trees - parent-child relationships -
> but not for unrelated processes one might want to group together. It
> also becomes a bit inefficient with many processes, requiring one JNI
> call/system call per process to kill.
>
> Process groups, OTOH, would allow us to group together any number of
> unrelated processes. We can then send them bulk signals, eg
> SIGTERM/SIGKILL with only one system call. And for that to work, the
> parent relationships do not matter, so we also reach processes which
> have been orphaned.
>
> There are more things one could do with process groups besides killing
> them: suspend/resume them together (SIGSTOP/CONT), or to send them to
> the background of the controlling terminal.
>
> In fact, one could write its own shell in Java :)
>
> 
>
> I drew up a tiny patch to demonstrate how this could look. This is
> just an example, to have something to play with and talk about:
>
> http://cr.openjdk.java.net/~stuefe/webrevs/processgroup-support/webrev.01/webrev/index.html
>
> and here is a small usage example:
>
> https://github.com/tstuefe/ojdk-repros/blob/master/src/other/RuntimeExecSimpleTestWithProcessGroup.java
>
> The suggested API changes are small:
>
> - A new class ProcessGroup as the platform's notion of a process
> group. In this patch, it offers four functions:
>   - destroy()/destroyForcibly() terminate or kill the whole process group
>   - suspend()/resume() puts them to sleep and wakes them up.
>   More functionality could be added if needed. This mostly depends on
> how tightly we want to be bound by platform limitations on Windows,
> where process groups cannot be translated 1:1 to Job Objects.
>
> - ProcessBuilder has now two new attributes:
>   - createProcessGroup() is a boolean flag directing the builder to
> let sub processes create their own process group, with themselves
> being the leader.
>   - processGroup() is a reference to a ProcessGroup object; when not
> null, subprocesses will join that process group.
>
> - The Process class gets a new query method to retrieve a ProcessGroup
> object linked to its process group id.
>
> Using these building stones, a typical pattern could be:
>
> 
> ProcessBuilder processBuilder = new ProcessBuilder(cmd);
> processBuilder.createProcessGroup(true);  <-- next process is pg 
> leader
>
> Process leader = processBuilder.start();
>
> ProcessGroup pgr = leader.processGroup();  <-- retrieve newly
> created process group
> processBuilder.processGroup(pgr); <-- next processes shall be
> members of this process group too
>
> processBuilder.start();
> processBuilder.start();
> 
> 
>
> and then call operations on the ProcessGroup object.
>
> 
>
> It is clear to me that this kind of change would require probably a
> JEP, if it is desired at all. With this mail I just wanted to gauge
> interest.
>
> What do you think?
>
> Thanks & Best Regards, Thomas