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

Feat/singlepass dylib #2802

Merged
merged 14 commits into from
Mar 17, 2022
Merged

Feat/singlepass dylib #2802

merged 14 commits into from
Mar 17, 2022

Conversation

ptitSeb
Copy link
Contributor

@ptitSeb ptitSeb commented Feb 23, 2022

Description

Enable Dylib+singlepass on Linux and macOS. Works fine, but larger wasm file may still fail to link on macOS/Aarch64.
For #2736

@ptitSeb ptitSeb requested a review from syrusakbary as a code owner February 23, 2022 11:33
@ptitSeb ptitSeb requested a review from Amanieu February 23, 2022 11:33
@@ -384,7 +384,7 @@ impl DylibArtifact {
// We are explicit on the target when the host system is
// Apple Silicon, otherwise compilation fails.
if target_triple_str == "arm64-apple-darwin" {
vec![format!("--target={}", target_triple_str)]
vec![form at!("--target={}", target_triple_str)]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes! :O

Comment on lines 404 to 405
(OperatingSystem::Linux, Architecture::X86_64) => vec!["-Wl,-z,notext"],
(OperatingSystem::MacOSX, Architecture::X86_64) => vec!["-Wl,-read_only_relocs,suppress"],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We actually want to remove both of these options. They currently tell the linker to allow text relocations which we don't want for PIC code.

@syrusakbary
Copy link
Member

I don't have enough context on this PR to approve myself. So we'll wait for @Amanieu review!

@Amanieu
Copy link
Contributor

Amanieu commented Mar 15, 2022

I tested this branch and it seems to generate a .so file with zero runtime relocations, which is good. However I don't see why the no-text flag is still needed. Additionally, this PR has a lot of unnecessary changes like:

  • emit_ldr_label and emit_nop that are never used.
  • Leftover change to traphandlers.c
  • It still includes the linker flags that we don't want. This should never trigger if we are indeed emitting code with zero relocations.

@ptitSeb
Copy link
Contributor Author

ptitSeb commented Mar 16, 2022

  • It still includes the linker flags that we don't want. This should never trigger if we are indeed emitting code with zero relocations.

The cross compile test is failling. It compile using dylib-cranelift targetting x86_64-linux-musl and fail with a relocation of type R_X86_64_64 for wasmer_vm_probestack in text. It only happens for musl. It seems to fails if I actually cross compile: if I cange the example to build to x86_64-linux-gnu on an x86_64 linux box, it works, but the same change on an arm64 box the sample fails. It seems to only happens when lld is used. ld/gold don't have the issue.

@Amanieu
Copy link
Contributor

Amanieu commented Mar 16, 2022

Can you dump the object files that are passed to the linker in both the success and failing cases? Also the arguments passed to the linker. The wasmer_vm_probestack symbol should be defined in the same object file, in a separate section that is added by engine-dylib.

@ptitSeb
Copy link
Contributor Author

ptitSeb commented Mar 16, 2022

Yes, I was thinking on the same thing, check the difference, if any, on both object files.

@ptitSeb
Copy link
Contributor Author

ptitSeb commented Mar 16, 2022

generated object file are identical in both case.
The dump show this

RELOCATION RECORDS FOR [.text]:
OFFSET           TYPE              VALUE
0000000000000002 R_X86_64_PC32     __WASMER_TRAMPOLINE0-0x0000000000000004
0000000000000008 R_X86_64_PC32     __WASMER_TRAMPOLINE1-0x0000000000000004
000000000000000e R_X86_64_PC32     __WASMER_TRAMPOLINE2-0x0000000000000004
0000000000000014 R_X86_64_PC32     __WASMER_TRAMPOLINE3-0x0000000000000004
000000000000001a R_X86_64_PC32     __WASMER_TRAMPOLINE4-0x0000000000000004
0000000000000020 R_X86_64_PC32     __WASMER_TRAMPOLINE5-0x0000000000000004
0000000000000026 R_X86_64_PC32     __WASMER_TRAMPOLINE6-0x0000000000000004
000000000000002c R_X86_64_PC32     __WASMER_TRAMPOLINE7-0x0000000000000004
0000000000000032 R_X86_64_PC32     __WASMER_TRAMPOLINE8-0x0000000000000004
0000000000000038 R_X86_64_PC32     __WASMER_TRAMPOLINE9-0x0000000000000004
000000000000003e R_X86_64_PC32     __WASMER_TRAMPOLINE10-0x0000000000000004
0000000000000044 R_X86_64_PC32     __WASMER_TRAMPOLINE11-0x0000000000000004
000000000000004a R_X86_64_PC32     __WASMER_TRAMPOLINE12-0x0000000000000004
0000000000000050 R_X86_64_PC32     __WASMER_TRAMPOLINE13-0x0000000000000004
0000000000000056 R_X86_64_PC32     __WASMER_TRAMPOLINE14-0x0000000000000004
000000000000005c R_X86_64_PC32     __WASMER_TRAMPOLINE15-0x0000000000000004
0000000000000062 R_X86_64_PC32     __WASMER_TRAMPOLINE16-0x0000000000000004
0000000000000068 R_X86_64_PC32     __WASMER_TRAMPOLINE17-0x0000000000000004
000000000000006e R_X86_64_PC32     __WASMER_TRAMPOLINE18-0x0000000000000004
0000000000000074 R_X86_64_PC32     __WASMER_TRAMPOLINE19-0x0000000000000004
000000000000007a R_X86_64_PC32     __WASMER_TRAMPOLINE20-0x0000000000000004
0000000000000080 R_X86_64_PC32     __WASMER_TRAMPOLINE21-0x0000000000000004
0000000000000086 R_X86_64_PC32     __WASMER_TRAMPOLINE22-0x0000000000000004
000000000000008c R_X86_64_PC32     __WASMER_TRAMPOLINE23-0x0000000000000004
0000000000000092 R_X86_64_PC32     __WASMER_TRAMPOLINE24-0x0000000000000004
0000000000000098 R_X86_64_PC32     __WASMER_TRAMPOLINE25-0x0000000000000004
000000000000009e R_X86_64_PC32     __WASMER_TRAMPOLINE26-0x0000000000000004
00000000000000a4 R_X86_64_PC32     __WASMER_TRAMPOLINE27-0x0000000000000004
00000000000000aa R_X86_64_PC32     __WASMER_TRAMPOLINE28-0x0000000000000004
00000000000000b0 R_X86_64_PC32     __WASMER_TRAMPOLINE29-0x0000000000000004
00000000000000b6 R_X86_64_PC32     __WASMER_TRAMPOLINE30-0x0000000000000004
00000000000000c0 R_X86_64_64       wasmer_vm_probestack

so there still is a reloc for the wasmer_vm_probestack.

@Amanieu
Copy link
Contributor

Amanieu commented Mar 16, 2022

Actually the R_X86_64_64 relocation seems strange. That's an absolute 64-bit relocation. I would have expected a R_X86_64_PC32 instead which is used for the offset in a call instruction.

I think that I know what the problem is, can you try applying this commit from #2807: 657bbae

You might also need to apply this fix: a280914

@ptitSeb
Copy link
Contributor Author

ptitSeb commented Mar 16, 2022

Yes, it works with both commit.

@ptitSeb
Copy link
Contributor Author

ptitSeb commented Mar 16, 2022

So I'll wait until the Stack/Corosensei branch is merged...

lib/object/src/module.rs Outdated Show resolved Hide resolved
@ptitSeb
Copy link
Contributor Author

ptitSeb commented Mar 17, 2022

bors r+

@bors
Copy link
Contributor

bors bot commented Mar 17, 2022

@bors bors bot merged commit 704d935 into master Mar 17, 2022
@bors bors bot deleted the feat/singlepass_dylib branch March 17, 2022 12:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants