search

rust-lang / rust-bindgen

Automatically generates Rust FFI bindings to C (and some C++) libraries.
https://rust-lang.github.io/rust-bindgen/
BSD 3-Clause "New" or "Revised" License
4.43k stars 695 forks source link

Bindgen can't translate `#pragma pack(n)` into `#[repr(packed = "n")]` #537

Closed asayers closed 5 years ago

asayers commented 7 years ago

Given the header

#pragma pack(1)
struct foo { int x; };

bindgen generates

#[repr(C)]
pub struct foo {
    pub x: ::std::os::raw::c_int,
}

which uses repr(C) instead of repr(packed), and therefore has the wrong alignment:

running 1 test
test bindgen_test_layout_foo ... FAILED

failures:

---- bindgen_test_layout_foo stdout ----
        thread 'bindgen_test_layout_foo' panicked at 'assertion failed: `(left == right)` (left: `4`, right: `1`): Alignment of foo', testcase.rs:12
note: Run with `RUST_BACKTRACE=1` for a backtrace.

failures:
    bindgen_test_layout_foo

test result: FAILED. 0 passed; 1 failed; 0 ignored; 0 measured

In fact, bindgen emits a warning to this effect:

WARN:bindgen::codegen: Type foo has an unkown attribute that may affect layout

Details

Steps to reproduce:

$ cat testcase.h
#pragma pack(1)
struct foo { int x; };
$ RUST_LOG=bindgen bindgen testcase.h -o testcase.rs
$ rustc --test testcase.rs
$ ./testcase
bindgen 0.22.0
rustc 1.15.1 (021bd294c 2017-02-08)
fitzgen commented 7 years ago

Thanks for the bug report! I'm not super familiar with #pragma pack -- is it standard? Or at least widely used? If not, we might want to consider whether implementing support is worth the pay off, and what ongoing maintenance costs might be.

If we did support this, I think we would want to use #[repr(C, packed)], not just #[repr(C)].

asayers commented 7 years ago

is it standard?

#pragma pack is understood by GCC and Visual Studio, and I think Borland too. I don't know about clang.

Or at least widely used?

I hope not! Abusing structs for serialisation purposes doesn't seem to be a good idea, and can cause problems. Unfortunately, I know of least one library which does this extensively.

whether implementing support is worth the pay off

bindgen is currently generating structs which have the wrong layout; this could also lead to some fairly strange behaviour. I think there are a number of ways you could go here:

  1. When bindgen sees #pragma pack(*) it aborts.
  2. When bindgen sees #pragma pack(*) it spits out a warning and stops generating bindings until it sees #pragma pack() (which resets things to normal).
  3. bindgen generates compatible structs. In this case, I think a warning is still in order, since repr(packed) can lead to undefined behaviour in rust.

[repr(C, packed)], not just #[repr(C)]

Ah yes, you're right.

emilio commented 7 years ago

I don't think we can even see #pragma packed from libclang?

I guess we can try to guess if we see the alignment is 1 and the relevant fields are packed when looking at their offsets, but...

radupopescu commented 7 years ago

Hello,

I'm having a similar issue on macOS 10.12.4. When generating the bindings for a header which includes "fcntl.h", the following struct is encountered:

#pragma pack(4)

struct log2phys {
    unsigned int    l2p_flags;   /* unused so far */
    off_t       l2p_contigbytes; /* F_LOG2PHYS:     unused so far */
                     /* F_LOG2PHYS_EXT: IN:  number of bytes to be queried */
                     /*                 OUT: number of contiguous bytes at this position */
    off_t       l2p_devoffset;   /* F_LOG2PHYS:     OUT: bytes into device */
                     /* F_LOG2PHYS_EXT: IN:  bytes into file */
                     /*                 OUT: bytes into device */
};

#pragma pack()

Rust-bindgen generates the following code:

#[repr(C)]
#[derive(Debug, Copy)]
pub struct log2phys {
    pub l2p_flags: ::std::os::raw::c_uint,
    pub l2p_contigbytes: off_t,
    pub l2p_devoffset: off_t,
}
#[test]
fn bindgen_test_layout_log2phys() {
    assert_eq!(::std::mem::size_of::<log2phys>() , 20usize , concat ! (
               "Size of: " , stringify ! ( log2phys ) ));
    assert_eq! (::std::mem::align_of::<log2phys>() , 4usize , concat ! (
                "Alignment of " , stringify ! ( log2phys ) ));
    assert_eq! (unsafe {
                & ( * ( 0 as * const log2phys ) ) . l2p_flags as * const _ as
                usize } , 0usize , concat ! (
                "Alignment of field: " , stringify ! ( log2phys ) , "::" ,
                stringify ! ( l2p_flags ) ));
    assert_eq! (unsafe {
                & ( * ( 0 as * const log2phys ) ) . l2p_contigbytes as * const
                _ as usize } , 4usize , concat ! (
                "Alignment of field: " , stringify ! ( log2phys ) , "::" ,
                stringify ! ( l2p_contigbytes ) ));
    assert_eq! (unsafe {
                & ( * ( 0 as * const log2phys ) ) . l2p_devoffset as * const _
                as usize } , 12usize , concat ! (
                "Alignment of field: " , stringify ! ( log2phys ) , "::" ,
                stringify ! ( l2p_devoffset ) ));
}

Some of these assertions are failing:

I don't know how helpful this is, but please let me know if I can supply more information.

Thanks for making bindgen!

mark-buer commented 7 years ago

I'm running into this same issue for osx/ios.

Some of the system provided headers use pack quite heavily.

Running:

cd /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX.sdk/
rg -c "^\s*#pragma\s*pack\(.*\d+\)" . | rg -v -N ":1$"

Gives the count of #pragma pack per header (with more than 1 occurrence):

usr/include/mach/clock.h:6
usr/include/mach/clock_priv.h:4
usr/include/mach/clock_reply.h:2
usr/include/mach/exc.h:6
usr/include/mach/host_security.h:4
usr/include/mach/host_priv.h:50
usr/include/mach/lock_set.h:12
usr/include/mach/mach_host.h:54
usr/include/mach/mach_port.h:72
usr/include/mach/mach_vm.h:40
usr/include/mach/mach_voucher.h:10
usr/include/mach/processor.h:12
usr/include/mach/processor_set.h:20
usr/include/mach/task.h:102
usr/include/mach/thread_act.h:56
usr/include/mach/vm_map.h:54
usr/include/net/if.h:4
usr/include/servers/netname.h:8
System/Library/Frameworks/AudioToolbox.framework/Versions/A/Headers/AudioUnitCarbonView.h:2
System/Library/Frameworks/IOKit.framework/Versions/A/Headers/OSMessageNotification.h:2
System/Library/Frameworks/IOKit.framework/Versions/A/Headers/iokitmig.h:160
System/Library/Frameworks/PCSC.framework/Versions/A/Headers/pcsclite.h:2
System/Library/Frameworks/IOKit.framework/Versions/A/Headers/scsi/SCSICmds_INQUIRY_Definitions.h:3
System/Library/Frameworks/IOKit.framework/Versions/A/Headers/usb/USB.h:5
System/Library/Frameworks/Kernel.framework/Versions/A/Headers/gssd/gssd_mach.h:18
System/Library/Frameworks/Kernel.framework/Versions/A/Headers/kextd/kextd_mach.h:2
System/Library/Frameworks/Kernel.framework/Versions/A/Headers/IOKit/OSMessageNotification.h:2
System/Library/Frameworks/Kernel.framework/Versions/A/Headers/lockd/lockd_mach.h:6
System/Library/Frameworks/Kernel.framework/Versions/A/Headers/mach/audit_triggers_server.h:2
System/Library/Frameworks/Kernel.framework/Versions/A/Headers/mach/clock.h:6
System/Library/Frameworks/Kernel.framework/Versions/A/Headers/mach/clock_priv.h:4
System/Library/Frameworks/Kernel.framework/Versions/A/Headers/mach/clock_reply_server.h:2
System/Library/Frameworks/Kernel.framework/Versions/A/Headers/mach/coalition_notification_server.h:2
System/Library/Frameworks/Kernel.framework/Versions/A/Headers/mach/exc_server.h:6
System/Library/Frameworks/Kernel.framework/Versions/A/Headers/mach/host_security.h:4
System/Library/Frameworks/Kernel.framework/Versions/A/Headers/mach/host_priv.h:50
System/Library/Frameworks/Kernel.framework/Versions/A/Headers/mach/ktrace_background.h:2
System/Library/Frameworks/Kernel.framework/Versions/A/Headers/mach/lock_set.h:12
System/Library/Frameworks/Kernel.framework/Versions/A/Headers/mach/mach_exc_server.h:6
System/Library/Frameworks/Kernel.framework/Versions/A/Headers/mach/mach_host.h:50
System/Library/Frameworks/Kernel.framework/Versions/A/Headers/mach/mach_port.h:72
System/Library/Frameworks/Kernel.framework/Versions/A/Headers/mach/mach_voucher.h:10
System/Library/Frameworks/Kernel.framework/Versions/A/Headers/mach/mach_voucher_attr_control.h:4
System/Library/Frameworks/Kernel.framework/Versions/A/Headers/mach/mach_vm.h:40
System/Library/Frameworks/Kernel.framework/Versions/A/Headers/mach/memory_object_default_server.h:2
System/Library/Frameworks/Kernel.framework/Versions/A/Headers/mach/memory_object_control.h:24
System/Library/Frameworks/Kernel.framework/Versions/A/Headers/net/if.h:2
System/Library/Frameworks/Kernel.framework/Versions/A/Headers/mach/notify_server.h:10
System/Library/Frameworks/Kernel.framework/Versions/A/Headers/mach/processor.h:12
System/Library/Frameworks/Kernel.framework/Versions/A/Headers/mach/processor_set.h:20
System/Library/Frameworks/Kernel.framework/Versions/A/Headers/mach/sysdiagnose_notification_server.h:2
System/Library/Frameworks/Kernel.framework/Versions/A/Headers/mach/task_access.h:4
System/Library/Frameworks/Kernel.framework/Versions/A/Headers/mach/task_access_server.h:4
System/Library/Frameworks/Kernel.framework/Versions/A/Headers/mach/task.h:102
System/Library/Frameworks/Kernel.framework/Versions/A/Headers/mach/telemetry_notification_server.h:2
System/Library/Frameworks/Kernel.framework/Versions/A/Headers/mach/thread_act.h:56
System/Library/Frameworks/Kernel.framework/Versions/A/Headers/mach/upl.h:8
System/Library/Frameworks/Kernel.framework/Versions/A/Headers/mach/vm_map.h:54
System/Library/Frameworks/Kernel.framework/Versions/A/Headers/sys/fcntl.h:2
System/Library/Frameworks/Kernel.framework/Versions/A/Headers/sys/mount.h:2
System/Library/Frameworks/Kernel.framework/Versions/A/Headers/IOKit/scsi/SCSICmds_INQUIRY_Definitions.h:3
System/Library/Frameworks/Kernel.framework/Versions/A/Headers/IOKit/usb/USB.h:5
System/Library/Frameworks/ApplicationServices.framework/Versions/A/Frameworks/QD.framework/Versions/A/Headers/ColorSyncDeprecated.h:2

And there are many more headers with a single #pragma pack occurrence (not listed here to avoid excessive noise).

fitzgen commented 7 years ago

AFAICT, @emilo is right, and libclang doesn't even give us access to #pragma pack.

For this header

#pragma pack(1)
struct foo { int x; };

we get this libclang AST (note the UnexposedAttr):

(
 kind = StructDecl
 spelling = "foo"
 location = /home/fitzgen/scratch/packed.h:2:8
 is-definition? true
 is-declaration? true
 is-inlined-function? false
 usr = "c:@S@foo"

 semantic-parent.kind = TranslationUnit
 semantic-parent.spelling = "/home/fitzgen/scratch/packed.h"
 semantic-parent.location = builtin definitions
 semantic-parent.is-definition? false
 semantic-parent.is-declaration? false
 semantic-parent.is-inlined-function? false

 type.kind = Record
 type.cconv = 100
 type.spelling = "struct foo"
 type.is-variadic? false

    (
     kind = UnexposedAttr
     spelling = ""
     location = builtin definitions
     is-definition? false
     is-declaration? false
     is-inlined-function? false

     type.kind = Invalid
    )
    (
     kind = FieldDecl
     spelling = "x"
     location = /home/fitzgen/scratch/packed.h:2:18
     is-definition? true
     is-declaration? true
     is-inlined-function? false
     usr = "c:@S@foo@FI@x"

     semantic-parent.kind = StructDecl
     semantic-parent.spelling = "foo"
     semantic-parent.location = /home/fitzgen/scratch/packed.h:2:8
     semantic-parent.is-definition? true
     semantic-parent.is-declaration? true
     semantic-parent.is-inlined-function? false
     semantic-parent.usr = "c:@S@foo"

     semantic-parent.semantic-parent.kind = TranslationUnit
     semantic-parent.semantic-parent.spelling = "/home/fitzgen/scratch/packed.h"
     semantic-parent.semantic-parent.location = builtin definitions
     semantic-parent.semantic-parent.is-definition? false
     semantic-parent.semantic-parent.is-declaration? false
     semantic-parent.semantic-parent.is-inlined-function? false

     type.kind = Int
     type.cconv = 100
     type.spelling = "int"
     type.is-variadic? false
    )
)
fitzgen commented 7 years ago

Another version of this issue:

struct AlignedToLess {
    int i;
} __attribute__ ((packed,aligned(2)));
LegNeato commented 7 years ago

Hmmm, reading http://llvm.org/viewvc/llvm-project?view=revision&revision=191345 (and the patch in http://lists.llvm.org/pipermail/cfe-commits/Week-of-Mon-20130923/089367.html) I would assume that packed should indeed be exposed by libclang?

It appears clang-sys at least has the constant: https://github.com/KyleMayes/clang-sys/blob/da6280de7ef5f5e7dde7e63ea00fb6df26eda601/src/lib.rs#L446

emilio commented 7 years ago

On 10/19/2017 08:25 PM, Christian Legnitto wrote:> Hmmm, reading

http://llvm.org/viewvc/llvm-project?view=revision&revision=191345 I would assume that packed should indeed be exposed by libclang? Yup, attribute(packed) is exposed by libclang, but not pragma pack, unfortunately.

fitzgen commented 6 years ago

I have a band-aid in https://github.com/rust-lang-nursery/rust-bindgen/pull/1136

It should make #pragma packed(1) work.

For #pragma pack(n) where n > 1, it detects that something is off and makes the type opaque. This is the best that we can do since Rust doesn't have #[repr(packed = "n")] yet.

gnzlbg commented 6 years ago

The PR for packed is there https://github.com/rust-lang/rust/pull/48528

It would be great if bindgen could start mapping #pragma pack(N) and the packed C attributes to #[repr(packed(N))] behind some sort of opt-in feature - the produced bindings are necessarily going to require nightly Rust until repr(packed) is stabilized.

Given that getting struct packing wrong is essentially undefined behavior, I'd wish that once the PR is merged it can be quickly stabilized.

gnzlbg commented 6 years ago

@fitzgen repr(packed(N)) just landed this week, would it be possible for bindgen to use it instead of making the struct opaque?

fitzgen commented 6 years ago

We can start doing that when the rust target = nightly now. I don't have time to write a PR myself, but I could review one.