From a31c931adf4cba81b8e7f8bd9772b81699955749 Mon Sep 17 00:00:00 2001 From: Cherry Mui Date: Wed, 21 May 2025 14:32:21 -0400 Subject: [PATCH] [release-branch.go1.24] cmd/link: allow linkname reference to a TEXT symbol regardless of size In CL 660696, we made the linker to choose the symbol of the larger size in case there are multiple contentless declarations of the same symbol. We also made it emit an error in the case that there are a contentless declaration of a larger size and a definition with content of a smaller size. In this case, we should choose the definition with content, but the code accesses it through the declaration of the larger size could fall into the next symbol, potentially causing data corruption. So we disallowed it. There is one spcial case, though, that some code uses a linknamed variable declaration to reference a function in assembly, in order to take its address. The variable is often declared as uintptr. The function symbol is the definition, which could sometimes be shorter. This would trigger the error case above, causing existing code failing to build. This CL allows it as a special case. It is still not safe to access the variable's content. But it is actually okay to just take its address, which the existing code often do. Updates #73617. Fixes #73832. Change-Id: I467381bc5f6baa16caee6752a0a824c7185422f6 Reviewed-on: https://go-review.googlesource.com/c/go/+/676636 Reviewed-by: David Chase LUCI-TryBot-Result: Go LUCI (cherry picked from commit 70109eb32625487d9c774d602a4fa2422e218f1b) Reviewed-on: https://go-review.googlesource.com/c/go/+/676957 --- src/cmd/link/internal/loader/loader.go | 61 ++++++++++++------- src/cmd/link/link_test.go | 5 +- src/cmd/link/testdata/linkname/textvar/asm.s | 6 ++ .../link/testdata/linkname/textvar/main.go | 17 ++++++ 4 files changed, 66 insertions(+), 23 deletions(-) create mode 100644 src/cmd/link/testdata/linkname/textvar/asm.s create mode 100644 src/cmd/link/testdata/linkname/textvar/main.go diff --git a/src/cmd/link/internal/loader/loader.go b/src/cmd/link/internal/loader/loader.go index 182379f0df..fa0d3457c8 100644 --- a/src/cmd/link/internal/loader/loader.go +++ b/src/cmd/link/internal/loader/loader.go @@ -452,33 +452,50 @@ func (st *loadState) addSym(name string, ver int, r *oReader, li uint32, kind in if oldsym.Dupok() { return oldi } - // If one is a DATA symbol (i.e. has content, DataSize != 0) - // and the other is BSS, the one with content wins. + // If one is a DATA symbol (i.e. has content, DataSize != 0, + // including RODATA) and the other is BSS, the one with content wins. // If both are BSS, the one with larger size wins. - // Specifically, the "overwrite" variable and the final result are // - // new sym old sym overwrite + // For a special case, we allow a TEXT symbol overwrites a BSS symbol + // even if the BSS symbol has larger size. This is because there is + // code like below to take the address of a function + // + // //go:linkname fn + // var fn uintptr + // var fnAddr = uintptr(unsafe.Pointer(&fn)) + // + // TODO: maybe limit this case to just pointer sized variable? + // + // In summary, the "overwrite" variable and the final result are + // + // new sym old sym result // --------------------------------------------- - // DATA DATA true => ERROR - // DATA lg/eq BSS sm/eq true => new wins - // DATA small BSS large true => ERROR - // BSS large DATA small true => ERROR - // BSS large BSS small true => new wins - // BSS sm/eq D/B lg/eq false => old wins - overwrite := r.DataSize(li) != 0 || oldsz < sz - if overwrite { + // TEXT BSS new wins + // DATA DATA ERROR + // DATA lg/eq BSS sm/eq new wins + // DATA small BSS large ERROR + // BSS large DATA small ERROR + // BSS large BSS small new wins + // BSS sm/eq D/B lg/eq old wins + // BSS TEXT old wins + oldtyp := sym.AbiSymKindToSymKind[objabi.SymKind(oldsym.Type())] + newtyp := sym.AbiSymKindToSymKind[objabi.SymKind(osym.Type())] + oldIsText := oldtyp.IsText() + newIsText := newtyp.IsText() + oldHasContent := oldr.DataSize(oldli) != 0 + newHasContent := r.DataSize(li) != 0 + oldIsBSS := oldtyp.IsData() && !oldHasContent + newIsBSS := newtyp.IsData() && !newHasContent + switch { + case newIsText && oldIsBSS, + newHasContent && oldIsBSS && sz >= oldsz, + newIsBSS && oldIsBSS && sz > oldsz: // new symbol overwrites old symbol. - oldtyp := sym.AbiSymKindToSymKind[objabi.SymKind(oldsym.Type())] - if !(oldtyp.IsData() && oldr.DataSize(oldli) == 0) || oldsz > sz { - log.Fatalf("duplicated definition of symbol %s, from %s and %s", name, r.unit.Lib.Pkg, oldr.unit.Lib.Pkg) - } l.objSyms[oldi] = objSym{r.objidx, li} - } else { - // old symbol overwrites new symbol. - typ := sym.AbiSymKindToSymKind[objabi.SymKind(oldsym.Type())] - if !typ.IsData() { // only allow overwriting data symbol - log.Fatalf("duplicated definition of symbol %s, from %s and %s", name, r.unit.Lib.Pkg, oldr.unit.Lib.Pkg) - } + case newIsBSS && (oldsz >= sz || oldIsText): + // old win, just ignore the new symbol. + default: + log.Fatalf("duplicated definition of symbol %s, from %s (type %s size %d) and %s (type %s size %d)", name, r.unit.Lib.Pkg, newtyp, sz, oldr.unit.Lib.Pkg, oldtyp, oldsz) } return oldi } diff --git a/src/cmd/link/link_test.go b/src/cmd/link/link_test.go index cd2f9e3953..7231a36dc3 100644 --- a/src/cmd/link/link_test.go +++ b/src/cmd/link/link_test.go @@ -1512,6 +1512,9 @@ func TestCheckLinkname(t *testing.T) { {"ok.go", true}, // push linkname is ok {"push.go", true}, + // using a linknamed variable to reference an assembly + // function in the same package is ok + {"textvar", true}, // pull linkname of blocked symbol is not ok {"coro.go", false}, {"coro_var.go", false}, @@ -1529,7 +1532,7 @@ func TestCheckLinkname(t *testing.T) { test := test t.Run(test.src, func(t *testing.T) { t.Parallel() - src := filepath.Join("testdata", "linkname", test.src) + src := "./testdata/linkname/" + test.src exe := filepath.Join(tmpdir, test.src+".exe") cmd := testenv.Command(t, testenv.GoToolPath(t), "build", "-o", exe, src) out, err := cmd.CombinedOutput() diff --git a/src/cmd/link/testdata/linkname/textvar/asm.s b/src/cmd/link/testdata/linkname/textvar/asm.s new file mode 100644 index 0000000000..332dcdb4e7 --- /dev/null +++ b/src/cmd/link/testdata/linkname/textvar/asm.s @@ -0,0 +1,6 @@ +// Copyright 2024 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 ·asmfunc(SB),0,$0-0 + RET diff --git a/src/cmd/link/testdata/linkname/textvar/main.go b/src/cmd/link/testdata/linkname/textvar/main.go new file mode 100644 index 0000000000..b38995e706 --- /dev/null +++ b/src/cmd/link/testdata/linkname/textvar/main.go @@ -0,0 +1,17 @@ +// Copyright 2024 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. + +// Using a linknamed variable to reference an assembly +// function in the same package is ok. + +package main + +import _ "unsafe" + +func main() { + println(&asmfunc) +} + +//go:linkname asmfunc +var asmfunc uintptr -- 2.50.0