Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[libclang5] Wrong code generation surfaced by #2787 #2922

Closed
Kriskras99 opened this issue Sep 3, 2024 · 6 comments
Closed

[libclang5] Wrong code generation surfaced by #2787 #2922

Kriskras99 opened this issue Sep 3, 2024 · 6 comments

Comments

@Kriskras99
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link
Contributor

pvdrz commented Sep 7, 2024

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
Copy link
Member

kulp commented Sep 15, 2024

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
Copy link
Contributor

emilio commented Sep 15, 2024

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.

@emilio emilio closed this as completed Sep 15, 2024
@Kriskras99
Copy link
Contributor Author

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?

@pvdrz
Copy link
Contributor

pvdrz commented Sep 16, 2024

Shall I create a PR to update the requirements page in the User Guide to libclang 9?

That would be much appreciated

And is it possible to add a compile time error when libclang 5 is detected?

Not as a constant but maybe as a build error. On the other hand, I think its up to the folks using bindgen if they decide an unsupported version of clang if it just happen to work correctly on some instances and they have some weird restriction on their environment setup that prevents them from using a more recent version. It might be better to not explicitly forbid clang 5 from that perspective.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants