cbindgen icon indicating copy to clipboard operation
cbindgen copied to clipboard

Implement volatile type qualifier

Open flaviojs opened this issue 1 year ago • 6 comments

~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 avatar May 22 '24 07:05 flaviojs

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.

flaviojs avatar May 23 '24 11:05 flaviojs

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...

emilio avatar May 26 '24 16:05 emilio

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 avatar May 26 '24 18:05 flaviojs

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 avatar May 27 '24 21:05 flaviojs

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

flaviojs avatar Jun 11 '24 08:06 flaviojs

Rebased to resolve merge conflicts. Still awaiting review.

flaviojs avatar Oct 28 '24 17:10 flaviojs