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

Jump to bottom

cmd/link,x/build: android-arm-corellium tries to link using arm64 C toolchain #58829

Closed
bcmills opened this issue Mar 2, 2023 · 5 comments
Closed

cmd/link,x/build: android-arm-corellium tries to link using arm64 C toolchain #58829

bcmills opened this issue Mar 2, 2023 · 5 comments
Assignees
@bcmills
Labels
arch-arm Issues solely affecting the 32-bit arm architecture. Builders x/build issues (builders, bots, dashboards) compiler/runtime Issues related to the Go compiler and/or runtime. FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. OS-Android Testing An issue that has been verified to require only test changes, not just a test failure.
Milestone
Go1.21

Comments

@bcmills
Copy link
Contributor

bcmills commented Mar 2, 2023

In CL 472096, I am experimenting with enabling tests that use go build on android, because I don't see a good reason not to. Unfortunately, cgo linking seems to be broken on the android-arm-corellium builder.

An example failure (https://storage.googleapis.com/go-build-log/1efd1f50/android-arm-corellium_80b1a8db.log):

--- FAIL: TestIssue28429 (1.05s)
    link_test.go:106: 'go tool link -importcfg=/data/data/com.termux/files/home/tmpdir/workdir-host-android-arm64-corellium-android/tmp/TestIssue284292101675739/001/importcfg main.a' failed: exit status 1, output: /data/data/com.termux/files/home/tmpdir/workdir-host-android-arm64-corellium-android/go/pkg/tool/android_arm64/link: running gcc failed: exit status 1
        ld.lld: error: /data/data/com.termux/files/home/tmpdir/workdir-host-android-arm64-corellium-android/tmp/go-link-1516915211/go.o is incompatible with aarch64linux

It appears that for some reason the external linker thinks it is linking for arm64 instead of arm.

This builder runs on the arm64 host, and it uses the same CC value:

CC=/data/data/com.termux/files/home/clangwrap

It also explicitly sets CGO_ENABLED=1, added in CL 174857 (CC @eliasnaur @bradfitz).

It isn't clear to me whether this is a bug in cmd/link (failing to set a needed arch flag), or a misconfiguration in the builder itself (supplying the wrong CC value, or erroneously setting CGO_ENABLED when it should not be set).

(attn @golang/android @steeve @changkun; CC @golang/compiler)
@bcmills bcmills added Builders x/build issues (builders, bots, dashboards) OS-Android NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. arch-arm Issues solely affecting the 32-bit arm architecture. labels Mar 2, 2023
@bcmills bcmills added this to the Backlog milestone Mar 2, 2023
@bcmills bcmills changed the title cmd/link,x/build: android-arm-corellium C toolchain cmd/link,x/build: android-arm-corellium tries to link using arm64 C toolchain Mar 2, 2023
@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Mar 2, 2023
@mknyszek
Copy link
Contributor

mknyszek commented Mar 8, 2023

In triage, we (C&RT) suspect this is more of a builder environment issue, but we'll keep an eye on it. (Though maybe we're passing the wrong flag or something to the C toolchain.)

@bcmills
Copy link
Contributor Author

bcmills commented Mar 8, 2023 •
edited

I think (*cmd/go/internal/work.Builder).gccArchArgs may be making unwarranted assumptions about whether the $CC in use is gcc or clang — it has lines like

if cfg.Goos == "darwin" {
	return []string{"-arch", "x86_64", "-m64"}
}

but as far as I can tell the -arch flag may be needed whenever $CC is clang or a variation thereof (in this case clangwrap) if it may be a cross-compiler, and may be invalid whenever $CC is gcc or a variation thereof (where cross-compilers are generally given per-arch executable names).

So I think this is a cmd/go bug (assuming clang or not for cross-compilation based on GOOS), but it may have a possible workaround on the builder side too (explicitly setting -arch in the CC variable, or perhaps in CGO_CFLAGS and CGO_LDFLAGS).

@bcmills
Copy link
Contributor Author

bcmills commented Mar 9, 2023 •
edited

Oh! This might also just be a bug in the test. go_android_exec explicitly sets CGO_ENABLED=0, but up until CL 472215 testenv.HasCGO was ignoring CGO_ENABLED and instead reporting whether the test binary itself was built with cgo enabled.

(That was a bug: the doc comment for HasCGO says it reports “the current system”, not “the system on which the binary was built”.)

@gopherbot
Copy link

Change https://go.dev/cl/475155 mentions this issue: cmd/link: use only the configured C compiler in TestCGOLTO

gopherbot pushed a commit that referenced this issue Mar 9, 2023
@gopherbot
cmd/link: use only the configured C compiler in TestCGOLTO
c9389a5 
The test had been assuming that any 'gcc' or 'clang' command found in
$PATH could be used to compile cgo dependencies for the target GOARCH
and GOOS. That assumption does not hold in general: for example,
the GOARCH/GOOS configuration may be cross-compiling, which will cause
the test to fail if the native 'gcc' and/or 'clang' is not configured
for the target architecture.

Instead, leave the 'CC' variable unset and assume only that the user
has configured it appropriate to the environment in which they are
running the test.

For #58829.

Change-Id: I9a1269ae3e0b4af281702114dabba844953f74bd
Reviewed-on: https://go-review.googlesource.com/c/go/+/475155
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Bryan Mills <bcmills@google.com>
Auto-Submit: Bryan Mills <bcmills@google.com>
Reviewed-by: Cherry Mui <cherryyz@google.com>
@bcmills
Copy link
Contributor Author

bcmills commented Apr 11, 2023

Fixed in CL 475155.

@bcmills bcmills closed this as completed Apr 11, 2023
@bcmills bcmills modified the milestones: Backlog, Go1.21 Apr 11, 2023
@bcmills bcmills self-assigned this Apr 11, 2023
@bcmills bcmills added the Testing An issue that has been verified to require only test changes, not just a test failure. label Apr 11, 2023
@golang golang locked and limited conversation to collaborators Apr 10, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@bcmills bcmills

Labels
arch-arm Issues solely affecting the 32-bit arm architecture. Builders x/build issues (builders, bots, dashboards) compiler/runtime Issues related to the Go compiler and/or runtime. FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. OS-Android Testing An issue that has been verified to require only test changes, not just a test failure.
Projects
None yet
Milestone
Go1.21
Development

No branches or pull requests
3 participants
@mknyszek @bcmills @gopherbot