Skip to content

Commit

Permalink
fix missing gc-roots (#169)
Browse files Browse the repository at this point in the history
* fix missing gc-roots

since calling `pointer()` explicitly discards gc-rooting
need to ensure that there is a memory barrier that uses the value
after we know for certain we are done with the value

note that a better pattern is usually to use `Ref` to make gc-safe interior pointers

* fix v0.7 compatibility with macrocall __source__ argument

* fix v0.6 compatibility with JLD00 and JLDArchive tests

* make JLD00-formatted files readable on v0.6

* Rebase fixes

* Fix tests
  • Loading branch information
vtjnash authored and simonster committed Jul 30, 2017
1 parent a3f130e commit 41ac60f
Show file tree
Hide file tree
Showing 7 changed files with 74 additions and 40 deletions.
2 changes: 1 addition & 1 deletion LICENSE.txt
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
The MIT License (MIT)
Copyright (c) 2012-2013 Timothy E. Holy, Konrad Hinsen, Tom Short, and Simon Kornblith
Copyright (c) 2012-2017 Timothy E. Holy, Konrad Hinsen, Tom Short, Simon Kornblith, Google Inc., and contributors

Permission is hereby granted, free of charge, to any person obtaining a copy of this software and associated documentation files (the "Software"), to deal in the Software without restriction, including without limitation the rights to use, copy, modify, merge, publish, distribute, sublicense, and/or sell copies of the Software, and to permit persons to whom the Software is furnished to do so, subject to the following conditions:

Expand Down
2 changes: 1 addition & 1 deletion REQUIRE
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
julia 0.6
HDF5
Compat 0.18.0
Compat 0.25.0
FileIO
LegacyStrings
72 changes: 52 additions & 20 deletions src/JLD.jl
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@ import Base: convert, length, endof, show, done, next, ndims, start, delete!, el
size, sizeof, unsafe_convert, datatype_pointerfree
import LegacyStrings: UTF16String

@noinline gcuse(x) = x # because of use of `pointer`, need to mark gc-use end explicitly

const magic_base = "Julia data file (HDF5), version "
const version_current = v"0.1.1"
const pathrefs = "/_refs"
Expand All @@ -28,7 +30,6 @@ end
### Dummy types used for converting attribute strings to Julia types
mutable struct UnsupportedType; end
mutable struct UnconvertedType; end
mutable struct CompositeKind; end # here this means "a type with fields"


struct JldDatatype
Expand Down Expand Up @@ -196,10 +197,8 @@ function jldopen(filename::AbstractString, rd::Bool, wr::Bool, cr::Bool, tr::Boo
end
if startswith(magic, Vector{UInt8}(magic_base))
version = convert(VersionNumber, unsafe_string(pointer(magic) + length(magic_base)))
gcuse(magic)
if version < v"0.1.0"
if !isdefined(JLD, :JLD00)
eval(:(include(joinpath($(dirname(@__FILE__)), "JLD00.jl"))))
end
fj = JLD00.jldopen(filename, rd, wr, cr, tr, ff; mmaparrays=mmaparrays)
else
f = HDF5.h5f_open(filename, wr ? HDF5.H5F_ACC_RDWR : HDF5.H5F_ACC_RDONLY, pa.id)
Expand Down Expand Up @@ -394,7 +393,9 @@ read_scalar(obj::JldDataset, dtype::HDF5Datatype, ::Type{T}) where {T<:BitsKindO
function read_scalar(obj::JldDataset, dtype::HDF5Datatype, T::Type)
buf = Vector{UInt8}(sizeof(dtype))
HDF5.readarray(obj.plain, dtype.id, buf)
return readas(jlconvert(T, file(obj), pointer(buf)))
sc = readas(jlconvert(T, file(obj), pointer(buf)))
gcuse(buf)
sc
end

## Arrays
Expand Down Expand Up @@ -454,6 +455,8 @@ function read_vals(obj::JldDataset, dtype::HDF5Datatype, T::Type, dspace_id::HDF
h5offset += h5sz
end
end
gcuse(buf)
gcuse(out)
out
end

Expand Down Expand Up @@ -622,6 +625,7 @@ end
h5convert!(offset, f, data[i], wsession)
offset += sz
end
gcuse(buf)
buf
end

Expand Down Expand Up @@ -690,6 +694,7 @@ function write_compound(parent::Union{JldFile, JldGroup}, name::String,

buf = Vector{UInt8}(HDF5.h5t_get_size(dtype))
h5convert!(pointer(buf), file(parent), s, wsession)
gcuse(buf)

dspace = HDF5Dataspace(HDF5.h5s_create(HDF5.H5S_SCALAR))
dprop, dprop_close = dset_create_properties(parent, length(buf), buf; kargs...)
Expand Down Expand Up @@ -871,12 +876,13 @@ const _where_macrocall = Symbol("@where")
function expand_where_macro(e::Expr)
e.head = :where
shift!(e.args)
Compat.macros_have_sourceloc && shift!(e.args)
return true
end

is_valid_type_ex(s::Symbol) = true
is_valid_type_ex(s::QuoteNode) = true
is_valid_type_ex(::T) where {T} = isbits(T)
is_valid_type_ex(s) = isbits(typeof(s))
function is_valid_type_ex(e::Expr)
if e.head === :curly || e.head == :tuple || e.head == :.
return all(is_valid_type_ex, e.args)
Expand All @@ -887,7 +893,14 @@ function is_valid_type_ex(e::Expr)
is_valid_type_ex(e.args[2].args[2])
elseif e.head == :call
f = e.args[1]
return f === :Union || f === :TypeVar || f === :symbol
if f isa Expr
if f.head === :core
f = f.args[1]
return f === :Union || f === :TypeVar || f === :UnionAll
end
elseif f isa Symbol
return f === :Union || f === :TypeVar || f === :symbol
end
end
return false
end
Expand All @@ -902,10 +915,19 @@ const typemap_Core = Dict(

const _typedict = Dict{String,Type}()

fixtypes(typ) = typ
function fixtypes(typ::Expr)
function fixtypes(typ)
whereall = []
typ = fixtypes(typ, whereall)
while !isempty(whereall)
var = pop!(whereall)
typ = Expr(:let, Expr(:call, Expr(:core, :UnionAll), var.args[1], typ), var)
end
return typ
end
fixtypes(typ, whereall) = typ
function fixtypes(typ::Expr, whereall::Vector{Any})
if typ.head === :macrocall && typ.args[1] === _where_macrocall
expand_where_macro(typ)
expand_where_macro(typ) # @where => TypeVar format forwards compatibility
end
if typ.head === :.
if length(typ.args) == 2 && typ.args[1] === :Core
Expand All @@ -920,16 +942,31 @@ function fixtypes(typ::Expr)
end

for i = 1:length(typ.args)
typ.args[i] = fixtypes(typ.args[i])
typ.args[i] = fixtypes(typ.args[i], whereall)
end

if (typ.head === :call && !isempty(typ.args) &&
typ.args[1] === :TypeVar) # TypeVar => where format backwards compatibility
tv = gensym()
push!(whereall, Expr(:(=), tv, typ))
return tv
end

if (typ.head === :call && !isempty(typ.args) &&
typ.args[1] === :Union)
return Expr(:curly, typ.args...)
typ = Expr(:curly, typ.args...)
end

if typ.head === :tuple
return Expr(:curly, :Tuple, typ.args...)
typ = Expr(:curly, :Tuple, typ.args...)
end

if typ.head === :curly
# assume literal TypeVar should work like `T{<:S}`
while !isempty(whereall)
var = pop!(whereall)
typ = Expr(:let, Expr(:call, Expr(:core, :UnionAll), var.args[1], typ), var)
end
end
return typ
end
Expand All @@ -953,13 +990,7 @@ end
function julia_type(e::Union{Symbol, Expr})
if is_valid_type_ex(e)
try # `try` needed to catch undefined symbols
typ = eval(current_module(), e)
typ == Type && return Type
isa(typ, Type) && return typ
end
try
# It's possible that `e` will represent a type not present in current_module(),
# but as long as `e` is fully qualified, it should exist/be reachable from Main
# `e` should be fully qualified, and thus reachable from Main
typ = eval(Main, e)
typ == Type && return Type
isa(typ, Type) && return typ
Expand Down Expand Up @@ -1273,4 +1304,5 @@ function __init__()
nothing
end

include("JLD00.jl")
end
26 changes: 14 additions & 12 deletions src/JLD00.jl
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ using HDF5, Compat, LegacyStrings
import HDF5: close, dump, exists, file, getindex, setindex!, g_create, g_open, o_delete, name, names, read, size, write,
HDF5ReferenceObj, HDF5BitsKind, ismmappable, readmmap
import Base: length, endof, show, done, next, start, delete!
import JLD
import ..JLD

# See julia issue #8907
replacements = Any[]
Expand Down Expand Up @@ -289,6 +289,9 @@ function read(obj::Union{JldFile, JldDataset})
error("Type ", typename, " is not recognized. As a fallback, you can load ", name(obj), " with readsafely().")
end
end
if T == UnsupportedType
error("Type expression ", typename, ", is not recognized.")
end
read(obj, T)
end
function readsafely(obj::Union{JldFile, JldDataset})
Expand Down Expand Up @@ -431,6 +434,7 @@ function read(obj::JldDataset, ::Type{Expr})
end

# CompositeKind
read(obj::JldDataset, T::UnionAll) = read(obj, Base.unwrap_unionall(T))
function read(obj::JldDataset, T::DataType)
if isempty(fieldnames(T)) && T.size > 0
return read_bitstype(obj, T)
Expand All @@ -439,11 +443,14 @@ function read(obj::JldDataset, T::DataType)
# Add the parameters
if exists(obj, "TypeParameters")
params = a_read(obj.plain, "TypeParameters")
p = Vector{Any}(length(params))
for i = 1:length(params)
p[i] = eval(current_module(), parse(params[i]))
if !isempty(params)
p = Vector{Any}(length(params))
for i = 1:length(params)
p[i] = eval(current_module(), parse(params[i]))
end
T = T.name.wrapper
T = T{p...}
end
T = T{p...}
end
v = getrefs(obj, Any)
if length(v) == 0
Expand Down Expand Up @@ -921,20 +928,15 @@ end

### Converting attribute strings to Julia types

is_valid_type_ex(s::Symbol) = true
is_valid_type_ex(s::QuoteNode) = true
is_valid_type_ex(x::Int) = true
is_valid_type_ex(e::Expr) = ((e.head == :curly || e.head == :tuple || e.head == :.) && all(is_valid_type_ex, e.args)) ||
(e.head == :call && (e.args[1] == :Union || e.args[1] == :TypeVar))

const _typedict = Dict{String,Type}()
_typedict["CompositeKind"] = CompositeKind
function _julia_type(s::AbstractString)
typ = get(_typedict, s, UnconvertedType)
if typ == UnconvertedType
e = parse(s)
e = JLD.fixtypes(e)
typ = UnsupportedType
if is_valid_type_ex(e)
if JLD.is_valid_type_ex(e)
try # try needed to catch undefined symbols
typ = eval(e)
if !isa(typ, Type)
Expand Down
6 changes: 4 additions & 2 deletions src/jld_types.jl
Original file line number Diff line number Diff line change
Expand Up @@ -150,7 +150,8 @@ function h5type(::JldFile, ::Type{T}, ::Bool) where T<:String
JldDatatype(HDF5Datatype(type_id, false), 0)
end

h5convert!(out::Ptr, ::JldFile, x::String, ::JldWriteSession) =
# no-inline needed to ensure gc-root for x
@noinline h5convert!(out::Ptr, ::JldFile, x::String, ::JldWriteSession) =
unsafe_store!(convert(Ptr{Ptr{UInt8}}, out), pointer(x))

if !(isdefined(Core, :String) && isdefined(Core, :AbstractString))
Expand Down Expand Up @@ -184,7 +185,8 @@ function h5type(parent::JldFile, ::Type{UTF16String}, commit::Bool)
commit ? commit_datatype(parent, dtype, UTF16String) : JldDatatype(dtype, -1)
end

h5convert!(out::Ptr, ::JldFile, x::UTF16String, ::JldWriteSession) =
# no-inline needed to ensure gc-root for x
@noinline h5convert!(out::Ptr, ::JldFile, x::UTF16String, ::JldWriteSession) =
unsafe_store!(convert(Ptr{HDF5.Hvl_t}, out), HDF5.Hvl_t(length(x.data), pointer(x.data)))

function jlconvert(::Type{UTF16String}, ::JldFile, ptr::Ptr)
Expand Down
3 changes: 1 addition & 2 deletions test/jldtests.jl
Original file line number Diff line number Diff line change
Expand Up @@ -791,7 +791,7 @@ end # compress in (false,true)
module JLDTemp1
using Compat: @compat
using JLD
import ..fn, Core.Intrinsics.box
import ..fn

mutable struct TestType1
x::Int
Expand Down Expand Up @@ -846,7 +846,6 @@ struct TestType3
x::TestType1
end

import Core.Intrinsics.box, Core.Intrinsics.unbox
jldopen(fn, "r") do file
@test_throws JLD.TypeMismatchException read(file, "x1")
@test_throws MethodError read(file, "x2")
Expand Down
3 changes: 1 addition & 2 deletions test/type_translation.jl
Original file line number Diff line number Diff line change
Expand Up @@ -33,10 +33,9 @@ mutable struct MyOldType
a::Int
end

translate("MyType", "MyOldType")
translate("MyType", "Translation.Reading.MyOldType")

t = jldopen(filename) do file
truncate_module_path(file, Reading)
read(file, "x")
end

Expand Down

0 comments on commit 41ac60f

Please sign in to comment.