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

impose stricter and earlier validation on ccall/cfunction types #43953

Merged
merged 2 commits into from
Feb 9, 2022

Conversation

vtjnash
Copy link
Sponsor Member

@vtjnash vtjnash commented Jan 27, 2022

Closes #43636

@vtjnash vtjnash requested a review from NHDaly January 27, 2022 21:05
Copy link
Member

@NHDaly NHDaly left a comment

Choose a reason for hiding this comment

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

Cool, thanks Jameson! The changes seem sane to me, although i'm pretty unfamiliar with this code. The tests definitely seem reasonable. Thanks for making the change! 👍

@oscardssmith oscardssmith added the domain:ffi foreign function interfaces, ccall, etc. label Jan 31, 2022
@vtjnash vtjnash merged commit a84a058 into master Feb 9, 2022
@vtjnash vtjnash deleted the jn/43636 branch February 9, 2022 19:50
fingolfin added a commit to oscar-system/GAP.jl that referenced this pull request Feb 14, 2022
The validation for ccall arguments was tightened, see
JuliaLang/julia#43953
fingolfin added a commit to oscar-system/GAP.jl that referenced this pull request Feb 14, 2022
The validation for ccall arguments was tightened, see
JuliaLang/julia#43953
@fingolfin
Copy link
Contributor

This seems to have broken several packages that worked well so far. I am just wondering if that's intentional? From reading the issue, it sounded to me as if this was mostly there to fix things that would have crashed before, but in our case, we had a ccall with return value Function, and it worked perfectly fine so far. Now it is rejected. Is that as intended? If so, perhaps it deserves a mention in some NEWS item or whatever, because it'll break existing working code?

@vtjnash
Copy link
Sponsor Member Author

vtjnash commented Feb 14, 2022

Function seems like a mildly odd choice for a return type there, where using Any agrees better with the ccall argument used to create it. You are right that it was intentional, and that it is more strict now than necessary. We have had the alternative also of Ref{Function} for a long time for promising "this returns a valid julia pointer to something that I promise is a subtype of Function".

yuyichao added a commit to yuyichao/FunctionWrappers.jl that referenced this pull request Mar 28, 2022
@andreasnoack
Copy link
Member

I've bisected this PR as the cause of an error similar to https://discourse.julialang.org/t/package-compiler-errors-with-system-image-too-large-message/85849. With version 1.7.x of Julia, our system image is about 1.1GB but after this PR, it fails with the system image too large error.

@LilithHafner LilithHafner added the kind:breaking This change will break code label Mar 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
domain:ffi foreign function interfaces, ccall, etc. kind:breaking This change will break code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Segfault in LLVM when attempting to compile a ccall that returns Core.MethodInstance
6 participants