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

struct addrinfo needs padding on Solaris/SPARC #43649

Closed
dhduvall opened this issue Aug 4, 2017 · 7 comments
Closed

struct addrinfo needs padding on Solaris/SPARC #43649

dhduvall opened this issue Aug 4, 2017 · 7 comments
Labels
O-SPARC Target: SPARC processors T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@dhduvall
Copy link

dhduvall commented Aug 4, 2017

On Solaris, struct addrinfo is defined like this:

struct addrinfo {
        int ai_flags;           /* AI_PASSIVE, AI_CANONNAME, ... */
        int ai_family;          /* PF_xxx */
        int ai_socktype;        /* SOCK_xxx */
        int ai_protocol;        /* 0 or IPPROTO_xxx for IPv4 and IPv6 */
#ifdef __sparcv9
        int _ai_pad;            /* for backwards compat with old size_t */
#endif /* __sparcv9 */
        socklen_t ai_addrlen;
        char *ai_canonname;     /* canonical name for hostname */
        struct sockaddr *ai_addr;       /* binary address */
        struct addrinfo *ai_next;       /* next structure in linked list */
};

That padding member needs to be represented in Rust's copy. This is pretty straightforward, if a bit ugly. The following patch against the 1.19.0 sources fixes all the networking related test failures on SPARC:

--- rustc-1.19.0-src/src/liblibc/src/unix/solaris/mod.rs
+++ rustc-1.19.0-src/src/liblibc/src/unix/solaris/mod.rs
@@ -204,6 +204,8 @@
         pub ai_family: ::c_int,
         pub ai_socktype: ::c_int,
         pub ai_protocol: ::c_int,
+        #[cfg(target_arch = "sparc64")]
+        pub __sparcv9_pad: ::c_int,
         pub ai_addrlen: ::socklen_t,
         pub ai_canonname: *mut ::c_char,
         pub ai_addr: *mut ::sockaddr,
--- rustc-1.19.0-src/src/libstd/sys_common/net.rs
+++ rustc-1.19.0-src/src/libstd/sys_common/net.rs
@@ -165,8 +165,21 @@
     init();
 
     let c_host = CString::new(host)?;
+    #[cfg(all(target_os = "solaris", target_arch = "sparc64"))]
     let hints = c::addrinfo {
         ai_flags: 0,
+        ai_family: 0,
+        ai_socktype: c::SOCK_STREAM,
+        ai_protocol: 0,
+        __sparcv9_pad: 0,
+        ai_addrlen: 0,
+        ai_addr: ptr::null_mut(),
+        ai_canonname: ptr::null_mut(),
+        ai_next: ptr::null_mut()
+    };
+    #[cfg(not(all(target_os = "solaris", target_arch = "sparc64")))]
+    let hints = c::addrinfo {
+        ai_flags: 0,
         ai_family: 0,
         ai_socktype: c::SOCK_STREAM,
         ai_protocol: 0,

Because this is against 1.19.0, so the #[cfg] directive isn't yet allowed inside the initializer, though presumably that could be used on master, or beta with struct_field_attributes.

But I'm struggling to find an idiomatic way to do this in such a way that it needn't be mentioned in net.rs at all, and just defaulted to 0 in solaris/mod.rs, as well as a way to make it non-pub. I haven't yet tried it, but I assume I could use the Default trait. But that would still require me to add ..Default::default() to the initializer, and I would need to make the change to all the struct addrinfo definitions in libc.

Is there a better way to fix this?

@sanxiyn sanxiyn added the O-SPARC Target: SPARC processors label Aug 5, 2017
@nagisa
Copy link
Member

nagisa commented Aug 5, 2017

It is fine to use any unstable features you need in the implementation of rustc and its standard libraries. Feel free to send in a PR and r? me.

That being said the change against the structure itself will have to be PRd against https://github.com/rust-lang/libc first and only then the libstd itself changed.

@nagisa nagisa added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Aug 5, 2017
@nagisa
Copy link
Member

nagisa commented Aug 5, 2017

Something like this can be used too for the same effect:

     let hints = c::addrinfo {
         ai_socktype: c::SOCK_STREAM,
         .. mem::zeroed()
    };

@dhduvall
Copy link
Author

dhduvall commented Aug 6, 2017

Thanks, that's great! I'm not wild about having the padding member be public, but I don't see a way around that, so I submitted rust-lang/libc#714 for the libc part, and will follow up with a PR on this side shortly.

@sfackler
Copy link
Member

sfackler commented Aug 6, 2017

I don't think it needs to be public - there are private fields in other structs in libc.

@dhduvall
Copy link
Author

dhduvall commented Aug 6, 2017

There are, but (at least for the ones in the Solaris-specific module), none of those structs are ever initialized directly, AFAICT. At least, the compiler complained at me for it not being public. I can double-check, though.

@dhduvall
Copy link
Author

dhduvall commented Aug 6, 2017

Indeed; if I remove pub from the field declaration, this is what I get when I try to compile:

error[E0451]: field `__sparcv9_pad` of struct `libc::addrinfo` is private
   --> src/libstd/sys_common/net.rs:170:12
    |
170 |         .. unsafe { mem::zeroed() }
    |            ^^^^^^^^^^^^^^^^^^^^^^^^ field `__sparcv9_pad` is private

error: aborting due to previous error

error: Could not compile `std`.

@sfackler
Copy link
Member

sfackler commented Aug 6, 2017

You can do let mut hints = mem::zeroed(); hints.ai_socktype: c::SOCK_STREAM; instead.

bors added a commit to rust-lang/libc that referenced this issue Aug 7, 2017
struct addrinfo needs padding on Solaris/SPARC

64-bit Solaris/SPARC has a 4-byte pad before `ai_addrlen` for historical reasons (`ai_addrlen` used to be defined as a `size_t`, which is 4 bytes in ILP32 and 8 in LP64, but was converted to a 4-byte `socklen_t` in Solaris 10, which necessitated padding for (more or less) binary compatibility).

See rust-lang/rust#43649.
dhduvall pushed a commit to dhduvall/rust that referenced this issue Aug 7, 2017
bors added a commit that referenced this issue Aug 7, 2017
addrinfo hint in lookup_host() clean initialization on all platforms

Fixes #43649
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
O-SPARC Target: SPARC processors T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

4 participants