On Wed, 3 May 2023 17:44:55 GMT, Maurizio Cimadamore <mcimadam...@openjdk.org> 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. This pull request has now been integrated. Changeset: 47422be2 Author: Maurizio Cimadamore <mcimadam...@openjdk.org> URL: https://git.openjdk.org/jdk/commit/47422be2d1d74e5e1b4b6c8e1a75e134e4f6aaf5 Stats: 85 lines in 6 files changed: 61 ins; 0 del; 24 mod 8307375: Alignment check on layouts used as sequence element is not correct Reviewed-by: jvernee ------------- PR: https://git.openjdk.org/jdk/pull/13784
