mozilla / cbindgen

A project for generating C bindings from Rust code
Mozilla Public License 2.0
2.27k stars 294 forks source link

Implement volatile type qualifier #962

Open flaviojs opened 1 month ago

flaviojs commented 1 month ago

~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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 3 weeks ago

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