Use playdate realloc for memory management

[Nycto]

Fixes #59

This change adds a deep integration within Nim for using the playdate memory allocator. This change fixes a host of memory corruption errors that I was experiencing on device only.

[samdze]

Nice work.
So this actually fixed your memory corruption errors, that's great to hear.
I suspect yes, but does it work as expected in the simulator too?

[Nycto]

Yup — no problems in any place I’ve tested it so far.

Also, I understand there might be some hesitation to merge this. If you would prefer, I can make this opt-in functionality behind a compiler flag. Let me know if you would prefer that and I’ll update this PR.

[samdze]

I think this should be good and harmless to already existing projects, I'd go all-in.
I'll do some small tests myself, but I'm quite sure you've done a good job

[samdze]

I started to test this on macOS, Nim 1.16.14 and Nimble 0.13.1 with the example project.
At first, I was getting errors during device compilation:

Error: cannot open file: /Users/sam/.nimble/pkgs/src/playdate/bindings/malloc.nim
     Error: Build failed for package: playdate_example
        ... Execution failed with exit code 256
        ... Command: /Users/sam/.nimble/bin/nim c --colors:on --noNimblePath -d:device -d:release -d:NimblePkgVersion=0.8.0 --path:/Users/sam/.nimble/pkgs/playdate-0.13.0 -o:/Users/sam/Developer/Playdate/playdate-nim/playdate_example/playdate_example /Users/sam/Developer/Playdate/playdate-nim/playdate_example/src/playdate_example.nim
stack trace: (most recent call last)
/private/var/folders/pm/35qljjdj50q2xzmlzjjl1bt40000gn/T/nimblecache-586789740/nimscriptapi_3465337693.nim(187, 16)
/Users/sam/.nimble/pkgs/playdate-0.13.0/playdate/build/nimble.nim(87, 16) deviceTask
/Users/sam/.nimble/pkgs/playdate-0.13.0/playdate/build/utils.nim(22, 10) nimble
/Users/sam/.choosenim/toolchains/nim-1.6.16/lib/system/nimscript.nim(273, 7) exec
/Users/sam/.choosenim/toolchains/nim-1.6.16/lib/system/nimscript.nim(273, 7) Error: unhandled exception: FAILED: nimble -d:device -d:release build --verbose [OSError]
     Error: Exception raised during nimble script execution

I noticed the directory the build process was using to resolve the malloc.nim file was wrong.
That was weird, [...]/src/playdate/bindings/malloc can't be a correct path pointing to an installed package on macOS, but I'd think the same for other platforms. Does it assume the package is in develop mode?

So I tried changing it to point to the correct place: /Users/sam/.nimble/pkgs/playdate-0.13.0/playdate/bindings/malloc.nim

After that, both simulator and device started building fine.
On device, the game ran without issues.

But in the simulator, the game eventually crashes:

This seems to happen when a collision array (SDKArrayObj) has to be deallocated.
This crash doesn't happen when the first thing you do is touch the left wall, then you can generally move left, right and down touching walls with no issue.
When you press up the crash threat comes back and touching a wall can crash the game.

Setting a breakpoint on free, I also noticed not every time there is a collision the function is called, although this may be due to paging?
Leaving the malloc patch path unchanged makes no difference regarding this behaviour.

Something is likely corrupting memory, but I haven't looked into it much further.

[Nycto]

Oof. I’ve been testing on Ubuntu using Nim v2

That’s a really interesting crash point as I actually thought I was fully bypassing those functions by calling playdate.system.realloc directly.

I’ll have to see if I can get my hands on a Mac to test.

[Nycto]

Leaving the malloc patch path unchanged makes no difference regarding this behaviour

Just to be clear: are you saying this happens with and without my changes?

[samdze]

I gave it another try, these changes to the config.nim file seem to fix the issue:

os set to any prevents simulator compilation to build with libraries that would not be available in device anyways, so it should be good.

[Nycto]

Awesome — I’ll squeeze those changes into my PR

[samdze]

Leaving the malloc patch path unchanged makes no difference regarding this behaviour

Just to be clear: are you saying this happens with and without my changes?

I was referring to the path given to patchFile in config.nim, simulator was crashing with both /Users/sam/.nimble/pkgs/src/playdate/bindings/malloc.nim (the path that this PR resolves) and /Users/sam/.nimble/pkgs/playdate-0.13.0/playdate/bindings/malloc.nim

[samdze]

Works now on macOS. Windows still remains to be tested

[samdze]

Works fine on Windows too.

One thing that worries me is the way we are resolving the path to the playdate package.

1. Here we are also checking parent directories for a local version of the playdate-nim package, likely for development reasons, but is it appropriate to keep this?
2. If the user has multiple versions of the bindings installed the script autonomously chooses which version to use, probably even if the project specifies an exact version or a version range?

[Nycto]

The resolution rules are definitely funky. And, like you say, I just pushed an update this morning to get local development working across packages.

There are a few things I’m running into:

1. The call to patchFile is happening in the context of the downstream package, so we can’t just use relative paths.
2. Using nimble path playdate should be canonical, but it’s not always respecting local overrides. I suspect this is a bug in nimble, perhaps specific to Nim 2.
3. Using ../../../ is hacky and obviously doesn’t work in non-overridden use cases.

Thus, I’ve landed on what you see in this PR.

If you feel like this needs more consideration, I’m happy to spend more time to see if I can get the pure nimble path strategy working

[Nycto]

If the user has multiple versions of the bindings installed the script autonomously chooses which version to use, probably even if the project specifies an exact version or a version range?

I think nimble path should solve this, as I believe it resolves the version used by the current package

[samdze]

I think nimble path should solve this, as I believe it resolves the version used by the current package

Doesn't seem to be the case unfortunately, at least using Nimble 0.13.1.
Nimble 0.14+ could be different, it should output a list of paths, but I'm not sure their order is related in any way to the project configuration.

Having 0.12.0, 0.13.0 and 0.14.0 installed with the project configured using requires "playdate == 0.13.0" makes nimble path playdate resolve only one (0.14.0) version (probably the latest?)

   Checking for [email protected]
      Info: Dependency on [email protected] already satisfied
  Verifying dependencies for [email protected]
   Building playdate_example/playdate_example using c backend
  Executing /Users/sam/.nimble/bin/nim c --colors:on --noNimblePath -d:simulator -d:debug -d:NimblePkgVersion=0.8.0 --path:/Users/sam/.nimble/pkgs/playdate-0.13.0 -o:/Users/sam/Developer/Playdate/playdate-nim/playdate_example/playdate_example /Users/sam/Developer/Playdate/playdate-nim/playdate_example/src/playdate_example.nim
  
playdate packages path: /Users/sam/.nimble/pkgs/playdate-0.14.0

We can consider this good enough for now, but it is a thing we should keep in mind for the future.

[Nycto]

Yup — you’re totally right. I checked the nimble docs and that’s definitely the case.

I asked on the Nim discord to see if there is a way to do what we need: https://discord.com/channels/371759389889003530/753721959308853319/1218960176875442196

[samdze]

I'd say we can cope with this for now if you agree. We'll fix this detail eventually, if feasible.
If the PR is ready, let's merge it.

[Nycto]

Sounds good to me 👍

[samdze]

Thank you!