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

don't capture arrays/tuples in BoundsError #43738

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

don't capture arrays/tuples in BoundsError #43738

Show file tree
Hide file tree
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
3 changes: 3 additions & 0 deletions base/Base.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -422,6 +422,9 @@ end_base_include = time_ns()
const _sysimage_modules = PkgId[]
in_sysimage(pkgid::PkgId) = pkgid in _sysimage_modules

# used for the hack within BoundsError constructor
const _BASE_DEFINED_AGE = get_world_counter()

# Precompiles for Revise
# TODO: move these to contrib/generate_precompile.jl
# The problem is they don't work there
Expand Down
39 changes: 36 additions & 3 deletions base/boot.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -270,13 +270,46 @@ end
macro inline() Expr(:meta, :inline) end
macro noinline() Expr(:meta, :noinline) end

import .Intrinsics: ult_int
struct Summarized
desc::String
size
type
function Summarized(@nospecialize a)
# try to get a summary of `a` here now rather than capturing it for later inspection,
# in order to allow compiler analyses or optimization passes to assume the invariant
# that this `BoundsError` doesn't escape `a` when it is a certain primitive object
# that they can reason about
if isa(a, Array) || isa(a, Tuple)
if isdefined(Main, :Base)
Base = Main.Base
# these `invoke_in_world`s here essentially prohibits users from changing
# the semantics of `BoundsError` by overriding `summary(a)` implementations
# (and potentially allowing `a` to be escaped to somewhere)
desc = Base.invoke_in_world(Base._BASE_DEFINED_AGE, Base.summary, a)::String
Copy link
Member

Choose a reason for hiding this comment

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

I am a bit worried about that. When I joked offline that we "supported" this, @JeffBezanson was "We do not!".

I think a better approach is to define a Core.throw_bounds_error that is defined in the inference world-age.
and have jl_throw_bounds_error invoke in that world-age.

Copy link
Member

Choose a reason for hiding this comment

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

I don't think we really need to (or should) call summary here; saving the size and type should be enough.

To handle calling size, we could define constructors here only for Array and Tuple, and add a constructor in Base for other types so it can call Base.size. (I hope to have a better general solution for this one day, but for now this is how it is.)

Copy link
Member

Choose a reason for hiding this comment

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

size -> axes

if isa(a, Array)
size = Base.invoke_in_world(Base._BASE_DEFINED_AGE, Base.size, a)
else
size = (nfields(a),)
end
else
desc = "`summary` implementation not available"
size = nothing
end
type = typeof(a)
return new(desc, size, type)
end
return a
end
end
struct BoundsError <: Exception
a::Any
a
i::Any
BoundsError() = new()
BoundsError(@nospecialize(a)) = (@noinline; new(a))
BoundsError(@nospecialize(a), i) = (@noinline; new(a,i))
BoundsError(@nospecialize(a)) = (@noinline; new(Summarized(a)))
BoundsError(@nospecialize(a), i) = (@noinline; new(Summarized(a), i))
end

struct DivideError <: Exception end
struct OutOfMemoryError <: Exception end
struct ReadOnlyMemoryError <: Exception end
Expand Down
7 changes: 6 additions & 1 deletion base/errorshow.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,12 @@ function showerror(io::IO, ex::BoundsError)
print(io, "BoundsError")
if isdefined(ex, :a)
print(io, ": attempt to access ")
summary(io, ex.a)
a = ex.a
if isa(a, Core.Summarized)
print(io, a.desc)
else
summary(io, a)
end
if isdefined(ex, :i)
print(io, " at index [")
if ex.i isa AbstractRange
Expand Down
3 changes: 3 additions & 0 deletions base/show.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2792,6 +2792,9 @@ function summary(x)
String(take!(io))
end

# NOTE `summary(a)` implementations for `Array` and `Tuple` should never escape `a` anywhere,
# since that invariant can be assumed by some compiler analyses and optimization passes

## `summary` for AbstractArrays
# sizes such as 0-dimensional, 4-dimensional, 2x3
dims2string(d) = isempty(d) ? "0-dimensional" :
Expand Down
18 changes: 13 additions & 5 deletions src/rtutils.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,11 +139,19 @@ JL_DLLEXPORT void JL_NORETURN jl_atomic_error(char *str) // == jl_exceptionf(jl_
jl_throw(jl_new_struct(jl_atomicerror_type, msg));
}

void JL_NORETURN jl_throw_bounds_error(jl_value_t *v, jl_value_t *t)
Copy link
Member

Choose a reason for hiding this comment

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

Does this code have a special meaning in the context of the other changes, or is this just a refactor?

{
jl_value_t **args;
JL_GC_PUSHARGS(args, 2);
args[0] = v;
args[1] = t;
jl_throw(jl_apply_generic((jl_value_t*)jl_boundserror_type, args, 2));
}

JL_DLLEXPORT void JL_NORETURN jl_bounds_error(jl_value_t *v, jl_value_t *t)
{
JL_GC_PUSH2(&v, &t); // root arguments so the caller doesn't need to
jl_throw(jl_new_struct((jl_datatype_t*)jl_boundserror_type, v, t));
jl_throw_bounds_error(v, t);
}

JL_DLLEXPORT void JL_NORETURN jl_bounds_error_v(jl_value_t *v, jl_value_t **idxs, size_t nidxs)
Expand All @@ -152,7 +160,7 @@ JL_DLLEXPORT void JL_NORETURN jl_bounds_error_v(jl_value_t *v, jl_value_t **idxs
// items in idxs are assumed to already be rooted
JL_GC_PUSH2(&v, &t); // root v so the caller doesn't need to
t = jl_f_tuple(NULL, idxs, nidxs);
jl_throw(jl_new_struct((jl_datatype_t*)jl_boundserror_type, v, t));
jl_throw_bounds_error(v, t);
}

JL_DLLEXPORT void JL_NORETURN jl_bounds_error_tuple_int(jl_value_t **v, size_t nv, size_t i)
Expand All @@ -169,15 +177,15 @@ JL_DLLEXPORT void JL_NORETURN jl_bounds_error_unboxed_int(void *data, jl_value_t
JL_GC_PUSH2(&v, &t);
v = jl_new_bits(vt, data);
t = jl_box_long(i);
jl_throw(jl_new_struct((jl_datatype_t*)jl_boundserror_type, v, t));
jl_throw_bounds_error(v, t);
}

JL_DLLEXPORT void JL_NORETURN jl_bounds_error_int(jl_value_t *v JL_MAYBE_UNROOTED, size_t i)
{
jl_value_t *t = NULL;
JL_GC_PUSH2(&v, &t); // root arguments so the caller doesn't need to
t = jl_box_long(i);
jl_throw(jl_new_struct((jl_datatype_t*)jl_boundserror_type, v, t));
jl_throw_bounds_error(v, t);
}

JL_DLLEXPORT void JL_NORETURN jl_bounds_error_ints(jl_value_t *v JL_MAYBE_UNROOTED,
Expand All @@ -191,7 +199,7 @@ JL_DLLEXPORT void JL_NORETURN jl_bounds_error_ints(jl_value_t *v JL_MAYBE_UNROOT
jl_svecset(t, i, jl_box_long(idxs[i]));
}
t = jl_f_tuple(NULL, jl_svec_data(t), nidxs);
jl_throw(jl_new_struct((jl_datatype_t*)jl_boundserror_type, v, t));
jl_throw_bounds_error(v, t);
}

JL_DLLEXPORT void JL_NORETURN jl_eof_error(void)
Expand Down
32 changes: 30 additions & 2 deletions test/core.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2640,8 +2640,16 @@ end

# pull request #9534
@test_throws BoundsError((1, 2), 3) begin; a, b, c = 1, 2; end
let a = []
@test try; a[]; catch ex; (ex::BoundsError).a === a && ex.i == (); end
let a = Any[]
try
a[]
catch ex
x = (ex::BoundsError).a
@test isa(x, Core.Summarized)
@test x.size === (0,)
@test x.type === Vector{Any}
@test ex.i == ()
end
@test_throws BoundsError(a, (1, 2)) a[1, 2]
@test_throws BoundsError(a, (10,)) a[10]
end
Expand Down Expand Up @@ -2681,6 +2689,26 @@ f9534h(a, b, c...) = c[a]
@test f9534h(4, 2, 3, 4, 5, 6) == 6
@test_throws BoundsError((3, 4, 5, 6), 5) f9534h(5, 2, 3, 4, 5, 6)

@test let # https://github.com/JuliaLang/julia/pull/43738
# BoundsError should never capture arrays
code = quote
const THROWN_ARRAY = Ref{Any}()
function Base.summary(io::IO, a::Array)
Main.THROWN_ARRAY[] = a
return "bad override"
end
try
a = []
a[1]
catch err
end
isdefined(THROWN_ARRAY, :x) && println(stderr, "test failed")
end |> string
cmd = `$(Base.julia_cmd()) -e $code`
stderr = IOBuffer()
success(pipeline(Cmd(cmd); stdout=stdout, stderr=stderr)) && isempty(String(take!(stderr)))
end

# issue #7978, comment 332352438
f7978a() = 1
@test_throws BoundsError(1, 2) begin; a, b = f7978a(); end
Expand Down