From ca05ce6fd96df3729f38e114caf4970204498f1a Mon Sep 17 00:00:00 2001 From: Blair McGlashan Date: Sun, 4 Jun 2023 08:26:56 +0100 Subject: [PATCH] Temps map entry incorrect for scopes with clean blocks Bug in "Move bytecodes of clean blocks to end of methods" 86c034206c206f3d97ade6dd346af397c542c037 The temps map entry for the scopes containing clean blocks that are moved out of line should not extend to the end of the new location of the clean blocks that are moved. --- Core/DolphinVM/Compiler/LexicalScope.h | 3 +- Core/DolphinVM/Compiler/optimizer.cpp | 2 +- .../Professional/Tools.DebugInfoPlugin.cls | 1 + .../Tests/Kernel.Tests.CompilerTest.cls | 78 ++++++++++++++++++- ...ernel.Tests.DolphinCompilerTestMethods.cls | 13 ++++ 5 files changed, 94 insertions(+), 3 deletions(-) diff --git a/Core/DolphinVM/Compiler/LexicalScope.h b/Core/DolphinVM/Compiler/LexicalScope.h index 9e0299584f..4f67d0a867 100644 --- a/Core/DolphinVM/Compiler/LexicalScope.h +++ b/Core/DolphinVM/Compiler/LexicalScope.h @@ -568,7 +568,8 @@ class LexicalScope if (m_finalIP < ip) { m_finalIP = ip; - if (Outer != nullptr) + // Clean blocks are moved out of line of the parent scope + if (Outer != nullptr && !IsCleanBlock) { Outer->FinalIP = ip; } diff --git a/Core/DolphinVM/Compiler/optimizer.cpp b/Core/DolphinVM/Compiler/optimizer.cpp index fda0a2f202..05776ee5c8 100755 --- a/Core/DolphinVM/Compiler/optimizer.cpp +++ b/Core/DolphinVM/Compiler/optimizer.cpp @@ -1338,7 +1338,6 @@ void Compiler::FixupJumps() } pCurrentScope->FinalIP = LastIp; - _ASSERTE(GetMethodScope()->FinalIP == LastIp); } size_t Compiler::OptimizeJumps() @@ -1876,6 +1875,7 @@ void Compiler::FixupJump(ip_t pos) case OpCode::NearJump: case OpCode::NearJumpIfFalse: _ASSERTE(!WantOptimize || !isInShortJumpRange(distance, 1)); // Why not optimized? + [[fallthrough]]; case OpCode::NearJumpIfTrue: case OpCode::NearJumpIfNil: case OpCode::NearJumpIfNotNil: diff --git a/Core/Object Arts/Dolphin/IDE/Professional/Tools.DebugInfoPlugin.cls b/Core/Object Arts/Dolphin/IDE/Professional/Tools.DebugInfoPlugin.cls index 0298138da3..2dfba44460 100644 --- a/Core/Object Arts/Dolphin/IDE/Professional/Tools.DebugInfoPlugin.cls +++ b/Core/Object Arts/Dolphin/IDE/Professional/Tools.DebugInfoPlugin.cls @@ -80,6 +80,7 @@ displayMethod ifTrue: [self clear. ^self]. + method selector: method selector asSymbol. disassemblyPresenter text: method disassembly. textMapPresenter list: debugInfo textMap! diff --git a/Core/Object Arts/Dolphin/System/Compiler/Tests/Kernel.Tests.CompilerTest.cls b/Core/Object Arts/Dolphin/System/Compiler/Tests/Kernel.Tests.CompilerTest.cls index 05ce818437..3cd3b89314 100644 --- a/Core/Object Arts/Dolphin/System/Compiler/Tests/Kernel.Tests.CompilerTest.cls +++ b/Core/Object Arts/Dolphin/System/Compiler/Tests/Kernel.Tests.CompilerTest.cls @@ -200,6 +200,7 @@ testBlockWithAllTempTypes self assert: method needsContext. self assert: method envTempCount equals: 2. self assert: method stackTempCount equals: 2. + self assert: method tempsMap equals: {(1 to: 53) -> #(#('copied' 0 1) #('innerBlock' 1 2) #('local' 0 2) #('sharedOuter' 1 1)). (16 to: 52) -> #(#('arg1' 0 1) #('arg2' 0 2) #('copied' 0 3) #('innerBlock' 2 2) #('localBlock' 0 4) #('sharedBlock' 1 1) #('sharedOuter' 2 1)). (39 to: 49) -> #(#('innerBlock' 3 2) #('sharedBlock' 2 1) #('sharedOuter' 3 1))}. block1 := method value: DolphinCompilerTestMethods new withArguments: #(). self assert: block1 isKindOf: BlockClosure. "Should only be one copied values, 0 temps and args" @@ -313,6 +314,36 @@ testCharacterScanning raise: MessageNotUnderstood matching: [:ex | ex tag = (Message selector: #G)]! +testCleanBlockInCopyingBlock + | method block | + method := self getTestMethod: #testCleanBlockInCopyingBlock. + block := method value: DolphinCompilerTestMethods new withArguments: #(). + self assert: block isKindOf: BlockClosure. + self assert: block value equals: #('1' '2' '3'). + + "Note how the copying block is moved out of line (i.e. after the main method return) because it is part of the enclosing clean block. If it copied values from the method context, then the enclosing block would also have to be a copying block, but it only copies the arg to the clean block." + self assert: method disassembly + equals: 'Normal, 0 args, 1 stack temps, 4 literals + + 1 Push Const[0]: #(1 2 3) + 2 Store Temp[0] + 3 Block Copy, 0 args, 1 copied values, skip +4 to 14 + 10 Push Temp[0] + 11 Push Const[3]: [] in Kernel.Tests.DolphinCompilerTestMethods>>#testCleanBlo… + 12 Send[2]: #collect: with 1 args + 13 Return From Block + 14 Return + 15 Push Temp[0]; Send[1]: #printString with 0 args + 17 Return From Block'. + + "The temps map entry for the main method body must not include the clean block moved out of line. The copyingBlock temp is optimised out of the code, but remains allocated. This keeps the stack frame the same for debug and non-debug methods." + self assert: method tempsMap + equals: { + (1 to: 14) -> #(#('a' 0 1)). + (10 to: 13) -> #(#('a' 0 1)). + (15 to: 17) -> #(#('each' 0 1)) + }! + testCompileTimeExpressions | result | self shouldnt: [result := self evaluateExpression: '##()'] raise: self compilationErrorClass. @@ -340,6 +371,38 @@ testConstExpressionReferences self assert: (method refersToLiteral: #y). self deny: (method refersToLiteral: #z)! +testCopyingBlockNestedInCleanBlock + | method block | + method := self getTestMethod: #testCopyingBlockNestedInCleanBlock. + block := method literals at: 4. + self assert: block isKindOf: BlockClosure. + self assert: (block value: 'xyz') value equals: 'xyzzyx'. + + "Note how the copying block is moved out of line (i.e. after the main method return) because it is part of the enclosing clean block. If it copied values from the method context, then the enclosing block would also have to be a copying block, but it only copies the arg to the clean block." + self assert: method disassembly + equals: 'Normal, 0 args, 2 stack temps, 5 literals + + 1 Push Const[0]: ''abc'' + 2 Pop Temp[0] + 3 Push Const[3]: [] in Kernel.Tests.DolphinCompilerTestMethods>>#testCopyingB… + 4 Push Temp[0] + 5 Special Send #value: + 6 Return + 7 Push Temp[0] + 8 Block Copy, 0 args, 1 copied values, skip +4 to 19 + 15 Push Temp[0] + 16 Push Temp[0]; Send[1]: #reverse with 0 args + 18 Send[2]: #, with 1 args + 19 Return From Block'. + + "The temps map entry for the main method body must not include the clean block moved out of line. The copyingBlock temp is optimised out of the code, but remains allocated. This keeps the stack frame the same for debug and non-debug methods." + ^self assert: method tempsMap + equals: { + (1 to: 6) -> #(#('a' 0 1) #('copyingBlock' 0 2)). + (7 to: 19) -> #(#('arg' 0 1)). + (15 to: 19) -> #(#('arg' 0 1)) + }! + testCopyingBlocks1 | method | method := self getTestMethod: #testCopyingBlocks1. @@ -1035,7 +1098,18 @@ testNestedWhileTrueAtStartOfCleanBlock self assert: literals last equals: #whileTrue. self deny: block identicalTo: []. "Block should start immediately after a short push const and return stack top" - self assert: block initialIP equals: 3! + self assert: block initialIP equals: 3. + self assert: method disassembly + equals: 'Normal, 0 args, 4 literals + + 1 Push Const[2]: [] in Kernel.Tests.DolphinCompilerTestMethods>>#testNestedWh… + 2 Return + 3 Push Const[0]: #() + 4 Send[1]: #notEmpty with 0 args + 5 Jump If True -4 to 3 + 7 Push nil + 8 Return From Block'. + self assert: method tempsMap equals: { (1 to: 2) -> #(). (3 to: 8) -> #(). (3 to: 4) -> #() }! testNotIdentical "Test that #~~ generates special send, but retained in literal frame" @@ -1794,8 +1868,10 @@ testBlockWithAllTempTypes!public!unit tests! ! testBraceArrays!public!unit tests! ! testByteArraysInLiteralArrays!public!unit tests! ! testCharacterScanning!public!unit tests! ! +testCleanBlockInCopyingBlock!public!unit tests! ! testCompileTimeExpressions!public!unit tests! ! testConstExpressionReferences!public!unit tests! ! +testCopyingBlockNestedInCleanBlock!public!unit tests! ! testCopyingBlocks1!public!unit tests! ! testCopyingBlocks2!public!unit tests! ! testDecrementPushTempOptimisation!public!unit tests! ! diff --git a/Core/Object Arts/Dolphin/System/Compiler/Tests/Kernel.Tests.DolphinCompilerTestMethods.cls b/Core/Object Arts/Dolphin/System/Compiler/Tests/Kernel.Tests.DolphinCompilerTestMethods.cls index ba7b10fb9e..8fb5c2ccc3 100644 --- a/Core/Object Arts/Dolphin/System/Compiler/Tests/Kernel.Tests.DolphinCompilerTestMethods.cls +++ b/Core/Object Arts/Dolphin/System/Compiler/Tests/Kernel.Tests.DolphinCompilerTestMethods.cls @@ -45,6 +45,17 @@ testBlockWithAllTempTypes innerBlock. localBlock]! +testCleanBlockInCopyingBlock + | a | + a := #(1 2 3). + ^[a collect: [:each | each printString]]! + +testCopyingBlockNestedInCleanBlock + | a copyingBlock | + a := 'abc'. + copyingBlock := [:arg | [arg , arg reverse]] value: a. + ^copyingBlock! + testCopyingBlocks1 | array | array := Array new. @@ -525,6 +536,8 @@ basicSize!public!unit tests! ! ifNil:!public!unit tests! ! ifNil:ifNotNil:!public!unit tests! ! testBlockWithAllTempTypes!public!unit tests! ! +testCleanBlockInCopyingBlock!public!unit tests! ! +testCopyingBlockNestedInCleanBlock!public!unit tests! ! testCopyingBlocks1!public!unit tests! ! testCopyingBlocks2!public!unit tests! ! testImmutableLiterals!public!unit tests! !