I think those lines are the problem:

"0 >= start, fail" cogit CmpCq: (objectMemory integerObjectOf: 0) R: startReg. jumpOutOfBounds1 := cogit JumpLess: 0. "0 >= replStart, fail" cogit CmpCq: (objectMemory integerObjectOf: 0) R: repStartReg. jumpOutOfBounds2 := cogit JumpLess: 0.

If this is equal if should jump out of bounds.

On Sun, Nov 26, 2017 at 9:53 PM, Clément Bera bera.clement@gmail.com wrote:

Bug seems to be with underflow access on replacement array:

(1 to: 10) collect: [:i| ['123456789' replaceFrom: 1 to: 4 with: 'abcdefgh' startingAt: 0] on: Error do: ['error']] #('error' ' abc56789' ' abc56789' ' abc56789' ' abc56789' ' abc56789' ' abc56789' ' abc56789' ' abc56789' ' abc56789').

Same thing on arrays since this is common code...

Generated code looks ok... I need to look again I may have inverted some branches.

On Sun, Nov 26, 2017 at 9:37 PM, Clément Bera bera.clement@gmail.com wrote:

...

Ok I have a look.

This is on byte objects...

On Sun, Nov 26, 2017 at 9:07 PM, Nicolas Cellier < nicolas.cellier.aka.nice@gmail.com> wrote:

...

Like reported on travis https://travis-ci.org/OpenSmalltalk/opensmalltalk-vm/jobs/307544570

the testTextReplacement3 now fails randomly. It sometimes omit to signal an Error...

-- Clément Béra Pharo consortium engineer https://clementbera.wordpress.com/ Bâtiment B 40, avenue Halley 59650 Villeneuve d'Ascq

-- Clément Béra Pharo consortium engineer https://clementbera.wordpress.com/ Bâtiment B 40, avenue Halley 59650 Villeneuve d'Ascq

Ok fixed in 2282 ! Thanks for reporting

On Sun, Nov 26, 2017 at 11:04 PM, Clément Bera bera.clement@gmail.com wrote:

This is correct: (lessOrEqual instead of less):

    "0 >= start, fail"

cogit CmpCq: (objectMemory integerObjectOf: 0) R: startReg. jumpOutOfBounds1 := cogit JumpLessOrEqual: 0. "0 >= replStart, fail" cogit CmpCq: (objectMemory integerObjectOf: 0) R: repStartReg. jumpOutOfBounds2 := cogit JumpLessOrEqual: 0.

I can't commit right now (my image has many changes I need to commit and I cannot branch easily on monticello...) Will do tomorrow. Thanks for reporting.

On Sun, Nov 26, 2017 at 10:58 PM, Clément Bera bera.clement@gmail.com wrote:

...

I think those lines are the problem:

    "0 >= start, fail"

cogit CmpCq: (objectMemory integerObjectOf: 0) R: startReg. jumpOutOfBounds1 := cogit JumpLess: 0. "0 >= replStart, fail" cogit CmpCq: (objectMemory integerObjectOf: 0) R: repStartReg. jumpOutOfBounds2 := cogit JumpLess: 0.

If this is equal if should jump out of bounds.

On Sun, Nov 26, 2017 at 9:53 PM, Clément Bera bera.clement@gmail.com wrote:

...

Bug seems to be with underflow access on replacement array:

(1 to: 10) collect: [:i| ['123456789' replaceFrom: 1 to: 4 with: 'abcdefgh' startingAt: 0] on: Error do: ['error']] #('error' ' abc56789' ' abc56789' ' abc56789' ' abc56789' ' abc56789' ' abc56789' ' abc56789' ' abc56789' ' abc56789').

Same thing on arrays since this is common code...

Generated code looks ok... I need to look again I may have inverted some branches.

On Sun, Nov 26, 2017 at 9:37 PM, Clément Bera bera.clement@gmail.com wrote:

...

Ok I have a look.

This is on byte objects...

On Sun, Nov 26, 2017 at 9:07 PM, Nicolas Cellier < nicolas.cellier.aka.nice@gmail.com> wrote:

...

Like reported on travis https://travis-ci.org/OpenSmalltalk/opensmalltalk-vm/jobs/307544570

the testTextReplacement3 now fails randomly. It sometimes omit to signal an Error...

-- Clément Béra Pharo consortium engineer https://clementbera.wordpress.com/ Bâtiment B 40, avenue Halley 59650 Villeneuve d'Ascq

-- Clément Béra Pharo consortium engineer https://clementbera.wordpress.com/ Bâtiment B 40, avenue Halley 59650 Villeneuve d'Ascq

-- Clément Béra Pharo consortium engineer https://clementbera.wordpress.com/ Bâtiment B 40, avenue Halley 59650 Villeneuve d'Ascq

-- Clément Béra Pharo consortium engineer https://clementbera.wordpress.com/ Bâtiment B 40, avenue Halley 59650 Villeneuve d'Ascq

2017-11-26 23:30 GMT+01:00 Nicolas Cellier < nicolas.cellier.aka.nice@gmail.com>:

Ah thanks!

But I think that there is another bug in replacement size test...

(1 to: 20) reject: [:n | a := String new: n. b := String new: n-1. ([a replaceFrom: 1 to: n with: b startingAt: 1] ifError: [nil]) isNil. ] -> #(2 4 6 8 10 12 14 16 18 20)

I'd expect they all fail, thus an empty #()

OK, I corrected it, the replArray size was not correct (VMMaker.oscog-nice.2283) The variable holding replArray format was already overwritten. So I choosed to store replArray format into repStartReg and that works OK.

2017-11-26 23:09 GMT+01:00 Clément Bera bera.clement@gmail.com:

...

Ok fixed in 2282 ! Thanks for reporting

On Sun, Nov 26, 2017 at 11:04 PM, Clément Bera bera.clement@gmail.com wrote:

...

This is correct: (lessOrEqual instead of less):

    "0 >= start, fail"

cogit CmpCq: (objectMemory integerObjectOf: 0) R: startReg. jumpOutOfBounds1 := cogit JumpLessOrEqual: 0. "0 >= replStart, fail" cogit CmpCq: (objectMemory integerObjectOf: 0) R: repStartReg. jumpOutOfBounds2 := cogit JumpLessOrEqual: 0.

I can't commit right now (my image has many changes I need to commit and I cannot branch easily on monticello...) Will do tomorrow. Thanks for reporting.

On Sun, Nov 26, 2017 at 10:58 PM, Clément Bera bera.clement@gmail.com wrote:

...

I think those lines are the problem:

    "0 >= start, fail"

cogit CmpCq: (objectMemory integerObjectOf: 0) R: startReg. jumpOutOfBounds1 := cogit JumpLess: 0. "0 >= replStart, fail" cogit CmpCq: (objectMemory integerObjectOf: 0) R: repStartReg. jumpOutOfBounds2 := cogit JumpLess: 0.

If this is equal if should jump out of bounds.

On Sun, Nov 26, 2017 at 9:53 PM, Clément Bera bera.clement@gmail.com wrote:

...

Bug seems to be with underflow access on replacement array:

(1 to: 10) collect: [:i| ['123456789' replaceFrom: 1 to: 4 with: 'abcdefgh' startingAt: 0] on: Error do: ['error']] #('error' ' abc56789' ' abc56789' ' abc56789' ' abc56789' ' abc56789' ' abc56789' ' abc56789' ' abc56789' ' abc56789').

Same thing on arrays since this is common code...

Generated code looks ok... I need to look again I may have inverted some branches.

On Sun, Nov 26, 2017 at 9:37 PM, Clément Bera bera.clement@gmail.com wrote:

...

Ok I have a look.

This is on byte objects...

On Sun, Nov 26, 2017 at 9:07 PM, Nicolas Cellier < nicolas.cellier.aka.nice@gmail.com> wrote:

> > Like reported on travis > https://travis-ci.org/OpenSmalltalk/opensmalltalk-vm/jobs/307544570 > > the testTextReplacement3 now fails randomly. > It sometimes omit to signal an Error... > >

-- Clément Béra Pharo consortium engineer https://clementbera.wordpress.com/ Bâtiment B 40, avenue Halley 59650 Villeneuve d'Ascq

-- Clément Béra Pharo consortium engineer https://clementbera.wordpress.com/ Bâtiment B 40, avenue Halley 59650 Villeneuve d'Ascq

-- Clément Béra Pharo consortium engineer https://clementbera.wordpress.com/ Bâtiment B 40, avenue Halley 59650 Villeneuve d'Ascq

-- Clément Béra Pharo consortium engineer https://clementbera.wordpress.com/ Bâtiment B 40, avenue Halley 59650 Villeneuve d'Ascq

-- Clément Béra Pharo consortium engineer https://clementbera.wordpress.com/ Bâtiment B 40, avenue Halley 59650 Villeneuve d'Ascq

2017-11-27 8:20 GMT+01:00 Clément Bera bera.clement@gmail.com:

Right thanks. I need to check if those tests are in Pharo somewhere and add them.

This primitive was tricky to implement. Not enough registers, it makes things difficult. With a couple more registers, things would be easier. We could try to change it so that the ARM and x64 version can use Extra0Reg/Extra1Reg and avoid reloading registers all the time.

Of course, this is assembler...

I clearly failed to review the generator code; such review requires good knowledge of generic instruction set, which I haven't + mental map of register usage... Despite your worthy efforts for using semantic names, this register scarcity indeed make such code kind of write only...

Thanks for writing the tough part anyway! Tests are there for polishing the corners ;)

I'm thinking maybe something like:

extraTempReg := backEnd hasExtra0Reg ifTrue: [Extra0Reg] ifFalse: [startReg]. And then later... backEnd hasExtra0Reg ifFalse: [cogit genStackArgAt: 3 into: startReg]. would not be that hard to implement and would speed things on x64/ARM (2-3 less instructions which are L1 cache memory reads).

However the main point of this intrinsic is to avoid the switch to the C stack for small copies, speeding those drastically, and there is no spill/reload in the copying loop so no slow down on long copies, so maybe it's better to keep a single version which is already difficult to get right...

It would then make generator code worse indeed. IMO, there are lower fruits hanging now.

On Mon, Nov 27, 2017 at 12:12 AM, Nicolas Cellier <nicolas.cellier.aka.nice@

gmail.com> wrote:

...

2017-11-26 23:30 GMT+01:00 Nicolas Cellier <nicolas.cellier.aka.nice@gmai l.com>:

...

Ah thanks!

But I think that there is another bug in replacement size test...

(1 to: 20) reject: [:n | a := String new: n. b := String new: n-1. ([a replaceFrom: 1 to: n with: b startingAt: 1] ifError: [nil]) isNil. ] -> #(2 4 6 8 10 12 14 16 18 20)

I'd expect they all fail, thus an empty #()

OK, I corrected it, the replArray size was not correct (VMMaker.oscog-nice.2283) The variable holding replArray format was already overwritten. So I choosed to store replArray format into repStartReg and that works OK.

...

2017-11-26 23:09 GMT+01:00 Clément Bera bera.clement@gmail.com:

...

Ok fixed in 2282 ! Thanks for reporting

On Sun, Nov 26, 2017 at 11:04 PM, Clément Bera bera.clement@gmail.com wrote:

...

This is correct: (lessOrEqual instead of less):

    "0 >= start, fail"

cogit CmpCq: (objectMemory integerObjectOf: 0) R: startReg. jumpOutOfBounds1 := cogit JumpLessOrEqual: 0. "0 >= replStart, fail" cogit CmpCq: (objectMemory integerObjectOf: 0) R: repStartReg. jumpOutOfBounds2 := cogit JumpLessOrEqual: 0.

I can't commit right now (my image has many changes I need to commit and I cannot branch easily on monticello...) Will do tomorrow. Thanks for reporting.

On Sun, Nov 26, 2017 at 10:58 PM, Clément Bera <bera.clement@gmail.com

...

wrote:

...

I think those lines are the problem:

    "0 >= start, fail"

cogit CmpCq: (objectMemory integerObjectOf: 0) R: startReg. jumpOutOfBounds1 := cogit JumpLess: 0. "0 >= replStart, fail" cogit CmpCq: (objectMemory integerObjectOf: 0) R: repStartReg. jumpOutOfBounds2 := cogit JumpLess: 0.

If this is equal if should jump out of bounds.

On Sun, Nov 26, 2017 at 9:53 PM, Clément Bera <bera.clement@gmail.com > wrote:

> Bug seems to be with underflow access on replacement array: > > (1 to: 10) collect: [:i| ['123456789' replaceFrom: 1 to: 4 with: > 'abcdefgh' startingAt: 0] on: Error do: ['error']] > #('error' ' abc56789' ' abc56789' ' abc56789' ' abc56789' ' > abc56789' ' abc56789' ' abc56789' ' abc56789' ' abc56789'). > > Same thing on arrays since this is common code... > > Generated code looks ok... I need to look again I may have inverted > some branches. > > > > > > On Sun, Nov 26, 2017 at 9:37 PM, Clément Bera < > bera.clement@gmail.com> wrote: > >> Ok I have a look. >> >> This is on byte objects... >> >> On Sun, Nov 26, 2017 at 9:07 PM, Nicolas Cellier < >> nicolas.cellier.aka.nice@gmail.com> wrote: >> >>> >>> Like reported on travis >>> https://travis-ci.org/OpenSmalltalk/opensmalltalk-vm/jobs/30 >>> 7544570 >>> >>> the testTextReplacement3 now fails randomly. >>> It sometimes omit to signal an Error... >>> >>> >> >> >> -- >> Clément Béra >> Pharo consortium engineer >> https://clementbera.wordpress.com/ >> Bâtiment B 40, avenue Halley 59650 Villeneuve d'Ascq >> > > > > -- > Clément Béra > Pharo consortium engineer > https://clementbera.wordpress.com/ > Bâtiment B 40, avenue Halley 59650 Villeneuve d'Ascq >

-- Clément Béra Pharo consortium engineer https://clementbera.wordpress.com/ Bâtiment B 40, avenue Halley 59650 Villeneuve d'Ascq

-- Clément Béra Pharo consortium engineer https://clementbera.wordpress.com/ Bâtiment B 40, avenue Halley 59650 Villeneuve d'Ascq

-- Clément Béra Pharo consortium engineer https://clementbera.wordpress.com/ Bâtiment B 40, avenue Halley 59650 Villeneuve d'Ascq

-- Clément Béra Pharo consortium engineer https://clementbera.wordpress.com/ Bâtiment B 40, avenue Halley 59650 Villeneuve d'Ascq