pokeruby icon indicating copy to clipboard operation
pokeruby copied to clipboard

Registers should be defined as unions of bit-struct and u16

Open LIJI32 opened this issue 6 years ago • 5 comments

I'm working on decompiling rock.c and I've noticed that the C code for do_boulder_dust used REG_BG1CNT as a bit-struct instead of an integer.

Specifically, this C statement REG_BG1CNT.priority = 1; (used by do_boulder_dust) translates to:

LDR             R4, =0x400000A
LDRB            R1, [R4]
MOVS            R0, #4
NEGS            R0, R0
ANDS            R0, R1
MOVS            R1, #1
ORRS            R0, R1
STRB            R0, [R4]

While the equivalent yet more arithmetic version REG_BG1CNT = (REG_BG1CNT & ~3) | 1; (used by sub_809EC38 from pokemon_summary_screen.c) translates to:

LDR             R4, =0x400000A
LDRH            R1, [R4]
LDR             R0, =0xFFFC
ANDS            R0, R1
MOVS            R1, #1
ORRS            R0, R1
STRH            R0, [R4]
LDRH            R1, [R4]

To properly decompile that function, the REG_BGXCNT registers must be redefined as unions, so they can be used as both integers and bit structs. Meanwhile, in order to avoid refactoring pretty much everything else in order to decompile this function, I'm using ((struct BGCNT *) &REG_BG1CNT) as a temporary hack.

LIJI32 avatar Jan 13 '18 22:01 LIJI32

Some of the code does use a bitfield, but other parts assign the values directly, and using a bitfield won't match. We already have a vBgCnt struct in types.h for this.

camthesaxman avatar Jan 15 '18 17:01 camthesaxman

Exactly, which is why they should be defined as unions. It's good to know these are already defined in types.h, so I don't need to redefine them. Additionally, do_boulder_dust also uses the DMA3 registers as structs of 3 u16s:

    REG_DMA3.SAD = (u32) &var_c;
    REG_DMA3.DAD = r1;
    REG_DMA3.CNT = 0x85000400;

LIJI32 avatar Jan 15 '18 17:01 LIJI32

Isnt that just a DmaFill32 ? Please use the macros defined in gba/macros.h

RevoSucks avatar Jan 15 '18 17:01 RevoSucks

Turns out it is, thanks!

LIJI32 avatar Jan 15 '18 18:01 LIJI32

Oh, I see what you mean about making it a union. I believe that would match at the cost of being slightly more verbose because of the extra member required. If there's a significant amount of code that accesses it as a bitfield, then the change is probably worth it. If the vast majority of code accesses it as a raw u16, then it might be good to leave it as it is.

camthesaxman avatar Jan 21 '18 14:01 camthesaxman