[release-branch.go1.23] 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 #73831.

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/+/676958
TryBot-Bypass: Cherry Mui <cherryyz@google.com>

diff --git a/src/cmd/link/internal/loader/loader.go b/src/cmd/link/internal/loader/loader.go
index 40fc949ee90ff6c294892b408a6cdde5c047e747..6697095fbaab9f6c18f025404a34a92370e91d41 100644 (file)
--- a/src/cmd/link/internal/loader/loader.go
+++ b/src/cmd/link/internal/loader/loader.go
@@ -450,33 +450,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 == sym.STEXT
+       newIsText := newtyp == sym.STEXT
+       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 9b845750f32dee58fde4664df443727f37fc21d9..08cdf1750b66bdf8aac26d626ea068b3830f9d47 100644 (file)
--- a/src/cmd/link/link_test.go
+++ b/src/cmd/link/link_test.go
@@ -1432,6 +1432,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},
@@ -1447,7 +1450,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