Feature parity between bytecode mode and interpreter mode#39
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughRenames function prototypes to templates, expands Souffle VM value/opcode/runtime surface (typed locals, blueprints, arrays/records, inline strings, ExtendedOperation), refactors the Goccia compiler into context-driven units with constant folding, adds JSX transform + source maps to loaders, and splits compile/execute timing. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI/Runner
participant Loader as ScriptLoader/BenchmarkRunner/TestRunner
participant JSX as TGocciaJSXTransformer
participant Parser as Parser
participant Compiler as TGocciaCompiler (Context)
participant Bytecode as TSouffleBytecodeModule
participant VM as TSouffleVM
participant RuntimeOps as TSouffleRuntimeOperations
CLI->>Loader: request load/benchmark/test run
Loader->>JSX: (if ggJSX) transform SourceText -> JSXResult + SourceMap
JSX-->>Loader: JSXResult + SourceMap
Loader->>Parser: lex/parse using transformed SourceText
Parser-->>Loader: AST + Warnings
Loader->>Compiler: BuildContext(AST, SourcePath, Scope)
Compiler->>Compiler: constant-fold, emit expressions & statements (templates, local-type hints)
Compiler->>Bytecode: produce TSouffleBytecodeModule (function templates)
CLI->>VM: execute Bytecode (Compile/Exec timing)
VM->>RuntimeOps: call RT hooks / ExtendedOperation as needed
RuntimeOps-->>VM: runtime-resolved values (promises, iterators, globals)
VM-->>CLI: result + timing metrics
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
Contributor
Benchmark Results252 benchmarks · 🟢 188 improved · 🔴 9 regressed · 55 unchanged · avg +17.8% arraybuffer.js — 🟢 1 improved, 🔴 6 regressed, 7 unchanged · avg -4.6%
arrays.js — 🟢 2 improved, 🔴 2 regressed, 15 unchanged · avg +0.6%
async-await.js — 🟢 4 improved, 2 unchanged · avg +5.9%
classes.js — 🟢 9 improved, 22 unchanged · avg +4.5%
closures.js — 🟢 11 improved · avg +24.4%
collections.js — 🟢 9 improved, 3 unchanged · avg +15.2%
destructuring.js — 🟢 22 improved · avg +29.2%
fibonacci.js — 🟢 6 improved · avg +19.9%
for-of.js — 🟢 7 improved · avg +32.0%
iterators.js — 🟢 20 improved · avg +29.0%
json.js — 🟢 18 improved, 2 unchanged · avg +17.7%
jsx.jsx — 🟢 19 improved, 2 unchanged · avg +16.2%
numbers.js — 🟢 10 improved, 1 unchanged · avg +12.8%
objects.js — 🟢 7 improved · avg +20.3%
promises.js — 🟢 12 improved · avg +23.0%
strings.js — 🟢 11 improved · avg +26.4%
typed-arrays.js — 🟢 20 improved, 🔴 1 regressed, 1 unchanged · avg +36.2%
Measured on ubuntu-latest x64. Changes within ±7% are considered insignificant. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 20
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
units/Goccia.Builtins.TestAssertions.pas (1)
167-173:⚠️ Potential issue | 🟠 MajorPin stored suite/test callbacks as GC roots.
SuiteFunctionandTestFunctionare retained in Pascal-owned objects and invoked later (e.g., Line 1602 and Line 1643). Without rooting on store/releasing on cleanup, they can be collected before execution.As per coding guidelines "Values held only by Pascal code (not in any GocciaScript scope) must be protected with AddTempRoot/RemoveTempRoot for the duration they are needed."Proposed fix
type TGocciaTestSuite = class public Name: string; SuiteFunction: TGocciaFunctionBase; IsSkipped: Boolean; constructor Create(const AName: string; const ASuiteFunction: TGocciaFunctionBase; const AIsSkipped: Boolean = False); + destructor Destroy; override; end; TGocciaTestCase = class public Name: string; TestFunction: TGocciaFunctionBase; SuiteName: string; IsSkipped: Boolean; constructor Create(const AName: string; const ATestFunction: TGocciaFunctionBase; const ASuiteName: string; const AIsSkipped: Boolean = False); + destructor Destroy; override; end; constructor TGocciaTestSuite.Create(const AName: string; const ASuiteFunction: TGocciaFunctionBase; const AIsSkipped: Boolean = False); begin inherited Create; Name := AName; SuiteFunction := ASuiteFunction; + if Assigned(TGocciaGarbageCollector.Instance) and Assigned(SuiteFunction) then + TGocciaGarbageCollector.Instance.AddTempRoot(SuiteFunction); IsSkipped := AIsSkipped; end; +destructor TGocciaTestSuite.Destroy; +begin + if Assigned(TGocciaGarbageCollector.Instance) and Assigned(SuiteFunction) then + TGocciaGarbageCollector.Instance.RemoveTempRoot(SuiteFunction); + inherited; +end; + constructor TGocciaTestCase.Create(const AName: string; const ATestFunction: TGocciaFunctionBase; const ASuiteName: string; const AIsSkipped: Boolean = False); begin inherited Create; Name := AName; TestFunction := ATestFunction; + if Assigned(TGocciaGarbageCollector.Instance) and Assigned(TestFunction) then + TGocciaGarbageCollector.Instance.AddTempRoot(TestFunction); SuiteName := ASuiteName; IsSkipped := AIsSkipped; end; + +destructor TGocciaTestCase.Destroy; +begin + if Assigned(TGocciaGarbageCollector.Instance) and Assigned(TestFunction) then + TGocciaGarbageCollector.Instance.RemoveTempRoot(TestFunction); + inherited; +end;Also applies to: 177-184
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@units/Goccia.Builtins.TestAssertions.pas` around lines 167 - 173, TGocciaTestSuite currently stores SuiteFunction (and similarly TestFunction in the test-case constructor) in Pascal-owned objects without pinning them as GC roots, so the callbacks can be garbage-collected before they are invoked; update the constructors (e.g., TGocciaTestSuite.Create and the analogous TGocciaTestCase constructor around lines 177-184) to call AddTempRoot on SuiteFunction/TestFunction after assignment and ensure the destructor or cleanup removes those roots with RemoveTempRoot, keeping the unique identifiers SuiteFunction and TestFunction as the values you pin/unpin.units/Goccia.Builtins.Benchmark.pas (1)
120-129:⚠️ Potential issue | 🟠 MajorRoot benchmark callbacks at registration time.
TBenchmarkCasestores function values beyond immediate call scope. Rooting only insideRunBenchmarksis late; callbacks can be collected before execution.As per coding guidelines "Values held only by Pascal code (not in any GocciaScript scope) must be protected with AddTempRoot/RemoveTempRoot for the duration they are needed."Proposed fix
type TBenchmarkCase = class public Name: string; SuiteName: string; SetupFunction: TGocciaFunctionBase; RunFunction: TGocciaFunctionBase; TeardownFunction: TGocciaFunctionBase; constructor Create(const AName: string; const ARunFunction: TGocciaFunctionBase; const ASuiteName: string; const ASetupFunction: TGocciaFunctionBase = nil; const ATeardownFunction: TGocciaFunctionBase = nil); + destructor Destroy; override; end; constructor TBenchmarkCase.Create(const AName: string; const ARunFunction: TGocciaFunctionBase; const ASuiteName: string; const ASetupFunction: TGocciaFunctionBase; const ATeardownFunction: TGocciaFunctionBase); begin inherited Create; Name := AName; RunFunction := ARunFunction; SuiteName := ASuiteName; SetupFunction := ASetupFunction; TeardownFunction := ATeardownFunction; + if Assigned(TGocciaGarbageCollector.Instance) then + begin + if Assigned(RunFunction) then TGocciaGarbageCollector.Instance.AddTempRoot(RunFunction); + if Assigned(SetupFunction) then TGocciaGarbageCollector.Instance.AddTempRoot(SetupFunction); + if Assigned(TeardownFunction) then TGocciaGarbageCollector.Instance.AddTempRoot(TeardownFunction); + end; end; + +destructor TBenchmarkCase.Destroy; +begin + if Assigned(TGocciaGarbageCollector.Instance) then + begin + if Assigned(RunFunction) then TGocciaGarbageCollector.Instance.RemoveTempRoot(RunFunction); + if Assigned(SetupFunction) then TGocciaGarbageCollector.Instance.RemoveTempRoot(SetupFunction); + if Assigned(TeardownFunction) then TGocciaGarbageCollector.Instance.RemoveTempRoot(TeardownFunction); + end; + inherited; +end;Also applies to: 216-216
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@units/Goccia.Builtins.Benchmark.pas` around lines 120 - 129, TBenchmarkCase stores GocciaScript function values (RunFunction, SetupFunction, TeardownFunction) at construction time which can be garbage-collected before RunBenchmarks executes; modify TBenchmarkCase.Create to root these function values with AddTempRoot when assigned (and ensure corresponding RemoveTempRoot is called when the TBenchmarkCase is destroyed or when those callbacks are no longer needed), and apply the same rooting pattern to the other constructor/assignment at the location referenced (~line 216) so all callback fields are protected while held by Pascal code.TestRunner.dpr (1)
295-302:⚠️ Potential issue | 🟡 Minor
TotalCompileNanosecondsis never populated from file results.The
TotalCompileNanosecondsfield is initialized to 0 at line 295, but it's never assigned fromFileResult. The accumulation at line 356 will always add 0. Either remove this dead field or add the missing assignment:🐛 Proposed fix to populate TotalCompileNanoseconds
Result.TotalLexNanoseconds := FileResult.Timing.LexTimeNanoseconds; Result.TotalParseNanoseconds := FileResult.Timing.ParseTimeNanoseconds; + Result.TotalCompileNanoseconds := FileResult.TotalCompileNanoseconds; Result.TotalExecNanoseconds := FileResult.Timing.ExecuteTimeNanoseconds;Note: This requires
TAggregatedTestResultreturned fromRunScriptFromFileto also haveTotalCompileNanosecondspopulated, which would need to come from a field onTTestFileResultorTGocciaScriptResult.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@TestRunner.dpr` around lines 295 - 302, Result.TotalCompileNanoseconds is initialized but never populated from FileResult so accumulation remains zero; either remove the dead TotalCompileNanoseconds field from TAggregatedTestResult (and all callers) or populate it by assigning Result.TotalCompileNanoseconds := FileResult.Timing.CompileTimeNanoseconds (or the appropriate field on TTestFileResult/TGocciaScriptResult) right after obtaining FileResult in RunGocciaScript, and ensure the upstream RunScriptFromFile/TAggregatedTestResult contract is updated so FileResult carries the compile timing.
🟡 Minor comments (5)
units/Goccia.Builtins.TestAssertions.pas-1567-1574 (1)
1567-1574:⚠️ Potential issue | 🟡 Minor
RunTestsno longer honors the legacysilentoptions key.Line 1572 only checks
showTestResults; existing test callers still passsilent, so output suppression no longer takes effect.Compatibility-friendly tweak
- Val := Param.GetProperty('showTestResults'); - if Assigned(Val) and not (Val is TGocciaUndefinedLiteralValue) then - ShowTestResults := Val.ToBooleanLiteral.Value - else - ShowTestResults := True; + Val := Param.GetProperty('showTestResults'); + if Assigned(Val) and not (Val is TGocciaUndefinedLiteralValue) then + ShowTestResults := Val.ToBooleanLiteral.Value + else + begin + Val := Param.GetProperty('silent'); + if Assigned(Val) and not (Val is TGocciaUndefinedLiteralValue) then + ShowTestResults := not Val.ToBooleanLiteral.Value + else + ShowTestResults := True; + end;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@units/Goccia.Builtins.TestAssertions.pas` around lines 1567 - 1574, The code sets ShowTestResults only from Param.GetProperty('showTestResults') and ignores the legacy 'silent' option; update the block that assigns ShowTestResults (the code using Param.GetProperty, TGocciaUndefinedLiteralValue and ToBooleanLiteral) to fall back to checking Param.GetProperty('silent') when 'showTestResults' is not present: if 'showTestResults' exists use its boolean value, else if 'silent' exists set ShowTestResults := not silent.ToBooleanLiteral.Value, otherwise leave the current default; keep the existing ExitOnFirstFailure logic unchanged.units/Goccia.Compiler.Context.pas-113-120 (1)
113-120:⚠️ Potential issue | 🟡 Minor
Assertdisabled in release builds — offset overflow may go undetected.The
Assert(Offset <= 255, ...)on line 118 will not execute in release builds ({$C-}). If the nil jump offset exceeds 8 bits, the code will silently truncate the offset viaUInt8(Offset), causing incorrect jump targets.🐛 Proposed fix to use runtime check
else if (TSouffleOpCode(Op) = OP_JUMP_IF_NIL) or (TSouffleOpCode(Op) = OP_JUMP_IF_NOT_NIL) then begin A := DecodeA(Instruction); B := DecodeB(Instruction); - Assert(Offset <= 255, 'Nil jump offset exceeds 8-bit range'); + if Offset > 255 then + raise Exception.Create('Nil jump offset exceeds 8-bit range'); ACtx.Template.PatchInstruction(AIndex, EncodeABC(TSouffleOpCode(Op), A, B, UInt8(Offset))); end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@units/Goccia.Compiler.Context.pas` around lines 113 - 120, The Assert(Offset <= 255, ...) is compiled out in releases so replace it with a runtime check in the OP_JUMP_IF_NIL / OP_JUMP_IF_NOT_NIL handling: test if Offset > 255 and raise a meaningful exception or call the existing error/reporting routine (so overflow cannot silently truncate) before calling ACtx.Template.PatchInstruction with EncodeABC; reference the TSouffleOpCode(Op) checks, the Offset variable, EncodeABC, UInt8(Offset) and ACtx.Template.PatchInstruction when making the change.souffle/Souffle.Value.pas-197-198 (1)
197-198:⚠️ Potential issue | 🟡 Minor
Math.IsNaNusage may cause AArch64 issues.The coding guidelines state: "NaN handling in the Souffle layer must use raw IEEE 754 bit-pattern checks (FloatBitsAreNaN), not FPC's Math.IsNaN, to avoid language-runtime dependencies and AArch64 pitfalls."
SouffleIsTrueat line 197 usesIsNaN(AValue.AsFloat)from theMathunit. Consider using a localFloatBitsAreNaNhelper consistent with Souffle.Bytecode.Chunk.pas.🐛 Proposed fix to use bit-pattern NaN check
Add to implementation section:
function FloatBitsAreNaN(const AValue: Double): Boolean; inline; var Bits: UInt64; begin Move(AValue, Bits, SizeOf(Double)); Result := ((Bits and $7FF0000000000000) = $7FF0000000000000) and ((Bits and $000FFFFFFFFFFFFF) <> 0); end;Then update line 197:
svkFloat: - Result := (AValue.AsFloat <> 0.0) and not IsNaN(AValue.AsFloat); + Result := (AValue.AsFloat <> 0.0) and not FloatBitsAreNaN(AValue.AsFloat);Also update
SouffleValueToString(lines 277-278):- if IsNaN(AValue.AsFloat) then + if FloatBitsAreNaN(AValue.AsFloat) then Result := 'NaN'As per coding guidelines: "NaN handling in the Souffle layer must use raw IEEE 754 bit-pattern checks (FloatBitsAreNaN), not FPC's Math.IsNaN."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@souffle/Souffle.Value.pas` around lines 197 - 198, SouffleIsTrue currently calls Math.IsNaN(AValue.AsFloat) which violates the Souffle layer guideline; add a local inline helper function FloatBitsAreNaN(const AValue: Double): Boolean that inspects the IEEE-754 bit pattern (move the double into a UInt64 and test exponent == $7FF and mantissa != 0) and replace IsNaN(AValue.AsFloat) with FloatBitsAreNaN(AValue.AsFloat); also update SouffleValueToString to use FloatBitsAreNaN when classifying NaN/Infinity instead of Math.IsNaN so all NaN handling in this unit uses the bit-pattern check consistent with Souffle.Bytecode.Chunk.pas.docs/souffle-vm.md-1110-1110 (1)
1110-1110:⚠️ Potential issue | 🟡 MinorOpcode counts in the cleanup summary look stale.
The “Tier 1 has 44 / Tier 2 has 44+1” statement no longer matches the opcode table in the current bytecode definition and should be updated to prevent confusion.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/souffle-vm.md` at line 1110, The opcode counts in the cleanup summary are outdated: update the sentence that reads “Tier 1 has 44 language-agnostic opcodes, Tier 2 has 44 runtime-dispatched opcodes + 1 extension opcode… the abstract interface has 45 generic methods + 1 extension entry point” to reflect the current bytecode definition/opcode table; inspect the opcode table in the bytecode definition to obtain the correct counts for Tier 1, Tier 2 (including extension opcodes), and the abstract interface method count, then replace the stale numbers and ensure the mention of the souffle/ directory and absence of Goccia.* imports remains accurate.docs/souffle-vm.md-505-505 (1)
505-505:⚠️ Potential issue | 🟡 MinorAdd a language hint to this fenced block.
This code fence is missing a language identifier, which triggers markdownlint MD040.
Suggested doc fix
-``` +```text TSouffleArray → WASM array type (array.new, array.get, array.set) TSouffleRecord → WASM struct type (struct.new, struct.get, struct.set) For dynamic records: WASM GC hash map (runtime library) OP_ARRAY_GET → array.get OP_RECORD_GET → struct.get (static) or runtime hash lookup (dynamic)</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@docs/souffle-vm.mdat line 505, The fenced code block containing the symbols
TSouffleArray, TSouffleRecord, OP_ARRAY_GET and OP_RECORD_GET is missing a
language hint (triggering markdownlint MD040); update the opening fence fromto a language-annotated fence such astext (or another appropriate language)
so the block reads ```text and retains the same contents to satisfy the linter.</details> </blockquote></details> </blockquote></details> <details> <summary>🧹 Nitpick comments (15)</summary><blockquote> <details> <summary>units/Goccia.CallStack.pas (1)</summary><blockquote> `15-16`: **LGTM — type alias follows guidelines.** The new `TGocciaCallFrameArray` type alias is correctly placed in the unit that declares `TGocciaCallFrame`. For full consistency, consider updating `FFrames` at line 21 to use this alias instead of the anonymous `array of TGocciaCallFrame`. As per coding guidelines: "define named type aliases in the unit declaring T and use the alias everywhere instead of re-specializing locally." <details> <summary>♻️ Optional: use the new alias for FFrames</summary> ```diff private - FFrames: array of TGocciaCallFrame; + FFrames: TGocciaCallFrameArray; FCount: Integer; ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@units/Goccia.CallStack.pas` around lines 15 - 16, Replace the anonymous array type used for the field FFrames with the new named alias TGocciaCallFrameArray: locate the declaration of the field FFrames (currently typed as "array of TGocciaCallFrame") and change its type to TGocciaCallFrameArray so the unit consistently uses the named alias defined above. ``` </details> </blockquote></details> <details> <summary>units/Goccia.Builtins.GlobalNumber.pas (1)</summary><blockquote> `46-47`: **Replace hardcoded Number property names with project constants.** These property keys are string literals in changed lines; use the corresponding `Goccia.Constants.PropertyNames` constants to keep naming centralized and prevent drift. As per coding guidelines: Use Goccia.Constants.PropertyNames constants (PROP_LENGTH, PROP_CONSTRUCTOR, etc.) instead of hardcoded string literals for property names. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@units/Goccia.Builtins.GlobalNumber.pas` around lines 46 - 47, Replace the hardcoded property name strings 'MAX_SAFE_INTEGER' and 'MIN_SAFE_INTEGER' with the centralized constants from Goccia.Constants.PropertyNames; update the two FBuiltinObject.DefineProperty calls that create TGocciaPropertyDescriptorData/TGocciaNumberLiteralValue so they reference the appropriate PropertyNames constants (e.g., the MAX and MIN safe integer constants) instead of string literals to keep naming centralized and consistent. ``` </details> </blockquote></details> <details> <summary>units/Goccia.Values.TypedArrayValue.pas (1)</summary><blockquote> `203-214`: **Apply implicit promotion for Int64→Double conversion on AArch64.** The conversion `Result := I64` may trigger bit reinterpretation instead of value conversion on FPC 3.2.2 AArch64. Use multiplication by 1.0 for safe implicit promotion. <details> <summary>Proposed fix</summary> ```diff takInt32: begin Move(FBufferData[Offset], I32, 4); I64 := I32; - Result := I64; + Result := I64 * 1.0; end; takUint32: begin Move(FBufferData[Offset], U32, 4); I64 := U32; - Result := I64; + Result := I64 * 1.0; end; ``` </details> Based on learnings: "On FPC 3.2.2 AArch64, use implicit promotion (Int64Var * 1.0 or Int64Var * 1000000.0) instead of Double(Int64Var) to avoid bit reinterpretation instead of value conversion" <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@units/Goccia.Values.TypedArrayValue.pas` around lines 203 - 214, The takInt32 and takUint32 branches assign I64 to Result which on FPC 3.2.2 AArch64 can reinterpret bits instead of converting to Double; to fix, perform an implicit promotion by multiplying the Int64 value by 1.0 (e.g., Result := I64 * 1.0) after populating I64 from I32/U32 so the value is safely converted to Double; update the branches that use FBufferData, I32, U32 and I64 to use this multiplication rather than direct assignment. ``` </details> </blockquote></details> <details> <summary>units/Goccia.Builtins.Benchmark.pas (1)</summary><blockquote> `165-169`: **Use `IsCallable` for callback validation.** Current checks rely on `TGocciaFunctionBase` RTTI, which hardcodes implementation type instead of callable behavior. <details> <summary>Refactor pattern</summary> ```diff - if not (AArgs.GetElement(1) is TGocciaFunctionBase) then Exit; + if not AArgs.GetElement(1).IsCallable then Exit; ``` </details> As per coding guidelines "Use Value.IsCallable instead of (Value is TGocciaFunctionBase) or (Value is TGocciaFunctionValue) or (Value is TGocciaNativeFunctionValue) checks." Also applies to: 203-214 <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@units/Goccia.Builtins.Benchmark.pas` around lines 165 - 169, The code validates the callback by checking type against TGocciaFunctionBase instead of checking callable behavior; replace the RTTI check in the block that assigns SuiteName and SuiteFunction (using AArgs.GetElement(1) and TGocciaFunctionBase) with the value's IsCallable check (Value.IsCallable) and then cast the callable value to the appropriate function type when assigning SuiteFunction; apply the same change to the other occurrences noted (lines ~203-214) so all callback validations use IsCallable rather than "is TGocciaFunctionBase" checks. ``` </details> </blockquote></details> <details> <summary>units/Goccia.Builtins.TestAssertions.pas (1)</summary><blockquote> `650-653`: **Prefer `IsCallable` over concrete `TGocciaFunctionBase` RTTI checks.** These guards/casts hardcode callable implementation type and work against the runtime callable abstraction. <details> <summary>Refactor pattern</summary> ```diff - if not (AArgs.GetElement(1) is TGocciaFunctionBase) then + if not AArgs.GetElement(1).IsCallable then ThrowError('... expects second argument to be a function', 0, 0); ``` </details> As per coding guidelines "Use Value.IsCallable instead of (Value is TGocciaFunctionBase) or (Value is TGocciaFunctionValue) or (Value is TGocciaNativeFunctionValue) checks." Also applies to: 682-685, 829-830, 862-867, 1330-1337, 1356-1364, 1398-1406, 1424-1429, 1453-1458, 1493-1498, 1509-1510, 1521-1522 <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@units/Goccia.Builtins.TestAssertions.pas` around lines 650 - 653, Replace the RTTI-based callable checks with the Value.IsCallable abstraction: instead of testing FActualValue with "is TGocciaFunctionBase"/"is TGocciaNativeFunctionValue"/ClassName comparisons (the code that sets IsInstance), call the value-level IsCallable property/method (e.g., FActualValue.IsCallable) to determine callability; apply the same replacement pattern to the other occurrences noted (around lines referenced: 682-685, 829-830, 862-867, 1330-1337, 1356-1364, 1398-1406, 1424-1429, 1453-1458, 1493-1498, 1509-1510, 1521-1522) so all concrete TGocciaFunction*/ClassName checks are removed and use IsCallable instead. ``` </details> </blockquote></details> <details> <summary>units/Goccia.Values.Primitives.pas (1)</summary><blockquote> `492-520`: **Add required ES spec-section annotations to updated coercion methods.** `ToBooleanLiteral` and `ToStringLiteral` are ECMAScript coercion behavior and should include the mandated spec-reference comments. As per coding guidelines, "When implementing ECMAScript-specified behavior, annotate each function with a comment referencing the relevant spec section using format // ESYYYY §X.Y.Z SpecMethodName(specParams)". <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@units/Goccia.Values.Primitives.pas` around lines 492 - 520, The ToBooleanLiteral and ToStringLiteral methods implement ECMAScript coercion and must be annotated with the required spec-section comments: add a single-line comment above TGocciaNumberLiteralValue.ToBooleanLiteral referencing the ES spec clause for ToBoolean (e.g. // ES2021 §7.1.2 ToBoolean(value)) and another above TGocciaNumberLiteralValue.ToStringLiteral referencing the spec clause for ToString (e.g. // ES2021 §7.1.12 ToString(value)) using the project's required format // ESYYYY §X.Y.Z SpecMethodName(specParams); keep the comments exact format required by the coding guidelines and place them immediately above the corresponding function declarations. ``` </details> </blockquote></details> <details> <summary>units/Goccia.Benchmark.Reporter.pas (1)</summary><blockquote> `323-329`: **Consider keeping JSON timing schema stable across modes.** The conditional field set forces consumers to branch on schema. Emitting all timing fields (`lex`, `parse`, `compile`) with `0` when not applicable simplifies downstream parsing. <details> <summary>♻️ Possible JSON output simplification</summary> ```diff - if FFiles[F].CompileTimeNanoseconds > 0 then - FOutput.Add(SysUtils.Format(' "compileTimeNanoseconds": %d,', [FFiles[F].CompileTimeNanoseconds])) - else - begin - FOutput.Add(SysUtils.Format(' "lexTimeNanoseconds": %d,', [FFiles[F].LexTimeNanoseconds])); - FOutput.Add(SysUtils.Format(' "parseTimeNanoseconds": %d,', [FFiles[F].ParseTimeNanoseconds])); - end; + FOutput.Add(SysUtils.Format(' "lexTimeNanoseconds": %d,', [FFiles[F].LexTimeNanoseconds])); + FOutput.Add(SysUtils.Format(' "parseTimeNanoseconds": %d,', [FFiles[F].ParseTimeNanoseconds])); + FOutput.Add(SysUtils.Format(' "compileTimeNanoseconds": %d,', [FFiles[F].CompileTimeNanoseconds])); ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@units/Goccia.Benchmark.Reporter.pas` around lines 323 - 329, Always emit all timing fields to keep the JSON schema stable: replace the conditional that only outputs "compileTimeNanoseconds" when FFiles[F].CompileTimeNanoseconds > 0 and otherwise emits "lexTimeNanoseconds" and "parseTimeNanoseconds" with logic that always adds "lexTimeNanoseconds", "parseTimeNanoseconds", and "compileTimeNanoseconds" entries to FOutput (using FFiles[F].LexTimeNanoseconds, FFiles[F].ParseTimeNanoseconds, and FFiles[F].CompileTimeNanoseconds respectively), writing zeros where a value is not applicable so consumers never need to branch on the presence of fields. ``` </details> </blockquote></details> <details> <summary>units/Goccia.Scope.pas (1)</summary><blockquote> `48-48`: **Avoid silent misses in `ForceUpdateBinding`.** Line 289 silently does nothing when the binding is absent. Returning a success flag (or raising in strict/debug paths) would make missed updates explicit and easier to diagnose. <details> <summary>♻️ Proposed refactor</summary> ```diff - procedure ForceUpdateBinding(const AName: string; const AValue: TGocciaValue); + function ForceUpdateBinding(const AName: string; const AValue: TGocciaValue): Boolean; - procedure TGocciaScope.ForceUpdateBinding(const AName: string; const AValue: TGocciaValue); + function TGocciaScope.ForceUpdateBinding(const AName: string; const AValue: TGocciaValue): Boolean; var LexicalBinding: TLexicalBinding; begin - if FLexicalBindings.TryGetValue(AName, LexicalBinding) then - begin - LexicalBinding.Value := AValue; - LexicalBinding.Initialized := True; - FLexicalBindings.AddOrSetValue(AName, LexicalBinding); - end; + Result := FLexicalBindings.TryGetValue(AName, LexicalBinding); + if not Result then + Exit; + LexicalBinding.Value := AValue; + LexicalBinding.Initialized := True; + FLexicalBindings.AddOrSetValue(AName, LexicalBinding); end; ``` </details> Also applies to: 285-295 <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@units/Goccia.Scope.pas` at line 48, Change ForceUpdateBinding from a procedure to return a Boolean (function ForceUpdateBinding(const AName: string; const AValue: TGocciaValue): Boolean) and update its implementation so it returns True when the binding is found+updated and False when the binding is absent; alternatively add an optional parameter (e.g. RaiseIfMissing: Boolean = False) that will raise an exception when missing in strict/debug paths. Update all callers of ForceUpdateBinding (and any overloads in unit Goccia.Scope.pas) to check the Boolean result or pass RaiseIfMissing=true where failures must be hard-faulted, and adjust unit tests or usages accordingly. ``` </details> </blockquote></details> <details> <summary>TestRunner.dpr (1)</summary><blockquote> `247-250`: **Consider clarifying the timing field semantics.** In bytecode mode, `LexTimeNanoseconds` is being repurposed to hold the full compilation time (lex + parse + compile). While this works for the display logic at lines 424-427, it creates a semantic mismatch with the field name. Consider documenting this dual-purpose behavior or adding a dedicated `CompileTimeNanoseconds` field to `TGocciaScriptResult` for cleaner separation. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@TestRunner.dpr` around lines 247 - 250, The timing fields currently store full compilation time in Result.Timing.LexTimeNanoseconds which mismatches the name; update the TGocciaScriptResult timing record to add a dedicated CompileTimeNanoseconds field, populate CompileTimeNanoseconds := CompileEnd - CompileStart in the code that sets timings (instead of reusing LexTimeNanoseconds), leave LexTimeNanoseconds as true lexing time (or set to 0 if not available), and update any display/consumption code (e.g., the display logic referenced around lines 424-427 that reads LexTimeNanoseconds) to use CompileTimeNanoseconds when in bytecode/compiled mode so semantics are consistent and clear. ``` </details> </blockquote></details> <details> <summary>units/Goccia.Compiler.Scope.pas (2)</summary><blockquote> `21-22`: **Initialize `IsGlobalBacked` in `DeclareLocal`.** The new `IsGlobalBacked` field is not explicitly initialized when declaring a local (unlike `TypeHint` which is initialized to `sltUntyped` at line 104). While `FillChar` or default record initialization may zero this field, explicit initialization would be more consistent with the existing pattern. <details> <summary>♻️ Suggested initialization</summary> ```diff FLocals[FLocalCount].IsConst := AIsConst; FLocals[FLocalCount].TypeHint := sltUntyped; + FLocals[FLocalCount].IsGlobalBacked := False; Result := FNextSlot; ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@units/Goccia.Compiler.Scope.pas` around lines 21 - 22, DeclareLocal does not explicitly initialize the new IsGlobalBacked Boolean like TypeHint is set to sltUntyped; update the DeclareLocal function to explicitly set IsGlobalBacked := False when initializing a new local (alongside setting TypeHint := sltUntyped) so the new field has a clear, consistent default state. ``` </details> --- `227-236`: **Add bounds checking in `MarkGlobalBacked` and `SetLocalTypeHint`.** Both methods directly index into `FLocals` without validating that `AIndex` is within bounds. Other methods like `GetLocal` and `GetUpvalue` perform similar array access, and while they don't have explicit bounds checks either, these new methods modify state and could corrupt data if called with an invalid index. <details> <summary>🛡️ Proposed defensive bounds check</summary> ```diff procedure TGocciaCompilerScope.MarkGlobalBacked(const AIndex: Integer); begin + if (AIndex < 0) or (AIndex >= FLocalCount) then + raise Exception.CreateFmt('MarkGlobalBacked: index %d out of range 0..%d', [AIndex, FLocalCount - 1]); FLocals[AIndex].IsGlobalBacked := True; end; procedure TGocciaCompilerScope.SetLocalTypeHint(const AIndex: Integer; const ATypeHint: TSouffleLocalType); begin + if (AIndex < 0) or (AIndex >= FLocalCount) then + raise Exception.CreateFmt('SetLocalTypeHint: index %d out of range 0..%d', [AIndex, FLocalCount - 1]); FLocals[AIndex].TypeHint := ATypeHint; end; ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@units/Goccia.Compiler.Scope.pas` around lines 227 - 236, Add defensive bounds checks to TGocciaCompilerScope.MarkGlobalBacked and TGocciaCompilerScope.SetLocalTypeHint: before accessing FLocals[AIndex] ensure AIndex is >= 0 and < Length(FLocals) and handle out-of-range by raising an appropriate exception (e.g., ERangeError) or using an assertion; then perform the existing assignments to FLocals[AIndex].IsGlobalBacked and FLocals[AIndex].TypeHint (TSouffleLocalType) so invalid indexes cannot corrupt FLocals. ``` </details> </blockquote></details> <details> <summary>souffle/Souffle.VM.NativeFunction.pas (1)</summary><blockquote> `64-68`: **Add null check for `FCallback` before invocation.** The `Invoke` method directly calls `FCallback` without checking if it's assigned. While callers should always provide a valid callback, defensive programming would prevent a crash if `TSouffleNativeFunction` is constructed with a nil callback. <details> <summary>🛡️ Suggested defensive check</summary> ```diff function TSouffleNativeFunction.Invoke(const AReceiver: TSouffleValue; const AArgs: PSouffleValue; const AArgCount: Integer): TSouffleValue; begin + if not Assigned(FCallback) then + raise Exception.Create('Native function has no callback assigned'); Result := FCallback(AReceiver, AArgs, AArgCount); end; ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@souffle/Souffle.VM.NativeFunction.pas` around lines 64 - 68, TSouffleNativeFunction.Invoke currently calls FCallback without verifying it’s assigned, which can crash if constructed with nil; add a defensive check using Assigned(FCallback) at the start of TSouffleNativeFunction.Invoke and if it’s not assigned raise a clear exception (e.g., raise Exception.Create('TSouffleNativeFunction callback not assigned') or a typed EInvalidOperation) so callers get a predictable error instead of an access violation; otherwise proceed to call FCallback(AReceiver, AArgs, AArgCount) and return its result. ``` </details> </blockquote></details> <details> <summary>units/Goccia.Compiler.Context.pas (1)</summary><blockquote> `135-162`: **Silent fallback to `OP_RT_ADD` for unknown token types.** `TokenTypeToRuntimeOp` returns `OP_RT_ADD` for any unrecognized token type (line 160). This could mask compiler bugs where an invalid operator token reaches this function. Consider raising an exception for unexpected cases. <details> <summary>🛡️ Proposed defensive handling</summary> ```diff gttLeftShift: Result := OP_RT_SHL; gttRightShift: Result := OP_RT_SHR; gttUnsignedRightShift: Result := OP_RT_USHR; else - Result := OP_RT_ADD; + raise Exception.CreateFmt('TokenTypeToRuntimeOp: unexpected token type %d', [Ord(ATokenType)]); end; end; ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@units/Goccia.Compiler.Context.pas` around lines 135 - 162, The function TokenTypeToRuntimeOp currently returns OP_RT_ADD for any unknown TGocciaTokenType which can silently hide bugs; replace the silent fallback in TokenTypeToRuntimeOp with a defensive failure that raises a clear exception (or uses an assert) reporting the unexpected TGocciaTokenType/ordinal and that it cannot be mapped to a TSouffleOpCode instead of returning OP_RT_ADD, so callers see an immediate, informative failure when an invalid token type is passed. ``` </details> </blockquote></details> <details> <summary>units/Goccia.Compiler.Expressions.pas (1)</summary><blockquote> `708-709`: **Use keyword/property constants instead of raw literals.** Identifiers like `'this'` and related keyword/property names should be referenced through constants to keep parser/compiler/runtime naming aligned. As per coding guidelines: use `Goccia.Keywords.Reserved` / `Goccia.Keywords.Contextual` constants for keywords and avoid hardcoded keyword literals. Also applies to: 1708-1718, 1733-1743 <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@units/Goccia.Compiler.Expressions.pas` around lines 708 - 709, Replace the hardcoded identifier '__receiver' with the appropriate keyword/property constant from Goccia.Keywords (use the Reserved or Contextual constant that represents the 'this' receiver name) so parser/compiler/runtime use the canonical symbol; update ChildScope.DeclareLocal('__receiver', False) and any other occurrences noted (around the other ranges 1708-1718 and 1733-1743) to use that constant (e.g., Goccia.Keywords.Reserved.<RECEIVER_CONST> or Goccia.Keywords.Contextual.<RECEIVER_CONST>) and ensure ChildTemplate.ParameterCount assignment remains unchanged. ``` </details> </blockquote></details> <details> <summary>units/Goccia.Compiler.Statements.pas (1)</summary><blockquote> `725-726`: **Replace hardcoded keyword/property strings with constants.** Use keyword/property constants for `'this'`, `'constructor'`, and related identifiers instead of raw literals to match project conventions and reduce drift. As per coding guidelines: use `Goccia.Keywords.Reserved` / `Goccia.Keywords.Contextual` constants for keywords, and `Goccia.Constants.PropertyNames` constants for property names instead of hardcoded string literals. Also applies to: 822-823, 901-902 <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@units/Goccia.Compiler.Statements.pas` around lines 725 - 726, Replace hardcoded identifier/property strings with the project's keyword and property-name constants: where you call ChildScope.DeclareLocal('this', False) and set ChildTemplate.ParameterCount := Length(AMethod.Parameters) (and the similar occurrences noted), replace the literal 'this' and any literal 'constructor' or property-name strings with the appropriate constants from Goccia.Keywords.Reserved or Goccia.Keywords.Contextual for keywords and Goccia.Constants.PropertyNames for properties; update all occurrences (including the ones referenced near the other blocks) to use those constants so the code uses Goccia.Keywords.* and Goccia.Constants.PropertyNames.* instead of raw string literals. ``` </details> </blockquote></details> </blockquote></details> --- <details> <summary>ℹ️ Review info</summary> **Configuration used**: defaults **Review profile**: CHILL **Plan**: Pro <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between bb4dd682d12db297440aec9d44e3398de9ea082a and 2cd5febcccca0def9c67c70fbfd6c2902682b3db. </details> <details> <summary>📒 Files selected for processing (55)</summary> * `AGENTS.md` * `BenchmarkRunner.dpr` * `ScriptLoader.dpr` * `TestRunner.dpr` * `docs/design-decisions.md` * `docs/souffle-vm.md` * `souffle/Souffle.Bytecode.Binary.pas` * `souffle/Souffle.Bytecode.Chunk.pas` * `souffle/Souffle.Bytecode.Module.pas` * `souffle/Souffle.Bytecode.pas` * `souffle/Souffle.Compound.pas` * `souffle/Souffle.Heap.pas` * `souffle/Souffle.VM.CallFrame.pas` * `souffle/Souffle.VM.Closure.pas` * `souffle/Souffle.VM.NativeFunction.pas` * `souffle/Souffle.VM.RuntimeOperations.pas` * `souffle/Souffle.VM.pas` * `souffle/Souffle.Value.pas` * `tests/built-ins/global-properties/global-this.js` * `tests/built-ins/global-properties/gocciascript.js` * `tests/built-ins/global-properties/infinity.js` * `tests/built-ins/global-properties/nan.js` * `tests/built-ins/global-properties/undefined.js` * `units/Goccia.Benchmark.Reporter.pas` * `units/Goccia.Builtins.Benchmark.pas` * `units/Goccia.Builtins.GlobalNumber.pas` * `units/Goccia.Builtins.GlobalObject.pas` * `units/Goccia.Builtins.TestAssertions.Test.pas` * `units/Goccia.Builtins.TestAssertions.pas` * `units/Goccia.CallStack.pas` * `units/Goccia.Compiler.ConstantFolding.pas` * `units/Goccia.Compiler.Context.pas` * `units/Goccia.Compiler.Expressions.pas` * `units/Goccia.Compiler.ExtOps.pas` * `units/Goccia.Compiler.Scope.pas` * `units/Goccia.Compiler.Statements.pas` * `units/Goccia.Compiler.Test.pas` * `units/Goccia.Compiler.pas` * `units/Goccia.Engine.Backend.pas` * `units/Goccia.Engine.pas` * `units/Goccia.Evaluator.Comparison.pas` * `units/Goccia.Evaluator.pas` * `units/Goccia.Runtime.Operations.pas` * `units/Goccia.Scope.pas` * `units/Goccia.TestSetup.pas` * `units/Goccia.Values.ArrayValue.pas` * `units/Goccia.Values.AsyncFunctionValue.pas` * `units/Goccia.Values.ClassHelper.pas` * `units/Goccia.Values.FunctionValue.Test.pas` * `units/Goccia.Values.FunctionValue.pas` * `units/Goccia.Values.ObjectValue.Test.pas` * `units/Goccia.Values.ObjectValue.pas` * `units/Goccia.Values.Primitives.Test.pas` * `units/Goccia.Values.Primitives.pas` * `units/Goccia.Values.TypedArrayValue.pas` </details> <details> <summary>💤 Files with no reviewable changes (2)</summary> * units/Goccia.Values.AsyncFunctionValue.pas * units/Goccia.Values.ClassHelper.pas </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
Comment on lines
+197
to
+207
| gttRightShift: | ||
| begin | ||
| EmitFoldedNumber(ACtx, Int32(Trunc(LeftVal)) shr (Int32(Trunc(RightVal)) and $1F), ADest); | ||
| Result := True; | ||
| end; | ||
| gttUnsignedRightShift: | ||
| begin | ||
| EmitFoldedNumber(ACtx, Int64(UInt32(Trunc(LeftVal)) shr | ||
| (Int32(Trunc(RightVal)) and $1F)), ADest); | ||
| Result := True; | ||
| end; |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for right shift operations and SarLongint usage
echo "=== Searching for SarLongint and arithmetic shift functions ==="
rg -i 'sarlongint|sarint|arithmetic.*shift|sar\s+' --type pas | head -20
echo -e "\n=== Searching for right shift tests ==="
rg -n 'RightShift|shr|>>|gttRightShift' --type pas -B2 -A2 | head -40
echo -e "\n=== Checking evaluator for right shift implementation ==="
fd 'Evaluator.*\.pas$' --type f | head -5Repository: frostney/GocciaScript
Length of output: 488
🏁 Script executed:
#!/bin/bash
# Look for existing right shift handling in evaluator
echo "=== Searching for right shift in evaluator ==="
rg -n 'opRightShift|opUnsignedRightShift|>>|>>>' --type pas units/ | head -20
echo -e "\n=== Check Goccia.inc for shift definitions ==="
cat Goccia.inc | grep -i shift || echo "No shift definitions in Goccia.inc"
echo -e "\n=== Looking at constant folding more carefully ==="
sed -n '190,215p' units/Goccia.Compiler.ConstantFolding.pasRepository: frostney/GocciaScript
Length of output: 1076
🏁 Script executed:
#!/bin/bash
# Check for test cases involving right shift
echo "=== Searching for right shift test cases ==="
find . -name '*Test*.pas' -o -name '*test*.js' | xargs grep -l 'RightShift\|>>\|>>>' 2>/dev/null | head -10Repository: frostney/GocciaScript
Length of output: 109
🏁 Script executed:
#!/bin/bash
# Check FPC Math unit documentation or look for shift function definitions
echo "=== Looking for shift-related functions in units ==="
rg -n 'function.*Sar|function.*shr|SarLongint|SarInt' --type pas
echo -e "\n=== Check if this is a real issue by looking at runtime evaluator ==="
rg -n 'gttRightShift|opRightShift' --type pas -B5 -A5 | head -40Repository: frostney/GocciaScript
Length of output: 261
🏁 Script executed:
#!/bin/bash
# Use find and grep instead of rg with type flags
echo "=== Searching for right shift in evaluator (Bitwise) ==="
find . -name "Goccia.Evaluator.Bitwise.pas" -exec cat {} \; | grep -n -A10 "RightShift\|opRightShift" | head -50
echo -e "\n=== Check the actual constant folding file ==="
find . -name "Goccia.Compiler.ConstantFolding.pas" -exec wc -l {} \;Repository: frostney/GocciaScript
Length of output: 1462
🏁 Script executed:
#!/bin/bash
# Look for how right shift is implemented at runtime
echo "=== Examining Bitwise evaluator ==="
find . -name "*.pas" -path "*/Evaluator*" -name "*Bitwise*" -exec head -250 {} \;Repository: frostney/GocciaScript
Length of output: 100
🏁 Script executed:
#!/bin/bash
# Search for test files mentioning shifts
echo "=== Finding test files ==="
find . -name "*.Test.pas" -o -name "*Test*.js" | head -10
echo -e "\n=== Check for shift operator implementation ==="
find . -name "*.pas" -exec grep -l ">>.*shr\|arithmetic.*shift\|signed.*right.*shift" {} \; 2>/dev/null | head -5Repository: frostney/GocciaScript
Length of output: 351
🏁 Script executed:
#!/bin/bash
# Check if there's a web query we should do or look at the evaluator directly
echo "=== Reading Goccia.Evaluator.Bitwise.pas directly ==="
find . -name "Goccia.Evaluator.Bitwise.pas" -exec head -300 {} \;Repository: frostney/GocciaScript
Length of output: 2360
🏁 Script executed:
#!/bin/bash
# Look for test cases with negative number shifts
echo "=== Searching for test cases with >> operator ==="
find . -type f \( -name "*.js" -o -name "*.Test.pas" \) -exec grep -l ">>" {} \; 2>/dev/null | head -5Repository: frostney/GocciaScript
Length of output: 378
🏁 Script executed:
#!/bin/bash
# Check if there are tests for negative right shift
echo "=== Checking bitwise-rightshift test ==="
cat tests/language/expressions/bitwise/bitwise-rightshift.js
echo -e "\n=== Checking edge cases ==="
cat tests/language/expressions/bitwise/edge-cases.js | head -100Repository: frostney/GocciaScript
Length of output: 947
🏁 Script executed:
#!/bin/bash
# Verify FPC shr behavior with a simple test
echo "=== Testing FPC shr behavior ==="
cat > /tmp/test_shift.pas << 'EOF'
program TestShift;
var
x: Int32;
y: Int64;
result: Int64;
begin
x := -8;
y := Trunc(-8.0);
{ This should be -2 in JavaScript, but will be large positive in FPC }
result := Int32(y) shr 2;
WriteLn('Int32(-8) shr 2 = ', result);
{ Try with explicit conversion }
result := x shr 2;
WriteLn('x shr 2 = ', result);
end.
EOF
# Check if we can compile it
fpc /tmp/test_shift.pas -o /tmp/test_shift 2>&1 | head -20 || echo "FPC not available in sandbox"Repository: frostney/GocciaScript
Length of output: 140
🏁 Script executed:
#!/bin/bash
# Look for tests that actually test negative number shifts
echo "=== Grep for negative shift tests ==="
grep -n "\-.*>>" tests/language/expressions/bitwise/bitwise-rightshift.js | head -20
echo -e "\n=== Full content of bitwise shift test ==="
wc -l tests/language/expressions/bitwise/bitwise-rightshift.js
echo -e "\n=== Sample of bitwise tests ==="
head -60 tests/language/expressions/bitwise/bitwise-rightshift.jsRepository: frostney/GocciaScript
Length of output: 441
🏁 Script executed:
#!/bin/bash
# Check if there are any uses of SarInt or similar in the codebase
echo "=== Searching for SAR/arithmetic shift implementations ==="
find . -name "*.pas" -type f -exec grep -l "Sar\|arithmetic" {} \; 2>/dev/null | head -10
echo -e "\n=== Check for any shift-related comments ==="
find . -name "*.pas" -type f -exec grep -i "shift.*sign\|arithmetic.*shift" {} + 2>/dev/null | head -10Repository: frostney/GocciaScript
Length of output: 1077
🏁 Script executed:
#!/bin/bash
# Check the test file for constant folding
echo "=== Checking Goccia.Compiler.Test.pas for shift tests ==="
grep -n "RightShift\|>>\|Shift" units/Goccia.Compiler.Test.pas | head -30
echo -e "\n=== Looking for test procedures related to constant folding ==="
grep -n "procedure.*Fold\|procedure.*Shift\|procedure.*Bitwise" units/Goccia.Compiler.Test.pas | head -20
echo -e "\n=== Check for negative number tests ==="
grep -n "\-[0-9].*>>\|\-1.*shr" units/Goccia.Compiler.Test.pasRepository: frostney/GocciaScript
Length of output: 228
🏁 Script executed:
#!/bin/bash
# Check if Runtime.Operations uses arithmetic shift
echo "=== Examining Goccia.Runtime.Operations.pas ==="
find . -name "Goccia.Runtime.Operations.pas" -exec cat {} \; | grep -A20 "RightShift\|Shift.*Right" | head -50Repository: frostney/GocciaScript
Length of output: 2215
🏁 Script executed:
#!/bin/bash
# Look for inline assembly or SAR usage
echo "=== Checking for SAR or inline assembly ==="
find . -name "*.pas" -type f -exec grep -l "sar\|SAR\|inline.*asm\|asm.*end" {} \; 2>/dev/null | head -10
echo -e "\n=== Check one of the files that had SAR matches ==="
grep -n -i "sar\|arithmetic" ./units/Goccia.Evaluator.Arithmetic.pas | head -20Repository: frostney/GocciaScript
Length of output: 234
🏁 Script executed:
#!/bin/bash
# Check if there's documentation about shift semantics
echo "=== Looking for comments about shift semantics ==="
find . -name "*.pas" -type f -exec grep -B2 -A2 ">>.*arithmetic\|sign.*extend\|arithmetic.*shift" {} + 2>/dev/null | head -40Repository: frostney/GocciaScript
Length of output: 115
🏁 Script executed:
#!/bin/bash
# Verify SarLongint is available - look for where it's defined
echo "=== Looking for SarLongint definition/import ==="
grep -r "SarLongint\|function.*Sar" --include="*.pas" . | head -20
echo -e "\n=== Check what units are imported in Runtime.Operations ==="
head -30 units/Goccia.Runtime.Operations.pas | grep -A20 "^uses"
echo -e "\n=== Verify the actual inconsistency ==="
echo "Constant folding (line 199):"
sed -n '197,201p' units/Goccia.Compiler.ConstantFolding.pas
echo ""
echo "Runtime ShiftRight implementation:"
grep -A5 "function.*ShiftRight.*TSouffleValue.*TSouffleValue" units/Goccia.Runtime.Operations.pas | head -7Repository: frostney/GocciaScript
Length of output: 1388
🏁 Script executed:
#!/bin/bash
# Check if there are any tests that test negative right shift
echo "=== Search all test files for negative >> tests ==="
find tests -name "*.js" -type f -exec grep -l "\-[0-9].*>>" {} \;
echo -e "\n=== Also check for test patterns like (-N >> M) ==="
find tests -name "*.js" -type f -exec grep "(\-[0-9]" {} + | grep ">>" | head -10Repository: frostney/GocciaScript
Length of output: 308
🏁 Script executed:
#!/bin/bash
# Find where SarLongint is defined
echo "=== Looking for SarLongint in FPC/Delphi standard units ==="
grep -r "SarLongint" --include="*.pas" . 2>/dev/null | head -20
echo -e "\n=== Check if it's imported from Math unit ==="
head -20 units/Goccia.Runtime.Operations.pas | grep "uses" -A30
echo -e "\n=== Get the full uses clause ==="
sed -n '/^uses/,/;$/p' units/Goccia.Runtime.Operations.pas | head -40Repository: frostney/GocciaScript
Length of output: 1429
🏁 Script executed:
#!/bin/bash
# Verify what SarLongint does - search documentation or comments
echo "=== Looking for documentation of SarLongint ==="
grep -B5 -A5 "SarLongint" units/Goccia.Runtime.Operations.pas
echo -e "\n=== Also check Evaluator.Bitwise for comparison ==="
sed -n '43,46p' units/Goccia.Evaluator.Bitwise.pasRepository: frostney/GocciaScript
Length of output: 671
🏁 Script executed:
#!/bin/bash
# Test with actual negative number to understand the issue
echo "=== JavaScript behavior: -8 >> 2 should be -2 ==="
echo "In JavaScript, -8 >> 2 = -2 (arithmetic shift, sign-extending)"
echo "In FPC logical shift: Int32(-8) shr 2 would give wrong result"
echo ""
echo "=== Check if SarLongint is from System or Math ==="
grep -n "function SarLongint\|procedure SarLongint" /usr/lib/fpc/*/units/*/*.ppu 2>/dev/null || echo "Checking source..."
echo -e "\n=== Verify by grepping Math unit imports ==="
grep "Math" units/Goccia.Runtime.Operations.pas
grep "System" units/Goccia.Runtime.Operations.pasRepository: frostney/GocciaScript
Length of output: 420
🏁 Script executed:
#!/bin/bash
# Let's verify SarLongint is indeed available and working as expected
echo "=== Check if SarLongint is documented in FPC Math unit ==="
python3 << 'EOF'
# Based on FPC documentation, SarLongint performs arithmetic shift right
# It's in the Math unit and does sign-extension
print("SarLongint(Int32(-8), 2) should return -2 (arithmetic right shift)")
print("Int32(-8) shr 2 would return a large positive (logical right shift - WRONG)")
print("")
print("The Math unit in FPC provides SarLongint for this purpose.")
EOF
echo -e "\n=== Verify constant folding doesn't match runtime ==="
echo "Constant folding uses: shr (logical)"
echo "Runtime uses: SarLongint (arithmetic)"
echo "This is inconsistent!"Repository: frostney/GocciaScript
Length of output: 478
🏁 Script executed:
#!/bin/bash
# Check if there are other places in ConstantFolding that need fixing
echo "=== Check all shifts in ConstantFolding ==="
sed -n '180,210p' units/Goccia.Compiler.ConstantFolding.pas | grep -n "shr\|shl"
echo -e "\n=== Check the pattern across all three implementations ==="
echo "=== 1. ConstantFolding.pas (line 199) ==="
sed -n '199p' units/Goccia.Compiler.ConstantFolding.pas | grep -o "shr\|shl"
echo "=== 2. Evaluator.Bitwise.pas (line 45) ==="
sed -n '45p' units/Goccia.Evaluator.Bitwise.pas | grep -o "shr\|shl"
echo "=== 3. Runtime.Operations.pas (ShiftRight) ==="
grep "SarLongint\|ShiftRight" units/Goccia.Runtime.Operations.pas | head -1Repository: frostney/GocciaScript
Length of output: 656
🏁 Script executed:
#!/bin/bash
# Final verification - check if Math is imported in ConstantFolding
echo "=== Check if Math unit is imported in ConstantFolding ==="
head -30 units/Goccia.Compiler.ConstantFolding.pas | grep -i "uses\|math"
echo -e "\n=== Check imports in Evaluator.Bitwise ==="
head -15 units/Goccia.Evaluator.Bitwise.pas
echo -e "\n=== Verify the exact line numbers in both files ==="
echo "ConstantFolding.pas:"
grep -n "gttRightShift:" units/Goccia.Compiler.ConstantFolding.pas
echo -e "\nEvaluator.Bitwise.pas:"
grep -n "function EvaluateRightShift" units/Goccia.Evaluator.Bitwise.pasRepository: frostney/GocciaScript
Length of output: 1093
🏁 Script executed:
#!/bin/bash
# Verify we need to add Math import to Evaluator.Bitwise
echo "=== Confirm Evaluator.Bitwise needs Math import added ==="
sed -n '1,20p' units/Goccia.Evaluator.Bitwise.pas
echo -e "\n=== Show the implementation lines that need fixing ==="
echo "ConstantFolding line 199:"
sed -n '199p' units/Goccia.Compiler.ConstantFolding.pas
echo -e "\nEvaluator.Bitwise line 45:"
sed -n '45p' units/Goccia.Evaluator.Bitwise.pasRepository: frostney/GocciaScript
Length of output: 1181
Signed right shift (>>) uses logical shift instead of arithmetic shift, producing incorrect results for negative numbers.
JavaScript's >> operator performs arithmetic (sign-extending) right shift. FPC's shr is a logical shift, which fills with zeros rather than preserving the sign bit. For example, -8 >> 2 should return -2, but Int32(-8) shr 2 produces a large positive number.
The same bug exists in two locations:
units/Goccia.Compiler.ConstantFolding.pasline 199 (gttRightShift)units/Goccia.Evaluator.Bitwise.pasline 45 (EvaluateRightShift)
The runtime implementation in Goccia.Runtime.Operations.pas correctly uses SarLongint() for signed right shift. Both locations must be updated to maintain consistency.
Proposed fixes using SarLongint for arithmetic shift
ConstantFolding.pas (Math is already imported):
gttRightShift:
begin
- EmitFoldedNumber(ACtx, Int32(Trunc(LeftVal)) shr (Int32(Trunc(RightVal)) and $1F), ADest);
+ EmitFoldedNumber(ACtx, SarLongint(Int32(Trunc(LeftVal)), Int32(Trunc(RightVal)) and $1F), ADest);
Result := True;
end;Evaluator.Bitwise.pas (add Math to implementation uses, then fix the shift):
implementation
uses
+ Math,
Goccia.Values.ClassHelper;
function EvaluateRightShift(const ALeft, ARight: TGocciaValue): TGocciaValue;
begin
- Result := TGocciaNumberLiteralValue.Create(Trunc(ALeft.ToNumberLiteral.Value) shr (Trunc(ARight.ToNumberLiteral.Value) and 31));
+ Result := TGocciaNumberLiteralValue.Create(SarLongint(Int32(Trunc(ALeft.ToNumberLiteral.Value)), Int32(Trunc(ARight.ToNumberLiteral.Value)) and 31));
end;SarLongint (from the Math unit) performs arithmetic right shift, preserving the sign bit.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@units/Goccia.Compiler.ConstantFolding.pas` around lines 197 - 207, The
gttRightShift constant-folding case is using the logical shr operator causing
wrong results for negative values; replace the shr-based shift in gttRightShift
(the EmitFoldedNumber call) with an arithmetic right-shift using SarLongint on
the Int32-truncated LeftVal and apply the masked shift amount as before, and do
the analogous change in EvaluateRightShift in the evaluator by calling
SarLongint for signed shifts (add Math to the uses if missing); leave the
unsigned-right-shift paths (gttUnsignedRightShift / UInt32 shr) unchanged.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 14
♻️ Duplicate comments (5)
units/Goccia.Values.ObjectValue.pas (1)
514-515:⚠️ Potential issue | 🟠 MajorUse
IsCallablefor accessor invocation guards instead of RTTI type checks.These checks still gate on
is TGocciaFunctionBaseinstead of the VMT contract (IsCallable), which is the project-standard callable test.🔧 Suggested pattern
- if Assigned(Accessor.Setter) and (Accessor.Setter is TGocciaFunctionBase) then + if Assigned(Accessor.Setter) and Accessor.Setter.IsCallable thenApply the same change for all getter/setter guard sites above.
As per coding guidelines: "Use Value.IsCallable instead of (Value is TGocciaFunctionBase) or (Value is TGocciaFunctionValue) or (Value is TGocciaNativeFunctionValue) checks".
Also applies to: 548-549, 649-650, 677-678
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@units/Goccia.Values.ObjectValue.pas` around lines 514 - 515, Replace RTTI checks that test accessor callability via "Accessor.Setter is TGocciaFunctionBase" (and similar checks for Accessor.Getter) with the VMT contract call IsCallable on the Value/Accessor (e.g., use Accessor.Setter.IsCallable or Value.IsCallable) so guards use the canonical IsCallable predicate; update all the other occurrences noted (the analogous guards at the sites flagged around 548-549, 649-650, 677-678) to follow the same pattern, ensuring you check for Assigned(...) before calling IsCallable to avoid nil dereference.units/Goccia.Compiler.ConstantFolding.pas (1)
43-44:⚠️ Potential issue | 🔴 CriticalUse encoder-aligned AsBx bounds in folded integer fast-path as well.
EmitFoldedNumbershould share the same signed-range constants asEncodeAsBxto avoid edge-value misencoding.🔧 Suggested fix
- and (Frac(AValue) = 0.0) and (AValue >= -32768) and (AValue <= 32767) then + and (Frac(AValue) = 0.0) and (AValue >= -SBIAS_16) and (AValue <= SBIAS_16) then(Prefer using the exact constants used by the bytecode encoder.)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@units/Goccia.Compiler.ConstantFolding.pas` around lines 43 - 44, The folded-integer fast path in EmitFoldedNumber uses hardcoded -32768..32767 bounds that can mismatch EncodeAsBx; change EmitFoldedNumber to use the same signed-range constants used by EncodeAsBx (the AsBx encoder limits) when deciding the fast-path so edge values are encoded consistently, and keep the EmitInstruction call to EncodeAsBx(OP_LOAD_INT, ADest, Int16(Trunc(AValue))) unchanged aside from using those shared constants.units/Goccia.Compiler.Expressions.pas (1)
147-150:⚠️ Potential issue | 🔴 Critical
OP_LOAD_INTfast-path bounds should be derived from theEncodeAsBxsigned range.Using hardcoded literals here can diverge from encoder/decoder bounds and misencode edge values.
🔧 Suggested fix
- and (TGocciaNumberLiteralValue(Value).Value >= -32768) - and (TGocciaNumberLiteralValue(Value).Value <= 32767) then + and (TGocciaNumberLiteralValue(Value).Value >= -SBIAS_16) + and (TGocciaNumberLiteralValue(Value).Value <= SBIAS_16) then(Use the same range constants the AsBx encoder/decoder uses.)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@units/Goccia.Compiler.Expressions.pas` around lines 147 - 150, The fast-path for OP_LOAD_INT currently uses hardcoded -32768/32767 bounds; instead use the same signed sBx range constants used by the AsBx encoder/decoder (the constants referenced by EncodeAsBx/DecodeAsBx) to decide the branch and to cast the literal (replace Int16(Trunc(...)) with a cast sized to that sBx range). Update the condition that checks TGocciaNumberLiteralValue(Value).Value to compare against the encoder's MIN_SBX and MAX_SBX (or whatever named constants the EncodeAsBx implementation exposes), and ensure EmitInstruction(ACtx, EncodeAsBx(OP_LOAD_INT, ADest, <casted sBx value>)) uses that same constant-derived range and cast.docs/souffle-vm.md (1)
1124-1129:⚠️ Potential issue | 🟠 MajorPortability claim contradicts the file’s own endianness section.
Line 1128 says
.sbcis portable, but Line 1001 explicitly says serialization is native-endian and needs normalization for cross-platform use. These statements conflict and should be reconciled.Based on learnings: `.sbc` binary format uses native endianness (not portable).📝 Suggested wording
-- **Portable binary format**: `.sbc` files are self-describing (runtime tag, version, debug info) +- **Self-describing binary format**: `.sbc` files include runtime tag/version/debug metadata, but serialization is native-endian and not inherently cross-platform portable without normalization.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/souffle-vm.md` around lines 1124 - 1129, The documentation currently contradicts itself about .sbc portability: update the "Portable binary format" bullet and/or its surrounding text so it reflects the endianness section (serialization is native-endian and requires normalization for cross-platform use). Replace the claim "portable" with a clear statement that .sbc is self-describing but uses native endianness (or add an explicit caveat), and add a cross-reference to the endianness/serialization section to guide readers (referencing the `.sbc` format, the "Portable binary format" bullet, and the endianness/serialization section).souffle/Souffle.VM.pas (1)
806-813:⚠️ Potential issue | 🔴 CriticalGuard
OP_MOD_INTagainst zero divisors.Line 811 computes integer modulo directly; when divisor is zero (Line 812), FreePascal raises a division-by-zero exception and aborts VM execution instead of returning a controlled language value.
🐛 Proposed fix
OP_MOD_INT: begin A := DecodeA(AInstruction); B := DecodeB(AInstruction); C := DecodeC(AInstruction); - FRegisters[Base + A] := SouffleInteger( - FRegisters[Base + B].AsInteger mod FRegisters[Base + C].AsInteger); + if FRegisters[Base + C].AsInteger = 0 then + FRegisters[Base + A] := SouffleFloat(NaN) + else + FRegisters[Base + A] := SouffleInteger( + FRegisters[Base + B].AsInteger mod FRegisters[Base + C].AsInteger); end;#!/bin/bash # Verify OP_MOD_INT currently has no zero-divisor guard. rg -n -A8 -B4 'OP_MOD_INT' souffle/Souffle.VM.pas🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@souffle/Souffle.VM.pas` around lines 806 - 813, OP_MOD_INT currently performs a raw integer modulo which will raise a division-by-zero exception when the divisor (FRegisters[Base + C].AsInteger) is zero; modify the OP_MOD_INT case to first read the divisor into a local (e.g., Divisor := FRegisters[Base + C].AsInteger) and if Divisor = 0 then assign a controlled value to the destination register (for example FRegisters[Base + A] := SouffleInteger(0) or another agreed sentinel) instead of computing the modulo, otherwise compute FRegisters[Base + A] := SouffleInteger(FRegisters[Base + B].AsInteger mod Divisor); ensure you reference AInstruction, DecodeA/DecodeB/DecodeC, Base and SouffleInteger when editing this block.
🧹 Nitpick comments (4)
units/Goccia.Engine.pas (1)
220-246: Make FPU mask restore exception-safe inDestroy.If teardown raises before Line 245, the previous mask is never restored. Wrap cleanup in
try..finallyso restoration always runs.Proposed refactor
destructor TGocciaEngine.Destroy; begin - if Assigned(TGocciaGarbageCollector.Instance) and Assigned(FInterpreter) then - TGocciaGarbageCollector.Instance.RemoveRoot(FInterpreter.GlobalScope); - - FBuiltinConsole.Free; - FBuiltinMath.Free; - FBuiltinGlobalObject.Free; - FBuiltinGlobalArray.Free; - FBuiltinGlobalNumber.Free; - FBuiltinGlobalString.Free; - FBuiltinGlobals.Free; - FBuiltinJSON.Free; - FBuiltinSymbol.Free; - FBuiltinSet.Free; - FBuiltinMap.Free; - FBuiltinPromise.Free; - FBuiltinTestAssertions.Free; - FBuiltinBenchmark.Free; - FBuiltinTemporal.Free; - FBuiltinArrayBuffer.Free; - - FInterpreter.Free; - if FOwnsResolver then - FResolver.Free; - SetExceptionMask(FPreviousExceptionMask); - inherited; + try + if Assigned(TGocciaGarbageCollector.Instance) and Assigned(FInterpreter) then + TGocciaGarbageCollector.Instance.RemoveRoot(FInterpreter.GlobalScope); + + FBuiltinConsole.Free; + FBuiltinMath.Free; + FBuiltinGlobalObject.Free; + FBuiltinGlobalArray.Free; + FBuiltinGlobalNumber.Free; + FBuiltinGlobalString.Free; + FBuiltinGlobals.Free; + FBuiltinJSON.Free; + FBuiltinSymbol.Free; + FBuiltinSet.Free; + FBuiltinMap.Free; + FBuiltinPromise.Free; + FBuiltinTestAssertions.Free; + FBuiltinBenchmark.Free; + FBuiltinTemporal.Free; + FBuiltinArrayBuffer.Free; + + FInterpreter.Free; + if FOwnsResolver then + FResolver.Free; + finally + SetExceptionMask(FPreviousExceptionMask); + inherited; + end; end;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@units/Goccia.Engine.pas` around lines 220 - 246, The destructor TGocciaEngine.Destroy currently frees resources and then restores the FPU exception mask, but if any cleanup raises the previous mask is never restored; wrap the entire teardown sequence (all Free calls, FInterpreter/Resolver cleanup, garbage collector removal) in a try..finally so that SetExceptionMask(FPreviousExceptionMask) is executed in the finally block; call inherited (the destructor) after restoring the mask (i.e., in the finally block after SetExceptionMask) to ensure proper unwind and guarantee the FPU mask is always restored even if frees raise exceptions.units/Goccia.Compiler.Expressions.pas (1)
829-829: Replace hardcoded keyword/property literals with project constants.Use constants for
constructorandthisto keep parser/compiler/runtime naming aligned.🔧 Suggested direction
+ uses + ... + Goccia.Constants.PropertyNames, + Goccia.Keywords.Reserved, + ... @@ - PropIdx := ACtx.Template.AddConstantString('constructor'); + PropIdx := ACtx.Template.AddConstantString(PROP_CONSTRUCTOR); @@ - ChildScope.DeclareLocal('this', False); + ChildScope.DeclareLocal(KEYWORD_THIS, False); @@ - LocalIdx := ACtx.Scope.ResolveLocal('this'); + LocalIdx := ACtx.Scope.ResolveLocal(KEYWORD_THIS); - UpvalIdx := ACtx.Scope.ResolveUpvalue('this'); + UpvalIdx := ACtx.Scope.ResolveUpvalue(KEYWORD_THIS);As per coding guidelines: "Use Goccia.Constants.PropertyNames constants (PROP_LENGTH, PROP_CONSTRUCTOR, etc.) instead of hardcoded string literals for property names" and "Use Goccia.Keywords.Reserved and Goccia.Keywords.Contextual constants (KEYWORD_THIS, KEYWORD_SUPER, KEYWORD_GET, etc.) instead of hardcoded keyword string literals".
Also applies to: 1082-1082, 1128-1128, 1760-1760, 1769-1769
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@units/Goccia.Compiler.Expressions.pas` at line 829, Replace hardcoded property/keyword string literals passed to ACtx.Template.AddConstantString (e.g., the 'constructor' literal assigned to PropIdx) with the appropriate project constants: use Goccia.Constants.PropertyNames.PROP_CONSTRUCTOR for "constructor" and Goccia.Keywords.Contextual.KEYWORD_THIS (or the appropriate KEYWORD_* constant from Goccia.Keywords.Reserved/Contextual) for "this" occurrences; update each AddConstantString call (including the spots referenced near PropIdx and the other locations mentioned) to pass the constant names instead of string literals so the parser/compiler/runtime use the centralized constants.units/Goccia.Compiler.pas (1)
314-319: Reset per-compilation state atCompileentry.Line 314 initializes a new module, but
FPendingClassNames,FPendingClasses, andFFormalParameterCountsare not cleared first. Reusing the same compiler instance can carry stale class names/counts into the next compile and alter deferral behavior.♻️ Proposed fix
function TGocciaCompiler.Compile( const AProgram: TGocciaProgram): TSouffleBytecodeModule; var I: Integer; @@ begin + FPendingClassNames.Clear; + SetLength(FPendingClasses, 0); + FFormalParameterCounts.Clear; + FModule := TSouffleBytecodeModule.Create(GOCCIA_RUNTIME_TAG, FSourcePath); FCurrentTemplate := TSouffleFunctionTemplate.Create('<module>');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@units/Goccia.Compiler.pas` around lines 314 - 319, At the start of the Compile entry (before creating FModule/TSouffleBytecodeModule and initializing FCurrentTemplate/FCurrentScope), reset the per-compilation state by clearing FPendingClassNames, FPendingClasses, and FFormalParameterCounts so they don't carry over from a previous run; locate the Compile method and add code to empty these fields (or recreate them) prior to the existing initialization of FModule, FCurrentTemplate, FCurrentScope, and the DeclareLocal call.units/Goccia.Compiler.Statements.pas (1)
758-758: Replace hardcoded keyword literals with keyword constants.
'this'is currently hardcoded in several scope operations. UseKEYWORD_THISfrom the keyword constants to keep compiler keyword handling centralized.As per coding guidelines: Use
Goccia.Keywords.ReservedandGoccia.Keywords.Contextualconstants (KEYWORD_THIS,KEYWORD_SUPER,KEYWORD_GET, etc.) instead of hardcoded keyword string literals.Also applies to: 837-837, 884-884, 1023-1024
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@units/Goccia.Compiler.Statements.pas` at line 758, Replace the hardcoded 'this' literal in scope operations with the keyword constant: change calls like ChildScope.DeclareLocal('this', False) to use KEYWORD_THIS (from Goccia.Keywords.Reserved or the appropriate keywords unit) and add the keywords unit to the uses clause if missing; do the same for the other occurrences noted (around the ChildScope.DeclareLocal / DeclareLocal/DeclareImplicit calls at the referenced locations) so all keyword literals (e.g., this, super, get) use the centralized KEYWORD_* constants.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/souffle-vm.md`:
- Around line 505-511: The fenced code block containing TSouffleArray,
TSouffleRecord, OP_ARRAY_GET and OP_RECORD_GET should include a language
identifier to satisfy markdownlint MD040; update the opening fence from ``` to
```text (or another appropriate label) so the block is labeled (e.g., change the
block that currently starts with ``` to ```text) while leaving the contents
unchanged.
In `@README.md`:
- Line 154: Update the README sentence about the Souffle VM to avoid overstating
parity: replace the “full GocciaScript language” and “100% of the test suite”
claims and instead add a concise caveats list referencing the known runtime
limitations (e.g., stubbed operations in TGocciaRuntimeOperations: GetIterator,
IteratorNext, SpreadInto, ImportModule, AwaitValue; closure receiver binding not
persisted), note .sbc uses native endianness (non-portable), and the ABC-encoded
instruction format limits constant pool references to 255 per prototype; keep
the link text “Souffle VM Architecture” but ensure the paragraph directs readers
to that doc for full details.
In `@souffle/Souffle.Compound.pas`:
- Around line 546-551: The loop in Delete (and likewise in DeleteChecked) calls
Move(FOrder[I + 1], FOrder[I], (FCount - I - 1) * SizeOf(Integer)) even when I =
FCount - 1, causing an out-of-bounds access with range checks enabled; update
the code in the methods that iterate FOrder (the Delete and DeleteChecked
routines) to only call Move when there are elements to move — e.g., guard the
Move call with a condition like (FCount - I - 1) > 0 or I < FCount - 1 — and
still call Dec(FCount) after removing the slot.
In `@TestRunner.dpr`:
- Around line 292-293: The new aggregate fields TotalCompileNanoseconds and
TotalExecNanoseconds are declared but never accumulated, so reporting still
reads compile time from TotalLexNanoseconds; update the code paths that process
per-file timings (where per-file compile and exec durations are computed) to add
those durations into TotalCompileNanoseconds and TotalExecNanoseconds
respectively, and change any bytecode/reporting logic that currently reads
TotalLexNanoseconds to use TotalCompileNanoseconds for compile-time metrics;
reference the fields TotalCompileNanoseconds, TotalExecNanoseconds and
TotalLexNanoseconds and the per-file timing accumulation sections around the
compile/exec loop to locate where to add the increments and adjust the
reporting.
In `@tests/language/expressions/arithmetic/increment.js`:
- Around line 30-38: Remove the decrement test block (the test titled "decrement
preserves fractional part" that uses variables x and y and the x--/--y
operations) from the increment test file and place an identical test into a new
decrement-focused test file named for decrement tests; ensure the original
increment file contains only increment-related tests and the new file contains
just this decrement test so each file targets a single operation.
In `@tests/language/expressions/destructuring/reassignment.js`:
- Around line 43-44: Add targeted Biome lint suppressions for the intentional
const reassignment assertions that verify runtime TypeError: insert a
"biome-ignore noConstAssign" suppression immediately above each expect that
reassigns a const (e.g., the expect(() => { x = 10; }).toThrow(TypeError); and
expect(() => { y = 20; }).toThrow(TypeError); lines, plus the similar
reassignment expect lines at the other two locations) so the local noConstAssign
rule is bypassed for those specific tests while leaving global linting intact.
In `@units/Goccia.Compiler.Expressions.pas`:
- Around line 1217-1224: GetPropertyNamesInOrder returns a TStringList that's
assigned to Names but never freed, causing a leak in the fallback
object-property compilation path; wrap the usage of Names (the result of
AExpr.GetPropertyNamesInOrder) in a try..finally and call Names.Free in the
finally block (ensure this surrounds the loop that uses Names and the calls to
AExpr.Properties.TryGetValue and CompileObjectProperty) so the list is always
released even on exceptions.
- Around line 743-770: The SwapState/child-scope setup and teardown around
function compilation (where ACtx.SwapState(ChildTemplate, ChildScope); ChildCtx
:= ACtx; ... ACtx.CompileFunctionBody(AExpr.Body); ...
ACtx.SwapState(OldTemplate, OldScope); ChildScope.Free) must be wrapped in a
try..finally so the original template/scope are restored and ChildScope is freed
even if compilation raises; move the body that calls EmitDefaultParameters,
EmitDestructuringParameters, ACtx.CompileFunctionBody, sets
ChildTemplate.MaxRegisters and adds upvalue descriptors into the try block, and
put ACtx.SwapState(OldTemplate, OldScope) and ChildScope.Free in the finally
block; apply the same try..finally pattern to the other function-emission paths
(arrow/method/getter/setter) referenced in the review.
- Around line 839-840: The UInt8(ArgCount) casts in the non-spread call emission
(e.g., the call emitting EncodeABC(OP_RT_CALL_METHOD, BaseReg, UInt8(ArgCount),
0) and the other similar EncodeABC/EncodeAD uses at the noted sites) can
silently truncate values >255; add an explicit guard before each UInt8 cast that
checks ArgCount <= 255 and handle the overflow (raise/emit a compile error or
switch to the spread-call path) so we never pass a truncated count into
EncodeABC/EncodeAD; update every occurrence referenced (the calls around
OP_RT_CALL_METHOD, OP_RT_CALL, OP_RT_CALL_CONSTRUCTOR, etc.) to perform this
check before casting.
In `@units/Goccia.Compiler.Statements.pas`:
- Around line 932-949: The code currently calls
ACtx.Scope.DeclareLocal('__super__', False) which creates a compiler-visible
lexical local; instead allocate a temporary/register slot for SuperReg (do not
introduce a named local) e.g. use the scope/register allocation API
(ACtx.Scope.AllocateRegister or ACtx.AllocRegister) to obtain SuperReg, use that
register for the subsequent ResolveLocal/ResolveUpvalue/EmitInstruction calls
(OP_MOVE, OP_GET_UPVALUE, OP_RT_GET_GLOBAL), and then release the temporary
(ACtx.Scope.ReleaseRegister/ACtx.FreeRegister) when class setup is done so no
named '__super__' binding or extra persistent local slot is introduced.
- Around line 1087-1089: The move instruction is using
ACtx.Scope.ResolveLocal(AStmt.Name) which returns a local-table index, not the
VM register slot; replace that with the actual slot assigned to the declared
local (use the slot returned/available from ACtx.Scope.DeclareLocal or a
ResolveSlot-style accessor) so EmitInstruction(ACtx, EncodeABC(OP_MOVE,
<declared-slot-for-AStmt.Name>, EnumSlot, 0)) writes to the correct VM register;
update the call around ACtx.Scope.DeclareLocal(AStmt.Name, False) to
capture/lookup the declared slot and use it instead of
ACtx.Scope.ResolveLocal(AStmt.Name).
- Around line 1036-1037: The code casts APendingIndex to UInt8 when emitting the
RT_EXT instruction which will truncate values >255; before calling
EmitInstruction/EncodeABC for OP_RT_EXT with ClassReg and GOCCIA_EXT_EVAL_CLASS,
add a guard that checks APendingIndex <= High(UInt8) and handle the overflow
(raise a compiler/assertion error or choose an alternative encoding path) so the
pending-class index cannot be silently truncated; reference EmitInstruction,
EncodeABC, OP_RT_EXT, ClassReg, GOCCIA_EXT_EVAL_CLASS and APendingIndex when
adding this range check and error-handling.
- Around line 713-718: GPendingFinally is still live while CompileBreakStatement
emits break-finally blocks which allows re-entrant finally code to
mutate/duplicate the pending list; to fix, detach the range of pending finally
entries before compiling by taking the slice from GPendingFinally (indices
GBreakFinallyBase..Count-1) into a local list/array, reduce
GPendingFinally.Count to GBreakFinallyBase (or otherwise mark them removed) and
then iterate the local copy calling EmitInstruction(ACtx,
EncodeABC(OP_POP_HANDLER,...)) and CompileBlockStatement(ACtx,
<local>.Items[I].FinallyBlock); this prevents re-entry into the original
GPendingFinally while emitting.
In `@units/Goccia.Evaluator.Bitwise.pas`:
- Around line 43-47: Add the required ECMAScript spec annotation comment
immediately above the EvaluateRightShift function: insert a single-line comment
in the format // ESYYYY §X.Y.Z EvaluateRightShift(ALeft, ARight) (replace YYYY
and §X.Y.Z with the correct edition year and section number for the
right-shift/ShiftExpression semantics) so the function is annotated per the
coding guideline; ensure the comment sits directly above the function
declaration for EvaluateRightShift.
---
Duplicate comments:
In `@docs/souffle-vm.md`:
- Around line 1124-1129: The documentation currently contradicts itself about
.sbc portability: update the "Portable binary format" bullet and/or its
surrounding text so it reflects the endianness section (serialization is
native-endian and requires normalization for cross-platform use). Replace the
claim "portable" with a clear statement that .sbc is self-describing but uses
native endianness (or add an explicit caveat), and add a cross-reference to the
endianness/serialization section to guide readers (referencing the `.sbc`
format, the "Portable binary format" bullet, and the endianness/serialization
section).
In `@souffle/Souffle.VM.pas`:
- Around line 806-813: OP_MOD_INT currently performs a raw integer modulo which
will raise a division-by-zero exception when the divisor (FRegisters[Base +
C].AsInteger) is zero; modify the OP_MOD_INT case to first read the divisor into
a local (e.g., Divisor := FRegisters[Base + C].AsInteger) and if Divisor = 0
then assign a controlled value to the destination register (for example
FRegisters[Base + A] := SouffleInteger(0) or another agreed sentinel) instead of
computing the modulo, otherwise compute FRegisters[Base + A] :=
SouffleInteger(FRegisters[Base + B].AsInteger mod Divisor); ensure you reference
AInstruction, DecodeA/DecodeB/DecodeC, Base and SouffleInteger when editing this
block.
In `@units/Goccia.Compiler.ConstantFolding.pas`:
- Around line 43-44: The folded-integer fast path in EmitFoldedNumber uses
hardcoded -32768..32767 bounds that can mismatch EncodeAsBx; change
EmitFoldedNumber to use the same signed-range constants used by EncodeAsBx (the
AsBx encoder limits) when deciding the fast-path so edge values are encoded
consistently, and keep the EmitInstruction call to EncodeAsBx(OP_LOAD_INT,
ADest, Int16(Trunc(AValue))) unchanged aside from using those shared constants.
In `@units/Goccia.Compiler.Expressions.pas`:
- Around line 147-150: The fast-path for OP_LOAD_INT currently uses hardcoded
-32768/32767 bounds; instead use the same signed sBx range constants used by the
AsBx encoder/decoder (the constants referenced by EncodeAsBx/DecodeAsBx) to
decide the branch and to cast the literal (replace Int16(Trunc(...)) with a cast
sized to that sBx range). Update the condition that checks
TGocciaNumberLiteralValue(Value).Value to compare against the encoder's MIN_SBX
and MAX_SBX (or whatever named constants the EncodeAsBx implementation exposes),
and ensure EmitInstruction(ACtx, EncodeAsBx(OP_LOAD_INT, ADest, <casted sBx
value>)) uses that same constant-derived range and cast.
In `@units/Goccia.Values.ObjectValue.pas`:
- Around line 514-515: Replace RTTI checks that test accessor callability via
"Accessor.Setter is TGocciaFunctionBase" (and similar checks for
Accessor.Getter) with the VMT contract call IsCallable on the Value/Accessor
(e.g., use Accessor.Setter.IsCallable or Value.IsCallable) so guards use the
canonical IsCallable predicate; update all the other occurrences noted (the
analogous guards at the sites flagged around 548-549, 649-650, 677-678) to
follow the same pattern, ensuring you check for Assigned(...) before calling
IsCallable to avoid nil dereference.
---
Nitpick comments:
In `@units/Goccia.Compiler.Expressions.pas`:
- Line 829: Replace hardcoded property/keyword string literals passed to
ACtx.Template.AddConstantString (e.g., the 'constructor' literal assigned to
PropIdx) with the appropriate project constants: use
Goccia.Constants.PropertyNames.PROP_CONSTRUCTOR for "constructor" and
Goccia.Keywords.Contextual.KEYWORD_THIS (or the appropriate KEYWORD_* constant
from Goccia.Keywords.Reserved/Contextual) for "this" occurrences; update each
AddConstantString call (including the spots referenced near PropIdx and the
other locations mentioned) to pass the constant names instead of string literals
so the parser/compiler/runtime use the centralized constants.
In `@units/Goccia.Compiler.pas`:
- Around line 314-319: At the start of the Compile entry (before creating
FModule/TSouffleBytecodeModule and initializing FCurrentTemplate/FCurrentScope),
reset the per-compilation state by clearing FPendingClassNames, FPendingClasses,
and FFormalParameterCounts so they don't carry over from a previous run; locate
the Compile method and add code to empty these fields (or recreate them) prior
to the existing initialization of FModule, FCurrentTemplate, FCurrentScope, and
the DeclareLocal call.
In `@units/Goccia.Compiler.Statements.pas`:
- Line 758: Replace the hardcoded 'this' literal in scope operations with the
keyword constant: change calls like ChildScope.DeclareLocal('this', False) to
use KEYWORD_THIS (from Goccia.Keywords.Reserved or the appropriate keywords
unit) and add the keywords unit to the uses clause if missing; do the same for
the other occurrences noted (around the ChildScope.DeclareLocal /
DeclareLocal/DeclareImplicit calls at the referenced locations) so all keyword
literals (e.g., this, super, get) use the centralized KEYWORD_* constants.
In `@units/Goccia.Engine.pas`:
- Around line 220-246: The destructor TGocciaEngine.Destroy currently frees
resources and then restores the FPU exception mask, but if any cleanup raises
the previous mask is never restored; wrap the entire teardown sequence (all Free
calls, FInterpreter/Resolver cleanup, garbage collector removal) in a
try..finally so that SetExceptionMask(FPreviousExceptionMask) is executed in the
finally block; call inherited (the destructor) after restoring the mask (i.e.,
in the finally block after SetExceptionMask) to ensure proper unwind and
guarantee the FPU mask is always restored even if frees raise exceptions.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (23)
AGENTS.mdREADME.mdScriptLoader.dprTestRunner.dprdocs/design-decisions.mddocs/souffle-vm.mdsouffle/Souffle.Compound.passouffle/Souffle.VM.pastests/language/expressions/arithmetic/increment.jstests/language/expressions/bitwise/bitwise-rightshift.jstests/language/expressions/destructuring/reassignment.jstests/language/for-of/for-of-break.jsunits/Goccia.Compiler.ConstantFolding.pasunits/Goccia.Compiler.Expressions.pasunits/Goccia.Compiler.Statements.pasunits/Goccia.Compiler.pasunits/Goccia.Engine.pasunits/Goccia.Evaluator.Bitwise.pasunits/Goccia.Evaluator.Comparison.pasunits/Goccia.Values.ArrayValue.pasunits/Goccia.Values.FunctionValue.pasunits/Goccia.Values.ObjectValue.pasunits/Goccia.Values.Primitives.pas
Comment on lines
+292
to
293
| TotalCompileNanoseconds: Int64; | ||
| TotalExecNanoseconds: Int64; |
Contributor
There was a problem hiding this comment.
TotalCompileNanoseconds is introduced but never populated from per-file timing.
The aggregate compile counter stays 0, and bytecode reporting still reads compile time from TotalLexNanoseconds. This makes the new compile metric inconsistent.
🔧 Suggested fix
function RunScriptFromFile(const AFileName: string): TAggregatedTestResult;
begin
@@
Result.TotalLexNanoseconds := FileResult.Timing.LexTimeNanoseconds;
Result.TotalParseNanoseconds := FileResult.Timing.ParseTimeNanoseconds;
+ Result.TotalCompileNanoseconds := FileResult.Timing.LexTimeNanoseconds;
Result.TotalExecNanoseconds := FileResult.Timing.ExecuteTimeNanoseconds;
@@
if GMode = ebSouffleVM then
Writeln(Format('Test Results Engine Timing: Compile: %s | Execute: %s | Total: %s',
- [FormatDuration(AResult.TotalLexNanoseconds), FormatDuration(AResult.TotalExecNanoseconds),
- FormatDuration(AResult.TotalLexNanoseconds + AResult.TotalExecNanoseconds)]))
+ [FormatDuration(AResult.TotalCompileNanoseconds), FormatDuration(AResult.TotalExecNanoseconds),
+ FormatDuration(AResult.TotalCompileNanoseconds + AResult.TotalExecNanoseconds)]))Also applies to: 303-311, 432-435
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@TestRunner.dpr` around lines 292 - 293, The new aggregate fields
TotalCompileNanoseconds and TotalExecNanoseconds are declared but never
accumulated, so reporting still reads compile time from TotalLexNanoseconds;
update the code paths that process per-file timings (where per-file compile and
exec durations are computed) to add those durations into TotalCompileNanoseconds
and TotalExecNanoseconds respectively, and change any bytecode/reporting logic
that currently reads TotalLexNanoseconds to use TotalCompileNanoseconds for
compile-time metrics; reference the fields TotalCompileNanoseconds,
TotalExecNanoseconds and TotalLexNanoseconds and the per-file timing
accumulation sections around the compile/exec loop to locate where to add the
increments and adjust the reporting.
Comment on lines
+30
to
+38
| test("decrement preserves fractional part", () => { | ||
| let x = 2.5; | ||
| x--; | ||
| expect(x).toBe(1.5); | ||
|
|
||
| let y = 0.5; | ||
| --y; | ||
| expect(y).toBe(-0.5); | ||
| }); |
Contributor
There was a problem hiding this comment.
Keep decrement tests in a dedicated decrement test file.
increment.js should stay focused on increment-only behavior. Please move this decrement block to a decrement-focused file so each file targets one operation.
Suggested change
- test("decrement preserves fractional part", () => {
- let x = 2.5;
- x--;
- expect(x).toBe(1.5);
-
- let y = 0.5;
- --y;
- expect(y).toBe(-0.5);
- });// tests/language/expressions/arithmetic/decrement.js
test("decrement preserves fractional part", () => {
let x = 2.5;
x--;
expect(x).toBe(1.5);
let y = 0.5;
--y;
expect(y).toBe(-0.5);
});As per coding guidelines: "Each test file must focus on a single method or operation (one method per file); never bundle multiple methods into a single file."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| test("decrement preserves fractional part", () => { | |
| let x = 2.5; | |
| x--; | |
| expect(x).toBe(1.5); | |
| let y = 0.5; | |
| --y; | |
| expect(y).toBe(-0.5); | |
| }); | |
| // (All increment-related tests remain, decrement test removed) | |
| // The file would contain only increment operator tests |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/language/expressions/arithmetic/increment.js` around lines 30 - 38,
Remove the decrement test block (the test titled "decrement preserves fractional
part" that uses variables x and y and the x--/--y operations) from the increment
test file and place an identical test into a new decrement-focused test file
named for decrement tests; ensure the original increment file contains only
increment-related tests and the new file contains just this decrement test so
each file targets a single operation.
Comment on lines
+713
to
+718
| if Assigned(GPendingFinally) and (GPendingFinally.Count > GBreakFinallyBase) then | ||
| for I := GPendingFinally.Count - 1 downto GBreakFinallyBase do | ||
| begin | ||
| EmitInstruction(ACtx, EncodeABC(OP_POP_HANDLER, 0, 0, 0)); | ||
| CompileBlockStatement(ACtx, GPendingFinally[I].FinallyBlock); | ||
| end; |
Contributor
There was a problem hiding this comment.
Detach GPendingFinally while emitting break-finally blocks.
CompileBreakStatement compiles finally blocks while GPendingFinally is still active. If a finally block contains abrupt control flow, it can re-enter the same pending-finally path and duplicate/loop emission.
💡 Suggested fix
procedure CompileBreakStatement(const ACtx: TGocciaCompilationContext);
var
I: Integer;
+ SavedFinally: TList<TPendingFinallyEntry>;
begin
if not Assigned(GBreakJumps) then
Exit;
if Assigned(GPendingFinally) and (GPendingFinally.Count > GBreakFinallyBase) then
- for I := GPendingFinally.Count - 1 downto GBreakFinallyBase do
- begin
- EmitInstruction(ACtx, EncodeABC(OP_POP_HANDLER, 0, 0, 0));
- CompileBlockStatement(ACtx, GPendingFinally[I].FinallyBlock);
- end;
+ begin
+ SavedFinally := GPendingFinally;
+ GPendingFinally := nil;
+ try
+ for I := SavedFinally.Count - 1 downto GBreakFinallyBase do
+ begin
+ EmitInstruction(ACtx, EncodeABC(OP_POP_HANDLER, 0, 0, 0));
+ CompileBlockStatement(ACtx, SavedFinally[I].FinallyBlock);
+ end;
+ finally
+ GPendingFinally := SavedFinally;
+ end;
+ end;
GBreakJumps.Add(EmitJumpInstruction(ACtx, OP_JUMP, 0));
end;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if Assigned(GPendingFinally) and (GPendingFinally.Count > GBreakFinallyBase) then | |
| for I := GPendingFinally.Count - 1 downto GBreakFinallyBase do | |
| begin | |
| EmitInstruction(ACtx, EncodeABC(OP_POP_HANDLER, 0, 0, 0)); | |
| CompileBlockStatement(ACtx, GPendingFinally[I].FinallyBlock); | |
| end; | |
| procedure CompileBreakStatement(const ACtx: TGocciaCompilationContext); | |
| var | |
| I: Integer; | |
| SavedFinally: TList<TPendingFinallyEntry>; | |
| begin | |
| if not Assigned(GBreakJumps) then | |
| Exit; | |
| if Assigned(GPendingFinally) and (GPendingFinally.Count > GBreakFinallyBase) then | |
| begin | |
| SavedFinally := GPendingFinally; | |
| GPendingFinally := nil; | |
| try | |
| for I := SavedFinally.Count - 1 downto GBreakFinallyBase do | |
| begin | |
| EmitInstruction(ACtx, EncodeABC(OP_POP_HANDLER, 0, 0, 0)); | |
| CompileBlockStatement(ACtx, SavedFinally[I].FinallyBlock); | |
| end; | |
| finally | |
| GPendingFinally := SavedFinally; | |
| end; | |
| end; | |
| GBreakJumps.Add(EmitJumpInstruction(ACtx, OP_JUMP, 0)); | |
| end; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@units/Goccia.Compiler.Statements.pas` around lines 713 - 718, GPendingFinally
is still live while CompileBreakStatement emits break-finally blocks which
allows re-entrant finally code to mutate/duplicate the pending list; to fix,
detach the range of pending finally entries before compiling by taking the slice
from GPendingFinally (indices GBreakFinallyBase..Count-1) into a local
list/array, reduce GPendingFinally.Count to GBreakFinallyBase (or otherwise mark
them removed) and then iterate the local copy calling EmitInstruction(ACtx,
EncodeABC(OP_POP_HANDLER,...)) and CompileBlockStatement(ACtx,
<local>.Items[I].FinallyBlock); this prevents re-entry into the original
GPendingFinally while emitting.
Comment on lines
+932
to
+949
| if HasSuper then | ||
| begin | ||
| SuperReg := ACtx.Scope.DeclareLocal('__super__', False); | ||
|
|
||
| LocalIdx := ACtx.Scope.ResolveLocal(ClassDef.SuperClass); | ||
| if LocalIdx >= 0 then | ||
| EmitInstruction(ACtx, EncodeABC(OP_MOVE, SuperReg, | ||
| ACtx.Scope.GetLocal(LocalIdx).Slot, 0)) | ||
| else | ||
| begin | ||
| UpvalIdx := ACtx.Scope.ResolveUpvalue(ClassDef.SuperClass); | ||
| if UpvalIdx >= 0 then | ||
| EmitInstruction(ACtx, EncodeABx(OP_GET_UPVALUE, SuperReg, | ||
| UInt16(UpvalIdx))) | ||
| else | ||
| EmitInstruction(ACtx, EncodeABx(OP_RT_GET_GLOBAL, SuperReg, | ||
| ACtx.Template.AddConstantString(ClassDef.SuperClass))); | ||
| end; |
Contributor
There was a problem hiding this comment.
Avoid declaring __super__ as a lexical local.
Using DeclareLocal('__super__', ...) introduces a compiler-internal binding into user scope and consumes a local slot beyond class setup needs.
💡 Suggested fix
if HasSuper then
begin
- SuperReg := ACtx.Scope.DeclareLocal('__super__', False);
+ SuperReg := ACtx.Scope.AllocateRegister;
+ try
LocalIdx := ACtx.Scope.ResolveLocal(ClassDef.SuperClass);
if LocalIdx >= 0 then
EmitInstruction(ACtx, EncodeABC(OP_MOVE, SuperReg,
ACtx.Scope.GetLocal(LocalIdx).Slot, 0))
else
begin
UpvalIdx := ACtx.Scope.ResolveUpvalue(ClassDef.SuperClass);
if UpvalIdx >= 0 then
EmitInstruction(ACtx, EncodeABx(OP_GET_UPVALUE, SuperReg,
UInt16(UpvalIdx)))
else
EmitInstruction(ACtx, EncodeABx(OP_RT_GET_GLOBAL, SuperReg,
ACtx.Template.AddConstantString(ClassDef.SuperClass)));
end;
EmitInstruction(ACtx, EncodeABC(OP_INHERIT, ClassReg, SuperReg, 0));
+ finally
+ ACtx.Scope.FreeRegister;
+ end;
end;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@units/Goccia.Compiler.Statements.pas` around lines 932 - 949, The code
currently calls ACtx.Scope.DeclareLocal('__super__', False) which creates a
compiler-visible lexical local; instead allocate a temporary/register slot for
SuperReg (do not introduce a named local) e.g. use the scope/register allocation
API (ACtx.Scope.AllocateRegister or ACtx.AllocRegister) to obtain SuperReg, use
that register for the subsequent ResolveLocal/ResolveUpvalue/EmitInstruction
calls (OP_MOVE, OP_GET_UPVALUE, OP_RT_GET_GLOBAL), and then release the
temporary (ACtx.Scope.ReleaseRegister/ACtx.FreeRegister) when class setup is
done so no named '__super__' binding or extra persistent local slot is
introduced.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
souffle/Souffle.VM.pas (2)
169-185:⚠️ Potential issue | 🟠 Major
FBaseFrameCountrestoration is not exception-safe for non-ESouffleThrowfailures.If another exception type escapes
ExecuteLoop,FBaseFrameCountremains corrupted for subsequent executions.Suggested fix
SavedBaseFrameCount := FBaseFrameCount; FBaseFrameCount := FCallStack.Count; - try - ExecuteLoop; - except - on E: ESouffleThrow do - begin - while FCallStack.Count >= FBaseFrameCount do - begin - CloseUpvalues(FCallStack.Peek^.BaseRegister); - FCallStack.Pop; - end; - FBaseFrameCount := SavedBaseFrameCount; - raise; - end; - end; - - FBaseFrameCount := SavedBaseFrameCount; + try + try + ExecuteLoop; + except + on E: ESouffleThrow do + begin + while FCallStack.Count >= FBaseFrameCount do + begin + CloseUpvalues(FCallStack.Peek^.BaseRegister); + FCallStack.Pop; + end; + raise; + end; + end; + finally + FBaseFrameCount := SavedBaseFrameCount; + end;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@souffle/Souffle.VM.pas` around lines 169 - 185, FBaseFrameCount is only restored for ESouffleThrow exceptions, leaving it corrupted on other exceptions; make the restoration exception-safe by moving the assignment FBaseFrameCount := SavedBaseFrameCount into a finally block that always runs after ExecuteLoop (while keeping the ESouffleThrow-specific cleanup in its except handler), ensuring CloseUpvalues/FCallStack cleanup still occurs on ESouffleThrow and that Result := FRegisters[Base] runs after the finally; update references around ExecuteLoop, ESouffleThrow, FBaseFrameCount, SavedBaseFrameCount, CloseUpvalues and FCallStack accordingly.
1625-1647:⚠️ Potential issue | 🟠 MajorRoot marking is incomplete for delegates and spilled args.
Two problems in this block:
- Early exit on empty call stack skips
FArrayDelegate/FRecordDelegateroots.- Scan high-water uses only
Template.MaxRegisters, butExecuteFunctioncan spill extra args above that window.Both can lead to collectible live references.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@souffle/Souffle.VM.pas` around lines 1625 - 1647, The root-marking exits early when FCallStack.IsEmpty which skips FArrayDelegate/FRecordDelegate; move the delegate checks (FArrayDelegate, FRecordDelegate) and FOpenUpvalues check before the early Exit so they are always considered; also replace the current HighWater calculation (FCallStack.Peek^.BaseRegister + FCallStack.Peek^.Template.MaxRegisters) with a computation that considers spilled arguments produced by ExecuteFunction — e.g., iterate all frames in FCallStack to compute HighWater = max(BaseRegister + Template.MaxRegisters + Frame^.SpilledArgsCount) (or the actual spilled-args field name on your Frame) and then mark FRegisters[0..HighWater-1]; keep marking closures and upvalues as before.
♻️ Duplicate comments (4)
units/Goccia.Compiler.Expressions.pas (2)
1226-1233:⚠️ Potential issue | 🟡 Minor
TStringListfromGetPropertyNamesInOrderstill isn’t released.This fallback path leaks
Nameswithout atry..finally.Suggested fix
begin Names := AExpr.GetPropertyNamesInOrder; - for I := 0 to Names.Count - 1 do - begin - Key := Names[I]; - if AExpr.Properties.TryGetValue(Key, ValExpr) then - CompileObjectProperty(ACtx, AExpr, ADest, Key, ValExpr); - end; + try + for I := 0 to Names.Count - 1 do + begin + Key := Names[I]; + if AExpr.Properties.TryGetValue(Key, ValExpr) then + CompileObjectProperty(ACtx, AExpr, ADest, Key, ValExpr); + end; + finally + Names.Free; + end; end;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@units/Goccia.Compiler.Expressions.pas` around lines 1226 - 1233, The TStringList returned by GetPropertyNamesInOrder (assigned to Names) is not freed in the fallback loop and leaks; wrap the usage of Names in a try..finally block (immediately after Names := AExpr.GetPropertyNamesInOrder) and call Names.Free in the finally, keeping the existing loop that calls CompileObjectProperty(ACtx, AExpr, ADest, Key, ValExpr) inside the try so Name access is safe and properly released.
822-825:⚠️ Potential issue | 🟠 MajorSpread calls are rejected too early by the
>255guard.The count guard runs before
UseSpreadis evaluated, so spread-mode calls can fail even though they don’t encodeArgCountin aUInt8operand.Suggested fix
ArgCount := AExpr.Arguments.Count; - if ArgCount > High(UInt8) then - raise Exception.Create('Compiler error: too many arguments (>255)'); UseSpread := HasSpreadArgument(AExpr); + if (not UseSpread) and (ArgCount > High(UInt8)) then + raise Exception.Create('Compiler error: too many arguments (>255)');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@units/Goccia.Compiler.Expressions.pas` around lines 822 - 825, The guard that raises for ArgCount > High(UInt8) runs before computing UseSpread, causing spread calls to be rejected; modify the logic around ArgCount := AExpr.Arguments.Count and UseSpread := HasSpreadArgument(AExpr) so that UseSpread is determined first (call HasSpreadArgument(AExpr) before enforcing the UInt8 limit) and only apply the ArgCount > High(UInt8) check when UseSpread is false (or otherwise ensure spread-mode bypasses the UInt8 argument-count restriction); update the block containing ArgCount, UseSpread and the exception so spread-mode calls are not incorrectly barred.units/Goccia.Compiler.Statements.pas (2)
714-719:⚠️ Potential issue | 🟠 MajorDetach pending-finally state while emitting break-finally blocks.
Compiling finally blocks with
GPendingFinallystill live allows re-entrant mutation/duplication of the same pending list.🛠️ Suggested fix
procedure CompileBreakStatement(const ACtx: TGocciaCompilationContext); var I: Integer; + SavedFinally: TList<TPendingFinallyEntry>; begin if not Assigned(GBreakJumps) then Exit; if Assigned(GPendingFinally) and (GPendingFinally.Count > GBreakFinallyBase) then - for I := GPendingFinally.Count - 1 downto GBreakFinallyBase do - begin - EmitInstruction(ACtx, EncodeABC(OP_POP_HANDLER, 0, 0, 0)); - CompileBlockStatement(ACtx, GPendingFinally[I].FinallyBlock); - end; + begin + SavedFinally := GPendingFinally; + GPendingFinally := nil; + try + for I := SavedFinally.Count - 1 downto GBreakFinallyBase do + begin + EmitInstruction(ACtx, EncodeABC(OP_POP_HANDLER, 0, 0, 0)); + CompileBlockStatement(ACtx, SavedFinally[I].FinallyBlock); + end; + finally + GPendingFinally := SavedFinally; + end; + end; GBreakJumps.Add(EmitJumpInstruction(ACtx, OP_JUMP, 0)); end;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@units/Goccia.Compiler.Statements.pas` around lines 714 - 719, The loop that emits break-finally blocks currently iterates directly over the shared GPendingFinally list (symbols: GPendingFinally, GBreakFinallyBase, CompileBlockStatement, EmitInstruction, OP_POP_HANDLER), which allows re-entrant mutations/duplication; fix it by detaching a snapshot of the pending-finally entries before emitting: create a local copy/array of the entries from index GBreakFinallyBase..GPendingFinally.Count-1, then iterate that local snapshot to call EmitInstruction and CompileBlockStatement, leaving the original GPendingFinally intact during emission so re-entrancy cannot mutate the active iteration.
933-953:⚠️ Potential issue | 🟠 MajorDon’t materialize
__super__as a lexical local.This introduces a compiler-internal binding into user scope and retains an unnecessary local slot after class setup.
🛠️ Suggested fix
if HasSuper then begin - SuperReg := ACtx.Scope.DeclareLocal('__super__', False); + SuperReg := ACtx.Scope.AllocateRegister; + try LocalIdx := ACtx.Scope.ResolveLocal(ClassDef.SuperClass); if LocalIdx >= 0 then EmitInstruction(ACtx, EncodeABC(OP_MOVE, SuperReg, ACtx.Scope.GetLocal(LocalIdx).Slot, 0)) @@ end; EmitInstruction(ACtx, EncodeABC(OP_INHERIT, ClassReg, SuperReg, 0)); + finally + ACtx.Scope.FreeRegister; + end; end;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@units/Goccia.Compiler.Statements.pas` around lines 933 - 953, The code incorrectly introduces a user-visible lexical local by calling ACtx.Scope.DeclareLocal('__super__', False) inside HasSuper; instead allocate a compiler-only temporary for SuperReg (do not call DeclareLocal with '__super__'), use the scope/register temp API (e.g. an Allocate/PushTemp or AllocateRegister function on ACtx.Scope) to hold the superclass value, emit the same OP_MOVE/OP_GET_UPVALUE/OP_RT_GET_GLOBAL into that temporary, emit OP_INHERIT with ClassReg and the temp, then release/free the temporary (e.g. FreeTemp/PopTemp) so no persistent local slot or user binding remains. Ensure all references to SuperReg (and the Replace of DeclareLocal('__super__', False)) are updated accordingly.
🧹 Nitpick comments (3)
docs/embedding.md (1)
462-465: Consider adding context about FPU exceptions for embedders.The section assumes embedders understand what "FPU exceptions" are and why masking them matters. Consider briefly explaining what FPU exceptions are (e.g., divide-by-zero, overflow, underflow, invalid operation) and providing a practical example of when an embedder might need to be aware of this behavior.
For instance, if the host application has set specific FPU exception handlers or relies on FPU exceptions being enabled, they should be aware that creating a long-lived engine will suppress these exceptions for the engine's lifetime.
📝 Suggested enhancement for clarity
## FPU Exception Mask -Both `TGocciaEngine` and `TSouffleVM` mask all FPU exceptions on creation (via `SetExceptionMask`) to enable IEEE 754 semantics (`NaN`, `Infinity`, `-0`). The previous mask is saved in the constructor and **restored in the destructor**, so the host application's FPU state is not permanently altered. This is transparent for one-shot execution (`RunScript`), but embedders creating long-lived engine instances should be aware that FPU exceptions are suppressed while the engine exists. +Both `TGocciaEngine` and `TSouffleVM` mask all FPU exceptions (divide-by-zero, overflow, underflow, invalid operation) on creation (via `SetExceptionMask`) to enable IEEE 754 semantics (`NaN`, `Infinity`, `-0`). The previous mask is saved in the constructor and **restored in the destructor**, so the host application's FPU state is not permanently altered. + +This is transparent for one-shot execution (`RunScript`), but embedders creating long-lived engine instances should be aware that FPU exceptions are suppressed while the engine exists. If your host application relies on FPU exception handling (e.g., for numerical computation validation), consider creating short-lived engines or managing engine lifecycles around critical FPU operations.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/embedding.md` around lines 462 - 465, Add a brief explanatory paragraph to the "FPU Exception Mask" section clarifying what FPU exceptions are (e.g., divide-by-zero, overflow, underflow, invalid operation), why masking them enables IEEE 754 semantics, and a short practical note for embedders (for example: if the host uses FPU exception handlers or depends on exceptions for error handling, creating long-lived TGocciaEngine or TSouffleVM instances that call SetExceptionMask will suppress those exceptions until the engine is destroyed; one-shot RunScript usage is unaffected). Reference TGocciaEngine, TSouffleVM, SetExceptionMask and RunScript in the text so readers can locate the related behavior.units/Goccia.Values.ObjectValue.pas (1)
669-698: Receiver preservation across prototype chain is now correct.The prototype lookup correctly preserves
AThisContextwhen invoking inherited accessors (Line 681) and delegates toGetPropertyWithContext(Line 696) for deeper chain lookups.Optional simplification: The explicit
GetOwnPropertyDescriptorcheck on the prototype (lines 671-695) before falling back toGetPropertyWithContextis functionally redundant—the recursive call would perform the same lookup. Consider simplifying to just:if Assigned(FPrototype) then begin Result := FPrototype.GetPropertyWithContext(AName, AThisContext); Exit; end;This would reduce code duplication while maintaining correct receiver semantics.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@units/Goccia.Values.ObjectValue.pas` around lines 669 - 698, The prototype-lookup block is redundant: remove the explicit GetOwnPropertyDescriptor handling (the code that checks FPrototype.GetOwnPropertyDescriptor, TGocciaPropertyDescriptorAccessor and TGocciaPropertyDescriptorData) and replace the entire conditional body with a single delegation to FPrototype.GetPropertyWithContext(AName, AThisContext); ensure you still check Assigned(FPrototype) and Exit after calling GetPropertyWithContext so receiver semantics via AThisContext are preserved.README.md (1)
154-154: Improved but consider a minor refinement.The updated text is more accurate than before — it now acknowledges the bridge layer delegation and lists the known structural limitations. However, the phrasing "passes 100% of the test suite (3,358 tests)" followed by "All language features work through a bridge layer" may still be slightly confusing.
Consider a small reordering for clarity:
📝 Suggested wording adjustment
-GocciaScript includes an alternative bytecode execution backend — the **Souffle VM** — a general-purpose register-based virtual machine. The bytecode backend passes 100% of the test suite (3,358 tests). All language features work through a bridge layer that delegates to the GocciaScript evaluator where needed. Known structural limitations: `.sbc` files use native endianness (not yet cross-platform portable); the ABC-encoded instruction format limits constant pool references to 255 per prototype. See [Souffle VM Architecture](docs/souffle-vm.md) for full details. +GocciaScript includes an alternative bytecode execution backend — the **Souffle VM** — a general-purpose register-based virtual machine. All language features work through a bridge layer that delegates to the GocciaScript evaluator where needed, passing 100% of the test suite (3,358 tests). Known structural limitations: `.sbc` files use native endianness (not yet cross-platform portable); the ABC-encoded instruction format limits constant pool references to 255 per prototype. See [Souffle VM Architecture](docs/souffle-vm.md) for full details.This reframing leads with the bridge architecture and positions the test coverage as validation of that approach.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` at line 154, Reorder the sentence in the README paragraph about the Souffle VM to lead with the bridge architecture and then state the test coverage as validation: start by mentioning that the Souffle VM is a register-based VM that routes language features through a bridge layer to the GocciaScript evaluator, then note that this approach is validated by passing 100% of the test suite (3,358 tests), and finally list known structural limitations (e.g., .sbc native endianness and ABC-encoded instruction format limiting constant pool references to 255 per prototype); update the paragraph containing "Souffle VM", ".sbc", and "ABC-encoded" accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/souffle-vm.md`:
- Around line 121-137: Update the opening sentence to correctly reflect the
table: change “45 methods + 1 extension entry point” to phrasing that shows the
table totals 45 methods including the extension, e.g. “45 methods (including the
ExtendedOperation extension entry point)”; reference the ExtendedOperation
symbol to ensure the extension entry is described as included rather than added
on.
In `@souffle/Souffle.Compound.pas`:
- Around line 749-757: The destructor TSouffleBlueprint.Destroy forgets to free
the lazily-allocated FPrototype (created by GetPrototype), causing a memory
leak; modify Destroy to check and Free (or FreeAndNil) FPrototype before calling
inherited (or include it in the existing frees), ensuring FPrototype is properly
released alongside FMethods, FGetters, FSetters, FStaticGetters, FStaticSetters,
and FStaticFields.
- Around line 386-395: Grow currently rebuilds FOrder by scanning buckets
(0..OldCapacity-1) which loses insertion order; instead iterate over the
previous order array and reinsert in that sequence. In the Grow routine (where
OldEntries, OldCapacity and FindEntry are used) use the previous order buffer
(e.g. OldOrder and OldCount) and loop from 0 to OldCount-1, get OldSlot :=
OldOrder[i], then rehash OldEntries[OldSlot] via
FindEntry(OldEntries[OldSlot].Key, OldEntries[OldSlot].Hash), assign into
FEntries[Slot], set Deleted := False, and append Slot to FOrder[FCount] while
incrementing FCount so insertion-order is preserved. Ensure OldOrder is
preserved/copied before replacing FOrder.
In `@souffle/Souffle.VM.pas`:
- Around line 83-88: The constructor TSouffleVM.Create (and corresponding
destructor) currently calls GetExceptionMask/SetExceptionMask and stores
FPreviousExceptionMask, which mutates a process-wide FPU exception mask and can
race across VM instances/threads; remove the global mask changes from
TSouffleVM.Create/Destroy and instead apply exception-mask changes locally
around the specific VM operations that require them (wrap those operations in a
try..finally that calls GetExceptionMask, SetExceptionMask, then restores the
previous mask), or if you must preserve per-thread state store the mask in a
threadvar and use that per-thread; update references to FPreviousExceptionMask,
SetExceptionMask and GetExceptionMask accordingly and ensure restoration happens
immediately after the critical section rather than in the
constructor/destructor.
- Around line 282-301: CallClosure copies only a truncated prefix of arguments
into callee registers (computed as ArgsToCopy = min(AArgCount,
AClosure.Template.MaxRegisters-1) and fills remaining registers with SouffleNil)
but still sets Frame^.ArgCount to the original AArgCount, causing rest-parameter
packing to read stale values; fix by clamping the recorded argument count to the
number actually copied: after computing ArgsToCopy, set Frame^.ArgCount :=
ArgsToCopy (i.e. replace Frame^.ArgCount := AArgCount with the clamped value) so
the callee sees the true number of initialized argument registers (keep
ArgSourceBase and register initialization as-is).
In `@units/Goccia.Compiler.Expressions.pas`:
- Around line 1377-1384: The template interpolation path currently emits
OP_RT_ADD (seen in the loop using ADest and PartReg), which allows numeric
addition; change CompileTemplateWithInterpolation (the code that calls
ACtx.CompileExpression for AExpr.Parts and emits OP_RT_ADD) so each interpolated
part is first coerced to a string with the runtime string-coercion
opcode/sequence that throws on Symbol (e.g., emit a ToString/OP_RT_TO_STRING for
ADest and PartReg or an explicit symbol-type check that raises TypeError), then
perform a string-concatenation opcode (not OP_RT_ADD) to combine ADest and
PartReg; ensure ACtx.Scope.FreeRegister and register usage remain correct.
In `@units/Goccia.Compiler.Statements.pas`:
- Around line 233-243: The code temporarily sets GPendingFinally to nil while
compiling finally blocks (using SavedFinally) but does not restore it if an
exception occurs, leaving GPendingFinally nil and corrupting state; wrap the
block that clears and iterates SavedFinally in a try..finally so GPendingFinally
is always restored (assign SavedFinally := GPendingFinally; GPendingFinally :=
nil; try ... finally GPendingFinally := SavedFinally; end), applying the same
pattern in both return-branches where this logic appears (references:
GPendingFinally, SavedFinally, CompileBlockStatement, EmitInstruction,
EncodeABC, OP_POP_HANDLER, ACtx).
In `@units/Goccia.Engine.pas`:
- Line 97: The FPU mask saving/restoring in TGocciaEngine is not nesting-safe
because each instance writes its own FPreviousExceptionMask and restores
unconditionally; change to a thread-local nesting counter and a single
thread-local original mask so only the outermost TGocciaEngine saves the
original TFPUExceptionMask and only when the counter drops back to zero do you
restore it. Implement threadvar (e.g., GocciaFpuDepth: Integer;
GocciaOriginalFpuMask: TFPUExceptionMask), increment GocciaFpuDepth and save
GocciaOriginalFpuMask only when entering from zero in the TGocciaEngine
constructor, and decrement GocciaFpuDepth and restore GocciaOriginalFpuMask only
when it becomes zero in TGocciaEngine.Destroy; remove or stop using the
per-instance FPreviousExceptionMask for restoration.
---
Outside diff comments:
In `@souffle/Souffle.VM.pas`:
- Around line 169-185: FBaseFrameCount is only restored for ESouffleThrow
exceptions, leaving it corrupted on other exceptions; make the restoration
exception-safe by moving the assignment FBaseFrameCount := SavedBaseFrameCount
into a finally block that always runs after ExecuteLoop (while keeping the
ESouffleThrow-specific cleanup in its except handler), ensuring
CloseUpvalues/FCallStack cleanup still occurs on ESouffleThrow and that Result
:= FRegisters[Base] runs after the finally; update references around
ExecuteLoop, ESouffleThrow, FBaseFrameCount, SavedBaseFrameCount, CloseUpvalues
and FCallStack accordingly.
- Around line 1625-1647: The root-marking exits early when FCallStack.IsEmpty
which skips FArrayDelegate/FRecordDelegate; move the delegate checks
(FArrayDelegate, FRecordDelegate) and FOpenUpvalues check before the early Exit
so they are always considered; also replace the current HighWater calculation
(FCallStack.Peek^.BaseRegister + FCallStack.Peek^.Template.MaxRegisters) with a
computation that considers spilled arguments produced by ExecuteFunction — e.g.,
iterate all frames in FCallStack to compute HighWater = max(BaseRegister +
Template.MaxRegisters + Frame^.SpilledArgsCount) (or the actual spilled-args
field name on your Frame) and then mark FRegisters[0..HighWater-1]; keep marking
closures and upvalues as before.
---
Duplicate comments:
In `@units/Goccia.Compiler.Expressions.pas`:
- Around line 1226-1233: The TStringList returned by GetPropertyNamesInOrder
(assigned to Names) is not freed in the fallback loop and leaks; wrap the usage
of Names in a try..finally block (immediately after Names :=
AExpr.GetPropertyNamesInOrder) and call Names.Free in the finally, keeping the
existing loop that calls CompileObjectProperty(ACtx, AExpr, ADest, Key, ValExpr)
inside the try so Name access is safe and properly released.
- Around line 822-825: The guard that raises for ArgCount > High(UInt8) runs
before computing UseSpread, causing spread calls to be rejected; modify the
logic around ArgCount := AExpr.Arguments.Count and UseSpread :=
HasSpreadArgument(AExpr) so that UseSpread is determined first (call
HasSpreadArgument(AExpr) before enforcing the UInt8 limit) and only apply the
ArgCount > High(UInt8) check when UseSpread is false (or otherwise ensure
spread-mode bypasses the UInt8 argument-count restriction); update the block
containing ArgCount, UseSpread and the exception so spread-mode calls are not
incorrectly barred.
In `@units/Goccia.Compiler.Statements.pas`:
- Around line 714-719: The loop that emits break-finally blocks currently
iterates directly over the shared GPendingFinally list (symbols:
GPendingFinally, GBreakFinallyBase, CompileBlockStatement, EmitInstruction,
OP_POP_HANDLER), which allows re-entrant mutations/duplication; fix it by
detaching a snapshot of the pending-finally entries before emitting: create a
local copy/array of the entries from index
GBreakFinallyBase..GPendingFinally.Count-1, then iterate that local snapshot to
call EmitInstruction and CompileBlockStatement, leaving the original
GPendingFinally intact during emission so re-entrancy cannot mutate the active
iteration.
- Around line 933-953: The code incorrectly introduces a user-visible lexical
local by calling ACtx.Scope.DeclareLocal('__super__', False) inside HasSuper;
instead allocate a compiler-only temporary for SuperReg (do not call
DeclareLocal with '__super__'), use the scope/register temp API (e.g. an
Allocate/PushTemp or AllocateRegister function on ACtx.Scope) to hold the
superclass value, emit the same OP_MOVE/OP_GET_UPVALUE/OP_RT_GET_GLOBAL into
that temporary, emit OP_INHERIT with ClassReg and the temp, then release/free
the temporary (e.g. FreeTemp/PopTemp) so no persistent local slot or user
binding remains. Ensure all references to SuperReg (and the Replace of
DeclareLocal('__super__', False)) are updated accordingly.
---
Nitpick comments:
In `@docs/embedding.md`:
- Around line 462-465: Add a brief explanatory paragraph to the "FPU Exception
Mask" section clarifying what FPU exceptions are (e.g., divide-by-zero,
overflow, underflow, invalid operation), why masking them enables IEEE 754
semantics, and a short practical note for embedders (for example: if the host
uses FPU exception handlers or depends on exceptions for error handling,
creating long-lived TGocciaEngine or TSouffleVM instances that call
SetExceptionMask will suppress those exceptions until the engine is destroyed;
one-shot RunScript usage is unaffected). Reference TGocciaEngine, TSouffleVM,
SetExceptionMask and RunScript in the text so readers can locate the related
behavior.
In `@README.md`:
- Line 154: Reorder the sentence in the README paragraph about the Souffle VM to
lead with the bridge architecture and then state the test coverage as
validation: start by mentioning that the Souffle VM is a register-based VM that
routes language features through a bridge layer to the GocciaScript evaluator,
then note that this approach is validated by passing 100% of the test suite
(3,358 tests), and finally list known structural limitations (e.g., .sbc native
endianness and ABC-encoded instruction format limiting constant pool references
to 255 per prototype); update the paragraph containing "Souffle VM", ".sbc", and
"ABC-encoded" accordingly.
In `@units/Goccia.Values.ObjectValue.pas`:
- Around line 669-698: The prototype-lookup block is redundant: remove the
explicit GetOwnPropertyDescriptor handling (the code that checks
FPrototype.GetOwnPropertyDescriptor, TGocciaPropertyDescriptorAccessor and
TGocciaPropertyDescriptorData) and replace the entire conditional body with a
single delegation to FPrototype.GetPropertyWithContext(AName, AThisContext);
ensure you still check Assigned(FPrototype) and Exit after calling
GetPropertyWithContext so receiver semantics via AThisContext are preserved.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c2b9f4b0-beca-431e-84d4-38d0798e88a5
📒 Files selected for processing (22)
AGENTS.mdREADME.mdTestRunner.dprdocs/code-style.mddocs/design-decisions.mddocs/embedding.mddocs/souffle-vm.mddocs/value-system.mdsouffle/Souffle.Bytecode.passouffle/Souffle.Compound.passouffle/Souffle.VM.RuntimeOperations.passouffle/Souffle.VM.pastests/language/expressions/arithmetic/decrement.jstests/language/expressions/arithmetic/increment.jsunits/Goccia.Compiler.ConstantFolding.pasunits/Goccia.Compiler.Expressions.pasunits/Goccia.Compiler.Statements.pasunits/Goccia.Engine.pasunits/Goccia.Evaluator.Bitwise.pasunits/Goccia.GarbageCollector.pasunits/Goccia.Runtime.Operations.pasunits/Goccia.Values.ObjectValue.pas
🚧 Files skipped from review as they are similar to previous changes (1)
- units/Goccia.Evaluator.Bitwise.pas
Comment on lines
83
to
88
| constructor TSouffleVM.Create(const ARuntimeOps: TSouffleRuntimeOperations); | ||
| begin | ||
| inherited Create; | ||
| FPreviousExceptionMask := GetExceptionMask; | ||
| SetExceptionMask([exInvalidOp, exDenormalized, exZeroDivide, exOverflow, exUnderflow, exPrecision]); | ||
| SetLength(FRegisters, MAX_REGISTERS); |
Contributor
There was a problem hiding this comment.
Constructor/destructor mutate process-wide FPU mask in an instance-unsafe way.
This can interfere with other VM instances/threads and restore stale masks when lifetimes overlap.
Also applies to: 101-107
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@souffle/Souffle.VM.pas` around lines 83 - 88, The constructor
TSouffleVM.Create (and corresponding destructor) currently calls
GetExceptionMask/SetExceptionMask and stores FPreviousExceptionMask, which
mutates a process-wide FPU exception mask and can race across VM
instances/threads; remove the global mask changes from TSouffleVM.Create/Destroy
and instead apply exception-mask changes locally around the specific VM
operations that require them (wrap those operations in a try..finally that calls
GetExceptionMask, SetExceptionMask, then restores the previous mask), or if you
must preserve per-thread state store the mask in a threadvar and use that
per-thread; update references to FPreviousExceptionMask, SetExceptionMask and
GetExceptionMask accordingly and ensure restoration happens immediately after
the critical section rather than in the constructor/destructor.
Comment on lines
+1564
to
+1572
| OP_RT_EXT: | ||
| begin | ||
| A := DecodeA(AInstruction); | ||
| B := DecodeB(AInstruction); | ||
| C := DecodeC(AInstruction); | ||
| FRuntimeOps.ExtendedOperation(B, | ||
| FRegisters[Base + A], FRegisters[Base + C], | ||
| FRegisters[Base + A + 1], AFrame^.Template, C); | ||
| end; |
Contributor
There was a problem hiding this comment.
OP_RT_EXT conflates immediate C with a register operand.
Many ext ops encode C as an immediate (e.g., constant index), but this path always reads FRegisters[Base + C]. That can feed wrong data and can read outside the active window near stack limits.
Suggested hardening
OP_RT_EXT:
begin
A := DecodeA(AInstruction);
B := DecodeB(AInstruction);
C := DecodeC(AInstruction);
+ // C is also used as an immediate by several ext ops; avoid unsafe reads.
+ if Base + C < Length(FRegisters) then
+ RecVal := FRegisters[Base + C]
+ else
+ RecVal := SouffleNil;
FRuntimeOps.ExtendedOperation(B,
- FRegisters[Base + A], FRegisters[Base + C],
+ FRegisters[Base + A], RecVal,
FRegisters[Base + A + 1], AFrame^.Template, C);
end;
Comment on lines
+827
to
+845
| if AExpr.Callee is TGocciaSuperExpression then | ||
| begin | ||
| ObjReg := ACtx.Scope.AllocateRegister; | ||
| CompileThis(ACtx, ObjReg); | ||
| BaseReg := ACtx.Scope.AllocateRegister; | ||
| SuperReg := ACtx.Scope.AllocateRegister; | ||
| CompileSuperAccess(ACtx, SuperReg); | ||
| PropIdx := ACtx.Template.AddConstantString(PROP_CONSTRUCTOR); | ||
| if PropIdx > High(UInt8) then | ||
| raise Exception.Create('Constant pool overflow'); | ||
| if SuperReg <> BaseReg + 1 then | ||
| EmitInstruction(ACtx, EncodeABC(OP_MOVE, BaseReg + 1, SuperReg, 0)); | ||
| EmitInstruction(ACtx, EncodeABC(OP_RT_EXT, BaseReg, GOCCIA_EXT_SUPER_GET, UInt8(PropIdx))); | ||
| ACtx.Scope.FreeRegister; | ||
|
|
||
| for I := 0 to ArgCount - 1 do | ||
| ACtx.CompileExpression(AExpr.Arguments[I], ACtx.Scope.AllocateRegister); | ||
| EmitInstruction(ACtx, EncodeABC(OP_RT_CALL_METHOD, BaseReg, UInt8(ArgCount), 0)); | ||
| for I := 0 to ArgCount - 1 do |
Contributor
There was a problem hiding this comment.
super(...) and super.method(...) paths currently ignore spread semantics.
Both branches always compile positional arguments and emit non-spread OP_RT_CALL_METHOD, so ... arguments are passed as plain values instead of being expanded.
Suggested fix pattern (apply to both super-call branches)
- for I := 0 to ArgCount - 1 do
- ACtx.CompileExpression(AExpr.Arguments[I], ACtx.Scope.AllocateRegister);
- EmitInstruction(ACtx, EncodeABC(OP_RT_CALL_METHOD, BaseReg, UInt8(ArgCount), 0));
- for I := 0 to ArgCount - 1 do
- ACtx.Scope.FreeRegister;
+ if UseSpread then
+ begin
+ ArgsReg := ACtx.Scope.AllocateRegister;
+ CompileSpreadArgsArray(ACtx, AExpr, ArgsReg);
+ EmitInstruction(ACtx, EncodeABC(OP_RT_CALL_METHOD, BaseReg, ArgsReg, 1));
+ ACtx.Scope.FreeRegister;
+ end
+ else
+ begin
+ for I := 0 to ArgCount - 1 do
+ ACtx.CompileExpression(AExpr.Arguments[I], ACtx.Scope.AllocateRegister);
+ EmitInstruction(ACtx, EncodeABC(OP_RT_CALL_METHOD, BaseReg, UInt8(ArgCount), 0));
+ for I := 0 to ArgCount - 1 do
+ ACtx.Scope.FreeRegister;
+ end;Also applies to: 858-876
Comment on lines
+1377
to
+1384
| ACtx.CompileExpression(AExpr.Parts[0], ADest); | ||
|
|
||
| for I := 1 to AExpr.Parts.Count - 1 do | ||
| begin | ||
| PartReg := ACtx.Scope.AllocateRegister; | ||
| ACtx.CompileExpression(AExpr.Parts[I], PartReg); | ||
| EmitInstruction(ACtx, EncodeABC(OP_RT_ADD, ADest, ADest, PartReg)); | ||
| ACtx.Scope.FreeRegister; |
Contributor
There was a problem hiding this comment.
Template interpolation path uses + semantics instead of guaranteed string coercion.
CompileTemplateWithInterpolation emits OP_RT_ADD directly, which can produce numeric-add behavior. Template parts should be string-coerced before concatenation.
Suggested fix
ACtx.CompileExpression(AExpr.Parts[0], ADest);
+ EmitInstruction(ACtx, EncodeABC(OP_RT_TO_STRING, ADest, ADest, 0));
for I := 1 to AExpr.Parts.Count - 1 do
begin
PartReg := ACtx.Scope.AllocateRegister;
ACtx.CompileExpression(AExpr.Parts[I], PartReg);
+ EmitInstruction(ACtx, EncodeABC(OP_RT_TO_STRING, PartReg, PartReg, 0));
EmitInstruction(ACtx, EncodeABC(OP_RT_ADD, ADest, ADest, PartReg));
ACtx.Scope.FreeRegister;
end;Based on learnings: implicit string coercion (template literals, + operator, String.prototype.concat) must check for symbols and throw TypeError at the operator level.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@units/Goccia.Compiler.Expressions.pas` around lines 1377 - 1384, The template
interpolation path currently emits OP_RT_ADD (seen in the loop using ADest and
PartReg), which allows numeric addition; change CompileTemplateWithInterpolation
(the code that calls ACtx.CompileExpression for AExpr.Parts and emits OP_RT_ADD)
so each interpolated part is first coerced to a string with the runtime
string-coercion opcode/sequence that throws on Symbol (e.g., emit a
ToString/OP_RT_TO_STRING for ADest and PartReg or an explicit symbol-type check
that raises TypeError), then perform a string-concatenation opcode (not
OP_RT_ADD) to combine ADest and PartReg; ensure ACtx.Scope.FreeRegister and
register usage remain correct.
Comment on lines
+233
to
+243
| if Assigned(GPendingFinally) then | ||
| begin | ||
| SavedFinally := GPendingFinally; | ||
| GPendingFinally := nil; | ||
| for I := SavedFinally.Count - 1 downto 0 do | ||
| begin | ||
| EmitInstruction(ACtx, EncodeABC(OP_POP_HANDLER, 0, 0, 0)); | ||
| CompileBlockStatement(ACtx, SavedFinally[I].FinallyBlock); | ||
| end; | ||
| GPendingFinally := SavedFinally; | ||
| end; |
Contributor
There was a problem hiding this comment.
Restore GPendingFinally via try..finally in return compilation.
If compiling any finally block raises, GPendingFinally is left as nil, corrupting subsequent compilation state.
🛠️ Suggested fix pattern (apply in both return branches)
if Assigned(GPendingFinally) then
begin
SavedFinally := GPendingFinally;
GPendingFinally := nil;
- for I := SavedFinally.Count - 1 downto 0 do
- begin
- EmitInstruction(ACtx, EncodeABC(OP_POP_HANDLER, 0, 0, 0));
- CompileBlockStatement(ACtx, SavedFinally[I].FinallyBlock);
- end;
- GPendingFinally := SavedFinally;
+ try
+ for I := SavedFinally.Count - 1 downto 0 do
+ begin
+ EmitInstruction(ACtx, EncodeABC(OP_POP_HANDLER, 0, 0, 0));
+ CompileBlockStatement(ACtx, SavedFinally[I].FinallyBlock);
+ end;
+ finally
+ GPendingFinally := SavedFinally;
+ end;
end;Also applies to: 250-260
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@units/Goccia.Compiler.Statements.pas` around lines 233 - 243, The code
temporarily sets GPendingFinally to nil while compiling finally blocks (using
SavedFinally) but does not restore it if an exception occurs, leaving
GPendingFinally nil and corrupting state; wrap the block that clears and
iterates SavedFinally in a try..finally so GPendingFinally is always restored
(assign SavedFinally := GPendingFinally; GPendingFinally := nil; try ... finally
GPendingFinally := SavedFinally; end), applying the same pattern in both
return-branches where this logic appears (references: GPendingFinally,
SavedFinally, CompileBlockStatement, EmitInstruction, EncodeABC, OP_POP_HANDLER,
ACtx).
| FBuiltinBenchmark: TGocciaBenchmark; | ||
| FBuiltinTemporal: TGocciaTemporalBuiltin; | ||
| FBuiltinArrayBuffer: TGocciaGlobalArrayBuffer; | ||
| FPreviousExceptionMask: TFPUExceptionMask; |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd /tmp/repo && git ls-files "units/Goccia.Engine.pas"Repository: frostney/GocciaScript
Length of output: 124
🏁 Script executed:
# Get the relevant sections of Goccia.Engine.pas
head -100 /tmp/repo/units/Goccia.Engine.pas | tail -10Repository: frostney/GocciaScript
Length of output: 157
🏁 Script executed:
# Read around line 97 to see the field declaration
sed -n '90,105p' /tmp/repo/units/Goccia.Engine.pasRepository: frostney/GocciaScript
Length of output: 141
🏁 Script executed:
# Read around lines 189-190 (constructor)
sed -n '180,200p' /tmp/repo/units/Goccia.Engine.pasRepository: frostney/GocciaScript
Length of output: 141
🏁 Script executed:
# Read around line 247 (destructor/cleanup)
sed -n '240,260p' /tmp/repo/units/Goccia.Engine.pasRepository: frostney/GocciaScript
Length of output: 141
🏁 Script executed:
# Also check the class definition to understand the structure
sed -n '1,150p' /tmp/repo/units/Goccia.Engine.pas | grep -A 50 "type.*TGocciaEngine"Repository: frostney/GocciaScript
Length of output: 141
🌐 Web query:
Free Pascal GetExceptionMask SetExceptionMask thread-local FPU state behavior
💡 Result:
-
GetExceptionMask/SetExceptionMaskareMathunit helpers that read/modify the current thread’s floating-point exception mask and return the previous value. In FPC’s RTL docs they’re described as “Get/Set the Floating Point Unit exception mask.” [1][2] -
Thread-local behavior (what actually happens): on mainstream OSes/CPUs the floating‑point environment (x87 control word + SSE
MXCSR, etc.) is part of a thread’s CPU context and is saved/restored on context switches, so changing the mask in one thread does not directly affect other threads. [3][4] -
Implication for FPC code: you must set the desired mask in every thread that needs it (typically at thread start, or around the code that needs a different mask):
uses Math; var old: TFPUExceptionMask; old := GetExceptionMask; try SetExceptionMask(old + [exZeroDivide, exInvalidOp]); // example: mask more exceptions // work finally SetExceptionMask(old); end;
-
Threads created outside the FPC RTL: if callbacks run on threads created by an external library, FPC may not have performed its usual per-thread RTL/thread initialization; the Lazarus VLC article explicitly warns about “threads … created outside of the Free Pascal RTL scope” and recommends forcing RTL thread initialization, and it also shows using
SetExceptionMaskto avoid FP exceptions from drivers. In practice, also set the exception mask inside those callback threads if you rely on a specific FP environment. [5]
Sources:
[1] FPC RTL reference (Math.GetExceptionMask) [rtl.pdf] (downloads.freepascal.org)
[2] FPC RTL reference / HTML page (Math.SetExceptionMask) (freepascal.org)
[3] Stack Overflow: MXCSR is per-thread and saved/restored on context switch (stackoverflow.com)
[4] Microsoft Learn: Windows context switches save/restore thread context (learn.microsoft.com)
[5] Lazarus VLC article (threads outside RTL; SetExceptionMask usage; thread-init note) (freepascal.org)
Citations:
- 1: https://downloads.freepascal.org/fpc/docs-pdf/rtl.pdf
- 2: https://www.freepascal.org/docs-html/rtl/math/setexceptionmask.html
- 3: https://stackoverflow.com/questions/4948057/scope-of-mxcsr-control-register-does-it-affect-other-threads?utm_source=openai
- 4: https://learn.microsoft.com/en-us/windows/win32/procthread/context-switches?utm_source=openai
- 5: https://www.freepascal.org/~michael/articles/lazvlc/lazvlc.pdf
🏁 Script executed:
git ls-files "**Goccia.Engine.pas"Repository: frostney/GocciaScript
Length of output: 88
🏁 Script executed:
# Look for files matching the pattern
find . -name "Goccia.Engine.pas" -type fRepository: frostney/GocciaScript
Length of output: 90
🏁 Script executed:
# Get the file structure
git ls-files | grep -i "engine"Repository: frostney/GocciaScript
Length of output: 120
🏁 Script executed:
# Check file size first
wc -l units/Goccia.Engine.pasRepository: frostney/GocciaScript
Length of output: 92
🏁 Script executed:
# Read the class declaration and field definition (around line 97)
sed -n '90,110p' units/Goccia.Engine.pasRepository: frostney/GocciaScript
Length of output: 1216
🏁 Script executed:
# Read the constructor area (around lines 189-190)
sed -n '180,210p' units/Goccia.Engine.pasRepository: frostney/GocciaScript
Length of output: 1102
🏁 Script executed:
# Read the destructor/cleanup area (around line 247)
sed -n '240,260p' units/Goccia.Engine.pasRepository: frostney/GocciaScript
Length of output: 487
🏁 Script executed:
# Get broader context of the class definition to understand full structure
sed -n '1,100p' units/Goccia.Engine.pas | tail -50Repository: frostney/GocciaScript
Length of output: 1651
🏁 Script executed:
# Get more context around the destructor
sed -n '230,260p' units/Goccia.Engine.pasRepository: frostney/GocciaScript
Length of output: 754
🏁 Script executed:
# Check if there are multiple constructor overloads
grep -n "constructor TGocciaEngine.Create" units/Goccia.Engine.pasRepository: frostney/GocciaScript
Length of output: 374
🏁 Script executed:
# Get full destructor
sed -n '220,250p' units/Goccia.Engine.pasRepository: frostney/GocciaScript
Length of output: 857
🏁 Script executed:
# Check if TGocciaEngine instances are created in application code (search for usage)
rg "TGocciaEngine.Create" --type pasRepository: frostney/GocciaScript
Length of output: 92
🏁 Script executed:
# Check if there are patterns suggesting multiple concurrent engines
rg "TGocciaEngine" units/Goccia.Engine.pas | head -30Repository: frostney/GocciaScript
Length of output: 2545
🏁 Script executed:
# Look at what the mask is being set for - understand the purpose
sed -n '187,220p' units/Goccia.Engine.pasRepository: frostney/GocciaScript
Length of output: 1125
🏁 Script executed:
# Check if there are test cases or examples with multiple concurrent engines
rg "TGocciaEngine" --type-list -t all tests/ examples/ 2>/dev/null | head -20Repository: frostney/GocciaScript
Length of output: 47
🏁 Script executed:
# Look for usage patterns in the codebase
rg -A 5 "TGocciaEngine.Create" --type-list -t all | head -40Repository: frostney/GocciaScript
Length of output: 454
🏁 Script executed:
# Get the full constructor sequence to understand timing
sed -n '182,220p' units/Goccia.Engine.pasRepository: frostney/GocciaScript
Length of output: 1318
🏁 Script executed:
# Look for patterns where multiple TGocciaEngine instances might overlap
rg "TGocciaEngine" --max-count 20Repository: frostney/GocciaScript
Length of output: 7730
🏁 Script executed:
# Check if there are nested engine creation scenarios in tests or examples
find . -name "*.pas" -type f -exec grep -l "TGocciaEngine.Create" {} \;Repository: frostney/GocciaScript
Length of output: 124
🏁 Script executed:
# Verify the exact line numbers match with what was stated in the review
sed -n '95,100p' units/Goccia.Engine.pasRepository: frostney/GocciaScript
Length of output: 270
🏁 Script executed:
# Also verify lines 247 mentioned in the review
sed -n '245,250p' units/Goccia.Engine.pasRepository: frostney/GocciaScript
Length of output: 167
🏁 Script executed:
# Look at the TestRunner and BenchmarkRunner to see actual usage patterns
grep -A 10 "Engine := TGocciaEngine.Create" TestRunner.dprRepository: frostney/GocciaScript
Length of output: 424
🏁 Script executed:
# Check if there are any scenarios with overlapping engine lifetimes
sed -n '1,50p' TestRunner.dprRepository: frostney/GocciaScript
Length of output: 1191
🏁 Script executed:
# Look for any documentation about engine lifetimes and FPU state
grep -B 3 -A 3 "long-lived engine" docs/embedding.mdRepository: frostney/GocciaScript
Length of output: 1409
Make FPU mask restore nesting-safe across multiple engine instances.
The current implementation stores the previous mask per instance (line 97) and restores it unconditionally in the destructor (line 247). If two TGocciaEngine instances are created and destroyed out of creation order—creating A, then B, but destroying B first—each instance independently saves/restores from its own field, leaving the thread's FPU state in an intermediate state rather than the original.
While the codebase currently uses sequential create-execute-destroy patterns, a defensive thread-local depth counter with original mask capture (at depth 0) would ensure the pattern works safely under arbitrary nesting and future refactoring:
Suggested fix
- FPreviousExceptionMask: TFPUExceptionMask; implementation
uses
Generics.Collections,
Math,
SysUtils,
TypInfo,
...
Goccia.Values.SymbolValue,
Goccia.Version;
+
+threadvar
+ GEngineExceptionMaskDepth: Integer;
+ GEnginePreviousExceptionMask: TFPUExceptionMask; constructor TGocciaEngine.Create(const AFileName: string; const ASourceLines: TStringList; const AGlobals: TGocciaGlobalBuiltins; const AResolver: TGocciaModuleResolver);
begin
- FPreviousExceptionMask := GetExceptionMask;
- SetExceptionMask([exInvalidOp, exDenormalized, exZeroDivide, exOverflow, exUnderflow, exPrecision]);
+ if GEngineExceptionMaskDepth = 0 then
+ begin
+ GEnginePreviousExceptionMask := GetExceptionMask;
+ SetExceptionMask([exInvalidOp, exDenormalized, exZeroDivide, exOverflow, exUnderflow, exPrecision]);
+ end;
+ Inc(GEngineExceptionMaskDepth); finally
- SetExceptionMask(FPreviousExceptionMask);
+ if GEngineExceptionMaskDepth > 0 then
+ begin
+ Dec(GEngineExceptionMaskDepth);
+ if GEngineExceptionMaskDepth = 0 then
+ SetExceptionMask(GEnginePreviousExceptionMask);
+ end;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@units/Goccia.Engine.pas` at line 97, The FPU mask saving/restoring in
TGocciaEngine is not nesting-safe because each instance writes its own
FPreviousExceptionMask and restores unconditionally; change to a thread-local
nesting counter and a single thread-local original mask so only the outermost
TGocciaEngine saves the original TFPUExceptionMask and only when the counter
drops back to zero do you restore it. Implement threadvar (e.g., GocciaFpuDepth:
Integer; GocciaOriginalFpuMask: TFPUExceptionMask), increment GocciaFpuDepth and
save GocciaOriginalFpuMask only when entering from zero in the TGocciaEngine
constructor, and decrement GocciaFpuDepth and restore GocciaOriginalFpuMask only
when it becomes zero in TGocciaEngine.Destroy; remove or stop using the
per-instance FPreviousExceptionMask for restoration.
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (5)
docs/souffle-vm.md (1)
1141-1141:⚠️ Potential issue | 🟡 MinorMethod-count wording regressed on Line [1141].
This line says “45 generic methods + 1 extension entry point,” which conflicts with the earlier “45 total including
ExtendedOperation.”✏️ Suggested wording
- 5. **Verification** — After cleanup: Tier 1 has 44 language-agnostic opcodes, Tier 2 has 44 runtime-dispatched opcodes + 1 extension opcode. The `souffle/` directory has zero `Goccia.*` imports. The abstract interface has 45 generic methods + 1 extension entry point. + 5. **Verification** — After cleanup: Tier 1 has 44 language-agnostic opcodes, Tier 2 has 44 runtime-dispatched opcodes + 1 extension opcode. The `souffle/` directory has zero `Goccia.*` imports. The abstract interface has 45 methods total (including the `ExtendedOperation` extension entry point).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/souffle-vm.md` at line 1141, The wording on the methods count is inconsistent: replace the phrase "45 generic methods + 1 extension entry point" with the consistent form used earlier, e.g., "45 total including `ExtendedOperation`", so the doc's method-count statement matches the earlier reference to ExtendedOperation; update the sentence containing that phrase to use the exact token `ExtendedOperation` for clarity.README.md (1)
154-154:⚠️ Potential issue | 🟠 MajorParity caveats on Line [154] look incomplete.
The new text still reads as near-full parity but only lists two structural limits. If the runtime stubs are still present, this should explicitly mention them here as well.
✏️ Suggested wording adjustment
- GocciaScript includes an alternative bytecode execution backend — the **Souffle VM** — a general-purpose register-based virtual machine that routes language features through a bridge layer to the GocciaScript evaluator. This approach passes 100% of the test suite (3,358 tests). Known structural limitations: `.sbc` files use native endianness (not yet cross-platform portable); the ABC-encoded instruction format limits constant pool references to 255 per prototype. See [Souffle VM Architecture](docs/souffle-vm.md) for full details. + GocciaScript includes an alternative bytecode execution backend — the **Souffle VM** — a general-purpose register-based virtual machine that routes language features through a bridge layer to the GocciaScript evaluator. It passes the current test suite (3,358 tests), with some runtime operations still in-progress. Known limitations include: iterator/import/await runtime stubs, closure receiver binding not persisted, `.sbc` native endianness (not cross-platform portable), and the ABC instruction limit of 255 constant-pool references per prototype. See [Souffle VM Architecture](docs/souffle-vm.md) for full details.Based on learnings: "Souffle VM known limitations: Iteration (GetIterator, IteratorNext, SpreadInto), module imports (ImportModule), and async/await (AwaitValue) are currently stubbed in TGocciaRuntimeOperations; closure receiver binding is accepted but not stored; .sbc binary format uses native endianness (not portable); ABC-encoded instructions limit constant pool references to 255 per prototype".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` at line 154, Update the parity caveats sentence to explicitly list the current runtime stubs and structural limits: mention that iteration ops (GetIterator, IteratorNext, SpreadInto), module imports (ImportModule), and async/await (AwaitValue) are stubbed in TGocciaRuntimeOperations; note that closure receiver binding is accepted but not stored; and keep the two structural points that .sbc uses native endianness (not cross-platform) and the ABC-encoded instruction format limits constant-pool references to 255 per prototype so readers see the full set of known limitations.souffle/Souffle.VM.pas (3)
1566-1573:⚠️ Potential issue | 🔴 Critical
OP_RT_EXTstill conflates immediateCwith register operand.
FRegisters[Base + C]is always read even whenCis used as an immediate sub-operand. That can feed incorrect data and may read out of bounds near register limits.🛡️ Suggested hardening
var A, B, C: UInt8; Bx: UInt16; Base: Integer; Done: Boolean; FloatB, FloatC: Double; + ExtArg: TSouffleValue; begin @@ OP_RT_EXT: begin A := DecodeA(AInstruction); B := DecodeB(AInstruction); C := DecodeC(AInstruction); + if (Base + C >= 0) and (Base + C < Length(FRegisters)) then + ExtArg := FRegisters[Base + C] + else + ExtArg := SouffleNil; FRuntimeOps.ExtendedOperation(B, - FRegisters[Base + A], FRegisters[Base + C], + FRegisters[Base + A], ExtArg, FRegisters[Base + A + 1], AFrame^.Template, C); end;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@souffle/Souffle.VM.pas` around lines 1566 - 1573, The OP_RT_EXT handler is incorrectly always reading FRegisters[Base + C] even when C is an immediate/sub-operand, which can produce wrong values or OOB reads; update the OP_RT_EXT logic (around DecodeA/DecodeB/DecodeC and the call to FRuntimeOps.ExtendedOperation) to detect whether C is a register operand or an immediate and pass the correct value: supply the register content only when C denotes a register (e.g., FRegisters[Base + C]) otherwise pass the immediate C value (and avoid indexing FRegisters in that branch); ensure the same distinction is used for any sub-operand access related to C and preserve existing parameters (Base+A, Base+A+1, AFrame^.Template) to FRuntimeOps.ExtendedOperation.
83-88:⚠️ Potential issue | 🟠 MajorAvoid VM-lifetime global FPU mask mutation.
Changing the exception mask in constructor/destructor is not instance-safe and can interfere with concurrent/overlapping VM usage.
#!/bin/bash # Verify all mask mutations are VM-lifetime scoped in this file. rg -nP 'FPreviousExceptionMask|GetExceptionMask|SetExceptionMask' souffle/Souffle.VM.pasAlso applies to: 101-107
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@souffle/Souffle.VM.pas` around lines 83 - 88, The constructor TSouffleVM.Create and its destructor currently call GetExceptionMask/SetExceptionMask and store FPreviousExceptionMask, which mutates a process-wide FPU exception mask and is not instance-safe; remove those global mask changes from the constructor/destructor and instead scope any FPU mask changes to the smallest possible region by surrounding only the floating-point-critical code paths with a local save/restore pattern (call GetExceptionMask into a local variable, call SetExceptionMask before the FP work, and always restore the saved mask in a finally block). Locate all usages of GetExceptionMask/SetExceptionMask and FPreviousExceptionMask in this unit (including TSouffleVM.Create and the corresponding destructor) and replace them so that no VM construction/destruction changes global state, using a per-operation save/restore approach around methods that require the specific mask.
285-303:⚠️ Potential issue | 🟠 Major
CallClosurestill truncates extra arguments.
ArgsToCopyis clamped to register-window size, so extra call arguments are silently dropped. This still breaks variadic/rest parity for nested closure calls.💡 Suggested fix
-var - NewBase, I, ArgsToCopy: Integer; +var + NewBase, I, RequiredSpace: Integer; Frame: PSouffleVMCallFrame; begin NewBase := FCallStack.Peek^.BaseRegister + FCallStack.Peek^.Template.MaxRegisters; - if NewBase + AClosure.Template.MaxRegisters > MAX_REGISTERS then + RequiredSpace := AClosure.Template.MaxRegisters; + if AArgCount + 1 > RequiredSpace then + RequiredSpace := AArgCount + 1; + if NewBase + RequiredSpace > MAX_REGISTERS then raise Exception.Create('Stack overflow'); FRegisters[NewBase] := AReceiver; - ArgsToCopy := AArgCount; - if ArgsToCopy + 1 > AClosure.Template.MaxRegisters then - ArgsToCopy := AClosure.Template.MaxRegisters - 1; - - for I := 0 to ArgsToCopy - 1 do + for I := 0 to AArgCount - 1 do FRegisters[NewBase + 1 + I] := FRegisters[AArgBase + I]; - for I := ArgsToCopy + 1 to AClosure.Template.MaxRegisters - 1 do + for I := AArgCount + 1 to AClosure.Template.MaxRegisters - 1 do FRegisters[NewBase + I] := SouffleNil; Frame := FCallStack.Push(AClosure.Template, AClosure, NewBase, AReturnAbsolute, FHandlerStack.Count); - Frame^.ArgCount := ArgsToCopy; + Frame^.ArgCount := AArgCount; Frame^.ArgSourceBase := NewBase + 1; end;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@souffle/Souffle.VM.pas` around lines 285 - 303, CallClosure currently clamps ArgsToCopy to the register window and then records Frame^.ArgCount := ArgsToCopy, silently dropping extra arguments; fix this by preserving the original argument count and source so the callee can access extras outside the register window: keep the current logic that copies up to AClosure.Template.MaxRegisters-1 into FRegisters (using ArgsToCopy) but set Frame^.ArgCount := AArgCount (not ArgsToCopy) and set Frame^.ArgSourceBase := AArgBase (not NewBase + 1) so the closure sees the full argument list while small-window args remain copied into the new frame.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@docs/souffle-vm.md`:
- Line 1141: The wording on the methods count is inconsistent: replace the
phrase "45 generic methods + 1 extension entry point" with the consistent form
used earlier, e.g., "45 total including `ExtendedOperation`", so the doc's
method-count statement matches the earlier reference to ExtendedOperation;
update the sentence containing that phrase to use the exact token
`ExtendedOperation` for clarity.
In `@README.md`:
- Line 154: Update the parity caveats sentence to explicitly list the current
runtime stubs and structural limits: mention that iteration ops (GetIterator,
IteratorNext, SpreadInto), module imports (ImportModule), and async/await
(AwaitValue) are stubbed in TGocciaRuntimeOperations; note that closure receiver
binding is accepted but not stored; and keep the two structural points that .sbc
uses native endianness (not cross-platform) and the ABC-encoded instruction
format limits constant-pool references to 255 per prototype so readers see the
full set of known limitations.
In `@souffle/Souffle.VM.pas`:
- Around line 1566-1573: The OP_RT_EXT handler is incorrectly always reading
FRegisters[Base + C] even when C is an immediate/sub-operand, which can produce
wrong values or OOB reads; update the OP_RT_EXT logic (around
DecodeA/DecodeB/DecodeC and the call to FRuntimeOps.ExtendedOperation) to detect
whether C is a register operand or an immediate and pass the correct value:
supply the register content only when C denotes a register (e.g.,
FRegisters[Base + C]) otherwise pass the immediate C value (and avoid indexing
FRegisters in that branch); ensure the same distinction is used for any
sub-operand access related to C and preserve existing parameters (Base+A,
Base+A+1, AFrame^.Template) to FRuntimeOps.ExtendedOperation.
- Around line 83-88: The constructor TSouffleVM.Create and its destructor
currently call GetExceptionMask/SetExceptionMask and store
FPreviousExceptionMask, which mutates a process-wide FPU exception mask and is
not instance-safe; remove those global mask changes from the
constructor/destructor and instead scope any FPU mask changes to the smallest
possible region by surrounding only the floating-point-critical code paths with
a local save/restore pattern (call GetExceptionMask into a local variable, call
SetExceptionMask before the FP work, and always restore the saved mask in a
finally block). Locate all usages of GetExceptionMask/SetExceptionMask and
FPreviousExceptionMask in this unit (including TSouffleVM.Create and the
corresponding destructor) and replace them so that no VM
construction/destruction changes global state, using a per-operation
save/restore approach around methods that require the specific mask.
- Around line 285-303: CallClosure currently clamps ArgsToCopy to the register
window and then records Frame^.ArgCount := ArgsToCopy, silently dropping extra
arguments; fix this by preserving the original argument count and source so the
callee can access extras outside the register window: keep the current logic
that copies up to AClosure.Template.MaxRegisters-1 into FRegisters (using
ArgsToCopy) but set Frame^.ArgCount := AArgCount (not ArgsToCopy) and set
Frame^.ArgSourceBase := AArgBase (not NewBase + 1) so the closure sees the full
argument list while small-window args remain copied into the new frame.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b96dec2d-bdb3-4f69-a7aa-bcd8c10a979e
📒 Files selected for processing (5)
README.mddocs/embedding.mddocs/souffle-vm.mdsouffle/Souffle.Compound.passouffle/Souffle.VM.pas
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/embedding.md
This was referenced Mar 7, 2026
Merged
Merged
This was referenced Mar 18, 2026
This was referenced Apr 11, 2026
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 by CodeRabbit
New Features
Bug Fixes
Documentation
Tests