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

Implement volatile type qualifier #962

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

Conversation

flaviojs
Copy link
Contributor

@flaviojs flaviojs commented May 22, 2024

Emits the C volatile type qualifier for tranaprent that have the annotation cbindgen:volatile=true.
Emit C volatile type qualifier for transparent 1-field structs and struct/union fields that have the volatile annotation.
Includes a mention in docs.md.
Includes test volatile.

Fixes #960
Fixes the paths of globals not being mangled.

I want to use this in public code so I would appreciate a release of cbindgen with this capability.
Pinging @emilio as the README mentions.

@flaviojs
Copy link
Contributor Author

flaviojs commented May 23, 2024

Btw, if you want I'm available to review+test+merge PRs that don't have merge conflicts, are not authored by me, and don't involve making releases.
I'd probably implement some "help wanted" issues too, after getting more familiar with the code.

@emilio
Copy link
Collaborator

emilio commented May 26, 2024

Hi, thanks for the PR!

Do you know what the use case for tagging a type itself with volatile is? That's generally not how qualifiers work, e.g. you apply the qualifier to specific usages of the type.

So, not opposed to supporting stuff like:

/// cbindgen:volatile
field: u32,

Or so. But my other question is... Given rust doesn't support this qualifier at all, I'd expect consumers to deal with this the same way Rust would. E.g., when in rust you have:

std::ptr::read_volatile(&self.member)

In C++ you'd have something like:

template <typename T>
T read_volatile(const T* ptr) {
  return *((const volatile T*)ptr);
}

return read_volatile(&this->member); // Or so, or in C with a similar macro using typeof or what not

Is there any reason something like that wouldn't work? It seems weird for cbindgen to transmit information that isn't on the rust code at all...

@flaviojs
Copy link
Contributor Author

flaviojs commented May 26, 2024

I just tried to allow volatile in whatever places C allows.
The enum Type contains is_volatile because the C volatile qualifier is attached to the type, like const.

My use case is a conversion from C to rust, where a struct has a volatile variable.
When I move the struct to rust the C code that uses the struct needs to behave the same way.
To preserve C code behavior, cbindgen needs to emit the volatile qualifier.
If I make a wrapper in rust that treats the inner type as volatile, I still need a way to emit the volatile qualifier.

The volatile test shows what I think might be encountered in real code.
Should I focus on what is possible with a volatile rust wrapper instead?

Sidenote:
I moved the contents of each enum Type variant to their own struct because it allows easy expansion of the contents.
Is this acceptable (temporarily causes conflicts with other PRs) or should I reimplement without that?

@flaviojs
Copy link
Contributor Author

flaviojs commented May 27, 2024

Also fixed the paths of globals not being mangled, discovered while testing the wrapper.
Not sure how to handle array types... left as a TODO in the volatile test.

@flaviojs
Copy link
Contributor Author

Just in case it's not clear, this PR is ready for review.

…ruct/union fields that have the `volatile` annotation.
@flaviojs
Copy link
Contributor Author

Rebased to resolve merge conflicts. Still awaiting review.

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.

How do I mark a struct variable volatile?
2 participants