Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Nintendo switch support #8069

Merged
merged 24 commits into from
Jun 27, 2018
Merged

Nintendo switch support #8069

merged 24 commits into from
Jun 27, 2018

Conversation

jyapayne
Copy link
Contributor

The long-awaited Nintendo Switch support! This allows compilation of Nintendo Switch architecture compatible elf files using the --os:nintendoswitch flag.

Environment vars required: DEVKITPRO which is the path to the root of your devkit pro installation. There are some directories expected to exist in a specific structure for now until I figure out a better way to specify them. They are:

  • DEVKITPRO/portlibs/switch/lib
  • DEVKITPRO/libnx/lib
  • DEVKITPRO/portlibs/switch/include
  • DEVKITPRO/libnx/include

Other environment variables that are not required: SWITCH_LIBS for any extra libraries required by your application (-lLIBNAME or -LLIBPATH), and SWITCH_INCLUDES for any extra include files (-IINCLUDE_PATH)

@dom96
Copy link
Contributor

dom96 commented Jun 19, 2018

Well, now I just have to buy a Nintendo Switch :P

Nice work!

Copy link
Contributor

@dom96 dom96 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd love to have this in the compiler. Not sure what @Araq's thoughts about it are, but this is awesome :)

changelog.md Outdated
@@ -170,4 +170,15 @@
- ``experimental`` is now a pragma / command line switch that can enable specific
language extensions, it is not an all-or-nothing switch anymore.

- Nintendo Switch was added as a new platform target. Simply add --os:nintendoswitch
to your usual ``nim c`` or ``nim cpp`` command. DevkitPro setup must be the same as
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great! But I think a better place for it is in the nimc docs: https://github.com/nim-lang/Nim/blob/devel/doc/nimc.rst

You can link to them from the changelog :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool, sounds good. I'll make that change.

@@ -83,6 +83,31 @@ compiler gcc:
props: {hasSwitchRange, hasComputedGoto, hasCpp, hasGcGuard, hasGnuAsm,
hasAttribute})

# GNU C and C++ Compiler
compiler aarch64NoneElfGCC:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick but I would name this switchGCC. We might have a generic aarch64-none-elf-gcc in the future (I guess?)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@@ -15,6 +15,7 @@ Platforms: """
dragonfly: i386;amd64
haiku: i386;amd64
android: i386;arm;arm64
nintendoswitch: armv8a57
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is only necessary if you are going to be compiling Nim on the Nintendo Switch. I'm guessing that won't happen, will it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, you never know. Linux can run on the switch already :)

@@ -124,7 +124,7 @@ type
disabledSf, writeOnlySf, readOnlySf, v2Sf

TSystemCC* = enum
ccNone, ccGcc, ccLLVM_Gcc, ccCLang, ccLcc, ccBcc, ccDmc, ccWcc, ccVcc,
ccNone, ccGcc, ccaarch64NoneElfGcc, ccLLVM_Gcc, ccCLang, ccLcc, ccBcc, ccDmc, ccWcc, ccVcc,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again I'd probably call this ccSwitch or ccNintendoSwitch.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright, that makes more sense. I wasn't sure what to call it, so ccNintendoSwitch shall be it's name!

@@ -208,7 +213,8 @@ const
(name: "sparc64", intSize: 64, endian: bigEndian, floatSize: 64, bit: 64),
(name: "mips64", intSize: 64, endian: bigEndian, floatSize: 64, bit: 64),
(name: "mips64el", intSize: 64, endian: littleEndian, floatSize: 64, bit: 64),
(name: "riscv64", intSize: 64, endian: littleEndian, floatSize: 64, bit: 64)]
(name: "riscv64", intSize: 64, endian: littleEndian, floatSize: 64, bit: 64),
(name: "armv8a57", intSize: 64, endian: bigEndian, floatSize: 64, bit: 64)]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't arm64 be reused? Is there a specific reason you need bigEndian?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, you're right. Checked the switch's cpu mode and it can be either.

Pthread_attr {.importc: "pthread_attr_t",
header: "<sys/types.h>".} = object
ThreadVarSlot {.importc: "pthread_key_t",
header: "<sys/types.h>".} = distinct cuint
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code is very similar to the code above it, can the above code be reused for this branch?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, you're right. I just wasn't sure about the abi: array[56 div sizeof(clong), clong], but it looks like it's fine.

@jyapayne
Copy link
Contributor Author

@dom96 Cool to see you're on board. It's super cool to have a program running Nim on the switch!

@@ -312,6 +312,8 @@ else:
include ioselects/ioselectors_poll # need to replace it with event ports
elif defined(genode):
include ioselects/ioselectors_select # TODO: use the native VFS layer
elif defined(nintendoswitch):
include ioselects/ioselectors_select
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

doesn't the os have something better than select? kqueue, epoll?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately, not that I can find. If you can find it in the devkitPro libs, I would be up for changing it.

@jyapayne
Copy link
Contributor Author

@Araq, what are your thoughts on this pull request? Any feedback?
@dom96 I've made the requested changes. If there's anything else let me know!

@Araq
Copy link
Member

Araq commented Jun 21, 2018

I like it very much but travis is not happy:

�[0m�[1mlib/pure/times.nim(794, 29) �[0m�[31mError: �[0mundeclared identifier: 'CTime'
�[0mError: call to nim compiler failed
�[31mError: �[0mexecution of an external program failed: './tools/niminst/niminst --var:version=0.18.1 --var:mingw=none csource --main:compiler/nim.nim compiler/installer.ini'

@jyapayne
Copy link
Contributor Author

@Araq, is that due to my code? I didn't mess with the times module. I'll check it out.

@jyapayne
Copy link
Contributor Author

Yep, it's due to my stuff. Looks like I have more stuff to implement!

final, pure.} = object
abi: array[((64+(sizeof(clong) * 8)-1) div (sizeof(clong) * 8)), clong]

proc si_pid*(info: SigInfo): Pid =
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, this is to get around the issue that Switch's header struct for siginfo doesn't contain si_pid, which means waiting for a child process to finish doesn't work. I don't think that's too big of a deal right now, but what are your thoughts on this? Is there some other way to correctly identify where the Signal is coming from?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know, sorry.

@jyapayne
Copy link
Contributor Author

Well I'm okay with the way this is now if you are. I can add a note in the docs about how waiting on subprocesses isn't supported yet and then I think it's done if it's okay with you guys.

@jyapayne
Copy link
Contributor Author

And if you have no comments on my most recent commits of course :)

@GULPF
Copy link
Member

GULPF commented Jun 23, 2018

I don't know anything about this, but if nintendo switch is supported by posix.nim, shouldn't defined(nintendoswitch) imply defined(posix)? Otherwise defined(posix) or defined(nintendoswitch) must be used everywhere, which is a bit annoying.

@jyapayne
Copy link
Contributor Author

@GULPF Yeah, you're right. I should have realized that. Nice find!

@jyapayne
Copy link
Contributor Author

@dom96 looks like the build failed, but I don't think it was me this time. It looks like yarn wasn't able to be found or something? Does this happen sometimes?

@jyapayne
Copy link
Contributor Author

Nevermind, I think it had to do with some old code. I updated from latest devel and ran the tests locally and it all looks good! Hopefully travis agrees :)

@jyapayne
Copy link
Contributor Author

Okay, I think I'm happy with this now. If there are any other outstanding issues, please let me know!

@@ -556,14 +582,20 @@ proc getCompileCFileCmd*(conf: ConfigRef; cfile: Cfile): string =
else:
cfile.obj

# D files are required by nintendo switch libs for
# compilation. They are basically a list of all includes.
let dfile = objfile.replace(".o", ".d").quoteShell()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use changeFileExt instead.

Copy link
Member

@Araq Araq left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

posix_nintendoswitch.nim seems copy&pasted code and I dunno why that is.

# Map files are required by Nintendo Switch compilation. They are a list
# of all function calls in the library and where they come from.
var mapfile = getNimcacheDir(conf) / splitFile(projectFile).name & ".map"
mapfile = quoteShell(mapfile)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Combine these two statements.

doc/nimc.rst Outdated
Simply add --os:nintendoswitch
to your usual ``nim c`` or ``nim cpp`` command. DevkitPro setup must be the same as
what is the default with their new installer
[here for Mac/Linux](https://github.com/devkitPro/pacman/releases) or
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's not how RST links look like.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, copy pasted from MD file. I'll update to be in line with rst.

doc/nimc.rst Outdated
to your usual ``nim c`` or ``nim cpp`` command. DevkitPro setup must be the same as
what is the default with their new installer
[here for Mac/Linux](https://github.com/devkitPro/pacman/releases) or
[here for Windows](https://github.com/devkitPro/installer/releases).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same.

doc/nimc.rst Outdated

This will generate a file called ``switchhomebrew.elf`` which can then be turned into
an nro file with the ``elf2nro`` tool in the DevkitPro release. Examples can be found at
[the nim-libnx github repo](https://github.com/jyapayne/nim-libnx.git).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same.

doc/nimc.rst Outdated
There are a few things that don't work because the DevkitPro libraries don't support them.
They are:

1. Waiting for a subprocess to finish. A subprocess can be started, but right
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't indent this list.

Run nim rst2html doc/nimc.rst and look at the produced HTML.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the tip! I didn't know about this command.

#

proc dlclose(lib: LibHandle) =
raise newException(OSError, "dlclose not implemented on Nintendo Switch!")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be better if these would cause a compile-time error but I can imagine why that's rather hard to do...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a way that you usually do this in Nim?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

proc dlclose(lib: LibHandle) {.error: "not implemented".} but it's a compilerProc so it needs a patch in cgen.nim.

#
#
# Nim's Runtime Library
# (c) Copyright 2018 Andreas Rumpf
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use 'Nim contributors' as the name or your own name.


# To be included from posix.nim!

{.deadCodeElim: on.} # dce option deprecated
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove this line.

aio_offset*: Off ## File offset.
reserved: array[32, uint8]

when hasSpawnH:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this is nintendo specific code, you know whether this hasSpawnH is true or not.

timezone* {.importc: "_timezone", header: "<time.h>".}: clong

# Regenerate using detect.nim!
include posix_linux_amd64_consts
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really? You detected these and put them into posix_linux_amd64_consts?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, I missed that one. I'll update to the generated code from the switch.

@jyapayne
Copy link
Contributor Author

jyapayne commented Jun 27, 2018

posix_nintendoswitch.nim seems copy&pasted code and I dunno why that is.

I did copy & paste from amd64 because the file was largely the same but the fields in each object were slightly different. I thought about doing when(nintendoswitch) statements, but there are so many of them I thought it would be better to group them in one file.

@Araq
Copy link
Member

Araq commented Jun 27, 2018

I thought about doing when(nintendoswitch) statements, but there are so many of them I thought it would be better to group them in one file.

Alright then.

@Araq Araq merged commit 559a761 into nim-lang:devel Jun 27, 2018
@kobi2187
Copy link
Contributor

kobi2187 commented Jul 2, 2018

@jyapayne Your patience is admirable!

@jyapayne
Copy link
Contributor Author

jyapayne commented Jul 2, 2018

@kobi2187 OSS contributions require a lot of patience most of the time :) Thank you for the kind words!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants