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

zig: deallocate a varargs function after applying it #591

Merged
merged 3 commits into from
Sep 19, 2024

Conversation

asarhaddon
Copy link
Contributor

This fixes an occasional segmentation fault.

@kanaka
Copy link
Owner

kanaka commented Dec 11, 2021

@asarhaddon sorry for the really slow response on this. I had to make the following change to get things to build (the /mal directory gets mounted over when running in docker mode):

diff --git a/impls/zig/Dockerfile b/impls/zig/Dockerfile
index 0c6a79eb..024c91d4 100644
--- a/impls/zig/Dockerfile
+++ b/impls/zig/Dockerfile
@@ -20,7 +20,7 @@ WORKDIR /mal
 
 RUN apt-get -y install ca-certificates curl gcc libc-dev libpcre3-dev libreadline-dev xz-utils
 
-RUN curl https://ziglang.org/download/0.8.1/zig-linux-x86_64-0.8.1.tar.xz | tar -xJC/mal
-RUN ln -fst/usr/local/bin /mal/zig-linux-x86_64-0.8.1/zig
+RUN curl https://ziglang.org/download/0.8.1/zig-linux-x86_64-0.8.1.tar.xz | tar -xJC/opt
+RUN ln -fst /usr/local/bin /opt/zig-linux-x86_64-0.8.1/zig
 
 ENV HOME /mal

Then I started getting a segfault in step6:

$ make "docker-build^zig"
$ make DOCKERIZE=1 "test^zig^step6"
...
TEST: '(f)' -> ['',9] -> SUCCESS
Testing whether closures can retain atoms
TEST: '(def! g (let* (atm (atom 0)) (fn* () (deref atm))))' -> ['',]
Exception: IOError(5, 'Input/output error')
Output before exception:
(def! g (let* (atm (atom 0)) (fn* () (deref atm))))
Segmentation fault at address 0x0
/opt/zig-linux-x86_64-0.8.1/lib/std/hash_map.zig:863:33: 0x20e638 in std.hash_map.HashMap([]const u8,*types.MalType,std.hash_map.StringContext,80).get (step6_file)
            return self.header().capacity;
                                ^

make: *** [Makefile:238: test^zig^step6] Error 1

@asarhaddon
Copy link
Contributor Author

In case this helps, a similar failure is visible with make test^zig^stepA REGRESS=1 during step3 tests, even if step3 itself passes the tests.
@rjtobin are you in a position to investigate the segmentation faults with zig 0.8.1 and/or update for zig 0.14.0?

Update build system, syntax and library calls for zig 0.13.0.

Rewrite the build system so that the steps can build separately.  Drop
intermediate symbolic links (unneeded complexity, confusing
timestamps).

Build with debugging options, this is a toy project full of memory
leaks.

Declare the allocators as global variables instead of passing and/or
storing always the same reference everywhere for no benefit.

Make apply_function a global variable instead of adding a reference to
EVAL in each function.

Pass arguments as a slice instead of using a different type for each
argument count.

There is no point in renaming default errors.

Remove a lot of reference counting and some indirection levels.
This fixes the current segmentation faults.

Create each object as soon as possible and use errdefer so that it is
deallocated if an exception occurs when computing its elemements.

Use a global variable to convey a MAL object alongside a thrown error.

Remove the unused logging_alloc module (but add a debug_alloc boolean
in types.zig).
@asarhaddon
Copy link
Contributor Author

asarhaddon commented Sep 18, 2024

I have rewritten large parts of the zig implementation, in order to fix the segmentation faults and merge eval_ast.
This should help with #657 and close #675, once the Docker image is updated.

@kanaka
Copy link
Owner

kanaka commented Sep 19, 2024

@asarhaddon The GHA failure is because the zig binary is under /mal which during builds is volume mounted over by the mal working directory. I think if you switch zig unpack location to /opt instead of /mal and symlink to it there it should work.

@kanaka kanaka merged commit 1c5ac14 into kanaka:master Sep 19, 2024
4 checks passed
@kanaka
Copy link
Owner

kanaka commented Sep 19, 2024

@asarhaddon I had forgotten that I have the ability to add commits to a PR (even from others) directly via the UI. Since it was a fairly trivial change, I went ahead and made it and things all passed. Merged.

@asarhaddon asarhaddon deleted the zig-segfault branch September 19, 2024 20:46
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

Successfully merging this pull request may close these issues.

2 participants