[foreign-jextract] RFR: JDK-8244270: reorganize ABI-dependent layout constants (second attempt)

[Henry Jen]

On Fri, 1 May 2020 21:45:03 GMT, Maurizio Cimadamore <mcimadamore at openjdk.org> wrote:

> This is another attempt to reorganize ABI-dependent layout constants. The constant reorg is identical to the one
> proposed in:
>  https://git.openjdk.java.net/panama-foreign/pull/97
> 
> But this time I've also integrated the changes into the jextract branch (as illustrated in this PR).
> 
> The main realization which led to this work was that we don't need the ABI type enum: we already have the jextract
> primitive type enum. So, we could just define constants for the common ABI types which are useful for ABI
> classification in SystemABI - and then we could start from here in jextract, and augment these layouts by attaching an
> extra attribute with jextract's Primitve/Kind enum.  I've simplified the API a bit by moving the layout inside the enum
> itself (so that the layout can be accessed directly from the enum constant) - and also simplified the primitive type
> factory.  Everything worked out as expected - note how much duplicated layout creation we managed to get rid of.
> One problem I had was that the loader we use inside jextract test was not delegating correctly (was using `null` as
> parent loader) so I had to fix that.
> I'm not 100% sure that this jextract/type extra annotation makes sense - at the end of the day it is now only used
> inside tests (as it makes layout comparison easier - since layouts might have additional attributes such as name and
> such).  So I'm open to suggestion on whether we want to keep it or not - I believe that tests which look for specific
> layouts should instead probably check for specific types instead. Anyway, we can also start from this cleanup, and take
> care of these issues later; the important thing is that, with this rewrite we are finally able to cleanup the ABI
> constants the way we wanted.

Overall I think this is a nice move, we want constants in SystemABI with proper MemoryLayout as we needed for access
ABI specific type.

I don't like the Utils.pick where we need to pass in all ABI candidates, we have picked an implementation class at that
point, we should be able to get those just from the implementation class. I would think instead of get rid of the enum,
we may actually hide the types internally and have each ABI class provide layout. However, this can be clean up later,
the important thing is that the constants are in place.

The other thing I don't think we should be doing is to make CLASS_ATTRIBUTE like "abi/sysv/class" part of the layout.

SystemABI constants should be a delegation to the implementation class, and strip the abi-class attribute. Trusting
this attribute from external is dangerous, a tool can compose a bad layout to do some harm?

Bottom line, type is reasonable to expose to public, ABI implementation detail such as classification is not.

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

PR: https://git.openjdk.java.net/panama-foreign/pull/141