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

adjust for current reality wrt. wide raw pointers #162

Merged
merged 10 commits into from
Sep 12, 2019

Conversation

RalfJung
Copy link
Member

src/what-unsafe-does.md Outdated Show resolved Hide resolved
Co-Authored-By: Mazdak Farrokhzad <twingoow@gmail.com>
@RalfJung
Copy link
Member Author

Turns out that for raw slices, the length condition can be violated in safe code.

So maybe I should just remove the slice condition here, effectively making it part of the safety invariant but not the validity invariant? That would preclude layout optimizations but otherwise not make much of a difference.

@RalfJung
Copy link
Member Author

I fixed the slice situation.

@Gankra
Copy link
Contributor

Gankra commented Aug 27, 2019

I am a bit confused: is it really UB to just call a function that you think is no-unwind, but actually can unwind? Like, you don't need to actually unwind for it to be UB?

@RalfJung
Copy link
Member Author

@gnzlbg see above. I used your wording, there are still open questions.

@gnzlbg
Copy link
Contributor

gnzlbg commented Aug 27, 2019

I am a bit confused: is it really UB to just call a function that you think is no-unwind, but actually can unwind?

I think this works in practice for most unwinding ABIs, I don't know if it works for all. Does any unwinding ABI require caller cooperation even when the callee does not unwind? (cc @comex ?).

Like, you don't need to actually unwind for it to be UB?

Due to something like rust-lang/unsafe-code-guidelines#198 one doesn't even need to call it for it to be UB. For example, see https://rust.godbolt.org/z/HQ0fvA

#![feature(unwind_attributes)]
pub mod bad0 {
    extern "C" { fn foo(); }
    pub unsafe fn call() -> usize { foo as usize }
}

pub mod bad1 {
    extern "C" { #[unwind(allow)]  fn foo(); }
    pub unsafe fn call() { foo() } // UB because `foo` is nounwind because bad0::foo is nounwind
}

We can probably fix that by never making extern declarations nounwind, but.. a declaration with the incorrect attribute in a different translation unit (e.g. in C++) could still break things via xLTO.

src/what-unsafe-does.md Outdated Show resolved Hide resolved
Co-Authored-By: gnzlbg <gnzlbg@users.noreply.github.com>
@comex
Copy link

comex commented Aug 27, 2019

Does any unwinding ABI require caller cooperation even when the callee does not unwind? (cc @comex ?).

I don't know of any that do. Even the WebAssembly and JSBackend exception ABIs, which operate at a higher level than native ones, don't change the signature of the function itself. Of course, who knows what will be invented in the future. I remember someone (maybe even you?) proposing that Rust could invent its own portable unwinding mechanism based on effectively wrapping all function return values in Result; that would probably require caller cooperation.

@Gankra
Copy link
Contributor

Gankra commented Aug 27, 2019

since those would be new calling conventions, we can add define that UB if they ever come into existence, right?

@RalfJung
Copy link
Member Author

We would likely want to disallow extern { ... } declarations without explicit unwind for such ABIs -- just like we disallow extern { ... } declarations without argument types.

The current extern { ... } syntax is really only suited for ABIs where there is a "default" for the unwinding ABI that always works, assuming unwinding doesn't happen.

@gnzlbg
Copy link
Contributor

gnzlbg commented Aug 28, 2019

since those would be new calling conventions, we can add define that UB if they ever come into existence, right?

Maybe? The problem is that extern "C" and extern "system" are stable and follow the platform calling convention, so if we were to support a platform where that happened, we'd need to do that for these ABIs there. We could just say that we'd never support those platforms and call it a day (EDIT: or do something different for those ABIs there). As @comex says, it's unlikely to happen. EDIT: It would be surprising if an ABI is doing this yet.

Still, for the wording here, does changing "call ABI or unwinding ABI" to "call ABI" or even just "ABI" make a difference? We haven't written down anywhere whether the unwinding ABI is part of the call ABI or not, and it is arguably part of the "ABI" in general...


@comex

I remember someone (maybe even you?) proposing that Rust could invent its own portable unwinding mechanism based on effectively wrapping all function return values in Result; that would probably require caller cooperation.

Are you referring to: https://github.com/CraneStation/cranelift/issues/553 ?

@RalfJung
Copy link
Member Author

Still, for the wording here, does changing "call ABI or unwinding ABI" to "call ABI" or even just "ABI" make a difference? We haven't written down anywhere whether the unwinding ABI is part of the call ABI or not, and it is arguably part of the "ABI" in general...

In terms of wording I would propose something like

Calling a function with the wrong call ABI or unwinding from a function with the wrong unwind ABI.

@RalfJung
Copy link
Member Author

RalfJung commented Sep 5, 2019

FWIW, the corresponding Reference PR landed (which is in sync with this one).

@Gankra
Copy link
Contributor

Gankra commented Sep 5, 2019

PSA i am dealing with medecine issues atm and am completely out of my mind and in too much pain to think for the forseeable

@gnzlbg
Copy link
Contributor

gnzlbg commented Sep 6, 2019

@Gankra I hope you get well soon, take care!

@RalfJung
Copy link
Member Author

RalfJung commented Sep 9, 2019

@Gankra all the best!


So how to we handle the nomicon in the mean time? Are there other editors? @Centril do you know? Are you fine landing this PR, to get Nomicon and reference in sync (now that we merged rust-lang/reference#659)?

@Centril
Copy link
Contributor

Centril commented Sep 9, 2019

I don't have r+ in this repo myself but I think the reference editors + UCG + T-lang should have r+ here.

That said I think I'd need to actually review the PR first myself. ;)

src/what-unsafe-does.md Outdated Show resolved Hide resolved
Co-Authored-By: Mazdak Farrokhzad <twingoow@gmail.com>
@RalfJung
Copy link
Member Author

I don't have r+ in this repo myself but I think the reference editors + UCG + T-lang should have r+ here.

Reference editors + T-lang sounds reasonable; UCG doesn't really have a formal notion of "membership" but I guess we could suggest some people to T-lang and see if T-lang is happy giving them r+ for the Nomicon.

Which team is the one to make such decisions?

@gnzlbg
Copy link
Contributor

gnzlbg commented Sep 11, 2019

@pietroalbini :

I don't have r+ in this repo myself but I think the reference editors + UCG + T-lang should have r+ here.

Could you give invite T-lang as maintainers of this repo, or at least @Centril ?

@pietroalbini
Copy link
Member

I can make the changes to permissions, yes, but I shouldn't be the one deciding those.

@gnzlbg
Copy link
Contributor

gnzlbg commented Sep 11, 2019

@Centril can T-lang decide whether they can give themselves permissions for this repo or shall that be decided by the core team? 😆

@Centril
Copy link
Contributor

Centril commented Sep 11, 2019

@pietroalbini As a start, can you grant the lang-team members, ehuss, matthewjasper, and RalfJung r+?

@pietroalbini
Copy link
Member

@Centril have we considered the fact the lang team doesn't exist on the nursery? 🙃

@Centril
Copy link
Contributor

Centril commented Sep 12, 2019

@pietroalibini Yes, give the permissions to individuals for now instead.

@pietroalbini
Copy link
Member

Invited some people.

@gnzlbg
Copy link
Contributor

gnzlbg commented Sep 12, 2019

Maybe the nomicon and the reference shouldn't live on the nursery, but on rust-lang/rust proper?

@Centril
Copy link
Contributor

Centril commented Sep 12, 2019

@pietroalbini seems like a good idea to move both the nomicon and the reference.

@Centril Centril merged commit 7b3c50b into rust-lang:master Sep 12, 2019
@RalfJung RalfJung deleted the raw-wide branch September 15, 2019 12:04
@Gankra
Copy link
Contributor

Gankra commented Oct 4, 2019

Just want to throw my hat out there again that I think the nomicon should be in the rust-lang/rust repo so that unstable code examples can be atomically adjusted along with the APIs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants