Initial SuperH SH-1/SH-2 Processor Support

[VGKintsugi]

Following up from issue 608, I am submitting a PR for my Sega Saturn (SuperH SH-1/SH-2) processor module.

I struggled with issues related to sign extensions, flags, conditionals, and branches. I also struggled writing SLEIGH code for some of the longer instructions such as addv, div1, mac.l, and mac.w. Thanks in advance.

[loudinthecloud]

Any plans to merge this soon?
We have added support for SH-2A and would like to add it as well.

[emteere]

Yes, Sorry for the delay in review. Very much appreciate the contribution!

The plan is to review for best practices and then accept after review. We're going to push to get this and others accepted. Do you have a sample binary that could be used for review?

[VGKintsugi]

No worries on the delay.

I have been using this site for homebrew Sega Saturn games to test: http://archives.dcemulation.org/vieille.merde.free.fr/vieille.merde.free.fr/index.html. In particular if you download one of the attachments labelled "binary executable", use SH-2 as the processor, and load to address 0x06004000 in Ghidra it should work.

I can also look for some ELF SH-1/SH-2 Linux binaries.

[mumbel]

Don't know the state of it, guessing @VGKintsugi is familiar but you could buiding gcc from https://github.com/SaturnSDK/Saturn-SDK-GCC-SH2

[emteere]

Would be great to have a C-compiler for SH1/2 we can use the just pushed emulation test framework to test your processor module.

[VGKintsugi]

Do you have notes on how to use the emulation test framework? I glanced at the REcon slides but I'm not sure how to launch it.

In terms of compilers:

1. As mumbel mentioned, https://github.com/SaturnSDK/Saturn-SDK-GCC-SH2 works great. To build:

• checkout the source code
• set the environment variables as specified in the README:
  export SRCDIR=$(pwd)/source
  export BUILDDIR=$(pwd)/build
  export TARGETMACH=sh-elf
  export BUILDMACH=x86_64-pc-linux-gnu
  export HOSTMACH=x86_64-pc-linux-gnu
  export INSTALLDIR=$(pwd)/toolchain
  export SYSROOTDIR=$INSTALLDIR/sysroot
  export ROOTDIR=$(pwd)
  export DOWNLOADDIR=$(pwd)/download
  export PROGRAM_PREFIX=saturn-sh2-
• run "./build-elf.sh"
• once complete I used "saturn-sh2-elf-gcc" to compile a hello world C file. I verified that Ghidra recognizes the file as an ELF, and that main() decompiled my hello world correctly.

2. Renesas has a compiler for the entire SuperH family if you prefer: https://www.renesas.com/sg/en/products/software-tools/tools/compiler-assembler/compiler-package-for-superh-family.html. I didn't try that out yet.

[VGKintsugi]

Thank you for the feedback. I will work on these changes and have something for next week.

[GhidorahRex]

Sounds great. Overall, the design was really well-thought out. Great work!

[VGKintsugi]

Can you take a look at this instruction:

:mov.l  @(disp,pc),rn_08_11  is pc & opcode_12_15=0b1101 & rn_08_11 & disp_00_07 [disp = (disp_00_07 << 2) + 4; ]
{
    local temp:4 = inst_start;
    rn_08_11 = *:4 ((temp & 0xFFFFFFFC) + disp);
}

The psuedocode is:

void MOVLI (int d, int n)
{
  unsigned int disp = (0x000000FF & d);
  R[n] = Read_32 ((PC & 0xFFFFFFFC) + 4 + (disp << 2));
  PC += 2;
}

Specifically when PC is 2-byte aligned but not 4-byte aligned I think there might be some issue. I've tried matching it up with a Sega Saturn emulator and it appears my processor module might be reading two bytes ahead of where it should. It could be an issue with the video game emulator or a bug in my code.

[mumbel]

Shouldn't have to worry about the 2 or 4 byte align since you mask PC with fffffffc, right? It's always 4 byte aligned after that. I do question why you have a + 4 in there. The manuals I checked do not have that. Edit: your ref html does, that's weird, hopefully it's not some non-spec instruction

2nd edit. Oh you have a branch delay issue maybe. The inst_start is incorrect if it were after a bra there is an issue with using inst_start but unrelated to delayslot

Oh PC is at 2 instructions after the mov, nmd +4 makes sense

[GhidorahRex]

@mumbel is correct about the byte-alignment issue.It shouldn't be an issue for branch delay slots since it's considered an illegal instruction in the branch delay.

[mumbel]

I admit I didn't read closely and misinterpreted the example. It made sense with (0x100a & 0xfffffffc) + (4<<2). But looks like it's (0x1012=>NEXT + 2 & 0xfffffffc) + 4?

[GhidorahRex]

I see what you mean now. You are very likely correct there.

[VGKintsugi]

I have pushed most of the changes requested from the code review. I have not:

1. I could not get the n==m checks to work. I tried something like "mov.w ... & opcode_04_07 & opcode_08_11 & opcode_04_07 == opcode_8_11" but the SLEIGH compiler was not happy. I implemented a workaround similar to the GOTO elimination.
2. I haven't had a chance to look into implementing the immediate subconstructor.

[mumbel]

I was just about to comment when i saw an email come in. A quick guess at the n==m thing, the syntax is only a single =.

:mov is the only insn w/ a space after the comma in operands. great review i know :D ... nothing looks out of place in your new revision btw, looks a lot cleaner, nice cleanup

[VGKintsugi]

Thanks @mumbel, I fixed the whitespace. I still couldn't get the n==m to work. I also gave you merge access to my fork in case you want to push @loudinthecloud's SH2A PR.

Also, do you know how to use the emulation test framework? I would like to try it.

[mumbel]

@GhidorahRex I have been reviewing some changes that @loudinthecloud has made to add 2A support, this is basically a few additional instructions and FPU support. @loudinthecloud 's PR is very clean in my POV and I'll will be working to merge that PR hopefully tomorrow night, so should show up here then. Hopefully this is not too burdensome, but seems ideal to get 2A in addition to 1 and 2.

[GhidorahRex]

I have pushed most of the changes requested from the code review. I have not:

1. I could not get the n==m checks to work. I tried something like "mov.w ... & opcode_04_07 & opcode_08_11 & opcode_04_07 == opcode_8_11" but the SLEIGH compiler was not happy. I implemented a workaround similar to the GOTO elimination.

It's a single equals, not a double equals. I was able to get it to compile by using:
rn_08_11 & rm_04_07 & (opcode_04_07 = opcode_08_11)
for the one where they are equal and just using
rn_08_11 & rm_04_07 for the one where they are different.

[mumbel]

@VGKintsugi @GhidorahRex I merged in the SH-2A (from @loudinthecloud ) and fixed these recent changes in rotcl and rtocr

I did not touch the n=m code though

[VGKintsugi]

Thanks again @mumbel. I implemented the n=m checks in the last commit.

[mumbel]

@GhidorahRex still thinking if it can get away with fewer goto's, but hopefully more readable now

[GhidorahRex]

It looks pretty good at this point. I'll submit it for internal review and if there are any additional suggested changes I'll add them here.

[VGKintsugi]

I apparently swapped the bsrf and the jsr instructions during the code review. It's fixed now.

[ryanmkurtz]

@VGKintsugi, we noticed that you are submitting this under the MIT license, rather than following the guidance defined in our Contributors Guide:

Consistent with Section D.6. of the GitHub Terms of Service as of 2019, and Section 5. of the Apache License, Version 2.0, the project maintainer for this project accepts contributions using the inbound=outbound model. When you submit a pull request to this repository (inbound), you are agreeing to license your contribution under the same terms as specified in LICENSE (outbound).

Was it your desire to license this to all Ghidra users under MIT instead? We’re happy to include it under Apache 2.0, and it’s a lot easier for us to proceed in this way. This wouldn’t preclude you from licensing the files under MIT in other venues. If you insist on using the MIT license, we can accommodate it, but it complicates things.

Just let us know so we can proceed with your contribution...thanks!

[VGKintsugi]

I am OK with including the code under Apache 2.0 to keep things simple. What's the easiest way to do that? Commit a different License file with this PR? Or is this message itself sufficient?

[loudinthecloud]

Hi @ryanmkurtz, we at Toka are also submitting our work in this pull request. I suggest to remove the license alltogether, and perhaps add a CREDITS file instead. Is that ok by you @VGKintsugi?

[VGKintsugi]

@loudinthecloud that works for me.

[VGKintsugi]

I pushed the license change and added a CREDITS file. Let me know if you there any concerns.

[ryanmkurtz]

I am OK with including the code under Apache 2.0 to keep things simple. What's the easiest way to do that? Commit a different License file with this PR? Or is this message itself sufficient?

@VGKintsugi, yes, your message itself is sufficient. We don't support a custom LICENSE file in the repo. We have an internal certification process that marks the license of each file, which then gets stored in the java file header or certification.manifest file for non-java/c files. When we do a build, the appropriate license file will get put into the module's directory. If you wish to declare copyright information, the only thing that will work right now is putting in your own comments in the appropriate source files. Currently, a custom CREDITS file will also get discarded by the build process. We'll have to have a discussion internally about that type of file. We have been giving credit so far just from the Git commit history.

[loudinthecloud]

Hi, do you have any update regarding this pull request? We are not insisting on adding a CREDITS file, but it would be nice.

[brainstorm]

@loudinthecloud Wouldn't it be enough to put your credits/names on the headers of each file you have contributed to? This wouldn't break/interfere with @ryanmkurtz building conventions/releng and you'll retain that authorship (as well as the commits themselves)?

I'm just eager to get this shipped soon along with #37 :)

[loudinthecloud]

We're fine with that too @brainstorm :)

[ryanmkurtz]

Sorry for the delay in responding. We just pushed this to master and it will be included in the 9.1 release! It's showing as closed instead of merged because we made a commit before incorporating your last commit that removed the LICENSE file. I guess that was enough for GitHub to not consider the branch associated with this pull request anymore. Please take a look and make sure everything looks right on your end with it. Thanks!