Skip to content

Commit 76fe398

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 07af96a commit 76fe398

File tree

6 files changed

+34
-22
lines changed

6 files changed

+34
-22
lines changed

embuilder.py

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -85,8 +85,10 @@
8585
'libc++-mt-noexcept',
8686
'libdlmalloc-mt',
8787
'libGL-emu',
88+
'libGL-emu-webgl2',
8889
'libGL-mt',
8990
'libGL-mt-emu',
91+
'libGL-mt-emu-webgl2-ofb',
9092
'libsockets_proxy',
9193
'libsockets-mt',
9294
'crtbegin',

src/jsifier.js

Lines changed: 21 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -228,17 +228,12 @@ function ${name}(${args}) {
228228
// what we just added to the library.
229229
}
230230

231-
function addFromLibrary(symbol, dependent) {
231+
function addFromLibrary(symbol, dependent, force = false) {
232232
// dependencies can be JS functions, which we just run
233233
if (typeof symbol == 'function') {
234234
return symbol();
235235
}
236236

237-
if (symbol in addedLibraryItems) {
238-
return;
239-
}
240-
addedLibraryItems[symbol] = true;
241-
242237
// don't process any special identifiers. These are looked up when
243238
// processing the base name of the identifier.
244239
if (isJsLibraryConfigIdentifier(symbol)) {
@@ -257,9 +252,14 @@ function ${name}(${args}) {
257252

258253
// if the function was implemented in compiled code, there is no need to
259254
// include the js version
260-
if (WASM_EXPORTS.has(symbol)) {
255+
if (WASM_EXPORTS.has(symbol) && !force) {
256+
return;
257+
}
258+
259+
if (symbol in addedLibraryItems) {
261260
return;
262261
}
262+
addedLibraryItems[symbol] = true;
263263

264264
// This gets set to true in the case of dynamic linking for symbols that
265265
// are undefined in the main module. In this case we create a stub that
@@ -334,17 +334,19 @@ function ${name}(${args}) {
334334
warn(`user library symbol '${symbol}' depends on internal symbol '${dep}'`);
335335
}
336336
});
337+
337338
let isFunction = false;
339+
let aliasTarget;
338340

339341
if (typeof snippet == 'string') {
340342
if (snippet[0] != '=') {
341-
const target = LibraryManager.library[snippet];
342-
if (target) {
343-
// Redirection for aliases. We include the parent, and at runtime make ourselves equal to it.
344-
// This avoid having duplicate functions with identical content.
345-
const redirectedTarget = snippet;
346-
deps.push(redirectedTarget);
347-
snippet = mangleCSymbolName(redirectedTarget);
343+
if (LibraryManager.library[snippet]) {
344+
// Redirection for aliases. We include the parent, and at runtime
345+
// make ourselves equal to it. This avoid having duplicate
346+
// functions with identical content.
347+
aliasTarget = snippet;
348+
snippet = mangleCSymbolName(aliasTarget);
349+
deps.push(aliasTarget);
348350
}
349351
}
350352
} else if (typeof snippet == 'object') {
@@ -376,7 +378,7 @@ function ${name}(${args}) {
376378
const deps_list = deps.join("','");
377379
const identDependents = symbol + `__deps: ['${deps_list}']`;
378380
function addDependency(dep) {
379-
return addFromLibrary(dep, `${identDependents}, referenced by ${dependent}`);
381+
return addFromLibrary(dep, `${identDependents}, referenced by ${dependent}`, dep === aliasTarget);
380382
}
381383
let contentText;
382384
if (isFunction) {
@@ -431,8 +433,11 @@ function ${name}(${args}) {
431433
}
432434

433435
let commentText = '';
436+
if (force) {
437+
commentText += '/** @suppress {duplicate } */\n'
438+
}
434439
if (LibraryManager.library[symbol + '__docs']) {
435-
commentText = LibraryManager.library[symbol + '__docs'] + '\n';
440+
commentText += LibraryManager.library[symbol + '__docs'] + '\n';
436441
}
437442

438443
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: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -131,9 +131,10 @@ 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-
136-
// Extensions:
134+
// Extensions that are aliases for the proxying functions defined above.
135+
// Normally these aliases get defined in library_webgl.js but when building with
136+
// __EMSCRIPTEN_OFFSCREEN_FRAMEBUFFER__ we want to intercept them in native
137+
// code and redirect them to thier proxying couterparts.
137138
GL_APICALL void GL_APIENTRY glVertexAttribDivisorNV(GLuint index, GLuint divisor) { glVertexAttribDivisor(index, divisor); }
138139
GL_APICALL void GL_APIENTRY glVertexAttribDivisorEXT(GLuint index, GLuint divisor) { glVertexAttribDivisor(index, divisor); }
139140
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
153154
GL_APICALL void GL_APIENTRY glDrawBuffersEXT(GLsizei n, const GLenum *bufs) { glDrawBuffers(n, bufs); }
154155
GL_APICALL void GL_APIENTRY glDrawBuffersWEBGL(GLsizei n, const GLenum *bufs) { glDrawBuffers(n, bufs); }
155156

157+
#endif // ~__EMSCRIPTEN_PTHREADS__) && __EMSCRIPTEN_OFFSCREEN_FRAMEBUFFER__
158+
156159
// Returns a function pointer to the given WebGL 2 extension function, when queried without
157160
// a GL extension suffix such as "EXT", "OES", or "ANGLE". This function is used by
158161
// 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)