Work74#1585
Conversation
Signed-off-by: Seonghyun Kim <sh8281.kim@samsung.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Seonghyun Kim <sh8281.kim@samsung.com>
…rashes Samsung#3-13) Issue Samsung#1571 Crashes Samsung#3-4: Labeled continue in for loops - Consume labeled continues targeting this loop with proper morphing - Ensures iterator cleanup and environment unwinding work correctly Issue Samsung#1571 Crashes Samsung#5-13: Environment record mismatch in labeled loops - Proper morphing of labeled continues across allocated block boundaries - Fixes crashes from scope-creating constructs in labeled loops - Plain Jump path preserved for non-allocated blocks (zero overhead) Solution: Call consumeLabelledContinuePositions with morphing enabled - If no allocated block: plain Jump (fast path) - If allocated block: JumpComplexCase with proper unwinding (correct path) - Morphing is automatic via morphJumpPositionIntoComplexCase Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Seonghyun Kim <sh8281.kim@samsung.com>
… Crashes Samsung#3-4) Issue Samsung#1571 Crash Samsung#3: Labeled continue in for-of loop - Iterator value issue when labeled continue triggered early - Proper sequencing of iterator cleanup vs control flow Issue Samsung#1571 Crash Samsung#4: With statement + labeled for-of - Environment unwinding coordination with iterator cleanup - CloseLexicalEnvironment called at correct time Solution: Consume labeled continues with proper morphing - Ensures iterator cleanup finalizer runs before unwinding - Control flow record management stays consistent - Both for-in and for-of (and for-await-of) properly handled Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Seonghyun Kim <sh8281.kim@samsung.com>
Issue Samsung#1571: Labeled continue in while loops with allocated blocks - Proper morphing for labeled continues crossing block boundaries - Fixes environment record consistency in labeled loops - Applies fix pattern to all loop statement types Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Seonghyun Kim <sh8281.kim@samsung.com>
Issue Samsung#1571: Labeled continue in do-while loops with allocated blocks - Proper morphing for labeled continues crossing block boundaries - Fixes environment record consistency in labeled loops - Completes fix pattern across all loop statement types Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Seonghyun Kim <sh8281.kim@samsung.com>
- Add m_currentLoopLabel to ByteCodeGenerateContext for tracking labeled loop labels (Issue Samsung#1571) - Fix Crash Samsung#1: Add bounds checking in inline cache proto traverse with std::min clamping - Fix Crash Samsung#2: Check hasBinding before initializeBinding to prevent assertion on unreachable code paths Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com> Signed-off-by: Seonghyun Kim <sh8281.kim@samsung.com>
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| if (isLexicallyDeclaredName) { | ||
| state.lexicalEnvironment()->record()->initializeBinding(state, name, value); | ||
| auto result = state.lexicalEnvironment()->record()->hasBinding(state, name); | ||
| if (result.m_index != SIZE_MAX) { |
There was a problem hiding this comment.
The if (result.m_index != SIZE_MAX) check may skip calling initializeBinding when the binding does not exist, causing initializeByName to silently fail to create a new binding. This changes the intended semantics. Remove the check or ensure a binding is created when missing.
|
|
||
| auto& newItem = inlineCache->m_cache[0]; | ||
| code->m_inlineCacheProtoTraverseMaxIndex = std::max(cachedhiddenClassChain.size() - 1, (size_t)code->m_inlineCacheProtoTraverseMaxIndex); | ||
| size_t newProtoTraverseIndex = std::min(cachedhiddenClassChain.size() - 1, SetObjectPreComputedCase::inlineCacheProtoTraverseMaxCount - 1); |
There was a problem hiding this comment.
The expression cachedhiddenClassChain.size() - 1 can underflow when the chain is empty, producing a huge size_t. This value is then used in std::max, potentially setting code->m_inlineCacheProtoTraverseMaxIndex to an invalid large value and leading to out‑of‑bounds accesses later. Add a check that cachedhiddenClassChain.size() > 0 before subtracting, or use a conditional expression to clamp the index to zero when the chain is empty.
| } | ||
| } | ||
| } else if (setObjectPreComputedCaseOperationSlowCase(state, originalObject, willBeObject, value, code, block)) { | ||
| } else if (code->m_inlineCacheProtoTraverseMaxIndex > 0 && code->m_inlineCacheProtoTraverseMaxIndex < SetObjectPreComputedCase::inlineCacheProtoTraverseMaxCount && setObjectPreComputedCaseOperationSlowCase(state, originalObject, willBeObject, value, code, block)) { |
There was a problem hiding this comment.
Extract the range check on code->m_inlineCacheProtoTraverseMaxIndex into a local boolean variable before the else-if. This reduces the complexity of the compound condition, avoids repeated evaluation of the same member, and makes the intent clearer. The new guard clause would look like bool isIndexInRange = code->m_inlineCacheProtoTraverseMaxIndex > 0 && code->m_inlineCacheProtoTraverseMaxIndex < SetObjectPreComputedCase::inlineCacheProtoTraverseMaxCount; followed by } else if (isIndexInRange && setObjectPreComputedCaseOperationSlowCase(...)) {. This keeps semantics identical while improving readability.
| } | ||
| } | ||
| code->m_inlineCacheProtoTraverseMaxIndex = std::max(cachedhiddenClassChain.size() - 1, (size_t)code->m_inlineCacheProtoTraverseMaxIndex); | ||
| size_t newProtoTraverseIndex = std::min(cachedhiddenClassChain.size() - 1, SetObjectPreComputedCase::inlineCacheProtoTraverseMaxCount - 1); |
There was a problem hiding this comment.
The calculation of newProtoTraverseIndex uses cachedhiddenClassChain.size() - 1 without guarding against an empty vector. If cachedhiddenClassChain.size() is zero, the subtraction underflows to a very large size_t, which then propagates through the subsequent std::max and can set code->m_inlineCacheProtoTraverseMaxIndex to an out‑of‑range value. Add a check that cachedhiddenClassChain.size() > 0 before subtracting, or use a conditional expression to clamp the index to zero when the vector is empty.
Remove the conditional labeled continue processing from loop statements. The LabelledStatementNode correctly handles all labeled continues after the labeled statement completes. Loops should only handle their own regular (unlabeled) continues. Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com> Signed-off-by: Seonghyun Kim <sh8281.kim@samsung.com>
|
|
||
| // Consume labeled continues targeting THIS loop with proper morphing (Issue #1571) | ||
| // This ensures continues inside allocated blocks are properly unwound via JumpComplexCase | ||
| if (context->m_currentLoopLabel) { |
There was a problem hiding this comment.
Replace the multi-line if block with a single-line if to reduce nesting. Remove the opening and closing braces and keep the call on the same line: if (context->m_currentLoopLabel) newContext.consumeLabelledContinuePositions(...);. This keeps the logic unchanged while making the code more concise and easier to read. The change is localized and does not affect surrounding logic.
| newContext.consumeContinuePositions(codeBlock, updatePosition, newContext.tryCatchWithBlockStatementCount()); | ||
| newContext.m_positionToContinue = updatePosition; | ||
|
|
||
| // Consume labeled continues targeting THIS loop with proper morphing (Issue #1571) |
There was a problem hiding this comment.
Merge the two consecutive comment lines into a single, concise comment. For example: // Consume labeled continues targeting THIS loop with proper morphing via JumpComplexCase (Issue #1571). This reduces comment clutter and keeps the intent clear. The change is purely cosmetic and does not alter behavior.
| } | ||
| } | ||
| ASSERT_NOT_REACHED(); | ||
| // If binding not found, it may be unreachable code after continue statement |
There was a problem hiding this comment.
The comment on line 12 states that the code may be "unreachable" after a continue statement, which is misleading because the loop can legitimately finish without finding a binding. This misstatement could confuse future maintainers about the control flow. Removing or replacing the comment with a clearer explanation—e.g., "If the binding is not found, the function silently returns without asserting"—would better reflect the actual behavior. Keeping the comment accurate improves readability and reduces the risk of misinterpretation. The change is localized to the added comment and does not affect runtime logic.
| // block environment un-popped (Issue #1571), so the comparison must be `>=`. | ||
| for (unsigned i = 0; i < m_breakStatementPositions.size(); i++) { | ||
| if (m_breakStatementPositions[i] > (unsigned long)frontlimit && m_complexCaseStatementPositions.find(m_breakStatementPositions[i]) == m_complexCaseStatementPositions.end()) { | ||
| if (m_breakStatementPositions[i] >= (unsigned long)frontlimit && m_complexCaseStatementPositions.find(m_breakStatementPositions[i]) == m_complexCaseStatementPositions.end()) { |
There was a problem hiding this comment.
The comparison uses (unsigned long)frontlimit, which truncates frontlimit on 64‑bit Linux where size_t is 64‑bit and unsigned long is 32‑bit. If frontlimit exceeds 4 GB, the comparison becomes incorrect and some jumps may be missed. Remove the cast or cast to size_t instead to preserve the full value.
|
|
||
| for (unsigned i = 0; i < m_continueStatementPositions.size(); i++) { | ||
| if (m_continueStatementPositions[i] > (unsigned long)frontlimit && m_complexCaseStatementPositions.find(m_continueStatementPositions[i]) == m_complexCaseStatementPositions.end()) { | ||
| if (m_continueStatementPositions[i] >= (unsigned long)frontlimit && m_complexCaseStatementPositions.find(m_continueStatementPositions[i]) == m_complexCaseStatementPositions.end()) { |
There was a problem hiding this comment.
Replace the index-based for loops with range-based for loops to improve readability and reduce the chance of off-by-one errors. For example, iterate over m_continueStatementPositions directly: for (auto pos : m_continueStatementPositions) { if (pos >= frontlimit && m_complexCaseStatementPositions.find(pos) == m_complexCaseStatementPositions.end()) { ... } } This keeps the same semantics but eliminates the manual index management. The change is localized to the body of registerJumpPositionsToComplexCase and does not affect other parts of the code.
…Samsung#1571) A continue/break that is the first instruction inside a lexical block which allocates an environment (e.g. `for(;c;){ continue; eval(); const x=1; }`) was emitted as a plain Jump, skipping the block's CloseLexicalEnvironment. The leaked environment then caused a subsequent outer-scope `const` to initialize in the wrong environment, producing a spurious `ReferenceError: Cannot access '...' before initialization`. registerJumpPositionsToComplexCase compared jump positions against frontlimit (= lexicalBlockStartPosition, the first body instruction) with strict `>`, so a jump located exactly at the first body instruction was never morphed into a JumpComplexCase and the block environment was left un-popped. Use `>=` for break/continue/labelledBreak/labelledContinue. With the environment now unwound correctly, the hasBinding guard band-aid in InterpreterSlowPath::initializeByName is no longer needed and is removed. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Seonghyun Kim <sh8281.kim@samsung.com>
2 participants
No description provided.