[DEBUGGER] Assemble dialog box instruction proper disassembly and absolute addresses support

[Nitch2024]

[DEBUGGER] Assemble dialog box instruction automatically filled with the right disassembly instead of .li and the hex since the hex edition is available in "replace instruction"
[DEBUGGER] Assemble dialog can now deal with absolute addresses computing automatically the offset. For example assembling b ->0x80359370 at the address 0x8035936c will result in the right relative jump. Add -> before any address you want to use as absolute to respect the disassembly syntax

[TryTwo]

@vyuuui Do you want to look at this?

[vyuuui]

The reason I never did this was because disassembler output is often not valid assembly, both for this assembler and just in general. As an example, the disassembly dolphin gives for 0x554a8006 is rlwinm r10, r10, 16, 0, 3 (0000f000), which has junk at the end of the line which is supposed to represent the mask. There's several other common examples, as well as the entirety of the PS instruction set. The "best" course of action would be a rewrite of the disassembler to be less bespoke, and if the meta information included in many instructions is still desired it could be moved to a separate query.

[Nitch2024]

The reason I never did this was because disassembler output is often not valid assembly, both for this assembler and just in general. As an example, the disassembly dolphin gives for 0x554a8006 is rlwinm r10, r10, 16, 0, 3 (0000f000), which has junk at the end of the line which is supposed to represent the mask. There's several other common examples, as well as the entirety of the PS instruction set. The "best" course of action would be a rewrite of the disassembler to be less bespoke, and if the meta information included in many instructions is still desired it could be moved to a separate query.

I did not think it was a big issue since you can delete the added part, and it is more useful than a .li but sure you have a good idea I can look into the disassembler code, add a boolean for verbosity.

[Nitch2024]

@vyuuui I made a pass to extend support as you suggested.

1. Adjusted disassembly decorations when using assemble dialog.
   • Right clicking and assembling "rlwinm r0, r0, 2, 0, 29 (3fffffff)" gets "rlwinm r0, r0, 2, 0, 29" in the dialog box.
   • Right clicking and assembling "ps_add p6, p2+p4" gets "ps_add p6, p2, p4" in the dialog box.
2. Added support for pair single p0, p1, ... registers for assembly of all ps instructions. "ps_add p6, p2, p4" works instead of "ps_add f6, f2, f4" required before
3. Added support for assembly of Graphics Quantization Registers for assembly. "psq_l p2, 0(r3), 0, qr0" can now be assembled for example

[TryTwo]

Wow thanks for doing this. Can we be sure the ps register order is always the same between the readable code and what the assembler expects?

[Nitch2024]

You mean for the ps_ instruction right? If yes, then yes. I tested as much as I could, the way I did it was by right clicking the ps instructions I would see, assemble them back and make sure the output had not changed. When I did not find them in the code, I assembled them, and made sure when disassembling the values were in the same order. Now I might have missed something, so open to any testing you recommend.

[vyuuui]

An OK sniff test would be to take the listing at the start of UnitTests/Common/AssemblerTest.cpp, assemble it, feed it back into the disassembler, then assemble it again and compare that against the first output (setting instruction addr to 0 should be fine). That covers all opcodes, but not all permutations of operands, given that would explode exponentially in size.

[Nitch2024]

An OK sniff test would be to take the listing at the start of UnitTests/Common/AssemblerTest.cpp, assemble it, feed it back into the disassembler, then assemble it again and compare that against the first output (setting instruction addr to 0 should be fine). That covers all opcodes, but not all permutations of operands, given that would explode exponentially in size.

Ok, I will see if I can add a unit test of some kind.

[Nitch2024]

@vyuuui I added a round trip unit test as you recommended. It was a good suggestion and helped find few potential bugs with the disassembler that I tried to fix, so you might want to double check that: ps_sel was missing a register in the disassembly, twi 8, r4, 1000 was disassembled as twgt instead of twgti, the ps_ functions were missing the . variants, and the y branch prediction hint was improperly modified with the displacement sign.

Might need help to review the y sign. The IBM documentation I found (https://fail0verflow.com/media/files/ppc_750cl.pdf) does not mention anything about negative displacements, whereas the freescale doc (https://www.nxp.com/docs/en/application-note/AN2491.pdf) does make a difference. Not sure which one is correct. If freescale, I can revert the y branch prediction and look into fixing the assembler then. With my change assembler and disassembler are now consistent and just use the y bit. If I revert the y change in the disassembler, it will have to be fixed in the assembler.

Let me know what you recommend doing.

[TryTwo]

Probably need to setup auto formatting?
https://github.com/dolphin-emu/dolphin/blob/master/Contributing.md#intro-formatting-issues

[Nitch2024]

Probably need to setup auto formatting? https://github.com/dolphin-emu/dolphin/blob/master/Contributing.md#intro-formatting-issues

Thanks. I use CTRL+K and CTRL+D, but for some reason I don't get all the warnings at once in the lint report. Wondering if there is way to check for it on Windows before I commit my changes.

[Dentomologist]

Probably need to setup auto formatting? https://github.com/dolphin-emu/dolphin/blob/master/Contributing.md#intro-formatting-issues

Thanks. I use CTRL+K and CTRL+D, but for some reason I don't get all the warnings at once in the lint report. Wondering if there is way to check for it on Windows before I commit my changes.

If you copy [Dolphin root]/Tools/lint.sh to [Dolphin root]/.git/hooks/pre-commit (or use a symlink or whatever) the script will be run every time you try to make a commit, and abort the commit if there are issues.

[Nitch2024]

Probably need to setup auto formatting? https://github.com/dolphin-emu/dolphin/blob/master/Contributing.md#intro-formatting-issues

Thanks. I use CTRL+K and CTRL+D, but for some reason I don't get all the warnings at once in the lint report. Wondering if there is way to check for it on Windows before I commit my changes.

If you copy [Dolphin root]/Tools/lint.sh to [Dolphin root]/.git/hooks/pre-commit (or use a symlink or whatever) the script will be run every time you try to make a commit, and abort the commit if there are issues.

Awesome. Thanks, I just did it. Fingers crossed, I don't have a lint issue anymore.

[jordan-woyak]

Can you please clean up the commit history so there aren't merges and lint fixes?

[Nitch2024]

Can you please clean up the commit history so there aren't merges and lint fixes?

Sure. Is there a way to do this within this PR or I should just create another PR?

[jordan-woyak]

Sure. Is there a way to do this within this PR or I should just create another PR?

You can clean up the history locally and then git push --force-with-lease.

[Nitch2024]

@jordan-woyak Thank you. Just did it. Hope I will not have lint issues again lol

[Nitch2024]

Probably need to setup auto formatting? https://github.com/dolphin-emu/dolphin/blob/master/Contributing.md#intro-formatting-issues

Thanks. I use CTRL+K and CTRL+D, but for some reason I don't get all the warnings at once in the lint report. Wondering if there is way to check for it on Windows before I commit my changes.

If you copy [Dolphin root]/Tools/lint.sh to [Dolphin root]/.git/hooks/pre-commit (or use a symlink or whatever) the script will be run every time you try to make a commit, and abort the commit if there are issues.

Unfortunately that does not work on Windows with VS. I get an error message saying the file does not start with #!/bin/sh but even if I change it, it still does not work and I get the same error message

[jordan-woyak]

You don't need to have "Fixing OSX and Lint" in the commit message. I wanted you to clean up the history to specifically not have mention of that. ;)

[Nitch2024]

You don't need to have "Fixing OSX and Lint" in the commit message. I wanted you to clean up the history to specifically not have mention of that. ;)

Removed it.

[Nitch2024]

@Dentomologist New lint issues despite me fixing all the previous ones for code I did not even touch. Is there a way to make the pre-commit work on Windows?

[TryTwo]

It can work, but I struggle with it too.

If it wasn't clear: you copy the contents of lint.sh into pre-commit (or just rename the file). You shouldn't have gotten the error if you did a full copy paste.

Then it might yell at you for having the wrong clang version. I don't know the proper way to fix that, so I just downloaded the 13.0.1 clang-format and replaced the VS studio one.

I also use a plugin called Format On Save, because the VS option to do that has never worked for me.

[Nitch2024]

It can work, but I struggle with it too.

If it wasn't clear: you copy the contents of lint.sh into pre-commit (or just rename the file). You shouldn't have gotten the error if you did a full copy paste.

Then it might yell at you for having the wrong clang version. I don't know the proper way to fix that, so I just downloaded the 13.0.1 clang-format and replaced the VS studio one.

I also use a plugin called Format On Save, because the VS option to do that has never worked for me.

Thanks I had created a pre-commit folder and added the file in there and got a different error about the header. Trying your way instead.

[dolphin-ci]

FifoCI detected that this change impacts graphical rendering. Here are the behavior differences detected by the system:

• last-story-shadows on mtl-osx-m1: diff

_{^{automated-fifoci-reporter}}

[vyuuui]

@vyuuui I added a round trip unit test as you recommended. It was a good suggestion and helped find few potential bugs with the disassembler that I tried to fix, so you might want to double check that: ps_sel was missing a register in the disassembly, twi 8, r4, 1000 was disassembled as twgt instead of twgti, the ps_ functions were missing the . variants, and the y branch prediction hint was improperly modified with the displacement sign.

Might need help to review the y sign. The IBM documentation I found (https://fail0verflow.com/media/files/ppc_750cl.pdf) does not mention anything about negative displacements, whereas the freescale doc (https://www.nxp.com/docs/en/application-note/AN2491.pdf) does make a difference. Not sure which one is correct. If freescale, I can revert the y branch prediction and look into fixing the assembler then. With my change assembler and disassembler are now consistent and just use the y bit. If I revert the y change in the disassembler, it will have to be fixed in the assembler.

Let me know what you recommend doing.

I recall basing the simplified mnemonics of static branch pred. branches on this document under appendix E (along with all other simplified mnemonics). References are correlated with comments starting at here. The implementation of my tables matches the provided document, as well as the official spec which states the base mnemonic implies predict not taken (-), where the LOB of the BO field is reset.

My input probably isn't worth too much here, but the disassembler code is quite a mess, especially considering it's largely sourced from an ancient tool circa the late 2000's, so I wouldn't be surprised if there's some weird bugs with it. Luckily the bugs are nearly zero-impact, since people rarely copy out disassembly from dolphin's debugger for usage other than short-term analysis. This is one place where the lack of accuracy in it bites us, which is why my original preference for rewriting it still stands despite your work here (no offense to you at all). I'd be willing to do that, as I've got my own workable reference implementation of it -- it's just a large change that would be tedious for devs working on this project to review especially considering it's so low impact.

[Nitch2024]

Thanks for the document. Page 704 actually brings new information and says we need to use the at bit too.

"+ Predict branch to be taken (at=0b11)

• Predict branch not to be taken (at=0b10)

Assemblers should use 0b00 as the default value for the “at”
bits, indicating that software has offered no prediction."

I might be misreading it since English is not my main language, but right now the code has either + or -, when it seems there should be a neutral state without any prediction maybe? Again sorry if I misread it.

I support your rewrite and losing my changes in the future, but initially my goal was just to improve something important right now which is the assembledialog to have proper disassembly and enable absolute branches which is something is use many times a day and is pretty good quality of life minor improvement.

Do you think it is acceptable to let those changes in until you do the full rewrite? I can also revert everything and just go back to the initial changes, but I still think all your suggestions made the overall changes more useful in the interim. Just let me know what your prefer.

[vyuuui]

I'm fine with this in the interim, as long as the disassembly is consistent with the assembler. You make a good point that it seems the document I provided gives a bit more specificity on BO encodings for branch prediction that allows for disabling hints, which would imply the encoding for bCC+, bCC- and bCC are all unique from eachother.

It is probably better to disregard this and just trust what other assemblers emit (e.g. from devKitPPC), which seems to match what the assembler in dolphin currently does:

y=0 => Not Taken
y=1 => Taken
bCC 0 == bCC- 0
bCC+ 0 != bCC- 0
bCC+ 0 != bCC 0

[Nitch2024]

I'm fine with this in the interim, as long as the disassembly is consistent with the assembler.

Thanks for your flexibility.

It is probably better to disregard this and just trust what other assemblers emit (e.g. from devKitPPC), which seems to match what the assembler in dolphin currently does:

Yes totally agree, so with the changes in the commit I think it is achieved but I had to change the logic on the y hint to ignore the displacement sign.

Here are the possible combinations:
assemble bgt -0x4 => 4181fffc in memory, y hint not set => shows in disassembler as bgt- -0x4
assemble bgt 0x4 => 41810004 in memory, y hint not set => shows in disassembler as bgt- 0x4
assemble bgt+ -0x4 => 41a1fffc, y hint set => shows in disassembler as bgt+ -0x4
assemble btg+ 0x4 => 41a10004, y hint set => shows in disassembler as bgt+ 0x4
assemble bgt- -0x4 => 4181fffc in memory, y hint not set => shows in disassembler as bgt- -0x4
assemble bgt- 0x4 => 41810004 in memory, y hint not set => shows in disassembler as bgt- 0x4

[TryTwo]

Is this ready to merge?

[Nitch2024]

I think so. @vyuuui do you need me to make more changes?

[vyuuui]

Nothing I can think of, no.