Skip to content

Potential bug when manually changing the pc #103

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
guillep opened this issue Apr 2, 2025 · 1 comment
Open

Potential bug when manually changing the pc #103

guillep opened this issue Apr 2, 2025 · 1 comment
Assignees
Labels
enhancement New feature or request

Comments

@guillep
Copy link

guillep commented Apr 2, 2025

Already discussed with @StevenCostiou

In the method SindaringDebugger>>#pc: there is some fishy code here:

	"If the first pc of the first statement is mapped to a block creation. That means that it needs the associated temp vector on top of the stack. The bytecode that pushes this vector on the stack precedes the block creation. So, here, this bytecode is mapped to the method node and has been skipped. Thus, we go back to the previous bytecode to execute it."
	self instructionStream willCreateBlock ifTrue: [
		self context pc: self instructionStream previousPc.
		self stepBytecode ].

This code seems to assume that a push closure bytecode is always preceded by a (single?) push temp vector instruction (which are not essentially temp vectors but copied variables from which some may be temp vectors).

This is however not always the case, some examples:

  • the push closure may be preceded by more than one copied variables
  • the push closure may be precedded by no copied variables => it could be a pop (if preceded by a statement) or a send, or any other instruction.

In those cases, executing partially the pushes will create an inconsistent closure.
Executing instructions that perform pops may corrupt/break the stack (pop more stuff that is there for example).

Some code examples, check the methods of the following:

| x t |
[ x := 2 ].

=>

33 <E7 01> push: (Array new: 1)
35 <D1> popIntoTemp: 1
36 <41> pushTemp: 1
37 <F9 00 01> fullClosure:a CompiledBlock: [ x := 2 ] NumCopied: 1
40 <5C> returnTop
| x t |
1+2.
[ :arg | arg + 1 ] value: 17.

=>

49 <51> pushConstant: 1
50 <20> pushConstant: 2
51 <60> send: +
52 <D8> pop
53 <F9 01 00> fullClosure:a CompiledBlock: [ :arg | arg + 1 ] NumCopied: 0
56 <22> pushConstant: 17
57 <7A> send: value:
58 <5C> returnTop
| x t |
self toto: 1+2 lala: ([ :arg | arg + 1 ] value: 17).

=>

57 <4C> self
58 <51> pushConstant: 1
59 <20> pushConstant: 2
60 <60> send: +
61 <F9 01 00> fullClosure:a CompiledBlock: [ :arg | arg + 1 ] NumCopied: 0
64 <22> pushConstant: 17
65 <7A> send: value:
66 <A3> send: toto:lala:
67 <5C> returnTop
@StevenCostiou StevenCostiou self-assigned this Apr 3, 2025
@StevenCostiou StevenCostiou added the enhancement New feature or request label Apr 3, 2025
@StevenCostiou
Copy link
Member

So, after a first investigation, it seems in the current state it's not a bug.
The willCreateBlock: will always check if we're in the case of a block creation.
In these scenarios it is never the case and the incriminated code is never executed.

I keep this issue for deeper investigation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants