fix(build): rebuild lsp_all.o when its included headers change#662
Open
KerseyFabrications wants to merge 1 commit into
Open
fix(build): rebuild lsp_all.o when its included headers change#662KerseyFabrications wants to merge 1 commit into
KerseyFabrications wants to merge 1 commit into
Conversation
lsp_all.o is a unity object that #includes every lsp/*.c resolver; those include cbm.h, and CBMCall is copied BY VALUE across the lsp->pipeline boundary (cbm_calls_push). Its Make rule depended only on lsp_all.c, not on the headers it pulls in. So after CBMCall gained a field (is_method, DeusData#477) an incremental build kept a stale lsp_all.o with the old struct layout while the rest of the tree recompiled against the new one -- an ODR mismatch ASan flags as a stack-buffer-underflow in cbm_calls_push (the by-value copy under-reads the shorter stale struct). Clean builds (CI) always match, so it only bites incremental developer builds. Add an LSP_UNITY_DEPS prerequisite (lsp/*.c + lsp/*.h + lsp/generated/*.c + internal/cbm/*.h) to lsp_all.o and prod_lsp_all.o so the object rebuilds when any included header or unity source changes. Wildcard-based, so new LSP resolvers are covered automatically; explicit because this Makefile has no -MMD auto-deps. Verified: touching internal/cbm/cbm.h now rebuilds lsp_all.o (was stale before); clean build + full ASan suite green, rustlsp_dbg_macro_inner_call no longer aborts. Signed-off-by: Kris Kersey <kris@kerseyfabrications.com>
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.
Fixes #661
Problem
On an incremental build,
makecan link a stalelsp_all.o. ASan surfaces it as a stack-buffer-underflow during Rust-LSP tests:Root cause
lsp_all.ois a unity object that#includes everylsp/*.cresolver; those includecbm.h, andCBMCallis copied by value across the lsp→pipeline boundary (cbm_calls_push). Its Make rule depended only onlsp_all.c, not oncbm.h. AfterCBMCallgainedbool is_method(#477), incremental builds kept a stalelsp_all.ocompiled against the old 304-byte layout, while the rest of the tree recompiled against the new 312-byte layout — an ODR mismatch, so the by-value struct copy under-reads the shorter stale struct.Clean builds (CI) always recompile both sides together, so the layouts match and CI never sees it. It only bites incremental developer builds after the struct changed.
Fix
Add an
LSP_UNITY_DEPSprerequisite list (lsp/*.c,lsp/*.h,lsp/generated/*.c,internal/cbm/*.h) to bothlsp_all.oandprod_lsp_all.o, so the unity object rebuilds when any included header or unity source changes. Wildcard-based, so new LSP resolvers (e.g. an addedperl_lsp.c) are covered automatically. Explicit prerequisites rather than-MMDauto-deps, to match the Makefile's existing style.Verification
touch internal/cbm/cbm.h→lsp_all.onow rebuilds (it didn't before); no-op rebuild is a clean no-op.rustlsp_dbg_macro_inner_callno longer aborts.