godotengine / godot

Godot Engine – Multi-platform 2D and 3D game engine
https://godotengine.org
MIT License
90.52k stars 21.08k forks source link

Android editor build error in etcpak on armv7 with NDK r23 #62516

Closed akien-mga closed 2 years ago

akien-mga commented 2 years ago

Godot version

4.0.dev (26dd4746a1a979132ef81c4674d3ea558a77710a)

System information

Fedora 36, compiling for Android armv7 with NDK r23

Issue description

Following #61691, it seems Android editor builds are failing in master due to an error in the etcpak dependency with some NEON intrinsics:

thirdparty/etcpak/ProcessRGB.cpp:3259:16: error: argument value 4 is outside the valid range 
        return vdupq_lane_s16( vget_low_s16( multipliers ), ClampConstant( Lane, 0, 4 ) );
               ^                                            ~~~~~~~~~~~~~~~~~~~~~~~~~~~
/root/sdk/ndk/23.2.8568313/toolchains/llvm/prebuilt/linux-x86_64/lib64/clang/12.0.9/include/arm_neon.h:5979:14: note: expanded from macro 'vdupq_lane_s16'
  __ret_24 = splatq_lane_s16(__s0_24, __p1_24); \
             ^                        ~~~~~~~
/root/sdk/ndk/23.2.8568313/toolchains/llvm/prebuilt/linux-x86_64/lib64/clang/12.0.9/include/arm_neon.h:829:23: note: expanded from macro 'splatq_lane_s16'
  __ret = (int16x8_t) __builtin_neon_splatq_lane_v((int8x8_t)__s0, __p1, 1); \
                      ^                                            ~~~~
thirdparty/etcpak/ProcessRGB.cpp:3723:46: note: in instantiation of function template specialization 'WidenMultiplier_EAC_NEON<8>' requested here
    uint8x8_t recVals[16] = { EAC_APPLY_16X( EAC_RECONSTRUCT_VALUE ) };
                                             ^

I've had to fix similar issues in the past (https://github.com/wolfpld/etcpak/issues/25), seems like my fixes worked for NDK r21 but not for NDK r23 or some of our build config changes. CC @madmiraal

Steps to reproduce

Minimal reproduction project

No response

akien-mga commented 2 years ago

Googling a bit gives https://reviews.llvm.org/D74619 which sounds like a change in Clang to do better validation of input to these intrinsics. So this probably just needs clamping between 0 and 3 instead of 0 and 4 upstream, I'll test and PR.