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.

Fixes #73617.

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>

diff --git a/src/cmd/link/internal/loader/loader.go b/src/cmd/link/internal/loader/loader.go
index 128173b8cf990c5dabb94c394173dd5be526bcec..6561362210d796ee6fcdc9df62b4e1c03af11a0e 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 7c3ca427edc5128068a88b7f524db9eb122e0cf5..2fe32600f1d6c7afad73e7d11d5e4ae0e7013472 100644 (file)
--- a/src/cmd/link/link_test.go
+++ b/src/cmd/link/link_test.go
@@ -1601,6 +1601,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},
@@ -1618,7 +1621,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