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

Fix -Z print-type-sizes's handling of zero-sized fields. #67215

Merged

Conversation

nnethercote
Copy link
Contributor

Currently, the type struct S { x: u32, y: u32, tag: () } is
incorrectly described like this:

print-type-size type: `S`: 8 bytes, alignment: 4 bytes
print-type-size     field `.x`: 4 bytes
print-type-size     field `.tag`: 0 bytes, offset: 0 bytes, alignment: 1 bytes
print-type-size     padding: 4 bytes
print-type-size     field `.y`: 4 bytes, alignment: 4 bytes

Specifically:

  • The padding line is wrong. (There is no padding.)
  • The offset and alignment on the .tag line shouldn't be printed.

The problem is that multiple fields can end up with the same offset, and
the printing code doesn't handle this correctly.

This commit fixes it by adjusting the field sorting so that zero-sized fields
are dealt with before non-zero-sized fields. With that in place, the
printing code works correctly.

The commit also corrects the "something is very wrong" comment.

The new output looks like this:

print-type-size type: `S`: 8 bytes, alignment: 4 bytes
print-type-size     field `.tag`: 0 bytes
print-type-size     field `.x`: 4 bytes
print-type-size     field `.y`: 4 bytes

r? @pnkfelix

Currently, the type `struct S { x: u32, y: u32, tag: () }` is
incorrectly described like this:
```
print-type-size type: `S`: 8 bytes, alignment: 4 bytes
print-type-size     field `.x`: 4 bytes
print-type-size     field `.tag`: 0 bytes, offset: 0 bytes, alignment: 1 bytes
print-type-size     padding: 4 bytes
print-type-size     field `.y`: 4 bytes, alignment: 4 bytes
```
Specifically:
- The `padding` line is wrong. (There is no padding.)
- The `offset` and `alignment` on the `.tag` line shouldn't be printed.

The problem is that multiple fields can end up with the same offset, and
the printing code doesn't handle this correctly.

This commit fixes it by adjusting the field sorting so that zero-sized fields
are dealt with before non-zero-sized fields. With that in place, the
printing code works correctly.

The commit also corrects the "something is very wrong" comment.

The new output looks like this:
```
print-type-size type: `S`: 8 bytes, alignment: 4 bytes
print-type-size     field `.tag`: 0 bytes
print-type-size     field `.x`: 4 bytes
print-type-size     field `.y`: 4 bytes
```
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 11, 2019
@nnethercote
Copy link
Contributor Author

nnethercote commented Dec 11, 2019

The real-world type that I encountered this on was Pointer. I mistakenly thought it was 24 bytes until just now; it's really 16 bytes.

@pnkfelix
Copy link
Member

great catch!

@pnkfelix
Copy link
Member

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Dec 11, 2019

📌 Commit c681841 has been approved by pnkfelix

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 11, 2019
JohnTitor added a commit to JohnTitor/rust that referenced this pull request Dec 12, 2019
…ro-sized-fields, r=pnkfelix

Fix `-Z print-type-sizes`'s handling of zero-sized fields.

Currently, the type `struct S { x: u32, y: u32, tag: () }` is
incorrectly described like this:
```
print-type-size type: `S`: 8 bytes, alignment: 4 bytes
print-type-size     field `.x`: 4 bytes
print-type-size     field `.tag`: 0 bytes, offset: 0 bytes, alignment: 1 bytes
print-type-size     padding: 4 bytes
print-type-size     field `.y`: 4 bytes, alignment: 4 bytes
```
Specifically:
- The `padding` line is wrong. (There is no padding.)
- The `offset` and `alignment` on the `.tag` line shouldn't be printed.

The problem is that multiple fields can end up with the same offset, and
the printing code doesn't handle this correctly.

This commit fixes it by adjusting the field sorting so that zero-sized fields
are dealt with before non-zero-sized fields. With that in place, the
printing code works correctly.

The commit also corrects the "something is very wrong" comment.

The new output looks like this:
```
print-type-size type: `S`: 8 bytes, alignment: 4 bytes
print-type-size     field `.tag`: 0 bytes
print-type-size     field `.x`: 4 bytes
print-type-size     field `.y`: 4 bytes
```
r? @pnkfelix
bors added a commit that referenced this pull request Dec 12, 2019
Rollup of 8 pull requests

Successful merges:

 - #62514 (Clarify `Box<T>` representation and its use in FFI)
 - #66983 (Fix `unused_parens` triggers on macro by example code)
 - #67215 (Fix `-Z print-type-sizes`'s handling of zero-sized fields.)
 - #67230 (Remove irelevant comment on `register_dtor`)
 - #67236 (resolve: Always resolve visibilities on impl items)
 - #67237 (Some small readability improvements)
 - #67238 (Small std::borrow::Cow improvements)
 - #67239 (Make TinyList::remove iterate instead of recurse)

Failed merges:

r? @ghost
@bors bors merged commit c681841 into rust-lang:master Dec 12, 2019
@nnethercote nnethercote deleted the fix-Zprint-type-size-zero-sized-fields branch December 12, 2019 05:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants