holzschu / a-shell

A terminal for iOS, with multiple windows
BSD 3-Clause "New" or "Revised" License
2.62k stars 116 forks source link

wasm3 cannot output colorized text #807

Closed kkebo closed 2 months ago

kkebo commented 2 months ago

Description

When I run swift-format.wasm with wasm, it outputs colorized text as expected. However, when I run it with wasm3, it outputs non-colorized text.

swift-format.wasm can be downloaded here.

Screenshots

IMG_0467

Environment

holzschu commented 2 months ago

Hi, Thanks for the report. That is a very weird error, but I have the same behaviour over here. I don't see any processing of the output text in Wasm3 (nothing that would remove the control characters that produce the colors). I ran some tests, and the text out of m3_wasi_generic_fd_write() has no control characters in it, and the function is just a memory copy from input to output, so they likely weren't there in the first place.

My first guess would be that the issue is elsewhere, higher up (and more worrying). For example, if there is an environment variable that checks for the terminal abilities, and wasm3 did not provide the right response (even though I checked, and getenv() does work).

I'm going to keep investigating.

kkebo commented 2 months ago

Thank you for your hints. I will also take a closer look at the swift-format code.

holzschu commented 2 months ago

If I run wasm with an empty environment, it still outputs with colors, so it's not the environment (which is reassuring for me). The system calls in swift-format before it starts printing are:

libc stat Tips.swift
libc stat Tips.swift
libc stat Tips.swift
libc stat .
libc stat .
libc stat .
libc stat .
libc stat Tips.swift
libc open Tips.swift 0
libc fstat 19
libc stat Tips.swift
libc stat .swift-format
libc fstat 19
libc read 19 8192 null 0
libc read 19 8192 708 0
libc close 19
libc write 2 27,91,49,59,51,55,109,84,105,112,115,46,115,119,105,102,116,58,49,51,58,56,58,32 24 0 (...)

So basically: check that Tips.swift exists, check whether .swift-format exists, open Tips.swift, read it, close it, write down the results. None of these are going to change the formatting. So it has to be some of the functions inside the WASI shim that do not go through the prompt() mechanism for wasm.

holzschu commented 2 months ago

I've had a look inside @wasmer/wasi/lib/index.js. The functions that can be called, and don't trigger a call to prompt() are:

fd_advise
fd_allocate
fd_renumber
fd_seek
fd_tell
fd_sync
poll_oneoff

I don't think any of these is responsible. There's also default values for some variables that might be different.

kkebo commented 2 months ago

swift-format doesn't seem to think the standard error is connected to a terminal when I run it with wasm3.

https://github.com/swiftlang/swift-format/blob/ffc641e68d5e5948b8907fbe001b56210940c6ad/README.md?plain=1#L166

  • --color-diagnostics/--no-color-diagnostics: By default, swift-format will print diagnostics in color if standard error is connected to a terminal and without color otherwise (for example, if standard error is being redirected to a file). These flags can be used to force colors on or off respectively, regardless of whether the output is going to a terminal.

IMG_0469

kkebo commented 2 months ago

To detect whether the standard error is connected to a terminal or not, swift-format seems to use isatty.

kkebo commented 2 months ago

Then, isatty calls fd_fdstat_get inside.

https://github.com/WebAssembly/wasi-libc/blob/b9ef79d7dbd47c6c5bafdae760823467c2f60b70/libc-bottom-half/sources/isatty.c#L5

So I think the difference between the following two implementations may be the root cause.

kkebo commented 2 months ago

This line is probably the root cause.

fdstat->fs_rights_base = (uint64_t)-1; // all rights

Because wasm3 always permits all rights including seeking and telling even though fs_rights_base must not permit seeking and telling on the character device in isatty.

    // A tty is a character device that we can't seek or tell on.
    if (statbuf.fs_filetype != __WASI_FILETYPE_CHARACTER_DEVICE ||
        (statbuf.fs_rights_base & (__WASI_RIGHTS_FD_SEEK | __WASI_RIGHTS_FD_TELL)) != 0) {
        errno = __WASI_ERRNO_NOTTY;
        return 0;
    }
kkebo commented 2 months ago

Oops. I missed this line.

https://github.com/wasm3/wasm3/blob/139076a98b8321b67f850a844f558b5e91b5ac83/source/m3_api_wasi.c#L367

holzschu commented 2 months ago

Thanks a lot for this analysis. I agree with you that the issue is very likely inside m3_wasi_generic_fd_fdstat_get, but the code should not create this issue.

holzschu commented 2 months ago

Update: you were right about the cause, but it wasn't the rights, it was the filetype. I've added a line for:

fdstat->fs_filetype =  __WASI_FILETYPE_CHARACTER_DEVICE

to m3_wasi_generic_fd_fdstat_get() and it now works as expected. I'm updating the new version.

kkebo commented 2 months ago

Thank you so much!

kkebo commented 2 months ago

I have confirmed that I have fixed in a-Shell mini 1.15.2 (421), so I close this issue.

holzschu commented 1 month ago

The fix is not good enough: it says stdout/stdin is a TTY even if the input or output has been redirected. I've had "xz" complain that it cannot handle TTY input with "xz -dcf < file". I'll have to rework this.

kkebo commented 1 month ago

If you have to revert the fix, feel free to reopen this issue.

holzschu commented 1 month ago

It's an open-and-shut case: I did not revert the fix, but I added an if (ios_isatty(fd)) before it.