-
-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
add linker -wrap flag #15097
add linker -wrap flag #15097
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cc @kubkon
This is causing annoyance in Uber's Go Monorepo. If you agree this is a simple-enough change, can we get it landed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please bump the Compilation.link_hash_implementation_version
in
Lines 2185 to 2189 in 28d6dd7
/// This is only observed at compile-time and used to emit a compile error | |
/// to remind the programmer to update multiple related pieces of code that | |
/// are in different locations. Bump this number when adding or deleting | |
/// anything from the link cache manifest. | |
pub const link_hash_implementation_version = 7; |
Secondly, I see you've added -wrap
flag which affects zig build-exe
etc, however I don't see a matching addition to zig cc
which would fall under `-Wl,"--wrap=symbol"' most likely. See
Line 1659 in 28d6dd7
.wl => { |
src/main.zig
Outdated
@@ -2263,6 +2267,14 @@ fn buildOutputType( | |||
next_arg, | |||
}); | |||
}; | |||
} else if (mem.eql(u8, arg, "-wrap") or | |||
mem.eql(u8, arg, "--wrap")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see any reason in having --wrap
here too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Secondly, I see you've added -wrap flag which affects zig build-exe etc, however I don't see a matching addition to zig cc which would fall under `-Wl,"--wrap=symbol"' most likely.
I don't believe an explicit case -wrap case is necessary. The final else case takes care of flags that are split by comma
Line 1718 in 6f15eed
try linker_args.append(linker_arg); |
-Wl,--wrap=symbol
is taken care by
Line 1668 in 6f15eed
if (mem.indexOfScalar(u8, linker_arg, '=')) |equals_pos| { |
Building from source captures both of these scenarios
> ./zig2 cc wrap_example.c foo.c -Wl,-wrap,foo -o main
> ./main
foo:wrap foo
wrapfoo:wrap foo
realfoo:foo
> ./zig2 cc wrap_example.c foo.c -Wl,-wrap=foo -o main
> ./main
foo:wrap foo
wrapfoo:wrap foo
realfoo:foo
I don't believe this flag is available for build-exe
> ./zig2 build-exe wrap_example.c foo.c -Wl,-wrap=foo -o main
error: unrecognized parameter: '-Wl,-wrap=foo'
> ./zig2 build-exe wrap_example.c foo.c -Wl,-wrap,foo -o main
error: unrecognized parameter: '-Wl,-wrap,foo'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch about -Wl,-wrap
being implicitly handled in the loop - I missed that.
Re the latter, zig build-exe
does not understand -Wl
flag and should handle an explicit -wrap
flag.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this what you mean by supporting zig build-exe
?
> ./zig2 build-exe -lc main.c foo.c -wrap foo
> ./main
foo:wrap foo
wrapfoo:wrap foo
realfoo:foo
* use a set instead of a list * use of this flag currently requires LLD * add documentation * make it only a zig cc compatibility flag for now because I personally think this is an anti-feature.
Head branch was pushed to by a user without write access
hi @zachcheu, I'm sorry for confusing you - I added a commit to the branch and force pushed, and then set to auto-merge once the CI tests passed. It looks like you attempted to push some other work after that just now, but ran into git troubles. I can take it from here. Are you OK with that? |
No worries. Feel free to take it from here. Thanks! |
Fixes: #15051
This passes
-wrap
through to the old.Test