Decompollaborate / spimdisasm

MIPS disassembler
https://pypi.org/project/spimdisasm/
MIT License
42 stars 13 forks source link

Generate branch and jump table labels based on function name instead of address #142

Closed cadmic closed 10 months ago

cadmic commented 10 months ago

This is to help match functions even if they're not in the same location in memory. Example:

glabel Player_ActionToModelGroup # 5
  addiu       $sp, $sp, -0x20
  sw          $ra, 0x14($sp)
  lui         $v1, 0x800F
  addu        $v1, $v1, $a1
  lbu         $v1, (0x800F7D64 & 0xFFFF)($v1)
  addiu       $at, $zero, 0x2
  bnel        $v1, $at, .LPlayer_ActionToModelGroup_2
   move       $v0, $v1
  jal         Player_IsChildWithHylianShield
   sw         $v1, 0x1C($sp)
  beqz        $v0, .LPlayer_ActionToModelGroup_1
   lw         $v1, 0x1C($sp)
  b           .LPlayer_ActionToModelGroup_2
   addiu      $v0, $zero, 0x1
.LPlayer_ActionToModelGroup_1:
  move        $v0, $v1
.LPlayer_ActionToModelGroup_2:
  lw          $ra, 0x14($sp)
  addiu       $sp, $sp, 0x20
  jr          $ra
   nop

I'm not sure how to test this more thoroughly, perhaps it does weird things with strange code or if there's already a label where the branch label should go

cadmic commented 10 months ago

Sounds good, what should the flag/setting be called?

AngheloAlf commented 10 months ago

Good question. Maybe something like SEQUENTIAL_BRANCH_LABELS?

AngheloAlf commented 10 months ago

Actually, I think it would be better to move the logic of generating a name for the labels to getDefaultName (on spimdisasm/common/ContextSymbols.py), so we keep that logic centralized on one place. You can add any extra members to the ContextSymbol class if you need them.

cadmic commented 10 months ago

Okay, I gave that a go. I've just realized though that I probably want this for jump table labels too (i.e. number all branch targets sequentially regardless of whether they're for direct or indirect branches). I'm not sure how to know the containing function or the index when jump tables labels are created. What's a good way to implement this?

AngheloAlf commented 10 months ago

That's actually a good question. Jumptable analysis is done kinda independently of the corresponding function. jumptable labels are generated on spimdisasm/mips/sections/MipsSectionRodata.py, by the addJumpTableLabel calls.

You could try to carry over the function via the jumptable symbol, so when the jumptable generate the labels it can pass the function information over too. To do this you can attach the Function symbol to the jumptable when it is being generated by the addJumpTable call on spimdisasm/mips/symbols/MipsSymbolFunction.py, and then do something similar on SectionRodata. lmk if it wasn't clear enough

cadmic commented 10 months ago

Thanks! I think I got it working, let me know what you think. Example (note that .LActor_SetTextWithPrefix_8 is both a direct branch target and a jump table target):

glabel Actor_SetTextWithPrefix
  sw          $a2, 0x8($sp)
  sll         $a2, $a2, 16
  sra         $a2, $a2, 16
  lhu         $t6, 0xA4($a0)
  sltiu       $at, $t6, 0x71
  beqz        $at, .LActor_SetTextWithPrefix_8
   sll        $t6, $t6, 2
  lui         $at, %hi(jtbl_80102F28_anon)
  addu        $at, $at, $t6
  lw          $t6, %lo(jtbl_80102F28_anon)($at)
  jr          $t6
   nop
jlabel .LActor_SetTextWithPrefix_1
  b           .LActor_SetTextWithPrefix_9
   addiu      $v0, $zero, 0x1000
jlabel .LActor_SetTextWithPrefix_2
  b           .LActor_SetTextWithPrefix_9
   addiu      $v0, $zero, 0x2000
jlabel .LActor_SetTextWithPrefix_3
  b           .LActor_SetTextWithPrefix_9
   addiu      $v0, $zero, 0x3000
jlabel .LActor_SetTextWithPrefix_4
  b           .LActor_SetTextWithPrefix_9
   addiu      $v0, $zero, 0x4000
jlabel .LActor_SetTextWithPrefix_5
  b           .LActor_SetTextWithPrefix_9
   addiu      $v0, $zero, 0x5000
jlabel .LActor_SetTextWithPrefix_6
  b           .LActor_SetTextWithPrefix_9
   addiu      $v0, $zero, 0x6000
jlabel .LActor_SetTextWithPrefix_7
  b           .LActor_SetTextWithPrefix_9
   addiu      $v0, $zero, 0x7000
jlabel .LActor_SetTextWithPrefix_8
  move        $v0, $zero
.LActor_SetTextWithPrefix_9:
  or          $t7, $v0, $a2
  sh          $t7, 0x10E($a1)
  jr          $ra
   nop
AngheloAlf commented 10 months ago

That looks so much nicer!

Btw, what is that _anon suffix on jtbl_80102F28_anon?

What do you think about applying a similar logic for naming jumptables themselves too? You don't need to do it on this PR tho, mainly to avoid the scope creep

cadmic commented 10 months ago

Oh, _anon is because I used --custom-suffix _anon in zelda64compare (it was originally `--custom-suffix $(VERSION)`)

What do you think about applying a similar logic for naming jumptables themselves too?

Oh good idea. I'll leave it for a different PR though

cadmic commented 10 months ago

Never mind, it was pretty easy. Though I'm not sure what to call the flags now, or if they should use the same flag

AngheloAlf commented 10 months ago

Cool! I think a single option for both is better, I don't see an use case where you would enable one and not the other. Maybe something like SEQUENTIAL_LABEL_NAMES?

cadmic commented 10 months ago

Okay, changed to 1 flag