Regal port update

[gabrielcuvillier]

Updated Regal port

1. Updated to latest port version. This brings:
   => support for -pthread compilation (REGAL_NO_TLS and REGAL_THREAD_LOCKING depends on -pthread option)
   => A couple of bugfixes in Quads support (GL_QUAD_STRIP bugfix), glsl generation (missing gl_ModelViewMatrixInverseTranspose, enable of GL_OES_standard_derivatives), and compilation size optimization (some unused stuff have been disabled)
   => Internalized some compilation define logic (no more need to define DREGAL_NO_PNG, DREGAL_GLSL_OPTIMIZER, etc.)

• O3 compilation is enabled by default. Regal have to be fast.

• Like with the SDL2 port, allow to customize compilation flags depending on provided compilation options/settings + specific library targets with dedicated suffixes:
  => add the -pthread flag if needed, along with '-mt' suffix
  => add the -fno-exceptions flag if needed, along with '-noexcept' suffix
  => add the -flto flag if needed. No suffix there, as the lib will already have a .bc extension instead of .a

@kripken
NB: I am not sure about how to handle the "clear" method. As we don't have the settings parameter, I can't decide which lib target to clear (-mt ? -noexcept ? etc.). I noticed there is a similar issue with libSDL2, which always clear the unsufixed version by default

[gabrielcuvillier]

.. huh, not sure what to do about the error. It seems the test look for "libregal.bc", but it should look for "libregal-noexcept.bc"

[kripken]

Good point about clear - it should probably take settings as a param too. However, you can do shared.Settings.X to get settings, so maybe both don't need it... we should pick one way and fix them all to do that, I guess.

[kripken]

The error means that embuilder.py needs to be updated, I believe, as it has the build target names hardcoded in there.

[gabrielcuvillier]

Ok, I'll take a look at embuilder.py + will use the shared.settings workaround for the clear method

[sbc100]

Nice!

Can you update the tite of the PR (what is Gab?) and finalize the commit message and this looks almost ready to land.

[gabrielcuvillier]

Okay, I just need to check the tests on my side. The fail looks suspicious and related to the PR. Maybe I did something wrong.

[gabrielcuvillier]

Hmm, after creating some new tests that targets Regal+pthreads, it look like I stumbled on an unrelated bug on Chrome, that I am able to reproduce on my system

The error message on Chrome is (I can reproduce it on my system):

Blob constructor present but fails: TypeError: Failed to construct 'Blob': The provided ArrayBufferView value must not be shared.; falling back to blob builder"

This happens during the file preload / imagePlugin handlers:

    Uncaught (in promise) TypeError: Browser.BlobBuilder is not a constructor
    at Object.imagePlugin_handle [as handle] (VM15 test.js:1596)
    at VM15 test.js:4359
    at Array.forEach (<anonymous>)
    at processData (VM15 test.js:4356)
    at Object.createPreloadedFile [as FS_createPreloadedFile] (VM15 test.js:4374)
    at DataRequest.finish (VM15 test.js:112)
    at DataRequest.onload (VM15 test.js:108)
    at processPackageData (VM15 test.js:138)
    at runWithFS (VM15 test.js:148)
    at callRuntimeCallbacks (VM15 test.js:1057)

After some look in the code, I stumbled onto this in library.js:

try {
        new Blob();
        Browser.hasBlobConstructor = true;
      } catch(e) {
        Browser.hasBlobConstructor = false;
        console.log("warning: no blob constructor, cannot create blobs with mimetypes");
      }
Browser.BlobBuilder = typeof MozBlobBuilder != "undefined" ? MozBlobBuilder : (typeof WebKitBlobBuilder != "undefined" ? WebKitBlobBuilder : (!Browser.hasBlobConstructor ? console.log("warning: no BlobBuilder") : null));

The last 'null' looks suspicious, that would mean that if

• We are neither on Firefox nor Webkit
• but it is possible to do 'new Blob()' (Browser.hasBlobConstructor = true)
  => there is no BlobBuilder available!
  Unless it is really the intent, as if we are there, it is because of the very first error (Failure to create a Blob due to using a shared buffer view).

I am confused :)

[kripken]

I think the idea in that code is: if we have the Blob constructor, we are ok (and we look for it first, in the code in library_browser.js. If we don't have it, then we need the BlobBuilder, so we look for that. If we don't have that, then we assign null to BlobBuilder, but we also issue a warning if we lack the Blob constructor, as that means we don't have either, and are in bad shape...

[kripken]

IOW that null is just "don't show the warning", but we assign null or undefined either way.

[gabrielcuvillier]

Ok, then I think I may have found a bug with image preloading code + PTHREADS. I'll create a separate issue for this with a more isolated scenario (without regal stuff), and will update the new regal tests I added to not use image preloading

[gabrielcuvillier]

Ok, I have opened #9992 for the preloaded image issue, and changed the regal-mt tests to not use image preloading. So far, everything looks good: firefox and chrome tests pass.

However, there is new test fails that look like to be unrelated to this PR: test-upstream-wasmXXX. This is failing on things related to freetype apprently, which I don't use at all in the tests. Or maybe I missed something ?

[sbc100]

I think you might just need to merge/rebase. I think the fix for the failure you seeing landed in #9957.

[kripken]

Is the exceptions variant necessary - does regal use exceptions, or build differently with them?

Otherwise lgtm, aside from the merge conflict.

[gabrielcuvillier]

@kripken , about 'exceptions': no, Regal does not use exceptions. But by default, if no particular flag is provided at compilation time, Regal is compiled with exceptions support enabled. And when some projects using Regal require -fno-exceptions (which is the case on one of the project I am working on), there is some linker error:

error: DISABLE_EXCEPTION_THROWING was set (likely due to -fno-exceptions), which means no C++ exception throwing support code is linked in, but such support is required by symbol __cxa_begin_catch. Either do not set DISABLE_EXCEPTION_THROWING (if you do want exception throwing) or compile all source files with -fno-except (so that no exceptions support code is required); also make sure DISABLE_EXCEPTION_CATCHING is set to the right value - if you want exceptions, it should be off, and vice versa.

The message is maybe a bit confusing (and the topic also by the way, there is many combinations: '-fno-exceptions' vs '-fexceptions' vs 'nothing specified', DISABLE_EXCEPTION_THROWING = 0|1, DISABLE_EXCEPTION_CATCHING = 0|1), but as far as I understand from that linker error, it is not possible to mix .lib compiled with '-fno-exceptions' with .lib compiled with '-fexceptions' ( or without any particular flag ). Am I right with this observation ?

In the end, this explains why there is the two variants of Regal: one with exceptions enabled, and one without: to support linking it against both kind of libraries requirements. I decided to explicitly add the '-fexception' and '-s DISABLE_EXCEPTION_CATCHING=0' flags (contrast with '-fno-exceptions' and '-s DISABLE_EXCEPTION_CATCHING=1') to be 100% clear about the intent of each build. (same thing with '-pthread' + 'USE_THREADS=1').

[kripken]

cc @tlively for the linking error - I think it should be ok for a library that does not use exceptions itself to build without them, but be linked with other code that does use exceptions. But I don't remember the linking logic for features well enough.

[tlively]

I assume we're talking about Emscripten's emulated exceptions rather than the exceptions target feature. In that case the exceptions do not have any effect on the target features section, so the linker error is due to a real problem, not just a feature mismatch. Would compiling a library with exceptions support change any function signatures or something like that? @gabrielcuvillier What is the linker error you are seeing?

[gabrielcuvillier]

@tlively @kripken
Hmmm... I begin to think that the linker error I see is maybe a bug instead of a normal behavior.

I'll open a separate issue to explain how to trigger the linker error, so you can decide if it is normal behavior and I simply made a mistake somewhere, if it is normal and I did things right (in that case, there is indeed the need to have two library versions: except VS no-except), or if it is simply a bug :)

[gabrielcuvillier]

@juj @kripken
I found the issue related to the linker error with fexceptions/fno-exceptions!

While Regal does not use try/catch/throw anywhere, it is internally using the boost library, and boost does behave differently if exceptions are enabled or not at compilation time. The linker error is valid then: when Regal is compiled with no flags = exceptions are enabled, and then the library embed some __cxa_allocate_exception symbol that trigger the linker error (because we link with fno-exceptions).

This PR solves this issue by doing different builds when there is exceptions enabled or not (initial Kripken question regarding if it was really needed to do two different builds).

I can also disable Boost exceptions from within Regal, if you think it is more appropriate.

[sbc100]

Regal uses boost internally? Does it bundle its own copy of boost?

[kripken]

If it bundles it, then I think disabling boost exceptions inside regal is best (smaller code size, and avoids the need for a separate build). Hopefully there's a convenient option for that?

[gabrielcuvillier]

Yes, it bundles it, but a very small subset only.

I've tested several options to disable exceptions from within Regal/boost itself, but no success so far.
Even after removing every single instance of 'throw','catch','try','noexcept'. Additionally, it even does not seems they are really used in the end.

Whatever I do, there is various std exceptions symbols in the regal library file :( I think I'm stuck, I just don't understand from where do they are included !?

[gabrielcuvillier]

Well, I finally got it: this is not Regal, not Boost, but... simply the standard C++ containers that drives cxa stuff.

// test.cpp

#include <vector>
int main() {
    std::vector<int> dummy_vector;
    dummy_vector.push_back(1);
}

emcc -c test.cpp -o test.o

If you just open the content of test.o, you'll find __cxa_allocate_exception inside. That basically mean that any project using STL containers will bring all the exceptions infrastructure (and can't be linked against a '-fno-exception' library), unless it is compiled itself with fno-exceptions.

Honestly, I don't think this is a bug, but a normal "feature".

Maybe some documentation somewhere would be good though: something like "be careful that by default code is compiled with exceptions support (-fexceptions), even though exception catching is disabled. So, if you make a library that may behaves differently with exceptions enabled/disabled, and even indirectly (STL containers, or Boost), then the library can't be linked against with code with the -fno-exception flag. If you want this, you have to compile the library also with -fno-exception)"

(not sure I'm clear, but you get the idea).

So, as Regal is using std::vector and co. everywhere, I guess there is no other option to provide two library variants.

[kripken]

Hmm, on your testcase, I can build it to an object, then link it with exceptions or without, and both work. So even though __cxa_allocate_exception is there, it is handled ok by the linker.

I don't fully understand the details there, but if regal does not need exceptions at all, can you build all the source files with -fno-exceptions? Then you should not see that symbol there.

[gabrielcuvillier]

Ok, so I'll come up with the following fix: to compile Regal without exceptions in all cases. Sorry for the time you passed on this PR, I have been confused by this exception thing :)

Though, I've opened #10060 to explain the issue faced on a very simple test case.

[kripken]

Great, thanks @gabrielcuvillier ! (and yes, let's separately look into that issue you just filed)