Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[build] Breaking changes in emscripten 3.1.51 #3713

Closed
Peter0x44 opened this issue Jan 7, 2024 · 11 comments
Closed

[build] Breaking changes in emscripten 3.1.51 #3713

Peter0x44 opened this issue Jan 7, 2024 · 11 comments
Labels
platform: Web Web platform

Comments

@Peter0x44
Copy link
Contributor

Peter0x44 commented Jan 7, 2024

Currently, attempting to build the examples for web leads to the following error:

error: linker: Undefined symbol: glfwGetProcAddress(). Please pass -sGL_ENABLE_GET_PROC_ADDRESS at link time to link in glfwGetProcAddress().

This is because of the following breaking change in emscripten:

Breaking change: Using the *glGetProcAddress() family of functions now requires passing a linker flag -sGL_ENABLE_GET_PROC_ADDRESS. This prevents ports of native GL renderers from later accidentally attempting to activate "dormant" features if web browser implementations gain new WebGL extensions in the future, which *glGetProcAddress() is not able to support. (#20802)

that is documented here:
https://github.com/emscripten-core/emscripten/blob/main/ChangeLog.md#3151---121323

This change was introduced for emscripten 3.1.51.

The call to glfwGetProcAddress is located here:

rlLoadExtensions(glfwGetProcAddress);

So, either platforms/rcore_web.c needs to be reviewed not to call glfwGetProcAddress, or adding the flag to compile commands needs to be documented. I'm not really sure which, so I'm opening this issue, so I can get input from others, and we can research together.

@Peter0x44
Copy link
Contributor Author

Related:
emscripten-core/emscripten#20802

@Peter0x44
Copy link
Contributor Author

Peter0x44 commented Jan 7, 2024

https://emscripten.org/docs/optimizing/Optimizing-WebGL.html#optimizing-load-times-and-other-best-practices

This page here says:

Avoid using any of the *glGetProcAddress() API functions. Emscripten provides static linking to all of the GL API functions, even for all WebGL extensions. The *glGetProcAddress() API is only provided for compatibility to ease porting of existing code, but accessing WebGL via calling dynamically obtained function pointers is noticeably slower than direct function calls, due to extra function pointer security validation that dynamic dispatching has to do in asm.js/WebAssembly. Since Emscripten provides all of the GL entry points statically linked in, it is recommended to take advantage of this for best performance.

It seems like the correct solution may just be to delete this code completely, and not call rlLoadExtensions on PLATFORM_WEB?

@Peter0x44
Copy link
Contributor Author

I tried commenting out the call - things seemed to work "as normal", but I'm not sure if it could have any bad effects.
Looking at rlLoadExtensions, if it's not called, none of the RLGL.ExtSupport.* get updated. I'm not sure if rlgl uses the value of these variables anywhere internally, or if there is any user code that does. If emscripten just "static links" them anyway, it might be okay to just add a section that assigns hardcoded values for PLATFORM_WEB.

At the very least, I think this confirms that it doesn't matter if glfwGetProcAddress is called or not, and the function should be reviewed not to do so.

@raysan5
Copy link
Owner

raysan5 commented Jan 7, 2024

@Peter0x44 Thanks for reporting! This is really a breaking change, I need to think what's the best way to address it and somewhat keep compatibility with older Emscripten versions...

@Peter0x44
Copy link
Contributor Author

@raysan5 as far as I can tell there's no breaking change if the code is refactored in a way that glfwGetProcAddress doesn't get called. It shouldn't be necessary to call it at all, and it's recommended not to for performance reasons.

@raysan5
Copy link
Owner

raysan5 commented Jan 7, 2024

@Peter0x44 Oh, is it neither required if using old Emscripten versions?

@Peter0x44
Copy link
Contributor Author

The advice saying not to use it is 8 years old, so I assume it's okay. There's no actual code behavior change here, only what was allowed by default before now requires an extra linker flag.

@ghost
Copy link

ghost commented Jan 7, 2024

FYI: https://github.com/emscripten-core/emscripten/blob/main/tools/settings.py#L106

Edit: Although I don't use rcore_web.c or GLFW anymore (and SDL2 requires SDL_GL_GetProcAddress() which requires me to use GL_ENABLE_GET_PROC_ADDRESS for web regardsless), I'd suggest reviewing rlLoadExtensions(glfwGetProcAddress); more carefully. Without it, L2297-L2432 won't be getting called (specially L2342-L2344, L2353-L2355, L2364-L2366, L2372-L2410, L2429-L2432), which doesn't look like a good idea.

Edit 2: Just to not leave PLATFORM_WEB examples broken while the usage of rlLoadExtensions() gets reviewed (which will probably take some time), I'd suggest using adding the -sGL_ENABLE_GET_PROC_ADDRESS to the examples Makefiles. Everything will work as it used to and shouldn't affect users using previous versions of Emscripten since on previous versions GL_ENABLE_GET_PROC_ADDRESS is not defined, thus, the flag will do nothing.

@raysan5 raysan5 added the platform: Web Web platform label Jan 31, 2024
@raysan5
Copy link
Owner

raysan5 commented Feb 5, 2024

Added required flag in Makefiles in ea31bd4

EDIT: Properly reviewing rlLoadExtensions() to set all required/expected WebGL 1.0 flags will require some more time...

@raysan5
Copy link
Owner

raysan5 commented Feb 5, 2024

UPDATE: Adding -sGL_ENABLE_GET_PROC_ADDRESS breaks the build process on previous emscripten versions (GitHub Actions) (emscripten-core/emscripten#20802 (comment)).

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

@raysan5 raysan5 changed the title [web] Breaking change in Emscripten 3.1.51 not accounted for - needs review [build] Breaking changes in emscripten 3.1.51 Mar 7, 2024
@raysan5
Copy link
Owner

raysan5 commented Apr 20, 2024

It seems this issue has been already addressed at Emscripten side: emscripten-core/emscripten#20798

@raysan5 raysan5 closed this as completed Apr 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
platform: Web Web platform
Projects
None yet
Development

No branches or pull requests

2 participants