Skip to content

How to sync rustc_abi with the "current" stable release #14846

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

Closed
Veykril opened this issue May 18, 2023 · 7 comments
Closed

How to sync rustc_abi with the "current" stable release #14846

Veykril opened this issue May 18, 2023 · 7 comments
Labels
A-layout Memory layout of types A-rustc issues regarding the rustc codebase and rustc private crates Broken Window Bugs / technical debt to be addressed immediately C-tracking-issue Category: tracking issue E-hard S-unactionable Issue requires feedback, design decisions or is blocked on other work

Comments

@Veykril
Copy link
Member

Veykril commented May 18, 2023

We now have support for calculating layouts of types which is a great thing to have, but there is a problem here. Type layouts are not stable so ideally the latest r-a we offer should work with the latest stable's type layout while the rustup releases should work with the corresponding rustc_abi crate there.

Currently we just try to do the former and the rustup releases are bound to just be out of sync with the toolchains actual layout (unless nothing changed over a version).

This is an interesting problem to have, and an important one to fix as with const eval being able to make use of this via `mem::size_of`` and friends, users will run into diagnostics due to mismatches and more. How can we solve this?

A simple somewhat working approach would be to just have current master always work with the latest stable version of the layout crate and patching upstream whenever a beta branch is done to use the upstream crate there. That will at least give us correct stable version matches, but ideally we would handle all releases (nightly as well) properly. That is somewhat tricky to do though since the crate has no stable API. Maybe we can abstract things so that we can have a feature toggle whether to use the upstream crate or the crates.io clone?

@Veykril Veykril added E-hard S-unactionable Issue requires feedback, design decisions or is blocked on other work Broken Window Bugs / technical debt to be addressed immediately C-tracking-issue Category: tracking issue A-rustc issues regarding the rustc codebase and rustc private crates labels May 18, 2023
@Veykril Veykril changed the title How to sync rustc_abi with the \*current\* stable release How to sync rustc_abi with the "current" stable release May 18, 2023
@HKalbasi
Copy link
Member

Maybe we can abstract things so that we can have a feature toggle whether to use the upstream crate or the crates.io clone?

If it was only size, it was easy. But we are currently using every small details of Layout in the mir interpreter to put the bytes in memory. So we will inevitably break if rustc decide to change layout radically (for example by adding a new encoding similar to niche encoding). But that kind of breakage is probably very low so maybe we can do that in a best effort basis (being compatible with both stable and nightly with a switch until a breakage happen, and then drop one of them until they become sync again).

An extreme solution is dropping our (really awesome) weekly release model, and making r-a a "normal" rust component, with normal nightly, beta and stable releases, shipped by rustup, ... . This will resolve the need for auto published crates, and saves some trouble in syncs with rustc repo.

Another extreme solution is declaring that r-a is a different rust compiler, and the layout of types is deferred to the implementation by the rust language, so whatever we show as the layout size is correct. We will try to keep it in sync with rustc stable, but users should add #[cfg(not(ra))] to static asserts that are sensitive to layout info. (or maybe #[cfg(rustc)], to make code also compatible with gccrs and other future rust compilers).

@flodiebold
Copy link
Member

But that kind of breakage is probably very low so maybe we can do that in a best effort basis (being compatible with both stable and nightly with a switch until a breakage happen, and then drop one of them until they become sync again).

Just to provide some more options: We could even keep supporting both versions, and decide either based on rustc version or even through some kind of feature detection which implementation to use / whether to use a specific layout feature. We'd have to see how much maintenance effort that actually is.

Yet another option would be to go the proc macro way and have a relatively lightweight separate component that we distribute with rustc that either does the layout calculations, or provides us enough information to do them correctly.

@HKalbasi
Copy link
Member

Doesn't proc macro have the same problem?

@Veykril
Copy link
Member Author

Veykril commented May 20, 2023

proc-macros are somewhat solved by having their abi be abstracted behind the proc-macro server which we communicate with via a stable interface defined by us. The proc-macro server is now being built upstream and shipped as a rustup component so we no longer have version mismatch problems there. The proc-macro server always runs the correct proc-macro abi for the installed toolchain.

So to do the same for layout here we'd basically need to lift out the layout calculations into a dynamic library we can load from the sysroot I suppose. That sounds really difficult to pull off though given the types involved.

@HKalbasi
Copy link
Member

I thought there was a problem similar with proc macros in the recent sync, but I didn't follow it in details so maybe it was unrelated.

I guess if we can use the dynamic library approach, we can also use the dependency patching approach. Dynamic library probably has the benefit that it will make the latest r-a working with all rustc versions, but it might not be possible due generic usage and it might have some performance considerations (mir interpreter and const eval might want to get layout for a huge number of types in a one action).

@Veykril
Copy link
Member Author

Veykril commented May 20, 2023

I thought there was a problem similar with proc macros in the recent sync, but I didn't follow it in details so maybe it was unrelated.

The syncs don't matter. Upstream proc-macro api changes require to change the rust-analyzer subtree upstream as well as its checked on CI that the component builts.

I'd personally would favor the feature toggle approach (as that would basically mean layout abi changes upstream have also fix rust-analyzer breakage upstream, effectively mirroring proc-macro server behavior). That way the rustup provided r-a will always have the toolchains layout. This does have a problem though, if a single r-a server is used for multiple workspaces using different toolchain versions, we will not be matching for some. The dylib approach would properly support that (just like proc-macro server does), but I think that's something we can ignore for now as adding components to rustup is a) more effort and b) they cannot be removed from rustup once added so its unlikely we'll get that added just like that.

@Veykril Veykril mentioned this issue Jun 19, 2023
7 tasks
@HKalbasi HKalbasi added the A-layout Memory layout of types label Jul 6, 2023
@Veykril Veykril mentioned this issue Sep 11, 2023
5 tasks
bors added a commit that referenced this issue Sep 19, 2023
Switch to in-tree rustc dependencies with a cfg flag

We can use this flag to detect and prevent breakages in rustc CI. (see #14846 and #15569)

~The `IN_RUSTC_REPOSITORY` is just a placeholder. Is there any existing cfg flag that rustc CI sets?~
@Veykril
Copy link
Member Author

Veykril commented Jan 11, 2024

I consider this "solved", it is not possible for the vscode extension to be synced for obvious reasons, but the rustup component is now dynamically linked against the rustc_driver so it will in fact be synced to the toolchain used. Thanks for figuring this out for us @HKalbasi

@Veykril Veykril closed this as completed Jan 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-layout Memory layout of types A-rustc issues regarding the rustc codebase and rustc private crates Broken Window Bugs / technical debt to be addressed immediately C-tracking-issue Category: tracking issue E-hard S-unactionable Issue requires feedback, design decisions or is blocked on other work
Projects
None yet
Development

No branches or pull requests

3 participants