search

RPGHacker / asar

(Now) official repository of the SNES assembler Asar, originally created by Alcaro
Other
204 stars 44 forks source link

Move Block compiles incorrectly #151

Open spannerisms opened 5 years ago

spannerisms commented 5 years ago

The conventional syntax for MVN and MVP is MVN SRC, DEST, but the 65816 expects those bytes to be $54 DEST SRC. Asar currently has the compilation backwards, writing $54 SRC DEST, which in turn makes the correct compilation require backwards syntax. This issue affects both instructions.

randomdude999 commented 5 years ago

We can't change the syntax now without breaking existing xkas and Asar patches. But I'd be open to suggestions for syntax where you explicitly write out which byte is the source and which is dest because I can never remember the order they're written in either.

Alcaro commented 5 years ago

Two-operand assembly instructions are a mess. Each assembly language chooses differently, and they expect you to learn which one is which. x86 is the worst, different assemblers use different answers.

But yes, it sucks. If you want an alternative, I propose

mvn< dst,src mvp> src,dst

unless that conflicts with macro arguments. (Worst case, support latter only, and let former error out so arguments can be swapped.)

The same syntax should be supported for SPC700 mov too.

spannerisms commented 5 years ago

I think following the decades old convention is more important in this case. Can it not be conditional based on version, such that older code uses the backwards syntax? If not, then user-decided style for move block could make its way into a command like blockmove sourcefirst, with the default being destfirst. Honestly, such a command would satisfy me.

randomdude999 commented 5 years ago

now that i think about it, an arrow operator may be neat (MVN $12->$34). Pretty sure it wouldn't conflict with macros either since argument names can't have hyphens last time i checked. Also I prefer something where you specify the order in each invocation instead of globally, I think it would make it easier to transition (or make any transitioning of existing code unnecessary in the first place).

Alcaro commented 5 years ago

I haven't heard of this decades old convention until now, so I must disagree about its importance. IIRC, I chose this way to match Romi's xkas v06.01; I don't know why he did it that way.

I'd prefer not adding more compilation modes. If different parts of the patch want different settings (for example if different parts are written by different people), you've got a mess. Especially if there are macros nearby.

I have no opinion on mvn> $12,$34 vs mvn $12->$34, I'm fine with both. mvn $12<-$34 would be ambiguous if operator< is implemented (didn't check), but even if it is, such code is so implausible I'm fine with breaking such patches.

e: actually, $12->$34 is slightly better.

spannerisms commented 5 years ago

I haven't heard of this decades old convention until now, so I must disagree about its importance.

It's only in every documentation of the chip you can find anywhere, including the manuals published by WDC themselves. The importance comes from the fact that anyone who learned the proper syntax will be confused by the backwards syntax. It makes debugging difficult, especially when you're stepping through instructions and see the instruction listed as MVN SRC, DEST, and then look back at your code and see MVN DEST, SRC. That's not a good thing. Tools for reading or writing code should all agree with each other on syntax. If there's an established convention, especially one recommended by the chip's designer, it should be followed.

Alcaro commented 5 years ago

And anyone who learned the backwards syntax will be confused by the proper syntax. If we swap the operands, we'd have to not only update all patches, but also retrain everyone who wrote said patches.

Consistency with debuggers is a valid argument, but for such a rarely-encountered opcode, it's not worth breaking backwards compatibility. Especially if old code miscompiles, rather than throwing an error.

Let's add the arrow syntax and encourage everyone to switch, then consider whether to deprecate the comma syntax once we see how well that one's received.

VitorVilela7 commented 5 years ago

An alternative is using MVN $srcdest i.e. MVN $057E which compiles as $54 $7E $05.

Honestly, I didn't know it was backwards. All debuggers seemed to follow that order, with the exception of bsnes-plus, apparently.

But changing it would lead to severe incompatibilities issues since there is a lot of legacy code that uses dest,src on the block move commands. A suggestion for an eventual asar 2.0 is adding a header that applies an eventual new syntax if it's needed on the future.