Skip to content

Be more careful computing the size of an array Cluster. #519

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 5 commits into from
May 11, 2021
Merged

Be more careful computing the size of an array Cluster. #519

merged 5 commits into from
May 11, 2021

Conversation

rcls
Copy link
Contributor

@rcls rcls commented May 10, 2021 •
edited
Loading

cluster_size_in_bits was ignoring array information, leading to incorrect padding with nested clusters.
I noticed this as incorrect padding on the three SARs in PASS0 on the Cypress cyt4bb. (The .svd for this requires registration on the Cypress website.)
It also shows up as incorrect padding on the TCPWM0 in the Cypress PSOC6_04 (svd at https://github.com/cypresssemiconductorco/psoc6pdl/blob/master/devices/svd/psoc6_04.svd)

E.g., for the PSOC TCPWM0, the diffs on the generated code from this change are:

diff --git a/src/tcpwm0.rs b/src/tcpwm0.rs
index 678d146..7165c46 100644
--- a/src/tcpwm0.rs
+++ b/src/tcpwm0.rs
@@ -3,7 +3,7 @@
 pub struct RegisterBlock {
     #[doc = "0x00 - Group of counters"]
     pub grp0: GRP,
-    _reserved1: [u8; 32640usize],
+    _reserved1: [u8; 31744usize],
     #[doc = "0x8000 - Group of counters"]
     pub grp1: GRP,
 }

Eyeballing the GRP, it has 8 * 128byte entries = 1024 bytes, and the correct padding is then 32768 - 1024 = 31744 bytes.

@rcls rcls requested a review from a team as a code owner May 10, 2021 10:07
@rust-highfive
Copy link

r? @therealprof

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-tools labels May 10, 2021
@burrbull
Copy link
Member

If you are right, this is big bug.

Rustfmt and add changelog, please.

@rcls
Copy link
Contributor Author

rcls commented May 10, 2021

rustfmt corrected, I hope, and brief changelog entry added.

I'm not sure what your expectations around the latter are - let me know if you want a different amount of detail.

@rcls
Copy link
Contributor Author

rcls commented May 10, 2021 •
edited
Loading

Incidentally, I'm not entirely sure whether cluster_size_in_bits gets the size of a register array correct, in the case where the size does not match the dimIncrement (is that allowed?)

@burrbull
Copy link
Member

Looks good, but we need to test it on different inputs first.
cc @therealprof @adamgreig @Disasm

@burrbull
Copy link
Member

Incidentally, I'm not entirely sure whether cluster_size_in_bits gets the size of a register array correct, in the case where the size does not match the dimIncrement (is that allowed?)

In this case we need to add "_reserved" at the end of cluster block.
I don't remember if this already implemented.

@rcls
Copy link
Contributor Author

rcls commented May 10, 2021

Padding at the end of a cluster block is not implemented as far as I can see, looking at the code. register_or_cluster_block() adds padding between items but not at the end.

My change to cluster_size_in_bits does assume that clusters get padded - without that I fear the change here will break on an array-of-clusters that gets rendered as a list of fields rather than a rust array.

Another case that svd2rust doesn't handle correctly, as far as I can see, if the rust struct created for a cluster is struct Foo{x:u32, y:u16}, then the size of Foo is 8 bytes (64 bits), due to alignment padding, but cluster_size_in_bits will count only 48 bits. (I'm assuming Rust has C-like rules for alignment padding.)

@burrbull
Copy link
Member

burrbull commented May 10, 2021 •
edited
Loading

I'm assuming Rust has C-like rules for alignment padding.

https://doc.rust-lang.org/nomicon/repr-rust.html
We add #[repr(C)] for cluster structs, so yes in this case.

@rcls
Copy link
Contributor Author

rcls commented May 10, 2021 •
edited
Loading

My change to cluster_size_in_bits does assume that clusters get padded - without that I fear the change here will break on an array-of-clusters that gets rendered as a list of fields rather than a rust array.

This wasn't broken on the first case I looked at, but then I dug further and found more cases, that were broken (differently!) in both current master and with the first rev of this PR.

I have updateed cluster_info_size_in_bits() to reflect what expand_cluster() actually does.

But even better would be to move the padding inside the array element, which would remove the need for the cluster_size_in_bits() entirely.

However, this would also be a non-compatible change to the generate rust in some cases - because it will change when the array gets emitted as a rust array, and when it gets expanded. How big a deal is it if we make non-compatible changes to the generated rust?

(The change on this PR also does this, but only for cases where the generated rust was previously incorrect, I believe.)

@burrbull
Copy link
Member

burrbull commented May 11, 2021 •
edited
Loading

I would want to see several different examples of generated code like:
source xml, before, after.

You add non-zero padding after last cluster. So they can be collected into array only if their increment equals to content size.
I was saying before about adding this padding inside cluster struct.

I can be wrong, of course. Need examples.

rcls added a commit to rcls/svd2rust-example that referenced this pull request May 11, 2021
@rcls
With fixed svd2rust.
79a8fd9 
This corresponds to the first version of
rust-embedded/svd2rust#519
rcls added a commit to rcls/svd2rust-example that referenced this pull request May 11, 2021
@rcls
Updated svd2rust again.
848395b 
This has the further fix mentioned in this comment:
rust-embedded/svd2rust#519 (comment)
@burrbull
Copy link
Member

Ok. Can you rebase it on https://github.com/rust-embedded/svd2rust/tree/hex_reserved and I'll merge this patch.

rcls added 2 commits May 11, 2021 20:05
@rcls
Be more careful computing the size of an array Cluster.
f067e74 
Add changelog entry.
@rcls
Also correct the case, where:
d59a264 
* A cluster contains a cluster that ends with a cluster array.
* The innermost cluster has a mismatch between the size of the element data
  and the array dim_increment.
@rcls
Copy link
Contributor Author

rcls commented May 11, 2021

Ok, I rebased on hex_reserved. (I also squashed the "clean up formatting commits at the same time).

@burrbull
Copy link
Member

bors r+

@bors
Copy link
Contributor

bors bot commented May 11, 2021

Build succeeded:

@bors bors bot merged commit 5e30d5d into rust-embedded:master May 11, 2021
Emilgardis
Emilgardis reviewed May 11, 2021
View reviewed changes
CHANGELOG.md
- Option `-o`(`--output-path`) let you specify output directory path

### Changed

- `_rererved` fields in `RegisterBlock` now hexidemical usize
Copy link
Member

Choose a reason for hiding this comment

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

Spelling

Copy link
Member

Choose a reason for hiding this comment

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

:) My mistake

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Emilgardis Emilgardis Emilgardis left review comments

@burrbull burrbull burrbull approved these changes

Assignees

@therealprof therealprof

Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-tools
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@rcls @rust-highfive @burrbull @Emilgardis @therealprof