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

c4test segfaults with e.g. basic1.c4m #37

Closed
ee7 opened this issue Jun 18, 2024 · 6 comments · Fixed by #38 or #40
Closed

c4test segfaults with e.g. basic1.c4m #37

ee7 opened this issue Jun 18, 2024 · 6 comments · Fixed by #38 or #40

Comments

@ee7
Copy link
Contributor

ee7 commented Jun 18, 2024

Running c4test sometimes works:

$ build/c4test tests/basic1.c4m
info: Compiling from: tests/basic1.c4m
Processing module tests/basic1.c4m

[...]

Heap Usage: 27932 kb of 131072 kb (103139 kb free)
 Copied 7960 allocation records. Trashed 32380 records.
New Usage: 6304 kb (21627 kb collected)

and sometimes segfaults:

$ build/c4test tests/basic1.c4m
info: Compiling from: tests/basic1.c4m
zsh: segmentation fault (core dumped)  build/c4test tests/basic1.c4m

It looks like this was introduced by 1e6a017 (the codegen PR, #31). The commit before that seems OK.

The segfaults still occur with #36.

@viega
Copy link
Contributor

viega commented Jun 18, 2024

I cannot reproduce locally; please provide a stack trace.

@ee7
Copy link
Contributor Author

ee7 commented Jun 18, 2024

Quick one, from the latest commit on main (1721c36):

#0  0x0000000100117969 in hatrack_dict_cleanup (self=0x1001a92a0) at ../src/hatrack/hash/dict.c:89
#1  0x000000010002b3d0 in c4m_rc_free_and_cleanup (ptr=0x1001a92a8, callback=0x100117832 <hatrack_dict_cleanup>)
    at ../include/con4m/refcount.h:58
#2  0x000000010002bd53 in c4m_delete_arena (arena=0x7fffe7401000) at ../src/con4m/gc.c:301
#3  0x000000010002c5ac in c4m_collect_arena (ptr_loc=0x7ffff7f10718) at ../src/con4m/gc.c:659
#4  0x000000010002c5ea in c4m_gc_thread_collect () at ../src/con4m/gc.c:667
#5  0x000000010000c463 in collect_and_print_stats () at ../src/tests/test.c:24
#6  0x000000010000cd2c in test2 () at ../src/tests/test.c:184
#7  0x000000010000f073 in main (argc=1, argv=0x7fffffffe6a8, envp=0x7fffffffe6b8) at ../src/tests/test.c:824

if (self->free_handler) {
store = atomic_load(&self->crown_instance.store_current);
for (i = 0; i <= store->last_slot; i++) {
bucket = &store->buckets[i];
hv = atomic_load(&bucket->hv);

@viega
Copy link
Contributor

viega commented Jun 18, 2024

Well, there is no free handler being set anywhere via con4m, so that branch shouldn't be running, period. That means the underlying object is most likely getting corrupted somehow (scribbling over the free_hander field). I say that because, even if not using calloc(), hatrack_dict_init is explicit about zeroing out fields. Unfortunately, when I add a watchpoint in a debugger, I see no corruption whatsoever.

So if you can track down any more info, that would be very helpful.

@viega
Copy link
Contributor

viega commented Jun 18, 2024

Okay, I found a candidate for the culprit.

Can you try, in c4m_collect_arena (gc.c), change from this:

    if (r == NULL) {
        r = c4m_rc_ref(global_roots);
    }

to this:

    if (r == NULL) {
        r = global_roots;
    }

    c4m_rc_ref(r);

It's definitely wrong and have made the change locally; even though I can't reproduce directly, I feel pretty good about it being the issue.

@ee7
Copy link
Contributor Author

ee7 commented Jun 18, 2024

Can you try

That fixed it, thanks.

Let's add an initial testing workflow in CI? I've created #38, which shows the current segfault:

./dev: line 123: 3353 Segmentation fault (core dumped) ./c4test $@

and succeeds when your fix is applied.

Still need to make it find the crypto library on macOS, though.

ee7 added a commit that referenced this issue Jun 18, 2024
After commit 1e6a017 (2024-06-17, "Code generation"), running
c4test could sometimes produce a segfault:

   $ build/c4test tests/basic1.c4m
   info: Compiling from: tests/basic1.c4m
   zsh: segmentation fault (core dumped)  build/c4test tests/basic1.c4m

Apply John's fix [1].

Fixes: #37

[1] #37 (comment)
@ee7
Copy link
Contributor Author

ee7 commented Jun 18, 2024

I don't know whether you wanted to make any other changes there right now, but I created #39 in case that's helpful. Please close it if you'd prefer to create a PR yourself.

This was referenced Jun 19, 2024
@viega viega closed this as completed in #40 Jun 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants