acpi: refine post-parse entity processing rules

This commit updates the post-parse step so that:
- the visitor not longer recurses into method bodies. Since code inside
  methods may potentially generate dynamic/scoped entities or even use
  conditional invocations (if CondRefOf(X) { X(...) }), symbol resolution
  will be deferred to the AML interpreter.
- parent-child relationships between entities are checked and updated if
  not properly specified

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

@@ -65,12 +65,27 @@ func (p *Parser) ParseAML(tableHandle uint8, tableName string, header *table.SDT
 	}
 	p.scopeExit()
 
-	// Pass 2: resolve forward references
+	// Pass 3: check parents and resolve forward references
 	var resolveFailed bool
 	scopeVisit(0, p.root, EntityTypeAny, func(_ int, ent Entity) bool {
+		// Skip method bodies; their contents will be lazily resolved by the interpreter
+		if _, isMethod := ent.(*Method); isMethod {
+			return false
+		}
+
+		// Populate parents for any entity args that are also entities but are not
+		// linked to a parent (e.g. a package inside a named entity).
+		for _, arg := range ent.getArgs() {
+			if argEnt, isArgEnt := arg.(Entity); isArgEnt && argEnt.Parent() == nil {
+				argEnt.setParent(ent.Parent())
+			}
+		}
+
 		if res, ok := ent.(resolver); ok && !res.Resolve(p.errWriter, p.root) {
 			resolveFailed = true
+			return false
 		}
+
 		return true
 	})
 
@@ -159,7 +174,7 @@ func (p *Parser) parseObj() bool {
 	curOffset = p.r.Offset()
 	if info, ok = p.nextOpcode(); !ok {
 		p.r.SetOffset(curOffset)
-		return p.parseMethodInvocationOrNameRef()
+		return p.parseNamedRef()
 	}
 
 	hasPkgLen := info.flags.is(opFlagHasPkgLen) || info.argFlags.contains(opArgTermList) || info.argFlags.contains(opArgFieldList)
@@ -415,34 +430,29 @@ func (p *Parser) makeObjForOpcode(info *opcodeInfo) Entity {
 	return obj
 }
 
-// parseMethodInvocationOrNameRef attempts to parse a method invocation and its term
+// parseNamedRef attempts to parse either a method invocation or a named
-// args. This method first scans the NameString and performs a lookup. If the
+// reference. As AML allows for forward references, the actual contents for
-// lookup returns a method definition then we consult it to figure out how many
+// this entity will not be known until the entire AML stream has been parsed.
-// arguments we need to parse.
 //
 // Grammar:
 // MethodInvocation := NameString TermArgList
 // TermArgList = Nothing | TermArg TermArgList
 // TermArg = Type2Opcode | DataObject | ArgObj | LocalObj | MethodInvocation
-func (p *Parser) parseMethodInvocationOrNameRef() bool {
+func (p *Parser) parseNamedRef() bool {
-	invocationStartOffset := p.r.Offset()
 	name, ok := p.parseNameString()
 	if !ok {
 		return false
 	}
 
-	// Lookup Name and try matching it to a function definition
+	var (
-	if methodDef, ok := scopeFind(p.scopeCurrent(), p.root, name).(*Method); ok {
+		curOffset uint32
-		var (
+		argIndex  uint8
-			invocation = &methodInvocationEntity{
+		arg       Entity
-				methodDef: methodDef,
+		argList   []interface{}
-			}
+	)
-			curOffset uint32
-			argIndex  uint8
-			arg       Entity
-		)
 
-		for argIndex < methodDef.argCount && !p.r.EOF() {
+	if argCount, isMethod := p.methodArgCount[name]; isMethod {
+		for argIndex < argCount && !p.r.EOF() {
 			// Peek next opcode
 			curOffset = p.r.Offset()
 			nextOpcode, ok := p.nextOpcode()
@@ -453,7 +463,7 @@ func (p *Parser) parseMethodInvocationOrNameRef() bool {
 				arg, ok = p.parseArgObj()
 			default:
 				// It may be a nested invocation or named ref
-				ok = p.parseMethodInvocationOrNameRef()
+				ok = p.parseNamedRef()
 				if ok {
 					arg = p.scopeCurrent().lastChild()
 					p.scopeCurrent().removeChild(arg)
@@ -466,23 +476,24 @@ func (p *Parser) parseMethodInvocationOrNameRef() bool {
 				break
 			}
 
-			invocation.setArg(argIndex, arg)
+			argList = append(argList, arg)
 			argIndex++
 		}
 
-		if argIndex != methodDef.argCount {
+		// Check whether all expected arguments have been parsed
-			kfmt.Fprintf(p.errWriter, "[table: %s, offset: %d] argument mismatch (exp: %d, got %d) for invocation of method: %s\n", p.tableName, invocationStartOffset, methodDef.argCount, argIndex, name)
+		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)
 			return false
 		}
 
-		p.scopeCurrent().Append(invocation)
+		return p.scopeCurrent().Append(&methodInvocationEntity{
-		return true
+			unnamedEntity: unnamedEntity{args: argList},
+			methodName:    name,
+		})
 	}
 
-	// This is a name reference; assume it's a forward reference for now
+	// Otherwise this is a reference to a named entity
-	// and delegate its resolution to a post-parse step.
+	return p.scopeCurrent().Append(&namedReference{targetName: name})
-	p.scopeCurrent().Append(&namedReference{targetName: name})
-	return true
 }
 
 func (p *Parser) nextOpcode() (*opcodeInfo, bool) {
@@ -893,7 +904,7 @@ func (p *Parser) parseTarget() (interface{}, bool) {
 	}
 
 	// In this case, this is either a NameString or a control method invocation.
-	if ok := p.parseMethodInvocationOrNameRef(); ok {
+	if ok := p.parseNamedRef(); ok {
 		obj := p.scopeCurrent().lastChild()
 		p.scopeCurrent().removeChild(obj)
 		return obj, ok

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

@@ -3,6 +3,7 @@ package aml
 import (
 	"gopheros/device/acpi/table"
 	"io/ioutil"
+	"os"
 	"path/filepath"
 	"runtime"
 	"strings"
@@ -14,6 +15,7 @@ func TestParser(t *testing.T) {
 	specs := [][]string{
 		[]string{"DSDT.aml", "SSDT.aml"},
 		[]string{"parser-testsuite-DSDT.aml"},
+		[]string{"parser-testsuite-fwd-decls-DSDT.aml"},
 	}
 
 	for specIndex, spec := range specs {
@@ -29,7 +31,11 @@ func TestParser(t *testing.T) {
 		rootNS.Append(&scopeEntity{op: opScope, name: `_SI_`}) // System indicators
 		rootNS.Append(&scopeEntity{op: opScope, name: `_TZ_`}) // ACPI 1.0 thermal zone namespace
 
-		p := NewParser(ioutil.Discard, rootNS)
+		// Inject pre-defined OSPM objects
+		rootNS.Append(&constEntity{name: "_OS_", val: "gopheros"})
+		rootNS.Append(&constEntity{name: "_REV", val: uint64(2)})
+
+		p := NewParser(os.Stderr, rootNS)
 
 		for _, tableName := range spec {
 			tableName = strings.Replace(tableName, ".aml", "", -1)
@@ -91,6 +97,30 @@ func TestTableHandleAssignment(t *testing.T) {
 	}
 }
 
+func TestParserForwardDeclParsing(t *testing.T) {
+	var resolver = mockResolver{
+		tableFiles: []string{"parser-testsuite-fwd-decls-DSDT.aml"},
+	}
+
+	// Create default scopes
+	rootNS := &scopeEntity{op: opScope, name: `\`}
+	rootNS.Append(&scopeEntity{op: opScope, name: `_GPE`}) // General events in GPE register block
+	rootNS.Append(&scopeEntity{op: opScope, name: `_PR_`}) // ACPI 1.0 processor namespace
+	rootNS.Append(&scopeEntity{op: opScope, name: `_SB_`}) // System bus with all device objects
+	rootNS.Append(&scopeEntity{op: opScope, name: `_SI_`}) // System indicators
+	rootNS.Append(&scopeEntity{op: opScope, name: `_TZ_`}) // ACPI 1.0 thermal zone namespace
+
+	p := NewParser(ioutil.Discard, rootNS)
+
+	for _, tableName := range resolver.tableFiles {
+		tableName = strings.Replace(tableName, ".aml", "", -1)
+		if err := p.ParseAML(0, tableName, resolver.LookupTable(tableName)); err != nil {
+			t.Errorf("[%s]: %v", tableName, err)
+			break
+		}
+	}
+}
+
 func TestParsePkgLength(t *testing.T) {
 	specs := []struct {
 		payload []byte
@@ -307,13 +337,12 @@ func TestParserErrorHandling(t *testing.T) {
 		}
 	})
 
-	t.Run("parseMethodInvocationOrNameRef errors", func(t *testing.T) {
+	t.Run("parseNamedRef errors", func(t *testing.T) {
 		t.Run("missing args", func(t *testing.T) {
 			p.root = &scopeEntity{op: opScope, name: `\`}
-			p.root.Append(&Method{
+			p.methodArgCount = map[string]uint8{
-				scopeEntity: scopeEntity{name: "MTHD"},
+				"MTHD": 10,
-				argCount:    10,
+			}
-			})
 
 			mockParserPayload(p, []byte{
 				'M', 'T', 'H', 'D',
@@ -321,8 +350,8 @@ func TestParserErrorHandling(t *testing.T) {
 			})
 
 			p.scopeEnter(p.root)
-			if p.parseMethodInvocationOrNameRef() {
+			if p.parseNamedRef() {
-				t.Fatal("expected parseMethodInvocationOrNameRef to return false")
+				t.Fatal("expected parseNamedRef to return false")
 			}
 		})
 	})
@@ -556,6 +585,77 @@ func TestParserErrorHandling(t *testing.T) {
 	})
 }
 
+func TestDetectMethodDeclarations(t *testing.T) {
+	p := &Parser{
+		errWriter: ioutil.Discard,
+	}
+
+	validMethod := []byte{
+		byte(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(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(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(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/table/tabletest/parser-testsuite-DSDT.dsl

@@ -49,6 +49,8 @@ DefinitionBlock ("parser-testsuite-DSDT.aml", "DSDT", 2, "GOPHER", "GOPHEROS", 0
 
     // Other entity types
     Event(HLO0)
+    Mutex(MUT0,1)
+    Signal(HLO0)
 
     // Other executable bits
     Method (EXE0, 1, Serialized)
@@ -79,12 +81,10 @@ DefinitionBlock ("parser-testsuite-DSDT.aml", "DSDT", 2, "GOPHER", "GOPHEROS", 0
         Reset(HLO0)
 
         // Mutex support
-        Mutex(MUT0, 1)
         Acquire(MUT0, 0xffff) // no timeout
         Release(MUT0)
 
         // Signal/Wait
-        Signal(HLO0)
         Wait(HLO0, 0xffff)
 
         // Get monotonic timer value

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

@@ -0,0 +1,16 @@
+DefinitionBlock ("parser-testsuite-fwd-decls-DSDT.aml", "DSDT", 2, "GOPHER", "GOPHEROS", 0x00000002)
+{
+  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))
+  }
+
+  // The declaration of NST1 in the AML stream occurs after the declaration
+  // of NST0 method above.
+  Method(NST1, 1, NotSerialized)
+  {
+    Return(Arg0+42)
+  }
+}