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.

Sign up for GitHub

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

Fix linker error if go_library with cgo references unresolved symbol #3174

Merged
merged 1 commit into from
Jun 4, 2022
Merged

Fix linker error if go_library with cgo references unresolved symbol #3174

merged 1 commit into from
Jun 4, 2022

Conversation

fmeum
Copy link
Collaborator

@fmeum fmeum commented Jun 3, 2022 •
edited
Loading

What type of PR is this?

Bug fix

What does this PR do? Why is it needed?

Fix linker error if go_library with cgo references unresolved symbol
Before this commit, if a go_library referenced an unresolved C symbol,
the build would fail with an error about an undefined symbol since
compilepkg.go tries to link all object files into an executable to be
consumed by cgo's -dynimport argument.

Since this library is never used for any purpose other than having its
symbols parsed by the cgo command, retry with additional linker flags
intended to make the linker ignore unresolved symbols. If they remain
missing in the real link step, they will still be reported as unresolved
there.

Fixes golang/go#25832 for rules_go.

@fmeum fmeum force-pushed the fix-cgo-dynimport-undefined-symbols branch 7 times, most recently from 3341c9c to fd9a38a Compare June 4, 2022 12:54
@fmeum fmeum changed the title Fix linker error if go_library with cgo references external symbol Fix linker error if go_library with cgo references unresolved symbol Jun 4, 2022
@fmeum fmeum force-pushed the fix-cgo-dynimport-undefined-symbols branch 2 times, most recently from 1ed2b13 to b8aa7de Compare June 4, 2022 13:55
@fmeum
Fix linker error if go_library with cgo references unresolved symbol
1aa2af7 
Before this commit, if a go_library referenced an unresolved C symbol,
the build would fail with an error about an undefined symbol since
compilepkg.go tries to link all object files into an executable to be
consumed by cgo's -dynimport argument.

Since this library is never used for any purpose other than having its
symbols parsed by the cgo command, retry with additional linker flags
intended to make the linker ignore unresolved symbols. If they remain
missing in the real link step, they will still be reported as unresolved
there.

Fixes golang/go#25832 for rules_go.
@fmeum fmeum force-pushed the fix-cgo-dynimport-undefined-symbols branch from b8aa7de to 1aa2af7 Compare June 4, 2022 15:54
@fmeum fmeum marked this pull request as ready for review June 4, 2022 16:05
@linzhp
Copy link
Contributor

linzhp commented Jun 4, 2022

I think this makes the go_library target harder to use. Anyone who wants to import :use_external_symbol has to provide external_symbol somewhere. It sounds like a bad practice that we should discourage

@fmeum
Copy link
Collaborator Author

fmeum commented Jun 4, 2022 •
edited
Loading

@linzhp I agree the test itself is quite pathological, I just wanted to avoid having to add more source files to it and thus (ab)used CGo directives to get an unresolved symbol.

There are realistic use cases of this pattern though: For example, at my company, we develop fuzzers based on LLVM's libFuzzer and would like to interact with its API from go_library targets via CGo. However, we can only link against the actual libFuzzer library from a cc_binary that bundles in the CGo libraries. Without allowing for external symbols to remain undefined (as is quite common for cc_library), this isn't possible.

Edit: Put short: While questionale in Go, this pattern isn't uncommon in C and not having it working in CGo makes it difficult to have dependency chains C -> Go -> C, see also the linked Go issue.

@linzhp linzhp merged commit 6ab73ec into bazelbuild:master Jun 4, 2022
@fmeum fmeum deleted the fix-cgo-dynimport-undefined-symbols branch June 4, 2022 17:13
@fmeum fmeum mentioned this pull request Jun 5, 2022
Merged
gcf-merge-on-green bot pushed a commit to googleapis/gapic-generator-go that referenced this pull request Jun 6, 2022
@renovate-bot
chore(deps): update dependency io_bazel_rules_go to v0.33.0 (#1019)
e0c880d 
[![Mend Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com)

This PR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
| [io_bazel_rules_go](https://togithub.com/bazelbuild/rules_go) | http_archive | minor | `v0.32.0` -> `v0.33.0` |

---

### Release Notes

<details>
<summary>bazelbuild/rules_go</summary>

### [`v0.33.0`](https://togithub.com/bazelbuild/rules_go/releases/tag/v0.33.0)

[Compare Source](https://togithub.com/bazelbuild/rules_go/compare/v0.32.0...v0.33.0)

##### Breaking changes

-   [cover](https://togithub.com/bazelbuild/rules_go/blob/master/go/toolchains.rst#cover) has been removed in v0.32.0. Please [leave a comment](https://togithub.com/bazelbuild/rules_go/issues/3168) if you are affected by this.

##### Deprecations

-   The [asm](https://togithub.com/bazelbuild/rules_go/blob/master/go/toolchains.rst#asm), [compile](https://togithub.com/bazelbuild/rules_go/blob/master/go/toolchains.rst#compile), and [pack](https://togithub.com/bazelbuild/rules_go/blob/master/go/toolchains.rst#pack) action generators provided by [go_context](https://togithub.com/bazelbuild/rules_go/blob/master/go/toolchains.rst#go_context) are deprecated and planned for removal in version v0.36.0. Please leave a comment on the [tracking bug](https://togithub.com/bazelbuild/rules_go/issues/3168) if [archive](https://togithub.com/bazelbuild/rules_go/blob/master/go/toolchains.rst#archive) and [link](https://togithub.com/bazelbuild/rules_go/blob/master/go/toolchains.rst#link) are not suitable replacements for your use cases.

##### Bug Fixes

-   [@&#8203;sluongng](https://togithub.com/sluongng) fixed a race condition that could cause non-sandboxed builds of `go_test` targets to fail ([bazelbuild/rules_go#3145)
-   [@&#8203;abhinav](https://togithub.com/abhinav) made `//go:embed` work with `go_path` ([bazelbuild/rules_go#3163)
-   [@&#8203;xytan0056](https://togithub.com/xytan0056) made gopackagesdriver work with Go 1.18 ([bazelbuild/rules_go#3157)
-   [@&#8203;nickgooding](https://togithub.com/nickgooding) ensured that `gomock` can be used with any Gazelle naming convention ([bazelbuild/rules_go#3155)
-   `go_library` targets using CGo can now reference unresolved symbols ([bazelbuild/rules_go#3174)

Thanks to all of the contributors!

##### Compatibility

The minimum required version of Bazel remains at 4.2.1.

##### Updated dependencies

-   Updated `org_golang_x_sys`, `org_golang_x_xerrors`, `org_golang_google_genproto`, `go_googleapis` to their most recent commit as of 2022-06-05

As always, you can use higher versions of rules_go's dependencies by declaring them in WORKSPACE before calling go_rules_dependencies. Lower versions may work but are not supported.

##### `WORKSPACE` code

    load("@&#8203;bazel_tools//tools/build_defs/repo:http.bzl", "http_archive")

    http_archive(
        name = "io_bazel_rules_go",
        sha256 = "685052b498b6ddfe562ca7a97736741d87916fe536623afb7da2824c0211c369",
        urls = [
            "https://mirror.bazel.build/github.com/bazelbuild/rules_go/releases/download/v0.33.0/rules_go-v0.33.0.zip",
            "https://github.com/bazelbuild/rules_go/releases/download/v0.33.0/rules_go-v0.33.0.zip",
        ],
    )

    load("@&#8203;io_bazel_rules_go//go:deps.bzl", "go_register_toolchains", "go_rules_dependencies")

    go_rules_dependencies()

    go_register_toolchains(version = "1.18.3")

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

♻ **Rebasing**: Whenever PR is behind base branch, or you tick the rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update again.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, click this checkbox.

---

This PR has been generated by [Mend Renovate](https://www.mend.io/free-developer-tools/renovate/). View repository job log [here](https://app.renovatebot.com/dashboard#github/googleapis/gapic-generator-go).
gcf-merge-on-green bot pushed a commit to googleapis/gapic-config-validator that referenced this pull request Jun 6, 2022
@renovate-bot
chore(deps): update dependency io_bazel_rules_go to v0.33.0 (#245)
b4e7f6e 
[![Mend Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com)

This PR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
| [io_bazel_rules_go](https://togithub.com/bazelbuild/rules_go) | http_archive | minor | `v0.32.0` -> `v0.33.0` |

---

### Release Notes

<details>
<summary>bazelbuild/rules_go</summary>

### [`v0.33.0`](https://togithub.com/bazelbuild/rules_go/releases/tag/v0.33.0)

[Compare Source](https://togithub.com/bazelbuild/rules_go/compare/v0.32.0...v0.33.0)

#### Breaking changes

-   [cover](https://togithub.com/bazelbuild/rules_go/blob/master/go/toolchains.rst#cover) has been removed in v0.32.0. Please [leave a comment](https://togithub.com/bazelbuild/rules_go/issues/3168) if you are affected by this.

#### Deprecations

-   The [asm](https://togithub.com/bazelbuild/rules_go/blob/master/go/toolchains.rst#asm), [compile](https://togithub.com/bazelbuild/rules_go/blob/master/go/toolchains.rst#compile), and [pack](https://togithub.com/bazelbuild/rules_go/blob/master/go/toolchains.rst#pack) action generators provided by [go_context](https://togithub.com/bazelbuild/rules_go/blob/master/go/toolchains.rst#go_context) are deprecated and planned for removal in version v0.36.0. Please leave a comment on the [tracking bug](https://togithub.com/bazelbuild/rules_go/issues/3168) if [archive](https://togithub.com/bazelbuild/rules_go/blob/master/go/toolchains.rst#archive) and [link](https://togithub.com/bazelbuild/rules_go/blob/master/go/toolchains.rst#link) are not suitable replacements for your use cases.

#### Bug Fixes

-   [@&#8203;sluongng](https://togithub.com/sluongng) fixed a race condition that could cause non-sandboxed builds of `go_test` targets to fail ([bazelbuild/rules_go#3145)
-   [@&#8203;abhinav](https://togithub.com/abhinav) made `//go:embed` work with `go_path` ([bazelbuild/rules_go#3163)
-   [@&#8203;xytan0056](https://togithub.com/xytan0056) made gopackagesdriver work with Go 1.18 ([bazelbuild/rules_go#3157)
-   [@&#8203;nickgooding](https://togithub.com/nickgooding) ensured that `gomock` can be used with any Gazelle naming convention ([bazelbuild/rules_go#3155)
-   `go_library` targets using CGo can now reference unresolved symbols ([bazelbuild/rules_go#3174)

Thanks to all of the contributors!

#### Compatibility

The minimum required version of Bazel remains at 4.2.1.

#### Updated dependencies

-   Updated `org_golang_x_sys`, `org_golang_x_xerrors`, `org_golang_google_genproto`, `go_googleapis` to their most recent commit as of 2022-06-05

As always, you can use higher versions of rules_go's dependencies by declaring them in WORKSPACE before calling go_rules_dependencies. Lower versions may work but are not supported.

#### `WORKSPACE` code

    load("@&#8203;bazel_tools//tools/build_defs/repo:http.bzl", "http_archive")

    http_archive(
        name = "io_bazel_rules_go",
        sha256 = "685052b498b6ddfe562ca7a97736741d87916fe536623afb7da2824c0211c369",
        urls = [
            "https://mirror.bazel.build/github.com/bazelbuild/rules_go/releases/download/v0.33.0/rules_go-v0.33.0.zip",
            "https://github.com/bazelbuild/rules_go/releases/download/v0.33.0/rules_go-v0.33.0.zip",
        ],
    )

    load("@&#8203;io_bazel_rules_go//go:deps.bzl", "go_register_toolchains", "go_rules_dependencies")

    go_rules_dependencies()

    go_register_toolchains(version = "1.18.3")

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

♻ **Rebasing**: Whenever PR is behind base branch, or you tick the rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update again.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, click this checkbox.

---

This PR has been generated by [Mend Renovate](https://www.mend.io/free-developer-tools/renovate/). View repository job log [here](https://app.renovatebot.com/dashboard#github/googleapis/gapic-config-validator).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

cmd/cgo: undefined reference for C function with -buildmode=c-archive
2 participants
@fmeum @linzhp