RA_Pr contains incorrect formula for z3

[fredrikr]

Veneer routine RA_Pr contains a formula to calculate the address of an object: https://github.com/DavidKinder/Inform6/blob/master/veneer.c#L408

This is the formula:

i = 0-->((i+124+cla*14)/2);

The constant 14 should be 9 for z3. 124 should also be something else, as it's meant to skip past default values for properties, and that table is a different size for z3.

This formula is used when using the superclass operator with a common property. To see the error this results in, compile and run this using PunyInform:

Include "globals";
Include "puny";

Property myprop;

Class Foo
 with myprop [; print "hello "; ];

Foo Bar
  with myprop [; "No"; ];

[ Initialise;
    print "yes ";
    Bar.Foo::myprop();
];

[erkyrath]

I don't suppose you know what the 124 should be?

[erkyrath]

60, I think, but the formula needs to be tweaked anyhow. It relies on the fact that object addresses are always even in v4+, but in v3 they're not!

[erkyrath]

Pretty sure that line should be i = (i+60+cla*9)-->0; (This locates the property table block of the class object.)

However, the next line also needs to be changed:

i = CP__Tab(i + 2*(0->i) + 1, -1)+6;

...and at this point I am entirely unable to follow the logic. i + 2*(0->i) + 1 skips the property table header; CP__TAB(x, -1) then skips the property table body; what's six bytes after that? Someone else is going to have to take a look.

[fredrikr]

Can +6 be used to skip past the two first properties, because they are always certain properties for internal use?

Den sön 30 apr. 2023 22:15Andrew Plotkin @.***> skrev:

Pretty sure that line should be i = (i+60+cla*9)-->0;

However, the next line also needs to be changed:

i = CP__Tab(i + 2*(0->i) + 1, -1)+6;

...and at this point I am entirely unable to follow the logic. i + 2*(0->i) + 1 skips the property table header; CP__TAB(x, -1) then skips the property table body; what's six bytes after that? Someone else is going to have to take a look.

— Reply to this email directly, view it on GitHub https://github.com/DavidKinder/Inform6/issues/215#issuecomment-1529129980, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAHUQTDPTEZCU4NGWHEHSC3XD3B45ANCNFSM6AAAAAAXQZ4VDA . You are receiving this because you authored the thread.Message ID: @.***>

[erkyrath]

Yeesh:

Immediately following the property values table for a class (which is bound to be short, since it can only contain the short name and perhaps property 3) is a six-byte block of attributes which would be inherited from the class (*), and then a second property values table, specifying the properties which members of the class would inherit from it. (These values are accessed at run time to implement the superclass access operator :: when applied to a common property.)

(ITM 9.4)

This is six bytes of attributes even in V3.

[erkyrath]

Okay, here's the upshot.

The correct code for veneer.c is:

             #IFV3;\
             i = (i+60+cla*9)-->0;\
             #IFNOT;\
             i = 0-->((i+124+cla*14)/2);\
             #ENDIF;\

However, this doesn't work unless you also replace CPTab(). The veneer's CPTab() assumes the v4+ property table layout.

A working implementation of CP__Tab() for v3... I'm pretty sure... is:

[ CP__Tab x id size;
    while (true) {
        size = x->0;
        if (size == 0) break;
        x++;
        if (id == (size & 31)) return x;
        x = x + (size/32)+1;
    }
    if (id<0) return x+1;
    rfalse;
];

[erkyrath]

So what to do? PunyInform already replaces a couple of veneer routines (CAPr(), ClMs()). Seems like the options are:

• Let Puny replace RAPr() and CPTab() as well.
• Update RAPr() and CPTab() in veneer.c with #ifv3 sections.
• Fix all four routines in veneer.c, so Puny doesn't have to.

I admit that I'm leaning towards the second option, even though it's inelegant.

(Also note that I haven't gone through the rest of RA__Pr() to see if there are other V4-isms in there.)

[fredrikr]

I think fixing them in the compiler is better, as the compiler does indeed offer (limited) v3 support, and these changes can be done in a clean way.

Puny's changes to CAPr and ClMs are a bit of a hack, and cuts out features of Inform. Puny has to add them, but I fully understand if you don't want to include them in the compiler.

[erkyrath]

Okay, I'll wrap up a change for RAPr() and CPTab().

Puny's changes to CAPr and ClMs are a bit of a hack, and cuts out features of Inform.

Yeah, that's the rub. (Specifically it drops the ability to pass multiple arguments on to a x.prop(...) call, because v3 can't count arguments in the same way later versions do. And it drops support for create/recreate/destroy/copy.)

What I think I want to do is add #ifv3; error; sections to those veneer routines. That way you'll get a sensible error message if you try to use them in v3, rather than a weird assembly error message. Puny won't be affected because it replaces them.

[fredrikr]

Ok, great.

I did have a thought while reading this code: Properties appear in descending order in the property list. Instead of checking if we've reached the value zero, could we check for any value lower than the property we're looking for? The .& operator is often used in both PunyInform and the standard library to test if an object provides a certain property, e.g. normally all present objects will be checked to see if they have a react_before routine every move. This could be a welcome optimization, unless the check turns out to actually slow things down.

Blocking attempts to create, destroy etc with a sensible error message: Perfect.

As for calling property routines in z3: Wouldn't it be worthwile to allow it for 0 or 1 argument, and document the limitation? Puny replaces this routine out of necessity. Replacing it means it doesn't benefit from bug fixes and enhancements in the compiler, unless we notice them.

[fredrikr]

I edited my last comment. I meant to say .& is often used to check if an object provides a certain property. CP__Tab could probably be made more efficient.

Hm, odd, looking at CP__Tab now, it doesn't look like it's meant to handle v3 games at all. But we haven't had any problems with it.

[erkyrath]

CPTab() is only called from RAPr(). It's not used in normal property lookup.

I am not going to worry about optimizing CP__Tab(). That's not part of this task and I don't really want to rely on Inform's ordering of properties.

[fredrikr]

Ok.

You're right of course, it's not part of this task.

It's not Inform's choice to order the properties this way, it's part of the Z-machine specification.

Message ID: @.***>

[erkyrath]

Whoops, you're right. I was thinking of the individual property table, which is unsorted (and not part of the Z-spec).

[fredrikr]

I tested the changes to RAPr and CPTab, and they seem to work fine.

I just discovered that RL__Pr is broken for z3 as well. Should I create a new ticket for that?

[erkyrath]

Let's wrap it up into this one, since it's part of the same problem.

I thought indiv properties used the same layout on z3 and z5, though? It seems like this case would have already been tested.

[fredrikr]

RL__Pr is used for common properties in a superclass as well.

This kludge makes it work for common properties in superclass:

[ RL__Pr obj identifier x;
 if (identifier<64 && identifier>0) {
    !return obj.#identifier;
    @get_prop_addr obj identifier -> x;
    @get_prop_len x -> sp;
    @ret_popped;
 }
 x = obj..&identifier;
 if (x==0) rfalse;
 #IFV3;
 return 1 + ((x-1)->0) / 32;
 #IFNOT;
 if (identifier&$C000==$4000)
     switch (((x-1)->0)&$C0)
     {    0: return 1;  
        $40: return 2;  
        $80: return ((x-1)->0)&$3F; 
     }
 return (x-1)->0;
 #ENDIF;     
];

[erkyrath]

Are you talking about an expression like obj.#cla::prop ?

[erkyrath]

Actually, I changed my mind. I don't understand the exact issue here. Please file a separate ticket for RLPr(). We can let https://github.com/DavidKinder/Inform6/pull/217 go in as it is while I figure out what's going on with RLPr().