Merge pull request OpenSmalltalk#499 from privat/fix-become

Become should not forget to remember

smalltalksrc/VMMaker/SpurMemoryManager.class.st

@@ -4026,7 +4026,8 @@ SpurMemoryManager >> containsOnlyValidBecomeObjects: array1 and: array2 twoWay:
 		Shouldn't become immutable objects => PrimErrNoModification.
 		Can't copy hash into immediates => PrimErrInappropriate.
 		Two-way become may require memory to create copies => PrimErrNoMemory.
-	 As a side-effect unforward any forwarders in the two arrays if answering 0."
+
+	 Arrays are not modified (and no forwarders unforward)."
 	<inline: true>
 	| fieldOffset effectsFlags oop1 oop2 size |
 	fieldOffset := self lastPointerOf: array1.
@@ -4035,14 +4036,14 @@ SpurMemoryManager >> containsOnlyValidBecomeObjects: array1 and: array2 twoWay:
 	[fieldOffset >= self baseHeaderSize] whileTrue:
 		[oop1 := self longAt: array1 + fieldOffset.
 		 (self isOopForwarded: oop1) ifTrue:
-			[oop1 := self followForwarded: oop1.
-			 self longAt: array1 + fieldOffset put: oop1].
+			[oop1 := self followForwarded: oop1
+			"Do not update the array here, or chose to handle the rememberset correctly."].
 		 self ifOopInvalidForBecome: oop1 errorCodeInto: [:errCode| ^errCode].
 		 effectsFlags := effectsFlags bitOr: (self becomeEffectFlagsFor: oop1).
 		 oop2 := self longAt: array2 + fieldOffset.
 		 (self isOopForwarded: oop2) ifTrue:
-			[oop2 := self followForwarded: oop2.
-			 self longAt: array2 + fieldOffset put: oop2].
+			[oop2 := self followForwarded: oop2
+			"Do not update the array here, or chose to handle the rememberset correctly."].
 		 isTwoWay
 			ifTrue:
 				[self ifOopInvalidForBecome: oop2 errorCodeInto: [:errCode| ^errCode].
@@ -6541,11 +6542,10 @@ SpurMemoryManager >> innerBecomeObjectsIn: array1 and: array2 copyHash: copyHash
 	"Inner loop of two-way become."
 	0 to: (self numSlotsOf: array1) - 1 do:
 		[:i| | obj1 obj2 |
-		"At first blush it would appear unnecessary to use followField: here since
-		 the validation in become:with:twoWay:copyHash: follows forwarders.  But
-		 there's nothing to ensure all elements of each array are unique and don't
-		 appear in the other array.  So the enumeration could encounter an object
-		 already becommed earlier in the same enumeration."
+		"Allways use followField: here since there's nothing to ensure all elements
+		 of each array are unique and don't appear in the other array.
+		 So the enumeration could encounter an object already becommed earlier
+		 in the same enumeration."
 		obj1 := self followField: i ofObject: array1.
 		obj2 := self followField: i ofObject: array2.
 		obj1 ~= obj2 ifTrue:
@@ -6559,11 +6559,10 @@ SpurMemoryManager >> innerBecomeObjectsIn: array1 to: array2 copyHash: copyHashF
 	"Inner loop of one-way become."
 	0 to: (self numSlotsOf: array1) - 1 do:
 		[:i| | obj1 obj2 |
-		"At first blush it would appear unnecessary to use followField: here since
-		 the validation in become:with:twoWay:copyHash: follows forwarders.  But
-		 there's nothing to ensure all elements of each array is unique and doesn't
-		 appear in the other array.  So the enumeration could encounter an object
-		 already becommed earlier in the same enumeration."
+		"Allways use followField: here since there's nothing to ensure all elements
+		 of each array are unique and don't appear in the other array.
+		 So the enumeration could encounter an object already becommed earlier
+		 in the same enumeration."
 		obj1 := self followField: i ofObject: array1.
 		obj2 := self followField: i ofObject: array2.
 		obj1 ~= obj2 ifTrue:

smalltalksrc/VMMakerTests/VMSpurOldSpaceGarbageCollectorTest.class.st

@@ -220,6 +220,38 @@ VMSpurOldSpaceGarbageCollectorTest >> testAnOldObjectReferencedFromVMVariableSho
 	self assertHashOf: self keptObjectInVMVariable1 equals: hash
 ]
 
+{ #category : #'tests-OldSpaceSize' }
+VMSpurOldSpaceGarbageCollectorTest >> testDoubleForwardToYoungs [
+
+		| obj1 obj2 obj3 arrFrom arrTo arrFrom2 arrTo2 |
+
+		obj1 := self newObjectWithSlots: 1.
+		obj2 := self newOldSpaceObjectWithSlots: 1.
+		obj3 := self newOldSpaceObjectWithSlots: 1.
+
+		arrFrom := self newArrayWithSlots: 1.
+		arrTo := self newArrayWithSlots: 1.
+
+		memory storePointer: 0 ofObject: arrFrom withValue: obj2.
+		memory storePointer: 0 ofObject: arrTo withValue: obj1.
+		memory become: arrFrom with: arrTo twoWay: false copyHash: false.
+		"obj2 forwards to obj1, but since obj1 is newspace, obj2 is remembered"
+
+		arrFrom2 := self newOldSpaceArrayWithSlots: 1.
+		arrTo2 := self newOldSpaceArrayWithSlots: 1.
+		memory storePointer: 0 ofObject: arrFrom2 withValue: obj3.
+		memory storePointer: 0 ofObject: arrTo2 withValue: obj2.
+		"Here, arrFrom2 & arrTo2 (oldspace) contains only oldspace."
+
+		memory become: arrFrom2 with: arrTo2 twoWay: false copyHash: false.
+
+		"We want arrFrom2 & arrTo2 points to obj1 and be remembered"
+		self assert: (memory fetchPointer: 0 ofObject: arrFrom2) equals: obj1.
+		self assert: (memory isRemembered: arrFrom2).
+		self assert: (memory fetchPointer: 0 ofObject: arrTo2) equals: obj1.
+		self assert: (memory isRemembered: arrTo2).
+]
+
 { #category : #'tests-OldSpaceSize' }
 VMSpurOldSpaceGarbageCollectorTest >> testDoNotCollectRoots [