cmd/asm: reject foo(SB)(AX) instead of silently treating as foo(SB)

Add test for assembly errors, to verify fix.
Make sure invalid instruction errors are printed just once
(was printing them once per span iteration, so typically twice).

Fixes #13282.

Change-Id: Id5f66f80a80b3bc4832e00084b0a91f1afec7f8f
Reviewed-on: https://go-review.googlesource.com/18858Reviewed-by: Rob Pike <r@golang.org>

src/cmd/asm/internal/asm/endtoend_test.go

@@ -10,6 +10,7 @@ import (
 	"io/ioutil"
 	"os"
 	"path/filepath"
+	"regexp"
 	"sort"
 	"strconv"
 	"strings"
@@ -35,11 +36,10 @@ func testEndToEnd(t *testing.T, goarch, file string) {
 	ctxt.Bso = obj.Binitw(os.Stdout)
 	defer ctxt.Bso.Flush()
 	failed := false
-	ctxt.Diag = func(format string, args ...interface{}) {
+	ctxt.DiagFunc = func(format string, args ...interface{}) {
 		failed = true
 		t.Errorf(format, args...)
 	}
-	obj.Binitw(ioutil.Discard)
 	pList.Firstpc, ok = parser.Parse()
 	if !ok || failed {
 		t.Errorf("asm: %s assembly failed", goarch)
@@ -175,7 +175,7 @@ Diff:
 	top := pList.Firstpc
 	var text *obj.LSym
 	ok = true
-	ctxt.Diag = func(format string, args ...interface{}) {
+	ctxt.DiagFunc = func(format string, args ...interface{}) {
 		t.Errorf(format, args...)
 		ok = false
 	}
@@ -250,8 +250,110 @@ func isHexes(s string) bool {
 	return true
 }
 
+// It would be nice if the error messages began with
+// the standard file:line: prefix,
+// but that's not where we are today.
+// It might be at the beginning but it might be in the middle of the printed instruction.
+var fileLineRE = regexp.MustCompile(`(?:^|\()(testdata[/\\][0-9a-z]+\.s:[0-9]+)(?:$|\))`)
+
+// Same as in test/run.go
+var (
+	errRE       = regexp.MustCompile(`// ERROR ?(.*)`)
+	errQuotesRE = regexp.MustCompile(`"([^"]*)"`)
+)
+
+func testErrors(t *testing.T, goarch, file string) {
+	lex.InitHist()
+	input := filepath.Join("testdata", file+".s")
+	architecture, ctxt := setArch(goarch)
+	lexer := lex.NewLexer(input, ctxt)
+	parser := NewParser(ctxt, architecture, lexer)
+	pList := obj.Linknewplist(ctxt)
+	var ok bool
+	testOut = new(bytes.Buffer) // The assembler writes test output to this buffer.
+	ctxt.Bso = obj.Binitw(os.Stdout)
+	defer ctxt.Bso.Flush()
+	failed := false
+	var errBuf bytes.Buffer
+	ctxt.DiagFunc = func(format string, args ...interface{}) {
+		failed = true
+		s := fmt.Sprintf(format, args...)
+		if !strings.HasSuffix(s, "\n") {
+			s += "\n"
+		}
+		errBuf.WriteString(s)
+	}
+	pList.Firstpc, ok = parser.Parse()
+	obj.Flushplist(ctxt)
+	if ok && !failed {
+		t.Errorf("asm: %s had no errors", goarch)
+	}
+
+	errors := map[string]string{}
+	for _, line := range strings.Split(errBuf.String(), "\n") {
+		if line == "" || strings.HasPrefix(line, "\t") {
+			continue
+		}
+		m := fileLineRE.FindStringSubmatch(line)
+		if m == nil {
+			t.Errorf("unexpected error: %v", line)
+			continue
+		}
+		fileline := m[1]
+		if errors[fileline] != "" {
+			t.Errorf("multiple errors on %s:\n\t%s\n\t%s", fileline, errors[fileline], line)
+			continue
+		}
+		errors[fileline] = line
+	}
+
+	// Reconstruct expected errors by independently "parsing" the input.
+	data, err := ioutil.ReadFile(input)
+	if err != nil {
+		t.Error(err)
+		return
+	}
+	lineno := 0
+	lines := strings.Split(string(data), "\n")
+	for _, line := range lines {
+		lineno++
+
+		fileline := fmt.Sprintf("%s:%d", input, lineno)
+		if m := errRE.FindStringSubmatch(line); m != nil {
+			all := m[1]
+			mm := errQuotesRE.FindAllStringSubmatch(all, -1)
+			if len(mm) != 1 {
+				t.Errorf("%s: invalid errorcheck line:\n%s", fileline, line)
+			} else if err := errors[fileline]; err == "" {
+				t.Errorf("%s: missing error, want %s", fileline, all)
+			} else if !strings.Contains(err, mm[0][1]) {
+				t.Errorf("%s: wrong error for %s:\n%s", fileline, all, err)
+			}
+		} else {
+			if errors[fileline] != "" {
+				t.Errorf("unexpected error on %s: %v", fileline, errors[fileline])
+			}
+		}
+		delete(errors, fileline)
+	}
+	var extra []string
+	for key := range errors {
+		extra = append(extra, key)
+	}
+	sort.Strings(extra)
+	for _, fileline := range extra {
+		t.Errorf("unexpected error on %s: %v", fileline, errors[fileline])
+	}
+}
+
 func Test386EndToEnd(t *testing.T) {
+	defer os.Setenv("GO386", os.Getenv("GO386"))
+
+	for _, go386 := range []string{"387", "sse"} {
+		os.Setenv("GO386", go386)
+		t.Logf("GO386=%v", os.Getenv("GO386"))
 		testEndToEnd(t, "386", "386")
+	}
 }
 
 func TestARMEndToEnd(t *testing.T) {
@@ -276,6 +378,10 @@ func TestAMD64Encoder(t *testing.T) {
 	testEndToEnd(t, "amd64", "amd64enc")
 }
 
+func TestAMD64Errors(t *testing.T) {
+	testErrors(t, "amd64", "amd64error")
+}
+
 func TestMIPS64EndToEnd(t *testing.T) {
 	testEndToEnd(t, "mips64", "mips64")
 }

src/cmd/asm/internal/asm/testdata/amd64error.s

+// Copyright 2016 The Go Authors.  All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+TEXT errors(SB),$0
+	MOVL	foo<>(SB)(AX), AX // ERROR "invalid instruction"
+	RET

src/cmd/asm/main.go

@@ -54,7 +54,7 @@ func main() {
 	lexer := lex.NewLexer(flag.Arg(0), ctxt)
 	parser := asm.NewParser(ctxt, architecture, lexer)
 	diag := false
-	ctxt.Diag = func(format string, args ...interface{}) {
+	ctxt.DiagFunc = func(format string, args ...interface{}) {
 		diag = true
 		log.Printf(format, args...)
 	}

src/cmd/compile/internal/gc/lex.go

@@ -105,7 +105,7 @@ func Main() {
 
 	Thearch.Linkarchinit()
 	Ctxt = obj.Linknew(Thearch.Thelinkarch)
-	Ctxt.Diag = Yyerror
+	Ctxt.DiagFunc = Yyerror
 	Ctxt.Bso = &bstdout
 	bstdout = *obj.Binitw(os.Stdout)
 

src/cmd/internal/obj/link.go

@@ -604,12 +604,13 @@ type Link struct {
 	Autosize           int32
 	Armsize            int32
 	Pc                 int64
-	Diag               func(string, ...interface{})
+	DiagFunc           func(string, ...interface{})
 	Mode               int
 	Cursym             *LSym
 	Version            int
 	Textp              *LSym
 	Etextp             *LSym
+	Errors             int
 
 	// state for writing objects
 	Text  *LSym
@@ -618,6 +619,11 @@ type Link struct {
 	Edata *LSym
 }
 
+func (ctxt *Link) Diag(format string, args ...interface{}) {
+	ctxt.Errors++
+	ctxt.DiagFunc(format, args...)
+}
+
 // The smallest possible offset from the hardware stack pointer to a local
 // variable on the stack. Architectures that use a link register save its value
 // on the stack in the function prologue and so always have a pointer between

src/cmd/internal/obj/x86/asm6.go

@@ -1856,6 +1856,7 @@ func span6(ctxt *obj.Link, s *obj.LSym) {
 	var loop int32
 	var m int
 	var p *obj.Prog
+	errors := ctxt.Errors
 	for {
 		loop = 0
 		for i = 0; i < len(s.R); i++ {
@@ -1968,6 +1969,9 @@ func span6(ctxt *obj.Link, s *obj.LSym) {
 		if loop == 0 {
 			break
 		}
+		if ctxt.Errors > errors {
+			return
+		}
 	}
 
 	if ctxt.Headtype == obj.Hnacl {
@@ -2294,6 +2298,11 @@ func oclass(ctxt *obj.Link, p *obj.Prog, a *obj.Addr) int {
 		return Yxxx
 
 	case obj.TYPE_MEM:
+		if a.Name != obj.NAME_NONE {
+			if ctxt.Asmode == 64 && (a.Reg != REG_NONE || a.Index != REG_NONE || a.Scale != 0) {
+				return Yxxx
+			}
+		}
 		return Ym
 
 	case obj.TYPE_ADDR: