acpi: provide more robust implementation for parsing AML method bodies

The previous implementation used brute-force approach where the parser
made an initial pass scanning the AML bytestream and looking for method
declaration opcodes. It then parsed out the method name and arg count
and populated a map which was used to detect the number of arguments to
be parsed upon encountering a method invocation. This approach proved to
be error-prone and would lead to an incorrect parse tree in the
following ASL example:

  Method (FOOF, 1, NotSerialized)
  {
    Return ("bar")
  }

  Method (TST0, 0, NotSerialized)
  {
    FOOF(0)
    \_SB.FOOF(2, 3)
  }

  Scope(\_SB){
    // Another FOOF method in \_SB which takes a different arg count
    Method (FOOF, 2, NotSerialized)
    {
      Return ("something")
    }
  }

In the above example the parser would correctly parse the first FOOF
call in TST0 but fail to parse the second invocation since the method
name contains a scope. The second invocation would actually yield the
following incorrect entity list (arguments appear as sibling entities):

Ref(\_SB.FOOF), Const(2), Const(3)

The new approach gets rid of the brute-force method and instead modifies
the initial parse of the tree not to parse the entities in the AML
method bodies but to instead track the start and end offset in the
AML stream for the body contents. In the second pass (where the parser
normally resolves symbol references), the parser can properly parse the
contents of method bodies since the entire AML tree is now known and the
parser can use the regular scope lookup rules to find the correct method
declaration for the invocation and figure out the argument count it
needs to parse.

src/gopheros/device/acpi/aml/entity/entity.go

@@ -1,6 +1,8 @@
 package entity
 
-import "gopheros/kernel"
+import (
+	"gopheros/kernel"
+)
 
 // Entity is an interface implemented by all AML entities.
 type Entity interface {
@@ -731,6 +733,9 @@ type Method struct {
 	ArgCount   uint8
 	Serialized bool
 	SyncLevel  uint8
+
+	BodyStartOffset uint32
+	BodyEndOffset   uint32
 }
 
 // NewMethod creats a new AML method entity.
@@ -775,37 +780,21 @@ func (ent *Method) SetArg(argIndex uint8, arg interface{}) bool {
 type Invocation struct {
 	Generic
 
-	MethodName string
-	MethodDef  *Method
+	Method *Method
 }
 
 // NewInvocation creates a new method invocation object.
-func NewInvocation(tableHandle uint8, name string, args []interface{}) *Invocation {
+func NewInvocation(tableHandle uint8, method *Method, args []interface{}) *Invocation {
 	return &Invocation{
 		Generic: Generic{
 			op:          OpMethodInvocation,
 			tableHandle: tableHandle,
 			args:        args,
 		},
-		MethodName: name,
+		Method: method,
 	}
 }
 
-// ResolveSymbolRefs receives as input the root of the AML entity tree and
-// attempts to resolve any symbol references using the scope searching rules
-// defined by the ACPI spec.
-func (ent *Invocation) ResolveSymbolRefs(rootNS Container) *kernel.Error {
-	var ok bool
-	if ent.MethodDef, ok = FindInScope(ent.Parent(), rootNS, ent.MethodName).(*Method); !ok {
-		return &kernel.Error{
-			Module:  "acpi_aml_vm",
-			Message: "could not resolve method invocation to: " + ent.MethodName + "; parent: " + ent.Parent().Name(),
-		}
-	}
-
-	return nil
-}
-
 // Device defines an AML device entity.
 type Device struct {
 	Scope

src/gopheros/device/acpi/aml/entity/entity_test.go

@@ -25,7 +25,7 @@ func TestEntityMethods(t *testing.T) {
 		{NewBankField(42), OpBankField, ""},
 		{NewReference(42, "TRG0"), OpName, ""},
 		{NewMethod(42, "FOO0"), OpMethod, "FOO0"},
-		{NewInvocation(42, "MTH0", nil), OpMethodInvocation, ""},
+		{NewInvocation(42, NewMethod(42, "MTH0"), nil), OpMethodInvocation, ""},
 		{NewMutex(42), OpMutex, ""},
 		{NewDevice(42, "DEV0"), OpDevice, "DEV0"},
 		{NewProcessor(42, "CPU0"), OpProcessor, "CPU0"},
@@ -283,14 +283,6 @@ func TestLazySymbolResolver(t *testing.T) {
 			&Reference{TargetName: "FLD0"},
 			"",
 		},
-		{
-			&Invocation{MethodName: "UNKNOWN"},
-			`could not resolve method invocation to: UNKNOWN; parent: \`,
-		},
-		{
-			&Invocation{MethodName: "MTH0"},
-			"",
-		},
 	}
 
 	for specIndex, spec := range specs {

src/gopheros/device/acpi/aml/parser/opcode_table.go

@@ -142,6 +142,9 @@ type opcodeInfo struct {
 	argFlags opArgFlags
 }
 
+// Used by Parser.parseMethodBody for deferred parsing of method bodies.
+const methodOpInfoIndex = 0x0d
+
 // The opcode table contains all opcode-related information that the parser knows.
 // This table is modeled after a similar table used in the acpica implementation.
 var opcodeTable = []opcodeInfo{

src/gopheros/device/acpi/aml/parser/parser.go

@@ -14,6 +14,13 @@ var (
 	errResolvingEntities = &kernel.Error{Module: "acpi_aml_parser", Message: "AML bytecode contains unresolvable entities"}
 )
 
+type parseOpt uint8
+
+const (
+	parseOptSkipMethodBodies parseOpt = iota
+	parseOptParseMethodBodies
+)
+
 // Parser implements an AML parser.
 type Parser struct {
 	r           amlStreamReader
@@ -23,19 +30,14 @@ type Parser struct {
 	tableName   string
 	tableHandle uint8
 
-	// methodArgCount is initialized in a pre-parse step with the names and expected
-	// number of args for each function declaration. This is required as function
-	// invocations do not employ any mechanism to indicate the number of args that
-	// need to be parsed. Moreover, the spec allows for forward function declarations.
-	methodArgCount map[string]uint8
+	parseOptions parseOpt
 }
 
 // NewParser returns a new AML parser instance.
 func NewParser(errWriter io.Writer, rootEntity entity.Container) *Parser {
 	return &Parser{
-		errWriter:      errWriter,
-		root:           rootEntity,
-		methodArgCount: make(map[string]uint8),
+		errWriter: errWriter,
+		root:      rootEntity,
 	}
 }
 
@@ -51,12 +53,9 @@ func (p *Parser) ParseAML(tableHandle uint8, tableName string, header *table.SDT
 		uint32(unsafe.Sizeof(table.SDTHeader{})),
 	)
 
-	// Pass 1: scan bytecode and locate all method declarations. This allows us to
-	// properly parse the arguments to method invocations at pass 2 even if the
-	// the name of the invoked method is a forward reference.
-	p.detectMethodDeclarations()
-
-	// Pass 2: decode bytecode and build entitites
+	// Pass 1: decode bytecode and build entitites without recursing into
+	// function bodies.
+	p.parseOptions = parseOptSkipMethodBodies
 	p.scopeStack = nil
 	p.scopeEnter(p.root)
 	if !p.parseObjList(header.Length) {
@@ -66,11 +65,15 @@ func (p *Parser) ParseAML(tableHandle uint8, tableName string, header *table.SDT
 	}
 	p.scopeExit()
 
-	// Pass 3: check parents and resolve symbol references
+	// Pass 2: parse method bodies, check entity parents and resolve all
+	// symbol references
 	var resolveFailed bool
 	entity.Visit(0, p.root, entity.TypeAny, func(_ int, ent entity.Entity) bool {
-		// Skip method bodies; their contents will be lazily resolved by the interpreter
-		if _, isMethod := ent.(*entity.Method); isMethod {
+		if method, isMethod := ent.(*entity.Method); isMethod {
+			resolveFailed = resolveFailed || !p.parseMethodBody(method)
+
+			// Don't recurse into method bodies; their contents
+			// will be lazilly resolved by the VM
 			return false
 		}
 
@@ -101,57 +104,6 @@ func (p *Parser) ParseAML(tableHandle uint8, tableName string, header *table.SDT
 	return nil
 }
 
-// detectMethodDeclarations scans the AML byte-stream looking for function
-// declarations.  For each discovered function, the method will parse its flags
-// and update the methodArgCount map with the number of required arguments.
-func (p *Parser) detectMethodDeclarations() {
-	var (
-		next              *opcodeInfo
-		method            string
-		startOffset       = p.r.Offset()
-		curOffset, pkgLen uint32
-		flags             uint64
-		ok                bool
-	)
-
-	for !p.r.EOF() {
-		if next, ok = p.nextOpcode(); !ok {
-			// Skip one byte to the right and try again. Maybe we are stuck inside
-			// the contents of a string or buffer
-			_, _ = p.r.ReadByte()
-			continue
-		}
-
-		if next.op != entity.OpMethod {
-			continue
-		}
-
-		// Parse pkg len; if this fails then this is not a method declaration
-		curOffset = p.r.Offset()
-		if pkgLen, ok = p.parsePkgLength(); !ok {
-			continue
-		}
-
-		// Parse method name
-		if method, ok = p.parseNameString(); !ok {
-			continue
-		}
-
-		// The next byte encodes the method flags which also contains the arg count
-		// at bits 0:2
-		if flags, ok = p.parseNumConstant(1); !ok {
-			continue
-		}
-
-		p.methodArgCount[method] = uint8(flags) & 0x7
-
-		// At this point we can use the pkg length to skip over the term list
-		p.r.SetOffset(curOffset + pkgLen)
-	}
-
-	p.r.SetOffset(startOffset)
-}
-
 // parseObjList tries to parse an AML object list. Object lists are usually
 // specified together with a pkgLen block which is used to calculate the max
 // read offset that the parser may reach.
@@ -174,8 +126,7 @@ func (p *Parser) parseObj() bool {
 	)
 
 	// If we cannot decode the next opcode then this may be a method
-	// invocation or a name reference. If neither is the case, we need to
-	// rewind the stream and parse a method invocation before giving up.
+	// invocation or a name reference.
 	curOffset = p.r.Offset()
 	if info, ok = p.nextOpcode(); !ok {
 		p.r.SetOffset(curOffset)
@@ -352,6 +303,16 @@ func (p *Parser) parseArg(info *opcodeInfo, obj entity.Entity, argIndex uint8, a
 	case opArgTarget:
 		arg, ok = p.parseTarget()
 	case opArgTermList:
+		// If this is a method and the SkipMethodBodies option is set
+		// then record the body start and end offset so we can parse
+		// it at a later stage.
+		if method, isMethod := obj.(*entity.Method); isMethod && p.parseOptions == parseOptSkipMethodBodies {
+			method.BodyStartOffset = p.r.Offset()
+			method.BodyEndOffset = maxReadOffset
+			p.r.SetOffset(maxReadOffset)
+			return true
+		}
+
 		// If object is a scoped entity enter it's scope before parsing
 		// the term list. Otherwise, create an unnamed scope, attach it
 		// as the next argument to obj and enter that.
@@ -450,6 +411,25 @@ func (p *Parser) makeObjForOpcode(info *opcodeInfo) entity.Entity {
 	return obj
 }
 
+// parseMethodBody parses the entities that make up a method's body. After the
+// entire AML tree has been parsed, the parser makes a second pass and calls
+// parseMethodBody for each Method entity.
+//
+// By deferring the parsing of the method body, we ensure that the parser can
+// lookup the method declarations (even if forward declarations are used) for
+// each method invocation. As method declarations contain information about the
+// expected argument count, the parser can use this information to properly
+// parse the invocation arguments. For more details see: parseNamedRef
+func (p *Parser) parseMethodBody(method *entity.Method) bool {
+	p.parseOptions = parseOptParseMethodBodies
+	p.scopeEnter(method)
+	p.r.SetOffset(method.BodyStartOffset)
+	ok := p.parseArg(&opcodeTable[methodOpInfoIndex], method, 2, opArgTermList, method.BodyEndOffset)
+	p.scopeExit()
+
+	return ok
+}
+
 // parseNamedRef attempts to parse either a method invocation or a named
 // reference. As AML allows for forward references, the actual contents for
 // this entity will not be known until the entire AML stream has been parsed.
@@ -464,15 +444,17 @@ func (p *Parser) parseNamedRef() bool {
 		return false
 	}
 
-	var (
-		curOffset uint32
-		argIndex  uint8
-		arg       entity.Entity
-		argList   []interface{}
-	)
+	// Check if this is a method invocation
+	ent := entity.FindInScope(p.scopeCurrent(), p.root, name)
+	if methodDef, isMethod := ent.(*entity.Method); isMethod {
+		var (
+			curOffset uint32
+			argIndex  uint8
+			arg       entity.Entity
+			argList   []interface{}
+		)
 
-	if argCount, isMethod := p.methodArgCount[name]; isMethod {
-		for argIndex < argCount && !p.r.EOF() {
+		for argIndex < methodDef.ArgCount && !p.r.EOF() {
 			// Peek next opcode
 			curOffset = p.r.Offset()
 			nextOpcode, ok := p.nextOpcode()
@@ -501,12 +483,12 @@ func (p *Parser) parseNamedRef() bool {
 		}
 
 		// Check whether all expected arguments have been parsed
-		if argIndex != argCount {
-			kfmt.Fprintf(p.errWriter, "[table: %s, offset: %d] unexpected arglist end for method %s invocation: expected %d; got %d\n", p.tableName, p.r.Offset(), name, argCount, argIndex)
+		if argIndex != methodDef.ArgCount {
+			kfmt.Fprintf(p.errWriter, "[table: %s, offset: %d] unexpected arglist end for method %s invocation: expected %d; got %d\n", p.tableName, p.r.Offset(), name, methodDef.ArgCount, argIndex)
 			return false
 		}
 
-		return p.scopeCurrent().Append(entity.NewInvocation(p.tableHandle, name, argList))
+		return p.scopeCurrent().Append(entity.NewInvocation(p.tableHandle, methodDef, argList))
 	}
 
 	// Otherwise this is a reference to a named entity

src/gopheros/device/acpi/aml/parser/parser_test.go

@@ -36,6 +36,63 @@ func TestParser(t *testing.T) {
 	}
 }
 
+func TestParsingOfMethodBodies(t *testing.T) {
+	var resolver = mockResolver{
+		tableFiles: []string{"parser-testsuite-fwd-decls-DSDT.aml"},
+	}
+
+	p := NewParser(os.Stderr, genDefaultScopes())
+	tableName := strings.Replace(resolver.tableFiles[0], ".aml", "", -1)
+	if err := p.ParseAML(0, tableName, resolver.LookupTable(tableName)); err != nil {
+		t.Fatalf("[%s]: %v", tableName, err)
+	}
+
+	// Collect invocations
+	var invocations []*entity.Invocation
+	entity.Visit(0, p.root, entity.TypeAny, func(_ int, ent entity.Entity) bool {
+		if inv, isInv := ent.(*entity.Invocation); isInv {
+			invocations = append(invocations, inv)
+		}
+		return true
+	})
+
+	specs := []struct {
+		expParentName string
+		expArgCount   int
+	}{
+		// Call to `\NST1`
+		{`\`, 1},
+		// Call to `\_SB.NST1`
+		{`_SB_`, 2},
+		// Call to `\NST1` (first argument of above invocation)
+		{`\`, 1},
+	}
+
+	if exp, got := len(specs), len(invocations); exp != got {
+		t.Fatalf("expected parser to produce %d method invocations; got %d", exp, got)
+	}
+
+	for specIndex, spec := range specs {
+		if got := invocations[specIndex].Method.Parent().Name(); got != spec.expParentName {
+			t.Errorf(
+				"[spec %d] expected invocation to target %s.%s; got %s.%s",
+				specIndex,
+				spec.expParentName, invocations[specIndex].Method.Name(),
+				got, invocations[specIndex].Method.Name(),
+			)
+		}
+
+		if got := len(invocations[specIndex].Args()); got != spec.expArgCount {
+			t.Errorf(
+				"[spec %d] expected invocation to target %s.%s to receive %d args; got %d",
+				specIndex,
+				spec.expParentName, invocations[specIndex].Method.Name(), spec.expArgCount,
+				got,
+			)
+		}
+	}
+}
+
 func TestTableHandleAssignment(t *testing.T) {
 	var resolver = mockResolver{tableFiles: []string{"parser-testsuite-DSDT.aml"}}
 
@@ -326,11 +383,12 @@ func TestParserErrorHandling(t *testing.T) {
 	})
 
 	t.Run("parseNamedRef errors", func(t *testing.T) {
-		t.Run("missing args", func(t *testing.T) {
+		t.Run("incorrect args for method", func(t *testing.T) {
 			p.root = entity.NewScope(entity.OpScope, 42, `\`)
-			p.methodArgCount = map[string]uint8{
-				"MTHD": 10,
-			}
+
+			methodDecl := entity.NewMethod(42, "MTHD")
+			methodDecl.ArgCount = 5
+			p.root.Append(methodDecl)
 
 			mockParserPayload(p, []byte{
 				'M', 'T', 'H', 'D',
@@ -605,77 +663,6 @@ func TestParserErrorHandling(t *testing.T) {
 	})
 }
 
-func TestDetectMethodDeclarations(t *testing.T) {
-	p := &Parser{
-		errWriter: ioutil.Discard,
-	}
-
-	validMethod := []byte{
-		byte(entity.OpMethod),
-		5, // pkgLen
-		'M', 'T', 'H', 'D',
-		2, // flags (2 args)
-	}
-
-	t.Run("success", func(t *testing.T) {
-		mockParserPayload(p, validMethod)
-		p.methodArgCount = make(map[string]uint8)
-		p.detectMethodDeclarations()
-
-		argCount, inMap := p.methodArgCount["MTHD"]
-		if !inMap {
-			t.Error(`detectMethodDeclarations failed to parse method "MTHD"`)
-		}
-
-		if exp := uint8(2); argCount != exp {
-			t.Errorf(`expected arg count for "MTHD" to be %d; got %d`, exp, argCount)
-		}
-	})
-
-	t.Run("bad pkgLen", func(t *testing.T) {
-		mockParserPayload(p, []byte{
-			byte(entity.OpMethod),
-			// lead byte bits (6:7) indicate 1 extra byte that is missing
-			byte(1 << 6),
-		})
-
-		p.methodArgCount = make(map[string]uint8)
-		p.detectMethodDeclarations()
-	})
-
-	t.Run("error parsing namestring", func(t *testing.T) {
-		mockParserPayload(p, append([]byte{
-			byte(entity.OpMethod),
-			byte(5), // pkgLen
-			10,      // bogus char, not part of namestring
-		}, validMethod...))
-
-		p.methodArgCount = make(map[string]uint8)
-		p.detectMethodDeclarations()
-
-		argCount, inMap := p.methodArgCount["MTHD"]
-		if !inMap {
-			t.Error(`detectMethodDeclarations failed to parse method "MTHD"`)
-		}
-
-		if exp := uint8(2); argCount != exp {
-			t.Errorf(`expected arg count for "MTHD" to be %d; got %d`, exp, argCount)
-		}
-	})
-
-	t.Run("error parsing method flags", func(t *testing.T) {
-		mockParserPayload(p, []byte{
-			byte(entity.OpMethod),
-			byte(5), // pkgLen
-			'F', 'O', 'O', 'F',
-			// Missing flag byte
-		})
-
-		p.methodArgCount = make(map[string]uint8)
-		p.detectMethodDeclarations()
-	})
-}
-
 func mockParserPayload(p *Parser, payload []byte) *table.SDTHeader {
 	resolver := fixedPayloadResolver{payload}
 	header := resolver.LookupTable("DSDT")

src/gopheros/device/acpi/aml/parser/stream_reader.go

@@ -80,5 +80,8 @@ func (r *amlStreamReader) Offset() uint32 {
 
 // SetOffset sets the reader offset to the supplied value.
 func (r *amlStreamReader) SetOffset(off uint32) {
+	if max := uint32(len(r.data)); off > max {
+		off = max
+	}
 	r.offset = off
 }

src/gopheros/device/acpi/table/tabletest/parser-testsuite-fwd-decls-DSDT.dsl

@@ -1,10 +1,21 @@
 DefinitionBlock ("parser-testsuite-fwd-decls-DSDT.aml", "DSDT", 2, "GOPHER", "GOPHEROS", 0x00000002)
 {
+  Scope(\_SB){
+    Method (NST1, 2, NotSerialized)
+    {
+      Return ("something")
+    }
+  }
+
   Method(NST0, 1, NotSerialized)
   {
-    // Invoke a method which has not been defined at the time the parser
-    // reaches this block (forward declaration)
-    Return(NST1(Arg0))
+    // NST1 is declared after NST0 (forward declaration)
+    NST1(Arg0)
+
+    // This version of NST1 is defined above and has a different signature.
+    // The parser should be able to resolve it to the correct method and
+    // parse the correct number of arguments
+    Return(\_SB.NST1(NST1(123), "arg"))
   }
 
   // The declaration of NST1 in the AML stream occurs after the declaration