infinite print using warn on a recursive struct (sometimes)

[cgag]

code here: https://gist.github.com/cgag/04e2e921cb54474fcee0a6f20a1e3d98

So, it seems if I build this with build-exe, and run it It segfaults during the warn on the e1:

e1: Expr{ .Binary = Binary{ .left = Expr{ .Binary = Binary{ .left = Expr{ .[2]    17262 segmentation fault (core dumped)  ./main

If I build with release-fast, it prints e1 correctly, then basically prints a million recursive binary structs and segfaults:

ryzen ~ » zig build-exe --release-fast main.zig  && ./main
zig build-exe --release-fast main.zig  0.95s user 0.06s system 100% cpu 1.000 total
e1: Expr{ .Binary = Binary{ .left = Expr{ .Literal = Literal{ .String = left-value } }, .right = Expr{ .Literal = Literal{ .Number = 2.0e+01 } }, .token = PLUS } }
e: Expr{ .Binary = Binary{ .left = Expr{ .Binary = Binary{ .left = Expr{ .Binary = Binary{ .left = Expr{ .Binary = Binary{ .left = Expr{ .Binary = Binary{ .left = Expr{ .Binary = Binary{ .left = Expr{ .Binary = Binary{ .left =
 Expr{ .Binary = Binary{ .left = Expr{ .Binary = Binary{ .left = Expr{ .Binary = Binary{ .left = Expr{ .Binary = Binary{ .left = Expr{ .Binary = Binary{ .left = Expr{ .Binary = Binary{ .left = Expr{ .Binary = Binary{ .left = E
xpr{ .Binary = Binary{ .left = Expr{ .Binary = Binary{ .left = Expr{ .Binary = Binary{ .left = Expr{ .Binary = Binary{ .left = Expr{ .Binary = Binary{ .left = Expr{ .Binary = Binary{ .left = Expr{ .Binary = Binary{ .left = Exp
r{ .Binary = ... <segfault>

The only difference between the functions that produce the value that prints successfully (if built with release-fast), is the on that prints infinitely, is the first one is first rendered to a string by a function that doesn't modify the Expr.

[andrewrk]

I think we'll probably resolve this by having it only print x levels, where x is probably 2. After the limit is reached the pointers are not followed and it just prints the address.

[0joshuaolson1]

Isn't that too simplistic a fix?

[andrewrk]

Isn't that too simplistic a fix?

Can you elaborate?

[0joshuaolson1]

Can the number of levels be configured? Is the actual problem some default choice of type in the union? Maybe Zig's printing magic is too simple for what amounts to human-readable serialization of arbitrary data structures.

[cgag]

I may be misunderstanding something, but my actual concrete struct isn't self-referential, it's a tree that's only two levels deep, and is printed correctly once in the output:

e1: Expr{ .Binary = Binary{ .left = Expr{ .Literal = Literal{ .String = left-value } }, .right = Expr{ .Literal = Literal{ .Number = 2.0e+01 } }, .token = PLUS } }.

I don't see why it wouldn't be printed like this in the case that goes infinite.

[andrewrk]

My mistake, @cgag. It was my understanding that the struct was self-referential. If that is not this issue then it is another one, because I didn't try to handle self-referencing structs yet.

I'll have another look after the shortly upcoming 0.3.0 release.

[0joshuaolson1]

I misunderstood too. Sorry.

[tgschultz]

I believe the issue is that you're attempting to access invalid addresses. This is often the kind of behavior you see when referencing pointers to invalid locations on the stack, with the behavior differing wildly between release modes.

pub fn parse_infinite() Expr {
    var l = Expr{
        .Literal = Literal {
            .String = "left-value",
        }
    };
    var r = Expr{
        .Literal = Literal {
            .Number = 20.0,
        }
    };
    var e = Expr {
        .Binary = Binary{
            .left  = &l,
            .right = &r,
            .token = "PLUS",
        }
    };
    return e;
}

Here you return e, which contains pointers to l and r, which are only valid for the lifetime of the function call. When you call warn it dereferences these pointers and gets invalid data, making the behavior unpredictable.

expr_print should work since both l and r are still valid during the formatting, but you just free s in parse1 and return e.

[cgag]

Oh, good call, I guess I should have realized that. I just had all those literal values in there for testing and ran into errors as I started pulling things apart. My bad.

[tgschultz]

@cgag I was playing with this to verify there weren't any other errors and discovered that:

var alloc = &std.heap.DirectAllocator.init().allocator;

Doesn't work on Windows. It probably works fine on POSIX OSs though because DirectAllocator doesn't need to keep any state for them.

var dalloc = std.heap.DirectAllocator.init();
var alloc = &dalloc.allocator;

Fixes it.

[andrewrk]

Those 2 things should be the same. The first code snippet is doing the second code snippet, but with a hidden stack variable instead of an explicit one.

[andrewrk]

It would be neat if we could have a runtime safety check that detected this that didn't come at too high of a speed cost. If anyone has a proposal for how that might work, file an issue. We could probably take inspiration from MemorySanitizer

[tgschultz]

Those 2 things should be the same. The first code snippet is doing the second code snippet, but with a hidden stack variable instead of an explicit one.

You would think, but I assure you that the first one crashes on alloc in Windows.