Ugh! OS_PlatformFeatures

OS_PlatformFeatures has a bug in (at least) RISC OS 3.7 and 5.x where the X bit is ignored when it generates the “unknown reason code” error (there’s a stack imbalance in its error generation code). So both X and non-X forms of the SWI end up raising an error. Naturally this makes it a bit tricky to add new reason codes to the SWI.

Since we’ve already started adding new reason codes in 5.23, I’m thinking that the least-disruptive way of fixing the bug would be to add a new flag to OS_PlatformFeatures 0 which indicates that the bug has been fixed. So anything which wants to use the new reason codes will have to check for that flag before attempting to call the option it’s actually interested in.

The only other option I can think of would be to move the new reason codes off of OS_PlatformFeatures and onto something else like OS_ReadSysInfo.

Anyone have any preferences for which of the two fixes they’d like to see?

The only other option I can think of

A quick test here shows it works on RISC OS 4.02, so it looks like it got fixed pretty early after 3.80 but never fed back into Pace’s kernel.

Another option, therefore, would be to add a ROM patch (we already have numerous ROM patches for 3.70) and not worry about 5.xx since all the platforms 5.xx has ever supported are still supported, so there’s no reason not to upgrade.

ie. adding a “I fixed it” flag to 5.23 would only be effective if you upgraded to 5.23 (and, later, 5.24) at which point you’ve fixed it anyway. Maybe that’s good enough? Either way, I’d keep processor features together in OS_PlatformFeatures rather than OS_ReadSysInfo.

A quick test here shows it works on RISC OS 4.02, so it looks like it got fixed pretty early after 3.80 but never fed back into Pace’s kernel.

Some digging through CVS reveals that it was fixed for 3.8. I guess the change wasn’t made obvious enough in order to get merged back into the main branch.

Another option, therefore, would be to add a ROM patch (we already have numerous ROM patches for 3.70) and not worry about 5.xx since all the platforms 5.xx has ever supported are still supported, so there’s no reason not to upgrade.

A ROM patch for 3.7 would be nice, but not caring about 5.xx seems a bit harsh. Remember that I’m planning on making use of the new CPU features reason code in CLib; if CLib isn’t able to detect buggy OS versions then it might stop people being able to run the softload or ROM flash tools, preventing them from upgrading altogether!

I’ve already got a version of CallASWI which adds the new CPU features reason code to 3.7/4/6 (passing any other reason codes onto the OS), so it wouldn’t be too much extra effort to have it also intercept OS_PlatformFeatures 0 and add in the ‘error generation is fixed’ flag (and for 3.7, use our own error code instead of passing unknown reasons onto the OS).

Can the module containing the new features be soft loaded onto 3.7 or 5.xx? Would it not be better to issue a softloadable version of that module, and then Boot sequences just need to ensure that the *RMEnsure the appropriate module?

I agree with Sprow on this though – it seems a bit icky to have to extend the API to cover a bug and it would make more sense to me to put a ROM patch for 3.7.

For what it’s worth I also agree about 5.xx – it makes little sense supporting the whole gamut of different versions when running 5.23 makes sense; although supporting the soft loading of the module would help.

Can the module containing the new features be soft loaded onto 3.7 or 5.xx?

The new CPU features reason code will be in CallASWI. Currently we only provide a 26bit version of CallASWI, so that will cover 3.7 but not 5.xx. There’s no reason why we can’t produce a 32bit version of CallASWI, it’s just one of those things that hasn’t been done yet (partly because there’s no technical reason why people on 5.xx can’t upgrade to a newer release)

But there’s also OS_PlatformFeatures 32 and 33 to take into account; currently there’s no module or patch to add those to earlier OS versions. So if code wanted to use those features in a safe way we’d either need to add them to CallASWI (so they can RMEnsure that), or ensure there’s a patch/module available for all applicable OS versions which fixes the bug.

We also need to consider what’s going to happen when we add more reason codes in the future.

If people are against the idea of adding a flag to indicate that the bug has been fixed, and we aren’t going to patch new OS_PlatformFeatures reason codes into old versions of 5.xx, then we could require programs to check the OS version number to see if the bug is present. This is relatively straightforward (OS_Module 20 can return the version number of ROM modules), and if there’s a ROM patch for 3.7 then there’d only be one range (5.00-5.22) to check for.

I don’t think it needs a “fixed” flag – it would set a bad precedent for starters. I’d suggest documenting the OS versions that contain the bug on the Wiki, possibly with example code to workaround it, and leave it to developers to handle how they deal with it.

It’s not been raised by anyone to date, so it’s unlikely there’s any software currently falling foul of this issue, or relying on the bug behaviour.

Agree with Jon :-).

if CLib isn’t able to detect buggy OS versions then it might stop people being able to run the softload or ROM flash tools, preventing them from upgrading altogether!

Oh yeah, that’d be disappointing.

If people are against the idea of adding a flag to indicate that the bug has been fixed, and we aren’t going to patch new OS_PlatformFeatures reason codes into old versions of 5.xx, then we could require programs to check the OS version number to see if the bug is present.

The sequence of RMEnsure’s to get CallASWI right is already pretty eye watering. How about a bit of different spin and call your new flag bit “Supports extra OS_Platform subreasons” rather than “Fixes broken OS code”?

possibly with example code to workaround it, and leave it to developers to handle how they deal with it.

I think Jeffrey’s point was you can normally do something like

SWI   XOS_ThingyMaBob
MOVVS r0, #DefaultValue
:
Do whatever with r0

It’s not been raised by anyone to date, so it’s unlikely there’s any software currently falling foul of this issue, or relying on the bug behaviour.

You do realise what is going wrong here, don’t you?

>SYS "XOS_PlatformFeatures", 123 TO a%, b%

Unknown OS_PlatformFeatures reason code
>

Namely, calling a future defined code on a current (or older) version of RISC OS that doesn’t support the code will result in an error being raised, regardless of whether or not you specified the ‘X’ form of the SWI.

That said:

I’d suggest documenting the OS versions that contain the bug on the Wiki, possibly with example code to workaround it, and leave it to developers to handle how they deal with it.

I think this is possibly better than having a “this isn’t broken” flag. Just document that there is an OS bug, how/when to detect it, and leave it to the application author to implement as they see fit.

To give an example, my server module revealed a pretty big problem with the callback ticker chain.
So I look at the OS build date, and know that I can safely use CallEvery if the build date is 12th August 2015 or later.¹ Otherwise I have to hook into TickerV and dispatch myself every fifty ticks.

   // Is it safe to use CallEvery, or should we be doing it the oldosbodge method?
   // Refer to hxxxs://www.riscosopen.org/forum/forums/5/topics/3544
   oldosbodge = TRUE;
   r.r[0] = 9; // Read OS version information
   r.r[1] = 2; // Build date string
   reply = _kernel_swi(OS_ReadSysInfo, &r, &r);
   if (reply == NULL)
   {
      if ( r.r[0] != 0 )
      {
         // we have something, so parse it
         r.r[2] = r.r[0]; // time string
         r.r[0] = -1;     // current territory
         r.r[1] = 3;      // format is "%W3, %DY-%M3-%CE%YR.%24:%MI:%SE"
         r.r[3] = (int)ordinals;
         _kernel_swi(Territory_ConvertTimeStringToOrdinals, &r, &r);

         // I have removed the example time checking code as it uses a
         // custom "SYSTIME" structure; however:
         //     ordinals[4] is the day
         //     ordinals[5] is the month
         // and ordinals[6] is the year
         // so validate as you see fit.

         // Set oldosbodge to FALSE if the system date is later or
         // equal to an acceptable version. 

         ...
      }
   }

If you can’t read the OS build date, assume that it is a vulnerable version (this code does, setting the bodge flag to TRUE unless conditions are met).

¹ It looks like the problem was introduced some time in 2014; however my server is intended to run on RISC OS 5; if it runs on earlier systems then this is by luck more than design (I require TaskManager_StartTask which rules out any Acorn-released version of RISC OS).

How about a bit of different spin and call your new flag bit “Supports extra OS_Platform subreasons” rather than “Fixes broken OS code”?

I don’t really care what the flag is called; they key thing is to make it clear that there’s a bug in the OS and that failing to check the flag before calling a new reason code will cause bad things to happen on old machines.

But from that perspective, clearly labelling the flag as “bug has been fixed” makes it more obvious to the programmer that the flag must be used, rather than it just being an optional informative thing.

CallASWI suffers from the same bug (for the OS_PlatformFeatures implementation it provides on < 3.7). Using a flag to indicate that the bug is fixed is going to be much easier for programmers to deal with than requiring them to check multiple OS/module version numbers.