FunctionHasExecutedCache: expose function names in getFunctionRanges#178
Open
bilby91 wants to merge 4 commits intooven-sh:mainfrom
Open
FunctionHasExecutedCache: expose function names in getFunctionRanges#178bilby91 wants to merge 4 commits intooven-sh:mainfrom
bilby91 wants to merge 4 commits intooven-sh:mainfrom
Conversation
Extend FunctionHasExecutedCache to store and return function names alongside execution status and byte ranges. This enables coverage tools to report per-function data (FN/FNDA records in LCOV) using real function names from JSC rather than heuristic source text parsing. Changes: - Add RangeValue struct holding hasExecuted bool + function name String - Change RangeMap value type from bool to RangeValue - Extend insertUnexecutedRange() to accept an optional function name - Extend getFunctionRanges() return type to include the name - Update all call sites to pass UnlinkedFunctionExecutable::ecmaName() - Add $vm.getFunctionRanges() test helper in JSDollarVM - Add JSTests/controlFlowProfiler/function-names.js test Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Author
|
I haven't submitted the PR on the |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughPer-range execution entries now include function names alongside execution flags. Call sites were updated to supply ECMA names when inserting unexecuted ranges. A Changes
🚥 Pre-merge checks | ✅ 1 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@JSTests/controlFlowProfiler/function-names.js`:
- Around line 36-42: Add the missing assertions for arrowFuncRange: verify its
execution status with an assertion on arrowFuncRange.hasExecuted (match the
expected execution like namedFunction, e.g., === true) and verify the range
validity with an assertion that arrowFuncRange.start < arrowFuncRange.end; place
these alongside the existing checks for namedFunctionRange and
anotherFunctionRange so arrowFuncRange has the same coverage.
In `@Source/JavaScriptCore/tools/JSDollarVM.cpp`:
- Around line 3675-3683: The loop that builds entries for functionRanges must
check for exceptions after calling result->putDirectIndex(globalObject, i,
entry) because putDirectIndex can throw (e.g., allocation during array growth);
update the for-loop in JSDollarVM.cpp to test vm->exception() (or the
appropriate exception-check API used elsewhere in this file) immediately after
each putDirectIndex call and return/propagate the exception or break out as the
surrounding function's error-handling convention requires, keeping all other
entry construction (JSFinalObject::create, putDirect for fields, and use of
result and functionRanges) unchanged.
- Around line 3666-3668: The code assumes a callable is a JSFunction and
directly calls jsExecutable() on the result of jsDynamicCast<JSFunction*>, which
can be null for non-JSFunction callables; modify the logic around JSValue
functionValue / jsDynamicCast<JSFunction*> to null-check the result of
jsDynamicCast<JSFunction*> before calling ->jsExecutable(), and handle the
nullptr case the same way as similar checks in this file (e.g., return an
appropriate exception or take the non-JSFunction path as done near lines
2541-2544), referencing the symbols JSValue, jsDynamicCast<JSFunction*>, and
jsExecutable() to locate and update the code.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 08ef7d16-cf5c-49ba-a913-a582cabc9bf6
📒 Files selected for processing (8)
JSTests/controlFlowProfiler/function-names.jsSource/JavaScriptCore/bytecode/CodeBlock.cppSource/JavaScriptCore/runtime/ControlFlowProfiler.cppSource/JavaScriptCore/runtime/FunctionHasExecutedCache.cppSource/JavaScriptCore/runtime/FunctionHasExecutedCache.hSource/JavaScriptCore/runtime/JSModuleRecord.cppSource/JavaScriptCore/runtime/ProgramExecutable.cppSource/JavaScriptCore/tools/JSDollarVM.cpp
- Add null check for jsDynamicCast<JSFunction*> in $vm.getFunctionRanges to handle non-JSFunction callables (BoundFunction, Proxy, etc.) - Add RETURN_IF_EXCEPTION after putDirectIndex in the loop - Add missing arrowFuncRange assertions in function-names.js test Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Source/JavaScriptCore/tools/JSDollarVM.cpp`:
- Around line 3679-3687: The loop repeatedly calls
JSFinalObject::createStructure(vm, globalObject,
globalObject->objectPrototype(), 4) which allocates a new Structure each
iteration; move that call out of the loop into a local Structure* (e.g.
reuseStructure) before iterating over functionRanges and use
JSFinalObject::create(vm, reuseStructure) inside the loop to reduce allocation
churn; keep existing uses of entry, functionRanges, result->putDirectIndex(...)
and RETURN_IF_EXCEPTION(...) unchanged except for replacing the inline
createStructure call with the reused Structure*.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 06b08c67-9f8c-402e-a16f-f9c2cdcce6f3
📒 Files selected for processing (2)
JSTests/controlFlowProfiler/function-names.jsSource/JavaScriptCore/tools/JSDollarVM.cpp
The entry Structure is identical for every iteration, so create it once before the loop to reduce allocation churn. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Source/JavaScriptCore/tools/JSDollarVM.cpp`:
- Around line 3658-3659: Update the comment for $vm.getFunctionRanges to match
the actual emitted object field order: currently the code emits objects in the
order hasExecuted, start, end, name, so change the documented shape from "{
name, hasExecuted, start, end }" to "{ hasExecuted, start, end, name }" (or
reorder the emitted properties in the function to match the comment) — locate
the comment near the $vm.getFunctionRanges implementation in JSDollarVM.cpp and
make the comment and emitted field order consistent.
- Around line 3669-3673: The code calls JSFunction::jsExecutable() unguarded,
which asserts for host/native functions; change it to retrieve the generic
Executable (via function->executable() or equivalent non-asserting accessor) and
then perform a dynamic cast to FunctionExecutable* using
jsDynamicCast<FunctionExecutable*>(...), and if the cast returns null call
throwVMTypeError(globalObject, scope, "Expected argument to be a source
JSFunction"_s) to reject non-source (host) functions before proceeding.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: dd1e1b70-4c20-4ca3-a749-dd553475a461
📒 Files selected for processing (1)
Source/JavaScriptCore/tools/JSDollarVM.cpp
- Use jsDynamicCast<FunctionExecutable*>(function->executable()) instead of function->jsExecutable() to avoid assert on host/native functions - Fix comment to match actual field emission order: hasExecuted, start, end, name Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.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
1 participant
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.
Summary
FunctionHasExecutedCacheto store and return function names alongside execution status and byte rangesRangeValuestruct holdinghasExecutedbool + function nameString, replacing the bareboolinRangeMapinsertUnexecutedRange()to accept an optional function name parameter (default empty, backward compatible)getFunctionRanges()return type to include the function name as a 4th tuple elementCodeBlock.cpp,ProgramExecutable.cpp,JSModuleRecord.cpp) to passUnlinkedFunctionExecutable::ecmaName()$vm.getFunctionRanges()test helper inJSDollarVM.cppJSTests/controlFlowProfiler/function-names.jstestMotivation
Coverage tools (e.g., Bun's LCOV reporter) need per-function names to emit standard
FN:andFNDA:records. Currently,getFunctionRanges()only returns(hasExecuted, startOffset, endOffset), forcing consumers to parse function names from source text heuristically. SinceUnlinkedFunctionExecutable::ecmaName()is already available at allinsertUnexecutedRange()call sites, this is a minimal change to expose it through the existing API.Test plan
JSTests/controlFlowProfiler/tests pass (8/8)function-names.jstest verifies named functions, arrow functions, and execution status🤖 Generated with Claude Code