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

dl_iterate_phdr implementation #2354

Merged
merged 5 commits into from
May 6, 2019
Merged

Conversation

LemonBoy
Copy link
Contributor

Also comes with a libc-less version that should work just fine in most cases.
I guess the code is portable enough to work on *BSDs too, I'd be happy to move it out of the linux namespace if somebody tests it.

I didn't manage to mark the __ehdr_entry as weak, similar to how you'd do with this C declaration:

extern const char __ehdr_start __attribute__((weak));

I'll squash the spurious commit once the review is over.

@andrewrk
Copy link
Member

This looks good. I'm taking a look into the extern weak thing and see if I can resolve that, then I'll merge.

@andrewrk
Copy link
Member

Ah - I do think it would be valuable to get some test coverage for this. Related:

if (false) {
// TODO this test is disabled because it is failing on the CI server's linux. when this is fixed
// enable it for at least linux
// TODO hook up the DynLib API for windows using LoadLibraryA
// TODO figure out how to make this work on darwin - probably libSystem has dlopen/dlsym in it
cases.addBuildFile("test/standalone/load_dynamic_library/build.zig");
}

std/c/linux.zig Outdated Show resolved Hide resolved
std/dynamic_library.zig Outdated Show resolved Hide resolved
std/dynamic_library.zig Outdated Show resolved Hide resolved
std/os/linux.zig Outdated Show resolved Hide resolved
std/os/linux.zig Show resolved Hide resolved
@andrewrk
Copy link
Member

andrewrk commented May 3, 2019

Some fix ups for you to look at, and then I do think we should have tests for this, and then I think it will be good to merge.

@@ -42,3 +44,39 @@ test "timer" {
// TODO implicit cast from *[N]T to [*]T
err = linux.epoll_wait(@intCast(i32, epoll_fd), @ptrCast([*]linux.epoll_event, &events), 8, -1);
}

export fn iter_fn(info: *linux.dl_phdr_info, size: usize, data: ?*usize) i32 {
var counter = data orelse @panic("???");
Copy link
Contributor

Choose a reason for hiding this comment

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

Could just be orelse unreachable I think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch, I went with .?

@LemonBoy
Copy link
Contributor Author

LemonBoy commented May 5, 2019

Is this PR ok to merge even before we solve the weak thing? What are the implications of this?

It should not matter since lld produces that symbol (and so does binutils as).

I've added a simple test for the non-libc case that should be quite exhaustive.

A few notes:

  • Is the exposed API ergonomic enough? I've made it accept a comptime T: type parameter in order to make it more type-safe but that's it.
  • We require the callback to be extern because we may call the libc function (IMO that's not needed, this implementation is 100% compatible with either libc and, if I didn't overlook anything, also with BSDs). This means the callback is effectively exported even though that's not needed, all we care for is the calling convention. Until use a builtin enum for calling conventions instead of keywords #661 (and unless @export is extended to also allow the user to select the symbol visibility) there's no way around this.

var counter: usize = 0;
const r = linux.dl_iterate_phdr(usize, iter_fn, &counter);
expect(r == 0);
expect(counter == 1);
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this liable to break in future if we somehow end up using other libraries in the test suite?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, but the test is easy to change when/if that happens.

@andrewrk andrewrk self-requested a review May 5, 2019 18:19
@andrewrk
Copy link
Member

andrewrk commented May 5, 2019

Test failure on the CI:

848/1106 os.linux.test.test "std-linux-x86_64-Debug-c-multi dl_iterate_phdr"...test failure
/home/vsts/work/1/s/std/testing.zig:146:14: 0x2ad418 in testing.expect (test)
/home/vsts/work/1/s/std/os/linux/test.zig:80:11: 0x3085a2 in os.linux.test.test "std-linux-x86_64-Debug-c-multi dl_iterate_phdr" (test)
/home/vsts/work/1/s/std/special/test_runner.zig:13:25: 0x6e8cca in std.special.main (test)
/home/vsts/work/1/s/std/special/bootstrap.zig:126:22: 0x6e8996 in std.special.main (test)

@LemonBoy
Copy link
Contributor Author

LemonBoy commented May 6, 2019

Test failure on the CI:

@daurnimator was right, the test was too flakey. I've replaced it with one inspired by this one and now the CI is green :)

@andrewrk andrewrk merged commit 7432fb0 into ziglang:master May 6, 2019
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.

3 participants