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

builds with cgo appear to miss .S assembly files #3411

Closed
tals opened this issue Dec 21, 2022 · 9 comments · Fixed by #3661
Closed

builds with cgo appear to miss .S assembly files #3411

tals opened this issue Dec 21, 2022 · 9 comments · Fixed by #3661

Comments

@tals
Copy link

tals commented Dec 21, 2022

What version of rules_go are you using?

0.35.0

What version of gazelle are you using?

0.27.0

What version of Bazel are you using?

Build label: 5.4.0

Does this issue reproduce with the latest releases of all the above?

Yes

What operating system and processor architecture are you using?

$ lsb_release -a
Distributor ID: Ubuntu
Description:    Ubuntu 22.04.1 LTS
Release:        22.04
Codename:       jammy

Any other potentially useful information about your toolchain?

What did you do?

I am attempting to build Go program using the popular zstd package from DataDog
This library vendors zstd, which includes accelerated operations in the .S file.

This works well with the vanilla go toolchain, however building this via rules_go fails to pick up an .S file for amd64, causing it to fail during link time

$ git clone https://github.com/tals/bazel-rulesgo-broken-asm-repro
$ cd bazel-rulesgo-broken-asm-repro
$ go run main.go
Hello, World!

$ bazel run  //:hello-go

INFO: Analyzed target //:hello-go (26 packages loaded, 195 targets configured).
INFO: Found 1 target...
ERROR: /home/tal/.cache/bazel/_bazel_tal/d6d42962320318bd35977430f4d78b15/external/com_github_datadog_zstd/BUILD.bazel:3:11: GoCompilePkg external/com_github_datadog_zstd/zstd.a failed: (Exit 1): builder failed: error executing command bazel-out/k8-opt-exec-2B5CBBC6/bin/external/go_sdk/builder compilepkg -sdk external/go_sdk -installsuffix linux_amd64 -src external/com_github_datadog_zstd/errors.go -src ... (remaining 211 arguments skipped)

Use --sandbox_debug to see verbose messages from the sandbox and retain the sandbox build root for debugging
/tmp/rules_go_work-585792782/cgo/github.com/DataDog/zstd/_x16.o:huf_decompress.c:function HUF_decompress4X1_usingDTable_internal_bmi2_asm: error: undefined reference to 'HUF_decompress4X1_usingDTable_internal_bmi2_asm_loop'
/tmp/rules_go_work-585792782/cgo/github.com/DataDog/zstd/_x16.o:huf_decompress.c:function HUF_decompress4X2_usingDTable_internal_bmi2_asm: error: undefined reference to 'HUF_decompress4X2_usingDTable_internal_bmi2_asm_loop'
collect2: error: ld returned 1 exit status
compilepkg: error running subcommand /usr/bin/gcc: exit status 1
Target //:hello-go failed to build
Use --verbose_failures to see the command lines of failed build steps.
INFO: Elapsed time: 4.087s, Critical Path: 3.93s
INFO: 2 processes: 2 internal.
FAILED: Build did NOT complete successfully
FAILED: Build did NOT complete successfully

The Gazelle-generated project does include the .S file, however:

go_library(
    name = "zstd",
    srcs = [
        ...
        "huf_compress.c",
        "huf_decompress.c",
        "huf_decompress_amd64.S", # <---

What did you expect to see?

Hello, World!

What did you see instead?

Use --sandbox_debug to see verbose messages from the sandbox and retain the sandbox build root for debugging
/tmp/rules_go_work-585792782/cgo/github.com/DataDog/zstd/_x16.o:huf_decompress.c:function HUF_decompress4X1_usingDTable_internal_bmi2_asm: error: undefined reference to 'HUF_decompress4X1_usingDTable_internal_bmi2_asm_loop'
/tmp/rules_go_work-585792782/cgo/github.com/DataDog/zstd/_x16.o:huf_decompress.c:function HUF_decompress4X2_usingDTable_internal_bmi2_asm: error: undefined reference to 'HUF_decompress4X2_usingDTable_internal_bmi2_asm_loop'
collect2: error: ld returned 1 exit status
compilepkg: error running subcommand /usr/bin/gcc: exit status 1
Target //:hello-go failed to build
Use --verbose_failures to see the command lines of failed build steps.
INFO: Elapsed time: 4.087s, Critical Path: 3.93s
INFO: 2 processes: 2 internal.
FAILED: Build did NOT complete successfully
FAILED: Build did NOT complete successfully
@fmeum
Copy link
Member

fmeum commented Dec 22, 2022

Looks like an incarnation of #2006.
@jayconrod Do you happen to recall what exactly would be needed to support this situation and resolve #2006?

@tals
Copy link
Author

tals commented Dec 29, 2022

I've also tried axing the asm build by passing in -DZSTD_DISABLE_ASM to copts, and now it blows up on portability_macros.h:42: unexpected token after '#': if (space between # and if ).

$ sed -n 40,45p portability_macros.h
#ifndef ZSTD_MEMORY_SANITIZER
#  if __has_feature(memory_sanitizer)
#    define ZSTD_MEMORY_SANITIZER 1
#  else
#    define ZSTD_MEMORY_SANITIZER 0

This also works correctly with the native golang toolchain.

prylabs-bulldozer bot added a commit to prysmaticlabs/prysm that referenced this issue Mar 2, 2023
* update

* gzl

* Using zstd workaround from @tals, per github.com/bazel-contrib/rules_go/issues/3411

* gaz

* hacky patch

---------

Co-authored-by: rkapka <[email protected]>
Co-authored-by: james-prysm <[email protected]>
Co-authored-by: Preston Van Loon <[email protected]>
Co-authored-by: prylabs-bulldozer[bot] <58059840+prylabs-bulldozer[bot]@users.noreply.github.com>
@jsharpe
Copy link
Member

jsharpe commented Jun 8, 2023

I hit this when trying to build go-ethereum - this PR fixes this by patching the build files but would be good if this just worked! https://github.com/aspect-build/rules_sol/pull/8/files#diff-1f2c31b701f066691b4a4024644f86e092ab731eb81cc5d6282017fe485082ae

@evanj
Copy link
Contributor

evanj commented Aug 8, 2023

Having debugged this a bit: It seems to me like there are two problems:

  1. rules_go is skipping this input .S, maybe because it is named the same as a GOARCH value, huf_decompress_amd64.S, which I think is a bug (see the Go build constraints documentation for details). I haven't been able to reproduce this separately yet.
  2. rules_go tries to run the Go assembler, rather than using the C compiler. The go build tool makes this switch for CGO_ENABLED packages, but gazelle does not. Ideally the workaround should not be needed.

I added a small demo program here which reproduces this error in a simpler context: https://github.com/evanj/rulesgotest/tree/main/cgoexample_asm

bazelisk run //cgoexample_asm
ERROR: rulesgotest/cgoexample_asm/BUILD.bazel:17:10: GoCompilePkg cgoexample_asm/cgoexample_asm.a failed: (Exit 1): builder failed: error executing command (from target //cgoexample_asm:cgoexample_asm) bazel-out/darwin_arm64-opt-exec-2B5CBBC6/bin/external/go_sdk/builder_reset/builder compilepkg -sdk external/go_sdk -installsuffix darwin_arm64 -src cgoexample_asm/cgoexample_asm.go -src ... (remaining 39 arguments skipped)

Use --sandbox_debug to see verbose messages from the sandbox and retain the sandbox build root for debugging
cgoexample_asm/asm_arm64.S:3: unexpected token after '#': if

@fmeum
Copy link
Member

fmeum commented Aug 9, 2023

@evanj Thanks for the detailed investigation!

Would you be interested in working on a fix for one or both of the issues you identified? I could support you along the way.

@evanj
Copy link
Contributor

evanj commented Aug 9, 2023

I'm going to investigate a bit in some spare time. Meanwhile: the patch approach is a viable workaround. I have an updated version of the patch from #3411 (comment) that works with the latest bazel/rules_go/zstd: https://gist.github.com/evanj/5f9619ace7a18a5acb8a1be9b9d4df23

fmeum added a commit that referenced this issue Aug 15, 2023
* compilepkg: cgo assembly uses the C compiler

This changes compilepkg to use the C compiler for .S and .s files to
use the C compiler, like go build does. Previously it would use the
Go assembler, which is used for pure Go packages. This should help
fix issue: #3411

I have added a cgo test for this issue that is intended to work with
both arm64 and amd64 machines. I have only tested it on Linux amd64
and Darwin arm64. Without this change it fails to build with the
following output. This fails to parse the assembly file because it
uses the Go assembler.

ERROR: rules_go/tests/core/cgo/asm/BUILD.bazel:3:11: GoCompilePkg
tests/core/cgo/asm/asm.a failed: (Exit 1): builder failed: error
executing command (from target //tests/core/cgo/asm:asm)
bazel-out/k8-opt-exec-2B5CBBC6/bin/external/go_sdk/builder_reset/builder
compilepkg -sdk external/go_sdk -installsuffix linux_amd64 -src
tests/core/cgo/asm/cgoasm.go -src tests/core/cgo/asm/asm_amd64.S ...

tests/core/cgo/asm/asm_amd64.S:4: expected identifier, found "."
asm: assembly of tests/core/cgo/asm/asm_amd64.S failed
compilepkg: error running subcommand external/go_sdk/pkg/tool/linux_amd64/asm: exit status 1

* add build constraint: unix && (amd64 || arm64)

* run buildifier; make asm conditional

* assembly fixes for Mac OS X and Linux

* Change build constraint to (amd64 || arm64): Should work on Windows

The tests were failing on Windows because rules_go incorrectly
believed this was a "native" Go package. I think this should work on
Windows.

---------

Co-authored-by: Fabian Meumertzheim <[email protected]>
fmeum pushed a commit that referenced this issue Aug 20, 2023
* cgo packages with assembly: Support CGO_ENABLED=0

Go supports building cgo packages both with and without Cgo. It uses
build constraints to exclude the appropriate files. When building a
Cgo package with assembly files, we must exclude them. Previous to
this change, rules_go would run the Go assembler on them. This meant
that packages which had "conditional" imports of cgo libraries with
assembly would fail when compiled with cgo disabled.

Add tests for these two cases:
* asm_conditional_cgo: A Go package that compiles both with and
  without Cgo.
* asm_dep_conditional_cgo: A Go package that conditionally imports
  a package with Cgo.

Fixes:
#3411

* code review improvements: remove unused main; clarify comment
@evanj
Copy link
Contributor

evanj commented Aug 20, 2023

I just confirmed with my independent reproduction that imports github.com/DataDog/zstd that this should work with the very latest version of rules_go! See https://github.com/evanj/ddzstdbazel

@fmeum
Copy link
Member

fmeum commented Aug 20, 2023

Great work and thanks for the very instructive example!

@tals
Copy link
Author

tals commented Aug 21, 2023

Awesome work @evanj !!

prestonvanloon added a commit to prysmaticlabs/prysm that referenced this issue Feb 27, 2024
github-merge-queue bot pushed a commit to prysmaticlabs/prysm that referenced this issue Feb 29, 2024
* downgrade level db

* fix current issues

* update geth

* Fix zstd build. The patch is now longer needed now that bazel-contrib/rules_go#3411 is fixed.

* Revert "update geth"

This reverts commit 2a7c51a.

* change to hex

---------

Co-authored-by: Preston Van Loon <[email protected]>
github-merge-queue bot pushed a commit to prysmaticlabs/prysm that referenced this issue Feb 29, 2024
* downgrade level db

* fix current issues

* update geth

* Fix zstd build. The patch is now longer needed now that bazel-contrib/rules_go#3411 is fixed.

* Revert "update geth"

This reverts commit 2a7c51a.

* change to hex

---------

Co-authored-by: Preston Van Loon <[email protected]>
nalepae pushed a commit to prysmaticlabs/prysm that referenced this issue Feb 29, 2024
* downgrade level db

* fix current issues

* update geth

* Fix zstd build. The patch is now longer needed now that bazel-contrib/rules_go#3411 is fixed.

* Revert "update geth"

This reverts commit 2a7c51a.

* change to hex

---------

Co-authored-by: Preston Van Loon <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants