[Vm-dev] [squeak-dev] Bug in PolygonMorph>>#filledForm

[Fabio Niephaus]

On Sat, Feb 22, 2020 at 9:30 AM Nicolas Cellier <
nicolas.cellier.aka.nice at gmail.com> 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

[1]
https://github.com/hpi-swa/graalsqueak/commit/30af6128c1e78ff769801972fd4b0ca18993d224


>
> Le ven. 21 févr. 2020 à 23:40, Nicolas Cellier <
> nicolas.cellier.aka.nice at gmail.com> a écrit :
>
>> Several candidates, maybe I missed some:
>> https://source.squeak.org/VMMaker/VMMaker.oscog-eem.2461.diff
>> https://source.squeak.org/VMMaker/VMMaker.oscog-eem.2460.diff (Oct 2018)
>> https://source.squeak.org/VMMaker/VMMaker.oscog-eem.2284.diff
>> https://source.squeak.org/VMMaker/VMMaker.oscog- nice.2281.diff
>>
>> Le ven. 21 févr. 2020 à 23:13, Nicolas Cellier <
>> nicolas.cellier.aka.nice at gmail.com> 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 <
>>> nicolas.cellier.aka.nice at gmail.com> 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 <
>>>> nicolas.cellier.aka.nice at gmail.com> a écrit :
>>>>
>>>>> Interesting case to debug...
>>>>> In 64bits VM, a problem first appears in first smear:distance:
>>>>> operation (direction 1 at 0 to the right) at skew=16...
>>>>>
>>>>> [image: 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)
>>>>>  https://github.com/OpenSmalltalk/opensmalltalk-vm/issues/426
>>>>> - bug in #copyLoop (which I also corrected in 2019)
>>>>> https://github.com/OpenSmalltalk/opensmalltalk-vm/issues/441
>>>>>
>>>>> 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 <
>>>>> lecteur at zogotounga.net> 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
>>>>>>
>>>>>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.squeakfoundation.org/pipermail/vm-dev/attachments/20200222/97a6bf9b/attachment-0001.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: Capture d’écran 2020-02-21 à 22.24.03.png
Type: image/png
Size: 18235 bytes
Desc: not available
URL: <http://lists.squeakfoundation.org/pipermail/vm-dev/attachments/20200222/97a6bf9b/attachment-0001.png>