Skip to content
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

Fix clean blocks with dead context on debugger #15153

Conversation

PalumboN
Copy link
Collaborator

Fix #13779

  • A dead context is created for CleanBlocks in case it is not found in the stack
  • Then small bugs appear -> check comment in sourceNodeForPC:
  • I also doubt how BytecodeEncoder >> isCreateBlockAt:in: was resolved.

Related PR: pharo-spec/NewTools#605

@Ducasse
Copy link
Member

Ducasse commented Oct 27, 2023

Some unrelated tests are failing

@MarcusDenker
Copy link
Member

the failing tests look somehow related, as they are about blocks (but in Sindarin)

unix-64 / Tests-unix-64 / testMoveToNodeWhenNodeIsInBlockThatCreatesContextAndBlockHasBeenCreated – Unix64.Sindarin.Tests.Base.SindarinDebuggerTest
unix-64 / Tests-unix-64 / testMoveToNodeWhenNodeIsInBlockThatCreatesContextAndBlockHasBeenCreatedBackward – Unix64.Sindarin.Tests.Base.SindarinDebuggerTest
unix-64 / Tests-unix-64 / testMoveToNodeWhenNodeIsNonInlinedAndEmbeddedInNonInlinedBlock – Unix64.Sindarin.Tests.Base.SindarinDebuggerTest

@jecisc jecisc added the Status: Need more work The issue is nearly ready. Waiting some last bits. label Oct 30, 2023
@jecisc jecisc added Status: Tests passed please review! and removed Status: Need more work The issue is nearly ready. Waiting some last bits. labels Oct 31, 2023
@jecisc
Copy link
Member

jecisc commented Oct 31, 2023

tests passed!

@PalumboN
Copy link
Collaborator Author

PalumboN commented Nov 2, 2023

Tests (Morphic) on OSX are crashing but I cannot reproduce it 👎

@jecisc
Copy link
Member

jecisc commented Nov 2, 2023

This is a known issue that I hope will be workaround with: pharo-spec/Spec#1467

all tests passed here

Copy link
Member

@MarcusDenker MarcusDenker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

@@ -469,7 +469,9 @@ EncoderForSistaV1 class >> isCreateBlockAt: pc in: method [

| byte |
byte := self nonExtensionBytecodeAt: pc in: method.
^ byte = 250
^ byte = 250 or: [
(method sourceNodeForPC: pc) in: [ :node | "Clean blocks are created as PushConstant"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Falling back to the AST here is not that nice as it breaks the abstraction (the idea is that the bytecode level abstractions should be usable without the AST). But we could merge it for now and improve later, this is used only the the debugger which needs the AST anyway.

@MarcusDenker MarcusDenker merged commit 34da04c into pharo-project:Pharo12 Nov 6, 2023
1 of 2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Clean Blocks] Test failing with clean blocks enabled: Debugger #13755
4 participants