I think I Found and Fixed a bug.

[JenniferDigital]

I think I found and fixed a bug, but before declaring it done, I'd just like for all you beautiful people to just look it over and see if I've missed any subtleties.

Code: Select all

; perform SWAP

LAB_SWAP
      JSR   LAB_GVAR          ; get var1 address
      STA   Lvarpl            ; save var1 address low byte
      STY   Lvarph            ; save var1 address high byte
      LDA   Dtypef            ; get data type flag, $FF=string, $00=numeric
      PHA                     ; save data type flag

      JSR   LAB_1C01          ; scan for "," , else do syntax error then warm start
      JSR   LAB_GVAR          ; get var2 address (pointer in Cvaral/h)
      PLA                     ; pull var1 data type flag
      EOR   Dtypef            ; compare with var2 data type
      
; BUGFIX:  SWAP would only swap a string with a number otherwise it would generate an error!
;
      ; BPL   SwapErr           ; exit if not both the same type
      BNE   SwapErr           ; exit if not both the same type
;
; End of bugfix.

[drogon]

JenniferDigital wrote:

I think I found and fixed a bug, but before declaring it done, I'd just like for all you beautiful people to just look it over and see if I've missed any subtleties.

Code: Select all

; perform SWAP

LAB_SWAP
      JSR   LAB_GVAR          ; get var1 address
      STA   Lvarpl            ; save var1 address low byte
      STY   Lvarph            ; save var1 address high byte
      LDA   Dtypef            ; get data type flag, $FF=string, $00=numeric
      PHA                     ; save data type flag

      JSR   LAB_1C01          ; scan for "," , else do syntax error then warm start
      JSR   LAB_GVAR          ; get var2 address (pointer in Cvaral/h)
      PLA                     ; pull var1 data type flag
      EOR   Dtypef            ; compare with var2 data type
      
; BUGFIX:  SWAP would only swap a string with a number otherwise it would generate an error!
;
      ; BPL   SwapErr           ; exit if not both the same type
      BNE   SwapErr           ; exit if not both the same type
;
; End of bugfix.

Hm... Not a great fan of diving into ehBasic, but I've had a quick look..

The test is for a string type vs. numeric type. String type is $FF, numeric is $00.

So number EOR number: $00 eor $00 -> 0 (positive)
String EOR string: $FF eor $FF -> 0 (positive)
Number EOR String: $00 EOR $FF -> $FF (negative)

I actually think the branch ought to be (or was intended to be) BMI rather than BPL, but BNE here works too. (So a logic swap waaaaaaay back)

It also makes me think that no-one has ever used the SWAP command...

Swapping a number & string is ... interesting:

I patched my version (the Floobydust 2.22p5C version) with BMI and it works as it ought to.

Cheers,

-Gordon

[floobydust]

Interesting.... when I worked on making a CMOS version of EhBasic, I implemented the patches that Klaus did, P5 being the latest. I guess I'm a bit surprised that Klaus hadn't found this particular bug.

It's been a few years since I worked on the CMOS version of EhBasic, as most of my recent activity (outside of my own projects. hardware, BIOS/Monitor code) has been creating a CMOS version of DOS/65.

So, moving forward... I will at least put the fix into the P5C code... now the decision, should this qualify as a P6 fix or something else? I'm open to suggestions.

[JenniferDigital]

So far I'm pleased that this little bugfix seems to be agreed with.

Now, as to why someone wouldn't spot this bug, I think its down to SWAP being a very uncommonly used command that isn't implemented on that many versions of BASIC and so wouldn't affect all that many people.

To err is human, to really 'make a hash of it' requires a computer. <- Polite versions of everything these days.

[teamtempest]

I'm a bit surprised that EOR was used instead of CMP, since the only point is to see if the two values are different or not. EOR takes a bit of thinking about to see if it comes out right, whereas CMP followed by BNE makes it pretty much obvious to any reader what's going on.

As for why it wasn't caught earlier, another possibility is that no one ever thought about swapping a number and string before. It's true the original author should have checked to make sure as part of the test routine, but possibly in practice no one tried to make a mistake like that.

[drogon]

teamtempest wrote:

I'm a bit surprised that EOR was used instead of CMP, since the only point is to see if the two values are different or not. EOR takes a bit of thinking about to see if it comes out right, whereas CMP followed by BNE makes it pretty much obvious to any reader what's going on.

Who knows. Lee isn't with us anymore - so it's something he (or someone else) added into the original Microsoft disassembly he worked with.

Quote:

As for why it wasn't caught earlier, another possibility is that no one ever thought about swapping a number and string before. It's true the original author should have checked to make sure as part of the test routine, but possibly in practice no one tried to make a mistake like that.

It didn't work when used correctly, but worked (or at least didn't produce an error - it didn't really work!) when you mixed string and number.

So it's been broken from day zero. It's not been found because no-one, not even the author tested it... I suspect it was added then forgotten about. It's not a common command in BASICs...

-Gordon

[cjs]

teamtempest wrote:

I'm a bit surprised that EOR was used instead of CMP, since the only point is to see if the two values are different or not. EOR takes a bit of thinking about to see if it comes out right, whereas CMP followed by BNE makes it pretty much obvious to any reader what's going on.

To a 6502 programmer, sure. But to an 8080 or Z80 programmer, who's used to exclusive-oring a value with itself to generate zero, it's probably instinctive. XRA A, or XOR A,A in Z80 syntax, is the standard idiom for loading zero into the accumulator because it's a byte shorter and more than three times faster than LDA 0.

This, and its use as a comparison tool, is even mentioned in the original 1975 Intel 8080 Assembly language Programming Manual, where the instruction is titled "XRA Logical Exclusive-Or Register or Memory With Accumulator (Zero Accumulator)," and the largest example given in that section is using XRA to determine if a new set of bits read from a device register has any bits changed from the previous value read, which naturally leads to using JNZ ("Jump if Not Zero") or CNZ to do something if the bits have changed. (Or RZ ("Return if Zero") to short-cut the remainder of the subroutine if the bits have not changed.)

[drogon]

cjs wrote:

teamtempest wrote:

I'm a bit surprised that EOR was used instead of CMP, since the only point is to see if the two values are different or not. EOR takes a bit of thinking about to see if it comes out right, whereas CMP followed by BNE makes it pretty much obvious to any reader what's going on.

To a 6502 programmer, sure. But to an 8080 or Z80 programmer, who's used to exclusive-oring a value with itself to generate zero, it's probably instinctive. XRA A, or XOR A,A in Z80 syntax, is the standard idiom for loading zero into the accumulator because it's a byte shorter and more than three times faster than LDA 0.

This, and its use as a comparison tool, is even mentioned in the original 1975 Intel 8080 Assembly language Programming Manual, where the instruction is titled "XRA Logical Exclusive-Or Register or Memory With Accumulator (Zero Accumulator)," and the largest example given in that section is using XRA to determine if a new set of bits read from a device register has any bits changed from the previous value read, which naturally leads to using JNZ ("Jump if Not Zero") or CNZ to do something if the bits have changed. (Or RZ ("Return if Zero") to short-cut the remainder of the subroutine if the bits have not changed.)

That makes sense - especially as the 6502 version came after Micro Soft wrote the 8080 version of their BASIC.... It might be interesting to see if there are other examples in the MS Basic sources like that - and I'm supposing Lee copied code from another routine that takes 2 parameters to implement his new SWAP command.

I've written (been paid to write) 8080 code just once but it was after my brain had been shaped by the 6502 so it's not something I recognise, but it might be amusing for me to go through all my old sources... One day, maybe....

-Gordon

[cjs]

drogon wrote:

That makes sense - especially as the 6502 version came after Micro Soft wrote the 8080 version of their BASIC.... It might be interesting to see if there are other examples in the MS Basic sources like that - and I'm supposing Lee copied code from another routine that takes 2 parameters to implement his new SWAP command.

I can't speak to the SWAP command, but yes, most 8-bit versions of Microsoft BASIC seem to share substantial bits of low-level design and idioms. The next one written after the original 8080 version was (the 6800 version) I belive and, though I've not looked at that for years, I seem to recall it had a bunch of things in it that really only made sense if someone had been working from the 8080 version, and the same with the 6502 version. This isn't surprising; there wasn't a lot of automated testing or unit tests or things like that in the day, so if you've got a working algorithm, you probably want to muck with it as little as you can avoid to avoid introducing an off-by-one error or something dumb like that. Even doing that, I'm sure that testing was still the greatest expense when coding this stuff.

Quote:

I've written (been paid to write) 8080 code just once but it was after my brain had been shaped by the 6502 so it's not something I recognise, but it might be amusing for me to go through all my old sources... One day, maybe....

I've found that disassembling and reverse-engineering code for other systems really helps to give one a sense of how they work. I've written less 6800 code than 6502 code, and almost no Z80 code, but I've disassembled a fair amount of all three and it's given me quite a bit of insight into the design decisions made for each CPU. (6800 vs. 6502 has been particularly enlightening.)

[JenniferDigital]

if there's one thing I've learned about software over the years its that no matter how hard you try, there's always one little bug left. I'd imagine porting code from one architecture to another has traps for the lazy too. In a way, when writing Z80 code I was spoiled for registers. I wonder if Klaus would consider adding my little amendment to the repo he has.

EDIT: I in no way think that Klaus is doing a bad job. Far from it, he is doing a great job. I just thought I'd get that out there in case anyone reads my comments otherwise.

[cjs]

JenniferDigital wrote:

I'd imagine porting code from one architecture to another has traps for the lazy too.

I really wish we could get off this whole, "Errors happen because programmers are 'lazy' thing." All humans make errors, and it's not because they're lazy, it's because they're efficient.

If you don't believe me, start by looking at every error you've ever made. I'm going to posit they are all because you are immensely lazy. Would you agree?

[JenniferDigital]

cjs wrote:

JenniferDigital wrote:

I'd imagine porting code from one architecture to another has traps for the lazy too.

I really wish we could get off this whole, "Errors happen because programmers are 'lazy' thing." All humans make errors, and it's not because they're lazy, it's because they're efficient.

If you don't believe me, start by looking at every error you've ever made. I'm going to posit they are all because you are immensely lazy. Would you agree?

Point taken.

[BigEd]

I've a feeling I've read that organisations with a very great need for flaw-free software put about ten times the effort into writing that software. For a factor of ten, for ordinary purposes, I'd be happy with a few bugs - let alone a few bits of inefficiency.

[teamtempest]

https://www.youtube.com/watch?v=GWYhtksrmhE

I suspect NASA would hate my Python code, since I love to use function indirection

[JenniferDigital]

NASA Won't allow you to use recursion, I think they forbid heap usage too. There will definitely be documentation on their requirements.

After all... Who wants to loose a multi-billion pound mission with years of work behind them and jobs on the line over a single bit out of place.