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

allow external absint to hold custom data in codeinst.inferred #53300

Merged
merged 2 commits into from
Feb 17, 2024

Conversation

aviatesk
Copy link
Sponsor Member

It has been possible for external abstract interpreters to keep custom data in codeinst.inferred (together /w overloading inlining_policy). After #52233, when such external absint uses InternalCodeCache, this data is passed to jl_ir_flag_inferred, leading to segfaults in assertion builds.

This commit resolves the issue by omitting jl_ir_flag_inferred checks when the cache_owner is external. Nonetheless, a better resolution might be necessary. It suggests that the current design of code_owner and InternalCodeCache for the external cache system is somewhat flawed. A conceivable approach could involve:

  • Adding a layer similar to inlining_policy in CC.get(::WorldView{InternalCodeCache}) to enable safe redirection of custom data to the native interpreter's implementation.
  • Prohibiting custom data in the inferred field and directing such data to be kept in analysis_results.

@vchuravy
Copy link
Sponsor Member

Does #53219 resolve this? It removes the need to handle code-instances in this location.

#52964 does require that code.inferred is something codegen and the interpreter can understand, so stashing additional data in .inferred is problematic and I would prefer custom data to go into analysis_results.

base/compiler/types.jl Outdated Show resolved Hide resolved
@vchuravy
Copy link
Sponsor Member

In JuliaDebug/Cthulhu.jl#540 I continued using the external code cache instead of using the internal code cache (but the external code cache no longer participates in invalidation logic)

@aviatesk
Copy link
Sponsor Member Author

Does #53219 resolve this? It removes the need to handle code-instances in this location.

Probably. I'm looking into it.

In JuliaDebug/Cthulhu.jl#540 I continued using the external code cache instead of using the internal code cache (but the external code cache no longer participates in invalidation logic)

Cthulhu has traditionally created a new cache for each instance of CthulhuInterpreter, which meant there was no inherent support for invalidation required. On a personal note, I'm inclined to retain it as it facilitates easier reflection of outcomes following modifications to the base abstract interpretation's implementation.

@Keno
Copy link
Member

Keno commented Feb 12, 2024

I think it's fine to have non-CodeInfo objects in (::CodeInstance).inferred. That's an explicit goal of #53219. #53219 also removes the (::CodeInfo).inferred field, which fixes the issue in this PR. It also fixes the underlying issue where the compiler is looking at the wrong CodeInstance in some cases. The only constraint we do need is that for owner==nothing, the CodeInstance's inferred field needs to be something the compiler understands.

Copy link
Sponsor Member

@vtjnash vtjnash left a comment

Choose a reason for hiding this comment

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

Waiting on resolution of #53219, since that already addresses the .inferred field issue here, and then this will need to be rebased

@aviatesk
Copy link
Sponsor Member Author

Yeah I'll work on the PR first and then we only need the test cases from this PR.

@aviatesk
Copy link
Sponsor Member Author

I know it might sound like I'm flip-flopping, but I want to move forward with this PR as is. This is because, as things stand post-#52233, virtually all external abstract interpreters utilizing the external code cache with custom data are rendered inoperative on assertion builds. Given that the code in question will be eliminated by #53219 anyway in the future, I think this PR doesn't make any harm and merging this for now alone can be justified with the added test cases.

aviatesk added a commit to aviatesk/JET.jl that referenced this pull request Feb 13, 2024
Leverages JuliaLang/julia#52233 to use the internal code cache that
comes with the inherent invalidation support.

Still requires:
- JuliaLang/julia#53300 (or JuliaLang/julia#53219)
- JuliaLang/julia#53318
aviatesk added a commit to aviatesk/JET.jl that referenced this pull request Feb 13, 2024
Leverages JuliaLang/julia#52233 to use the internal code cache that
comes with the inherent invalidation support.

Still requires:
- JuliaLang/julia#53300 (or JuliaLang/julia#53219)
- JuliaLang/julia#53318
Copy link
Sponsor Member

@vchuravy vchuravy left a comment

Choose a reason for hiding this comment

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

There are currently more places that make assumptions about the content of this field.

If the relocatability flag on the codeinstance is 0x0 we do not safe it as part of a package image. This is determined here

relocatability = 0x0
if const_flags == 0x3 && may_discard_trees(interp)
inferred_result = nothing
relocatability = 0x1
else
inferred_result = transform_result_for_cache(interp, result.linfo, valid_worlds, result)
if isa(inferred_result, String)
t = @_gc_preserve_begin inferred_result
relocatability = unsafe_load(unsafe_convert(Ptr{UInt8}, inferred_result), Core.sizeof(inferred_result))
@_gc_preserve_end t
elseif inferred_result === nothing
relocatability = 0x1
end
end
# relocatability = isa(inferred_result, String) ? inferred_result[end] : UInt8(0)

and checked here
if (!ci->relocatability)

Even if I force relocatability to 0x1 we hit an assertion later

assert(jl_is_string(data));
from
jl_ir_flag_inferred(inferred) &&

While we could thread everything through this does raise the question that we store inferred normally in a compressed format, using jl_compress_ir which of course doesn't know how to handle custom results.

For me this indicates that using analysis_results over additional data in inferred should be preferred. Or we need to figure out how to compress custom data.

aviatesk added a commit to aviatesk/JET.jl that referenced this pull request Feb 14, 2024
Leverages JuliaLang/julia#52233 to use the internal code cache that
comes with the inherent invalidation support.

Still requires:
- JuliaLang/julia#53300 (or JuliaLang/julia#53219)
- JuliaLang/julia#53318
aviatesk added a commit to aviatesk/JET.jl that referenced this pull request Feb 14, 2024
Leverages JuliaLang/julia#52233 to use the internal code cache that
comes with the inherent invalidation support.

Still requires:
- JuliaLang/julia#53300 (or JuliaLang/julia#53219)
- JuliaLang/julia#53318
@aviatesk aviatesk force-pushed the avi/ext-interp-custom-data branch 2 times, most recently from 5bd972a to 31bebd3 Compare February 15, 2024 14:22
It has been possible for external abstract interpreters to keep custom
data in `codeinst.inferred` (together /w overloading `inlining_policy`).
After #52233, when such external absint uses
`InternalCodeCache`, this data is passed to `jl_ir_flag_inferred`,
leading to segfaults in assertion builds.

This commit resolves the issue by omitting `jl_ir_flag_inferred` checks
when the `cache_owner` is external. Nonetheless, a better resolution
might be necessary. It suggests that the current design of `code_owner`
and `InternalCodeCache` for the external cache system is somewhat flawed.
A conceivable approach could involve:
- Adding a layer similar to `inlining_policy` in `CC.get(::WorldView{InternalCodeCache})`
  to enable safe redirection of custom data to the native interpreter's
  implementation.
- Prohibiting custom data in the `inferred` field and directing such
  data to be kept in `analysis_results`.
@vchuravy vchuravy added this to the 1.11 milestone Feb 16, 2024
@vchuravy vchuravy added the backport 1.11 Change should be backported to release-1.11 label Feb 16, 2024
Copy link
Sponsor Member

@vchuravy vchuravy left a comment

Choose a reason for hiding this comment

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

I am still not a big fan of using .inferred for these purposes,
but I am okay with this change to unblock current uses of AbstractInterpreter.

@aviatesk do you want to add an explicit serialization test, where you override relocatability?

@vtjnash
Copy link
Sponsor Member

vtjnash commented Feb 17, 2024

Yeah, since jl_ir_flag_inferred is deleted in #53219 it may conflict somewhat with that PR content, but seems safe to merge this now as it is mostly just added tests

@vtjnash vtjnash merged commit 93876c9 into master Feb 17, 2024
8 checks passed
@vtjnash vtjnash deleted the avi/ext-interp-custom-data branch February 17, 2024 17:41
@vtjnash
Copy link
Sponsor Member

vtjnash commented Feb 17, 2024

To address Valentin's comment: it looks like this doesn't really express an opinion specifically on whether it is acceptable to use this field, but simply attempts to address the possibility of a missing type assert, which would be valid to check there regardless of whether we later state that other content should be put here

Thinking more broadly, I wonder if we should restructure this layering of types to instead look like this, so that there is a more bright line distinction in the types what is volatile and what is constant:

struct MethodInstance #= mostly unchanged =# end

mutable struct CodeCache # renamed from CodeInstance to indicate this is volatile
    parent::MethodInstance # for show reflection
    #= most fields from existing CodeInstance =#
   ...
    @atomic inferred::Union{CodeInfo, CompressedCodeInfo}
end

struct CodeInfo # partially kept as-is, but extended to allow external stmt representations
    #= fields from existing CodeInstance that are properties of the whole function =#
    ...
    parent::MethodInstance # for show reflection
    stmts::Union{StmtInfo, OpaqueExternalObjects, Nothing} # custom info goes here
end

struct StmtInfo # split from CodeInfo
    #= fields from existing CodeInstance that are arrays of per stmt info =#
   ...
end

KristofferC pushed a commit that referenced this pull request Feb 26, 2024
)

It has been possible for external abstract interpreters to keep custom
data in `codeinst.inferred` (together /w overloading `inlining_policy`).
After #52233, when such external absint uses
`InternalCodeCache`, this data is passed to `jl_ir_flag_inferred`,
leading to segfaults in assertion builds.

This commit resolves the issue by omitting `jl_ir_flag_inferred` checks
when the `cache_owner` is external. Nonetheless, a better resolution
might be necessary. It suggests that the current design of `code_owner`
and `InternalCodeCache` for the external cache system is somewhat
flawed. A conceivable approach could involve:
- Adding a layer similar to `inlining_policy` in
`CC.get(::WorldView{InternalCodeCache})` to enable safe redirection of
custom data to the native interpreter's implementation.
- Prohibiting custom data in the `inferred` field and directing such
data to be kept in `analysis_results`.

(cherry picked from commit 93876c9)
@KristofferC KristofferC mentioned this pull request Feb 26, 2024
28 tasks
aviatesk added a commit that referenced this pull request Feb 26, 2024
While experimenting with precompilation for external absints on builds
just after #53300 was merged, I found that the test case for
`CustomAbstractInterpreterCaching2.jl` fails if the test case for
`CustomAbstractInterpreterCaching1.jl` isn't run in the same session
beforehand. That is probably because of the previous lack of support
for proper `CodeInstance` caching. To address this, I've changed the
tests to run in separate processes in this commit. Note that it appears
that a recent refactor concerning `CodeInstance` might have resolved
this issue, so the new test cases runs successfully on master. However,
I suspect the fix hasn't been applied to v1.11 yet, we would need more
research.
aviatesk added a commit that referenced this pull request Feb 26, 2024
While experimenting with precompilation for external absints on builds
just after #53300 was merged, I found that the test case for
`CustomAbstractInterpreterCaching2.jl` fails if the test case for
`CustomAbstractInterpreterCaching1.jl` isn't run in the same session
beforehand. That is probably because of the previous lack of support
for proper `CodeInstance` caching. To address this, I've changed the
tests to run in separate processes in this commit. Note that it appears
that a recent refactor concerning `CodeInstance` might have resolved
this issue, so the new test cases runs successfully on master. However,
I suspect the fix hasn't been applied to v1.11 yet, we would need more
research.
aviatesk added a commit that referenced this pull request Feb 27, 2024
While experimenting with precompilation for external absints on builds
just after #53300 was merged, I found that the test case for
`CustomAbstractInterpreterCaching2.jl` fails if the test case for
`CustomAbstractInterpreterCaching1.jl` isn't run in the same session
beforehand. That is probably because of the previous lack of support
for proper `CodeInstance` caching. To address this, I've changed the
tests to run in separate processes in this commit. Note that it appears
that a recent refactor concerning `CodeInstance` might have resolved
this issue, so the new test cases runs successfully on master. However,
I suspect the fix hasn't been applied to v1.11 yet, we would need more
research.
aviatesk added a commit that referenced this pull request Feb 27, 2024
As mentioned in #53478, the precompilation support for external abstract
interpreters in v1.11 isn't perfect, and directly cherry-picking the
refined test cases from #53478 into the v1.11 backport branch leads to
a test failure (note that this particular problem has likely been fixed
in the master branch, probably thanks to #53300). To address this, this
commit does more than just cherry-pick the test case, and it also
modifies the `CodeInstance(::AbstractInterpreter, ::InferenceResult)`
constructor to allow precompilation for external abstract interpreters
in v1.11.
KristofferC added a commit that referenced this pull request Mar 1, 2024
Backported PRs:
- [x] #53361 <!-- 🤖 [master] Bump the SparseArrays stdlib from c9f7293
to cb602d7 -->
- [x] #53300 <!-- allow external absint to hold custom data in
`codeinst.inferred` -->
- [x] #53342 <!-- Add `Base.wrap` to docs -->
- [x] #53372 <!-- Silence warnings in `test/file.jl` -->
- [x] #53357 <!-- 🤖 [master] Bump the Pkg stdlib from 6dd0e7c9e to
76070d295 -->
- [x] #53373 <!-- fix sysimage-native-code=no option with pkgimages -->
- [x] #53333 <!-- More consistent return value for annotations, take 2
-->
- [x] #53354 <!-- fix code coverage bug in tail position and `else` -->
- [x] #53407 <!-- fix sysimage-native-code=yes option -->
- [x] #53388 <!-- Fix documentation: thread pool of main thread -->
- [x] #53355 <!-- Fix synchronization issues on the GC scheduler. -->
- [x] #53429 <!-- Subtype: skip slow-path in `local_∀_∃_subtype` if
inputs contain no ∃ typevar. -->
- [x] #53437 <!-- Add debug variant of loader_trampolines.o -->
- [x] #53284 <!-- Add annotate! method for AnnotatedIOBuffer -->
- [x] #53466 <!-- [MozillaCACerts_jll] Update to v2023-12-12 -->
- [x] #53467 <!-- [LibGit2_jll] Update to v1.7.2 -->
- [x] #53326 <!-- RFC: when loading code for internal purposes, load
stdlib files directly, bypassing DEPOT_PATH, LOAD_PATH, and stale checks
-->
- [x] #53332
- [x] #53320 <!-- Add `Sys.isreadable, Sys.iswriteable`, update `ispath`
-->
- [x] #53476

Contains multiple commits, manual intervention needed:
- [ ] #53285 <!-- Add update mechanism for Terminfo, and common
user-alias data -->

Non-merged PRs with backport label:
- [ ] #53424 <!-- yet more atomics & cache-line fixes on work-stealing
queue -->
- [ ] #53408 <!-- task splitting: change additive accumulation to
multiplicative -->
- [ ] #53403 <!-- Move parallel precompilation to Base -->
- [ ] #53402 <!-- Add `jl_getaffinity` and `jl_setaffinity` -->
- [ ] #53391 <!-- Default to the medium code model in x86 linux -->
- [ ] #53125 <!-- coverage: count coverage where explicitly requested by
inference only -->
- [ ] #52694 <!-- Reinstate similar for AbstractQ for backward
compatibility -->
@KristofferC KristofferC removed the backport 1.11 Change should be backported to release-1.11 label Mar 1, 2024
tecosaur pushed a commit to tecosaur/julia that referenced this pull request Mar 4, 2024
…iaLang#53300)

It has been possible for external abstract interpreters to keep custom
data in `codeinst.inferred` (together /w overloading `inlining_policy`).
After JuliaLang#52233, when such external absint uses
`InternalCodeCache`, this data is passed to `jl_ir_flag_inferred`,
leading to segfaults in assertion builds.

This commit resolves the issue by omitting `jl_ir_flag_inferred` checks
when the `cache_owner` is external. Nonetheless, a better resolution
might be necessary. It suggests that the current design of `code_owner`
and `InternalCodeCache` for the external cache system is somewhat
flawed. A conceivable approach could involve:
- Adding a layer similar to `inlining_policy` in
`CC.get(::WorldView{InternalCodeCache})` to enable safe redirection of
custom data to the native interpreter's implementation.
- Prohibiting custom data in the `inferred` field and directing such
data to be kept in `analysis_results`.
aviatesk added a commit that referenced this pull request Mar 14, 2024
While experimenting with precompilation for external absints on builds
just after #53300 was merged, I found that the test case for
`CustomAbstractInterpreterCaching2.jl` fails if the test case for
`CustomAbstractInterpreterCaching1.jl` isn't run in the same session
beforehand. That is probably because of the previous lack of support
for proper `CodeInstance` caching. To address this, I've changed the
tests to run in separate processes in this commit. Note that it appears
that a recent refactor concerning `CodeInstance` might have resolved
this issue, so the new test cases runs successfully on master. However,
I suspect the fix hasn't been applied to v1.11 yet, we would need more
research.
aviatesk added a commit that referenced this pull request Mar 14, 2024
As mentioned in #53478, the precompilation support for external abstract
interpreters in v1.11 isn't perfect, and directly cherry-picking the
refined test cases from #53478 into the v1.11 backport branch leads to
a test failure (note that this particular problem has likely been fixed
in the master branch, probably thanks to #53300). To address this, this
commit does more than just cherry-pick the test case, and it also
modifies the `CodeInstance(::AbstractInterpreter, ::InferenceResult)`
constructor to allow precompilation for external abstract interpreters
in v1.11.
aviatesk added a commit that referenced this pull request Mar 15, 2024
)

While experimenting with precompilation for external absints on builds
just after #53300 was merged, I found that the test case for
`CustomAbstractInterpreterCaching2.jl` fails if the test case for
`CustomAbstractInterpreterCaching1.jl` isn't run in the same session
beforehand. That is probably because of the previous lack of support for
proper `CodeInstance` caching. To address this, I've changed the tests
to run in separate processes in this commit. Note that it appears that a
recent refactor concerning `CodeInstance` might have resolved this
issue, so the new test cases runs successfully on master. However, I
suspect the fix hasn't been applied to v1.11 yet, we would need more
research.
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.

None yet

5 participants