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

Asyncify lists does not work correctly with dots #5271

Closed
caiiiycuk opened this issue Nov 17, 2022 · 6 comments
Closed

Asyncify lists does not work correctly with dots #5271

caiiiycuk opened this issue Nov 17, 2022 · 6 comments

Comments

@caiiiycuk
Copy link
Contributor

Hi. I am using asyncify for jsdos project. And I found that ... is not handled correctly, for example I have function:

Program::WriteOut(char const*, ...)

When I compile I have warning:

warning: Asyncify onlylist contained a non-matching pattern: Program::WriteOut(char const*,    ) (Program::WriteOut\28char\20const*\2c\20\20\20\20\29)

As you can see dots are replaced with spaces:

expected (from wasm dis): Program::WriteOut\28char\20const*\2c\20...\29
actual                  : Program::WriteOut\28char\20const*\2c\20\20\20\20\29
@kripken
Copy link
Member

kripken commented Nov 17, 2022

Hmm, that could be a problem in wasm-ld where it demangles names into the names section, or it could be a problem in binaryen where it reads it. Can you look in the names section in the wasm binary (before binaryen runs) and see what appears there?

@kripken
Copy link
Member

kripken commented Nov 17, 2022

(I mean, to look with a hex editor - wasm-dis may be too late, if the bug is in binaryen parsing.)

@caiiiycuk
Copy link
Contributor Author

I have this in wdosbox-x.wasm:

F#Program::WriteOut(char const*, ...)Ø^F+Program::WriteOut(char const*, char const*)Ù^F.Program::WriteOut_NoParsing(char const*, bool)Ú^

@kripken
Copy link
Member

kripken commented Nov 17, 2022

Looks correct, thanks... ok, I dug into this now since it's clear it's either an emscripten or a binaryen issue. Turns out it's on the emscripten side,

emscripten-core/emscripten#13477

The function unmangle_symbols_from_cmdline replaces . with spaces,

https://github.com/emscripten-core/emscripten/blob/c215ec6ad3ded63c7e50cf818006ad1cac469b4a/emcc.py#L804-L806

It looks like . is part of the escaping approach there, but the problem is that . is also a necessary character sometimes, with things like ... that appear in signatures.

cc @juj - what do you think the best solution is? One idea might be to not run unmangle_symbols_from_cmdline if the input arrives from a response file. Then at least response files would be a way to know exactly what you are getting. But maybe the escaping can be tweaked?

@juj
Copy link
Collaborator

juj commented Nov 22, 2022

cc @juj - what do you think the best solution is? One idea might be to not run unmangle_symbols_from_cmdline if the input arrives from a response file. Then at least response files would be a way to know exactly what you are getting. But maybe the escaping can be tweaked?

Ouch, ellipsis never occurred to me, I thought I was taking three characters that were all never used in the LLVM name mangling.

Maybe it would be good idea not to unmangle if the input does come from a response file. That would lose the kind of "interchangeability", but given that the problem is a bit exotic shell expansion in the first place, maybe that would be ok.

Another way might be to look for another character than '.' to replace with a space, e.g. does - ever appear in a symbol name since it is an operator? Though that might become awkward, since it is a breaking change for existing users who might rely on that mangling. (maybe such users are very rare though)

@kripken
Copy link
Member

kripken commented Nov 28, 2022

Good point about interchangeability... yeah, special behavior for response files seems risky.

A breaking change might be an option. I hope it would affect few people while it unbreaks this.

Another option might be to special-case ..., that is to not unmangle a sequence of three dots. Or are there cases where three dots are needed in escaped form - I think no, since that would mean three spaces?

caiiiycuk added a commit to caiiiycuk/binaryen-fwasm-exceptions that referenced this issue Oct 25, 2023
caiiiycuk added a commit to caiiiycuk/binaryen-fwasm-exceptions that referenced this issue Oct 27, 2023
… file format for asyncify lists

Co-authored-by: Alon Zakai <alonzakai@gmail.com>
radekdoulik pushed a commit to dotnet/binaryen that referenced this issue Jul 12, 2024
…mbly#6051)

If there are newlines in the list, then we split using them in a simple manner
(that does not take into account nesting of any other delimiters).

Fixes WebAssembly#6047
Fixes WebAssembly#5271
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

No branches or pull requests

3 participants