RFR: 7903449: Jextract generates structs that cannot be compiled
While most of the constants required by jextract are stored in the auxiliary constant$xyz files, some constants are stored inside the class declaration corresponding to a struct. This was done to minimize the number of constant classes being generated. However, this can backfire, as when the generated struct is big, too many constants are added to it, which can result in a source code which fails to compile (as it's too big). Instead of doing something even more convoluted, I've decided to centralize the generation of _all_ constants under the constant$xyz files. This means that there should be no constants generated in structs (or any other "public" declaration). Doing this was problematic, as the current arrangement somehow limits the name clashes when adding constants with same name to different structs. After some attempts, I realized that the best way forward would be to simplify the `ConstantBuilder` class, so that it no longer cares about "public" Java names. Instead that class should only be used to generate declaration for constants that are used internally, by the jextract bindings. Because of that, _any_ name will do - for this reason this patch switches to a regular naming schemes for all constants, like `const$0`, `const$1`, etc. Note that these are **not** the names exposed by jextract bindings - such names (the names of the constant *getters*) are still the same as before. The main changes are around `ConstantBuilder`, which is now gone. In its place we find `Constants`, which acts as a sort of source-based constant pool. `Constants` has a cache (where already defined constants of certain kinds are stored), and new constant declarations are only emitted when needed. I've also simplified the API which is used by clients to generate constants, to remove the need of lambda expressions (which seemed a bit over the top). I've also centralize the generation of numeric constants under `Constants` as well (before, these constants where generated on the fly in `HeaderFileBuilder`). And, I also realized that our setup for sharing primitive layout constants was too complex. Most primitive layout constants are just wired to some constant in `ValueLayout`. The only special case is `C_POINTER`, which needs its *target layout* set (and presumably we only want to do that once). A much simpler solution is to just store `C_POINTER` inside `RuntimeHelper` and just use that. ------------- Commit messages: - Further simplify code - Initial push Changes: https://git.openjdk.org/jextract/pull/117/files Webrev: https://webrevs.openjdk.org/?repo=jextract&pr=117&range=00 Issue: https://bugs.openjdk.org/browse/CODETOOLS-7903449 Stats: 1295 lines in 10 files changed: 525 ins; 646 del; 124 mod Patch: https://git.openjdk.org/jextract/pull/117.diff Fetch: git fetch https://git.openjdk.org/jextract.git pull/117/head:pull/117 PR: https://git.openjdk.org/jextract/pull/117
While most of the constants required by jextract are stored in the auxiliary constant$xyz files, some constants are stored inside the class declaration corresponding to a struct. This was done to minimize the number of constant classes being generated. However, this can backfire, as when the generated struct is big, too many constants are added to it, which can result in a source code which fails to compile (as it's too big).
Instead of doing something even more convoluted, I've decided to centralize the generation of _all_ constants under the constant$xyz files. This means that there should be no constants generated in structs (or any other "public" declaration).
Doing this was problematic, as the current arrangement somehow limits the name clashes when adding constants with same name to different structs. After some attempts, I realized that the best way forward would be to simplify the `ConstantBuilder` class, so that it no longer cares about "public" Java names. Instead that class should only be used to generate declaration for constants that are used internally, by the jextract bindings. Because of that, _any_ name will do - for this reason this patch switches to a regular naming schemes for all constants, like `const$0`, `const$1`, etc.
Note that these are **not** the names exposed by jextract bindings - such names (the names of the constant *getters*) are still the same as before.
The main changes are around `ConstantBuilder`, which is now gone. In its place we find `Constants`, which acts as a sort of source-based constant pool. `Constants` has a cache (where already defined constants of certain kinds are stored), and new constant declarations are only emitted when needed. I've also simplified the API which is used by clients to generate constants, to remove the need of lambda expressions (which seemed a bit over the top).
I've also centralize the generation of numeric constants under `Constants` as well (before, these constants where generated on the fly in `HeaderFileBuilder`).
And, I also realized that our setup for sharing primitive layout constants was too complex. Most primitive layout constants are just wired to some constant in `ValueLayout`. The only special case is `C_POINTER`, which needs its *target layout* set (and presumably we only want to do that once). A much simpler solution is to just store `C_POINTER` inside `RuntimeHelper` and just use that.
Maurizio Cimadamore has updated the pull request incrementally with one additional commit since the last revision: Add support for immediate constants ------------- Changes: - all: https://git.openjdk.org/jextract/pull/117/files - new: https://git.openjdk.org/jextract/pull/117/files/ae6bdfa8..438cf6c5 Webrevs: - full: https://webrevs.openjdk.org/?repo=jextract&pr=117&range=01 - incr: https://webrevs.openjdk.org/?repo=jextract&pr=117&range=00-01 Stats: 215 lines in 1 file changed: 115 ins; 68 del; 32 mod Patch: https://git.openjdk.org/jextract/pull/117.diff Fetch: git fetch https://git.openjdk.org/jextract.git pull/117/head:pull/117 PR: https://git.openjdk.org/jextract/pull/117
On Fri, 24 Mar 2023 15:57:29 GMT, Maurizio Cimadamore <mcimadamore@openjdk.org> wrote:
While most of the constants required by jextract are stored in the auxiliary constant$xyz files, some constants are stored inside the class declaration corresponding to a struct. This was done to minimize the number of constant classes being generated. However, this can backfire, as when the generated struct is big, too many constants are added to it, which can result in a source code which fails to compile (as it's too big).
Instead of doing something even more convoluted, I've decided to centralize the generation of _all_ constants under the constant$xyz files. This means that there should be no constants generated in structs (or any other "public" declaration).
Doing this was problematic, as the current arrangement somehow limits the name clashes when adding constants with same name to different structs. After some attempts, I realized that the best way forward would be to simplify the `ConstantBuilder` class, so that it no longer cares about "public" Java names. Instead that class should only be used to generate declaration for constants that are used internally, by the jextract bindings. Because of that, _any_ name will do - for this reason this patch switches to a regular naming schemes for all constants, like `const$0`, `const$1`, etc.
Note that these are **not** the names exposed by jextract bindings - such names (the names of the constant *getters*) are still the same as before.
The main changes are around `ConstantBuilder`, which is now gone. In its place we find `Constants`, which acts as a sort of source-based constant pool. `Constants` has a cache (where already defined constants of certain kinds are stored), and new constant declarations are only emitted when needed. I've also simplified the API which is used by clients to generate constants, to remove the need of lambda expressions (which seemed a bit over the top).
I've also centralize the generation of numeric constants under `Constants` as well (before, these constants where generated on the fly in `HeaderFileBuilder`).
And, I also realized that our setup for sharing primitive layout constants was too complex. Most primitive layout constants are just wired to some constant in `ValueLayout`. The only special case is `C_POINTER`, which needs its *target layout* set (and presumably we only want to do that once). A much simpler solution is to just store `C_POINTER` inside `RuntimeHelper` and just use that.
I've uploaded a new revision which improves how primitive constants (e.g. numerics) are handled. Now constants can be either "named" (e.g. a field in a class) or "immediate" (e.g. just an expression on the fly). This way, we can model certain constants in a more direct form, w/o the need of creating a new field. Generating fields for all basic primitive constants balooned up the number of constant classes in an unacceptable way. ------------- PR Comment: https://git.openjdk.org/jextract/pull/117#issuecomment-1483175315
While most of the constants required by jextract are stored in the auxiliary constant$xyz files, some constants are stored inside the class declaration corresponding to a struct. This was done to minimize the number of constant classes being generated. However, this can backfire, as when the generated struct is big, too many constants are added to it, which can result in a source code which fails to compile (as it's too big).
Instead of doing something even more convoluted, I've decided to centralize the generation of _all_ constants under the constant$xyz files. This means that there should be no constants generated in structs (or any other "public" declaration).
Doing this was problematic, as the current arrangement somehow limits the name clashes when adding constants with same name to different structs. After some attempts, I realized that the best way forward would be to simplify the `ConstantBuilder` class, so that it no longer cares about "public" Java names. Instead that class should only be used to generate declaration for constants that are used internally, by the jextract bindings. Because of that, _any_ name will do - for this reason this patch switches to a regular naming schemes for all constants, like `const$0`, `const$1`, etc.
Note that these are **not** the names exposed by jextract bindings - such names (the names of the constant *getters*) are still the same as before.
The main changes are around `ConstantBuilder`, which is now gone. In its place we find `Constants`, which acts as a sort of source-based constant pool. `Constants` has a cache (where already defined constants of certain kinds are stored), and new constant declarations are only emitted when needed. I've also simplified the API which is used by clients to generate constants, to remove the need of lambda expressions (which seemed a bit over the top).
I've also centralize the generation of numeric constants under `Constants` as well (before, these constants where generated on the fly in `HeaderFileBuilder`).
And, I also realized that our setup for sharing primitive layout constants was too complex. Most primitive layout constants are just wired to some constant in `ValueLayout`. The only special case is `C_POINTER`, which needs its *target layout* set (and presumably we only want to do that once). A much simpler solution is to just store `C_POINTER` inside `RuntimeHelper` and just use that.
Maurizio Cimadamore has updated the pull request incrementally with one additional commit since the last revision: Share more constants ------------- Changes: - all: https://git.openjdk.org/jextract/pull/117/files - new: https://git.openjdk.org/jextract/pull/117/files/438cf6c5..d789c752 Webrevs: - full: https://webrevs.openjdk.org/?repo=jextract&pr=117&range=02 - incr: https://webrevs.openjdk.org/?repo=jextract&pr=117&range=01-02 Stats: 54 lines in 4 files changed: 27 ins; 9 del; 18 mod Patch: https://git.openjdk.org/jextract/pull/117.diff Fetch: git fetch https://git.openjdk.org/jextract.git pull/117/head:pull/117 PR: https://git.openjdk.org/jextract/pull/117
On Fri, 24 Mar 2023 18:22:46 GMT, Maurizio Cimadamore <mcimadamore@openjdk.org> wrote:
While most of the constants required by jextract are stored in the auxiliary constant$xyz files, some constants are stored inside the class declaration corresponding to a struct. This was done to minimize the number of constant classes being generated. However, this can backfire, as when the generated struct is big, too many constants are added to it, which can result in a source code which fails to compile (as it's too big).
Instead of doing something even more convoluted, I've decided to centralize the generation of _all_ constants under the constant$xyz files. This means that there should be no constants generated in structs (or any other "public" declaration).
Doing this was problematic, as the current arrangement somehow limits the name clashes when adding constants with same name to different structs. After some attempts, I realized that the best way forward would be to simplify the `ConstantBuilder` class, so that it no longer cares about "public" Java names. Instead that class should only be used to generate declaration for constants that are used internally, by the jextract bindings. Because of that, _any_ name will do - for this reason this patch switches to a regular naming schemes for all constants, like `const$0`, `const$1`, etc.
Note that these are **not** the names exposed by jextract bindings - such names (the names of the constant *getters*) are still the same as before.
The main changes are around `ConstantBuilder`, which is now gone. In its place we find `Constants`, which acts as a sort of source-based constant pool. `Constants` has a cache (where already defined constants of certain kinds are stored), and new constant declarations are only emitted when needed. I've also simplified the API which is used by clients to generate constants, to remove the need of lambda expressions (which seemed a bit over the top).
I've also centralize the generation of numeric constants under `Constants` as well (before, these constants where generated on the fly in `HeaderFileBuilder`).
And, I also realized that our setup for sharing primitive layout constants was too complex. Most primitive layout constants are just wired to some constant in `ValueLayout`. The only special case is `C_POINTER`, which needs its *target layout* set (and presumably we only want to do that once). A much simpler solution is to just store `C_POINTER` inside `RuntimeHelper` and just use that.
Maurizio Cimadamore has updated the pull request incrementally with one additional commit since the last revision:
Share more constants
Very nice src/main/java/org/openjdk/jextract/impl/Constants.java line 423:
421: } 422: 423: private Constant emitConstantSegment(Object value) {
Maybe this should be renamed to `emitConstantString` (if you're here any way...) src/main/java/org/openjdk/jextract/impl/Constants.java line 506:
504: record DowncallKey(FunctionDescriptor desc) { } 505: DowncallKey downcallKey = new DowncallKey(descriptor); 506: Constant constant = cache.get(downcallKey);
Isn't dropping the names from the layouts required here as well? src/main/java/org/openjdk/jextract/impl/FunctionalInterfaceBuilder.java line 190:
188: private String prefixName() { 189: return qualifiedName(this) + "$FI"; 190: }
These seem to be unused? ------------- PR Review: https://git.openjdk.org/jextract/pull/117#pullrequestreview-1361422090 PR Review Comment: https://git.openjdk.org/jextract/pull/117#discussion_r1150842065 PR Review Comment: https://git.openjdk.org/jextract/pull/117#discussion_r1150840046 PR Review Comment: https://git.openjdk.org/jextract/pull/117#discussion_r1150845573
On Tue, 28 Mar 2023 15:55:35 GMT, Jorn Vernee <jvernee@openjdk.org> wrote:
Maurizio Cimadamore has updated the pull request incrementally with one additional commit since the last revision:
Share more constants
src/main/java/org/openjdk/jextract/impl/Constants.java line 506:
504: record DowncallKey(FunctionDescriptor desc) { } 505: DowncallKey downcallKey = new DowncallKey(descriptor); 506: Constant constant = cache.get(downcallKey);
Isn't dropping the names from the layouts required here as well?
Likely, if we want to deduplicate fully. I'm a bit worried about doing a full traversal and creating a brand new function descriptor just for the lookup though. I guess I could implement some custom equals method in the key. ------------- PR Review Comment: https://git.openjdk.org/jextract/pull/117#discussion_r1151221349
On Tue, 28 Mar 2023 22:42:37 GMT, Maurizio Cimadamore <mcimadamore@openjdk.org> wrote:
src/main/java/org/openjdk/jextract/impl/Constants.java line 506:
504: record DowncallKey(FunctionDescriptor desc) { } 505: DowncallKey downcallKey = new DowncallKey(descriptor); 506: Constant constant = cache.get(downcallKey);
Isn't dropping the names from the layouts required here as well?
Likely, if we want to deduplicate fully. I'm a bit worried about doing a full traversal and creating a brand new function descriptor just for the lookup though. I guess I could implement some custom equals method in the key.
After some more consideration, I decided to leave this as is. Reason being that the benefit would be somewhat limited - note that this method is only used for functional upcall interfaces. So, for this to make a difference you'd need to have several function pointers taking different structs by value with the same underlying layout (modulo names) - which seems a bit obscure. If needed we can fix later. ------------- PR Review Comment: https://git.openjdk.org/jextract/pull/117#discussion_r1153087263
While most of the constants required by jextract are stored in the auxiliary constant$xyz files, some constants are stored inside the class declaration corresponding to a struct. This was done to minimize the number of constant classes being generated. However, this can backfire, as when the generated struct is big, too many constants are added to it, which can result in a source code which fails to compile (as it's too big).
Instead of doing something even more convoluted, I've decided to centralize the generation of _all_ constants under the constant$xyz files. This means that there should be no constants generated in structs (or any other "public" declaration).
Doing this was problematic, as the current arrangement somehow limits the name clashes when adding constants with same name to different structs. After some attempts, I realized that the best way forward would be to simplify the `ConstantBuilder` class, so that it no longer cares about "public" Java names. Instead that class should only be used to generate declaration for constants that are used internally, by the jextract bindings. Because of that, _any_ name will do - for this reason this patch switches to a regular naming schemes for all constants, like `const$0`, `const$1`, etc.
Note that these are **not** the names exposed by jextract bindings - such names (the names of the constant *getters*) are still the same as before.
The main changes are around `ConstantBuilder`, which is now gone. In its place we find `Constants`, which acts as a sort of source-based constant pool. `Constants` has a cache (where already defined constants of certain kinds are stored), and new constant declarations are only emitted when needed. I've also simplified the API which is used by clients to generate constants, to remove the need of lambda expressions (which seemed a bit over the top).
I've also centralize the generation of numeric constants under `Constants` as well (before, these constants where generated on the fly in `HeaderFileBuilder`).
And, I also realized that our setup for sharing primitive layout constants was too complex. Most primitive layout constants are just wired to some constant in `ValueLayout`. The only special case is `C_POINTER`, which needs its *target layout* set (and presumably we only want to do that once). A much simpler solution is to just store `C_POINTER` inside `RuntimeHelper` and just use that.
Maurizio Cimadamore has updated the pull request incrementally with one additional commit since the last revision: Address review comments ------------- Changes: - all: https://git.openjdk.org/jextract/pull/117/files - new: https://git.openjdk.org/jextract/pull/117/files/d789c752..00bd3698 Webrevs: - full: https://webrevs.openjdk.org/?repo=jextract&pr=117&range=03 - incr: https://webrevs.openjdk.org/?repo=jextract&pr=117&range=02-03 Stats: 17 lines in 2 files changed: 0 ins; 15 del; 2 mod Patch: https://git.openjdk.org/jextract/pull/117.diff Fetch: git fetch https://git.openjdk.org/jextract.git pull/117/head:pull/117 PR: https://git.openjdk.org/jextract/pull/117
On Thu, 30 Mar 2023 11:01:43 GMT, Maurizio Cimadamore <mcimadamore@openjdk.org> wrote:
While most of the constants required by jextract are stored in the auxiliary constant$xyz files, some constants are stored inside the class declaration corresponding to a struct. This was done to minimize the number of constant classes being generated. However, this can backfire, as when the generated struct is big, too many constants are added to it, which can result in a source code which fails to compile (as it's too big).
Instead of doing something even more convoluted, I've decided to centralize the generation of _all_ constants under the constant$xyz files. This means that there should be no constants generated in structs (or any other "public" declaration).
Doing this was problematic, as the current arrangement somehow limits the name clashes when adding constants with same name to different structs. After some attempts, I realized that the best way forward would be to simplify the `ConstantBuilder` class, so that it no longer cares about "public" Java names. Instead that class should only be used to generate declaration for constants that are used internally, by the jextract bindings. Because of that, _any_ name will do - for this reason this patch switches to a regular naming schemes for all constants, like `const$0`, `const$1`, etc.
Note that these are **not** the names exposed by jextract bindings - such names (the names of the constant *getters*) are still the same as before.
The main changes are around `ConstantBuilder`, which is now gone. In its place we find `Constants`, which acts as a sort of source-based constant pool. `Constants` has a cache (where already defined constants of certain kinds are stored), and new constant declarations are only emitted when needed. I've also simplified the API which is used by clients to generate constants, to remove the need of lambda expressions (which seemed a bit over the top).
I've also centralize the generation of numeric constants under `Constants` as well (before, these constants where generated on the fly in `HeaderFileBuilder`).
And, I also realized that our setup for sharing primitive layout constants was too complex. Most primitive layout constants are just wired to some constant in `ValueLayout`. The only special case is `C_POINTER`, which needs its *target layout* set (and presumably we only want to do that once). A much simpler solution is to just store `C_POINTER` inside `RuntimeHelper` and just use that.
Maurizio Cimadamore has updated the pull request incrementally with one additional commit since the last revision:
Address review comments
Marked as reviewed by jvernee (Committer). ------------- PR Review: https://git.openjdk.org/jextract/pull/117#pullrequestreview-1364980281
On Fri, 24 Mar 2023 15:57:29 GMT, Maurizio Cimadamore <mcimadamore@openjdk.org> wrote:
While most of the constants required by jextract are stored in the auxiliary constant$xyz files, some constants are stored inside the class declaration corresponding to a struct. This was done to minimize the number of constant classes being generated. However, this can backfire, as when the generated struct is big, too many constants are added to it, which can result in a source code which fails to compile (as it's too big).
Instead of doing something even more convoluted, I've decided to centralize the generation of _all_ constants under the constant$xyz files. This means that there should be no constants generated in structs (or any other "public" declaration).
Doing this was problematic, as the current arrangement somehow limits the name clashes when adding constants with same name to different structs. After some attempts, I realized that the best way forward would be to simplify the `ConstantBuilder` class, so that it no longer cares about "public" Java names. Instead that class should only be used to generate declaration for constants that are used internally, by the jextract bindings. Because of that, _any_ name will do - for this reason this patch switches to a regular naming schemes for all constants, like `const$0`, `const$1`, etc.
Note that these are **not** the names exposed by jextract bindings - such names (the names of the constant *getters*) are still the same as before.
The main changes are around `ConstantBuilder`, which is now gone. In its place we find `Constants`, which acts as a sort of source-based constant pool. `Constants` has a cache (where already defined constants of certain kinds are stored), and new constant declarations are only emitted when needed. I've also simplified the API which is used by clients to generate constants, to remove the need of lambda expressions (which seemed a bit over the top).
I've also centralize the generation of numeric constants under `Constants` as well (before, these constants where generated on the fly in `HeaderFileBuilder`).
And, I also realized that our setup for sharing primitive layout constants was too complex. Most primitive layout constants are just wired to some constant in `ValueLayout`. The only special case is `C_POINTER`, which needs its *target layout* set (and presumably we only want to do that once). A much simpler solution is to just store `C_POINTER` inside `RuntimeHelper` and just use that.
This pull request has now been integrated. Changeset: f3c7c85f Author: Maurizio Cimadamore <mcimadamore@openjdk.org> URL: https://git.openjdk.org/jextract/commit/f3c7c85fa6a9385dd0e8f353bdb4b38f5c23... Stats: 1349 lines in 10 files changed: 576 ins; 647 del; 126 mod 7903449: Jextract generates structs that cannot be compiled Reviewed-by: jvernee ------------- PR: https://git.openjdk.org/jextract/pull/117
participants (2)
-
Jorn Vernee
-
Maurizio Cimadamore