8260976: investigate ways to filter jextract output #469

mcimadamore · 2021-03-12T14:10:11Z

This patch adds new filtering capabilities to jextract, along the lines of what discussed in [1] (thanks Duncan for all the insightful comments on the topic!).

The patch adds some new options:

--include-{union, struct, var, function, typedef} = <symbol-name>

When one (or more) options like these are specified, jextract goes into some kind of avdanced filtering mode, and will only emit bindings for the specified symbols. Note: when no --include-xyz option is used, jextract behaves exactly like before - e.g. everything is extracted.

To help the user with the filtering process, another option has been added:

--dump-includes <file-name>

When this option is used, jextract will not emit any bindings - instead, it will dump all the included symbol on the specified file. The file is a simple text file, organized as follows (the following has been obtained extracting OpenGL):

#### Extracted from: /usr/include/KHR/khrplatform.h

--include-macro KHRONOS_BOOLEAN_ENUM_FORCE_SIZE    # header: /usr/include/KHR/khrplatform.h
--include-macro KHRONOS_FALSE                      # header: /usr/include/KHR/khrplatform.h
--include-macro KHRONOS_MAX_ENUM                   # header: /usr/include/KHR/khrplatform.h
--include-macro KHRONOS_SUPPORT_FLOAT              # header: /usr/include/KHR/khrplatform.h
--include-macro KHRONOS_SUPPORT_INT64              # header: /usr/include/KHR/khrplatform.h
--include-macro KHRONOS_TRUE                       # header: /usr/include/KHR/khrplatform.h
--include-typedef khronos_boolean_enum_t           # header: /usr/include/KHR/khrplatform.h
--include-typedef khronos_float_t                  # header: /usr/include/KHR/khrplatform.h
--include-typedef khronos_int16_t                  # header: /usr/include/KHR/khrplatform.h
--include-typedef khronos_int32_t                  # header: /usr/include/KHR/khrplatform.h
--include-typedef khronos_int64_t                  # header: /usr/include/KHR/khrplatform.h
--include-typedef khronos_int8_t                   # header: /usr/include/KHR/khrplatform.h
--include-typedef khronos_intptr_t                 # header: /usr/include/KHR/khrplatform.h
--include-typedef khronos_ssize_t                  # header: /usr/include/KHR/khrplatform.h
--include-typedef khronos_stime_nanoseconds_t      # header: /usr/include/KHR/khrplatform.h
--include-typedef khronos_uint16_t                 # header: /usr/include/KHR/khrplatform.h
--include-typedef khronos_uint32_t                 # header: /usr/include/KHR/khrplatform.h
--include-typedef khronos_uint64_t                 # header: /usr/include/KHR/khrplatform.h
--include-typedef khronos_uint8_t                  # header: /usr/include/KHR/khrplatform.h
--include-typedef khronos_uintptr_t                # header: /usr/include/KHR/khrplatform.h
--include-typedef khronos_usize_t                  # header: /usr/include/KHR/khrplatform.h
--include-typedef khronos_utime_nanoseconds_t      # header: /usr/include/KHR/khrplatform.h

#### Extracted from: /usr/include/alloca.h

--include-function alloca    # header: /usr/include/alloca.h
--include-macro _ALLOCA_H    # header: /usr/include/alloca.h

#### Extracted from: /usr/include/endian.h

--include-macro BIG_ENDIAN       # header: /usr/include/endian.h
--include-macro BYTE_ORDER       # header: /usr/include/endian.h
--include-macro LITTLE_ENDIAN    # header: /usr/include/endian.h
--include-macro PDP_ENDIAN       # header: /usr/include/endian.h
--include-macro _ENDIAN_H        # header: /usr/include/endian.h

In other words, the generated file contains all the symbols that have been inclided, in the form of "options". In fact, the generated file can be played back into jextract - assuming foo.conf is the name of our file, the we can do the following:

jextract @foo.conf Foo.h

Multiple @file can be used with jextract, and can also be mixed with explicit command-line options (all this support predates the work discussed in this patch, but makes it very worthwhile); this is also the same syntax supported by other launchers like javac.

The output file should be stable - e.g. all entries are grouped by header file (and header files are sorted alphabetially); whithin each header, entries are sorted by category (e.g. variable vs. function) and then also alphabetically within same category. This should help users finding things quickly.

In addition, each line has a comment which states from which header is the symbol coming from. I found this super helpful to define custom header-based filtering scheme using grep and regex (similar to what --filter used to do).

The implementation is relatively straightforward - other than adding support for the new option, it was mostly matter of making sure that OutputFactory does notgenerate the filtered out symbols.

[1] - https://mail.openjdk.java.net/pipermail/panama-dev/2021-March/012429.html

Progress

Change must not contain extraneous whitespace
Change must be properly reviewed

Issue

JDK-8260976: investigate ways to filter jextract output

Reviewers

Jorn Vernee (@JornVernee - Committer) ⚠️ Review applies to 5aed615

Download

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

Remove support for --filter

bridgekeeper · 2021-03-12T14:10:50Z

👋 Welcome back mcimadamore! A progress list of the required criteria for merging this PR into foreign-jextract will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

mlbridge · 2021-03-12T14:14:44Z

Webrevs

01: Full - Incremental (dc41d4e)
00: Full (5aed615)

JornVernee

LGTM.

One minor suggestion inline.

src/jdk.incubator.jextract/share/classes/jdk/internal/jextract/impl/IncludeHelper.java

openjdk · 2021-03-12T16:42:39Z

@mcimadamore This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8260976: investigate ways to filter jextract output

Reviewed-by: jvernee

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 88 new commits pushed to the foreign-jextract branch:

a48381b: Automatic merge of foreign-memaccess+abi into foreign-jextract
c7bfd8f: Automatic merge of master into foreign-memaccess+abi
9f32e80: Automatic merge of jdk:master into master
a9b156d: 8258414: OldObjectSample events too expensive
0bbe064: 8263354: Accumulated C2 code cleanups
aa33443: 8262454: Handshake timeout improvements, single target, kill unfinished thread
ff25939: 8263426: Reflow JfrNetworkUtilization::send_events
e25ad73: 8263430: Uninitialized Method* variables after JDK-8233913
9f6b1d7: 8263436: Silly array comparison in GaloisCounterMode.overlapDetection
ad1f605: 8263353: assert(CompilerOracle::option_matches_type(option, value)) failed: Value must match option type
... and 78 more: https://git.openjdk.java.net/panama-foreign/compare/a11bb7d2606543d43ddbda835ca7c4020b5abc8c...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 this automatic rebasing, please check the documentation for the /integrate command for further details.

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

mlbridge · 2021-03-14T13:39:08Z

Mailing list message from Duncan Gittins on panama-dev:

Maurizio

This works very well and I've abandoned my local jextract edits as I can
use jextract as-is to filter out unused symbols. The per line #header
comments are very helpful to filter with findstr/grep and to understand
where symbols come from.

As you mentioned in JDK-8260976, point #7 will help to warn if there are
un-used --includes, plus I think it would useful in all cases if
jextract reported a one line totals summary "Generated X symbols of Y,
filtered out (Y-X)" to give quick / easy feedback on the impact after a
command line or config file change.

Kind regards

Duncan

On 12/03/2021 14:14, Maurizio Cimadamore wrote:

mcimadamore · 2021-04-08T13:46:27Z

/integrate

openjdk · 2021-04-08T13:48:17Z

@mcimadamore Since your change was applied there have been 467 commits pushed to the foreign-jextract branch:

94b76fd: 8264887: Implicit scope objects are used with resource try
b93da9b: 8264825: update libclang bindings after recent jextract changes
eb6c197: 8264787: update jextract for foreign-memory-abi API changes and --enable-native-access option
389e95d: Automatic merge of foreign-memaccess+abi into foreign-jextract
b5b1ebd: Merge master
106de79: Automatic merge of jdk:master into master
7d0a0ba: 8264109: Add vectorized implementation for VectorMask.andNot()
0935eaa: 8264409: AArch64: generate better code for Vector API allTrue
4793557: 8264502: (fc) FileDispatcherImpl.setDirect0 might return uninitialized variable on some platforms
da643cd: 8264526: javax/swing/text/html/parser/Parser/8078268/bug8078268.java timeout
... and 457 more: https://git.openjdk.java.net/panama-foreign/compare/a11bb7d2606543d43ddbda835ca7c4020b5abc8c...foreign-jextract

Your commit was automatically rebased without conflicts.

Pushed as commit 04160de.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

mcimadamore added 5 commits March 11, 2021 19:03

Initial push

435e027

Cleanup ImportHelper support and OutputFactory

b9f6467

Add support for dumping configuration

dcd1ff5

Sort entries in include file alphabetically, and align them.

6051acf

Remove support for --filter

Add filtering test

5aed615

openjdk bot added the rfr Ready for review label Mar 12, 2021

JornVernee approved these changes Mar 12, 2021

View reviewed changes

src/jdk.incubator.jextract/share/classes/jdk/internal/jextract/impl/IncludeHelper.java Outdated Show resolved Hide resolved

src/jdk.incubator.jextract/share/classes/jdk/internal/jextract/impl/IncludeHelper.java Outdated Show resolved Hide resolved

openjdk bot added the ready Ready to be integrated label Mar 12, 2021

Address review comments

dc41d4e

openjdk bot closed this Apr 8, 2021

openjdk bot added integrated Pull request has been integrated and removed ready Ready to be integrated rfr Ready for review labels Apr 8, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8260976: investigate ways to filter jextract output #469

8260976: investigate ways to filter jextract output #469

mcimadamore commented Mar 12, 2021 •

edited by openjdk bot

Loading

bridgekeeper bot commented Mar 12, 2021

mlbridge bot commented Mar 12, 2021 •

edited

Loading

JornVernee left a comment

openjdk bot commented Mar 12, 2021 •

edited

Loading

mlbridge bot commented Mar 14, 2021

mcimadamore commented Apr 8, 2021

openjdk bot commented Apr 8, 2021

8260976: investigate ways to filter jextract output #469

8260976: investigate ways to filter jextract output #469

Conversation

mcimadamore commented Mar 12, 2021 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Download

bridgekeeper bot commented Mar 12, 2021

mlbridge bot commented Mar 12, 2021 • edited Loading

Webrevs

JornVernee left a comment

Choose a reason for hiding this comment

openjdk bot commented Mar 12, 2021 • edited Loading

mlbridge bot commented Mar 14, 2021

mcimadamore commented Apr 8, 2021

openjdk bot commented Apr 8, 2021

mcimadamore commented Mar 12, 2021 •

edited by openjdk bot

Loading

mlbridge bot commented Mar 12, 2021 •

edited

Loading

openjdk bot commented Mar 12, 2021 •

edited

Loading