Upgrade LLVM to version 12 #2542
Conversation
to see what breaks, and also to get NixOS/nixpkgs@5a923e5 which may allow us to use `rustc` from `nixpkgs` again (see #2519)
This reverts commit d5e3399.
This reverts commit ab44bbf.
|
@basvandijk increased the timeout, such that |
|
I think increasing the timeout won't be necessary if we merge and deploy https://github.com/dfinity-lab/hydra-jobsets/pull/99 |
Sorry, I was confused. That PR is not related, we will have to increase the timeout or somehow upload the nix artifacts to the cache manually (assuming that's possible). |
to include the patch we tried in #2542
nomeata
force-pushed
the
joachim/llvm-12
branch
from
f6fa941
to
4fdefc5
Compare
src/linking/linkModule.ml
Outdated
| @@ -599,7 +599,6 @@ let fill_table_base_import new_base : module_' -> module_' = fun m -> | |||
| (* Concatenation of modules *) | |||
|
|
|||
| let join_modules (em1 : extended_module) (m2 : module_') (ns2 : name_section) : extended_module = | |||
| assert (m2.start = None); | |||
There was a problem hiding this comment.
I forgot if we discussed this. If m2 now has a (start) function, is it ok to ignore it?
There was a problem hiding this comment.
So this is really hacky. I think as first thing we should clarify here (in comments/documentation) that this linker and join_modules in particular cannot link/join two arbitrary Wasm modules, we have various assumptions like the second module being generated by LLVM and first one by moc (and details related to LLVM- and moc- generated code).
In this case, module 2 is the RTS module and it has a start after updating to LLVM 12: (start $__wasm_apply_global_relocs). We add (call $__wasm_apply_global_relocs) to the final module after joining modules, so that's why we ignore start here.
So it's not OK to ignore start in general, we only handle the case specific to LLVM 12 here. If we want this function to link arbitrary Wasm modules then this is wrong.
There was a problem hiding this comment.
But sound it would be cleaner to not ignore the start here, bit rather make sure it is called from the linked start function? But that's what the other comment would yield, I think.
The linker does rely on internals of drun output but ideally not of LLVM internals (like function names in the debug section).
There was a problem hiding this comment.
Second module's start is now prepended to the merged module's start, so marking this as resolved as well.
There was a problem hiding this comment.
Sigh.. GitHub UI does not show new inline comments without refreshing the page, so I missed your second comment above.
I guess we could join starts here, instead of ignoring it here and adding module 2's start after join_modules. Will update.
There was a problem hiding this comment.
Done. Not sure if it's any better though..
|
With |
…S start to merged module start
|
I feel a bit uneasy about rustc and clang using different versions of LLVM, but maybe that's OK. Should we make this ready for review? |
|
Should we put the linker improvement into is own PR (can land first, I guess, at it's backwash m backwards compatible)? Or is that OCD on my part? |
- Implement extended name section (https://github.com/WebAssembly/extended-name-section/blob/master/proposals/extended-name-section/Overview.md) - Handle `start` functions in the RTS module in linker These changes are needed to update to LLVM 12: #2542
|
- Implement extended name section (https://github.com/WebAssembly/extended-name-section/blob/master/proposals/extended-name-section/Overview.md) - Handle `start` functions in the RTS module in linker These changes are needed to update to LLVM 12: #2542
No description provided.