Fix function dependecy handling in library_gl.js

[mosra]

I tried to use the glDrawRangeElements() function that's available on WebGL 2 but the generated code looks like this:

function _glDrawElements(mode, count, type, indices) {
    GLctx.drawElements(mode, count, type, indices)
}
function _glDrawElementsInstanced(mode, count, type, indices, primcount) {
    GLctx["drawElementsInstanced"](mode, count, type, indices, primcount)
}
function _emscripten_glDrawElements(mode, count, type, indices) {
    GLctx.drawElements(mode, count, type, indices)
}
function _glDrawRangeElements(_emscripten_glDrawRangeElementsmode, start, end, count, type, indices) {
    _emscripten_glDrawElements(mode, count, type, indices)
}

Note the _emscripten_glDrawRangeElements prepended before mode. That obviously fails at runtime with

ReferenceError: mode is not defined

This seems to be because glDrawRangeElements() is one of the very few functions that depend on other GL functions and the code that handles the dependencies is broken. Since glDrawRangeElements() is a WebGL2-only function that caused a crash on Firefox until relatively recently and all the other functions with dependencies are either emulating legacy GL or non-standard and (so I assume) very seldom used, I think the dependency handling code never really worked to begin with. The code originates in 72c0b9e back from February 2014 and was not changed since. To me at least, the line I removed doesn't seem to serve any purpose other than breaking the code.

Note: I don't have any means to test the legacy GL emulation, so I can't verify it really didn't break anything there. Nevertheless, I can't find any purpose that the removed line could have in the legacy emulation code either.

[kripken]

Very strange. So what that code is doing is this:

function (id) {
==>
function _emscripten_glDeleteObject(id) {

(edit: oops, I had that reversed)

That is, it adds a function name instead of leaving it unnamed (the name helps in stack traces). Not sure how that code could break things for you, though - we do have a test with it, ./tests/runner.py browser.test_cubegeom_normal_dap_far_range (which runs tests/cubegeom_normal_dap_far_range.c) , does it not pass for you?

If it fails, maybe add logging like this:

index 5a8b163..d2ce83a 100644
--- a/src/library_gl.js
+++ b/src/library_gl.js
@@ -7971,7 +7971,9 @@ keys(LibraryGL).forEach(function(x) {
       var orig = dep;
       dep = 'emscripten_' + dep;
       var fixed = LibraryGL[x].toString().replace(new RegExp('_' + orig + '\\(', 'g'), '_' + dep + '(');
+      console.log('before ' + fixed);
       fixed = fixed.substr(0, 9) + '_' + y + fixed.substr(9);
+      console.log('after ' + fixed);
       LibraryGL[x] = eval('(function() { return ' + fixed + ' })()');
     }
     return dep;

I'm curious to see how it's different on your machine. I'm guessing it's the JS engine which somehow renders functions to text differently - are you not using node.js as the compiler JS engine? Maybe we need to be more careful about the parsing there.

[mosra]

Thanks for the explanation!

Very strange. So what that code is doing is this:

function (id) {
==>
function _emscripten_glDeleteObject(id) {

Yes, but there is no space in front of the parenthesis anywhere:

  glDeleteObject: function(id) {

... so you get what I'm seeing here:

function(id) {
==>
function(_emscripten_glDeleteObjectid) {

The test fails for me without the fix and passes with the fix, the before/after results are consistent with the above. I hope I can assume toString() just takes whatever is in the source, including comments, and returns it verbatim without adding/removing whitespace, right?

I'm using the ArchLinux emscripten package and it seems to be just using the default node.js. Though I didn't update my system in a while and I'm not on the latest and greatest version of it (v10.4.1, latest in the repos is 10.10.1, but it shouldn't have such breaking changes, right?). I can try again after fully updating my system, but so far what I'm seeing is consistent with what the code looks like it should be doing.

For yout the test does pass?

Is there some lint / code style pass run over the library_gl.js file before it gets processed, causing function( to become function (? That seems weird to me.

So, based on the above, I think the following would be a proper fix instead, working independently on whether the space is there or not ... does it look okay?

diff --git a/src/library_gl.js b/src/library_gl.js
index 5a8b1634e..93eb8d9be 100644
--- a/src/library_gl.js
+++ b/src/library_gl.js
@@ -7971,7 +7971,7 @@ keys(LibraryGL).forEach(function(x) {
       var orig = dep;
       dep = 'emscripten_' + dep;
       var fixed = LibraryGL[x].toString().replace(new RegExp('_' + orig + '\\(', 'g'), '_' + dep + '(');
-      fixed = fixed.substr(0, 9) + '_' + y + fixed.substr(9);
+      // `function` is 8 characters, add an explicit name after
+      fixed = fixed.substr(0, 8) + ' _' + y + fixed.substr(8);
       LibraryGL[x] = eval('(function() { return ' + fixed + ' })()');
     }
     return dep;

[kripken]

Yeah, the test passes for me (also passes on CI here). I suspect your version of node.js is either very old or very new, and the toString() of functions has changed..?

In any case, yeah, changing the 9 to 8 (and adding a space before _ as you did) seems correct, as it would work whether or not there's a space there.

[mosra]

Ah! After a while of digging, I managed to find out that the toString() behavior changed in Node.js 10.0 (and apparently the changelog doesn't mention any of that): nodejs/node#20355

That in turn is a change in V8 6.6, which is mentioned here: https://v8project.blogspot.com/2018/03/v8-release-66.html

I tried running (function() {}).toString() in consoles on various browsers and I got a space before () only on an older Chrome v65 (V8 6.6 was released with Chrome 66). Newer Chrome and Firefox have no space before (). I'm not sure how much Emscripten code depends on this particular behavior, maybe it could be a good thing to check (or update the CIs with a newer Node.js).

I updated the PR with the above patch to make it ready for a merge.

[kripken]

Interesting, thanks for investigating.

I see the CI failures locally too. It seems like sometimes there is already a name there. It works for me with this:

-      fixed = fixed.substr(0, 9) + '_' + y + fixed.substr(9);
+      // `function` is 8 characters, add space and an explicit name after if there isn't one already
+      if (fixed.startsWith('function(') || fixed.startsWith('function (')) {
+        fixed = fixed.substr(0, 8) + ' _' + y + fixed.substr(8);
+      }

Hopefully that fixes it for you too?

[mosra]

Yes, this works too 👍

[kripken]

Great, please push that to this PR then and we should be green and can merge.

[mosra]

Pushed.

[kripken]

Thanks!

[mosra]

Thank you for the merge! 👍