beardypig / ghidra-emotionengine

Ghidra Processor for the Play Station 2's Emotion Engine MIPS based CPU
Apache License 2.0
198 stars 35 forks source link

bug in master branch #63

Closed SpaceDog61 closed 3 years ago

SpaceDog61 commented 3 years ago

Hello everyone. Found a strange bug in the master branch. Global variables are defined incorrectly. Perhaps the addiu instruction is being handled incorrectly. In version 1.6 everything is ok. Ghidra 9.2.2, 9.2.3, 9.2.4. It seems to me that this bug appeared after commits Tupelov. Ghidra9 2 4 _master Ghidra9 2 4 _master

Ghidra9 2 4 _1.6 Ghidra9 2 4 _1 6

astrelsky commented 3 years ago

Just a heads up, I have seen this. Will be busy the next few days but it will be looked into once time allows.

JayFoxRox commented 3 years ago

I'm convinced that #55 is the culprit here, it affects those touched instructions. Look at this loop for example (I removed some irrelevant code):

01 00 04 24        li         a0,0x1
ff ff 05 24        li         a1,0xffff
loop:
ff ff 84 24        addiu      a0,a0,0xffff
...
fb ff 85 14        bne        a0,a1,loop
...
int iVar5;
iVar5 = 1;
do {
  iVar5 = iVar5 + 0xffff;
} while (iVar5 != 0xffff);

However, this should have been:

int iVar5;
iVar5 = 1;
do {
  iVar5 = iVar5 - 1;
} while (iVar5 != -1);

or:

01 00 04 24        li         a0,0x1
ff ff 05 24        li         a1,0xffffffff // ~signExtend(0xffff)
loop:
ff ff 84 24        addiu      a0,a0,0xffffffff // ~signExtend(0xffff)
...
fb ff 85 14        bne        a0,a1,loop
...

Such patterns can be found everywhere; so this breaks almost any disassembly / decompilation.

Note that the original mips.sinc came from Ghidra, which also treats this as signed: https://github.com/NationalSecurityAgency/ghidra/blob/b6ba209ed796343880327bc3337355c303b760cd/Ghidra/Processors/MIPS/data/languages/mips32Instructions.sinc#L465-L469


@Tupelov Looking at #53 I'm not sure how you concluded those values should be any different. I think this is an issue which comes down to how Ghidra displays number, rather than how they are decoded. If you want unsigned numbers, this should be the expected result:

  addiu v0,v0,0xfffffd80
  addiu v1,v1,0xffffe330

Wether you want to see signed or unsigned numbers depends on a case-by-case basis of course. Ghidra allows you to change it: https://reverseengineering.stackexchange.com/questions/21253/how-do-i-make-a-hex-literal-a-decimal-literal-in-ghidra

If you are convinced that these should be different numbers than 0xfffffd80 and 0xffffe330, I suggest to check if there might be a relocation table or something which is fixing these offsets.


Please revert #55 (as done in #69), I don't think any of the changes were correct. I suggest to stick to what Ghidra does for the standard MIPS stuff (with minor fixes for the PS2): https://github.com/NationalSecurityAgency/ghidra/blob/b6ba209ed796343880327bc3337355c303b760cd/Ghidra/Processors/MIPS/data/languages/mips32Instructions.sinc#L24-L30