Fix runtime optimization correctness regressions#14
Conversation
|
Important Review skippedNo new commits to review since the last review. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe changes add fresh string-key detection for map assignment, expand ownership and integer-sorting tests, adjust introsort depth handling, preserve task results during thread joins, and update CI and changelog entries. ChangesMap key ownership
Integer sorting
Task-thread joining
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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] review |
|
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 @.github/workflows/ci.yml:
- Around line 374-379: Add read-only token permissions for the ASan job
containing “Check map key ownership” by setting its permissions to contents:
read. Keep the existing ASAN_OPTIONS, MAKO_TEST_TIMEOUT_SECS, and test command
unchanged.
In `@examples/testing/map_set_take_alias_test.mko`:
- Around line 10-18: Extend TestBorrowedStringArrayKeyAndValueAreCloned with an
assertion that items[2] remains "second" after the second map assignment, while
preserving the existing assertions.
In `@runtime/mako_rt.h`:
- Around line 6408-6411: Serialize calls to mako_task_join_thread so concurrent
mako_await and mako_await_timeout_ms callers cannot invoke pthread_join on the
same task. Add or reuse a per-task mutex/state that elects one joiner, makes
other callers wait for the join to complete, and only publishes joined after the
winner finishes; preserve the existing NULL retval behavior.
🪄 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
Run ID: fef569eb-e44e-4683-b5bd-053a14e407aa
📒 Files selected for processing (6)
.github/workflows/ci.ymlCHANGELOG.mdexamples/testing/map_set_take_alias_test.mkoexamples/testing/sort_test.mkoruntime/mako_rt.hsrc/codegen/mod.rs
| - name: Check map key ownership | ||
| env: | ||
| ASAN_OPTIONS: detect_leaks=0 | ||
| MAKO_TEST_TIMEOUT_SECS: "120" | ||
| run: | | ||
| ./target/release/mako test --sanitize address examples/testing/map_set_take_alias_test.mko |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the workflow around the ASan job and any permissions blocks.
file=".github/workflows/ci.yml"
echo "== workflow snippets =="
sed -n '330,420p' "$file"
echo
echo "== permissions mentions =="
grep -nE '^\s*permissions:|^\s*[A-Za-z0-9_-]+:\s*$|^\s*contents:\s*read|^\s*contents:\s*write|^\s*actions:\s*read|^\s*id-token:\s*' "$file" | sed -n '1,120p'Repository: loreste/mako
Length of output: 4436
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,40p' .github/workflows/ci.ymlRepository: loreste/mako
Length of output: 1112
Restrict the ASan job token to read-only contents. This job runs PR-controlled code and currently inherits the workflow/repository default GITHUB_TOKEN permissions; add permissions: contents: read at the job or workflow level.
🧰 Tools
🪛 zizmor (1.26.1)
[warning] 343-379: overly broad permissions (excessive-permissions): default permissions used due to no permissions: block
(excessive-permissions)
🤖 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 @.github/workflows/ci.yml around lines 374 - 379, Add read-only token
permissions for the ASan job containing “Check map key ownership” by setting its
permissions to contents: read. Keep the existing ASAN_OPTIONS,
MAKO_TEST_TIMEOUT_SECS, and test command unchanged.
Source: Linters/SAST tools
| fn TestBorrowedStringArrayKeyAndValueAreCloned() { | ||
| let items = ["key", "first", "second"] | ||
| let mut values = make(map[string]string) | ||
| values[items[0]] = items[1] | ||
| values[items[0]] = items[2] | ||
| assert_eq_str(values["key"], "second") | ||
| assert_eq_str(items[0], "key") | ||
| assert_eq_str(items[1], "first") | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert that the second borrowed value remains valid.
The test checks items[1] but never reads items[2] after the update assignment on Line 14. Add assert_eq_str(items[2], "second") to cover both borrowed-value assignments.
🤖 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/map_set_take_alias_test.mko` around lines 10 - 18, Extend
TestBorrowedStringArrayKeyAndValueAreCloned with an assertion that items[2]
remains "second" after the second map assignment, while preserving the existing
assertions.
| static inline void mako_task_join_thread(MakoTask *t) { | ||
| /* Keep retval NULL: the Windows pthread shim would replace t->result with NULL. */ | ||
| pthread_join(t->thread, NULL); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
# Inspect every native join and all uses of the completion flag.
rg -n -C 6 '\bpthread_join\s*\(' runtime/mako_rt.h
rg -n -C 10 '\bjoined\b' runtime/mako_rt.hRepository: loreste/mako
Length of output: 13198
Serialize the native thread join. mako_await and mako_await_timeout_ms both read joined before storing it, so two concurrent callers can enter pthread_join on the same task. That is undefined for pthreads and can double-close the Windows shim handle; guard the join with a separate in-progress state or mutex, and let losing callers wait for the winner to publish completion.
🤖 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 `@runtime/mako_rt.h` around lines 6408 - 6411, Serialize calls to
mako_task_join_thread so concurrent mako_await and mako_await_timeout_ms callers
cannot invoke pthread_join on the same task. Add or reuse a per-task mutex/state
that elects one joiner, makes other callers wait for the join to complete, and
only publishes joined after the winner finishes; preserve the existing NULL
retval behavior.
2 participants
Fixes correctness regressions introduced by the recent runtime optimizations.
set_takepathVerified with the Rust test suite, the sort and task-result suites on Windows, and the map-key regression under ASan.
@coderabbitai[bot] review
Summary by CodeRabbit
Performance
Bug Fixes
Tests