For Loop Reached Unreachable Code

NEETdemon · June 3, 2024, 2:50am

Hello!

I come from garbage collected scripting langs like Python and TS, so I’m sorry if this is a redundant question. I tried looking it up but I don’t know what it is I’m looking at.

I’m experimenting more with this awesome language. What I am trying to do is have an array of strings and print out those same strings but within a new array/slice. I am using std.ArrayList to store the data, and it seemed to be going fine until I decided to use a For loop. My program compiles using:

zig build-exe strings_array.zig

But when I run it I get an error: thread 212950 panic: reached unreachable code

I’m thinking maybe I’m just managing memory incorrectly. Here is my code:

const std = @import("std");

const hello_arr = [_][]const u8{ "howdy", "hello", "waddup" };

fn handleStr(str: []const u8, allocator: std.mem.Allocator) ![]const u8 {
    var new_str = std.ArrayList(u8).init(allocator);
    errdefer new_str.deinit();

    for (str) |char| {
        try new_str.append(char);
    }

    return try new_str.toOwnedSlice();
}

pub fn main() !void {
    var gpa = std.heap.GeneralPurposeAllocator(.{}){};
    defer _ = gpa.deinit();
    const allocator = gpa.allocator();
    var words_arr = std.ArrayList([]const u8).init(allocator);
    errdefer words_arr.deinit();

    for (hello_arr) |str| {
        const new_str = try handleStr(str, allocator);
        defer allocator.free(new_str);
        try words_arr.append(new_str);
    }

    const new_str_arr = try words_arr.toOwnedSlice();
    defer allocator.free(new_str_arr);

    std.debug.print("{s}\n", .{new_str_arr[0]});
}

Again, I’m sorry if this is a redundant question and I would appreciate any advice on how to handle this. Thank you very very much!

permutationlock · June 3, 2024, 3:48am

The defer statement is running at the end of the for loop scope, so new_str is being freed just after it is appended to words_arr.

nyc · June 3, 2024, 3:49am

The defer runs that the end of the block, for loops that means after each iteration (this is in contrast to Go where they are run at the end of the function I believe).

So in your loop you are freeing the string right after you added it. Append (or any arraylist function) does not make a ddep copy.

remoeve that inner loop defer. Then at the bottom of main loop through the arraylist and frree everything then.

NEETdemon · June 3, 2024, 5:57am

You guys are awesome, thank you so much!

EDIT (sharing the solution):
Just like @nyc said, I just had to loop through the ArrayList and free everything inside:

pub fn main() !void {
    // ...

    for (hello_arr) |str| {
        const new_str = try handleStr(str, allocator);
        // removed the defer that was here
        try words_arr.append(new_str);
    }

    const new_str_arr = try words_arr.toOwnedSlice();
    defer allocator.free(new_str_arr);

    // loop through the new array, print the string, and free everything inside.
    for (new_str_arr) |str| {
        std.debug.print("{s}\n", .{str});
        allocator.free(str);
    }
}

squeek502 · June 3, 2024, 11:26am

Just to go the extra mile: this will leak memory on certain error conditions. For example, if the words_arr.append call within the loop fails, then the new_str memory returned from handleStr will not be freed.

The fix for that is pretty simple: errdefer

    for (hello_arr) |str| {
        const new_str = try handleStr(str, allocator);
        errdefer allocator.free(new_str);
        try words_arr.append(new_str);
    }

Note, though, that this class of problems can be tested for by using std.testing.checkAllAllocationFailures. See

for details on what it’s for/how it works, but here’s a quick adaptation of your code to use it:

const std = @import("std");

fn handleStr(str: []const u8, allocator: std.mem.Allocator) ![]const u8 {
    var new_str = std.ArrayList(u8).init(allocator);
    errdefer new_str.deinit();

    for (str) |char| {
        try new_str.append(char);
    }

    return try new_str.toOwnedSlice();
}

test "leaks" {
    try std.testing.checkAllAllocationFailures(std.testing.allocator, testImpl, .{});
}

const hello_arr = [_][]const u8{ "howdy", "hello", "waddup" };

fn testImpl(allocator: std.mem.Allocator) !void {
    var words_arr = std.ArrayList([]const u8).init(allocator);
    errdefer words_arr.deinit();

    for (hello_arr) |str| {
        const new_str = try handleStr(str, allocator);
        errdefer allocator.free(new_str);
        try words_arr.append(new_str);
    }

    const new_str_arr = try words_arr.toOwnedSlice();
    defer allocator.free(new_str_arr);

    for (new_str_arr) |str| {
        std.debug.print("{s}\n", .{str});
        allocator.free(str);
    }
}

With the errdefer in the loop, we still hit a leak (truncated to only include the most relevant lines of the stack trace):

fail_index: 2/4
allocated bytes: 136
freed bytes: 131
allocations: 2
deallocations: 1
allocation that was made to fail:
C:\Users\Ryan\Programming\Zig\tmp\leaks.zig:8:27: 0xfc10bd in handleStr (test.exe.obj)
        try new_str.append(char);
                          ^
C:\Users\Ryan\Programming\Zig\tmp\leaks.zig:25:38: 0xfc17e8 in testImpl (test.exe.obj)
        const new_str = try handleStr(str, allocator);
                                     ^
1/1 leaks.test.leaks...FAIL (MemoryLeakDetected)
C:\Users\Ryan\Programming\Zig\zig\lib\std\testing.zig:1103:21: 0xfc2801 in checkAllAllocationFailures__anon_2808 (test.exe.obj)
                    return error.MemoryLeakDetected;
                    ^
C:\Users\Ryan\Programming\Zig\tmp\leaks.zig:15:5: 0xfc2afc in test.leaks (test.exe.obj)
    try std.testing.checkAllAllocationFailures(std.testing.allocator, testImpl, .{});
    ^
[gpa] (err): memory address 0x23dc3150000 leaked:
C:\Users\Ryan\Programming\Zig\tmp\leaks.zig:8:27: 0xfc10bd in handleStr (test.exe.obj)
        try new_str.append(char);
                          ^
C:\Users\Ryan\Programming\Zig\tmp\leaks.zig:25:38: 0xfc17e8 in testImpl (test.exe.obj)
        const new_str = try handleStr(str, allocator);
                                     ^

This is a bit confusing because it shows the allocation that was made to fail and the allocation that leaked as the same code, but the problem is:

After a successful words_arr.append, there is a heap-allocated slice in the words_arr ArrayList
The slices within the words_arr ArrayList are only freed at the end of the test in the for (new_str_arr) loop which only runs when there are no errors.

So, the fix is to always free the slices that successfully get added to the ArrayList by moving the free into a defer. I’m going to make a few changes here, since the new_str_arr is also not necessary (you likely just want to access words_arr.items).

    var words_arr = std.ArrayList([]const u8).init(allocator);
    defer {
        for (words_arr.items) |str| {
            allocator.free(str);
        }
        words_arr.deinit();
    }

    for (hello_arr) |str| {
        const new_str = try handleStr(str, allocator);
        // Only free on error; if the append call succeeds then the defer
        // above will take care of it
        errdefer allocator.free(new_str);
        try words_arr.append(new_str);
    }

    // ...

And with that, the test passes when using checkAllAllocationFailures and we can be reasonably confident that we’re handling memory correctly even in the face of allocation failures.

NEETdemon · June 3, 2024, 9:23pm

I just keep learning more and more everyday. I forgot you could use brackets with defer and it doesn’t have to be a single line. I didn’t even know checkAllAllocationFailures even existed. Thank you so much!!

squeek502 · June 4, 2024, 12:33am

By the way, I typically take complicated defer blocks as a sign that it might (but not always) be worth wrapping the behavior in a struct. For example:

const WordsList = struct {
    list: std.ArrayList([]const u8),

    pub fn init(allocator: std.mem.Allocator) WordsList {
        return .{ .list = std.ArrayList([]const u8).init(allocator) };
    }

    pub fn deinit(self: *WordsList) void {
        for (self.list.items) |word| {
            self.list.allocator.free(word);
        }
        self.list.deinit();
    }

    /// Assumes the `word` is allocated by the same allocator
    /// as `self.list` and takes ownership of the slice on success
    /// (meaning `word` is freed when this `WordsList` is freed)
    pub fn appendOwned(self: *WordsList, word: []const u8) !void {
        return self.list.append(word);
    }

    /// Duplicates the `word` and inserts it into the list
    pub fn append(self: *WordsList, word: []const u8) !void {
        const duped = try self.list.allocator.dupe(u8, word);
        errdefer self.list.allocator.free(duped);

        return self.appendOwned(duped);
    }
}

this would change usage to:

var words_arr = WordsList.init(allocator);
defer words_arr.deinit();

for (hello_arr) |str| {
    const new_str = try handleStr(str, allocator);
    // Only free on error; if the append call succeeds then the defer
    // above will take care of it
    errdefer allocator.free(new_str);

    // Using appendOwned means that new_str will get freed during
    // `words_arr.deinit()` using the allocator passed in during `WordsList.init`
    try words_arr.appendOwned(new_str);
}

// or, if you had slices that you wanted to add that you don't want
// the WordsList to take ownership of/free, then:
for (hello_arr) |str| {
    // `append` will duplicate the str before adding it to the list
    try words_arr.append(str);
}

// To loop over the items in the list
for (words_arr.list.items) |word| {
    std.debug.print("{s}\n", .{word});
}

nyc · June 4, 2024, 12:47am

I despise these one or two line functions in APIs. Unless there is some way that the struct can do something better, faster, more interesting I think they are really bad API design. They increase the mental load, clog up documentation, and there are al almost infinite number of them. Let the caller dup them. It’s not that these functions are bad, but that they don’t carry their weight enough to justify them.

mnemnion · June 4, 2024, 2:53am

This looks like a case where the extra function pulls its weight imho. I would probably name them append and appendDupe though. A couple reasons: the function which does more says more, and appendOwned doesn’t actually make it clear what owns what. It could be interpreted as “append a string the caller owns (and therefore will free)”, the opposite of what it does. I would generally assume that if I feed something to an append function, the data structure now owns that something.

But I do think the split is good design, because you’re going to want to use it one way or the other, but not both. If it’s just append, or duplicate and then append in two lines, it’s harder to scan the function and make sure you used it consistently in the latter case. Then you have one string in an array which got moved because you didn’t dupe it, and only that string gets freed.

The problem that would cause can propagate quite a ways away from the source of injury.

In general (not just in Zig) I’ve come to favor more functions and fewer parameters. Although in the specific case of ...AssumeCapacity I’ve come around to thinking they should be ditched in favor of catch unreachable.

nyc · June 4, 2024, 3:09am

I didn’t think this was an important critique, but I actually had to go do this - I just thought it was a me thing. Seems legit.

It isn’t a huge deal with me, and we could name it appendDupe, I’d be ecstatic. (dyi, dupe is to deceive – maybe we could have appendScam, and appendGraft too! You prob meant dup, just the though of somebody seeing Dupe and going “WTF? Are they trying to be funny?” amused me.

you didn’t dupe it

gotta trick those arrays when you can and keep them on their toes or else they get a little complacent and don’t want to grow when they need to…

squeek502 · June 4, 2024, 5:39am

It can also mean duplicate, see definitions 3 and 4. See also std.mem.Allocator.dupe

nyc · June 4, 2024, 4:53pm

I didn’t know dupe was short for duplicate in the real world. Totally, thank you. (My programmer brain just assumed dup was the only legit abbreviation of that).