Re: RFR: 8287244: Add bound check in indexed memory access var handle [v2]

2022-05-24 Thread Maurizio Cimadamore
On Tue, 24 May 2022 18:02:50 GMT, Maurizio Cimadamore  
wrote:

>> Sorry i misread the text, we are talking about the same thing. I think it 
>> would be clearer to refer `x_i` being in the range of `0` (inclusive) and 
>> `b_i` (exclusive), otherwise an  is thrown. That way in subsequent doc 
>> on other methods in matches with `B`, which is exclusive.
>
> yes, but that seems to affect this statement:
> 
> 
> `0 <= x_i <= b_i, where 0 <= i <= n`, or IndexOutOfBoundsException is thrown.
> 
> So we can replace with
> 
> 
> 0 <= x_i < b_i, where 1 <= i <= n`, or IndexOutOfBoundsException is thrown.

> Sorry i misread the text, we are talking about the same thing. I think it 
> would be clearer to refer `x_i` being in the range of `0` (inclusive) and 
> `b_i` (exclusive), otherwise an  is thrown. That way in subsequent doc on 
> other methods in matches with `B`, which is exclusive.

I apologize too - I misread your original question and thought it was about the 
use of ceilDiv :-) - anyway, at least that prodded me to come up with an 
example that explains why the logic is the way it is :-)

-

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


Re: RFR: 8287244: Add bound check in indexed memory access var handle [v2]

2022-05-24 Thread Maurizio Cimadamore
On Tue, 24 May 2022 18:00:46 GMT, Paul Sandoz  wrote:

>> The terms `x_1, x_2, ... x__n` are defined, but `x_0` is not. 
>> 
>> I think you can refer to the first index out of bounds as the exclusive 
>> upper bound of the range?
>
> Sorry i misread the text, we are talking about the same thing. I think it 
> would be clearer to refer `x_i` being in the range of `0` (inclusive) and 
> `b_i` (exclusive), otherwise an  is thrown. That way in subsequent doc on 
> other methods in matches with `B`, which is exclusive.

yes, but that seems to affect this statement:


`0 <= x_i <= b_i, where 0 <= i <= n`, or IndexOutOfBoundsException is thrown.

So we can replace with


0 <= x_i < b_i, where 1 <= i <= n`, or IndexOutOfBoundsException is thrown.

-

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


Re: RFR: 8287244: Add bound check in indexed memory access var handle [v2]

2022-05-24 Thread Paul Sandoz
On Tue, 24 May 2022 17:55:13 GMT, Paul Sandoz  wrote:

>> Here's a concrete example:
>> 
>> Consider a sequence layout with 6 elements. Then:
>> 
>> element count = 6
>> valid indices 0, 1, 2, 3, 4, 5
>> 
>> Now consider a var handle that is obtained by calling the path element 
>> method, passing the following parameters
>> 
>> start = 1
>> step = 2
>> 
>> This sets up the following mapping between logical an physical indices:
>> 
>> 0 -> 1
>> 1 -> 3
>> 2 -> 5
>> 
>> Where on the LHS we have the logical index (the one passed to the VH) and on 
>> the RHS we have the actual index it is translated to.
>> 
>> Note that the index map has three elements. So the upper bound (exclusive) 
>> of the index map is 3 - that is, we can pass indices 0, 1, 2.
>> 
>> According to the formula shown in the javadoc:
>> 
>> B = ceilDiv((elementCount - start) / step);
>> 
>> so:
>> 
>> B = ceilDiv((6 - 1) / 2)
>> = ceilDiv(5 / 2)
>> 
>> Note how, w/o ceilDiv we'd get 2 (the last valid index), and not 3 (the 
>> first invalid index).
>
> The terms `x_1, x_2, ... x__n` are defined, but `x_0` is not. 
> 
> I think you can refer to the first index out of bounds as the exclusive upper 
> bound of the range?

Sorry i misread the text, we are talking about the same thing. I think it would 
be clearer to refer `x_i` being in the range of `0` (inclusive) and `b_i` 
(exclusive), otherwise an  is thrown. That way in subsequent doc on other 
methods in matches with `B`, which is exclusive.

-

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


Re: RFR: 8287244: Add bound check in indexed memory access var handle [v2]

2022-05-24 Thread Paul Sandoz
On Tue, 24 May 2022 17:53:38 GMT, Maurizio Cimadamore  
wrote:

>> Indices start at zero. The ceilDiv operation is needed so that the operation 
>> returns the first index outisde the range (it's a bit subtle, sorry, but I 
>> don't know how else to express).
>
> Here's a concrete example:
> 
> Consider a sequence layout with 6 elements. Then:
> 
> element count = 6
> valid indices 0, 1, 2, 3, 4, 5
> 
> Now consider a var handle that is obtained by calling the path element 
> method, passing the following parameters
> 
> start = 1
> step = 2
> 
> This sets up the following mapping between logical an physical indices:
> 
> 0 -> 1
> 1 -> 3
> 2 -> 5
> 
> Where on the LHS we have the logical index (the one passed to the VH) and on 
> the RHS we have the actual index it is translated to.
> 
> Note that the index map has three elements. So the upper bound (exclusive) of 
> the index map is 3 - that is, we can pass indices 0, 1, 2.
> 
> According to the formula shown in the javadoc:
> 
> B = ceilDiv((elementCount - start) / step);
> 
> so:
> 
> B = ceilDiv((6 - 1) / 2)
> = ceilDiv(5 / 2)
> 
> Note how, w/o ceilDiv we'd get 2 (the last valid index), and not 3 (the first 
> invalid index).

The terms `x_1, x_2, ... x__n` are defined, but `x_0` is not. 

I think you can refer to the first index out of bounds as the exclusive upper 
bound of the range?

-

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


Re: RFR: 8287244: Add bound check in indexed memory access var handle [v2]

2022-05-24 Thread Maurizio Cimadamore
On Tue, 24 May 2022 17:43:52 GMT, Maurizio Cimadamore  
wrote:

>> src/java.base/share/classes/java/lang/foreign/MemoryLayout.java line 374:
>> 
>>> 372:  *
>>> 373:  * Additionally, the provided dynamic values must conform to some 
>>> bound which is derived from the layout path, that is,
>>> 374:  * {@code 0 <= x_i <= b_i}, where {@code 0 <= i <= n}, or {@link 
>>> IndexOutOfBoundsException} is thrown.
>> 
>> Suggestion:
>> 
>>  * {@code 0 <= x_i < b_i}, where {@code 1 <= i <= n}, or {@link 
>> IndexOutOfBoundsException} is thrown.
>> 
>> We refer later to `B` being an exclusive upper bound (computed using 
>> `ceilDiv`).
>
> Indices start at zero. The ceilDiv operation is needed so that the operation 
> returns the first index outisde the range (it's a bit subtle, sorry, but I 
> don't know how else to express).

Here's a concrete example:

Consider a sequence layout with 6 elements. Then:

element count = 6
valid indices 0, 1, 2, 3, 4, 5

Now consider a var handle that is obtained by calling the path element method, 
passing the following parameters

start = 1
step = 2

This sets up the following mapping between logical an physical indices:

0 -> 1
1 -> 3
2 -> 5

Where on the LHS we have the logical index (the one passed to the VH) and on 
the RHS we have the actual index it is translated to.

Note that the index map has three elements. So the upper bound (exclusive) of 
the index map is 3 - that is, we can pass indices 0, 1, 2.

According to the formula shown in the javadoc:

B = ceilDiv((elementCount - start) / step);

so:

B = ceilDiv((6 - 1) / 2)
= ceilDiv(5 / 2)

Note how, w/o ceilDiv we'd get 2 (the last valid index), and not 3 (the first 
invalid index).

-

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


Re: RFR: 8287244: Add bound check in indexed memory access var handle [v2]

2022-05-24 Thread Maurizio Cimadamore
On Tue, 24 May 2022 16:23:52 GMT, Paul Sandoz  wrote:

>> Maurizio Cimadamore has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   Tweak javadoc for ValueLayout::arrayElementVarHandle
>
> src/java.base/share/classes/java/lang/foreign/MemoryLayout.java line 374:
> 
>> 372:  *
>> 373:  * Additionally, the provided dynamic values must conform to some 
>> bound which is derived from the layout path, that is,
>> 374:  * {@code 0 <= x_i <= b_i}, where {@code 0 <= i <= n}, or {@link 
>> IndexOutOfBoundsException} is thrown.
> 
> Suggestion:
> 
>  * {@code 0 <= x_i < b_i}, where {@code 1 <= i <= n}, or {@link 
> IndexOutOfBoundsException} is thrown.
> 
> We refer later to `B` being an exclusive upper bound (computed using 
> `ceilDiv`).

Indices start at zero. The ceilDiv operation is needed so that the operation 
returns the first index outisde the range (it's a bit subtle, sorry, but I 
don't know how else to express).

-

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


Re: RFR: 8287244: Add bound check in indexed memory access var handle [v2]

2022-05-24 Thread Paul Sandoz
On Tue, 24 May 2022 15:28:27 GMT, Maurizio Cimadamore  
wrote:

>> Constructing indexed var handles using the `MemoryLayout` API produces 
>> `VarHandle` which do not check the input indices for out-of-bounds 
>> conditions.
>> While this can never result in a VM crash (after all the memory segment will 
>> protect against "true" OOB access), it is still possible for an access 
>> expression to refer to parts of a segment that are logically unrelated.
>> 
>> This patch adds a "logical" bound check to all indexed var handles generated 
>> using the layout API.
>> Benchmarks are not affected by the check. Users are still able to create 
>> custom "unchecked" var handles, using the combinator API in `MethodHandles`.
>
> Maurizio Cimadamore has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Tweak javadoc for ValueLayout::arrayElementVarHandle

src/java.base/share/classes/jdk/internal/foreign/LayoutPath.java line 256:

> 254: private long[] addBound(long maxIndex) {
> 255: long[] newBounds = new long[bounds.length + 1];
> 256: System.arraycopy(bounds, 0, newBounds, 0, bounds.length);

Suggestion:

long[] newBounds = Arrays.copyOf(bounds, bounds.length + 1);

-

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


Re: RFR: 8287244: Add bound check in indexed memory access var handle [v2]

2022-05-24 Thread Paul Sandoz
On Tue, 24 May 2022 15:28:27 GMT, Maurizio Cimadamore  
wrote:

>> Constructing indexed var handles using the `MemoryLayout` API produces 
>> `VarHandle` which do not check the input indices for out-of-bounds 
>> conditions.
>> While this can never result in a VM crash (after all the memory segment will 
>> protect against "true" OOB access), it is still possible for an access 
>> expression to refer to parts of a segment that are logically unrelated.
>> 
>> This patch adds a "logical" bound check to all indexed var handles generated 
>> using the layout API.
>> Benchmarks are not affected by the check. Users are still able to create 
>> custom "unchecked" var handles, using the combinator API in `MethodHandles`.
>
> Maurizio Cimadamore has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Tweak javadoc for ValueLayout::arrayElementVarHandle

src/java.base/share/classes/java/lang/foreign/MemoryLayout.java line 374:

> 372:  *
> 373:  * Additionally, the provided dynamic values must conform to some 
> bound which is derived from the layout path, that is,
> 374:  * {@code 0 <= x_i <= b_i}, where {@code 0 <= i <= n}, or {@link 
> IndexOutOfBoundsException} is thrown.

Suggestion:

 * {@code 0 <= x_i < b_i}, where {@code 1 <= i <= n}, or {@link 
IndexOutOfBoundsException} is thrown.

We refer later to `B` being an exclusive upper bound (computed using `ceilDiv`).

-

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


Re: RFR: 8287244: Add bound check in indexed memory access var handle [v2]

2022-05-24 Thread Maurizio Cimadamore
> Constructing indexed var handles using the `MemoryLayout` API produces 
> `VarHandle` which do not check the input indices for out-of-bounds conditions.
> While this can never result in a VM crash (after all the memory segment will 
> protect against "true" OOB access), it is still possible for an access 
> expression to refer to parts of a segment that are logically unrelated.
> 
> This patch adds a "logical" bound check to all indexed var handles generated 
> using the layout API.
> Benchmarks are not affected by the check. Users are still able to create 
> custom "unchecked" var handles, using the combinator API in `MethodHandles`.

Maurizio Cimadamore has updated the pull request incrementally with one 
additional commit since the last revision:

  Tweak javadoc for ValueLayout::arrayElementVarHandle

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/8868/files
  - new: https://git.openjdk.java.net/jdk/pull/8868/files/453392ae..a682cc03

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=8868=01
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8868=00-01

  Stats: 17 lines in 1 file changed: 16 ins; 0 del; 1 mod
  Patch: https://git.openjdk.java.net/jdk/pull/8868.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/8868/head:pull/8868

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