-
Notifications
You must be signed in to change notification settings - Fork 697
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
Unable to use LTO in Bazel. #971
Comments
I think the problem is that we don't build all libraries in all configurations when building the emsdk tar archives. In order to fix this we would need to have bazel create additional libraries, perhaps in a supplemental archive, which are also archived somewhere. I'm not sure what the best way to do that might be? @dschuff @kripken .. should be look into adding pre-built lto versions of all the standard libraries to the emsdk download? How much do we care about marginal increases in download size? |
But that used to work, since previously those missing libraries were built on demand. However, |
Yes, the bazel toolchain does not support the "build libraries on demand" feature of emscripten. I think this is generally a good thing.. one does not generally want the contents of the sysroot to change during a build. |
Yeah, I agree it's a good thing, once things are working :) |
If there is a strong reason to add more builds by default that might make sense. But the matrix of combinations is constantly increasing... In general I think the right workflow would be to invoke
That is, to fully construct the necessary sysroot first. Could the bazel support do that automatically perhaps? (Does it have the necessary information? Sorry, I'm not familiar with bazel.) |
I think the problem is that bazel pulls the .tar.gz files that we generate. @PiotrSikora @walkingeyerobot do you know if we can run a kind of "post-download" step on the downloaded archive? That would be a good solution I think.. as long as it was a just a one-off thing that run only on first download. |
You can override the This is what we're currently doing:
|
In that case I think you should be able to run We should also do this for the un-patched toolchain I think. |
This actually doesn't work. While I tried to make this work, patching Is there a chance that we could include LTO system in the official binaries? Looks like I'm not the only one that asked about this (see: #807). |
emsdk still relies on shipping just a subset of libraries pre-built, while leaving some libraries to be built on demand. This allows is to strike a balance between build time and download size and user convenience. Even if we start shipping LTO libraries there are also maybe other configuration for the system libraries: I wonder if we can come up with a way to allow bazel to allow built-on-demand libraries? Or maybe to allow users to inject their own overlay of extra pre-built libraries. Possible solutions:
Having said all that as a short term solution we could consider adding just one more configuration (i.e. doubling the amount of libraries we include in the tar archive). |
I think this would be the perfect solution (even ignoring LTO issue, prebuilt libraries should be avoided in Bazel ecosystem). Could we get this done? cc @trybka @walkingeyerobot |
This is definitely on our TODO list, but basically I was the only person who thought this was useful. Having another use-case is motivating. |
Just ran into this issue myself, would be great to get this going properly. Is there a workaround for now I could use to get LTO? |
A workaround here is tricky, but doable. The problem is the system libraries don't exist with the flags relevant to lto. Right now, the bazel build is limited to what prebuilts exist, and lto prebuilts don't exist. If you were to build them yourself outside of bazel (as described here: #971 (comment) ) and then put them where bazel can see them, that should work. I have been working to fix this. The proper fix is to have bazel build the system libraries on demand. This is tricky, but I'm making progress. |
So, I don't know much about bazel, but with LTO I think it's still possible to link the same system libraries that a non-LTO build would use (i.e. you can link LTO/bitcode object files together with regular object files). So it might just be a matter of juggling the linker search paths, even if you can't build LTO-enabled libraries. Emscripten does use LTO-enabled library builds by default, but I don't think it's actually necessary. |
Ah yeah, it should possible to simply drop All |
To whoever having a problem with FROZEN_CACHE. In my case I was building with Memory64, then hit this problem. Then I disabled FROZEN_CACHE, then I hit the "read-only blabla" issue with bazel in sandbox mode. Here're two different workarounds that get this to build:
In sandbox mode, bazel is allowed to write into ~/.cache folder, but not emscripten's default implementation |
Does anyone know if it would be possible to change the default for the bazel tool chain from |
I assume you meant for My understanding is that it would require people to patch emscripten and build it from source and in emsdk people need to bypass prebuilt binaries (referred in emscripten_deps macro in emsdk) but rather refer to the built-from-souce version. |
I don't think so no. I believe what is being proposed here is to set the cache location via Setting |
@sbc100 @walkingeyerobot would it be possible to build the Emscripten ports when declaring the toolchain in a bazel workspace, e.g., something like: load("@emsdk//:emscripten_deps.bzl", emsdk_emscripten_ports = "emscripten_ports")
emsdk_emscripten_ports("bzip2", "zlib", "sqlite3") The idea being that if the dependencies of the given ports (and their flags) were already built, then it wouldn't be necessary to build/lock the cache later on, right? |
@wzheng21 for some reason it seems like it was sufficient for me to just set |
For context, I'm working on updating Proxy-Wasm C++ SDK from our custom toolchain to @emsdk (see: proxy-wasm/proxy-wasm-cpp-sdk#132) and I'm seeing some regressions in terms of features.
Same without
--no-entry
for commands:The text was updated successfully, but these errors were encountered: