Skip to content

aarch64: Support GOT Relative relocations in PIC mode - #5550

Merged
jameysharp merged 4 commits into
bytecodealliance:mainfrom
afonso360:aarch64-got
Feb 15, 2023
Merged

jameysharp merged 4 commits into
bytecodealliance:mainfrom
afonso360:aarch64-got

Conversation

@afonso360

Copy link
Copy Markdown
Contributor

👋 Hey,

This PR adds support for using the GOT in PIC mode on AArch64. It implements the relocations for both ELF and MachO.

In order to support the MachO relocations we need to update the object crate. Unfortunately that also brings a few other dependencies which also need to be vetted.

@cfallin has already validated the object crate update up to 0.30.0 in #5434 so we just need to do 0.30.0 -> 0.30.1 and the rest of the new dependencies.

Tested this using the awesome example provided by @acw in zulip.
On aarch64-linux I was able to fully test this, on aarch64-darwin the decompiled file seems correct but I don't have a M1/2 machine to link and run it. Maybe @acw can test and report back if anything is wrong?

Fixes #2907
Fixes #5544

@github-actions github-actions Bot added cranelift Issues related to the Cranelift code generator cranelift:area:aarch64 Issues related to AArch64 backend. cranelift:module labels Jan 8, 2023
@acw

acw commented Jan 9, 2023

Copy link
Copy Markdown
Contributor

Interesting. Looks like it runs and generates output of the sort I'd expect:

% cargo run               
...
% objdump -dr output.o    

output.o:       file format mach-o arm64

Disassembly of section __TEXT,__text:

0000000000000020 <_gogogo>:
      20: fd 7b bf a9   stp     x29, x30, [sp, #-16]!
      24: fd 03 00 91   mov     x29, sp
      28: 00 00 00 90   adrp    x0, 0x0 <_gogogo+0x8>
                0000000000000028:  ARM64_RELOC_GOT_LOAD_PAGE21  _variable-name-x
      2c: 00 00 40 f9   ldr     x0, [x0]
                000000000000002c:  ARM64_RELOC_GOT_LOAD_PAGEOFF12       _variable-name-x
      30: 81 00 80 d2   mov     x1, #4
      34: 03 00 00 90   adrp    x3, 0x0 <_gogogo+0x14>
                0000000000000034:  ARM64_RELOC_GOT_LOAD_PAGE21  _print
      38: 63 00 40 f9   ldr     x3, [x3]
                0000000000000038:  ARM64_RELOC_GOT_LOAD_PAGEOFF12       _print
      3c: 60 00 3f d6   blr     x3
      40: fd 7b c1 a8   ldp     x29, x30, [sp], #16
      44: c0 03 5f d6   ret

But that particular relocation type isn't supported on arm64:

% gcc -o test rts.c output.o
ld: in section __TEXT,__text reloc 1: pcrel and ARM64_RELOC_GOT_LOAD_PAGEOFF12 not supported file 'output.o' for architecture arm64
clang: error: linker command failed with exit code 1 (use -v to see invocation)

Although, I did earlier discover that my Xcode suite is a version behind, so I'm going to save this comment for reference, and then see if this magically gets better after that.

@acw

acw commented Jan 9, 2023

Copy link
Copy Markdown
Contributor

Although, I did earlier discover that my Xcode suite is a version behind, so I'm going to save this comment for reference, and then see if this magically gets better after that.

Sad to say: that wasn't it. Same error, after making sure everything was up-to-date.

@afonso360

afonso360 commented Jan 9, 2023 •

Copy link
Copy Markdown
Contributor Author

I think I got it, the Page Offset relocation was marked as being PC relative when writing the output file. And I think that was causing some confusion in the linker. Can you try with this latest commit?

@acw

acw commented Jan 9, 2023

Copy link
Copy Markdown
Contributor

It works!

% cargo run
    Finished dev [unoptimized + debuginfo] target(s) in 0.03s
     Running `target/debug/macos-example`
% objdump -dr output.o

output.o:       file format mach-o arm64

Disassembly of section __TEXT,__text:

0000000000000020 <_gogogo>:
      20: fd 7b bf a9   stp     x29, x30, [sp, #-16]!
      24: fd 03 00 91   mov     x29, sp
      28: 00 00 00 90   adrp    x0, 0x0 <_gogogo+0x8>
                0000000000000028:  ARM64_RELOC_GOT_LOAD_PAGE21  _variable-name-x
      2c: 00 00 40 f9   ldr     x0, [x0]
                000000000000002c:  ARM64_RELOC_GOT_LOAD_PAGEOFF12       _variable-name-x
      30: 81 00 80 d2   mov     x1, #4
      34: 03 00 00 90   adrp    x3, 0x0 <_gogogo+0x14>
                0000000000000034:  ARM64_RELOC_GOT_LOAD_PAGE21  _print
      38: 63 00 40 f9   ldr     x3, [x3]
                0000000000000038:  ARM64_RELOC_GOT_LOAD_PAGEOFF12       _print
      3c: 60 00 3f d6   blr     x3
      40: fd 7b c1 a8   ldp     x29, x30, [sp], #16
      44: c0 03 5f d6   ret
% gcc -fPIC -g3 -o test rts.c output.o
% ./test
x = 4

Thank you!

@cfallin

cfallin commented Jan 23, 2023

Copy link
Copy Markdown
Member

I worked through the audit backlog and pushed a commit to this branch with audit results. Someone else will need to review the PR since it has a commit from me now.

@afonso360

afonso360 commented Jan 26, 2023 •

Copy link
Copy Markdown
Contributor Author

Thanks for looking into this! I had to rebase since we had a conflict in Cargo.lock but should be ready for review now.

Edit: It looks like we have a similar situation to #5619 and its failing in CI due to duplicate deps.

I can de-duplicate dependencies the same way I did in #5619 but I'm not sure I can de-duplicate hashbrown without updating a bunch of other stuff.

Edit2: I can't de-duplicate hashbrown, version 0.12 is depended by indexmap and even the latest published version 1.9.2 still depends on hashbrown 0.12. Not entirely sure what to do now.

@jameysharp

Copy link
Copy Markdown
Contributor

If I'm reading the cargo-deny documentation correctly, it looks like we should be able to add something like this to deny.toml:

[[bans.skip]]
name = "hashbrown"
version = "=0.12.3"

Unlike cargo-vet exceptions, I think it's reasonable to add this kind of targeted exception to the "no duplicate dependencies" rule.

@afonso360

Copy link
Copy Markdown
Contributor Author

Yeah, that seemed to have worked locally. I've applied the same updates that I did in the other PR (except object which is still at 0.30.1 instead of .3).

@afonso360

afonso360 commented Feb 5, 2023 •

Copy link
Copy Markdown
Contributor Author

@cfallin would it be possible to vet the remaining dependencies? Once i updated our version of gimli it started saying that we need a safe to deploy validation, but weirdly it didn't before when it was just a transitive dependency of object 0.30.

Edit: It looks like some of these might get vetted via #5513

@cfallin

cfallin commented Feb 8, 2023

Copy link
Copy Markdown
Member

@afonso360 would you be willing to rebase? I think this is good to go otherwise!

@afonso360

Copy link
Copy Markdown
Contributor Author

Rebased on top of main, and removed the hashbrown exception. #5513 includes sort of the same exception, but ignores the tree of indexmap which solves the same issue I was having.

@jameysharp jameysharp left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good to me! Thanks for your patience as we figure out how best to use cargo-vet.

@jameysharp
jameysharp merged commit eabd43a into bytecodealliance:main Feb 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cranelift:area:aarch64 Issues related to AArch64 backend. cranelift:module cranelift Issues related to the Cranelift code generator

Projects

None yet

4 participants