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

Compute data layout of types #13490

Merged
merged 4 commits into from
Dec 7, 2022
Merged

Compute data layout of types #13490

merged 4 commits into from
Dec 7, 2022

Conversation

HKalbasi
Copy link
Member

@HKalbasi HKalbasi commented Oct 26, 2022

cc #4091

Things that aren't working:

  • Closures
  • Generators (so no support for Future I think)
  • Opaque types
  • Type alias and associated types which may need normalization

Things that show wrong result:

  • Enums with explicit discriminant
  • SIMD types
  • NonZero* and similar standard library items which control layout with special attributes

At the user level, I didn't put much work, since I wasn't confident about what is the best way to present this information. Currently it shows size and align for ADTs, and size, align, offset for struct fields, in the hover, similar to clangd. I used it some days and I feel I liked it, but we may consider it too noisy and move it to an assist or command.

Copy link
Contributor

@jonas-schievink jonas-schievink left a comment

Choose a reason for hiding this comment

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

I'm somewhat worried about maintainability here. This copies 2000 lines from rustc instead of factoring the functionality out into a reusable crate, which is going to be hard to keep in sync.

crates/hir-ty/src/layout/tests.rs Outdated Show resolved Hide resolved
f32_align: AbiAndPrefAlign::new(Align::from_bytes(4).unwrap()),
f64_align: AbiAndPrefAlign::new(Align::from_bytes(8).unwrap()),
pointer_size,
pointer_align: AbiAndPrefAlign::new(Align::from_bytes(8).unwrap()),
Copy link
Contributor

Choose a reason for hiding this comment

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

This value is not correct on all targets (and others potentially too)

Copy link
Member Author

Choose a reason for hiding this comment

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

This is now fixed. About others, instruction_address_space is wrong for Harvard architectures but it doesn't affect layout computation, and vector_align is wrong but is for SIMD types which are not supported. I believe the rest is correct for at least x86, arm, riscv and powerpc.

@HKalbasi
Copy link
Member Author

rust-lang/rust#103693 can reduce ~800 lines of copied codes. But since rustc_ap_* crates aren't updated for a long time, are we actually able to use rustc crates if they are buildable on stable? How we can upgrade rustc_lexer if needed?

@Veykril Veykril added the S-blocked Status: marked as blocked ❌ on something else such as an RFC or other implementation work. label Nov 3, 2022
@HKalbasi HKalbasi force-pushed the layout branch 2 times, most recently from 8041c03 to f47fb10 Compare November 11, 2022 21:36
@lnicola
Copy link
Member

lnicola commented Nov 12, 2022

But since rustc_ap_* crates aren't updated for a long time, are we actually able to use rustc crates if they are buildable on stable?

I'm not familiar with the process of publishing those, but what do you mean? I remember rustc-ap-rustc_lexer being published pretty quickly (though I think we're behind on updating that).

@HKalbasi
Copy link
Member Author

HKalbasi commented Nov 12, 2022

The last published version is a year or so old: https://crates.io/crates/rustc-ap-rustc_lexer/versions. Before that it was published every week.

@HKalbasi HKalbasi force-pushed the layout branch 2 times, most recently from ae65739 to 6601b73 Compare November 12, 2022 20:24
crates/hir-def/Cargo.toml Outdated Show resolved Hide resolved
@HKalbasi
Copy link
Member Author

I updated the PR to use the new rustc_abi crate. There is no longer pure copy pasted code here, and the whole layout code is around 400 lines of code. I expect the rustc side to not change significantly, so I think this can be reviewed and especially if a change is needed on rustc's side, I can do it before the merge.

It can even be merged before the rustc PR, since I published my fork to the crates.io and it works at the current state. I will come with a follow up PR when my changes in rustc merged and rustc auto publish story becomes clear.

@rustbot review

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-blocked Status: marked as blocked ❌ on something else such as an RFC or other implementation work. labels Nov 12, 2022
bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 24, 2022
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.
@HKalbasi
Copy link
Member Author

rust-lang/rust#103693 just merged, and I updated my auto published crates to use upstream rustc.

@bors
Copy link
Contributor

bors commented Nov 29, 2022

☔ The latest upstream changes (presumably #13686) made this pull request unmergeable. Please resolve the merge conflicts.

@HKalbasi HKalbasi force-pushed the layout branch 2 times, most recently from 0bc0aed to c627fc4 Compare November 30, 2022 15:02
flip1995 pushed a commit to flip1995/rust-clippy that referenced this pull request Dec 1, 2022
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.
Copy link
Contributor

@jonas-schievink jonas-schievink left a comment

Choose a reason for hiding this comment

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

This looks pretty good!

crates/hir-ty/src/layout/tests.rs Outdated Show resolved Hide resolved
lib/la-arena/src/lib.rs Outdated Show resolved Hide resolved
check_size_and_align(
stringify!($($t)*),
::std::mem::size_of::<Goal>() as u64,
::std::mem::align_of::<Goal>() as u64,
Copy link
Contributor

Choose a reason for hiding this comment

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

Neat trick! This should even work when cross-compiling r-a.

i16_align: AbiAndPrefAlign::new(Align::from_bytes(2).unwrap()),
i32_align: AbiAndPrefAlign::new(Align::from_bytes(4).unwrap()),
i64_align: AbiAndPrefAlign::new(Align::from_bytes(8).unwrap()),
i128_align: AbiAndPrefAlign::new(Align::from_bytes(8).unwrap()),
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think the alignment of i64 and i128 are always 8 Bytes on all platforms. If there's no way to query the correct alignment from the cfg_options, can you add a FIXME comment?

Copy link
Member Author

Choose a reason for hiding this comment

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

There is a way: rustc +nightly -Z unstable-options --print target-spec-json. But it needs nightly. Is it possible to use it?

Copy link
Contributor

Choose a reason for hiding this comment

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

The rustc_cfg code already tries to use nightly-only features, and falls back to stable-compatible behavior if those are unavailable or don't work, so this should be doable. However, I think just setting the align to 8 should be fine for now, we can follow up with a fix that asks rustc later.

Copy link
Member

Choose a reason for hiding this comment

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

At least aarch64 and riscv64 use a 16 byte alignment for u/i128.

Copy link
Member Author

Choose a reason for hiding this comment

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

Added a fixme comment

@jonas-schievink
Copy link
Contributor

@bors r+

1 similar comment
@jonas-schievink
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Dec 7, 2022

📌 Commit 948a8f0 has been approved by jonas-schievink

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Dec 7, 2022

⌛ Testing commit 948a8f0 with merge 6e8a54d...

@bors
Copy link
Contributor

bors commented Dec 7, 2022

☀️ Test successful - checks-actions
Approved by: jonas-schievink
Pushing 6e8a54d to master...

@bors bors merged commit 6e8a54d into rust-lang:master Dec 7, 2022
bors added a commit that referenced this pull request Dec 9, 2022
Show type info on hover of enum variant fields

Small addition to #13490
@lnicola
Copy link
Member

lnicola commented Dec 12, 2022

image

bors added a commit that referenced this pull request May 6, 2023
Show type alias layout

This PR expands on #13490 to allow displaying layout data on hover for type aliases.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants