Re: RFR: 8287244: Add bound check in indexed memory access var handle [v3]
On Tue, 24 May 2022 18:15:45 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: > > Address review comments Very nice! - Marked as reviewed by jvernee (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8868
Re: RFR: 8287244: Add bound check in indexed memory access var handle [v3]
On Tue, 24 May 2022 18:15:45 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: > > Address review comments Marked as reviewed by psandoz (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/8868
Re: RFR: 8287244: Add bound check in indexed memory access var handle [v3]
> 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: Address review comments - Changes: - all: https://git.openjdk.java.net/jdk/pull/8868/files - new: https://git.openjdk.java.net/jdk/pull/8868/files/a682cc03..66cf582a Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=8868&range=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=8868&range=01-02 Stats: 7 lines in 2 files changed: 1 ins; 2 del; 4 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
Re: RFR: 8287244: Add bound check in indexed memory access var handle [v2]
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]
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]
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]
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]
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]
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]
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]
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]
> 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&pr=8868&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=8868&range=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
Re: RFR: 8287244: Add bound check in indexed memory access var handle
On Tue, 24 May 2022 14:40:56 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`.
src/java.base/share/classes/java/lang/foreign/MemoryLayout.java line 537:
> 535: *
> 536: *
> 537: *if {@code F > 0}, then {@code B = ceilDiv(C - S,
> F)}
These formulas come from the formula for computing the accessed index A:
`A = S + I * F`
And then deriving the value for I, by equating `A = C` (for F > 0) and `A = -1`
(for F < 0) - that is equating the accessed index to the "first" out of bound
index. `ceilDiv` ensures there is "some room" between the max/min index and the
selected one.
src/java.base/share/classes/jdk/internal/foreign/LayoutPath.java line 109:
> 107: SequenceLayout seq = (SequenceLayout)layout;
> 108: checkSequenceBounds(seq, index);
> 109: long elemSize = seq.elementLayout().bitSize();
I've simplified the code here, as it still had traces of attempts to avoid the
call to `bitSize` (this method used to be partial).
-
PR: https://git.openjdk.java.net/jdk/pull/8868
Re: RFR: 8287244: Add bound check in indexed memory access var handle
On Tue, 24 May 2022 14:51:10 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`.
>
> src/java.base/share/classes/java/lang/foreign/MemoryLayout.java line 537:
>
>> 535: *
>> 536: *
>> 537: *if {@code F > 0}, then {@code B = ceilDiv(C - S,
>> F)}
>
> These formulas come from the formula for computing the accessed index A:
>
> `A = S + I * F`
>
> And then deriving the value for I, by equating `A = C` (for F > 0) and `A =
> -1` (for F < 0) - that is equating the accessed index to the "first" out of
> bound index. `ceilDiv` ensures there is "some room" between the max/min index
> and the selected one.
Note also that these complex bound calculation are performed statically, when
we build the layout path. When we're done, we just have an upper bound, which
we can check against using `Objects.checkIndex`.
-
PR: https://git.openjdk.java.net/jdk/pull/8868
RFR: 8287244: Add bound check in indexed memory access var handle
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`. - Commit messages: - Tweak javadoc - Merge branch 'master' into vh_bound_checks - Initial push Changes: https://git.openjdk.java.net/jdk/pull/8868/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=8868&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8287244 Stats: 73 lines in 3 files changed: 54 ins; 4 del; 15 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
