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

Spec abi chapter #1545

Open
wants to merge 17 commits into
base: master
Choose a base branch
from
Open

Conversation

chorman0773
Copy link
Contributor

This rewrites the abi chapter, and adds call compatibility to the chapter.

@chorman0773 chorman0773 added A-abi Area: ABI S-waiting-on-review Status: The marked PR is awaiting review from the PR author. T-spec Team: spec and removed T-spec Team: spec labels Jul 24, 2024
src/abi.md Show resolved Hide resolved
src/abi.md Outdated
* There exists a type `V`, such that `T` is *abi compatible* with `V` an `V` is *abi compatuble* with `U`,

> [!NOTE]
> These properties ensure that *abi compatibility* is an equivalence relation.
Copy link
Member

@pnkfelix pnkfelix Jul 25, 2024

Choose a reason for hiding this comment

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

Consider adding the terms "reflexivity", "symmetry", and "transitivity" to the three bullets above, especially since you use at least the term "transitivity' in the text that follows.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Probably a good idea. I wouldn't want to put it on the bullets though, maybe add it to the note.

These properties are respectively called "reflexivity", "symmetry", and "transitivity". They ensure that abi compatibility is an equivalence relation.

src/abi.md Outdated Show resolved Hide resolved
src/abi.md Outdated

fn main(){
let f: unsafe fn(*mut ()) = unsafe{core::mem::transmute(foo as unsafe fn(_))}; // Type Erase the function
let mut val = 0;
Copy link
Member

Choose a reason for hiding this comment

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

this example is relying on i32 being the fallback integer type when there is no other alternative being imposed, right?

(E.g., if one had written 5_i8 down below, then that ends up affecting the type inferred for val, and yields UB overall since now the write will be out-of-bounds, at least according to miri.)

I am wondering whether it would be better, for purposes of this example, to explicitly assign the i32 type via let val: i32 = 0; and then you avoid discussion of how integer type fallback is handled in this part of the spec.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Probably, yeah. I guess my brain was just on autopilot writing that test.

src/abi.md Outdated Show resolved Hide resolved
src/abi.md Outdated


r[abi.compatibility.fn-ptr]
An [`fn`-ptr type] `T` is compatible with an [`fn`-ptr type] `U` if `T` and `U` have *abi compatible* tags.
Copy link
Member

@pnkfelix pnkfelix Jul 25, 2024

Choose a reason for hiding this comment

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

is this supposed to say *abi compatible* or just compatible ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That one is supposed to say abi compatible, yes.

@chorman0773
Copy link
Contributor Author

We need to make mdbook-spec linkify Result<T,S>

Copy link
Contributor

@ehuss ehuss left a comment

Choose a reason for hiding this comment

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

Thanks @chorman0773! I really appreciate this!

src/abi.md Outdated Show resolved Hide resolved
src/abi.md Outdated Show resolved Hide resolved
src/abi.md Outdated Show resolved Hide resolved
src/abi.md Outdated Show resolved Hide resolved
src/abi.md Outdated Show resolved Hide resolved
src/abi.md Outdated
```

r[abi.symbol-name.names]
The *`no_mangle` attribute* and the *`export_name` attribute* shall only be applied to a `static` or `fn` item. The *`export_name` attribute* shall not be applied to an item declared within an [`extern` block].
Copy link
Contributor

Choose a reason for hiding this comment

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

Can't no_mangle be applied to all other items?

src/abi.md Outdated Show resolved Hide resolved
src/abi.md Outdated
@@ -79,22 +344,46 @@ The *`link_section` attribute* specifies the section of the object file that a
pub static VAR1: u32 = 1;
```

## The `export_name` attribute
r[abi.link_section.def]
An item with the *`link_section` attribute* is placed in the specified section when linking. The section specified shall not violate the constraints on section names on the target, and shall not be invalid for the item type, no diagnostic is required.
Copy link
Contributor

Choose a reason for hiding this comment

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

Similar to our conversation before about ndr, I think it would be best to avoid this for now until we have a decision on it.

src/abi.md Outdated
exported on a [function] or [static]. It uses the [_MetaNameValueStr_] syntax
to specify the symbol name.
> [!NOTE]
> A section name may be invalid if it violates the requirements for the item type, for example, an `fn` item must be placed in an executable section, and a mutable static item (`static mut` or one containing an `UnsafeCell`) must be placed in a writable section.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
> A section name may be invalid if it violates the requirements for the item type, for example, an `fn` item must be placed in an executable section, and a mutable static item (`static mut` or one containing an `UnsafeCell`) must be placed in a writable section.
> A section name may be invalid if it violates the requirements for the item type. For example, an `fn` item must be placed in an executable section, and a mutable static item (`static mut` or one containing an `UnsafeCell`) must be placed in a writable section.

linking external libraries.
## ABI Compatibility

r[abi.compatibility]
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add a few sentences explaining what abi compatibility is? Such as the consequence of what it means for types to be abi compatible? In general, I felt a little uncertain what I was reading, since it immediately jumps into a list of rules without giving context or explaining what this section is about.

@ehuss
Copy link
Contributor

ehuss commented Jul 26, 2024

We need to make mdbook-spec linkify Result<T,S>

For now, you'll need to use a fully qualified path like [`Result<T, S>`](std::result::Result). It's not really possible to support prelude items, since it needs some kind of token (like std::) to know if it is a standard library link.

@chorman0773
Copy link
Contributor Author

That's current what I'm using - the issue is on the <T,S>. It seems to handle 1 generic parameter fine, but not 2.

@ehuss
Copy link
Contributor

ehuss commented Jul 26, 2024

Ah, I see! Posted #1549 with a fix.

@ehuss
Copy link
Contributor

ehuss commented Jul 30, 2024

A concern that I think we should consider is that this seems to duplicate content from the core documentation. I think this is an important question about how we want to handle that. There are a few options:

  • Move everything to the reference, and remove from the core docs and link to the reference.
  • Keep everything in the core docs, and don't add it to the reference.
  • Duplicate in both places.

I think if it is duplicated, it will get out of sync, which I think will contribute to confusion, and cause more work.

I lean towards moving it to the reference, but there are some considerations of it being very useful to be in the core docs.

@traviscross
Copy link
Contributor

cc @RalfJung

@workingjubilee
Copy link
Member

I lean towards moving it to the reference, but there are some considerations of it being very useful to be in the core docs.

I think that where it is relevant, some things should be documented in the standard library even at the cost of duplication and even at the cost of desync.

Tersely, and then immediately (ahem) reference the Reference.

@workingjubilee
Copy link
Member

This probably could afford splitting the attributes and argument/return type equivalence into different files?

@ehuss
Copy link
Contributor

ehuss commented Sep 21, 2024

cc rust-lang/rust#130653

@traviscross
Copy link
Contributor

cc @rust-lang/opsem

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-abi Area: ABI S-waiting-on-review Status: The marked PR is awaiting review from the PR author.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants