Skip to content

fix #57749, model ccall binding access in inference #58872

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Aug 4, 2025
Merged

fix #57749, model ccall binding access in inference #58872

merged 1 commit into from
Aug 4, 2025

Conversation

JeffBezanson
Copy link
Member

I've played around with ccall semantics a bunch (#57931) but concluded that a bandaid like this is far better for 1.12.

fixes #57749

@JeffBezanson JeffBezanson added this to the 1.12 milestone Jul 1, 2025
@JeffBezanson JeffBezanson added bugfix This change fixes an existing bug backport 1.12 Change should be backported to release-1.12 labels Jul 1, 2025
@topolarity topolarity requested a review from Keno July 1, 2025 23:13
@JeffBezanson
Copy link
Member Author

JeffBezanson commented Jul 2, 2025 •
edited
Loading

So we basically need a custom interpreter here to handle the non-standard IR in ccall names, which I have tried to do but am flailing a bit 😄 It doesn't really need to be "correct" and its value is not used. It just needs to be enough for inference to tell which bindings are accessed.

So far I only have special support for nests of calls, which handles many cases like A.B.C but I wonder if there are any other syntaxes used out there?

JeffBezanson
JeffBezanson commented Jul 3, 2025
View reviewed changes
@KristofferC KristofferC mentioned this pull request Jul 8, 2025
Merged
60 tasks
@Keno Keno added the needs pkgeval Tests for all registered packages should be run with this change label Jul 10, 2025
@Keno
Copy link
Member

Keno commented Jul 10, 2025

One inline comment and I think we need to pkgeval this to see if there's any corner cases, but I think as a workaround, this seems reasonable.

@JeffBezanson JeffBezanson force-pushed the jb/fix57749 branch 2 times, most recently from d4e80a0 to 3e868b2 Compare July 11, 2025 21:43
@JeffBezanson
Copy link
Member Author

@nanosoldier runtests()

@Keno
Copy link
Member

Keno commented Jul 16, 2025

@KristofferC already pkgeval'd this in #58987

@JeffBezanson
Copy link
Member Author

This PR wasn't linked nor the tag removed. In any case that run was not too useful since there were assertion failures from the other PR.

@nanosoldier
Copy link
Collaborator

The package evaluation job you requested has completed - possible new issues were detected.
The full report is available.

Report summary

❗ Packages that crashed

3 packages crashed only on the current version.

  • The process was aborted: 1 packages
  • Invalid LLVM IR was generated: 1 packages
  • GC corruption was detected: 1 packages

3 packages crashed on the previous version too.

✖ Packages that failed

42 packages failed only on the current version.

  • Illegal method overwrites during precompilation: 1 packages
  • Package has test failures: 2 packages
  • Package tests unexpectedly errored: 2 packages
  • Tests became inactive: 1 packages
  • Test duration exceeded the time limit: 21 packages
  • Test log exceeded the size limit: 15 packages

1156 packages failed on the previous version too.

✔ Packages that passed tests

15 packages passed tests only on the current version.

  • Other: 15 packages

5369 packages passed tests on the previous version too.

~ Packages that at least loaded

16 packages successfully loaded only on the current version.

  • Other: 16 packages

3224 packages successfully loaded on the previous version too.

➖ Packages that were skipped altogether

2 packages were skipped only on the current version.

  • Package could not be installed: 2 packages

903 packages were skipped on the previous version too.

@vtjnash
Copy link
Member

vtjnash commented Jul 17, 2025

Looks like up to 15 failures caused by this PR (CustomGaussQuadrature was in the last report as failing from this PR too)

Test log exceeded the size limit: 15 packages
Package	Version	Primary	Against	History (6-16 to 7-15)
Quadmath	v0.5.13	fail	test	▇▇▇▇▇▇▇▇▇▇▇▇▇
HyperDualNumbers	v4.0.10	fail	test	▇▇▇▇▇▇▇▇▇▇▇▇▇
JSOSolvers	v0.14.0	fail	test	▇▇▇▇▇▇▇▇▇▇▇▇▇
Lehmann	v0.2.7	fail	test	▇▇▇▇▇▇▇▇▇▇▇▇▇
GreenFunc	v0.2.6	fail	test	▇▇▇▇▇▇▇▇▇▇▇▇▇
ElectronGas	v0.2.6	fail	test	▇▇▇▇▇▇▇▇▇▇▇▇▇
TensorTrains	v0.12.1	fail	test	▇▇▅▅▇▅▇▅▅▇▅▅▇
IterativeRefinement	v0.2.1	fail	test	▇▇▇▇▇▇▇▇▇▇▇▇▇
HAdaptiveIntegration	v0.2.0	fail	test	▇▇▇▇▇▇▇▇▇▇▇▇▇
JacobiEigen	v1.0.1	fail	test	▇▇▇▇▇▇▇▇▇▇▇▇▇
RationalFunctionApproximation	v0.2.4	fail	test	▇▇▇▇▇▇▇▇▇▇▇▇▇
CaNNOLeS	v0.8.0	fail	test	▇▇▇▇▇▇▇▇▇▇▇▇▇
NEFTInterface	v0.1.0	fail	load	▅▅▅▅▅▅▅▅▅▅▅▅▅
CaratheodoryFejerApprox	v0.1.0	fail	test	▇▇▇▅▅▅▅▅▇▇▇▇▇
CustomGaussQuadrature	v1.0.6	fail	test	▇▇▇▇▇▇▇▇▇▇▇▇

vtjnash
vtjnash reviewed Jul 17, 2025
View reviewed changes
Compiler/src/abstractinterpretation.jl
argtypes[i] = ai[].rt
i += 1
end
res = abstract_call(interp, ArgInfo(e.args, argtypes), sstate, sv)
Copy link
Member

Choose a reason for hiding this comment

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

IIRC, this call will occasionally corrupt the InferenceState structure for the current statement, since this is not actually the current statement (there might be some support code in return_type_tfunc for working around it though)

Copy link
Member

Choose a reason for hiding this comment

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

return_type_tfunc twiddles restrict_abstract_call_sites, but that's just a correctness issue for return_type. The world age side effect that we care about here will still be conservatively correct (though conservatively correct might still mess with out codegen, so not perfect, but better than before). I don't see it doing anything else that would be required here.

@KristofferC KristofferC mentioned this pull request Jul 22, 2025
Merged
20 tasks
@JeffBezanson JeffBezanson force-pushed the jb/fix57749 branch 2 times, most recently from 596efb8 to d2bbd0a Compare July 29, 2025 16:20
@JeffBezanson
Copy link
Member Author

@nanosoldier runtests()

@nanosoldier
Copy link
Collaborator

The package evaluation job you requested has completed - possible new issues were detected.
The full report is available.

Report summary

❗ Packages that crashed

8 packages crashed on the previous version too.

✖ Packages that failed

13 packages failed only on the current version.

  • Package has test failures: 2 packages
  • Package tests unexpectedly errored: 1 packages
  • Networking-related issues were detected: 1 packages
  • Test duration exceeded the time limit: 9 packages

1187 packages failed on the previous version too.

✔ Packages that passed tests

27 packages passed tests only on the current version.

  • Other: 27 packages

5419 packages passed tests on the previous version too.

~ Packages that at least loaded

17 packages successfully loaded only on the current version.

  • Other: 17 packages

3216 packages successfully loaded on the previous version too.

➖ Packages that were skipped altogether

905 packages were skipped on the previous version too.

@JeffBezanson
Copy link
Member Author

Failures look unrelated?

@Keno Keno removed the needs pkgeval Tests for all registered packages should be run with this change label Aug 1, 2025
@topolarity topolarity added the merge me PR is reviewed. Merge when all tests are passing label Aug 1, 2025
@JeffBezanson JeffBezanson merged commit b35c4f4 into master Aug 4, 2025
5 of 7 checks passed
@JeffBezanson JeffBezanson deleted the jb/fix57749 branch August 4, 2025 21:07
KristofferC pushed a commit that referenced this pull request Aug 6, 2025
@JeffBezanson @KristofferC
model ccall binding access in inference (#58872)
41aa62e 
fixes #57749

(cherry picked from commit b35c4f4)
@KristofferC KristofferC mentioned this pull request Aug 6, 2025
Open
38 tasks
@giordano giordano removed the merge me PR is reviewed. Merge when all tests are passing label Aug 11, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vtjnash vtjnash vtjnash left review comments

@Keno Keno Keno approved these changes

Assignees
No one assigned
Labels
backport 1.12 Change should be backported to release-1.12 bugfix This change fixes an existing bug
Projects
None yet
Milestone
1.12
Development

Successfully merging this pull request may close these issues.

ccall sometimes fails to statically evaluate arguments due to binding ages
6 participants
@JeffBezanson @Keno @nanosoldier @vtjnash @giordano @topolarity