Skip to content

Commit 8ae41e7

Browse files
committed
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.
1 parent 9d46f34 commit 8ae41e7

File tree

6 files changed

+28
-19
lines changed

6 files changed

+28
-19
lines changed

embuilder.py

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -87,6 +87,7 @@
8787
'libGL-emu',
8888
'libGL-mt',
8989
'libGL-mt-emu',
90+
'libGL-mt-emu-webgl2-ofb',
9091
'libsockets_proxy',
9192
'libsockets-mt',
9293
'crtbegin',

src/jsifier.js

Lines changed: 20 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -230,7 +230,7 @@ function ${name}(${args}) {
230230

231231
const TOP_LEVEL = 'top-level compiled C/C++ code';
232232

233-
function addFromLibrary(item, dependent) {
233+
function addFromLibrary(item, dependent, force = false) {
234234
// dependencies can be JS functions, which we just run
235235
if (typeof item == 'function') {
236236
return item();
@@ -239,11 +239,6 @@ function ${name}(${args}) {
239239
const symbol = item.symbol;
240240
const mangled = item.mangled;
241241

242-
if (symbol in addedLibraryItems) {
243-
return;
244-
}
245-
addedLibraryItems[symbol] = true;
246-
247242
// don't process any special identifiers. These are looked up when
248243
// processing the base name of the identifier.
249244
if (isJsLibraryConfigIdentifier(symbol)) {
@@ -262,10 +257,15 @@ function ${name}(${args}) {
262257

263258
// if the function was implemented in compiled code, there is no need to
264259
// include the js version
265-
if (WASM_EXPORTS.has(symbol)) {
260+
if (WASM_EXPORTS.has(symbol) && !force) {
266261
return;
267262
}
268263

264+
if (symbol in addedLibraryItems) {
265+
return;
266+
}
267+
addedLibraryItems[symbol] = true;
268+
269269
// This gets set to true in the case of dynamic linking for symbols that
270270
// are undefined in the main module. In this case we create a stub that
271271
// will resolve the correct symbol at runtime, or assert if its missing.
@@ -339,15 +339,17 @@ function ${name}(${args}) {
339339
});
340340
let isFunction = false;
341341

342+
const forceDeps = new Set();
342343
if (typeof snippet == 'string') {
343344
if (snippet[0] != '=') {
344-
const target = LibraryManager.library[snippet];
345-
if (target) {
346-
// Redirection for aliases. We include the parent, and at runtime make ourselves equal to it.
347-
// This avoid having duplicate functions with identical content.
345+
if (LibraryManager.library[snippet]) {
346+
// Redirection for aliases. We include the parent, and at runtime
347+
// make ourselves equal to it. This avoid having duplicate
348+
// functions with identical content.
348349
const redirectedTarget = snippet;
349-
deps.push(redirectedTarget);
350350
snippet = mangleCSymbolName(redirectedTarget);
351+
forceDeps.add(redirectedTarget);
352+
deps.push(redirectedTarget);
351353
}
352354
}
353355
} else if (typeof snippet == 'object') {
@@ -381,10 +383,11 @@ function ${name}(${args}) {
381383
const deps_list = deps.join("','");
382384
const identDependents = symbol + `__deps: ['${deps_list}']`;
383385
function addDependency(dep) {
386+
const forceThis = forceDeps.has(dep);
384387
if (typeof dep != 'function') {
385388
dep = {symbol: dep, mangled: mangleCSymbolName(dep)};
386389
}
387-
return addFromLibrary(dep, `${identDependents}, referenced by ${dependent}`);
390+
return addFromLibrary(dep, `${identDependents}, referenced by ${dependent}`, forceThis);
388391
}
389392
let contentText;
390393
if (isFunction) {
@@ -439,8 +442,11 @@ function ${name}(${args}) {
439442
}
440443

441444
let commentText = '';
445+
if (force) {
446+
commentText += '/** @suppress {duplicate } */\n'
447+
}
442448
if (LibraryManager.library[symbol + '__docs']) {
443-
commentText = LibraryManager.library[symbol + '__docs'] + '\n';
449+
commentText += LibraryManager.library[symbol + '__docs'] + '\n';
444450
}
445451

446452
const depsText = (deps ? deps.map(addDependency).filter((x) => x != '').join('\n') + '\n' : '');

src/parseTools.js

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -838,7 +838,7 @@ function hasExportedSymbol(sym) {
838838
// it is a BigInt. Otherwise, we legalize into pairs of i32s.
839839
function defineI64Param(name) {
840840
if (WASM_BIGINT) {
841-
return `/** @type {!BigInt} */ ${name}`;
841+
return name;
842842
}
843843
return `${name}_low, ${name}_high`;
844844
}

system/lib/gl/webgl2.c

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -131,8 +131,6 @@ ASYNC_GL_FUNCTION_5(EM_FUNC_SIG_VIIIII, void, glTexStorage2D, GLenum, GLsizei, G
131131
ASYNC_GL_FUNCTION_6(EM_FUNC_SIG_VIIIIII, void, glTexStorage3D, GLenum, GLsizei, GLenum, GLsizei, GLsizei, GLsizei);
132132
VOID_SYNC_GL_FUNCTION_5(EM_FUNC_SIG_VIIIII, void, glGetInternalformativ, GLenum, GLenum, GLenum, GLsizei, GLint *);
133133

134-
#endif // ~__EMSCRIPTEN_PTHREADS__
135-
136134
// Extensions:
137135
GL_APICALL void GL_APIENTRY glVertexAttribDivisorNV(GLuint index, GLuint divisor) { glVertexAttribDivisor(index, divisor); }
138136
GL_APICALL void GL_APIENTRY glVertexAttribDivisorEXT(GLuint index, GLuint divisor) { glVertexAttribDivisor(index, divisor); }
@@ -153,6 +151,8 @@ GL_APICALL GLboolean GL_APIENTRY glIsVertexArrayOES(GLuint array) { return glIsV
153151
GL_APICALL void GL_APIENTRY glDrawBuffersEXT(GLsizei n, const GLenum *bufs) { glDrawBuffers(n, bufs); }
154152
GL_APICALL void GL_APIENTRY glDrawBuffersWEBGL(GLsizei n, const GLenum *bufs) { glDrawBuffers(n, bufs); }
155153

154+
#endif // ~__EMSCRIPTEN_PTHREADS__) && __EMSCRIPTEN_OFFSCREEN_FRAMEBUFFER__
155+
156156
// Returns a function pointer to the given WebGL 2 extension function, when queried without
157157
// a GL extension suffix such as "EXT", "OES", or "ANGLE". This function is used by
158158
// emscripten_GetProcAddress() to implement legacy GL emulation semantics for portability.

system/lib/gl/webgl_internal.h

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,8 @@ EMSCRIPTEN_RESULT emscripten_webgl_do_commit_frame(void);
77
EM_BOOL emscripten_supports_offscreencanvas(void);
88
void _emscripten_proxied_gl_context_activated_from_main_browser_thread(EMSCRIPTEN_WEBGL_CONTEXT_HANDLE context);
99

10+
#if defined(__EMSCRIPTEN_PTHREADS__) && defined(__EMSCRIPTEN_OFFSCREEN_FRAMEBUFFER__)
11+
1012
#ifdef EMSCRIPTEN_WEBGL_TRACE
1113
#define GL_FUNCTION_TRACE(func) printf(#func "\n")
1214
#else
@@ -52,8 +54,6 @@ void _emscripten_proxied_gl_context_activated_from_main_browser_thread(EMSCRIPTE
5254
#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); }
5355
#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); }
5456

55-
#if defined(__EMSCRIPTEN_PTHREADS__) && defined(__EMSCRIPTEN_OFFSCREEN_FRAMEBUFFER__)
56-
5757
#include <pthread.h>
5858

5959
extern pthread_key_t currentActiveWebGLContext;

test/test_other.py

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8685,6 +8685,7 @@ def test_full_js_library_minimal_runtime(self):
86858685
# binaryen tools get run, which can affect how debug info is kept around
86868686
'bigint': [['-sWASM_BIGINT']],
86878687
'pthread': [['-pthread', '-Wno-experimental']],
8688+
'pthread_offscreen': [['-pthread', '-Wno-experimental', '-sOFFSCREEN_FRAMEBUFFER']],
86888689
})
86898690
def test_closure_full_js_library(self, args):
86908691
# Test for closure errors and warnings in the entire JS library.
@@ -8698,6 +8699,7 @@ def test_closure_full_js_library(self, args):
86988699
'-sFETCH',
86998700
'-sFETCH_SUPPORT_INDEXEDDB',
87008701
'-sLEGACY_GL_EMULATION',
8702+
'-sMAX_WEBGL_VERSION=2',
87018703
] + args)
87028704

87038705
def test_closure_webgpu(self):

0 commit comments

Comments
 (0)