ccall: make distinction of pointer vs name a syntactic distinction #59165
Conversation
vtjnash
added
minor change
|
Can we specify that the variable must be a constant binding (in the evaluation world) and have it participate in the invalidation? I don't know that I like the time traveling binding behavior. Seems like a nice change otherwise though. |
vtjnash
added
the
needs pkgeval
|
No, far too much code (all JLLs before they are recompiled to be lazy) relies upon the dynamic behavior of allowing a non-constant binding to evaluate at runtime and cache. Basically, I don't consider this to be time traveling anymore either, but rather that it is intended to express equivalent behavior to there existing a separate |
|
Ok. Perhaps we could still invalidate on binding replacement though? I don't think that'd be much harder to implement, and gets us 90% of the way there (and non-const could be disallowed in the future or in strict mode). |
|
I'm still not sure I quite follow the request. Are you suggesting we add a semantic dependence on data, or simply best-effort? I think we already will do a best effort attempt which should happen in most cases (particularly after Jeff's PR, and other related corrections to trimming) |
|
My request is to make that invalidation a semantic guarantee by making the completely generic fallback path record the world age it was created in and then checking if the latest binding partition covers that world age. |
topolarity
changed the title
|
Thanks for taking the initiative! This feels like a much more pragmatic (and complete) solution to #57931 than anything else we've implemented so far. I think I prefer this to the more dynamic options proposed in #57931, esp. since those only covered the case of a dynamically-chosen library
Why does this make static compilation impossible? It doesn't seem like, e.g., the behavior proposed in solution (5) in #57931 is impossible to statically compile, assuming it is type-stable (but not necessarily value-stable) - but maybe you were referring just to the particular implementation in that PR? |
It seemed mainly that we couldn't quite define whether it was a static name expression or a dynamic pointer so it couldn't be quite linearized so it was sort of an embedded |
| @@ -151,7 +151,7 @@ using ..GMP: BigInt, Limb, BITS_PER_LIMB, libgmp | |||
| const mpz_t = Ref{BigInt} | |||
| const bitcnt_t = Culong | |||
|
|
|||
| gmpz(op::Symbol) = (Symbol(:__gmpz_, op), libgmp) | |||
| gmpz(op::Symbol) = Expr(:tuple, QuoteNode(Symbol(:__gmpz_, op)), GlobalRef(MPZ, :libgmp)) | |||
There was a problem hiding this comment.
I wouldn't be surprised if some other packages do this. We might need to allow it.
There was a problem hiding this comment.
Yes, we might, but until PkgEval results finish, we won't know if we need to add yet more cases or are fine with what we already have covering all uses. It is fairly easy to add (it is the same as the syntactic string case), but we won't know if it worth documenting yet another optional form of the same thing until we finish the test run.
| if (nargs_tuple == 0) | ||
| jl_error("ccall function name cannot be empty tuple"); | ||
| if (nargs_tuple > 2) | ||
| jl_error("ccall function name tuple can have at most 2 elements"); |
There was a problem hiding this comment.
I think these kinds of syntax validity checks should be done by lowering.
There was a problem hiding this comment.
These are value checks (it is preparing to call jl_toplevel_eval), so they are not straightforward to have duplicated in both of our lowering implementations now
|
I'm not sure we should do this. I'm willing to hold out for a version that eliminates |
|
This version does eliminate it semantically, though I didn't quite remove it, it only gets trivial cases now and only as an optimization (to peek at whether the expression is constant before emitting a branch to contain it). It is still used by cglobal, which I left as followup work. |
We have long expected users to be explicit about the library name for `ccall`, and the `@ccall` macro has even always enforced that. That means users should have already been using explicit syntax. And indeed, other syntax forms weren't handled reliably (since doing so would require linearizing IR if and only if the runtime values required it, which is not something that is computable, and thus was often done wrong). This now aligns the runtime and compiler to expect only syntax forms that we could reliably handle before without errors, and adds explicit errors for cases that we previously knew would be unreliable because they relied upon inference making particular decisions for the semantics. The `ccall` function is already very special since it is more like a actual macro (it does not exist as a binding or value), so we can make unusual syntax decisions like this, mirroring `@ccall`. This drops support for #37123, since we were going to use that for LazyLibraries, be we decided that approach was quite buggy and that PR would make static compilation quite impossible to support, so we instead actually implemented LazyLibraries with a different approach. It could be re-enabled, but we never had correct lowering or inference support for it, so it is presumably still unused. The goal is to cause breakage only where the package authors really failed to express intent with syntax, and otherwise to explicitly maintain support by adding cases to normalize the given syntax into one of the supported cases. All existing functionality (and more) can be accessed by explicit management of a pointer or by a LazyLibrary-like type, so this shouldn't cause any reduction in possible functionality, just possibly altered syntax.
vtjnash
force-pushed
the
jn/syntax-verified-ccall
branch
from
8aed4d7 to
cce193b
Compare
|
Unsurprisingly, looks like JuliaInterpreter and friends need some adjusting: https://buildkite.com/julialang/julia-master/builds/49600#01986719-957f-4f8a-9636-9470d50ae64b/836-1055 |
|
Yes, it looks like given the choices among possible buggy reimplementations of the ill-defined |
|
@nanosoldier |
|
The package evaluation job you requested has completed - possible new issues were detected. Report summary❗ Packages that crashed5 packages crashed on the previous version too. ✖ Packages that failed988 packages failed only on the current version.
1192 packages failed on the previous version too. ✔ Packages that passed tests12 packages passed tests only on the current version.
4832 packages passed tests on the previous version too. ~ Packages that at least loaded3 packages successfully loaded only on the current version.
2857 packages successfully loaded on the previous version too. ➖ Packages that were skipped altogether6 packages were skipped only on the current version.
904 packages were skipped on the previous version too. |
|
It looks like a majority of those were due to a recent breaking change in FFTW (JuliaMath/FFTW.jl#207) which could just be partly reverted it seems (it had been in preparation for LazyLibraries, but then we went a different way with implementation of those, so now this should be reverted to actually support them) |
We have long expected users to be explicit about the library name for
ccall, and the@ccallmacro has even always enforced that. That means users should have already been using explicit syntax, even though it wasn't strictly enforced. And indeed, the other syntax forms weren't handled reliably anyways (since doing so would require linearizing IR if and only if the runtime values required it, which is not something that is computable, and thus was often done wrong). This now intends to align the runtime and compiler to expect only those syntax forms that we could reliably handle in the past without errors, and adds explicit errors for other cases, most of which we previously knew would be unreliable due to reliance upon inference in making particular decisions for the semantics. Theccallfunction is already very special since it is more like a actual macro (it does not exist as a binding or value), so we can make unusual syntax decisions like this, mirroring@ccallalso.This fixes #57931, mostly by restricting the set of things that are allowed to the set of things that have an obvious and pre-existing behavior to be guaranteed to do that behavior. The hope is to PkgEval this to check if any packages are doing something unusual and see if we even need to allow anything else.
This drops support for #37123, since we were going to use that for LazyLibraries, be we decided that approach was quite buggy and that PR would make static compilation quite impossible to support, so we instead actually implemented LazyLibraries with a different approach. It could be re-enabled, but we never had correct lowering or inference support for it, so it is presumably still unused.
The goal is to cause breakage only where the package authors really failed to express intent with syntax, and otherwise to explicitly maintain support by adding cases to normalize the given syntax into one of the supported cases. All existing functionality (and more) can be accessed by explicit management of a pointer or by a LazyLibrary-like type, so this shouldn't cause any reduction in possible functionality, just possibly altered syntax.
In followup work, we may want to apply some similar improvements to the cglobal semantics, though not nearly as urgent, since cglobal on a pointer is permitted as a complicated bitcast right now, which doesn't make sense and probably should be deprecated, so that there is no ambiguity with that function's meaning.
Mostly written by Claude