Quantcast

RFR: 8174957: [JVMCI] jaotc is broken in Xcomp mode

classic Classic list List threaded Threaded
8 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

RFR: 8174957: [JVMCI] jaotc is broken in Xcomp mode

Doug Simon @ Oracle
Please review this fix for a bug introduced by JDK-8173912. The value written by readConfiguation (in jvmciCompilerToVM.cpp) to VMField.value may now be a Boolean. As such, the type of VMField.value must be Object.

https://bugs.openjdk.java.net/browse/JDK-8174957
http://cr.openjdk.java.net/~dnsimon/8174957/webrev/

-Doug
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR: 8174957: [JVMCI] jaotc is broken in Xcomp mode

Igor Veresov
Looks good, but I think the following bit would also be required to fix this particular issue:

diff --git a/src/jdk.aot/share/classes/jdk.tools.jaotc/src/jdk/tools/jaotc/DataBuilder.java b/src/jdk.aot/share/classes/jdk.tools.jaotc/src/jdk/tools/jaotc/DataBuilder.java
--- a/src/jdk.aot/share/classes/jdk.tools.jaotc/src/jdk/tools/jaotc/DataBuilder.java
+++ b/src/jdk.aot/share/classes/jdk.tools.jaotc/src/jdk/tools/jaotc/DataBuilder.java
@@ -70,7 +70,7 @@
      */
     private void fillVMAddresses(HotSpotVMConfigStore config) {
         for (VMField vmField : config.getFields().values()) {
-            if (vmField.value != null) {
+            if (vmField.value != null && vmField.value instanceof Long) {
                 final long address = vmField.value;
                 String value = vmField.name;
                 /*

Thanks,
igor

On Feb 15, 2017, at 2:24 AM, Doug Simon <[hidden email]> wrote:

Please review this fix for a bug introduced by JDK-8173912. The value written by readConfiguation (in jvmciCompilerToVM.cpp) to VMField.value may now be a Boolean. As such, the type of VMField.value must be Object.

https://bugs.openjdk.java.net/browse/JDK-8174957
http://cr.openjdk.java.net/~dnsimon/8174957/webrev/

-Doug

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR: 8174957: [JVMCI] jaotc is broken in Xcomp mode

Igor Veresov
Sorry, it would also need a cast of course..

diff --git a/src/jdk.aot/share/classes/jdk.tools.jaotc/src/jdk/tools/jaotc/DataBuilder.java b/src/jdk.aot/share/classes/jdk.tools.jaotc/src/jdk/tools/jaotc/DataBuilder.java
--- a/src/jdk.aot/share/classes/jdk.tools.jaotc/src/jdk/tools/jaotc/DataBuilder.java
+++ b/src/jdk.aot/share/classes/jdk.tools.jaotc/src/jdk/tools/jaotc/DataBuilder.java
@@ -70,8 +70,8 @@
      */
     private void fillVMAddresses(HotSpotVMConfigStore config) {
         for (VMField vmField : config.getFields().values()) {
-            if (vmField.value != null) {
-                final long address = vmField.value;
+            if (vmField.value != null && vmField.value instanceof Long) {
+                final long address = (Long) vmField.value;
                 String value = vmField.name;
                 /*
                  * Some fields don't contain addresses but integer values. At least don't add zero


On Feb 15, 2017, at 8:32 AM, Igor Veresov <[hidden email]> wrote:

Looks good, but I think the following bit would also be required to fix this particular issue:

diff --git a/src/jdk.aot/share/classes/jdk.tools.jaotc/src/jdk/tools/jaotc/DataBuilder.java b/src/jdk.aot/share/classes/jdk.tools.jaotc/src/jdk/tools/jaotc/DataBuilder.java
--- a/src/jdk.aot/share/classes/jdk.tools.jaotc/src/jdk/tools/jaotc/DataBuilder.java
+++ b/src/jdk.aot/share/classes/jdk.tools.jaotc/src/jdk/tools/jaotc/DataBuilder.java
@@ -70,7 +70,7 @@
      */
     private void fillVMAddresses(HotSpotVMConfigStore config) {
         for (VMField vmField : config.getFields().values()) {
-            if (vmField.value != null) {
+            if (vmField.value != null && vmField.value instanceof Long) {
                 final long address = vmField.value;
                 String value = vmField.name;
                 /*

Thanks,
igor

On Feb 15, 2017, at 2:24 AM, Doug Simon <[hidden email]> wrote:

Please review this fix for a bug introduced by JDK-8173912. The value written by readConfiguation (in jvmciCompilerToVM.cpp) to VMField.value may now be a Boolean. As such, the type of VMField.value must be Object.

https://bugs.openjdk.java.net/browse/JDK-8174957
http://cr.openjdk.java.net/~dnsimon/8174957/webrev/

-Doug


Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR: 8174957: [JVMCI] jaotc is broken in Xcomp mode

Doug Simon @ Oracle
Ok, I’ve updated the webrev.

I assume the missing change to DataBuilder would have been caught by AOT specific tests in jprt?

-Doug

> On 15 Feb 2017, at 17:34, Igor Veresov <[hidden email]> wrote:
>
> Sorry, it would also need a cast of course..
>
> diff --git a/src/jdk.aot/share/classes/jdk.tools.jaotc/src/jdk/tools/jaotc/DataBuilder.java b/src/jdk.aot/share/classes/jdk.tools.jaotc/src/jdk/tools/jaotc/DataBuilder.java
> --- a/src/jdk.aot/share/classes/jdk.tools.jaotc/src/jdk/tools/jaotc/DataBuilder.java
> +++ b/src/jdk.aot/share/classes/jdk.tools.jaotc/src/jdk/tools/jaotc/DataBuilder.java
> @@ -70,8 +70,8 @@
>       */
>      private void fillVMAddresses(HotSpotVMConfigStore config) {
>          for (VMField vmField : config.getFields().values()) {
> -            if (vmField.value != null) {
> -                final long address = vmField.value;
> +            if (vmField.value != null && vmField.value instanceof Long) {
> +                final long address = (Long) vmField.value;
>                  String value = vmField.name;
>                  /*
>                   * Some fields don't contain addresses but integer values. At least don't add zero
>
>
>> On Feb 15, 2017, at 8:32 AM, Igor Veresov <[hidden email]> wrote:
>>
>> Looks good, but I think the following bit would also be required to fix this particular issue:
>>
>> diff --git a/src/jdk.aot/share/classes/jdk.tools.jaotc/src/jdk/tools/jaotc/DataBuilder.java b/src/jdk.aot/share/classes/jdk.tools.jaotc/src/jdk/tools/jaotc/DataBuilder.java
>> --- a/src/jdk.aot/share/classes/jdk.tools.jaotc/src/jdk/tools/jaotc/DataBuilder.java
>> +++ b/src/jdk.aot/share/classes/jdk.tools.jaotc/src/jdk/tools/jaotc/DataBuilder.java
>> @@ -70,7 +70,7 @@
>>       */
>>      private void fillVMAddresses(HotSpotVMConfigStore config) {
>>          for (VMField vmField : config.getFields().values()) {
>> -            if (vmField.value != null) {
>> +            if (vmField.value != null && vmField.value instanceof Long) {
>>                  final long address = vmField.value;
>>                  String value = vmField.name;
>>                  /*
>>
>> Thanks,
>> igor
>>
>>> On Feb 15, 2017, at 2:24 AM, Doug Simon <[hidden email]> wrote:
>>>
>>> Please review this fix for a bug introduced by JDK-8173912. The value written by readConfiguation (in jvmciCompilerToVM.cpp) to VMField.value may now be a Boolean. As such, the type of VMField.value must be Object.
>>>
>>> https://bugs.openjdk.java.net/browse/JDK-8174957
>>> http://cr.openjdk.java.net/~dnsimon/8174957/webrev/
>>>
>>> -Doug
>>
>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR: 8174957: [JVMCI] jaotc is broken in Xcomp mode

Igor Veresov
> On Feb 15, 2017, at 8:46 AM, Doug Simon <[hidden email]> wrote:
>
> Ok, I’ve updated the webrev.

Thanks, looks good!
>
> I assume the missing change to DataBuilder would have been caught by AOT specific tests in jprt?
>

Yes, I think it would’ve failed building on linux x64.


igor

> -Doug
>
>> On 15 Feb 2017, at 17:34, Igor Veresov <[hidden email]> wrote:
>>
>> Sorry, it would also need a cast of course..
>>
>> diff --git a/src/jdk.aot/share/classes/jdk.tools.jaotc/src/jdk/tools/jaotc/DataBuilder.java b/src/jdk.aot/share/classes/jdk.tools.jaotc/src/jdk/tools/jaotc/DataBuilder.java
>> --- a/src/jdk.aot/share/classes/jdk.tools.jaotc/src/jdk/tools/jaotc/DataBuilder.java
>> +++ b/src/jdk.aot/share/classes/jdk.tools.jaotc/src/jdk/tools/jaotc/DataBuilder.java
>> @@ -70,8 +70,8 @@
>>      */
>>     private void fillVMAddresses(HotSpotVMConfigStore config) {
>>         for (VMField vmField : config.getFields().values()) {
>> -            if (vmField.value != null) {
>> -                final long address = vmField.value;
>> +            if (vmField.value != null && vmField.value instanceof Long) {
>> +                final long address = (Long) vmField.value;
>>                 String value = vmField.name;
>>                 /*
>>                  * Some fields don't contain addresses but integer values. At least don't add zero
>>
>>
>>> On Feb 15, 2017, at 8:32 AM, Igor Veresov <[hidden email]> wrote:
>>>
>>> Looks good, but I think the following bit would also be required to fix this particular issue:
>>>
>>> diff --git a/src/jdk.aot/share/classes/jdk.tools.jaotc/src/jdk/tools/jaotc/DataBuilder.java b/src/jdk.aot/share/classes/jdk.tools.jaotc/src/jdk/tools/jaotc/DataBuilder.java
>>> --- a/src/jdk.aot/share/classes/jdk.tools.jaotc/src/jdk/tools/jaotc/DataBuilder.java
>>> +++ b/src/jdk.aot/share/classes/jdk.tools.jaotc/src/jdk/tools/jaotc/DataBuilder.java
>>> @@ -70,7 +70,7 @@
>>>      */
>>>     private void fillVMAddresses(HotSpotVMConfigStore config) {
>>>         for (VMField vmField : config.getFields().values()) {
>>> -            if (vmField.value != null) {
>>> +            if (vmField.value != null && vmField.value instanceof Long) {
>>>                 final long address = vmField.value;
>>>                 String value = vmField.name;
>>>                 /*
>>>
>>> Thanks,
>>> igor
>>>
>>>> On Feb 15, 2017, at 2:24 AM, Doug Simon <[hidden email]> wrote:
>>>>
>>>> Please review this fix for a bug introduced by JDK-8173912. The value written by readConfiguation (in jvmciCompilerToVM.cpp) to VMField.value may now be a Boolean. As such, the type of VMField.value must be Object.
>>>>
>>>> https://bugs.openjdk.java.net/browse/JDK-8174957
>>>> http://cr.openjdk.java.net/~dnsimon/8174957/webrev/
>>>>
>>>> -Doug
>>>
>>
>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR: 8174957: [JVMCI] jaotc is broken in Xcomp mode

Doug Simon @ Oracle

> On 15 Feb 2017, at 18:05, Igor Veresov <[hidden email]> wrote:
>
>> On Feb 15, 2017, at 8:46 AM, Doug Simon <[hidden email]> wrote:
>>
>> Ok, I’ve updated the webrev.
>
> Thanks, looks good!
>>
>> I assume the missing change to DataBuilder would have been caught by AOT specific tests in jprt?
>>
>
> Yes, I think it would’ve failed building on linux x64.

Good to know ;-)

Thanks for the review.

-Doug

>> -Doug
>>
>>> On 15 Feb 2017, at 17:34, Igor Veresov <[hidden email]> wrote:
>>>
>>> Sorry, it would also need a cast of course..
>>>
>>> diff --git a/src/jdk.aot/share/classes/jdk.tools.jaotc/src/jdk/tools/jaotc/DataBuilder.java b/src/jdk.aot/share/classes/jdk.tools.jaotc/src/jdk/tools/jaotc/DataBuilder.java
>>> --- a/src/jdk.aot/share/classes/jdk.tools.jaotc/src/jdk/tools/jaotc/DataBuilder.java
>>> +++ b/src/jdk.aot/share/classes/jdk.tools.jaotc/src/jdk/tools/jaotc/DataBuilder.java
>>> @@ -70,8 +70,8 @@
>>>     */
>>>    private void fillVMAddresses(HotSpotVMConfigStore config) {
>>>        for (VMField vmField : config.getFields().values()) {
>>> -            if (vmField.value != null) {
>>> -                final long address = vmField.value;
>>> +            if (vmField.value != null && vmField.value instanceof Long) {
>>> +                final long address = (Long) vmField.value;
>>>                String value = vmField.name;
>>>                /*
>>>                 * Some fields don't contain addresses but integer values. At least don't add zero
>>>
>>>
>>>> On Feb 15, 2017, at 8:32 AM, Igor Veresov <[hidden email]> wrote:
>>>>
>>>> Looks good, but I think the following bit would also be required to fix this particular issue:
>>>>
>>>> diff --git a/src/jdk.aot/share/classes/jdk.tools.jaotc/src/jdk/tools/jaotc/DataBuilder.java b/src/jdk.aot/share/classes/jdk.tools.jaotc/src/jdk/tools/jaotc/DataBuilder.java
>>>> --- a/src/jdk.aot/share/classes/jdk.tools.jaotc/src/jdk/tools/jaotc/DataBuilder.java
>>>> +++ b/src/jdk.aot/share/classes/jdk.tools.jaotc/src/jdk/tools/jaotc/DataBuilder.java
>>>> @@ -70,7 +70,7 @@
>>>>     */
>>>>    private void fillVMAddresses(HotSpotVMConfigStore config) {
>>>>        for (VMField vmField : config.getFields().values()) {
>>>> -            if (vmField.value != null) {
>>>> +            if (vmField.value != null && vmField.value instanceof Long) {
>>>>                final long address = vmField.value;
>>>>                String value = vmField.name;
>>>>                /*
>>>>
>>>> Thanks,
>>>> igor
>>>>
>>>>> On Feb 15, 2017, at 2:24 AM, Doug Simon <[hidden email]> wrote:
>>>>>
>>>>> Please review this fix for a bug introduced by JDK-8173912. The value written by readConfiguation (in jvmciCompilerToVM.cpp) to VMField.value may now be a Boolean. As such, the type of VMField.value must be Object.
>>>>>
>>>>> https://bugs.openjdk.java.net/browse/JDK-8174957
>>>>> http://cr.openjdk.java.net/~dnsimon/8174957/webrev/
>>>>>
>>>>> -Doug
>>>>
>>>
>>
>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR: 8174957: [JVMCI] jaotc is broken in Xcomp mode

Vladimir Kozlov
On 2/15/17 9:19 AM, Doug Simon wrote:
>
>> On 15 Feb 2017, at 18:05, Igor Veresov <[hidden email]> wrote:
>>
>>> On Feb 15, 2017, at 8:46 AM, Doug Simon <[hidden email]> wrote:
>>>
>>> Ok, I’ve updated the webrev.
>>
>> Thanks, looks good!

+1

>>>
>>> I assume the missing change to DataBuilder would have been caught by AOT specific tests in jprt?
>>>
>>
>> Yes, I think it would’ve failed building on linux x64.

The original problem (with -Xcomp) was not caught by JPRT and Nightly testing because SQE stop running tests with -Xcomp
flag. Only PIT testing, once per week, run them.

But running RBT caught it - I hit it yesterday with testing jvmci module renaming.
So we need to run our changes through RBT before pushing them.

Thanks,
Vladimir

>
> Good to know ;-)
>
> Thanks for the review.
>
> -Doug
>
>>> -Doug
>>>
>>>> On 15 Feb 2017, at 17:34, Igor Veresov <[hidden email]> wrote:
>>>>
>>>> Sorry, it would also need a cast of course..
>>>>
>>>> diff --git a/src/jdk.aot/share/classes/jdk.tools.jaotc/src/jdk/tools/jaotc/DataBuilder.java b/src/jdk.aot/share/classes/jdk.tools.jaotc/src/jdk/tools/jaotc/DataBuilder.java
>>>> --- a/src/jdk.aot/share/classes/jdk.tools.jaotc/src/jdk/tools/jaotc/DataBuilder.java
>>>> +++ b/src/jdk.aot/share/classes/jdk.tools.jaotc/src/jdk/tools/jaotc/DataBuilder.java
>>>> @@ -70,8 +70,8 @@
>>>>     */
>>>>    private void fillVMAddresses(HotSpotVMConfigStore config) {
>>>>        for (VMField vmField : config.getFields().values()) {
>>>> -            if (vmField.value != null) {
>>>> -                final long address = vmField.value;
>>>> +            if (vmField.value != null && vmField.value instanceof Long) {
>>>> +                final long address = (Long) vmField.value;
>>>>                String value = vmField.name;
>>>>                /*
>>>>                 * Some fields don't contain addresses but integer values. At least don't add zero
>>>>
>>>>
>>>>> On Feb 15, 2017, at 8:32 AM, Igor Veresov <[hidden email]> wrote:
>>>>>
>>>>> Looks good, but I think the following bit would also be required to fix this particular issue:
>>>>>
>>>>> diff --git a/src/jdk.aot/share/classes/jdk.tools.jaotc/src/jdk/tools/jaotc/DataBuilder.java b/src/jdk.aot/share/classes/jdk.tools.jaotc/src/jdk/tools/jaotc/DataBuilder.java
>>>>> --- a/src/jdk.aot/share/classes/jdk.tools.jaotc/src/jdk/tools/jaotc/DataBuilder.java
>>>>> +++ b/src/jdk.aot/share/classes/jdk.tools.jaotc/src/jdk/tools/jaotc/DataBuilder.java
>>>>> @@ -70,7 +70,7 @@
>>>>>     */
>>>>>    private void fillVMAddresses(HotSpotVMConfigStore config) {
>>>>>        for (VMField vmField : config.getFields().values()) {
>>>>> -            if (vmField.value != null) {
>>>>> +            if (vmField.value != null && vmField.value instanceof Long) {
>>>>>                final long address = vmField.value;
>>>>>                String value = vmField.name;
>>>>>                /*
>>>>>
>>>>> Thanks,
>>>>> igor
>>>>>
>>>>>> On Feb 15, 2017, at 2:24 AM, Doug Simon <[hidden email]> wrote:
>>>>>>
>>>>>> Please review this fix for a bug introduced by JDK-8173912. The value written by readConfiguation (in jvmciCompilerToVM.cpp) to VMField.value may now be a Boolean. As such, the type of VMField.value must be Object.
>>>>>>
>>>>>> https://bugs.openjdk.java.net/browse/JDK-8174957
>>>>>> http://cr.openjdk.java.net/~dnsimon/8174957/webrev/
>>>>>>
>>>>>> -Doug
>>>>>
>>>>
>>>
>>
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR: 8174957: [JVMCI] jaotc is broken in Xcomp mode

Igor Veresov

> On Feb 15, 2017, at 10:16 AM, Vladimir Kozlov <[hidden email]> wrote:
>
> On 2/15/17 9:19 AM, Doug Simon wrote:
>>
>>> On 15 Feb 2017, at 18:05, Igor Veresov <[hidden email]> wrote:
>>>
>>>> On Feb 15, 2017, at 8:46 AM, Doug Simon <[hidden email]> wrote:
>>>>
>>>> Ok, I’ve updated the webrev.
>>>
>>> Thanks, looks good!
>
> +1
>
>>>>
>>>> I assume the missing change to DataBuilder would have been caught by AOT specific tests in jprt?
>>>>
>>>
>>> Yes, I think it would’ve failed building on linux x64.
>
> The original problem (with -Xcomp) was not caught by JPRT and Nightly testing because SQE stop running tests with -Xcomp flag. Only PIT testing, once per week, run them.
>
> But running RBT caught it - I hit it yesterday with testing jvmci module renaming.
> So we need to run our changes through RBT before pushing them.

It’s alright, I’ve manually tested this one. But, yes, I guess, running RBT won’t hurt in general.

igor


>
> Thanks,
> Vladimir
>
>>
>> Good to know ;-)
>>
>> Thanks for the review.
>>
>> -Doug
>>
>>>> -Doug
>>>>
>>>>> On 15 Feb 2017, at 17:34, Igor Veresov <[hidden email]> wrote:
>>>>>
>>>>> Sorry, it would also need a cast of course..
>>>>>
>>>>> diff --git a/src/jdk.aot/share/classes/jdk.tools.jaotc/src/jdk/tools/jaotc/DataBuilder.java b/src/jdk.aot/share/classes/jdk.tools.jaotc/src/jdk/tools/jaotc/DataBuilder.java
>>>>> --- a/src/jdk.aot/share/classes/jdk.tools.jaotc/src/jdk/tools/jaotc/DataBuilder.java
>>>>> +++ b/src/jdk.aot/share/classes/jdk.tools.jaotc/src/jdk/tools/jaotc/DataBuilder.java
>>>>> @@ -70,8 +70,8 @@
>>>>>    */
>>>>>   private void fillVMAddresses(HotSpotVMConfigStore config) {
>>>>>       for (VMField vmField : config.getFields().values()) {
>>>>> -            if (vmField.value != null) {
>>>>> -                final long address = vmField.value;
>>>>> +            if (vmField.value != null && vmField.value instanceof Long) {
>>>>> +                final long address = (Long) vmField.value;
>>>>>               String value = vmField.name;
>>>>>               /*
>>>>>                * Some fields don't contain addresses but integer values. At least don't add zero
>>>>>
>>>>>
>>>>>> On Feb 15, 2017, at 8:32 AM, Igor Veresov <[hidden email]> wrote:
>>>>>>
>>>>>> Looks good, but I think the following bit would also be required to fix this particular issue:
>>>>>>
>>>>>> diff --git a/src/jdk.aot/share/classes/jdk.tools.jaotc/src/jdk/tools/jaotc/DataBuilder.java b/src/jdk.aot/share/classes/jdk.tools.jaotc/src/jdk/tools/jaotc/DataBuilder.java
>>>>>> --- a/src/jdk.aot/share/classes/jdk.tools.jaotc/src/jdk/tools/jaotc/DataBuilder.java
>>>>>> +++ b/src/jdk.aot/share/classes/jdk.tools.jaotc/src/jdk/tools/jaotc/DataBuilder.java
>>>>>> @@ -70,7 +70,7 @@
>>>>>>    */
>>>>>>   private void fillVMAddresses(HotSpotVMConfigStore config) {
>>>>>>       for (VMField vmField : config.getFields().values()) {
>>>>>> -            if (vmField.value != null) {
>>>>>> +            if (vmField.value != null && vmField.value instanceof Long) {
>>>>>>               final long address = vmField.value;
>>>>>>               String value = vmField.name;
>>>>>>               /*
>>>>>>
>>>>>> Thanks,
>>>>>> igor
>>>>>>
>>>>>>> On Feb 15, 2017, at 2:24 AM, Doug Simon <[hidden email]> wrote:
>>>>>>>
>>>>>>> Please review this fix for a bug introduced by JDK-8173912. The value written by readConfiguation (in jvmciCompilerToVM.cpp) to VMField.value may now be a Boolean. As such, the type of VMField.value must be Object.
>>>>>>>
>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8174957
>>>>>>> http://cr.openjdk.java.net/~dnsimon/8174957/webrev/
>>>>>>>
>>>>>>> -Doug
>>>>>>
>>>>>
>>>>
>>>
>>

Loading...