Reclaim owned string temporaries (concat operand + encode builtins)#25
Open
loreste wants to merge 431 commits into
Open
Reclaim owned string temporaries (concat operand + encode builtins)#25loreste wants to merge 431 commits into
loreste wants to merge 431 commits into
Conversation
Encode/decode MessagePack and CBOR for int/bool/nil-null/string and int arrays; hex helpers. Add int list combinators (take/drop/zip/map/filter/fold). Packages under encoding/msgpack and encoding/cbor; suite test coverage.
Add Avro encode/decode for long/bool/null/string/array[long], expand GraphQL helpers (query/op name/vars/data2), package protobuf and graphql under std, and fixed named timezone offsets with format_offset. Suite test coverage.
Product version 0.1.7 across Cargo, docs, packaging, and release notes. Freezes CBOR/MessagePack/Avro, list combinators, GraphQL/protobuf packages, and named timezone offsets shipped since 0.1.6.
Bloom/PageMan/Predict (and ZipWriter/ReflectValue/Limits) no longer fall through to int64_t in type_expr_c, so they work as params, returns, and struct fields. bloom_clear resets a filter for in-place rebuild.
Run fill-release-packaging.sh against published Windows zip SHA and source tarball for tag v0.1.7.
Release CI finished for linux, darwin, and windows; refresh checksum notes after all platform archives published.
Singleton manifests are rejected by microsoft/winget-pkgs. Switch packaging templates and fill/publish scripts to the three-file format used in PR #402823 after the Manifest-Validation-Error.
Demand-driven arr helpers only tracked map values, so bare []Option[T] and []map[K]V failed to link MakoArr_opt_*/MakoArr_map_*. Track used array element tags and emit those monomorphs. Replace PTHREAD_MUTEX_INITIALIZER for mako_box_mu and mako_detached_mu with lazy init so Windows CRITICAL_SECTION shims compile.
First-class Bloom/PageMan/MultiMap handles; growing range buffers with iterators; multimap multi-value keys; string-key helpers; btree_save v2 checksums; pman bulk page I/O; domain registry slots; zero-copy CI string and builder_write_slice; file_append2/3 writev; sst_build8/n.
can_send/can_recv were shared by "slot free" and "handoff complete" waiters, but only one waiter was signaled. With concurrent producers (TestChanRendezvousMulti) a signal could wake the completing sender and leave other producers sleeping forever. Broadcast instead.
Encoder already uses compact integer forms; update expectations so CI matches the implementation (was still checking legacy fixed-width hex).
Use the same 2s sleep as wave8 so join_timeout(50) cannot race the task completing under macOS Actions load.
- wyhash replaces FNV-1a for map key hashing (4-8x faster on string keys) - Stack-based f-string builder (256B stack, zero malloc for short strings) - Compile-time constant folding for integer literal binary expressions - Zero-copy string views for comparisons, match arms, str_eq, str_has_prefix, str_has_suffix, str_contains, and print with string literals - Codegen want_map joined key lookup (eliminates per-call String allocation) - HTTP connection table scaled 32 to 1024 with atomic active count - HTTP header interning via length-bucketed switch dispatch - Channel cap() lock-free (immutable field, no mutex needed) - Slice append preserves sub-slice aliasing safety (malloc+copy, not realloc) - Fix msgpack test expectations to match spec-compliant compact encoding
The atomic active-count introduced with the 1024-slot table was never updated on accept/close, so graceful-shutdown drain saw 0 forever. Route all live transitions through mako_http_conn_set_live, and emit string-literal helpers without double-evaluating non-literal args.
Product version 0.1.8 across Cargo, docs, packaging, and release notes. Ships wyhash maps, stack f-strings, zero-copy string literals, constant folding, select condvar wakeups, HTTP 1024-slot table with correct atomic active count, lock-free chan_cap, and leaner codegen emit paths.
Ship compile-time monomorphization for generic structs and enums, interface bounds on type parameters, and seed support for iterator for-in and mutable closure captures. Full examples/testing suite green (322). Document known limits on by-value iterators and multi-statement mut lambdas.
Expand LANGUAGE_SPEC, GUIDE, book tour, LANGUAGE, STATUS, ERGONOMICS, and llms* for generic structs/enums, interface bounds, and iterator/ closure seeds. Fill Formula SHA, winget InstallerSha256, and multi-OS RELEASE-CHECKSUMS for tag v0.1.9.
Select wakeup used PTHREAD_MUTEX/COND_INITIALIZER and pthread_cond_timedwait, which are unavailable under the Windows CRITICAL_SECTION shims. Lazy-init the select mutex/condvar and add a SleepConditionVariableCS-based timedwait so CI native Windows and cross-compile smoke pass again.
Remove internal process notes and replace with contribution guidelines reference. Keep design principles inline.
- Add generic structs/enums/bounds, iterator protocol, f-strings - Add performance section (wyhash, zero-copy strings, constant folding, stack builder, condvar select, atomic HTTP tracking) - Expand stdlib listing (SIP, SMTP, templates, domain storage, crash reporting, full test categories) - Fix version tag at end of llms-full.txt (was 0.1.1) - Remove outdated "no full f-string" note
- Type-check chan_len and chan_cap as generic builtins accepting any chan[T], not just chan[int] - Add mako_chan_ptr_len runtime function for pointer-based channels - Add mako_chan_ptr_recv_timeout runtime function for timed receives - Codegen dispatches to mako_chan_ptr_len for MakoChanPtr* channels
Treat TypeExpr::Tuple channel elements as chan_struct, and register named-struct/enum and tuple monomorphs in chan_ptr_elems when the type is written as chan[T] so len/cap and ptr-channel helpers stay consistent.
Document that chan_len/chan_cap accept struct, tuple, enum, and string
channels. Add mako_chan_ptr_cap and mako_chan_str_{len,cap} so codegen
matches the docs, plus a regression test covering all three rings.
Align book, howto, STDLIB, STATUS, ROADMAP, website, and supporting guides with product tip 0.1.9 — generic types, chan_len/cap on any chan[T], prebuilt install, and next milestone 0.2.0 (not released).
- Add v0.1.10 milestone for deepening generics (multi-statement lambdas, &mut self) which block the v0.2.0 stdlib rewrite - Document all speed optimizations shipped (wyhash, stack f-strings, constant folding, zero-copy strings, select condvar, emit_line) - Document v0.1.9 residuals (enum variant collisions, error messages) - Add leba ecosystem section (v0.7.0 with admin UI, session auth, RBAC) - Honest status on what blocks v0.2.0
Multi-statement lambda bodies - Lambdas with let, assignments, if/else, while loops emit as full C function bodies instead of single return expressions - Enables real callbacks with control flow for stdlib work Mutable self on methods - fn m(mut self) passes receiver by pointer; mutations persist in caller - Enables real iterators (next_val advances state) and stateful methods - Field access on pointer self uses -> instead of . - Method call sites pass &receiver for mut self functions Generic enum variant disambiguation - Multiple instantiations (MyBox[int] + MyBox[string]) no longer collide - Qualified variant lookup (EnumName::Variant) in type checker and codegen - Return type context resolves correct instantiation for construct/match
io package (std/io) - StringReader with mut self read() advancing position - ByteWriter with mut self write() appending bytes - drain() reads StringReader to completion - 8 tests collections package (std/collections/stack.mko) - IntStack with push/pop/peek/size/clear using mut self - StrStack with push/pop/peek/size - IntQueue with enqueue/dequeue/peek_front, auto-compacting - All return Option for empty cases - 6 tests context package (std/context) - Context struct with deadline_ms and cancelled flag - background(), with_timeout(ms), with_deadline(ms), with_cancel() - done(), err(), remaining(), has_deadline() methods - 5 tests Parser fix: skip mut self in interface method declarations
encoding/json: ObjectBuilder/ArrayBuilder with set_string/set_int/ set_bool/set_null/set_raw, proper escaping, parser wrappers. 6 tests. net/http: Request/Response types, Router with method+path matching, text/json/html response constructors. 5 tests. database/sql: Pool with acquire/release slot management, begin_tx/commit_tx/rollback_tx transaction helpers. 2 tests. Fix: restore json.object_str compat wrapper for import_paths_test.
Fix ownership cleanup and native runtime packaging
Make native select differential deterministic
Two memory-safety bugs in owned slice fields (both surfaced by collections_mako_test on the native backend): - `self.xs = self.xs[a:b]` stored a view that aliases the field's own buffer, then freed that buffer while overwriting — use-after-free (garbage reads, SIGABRT). Clone the view's data into an independent slice before dropping the old field, matching the local-assignment path. - `self.xs = append(self.xs, v)` leaked the original field header/buffer. A field read is a borrow, so append clones it and consumes the clone; the stale "consumes old field" exception then skipped dropping the original. Always drop the old field. Affects every slice-backed struct (stacks, lists, queues). Native corpus now 376/376; every native fixture is 0-leak and GuardMalloc-clean.
Replace the terse ADAPTIVE_OPT.md draft with clear prose describing the offline feedback loop: a fully compiled binary plus opt-in hot-site counters (off by default, 256 sites) exported at /debug/hot_sites and folded into the next build via an offline PGO cycle. Frame it as ahead-of-time with an offline loop; drop the runtime-code-generation phrasing from the README compilation section.
Add versioned check diagnostics
GraphQL: replace the placeholder resolver with a real recursive executor
behind graphql_schema_resolve (both C and native backends, no ABI change):
nested projection over objects/arrays, aliases, arguments + operation
variable defaults, __typename, __schema/__type introspection, named and
inline fragments, field validation, and spec-shaped {data,errors} output.
Fix a pre-existing per-request leak: resolve results and owned query-arg
temporaries were never reclaimed (allowlist owned-string-returning builtins
in scope-drop cleanup; gate the query-arg free by ownership to avoid double
free). Verified leak-free per request and ASan/UBSan-clean on Linux; new
graphql_exec_test.mko (13 cases).
Native backend: List[T] now lowers for any element type (structs, nested
List[List[int]], strings, pointer arrays) by aliasing []T instead of erroring
for non-scalar T; new native_list_generic_test.mko, differential vs C.
Anneal: name the adaptive-optimization loop; add scripts/anneal-cycle.sh over
adaptive-opt-cycle.sh (stable hot_site_* API unchanged); broaden hot_site_test
with boundary ids, top reassignment, enable transitions, and concurrent
atomic-counter tallies.
Version: 0.4.15 -> 0.4.16; update CHANGELOG, docs, and tip markers.
A string/slice/map returned from a user-defined function and bound to a local was never freed when dropped (one allocation leaked per call — a per-request leak in long-running servers). Recognize such call results as owned producers in scope-drop cleanup so they free at scope exit; move analysis still suppresses the drop on return/pass-on/store. Enums are excluded (match consumes them — an unconditional free double-frees) and structs are excluded (field-by-field frees). Verified on Linux: full suite green under AddressSanitizer (no double-free/UAF), leak eliminated for []int / string / List[Struct] returns, generic-enum tests still pass. New owned_return_drop_test.mko.
…-0416 Real GraphQL executor, native List[T], Anneal; 0.4.16
…andles Add protocol-agnostic timer_heap and peer_table, TLS pool mTLS with caller timeouts, native SCTP transport, and optional Diameter as a protocol pack that composes the general primitives. Harden with generation-packed handles, stable timer cancel tokens, TLS pool inflight close, and fail-closed bounds.
…builtins) Re-applies the string-ownership leak fixes on top of the networking/ Diameter codegen refactor. - Free the right operand of `+` after concat when it is an unambiguously owned temporary (gated by expr_is_scope_drop_safe, so borrowed views — literals/index/method — are never freed; no double-free). - Recognize int_to_string / format_int / base64_encode / base32_encode / base64_decode / bytes_to_hex / csv_join_row / auth_bearer / auth_basic_header as owned producers so their results free instead of leaking per call. string(x) stays excluded (polymorphic). New concat_owned_operand_test.mko. Local: full suite 389/0.
|
Warning Review limit reached
Next review available in: 11 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughOwned string-producing builtins are classified during code generation, concatenation reclaims owned right-hand temporaries, and GraphQL request builders free escaped query buffers. Tests cover conversions, builders, chained concatenations, and dropped builtin results. ChangesOwned string reclamation
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
examples/testing/concat_owned_operand_test.mko (1)
33-37: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winCover borrowed index and method operands too.
This test covers owned producers and borrowed literals, but not the index/method borrow paths that must remain excluded from reclamation. Add regression cases for those operands, or verify equivalent existing tests, so an ownership-classification regression cannot cause a double-free or leak unnoticed by this suite.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@examples/testing/concat_owned_operand_test.mko` around lines 33 - 37, Extend TestConcatChain to exercise concatenation operands produced through borrowed index access and method calls, in addition to the existing owned producers and literals. Assert the resulting strings and ensure the cases cover the ownership paths that must remain excluded from reclamation; reuse equivalent existing regression tests if they already provide this coverage.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CHANGELOG.md`:
- Around line 56-60: Update the changelog entry’s “leaks eliminated” wording to
explicitly limit the claim to scope-drop leaks, while preserving the existing
validation details and separate builtin residual-leak note.
---
Nitpick comments:
In `@examples/testing/concat_owned_operand_test.mko`:
- Around line 33-37: Extend TestConcatChain to exercise concatenation operands
produced through borrowed index access and method calls, in addition to the
existing owned producers and literals. Assert the resulting strings and ensure
the cases cover the ownership paths that must remain excluded from reclamation;
reuse equivalent existing regression tests if they already provide this
coverage.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: e419e040-3d08-4cc0-b15c-3443d0ab3d39
📒 Files selected for processing (3)
CHANGELOG.mdexamples/testing/concat_owned_operand_test.mkosrc/codegen/mod.rs
Replace the hand-maintained per-name allowlist in expr_is_scope_drop_safe with builtin_returns_owned_string(name): a conservative classifier keyed on operation-family suffixes/prefixes (_encode, _decode, _to_string, _hash, _sign, _hex, _json, _sdl, format_*, base64_*, base32_*). Those operations produce fresh content that can never be a borrowed view, so their results are always safe to reclaim; any unclassified builtin stays not-owned (may leak, never double-free). Known view-returners (str_as_view, bytes_as_str, buf_to_string) and polymorphic string(x) are excluded. A new encode/format builtin is now covered by its family with no code change. Covers ~90+ owned-string builtins (was a hand-list of ~10). New owned_string_builtin_test.mko. Local: full suite 389/0.
…_response/_connect_url/_en|decode_string) Extend builtin_returns_owned_string with protocol/URL/request builder families whose operation serializes fresh content (never a view): _frame (http2/nats/quic/rpc), _connect_url (db drivers), _request / _response (dap/elastic/graphql/openapi), and _encode_string / _decode_string (avro/cbor/msgpack/gob) missed by the bare _encode/_decode suffixes. Accessor families (_header, _get_string, plain _string like buf_to_string) stay excluded. Local: full suite 390/0.
Both builders escaped the query into a temp (mako_json_escape always mallocs) but never freed it — one buffer leaked per call. Reclaim it. Verified CLEAN under LeakSanitizer. NOTE: the same unfreed-escape pattern exists across many json_object_str / json_si / … builders (~200 sites) — a separate systematic runtime-C audit.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
examples/testing/owned_string_builtin_test.mko (1)
41-46: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd regression coverage for excluded borrowed results.
The ownership classifier explicitly excludes view-returning builtins and polymorphic
string(x), but this file covers only owned-producing families. Add a small drop/churn test for those exclusions to prevent a future classifier change from reclaiming borrowed aliases.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@examples/testing/owned_string_builtin_test.mko` around lines 41 - 46, Add regression coverage in TestEncodeInConcat for excluded borrowed results by adding a small drop/churn scenario using view-returning builtins and polymorphic string(x). Verify the borrowed aliases remain valid after the churn, without changing the existing owned-concat assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@examples/testing/owned_string_builtin_test.mko`:
- Around line 41-45: Update TestEncodeInConcat assertions to verify the complete
expected encoded substrings, including the results of bytes_to_hex("x") and
base64_encode("y"), rather than only checking the "hex=" and "b64=" prefixes.
Preserve the existing concatenation and ownership-cleanup coverage.
---
Nitpick comments:
In `@examples/testing/owned_string_builtin_test.mko`:
- Around line 41-46: Add regression coverage in TestEncodeInConcat for excluded
borrowed results by adding a small drop/churn scenario using view-returning
builtins and polymorphic string(x). Verify the borrowed aliases remain valid
after the churn, without changing the existing owned-concat assertions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 0724065a-9808-4b46-8e48-e28f0209abc5
📒 Files selected for processing (4)
CHANGELOG.mdexamples/testing/owned_string_builtin_test.mkoruntime/mako_std.hsrc/codegen/mod.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- src/codegen/mod.rs
- CHANGELOG.md
Comment on lines
+41
to
+45
| fn TestEncodeInConcat() { | ||
| // Encode results as owned concat operands — reclaimed after concat. | ||
| let s = "hex=" + bytes_to_hex("x") + " b64=" + base64_encode("y") | ||
| assert(str_contains(s, "hex=")) | ||
| assert(str_contains(s, "b64=")) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert the encoded operand contents, not only the prefixes.
These assertions pass even if bytes_to_hex("x") or base64_encode("y") produces empty or corrupted data. Assert the expected encoded substrings as well, so this test validates concat correctness in addition to ownership cleanup.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@examples/testing/owned_string_builtin_test.mko` around lines 41 - 45, Update
TestEncodeInConcat assertions to verify the complete expected encoded
substrings, including the results of bytes_to_hex("x") and base64_encode("y"),
rather than only checking the "hex=" and "b64=" prefixes. Preserve the existing
concatenation and ownership-cleanup coverage.
~20 flat JSON builders (json_object_str/json_si/json_ss/json_i, LLM message builders, …) escaped args via mako_json_escape (always allocates) into a temp used only for snprintf, but never freed it — one buffer leaked per call on serialization hot paths. Reclaim the escape temps (applied by a conservative transform limited to single unguarded end-of-function returns with function-top-level escapes; loop-based and guarded-return builders left for a follow-up). Verified on Linux: json_object_str now CLEAN under LeakSanitizer; json/adapters/graphql suites pass under AddressSanitizer (no double-free).
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
3 participants
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.
Rebased onto the networking/Diameter main (5d23c9d). Closes the string-ownership leak family:
+after concat when it is an unambiguously owned temporary (gated by expr_is_scope_drop_safe — literal views / index / method borrows are never freed, so no double-free). Fixes"row-" + int_to_string(n)leaking one buffer per concat.Verification (Linux): concat churn leak-free under LeakSanitizer; full suite green under AddressSanitizer (389/0, no double-free/UAF, incl. the new Diameter/SCTP/timer/peer/TLS-pool tests). New concat_owned_operand_test.mko.
Supersedes #24 (which was based on pre-refactor main and conflicts). Residual: a few builtins still leak a temp inside their own C impl — separate runtime-level issue.
Summary by CodeRabbit