Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixing the FfmTerminal to run on JDK 22 and on Linux. #945

Merged
merged 3 commits into from
Apr 17, 2024

Conversation

lahodaj
Copy link
Contributor

@lahodaj lahodaj commented Mar 1, 2024

(As a foreword, I assume this PR will need some discussion and/or changes, as it changes the required JDK for build to JDK 22.)

I am working on an upgrade of the JLine inside the OpenJDK, and since JLine now has a Foreign Function and Memory (FFM) Terminal implementation, I would ideally want like to use that instead of the custom native code OpenJDK uses now.

Note the FFM was a preview API in JDK 21, and is final in JDK 22.

But, when I tried to use the FFM-base Terminal, there were some problems:

  • the API changed slightly:
    • a few methods (allocateArray, allocateUtf8String) got changed,
    • the VarHandles returned from MemoryLayout.varHandle(...) now accept one more "offset" parameter
  • the termios structure has a different layout and types than on Mac OS/X. The layout in JLine was the Mac OS/X layout, and this lead to some very weird behavior.

The changes proposed in this patch are:

  • the allocateArray, allocateUtf8String methods are changed to what I believe are their new versions
  • when creating the VarHandles, there's a utility that will inject offset 0L, to keep the handles easy to use
  • adding separate layouts for termios on Mac and on Linux. The provider will fail on other platforms, as I don't know what are the layout there - can be improved by someone who has access to other platforms.
  • in addition to the previous, on Linux, the VarHandles for termios are adapted from int to long, so they appear the same as on Mac to other code.
  • removed suppress warnings for "preview", and added suppress warnings for "restricted", which is a new javac warning in JDK 22.
  • in Kernel32, there was java.lang.foreign.MemorySegment length = arena.allocate(java.lang.foreign.ValueLayout.JAVA_INT, 0);, which is, as far as I can tell, used to store how many characters were read from the input. The 0 there does not make much sense to me, and it didn't work for me. Using 1 in this patch (effectively allocating an array of JAVA_INT of size 1, at least per my understanding).
  • adjusting the build to require and run using JDK 22. I assume this will need some discussion and/or changes.

Please let me know what you think! Thanks.

@lahodaj
Copy link
Contributor Author

lahodaj commented Mar 6, 2024

Added one more fix here:
bbe3c2a

the problem this is trying to fix is when (e.g.) the VINTR control character is set to some value (like 0):

        Attributes attrs = terminal.getAttributes();
        attrs.setControlChar(Attributes.ControlChar.VINTR, 0);
        terminal.setAttributes(attrs);

then the code attempts to write the value at the proper slot in the control characters array. And this works. But, on Linux, there is apparently no VDSUSP, and hence VDSUSP is never set by the code, so the value of that field is 0 (the default). And VINTR on Linux is also 0. So, after VINTR is written, this line is performed:

c_cc[VDSUSP] = (byte) t.getControlChar(Attributes.ControlChar.VDSUSP);

and that rewrites the value of VINTR with the value of t.getControlChar(Attributes.ControlChar.VDSUSP) (and something similar happens when reading the control characters from the array into Attributes). This then leads to an incorrect VINTR character, and misbehavior on Linux.

This change proposes to always set all the index variables for control characters, and just use -1 for those that don't exist on the given platform, and then not setting those/not reading those.

@gnodet
Copy link
Member

gnodet commented Mar 6, 2024

This looks good, I need to review more carefully, but this is definitely something which should get in.

@gnodet gnodet added this to the 3.26.0 milestone Mar 6, 2024
@@ -115,4 +119,13 @@ public int systemStreamWidth(SystemStream stream) {
public String toString() {
return "TerminalProvider[" + name() + "]";
}

static VarHandle lookupVarHandle(MemoryLayout layout, PathElement... element) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe name this layoutVarHandle or something?

@gnodet gnodet changed the title Fixing the FffmTerminal to run on JDK 22 and on Linux. Fixing the FfmTerminal to run on JDK 22 and on Linux. Mar 7, 2024
…inalProvider.java

Co-authored-by: ExE Boss <3889017+ExE-Boss@users.noreply.github.com>
@gnodet gnodet merged commit 9393ed6 into jline:master Apr 17, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants