[API-breaking] Expose si_addr on siginfo_t. Refs #716

[alex]

This is the bare minimum to expose this. It also seems to have required making the unions pub, which I wasn't wild about, and I had no clue what to name the anonymous union. But it's a starting point!

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @gnzlbg (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

[alex]

Hmm, so the macros for unions don't automatically mark them Eq/Debug/etc. but we do for structs. These are fields in a public struct, so they need to be Eq somehow...

[gnzlbg]

In the open issue #716 I was hoping that the padding field would be private, which would have allowed us to change this, but for some reason it is public, so this would be a hard breaking change, which I don't think we can make until the next breaking version is released.

[alex]

Depending on how long we expect it to take to get this into a mergable state, perhaps it would make sense for me to send a seperate PR to make that field private, which could be merged/released sooner?

[gnzlbg]

Hmm, so the macros for unions don't automatically mark them Eq/Debug/etc. but we do for structs. These are fields in a public struct, so they need to be Eq somehow...

The problem is that a union does not tell which field is active, so automatically deriving these cannot in general work. I think it is ok for these unions to not implement those traits. You could manually implement Debug to at least print the union name.

Usually, these unions are members of structs, where the struct stores a discriminant that allows it to tell which field is the active one. For such structs one can manually implement PartialEq, Debug, etc.

[alex]

For bonus points 1.13 doesn't like unions :-(

[gnzlbg]

@alex yes, I hope I mentioned that in the issue. You need to use cfg(libc_union), generating both type variants (with and without union), and implementing the methods for both cases (e.g. without unions, you just read the appropriate type from the beginning of the padding field).

[alex]

Hmm, so I attempted to make s_no_extra_traits! support non-pub types, and that didn't seem to go so well. Am I barking up the wrong tree here?

[gnzlbg]

That should be possible, but I don't know how easy / hard doing that is. I would keep it simple for now, and just implement these outside any s_...! macros. We can figure out how to add support for these in the macros later. That might mean that the style lint might fail temporarily for this, that's ok.

[alex]

Ok, after much pain this now builds!

It's failing the size_of check, which is confusing to me, because it looks like it's failing in precisely the way you'd get if the repr(packed) was missing. A local test seems to indicate that this ought to work...

alexgaynor@penguin /tmp> cat t.rs 
use std::mem;

#[repr(C)]
#[derive(Copy, Clone)]
struct addr_bnd_t {
    lower: *mut (),
    upper: *mut (),
}

#[repr(C)]
#[derive(Copy, Clone)]
union sigfault_t_anonymous_union {
    addr_bnd: addr_bnd_t,
    pkey: u32,
}

#[repr(C)]
#[derive(Copy, Clone)]
struct sigfault_t {
    addr: *mut (),
    #[cfg(target_arch = "sparc")]
    trapno: i32,
    addr_lsb: i16,
    anonymous_union: sigfault_t_anonymous_union,
}

#[repr(C, packed)]
#[derive(Copy, Clone)]
union sifields_t {
    _pad: [i32; 29],
    sigfault: sigfault_t,
}

pub struct siginfo_t {
    pub si_signo: i32,
    pub si_errno: i32,
    pub si_code: i32,
    sifields: sifields_t,

    #[cfg(target_arch = "x86_64")]
    _align: [u64; 0],
    #[cfg(not(target_arch = "x86_64"))]
    _align: [usize; 0],
}

fn main() {
    println!("{}", mem::size_of::<siginfo_t>());
}
alexgaynor@penguin /tmp> rustc +nightly t.rs; and ./t
warning: type `siginfo_t` should have an upper camel case name
  --> t.rs:34:12
   |
34 | pub struct siginfo_t {
   |            ^^^^^^^^^ help: convert the identifier to upper camel case: `SiginfoT`
   |
   = note: #[warn(non_camel_case_types)] on by default

warning: field is never used: `sifields`
  --> t.rs:38:5
   |
38 |     sifields: sifields_t,
   |     ^^^^^^^^^^^^^^^^^^^^
   |
   = note: #[warn(dead_code)] on by default

128

[alex]

Let's go around the room, what's the dumbest thing you've done so far today?

I'll go first: I didn't actually push the commit where I added packed.

No one else needs to go :-)

[gnzlbg]

Looking good! Some nitpicks!

[alex]

Is there anything I can be doing to help push this forward at this point, or are we fully blocked on needing to consider how to handle the backwards incompatibility?

[gnzlbg]

I've tried to inspect crates using the struct, but sourcegraph is broken AFAICT. We can't really use crater for this, since that won't catch code using libc from crates.io. I think it might be better to submit a parallel PR that marks the field as "deprecated", and do a release, telling people to report on an issue here, to see who (if anybody) is using the field.

[bors]

☔ The latest upstream changes (presumably #1329) made this pull request unmergeable. Please resolve the merge conflicts.

[jedisct1]

si_addr is now a function on Linux, but remains a field on MacOS, *BSD and Solaris derivatives.

For consistency, could it be a function everywhere?

[gnzlbg]

A consistent API is provided in the nix crate, would that solve your problem?

[jedisct1]

nix only reexports libc::siginfo_t.

[gnzlbg]

@jedisct1 then I think it might be worth it to raise this issue there (EDIT: reported here nix-rust/nix#1051). libc just exposes the system APIs, and the API of linux differs from that of many other OSes when it comes to siginfo_t.

[bors]

☔ The latest upstream changes (presumably #1346) made this pull request unmergeable. Please resolve the merge conflicts.

[bors]

☔ The latest upstream changes (presumably #1389) made this pull request unmergeable. Please resolve the merge conflicts.

[bors]

☔ The latest upstream changes (presumably #1390) made this pull request unmergeable. Please resolve the merge conflicts.

[DoumanAsh]

Just curious, I'm currently using _pad to get out provided pointer with my data when getting signal.

Currently compiler throws warning on it with reference to this PR, can I assume that any API breakage will lead to libc 0.2?

[alex]

@DoumanAsh don't use _pad, it leads to alignment issues -- instead cast the whole struct, as done here: https://github.com/rust-lang/libc/blob/master/src/unix/linux_like/linux/gnu/mod.rs#L185-L196

[DoumanAsh]

I'm not sure how you expect me to cast the whole struct, which I already have, but libc doesn't expose sigval field for me.
Regardless my question is whether this PR will bump minor version or patch version?

[gnzlbg]

@DoumanAsh which field are you trying to access precisely ? Maybe we can add a method here to provide this field, and you could start using that instead.

Regardless my question is whether this PR will bump minor version or patch version?

We don't know, will depend on breakage, but since this should use an union, a change is going to need to happen anyways. Doing a minor version bump of libc has huge ecosystem-wide consequences.

[DoumanAsh]

@gnzlbg si_value

I think most platforms aside freebsd has it as part of enum since it is used by Posix timers
But if libc would be able somehow imitate C like enums that would be actually great

On linux I can just read padding:

let raw_bytes = (*info)._pad;
let val: libc::sigval = ptr::read(raw_bytes[3..].as_ptr() as *const _);

[gnzlbg]

But if libc would be able somehow imitate C like enums that would be actually great

We want to use a proper rust union here, but we can't do that without breaking those who are accessing the padding field directly. I think we can just add a si_value method that returns that, and you can migrate to use that.

Would you like to send a PR implementing it ? The current siginfo_t already has a method exposing si_addr, you could add a si_value method that internally accesses the padding, just like you are doing right now in your code.

[DoumanAsh]

@gnzlbg To be honest I'm not exactly sure my method is the best one.
I wouldn't mind libc switching to unions, and then relying on it rather than trying to read particular offsets.
The problem is that I imagine libc doesn't want to bump minor version which means breaking changes are not allowed...

About PR, I'll try to take a look and add it for platforms with which I'm familiar enough

[gnzlbg]

The plan is to try to make the padding field private with a patch bump, hoping that it doesn't break anybody. If that success, we can modify the internals to use unions like this PR does, and once that works correctly, we can make them public.

We will need to keep the methods around for backward compatibility with toolchains that do not support unions (libc supports up to Rust 1.13.0...).

[bors]

☔ The latest upstream changes (presumably #1421) made this pull request unmergeable. Please resolve the merge conflicts.

[alex]

I'm going to close this for now, it's significantly bit-roting, and from my perspective the current implementation works well enough.