Cross file updates

I've just committed some changes to the Cross platform files that should have no effect for anything but the Acorn code.
I had to fudge a definition of fileno() so that the push/popOutputFile stuff would compile.
Added a decl for ioSetCursorARGB()
Fudged FilePlugin.h for ACORN to keep a long for my (ab)use of the lastOp field.
Fixed a mismatched decl for ioShowDisplayOnWindow() in HostWindowPlugin.h
SVN thinks there are a few other lines changed even though I can't find any difference even with a binary editor.

tim
--
tim Rowledge; [hidden email]; http://www.rowledge.org/tim
Useful random insult:- Went to the dentist to have his cranial cavity filled.

On 06-03-2013, at 5:20 AM, "David T. Lewis" <[hidden email]> wrote:

Interesting - the reason I had to make that change was because I got the equivalent complaint. I don't care much whether we actually use int or char since it's just a pointer to the bitmap that gets dereferenced. It's quite a while since I did that bit so I'm not sure what my reason was for making a choice one way or the other. I'll look after gym.

tim
--
tim Rowledge; [hidden email]; http://www.rowledge.org/tim
Two wrongs are only the beginning.

 
On Wed, Mar 06, 2013 at 10:33:44AM -0800, tim Rowledge wrote:
I'll check also as soon as I get a chance, just did not have time this morning.

I'm quite sure that the unix platform code needed an update anyway, as I think
I recall seeing this when I was checking for 64-bit cleanliness a few years ago.

BTW, I really want to put FFI back on top of the to-do list for 64-bit fixing,
as I think it is currently the biggest inhibitor to getting clean VM builds
in 64-bit mode, and nowadays folks are beginning to expect that sort of thing
to just work.

Dave
 

OK, so the original HostWindowBlah stuff used 'unsigned *' because that is what we got from
"dispBits = ((unsigned *) (interpreterProxy->firstIndexableField();"
in the plugin code. Please don't ask me why, because that was too long ago.

3.9-ish HostWindowPlugin.h uses 'unsigned *'. At that point unix has no relevant code, Mac used 'unsigned int*' (and evidently compiled despite the disagreement), windows used 'unsigned *' and obviously RISC OS used 'unsigned *' since it  matched the plugin I had originally written.

4.4-ish HostWindowPlugin.h had 'unsigned char *' until I changed it. Unix uses 'unsigned char *', windows now uses 'unsigned char *', Mac now uses 'unsigned char *' and RISC OS still uses 'unsigned *'.

The sequence tells me exactly what happened; I did my initial re-starting work in the 3.9-ish sources so I could work on something I had tools for stashed away. I made the code work with the situation pertaining at that point - ie 'unsigned *'.
When I bumped up to the 4.4-ish sources I still had the changes awaiting commit since I had no write permissions to Cross until this week; I hadn't thought that stuff in that area would change so didn't manually check the other platforms.

Now it wouldn't be hard to change the RISC OS code to use 'unsigned char *' of course, except that the generated plugin code is *still*
dispBits = ((unsigned *) (interpreterProxy->firstIndexableField(…..
which means that
ok= ioShowDisplayOnWindow(dispBits,…
really ought to be declared as
ioShowDisplayOnWindow(unsigned * displayThingy...
and not
ioShowDisplayOnWindow(unsigned char * dippitydoodah…

Except, we should recall, of course that interp.c has firstIndexableField defined as
char * first indexableField
But sqVirtualMachine.c & .h have
void * firstIndexableField
just to add fun.

Take your pick! I suppose we should probably change the plugin to generate "(unsigned char *)( (interpreterProxy->indexabibbley" and then fix sqVirtualMachine.[ch] to match and return HostWindowPlugin.h to 'unsigned char *'. Any problems that would cause?

tim
--
tim Rowledge; [hidden email]; http://www.rowledge.org/tim
C for sinking, java for drinking, Smalltalk for thinking

On Mar 7, 2013, at 05:29 , tim Rowledge wrote:

> 3.9-ish HostWindowPlugin.h uses 'unsigned *'. At that point unix has no relevant code, Mac used 'unsigned int*' (and evidently compiled despite the disagreement)

There is no disagreement.  The modifier 'unsigned' modifies an implicit 'int' type if there is no explicit type given (in exactly the same way that 'short' and 'long' modify an implicit 'int' that almost everyone omits from their code).  Implicit 'int' used to be the rule in C, rather than the exception.

> 4.4-ish HostWindowPlugin.h had 'unsigned char *' until I changed it. Unix uses 'unsigned char *', windows now uses 'unsigned char *', Mac now uses 'unsigned char *' and RISC OS still uses 'unsigned *'.

Then everything was in agreement except ROS, which was using 'unsigned int *' and should be changed to conform.

> Except, we should recall, of course that interp.c has firstIndexableField defined as
> char * first indexableField
> But sqVirtualMachine.c & .h have
> void * firstIndexableField
> just to add fun.

This should be changed to eliminate ugly warnings about incompatible definitions and/or assignments.  The semantics will not change and the different declarations will not introduce any bugs.  On every common architecture, including all that we currently support, there is effectively no difference between 'char *' and 'void *' until you try to dereference the latter.

Ian

On 06-03-2013, at 10:59 PM, Ian Piumarta <[hidden email]> wrote:
>
>> 4.4-ish HostWindowPlugin.h had 'unsigned char *' until I changed it. Unix uses 'unsigned char *', windows now uses 'unsigned char *', Mac now uses 'unsigned char *' and RISC OS still uses 'unsigned *'.
>
> Then everything was in agreement except ROS, which was using 'unsigned int *' and should be changed to conform.


I'm entirely content to make the RISC OS code conform to everyone else's usage - but it does require a minor change to the plugin code; the 'dispBits' var in primitiveShowHostWindowRect is specified as WordArray which generates as (unsigned *)(foo). Clearly it is an error to pass an unsigned * to a function specified to take an unsigned char *.

Changing to ByteArray generates dispBits as (char *) - and here I'll note the
        self var: #dispBits type: 'unsigned char * '.
in the method as having no effect whatsoever - which is better but still strictly an error vs unsigned char *.
At least, that is the viewpoint encoded into NorCroft C and I'm not about to debate C syntax with Prof. Alan Mycroft. Anyone that wishes to do so can find contact information at http://www.codemist.co.uk/ncc/index.html

I *can* suppress all implicit cast errors and make it compile but really I don't like that much. Errors are reported for a reason.


tim
--
tim Rowledge; [hidden email]; http://www.rowledge.org/tim
Cloister: a pretentious clam

 
On Thu, Mar 07, 2013 at 02:26:48PM -0800, tim Rowledge wrote:
Uh-oh. I don't like what I'm seeing here. I think there is a bug in the
code generator for SmartSyntaxInterpreterPlugin.

HostWindowPlugin deals with Bitmaps, which are arrays of 32-bit words.

The #primitiveShowHostWindow:bits:width:height:depth:left:right:top:bottom:
method declares the parameter as

        self var: #dispBits type: 'unsigned char * '.

But the translated code in primitiveShowHostWindowRect() renders it incorrectly as

        usqInt *dispBits;

and later in the same method casts it to 'unsigned *' here:

        dispBits = ((unsigned *) (interpreterProxy->firstIndexableField(interpreterProxy->stackValue(7))));

Meanwhile the header file in Cross declares it correctly for ioShowDisplayOnWindow
as 'unsigned * '.

Either 'unsigned char *' or 'unsigned *' would be reasonable declarations,
but 'usqInt *' is definitely wrong because usqInt may be 64 bits in a 64-bit
object memory, and Bitmaps are always 32 bit in either the 32-bit or the
64-bit object memory.

So the declarations in Cross and in VMMaker are inconsistent, *and* the
code generated by VMMaker is wrong. I think it's a bug that is specific to
SmartSyntaxInterpreterPlugin code generation, though I'm not certain.

Ugh.

Dave

p.s. I think that 'unsigned *' is a better declaration than 'unsigned char *'
because without it the platform code would need to sort out the little/big
endianness in the 32 bit words, right? Or did I get that bass ackwards ...
 

On 07-03-2013, at 5:13 PM, "David T. Lewis" <[hidden email]> wrote:
The self var: #dispBits type: 'unsigned char * '. line seems to do nothing in the current VMMaker; I'm pretty sure it used to. Somewhere the SmartSyntax codegen stuff is over-riding that decl (or ignoring is more likely I guess) because of the primitive declaration using a 'WordArray'; that causes the use of (unsigned *). Using ByteArray makes is use (char *) instead.


>
> Meanwhile the header file in Cross declares it correctly for ioShowDisplayOnWindow
> as 'unsigned * '.

I returned that to 'unsigned char *', though that doesn't strictly agree with the plugin 'char *'.

>
> Either 'unsigned char *' or 'unsigned *' would be reasonable declarations,
> but 'usqInt *' is definitely wrong because usqInt may be 64 bits in a 64-bit
> object memory, and Bitmaps are always 32 bit in either the 32-bit or the
> 64-bit object memory.
>
> So the declarations in Cross and in VMMaker are inconsistent, *and* the
> code generated by VMMaker is wrong. I think it's a bug that is specific to
> SmartSyntaxInterpreterPlugin code generation, though I'm not certain.

I'm reasonably sure that the VMMaker revision I just committed gets us closer. There is still a clash between unsigned char * and char * but for now I can survive it with an ugly compiler flag.

>
> Ugh.
>
> Dave
>
> p.s. I think that 'unsigned *' is a better declaration than 'unsigned char *'
> because without it the platform code would need to sort out the little/big
> endianness in the 32 bit words, right? Or did I get that bass ackwards …

It doesn't matter a lot in this case because we don't use the pointer for anything more than pointing; no structure maths for us!


tim
--
tim Rowledge; [hidden email]; http://www.rowledge.org/tim
"Bother" said Pooh, as he flunked the the sobriety test.

 
On Thu, Mar 07, 2013 at 06:07:45PM -0800, tim Rowledge wrote:
>
> The self var: #dispBits type: 'unsigned char * '. line seems to do nothing in the current VMMaker; I'm pretty sure it used to. Somewhere the SmartSyntax codegen stuff is over-riding that decl (or ignoring is more likely I guess) because of the primitive declaration using a 'WordArray'; that causes the use of (unsigned *). Using ByteArray makes is use (char *) instead.
>

It's certainly something related to that line. Nowadays, declarations of the form:

        self var: #dispBits type: 'unsigned char * '.

are done instead with method annotations:

        <var: #dispBits type: 'unsigned char * '>

but I have tried to keep the original mechanism intact in trunk VMMaker.

I tried changing the declaration to <var: #dispBits type: 'unsigned char * '>
and I still get exactly the same incorrect code generation.

Tim, if you have an older version of VMMaker readily at hand from your
recent Pi work, could you take a look at the generated code for HostWindowPlugin
and see if it has this same bug?

Dave

On 07-03-2013, at 8:06 PM, "David T. Lewis" <[hidden email]> wrote:

> I tried changing the declaration to <var: #dispBits type: 'unsigned char * '>
> and I still get exactly the same incorrect code generation.

That's a little alarming. Looks like something that used to over-ride the default with the contents of the special declarations is being sidestepped now.

>
> Tim, if you have an older version of VMMaker readily at hand from your
> recent Pi work, could you take a look at the generated code for HostWindowPlugin
> and see if it has this same bug?

The 3.9-ish code that I generated a month or so ago has
usqInt * dispBits
&
dispBits = ((unsigned *)(interpreterProxy->…

The very old 3.8 code I had archived uses 'unsigned *' in both cases but since that is what was originally in HostWindowPlugin.h that isn't surprising.


tim
--
tim Rowledge; [hidden email]; http://www.rowledge.org/tim
Useful random insult:- Full of wisdumb.

 
On Thu, Mar 07, 2013 at 11:06:32PM -0500, David T. Lewis wrote:
Actually, looking that this again I have to say that the slang makes no
sense at all. We have:

        self var: #dispBits type: 'unsigned char * '.

which as you pointed out is a declaration that contradicts the WordArray
declaration in the parameter list:

        self primitive: 'primitiveShowHostWindowRect'
                parameters: #(SmallInteger WordArray SmallInteger SmallInteger SmallInteger
                                SmallInteger SmallInteger SmallInteger SmallInteger).

My guess is that the SmartSyntax generator declares the parameter #dispBits
improperly, and somebody noticed that problem and tried to fix it by adding
the self var: #dispBits type: 'unsigned char * ', which does not the fix the
problem at all. In fact it has no effect on the generated code, but it looks
like it *should* be doing something, thus contributing to the general confusion.

I dug up and older version of HostWindowPlugin from the Squeak 3.8 time frame,
and the self var: #dispBits type: 'unsigned char * ' is not present in that
version.

Bottom line: The "self var: #dispBits type: 'unsigned char * ' " is probably
a mistake that should be removed, and there is also a bug(let) in the SmartSyntax
code generator that should be fixed.

And I hate to say it but you were probably right to change the header declaration
to (unsigned *) in the first place, so maybe your last reversion should be reverted.
Better to get the declarations right in Cross and fix up VMM and elsewhere as needed.

Dave

 
On Thu, Mar 07, 2013 at 08:36:59PM -0800, tim Rowledge wrote:
That is definitely wrong, because usqInt may be either 32 or 64 bits wide.

>
> The very old 3.8 code I had archived uses 'unsigned *' in both cases but since that is what was originally in HostWindowPlugin.h that isn't surprising.
>

This looks correct to me. Pointer to unsigned 32-bit words, the elements
of a Bitmap.

Dave