Use pkgimages for coverage & malloc tracking by ignoring native code in tracked packages #52123

IanButterworth · 2023-11-11T04:09:52Z

Alternative to #48183

This enables and defaults to using pkgimage caches for code-coverage & malloc tracking by ignoring the native code in the pkgimage, so that code must be JIT compiled, at which point the code tracking mechanism will be compiled into the native code.

This is controlled specifically at the package level, rather than globally, meaning that if the --code-coverage=@path approach is used (which is the default for Pkg.test), pkgimage cached native code will be used for all packages outside of path, meaning faster stdlibs in CI.

Benefits of this approach

We don't need to ship --pkgimage=no stdlib caches (faster julia builds).
Ecosystem CI will be running in a closer to default state with pkgimages being generated

Based on a suggestion by @vchuravy

Waiting for:

Make c func abspath consistent on Windows. Fix tracking path conversion. #52140

src/init.c

IanButterworth · 2023-11-11T15:46:00Z

src/codegen.cpp

+    // If a pkgimage or sysimage is being generated, disable tracking.
+    // This means sysimage build or pkgimage precompilation workloads aren't tracked.
    auto do_coverage = [&] (bool in_user_code, bool is_tracked) {
-        return (coverage_mode == JL_LOG_ALL ||
+        return (jl_generating_output() == 0 &&
+                (coverage_mode == JL_LOG_ALL ||
                (in_user_code && coverage_mode == JL_LOG_USER) ||
-                (is_tracked && coverage_mode == JL_LOG_PATH));
+                (is_tracked && coverage_mode == JL_LOG_PATH)));
    };
    auto do_malloc_log = [&] (bool in_user_code, bool is_tracked) {
-        return (malloc_log_mode == JL_LOG_ALL ||
+        return (jl_generating_output() == 0 &&
+                (malloc_log_mode == JL_LOG_ALL ||
                (in_user_code && malloc_log_mode == JL_LOG_USER) ||
-                (is_tracked && malloc_log_mode == JL_LOG_PATH));
+                (is_tracked && malloc_log_mode == JL_LOG_PATH)));


This is to avoid baking tracking-enabled native code into pkgimages, which is likely undesirable (though arguably reasonable in CI environments)

Perhaps it could be allowed if --check-bounds=yes is also forced, as it is by Pkg.test, given that is unlikely in non-test sessions.

And in that case we wouldn't need to zero out the fptrs from the pkgimage.

Maybe @Keno has an idea on how to handle --check-bounds=yes.

IIUC the problem is that --check-bounds doesn't just change the LLVM code, but actually the behaviour of inference and co. See #50239 (comment)

So right now I don't see away around us needing at least those two copies.

base/loading.jl

vchuravy · 2023-11-11T20:40:13Z

base/util.jl

-    else
-        # If pkgimage is set, malloc_log and code_coverage should not
-        @assert opts.malloc_log == 0 && opts.code_coverage == 0


Maybe we should check that malloc_log/code_coverage don't reach the generating-ouput process?

src/staticdata.c

vchuravy · 2023-11-11T20:47:03Z

We may consider backporting this to 1.10.1

vtjnash · 2024-02-22T17:52:40Z

Oh, I think I missed what this PR was doing. This needs be reverted as it is entirely illegal to zero this memory here and it cannot be permitted to do so without some difficult changes to codegen:

julia/src/staticdata.c

Lines 3771 to 3773 in a96726b

    
           if (ignore_native){ 
        
               memset(&pkgimage.fptrs, 0, sizeof(pkgimage.fptrs)); 
        
           }

It is causing the following test to fail an assert, which is why CI has been red for awhile:

$ ./julia -C native -J./usr/lib/julia/sys.so --code-coverage=user  --code-coverage=temp-tracefile.info --startup-file=no   --eval 'using Test'
julia: /home/vtjnash/julia/src/staticdata.c:2266: jl_root_new_gvars: Assertion `codeinst && (jl_atomic_load_relaxed(&codeinst->specsigflags) & 0b01) && jl_atomic_load_relaxed(&codeinst->specptr.fptr)' failed.

Where the offending CodeInstance was created by StyledStrings and later attempted to be accessed by Test.

vchuravy · 2024-02-22T18:18:10Z

Isn't this very similar to 02699bb ?

But I agree we need to "taint" any package that depends on this package and also ignore their native code, but that seems complicated (also what happens if we have a code instance duplication and one of them is coming from a pkgimg with disabled native code)

Right now pkgimage assumes that the fptr inside the codeinstance is valid and we can load and jump to it. We would need to maybe change calls across images to something like #52797 so that we can support calling a CI with deleted fptrs.

Oof.

This should fix the assertion failure that has been plaguing the Pkg tests, as discussed in #52123 (comment)

When we use options like code coverage, we can't use the native code present in the cache file since it is not instrumented. PR #52123 introduced the capability of skipping the native code during loading, but created the issue that subsequent packages could have an explicit or implicit dependency on the native code. PR #53439 tainted the current process by setting `use_sysimage_native_code`, but this flag is propagated to subprocesses and lead to a regression in test time. Move this to a process local flag to avoid the regression. In the future we might be able to change the calling convention for cross-image calls to `invoke(ci::CodeInstance, args...)` instead of `ci.fptr(args...)` to handle native code not being present.

When we use options like code coverage, we can't use the native code present in the cache file since it is not instrumented. PR #52123 introduced the capability of skipping the native code during loading, but created the issue that subsequent packages could have an explicit or implicit dependency on the native code. PR #53439 tainted the current process by setting `use_sysimage_native_code`, but this flag is propagated to subprocesses and lead to a regression in test time. Move this to a process local flag to avoid the regression. In the future we might be able to change the calling convention for cross-image calls to `invoke(ci::CodeInstance, args...)` instead of `ci.fptr(args...)` to handle native code not being present. --------- Co-authored-by: Jameson Nash <[email protected]>

This should fix the assertion failure that has been plaguing the Pkg tests, as discussed in #52123 (comment) (cherry picked from commit 6cbed31)

When we use options like code coverage, we can't use the native code present in the cache file since it is not instrumented. PR #52123 introduced the capability of skipping the native code during loading, but created the issue that subsequent packages could have an explicit or implicit dependency on the native code. PR #53439 tainted the current process by setting `use_sysimage_native_code`, but this flag is propagated to subprocesses and lead to a regression in test time. Move this to a process local flag to avoid the regression. In the future we might be able to change the calling convention for cross-image calls to `invoke(ci::CodeInstance, args...)` instead of `ci.fptr(args...)` to handle native code not being present. --------- Co-authored-by: Jameson Nash <[email protected]> (cherry picked from commit b8a0a39)

…#53439) This should fix the assertion failure that has been plaguing the Pkg tests, as discussed in JuliaLang#52123 (comment)

When we use options like code coverage, we can't use the native code present in the cache file since it is not instrumented. PR JuliaLang#52123 introduced the capability of skipping the native code during loading, but created the issue that subsequent packages could have an explicit or implicit dependency on the native code. PR JuliaLang#53439 tainted the current process by setting `use_sysimage_native_code`, but this flag is propagated to subprocesses and lead to a regression in test time. Move this to a process local flag to avoid the regression. In the future we might be able to change the calling convention for cross-image calls to `invoke(ci::CodeInstance, args...)` instead of `ci.fptr(args...)` to handle native code not being present. --------- Co-authored-by: Jameson Nash <[email protected]>

…#53439) This should fix the assertion failure that has been plaguing the Pkg tests, as discussed in JuliaLang#52123 (comment)

When we use options like code coverage, we can't use the native code present in the cache file since it is not instrumented. PR JuliaLang#52123 introduced the capability of skipping the native code during loading, but created the issue that subsequent packages could have an explicit or implicit dependency on the native code. PR JuliaLang#53439 tainted the current process by setting `use_sysimage_native_code`, but this flag is propagated to subprocesses and lead to a regression in test time. Move this to a process local flag to avoid the regression. In the future we might be able to change the calling convention for cross-image calls to `invoke(ci::CodeInstance, args...)` instead of `ci.fptr(args...)` to handle native code not being present. --------- Co-authored-by: Jameson Nash <[email protected]>

- JuliaLang/julia#52123: Added the `int ignore_native` argument to jl_restore_package_image_from_file - JuliaLang/julia#49538: Added the `const char *pkgname` argument to jl_restore_package_image_from_file and jl_restore_incremental.

IanButterworth requested a review from vchuravy November 11, 2023 04:09

IanButterworth commented Nov 11, 2023

View reviewed changes

src/init.c Outdated Show resolved Hide resolved

IanButterworth commented Nov 11, 2023

View reviewed changes

vchuravy reviewed Nov 11, 2023

View reviewed changes

base/loading.jl Outdated Show resolved Hide resolved

vchuravy reviewed Nov 11, 2023

View reviewed changes

src/staticdata.c Show resolved Hide resolved

vchuravy approved these changes Nov 11, 2023

View reviewed changes

vchuravy added this to the 1.11 milestone Nov 11, 2023

IanButterworth force-pushed the ib/ignore_pkgimage_code branch 3 times, most recently from 1a76013 to 3fdbf4f Compare November 12, 2023 22:16

IanButterworth mentioned this pull request Nov 13, 2023

Make c func abspath consistent on Windows. Fix tracking path conversion. #52140

Merged

IanButterworth force-pushed the ib/ignore_pkgimage_code branch 5 times, most recently from 956cd59 to 0b05e3b Compare November 14, 2023 02:31

IanButterworth mentioned this pull request Nov 14, 2023

Enable cross-OS caching for generic dirs. Enable OS-specific cache-compiled by default. julia-actions/cache#45

Closed

Use pkgimages for coverage ignoring native code

1fdaace

IanButterworth force-pushed the ib/ignore_pkgimage_code branch from 0b05e3b to 1fdaace Compare November 15, 2023 03:03

IanButterworth mentioned this pull request Nov 15, 2023

Don't use pkgimage for package if any includes fall in tracked path for coverage or alloc tracking #48183

Closed

This comment was marked as resolved.

Sign in to view

IanButterworth added 3 commits November 15, 2023 11:50

add fast path during load that avoids header parse

d7d28ca

Merge branch 'master' into ib/ignore_pkgimage_code

448876b

Merge branch 'master' into ib/ignore_pkgimage_code

a56b586

IanButterworth requested a review from vchuravy November 16, 2023 03:01

IanButterworth merged commit b1c8e12 into JuliaLang:master Nov 16, 2023
5 of 7 checks passed

IanButterworth deleted the ib/ignore_pkgimage_code branch November 16, 2023 19:52

This was referenced Nov 19, 2023

Improve reuse of /compiled cache in CI #50667

Closed

Julia 1.9 pkgimages disabled by default julia-actions/julia-runtest#82

Open

IanButterworth mentioned this pull request Dec 8, 2023

Coverage isn't generated if tracked path is in a symlink #52458

Open

vtjnash mentioned this pull request Feb 4, 2024

coverage: count coverage where explicitly requested by inference only #53125

Closed

vtjnash mentioned this pull request Feb 22, 2024

staticdata: fix assert from partially disabled native code #53439

Merged

vtjnash added a commit that referenced this pull request Feb 23, 2024

staticdata: fix assert from partially disabled native code (#53439)

6cbed31

This should fix the assertion failure that has been plaguing the Pkg tests, as discussed in #52123 (comment)

vchuravy mentioned this pull request Feb 24, 2024

Prevent tainting native code loading from propagating #53457

Merged

fatteneder mentioned this pull request May 12, 2024

Update ccalls in info_cachefile to Julia upstream changes timholy/PkgCacheInspector.jl#31

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use pkgimages for coverage & malloc tracking by ignoring native code in tracked packages #52123

Use pkgimages for coverage & malloc tracking by ignoring native code in tracked packages #52123

IanButterworth commented Nov 11, 2023 •

edited

Loading

IanButterworth Nov 11, 2023

IanButterworth Nov 11, 2023 •

edited

Loading

vchuravy Nov 11, 2023

vchuravy Nov 11, 2023

vchuravy commented Nov 11, 2023

This comment was marked as resolved.

This comment was marked as resolved.

vtjnash commented Feb 22, 2024 •

edited

Loading

vchuravy commented Feb 22, 2024

Use pkgimages for coverage & malloc tracking by ignoring native code in tracked packages #52123

Use pkgimages for coverage & malloc tracking by ignoring native code in tracked packages #52123

Conversation

IanButterworth commented Nov 11, 2023 • edited Loading

IanButterworth Nov 11, 2023

Choose a reason for hiding this comment

IanButterworth Nov 11, 2023 • edited Loading

Choose a reason for hiding this comment

vchuravy Nov 11, 2023

Choose a reason for hiding this comment

vchuravy Nov 11, 2023

Choose a reason for hiding this comment

vchuravy commented Nov 11, 2023

This comment was marked as resolved.

This comment was marked as resolved.

vtjnash commented Feb 22, 2024 • edited Loading

vchuravy commented Feb 22, 2024

IanButterworth commented Nov 11, 2023 •

edited

Loading

IanButterworth Nov 11, 2023 •

edited

Loading

vtjnash commented Feb 22, 2024 •

edited

Loading