Bad OS_Byte 129 use in the wimp
Charles Ferguson (8243) 427 posts |
Hiya, This is a simple bug in the Wimp `pollforkey` which does not follow the documented behaviour. OS_Byte 129 (INKEY) is explicitly documented as having the following exit conditions : If reading a key within a time limit: R1 = ASCII code if character read, else undefined R2 = &00 if character read, &1B if an escape condition exists, or &FF if timeout The Wimp does this (https://gitlab.riscosopen.org/RiscOS/Sources/Desktop/Wimp/-/blob/master/s/Wimp01#L5948): MOV R0,#&81 ; read key with 0 time limit MOV R1,#0 MOV R2,#0 SWI XOS_Byte ; TEQ R1,#&FF Debug err,"Key ?" Pull "R2,R3,PC",LS ; EQ or CC So there are two behaviours that are incorrect here. 1. It’s checking R1, which is explicitly declared to be undefined. It should be checking R2. The actual behaviour of the Kernel is to set C if no key was seen, and clear C if a key was seen, but that’s not documented. It does appear to be documented for the BBC (see https://beebwiki.mdfs.net/OSBYTE_%2681) On entry: Y<128 - XY=centisecond timeout - INKEY(0) to INKEY(32767) On exit: Cy=0, Y=0, X=character : character returned Cy=1, Y=255, X preserved : timed out Cy=1, Y=27 X preserved : Escape key pressed The caller can test for either Cy=0 or Y=0 for a valid returned character. If Cy=1 or Y<>0 the contents of X should not be relied on. and my copy of the BBC Advanced user guide states: On exit, If a character is detected, X=ASCII value of key pressed, Y=0 and C=0. If a character is not detected within timeout then Y=&FF and C=1 If Escape is pressed then Y=&1B (27) and C=1. So whilst BBC wiki has it explicitly documented that X (R1 in RISC OS) should not be relied on, the AUG says nothing (and presumably should not be relied on). Therefore, the Wimp is relying on BBC behaviour (and undocumented RISC OS behaviour) for the status of the C flag, and an implementation detail for the state of R1. The Wimp can be changed to comply with documented behaviour by something like: TEQ r2, #27 BEQ %FT20 TEQ r2, #255 Pull "R2,R3,PC" ; Z=0 if key pressed; Z=1 if no key 20 ; acknowledge the escape condition |
Matthew Phillips (473) 714 posts |
Do you think the kernel behaviour in relation to the C flag ought to be documented as well, seeing as what has been inherited from the BBC has probably been that way throughout the existence of RISC OS? Or is it best left undocumented, but unchanged in case other applications rely on it. |
Stuart Swales (8827) 1326 posts |
Nasty – looks suspiciously like someone changed a CMP to TEQ (and the register is wrong).
It definitely ought to be documented as it’s one of the reasons _kernel_swi_c was concocted. Does anyone have a copy of the RISC OS 2 PRMs – was it in those? I did once have a set as part-payment for proofreading them but think they disappeared at Colton Software sometime. [Or even the RO2 WM source? Somewhat unlikely. I think most of us avoided TEQ in favour of CMP unless there was a really good reason to TEQ (e.g. TEQVCP).] |
Charles Ferguson (8243) 427 posts |
Yeah I thought that, but then noticed that the register was wrong, too.
Additional reference: Archimedes manual: https://www.4corn.co.uk/archive/docs/Archimedes%20Programmer’s%20Reference%20Manual%20-%20Volume%201-opt.pdf Page 140, PDF page 147 – no documentation of the state of C flag. However R1 is defined to be 255 if there was a timeout, so there’s some lineage here for that. RISCOS 2 manual: https://www.4corn.co.uk/archive/docs/RISC%20OS%20Programmer’s%20Reference%20Manual%20-%20Volume%202-opt.pdf Page 498, PDF page 43 – R1 declared as undefined, and no documentation of the C flag.
Both fixing the Wimp and documenting the behaviour should be done. Without documentation it’s not possible to know what the behaviour should be, and the intent is more important if you’re trying to keep things working in the future and in past versions. For example a claimant of ByteV is entirely at liberty to corrupt R1 in the case where escape or no key is pressed, according to the documentation. If they do that, things won’t work. That’s not their fault – that’s the documentation and the Wimp’s fault for not doing what was documented. When implementing Pyromaniac the interfaces are being implemented using the documentation whereever possible (except in those cases where I’m sufficiently confident that I can do it from memory – which is fallible!). Hence why this failure was found. The implementation of the ‘read with timeout’ was: # Read with timeout timeout = (high * 256) + low key = ro.kernel.input.getchar(timeout=timeout/100.0) if key is Escape: regs[1] = 0 regs[2] = 0x1B elif key is None: regs[1] = 0 regs[2] = 0xFF else: regs[1] = ord(key) regs[2] = 0 It has now been updated to report the C flag in order to keep the Wimp happy (and I’ve fixed the Wimp as suggested above). |
Stuart Swales (8827) 1326 posts |
This is the great thing about having your independent implementation! I may be a twit, but can’t access those URLs. |
Paolo Fabio Zaino (28) 1821 posts |
On the RO2 PRM (page 498): R0 – 129 (&81) So identical to what Gerph reported and just for the sake of clarity also Arthur Programmer reference reports the same. Arthur PRM page 140: On exit: Hope this helps |
Paolo Fabio Zaino (28) 1821 posts |
oh Sorry, Gerph already posted the info while I was seeking for them |
Charles Ferguson (8243) 427 posts |
There’s limited behaviour that has surprised me, although I’m working with modules from Select and I had already been stripping out the uses of undefined behaviour and private interfaces, so I may be working from a better base. There was something recently that surprised me, although it wasn’t undefined behaviour – there was some module that was using the StartupFS interface triggered by the OS_Byte version of the service call in order to determine whether a filing system was present. I think that was it. ResourceFS Filer! That was it! I fixed that in my copy but it’s nuts. My version just checks whether Resources:$ exists and if so continues. I’ll create a separate topic for that as it’s come up.
I think the links have been mangled by either copying or the forum software – certainly they’re not being linked for the entire length of the URL. I got them from here: https://www.4corn.co.uk/articles/docs/ – so you should be able to find them some way down the page. I can’t remember if an apostrophe is meant to be escaped or not, but it’s just what came out of Firefox :-( |
Stuart Swales (8827) 1326 posts |
Ta. |
Steve Pampling (1551) 8125 posts |
A pet hate: Spaces in filenames – the gift that keeps kicking you in the nads. |
Chris Mahoney (1684) 2160 posts |
The spaces aren’t the problem in this case. |
Steve Pampling (1551) 8125 posts |
They started the mangling sequence by causing the html links to be using %20 and thus difficult to read. That errant apostrophe… |
Stefano Bertinetti (2512) 21 posts |
Is this correct? |
Rick Murray (539) 13750 posts |
Depends how literal you want to be. Clearly R1 will be &FF if R1 is &FF. ☺ However, given the mention of the timeout, I think it’s reasonable to suggest that it’s a typo and they meant R1 will be &FF if R2 is &FF. That being said, I would like to draw your attention to the fifth paragraph on page 355 (that’s the very start of book two).
There is so much wrong with that statement that a part of me wonders if it got slipped in as a dare to see if anybody noticed. |
Stuart Swales (8827) 1326 posts |
I don’t remember anyone giving the Arthur PRM the once-over! We did proofread RISC OS 2 PRMs. |