[BASIC] Fix 1E39 bug (Issue #247)

[calmsacibis995]

The following modifications in basic/code17.s fixes the 1E39 bug (see #247) that we inherited from the C64.

[calmsacibis995]

I replaced strng2 with index2, and it no longer errors out when using VAL() with valid parameters, but the comment is still cut off when it exceeds a set number of characters.

[mooinglemur]

I don't think any trivial changes like this would solve this completely. It all comes down to how passing strings around happens in BASIC.

If the string passed to VAL() is a string literal, let's take your example. In the original code, the final quote is replaced with a null, and thus the terminated string 1E39 is passed to the math routine.

Your usage of sta (##),y simply places the null byte somewhere where it doesn't belong, which is a little bit further into the program source. The reason this gives the right answer is that there is a null character at the end of the line with the VAL string in the BASIC source. Passing the string 1234") into the assembly routine gives the right answer, simply because the punctuation at the end of the string does not affect conversion of the string into the number.

Where the lack of termination in the correct spot will certainly frequently break is if you call VAL(A$). Since you're passing in a variable, the value is coming from the string heap. If you pass in an unterminated string, the assembly routine may not find a null character anywhere within the first 256 bytes after the number starts, or it may find one but the routine gets a combined string that evaluates into a different number.

The most simple solutions here would probably involve allocating a new string off the heap with a length of the original string +1, and then copying the data to the new location while terminating it, and then allowing garbage collection to reclaim the space later.

[calmsacibis995]

I reverted my chages from sta (##),y to sta abs and reworked the function by handling strings properly, allocating a string using the strspa function, with a length of the original +1, as you said above.

As you can see in the screenshot below, the comment is not destroyed, regardless of its length, due to proper string handling, but the VAL() error message is reporting the wrong answer when given real use cases.

e.g. line 10: PRINT VAL("1234") should report 1234 and exit without an error message. But it reports 0 and throws an error message.

But I'm so close to fixing this ancient bug.

[mooinglemur]

I think the primary issue with your current code is that you tried to allocate variables in ROM space. You won't get assembler warnings, it simply won't work because trying to store to a ROM address is essentially a no-op, so your read-back would be the default fill value in ROM.

There are locations you can use to store temporary state in BASIC, but you have to be extra careful that there's no stomping on other state. I don't think we need it though.

val currently uses these variables:

• txtptr - the current execution pointer, but we need to reposition it for use with finh, so we save its contents in strng2 temporarily
• strng2 (shared wth bufptr and curtol as well as the math vars polypt and fbufpt) - used only for preserving txtptr
• index1 - the current evaluation pointer, e.g. the beginning of the number we're evaluating. We currently copy this into txtptr before calling finh.
• index2 - after adding the string length to index1, so that we can terminate (and restore) it.

Unfortunately, strspa is destructive to index1 if it needs to perform garbage collection before giving you a string descriptor, so if this is what you want to do, you will need to preserve index1.

I'd structure it something like this:

val jsr len1
val_str bne @1
    jmp zerofc
@1: ldx txtptr
    ldy txtptr+1
    ;stx strng2
    ;sty strng2+1
    ; I would just use the stack here.  I imagine the original devs would have also done so if they had access to 65C02 opcodes
    phy
    phx
    ;ldx index1
    ;stx txtptr
    ;clc
    ;adc index1
    ;sta index2
    ;ldx index1+1
    ;stx txtptr+1
    ;bcc val2
    ;inx
;val2   stx index2+1
    ; I would preserve index1 here, also using the stack, and also without disturbing .A which contains our string length
    ldx index1
    ldy index1+1
    phy
    phx
    ; now we can preserve the length, and then get a new string descriptor
    pha ; string length is here
    inc ; leave space for null termination
    jsr strspa
    ; now dsctmp+1 points to our new string space, and index1 is assumed trashed, so let's restore it
    plx ; string length
    pla
    sta index1
    pla
    sta index1+1
    lda dsctmp+1
    sta txtptr
    lda dsctmp+2
    sta txtptr+1
    ldy #0
@2: lda (index1),y
    sta (txtptr),y
    iny
    dex
    bne @2
    lda #0
    sta (txtptr),y ; null terminate
    ; now finish it off and call finh (txtptr points to our string copy)
    ; and then be sure to unwind the stack and restore txtptr to the state where val was called

[mooinglemur]

perhaps some cheap bounds checking after inc could be useful too (beq to string too long error in case we have a 255 character string)

[mooinglemur]

As an aside, for code style reasons, please use spaces for comments on the same line as assembly code rather than tabs. There should be a maximum of one tab per line, and the first indent character should always be a tab, allowing for good visual alignment regardless of tab width.

[calmsacibis995]

I went with your suggestion, and it fixed the issue. I made my modifications to handle the pointers correctly at the end, by pulling them from the stack that we had saved previously, and added an rts and the end. I also added a check for a string that is 255 characters long (the null byte is included).

Even though there is a valrts function that is supposed to end the function, when I removed it, thinking that there was no point in keeping it, but it breaks VAL(), so I kept it.

[calmsacibis995]

I believe now it can be merged, as a proper, permanent fix has been implemented.

[mooinglemur]

Unfortunately with the code you pushed, this is not what happens for me.

[calmsacibis995]

OK, let's do what you suggested. That should fix it.

[calmsacibis995]

OK, VAL() is now fixed. It should work as intended.