relative addressing errors

[binary1230]

First, apologies, this is an incomplete bug report. Working on a pull request, but, figured I'd put what I have here in case any 65816 gurus knew how to fix this quickly. I have some good repro cases though.

Bugs: Instructions that use "relative addressing", like BRL [16bit] and BRA [8bit] can sometimes throw errors incorrectly for valid assembly. Here I'll use BRL as the example, but I think all this happens with BRA too.

This all seems to revolve around issues converting back and forth between 4 byte unsigned numbers and 2 byte signed numbers.

Bug 1:

bug1.asm

ORG $C000DB
BRL label1          ; errors, but should generate 'BRL $B193'

ORG $C0B271
label1: BRK         ; doesn't matter, can be anything 

asar.exe bug1.asm

Expected result: Output ROM is: 82 93 B1 which is BRL $B193

Actual result: Error message:

Errors were detected while assembling the patch. Assembling aborted. Your ROM has not been modified.
bug1.asm:3: error: (E5037): Relative branch out of bounds. (Distance is 45459). [BRL label1]

Analysis: Funny story: Asar actually does the right thing and outputs the correct bytes, but, afterwards a different bounds check fails. that check is located in https://github.com/RPGHacker/asar/blob/master/src/asar/arch-65816.cpp#L55 , in macro as_rel2().

Here's that code with annotation:

int pos = (int)num - ((snespos&0xFFFFFF)+3); $0000B193 = $00C0B271 - ($00C000DB + 3)

this section writes the bytes to the ROM:

write1((unsigned int)byte);  // $82 = BRL, this is correct 
write2((unsigned int)pos);   // $93B1, this is correct 

"BRL $B193" is interpreted the same as: "BRL -20,077"

so far so good. but now here's the error: going into this, pass=2, foundlabel=true

if (pass==2 && foundlabel && (pos<-32768 || pos>32767))
    [error msg that prints 'pos']

specifically this check is the problem:

(pos<-32768 || pos>32767)

At first glance, the intention seems right. The range of a signed 2-byte word which is 0x0000 thru 0xFFFF or -32768 thru 32767), so checking for it makes sense.

However, it causes this to incorrectly print:

bug1.asm:3: error: (E5037): Relative branch out of bounds. (Distance is 45459). [BRL label1]

If our pos variable really was a signed word (2 bytes) this would be fine because: $B193 => interpreted as signed word =>-20,077, and that's in range of the bounds check.

-20,077 is the correct value we're looking for.

The issue is: pos here is actually a 4 byte unsigned int. In hex, its value is: $0000B193

That contains the right bytes (B193), but it's not quite good enough. Interpreting this 4 byte unsigned int as a 4 byte signed int gives us an incorrect result of: 45,459

We're trying to get it to output -20,077.

The hex representation of that is: $FFFFB193 as signed int = -20,077

Takeaways:

1. Using 4 bytes to hold the word that contains the offset is fine for writing the 2 bytes manually with write2()
2. The check can't look at the raw 4 bytes directly without either masking it with 0xFFFF, or, by converting the original value into it's signed form of $FFFFB193.

That leads to the question of: what's the point of the check? If it's to validate the parsed input to make sure the user hasn't typed in a value > $FFFF, then, see below for the next bug:

Bug 2. If you pass in too large a value for BRL or BRA, asar will fail to throw an error but still write out the lower two bytes of the number.

bug2.asm

hirom
BRL $01B193

EXPECTED: fatal error thrown saying $01B193 is too large, max is $FFFF (2 bytes) ACTUAL: no error thrown, and it outputs (wait.. twice in the ROM? seems even weirder) the following bytes: 82 93 B1 BRL $B193

There's a check (same as the above one) that looks like it might catch this value being out of range, but, it's not called because we're not using a label here.

going into this, pass=2, foundlabel=false

if (pass==2 && foundlabel && (pos<-32768 || pos>32767))
    [error msg that prints 'pos']

we skip the: (pos<-32768 || pos>32767) check, and continue with assembly, incorrectly not throwing an error.

So, we need to add a check that if pos > 0xFFFF then it throws an error.

---

More stuff to check, any bugfixes should add these as a test suite and need to make sure these work. (Disclaimer: from my reading of the CPU manual, I'm not super-familiar with 65816, so double check me)

expected (I think?) should throw an error that you can't BRL into a label in a different bank. even though the destination ROM address is less than 32k bytes away.

ORG $C9FF00
BRL label_in_another_bank

ORG $CA0000 ; should fail because branch can't cross bank. i.e. it can only modify PC, not DB
label_in_another_bank: BRK

That's only 256 bytes away in ROM, so it's in range of a 2 byte offset, BUT, [I think?] BRL is only for intra-bank transfers.

could add a check like:

    if (pass == 2 && foundlabel && ((snespos & 0xFF000) != (num & 0xFF000)))
                asar_throw_error(pass, error_type_block, error_id_bank_border_crossed, string("Can't branch to label in a different bank#."));

---

Other stuff:

1. This is really my first time looking at asar, anything we need to keep in mind about freespace? Does it use 0xFF000000 addresses or something? Do we need to deal with that in these checks?
2. Need to check all this for scenarios where we're branching backwards to an earlier position in the bank (i.e. branching from address $C000F0 back to $C00000)

---

Anyway, I thought this might be easy but it turned into a rabbit hole :)

I was planning on writing some tests to cover all the cases. It seems like if this gets more complex, we'll want to extract macros as_rel2() and as_rel1() into functions. The only difference is as_rel1() operates on 1 byte like this:

https://wiki.nesdev.com/w/images/7/76/Programmanual.pdf

The Operand byte, a two’s complement signed value, is sign-extended to 16 bits, then
added to the Program Counter (its value is the address of the opcode following this one)

So, probably they could both call a new function like relative_address(offset, num_bytes), and it only needs to change the range check and # of bytes written.

Phew. OK, lemme know what you think. Open to suggestions.

[RPGHacker]

Not too experienced with the official specs myself. I think the first bug is a consequence of treating BRL just like BRA and limiting the range to its possible signed values. I think this makes sense for BRA (pretty sure branches that are outside the range of -128 to 127 are invalid), but I suppose the correct behavior for BRL instead would be to not care about signed value overflows and just "wrap around", as long as the destination address stays within the same bank? I don't know, as I said, not familiar with the offical specs.

As for the second bug, probably a consequence of Asar implicitly casting numbers down to whatever size is necessary to fit the command being used. Fixing that one might be tricky, and technically, I'm not sure if we can consider that a bug, since it's the intended behavior. I suppose we might have to treat BRL differentl from other commands. Then again, I'm not sure how important that bug would be to fix, anyways, because while directly using numbers with branch commands is technically valid, I'm not sure if that's an actual use case for anything. Normally you would use labels with branch commands. Though I suppose if the bug also occurs while using labels, then it's worth investigating further.

[binary1230]

Yeah I think you are correct about the correct behavior being it should overflow and wrap around the bank. The actual 2 bytes written to the file are correct and will do that. it's only the error check that is messing up the conversion back to signed short and falsely flagging it as an error when it was actually ok.

Bug 1 is happening for me with labels on a disassembly project I'm working on.

bug 2 is more hypothetical and probably comes into play only if someone is typing in addresses by hand instead of using labels. Like you said that seems pretty unlikely. I actually only mentioned it here because when I tried to implement a fix for the first bug it occurred to me that it might introduce the second bug.

On Tue, Sep 8, 2020, 3:49 AM RPG Hacker notifications@github.com wrote:

Not too experienced with the official specs myself. I think the first bug is a consequence of treating BRL just like BRA and limiting the range to its possible signed values. I think this makes sense for BRA (pretty sure branches that are outside the range of -128 to 127 are invalid), but I suppose the correct behavior for BRL instead would be to not care about signed value overflows and just "wrap around", as long as the destination address stays within the same bank? I don't know, as I said, not familiar with the offical specs.

As for the second bug, probably a consequence of Asar implicitly casting numbers down to whatever size is necessary to fit the command being used. Fixing that one might be tricky, and technically, I'm not sure if we can consider that a bug, since it's the intended behavior. I suppose we might have to treat BRL differentl from other commands. Then again, I'm not sure how important that bug would be to fix, anyways, because while directly using numbers with branch commands is technically valid, I'm not sure if that's an actual use case for anything. Normally you would use labels with branch commands. Though I suppose if the bug also occurs while using labels, then it's worth investigating further.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/RPGHacker/asar/issues/170#issuecomment-688687816, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABJJRSFZ3XT6WDMDLHT5WCLSEXO2HANCNFSM4Q7L4YPQ .

[binary1230]

Got around to checking into this more. I extracted the relative addressing function into a separate project and built a few unit test cases, played around with it til I got my test cases all working.

https://gist.github.com/binary1230/894b4a21758520cfa55a5b88b286a198#file-relative-branching-65816-asar-unit-tests-cpp-L48

Next move I'll integrate that as a PR if it looks like it's in the right ballpark, add some more tests on your asm testing scaffolding, and probably study it in a SNES debugger to make sure the output ROM is doing the right thing. I -THINK- it's doing the right thing, but I'm not an expert.

My two questions are:

1. anything I need to be paying attention to with "Freespace" and addresses like 'pos' and 'snespos' here?
2. do the first two passes have to get output bytes written (via stuff like write1 and write2?) before errors are thrown? if not, I can simplify my code a bit and get rid of a bunch of error flags.
3. is there any unit testing built into asar with something like xunit or google tests or something like that? if not, any interest? I found it supremely helpful to coalesce the various requirements together.

[RPGHacker]

1. I honestly don't know, I have done little to no work on that particular code section. Maybe @Alcaro or @randomdude999 have a better understanding?
2. I'm not sure about that without looking it up right now. It's possible that Asar doesn't write out data until the third pass, but I really don't know. Even if data is written out earlier, it's possible that you could get away with just writing out dummy data.
3. We don't use any external unit testing framework, but have our own simple test application. Check the repository's description/readme for instructions on how to operate it.

[Alcaro]

1) You should use &0xFFFF0000 in err_banks_differ, top byte is a freespace ID. I don't know what happens if the question 'are labels A and B in same bank?' gets different answers between pass 2 and 3, and I'd recommend not finding out. 2) Doesn't matter. Most opcodes already throw before writing; for example, LDA LabelThatDoesntExist,x succeeds on first pass, but errors before writing on second. 3) what rpg said

[randomdude999]

the PR's logic was implemented in asar_2_beta, so i'm considering this done