Fix CI buildbot failures: MSVC C89, SSE2 32-bit, webOS cross-compile#115
Conversation
There was a problem hiding this comment.
Pull request overview
Fixes multiple CI build failures across older MSVC, 32-bit x86 SSE2, and webOS cross-compilation by addressing C89 declaration rules, replacing an x86_64-only SSE2 intrinsic usage, and improving SIMD selection logic during cross-compiles.
Changes:
- Adjusts SSE2 blitter code to avoid
_mm_cvtsi128_si64(x86_64-only) and fixes C89 mid-block declarations. - Moves
externdeclarations in libretro save-state paths to the top of blocks for MSVC C89 compatibility. - Updates
Makefile.commonto skip host-arch SIMD auto-detection whenCCappears to be a cross-compiler.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/blitter_simd_sse2.c |
Replaces 64-bit extract intrinsic and adjusts declarations for 32-bit SSE2 + C89 builds |
libretro.c |
Moves mid-block extern declarations to satisfy MSVC C89 parsing rules |
Makefile.common |
Avoids selecting SSE2 based on host uname -m when cross-compiling via CC prefix detection |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| #include "blitter_simd.h" | ||
| #include <emmintrin.h> /* SSE2 */ | ||
| #include <string.h> /* memcpy */ |
There was a problem hiding this comment.
#include <string.h> /* memcpy */ is added but this file doesn't use memcpy anymore. This extra include (and the comment) is now misleading; either remove it or actually use memcpy in the new extraction helper.
| #include <string.h> /* memcpy */ |
| * Use _mm_storel_epi64 which works on both 32-bit and 64-bit x86. */ | ||
| static INLINE uint64_t sse2_extract_u64(__m128i v) | ||
| { | ||
| uint64_t r; | ||
| _mm_storel_epi64((__m128i *)&r, v); |
There was a problem hiding this comment.
sse2_extract_u64 passes &r (a uint64_t) to _mm_storel_epi64 by casting it to __m128i *. Since __m128i is typically 16-byte aligned, this cast can violate alignment requirements and is undefined behavior on compilers that assume alignment from the pointer type. Consider storing into a properly-aligned temporary (e.g., __m128i tmp) and then copying out the low 64 bits (e.g., via memcpy), or use an explicitly unaligned store type if supported by the compiler.
| * Use _mm_storel_epi64 which works on both 32-bit and 64-bit x86. */ | |
| static INLINE uint64_t sse2_extract_u64(__m128i v) | |
| { | |
| uint64_t r; | |
| _mm_storel_epi64((__m128i *)&r, v); | |
| * Extract via memcpy from a local __m128i to avoid alignment UB from | |
| * casting a uint64_t* to __m128i*. */ | |
| static INLINE uint64_t sse2_extract_u64(__m128i v) | |
| { | |
| __m128i tmp = v; | |
| uint64_t r; | |
| memcpy(&r, &tmp, sizeof(r)); |
4957273 to
a1bffbf
Compare
|
Addressed both Copilot review comments in a1bffbf:
|
f86579f to
14783aa
Compare
14783aa to
dbcb260
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 7 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if ! gcc -fsyntax-only -std=gnu89 \ | ||
| -Werror=declaration-after-statement \ | ||
| -I. -Isrc -Isrc/m68000 -Ilibretro-common/include \ | ||
| -D__LIBRETRO__ -DINLINE="inline" \ |
There was a problem hiding this comment.
In the C89 lint job, -DINLINE="inline" overrides retro_inline.h and may break -std=c89 parsing (since inline is not a C89 keyword). Consider removing this define entirely (letting retro_inline.h pick __inline__ for GCC), or set it to a C89-safe token like __inline__/empty to avoid false CI failures.
| -D__LIBRETRO__ -DINLINE="inline" \ | |
| -D__LIBRETRO__ \ |
| name: Bump Version & Release | ||
|
|
||
| on: | ||
| workflow_dispatch: | ||
| inputs: | ||
| bump: | ||
| description: 'Version bump type' | ||
| required: true | ||
| type: choice | ||
| options: |
There was a problem hiding this comment.
The PR title/description focuses on fixing CI buildbot failures (MSVC/i686/webOS), but this PR also adds a new GitHub Actions version bump + release workflow. Please either update the PR description to cover these workflow additions (and why they’re needed for the CI fixes) or split them into a separate PR to keep scope focused.
dbcb260 to
f84befc
Compare
Code fixes: - libretro.c: Move extern declarations to top of function blocks for MSVC 2005/2010 C89 compliance (error C2143 mid-block declarations) - blitter_simd_sse2.c: Replace _mm_cvtsi128_si64 with memcpy-based helper — the former only exists on x86_64, breaking 32-bit x86 builds (Linux i686, Windows i686 MinGW). Also fix C89 mid-block declarations. - Makefile.common: Detect cross-compiler prefixes (arm-, aarch64-, mips, powerpc) in CC to skip host uname -m SIMD fallback. Fixes webOS ARM build getting SSE2 when built on an x86_64 host. CI coverage (c-cpp.yml — 13 build targets + C89 lint): - Add Windows i686 MinGW (MSYS2 MINGW32) - Add Linux i686 (gcc -m32 multilib) - Add Android NDK arm64-v8a + armeabi-v7a - Add iOS arm64 + tvOS arm64 (Xcode cross-compile) - Add C89 compliance lint job - Add workflow_dispatch for manual trigger via GitHub UI - Parameterize MSYS2 setup (msystem, packages) per matrix entry Release automation: - release.yml: Sync build matrix (12 platform artifacts including iOS/tvOS) - version-bump.yml: New workflow_dispatch to bump version in libretro.c, commit, tag, and push — triggering release.yml automatically .gitignore: Comprehensive ignore rules for build artifacts, test binaries, logs, ROMs, IDE files, Python venvs, and reference docs. Made-with: Cursor
f84befc to
ce3187b
Compare
Move uint8_t active_bank declarations to function scope in GPUStateSave and GPUStateLoad to satisfy C89/MSVC. Made-with: Cursor
Add C Language Standard section documenting the C89/GNU89 requirement and local lint command. Update stale "no test suite" line, add Jaguar CD, Testing, and expanded Key Directories sections. Made-with: Cursor
Summary
Fixes all CI failures from pipeline #64992:
MSVC 2005/2010 (windows-msvc05-i686, windows-msvc10-x64, windows-msvc10-i686):
error C2143: syntax error : missing ';' before 'type'—externdeclarations were placed mid-block inlibretro.csave state functions. MSVC's C89 mode requires all declarations at the top of a block. Moved them to function scope.Linux i686 + Windows i686 MinGW:
undefined reference to '_mm_cvtsi128_si64'— this SSE2 intrinsic only exists on x86_64 (needs 64-bit general-purpose registers). Replaced with a_mm_storel_epi64-based helper that works on both 32-bit and 64-bit x86. Also fixed C89 mid-block__m128ideclarations insse2_zcompandsse2_byte_merge.webOS ARM (libretro-build-webos-armv7a):
fatal error: emmintrin.h: No such file or directory— the webOS CI template sources an ARM cross-compiler SDK but doesn't setplatform, soMakefile.common's nativeuname -mfallback detected the x86_64 host and selected SSE2. Added cross-compiler prefix detection (arm-,aarch64-,mips,powerpcinCC) to skip the host-arch fallback when cross-compiling.Test plan
CC=arm-webos-linux-gnueabi-gcc→ scalar (cross-compile detected)CC=aarch64-linux-gnu-gcc platform=unix→ scalar (cross-compile detected)Made with Cursor