8244244: Better representing typedef in jextract API

[slowhog]

Currently jextract API only present typedef declaration for record types, and using Declaration.Scoped to do so.

This makes downstream tools like jextract impossible to generate more comprehensive type. For example, if the tool would like to create a carrier type for size_t with similar name, because we pass through that, this is impossible.

This patch try to present typedef as Declaration.Variable with Kind TYPE. This will give us more transparency on dealing with typedef, and enable factoring an Declaration.Variable.TYPE from a Type.Delegated.TYPEDEF.

This would also be useful when a tool collecting type dependencies and encounter anonymous type or types without declaration, this gave the tool a chance to inject an implicit typedef to associate a type with a proper name.

There is one thing worth further discussion is that if we would like to consider generate array with typedef, currently the array element type won't be a Type.Delegated.TYPEDEF, but canonical type. The test case try to explain what should be expected for most use cases, if there is any possible way to use typedef, I'll try to add them.

---

Progress

• Change must not contain extraneous whitespace
• Change must be properly reviewed

Issue

• JDK-8244244: Better representing typedef in jextract API

Reviewers

• Maurizio Cimadamore (mcimadamore - Committer)
• Athijegannathan Sundararajan (sundar - Committer)

Download

$ git fetch https://git.openjdk.java.net/panama-foreign pull/137/head:pull/137
$ git checkout pull/137

[bridgekeeper]

👋 Welcome back henryjen! A progress list of the required criteria for merging this PR into foreign-jextract will be added to the body of your pull request.

[openjdk]

⚠️ @slowhog a branch with the same name as the source branch for this pull request (foreign-jextract) is present in the target repository. If you eventually integrate this pull request then the branch foreign-jextract in your personal fork will diverge once you sync your personal fork with the upstream repository.

To avoid this situation, create a new branch for your changes and reset the foreign-jextract branch. You can do this by running the following commands in a local repository for your personal fork. Note: you do not have to name the new branch NEW-BRANCH-NAME.

$ git checkout -b NEW-BRANCH-NAME
$ git branch -f foreign-jextract 73f6443c11651ef86ef7404e0fce7c5aaaa55ee6
$ git push -f origin foreign-jextract

Then proceed to create a new pull request with NEW-BRANCH-NAME as the source branch and close this one.

[mlbridge]

Webrevs

• 02: Full - Incremental (a8c6075)
• 01: Full - Incremental (2fa5f91)
• 00: Full (d5030a4)

[mcimadamore]

I disagree (pretty strongly) with the direction proposed in this patch. Making a typedef a variable is a workaround. Under no circumstances you can get a value out of a typedef - a typedef does not model a variable, and trying to bend the API to do so is, IMHO, wrong.

To me the big issue is that a typedef declaration, as it stands, is only good to wrap a declaration. But the solution I think, would be to have a more explicit TypeDef declaration tree, which has a name and a type (where the type can be a Declared, if need be).

[slowhog]

I disagree (pretty strongly) with the direction proposed in this patch. Making a typedef a variable is a workaround. Under no circumstances you can get a value out of a typedef - a typedef does not model a variable, and trying to bend the API to do so is, IMHO, wrong.

While I agree with your statement, the Declaration.Variable API doesn't present a value(), only Constant has a value().

In a sense, type() is the value of a Type declaration.

To me the big issue is that a typedef declaration, as it stands, is only good to wrap a declaration. But the solution I think, would be to have a more explicit TypeDef declaration tree, which has a name and a type (where the type can be a Declared, if need be).

As current Declaration.Variable stands, it's perfect, with name(), type() and kind(). We can either create a different interface, or rename to something else if that helps.

[mcimadamore]

As current Declaration.Variable stands, it's perfect, with name(), type() and kind(). We can either create a different interface, or rename to something else if that helps.

Variable is perfect in the sense that it has the right structure. But it's not at all what a typdef is! Imagine I write a visitor for the declaration API: if I see vistVariable and I'd expect to see a global, or a struct field, I would now have to defend against typedefs?

From a logical point of view a typedef has nothing to do with a variable declaration, so a new Declaration.Typedef interface would be preferrable

[mlbridge]

Mailing list message from Henry Jen on panama-dev:

On Apr 30, 2020, at 9:53 AM, Maurizio Cimadamore <mcimadamore at openjdk.java.net> wrote:

On Thu, 30 Apr 2020 15:00:24 GMT, Henry Jen <henryjen at openjdk.org> wrote:

As current Declaration.Variable stands, it's perfect, with name(), type() and kind(). We can either create a different
interface, or rename to something else if that helps.

Variable is perfect in the sense that it has the right structure.

Yes.

But it's not at all what a typdef is! Imagine I write
a visitor for the declaration API: if I see vistVariable and I'd expect to see a global, or a struct field, I would now
have to defend _against_ typedefs?

Regarding the defense, isn?t that the case already when we suppose to check kind() to do proper action?

However, I understand that by ?Variable?, we maybe thinking a classification that you will need to generate a getter/setter for its value, which won?t be the case for typedefs, but it is somewhat a stretch for PARAMETER as well.

From a logical point of view a typedef has nothing to do with a variable declaration, so a new `Declaration.Typedef`
interface would be preferrable

Sure. We should add a new Visitor method as well?

Cheers,
Henry

[mcimadamore]

Sure. We should add a new Visitor method as well?

I think so.

[mcimadamore]

There are many tests which still depend on Variable.kind.TYPE - how can they pass, given that the code never seem to create such vars?

[slowhog]

There are many tests which still depend on Variable.kind.TYPE - how can they pass, given that the code never seem to create such vars?

I didn't realizes TestAnonymousDecl.java was in there. That is something from another branch and have dependency on other part not included in this patch.

I must added by accident. Only TestTypedef.java is suppose to be added.

Strangely, I thought that should be caught by make run-test-jdk_jextract?

[openjdk]

@slowhog This change now passes all automated pre-integration checks, type /integrate in a new comment to proceed. After integration, the commit message will be:

8244244: Better representing typedef in jextract API

Reviewed-by: mcimadamore, sundar

• If you would like to add a summary, use the /summary command.
• To credit additional contributors, use the /contributor command.
• To add additional solved issues, use the /solves command.

Since the source branch of this PR was last updated there have been 130 commits pushed to the foreign-jextract branch:

• 629afd9: Merge foreign-abi
• 0783de0: Automatic merge of foreign-memaccess into foreign-abi
• 4397c2d: Automatic merge of master into foreign-memaccess
• ad3a6fa: Automatic merge of jdk:master into master
• 60b4157: 8243628: Deprecate -XX:ForceNUMA option
• 07cb35a: 8244087: 2020-04-24 public suffix list update
• bdbdb93: Fix foreign-jextract after latest library lookup changes
• b1cc9d1: Automatic merge of foreign-abi into foreign-jextract
• 7b6d993: JDK-8243669: Improve library loading for Panama libraries
• 908e576: 8219536: Add Option for user defined jlink options
• ... and 120 more: https://git.openjdk.java.net/panama-foreign/compare/73f6443c11651ef86ef7404e0fce7c5aaaa55ee6...foreign-jextract

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid automatic rebasing, please merge foreign-jextract into your branch, and then specify the current head hash when integrating, like this: /integrate 629afd98f16f0ffe6ad972417cad30ac787327b0.

➡️ To integrate this PR with the above commit message to the foreign-jextract branch, type /integrate in a new comment.

[mcimadamore]

Neat work, thanks

[slowhog]

/integrate

[openjdk]

@slowhog The following commits have been pushed to foreign-jextract since your change was applied:

• 629afd9: Merge foreign-abi
• 0783de0: Automatic merge of foreign-memaccess into foreign-abi
• 4397c2d: Automatic merge of master into foreign-memaccess
• ad3a6fa: Automatic merge of jdk:master into master
• 60b4157: 8243628: Deprecate -XX:ForceNUMA option
• 07cb35a: 8244087: 2020-04-24 public suffix list update
• bdbdb93: Fix foreign-jextract after latest library lookup changes
• b1cc9d1: Automatic merge of foreign-abi into foreign-jextract
• 7b6d993: JDK-8243669: Improve library loading for Panama libraries
• 908e576: 8219536: Add Option for user defined jlink options
• a0d04ad: 8244173: Uncomment subtest in runtime/InvocationTests/invocationC1Tests.java
• eddab11: 8225068: Remove DocuSign root certificate that is expiring in May 2020
• 2ebf5a2: 8244152: Remove unnecessary hash map resize in LocaleProviderAdapters
• a15b1ea: 8244210: The javac server is never used
• 2278680: 8241793: Shenandoah: Enable concurrent class unloading for aarch64
• 7f877f6: 8243495: Shenandoah: print root statistics for concurrent weak/strong root phases
• 38e6f36: 8244180: Shenandoah: carry Phase to ShWorkerTimingsTracker explicitly
• 1e1c724: 8243428: use reproducible random in :vmTestbase_vm_compiler
• a0ed53b: 8243427: use reproducible random in :vmTestbase_vm_mlvm
• c37bd06: 8244107: Incorrect parameters in ReservedSpace constructor change
• d74e4f2: 8243572: Multiple tests fail with assert(cld->klasses() != 0LL) failed: unexpected NULL for cld->klasses()
• a8f4c8d: 8244172: jextract should generate upcall stub creating helper in the functional interface class
• 5b86c4a: Merge
• 3153373: 8242898: Clean up InstanceKlass::_array_klasses
• 68e5306: 8240679: ZGC GarbageCollectorMXBean reports inaccurate post GC heap size for ZHeap pool
• 05b3bc5: 8243573: Shenandoah: rename GCParPhases and related code
• e513aca: 8214797: TestJmapCoreMetaspace.java timed out
• 46fe7e3: 8243945: use driver mode in runtime tests
• 175f02f: Automatic merge of foreign-abi into foreign-jextract
• f84874e: Automatic merge of foreign-memaccess into foreign-abi
• 98ad8aa: 8244128: Allocations larger than MAX_ALIGN can fail to be sliced to proper size.
• 27e1802: Automatic merge of foreign-abi into foreign-jextract
• 63c69a0: Automatic merge of foreign-memaccess into foreign-abi
• 7fad4ad: 8244127: "memory stomping error" when running java/foreign/TestNative.java on a debug build
• 079b2bc: 8244055: jextract should generate struct and union classes as nested classes of header file class
• e93cd7e: 8243155: AArch64: Add support for SqrtVF
• d813a88: Added tag jdk-15+21 for changeset 12b55fad80f3
• 8a7ff65: 8242522: Minor LingeredApp improvements
• 560da25: 8243598: Typos in java.lang.invoke package-info
• 5c70479: 8244061: Disable jvmci/graal/aot when building linux-aarch64 at Oracle
• 25e0f47: 8244051: AbsPathsInImage.java still fails on Windows
• bef54e0: 8243673: Mac signing process should not use --deep arg
• f0b37f1: 8239569: PublicMethodsTest.java failed due to NPE in java.base/java.nio.file.FileSystems.getFileSystem(FileSystems.java:230)
• e7aafcd: 8243933: use driver mode in gc tests
• 311c9ab: 8244052: remove copying of s.h.WB$WhiteBoxPermission in test/jdk
• d7b3692: 8243929: use @requires in serviceability/attach/AttachWithStalePidFile.java test
• 354033e: 8244097: make bootcycle-images fails after JDK-8244036
• 3cb0f00: 8242502: UnexpectedDeoptimizationTest.java failed "assert(phase->type(obj)->isa_oopptr()) failed: only for oop input"
• 478773c: 8243326: Cleanup use of volatile in taskqueue code
• 313758a: 8243489: Thread CPU Load event may contain wrong data for CPU time under certain conditions
• 5bbee05: 8243665: exploded-image-optimize touches module-info.class in all modules
• 46a67f4: 8243648: Windows 32bit compile error src/jdk.incubator.jpackage/windows/native/libjpackage/VersionInfo.cpp
• fe152cd: 8243666: ModuleHashes attribute generated for JMOD and JAR files depends on timestamps
• 35af52d: 8244010: Simplify usages of ProcessTools.createJavaProcessBuilder in our tests
• 60e2afe: 8243389: enhance os::pd_print_cpu_info on linux
• 0de9bbd: 8244044: Refactor phase makefiles to be structured per module
• 739e8e3: 8216557: Aarch64: Add support for Concurrent Class Unloading
• 408bc48: 8244036: Refresh SetupJavaCompilation, and remove support for sjavac
• 0783dd6: 8241807: JDWP needs update for hidden classes
• 7f49c91: 8244066: ClassFileInstaller should be run in driver mode
• a9d14e1: 8243944: use SkippedException and @requires in runtime/memory/ReadFromNoaccessArea.java test
• 6ff66db: 8242314: use reproducible random in vmTestbase shared code
• 9320f9c: 8243634: Add pandoc dependency when building linux-aarch64 at Oracle
• 70e632d: 8243935: remove copying of s.h.WB$WhiteBoxPermission in hotspot tests
• 6911667: 8243500: SA: Incorrect BCI and Line Number with jstack if the top frame is in the interpreter (BSD and Windows)
• 5d2740b: 8231634: SA stack walking fails with "illegal bci"
• 066346c: 8243541: (tz) Upgrade time-zone data to tzdata2020a
• f159234: 8243991: Remove obsolete -XX:ThreadStackSize from java command line
• 9687723: 8243932: compiler/codecache/cli/printcodecache/TestPrintCodeCacheOption.java test can use driver mode
• 9921097: 8243942: use SkippedException in gc/arguments/TestSmallInitialHeapWithLargePageAndNUMA.java test
• 3a416b9: 8243988: Added flexibility in build system for unusal hotspot configurations
• f4cb2bf: 8244009: Separate -Xdoclint options in CompileJavaModules.gmk
• 1b16192: 8243997: Linux build failed after JDK-8242244
• 04ae3fd: 8243848: Shenandoah: Windows build fails after JDK-8239786
• b723b94: 8244021: Hide warning from jlink about incubating modules
• 87f0ff6: 8243510: AbsPathsInImage.java fails on Windows
• 941643e: 8242921: test/hotspot/jtreg/runtime/CompactStrings/TestMethodNames.java uses nashorn script engine
• 09e8b7c: 8243985: Make source generation by generatecharacter reproducible
• c03a9bc: 8243973: Clarify difference between JAVA_OPTIONS and VM_OPTIONS
• 0b5f5d5: 8243982: Fix testing documentation after JDK-8240241
• 3ed0849: 8243393: Improve ReservedSpace constructor resolution
• a8ffbb3: 8243989: test/hotspot/jtreg/serviceability/jvmti/CanGenerateAllClassHook/CanGenerateAllClassHook.java needs to use othervm
• 1b0a423: 8242244: Remove redundant ELF machine definitions
• 184b433: 8242999: HTTP/2 client may not handle CONTINUATION frames correctly
• 7a937e0: 8243567: Update download link of jtreg provided by Adoption Group
• 6534414: 8243000: javac only build fails after removal of Nashorn
• 9cea1a5: 8243954: serviceability/logging/TestQuotedLogOutputs.java fails after 8243928
• 538e005: 8242459: ForceNUMA and only one available NUMA node hits a guarantee
• 1041efe: 8243946: serviceability/sa and jvmti tests fail after JDK-8243928
• ae9d563: 8243941: build issue introduced with the push of 8242237
• 3a9f764: 8243928: several svc tests can be run in driver mode
• 223ca80: 8242237: Improve JVM TI HiddenClasses tests
• a740f83: 8243930: update copyright years
• 18c4324: 8241815: Unnecessary calls to SystemDictionaryShared::define_shared_package
• 03f8e6c: 8219607: Add support in Graal and AOT for hidden class
• 68b189a: 8243633: Remove cups dependency when building linux at Oracle
• 9697772: 8243664: JavaDoc of CompactNumberFormat points to wrong enum
• c2d3ff3: 8242239: [Graal] javax/management/generified/GenericTest.java fails: FAILED: queryMBeans sets same
• d84e4f1: 8243565: some gc tests use 'test.java.opts' and not 'test.vm.opts'
• e0f46d0: 8243620: a few compiler/jvmci tests can be run in driver mode
• a3d14c6: 8243622: all actions in compiler/aot/fingerprint/SelfChangedCDS.java can be run in driver mode
• a075c32: 8243621: use SkippedException in compiler/jsr292/MHInlineTest.java test
• 83a6527: 8243619: compiler/codecache/CheckSegmentedCodeCache.java test misses -version
• 9097c79: 8243618: compiler/rtm/cli tests can be run w/o WhiteBox
• 27dc913: 8243617: compiler/onSpinWait/TestOnSpinWaitC1.java test uses wrong class
• bdf6726: 8243010: Test support: Customizable Hex Printer
• d2e0d0e: 8243469: Lazily encode name in ZipFile.getEntryPos
• c55e7d5: 8242034: Remove JRE_HOME references
• 0bbdcda: 8240783: JFR: TestClose could not finish chunk
• 32eb99e: 8243563: Doc comments cleanup
• 5d783f7: 8242846: Bring back test/jdk/tools/jlink/plugins/OrderResourcesPluginTest.java
• e34508c: 8242933: jdk/jfr/api/consumer/TestHiddenMethod uses nashorn script engine
• d07d6bd: 8243576: Remove residual reference to nashorn modules in make/CompileJavaModules.gmk
• 0e07f5a: 8151030: PPC64: AllocatePrefetchStyle=4 is out of range
• b2db7a0: 8243539: Copyright info (Year) should be updated for fix of 8241638
• 7fb4897: 8243587: Missing comma in copyright header
• 8065665: 8243568: serviceability/logging/TestLogRotation.java uses 'test.java.opts' and not 'test.vm.opts'
• 0fd64de: 8243591: Change to GCC 9.2 for building Linux/aarch64 at Oracle
• b0739f4: 8243590: Bump boot jdk to JDK 14 on aarch64 at Oracle
• 5fc5cb9: 8243549: sun/security/ssl/CipherSuite/NamedGroupsWithCipherSuite.java failed with Unsupported signature algorithm: DSA
• f8ab03f: 8243503: InstanceKlass::_array_name is not needed and leaks
• 05bf8dc: 8243578: Shenandoah: Cleanup ShenandoahStringDedup::parallel_oops_do()
• bfcb340: 8236464: SO_LINGER option is ignored by SSLSocket in JDK 11
• 88f3861: 8236129: Exe installers have wrong properties
• 33d9178: 8243574: java.lang.invoke.InvokerBytecodeGenerator.ClassData should be package-private
• e19d880: 8243575: Trivial javadoc fix of j.l.i.MethodHandles::arrayElementVarHandle
• b4615b1: 8243562: Make display of search results consistent with color scheme
• f9b816b: 8242649: improve the CSS class names used for summary and details tables
• 94a99ab: 8243246: HTTP Client sometimes gets java.io.IOException -> Invalid chunk header byte 32
• 04c6d13: 8241153: Refactor HeapRegionManager::find_unavailable_from_idx to simplify expand_at

Your commit was automatically rebased without conflicts.

Pushed as commit 067ccd7.