Simple/naive CodeInfo validation pass #22938

jrevels · 2017-07-24T16:24:47Z

This PR implements validate_code_info(c::CodeInfo), a barebones IR validation pass that returns 0 if c is valid or a positive integer error code otherwise. I can change the return convention to whatever we want; returning integers was just the easiest way to get started.

The validation pass is in its own file, which is included from Base.Core.Inference. Is inference the right location for this?

I'm also not sure where exactly validate_code_info should be called.

ref #22440

vtjnash · 2017-07-24T16:29:27Z

I can change the return convention to whatever we want; returning integers was just the easiest way to get started.

Perhaps Vector{Exception}? (The IR is valid if isempty(errors).)

JeffBezanson · 2017-07-24T16:46:45Z

base/codeinfovalidation.jl

+# This file is a part of Julia. License is MIT: https://julialang.org/license
+
+const VALID_EXPR_HEADS = Symbol[:call, :invoke, :static_parameter, :line, :gotoifnot, :(=),
+ :method, :const, :null, :new, :return, :the_exception,


Another possible rule: method is only valid in top-level code (i.e. inside a function with no arguments).

JeffBezanson · 2017-07-24T16:49:41Z

base/codeinfovalidation.jl

+"""
+function validate_code_info(c::CodeInfo)
+ !(c.inferred) && (c.slottypes != nothing) && return 5
+ (length(c.slotnames) < 1 || c.slotnames[1] != Symbol("#self#")) && return 8


The first slot can have a different name for code like (f::FuncType)(...) = ....

JeffBezanson · 2017-07-24T16:53:50Z

base/codeinfovalidation.jl

+ nslots = length(slotnums)
+ nssavals = length(ssavals)
+ length(c.slotflags) != nslots && return 1
+ length(c.slotnames) != nslots && return 2


These checks are too strict. I don't think every slot needs to be referenced. But should check length(c.slotflags) == length(c.slotnames).

JeffBezanson · 2017-07-24T16:57:12Z

base/codeinfovalidation.jl

+ elseif x.head == :(=) && !(is_valid_lhs(x.args[1]))
+ error_code = 9
+ return true
+ elseif x.head == :call && !(all(is_valid_call_arg(i) for i in x.args[2:end]))


is_valid_call_arg should also be checked on the argument to :gotoifnot exprs.

JeffBezanson · 2017-07-24T16:57:59Z

base/codeinfovalidation.jl

+
+is_valid_lhs(lhs) = isa(lhs, SlotNumber) || isa(lhs, SSAValue) || isa(lhs, GlobalRef)
+
+is_valid_call_arg(arg) = !(isa(arg, Expr)) || arg.head != :gotoifnot


Many more things are invalid here, e.g. LineNumberNode, LabelNode, GotoNode, various other expr heads.

Yeah, this was just a start. I'll add the ones you listed.

Maybe someday we can just replace this function with isa(arg, SSAValue) || isa(arg, SlotNumber) 😛

JeffBezanson · 2017-07-24T17:01:17Z

Cool! This will be nice to have.

I think there should be an additional method for this that accepts a Method object, in order to validate that nargs and isva are consistent with the type signature and CodeInfo.

ararslan · 2017-07-24T17:19:08Z

base/codeinfovalidation.jl

+ error_code = 0
+ walkast(c.code) do x
+ if isa(x, Expr)
+ if !(in(x.head, VALID_EXPR_HEADS))


Super minor point, but parentheses aren't required after ! when the condition isn't a compound expression of some kind. For consistency with the rest of Base it might be nice to just use !in(x.head, VALID_EXPR_HEADS) and likewise elsewhere in here.

jrevels · 2017-07-24T21:11:31Z

I believe the latest commits implement all of the comments here besides validate_code_info(::Method) and the :method head check (which I'm going to tackle next). Might be buggy since I haven't written tests yet.

Biggest change so far was @vtjnash's suggestion. Now, validate_code_info no longer returns immediately after finding a bug, but keeps chugging along, pushing uncovered bugs into a vector that gets returned at the end. Wasn't sure if performance is a big deal here or not; if so, CodeInfoError can be changed to store indices into a preallocated message table instead of taking in a string (assuming those allocations don't already get compiled away - I thought at one point they didn't, but that might be fixed by now).

jrevels · 2017-07-26T21:03:13Z

Alright, added some tests that pass locally (let's see how Travis fares...).

Still have these questions from the OP:

The validation pass is in its own file, which is included from Base.Core.Inference. Is inference the right location for this?

I'm also not sure where exactly validate_code_info should be called.

jrevels · 2017-07-31T14:54:25Z

Anything left to be done here besides hooking up a call to this thing somewhere (presumably somewhere in inference, see my question above)?

Tests pass locally, but I'll rebase to see if we can't get the CI badges green...

tkelman · 2017-07-31T22:15:43Z

base/codevalidation.jl

+InvalidCodeError(errno, msg) = InvalidCodeError(errno, msg, nothing)
+
+"""
+ validate_code!(errors::Vector{>:InvalidCodeError}, c::CodeInfo)


I'm confused, why is this a vector of any supertype of InvalidCodeError?

Because the element type can be "anything", as long as I can push objects of type InvalidCodeError into it...at least that was my intent, did I screw something up?

tkelman · 2017-08-01T08:44:30Z

base/codevalidation.jl

+"""
+function validate_code!(errors::Vector{>:InvalidCodeError}, m::Method)
+ if length(m.sig.parameters) != m.nargs
+ push!(errors, InvalidCodeError(11, "number of types in method signature does not match number of arguments", (length(m.sig.parameters), m.nargs)))


wrap long lines

tkelman · 2017-08-01T08:45:13Z

base/codevalidation.jl

+ push!(errors, InvalidCodeError(7, "not all SSAValues in AST have a type in ssavaluetypes", missing))
+ end
+ else
+ if c.slottypes != nothing


tkelman · 2017-08-01T08:50:47Z

test/inference.jl

+
+# InvalidCodeError 13: encountered Expr head `:method` in non-top-level code
+
+# TODO: This is a tough case to test an isolation...


in isolation

JeffBezanson · 2017-08-01T18:23:06Z

base/codevalidation.jl

+- `h != :method` for any subexpression head `h` if `m.nargs > 0`
+"""
+function validate_code!(errors::Vector{>:InvalidCodeError}, m::Method)
+ if length(m.sig.parameters) != m.nargs


Need Base.unwrap_unionall(m.sig).parameters.

JeffBezanson · 2017-08-01T18:24:53Z

base/codevalidation.jl

+ msg = "number of types in method signature does not match number of arguments"
+ push!(errors, InvalidCodeError(11, msg, (length(m.sig.parameters), m.nargs)))
+ end
+ if m.isva != (last(m.sig.parameters) <: Vararg{Any})


This should use Base.isvatuple. Vararg is not a first-class type and this check will become an error eventually.

These two tests aren't quite correct. If isva is true, at most you can say about length(unwrap_unionall(m.sig.parameters)) is that it is >= m.nargs - 1. Otherwise, it is true that they must be exactly equal.

JeffBezanson · 2017-08-01T18:26:53Z

base/codevalidation.jl

+ if !in(x.head, VALID_EXPR_HEADS)
+ push!(errors, InvalidCodeError(1, "encountered invalid expression head", x.head))
+ elseif x.head == :(=) && !is_valid_lhs(x.args[1])
+ push!(errors, InvalidCodeError(2, "encountered invalid LHS value", x.args[1]))


is_valid_call_arg can be renamed is_valid_rvalue, and also used to check the RHS of assignments here.

JeffBezanson · 2017-08-01T18:28:33Z

base/codevalidation.jl

+- all assigned-to slots have bit flag 2 set in their respective slotflags
+"""
+function validate_code!(errors::Vector{>:InvalidCodeError}, c::CodeInfo)
+ ssavals = SSAValue[]


Maybe this should be a Set?

JeffBezanson · 2017-08-01T18:31:47Z

I don't feel a strong need to call this anywhere by default (except maybe in a debug build?). More stuff that could potentially slow down the compiler further is the last thing we need.

vtjnash · 2017-08-01T18:18:59Z

base/codevalidation.jl

+ push!(errors, InvalidCodeError(1, "encountered invalid expression head", x.head))
+ elseif x.head == :(=) && !is_valid_lhs(x.args[1])
+ push!(errors, InvalidCodeError(2, "encountered invalid LHS value", x.args[1]))
+ elseif x.head == :call


recurse on invoke here too

vtjnash · 2017-08-01T19:22:54Z

base/codevalidation.jl

+ msg = "number of types in method signature does not match number of arguments"
+ push!(errors, InvalidCodeError(11, msg, (length(m.sig.parameters), m.nargs)))
+ end
+ if m.isva != (last(m.sig.parameters) <: Vararg{Any})


These two tests aren't quite correct. If isva is true, at most you can say about length(unwrap_unionall(m.sig.parameters)) is that it is >= m.nargs - 1. Otherwise, it is true that they must be exactly equal.

jrevels · 2017-08-01T21:49:50Z

Okay, I did a refactor to implement the suggestions here (thanks for the review!) and some suggestions Jameson gave me offline. The tests are only half-updated to reflect the new changes; I'll update them/add more tests tomorrow (and probably move them to their own file).

except maybe in a debug build?

I'm down to add that; what's the right way to do so?

jrevels · 2017-08-02T19:42:31Z

Took a stab at enabling the validation pass for debug builds. If enabled, it just prints any encountered invalidities as warnings; we can switch to throwing errors later if we want. Do we test debug builds as part of CI?

Also, I updated the tests so that they pass locally and moved them to their own file.

JeffBezanson · 2017-08-06T18:48:11Z

base/codevalidation.jl

+# This file is a part of Julia. License is MIT: https://julialang.org/license
+
+# Expr head => argument count bounds
+const VALID_EXPR_HEADS = Pair{Symbol,UnitRange{Int}}[


Should this be a Dict?

It originally was, before I realized that Dict isn't defined yet. Core.Inference is a strange, strange world 😛

Oh, right. ObjectIdDict should work though.

jrevels · 2017-08-08T21:43:31Z

Finally got around to running the full test suite on the debug build. After fixing some bugs, the only InvalidCodeError warnings I got were all encountered Expr head :method in non-top-level code (i.e. nargs > 0).

It seems like a bunch of these warnings were triggered by tests like this one:

# issue #15809 --- TODO: this code should be disallowed
function f15809()
    global g15809
    g15809(x::T) where {T} = T
end
f15809()
@test g15809(2) === Int

Given the TODO there, I'm assuming the validator was actually correct? Might be good for somebody to give that constraint one last check.

Otherwise, I think this is good to go.

jrevels · 2017-08-10T16:58:10Z

Given that tests are passing and it's been a few days, I'll plan on merging this tomorrow (barring anybody finding any other issues).

jrevels added the needs tests Unit tests are required for this change label Jul 24, 2017

JeffBezanson reviewed Jul 24, 2017

View reviewed changes

ararslan reviewed Jul 24, 2017

View reviewed changes

jrevels removed the needs tests Unit tests are required for this change label Jul 26, 2017

jrevels force-pushed the jr/irvalidator branch from 28c9478 to 9babcb1 Compare July 27, 2017 11:50

jrevels force-pushed the jr/irvalidator branch from 9babcb1 to 20d6e10 Compare July 31, 2017 14:55

jrevels mentioned this pull request Jul 31, 2017

allow CodeInfo to be returned directly from generated function generator #22440

Merged

tkelman reviewed Jul 31, 2017

View reviewed changes

tkelman reviewed Aug 1, 2017

View reviewed changes

JeffBezanson reviewed Aug 1, 2017

View reviewed changes

vtjnash reviewed Aug 1, 2017

View reviewed changes

jrevels force-pushed the jr/irvalidator branch from d59bc59 to a7e764f Compare August 2, 2017 17:17

jrevels force-pushed the jr/irvalidator branch from c0ba518 to 6023667 Compare August 3, 2017 12:54

JeffBezanson reviewed Aug 6, 2017

View reviewed changes

jrevels added 21 commits August 7, 2017 16:17

mock out naive IR validator

481def5

add more validity constraints

5c416d3

implement validate_code_info

8d7ab48

small bug fixes

36bdc75

clarify nslots in docs

e80cc25

remove parens from negated conditionals

ff0d431

fix some validation conditions

e36dcc8

return vector of exceptions rather than interger error codes

8d5023c

add Method validation on top of CodeInfo validation

114bfe4

get started on a couple tests

44b1f6e

add more code validation tests

a92f6f4

minor touch-ups

4f86522

implement suggestions from Jeff and Jameson

dcec775

enable CodeInfo validation as part of inference for debug builds

256e39e

update CodeInfo validation tests and move them to their own file

d3dc450

do not use warn in Inference since it doesn't exist yet

369ea17

add codevalidation to choosetests.jl

3da5862

use imperative voice for codevalidation docstrings

19e1474

fix validator bugs

3eee591

use ObjectIdDict for VALID_EXPR_HEADS

6d4c38d

fix overly strict nargs validation check

c639056

jrevels force-pushed the jr/irvalidator branch from 72946aa to c639056 Compare August 7, 2017 20:17

jrevels added 3 commits August 7, 2017 16:24

fix typo

300e433

better message for invalid code warning

657bff9

fix varargs validation bug + tests

6b9c445

jrevels merged commit 1fcc47c into master Aug 11, 2017

jrevels deleted the jr/irvalidator branch August 11, 2017 15:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Simple/naive CodeInfo validation pass #22938

Simple/naive CodeInfo validation pass #22938

jrevels commented Jul 24, 2017

vtjnash commented Jul 24, 2017

JeffBezanson Jul 24, 2017

JeffBezanson Jul 24, 2017

JeffBezanson Jul 24, 2017

JeffBezanson Jul 24, 2017

JeffBezanson Jul 24, 2017

jrevels Jul 24, 2017

JeffBezanson commented Jul 24, 2017

ararslan Jul 24, 2017

jrevels commented Jul 24, 2017

jrevels commented Jul 26, 2017

jrevels commented Jul 31, 2017 •

edited

Loading

tkelman Jul 31, 2017

jrevels Jul 31, 2017

tkelman Aug 1, 2017

tkelman Aug 1, 2017

tkelman Aug 1, 2017

JeffBezanson Aug 1, 2017

JeffBezanson Aug 1, 2017

vtjnash Aug 1, 2017

JeffBezanson Aug 1, 2017

JeffBezanson Aug 1, 2017

JeffBezanson commented Aug 1, 2017

vtjnash Aug 1, 2017

vtjnash Aug 1, 2017

jrevels commented Aug 1, 2017

jrevels commented Aug 2, 2017

JeffBezanson Aug 6, 2017

jrevels Aug 7, 2017

JeffBezanson Aug 7, 2017

jrevels commented Aug 8, 2017

jrevels commented Aug 10, 2017


		is_valid_lhs(lhs) = isa(lhs, SlotNumber) \|\| isa(lhs, SSAValue) \|\| isa(lhs, GlobalRef)

		is_valid_call_arg(arg) = !(isa(arg, Expr)) \|\| arg.head != :gotoifnot


		# InvalidCodeError 13: encountered Expr head `:method` in non-top-level code

		# TODO: This is a tough case to test an isolation...

Simple/naive CodeInfo validation pass #22938

Simple/naive CodeInfo validation pass #22938

Conversation

jrevels commented Jul 24, 2017

vtjnash commented Jul 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JeffBezanson commented Jul 24, 2017

Choose a reason for hiding this comment

jrevels commented Jul 24, 2017

jrevels commented Jul 26, 2017

jrevels commented Jul 31, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JeffBezanson commented Aug 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrevels commented Aug 1, 2017

jrevels commented Aug 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrevels commented Aug 8, 2017

jrevels commented Aug 10, 2017

jrevels commented Jul 31, 2017 •

edited

Loading