date:20170301

RE: [NEW BUG]: Avoid allocations in String.replace(CharSequence, CharSequence) in case no replacement happened

2017-03-01 Thread Christoph Dreis

Hey Vitaly and Jonathan,

> As mentioned offline, I would move the null check right before "return this".

@Vitaly: Thanks again. Adjusted. 
@Jonathan: Thanks. Thought about that as well, but I'd probably rather go with 
explaining the explicit nullcheck.

See the adjusted patch below. What do you think?

= PATCH ==
diff --git a/src/java.base/share/classes/java/lang/String.java 
b/src/java.base/share/classes/java/lang/String.java
--- a/src/java.base/share/classes/java/lang/String.java
+++ b/src/java.base/share/classes/java/lang/String.java
@@ -2177,11 +2177,13 @@
  */
 public String replace(CharSequence target, CharSequence replacement) {
 String tgtStr = target.toString();
-String replStr = replacement.toString();
 int j = indexOf(tgtStr);
 if (j < 0) {
+// Explicit nullcheck of replacement to fulfill NPE contract in 
early out
+Objects.requireNonNull(replacement);
 return this;
 }
+String replStr = replacement.toString();
 int tgtLen = tgtStr.length();
 int tgtLen1 = Math.max(tgtLen, 1);
 int thisLen = length();

RE: RFR [XS] : 8175000 : jexec fails to execute simple helloworld.jar

2017-03-01 Thread Baesken, Matthias

>If there are no further changes, I don't need to see it, and you
>can add me as a reviewer, and I don't need credit for the test.

Thanks Kumar for review and adding the test.

>As for jexec, it appears that only Linux supports this, and I am
>uncertain who is using it in the Linux community.
>Perhaps we should try deprecate it in jdk10.
>Filed: https://bugs.openjdk.java.net/browse/JDK-8176066

I agree that it makes sense to look into a deprecation of  jexec for jdk10.

Best regards, Matthias

From: Kumar Srinivasan [mailto:kumar.x.sriniva...@oracle.com]
Sent: Mittwoch, 1. März 2017 23:22
To: Thomas Stüfe 
Cc: Baesken, Matthias ; Michel Trudeau 
; core-libs-dev@openjdk.java.net; Simonis, Volker 

Subject: Re: RFR [XS] : 8175000 : jexec fails to execute simple helloworld.jar

Hello Baesken,

I have attached 8175000.patch to the bug report,
it includes Thomas' suggested fix + a test.

I have also tested it,  you may want to post it to corelibs-dev.
If posting a new webrev, please add the pipermail to the bug report,
http://mail.openjdk.java.net/pipermail/core-libs-dev/2017-March/046548.html

If there are no further changes, I don't need to see it, and you
can add me as a reviewer, and I don't need credit for the test.

As for jexec, it appears that only Linux supports this, and I am
uncertain who is using it in the Linux community.
Perhaps we should try deprecate it in jdk10.
Filed: https://bugs.openjdk.java.net/browse/JDK-8176066

Thanks
Kumar

On 3/1/2017 9:14 AM, Thomas Stüfe wrote:
Hi Matthias,

On Wed, Mar 1, 2017 at 5:07 PM, Baesken, Matthias 
> wrote:
Hi Thomas , thanks for looking into it .

I suggest  to handle the read issue in another bug  because this one just deals 
with “jexec fails to execute simple helloworld.jar” .

( there seem to be  a few other read(…) calls  in the jdk code base to consider 
as well )

Can we push the existing change ?

Not a Reviewer, so it is not for me to decide. Someone from core libs should 
look at this.

...Thomas

Regards, Matthias

From: Thomas Stüfe 
[mailto:thomas.stu...@gmail.com]
Sent: Mittwoch, 1. März 2017 12:47
To: Baesken, Matthias 
>
Cc: core-libs-dev@openjdk.java.net; 
Simonis, Volker >
Subject: Re: RFR [XS] : 8175000 : jexec fails to execute simple helloworld.jar

Hi Matthias,

the fix makes sense, this is very clearly a bug.

I'd suggest a simpler fix though:

  end -= 4; // make sure there are 4 bytes to read at start
- while (start < end) {
+while (start <= end) {

Note that the code has a diffent bug too, very unlikely but not impossible to 
hit:

 321 ssize_t count = read(fd, buf, CHUNK_SIZE);
 322 if (count >= MIN_SIZE) {

We attempt to read CHUNK_SIZE bytes and require the read to have returned at 
least MIN_SIZE (something like 30ish bytes). If not, jexec fails.

read may have been interrupted (EINTR) or may have returned less bytes than 
MIN_SIZE, so it should read in a loop til eof or CHUNK_SIZE bytes are read.

Kind Regards, Thomas

On Wed, Mar 1, 2017 at 10:23 AM, Baesken, Matthias 
> wrote:
Ping ...

Can I get a review please for the change ?

Thanks, Matthias

From: Baesken, Matthias
Sent: Donnerstag, 23. Februar 2017 12:28
To: 'core-libs-dev@openjdk.java.net' 
>
Cc: Langer, Christoph 
>; Erik Joelsson 
(erik.joels...@oracle.com) 
>; 'Michel Trudeau' 
>
Subject: RE: RFR [XS] : 8175000 : jexec fails to execute simple helloworld.jar

Here is  the webrev for jdk9 :

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

?  And btw I really wonder  - is  jexec still needed in future jdk's like jdk10 
 ? Seems it is not used much .

?

In case  jexec will stay in  the jdk  I might add a test for the tool as well, 
if there is interest  ( could not really find one that tests execution of a 
simple jar-file with jexec).

Best regards, Matthias

From: Baesken, Matthias
Sent: Donnerstag, 23. Februar 2017 07:39
To: 'core-libs-dev@openjdk.java.net' 
>>
Cc: Langer, Christoph

Re: [NEW BUG]: Avoid allocations in String.replace(CharSequence, CharSequence) in case no replacement happened

2017-03-01 Thread Vitaly Davidovich

As mentioned offline, I would move the null check right before "return
this".
On Wed, Mar 1, 2017 at 6:50 PM Christoph Dreis 
wrote:

> Hey Vitaly,
>
> > Seems like a good idea.  You probably want to null check 'replacement'
> before the bail out as the method is specified as throwing NPE in that case.
>
> Thanks for your feedback. See the adjusted patch below.
>
> = PATCH ==
>
> diff --git a/src/java.base/share/classes/java/lang/String.java
> b/src/java.base/share/classes/java/lang/String.java
> --- a/src/java.base/share/classes/java/lang/String.java
> +++ b/src/java.base/share/classes/java/lang/String.java
> @@ -2176,12 +2176,13 @@
>   * @since 1.5
>   */
>  public String replace(CharSequence target, CharSequence replacement) {
> +Objects.requireNonNull(replacement);
>  String tgtStr = target.toString();
> -String replStr = replacement.toString();
>  int j = indexOf(tgtStr);
>  if (j < 0) {
>  return this;
>  }
> +String replStr = replacement.toString();
>  int tgtLen = tgtStr.length();
>  int tgtLen1 = Math.max(tgtLen, 1);
>  int thisLen = length();
>
> --
Sent from my phone

Re: [NEW BUG]: Avoid allocations in String.replace(CharSequence, CharSequence) in case no replacement happened

2017-03-01 Thread Jonathan Bluett-Duncan

Hi Christoph,

I think it's worth adding a small comment to the end of `String tgtStr =
target.toString();`, saying that the null check is implicit, in case people
read the code in the future and get confused as to why only `replacement`
is apparently null-checked. What do you think?

Kind regards,
Jonathan

On 1 March 2017 at 23:47, Christoph Dreis 
wrote:

> Hey Vitaly,
>
> > Seems like a good idea.  You probably want to null check 'replacement'
> before the bail out as the method is specified as throwing NPE in that case.
>
> Thanks for your feedback. See the adjusted patch below.
>
> = PATCH ==
>
> diff --git a/src/java.base/share/classes/java/lang/String.java
> b/src/java.base/share/classes/java/lang/String.java
> --- a/src/java.base/share/classes/java/lang/String.java
> +++ b/src/java.base/share/classes/java/lang/String.java
> @@ -2176,12 +2176,13 @@
>   * @since 1.5
>   */
>  public String replace(CharSequence target, CharSequence replacement) {
> +Objects.requireNonNull(replacement);
>  String tgtStr = target.toString();
> -String replStr = replacement.toString();
>  int j = indexOf(tgtStr);
>  if (j < 0) {
>  return this;
>  }
> +String replStr = replacement.toString();
>  int tgtLen = tgtStr.length();
>  int tgtLen1 = Math.max(tgtLen, 1);
>  int thisLen = length();
>
>

RE: [NEW BUG]: Avoid allocations in String.replace(CharSequence, CharSequence) in case no replacement happened

2017-03-01 Thread Christoph Dreis

Hey Vitaly,

> Seems like a good idea.  You probably want to null check 'replacement' before 
> the bail out as the method is specified as throwing NPE in that case.

Thanks for your feedback. See the adjusted patch below.

= PATCH ==

diff --git a/src/java.base/share/classes/java/lang/String.java 
b/src/java.base/share/classes/java/lang/String.java
--- a/src/java.base/share/classes/java/lang/String.java
+++ b/src/java.base/share/classes/java/lang/String.java
@@ -2176,12 +2176,13 @@
  * @since 1.5
  */
 public String replace(CharSequence target, CharSequence replacement) {
+Objects.requireNonNull(replacement);
 String tgtStr = target.toString();
-String replStr = replacement.toString();
 int j = indexOf(tgtStr);
 if (j < 0) {
 return this;
 }
+String replStr = replacement.toString();
 int tgtLen = tgtStr.length();
 int tgtLen1 = Math.max(tgtLen, 1);
 int thisLen = length();

Re: [NEW BUG]: Avoid allocations in String.replace(CharSequence, CharSequence) in case no replacement happened

2017-03-01 Thread Vitaly Davidovich

Seems like a good idea.  You probably want to null check 'replacement'
before the bail out as the method is specified as throwing NPE in that case.
On Wed, Mar 1, 2017 at 4:37 PM Christoph Dreis 
wrote:

> Hey,
>
> I just noticed a small improvement for String.replace(CharSequence,
> CharSequence) in case the target sequence is not found.
>
> There is the possibility of an early-out in case there is nothing to
> replace. Yet there is a call to replacement.toString();
> While this is usually not a problem in case of String parameters, a
> StringBuilder - or other more "dynamic" CharSequence.toString()
> implementations - will likely produce unnecessary allocations here.
>
> I think the JDK could prevent that with the given patch below.
>
> I would be very happy if this is sponsored for JDK 10.
>
> Cheers,
> Christoph
>
> = PATCH ==
> diff --git a/src/java.base/share/classes/java/lang/String.java
> b/src/java.base/share/classes/java/lang/String.java
> --- a/src/java.base/share/classes/java/lang/String.java
> +++ b/src/java.base/share/classes/java/lang/String.java
> @@ -2177,11 +2177,11 @@
>   */
>  public String replace(CharSequence target, CharSequence replacement) {
>  String tgtStr = target.toString();
> -String replStr = replacement.toString();
>  int j = indexOf(tgtStr);
>  if (j < 0) {
>  return this;
>  }
> +String replStr = replacement.toString();
>  int tgtLen = tgtStr.length();
>  int tgtLen1 = Math.max(tgtLen, 1);
>  int thisLen = length();
>
> --
Sent from my phone

Re: Proposal: java.lang.reflect.Proxy and default methods

2017-03-01 Thread Mandy Chung

You can use MethodHandles.privateLookupIn (new in JDK 9) to replace Constructor 
hack to get a Lookup object with private access for the target class.

Mandy

> On Mar 1, 2017, at 9:14 PM, mp911de  wrote:
> 
> Is there any progress on this issue? In the light of Java 9, the workaround
> with
> MethodHandles.lookup()/unreflectSpecial does not work anymore because
> MethodHandles is encapsulated and calling setAccessible(true) on the
> constructor fails.
> 
> Resolving method handles inside the same module seems to work with public
> lookup,
> but as soon as a module defines an interface with default methods and this
> interface is called by a proxy handler that comes from a different module,
> it's
> no longer possible to resolve the MethodHandle.
> 
> Is this the appropriate mailing list for this case?
> 
> 
> 
> --
> View this message in context: 
> http://openjdk.5641.n7.nabble.com/Proposal-java-lang-reflect-Proxy-and-default-methods-tp273894p300249.html
> Sent from the OpenJDK Core Libraries mailing list archive at Nabble.com.

Re: RFR [XS] : 8175000 : jexec fails to execute simple helloworld.jar

2017-03-01 Thread Kumar Srinivasan

Hello Baesken,

I have attached 8175000.patch to the bug report,
it includes Thomas' suggested fix + a test.

I have also tested it,  you may want to post it to corelibs-dev.
If posting a new webrev, please add the pipermail to the bug report,
http://mail.openjdk.java.net/pipermail/core-libs-dev/2017-March/046548.html

If there are no further changes, I don't need to see it, and you
can add me as a reviewer, and I don't need credit for the test.

As for jexec, it appears that only Linux supports this, and I am
uncertain who is using it in the Linux community.
Perhaps we should try deprecate it in jdk10.
Filed: https://bugs.openjdk.java.net/browse/JDK-8176066

Thanks
Kumar

On 3/1/2017 9:14 AM, Thomas Stüfe wrote:

Hi Matthias,

On Wed, Mar 1, 2017 at 5:07 PM, Baesken, Matthias 
> wrote:

Hi Thomas , thanks for looking into it .

I suggest  to handle the read issue in another bug  because this
one just deals with “jexec fails to execute simple helloworld.jar” .

( there seem to be  a few other read(…) calls  in the jdk code
base to consider as well )

Can we push the existing change ?

Not a Reviewer, so it is not for me to decide. Someone from core libs 
should look at this.

...Thomas

Regards, Matthias

*From:*Thomas Stüfe [mailto:thomas.stu...@gmail.com
]
*Sent:* Mittwoch, 1. März 2017 12:47
*To:* Baesken, Matthias >
*Cc:* core-libs-dev@openjdk.java.net
; Simonis, Volker
>
*Subject:* Re: RFR [XS] : 8175000 : jexec fails to execute simple
helloworld.jar

Hi Matthias,

the fix makes sense, this is very clearly a bug.

I'd suggest a simpler fix though:

  end -= 4; // make sure there are 4 bytes to
read at start
- while (start < end) {
+while (start <= end) {

Note that the code has a diffent bug too, very unlikely but not
impossible to hit:

 321 ssize_t count = read(fd, buf, CHUNK_SIZE);
 322 if (count >= MIN_SIZE) {

We attempt to read CHUNK_SIZE bytes and require the read to have
returned at least MIN_SIZE (something like 30ish bytes). If not,
jexec fails.

read may have been interrupted (EINTR) or may have returned less
bytes than MIN_SIZE, so it should read in a loop til eof or
CHUNK_SIZE bytes are read.

Kind Regards, Thomas

On Wed, Mar 1, 2017 at 10:23 AM, Baesken, Matthias
> wrote:

Ping ...

Can I get a review please for the change ?

Thanks, Matthias

From: Baesken, Matthias
Sent: Donnerstag, 23. Februar 2017 12:28
To: 'core-libs-dev@openjdk.java.net
'
>
Cc: Langer, Christoph >; Erik Joelsson
(erik.joels...@oracle.com )
>;
'Michel Trudeau' >
Subject: RE: RFR [XS] : 8175000 : jexec fails to execute
simple helloworld.jar

Here is  the webrev for jdk9 :

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

?  And btw I really wonder  - is  jexec still needed in future
jdk's like jdk10 ? Seems it is not used much .

?

In case  jexec will stay in  the jdk  I might add a test for
the tool as well, if there is interest  ( could not really
find one that tests execution of a simple jar-file with jexec).

Best regards, Matthias

From: Baesken, Matthias
Sent: Donnerstag, 23. Februar 2017 07:39
To: 'core-libs-dev@openjdk.java.net
'
>>

Cc: Langer, Christoph >>; Erik Joelsson
(erik.joels...@oracle.com
>) >>
Subject: RE: RFR [XS] : 8175000

[NEW BUG]: Avoid allocations in String.replace(CharSequence, CharSequence) in case no replacement happened

2017-03-01 Thread Christoph Dreis

Hey,

I just noticed a small improvement for String.replace(CharSequence,
CharSequence) in case the target sequence is not found.

There is the possibility of an early-out in case there is nothing to
replace. Yet there is a call to replacement.toString();
While this is usually not a problem in case of String parameters, a
StringBuilder - or other more "dynamic" CharSequence.toString()
implementations - will likely produce unnecessary allocations here.

I think the JDK could prevent that with the given patch below.

I would be very happy if this is sponsored for JDK 10. 

Cheers,
Christoph

= PATCH ==
diff --git a/src/java.base/share/classes/java/lang/String.java
b/src/java.base/share/classes/java/lang/String.java
--- a/src/java.base/share/classes/java/lang/String.java
+++ b/src/java.base/share/classes/java/lang/String.java
@@ -2177,11 +2177,11 @@
  */
 public String replace(CharSequence target, CharSequence replacement) {
 String tgtStr = target.toString();
-String replStr = replacement.toString();
 int j = indexOf(tgtStr);
 if (j < 0) {
 return this;
 }
+String replStr = replacement.toString();
 int tgtLen = tgtStr.length();
 int tgtLen1 = Math.max(tgtLen, 1);
 int thisLen = length();

Re: Proposal: java.lang.reflect.Proxy and default methods

2017-03-01 Thread Steven Schlansker


> On Mar 1, 2017, at 1:14 PM, mp911de  wrote:
> 
> Is there any progress on this issue? In the light of Java 9, the workaround
> with
> MethodHandles.lookup()/unreflectSpecial does not work anymore because
> MethodHandles is encapsulated and calling setAccessible(true) on the
> constructor fails.
> 
> Resolving method handles inside the same module seems to work with public
> lookup,
> but as soon as a module defines an interface with default methods and this
> interface is called by a proxy handler that comes from a different module,
> it's
> no longer possible to resolve the MethodHandle.
> 
> Is this the appropriate mailing list for this case?

This is still a sticking point for us too, our (moderately popular) library
currently will not work in Java 9 because of this:

https://github.com/jdbi/jdbi/issues/497

Re: Proposal: java.lang.reflect.Proxy and default methods

2017-03-01 Thread mp911de

Is there any progress on this issue? In the light of Java 9, the workaround
with
MethodHandles.lookup()/unreflectSpecial does not work anymore because
MethodHandles is encapsulated and calling setAccessible(true) on the
constructor fails.

Resolving method handles inside the same module seems to work with public
lookup,
but as soon as a module defines an interface with default methods and this
interface is called by a proxy handler that comes from a different module,
it's
no longer possible to resolve the MethodHandle.

Is this the appropriate mailing list for this case?



--
View this message in context: 
http://openjdk.5641.n7.nabble.com/Proposal-java-lang-reflect-Proxy-and-default-methods-tp273894p300249.html
Sent from the OpenJDK Core Libraries mailing list archive at Nabble.com.

Re: Forcing initialization of string concat INDY expressions

2017-03-01 Thread Remi Forax

The worst thing here is that conceptually, for an invokedynamic, the 
implementation do not use the fact that MethodTypes are interned.
Only a direct call to a method handle (with invokeExact/invoke) needs 
MethodTypes to be interned to speedup the test that verifies that the parameter 
types are compatible.

One way to solve that is to not use the cache when creating a MethodType but 
only when invoking a method handle directly. A MethodHandle can have two 
fields, one storing the non-interned method type (which is final) and one 
storing the corresponding interned method type (which is @Stable) and computed 
it if the pointer check of an invokeExact check fail because 

mh.invokeExact(desc) 
  if (mh.internedMethodType == desc.methodType) {
// fastpath
  }
  if (mh.internedMethodType == null) {
mh.internedMethodType = intern(mh.type);
if (mh.internedMethodType == desc.methodType) {
  // fastpath
}
  }
  ...
   

Rémi

- Mail original -
> De: "David Holmes" 
> À: "Aleksey Shipilev" , "core-libs-dev Libs" 
> 
> Envoyé: Mercredi 1 Mars 2017 13:36:49
> Objet: Re: Forcing initialization of string concat INDY expressions

> Hi Aleksey,
> 
> On 1/03/2017 7:46 PM, Aleksey Shipilev wrote:
>> Hi,
>>
>> On 03/01/2017 07:36 AM, David Holmes wrote:
>>> The INDY-fication of string concatenation has triggered a problem where a 
>>> JVM TI
>>> agent's monitor-wait/ed callback hits an error path that uses string concat
>>> which triggers a mass of indy related initialization, which in turn hits 
>>> monitor
>>> use in MethodType$ConcurrentWeakInternSet.get, which causes the VM monitor
>>> subsystem to be re-entered (and it is not reentrant!) so we crash. (log 
>>> extract
>>> below - the amount of code to process this is truly scary!)
>>
>> Ouch. This is an unexpected circularity. It is unusual to see Java thread to 
>> do
>> Java stuff when doing Object.wait.
> 
> That's the fatal flaw in JVM TI. It pretends you can invoke arbitrary
> Java code via an agent callback. You can invoke simple Java code - as
> most real callbacks do - but not arbitrary Java code. The VM is not
> reentrant in some subsystems - like monitors.
> 
>>> I assume I can cover the exact case above by replicating it? But can I
>>> generalize it to cover arbitrary string concat expressions that might arise 
>>> on
>>> other error paths?
>>
>> Yes, the StringConcatFactory code is deliberately lazy. If you want to link
>> eagerly, you would need to match the concat shape exactly (number and type of
>> arguments) -- probably by using the same concat code the test failed on. We 
>> can
>> probably eagerly link some popular concat shapes early during system init, 
>> but
>> that requires JDK changes.
> 
> So I can cover the current failing case easily enough but can't
> generalize except by covering each case individually.
> 
>> Looking at stack trace, it seems to be generic failure when adding new
>> MethodType to the MethodType cache. This is not the first time that cache
>> participates in circularities (I wonder if it *always* does). But I am 
>> puzzled
>> how does this happen:
>>
>> ...
>> V  [jvm.dll+0x2b5412]  Runtime1::monitorenter+0x1e2;;
>> ?monitorenter@Runtime1@@CAXPAVJavaThread@@PAVoopDesc@@PAVBasicObjectLock@@@Z+0x1e2
>> v  ~RuntimeStub::monitorenter_nofpu Runtime1 stub
>> J 192 c1
>> java.lang.invoke.MethodType$ConcurrentWeakInternSet.get(Ljava/lang/Object;)Ljava/lang/Object;
>> java.base@9-internal (54 bytes) @ 0x0242d7da [0x0242d4c0+0x031a]
>> ...
>>
>> It calls RuntimeStub::monitorenter_nofpu destructor? Why it ends up calling 
>> into
>> monitorenter? There are no locks in j.l.i.MT$CWIS.get (I checked the bytecode
>> too).
> 
> get() is compiled so I'm assuming it has inlined something from CHM that
> does the locking.
> 
>> If you want a nuclear option for your test, you may want to pass
>> -XDstringConcat:inline to javac to disable indy string concat to bypass that
>> circularity completely.
> 
> H ... that is worth thinking about.
> 
> Thanks,
> David
> 
>> Thanks,
>> -Aleksey

Re: RFR [XS] : 8175000 : jexec fails to execute simple helloworld.jar

2017-03-01 Thread Kumar Srinivasan

Hi Baesken,

Thanks for fixing this, BUT..
Has this been tested on other platforms, especially on both the
Endian platforms.

I think we should have a simplistic regression test for this in
jdk/test/tools/launcher area, until we EOL this.

Once we have that then we need to test this on all platforms.
So I will write a test, and test this change, through our test and
build system, please hold off until then.

Thanks
Kumar

Hi Matthias,

On Wed, Mar 1, 2017 at 5:07 PM, Baesken, Matthias 
> wrote:

Hi Thomas , thanks for looking into it .

I suggest  to handle the read issue in another bug  because this
one just deals with “jexec fails to execute simple helloworld.jar” .

( there seem to be  a few other read(…) calls  in the jdk code
base to consider as well )

Can we push the existing change ?

Not a Reviewer, so it is not for me to decide. Someone from core libs 
should look at this.

...Thomas

Regards, Matthias

*From:*Thomas Stüfe [mailto:thomas.stu...@gmail.com
]
*Sent:* Mittwoch, 1. März 2017 12:47
*To:* Baesken, Matthias >
*Cc:* core-libs-dev@openjdk.java.net
; Simonis, Volker
>
*Subject:* Re: RFR [XS] : 8175000 : jexec fails to execute simple
helloworld.jar

Hi Matthias,

the fix makes sense, this is very clearly a bug.

I'd suggest a simpler fix though:

  end -= 4; // make sure there are 4 bytes to
read at start
- while (start < end) {
+while (start <= end) {

Note that the code has a diffent bug too, very unlikely but not
impossible to hit:

 321 ssize_t count = read(fd, buf, CHUNK_SIZE);
 322 if (count >= MIN_SIZE) {

We attempt to read CHUNK_SIZE bytes and require the read to have
returned at least MIN_SIZE (something like 30ish bytes). If not,
jexec fails.

read may have been interrupted (EINTR) or may have returned less
bytes than MIN_SIZE, so it should read in a loop til eof or
CHUNK_SIZE bytes are read.

Kind Regards, Thomas

On Wed, Mar 1, 2017 at 10:23 AM, Baesken, Matthias
> wrote:

Ping ...

Can I get a review please for the change ?

Thanks, Matthias

From: Baesken, Matthias
Sent: Donnerstag, 23. Februar 2017 12:28
To: 'core-libs-dev@openjdk.java.net
'
>
Cc: Langer, Christoph >; Erik Joelsson
(erik.joels...@oracle.com )
>;
'Michel Trudeau' >
Subject: RE: RFR [XS] : 8175000 : jexec fails to execute
simple helloworld.jar

Here is  the webrev for jdk9 :

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

?  And btw I really wonder  - is  jexec still needed in future
jdk's like jdk10 ? Seems it is not used much .

?

In case  jexec will stay in  the jdk  I might add a test for
the tool as well, if there is interest  ( could not really
find one that tests execution of a simple jar-file with jexec).

Best regards, Matthias

From: Baesken, Matthias
Sent: Donnerstag, 23. Februar 2017 07:39
To: 'core-libs-dev@openjdk.java.net
'
>>

Cc: Langer, Christoph >>; Erik Joelsson
(erik.joels...@oracle.com
>) >>
Subject: RE: RFR [XS] : 8175000 : jexec fails to execute
simple helloworld.jar

Hello,  probably I should add the info that the fix is needed 
in jdk9 as well , not only jdk10 .

Without the fix jdk9/10show this error  when executing a
small example jar :

Re: RFR [XS] : 8175000 : jexec fails to execute simple helloworld.jar

2017-03-01 Thread Henry Jen


> On Mar 1, 2017, at 3:47 AM, Thomas Stüfe  wrote:
> 
> Hi Matthias,
> 
> the fix makes sense, this is very clearly a bug.
> 
> I'd suggest a simpler fix though:
> 
>  end -= 4; // make sure there are 4 bytes to read at
> start
> - while (start < end) {
> +while (start <= end) {
> 

+1.

Cheers,
Henry


> Note that the code has a diffent bug too, very unlikely but not impossible
> to hit:
> 
> 321 ssize_t count = read(fd, buf, CHUNK_SIZE);
> 322 if (count >= MIN_SIZE) {
> 
> We attempt to read CHUNK_SIZE bytes and require the read to have returned
> at least MIN_SIZE (something like 30ish bytes). If not, jexec fails.
> 
> read may have been interrupted (EINTR) or may have returned less bytes than
> MIN_SIZE, so it should read in a loop til eof or CHUNK_SIZE bytes are read.
> 
> Kind Regards, Thomas
> 
> 
> On Wed, Mar 1, 2017 at 10:23 AM, Baesken, Matthias > wrote:
> 
>> Ping ...
>> 
>> Can I get a review please for the change ?
>> 
>> 
>> Thanks, Matthias
>> 
>> From: Baesken, Matthias
>> Sent: Donnerstag, 23. Februar 2017 12:28
>> To: 'core-libs-dev@openjdk.java.net' 
>> Cc: Langer, Christoph ; Erik Joelsson (
>> erik.joels...@oracle.com) ; 'Michel Trudeau' <
>> michel.trud...@oracle.com>
>> Subject: RE: RFR [XS] : 8175000 : jexec fails to execute simple
>> helloworld.jar
>> 
>> Here is  the webrev for jdk9 :
>> 
>> http://cr.openjdk.java.net/~mbaesken/webrevs/8175000/
>> 
>> 
>> ?  And btw I really wonder  - is  jexec still needed in future jdk's like
>> jdk10  ? Seems it is not used much .
>> 
>> ?
>> 
>> In case  jexec will stay in  the jdk  I might add a test for the tool as
>> well, if there is interest  ( could not really find one that tests
>> execution of a simple jar-file with jexec).
>> 
>> Best regards, Matthias
>> 
>> 
>> From: Baesken, Matthias
>> Sent: Donnerstag, 23. Februar 2017 07:39
>> To: 'core-libs-dev@openjdk.java.net' > >
>> Cc: Langer, Christoph  christoph.lan...@sap.com>>; Erik Joelsson (erik.joels...@oracle.com<
>> mailto:erik.joels...@oracle.com>)  erik.joels...@oracle.com>>
>> Subject: RE: RFR [XS] : 8175000 : jexec fails to execute simple
>> helloworld.jar
>> 
>> Hello,  probably I should add the info that the fix is needed  in jdk9 as
>> well , not only jdk10 .
>> 
>> Without the fix jdk9/10show this error  when executing a small
>> example jar :
>> 
>> /myjdk9/images/jdk/lib/jexec  /java_test/hellojar/helloworld.jar
>> invalid file (bad magic number): Exec format error
>> 
>> with the fix :
>> 
>> jdk/lib/jexec/java_test/hellojar/helloworld.jar
>> Hello world from a jar file
>> 
>> 
>> And btw I really wonder  - is  jexec still needed in future jdk's like
>> jdk10  ? Seems it is not used much .
>> 
>> Best regards, Matthias
>> 
>> 
>> From: Baesken, Matthias
>> Sent: Mittwoch, 22. Februar 2017 18:16
>> To: core-libs-dev@openjdk.java.net
>> Cc: Langer, Christoph  christoph.lan...@sap.com>>; Erik Joelsson (erik.joels...@oracle.com<
>> mailto:erik.joels...@oracle.com>)  erik.joels...@oracle.com>>
>> Subject: RFR [XS] : 8175000 : jexec fails to execute simple helloworld.jar
>> 
>> Hello , when looking into  the jexec build I noticed   that  execution of
>> a simple helloworld.jar   with jexec does not work any more.
>> 
>> I did a little patch for this which adjusted the addition done with  CR
>> 8156478: 3 Buffer overrun defect groups in jexec.c> oracle.com/mproxy/repository/technology/java2/jdk9/jdk/rev/4f96129b45ee>
>> .
>> 
>> Could I have a review ( just a diff this time is provided because of
>> infrastructure issues) for it ?
>> 
>> 
>> Thanks, Matthias
>> 
>> Bug :
>> https://bugs.openjdk.java.net/browse/JDK-8175000
>> 
>> 
>> Diff for jdk10  :
>> 
>> # HG changeset patch
>> # User mbaesken
>> # Date 1487782485 -3600
>> #  Wed Feb 22 17:54:45 2017 +0100
>> # Node ID 93d55a711f3b1c3f282e6889c24d13f16d4a4548
>> # Parent  884872263accabd4ab68d005abd4e5393144aa4f
>> 8175000: jexec fails to execute simple helloworld.jar
>> 
>> diff --git a/src/java.base/unix/native/launcher/jexec.c
>> b/src/java.base/unix/native/launcher/jexec.c
>> --- a/src/java.base/unix/native/launcher/jexec.c
>> +++ b/src/java.base/unix/native/launcher/jexec.c
>> @@ -331,8 +331,9 @@
>> off_t end   = start  + xlen;
>> 
>> if (end <= count) {
>> -end -= 4; // make sure there are 4 bytes to read at
>> start
>> -while (start < end) {
>> +// make sure there are 4 bytes to read at start
>> +end -= 3;
>> +while ((start <

Re: [10] RFR: 8176041: Optimize handling of comment lines in Properties$LineReader.readLine

2017-03-01 Thread Xueming Shen


+1

On 3/1/17 9:11 AM, Claes Redestad wrote:

Hi Aleksey!

On 03/01/2017 03:40 PM, Aleksey Shipilev wrote:

On 03/01/2017 04:34 PM, Claes Redestad wrote:

the properties line reader has some relevance to startup, e.g.,
when reading the java.security file, and as more documentation
content has been added to this file it has been made apparent
that the handling of comment lines is inefficient due to treating
every character after a comment line marker ('#' or '!') as any
other character (writes it to an internal buffer etc) when a
fast-path consuming the rest of the line would do.

Bug: https://bugs.openjdk.java.net/browse/JDK-8176041
Webrev: http://cr.openjdk.java.net/~redestad/8176041/jdk.01

The idea looks fine.

I think you can go further and unswitch the loop, saving branch in a 
hot loop,
and using byte comparisons (may require some work to do efficient 
byte-char

comparisons):

 if (inStream != null) {
while (inOff < inLimit) {
   byte b = inByteBuf[inOff++];
   if (b == '\n' || b == '\r' || b == '\\') {
  break;
   }
}
 } else {
while (inOff < inLimit) {
   c = inCharBuf[inOff++];
   if (c == '\n' || c == '\r' || c == '\\') {
  break;
   }
}
 }


Sure, with a bit of fix-up this drops another 170k executed bytecodes
to read in the java.security file (original: 1882k, now: 980k):

http://cr.openjdk.java.net/~redestad/8176041/jdk.02/

It seems javac optimizes byte-char comparisons pretty well in this
case since the chars we're comparing against are all in the 0-127 range,
so I don't think there's much we can do:

   256: getfield  #7  // Field inByteBuf:[B
   259: aload_0
   260: dup
   261: getfield  #5  // Field inOff:I
   264: dup_x1
   265: iconst_1
   266: iadd
   267: putfield  #5  // Field inOff:I
   270: baload
   271: istore9
   273: iload 9
   275: bipush10 <- '\n'
   277: if_icmpeq 294
   280: iload 9
   282: bipush13  <- '\r'
   284: if_icmpeq 294
   287: iload 9
   289: bipush92   <- '\\'

Thanks!

/Claes



Thanks,
-Aleksey

Re: [10] RFR: 8176041: Optimize handling of comment lines in Properties$LineReader.readLine

2017-03-01 Thread Aleksey Shipilev

On 03/01/2017 06:11 PM, Claes Redestad wrote:
> Sure, with a bit of fix-up this drops another 170k executed bytecodes
> to read in the java.security file (original: 1882k, now: 980k):
> 
> http://cr.openjdk.java.net/~redestad/8176041/jdk.02/

Ok, that's fine.

This is slightly more idiomatic, you might want to change before pushing:
  c = (char)(b & 0xFF);

Thanks,
-Aleksey

Re: RFR [XS] : 8175000 : jexec fails to execute simple helloworld.jar

2017-03-01 Thread Thomas Stüfe

Hi Matthias,

On Wed, Mar 1, 2017 at 5:07 PM, Baesken, Matthias 
wrote:

> Hi Thomas , thanks for looking into it .
>
>
>
> I suggest  to handle the read issue in another bug  because this one just
> deals with “jexec fails to execute simple helloworld.jar” .
>
>
>
> ( there seem to be  a few other read(…) calls  in the jdk code base to
> consider as well )
>
>
>
> Can we push the existing change ?
>
>
>

Not a Reviewer, so it is not for me to decide. Someone from core libs
should look at this.

...Thomas



> Regards, Matthias
>
>
>
>
>
> *From:* Thomas Stüfe [mailto:thomas.stu...@gmail.com]
> *Sent:* Mittwoch, 1. März 2017 12:47
> *To:* Baesken, Matthias 
> *Cc:* core-libs-dev@openjdk.java.net; Simonis, Volker <
> volker.simo...@sap.com>
> *Subject:* Re: RFR [XS] : 8175000 : jexec fails to execute simple
> helloworld.jar
>
>
>
> Hi Matthias,
>
>
>
> the fix makes sense, this is very clearly a bug.
>
> I'd suggest a simpler fix though:
>
>   end -= 4; // make sure there are 4 bytes to read at
> start
> - while (start < end) {
> +while (start <= end) {
>
> Note that the code has a diffent bug too, very unlikely but not impossible
> to hit:
>
>  321 ssize_t count = read(fd, buf, CHUNK_SIZE);
>  322 if (count >= MIN_SIZE) {
>
> We attempt to read CHUNK_SIZE bytes and require the read to have returned
> at least MIN_SIZE (something like 30ish bytes). If not, jexec fails.
>
> read may have been interrupted (EINTR) or may have returned less bytes
> than MIN_SIZE, so it should read in a loop til eof or CHUNK_SIZE bytes are
> read.
>
> Kind Regards, Thomas
>
>
>
>
>
> On Wed, Mar 1, 2017 at 10:23 AM, Baesken, Matthias <
> matthias.baes...@sap.com> wrote:
>
> Ping ...
>
> Can I get a review please for the change ?
>
>
> Thanks, Matthias
>
> From: Baesken, Matthias
> Sent: Donnerstag, 23. Februar 2017 12:28
> To: 'core-libs-dev@openjdk.java.net' 
> Cc: Langer, Christoph ; Erik Joelsson (
> erik.joels...@oracle.com) ; 'Michel Trudeau' <
> michel.trud...@oracle.com>
> Subject: RE: RFR [XS] : 8175000 : jexec fails to execute simple
> helloworld.jar
>
> Here is  the webrev for jdk9 :
>
> http://cr.openjdk.java.net/~mbaesken/webrevs/8175000/
>
>
> ?  And btw I really wonder  - is  jexec still needed in future jdk's like
> jdk10  ? Seems it is not used much .
>
> ?
>
> In case  jexec will stay in  the jdk  I might add a test for the tool as
> well, if there is interest  ( could not really find one that tests
> execution of a simple jar-file with jexec).
>
> Best regards, Matthias
>
>
> From: Baesken, Matthias
> Sent: Donnerstag, 23. Februar 2017 07:39
> To: 'core-libs-dev@openjdk.java.net'  >
>
> Cc: Langer, Christoph >; Erik Joelsson (erik.joels...@oracle.com<
> mailto:erik.joels...@oracle.com>) >
> Subject: RE: RFR [XS] : 8175000 : jexec fails to execute simple
> helloworld.jar
>
> Hello,  probably I should add the info that the fix is needed  in jdk9 as
> well , not only jdk10 .
>
> Without the fix jdk9/10show this error  when executing a small
> example jar :
>
> /myjdk9/images/jdk/lib/jexec  /java_test/hellojar/helloworld.jar
> invalid file (bad magic number): Exec format error
>
> with the fix :
>
> jdk/lib/jexec/java_test/hellojar/helloworld.jar
> Hello world from a jar file
>
>
> And btw I really wonder  - is  jexec still needed in future jdk's like
> jdk10  ? Seems it is not used much .
>
> Best regards, Matthias
>
>
> From: Baesken, Matthias
> Sent: Mittwoch, 22. Februar 2017 18:16
> To: core-libs-dev@openjdk.java.net
> Cc: Langer, Christoph >; Erik Joelsson (erik.joels...@oracle.com<
> mailto:erik.joels...@oracle.com>) >
> Subject: RFR [XS] : 8175000 : jexec fails to execute simple helloworld.jar
>
> Hello , when looking into  the jexec build I noticed   that  execution of
> a simple helloworld.jar   with jexec does not work any more.
>
> I did a little patch for this which adjusted the addition done with  CR
> 8156478: 3 Buffer overrun defect groups in jexec.c oracle.com/mproxy/repository/technology/java2/jdk9/jdk/rev/4f96129b45ee>
> .
>
> Could I have a review ( just a diff this time is provided because of
> infrastructure issues) for it ?
>
>
> Thanks, Matthias
>
> Bug :
> https://bugs.openjdk.java.net/browse/JDK-8175000
>
>
> Diff for jdk10  :
>
> # HG changeset patch
> # User mbaesken
> # Date 1487782485 -3600
> #  Wed Feb 22 17:54:45 2017 +0100
> # Node ID 93d55a711f3b1c3f282e6889c24d13f16d4a4548
> # Parent  884872263accabd4ab68d005abd4e5393144aa4f

Re: [10] RFR: 8176041: Optimize handling of comment lines in Properties$LineReader.readLine

2017-03-01 Thread Claes Redestad


Hi Aleksey!

On 03/01/2017 03:40 PM, Aleksey Shipilev wrote:

On 03/01/2017 04:34 PM, Claes Redestad wrote:

the properties line reader has some relevance to startup, e.g.,
when reading the java.security file, and as more documentation
content has been added to this file it has been made apparent
that the handling of comment lines is inefficient due to treating
every character after a comment line marker ('#' or '!') as any
other character (writes it to an internal buffer etc) when a
fast-path consuming the rest of the line would do.

Bug: https://bugs.openjdk.java.net/browse/JDK-8176041
Webrev: http://cr.openjdk.java.net/~redestad/8176041/jdk.01

The idea looks fine.

I think you can go further and unswitch the loop, saving branch in a hot loop,
and using byte comparisons (may require some work to do efficient byte-char
comparisons):

 if (inStream != null) {
while (inOff < inLimit) {
   byte b = inByteBuf[inOff++];
   if (b == '\n' || b == '\r' || b == '\\') {
  break;
   }
}
 } else {
while (inOff < inLimit) {
   c = inCharBuf[inOff++];
   if (c == '\n' || c == '\r' || c == '\\') {
  break;
   }
}
 }


Sure, with a bit of fix-up this drops another 170k executed bytecodes
to read in the java.security file (original: 1882k, now: 980k):

http://cr.openjdk.java.net/~redestad/8176041/jdk.02/

It seems javac optimizes byte-char comparisons pretty well in this
case since the chars we're comparing against are all in the 0-127 range,
so I don't think there's much we can do:

   256: getfield  #7  // Field inByteBuf:[B
   259: aload_0
   260: dup
   261: getfield  #5  // Field inOff:I
   264: dup_x1
   265: iconst_1
   266: iadd
   267: putfield  #5  // Field inOff:I
   270: baload
   271: istore9
   273: iload 9
   275: bipush10 <- '\n'
   277: if_icmpeq 294
   280: iload 9
   282: bipush13  <- '\r'
   284: if_icmpeq 294
   287: iload 9
   289: bipush92   <- '\\'

Thanks!

/Claes



Thanks,
-Aleksey

RE: RFR [XS] : 8175000 : jexec fails to execute simple helloworld.jar

2017-03-01 Thread Baesken, Matthias

Hi Thomas , thanks for looking into it .

I suggest  to handle the read issue in another bug  because this one just deals 
with “jexec fails to execute simple helloworld.jar” .

( there seem to be  a few other read(…) calls  in the jdk code base to consider 
as well )

Can we push the existing change ?

Regards, Matthias

From: Thomas Stüfe [mailto:thomas.stu...@gmail.com]
Sent: Mittwoch, 1. März 2017 12:47
To: Baesken, Matthias 
Cc: core-libs-dev@openjdk.java.net; Simonis, Volker 
Subject: Re: RFR [XS] : 8175000 : jexec fails to execute simple helloworld.jar

Hi Matthias,

the fix makes sense, this is very clearly a bug.

I'd suggest a simpler fix though:

  end -= 4; // make sure there are 4 bytes to read at start
- while (start < end) {
+while (start <= end) {

Note that the code has a diffent bug too, very unlikely but not impossible to 
hit:

 321 ssize_t count = read(fd, buf, CHUNK_SIZE);
 322 if (count >= MIN_SIZE) {

We attempt to read CHUNK_SIZE bytes and require the read to have returned at 
least MIN_SIZE (something like 30ish bytes). If not, jexec fails.

read may have been interrupted (EINTR) or may have returned less bytes than 
MIN_SIZE, so it should read in a loop til eof or CHUNK_SIZE bytes are read.

Kind Regards, Thomas

On Wed, Mar 1, 2017 at 10:23 AM, Baesken, Matthias 
> wrote:
Ping ...

Can I get a review please for the change ?

Thanks, Matthias

From: Baesken, Matthias
Sent: Donnerstag, 23. Februar 2017 12:28
To: 'core-libs-dev@openjdk.java.net' 
>
Cc: Langer, Christoph 
>; Erik Joelsson 
(erik.joels...@oracle.com) 
>; 'Michel Trudeau' 
>
Subject: RE: RFR [XS] : 8175000 : jexec fails to execute simple helloworld.jar

Here is  the webrev for jdk9 :

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

?  And btw I really wonder  - is  jexec still needed in future jdk's like jdk10 
 ? Seems it is not used much .

?

In case  jexec will stay in  the jdk  I might add a test for the tool as well, 
if there is interest  ( could not really find one that tests execution of a 
simple jar-file with jexec).

Best regards, Matthias

From: Baesken, Matthias
Sent: Donnerstag, 23. Februar 2017 07:39
To: 'core-libs-dev@openjdk.java.net' 
>>
Cc: Langer, Christoph 
>>;
 Erik Joelsson 
(erik.joels...@oracle.com>)

>>
Subject: RE: RFR [XS] : 8175000 : jexec fails to execute simple helloworld.jar

Hello,  probably I should add the info that the fix is needed  in jdk9 as well 
, not only jdk10 .

Without the fix jdk9/10show this error  when executing a small  example jar 
:

/myjdk9/images/jdk/lib/jexec  /java_test/hellojar/helloworld.jar
invalid file (bad magic number): Exec format error

with the fix :

jdk/lib/jexec/java_test/hellojar/helloworld.jar
Hello world from a jar file

And btw I really wonder  - is  jexec still needed in future jdk's like jdk10  ? 
Seems it is not used much .

Best regards, Matthias

From: Baesken, Matthias
Sent: Mittwoch, 22. Februar 2017 18:16
To: 
core-libs-dev@openjdk.java.net>
Cc: Langer, Christoph 
>>;
 Erik Joelsson 
(erik.joels...@oracle.com>)

>>
Subject: RFR [XS] : 8175000 : jexec fails to execute simple helloworld.jar

Hello , when looking into  the jexec build I noticed   that  execution of a 
simple helloworld.jar   with jexec does not work any more.

I did a little patch for this which adjusted the addition done with  CR  
8156478: 3 Buffer overrun defect groups in 
jexec.c
  .

Re: [10] RFR: 8176041: Optimize handling of comment lines in Properties$LineReader.readLine

2017-03-01 Thread Aleksey Shipilev

On 03/01/2017 04:34 PM, Claes Redestad wrote:
> the properties line reader has some relevance to startup, e.g.,
> when reading the java.security file, and as more documentation
> content has been added to this file it has been made apparent
> that the handling of comment lines is inefficient due to treating
> every character after a comment line marker ('#' or '!') as any
> other character (writes it to an internal buffer etc) when a
> fast-path consuming the rest of the line would do.
> 
> Bug: https://bugs.openjdk.java.net/browse/JDK-8176041
> Webrev: http://cr.openjdk.java.net/~redestad/8176041/jdk.01

The idea looks fine.

I think you can go further and unswitch the loop, saving branch in a hot loop,
and using byte comparisons (may require some work to do efficient byte-char
comparisons):

if (inStream != null) {
   while (inOff < inLimit) {
  byte b = inByteBuf[inOff++];
  if (b == '\n' || b == '\r' || b == '\\') {
 break;
  }
   }
} else {
   while (inOff < inLimit) {
  c = inCharBuf[inOff++];
  if (c == '\n' || c == '\r' || c == '\\') {
 break;
  }
   }
}

Thanks,
-Aleksey

[10] RFR: 8176041: Optimize handling of comment lines in Properties$LineReader.readLine

2017-03-01 Thread Claes Redestad


Hi,

the properties line reader has some relevance to startup, e.g.,
when reading the java.security file, and as more documentation
content has been added to this file it has been made apparent
that the handling of comment lines is inefficient due to treating
every character after a comment line marker ('#' or '!') as any
other character (writes it to an internal buffer etc) when a
fast-path consuming the rest of the line would do.

Bug: https://bugs.openjdk.java.net/browse/JDK-8176041
Webrev: http://cr.openjdk.java.net/~redestad/8176041/jdk.01

This cuts off ~1ms from startup on apps that read the
java.security property file, and while this is tailored to improve
performance in interpreted code during startup there's also a
reasonable speedup in JIT:ed code.

(As this is actually resolving an extremely small regression in
9 due to the java.security file having grown, it's likely too late
and minor to go into 9..)

Thanks!

/Claes

Re: 10 RFR: 8150687: typedefs without type names

2017-03-01 Thread Kim Barrett

> On Feb 28, 2017, at 9:20 PM, Brian Burkhalter  
> wrote:
> 
> +1
> 
> Brian
> 
> On Feb 28, 2017, at 6:05 PM, David Holmes  wrote:
> 
>> Hi Kim,
>> 
>> nio-dev should look at this - cc'd.
>> 
>> But looks fine to me. :)
>> 
>> David
>> 
>> On 1/03/2017 11:56 AM, Kim Barrett wrote:
>>> Please review this small change, removing unnecessary uses of "typedef" 
>>> that lead to warnings from Visual Studio 2015.
>>> 
>>> CR:
>>> https://bugs.openjdk.java.net/browse/JDK-8150687
>>> 
>>> Webrev:
>>> http://cr.openjdk.java.net/~kbarrett/8150687/jdk.00/
>>> 
>>> Testing:
>>> jprt

Thanks, David and Brian.

Re: Forcing initialization of string concat INDY expressions

2017-03-01 Thread David Holmes

Hi Aleksey,

On 1/03/2017 7:46 PM, Aleksey Shipilev wrote:

Hi,

On 03/01/2017 07:36 AM, David Holmes wrote:

The INDY-fication of string concatenation has triggered a problem where a JVM TI
agent's monitor-wait/ed callback hits an error path that uses string concat
which triggers a mass of indy related initialization, which in turn hits monitor
use in MethodType$ConcurrentWeakInternSet.get, which causes the VM monitor
subsystem to be re-entered (and it is not reentrant!) so we crash. (log extract
below - the amount of code to process this is truly scary!)

Ouch. This is an unexpected circularity. It is unusual to see Java thread to do
Java stuff when doing Object.wait.

That's the fatal flaw in JVM TI. It pretends you can invoke arbitrary
Java code via an agent callback. You can invoke simple Java code - as
most real callbacks do - but not arbitrary Java code. The VM is not
reentrant in some subsystems - like monitors.

I assume I can cover the exact case above by replicating it? But can I
generalize it to cover arbitrary string concat expressions that might arise on
other error paths?

Yes, the StringConcatFactory code is deliberately lazy. If you want to link
eagerly, you would need to match the concat shape exactly (number and type of
arguments) -- probably by using the same concat code the test failed on. We can
probably eagerly link some popular concat shapes early during system init, but
that requires JDK changes.

So I can cover the current failing case easily enough but can't
generalize except by covering each case individually.

Looking at stack trace, it seems to be generic failure when adding new
MethodType to the MethodType cache. This is not the first time that cache
participates in circularities (I wonder if it *always* does). But I am puzzled
how does this happen:

...
V [jvm.dll+0x2b5412] Runtime1::monitorenter+0x1e2;;
?monitorenter@Runtime1@@CAXPAVJavaThread@@PAVoopDesc@@PAVBasicObjectLock@@@Z+0x1e2
v ~RuntimeStub::monitorenter_nofpu Runtime1 stub
J 192 c1
java.lang.invoke.MethodType$ConcurrentWeakInternSet.get(Ljava/lang/Object;)Ljava/lang/Object;
java.base@9-internal (54 bytes) @ 0x0242d7da [0x0242d4c0+0x031a]
...

It calls RuntimeStub::monitorenter_nofpu destructor? Why it ends up calling into
monitorenter? There are no locks in j.l.i.MT$CWIS.get (I checked the bytecode
too).

get() is compiled so I'm assuming it has inlined something from CHM that
does the locking.

If you want a nuclear option for your test, you may want to pass
-XDstringConcat:inline to javac to disable indy string concat to bypass that
circularity completely.

H ... that is worth thinking about.

Thanks,
David

Thanks,
-Aleksey

Re: RFR [XS] : 8175000 : jexec fails to execute simple helloworld.jar

2017-03-01 Thread Thomas Stüfe

Hi Matthias,

the fix makes sense, this is very clearly a bug.

I'd suggest a simpler fix though:

  end -= 4; // make sure there are 4 bytes to read at
start
- while (start < end) {
+while (start <= end) {

Note that the code has a diffent bug too, very unlikely but not impossible
to hit:

 321 ssize_t count = read(fd, buf, CHUNK_SIZE);
 322 if (count >= MIN_SIZE) {

We attempt to read CHUNK_SIZE bytes and require the read to have returned
at least MIN_SIZE (something like 30ish bytes). If not, jexec fails.

read may have been interrupted (EINTR) or may have returned less bytes than
MIN_SIZE, so it should read in a loop til eof or CHUNK_SIZE bytes are read.

Kind Regards, Thomas


On Wed, Mar 1, 2017 at 10:23 AM, Baesken, Matthias  wrote:

> Ping ...
>
> Can I get a review please for the change ?
>
>
> Thanks, Matthias
>
> From: Baesken, Matthias
> Sent: Donnerstag, 23. Februar 2017 12:28
> To: 'core-libs-dev@openjdk.java.net' 
> Cc: Langer, Christoph ; Erik Joelsson (
> erik.joels...@oracle.com) ; 'Michel Trudeau' <
> michel.trud...@oracle.com>
> Subject: RE: RFR [XS] : 8175000 : jexec fails to execute simple
> helloworld.jar
>
> Here is  the webrev for jdk9 :
>
> http://cr.openjdk.java.net/~mbaesken/webrevs/8175000/
>
>
> ?  And btw I really wonder  - is  jexec still needed in future jdk's like
> jdk10  ? Seems it is not used much .
>
> ?
>
> In case  jexec will stay in  the jdk  I might add a test for the tool as
> well, if there is interest  ( could not really find one that tests
> execution of a simple jar-file with jexec).
>
> Best regards, Matthias
>
>
> From: Baesken, Matthias
> Sent: Donnerstag, 23. Februar 2017 07:39
> To: 'core-libs-dev@openjdk.java.net'  >
> Cc: Langer, Christoph >; Erik Joelsson (erik.joels...@oracle.com<
> mailto:erik.joels...@oracle.com>) >
> Subject: RE: RFR [XS] : 8175000 : jexec fails to execute simple
> helloworld.jar
>
> Hello,  probably I should add the info that the fix is needed  in jdk9 as
> well , not only jdk10 .
>
> Without the fix jdk9/10show this error  when executing a small
> example jar :
>
> /myjdk9/images/jdk/lib/jexec  /java_test/hellojar/helloworld.jar
> invalid file (bad magic number): Exec format error
>
> with the fix :
>
> jdk/lib/jexec/java_test/hellojar/helloworld.jar
> Hello world from a jar file
>
>
> And btw I really wonder  - is  jexec still needed in future jdk's like
> jdk10  ? Seems it is not used much .
>
> Best regards, Matthias
>
>
> From: Baesken, Matthias
> Sent: Mittwoch, 22. Februar 2017 18:16
> To: core-libs-dev@openjdk.java.net
> Cc: Langer, Christoph >; Erik Joelsson (erik.joels...@oracle.com<
> mailto:erik.joels...@oracle.com>) >
> Subject: RFR [XS] : 8175000 : jexec fails to execute simple helloworld.jar
>
> Hello , when looking into  the jexec build I noticed   that  execution of
> a simple helloworld.jar   with jexec does not work any more.
>
> I did a little patch for this which adjusted the addition done with  CR
> 8156478: 3 Buffer overrun defect groups in jexec.c oracle.com/mproxy/repository/technology/java2/jdk9/jdk/rev/4f96129b45ee>
> .
>
> Could I have a review ( just a diff this time is provided because of
> infrastructure issues) for it ?
>
>
> Thanks, Matthias
>
> Bug :
> https://bugs.openjdk.java.net/browse/JDK-8175000
>
>
> Diff for jdk10  :
>
> # HG changeset patch
> # User mbaesken
> # Date 1487782485 -3600
> #  Wed Feb 22 17:54:45 2017 +0100
> # Node ID 93d55a711f3b1c3f282e6889c24d13f16d4a4548
> # Parent  884872263accabd4ab68d005abd4e5393144aa4f
> 8175000: jexec fails to execute simple helloworld.jar
>
> diff --git a/src/java.base/unix/native/launcher/jexec.c
> b/src/java.base/unix/native/launcher/jexec.c
> --- a/src/java.base/unix/native/launcher/jexec.c
> +++ b/src/java.base/unix/native/launcher/jexec.c
> @@ -331,8 +331,9 @@
>  off_t end   = start  + xlen;
>
>  if (end <= count) {
> -end -= 4; // make sure there are 4 bytes to read at
> start
> -while (start < end) {
> +// make sure there are 4 bytes to read at start
> +end -= 3;
> +while ((start < end) && (start < count-3)) {
>  off_t xhid  = SH(buf, start);
>  off_t xdlen = SH(buf, start + 2);
>
>

Re: Forcing initialization of string concat INDY expressions

2017-03-01 Thread Aleksey Shipilev

Hi,

On 03/01/2017 07:36 AM, David Holmes wrote:
> The INDY-fication of string concatenation has triggered a problem where a JVM 
> TI
> agent's monitor-wait/ed callback hits an error path that uses string concat
> which triggers a mass of indy related initialization, which in turn hits 
> monitor
> use in MethodType$ConcurrentWeakInternSet.get, which causes the VM monitor
> subsystem to be re-entered (and it is not reentrant!) so we crash. (log 
> extract
> below - the amount of code to process this is truly scary!)

Ouch. This is an unexpected circularity. It is unusual to see Java thread to do
Java stuff when doing Object.wait.

> I assume I can cover the exact case above by replicating it? But can I
> generalize it to cover arbitrary string concat expressions that might arise on
> other error paths?

Yes, the StringConcatFactory code is deliberately lazy. If you want to link
eagerly, you would need to match the concat shape exactly (number and type of
arguments) -- probably by using the same concat code the test failed on. We can
probably eagerly link some popular concat shapes early during system init, but
that requires JDK changes.

Looking at stack trace, it seems to be generic failure when adding new
MethodType to the MethodType cache. This is not the first time that cache
participates in circularities (I wonder if it *always* does). But I am puzzled
how does this happen:

...
V  [jvm.dll+0x2b5412]  Runtime1::monitorenter+0x1e2;;
?monitorenter@Runtime1@@CAXPAVJavaThread@@PAVoopDesc@@PAVBasicObjectLock@@@Z+0x1e2
v  ~RuntimeStub::monitorenter_nofpu Runtime1 stub
J 192 c1
java.lang.invoke.MethodType$ConcurrentWeakInternSet.get(Ljava/lang/Object;)Ljava/lang/Object;
java.base@9-internal (54 bytes) @ 0x0242d7da [0x0242d4c0+0x031a]
...

It calls RuntimeStub::monitorenter_nofpu destructor? Why it ends up calling into
monitorenter? There are no locks in j.l.i.MT$CWIS.get (I checked the bytecode 
too).

If you want a nuclear option for your test, you may want to pass
-XDstringConcat:inline to javac to disable indy string concat to bypass that
circularity completely.

Thanks,
-Aleksey

RE: RFR [XS] : 8175000 : jexec fails to execute simple helloworld.jar

2017-03-01 Thread Baesken, Matthias

Ping ...

Can I get a review please for the change ?

Thanks, Matthias

From: Baesken, Matthias
Sent: Donnerstag, 23. Februar 2017 12:28
To: 'core-libs-dev@openjdk.java.net' 
Cc: Langer, Christoph ; Erik Joelsson 
(erik.joels...@oracle.com) ; 'Michel Trudeau' 

Subject: RE: RFR [XS] : 8175000 : jexec fails to execute simple helloworld.jar

Here is  the webrev for jdk9 :

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

?  And btw I really wonder  - is  jexec still needed in future jdk's like jdk10 
 ? Seems it is not used much .

?

In case  jexec will stay in  the jdk  I might add a test for the tool as well, 
if there is interest  ( could not really find one that tests execution of a 
simple jar-file with jexec).

Best regards, Matthias

From: Baesken, Matthias
Sent: Donnerstag, 23. Februar 2017 07:39
To: 'core-libs-dev@openjdk.java.net' 
>
Cc: Langer, Christoph 
>; Erik Joelsson 
(erik.joels...@oracle.com) 
>
Subject: RE: RFR [XS] : 8175000 : jexec fails to execute simple helloworld.jar

Hello,  probably I should add the info that the fix is needed  in jdk9 as well 
, not only jdk10 .

Without the fix jdk9/10show this error  when executing a small  example jar 
:

/myjdk9/images/jdk/lib/jexec  /java_test/hellojar/helloworld.jar
invalid file (bad magic number): Exec format error

with the fix :

jdk/lib/jexec/java_test/hellojar/helloworld.jar
Hello world from a jar file

And btw I really wonder  - is  jexec still needed in future jdk's like jdk10  ? 
Seems it is not used much .

Best regards, Matthias

From: Baesken, Matthias
Sent: Mittwoch, 22. Februar 2017 18:16
To: core-libs-dev@openjdk.java.net
Cc: Langer, Christoph 
>; Erik Joelsson 
(erik.joels...@oracle.com) 
>
Subject: RFR [XS] : 8175000 : jexec fails to execute simple helloworld.jar

Hello , when looking into  the jexec build I noticed   that  execution of a 
simple helloworld.jar   with jexec does not work any more.

I did a little patch for this which adjusted the addition done with  CR  
8156478: 3 Buffer overrun defect groups in 
jexec.c
  .

Could I have a review ( just a diff this time is provided because of 
infrastructure issues) for it ?

Thanks, Matthias

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

Diff for jdk10  :

# HG changeset patch
# User mbaesken
# Date 1487782485 -3600
#  Wed Feb 22 17:54:45 2017 +0100
# Node ID 93d55a711f3b1c3f282e6889c24d13f16d4a4548
# Parent  884872263accabd4ab68d005abd4e5393144aa4f
8175000: jexec fails to execute simple helloworld.jar

diff --git a/src/java.base/unix/native/launcher/jexec.c 
b/src/java.base/unix/native/launcher/jexec.c
--- a/src/java.base/unix/native/launcher/jexec.c
+++ b/src/java.base/unix/native/launcher/jexec.c
@@ -331,8 +331,9 @@
 off_t end   = start  + xlen;

 if (end <= count) {
-end -= 4; // make sure there are 4 bytes to read at start
-while (start < end) {
+// make sure there are 4 bytes to read at start
+end -= 3;
+while ((start < end) && (start < count-3)) {
 off_t xhid  = SH(buf, start);
 off_t xdlen = SH(buf, start + 2);

RE: [NEW BUG]: Avoid allocations in String.replace(CharSequence, CharSequence) in case no replacement happened

RE: RFR [XS] : 8175000 : jexec fails to execute simple helloworld.jar

Re: [NEW BUG]: Avoid allocations in String.replace(CharSequence, CharSequence) in case no replacement happened

Re: [NEW BUG]: Avoid allocations in String.replace(CharSequence, CharSequence) in case no replacement happened

RE: [NEW BUG]: Avoid allocations in String.replace(CharSequence, CharSequence) in case no replacement happened

Re: [NEW BUG]: Avoid allocations in String.replace(CharSequence, CharSequence) in case no replacement happened

Re: Proposal: java.lang.reflect.Proxy and default methods

Re: RFR [XS] : 8175000 : jexec fails to execute simple helloworld.jar

[NEW BUG]: Avoid allocations in String.replace(CharSequence, CharSequence) in case no replacement happened

Re: Proposal: java.lang.reflect.Proxy and default methods

Re: Proposal: java.lang.reflect.Proxy and default methods

Re: Forcing initialization of string concat INDY expressions

Re: RFR [XS] : 8175000 : jexec fails to execute simple helloworld.jar

Re: RFR [XS] : 8175000 : jexec fails to execute simple helloworld.jar

Re: [10] RFR: 8176041: Optimize handling of comment lines in Properties$LineReader.readLine

Re: [10] RFR: 8176041: Optimize handling of comment lines in Properties$LineReader.readLine

Re: RFR [XS] : 8175000 : jexec fails to execute simple helloworld.jar

Re: [10] RFR: 8176041: Optimize handling of comment lines in Properties$LineReader.readLine

RE: RFR [XS] : 8175000 : jexec fails to execute simple helloworld.jar

Re: [10] RFR: 8176041: Optimize handling of comment lines in Properties$LineReader.readLine

[10] RFR: 8176041: Optimize handling of comment lines in Properties$LineReader.readLine

Re: 10 RFR: 8150687: typedefs without type names

Re: Forcing initialization of string concat INDY expressions

Re: RFR [XS] : 8175000 : jexec fails to execute simple helloworld.jar

Re: Forcing initialization of string concat INDY expressions

RE: RFR [XS] : 8175000 : jexec fails to execute simple helloworld.jar

26 matches

Site Navigation

Mail list logo

Footer information