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 |
| 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. |
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) |
…rary improvements (#318) We had initially planned a breaking change design for LazyLibrary, which this package was updated for, but later we realized a better design which allows going back to the original version of this code. The previous form is still very poorly supported in the compiler and was still rather buggy internally. Revert "Replace `__init__` with `OncePerProcess` on supported Julia versions (#316)" This reverts commit dbcb5d2. Revert "Make FFTW relocatable for PackageCompiler" This reverts commit e96270a. Refs: [ccall: make distinction of pointer vs name a syntactic distinction](JuliaLang/julia#59165)
|
@nanosoldier |
|
The package evaluation job you requested has completed - possible new issues were detected. Report summary✖ Packages that failed24 packages failed only on the current version.
74 packages failed on the previous version too. ✔ Packages that passed tests1 packages passed tests only on the current version.
➖ Packages that were skipped altogether4 packages were skipped on the previous version too. |
vtjnash
force-pushed
the
jn/syntax-verified-ccall
branch
from
b251dac to
159a24f
Compare
|
Is there a reason this needs backporting? |
|
@nanosoldier |
|
The package evaluation job you requested has completed - possible new issues were detected. Report summary✖ Packages that failed15 packages failed only on the current version.
9 packages failed on the previous version too. |
|
Good. Now only the desired (ambiguous or unexpected method call) failures. And about half are just from having a dependency on a failing package (e.g. because they pinned to an old version of LinearSolve), so fixing those half will fix the rest. |
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
159a24f to
8441383
Compare
|
@vtjnash Could you give any pointer how to fix the nanosoldier failures? I am currently looking into GAP.jl, which, according to the log, fails around https://github.com/oscar-system/GAP.jl/blob/f50b8d0473cf13f196a9a90507614215692dc772/src/ccalls.jl#L21 cc @fingolfin |
|
Function calls (JuliaInterface_path()) was an experimental feature we introduced a couple years ago, but it never really worked correctly, and then we replaced it with LazyLibraries (overloads of Libdl.dlopen). So switch to LazyLibraries (or roll your own with dlopen overloads on your own lazy library like type). I can send along a PR if you remind me next week. |
|
Since this is marked as a "minor change" and it seems to break some packages in practice (like CxxWrap and a few other ones mentioned in #59165 (comment)), I don't think this should be backported to 1.12. cc @KristofferC |
oscardssmith
removed
the
backport 1.12
|
So, wait... are you saying this PR now broke CxxWrap, and the 50+ packages depending on it, without any warning there (e.g. at least an issue that something has to be changed, with some hint as to what)? That seems... suboptimal? |
|
PkgEval only identified a bug in the CxxWrap tests (https://github.com/JuliaInterop/CxxWrap.jl/blob/ece5014a34f2c9434ad714ebf7e7fa2238c523ed/test/functions.jl#L246) and not in the package itself (so no dependencies were noted in PkgEval runs) |
Huh, what? So looking at the Julia manual, it says:
Is this the same feature we are talking about? Because the one I am quoting describes pretty much what we use in GAP.jl as far as I can tell, and I don't see any references about it being "experimental". What am I missing? |
|
Yes, it wasn't intended to be experimental, but it never worked correctly, and only a couple packages started attempting to use it (GAP being the main one). We replaced it almost immediately after attempting to introduce it with LazyLibraries, but hadn't fixed up any of the old code to error reliably, so it would just crash sometimes instead. This just makes the error path reliable. |
|
You didn't need this whole PR, and shouldn't have ever really needed it, you just needed to remove the |
For GAP.jl, I already have a draft PR at oscar-system/GAP.jl#1267 where it would be great to hear your feedback. |
Well, it did work well for GAP, at least. And while you are right, working code could have been had easier, this was based on measuring code performance. Anyway, we'll deal with it somehow. BTW at the top you wrote
which I don't understand, because we have been and are using But what riles me up about this is the way it was handled: a feature was added years ago to Julia, and documented, with no hint of it being experimental. Then apparently issues were found and it now is being yanked. This is a breaking change. While I understand it may be necessary, I also think such a thing should then always be communicated very clearly, in particular:
So, again: I can live with this change, but I really think this kind of communication breakdown is a good example for why Julia sometimes gets a bad rep on HackerNews :-(. Foremost, I'd suggest to avoid any hints or implications that it is somehow the user's fault for using this supposed "experimental" feature, which was actually documented as a regular feature, or blame for using a barely documented supposed "replacement" feature. |
Introduced in JuliaLang/julia#59165
|
I hesitated to mention it too much in NEWS, since it was just a couple packages that had started to use it, so it wasn't broadly applicable to all audiences. I apologize for not being more proactive to let you guys know what changes to expect. I didn't realize LazyLibraries hadn't gotten more than a passing mention in the documentation. Hopefully this should help address that deficiency: #59841 |
Introduced in JuliaLang/julia#59165
8 participants
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