[Vm-dev] bugs in DeflatePlugin?

Tue Jan 11 12:43:33 UTC 2022

Hi Florin --

Not sure whether you suggested a fix but ... I just have to replace "readStreamInstSize" with "writeStreamInstSize", too. I will do that.

Best,
Marcel
Am 11.01.2022 13:27:50 schrieb Florin Mateoc <florin.mateoc at gmail.com>:
Hi Marcel,

Thank you for the quick response, but I am afraid the fix in
#primitiveZipSendBlock is still wrong.
While the method is working with multiple types of objects, in the rcvr
variable it clearly loads a zip encoder, and only a zip encoder:

(self loadZipEncoderFrom: rcvr)

And at the end of the method, it is in this ZipEncoder instance rcvr that
it is storing the modified values.
ZipEncoder is a subclass of WriteStream, not of ReadStream.

All the best,
Florin

On Tue, Jan 11, 2022 at 6:22 AM Marcel Taeumel
wrote:

>
> Hi Florin, hi Fabio --
>
> Fixed via VMMaker.oscog-mt.3135.
>
> Thanks for reporting this!
>
> Best,
> Marcel
>
> Am 10.01.2022 19:39:46 schrieb Florin Mateoc :
> Also now MCSerializationTest>>#testMczSerialization is succeeding for me,
> it was failing before (in JsSqueak)
>
> On Mon, Jan 10, 2022 at 3:42 AM Fabio Niephaus wrote:
>
> >
> > Hi Florin,
> >
> > Happy new year and thanks for the bug report! PNGReadWriterTest seems
> > to pass on TruffleSqueak (although I remember some bug fixing a while
> > ago). Anyway, I looked into #primitiveZipSendBlock which seems to be
> > the only sender of #loadZipEncoderFrom:.
> >
> > While #loadZipEncoderFrom: reads from writeStreamInstSize + 1 and + 2,
> > which is off by one as you mentioned, #primitiveZipSendBlock stores at
> > readStreamInstSize + 1 and + 2. And as it happens, readStreamInstSize
> > seems to be 4 while writeStreamInstSize is 5, which compensates for
> > the off-by-one error. However, I don't understand why reading from the
> > wrong positions does not cause the tests to fail. Anyway, I cleaned
> > this up in TruffleSqueak (see [1]).
> >
> > Fabio
> >
> > [1]
> >
> https://github.com/hpi-swa/trufflesqueak/commit/5b6066b54e03e19ec4c4d2b22550ff081e46f6e6
> >
> > On Mon, Jan 10, 2022 at 4:33 AM Florin Mateoc
> > wrote:
> > >
> > >
> > > I should have also mentioned that PNGReadWriterTest also has 18
> failures
> > for SqueakJS, and I think I remember them failing in TruffleSqueak as
> well
> > >
> > > On Sun, Jan 9, 2022 at 10:18 PM Florin Mateoc
> > wrote:
> > >>
> > >> Hi all, and Happy New Year!
> > >>
> > >> I think I just found a couple of bugs in the DeflatePlugin. They were
> > causing errors in my JsSqueak, I don't fully understand why they don't
> seem
> > to cause problems in Squeak. Anyway, after I fixed them, the primitives
> > work and the PNGReadWriterTest tests are all succeeding for me.
> > >>
> > >> One is in #loadZipEncoderFrom:
> > >> The method checks and then determines the writeStreamInstSize and
> then
> > fetches the bitBuffer and bitPosition (the first two instvars of
> > ZipEncoder, a direct subclass of WriteStream) from offsets
> > writeStreamInstSize + 1 and writeStreamInstSize + 2, although the
> fetches
> > are 0-based
> > >>
> > >> The other is in #primitiveZipSendBlock
> > >> The method, still working on a ZipEncoder, at the end stores the
> > modified values for bitBuffer and bitPosition at offset
> readStreamInstSize
> > + 1 and readStreamInstSize + 2, which is just wrong.
> > >>
> > >> As I said, I don't quite get why this does not cause at least
> > image-side errors if not VM crashes in Squeak, but they should probably
> be
> > fixed anyway :)
> > >>
> > >> All the best,
> > >> Florin
> >
> Also now MCSerializationTest>>#testMczSerialization is succeeding for me,
> it was failing before
> (in JsSqueak)
>
>
> On Mon, Jan 10, 2022 at 3:42 AM Fabio Niephaus
> wrote:
>
>>
>>
>> Hi Florin,
>>
>>
>>
>> Happy new year and thanks for the bug report! PNGReadWriterTest seems
>>
>> to pass on TruffleSqueak (although I remember some bug fixing a while
>>
>> ago). Anyway, I looked into #primitiveZipSendBlock which seems to be
>>
>> the only sender of #loadZipEncoderFrom:.
>>
>>
>>
>> While #loadZipEncoderFrom: reads from writeStreamInstSize + 1 and + 2,
>>
>> which is off by one as you mentioned, #primitiveZipSendBlock stores at
>>
>> readStreamInstSize + 1 and + 2. And as it happens, readStreamInstSize
>>
>> seems to be 4 while writeStreamInstSize is 5, which compensates for
>>
>> the off-by-one error. However, I don't understand why reading from the
>>
>> wrong positions does not cause the tests to fail. Anyway, I cleaned
>>
>> this up in TruffleSqueak (see [1]).
>>
>>
>>
>> Fabio
>>
>>
>>
>> [1]
>> https://github.com/hpi-swa/trufflesqueak/commit/5b6066b54e03e19ec4c4d2b22550ff081e46f6e6
>>
>>
>>
>> On Mon, Jan 10, 2022 at 4:33 AM Florin Mateoc
>> wrote:
>>
>> >
>>
>> >
>>
>> > I should have also mentioned that PNGReadWriterTest also has 18
>> failures for SqueakJS, and I think I remember them failing in TruffleSqueak
>> as well
>>
>> >
>>
>> > On Sun, Jan 9, 2022 at 10:18 PM Florin Mateoc
>> wrote:
>>
>> >>
>>
>> >> Hi all, and Happy New Year!
>>
>> >>
>>
>> >> I think I just found a couple of bugs in the DeflatePlugin. They were
>> causing errors in my JsSqueak, I don't fully understand why they don't seem
>> to cause problems in Squeak. Anyway, after I fixed them, the primitives
>> work and the PNGReadWriterTest tests are all succeeding for me.
>>
>> >>
>>
>> >> One is in #loadZipEncoderFrom:
>>
>> >> The method checks and then determines the writeStreamInstSize and then
>> fetches the bitBuffer and bitPosition (the first two instvars of
>> ZipEncoder, a direct subclass of WriteStream) from offsets
>> writeStreamInstSize + 1 and writeStreamInstSize + 2, although the fetches
>> are 0-based
>>
>> >>
>>
>> >> The other is in #primitiveZipSendBlock
>>
>> >> The method, still working on a ZipEncoder, at the end stores the
>> modified values for bitBuffer and bitPosition at offset readStreamInstSize
>> + 1 and readStreamInstSize + 2, which is just wrong.
>>
>> >>
>>
>> >> As I said, I don't quite get why this does not cause at least
>> image-side errors if not VM crashes in Squeak, but they should probably be
>> fixed anyway :)
>>
>> >>
>>
>> >> All the best,
>>
>> >> Florin
>>
>>
>

Hi Marcel,

Thank you for the quick response, but I am afraid the fix in
#primitiveZipSendBlock is still wrong.
While the method is working with multiple types of objects, in the rcvr variable it clearly loads a zip encoder, and only a zip encoder:

(self loadZipEncoderFrom: rcvr)

And at the end of the method, it is in this ZipEncoder instance rcvr that it is storing the modified values.
ZipEncoder is a subclass of WriteStream, not of ReadStream.

All the best,
Florin

On Tue, Jan 11, 2022 at 6:22 AM Marcel Taeumel <marcel.taeumel at hpi.de [mailto:marcel.taeumel at hpi.de]> wrote:

Hi Florin, hi Fabio --

Fixed via VMMaker.oscog-mt.3135.

Thanks for reporting this!

Best,
Marcel

Am 10.01.2022 19:39:46 schrieb Florin Mateoc <florin.mateoc at gmail.com [mailto:florin.mateoc at gmail.com]>:
Also now MCSerializationTest>>#testMczSerialization is succeeding for me,

it was failing before (in JsSqueak)

On Mon, Jan 10, 2022 at 3:42 AM Fabio Niephaus wrote:

>

> Hi Florin,

>

> Happy new year and thanks for the bug report! PNGReadWriterTest seems

> to pass on TruffleSqueak (although I remember some bug fixing a while

> ago). Anyway, I looked into #primitiveZipSendBlock which seems to be

> the only sender of #loadZipEncoderFrom:.

>

> While #loadZipEncoderFrom: reads from writeStreamInstSize + 1 and + 2,

> which is off by one as you mentioned, #primitiveZipSendBlock stores at

> readStreamInstSize + 1 and + 2. And as it happens, readStreamInstSize

> seems to be 4 while writeStreamInstSize is 5, which compensates for

> the off-by-one error. However, I don't understand why reading from the

> wrong positions does not cause the tests to fail. Anyway, I cleaned

> this up in TruffleSqueak (see [1]).

>

> Fabio

>

> [1]

> https://github.com/hpi-swa/trufflesqueak/commit/5b6066b54e03e19ec4c4d2b22550ff081e46f6e6 [https://github.com/hpi-swa/trufflesqueak/commit/5b6066b54e03e19ec4c4d2b22550ff081e46f6e6]

>

> On Mon, Jan 10, 2022 at 4:33 AM Florin Mateoc

> wrote:

> >

> >

> > I should have also mentioned that PNGReadWriterTest also has 18 failures

> for SqueakJS, and I think I remember them failing in TruffleSqueak as well

> >

> > On Sun, Jan 9, 2022 at 10:18 PM Florin Mateoc

> wrote:

> >>

> >> Hi all, and Happy New Year!

> >>

> >> I think I just found a couple of bugs in the DeflatePlugin. They were

> causing errors in my JsSqueak, I don't fully understand why they don't seem

> to cause problems in Squeak. Anyway, after I fixed them, the primitives

> work and the PNGReadWriterTest tests are all succeeding for me.

> >>

> >> One is in #loadZipEncoderFrom:

> >> The method checks and then determines the writeStreamInstSize and then

> fetches the bitBuffer and bitPosition (the first two instvars of

> ZipEncoder, a direct subclass of WriteStream) from offsets

> writeStreamInstSize + 1 and writeStreamInstSize + 2, although the fetches

> are 0-based

> >>

> >> The other is in #primitiveZipSendBlock

> >> The method, still working on a ZipEncoder, at the end stores the

> modified values for bitBuffer and bitPosition at offset readStreamInstSize

> + 1 and readStreamInstSize + 2, which is just wrong.

> >>

> >> As I said, I don't quite get why this does not cause at least

> image-side errors if not VM crashes in Squeak, but they should probably be

> fixed anyway :)

> >>

> >> All the best,

> >> Florin

>

Also now MCSerializationTest>>#testMczSerialization is succeeding for me, it was failing before

(in JsSqueak)

On Mon, Jan 10, 2022 at 3:42 AM Fabio Niephaus <lists at fniephaus.com [mailto:lists at fniephaus.com]> wrote:

Hi Florin,

Happy new year and thanks for the bug report! PNGReadWriterTest seems

to pass on TruffleSqueak (although I remember some bug fixing a while

ago). Anyway, I looked into #primitiveZipSendBlock which seems to be

the only sender of #loadZipEncoderFrom:.

While #loadZipEncoderFrom: reads from writeStreamInstSize + 1 and + 2,

which is off by one as you mentioned, #primitiveZipSendBlock stores at

readStreamInstSize + 1 and + 2. And as it happens, readStreamInstSize

seems to be 4 while writeStreamInstSize is 5, which compensates for

the off-by-one error. However, I don't understand why reading from the

wrong positions does not cause the tests to fail. Anyway, I cleaned

this up in TruffleSqueak (see [1]).

Fabio

[1] https://github.com/hpi-swa/trufflesqueak/commit/5b6066b54e03e19ec4c4d2b22550ff081e46f6e6 [https://github.com/hpi-swa/trufflesqueak/commit/5b6066b54e03e19ec4c4d2b22550ff081e46f6e6]

On Mon, Jan 10, 2022 at 4:33 AM Florin Mateoc <florin.mateoc at gmail.com [mailto:florin.mateoc at gmail.com]> wrote:

>

>

> I should have also mentioned that PNGReadWriterTest also has 18 failures for SqueakJS, and I think I remember them failing in TruffleSqueak as well

>

> On Sun, Jan 9, 2022 at 10:18 PM Florin Mateoc <florin.mateoc at gmail.com [mailto:florin.mateoc at gmail.com]> wrote:

>>

>> Hi all, and Happy New Year!

>>

>> I think I just found a couple of bugs in the DeflatePlugin. They were causing errors in my JsSqueak, I don't fully understand why they don't seem to cause problems in Squeak. Anyway, after I fixed them, the primitives work and the PNGReadWriterTest tests are all succeeding for me.

>>

>> One is in #loadZipEncoderFrom:

>> The method checks and then determines the writeStreamInstSize and then fetches the bitBuffer and bitPosition (the first two instvars of ZipEncoder, a direct subclass of WriteStream) from offsets writeStreamInstSize + 1 and writeStreamInstSize + 2, although the fetches are 0-based

>>

>> The other is in #primitiveZipSendBlock

>> The method, still working on a ZipEncoder, at the end stores the modified values for bitBuffer and bitPosition at offset readStreamInstSize + 1 and readStreamInstSize + 2, which is just wrong.

>>

>> As I said, I don't quite get why this does not cause at least image-side errors if not VM crashes in Squeak, but they should probably be fixed anyway :)

>>

>> All the best,

>> Florin

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.squeakfoundation.org/pipermail/vm-dev/attachments/20220111/f9d95c04/attachment-0001.html>