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

[MachO] add -pagezero_size #11875

Merged
merged 4 commits into from
Jun 20, 2022
Merged

[MachO] add -pagezero_size #11875

merged 4 commits into from
Jun 20, 2022

Conversation

motiejus
Copy link
Contributor

Pass -pagezero_size to the MachO linker. This is the final
"unsupported linker arg" that I could chase that CGo uses. After this
and #11874 we may be able to fail on an "unsupported linker arg" instead
of emiting a warning.

Test case:

zig=/code/zig/build/zig
CGO_ENABLED=1 GOOS=darwin GOARCH=amd64 CC="$zig cc -target x86_64-macos" CXX="$zig c++ -target x86_64-macos" go build -a -ldflags "-s -w" cgo.go

I compiled a trivial CGo program and executed it on an amd64 Darwin
host.

To be honest, I am not entirely sure what this is doing. This feels
right after reading what this argument does in LLVM sources, but I am by
no means qualified to make MachO pull requests. Will take feedback.

Copy link
Member

@kubkon kubkon left a comment

Choose a reason for hiding this comment

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

Thanks @motiejus. I hope you won't mind if I take it from here. As it currently stands, this PR needs a bit more work as it doesn't check the alignment on the requested page size, nor does it get rid of the segment if the requested page size is 0.

I found this PR in LLVM which I will use a guide when patching things up a little: D118724.

@@ -4374,7 +4375,7 @@ fn populateMissingMetadata(self: *MachO) !void {
.segment = .{
.inner = .{
.segname = makeStaticString("__PAGEZERO"),
.vmsize = pagezero_vmsize,
.vmsize = self.base.options.pagezero_size orelse pagezero_vmsize,
Copy link
Member

Choose a reason for hiding this comment

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

AFAIK you cannot just set the value to whatever you want without aligning to the page size.

@kubkon kubkon self-assigned this Jun 17, 2022
@motiejus
Copy link
Contributor Author

Thanks @motiejus. I hope you won't mind if I take it from here. As it currently stands, this PR needs a bit more work as it doesn't check the alignment on the requested page size, nor does it get rid of the segment if the requested page size is 0.

I found this PR in LLVM which I will use a guide when patching things up a little: D118724.

By all means, thank you!

@kubkon
Copy link
Member

kubkon commented Jun 20, 2022

@motiejus I've applied all fixes that were required, and tested that the resultant binaries (in user-space) run fine with different __PAGEZERO segment sizes on both x86_64 and arm64. I haven't tested CGO though so if you could give it a spin I'd be very grateful. I've also noted that your original patch wasn't correctly transferring the value of user-requested pagezero_size to the linker hence it might be worth double checking that CGO will actually work with these changes. Finally, also note that __PAGEZERO of VM size 0 is an immediate kill on both x86_64 and arm64 for me - apparently this requires running directly in kernel-space, judging by the description of this LLD's patch D118724.

@kubkon kubkon removed their assignment Jun 20, 2022
motiejus and others added 2 commits June 20, 2022 13:39
Pass `-pagezero_size` to the MachO linker. This is the final
"unsupported linker arg" that I could chase that CGo uses. After this
and ziglang#11874 we may be able to fail on an "unsupported linker arg" instead
of emiting a warning.

Test case:

    zig=/code/zig/build/zig
    CGO_ENABLED=1 GOOS=darwin GOARCH=amd64 CC="$zig cc -target x86_64-macos" CXX="$zig c++ -target x86_64-macos" go build -a -ldflags "-s -w" cgo.go

I compiled a trivial CGo program and executed it on an amd64 Darwin
host.

To be honest, I am not entirely sure what this is doing. This feels
right after reading what this argument does in LLVM sources, but I am by
no means qualified to make MachO pull requests. Will take feedback.
If page aligned requested pagezero size is 0, skip generating
__PAGEZERO segment.

Add misc improvements to the pipeline, and correctly transfer the
requested __PAGEZERO size to the linker.
@motiejus
Copy link
Contributor Author

@motiejus I've applied all fixes that were required, and tested that the resultant binaries (in user-space) run fine with different __PAGEZERO segment sizes on both x86_64 and arm64. I haven't tested CGO though so if you could give it a spin I'd be very grateful.

OK, will report here.

I've also noted that your original patch wasn't correctly transferring the value of user-requested pagezero_size to the linker hence it might be worth double checking that CGO will actually work with these changes.

The cgo binary executed; that's all I can report. It was very simple though, so maybe it was not hitting a code path that required the pagezero_size to be a non-default.

Finally, also note that __PAGEZERO of VM size 0 is an immediate kill on both x86_64 and arm64 for me - apparently this requires running directly in kernel-space, judging by the description of this LLD's patch D118724.

Ack.

@kubkon
Copy link
Member

kubkon commented Jun 20, 2022

I've also noted that your original patch wasn't correctly transferring the value of user-requested pagezero_size to the linker hence it might be worth double checking that CGO will actually work with these changes.

The cgo binary executed; that's all I can report. It was very simple though, so maybe it was not hitting a code path that required the pagezero_size to be a non-default.

No, you literally forgot to pass the value to link.zig which then wasn't passed to MachO.zig and hence the __PAGEZERO segment wasn't modified in any way :-)

@motiejus
Copy link
Contributor Author

motiejus commented Jun 20, 2022

cgo.go

package main

// #define _FILE_OFFSET_BITS 64
// #include <unistd.h>
// #include <fcntl.h>
// #include <stdio.h>
// char* hello() { return "hello, world"; }
// void phello() { printf("%s, your lucky number is %p\n", hello(), fcntl); }
import "C"

func main() {
        C.phello()
}

func Chello() string {
        return C.GoString(C.hello())
}

Compile:

$ $ CGO_ENABLED=1 GOOS=darwin GOARCH=amd64 CC="$zig cc -target x86_64-macos" CXX="$zig c++ -target x86_64-macos" go build -a -ldflags "-s -w" cgo.go
# command-line-arguments
warning: unsupported linker arg: -no_pie
warning(link): requested __PAGEZERO size is not page aligned
warning(link):   rounding down to 0x3d0000

Golang requests this:

				argv = append(argv, "-Wl,-pagezero_size,4000000")

https://github.com/golang/go/blob/go1.18.3/src/cmd/link/internal/ld/lib.go#L1327

... which, if unaligned, should probably be reported to them?

Also, it would be good if the warning message would report the requested pagezero_size together with the "rounded down" to make debugging easier.

Also, the resulting binary executes correctly.

@kubkon
Copy link
Member

kubkon commented Jun 20, 2022

cgo.go

package main

// #define _FILE_OFFSET_BITS 64
// #include <unistd.h>
// #include <fcntl.h>
// #include <stdio.h>
// char* hello() { return "hello, world"; }
// void phello() { printf("%s, your lucky number is %p\n", hello(), fcntl); }
import "C"

func main() {
        C.phello()
}

func Chello() string {
        return C.GoString(C.hello())
}

Compile:

$ $ CGO_ENABLED=1 GOOS=darwin GOARCH=amd64 CC="$zig cc -target x86_64-macos" CXX="$zig c++ -target x86_64-macos" go build -a -ldflags "-s -w" cgo.go
# command-line-arguments
warning: unsupported linker arg: -no_pie
warning(link): requested __PAGEZERO size is not page aligned
warning(link):   rounding down to 0x3d0000

CGO_ENABLED=1 GOOS=darwin GOARCH=amd64 CC="$zig cc -target x86_64-macos" CXX="$zig c++ -target x86_64-macos" go build -a -ldflags "-s -w" cgo.go
# command-line-arguments
warning: unsupported linker arg: -no_pie
warning(link): requested __PAGEZERO size is not page aligned
warning(link):   rounding down to 0x3d0000

Golang requests this:

				argv = append(argv, "-Wl,-pagezero_size,4000000")

https://github.com/golang/go/blob/go1.18.3/src/cmd/link/internal/ld/lib.go#L1327

... which, if unaligned, should probably be reported to them?

Oh, I think we don't parse it correctly. -pagezero_size should assume the input value in hex. Lemme double check as in hex this is properly aligned value.

Also, it would be good if the warning message would report the requested pagezero_size together with the "rounded down" to make debugging easier.

Sounds good.

Also, the resulting binary executes correctly.

This matches the behavior of other linkers out there including
`ld64` and `lld`.
@kubkon
Copy link
Member

kubkon commented Jun 20, 2022

@motiejus if you try running CGO again, the warning is gone - it was a false positive. The -pagezero_size value is always assumed to be hex which I've now fixed in 8752db3.

@motiejus
Copy link
Contributor Author

Thanks. I think we are in LGTM land now. :)

@kubkon kubkon merged commit 74ed7c1 into ziglang:master Jun 20, 2022
@motiejus motiejus deleted the pagezero-size branch June 22, 2022 09:09
motiejus added a commit to motiejus/zig that referenced this pull request Jun 22, 2022
Currently `zig cc`, when confronted with a linker argument it does
not understand, skips the flag and emits a warning.

This has been causing headaches for people that build third-party
software (including me). Zig seemingly builds and links the final
executable, only to segfault when running it.

If there are linker warnings when compiling software, the first thing we
have to do is add support for ones linker is complaining, and only then
go file issues. If zig "successfully" (i.e. status code = 0) compiles a
binary, there is instead a tendency to blaim "zig doing something
weird". (I am guilty of this.) In my experience, adding the unsupported
arguments has been quite easy; see ziglang#11679, ziglang#11875, ziglang#11874 for recent
examples.

With the current ones (+ prerequisites below) I was able to build all of
the CGo programs that I am encountering at $dayjob. CGo is a reasonable
example, because it is exercising the unusual linker args quite a bit.

Prerequisites: ziglang#11614 and ziglang#11863.
kubkon pushed a commit to motiejus/zig that referenced this pull request Jun 25, 2022
Currently `zig cc`, when confronted with a linker argument it does
not understand, skips the flag and emits a warning.

This has been causing headaches for people that build third-party
software (including me). Zig seemingly builds and links the final
executable, only to segfault when running it.

If there are linker warnings when compiling software, the first thing we
have to do is add support for ones linker is complaining, and only then
go file issues. If zig "successfully" (i.e. status code = 0) compiles a
binary, there is instead a tendency to blaim "zig doing something
weird". (I am guilty of this.) In my experience, adding the unsupported
arguments has been quite easy; see ziglang#11679, ziglang#11875, ziglang#11874 for recent
examples.

With the current ones (+ prerequisites below) I was able to build all of
the CGo programs that I am encountering at $dayjob. CGo is a reasonable
example, because it is exercising the unusual linker args quite a bit.

Prerequisites: ziglang#11614 and ziglang#11863.
@motiejus
Copy link
Contributor Author

Just checked that Go uses -Wl,-pagezero_size here.

andrewrk pushed a commit to motiejus/zig that referenced this pull request Dec 12, 2022
Currently `zig cc`, when confronted with a linker argument it does
not understand, skips the flag and emits a warning.

This has been causing headaches for people that build third-party
software (including me). Zig seemingly builds and links the final
executable, only to segfault when running it.

If there are linker warnings when compiling software, the first thing we
have to do is add support for ones linker is complaining, and only then
go file issues. If zig "successfully" (i.e. status code = 0) compiles a
binary, there is instead a tendency to blaim "zig doing something
weird". (I am guilty of this.) In my experience, adding the unsupported
arguments has been quite easy; see ziglang#11679, ziglang#11875, ziglang#11874 for recent
examples.

With the current ones (+ prerequisites below) I was able to build all of
the CGo programs that I am encountering at $dayjob. CGo is a reasonable
example, because it is exercising the unusual linker args quite a bit.

Prerequisites: ziglang#11614 and ziglang#11863.
andrewrk pushed a commit to motiejus/zig that referenced this pull request Jan 27, 2023
Currently `zig cc`, when confronted with a linker argument it does
not understand, skips the flag and emits a warning.

This has been causing headaches for people that build third-party
software (including me). Zig seemingly builds and links the final
executable, only to segfault when running it.

If there are linker warnings when compiling software, the first thing we
have to do is add support for ones linker is complaining, and only then
go file issues. If zig "successfully" (i.e. status code = 0) compiles a
binary, there is instead a tendency to blaim "zig doing something
weird". (I am guilty of this.) In my experience, adding the unsupported
arguments has been quite easy; see ziglang#11679, ziglang#11875, ziglang#11874 for recent
examples.

With the current ones (+ prerequisites below) I was able to build all of
the CGo programs that I am encountering at $dayjob. CGo is a reasonable
example, because it is exercising the unusual linker args quite a bit.

Prerequisites: ziglang#11614 and ziglang#11863.
motiejus added a commit to motiejus/zig that referenced this pull request Feb 9, 2023
Currently `zig cc`, when confronted with a linker argument it does
not understand, skips the flag and emits a warning.

This has been causing headaches for people that build third-party
software (including me). Zig seemingly builds and links the final
executable, only to segfault when running it.

If there are linker warnings when compiling software, the first thing we
have to do is add support for ones linker is complaining, and only then
go file issues. If zig "successfully" (i.e. status code = 0) compiles a
binary, there is instead a tendency to blaim "zig doing something
weird". (I am guilty of this.) In my experience, adding the unsupported
arguments has been quite easy; see ziglang#11679, ziglang#11875, ziglang#11874 for recent
examples.

With the current ones (+ prerequisites below) I was able to build all of
the CGo programs that I am encountering at $dayjob. CGo is a reasonable
example, because it is exercising the unusual linker args quite a bit.

Prerequisites: ziglang#11614 and ziglang#11863.
andrewrk pushed a commit that referenced this pull request Feb 17, 2023
Currently `zig cc`, when confronted with a linker argument it does
not understand, skips the flag and emits a warning.

This has been causing headaches for people that build third-party
software (including me). Zig seemingly builds and links the final
executable, only to segfault when running it.

If there are linker warnings when compiling software, the first thing we
have to do is add support for ones linker is complaining, and only then
go file issues. If zig "successfully" (i.e. status code = 0) compiles a
binary, there is instead a tendency to blaim "zig doing something
weird". (I am guilty of this.) In my experience, adding the unsupported
arguments has been quite easy; see #11679, #11875, #11874 for recent
examples.

With the current ones (+ prerequisites below) I was able to build all of
the CGo programs that I am encountering at $dayjob. CGo is a reasonable
example, because it is exercising the unusual linker args quite a bit.

Prerequisites: #11614 and #11863.
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 this pull request may close these issues.

None yet

2 participants