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: make zig cc pass -l/-L like Clang/GCC for ELF #19818

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

fix: make zig cc pass -l/-L like Clang/GCC for ELF #19818

wants to merge 3 commits into from
+90 −16

Conversation

a-khabarov
Copy link

This PR is my attempt to fix #19699 by making the way zig cc passes -l/-L flags for ELF linking consistent with Clang and GCC.
I've added a test case that mimics the example in the issue description. Without the changes this PR makes to src, the new test case fails as expected with

========= expected to find: ==========================
NEEDED libfoo.so
========= but parsed file does not contain it: =======
dynamic section
RUNPATH foo
NEEDED foo/libfoo.so
NEEDED libc.so.6

I am not sure if test/tests.zig is the best place for it - I couldn't find a good way to test this in test/link/elf.zig or test/standalone.

@a-khabarov
Copy link
Author

@motiejus @kubkon This PR builds on the functionality implemented in #15743 and should also fix cc_test not working correctly when using hermetic_cc_toolchain as mentioned in #15743 (comment).

kubkon
kubkon requested changes May 1, 2024
View reviewed changes
test/tests.zig Outdated
@@ -769,6 +769,54 @@ pub fn addCliTests(b: *std.Build) *Step {
step.dependOn(&cleanup.step);
}

{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This set of tests belongs in linker tests in test/link/elf.zig. I also don't understand why can't it be orchestrated using the build system? Why do you need to invoke zig exe via addSystemCommand? exe.linkSystemLibrary("foo") emits as -lfoo fwiw. Did you actually have a look at test/link/elf.zig? We do have a substantial amount of tests that rely on emitting and linking against system libs.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've moved the testing to test/link/elf.zig. The new commits add a few bits of extra functionality that are needed to reproduce the problem.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kubkon I think I've addressed your comments on this PR, any thoughts?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@andrewrk @kubkon Could you please have a look at this PR?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@andrewrk @kubkon Is there any way we can land this PR in 0.14.0? Please see my email regarding this

@a-khabarov a-khabarov force-pushed the fix-make-zig-cc-pass-l-L-like-Clang-GCC-for-ELF branch from 5f810f0 to 1860048 Compare May 20, 2024 12:05
@a-khabarov a-khabarov requested a review from kubkon May 20, 2024 12:23
@a-khabarov a-khabarov force-pushed the fix-make-zig-cc-pass-l-L-like-Clang-GCC-for-ELF branch from 9107b5a to 8d5fc36 Compare June 4, 2024 15:38
@a-khabarov a-khabarov force-pushed the fix-make-zig-cc-pass-l-L-like-Clang-GCC-for-ELF branch from b4dcd83 to 094faf4 Compare June 17, 2024 10:02
@a-khabarov a-khabarov force-pushed the fix-make-zig-cc-pass-l-L-like-Clang-GCC-for-ELF branch from 094faf4 to b3cfcab Compare July 5, 2024 16:08
@a-khabarov a-khabarov force-pushed the fix-make-zig-cc-pass-l-L-like-Clang-GCC-for-ELF branch from b3cfcab to f211d2d Compare July 26, 2024 12:26
@alexrp alexrp added this to the 0.14.0 milestone Feb 19, 2025
@a-khabarov
Copy link
Author

I am in the process of rebasing this on current mainline

@a-khabarov a-khabarov force-pushed the fix-make-zig-cc-pass-l-L-like-Clang-GCC-for-ELF branch from f211d2d to 46752f5 Compare February 21, 2025 15:57
@a-khabarov a-khabarov changed the title fix: make zig cc pass -l/-L like Clang/GCC for ELF fix: make zig cc pass -l/-L like Clang/GCC for ELF Feb 21, 2025
@a-khabarov a-khabarov force-pushed the fix-make-zig-cc-pass-l-L-like-Clang-GCC-for-ELF branch from 46752f5 to 2af0d55 Compare February 21, 2025 17:02
@a-khabarov
Copy link
Author

I've rebased this PR.

The fix itself is in src/link/Elf.zig.

The new test is in test/link/elf.zig. It follows the example from
#19699.

The rest of the changes are new functionality needed for the test:

  • omit_soname option for Build.Module, OverlayOptions.
    This passes -fno-soname to the linker when enabled.
  • addLibraryPathSpecial for Build.Module, Build.Step.Compile.
    Like addLibraryPath but for []const u8.
    addRPathSpecial is also added for completeness.
  • setCwd for Build.Step.Compile.
    Allows setting current directory for compile steps.
  • opt_cwd for evalZigProcess in Build.Step.
    Allows running evalZigProcess in a specific directory.
    Needed for setting current directory for compile steps.
  • --zig-lib-dir is now appended differently in Build.Step.Compile.
    This is needed for compatibility with setCwd.

@a-khabarov a-khabarov force-pushed the fix-make-zig-cc-pass-l-L-like-Clang-GCC-for-ELF branch from 2af0d55 to b8f46fa Compare February 24, 2025 19:28
@a-khabarov
Copy link
Author

I made some changes to avoid reconstructing the library name and directory in src/link/Elf.zig.
Everything is now passed via src/link.zig

a-khabarov added a commit to a-khabarov/zig that referenced this pull request Feb 25, 2025
@a-khabarov
lib: implement addLibraryPathSpecial
b80a4ae 
This commit adds `addLibraryPathSpecial` for `Build.Module` and
`Build.Step.Compile`. Like `addLibraryPath` but for `[]const u8`.
`addRPathSpecial` is also added for `Build.Step.Compile` for
completeness.

This is needed to implement the new test in
ziglang#19818.
a-khabarov added a commit to a-khabarov/zig that referenced this pull request Feb 25, 2025
@a-khabarov
lib: add support for omitting soname
fc2de1c 
This commit adds the `soname` option for `Build.Module` and
`OverlayOptions`. This controls whether `-fsoname` or `-fno-soname` is
passed to the linker.

This is needed to implement the new test in
ziglang#19818
a-khabarov added a commit to a-khabarov/zig that referenced this pull request Feb 25, 2025
@a-khabarov
lib: allow setting current directory for compile steps
585e70b 
We need this to implement the new test in
ziglang#19818

This commits adds:

* `setCwd` for `Build.Step.Compile`.
  Allows setting current directory for compile steps.
* `opt_cwd` for `evalZigProcess` in `Build.Step`.
  Allows running `evalZigProcess` in a specific directory.
  Needed for setting current directory for compile steps.

`--zig-lib-dir` is now appended differently in `Build.Step.Compile`.
This is needed for compatibility with `setCwd`.
@a-khabarov a-khabarov force-pushed the fix-make-zig-cc-pass-l-L-like-Clang-GCC-for-ELF branch from b8f46fa to 077f287 Compare February 25, 2025 15:37
a-khabarov added a commit to a-khabarov/zig that referenced this pull request Feb 25, 2025
@a-khabarov
lib: add support for omitting soname
d708c4c 
This commit adds the `omit_soname` option for `Build.Module` and
`OverlayOptions`. This controls whether `-fsoname` or `-fno-soname` is
passed to the linker.

This is needed to implement the new test in
ziglang#19818
a-khabarov added a commit to a-khabarov/zig that referenced this pull request Feb 25, 2025
@a-khabarov
lib: allow setting current directory for compile steps
67441e6 
We need this to implement the new test in
ziglang#19818

This commits adds:

* `setCwd` for `Build.Step.Compile`.
  Allows setting current directory for compile steps.
* `opt_cwd` for `evalZigProcess` in `Build.Step`.
  Allows running `evalZigProcess` in a specific directory.
  Needed for setting current directory for compile steps.

`--zig-lib-dir` is now appended differently in `Build.Step.Compile`.
This is needed for compatibility with `setCwd`.
@a-khabarov a-khabarov force-pushed the fix-make-zig-cc-pass-l-L-like-Clang-GCC-for-ELF branch from 077f287 to ac69db5 Compare February 25, 2025 15:44
a-khabarov added a commit to a-khabarov/zig that referenced this pull request Feb 25, 2025
@a-khabarov
lib: add support for omitting soname
4c3367a 
This commit adds the `soname` option for `Build.Module` and
`OverlayOptions`. This controls whether `-fsoname` or `-fno-soname` is
passed to the linker.

This is needed to implement the new test in
ziglang#19818
@a-khabarov a-khabarov force-pushed the fix-make-zig-cc-pass-l-L-like-Clang-GCC-for-ELF branch from ac69db5 to 0a646aa Compare February 25, 2025 20:51
@a-khabarov
lib: add support for omitting soname
93050aa 
This commit adds the `soname` option for `Build.Module` and
`OverlayOptions`. This controls whether `-fsoname` or `-fno-soname` is
passed to the linker.

This is needed to implement the new test in
ziglang#19818
@a-khabarov
test: check that zig cc passes -l/-L like Clang/GCC for ELF
8a0363a 
This test follows the example from
ziglang#19699.
@a-khabarov
link: make zig cc pass -l/-L like Clang/GCC for ELF
b0c5bbb
@a-khabarov a-khabarov force-pushed the fix-make-zig-cc-pass-l-L-like-Clang-GCC-for-ELF branch from 0a646aa to b0c5bbb Compare February 25, 2025 20:53
@a-khabarov
Copy link
Author

a-khabarov commented Feb 25, 2025 •
edited
Loading

I've removed everything related to setCwd and addLibraryPathSpecial, it turns out that they are not necessary.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kubkon kubkon Awaiting requested review from kubkon

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
0.14.0
Development

Successfully merging this pull request may close these issues.

zig cc -l example sets the DSO paths incorrectly
3 participants
@a-khabarov @kubkon @alexrp