Wasmtime GitHub notifications bot (Jun 23 2022 at 18:06): Wasmtime GitHub notifications bot (Jun 23 2022 at 18:06):alexcrichton opened PR #4306 from global-asm to main:
This commit moves the external assembly files of the
wasmtime-fiber
crate intoglobal_asm!blocks defined in Rust. The motivation for
doing this is not very strong at this time, but the points in favor of
this are:
One less tool needed to cross-compile Wasmtime. A linker is still
needed but perhaps one day that will improve as well.A "modern" assembler, built-in to LLVM, is used instead of whatever
appears on the system.The first point hasn't really cropped up that much and typically getting
an assembler is just as hard as getting a linker nowadays. The second
point though has us usinghint #xxin aarch64 assembly instead of the
actual instructions for assembler compatibility, and I believe that's no
longer necessary because the LLVM assembler supports the modern
instruction names.The translation of the x86/x86_64 assembly has been done to Intel
syntax as well as opposed to the old AT&T syntax since that's Rust's
default. Additionally s390x still remains in an external assembler file
becauseglobal_asm!is still unstable in Rust on that platform.<!--
Please ensure that the following steps are all taken care of before submitting
the PR.
[ ] This has been discussed in issue #..., or if not, please tell us why
here.[ ] A short description of what this does, why it is needed; if the
description becomes long, the matter should probably be discussed in an issue
first.[ ] This PR contains test cases, if meaningful.
- [ ] A reviewer from the core maintainer team has been assigned for this PR.
If you don't know who could review this, please indicate so. The list of
suggested reviewers on the right can help you.Please ensure all communication adheres to the code of conduct.
-->
Wasmtime GitHub notifications bot (Jun 23 2022 at 18:14):alexcrichton submitted PR review.
Wasmtime GitHub notifications bot (Jun 23 2022 at 18:14):alexcrichton created PR review comment:
@akirilov-arm I have temporarily dropped this block in the hopes that it's only required for external assembly files rather than
global_asm!blocks in Rust itself. I am not super familiar withglobal_asm!though so I could be wrong here. Could you help by taking a peek at the artifacts of this PR (when they're available) and see if everything looks correct?
Wasmtime GitHub notifications bot (Jun 23 2022 at 18:14):alexcrichton submitted PR review.
Wasmtime GitHub notifications bot (Jun 23 2022 at 18:16):alexcrichton updated PR #4306 from global-asm to main.
Wasmtime GitHub notifications bot (Jun 23 2022 at 18:41):akirilov-arm submitted PR review.
Wasmtime GitHub notifications bot (Jun 23 2022 at 18:41):akirilov-arm created PR review comment:
Given that since PR #3799 Cranelift no longer supports the 32-bit Arm architecture, do we need to keep this file?
Wasmtime GitHub notifications bot (Jun 23 2022 at 18:41):akirilov-arm submitted PR review.
Wasmtime GitHub notifications bot (Jun 23 2022 at 18:41):akirilov-arm created PR review comment:
For the sake of other people who might look at this PR - we are also discussing this issue in the Zulip chat.
Wasmtime GitHub notifications bot (Jun 23 2022 at 18:56):alexcrichton created PR review comment:
Ok while very roundabout I have confirmed that I don't believe it's necessary to include this section in the
global_asm!anywhere. Theglobal_asm!gets shoved directly into the final object file and the object files coming out of rustc otherwise have this section already configured with the-Zbranch-protection=btiflag.As I mentioned in Zulip there's a whole mess of other reason why the note won't show up but they're all unrelated to this stanza in this assembly (the other issues being that (a) your local rust precompiled standard library needs the note and (b) your local glibc toolchain with
crt*.oalso needs the note, neither of which have it by default)
Wasmtime GitHub notifications bot (Jun 23 2022 at 18:56):alexcrichton submitted PR review.
Wasmtime GitHub notifications bot (Jun 23 2022 at 19:16):alexcrichton updated PR #4306 from global-asm to main.
Wasmtime GitHub notifications bot (Jun 23 2022 at 21:02):alexcrichton updated PR #4306 from global-asm to main.
Wasmtime GitHub notifications bot (Jun 23 2022 at 21:02):alexcrichton updated PR #4306 from global-asm to main.
Wasmtime GitHub notifications bot (Jun 23 2022 at 21:11):alexcrichton created PR review comment:
It's true yeah, although knowing that I originally wrote this file and the x86.S file when I first made the
wasmtime-fibercrate. My thinking was that I had all the context of fibers booted into my head and the CFI directives and such in particular are really tricky so it was easy for me to bang out some other "maybe one day we'll support these" architectures here. These aren't actually tested on CI at all and there's no actual way to use them unless you use thewasmtime-fibercrate directly (and why would you).Personally I'd err on the side of leaving them here in the sense that I've tested them to make sure they work (even with this PR) and otherwise it's one less thing to add if anyone ever wants to add support for arm/x86. If anyone else spends even a crumb of thought on updating these files though they should definitely be deleted immediately as they're not worth that much.
Wasmtime GitHub notifications bot (Jun 23 2022 at 21:11):alexcrichton submitted PR review.
Wasmtime GitHub notifications bot (Jun 24 2022 at 12:53):akirilov-arm submitted PR review.
Wasmtime GitHub notifications bot (Jun 24 2022 at 12:53):akirilov-arm submitted PR review.
Wasmtime GitHub notifications bot (Jun 24 2022 at 12:53):akirilov-arm created PR review comment:
Nit - there is a space between
backward-andcompatible; it just looks weird.
Wasmtime GitHub notifications bot (Jun 24 2022 at 12:53):akirilov-arm created PR review comment:
How about using
.p2align(or.balign) instead, since apparently the semantics of.aligndiffer between platforms?
Wasmtime GitHub notifications bot (Jun 24 2022 at 12:53):akirilov-arm created PR review comment:
Nit - do you mind moving this after the next directive (about x30), since it refers to the same register? It would look a bit nicer IMHO.
Wasmtime GitHub notifications bot (Jun 24 2022 at 14:21):alexcrichton updated PR #4306 from global-asm to main.
Wasmtime GitHub notifications bot (Jun 24 2022 at 14:21):alexcrichton submitted PR review.
Wasmtime GitHub notifications bot (Jun 24 2022 at 14:21):alexcrichton created PR review comment:
Oh dear thanks for pointing this out!
Wasmtime GitHub notifications bot (Jun 24 2022 at 21:49):akirilov-arm submitted PR review.
Wasmtime GitHub notifications bot (Jun 27 2022 at 13:54):alexcrichton requested fitzgen for a review on PR #4306.
Wasmtime GitHub notifications bot (Jun 27 2022 at 18:10):fitzgen submitted PR review.
Wasmtime GitHub notifications bot (Jun 27 2022 at 18:20):alexcrichton merged PR #4306.
Last updated: Nov 22 2024 at 16:03 UTC