Smalltalk › Squeak › Squeak VM

What a surface mess !!!

_‹ Previous Topic Next Topic _›

Classic

List

Threaded

4 messages Options

Nicolas Cellier

What a surface mess !!!

Hi,

I'm looking at various surface plugins... It's a mess.

There is a mix of int/long/void * in the various declarations.

What is very nice, is that they are all of equal size in ILP32 convention, so we can be very careless and ignore all the compiler warnings.

Well, until we try to port to LP64 or LLP64...

When trying to untangle that mess, I see there is the handle/pointer/ID confusion.

SurfacePlugin.h expects a surfaceHandle to be a void *, and declare that it will pass that type to the four functions (getSurfaceFormat,lockSurface,unlockSurface,showSurface).

Of course, the comment a few lines below does not quite agrees, it says it's rather a long.

The functions defined in end of same file and which are used by interpreterProxy interface declare the surfaceHandle as long...

And sqMacWindow.c uses an int handle!!! It's used as an ID.

I would have thought that the conversion SurfaceID -> SurfaceHandle would have occured a bit sooner, but here no.

Same mess for the other parameters, x,y,w,h,pitch,isMSB, they are either declared int or long... It may work by pure luck on little endian machines when we pass args by registers, and SYSV X64 convention uses a lot of these, or when each arg passed thru stack is 8-bytes aligned, and when the caller has the longer type. But that's dramatically wrong when we pass a pointer to such data, we'll then get pure UB related to uninitialized MSB...

I'm going to fix that, don't be amazed if you see a bit of traffic in the next hours.

johnmci

Re: What a surface mess !!!

Nicolas

There is another bug you should be aware of. Bitblit aka the drawing engine will read one byte beyond the edge of the pixel rectangle to provide transparency/blurring. Oddly this causes a page protection fault if the screen area is a multiple of the Operating System page size and the drawing area maps the end byte of the blit Say the screen area is 4096 byte, the prefetch will read the 4097 byte causing the fault because likely you don't owe that page, due to the VM manger being helpful in preventing out of bounds read/write operations off the end of your allocated memory. Especially true on OS X.

A decade back for Sophie we looked at fixing it but simply adjusting the algorithm didn't work as expected(drawing artifacts when swirling the mouse cursor) Or range checking during the copy of each raster line was very expensive.

Sent from my iPhone

On Nov 11, 2016, at 18:27, Nicolas Cellier <[hidden email]> wrote:

Hi,
I'm looking at various surface plugins... It's a mess.

There is a mix of int/long/void * in the various declarations.
What is very nice, is that they are all of equal size in ILP32 convention, so we can be very careless and ignore all the compiler warnings.
Well, until we try to port to LP64 or LLP64...

When trying to untangle that mess, I see there is the handle/pointer/ID confusion.

SurfacePlugin.h expects a surfaceHandle to be a void *, and declare that it will pass that type to the four functions (getSurfaceFormat,lockSurface,unlockSurface,showSurface).

Of course, the comment a few lines below does not quite agrees, it says it's rather a long.

The functions defined in end of same file and which are used by interpreterProxy interface declare the surfaceHandle as long...

And sqMacWindow.c uses an int handle!!! It's used as an ID.
I would have thought that the conversion SurfaceID -> SurfaceHandle would have occured a bit sooner, but here no.

Same mess for the other parameters, x,y,w,h,pitch,isMSB, they are either declared int or long... It may work by pure luck on little endian machines when we pass args by registers, and SYSV X64 convention uses a lot of these, or when each arg passed thru stack is 8-bytes aligned, and when the caller has the longer type. But that's dramatically wrong when we pass a pointer to such data, we'll then get pure UB related to uninitialized MSB...

I'm going to fix that, don't be amazed if you see a bit of traffic in the next hours.

smime.p7s (3K) Download Attachment

timfelgentreff

Re: What a surface mess !!!

There is even a specific check to work around that BitBlt bug in the simulation when reading from any CArrayAccessor. Balloon has a similar bug, but instead of reading beyond the memory, it reads one word before.

John McIntosh <[hidden email]> schrieb am Sa., 12. Nov. 2016, 03:51:

Nicolas

There is another bug you should be aware of. Bitblit aka the drawing engine will read one byte beyond the edge of the pixel rectangle to provide transparency/blurring. Oddly this causes a page protection fault if the screen area is a multiple of the Operating System page size and the drawing area maps the end byte of the blit Say the screen area is 4096 byte, the prefetch will read the 4097 byte causing the fault because likely you don't owe that page, due to the VM manger being helpful in preventing out of bounds read/write operations off the end of your allocated memory. Especially true on OS X.

A decade back for Sophie we looked at fixing it but simply adjusting the algorithm didn't work as expected(drawing artifacts when swirling the mouse cursor) Or range checking during the copy of each raster line was very expensive.

Sent from my iPhone

On Nov 11, 2016, at 18:27, Nicolas Cellier <[hidden email]> wrote:

Hi,
I'm looking at various surface plugins... It's a mess.

There is a mix of int/long/void * in the various declarations.
What is very nice, is that they are all of equal size in ILP32 convention, so we can be very careless and ignore all the compiler warnings.
Well, until we try to port to LP64 or LLP64...

When trying to untangle that mess, I see there is the handle/pointer/ID confusion.

SurfacePlugin.h expects a surfaceHandle to be a void *, and declare that it will pass that type to the four functions (getSurfaceFormat,lockSurface,unlockSurface,showSurface).

Of course, the comment a few lines below does not quite agrees, it says it's rather a long.

The functions defined in end of same file and which are used by interpreterProxy interface declare the surfaceHandle as long...

And sqMacWindow.c uses an int handle!!! It's used as an ID.
I would have thought that the conversion SurfaceID -> SurfaceHandle would have occured a bit sooner, but here no.

Same mess for the other parameters, x,y,w,h,pitch,isMSB, they are either declared int or long... It may work by pure luck on little endian machines when we pass args by registers, and SYSV X64 convention uses a lot of these, or when each arg passed thru stack is 8-bytes aligned, and when the caller has the longer type. But that's dramatically wrong when we pass a pointer to such data, we'll then get pure UB related to uninitialized MSB...

I'm going to fix that, don't be amazed if you see a bit of traffic in the next hours.

Nicolas Cellier

Re: What a surface mess !!!

Argh! Thanks both for the reminder.

I was acting more on the form (use correct/consistent declarations) than the substance (the algorithm).

So that's a well known problem (2 problems) that we can trigger with the VM simulator?

The fact that those bugs remain after all these years makes me wonder if they are really that hard to solve?

2016-11-12 8:45 GMT+01:00 Tim Felgentreff <[hidden email]>:

There is even a specific check to work around that BitBlt bug in the simulation when reading from any CArrayAccessor. Balloon has a similar bug, but instead of reading beyond the memory, it reads one word before.

John McIntosh <[hidden email]> schrieb am Sa., 12. Nov. 2016, 03:51:

Nicolas

There is another bug you should be aware of. Bitblit aka the drawing engine will read one byte beyond the edge of the pixel rectangle to provide transparency/blurring. Oddly this causes a page protection fault if the screen area is a multiple of the Operating System page size and the drawing area maps the end byte of the blit Say the screen area is 4096 byte, the prefetch will read the 4097 byte causing the fault because likely you don't owe that page, due to the VM manger being helpful in preventing out of bounds read/write operations off the end of your allocated memory. Especially true on OS X.

A decade back for Sophie we looked at fixing it but simply adjusting the algorithm didn't work as expected(drawing artifacts when swirling the mouse cursor) Or range checking during the copy of each raster line was very expensive.

Sent from my iPhone

On Nov 11, 2016, at 18:27, Nicolas Cellier <[hidden email]> wrote:

Hi,
I'm looking at various surface plugins... It's a mess.

There is a mix of int/long/void * in the various declarations.
What is very nice, is that they are all of equal size in ILP32 convention, so we can be very careless and ignore all the compiler warnings.
Well, until we try to port to LP64 or LLP64...

When trying to untangle that mess, I see there is the handle/pointer/ID confusion.

SurfacePlugin.h expects a surfaceHandle to be a void *, and declare that it will pass that type to the four functions (getSurfaceFormat,lockSurface,unlockSurface,showSurface).

Of course, the comment a few lines below does not quite agrees, it says it's rather a long.

The functions defined in end of same file and which are used by interpreterProxy interface declare the surfaceHandle as long...

And sqMacWindow.c uses an int handle!!! It's used as an ID.
I would have thought that the conversion SurfaceID -> SurfaceHandle would have occured a bit sooner, but here no.

Same mess for the other parameters, x,y,w,h,pitch,isMSB, they are either declared int or long... It may work by pure luck on little endian machines when we pass args by registers, and SYSV X64 convention uses a lot of these, or when each arg passed thru stack is 8-bytes aligned, and when the caller has the longer type. But that's dramatically wrong when we pass a pointer to such data, we'll then get pure UB related to uninitialized MSB...

I'm going to fix that, don't be amazed if you see a bit of traffic in the next hours.