Quantcast

[1] RFR (S) 8179019: Correct range checks for command-line options ArraycopySrcPrefetchDistance and ArraycopyDstPrefetchDistance

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

[1] RFR (S) 8179019: Correct range checks for command-line options ArraycopySrcPrefetchDistance and ArraycopyDstPrefetchDistance

Zoltán Majó
Hi,


please review the following fix for 8179019.
https://bugs.openjdk.java.net/browse/JDK-8179019
http://cr.openjdk.java.net/~zmajo/8179019/webrev.00/

Currently, the HotSpot allows Arraycopy{Src|Dst}PrefetchDistance only
one value. Before JDK-8078554, values in the interval [0, 4096] were
allowed (without explicit checking, though).

This change extends the set of values allowed to the interval [0, 4031].
We need the high bound 4032 (instead of 4096) because of [1]: the
largest value of offset is 64, so 4031 + 64 = 4095 is the maximum that
fits into the signed range of 13 bits. The change also includes
suggestions by Vladimir K [2] to avoid using too large immediate values.

I tested the change manually with some values from both ends of the
range and also with JPRT (incl. the currently disabled
TestOptionsWithRanges.java test).

Thank you!

Best regards,


Zoltan

[1]
http://hg.openjdk.java.net/jdk10/hs/hotspot/file/1617b39a1ae4/src/cpu/sparc/vm/stubGenerator_sparc.cpp#l2128
[2]
https://bugs.openjdk.java.net/browse/JDK-8179019?focusedCommentId=14071776&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14071776

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

Re: [1] RFR (S) 8179019: Correct range checks for command-line options ArraycopySrcPrefetchDistance and ArraycopyDstPrefetchDistance

Zoltán Majó
P.S.: Sorry for the error in the subject: I suggest this change for JDK
10 (and not 1.0).

On 04/24/2017 05:20 PM, Zoltán Majó wrote:

> Hi,
>
>
> please review the following fix for 8179019.
> https://bugs.openjdk.java.net/browse/JDK-8179019
> http://cr.openjdk.java.net/~zmajo/8179019/webrev.00/
>
> Currently, the HotSpot allows Arraycopy{Src|Dst}PrefetchDistance only
> one value. Before JDK-8078554, values in the interval [0, 4096] were
> allowed (without explicit checking, though).
>
> This change extends the set of values allowed to the interval [0,
> 4031]. We need the high bound 4032 (instead of 4096) because of [1]:
> the largest value of offset is 64, so 4031 + 64 = 4095 is the maximum
> that fits into the signed range of 13 bits. The change also includes
> suggestions by Vladimir K [2] to avoid using too large immediate values.
>
> I tested the change manually with some values from both ends of the
> range and also with JPRT (incl. the currently disabled
> TestOptionsWithRanges.java test).
>
> Thank you!
>
> Best regards,
>
>
> Zoltan
>
> [1]
> http://hg.openjdk.java.net/jdk10/hs/hotspot/file/1617b39a1ae4/src/cpu/sparc/vm/stubGenerator_sparc.cpp#l2128
> [2]
> https://bugs.openjdk.java.net/browse/JDK-8179019?focusedCommentId=14071776&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14071776
>

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

Re: [1] RFR (S) 8179019: Correct range checks for command-line options ArraycopySrcPrefetchDistance and ArraycopyDstPrefetchDistance

Vladimir Kozlov
In reply to this post by Zoltán Majó
Good.

Thanks,
vladimir

On 4/24/17 8:20 AM, Zoltán Majó wrote:

> Hi,
>
>
> please review the following fix for 8179019.
> https://bugs.openjdk.java.net/browse/JDK-8179019
> http://cr.openjdk.java.net/~zmajo/8179019/webrev.00/
>
> Currently, the HotSpot allows Arraycopy{Src|Dst}PrefetchDistance only
> one value. Before JDK-8078554, values in the interval [0, 4096] were
> allowed (without explicit checking, though).
>
> This change extends the set of values allowed to the interval [0, 4031].
> We need the high bound 4032 (instead of 4096) because of [1]: the
> largest value of offset is 64, so 4031 + 64 = 4095 is the maximum that
> fits into the signed range of 13 bits. The change also includes
> suggestions by Vladimir K [2] to avoid using too large immediate values.
>
> I tested the change manually with some values from both ends of the
> range and also with JPRT (incl. the currently disabled
> TestOptionsWithRanges.java test).
>
> Thank you!
>
> Best regards,
>
>
> Zoltan
>
> [1]
> http://hg.openjdk.java.net/jdk10/hs/hotspot/file/1617b39a1ae4/src/cpu/sparc/vm/stubGenerator_sparc.cpp#l2128
>
> [2]
> https://bugs.openjdk.java.net/browse/JDK-8179019?focusedCommentId=14071776&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14071776
>
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [1] RFR (S) 8179019: Correct range checks for command-line options ArraycopySrcPrefetchDistance and ArraycopyDstPrefetchDistance

Zoltán Majó
Thank you, Vladimir.

Best regards,


Zoltan

On 04/25/2017 09:34 AM, Vladimir Kozlov wrote:

> Good.
>
> Thanks,
> vladimir
>
> On 4/24/17 8:20 AM, Zoltán Majó wrote:
>> Hi,
>>
>>
>> please review the following fix for 8179019.
>> https://bugs.openjdk.java.net/browse/JDK-8179019
>> http://cr.openjdk.java.net/~zmajo/8179019/webrev.00/
>>
>> Currently, the HotSpot allows Arraycopy{Src|Dst}PrefetchDistance only
>> one value. Before JDK-8078554, values in the interval [0, 4096] were
>> allowed (without explicit checking, though).
>>
>> This change extends the set of values allowed to the interval [0, 4031].
>> We need the high bound 4032 (instead of 4096) because of [1]: the
>> largest value of offset is 64, so 4031 + 64 = 4095 is the maximum that
>> fits into the signed range of 13 bits. The change also includes
>> suggestions by Vladimir K [2] to avoid using too large immediate values.
>>
>> I tested the change manually with some values from both ends of the
>> range and also with JPRT (incl. the currently disabled
>> TestOptionsWithRanges.java test).
>>
>> Thank you!
>>
>> Best regards,
>>
>>
>> Zoltan
>>
>> [1]
>> http://hg.openjdk.java.net/jdk10/hs/hotspot/file/1617b39a1ae4/src/cpu/sparc/vm/stubGenerator_sparc.cpp#l2128 
>>
>>
>> [2]
>> https://bugs.openjdk.java.net/browse/JDK-8179019?focusedCommentId=14071776&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14071776 
>>
>>
>>

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

Re: [1] RFR (S) 8179019: Correct range checks for command-line options ArraycopySrcPrefetchDistance and ArraycopyDstPrefetchDistance

Zoltán Majó
In reply to this post by Vladimir Kozlov
Hi Vladimir,


testing on all JPRT platforms (previously I tested only on SPARC)
revealed a small problem.

The flags we're checking are *unsigned* ints, therefore in
commandLineFlagConstraintsCompiler.cpp it is sufficient to check

if (value >= 4032) {

instead of

if (value < 0 || value >= 4032) {

The change is necessary to make gcc happy on linux_arm. (The '< 0' part
of the check is anyway performed when the value is read -- I verified).

Here's the updated webrev, please take a look:
http://cr.openjdk.java.net/~zmajo/8179019/webrev.01/

I re-tested with JPRT and did also some manual testing.

Thank you!

Best regards,


Zoltan

On 04/25/2017 09:34 AM, Vladimir Kozlov wrote:

> Good.
>
> Thanks,
> vladimir
>
> On 4/24/17 8:20 AM, Zoltán Majó wrote:
>> Hi,
>>
>>
>> please review the following fix for 8179019.
>> https://bugs.openjdk.java.net/browse/JDK-8179019
>> http://cr.openjdk.java.net/~zmajo/8179019/webrev.00/
>>
>> Currently, the HotSpot allows Arraycopy{Src|Dst}PrefetchDistance only
>> one value. Before JDK-8078554, values in the interval [0, 4096] were
>> allowed (without explicit checking, though).
>>
>> This change extends the set of values allowed to the interval [0, 4031].
>> We need the high bound 4032 (instead of 4096) because of [1]: the
>> largest value of offset is 64, so 4031 + 64 = 4095 is the maximum that
>> fits into the signed range of 13 bits. The change also includes
>> suggestions by Vladimir K [2] to avoid using too large immediate values.
>>
>> I tested the change manually with some values from both ends of the
>> range and also with JPRT (incl. the currently disabled
>> TestOptionsWithRanges.java test).
>>
>> Thank you!
>>
>> Best regards,
>>
>>
>> Zoltan
>>
>> [1]
>> http://hg.openjdk.java.net/jdk10/hs/hotspot/file/1617b39a1ae4/src/cpu/sparc/vm/stubGenerator_sparc.cpp#l2128 
>>
>>
>> [2]
>> https://bugs.openjdk.java.net/browse/JDK-8179019?focusedCommentId=14071776&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14071776 
>>
>>
>>

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

Re: [1] RFR (S) 8179019: Correct range checks for command-line options ArraycopySrcPrefetchDistance and ArraycopyDstPrefetchDistance

Vladimir Kozlov
On 4/26/17 5:15 AM, Zoltán Majó wrote:

> Hi Vladimir,
>
>
> testing on all JPRT platforms (previously I tested only on SPARC)
> revealed a small problem.
>
> The flags we're checking are *unsigned* ints, therefore in
> commandLineFlagConstraintsCompiler.cpp it is sufficient to check
>
> if (value >= 4032) {

okay

>
> instead of
>
> if (value < 0 || value >= 4032) {
>
> The change is necessary to make gcc happy on linux_arm. (The '< 0' part
> of the check is anyway performed when the value is read -- I verified).
>
> Here's the updated webrev, please take a look:
> http://cr.openjdk.java.net/~zmajo/8179019/webrev.01/

Good.

Thanks,
Vladimir

>
> I re-tested with JPRT and did also some manual testing.
>
> Thank you!
>
> Best regards,
>
>
> Zoltan
>
> On 04/25/2017 09:34 AM, Vladimir Kozlov wrote:
>> Good.
>>
>> Thanks,
>> vladimir
>>
>> On 4/24/17 8:20 AM, Zoltán Majó wrote:
>>> Hi,
>>>
>>>
>>> please review the following fix for 8179019.
>>> https://bugs.openjdk.java.net/browse/JDK-8179019
>>> http://cr.openjdk.java.net/~zmajo/8179019/webrev.00/
>>>
>>> Currently, the HotSpot allows Arraycopy{Src|Dst}PrefetchDistance only
>>> one value. Before JDK-8078554, values in the interval [0, 4096] were
>>> allowed (without explicit checking, though).
>>>
>>> This change extends the set of values allowed to the interval [0, 4031].
>>> We need the high bound 4032 (instead of 4096) because of [1]: the
>>> largest value of offset is 64, so 4031 + 64 = 4095 is the maximum that
>>> fits into the signed range of 13 bits. The change also includes
>>> suggestions by Vladimir K [2] to avoid using too large immediate values.
>>>
>>> I tested the change manually with some values from both ends of the
>>> range and also with JPRT (incl. the currently disabled
>>> TestOptionsWithRanges.java test).
>>>
>>> Thank you!
>>>
>>> Best regards,
>>>
>>>
>>> Zoltan
>>>
>>> [1]
>>> http://hg.openjdk.java.net/jdk10/hs/hotspot/file/1617b39a1ae4/src/cpu/sparc/vm/stubGenerator_sparc.cpp#l2128
>>>
>>>
>>> [2]
>>> https://bugs.openjdk.java.net/browse/JDK-8179019?focusedCommentId=14071776&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14071776
>>>
>>>
>>>
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [1] RFR (S) 8179019: Correct range checks for command-line options ArraycopySrcPrefetchDistance and ArraycopyDstPrefetchDistance

Zoltán Majó
Hi Vladimir,


On 04/26/2017 07:09 PM, Vladimir Kozlov wrote:
> On 4/26/17 5:15 AM, Zoltán Majó wrote:
>> [...]
>> Here's the updated webrev, please take a look:
>> http://cr.openjdk.java.net/~zmajo/8179019/webrev.01/
>
> Good.

Thank you for the review!

Best regards,


Zoltan

>
>
> Thanks,
> Vladimir
>
>>
>> I re-tested with JPRT and did also some manual testing.
>>
>> Thank you!
>>
>> Best regards,
>>
>>
>> Zoltan
>>
>> On 04/25/2017 09:34 AM, Vladimir Kozlov wrote:
>>> Good.
>>>
>>> Thanks,
>>> vladimir
>>>
>>> On 4/24/17 8:20 AM, Zoltán Majó wrote:
>>>> Hi,
>>>>
>>>>
>>>> please review the following fix for 8179019.
>>>> https://bugs.openjdk.java.net/browse/JDK-8179019
>>>> http://cr.openjdk.java.net/~zmajo/8179019/webrev.00/
>>>>
>>>> Currently, the HotSpot allows Arraycopy{Src|Dst}PrefetchDistance only
>>>> one value. Before JDK-8078554, values in the interval [0, 4096] were
>>>> allowed (without explicit checking, though).
>>>>
>>>> This change extends the set of values allowed to the interval [0,
>>>> 4031].
>>>> We need the high bound 4032 (instead of 4096) because of [1]: the
>>>> largest value of offset is 64, so 4031 + 64 = 4095 is the maximum that
>>>> fits into the signed range of 13 bits. The change also includes
>>>> suggestions by Vladimir K [2] to avoid using too large immediate
>>>> values.
>>>>
>>>> I tested the change manually with some values from both ends of the
>>>> range and also with JPRT (incl. the currently disabled
>>>> TestOptionsWithRanges.java test).
>>>>
>>>> Thank you!
>>>>
>>>> Best regards,
>>>>
>>>>
>>>> Zoltan
>>>>
>>>> [1]
>>>> http://hg.openjdk.java.net/jdk10/hs/hotspot/file/1617b39a1ae4/src/cpu/sparc/vm/stubGenerator_sparc.cpp#l2128 
>>>>
>>>>
>>>>
>>>> [2]
>>>> https://bugs.openjdk.java.net/browse/JDK-8179019?focusedCommentId=14071776&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14071776 
>>>>
>>>>
>>>>
>>>>
>>

Loading...