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

translate undefined intrinsics to an unreachable and a lint #1478

Closed
wants to merge 1 commit into from

Conversation

arielb1
Copy link
Contributor

@arielb1 arielb1 commented Jan 25, 2016

rendered

includes #1477 up to has_native_atomic_ops

@comex
Copy link

comex commented Jan 25, 2016

Why not trap instead?

@nrc nrc added T-lang Relevant to the language team, which will review and decide on the RFC. T-compiler Relevant to the compiler team, which will review and decide on the RFC. labels Jan 25, 2016
@arielb1
Copy link
Contributor Author

arielb1 commented Jan 25, 2016

@comex

Not that sure of it. Maybe a trap would be better. Both should have roughly the same effect.

@Ericson2314
Copy link
Contributor

The run-time aspect of this kinda scares me, and I remain hopeful for type-level naturals someday like Haskell :). How about we have some system to try compiling one set of items, and if a compilation error is encountered, compile this other set of items instead? Kinda like "Substitution Failure Is Not An Error" meets http://alexcrichton.com/cfg-if .

@strega-nil
Copy link

I don't like this. We should try to stop undefined behavior in all cases, not just in safe code, and whenever we find something that definitely would be undefined behavior we should error out. A much better idea, imho, would be to make the intrinsics' errors better.

@nikomatsakis nikomatsakis self-assigned this Jan 28, 2016
@nikomatsakis
Copy link
Contributor

Hear ye, hear ye! This RFC is now entering final comment period.

@nikomatsakis nikomatsakis added the final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. label May 6, 2016
@Ericson2314
Copy link
Contributor

Hmm wasn't something for rustc to calculate layouts just landed? That + specialization would seem to be the way to go.

@ticki
Copy link
Contributor

ticki commented May 7, 2016

I'd prefer INT3 as well.

@rkjnsn
Copy link
Contributor

rkjnsn commented May 10, 2016

It seems like it would be best to provide a way to select different code paths based on, e.g., atomic availability at compile time. If that's not practical I would prefer if the default were still to generate an error during compilation, with a way to assert that your code is doing the appropriate run-time checks and opt-in to generating unreachable.

@nikomatsakis
Copy link
Contributor

Hmm. I am trying to remember the details of a conversation that @arielb1 and I had recently on the topic of this RFC, but I remember that I was feeling generally sympathetic to its aims -- however, I do prefer generating a trap to generating undefined behavior.

I guess the most urgent questions to resolve are:

  • when intrinsics can have edge cases, should we try to rule those out statically, or use the "trap+lint" approach advocated here?
    • examples:
      • transmute
      • atomics
      • etc
  • it seems plausible that there might be edge cases where the unsafe code knows that the trap path will not be reached at runtime (but can't prevent it from being generated);
  • currently the transmute restriction rules out a lot of generic code patterns; this could be fixed with some magic traits, but it would certainly be easier to just generate a trap
    • at the time when we removed the transmute code, we didn't track spans through inlining, so when you did have a failure, you had no good way to know which transmute it was that died

@Amanieu
Copy link
Member

Amanieu commented May 12, 2016

I've been using a workaround for transmutes in generic code for atomic-rs: https://github.com/Amanieu/atomic-rs/blob/master/src/nightly.rs#L39

The basic idea is to just perform size checks using mem::size_of and then use transmute_copy which can be used with generic types.

@Amanieu
Copy link
Member

Amanieu commented May 12, 2016

Considering atomic intrinsics only work with integer types and that trasmute restrictions on generic types can easily be bypassed by using transmute_copy, I don't see much point in this RFC.

@nikomatsakis
Copy link
Contributor

We discussed this in the @rust-lang/lang meeting and decided not to accept this RFC at this time (though I think there is some interest in potentially revisiting this approach at a later time). Basically the feeling is that the point is to some extent moot: As @Amanieu commented, the atomic question has been resolved, and transmute is being handled (for the moment) in a satisfactory way, and has a workaround for the known limitations.

If we did want to revisit transmute, there are basically two options:

  • the way advocated in this rfc (perhaps with trap);
  • or, adding a special trait that means "can transmute", so that one can use transmute in generic code.

This RFC ably described the former option, but we'd like to explore the latter a bit before deciding.

@nikomatsakis
Copy link
Contributor

(Thanks @arielb1!)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. T-compiler Relevant to the compiler team, which will review and decide on the RFC. T-lang Relevant to the language team, which will review and decide on the RFC.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants