Reformat inline-assembly chapter as a spec chapter. #1523
Conversation
Co-authored-by: Eric Huss <[email protected]>
|
@Amanieu: We on the spec team would appreciate if you could have a look at this PR. The context here is that we're reformatting and reworking Reference chapters to make them more "spec like". Principally, that means that we're adding an identifier to each block. Since each identifier has a name, that means reworking things a bit such that "one idea" falls within each such block. We 1) want to be sure you know this is going on, and 2) are happy with how it turned out, especially as there was some reworking, rewording, and the addition of some new content. We're using this chapter as a model for reworking other ones (so please bear with us a bit as this is the first). |
This comment was marked as outdated.
This comment was marked as outdated.
|
Let's use rfcbot here. We should FCP this since it's the model for other chapters. I'm going to propose FCP merge, even though I still need to review carefully, because I certainly want this in some form. And I'll check Joel's box as he's checked off above. (I'll file a concern to hold space for that review.) (We'll see if rfcbot works with T-spec here. It's the first go of this, at least in this repo.) @rfcbot fcp merge |
|
Team member @traviscross has proposed to merge this. The next step is review by the rest of the tagged team members: Concerns:
Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up! See this document for info about what commands tagged team members can give me. |
|
@rfcbot concern tc-wants-to-review-carefully |
src/inline-assembly.md
Outdated
|
|
||
| Support for inline assembly is stable on the following architectures: | ||
| r[asm.safety] | ||
| The macro [`core::arch::asm!`] shall be expanded only within an `unsafe` block. |
There was a problem hiding this comment.
This makes it sound a bit like it doesn't expand outside an unsafe block, rather than usage outside unsafe {} resulting in an error.
There was a problem hiding this comment.
Does it? Shall is a static constraint on the program, so program is ill-formed if violated.
There was a problem hiding this comment.
As a casual reader, I agree that it could stand to be rephrased to mention a compiler error, or state some similarity to calling an unsafe fn.
Though the below compile_fail test, which you've asked to have stylistically called out as "this will fail to compile", might serve to alleviate the concern a little bit here. But it would be nice if this sentence could avoid being misleading in the first place.
|
|
||
| r[asm.directives.arm] | ||
|
|
||
| > The following directives are guaranteed to be supported on 32-bit ARM platforms |
There was a problem hiding this comment.
Isn't this [!TARGET-SPECIFIC] too?
|
Discussion re. [asm.invocation.prefix-instr] and [asm.invocation.global-section] https://rust-lang.zulipchat.com/#narrow/stream/216763-project-inline-asm/topic/.60.2Esection.60.20directives.2E |
src/inline-assembly.md
Outdated
| # #[cfg(not(target_arch = "x86_64"))] compile_error!("Inline Assembly Tests are not supported off of x86_64"); | ||
| ``` | ||
|
|
||
| ## Template modifiers r |
There was a problem hiding this comment.
| ## Template modifiers r | |
| ## Template modifiers |
src/inline-assembly.md
Outdated
| r[asm.invocation] | ||
|
|
||
| r[asm.invocation.asm] | ||
| The [`core::arch::asm!`] macro shall be expanded in an expression context only. The input tokens shall match the `asm_inner` production. The expansion is [`unsafe`][static.expr.safety] and has type `()`, unless the option `noreturn` is specified, in which case it has type `!`. |
There was a problem hiding this comment.
This link to unsafe is broken since there aren't any rules for that, yet. Can you make sure to link directly until those sections are ready? In this case,
[`unsafe`]: unsafety.md
src/inline-assembly.md
Outdated
| | RISC-V | `"C"`, `"system"`, `"efiapi"` | `x1`, `x[5-7]`, `x[10-17]`, `x[28-31]`, `f[0-7]`, `f[10-17]`, `f[28-31]`, `v[0-31]` | | ||
| | LoongArch | `"C"`, `"system"`, `"efiapi"` | `$r1`, `$r[4-20]`, `$f[0-23]` | | ||
| > [!TARGET-SPECIFIC] | ||
| > The correspondance between the operation performed by the asm block is target-dependant and implementation-dependant, subject to the rules set in [asm.operands]. Unless the program modifies the execution state, the basic operation performed by the asm block is the one performed by executing the sequence of instructions specified in the *expanded asm-string* starting with the first instruction. |
There was a problem hiding this comment.
| > The correspondance between the operation performed by the asm block is target-dependant and implementation-dependant, subject to the rules set in [asm.operands]. Unless the program modifies the execution state, the basic operation performed by the asm block is the one performed by executing the sequence of instructions specified in the *expanded asm-string* starting with the first instruction. | |
| > The correspondence between the operation performed by the asm block is target-dependant and implementation-dependant, subject to the rules set in [asm.operands]. Unless the program modifies the execution state, the basic operation performed by the asm block is the one performed by executing the sequence of instructions specified in the *expanded asm-string* starting with the first instruction. |
src/inline-assembly.md
Outdated
| > The target may define that the register value (or some portion thereof) is undefined. | ||
|
|
||
| r[asm.evaluation.constraints] | ||
| Certain constraints may be placed on the asm block, and on the requirements of the correspondance, by default or by an option explicitly specified on the asm block. The behaviour is undefined if any such constraint is violated. |
There was a problem hiding this comment.
| Certain constraints may be placed on the asm block, and on the requirements of the correspondance, by default or by an option explicitly specified on the asm block. The behaviour is undefined if any such constraint is violated. | |
| Certain constraints may be placed on the asm block, and on the requirements of the correspondence, by default or by an option explicitly specified on the asm block. The behaviour is undefined if any such constraint is violated. |
Also, in general, I don't understand what "requirements of the correspondance" means.
Also, I'm in general not understanding this rule. What constraints is it referring to? Can you point to the original chapter that this is referring to?
There was a problem hiding this comment.
Can you point to the original chapter that this is referring to?
This is the formalization of the "Rule of Inline Assembly" section's preamble. The constraints mentioned there are then elaborated in the rest of the section (for default constraints) and in [asm.options] (for option specific constraints).
The correspondence is the correspondence between the abstract machine state and the physical machine state affected by the inline assembly - defined in the previous clause and target-specific admonition, and constrained in various sections (such as in [asm.operands] and [asm.registers]). IE. when you do X in assembly, what effect does that have on the Rust/Minirust AM, and how do we overall map both incoming state and outgoing state. The constraints then limit the results of the correspondence. For example, if the correspondence says that the operation wrote to some allocation, but the nomem option was set, you've violated that constraint on the asm block. Another example is the fact that if a register isn't mentioned by an out, lateout, inout, or inlateout operand, it must be preserved by the asm block. An asm block that then clobbers a register that is required to be preserved violates that constraint.
src/inline-assembly.md
Outdated
|
|
||
| `clobber_abi` may be specified any number of times. It will insert a clobber for all unique registers in the union of all specified calling conventions. | ||
| r[asm.evaluation.general] | ||
| Each evaluation of an asm block (invocation of [`core::arch::asm!`]) shall perform an operation that correpsonds to the result of a valid sequence of operations on the Minirust Abstract Machine. The behaviour is undefined if the operations performed by the asm block do not validly correspond to a valid sequence of Minirust operations. |
There was a problem hiding this comment.
| Each evaluation of an asm block (invocation of [`core::arch::asm!`]) shall perform an operation that correpsonds to the result of a valid sequence of operations on the Minirust Abstract Machine. The behaviour is undefined if the operations performed by the asm block do not validly correspond to a valid sequence of Minirust operations. | |
| Each evaluation of an asm block (invocation of [`core::arch::asm!`]) shall perform an operation that corresponds to the result of a valid sequence of operations on the Minirust Abstract Machine. The behavior is undefined if the operations performed by the asm block do not validly correspond to a valid sequence of Minirust operations. |
|
I'm quickly adding "Terms used in the Specification" to the glossary in order to resolve some of the above wording concerns. We'll need the terms defined anyways, and we might as well have them sooner rather than later. |
Co-authored-by: Eric Huss <[email protected]>
src/inline-assembly.md
Outdated
|
|
||
| `clobber_abi` may be specified any number of times. It will insert a clobber for all unique registers in the union of all specified calling conventions. | ||
| r[asm.evaluation.general] | ||
| Each evaluation of an *asm block* shall perform an operation that correpsonds to the result of a valid sequence of operations on the Minirust Abstract Machine. The behaviour is undefined if the operations performed by the asm block do not validly correspond to a valid sequence of Minirust operations. |
There was a problem hiding this comment.
This seems like a new claim that we will be defining things in terms of Minirust. Is that something we have decided to do?
There was a problem hiding this comment.
So there are two separate "new claims" here - one is that the operational semantics will be defined in terms of Minirust in general (that had a whole meeting on it, where we took an hour and a half to say "Yes"... actually, no, we took 5 minutes to say "Yes" and then spent the other hour 25 minutes debating "to what extent"), and the other is that inline assembly must affect the Rust AM in a way that Rust Code could possibly have done.
I believe the latter has been the opinion of the opsem team since before it was the opsem team, and is more generally, makes the claim that compilers can optimize Rust in the face of unanalyzed code (which has been assumed as true since we started plugging Rust into llvm).
There was a problem hiding this comment.
In practice, this means that this code has UB, because it doesn't correspond back to a valid sequence of Minirust operations (that do not themselves have undefined behaviour):
let x: i32 = 0;
asm!("mov dword ptr [{}], 5", in(reg) &x);
println!("{x}"); // May be turned into `println!("0");`The code writes to a Frozen tag, which is UB. But if inline assembly isn't constrained to simply doing things (to the AM) Rust code itself can do, then it could circumvent that rule.
There was a problem hiding this comment.
that had a whole meeting on it, where we took an hour and a half to say "Yes"... actually, no, we took 5 minutes to say "Yes" and then spent the other hour 25 minutes debating "to what extent")
I don't recall having a team decision on this point, and I don't see any discussions about that decision in the meeting notes.
I'm not opposed to Minirust as an eventual basis to use for defining things, but I'm not yet convinced that we are in a position where we are ready to do that today.
I'd also say that adding things like this are pretty heavy decisions that are probably best not to bundle in one large update. This makes it harder to review and make progress when there are many new changes included in one PR. Is it possible to work in smaller batches, and to keep new additions separate from things that are more editorially oriented?
There was a problem hiding this comment.
I think we need to be able to say what an inline assembly block does, and I'm not sure there's any other model we can use. We know that it can do more than Rust code can, but less than pure assembly can in a pure assembly program. For example, an asm block can yield a nondeterministic value, directly accessing the pick Minirust operation, but we also know that an asm block can't modify a byte in a rust allocation without disabling any tags that are frozen for that byte (and tripping protectors on those tags).
There was a problem hiding this comment.
@ehuss is right here. Maybe we do, as you say, ...
need to be able to say what an inline assembly block does
...in more detail than what the Reference chapter says today. But that doesn't need to all happen in this PR. And trying to do that is making it difficult to review this and make progress here.
There was a problem hiding this comment.
The main thing is that the later definitions don't really make sense unless we say what the correspondence is.
There was a problem hiding this comment.
FWIW this paragraph is not specific to inline assembly, it generally applies to any kind of FFI call.
There was a problem hiding this comment.
Clarifying my statement at the T-spec meeting:
- I think we need a claim here, for the rest of the section and the following section ([asm.options]) to make sense.
- I don't think it necessarily needs to make a claim about Minirust.
- We know that the claim should allow something more than what Rust itself allows, but less than what the hardware allows.
An example of something that Rust wouldn't allow, but Minirust and thus inline assembly does:
let x: u64;
asm!("", out("eax") x, options(nomem, nostack));
println!("{x}");Which is equivalent to the Minirust code
let x: u64;
x = pick::<u64>();
/*println!("{x}") expansion*/The go forward plan adopted by T-spec on 2024-06-13 includes, as one step, to reformat all of the chapters of the Reference to attach named identifiers to each claim, more or less. The resulting text will use the `mdbook-spec` extension for rendering (see PR #1542). Adding these named identifiers more granularly throughout the document is one step in allowing the Reference to be used as a specification for Rust in safety-critical applications. Per our plan, we want to reformat one chapter first, to ensure our happiness with that and to perfect our process, and to then reformat (and review and merge those reformattings of) all of the rest of the Reference chapters in the same way. We discussed and imagined that this reformatting would be somewhat mechanical, and that it could be done by a technical writer with limited experience with Rust. This is what gave us confidence that this work could be hired out, completed, and reviewed on the months-scale timeline that we had set out. As things turned out, we were fortunate enough to hire someone with significant experience with Rust. This resulted in a PR that amounts to a rewrite of the inline assembly chapter (see PR #1523). Whatever the virtues of that, it's not what we were expecting, and the degree of change in one PR has complicated and stalled effective review. This puts in jeopardy completion of the reformatting on the timeline we had expected. Consequently, this PR takes a different approach. Here, we perform *only* reformatting. That is, we add identifiers to each claim, more or less, *and nothing else*. We change *none* of the verbiage in this chapter. Contra to earlier claims that this is not possible to do, our finding, having done this, is that it works out fine. The original text was already organized reasonably well enough to just leave it in place and add the identifiers. Is this perfect? Almost certainly not. There's always more that could be done to improve a chapter. E.g., perhaps some claims could be broken down further still and more identifiers added, though there is a distinct readability tradeoff here. We've tried to strike a reasonable balance in this PR. The point of the exercise contained in this PR is that this diff is straightforward to review and moves us in the direction that we want to go. We can always make other changes later, and by separating them out, those changes will be easier to review also. Note that we're unhappy with the current rendering when two identifiers need to be stacked, e.g.: ``` [asm] [asm.intro] ``` We'll plan to improve this later and separately with work in `mdbook-spec` or in the style sheets.
|
@rfcbot concern decide-on-issue-1550-first Let's decide, first, on the FCP in: |
This rewrites the inline-assembly chapter to use the agreed upon style and mdbook-spec extensions. It also more formally defines certain portions and adds several more examples.