Re: RFR: 8229516: Thread.isInterrupted() always returns false after thread termination

2019-10-31 Thread David Holmes 

Thanks Doug! Appreciate the help getting the Graal bits correct.

David

On 31/10/2019 5:47 pm, Doug Simon wrote:



On 31 Oct 2019, at 05:30, David Holmes > wrote:


Hi Doug,

On 30/10/2019 10:06 pm, Doug Simon wrote:
On 30 Oct 2019, at 05:28, David Holmes > wrote:


Hi Doug,

Your proposed patch wasn't quite right. I made some adjustments but 
I'm still having issues with the test, 
HotSpotMethodSubstitutionTest.testThreadSubstitutions, as I don't 
see how to make the test execution conditional on the VM config.

Like this:


Thanks for that! One alteration below ...

diff --git 
a/compiler/src/org.graalvm.compiler.hotspot.test/src/org/graalvm/compiler/hotspot/test/HotSpotMethodSubstitutionTest.java 
b/compiler/src/org.graalvm.compiler.hotspot.test/src/org/graalvm/compiler/hotspot/test/HotSpotMethodSubstitutionTest.java

index 96e7d979d36..3c928b86961 100644
--- 
a/compiler/src/org.graalvm.compiler.hotspot.test/src/org/graalvm/compiler/hotspot/test/HotSpotMethodSubstitutionTest.java
+++ 
b/compiler/src/org.graalvm.compiler.hotspot.test/src/org/graalvm/compiler/hotspot/test/HotSpotMethodSubstitutionTest.java

@@ -28,12 +28,13 @@ import java.lang.invoke.ConstantCallSite;
 import java.lang.invoke.MethodHandles;
 import java.lang.invoke.MethodType;
-import org.graalvm.compiler.nodes.IfNode;
-import org.junit.Test;
-
 import org.graalvm.compiler.api.directives.GraalDirectives;
 import org.graalvm.compiler.api.replacements.MethodSubstitution;
+import org.graalvm.compiler.hotspot.GraalHotSpotVMConfig;
+import org.graalvm.compiler.hotspot.HotSpotBackend;
+import org.graalvm.compiler.nodes.IfNode;
 import org.graalvm.compiler.replacements.test.MethodSubstitutionTest;
+import org.junit.Test;
 /**
  * Tests HotSpot specific {@link MethodSubstitution}s.
@@ -133,13 +134,18 @@ public class HotSpotMethodSubstitutionTest 
extends MethodSubstitutionTest {

 @Test
 public void testThreadSubstitutions() {
+    GraalHotSpotVMConfig config = ((HotSpotBackend) 
getBackend()).getRuntime().getVMConfig();

 testGraph("currentThread");
-    assertInGraph(testGraph("threadIsInterrupted", 
"isInterrupted", true), IfNode.class);
-    assertInGraph(testGraph("threadInterrupted", 
"isInterrupted", true), IfNode.class);

+    if (config.osThreadOffset != Integer.MAX_VALUE) {


s/osThreadOffset/osThreadInterruptedOffset/


Good catch. Sorry about that.


This change removes the interrupted field from osThread but not 
osThread itself. Though as osThread is only used to then access the 
interrupted field, I could both the same. Here's updated webrev 
showing that:


http://cr.openjdk.java.net/~dholmes/8229516/webrev.v2/


Looks good.

-Doug

Re: RFR: 8229516: Thread.isInterrupted() always returns false after thread termination

2019-10-31 Thread Doug Simon 



> On 31 Oct 2019, at 05:30, David Holmes  wrote:
> 
> Hi Doug,
> 
> On 30/10/2019 10:06 pm, Doug Simon wrote:
>>> On 30 Oct 2019, at 05:28, David Holmes >> > wrote:
>>> 
>>> Hi Doug,
>>> 
>>> Your proposed patch wasn't quite right. I made some adjustments but I'm 
>>> still having issues with the test, 
>>> HotSpotMethodSubstitutionTest.testThreadSubstitutions, as I don't see how 
>>> to make the test execution conditional on the VM config.
>> Like this:
> 
> Thanks for that! One alteration below ...
> 
>> diff --git 
>> a/compiler/src/org.graalvm.compiler.hotspot.test/src/org/graalvm/compiler/hotspot/test/HotSpotMethodSubstitutionTest.java
>>  
>> b/compiler/src/org.graalvm.compiler.hotspot.test/src/org/graalvm/compiler/hotspot/test/HotSpotMethodSubstitutionTest.java
>> index 96e7d979d36..3c928b86961 100644
>> --- 
>> a/compiler/src/org.graalvm.compiler.hotspot.test/src/org/graalvm/compiler/hotspot/test/HotSpotMethodSubstitutionTest.java
>> +++ 
>> b/compiler/src/org.graalvm.compiler.hotspot.test/src/org/graalvm/compiler/hotspot/test/HotSpotMethodSubstitutionTest.java
>> @@ -28,12 +28,13 @@ import java.lang.invoke.ConstantCallSite;
>>  import java.lang.invoke.MethodHandles;
>>  import java.lang.invoke.MethodType;
>> -import org.graalvm.compiler.nodes.IfNode;
>> -import org.junit.Test;
>> -
>>  import org.graalvm.compiler.api.directives.GraalDirectives;
>>  import org.graalvm.compiler.api.replacements.MethodSubstitution;
>> +import org.graalvm.compiler.hotspot.GraalHotSpotVMConfig;
>> +import org.graalvm.compiler.hotspot.HotSpotBackend;
>> +import org.graalvm.compiler.nodes.IfNode;
>>  import org.graalvm.compiler.replacements.test.MethodSubstitutionTest;
>> +import org.junit.Test;
>>  /**
>>   * Tests HotSpot specific {@link MethodSubstitution}s.
>> @@ -133,13 +134,18 @@ public class HotSpotMethodSubstitutionTest extends 
>> MethodSubstitutionTest {
>>  @Test
>>  public void testThreadSubstitutions() {
>> +GraalHotSpotVMConfig config = ((HotSpotBackend) 
>> getBackend()).getRuntime().getVMConfig();
>>  testGraph("currentThread");
>> -assertInGraph(testGraph("threadIsInterrupted", "isInterrupted", 
>> true), IfNode.class);
>> -assertInGraph(testGraph("threadInterrupted", "isInterrupted", 
>> true), IfNode.class);
>> +if (config.osThreadOffset != Integer.MAX_VALUE) {
> 
> s/osThreadOffset/osThreadInterruptedOffset/

Good catch. Sorry about that.
> 
> This change removes the interrupted field from osThread but not osThread 
> itself. Though as osThread is only used to then access the interrupted field, 
> I could both the same. Here's updated webrev showing that:
> 
> http://cr.openjdk.java.net/~dholmes/8229516/webrev.v2/ 
> 

Looks good.

-Doug

Re: RFR: 8229516: Thread.isInterrupted() always returns false after thread termination

2019-10-30 Thread David Holmes 

Hi Doug,

On 30/10/2019 10:06 pm, Doug Simon wrote:




On 30 Oct 2019, at 05:28, David Holmes  wrote:

Hi Doug,

Your proposed patch wasn't quite right. I made some adjustments but I'm still 
having issues with the test, 
HotSpotMethodSubstitutionTest.testThreadSubstitutions, as I don't see how to 
make the test execution conditional on the VM config.


Like this:


Thanks for that! One alteration below ...


diff --git 
a/compiler/src/org.graalvm.compiler.hotspot.test/src/org/graalvm/compiler/hotspot/test/HotSpotMethodSubstitutionTest.java
 
b/compiler/src/org.graalvm.compiler.hotspot.test/src/org/graalvm/compiler/hotspot/test/HotSpotMethodSubstitutionTest.java
index 96e7d979d36..3c928b86961 100644
--- 
a/compiler/src/org.graalvm.compiler.hotspot.test/src/org/graalvm/compiler/hotspot/test/HotSpotMethodSubstitutionTest.java
+++ 
b/compiler/src/org.graalvm.compiler.hotspot.test/src/org/graalvm/compiler/hotspot/test/HotSpotMethodSubstitutionTest.java
@@ -28,12 +28,13 @@ import java.lang.invoke.ConstantCallSite;
  import java.lang.invoke.MethodHandles;
  import java.lang.invoke.MethodType;

-import org.graalvm.compiler.nodes.IfNode;
-import org.junit.Test;
-
  import org.graalvm.compiler.api.directives.GraalDirectives;
  import org.graalvm.compiler.api.replacements.MethodSubstitution;
+import org.graalvm.compiler.hotspot.GraalHotSpotVMConfig;
+import org.graalvm.compiler.hotspot.HotSpotBackend;
+import org.graalvm.compiler.nodes.IfNode;
  import org.graalvm.compiler.replacements.test.MethodSubstitutionTest;
+import org.junit.Test;

  /**
   * Tests HotSpot specific {@link MethodSubstitution}s.
@@ -133,13 +134,18 @@ public class HotSpotMethodSubstitutionTest extends 
MethodSubstitutionTest {

  @Test
  public void testThreadSubstitutions() {
+GraalHotSpotVMConfig config = ((HotSpotBackend) 
getBackend()).getRuntime().getVMConfig();
  testGraph("currentThread");
-assertInGraph(testGraph("threadIsInterrupted", "isInterrupted", true), 
IfNode.class);
-assertInGraph(testGraph("threadInterrupted", "isInterrupted", true), 
IfNode.class);
+if (config.osThreadOffset != Integer.MAX_VALUE) {


s/osThreadOffset/osThreadInterruptedOffset/

This change removes the interrupted field from osThread but not osThread 
itself. Though as osThread is only used to then access the interrupted 
field, I could both the same. Here's updated webrev showing that:


http://cr.openjdk.java.net/~dholmes/8229516/webrev.v2/

All tests under compiler/graalunit now pass.

Thanks,
David
-


+assertInGraph(testGraph("threadIsInterrupted", "isInterrupted", 
true), IfNode.class);
+assertInGraph(testGraph("threadInterrupted", "isInterrupted", 
true), IfNode.class);
+}

  Thread currentThread = Thread.currentThread();
  test("currentThread", currentThread);
-test("threadIsInterrupted", currentThread);
+if (config.osThreadOffset != Integer.MAX_VALUE) {
+test("threadIsInterrupted", currentThread);
+}
  }

  @SuppressWarnings("all")



Updated webrev:

http://cr.openjdk.java.net/~dholmes/8229516/webrev.v2/

Thanks,
David
-


On 29/10/2019 7:14 pm, Doug Simon wrote:

On 29 Oct 2019, at 10:12, David Holmes  wrote:

Hi Doug,

Thanks for taking a look so quickly! :)

On 29/10/2019 6:55 pm, Doug Simon wrote:

Hi David,
Since Graal needs to work against multiple JVMCI versions (and VM versions!), 
the Graal changes should only disable the intrinsic when the relevant VM config 
is missing:


So to be clear I should revert all the Graal file changes I made and just apply 
the patch below?

Yes please.
-Doug



diff --git 
a/compiler/src/org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/GraalHotSpotVMConfig.java
 
b/compiler/src/org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/GraalHotSpotVMConfig.java
index 0752a621215..baa2136a6ba 100644
--- 
a/compiler/src/org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/GraalHotSpotVMConfig.java
+++ 
b/compiler/src/org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/GraalHotSpotVMConfig.java
@@ -315,7 +315,7 @@ public class GraalHotSpotVMConfig extends 
GraalHotSpotVMConfigBase {
  public final int javaThreadAnchorOffset = getFieldOffset("JavaThread::_anchor", 
Integer.class, "JavaFrameAnchor");
  public final int javaThreadShouldPostOnExceptionsFlagOffset = 
getFieldOffset("JavaThread::_should_post_on_exceptions_flag", Integer.class, 
"int", Integer.MIN_VALUE);
  public final int threadObjectOffset = getFieldOffset("JavaThread::_threadObj", 
Integer.class, "oop");
-public final int osThreadOffset = getFieldOffset("JavaThread::_osthread", 
Integer.class, "OSThread*");
+public final int osThreadOffset = getFieldOffset("JavaThread::_osthread", 
Integer.class, "OSThread*", Integer.MAX_VALUE);
  public final int threadIsMethodHandleReturnOffset = 
getFieldOffset("JavaThread::_is_method_handle_return",

Re: RFR: 8229516: Thread.isInterrupted() always returns false after thread termination

2019-10-30 Thread David Holmes 

Thanks Serguei!

David

On 31/10/2019 10:36 am, serguei.spit...@oracle.com wrote:

Hi David,

The update looks good.

Thanks,
Serguei

On 10/29/19 9:28 PM, David Holmes wrote:

Hi Doug,

Your proposed patch wasn't quite right. I made some adjustments but 
I'm still having issues with the test, 
HotSpotMethodSubstitutionTest.testThreadSubstitutions, as I don't see 
how to make the test execution conditional on the VM config.


Updated webrev:

http://cr.openjdk.java.net/~dholmes/8229516/webrev.v2/

Thanks,
David
-


On 29/10/2019 7:14 pm, Doug Simon wrote:




On 29 Oct 2019, at 10:12, David Holmes  wrote:

Hi Doug,

Thanks for taking a look so quickly! :)

On 29/10/2019 6:55 pm, Doug Simon wrote:

Hi David,
Since Graal needs to work against multiple JVMCI versions (and VM 
versions!), the Graal changes should only disable the intrinsic 
when the relevant VM config is missing:


So to be clear I should revert all the Graal file changes I made and 
just apply the patch below?


Yes please.

-Doug



diff --git 
a/compiler/src/org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/GraalHotSpotVMConfig.java 
b/compiler/src/org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/GraalHotSpotVMConfig.java 


index 0752a621215..baa2136a6ba 100644
--- 
a/compiler/src/org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/GraalHotSpotVMConfig.java 

+++ 
b/compiler/src/org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/GraalHotSpotVMConfig.java 

@@ -315,7 +315,7 @@ public class GraalHotSpotVMConfig extends 
GraalHotSpotVMConfigBase {
  public final int javaThreadAnchorOffset = 
getFieldOffset("JavaThread::_anchor", Integer.class, 
"JavaFrameAnchor");
  public final int javaThreadShouldPostOnExceptionsFlagOffset = 
getFieldOffset("JavaThread::_should_post_on_exceptions_flag", 
Integer.class, "int", Integer.MIN_VALUE);
  public final int threadObjectOffset = 
getFieldOffset("JavaThread::_threadObj", Integer.class, "oop");
-    public final int osThreadOffset = 
getFieldOffset("JavaThread::_osthread", Integer.class, "OSThread*");
+    public final int osThreadOffset = 
getFieldOffset("JavaThread::_osthread", Integer.class, "OSThread*", 
Integer.MAX_VALUE);
  public final int threadIsMethodHandleReturnOffset = 
getFieldOffset("JavaThread::_is_method_handle_return", 
Integer.class, "int");
  public final int threadObjectResultOffset = 
getFieldOffset("JavaThread::_vm_result", Integer.class, "oop");
  public final int jvmciCountersThreadOffset = 
getFieldOffset("JavaThread::_jvmci_counters", Integer.class, 
"jlong*");
diff --git 
a/compiler/src/org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/meta/HotSpotGraphBuilderPlugins.java 
b/compiler/src/org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/meta/HotSpotGraphBuilderPlugins.java 


index 6b37fff083d..ffc8032d2b0 100644
--- 
a/compiler/src/org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/meta/HotSpotGraphBuilderPlugins.java 

+++ 
b/compiler/src/org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/meta/HotSpotGraphBuilderPlugins.java 


@@ -441,7 +441,9 @@ public class HotSpotGraphBuilderPlugins {
  }
  });
  - r.registerMethodSubstitution(ThreadSubstitutions.class, 
"isInterrupted", Receiver.class, boolean.class);

+    if (config.osThreadOffset != Integer.MAX_VALUE) {
+ r.registerMethodSubstitution(ThreadSubstitutions.class, 
"isInterrupted", Receiver.class, boolean.class);

+    }
  }
    public static final String reflectionClass;
diff --git 
a/compiler/src/org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/replacements/HotSpotReplacementsUtil.java 
b/compiler/src/org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/replacements/HotSpotReplacementsUtil.java 


index 1d694fae2a4..8500c4de115 100644
--- 
a/compiler/src/org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/replacements/HotSpotReplacementsUtil.java 

+++ 
b/compiler/src/org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/replacements/HotSpotReplacementsUtil.java 


@@ -308,6 +308,7 @@ public class HotSpotReplacementsUtil {
    @Fold
  public static int osThreadOffset(@InjectedParameter 
GraalHotSpotVMConfig config) {
+    assert config.instanceKlassInitThreadOffset != 
Integer.MAX_VALUE;

  return config.osThreadOffset;
  }
  All the other JVMCI changes look good to me.
-Doug
On 29 Oct 2019, at 08:42, David Holmes  
wrote:


Bug: https://bugs.openjdk.java.net/browse/JDK-8229516
CSR: https://bugs.openjdk.java.net/browse/JDK-8232676 (already 
approved)

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

This cross-cuts core-libs, hotspot-runtime and the JIT compilers, 
but only in small pieces each. There is also a small touch to 
serviceability code.


This change is "simply" moving the "interrupted" field out of the 
osThread and into the java.lang.Thread so that it can be set 
independently

Re: RFR: 8229516: Thread.isInterrupted() always returns false after thread termination

2019-10-30 Thread serguei . spitsyn 

Hi David,

The update looks good.

Thanks,
Serguei

On 10/29/19 9:28 PM, David Holmes wrote:

Hi Doug,

Your proposed patch wasn't quite right. I made some adjustments but 
I'm still having issues with the test, 
HotSpotMethodSubstitutionTest.testThreadSubstitutions, as I don't see 
how to make the test execution conditional on the VM config.


Updated webrev:

http://cr.openjdk.java.net/~dholmes/8229516/webrev.v2/

Thanks,
David
-


On 29/10/2019 7:14 pm, Doug Simon wrote:




On 29 Oct 2019, at 10:12, David Holmes  wrote:

Hi Doug,

Thanks for taking a look so quickly! :)

On 29/10/2019 6:55 pm, Doug Simon wrote:

Hi David,
Since Graal needs to work against multiple JVMCI versions (and VM 
versions!), the Graal changes should only disable the intrinsic 
when the relevant VM config is missing:


So to be clear I should revert all the Graal file changes I made and 
just apply the patch below?


Yes please.

-Doug



diff --git 
a/compiler/src/org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/GraalHotSpotVMConfig.java 
b/compiler/src/org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/GraalHotSpotVMConfig.java 


index 0752a621215..baa2136a6ba 100644
--- 
a/compiler/src/org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/GraalHotSpotVMConfig.java
+++ 
b/compiler/src/org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/GraalHotSpotVMConfig.java
@@ -315,7 +315,7 @@ public class GraalHotSpotVMConfig extends 
GraalHotSpotVMConfigBase {
  public final int javaThreadAnchorOffset = 
getFieldOffset("JavaThread::_anchor", Integer.class, 
"JavaFrameAnchor");
  public final int javaThreadShouldPostOnExceptionsFlagOffset = 
getFieldOffset("JavaThread::_should_post_on_exceptions_flag", 
Integer.class, "int", Integer.MIN_VALUE);
  public final int threadObjectOffset = 
getFieldOffset("JavaThread::_threadObj", Integer.class, "oop");
-    public final int osThreadOffset = 
getFieldOffset("JavaThread::_osthread", Integer.class, "OSThread*");
+    public final int osThreadOffset = 
getFieldOffset("JavaThread::_osthread", Integer.class, "OSThread*", 
Integer.MAX_VALUE);
  public final int threadIsMethodHandleReturnOffset = 
getFieldOffset("JavaThread::_is_method_handle_return", 
Integer.class, "int");
  public final int threadObjectResultOffset = 
getFieldOffset("JavaThread::_vm_result", Integer.class, "oop");
  public final int jvmciCountersThreadOffset = 
getFieldOffset("JavaThread::_jvmci_counters", Integer.class, 
"jlong*");
diff --git 
a/compiler/src/org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/meta/HotSpotGraphBuilderPlugins.java 
b/compiler/src/org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/meta/HotSpotGraphBuilderPlugins.java 


index 6b37fff083d..ffc8032d2b0 100644
--- 
a/compiler/src/org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/meta/HotSpotGraphBuilderPlugins.java
+++ 
b/compiler/src/org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/meta/HotSpotGraphBuilderPlugins.java

@@ -441,7 +441,9 @@ public class HotSpotGraphBuilderPlugins {
  }
  });
  - r.registerMethodSubstitution(ThreadSubstitutions.class, 
"isInterrupted", Receiver.class, boolean.class);

+    if (config.osThreadOffset != Integer.MAX_VALUE) {
+ r.registerMethodSubstitution(ThreadSubstitutions.class, 
"isInterrupted", Receiver.class, boolean.class);

+    }
  }
    public static final String reflectionClass;
diff --git 
a/compiler/src/org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/replacements/HotSpotReplacementsUtil.java 
b/compiler/src/org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/replacements/HotSpotReplacementsUtil.java 


index 1d694fae2a4..8500c4de115 100644
--- 
a/compiler/src/org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/replacements/HotSpotReplacementsUtil.java
+++ 
b/compiler/src/org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/replacements/HotSpotReplacementsUtil.java

@@ -308,6 +308,7 @@ public class HotSpotReplacementsUtil {
    @Fold
  public static int osThreadOffset(@InjectedParameter 
GraalHotSpotVMConfig config) {
+    assert config.instanceKlassInitThreadOffset != 
Integer.MAX_VALUE;

  return config.osThreadOffset;
  }
  All the other JVMCI changes look good to me.
-Doug
On 29 Oct 2019, at 08:42, David Holmes  
wrote:


Bug: https://bugs.openjdk.java.net/browse/JDK-8229516
CSR: https://bugs.openjdk.java.net/browse/JDK-8232676 (already 
approved)

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

This cross-cuts core-libs, hotspot-runtime and the JIT compilers, 
but only in small pieces each. There is also a small touch to 
serviceability code.


This change is "simply" moving the "interrupted" field out of the 
osThread and into the java.lang.Thread so that it can be set 
independently of whether the thread is alive (and to make things 
easier for loom in the near future). It is

Re: RFR: 8229516: Thread.isInterrupted() always returns false after thread termination

2019-10-30 Thread David Holmes 

Hi Alan,

Thanks for taking a look at this.

On 31/10/2019 1:33 am, Alan Bateman wrote:

On 29/10/2019 07:42, David Holmes wrote:

Bug: https://bugs.openjdk.java.net/browse/JDK-8229516
CSR: https://bugs.openjdk.java.net/browse/JDK-8232676 (already approved)
webrev: http://cr.openjdk.java.net/~dholmes/8229516/webrev/

The implNote has "In the JDK Reference Implementation". I think you can 
replace that with "In this implementation" so that it is consistent with 
the other impl notes that we've added in recent releases.


Joe Darcy specifically requested that terminology in the CSR request as 
the new way of referring to "this implementation". There are 24 existing 
uses.


In any case, I went through the Thread changes and the call into the VM 
to clear the event on Windows. Looks okay to me (we'll have re-work to 
do in Loom of course but that was expected).


Thanks again.

David


-Alan

Re: RFR: 8229516: Thread.isInterrupted() always returns false after thread termination

2019-10-30 Thread Alan Bateman 

On 29/10/2019 07:42, David Holmes wrote:

Bug: https://bugs.openjdk.java.net/browse/JDK-8229516
CSR: https://bugs.openjdk.java.net/browse/JDK-8232676 (already approved)
webrev: http://cr.openjdk.java.net/~dholmes/8229516/webrev/

The implNote has "In the JDK Reference Implementation". I think you can 
replace that with "In this implementation" so that it is consistent with 
the other impl notes that we've added in recent releases.


In any case, I went through the Thread changes and the call into the VM 
to clear the event on Windows. Looks okay to me (we'll have re-work to 
do in Loom of course but that was expected).


-Alan

Re: RFR: 8229516: Thread.isInterrupted() always returns false after thread termination

2019-10-30 Thread Doug Simon 



> On 30 Oct 2019, at 05:28, David Holmes  wrote:
> 
> Hi Doug,
> 
> Your proposed patch wasn't quite right. I made some adjustments but I'm still 
> having issues with the test, 
> HotSpotMethodSubstitutionTest.testThreadSubstitutions, as I don't see how to 
> make the test execution conditional on the VM config.

Like this:

diff --git 
a/compiler/src/org.graalvm.compiler.hotspot.test/src/org/graalvm/compiler/hotspot/test/HotSpotMethodSubstitutionTest.java
 
b/compiler/src/org.graalvm.compiler.hotspot.test/src/org/graalvm/compiler/hotspot/test/HotSpotMethodSubstitutionTest.java
index 96e7d979d36..3c928b86961 100644
--- 
a/compiler/src/org.graalvm.compiler.hotspot.test/src/org/graalvm/compiler/hotspot/test/HotSpotMethodSubstitutionTest.java
+++ 
b/compiler/src/org.graalvm.compiler.hotspot.test/src/org/graalvm/compiler/hotspot/test/HotSpotMethodSubstitutionTest.java
@@ -28,12 +28,13 @@ import java.lang.invoke.ConstantCallSite;
 import java.lang.invoke.MethodHandles;
 import java.lang.invoke.MethodType;

-import org.graalvm.compiler.nodes.IfNode;
-import org.junit.Test;
-
 import org.graalvm.compiler.api.directives.GraalDirectives;
 import org.graalvm.compiler.api.replacements.MethodSubstitution;
+import org.graalvm.compiler.hotspot.GraalHotSpotVMConfig;
+import org.graalvm.compiler.hotspot.HotSpotBackend;
+import org.graalvm.compiler.nodes.IfNode;
 import org.graalvm.compiler.replacements.test.MethodSubstitutionTest;
+import org.junit.Test;

 /**
  * Tests HotSpot specific {@link MethodSubstitution}s.
@@ -133,13 +134,18 @@ public class HotSpotMethodSubstitutionTest extends 
MethodSubstitutionTest {

 @Test
 public void testThreadSubstitutions() {
+GraalHotSpotVMConfig config = ((HotSpotBackend) 
getBackend()).getRuntime().getVMConfig();
 testGraph("currentThread");
-assertInGraph(testGraph("threadIsInterrupted", "isInterrupted", true), 
IfNode.class);
-assertInGraph(testGraph("threadInterrupted", "isInterrupted", true), 
IfNode.class);
+if (config.osThreadOffset != Integer.MAX_VALUE) {
+assertInGraph(testGraph("threadIsInterrupted", "isInterrupted", 
true), IfNode.class);
+assertInGraph(testGraph("threadInterrupted", "isInterrupted", 
true), IfNode.class);
+}

 Thread currentThread = Thread.currentThread();
 test("currentThread", currentThread);
-test("threadIsInterrupted", currentThread);
+if (config.osThreadOffset != Integer.MAX_VALUE) {
+test("threadIsInterrupted", currentThread);
+}
 }

 @SuppressWarnings("all")

> 
> Updated webrev:
> 
> http://cr.openjdk.java.net/~dholmes/8229516/webrev.v2/
> 
> Thanks,
> David
> -
> 
> 
> On 29/10/2019 7:14 pm, Doug Simon wrote:
>>> On 29 Oct 2019, at 10:12, David Holmes  wrote:
>>> 
>>> Hi Doug,
>>> 
>>> Thanks for taking a look so quickly! :)
>>> 
>>> On 29/10/2019 6:55 pm, Doug Simon wrote:
 Hi David,
 Since Graal needs to work against multiple JVMCI versions (and VM 
 versions!), the Graal changes should only disable the intrinsic when the 
 relevant VM config is missing:
>>> 
>>> So to be clear I should revert all the Graal file changes I made and just 
>>> apply the patch below?
>> Yes please.
>> -Doug
>>> 
 diff --git 
 a/compiler/src/org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/GraalHotSpotVMConfig.java
  
 b/compiler/src/org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/GraalHotSpotVMConfig.java
 index 0752a621215..baa2136a6ba 100644
 --- 
 a/compiler/src/org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/GraalHotSpotVMConfig.java
 +++ 
 b/compiler/src/org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/GraalHotSpotVMConfig.java
 @@ -315,7 +315,7 @@ public class GraalHotSpotVMConfig extends 
 GraalHotSpotVMConfigBase {
  public final int javaThreadAnchorOffset = 
 getFieldOffset("JavaThread::_anchor", Integer.class, "JavaFrameAnchor");
  public final int javaThreadShouldPostOnExceptionsFlagOffset = 
 getFieldOffset("JavaThread::_should_post_on_exceptions_flag", 
 Integer.class, "int", Integer.MIN_VALUE);
  public final int threadObjectOffset = 
 getFieldOffset("JavaThread::_threadObj", Integer.class, "oop");
 -public final int osThreadOffset = 
 getFieldOffset("JavaThread::_osthread", Integer.class, "OSThread*");
 +public final int osThreadOffset = 
 getFieldOffset("JavaThread::_osthread", Integer.class, "OSThread*", 
 Integer.MAX_VALUE);
  public final int threadIsMethodHandleReturnOffset = 
 getFieldOffset("JavaThread::_is_method_handle_return", Integer.class, 
 "int");
  public final int threadObjectResultOffset = 
 getFieldOffset("JavaThread::_vm_result", Integer.class, "oop");
  public final int jvmciCountersThreadOffset = 
 getFieldOffset("JavaThread::_jvmci_counters", Integer.class,

Re: RFR: 8229516: Thread.isInterrupted() always returns false after thread termination

2019-10-29 Thread David Holmes 

Hi Doug,

Your proposed patch wasn't quite right. I made some adjustments but I'm 
still having issues with the test, 
HotSpotMethodSubstitutionTest.testThreadSubstitutions, as I don't see 
how to make the test execution conditional on the VM config.


Updated webrev:

http://cr.openjdk.java.net/~dholmes/8229516/webrev.v2/

Thanks,
David
-


On 29/10/2019 7:14 pm, Doug Simon wrote:




On 29 Oct 2019, at 10:12, David Holmes  wrote:

Hi Doug,

Thanks for taking a look so quickly! :)

On 29/10/2019 6:55 pm, Doug Simon wrote:

Hi David,
Since Graal needs to work against multiple JVMCI versions (and VM versions!), 
the Graal changes should only disable the intrinsic when the relevant VM config 
is missing:


So to be clear I should revert all the Graal file changes I made and just apply 
the patch below?


Yes please.

-Doug




diff --git 
a/compiler/src/org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/GraalHotSpotVMConfig.java
 
b/compiler/src/org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/GraalHotSpotVMConfig.java
index 0752a621215..baa2136a6ba 100644
--- 
a/compiler/src/org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/GraalHotSpotVMConfig.java
+++ 
b/compiler/src/org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/GraalHotSpotVMConfig.java
@@ -315,7 +315,7 @@ public class GraalHotSpotVMConfig extends 
GraalHotSpotVMConfigBase {
  public final int javaThreadAnchorOffset = getFieldOffset("JavaThread::_anchor", 
Integer.class, "JavaFrameAnchor");
  public final int javaThreadShouldPostOnExceptionsFlagOffset = 
getFieldOffset("JavaThread::_should_post_on_exceptions_flag", Integer.class, 
"int", Integer.MIN_VALUE);
  public final int threadObjectOffset = getFieldOffset("JavaThread::_threadObj", 
Integer.class, "oop");
-public final int osThreadOffset = getFieldOffset("JavaThread::_osthread", 
Integer.class, "OSThread*");
+public final int osThreadOffset = getFieldOffset("JavaThread::_osthread", 
Integer.class, "OSThread*", Integer.MAX_VALUE);
  public final int threadIsMethodHandleReturnOffset = 
getFieldOffset("JavaThread::_is_method_handle_return", Integer.class, "int");
  public final int threadObjectResultOffset = 
getFieldOffset("JavaThread::_vm_result", Integer.class, "oop");
  public final int jvmciCountersThreadOffset = 
getFieldOffset("JavaThread::_jvmci_counters", Integer.class, "jlong*");
diff --git 
a/compiler/src/org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/meta/HotSpotGraphBuilderPlugins.java
 
b/compiler/src/org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/meta/HotSpotGraphBuilderPlugins.java
index 6b37fff083d..ffc8032d2b0 100644
--- 
a/compiler/src/org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/meta/HotSpotGraphBuilderPlugins.java
+++ 
b/compiler/src/org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/meta/HotSpotGraphBuilderPlugins.java
@@ -441,7 +441,9 @@ public class HotSpotGraphBuilderPlugins {
  }
  });
  -r.registerMethodSubstitution(ThreadSubstitutions.class, 
"isInterrupted", Receiver.class, boolean.class);
+if (config.osThreadOffset != Integer.MAX_VALUE) {
+r.registerMethodSubstitution(ThreadSubstitutions.class, 
"isInterrupted", Receiver.class, boolean.class);
+}
  }
public static final String reflectionClass;
diff --git 
a/compiler/src/org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/replacements/HotSpotReplacementsUtil.java
 
b/compiler/src/org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/replacements/HotSpotReplacementsUtil.java
index 1d694fae2a4..8500c4de115 100644
--- 
a/compiler/src/org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/replacements/HotSpotReplacementsUtil.java
+++ 
b/compiler/src/org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/replacements/HotSpotReplacementsUtil.java
@@ -308,6 +308,7 @@ public class HotSpotReplacementsUtil {
@Fold
  public static int osThreadOffset(@InjectedParameter GraalHotSpotVMConfig 
config) {
+assert config.instanceKlassInitThreadOffset != Integer.MAX_VALUE;
  return config.osThreadOffset;
  }
  All the other JVMCI changes look good to me.
-Doug

On 29 Oct 2019, at 08:42, David Holmes  wrote:

Bug: https://bugs.openjdk.java.net/browse/JDK-8229516
CSR: https://bugs.openjdk.java.net/browse/JDK-8232676 (already approved)
webrev: http://cr.openjdk.java.net/~dholmes/8229516/webrev/

This cross-cuts core-libs, hotspot-runtime and the JIT compilers, but only in 
small pieces each. There is also a small touch to serviceability code.

This change is "simply" moving the "interrupted" field out of the osThread and 
into the java.lang.Thread so that it can be set independently of whether the thread is alive (and 
to make things easier for loom in the near future). It is very straightforward:

- old scheme
  - interrupted field is in osThread
  - VM can read/write

Re: RFR: 8229516: Thread.isInterrupted() always returns false after thread termination

2019-10-29 Thread David Holmes 

Hi Remi,

On 30/10/2019 7:38 am, Remi Forax wrote:

Hi David,
in java.lang.Thread interrupt0 should be renamed to setInterruptEvent, because 
what it does now


There is only an "interrupt event" on Windows. What interrupt0 does is 
issue all the necessary unparks so that blocked threads will wake up and 
recheck if they were interrupted. So I don't think a rename is needed.



and I don't really understand the comment in interrupted(), if a thread is 
interrupted by two other threads calling interrupt(), you will loose an 
interrupt anyway.


Suppose a Thread is checking interrupted() ahead of a blocking 
operation, it reads the interrupted field and sees that it is false. 
Then another thread interrupts it, setting the field to true and issuing 
the unparks. The interrupted() call continues and sets the interrupted 
field to false, and then returns false so the blocking operation 
proceeds. Because of the unparks the blocking call returns immediately. 
The thread then checks again to see if it was interrupted, but the field 
is false and so it re-parks (treating this as a spurious wakeup). But 
the thread actually was interrupted and should have thrown 
InterruptedException. The interrupt has been lost.


By only clearing the interrupted field if it was set we avoid this 
problem. This is the logic the VM code has been implementing for 20 
years, so the same logic is needed in the Java code. I added the 
additional comment to the VM code to make it clearer.


Hope that clarifies things.

Thanks,
David
-


Rémi

- Mail original -

De: "David Holmes" 
À: "hotspot-runtime-dev" , "hotspot 
compiler"
, "core-libs-dev" 
, "Doug Simon"
, "TOM.RODRIGUEZ" , 
"serviceability-dev"
, "Roger Riggs" 
Envoyé: Mardi 29 Octobre 2019 08:42:08
Objet: RFR: 8229516: Thread.isInterrupted() always returns false after thread 
termination



Bug: https://bugs.openjdk.java.net/browse/JDK-8229516
CSR: https://bugs.openjdk.java.net/browse/JDK-8232676 (already approved)
webrev: http://cr.openjdk.java.net/~dholmes/8229516/webrev/

This cross-cuts core-libs, hotspot-runtime and the JIT compilers, but
only in small pieces each. There is also a small touch to serviceability
code.

This change is "simply" moving the "interrupted" field out of the
osThread and into the java.lang.Thread so that it can be set
independently of whether the thread is alive (and to make things easier
for loom in the near future). It is very straightforward:

- old scheme
   - interrupted field is in osThread
   - VM can read/write directly
   - Java code calls into VM to read/write

- new scheme
   - interrupted field is in java.lang.Thread
   - VM has to use javaCalls to read/write "directly"
   - Java code can read/write directly

No changes to any of the semantics regarding the actual interrupt
mechanism. Special thanks to Patricio for tracking down a bug I had
introduced in that regard!

Special Note (Hi Roger!): on windows we still need to set/clear the
_interrupt_event used by the Process.waitFor logic. To facilitate
clearing via Thread.interrupted() I had to introduce a native method
that is a no-op except on Windows. This seemed the cheapest and least
intrusive means to achieve this.

Other changes revolve around the fact we used to have an intrinsic for
Thread.isInterrupted and that is not needed any more. So we strip some
code out of C1/C2.

The changes in the JVMCI/Graal code are a bit more far reaching as
entire classes disappear. I've cc'd Doug and Tom at Vladimir's request
so that they can comment on the JVMCI changes and whether I have gone
too far or not far enough. There are a bunch of tests for interruption
in JVMCI that could potentially be deleted if they are only intended to
test the JVMCI handling of interrupt:

./jdk.internal.vm.compiler/share/classes/org.graalvm.compiler.jtt/src/org/graalvm/compiler/jtt/threads/Thread_isInterrupted*.java

Testing:
  - Tiers 1-3 on all Oracle platforms
  - Focused testing on Linux x64:
 - Stress runs of JSR166TestCase
 - Anything that seems to use interrupt():
   - JDK
 - java/lang/Thread
 - java/util/concurrent
 - jdk/internal/loader/InterruptedClassLoad.java
 - javax/management
 - java/nio/file/Files
 - java/nio/channels
 - java/net/Socket/Timeouts.java
 - java/lang/Runtime/shutdown/
 - java/lang/ProcessBuilder/Basic.java
 - com/sun/jdi/
   - Hotspot
 - vmTestbase/nsk/monitoring/
 - vmTestbase/nsk/jdwp
 - vmTestbase/nsk/jdb/
 - vmTestbase/nsk/jdi/
 - vmTestbase/nsk/jvmti/
 - runtime/Thread
 - serviceability/jvmti/
 - serviceability/jdwp
 - serviceability/sa

Thanks,
David
-

Re: RFR: 8229516: Thread.isInterrupted() always returns false after thread termination

2019-10-29 Thread David Holmes 

Thanks for the review Dan!

Fixed the nit in thread.c

Thanks,
David

On 30/10/2019 6:51 am, Daniel D. Daugherty wrote:

On 10/29/19 3:42 AM, David Holmes wrote:

Bug: https://bugs.openjdk.java.net/browse/JDK-8229516
CSR: https://bugs.openjdk.java.net/browse/JDK-8232676 (already approved)
webrev: http://cr.openjdk.java.net/~dholmes/8229516/webrev/


make/hotspot/symbols/symbols-unix
     No comments.

src/hotspot/os/windows/osThread_windows.cpp
     No comments.

src/hotspot/os/windows/osThread_windows.hpp
     No comments.

src/hotspot/share/aot/aotCodeHeap.cpp
     Skipped.

src/hotspot/share/classfile/javaClasses.cpp
     No comments.

src/hotspot/share/classfile/javaClasses.hpp
     No comments.

src/hotspot/share/classfile/vmSymbols.cpp
     No comments.

src/hotspot/share/classfile/vmSymbols.hpp
     No comments.

src/hotspot/share/include/jvm.h
     No comments.

src/hotspot/share/jvmci/jvmciRuntime.cpp
src/hotspot/share/jvmci/jvmciRuntime.hpp
src/hotspot/share/jvmci/vmStructs_jvmci.cpp
     Skipped these three.

src/hotspot/share/oops/oop.hpp
     No comments.

src/hotspot/share/oops/oop.inline.hpp
     No comments.

src/hotspot/share/opto/c2compiler.cpp
     No comments.

src/hotspot/share/opto/library_call.cpp
     No comments.

src/hotspot/share/prims/jvm.cpp
     No comments.

src/hotspot/share/prims/jvmtiEnv.cpp
     No comments.

src/hotspot/share/prims/jvmtiEnvBase.cpp
     No comments.

src/hotspot/share/runtime/osThread.cpp
     No comments.

src/hotspot/share/runtime/osThread.hpp
     No comments.

src/hotspot/share/runtime/thread.cpp
     No comments.

src/hotspot/share/runtime/vmStructs.cpp
     No comments.

src/java.base/share/classes/java/lang/Thread.java
     No comments.

src/java.base/share/native/libjava/Thread.c
     L76:   // Need to reset the interrupt event used by Process.waitFor
     L77:   ResetEvent((HANDLE) JVM_GetThreadInterruptEvent());
     nit - the indent in JDK .c files is four spaces.

src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/runtime/OSThread.java
     No comments.

src/jdk.internal.vm.compiler/share/classes/org.graalvm.compiler.hotspot.test/src/org/graalvm/compiler/hotspot/test/HotSpotMethodSubstitutionTest.java 

src/jdk.internal.vm.compiler/share/classes/org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/GraalHotSpotVMConfig.java 

src/jdk.internal.vm.compiler/share/classes/org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/meta/HotSpotGraphBuilderPlugins.java 

src/jdk.internal.vm.compiler/share/classes/org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/replacements/HotSpotReplacementsUtil.java 

src/jdk.internal.vm.compiler/share/classes/org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/replacements/ThreadSubstitutions.java 


     Skipped these 5 graal files.

Thumbs up! I only have one nit above and don't need to see
another webrev if you decide to fix it.

Thanks for being so thorough on your testing.

Dan




This cross-cuts core-libs, hotspot-runtime and the JIT compilers, but 
only in small pieces each. There is also a small touch to 
serviceability code.


This change is "simply" moving the "interrupted" field out of the 
osThread and into the java.lang.Thread so that it can be set 
independently of whether the thread is alive (and to make things 
easier for loom in the near future). It is very straightforward:


- old scheme
  - interrupted field is in osThread
  - VM can read/write directly
  - Java code calls into VM to read/write

- new scheme
  - interrupted field is in java.lang.Thread
  - VM has to use javaCalls to read/write "directly"
  - Java code can read/write directly

No changes to any of the semantics regarding the actual interrupt 
mechanism. Special thanks to Patricio for tracking down a bug I had 
introduced in that regard!


Special Note (Hi Roger!): on windows we still need to set/clear the 
_interrupt_event used by the Process.waitFor logic. To facilitate 
clearing via Thread.interrupted() I had to introduce a native method 
that is a no-op except on Windows. This seemed the cheapest and least 
intrusive means to achieve this.


Other changes revolve around the fact we used to have an intrinsic for 
Thread.isInterrupted and that is not needed any more. So we strip some 
code out of C1/C2.


The changes in the JVMCI/Graal code are a bit more far reaching as 
entire classes disappear. I've cc'd Doug and Tom at Vladimir's request 
so that they can comment on the JVMCI changes and whether I have gone 
too far or not far enough. There are a bunch of tests for interruption 
in JVMCI that could potentially be deleted if they are only intended 
to test the JVMCI handling of interrupt:


./jdk.internal.vm.compiler/share/classes/org.graalvm.compiler.jtt/src/org/graalvm/compiler/jtt/threads/Thread_isInterrupted*.java 



Testing:
 - Tiers 1-3 on all Oracle platforms
 - Focused testing on Linux x64:
    - Stress runs of JSR166TestCase
    - Anything that seems to use interrupt():
   

Re: RFR: 8229516: Thread.isInterrupted() always returns false after thread termination

2019-10-29 Thread David Holmes 

On 30/10/2019 4:05 am, serguei.spit...@oracle.com wrote:

Hi David,

The fix looks good to me.


Thanks Serguei!

David


I did not pay much attention to the Graal related changes though.
The test coverage for Serviceability is complete.
Running java/lang/instrument tests is not necessary.

Thanks,
Serguei


On 10/29/19 00:42, David Holmes wrote:

Bug: https://bugs.openjdk.java.net/browse/JDK-8229516
CSR: https://bugs.openjdk.java.net/browse/JDK-8232676 (already approved)
webrev: http://cr.openjdk.java.net/~dholmes/8229516/webrev/

This cross-cuts core-libs, hotspot-runtime and the JIT compilers, but 
only in small pieces each. There is also a small touch to 
serviceability code.


This change is "simply" moving the "interrupted" field out of the 
osThread and into the java.lang.Thread so that it can be set 
independently of whether the thread is alive (and to make things 
easier for loom in the near future). It is very straightforward:


- old scheme
  - interrupted field is in osThread
  - VM can read/write directly
  - Java code calls into VM to read/write

- new scheme
  - interrupted field is in java.lang.Thread
  - VM has to use javaCalls to read/write "directly"
  - Java code can read/write directly

No changes to any of the semantics regarding the actual interrupt 
mechanism. Special thanks to Patricio for tracking down a bug I had 
introduced in that regard!


Special Note (Hi Roger!): on windows we still need to set/clear the 
_interrupt_event used by the Process.waitFor logic. To facilitate 
clearing via Thread.interrupted() I had to introduce a native method 
that is a no-op except on Windows. This seemed the cheapest and least 
intrusive means to achieve this.


Other changes revolve around the fact we used to have an intrinsic for 
Thread.isInterrupted and that is not needed any more. So we strip some 
code out of C1/C2.


The changes in the JVMCI/Graal code are a bit more far reaching as 
entire classes disappear. I've cc'd Doug and Tom at Vladimir's request 
so that they can comment on the JVMCI changes and whether I have gone 
too far or not far enough. There are a bunch of tests for interruption 
in JVMCI that could potentially be deleted if they are only intended 
to test the JVMCI handling of interrupt:


./jdk.internal.vm.compiler/share/classes/org.graalvm.compiler.jtt/src/org/graalvm/compiler/jtt/threads/Thread_isInterrupted*.java 



Testing:
 - Tiers 1-3 on all Oracle platforms
 - Focused testing on Linux x64:
    - Stress runs of JSR166TestCase
    - Anything that seems to use interrupt():
  - JDK
    - java/lang/Thread
    - java/util/concurrent
    - jdk/internal/loader/InterruptedClassLoad.java
    - javax/management
    - java/nio/file/Files
    - java/nio/channels
    - java/net/Socket/Timeouts.java
    - java/lang/Runtime/shutdown/
    - java/lang/ProcessBuilder/Basic.java
    - com/sun/jdi/
  - Hotspot
    - vmTestbase/nsk/monitoring/
    - vmTestbase/nsk/jdwp
    - vmTestbase/nsk/jdb/
    - vmTestbase/nsk/jdi/
    - vmTestbase/nsk/jvmti/
    - runtime/Thread
    - serviceability/jvmti/
    - serviceability/jdwp
    - serviceability/sa

Thanks,
David
-

Re: RFR: 8229516: Thread.isInterrupted() always returns false after thread termination

2019-10-29 Thread Remi Forax 
Hi David,
in java.lang.Thread interrupt0 should be renamed to setInterruptEvent, because 
what it does now
and I don't really understand the comment in interrupted(), if a thread is 
interrupted by two other threads calling interrupt(), you will loose an 
interrupt anyway.

Rémi

- Mail original -
> De: "David Holmes" 
> À: "hotspot-runtime-dev" , "hotspot 
> compiler"
> , "core-libs-dev" 
> , "Doug Simon"
> , "TOM.RODRIGUEZ" , 
> "serviceability-dev"
> , "Roger Riggs" 
> Envoyé: Mardi 29 Octobre 2019 08:42:08
> Objet: RFR: 8229516: Thread.isInterrupted() always returns false after thread 
> termination

> Bug: https://bugs.openjdk.java.net/browse/JDK-8229516
> CSR: https://bugs.openjdk.java.net/browse/JDK-8232676 (already approved)
> webrev: http://cr.openjdk.java.net/~dholmes/8229516/webrev/
> 
> This cross-cuts core-libs, hotspot-runtime and the JIT compilers, but
> only in small pieces each. There is also a small touch to serviceability
> code.
> 
> This change is "simply" moving the "interrupted" field out of the
> osThread and into the java.lang.Thread so that it can be set
> independently of whether the thread is alive (and to make things easier
> for loom in the near future). It is very straightforward:
> 
> - old scheme
>   - interrupted field is in osThread
>   - VM can read/write directly
>   - Java code calls into VM to read/write
> 
> - new scheme
>   - interrupted field is in java.lang.Thread
>   - VM has to use javaCalls to read/write "directly"
>   - Java code can read/write directly
> 
> No changes to any of the semantics regarding the actual interrupt
> mechanism. Special thanks to Patricio for tracking down a bug I had
> introduced in that regard!
> 
> Special Note (Hi Roger!): on windows we still need to set/clear the
> _interrupt_event used by the Process.waitFor logic. To facilitate
> clearing via Thread.interrupted() I had to introduce a native method
> that is a no-op except on Windows. This seemed the cheapest and least
> intrusive means to achieve this.
> 
> Other changes revolve around the fact we used to have an intrinsic for
> Thread.isInterrupted and that is not needed any more. So we strip some
> code out of C1/C2.
> 
> The changes in the JVMCI/Graal code are a bit more far reaching as
> entire classes disappear. I've cc'd Doug and Tom at Vladimir's request
> so that they can comment on the JVMCI changes and whether I have gone
> too far or not far enough. There are a bunch of tests for interruption
> in JVMCI that could potentially be deleted if they are only intended to
> test the JVMCI handling of interrupt:
> 
> ./jdk.internal.vm.compiler/share/classes/org.graalvm.compiler.jtt/src/org/graalvm/compiler/jtt/threads/Thread_isInterrupted*.java
> 
> Testing:
>  - Tiers 1-3 on all Oracle platforms
>  - Focused testing on Linux x64:
> - Stress runs of JSR166TestCase
> - Anything that seems to use interrupt():
>   - JDK
> - java/lang/Thread
> - java/util/concurrent
> - jdk/internal/loader/InterruptedClassLoad.java
> - javax/management
> - java/nio/file/Files
> - java/nio/channels
> - java/net/Socket/Timeouts.java
> - java/lang/Runtime/shutdown/
> - java/lang/ProcessBuilder/Basic.java
> - com/sun/jdi/
>   - Hotspot
> - vmTestbase/nsk/monitoring/
> - vmTestbase/nsk/jdwp
> - vmTestbase/nsk/jdb/
> - vmTestbase/nsk/jdi/
> - vmTestbase/nsk/jvmti/
> - runtime/Thread
> - serviceability/jvmti/
> - serviceability/jdwp
> - serviceability/sa
> 
> Thanks,
> David
> -

Re: RFR: 8229516: Thread.isInterrupted() always returns false after thread termination

2019-10-29 Thread Daniel D. Daugherty 

On 10/29/19 3:42 AM, David Holmes wrote:

Bug: https://bugs.openjdk.java.net/browse/JDK-8229516
CSR: https://bugs.openjdk.java.net/browse/JDK-8232676 (already approved)
webrev: http://cr.openjdk.java.net/~dholmes/8229516/webrev/


make/hotspot/symbols/symbols-unix
    No comments.

src/hotspot/os/windows/osThread_windows.cpp
    No comments.

src/hotspot/os/windows/osThread_windows.hpp
    No comments.

src/hotspot/share/aot/aotCodeHeap.cpp
    Skipped.

src/hotspot/share/classfile/javaClasses.cpp
    No comments.

src/hotspot/share/classfile/javaClasses.hpp
    No comments.

src/hotspot/share/classfile/vmSymbols.cpp
    No comments.

src/hotspot/share/classfile/vmSymbols.hpp
    No comments.

src/hotspot/share/include/jvm.h
    No comments.

src/hotspot/share/jvmci/jvmciRuntime.cpp
src/hotspot/share/jvmci/jvmciRuntime.hpp
src/hotspot/share/jvmci/vmStructs_jvmci.cpp
    Skipped these three.

src/hotspot/share/oops/oop.hpp
    No comments.

src/hotspot/share/oops/oop.inline.hpp
    No comments.

src/hotspot/share/opto/c2compiler.cpp
    No comments.

src/hotspot/share/opto/library_call.cpp
    No comments.

src/hotspot/share/prims/jvm.cpp
    No comments.

src/hotspot/share/prims/jvmtiEnv.cpp
    No comments.

src/hotspot/share/prims/jvmtiEnvBase.cpp
    No comments.

src/hotspot/share/runtime/osThread.cpp
    No comments.

src/hotspot/share/runtime/osThread.hpp
    No comments.

src/hotspot/share/runtime/thread.cpp
    No comments.

src/hotspot/share/runtime/vmStructs.cpp
    No comments.

src/java.base/share/classes/java/lang/Thread.java
    No comments.

src/java.base/share/native/libjava/Thread.c
    L76:   // Need to reset the interrupt event used by Process.waitFor
    L77:   ResetEvent((HANDLE) JVM_GetThreadInterruptEvent());
    nit - the indent in JDK .c files is four spaces.

src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/runtime/OSThread.java
    No comments.

src/jdk.internal.vm.compiler/share/classes/org.graalvm.compiler.hotspot.test/src/org/graalvm/compiler/hotspot/test/HotSpotMethodSubstitutionTest.java
src/jdk.internal.vm.compiler/share/classes/org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/GraalHotSpotVMConfig.java
src/jdk.internal.vm.compiler/share/classes/org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/meta/HotSpotGraphBuilderPlugins.java
src/jdk.internal.vm.compiler/share/classes/org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/replacements/HotSpotReplacementsUtil.java
src/jdk.internal.vm.compiler/share/classes/org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/replacements/ThreadSubstitutions.java
    Skipped these 5 graal files.

Thumbs up! I only have one nit above and don't need to see
another webrev if you decide to fix it.

Thanks for being so thorough on your testing.

Dan




This cross-cuts core-libs, hotspot-runtime and the JIT compilers, but 
only in small pieces each. There is also a small touch to 
serviceability code.


This change is "simply" moving the "interrupted" field out of the 
osThread and into the java.lang.Thread so that it can be set 
independently of whether the thread is alive (and to make things 
easier for loom in the near future). It is very straightforward:


- old scheme
  - interrupted field is in osThread
  - VM can read/write directly
  - Java code calls into VM to read/write

- new scheme
  - interrupted field is in java.lang.Thread
  - VM has to use javaCalls to read/write "directly"
  - Java code can read/write directly

No changes to any of the semantics regarding the actual interrupt 
mechanism. Special thanks to Patricio for tracking down a bug I had 
introduced in that regard!


Special Note (Hi Roger!): on windows we still need to set/clear the 
_interrupt_event used by the Process.waitFor logic. To facilitate 
clearing via Thread.interrupted() I had to introduce a native method 
that is a no-op except on Windows. This seemed the cheapest and least 
intrusive means to achieve this.


Other changes revolve around the fact we used to have an intrinsic for 
Thread.isInterrupted and that is not needed any more. So we strip some 
code out of C1/C2.


The changes in the JVMCI/Graal code are a bit more far reaching as 
entire classes disappear. I've cc'd Doug and Tom at Vladimir's request 
so that they can comment on the JVMCI changes and whether I have gone 
too far or not far enough. There are a bunch of tests for interruption 
in JVMCI that could potentially be deleted if they are only intended 
to test the JVMCI handling of interrupt:


./jdk.internal.vm.compiler/share/classes/org.graalvm.compiler.jtt/src/org/graalvm/compiler/jtt/threads/Thread_isInterrupted*.java 



Testing:
 - Tiers 1-3 on all Oracle platforms
 - Focused testing on Linux x64:
    - Stress runs of JSR166TestCase
    - Anything that seems to use interrupt():
  - JDK
    - java/lang/Thread
    - java/util/concurrent
    - jdk/internal/loader/InterruptedClassLoad.java
    - javax/management
   

Re: RFR: 8229516: Thread.isInterrupted() always returns false after thread termination

2019-10-29 Thread serguei.spit...@oracle.com 

Hi David,

The fix looks good to me.
I did not pay much attention to the Graal related changes though.
The test coverage for Serviceability is complete.
Running java/lang/instrument tests is not necessary.

Thanks,
Serguei


On 10/29/19 00:42, David Holmes wrote:

Bug: https://bugs.openjdk.java.net/browse/JDK-8229516
CSR: https://bugs.openjdk.java.net/browse/JDK-8232676 (already approved)
webrev: http://cr.openjdk.java.net/~dholmes/8229516/webrev/

This cross-cuts core-libs, hotspot-runtime and the JIT compilers, but 
only in small pieces each. There is also a small touch to 
serviceability code.


This change is "simply" moving the "interrupted" field out of the 
osThread and into the java.lang.Thread so that it can be set 
independently of whether the thread is alive (and to make things 
easier for loom in the near future). It is very straightforward:


- old scheme
  - interrupted field is in osThread
  - VM can read/write directly
  - Java code calls into VM to read/write

- new scheme
  - interrupted field is in java.lang.Thread
  - VM has to use javaCalls to read/write "directly"
  - Java code can read/write directly

No changes to any of the semantics regarding the actual interrupt 
mechanism. Special thanks to Patricio for tracking down a bug I had 
introduced in that regard!


Special Note (Hi Roger!): on windows we still need to set/clear the 
_interrupt_event used by the Process.waitFor logic. To facilitate 
clearing via Thread.interrupted() I had to introduce a native method 
that is a no-op except on Windows. This seemed the cheapest and least 
intrusive means to achieve this.


Other changes revolve around the fact we used to have an intrinsic for 
Thread.isInterrupted and that is not needed any more. So we strip some 
code out of C1/C2.


The changes in the JVMCI/Graal code are a bit more far reaching as 
entire classes disappear. I've cc'd Doug and Tom at Vladimir's request 
so that they can comment on the JVMCI changes and whether I have gone 
too far or not far enough. There are a bunch of tests for interruption 
in JVMCI that could potentially be deleted if they are only intended 
to test the JVMCI handling of interrupt:


./jdk.internal.vm.compiler/share/classes/org.graalvm.compiler.jtt/src/org/graalvm/compiler/jtt/threads/Thread_isInterrupted*.java 



Testing:
 - Tiers 1-3 on all Oracle platforms
 - Focused testing on Linux x64:
    - Stress runs of JSR166TestCase
    - Anything that seems to use interrupt():
  - JDK
    - java/lang/Thread
    - java/util/concurrent
    - jdk/internal/loader/InterruptedClassLoad.java
    - javax/management
    - java/nio/file/Files
    - java/nio/channels
    - java/net/Socket/Timeouts.java
    - java/lang/Runtime/shutdown/
    - java/lang/ProcessBuilder/Basic.java
    - com/sun/jdi/
  - Hotspot
    - vmTestbase/nsk/monitoring/
    - vmTestbase/nsk/jdwp
    - vmTestbase/nsk/jdb/
    - vmTestbase/nsk/jdi/
    - vmTestbase/nsk/jvmti/
    - runtime/Thread
    - serviceability/jvmti/
    - serviceability/jdwp
    - serviceability/sa

Thanks,
David
-

Re: RFR: 8229516: Thread.isInterrupted() always returns false after thread termination

2019-10-29 Thread Doug Simon 



> On 29 Oct 2019, at 10:12, David Holmes  wrote:
> 
> Hi Doug,
> 
> Thanks for taking a look so quickly! :)
> 
> On 29/10/2019 6:55 pm, Doug Simon wrote:
>> Hi David,
>> Since Graal needs to work against multiple JVMCI versions (and VM 
>> versions!), the Graal changes should only disable the intrinsic when the 
>> relevant VM config is missing:
> 
> So to be clear I should revert all the Graal file changes I made and just 
> apply the patch below?

Yes please.

-Doug

> 
>> diff --git 
>> a/compiler/src/org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/GraalHotSpotVMConfig.java
>>  
>> b/compiler/src/org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/GraalHotSpotVMConfig.java
>> index 0752a621215..baa2136a6ba 100644
>> --- 
>> a/compiler/src/org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/GraalHotSpotVMConfig.java
>> +++ 
>> b/compiler/src/org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/GraalHotSpotVMConfig.java
>> @@ -315,7 +315,7 @@ public class GraalHotSpotVMConfig extends 
>> GraalHotSpotVMConfigBase {
>>  public final int javaThreadAnchorOffset = 
>> getFieldOffset("JavaThread::_anchor", Integer.class, "JavaFrameAnchor");
>>  public final int javaThreadShouldPostOnExceptionsFlagOffset = 
>> getFieldOffset("JavaThread::_should_post_on_exceptions_flag", Integer.class, 
>> "int", Integer.MIN_VALUE);
>>  public final int threadObjectOffset = 
>> getFieldOffset("JavaThread::_threadObj", Integer.class, "oop");
>> -public final int osThreadOffset = 
>> getFieldOffset("JavaThread::_osthread", Integer.class, "OSThread*");
>> +public final int osThreadOffset = 
>> getFieldOffset("JavaThread::_osthread", Integer.class, "OSThread*", 
>> Integer.MAX_VALUE);
>>  public final int threadIsMethodHandleReturnOffset = 
>> getFieldOffset("JavaThread::_is_method_handle_return", Integer.class, "int");
>>  public final int threadObjectResultOffset = 
>> getFieldOffset("JavaThread::_vm_result", Integer.class, "oop");
>>  public final int jvmciCountersThreadOffset = 
>> getFieldOffset("JavaThread::_jvmci_counters", Integer.class, "jlong*");
>> diff --git 
>> a/compiler/src/org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/meta/HotSpotGraphBuilderPlugins.java
>>  
>> b/compiler/src/org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/meta/HotSpotGraphBuilderPlugins.java
>> index 6b37fff083d..ffc8032d2b0 100644
>> --- 
>> a/compiler/src/org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/meta/HotSpotGraphBuilderPlugins.java
>> +++ 
>> b/compiler/src/org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/meta/HotSpotGraphBuilderPlugins.java
>> @@ -441,7 +441,9 @@ public class HotSpotGraphBuilderPlugins {
>>  }
>>  });
>>  -r.registerMethodSubstitution(ThreadSubstitutions.class, 
>> "isInterrupted", Receiver.class, boolean.class);
>> +if (config.osThreadOffset != Integer.MAX_VALUE) {
>> +r.registerMethodSubstitution(ThreadSubstitutions.class, 
>> "isInterrupted", Receiver.class, boolean.class);
>> +}
>>  }
>>public static final String reflectionClass;
>> diff --git 
>> a/compiler/src/org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/replacements/HotSpotReplacementsUtil.java
>>  
>> b/compiler/src/org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/replacements/HotSpotReplacementsUtil.java
>> index 1d694fae2a4..8500c4de115 100644
>> --- 
>> a/compiler/src/org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/replacements/HotSpotReplacementsUtil.java
>> +++ 
>> b/compiler/src/org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/replacements/HotSpotReplacementsUtil.java
>> @@ -308,6 +308,7 @@ public class HotSpotReplacementsUtil {
>>@Fold
>>  public static int osThreadOffset(@InjectedParameter 
>> GraalHotSpotVMConfig config) {
>> +assert config.instanceKlassInitThreadOffset != Integer.MAX_VALUE;
>>  return config.osThreadOffset;
>>  }
>>  All the other JVMCI changes look good to me.
>> -Doug
>>> On 29 Oct 2019, at 08:42, David Holmes  wrote:
>>> 
>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8229516
>>> CSR: https://bugs.openjdk.java.net/browse/JDK-8232676 (already approved)
>>> webrev: http://cr.openjdk.java.net/~dholmes/8229516/webrev/
>>> 
>>> This cross-cuts core-libs, hotspot-runtime and the JIT compilers, but only 
>>> in small pieces each. There is also a small touch to serviceability code.
>>> 
>>> This change is "simply" moving the "interrupted" field out of the osThread 
>>> and into the java.lang.Thread so that it can be set independently of 
>>> whether the thread is alive (and to make things easier for loom in the near 
>>> future). It is very straightforward:
>>> 
>>> - old scheme
>>>  - interrupted field is in osThread
>>>  - VM can read/write directly
>>>  - Java code calls into VM to read/write
>>> 
>>> - new scheme
>>>  - interrupted field is in

Re: RFR: 8229516: Thread.isInterrupted() always returns false after thread termination

2019-10-29 Thread David Holmes 

Hi Doug,

Thanks for taking a look so quickly! :)

On 29/10/2019 6:55 pm, Doug Simon wrote:

Hi David,

Since Graal needs to work against multiple JVMCI versions (and VM versions!), 
the Graal changes should only disable the intrinsic when the relevant VM config 
is missing:


So to be clear I should revert all the Graal file changes I made and 
just apply the patch below?


Thanks,
David
-


diff --git 
a/compiler/src/org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/GraalHotSpotVMConfig.java
 
b/compiler/src/org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/GraalHotSpotVMConfig.java
index 0752a621215..baa2136a6ba 100644
--- 
a/compiler/src/org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/GraalHotSpotVMConfig.java
+++ 
b/compiler/src/org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/GraalHotSpotVMConfig.java
@@ -315,7 +315,7 @@ public class GraalHotSpotVMConfig extends 
GraalHotSpotVMConfigBase {
  public final int javaThreadAnchorOffset = getFieldOffset("JavaThread::_anchor", 
Integer.class, "JavaFrameAnchor");
  public final int javaThreadShouldPostOnExceptionsFlagOffset = 
getFieldOffset("JavaThread::_should_post_on_exceptions_flag", Integer.class, 
"int", Integer.MIN_VALUE);
  public final int threadObjectOffset = getFieldOffset("JavaThread::_threadObj", 
Integer.class, "oop");
-public final int osThreadOffset = getFieldOffset("JavaThread::_osthread", 
Integer.class, "OSThread*");
+public final int osThreadOffset = getFieldOffset("JavaThread::_osthread", 
Integer.class, "OSThread*", Integer.MAX_VALUE);
  public final int threadIsMethodHandleReturnOffset = 
getFieldOffset("JavaThread::_is_method_handle_return", Integer.class, "int");
  public final int threadObjectResultOffset = 
getFieldOffset("JavaThread::_vm_result", Integer.class, "oop");
  public final int jvmciCountersThreadOffset = 
getFieldOffset("JavaThread::_jvmci_counters", Integer.class, "jlong*");
diff --git 
a/compiler/src/org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/meta/HotSpotGraphBuilderPlugins.java
 
b/compiler/src/org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/meta/HotSpotGraphBuilderPlugins.java
index 6b37fff083d..ffc8032d2b0 100644
--- 
a/compiler/src/org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/meta/HotSpotGraphBuilderPlugins.java
+++ 
b/compiler/src/org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/meta/HotSpotGraphBuilderPlugins.java
@@ -441,7 +441,9 @@ public class HotSpotGraphBuilderPlugins {
  }
  });
  
-r.registerMethodSubstitution(ThreadSubstitutions.class, "isInterrupted", Receiver.class, boolean.class);

+if (config.osThreadOffset != Integer.MAX_VALUE) {
+r.registerMethodSubstitution(ThreadSubstitutions.class, 
"isInterrupted", Receiver.class, boolean.class);
+}
  }
  
  public static final String reflectionClass;

diff --git 
a/compiler/src/org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/replacements/HotSpotReplacementsUtil.java
 
b/compiler/src/org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/replacements/HotSpotReplacementsUtil.java
index 1d694fae2a4..8500c4de115 100644
--- 
a/compiler/src/org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/replacements/HotSpotReplacementsUtil.java
+++ 
b/compiler/src/org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/replacements/HotSpotReplacementsUtil.java
@@ -308,6 +308,7 @@ public class HotSpotReplacementsUtil {
  
  @Fold

  public static int osThreadOffset(@InjectedParameter GraalHotSpotVMConfig 
config) {
+assert config.instanceKlassInitThreadOffset != Integer.MAX_VALUE;
  return config.osThreadOffset;
  }
  
All the other JVMCI changes look good to me.


-Doug


On 29 Oct 2019, at 08:42, David Holmes  wrote:

Bug: https://bugs.openjdk.java.net/browse/JDK-8229516
CSR: https://bugs.openjdk.java.net/browse/JDK-8232676 (already approved)
webrev: http://cr.openjdk.java.net/~dholmes/8229516/webrev/

This cross-cuts core-libs, hotspot-runtime and the JIT compilers, but only in 
small pieces each. There is also a small touch to serviceability code.

This change is "simply" moving the "interrupted" field out of the osThread and 
into the java.lang.Thread so that it can be set independently of whether the thread is alive (and 
to make things easier for loom in the near future). It is very straightforward:

- old scheme
  - interrupted field is in osThread
  - VM can read/write directly
  - Java code calls into VM to read/write

- new scheme
  - interrupted field is in java.lang.Thread
  - VM has to use javaCalls to read/write "directly"
  - Java code can read/write directly

No changes to any of the semantics regarding the actual interrupt mechanism. 
Special thanks to Patricio for tracking down a bug I had introduced in that 
regard!

Special Note (Hi Roger!): on windows we still need to set/clear the

Re: RFR: 8229516: Thread.isInterrupted() always returns false after thread termination

2019-10-29 Thread Doug Simon 
Hi David,

Since Graal needs to work against multiple JVMCI versions (and VM versions!), 
the Graal changes should only disable the intrinsic when the relevant VM config 
is missing:

diff --git 
a/compiler/src/org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/GraalHotSpotVMConfig.java
 
b/compiler/src/org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/GraalHotSpotVMConfig.java
index 0752a621215..baa2136a6ba 100644
--- 
a/compiler/src/org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/GraalHotSpotVMConfig.java
+++ 
b/compiler/src/org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/GraalHotSpotVMConfig.java
@@ -315,7 +315,7 @@ public class GraalHotSpotVMConfig extends 
GraalHotSpotVMConfigBase {
 public final int javaThreadAnchorOffset = 
getFieldOffset("JavaThread::_anchor", Integer.class, "JavaFrameAnchor");
 public final int javaThreadShouldPostOnExceptionsFlagOffset = 
getFieldOffset("JavaThread::_should_post_on_exceptions_flag", Integer.class, 
"int", Integer.MIN_VALUE);
 public final int threadObjectOffset = 
getFieldOffset("JavaThread::_threadObj", Integer.class, "oop");
-public final int osThreadOffset = getFieldOffset("JavaThread::_osthread", 
Integer.class, "OSThread*");
+public final int osThreadOffset = getFieldOffset("JavaThread::_osthread", 
Integer.class, "OSThread*", Integer.MAX_VALUE);
 public final int threadIsMethodHandleReturnOffset = 
getFieldOffset("JavaThread::_is_method_handle_return", Integer.class, "int");
 public final int threadObjectResultOffset = 
getFieldOffset("JavaThread::_vm_result", Integer.class, "oop");
 public final int jvmciCountersThreadOffset = 
getFieldOffset("JavaThread::_jvmci_counters", Integer.class, "jlong*");
diff --git 
a/compiler/src/org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/meta/HotSpotGraphBuilderPlugins.java
 
b/compiler/src/org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/meta/HotSpotGraphBuilderPlugins.java
index 6b37fff083d..ffc8032d2b0 100644
--- 
a/compiler/src/org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/meta/HotSpotGraphBuilderPlugins.java
+++ 
b/compiler/src/org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/meta/HotSpotGraphBuilderPlugins.java
@@ -441,7 +441,9 @@ public class HotSpotGraphBuilderPlugins {
 }
 });
 
-r.registerMethodSubstitution(ThreadSubstitutions.class, 
"isInterrupted", Receiver.class, boolean.class);
+if (config.osThreadOffset != Integer.MAX_VALUE) {
+r.registerMethodSubstitution(ThreadSubstitutions.class, 
"isInterrupted", Receiver.class, boolean.class);
+}
 }
 
 public static final String reflectionClass;
diff --git 
a/compiler/src/org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/replacements/HotSpotReplacementsUtil.java
 
b/compiler/src/org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/replacements/HotSpotReplacementsUtil.java
index 1d694fae2a4..8500c4de115 100644
--- 
a/compiler/src/org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/replacements/HotSpotReplacementsUtil.java
+++ 
b/compiler/src/org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/replacements/HotSpotReplacementsUtil.java
@@ -308,6 +308,7 @@ public class HotSpotReplacementsUtil {
 
 @Fold
 public static int osThreadOffset(@InjectedParameter GraalHotSpotVMConfig 
config) {
+assert config.instanceKlassInitThreadOffset != Integer.MAX_VALUE;
 return config.osThreadOffset;
 }
 
All the other JVMCI changes look good to me.

-Doug

> On 29 Oct 2019, at 08:42, David Holmes  wrote:
> 
> Bug: https://bugs.openjdk.java.net/browse/JDK-8229516
> CSR: https://bugs.openjdk.java.net/browse/JDK-8232676 (already approved)
> webrev: http://cr.openjdk.java.net/~dholmes/8229516/webrev/
> 
> This cross-cuts core-libs, hotspot-runtime and the JIT compilers, but only in 
> small pieces each. There is also a small touch to serviceability code.
> 
> This change is "simply" moving the "interrupted" field out of the osThread 
> and into the java.lang.Thread so that it can be set independently of whether 
> the thread is alive (and to make things easier for loom in the near future). 
> It is very straightforward:
> 
> - old scheme
>  - interrupted field is in osThread
>  - VM can read/write directly
>  - Java code calls into VM to read/write
> 
> - new scheme
>  - interrupted field is in java.lang.Thread
>  - VM has to use javaCalls to read/write "directly"
>  - Java code can read/write directly
> 
> No changes to any of the semantics regarding the actual interrupt mechanism. 
> Special thanks to Patricio for tracking down a bug I had introduced in that 
> regard!
> 
> Special Note (Hi Roger!): on windows we still need to set/clear the 
> _interrupt_event used by the Process.waitFor logic. To facilitate clearing 
> via Thread.interrupted() I had to introduce a native method that is a no-op 
> except on Windows. This