On Wed, 3 May 2023 17:44:55 GMT, Maurizio Cimadamore <[email protected]>
wrote:
> This patch fixes `Utils::checkElementAlignment` to do the right thing for
> _all_ layouts.
>
> The current implementation is broken, as it only works correctly when the
> input layout is a value layout.
> Since value layouts have a size that is a power of two (and size all layouts
> have alignment that is also a power of two), then verifying that `size >
> alignment` works well.
>
> But if the input layout is some other layout (e.g. a `StructLayout`), this
> "power of two" assumption no longer holds. E.g. we can have a layout whose
> size is 48, and whose alignment is 32. While 48 is clearly bigger than 32,
> such a layout is still not suitable to be used as an element layout in a
> sequence.
>
> The fix is to provide two overloads for `Utils::checkElementAlignment` - one
> which works on `ValueLayout` and another which works on any `MemoryLayout`.
> The `ValueLayout` version works as before (so performance is not affected).
> The `MemoryLayout` variant would perform a full check using the `%` operator.
> Currently we only use this when creating a new sequence layout and when
> creating a stream out of a memory segment, so I'm not worried about potential
> performance regressions.
>
> I've fixed the javadoc so that the various `@throws` clauses in the affected
> methods reflect the correct behavior.
>
> Finally, I've made the existing alignment/layout tests a bit more robust, by
> also adding pair-wise combinations of layouts, wrapped in a struct/union.
> This does generate illegal layout cases which would not have been detected
> w/o this patch.
Marked as reviewed by jvernee (Reviewer).
src/java.base/share/classes/jdk/internal/foreign/Utils.java line 179:
> 177: public static void checkElementAlignment(ValueLayout layout, String
> msg) {
> 178: // Fast-path: if both size and alignment are powers of two, we
> can just
> 179: // check if one is greater than the other.
Maybe we could add an `assert` here to check that the size is actually a power
of two as well. Theoretically there are some value layouts for which is not the
case (`long double` is 12 bytes on certain systems).
-------------
PR Review: https://git.openjdk.org/jdk/pull/13784#pullrequestreview-1413047960
PR Review Comment: https://git.openjdk.org/jdk/pull/13784#discussion_r1184986370
