RFR: 8307375: Alignment check on layouts used as sequence element is not correct [v2]

Maurizio Cimadamore mcimadamore at openjdk.org
Thu May 4 15:03:22 UTC 2023


> 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.

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

  Address review comments (add assert for power of two)

-------------

Changes:
  - all: https://git.openjdk.org/jdk/pull/13784/files
  - new: https://git.openjdk.org/jdk/pull/13784/files/1f2fd4b0..682bfaf1

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk&pr=13784&range=01
 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=13784&range=00-01

  Stats: 9 lines in 2 files changed: 7 ins; 0 del; 2 mod
  Patch: https://git.openjdk.org/jdk/pull/13784.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/13784/head:pull/13784

PR: https://git.openjdk.org/jdk/pull/13784


More information about the core-libs-dev mailing list