Fix bad pointer dereference and other issues with *ScreenLoad

Detail:
  s/vdu/vdugrafj - Rewrite WritePaletteFromSprite to fix some issues with the logic which decides whether to change screen mode:
  - ModeNo was always being treated as if it was a mode selector block, causing bad pointer dereferences if it was actually a numbered mode. Prior to zero page protection the code would have eventually stumbled its way through to the mode change code.
  - For checking the pixel format, only the Log2BPP was being compared, resulting in code deciding that (e.g.) modes with differing RGB order were the same
  - However the eigen value checking was also broken (checking wrong part of generated mode selector block), causing the mode change logic to always be taken
  s/vdu/vdugrafdec - Increase size of SloadModeSel so it's actually large enough to hold the generated mode selector - old code would have run off the end a bit (thankfully, it was the last thing in that particular workspace block)
Admin:
  Tested on BB-xM, *ScreenLoad'ing sprites from various modes
  Fixes issue reported on forums:
  https://www.riscosopen.org/forum/forums/4/topics/3649


Version 5.35, 4.79.2.294. Tagged as 'Kernel-5_35-4_79_2_294'

VersionASM

@@ -13,11 +13,11 @@
                         GBLS    Module_ComponentPath
 Module_MajorVersion     SETS    "5.35"
 Module_Version          SETA    535
-Module_MinorVersion     SETS    "4.79.2.293"
-Module_Date             SETS    "10 Oct 2015"
-Module_ApplicationDate  SETS    "10-Oct-15"
+Module_MinorVersion     SETS    "4.79.2.294"
+Module_Date             SETS    "12 Oct 2015"
+Module_ApplicationDate  SETS    "12-Oct-15"
 Module_ComponentName    SETS    "Kernel"
 Module_ComponentPath    SETS    "castle/RiscOS/Sources/Kernel"
-Module_FullVersion      SETS    "5.35 (4.79.2.293)"
-Module_HelpVersion      SETS    "5.35 (10 Oct 2015) 4.79.2.293"
+Module_FullVersion      SETS    "5.35 (4.79.2.294)"
+Module_HelpVersion      SETS    "5.35 (12 Oct 2015) 4.79.2.294"
                         END

VersionNum

@@ -5,19 +5,19 @@
  *
  */
 #define Module_MajorVersion_CMHG        5.35
-#define Module_MinorVersion_CMHG        4.79.2.293
-#define Module_Date_CMHG                10 Oct 2015
+#define Module_MinorVersion_CMHG        4.79.2.294
+#define Module_Date_CMHG                12 Oct 2015
 
 #define Module_MajorVersion             "5.35"
 #define Module_Version                  535
-#define Module_MinorVersion             "4.79.2.293"
-#define Module_Date                     "10 Oct 2015"
+#define Module_MinorVersion             "4.79.2.294"
+#define Module_Date                     "12 Oct 2015"
 
-#define Module_ApplicationDate          "10-Oct-15"
+#define Module_ApplicationDate          "12-Oct-15"
 
 #define Module_ComponentName            "Kernel"
 #define Module_ComponentPath            "castle/RiscOS/Sources/Kernel"
 
-#define Module_FullVersion              "5.35 (4.79.2.293)"
-#define Module_HelpVersion              "5.35 (10 Oct 2015) 4.79.2.293"
+#define Module_FullVersion              "5.35 (4.79.2.294)"
+#define Module_HelpVersion              "5.35 (12 Oct 2015) 4.79.2.294"
 #define Module_LibraryVersionInfo       "5:35"

s/vdu/vdugrafdec

@@ -270,7 +270,7 @@ ScrLoaAreaCB   # SpriteAreaCBsize
 
 SPltAction # 4                          ; Plot action used (0 => store)
 
-SloadModeSel # 48                       ; Mode selector for screenloading new sprites
+SloadModeSel # 56                       ; Mode selector for screenloading new sprites
 
         ASSERT @ < EndGraphicWs
 

s/vdu/vdugrafj

@@ -1038,193 +1038,167 @@ WritePaletteFromSprite ROUT
         LDR     R0, [WsPtr, #ModeNo]
         LDR     R1, [R2, #spMode]
 
-        [ {TRUE} :LAND: ModeSelectors
-
         ;logic for this routine
         ;
         ;[WsPtr, #ModeNo] is the current mode/ptr to mode selector
         ;R2 points at sprite data
-        ;[WsPtr, #SloadModeSel] is 36 bytes for building a mode selector for the sprite
+        ;[WsPtr, #SloadModeSel] is space for building a mode selector for the sprite
         ;
-        ;sprite mode < 256 ?
-        ;yes: equal to current mode ?
-        ;     yes: already in correct mode. done.
-        ;     no:  change to mode sprite wants, done.
-        ;no:  build a mode selector for the sprite
-        ;     check pixel depth, xres, yres, xdpi and ydpi
-        ;     all identical ?
-        ;     yes: already in suitable mode. done.
-        ;     no:  change mode. done.
-        ;if we do a mode change, remember to re-remove cursors
-
-        ;amg 15 Oct '93 Screensave is about to be changed to use a representative
-        ;mode number of the eigs and depth (only), so screenload no longer believes
-        ;the screen mode number in the file.
-
-        ;amg 21 Dec '93 Slight modification - if the screen mode change failed, and
-        ;we have an old screen mode number, use that as a last gasp
-
-;        CMP     R1, #256
-;        BCS     %FT30                   ;branch if a new format sprite mode word
-
-;        CMP     R1, R0                  ;are we in the right (old style) mode ?
-;        BEQ     %FT10
-
-;        MOV     R0,#ScreenModeReason_SelectMode
-;        SWI     XOS_ScreenMode
-;        STRVS   R0, [WsPtr, #RetnReg0]  ;exit on error
-;        Pull    "R0-R6,PC", VS
-;        B       %FT40                   ;otherwise get on with it
-
-30      ; new format sprite mode word
-        ; build the mode selector at SLoadModeSel
-
+        ; 1. construct a mode selector for the sprite (even if the sprite uses
+        ;    a numbered mode, we prefer to use a mode selector because the
+        ;    sprite width/height might not match that of the mode - old, deleted
+        ;    comments from '93 suggest that ScreenSave 'is about to be changed'
+        ;    to save out mode numbers even if a numbered mode isn't in use)
+        ; 2. compare the mode selector (width, height, eigen values, pixel
+        ;    format) against the current mode
+        ; 3. if any of the values differ, try using the mode selector we
+        ;    generated to change mode
+        ; 4. if using the mode selector failed, and the sprite specified a
+        ;    numbered mode, try changing to that mode directly
+        ; 5. give up if all mode changes failed
+        ; 6. if we changed mode, remember to re-remove cursors
+        ; 7. proceed to write palette
+
+        ; Step 1: mode selector construction
         MOV     R5, R1                  ;keep the mode number/sprite mode word safe
-
-        ;do the absolutes first
-        MOV     R3, #-1
-        STR     R3, [WsPtr, #SloadModeSel+ModeSelector_FrameRate]
-        STR     R3, [WsPtr, #SloadModeSel+ModeSelector_ModeVars+16] ;list terminator after two pairs
-        STR     R3, [WsPtr, #SloadModeSel+ModeSelector_ModeVars+32] ;list terminator after four pairs
-        MOV     R3, #128
-        STR     R3, [WsPtr, #SloadModeSel+ModeSelector_ModeVars+20]  ;modeflags value, if needed
-        MOV     R3, #VduExt_NColour
-        STR     R3, [WsPtr, #SloadModeSel+ModeSelector_ModeVars+24]
-        MOV     R3, #255
-        STR     R3, [WsPtr, #SloadModeSel+ModeSelector_ModeVars+28]
-
+        MOV     R4, R2                  ;save the sprite pointer
 
         MOV     R3, #1
         STR     R3, [WsPtr, #SloadModeSel+ModeSelector_Flags]
-        MOV     R3, #VduExt_XEigFactor
-        STR     R3, [WsPtr, #SloadModeSel+ModeSelector_ModeVars]    ; modevar 1 = xeig
-        MOV     R3, #VduExt_YEigFactor
-        STR     R3, [WsPtr, #SloadModeSel+ModeSelector_ModeVars+8]  ; modevar 2 = Yeig
-
-        ;now the things from the sprite
-;        MOV     R3, R1, LSR #27         ;sprite type
-;        ADRL    R4, NSM_bpptable-4      ;readmodevar's table
-;        LDR     R3, [R4, R3, LSL #2]    ;word index
-;        STR     R3, [WsPtr, #SloadModeSel+ModeSelector_PixelDepth]
-
-        ;change to calling read mode variable to cope with mode number or sprite mode word
-        MOV     R4, R2                  ;save the sprite pointer
-        MOV     R0, R5                  ;sprite mode word/mode number
-        MOV     R1, #VduExt_Log2BPP
-        SWI     XOS_ReadModeVariable
-        STR     R2, [WsPtr, #SloadModeSel+ModeSelector_PixelDepth]
-        MOV     R3, R2
-
-        ;if log2bpp=3, and size of palette data indicates full palette, we need to force
-        ;a suitable mode
-        CMP     R3, #3
-        BNE     %FT40
-
-        ADD     LR, R4, #spImage        ;point to image/mask start
-        LDMIA   LR,{R2,LR}              ;fetch them
-        CMP     R2,LR                   ;which is bigger ?
-        MOVGT   R2,LR                   ;use the least
-        SUB     R2,R2,#spPalette        ;and the palette size is...
-
-        CMP     R2,#&800                ;full entry 256 colour
-
-        ;change the mode selector so it includes a modeflags word
-        ;(following two words already set up)
-
-        MOVEQ   R2, #0
-        ;amg 28/4/94 bugfix - following inst wasn't conditional
-        STREQ   R2, [WsPtr, #SloadModeSel+ModeSelector_ModeVars+16]
-
-40
-        MOV     R2, R4                  ;restore the sprite pointer
-
-        LDR     R4, [R2, #spWidth]      ;number of words
-        MOV     R4, R4, LSL #5          ;convert to a number of bits
-        LDR     LR, [R2, #spRBit]       ;last bit used
-        ADD     LR, LR, #1              ;convert to number of bits
-        ADD     R4, R4, LR              ;combine
-        MOV     R4, R4, LSR R3          ;and convert to pixels
-        STR     R4, [WsPtr, #SloadModeSel+ModeSelector_XRes]
 
-        LDR     R3, [R2, #spHeight]
-        ADD     R3, R3, #1
-        STR     R3, [WsPtr, #SloadModeSel+ModeSelector_YRes]
-        MOV     R6, R2                  ;save the sprite pointer for later
-
-        ;that leaves the x and y eig factors, which are derived
-        ;from the dpi
-
-        MOV     R0, R5                  ;R0 = sprite mode word
+        MOV     R0, R5
         MOV     R1, #VduExt_XEigFactor
+        STR     R1, [WsPtr, #SloadModeSel+ModeSelector_ModeVars]
         SWI     XOS_ReadModeVariable
         STRCC   R2, [WsPtr, #SloadModeSel+ModeSelector_ModeVars+4]
+
         MOVCC   R1, #VduExt_YEigFactor
+        STRCC   R1, [WsPtr, #SloadModeSel+ModeSelector_ModeVars+8]
         SWICC   XOS_ReadModeVariable
         STRCC   R2, [WsPtr, #SloadModeSel+ModeSelector_ModeVars+12]
-        BCS     %FT90                   ;we canna take it captain....
 
-        ;do the comparison which involve the mode selectors first
+        MOVCC   R1, #VduExt_ModeFlags
+        STRCC   R1, [WsPtr, #SloadModeSel+ModeSelector_ModeVars+16]
+        SWICC   XOS_ReadModeVariable
+        STRCC   R2, [WsPtr, #SloadModeSel+ModeSelector_ModeVars+20] ; n.b. may be tweaked below for 8bpp modes
 
-        ;depth
-        LDR     LR, [WsPtr, #ModeNo]
+        MOVCC   R1, #VduExt_NColour
+        STRCC   R1, [WsPtr, #SloadModeSel+ModeSelector_ModeVars+24]
+        SWICC   XOS_ReadModeVariable
+        STRCC   R2, [WsPtr, #SloadModeSel+ModeSelector_ModeVars+28]
 
-        LDR     R3, [LR, #ModeSelector_PixelDepth]
-        LDR     R4, [WsPtr, #SloadModeSel+ModeSelector_PixelDepth]
-        TEQ     R3, R4
-        BNE     %FT80                   ;need to change mode to new mode selr
+        MOVCC   R1, #VduExt_Log2BPP
+        SWICC   XOS_ReadModeVariable
+        STRCC   R2, [WsPtr, #SloadModeSel+ModeSelector_PixelDepth]
+        BCS     %FT90                   ;bad mode
 
-        LDR     R3, [LR, #ModeSelector_XRes]
-        LDR     R4, [WsPtr, #SloadModeSel+ModeSelector_XRes]
-        TEQ     R3, R4
-        BNE     %FT80                   ;need to change mode to new mode selr
+        MOV     R3, #-1
+        STR     R3, [WsPtr, #SloadModeSel+ModeSelector_FrameRate]
+        ASSERT  ?SloadModeSel >= ModeSelector_ModeVars+32+4
+        STR     R3, [WsPtr, #SloadModeSel+ModeSelector_ModeVars+32] ;list terminator
 
-        LDR     R3, [LR, #ModeSelector_YRes]
-        LDR     R4, [WsPtr, #SloadModeSel+ModeSelector_YRes]
-        TEQ     R3, R4
-        BNE     %FT80                   ;need to change mode to new mode selr
+        ; calculate sprite width/height
+        ; (R2 = Log2BPP from above)
 
-        ;now the eigs
-        LDR     R3, [WsPtr, #XEigFactor]
-        LDR     R4, [WsPtr, #SloadModeSel+ModeSelector_Flags+4]
-        TEQ     R3, R4
-        BNE     %FT80                   ;need to change mode to new mode selr
+        LDR     R0, [R4, #spWidth]      ;number of words
+        MOV     R0, R0, LSL #5          ;convert to a number of bits
+        LDR     LR, [R4, #spRBit]       ;last bit used
+        ADD     LR, LR, #1              ;convert to number of bits
+        ADD     R0, R0, LR              ;combine
+        MOV     R0, R0, LSR R2          ;and convert to pixels
+        STR     R0, [WsPtr, #SloadModeSel+ModeSelector_XRes]
 
-        LDR     R3, [WsPtr, #YEigFactor]
-        LDR     R4, [WsPtr, #SloadModeSel+ModeSelector_Flags+12]
-        TEQ     R3, R4
+        LDR     R3, [R4, #spHeight]
+        ADD     R1, R3, #1
+        STR     R1, [WsPtr, #SloadModeSel+ModeSelector_YRes]
 
-        BEQ     %FT10                   ;this mode is suitable
+        ; If it's an 8bpp mode, use the palette size to determine the FullPalette mode flag
+        CMP     R2, #3
+        BNE     %FT20
 
-80
+        ADD     LR, R4, #spImage        ;point to image/mask start
+        LDMIA   LR, {R6,LR}             ;fetch them
+        CMP     R6, LR                  ;which is bigger ?
+        MOVGT   R6, LR                  ;use the least
+        SUB     R6, R6, #spPalette      ;and the palette size is...
+        CMP     R6, #&800               ;full entry 256 colour
+        MOVEQ   R6, #ModeFlag_FullPalette
+        MOVNE   R6, #0
+        STR     R6, [WsPtr, #SloadModeSel+ModeSelector_ModeVars+20]
+
+20
+        ; Step 2: Compare mode selector to current mode
+
+        ; X, Y, Log2BPP checked first since we have those in registers
+        LDR     R1, [WsPtr, #XWindLimit]
+        ADD     R1, R1, #1
+        CMP     R1, R0
+        LDREQ   R1, [WsPtr, #YWindLimit]
+        CMPEQ   R1, R3
+        LDREQ   R1, [WsPtr, #Log2BPP]
+        CMPEQ   R1, R2
+
+        ; Check the other mode variables
+        LDREQ   R1, [WsPtr, #XEigFactor]
+        LDREQ   R0, [WsPtr, #SloadModeSel+ModeSelector_ModeVars+4]
+        CMPEQ   R1, R0
+        LDREQ   R1, [WsPtr, #YEigFactor]
+        LDREQ   R0, [WsPtr, #SloadModeSel+ModeSelector_ModeVars+12]
+        CMPEQ   R1, R0
+        LDREQ   R1, [WsPtr, #NColour]
+        LDREQ   R0, [WsPtr, #SloadModeSel+ModeSelector_ModeVars+28]
+        CMPEQ   R1, R0
+        BNE     %FT30
+
+        ; Check ModeFlags last because there are several flags we need to ignore
+        LDR     LR, =ModeFlag_FullPalette :OR: ModeFlag_64k :OR: ModeFlag_DataFormat_Mask
+        CMP     R2, #7
+        ORRHI   LR, LR, #ModeFlag_ChromaSubsampleMode ; Only include if actually a YCbCr mode (aliases GreyscalePalette and would cause false-positives otherwise)
+        LDR     R1, [WsPtr, #ModeFlags]
+        LDR     R0, [WsPtr, #SloadModeSel+ModeSelector_ModeVars+20]
+        EOR     R1, R1, R0
+        ANDS    R1, R1, LR
+        BEQ     %FT70
+
+30
+        ; Step 3: Change mode using mode selector
         MOV     R0,#ScreenModeReason_SelectMode
         ADD     R1,WsPtr,#SloadModeSel
         SWI     XOS_ScreenMode
+        BVC     %FT60
 
-
-        [ {TRUE}
-        ;ensure we preserve the error pointer in situations where we can't try to
-        ;fall back to the mode number in the sprite header
-
-        ;if it errored try again if there's a mode number available
-        BVC     %FT40
-
-        LDR     R1, [R6, #spMode]
+        ; Step 4: Change mode using mode number if selector failed
+        LDR     R1, [R4, #spMode]
         BICS    R14, R1, #&FF           ; EQ if sprite mode is a number (< 256), (V still set afterwards)
         MOVEQ   R0, #ScreenModeReason_SelectMode
         SWIEQ   XOS_ScreenMode          ; if called, will set V appropriately
-        |
-        ;if it errored try again if there's a mode number available
-        BVC     %FT40
-
-        MOV     R0, #ScreenModeReason_SelectMode
-        LDR     R1, [R6, #spMode]
-        BICS    R14, R1, #&FF           ; EQ if sprite mode is a number (< 256), (V still set afterwards)
-        SWIEQ   XOS_ScreenMode          ; if called, will set V appropriately
-        ]
 
-        STRVS   R0, [WsPtr, #RetnReg0]  ;exit on error
+        ; Step 5: Give up if all mode changes failed
+        STRVS   R0, [WsPtr, #RetnReg0]
         Pull    "R0-R6,PC", VS
-        B       %FT40                   ;otherwise get on with it
+
+60
+        ; Step 6: Remember to re-remove cursors
+        SWI     XOS_RemoveCursors
+
+70
+        ; Step 7: Actually write the palette
+        LDR     R3, [R4, #spImage]
+        CMP     R3, #spPalette          ; will clear V if EQ
+        Pull    "R0-R6, PC", EQ         ; no palette data
+
+        LDR     R0, [WsPtr, #NColour]
+        ADD     R3, R4, #spPalette+8
+        ADD     R3, R3, R0, LSL #3
+75
+        LDMDB   R3!, {R1,R2}
+
+        BL      SendPalettePair         ; Note: Writes error directly to RetnReg0
+        Pull    "R0-R6, PC", VS
+        SUBS    R0, R0, #1              ; (V will be cleared by this)
+        BCS     %BT75
+        Pull    "R0-R6, PC"
+
 90
         ADRL    R0,SpriteErr_InvalidSpriteMode
       [ International
@@ -1234,50 +1208,6 @@ WritePaletteFromSprite ROUT
       ]
         STR     R0, [WsPtr, #RetnReg0]
         Pull    "R0-R6,PC"
-        |
-
-        ;as originally done this code tended to compare mode specifiers against new
-        ;sprite mode words, and worse still tried to select a mode from a new sprite
-        ;mode word. the rewrite above takes a more logical approach
-
-        CMP     R0, R1                  ; if already in correct mode
-        BEQ     %FT10                   ; then skip
-
- [ ModeSelectors
-        MOV     r0, #ScreenModeReason_SelectMode
-        SWI     XOS_ScreenMode
- |
-        MOV     R0, #22
-        SWI     XOS_WriteC
-        MOVVC   R0, R1
-        SWIVC   XOS_WriteC
- ]
-        STRVS   R0, [WsPtr, #RetnReg0]
-        Pull    "R0-R6,PC", VS
-        ]
-40
-        SWI     XOS_RemoveCursors       ; remove cursors again
-10
-        MOV     R2, R6
-        LDR     R3, [R2, #spImage]
-        CMP     R3, #spPalette          ; will clear V if EQ
-        Pull    "R0-R6, PC", EQ         ; no palette data
-
-        LDR     R4, [WsPtr, #NColour]
-
-        ADD     R3, R2, #spPalette
-        ADD     R3, R3, R4, LSL #3
-20
-        LDMIA   R3, {R1,R2}
-        MOV     R0, R4
-
-        BL      SendPalettePair
-        Pull    "R0-R6, PC", VS
-
-        SUB     R3, R3, #8
-        SUBS    R4, R4, #1              ; (V will be cleared by this)
-        BCS     %BT20
-        Pull    "R0-R6, PC"
 
 ; *****************************************************************************
 ;