fix(asm): reject duplicate symbol declarations like the toolchain

Assisted-by: GLM 5.3 Flash

asm/link.go

@@ -557,11 +557,66 @@ type dataSym struct {
 	relocs []Reloc
 }
 
+// checkDuplicateDecls rejects a symbol the file declares twice, the
+// toolchain's OnList rule (cmd/internal/obj, InitTextSym and GloblPos,
+// measured with go tool asm): the second declaration of a symbol is
+// diagnosed as a redeclaration whether the pair is two TEXTs, two GLOBLs
+// or one of each, and a second TEXT carries the other declaration's line
+// the way the toolchain's note does.  The DUPOK flag plays no part at
+// assembly time: it legalises duplicate definitions across files
+// (AttrDuplicateOK, which the linker resolves), never two declarations
+// inside one file, so a DUPOK pair here is rejected exactly like a plain
+// one.  Symbol identity follows the object model: the package prefix and
+// the <> static marker are part of the name (foo(SB), foo<>(SB) and
+// other·foo(SB) are three symbols); the ABI selector is not, because
+// outside the runtime package, where alone it is legal, foo<ABIInternal>(SB)
+// resolves to foo(SB) and is a redeclaration.
+func checkDuplicateDecls(f *ast.File) error {
+	type decl struct {
+		text bool
+		line int
+	}
+	seen := make(map[string]decl)
+	symKey := func(s *ast.Symbol) string {
+		key := s.Pkg + "\x00" + s.Name
+		if s.Static {
+			key += "\x00<>"
+		}
+		return key
+	}
+	for _, d := range f.Decls {
+		switch t := d.(type) {
+		case *ast.Text:
+			if first, dup := seen[symKey(t.Name)]; dup {
+				if first.text {
+					return fmt.Errorf("symbol %q redeclared (other declaration on line %d)", t.Name.Name, first.line)
+				}
+				return fmt.Errorf("symbol %q redeclared", t.Name.Name)
+			}
+			seen[symKey(t.Name)] = decl{text: true, line: t.Pos().Line}
+		case *ast.Globl:
+			if t.Name == nil || t.Name.Pseudo != "SB" {
+				continue
+			}
+			if _, dup := seen[symKey(t.Name)]; dup {
+				return fmt.Errorf("symbol %q redeclared", t.Name.Name)
+			}
+			seen[symKey(t.Name)] = decl{line: t.Pos().Line}
+		}
+	}
+	return nil
+}
+
 // collectData gathers the file's static symbols (GLOBL) and their initial
 // contents (DATA) into byte buffers.  Two passes: the Plan 9 convention puts
 // every DATA line before its symbol's GLOBL, so the symbols are registered
-// before the initialisers are applied.
+// before the initialisers are applied.  Every per-architecture entry point
+// collects data before assembling text, so the duplicate-declaration check
+// rides here and covers them all.
 func collectData(f *ast.File) ([]dataSym, error) {
+	if err := checkDuplicateDecls(f); err != nil {
+		return nil, err
+	}
 	index := map[string]int{}
 	var syms []dataSym
 	for _, d := range f.Decls {
@@ -573,9 +628,6 @@ func collectData(f *ast.File) ([]dataSym, error) {
 			continue
 		}
 		name := gd.Name.Name
-		if _, dup := index[name]; dup {
-			return nil, fmt.Errorf("duplicate GLOBL %q", name)
-		}
 		size := 0
 		if gd.Size != nil && gd.Size.Imm.HasVal {
 			size = int(gd.Size.Imm.Val)

asm/link_test.go

@@ -12,9 +12,107 @@ import (
 	"strings"
 	"testing"
 
+	"sourcedock.dev/petrbalvin/gasm-sdk/ast"
 	"sourcedock.dev/petrbalvin/gasm-sdk/parser"
 )
 
+// TestDuplicateDecls pins the duplicate-declaration rule against the
+// toolchain's, measured with go tool asm (Go 1.27.1): a symbol declared
+// twice in one file is rejected at the second declaration whether the pair
+// is two TEXTs, two GLOBLs or one of each, and DUPOK never legalises it
+// within a file (the toolchain's duperror.s testdata asserts the plain
+// pair).  DUPOK legalises duplicates across files, so each file of a pair
+// assembles cleanly on its own, and symbol identity keeps the package
+// prefix and the <> static marker: foo(SB), foo<>(SB) and other·foo(SB)
+// are three symbols, not one.  Every case runs through all four
+// per-architecture entry points.
+func TestDuplicateDecls(t *testing.T) {
+	targets := []struct {
+		goarch string
+		file   string
+		asm    func(*ast.File) (*Image, error)
+	}{
+		{"amd64", "dup_amd64.s", func(f *ast.File) (*Image, error) { return AssembleFile(f) }},
+		{"arm64", "dup_arm64.s", AssembleFileARM64},
+		{"riscv64", "dup_riscv64.s", AssembleFileRISCV},
+		{"loong64", "dup_loong64.s", AssembleFileLOONG64},
+	}
+	cases := []struct {
+		name string
+		src  string
+		want string // substring of the error, "" for no error
+	}{
+		{
+			"plain TEXT duplicate rejected",
+			"TEXT foo(SB), NOSPLIT, $0\n\tRET\nTEXT foo(SB), NOSPLIT, $0\n\tRET\n",
+			`symbol "foo" redeclared (other declaration on line 1)`,
+		},
+		{
+			"DUPOK pair in one file rejected",
+			"TEXT foo(SB), DUPOK, $0\n\tRET\nTEXT foo(SB), DUPOK, $0\n\tRET\n",
+			`symbol "foo" redeclared (other declaration on line 1)`,
+		},
+		{
+			"TEXT then GLOBL rejected",
+			"TEXT foo(SB), NOSPLIT, $0\n\tRET\nGLOBL foo(SB), NOPTR, $8\n",
+			`symbol "foo" redeclared`,
+		},
+		{
+			"GLOBL then TEXT rejected",
+			"GLOBL foo(SB), NOPTR, $8\nTEXT foo(SB), NOSPLIT, $0\n\tRET\n",
+			`symbol "foo" redeclared`,
+		},
+		{
+			"plain GLOBL duplicate rejected",
+			"GLOBL bar(SB), NOPTR, $8\nGLOBL bar(SB), NOPTR, $8\n",
+			`symbol "bar" redeclared`,
+		},
+		{
+			"static and package markers separate symbols",
+			"TEXT foo(SB), NOSPLIT, $0\n\tRET\nTEXT foo<>(SB), NOSPLIT, $0\n\tRET\nTEXT other\u00b7foo(SB), NOSPLIT, $0\n\tRET\n",
+			"",
+		},
+		{
+			"single DUPOK TEXT assembles",
+			"TEXT foo(SB), DUPOK, $0\n\tRET\n",
+			"",
+		},
+	}
+	for _, tg := range targets {
+		for _, c := range cases {
+			f, errs := parser.Parse(tg.file, c.src)
+			if len(errs) > 0 {
+				t.Fatalf("%s/%s: parse: %v", tg.goarch, c.name, errs)
+			}
+			_, err := tg.asm(f)
+			if c.want == "" {
+				if err != nil {
+					t.Errorf("%s/%s: error %v, want none", tg.goarch, c.name, err)
+				}
+				continue
+			}
+			if err == nil || !strings.Contains(err.Error(), c.want) {
+				t.Errorf("%s/%s: error %v, want substring %q", tg.goarch, c.name, err, c.want)
+			}
+		}
+	}
+
+	// A DUPOK pair across files is the case DUPOK exists for: the linker
+	// resolves it, the assembler never sees both sides, so each file of the
+	// pair assembles cleanly on its own.
+	for _, tg := range targets {
+		for _, name := range []string{"first", "second"} {
+			f, errs := parser.Parse(tg.file, "TEXT foo(SB), DUPOK, $0\n\tRET\n")
+			if len(errs) > 0 {
+				t.Fatalf("%s/dupok cross-file %s: parse: %v", tg.goarch, name, errs)
+			}
+			if _, err := tg.asm(f); err != nil {
+				t.Errorf("%s/dupok cross-file %s: error %v, want none", tg.goarch, name, err)
+			}
+		}
+	}
+}
+
 // TestAssembleFileStaticData checks the whole-image layout; code, padding
 // and the data section; and that the RIP-relative displacements of static
 // symbol loads resolve to the right bytes.