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

Remove duplicate definition of jl_gc_safepoint #45120

Merged

Conversation

fingolfin
Copy link
Contributor

This prevents a compiler warning on some systems

Please also backport to 1.8

@giordano giordano added the backport 1.8 Change should be backported to release-1.8 label Apr 29, 2022
@giordano
Copy link
Contributor

This prevents a compiler warning on some systems

Which systems? Also, where is the other definition?

@fingolfin
Copy link
Contributor Author

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
Copy link
Member

Keno commented Apr 30, 2022

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
Copy link
Contributor Author

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 :/)

@KristofferC KristofferC mentioned this pull request May 16, 2022
67 tasks
@fingolfin
Copy link
Contributor Author

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?

@KristofferC KristofferC mentioned this pull request May 28, 2022
36 tasks
@fingolfin
Copy link
Contributor Author

@Keno which way should I resolve this?

@vchuravy vchuravy added this to the 1.8 milestone Jun 9, 2022
@vchuravy
Copy link
Member

@Keno bump

@Keno
Copy link
Member

Keno commented Jun 30, 2022

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 fingolfin force-pushed the mh/remove-duplicate-jl_gc_safepoint branch from 1b08e35 to 3cf5e58 Compare July 1, 2022 06:52
This prevents a compiler warning on some systems
@fingolfin fingolfin force-pushed the mh/remove-duplicate-jl_gc_safepoint branch from 3cf5e58 to 17dc921 Compare July 1, 2022 06:59
@fingolfin
Copy link
Contributor Author

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 Keno merged commit 05eb153 into JuliaLang:master Jul 1, 2022
@Keno
Copy link
Member

Keno commented Jul 1, 2022

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.

KristofferC pushed a commit that referenced this pull request Jul 4, 2022
This prevents a compiler warning on some systems

(cherry picked from commit 05eb153)
KristofferC pushed a commit that referenced this pull request Jul 4, 2022
This prevents a compiler warning on some systems

(cherry picked from commit 05eb153)
@KristofferC KristofferC removed the backport 1.8 Change should be backported to release-1.8 label Jul 6, 2022
@fingolfin fingolfin deleted the mh/remove-duplicate-jl_gc_safepoint branch July 12, 2022 08:49
pcjentsch pushed a commit to pcjentsch/julia that referenced this pull request Aug 18, 2022
This prevents a compiler warning on some systems
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.

5 participants