Hi All,

On Tue, 16 Jun 2020, commits at source.squeak.org wrote:

> Marcel Taeumel uploaded a new version of Collections to project The Trunk:
> http://source.squeak.org/trunk/Collections-mt.898.mcz
>
> ==================== Summary ====================
>
> Name: Collections-mt.898
> Author: mt
> Time: 16 June 2020, 8:55:38.416917 am
> UUID: 89207449-befb-f84f-83a0-ff0d727d40bc
> Ancestors: Collections-mt.896, Collections-ul.897
>
> Merges ancestry. 
>
> Does somebody know where to find Collections-ul.896?

Well, that's not the actual Collections-ul.897 I intended to push to 
the Inbox/Trunk. It contains my experiments with primitive 158, but does 
not use the primitive yet (the pragmas are commented out).
Fortunately, the code is backwards compatible. It introduces minor 
slowdown because it uses the fallback code (which is the "old" primitive) 
instead of primitive 158 for string comparison.

The reason why I mistakenly uploaded it is because MC silently ignores 
your commit when a file with the same name exists.
And when MC calculates the next update number, it doesn't take into 
account what's in your package cache. It only looks up the remote 
repositories and calculates the next number based on those.
After "saving" it also shows the window with your freshly "saved" 
version open, so you can easily copy "it" to any repository you want to.

Since I already had Collections-ul.897 in my package cache, my version 
with only the WeakIdentityDictionary and the removal of ByteArray >> 
#atAllPut: got lost.

Back to primitive 158, there is a bug in either the jitted variant of the 
primitive or the jit engine I was trying to track down. But I couldn't 
find out how to set a breakpoint in jitted code with gdb, nor did I want 
to build the bochs plugin, so I simply gave up.
If someone has the time and tools ready to debug it, here's a recipe to 
trigger the error:
1. Make sure your image has been saved as the bug might cause corruption.
2. Uncomment the primitive pragmas in String >> #compareWith: and String 
>> #compareWith:collated:
3. Run ClosureCompilerTest >> #testDecompiledDoitMethodTempNames which 
will fail: the primitive will return a non-zero value even though the two 
compared strings are equal.
4. Don't save your image, it might have been corrupted.

I couldn't write a script to reproduce the bug, hence the testcase.
The bug is so hard to trigger, you won't notice string comparison being 
broken: your image will keep running.
When the testcase fails, the primitive's return value suggests that the 
jitted code probably sees one of the strings shorter than it actually is.
The non-jitted version of the primitive works fine.


Levente

>
> =============== Diff against Collections-mt.896 ===============
>
> Item was removed:
> - ----- Method: ByteArray>>atAllPut: (in category 'accessing') -----
> - atAllPut: value
> - 	"Fill the receiver with the given value"
> - 
> - 	<primitive: 145>
> - 	super atAllPut: value!
>
> Item was changed:
>  ----- Method: String>>< (in category 'comparing') -----
>  < aString
>  	"Answer whether the receiver sorts before aString.
>  	The collation order is simple ascii (with case differences)."
> 
> + 	^(self compareWith: aString) < 0!
> - 	^ (self compare: self with: aString collated: AsciiOrder) = 1!
>
> Item was changed:
>  ----- Method: String>><= (in category 'comparing') -----
>  <= aString
>  	"Answer whether the receiver sorts before or equal to aString.
>  	The collation order is simple ascii (with case differences)."
> 
> + 	^(self compareWith: aString) <= 0!
> - 	^ (self compare: self with: aString collated: AsciiOrder) <= 2!
>
> Item was changed:
>  ----- Method: String>>= (in category 'comparing') -----
>  = aString
>  	"Answer whether the receiver sorts equally as aString.
>  	The collation order is simple ascii (with case differences)."
>
>  	self == aString ifTrue: [ ^true ].
>  	aString isString ifFalse: [ ^false ].
>  	self size = aString size ifFalse: [ ^false ].
> + 	^ (self compareWith: aString) = 0!
> - 	^ (self compare: self with: aString collated: AsciiOrder) = 2!
>
> Item was changed:
>  ----- Method: String>>> (in category 'comparing') -----
>  > aString
>  	"Answer whether the receiver sorts after aString.
>  	The collation order is simple ascii (with case differences)."
> 
> + 	^(self compareWith: aString) > 0!
> - 	^ (self compare: self with: aString collated: AsciiOrder) = 3!
>
> Item was changed:
>  ----- Method: String>>>= (in category 'comparing') -----
>  >= aString
>  	"Answer whether the receiver sorts after or equal to aString.
>  	The collation order is simple ascii (with case differences)."
> 
> + 	^(self compareWith: aString) >= 0!
> - 	^ (self compare: self with: aString collated: AsciiOrder) >= 2!
>
> Item was changed:
>  ----- Method: String>>compare:caseSensitive: (in category 'comparing') -----
>  compare: aString caseSensitive: aBool
>  	"Answer a comparison code telling how the receiver sorts relative to aString:
>  		1 - before
>  		2 - equal
>  		3 - after.
>  	"
>  	| map |
>  	map := aBool ifTrue:[CaseSensitiveOrder] ifFalse:[CaseInsensitiveOrder].
> + 	^(self compareWith: aString collated: map) + 2!
> - 	^self compare: self with: aString collated: map!
>
> Item was added:
> + ----- Method: String>>compareWith: (in category 'comparing') -----
> + compareWith: aString
> + 
> + 	"<primitive: 158>"
> + 	^(self compare: self with: aString collated: AsciiOrder) - 2!
>
> Item was added:
> + ----- Method: String>>compareWith:collated: (in category 'comparing') -----
> + compareWith: aString collated: collation
> + 
> + 	"<primitive: 158>"
> + 	^(self compare: self with: aString collated: collation) - 2!
>
> Item was removed:
> - ----- Method: WeakIdentityDictionary>>cleanupIndex: (in category 'private') -----
> - cleanupIndex: anInteger
> - 	array at: anInteger put: vacuum.
> - 	tally := tally - 1.
> - 	self fixCollisionsFrom: anInteger.!
>
> Item was changed:
>  ----- Method: WeakIdentityDictionary>>fixCollisionsFrom: (in category 'private') -----
>  fixCollisionsFrom: start
>  	"The element at start has been removed and replaced by vacuum.
>  	This method moves forward from there, relocating any entries
>  	that had been placed below due to collisions with this one."
>
>  	| element index |
>  	index := start.
>  	[ (element := array at: (index := index \\ array size + 1)) == vacuum ] whileFalse: [
>  		element
>  			ifNil:
>  				[ "The binding at this slot was reclaimed - finish the cleanup"
>  				array at: index put: vacuum.
>  				tally := tally - 1 ]
>  			ifNotNil:
>  				[| newIndex |
> + 				(newIndex := self scanFor: element key) = index ifFalse: [
> - 				(newIndex := self scanWithoutGarbagingFor: element key) = index ifFalse: [
>  					array
>  						at: newIndex put: element;
>  						at: index put: vacuum ] ] ]!
>
> Item was changed:
>  ----- Method: WeakIdentityDictionary>>removeKey:ifAbsent: (in category 'removing') -----
>  removeKey: key ifAbsent: aBlock
>  	"Remove key (and its associated value) from the receiver. If key is not in
>  	the receiver, answer the result of evaluating aBlock. Otherwise, answer
>  	the value externally named by key."
>
>  	| index association |
>  	index := self scanFor: key.
>  	(association := (array at: index)) == vacuum ifTrue: [ ^aBlock value ].
> + 	array at: index put: vacuum.
> + 	tally := tally - 1.
> + 	self fixCollisionsFrom: index.
> - 	self cleanupIndex: index.
>  	^association value!
>
> Item was changed:
>  ----- Method: WeakIdentityDictionary>>scanFor: (in category 'private') -----
>  scanFor: anObject
>  	"Scan the array for the first slot containing either
>  	- a vacuum object indicating an empty slot
>  	- or a binding whose key matches anObject.
> + 	Answer the index of that slot or raise an error if no slot is found which should never happen."
> - 	Answer the index of that slot or raise an error if no slot is found.
> - 	When garbage collected slots are encountered, perform a clean-up."
> 
> + 	| index start size |
> + 	index := start := anObject scaledIdentityHash \\ (size := array size) + 1.
> + 	[ 
> + 		(array at: index) ifNotNil: [ :element |
> + 			(element == vacuum or: [ element key == anObject ])
> + 				ifTrue: [ ^index ] ].
> + 		(index := index \\ size + 1) = start ] whileFalse.
> - 	| index start rescan | 
> - 	[
> - 		rescan := false.
> - 		index := start := anObject scaledIdentityHash \\ array size + 1.
> - 		[ 
> - 			(array at: index) 
> - 				ifNil:
> - 					["Object at this slot has been garbage collected.
> - 					A rescan is necessary because fixing collisions
> - 					might have moved the target before current index."
> - 					self cleanupIndex: index.
> - 					rescan := true]
> - 				ifNotNil:
> - 					[:element | (element == vacuum or: [ element key == anObject ])
> - 						ifTrue: [ ^index ].
> - 					(index := index \\ array size + 1) = start ] ] whileFalse.
> - 		rescan ] whileTrue.
>  	self errorNoFreeSpace!
>
> Item was changed:
>  ----- Method: WeakIdentityDictionary>>scanForEmptySlotFor: (in category 'private') -----
>  scanForEmptySlotFor: anObject
> + 	"Scan the array for the first empty slot marked by vacuum object or nil.
> + 	Answer the index of that slot or raise an error if no slot is found, which should never happen."
> - 	"Scan the array for the first empty slot marked by vacuum object.
> - 	Answer the index of that slot or raise an error if no slot is found.
> - 	Ignore the slots that have been garbage collected (those containing nil)."
>
>  	| index start |
>  	index := start := anObject scaledIdentityHash \\ array size + 1.
>  	[ 
> + 		| element |
> + 		((element := array at: index) == vacuum or: [ element == nil ]) ifTrue: [ ^index ].
> - 		(array at: index) 
> - 			ifNotNil:
> - 				[:element | element == vacuum ifTrue: [ ^index ] ].
>  		(index := index \\ array size + 1) = start ] whileFalse.
>  	self errorNoFreeSpace!
>
> Item was removed:
> - ----- Method: WeakIdentityDictionary>>scanWithoutGarbagingFor: (in category 'private') -----
> - scanWithoutGarbagingFor: anObject
> - 	"Scan the array for the first slot containing either
> - 	- a vacuum object indicating an empty slot
> - 	- or a binding whose key matches anObject.
> - 	Answer the index of that slot or raise an error if no slot is found.
> - 	Ignore the slots that have been garbage collected (those containing nil)"
> - 
> - 	| index start | 
> - 	index := start := anObject scaledIdentityHash \\ array size + 1.
> - 	[ 
> - 		(array at: index) 
> - 			ifNotNil:
> - 				[:element | (element == vacuum or: [ element key == anObject ])
> - 					ifTrue: [ ^index ] ].
> - 		(index := index \\ array size + 1) = start ] whileFalse.
> - 	self errorNoFreeSpace!