Make rustc_target usable outside of rustc #103693

HKalbasi · 2022-10-28T14:58:50Z

I'm working on showing type size in rust-analyzer (rust-lang/rust-analyzer#13490) and I currently copied rustc code inside rust-analyzer, which works, but is bad. With this change, I would become able to use rustc_target and rustc_index directly in r-a, reducing the amount of copy needed.

This PR contains some feature flag to put nightly features behind them to make crates buildable on the stable compiler + makes layout related types generic over index type + removes interning of nested layouts.

rustbot · 2022-10-28T14:58:56Z

r? @petrochenkov

(rustbot has picked a reviewer for you, use r? to override)

petrochenkov · 2022-10-28T19:02:33Z

Which parts of rustc_target do you need for rust-analyzer?
Maybe it's some more or less self-contained subset that can be factored out to a separate crate instead?

I'm going to nominate this for @rust-lang/compiler to gather feedback.

HKalbasi · 2022-10-28T19:47:55Z

Things I need overall are rustc_target::abi::*, rustc_index::IndexVec, rustc_middle::ty::ReprOptions and some of codes in rustc_ty_utils::layout. It might make sense to move all of them to a rustc_layout crate?

thomcc · 2022-10-28T19:57:38Z

IIRC there are a few parts of this that would be useful for cc too (perhaps unsurprisingly).

mati865 · 2022-10-29T08:27:23Z

Intellij-rust would also be interested in using such crate so there are more reasons to do that.

petrochenkov · 2022-10-29T08:40:10Z

It might make sense to move all of them to a rustc_layout crate?

I've heard something about this idea before, from @eddyb maybe?

oli-obk · 2022-10-29T09:36:25Z

What's the way you plan to use this crate? Download the rustc-src component and build it from there?

HKalbasi · 2022-10-29T11:17:09Z

No, I want to use auto published versions in crates.io, similar to how rustc_lexer is used in rust analyzer.

oli-obk · 2022-10-29T16:32:34Z

OK, so from our end we don't need to have any stability across releases? Just publish breaking version bumps when they happen?

eddyb · 2022-11-01T10:05:20Z

It might make sense to move all of them to a rustc_layout crate?

I've heard something about this idea before, from @eddyb maybe?

Thanks for pinging me, though I'm not sure how useful I can be for this specific usecase.

One of the old thoughts (circa 2017 IIRC?) about the whole ABI (incl. type layout) abstraction system in rustc_target, was indeed to let it be reusable, as long as a compiler has a close enough concept of "interned types" and can compute layouts from those types.

Naming-wise, I prefer putting "type layout" under the larger umbrella of "ABI", i.e. I've been wanting to rename ty::layout to ty::abi, but perhaps @RalfJung has conflicting opinions (and mine aren't that strong anyway).

In terms of making the Rust type -> layout computation reusable _{(which I would put in rustc_target::abi::layout, or if that seems like bundling too much together, rustc_abi::layout - in fact, rustc_target::spec could became rustc_abi::target_spec, and still keep everything together in one crate, but maybe that's going too far?)}
the way I would do it is make a simpler categorization of Rust types (halfway between Ty and Layout), so that the rustc-specific code creates shallow values/calls methods to "explain" Ty (erasing e.g. the distinction between enum/struct/tuples/closures, pretending ! is just an empty enum, etc. etc.) to the hypothetical reusable code that computes the complete Layout.

However if you want to be 1:1 compatible with rustc, this may be an X/Y problem, because rustc's logic both changes over time and is dependent on compilation flags and e.g. internal details of incremental-stable hashing (for e.g. the randomized padding mode - similar to how you cannot predict the value of TypeId for a type without a lot of rustc's code).

So it would be useful to know the limitations you expect - you mention type size, and you could approximate that, but if you want to be exact, testing-oriented features like #[rustc_layout(debug)] are the most accurate option right now...

rustbot · 2022-11-01T15:55:53Z

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

HKalbasi · 2022-11-01T16:38:37Z

I moved ReprOptions and parts of layout code in rustc_ty_utils to rustc_target::abi::layout.

In terms of making the Rust type -> layout computation reusable ...

I had the same thing in my mind and tried to implement it. There is now a function for unions, a function for structs and enums, one for never type, and a function for univariants which is used for closures and similars and inside implementation of structs. I think it covers most of the interesting parts, and the rest is some type-dependent boilerplate which trying to reuse them will cause more trouble than good, until a shared representation of types between rustc and rust-analyzer.

However if you want to be 1:1 compatible with rustc, this may be an X/Y problem, because rustc's logic both changes over time and is dependent on compilation flags and e.g. internal details of incremental-stable hashing (for e.g. the randomized padding mode - similar to how you cannot predict the value of TypeId for a type without a lot of rustc's code).

I want to be compatible with the latest stable rustc with a reasonable delay, and I think by frequently bumping the rustc_target it can be achieved. About compiler flags, I do want to support flags available on stable, but not supporting -Z randomize-layout and similar unstable flags is fine I think. But this is my opinion, @jonas-schievink do you consider not being 1:1 compatible with rustc as a deal breaker for showing type sizes in r-a?

jonas-schievink · 2022-11-01T16:56:47Z

Probably not, we're not 100% compatible in other ways already.

pnkfelix · 2022-11-03T14:39:58Z

So the big risks I can see here are:

This is hopefully "just a big refactoring", but its hard to actually verify that it is a refactoring, apart from the fact that the tests all still pass without being modified. (Maybe that's the best we can hope for.)
Is there some way to ensure that future changes don't break the usage patterns this person is seeking to support? (Could some unit tests on this PR somehow confirm this, or some other way of locally verifying that the exported forms of the crates "conform" to your expectations?)

pnkfelix · 2022-11-03T14:42:30Z

As an example of what I meant by 2, @wesleywiser brought up the idea in the triage meeting of a CI check to make sure the crate compiles on stable.

pnkfelix · 2022-11-03T14:47:51Z

We discussed this in T-compiler triage, see discussion here

Overall we think things can move forward here. There may be a number of creaky spots that we'll have to deal with (e.g. adding CI checks as mentioned above, and also the auto-publish scripts haven't actually been running for a while). But these issues need not block this PR.

bors · 2022-11-24T19:50:22Z

📌 Commit 390a637 has been approved by oli-obk

It is now in the queue for this repository.

bors · 2022-11-24T20:29:17Z

⌛ Testing commit 390a637 with merge b3bc6bf...

bors · 2022-11-24T23:45:25Z

☀️ Test successful - checks-actions
Approved by: oli-obk
Pushing b3bc6bf to master...

rust-timer · 2022-11-25T01:04:15Z

Finished benchmarking commit (b3bc6bf): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	3.1%	[2.1%, 4.2%]	3
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-1.9%	[-1.9%, -1.9%]	1
All ❌✅ (primary)	-	-	0

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	2.9%	[2.8%, 3.0%]	3
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.0%	[-2.0%, -2.0%]	1
All ❌✅ (primary)	-	-	0

eddyb · 2022-11-28T08:29:44Z

compiler/rustc_abi/src/lib.rs

+
+#[derive(PartialEq, Eq, Hash, Clone)]
+#[cfg_attr(feature = "nightly", derive(HashStable_Generic))]
+pub struct LayoutS<V: Idx> {


If you removed the other Layout, can you please rename this back?

The S suffix is a historical accidental that has been recently re-perpetuated, and I've told some people about it but sadly without more communication it won't be systemically fixed.

(i.e. it's always a mistake to do, the correct thing looks more like LayoutData, etc. - or in this case, since the weird-new-style interning is gone again, just Layout).

cc @oli-obk who seems to have been cleaning up some of that noise

Layout is still there, in rustc_target, just nested layouts are not interned.

eddyb · 2022-11-28T08:30:41Z

compiler/rustc_abi/src/lib.rs

+}
+
+#[derive(PartialEq, Eq, Hash, Clone, Debug)]
+#[cfg_attr(feature = "nightly", derive(HashStable_Generic))]


These features should've been named dep-of-rustc or something, HashStable_Generic is internal and not available "on nightly".

Hmm, maybe nightly isn't the best name, but since the auto publish script will publish dependencies as well, it will work in a nightly compiler and doesn't need to be in rustc necessarily.

eddyb · 2022-11-28T08:34:23Z

compiler/rustc_target/src/spec/mod.rs

+impl Target {
+    pub fn parse_data_layout<'a>(&'a self) -> Result<TargetDataLayout, TargetDataLayoutErrors<'a>> {


I don't like that this is in the "spec" section, the syntax is not dictated by rustc target specs, it's a LLVM thing.

So maybe we need TargetDataLayout::parse_from_llvm_datalayout_string and then this method can keep the extra checks?

eddyb · 2022-11-28T08:35:28Z

compiler/rustc_index/src/lib.rs

+#![cfg_attr(
+    feature = "nightly",
+    feature(
+        allow_internal_unstable,
+        extend_one,
+        min_specialization,
+        new_uninit,
+        step_trait,
+        stmt_expr_attributes,
+        test
+    )
+)]


Oh, I see... So you want both a "nightly" feature and a "dep-of-rustc" feature (or however that's spelled) and then the latter should imply the former.

eddyb · 2022-11-28T08:40:58Z

compiler/rustc_target/src/abi/mod.rs

 rustc_index::newtype_index! {
    pub struct VariantIdx {
        derive [HashStable_Generic]
    }
 }



Why keep this here? Does RA want to substitute its own?

(Long-term it might make sense to have Layout take, as a generic parameter, some type that implements a trait with one associated parameter per type we want to customize etc. - instead of just adding more and more parameters - if the need arises, anyway)

Speaking of which, such a generic parameter would've allowed you to keep interning for variants, for rustc (which IIRC @nnethercote added intentionally), but use e.g. Box<Layout<...>> in other cases.

Why keep this here? Does RA want to substitute its own?

It needed to make changes rustc_macros, and r-a had an index system itself, so I choosed to do this.

Long-term it might make sense to have Layout take, as a generic parameter, some type that implements a trait with one associated parameter per type

Similar to chalk's interner. It make sense, but currently there is only one such argument. If we want to bring interning of nested layouts back (which doesn't showed itself in perf, maybe it only affects memory usage?) I can do that.

Extract llvm datalayout parsing out of spec module fix rust-lang#103693 (comment)

Make rustc_target usable outside of rustc I'm working on showing type size in rust-analyzer (rust-lang/rust-analyzer#13490) and I currently copied rustc code inside rust-analyzer, which works, but is bad. With this change, I would become able to use `rustc_target` and `rustc_index` directly in r-a, reducing the amount of copy needed. This PR contains some feature flag to put nightly features behind them to make crates buildable on the stable compiler + makes layout related types generic over index type + removes interning of nested layouts.

sunmy2019 · 2023-11-11T20:53:28Z

#117812 bisects to this

rustbot assigned petrochenkov Oct 28, 2022

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Oct 28, 2022

HKalbasi mentioned this pull request Oct 28, 2022

Compute data layout of types rust-lang/rust-analyzer#13490

Merged

HKalbasi force-pushed the master branch from e6c50be to ac75e4d Compare October 28, 2022 15:16

mati865 mentioned this pull request Oct 29, 2022

Feature request: Show the size/alignment of a type intellij-rust/intellij-rust#9329

Open

HKalbasi force-pushed the master branch from ac75e4d to 1a1a9d6 Compare November 1, 2022 15:55

rustbot added the T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) label Nov 1, 2022

HKalbasi force-pushed the master branch from 1a1a9d6 to b61ce04 Compare November 1, 2022 16:20

bors added the merged-by-bors This PR was explicitly merged by bors. label Nov 24, 2022

bors merged commit b3bc6bf into rust-lang:master Nov 24, 2022

rustbot added this to the 1.67.0 milestone Nov 24, 2022

eddyb reviewed Nov 28, 2022

View reviewed changes

HKalbasi mentioned this pull request Nov 30, 2022

Extract llvm datalayout parsing out of spec module #105087

Merged

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Dec 1, 2022

Rollup merge of rust-lang#105087 - HKalbasi:master, r=eddyb

a2e73d8

Extract llvm datalayout parsing out of spec module fix rust-lang#103693 (comment)

Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Dec 1, 2022

Rollup merge of rust-lang#105087 - HKalbasi:master, r=eddyb

8bdae2e

Extract llvm datalayout parsing out of spec module fix rust-lang#103693 (comment)

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Dec 1, 2022

Rollup merge of rust-lang#105087 - HKalbasi:master, r=eddyb

adec7ee

Extract llvm datalayout parsing out of spec module fix rust-lang#103693 (comment)

adpaco-aws mentioned this pull request Dec 12, 2022

[DRAFT] Update Rust toolchain to 2022-12-04 model-checking/kani#1983

Closed

4 tasks

celinval mentioned this pull request Jan 20, 2023

Update rust toolchain to 2022-12-11 model-checking/kani#2045

Merged

4 tasks

matthiaskrgr mentioned this pull request Jul 17, 2023

ICE: expected type of closure body to be a closure or coroutine #113776

Closed

MahadMuhammad mentioned this pull request Aug 26, 2023

ErrorCode Support Rust-GCC/gccrs#2553

Open

matthiaskrgr mentioned this pull request Nov 11, 2023

ICE: transmute: assertion failed: min <= max' #117812

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make rustc_target usable outside of rustc #103693

Make rustc_target usable outside of rustc #103693

HKalbasi commented Oct 28, 2022

rustbot commented Oct 28, 2022

petrochenkov commented Oct 28, 2022

HKalbasi commented Oct 28, 2022

thomcc commented Oct 28, 2022

mati865 commented Oct 29, 2022

petrochenkov commented Oct 29, 2022

oli-obk commented Oct 29, 2022

HKalbasi commented Oct 29, 2022

oli-obk commented Oct 29, 2022

eddyb commented Nov 1, 2022

rustbot commented Nov 1, 2022

HKalbasi commented Nov 1, 2022

jonas-schievink commented Nov 1, 2022

pnkfelix commented Nov 3, 2022

pnkfelix commented Nov 3, 2022

pnkfelix commented Nov 3, 2022

bors commented Nov 24, 2022

bors commented Nov 24, 2022

bors commented Nov 24, 2022

rust-timer commented Nov 25, 2022

eddyb Nov 28, 2022

eddyb Nov 28, 2022

HKalbasi Nov 29, 2022

eddyb Nov 28, 2022

HKalbasi Nov 29, 2022

eddyb Nov 28, 2022

eddyb Nov 28, 2022

eddyb Nov 28, 2022

HKalbasi Nov 29, 2022

sunmy2019 commented Nov 11, 2023

		impl Target {
		pub fn parse_data_layout<'a>(&'a self) -> Result<TargetDataLayout, TargetDataLayoutErrors<'a>> {

Make rustc_target usable outside of rustc #103693

Make rustc_target usable outside of rustc #103693

Conversation

HKalbasi commented Oct 28, 2022

rustbot commented Oct 28, 2022

petrochenkov commented Oct 28, 2022

HKalbasi commented Oct 28, 2022

thomcc commented Oct 28, 2022

mati865 commented Oct 29, 2022

petrochenkov commented Oct 29, 2022

oli-obk commented Oct 29, 2022

HKalbasi commented Oct 29, 2022

oli-obk commented Oct 29, 2022

eddyb commented Nov 1, 2022

rustbot commented Nov 1, 2022

HKalbasi commented Nov 1, 2022

jonas-schievink commented Nov 1, 2022

pnkfelix commented Nov 3, 2022

pnkfelix commented Nov 3, 2022

pnkfelix commented Nov 3, 2022

bors commented Nov 24, 2022

bors commented Nov 24, 2022

bors commented Nov 24, 2022

rust-timer commented Nov 25, 2022

Overall result: no relevant changes - no action needed

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sunmy2019 commented Nov 11, 2023