Julia: syntax: bad ssavalue / `GC.@preserve` interaction

[StefanKarpinski]

Julia fails at parsing the following seemingly-legal function definition:

function handle_repos_add!(ctx::Context, pkgs::AbstractVector{PackageSpec};
                           upgrade_or_add::Bool=true, credentials=nothing)
    # Always update the registry when adding
    UPDATED_REGISTRY_THIS_SESSION[] || update_registries(ctx)
    creds = credentials !== nothing ? credentials : LibGit2.CachedCredentials()
    try
        env = ctx.env
        new_uuids = UUID[]
        for pkg in pkgs
            pkg.repo == nothing && continue
            pkg.special_action = PKGSPEC_REPO_ADDED
            isempty(pkg.repo.url) && set_repo_for_pkg!(env, pkg)
            clones_dir = joinpath(depots1(), "clones")
            mkpath(clones_dir)
            repo_path = joinpath(clones_dir, string(hash(pkg.repo.url)))
            repo = nothing
            do_nothing_more = false
            project_path = nothing
            folder_already_downloaded = false
            try
                repo = if ispath(repo_path)
                    LibGit2.GitRepo(repo_path)
                else
                    GitTools.clone(pkg.repo.url, repo_path, isbare=true, credentials=creds)
                end
                info = manifest_info(env, pkg.uuid)
                pinned = (info != nothing && get(info, "pinned", false))
                upgrading = upgrade_or_add && !pinned
                if upgrading
                    GitTools.fetch(repo; refspecs=refspecs, credentials=creds)
                    rev = pkg.repo.rev
                    # see if we can get rev as a branch
                    if isempty(rev)
                        if LibGit2.isattached(repo)
                            rev = LibGit2.branch(repo)
                        else
                            rev = string(LibGit2.GitHash(LibGit2.head(repo)))
                        end
                    end
                else
                    # Not upgrading so the rev should be the current git-tree-sha
                    rev = info["git-tree-sha1"]
                    pkg.version = VersionNumber(info["version"])
                end

                gitobject, isbranch = get_object_branch(repo, rev, creds)
                # If the user gave a shortened commit SHA, might as well update it to the full one
                try
                    if upgrading
                        pkg.repo.rev = isbranch ? rev : string(LibGit2.GitHash(gitobject))
                    end
                    LibGit2.with(LibGit2.peel(LibGit2.GitTree, gitobject)) do git_tree
                        @assert git_tree isa LibGit2.GitTree
                        pkg.repo.git_tree_sha1 = SHA1(string(LibGit2.GitHash(git_tree)))
                            version_path = nothing
                            folder_already_downloaded = false
                        if has_uuid(pkg) && has_name(pkg)
                            version_path = Pkg.Operations.find_installed(pkg.name, pkg.uuid, pkg.repo.git_tree_sha1)
                            isdir(version_path) && (folder_already_downloaded = true)
                            info = manifest_info(env, pkg.uuid)
                            if info != nothing && get(info, "git-tree-sha1", "") == string(pkg.repo.git_tree_sha1) && folder_already_downloaded
                                # Same tree sha and this version already downloaded, nothing left to do
                                do_nothing_more = true
                            end
                        end
                        if folder_already_downloaded
                            project_path = version_path
                        else
                            project_path = mktempdir()
                            GC.@preserve project_path begin
                                opts = LibGit2.CheckoutOptions(
                                    checkout_strategy = LibGit2.Consts.CHECKOUT_FORCE,
                                    target_directory = Base.unsafe_convert(Cstring, project_path),
                                )
                                LibGit2.checkout_tree(repo, git_tree, options=opts)
                            end
                        end
                    end
                finally
                    close(gitobject)
                end
            finally
                repo isa LibGit2.GitRepo && close(repo)
            end
            do_nothing_more && continue
            parse_package!(ctx, pkg, project_path)
            if !folder_already_downloaded
                version_path = Pkg.Operations.find_installed(pkg.name, pkg.uuid, pkg.repo.git_tree_sha1)
                mkpath(version_path)
                mv(project_path, version_path; force=true)
                push!(new_uuids, pkg.uuid)
            end
            Base.rm(project_path; force = true, recursive = true)
            @assert has_uuid(pkg)
        end
        return new_uuids
    finally
        creds !== credentials && Base.shred!(creds)
    end
end

with this error message:

ERROR: syntax: ssavalue with no def

There is no error without the GC.@preserve block.

Found at https://github.com/JuliaLang/Pkg.jl/pull/902.

[StefanKarpinski]

cc @Keno since you implemented both SSAIR and GC.@preserve. Let me know if it would be helpful to whittle the function down further. I've tried mocking up the basic try/finally/GC.@preserve structure but that doesn't replicate this issue so it's something a bit more complex than that.

[maleadt]

Reduced to:

for a in b
    c = try
        try
            d() do 
                if  GC.@preserve c begin

                    end
                end
            end
        finally
        end
    finally
    end
end

[Keno]

cc @Keno since you implemented both SSAIR and GC.@preserve

You'd think so, but that error is from the SSA values the frontend generates, i.e. that's for @JeffBezanson

[StefanKarpinski]

@maleadt: in your reduction c is preserved before it's assigned, which is arguably actually an error (maybe it shouldn't be a syntax error). If I delete c = then this doesn't error anymore.

[StefanKarpinski]

I've asked @vtjnash to write down some of the things he's said on the triage call about this but the high-level gist of it seems to be that from the compilers perspective project_path is initialized in a different function, not in the closure body where the GC.@preserve occurs, so it can't be sure that it has been initialized at that point.

I made the argument that GC.@preserve should preserve values—whatever value an expression evaluates to at the time if executes—rather than variables. For example, it would even make sense to write something like this:

GC.@preserve a[i] begin
    # access `a[i]` here
end

This should be equivalent to doing this:

let tmp = a[i]
GC.@preserve tmp begin
    # access `a[i]` here
end

Note that there is no rewrite of the body to change a[i] to tmp or anything like that. If someone does a[i] = other then other is _not_ preserved—if a isn't rooted and a[i] is the only reference to other then other could get collected. Only the old value of a[i] is actually preserved. So a straightforward solution might be to change the lowering of GC.@preserve to introduce new names for the expressions that it preserves.

[maleadt]

in your reduction c is preserved before it's assigned, which is arguably actually an error

Yeah, I noticed. This was an automatic testcase reduction though, so I didn't have any say in it :slightly_smiling_face:
Either way, such an error probably shouldn't ever show up (?) so I figured it would be useful to report that reduced test case as it might help isolate the underlying cause.

[StefanKarpinski]

This was an automatic testcase reduction though

What magical tool do you use for that?

[JeffBezanson]

It looks to me like the implementation of gc_preserve does in fact evaluate all its arguments, but the front-end macro for it requires all arguments to be symbols to be conservative. For example, it lets us mostly avoid the question of whether side-effects in the arguments are guaranteed to happen.

[maleadt]

What magical tool do you use for that?

C-Reduce with --not-c. I've put up some details at https://github.com/maleadt/creduce_julia

[JeffBezanson]

C-Reduce with --not-c

:joy: