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

[meta] update MSRV to 1.75, turn on asm support unconditionally #267

Conversation

sunshowers
Copy link
Collaborator

There were a lot of build flags working around asm incompatibilities. Now that
inline asm has been stable for a while, we can rip it all out.

Also update the MSRV to 1.75.

Created using spr 1.3.6-beta.1
@sunshowers sunshowers requested review from bnaecker and pfmooney July 29, 2024 23:36
sunshowers added a commit that referenced this pull request Jul 29, 2024
Created using spr 1.3.6-beta.1
usdt-impl/src/no-linker.rs Outdated Show resolved Hide resolved
usdt-impl/src/linker.rs Outdated Show resolved Hide resolved
@@ -5,7 +5,7 @@ edition = "2021"
license = "Apache-2.0"
description = "Dust your Rust with USDT probes"
repository = "https://github.com/oxidecomputer/usdt.git"
rust-version = "1.63.0"
rust-version = "1.75.0"

Copy link
Contributor

Choose a reason for hiding this comment

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

Commenting here, rather than lines 32-35, because GH review is bad...

We should be able to get rid of the asm feature in the child crates (usdt-impl, usdt-macro, usdt-attr-macro), I think? Leaving it at the top level makes sense, so that consumers are not forced to update their consumption until such time that we cut a version for breaking changes.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh good point. I wasn't actually sure whether those features were private or not so I left them in place. Sounds like it's just usdt that's public, and the rest are private.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, all the crates besides usdt are private implementation details, so I agree with Patrick, it'd be good to remove them. When we do get around to cutting a v0.6.0 and dropping the public features, we can close #57.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Dropped non-public features, and added a comment.

Copy link
Collaborator

@bnaecker bnaecker left a comment

Choose a reason for hiding this comment

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

Thanks for this, it's great cleanup! A couple of thoughts, but otherwise LGTM.

probe-test-build/build.rs Show resolved Hide resolved
@@ -5,7 +5,7 @@ edition = "2021"
license = "Apache-2.0"
description = "Dust your Rust with USDT probes"
repository = "https://github.com/oxidecomputer/usdt.git"
rust-version = "1.63.0"
rust-version = "1.75.0"

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, all the crates besides usdt are private implementation details, so I agree with Patrick, it'd be good to remove them. When we do get around to cutting a v0.6.0 and dropping the public features, we can close #57.

@sunshowers
Copy link
Collaborator Author

@pfmooney @bnaecker one consequence of this would be that selecting the no-op implementation documented here would no longer be possible:

usdt/usdt/src/lib.rs

Lines 312 to 344 in d4b8090

//! Selecting the no-op implementation
//! ----------------------------------
//!
//! It's possible to use the `usdt` crate in libraries without transitively requiring a nightly
//! compiler of one's users (prior to Rust 1.66). Though `asm` is a default feature of the `usdt`
//! crate, users can opt to build with `--no-default-features`, which uses a no-op implementation of
//! the internals. This generates the same probe macros, but with empty bodies, meaning the code can
//! be compiled unchanged.
//!
//! Library developers may choose to re-export this feature, with a name such as `probes`, which
//! implies the `asm` feature of the `usdt` crate. This feature-gating allows users to select a
//! nightly compiler in exchange for probes, but still allows the code to be compiled with a stable
//! toolchain.
//!
//! Note that prior to Rust 1.66, the appropriate features are required anywhere the generated
//! macros are _called_, rather than where they're defined. (Because they're macros-by-example, and
//! expand to an actual `asm!` macro call.) So library writers should probably gate the feature
//! directive on their own re-exported feature, e.g., `#![cfg_attr(feature = "probes",
//! feature(asm))]`, and instruct developers consuming their libraries to do the same.
//!
//! It's important to keep in mind how Cargo unifies features, however. Specifically, if `usdt` is
//! a dependency of two other dependencies in a package, it's possible to end up in a confusing
//! situation. Cargo takes the _union_ of all features in such a case. Thus if one crate is built
//! expecting to use the no-op implementation and another is built _using_ the real, `asm`-based
//! implementation, the latter will be chosen. This can be confusing or downright dangerous. First,
//! the former crate will fail at compile time, because the `asm!` macro will actually be emitted,
//! but the `#![feature(asm)]` flag will not be included. More troubling, the probes will actually
//! exist in the resulting object file, even if the user specifically opted to not use them.
//!
//! To handle this, library writers may place _all_ references to `usdt`-related code behind a
//! conditional compilation directive. This will ensure that the crate is not even used, rather
//! than it being used with an unexpected implementation. This is most relevant for crates whose
//! minimum supported Rust version is earlier than 1.66.

Could you confirm that this is what we want?

Created using spr 1.3.6-beta.1
@sunshowers sunshowers requested review from bnaecker and pfmooney July 30, 2024 02:56
@sunshowers
Copy link
Collaborator Author

(re-requesting review per q above)

Copy link
Collaborator

@bnaecker bnaecker left a comment

Choose a reason for hiding this comment

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

I think this looks good to me. Being able to select the no-op implementation was mostly a way to let libraries decide whether to trade-off a nightly compiler in exchange for probes. That's no longer needed, so emitting the right probe bodies unconditionally seems like the right call. Those are nearly free so long as the probe is disabled (just a load and compare), so it seems reasonable to always emit them.

Thanks for doing this!

@sunshowers sunshowers merged commit 51cbe87 into master Jul 30, 2024
11 checks passed
@sunshowers sunshowers deleted the sunshowers/spr/meta-update-msrv-to-175-turn-on-asm-support-unconditionally branch July 30, 2024 18:28
@sunshowers
Copy link
Collaborator Author

Thanks -- landed this, happy to address any remaining concerns in followups.

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.

3 participants