Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cmd/link: support relocations typical with -mcmodel=medium with internal linking #67475

Open
thanm opened this issue May 17, 2024 · 1 comment
Assignees
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@thanm
Copy link
Contributor

thanm commented May 17, 2024

TLDR: it would useful if the Go linker could be changed to add support for relocations typically emitted by C compilers for code built with -mcmodel=medium.

More detail:

Consider this program (using txtar format):


-- go.mod --
module mcmodel.medium

go 1.23
-- main.go --
package main

import mcmodel "mcmodel.medium/mm"

func main() {
	println(mcmodel.Foo(1))
}
-- mm/bar.go --
package mcmodel

/*

#cgo CFLAGS: -mlarge-data-threshold=3  -mcmodel=medium

#include "defs.h"

LL ExtBigVar;

int ExtSmallVar;
*/
import "C"

func Bar(q int) int {
	return 43
}
-- mm/foo.go --
package mcmodel

/*

#cgo CFLAGS: -mlarge-data-threshold=3  -mcmodel=medium

#include "defs.h"

LL BigVar;

extern LL ExtBigVar;

int SmallVar;

extern int ExtSmallVar;

int ret1() {
  printf("%d %d\n", SmallVar, ExtSmallVar);
  return SmallVar + ExtSmallVar;
}

int ret2() {
  printf("%d %d\n", BigVar.x, ExtBigVar.x);
  return BigVar.x + ExtBigVar.x;
}
*/
import "C"

func Foo(x int) int {
	return Bar(x) + int(C.ret1()+C.ret2())
}

This code currently will not build due to "-mcmodel" etc not being on the allowlist for CGO cflags, but if you hack the go command to allow these options, then build:

# externl linking works
$ CC=cc go build .
# internal linking does not
$ CC=cc go build -ldflags=-linkmode=internal
# mcmodel.medium
$ WORK/b002/_pkg_.a(_x003.o): unknown relocation type 26; compiled without -fpic?
$

Note the "unknown relocation type 26". This is R_X86_64_GOTPC32; this reloc and R_X86_64_GOTOFF64 are frequently generated when building with the medium code model.

It would be useful to ungrade the linker to handle these relocations; this is not a critical problem by any means since we can always fall back on external linking, but given the increased prevalence of "-mcmodel=medium" out in the wild for building very large programs, it might well be a good thing for us to add support for this.

NB: patch required to get to this to build with go command is below.

$ git diff
diff --git a/src/cmd/go/internal/work/security.go b/src/cmd/go/internal/work/security.go
index 8e788b0425..1c9d466189 100644
--- a/src/cmd/go/internal/work/security.go
+++ b/src/cmd/go/internal/work/security.go
@@ -117,6 +117,8 @@ var validCompilerFlags = []*lazyregexp.Regexp{
        re(`-mthumb(-interwork)?`),
        re(`-mthreads`),
        re(`-mwindows`),
+       re(`-mcmodel=(.+)`),
+       re(`-mlarge-data-threshold=(.+)`),
        re(`-no-canonical-prefixes`),
        re(`--param=ssp-buffer-size=[0-9]*`),
        re(`-pedantic(-errors)?`),
$ 

Related google-internal bug: b/341263385.

For some background on just exactly what -mcmodel=medium implies, see Eli's blog post.

@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label May 17, 2024
@thanm thanm added this to the Backlog milestone May 17, 2024
@thanm thanm added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label May 17, 2024
@gopherbot
Copy link

Change https://go.dev/cl/586675 mentions this issue: cmd/go: accept -mcmodel and -mlarge-data-threshold compiler flags

gopherbot pushed a commit that referenced this issue May 20, 2024
For #67475

Change-Id: Ia90952eb7c19764306205928ead50bff22857378
Reviewed-on: https://go-review.googlesource.com/c/go/+/586675
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Auto-Submit: Ian Lance Taylor <iant@golang.org>
Reviewed-by: Ian Lance Taylor <iant@google.com>
Auto-Submit: Ian Lance Taylor <iant@google.com>
Reviewed-by: Than McIntosh <thanm@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
Development

No branches or pull requests

2 participants