From 9d74f6030c9749f0362e27158a84ddf1982cc98a Mon Sep 17 00:00:00 2001 From: Sam Clegg Date: Wed, 22 Mar 2023 11:58:38 -0700 Subject: [PATCH] jsifier: Make JS libraries aliases work even when native export exists Followup to #19033. Updating the `test_closure_full_js_library` to include webgl2 caused a lot of `JSC_REFERENCE_BEFORE_DECLARE` closure errors. This is because we had JS aliases who's target was defined both in JS and in native code. In this case the jsifier was choosing not to include the JS version, which in the case of the GL symbols not what we want. This is why, prior to #19033, we were duplicating the library entries. This this change JS symbol aliases now force the alias target to be included from the JS library, even in the case that the native code also emits its own version of a symbol. This happens in the case of pthreads + MAIN_MODULE + OFFSCREEN_FRAMEBUFFER. In this mode webgl2.c defines (and also exports) the entire set of GL library symbols. In this case we still want the `emscripten_XX` aliases to point to the original JS versions, no the natively exported version. Sadly it looks like we have at testing gab for that particular combination. --- embuilder.py | 3 +++ src/jsifier.js | 37 +++++++++++++++++++--------------- src/parseTools.js | 2 +- system/lib/gl/webgl2.c | 9 ++++++--- system/lib/gl/webgl_internal.h | 4 ++-- test/test_other.py | 2 ++ 6 files changed, 35 insertions(+), 22 deletions(-) diff --git a/embuilder.py b/embuilder.py index 1fc59af5c17ed..c6b01b677ee42 100755 --- a/embuilder.py +++ b/embuilder.py @@ -85,8 +85,11 @@ 'libc++-mt-noexcept', 'libdlmalloc-mt', 'libGL-emu', + 'libGL-emu-webgl2', 'libGL-mt', 'libGL-mt-emu', + 'libGL-mt-emu-webgl2', + 'libGL-mt-emu-webgl2-ofb', 'libsockets_proxy', 'libsockets-mt', 'crtbegin', diff --git a/src/jsifier.js b/src/jsifier.js index 0357104dc8431..6b88ce7bb53bf 100644 --- a/src/jsifier.js +++ b/src/jsifier.js @@ -228,17 +228,12 @@ function ${name}(${args}) { // what we just added to the library. } - function addFromLibrary(symbol, dependent) { + function addFromLibrary(symbol, dependent, force = false) { // dependencies can be JS functions, which we just run if (typeof symbol == 'function') { return symbol(); } - if (symbol in addedLibraryItems) { - return; - } - addedLibraryItems[symbol] = true; - // don't process any special identifiers. These are looked up when // processing the base name of the identifier. if (isJsLibraryConfigIdentifier(symbol)) { @@ -257,9 +252,14 @@ function ${name}(${args}) { // if the function was implemented in compiled code, there is no need to // include the js version - if (WASM_EXPORTS.has(symbol)) { + if (WASM_EXPORTS.has(symbol) && !force) { + return; + } + + if (symbol in addedLibraryItems) { return; } + addedLibraryItems[symbol] = true; // This gets set to true in the case of dynamic linking for symbols that // are undefined in the main module. In this case we create a stub that @@ -334,17 +334,19 @@ function ${name}(${args}) { warn(`user library symbol '${symbol}' depends on internal symbol '${dep}'`); } }); + let isFunction = false; + let aliasTarget; if (typeof snippet == 'string') { if (snippet[0] != '=') { - const target = LibraryManager.library[snippet]; - if (target) { - // Redirection for aliases. We include the parent, and at runtime make ourselves equal to it. - // This avoid having duplicate functions with identical content. - const redirectedTarget = snippet; - deps.push(redirectedTarget); - snippet = mangleCSymbolName(redirectedTarget); + if (LibraryManager.library[snippet]) { + // Redirection for aliases. We include the parent, and at runtime + // make ourselves equal to it. This avoid having duplicate + // functions with identical content. + aliasTarget = snippet; + snippet = mangleCSymbolName(aliasTarget); + deps.push(aliasTarget); } } } else if (typeof snippet == 'object') { @@ -376,7 +378,7 @@ function ${name}(${args}) { const deps_list = deps.join("','"); const identDependents = symbol + `__deps: ['${deps_list}']`; function addDependency(dep) { - return addFromLibrary(dep, `${identDependents}, referenced by ${dependent}`); + return addFromLibrary(dep, `${identDependents}, referenced by ${dependent}`, dep === aliasTarget); } let contentText; if (isFunction) { @@ -431,8 +433,11 @@ function ${name}(${args}) { } let commentText = ''; + if (force) { + commentText += '/** @suppress {duplicate } */\n' + } if (LibraryManager.library[symbol + '__docs']) { - commentText = LibraryManager.library[symbol + '__docs'] + '\n'; + commentText += LibraryManager.library[symbol + '__docs'] + '\n'; } const depsText = (deps ? deps.map(addDependency).filter((x) => x != '').join('\n') + '\n' : ''); diff --git a/src/parseTools.js b/src/parseTools.js index 9b7432349a292..d97dfdf221434 100644 --- a/src/parseTools.js +++ b/src/parseTools.js @@ -838,7 +838,7 @@ function hasExportedSymbol(sym) { // it is a BigInt. Otherwise, we legalize into pairs of i32s. function defineI64Param(name) { if (WASM_BIGINT) { - return `/** @type {!BigInt} */ ${name}`; + return name; } return `${name}_low, ${name}_high`; } diff --git a/system/lib/gl/webgl2.c b/system/lib/gl/webgl2.c index f4fee6ad59038..ea3d545578474 100644 --- a/system/lib/gl/webgl2.c +++ b/system/lib/gl/webgl2.c @@ -131,9 +131,10 @@ ASYNC_GL_FUNCTION_5(EM_FUNC_SIG_VIIIII, void, glTexStorage2D, GLenum, GLsizei, G ASYNC_GL_FUNCTION_6(EM_FUNC_SIG_VIIIIII, void, glTexStorage3D, GLenum, GLsizei, GLenum, GLsizei, GLsizei, GLsizei); VOID_SYNC_GL_FUNCTION_5(EM_FUNC_SIG_VIIIII, void, glGetInternalformativ, GLenum, GLenum, GLenum, GLsizei, GLint *); -#endif // ~__EMSCRIPTEN_PTHREADS__ - -// Extensions: +// Extensions that are aliases for the proxying functions defined above. +// Normally these aliases get defined in library_webgl.js but when building with +// __EMSCRIPTEN_OFFSCREEN_FRAMEBUFFER__ we want to intercept them in native +// code and redirect them to thier proxying couterparts. GL_APICALL void GL_APIENTRY glVertexAttribDivisorNV(GLuint index, GLuint divisor) { glVertexAttribDivisor(index, divisor); } GL_APICALL void GL_APIENTRY glVertexAttribDivisorEXT(GLuint index, GLuint divisor) { glVertexAttribDivisor(index, divisor); } GL_APICALL void GL_APIENTRY glVertexAttribDivisorARB(GLuint index, GLuint divisor) { glVertexAttribDivisor(index, divisor); } @@ -153,6 +154,8 @@ GL_APICALL GLboolean GL_APIENTRY glIsVertexArrayOES(GLuint array) { return glIsV GL_APICALL void GL_APIENTRY glDrawBuffersEXT(GLsizei n, const GLenum *bufs) { glDrawBuffers(n, bufs); } GL_APICALL void GL_APIENTRY glDrawBuffersWEBGL(GLsizei n, const GLenum *bufs) { glDrawBuffers(n, bufs); } +#endif // ~__EMSCRIPTEN_PTHREADS__) && __EMSCRIPTEN_OFFSCREEN_FRAMEBUFFER__ + // Returns a function pointer to the given WebGL 2 extension function, when queried without // a GL extension suffix such as "EXT", "OES", or "ANGLE". This function is used by // emscripten_GetProcAddress() to implement legacy GL emulation semantics for portability. diff --git a/system/lib/gl/webgl_internal.h b/system/lib/gl/webgl_internal.h index 01b84f2f20cf8..8edf22f18000c 100644 --- a/system/lib/gl/webgl_internal.h +++ b/system/lib/gl/webgl_internal.h @@ -7,6 +7,8 @@ EMSCRIPTEN_RESULT emscripten_webgl_do_commit_frame(void); EM_BOOL emscripten_supports_offscreencanvas(void); void _emscripten_proxied_gl_context_activated_from_main_browser_thread(EMSCRIPTEN_WEBGL_CONTEXT_HANDLE context); +#if defined(__EMSCRIPTEN_PTHREADS__) && defined(__EMSCRIPTEN_OFFSCREEN_FRAMEBUFFER__) + #ifdef EMSCRIPTEN_WEBGL_TRACE #define GL_FUNCTION_TRACE(func) printf(#func "\n") #else @@ -52,8 +54,6 @@ void _emscripten_proxied_gl_context_activated_from_main_browser_thread(EMSCRIPTE #define VOID_SYNC_GL_FUNCTION_10(sig, ret, functionName, t0, t1, t2, t3, t4, t5, t6, t7, t8, t9) ret functionName(t0 p0, t1 p1, t2 p2, t3 p3, t4 p4, t5 p5, t6 p6, t7 p7, t8 p8, t9 p9) { GL_FUNCTION_TRACE(functionName); if (pthread_getspecific(currentThreadOwnsItsWebGLContext)) emscripten_##functionName(p0, p1, p2, p3, p4, p5, p6, p7, p8, p9); else emscripten_sync_run_in_main_runtime_thread(sig, &emscripten_##functionName, p0, p1, p2, p3, p4, p5, p6, p7, p8, p9); } #define VOID_SYNC_GL_FUNCTION_11(sig, ret, functionName, t0, t1, t2, t3, t4, t5, t6, t7, t8, t9, t10) ret functionName(t0 p0, t1 p1, t2 p2, t3 p3, t4 p4, t5 p5, t6 p6, t7 p7, t8 p8, t9 p9, t10 p10) { GL_FUNCTION_TRACE(functionName); if (pthread_getspecific(currentThreadOwnsItsWebGLContext)) emscripten_##functionName(p0, p1, p2, p3, p4, p5, p6, p7, p8, p9, p10); else emscripten_sync_run_in_main_runtime_thread(sig, &emscripten_##functionName, p0, p1, p2, p3, p4, p5, p6, p7, p8, p9, p10); } -#if defined(__EMSCRIPTEN_PTHREADS__) && defined(__EMSCRIPTEN_OFFSCREEN_FRAMEBUFFER__) - #include extern pthread_key_t currentActiveWebGLContext; diff --git a/test/test_other.py b/test/test_other.py index 429e344794a11..40c2a907409eb 100644 --- a/test/test_other.py +++ b/test/test_other.py @@ -8685,6 +8685,7 @@ def test_full_js_library_minimal_runtime(self): # binaryen tools get run, which can affect how debug info is kept around 'bigint': [['-sWASM_BIGINT']], 'pthread': [['-pthread', '-Wno-experimental']], + 'pthread_offscreen': [['-pthread', '-Wno-experimental', '-sOFFSCREEN_FRAMEBUFFER']], }) def test_closure_full_js_library(self, args): # Test for closure errors and warnings in the entire JS library. @@ -8698,6 +8699,7 @@ def test_closure_full_js_library(self, args): '-sFETCH', '-sFETCH_SUPPORT_INDEXEDDB', '-sLEGACY_GL_EMULATION', + '-sMAX_WEBGL_VERSION=2', ] + args) def test_closure_webgpu(self):