On Mon, 30 Sep 2024 14:05:23 GMT, Vladimir Kozelkov <d...@openjdk.org> wrote:
> > abstract "reduction" rules
>
> I already use a similar "reduction" in stripNames(), but for all layouts, not
> just PaddingLayout. For example, nested structures and unions are unpacked to
> be flatter and the MethodHandle cache is more efficient. I like this approach.
It seems something that makes sense for the implementation to do (e.g. to
maximize number of cache hits). Not sure if e.g. `struct { x, y }` == `struct {
struct { x } { struct y } }` in all ABIs, so probably something that the linker
will want to do carefully.
It is also possible that maybe the javadoc could get away by just saying that
there must be some padding, w/o really having to specify how such padding
should be encoded (e.g. via a single padding layout, or multiple, or nested in
a sequence).
>
> > current PR to reject that
>
> In its current state it doesn't seem to do this. Will you be making further
> changes?
I believe we will need to make further changes based on your additional
examples. E.g. we need to decide how liberal we want to be with respect to
padding and then enforce that uniformly. I agree that the current patch doesn't
enforce the invariants I had in mind reliably.
>
> What about my examples 3 and 4 with overaligned unions? They look like a
> serious problem.
Alignment and padding is also a problem. In that you can't merge padding
layouts if the alignment of the merged parts is different (which is why I was
initially skeptical that we could capture all these rules in the javadoc).
Realistically, the Linker should probably reject any padding layout that has
alignment other than 1. As a stretch we could allow padding that is naturally
aligned (e.g. padding of size 4, aligned to 4), but even that seems a bit
overkill.
Also, in your union example there's another problem as well: the additional
padding field is redundant, at least according to the rules in the javadoc:
there's a single non-padding field with size 32 and alignment 8. I don't think
anything else is needed here.
-------------
PR Comment: https://git.openjdk.org/jdk/pull/21041#issuecomment-2383353460
