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

Add tracef to native runtime #599

Merged
merged 1 commit into from
Apr 20, 2024
Merged

Add tracef to native runtime #599

merged 1 commit into from
Apr 20, 2024

Conversation

JerwuQu
Copy link
Contributor

@JerwuQu JerwuQu commented Dec 1, 2022

Updated 2024-04-18!

Using vprintf directly would segfault on %s because it would try to read the pointer from host memory rather than WASM memory.

Code is mostly ported from web runtime.

Test code:

#include "wasm4.h"
void start() {
	tracef("|A|%%|%c|%d|0x%x|%s|%f|%u|%d|", 'B', 123, 0xcafe, "string", 1.23, 456, 789);
}

Results

Old:
- Web:    |A|%|B|123|0xcafe|string|1.23||456|
- Native: |A|%%|%c|%d|0x%x|%s|%f|%u|%d|
New:
+ Web:    |A|%|B|123|0xcafe|string|1.23|%u|456|
+ Native: |A|%|B|123|0xcafe|string|1.23|%u|456|

The web runtime was also modified to show unrecognized specifiers as they are rather than remove them (see %u).

@JerwuQu JerwuQu force-pushed the native-tracef branch 2 times, most recently from ef3d8d5 to 3f92563 Compare December 2, 2022 14:37
Copy link
Owner

@aduros aduros left a comment

Choose a reason for hiding this comment

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

Thanks!

In general what do you feel like should be our strategy with respect to buffer overruns, when users pass in a pointer that doesn't end in a null character? This goes for trace, the %s param of tracef, and text.

I guess we have 3 options.

  1. Consider these cases undefined behavior.
  2. Define that the cart will always crash immediately when an overrun occurs.
  3. Cap all reads to the end of the 64 KB memory (perhaps implemented by padding the 64 KB memory with an extra zero byte)

Now that I think about it, we might also want to standardize overrun behavior for blit/blitSub as well?

runtimes/native/src/runtime.c Outdated Show resolved Hide resolved
@JerwuQu
Copy link
Contributor Author

JerwuQu commented Dec 4, 2022

In general what do you feel like should be our strategy with respect to buffer overruns

Hmm... I want to say option 2 is the best, since crashes are often better than silent weird behavior. This code currently does option 3 since that's what the web runtime does. Perhaps we can split overruns into a separate issue and solve them all at the same time? Pretty much all pointers sent from WASM will have to be bounds- and null-terminator-checked. I'd avoid defining things as undefined behavior since that will make things runtime dependent.

Now that I think about it, we might also want to standardize overrun behavior for blit/blitSub as well?

Agreed! If we decide on BSOD for strings, we should do it for sprites as well.

@JerwuQu JerwuQu mentioned this pull request Feb 21, 2023
Using vprintf directly would segfault on %s because it would try to read the
pointer from host memory rather than WASM memory.
@JerwuQu
Copy link
Contributor Author

JerwuQu commented Apr 18, 2024

I changed <unknown symbol '%c'> to instead print the unknown character as if it wasn't handled, which should make it just as clear to the user without looking ugly.

For buffer overruns, I will open a separate issue.

@aduros
Copy link
Owner

aduros commented Apr 20, 2024

Thanks!

@aduros aduros merged commit 304127c into aduros:main Apr 20, 2024
5 checks passed
@JerwuQu JerwuQu deleted the native-tracef branch April 20, 2024 18:23
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