Why does the stac allocated version crash while the heap allocated version doesn't?

Help
1

hello
I apologize if it’s frowned on/not allowed to send this much content, please inform me if so
so I have this file

const std = @import("std");
const c = @cImport({
    @cInclude("miniaudio.h");
});

pub const Sound = struct {
    sound: c.ma_sound,
    allocator: std.mem.Allocator,
    engine: *c.ma_engine,
    pub fn init(allocator: std.mem.Allocator, engine: *c.ma_engine, path: []const u8) anyerror!Sound {
        var self: Sound = .{ .engine = engine, .allocator = allocator, .sound = undefined };
        const result = c.ma_sound_init_from_file(engine, path.ptr, 0, null, null, &self.sound);
        if (result != c.MA_SUCCESS) {
            return error.MiniaudioSoundInitializationFailed;
        }
        return self;
    }
    pub fn deinit(self: *Sound) void {
        c.ma_sound_uninit(&self.sound);
    }
    pub fn setLooping(self: *Sound, looping: bool) void {
        c.ma_sound_set_looping(&self.sound, if (looping) 1 else 0);
    }
    pub fn getLooping(self: Sound) bool {
        return if (c.ma_sound_is_looping(&self.sound) == 1) true else false;
    }
    pub fn playing(self: *Sound) bool {
        return if (c.ma_sound_is_playing(&self.sound) == 1) true else false;
    }
    pub fn setVolume(self: *Sound, volume: f32) anyerror!void {
        c.ma_sound_set_volume(&self.sound, volume);
    }
    pub fn getVolume(self: Sound) f32 {
        return c.ma_sound_get_volume(&self.sound);
    }
    pub fn setPan(self: *Sound, pan: f32) anyerror!void {
        c.ma_sound_set_pan(&self.sound, pan);
    }
    pub fn getPan(self: Sound) f32 {
        return c.ma_sound_get_pan(&self.sound);
    }

    pub fn setPitch(self: *Sound, pitch: f32) anyerror!void {
        c.ma_sound_set_pitch(&self.sound, pitch);
    }
    pub fn getPitch(self: Sound) f32 {
        return c.ma_sound_get_pitch(&self.sound);
    }
    pub fn play(self: *Sound, looping: bool) anyerror!void {
        self.setLooping(looping);
        const result = c.ma_sound_start(&self.sound);
        if (result != c.MA_SUCCESS) {
            return error.MiniAudioSoundStartFailed;
        }
    }
};

pub const SoundManager = struct {
    sounds: std.ArrayList(Sound),
    cleanFrequency: isize,
    engine: *c.ma_engine,
    allocator: std.mem.Allocator,
    pub fn init(allocator: std.mem.Allocator) anyerror!SoundManager {
        const engine: *c.ma_engine = try allocator.create(c.ma_engine);
        const result = c.ma_engine_init(null, engine);
        if (result != c.MA_SUCCESS) {
            return error.FailedToInitializeMiniaudioEngine;
        }
        return .{
            .engine = engine,
            .sounds = std.ArrayList(Sound).init(allocator),
            .allocator = allocator,
            .cleanFrequency = 3,
        };
    }
    pub fn deinit(self: *SoundManager) void {
        for (self.sounds.items) |*sound| {
            sound.deinit();
        }
        self.sounds.deinit();
        c.ma_engine_uninit(self.engine);
        self.allocator.destroy(self.engine);
    }
    pub fn play(self: *SoundManager, path: []const u8, looping: bool) anyerror!*Sound {
        var sound = try Sound.init(self.allocator, self.engine, path);
        try sound.play(looping);
        try self.sounds.append(sound);
        try self.clean();
        return &self.sounds.items[self.sounds.items.len - 1];
    }
    pub fn clean(self: *SoundManager) anyerror!void {
        self.cleanFrequency -= 1;
        if (self.cleanFrequency <= 0) {
            for (0..self.sounds.items.len - 1) |i| {
                if (i >= self.sounds.items.len or self.sounds.items[i].playing() or self.sounds.items[i].getLooping()) {
                    continue;
                }
                var sound = self.sounds.orderedRemove(i);
                sound.deinit();
            }
        }
    }
};

it crashs with a segmentation fault when I create an instance of SoundManager then call play(), however, when I instead allocate sound into the heap, the result is different

const std = @import("std");
const c = @cImport({
    @cInclude("miniaudio.h");
});

pub const Sound = struct {
    sound: *c.ma_sound,
    allocator: std.mem.Allocator,
    engine: *c.ma_engine,
    pub fn init(allocator: std.mem.Allocator, engine: *c.ma_engine, path: []const u8) anyerror!*Sound {
        const self = try allocator.create(Sound);
        self.engine = engine;
        self.allocator = allocator;
        self.sound = try allocator.create(c.ma_sound);
        _ = c.ma_sound_init_from_file(engine, path.ptr, 0, null, null, self.sound);
        return self;
    }
    pub fn deinit(self: *Sound) void {
        c.ma_sound_uninit(self.sound);
        self.allocator.destroy(self.sound);
        self.allocator.destroy(self);
    }
    pub fn setLooping(self: *Sound, looping: bool) void {
        c.ma_sound_set_looping(self.sound, if (looping) 1 else 0);
    }
    pub fn getLooping(self: Sound) bool {
        return if (c.ma_sound_is_looping(self.sound) == 1) true else false;
    }
    pub fn playing(self: *Sound) bool {
        return if (c.ma_sound_is_playing(self.sound) == 1) true else false;
    }
    pub fn setVolume(self: *Sound, volume: f32) anyerror!void {
        c.ma_sound_set_volume(self.sound, volume);
    }
    pub fn getVolume(self: Sound) f32 {
        return c.ma_sound_get_volume(self.sound);
    }
    pub fn setPan(self: *Sound, pan: f32) anyerror!void {
        c.ma_sound_set_pan(self.sound, pan);
    }
    pub fn getPan(self: Sound) f32 {
        return c.ma_sound_get_pan(self.sound);
    }

    pub fn setPitch(self: *Sound, pitch: f32) anyerror!void {
        c.ma_sound_set_pitch(self.sound, pitch);
    }
    pub fn getPitch(self: Sound) f32 {
        return c.ma_sound_get_pitch(self.sound);
    }
    pub fn play(self: *Sound, looping: bool) anyerror!void {
        self.setLooping(looping);
        const result = c.ma_sound_start(self.sound);
        if (result != c.MA_SUCCESS) {
            return error.MiniAudioSoundStartFailed;
        }
    }
};

pub const SoundManager = struct {
    sounds: std.ArrayList(*Sound),
    cleanFrequency: isize,
    engine: *c.ma_engine,
    allocator: std.mem.Allocator,
    pub fn init(allocator: std.mem.Allocator) anyerror!SoundManager {
        const engine: *c.ma_engine = try allocator.create(c.ma_engine);
        const result = c.ma_engine_init(null, engine);
        if (result != c.MA_SUCCESS) {
            return error.FailedToInitializeMiniaudioEngine;
        }
        return .{
            .engine = engine,
            .sounds = std.ArrayList(*Sound).init(allocator),
            .allocator = allocator,
            .cleanFrequency = 3,
        };
    }
    pub fn deinit(self: *SoundManager) void {
        for (self.sounds.items) |sound| {
            sound.deinit();
        }
        self.sounds.deinit();
        c.ma_engine_uninit(self.engine);
        self.allocator.destroy(self.engine);
    }
    pub fn play(self: *SoundManager, path: []const u8, looping: bool) anyerror!*Sound {
        const sound = try Sound.init(self.allocator, self.engine, path);
        try sound.play(looping);
        try self.sounds.append(sound);
        try self.clean();
        return sound;
    }
    pub fn clean(self: *SoundManager) anyerror!void {
        self.cleanFrequency -= 1;
        if (self.cleanFrequency <= 0) {
            for (0..self.sounds.items.len - 1) |i| {
                if (i >= self.sounds.items.len or self.sounds.items[i].playing() or self.sounds.items[i].getLooping()) {
                    continue;
                }
                var sound = self.sounds.orderedRemove(i);
                sound.deinit();
            }
        }
    }
};

this works perfectly fine
I want to know the reason?
if this is not enough information then do tell.

2

I believe that when you call ma_sound_init_from_file miniaudio is storing the passed in pointer to ma_sound struct in its internal graph of “engine nodes.” In your case this pointer is to the temporary stack value self, which goes out of scope when the function returns.

One way to do this without heap allocation would be to have the init function take a pointer to a sound struct to initialize, e.g. something like:

    pub fn init(self: *Sound, allocator: std.mem.Allocator, engine: *c.ma_engine, path: []const u8) anyerror!void {
        self.* = .{ .engine = engine, .allocator = allocator, .sound = undefined };
        const result = c.ma_sound_init_from_file(engine, path.ptr, 0, null, null, &self.sound);
        if (result != c.MA_SUCCESS) {
            return error.MiniaudioSoundInitializationFailed;
        }
    }

Of course then your SoundManager.play function would have to become something kind of ugly like this:

    pub fn play(self: *SoundManager, path: []const u8, looping: bool) anyerror!*Sound {
        try self.sounds.addOne();
        const sound = &self.sounds.items[self.sounds.items.len - 1];
        try sound.init(self.allocator, self.engine, path);
        try sound.play(looping);
        try self.clean();
        return sound;
    }

I’m sure there is a better way to organize this, but this is a “fix” with minimal changes to what is already there.

Edit: actually, this fix doesn’t work since the array list may resize on appending and those ma_sound objects really need to stay in place. So yes you likely either need to heap allocate them or use a fixed array (e.g. a BoundedArray instead of an ArrayList).

1 Like
3

oh, I see, makes sense
I guessed I likely will want heap allocated considering that I’m using c and c libraries tend to be dangerous with lifetimes compared to having explicit control when something dies or not, but I always realized that if I ignored it I won’t learn how to avoid i in the future or solve problems so I asked
what are your opinion, is this sound struct better being stack or heap allocated? Do you think it’s a waste/etc, lots of people told me to try avoid heap allocation as much as possible so I’m trying to have that mindset. Even though if the control of heap allocation is much more convenient.

4

I’ve been through this myself. I am hoping that Zig can get better at detecting UB over time.

If you’re not allocating stuff, it will go out of scope, get freed and you’ll be referencing invalid memory.

That doesn’t mean you have to allocate everything, but you need to take care when passing stack objects around.

5

I think it is fine to use heap allocation for pragmatic reasons like the APIs being easier to work with that way. It is good to always consider both and have it in the back of your mind as a thing that could be important. But I think it is most important to develop a sense of familiarity with both and learning when they are useful so that you can switch between them and decide when to use them.

There can be good reasons to avoid the heap and for people coming from languages like javascript or python that throw almost everything onto the heap, it is definitely a good exercise to make good use of the stack, but I wouldn’t go so far to say that the heap should be avoided at all costs.

The heap has its own benefits for dealing with things that have a lifetime that doesn’t correlate to function calls and also when dealing with things that are just far too big to fit on the stack, or are long-lived but also rarely used.

Overall it is a matter of tradeoffs and so it is mostly about gaining experience by trying both approaches and learning what works better in certain scenarios.

I think the main thing that should be avoided is to create every individual thing as an individual allocation, for example there is a big difference between allocating a single i32 over and over again and then having 10000+ individual allocations, or having like 100 sound objects which are allocated out of convenience, with you knowing why you did it and how and when you would change it when the circumstances change. So I would say the main thing that should be avoided is being completely unaware of your allocation patterns.

1 Like