Compile libpng with shared memory when using pthreads

[connorjclark]

Fixes #16681

Bisect here

This patch always builds libpng with -pthread. The alternative is to build a -mt variant, but the last time this issue happened @sbc100 suggested this as the better approach.

This patch builds a -mt variant with shared memory iff USE_PTHREAD is defined.

[sbc100]

@tlively @kripken .. can/should we rely on the use of atomics with non-shared memories? (i.e. do all the browsers support this these days?) I fear I may have spoken too soon when I suggested this approach. Perhaps binaryen can/should lower all the atomics away instead?

[tlively]

I don't know for sure about browser support, but it's definitely a bug if they don't support this. The only pitfall would be if this library uses any TLS or atomics, in which case building it with pthread support would make it unusable with non-pthreads builds. Do we have a test that checks that that works?

[sbc100]

The only pitfall would be if this library uses any TLS or atomics, in which case building it with pthread support would make it unusable with non-pthreads builds.

Is there a typo in this sentence? I though that whole point was to build things with pthread support and they would be usable in all builds (both pthread and non-pthread builds).

[sbc100]

I'm sorry to go back on my previous suggestions, but I think making a -mt build is safer for now. While the "use -pthread objects in single threading binaries" approach may work it is something that we have not yet done so has some risk, and in particular might not be compatible with older browsers that have no atomics support at all.

[connorjclark]

RE: browser support

caniuse states that atomics is available in the latest major browsers, except that in Safari it is gated behind COOP/COEP response headers.

The site states that Android chrome/ff has it disabled... but this bug suggests otherwise. I reported this as a bug in the browser compat data.

Anecdotally I've definitely been able to use emscripten applications built with pthreads on my android Chrome and FF browsers.

I'm sorry to go back on my previous suggestions

No worries! I'll make the requested changes.

[tlively]

FWIW, that caniuse page is about the Atomics built-in in JS, which is similar to WebAssembly atomics but not the same. In particular I would not expect JS Atomics to predate WebAssembly atomics on most browsers.

[connorjclark]

not sure why CI is failing, this test is passing for me locally (on mac):

python3 tests/runner.py other.test_libpng_with_pthreads

could someone help me diagnose? CI just says subprocess exited with non-zero return code(1), no logs.

[kleisauke]

It looks like it's failing with:

Traceback (most recent call last):
  File "/root/project/emcc.py", line 4010, in <module>
    sys.exit(main(sys.argv))
  File "/usr/lib/python3.6/contextlib.py", line 52, in inner
    return func(*args, **kwds)
  File "/root/project/emcc.py", line 4003, in main
    ret = run(args)
  File "/root/project/emcc.py", line 1136, in run
    linker_inputs = phase_compile_inputs(options, state, newargs, input_files)
  File "/usr/lib/python3.6/contextlib.py", line 52, in inner
    return func(*args, **kwds)
  File "/root/project/emcc.py", line 2697, in phase_compile_inputs
    compile_source_file(i, input_file)
  File "/root/project/emcc.py", line 2677, in compile_source_file
    cmd = get_clang_command(input_file)
  File "/root/project/emcc.py", line 2618, in get_clang_command
    return get_compiler(src_file) + get_cflags(state.orig_args) + compile_args + [src_file]
  File "/root/project/emcc.py", line 888, in get_cflags
    ports.add_cflags(cflags, settings)
  File "/root/project/tools/ports/__init__.py", line 364, in add_cflags
    port.get(Ports, settings, shared)
  File "/root/project/tools/ports/libpng.py", line 47, in get
    return [shared.Cache.get_lib(get_lib_name(settings), create, what='port')]
  File "/root/project/tools/cache.py", line 139, in get_lib
    return self.get(name, *args, **kwargs)
  File "/root/project/tools/cache.py", line 154, in get
    raise Exception(f'FROZEN_CACHE is set, but cache file is missing: "{shortname}" (in cache root path "{self.dirname}")')
Exception: FROZEN_CACHE is set, but cache file is missing: "sysroot/lib/wasm32-emscripten/libpng-mt.a" (in cache root path "/root/cache")

Adding this entry:

    'libpng-mt': ('libpng', {'USE_PTHREADS': 1}),

to the dictionary here:

emscripten/embuilder.py

Line 64 in 7e9a238

PORT_VARIANTS = {

should fix that.

[sbc100]

Why not USE_PTHREADS like the other ports do?

[connorjclark]

I was following the suggestion of @kleisauke

#16681 (comment)

libpng doesn't use pthreads, it just needs to be built with shared memory.

[sbc100]

Sure, but I think that is probably true for most of the ports that have -mt builds. I don't know of any of them that actually start threads.. they just use atomics and pthread locks to make themselves thread safe.

I don't feel strongly but I think it would be better to be consistent across ports on this.

[sbc100]

Looks like the main thing is to add to PORT_VARIANTS so that this change can land.

[connorjclark]

test_fetch_sync_xhr ff failures seems flaky, but let me know if there's anything else for me to change.