Safe intrinsics RFC #1248

Closed
wants to merge 1 commit into
from

Projects

None yet
@Aatch
Contributor
Aatch commented Aug 11, 2015

Allow intrinsics to be marked as safe, overriding the implicit unsafe from being in an extern block.

Rendered

@sfackler
Member

Should we instead make intrinsics safe by default and then add an unsafe for transumute etc?

@nikomatsakis nikomatsakis added the T-lang label Aug 11, 2015
@nikomatsakis
Contributor

I'm marking this as T-lang, since I feel like the approach to intrinsics is fundamentally a language decision, but obviously this impacts libs too.

cc @rust-lang/lang @rust-lang/libs

@nikomatsakis
Contributor

I feel like intrinsics and lang items serve very similar purposes and I am not convinced we should have both of them. It feels like we could remove all intrinsics and replace them with lang items, which can assume arbitrary signatures -- we could then have the compiler have particular semantics when it calls a lang item. Frequently, I would imagine, the body of those lang items would be irrelevant, as the compiler will supply it (perhaps just panic!("should be generated directly by trans")). We could then choose to stabilize lang items like any other thing that we stabilize.

@arielb1
Contributor
arielb1 commented Aug 11, 2015

@nikomatsakis

This would rather mess the plan for SIMD intrinsics.

Maybe allow "#[safe]" for FFI functions too? Why not just write a wrapper? Thinking about it, maybe an #[inline(rustc_mir)] would be a better solution for the expect problem.

@alexcrichton
Member

I agree that it's somewhat annoying that safe intrinsics have to be declare unsafe, but having to write a wrapper generally seems like it's just a bit of an annoyance. Two thoughts:

  • Are there numbers for the technical impact of just reexporting intrinsics vs writing wrappers for them? We've seen wins in the past, but not all intrinsics are that commonly called.
  • Could you elaborate on the problem with the likely/unlikely intrinsics? Not being able to implement an intrinsic is pretty strong motivation, but I'm unclear on the details here.
@arielb1
Contributor
arielb1 commented Aug 11, 2015

@alexcrichton

LLVM does not notice expect (used by likely/unlikely) unless they occur very close to the value's actual use - wrappers break this. An (at-first extremely limited) #[inline(rust_mir)] would solve this at the cost of some implementation work. Opportunistic MIR inlining (which me and @eddyb are thinking of doing) would also solve this if made to recognize unlikely.

@nikomatsakis
Contributor

@arielb1

This would rather mess the plan for SIMD intrinsics.

True, though I guess I've never been crazy about the ducktyping part of the plan anyway. That said, you can imagine handling SIMD intrinsics through some other attribute like #[simd_operation(add)] applied to a fn.

UPDATE: Edited for clarity.

@nikomatsakis
Contributor

That said, you can imagine handling SIMD intrinsics through some other attribute like #[simd_operation(add)] applied to a fn.

...or just allow a "simd" lang item to be defined any number of times.

@huonw
Member
huonw commented Aug 11, 2015

It seems a bit strange to me to have hundred/thousands of lines of code like

#[simd_operation(x86_mm_sad_epu8)]
fn x86_mm_sad_epu8(x: u8x16, y: u8x16) -> u8x16 { panic!() }
@Diggsey
Contributor
Diggsey commented Aug 12, 2015

What if you tie (default) unsafety to the calling convention rather than whether it is in an extern block, so that "rust-call" methods are safe by default? (memory safety could be a requirement of the "rust-call" calling convention)

@nrc nrc commented on the diff Aug 13, 2015
text/0000-safe-intrinsics.md
+* `needs_drop`
+* Math intrinsics
+* Bit manipulation intrinsics
+* `bswap16`, `bswap32`, `bswap64`
+* Checked arithmetic
+* `overflowing_add`, `overflowing_sub`, `overflowing_mul`
+
+# Drawbacks
+
+None.
+
+# Alternatives
+
+* Remove the restriction that the attribute is only valid on intrinsics. This runs counter to the
+ idea that functions we don't know the bodies of are presumed unsafe.
+* Hard-code the safety for each intrinsic. This ensures consistency since you can declare the
@nrc
nrc Aug 13, 2015 Contributor

IS this suggestion saying that we treat intrinsics as safe by default and then add unsafe keywords to the unsafe one? If not, is that a possibility? It seems nice in that we treat intrinsics like other functions (although not like FFI functions which are arguably more similar) and it doesn't need a new attribute.

@eddyb
eddyb Aug 13, 2015 Member

I believe it's saying "leave the declarations in libcore exactly as they are and compute the unsafety of the fn type based on the name".

@nikomatsakis
Contributor

@huonw

It seems a bit strange to me to have hundred/thousands of lines of code like

#[simd_operation(x86_mm_sad_epu8)]
fn x86_mm_sad_epu8(x: u8x16, y: u8x16) -> u8x16 { panic!() }

Good case for a macro? We probably want one anyway, since libraries that re-declare these intrinsics will need a similar array of such declarations.

Anyway, taking a step back, clearly I'm proposing something somewhat orthogonal to this RFC, except that:

  • this RFC marches intrinsics one step closer to being stabilized, so deciding if we like the general way we are handling intrinsics seems important.
  • lang items already can be public, exported, safe, unsafe, whatever, so this proposal encompasses this RFC and then some. A growing array of attributes like #[safe], used to tweak various aspects of intrinsics we want to change, doesn't appeal to me. Being able to just make any declaration, of any kind, special seems like it addresses that.
@eddyb
Member
eddyb commented Aug 13, 2015

@nikomatsakis If we stop using extern {} blocks for intrinsics, would we remove generics from them?
That made me think of the following possible strategy for C++ FFI. Somewhat offtopic, but I wanted to write it down:

extern "C++" {
    fn vector_push<T>(v: *mut stl::vector<T>, x: T) {
        v->push_back(x);
    }
}

I believe @mystor's crate is somewhat similar, although implemented via a plugin.

@mystor
mystor commented Aug 13, 2015

@eddyb, rust-cpp is similar in idea (body of code is C++, types declared in Rust & translated using simple-ish set of rules), however definitely doesn't support generics, or anything complicated, yet (I imagine that that would take more compiler support/hackery) ^.^ (sorry about off-topic)

@nikomatsakis nikomatsakis self-assigned this Aug 13, 2015
@nikomatsakis
Contributor

@rfcbot fcp close

I move that we close this RFC. Not because I disagree with the sentiment: I think we will want to eventually have both safe and stable intrinsics. However, I would like to change the way we achieve that: I think it'd be better to just merge the idea of "intrinsics" and "lang items" -- so that an intrinsic is just a function with a suitable attribute.

I think the usual form would be as follows (hat tip to @eddyb):

#[lang name="special"]
fn special(x, y, z) -> T {
    special(x, y, z)
}

Having the fn calls itself is basically a convenient hack. This way, the compiler can detect direct calls to special and give them particular semantics. Indirect calls, if we decide to support them, fall out naturally from just treating special as an ordinary function -- since it contains a direct call, that inner call will be translated specially. (If we wish to prohibit indirect calls for some particular intrinsic, we can do it by prohibiting that particular lang item from being reified into a fn ptr.)

@rfcbot
rfcbot commented Nov 11, 2016 edited

Team member @nikomatsakis has proposed to close this. The next step is review by the rest of the tagged teams:

No concerns currently listed.

Once these reviewers reach consensus, 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.

@aturon
Contributor
aturon commented Nov 12, 2016

@nikomatsakis I'm in favor of a merger between lang items and intrinsics. This is a question we'll want to be thinking about as part of the recent work on SIMD, of course.

@rfcbot
rfcbot commented Dec 1, 2016

🔔 This is now entering its final comment period, as per the review above. 🔔

@arielb1
Contributor
arielb1 commented Dec 2, 2016

Having the fn calls itself is basically a convenient hack. This way, the compiler can detect direct calls to special and give them particular semantics. Indirect calls, if we decide to support them, fall out naturally from just treating special as an ordinary function -- since it contains a direct call, that inner call will be translated specially. (If we wish to prohibit indirect calls for some particular intrinsic, we can do it by prohibiting that particular lang item from being reified into a fn ptr.)

That's basically how <i32 as Add<i32>>::add etc. work?

@solson
Member
solson commented Dec 11, 2016

@arielb1 Yeah, I think it's effectively the exact same idea.

@aturon aturon referenced this pull request in dikaiosune/rust-dashboard Dec 13, 2016
Open

rfcbot is not posting that FCP is complete #117

@aturon
Contributor
aturon commented Dec 13, 2016

The RFC bot seems to be stuck, but at this point the final comment period has elapsed. Nothing has changed since the earlier summary, so I'm going to go ahead and close the RFC. Let's revisit with a merger with lang items.

Thanks, @Aatch, for the RFC!

@aturon aturon closed this Dec 13, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment