Emscripten: Warning on invalid exported runtime methods should be shown even when assertions are off

[Auron52]

Steps to Reproduce:

• Build the attached sample with the latest incoming Emscripten or look at the .js in the Output folder.
• Note that stringToUTF16 is not exported despite being included in the EXTRA_EXPORTED_RUNTIME_METHODS list

Previous tests on my side have revealed that

1. If all spaces within EXTRA_EXPORTED_RUNTIME_METHODS are removed everything works as expected.
2. All subsequent entries after the first space within EXTRA_EXPORTED_RUNTIME_METHODS are ignored.
3. EXPORTED_FUNCTIONS still works correctly even with spaces.

Thanks,

David

AnimalLibraryCommandLineBug.zip

[kripken]

I think this might be a regression from #6436. cc @DJMcNab

[DJMcNab]

I've worked out what's wrong. The two parts of the list are being in different argument postions. To fix this, you need can quote all of ['UTF16ToString', 'stringToUTF16'], removing the quotes around the values. I.e. '[UTF16ToString, stringToUTF16]'. This should allow quotes.

@kripken - In my mind this raises two issues. First of all, I would have expected the unlcosed list to trigger this assertion: https://github.com/kripken/emscripten/blob/a977b34fd94f6e16008ac6c742269ab4d31302e1/emcc.py#L2871, but it seems not to be. Have you got any idea why this might be?

Additionally, it brings up the idea of using multiple members of system.argv in the parse_value function. That would remove this issue, but it could also be a lot of added complexity to get right. Do you think it's worth it?

[kripken]

@DJMcNab I'm not sure why that assertion is not hit. I think as a first step we should debug that to see why it doesn't happen.

Doesn't your code already have the ability to handle multiple members of system.argv as necessary? I seem to remember that specifically was handled, did I get it wrong?

[DJMcNab]

Yeah, I probably should try to debug that. Unfortunately I am on a windows host and so am having trouble running the cmake stuff (no experience working with C++ either)

No, it didn't handle multiple argv members, but it did handle quoted strings with commas in lists correctly. The issue with using multiple argv members is that I'm not sure how I would increase the outer i counter variable. I will see if I can implement reading multiple argv members.

[DJMcNab]

First of all, I'm just going to say that I have literally no idea what I'm meant to be doing with cmake etc. The following is the best I can do to debug

When I run the command which it seems that cmake should be running:

emcc -s DISABLE_EXCEPTION_CATCHING=0 -s ALLOW_MEMORY_GROWTH=1 -s EXPORTED_FUNCTIONS="['_ANM_GetAge','_ANM_SetOwnerName','_ANM_CatCreatePersian', '_ANM_CatCreateScottishFold','_ANM_CatGetMeowIntensity','_ANM_CatGetSoftnessFactor','_ANM_CatGetWhiskerLength','_ANM_CatGetClawLength']" -s EXTRA_EXPORTED_RUNTIME_METHODS=['UTF16ToString', 'stringToUTF16'] -Oz libanimal_library.so --bind --llvm-lto 3 -s NO_EXIT_RUNTIME=1 -s TOTAL_MEMORY=50331648 -s RESERVED_FUNCTION_POINTERS=10 -s WASM=1 -s BINARYEN_METHOD='native-wasm' -o AnimalLibrary.js

I get the following error

ERROR:root:stringToUTF16]: No such file or directory ("stringToUTF16]" was expec                                                                                                                                                                                               ted to be an input file, based on the commandline arguments provided)

Which is from:
https://github.com/kripken/emscripten/blob/5874b9e88798981c788fbdd7050c95d9d871b3e6/emcc.py#L762.

No output files are created from this. Sorry @Auron52, but I cannot reproduce. Have you got the exact commands which you would run inside the AnimalLibraryCommandLineBug folder?

Additionally @kripken, I have looked into this some more, and it would require a massive refactor of command line argument handling to allow -s to eat multiple argv arguments, which I don't have time to do.
N.B. At the moment, the command line handling seems quite messy, so a cleanup might be in order anyway.

[Auron52]

Hmm, looks like I got bitten by a peculiarity of CMake.... This is the command line it runs:
emcc -s DISABLE_EXCEPTION_CATCHING=0 -s ALLOW_MEMORY_GROWTH=1 -s EXPORTED_FUNCTIONS="['_ANM_GetAge','_ANM_SetOwnerName','_ANM_CatCreatePersian', '_ANM_CatCreateScottishFold','_ANM_CatGetMeowIntensity','_ANM_CatGetSoftnessFactor','_ANM_CatGetWhiskerLength','_ANM_CatGetClawLength']" -s EXTRA_EXPORTED_RUNTIME_METHODS=['UTF16ToString',;'stringToUTF16'] -Oz libanimal_library.so --bind --llvm-lto 3 -s NO_EXIT_RUNTIME=1 -s TOTAL_MEMORY=50331648 -s RESERVED_FUNCTION_POINTERS=10 -s WASM=1 -s BINARYEN_METHOD='native-wasm' -o AnimalLibrary.js

As you can see it replaces the space with a ';' thereby leading to the problem. (incidentally this also explains why EXPORTED_FUNCTIONS was working since cmake will not do this silliness when the result is quoted. Overall while it might be nice to have the parser throw an error from or ignore the extra semicolon this is much less important than I first thought.

Thanks,

David

[DJMcNab]

OK. I'm still surprised an error wasn't thrown at some point though. I think that the EXTRA_EXPORTED_RUNTIME_METHODS should have had the value of ["UTF16ToString",";'stringToUTF16'"] when parsed.

I think that ;'stringToUTF16' not existing should then have thrown an error somewhere further, but this seems to be a bug elsewhere in the handling of EXTRA_EXPORTED_RUNTIME_METHODS.
Again, I have no experience compiling C/C++ programs, so someone else would have to create a minimal reproduction. It could even be as simple as something like emcc -s EXTRA_EXPORTED_RUNTIME_METHODS=[doesntexist], but I'm not sure.

[Auron52]

emcc checking EXTRA_EXPORTED_RUNTIME_METHODS for existence and throwing an error would indeed be ideal in my opinion. Currently it will accept anything you pass and silently ignore them.
Do you have any thoughts on this @kripken?
Thanks,
David

[kripken]

Hmm, we seem to already warn for this:

./emcc tests/hello_world.c -s 'EXTRA_EXPORTED_RUNTIME_METHODS=["foo"]'
warning: invalid item (maybe a typo?) in EXPORTED_RUNTIME_METHODS: foo

@Auron52 @DJMcNab Do you not see that warning?

[Auron52]

I don't see the warning. However, by removing the optimization flag I am able to get it to show up.
For example:
emcc -s EXTRA_EXPORTED_RUNTIME_METHODS=['meowsa'] libanimal_library.so -s WASM=1 -o MyOutput.js
Output ":warning: invalid item (maybe a typo?) in EXPORTED_RUNTIME_METHODS: meowsa"

emcc -s EXTRA_EXPORTED_RUNTIME_METHODS=['meowsa'] -Oz libanimal_library.so -s WASM=1 -o MyOutput.js
Output ""

Is there a known reason why this would occur?
Thanks,
David

[kripken]

Thanks @Auron52 - looks like when optimizations are on, we disable assertions, and that check is only done when assertions are on. That seems wrong, I think we should fix it so it's always shown.

[rla]

emcc.py function parse_string_list_members also cannot handle space (including newline) after last item and closing ], it causes warning to be emitted for the last quoted item, such as:

warning: invalid item (maybe a typo?) in EXPORTED_RUNTIME_METHODS: ccall"

The list in file could be formatted to have the closing ] on the last line of the file for better readability.

[DJMcNab]

Sorry again! Small typo, I think that https://github.com/kripken/emscripten/blob/e1d6f0dc3440e91cbf5d33ef982a38e0a091aba3/emcc.py#L3012 should be result.append(current.rstrip()[1:-1]) to fix this.

[kripken]

Thanks @DJMcNab, I opened #https://github.com/kripken/emscripten/pull/7067 with that.

[kripken]

Should be fixed by that PR.