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>

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 {
-		// new symbol overwrites old symbol.
+	// 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())]
-		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)
-		}
+	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.
 		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
 }

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()

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

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