Automatically use the allocation-free API for Rust 1.27.0+. #103

anp · 2018-06-22T22:44:13Z

Introduces a buildscript to help decide which implementation to use by default. If using a version of Rust >= 1.27.0, what I've renamed the "inline" implementation will be automatically selected. On older versions of Rust, what I'm calling the "heap" implementation (previously the default one) will be selected. Originally I had removed the nightly feature entirely, but then I decided to keep it to preserve semver-major compatibility (let me know if you think this is taking compatibility too far -- it'd be nice to clean the feature up).

Another decision I made was to re-use the rustc_version crate for parsing version numbers rather than rolling our own as @BurntSushi did for regex. This does add build-time dependencies although no extra crates will be added to consumers' binaries. I think it improves clarity slightly and reduces overall churn. Happy to discuss if there's disagreement though.

Test plan: tests pass locally with 1.21.0, 1.27.0, and nightly-2018-06-20. Also will see what CI says.

Closes #102.

anp · 2018-06-22T22:48:11Z

Looks like nightly builds are failing right now for what I can only assume is an unrelated reason. The 1.21.0 and stable tests are passing on travis.

KodrAus · 2018-06-23T01:06:25Z

Thanks @anp! I'll dig into this later today.

Yeh it looks like the latest nightly is a bit broken...

KodrAus · 2018-06-24T07:22:01Z

Thanks @anp! I think we could simplify the cfg attributes a bit if we roll the nightly feature check into the version check in the build script. That way we only set the use_heap_impl if we're on a non-nightly compiler before 1.27.0 and leave nightly as a marker for spin, which only compiles on nightly.

What do you think?

anp · 2018-06-27T23:20:11Z

@KodrAus I like that idea a lot. Want to take a look at this approach (assuming CI passes after I write this)?

anp · 2018-06-27T23:37:10Z

I'm not able to reproduce this compiletest failure locally, but it seems like it's probably tripping up on an error message whose format has changed.

KodrAus

Thanks @anp! This looks good to me.

We've fixed up the compilefail test in #104. Would you like to rebase, just so we can sanity check CI?

anp · 2018-06-28T06:49:04Z

Rebased and squashed, thanks for the fast turnaround on CI repair!

anp · 2018-06-28T07:38:27Z

@KodrAus looks like we're all green.

anp · 2018-06-28T08:47:02Z

Actually, can we hold off on merging for a bit?

Specifically I want to ask about a soundness issue before this becomes part of the de facto 1.0 API for the crate, but it's late here.

eddyb · 2018-06-28T09:19:19Z

The reasons to hold are both simplification (#102 (comment)) and soundness (#105).

1.27.0 stabilized hint::unreachable, allowing the previously nightly-only API to be used on stable. The version detection can still be overridden using `cfg`s (or their related environment variables through cargo), and both the nightly and spin_no_std features are preserved for compatibility.

This is to patch an obscure soundness hole. With the fields public, it is possible to for a consumer crate to access and call the std::sync::Once without assigning the correct value to the pointer in the tuple's first field, making it possible to later deref a null pointer from safe code. However now that lazy_static targets 1.21.0+, we can use an associated constant in the __lazy_static_create macro instead of requiring consumers to literally construct the inner values post-expansion. This is technically a breaking change, but given that any existing use of these inner fields is very likely to cause unsoundness I think it's consistent with Rust's semver policy to make this change and stay at 1.0.

anp · 2018-07-08T21:23:10Z

Alright, thanks for being patient with me here. Unfortunately it's not possible to achieve all of the cleanup @eddyb suggested, as the inline impl requires Drop in statics, which wasn't stable until 1.22.0. If bumping the minimum compiler version of lazy_static to 1.22 is on the table, then we could go from 3 implementations to 2. Since I'm guessing it's not on the table (for good reasons), I've pushed some new commits to try to maximize outcomes for all consumers:

Fix the soundness problem (albeit an obscure one) in the inline impl
Also fix the soundness problem in the heap impl (this is technically a breaking change, see commit message) -- I don't think this is a requirement to enable the optimization for everyone but I think it's basically a non-breaking change and I think having lazy_static be as airtight as possible would be great
Finally I added a polyfill for core::hint::unreachable_unchecked that allows consumers at versions 1.22+ to use the allocation-free implementation by default

out of date

anp · 2018-07-09T16:50:04Z

Looks like the AppVeyor build failed due to network timeout -- is there an easy way to restart those?

KodrAus · 2018-07-09T21:46:34Z

Thanks @anp! This looks good to me with a quick scan on mobile. The rejigging of features looks like a good change 👍 I'll dig through again shortly.

Unfortunately there isn't an easy way to retry AppVeyor builds short of updating your branch. Their account model is a bit clumsier for organization ownership so a lot of projects end up tied to individual users.

…le_unchecked. Also editing the commit message to force a CI run. Woo!

anp · 2018-07-09T21:55:50Z

OK, I edited a commit message and re-pushed. CI is re-running.

anp · 2018-07-10T00:22:28Z

CI passed this time. I think this is good to go, pending review of the extra complexity I've added to the build matrix.

KodrAus · 2018-07-10T00:37:08Z

src/inline_lazy.rs

+}
+
+#[cfg(lazy_static_core_unreachable_unchecked)]
+use core::hint::unreachable_unchecked;


Do we need to bother with core::hint::unreachable_unchecked at all with our polyfill?

Nope, we can wait until the minimum version is raised for some other reason, to switch to the std version.

eddyb · 2018-07-10T06:14:33Z

src/inline_lazy.rs

+/// Polyfill for core::hint::unreachable_unchecked. Included to support Rust prior to 1.27. See
+/// [issue #102](https://github.com/rust-lang-nursery/lazy-static.rs/issues/102#issuecomment-400959779)
+/// for details.
+unsafe fn unreachable_unchecked() -> ! {


You don't need to do this yourself, turns out, https://docs.rs/unreachable exists.

Hm, I think it's reasonable in this case for lazy_static to include this very simple function to avoid bringing in 2 additional dependencies.

Ah that's fine, if the rationalization is added to the comment.

… polyfill.

anp · 2018-07-10T17:29:41Z

Thanks for the feedback, addressed both of these.

anp · 2018-07-16T15:49:30Z

@KodrAus ping

KodrAus

Thanks for your patience @anp! This looks good to me. We'll do a crater run to sniff out any breakage from the visibility changes.

anp · 2018-08-02T18:38:44Z

Looks like the crater run came back clean!

KodrAus · 2018-08-02T22:49:47Z

Merged 🎉 We might have a build fail on master again because of #110

KodrAus previously approved these changes Jun 28, 2018

View reviewed changes

pietroalbini mentioned this pull request Jun 28, 2018

lazy::Lazy<T> having public fields is unsound. #105

Closed

anp added 3 commits July 8, 2018 13:55

Patch obscure soundness issue in the inline lazy impl.

1a18c65

Makes inline impl the default for 1.22+ with a polyfill for unreachab…

f85db0b

…le_unchecked. Also editing the commit message to force a CI run. Woo!

KodrAus reviewed Jul 10, 2018

View reviewed changes

eddyb reviewed Jul 10, 2018

View reviewed changes

Remove extra build config, we'll always use our unreachable_unchecked…

0463a90

… polyfill.

KodrAus approved these changes Jul 17, 2018

View reviewed changes

KodrAus merged commit e714bee into rust-lang-nursery:master Aug 2, 2018

KodrAus mentioned this pull request Aug 4, 2018

Prepare for 1.1.0 release #112

Merged

KodrAus mentioned this pull request Nov 1, 2018

Remove the heap impl now that inline is always available #132

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Automatically use the allocation-free API for Rust 1.27.0+. #103

Automatically use the allocation-free API for Rust 1.27.0+. #103

anp commented Jun 22, 2018 •

edited

Loading

anp commented Jun 22, 2018

KodrAus commented Jun 23, 2018

KodrAus commented Jun 24, 2018

anp commented Jun 27, 2018

anp commented Jun 27, 2018

KodrAus left a comment

anp commented Jun 28, 2018

anp commented Jun 28, 2018

anp commented Jun 28, 2018 •

edited

Loading

eddyb commented Jun 28, 2018

anp commented Jul 8, 2018

anp commented Jul 9, 2018

KodrAus commented Jul 9, 2018

anp commented Jul 9, 2018

anp commented Jul 10, 2018

KodrAus Jul 10, 2018

eddyb Jul 10, 2018

eddyb Jul 10, 2018

KodrAus Jul 10, 2018

eddyb Jul 10, 2018

anp commented Jul 10, 2018

anp commented Jul 16, 2018

KodrAus left a comment

anp commented Aug 2, 2018

KodrAus commented Aug 2, 2018

Automatically use the allocation-free API for Rust 1.27.0+. #103

Automatically use the allocation-free API for Rust 1.27.0+. #103

Conversation

anp commented Jun 22, 2018 • edited Loading

anp commented Jun 22, 2018

KodrAus commented Jun 23, 2018

KodrAus commented Jun 24, 2018

anp commented Jun 27, 2018

anp commented Jun 27, 2018

KodrAus left a comment

Choose a reason for hiding this comment

anp commented Jun 28, 2018

anp commented Jun 28, 2018

anp commented Jun 28, 2018 • edited Loading

eddyb commented Jun 28, 2018

anp commented Jul 8, 2018

anp commented Jul 9, 2018

KodrAus commented Jul 9, 2018

anp commented Jul 9, 2018

anp commented Jul 10, 2018

KodrAus Jul 10, 2018

Choose a reason for hiding this comment

eddyb Jul 10, 2018

Choose a reason for hiding this comment

eddyb Jul 10, 2018

Choose a reason for hiding this comment

KodrAus Jul 10, 2018

Choose a reason for hiding this comment

eddyb Jul 10, 2018

Choose a reason for hiding this comment

anp commented Jul 10, 2018

anp commented Jul 16, 2018

KodrAus left a comment

Choose a reason for hiding this comment

anp commented Aug 2, 2018

KodrAus commented Aug 2, 2018

anp commented Jun 22, 2018 •

edited

Loading

anp commented Jun 28, 2018 •

edited

Loading