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

EMSDK 3.1.14: no main argv with PROXY_TO_PTHREAD #17338

Closed
tiran opened this issue Jun 28, 2022 · 4 comments · Fixed by #17340
Closed

EMSDK 3.1.14: no main argv with PROXY_TO_PTHREAD #17338

tiran opened this issue Jun 28, 2022 · 4 comments · Fixed by #17340

Comments

@tiran
Copy link
Contributor

tiran commented Jun 28, 2022

EMSDK 3.1.14 no longer passes any argc / argv to the program with linker flag -sPROXY_TO_PTHREAD. This used to work fine with 3.1.13 and earlier versions of Emscripten. It is trival to reproduce with a C program that prints argc.

Version of emscripten/emsdk:

emcc (Emscripten gcc/clang-like replacement + linker emulating GNU ld) 3.1.14 (4343cbe)
clang version 15.0.0 (https://github.com/llvm/llvm-project 7effcbda49ba32991b8955821b8fdbd4f8f303e2)
Target: wasm32-unknown-emscripten
Thread model: posix
InstalledDir: /opt/emsdk/upstream/bin

Failing command line in full:

#include <stdio.h>

int main(int argc, char *argv[])
{
        printf("argc: %i\n", argc);
        return 0;
}
$ emcc -pthread -sUSE_PTHREADS -sPROXY_TO_PTHREAD -sEXIT_RUNTIME main.c -o main.js
$ node --experimental-wasm-threads --experimental-wasm-bulk-memory main.js egg spam
argc: 0
$ emcc -pthread -sUSE_PTHREADS -sEXIT_RUNTIME main.c -o main.js
$ node --experimental-wasm-threads --experimental-wasm-bulk-memory main.js egg spam
argc: 3
@tiran
Copy link
Contributor Author

tiran commented Jun 28, 2022

WIth emscripten/emsdk:3.1.13 container:

# emcc -v
emcc (Emscripten gcc/clang-like replacement + linker emulating GNU ld) 3.1.13 (531257621816c200bc7c3be53129494afd029aec)
clang version 15.0.0 (https://github.com/llvm/llvm-project 5c6ed60c517c47b25b6b25d8ac3666d0e746b0c3)
Target: wasm32-unknown-emscripten
Thread model: posix
InstalledDir: /emsdk/upstream/bin
# emcc -pthread -sUSE_PTHREADS -sPROXY_TO_PTHREAD -sEXIT_RUNTIME main.c -o main.js
# node --experimental-wasm-threads --experimental-wasm-bulk-memory main.js egg spam
argc: 3

@tiran
Copy link
Contributor Author

tiran commented Jun 28, 2022

In 3.1.14 the callMain function no longer sets up argc and argv like in 3.1.13

function callMain(args) {
  assert(runDependencies == 0, 'cannot call main when async dependencies remain! (listen on Module["onRuntimeInitialized"])');
  assert(__ATPRERUN__.length == 0, 'cannot call main when preRun functions remain to be called');

  // User requested the PROXY_TO_PTHREAD option, so call a stub main which pthread_create()s a new thread
  // that will call the user's real main() for the application.
  var entryFunction = Module['__emscripten_proxy_main'];

  var argc = 0;
  var argv = 0;

  try {

    var ret = entryFunction(argc, argv);

    // In PROXY_TO_PTHREAD builds, we should never exit the runtime below, as
    // execution is asynchronously handed off to a pthread.
    assert(ret == 0, '_emscripten_proxy_main failed to start proxy thread: ' + ret);
  } finally {
    calledMain = true;

  }
}

3.1.13 had this block:

  args = args || [];
  args.unshift(thisProgram);

  var argc = args.length;
  var argv = stackAlloc((argc + 1) * 4);
  var argv_ptr = argv >> 2;
  args.forEach((arg) => {
    HEAP32[argv_ptr++] = allocateUTF8OnStack(arg);
  });
  HEAP32[argv_ptr] = 0;

If I patch the block into my JS file, then argument passing works again.

@tiran
Copy link
Contributor Author

tiran commented Jun 28, 2022

I suspect that the issue might be related to MAIN_READS_PARAMS setting and new name mangling of __main_argc_argv.

@kripken
Copy link
Member

kripken commented Jun 28, 2022

Bisected to

98507fab7b60292533a17cb935c36ccd439694c9 is the first bad commit
commit 98507fab7b60292533a17cb935c36ccd439694c9
Author: chromium-autoroll <[email protected]>
Date:   Fri Jun 10 18:07:56 2022 +0000

    Roll emscripten from b725fbf23815 to b86798af5d70 (2 revisions)
    
    https://chromium.googlesource.com/external/github.com/emscripten-core/emscripten.git/+log/b725fbf23815..b86798af5d70
    
    2022-06-10 [email protected] Move PROXY_TO_PTHREAD handling into native code. NFC (#17153)
    2022-06-10 [email protected] Add support for noexcept keyword to embind.  (#17140)
    
    If this roll has caused a breakage, revert this CL and stop the roller
    using the controls here:
    https://autoroll.skia.org/r/emscripten-emscripten-releases
    Please CC [email protected] on the revert to ensure that a human
    is aware of the problem.
    
    To report a problem with the AutoRoller itself, please file a bug:
    https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug
    
    Documentation for the AutoRoller is here:
    https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
    
    Tbr: [email protected]
    Change-Id: Ic793eb7c43b147e54addb2e321ffe63abf799dcd
    Reviewed-on: https://chromium-review.googlesource.com/c/emscripten-releases/+/3700611
    Bot-Commit: chromium-autoroll <[email protected]>
    Commit-Queue: chromium-autoroll <[email protected]>

 DEPS | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

So I assume #17153 is the cause. cc @sbc100

Also, surprising we don't have a test for this...

sbc100 added a commit that referenced this issue Jun 29, 2022
This was broken in #17153 but we didn't have any tests.

Fixes: #17338
sbc100 added a commit that referenced this issue Jun 29, 2022
This was broken in #17153 but we didn't have any tests.  The fix is
to export `main` even though we don't technically need to.  We could
add a little complexity to allow metadce to remove it, but I'm not
sure its worth it.

Fixes: #17338
sbc100 added a commit that referenced this issue Jun 29, 2022
This was broken in #17153 but we didn't have any tests.  The fix is
to export `main` even though we don't technically need to.  We could
add a little complexity to allow metadce to remove it, but I'm not
sure its worth it.

Fixes: #17338
sbc100 added a commit that referenced this issue Jun 30, 2022
This was broken in #17153 but we didn't have any tests.  The fix is
to export `main` even though we don't technically need to.  We could
add a little complexity to allow metadce to remove it, but I'm not
sure its worth it.

Fixes: #17338
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 a pull request may close this issue.

2 participants