cranelift: Implement TLS on aarch64 Mach-O (Apple Silicon) #5434

nathanwhit · 2022-12-14T02:44:11Z

This PR implements support for TLS on aarch64 Mach-O (i.e. Apple silicon). This also includes an update to object as this PR depends on a fix released in object 0.30.

For testing, I've added a filetest similar to the existing test for aarch64 ELF. I wasn't sure if I should add an emit test as well, since there doesn't seem to be one for TLS on aarch64 ELF. I've also tested these changes on a local branch of cg_clif, and it passes the TLS tests there.

cfallin

Nicely done! The register handling and regalloc metadata for the new pseudoinstruction is the trickiest part, and that looks basically correct here. There are a few things that could actually be omitted in the call (but no harm when present). A few nits below but otherwise LGTM.

It looks like the cargo-deny failure is because two versions of object are present in the deps; could you find where object 0.29 is still in use and resolve that? Then the upgrade to 0.30 is causing the cargo-vet failure. Our policy is to have a core committer vet upgrades, so I'll take a look at the 0.29 -> 0.30 diff and push a "vetted by" commit to this PR if it looks OK.

Thanks again!

cfallin · 2022-12-14T02:49:21Z

cranelift/codegen/src/binemit/mod.rs

@@ -56,6 +56,14 @@ pub enum Reloc {
    /// Mach-O x86_64 32 bit signed PC relative offset to a `__thread_vars` entry.
    MachOX86_64Tlv,

+    /// Mach-O AAarch64 TLS


s/AAarch64/Aarch64/ (and in comment below too)

cfallin · 2022-12-14T02:52:40Z

cranelift/codegen/src/isa/aarch64/inst/emit.rs

+                Inst::CallInd {
+                    info: crate::isa::Box::new(CallIndInfo {
+                        rn: rtmp.to_reg(),
+                        uses: smallvec![CallArgPair {


uses and defs and clobbers can all be empty here, as this emission happens after regalloc runs so we don't need any of this metadata.

cranelift/codegen/src/isa/aarch64/lower.isle

cfallin · 2022-12-14T03:25:03Z

I did an audit of object's upgrade; however cargo vet is showing an audit backlog of hashbrown, once_cell, and ahash, and I don't really feel qualified to audit those at all. Anyone else want to take a crack at it (@alexcrichton, @fitzgen, @jameysharp, @elliottt maybe?) or am I missing something about our policy here on core/popular crates?

fitzgen · 2022-12-14T22:23:21Z

or am I missing something about our policy here on core/popular crates?

Not missing anything AFAIK. Someone just has to bite the bullet and review the new deps and delta for upgraded deps if Firefox folks haven't already done so.

cfallin

One more nit below; but otherwise LGTM, modulo the cargo-vet TODO. One of us will try to get to this soon...

cfallin · 2022-12-15T01:18:24Z

cranelift/codegen/src/isa/aarch64/inst/mod.rs

@@ -124,7 +124,7 @@ fn count_zero_half_words(mut value: u64, num_half_words: u8) -> usize {
 fn inst_size_test() {
    // This test will help with unintentionally growing the size
    // of the Inst enum.
-    assert_eq!(32, std::mem::size_of::<Inst>());
+    assert_eq!(40, std::mem::size_of::<Inst>());


We probably don't want to increase the size of Inst if we can help it at all -- the point of this test is to guard against perf regressions. Can we find a way to remove a field or, failing that, box the contents of the inst? Two options come to mind:

(Maybe preferred) eliminate rtmp, and use x1 explicitly instead. This is part of the clobber-set of the call already, so it should be legal to use it to hold the address of the called function.

(Alternately) create a struct and hold a Box in the inst, taking only 8 bytes

Ah, I wasn't sure that it was okay to use x1, but your explanation makes sense (and helped clear up my confusion) - thanks!

Just pushed a commit to make that change (and reverted the change to the size test as well)

EdorianDark · 2023-01-29T11:46:02Z

I did an audit of object's upgrade; however cargo vet is showing an audit backlog of hashbrown, once_cell, and ahash, and I don't really feel qualified to audit those at all. Anyone else want to take a crack at it (@alexcrichton, @fitzgen, @jameysharp, @elliottt maybe?) or am I missing something about our policy here on core/popular crates?

Since there is no review process for an Rust update, all of std is considered trusted.
Hashbrown is part of the rust-lang organization and is used by std::HashMap, so it is trusted if it is part of std.
I think that trust should be extended if it is used directly and also to its dependencies like ahash and indirect dependencies like once_cell .

EDIT: The reviews were already done in #5550, which will probably land soon.

cfallin · 2023-02-09T00:06:48Z

@nathanwhit would you be willing to rebase this? I think we should be close to able to merge this with vets that have happened in the meantime...

nathanwhit · 2023-02-09T01:42:33Z

@nathanwhit would you be willing to rebase this? I think we should be close to able to merge this with vets that have happened in the meantime...

Done!

jameysharp · 2023-02-09T02:08:30Z

Ah, the merged cargo-vet review for the object crate is for 0.30.1, not 0.30. If you bump to that version, I believe the cargo-vet check will pass.

In addition, cargo-deny is failing because there are multiple versions of ahash, hashbrown, and object being pulled in by different crates. I think these have all been resolved in #5550, but that hasn't merged yet.

I'm sorry that this PR is taking a long time to merge. We're still working out our processes for supply-chain review. We're learning though!

EdorianDark · 2023-02-09T17:36:27Z

@cfallin I think the best way would be to merge #5513, since there are object is already updated to 0.30.1

EdorianDark · 2023-02-15T17:48:22Z

Now object has been updated in master. I think with an rebase the build should succeed.
Thanks for your work!

cbeuw · 2023-03-24T09:45:31Z

Rebased this locally onto main and all tests pass (except for a filetest needing bless due to #5780). Any updates on this?

- `Aarch64` instead of `AArch64` in comments - Remove unnecessary guard in tls_value lowering - Remove unnecessary regalloc metadata in emission

- Instead of passing in a temporary register to use when emitting the TLS code, just use `x1`, as it's already in the clobber set. This also keeps the size of `aarch64::inst::Inst` at 32 bytes. - Update filetest accordingly

nathanwhit · 2023-03-24T17:19:52Z

Ah sorry, completely forgot this hadn't been merged. Thanks for the reminder!

I've rebased on top of main so it should be good to go now

nathanwhit mentioned this pull request Dec 14, 2022

Cranelift: Support tls_value on aarch64 Mach-O #5433

Closed

cfallin reviewed Dec 14, 2022

View reviewed changes

github-actions bot added cranelift Issues related to the Cranelift code generator cranelift:area:aarch64 Issues related to AArch64 backend. cranelift:module labels Dec 14, 2022

cfallin approved these changes Dec 15, 2022

View reviewed changes

nathanwhit force-pushed the aarch64-macho-support-tls branch from 89279b4 to 013217b Compare December 15, 2022 02:20

afonso360 mentioned this pull request Jan 8, 2023

Cranelift: error "unimplemented relocation addend Relocation" on MacOS on aarch64 #4564

Open

bjorn3 mentioned this pull request Jan 8, 2023

Add macOS AArch64 support rust-lang/rustc_codegen_cranelift#1248

Closed

6 tasks

afonso360 mentioned this pull request Jan 8, 2023

aarch64: Support GOT Relative relocations in PIC mode #5550

Merged

This was referenced Jan 21, 2023

[ICE]: should be implemented in ISLE: inst = v0 = tls_value.i64 gv0, type = Some(types::I64) rust-lang/rustc_codegen_cranelift#1343

Closed

cranelift-object: Make sections read only by default #5619

Merged

nathanwhit force-pushed the aarch64-macho-support-tls branch from 013217b to 1d6a9b0 Compare February 9, 2023 01:42

nathanwhit added 5 commits March 24, 2023 10:12

Implement TLS on Aarch64 Mach-O

2cb2458

Add aarch64 macho TLS filetest

59f3e35

Address review comments

bebbb41

- `Aarch64` instead of `AArch64` in comments - Remove unnecessary guard in tls_value lowering - Remove unnecessary regalloc metadata in emission

Use x1 as temporary register in emission

62b2228

- Instead of passing in a temporary register to use when emitting the TLS code, just use `x1`, as it's already in the clobber set. This also keeps the size of `aarch64::inst::Inst` at 32 bytes. - Update filetest accordingly

Update aarch64 mach-o TLS filetest

e5aa875

nathanwhit force-pushed the aarch64-macho-support-tls branch from 1d6a9b0 to e5aa875 Compare March 24, 2023 17:19

cfallin added this pull request to the merge queue Mar 24, 2023

Merged via the queue into bytecodealliance:main with commit c3decdf Mar 24, 2023

nathanwhit deleted the aarch64-macho-support-tls branch March 24, 2023 21:23

cranelift: Implement TLS on aarch64 Mach-O (Apple Silicon) #5434

cranelift: Implement TLS on aarch64 Mach-O (Apple Silicon) #5434

Uh oh!

Conversation

nathanwhit commented Dec 14, 2022

Uh oh!

cfallin left a comment

Choose a reason for hiding this comment

Uh oh!

cfallin Dec 14, 2022

Choose a reason for hiding this comment

Uh oh!

cfallin Dec 14, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

cfallin commented Dec 14, 2022

Uh oh!

fitzgen commented Dec 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cfallin left a comment

Choose a reason for hiding this comment

Uh oh!

cfallin Dec 15, 2022

Choose a reason for hiding this comment

Uh oh!

nathanwhit Dec 15, 2022

Choose a reason for hiding this comment

Uh oh!

EdorianDark commented Jan 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cfallin commented Feb 9, 2023

Uh oh!

nathanwhit commented Feb 9, 2023

Uh oh!

jameysharp commented Feb 9, 2023

Uh oh!

EdorianDark commented Feb 9, 2023

Uh oh!

EdorianDark commented Feb 15, 2023

Uh oh!

cbeuw commented Mar 24, 2023

Uh oh!

nathanwhit commented Mar 24, 2023

Uh oh!

Uh oh!

fitzgen commented Dec 14, 2022 •

edited

Loading

EdorianDark commented Jan 29, 2023 •

edited

Loading