Skip to content

Conversation

@racerxdl
Copy link
Contributor

@racerxdl racerxdl commented Sep 8, 2020

These fixes are for what it looks like a compiler bug. Me and @deadprogram tried for a while to debug what caused that problems, and it looks like it is some unknown compiler bug which it doesn't seens to happen for other platforms.

There are two bugs:

  1. Depending on how the compiler optimizes the putchar, the slicing of a pre-allocated slice triggers a new memory allocation, which in some parts of the code isn't possible.
  2. The panic call in runtime_nintendoswitch_heap.go if only there, works just fine. But if the target code has another panic, neither of the panic's output is printed correctly (the string passed to the print call has a null pointer)

I can't be sure what's causing these errors, but this fixes it for now. I haven't got any issues in target code if that runtime code is like this.

@aykevl with this, the issues you were having in #1226 should be gone.

@racerxdl racerxdl force-pushed the fix/nintendo-switch-print branch from 4c1444b to e41db0c Compare September 8, 2020 21:15
@aykevl
Copy link
Member

aykevl commented Sep 8, 2020

Well that's interesting, and something I'd like to investigate further.

@racerxdl
Copy link
Contributor Author

racerxdl commented Sep 8, 2020

The panic thing is very easy to simulate, just make an code that panics anything. With the nxOutputDebug string in src/runtime/runtime_nintendoswitch_heap.go, the panic prints out correctly in Yuzu debug output. If you use panic in src/runtime/runtime_nintendoswitch_heap.go it gives an Invalid Memory Read for each character in the string.

Copy link
Member

@aykevl aykevl left a comment

Choose a reason for hiding this comment

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

I have confirmed that the changes in putchar do indeed fix the issue. Looking at the IR, this change makes sure putchar is not considered a potentially blocking function anymore, which is good. I haven't looked into why this change fixes that, but it might be because it proves to the compiler the function doesn't allocate (the compiler probably isn't smart enough to determine that the slice append could not allocate).

The panic below appears unrelated so I think it would be best to revert it (or change to runtimePanic) for code readability.

@aykevl
Copy link
Member

aykevl commented Sep 9, 2020

I managed to reproduce the incorrect behavior in the old code (with a plain panic in preinit). This issue is fixed by using runtimePanic instead.

@aykevl aykevl merged commit 0b9b293 into tinygo-org:dev Sep 9, 2020
@aykevl
Copy link
Member

aykevl commented Sep 9, 2020

Thank you!

@racerxdl racerxdl deleted the fix/nintendo-switch-print branch September 9, 2020 16:58
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.

2 participants