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

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

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
45 changes: 42 additions & 3 deletions base/loading.jl
Original file line number Diff line number Diff line change
Expand Up @@ -2444,7 +2444,21 @@ function compilecache(pkg::PkgId, path::String, internal_stderr::IO = stderr, in
# create a temporary file in `cachepath` directory, write the cache in it,
# write the checksum, _and then_ atomically move the file to `cachefile`.
mkpath(cachepath)
cache_objects = JLOptions().use_pkgimages != 0
if JLOptions().use_pkgimages == 0
cache_objects = false
else
if JLOptions().tracked_path == C_NULL
cache_objects = true
else
tracked_path = unsafe_string(JLOptions().tracked_path)
# disable pkgimages if srcpath falls within a code-coverage or allocation-tracking path
# TODO: disable if any includes fall within tracked_path, not just the srcpath
# harder because includes aren't currently known before cache generation
# or implement https://github.com/JuliaLang/julia/issues/51412
cache_objects = !startswith(path, tracked_path)
end
end

tmppath, tmpio = mktemp(cachepath)

if cache_objects
Expand Down Expand Up @@ -3159,6 +3173,7 @@ end
return stale_cachefile(PkgId(""), UInt128(0), modpath, cachefile; ignore_loaded)
end
@constprop :none function stale_cachefile(modkey::PkgId, build_id::UInt128, modpath::String, cachefile::String; ignore_loaded::Bool = false)
tracked_path = JLOptions().tracked_path == C_NULL ? "" : unsafe_string(JLOptions().tracked_path)
io = open(cachefile, "r")
try
checksum = isvalid_cache_header(io)
Expand All @@ -3170,10 +3185,19 @@ end
if isempty(modules)
return true # ignore empty file
end
if ccall(:jl_match_cache_flags, UInt8, (UInt8,), flags) == 0
current_flags = ccall(:jl_cache_flags, UInt8, ())
if !isempty(tracked_path)
for chi in includes
startswith(chi.filename, tracked_path) || continue
@debug "Allowing pkgimages=no for $modkey because it falls in coverage/allocation tracking path $tracked_path"
current_flags &= 0b11111110 # disable pkgimages flag
Copy link
Member

Choose a reason for hiding this comment

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

Can we avoid a magic number here? Maybe CacheFlags(current_flags, pkgimage=no)

Copy link
Sponsor Member Author

Choose a reason for hiding this comment

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

current_flags needs to be passed into the c func jl_match_cache_flags so I didn't think the transform should be done to the julia struct

Copy link
Member

Choose a reason for hiding this comment

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

The struct is the only thing in Julia that knows the position of the bits. I am trying to avoid a circumstance where we move the bits around and forget to update this.

break
end
end
if ccall(:jl_match_cache_flags, UInt8, (UInt8, UInt8), flags, current_flags) == 0
@debug """
Rejecting cache file $cachefile for $modkey since the flags are mismatched
current session: $(CacheFlags())
current session: $(CacheFlags(current_flags))
cache file: $(CacheFlags(flags))
"""
return true
Expand Down Expand Up @@ -3284,11 +3308,14 @@ end
if !ispath(f)
_f = fixup_stdlib_path(f)
if isfile(_f) && startswith(_f, Sys.STDLIB)
check_if_tracked(_f, tracked_path, cachefile) && return true
continue
end
@debug "Rejecting stale cache file $cachefile because file $f does not exist"
return true
end
check_if_tracked(f, tracked_path, cachefile) && return true

if ftime_req >= 0.0
# this is an include_dependency for which we only recorded the mtime
ftime = mtime(f)
Expand Down Expand Up @@ -3341,6 +3368,18 @@ end
end
end

function check_if_tracked(included_file::String, tracked_path::String, cachefile::String)
if JLOptions().use_pkgimages != 0 && JLOptions().code_coverage == 3 && occursin(tracked_path, included_file)
@debug "Rejecting cache file $cachefile because included file $included_file is being tracked by --code-coverage=@$tracked_path"
return true
end
if JLOptions().use_pkgimages != 0 && JLOptions().malloc_log == 3 && occursin(tracked_path, included_file)
@debug "Rejecting cache file $cachefile because included file $included_file is being tracked by --track-allocation=@$tracked_path"
return true
end
return false
end

"""
@__FILE__ -> String

Expand Down
2 changes: 1 addition & 1 deletion base/util.jl
Original file line number Diff line number Diff line change
Expand Up @@ -245,7 +245,7 @@ function julia_cmd(julia=joinpath(Sys.BINDIR, julia_exename()); cpu_target::Unio
push!(addflags, "--pkgimages=no")
else
# If pkgimage is set, malloc_log and code_coverage should not
@assert opts.malloc_log == 0 && opts.code_coverage == 0
@assert opts.malloc_log in (0, 3) && opts.code_coverage in (0, 3)
end
return `$julia -C$cpu_target -J$image_file $addflags`
end
Expand Down
3 changes: 2 additions & 1 deletion src/init.c
Original file line number Diff line number Diff line change
Expand Up @@ -804,7 +804,8 @@ JL_DLLEXPORT void julia_init(JL_IMAGE_SEARCH rel)
#endif

if ((jl_options.outputo || jl_options.outputbc || jl_options.outputasm) &&
(jl_options.code_coverage || jl_options.malloc_log)) {
(jl_options.code_coverage == 1 || jl_options.code_coverage == 2 ||
jl_options.malloc_log == 1 || jl_options.malloc_log == 2)) {
jl_error("cannot generate code-coverage or track allocation information while generating a .o, .bc, or .s output file");
}

Expand Down
11 changes: 9 additions & 2 deletions src/jloptions.c
Original file line number Diff line number Diff line change
Expand Up @@ -860,10 +860,17 @@ JL_DLLEXPORT void jl_parse_opts(int *argcp, char ***argvp)
"This is a bug, please report it.", c);
}
}
if (codecov || malloclog) {
if (codecov == 1 || codecov == 2) {
if (pkgimage_explicit && jl_options.use_pkgimages) {
jl_errorf("julia: Can't use --pkgimages=yes together "
"with --track-allocation or --code-coverage.");
"with --code-coverage=user|all");
}
jl_options.use_pkgimages = 0;
}
if (malloclog == 1 || malloclog == 2) {
if (pkgimage_explicit && jl_options.use_pkgimages) {
jl_errorf("julia: Can't use --pkgimages=yes together "
"with --track-allocation=user|all");
}
jl_options.use_pkgimages = 0;
}
Expand Down
2 changes: 1 addition & 1 deletion src/staticdata.c
Original file line number Diff line number Diff line change
Expand Up @@ -3494,7 +3494,7 @@ static jl_value_t *jl_validate_cache_file(ios_t *f, jl_array_t *depmods, uint64_
"Precompile file header verification checks failed.");
}
uint8_t flags = read_uint8(f);
if (pkgimage && !jl_match_cache_flags(flags)) {
if (pkgimage && !jl_match_cache_flags(flags, jl_cache_flags())) {
return jl_get_exceptionf(jl_errorexception_type, "Pkgimage flags mismatch");
}
if (!pkgimage) {
Expand Down
14 changes: 7 additions & 7 deletions src/staticdata_utils.c
Original file line number Diff line number Diff line change
Expand Up @@ -599,20 +599,20 @@ JL_DLLEXPORT uint8_t jl_cache_flags(void)
{
// OOICCDDP
uint8_t flags = 0;
flags |= (jl_options.use_pkgimages & 1); // 0-bit
// don't use use_pkgimages here because no outputo overrides it
flags |= ((jl_options.outputo == NULL || jl_options.outputo[0] == '\0') | 1); // 0-bit
Copy link
Member

Choose a reason for hiding this comment

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

This seems a bit odd... Are you missing the negation? And | 1 won't it put the pkgimage bit high always?

flags |= (jl_options.debug_level & 3) << 1; // 1-2 bit
flags |= (jl_options.check_bounds & 3) << 3; // 3-4 bit
flags |= (jl_options.can_inline & 1) << 5; // 5-bit
flags |= (jl_options.opt_level & 3) << OPT_LEVEL; // 6-7 bit
return flags;
}

JL_DLLEXPORT uint8_t jl_match_cache_flags(uint8_t flags)
JL_DLLEXPORT uint8_t jl_match_cache_flags(uint8_t cache_flags, uint8_t current_flags)
{
// 1. Check which flags are relevant
uint8_t current_flags = jl_cache_flags();
uint8_t supports_pkgimage = (current_flags & 1);
uint8_t is_pkgimage = (flags & 1);
uint8_t is_pkgimage = (cache_flags & 1);

// For .ji packages ignore other flags
if (!supports_pkgimage && !is_pkgimage) {
Expand All @@ -621,12 +621,12 @@ JL_DLLEXPORT uint8_t jl_match_cache_flags(uint8_t flags)

// 2. Check all flags, execept opt level must be exact
uint8_t mask = (1 << OPT_LEVEL)-1;
if ((flags & mask) != (current_flags & mask))
if ((cache_flags & mask) != (current_flags & mask))
return 0;
// 3. allow for higher optimization flags in cache
flags >>= OPT_LEVEL;
cache_flags >>= OPT_LEVEL;
current_flags >>= OPT_LEVEL;
return flags >= current_flags;
return cache_flags >= current_flags;
}

// "magic" string and version header of .ji file
Expand Down