From c107b459334f03aef89f1c92ba68c6af3bfc9233 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Petr=20Balv=C3=ADn?= Date: Tue, 6 Oct 2026 23:23:33 +0200 Subject: [PATCH] fix(asm): reject duplicate symbol declarations like the toolchain Assisted-by: GLM 5.3 Flash --- asm/link.go | 60 +++++++++++++++++++++++++++-- asm/link_test.go | 98 ++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 154 insertions(+), 4 deletions(-) diff --git a/asm/link.go b/asm/link.go index 0133505..ef28d22 100644 --- a/asm/link.go +++ b/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(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) diff --git a/asm/link_test.go b/asm/link_test.go index df48fb0..f692027 100644 --- a/asm/link_test.go +++ b/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.