RFR: 7903602: Simplify handling of structs/unions

[Jorn Vernee]

On Wed, 6 Dec 2023 13:57:35 GMT, Maurizio Cimadamore <mcimadamore at openjdk.org> wrote:

> This PR simplifies how declarations and layouts for C structs/unions are created, by splitting the monolithic `RecordLayoutComputer` into two passes:
> 
> 1. As part of parsing (in `TreeMaker`) a scoped declaration is created for a struct, which contains all the relevant fields. This declaration is annotated with a bunch of attributes which reify some of the properties of the underlying clang cursors/types (such as sizes, alignments and offsets).
> 2. We now have a new static method in `Declaration`, namely `Declaration::layoutFor` which allows clients to obtain the layout for a given declaration. For most declaration, this method will get the type and then call `Type::layoutFor`. But for scoped declarations we will compute a new struct/union layout.
> 3. The already computed layouts are attached to the scoped declaration using an attribute, so that we avoid computing things multiple times.
> 
> I actually started off with a declaration visitor which attached layouts to all scoped declarations, but then reverted to a more lazy approach like the one described in (2) because this approach is more amenable for tests dynamically querying layouts of parsed declarations (whereas if layouts depended on a certain visitor being run after parsing, all such tests could not be supported).
> 
> Note that the logic which computes a struct/union layout is designed so that it always yields the same answer. That is, even if a client asks the layout for a nested anon struct first, the layout is computed correctly. This is possible because I've removed a lot of stateful logic from the computation, namely:
> 
> * the name of anonymous nested declaration is uniquely derived from the declaration position
> * the starting offset used for layout computation is derived from the offset of the first member of the scoped declaration (if we're seeing an anonymous struct, otherwise the starting offset is just zero)
> 
> An interesting consequence of this PR is that now the Declaration/Type API become layout-agnostic: while it's possible to compute a layout/function descriptor from a declaration or type, the declaration/type IRs do not embed any layout information. This change is also evident in the treatment for primitive types, which no longer have a pre-determined associated layout. That is, jextract declaration are now pure wrappers around clang cursors, and nothing more. Of course given the information in a declaration we can derive a layout, but that's an orthogonal operation: a declaration exists even w/o...

Marked as reviewed by jvernee (Committer).

src/main/java/org/openjdk/jextract/impl/MacroParserImpl.java line 340:

> 338:         void reparseMacros(boolean recovery) {
> 339:             String snippet = macroDecl(recovery);
> 340:             TreeMaker treeMaker = new TreeMaker(); // @@@: what about de-duplicated declarations?

Leftover comment? I don't think missing de-duplication is problematic here, since the trees that we create don't outlast the macro parser, so should never get to the point where we start annotating them.

src/main/java/org/openjdk/jextract/impl/TreeMaker.java line 264:

> 262:                         // process struct recursively
> 263:                         pendingFields.add(recordDeclaration(parent, fc).tree());
> 264:                     } else if (!fc.isBitField() && !fc.spelling().isEmpty()) {

The condition here seems to be unnecessary? We should already be in the no-bitfield case due to enclosing if/else, and the isAnonymousStruct check should filter out fields without a name?

src/main/java/org/openjdk/jextract/impl/TreeMaker.java line 305:

> 303:         if (c.isDefinition()) {
> 304:             //just a declaration AND definition, we have a layout
> 305:             return Declaration.enum_(CursorPosition.of(c), c.spelling(), decls.toArray(new Declaration[0]));

`TypeMaker.valueLayoutForSize` is now unused, so it can be removed.

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

PR Review: https://git.openjdk.org/jextract/pull/154#pullrequestreview-1768237165
PR Review Comment: https://git.openjdk.org/jextract/pull/154#discussion_r1420684899
PR Review Comment: https://git.openjdk.org/jextract/pull/154#discussion_r1420750055
PR Review Comment: https://git.openjdk.org/jextract/pull/154#discussion_r1420732738