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

[build] Illegal instruction when calling 'ImageResize', related to "Disable UBSAN in zig builds" #3674

Closed
wisonye opened this issue Dec 26, 2023 · 7 comments

Comments

@wisonye
Copy link
Contributor

wisonye commented Dec 26, 2023

Please, before submitting a new issue verify and check:

  • [ x] I tested it on latest raylib version from master branch
  • [ x] I checked there is no similar issue already reported
  • [x ] I checked the documentation on the wiki
  • [ x] My code has no errors or misuse of raylib

Issue description

I'm using the latest master version and zig build to produce raylib/zig-out/libraylib.a, and then I write a simple zig program to load the image and draw texture. Everything works fine except it crashes if calling ImageResize. Here is the console log:

INFO: Initializing raylib 5.1-dev
INFO: Platform backend: DESKTOP (GLFW)
INFO: Supported raylib modules:
INFO:     > rcore:..... loaded (mandatory)
INFO:     > rlgl:...... loaded (mandatory)
INFO:     > rshapes:... loaded (optional)
INFO:     > rtextures:. loaded (optional)
INFO:     > rtext:..... loaded (optional)
INFO:     > rmodels:... loaded (optional)
INFO:     > raudio:.... loaded (optional)
INFO: DISPLAY: Device initialized successfully
INFO:     > Display size: 2560 x 1600
INFO:     > Screen size:  1024 x 768
INFO:     > Render size:  1024 x 768
INFO:     > Viewport offsets: 0, 0
INFO: GLAD: OpenGL extensions loaded successfully
INFO: GL: Supported extensions count: 153
INFO: GL: OpenGL device information:
INFO:     > Vendor:   Mesa
INFO:     > Renderer: virgl (AMD Radeon Pro 570X OpenGL Engine (Compat))
INFO:     > Version:  4.0 (Core Profile) Mesa 23.3.1-arch1.1
INFO:     > GLSL:     4.00
INFO: GL: VAO extension detected, VAO functions loaded successfully
INFO: GL: NPOT textures extension detected, full NPOT textures supported
INFO: GL: DXT compressed textures supported
INFO: GL: ETC2/EAC compressed textures supported
INFO: PLATFORM: DESKTOP (GLFW): Initialized successfully
INFO: TEXTURE: [ID 1] Texture loaded successfully (1x1 | R8G8B8A8 | 1 mipmaps)
INFO: TEXTURE: [ID 1] Default texture loaded successfully
INFO: SHADER: [ID 1] Vertex shader compiled successfully
INFO: SHADER: [ID 2] Fragment shader compiled successfully
INFO: SHADER: [ID 3] Program shader loaded successfully
INFO: SHADER: [ID 3] Default shader loaded successfully
INFO: RLGL: Render batch vertex buffers loaded successfully in RAM (CPU)
INFO: RLGL: Render batch vertex buffers loaded successfully in VRAM (GPU)
INFO: RLGL: Default OpenGL state initialized successfully
INFO: TEXTURE: [ID 2] Texture loaded successfully (128x128 | GRAY_ALPHA | 1 mipmaps)
INFO: FONT: Default font loaded successfully (224 glyphs)
INFO: TIMER: Target time per frame: 16.667 milliseconds
INFO: FILEIO: [resources/blue_laser.png] File loaded successfully
INFO: IMAGE: Data loaded successfully (53x271 | R8G8B8A8 | 1 mipmaps)
INFO: TEXTURE: [ID 3] Texture loaded successfully (53x271 | R8G8B8A8 | 1 mipmaps)
INFO: FILEIO: [resources/lightning_ball.png] File loaded successfully
INFO: IMAGE: Data loaded successfully (848x836 | R8G8B8A8 | 1 mipmaps)
Illegal instruction at address 0x506653
src/external/stb_image_resize2.h:6806:5: 0x506653 in stbir__alloc_internal_mem_and_build_samplers (/home/wison/zig/raylib-box2d-tutorials/raylib/src/rtextures.c)
src/external/stb_image_resize2.h:7603:14: 0x483415 in stbir__perform_build (/home/wison/zig/raylib-box2d-tutorials/raylib/src/rtextures.c)
src/external/stb_image_resize2.h:7639:12: 0x482184 in stbir_build_samplers_with_splits (/home/wison/zig/raylib-box2d-tutorials/raylib/src/rtextures.c)
src/external/stb_image_resize2.h:7649:10: 0x4835c9 in stbir_build_samplers (/home/wison/zig/raylib-box2d-tutorials/raylib/src/rtextures.c)
src/external/stb_image_resize2.h:7666:11: 0x483840 in stbir_resize_extended (/home/wison/zig/raylib-box2d-tutorials/raylib/src/rtextures.c)
src/external/stb_image_resize2.h:7775:9: 0x4866e9 in stbir_resize_uint8_linear (/home/wison/zig/raylib-box2d-tutorials/raylib/src/rtextures.c)
src/rtextures.c:1628:53: 0x4a3c4f in ImageResize (/home/wison/zig/raylib-box2d-tutorials/raylib/src/rtextures.c)
/home/wison/zig/raylib-box2d-tutorials/src/temp_test.zig:74:19: 0x27b295 in main (temp-test)
    rl.ImageResize(
                  ^
/home/wison/my-shell/zig-nightly/lib/std/start.zig:585:37: 0x27ba1b in main (temp-test)
            const result = root.main() catch |err| {
                                    ^
???:?:?: 0x7fa9a0274ccf in ??? (libc.so.6)
Unwind information for `libc.so.6:0x7fa9a0274ccf` was not available, trace may be incomplete

run temp-test: error: the following command terminated unexpectedly:
/home/wison/zig/raylib-box2d-tutorials/zig-cache/o/58730837b6dcd67266d95753780e6dd9/temp-test
Build Summary: 3/5 steps succeeded; 1 failed (disable with --summary none)
run-temp-test transitive failure
└─ run temp-test failure
error: the following build command failed with exit code 1:
/home/wison/zig/raylib-box2d-tutorials/zig-cache/o/133d2cba4db6e0394cb6df6df719b728/build /home/wison/my-shell/zig-nightly/zig /home/wison/zig/raylib-box2d-tutorials /home/wison/zig/raylib-box2d-tutorials/zig-cache /home/wison/.cache/zig --seed 0x33714a2a run-temp-test

Environment

  • OS : Arch Linux x86_64
  • Linux Kernel : 6.1.68-1-lts
  • Memory : 16.0 GiB
  • CPU: Intel i5-8500 (6) @ 3.000GHz
  • GPU: 01:00.0 Red Hat, Inc. Virtio 1.0 GPU
  • OpenGL : 4.0 (Compatibility Profile) Mesa 23.3.1-arch1.1
  • raylib commit hash ID(latest) : 9f22a4

Code Example

const rl = @cImport({
    @cInclude("raylib.h");
});

const GAME_FPS = 60;

///
///
///
pub fn main() !void {
    // Create a window with a particular size and title
    rl.InitWindow(1024, 768, "Minimal raylib program code template");
    defer rl.CloseWindow();

    // Set refresh rate (AKA, FPS: Frame Per Second)
    rl.SetTargetFPS(GAME_FPS);

    // Set tracing log level (DEBUG/INFO/WARN/ERROR)
    rl.SetTraceLogLevel(rl.LOG_DEBUG);

    //
    // Load image texture
    //
    var laser_sword_image = rl.LoadImage("resources/blue_laser.png");

    //
    // Crash here!!!
    // Crash here!!!
    // Crash here!!!
    //
    rl.ImageResize(@as([*c]rl.Image, @ptrCast(&laser_sword_image)), 20, 100);

    const laser_sword_texture: ?rl.Texture2D = rl.LoadTextureFromImage(laser_sword_image);
    rl.UnloadImage(laser_sword_image);

    //
    // Game loop
    //
    while (true) {
        rl.BeginDrawing();

        // Clear background
        rl.ClearBackground(rl.Color{ .r = 0x23, .g = 0x21, .b = 0x1B, .a = 0xFF });

        // Draw image texture
        if (laser_sword_texture) |texture| {
            rl.DrawTexturePro(
                texture,
                // Texture rect to draw from
                rl.Rectangle{
                    .x = 0.0,
                    .y = 0.0,
                    .width = @as(f32, @floatFromInt(texture.width)),
                    .height = @as(f32, @floatFromInt(texture.height)),
                },
                // Target rect to draw (orgin is TopLeft by default!!!)
                rl.Rectangle{
                    .x = 20.0,
                    .y = 20.0,
                    .width = @as(f32, @floatFromInt(texture.width)),
                    .height = @as(f32, @floatFromInt(texture.height)),
                },
                // Origin offset of the target rect to draw (TopLeft by default)
                rl.Vector2{ .x = 0.0, .y = 0.0 },
                0.0,
                rl.Color{ .r = 0xAC, .g = 0xE6, .b = 0xFE, .a = 0xFF },
            );
        }

        rl.EndDrawing();
    }

    //
    // Unload/clean up resources
    //
    if (laser_sword_texture) |value| rl.UnloadTexture(value);
}
@wisonye
Copy link
Contributor Author

wisonye commented Dec 26, 2023

I searched from the existing issues and then I found this pull request related to it.

This is the fix: -fno-sanitize=undefined which came from this commit

But it has been reverted in this commit

So, if I add -fno-sanitize=undefined back to the src/build.zig like this:

// This has been tested to work with zig 0.11.0 and zig 0.12.0-dev.1390+94cee4fb2
pub fn addRaylib(b: *std.Build, target: std.zig.CrossTarget, optimize: std.builtin.OptimizeMode, options: Options) *std.Build.CompileStep {
    const raylib_flags = &[_][]const u8{
        "-std=gnu99",
        "-D_GNU_SOURCE",
        "-DGL_SILENCE_DEPRECATION=199309L",
        "-fno-sanitize=undefined", // Add this line
    };

Then re-run zig build to re-produce raylib/zig-out/libraylib.a, the code example above works fine with no problem: rl.ImageResize works fine.

fixed-and-works

@raysan5
Copy link
Owner

raysan5 commented Dec 26, 2023

@wisonye Thanks for reporting! Feel free to send a PR!

NOTE: Not sure if related but recently stb_image_resize.h library has been replaced by stb_image_resize2.h, that uses CPU advance math functionality (SSE2, AVX, Neon and WASM SIMD), only used by ImageResize(). Not sure if it can be related.

@raysan5 raysan5 changed the title Illegal instruction when calling 'ImageResize', related to "Disable UBSAN in zig builds" [build] Illegal instruction when calling 'ImageResize', related to "Disable UBSAN in zig builds" Dec 26, 2023
@dertseha
Copy link
Contributor

Hi all, looking at the involved source I am not sure whether disabling a sanitizer is a good approach.

Short:
STBIR has separate code paths for cases where sanitizers are in use. I assume this avoids statements that would be flagged by these sanitizers. I furthermore suspect the sanitizer for "unknown behaviour" is not considered, and thus code is used that other sanitizers would complain about.

I believe it is better to explicitly force STBIR to use the safer code paths in a zig build.

Long:
The statement at the reported src/external/stb_image_resize2.h:6806 is the following (code link to v5.0):

STBIR__NEXT_PTR( info, sizeof( stbir__info ), stbir__info );

This is a macro, which is provided a few lines before, depending on another define:

#ifdef STBIR__SEPARATE_ALLOCATIONS
    #define STBIR__NEXT_PTR( ptr, size, ntype ) if ( alloced ) { void * p = STBIR_MALLOC( size, user_data); if ( p == 0 ) { stbir__free_internal_mem( info ); return 0; } (ptr) = (ntype*)p; }
#else
    #define STBIR__NEXT_PTR( ptr, size, ntype ) advance_mem = (void*) ( ( ((size_t)advance_mem) + 15 ) & ~15 ); if ( alloced ) ptr = (ntype*)advance_mem; advance_mem = ((char*)advance_mem) + (size);
#endif

The STBIR__SEPARATE_ALLOCATIONS define is set if an address sanitizer is detected. Relevant code here for v5.0.
I guess the "unknown" one is not considered, and it trips for, I assume, the pointer-alignment-magic that is performed by the #else path in STBIR__NEXT_PTR.

Furthermore:
I don't know the priorities of the code, whether it is for speed or for correctness, so I don't know at this point which approach to favour.
Personally I prefer the correct approach, so the (zig) build should (for now) set STBIR__SEPARATE_ALLOCATIONS. I say "for now" because this probably should be clarified in STBIR - However, because STBIR covers already for sanitizers, I suspect it would include the "unknown" one as well and defer to separate allocations.

Though, to be sure my cursory poking in the source is actually correct - @wisonye , may I ask you to test if building it with -DSTBIR__SEPARATE_ALLOCATIONS is an alternative fix, instead of disabling the sanitizer?

@wisonye
Copy link
Contributor Author

wisonye commented Dec 27, 2023

src/external/stb_image_resize2.h

@dertseha Hi, thanks for taking the time to dive deep and figure it out, but it doesn't fix the problem. Here is what I've tried in src/build.zig:

    const raylib_flags = &[_][]const u8{
        "-std=gnu99",
        "-D_GNU_SOURCE",
        "-DGL_SILENCE_DEPRECATION=199309L",
        "-DSTBIR__SEPARATE_ALLOCATIONS",  // What you ask for a test
    };

The problem is still here, same error:)

One more thing, if I remove the -fno-sanitize=undefined flag, another module also crashes (I just found it today trying to load and play audio in my demo).

This is what happened when I was trying to do const sound_effect = rl.LoadSound("resources/enable_fireball.wav");

INFO: AUDIO: Device initialized successfully
INFO:     > Backend:       miniaudio / PulseAudio
INFO:     > Format:        32-bit IEEE Floating Point -> 16-bit Signed Integer
INFO:     > Channels:      2 -> 2
INFO:     > Sample rate:   48000 -> 48000
INFO:     > Periods size:  3600
INFO: FILEIO: [resources/enable_fireball.wav] File loaded successfully
INFO: WAVE: Data loaded successfully (48000 Hz, 16 bit, 2 channels)
Illegal instruction at address 0x6fc636
src/external/miniaudio.h:54131:82: 0x6fc636 in ma_data_converter_init_preallocated (/home/wison/zig/raylib-box2d-tutorials/raylib/src/raudio.c)
src/external/miniaudio.h:54242:14: 0x6fe619 in ma_data_converter_init (/home/wison/zig/raylib-box2d-tutorials/raylib/src/raudio.c)
src/external/miniaudio.h:56061:14: 0x704fd5 in ma_convert_frames_ex (/home/wison/zig/raylib-box2d-tutorials/raylib/src/raudio.c)
src/external/miniaudio.h:56049:12: 0x704f71 in ma_convert_frames (/home/wison/zig/raylib-box2d-tutorials/raylib/src/raudio.c)
src/raudio.c:911:43: 0x7b599f in LoadSoundFromWave (/home/wison/zig/raylib-box2d-tutorials/raylib/src/raudio.c)
src/raudio.c:884:19: 0x7b5796 in LoadSound (/home/wison/zig/raylib-box2d-tutorials/raylib/src/raudio.c)
/home/wison/zig/raylib-box2d-tutorials/src/temp_test.zig:41:38: 0x2a3ff6 in main (temp-test)
    const sound_effect = rl.LoadSound("resources/enable_fireball.wav");
                                     ^
/home/wison/my-shell/zig-nightly/lib/std/start.zig:585:37: 0x2a480b in main (temp-test)
            const result = root.main() catch |err| {
                                    ^
???:?:?: 0x7fe8c9a77ccf in ??? (libc.so.6)
Unwind information for `libc.so.6:0x7fe8c9a77ccf` was not available, trace may be incomplete

Just in case you might doubt about the zig cache, I made sure that I removed all zig cache before making a clean build like this:

# Remove 'raylib' submodule zig related folder
rm -rf raylib/{zig-cache, zig-out}

# Remove my project zig related folder
rm -rf ./{zig-cache, zig-out}

# Then rebuild everything ....

@wisonye
Copy link
Contributor Author

wisonye commented Dec 27, 2023

I found that the better way is only to apply -fno-sanitize=undefined flag to the particular module in src/build.zig like this:

    if (options.raudio) {
        addCSourceFilesVersioned(raylib, &.{
            srcdir ++ "/raudio.c",
        }, &[_][]const u8{
            "-fno-sanitize=undefined",
        } ++ raylib_flags); // https://github.com/raysan5/raylib/issues/3674
    }

    if (options.rtextures) {
        addCSourceFilesVersioned(raylib, &.{
            srcdir ++ "/rtextures.c",
        }, &[_][]const u8{
            "-fno-sanitize=undefined",
        } ++ raylib_flags); // https://github.com/raysan5/raylib/issues/3674
    }
``

@dertseha
Copy link
Contributor

Well, that's unfortunate. I'm sorry that the approach with the define doesn't work. (Albeit I'm a bit puzzled that the define isn't picked up by STBIR)

As for the other crash, in miniaudio, the line 54131:82 refers to another macro that does some pointer offset magic (ma_offset_ptr, which is #define ma_offset_ptr(p, offset) (((ma_uint8*)(p)) + (offset)) -- no source linking possible on this one due to file size)
It's odd that even such rather "simple" byte offset operations are flagged. Either way, miniaudio doesn't have such alternative paths to try to use.

I'm still not satisfied that the only way out would be to disable a sanitizer, yet I'm also lacking further knowledge on these details. I wonder which particular operation is the wrong one - as in: Which undefined behaviour is the code relying on. (Plus: shouldn't the sanitizer name what it trips on?)
Yet it is also almost an academic curiosity.
Perhaps someone else can jump in on this topic then - otherwise, turning off the sanitizer it is, I guess.

@wisonye
Copy link
Contributor Author

wisonye commented Dec 27, 2023

Well, that's unfortunate. I'm sorry that the approach with the define doesn't work. (Albeit I'm a bit puzzled that the define isn't picked up by STBIR)

As for the other crash, in miniaudio, the line 54131:82 refers to another macro that does some pointer offset magic (ma_offset_ptr, which is #define ma_offset_ptr(p, offset) (((ma_uint8*)(p)) + (offset)) -- no source linking possible on this one due to file size) It's odd that even such rather "simple" byte offset operations are flagged. Either way, miniaudio doesn't have such alternative paths to try to use.

I'm still not satisfied that the only way out would be to disable a sanitizer, yet I'm also lacking further knowledge on these details. I wonder which particular operation is the wrong one - as in: Which undefined behaviour is the code relying on. (Plus: shouldn't the sanitizer name what it trips on?) Yet it is also almost an academic curiosity. Perhaps someone else can jump in on this topic then - otherwise, turning off the sanitizer it is, I guess.

@dertseha Yup, I know what you mean and usually, I will do that same when I have time (I prefer to figure out what happens under the hood and fix it in the right way). But I don't have much time on this at this moment, just want to finish my tutorial before my holiday ends:)

Also, what src/build.zig does is just build a zig program to compile all raylib C source files (like what zig cc does). But it passes different flags to (embedded clang) in different modes. For example, here is the thing you might want to know about:

ziglang/zig#18147 (comment)

Also, the following content comes from this blog

> When compiling without -O2 or -O3, Zig infers Debug Mode. Zig passes -fsanitize=undefined -fsanitize-trap=undefined to Clang in this mode. This causes Undefined Behavior to cause an Illegal Instruction.

So, I will fire the PR later for the quick fix, plz feel free to update here if you found a better solution:)

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

No branches or pull requests

3 participants