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

Implements TextFormat in JS #43

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

VICTORVICKIE
Copy link
Contributor

I have implemented essential formatting (without 3rd party dependency). if you are still in the idea of compiling stb_printf, please close this.

This implementation is around 400L of code, potentially fixes #34

The below needs review, I still have the following question regarding the Wasm memory layout.

Qn: Is it safe to use the slots in the memory buffer starting from 0? Is there any way to create an internal shared buffer that can be shared with Wasm?

TextFormat(text_ptr, args_ptr) {
        const buffer = this.wasm.instance.exports.memory.buffer;
        const fmt_text = cstr_fmt_by_ptr(buffer, text_ptr, args_ptr);

        // REVIEW: Is it safe to use the slots in the memory buffer starting from 0?
        // Is there any way to create an internal shared buffer that can be shared with Wasm?
        const bytes = new Uint8Array(buffer, 0, fmt_text.length + 1);
        for (let i = 0; i < fmt_text.length; i++) {
            bytes[i] = fmt_text.charCodeAt(i);
        }
        bytes[fmt_text.length] = 0;
        return bytes;
    }

Result (Underlined in RED)
image

Comparison: (https://en.cppreference.com/w/c/io/fprintf)
clang version 17.0.4
Target: x86_64-w64-windows-gnu
image

wasm:
image

@Markos-Th09
Copy link

A memory in webassembly is the entire address space of the program, which means it can be overriden. In this case I think it is fine, however a proper allocator might be a better solution.

@VICTORVICKIE
Copy link
Contributor Author

@Markos-Th09 Thank you for the clarification. Could you please assist me with finding the resource for the allocator? I tried using the https://developer.mozilla.org/en-US/docs/WebAssembly/JavaScript_interface/Memory API, but I couldn't get it to work as expected. My goal is to avoid any memory corruption. Would my current approach lead to memory corruption? Currently, the web and native functions seem to be working fine.
image

@AntonPieper
Copy link

AntonPieper commented Feb 21, 2024

There are more exports available if you compile with the needed options. Maybe these are needed to write a custom allocator.

The following are the exports I get when I add -Wl,--export-all to the nob build script (a selected snippet from wasm2wat wasm/core_basic_window.wasm):

  (type (;0;) (func))
  (type (;1;) (func (param i32)))
  (type (;2;) (func (param i32 i32 i32 i32 i32)))
  (type (;3;) (func (param i32 i32 i32)))
  (type (;4;) (func (result i32)))
  (type (;5;) (func (param i32 i32) (result i32)))
  (table (;0;) 2 2 funcref)
  (memory (;0;) 2)
  (global (;1;) i32 (i32.const 1024))
  (global (;2;) i32 (i32.const 1102))
  (global (;3;) i32 (i32.const 1104))
  (global (;4;) i32 (i32.const 66640))
  (global (;5;) i32 (i32.const 1024))
  (global (;6;) i32 (i32.const 66640))
  (global (;7;) i32 (i32.const 131072))
  (global (;8;) i32 (i32.const 0))
  (global (;9;) i32 (i32.const 1))
  (elem (;0;) (i32.const 1) func $GameFrame)
  (export "memory" (memory 0))
  (export "__wasm_call_ctors" (func $__wasm_call_ctors))
  (export "GameFrame" (func $GameFrame))
  (export "__original_main" (func $__original_main))
  (export "main" (func $main))
  (export "__main_void" (func $__original_main))
  (export "__indirect_function_table" (table 0))
  (export "__dso_handle" (global 1))
  (export "__data_end" (global 2))
  (export "__stack_low" (global 3))
  (export "__stack_high" (global 4))
  (export "__global_base" (global 5))
  (export "__heap_base" (global 6))
  (export "__heap_end" (global 7))
  (export "__memory_base" (global 8))
  (export "__table_base" (global 9))

Also you can see there are *_low exports, so if you never exceed any of those, I guess you are fine with address 0

@Markos-Th09
Copy link

Markos-Th09 commented Feb 21, 2024

Forget what I said about 0 being safe. Actually address 0 by default is in the data region, which ends at address 1024.

@VICTORVICKIE
Copy link
Contributor Author

image

I can see it overrides the previous call's allocation, but since this is an immediate mode, it gets recreated every call frame, so I guess it will be fine if I use an index after the heap base.

@Markos-Th09
Copy link

Markos-Th09 commented Feb 22, 2024

image

I can see it overrides the previous call's allocation, but since this is an immediate mode, it gets recreated every call frame, so I guess it will be fine if I use an index after the heap base.

Obviously it overrides the previous frame's allocation because it is always on the same address, which why an allocator could be better

@VICTORVICKIE
Copy link
Contributor Author

Yeah, but that needs more work like making sure we are writing to chunk where there is no other data and keeping track of index and stuff. Does the job for now, right? Is there an easy way, like an internal memory using WebAssembly.memory?

@Markos-Th09
Copy link

Markos-Th09 commented Feb 22, 2024

Yeah, but that needs more work like making sure we are writing to chunk where there is no other data and keeping track of index and stuff. Does the job for now, right? Is there an easy way, like an internal memory using WebAssembly.memory?

For simplicity sake you can use a simple bump allocator for now, but you can also write a better one. You can just increment a pointer from __heap_base every time you need to allocate memory.

@Markos-Th09
Copy link

Or you can use an approach like the actual raylib which has a static array of buffers which is stored in the data section

@VICTORVICKIE
Copy link
Contributor Author

I think it can be overridden, it does some sort of copy from this wasm memory, I don't see any flickering or weird text, will just use heap base and let it be as it is for now.

Desktop.2024.02.22.-.10.41.48.02.mp4

@Markos-Th09
Copy link

I think it can be overridden, it does some sort of copy from this wasm memory, I don't see any flickering or weird text, will just use heap base and let it be as it is for now.

Desktop.2024.02.22.-.10.41.48.02.mp4

well any region can be overridden by dumb code but nobody intentionally writes to the data section unless they need a static. you are free to implement it however you like

@VICTORVICKIE
Copy link
Contributor Author

Thanks for the suggestion, will update in future if any issue arises, will make sure to mention in code comment too.

@Markos-Th09
Copy link

Markos-Th09 commented Feb 23, 2024

Thanks for the suggestion, will update in future if any issue arises, will make sure to mention in code comment too.

This still doesn’t solve the problem that the text will overwritten each time this function is executed. I highly suggest you take a look at the actual raylib implementation.

@AntonPieper
Copy link

Thanks for the suggestion, will update in future if any issue arises, will make sure to mention in code comment too.

This still doesn’t solve the problem that the text will overwritten each time this function is executed.

In raylib, a static ring buffer is used, maybe we could use a similar solution?

@VICTORVICKIE
Copy link
Contributor Author

Thanks for the suggestion, will update in future if any issue arises, will make sure to mention in code comment too.

This still doesn’t solve the problem that the text will overwritten each time this function is executed. I highly suggest you take a look at the actual raylib implementation.

I have looked into actual raylib implementation they are using an array of buffers and cycling through them, but in our case, I'm just using heap and since I don't actually see any issue with the current implementation even though they overwritten each time, so why fix which is not broken.

@VICTORVICKIE
Copy link
Contributor Author

VICTORVICKIE commented Feb 23, 2024

If you can reproduce any issue (apart from another function trying to write to same heap base) with my current implementation, please do let me know, so i can fix on that

@VICTORVICKIE
Copy link
Contributor Author

If you can reproduce any issue (apart from another function trying to write to same heap base) with my current implementation, please do let me know, so i can fix on that

I agree we need to fix collisions with other function implementations which may use heap base and may override TextFormat buffer, even if we use an allocator, I doubt that will fix it, so if you have any idea on how to fix this issue, do let me know, I am looking for simplistic alternative too.

@Markos-Th09
Copy link

Thanks for the suggestion, will update in future if any issue arises, will make sure to mention in code comment too.

This still doesn’t solve the problem that the text will overwritten each time this function is executed. I highly suggest you take a look at the actual raylib implementation.

I have looked into actual raylib implementation they are using an array of buffers and cycling through them, but in our case, I'm just using heap and since I don't actually see any issue with the current implementation even though they overwritten each time, so why fix which is not broken.

Basically your current implementation breaks when you try to store a format in a variable and then use the function again. the value in the variable will be the new format since the same location in memory was used by both formats. Raylib solves this with a fixed size ring buffer which sufficiently large.

@Markos-Th09
Copy link

About the allocator thing, if both sides agree on using the same allocator you can manage the heap in a way that you can avoid memory corruption, unless you really want to cause memory corruption. Although this a bit beyond the scope of this PR in my opinion.

@VICTORVICKIE
Copy link
Contributor Author

VICTORVICKIE commented Feb 23, 2024

I ll make a todo for now and if this pr gets accepted, i ll work on the allocator, as you said it is out of scope for this pr.

@AntonPieper
Copy link

AntonPieper commented Feb 23, 2024

Instead of this you could also only implement sprintf and Link with the necessary raylib code.

Example:

raylib-native.c

// From rcore.c
#include <stdarg.h>
typedef unsigned long size_t;

int sprintf ( char * str, const char * format, ... );
int vsnprintf (char * s, size_t n, const char * format, va_list arg);
void * memset ( void * ptr, int value, size_t num);

// Formatting of text with variables to 'embed'
// WARNING: String returned will expire after this function is called MAX_TEXTFORMAT_BUFFERS times
const char *TextFormat(const char *text, ...)
{
#ifndef MAX_TEXTFORMAT_BUFFERS
    #define MAX_TEXTFORMAT_BUFFERS      4        // Maximum number of static buffers for text formatting
#endif
#ifndef MAX_TEXT_BUFFER_LENGTH
    #define MAX_TEXT_BUFFER_LENGTH   1024        // Maximum size of static text buffer
#endif

    // We create an array of buffers so strings don't expire until MAX_TEXTFORMAT_BUFFERS invocations
    static char buffers[MAX_TEXTFORMAT_BUFFERS][MAX_TEXT_BUFFER_LENGTH] = { 0 };
    static int index = 0;

    char *currentBuffer = buffers[index];
    memset(currentBuffer, 0, MAX_TEXT_BUFFER_LENGTH);   // Clear buffer before using

    va_list args;
    va_start(args, text);
    int requiredByteCount = vsnprintf(currentBuffer, MAX_TEXT_BUFFER_LENGTH, text, args);
    va_end(args);

    // If requiredByteCount is larger than the MAX_TEXT_BUFFER_LENGTH, then overflow occured
    if (requiredByteCount >= MAX_TEXT_BUFFER_LENGTH)
    {
        // Inserting "..." at the end of the string to mark as truncated
        char *truncBuffer = buffers[index] + MAX_TEXT_BUFFER_LENGTH - 4; // Adding 4 bytes = "...\0"
        sprintf(truncBuffer, "...");
    }

    index += 1;     // Move to next buffer for next function call
    if (index >= MAX_TEXTFORMAT_BUFFERS) index = 0;

    return currentBuffer;
}

test.c

const char *TextFormat(const char *text, ...);
int main() {
  const char *text = TextFormat("%s!", "Hello, World");
}

To compile:

# compile "raylib-native.o"
clang -o raylib-native.o raylib-native.c --target=wasm32 -c
# compile "test.wasm"
clang -o test.wasm test.c raylib-native.o --target=wasm32 -Wl,--export-all,--no-entry,--allow-undefined -nostdlib

@VICTORVICKIE
Copy link
Contributor Author

Instead of this you could also only implement sprintf and Link with the necessary raylib code.

Example:

raylib-native.c

// From rcore.c
#include <stdarg.h>
typedef unsigned long size_t;

int sprintf ( char * str, const char * format, ... );
int vsnprintf (char * s, size_t n, const char * format, va_list arg);
void * memset ( void * ptr, int value, size_t num);

// Formatting of text with variables to 'embed'
// WARNING: String returned will expire after this function is called MAX_TEXTFORMAT_BUFFERS times
const char *TextFormat(const char *text, ...)
{
#ifndef MAX_TEXTFORMAT_BUFFERS
    #define MAX_TEXTFORMAT_BUFFERS      4        // Maximum number of static buffers for text formatting
#endif
#ifndef MAX_TEXT_BUFFER_LENGTH
    #define MAX_TEXT_BUFFER_LENGTH   1024        // Maximum size of static text buffer
#endif

    // We create an array of buffers so strings don't expire until MAX_TEXTFORMAT_BUFFERS invocations
    static char buffers[MAX_TEXTFORMAT_BUFFERS][MAX_TEXT_BUFFER_LENGTH] = { 0 };
    static int index = 0;

    char *currentBuffer = buffers[index];
    memset(currentBuffer, 0, MAX_TEXT_BUFFER_LENGTH);   // Clear buffer before using

    va_list args;
    va_start(args, text);
    int requiredByteCount = vsnprintf(currentBuffer, MAX_TEXT_BUFFER_LENGTH, text, args);
    va_end(args);

    // If requiredByteCount is larger than the MAX_TEXT_BUFFER_LENGTH, then overflow occured
    if (requiredByteCount >= MAX_TEXT_BUFFER_LENGTH)
    {
        // Inserting "..." at the end of the string to mark as truncated
        char *truncBuffer = buffers[index] + MAX_TEXT_BUFFER_LENGTH - 4; // Adding 4 bytes = "...\0"
        sprintf(truncBuffer, "...");
    }

    index += 1;     // Move to next buffer for next function call
    if (index >= MAX_TEXTFORMAT_BUFFERS) index = 0;

    return currentBuffer;
}

test.c

const char *TextFormat(const char *text, ...);
int main() {
  const char *text = TextFormat("%s!", "Hello, World");
}

To compile:

# compile "raylib-native.o"
clang -o raylib-native.o raylib-native.c --target=wasm32 -c
# compile "test.wasm"
clang -o test.wasm test.c raylib-native.o --target=wasm32 -Wl,--export-all,--no-entry,--allow-undefined -nostdlib

this seems more of a pain tbh, not sure how I feel about it, system should be as simple as possible imo

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.

sprintf in js
3 participants