### 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=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