RFR(L/M) : 8210112 : remove jdk.testlibrary.ProcessTools

[Igor Ignatyev]

http://cr.openjdk.java.net/~iignatyev//8210112/webrev.00/index.html
> 2375 lines changed: 322 ins; 1662 del; 391 mod

Hi all,

could you please review the patch which removes jdk.testlibrary.ProcessTools 
and its friends and replaces all theirs usages w/ corresponding classes from 
jdk.test.lib.process?

there were a few differences b/w implementations which are addressed by the 
patch:
 - j.t.l.p.ProcessTools missed executeProcess(ProcessBuilder, String) method
 - j.t.l.p.OutputAnalyzer didn't have shouldMatchByLine methods family
 - j.t.l.p.OutputBuffer was a very rudimentary and didn't serve any purposes, 
while j.t.OutputBuffer provided lazy access to a process's cout, cerr and 
exitcode. I have changed j.t.l.p.OutputBuffer to be an interface w/ two 
implementations LazyOutputBuffer and EagerOutputBuffer, and updated 
j.t.l.p.OutputAnalyzer to get values from an OutputBuffer instead of storing 
them.
 - j.t.l.p.ProcessTools::createJavaProcessBuilder always adds '-cp', but 
j.t.ProcessTools::createJavaProcessBuilder did not. I have identified tests 
which really depend on absence of '-cp' and updated them to create 
ProcessBuilder directly, namely JavaClassPathTest and 
AppendToClassPathModuleTest.

the rest of the patch is straightforward change of used classes w/ adding 
@library /test/lib if necessary and removing @library /lib/testlibrary if 
possible.  

webrev: http://cr.openjdk.java.net/~iignatyev//8210112/webrev.00/index.html
testing: tier1-tier3 + :jdk_svc
JBS: https://bugs.openjdk.java.net/browse/JDK-8210112

Thanks,
-- Igor 

Re: RFR: 8139965 - Hang seen when using com.sun.jndi.ldap.search.replyQueueSize

[Rob McKenna]

Thanks for the reviews folks.

I believe the following captures your recommended changes:

http://cr.openjdk.java.net/~robm/8139965/webrev.02/

W.r.t. testing I think this area has been difficult to test
traditionally. I'll have a dig through the existing testbase (and I'll
get back to you) to see if there's anything similar but afaik most tests
simply mimic a bindable dummy ldap server.

Vyom, are you aware of any more rigorous tests / ldap test frameworks?

-Rob

On 04/09/18 10:22, Daniel Fuchs wrote:
> Hi Rob,
> 
> I concur with Chris.
> completed needs to be volatile and close() needs to
> set a flag and use offer like cancel().
> 
> The condition for testing for closed then becomes
> that the flag is set and the queue is empty or has EOF
> as its head.
> 
> Is there any way this could be tested by a regression
> test?
> 
> best regards,
> 
> -- daniel
> 
> On 04/09/2018 10:00, Chris Hegarty wrote:
> > Rob,
> > 
> > > On 3 Sep 2018, at 22:48, Rob McKenna  wrote:
> > > 
> > > Hi folks,
> > > 
> > > I'd like to get a re-review of this change:
> > > 
> > > https://bugs.openjdk.java.net/browse/JDK-8139965 
> > > 
> > 
> > This issue is closed as `will not fix`. I presume you will re-open it 
> > before pushing.
> > 
> > > http://cr.openjdk.java.net/~robm/8139965/webrev/ 
> > > 
> > 
> > 
> > 43 private boolean completed;
> > Won’t `completed` need to be volatile now? ( since the removal of 
> > synchronized from hasSearchCompleted )
> > 
> > LdapRequest.close puts EOF into the queue, but that is a potentially 
> > blocking operation ( while holding the lock ). If the queue is at capacity, 
> > then it will block forever. This model will only work if `getReplyBer` is 
> > always guaranteed to be running concurrently. Is it?
> > 
> > Please check the indentation of LdapRequest.java L103 ( in
> > the new file ). It appears, in the webrev, that the trailing `}` is
> > not lined up.
> > 
> > The indentation doesn’t look right here either.
> >   624 if (nparent) {
> >   625 LdapRequest ldr = pendingRequests;
> >   626 while (ldr != null) {
> >   627 ldr.close();
> >   628 ldr = ldr.next;
> >   629 }
> >   630 }
> >   631 }
> > 
> > -Chris
> > 
> 

Re: RFR 8098798: Thread.join(ms) on Linux still affected by changes to the time-of-day clock

[Martin Buchholz]

On Tue, Sep 4, 2018 at 12:02 PM, Roger Riggs  wrote:

>
>> One sharp corner is that wait(0) waits forever, and TimeUnit conversion
>> truncates.  You can probably avoid those problems via TimeUnit.timedWait.
>>
>> Not trivial since a long cannot hold the combined time of millis(long)
> and nanos (long) in a TimeUnit(Nanos)
> and the cumulative wait time needs to be measured by System.nanoTime().


This sort of code is never trivial, but ... in j.u.c. we usually convert to
nanos as soon as we can.  Yes, we lose some range for large inputs - we
saturate to 2^63 nanoseconds.  But 292 years ought to be enough for ...
people calling wait, if not for paleontologists.   And nanoTime will wrap
around then anyways.

Re: RFR(M) : 8210039 : move OSInfo to top level testlibrary

[Igor Ignatyev]

Daniel, Chris, Alan,

thank you for your review.

I've pushed it w/ Platfform::privilegedGetProperty method added to shorten 
lines in Platform.java

Cheers,
-- Igor


> On Sep 4, 2018, at 6:08 AM, Daniel Fuchs  wrote:
> 
> Hi Igor,
> 
> Nit: You could have introduced a
> private static String getProperty(String name) {
> return AccessController.doP
> }
> in Platform.java to avoid the long lines.
> 
> Otherwise looks good to me too!
> 
> best regards,
> 
> -- daniel
> 
> On 31/08/2018 19:42, Igor Ignatyev wrote:
>> Alan, Chris,
>> thanks for looking at it, I went w/ the alternative suggested by Chris. that 
>> required a sprinkle of doPrivileged in the testlibrary, but now 
>> Sockets/policy.* files grant the minimal required permissions to the test 
>> code.
>> the incremental webrev can found here[1], please let me know if you need the 
>> whole webrev.
>> [1] http://cr.openjdk.java.net/~iignatyev//8210039/webrev.0-1/index.html
>> Thanks,
>> -- Igor
>>> On Aug 30, 2018, at 3:28 AM, Chris Hegarty  wrote:
>>> 
 
 On 30 Aug 2018, at 08:51, Alan Bateman  wrote:
 
 On 28/08/2018 17:50, Igor Ignatyev wrote:
> http://cr.openjdk.java.net/~iignatyev//8210039/webrev.00/index.html
>> 698 lines changed: 114 ins; 240 del; 344 mod
> Hi all,
> 
> could you please review this clean up of jdk testlibrary?
> the patch updates the tests to use jdk.test.lib.Platform instead of 
> jdk.testlibrary.OSInfo.OSType, cleans up OSInfo and renames it to 
> jdk.test.lib.OSVersion.
> 
> testing: changed tests + :jdk_desktop test group
> webrev: 
> http://cr.openjdk.java.net/~iignatyev//8210039/webrev.00/index.html
> JBS: https://bugs.openjdk.java.net/browse/JDK-8210039
> 
 The updates to the Sockets policy file suggests using this 
 jdk.test.lib.Platform/OSVersion requires permissions that the test 
 infrastructure needs, not the test. It's not wrong but it's always a 
 concern when tests running with a security manager are granted non-obvious 
 permissions.
>>> 
>>> The uses of test libraries with security manager is a little
>>> cumbersome, and usually ends up with the test code being
>>> granted more permissions than is necessary. I share Alan’s
>>> concern.
>>> 
>>> Another alternative, that we used in other areas, is to grant
>>> the test library only minimal permissions, separate to the
>>> actual test code. For example:
>>> 
>>> http://hg.openjdk.java.net/jdk/jdk/file/9183040e34d8/test/jdk/java/net/httpclient/AsFileDownloadTest.policy#l24
>>>  
>>> 
>>> 
>>> -Chris.
> 

Re: [12] RFR: 8210142: java.util.Calendar.clone() doesn't respect sharedZone flag

[Naoto Sato]

Hi Roger,

I tentatively tried to return a shared zone within a cloned Calendar. 
One test failed: java/util/Calendar/CalendarRegression.Test4136399, 
where it makes sure that the cloned Calendar object hash code should be 
different for the better distribution. Although the comment does not 
reflect the current implementation (getTimeZone() returning cloned 
zone), the intention here seems to have the better hash distribution for 
cloned objects.


Naoto

On 9/4/18 1:41 PM, Roger Riggs wrote:

Hi Naoto,

That access via reflection is going to go away sometime; so I'm not too 
concerned about

maintaining compatibility of the internal implementation.
I think I'd rather see the memory savings, however small.
Let see if anyone else has a recommendation.

Roger


On 9/4/18 4:12 PM, Naoto Sato wrote:

Hi Roger,

Yes, I considered that too, but did not change the behavior and just 
to maintain the field consistent. I agree that it would not be 
observable via the public Calendar API, but some apps (like how the 
submitter found it) may be doing something using reflection.


Naoto

On 9/4/18 12:31 PM, Roger Riggs wrote:

Hi Naoto,

The spec for clone doesn't say whether the clone should share or not 
share the TimeZone.
Did you consider that if sharedZone was true , *not* to clone the 
TimeZone?

It would still get cloned when requested from getTimeZone().

This does seem somewhat safer not to change the cloning behavior but 
I don't think the behavior would be observable.


The current code and test is fine, except for reducing the potential 
for sharing the TimeZone.


Thanks, Roger

On 9/4/2018 2:14 PM, Naoto Sato wrote:

Hello,

Please review the fix to the following issue:

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

The proposed fix is located at:

http://cr.openjdk.java.net/~naoto/8210142/webrev.00/

The fix is a simple one line change, which is to make the sharedZone 
field consistent with the cloned TimeZone instance in Calendar.clone().


Naoto

Re: [12] RFR: 8210142: java.util.Calendar.clone() doesn't respect sharedZone flag

[Roger Riggs]

Hi Naoto,

That access via reflection is going to go away sometime; so I'm not too 
concerned about

maintaining compatibility of the internal implementation.
I think I'd rather see the memory savings, however small.
Let see if anyone else has a recommendation.

Roger


On 9/4/18 4:12 PM, Naoto Sato wrote:

Hi Roger,

Yes, I considered that too, but did not change the behavior and just 
to maintain the field consistent. I agree that it would not be 
observable via the public Calendar API, but some apps (like how the 
submitter found it) may be doing something using reflection.


Naoto

On 9/4/18 12:31 PM, Roger Riggs wrote:

Hi Naoto,

The spec for clone doesn't say whether the clone should share or not 
share the TimeZone.
Did you consider that if sharedZone was true , *not* to clone the 
TimeZone?

It would still get cloned when requested from getTimeZone().

This does seem somewhat safer not to change the cloning behavior but 
I don't think the behavior would be observable.


The current code and test is fine, except for reducing the potential 
for sharing the TimeZone.


Thanks, Roger

On 9/4/2018 2:14 PM, Naoto Sato wrote:

Hello,

Please review the fix to the following issue:

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

The proposed fix is located at:

http://cr.openjdk.java.net/~naoto/8210142/webrev.00/

The fix is a simple one line change, which is to make the sharedZone 
field consistent with the cloned TimeZone instance in Calendar.clone().


Naoto

Re: [12] RFR: 8210142: java.util.Calendar.clone() doesn't respect sharedZone flag

[Naoto Sato]

Hi Roger,

Yes, I considered that too, but did not change the behavior and just to 
maintain the field consistent. I agree that it would not be observable 
via the public Calendar API, but some apps (like how the submitter found 
it) may be doing something using reflection.


Naoto

On 9/4/18 12:31 PM, Roger Riggs wrote:

Hi Naoto,

The spec for clone doesn't say whether the clone should share or not 
share the TimeZone.

Did you consider that if sharedZone was true , *not* to clone the TimeZone?
It would still get cloned when requested from getTimeZone().

This does seem somewhat safer not to change the cloning behavior but I 
don't think the behavior would be observable.


The current code and test is fine, except for reducing the potential for 
sharing the TimeZone.


Thanks, Roger

On 9/4/2018 2:14 PM, Naoto Sato wrote:

Hello,

Please review the fix to the following issue:

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

The proposed fix is located at:

http://cr.openjdk.java.net/~naoto/8210142/webrev.00/

The fix is a simple one line change, which is to make the sharedZone 
field consistent with the cloned TimeZone instance in Calendar.clone().


Naoto

Re: [PATCH] Simplification of TreeMap

[Michael Kuhlmann]

Hi Sergey,

I was also wondering why TreeMap doesn't just use a default comparator
and always checks for null instead.

The problem with your patch is that deserialized TreeMap instances which
were serialized from a previous version would still have the comparator
set to null, thus resulting in a NPE after your patch. And you can't
easily fix this because comparator is a final field.

Best,
Michael


On 04.09.2018 21:14, Сергей Цыпанов wrote:
> Hi,
> 
> currently (latest code of JDK 11) an instance of TreeMap created with no-arg 
> contructor has nullable comparator i.e. utilizes no comparator.
> 
> As it comes from the code, a TreeMap created with nullable comparator works 
> exactly as a TreeMap created with comparator provided by 
> Comparator.naturalOrder(). This is also explicitly specifid in Javadoc.
> 
> I propose to initialize default comparator of TreeMap with instance of 
> Comparator returned by Comparator.naturalOrder() instead of null.
> This allows to remove the code responsible for handling nullable comparator, 
> e. g. TreeMap::getEntryUsingComparator can be completely removed in favour of 
> TreeMap::getEntry.
> Similar simplification available for TreeMap::put, TreeMap::compare, 
> EntrySpliterator::getComparator.
> 
> I've prepared a patch for this.
> The patch contains both described major change and some tiny clean-ups e. g. 
> utilization of Objects::requireNonNull where appropriate and Objects::equals 
> instead of hand-written TreeMap::valEquals.
> 
> TreeMapTest is green after my changes.
> 
> Regards,
> Sergey Tsypanov
> 
> 

Re: [PATCH] Simplification of TreeMap

[Remi Forax]

Hi Sergey,
if think the code is as it is because it's faster to check for the null 
comparator and have an ad-hoc code for this special case than using 
Comparator.naturalOrder(),
obviously, it's not may be true anymore, so you should test that there is no 
perf regression when creating your patch.

regards,
Rémi

- Mail original -
> De: "Сергей Цыпанов" 
> À: "core-libs-dev" 
> Envoyé: Mardi 4 Septembre 2018 21:14:21
> Objet: [PATCH] Simplification of TreeMap

> Hi,
> 
> currently (latest code of JDK 11) an instance of TreeMap created with no-arg
> contructor has nullable comparator i.e. utilizes no comparator.
> 
> As it comes from the code, a TreeMap created with nullable comparator works
> exactly as a TreeMap created with comparator provided by
> Comparator.naturalOrder(). This is also explicitly specifid in Javadoc.
> 
> I propose to initialize default comparator of TreeMap with instance of
> Comparator returned by Comparator.naturalOrder() instead of null.
> This allows to remove the code responsible for handling nullable comparator, 
> e.
> g. TreeMap::getEntryUsingComparator can be completely removed in favour of
> TreeMap::getEntry.
> Similar simplification available for TreeMap::put, TreeMap::compare,
> EntrySpliterator::getComparator.
> 
> I've prepared a patch for this.
> The patch contains both described major change and some tiny clean-ups e. g.
> utilization of Objects::requireNonNull where appropriate and Objects::equals
> instead of hand-written TreeMap::valEquals.
> 
> TreeMapTest is green after my changes.
> 
> Regards,
> Sergey Tsypanov

Re: RFR 8098798: Thread.join(ms) on Linux still affected by changes to the time-of-day clock

[Ivan Gerasimov]

Thank you Roger!

I'm not sure if it plays any significant role, but an unnecessary call 
to nanoTime() after wait(delay) could be avoided with the code like this:


if (millis > 0) {
if (isAlive()) {
final long startTime = System.nanoTime();
long delay = millis;
do {
wait(delay);
} while (isAlive() && (delay = millis - 
TimeUnit.NANOSECONDS.toMillis(System.nanoTime() - startTime)) > 0);

}
} else if (millis == 0) {


With kind regards,
Ivan


On 9/4/18 12:02 PM, Roger Riggs wrote:

Hi Martin, Ivan,

Thanks for the suggestions.

Update in place:
http://cr.openjdk.java.net/~rriggs/webrev-thread-early-8098798/

On 8/29/2018 5:36 PM, Martin Buchholz wrote:

Thanks for taking this on.
Wait loops are notoriously hard to get right...

One sharp corner is that wait(0) waits forever, and TimeUnit 
conversion truncates.  You can probably avoid those problems via 
TimeUnit.timedWait.


Not trivial since a long cannot hold the combined time of millis(long) 
and nanos (long) in a TimeUnit(Nanos)

and the cumulative wait time needs to be measured by System.nanoTime().

In code like this in j.u.c. we try to optimize away the call to 
nanoTime when waiting is not necessary, by using a special 
"uninitialized" initial value for remaining nanos, e.g. in 
FutureTask.awaitDone


long startTime = 0L;// Special value 0L means not yet parked

ok


(I prefer the variable name "startTime")

ok


(j.u.c. code can also be improved)

(there's a pre-existing buglet - we should probably check for 
overflow when millis = MAX_VALUE and nanos > 0 (sigh))

ok,


(I would reorder clauses to first check the common case millis > 0, 
then millis == 0, last the error case millis < 0)

ok


(it's a bad API that millis < 0 is an error)
too late for a behavior change though I suppose its in the direction 
of not getting error instead of the opposite



An Observation:

Join(ms) and join(ms, ns) might wait a bit longer than strictly 
necessary because the bottom out in Object.wait(ms).

It might be better if both ended up calling Object.wait(ms, ns).
But since Object.wait(ms,ns) rounds up to Object.wait(ms) that won't 
make a difference and to take advantage
of a finer clock resolution would mean native/vm changes to support 
object.wait(ms, ns).


I'm inclined to address only the immediate issues raised in the early 
return and the clock dependency now.


(BTW, I found no tests for Thread.sleep or .join.)

Thanks, Roger




On Wed, Aug 29, 2018 at 1:06 PM, Roger Riggs > wrote:


Please review changes for:

8098798: Thread.join(ms) on Linux still affected by changes to the
time-of-day clock
 Use System.nanoTime to compute any remaining delay

8210004: Thread.sleep(millis, nanos) timeout returns early
 Avoid an early return by rounding the milliseconds up if
there are any nanoseconds as was done in Object.wait(ms, ns).

(If its not appropriate to do the two reviews together, I can
split them).

Webrev:
http://cr.openjdk.java.net/~rriggs/webrev-thread-early-8098798/


Thanks, Roger







--
With kind regards,
Ivan Gerasimov

Re: [12] RFR: 8210142: java.util.Calendar.clone() doesn't respect sharedZone flag

[Roger Riggs]

Hi Naoto,

The spec for clone doesn't say whether the clone should share or not 
share the TimeZone.

Did you consider that if sharedZone was true , *not* to clone the TimeZone?
It would still get cloned when requested from getTimeZone().

This does seem somewhat safer not to change the cloning behavior but I 
don't think the behavior would be observable.


The current code and test is fine, except for reducing the potential for 
sharing the TimeZone.


Thanks, Roger

On 9/4/2018 2:14 PM, Naoto Sato wrote:

Hello,

Please review the fix to the following issue:

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

The proposed fix is located at:

http://cr.openjdk.java.net/~naoto/8210142/webrev.00/

The fix is a simple one line change, which is to make the sharedZone 
field consistent with the cloned TimeZone instance in Calendar.clone().


Naoto

[PATCH] Simplification of TreeMap

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

Hi,

currently (latest code of JDK 11) an instance of TreeMap created with no-arg 
contructor has nullable comparator i.e. utilizes no comparator.

As it comes from the code, a TreeMap created with nullable comparator works 
exactly as a TreeMap created with comparator provided by 
Comparator.naturalOrder(). This is also explicitly specifid in Javadoc.

I propose to initialize default comparator of TreeMap with instance of 
Comparator returned by Comparator.naturalOrder() instead of null.
This allows to remove the code responsible for handling nullable comparator, e. 
g. TreeMap::getEntryUsingComparator can be completely removed in favour of 
TreeMap::getEntry.
Similar simplification available for TreeMap::put, TreeMap::compare, 
EntrySpliterator::getComparator.

I've prepared a patch for this.
The patch contains both described major change and some tiny clean-ups e. g. 
utilization of Objects::requireNonNull where appropriate and Objects::equals 
instead of hand-written TreeMap::valEquals.

TreeMapTest is green after my changes.

Regards,
Sergey Tsypanov


diff --git a/src/java.base/share/classes/java/util/TreeMap.java b/src/java.base/share/classes/java/util/TreeMap.java
--- a/src/java.base/share/classes/java/util/TreeMap.java
+++ b/src/java.base/share/classes/java/util/TreeMap.java
@@ -133,6 +133,11 @@
 private transient int modCount = 0;
 
 /**
+ * Comparator used by default.
+ */
+private static final Comparator DEFAULT_COMPARATOR = Comparator.naturalOrder();
+
+/**
  * Constructs a new, empty tree map, using the natural ordering of its
  * keys.  All keys inserted into the map must implement the {@link
  * Comparable} interface.  Furthermore, all such keys must be
@@ -145,7 +150,7 @@
  * {@code ClassCastException}.
  */
 public TreeMap() {
-comparator = null;
+comparator = DEFAULT_COMPARATOR;
 }
 
 /**
@@ -163,7 +168,7 @@
  *ordering} of the keys will be used.
  */
 public TreeMap(Comparator comparator) {
-this.comparator = comparator;
+this.comparator = comparator == null ? DEFAULT_COMPARATOR : comparator;
 }
 
 /**
@@ -181,7 +186,7 @@
  * @throws NullPointerException if the specified map is null
  */
 public TreeMap(Map m) {
-comparator = null;
+this();
 putAll(m);
 }
 
@@ -195,7 +200,8 @@
  * @throws NullPointerException if the specified map is null
  */
 public TreeMap(SortedMap m) {
-comparator = m.comparator();
+Comparator comparator = m.comparator();
+this.comparator = comparator == null ? DEFAULT_COMPARATOR : comparator;
 try {
 buildFromSorted(m.size(), m.entrySet().iterator(), null, null);
 } catch (java.io.IOException | ClassNotFoundException cannotHappen) {
@@ -246,7 +252,7 @@
  */
 public boolean containsValue(Object value) {
 for (Entry e = getFirstEntry(); e != null; e = successor(e))
-if (valEquals(value, e.value))
+if (Objects.equals(value, e.value))
 return true;
 return false;
 }
@@ -312,7 +318,7 @@
 int mapSize = map.size();
 if (size==0 && mapSize!=0 && map instanceof SortedMap) {
 Comparator c = ((SortedMap)map).comparator();
-if (c == comparator || (c != null && c.equals(comparator))) {
+if (Objects.equals(c, comparator)) {
 ++modCount;
 try {
 buildFromSorted(mapSize, map.entrySet().iterator(),
@@ -338,16 +344,14 @@
  * does not permit null keys
  */
 final Entry getEntry(Object key) {
-// Offload comparator-based version for sake of performance
-if (comparator != null)
-return getEntryUsingComparator(key);
-if (key == null)
+if (key == null && comparator == DEFAULT_COMPARATOR)
 throw new NullPointerException();
 @SuppressWarnings("unchecked")
-Comparable k = (Comparable) key;
+K k = (K) key;
+Comparator cpr = comparator;
 Entry p = root;
 while (p != null) {
-int cmp = k.compareTo(p.key);
+int cmp = cpr.compare(k, p.key);
 if (cmp < 0)
 p = p.left;
 else if (cmp > 0)
@@ -359,31 +363,6 @@
 }
 
 /**
- * Version of getEntry using comparator. Split off from getEntry
- * for performance. (This is not worth doing for most methods,
- * that are less dependent on comparator performance, but is
- * worthwhile here.)
- */
-final Entry getEntryUsingComparator(Object key) {
-@SuppressWarnings("unchecked")
-K k = (K) key;
-Comparator cpr = comparator;
-if (cpr != null) {
-Entry p = root;
-while (p != null) {
-int cmp = cpr.compare(k, p.key);
-if (cmp < 0)
-p = p.left;
-  

Re: RFR 8098798: Thread.join(ms) on Linux still affected by changes to the time-of-day clock

[Roger Riggs]

Hi Martin, Ivan,

Thanks for the suggestions.

Update in place:
http://cr.openjdk.java.net/~rriggs/webrev-thread-early-8098798/

On 8/29/2018 5:36 PM, Martin Buchholz wrote:

Thanks for taking this on.
Wait loops are notoriously hard to get right...

One sharp corner is that wait(0) waits forever, and TimeUnit 
conversion truncates.  You can probably avoid those problems via 
TimeUnit.timedWait.


Not trivial since a long cannot hold the combined time of millis(long) 
and nanos (long) in a TimeUnit(Nanos)

and the cumulative wait time needs to be measured by System.nanoTime().

In code like this in j.u.c. we try to optimize away the call to 
nanoTime when waiting is not necessary, by using a special 
"uninitialized" initial value for remaining nanos, e.g. in 
FutureTask.awaitDone


        long startTime = 0L;    // Special value 0L means not yet parked

ok


(I prefer the variable name "startTime")

ok


(j.u.c. code can also be improved)

(there's a pre-existing buglet - we should probably check for overflow 
when millis = MAX_VALUE and nanos > 0 (sigh))

ok,


(I would reorder clauses to first check the common case millis > 0, 
then millis == 0, last the error case millis < 0)

ok


(it's a bad API that millis < 0 is an error)
too late for a behavior change though I suppose its in the direction of 
not getting error instead of the opposite



An Observation:

Join(ms) and join(ms, ns) might wait a bit longer than strictly 
necessary because the bottom out in Object.wait(ms).

It might be better if both ended up calling Object.wait(ms, ns).
But since Object.wait(ms,ns) rounds up to Object.wait(ms) that won't 
make a difference and to take advantage
of a finer clock resolution would mean native/vm changes to support 
object.wait(ms, ns).


I'm inclined to address only the immediate issues raised in the early 
return and the clock dependency now.


(BTW, I found no tests for Thread.sleep or .join.)

Thanks, Roger




On Wed, Aug 29, 2018 at 1:06 PM, Roger Riggs > wrote:


Please review changes for:

8098798: Thread.join(ms) on Linux still affected by changes to the
time-of-day clock
 Use System.nanoTime to compute any remaining delay

8210004: Thread.sleep(millis, nanos) timeout returns early
 Avoid an early return by rounding the milliseconds up if
there are any nanoseconds as was done in Object.wait(ms, ns).

(If its not appropriate to do the two reviews together, I can
split them).

Webrev:
http://cr.openjdk.java.net/~rriggs/webrev-thread-early-8098798/


Thanks, Roger

Re: RFR 8210285 : CharsetDecoder/Encoder's constructor does not reject NaN

[Ivan Gerasimov]

On 9/4/18 9:40 AM, joe darcy wrote:

Hello,

Please also create a quick CSR to cover the behavioral change.


Okay, here it is:
https://bugs.openjdk.java.net/browse/JDK-8210377

Would you please help review it?

Thanks in advance!

Ivan



Thanks,

-Joe


On 8/31/2018 5:17 PM, Ivan Gerasimov wrote:

Hello!

The javadoc for CharsetDecoder [1] states that an exception is thrown 
when a non-positive number is passed in as an argument.


However we only reject negative or zero numbers, but not NaN.

And likewise for CharsetEncoder.

Would you please help review a trivial fix?

BUGURL: https://bugs.openjdk.java.net/browse/JDK-8210285
WEBREV: http://cr.openjdk.java.net/~igerasim/8210285/00/webrev/

Thanks in advance!

[1] 
https://download.java.net/java/early_access/jdk11/docs/api/java.base/java/nio/charset/CharsetDecoder.html#%3Cinit%3E(java.nio.charset.Charset,float,float)







--
With kind regards,
Ivan Gerasimov

[12] RFR: 8210142: java.util.Calendar.clone() doesn't respect sharedZone flag

[Naoto Sato]

Hello,

Please review the fix to the following issue:

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

The proposed fix is located at:

http://cr.openjdk.java.net/~naoto/8210142/webrev.00/

The fix is a simple one line change, which is to make the sharedZone 
field consistent with the cloned TimeZone instance in Calendar.clone().


Naoto

Re: RFR 8210285 : CharsetDecoder/Encoder's constructor does not reject NaN

[Ivan Gerasimov]

Thanks everyone for reviewing!

As suggested, I added a test to cover CharsetEncoder constructor and a 
sanity check that the constructor doesn't throw given valid arguments.


http://cr.openjdk.java.net/~igerasim/8210285/01/webrev/

With kind regards,

Ivan


On 9/3/18 8:31 AM, Martin Buchholz wrote:

Looks good to me.  I would add a call to
new MyDecoder(ascii, 0.5f, 1.5f)
to make sure all calls to the constructor don't throw (because e.g. 
for ASCII we know the correct values are 1.0f).


(In any case it feels like an API design mistake - the Charset itself 
should be the source of truth about charsPerByte)


On Mon, Sep 3, 2018 at 3:57 AM, Alan Bateman > wrote:


On 03/09/2018 05:54, Ivan Gerasimov wrote:

Thanks Sherman and Stuart for the review!

On 9/2/18 2:45 PM, Stuart Marks wrote:

Yes, the fix itself looks fine. Quite subtle, good catch.

But should this have a regression test? I can imagine
somebody coming along later and "simplifying" (!(... >
...)) to (... <= ...) which would reintroduce the bug.

Yes, there is a regression test added:

http://cr.openjdk.java.net/~igerasim/8210285/00/webrev/test/jdk/java/nio/charset/CharsetDecoder/NaNinCtor.java.html




The update to Charset-X-Coder.java.template looks okay. The
changes also CharsetEncoder so we'll need test coverage for that too.

-Alan




--
With kind regards,
Ivan Gerasimov

Re: [12] RFR of JDK-8209832: Refactor jdk/internal/reflect/Reflection/GetCallerClassTest.sh to plain java test

[mandy chung]

On 9/4/18 2:26 AM, Amy Lu wrote:


I updated SetupGetCallerClass:
http://cr.openjdk.java.net/~amlu/8209832/webrev.01/



Looks fine to me.

Mandy

Re: StackWalker::getCallerClass throws UnsupportedOperationException

[mandy chung]

Thanks for reporting this.   I have created:
   https://bugs.openjdk.java.net/browse/JDK-8210375

and will look into it.

Mandy

On 9/4/18 4:26 AM, Michael Rasmussen wrote:

Hi

StackWalker::getCallerClass can throw "UnsupportedOperationException: 
StackWalker::getCallerClass called from @CallerSensitive", depending on how the 
underlying fetchNextBatch/fill_in_frames splits the stacktrace.

This seems to be a result of what was introduced in JDK-8157464.

The following code example produces the exception (Tested on the jdk.java.net 
releases of OpenJDK 10 and 11-ea build 28):

// - snip 
package com.test;

import java.lang.reflect.Method;

public class Main {
   public static void main(String[] args) throws Exception {
 Method method = Main.class.getDeclaredMethod("bar");
 Method m = Method.class.getMethod("invoke", Object.class, Object[].class);

 // get 'm' past compilation threshold
 for (int i = 0; i < 15; i++) {
   m.invoke(Main.class.getDeclaredMethod("dummy"), null, null);
 }

 m.invoke(m, m, new Object[] {method, new Object[] {null, null}});
   }

   static void dummy() { }

   static void bar() {
 
System.out.println(StackWalker.getInstance(StackWalker.Option.RETAIN_CLASS_REFERENCE).getCallerClass());
   }
}
// - snip 

The above code is constructed so Method::invoke ends up as the frame for which 
index==start_index in fill_in_frames, thus causing the exception.

For example output, including stackwalk debug output, see below.

// - snip 
$ java -Xlog:stackwalk=debug com.test.Main
[0.154s][debug][stackwalk] Start walking: mode 6 skip 0 frames batch size 6
[0.155s][debug][stackwalk]   skip  
java.lang.StackStreamFactory$AbstractStackWalker::callStackWalk
[0.156s][debug][stackwalk]   skip  
java.lang.StackStreamFactory$AbstractStackWalker::beginStackWalk
[0.157s][debug][stackwalk]   skip  
java.lang.StackStreamFactory$AbstractStackWalker::walk
[0.158s][debug][stackwalk]   skip  
java.lang.StackStreamFactory$CallerClassFinder::findCaller
[0.159s][debug][stackwalk]   skip  java.lang.StackWalker::getCallerClass
[0.160s][debug][stackwalk] fill_in_frames limit=6 start=2 frames length=8
[0.161s][debug][stackwalk]   2: frame method:  com.test.Main::bar bci=9
[0.162s][debug][stackwalk]   3: frame method:  
jdk.internal.reflect.NativeMethodAccessorImpl::invoke0 bci=0
[0.162s][debug][stackwalk]   4: frame method:  
jdk.internal.reflect.NativeMethodAccessorImpl::invoke bci=100
[0.163s][debug][stackwalk]   5: frame method:  
jdk.internal.reflect.DelegatingMethodAccessorImpl::invoke bci=6
[0.163s][debug][stackwalk]   6: frame method:  java.lang.reflect.Method::invoke 
bci=59
[0.168s][debug][stackwalk]   7: frame method:  
jdk.internal.reflect.GeneratedMethodAccessor1::invoke bci=48
[0.169s][debug][stackwalk] StackWalk::fetchNextBatch frame_count 8 
existing_stream 0x00a5b8ffb5f0 start 2 frames 10
[0.170s][debug][stackwalk] fill_in_frames limit=8 start=2 frames length=10
[0.171s][debug][stackwalk]   2: frame method:  
jdk.internal.reflect.DelegatingMethodAccessorImpl::invoke bci=6
[0.171s][debug][stackwalk]   3: frame method:  java.lang.reflect.Method::invoke 
bci=59
[0.172s][debug][stackwalk]   4: frame method:  
jdk.internal.reflect.GeneratedMethodAccessor1::invoke bci=48
[0.173s][debug][stackwalk]   5: frame method:  
jdk.internal.reflect.DelegatingMethodAccessorImpl::invoke bci=6
[0.174s][debug][stackwalk]   6: frame method:  java.lang.reflect.Method::invoke 
bci=59
[0.175s][debug][stackwalk]   7: frame method:  
jdk.internal.reflect.NativeMethodAccessorImpl::invoke0 bci=0
[0.175s][debug][stackwalk]   8: frame method:  
jdk.internal.reflect.NativeMethodAccessorImpl::invoke bci=100
[0.179s][debug][stackwalk]   9: frame method:  
jdk.internal.reflect.DelegatingMethodAccessorImpl::invoke bci=6
[0.180s][debug][stackwalk] StackWalk::fetchNextBatch frame_count 8 
existing_stream 0x00a5b8ffb5f0 start 2 frames 10
[0.181s][debug][stackwalk] fill_in_frames limit=8 start=2 frames length=10
[0.181s][debug][stackwalk]   2: frame method:  java.lang.reflect.Method::invoke 
bci=59
Exception in thread "main" java.lang.reflect.InvocationTargetException
 at 
java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
 at 
java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
 at 
java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
 at java.base/java.lang.reflect.Method.invoke(Method.java:566)
 at com.test.Main.main(Main.java:15)
Caused by: java.lang.reflect.InvocationTargetException
 at 
java.base/jdk.internal.reflect.GeneratedMethodAccessor1.invoke(Unknown Source)
 at 
java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
 at java.base/java.lang.reflect.Method.invoke(Method.java:566)
 ... 5 more
Caused by: 

Re: RFR 8210318: idea.sh script doesn't work on Mac

[Maurizio Cimadamore]

Ah - thanks for the tip, I'll give that a try

Maurizio


On 04/09/18 18:50, Erik Joelsson wrote:

Hello,

$TARGET was just pseudo code. In your case it's $1.tmp.

/Erik


On 2018-09-04 10:34, Maurizio Cimadamore wrote:

Hi Erik,
would $TARGET be set by make?

Maurizio


On 04/09/18 16:55, Erik Joelsson wrote:

Hello,

When choosing a temp file in the build, we avoid using /tmp whenever 
possible. A common pattern is instead to write to $TARGET.tmp and 
then mv that to $TARGET. Though unlikely to cause an issue, 
/tmp/replacement is a global location which is potentially risky 
(file permissions, concurrent execution etc).


Otherwise looks good.

/Erik

On 2018-09-03 05:39, Maurizio Cimadamore wrote:

Hi,
following the latest updates to the idea.sh script, Mac users 
reported issues - mostly having to do with usage of 'sed' - more 
specifically:


* sed -i option is not portable - it has different formats in Mac 
vs. Linux. This patch does without -i, by moving the replaced file 
onto a temporary file, then moving such file on top of the template 
file in a subsequent step. This should be more robust.


* sed doesn't like newlines in replaced text in Mac. I've thus 
omitted the newline from the SOURCE template - as that was mostly 
cosmetic.


Thanks for Michael McMahon to report (and figure out how to deal 
with) these issues, and to Alan Bateman for testing the patch.


I also fixed another minor glitch, this time in the langtools-only 
template - which was still referring to the old ant file location 
in the various run configuration.


Webrev:

http://cr.openjdk.java.net/~mcimadamore/8210318/

Cheers
Maurizio

Re: RFR 8210318: idea.sh script doesn't work on Mac

[Erik Joelsson]

Hello,

$TARGET was just pseudo code. In your case it's $1.tmp.

/Erik


On 2018-09-04 10:34, Maurizio Cimadamore wrote:

Hi Erik,
would $TARGET be set by make?

Maurizio


On 04/09/18 16:55, Erik Joelsson wrote:

Hello,

When choosing a temp file in the build, we avoid using /tmp whenever 
possible. A common pattern is instead to write to $TARGET.tmp and 
then mv that to $TARGET. Though unlikely to cause an issue, 
/tmp/replacement is a global location which is potentially risky 
(file permissions, concurrent execution etc).


Otherwise looks good.

/Erik

On 2018-09-03 05:39, Maurizio Cimadamore wrote:

Hi,
following the latest updates to the idea.sh script, Mac users 
reported issues - mostly having to do with usage of 'sed' - more 
specifically:


* sed -i option is not portable - it has different formats in Mac 
vs. Linux. This patch does without -i, by moving the replaced file 
onto a temporary file, then moving such file on top of the template 
file in a subsequent step. This should be more robust.


* sed doesn't like newlines in replaced text in Mac. I've thus 
omitted the newline from the SOURCE template - as that was mostly 
cosmetic.


Thanks for Michael McMahon to report (and figure out how to deal 
with) these issues, and to Alan Bateman for testing the patch.


I also fixed another minor glitch, this time in the langtools-only 
template - which was still referring to the old ant file location in 
the various run configuration.


Webrev:

http://cr.openjdk.java.net/~mcimadamore/8210318/

Cheers
Maurizio

Re: RFR 8210318: idea.sh script doesn't work on Mac

[Maurizio Cimadamore]

Hi Erik,
would $TARGET be set by make?

Maurizio


On 04/09/18 16:55, Erik Joelsson wrote:

Hello,

When choosing a temp file in the build, we avoid using /tmp whenever 
possible. A common pattern is instead to write to $TARGET.tmp and then 
mv that to $TARGET. Though unlikely to cause an issue, 
/tmp/replacement is a global location which is potentially risky (file 
permissions, concurrent execution etc).


Otherwise looks good.

/Erik

On 2018-09-03 05:39, Maurizio Cimadamore wrote:

Hi,
following the latest updates to the idea.sh script, Mac users 
reported issues - mostly having to do with usage of 'sed' - more 
specifically:


* sed -i option is not portable - it has different formats in Mac vs. 
Linux. This patch does without -i, by moving the replaced file onto a 
temporary file, then moving such file on top of the template file in 
a subsequent step. This should be more robust.


* sed doesn't like newlines in replaced text in Mac. I've thus 
omitted the newline from the SOURCE template - as that was mostly 
cosmetic.


Thanks for Michael McMahon to report (and figure out how to deal 
with) these issues, and to Alan Bateman for testing the patch.


I also fixed another minor glitch, this time in the langtools-only 
template - which was still referring to the old ant file location in 
the various run configuration.


Webrev:

http://cr.openjdk.java.net/~mcimadamore/8210318/

Cheers
Maurizio

Re: RFR - 8210366: Typo in MethodHandles.Lookup: must be either be

[Roger Riggs]

Looks fine,

Regards, Roger

On 9/4/2018 12:39 PM, Daniel Fuchs wrote:

Hi,

This is a simple doc-only fix for a typo in MethodHandles.Lookup:

8210366: Typo in MethodHandles.Lookup: must be either be
https://bugs.openjdk.java.net/browse/JDK-8210366

patch:

 

diff --git 
a/src/java.base/share/classes/java/lang/invoke/MethodHandles.java 
b/src/java.base/share/classes/java/lang/invoke/MethodHandles.java

--- a/src/java.base/share/classes/java/lang/invoke/MethodHandles.java
+++ b/src/java.base/share/classes/java/lang/invoke/MethodHandles.java
@@ -450,7 +450,7 @@
  * independently of any {@code Lookup} object.
  * 
  * If the desired member is {@code protected}, the usual JVM 
rules apply,
- * including the requirement that the lookup class must be either 
be in the
+ * including the requirement that the lookup class must either be 
in the

  * same package as the desired member, or must inherit that member.
  * (See the Java Virtual Machine Specification, sections 4.9.2, 
5.4.3.5, and 6.4.)
  * In addition, if the desired member is a non-static field or 
method
 



best regards,

-- daniel

Re: RFR 8210285 : CharsetDecoder/Encoder's constructor does not reject NaN

[joe darcy]

Hello,

Please also create a quick CSR to cover the behavioral change.

Thanks,

-Joe


On 8/31/2018 5:17 PM, Ivan Gerasimov wrote:

Hello!

The javadoc for CharsetDecoder [1] states that an exception is thrown 
when a non-positive number is passed in as an argument.


However we only reject negative or zero numbers, but not NaN.

And likewise for CharsetEncoder.

Would you please help review a trivial fix?

BUGURL: https://bugs.openjdk.java.net/browse/JDK-8210285
WEBREV: http://cr.openjdk.java.net/~igerasim/8210285/00/webrev/

Thanks in advance!

[1] 
https://download.java.net/java/early_access/jdk11/docs/api/java.base/java/nio/charset/CharsetDecoder.html#%3Cinit%3E(java.nio.charset.Charset,float,float)

RFR - 8210366: Typo in MethodHandles.Lookup: must be either be

[Daniel Fuchs]

Hi,

This is a simple doc-only fix for a typo in MethodHandles.Lookup:

8210366: Typo in MethodHandles.Lookup: must be either be
https://bugs.openjdk.java.net/browse/JDK-8210366

patch:


diff --git 
a/src/java.base/share/classes/java/lang/invoke/MethodHandles.java 
b/src/java.base/share/classes/java/lang/invoke/MethodHandles.java

--- a/src/java.base/share/classes/java/lang/invoke/MethodHandles.java
+++ b/src/java.base/share/classes/java/lang/invoke/MethodHandles.java
@@ -450,7 +450,7 @@
  * independently of any {@code Lookup} object.
  * 
  * If the desired member is {@code protected}, the usual JVM rules 
apply,
- * including the requirement that the lookup class must be either 
be in the
+ * including the requirement that the lookup class must either be 
in the

  * same package as the desired member, or must inherit that member.
  * (See the Java Virtual Machine Specification, sections 4.9.2, 
5.4.3.5, and 6.4.)

  * In addition, if the desired member is a non-static field or method


best regards,

-- daniel

Re: RFR 8210318: idea.sh script doesn't work on Mac

[Erik Joelsson]

Hello,

When choosing a temp file in the build, we avoid using /tmp whenever 
possible. A common pattern is instead to write to $TARGET.tmp and then 
mv that to $TARGET. Though unlikely to cause an issue, /tmp/replacement 
is a global location which is potentially risky (file permissions, 
concurrent execution etc).


Otherwise looks good.

/Erik

On 2018-09-03 05:39, Maurizio Cimadamore wrote:

Hi,
following the latest updates to the idea.sh script, Mac users reported 
issues - mostly having to do with usage of 'sed' - more specifically:


* sed -i option is not portable - it has different formats in Mac vs. 
Linux. This patch does without -i, by moving the replaced file onto a 
temporary file, then moving such file on top of the template file in a 
subsequent step. This should be more robust.


* sed doesn't like newlines in replaced text in Mac. I've thus omitted 
the newline from the SOURCE template - as that was mostly cosmetic.


Thanks for Michael McMahon to report (and figure out how to deal with) 
these issues, and to Alan Bateman for testing the patch.


I also fixed another minor glitch, this time in the langtools-only 
template - which was still referring to the old ant file location in 
the various run configuration.


Webrev:

http://cr.openjdk.java.net/~mcimadamore/8210318/

Cheers
Maurizio

Re: [OpenJDK 2D-Dev] RFR JDK-8209786: gcc 7.3 compiler errors on zLinux

[Magnus Ihse Bursie]

Looks good to me. 

/Magnus

> 4 sep. 2018 kl. 12:11 skrev Andrew Leonard :
> 
> Hi Brian/Goetz, 
> Yes, that seems sensible. I have created a new webrev with fdlibm compiler 
> option disabled, and mediaLib code fixed: 
> http://cr.openjdk.java.net/~aleonard/8209786/webrev.02/ 
> I've built and tested on xLinux and zLinux, with gcc 4.8.4 and 7.3. 
> 
> Are we good to go? 
> Thanks 
> Andrew 
> 
> hg export: 
> # HG changeset patch 
> # User aleonard 
> # Date 1536055438 -3600 
> #  Tue Sep 04 11:03:58 2018 +0100 
> # Node ID c2523f285c503e8f82f1212b38de1cb54093255e 
> # Parent  3ee91722550680c18b977f0e00b1013323b5c9ef 
> 8209786: JDK12 fails to build on s390x with gcc 7.3 
> 
> diff -r 3ee917225506 -r c2523f285c50 make/lib/CoreLibraries.gmk 
> --- a/make/lib/CoreLibraries.gmkTue Sep 04 14:47:55 2018 +0800 
> +++ b/make/lib/CoreLibraries.gmkTue Sep 04 11:03:58 2018 +0100 
> @@ -68,7 +68,7 @@ 
>CFLAGS_linux_ppc64le := -ffp-contract=off, \ 
>CFLAGS_linux_s390x := -ffp-contract=off, \ 
>CFLAGS_linux_aarch64 := -ffp-contract=off, \ 
> -  DISABLED_WARNINGS_gcc := sign-compare misleading-indentation, \ 
> +  DISABLED_WARNINGS_gcc := sign-compare misleading-indentation 
> array-bounds, \ 
>DISABLED_WARNINGS_microsoft := 4146 4244 4018, \ 
>ARFLAGS := $(ARFLAGS), \ 
>OBJECT_DIR := $(SUPPORT_OUTPUTDIR)/native/$(MODULE)/libfdlibm, \ 
> diff -r 3ee917225506 -r c2523f285c50 
> src/java.desktop/share/native/libmlib_image/mlib_ImageLookUp_Bit.c 
> --- a/src/java.desktop/share/native/libmlib_image/mlib_ImageLookUp_Bit.c  
>   Tue Sep 04 14:47:55 2018 +0800 
> +++ b/src/java.desktop/share/native/libmlib_image/mlib_ImageLookUp_Bit.c  
>   Tue Sep 04 11:03:58 2018 +0100 
> @@ -1,5 +1,5 @@ 
>  /* 
> - * Copyright (c) 2003, Oracle and/or its affiliates. All rights reserved. 
> + * Copyright (c) 2003, 2018, 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 
> @@ -259,18 +259,18 @@ 
>} 
>   
>  #ifdef _LITTLE_ENDIAN 
> -  emask = (mlib_u32)((mlib_s32)(-1)) >> ((4 - (size - i)) * 8); 
> +  emask = (~(mlib_u32)0) >> ((4 - (size - i)) * 8); 
>  #else 
> -  emask = (mlib_s32)(-1) << ((4 - (size - i)) * 8); 
> +  emask = (~(mlib_u32)0) << ((4 - (size - i)) * 8); 
>  #endif /* _LITTLE_ENDIAN */ 
>((mlib_u32*)da)[0] = (val1 & emask) | (((mlib_u32*)da)[0] &~ emask); 
>   
>  #else /* _NO_LONGLONG */ 
>   
>  #ifdef _LITTLE_ENDIAN 
> -  mlib_u64 emask = (mlib_u64)((mlib_s64)(-1)) >> ((8 - (size - i)) * 8); 
> +  mlib_u64 emask = (~(mlib_u64)0) >> ((8 - (size - i)) * 8); 
>  #else 
> -  mlib_u64 emask = (mlib_s64)(-1) << ((8 - (size - i)) * 8); 
> +  mlib_u64 emask = (~(mlib_u64)0) << ((8 - (size - i)) * 8); 
>  #endif /* _LITTLE_ENDIAN */ 
>   
>((mlib_u64*)da)[0] = (((mlib_u64*)dd_array)[sa[0]] & emask) | 
> (((mlib_u64*)da)[0] &~ emask); 
> @@ -395,9 +395,9 @@ 
>} 
>   
>  #ifdef _LITTLE_ENDIAN 
> -  emask = (mlib_u32)((mlib_s32)(-1)) >> ((4 - (size - i)) * 8); 
> +  emask = (~(mlib_u32)0) >> ((4 - (size - i)) * 8); 
>  #else 
> -  emask = (mlib_s32)(-1) << ((4 - (size - i)) * 8); 
> +  emask = (~(mlib_u32)0) << ((4 - (size - i)) * 8); 
>  #endif /* _LITTLE_ENDIAN */ 
>((mlib_u32*)da)[0] = (dd1 & emask) | (((mlib_u32*)da)[0] &~ emask); 
>   
> @@ -413,9 +413,9 @@ 
>} 
>   
>  #ifdef _LITTLE_ENDIAN 
> -  emask = (mlib_u64)((mlib_s64)(-1)) >> ((8 - (size - i)) * 8); 
> +  emask = (~(mlib_u64)0) >> ((8 - (size - i)) * 8); 
>  #else 
> -  emask = (mlib_s64)(-1) << ((8 - (size - i)) * 8); 
> +  emask = (~(mlib_u64)0) << ((8 - (size - i)) * 8); 
>  #endif /* _LITTLE_ENDIAN */ 
>((mlib_u64*)da)[0] = (dd & emask) | (((mlib_u64*)da)[0] &~ emask); 
>   
> @@ -565,9 +565,9 @@ 
>} 
>   
>  #ifdef _LITTLE_ENDIAN 
> -  emask = (mlib_u32)((mlib_s32)(-1)) >> ((4 - (size - i)) * 8); 
> +  emask = (~(mlib_u32)0) >> ((4 - (size - i)) * 8); 
>  #else 
> -  emask = (mlib_s32)(-1) << ((4 - (size - i)) * 8); 
> +  emask = (~(mlib_u32)0) << ((4 - (size - i)) * 8); 
>  #endif /* _LITTLE_ENDIAN */ 
>da[0] = (dd & emask) | (da[0] &~ emask); 
>  } 
> 
> 
> 
> 
> 
> 
> 
> Andrew Leonard
> Java Runtimes Development
> IBM Hursley
> IBM United Kingdom Ltd
> Phone internal: 245913, external: 01962 815913
> internet email: andrew_m_leon...@uk.ibm.com 
> 
> 
> 
> 
> From:Brian Burkhalter  
> To:Magnus Ihse Bursie  
> Cc:Andrew Leonard , "Lindenmaier, Goetz" 
> , 2d-dev <2d-...@openjdk.java.net>, build-dev 
> , core-libs-dev , 
> Philip Race  
> Date:31/08/2018 15:44 
> Subject:Re: [OpenJDK 2D-Dev] RFR JDK-8209786: gcc 7.3 compiler errors 
> on zLinux 
> 
> 
> 
> 
> On Aug 31, 2018, at 2:28 AM, Magnus Ihse Bursie 
>  wrote: 
> 
> Magnus, 

Re: RFR:8205330 InitialDirContext ctor sometimes throws NPE if the server has sent a disconnection

[Daniel Fuchs]

Hi Vyom,

IIUC, the issue only happens when connections (clients?) to the
server are pooled. I assume the server may decide to
close what it thinks is an idle connection at any time.

So what we see here is actually a race between an open
connection being retrieved from the pool, and the server
deciding to close that connection.
The connection/client is retrieved from the pool, then
unfortunately closed by the server after having been selected
for the next operation.

The question for me would be "what is the expected behavior
if the server decides to close an idle connection?"
I would expect that new InitialDirContext() should have some
way of dealing with that with retrying?
If so will leaving the 'conn' field assigned ensure that
the retry happens, or will new InitialDirContext() fail
with some other exception because the connection has
been closed? Or does the code simply assume that asynchronous
closing of the connection by the server can only happen while
it's sitting idle in the pool?

I wonder if some other mechanism could be used to reduce
and hopefully eliminate the time window in which the race
could appear. For instance taking the connection out of
the pool before closing it, instead of closing it before
taking it out of the pool, etc...

best regards,

-- daniel

On 04/09/2018 15:04, vyom tewari wrote:



On Friday 24 August 2018 08:52 PM, Chris Hegarty wrote:

Hi Vyom,

On 24/08/18 11:35, vyom tewari wrote:

Hi All,

Please review this simple fix below

webrev: http://cr.openjdk.java.net/~vtewari/8205330/webrev0.0/index.html

bugid: https://bugs.openjdk.java.net/browse/JDK-8205330

This fix will resolve the race in LdapClient  where we are explicitly 
making "null" to LdapClient.conn.


Sorry, I don't know this code all that well, but I think
that more explanation will be needed before this code
can be reviewed.


Hi Chris, let me try to explain issue little bit.

The issue is a if ldap connection has already been established and then 
the LDAP directory server sends an (unsolicited) "Notice of 
Disconnection", the client's processing of this LDAP message can race 
with an application thread calling new InitialDirContext() to 
authenticate user credentials (i.e.bind) and throw NPE.


I did some analysis and found out that when server send an (unsolicited) 
"Notice of Disconnection",  LdapClient.forceClose() will be called in 
LdapClient.processUnsolicited() which is called asynchronously by the 
reader thread in Connection, this means 'LdapClient.conn' may set to 
null anytime after it received  "Notice of Disconnection".



The LdapClient and the Connection seem to be loosely
coupled. I think part of this is to allow the LdapClient
to be GC'ed and finalized separately to the Connection
( that can be reused ). Not setting `conn` to null could
have a negative impact on this loose coupling. I also
note that the synchronization is implemented poorly in
the LdapClient, `conn` is operated on both from within
synchronized blocks and from unsynchronized blocks (
which I think is the reason you see the unexpected
null ).

I agree, not setting  'conn' to null will definitely have some impact. I 
check the LdapClient and it looks to me it is intentional(i may be 
wrong) that 'conn' is operated on both from within synchronize blocks 
and from unsynchronize block.


LdapClient, java doc says that connection(conn) take care of it's own sync.

##
    access from outside LdapClient must sync;
  *   conn - no sync; Connection takes care of its own sync
  *   unsolicited - sync Vector; multiple operations sync'ed

##

Please have a look and do let me know your thought on the above.

Thanks,
Vyom

-Chris.

Re: RFR:8205330 InitialDirContext ctor sometimes throws NPE if the server has sent a disconnection

[vyom tewari]

On Friday 24 August 2018 08:52 PM, Chris Hegarty wrote:

Hi Vyom,

On 24/08/18 11:35, vyom tewari wrote:

Hi All,

Please review this simple fix below

webrev: http://cr.openjdk.java.net/~vtewari/8205330/webrev0.0/index.html

bugid: https://bugs.openjdk.java.net/browse/JDK-8205330

This fix will resolve the race in LdapClient  where we are explicitly 
making "null" to LdapClient.conn.


Sorry, I don't know this code all that well, but I think
that more explanation will be needed before this code
can be reviewed.


Hi Chris, let me try to explain issue little bit.

The issue is a if ldap connection has already been established and then 
the LDAP directory server sends an (unsolicited) "Notice of 
Disconnection", the client's processing of this LDAP message can race 
with an application thread calling new InitialDirContext() to 
authenticate user credentials (i.e.bind) and throw NPE.


I did some analysis and found out that when server send an (unsolicited) 
"Notice of Disconnection",  LdapClient.forceClose() will be called in 
LdapClient.processUnsolicited() which is called asynchronously by the 
reader thread in Connection, this means 'LdapClient.conn' may set to 
null anytime after it received  "Notice of Disconnection".



The LdapClient and the Connection seem to be loosely
coupled. I think part of this is to allow the LdapClient
to be GC'ed and finalized separately to the Connection
( that can be reused ). Not setting `conn` to null could
have a negative impact on this loose coupling. I also
note that the synchronization is implemented poorly in
the LdapClient, `conn` is operated on both from within
synchronized blocks and from unsynchronized blocks (
which I think is the reason you see the unexpected
null ).

I agree, not setting  'conn' to null will definitely have some impact.  
I check the LdapClient and it looks to me it is intentional(i may be 
wrong) that 'conn' is operated on both from within synchronize blocks 
and from unsynchronize block.


LdapClient, java doc says that connection(conn) take care of it's own sync.

##
   access from outside LdapClient must sync;
 *   conn - no sync; Connection takes care of its own sync
 *   unsolicited - sync Vector; multiple operations sync'ed

##

Please have a look and do let me know your thought on the above.

Thanks,
Vyom

-Chris.

Re: [RFR] 8205525 : Improve exception messages during manifest parsing of jar archives

[Weijun Wang]

I've added myself as a reviewer. You might want to set scope to "JDK".

A CSR is approved by the CSR group after you finalize it. First you should 
propose it. If you think it's urgent or trivial, you can also fast-track it.

Read https://wiki.openjdk.java.net/display/csr/Main for more details.

Thanks
Max

> On Sep 4, 2018, at 7:33 PM, Baesken, Matthias  
> wrote:
> 
>> The change looks fine. We can enhance the name if we want to support .SF
>> parsing later.
>> 
>> Please revise your CSR and get it approved first.
> 
> Hi Max, Thanks !
> 
> I adjusted the CSR , please approve  :
> 
> https://bugs.openjdk.java.net/browse/JDK-8207768
> 
> Best regards, Matthias
> 
> 
> 
>> -Original Message-
>> From: Weijun Wang 
>> Sent: Montag, 3. September 2018 17:14
>> To: Baesken, Matthias 
>> Cc: Alan Bateman ; Sean Mullan
>> ; Chris Hegarty ;
>> security-...@openjdk.java.net; core-libs-dev@openjdk.java.net
>> Subject: Re: [RFR] 8205525 : Improve exception messages during manifest
>> parsing of jar archives
>> 
>> Hi Matthias
>> 
>> The change looks fine. We can enhance the name if we want to support .SF
>> parsing later.
>> 
>> Please revise your CSR and get it approved first.
>> 
>> Thanks
>> Max
>> 
>>> On Sep 3, 2018, at 10:19 PM, Baesken, Matthias
>>  wrote:
>>> 
>>> Hi Max,  I
>>> 
>>> - moved getErrorPosition  method   to  Manifest.java
>>> - in read() method,   removed "int offset"
>>> - in  the exception message, I write now  " manifest of "  ...   (without
>> mentioning a manifest name)
>>> 
>>> http://cr.openjdk.java.net/~mbaesken/webrevs/8205525.8/
>>> 
>>> 
>>> Best regards, Matthias
>>> 
>>> 
 -Original Message-
 From: Weijun Wang 
 Sent: Freitag, 31. August 2018 15:53
 To: Baesken, Matthias 
 Cc: Alan Bateman ; Sean Mullan
 ; Chris Hegarty ;
 security-...@openjdk.java.net; core-libs-dev@openjdk.java.net
 Subject: Re: [RFR] 8205525 : Improve exception messages during manifest
 parsing of jar archives
 
 
 
> On Aug 31, 2018, at 8:52 PM, Baesken, Matthias
  wrote:
> 
> Hi Max :
> 
>> 
>> - No need to "import java.security.Security".
> 
> Sure  I can remove this, it is a leftover.
> 
>> - In the updated read() method, I think there is no need to use an "int
 offset"
>> parameter. "int lineNumber" is enough and you can modify it and
>> return it
>> without a new local variable.
>> 
> 
> Currently we need it in Manifest.java  public void read(InputStream is)
 throws IOException { ... }
 
 I was talking about the name of the parameter. You can simply use
 
 int read(Manifest.FastInputStream is, byte[] lbuf, String filename, int
 lineNumber)
 
 and there is no need to reassign it with "int lineNumber = offset"
>> anymore.
 
> 
>> In fact, I have a small concern on the hardcoded file name here,
>> because
>> when verifying a signed jar, the META-INF/X.SF file is also parsed into a
>> Manifest object and if it's invalid similar exceptions will be thrown. I
>> don't
>> have a nice solution now.
> 
> Then lets just say   :   (e.g.  test.jar:10 )
> 
> Or ( to mention that it is about a manifest  from the jar )
> 
> :manifest-line(e.g.  test.jar:manifest-line 10 )
 
 How about you pass in the full name ("/path/to/file.jar!META-
 INF/MANIFEST.MF") to "new Manifest(stream,name)" directly?
 
 So the name will be constructed in JarFile.java (after checking for
 jarPathInExceptionText). The getErrorPosition method simply concat the
 name (if not null) and the line number. Thus the exception thrown from
 parsing X.SF simply will not include any file info. If we want it we can
>> enhance
 later.
 
 Thanks
 Max
 
> 
> 
> Best regards, Matthias
> 
> 
> 
>> -Original Message-
>> From: Weijun Wang 
>> Sent: Freitag, 31. August 2018 04:32
>> To: Baesken, Matthias 
>> Cc: Alan Bateman ; Sean Mullan
>> ; Chris Hegarty
>> ;
>> security-...@openjdk.java.net; core-libs-dev@openjdk.java.net
>> Subject: Re: [RFR] 8205525 : Improve exception messages during
>> manifest
>> parsing of jar archives
>> 
>> Some more comments:
>> 
>> Attributes.java
>> 
>> - No need to "import java.security.Security".
>> 
>> - In the updated read() method, I think there is no need to use an "int
 offset"
>> parameter. "int lineNumber" is enough and you can modify it and
>> return it
>> without a new local variable.
>> 
>> - I feel like calling Attributes::getErrorPosition from Manifest a little
 strange.
>> Maybe it's better to define the method in Manifest and call it from
>> Attributes. First, I think putting the method in a higher level object
>> makes
>> more sense. Second, the position is for the whole Manifest and not an
>> Attributes section (I 

RE: RFR: 8209937: Enhance java.io.Console - provide methods to query console width and height

[Langer, Christoph]

Hi Alan,

thanks for your feedback (and thanks of course to all the others who commented 
on this proposal).

As I've written in my first mail, this request came up because we had 
implemented getWidth/getHeigth APIs for console windows in our licensed VM to 
be used in a certain usage scenario. Now we are in the process of moving this 
user to an OpenJDK based VM and thought these APIs might be candidates for 
contribution as general purpose API. Looking at java.io.Console, it seemed the 
APIs would fit in there. We also thought that there could be further additions 
to this class.

However, we had not been aware of the history of java.io.Console and we also 
did not know jline project nor that it was adopted by the OpenJDK into 
jdk.internal.le for usage by jshell and nashorn. Thanks for pointing this out. 
After exploring this a bit, I'm at a point where I would go to our user and ask 
them to try jline. I'm quite sure they'll be able to adopt it.

Regardless of that, isn't it still worth to gradually enhance java.io.Console 
over time? I don't know whether we'd eventually get to a point where jline 
could be replaced for jshell but maybe that could be a long term goal.

If we can agree to evolve java.io.Console, I'll continue with my work for 
width/height APIs. But if it's consent to keep java.io.Console in its limited 
current state, I would also be willing to withdraw this request.

What's the way to go?
 
Best regards
Christoph

> -Original Message-
> From: Alan Bateman 
> Sent: Montag, 27. August 2018 13:30
> To: Langer, Christoph 
> Cc: core-libs-dev ; Baesken, Matthias
> 
> Subject: Re: RFR: 8209937: Enhance java.io.Console - provide methods to
> query console width and height
> 
> On 24/08/2018 14:18, Langer, Christoph wrote:
> > Hi Rémi, Hi Peter,
> >
> > thanks for your quick answers.
> >
> > What you've suggested, Rémi, is perfectly right. I've updated my webrev.
> The methods were copied from our old implementation (of a different class)
> where they were provided as static.
> >
> > I will also think of using an optional. I'm furthermore wondering if we
> should provide a method "dimensions()" returning an (optional)
> java.io.Console.Dimension object that contains both height and width...
> >
> > Here is a new webrev:
> http://cr.openjdk.java.net/~clanger/webrevs/8209937.1/
> I'm in two minds as to whether this API is the right thing to do. Can
> you expand a bit on how you are using them? There have been several
> requests for additions to the Console API over the years, mostly to
> support formatting or exposing the console encoding, but I don't recall
> anyone looking for the dimensions. Roger is right that it's a bit of a
> slippery slope. As I recall, java.io.Console was deliberately minimized
> for Java SE 6 to focus on password input and avoid introducing an
> extensive API.  In the mean-time, the additional of jshell has exposed
> some of the shorting comings and maybe we should look at exposing just
> enough to support that type of use-case (editing and line history mostly).
> 
> As regards the proposal then I think the API docs will need fleshing out
> to define what height and width as the current javadoc isn't clear if it
> means bytes, characters, pixels in whatever font is used for the
> console, or something else. In addition, the javadoc would need to be
> clear that it's just a snaphot of the dimension as it can change at any
> time.
> 
> -Alan

Re: RFR 8210318: idea.sh script doesn't work on Mac

[Michael McMahon]

The change looks fine to me Maurizio. Maybe you could append a .$$ to 
the temporary
file name to make it less likely to overwrite something that already 
exists in /tmp


- Michael.

On 03/09/2018, 13:39, Maurizio Cimadamore wrote:

Hi,
following the latest updates to the idea.sh script, Mac users reported 
issues - mostly having to do with usage of 'sed' - more specifically:


* sed -i option is not portable - it has different formats in Mac vs. 
Linux. This patch does without -i, by moving the replaced file onto a 
temporary file, then moving such file on top of the template file in a 
subsequent step. This should be more robust.


* sed doesn't like newlines in replaced text in Mac. I've thus omitted 
the newline from the SOURCE template - as that was mostly cosmetic.


Thanks for Michael McMahon to report (and figure out how to deal with) 
these issues, and to Alan Bateman for testing the patch.


I also fixed another minor glitch, this time in the langtools-only 
template - which was still referring to the old ant file location in 
the various run configuration.


Webrev:

http://cr.openjdk.java.net/~mcimadamore/8210318/

Cheers
Maurizio

Re: RFR(M) : 8210039 : move OSInfo to top level testlibrary

[Daniel Fuchs]

Hi Igor,

Nit: You could have introduced a
private static String getProperty(String name) {
 return AccessController.doP
}
in Platform.java to avoid the long lines.

Otherwise looks good to me too!

best regards,

-- daniel

On 31/08/2018 19:42, Igor Ignatyev wrote:

Alan, Chris,

thanks for looking at it, I went w/ the alternative suggested by Chris. that 
required a sprinkle of doPrivileged in the testlibrary, but now 
Sockets/policy.* files grant the minimal required permissions to the test code.
the incremental webrev can found here[1], please let me know if you need the 
whole webrev.

[1] http://cr.openjdk.java.net/~iignatyev//8210039/webrev.0-1/index.html

Thanks,
-- Igor



On Aug 30, 2018, at 3:28 AM, Chris Hegarty  wrote:



On 30 Aug 2018, at 08:51, Alan Bateman  wrote:

On 28/08/2018 17:50, Igor Ignatyev wrote:

http://cr.openjdk.java.net/~iignatyev//8210039/webrev.00/index.html

698 lines changed: 114 ins; 240 del; 344 mod

Hi all,

could you please review this clean up of jdk testlibrary?
the patch updates the tests to use jdk.test.lib.Platform instead of 
jdk.testlibrary.OSInfo.OSType, cleans up OSInfo and renames it to 
jdk.test.lib.OSVersion.

testing: changed tests + :jdk_desktop test group
webrev: http://cr.openjdk.java.net/~iignatyev//8210039/webrev.00/index.html
JBS: https://bugs.openjdk.java.net/browse/JDK-8210039


The updates to the Sockets policy file suggests using this 
jdk.test.lib.Platform/OSVersion requires permissions that the test 
infrastructure needs, not the test. It's not wrong but it's always a concern 
when tests running with a security manager are granted non-obvious permissions.


The uses of test libraries with security manager is a little
cumbersome, and usually ends up with the test code being
granted more permissions than is necessary. I share Alan’s
concern.

Another alternative, that we used in other areas, is to grant
the test library only minimal permissions, separate to the
actual test code. For example:

http://hg.openjdk.java.net/jdk/jdk/file/9183040e34d8/test/jdk/java/net/httpclient/AsFileDownloadTest.policy#l24
 


-Chris.

Re: RFR(M) : 8210039 : move OSInfo to top level testlibrary

[Chris Hegarty]

Igor,

> On 31 Aug 2018, at 19:42, Igor Ignatyev  wrote:
> 
> Alan, Chris,
> 
> thanks for looking at it, I went w/ the alternative suggested by Chris. that 
> required a sprinkle of doPrivileged in the testlibrary, but now 
> Sockets/policy.* files grant the minimal required permissions to the test 
> code.

It’s bit annoying to have retrofit the test library classes with doPriv,
but I think that it is the right approach, if we intend them to be
usable with tests running with a security manager ( which we do ).

> the incremental webrev can found here[1], please let me know if you need the 
> whole webrev.
> 
> [1] http://cr.openjdk.java.net/~iignatyev//8210039/webrev.0-1/index.html 
> 
I think this is fine.

Thanks,
-Chris.

Re: RFR(M) : 8210039 : move OSInfo to top level testlibrary

[Alan Bateman]

On 31/08/2018 19:42, Igor Ignatyev wrote:

Alan, Chris,

thanks for looking at it, I went w/ the alternative suggested by 
Chris. that required a sprinkle of doPrivileged in the testlibrary, 
but now Sockets/policy.* files grant the minimal required permissions 
to the test code.
the incremental webrev can found here[1], please let me know if you 
need the whole webrev.


[1] 
http://cr.openjdk.java.net/~iignatyev//8210039/webrev.0-1/index.html 



I can't quickly verify that the code source 
"file:${test.classes}/../../../../test/lib/-" results in granting the 
permission to the right library but I assume it will fail if it's not 
correct. So I think this approach looks good to me.


-Alan

Re: [12] RFR of JDK-8209832: Refactor jdk/internal/reflect/Reflection/GetCallerClassTest.sh to plain java test

[Alan Bateman]

On 04/09/2018 10:26, Amy Lu wrote:


Thank you Alan!

I updated SetupGetCallerClass:
http://cr.openjdk.java.net/~amlu/8209832/webrev.01/


This looks okay to me.

-Alan

RE: [RFR] 8205525 : Improve exception messages during manifest parsing of jar archives

[Baesken, Matthias]

> The change looks fine. We can enhance the name if we want to support .SF
> parsing later.
> 
> Please revise your CSR and get it approved first.

Hi Max, Thanks !

I adjusted the CSR , please approve  :

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

Best regards, Matthias



> -Original Message-
> From: Weijun Wang 
> Sent: Montag, 3. September 2018 17:14
> To: Baesken, Matthias 
> Cc: Alan Bateman ; Sean Mullan
> ; Chris Hegarty ;
> security-...@openjdk.java.net; core-libs-dev@openjdk.java.net
> Subject: Re: [RFR] 8205525 : Improve exception messages during manifest
> parsing of jar archives
> 
> Hi Matthias
> 
> The change looks fine. We can enhance the name if we want to support .SF
> parsing later.
> 
> Please revise your CSR and get it approved first.
> 
> Thanks
> Max
> 
> > On Sep 3, 2018, at 10:19 PM, Baesken, Matthias
>  wrote:
> >
> > Hi Max,  I
> >
> > - moved getErrorPosition  method   to  Manifest.java
> > - in read() method,   removed "int offset"
> > - in  the exception message, I write now  " manifest of "  ...   (without
> mentioning a manifest name)
> >
> > http://cr.openjdk.java.net/~mbaesken/webrevs/8205525.8/
> >
> >
> > Best regards, Matthias
> >
> >
> >> -Original Message-
> >> From: Weijun Wang 
> >> Sent: Freitag, 31. August 2018 15:53
> >> To: Baesken, Matthias 
> >> Cc: Alan Bateman ; Sean Mullan
> >> ; Chris Hegarty ;
> >> security-...@openjdk.java.net; core-libs-dev@openjdk.java.net
> >> Subject: Re: [RFR] 8205525 : Improve exception messages during manifest
> >> parsing of jar archives
> >>
> >>
> >>
> >>> On Aug 31, 2018, at 8:52 PM, Baesken, Matthias
> >>  wrote:
> >>>
> >>> Hi Max :
> >>>
> 
>  - No need to "import java.security.Security".
> >>>
> >>> Sure  I can remove this, it is a leftover.
> >>>
>  - In the updated read() method, I think there is no need to use an "int
> >> offset"
>  parameter. "int lineNumber" is enough and you can modify it and
> return it
>  without a new local variable.
> 
> >>>
> >>> Currently we need it in Manifest.java  public void read(InputStream is)
> >> throws IOException { ... }
> >>
> >> I was talking about the name of the parameter. You can simply use
> >>
> >> int read(Manifest.FastInputStream is, byte[] lbuf, String filename, int
> >> lineNumber)
> >>
> >> and there is no need to reassign it with "int lineNumber = offset"
> anymore.
> >>
> >>>
>  In fact, I have a small concern on the hardcoded file name here,
> because
>  when verifying a signed jar, the META-INF/X.SF file is also parsed into a
>  Manifest object and if it's invalid similar exceptions will be thrown. I
> don't
>  have a nice solution now.
> >>>
> >>> Then lets just say   :   (e.g.  test.jar:10 )
> >>>
> >>> Or ( to mention that it is about a manifest  from the jar )
> >>>
> >>> :manifest-line(e.g.  test.jar:manifest-line 10 )
> >>
> >> How about you pass in the full name ("/path/to/file.jar!META-
> >> INF/MANIFEST.MF") to "new Manifest(stream,name)" directly?
> >>
> >> So the name will be constructed in JarFile.java (after checking for
> >> jarPathInExceptionText). The getErrorPosition method simply concat the
> >> name (if not null) and the line number. Thus the exception thrown from
> >> parsing X.SF simply will not include any file info. If we want it we can
> enhance
> >> later.
> >>
> >> Thanks
> >> Max
> >>
> >>>
> >>>
> >>> Best regards, Matthias
> >>>
> >>>
> >>>
>  -Original Message-
>  From: Weijun Wang 
>  Sent: Freitag, 31. August 2018 04:32
>  To: Baesken, Matthias 
>  Cc: Alan Bateman ; Sean Mullan
>  ; Chris Hegarty
> ;
>  security-...@openjdk.java.net; core-libs-dev@openjdk.java.net
>  Subject: Re: [RFR] 8205525 : Improve exception messages during
> manifest
>  parsing of jar archives
> 
>  Some more comments:
> 
>  Attributes.java
> 
>  - No need to "import java.security.Security".
> 
>  - In the updated read() method, I think there is no need to use an "int
> >> offset"
>  parameter. "int lineNumber" is enough and you can modify it and
> return it
>  without a new local variable.
> 
>  - I feel like calling Attributes::getErrorPosition from Manifest a little
> >> strange.
>  Maybe it's better to define the method in Manifest and call it from
>  Attributes. First, I think putting the method in a higher level object
> makes
>  more sense. Second, the position is for the whole Manifest and not an
>  Attributes section (I mean the line number is counted from the first line
> of
>  the manifest).
> 
> > On Aug 30, 2018, at 10:50 PM, Baesken, Matthias
>   wrote:
> >
> >
> >
> > Hi  Max, probably   we should add  the  info about the MANIFEST.MF  ,
> >> for
>  example :
> > change  getErrorPosition  to
> >
> >
> 
> >>
> http://cr.openjdk.java.net/~mbaesken/webrevs/8205525.7/src/java.base/s
>  

StackWalker::getCallerClass throws UnsupportedOperationException

[Michael Rasmussen]

Hi

StackWalker::getCallerClass can throw "UnsupportedOperationException: 
StackWalker::getCallerClass called from @CallerSensitive", depending on how the 
underlying fetchNextBatch/fill_in_frames splits the stacktrace.

This seems to be a result of what was introduced in JDK-8157464.

The following code example produces the exception (Tested on the jdk.java.net 
releases of OpenJDK 10 and 11-ea build 28):

// - snip 
package com.test;

import java.lang.reflect.Method;

public class Main {
  public static void main(String[] args) throws Exception {
Method method = Main.class.getDeclaredMethod("bar");
Method m = Method.class.getMethod("invoke", Object.class, Object[].class);

// get 'm' past compilation threshold
for (int i = 0; i < 15; i++) {
  m.invoke(Main.class.getDeclaredMethod("dummy"), null, null);
}

m.invoke(m, m, new Object[] {method, new Object[] {null, null}});
  }

  static void dummy() { }

  static void bar() {

System.out.println(StackWalker.getInstance(StackWalker.Option.RETAIN_CLASS_REFERENCE).getCallerClass());
  }
}
// - snip 

The above code is constructed so Method::invoke ends up as the frame for which 
index==start_index in fill_in_frames, thus causing the exception.

For example output, including stackwalk debug output, see below.

// - snip 
$ java -Xlog:stackwalk=debug com.test.Main
[0.154s][debug][stackwalk] Start walking: mode 6 skip 0 frames batch size 6
[0.155s][debug][stackwalk]   skip  
java.lang.StackStreamFactory$AbstractStackWalker::callStackWalk
[0.156s][debug][stackwalk]   skip  
java.lang.StackStreamFactory$AbstractStackWalker::beginStackWalk
[0.157s][debug][stackwalk]   skip  
java.lang.StackStreamFactory$AbstractStackWalker::walk
[0.158s][debug][stackwalk]   skip  
java.lang.StackStreamFactory$CallerClassFinder::findCaller
[0.159s][debug][stackwalk]   skip  java.lang.StackWalker::getCallerClass
[0.160s][debug][stackwalk] fill_in_frames limit=6 start=2 frames length=8
[0.161s][debug][stackwalk]   2: frame method:  com.test.Main::bar bci=9
[0.162s][debug][stackwalk]   3: frame method:  
jdk.internal.reflect.NativeMethodAccessorImpl::invoke0 bci=0
[0.162s][debug][stackwalk]   4: frame method:  
jdk.internal.reflect.NativeMethodAccessorImpl::invoke bci=100
[0.163s][debug][stackwalk]   5: frame method:  
jdk.internal.reflect.DelegatingMethodAccessorImpl::invoke bci=6
[0.163s][debug][stackwalk]   6: frame method:  java.lang.reflect.Method::invoke 
bci=59
[0.168s][debug][stackwalk]   7: frame method:  
jdk.internal.reflect.GeneratedMethodAccessor1::invoke bci=48
[0.169s][debug][stackwalk] StackWalk::fetchNextBatch frame_count 8 
existing_stream 0x00a5b8ffb5f0 start 2 frames 10
[0.170s][debug][stackwalk] fill_in_frames limit=8 start=2 frames length=10
[0.171s][debug][stackwalk]   2: frame method:  
jdk.internal.reflect.DelegatingMethodAccessorImpl::invoke bci=6
[0.171s][debug][stackwalk]   3: frame method:  java.lang.reflect.Method::invoke 
bci=59
[0.172s][debug][stackwalk]   4: frame method:  
jdk.internal.reflect.GeneratedMethodAccessor1::invoke bci=48
[0.173s][debug][stackwalk]   5: frame method:  
jdk.internal.reflect.DelegatingMethodAccessorImpl::invoke bci=6
[0.174s][debug][stackwalk]   6: frame method:  java.lang.reflect.Method::invoke 
bci=59
[0.175s][debug][stackwalk]   7: frame method:  
jdk.internal.reflect.NativeMethodAccessorImpl::invoke0 bci=0
[0.175s][debug][stackwalk]   8: frame method:  
jdk.internal.reflect.NativeMethodAccessorImpl::invoke bci=100
[0.179s][debug][stackwalk]   9: frame method:  
jdk.internal.reflect.DelegatingMethodAccessorImpl::invoke bci=6
[0.180s][debug][stackwalk] StackWalk::fetchNextBatch frame_count 8 
existing_stream 0x00a5b8ffb5f0 start 2 frames 10
[0.181s][debug][stackwalk] fill_in_frames limit=8 start=2 frames length=10
[0.181s][debug][stackwalk]   2: frame method:  java.lang.reflect.Method::invoke 
bci=59
Exception in thread "main" java.lang.reflect.InvocationTargetException
at 
java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
at 
java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
at 
java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
at java.base/java.lang.reflect.Method.invoke(Method.java:566)
at com.test.Main.main(Main.java:15)
Caused by: java.lang.reflect.InvocationTargetException
at 
java.base/jdk.internal.reflect.GeneratedMethodAccessor1.invoke(Unknown Source)
at 
java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
at java.base/java.lang.reflect.Method.invoke(Method.java:566)
... 5 more
Caused by: java.lang.reflect.InvocationTargetException
at 
java.base/jdk.internal.reflect.GeneratedMethodAccessor1.invoke(Unknown Source)
at 

Re: [OpenJDK 2D-Dev] RFR JDK-8209786: gcc 7.3 compiler errors on zLinux

[Andrew Leonard]

Hi Brian/Goetz,
Yes, that seems sensible. I have created a new webrev with fdlibm compiler 
option disabled, and mediaLib code fixed:
http://cr.openjdk.java.net/~aleonard/8209786/webrev.02/
I've built and tested on xLinux and zLinux, with gcc 4.8.4 and 7.3.

Are we good to go?
Thanks
Andrew

hg export:
# HG changeset patch
# User aleonard
# Date 1536055438 -3600
#  Tue Sep 04 11:03:58 2018 +0100
# Node ID c2523f285c503e8f82f1212b38de1cb54093255e
# Parent  3ee91722550680c18b977f0e00b1013323b5c9ef
8209786: JDK12 fails to build on s390x with gcc 7.3

diff -r 3ee917225506 -r c2523f285c50 make/lib/CoreLibraries.gmk
--- a/make/lib/CoreLibraries.gmkTue Sep 04 14:47:55 2018 +0800
+++ b/make/lib/CoreLibraries.gmkTue Sep 04 11:03:58 2018 +0100
@@ -68,7 +68,7 @@
   CFLAGS_linux_ppc64le := -ffp-contract=off, \
   CFLAGS_linux_s390x := -ffp-contract=off, \
   CFLAGS_linux_aarch64 := -ffp-contract=off, \
-  DISABLED_WARNINGS_gcc := sign-compare misleading-indentation, \
+  DISABLED_WARNINGS_gcc := sign-compare misleading-indentation 
array-bounds, \
   DISABLED_WARNINGS_microsoft := 4146 4244 4018, \
   ARFLAGS := $(ARFLAGS), \
   OBJECT_DIR := $(SUPPORT_OUTPUTDIR)/native/$(MODULE)/libfdlibm, \
diff -r 3ee917225506 -r c2523f285c50 
src/java.desktop/share/native/libmlib_image/mlib_ImageLookUp_Bit.c
--- a/src/java.desktop/share/native/libmlib_image/mlib_ImageLookUp_Bit.c   
  Tue Sep 04 14:47:55 2018 +0800
+++ b/src/java.desktop/share/native/libmlib_image/mlib_ImageLookUp_Bit.c   
  Tue Sep 04 11:03:58 2018 +0100
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2003, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2003, 2018, 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
@@ -259,18 +259,18 @@
   }
 
 #ifdef _LITTLE_ENDIAN
-  emask = (mlib_u32)((mlib_s32)(-1)) >> ((4 - (size - i)) * 8);
+  emask = (~(mlib_u32)0) >> ((4 - (size - i)) * 8);
 #else
-  emask = (mlib_s32)(-1) << ((4 - (size - i)) * 8);
+  emask = (~(mlib_u32)0) << ((4 - (size - i)) * 8);
 #endif /* _LITTLE_ENDIAN */
   ((mlib_u32*)da)[0] = (val1 & emask) | (((mlib_u32*)da)[0] &~ 
emask);
 
 #else /* _NO_LONGLONG */
 
 #ifdef _LITTLE_ENDIAN
-  mlib_u64 emask = (mlib_u64)((mlib_s64)(-1)) >> ((8 - (size - i)) * 
8);
+  mlib_u64 emask = (~(mlib_u64)0) >> ((8 - (size - i)) * 8);
 #else
-  mlib_u64 emask = (mlib_s64)(-1) << ((8 - (size - i)) * 8);
+  mlib_u64 emask = (~(mlib_u64)0) << ((8 - (size - i)) * 8);
 #endif /* _LITTLE_ENDIAN */
 
   ((mlib_u64*)da)[0] = (((mlib_u64*)dd_array)[sa[0]] & emask) | 
(((mlib_u64*)da)[0] &~ emask);
@@ -395,9 +395,9 @@
   }
 
 #ifdef _LITTLE_ENDIAN
-  emask = (mlib_u32)((mlib_s32)(-1)) >> ((4 - (size - i)) * 8);
+  emask = (~(mlib_u32)0) >> ((4 - (size - i)) * 8);
 #else
-  emask = (mlib_s32)(-1) << ((4 - (size - i)) * 8);
+  emask = (~(mlib_u32)0) << ((4 - (size - i)) * 8);
 #endif /* _LITTLE_ENDIAN */
   ((mlib_u32*)da)[0] = (dd1 & emask) | (((mlib_u32*)da)[0] &~ emask);
 
@@ -413,9 +413,9 @@
   }
 
 #ifdef _LITTLE_ENDIAN
-  emask = (mlib_u64)((mlib_s64)(-1)) >> ((8 - (size - i)) * 8);
+  emask = (~(mlib_u64)0) >> ((8 - (size - i)) * 8);
 #else
-  emask = (mlib_s64)(-1) << ((8 - (size - i)) * 8);
+  emask = (~(mlib_u64)0) << ((8 - (size - i)) * 8);
 #endif /* _LITTLE_ENDIAN */
   ((mlib_u64*)da)[0] = (dd & emask) | (((mlib_u64*)da)[0] &~ emask);
 
@@ -565,9 +565,9 @@
   }
 
 #ifdef _LITTLE_ENDIAN
-  emask = (mlib_u32)((mlib_s32)(-1)) >> ((4 - (size - i)) * 8);
+  emask = (~(mlib_u32)0) >> ((4 - (size - i)) * 8);
 #else
-  emask = (mlib_s32)(-1) << ((4 - (size - i)) * 8);
+  emask = (~(mlib_u32)0) << ((4 - (size - i)) * 8);
 #endif /* _LITTLE_ENDIAN */
   da[0] = (dd & emask) | (da[0] &~ emask);
 }







Andrew Leonard
Java Runtimes Development
IBM Hursley
IBM United Kingdom Ltd
Phone internal: 245913, external: 01962 815913
internet email: andrew_m_leon...@uk.ibm.com 




From:   Brian Burkhalter 
To: Magnus Ihse Bursie 
Cc: Andrew Leonard , "Lindenmaier, Goetz" 
, 2d-dev <2d-...@openjdk.java.net>, build-dev 
, core-libs-dev 
, Philip Race 
Date:   31/08/2018 15:44
Subject:Re: [OpenJDK 2D-Dev] RFR JDK-8209786: gcc 7.3 compiler 
errors on zLinux




On Aug 31, 2018, at 2:28 AM, Magnus Ihse Bursie <
magnus.ihse.bur...@oracle.com> wrote:

Magnus, Philip, Brian, Goetz, can we have a vote? => "Fix" or 
"DisableWarnings" ?

Note that this decision can be different for the two libraries. I'd argue 
that the maintainer of each library decides. And if so, it seems to be 
"compiler fix" for libfdlibm, and "source fix" for libmlib_image.

I think we can safely say “disable compiler errors” for fdlibm as 
indicated by Joe Darcy’s comment in the issue (he owns fdlibm), and source 
code change for 

[12] RFR of JDK-8210353: Move java/util/Arrays/TimSortStackSize2.java back to tier1

[Amy Lu]

test/jdk/java/util/Arrays/TimSortStackSize2.java

This test was demoted to tier2 due to JDK-8199265 (test fails with OOM). 
This issue has been fixed (in jdk-11+20) and test has been run (in 
tier2) without failure happening after that.


Let's move it back to tier1.

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

Thanks,
Amy

diff --git a/test/jdk/TEST.groups b/test/jdk/TEST.groups
--- a/test/jdk/TEST.groups
+++ b/test/jdk/TEST.groups
@@ -35,8 +35,7 @@
 :jdk_lang

 tier1_part2 = \
-    :jdk_util \
-    -java/util/Arrays/TimSortStackSize2.java
+    :jdk_util

 tier1_part3 = \
 :jdk_math \
@@ -67,9 +66,7 @@
 -sun/nio/cs/ISO8859x.java \
 :jdk_other \
 :jdk_text \
-    :jdk_time \
-    java/util/Arrays/TimSortStackSize2.java
-
+    :jdk_time

 tier2_part3 = \
 :jdk_net
diff --git a/test/jdk/java/util/Arrays/TimSortStackSize2.java 
b/test/jdk/java/util/Arrays/TimSortStackSize2.java

--- a/test/jdk/java/util/Arrays/TimSortStackSize2.java
+++ b/test/jdk/java/util/Arrays/TimSortStackSize2.java
@@ -25,7 +25,6 @@
  * @test
  * @bug 8072909
  * @summary Test TimSort stack size on big arrays
- * @key intermittent
  * @library /lib/testlibrary /test/lib
  * @modules java.management
  *  java.base/jdk.internal

Re: [12] RFR of JDK-8209832: Refactor jdk/internal/reflect/Reflection/GetCallerClassTest.sh to plain java test

[Amy Lu]

On 2018/9/4 3:14 PM, Alan Bateman wrote:

On 04/09/2018 06:41, Amy Lu wrote:

test/jdk/jdk/internal/reflect/Reflection/GetCallerClassTest.sh

Please review this patch to refactor above shell script test to java.

bug: https://bugs.openjdk.java.net/browse/JDK-8209832
webrev: http://cr.openjdk.java.net/~amlu/8209832/webrev.00/
This looks okay. If you want, you can reduce the code in 
SetupGetCallerClass by replacing the initialization of dest with 
Path.of("bcp", "boot") as userDir is not needed. Also the nonExists 
check isn't needed either.


-Alan


Thank you Alan!

I updated SetupGetCallerClass:
http://cr.openjdk.java.net/~amlu/8209832/webrev.01/

Thanks,
Amy

Re: RFR: 8139965 - Hang seen when using com.sun.jndi.ldap.search.replyQueueSize

[Daniel Fuchs]

Hi Rob,

I concur with Chris.
completed needs to be volatile and close() needs to
set a flag and use offer like cancel().

The condition for testing for closed then becomes
that the flag is set and the queue is empty or has EOF
as its head.

Is there any way this could be tested by a regression
test?

best regards,

-- daniel

On 04/09/2018 10:00, Chris Hegarty wrote:

Rob,


On 3 Sep 2018, at 22:48, Rob McKenna  wrote:

Hi folks,

I'd like to get a re-review of this change:

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



This issue is closed as `will not fix`. I presume you will re-open it before 
pushing.


http://cr.openjdk.java.net/~robm/8139965/webrev/ 




43 private boolean completed;
Won’t `completed` need to be volatile now? ( since the removal of synchronized 
from hasSearchCompleted )

LdapRequest.close puts EOF into the queue, but that is a potentially blocking 
operation ( while holding the lock ). If the queue is at capacity, then it will 
block forever. This model will only work if `getReplyBer` is always guaranteed 
to be running concurrently. Is it?

Please check the indentation of LdapRequest.java L103 ( in
the new file ). It appears, in the webrev, that the trailing `}` is
not lined up.

The indentation doesn’t look right here either.
  624 if (nparent) {
  625 LdapRequest ldr = pendingRequests;
  626 while (ldr != null) {
  627 ldr.close();
  628 ldr = ldr.next;
  629 }
  630 }
  631 }

-Chris

Re: RFR: 8139965 - Hang seen when using com.sun.jndi.ldap.search.replyQueueSize

[Chris Hegarty]

Rob,

> On 3 Sep 2018, at 22:48, Rob McKenna  wrote:
> 
> Hi folks,
> 
> I'd like to get a re-review of this change:
> 
> https://bugs.openjdk.java.net/browse/JDK-8139965 
> 

This issue is closed as `will not fix`. I presume you will re-open it before 
pushing.

> http://cr.openjdk.java.net/~robm/8139965/webrev/ 
> 


43 private boolean completed;
Won’t `completed` need to be volatile now? ( since the removal of synchronized 
from hasSearchCompleted )

LdapRequest.close puts EOF into the queue, but that is a potentially blocking 
operation ( while holding the lock ). If the queue is at capacity, then it will 
block forever. This model will only work if `getReplyBer` is always guaranteed 
to be running concurrently. Is it?

Please check the indentation of LdapRequest.java L103 ( in
the new file ). It appears, in the webrev, that the trailing `}` is
not lined up.

The indentation doesn’t look right here either.
 624 if (nparent) {
 625 LdapRequest ldr = pendingRequests;
 626 while (ldr != null) {
 627 ldr.close();
 628 ldr = ldr.next;
 629 }
 630 }
 631 }

-Chris

Re: RFR: 8139965 - Hang seen when using com.sun.jndi.ldap.search.replyQueueSize

[vyom tewari]

Hi Rob,

Code change looks good to me.

Thanks,

Vyom


On Tuesday 04 September 2018 03:18 AM, Rob McKenna wrote:

Hi folks,

I'd like to get a re-review of this change:

https://bugs.openjdk.java.net/browse/JDK-8139965
http://cr.openjdk.java.net/~robm/8139965/webrev/

In case the original has gone stale:

http://mail.openjdk.java.net/pipermail/core-libs-dev/2016-August/042767.html

 -Rob

Re: [12] RFR of JDK-8209832: Refactor jdk/internal/reflect/Reflection/GetCallerClassTest.sh to plain java test

[Alan Bateman]

On 04/09/2018 06:41, Amy Lu wrote:

test/jdk/jdk/internal/reflect/Reflection/GetCallerClassTest.sh

Please review this patch to refactor above shell script test to java.

bug: https://bugs.openjdk.java.net/browse/JDK-8209832
webrev: http://cr.openjdk.java.net/~amlu/8209832/webrev.00/
This looks okay. If you want, you can reduce the code in 
SetupGetCallerClass by replacing the initialization of dest with 
Path.of("bcp", "boot") as userDir is not needed. Also the nonExists 
check isn't needed either.


-Alan

[12] RFR 8210339: Add 10 JNDI tests to com/sun/jndi/dns/FedTests/

[Chris Yin]

Please review the changes to add 10 JNDI tests to com/sun/jndi/dns/FedTests/ in 
OpenJDK, thanks

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

Regards,
Chris