Is this segfault due to parameter reference optimization or something else?

Help
,
1

Hello Ziggit! I’ve been trying to understand a bug I came across recently. Here’s a demo that shows what I’m talking about: when I run this (just using zig run <file>) with Zig version 0.12.0-dev.3496+a2df84d0f, I get a segfault.

const std = @import("std");

const Item = struct {
    number: usize,
};

pub fn main() !void {
    var gpa = std.heap.GeneralPurposeAllocator(.{}){};
    const allocator = gpa.allocator();

    var list = std.ArrayList(Item).init(allocator);
    defer list.deinit();

    try list.append(.{ .number = 1 });

    for (0..8) |_| {
        const item = list.items[0];
        try list.append(item);
    }
}

It seemed to me the bug was caused by an internal allocation/copy in the ArrayList invalidating pointers to the old list.items slice… but as far as I could tell, there shouldn’t be any such pointer, as const item = list.items[0] should make a copy as far as I understand (as should passing it to list.append). Using a debugger, I saw that the appended item indeed has the same address as list.items[0], but couldn’t figure out why that was the case.

Today, I came across the “ATTACK of the KILLER FEATURES” talk by Martin Wickham. Starting at 5:39 in that talk, there’s a description of what seems like exactly this bug: Zig will optimize and use a reference when passing item into list.append, which means we do have an invalid pointer into list.items. This feature is called “parameter reference optimization”.

However, now that I think about it, I still don’t quite understand what’s going on: where the video does something like list.append(list.items[0]), I do the extra step of setting a const to list.items[0]. Shouldn’t that make a copy regardless, meaning that even if a reference is passed into list.append, it won’t be a problem?

(Actually, I also found this issue which seems to be the same as my problem, const and all. So are there two things going on: a stack variable being optimized away by the compiler, and the aforementioned parameter reference optimization?)

2

Yes, it is due to parameter reference optimization, as explained in the video.

The following code works:

const std = @import("std");

const Item = struct {
    number: usize,
};

pub fn main() !void {
    var gpa = std.heap.GeneralPurposeAllocator(.{}){};
    defer _ = gpa.deinit();
    const allocator = gpa.allocator();

    var list = try std.ArrayList(Item).initCapacity(allocator, 9);
    defer list.deinit();

    try list.append(.{ .number = 1 });

    for (0..8) |_| {
        const item = list.items[0];
        try list.append(item);
    }
}

and the following code works:

const std = @import("std");

const Item = struct {
    number: usize,
};

pub fn main() !void {
    var gpa = std.heap.GeneralPurposeAllocator(.{}){};
    const allocator = gpa.allocator();

    var list = std.ArrayList(Item).init(allocator);
    defer list.deinit();

    const item: Item = .{ .number = 1 };
    try list.append(item);

    for (0..8) |_| {
        try list.append(item);
    }
}
1 Like
3

and this is considered a bug in Zig which will be resolved or “by design” thing?

is there a way to force a copy in this line somehow?

const item = list.items[0]; // i.e. @copy(list.items[0]);

just use var?

4

Hello @mholub, welcome to ziggit :slight_smile:

Yes, it is a bug.
The bug is described in the video:

5

It’s worth mentioning that we’ve had @andrewrk weigh in on this at one point in one of our more contentious threads. He’s confirmed that this is on their radar and they have ideas for handling the most common cases of this happening, but it’s just not front of the list right now.

1 Like
6

Thanks for the replies!

@dimdin, @AndrewCodeDev: how about the const in the loop not making a copy, is that a separate optimization contributing to the problem?

Also: I can write this up for the “Docs” section here on the forum, if you think this would be a suitable topic?

@mholub: At least, making it a var as you mentioned seems to solve the problem, though I think you then also have to do something like _ = &item to make it compile.

7

You could try reassigning to var in the loop? x = list.items[0] where x was previously declared. Just a hunch.

It’s going to be hard to write an example where the compiler doesn’t see through that chain of reasoning at higher optimization levels while keeping the example trivial.

We’re basically in a similar position here to something like the following:

list.appendSlice(list.items);

Which ironically could technically still work if you have the capacity but if it needs to resize then… ouch.

The compiler has a lot of dominion over const things (in this case, a little too much… but that’s something they’re going to look into as things develop). Try using a temporary var outside the loop to write to before appending it again. I looked at the assembly and it looks like it’s keeping the load and store instructions in order but I haven’t tried running this.

    var tmp: Item = .{ .number = 42 };

    for (0..8) |_| {
        tmp = list.items[0];
        try list.append(tmp);
    }
2 Likes