OCIRTranslatorVisitor>>visitMethod: supposes that OCLiteralSet is ordered

[VincentBlondeau]

Describe the problem When generating the IR of a method, a OCLiteralSet is used to store the literals of the methods. OCLiteralSet being a subclass of Set, there could be no supposition on the order of items when executing the #do: method. Therefore, OCIRTranslatorVisitor>>visitMethod: in "anIr additionalLiterals do: [ :each | gen addLiteral: each ]" could consider badly the ordering of the literals when executing this method that could lead to wrong bytecode generation:

Classes involved OCLiteralSet, OCIRMethod, OCLiteralList, OCIRTranslatorVisitor

Proposal Changing OCLiteralSet to an OrderedCollection

Version information:

• Version: Pharo 11 -> 14

Expected development cost ?

Additional context This issue arises when trying to make the Set>>#do: method iterating randomly to be compliant to Gemstone behavior, with this implementation:

do: aBlock
	tally = 0 ifTrue: [^self].
	(Time millisecondClockValue \\ 2) isZero
		ifTrue: 
			[1 to: array size
				do: 
					[:index |
					| each |
					(each := array at: index) ifNotNil: [aBlock value: each enclosedElement]]]
		ifFalse: 
			[array size to: 1
				by: -1
				do: 
					[:index |
					| each |
					(each := array at: index) ifNotNil: [aBlock value: each enclosedElement]]]

Lifeware related issue.

[VincentBlondeau]

Actually, I found another piece of code where one supposes the order:

OCBlockScope>>#inComingCopiedVarNames
inComingCopiedVarNames
	^ self copiedVarNames intersection: outerScope copiedVarNames

If we consider this method:

openingTimeAt: aTime 
	| candidates currentOrigin lastVisible |
	currentOrigin := Past new.
	candidates := OrderedCollection new.
	(versions select: [:each | each isActive]) do: 
			[:each | 
			each origin = currentOrigin 
				ifFalse: 
					[candidates add: each.
					currentOrigin := each origin]].
	lastVisible := candidates reverse 
				detect: [:each | each time isVisibleAt: aTime]
				ifNone: [^nil].
	^lastVisible time

At the second execution of OCBlockScope>>#inComingCopiedVarNames, "self copiedVarNames" has for value #(0vector0 candidates) "outerScope copiedVarNames" has for value #(aTime candidates 0vector0)

Not presupposing any order on Set, the intersection can be either: #(candidates 0vector0) or #(0vector0 candidates)

This could results into different bytecodes. The expected one:

And the non expected on:

I would suggest to implement the method:

inComingCopiedVarNames

	^ outerScope copiedVarNames select: [ :e | self copiedVarNames includes: e ]

Changing Collection>>#intersection could be quite challenging.

[Ducasse]

Thanks vincent. With Guille we are off for a couple of weeks. Many be marcus can propose a fix.

[Ducasse]

I just saw that you proposed a fix. Even better :). Indeed a set does not like a good datastructure here. We should cover that with tests. I would like to make sure that ARMOR will capture this correctly.

[VincentBlondeau]

@Ducasse Actually, I found 2 issues. The first message about OCLiteralSet is one, the second about OCBlockScope>>#inComingCopiedVarNames is the other one. The fix is only about the latter. Could you please reopen the issue?

Also, we need a backport of the PR I did for Pharo 14 to Pharo 11, 12 and 13. I'll add some PR about this soon.

[MarcusDenker]

I will have a look!

[VincentBlondeau]

Thanks Marcus! I added the PR for the backport. We will need them to be integrated since it is not something we can patch on the fly in our Lifeware image. It needs a full method recompilation.