Re: [squeak-dev] Bug in PolygonMorph>>#filledForm

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
3 messages Options
Reply | Threaded
Open this post in threaded view
|

Re: [squeak-dev] Bug in PolygonMorph>>#filledForm

Nicolas Cellier
 
I do not observe the artifacts at same smear step (nor any other) with
VM [CoInterpreterPrimitives VMMaker.oscog-eem.2266] 5.0.201708312323

That's just 222 commits to bissect...

Le ven. 21 févr. 2020 à 23:00, Nicolas Cellier <[hidden email]> a écrit :
There's already a problem with end 2018 VM
VM [CoInterpreterPrimitives VMMaker.oscog-eem.2488] 5.0.201812040127

Le ven. 21 févr. 2020 à 22:48, Nicolas Cellier <[hidden email]> a écrit :
Interesting case to debug...
In 64bits VM, a problem first appears in first smear:distance: operation (direction 1@0 to the right) at skew=16...

Capture d’écran 2020-02-21 à 22.24.03.png


It seems that we copy bits too far to the right (hence we overwrite the bottom rows)
Fortunately, the artefacts outside polygon bounds disappear at final bitAnd operation, but that's scary!

It could be:
- case of overlap detection failure (#checkSourceOverlap), but the code did not change
- bad initialisation in #sourceSkewAndPointerInit (the one I corrected in 2019)
- bug in #copyLoop (which I also corrected in 2019)

Weird kind of integer overflow? (I've made most of the int unsigned, so this might not be detected by UB sanitizer).

Or could it be that the extra buffer overrun that I removed did have a side effect of filling more gaps?
It would be interesting to observe what happens at same #smear:distance: step with older VM.

Le ven. 21 févr. 2020 à 20:39, Stéphane Rollandin <[hidden email]> a écrit :
> Hmmm... the form was still filled in Squeak 5.1 (Update 16549),
> VM 201701311639 (32-bit). Not so in Squeak 5.2.

Yes, but that same 5.1 image with a current VM has the problem...

Stef

Reply | Threaded
Open this post in threaded view
|

Re: [squeak-dev] Bug in PolygonMorph>>#filledForm

Nicolas Cellier
 
We are overwriting upper row because taking hdir=-1 because of overlap
The problem is not that we write too far, but that we pick too early (on previous row), and it is related to ???

preload!!

To determine if we must preload, we have to take the endBits when the direction is reversed due to overlap (hDir = -1)
endBits := (sx + bbW - 1 bitAnd: pixPerM1) + 1
First error above: checkSourceOverlap has already done that:
sx := sx + bbW - 1.
So it must be:
endBits := (sx bitAnd: pixPerM1) + 1

Then we build mask1:
m1 := destMSB = (hDir > 0)
ifTrue: [AllOnes >> (32 - (startBits * destDepth))]
ifFalse: [AllOnes << (32 - (startBits * destDepth)) bitAnd: AllOnes].
Second error above: if we have reversed direction (hDir = -1), then we must build mask2:
destMSB
ifTrue: [mask2 := AllOnes << (32 - (endBits * destDepth)) bitAnd: AllOnes]
ifFalse: [mask2 := AllOnes >> (32 - (endBits * destDepth))].
Replace mask2 by m1 and endBits by startBits (because startBits contains endBits if hDir=-1)
And you see that the conditions destMSB is reversed...

Last thing: on 64bits, mask1 and mask2 have been declared int64...
But we only want 32 bits mask.
That is why I have protected with the bitAnd: AllOnes
(and thus removed the cCode:inSmalltalk: simulation guard)

Le ven. 21 févr. 2020 à 23:40, Nicolas Cellier <[hidden email]> a écrit :

Le ven. 21 févr. 2020 à 23:13, Nicolas Cellier <[hidden email]> a écrit :
I do not observe the artifacts at same smear step (nor any other) with
VM [CoInterpreterPrimitives VMMaker.oscog-eem.2266] 5.0.201708312323

That's just 222 commits to bissect...

Le ven. 21 févr. 2020 à 23:00, Nicolas Cellier <[hidden email]> a écrit :
There's already a problem with end 2018 VM
VM [CoInterpreterPrimitives VMMaker.oscog-eem.2488] 5.0.201812040127

Le ven. 21 févr. 2020 à 22:48, Nicolas Cellier <[hidden email]> a écrit :
Interesting case to debug...
In 64bits VM, a problem first appears in first smear:distance: operation (direction 1@0 to the right) at skew=16...

Capture d’écran 2020-02-21 à 22.24.03.png


It seems that we copy bits too far to the right (hence we overwrite the bottom rows)
Fortunately, the artefacts outside polygon bounds disappear at final bitAnd operation, but that's scary!

It could be:
- case of overlap detection failure (#checkSourceOverlap), but the code did not change
- bad initialisation in #sourceSkewAndPointerInit (the one I corrected in 2019)
- bug in #copyLoop (which I also corrected in 2019)

Weird kind of integer overflow? (I've made most of the int unsigned, so this might not be detected by UB sanitizer).

Or could it be that the extra buffer overrun that I removed did have a side effect of filling more gaps?
It would be interesting to observe what happens at same #smear:distance: step with older VM.

Le ven. 21 févr. 2020 à 20:39, Stéphane Rollandin <[hidden email]> a écrit :
> Hmmm... the form was still filled in Squeak 5.1 (Update 16549),
> VM 201701311639 (32-bit). Not so in Squeak 5.2.

Yes, but that same 5.1 image with a current VM has the problem...

Stef

Reply | Threaded
Open this post in threaded view
|

Re: [squeak-dev] Bug in PolygonMorph>>#filledForm

fniephaus
 
On Sat, Feb 22, 2020 at 9:30 AM Nicolas Cellier <[hidden email]> wrote:
 
We are overwriting upper row because taking hdir=-1 because of overlap
The problem is not that we write too far, but that we pick too early (on previous row), and it is related to ???

preload!!

To determine if we must preload, we have to take the endBits when the direction is reversed due to overlap (hDir = -1)
endBits := (sx + bbW - 1 bitAnd: pixPerM1) + 1
First error above: checkSourceOverlap has already done that:
sx := sx + bbW - 1.
So it must be:
endBits := (sx bitAnd: pixPerM1) + 1

Then we build mask1:
m1 := destMSB = (hDir > 0)
ifTrue: [AllOnes >> (32 - (startBits * destDepth))]
ifFalse: [AllOnes << (32 - (startBits * destDepth)) bitAnd: AllOnes].
Second error above: if we have reversed direction (hDir = -1), then we must build mask2:
destMSB
ifTrue: [mask2 := AllOnes << (32 - (endBits * destDepth)) bitAnd: AllOnes]
ifFalse: [mask2 := AllOnes >> (32 - (endBits * destDepth))].
Replace mask2 by m1 and endBits by startBits (because startBits contains endBits if hDir=-1)
And you see that the conditions destMSB is reversed...

Last thing: on 64bits, mask1 and mask2 have been declared int64...
But we only want 32 bits mask.
That is why I have protected with the bitAnd: AllOnes
(and thus removed the cCode:inSmalltalk: simulation guard)

Thanks a lot for fixing this, Nicolas! Well done!
I was able to reproduce the bug and fix it with a port of your patch [1] in GraalSqueak.

Fabio

 

Le ven. 21 févr. 2020 à 23:40, Nicolas Cellier <[hidden email]> a écrit :

Le ven. 21 févr. 2020 à 23:13, Nicolas Cellier <[hidden email]> a écrit :
I do not observe the artifacts at same smear step (nor any other) with
VM [CoInterpreterPrimitives VMMaker.oscog-eem.2266] 5.0.201708312323

That's just 222 commits to bissect...

Le ven. 21 févr. 2020 à 23:00, Nicolas Cellier <[hidden email]> a écrit :
There's already a problem with end 2018 VM
VM [CoInterpreterPrimitives VMMaker.oscog-eem.2488] 5.0.201812040127

Le ven. 21 févr. 2020 à 22:48, Nicolas Cellier <[hidden email]> a écrit :
Interesting case to debug...
In 64bits VM, a problem first appears in first smear:distance: operation (direction 1@0 to the right) at skew=16...

Capture d’écran 2020-02-21 à 22.24.03.png


It seems that we copy bits too far to the right (hence we overwrite the bottom rows)
Fortunately, the artefacts outside polygon bounds disappear at final bitAnd operation, but that's scary!

It could be:
- case of overlap detection failure (#checkSourceOverlap), but the code did not change
- bad initialisation in #sourceSkewAndPointerInit (the one I corrected in 2019)
- bug in #copyLoop (which I also corrected in 2019)

Weird kind of integer overflow? (I've made most of the int unsigned, so this might not be detected by UB sanitizer).

Or could it be that the extra buffer overrun that I removed did have a side effect of filling more gaps?
It would be interesting to observe what happens at same #smear:distance: step with older VM.

Le ven. 21 févr. 2020 à 20:39, Stéphane Rollandin <[hidden email]> a écrit :
> Hmmm... the form was still filled in Squeak 5.1 (Update 16549),
> VM 201701311639 (32-bit). Not so in Squeak 5.2.

Yes, but that same 5.1 image with a current VM has the problem...

Stef