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

langref: improve packed struct memory layout description #21741

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

kj4tmp
Copy link
Contributor

@kj4tmp kj4tmp commented Oct 18, 2024

intended to close
#14384
#20647

Questions:

1. I don't know what to do about this TODO:

> TODO update these docs with a recommendation on how to use packed structs with MMIO
> (the use case for volatile packed structs) once this issue is resolved.
> Don't worry, there will be a good solution for this use case in zig.

  1. Does something different happen if the backing integer is specified as unsigned vs signed?

Looks like this:

image

</li>
<li>The backing integer has the same bit count as the fields' total bit width.</li>
<li>Field access and assignment can be understood as shorthand for bitshifts on the backing integer.</li>
<li>Integers will use exactly as many bits as they have. For example, a {#syntax#}u5{#endsyntax#} will use 5 bits of the backing integer.
Copy link
Contributor

Choose a reason for hiding this comment

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

</li> end tag missing here, which is reported by the CI failure. (I think that's only one).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! bit of a trip to reproduce this error locally.

I had to:

  1. Download superhtml and add it to my PATH (still a bit of a linux novice but I just added the executable to ~/bin/ and ran chmod +x on the executable)
  2. run zig build langref -Denable-superhtml

Copy link
Contributor

@rootbeer rootbeer left a comment

Choose a reason for hiding this comment

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

LGTM. I have a suggestion and a question, but this is an improvement without addressing either of those.

</li>
<li>The backing integer has the same bit count as the fields' total bit width.</li>
Copy link
Contributor

Choose a reason for hiding this comment

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

In "The backing integer has the same bit count as the fields' total bit width", should this say "must have" instead of "has"? (Also maybe add "If not inferred, ..."?)

Copy link
Contributor Author

@kj4tmp kj4tmp Oct 19, 2024

Choose a reason for hiding this comment

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

I don't think "must have" is best here because it communicates a constraint that the user must comply with, but the backing integer is constructed by the compiler for the user, so the user doesn't really need to do anything if they don't want to.

"Has" here just communicates a fact about the backing integer that is useful when communicating the memory layout.

Later in the doc, inferring vs. specifying the backing integer is explained as a readability and program correctness assertion feature, which I think is the correct way to explain it:

The backing integer can be inferred or explicitly provided. When inferred it will be unsigned, and when explicitly provided, its bit count will be enforced at compile time:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated inferred vs explicitly provided sentence

af40751

to read:

The backing integer can be inferred or explicitly provided. When inferred, it will be unsigned. When explicitly provided, its bit width will be enforced at compile time to exactly match the total bit width of the fields:

<li>{#syntax#}bool{#endsyntax#} fields use exactly 1 bit.</li>
<li>An {#link|enum#} field uses exactly the bit width of its integer tag type.</li>
<li>A {#link|packed union#} field uses exactly the bit width of the union field with
the largest bit width.</li>
<li>Packed structs, when they themselves are within a packed struct, will only use as many bits as their backing integer.</li>
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add anything to this doc about backing ints that are not a multiple of 8 bits?

(Perhaps this is mostly relevant in the MMIO case, where users wouldn't want undefined bits, and the load/store operations are going to be an 8-bit multiple? Hrm, and would a u24 get rounded up to a u32?)

The PR is still a doc improvement without this detail, so feel free to leave it off.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can you add anything to this doc about backing ints that are not a multiple of 8 bits?

This is intended to be captured by:
"The backing integer is subject to the same rules as any integer, including alignment,"

(Perhaps this is mostly relevant in the MMIO case, where users wouldn't want undefined bits, and the load/store operations are going to be an 8-bit multiple? Hrm, and would a u24 get rounded up to a u32?)

I suspect you are correct about its relevance to the MMIO case, and just below in the docs it talks about using overaligned pointers to override the alignment of the packed struct, which I guess is what the MMIO use case needs to do to avoid writing/reading to memory areas that are just padding for alignment?

"Packed structs have the same alignment as their backing integer, however, overaligned pointers to packed structs can override this:"

Copy link
Contributor Author

@kj4tmp kj4tmp Oct 19, 2024

Choose a reason for hiding this comment

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

Can you add anything to this doc about backing ints that are not a multiple of 8 bits?

In case you were referring to packed struct fields inside a packed struct,
6c330be
adds an example to to help explain this case.

@kj4tmp
Copy link
Contributor Author

kj4tmp commented Oct 19, 2024

I found an article about MMIO in zig: https://www.scattered-thoughts.net/writing/mmio-in-zig/

However, this was written in 2021, and integer-backed packed structs landed in 2022: #5049 (comment)

I will attempt to tailor the information into some recommendation for MMIO. Right now, I am thinking:

  1. remind user about alignment
  2. tell them how to override the alignment using over-align pointer and @ptrFromInt

But I won't be able to make a doc-test that actually uses @ptrFromInt because it would likely segfault so perhaps it will just be a paragraph

@kj4tmp kj4tmp marked this pull request as draft October 19, 2024 05:25
@kj4tmp kj4tmp marked this pull request as ready for review October 19, 2024 06:28
@kj4tmp
Copy link
Contributor Author

kj4tmp commented Oct 19, 2024

MMIO example needs close review from someone familiar with this use-case.

It is inspired by examples in the microzig framework:
https://github.com/ZigEmbeddedGroup/microzig/blob/1b8dff99cb3e034e853309c165c59f5c25bfa430/core/src/cpus/cortex_m.zig#L40

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants