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

Run non-Julia finalizers at exit #19911

Merged
merged 1 commit into from Jan 19, 2017
Merged

Conversation

barche
Copy link
Contributor

@barche barche commented Jan 6, 2017

It seems function-pointer finalizers are not run at exit. The example at the bottom of this comment illustrates the problem, the proposed fix here makes that work but crashes on an MPFR test in make test-all:

signal (11): Segmentation fault
while loading no file, in expression starting on line 0
mpfr_clear at /usr/lib/libmpfr.so (unknown line)
jl_gc_run_finalizers_in_list at /home/bjanssens/src/julia/julia/src/gc.c:199 [inlined]
run_finalizers at /home/bjanssens/src/julia/julia/src/gc.c:229 [inlined]
jl_gc_run_all_finalizers at /home/bjanssens/src/julia/julia/src/gc.c:266
jl_atexit_hook at /home/bjanssens/src/julia/julia/src/init.c:223
jl_exit at /home/bjanssens/src/julia/julia/src/jl_uv.c:554
exit at ./initdefs.jl:22
unknown function (ip: 0x7f4d3eda2c08)
jl_call_method_internal at /home/bjanssens/src/julia/julia/src/julia_internal.h:246 [inlined]
jl_apply_generic at /home/bjanssens/src/julia/julia/src/gf.c:2198
jl_apply at /home/bjanssens/src/julia/julia/src/julia.h:1388 [inlined]
jl_f__apply at /home/bjanssens/src/julia/julia/src/builtins.c:548
#512 at ./multi.jl:1488
run_work_thunk at ./multi.jl:1024
#511 at ./event.jl:66
jl_call_method_internal at /home/bjanssens/src/julia/julia/src/julia_internal.h:246 [inlined]
jl_apply_generic at /home/bjanssens/src/julia/julia/src/gf.c:2198
jl_apply at /home/bjanssens/src/julia/julia/src/julia.h:1388 [inlined]
start_task at /home/bjanssens/src/julia/julia/src/task.c:261

Clearly I did something wrong, but I'm out of ideas, so there goes my great plan to fix this myself :)

Example C code:

#include <iostream>

extern "C"
{

int c_int = 0;

}

struct PrintDestruction
{
  ~PrintDestruction()
  {
    std::cout << "final c_int is " << c_int << std::endl;
  }

  static PrintDestruction& instance()
  {
    static PrintDestruction m_instance;
    return m_instance;
  }

private:
  PrintDestruction()
  {
    std::cout << "initial c_int is " << c_int << std::endl;
  }
};

extern "C"
{

int get_c_int(void)
{
    return c_int;
}

void set_c_int(int i)
{
    c_int = i;
    PrintDestruction::instance();
}

void finalizer_cptr(void* v)
{
  std::cout << "Running C finalizer" << std::endl;
  set_c_int(-1);
}

}

Example Julia code:

using Base.Test

const libccalltest = joinpath(dirname(@__FILE__),"finalizers")

let A = [1]
    ccall((:set_c_int, libccalltest), Void, (Cint,), 1)
    @test ccall((:get_c_int, libccalltest), Cint, ()) == 1
    finalizer(A, cglobal((:finalizer_cptr, libccalltest), Void))
end

if (!gc_ptr_tag(v, 1)) {
schedule_finalization(v, f);
}
schedule_finalization(v, f);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is wrong. You have to run the finalizer instead of pushing it to the list.

@yuyichao
Copy link
Contributor

yuyichao commented Jan 6, 2017

And the correct fix should be

diff --git a/src/gc.c b/src/gc.c
index 83a22e9..4bde4b4 100644
--- a/src/gc.c
+++ b/src/gc.c
@@ -246,6 +246,9 @@ static void schedule_all_finalizers(arraylist_t *flist)
         if (!gc_ptr_tag(v, 1)) {
             schedule_finalization(v, f);
         }
+        else {
+            ((void (*)(void*))f)(gc_ptr_clear_tag(v, 1));
+        }
     }
     flist->len = 0;
 }

@barche
Copy link
Contributor Author

barche commented Jan 6, 2017

@yuyichao That's actually the first thing I tried, but it gave the same segfault in the MPFR test, again at the line that calls the non-Julia finalizer. I'm wondering if that segfault was just hidden because the finalizer didn't get called...

@barche
Copy link
Contributor Author

barche commented Jan 6, 2017

I didn't do the gc_ptr_clear_tag though, so maybe that was it.

@yuyichao
Copy link
Contributor

yuyichao commented Jan 7, 2017

At least your version segfault because it didn't clear the pointer tag and I imagine that's the issue in your other versions too. You didn't see that in you test case because you didn't actually read the pointer.

The use of tagged pointers and the restrictions of them in to_finalize is documented in the code comment.

@barche
Copy link
Contributor Author

barche commented Jan 7, 2017

Confirmed, this works without segfault on my machine. To be sure, the fact that the gc_ptr_clear_tag is missing here is not a bug, right?

@yuyichao
Copy link
Contributor

yuyichao commented Jan 7, 2017

julia/src/gc.c

Line 1603 in 77dfb35

void *v = gc_ptr_clear_tag(v0, 1);

@yuyichao yuyichao added the GC Garbage collector label Jan 7, 2017
@tkelman
Copy link
Contributor

tkelman commented Jan 7, 2017

Should we add the test case to libccalltest?

@yuyichao
Copy link
Contributor

yuyichao commented Jan 7, 2017

We don't need to add those to libccalltest. Mainly need to launch a separate process to test this.

@barche
Copy link
Contributor Author

barche commented Jan 8, 2017

Thanks for the comments and the explanations. I didn't add the test because I have no idea how to automate it, maybe it's possible to set the exit state in the finalizer or something?

@yuyichao
Copy link
Contributor

yuyichao commented Jan 8, 2017

You can print something jl_safe_printf in a C finalizer and check the output of a separate julia process that adds that C finalizer to a global variable.

@barche
Copy link
Contributor Author

barche commented Jan 11, 2017

I have added a test. I used fprintf, not sure if that's OK, but the output was not captured by readstring when using jl_safe_printf.

@yuyichao
Copy link
Contributor

fprintf or printf are fine (as long as it's not jl_printf). jl_safe_printf prints to STDERR so you need to capture that if you use it... The test should ideally also check the pointer. I would probably use it on a Ref{Cint}(42) and do printf("c_exit_finalizer: %d, %u\n", *(int*)v, ((unsigned)(uintptr_t)v) & 0xf); and check if the output is c_exit_finalizer: 42, 0.

@@ -798,6 +798,11 @@ let A = [1]
@test ccall((:get_c_int, libccalltest), Cint, ()) == -1
end

# Pointer finalizer at exit (PR #19911)
let result = readstring(`$(Base.julia_cmd()) -e "A = [1]; finalizer(A, cglobal((:c_exit_finalizer, \"$libccalltest\"), Void))"`)
@test result == "c_exit_finalizer"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

4 space indent

@@ -798,6 +798,11 @@ let A = [1]
@test ccall((:get_c_int, libccalltest), Cint, ()) == -1
end

# Pointer finalizer at exit (PR #19911)
let result = readstring(`$(Base.julia_cmd()) -e "A = [1]; finalizer(A, cglobal((:c_exit_finalizer, \"$libccalltest\"), Void))"`)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add --startup-file=no

@barche
Copy link
Contributor Author

barche commented Jan 12, 2017

OK, done!

@barche
Copy link
Contributor Author

barche commented Jan 13, 2017

I did one more update because the pointer tag check failed on 32 bit.

@@ -973,3 +973,7 @@ JL_DLLEXPORT float32x4_t test_ppc64_vec2(int64_t d1, float32x4_t a, float32x4_t
JL_DLLEXPORT int threadcall_args(int a, int b) {
return a + b;
}

JL_DLLEXPORT void c_exit_finalizer(void* v) {
printf("c_exit_finalizer: %d, %u", *(int*)v, (unsigned)((uintptr_t)v & (uintptr_t)1));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be 3 but this is also good enough...

@vchuravy vchuravy added this to the 0.6.0 milestone Jan 14, 2017
@tkelman
Copy link
Contributor

tkelman commented Jan 19, 2017

Can we merge this then?

@yuyichao yuyichao merged commit 4331b29 into JuliaLang:master Jan 19, 2017
kpamnany added a commit that referenced this pull request Oct 11, 2023
Since we no longer run finalizers at exit.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
GC Garbage collector
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants