#375 M68K/BCPL: toBSTR is broken
I examined the "toBSTR" routine in the Kickstart 1.3 ROM and compared it with the AROS implementation. As far as I can tell, the AROS version is completely broken:
- Data is copied *from* destination *to* source.
- Even if that were fixed, the terminating 0 byte is also copied to the dest; it shouldn't be.
- The instruction which updates the length field of the dest adds 2 (not 1) to a longword (not byte).
In addition to that, there are two other issues:
- There is no length limiting, so a >255-char C string will overwrite more than 256 bytes at the destination.
- The Kickstart toBSTR code returns a BPTR to the destination in D1. The current AROS code returns a normal pointer to the destination.
I've pasted the toBSTR code below, with some comments (the comments don't address the return value and lack of length limit). Below that is a suggested fully-fixed version, which I haven't tested at all.
BCPL toBSTR /* -7c, void, &src, @dest */
movem.l %d1-%d2,%sp@-
lsl.l #2,%d2 ;Get pointer to dest
move.l %d2,%d0
addq.l #1,%d2 ;Point to where string will be copied to
clr.b %a0@(%d0) ;Clear the length byte in dest
0:
move.b %a0@(%d2),%a0@(%d1) ;Copy *from* dest *to* source!?!?! - should be %a0@(%d1),%a0@(%d2)
; But even then, we'd end up copying the terminating 0 byte.
tst.b %a0@(%d1) ;Reached end of string?
beq 1f
addq.l #1,%d1 ;Update pointers
addq.l #1,%d2
addq.l #2,%a0@(%d0) ;Add *2* to length *longword*???? - should be addq.b #1,%a0@(%d0)
jmp 0b
1:
movem.l %sp@+,%d1-%d2
BRTS
Hopefully-fixed (untested) code:
BCPL toBSTR /* -7c, void, &src, @dest */
movem.l %d1-%d2,%sp@-
lsl.l #2,%d2 ;Get pointer to dest
move.l %d2,%d0
addq.l #1,%d2 ;Point to where string will be copied to
clr.b %a0@(%d0) ;Clear the length byte in dest
0:
tst.b %a0@(%d1) ;Reached end of string?
beq 1f
move.b %a0@(%d1),%a0@(%d2)
addq.l #1,%d1 ;Update pointers
addq.l #1,%d2
addq.b #1,%a0@(%d0) ;Update length byte in dest
bne 0b ;Ensure we don't copy more than 255 bytes to dest
st %a0@(%d0) ;Max length is 255.
1:
lsr.l #2,%d0 ;Return BPTR to dest
movem.l %sp@+,%d1-%d2
BRTS
My hopefully-fixed code isn't entirely compatible with the code in Kickstart 1.3 (at address $FF4EB4 if anyone wants to check).
When the source string is over 255 bytes long, the Kickstart routine -- if my reading of the code is correct -- writes an empty BCPL string (i.e. a single 0 byte) to the destination. My version would write the first 255 characters in that case.
The Kickstart code determines the source string length before doing the copy, and a properly-fixed equivalent would have to do that. I'll try to come up with something.
Fixed in r44787 - please verify and close