Skip to content

fix(ts-lsp): allocate tuple returns in arena#374

Closed
casualjim wants to merge 3 commits into
DeusData:mainfrom
casualjim:indexing-fix
Closed

fix(ts-lsp): allocate tuple returns in arena#374
casualjim wants to merge 3 commits into
DeusData:mainfrom
casualjim:indexing-fix

Conversation

@casualjim
Copy link
Copy Markdown
Contributor

I don't know if this is the right fix. But when I tried to follow main I'd end up with segfaults.
I could reproduce this in mac and linux host, this stops those segfaults.

@Shidfar
Copy link
Copy Markdown

Shidfar commented May 26, 2026

Cross-linking from #381 — this PR fixes the same crash. Independent reproduction confirms it.

Stack match: the ASan SEGV from your new tslsp_nocrash_crossfile_multi_return_call on unfixed upstream/main (6226972) lines up exactly with the gdb backtrace I posted in #381 (cbm_arena_alloccbm_type_tuplereturn_type_of at ts_lsp.c:1327ts_eval_expr_type). Same site, same root cause.

Coverage analysis: I had hypothesized three candidate paths that could feed return_type_of a multi-element return_types array. Your test exercises the only one reachable for TS in practice:

  1. cbm_run_ts_lsp_cross |-split on CBMLSPDef.return_types (ts_lsp.c:4351-4371) — what your test covers.
  2. register_file_defs array path (ts_lsp.c:3386-3398) — dead for TS sources: extract_return_types only produces multi-element arrays via extract_go_multi_return for Go's parameter_list node. For TS, add_cleaned_type always produces a 1-element array.
  3. Shallow-copy override (ts_lsp.c:3902, 3923) — just propagates rets from paths 1/2; not an independent trigger.

One test, one reachable path, full coverage. I drafted a 3-element variant locally to verify, and it exercises identical bytecode (the NULL arena is dereffed before count is read) — adding it would be padding.

Optimization sensitivity (FYI — explains why this only surfaced now): the bug is opt-level dependent on upstream/main:

That's why CI hadn't caught it — make -f Makefile.cbm cbm builds at -O2 and looks healthy. scripts/test.sh does catch it, so the existing test discipline holds; this path just hadn't been exercised by any test until yours.

Small heads-up on internal/cbm/arena.c: this PR patches both internal/cbm/arena.c and src/foundation/arena.c with the same if (!a || n == 0) return NULL; guard. Both files have the same fix, but only src/foundation/arena.c is referenced from Makefile.cbm:95-96 (FOUNDATION_SRCS = src/foundation/arena.c …). internal/cbm/arena.c looks like orphaned code from the c89538f → 94a9a92 "C-native pipeline" restructure that was never deleted. The double-patch is harmless (the orphan isn't linked), but you may want to remove the orphan separately — happy to file a focused cleanup PR for it if useful.

The if (!a || n == 0) return NULL; defense-in-depth in this PR is a nice belt-and-suspenders touch. Closing #381 as duplicate of this PR.

DeusData pushed a commit that referenced this pull request May 30, 2026
- cbm_arena_alloc returns NULL on a NULL arena (both arena.c copies)
  instead of dereferencing it — defense-in-depth against the NULL-arena
  type-allocation path that could crash the LSP type layer.
- discover: add "vendored" to the always-skip directory list.

Distilled from #374, taking the parts not already on main. The PR's
ts_lsp tuple-arena allocation, the C/C++ template formal-count clamp,
and the type_rep unbound-param preservation it also carried all landed
independently in v0.7.0 / via #322 / #360, so only these two
defensive improvements remain. Relates to #390.
@DeusData
Copy link
Copy Markdown
Owner

Thank you, @casualjim! 🙏 And thank you for your honesty in the description — that candor actually helped me triage this well. You were chasing real segfaults, and the good news is that most of what you fixed has since landed on main independently, so you were on exactly the right track:

  • The ts_lsp tuple-arena allocation (return_type_of passing NULL to cbm_type_tuple) — your titular fix — is already on main: return_type_of now threads ctx->arena through. That was indeed a real crash, fixed in v0.7.0.
  • The C/C++ template formal_count clamp and pending-call NULL guards landed via fix(c-lsp): guard pending template call resolution #322.
  • The type_rep unbound-param preservation (return type_args[i] ? type_args[i] : t) landed via Fix C++ template call segfault #360.

So the segfaults you reproduced on Mac and Linux should be gone on current main — I'd encourage a re-test.

Two genuinely-new improvements from your PR weren't on main, so I distilled them in and credited you as author (9e2bb92): the !a NULL guard in cbm_arena_alloc (nice defense-in-depth backstop for exactly the NULL-arena→tuple path) and skipping vendored/ directories, both with your tests. Build clean, all 3,624 tests pass.

I intentionally left out two things: graph-ui/tsconfig.tsbuildinfo (a generated build artifact that slipped in), and your C/C++ LSP wall-clock time budget (CBM_C_LSP_BUDGET_MS / c_lsp_should_abort). That last one is genuinely interesting — it complements the per-file eval-step budget that landed via #323 — but it was interleaved with the now-redundant template code, and I didn't want to risk a messy extraction. If C/C++ indexing still hangs for you on current main, a focused standalone PR for just that wall-clock budget against today's c_lsp.c would be very welcome. Thank you again! 🙏

@DeusData DeusData closed this May 30, 2026
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.

3 participants