Remove duplicate definition of jl_gc_safepoint

[fingolfin]

This prevents a compiler warning on some systems

Please also backport to 1.8

[giordano]

This prevents a compiler warning on some systems

Which systems? Also, where is the other definition?

[fingolfin]

On Yggdrasil on eg freebsd

in julia_threads.h IIRC (there are only two word matches in all src/*.h for it). I kept that other one as it is properly protected against preprocessor substitution.

[Keno]

This seems odd to me. I think it's properly in julia.h for external code. Internal code should be using the manually inlined define in julia-threads.h. they used to be named the same, but the define is now jl_gc_safepoint_. Perhaps julia-threads needs to have a define to tell users to use the underscore version instead.

[fingolfin]

Note that julia_threads.h is unconditionally included from julia.h, so there is no way to avoid the duplicate definition for external code (hence this patch).

(In general the Julia headers seem to be a bit of a mess :/)

[fingolfin]

Ping?

Maybe to drive home my point, this is in src/julia_threads.h (note the parens around the name to prevent macro expansion, presumably because it used to have a macro jl_gc_safepoint(x), but that is now indeed called jl_gc_safepoint_):

JL_DLLEXPORT void (jl_gc_safepoint)(void);

So, the alternative to this patch would be remove the definition there and retain the one in julia.h. The effect is equivalent. I don't mind which one to remove, just tell me which one?

[fingolfin]

@Keno which way should I resolve this?

[vchuravy]

@Keno bump

[Keno]

Remove the one in julia_threads.h and add a define in julia_internal.h that tells people not to use it in favor of the underscore version. In the future, we can clean up the headers further.

[fingolfin]

I've changed it now to keep the one in julia.h.

Regarding adding a define to julia_internal.h, you mean something in the spirit of this?

#define jl_gc_safepoint(x)  "do not use jl_gc_safepoint" @#$%

to raise a compiler error if something uses jl_gc_safepoint?

But that would cause some code in the kernel to not compile anymore, because it does use jl_gc_safepoint. If all of these can be safely fixed by changing to jl_gc_safepoint_(jl_current_task->ptls), I'd be happy to make that change, but if more judgement beyond that is needed, I am not the person for the job -- and I'd find it somewhat unfair to hold this simple PR hostage to additional refactoring requirements. All I wanted to do is silence a compiler warning, which requires removing a single line of code that is obviously safe to remove; I don't think it is reasonable to expect additional refactoring.

[Keno]

If all of these can be safely fixed by changing to jl_gc_safepoint_(jl_current_task->ptls)

Yes, I think that should be safe everywhere.