Build failure with clang-16 or later

[uweigand]

Attempting to build libffi-rs using clang-16 or later as the C compiler fails with:

  ../src/tramp.c:262:22: error: call to undeclared function 'open_temp_exec_file'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
    tramp_globals.fd = open_temp_exec_file ();
                       ^
  1 error generated.

(This is the same problem pointed out here https://github.com/tov/libffi-rs/pull/69#issuecomment-1490844809, pulled out into a new issue since it is really unrelated to that pull request.)

The C source code is in fact invalid, C99 and later do not support implicit function declarations. However, most compilers up to now have treated this as simply a warning (and you do indeed see that warning when building libffi-rs with older clang versions or GCC), but as of clang-16, this is now treated as hard error by default. (See https://reviews.llvm.org/D122983 for the upstream discussion.)

That invalid source code was introduced into the libffi sources here https://github.com/libffi/libffi/commit/fc6b939066f211b99eed3c8111e446ec95d51ca6, which made its way into the 3.4.4 release. The bug was subsequently fixed via https://github.com/libffi/libffi/issues/760 and https://github.com/libffi/libffi/pull/764, which landed upstream but is not yet part of any official release.

This is unfortunate as clang-16 starts to become more frequently used, as this will mean the current libffi-rs release will not build out of the box when that compiler is used as default. In particular, this showed up as failure preventing moving rust's miri tool to the latest libffi-rs release (see https://github.com/rust-lang/rust/pull/109771), as some of the rust regression test builders already use clang-16.

I've been investigating options to fix this problem and make libffi-rs buildable again. These all boil down to one of two approaches: fix the actual problem in the C sources, or else downgrade the error back to a warning as it used to be:

1. Wait until libffi upstream creates the next release which will contain the fix, and pull that release into libffi-rs. This has the drawback that a significant amount of time may pass until an upstream release happens (they typically tend to release once a year or so).
2. Apply the fix from https://github.com/libffi/libffi/pull/764 to the copy of the libffi sources embedded in libffi-rs. I understand you're hesitant to maintain a set of patches; however, I don't think this is actually necessary. My suggestion would be to simply apply the fix directly to the libffi-rs repo. Once the next upstream libffi release happens, you can still simply fully replace the embedded copy with code from the next release - as this will already contain the fix, there's no need to maintain or track that fix separately in any way. Also, the fix is small and self-contained and has no user-visible impact apart from fixing the build error.
3. Disable or downgrade the error to a warning. This could be done by adding -Wno-implicit-function-declaration or -Wno-error=implicit-function-declaration to the CFLAGS when building the C portions of libffi, which could be done automatically e.g. by updating the libffi-sys-rs/build/not_msvc.rs script. However, there might be some issue with compilers that do not recognize this flag (GCC and clang do, but I'm not sure if there are any other compilers that could possibly be used to build libffi-rs).
4. Recommend to users of libffi-rs to add that flag when building libffi-rs, either manually or as part of their automated build processes. This has the obvious drawback of requiring action from all those users

My preference would be option 2, but the decision is of course up to you. I'd be happy to help implement any of those if needed.

[yorickpeterse]

Once the next upstream libffi release happens, you can still simply fully replace the embedded copy with code from the next release - as this will already contain the fix, there's no need to maintain or track that fix separately in any way.

This is only true if the next release in fact includes the changes. If for whatever reason this isn't the case (e.g. we update to some patch release not including the fixes), we have to re-apply the patch(es). This is precisely what I don't want.

Also, the fix is small and self-contained and has no user-visible impact apart from fixing the build error.

It's not the size that matters, but the extra work/effort needed when updating the libffi source code. It also sets a precedent of maintaining patches, which again isn't something I want.

If we can instead provide a flag to turn the error back into a warning, I'd much rather do that. I highly doubt anybody would compile libffi-rs with anything but clang or gcc, so the flag shouldn't be an issue. In the worst case we check what compiler is used and add the flag conditionally.

[uweigand]

Once the next upstream libffi release happens, you can still simply fully replace the embedded copy with code from the next release - as this will already contain the fix, there's no need to maintain or track that fix separately in any way.

This is only true if the next release in fact includes the changes. If for whatever reason this isn't the case (e.g. we update to some patch release not including the fixes), we have to re-apply the patch(es). This is precisely what I don't want.

OK, fair enough.

If we can instead provide a flag to turn the error back into a warning, I'd much rather do that. I highly doubt anybody would compile libffi-rs with anything but clang or gcc, so the flag shouldn't be an issue. In the worst case we check what compiler is used and add the flag conditionally.

Turns out this was easier than I expected, as the rust cc crate already provides a flag_if_supported mechanism, which allows adding a compiler flag only if the current compiler supports that flag - this is exactly what we want here.

I've submitted a patch as https://github.com/tov/libffi-rs/pull/74.

[yorickpeterse]

This should be fixed in version 2.2.1 of libffi-sys.

[uweigand]

Thanks again!

[JamesClarke7283]

rustpython-test-compile-failure.log

Getting the error, even when on "2.3.0".

[uweigand]

rustpython-test-compile-failure.log

Getting the error, even when on "2.3.0".

In your log I see:

  thread 'main' panicked at /home/impulse/.cargo/registry/src/index.crates.io-6f17d22bba15001f/libffi-sys-2.1.0/build/common.rs:8:5:

so it looks like 2.1.0 is actually getting built here.