libsyntax_pos: Don't use packed attribute for Span on sparc64/v9

[glaubitz]

Due the limitation that #[derive(...)] on #[repr(packed)] structs does not guarantee proper alignment of the compiler-generated impls is not guaranteed (#39696), the change in #44646 to compress Spans results in the compiler generating code with unaligned access.

Until #39696 has been fixed, the issue can be worked around by not using the packed attribute on sparc64 and sparcv9 on the Span struct.

Fixes: #45509

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @nikomatsakis (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.

[kennytm]

You could simplify this whole thing into:

#[derive(Clone, Copy, PartialEq, Eq, Hash)]
#[cfg_attr(not(any(target_arch = "sparc64", target_arch = "sparcv9")), repr(packed))]
pub struct Span(u32);

[glaubitz]

Thanks! I was expecting that there would be a simpler way. I'll try it out.

[kennytm]

Please prefix the comment with FIXME: to make this line easier to be found.

[nikomatsakis]

Couldn't we just implement this safely on all architectures by implementing PartialEq ourselves?

e.g.

impl PartialEq for Span {
    fn eq(&self, other: &Span) {
        let a: u32 = self.0;
        let b: u32 = self.1;
        a == b
    }
}

(Maybe do the same for Clone, Hash, and friends?)

(@arielb1 and I had talked about just making #[derive] handle this automatically in this fashion for Copy types.)

[glaubitz]

Couldn't we just implement this safely on all architectures by implementing PartialEq ourselves?

Yes, that would be possible, too. We have verified this with:

extern crate core;
use core::cmp::PartialEq;

#[repr(packed)]
struct Demo(u8,u32);

impl PartialEq for Demo {
    fn eq(&self, rhs: &Demo) -> bool {
         // This crashes with bus error
         // PartialEq::eq(&self.0, &rhs.0) && PartialEq::eq(&self.1, &rhs.1)
         // This doesn't
         self.0 == rhs.0 && self.1 == rhs.1
    }
}

fn main() {
    let a = Demo(0,4);
    let b = Demo(0,4);                                                                                                                                                      
    if a != b {                                                                                                                                                             
        println!("Hello World!");                                                                                                                                           
    }
}

But I guess that would just work around #39696 in a different way.

[glaubitz]

@kennytm I have just verified that your variant works as well. Thanks for the heads-up!

[nikomatsakis]

So which version should we do? My concern with the current one is that there may be other platforms where unaligned accesses are unsafe. Maybe we could invert the #[repr(packed)] -- rather than disabling it for sparc, only enable it for the platforms we know it to be ok?

Otherwise, we could write the "correct everywhere" impls.

I'm good with either one, I think.

cc @arielb1, who may have an opinion

[glaubitz]

So which version should we do? My concern with the current one is that there may be other platforms where unaligned accesses are unsafe.

From my current knowledge, this affects sparc* only from the platforms that are supported by Rust. In fact, from the 20+ architectures, unaligned access results in a "Bus Error" on sparc64 (and the old sparc) only. Other architectures tend to use kernel helpers if the alignment needs to be fixed which can slow down the overall access.

[petrochenkov]

I'd rather add manual impls for PartialEq and Hash like b012d0d did until #44884 lands in some form (thus avoiding any platform-specific code and issues).

[nikomatsakis]

Are there many other types using #[repr(packed)]? I'm wondering (out of curiosity) if a newtype like

#[repr(packed)]
pub struct Packed<T: Copy> { pub value: T }

that then implements PartialEq and friends the right way would be valuable. Then this could just be struct Span(Packed<u32>). Probably overkill if we have a better fix en route.

[arielb1]

@nikomatsakis

Unless we want to specialize on the fields being Copy or equivalently use a MIR shim (neither of which I want), we can't get a "working" version until we finish with the warning period for #44884, so I favor the b012d0d solution until we finish the warning period.

[nikomatsakis]

@glaubitz well if @petrochenkov and @arielb1 agree, who am I to disagree. Care to port this to "the b012d0d solution"?

[glaubitz]

Care to port this to "the b012d0d solution"?

If I understand correctly, this particular change would cause the build to fail but not yet fix the actual bug, correct?

I'm also not sure whether my Rust skills are sufficient enough yet to help implement such a particular fix.

[nikomatsakis]

@glaubitz I have to admit, clicking on that commit, I am not sure what was meant either. My assumption was that it is rewriting the partial-eq and other impls to copy the fields out, such as we discussed earlier (I'd probably favor a helper type, as I described here), but perhaps @petrochenkov or @arielb1 can elaborate.

[arielb1]

My assumption was that it is rewriting the partial-eq and other impls to copy the fields out, such as we discussed earlier (I'd probably favor a helper type, as I described here)

That's the idea, either in SpanData itself or in a wrapper type.

[nikomatsakis]

@glaubitz think you will have time to work on this?

[glaubitz]

@nikomatsakis Do you mean just implementing the impl manually instead of letting the compiler generate them?

[arielb1]

@nikomatsakis

I just need to actually change my repr(packed) PR to do future compat warnings - it already has an impl

[nikomatsakis]

@glaubitz I did mean that, but maybe we should close this in favor of @arielb1's PR -- what PR is that @arielb1 ?

[glaubitz]

@nikomatsakis @arielb1 Sure. If there is anything to test on SPARC, let me know.

[arielb1]

@glaubitz

We'll let you know when my PR lands.

[nikomatsakis]

@arielb1 should this PR be closed?

[arielb1]

Sure.