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

Allow SHARED_MEMORY flag to be used independently. #22683

Merged
merged 14 commits into from
Oct 16, 2024
Merged
Show file tree
Hide file tree
Changes from 10 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
54 changes: 3 additions & 51 deletions system/lib/compiler-rt/stack_limits.S
Original file line number Diff line number Diff line change
Expand Up @@ -15,14 +15,17 @@
#endif

.globaltype __stack_pointer, PTR
.globl __stack_pointer

.section .globals,"",@

# TODO(sbc): It would be nice if these we initialized directly
# using PTR.const rather than using the `emscripten_stack_init`
.globaltype __stack_end, PTR
__stack_end:
.globl __stack_end
.globaltype __stack_base, PTR
.globl __stack_base
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't love that these are now globals symbols...I wish we could find a way to avoid the need for divergence here between wasm workers and pthreads, but for now this seems reasonable.

__stack_base:

.section .text,"",@
Expand Down Expand Up @@ -82,57 +85,6 @@ emscripten_stack_get_free:
PTR.sub
end_function

#ifdef __EMSCRIPTEN_WASM_WORKERS__
# TODO: Relocate the following to its own file wasm_worker.S, but need to figure out how to reference
# __stack_base and __stack_end globals from a separate file as externs in order for that to work.
.globl _emscripten_wasm_worker_initialize
_emscripten_wasm_worker_initialize:
.functype _emscripten_wasm_worker_initialize (PTR /*stackLowestAddress*/, i32 /*stackSize*/) -> ()

// __stack_end = stackLowestAddress + (__builtin_wasm_tls_size() + 15) & -16;
local.get 0
.globaltype __tls_size, PTR, immutable
global.get __tls_size
PTR.add
PTR.const 0xf
PTR.add
PTR.const -0x10
PTR.and
global.set __stack_end

// __stack_base = stackLowestAddress + stackSize;
local.get 0
local.get 1
#ifdef __wasm64__
i64.extend_i32_u
#endif
PTR.add
global.set __stack_base

// TODO: We'd like to do this here to avoid JS side calls to __set_stack_limits.
// (or even better, we'd like to avoid duplicate versions of the stack variables)
// See https://github.com/emscripten-core/emscripten/issues/16496
// global.get __stack_base
// global.get __stack_end
// .functype __set_stack_limits (PTR, PTR) -> ()
// call __set_stack_limits

// __wasm_init_tls(stackLowestAddress);
local.get 0
.functype __wasm_init_tls (PTR) -> ()
call __wasm_init_tls

// N.b. The function __wasm_init_tls above does not need
// __stack_pointer initialized, and it destroys the value it was set to.
// So we must initialize __stack_pointer only *after* completing __wasm_init_tls:

// __stack_pointer = __stack_base;
global.get __stack_base
global.set __stack_pointer

end_function
#endif

# Add emscripten_stack_init to static ctors
.section .init_array.1,"",@
.p2align ALIGN
Expand Down
3 changes: 1 addition & 2 deletions system/lib/dlmalloc.c
Original file line number Diff line number Diff line change
Expand Up @@ -21,13 +21,12 @@
/* XXX Emscripten Tracing API. This defines away the code if tracing is disabled. */
#include <emscripten/trace.h>

#ifdef __EMSCRIPTEN_WASM_WORKERS__
#ifdef __EMSCRIPTEN_SHARED_MEMORY__
#define USE_LOCKS 1
#endif

/* Make malloc() and free() threadsafe by securing the memory allocations with pthread mutexes. */
#if __EMSCRIPTEN_PTHREADS__
#define USE_LOCKS 1
#define USE_SPIN_LOCKS 0 // Ensure we use pthread_mutex_t.
#endif

Expand Down
64 changes: 64 additions & 0 deletions system/lib/wasm_worker/wasm_worker.S
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
.extern __stack_pointer
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, one last change. Can we call this file emscripten_wasm_worker_initialize.S or wasm_worker_initialize.S

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure!

.extern __stack_base
.extern __stack_end

#ifdef __wasm64__
#define PTR i64
#define ALIGN 3
#define PTRSTORE .int64
#else
#define PTR i32
#define ALIGN 2
#define PTRSTORE .int32
#endif

.globaltype __stack_pointer, PTR
.globaltype __stack_end, PTR
.globaltype __stack_base, PTR

.globl _emscripten_wasm_worker_initialize
_emscripten_wasm_worker_initialize:
.functype _emscripten_wasm_worker_initialize (PTR /*stackLowestAddress*/, i32 /*stackSize*/) -> ()

// __stack_end = stackLowestAddress + (__builtin_wasm_tls_size() + 15) & -16;
local.get 0
.globaltype __tls_size, PTR, immutable
global.get __tls_size
PTR.add
PTR.const 0xf
PTR.add
PTR.const -0x10
PTR.and
global.set __stack_end

// __stack_base = stackLowestAddress + stackSize;
local.get 0
local.get 1
#ifdef __wasm64__
i64.extend_i32_u
#endif
PTR.add
global.set __stack_base

// TODO: We'd like to do this here to avoid JS side calls to __set_stack_limits.
// (or even better, we'd like to avoid duplicate versions of the stack variables)
// See https://github.com/emscripten-core/emscripten/issues/16496
// global.get __stack_base
// global.get __stack_end
// .functype __set_stack_limits (PTR, PTR) -> ()
// call __set_stack_limits

// __wasm_init_tls(stackLowestAddress);
local.get 0
.functype __wasm_init_tls (PTR) -> ()
call __wasm_init_tls

// N.b. The function __wasm_init_tls above does not need
// __stack_pointer initialized, and it destroys the value it was set to.
// So we must initialize __stack_pointer only *after* completing __wasm_init_tls:

// __stack_pointer = __stack_base;
global.get __stack_base
global.set __stack_pointer

end_function
2 changes: 2 additions & 0 deletions test/core/test_dlfcn_self.exports
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,9 @@ __progname_full
__sig_actions
__sig_pending
__signgam
__stack_base
__stack_chk_guard
__stack_end
__threwValue
__timezone
__tzname
Expand Down
2 changes: 1 addition & 1 deletion test/core/test_em_asm_unicode.out
Original file line number Diff line number Diff line change
@@ -1 +1 @@
hello world…
hello world…
2 changes: 1 addition & 1 deletion test/core/test_longjmp.out
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
first
result: 1 1
second
result: 2 -1
result: 2 -1
2 changes: 1 addition & 1 deletion tools/link.py
Original file line number Diff line number Diff line change
Expand Up @@ -1413,7 +1413,7 @@ def phase_linker_setup(options, state, newargs):
'removeRunDependency',
]

if settings.SHARED_MEMORY or settings.RELOCATABLE or settings.ASYNCIFY_LAZY_LOAD_CODE:
if settings.PTHREADS or settings.WASM_WORKERS or settings.RELOCATABLE or settings.ASYNCIFY_LAZY_LOAD_CODE:
settings.IMPORTED_MEMORY = 1

set_initial_memory()
Expand Down
44 changes: 31 additions & 13 deletions tools/system_libs.py
Original file line number Diff line number Diff line change
Expand Up @@ -717,38 +717,43 @@ def get_usable_variations(cls):
class MTLibrary(Library):
def __init__(self, **kwargs):
self.is_mt = kwargs.pop('is_mt')
self.is_ww = kwargs.pop('is_ww') and not self.is_mt
self.is_sm = kwargs.pop('is_sm') and not self.is_mt
super().__init__(**kwargs)

def get_cflags(self):
cflags = super().get_cflags()
if self.is_mt:
cflags += ['-pthread', '-sWASM_WORKERS']
if self.is_ww:
cflags += ['-sWASM_WORKERS']
if self.is_sm:
cflags += ['-sSHARED_MEMORY=1']
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is now the only remaining controversial change. This does mean that no -ww libraries can ever use __EMSCRIPTEN_WASM_WORKERS__ which seems odd. So perhaps revert this line too.

The net result of this PR would then me "SHARED_MEMORY uses WASM_WORKERS libraries for now, but most them will work just fine even without the wasm workers library itself".

I think we could land that and then iterate from there if it doesn't work in all cases.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the long run I agree we want to almost all libraries to be built only with SHARED_MEMORY and then only libwasm_workers and libpthreads (and perhaps one or two more) to be build with specificly wasm workers or pthreads.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I see. I'll look into changing those

return cflags

def get_base_name(self):
name = super().get_base_name()
if self.is_mt:
name += '-mt'
if self.is_ww:
if self.is_sm:
name += '-ww'
return name

@classmethod
def vary_on(cls):
return super().vary_on() + ['is_mt', 'is_ww']
return super().vary_on() + ['is_mt', 'is_sm']

@classmethod
def get_default_variation(cls, **kwargs):
return super().get_default_variation(is_mt=settings.PTHREADS, is_ww=settings.WASM_WORKERS and not settings.PTHREADS, **kwargs)
return super().get_default_variation(
is_mt=settings.PTHREADS,
is_sm=settings.SHARED_MEMORY and not settings.PTHREADS,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you can revert all changes to this class except this one line?

**kwargs
)

@classmethod
def variations(cls):
combos = super(MTLibrary, cls).variations()
# To save on # of variations, pthreads and Wasm workers when used together, just use pthreads variation.
return [combo for combo in combos if not combo['is_mt'] or not combo['is_ww']]

# These are mutually exclusive, only one flag will be set at any give time.
return [combo for combo in combos if not combo['is_mt'] or not combo['is_sm']]


class DebugLibrary(Library):
Expand Down Expand Up @@ -1416,10 +1421,13 @@ class libwasm_workers(MTLibrary):

def __init__(self, **kwargs):
self.debug = kwargs.pop('debug')
self.is_ww = kwargs.pop('is_ww')
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this class no longer inherit from MTLibrary?

super().__init__(**kwargs)

def get_cflags(self):
cflags = super().get_cflags()
if self.is_ww:
cflags += ['-sWASM_WORKERS']
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you make the above change you can also revert this change.

if self.debug:
cflags += ['-D_DEBUG']
# library_wasm_worker.c contains an assert that a nonnull parameter
Expand All @@ -1445,16 +1453,26 @@ def get_base_name(self):

@classmethod
def vary_on(cls):
return super().vary_on() + ['debug']
return super().vary_on() + ['debug', 'is_ww']

@classmethod
def get_default_variation(cls, **kwargs):
return super().get_default_variation(debug=settings.ASSERTIONS >= 1, **kwargs)
return super().get_default_variation(is_ww=settings.WASM_WORKERS, debug=settings.ASSERTIONS >= 1, **kwargs)

def get_files(self):
files = []
if self.is_ww:
files = [
'library_wasm_worker.c',
'wasm_worker.S',
]
else:
files = [
'library_wasm_worker_stub.c'
]
return files_in_path(
path='system/lib/wasm_worker',
filenames=['library_wasm_worker.c' if self.is_ww or self.is_mt else 'library_wasm_worker_stub.c'])
filenames=files)
sbc100 marked this conversation as resolved.
Show resolved Hide resolved

def can_use(self):
# see src/library_wasm_worker.js
Expand Down Expand Up @@ -1568,7 +1586,7 @@ def __init__(self, **kwargs):

def get_cflags(self):
cflags = super().get_cflags()
if not self.is_mt and not self.is_ww:
if not self.is_mt and not self.is_sm:
cflags.append('-D_LIBCXXABI_HAS_NO_THREADS')
if self.eh_mode == Exceptions.NONE:
cflags.append('-D_LIBCXXABI_NO_EXCEPTIONS')
Expand Down Expand Up @@ -1682,7 +1700,7 @@ def can_use(self):
def get_cflags(self):
cflags = super().get_cflags()
cflags.append('-DNDEBUG')
if not self.is_mt and not self.is_ww:
if not self.is_mt and not self.is_sm:
cflags.append('-D_LIBUNWIND_HAS_NO_THREADS')
if self.eh_mode == Exceptions.NONE:
cflags.append('-D_LIBUNWIND_HAS_NO_EXCEPTIONS')
Expand Down