Skip to content

Strip LTO flags from static Lua module build#3555

Open
hanxizh9910 wants to merge 3 commits intovalkey-io:unstablefrom
hanxizh9910:fix-clang-lto-linker-mismatch
Open

Strip LTO flags from static Lua module build#3555
hanxizh9910 wants to merge 3 commits intovalkey-io:unstablefrom
hanxizh9910:fix-clang-lto-linker-mismatch

Conversation

@hanxizh9910
Copy link
Copy Markdown
Contributor

@hanxizh9910 hanxizh9910 commented Apr 24, 2026

Summary

The daily CI sanitizer jobs with clang are failing during the build step.
When the static Lua module is built with -flto, the .o files contain
LLVM bitcode that gets archived into libvalkeylua.a. The system linker
cannot read this bitcode, causing build failures:

/usr/bin/ld: /home/runner/work/valkey/valkey/src/modules/lua/libvalkeylua.a: member /home/runner/work/valkey/valkey/src/modules/lua/libvalkeylua.a(debug_lua.o) in archive is not an object

The previous fix (#3546) pinned clang to version 17, but this was
insufficient, the issue is not just a version mismatch but that the
system linker fundamentally cannot read LTO bitcode from .a archives.

Example failure: https://github.com/valkey-io/valkey/actions/runs/24865821147/job/72801509768

Fix

Strip LTO flags from OPTIMIZATION in the Lua module Makefile using
override

Tested: https://github.com/hanxizh9910/valkey/actions/runs/24913834442

@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 24, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 76.33%. Comparing base (d2db0c2) to head (caf189a).

Additional details and impacted files
@@             Coverage Diff              @@
##           unstable    #3555      +/-   ##
============================================
- Coverage     76.35%   76.33%   -0.03%     
============================================
  Files           159      159              
  Lines         80054    80054              
============================================
- Hits          61125    61106      -19     
- Misses        18929    18948      +19     

see 26 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

hpatro added a commit that referenced this pull request Apr 24, 2026
Reverts #3546

This didn't help fix the build issue. Follow up PR is performed on
#3555

Signed-off-by: Harkrishn Patro <bunty.hari@gmail.com>
@hpatro
Copy link
Copy Markdown
Contributor

hpatro commented Apr 24, 2026

I've reverted #3546 in #3556 independently. I'll let @eifrah-aws review this.

@hanxizh9910 hanxizh9910 force-pushed the fix-clang-lto-linker-mismatch branch from db7f331 to 7664381 Compare April 24, 2026 06:44
Comment thread src/Makefile Outdated
# engine_lua.so
$(LUA_MODULE_NAME): .make-prerequisites
$(MAKE) -C modules/lua OPTIMIZATION="$(OPTIMIZATION)" BUILD_LUA="$(BUILD_LUA)"
$(MAKE) -C modules/lua OPTIMIZATION="$(LUA_MODULE_OPTIMIZATION)" BUILD_LUA="$(BUILD_LUA)"
Copy link
Copy Markdown
Contributor

@eifrah-aws eifrah-aws Apr 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would move this logic to the Lua makefile. (modules/lua/Makefile)

This part:

LUA_MODULE_OPTIMIZATION=$(subst -flto,,$(subst -ffat-lto-objects,,$(subst -flto=auto,,$(OPTIMIZATION))))

@hanxizh9910 hanxizh9910 force-pushed the fix-clang-lto-linker-mismatch branch 3 times, most recently from 3358bb6 to 0959311 Compare April 24, 2026 20:31
Signed-off-by: Hanxi Zhang <hanxizh@amazon.com>
Signed-off-by: Hanxi Zhang <hanxizh@amazon.com>
Signed-off-by: Hanxi Zhang <hanxizh@amazon.com>
@hanxizh9910 hanxizh9910 force-pushed the fix-clang-lto-linker-mismatch branch from 0959311 to caf189a Compare April 24, 2026 20:32
Copy link
Copy Markdown
Member

@roshkhatri roshkhatri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, this incorporates @eifrah-aws's suggestion

@sarthakaggarwal97
Copy link
Copy Markdown
Contributor

@hanxizh9910 can you share the latest test link, the one in the PR description tests the changes done in source makefile.

@hanxizh9910
Copy link
Copy Markdown
Contributor Author

@hanxizh9910 can you share the latest test link, the one in the PR description tests the changes done in source makefile.

@sarthakaggarwal97 Hi this is the latest test link: https://github.com/hanxizh9910/valkey/actions/runs/24913834442. I also updated the PR description

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

Successfully merging this pull request may close these issues.

5 participants