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.

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

Add repr(transparent) to VolatileCell #5

Closed
garasubo opened this issue Apr 22, 2019 · 3 comments
Closed

Add repr(transparent) to VolatileCell #5

garasubo opened this issue Apr 22, 2019 · 3 comments

Comments

@garasubo
Copy link
Contributor

repr(tranparent) guarantees the same layout and ABI as one single field for the struct with a single field. So, we can guarantee VolatileCell<T> layout and ABI as T.

This struct is already used in the code generated by svd2rust and it is ABI sensitive because it is used in structs with repr(C).
This was referenced Apr 22, 2019
Merged
Closed
@therealprof
Copy link

@japaric This seems very useful to have.

@couchand couchand mentioned this issue Oct 10, 2020
Merged
bors bot added a commit to rust-embedded/svd2rust that referenced this issue Nov 19, 2020
@bors @couchand
Merge #480
d6c668d 
480: Add repr(transparent) to Reg struct r=therealprof a=couchand

To guarantee that the register has the proper layout so that the [cast](https://github.com/rust-embedded/svd2rust/blob/971814070cd7be848d3c6524734192903de66866/src/generate/peripheral.rs#L63) of an integer to a `*const RegisterBlock` is sound.

Note that this is necessary but not sufficient.  The underlying `VolatileCell` also currently uses the default repr.  There is an open [issue](japaric/vcell#5) and [PR](https://github.com/japaric/vcell/pull/6/files) to address the concern.

Co-authored-by: Andrew Dona-Couch <hi@andrewcou.ch>
@Sympatron
Copy link

Sympatron commented Dec 3, 2020 •
edited
Loading

What is the current state here? I was quite surprised, that VolatileCell is not repr(transparent), because that means relying on VolatileCell<T> to have the same ABI as T is UB at the moment.

There is already a PR for this: #6

@korken89
Copy link
Collaborator

korken89 commented Jan 1, 2021

Merged and published in v0.1.3

@korken89 korken89 closed this as completed Jan 1, 2021
jannic added a commit to jannic-dev-forks/rp2040-pac that referenced this issue Aug 2, 2023
@jannic
Bump dependency on vcell to version 0.1.3
cfb0c4c 
The depenency used to be on version 0.1.0, but the pac doesn't actually
compile with that version:

```
error[E0599]: no method named `as_ptr` found for struct `vcell::VolatileCell` in the current scope
   --> src/generic.rs:101:23
    |
101 |         self.register.as_ptr()
    |                       ^^^^^^ method not found in `VolatileCell<<REG as RegisterSpec>::Ux>`
```

`as_ptr` was added in version 0.1.1 of vcell, but that version is marked
as yanked for unknown reasons. (japaric/vcell#13)

Version 0.1.2 would have been another option, but 0.1.3 contains a fix for
japaric/vcell#5 which looks important.
@jannic jannic mentioned this issue Aug 2, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@korken89 @garasubo @therealprof @Sympatron