Fix __int128_t redefinition in int128.h !CONFIG_INT128 fallback on Clang/GCC - #2360
Open
robertream wants to merge 1 commit into
Open
Fix __int128_t redefinition in int128.h !CONFIG_INT128 fallback on Clang/GCC#2360robertream wants to merge 1 commit into
robertream wants to merge 1 commit into
Conversation
…ang/GCC
The struct-based fallback aliases the reserved builtin name __int128_t to
`struct Int128` so fallback code can name the type. That is only valid on
compilers that do NOT provide __int128_t as a builtin; on GCC/Clang it is a
"typedef redefinition with different types ('Int128' vs '__int128')" error
whenever a build reaches this fallback. It bit real Clang builds — Apple clang
on arm64, and the Rust `unicorn-engine-sys` crate's vendored build.
PR unicorn-engine#2251 guarded this only for clang-cl (`_MSC_VER && __clang__`). Gate instead
on `!defined(__SIZEOF_INT128__)` — the standard macro that is defined iff the
compiler provides the 128-bit builtin — which correctly skips the alias for all
GCC/Clang (including clang-cl) and keeps emitting it for MSVC, subsuming unicorn-engine#2251.
Add tests/regress/int128_redefinition.c reproducing the exact fallback construct
(fails to compile with the old guard on any __int128-capable compiler, compiles
with the fix), and a CI workflow compiling it on macos-14 (Apple Silicon, arm64
clang), macos-13 (Intel) and Linux. Existing CI never covers this: the C build
on macos-14 uses cmake with CONFIG_INT128 defined (native path, fallback not
compiled), and the Rust-crate CI does not run on Apple Silicon.
Member
|
Hi please note our PR should go against the dev branch. Also tests go to |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Building QEMU's
int128.hfails on Clang/GCC whenever the build reaches the!CONFIG_INT128fallback:The fallback aliases the reserved builtin name
__int128_ttostruct Int128so fallback code can name the type. That is only valid on compilers that do not
provide
__int128_tas a builtin. On GCC/Clang the builtin always exists (when128-bit ints are supported), so the alias is a redefinition error.
This bit two real setups:
unicorn-engine-syscrate's vendored build (its cmake path ends upwithout
CONFIG_INT128, so it compiles the fallback — and fails on any modernclang/gcc).
#2251 added the current guard
#if !(defined(_MSC_VER) && defined(__clang__)),but that only covers clang-cl; regular Clang/GCC still hit the redefinition.
Fix
Gate the alias on
!defined(__SIZEOF_INT128__).__SIZEOF_INT128__is the standardmacro that is defined iff the compiler provides the 128-bit builtin, so:
This subsumes #2251 and fixes the general case.
Validation
tests/regress/int128_redefinition.c— a self-contained reproduction of thefallback construct. It fails to compile with the old guard on any
__int128-capable compiler and compiles cleanly with the fix (verified on Appleclang / arm64, producing the exact error above pre-fix).
.github/workflows/int128-regression.yml— compiles + runs that test onmacos-14(Apple Silicon, arm64 clang),macos-13(Intel) and Linux.Why a new job: existing CI never exercises this fallback on arm64 clang — the
C-library CI builds on
macos-14via cmake withCONFIG_INT128defined (nativepath, fallback not compiled), and the Rust-crate CI (
Crate-publishing.yml) doesnot run on Apple Silicon.
Happy to drop the test/CI or fold it into an existing workflow if maintainers prefer.