[libclang5] Wrong code generation surfaced by #2787

Kriskras99 commented 1 week ago

If bindgen does not support libclang5 anymore (very reasonable stance), then this issue can be immediately closed.

This issue was surfaced by #2787

Input C/C++ Header

typedef struct {
    int a;
    _Atomic(struct c *) b;
} d;

Bindgen Invocation

$ bindgen input.h

Actual Results

llvm7/libclang5 generated bindings:

/* automatically generated by rust-bindgen 0.70.1 */

#[repr(C)]
#[derive(Debug, Copy, Clone)]
pub struct d {
    pub a: ::std::os::raw::c_int,
    pub b: d_c,
    pub __bindgen_padding_0: u64,
}
#[allow(clippy::unnecessary_operation, clippy::identity_op)]
const _: () = {
    ["Size of d"][::std::mem::size_of::<d>() - 16usize];
    ["Alignment of d"][::std::mem::align_of::<d>() - 8usize];
    ["Offset of field: d::a"][::std::mem::offset_of!(d, a) - 0usize];
    ["Offset of field: d::b"][::std::mem::offset_of!(d, b) - 8usize];
};
#[repr(C)]
#[derive(Debug, Copy, Clone)]
pub struct d_c {
    _unused: [u8; 0],
}

causing

error[E0080]: evaluation of constant value failed
  --> llvm7_libclang5.rs:15:31
   |
15 |     ["Offset of field: d::b"][::std::mem::offset_of!(d, b) - 8usize];
   |                               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ attempt to compute `4_usize - 8_usize`, which would overflow

error: aborting due to 1 previous error

For more information about this error, try `rustc --explain E0080`.

llvm11/libclang11 generated bindings (these do work)

/* automatically generated by rust-bindgen 0.70.1 */

#[repr(C)]
#[derive(Debug, Copy, Clone)]
pub struct d {
    pub a: ::std::os::raw::c_int,
    pub b: *mut c,
}
#[allow(clippy::unnecessary_operation, clippy::identity_op)]
const _: () = {
    ["Size of d"][::std::mem::size_of::<d>() - 16usize];
    ["Alignment of d"][::std::mem::align_of::<d>() - 8usize];
    ["Offset of field: d::a"][::std::mem::offset_of!(d, a) - 0usize];
    ["Offset of field: d::b"][::std::mem::offset_of!(d, b) - 8usize];
};
#[repr(C)]
#[derive(Debug, Copy, Clone)]
pub struct c {
    pub _address: u8,
}

Expected Results

The bindings to not break the compiler. Note that it's not only field offsets that can go wrong, in the original header the size_of is also wrong:

error[E0080]: evaluation of constant value failed
   --> /home/christiaan/scate2-dev/rust/target/release-customer/build/julia-sys-346f0adfcf2f3b4c/out/bindings.rs:477:5
    |
477 |     ["Size of jl_mutex_t"][::std::mem::size_of::<jl_mutex_t>() - 16usize];
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ index out of bounds: the length is 1 but the index is 368

I'd like to repeat, that it's perfectly valid to drop libclang5. Even for CentOS 7 users there is the possibility to install libclang11.

Kriskras99 commented 1 week ago

Here's a Dockerfile for anyone who wants to mess with llvm7/libclang5:

FROM centos:7

RUN sed -i s/mirror.centos.org/vault.centos.org/g /etc/yum.repos.d/*.repo && \
    sed -i s/^#.*baseurl=http/baseurl=http/g /etc/yum.repos.d/*.repo && \
    sed -i s/^mirrorlist=http/#mirrorlist=http/g /etc/yum.repos.d/*.repo

RUN yum -y update
RUN yum install -y liblz4-dev curl make python3 pigz zlib-devel openssl centos-release-scl scl-utils scl-utils-build gcc gcc-c++
RUN sed -i s/mirror.centos.org/vault.centos.org/g /etc/yum.repos.d/*.repo && \
    sed -i s/^#.*baseurl=http/baseurl=http/g /etc/yum.repos.d/*.repo && \
    sed -i s/^mirrorlist=http/#mirrorlist=http/g /etc/yum.repos.d/*.repo
RUN yum install -y llvm-toolset7

ARG USERNAME=user
ARG USER_UID=1000
ARG USER_GID=$USER_UID

RUN groupadd --gid $USER_GID $USERNAME \
    && useradd --uid $USER_UID --gid $USER_GID -m $USERNAME -s /bin/bash

USER ${USERNAME}
WORKDIR /home/${USERNAME}
# Rust install
RUN curl https://sh.rustup.rs -sSf | bash -s -- -y

Use source /opt/rh/llvm-toolset7/enable to enable libclang5.

For those who still need CentOS 7 but need llvm11/libclang11:

FROM centos:7

RUN sed -i s/mirror.centos.org/vault.centos.org/g /etc/yum.repos.d/*.repo && \
    sed -i s/^#.*baseurl=http/baseurl=http/g /etc/yum.repos.d/*.repo && \
    sed -i s/^mirrorlist=http/#mirrorlist=http/g /etc/yum.repos.d/*.repo

RUN yum -y update
RUN yum install -y liblz4-dev curl make python3 pigz zlib-devel openssl centos-release-scl scl-utils scl-utils-build gcc gcc-c++

RUN <<EOF cat > /etc/yum.repos.d/llvmtoolset-build.repo
[llvmtoolset-build]
name=LLVM Toolset 11.0 - Build
baseurl=https://buildlogs.centos.org/c7-llvm-toolset-11.0.x86_64/
gpgcheck=0 
enabled=1
EOF
RUN yum install -y --nogpgcheck llvm-toolset-11.0

ARG USERNAME=user
ARG USER_UID=1000
ARG USER_GID=$USER_UID

RUN groupadd --gid $USER_GID $USERNAME \
    && useradd --uid $USER_UID --gid $USER_GID -m $USERNAME -s /bin/bash

USER ${USERNAME}
WORKDIR /home/${USERNAME}

# Rust install
RUN curl https://sh.rustup.rs -sSf | bash -s -- -y

Use source /opt/rh/llvm-toolset-11.0/enable to enable libclang11.

pvdrz commented 1 week ago

I checked as far as I could with git bisect and this has been broken for a very very long time. At some point rust starts complaining about edition errors so I couldn't go further back into the commit history.

Maybe @emilio has some idea on when this happened as he has been around longer

kulp commented 4 hours ago

As @Kriskras99 says above, I suggest the answer to this issue is to drop support for libclang 5.0, in the spirit of #2166 and its related PRs, and moreover to prevent linking or loading libclang 5.0 or older.

But this is just a drive-by opinion; I have nothing more useful to add.

emilio commented 4 hours ago

Yes. In fact we no longer have CI for libclang 5 (oldest is libclang 9). It seems that what is going on is the following:

_Atomic didn't use to get exposed to libclang.
For some reason in libclang 5 we don't get reasonable layout information, so we end up with an empty struct rather than a pointer to an empty struct.
Thus the layout of the struct is incorrect. As of recently, we use compile tests rather than #[test] by default which exposed this.

I don't think this would be fixable on our end even if we wanted to fix it.

Kriskras99 commented 1 hour ago

Shall I create a PR to update the requirements page in the User Guide to libclang 9? And is it possible to add a compile time error when libclang 5 is detected?

rust-lang / rust-bindgen