GL_ENABLE_GET_PROC_ADDRESS

[juj]

Some renderers that are ports from native codebases have a problem that they use the *glGetProcAddress() functions to dynamically discover the function entry points for different GL extensions.

In native GL world, there is a tacit guarantee that if an extension GL_FOO includes an entrypoint glFoo(), then if an implementation advertise support for GL_FOO extension, then calling glGetProcAddress("glFoo") can be expected to succeed.

More robust user code would check not only the presence of the extension, but also that the function glGetProcAddress("glFoo") does not return null, but such robustness is not necessarily built in to user applications since in the native GL world, this expectation doesn't generally fail.

Since some 6 years apo, our best practices document has advocated users against using *glGetProcAddress() functions when targeting the web: https://emscripten.org/docs/optimizing/Optimizing-WebGL.html#optimizing-load-times-and-other-best-practices , due to the massive code size bloat that it causes (but also due to the performance problems that using it brings). Users are advised to not use that function.

Nevertheless, the function exists to ease porting existing codebases and to maintain parity between platforms.

One additional identified drawback is that this *glGetProcAddress() scheme enables GL code that is compiled to the web the possibility to activate "dormant" features from native GL side that were not enabled at the time the site is built; but later when a browser exposes a new WebGL extension, it can suddenly cause the site to try to activate those GL features.

This by itself wouldn't be a problem, except that WebAssembly & WebGL do not support a facility to enable a true enumerating *glGetProcAddress() implementation, but we must emulate that via a big if-else string search table. And so this statically implemented if-else chain cannot automatically manage new extension entry points as they come online when browsers evolve.

To fix above issues, this PR moves the *glGetProcAddress() functions behind a dedicated linker flag -sGL_ENABLE_GET_PROC_ADDRESS=1 that users must now pass if they want to use a *glGetProcAddress() function. This enables our implementation to explicitly restrict to only advertising WebGL extensions that exist at the time that the site is built, so that the limited implementation of *glGetProcAddress() won't be able to break the promise that the existence of GL_FOO extension should imply that calling glGetProcAddress("glFoo") would succeed.

Fixes #20798

[sbc100]

There is quite a lot in the PR. Perhaps we could split out the adding of support for the new extensions? (that change could land either before or after the adding of the new setting).

[juj]

There is quite a lot in the PR. Perhaps we could split out the adding of support for the new extensions? (that change could land either before or after the adding of the new setting).

I really prefer not to. I don't think the changes here have large technical complexity itself, it is mostly all just plumbing changes.

[juj]

The one thing to consider here is that this will be a breaking change, and possibly could be a breaking change to surprisingly many Emscripten users that build GLFW/GLEW/EGL based projects, since after this PR they will need to start passing -sGL_ENABLE_GET_PROC_ADDRESS in the link if they use *glGetProcAddress(). Currently the linker error comes from wasm-ld, so we don't have a mechanism to interject a helpful error message to the users. (maybe we could start intercepting wasm-ld if we care about this?)

It looks like SDL currently unconditionally uses that, so to mitigate the churn, I did make it enabled by default for SDL targets at least. I suppose I could do the same for GLFW/GLEW/EGL as well if we are concerned.

The recommendation for Filament would be to get rid of *glGetProcAddress() altogether for better codegen and GL performance.

[sbc100]

How about if we land this new setting and start by only enabling in -sSTRICT mode? (Which is for folks that are opting int these types of things early).

That would allows us to defer to decision around how and when to start breaking other users.

Regarding intercepting the wasm-ld error message and augmenting them.. that is something I've tried to avoid doing in the past and maybe we can avoid it here too: How about including a dummy version of glGetProcAddress in the JS library which triggers and meaningful error when included? (see https://github.com/emscripten-core/emscripten/blob/main/src/library_exceptions_stub.js for an example of where we do this already)

[kainino0x]

I didn't think about the fact that the code size would only need to impact GetProcAddress builds which are already quite large. Nice!

Note I've proposed renaming EXT_polygon_offset_clamp and EXT_clip_control to the WebGL WG so Emscripten will have to make changes if they do. This is fine though.

LGTM with the caveat that I'll leave decisions about breaking changes to other maintainers. I think it's a good change though.

[sbc100]

There is quite a lot in the PR. Perhaps we could split out the adding of support for the new extensions? (that change could land either before or after the adding of the new setting).

I really prefer not to. I don't think the changes here have large technical complexity itself, it is mostly all just plumbing changes.

That in kind of my point though. All that unrelated plumbing boilerplate distracts from the functional stuff when trying to understand this PR.

I won't push hard for this, because I know how busy you likely are, but IMHO its always best to separate these things out, for the sake of the review process and also for future us who may be bisecting these changes.

[juj]

Ok, removed the new extensions from this PR.

Also added the link time errors, that was a great suggestion. With that, I would be happy to start breaking users directly, since the error message will clearly say what they need to do.

[sbc100]

lgtm

BTW, what do you you think about putting this behind -sSTRICT first? (then there would be no changes to the test/ files needed as part of this PR). Maybe no need to such a period because we have good error reporting?

[sbc100]

Why is this outer #if required?

[juj]

BTW, what do you you think about putting this behind -sSTRICT first?

My impression (given the "bad Emscripten" comments from linked thread) is that the WebGL people are regarding this to be a critical problem that prevents standardization from proceeding, so it doesn't seem like people should be given an opt-in choice.

I am happy that there will be a meaningful error message that hints how to fix the build error at least. Maybe there won't be too many angry users with the help of that.

I don't know if there are other users like Filament who might have an issue like this with their use of *glGetProcAddress and possibly dormant features that might be attempted to be accessed via *glGetProcAddress. As a data point, I checked that Unity has migrated away from using *glGetProcAddress a long time ago in August 2018.

[sbc100]

(given the "bad Emscripten" comments from linked thread)

Sounds good to me.

BTW what is '"bad Emscripten" comments from linked thread'? (Sorry I don't see the link )

[kainino0x]

@sbc100 This PR is meant to be linked with issue #20798
That issue was mentioned in (and so has a GitHub-generated link to) KhronosGroup/WebGL#3604 that juj is referring to.

[sbc100]

@sbc100 This PR is meant to be linked with issue #20798 That issue was mentioned in (and so has a GitHub-generated link to) KhronosGroup/WebGL#3604 that juj is referring to.

It seems like that issue relates to "advertising extension we don't implement", and not "glGetProcAddress bloats applications". IIUC fix for "advertising extension we don't implement" (i.e. the new setting in the title of this PR) is completely separate and unrelated the "advertising extension we don't implement".

So just as a point of clarity, I don't think putting -sGL_ENABLE_GET_PROC_ADDRESS behind -sSTRICT would effect the fix for that issue. However, I also don't need to push for that, I'm happy to go directly to on by default.. I just want to check my assumption that the bug fix and the new flag are separate?

[juj]

fix for "advertising extension we don't implement" [...] is completely separate and unrelated the "advertising extension we don't implement".

Maybe some typo?

I just want to check my assumption that the bug fix and the new flag are separate?

No, the new flag is fundamental to the bugfix.

Because there may exist codebases like Filament that depend on the fact that all enumerated GL extensions should be acquirable via *glGetProcAddress() function, we need to synchronize these two to go 1:1 hand in hand.

Utopistically, the correct fix to synchronize these two would be to repair *glGetProcAddress() to perform true enumeration of GL function pointers that WebGL extensions expose. But that is impossible to do in practical terms due to how WebAssembly-JS interop and WebGL specification works.

So the realistic second best thing instead is to limit WebGL contexts to only advertise extensions that match 1:1 what *glGetProcAddress() is able to serve at the time that the codebase was compiled.

But that is a drastic pessimization in the big picture, and would be horrible to ship to all users, since only a small set of users utilize *glGetProcAddress(). Codebases that do not, and instead follow the best practices guide to statically link to WebGL extension functions are immune to this limitation of *glGetProcAddress().

If we holistically applied this change to all codebases and not just those that use *glGetProcAddress(), we would have two major problems:
a) the change would cause a code size impact also to developers that didn't suffer from the problem in the first place, and
b) the change would needlessly require all users to update Emscripten whenever the want to "see" a new WebGL extension

By adopting this flag, we can target the above two limitations only to developers who actually do use *glGetProcAddress() and other WebGL utilizing projects won't be disturbed.

[sbc100]

What about if the user calls glGetString(GL_EXTENSIONS) but they don't set GL_ENABLE_GET_PROC_ADDRESS? Is it OK that they see the full unfiltered list in this case?

Similarly emscripten_webgl_get_supported_extensions.. if GL_ENABLE_GET_PROC_ADDRESS is not set what do we expect it to return?

How about GL_SUPPORT_AUTOMATIC_ENABLE_EXTENSIONS which is enabled by default.. do we want to see the full unfiltered list by default?

How about the existing wrapping of getSupportedExtensions by GL_DISABLE_HALF_FLOAT_EXTENSION_IF_BROKEN? Does that wrapper simply layer in top of this one?

[kainino0x]

I think it should be OK to expose the unfiltered list in all cases when GetProcAddress is not enabled, because any code that uses an entry point Emscripten doesn't implement will cause a linker error. (And any extension that doesn't have entry points is OK.) It is a little unfortunate that they will have to #ifdef __EMSCRIPTEN__ though instead of #ifdef GL_EXT_clip_control which many codebases will already have.

Come to think of it, it must be spec-valid in OpenGL for an implementation to list extensions that aren't available in the header - because you can use an older header with a newer driver. It's less guaranteed that applications would be robust to this though.

How about the existing wrapping of getSupportedExtensions by GL_DISABLE_HALF_FLOAT_EXTENSION_IF_BROKEN? Does that wrapper simply layer in top of this one?

That wrapper shadows WebGLRenderingContext.prototype.getSupportedExtensions with ctx.getSupportedExtensions so I think it will apply first (at the JS level) and then this patch will apply on top.

[kainino0x]

It's less guaranteed that applications would be robust to this though.

Er, my point is, Emscripten could remove them from the header instead of filtering them out, and that would be a less robust but technically valid fix (that would work for Filament at least in this case).

Actually it would be more robust than I thought - if we did this, we'd be disabling not just the static entrypoint glClipControlEXT, but also PFNGLCLIPCONTROLEXTPROC which should be used by any program that's using GetProcAddress on that function. Example: https://github.com/google/filament/blob/b007e9137eb029e256675eceb5fd52e6b873cecd/filament/backend/src/opengl/gl_headers.h#L132

[juj]

What about if the user calls glGetString(GL_EXTENSIONS) but they don't set GL_ENABLE_GET_PROC_ADDRESS? Is it OK that they see the full unfiltered list in this case?

Yes that is ok. That is just the common case as if user had hand-written native JavaScript code and called ctx.getSupportedExtensions() in it.

Similarly emscripten_webgl_get_supported_extensions.. if GL_ENABLE_GET_PROC_ADDRESS is not set what do we expect it to return?

Same as above. All extensions that the WebGL implementation advertises, unfiltered.

How about GL_SUPPORT_AUTOMATIC_ENABLE_EXTENSIONS which is enabled by default.. do we want to see the full unfiltered list by default?

Yes. The root problem is in the use of the glGetProcAddress() function, because it is unable to truly enumerate extensions.

How about the existing wrapping of getSupportedExtensions by GL_DISABLE_HALF_FLOAT_EXTENSION_IF_BROKEN? Does that wrapper simply layer in top of this one?

Yeah, the two things should cascade well together.

[bkaradzic]

This breaks code building with new SDK, and once applied, it breaks build with old SDK.

I had to add GL_ENABLE_GET_PROC_ADDRESS to get GH actions to build with latest. But now when I build with previous version I get following error:

em++: error: Attempt to set a non-existent setting: 'GL_ENABLE_GET_PROC_ADDRESS'

[sbc100]

This seems like a generic problem that exists whenever we add a new setting to emscripten. I suppose we could consider downgrading the Attempt to set a non-existent setting error to a warning so that it could be disabled?

However, I wonder if this isn't a normal/common thing with all compiler tools. When clang or gcc adds a new flag that flag isn't expected to be silently ignored by older versions of clang, is it? I would assume the solution would be to detect which version of emcc is being used and set the new flag accordingly.. isn't that how new flags work in most command line applications?

[MarkCallow]

I was pointed to this PR as a victim of this change. My CI builds suddenly stopped working.

As a long time OpenGL user, the correct fix seems to me to be filtering the string returned by glGetExtensions to only include the extensions supported by the Emscripten OpenGL emulation. The other issue mentioned here, the code size when a glGetProcAddress function is used, seems a red herring. If the application is not calling a GetProcAddress function why is the code linked into it?

[sbc100]

I was pointed to this PR as a victim of this change. My CI builds suddenly stopped working.

Can I ask how this change broke your build? My understanding was that it should only have broken users who's programs reference the GetProcAddress family of function, but maybe I am misunderstanding? What error are you seeing?

[MarkCallow]

Can I ask how this change broke your build?

My code uses emscripten_GetProcAddress(). It uses *GetProcAddress because it is a part of a library which has many non-GL uses and I do not wish to force non-GL users to have to link with OpenGL.

In native OpenGL you have to (a) check if the implementation supports an extension by calling glGetString or glGetStringi and (b) call *GetProcAddress to retrieve any function pointers needed to use it. Many OpenGL drivers support multiple versions, as a result of which *GetProcAddress may return a non-NULL value even when the current context does not support the queried extension. Therefore any well-behaved OpenGL program wiil do (a) first.

Also the OpenGL spec. says

"GetString returns ... the extension names (in the EXTENSIONS string) that can be supported by the current GL context. Thus, if the client and server support different ... extensions, a compatible ... list of extensions is returned."

We may consider WebGL to be the server in this case and Emscripten's emulation the client. Therefore filtering the extension list instead of returning WebGL's list verbatim will be GL spec. compliant and will fix the problem without a breaking change.

[juj]

Opened #20948 to flip the flag into opt-out from opt-in.

[kainino0x]

The other issue mentioned here, the code size when a glGetProcAddress function is used, seems a red herring. If the application is not calling a GetProcAddress function why is the code linked into it?

It's not that it hurts code size for applications that don't use GetProcAddress - it's that it's a known "footgun" because GetProcAddress has a disproportionately huge code size, and when porting native applications it's easy to link it accidentally when you don't really need to.