[libunwind] Add Unwind-wasm.c to CMakeLists.txt #20371
Conversation
Even though we don't use `CMakeLists.txt`, `Unwind-wasm.c` needs to be included in the file to be a part of libunwind. Reflecting changes of llvm/llvm-project#67770. The order of headers and inserted newlines are to satisfy LLVM CI and clang-format. - `stdbool.h` should be before `config.h` because otherwise `bool` is not defined. But there should be a newline between them because otherwise clang-format will reorder them alphabetically - We need at least one header outside of `#ifdef __USING_WASM_EXCEPTIONS__` because LLVM CI complains there should be at least one declaration within a compilation unit, and `stdbool.h` doesn't have any, so we need another header outside - `threads.h` doesn't exist in other platforms. (It's in musl) So it needs to be in `#ifdef __USING_WASM_EXCEPTIONS__`
|
Is there any point in landing this downstream in emscripten? Should we not just wait for the llvm change to make its way into a release? Doing this way will cause our patch size against 16.0.6 to increase, no? |
|
Yeah it might not be necessary... I just wanted to sync them because I don't expect changes to libunwind for a while. Not sure what you mean by increasing the patch size. |
At any given time emscripten carries a certain set of patches from the upstream llvm repo. For example, right now that is the difference between the 16.0.6 tag and our branch in our llvm fork (emscripten-libs-16). Every time we change emscripten's copy of these core libraries it increases our patch size. Even if that change is cherry-picked from llvm main (as this one is). One way so see that the patch size is increased would be to run Cherry-picking from main might be OK, if there was good reason to, but I'm not sure there is in this case. |
|
I guess I see what you mean by increasing the patch size, but does the patch size matter, or is there a reason we should minimize it? |
In general, we want to keep our deviations from upstream as small as possible yes. This makes all out processes like merging and rebasing easier. In this case that patch happens to be one that is already landed upsteam on main.. so its unlikely to cause trouble. But I also don't see any reason to cherry-pick it early. |
Perhaps there is some reason that I am missing? Why would we want this file to be on llvm-main and the other files to be on llvm-16? |
No I wasn't thinking in terms of llvm-main vs. llvm-16. Because I recently upstreamed our libc++abi / libunwind changes to the llvm repo, but this small change was missing, I just wanted to sync them, that's all. As you pointed out, this change doesn't affect us one way or another so it's not urgent or anything. I was just wondering why we needed to make the patch size small. |
Because we sync to llvm only on major release I think its OK to be out-of-sync with main at any given time... unless the changes from main are some kind of critical bug fix. Of course, if you disagree and feel that these particular files should llvm-main we can land this. |
|
It's not urgent; I will close it then. |
Even though we don't use
CMakeLists.txt,Unwind-wasm.cneeds to be included in the file to be a part of libunwind. Reflecting changes of llvm/llvm-project#67770.The order of headers and inserted newlines are to satisfy LLVM CI and clang-format.
stdbool.hshould be beforeconfig.hbecause otherwiseboolis not defined. But there should be a newline between them because otherwise clang-format will reorder them alphabetically#ifdef __USING_WASM_EXCEPTIONS__because LLVM CI complains there should be at least one declaration within a compilation unit, andstdbool.hdoesn't have any, so we need another header outsidethreads.hdoesn't exist in other platforms. (It's in musl) So it needs to be in#ifdef __USING_WASM_EXCEPTIONS__