Re: JDK 8 RFR 8010371: getaddrinfo can fail with EAI_SYSTEM/EAGAIN, causes UnknownHostException to be thrown

[David Holmes]

Brian,

On 3/10/2013 5:40 AM, Brian Burkhalter wrote:

I agree that is an ugly style but it guarantees that mistakes such as

if (error = EAI_AGAIN) {}

are caught at compilation time.


True but as I understand it this is not the preferred/common style in 
the JDK code.


David

hg: jdk8/tl/jdk: 8026232: Move libnpt from profile compact1 to compact3

[david . holmes]

Changeset: 254173b48dcb
Author:dholmes
Date:  2013-10-10 04:57 -0400
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/254173b48dcb

8026232: Move libnpt from profile compact1 to compact3
Reviewed-by: mchung, alanb

! makefiles/profile-includes.txt

hg: jdk8/tl/jdk: 8026378: TEST_BUG: Clean up TEST.groups

[david . holmes]

Changeset: e33aea66caa3
Author:dholmes
Date:  2013-10-15 20:54 -0400
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/e33aea66caa3

8026378: TEST_BUG: Clean up TEST.groups
Reviewed-by: mduigou, mchung, alanb

! test/TEST.groups

Re: JDK 8 RFR 8010371: getaddrinfo can fail with EAI_SYSTEM/EAGAIN, causes UnknownHostException to be thrown

[David Holmes]

Brian,

On 17/10/2013 9:21 AM, Brian Burkhalter wrote:

Dmitry,

I think you are correct: that slipped by both me and the reviewers. I have 
reopened the issue and posted an amendment to the original webrev here:


You can not reopen a bug once it has been fixed! You need to create a 
new bug for this issue and re-close the original ensuring that you 
restore all the correct field values ie "Fixed in build".


David


http://cr.openjdk.java.net/~bpb/8010371/webrev.4-amendment/

Thanks for catching this.

Brian

On Oct 16, 2013, at 3:06 PM, Dmitry Samersoff wrote:


You have to check error != 0 before call to WSAGetLastError() at
ll. 134 Inet6AddressImpl.c

Besides that - the fix looks good for me.


Here's the patch updated for this option:

http://cr.openjdk.java.net/~bpb/8010371/webrev.4/

hg: jdk8/tl/jdk: 8025198: Intermittent test failure: java/util/concurrent/ThreadPoolExecutor/ThrowingTasks.java

[david . holmes]

Changeset: d19ab5da83cc
Author:dholmes
Date:  2013-11-04 06:58 -0500
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/d19ab5da83cc

8025198: Intermittent test failure: 
java/util/concurrent/ThreadPoolExecutor/ThrowingTasks.java
Reviewed-by: martin, dholmes
Contributed-by: Tristan Yan 

! makefiles/CompileLaunchers.gmk
! makefiles/lib/CoreLibraries.gmk
! src/share/bin/java.c
! test/java/util/concurrent/ThreadPoolExecutor/ThrowingTasks.java

URGENT - In correct push Fwd: [JBS] (JDK-8025198) Intermittent test failure: java/util/concurrent/ThreadPoolExecutor/ThrowingTasks.java

[David Holmes]

My commit pulled in a bunch of local changes that should never have been 
pushed (the import commit failed due to whitespace and when I re-issued 
the commit I didn't restrict it to the single test file).


Can anyone roll this back on the actual server?

Thanks,
David

 Original Message 
Subject: [JBS] (JDK-8025198) Intermittent test failure: 
java/util/concurrent/ThreadPoolExecutor/ThrowingTasks.java

Date: Mon, 4 Nov 2013 12:03:22 + (UTC)
From: HG Updates (JBS) 
To: david.hol...@oracle.com


 [ 
https://bugs.openjdk.java.net/browse/JDK-8025198?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel 
]


HG Updates resolved JDK-8025198.


Resolved In Build: team
Fix Version/s: 8
   Resolution: Fixed

URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/d19ab5da83cc
User:  dholmes
Date:  2013-11-04 12:01:30 +



Intermittent test failure: 
java/util/concurrent/ThreadPoolExecutor/ThrowingTasks.java
-

Key: JDK-8025198
URL: https://bugs.openjdk.java.net/browse/JDK-8025198
Project: JDK
 Issue Type: Bug
 Components: core-libs
   Affects Versions: 8
   Reporter: Amy Lu
   Assignee: Tristan Yan
 Labels: same-binary, sqebug, teststabilization
Fix For: 8


TESTFAIL:java/util/concurrent/ThreadPoolExecutor/ThrowingTasks.java
java/util/concurrent/ThreadPoolExecutor/ThrowingTasks.java failing 
intermittently:
#section:main
--messages:(3/175)--
command: main -XX:-UseVMInterruptibleIO ThrowingTasks
reason: User specified action: run main/othervm -XX:-UseVMInterruptibleIO 
ThrowingTasks
elapsed time (seconds): 480.015
--System.out:(0/0)--
--System.err:(0/0)--
result: Error. Program `/Users/aurora/sandbox_keepme/jdk/bin/java' interrupted! 
(timed out?)


--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA 
administrators: 
https://bugs.openjdk.java.net/secure/ContactAdministrators!default.jspa

For more information on JIRA, see: http://www.atlassian.com/software/jira

Re: URGENT - In correct push Fwd: [JBS] (JDK-8025198) Intermittent test failure: java/util/concurrent/ThreadPoolExecutor/ThrowingTasks.java

[David Holmes]

On 4/11/2013 10:35 PM, Alan Bateman wrote:

On 04/11/2013 12:10, David Holmes wrote:

My commit pulled in a bunch of local changes that should never have
been pushed (the import commit failed due to whitespace and when I
re-issued the commit I didn't restrict it to the single test file).

Can anyone roll this back on the actual server?

It might be simpler to just push another change that anti-deltas the
launcher/other changes. That would get jdk8/tl fixed up before too many
people are impacted.


Okay RFR sent out to core-libs. Thanks Alan.

Apologies for the mess and the noise.

David


-Alan.

hg: jdk8/tl/jdk: 8027755: Anti-delta incorrect push for 8025198

[david . holmes]

Changeset: 23982079ad49
Author:dholmes
Date:  2013-11-04 07:39 -0500
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/23982079ad49

8027755: Anti-delta incorrect push for 8025198
Reviewed-by: alanb

! makefiles/CompileLaunchers.gmk
! makefiles/lib/CoreLibraries.gmk
! src/share/bin/java.c

Re: RFR: (8030875) Macros for checking and returning on exceptions

[David Holmes]

Hi Roger,

On 11/01/2014 1:37 AM, roger riggs wrote:

Please review:

To enable native code checking consistently for thrown exceptions,
the macros in net_util.h and java/util/jar/pack/coding.cpp are
made consolidated and promoted to jni_util.h

webrev:
http://cr.openjdk.java.net/~rriggs/webrev-check-exception-8030875/


Do these macro definitions not cause "empty statement" warnings from the 
compiler? (Existing ones have the same problem I guess)


Also I don't see any use of the CHECK_EXCEPTION macros? In fact the bulk 
of changes here seem completely unrelated to the "exception" aspect of 
this CR.


Cheers,
David


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

Re: RFR(L): 8031581: PPC64: Addons and fixes for AIX to pass the jdk regression tests

[David Holmes]

Just a note on this part (I havent looked at the code):


On AIX, the constants used for the polling events (i.e. POLLIN, POLLOUT, ...) 
are defined to different values than on other operating systems. The problem is 
however, that these constants are hardcoded as public final static members of 
various, shared Java classes.


Sounds like this should be handled the same way that the other "system 
constants" are handled - you can either store a platform file in the 
repo (for cross-compiling) or you generate the class containing the 
constants at build time.


David

On 14/01/2014 6:40 PM, Volker Simonis wrote:

Hi,

could you please review the following changes for the ppc-aix-port
stage/stage-9 repositories (the changes are planned for integration into
ppc-aix-port/stage-9 and subsequent backporting to ppc-aix-port/stage):

http://cr.openjdk.java.net/~simonis/webrevs/8031581/

I've build and smoke tested without any problems on Linux/x86_64 and
PPC64, Windows/x86_64, MacOSX, Solaris/SPARC64 and AIX7PPC64.

With these changes (and together with the changes from "8028537: PPC64:
Updated jdk/test scripts to understand the AIX os and environment" and
"8031134 : PPC64: implement printing on AIX") our port passes all but
the following 7 jtreg regression tests on AIX (compared to the
Linux/x86_64 baseline from
www.java.net/download/jdk8/testresults/testresults.html
‎):

java/net/Inet6Address/B6558853.java
java/nio/channels/AsynchronousChannelGroup/Basic.java (sporadically)
java/nio/channels/AsynchronousChannelGroup/GroupOfOne.java
java/nio/channels/AsynchronousChannelGroup/Unbounded.java (sporadically)
java/nio/channels/Selector/RacyDeregister.java
sun/security/krb5/auto/Unreachable.java (only on IPv6)

Thank you and best regards,
Volker


Following a detailed description of the various changes:


src/share/native/java/util/zip/zip_util.c
src/share/native/sun/management/DiagnosticCommandImpl.c

  * According to ISO C it is perfectly legal for malloc to return zero
if called with a zero argument. Fix various places where malloc can
potentially correctly return zero because it was called with a zero
argument.
  * Also fixed |DiagnosticCommandImpl.c| to include |stdlib.h|. This
only fixes a compiler warning on Linux, but on AIX it prevents a VM
crash later on because the return value of |malloc()| will be casted
to |int| which is especially bad if that pointer was bigger than
32-bit.


make/CompileJavaClasses.gmk

  * Also use |PollingWatchService| on AIX.


make/lib/NioLibraries.gmk
src/aix/native/sun/nio/ch/AixNativeThread.c

  * Put the implementation for the native methods of |NativeThread| into
|AixNativeThread.c| on AIX.


src/solaris/native/sun/nio/ch/PollArrayWrapper.c
src/solaris/native/sun/nio/ch/Net.c
src/aix/classes/sun/nio/ch/AixPollPort.java
src/aix/native/sun/nio/ch/AixPollPort.c
src/aix/native/java/net/aix_close.c

  * On AIX, the constants used for the polling events (i.e. |POLLIN|,
|POLLOUT|, ...) are defined to different values than on other
operating systems. The problem is however, that these constants are
hardcoded as public final static members of various, shared Java
classes. We therefore have to map them from Java to native every
time before calling one of the native poll functions and back to
Java after the call on AIX in order to get the right semantics.


src/share/classes/java/nio/file/CopyMoveHelper.java

  * As discussed on the core-libs mailing list (see

http://mail.openjdk.java.net/pipermail/core-libs-dev/2013-December/024119.html)
it is not necessary to call |Files.getFileAttributeView()| with any
|linkOptions| because at that place we've already checked that the
target file can not be a symbolic link. This change makes the
implementation more robust on platforms which support symbolic links
but do not support the |O_NOFOLLOW| flag to the |open| system call.
It also makes the JDK pass the |demo/zipfs/basic.sh| test on AIX.


src/share/classes/sun/nio/cs/ext/ExtendedCharsets.java

  * Support "compound text" on AIX in the same way like on other Unix
platforms.



src/share/classes/sun/tools/attach/META-INF/services/com.sun.tools.attach.spi.AttachProvider

  * Define the correct attach provider for AIX.


src/solaris/native/java/net/net_util_md.h
src/solaris/native/sun/nio/ch/FileDispatcherImpl.c
src/solaris/native/sun/nio/ch/ServerSocketChannelImpl.c

  * AIX needs a workaround for I/O cancellation (see:

http://publib.boulder.ibm.com/infocenter/pseries/v5r3/index.jsp?topic=/com.ibm.aix.basetechref/doc/basetrf1/close.htm).
"..The |close()| subroutine is blocked until all subroutines which
use the file descriptor return to usr space. For example, when a
thread is calling close and another t

Re: RFR(L): 8031581: PPC64: Addons and fixes for AIX to pass the jdk regression tests

[David Holmes]

On 15/01/2014 12:10 AM, Volker Simonis wrote:

On Tue, Jan 14, 2014 at 12:29 PM, David Holmes mailto:david.hol...@oracle.com>> wrote:

Just a note on this part (I havent looked at the code):


On AIX, the constants used for the polling events (i.e. POLLIN,
POLLOUT, ...) are defined to different values than on other
operating systems. The problem is however, that these constants
are hardcoded as public final static members of various, shared
Java classes.


Sounds like this should be handled the same way that the other
"system constants" are handled - you can either store a platform
file in the repo (for cross-compiling) or you generate the class
containing the constants at build time.


Hi David,

thanks for your comments. That sound like a good idea but I'm not sure
if it would make sense to duplicate the following files:

src/share/classes/sun/nio/ch/AbstractPollArrayWrapper.java:
src/solaris/classes/sun/nio/ch/Port.java

because of this. Do you have a concrete example where Java-classes are
being generated with different constants in the class library build?


There are two files generated:

UnixConstants.java (or SolarisConstants.java) for general I/O type values
SocketOptionRegistry.java for socket options.

See jdk/make/gensrc/GensrcMisc.gmk.


Both solutions would result in different class files on Aix and other
Unix variants. What do you think about assigning the concrete values
depending on 'os.name <http://os.name>' in the static initializers of
the corresponding classes? I think that shouldn't introduce too much
overhead and I could get rid of all the ugly conversion code.


I'm not a fan of runtime checks of this kind though if it is only a very 
samll number of values it might be okay.


Another option would be to make those classes into "templates" as done 
with Version.java.template and substitute the right values at build time.


But I'll let Alan and net-dev folk come back with their preferred 
technique for this.


Cheers,
David



Regards,
Volker

David


On 14/01/2014 6:40 PM, Volker Simonis wrote:

Hi,

could you please review the following changes for the ppc-aix-port
stage/stage-9 repositories (the changes are planned for
integration into
ppc-aix-port/stage-9 and subsequent backporting to
ppc-aix-port/stage):

http://cr.openjdk.java.net/~__simonis/webrevs/8031581/
<http://cr.openjdk.java.net/~simonis/webrevs/8031581/>

I've build and smoke tested without any problems on Linux/x86_64 and
PPC64, Windows/x86_64, MacOSX, Solaris/SPARC64 and AIX7PPC64.

With these changes (and together with the changes from "8028537:
PPC64:
Updated jdk/test scripts to understand the AIX os and
environment" and
"8031134 : PPC64: implement printing on AIX") our port passes
all but
the following 7 jtreg regression tests on AIX (compared to the
Linux/x86_64 baseline from
www.java.net/download/jdk8/__testresults/testresults.html
<http://www.java.net/download/jdk8/testresults/testresults.html>
<http://www.java.net/download/__jdk8/testresults/testresults.__html
<http://www.java.net/download/jdk8/testresults/testresults.html>>‎):


java/net/Inet6Address/__B6558853.java
java/nio/channels/__AsynchronousChannelGroup/__Basic.java
(sporadically)
java/nio/channels/__AsynchronousChannelGroup/__GroupOfOne.java
java/nio/channels/__AsynchronousChannelGroup/__Unbounded.java
(sporadically)
java/nio/channels/Selector/__RacyDeregister.java
sun/security/krb5/auto/__Unreachable.java (only on IPv6)

Thank you and best regards,
Volker


Following a detailed description of the various changes:


 src/share/native/java/util/__zip/zip_util.c
 src/share/native/sun/__management/__DiagnosticCommandImpl.c

   * According to ISO C it is perfectly legal for malloc to
return zero

 if called with a zero argument. Fix various places where
malloc can
 potentially correctly return zero because it was called
with a zero
 argument.
   * Also fixed |DiagnosticCommandImpl.c| to include |stdlib.h|.
This

 only fixes a compiler warning on Linux, but on AIX it
prevents a VM
 crash later on because the return value of |malloc()| will
be casted
 to |int| which is especially bad if that pointer was bigger
than
 32-bit.


 make/CompileJavaClasses.gmk

   * Also use |PollingWatchService| on AIX.


 make/lib/NioLibraries.gmk
 src/aix/native/sun/nio/ch

Re: Discussion on root cause analysis of JDK-7052625 : com/sun/net/httpserver/bugs/6725892/Test.java fails intermittently

[David Holmes]

Sounds like a question for net-dev more than core-libs - cc'd.

David

On 20/02/2014 4:11 PM, michael cui wrote:

On 02/18/2014 12:51 AM, michael cui wrote:

Hi,

I would like to discuss my current root cause analysis of JDK-7052625
: com/sun/net/httpserver/bugs/6725892/Test.java fails intermittently

As JDK-6725892 
stated, the purpose of this regression test is testing bad http
connections can be handled correctly which including
+ send no request
+ send an incomplete request
+ fail to read the response completely.

test3() method will start 20 threads for each type listed above at
same time. So totally 60 threads started in test3(). Each thread will
open connection to httpserver and simulate the normal or bad http
request to see if http server can handle them correctly. (20 threads
for incomplete read, 20 threads for incomplete write, 20 threads for
read/write normal case)

Those threads will be started at same time. Among them, 40 threads
using sleep to simulate bad request.

The http server created by the following api call :
s1 = HttpServer.create (addr, 0);

According API doc

and ServerSocket.java source code, the second parameter is backlog of
socket which is the maximum number of queued incoming connections to
allow on the listening socket. Queued TCP connections exceeding this
limit may be rejected by the TCP implementation.. The default value 50
will be used if it was set to zero (See api doc

and ServerSocket.java )).

Since in test3(), 40 threads out of total 60 threads will simulate bad
http request by sleeping either at reading or writing, there could be
a very little possibility that httpserver 's socket connection queue
reach his limit (50 for default value) and some tcp connection will be
rest at that situation.

This could be the root cause of this intermittently failure.

Test result of the original version :
0 failure on Linux for 1 runs.
0 failure on solaris for 1 runs.
6 failure on windows for 1 runs
28 failures on mac for 1 runs

By increasing the thread number of bad request, we can observe that
the frequency of failure will be increased.

Test result of fix version in which backlog of httpserver was changed
from 0 to 100.
0 failure on Linux for 1 runs.
0 failure on solaris for 1 runs.
0 failure on windows for 1 runs
0 failures on mac for 1 runs

It seems to me that using default 0 for backlog of httpserver could be
root cause of this intermittently failure.
Are we comfortable with this analysis? If it is the root cause, could
setting backlog as 100 be a suggest fix?

Thanks,
Michael Cui


Could anyone provide some insight on this analysis?

Michael Cui

Re: [patch] JDK-4906983

[David Holmes]

Hi Sebastian,

On 8/09/2015 1:54 PM, Sebastian Sickelmann wrote:

Hi,

please find the link to the webrev[1] hosted at my dropbox in my first mail in 
this thread at the buttom of this mail.

A direkt link to the including patch file can be found here: 
https://dl.dropboxusercontent.com/u/43692695/oss-patches/openjdk/JDK-4906983/jdk.patch


Another rule is that all submitted patches must come in via OpenJDK 
infrustructure so they fall under the term-of-use [1] - either within 
the email to the list (though attachments get stripped often), or hosted 
on cr.openjdk.java.net. An OpenJDK Author can host the patch on 
cr.openjdk.java.net on your behalf.


Cheers,
David

[1] http://openjdk.java.net/legal/tou/terms



-- Sebastian

Am 08.09.2015 2:03 vorm. schrieb David Holmes :


On 7/09/2015 11:46 PM, Ivan Krylov wrote:

Hi Sebastian,

The current OpenJDK rules [1] do not allow to accept patches from people
that are not OCA signatories.


For the record, small patches can be accepted:

"A Participant is an individual who has subscribed to one or more
OpenJDK mailing lists. A Participant may post messages to a list, submit
simple patches, and make other kinds of small contributions.

A Contributor is a Participant who has signed the Oracle Contributor
Agreement (OCA), ..."

---

I did not see the referenced patch so don't know if it would be
considered small or not.

Cheers,
David


If you would like your patch to be considered - please sign the OCA[2],
that will be reflected at [3].
The process of becoming an OpenJDK contributor is described at [4].

Thanks,

Ivan


[1] http://openjdk.java.net/bylaws
[2] http://www.oracle.com/technetwork/oca-405177.pdf
[3] http://openjdk.java.net/census
[4] http://openjdk.java.net/contribute/


On 06/09/2015 09:58, Sebastian Sickelmann wrote:

Please find my small patch[1] to javadoc in java.net.URL that adresses
JDK-4906983(javadoc-fix)[2].

-- Sebastian

[1]
https://dl.dropboxusercontent.com/u/43692695/oss-patches/openjdk/JDK-4906983/index.html

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

Re: ServerImpl misplaced null check

[David Holmes]

Hi Scott,

cc'ing net-dev as this is not a build issue.

David

On 21/01/2016 4:18 AM, Scott Palmer wrote:

I was searching for a way to set TCP_NODELAY for an Endpoint using the default 
HTTP server and after finally tracking down the existence of the 
“sun.net.httpserver.nodelay” system property I noticed what appears to be a 
mistake (though not a big one) in the source code.

Look here:
http://hg.openjdk.java.net/jdk9/jdk9/jdk/file/c3ecf996006a/src/jdk.httpserver/share/classes/sun/net/httpserver/ServerImpl.java#l377

Notice the null check on ‘chan’ is performed after it may have been 
dereferenced to set the TCP_NODELAY on the channel’s socket.  If chan truly can 
be null at that point, the check needs to be move up a few lines.

Regards,

Scott

Re: [DONG] Re: [DING] Re: [PING] Potential infinite waiting at JMXConnection#createConnection

[David Holmes]

Hi Yuji,

Not sure who would look at this so cc'ing net-dev.

Also note that contributions can only be accepted if presented via 
OpenJKDK infrastructure. Links to patches on 
http://icedtea.classpath.org are not acceptable. The patch needs to be 
included in the email (beware stripped attachments) if you can't get it 
hosted on cr.openjdk.java.net. Sorry.


David

On 9/02/2016 12:10 AM, KUBOTA Yuji wrote:

Hi all,

Could someone review this fix?

Thanks,
Yuji

2016-02-04 2:27 GMT+09:00 KUBOTA Yuji :

Hi all,

Could someone please review and sponsor this fix ?
I write the details of this issue again. Please review it.

=Problem=
Potential infinite waiting at TCPChannel#createConnection.

This method flushes the DataOutputStream without the socket
timeout settings when choose stream protocol [1]. If connection lost
or the destination server do not return response during the flush,
this method wait forever because the timeout settings is set the
default value of SO_TIMEOUT, i.e., infinite.

[1]: 
http://hg.openjdk.java.net/jdk9/dev/jdk/file/7adef1c3afd5/src/java.rmi/share/classes/sun/rmi/transport/tcp/TCPChannel.java#l227

I think this issue is rarely, however serious.

=Reproduce=
I write a test program to reproduce. You can reproduce by the below.

* hg clone 
http://icedtea.classpath.org/people/ykubota/fixLoopAtJMXConnectorFactory/
* cd fixLoopAtJMXConnectorFactory; mvn package
* setting "stop_time" at debugcontrol.properties if you need.
* java -cp .:target/debugcontrol-1.0-SNAPSHOT.jar debugcontrol.DebugController

This program keep to wait at TCPChannel#createConnection due to
this issue. After "debugcontroltest.stop_time" ms, this program release
the waiting by sending quit to jdb which is stopping the destination
server. Finally, return 2.

=Solution=
Set timeout by using property-configured value:
sun.rmi.transport.tcp.responseTimeout.

My patch is below.
http://icedtea.classpath.org/people/ykubota/fixLoopAtJMXConnectorFactory/file/e31044f0804f/jdk9.patch

If you run the test program with modified JDK9 by my patch, the test
program will get java.net.SocketTimeoutException after the connection
timeout happen, then return 0.

Thanks,
Yuji.


2016-01-13 23:31 GMT+09:00 KUBOTA Yuji :

Hi all,

Can somebody please review and sponsor this fix ?

Thanks,
Yuji

2016-01-05 17:56 GMT+09:00 KUBOTA Yuji :

Hi Jaroslav and core-libs-dev,

Thank Jaroslav for your kindness!

For core-libs-dev members, links the information about this issue.

  * details of problem
  http://mail.openjdk.java.net/pipermail/jdk9-dev/2015-April/002152.html

  * patch
  
http://icedtea.classpath.org/people/ykubota/fixLoopAtJMXConnectorFactory/file/e31044f0804f/jdk9.patch

  * testcase for reproduce
  
http://icedtea.classpath.org/people/ykubota/fixLoopAtJMXConnectorFactory/file/e31044f0804f/testProgram
  
http://mail.openjdk.java.net/pipermail/serviceability-dev/2015-December/018415.html

Could you please review these reports?
Hope this patch helps to community.

Thanks,
Yuji

2016-01-04 23:51 GMT+09:00 Jaroslav Bachorik :

Hi Yuji,

On 4.1.2016 15:14, KUBOTA Yuji wrote:


Hi all,

Could you please review this patch?



Sorry for the long delay. Shanliang has not been present for some time and
probably this slipped the attention of the others.

However, core-libs mailing list might be more appropriate place to review
this change since you are dealing with s.r.t.t.TCPChannel
(http://icedtea.classpath.org/people/ykubota/fixLoopAtJMXConnectorFactory/file/e31044f0804f/jdk9.patch)

Regards,

-JB-

Re: JDK10 RFR: 8165437 Evaluate the use of gettimeofday in Networking code

[David Holmes]

please find the updated
webrev(http://cr.openjdk.java.net/~vtewari/8165437/webrev0.7/index.html).


This change is broken on 32-bit systems - JVM_Nanotime returns a jlong 
which is always 64-bit, but the code uses long for the nanotimeout 
values, which will be 32-bit on 32-bit systems!


This change appears to be causing massive testing failures due to jtreg 
agentvm failing due to sock connection issues. Most obvious with 32-bit 
linux builds. However we also see a some failures on OSX which is not 
explained by the long vs jlong issue.


David
-

Re: JDK10 RFR: 8165437 Evaluate the use of gettimeofday in Networking code

[David Holmes]

On 4/05/2017 2:07 PM, Vyom Tewari wrote:

Hi David,

I will look into the issue.


Thanks. I filed:

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

as you probably saw.

David



Thanks,

Vyom


On Thursday 04 May 2017 06:29 AM, David Holmes wrote:

please find the updated
webrev(http://cr.openjdk.java.net/~vtewari/8165437/webrev0.7/index.html).



This change is broken on 32-bit systems - JVM_Nanotime returns a jlong
which is always 64-bit, but the code uses long for the nanotimeout
values, which will be 32-bit on 32-bit systems!

This change appears to be causing massive testing failures due to
jtreg agentvm failing due to sock connection issues. Most obvious with
32-bit linux builds. However we also see a some failures on OSX which
is not explained by the long vs jlong issue.

David
-

Re: Adding SocketChannel toString to connection exception messages

[David Holmes]

cc'ing net-dev as that might be the more appropriate list.

On 22/12/2017 10:59 AM, Steven Schlansker wrote:



On Dec 21, 2017, at 4:35 PM, David Holmes  wrote:

On 22/12/2017 10:29 AM, Steven Schlansker wrote:

On Dec 21, 2017, at 11:11 AM, Steven Schlansker  
wrote:

What if ConnectException included the attempted hostname / IP / port 
SocketAddress?
java.net.ConnectException: Connection to 'foo.mycorp.com[10.x.x.x]:12345' 
refused
Much more useful!  This could also be extended to various other socket 
exceptions.


I believe there are concerns with too much information that can be considered 
"sensitive" (like host names and IP addresses) appearing in error messages due 
to them ending up in log files and bug reports.


Unfortunately that's exactly the information that is crucial to someone trying 
to diagnose issues...
Could it be an opt-in policy?  Perhaps by a system property?


The expectation is that such information should be added by layers 
higher in the call chain, rather than the JDK libraries assuming it is 
okay to do.



Currently the alternative I'm faced with is going through every piece of user 
code and library that *might*
throw this exception and wrapping it to add this critical diagnostic 
information.  For an application that uses
java.net heavily, you can imagine how that is a tall task and possibly even not 
realistically achievable...

(Is there a written policy regarding this somewhere, or is it up to the 
personal feelings of the contributors?)


This is covered by the secure coding guidelines, under 'Confidential 
information':


http://www.oracle.com/technetwork/java/seccodeguide-139067.html#2

"Guideline 2-1 / CONFIDENTIAL-1: Purge sensitive information from 
exceptions"


I know for a fact that we'd have to scrub this information from test 
failures when putting the info into a bug report.


If net-dev folk can't expand further on this then I suggest filing a CSR 
request so that the CSR group can consider it. But I think this will be 
a no-go (I'm a member of the CSR group).


Cheers,
David

Re: RFR 8203369 : Check for both EAGAIN and EWOULDBLOCK error codes

[David Holmes]

Hi Ivan,

Just a thought, but just because the actual native function may return 
either code, that doesn't mean our native wrapper can't treat them the 
same and present them to the Java code as one error?


It seems pointless to double up these condition checks everywhere just 
in case there is some platform (do we know of one?) where this may be 
necessary.


I also wonder whether a smart compiler might not flag code where the 
errors do infact have the same value:


if (errno == 11 || errno == 11) ...

Cheers,
David

On 25/05/2018 6:57 AM, Ivan Gerasimov wrote:

Hello!

On Unix systems several system calls (including pread, read, readv, 
recvfrom, recvmsg, send, sendfile, sendmsg, sendto) may set errno to 
either EAGAIN or EWOULDBLOCK on the same condition.


On Linux these two constants are the same, but they are not required to 
be the same.


For example, here's an extract from the Linux man page of send():
EAGAIN or EWOULDBLOCK
The  socket  is marked nonblocking and the requested operation would 
block.  POSIX.1-2001 allows either error to be returned for this case, 
and does not require these constants to have the same value, so a 
portable application should check for both possibilities.


We should check for both error codes when appropriate.

Would you please help review the fix?

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

Thanks!

Re: RFR 8203369 : Check for both EAGAIN and EWOULDBLOCK error codes

[David Holmes]

Hi Ivan,

On 25/05/2018 1:58 PM, Ivan Gerasimov wrote:

Hi David!

Thank you for reviewing the fix!


I looked but did not review - it was just an observation :)



On 5/24/18 7:44 PM, David Holmes wrote:

Hi Ivan,

Just a thought, but just because the actual native function may return 
either code, that doesn't mean our native wrapper can't treat them the 
same and present them to the Java code as one error?


Currently in some places we check for only one of the values (on the 
supported platforms we could have being checking for the other with 
exactly same effect).  In other places we already check for both values, 
so it is proposed to do it consistently with accordance to the 
documentation.


It seems pointless to double up these condition checks everywhere just 
in case there is some platform (do we know of one?) where this may be 
necessary.


That's exactly what man pages suggest: "...a portable application should 
check for both..."


Yes but that's the native code that calls the library methods. That 
doesn't necessarily mean we have to propagate the ambiguity through our 
own native wrappers and/or Java code.



And yes, there exist such platforms.

I also wonder whether a smart compiler might not flag code where the 
errors do infact have the same value:


if (errno == 11 || errno == 11) ...

At least gcc -O completely removes the second redundant test, so no 
observable changes is expected on supported platforms.


I'm more worried about a new compiler warning - especially if you 
happened to use them in a switch! - resulting in future build failures.


Cheers,
David


With kind regards,
Ivan


Cheers,
David

On 25/05/2018 6:57 AM, Ivan Gerasimov wrote:

Hello!

On Unix systems several system calls (including pread, read, readv, 
recvfrom, recvmsg, send, sendfile, sendmsg, sendto) may set errno to 
either EAGAIN or EWOULDBLOCK on the same condition.


On Linux these two constants are the same, but they are not required 
to be the same.


For example, here's an extract from the Linux man page of send():
EAGAIN or EWOULDBLOCK
The  socket  is marked nonblocking and the requested operation would 
block.  POSIX.1-2001 allows either error to be returned for this 
case, and does not require these constants to have the same value, so 
a portable application should check for both possibilities.


We should check for both error codes when appropriate.

Would you please help review the fix?

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

Thanks!

Re: RFR 8203369 : Check for both EAGAIN and EWOULDBLOCK error codes

[David Holmes]

On 25/05/2018 3:34 PM, Ivan Gerasimov wrote:

Hi David!


On 5/24/18 9:45 PM, David Holmes wrote:


I looked but did not review - it was just an observation :)



Well, thank you anyway :)



It seems pointless to double up these condition checks everywhere 
just in case there is some platform (do we know of one?) where this 
may be necessary.


That's exactly what man pages suggest: "...a portable application 
should check for both..."


Yes but that's the native code that calls the library methods. That 
doesn't necessarily mean we have to propagate the ambiguity through 
our own native wrappers and/or Java code.


Ah, I didn't immediately understood you're talking about constants in 
UnixConstants.java and LinuxWatchService.java.
This part might probably be skipped (because we know that on Linux the 
constants have the same values), but I thought it's better it add it for 
consistency.


In other parts of the fix we do treat the constants uniformly and 
propagate some non-ambiguous value to Java, like returning 
IOS_UNAVAILABLE in most cases.



And yes, there exist such platforms.

I also wonder whether a smart compiler might not flag code where the 
errors do infact have the same value:


if (errno == 11 || errno == 11) ...

At least gcc -O completely removes the second redundant test, so no 
observable changes is expected on supported platforms.


I'm more worried about a new compiler warning - especially if you 
happened to use them in a switch! - resulting in future build failures.




What compiler do you mean: gcc or javac?


gcc

If gcc, then we already have the same test for both constants in code 
with no issues.
For example, java.base/unix/native/libnet/SocketInputStream.c, in 
NET_ReadWithTimeout():

     result = NET_NonBlockingRead(fd, bufP, len);
     if (result == -1 && ((errno == EAGAIN) || (errno == 
EWOULDBLOCK))) {



If javac, then, I was thinking about it too, but I don't have a good a 
universal solution to propose right now.


javac should treat these symbolically rather than based on actual value, 
so I don't see any issue there. It's the C compiler that sees the raw 
value after preprocessing and so sees "duplicate" clauses.


If one day someone needs to use these (platform dependent by definition) 
constants in switch, one will need to invent something to workaround the 
fact that some constants may have the same values on some platforms.
With respect to EAGAIN and EWOULDBLOCK, it will be caught early enough 
because it will fail during the very first build on any currently 
supported Unix platform.


Ok.

Cheers,
David



With kind regards,
Ivan



Cheers,
David


With kind regards,
Ivan


Cheers,
David

On 25/05/2018 6:57 AM, Ivan Gerasimov wrote:

Hello!

On Unix systems several system calls (including pread, read, readv, 
recvfrom, recvmsg, send, sendfile, sendmsg, sendto) may set errno 
to either EAGAIN or EWOULDBLOCK on the same condition.


On Linux these two constants are the same, but they are not 
required to be the same.


For example, here's an extract from the Linux man page of send():
EAGAIN or EWOULDBLOCK
The  socket  is marked nonblocking and the requested operation 
would block.  POSIX.1-2001 allows either error to be returned for 
this case, and does not require these constants to have the same 
value, so a portable application should check for both possibilities.


We should check for both error codes when appropriate.

Would you please help review the fix?

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

Thanks!

Re: RFR 8203369 : Check for both EAGAIN and EWOULDBLOCK error codes

[David Holmes]

On 25/05/2018 4:14 PM, Ivan Gerasimov wrote:

Hi David!

If gcc, then we already have the same test for both constants in code 
with no issues.
For example, java.base/unix/native/libnet/SocketInputStream.c, in 
NET_ReadWithTimeout():

 result = NET_NonBlockingRead(fd, bufP, len);
 if (result == -1 && ((errno == EAGAIN) || (errno == 
EWOULDBLOCK))) {



If javac, then, I was thinking about it too, but I don't have a good 
a universal solution to propose right now.


javac should treat these symbolically rather than based on actual 
value, so I don't see any issue there. It's the C compiler that sees 
the raw value after preprocessing and so sees "duplicate" clauses.


If fact, as UnixContant.java is built from the template file 
UnixConstants.java.template, which is processed with cpp preprocessor, 
javac also sees the raw values.


So if someone decides to write something like
switch (exc.errno()) {
     case UnixConstants.EAGAIN:
     case UnixConstants.EWOULDBLOCK:
}
then there will be compile time trouble on some platforms and no issues 
on the others.


I was thinking javac wouldn't examine the actual value of the fields, 
but it does for switch :(


Oh well.

David
-

Fortunately, UnixContant class is package private, so it is quite 
unlikely to happen.


I was even thinking about proposing a language-level enhancement: If a 
switch statement contains duplicate cases *and* these are combined, then 
treat them as just one case.

Though the use case it too limited to justify the need :)

With kind regards,
Ivan

Re: RFR : 8205959 : Do not restart close if errno is EINTR

[David Holmes]

In reference to 8205959, where is it stated that dup2 is any more 
restartable than close ??


AFAICS both leave things undefined/unspecified if they set EINTR.

David

On 2/07/2018 7:03 PM, Langer, Christoph wrote:

Hi Matthias,

forwarding to serviceability-dev, because debugging is usually discussed there.

Yes, I would think this coding should be fixed, too. Can you open a bug and 
prepare a change?

Thanks
Christoph


-Original Message-
From: net-dev [mailto:net-dev-boun...@openjdk.java.net] On Behalf Of
Norman Maurer
Sent: Montag, 2. Juli 2018 10:23
To: Baesken, Matthias 
Cc: Stuefe, Thomas ; net-dev@openjdk.java.net
Subject: Re: RFR : 8205959 : Do not restart close if errno is EINTR

+1 retry a close on EINTR has most likely not the outcome you expect and
may even close a wrong FD if the same FD is reused already (as even if EINTR
is returned it may have closed the FD)


Am 02.07.2018 um 10:17 schrieb Baesken, Matthias

:


Hello  ,  there is a similar pattern (attempt to restart close in case of EINTR)

in the coding as well   in  socket_md.c   :


src/jdk.jdwp.agent/unix/native/libdt_socket/socket_md.c-147-int rv;
src/jdk.jdwp.agent/unix/native/libdt_socket/socket_md.c-148-do {
src/jdk.jdwp.agent/unix/native/libdt_socket/socket_md.c-149-rv =

close(fd);

src/jdk.jdwp.agent/unix/native/libdt_socket/socket_md.c:150:} while (rv

== -1 && errno == EINTR);

src/jdk.jdwp.agent/unix/native/libdt_socket/socket_md.c-151-
src/jdk.jdwp.agent/unix/native/libdt_socket/socket_md.c-152-return rv;
src/jdk.jdwp.agent/unix/native/libdt_socket/socket_md.c-153-}

Do you think this needs adjustment   (on LINUX)  as well ?

Best regards, Matthias



Message: 2
Date: Thu, 28 Jun 2018 18:19:46 +0100
From: Alan Bateman 
To: David Lloyd , ivan.gerasi...@oracle.com
Cc: OpenJDK Network Dev list 
Subject: Re: RFR : 8205959 : Do not restart close if errno is EINTR
Message-ID: <3fd1496f-ab83-a2d5-0699-13c8b735d...@oracle.com>
Content-Type: text/plain; charset=utf-8; format=flowed


On 28/06/2018 17:35, David Lloyd wrote:
:
Do you (or Alan) think that this might have accounted for real-world
connection problems?


In the file I/O area, with NFS I think, we had an issue a long time ago
where close was retried after EIO. That issue was fixed a long time ago
but it's one that comes to mind in this general area.

-Alan

Re: [CAUTION] RFR : 8211146 : fix problematic elif-tests after recent gcc warning changes Werror=undef

[David Holmes]

That all seems fine to me.

Thanks for fixing.

David

On 26/09/2018 5:48 AM, Langer, Christoph wrote:

Hi Matthias,

looks good (and trivial). Ccing serviceability-dev because of change in 
libjdwp.


Best regards

Christoph

*From:*nio-dev  *On Behalf Of 
*Baesken, Matthias

*Sent:* Mittwoch, 26. September 2018 11:25
*To:* 'build-...@openjdk.java.net' ; net-dev 
; nio-...@openjdk.java.net
*Cc:* Lindenmaier, Goetz ; Schmidt, Lutz 

*Subject:* [CAUTION] RFR : 8211146 : fix problematic elif-tests after 
recent gcc warning changes Werror=undef


Hello, please review this small build fix .

After the recent  changes  to  the gcc compile flags   with  8211029   
  ,  most of our  Linux builds stopped working .


Error :

=== Output from failing command(s) repeated here ===

* For target support_native_java.base_libnet_DatagramPacket.o:

In file included from 
/OpenJDK/8210319/jdk/src/java.base/share/native/libnet/net_util.h:31:0,


 from 
/OpenJDK/8210319/jdk/src/java.base/share/native/libnet/DatagramPacket.c:27:


/OpenJDK/8210319/jdk/src/java.base/unix/native/libnet/net_util_md.h:50:7: error: 
"__solaris__" is not defined [-Werror=undef]


#elif __solaris__

    ^

After looking into it,  it seems  that  the   
jdk/src/java.base/unix/native/libnet/net_util_md.h    compile error is 
only triggered on older Linux OS  (e.g. SLES11).


Probably that’s why it was not seen at Oracle  after  puhsing  after 
8211029   .


Some greps  showed me a number of similar problematic  #elif-checks for 
OS, I adjusted them too .


Bug / webrev :

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

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



Thanks, Matthias

Re: RFR: 8210107: vmTestbase/nsk/stress/network tests fail with Cannot assign requested address (Bind failed)

[David Holmes]

Hi Leonid,

This seems fine to me. It's been some time since hotspot hosted any of 
the native code used by networking.


Thanks,
David

On 5/12/2018 7:36 am, Leonid Mesnik wrote:

Hi

Could you please review following fix which removes 
vmTestbase/nsk/stress/network tests. These tests  are pretty old stress 
networking tests and are don't seem to be useful for hotspot.
I was not able to find any product bugs related with these tests failures.

Bug JDK-4245704  which is 
mentioned in the test sources is not related with these tests itself.

If anyone from network team consider these tests useful then they could be just 
moved in more appropriate location.

I verified that there are no additional changes in TEST.groups file are 
required.

webrev: http://cr.openjdk.java.net/~lmesnik/8210107/webrev.00/ 

bug: https://bugs.openjdk.java.net/browse/JDK-8210107 


Leonid

Re: RFR(xs): 8221375: Windows 32bit build (VS2017) broken

[David Holmes]

Hi Thomas,

On 25/03/2019 5:01 pm, Thomas Stüfe wrote:

Hi David,

(added net-dev, awt-dev, security-dev since part of these fixes are in 
their territory)


May be better to split up the reviews, cross-posting that many groups 
gets very messy given most people won't be subscribed to them all - 
myself included.


On Mon, Mar 25, 2019 at 1:34 AM David Holmes <mailto:david.hol...@oracle.com>> wrote:


Hi Thomas,

A few queries, comments and concerns ...

On 25/03/2019 6:58 am, Thomas Stüfe wrote:
 > Hi all,
 >
 > After a long time I tried to build a Windows 32bit VM (VS2017)
and failed;

I'm somewhat surprised as I thought someone was actively doing Windows
32-bit builds out there, plus there are shared code changes that should
also have been caught by non-Windows 32-bit builds. :(


Not sure what others do. I did a 32bit windows build, slowdebug, warning 
enabled, and it failed with those 5+ issues.


 > multiple errors and warnings. Lets reverse the bitrot:
 >
 > cr:
 >

http://cr.openjdk.java.net/~stuefe/webrevs/8221375--windows-32bit-build-(vs2017)-broken-in-many-places/webrev.00/webrev/
 >
 > Issue: https://bugs.openjdk.java.net/browse/JDK-8221375
 >
 > Most of the fixes are trivial: Calling convention mismatches (awt
DTRACE
 > callbacks), printf specifiers etc.
 >
 > Had to supress a warning in os_windows_x86.cpp - I was surprised
by this
 > since this did not look 32bit specifc, do we disable warnings on
Windows
 > 64bit builds?

What version of VS2017? We use VS2017 15.9.6 and we don't disable
warnings.


I use VS2017 CE. Not sure which version spcifically, but my compiler is at

Microsoft (R) C/C++ Optimizing Compiler Version 19.14.26431 for x86


I think that would equate to an older version - 15.7

MSVC++ 14.14 _MSC_VER == 1914 (Visual Studio 2017 version 15.7)

Any chance you can upgrade to latest version? (Especially in light of 
the apparent compiler bug you cite below.)


Thanks,
David
-


 > The error I had in vmStructs.cpp was a bit weird: compiler
complained about
 > an assignment of an enum value defined like this:
 >
 > hash_mask_in_place       = (address_word)hash_mask << hash_shift
 >
 > to an uint64_t variable, complaining about narrowing. I did not
find out
 > what his problem was. In the end, I decided to add an explicit
cast to
 > GENERATE_VM_LONG_CONSTANT_ENTRY(name) (see vmStructs.hpp).

Not at all sure that's the right fix. In markOop.hpp we see that value
gets special treatment on Windows-x64:


#ifndef _WIN64
           ,hash_mask               = right_n_bits(hash_bits),
           hash_mask_in_place       = (address_word)hash_mask <<
hash_shift
#endif
    };

    // Alignment of JavaThread pointers encoded in object header
required
by biased locking
    enum { biased_lock_alignment    = 2 << (epoch_shift + epoch_bits)
    };

#ifdef _WIN64
      // These values are too big for Win64
      const static uintptr_t hash_mask = right_n_bits(hash_bits);
      const static uintptr_t hash_mask_in_place  =
                              (address_word)hash_mask << hash_shift;
#endif

perhaps something special is needed for Windows-x86. I'm unclear how
the
values can be "too big" ??


I banged my head against this for an hour or so and I think this is a 
compiler bug.


What I get is:

warning C4838: conversion from '' to 'uint64_t' requires a narrowing 
conversion


(Note the empty "from" string.)

Here are my tries to provoke the error:

VMLongConstantEntry iii[] = {  { "hallo", 
markOopDesc::hash_mask_in_place }, {0,0}};  <<< this fails
VMLongConstantEntry iii = { "hallo", markOopDesc::hash_mask_in_place };  
  << but this succeeds

uint64_t iii = markOopDesc::hash_mask_in_place;   << this succeeds  too

I have no clue what this means. It is difficult to fix since the 
expression is hidden in such a macro pile. But I think either we go with 
my change or we disable the warning for win32 for the whole section.


 >
 > With this patch we can build with warnings enabled on 32bit and 64bit
 > windows.
 >
 > Since patch fixes both hotspot and JDK parts, I'm sending it to
hs-dev and
 > core-libs-dev.

Don't see anything that actually comes under core-libs-dev. Looks like
one net-dev, one awt-dev and one security-dev. Sorry.


Okay, I will add them.

Cheers,
David
-


Thanks for reviewing,

Thomas

 > Thanks, Thomas
 >

Re: RFR: 8223553: Fix code constructs that do not compile with the Eclipse Java Compiler

[David Holmes]

Hi Christoph,

I'm very reluctant to see changes like this that the compiler folk have 
not determined are actually incorrect. That said ...


On 15/05/2019 7:03 am, Langer, Christoph wrote:

Thanks Daniel.

Can anybody help reviewing the changes to:
src/java.base/share/classes/java/lang/invoke/MethodHandles.java


The introduction of the intermediate local variable seems harmless 
(though why it should be necessary is another matter).



src/java.base/share/classes/java/util/concurrent/ConcurrentSkipListMap.java


As you note this should be ok'd by jsr166 folk so I've cc'd Martin 
Buccholz. I dislike seeing a raw type introduced here though.



and
src/java.management/share/classes/java/lang/management/ManagementFactory.java ?


Introducing an unchecked cast seems very crude. I'd want the core-libs 
stream experts to comment on this.


Cheers,
David



Thanks
Christoph


-Original Message-
From: Daniel Fuchs 
Sent: Dienstag, 14. Mai 2019 18:04
To: Langer, Christoph ; core-libs-dev ; net-dev 
Cc: compiler-...@openjdk.java.net
Subject: Re: RFR: 8223553: Fix code constructs that do not compile with the
Eclipse Java Compiler

Hi Christoph,

That looks much better, thanks!
(but still not commenting on the other changes ;-))

best regards,

-- daniel

On 14/05/2019 13:57, Langer, Christoph wrote:

Hi Daniel,


unfortunately, your proposed solution does not work with javac. I get

this

in the build:

Oh darn. I should have double checked.
Can we at least reduce the scope of the @SuppressedWarnings by
introducing a private method that just has the return call?


Sure, what about this one:

http://cr.openjdk.java.net/~clanger/webrevs/8223553.2/ ?


Thanks
Christoph

Re: RFR: JDK-8224028: loop initial declarations introduced by JDK-8184770 (jdwp)

[David Holmes]

What compiler was used here? We shouldn't be using anything that doesn't 
handle loop variable declarations!


Thanks,
David

On 16/05/2019 7:41 pm, Daniel Fuchs wrote:

Hi Ao Qi,

I'm adding serviceability-dev, since this is for jdwp.

The proposed changes look good to me - but please get
someone from the serviceability team to review this.

best regards,

-- daniel


On 16/05/2019 08:41, Ao Qi wrote:

Hi,

I found build is failed on CentOS 7.6, because of loop initial
declarations. Could I please get reviews for this?

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

Webrev:
http://cr.openjdk.java.net/~aoqi/8224028/webrev.00/

Tested:
linux-x86_64-server-release tier1

Thanks,
Ao Qi

Re: RFR: JDK-8224028: loop initial declarations introduced by JDK-8184770 (jdwp)

[David Holmes]

On 17/05/2019 12:26 am, Jean Christophe Beyler wrote:
 From my experience, some compiler in Solaris/Windows complain about 
this (or used to a year ago via the submit repo); Serguei and I had to 
do this dance when we were getting the heap monitoring tests in. An 


I think the tests are different. This file is part of the main build and 
is being built the same way as all the other .c files in the core 
libraries. The Windows and Solaris sources already contain code with 
loop variable declarations. Yet much to my surprise the shared sources 
do not - seems gcc is the weak link here. :(


alternative is to move the file to a C++ file. Adding an extern "C" at 
the top would make symbols not be mangled and it should "work"  without 
having to go to old-C requirements.


Not sure if that's feasible.

Cheers,
David
-


Thanks,
Jc

On Thu, May 16, 2019 at 5:31 AM David Holmes <mailto:david.hol...@oracle.com>> wrote:


What compiler was used here? We shouldn't be using anything that
doesn't
handle loop variable declarations!

Thanks,
David

On 16/05/2019 7:41 pm, Daniel Fuchs wrote:
 > Hi Ao Qi,
 >
 > I'm adding serviceability-dev, since this is for jdwp.
 >
 > The proposed changes look good to me - but please get
 > someone from the serviceability team to review this.
 >
 > best regards,
 >
 > -- daniel
 >
 >
 > On 16/05/2019 08:41, Ao Qi wrote:
 >> Hi,
 >>
 >> I found build is failed on CentOS 7.6, because of loop initial
 >> declarations. Could I please get reviews for this?
 >>
 >> Bug:
 >> https://bugs.openjdk.java.net/browse/JDK-8224028
 >>
 >> Webrev:
 >> http://cr.openjdk.java.net/~aoqi/8224028/webrev.00/
 >>
 >> Tested:
 >> linux-x86_64-server-release tier1
 >>
 >> Thanks,
 >> Ao Qi
 >>
 >



--

Thanks,
Jc

Re: RFR: JDK-8224028: loop initial declarations introduced by JDK-8184770 (jdwp)

[David Holmes]

On 16/05/2019 11:41 pm, Ao Qi wrote:

Hi Serguei,

I saw your email [1], but I didn't receive it yet. Thanks for your
review! I updated:

http://cr.openjdk.java.net/~aoqi/8224028/webrev.01/

On Thu, May 16, 2019 at 8:30 PM David Holmes  wrote:


What compiler was used here? We shouldn't be using anything that doesn't
handle loop variable declarations!


The compiler I used is gcc 4.8.5. This machine is used for testing
jdk/jdk for months. As far as I remember, loop variable declarations
issue never been found. If gcc 4.8.5 is not a supported compiler, I
think we should update building doc [2].


I'm surprised the out-of-the-box settings for 4.8.5 result in such an 
archaic version of C being supported. I thought we had addressed such 
limitations quite a while ago. :(


That said perhaps it is time to bump the minimum gcc level beyond 4.8 ...

Thanks,
David
-



Thanks,
David

On 16/05/2019 7:41 pm, Daniel Fuchs wrote:

Hi Ao Qi,

I'm adding serviceability-dev, since this is for jdwp.

The proposed changes look good to me - but please get
someone from the serviceability team to review this.


Thanks Daniel!

Cheers,
Ao Qi

[1] 
https://mail.openjdk.java.net/pipermail/serviceability-dev/2019-May/028097.html
[2] 
https://hg.openjdk.java.net/jdk/jdk/raw-file/17926213de55/doc/building.html#gcc




best regards,

-- daniel


On 16/05/2019 08:41, Ao Qi wrote:

Hi,

I found build is failed on CentOS 7.6, because of loop initial
declarations. Could I please get reviews for this?

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

Webrev:
http://cr.openjdk.java.net/~aoqi/8224028/webrev.00/

Tested:
linux-x86_64-server-release tier1

Thanks,
Ao Qi

Re: RFR: JDK-8224028: loop initial declarations introduced by JDK-8184770 (jdwp)

[David Holmes]

On 17/05/2019 8:57 am, Martin Buchholz wrote:

Maybe you just need to ask gcc to use a more modern -std=...
It might reasonably be defaulting to gnu89
https://stackoverflow.com/questions/14737104/what-is-the-default-c-mode-for-the-current-gcc-especially-on-ubuntu


Yes, but I thought we'd already done this dance. Solaris was setting a 
flag to use C89 IIRC and we removed it.


Cheers,
David

On Thu, May 16, 2019 at 3:25 PM David Holmes <mailto:david.hol...@oracle.com>> wrote:


On 16/05/2019 11:41 pm, Ao Qi wrote:
 > Hi Serguei,
 >
 > I saw your email [1], but I didn't receive it yet. Thanks for your
 > review! I updated:
 >
 > http://cr.openjdk.java.net/~aoqi/8224028/webrev.01/
 >
 > On Thu, May 16, 2019 at 8:30 PM David Holmes
mailto:david.hol...@oracle.com>> wrote:
 >>
 >> What compiler was used here? We shouldn't be using anything that
doesn't
 >> handle loop variable declarations!
 >
 > The compiler I used is gcc 4.8.5. This machine is used for testing
 > jdk/jdk for months. As far as I remember, loop variable declarations
 > issue never been found. If gcc 4.8.5 is not a supported compiler, I
 > think we should update building doc [2].

I'm surprised the out-of-the-box settings for 4.8.5 result in such an
archaic version of C being supported. I thought we had addressed such
limitations quite a while ago. :(

That said perhaps it is time to bump the minimum gcc level beyond
4.8 ...

Thanks,
David
-

 >>
 >> Thanks,
 >> David
 >>
 >> On 16/05/2019 7:41 pm, Daniel Fuchs wrote:
 >>> Hi Ao Qi,
 >>>
 >>> I'm adding serviceability-dev, since this is for jdwp.
 >>>
 >>> The proposed changes look good to me - but please get
 >>> someone from the serviceability team to review this.
 >
 > Thanks Daniel!
 >
 > Cheers,
 > Ao Qi
 >
 > [1]

https://mail.openjdk.java.net/pipermail/serviceability-dev/2019-May/028097.html
 > [2]

https://hg.openjdk.java.net/jdk/jdk/raw-file/17926213de55/doc/building.html#gcc
 >
 >
 >>>
 >>> best regards,
 >>>
 >>> -- daniel
 >>>
 >>>
 >>> On 16/05/2019 08:41, Ao Qi wrote:
 >>>> Hi,
 >>>>
 >>>> I found build is failed on CentOS 7.6, because of loop initial
 >>>> declarations. Could I please get reviews for this?
 >>>>
 >>>> Bug:
 >>>> https://bugs.openjdk.java.net/browse/JDK-8224028
 >>>>
 >>>> Webrev:
 >>>> http://cr.openjdk.java.net/~aoqi/8224028/webrev.00/
 >>>>
 >>>> Tested:
 >>>> linux-x86_64-server-release tier1
 >>>>
 >>>> Thanks,
 >>>> Ao Qi
 >>>>
 >>>

Re: RFR: JDK-8224028: loop initial declarations introduced by JDK-8184770 (jdwp)

[David Holmes]

On 17/05/2019 9:14 am, Martin Buchholz wrote:
On Thu, May 16, 2019 at 4:05 PM David Holmes <mailto:david.hol...@oracle.com>> wrote:


On 17/05/2019 8:57 am, Martin Buchholz wrote:
 > Maybe you just need to ask gcc to use a more modern -std=...
 > It might reasonably be defaulting to gnu89
 >

https://stackoverflow.com/questions/14737104/what-is-the-default-c-mode-for-the-current-gcc-especially-on-ubuntu

Yes, but I thought we'd already done this dance. Solaris was setting a
flag to use C89 IIRC and we removed it.


A flag to use C89 is obviously bad if you're using features from a later 
standard.
I was suggesting that you could pass gcc -std=gnu99 or -std= c99 (I 
would go whole hog to C11)


Again I thought we had done this dance. We set -std=gnu++98 but that 
only affects .cpp files. We need a similar thing for .c files. I know 
this has been discussed so I'll see if I can dig up the history and find 
out why we didn't do it. I'll file a build bug if needed.


Cheers,
David



  $ gcc -v --help |& grep std=.*' C '
   -std=c11                    Conform to the ISO 2011 C standard
   -std=c89                    Conform to the ISO 1990 C standard
   -std=c90                    Conform to the ISO 1990 C standard
   -std=c99                    Conform to the ISO 1999 C standard
   -std=gnu11                  Conform to the ISO 2011 C standard with GNU
   -std=gnu89                  Conform to the ISO 1990 C standard with GNU
   -std=gnu90                  Conform to the ISO 1990 C standard with GNU
   -std=gnu99                  Conform to the ISO 1999 C standard with GNU
   -std=iso9899:1990           Conform to the ISO 1990 C standard
   -std=iso9899:199409         Conform to the ISO 1990 C standard as 
amended in

   -std=iso9899:1999           Conform to the ISO 1999 C standard
   -std=iso9899:2011           Conform to the ISO 2011 C standard

Re: Need sponsor to fix Javadoc warnings

[David Holmes]

9:24.982172591 +0530
@@ -1,5 +1,5 @@
/*
- * Copyright (c) 2000, 2018, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2000, 2020, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -69,7 +69,7 @@
 /**
  * Initialize the builder with the input parameters.
  *
- * @param params the parameter set used to build a certification path
+ * @param buildParams the parameter set used to build a certification path
  */
 Builder(BuilderParams buildParams) {
 this.buildParams = buildParams;
--- old/src/java.base/share/classes/sun/text/DictionaryBasedBreakIterator.java  
2020-04-06 00:19:26.618178853 +0530
+++ new/src/java.base/share/classes/sun/text/DictionaryBasedBreakIterator.java  
2020-04-06 00:19:26.210177291 +0530
@@ -1,5 +1,5 @@
/*
- * Copyright (c) 1999, 2016, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 1999, 2020, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -111,7 +111,7 @@
  * @param ruleFile   the name of the rule data file
  * @param ruleData   the rule data loaded from the rule data file
  * @param dictionaryFile the name of the dictionary file
- * @param dictionartData the dictionary data loaded from the dictionary 
file
+ * @param dictionaryData the dictionary data loaded from the dictionary 
file
  * @throws MissingResourceException if rule data or dictionary 
initialization failed
  */
 public DictionaryBasedBreakIterator(String ruleFile, byte[] ruleData,



On 6 Apr 2020, at 17:07, Vipin Sharma  wrote:

Hi David,

I forgot to mention this is my second patch here. I am new to this project, as 
per my understanding we need to contribute 3 patches to get user id and space 
on cr.openjdk.java.net, for now I need sponsor who can create bug id and upload 
this webrev on cr.openjdk.java.net
Please suggest if there is any way I can create my user id to upload this patch.

This is ~300 line patch file.

Regards,
Vipin


On Apr 6, 2020, at 3:25 AM, David Holmes  wrote:

Hi Vipin,

On 6/04/2020 6:42 am, Vipin Sharma wrote:

Hi,
I have fixed a few warnings where the method parameter name is different in
code and Javadoc, need a sponsor for this patch.
Webrev is available at
https://drive.google.com/open?id=1EXUXKqGxzSR7sW2LShy0sgvP4z-bPL0e


webrevs needs to be hosted on OpenJDK systems - either cr.openjdk.java.net, or 
inline in an email to the list (not an attachment) if small enough.

Thanks,
David


Regards,
Vipin





Thanks,
Vipin

Re: Need sponsor to fix Javadoc warnings

[David Holmes]

Hi Pavel,

On 8/04/2020 11:23 pm, Pavel Rappo wrote:

Hey David,

Where exactly? In the files affected by this changeset? If so, then we will
introduce inconsistency. Otherwise it's a huge change. From what I can see there
are some 250 occurrences of `@exception` in 
src/java.base/share/classes/com/sun/{crypto, security}
and some 7,300 in src.


Okay as I said I didn't examine in detail to see the size of the change 
- and its obviously too big.



Personally, out of all tag renovations, changing `@exception` to `@throws`
probably gives the least bang for the buck. If nothing else, it gives you 3
extra characters on the same line to fill with something more useful.


There was an effort to do this conversion a while back, at least while 
touching affected files. It's not a big deal to me.


Cheers,
David


I would be more inclined to change `...` to `{@code ...}`, but
given how error-prone that can be, I still wouldn't do it in this changeset.

-Pavel


On 8 Apr 2020, at 13:56, David Holmes  wrote:

Hi Pavel,

Not a review ...

On 8/04/2020 9:50 pm, Pavel Rappo wrote:

Vipin, here you go:
 https://bugs.openjdk.java.net/browse/JDK-8242366
 http://cr.openjdk.java.net/~prappo/8242366/webrev.00/
I took the liberty of additionally fixing a couple of parameters' names,
a typo, and `@exception` tags for checked exceptions that were neither thrown
nor imported.


While you are in there is it worth changing @exception to @throws? (I didn't 
look to see how big that change would be.)

Cheers,
David


The bulk of the change is in Security. Some changes are in Networking. The
appropriate mailing lists are in CC for this email. We should wait for their
feedback.
Changes in core area look good to me and I'd be surprised if there are any
problems with the remaining portion of the changeset.
-Pavel

On 7 Apr 2020, at 19:50, Vipin Sharma  wrote:

Hi Pavel,


On Apr 7, 2020, at 11:11 PM, Pavel Rappo  wrote:

I assume you have signed the OCA [1]. If not and you want to continue, please 
do it. If you've already done so, which is probably the case [2], please attach 
your patch as text to this thread with the next email. Do not use zip or the 
like. I will take it from there and sponsor that for you.

Yes I have signed OCA.


-Pavel

[1] https://www.oracle.com/technetwork/community/oca-486395.html
[2] changeset:   58344:65f30e209890
user:clanger
date:Wed Mar 11 13:50:13 2020 +0100
files:   test/jdk/java/lang/Boolean/GetBoolean.java 
test/jdk/java/lang/Boolean/MakeBooleanComparable.java 
test/jdk/java/lang/Boolean/ParseBoolean.java
description:
8240524: Remove explicit type argument in test 
jdk/java/lang/Boolean/MakeBooleanComparable.java
Reviewed-by: clanger, vtewari
Contributed-by: vipinsharma85 at gmail.com


Yes this is my first contribution.

Patch text:

--- old/src/java.base/share/classes/com/sun/crypto/provider/AESCipher.java  
2020-04-06 00:19:10.546117441 +0530
+++ new/src/java.base/share/classes/com/sun/crypto/provider/AESCipher.java  
2020-04-06 00:19:10.130115855 +0530
@@ -1,5 +1,5 @@
/*
- * Copyright (c) 2002, 2017, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2002, 2020, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -202,7 +202,7 @@
 /**
  * Sets the padding mechanism of this cipher.
  *
- * @param padding the padding mechanism
+ * @param paddingScheme the padding mechanism
  *
  * @exception NoSuchPaddingException if the requested padding mechanism
  * does not exist
--- old/src/java.base/share/classes/com/sun/crypto/provider/AESWrapCipher.java  
2020-04-06 00:19:11.526121179 +0530
+++ new/src/java.base/share/classes/com/sun/crypto/provider/AESWrapCipher.java  
2020-04-06 00:19:11.118119622 +0530
@@ -1,5 +1,5 @@
/*
- * Copyright (c) 2004, 2017, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2004, 2020, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -313,10 +313,10 @@
  * current Cipher.engineInit(...) implementation,
  * IllegalStateException will always be thrown upon invocation.
  *
- * @param in the input buffer
- * @param inOffset the offset in in where the input
+ * @param input the input buffer
+ * @param inputOffset the offset in in where the input
  * starts
- * @param inLen the input length.
+ * @param inputLen the input length.
  *
  * @return n/a.
  *
--- old/src/java.base/share/classes/com/sun/crypto/provider/BlowfishCrypt.java  
2020-04-06 00:19:12.462124749 +0530
+++ new/src/java.base/share/classes/com/sun/crypto/provider/BlowfishCrypt.java  
2020-04-06 00:19:12.054123193 +0530
@@ -1,5 +1,5 @@
/*

Re: RFR(XS): 8245518: Problem list java/net/SocketOption/AfterClose.java

[David Holmes]

Ship it!

Thanks,
David


The test started failing after JDK-8243099.

JBS: https://bugs.openjdk.java.net/browse/JDK-8245518
webrev: 
http://cr.openjdk.java.net/~mikael/webrevs/8245518/webrev.00/open/webrev/

Cheers,
Mikael

Re: RFR: 8252837: Cleanup SAP Copyright file headers

[David Holmes]

Looks good to me - and trivial.

Thanks,
David

On 7/09/2020 2:22 pm, Christoph Langer wrote:

The format for SAP copyrights in OpenJDK headers should be like:
Copyright (c)  SAP SE. All rights reserved.
or
Copyright (c) ,  SAP SE. All rights reserved.

There should not be a comma character (",") before "SAP SE".

This is inconsistent in some files which calls for some cleanup.

Please review :)

-

Commit messages:
  - 8252837: Cleanup SAP Copyright file headers

Changes: https://git.openjdk.java.net/jdk/pull/36/files
  Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=36&range=00
   Issue: https://bugs.openjdk.java.net/browse/JDK-8252837
   Stats: 55 lines in 55 files changed: 0 ins; 0 del; 55 mod
   Patch: https://git.openjdk.java.net/jdk/pull/36.diff
   Fetch: git fetch https://git.openjdk.java.net/jdk pull/36/head:pull/36

PR: https://git.openjdk.java.net/jdk/pull/36

Re: RFR: 8252837: Cleanup SAP Copyright file headers

[David Holmes]

On Mon, 7 Sep 2020 07:28:36 GMT, Christoph Langer  wrote:

>>> _Mailing list message from [David Holmes](mailto:david.hol...@oracle.com) on
>>> [nio-dev](mailto:nio-...@openjdk.java.net):_
>>> Looks good to me - and trivial.
>>> 
>>> Thanks,
>>> David
>>> 
>>> On 7/09/2020 2:22 pm, Christoph Langer wrote:
>> 
>> Thanks, David!
>
>> _Mailing list message from [Lindenmaier, 
>> Goetz](mailto:goetz.lindenma...@sap.com) on
>> [nio-dev](mailto:nio-...@openjdk.java.net):_
>> Hi Christoph,
>> 
>> Looks good to me ??
>> 
>> Best regards,
>> Goetz.
>> 
> Thanks, Goetz :)

Note that Goetz was not officially added as a reviewer. The email was included 
in this PR as a comment, but he either
has to add himself as a reviewer, or the PR author has to based on the email.

-

PR: https://git.openjdk.java.net/jdk/pull/36

Re: RFR: 8252837: Cleanup SAP Copyright file headers

[David Holmes]

On Mon, 7 Sep 2020 04:14:06 GMT, Christoph Langer  wrote:

> The format for SAP copyrights in OpenJDK headers should be like:
> Copyright (c)  SAP SE. All rights reserved.
> or
> Copyright (c) ,  SAP SE. All rights reserved.
> 
> There should not be a comma character (",") before "SAP SE".
> 
> This is inconsistent in some files which calls for some cleanup.
> 
> Please review :)

Marked as reviewed by dholmes (Reviewer).

-

PR: https://git.openjdk.java.net/jdk/pull/36

Re: RFR: 8252999: Cleanup: replace .equals("") with .isEmpty() within all codebase

[David Holmes]

On Thu, 10 Sep 2020 10:40:15 GMT, Alan Bateman  wrote:

>> @kevinrushforth thanks. Done.
>> 
>> Similar issues:
>> https://bugs.java.com/bugdatabase/view_bug.do?bug_id=8215014
>> https://bugs.java.com/bugdatabase/view_bug.do?bug_id=8251246
>> https://bugs.java.com/bugdatabase/view_bug.do?bug_id=8223237
>> 
>> could be joined somehow?
>
> The code in java.base was updated to use String::isEmpty in JDK 12 
> (JDK-8215281). There was follow-up in JDK 13 to do
> the same in the java.desktop module (JDK-8223237). Changing the remaining 
> usages make sense although I see that more
> more than half are in tests.  It would be good to hear from security-dev on 
> the changes to the Apache Santuario code
> (in java.xml.crypto module) in case it would be better to contribute those 
> upstream instead. Ditto for the Apache Xalan
> code (in the java.xml module) but it may be significantly forked already that 
> it doesn't matter.  I assume you can use
> JDK-8215014 rather than creating a new issue.

This should be broken up to deal with the files in different functional areas 
under different bugids and PRs. Otherwise
the cross-posting to so many lists is prohibitive. Files in different areas 
need to be reviewed by different teams.
Thank you.

-

PR: https://git.openjdk.java.net/jdk/pull/29

Re: RFR: 8252999: Cleanup: replace .equals("") with .isEmpty() within all codebase

[David Holmes]

On Thu, 10 Sep 2020 12:18:51 GMT, Kevin Rushforth  wrote:

>> This should be broken up to deal with the files in different functional 
>> areas under different bugids and PRs. Otherwise
>> the cross-posting to so many lists is prohibitive. Files in different areas 
>> need to be reviewed by different teams.
>> Thank you.
>
> @dholmes-ora raises a good point. Hopefully there is a balance point between 
> a dozen different bugs / pull requests
> each targeting one area and one bug / PR targeting a dozen separate areas. I 
> don't have a good general rule to suggest.
> Maybe @AlanBateman or @jddarcy can offer some advice?

14 cc'd mailing lists is unworkable. You need to be subscribed to lists to 
post, but even if you are a reply-all will
be delayed due to all of the mails being held up for moderator approval due to:

" Too many recipients to the message"

I have a longer email coming once it gets through the moderator approval delay. 
:(

-

PR: https://git.openjdk.java.net/jdk/pull/29

Re: RFR: 8267184: JEP 411: Add -Djava.security.manager=allow to tests calling System.setSecurityManager

[David Holmes]

On Mon, 17 May 2021 17:51:36 GMT, Weijun Wang  wrote:

> Please review the test changes for [JEP 
> 411](https://openjdk.java.net/jeps/411).
> 
> With JEP 411 and the default value of `-Djava.security.manager` becoming 
> `disallow`, tests calling `System.setSecurityManager()`  need 
> `-Djava.security.manager=allow` when launched. This PR covers such changes 
> for tier1 to tier3 (except for the JCK tests).
> 
> To make it easier to focus your review on the tests in your area, this PR is 
> divided into multiple commits for different areas. Mostly the rule is the 
> same as how Skara adds labels, but there are several small tweaks:
> 
> 1. When a file is covered by multiple labels, only one is chosen. I make my 
> best to avoid putting too many tests into `core-libs`. If a file is covered 
> by `core-libs` and another label, I categorized it into the other label.
> 2. If a file is in `core-libs` but contains `/xml/` in its name, it's in the 
> `xml` commit.
> 3. If a file is in `core-libs` but contains `/rmi/` in its name, it's in the 
> `rmi` commit.
> 4. One file not covered by any label -- 
> `test/jdk/com/sun/java/accessibility/util/8051626/Bug8051626.java` -- is in 
> the `swing` commit.
> 
> Due to the size of this PR, no attempt is made to update copyright years for 
> all files to minimize unnecessary merge conflict.
> 
> Please note that this PR can be integrated before the source changes for JEP 
> 411, as the possible values of this system property was already defined long 
> time ago in JDK 9.
> 
> Most of the change in this PR is a simple adding of 
> `-Djava.security.manager=allow` to the `@run main/othervm` line. Sometimes it 
> was not `othervm` and we add one. Sometimes there's no `@run` at all and we 
> add the line.
> 
> There are several tests that launch another Java process that needs to call 
> the `System.setSecurityManager()` method, and the system property is added to 
> `ProcessBuilder`, `ProcessTools`, or the java command line (if the test is a 
> shell test).
> 
> 3 langtools tests are added into problem list due to 
> [JDK-8265611](https://bugs.openjdk.java.net/browse/JDK-8265611).
> 
> 2 SQL tests are moved because they need different options on the `@run` line 
> but they are inside a directory that has a `TEST.properties`:
> 
> rename test/jdk/java/sql/{testng/test/sql/othervm => 
> permissionTests}/DriverManagerPermissionsTests.java (93%)
> rename test/jdk/javax/sql/{testng/test/rowset/spi => 
> permissionTests}/SyncFactoryPermissionsTests.java (95%)
>  ```
> 
> The source change for JEP 411 is at https://github.com/openjdk/jdk/pull/4073.

Changes to hotspot-* and serviceability look good.

Thanks,
David

-

Marked as reviewed by dholmes (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/4071

Re: RFR: 8268349: Provide more detail in JEP 411 warning messages

[David Holmes]

On Mon, 7 Jun 2021 20:42:53 GMT, Weijun Wang  wrote:

> More loudly and precise warning messages when a security manager is either 
> enabled at startup or installed at runtime.

There are a number of hotspot tests that will trigger this warning, so please 
ensure they work correctly with the extra output.

You might want to make your "WARNING" consistent with the VM's "Warning" so 
that OutputAnalyzer's logic to ignore warnings will automatically ignore these 
too.

Thanks,
David

-

PR: https://git.openjdk.java.net/jdk/pull/4400

Re: RFR: 8276348: Use blessed modifier order in java.base

[David Holmes]

On 4/11/2021 12:14 am, Pavel Rappo wrote:

On Tue, 2 Nov 2021 16:30:56 GMT, Pavel Rappo  wrote:


This PR follows up one of the recent PRs, where I used a non-canonical modifier 
order. Since the problem was noticed [^1], why not to address it en masse?

As far as I remember, the first mass-canonicalization of modifiers took place 
in JDK-8136583 in 2015 [^2]. That change affected 1780 lines spanning 453 
files. Since then modifiers have become a bit loose, and it makes sense to 
re-bless (using the JDK-8136583 terminology) them.

This change was produced by running the below command followed by updating the 
copyright years on the affected files where necessary:

 $ sh ./bin/blessed-modifier-order.sh src/java.base

The resulting change is much smaller than that of 2015: 39 lines spanning 21 
files.

[^1]: 
https://mail.openjdk.java.net/pipermail/core-libs-dev/2021-November/082987.html 
(or https://github.com/openjdk/jdk/pull/6191#pullrequestreview-79465)
[^2]: 
http://mail.openjdk.java.net/pipermail/core-libs-dev/2015-September/035217.html



_Mailing list message from [David Holmes](mailto:david.hol...@oracle.com) on 
[core-libs-dev](mailto:core-libs-...@mail.openjdk.java.net):_

On 3/11/2021 9:26 pm, Pavel Rappo wrote:


On Tue, 2 Nov 2021 20:34:44 GMT, Martin Buchholz  wrote:

Pragmatically, fix the script to ignore those keywords on comment lines. Learn 
Perl, its just a regular expression pattern match and replace expression.



I understand in principle how to modify that script to ignore doc comments. The thing I 
was referring to when said "btw, how would we do that?" was this: not all 
comment lines are prose. Some of those lines belong to snippets of code, which I guess 
you would also like to be properly formatted.

But having seen several reviewers be unmoved by the difference, the real 
pragmatic view is to ignore the English.



I'm sorry you feel that way. Would it be okay if I made it clear that those two 
words are not English adjectives but are special symbols that happen to use 
Latin script and originate from the English words they resemble? If so, I could 
enclose each of them in `{@code ... }`. If not, I could drop that particular 
change from this PR.



The blessed-modifier-order.sh script intentionally modifies comments, with the 
hope of finding code snippets (it did!)
Probably I manually deleted the change to Object.java back in 2015, to avoid 
the sort of controversy we're seeing now.
I don't have a strong feeling either way on changing that file.
I agree with @pavelrappo  that script-generated changes should not be mixed 
with manual changes.
I would also not update copyright years for such changes.
It's a feature of blessed-modifier-order.sh that all existing formatting is 
perfectly preserved.



One more thing. Please have a look at this other line in the same file; this 
line was there before the change 
https://github.com/openjdk/jdk/blob/465d350d0b3cac277a58b9f8ece196c1cde68e80/src/java.base/share/classes/java/lang/Object.java#L49
So before the change, the file was somewhat inconsistent. The change made it 
consistent. **If one is going to ever revert that controversial part of the 
change, please update both lines so that the file remains consistent.**


Line 281 is (was!) consistent with line 277 because it is distinguishing a synchronized "static method" from 
a synchronized "instance method". There was no need to make any change to line 281 because of the 
blessed-order of modifiers as defined for method declarations! This text is just prose. Now for consistency you should 
change line 277 to refer to a "non-static synchronized method" (as "instance synchronized method" 
would be truly awful).


Thanks, David. You've provided a clear and convincing argument, and I can see 
the inconsistency I introduced. I can revert that particular piece back if you 
think that it would be appropriate.

That said, this line will have to be filtered out every time the script is run. 
I could probably provide a more involved script that harnesses the power of AST 
(com.sun.source.doctree) to try to filter out prose, but it would be impossible 
to beat the simplicity of the current script; and simplicity is also important.


Given this is prose, the adjectives should be separated by commas: "a 
synchronized, static method", and "a synchronized, instance method". 
Does that avoid the problem with the script?



Line 49 places "static synchronized" in code font, implying that it is referring to the actual method 
modifiers and as such using the blessed order is appropriate. Line 49 does not need to be "consistent" with 
line 281. If line 49 used a normal font so the words "static" and "synchronized" were just prose 
then either order would be perfectly fine in terms of English language, but then you could make a case for having it be 
consiste

Re: [jdk18] RFR: JDK-8273179: Update nroff pages in JDK 18 before RC

[David Holmes]

On Fri, 10 Dec 2021 01:46:03 GMT, Jonathan Gibbons  wrote:

> Please review this semi-automatic update for the nroff man pages for JDK 18.  
> The changes update the version number, copyright year, and incorporate the 
> changes from the latest upstream files.

Hi Jon,

This all looks good - I'm familiar with many of the changes made to the sources 
but not yet generated.

We will have to be careful not to regress the copyright year or version if any 
subsequent updates are made before next year, or before the version loses the 
EA designator.

Thanks,
David

-

Marked as reviewed by dholmes (Reviewer).

PR: https://git.openjdk.java.net/jdk18/pull/5

Re: [jdk18] RFR: JDK-8273179: Update nroff pages in JDK 18 before RC

[David Holmes]

On Fri, 10 Dec 2021 02:48:43 GMT, Jonathan Gibbons  wrote:

>> Please review this semi-automatic update for the nroff man pages for JDK 18. 
>>  The changes update the version number, copyright year, and incorporate the 
>> changes from the latest upstream files.
>
> We will also want to regenerate any appropriate files if any more updates to 
> the man pages are made during the ramp down period.

Hi @jonathan-gibbons ,

I have a couple of manpage related tasks for early in the 19 release cycle that 
will take care of updating the version number.

Unfortunately for this change it looks like whomever did the source uodate to 
the javadoc manpage did not know about the test:

test/langtools/jdk/javadoc/tool/CheckManPageOptions.java

which is now failing on all platforms. :( I will file a bug.

David

-

PR: https://git.openjdk.java.net/jdk18/pull/5

Re: RFR: 8282657: Code cleanup: removing double semicolons at the end of lines

[David Holmes]

On Fri, 28 Jan 2022 14:39:31 GMT, Matteo Baccan  wrote:

> Hi
> 
> I have reviewed the code for removing double semicolons at the end of lines
> 
> all the best
> matteo

I eyeballed the diff file and all seems okay.

Thanks,
David

-

Marked as reviewed by dholmes (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/7268

Re: RFR: 8285366: Fix typos in serviceability

[David Holmes]

On Thu, 21 Apr 2022 18:08:05 GMT, Kevin Walls  wrote:

>> But on the other hand we have `javax.script.Invocable`. :-) 
>> 
>> Codespell suggested this change, and I based my decision to keep it based on 
>> [Merriam-Webster](https://www.merriam-webster.com/dictionary/invocable) not 
>> even listing "invokable" as an alternate spelling, and that "invocable" has 
>> about 3x the number of Google hits than "invokable". 
>> 
>> But sure, there is perhaps reason to consider "invokable" an acceptable 
>> alternative and keep it. I guess it depends on if you consider the word to 
>> be based on "invoke" with a suffix, or a latinized form, like "invocation". 
>> 
>> I'll wait a while for others to chime in, otherwise I'll revert the 
>> "invokable" changes.
>
> Sure, I just thought there was enough evidence that invokable is not 
> definitely wrong, even if invocable is more correct and popular, so it's not 
> obvious it should be changed.  Don't lose sleep over it. 8-)
> 
> More importantly, on the tests -- I saw the changes in exception messages, 
> searched for the wrong text in the test directories, and didn't find any 
> matches that looked like checks.

Invocable, Invokable and Invokeable are all used in practice. We have a mix of 
invocable and invokable throughout our codebase, with more of the former than 
the latter - and in prose "invocable" is probably best.

-

PR: https://git.openjdk.java.net/jdk/pull/8334

Request for review: 7022386 gethostbyname_r needs a pointer aligned buffer passed to it

[David Holmes]

webrev: http://cr.openjdk.java.net/~dholmes/7022386/webrev/

This is a very simple forward-port of a change that already went into 6u23.

Please cc me as I'm not on the net-dev list (which probably means this 
will get held up for approval :( )


Thanks,
David

hg: jdk7/tl/jdk: 7022386: gethostbyname_r needs a pointer aligned buffer passed to it

[david . holmes]

Changeset: a3b6c262195b
Author:dholmes
Date:  2011-02-28 06:40 -0500
URL:   http://hg.openjdk.java.net/jdk7/tl/jdk/rev/a3b6c262195b

7022386: gethostbyname_r needs a pointer aligned buffer passed to it
Summary: Change buffer type to ensure pointer-alignment
Reviewed-by: alanb, chegar

! src/solaris/native/java/net/Inet4AddressImpl.c

hg: jdk7/tl/jdk: 7022370: Launcher ergonomics should provide simple default implementation

[david . holmes]

Changeset: 38626f41e458
Author:dholmes
Date:  2011-03-09 19:52 -0500
URL:   http://hg.openjdk.java.net/jdk7/tl/jdk/rev/38626f41e458

7022370: Launcher ergonomics should provide simple default implementation
Summary: Use a common, platform independent, implementation unless an 
architecture specific implementation exists
Reviewed-by: ksrini, mchung, ohair, gbenson

! make/java/jli/Makefile
! src/solaris/bin/ergo.c
- src/solaris/bin/ergo_sparc.c
- src/solaris/bin/ergo_zero.c

hg: jdk7/tl/jdk: 56 new changesets

[david . holmes]

Changeset: eb54e565c491
Author:ohair
Date:  2011-02-26 09:45 -0800
URL:   http://hg.openjdk.java.net/jdk7/tl/jdk/rev/eb54e565c491

7016175: HTML generated from new JavaDoc has tags added from makefile
Reviewed-by: jjg

! make/common/shared/Defs-javadoc.gmk
! make/docs/Makefile

Changeset: 391a9ef69036
Author:ohair
Date:  2011-02-26 10:12 -0800
URL:   http://hg.openjdk.java.net/jdk7/tl/jdk/rev/391a9ef69036

Merge


Changeset: e88c8381eaca
Author:ohair
Date:  2011-02-26 12:11 -0800
URL:   http://hg.openjdk.java.net/jdk7/tl/jdk/rev/e88c8381eaca

7022237: Fix use of \" in the new "release" file at the top of the install, 
windows issues
Reviewed-by: ohrstrom

! make/common/Release.gmk

Changeset: 123dd69407f9
Author:ohair
Date:  2011-02-26 12:42 -0800
URL:   http://hg.openjdk.java.net/jdk7/tl/jdk/rev/123dd69407f9

Merge


Changeset: ed1d4691da29
Author:ohrstrom
Date:  2011-02-28 10:56 +0100
URL:   http://hg.openjdk.java.net/jdk7/tl/jdk/rev/ed1d4691da29

7021753: Add a build times report
Summary: Report the build times at end of a jdkroot build.
Reviewed-by: ohair

! make/common/shared/Defs-utils.gmk

Changeset: f32f0ae3d873
Author:ohair
Date:  2011-03-02 12:09 -0800
URL:   http://hg.openjdk.java.net/jdk7/tl/jdk/rev/f32f0ae3d873

Merge


Changeset: 869cba583dd4
Author:ohair
Date:  2011-03-02 13:18 -0800
URL:   http://hg.openjdk.java.net/jdk7/tl/jdk/rev/869cba583dd4

7020815: REBASE should not be required for windows jdk repo builds - can't 
build with VS 2010 Express
Reviewed-by: prr

! make/common/shared/Defs-utils.gmk
! make/common/shared/Sanity.gmk

Changeset: e5cd10425e7e
Author:ohair
Date:  2011-03-03 07:02 -0800
URL:   http://hg.openjdk.java.net/jdk7/tl/jdk/rev/e5cd10425e7e

Merge

- src/share/classes/java/dyn/NoAccessException.java
- src/share/classes/java/dyn/Switcher.java
- test/java/lang/Thread/StopBeforeStart.java

Changeset: c588355b5bb7
Author:ohair
Date:  2011-03-03 15:30 -0800
URL:   http://hg.openjdk.java.net/jdk7/tl/jdk/rev/c588355b5bb7

Merge


Changeset: 7931291bc5d3
Author:herrick
Date:  2011-03-01 17:09 -0500
URL:   http://hg.openjdk.java.net/jdk7/tl/jdk/rev/7931291bc5d3

7021567: need to create jnlp javadoc for 64 bit bundles.
Summary: need to create jnlp javadoc for 64 bit bundles.
Reviewed-by: igor, ohair

! make/common/Release.gmk

Changeset: c53711f82bfb
Author:igor
Date:  2011-03-08 14:26 -0800
URL:   http://hg.openjdk.java.net/jdk7/tl/jdk/rev/c53711f82bfb

Merge

- test/java/lang/Thread/StopBeforeStart.java

Changeset: 5e5f68a01d12
Author:ohair
Date:  2011-03-08 16:05 -0800
URL:   http://hg.openjdk.java.net/jdk7/tl/jdk/rev/5e5f68a01d12

Merge

! make/common/Release.gmk

Changeset: 6aeed99af874
Author:mchung
Date:  2011-03-09 23:11 -0800
URL:   http://hg.openjdk.java.net/jdk7/tl/jdk/rev/6aeed99af874

7025631: Remove the modules build support from jdk 7
Reviewed-by: alanb, ohair

! make/Makefile
! make/com/sun/crypto/provider/Makefile
! make/com/sun/java/pack/Makefile
! make/com/sun/java/pack/prop/Makefile
! make/com/sun/jndi/cosnaming/Makefile
! make/com/sun/jndi/dns/Makefile
! make/com/sun/jndi/ldap/Makefile
! make/com/sun/jndi/rmi/registry/Makefile
! make/com/sun/nio/sctp/Makefile
! make/com/sun/org/apache/xml/Makefile
! make/com/sun/rowset/Makefile
! make/com/sun/script/Makefile
! make/com/sun/security/auth/module/Makefile
! make/com/sun/servicetag/Makefile
! make/com/sun/tools/attach/Makefile
! make/common/Defs.gmk
! make/common/Demo.gmk
! make/common/Library.gmk
! make/common/Program.gmk
! make/common/Release.gmk
! make/common/Sanity.gmk
! make/common/Subdirs.gmk
! make/common/shared/Sanity.gmk
! make/java/awt/Makefile
! make/java/fdlibm/Makefile
! make/java/instrument/Makefile
! make/java/java/Makefile
! make/java/java_crw_demo/Makefile
! make/java/java_hprof_demo/Makefile
! make/java/jli/Makefile
! make/java/jvm/Makefile
! make/java/logging/Makefile
! make/java/main/java/Makefile
! make/java/main/javaw/Makefile
! make/java/management/Makefile
! make/java/net/Makefile
! make/java/nio/Makefile
! make/java/npt/Makefile
! make/java/redist/Makefile
! make/java/redist/fonts/Makefile
! make/java/redist/sajdi/Makefile
! make/java/security/Makefile
! make/java/sql/Makefile
! make/java/text/base/Makefile
! make/java/verify/Makefile
! make/java/zip/Makefile
! make/javax/crypto/Makefile
! make/javax/imageio/Makefile
! make/javax/print/Makefile
! make/javax/sound/Makefile
! make/javax/sound/jsoundalsa/Makefile
! make/javax/sound/jsoundds/Makefile
! make/javax/sql/Makefile
! make/javax/swing/Makefile
! make/javax/swing/plaf/Makefile
! make/jpda/back/Makefile
! make/jpda/transport/Makefile
! make/jpda/transport/shmem/Makefile
! make/jpda/transport/socket/Makefile
! make/jpda/tty/Makefile
! make/launchers/Makefile
! make/mkdemo/jvmti/Makefile
! make/mkdemo/management/Makefile
! make/mksample/dtrace/Makefile
! make/mksample/jmx/jmx-scandir/M

Re: hg: jdk7/tl/jdk: 56 new changesets

[David Holmes]

Just a heads up that this changeset includes a pre-integration with 
changes from jdk7/build/jdk. Those changes needed to be merged with my 
changset so that the integrators wouldn't have to handle a larger merge 
down the track. The changes would have been pulled down later this week.


David

david.hol...@oracle.com said the following on 03/16/11 19:33:

Changeset: eb54e565c491
Author:ohair
Date:  2011-02-26 09:45 -0800
URL:   http://hg.openjdk.java.net/jdk7/tl/jdk/rev/eb54e565c491

7016175: HTML generated from new JavaDoc has tags added from makefile
Reviewed-by: jjg

! make/common/shared/Defs-javadoc.gmk
! make/docs/Makefile

Changeset: 391a9ef69036
Author:ohair
Date:  2011-02-26 10:12 -0800
URL:   http://hg.openjdk.java.net/jdk7/tl/jdk/rev/391a9ef69036

Merge


Changeset: e88c8381eaca
Author:ohair
Date:  2011-02-26 12:11 -0800
URL:   http://hg.openjdk.java.net/jdk7/tl/jdk/rev/e88c8381eaca

7022237: Fix use of \" in the new "release" file at the top of the install, 
windows issues
Reviewed-by: ohrstrom

! make/common/Release.gmk

Changeset: 123dd69407f9
Author:ohair
Date:  2011-02-26 12:42 -0800
URL:   http://hg.openjdk.java.net/jdk7/tl/jdk/rev/123dd69407f9

Merge


Changeset: ed1d4691da29
Author:ohrstrom
Date:  2011-02-28 10:56 +0100
URL:   http://hg.openjdk.java.net/jdk7/tl/jdk/rev/ed1d4691da29

7021753: Add a build times report
Summary: Report the build times at end of a jdkroot build.
Reviewed-by: ohair

! make/common/shared/Defs-utils.gmk

Changeset: f32f0ae3d873
Author:ohair
Date:  2011-03-02 12:09 -0800
URL:   http://hg.openjdk.java.net/jdk7/tl/jdk/rev/f32f0ae3d873

Merge


Changeset: 869cba583dd4
Author:ohair
Date:  2011-03-02 13:18 -0800
URL:   http://hg.openjdk.java.net/jdk7/tl/jdk/rev/869cba583dd4

7020815: REBASE should not be required for windows jdk repo builds - can't 
build with VS 2010 Express
Reviewed-by: prr

! make/common/shared/Defs-utils.gmk
! make/common/shared/Sanity.gmk

Changeset: e5cd10425e7e
Author:ohair
Date:  2011-03-03 07:02 -0800
URL:   http://hg.openjdk.java.net/jdk7/tl/jdk/rev/e5cd10425e7e

Merge

- src/share/classes/java/dyn/NoAccessException.java
- src/share/classes/java/dyn/Switcher.java
- test/java/lang/Thread/StopBeforeStart.java

Changeset: c588355b5bb7
Author:ohair
Date:  2011-03-03 15:30 -0800
URL:   http://hg.openjdk.java.net/jdk7/tl/jdk/rev/c588355b5bb7

Merge


Changeset: 7931291bc5d3
Author:herrick
Date:  2011-03-01 17:09 -0500
URL:   http://hg.openjdk.java.net/jdk7/tl/jdk/rev/7931291bc5d3

7021567: need to create jnlp javadoc for 64 bit bundles.
Summary: need to create jnlp javadoc for 64 bit bundles.
Reviewed-by: igor, ohair

! make/common/Release.gmk

Changeset: c53711f82bfb
Author:igor
Date:  2011-03-08 14:26 -0800
URL:   http://hg.openjdk.java.net/jdk7/tl/jdk/rev/c53711f82bfb

Merge

- test/java/lang/Thread/StopBeforeStart.java

Changeset: 5e5f68a01d12
Author:ohair
Date:  2011-03-08 16:05 -0800
URL:   http://hg.openjdk.java.net/jdk7/tl/jdk/rev/5e5f68a01d12

Merge

! make/common/Release.gmk

Changeset: 6aeed99af874
Author:mchung
Date:  2011-03-09 23:11 -0800
URL:   http://hg.openjdk.java.net/jdk7/tl/jdk/rev/6aeed99af874

7025631: Remove the modules build support from jdk 7
Reviewed-by: alanb, ohair

! make/Makefile
! make/com/sun/crypto/provider/Makefile
! make/com/sun/java/pack/Makefile
! make/com/sun/java/pack/prop/Makefile
! make/com/sun/jndi/cosnaming/Makefile
! make/com/sun/jndi/dns/Makefile
! make/com/sun/jndi/ldap/Makefile
! make/com/sun/jndi/rmi/registry/Makefile
! make/com/sun/nio/sctp/Makefile
! make/com/sun/org/apache/xml/Makefile
! make/com/sun/rowset/Makefile
! make/com/sun/script/Makefile
! make/com/sun/security/auth/module/Makefile
! make/com/sun/servicetag/Makefile
! make/com/sun/tools/attach/Makefile
! make/common/Defs.gmk
! make/common/Demo.gmk
! make/common/Library.gmk
! make/common/Program.gmk
! make/common/Release.gmk
! make/common/Sanity.gmk
! make/common/Subdirs.gmk
! make/common/shared/Sanity.gmk
! make/java/awt/Makefile
! make/java/fdlibm/Makefile
! make/java/instrument/Makefile
! make/java/java/Makefile
! make/java/java_crw_demo/Makefile
! make/java/java_hprof_demo/Makefile
! make/java/jli/Makefile
! make/java/jvm/Makefile
! make/java/logging/Makefile
! make/java/main/java/Makefile
! make/java/main/javaw/Makefile
! make/java/management/Makefile
! make/java/net/Makefile
! make/java/nio/Makefile
! make/java/npt/Makefile
! make/java/redist/Makefile
! make/java/redist/fonts/Makefile
! make/java/redist/sajdi/Makefile
! make/java/security/Makefile
! make/java/sql/Makefile
! make/java/text/base/Makefile
! make/java/verify/Makefile
! make/java/zip/Makefile
! make/javax/crypto/Makefile
! make/javax/imageio/Makefile
! make/javax/print/Makefile
! make/javax/sound/Makefile
! make/javax/sound/jsoundalsa/Makefile
! make/javax/sound/jsoundds/Makefile
! make/javax/sql/Makefile
! make/javax/

hg: jdk7/tl/jdk: 7027910: Add basic cross-compilation support and add ARM/PPC to the known architectures in the open code

[david . holmes]

Changeset: 54d8193f177b
Author:dholmes
Date:  2011-03-16 18:54 -0400
URL:   http://hg.openjdk.java.net/jdk7/tl/jdk/rev/54d8193f177b

7027910: Add basic cross-compilation support and add ARM/PPC to the known 
architectures in the open code
Summary: Cross-compilation support
Reviewed-by: ohair, andrew

! make/common/Defs-linux.gmk
! make/common/Defs.gmk
! make/common/Program.gmk
! make/common/shared/Defs-linux.gmk
! make/common/shared/Defs-solaris.gmk
! make/common/shared/Defs-utils.gmk
! make/common/shared/Defs-versions.gmk
! make/common/shared/Platform.gmk
! make/common/shared/Sanity-Settings.gmk
! make/common/shared/Sanity.gmk
! make/java/instrument/Makefile
! make/java/nio/Makefile
! make/javax/sound/SoundDefs.gmk
! make/sun/jdbc/Makefile
! make/tools/Makefile
! src/share/native/com/sun/media/sound/SoundDefs.h
! src/share/native/java/lang/fdlibm/include/fdlibm.h

hg: jdk7/tl/jdk: 7030063: AWT support for SE-Embedded integration

[david . holmes]

Changeset: a2793622a8d8
Author:dholmes
Date:  2011-03-25 07:09 -0400
URL:   http://hg.openjdk.java.net/jdk7/tl/jdk/rev/a2793622a8d8

7030063: AWT support for SE-Embedded integration
Summary: AWT support for SE-Embedded
Reviewed-by: anthony, art, bobv, collins, alanb

! make/launchers/Makefile
! make/sun/Makefile
! make/sun/awt/mawt.gmk
! make/sun/jawt/Makefile
! make/sun/jpeg/Makefile
! make/sun/security/tools/Makefile
! make/sun/xawt/Makefile
! src/share/classes/java/awt/Toolkit.java
+ src/share/classes/sun/awt/HToolkit.java
! src/solaris/classes/sun/awt/X11/XToolkit.java
! src/solaris/classes/sun/awt/X11/XTrayIconPeer.java
! src/solaris/native/java/lang/java_props_md.c
! src/solaris/native/sun/awt/jawt.c
! src/solaris/native/sun/xawt/XToolkit.c

hg: jdk7/tl/jdk: 7031929: Variable names typos in Release-embedded.gmk

[david . holmes]

Changeset: 19567f9d6962
Author:dholmes
Date:  2011-03-29 08:15 -0400
URL:   http://hg.openjdk.java.net/jdk7/tl/jdk/rev/19567f9d6962

7031929: Variable names typos in Release-embedded.gmk
Reviewed-by: alanb

! make/common/Release-embedded.gmk

hg: jdk7/tl/jdk: 7032364: Add jvm.cfg file for ARM and PPC architectures

[david . holmes]

Changeset: 5107fb3a9c06
Author:dholmes
Date:  2011-03-30 22:20 -0400
URL:   http://hg.openjdk.java.net/jdk7/tl/jdk/rev/5107fb3a9c06

7032364: Add jvm.cfg file for ARM and PPC architectures
Reviewed-by: darcy, bdelsart, alanb, mduigou

+ src/solaris/bin/arm/jvm.cfg
+ src/solaris/bin/ppc/jvm.cfg

hg: jdk7/tl/jdk: 7025066: Build systems changes to support SE Embedded Integration

[david . holmes]

Changeset: fe232d7e4ff1
Author:dholmes
Date:  2011-03-22 18:56 -0400
URL:   http://hg.openjdk.java.net/jdk7/tl/jdk/rev/fe232d7e4ff1

7025066: Build systems changes to support SE Embedded Integration
Summary: Define Embedded specific files and include them in the main files. 
Allow finer control over some build options.
Reviewed-by: ohair, bobv, collins

+ make/common/Defs-embedded.gmk
! make/common/Defs.gmk
! make/common/Library.gmk
+ make/common/Release-embedded.gmk
! make/common/Release.gmk
! make/common/shared/Sanity-Settings.gmk
! make/java/zip/Makefile
! make/sun/nio/cs/Makefile
! src/share/classes/sun/misc/Version.java.template

hg: jdk8/tl/jdk: 7039182: PPC: NIO: java.io.IOException: Invalid argument in sun.nio.ch.FileDispatcherImpl.read0

[david . holmes]

Changeset: 3abc52f0a4dc
Author:dholmes
Date:  2011-06-27 20:13 -0400
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/3abc52f0a4dc

7039182: PPC: NIO: java.io.IOException: Invalid argument in 
sun.nio.ch.FileDispatcherImpl.read0
Summary: Allow platform specific files to be located at build time instead of 
generating them
Reviewed-by: alanb, ohair

! make/common/Defs-embedded.gmk
! make/java/nio/Makefile

hg: jdk8/tl/jdk: 7012206: ~20 tools tests failing due to -XX:-UsePerfData default in Java SE Embedded

[david . holmes]

Changeset: d177eecda07e
Author:dholmes
Date:  2011-09-20 22:20 -0400
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/d177eecda07e

7012206: ~20 tools tests failing due to -XX:-UsePerfData default in Java SE 
Embedded
Summary: Explicitly enable UsePerfData for the tools that require it to be 
enabled
Reviewed-by: alanb, ohair

! test/sun/jvmstat/perfdata/PrologSanity/PrologSizeSanityCheck.java
! test/sun/tools/common/ApplicationSetup.sh
! test/sun/tools/jinfo/Basic.sh
! test/sun/tools/jmap/Basic.sh
! test/sun/tools/jps/jps-Defaults.sh
! test/sun/tools/jps/jps-V_2.sh
! test/sun/tools/jps/jps-Vm_2.sh
! test/sun/tools/jps/jps-Vvm.sh
! test/sun/tools/jps/jps-Vvml.sh
! test/sun/tools/jps/jps-Vvml_2.sh
! test/sun/tools/jps/jps-help.sh
! test/sun/tools/jps/jps-l_1.sh
! test/sun/tools/jps/jps-l_2.sh
! test/sun/tools/jps/jps-lm.sh
! test/sun/tools/jps/jps-m.sh
! test/sun/tools/jps/jps-m_2.sh
! test/sun/tools/jps/jps-q.sh
! test/sun/tools/jps/jps-v_1.sh
! test/sun/tools/jps/jps-vm_1.sh
! test/sun/tools/jstack/Basic.sh
! test/sun/tools/jstat/jstatClassOutput1.sh
! test/sun/tools/jstat/jstatClassloadOutput1.sh
! test/sun/tools/jstat/jstatCompilerOutput1.sh
! test/sun/tools/jstat/jstatFileURITest1.sh
! test/sun/tools/jstat/jstatGcCapacityOutput1.sh
! test/sun/tools/jstat/jstatGcCauseOutput1.sh
! test/sun/tools/jstat/jstatGcNewCapacityOutput1.sh
! test/sun/tools/jstat/jstatGcNewOutput1.sh
! test/sun/tools/jstat/jstatGcOldCapacityOutput1.sh
! test/sun/tools/jstat/jstatGcOldOutput1.sh
! test/sun/tools/jstat/jstatGcOutput1.sh
! test/sun/tools/jstat/jstatGcPermCapacityOutput1.sh
! test/sun/tools/jstat/jstatHelp.sh
! test/sun/tools/jstat/jstatLineCounts1.sh
! test/sun/tools/jstat/jstatLineCounts2.sh
! test/sun/tools/jstat/jstatLineCounts3.sh
! test/sun/tools/jstat/jstatLineCounts4.sh
! test/sun/tools/jstat/jstatOptions1.sh
! test/sun/tools/jstat/jstatPrintCompilationOutput1.sh
! test/sun/tools/jstat/jstatSnap1.sh
! test/sun/tools/jstat/jstatSnap2.sh
! test/sun/tools/jstat/jstatTimeStamp1.sh
! test/sun/tools/jstatd/jstatdDefaults.sh
! test/sun/tools/jstatd/jstatdExternalRegistry.sh
! test/sun/tools/jstatd/jstatdPort.sh
! test/sun/tools/jstatd/jstatdServerName.sh

hg: jdk8/tl/jdk: 7109092: Wrong computation results with double at armsflt

[david . holmes]

Changeset: cf47846165f4
Author:dholmes
Date:  2011-11-29 00:26 -0500
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/cf47846165f4

7109092: Wrong computation results with double at armsflt
Summary: need to link to custom soft-float library with required FP accuracy
Reviewed-by: alanb, ohair

! make/common/Defs-embedded.gmk

hg: jdk8/tl/jdk: 7092140: Test: java/util/concurrent/locks/Lock/TimedAcquireLeak.java fails on SE-E due to -XX:-UsePerfData

[david . holmes]

Changeset: d4a6627d5004
Author:dholmes
Date:  2012-03-08 00:46 -0500
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/d4a6627d5004

7092140: Test: java/util/concurrent/locks/Lock/TimedAcquireLeak.java fails on 
SE-E due to -XX:-UsePerfData
Summary: Add -XX:+UsePerfData to invocation of exec'd JVM
Reviewed-by: alanb, chegar

! test/java/util/concurrent/locks/Lock/TimedAcquireLeak.java

hg: jdk8/tl/jdk: 7103570: AtomicIntegerFieldUpdater does not work when SecurityManager is installed

[david . holmes]

Changeset: 48513d156965
Author:dholmes
Date:  2012-05-08 02:59 -0400
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/48513d156965

7103570: AtomicIntegerFieldUpdater does not work when SecurityManager is 
installed
Summary: Perform class.getField inside a doPrivileged block
Reviewed-by: chegar, psandoz

! src/share/classes/java/util/concurrent/atomic/AtomicIntegerFieldUpdater.java
! src/share/classes/java/util/concurrent/atomic/AtomicLongFieldUpdater.java
! src/share/classes/java/util/concurrent/atomic/AtomicReferenceFieldUpdater.java
+ test/java/util/concurrent/atomic/AtomicUpdaters.java

hg: jdk8/tl/jdk: 7178483: Change version string for Embedded releases

[david . holmes]

Changeset: dfe5617c18b4
Author:dholmes
Date:  2012-06-20 22:40 -0400
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/dfe5617c18b4

7178483: Change version string for Embedded releases
Reviewed-by: dholmes, lancea
Contributed-by: Gary Collins 

! make/common/Defs-embedded.gmk

hg: jdk8/tl/jdk: 7161229: PriorityBlockingQueue keeps hard reference to last removed element

[david . holmes]

Changeset: 94b525ce3653
Author:dholmes
Date:  2012-06-27 01:36 -0400
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/94b525ce3653

7161229: PriorityBlockingQueue keeps hard reference to last removed element
Reviewed-by: dholmes, forax, alanb
Contributed-by: Doug Lea 

! src/share/classes/java/util/concurrent/PriorityBlockingQueue.java
! test/java/util/concurrent/BlockingQueue/LastElement.java

Re: Crash due to missing synchronization on 'gconf_client' in 'jdk/src/solaris/native/sun/net/spi/DefaultProxySelector.c'

[David Holmes]

Hi Christian,

Probably best to discuss this on the net-dev@openjdk.java.net list (cc'd).

Two comments from me (I'm not on net-dev):

1. CHECK_NULL does a return so you will be leaving the monitor locked if 
you encounter any nulls.


2. Is it simpler to add synchronization at the Java level? (I don't know 
how this code is used)


David Holmes

On 2/08/2012 8:54 PM, Christian Schulte wrote:

Hi,

using the system property 'java.net.useSystemProxies', JDK 7 crashes on
OpenBSD 5.2.

$ /usr/local/jre-1.7.0/bin/java -version
openjdk version "1.7.0_03"
OpenJDK Runtime Environment (build 1.7.0_03-b04)
OpenJDK Server VM (build 22.1-b02, mixed mode)

$ /usr/local/jre-1.7.0/bin/java -cp . Crash
2538: assertion failed "allocator->lock_loc == NULL" file
"/usr/ports/pobj/dbus-1.6.2/dbus-1.6.2/dbus/dbus-dataslot.c" line 79
function _dbus_data_slot_allocator_alloc
2538: assertion failed "allocator->lock_loc == NULL" file
"/usr/ports/pobj/dbus-1.6.2/dbus-1.6.2/dbus/dbus-dataslot.c" line 79
function _dbus_data_slot_allocator_alloc
2538: assertion failed "allocator->lock_loc == NULL" file
"/usr/ports/pobj/dbus-1.6.2/dbus-1.6.2/dbus/dbus-dataslot.c" line 79
function _dbus_data_slot_allocator_alloc
2538: assertion failed "allocator->lock_loc == NULL" file
"/usr/ports/pobj/dbus-1.6.2/dbus-1.6.2/dbus/dbus-dataslot.c" line 79
function _dbus_data_slot_allocator_alloc
D-Bus not compiled with backtrace support so unable to print a backtrace
D-Bus not compiled with backtrace support so unable to print a backtrace

$ /usr/local/jre-1.7.0/bin/java -cp . Crash
27421: assertion failed "!(connection)->have_connection_lock" file
"/usr/ports/pobj/dbus-1.6.2/dbus-1.6.2/dbus/dbus-connection.c" line 1133
function _dbus_connection_acquire_io_path
D-Bus not compiled with backtrace support so unable to print a backtrace
Abort trap (core dumped)

Looking at
'openjdk/jdk/src/solaris/native/sun/net/spi/DefaultProxySelector.c',
there is a 'static void* gconf_client' which is initialized by calling
'gconf_client_get_default' from 'libgconf-2'. Uses of that client are
not protected against concurrent accesses by multiple threads although
that gconf client is not thread-safe. Trying to add some protection
myself resulted in the attached patch. Rebuilding JDK 1.7 with this
patch applied, the 'gconf'/'dbus' related crashes no longer happen.

Regards,

hg: jdk8/tl/jdk: 7198815: Add the minimal VM as "known" in jvm.cfg

[david . holmes]

Changeset: 9b3867244eec
Author:dholmes
Date:  2012-11-01 18:09 -0400
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/9b3867244eec

7198815: Add the minimal VM as "known" in jvm.cfg
Reviewed-by: alanb, forax, mchung

! src/solaris/bin/arm/jvm.cfg
! src/solaris/bin/i586/jvm.cfg
! src/solaris/bin/ppc/jvm.cfg
! src/solaris/bin/sparc/jvm.cfg

hg: jdk8/tl/jdk: 7197210: java/lang/invoke/CallSiteTest.java failing on armsflt.

[david . holmes]

Changeset: cb65e3315b27
Author:jiangli
Date:  2012-11-05 12:51 -0500
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/cb65e3315b27

7197210: java/lang/invoke/CallSiteTest.java failing on armsflt.
Summary: Reduce work load and set longer timeout for java/lang/invoke tests.
Reviewed-by: kvn, twisti

! test/java/lang/invoke/BigArityTest.java
! test/java/lang/invoke/CallSiteTest.java
! test/java/lang/invoke/MethodHandlesTest.java
! test/java/lang/invoke/RicochetTest.java

hg: jdk8/tl/jdk: 7200297: agent code does not handle multiple boot library path elements correctly

[david . holmes]

Changeset: 60550cd2b527
Author:dholmes
Date:  2012-12-02 19:16 -0500
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/60550cd2b527

7200297: agent code does not handle multiple boot library path elements 
correctly
Summary: When bug 6819213 was fixed it enabled sun.boot.library.path property 
to contain multiple paths. Code in agents does not handle multiple paths when 
attempting to find dependent shared libs.
Reviewed-by: dholmes, sspitsyn, dsamersoff
Contributed-by: Bill Pittore 

! src/share/back/debugInit.c
! src/share/back/error_messages.c
! src/share/back/transport.c
! src/share/demo/jvmti/hprof/hprof.h
! src/share/demo/jvmti/hprof/hprof_init.c
! src/solaris/back/linker_md.c
! src/solaris/demo/jvmti/hprof/hprof_md.c
! src/solaris/npt/npt_md.h
! src/windows/back/linker_md.c
! src/windows/demo/jvmti/hprof/hprof_md.c
! src/windows/npt/npt_md.h

hg: jdk8/tl/jdk: 8003632: HPROF class file version java.lang.RuntimeException errors

[david . holmes]

Changeset: 8d7323a9d8ed
Author:dholmes
Date:  2012-12-13 21:18 -0500
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/8d7323a9d8ed

8003632: HPROF class file version java.lang.RuntimeException errors
Reviewed-by: mchung, lancea

! src/share/javavm/export/classfile_constants.h

Re: RFR JDK-8005120

[David Holmes]

Real sense of deja-vu here. Didn't we go through this same thing with 
the HPI socket routines?


Depending on the OS (and version?) we should be using socklen_t not int 
and not uint32_t.


David

On 20/12/2012 2:35 AM, Chris Hegarty wrote:

John,

I grabbed your patch, and with it I now see different warnings.

../../../../src/share/transport/socket/socketTransport.c: In function
'socketTransport_startListening':
../../../../src/share/transport/socket/socketTransport.c:310:40:
warning: pointer targets in passing argument 3 of 'dbgsysGetSocketName'
differ in signedness [-Wpointer-sign]
../../../../src/share/transport/socket/sysSocket.h:58:5: note: expected
'uint32_t *' but argument is of type 'int *'
../../../../src/share/transport/socket/socketTransport.c: In function
'socketTransport_accept':
../../../../src/share/transport/socket/socketTransport.c:371:33:
warning: pointer targets in passing argument 3 of 'dbgsysAccept' differ
in signedness [-Wpointer-sign]
../../../../src/share/transport/socket/sysSocket.h:41:5: note: expected
'uint32_t *' but argument is of type 'int *'

Do you see these in your build?

-Chris.

On 12/19/2012 03:42 PM, Alan Bateman wrote:


John - this is the debugger socket transport so cc'ing the
serviceability-dev list as that is where this code is maintained.

On 19/12/2012 15:36, John Zavgren wrote:

Greetings:
Please consider the following change to the two files:
src/share/transport/socket/sysSocket.h
src/solaris/transport/socket/socket_md.c
that eliminate compiler warnings that stem from the fact that the
variables that the native code passes to various system calls were not
declared correctly. They were declared as integers, but they must be
"unsigned" integers because they are used to define buffer lengths.
Were one to supply a negative value as an argument, it would be cast
into an unsigned "Martian" value and there'd be (hopefully) a system
call error.

Thanks!
John Zavgren

http://cr.openjdk.java.net/~mullan/webrevs/jzavgren/8005120/

hg: jdk8/tl/jdk: 8005232: (JEP-149) Class Instance size reduction

[david . holmes]

Changeset: 1109bfff4e92
Author:dholmes
Date:  2013-01-13 19:57 -0500
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/1109bfff4e92

8005232: (JEP-149) Class Instance size reduction
Summary: Moved the fields for cached reflection objects into a seperate 
ReflectionData object to reduce dynamic footprint.
Reviewed-by: dholmes, mchung, shade
Contributed-by: Peter Levart 

! src/share/classes/java/lang/Class.java

hg: jdk8/tl: 8009529: Fix for 8006988 missed closed configure changes

[david . holmes]

Changeset: 929e2461818b
Author:dholmes
Date:  2013-03-05 22:45 -0500
URL:   http://hg.openjdk.java.net/jdk8/tl/rev/929e2461818b

8009529: Fix for 8006988 missed closed configure changes
Reviewed-by: mr

! common/autoconf/generated-configure.sh

hg: jdk8/tl: 8009428: Revert changes to $ substitution performed as part of nashorn integration

[david . holmes]

Changeset: 19a59a13b3ef
Author:dholmes
Date:  2013-03-14 01:41 -0400
URL:   http://hg.openjdk.java.net/jdk8/tl/rev/19a59a13b3ef

8009428: Revert changes to $ substitution performed as part of nashorn 
integration
Reviewed-by: alanb, erikj

! common/makefiles/MakeBase.gmk

hg: jdk8/tl/jdk: 8009429: Miscellaneous profiles cleanup; ...

[david . holmes]

Changeset: 41289b4a1819
Author:dholmes
Date:  2013-03-14 01:47 -0400
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/41289b4a1819

8009429: Miscellaneous profiles cleanup
8009428: Revert changes to $ substitution performed as part of nashorn 
integration
Reviewed-by: alanb, erikj

! makefiles/CreateJars.gmk
! makefiles/ProfileNames.gmk
! makefiles/Profiles.gmk
! makefiles/profile-includes.txt
! makefiles/profile-rtjar-includes.txt

hg: jdk8/tl/langtools: 8009429: Miscellaneous profiles cleanup

[david . holmes]

Changeset: 82dc1e827c2a
Author:dholmes
Date:  2013-03-14 01:45 -0400
URL:   http://hg.openjdk.java.net/jdk8/tl/langtools/rev/82dc1e827c2a

8009429: Miscellaneous profiles cleanup
Reviewed-by: jjg, alanb

! src/share/classes/com/sun/tools/javac/sym/Profiles.java

tl forest update

[David Holmes]

Sorry for the wide distribution but you all see the push messages anyway.

I've just pushed a coordinated set of changes to the top-level, 
langtools and jdk repos in the tl forest. If you use tl you will need to 
ensure that you update all of these repos to ensure they are in sync.


Thanks,
David

hg: jdk8/tl/jdk: 8009426: "profiles" target fails due to nashorn if "images" is not built first

[david . holmes]

Changeset: e766da5575fa
Author:dholmes
Date:  2013-03-19 06:01 -0400
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/e766da5575fa

8009426: "profiles" target fails due to nashorn if "images" is not built first
Reviewed-by: alanb

! makefiles/CreateJars.gmk
! makefiles/Profiles.gmk
! makefiles/profile-includes.txt

hg: jdk8/tl/jdk: 8006818: SunEC and SunPKCS11 providers should be in all profiles

[david . holmes]

Changeset: 3c1a4966d901
Author:dholmes
Date:  2013-03-20 22:39 -0400
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/3c1a4966d901

8006818: SunEC and SunPKCS11 providers should be in all profiles
Reviewed-by: dholmes, alanb, valeriep
Contributed-by: Jen Dority 

! makefiles/profile-includes.txt

hg: jdk8/tl: 8011152: Precision problems on sflt builds

[david . holmes]

Changeset: 10775618db00
Author:aharlap
Date:  2013-04-26 15:54 -0400
URL:   http://hg.openjdk.java.net/jdk8/tl/rev/10775618db00

8011152: Precision problems on sflt builds
Summary: Need to add global flag to the linker
Reviewed-by: tbell, dholmes

! common/makefiles/NativeCompilation.gmk

hg: jdk8/tl/jdk: 8010280: jvm.cfg needs updating for non-server builds

[david . holmes]

Changeset: 138f767b8eff
Author:dholmes
Date:  2013-04-29 07:40 -0400
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/138f767b8eff

8010280: jvm.cfg needs updating for non-server builds
Summary: Generate jvm.cfg based on chosen VMs for non-"standard" builds and 
remove legacy entries from committed jvm.cfg files
Reviewed-by: mduigou, tbell

! makefiles/CopyFiles.gmk
! src/macosx/bin/x86_64/jvm.cfg
! src/solaris/bin/amd64/jvm.cfg
! src/solaris/bin/arm/jvm.cfg
! src/solaris/bin/i586/jvm.cfg
! src/solaris/bin/ia64/jvm.cfg
! src/solaris/bin/ppc/jvm.cfg
! src/solaris/bin/sparc/jvm.cfg
! src/solaris/bin/sparcv9/jvm.cfg
! src/solaris/bin/zero/jvm.cfg
! src/windows/bin/amd64/jvm.cfg
! src/windows/bin/i586/jvm.cfg
! src/windows/bin/ia64/jvm.cfg

hg: jdk8/tl/jdk: 8013395: StringBuffer.toString performance regression impacting embedded benchmarks

[david . holmes]

Changeset: a3d79a4c2a24
Author:dholmes
Date:  2013-05-15 00:36 -0400
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/a3d79a4c2a24

8013395: StringBuffer.toString performance regression impacting embedded 
benchmarks
Summary: cache a copy of the char[] to use with toString() and clear it when 
ever the sb content is modified
Reviewed-by: alanb, plevart, mduigou, forax

! src/share/classes/java/lang/StringBuffer.java
+ test/java/lang/StringBuffer/ToStringCache.java

hg: jdk8/tl/jdk: 8014477: (str) Race condition in String.contentEquals when comparing with StringBuffer

[david . holmes]

Changeset: 08ebdb2b53cc
Author:plevart
Date:  2013-05-17 14:41 +0200
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/08ebdb2b53cc

8014477: (str) Race condition in String.contentEquals when comparing with 
StringBuffer
Reviewed-by: alanb, mduigou, dholmes

! src/share/classes/java/lang/String.java
+ test/java/lang/String/StringContentEqualsBug.java

hg: jdk8/tl/jdk: 8014814: (str) StringBuffer "null" is not appended

[david . holmes]

Changeset: a1a8e71e130a
Author:dholmes
Date:  2013-05-22 20:21 -0400
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/a1a8e71e130a

8014814: (str) StringBuffer "null" is not appended
Reviewed-by: alanb

! src/share/classes/java/lang/StringBuffer.java
! test/java/lang/StringBuffer/ToStringCache.java

hg: jdk8/tl/jdk: 7038914: VM could throw uncaught OOME in ReferenceHandler thread

[david . holmes]

Changeset: 0b8dab7fec54
Author:plevart
Date:  2013-05-27 09:41 +0200
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/0b8dab7fec54

7038914: VM could throw uncaught OOME in ReferenceHandler thread
Summary: Catch OutOfMemoryError in reference handler thread if caused by 
allocation of an InterruptedException
Reviewed-by: dholmes, alanb

! src/share/classes/java/lang/ref/Reference.java
+ test/java/lang/ref/OOMEInReferenceHandler.java

hg: jdk8/tl/jdk: 8015470: Remove redundant calls of toString() on String objects

[david . holmes]

Changeset: 571e5f452640
Author:dholmes
Date:  2013-06-06 05:32 -0400
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/571e5f452640

8015470: Remove redundant calls of toString() on String objects
Reviewed-by: dholmes, alanb
Contributed-by: Otavio Goncalves 

! src/share/classes/com/sun/jndi/toolkit/dir/SearchFilter.java
! src/share/classes/java/lang/annotation/IncompleteAnnotationException.java
! src/share/classes/sun/rmi/rmic/Main.java
! src/share/classes/sun/tools/java/MemberDefinition.java
! src/share/classes/sun/tools/jconsole/inspector/Utils.java

hg: jdk8/tl/jdk: 8016341: java/lang/ref/OOMEInReferenceHandler.java failing intermittently

[david . holmes]

Changeset: 7bb17aa9a09f
Author:dholmes
Date:  2013-07-09 22:01 -0400
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/7bb17aa9a09f

8016341: java/lang/ref/OOMEInReferenceHandler.java failing intermittently
Summary: Ensure WeakRef object can't be prematurely gc'd
Reviewed-by: chegar, plevart

! test/java/lang/ref/OOMEInReferenceHandler.java

Re: RFR [8022584] Memory leak in some NetworkInterface methods

[David Holmes]

On 8/08/2013 11:20 AM, Ivan Gerasimov wrote:

Thanks Alan!

I've checked that and it turns out that GetStringUTFChars cannot return
NULL.
For allocation of the result string it calls AllocateHeap() with the
default EXIT_OOM fail strategy.
Thus, in case of being unable to allocate memory it simply stops VM.


Ouch! That is a hotspot bug. GetStringUTFChars is supposed to throw 
OutOfMemoryError if it has memory issues, not abort the VM!


I recommend that you check for NULL and/or a pending exception.

David


Sincerely yours,
Ivan

On 08.08.2013 5:05, Alan Bateman wrote:

(cc'ing net-dev).

The change looks okay to me. One suggestion (while you are there) is
to check the return from  GetStringUTFChars so that the name returns
when it fails with NULL.

-Alan.

On 07/08/2013 17:46, Ivan Gerasimov wrote:

Hello everybody!

Some methods of NetworkInterface class were reported to leak memory.
http://bugs.sun.com/view_bug.do?bug_id=JDK-8022584 (not yet available.)

Examples are isUp() and isLoopback().

Affected versions of jdk are 6, 7 and 8

Would you please review a simple fix that removes the unnecessary
allocation?
http://cr.openjdk.java.net/~igerasim/8022584/0/webrev/

Sincerely yours,
Ivan

Re: RFR [8022584] Memory leak in some NetworkInterface methods

[David Holmes]

Ivan,

On 8/08/2013 2:05 PM, Ivan Gerasimov wrote:

David, Alan,

I added checking for NULL  results and throwing OOMException if necessary.


You don't need to throw it yourself:

  JNU_ThrowOutOfMemoryError(env, NULL);

Assuming a correct VM implementation if NULL is returned then an OOME 
should already be pending and will be thrown as soon as your native code 
returns to Java.


Thanks,
David


I've also added some spaces along the code to improve indentation.

Would you please review the updated webrev?
http://washi.ru.oracle.com/~igerasim/webrevs/8022584/1/webrev/

Sincerely yours,
Ivan


On 08.08.2013 5:35, David Holmes wrote:

On 8/08/2013 11:20 AM, Ivan Gerasimov wrote:

Thanks Alan!

I've checked that and it turns out that GetStringUTFChars cannot return
NULL.
For allocation of the result string it calls AllocateHeap() with the
default EXIT_OOM fail strategy.
Thus, in case of being unable to allocate memory it simply stops VM.


Ouch! That is a hotspot bug. GetStringUTFChars is supposed to throw
OutOfMemoryError if it has memory issues, not abort the VM!

I recommend that you check for NULL and/or a pending exception.

David


Sincerely yours,
Ivan

On 08.08.2013 5:05, Alan Bateman wrote:

(cc'ing net-dev).

The change looks okay to me. One suggestion (while you are there) is
to check the return from  GetStringUTFChars so that the name returns
when it fails with NULL.

-Alan.

On 07/08/2013 17:46, Ivan Gerasimov wrote:

Hello everybody!

Some methods of NetworkInterface class were reported to leak memory.
http://bugs.sun.com/view_bug.do?bug_id=JDK-8022584 (not yet
available.)

Examples are isUp() and isLoopback().

Affected versions of jdk are 6, 7 and 8

Would you please review a simple fix that removes the unnecessary
allocation?
http://cr.openjdk.java.net/~igerasim/8022584/0/webrev/

Sincerely yours,
Ivan

Re: RFR [8022584] Memory leak in some NetworkInterface methods

[David Holmes]

Main fix looks good to me.

Regression test may need some tweaking eg I think othervm will be needed.

Also this:

 System.out.println("WARNING: Cannot perform memory leak detection on 
this OS");


should probably just say something like "Test skipped on this OS" - 
there are other tests that do this so just check if there is common 
terminology. (In the future we may have keyword tags to flag this as 
linux only etc.)


Thanks,
David

On 9/08/2013 4:41 AM, Ivan Gerasimov wrote:

Chris, if it's not too late, I'd like to include a regtest in the fix.

Here's webrev that includes the test:
http://cr.openjdk.java.net/~igerasim/8022584/3/webrev/

The test gets past with the fixed jdk8 and fails with jdk8-b101 and jdk7
as expected.

Sincerely yours,
Ivan

On 08.08.2013 17:50, Chris Hegarty wrote:

Looks good to me. Thanks Ivan.

-Chris.

On 08/08/2013 02:33 PM, Ivan Gerasimov wrote:

Hello Chris!

Here's the update:
http://cr.openjdk.java.net/~igerasim/8022584/2/webrev/

Thanks for offering the sponsorship!
Here's the hg-export
http://cr.openjdk.java.net/~igerasim/2commit/8022584-jdk8-Memleak-in-NetworkInterface.patch



Sincerely yours,
Ivan

On 08.08.2013 12:43, Chris Hegarty wrote:

Thanks for taking this Ivan.

Can you please make the changes suggested by both David and Alan (
simply return NULL/-1/JNI_FALSE, as appropriate, if GetStringUTFChars
fails ( returns NULL ), then I will sponsor this change into jdk8 for
you.

Please post an update webrev to cr.openjdk.java.net.

-Chris.

On 08/08/2013 06:01 AM, David Holmes wrote:

Ivan,

On 8/08/2013 2:05 PM, Ivan Gerasimov wrote:

David, Alan,

I added checking for NULL  results and throwing OOMException if
necessary.


You don't need to throw it yourself:

   JNU_ThrowOutOfMemoryError(env, NULL);

Assuming a correct VM implementation if NULL is returned then an OOME
should already be pending and will be thrown as soon as your native
code
returns to Java.

Thanks,
David


I've also added some spaces along the code to improve indentation.

Would you please review the updated webrev?
http://washi.ru.oracle.com/~igerasim/webrevs/8022584/1/webrev/

Sincerely yours,
Ivan


On 08.08.2013 5:35, David Holmes wrote:

On 8/08/2013 11:20 AM, Ivan Gerasimov wrote:

Thanks Alan!

I've checked that and it turns out that GetStringUTFChars cannot
return
NULL.
For allocation of the result string it calls AllocateHeap() with
the
default EXIT_OOM fail strategy.
Thus, in case of being unable to allocate memory it simply stops
VM.


Ouch! That is a hotspot bug. GetStringUTFChars is supposed to throw
OutOfMemoryError if it has memory issues, not abort the VM!

I recommend that you check for NULL and/or a pending exception.

David


Sincerely yours,
Ivan

On 08.08.2013 5:05, Alan Bateman wrote:

(cc'ing net-dev).

The change looks okay to me. One suggestion (while you are
there) is
to check the return from  GetStringUTFChars so that the name
returns
when it fails with NULL.

-Alan.

On 07/08/2013 17:46, Ivan Gerasimov wrote:

Hello everybody!

Some methods of NetworkInterface class were reported to leak
memory.
http://bugs.sun.com/view_bug.do?bug_id=JDK-8022584 (not yet
available.)

Examples are isUp() and isLoopback().

Affected versions of jdk are 6, 7 and 8

Would you please review a simple fix that removes the unnecessary
allocation?
http://cr.openjdk.java.net/~igerasim/8022584/0/webrev/

Sincerely yours,
Ivan

Re: RFR [8022584] Memory leak in some NetworkInterface methods

[David Holmes]

Thumbs up!

Thanks,
David

On 9/08/2013 8:19 AM, Ivan Gerasimov wrote:

Thanks David!

On 09.08.2013 1:15, David Holmes wrote:

Main fix looks good to me.

Regression test may need some tweaking eg I think othervm will be needed.


Yes, it's a good point.
Since there may be a memory leak in the test, it'd better not interfere
with other tests in jtreg.


Also this:

 System.out.println("WARNING: Cannot perform memory leak detection on
this OS");

should probably just say something like "Test skipped on this OS" -
there are other tests that do this so just check if there is common
terminology. (In the future we may have keyword tags to flag this as
linux only etc.)


OK, I changed the phrase to "Test only runs on Linux".

Updated webrev is here:
http://cr.openjdk.java.net/~igerasim/8022584/4/webrev/

Updated export is at the same place:
http://cr.openjdk.java.net/~igerasim/2commit/8022584-jdk8-Memleak-in-NetworkInterface.patch


Sincerely yours,
Ivan



Thanks,
David

On 9/08/2013 4:41 AM, Ivan Gerasimov wrote:

Chris, if it's not too late, I'd like to include a regtest in the fix.

Here's webrev that includes the test:
http://cr.openjdk.java.net/~igerasim/8022584/3/webrev/

The test gets past with the fixed jdk8 and fails with jdk8-b101 and jdk7
as expected.

Sincerely yours,
Ivan

On 08.08.2013 17:50, Chris Hegarty wrote:

Looks good to me. Thanks Ivan.

-Chris.

On 08/08/2013 02:33 PM, Ivan Gerasimov wrote:

Hello Chris!

Here's the update:
http://cr.openjdk.java.net/~igerasim/8022584/2/webrev/

Thanks for offering the sponsorship!
Here's the hg-export
http://cr.openjdk.java.net/~igerasim/2commit/8022584-jdk8-Memleak-in-NetworkInterface.patch




Sincerely yours,
Ivan

On 08.08.2013 12:43, Chris Hegarty wrote:

Thanks for taking this Ivan.

Can you please make the changes suggested by both David and Alan (
simply return NULL/-1/JNI_FALSE, as appropriate, if GetStringUTFChars
fails ( returns NULL ), then I will sponsor this change into jdk8 for
you.

Please post an update webrev to cr.openjdk.java.net.

-Chris.

On 08/08/2013 06:01 AM, David Holmes wrote:

Ivan,

On 8/08/2013 2:05 PM, Ivan Gerasimov wrote:

David, Alan,

I added checking for NULL  results and throwing OOMException if
necessary.


You don't need to throw it yourself:

   JNU_ThrowOutOfMemoryError(env, NULL);

Assuming a correct VM implementation if NULL is returned then an
OOME
should already be pending and will be thrown as soon as your native
code
returns to Java.

Thanks,
David


I've also added some spaces along the code to improve indentation.

Would you please review the updated webrev?
http://washi.ru.oracle.com/~igerasim/webrevs/8022584/1/webrev/

Sincerely yours,
Ivan


On 08.08.2013 5:35, David Holmes wrote:

On 8/08/2013 11:20 AM, Ivan Gerasimov wrote:

Thanks Alan!

I've checked that and it turns out that GetStringUTFChars cannot
return
NULL.
For allocation of the result string it calls AllocateHeap() with
the
default EXIT_OOM fail strategy.
Thus, in case of being unable to allocate memory it simply stops
VM.


Ouch! That is a hotspot bug. GetStringUTFChars is supposed to
throw
OutOfMemoryError if it has memory issues, not abort the VM!

I recommend that you check for NULL and/or a pending exception.

David


Sincerely yours,
Ivan

On 08.08.2013 5:05, Alan Bateman wrote:

(cc'ing net-dev).

The change looks okay to me. One suggestion (while you are
there) is
to check the return from  GetStringUTFChars so that the name
returns
when it fails with NULL.

-Alan.

On 07/08/2013 17:46, Ivan Gerasimov wrote:

Hello everybody!

Some methods of NetworkInterface class were reported to leak
memory.
http://bugs.sun.com/view_bug.do?bug_id=JDK-8022584 (not yet
available.)

Examples are isUp() and isLoopback().

Affected versions of jdk are 6, 7 and 8

Would you please review a simple fix that removes the
unnecessary
allocation?
http://cr.openjdk.java.net/~igerasim/8022584/0/webrev/

Sincerely yours,
Ivan

Re: RFR [8022584] Memory leak in some NetworkInterface methods

[David Holmes]

Sorry I messed this up. The JNI book says GetStringUTFChars will return 
NULL and post OOME but the JNI spec (latest version 6.0) does not - it 
only says it will return NULL on failure.


So your previous version was the more correct. Given we just failed to 
allocate C-heap I think we are on thin ice anyway, but better to at 
least attempt to do the right thing.


FYI I filed 8022683 to fix GetStringUTFChars.

David
-

On 9/08/2013 3:21 PM, David Holmes wrote:

Thumbs up!

Thanks,
David

On 9/08/2013 8:19 AM, Ivan Gerasimov wrote:

Thanks David!

On 09.08.2013 1:15, David Holmes wrote:

Main fix looks good to me.

Regression test may need some tweaking eg I think othervm will be
needed.


Yes, it's a good point.
Since there may be a memory leak in the test, it'd better not interfere
with other tests in jtreg.


Also this:

 System.out.println("WARNING: Cannot perform memory leak detection on
this OS");

should probably just say something like "Test skipped on this OS" -
there are other tests that do this so just check if there is common
terminology. (In the future we may have keyword tags to flag this as
linux only etc.)


OK, I changed the phrase to "Test only runs on Linux".

Updated webrev is here:
http://cr.openjdk.java.net/~igerasim/8022584/4/webrev/

Updated export is at the same place:
http://cr.openjdk.java.net/~igerasim/2commit/8022584-jdk8-Memleak-in-NetworkInterface.patch



Sincerely yours,
Ivan



Thanks,
David

On 9/08/2013 4:41 AM, Ivan Gerasimov wrote:

Chris, if it's not too late, I'd like to include a regtest in the fix.

Here's webrev that includes the test:
http://cr.openjdk.java.net/~igerasim/8022584/3/webrev/

The test gets past with the fixed jdk8 and fails with jdk8-b101 and
jdk7
as expected.

Sincerely yours,
Ivan

On 08.08.2013 17:50, Chris Hegarty wrote:

Looks good to me. Thanks Ivan.

-Chris.

On 08/08/2013 02:33 PM, Ivan Gerasimov wrote:

Hello Chris!

Here's the update:
http://cr.openjdk.java.net/~igerasim/8022584/2/webrev/

Thanks for offering the sponsorship!
Here's the hg-export
http://cr.openjdk.java.net/~igerasim/2commit/8022584-jdk8-Memleak-in-NetworkInterface.patch





Sincerely yours,
Ivan

On 08.08.2013 12:43, Chris Hegarty wrote:

Thanks for taking this Ivan.

Can you please make the changes suggested by both David and Alan (
simply return NULL/-1/JNI_FALSE, as appropriate, if
GetStringUTFChars
fails ( returns NULL ), then I will sponsor this change into jdk8
for
you.

Please post an update webrev to cr.openjdk.java.net.

-Chris.

On 08/08/2013 06:01 AM, David Holmes wrote:

Ivan,

On 8/08/2013 2:05 PM, Ivan Gerasimov wrote:

David, Alan,

I added checking for NULL  results and throwing OOMException if
necessary.


You don't need to throw it yourself:

   JNU_ThrowOutOfMemoryError(env, NULL);

Assuming a correct VM implementation if NULL is returned then an
OOME
should already be pending and will be thrown as soon as your native
code
returns to Java.

Thanks,
David


I've also added some spaces along the code to improve indentation.

Would you please review the updated webrev?
http://washi.ru.oracle.com/~igerasim/webrevs/8022584/1/webrev/

Sincerely yours,
Ivan


On 08.08.2013 5:35, David Holmes wrote:

On 8/08/2013 11:20 AM, Ivan Gerasimov wrote:

Thanks Alan!

I've checked that and it turns out that GetStringUTFChars cannot
return
NULL.
For allocation of the result string it calls AllocateHeap() with
the
default EXIT_OOM fail strategy.
Thus, in case of being unable to allocate memory it simply stops
VM.


Ouch! That is a hotspot bug. GetStringUTFChars is supposed to
throw
OutOfMemoryError if it has memory issues, not abort the VM!

I recommend that you check for NULL and/or a pending exception.

David


Sincerely yours,
Ivan

On 08.08.2013 5:05, Alan Bateman wrote:

(cc'ing net-dev).

The change looks okay to me. One suggestion (while you are
there) is
to check the return from  GetStringUTFChars so that the name
returns
when it fails with NULL.

-Alan.

On 07/08/2013 17:46, Ivan Gerasimov wrote:

Hello everybody!

Some methods of NetworkInterface class were reported to leak
memory.
http://bugs.sun.com/view_bug.do?bug_id=JDK-8022584 (not yet
available.)

Examples are isUp() and isLoopback().

Affected versions of jdk are 6, 7 and 8

Would you please review a simple fix that removes the
unnecessary
allocation?
http://cr.openjdk.java.net/~igerasim/8022584/0/webrev/

Sincerely yours,
Ivan

Re: RFR [8022584] Memory leak in some NetworkInterface methods

[David Holmes]

Hi Chris,

On 9/08/2013 8:36 PM, Chris Hegarty wrote:

Firstly, I think the memory leak issue should be moved forward
separately to this cleanup effort. They are unrelated, and I'm starting
to get the feeling that this could take some time to reach conclusion.
It seems reasonable to separate the issues.


I agree. I'm sure when Alan suggested to check the return he didn't 
expect it to unravel like this :) As we know hotspot will never actually 
return NULL there is no urgency to add this in.



On 09/08/2013 10:27, Ivan Gerasimov wrote:

Chris,

I would use this

if ((name_utf = (*env)->GetStringUTFChars(env, name, &isCopy)) == NULL) {
JNU_ThrowOutOfMemoryError(env, "GetStringUTFChars failed");
return NULL/JNI_False/-1;
}

If I understand it correctly, JNU_ThrowOutOfMemoryError throws an
exception only if it hasn't been already thrown.


JNU_ThrowOutOfMemoryError is simply a convenience wrapper for
   JNU_ThrowByName(env, "java/lang/OutOfMemoryError", msg);

  ...and JNU_ThrowByName [1] is defined as...

  JNU_ThrowByName(JNIEnv *env, const char *name, const char *msg) {
 class cls = (*env)->FindClass(env, name);
 if (cls != 0) /* Otherwise an exception has already been thrown */
 (*env)->ThrowNew(env, cls, msg);
 }
  }

Neither FindClass or ThrowNew is safe to call if there is a pending
exception [1].


Right - we have to check for a pending exception before trying to throw one.


Now the issue comes down to; could there ever be a pending exception if
GetStringUTFChars returns NULL? The latest specs doesn't indicate that
there could be, but every copy of "The Java Native Interface
Programmer's Guide and Specification" I can find does. There also
appears to be an assumption of this if you look at the usages in the JDK.


AFAIK there is only one version of the JNI spec book and it never got 
updated. The official spec says no throw, but when people have the book 
on their bookshelf that is what they tend to rely on. I looked at some 
of the usages and they seem exception agnostic - many of them don't even 
check for NULL :(


The implementation as it stands will not throw and will not return NULL.


I would really like to get a definitive answer on the JNI specification
for GetStringUTFChars before making any changes here.


The JNI spec (as opposed to the book) is definitive. If we don't like 
what is there then it requires a spec change.


I can't find any reference to this particular issue being raised before.

Cheers,
David


-Chris.

[1]
http://hg.openjdk.java.net/jdk8/tl/jdk/file/54f0ccdd9ad7/src/share/native/common/jni_util.c

[2]
http://docs.oracle.com/javase/6/docs/technotes/guides/jni/spec/design.html#wp17626




Sincerely yours,
Ivan


On 09.08.2013 11:25, Chris Hegarty wrote:

On 09/08/2013 06:47, David Holmes wrote:

Sorry I messed this up. The JNI book says GetStringUTFChars will return
NULL and post OOME but the JNI spec (latest version 6.0) does not - it
only says it will return NULL on failure.


This is indeed strange. Most usages of this function in the jdk expect
the former. If this is not the case, then we may need to do an audit
of all usages.


So your previous version was the more correct. Given we just failed to
allocate C-heap I think we are on thin ice anyway, but better to at
least attempt to do the right thing.


I'm not sure what the right thing to do here is? It seems a little
unwieldy!

if ((name_utf = (*env)->GetStringUTFChars(env, name, &isCopy)) ==
NULL) {
if ((*env)->ExceptionOccurred(env)) {
return NULL/JNI_False/-1;
} else {
throwException(env, "java/lang/InternalError", "GetStringUTFChars
failed");
return NULL/JNI_False/-1;
}

Given we have no idea why GetStringUTFChars may have failed, what
exception do we throw?

Also worth noting is that this bug fix has moved away from the
original problem (memory leak), and is now focused on code cleanup.

If we cannot get agreement on the cleanup, or it looks like more
clarification is needed around the cleanup effort, then I would like
to suggest that we proceed with the original fix for the memory leak
and separate out the cleanup effort.

-Chris.


FYI I filed 8022683 to fix GetStringUTFChars.

David
-

On 9/08/2013 3:21 PM, David Holmes wrote:

Thumbs up!

Thanks,
David

On 9/08/2013 8:19 AM, Ivan Gerasimov wrote:

Thanks David!

On 09.08.2013 1:15, David Holmes wrote:

Main fix looks good to me.

Regression test may need some tweaking eg I think othervm will be
needed.


Yes, it's a good point.
Since there may be a memory leak in the test, it'd better not
interfere
with other tests in jtreg.


Also this:

System.out.println("WARNING: Cannot perform memory leak detection on
this OS");

should probably just say something like "Test skipped on this OS" -
there are other tests that do this so just check if there is common
terminology. (In th

Re: RFR [8022584] Memory leak in some NetworkInterface methods

[David Holmes]

Thanks Ivan.

David

On 12/08/2013 10:33 PM, Ivan Gerasimov wrote:

David, Chris,

I reverted back NULL-checking.
Now the change consists of one line removal and a regression test.

Webrev: http://cr.openjdk.java.net/~igerasim/8022584/6/webrev/
Hg export:
http://cr.openjdk.java.net/~igerasim/2commit/8022584-jdk8-Memleak-in-NetworkInterface.patch


Sincerely yours,
Ivan

On 09.08.2013 16:18, David Holmes wrote:

Hi Chris,

On 9/08/2013 8:36 PM, Chris Hegarty wrote:

Firstly, I think the memory leak issue should be moved forward
separately to this cleanup effort. They are unrelated, and I'm starting
to get the feeling that this could take some time to reach conclusion.
It seems reasonable to separate the issues.


I agree. I'm sure when Alan suggested to check the return he didn't
expect it to unravel like this :) As we know hotspot will never
actually return NULL there is no urgency to add this in.


On 09/08/2013 10:27, Ivan Gerasimov wrote:

Chris,

I would use this

if ((name_utf = (*env)->GetStringUTFChars(env, name, &isCopy)) ==
NULL) {
JNU_ThrowOutOfMemoryError(env, "GetStringUTFChars failed");
return NULL/JNI_False/-1;
}

If I understand it correctly, JNU_ThrowOutOfMemoryError throws an
exception only if it hasn't been already thrown.


JNU_ThrowOutOfMemoryError is simply a convenience wrapper for
   JNU_ThrowByName(env, "java/lang/OutOfMemoryError", msg);

  ...and JNU_ThrowByName [1] is defined as...

  JNU_ThrowByName(JNIEnv *env, const char *name, const char *msg) {
 class cls = (*env)->FindClass(env, name);
 if (cls != 0) /* Otherwise an exception has already been thrown */
 (*env)->ThrowNew(env, cls, msg);
 }
  }

Neither FindClass or ThrowNew is safe to call if there is a pending
exception [1].


Right - we have to check for a pending exception before trying to
throw one.


Now the issue comes down to; could there ever be a pending exception if
GetStringUTFChars returns NULL? The latest specs doesn't indicate that
there could be, but every copy of "The Java Native Interface
Programmer's Guide and Specification" I can find does. There also
appears to be an assumption of this if you look at the usages in the
JDK.


AFAIK there is only one version of the JNI spec book and it never got
updated. The official spec says no throw, but when people have the
book on their bookshelf that is what they tend to rely on. I looked at
some of the usages and they seem exception agnostic - many of them
don't even check for NULL :(

The implementation as it stands will not throw and will not return NULL.


I would really like to get a definitive answer on the JNI specification
for GetStringUTFChars before making any changes here.


The JNI spec (as opposed to the book) is definitive. If we don't like
what is there then it requires a spec change.

I can't find any reference to this particular issue being raised before.

Cheers,
David


-Chris.

[1]
http://hg.openjdk.java.net/jdk8/tl/jdk/file/54f0ccdd9ad7/src/share/native/common/jni_util.c


[2]
http://docs.oracle.com/javase/6/docs/technotes/guides/jni/spec/design.html#wp17626





Sincerely yours,
Ivan


On 09.08.2013 11:25, Chris Hegarty wrote:

On 09/08/2013 06:47, David Holmes wrote:

Sorry I messed this up. The JNI book says GetStringUTFChars will
return
NULL and post OOME but the JNI spec (latest version 6.0) does not
- it
only says it will return NULL on failure.


This is indeed strange. Most usages of this function in the jdk expect
the former. If this is not the case, then we may need to do an audit
of all usages.


So your previous version was the more correct. Given we just
failed to
allocate C-heap I think we are on thin ice anyway, but better to at
least attempt to do the right thing.


I'm not sure what the right thing to do here is? It seems a little
unwieldy!

if ((name_utf = (*env)->GetStringUTFChars(env, name, &isCopy)) ==
NULL) {
if ((*env)->ExceptionOccurred(env)) {
return NULL/JNI_False/-1;
} else {
throwException(env, "java/lang/InternalError", "GetStringUTFChars
failed");
return NULL/JNI_False/-1;
}

Given we have no idea why GetStringUTFChars may have failed, what
exception do we throw?

Also worth noting is that this bug fix has moved away from the
original problem (memory leak), and is now focused on code cleanup.

If we cannot get agreement on the cleanup, or it looks like more
clarification is needed around the cleanup effort, then I would like
to suggest that we proceed with the original fix for the memory leak
and separate out the cleanup effort.

-Chris.


FYI I filed 8022683 to fix GetStringUTFChars.

David
-

On 9/08/2013 3:21 PM, David Holmes wrote:

Thumbs up!

Thanks,
David

On 9/08/2013 8:19 AM, Ivan Gerasimov wrote:

Thanks David!

On 09.08.2013 1:15, David Holmes wrote:

Main fix looks good to me.

Regression test may need some tweaking eg I think othervm will be
needed.


Yes, i

hg: jdk8/tl/jdk: 8023311: Clean up profile-includes.txt

[david . holmes]

Changeset: e17da8b09f5e
Author:dholmes
Date:  2013-08-20 03:18 -0400
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/e17da8b09f5e

8023311: Clean up profile-includes.txt
Reviewed-by: alanb

! makefiles/profile-includes.txt

hg: jdk8/tl/jdk: 8023460: OPENJDK build fails due to missing jfr.jar

[david . holmes]

Changeset: 3b8fed46b2a8
Author:dholmes
Date:  2013-08-21 05:56 -0400
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/3b8fed46b2a8

8023460: OPENJDK build fails due to missing jfr.jar
Reviewed-by: alanb

! makefiles/Profiles.gmk

hg: jdk8/tl/jdk: 8014135: The JVMTI specification does not conform to recent changes in JNI specification

[david . holmes]

Changeset: 9586ca82bd8b
Author:bpittore
Date:  2013-08-26 11:27 -0400
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/9586ca82bd8b

8014135: The JVMTI specification does not conform to recent changes in JNI 
specification
Summary: Added support for statically linked agents
Reviewed-by: alanb, sspitsyn, bobv, coleenp

! src/share/classes/com/sun/tools/attach/VirtualMachine.java

hg: jdk8/tl/jdk: 8024140: [TESTBUG] Profile based regression test groups for jdk repo

[david . holmes]

Changeset: 2cdd1078f45b
Author:dholmes
Date:  2013-09-03 23:47 -0400
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/2cdd1078f45b

8024140: [TESTBUG] Profile based regression test groups for jdk repo
Reviewed-by: alanb, chegar

! test/TEST.groups