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

Feature: Add a memory layout viewer #15081

Merged
merged 9 commits into from
Jul 9, 2023
Merged

Conversation

adenine-dev
Copy link
Contributor

Motivation: rustc by default doesn't enforce a particular memory layout, however it can be useful to see what it is doing under the hood, or if using a particular repr ensure it is behaving how you want it to. This command provides a way to visually explore memory layouts of structures.

Example:
this structure:

struct X {
    x: i32,
    y: u8,
    z: Vec<bool>,
    w: usize,
}

produces this output:
image

Work yet to be done:

  • tests (see below)
  • html is mildly janky (see below)
  • enums and unions are viewed flatly, how should they be represented?
  • should niches be marked somehow?

This was written for my own use, and the jank is fine for me, but in its current state it is probably not ready to merge mostly because it is missing tests, and also because the code quality is not great. However, before I spend time fixing those things idk if this is even something wanted, if it is I am happy to clean it up, if not that's cool too.

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 19, 2023
@HKalbasi
Copy link
Member

enums and unions are viewed flatly, how should they be represented?

What flat means here? Can you give some screen shots? Option<Vec<i32>> for niche optimized and Result<i32, i64> for direct would work.

@adenine-dev
Copy link
Contributor Author

By viewed flatly I mean that further fields are not shown because there aren't defined fields.

For example this structure:

struct X {
    x: Option<Vec<i32>>,
    y: Result<i32, i64>,
}

gives this:
image
So still correct size/align, but you don't get the inner layout of Vec like you would if it wasn't in an enum. This is fine since they don't have defined fields but maybe something could more sophisticated could be done with it idk.

@HKalbasi
Copy link
Member

It could cascade layout for each variant together. For example for result:

- 0
| Result<i32, i64> | tag       |  tag      |
- 4
|                  | i32       |  padding  |
- 8
|                  | padding   |  i64      |
- 12
|                  | padding   |           |
- 16                  Ok(i32)     Err(i64) 

But probably only for top level enums as it would become too noisy otherwise. And the current state is good for start I think.

Why it is left to right? I think top to bottom is more natural to represent these, and it would enable to render texts like Opti... completely. But it is just my personal preference.

@adenine-dev
Copy link
Contributor Author

its ltr because for larger structures the text will be cut off immediately without enough width and for me that came faster than how it will get cut off now, tho tbh this is a problem anyway but here one can see the first few characters and filter from there, also tbh it just felt more intuitive to read it that way since that's how structures are laid out in C.

It isn't shown but there is also a tooltip that gives all information, so you can still hover over it if you don't know what the field is:
image

@HKalbasi
Copy link
Member

HKalbasi commented Jul 6, 2023

@adenine-dev I think this is really nice and useful, and is small enough to be included in this repo. So if you make it pass CI and add some test we can go forward (enums and niches can be deferred to the future).

@rustbot author

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 6, 2023
@adenine-dev
Copy link
Contributor Author

Thanks for letting me know, work has been hectic so I haven't had much time to work on this, but I'll try to get it working over the weekend. :)

docs/dev/lsp-extensions.md Outdated Show resolved Hide resolved
docs/dev/lsp-extensions.md Outdated Show resolved Hide resolved
)
.unwrap();

assert_eq!(ml.nodes.len(), 1);
Copy link
Member

Choose a reason for hiding this comment

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

Would it be better to write a to_string method and use expect_test here? It seems that these are prone to change due changes in rustc side so snapshot testing can help here (you tried to use #[repr(C)] for that, but it wouldn't work for e.g. tuples). The to_string method can just print each node with id of its children, but if you make it human readable we can probably also expose it to terminal based editors.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think all of the tests are independent from rustc layout variance stuffs to my knowledge, though the downside to this is that it doesn't test tuple members or rustc struct layouts. I think one could use an offset_of macro to test them maybe, but not sure about that and idk if rust can do that in a not weird and hacky way.
As for to_string stuffs not really sure how that would change the problem?
For the type

struct AB {
  a: u32, 
  b: u32
}

rustc could lay it out as either (incomplete stringification as example):

AB: size 8, offset 0
  a: size 4, offset 0
  b: size 4, offset 4

or 

AB: size 8, offset 0
  b: size 4, offset 0
  a: size 4, offset 4

Even if it was reordered so that the fields were always consistently ordered the offsets would be different.

Having an in-built to_string method for terminals sounds like a good idea but since I don't use one not sure what design requirements it would need to consider.

Copy link
Member

Choose a reason for hiding this comment

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

rustc could lay it out as either (incomplete stringification as example):

The situation is not that bad. Rustc output is deterministic (even randomized layout which we don't support accepts a seed) but unstable. So it wouldn't change unless we bump the rustc_abi dependency crate version. Using to_string and expect-test we can run env UPDATE_EXPECT=1 cargo test and update all these tests with minimal effort.

I don't use one not sure what design requirements it would need to consider.

I don't know either, but I guess any string with a reasonable width should work? Maybe @nemethf can help us about this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

d0df00d reworks all the tests to use expect-test, I wrote a pretty simple to_string impl, though its not super useful as a visualization tool so I didn't expose it through the api (not really sure it should be since its extra work for those who don't use it, and probably editors will want to take into account users' preferences for tab/space, colorization, etc? 🤷‍♀️).

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe @nemethf can help us about this.

Unfortunately, I cannot. (I read this and other pull requests from the perspective of an LSP-client developer.)

@HKalbasi
Copy link
Member

HKalbasi commented Jul 9, 2023

Thanks!
@bors r+

@bors
Copy link
Contributor

bors commented Jul 9, 2023

📌 Commit d0df00d has been approved by HKalbasi

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Jul 9, 2023

⌛ Testing commit d0df00d with merge 5f11c9a...

@bors
Copy link
Contributor

bors commented Jul 9, 2023

☀️ Test successful - checks-actions
Approved by: HKalbasi
Pushing 5f11c9a to master...

@bors bors merged commit 5f11c9a into rust-lang:master Jul 9, 2023
@lnicola
Copy link
Member

lnicola commented Jul 10, 2023

image

@Veykril
Copy link
Member

Veykril commented Jul 10, 2023

Might be good to somehow mark in the output, that the layout is not stable for a viewed type if its not annotated with repr(C)

@c410-f3r
Copy link
Contributor

c00l feature! Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants