Skip to content

Fix CleanBlockClosure/ConstantBlockClosure Tost materialization (dead startpc reference) - #10

Closed
oliveiraallex wants to merge 4 commits into
pharo-ide:dev13from
oliveiraallex:dev13
Closed

oliveiraallex wants to merge 4 commits into
pharo-ide:dev13from
oliveiraallex:dev13

Conversation

@oliveiraallex

Copy link
Copy Markdown
Contributor

TostSerializer's BlockClosure>>correctTostMaterializationWith: shifts a
startpc instance variable to correct for word-size differences when
materializing a block transferred by value. That variable belonged to
the pre-Pharo-11 block representation, where a BlockClosure embedded
its own bytecode inline in its instance-variable-adjacent storage.

Since Pharo 11's FullBlockClosure redesign, BlockClosure no longer
holds bytecode at all -- it's split into outerContext/compiledBlock/
numArgs, with compiledBlock a separate CompiledBlock object owning its
own bytecode and literals. startpc is a stale reference: reading it
now raises UndeclaredVariableRead, since the compiler resolves it to
an unbound global rather than a real instance variable.

FullBlockClosure already had a no-op override for this (someone
already handled that half of the hierarchy). CleanBlockClosure and its
subclass ConstantBlockClosure -- the other concrete block classes from
the same redesign, used for blocks needing no/only-constant outer
state (e.g. [12], [42]) -- did not, and fell back to BlockClosure's
broken implementation, crashing on materialization with "Attempt to
read undeclared variable startpc".

Added mirrored no-op overrides on CleanBlockClosure:

  • correctTostMaterializationWith: (no-op, nothing to correct)
  • class>>createTostInstanceWith: (simplified to Object's basicNew:-based
    instantiation, matching FullBlockClosure's approach)
  • writeTostBodyWith: (simplified to Object's size-based serialization)

Confirmed via local Pharo 9 vs Pharo 11+ source comparison and Pharo 11
release notes ("Extended Full Blocks and Constant Clock closures
support") that this representation change landed in Pharo 11. Checked
the repo's other branches for an existing fix -- none found, including
origin/pharo9-deprecation-methods, which still carries the same broken
startpc code.

Verified live on Pharo 13: peer evaluate: [12] -> 12, peer evaluate: [1 + 2] ->
3, both clean, no debuggers

… startpc reference)

TostSerializer's BlockClosure>>correctTostMaterializationWith: shifts a
`startpc` instance variable to correct for word-size differences when
materializing a block transferred by value. That variable belonged to
the pre-Pharo-11 block representation, where a BlockClosure embedded
its own bytecode inline in its instance-variable-adjacent storage.

Since Pharo 11's FullBlockClosure redesign, BlockClosure no longer
holds bytecode at all -- it's split into outerContext/compiledBlock/
numArgs, with compiledBlock a separate CompiledBlock object owning its
own bytecode and literals. `startpc` is a stale reference: reading it
now raises UndeclaredVariableRead, since the compiler resolves it to
an unbound global rather than a real instance variable.

FullBlockClosure already had a no-op override for this (someone
already handled that half of the hierarchy). CleanBlockClosure and its
subclass ConstantBlockClosure -- the other concrete block classes from
the same redesign, used for blocks needing no/only-constant outer
state (e.g. `[12]`, `[42]`) -- did not, and fell back to BlockClosure's
broken implementation, crashing on materialization with "Attempt to
read undeclared variable startpc".

Added mirrored no-op overrides on CleanBlockClosure:
- correctTostMaterializationWith: (no-op, nothing to correct)
- class>>createTostInstanceWith: (simplified to Object's basicNew:-based
  instantiation, matching FullBlockClosure's approach)
- writeTostBodyWith: (simplified to Object's size-based serialization)

Confirmed via local Pharo 9 vs Pharo 11+ source comparison and Pharo 11
release notes ("Extended Full Blocks and Constant Clock closures
support") that this representation change landed in Pharo 11. Checked
the repo's other branches for an existing fix -- none found, including
origin/pharo9-deprecation-methods, which still carries the same broken
startpc code.

Verified live: `peer evaluate: [12]` -> 12, `peer evaluate: [1 + 2]` ->
3, both clean, no debuggers
@oliveiraallex

Copy link
Copy Markdown
Contributor Author

Split in 2 PRs #12 and #11 with the correct baseline.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant