interp: improve error reporting

This commit improves error reporting in several ways:

  * Location information is read from the intruction that causes the
    error, as far as that's available.
  * The package that is being interpreted is included in the error
    message. This may be the most useful part of the improvements.
  * The hashmap update intrinsics now doesn't panic, instead it logs a
    clear error (with location information, as in the above two bullet
    points).

This is possible thanks to improvements in LLVM 9. This means that after
this change, TinyGo will depend on LLVM 9.

Author: aykevl

go.mod

@@ -10,5 +10,5 @@ require (
 	go.bug.st/serial.v1 v0.0.0-20180827123349-5f7892a7bb45
 	golang.org/x/sys v0.0.0-20191010194322-b09406accb47 // indirect
 	golang.org/x/tools v0.0.0-20190227180812-8dcc6e70cdef
-	tinygo.org/x/go-llvm v0.0.0-20191113125529-bad6d01809e8
+	tinygo.org/x/go-llvm v0.0.0-20191124211856-b2db3df3f257
 )

go.sum

@@ -28,3 +28,5 @@ tinygo.org/x/go-llvm v0.0.0-20191103200204-37e93e3f04e2 h1:Q5Hv3e5cLMGkiYwYgZL1Z
 tinygo.org/x/go-llvm v0.0.0-20191103200204-37e93e3f04e2/go.mod h1:fv1F0BSNpxMfCL0zF3M4OPFbgYHnhtB6ST0HvUtu/LE=
 tinygo.org/x/go-llvm v0.0.0-20191113125529-bad6d01809e8 h1:9Bfvso+tTVQg16UzOA614NaYA4x8vsRBNtd3eBrXwp0=
 tinygo.org/x/go-llvm v0.0.0-20191113125529-bad6d01809e8/go.mod h1:fv1F0BSNpxMfCL0zF3M4OPFbgYHnhtB6ST0HvUtu/LE=
+tinygo.org/x/go-llvm v0.0.0-20191124211856-b2db3df3f257 h1:o8VDylrMN7gWemBMu8rEyuogKPhcLTdx5KrUAp9macc=
+tinygo.org/x/go-llvm v0.0.0-20191124211856-b2db3df3f257/go.mod h1:fv1F0BSNpxMfCL0zF3M4OPFbgYHnhtB6ST0HvUtu/LE=

interp/errors.go

@@ -3,15 +3,89 @@ package interp
 // This file provides useful types for errors encountered during IR evaluation.
 
 import (
+	"errors"
+	"go/scanner"
+	"go/token"
+	"path/filepath"
+
 	"tinygo.org/x/go-llvm"
 )
 
+// errUnreachable is returned when an unreachable instruction is executed. This
+// error should not be visible outside of the interp package.
+var errUnreachable = errors.New("interp: unreachable executed")
+
+// Unsupported is the specific error that is returned when an unsupported
+// instruction is hit while trying to interpret all initializers.
 type Unsupported struct {
-	Inst llvm.Value
+	ImportPath string
+	Inst       llvm.Value
+	Pos        token.Position
 }
 
 func (e Unsupported) Error() string {
 	// TODO: how to return the actual instruction string?
 	// It looks like LLVM provides no function for that...
-	return "interp: unsupported instruction"
+	return scanner.Error{
+		Pos: e.Pos,
+		Msg: "interp: unsupported instruction",
+	}.Error()
+}
+
+// unsupportedInstructionError returns a new "unsupported instruction" error for
+// the given instruction. It includes source location information, when
+// available.
+func (e *evalPackage) unsupportedInstructionError(inst llvm.Value) *Unsupported {
+	return &Unsupported{
+		ImportPath: e.packagePath,
+		Inst:       inst,
+		Pos:        getPosition(inst),
+	}
+}
+
+// Error encapsulates compile-time interpretation errors with an associated
+// import path. The errors may not have a precise location attached.
+type Error struct {
+	ImportPath string
+	Errs       []scanner.Error
+}
+
+// Error returns the string of the first error in the list of errors.
+func (e Error) Error() string {
+	return e.Errs[0].Error()
+}
+
+// errorAt returns an error value for the currently interpreted package at the
+// location of the instruction. The location information may not be complete as
+// it depends on debug information in the IR.
+func (e *evalPackage) errorAt(inst llvm.Value, msg string) Error {
+	return Error{
+		ImportPath: e.packagePath,
+		Errs:       []scanner.Error{errorAt(inst, msg)},
+	}
+}
+
+// errorAt returns an error value at the location of the instruction.
+// The location information may not be complete as it depends on debug
+// information in the IR.
+func errorAt(inst llvm.Value, msg string) scanner.Error {
+	return scanner.Error{
+		Pos: getPosition(inst),
+		Msg: msg,
+	}
+}
+
+// getPosition returns the position information for the given instruction, as
+// far as it is available.
+func getPosition(inst llvm.Value) token.Position {
+	loc := inst.InstructionDebugLoc()
+	if loc.IsNil() {
+		return token.Position{}
+	}
+	file := loc.LocationScope().ScopeFile()
+	return token.Position{
+		Filename: filepath.Join(file.FileDirectory(), file.FileFilename()),
+		Line:     int(loc.LocationLine()),
+		Column:   int(loc.LocationColumn()),
+	}
 }

interp/frame.go

@@ -4,21 +4,17 @@ package interp
 // functions.
 
 import (
-	"errors"
 	"strings"
 
 	"tinygo.org/x/go-llvm"
 )
 
 type frame struct {
-	*Eval
-	fn      llvm.Value
-	pkgName string
-	locals  map[llvm.Value]Value
+	*evalPackage
+	fn     llvm.Value
+	locals map[llvm.Value]Value
 }
 
-var ErrUnreachable = errors.New("interp: unreachable executed")
-
 // evalBasicBlock evaluates a single basic block, returning the return value (if
 // ending with a ret instruction), a list of outgoing basic blocks (if not
 // ending with a ret instruction), or an error on failure.
@@ -79,13 +75,13 @@ func (fr *frame) evalBasicBlock(bb, incoming llvm.BasicBlock, indent string) (re
 				fr.locals[inst] = &LocalValue{fr.Eval, fr.builder.CreateXor(lhs, rhs, "")}
 
 			default:
-				return nil, nil, &Unsupported{inst}
+				return nil, nil, fr.unsupportedInstructionError(inst)
 			}
 
 		// Memory operators
 		case !inst.IsAAllocaInst().IsNil():
 			allocType := inst.Type().ElementType()
-			alloca := llvm.AddGlobal(fr.Mod, allocType, fr.pkgName+"$alloca")
+			alloca := llvm.AddGlobal(fr.Mod, allocType, fr.packagePath+"$alloca")
 			alloca.SetInitializer(llvm.ConstNull(allocType))
 			alloca.SetLinkage(llvm.InternalLinkage)
 			fr.locals[inst] = &LocalValue{
@@ -247,12 +243,12 @@ func (fr *frame) evalBasicBlock(bb, incoming llvm.BasicBlock, indent string) (re
 				if size != typeSize {
 					// allocate an array
 					if size%typeSize != 0 {
-						return nil, nil, &Unsupported{inst}
+						return nil, nil, fr.unsupportedInstructionError(inst)
 					}
 					elementCount = int(size / typeSize)
 					allocType = llvm.ArrayType(allocType, elementCount)
 				}
-				alloc := llvm.AddGlobal(fr.Mod, allocType, fr.pkgName+"$alloc")
+				alloc := llvm.AddGlobal(fr.Mod, allocType, fr.packagePath+"$alloc")
 				alloc.SetInitializer(llvm.ConstNull(allocType))
 				alloc.SetLinkage(llvm.InternalLinkage)
 				result := &LocalValue{
@@ -270,13 +266,16 @@ func (fr *frame) evalBasicBlock(bb, incoming llvm.BasicBlock, indent string) (re
 				valueSize := inst.Operand(1).ZExtValue()
 				fr.locals[inst] = &MapValue{
 					Eval:      fr.Eval,
-					PkgName:   fr.pkgName,
+					PkgName:   fr.packagePath,
 					KeySize:   int(keySize),
 					ValueSize: int(valueSize),
 				}
 			case callee.Name() == "runtime.hashmapStringSet":
 				// set a string key in the map
-				m := fr.getLocal(inst.Operand(0)).(*MapValue)
+				m, ok := fr.getLocal(inst.Operand(0)).(*MapValue)
+				if !ok {
+					return nil, nil, fr.errorAt(inst, "could not update map with string key")
+				}
 				// "key" is a Go string value, which in the TinyGo calling convention is split up
 				// into separate pointer and length parameters.
 				keyBuf := fr.getLocal(inst.Operand(1)).(*LocalValue)
@@ -285,7 +284,10 @@ func (fr *frame) evalBasicBlock(bb, incoming llvm.BasicBlock, indent string) (re
 				m.PutString(keyBuf, keyLen, valPtr)
 			case callee.Name() == "runtime.hashmapBinarySet":
 				// set a binary (int etc.) key in the map
-				m := fr.getLocal(inst.Operand(0)).(*MapValue)
+				m, ok := fr.getLocal(inst.Operand(0)).(*MapValue)
+				if !ok {
+					return nil, nil, fr.errorAt(inst, "could not update map")
+				}
 				keyBuf := fr.getLocal(inst.Operand(1)).(*LocalValue)
 				valPtr := fr.getLocal(inst.Operand(2)).(*LocalValue)
 				m.PutBinary(keyBuf, valPtr)
@@ -304,7 +306,7 @@ func (fr *frame) evalBasicBlock(bb, incoming llvm.BasicBlock, indent string) (re
 				}
 				globalType := llvm.ArrayType(fr.Mod.Context().Int8Type(), len(result))
 				globalValue := llvm.ConstArray(fr.Mod.Context().Int8Type(), vals)
-				global := llvm.AddGlobal(fr.Mod, globalType, fr.pkgName+"$stringconcat")
+				global := llvm.AddGlobal(fr.Mod, globalType, fr.packagePath+"$stringconcat")
 				global.SetInitializer(globalValue)
 				global.SetLinkage(llvm.InternalLinkage)
 				global.SetGlobalConstant(true)
@@ -336,20 +338,20 @@ func (fr *frame) evalBasicBlock(bb, incoming llvm.BasicBlock, indent string) (re
 					srcArray = srcArray.GetElementPtr([]uint32{0, 0}).(*LocalValue)
 				}
 				if fr.Eval.TargetData.TypeAllocSize(dstArray.Type().ElementType()) != elementSize {
-					return nil, nil, errors.New("interp: slice dst element size does not match pointer type")
+					return nil, nil, fr.errorAt(inst, "interp: slice dst element size does not match pointer type")
 				}
 				if fr.Eval.TargetData.TypeAllocSize(srcArray.Type().ElementType()) != elementSize {
-					return nil, nil, errors.New("interp: slice src element size does not match pointer type")
+					return nil, nil, fr.errorAt(inst, "interp: slice src element size does not match pointer type")
 				}
 				if dstArray.Type() != srcArray.Type() {
-					return nil, nil, errors.New("interp: slice element types don't match")
+					return nil, nil, fr.errorAt(inst, "interp: slice element types don't match")
 				}
 				length := dstLen.Value().SExtValue()
 				if srcLength := srcLen.Value().SExtValue(); srcLength < length {
 					length = srcLength
 				}
 				if length < 0 {
-					return nil, nil, errors.New("interp: trying to copy a slice with negative length?")
+					return nil, nil, fr.errorAt(inst, "interp: trying to copy a slice with negative length?")
 				}
 				for i := int64(0); i < length; i++ {
 					// *dst = *src
@@ -370,7 +372,7 @@ func (fr *frame) evalBasicBlock(bb, incoming llvm.BasicBlock, indent string) (re
 				}
 				globalType := llvm.ArrayType(fr.Mod.Context().Int8Type(), len(result))
 				globalValue := llvm.ConstArray(fr.Mod.Context().Int8Type(), vals)
-				global := llvm.AddGlobal(fr.Mod, globalType, fr.pkgName+"$bytes")
+				global := llvm.AddGlobal(fr.Mod, globalType, fr.packagePath+"$bytes")
 				global.SetInitializer(globalValue)
 				global.SetLinkage(llvm.InternalLinkage)
 				global.SetGlobalConstant(true)
@@ -489,7 +491,7 @@ func (fr *frame) evalBasicBlock(bb, incoming llvm.BasicBlock, indent string) (re
 					//     compile time.
 					//   * Unbounded: cannot call at runtime so we'll try to
 					//     interpret anyway and hope for the best.
-					ret, err = fr.function(callee, params, fr.pkgName, indent+"    ")
+					ret, err = fr.function(callee, params, indent+"    ")
 					if err != nil {
 						return nil, nil, err
 					}
@@ -499,7 +501,7 @@ func (fr *frame) evalBasicBlock(bb, incoming llvm.BasicBlock, indent string) (re
 				}
 			default:
 				// function pointers, etc.
-				return nil, nil, &Unsupported{inst}
+				return nil, nil, fr.unsupportedInstructionError(inst)
 			}
 		case !inst.IsAExtractValueInst().IsNil():
 			agg := fr.getLocal(inst.Operand(0)).(*LocalValue) // must be constant
@@ -509,7 +511,7 @@ func (fr *frame) evalBasicBlock(bb, incoming llvm.BasicBlock, indent string) (re
 				fr.locals[inst] = fr.getValue(newValue)
 			} else {
 				if len(indices) != 1 {
-					return nil, nil, errors.New("cannot handle extractvalue with not exactly 1 index")
+					return nil, nil, fr.errorAt(inst, "interp: cannot handle extractvalue with not exactly 1 index")
 				}
 				fr.locals[inst] = &LocalValue{fr.Eval, fr.builder.CreateExtractValue(agg.Underlying, int(indices[0]), inst.Name())}
 			}
@@ -522,7 +524,7 @@ func (fr *frame) evalBasicBlock(bb, incoming llvm.BasicBlock, indent string) (re
 				fr.locals[inst] = &LocalValue{fr.Eval, newValue}
 			} else {
 				if len(indices) != 1 {
-					return nil, nil, errors.New("cannot handle insertvalue with not exactly 1 index")
+					return nil, nil, fr.errorAt(inst, "interp: cannot handle insertvalue with not exactly 1 index")
 				}
 				fr.locals[inst] = &LocalValue{fr.Eval, fr.builder.CreateInsertValue(agg.Underlying, val.Value(), int(indices[0]), inst.Name())}
 			}
@@ -540,12 +542,12 @@ func (fr *frame) evalBasicBlock(bb, incoming llvm.BasicBlock, indent string) (re
 			thenBB := inst.Operand(1)
 			elseBB := inst.Operand(2)
 			if !cond.IsAInstruction().IsNil() {
-				return nil, nil, errors.New("interp: branch on a non-constant")
+				return nil, nil, fr.errorAt(inst, "interp: branch on a non-constant")
 			}
 			if !cond.IsAConstantExpr().IsNil() {
 				// This may happen when the instruction builder could not
 				// const-fold some instructions.
-				return nil, nil, errors.New("interp: branch on a non-const-propagated constant expression")
+				return nil, nil, fr.errorAt(inst, "interp: branch on a non-const-propagated constant expression")
 			}
 			switch cond {
 			case llvm.ConstInt(fr.Mod.Context().Int1Type(), 0, false): // false
@@ -561,10 +563,11 @@ func (fr *frame) evalBasicBlock(bb, incoming llvm.BasicBlock, indent string) (re
 		case !inst.IsAUnreachableInst().IsNil():
 			// Unreachable was reached (e.g. after a call to panic()).
 			// Report this as an error, as it is not supposed to happen.
-			return nil, nil, ErrUnreachable
+			// This is a sentinel error value.
+			return nil, nil, errUnreachable
 
 		default:
-			return nil, nil, &Unsupported{inst}
+			return nil, nil, fr.unsupportedInstructionError(inst)
 		}
 	}
 

interp/interp.go

@@ -7,7 +7,6 @@ package interp
 // methods.
 
 import (
-	"errors"
 	"strings"
 
 	"tinygo.org/x/go-llvm"
@@ -22,6 +21,15 @@ type Eval struct {
 	sideEffectFuncs map[llvm.Value]*sideEffectResult // cache of side effect scan results
 }
 
+// evalPackage encapsulates the Eval type for just a single package. The Eval
+// type keeps state across the whole program, the evalPackage type keeps extra
+// state for the currently interpreted package.
+type evalPackage struct {
+	*Eval
+	packagePath string
+	errors      []error
+}
+
 // Run evaluates the function with the given name and then eliminates all
 // callers.
 func Run(mod llvm.Module, debug bool) error {
@@ -56,7 +64,7 @@ func Run(mod llvm.Module, debug bool) error {
 			break // ret void
 		}
 		if inst.IsACallInst().IsNil() || inst.CalledValue().IsAFunction().IsNil() {
-			return errors.New("expected all instructions in " + name + " to be direct calls")
+			return errorAt(inst, "interp: expected all instructions in "+name+" to be direct calls")
 		}
 		initCalls = append(initCalls, inst)
 	}
@@ -66,13 +74,17 @@ func Run(mod llvm.Module, debug bool) error {
 	for _, call := range initCalls {
 		initName := call.CalledValue().Name()
 		if !strings.HasSuffix(initName, ".init") {
-			return errors.New("expected all instructions in " + name + " to be *.init() calls")
+			return errorAt(call, "interp: expected all instructions in "+name+" to be *.init() calls")
 		}
 		pkgName := initName[:len(initName)-5]
 		fn := call.CalledValue()
 		call.EraseFromParentAsInstruction()
-		_, err := e.Function(fn, []Value{&LocalValue{e, undefPtr}, &LocalValue{e, undefPtr}}, pkgName)
-		if err == ErrUnreachable {
+		evalPkg := evalPackage{
+			Eval:        e,
+			packagePath: pkgName,
+		}
+		_, err := evalPkg.function(fn, []Value{&LocalValue{e, undefPtr}, &LocalValue{e, undefPtr}}, "")
+		if err == errUnreachable {
 			break
 		}
 		if err != nil {
@@ -83,16 +95,14 @@ func Run(mod llvm.Module, debug bool) error {
 	return nil
 }
 
-func (e *Eval) Function(fn llvm.Value, params []Value, pkgName string) (Value, error) {
-	return e.function(fn, params, pkgName, "")
-}
-
-func (e *Eval) function(fn llvm.Value, params []Value, pkgName, indent string) (Value, error) {
+// function interprets the given function. The params are the function params
+// and the indent is the string indentation to use when dumping all interpreted
+// instructions.
+func (e *evalPackage) function(fn llvm.Value, params []Value, indent string) (Value, error) {
 	fr := frame{
-		Eval:    e,
-		fn:      fn,
-		pkgName: pkgName,
-		locals:  make(map[llvm.Value]Value),
+		evalPackage: e,
+		fn:          fn,
+		locals:      make(map[llvm.Value]Value),
 	}
 	for i, param := range fn.Params() {
 		fr.locals[param] = params[i]