Properly guard UpsilonNode unboxed store #51853

Keno · 2023-10-25T02:06:34Z

In #51852, we are coercing a boxed Union{@NamedTuple{progress::String}, @NamedTuple{progress::Float64}}
to @NamedTuple{progress::String} via convert_julia_type. This results in a jl_cgval_t that has
a Vboxed that points to a boxed @NamedTuple{progress::Float64} but with a @NamedTuple{progress::String}
type tag that the up upsilonnode code then tries to unbox into the typed PhiC slot. This ends up treating
the Float64 as a pointer and crashing in GC. Avoid this by adding a runtime check that the converted value
is actually compatible (we already had this kind of check for the non-boxed cases) and then making the
unboxing runtime-conditional on the type of the union matching the type of the phic.

vtjnash

Lgtm. Seems like a couple extraneous return statements added to codegen? But otherwise a good cleanup and fix.

vtjnash · 2023-10-25T12:49:16Z

src/codegen.cpp

@@ -2320,6 +2321,9 @@ static jl_cgval_t convert_julia_type(jl_codectx_t &ctx, const jl_cgval_t &v, jl_
        if (v.TIndex && !jl_is_pointerfree(typ)) {
            // discovered that this union-split type must actually be isboxed
            if (v.Vboxed) {
+                if (skip) {
+                    *skip = ctx.builder.CreateNot(emit_exactly_isa(ctx, v, (jl_datatype_t*)typ));


Isn't Vboxed potentially nullptr at runtime here? Maybe test that tindex==0x80 instead?

Yes it is. However, tindex==0x80 check isn't sufficient here either. Needs an extra case in emit_exactly_isa to check for null (in this case only).

gbaraldi · 2023-10-25T12:53:55Z

The underlying inference bug is still present right?

oscardssmith · 2023-10-25T12:55:35Z

why is this on the backport list? we're pretty sure what this was a big introduced a few weeks ago

topolarity · 2023-10-25T21:00:04Z

This appears to resolve the more standalone MRE (#51852 (comment)) but not the original one in the issue:

function f(;kwargs...)
    try
        kwargs = rand((values(kwargs), (progress=1.0,)))
    catch
    end
    GC.gc()
    return kwargs
end

f(; progress="0.5")
f(; progress="0.5")
f(; progress="0.5")
f(; progress="0.5")
f(; progress="0.5")

This segfaults for me very reliably locally, with the same signature.

Keno · 2023-10-25T22:20:24Z

This segfaults for me very reliably locally, with the same signature.

Indeed, although it takes more than one execution for me, so ... progress? I'll take a look.

oscardssmith · 2023-10-25T22:29:08Z

since it calls rand it only segfaults half the time.

Keno · 2023-10-25T22:30:44Z

Fair observation.

Keno · 2023-10-26T06:19:01Z

Updated MRE:

julia> function phic_type5()
           a = (;progress = "a")
           try
               vals = (a, (progress=1.0,))
               a = vals[Base.inferencebarrier(false) ? 1 : 2]
           catch
           end
           GC.gc()
           return a
       end

Keno · 2023-10-26T09:03:23Z

I managed to fix the MRE, but I found a test case (after fixing a related inference bug that was hiding it) for @vtjnash's comment above, so I'll need to fix that tomorrow before we can merge this.

Keno · 2023-10-26T09:46:00Z

I'm gonna remove the backport label here. The bug is technically present on 1.10, but we don't have the inference precision to actually see it and I think there is some risk in destabilizing things, esp, since I included that inference precision fix (so it wouldn't backport cleanly anyway).

Keno · 2023-10-26T09:47:18Z

so I'll need to fix that tomorrow before we can merge this.

I thought about it and it was easier than I thought, so I just did it now.

topolarity · 2023-10-26T11:59:22Z

What is the inference fix here for? Is it a required part of the change?

oscardssmith · 2023-10-26T15:06:33Z

The change is in the details of effect convergence.

Keno · 2023-10-26T16:05:25Z

What is the inference fix here for? Is it a required part of the change?

It's a fix to the logic of the previous phic narrowing PR. It's not required, but without it's not possible to write a test for one of the branches of the codegen fix because it's hidden by the inference issue, so I fixed that too.

oscardssmith · 2023-10-26T16:16:05Z

CI seems very unhappy about this currently.

stdlib/REPL/src/REPLCompletions.jl

Keno · 2023-10-26T22:15:00Z

@nanosoldier runtests()

nanosoldier · 2023-10-27T16:00:52Z

The package evaluation job you requested has completed - possible new issues were detected.
The full report is available.

topolarity · 2023-10-27T21:36:44Z

Another day, another MWE. It looks like the FLoops.jl failure is relevant.

Reduced to:

julia> foo() = try (a = a, b = 2) catch end
foo (generic function with 1 method)

julia> foo()
ERROR: UndefVarError: `a` not defined
Stacktrace:
 [1] foo()
   @ Main ./REPL[1]:1
 [2] top-level scope
   @ REPL[2]:1

Seems like we're incorrectly inferring that the catch block is unreachable.

We then illegally optimize away the try-catch:

julia> @code_typed optimize=false foo()
CodeInfo(
1 ─ %1 = $(Expr(:enter, #4))
2 ─ %2 = (:a, :b)::Core.Const((:a, :b))
│   %3 = Core.apply_type(Core.NamedTuple, %2)::Core.Const(NamedTuple{(:a, :b)})
│   %4 = Core.tuple(Main.a, 2)::Core.PartialStruct(Tuple{Any, Int64}, Any[Any, Core.Const(2)])
│   %5 = (%3)(%4)::NamedTuple{(:a, :b), <:Tuple{Any, Int64}}
└──      $(Expr(:leave, :(%1)))
3 ─      return %5
4 ┄      Core.Const(:($(Expr(:leave, :(%1)))))::Union{}
│        Core.Const(:($(Expr(:pop_exception, :(%1)))))::Union{}
└──      Core.Const(:(return nothing))::Union{}
) => NamedTuple{(:a, :b), <:Tuple{Any, Int64}}

julia> @code_typed optimize=true foo()
CodeInfo(
1 ─      nothing::Nothing
│   %2 = Main.a::Any
│   %3 = Core.tuple(%2, 2)::Tuple{Any, Int64}
│   %4 = Core.typeof(%3)::Type{<:Tuple{Any, Int64}}
│   %5 = Core.apply_type(Core.NamedTuple, (:a, :b), %4)::Type{NamedTuple{(:a, :b), var"#s179"}} where var"#s179"<:Tuple{Any, Int64}
│   %6 = %new(%5, %2, 2)::NamedTuple{(:a, :b), <:Tuple{Any, Int64}}
└──      return %6
) => NamedTuple{(:a, :b), <:Tuple{Any, Int64}}

Keno · 2023-10-27T21:49:55Z

Ok, that should a quick fix. Looks like we're forgetting that the GlobalRef is not nothrow.

Introduce UNION_BOX_MARKER, to make it easier to grep for all the places where this is being looked at and distinguish them from other uses of 0x80 as a constant.

aviatesk · 2023-11-02T09:29:49Z

base/compiler/abstractinterpretation.jl

    if isdefined_globalref(g) && isconst(g)
        return Const(ccall(:jl_get_globalref_value, Any, (Any,), g))
    end
    ty = ccall(:jl_get_binding_type, Any, (Any, Any), g.mod, g.name)
    ty === nothing && return Any
    return ty
 end
-abstract_eval_global(M::Module, s::Symbol) = abstract_eval_globalref(GlobalRef(M, s))
+abstract_eval_global(M::Module, s::Symbol) = abstract_eval_globalref_type(GlobalRef(M, s))


Suggested change

abstract_eval_global(M::Module, s::Symbol) = abstract_eval_globalref_type(GlobalRef(M, s))

abstract_eval_global_type(M::Module, s::Symbol) = abstract_eval_globalref_type(GlobalRef(M, s))

to reflect the fact that it returns type only? I don't think this definition is being used anywhere though.

In #51852, we are coercing a boxed `Union{@NamedTuple{progress::String}, @NamedTuple{progress::Float64}}` to `@NamedTuple{progress::String}` via convert_julia_type. This results in a jl_cgval_t that has a Vboxed that points to a boxed `@NamedTuple{progress::Float64}` but with a `@NamedTuple{progress::String}` type tag that the up upsilonnode code then tries to unbox into the typed PhiC slot. This ends up treating the Float64 as a pointer and crashing in GC. Avoid this by adding a runtime check that the converted value is actually compatible (we already had this kind of check for the non-boxed cases) and then making the unboxing runtime-conditional on the type of the union matching the type of the phic. Fixes #51852

topolarity · 2023-11-07T16:41:03Z

src/codegen.cpp

+    Value *skip = NULL;
+    rval_info = convert_julia_type(ctx, rval_info, slot_type, &skip);
+    if (!allow_mismatch && skip) {
+        CreateTrap(ctx.builder);


Doesn't this (incorrectly) trap unconditionally trap in the new (v.Vboxed && (v.isboxed || mustbox_union)) case?

This branch seems to be dead at least when compiling the sysimage + stdlibs, but perhaps it'd be a bug if it were exercised?

Can you try to come up with a testcase (or if you can't, you can try pkgevaling this with a compile-time error to find if there's one in the wild).

Currently [1] it is illegal [2] in IRCode to have a GlobalRef in value position that could potentially throw. This is because in IRCode, we want to assign flags to every statement and if there are multiple things with effects in a statement, we lose precision in tracking which they apply to. However, we currently do allow this in `CodeInfo`. Now that we're starting to make more use of flags in inference also, this is becoming annoying (as it did for IRCode), so I would like to do this transformation earlier. This is an attempt to do this during lowering. It is not entirely clear that this is precisely the correct place for it. We could alternatively consider doing it during the global resolve pass in method.c, but that currently does not renumber SSAValues, so doing it during the renumbering inside lowering may be easier. N.B.: This is against #51853, because this needs some of the inference precision improvements in that PR to avoid regressing the try/catch elision tests (which before that PR, we were incorrectly computing effects for statement-position GlobalRefs). [1] 39c278b [2] https://github.com/JuliaLang/julia/blob/2f63cc99fb134fb4adb7f11ba86a4e2ab5adcd48/base/compiler/ssair/verify.jl#L54-L58 --------- Co-authored-by: Jeff Bezanson <jeff.bezanson@gmail.com> Co-authored-by: Oscar Smith <oscardssmith@gmail.com>

vtjnash approved these changes Oct 25, 2023

View reviewed changes

vtjnash added compiler:codegen Generation of LLVM IR and native code merge me PR is reviewed. Merge when all tests are passing don't squash Don't squash merge backport 1.10 Change should be backported to the 1.10 release labels Oct 25, 2023

Keno force-pushed the kf/51852 branch from 1d13fb3 to e3e9ef0 Compare October 25, 2023 06:48

vtjnash reviewed Oct 25, 2023

View reviewed changes

Keno force-pushed the kf/51852 branch from e3e9ef0 to e1d6fd8 Compare October 26, 2023 09:00

Keno removed the merge me PR is reviewed. Merge when all tests are passing label Oct 26, 2023

Keno force-pushed the kf/51852 branch from e1d6fd8 to d311d27 Compare October 26, 2023 09:01

Keno force-pushed the kf/51852 branch from d311d27 to 8c68eae Compare October 26, 2023 09:44

Keno removed the backport 1.10 Change should be backported to the 1.10 release label Oct 26, 2023

Keno force-pushed the kf/51852 branch from 8c68eae to cef3ff5 Compare October 26, 2023 09:46

Keno force-pushed the kf/51852 branch from cef3ff5 to 19d9ae7 Compare October 26, 2023 16:36

topolarity reviewed Oct 26, 2023

View reviewed changes

stdlib/REPL/src/REPLCompletions.jl Outdated Show resolved Hide resolved

Keno force-pushed the kf/51852 branch from d1a9287 to f8097fb Compare October 26, 2023 20:05

oscardssmith added the bugfix This change fixes an existing bug label Oct 26, 2023

topolarity force-pushed the kf/51852 branch from f8097fb to 9dce089 Compare October 26, 2023 22:10

Don't use 0x80 as a magic constant

072896d

Introduce UNION_BOX_MARKER, to make it easier to grep for all the places where this is being looked at and distinguish them from other uses of 0x80 as a constant.

Keno force-pushed the kf/51852 branch from 9dce089 to 47ed081 Compare November 1, 2023 02:58

Keno mentioned this pull request Nov 1, 2023

Outline potentially undefined globals during lowering #51970

Merged

aviatesk reviewed Nov 2, 2023

View reviewed changes

topolarity force-pushed the kf/51852 branch from 47ed081 to 3886803 Compare November 3, 2023 18:42

MasonProtter mentioned this pull request Nov 5, 2023

Unpredictable segfault on master when multithreading with many allocations #52032

Open

oscardssmith merged commit 65a0fd0 into master Nov 5, 2023

oscardssmith deleted the kf/51852 branch November 5, 2023 17:01

oscardssmith mentioned this pull request Nov 5, 2023

GC segfaults during JuliaInterpreter tests #51800

Open

topolarity reviewed Nov 7, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Properly guard UpsilonNode unboxed store #51853

Properly guard UpsilonNode unboxed store #51853

Keno commented Oct 25, 2023

vtjnash left a comment

vtjnash Oct 25, 2023

Keno Oct 26, 2023

gbaraldi commented Oct 25, 2023

oscardssmith commented Oct 25, 2023

topolarity commented Oct 25, 2023

Keno commented Oct 25, 2023

oscardssmith commented Oct 25, 2023

Keno commented Oct 25, 2023

Keno commented Oct 26, 2023

Keno commented Oct 26, 2023

Keno commented Oct 26, 2023

Keno commented Oct 26, 2023

topolarity commented Oct 26, 2023

oscardssmith commented Oct 26, 2023

Keno commented Oct 26, 2023

oscardssmith commented Oct 26, 2023

Keno commented Oct 26, 2023

nanosoldier commented Oct 27, 2023

topolarity commented Oct 27, 2023 •

edited

Loading

Keno commented Oct 27, 2023

aviatesk Nov 2, 2023

topolarity Nov 7, 2023 •

edited

Loading

Keno Nov 9, 2023

	abstract_eval_global(M::Module, s::Symbol) = abstract_eval_globalref_type(GlobalRef(M, s))
	abstract_eval_global_type(M::Module, s::Symbol) = abstract_eval_globalref_type(GlobalRef(M, s))

Properly guard UpsilonNode unboxed store #51853

Properly guard UpsilonNode unboxed store #51853

Conversation

Keno commented Oct 25, 2023

vtjnash left a comment

Choose a reason for hiding this comment

vtjnash Oct 25, 2023

Choose a reason for hiding this comment

Keno Oct 26, 2023

Choose a reason for hiding this comment

gbaraldi commented Oct 25, 2023

oscardssmith commented Oct 25, 2023

topolarity commented Oct 25, 2023

Keno commented Oct 25, 2023

oscardssmith commented Oct 25, 2023

Keno commented Oct 25, 2023

Keno commented Oct 26, 2023

Keno commented Oct 26, 2023

Keno commented Oct 26, 2023

Keno commented Oct 26, 2023

topolarity commented Oct 26, 2023

oscardssmith commented Oct 26, 2023

Keno commented Oct 26, 2023

oscardssmith commented Oct 26, 2023

Keno commented Oct 26, 2023

nanosoldier commented Oct 27, 2023

topolarity commented Oct 27, 2023 • edited Loading

Keno commented Oct 27, 2023

aviatesk Nov 2, 2023

Choose a reason for hiding this comment

topolarity Nov 7, 2023 • edited Loading

Choose a reason for hiding this comment

Keno Nov 9, 2023

Choose a reason for hiding this comment

topolarity commented Oct 27, 2023 •

edited

Loading

topolarity Nov 7, 2023 •

edited

Loading