kassane / sokol-d

D bindings for the sokol headers (https://github.com/floooh/sokol)
http://sokol-d.dub.pm/
zlib License
11 stars 5 forks source link

sokol-header: Assertion failed #5

Closed kassane closed 9 months ago

kassane commented 10 months ago

Occurs in ReleaseSafe and Debug build mode during run:

$> zig build clear          
$> ./zig-out/bin/clear      
Backend: Glcore33
clear: /home/kassane/sokol-d/src/sokol/c/sokol_gfx.h:8995: void _sg_gl_commit(void): Assertion '!_sg.gl.in_pass' failed.
[1]    12762 IOT instruction (core dumped)  ./zig-out/bin/clear

$> zig build debugtext_print
$> ./zig-out/bin/debugtext_print 
debugtext_print: /home/kassane/sokol-d/src/sokol/c/sokol_debugtext.h:3780: sdtx_context_desc_t _sdtx_context_desc_defaults(const sdtx_context_desc_t *): Assertion 'res.canvas_width > 0.0f' failed.
[1]    13331 IOT instruction (core dumped)  ./zig-out/bin/debugtext_print

sgl_context no assertion issue.

cc: @floooh

floooh commented 10 months ago

First one is easy:

https://github.com/kassane/sokol-d/blob/1b3d111ed3c0c5ee32c373c6e000cd0afd6d3441/src/examples/clear.d#L48-L49

The order of sg.commit() and sg.endPass() must be reversed (basically, sg.commit() must be the last thing in a frame).

The second one is tricky, it's this line:

https://github.com/floooh/sokol/blob/adf1f83657ac011168412eeb8f98f56dcd2969fe/util/sokol_debugtext.h#L3780

...but this assert should actually be impossible to hit, because the 0.0f value should have been replaced with _SDTX_DEFAULT_CANVAS_WIDTH (which is 640) here:

https://github.com/floooh/sokol/blob/adf1f83657ac011168412eeb8f98f56dcd2969fe/util/sokol_debugtext.h#L3775

This could be some sort of memory corruption or ABI issue. What operating system is this on? Also, stepping to the code in the debugger might help identifying the issue.

kassane commented 10 months ago

The order of sg.commit() and sg.endPass() must be reversed (basically, sg.commit() must be the last thing in a frame).

fixed!!

This could be some sort of memory corruption or ABI issue. What operating system is this on? Also, stepping to the code in the debugger might help identifying the issue.

I'm running on Linux. I'll debug it. Thanks for the clarification.

Also on the other examples, except clear. Although it was replicated line by line. I haven't had the same result... I'll have to see how the LDC2 compiler affects compared to the zig compiler.

kassane commented 10 months ago

WiP (based on rust example): image

floooh commented 10 months ago

How did you fix it? FWIW, sometimes I've been seeing weird platform-specific ABI related issues in language bindings where the struct params were corrupted on their way to the other language (for instance in Zig on Intel Macs).

I'll take care of a couple smaller requests and PRs over the next few days, and will most likely have time to look into the D bindings starting this weekend (unless you need more time for sample code).

(in any case I'll try to get a bit familiar with the D toolchain and tinker around with the bindings)

kassane commented 10 months ago

I''ve been seeing weird platform-specific ABI related issues in language bindings where the struct params were corrupted on their way to the other language (for instance in Zig on Intel Macs).

Yeah, I'm getting these issues in D, including betterC enabled (deactivating D runtime + part of phobos(std) and garbage collector). At the moment, I'm debugging the C solutions and the other bindings to track some changes in behavior.

How did you fix it?

The problem in the debug still persists, but I've had to rewrite the example followed by rust, instead of zig example.


Edit

Also, I'm adding a few more examples so that refactoring will also impact the bindgen!!! (e.g.: cube and blend)

Any help with revision is welcome!

kassane commented 10 months ago
module mymodule;

extern(C)
@nogc
nothrow {

    void my_c_function();

}

I find it to look cleaner, minimizing risks of mistakes

Bindgen already translates with a similar interface. https://github.com/kassane/sokol-d/blob/c42a959ab93e7c09bf64e36147e04c9885a54c5d/src/sokol/gfx.d#L1797-L1808

Only the examples are not auto-generated, allowing you to use what suits you best (with or without GC).

floooh commented 10 months ago

globals in D are threadlocal, i don't remember if sokol is threadsafe or not, so to be safe any global you use, you should mark them as __gshared to match C globals

This shouldn't matter for the sample code I would think. Personally I would prefer sample code that isn't annotated like this unless absolutely needed.

kassane commented 10 months ago

This won't work in -betterC, you have the same function in bunch of modules and seem to be only used in the app one, it's completely useless imo, just remove it, better to leave it to the user how to handle the ownership/lifetime of that c string, plus D has slices, [0 .. nullTermPos] is enough

example:

string cStrTod(inout(char)*  c_str) {
    auto start = c_str;
    auto end = cast(char*) c_str;
    for (; *end; end++){}
    return cast(string) c_str[0 .. end - start];
}

Fixed on https://github.com/kassane/sokol-d/commit/06840586d76619d1c63af55869e82cb9ae3a5aaa https://github.com/kassane/sokol-d/actions/runs/7433435082

Issue solved on betterC [Windows/MacOS only]

error: undefined reference to symbol __D4core8internal5array11duplication__T8_dupCtfeTaTyaZQpFNaNbNiNfMAaZAya
    note: referenced in /Users/runner/work/sokol-d/sokol-d/zig-cache/o/8441520852fd3bf30fcee699975c9b06/debugtext_print.o
error: undefined reference to symbol __D4core8internal5array11duplication__T8_dupCtfeTxaTaZQpFNaNbNiNfMAxaZAa
    note: referenced in /Users/runner/work/sokol-d/sokol-d/zig-cache/o/8441520852fd3bf30fcee699975c9b06/debugtext_print.o

Thank you!!

kassane commented 9 months ago

Back to the original topic of the problem. After running some debugging tests, I realized that the error in the assert is related to the fact that the float/double in D is nan instead of 0.0f by default.

sokol_debugtext.h:3780: sdtx_context_desc_t _sdtx_context_desc_defaults(const sdtx_context_desc_t *): Assertion `res.canvas_width > 0.0f` failed.

References

kassane commented 9 months ago

New brief test in sokol_debugtext.h:

diff --git a/src/sokol/c/sokol_debugtext.h b/src/sokol/c/sokol_debugtext.h
index 09da601..a44a673 100644
--- a/src/sokol/c/sokol_debugtext.h
+++ b/src/sokol/c/sokol_debugtext.h
@@ -3777,6 +3777,7 @@ static sdtx_context_desc_t _sdtx_context_desc_defaults(const sdtx_context_desc_t
     res.tab_width = _sdtx_def(res.tab_width, _SDTX_DEFAULT_TAB_WIDTH);
     // keep pixel format attrs are passed as is into pipeline creation
     SOKOL_ASSERT(res.char_buf_size > 0);
+    SOKOL_ASSERT(!isnan(res.canvas_width));
     SOKOL_ASSERT(res.canvas_width > 0.0f);
     SOKOL_ASSERT(res.canvas_height > 0.0f);
     return res;

Output:

steps [2/4] debugtext_print... debugtext_print: /home/kassane/sokol-d/src/sokol/c/sokol_debugtext.h:3780: sdtx_context_desc_t _sdtx_context_desc_defaults(const sdtx_context_desc_t *): Assertion `!__builtin_isnan (res.canvas_width)' failed.
run-debugtext_print
└─ run /home/kassane/sokol-d/zig-out/bin/debugtext_print failure
error: the following command terminated unexpectedly:
/home/kassane/sokol-d/zig-out/bin/debugtext_print
kassane commented 9 months ago

Finally fix! Added default values to structs number fields as opposed to init (nan).

image

floooh commented 9 months ago

Yeah that's what I also do in the Zig bindings:

https://github.com/floooh/sokol-zig/blob/7c941144c8b303ed58f1c9a47e3b4206d7c3fce3/src/sokol/gfx.zig#L468-L489

...it's important that non-initialized struct items are set to zero.

kassane commented 9 months ago

Yeah that's what I also do in the Zig bindings:

https://github.com/floooh/sokol-zig/blob/7c941144c8b303ed58f1c9a47e3b4206d7c3fce3/src/sokol/gfx.zig#L468-L489

...it's important that non-initialized struct items are set to zero.

Yes, when I started porting the zig script to D, it removed all default values, in consideration of the C concept. Now I've had a look at how to do float/double array filling without GC.

Maybe this solves the pipeline shader problem in sokol-tools. https://d.godbolt.org/z/W39ofzKqc (fill static array)