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.

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

staticdata: fix assert from partially disabled native code #53439

Merged
merged 1 commit into from
Feb 23, 2024
Merged

staticdata: fix assert from partially disabled native code #53439

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions src/staticdata.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3768,8 +3768,11 @@ JL_DLLEXPORT jl_value_t *jl_restore_package_image_from_file(const char *fname, j

jl_image_t pkgimage = jl_init_processor_pkgimg(pkgimg_handle);

if (ignore_native){
memset(&pkgimage.fptrs, 0, sizeof(pkgimage.fptrs));
if (ignore_native) {
Copy link
Member

Choose a reason for hiding this comment

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

Should we just remove ignore_native and move this instead into _tryrequire_from_serialized here

julia/base/loading.jl

Line 1805 in a96726b

# then load the file

// Must disable using native code in possible downstream users of this code:
// https://github.com/JuliaLang/julia/pull/52123#issuecomment-1959965395.
// The easiest way to do that is to disable it in all of them.
jl_options.use_sysimage_native_code = JL_OPTIONS_USE_SYSIMAGE_NATIVE_CODE_NO;
Comment on lines +3774 to +3775
Copy link
Member

Choose a reason for hiding this comment

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

Don't let me hold this up, but I don't quite understand what this does and how setting a sysimage flag relates to pkgimage usage.. Will this still allow pkgimages to be used by any packages, most importantly stdlibs like Pkg which are dead slow without pkgimages..

Copy link
Member

Choose a reason for hiding this comment

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

This essentially turns off native code for all images loaded after this moment (including the current one)

In #53373 this option was fixed for pkgimages (where when the sysimage is loaded without native code, pkgimages can use native code either)

IIUC we load the sysimg and all dependency with native code enabled and once we see the first cache file with ignore_native all subsequent cache files can't use native code, since they might point to images that were loaded without native code.

We could be less discriminate if we could prove that there is no overlap in the cached code.

As an example of a slightly problematic issue it is now order dependent if we use native code or not.

using Test # with native code
using MyPackage # disables native code

But

using MyPackage # disables native code
using Test # without native code

Since we can't know that a code instance is not duplicated between MyPackage and Test, and it depends on which one we load first.

So this generally means that we might ignore the native code of test dependencies, but we will use the native code of actual dependencies.

}

jl_value_t* mod = jl_restore_incremental_from_buf(pkgimg_handle, pkgimg_data, &pkgimage, *plen, depmods, completeinfo, pkgname, 0);
Expand Down