C Atomics are not converted to equivalent atomics in Rust

[morrisonlevi]

Input C/C++ Header

struct globals {
  _Atomic _Bool interrupted;
};

Bindgen Invocation

$ bindgen input.h

Actual Results

In general, these get translated into u8 e.g.

/* automatically generated by rust-bindgen 0.59.2 */

#[repr(C)]
#[derive(Debug, Copy, Clone)]
pub struct globals {
    pub interrupted: u8,
}

However, while trying to make an even smaller reproduce case:

extern _Atomic _Bool interrupted;

I got a panic:

thread 'main' panicked at 'assertion failed: `(left == right)`
  left: `177`,
 right: `118`: Couldn't resolve constant type, and it wasn't an nondeductible auto type!', /Users/levi.morrison/.cargo/registry/src/github.com-1ecc6299db9ec823/bindgen-0.59.2/src/ir/var.rs:318:25
stack backtrace:
   0: rust_begin_unwind
             at /rustc/f1edd0429582dd29cccacaf50fd134b05593bd9c/library/std/src/panicking.rs:517:5
   1: core::panicking::panic_fmt
             at /rustc/f1edd0429582dd29cccacaf50fd134b05593bd9c/library/core/src/panicking.rs:100:14
   2: core::panicking::assert_failed_inner
             at /rustc/f1edd0429582dd29cccacaf50fd134b05593bd9c/library/core/src/panicking.rs:175:23
   3: core::panicking::assert_failed
   4: <bindgen::ir::var::Var as bindgen::parse::ClangSubItemParser>::parse
   5: <bindgen::ir::item::Item as bindgen::parse::ClangItemParser>::parse
   6: bindgen::parse_one
   7: bindgen::clang::visit_children
   8: __ZN5clang8cxcursor13CursorVisitor5VisitE8CXCursorb
   9: __ZN5clang8cxcursor13CursorVisitor23handleDeclForVisitationEPKNS_4DeclE
  10: __ZN5clang8cxcursor13CursorVisitor16VisitDeclContextEPNS_11DeclContextE
  11: __ZN5clang8cxcursor13CursorVisitor13VisitChildrenE8CXCursor
  12: _clang_visitChildren
  13: clang_sys::clang_visitChildren
  14: bindgen::Builder::generate
  15: std::panicking::try
  16: bindgen::main
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.

Expected Results

I would expect something more like this:

#[repr(C)]
#[derive(Debug, Copy, Clone)]
pub struct globals {
    pub interrupted: std::sync::atomic::AtomicBool,
}

Since std::sync::atomic::AtomicBool has the same in-memory representation as a bool does, I think that might be exactly what I would expect as output.

[ccleve]

Along the same lines, it appears that the volatile keyword is ignored.

extern volatile unsigned int myvolatile;

gets generated as

extern "C" {
    pub static mut myvolatile: ::std::os::raw::c_uint;
}

I'm honestly not sure how it should get generated.

read_volatile() and write_volatile() don't really work here, unless you somehow wrap all access to this variable in them. But I'm not even sure that would be correct. Here's a pretty good blog post explaining the issue: https://lokathor.github.io/volatile/ Apparently, volatile in C is not like volatile in Java, and in LLVM it's something different entirely.

Also, in my particular app, we're accessing shared memory across processes, not threads. That might or might not make a difference.

The myvolatile variable should probably be an atomic as well, and if that doesn't work then bindgen should throw an error and tell us to wrap all access to this variable in a Mutex.

[emilio]

Hmm, yeah, this is a bit tricky. Part of the issue here is that they're not quite the same, ABI wise. An AtomicBool is a struct with ``#[repr(C)], while I'm not sure how C++ compilers represent an _Atomic` variable (but most likely not as a struct).

But we should definitely not crash.

[emilio]

#2153 deals with the crash. What to do with atomics more generally needs more nuance.

[nico-abram]

According to rust-lang/unsafe-code-guidelines#208

the standard library makes no guarantees about how AtomicBool is implemented, in particular, it does not guarantee that it is implemented using UnsafeCell, i.e., the implementation can change to use UnsafeCell any time and that probably wouldn't be a breaking change

Also notice that an AtomicBool with anything but a 1 or 0 would be unsound because of the safe pub fn load(&self, order: Ordering) -> bool (Among others)

On the other hand, the docs for atomics like AtomicU8 https://doc.rust-lang.org/std/sync/atomic/struct.AtomicU8.html specify

This type has the same in-memory representation as the underlying integer type, u8.

I'm not sure since I'm not very familiar with C atomics, but maybe those are fine, and for AtomicBool maybe AtomicU8 can be used. As long as C atomic _Bool is guaranteed to be layout and ABI compatible with uint8_t.

[morrisonlevi]

Rust's bool is guaranteed to be the same as C's _Bool: rust-lang/rust#46176 (comment). I think it's the same for all integer types.

Seems what's missing is the guarantee in C that _Atomic(T) has the same representation as T. This is not required by the standard but is recommended when possible, especially for scalar types. However, this means that without that guarantee we can't say anything about what type it should convert into. If we are going to convert them into something instead of erroring, it seems like we should convert them into the corresponding Atomic* type in Rust, if we have one (it's not going to work for arbitrary types).

What do you think?