[release-branch.go1.24] cmd/link: allow linkname reference to a TEXT symbol regardles...

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 <drchase@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
(cherry picked from commit 70109eb32625487d9c774d602a4fa2422e218f1b)
Reviewed-on: https://go-review.googlesource.com/c/go/+/676957

diff --git a/src/cmd/link/internal/loader/loader.go b/src/cmd/link/internal/loader/loader.go
index 182379f0df0f68b8f49cb5e15006aebacb81e143..fa0d3457c8417e43b0109aa04aa015fc7141a278 100644 (file)
--- 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 cd2f9e3953ca5aa509a6cbd56f9cc1c157428354..7231a36dc371ad0b174f04acca97e61f4d6a94bc 100644 (file)
--- 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 (file)
index 0000000..332dcdb
--- /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 (file)
index 0000000..b38995e
--- /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