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

Use Base.CoreLogging #78

Closed
wants to merge 3 commits into from
Closed

Use Base.CoreLogging #78

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d91b372
Use Base.CoreLogging
maleadt Dec 15, 2017
d37898e
Simplify debug checks.
maleadt Dec 15, 2017
a14d9e1
Properly parse booleans and enable debug testing on Travis.
maleadt Dec 15, 2017
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
4 changes: 4 additions & 0 deletions .travis.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,10 @@ os:
- linux
- osx

env:
- DEBUG=true
- DEBUG=false

julia:
- nightly

Expand Down
5 changes: 3 additions & 2 deletions deps/build.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
using Compat

include(joinpath(@__DIR__, "..", "src", "util", "logging.jl"))
using Logging
parse(Bool, get(ENV, "DEBUG", "false")) && global_logger(SimpleLogger(global_logger().stream, Logging.Debug))
Copy link

Choose a reason for hiding this comment

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

Right, there's not a system for configuration yet, but what you're doing here is a very common pattern for this kind of thing which needs some kind of support. Actually I'm not sure I'll even have time for doing something nice for configuration in 1.0 - there's still a lot of other arguably-breaking-but-probably-not-feature-frozen things which need straightening out in Base, while configuration can be done in a package.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sure, I can work with an env flag and global logger for now.


const config_path = joinpath(@__DIR__, "ext.jl")
const previous_config_path = config_path * ".bak"
Expand Down Expand Up @@ -39,7 +40,7 @@ function main()
joinpath(dirname(JULIA_HOME), "lib", "julia", libllvm_name)] # dists
end

debug("Looking for $(libllvm_name) in ", join(libllvm_paths, ", "))
@debug "Looking for $(libllvm_name) in $(join(libllvm_paths, ", "))"
filter!(isfile, libllvm_paths)
isempty(libllvm_paths) && error("Could not find $(libllvm_name), is Julia built with USE_LLVM_SHLIB=1?")
config[:libllvm_path] = first(libllvm_paths)
Expand Down
6 changes: 3 additions & 3 deletions src/LLVM.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,9 @@ __precompile__()

module LLVM

using Logging
parse(Bool, get(ENV, "DEBUG", "false")) && global_logger(SimpleLogger(global_logger().stream, Logging.Debug))

using Unicode
using Compat

Expand All @@ -10,7 +13,6 @@ isfile(ext) || error("LLVM.jl has not been built, please run Pkg.build(\"LLVM\")
include(ext)
const libllvm = libllvm_path

include("util/logging.jl")
include("util/types.jl")

include("base.jl")
Expand Down Expand Up @@ -63,8 +65,6 @@ if Compat.Sys.islinux()
end

function __init__()
__init_logging__()

_install_handlers()
_install_handlers(GlobalContext())

Expand Down
4 changes: 2 additions & 2 deletions src/core/context.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,9 +65,9 @@ function handle_diagnostic(diag_ref::API.LLVMDiagnosticInfoRef, args::Ptr{Void})
if sev == API.LLVMDSError
throw(LLVMException(msg))
elseif sev == API.LLVMDSWarning
warn(msg)
@warn msg
elseif sev == API.LLVMDSRemark || sev == API.LLVMDSNote
debug(msg)
@debug msg
else
error("unknown diagnostic severity level $sev")
end
Expand Down
6 changes: 2 additions & 4 deletions src/core/instructions.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,11 +12,9 @@ identify(::Type{Instruction}, ::Val{K}) where {K} = bug("Unknown instruction kin

@inline function check(::Type{T}, ref::API.LLVMValueRef) where T<:Instruction
ref==C_NULL && throw(NullException())
@static if DEBUG
@debug begin
T′ = identify(Instruction, ref)
if T != T′
error("invalid conversion of $T′ instruction reference to $T")
end
@assert T==T′ "invalid conversion of $T′ instruction reference to $T"
end
end

Expand Down
6 changes: 2 additions & 4 deletions src/core/type.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,11 +9,9 @@ identify(::Type{LLVMType}, ::Val{K}) where {K} = bug("Unknown type kind $K")

@inline function check(::Type{T}, ref::API.LLVMTypeRef) where T<:LLVMType
ref==C_NULL && throw(NullException())
@static if DEBUG
@debug begin
Copy link

Choose a reason for hiding this comment

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

This seems like an odd use of @debug to contain an @assert.

Really @debug is for emitting verbose, low priority messages which may be ignored. The expression passed to @debug should evaluate to a human-readable message.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hmm, I wanted to avoid (DRY) another global DEBUG setting that determines whether costly checks are enabled, and (ab)using @debug seemed like a prime candidate for that. I see now how it still prints something.

T′ = identify(LLVMType, ref)
if T != T′
error("invalid conversion of $T′ type reference to $T")
end
@assert T==T′ "invalid conversion of $T′ type reference to $T"
end
end

Expand Down
6 changes: 2 additions & 4 deletions src/core/value.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,11 +12,9 @@ identify(::Type{Value}, ::Val{K}) where {K} = bug("Unknown value kind $K")

@inline function check(::Type{T}, ref::API.LLVMValueRef) where T<:Value
ref==C_NULL && throw(NullException())
@static if DEBUG
@debug begin
T′ = identify(Value, ref)
if T != T′
error("invalid conversion of $T′ value reference to $T")
end
@assert T==T′ "invalid conversion of $T′ value reference to $T"
end
end

Expand Down
51 changes: 0 additions & 51 deletions src/util/logging.jl
View file
Open in desktop

This file was deleted.

4 changes: 2 additions & 2 deletions test/core.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ Context() do ctx
@test typeof(LLVM.ref(typ)) == LLVM.API.LLVMTypeRef # untyped

@test typeof(LLVM.IntegerType(LLVM.ref(typ))) == LLVM.IntegerType # type reconstructed
if LLVM.DEBUG
@debug begin # TODO: does this correspond with the loglevel of the LLVM module?
Copy link

Choose a reason for hiding this comment

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

This seems like the wrong use of @debug.

@test_throws ErrorException LLVM.FunctionType(LLVM.ref(typ)) # wrong type
end
@test_throws NullException LLVM.FunctionType(LLVM.API.LLVMTypeRef(C_NULL))
Expand Down Expand Up @@ -182,7 +182,7 @@ LLVM.Module("SomeModule", ctx) do mod
@test typeof(LLVM.ref(val)) == LLVM.API.LLVMValueRef # untyped

@test typeof(LLVM.Instruction(LLVM.ref(val))) == LLVM.AllocaInst # type reconstructed
if LLVM.DEBUG
@debug begin # TODO: does this correspond with the loglevel of the LLVM module?
@test_throws ErrorException LLVM.Function(LLVM.ref(val)) # wrong
end
@test_throws NullException LLVM.Function(LLVM.API.LLVMValueRef(C_NULL))
Expand Down