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

Fix load alignment being applied to referenced value #551

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

sapir
Copy link
Contributor

@sapir sapir commented Aug 5, 2024

In some code I compiled, the align argument (edit: in Builder::load) wasn't the same as the pointer's regular alignment, and gcc ended up dereferencing the wrong address. So I think the align argument is meant to only apply to the loaded value and not to the one that's being read.

For comparison, rustc_codegen_llvm does the following:

let load = llvm::LLVMBuildLoad2(self.llbuilder, ty, ptr, UNNAMED);
llvm::LLVMSetAlignment(load, align.bytes() as c_uint);

@antoyo
Copy link
Contributor

antoyo commented Aug 5, 2024

llvm::LLVMSetAlignment(load, align.bytes() as c_uint);

The load alignment is somewhat confusing.
I don't remember for sure, but I believe the LLVMSetAlignment function applies to the load instruction and not the location where the value is loaded.
Since your fix makes some test fails, I would tend to think that the code is correct as is, but I'm might be wrong.

In some code I compiled, the align argument (edit: in Builder::load) wasn't the same as the pointer's regular alignment, and gcc ended up dereferencing the wrong address.

Could you please share an example where this happens?

@sapir
Copy link
Contributor Author

sapir commented Aug 5, 2024

I'll try to make an example tomorrow.

@sapir
Copy link
Contributor Author

sapir commented Aug 7, 2024

ok I think I've got it now. Also, I have some code to reproduce the problem.

// repr(C) to prevent any field reordering.
#[repr(C)]
struct X {
    // This causes the struct to require 8 byte alignment.
    a: u64,
    // This field is aligned to 8 bytes because the struct is aligned to 8 bytes, and it's at offset 8.
    // The type itself only requires 4 bytes, though.
    b: u32,
    c: u32,
    d: u32,
}

#[inline(never)]
fn foo(b1: u32, b2: u32) {
    dbg!(b1);
    dbg!(b2);
}

#[inline(never)]
fn bar(x: &X) {
    // x.b loads the value from b with 8-byte alignment.

    // The load result also has 8-byte alignment, so when being passed to foo,
    // the second argument gets aligned to 8-bytes and is passed at argument offset 8
    // as if it were the third 4-byte argument.

    foo(x.b, x.b);
}

fn main() {
    let x = X {
        a: 1,
        b: 2,
        c: 3,
        d: 4,
    };
    bar(&x);
}

So I think maybe the alignment shouldn't be applied to the load result.

@antoyo
Copy link
Contributor

antoyo commented Aug 7, 2024

Can you explain what's the expected result with this code?

I get:

[src/main.rs:15:5] b1 = 2
[src/main.rs:16:5] b2 = 2

which seems good.

@sapir
Copy link
Contributor Author

sapir commented Aug 7, 2024

I'm getting 2 on the first line and some random value on the second.

@antoyo
Copy link
Contributor

antoyo commented Aug 7, 2024

Which architecture are you on?

@sapir
Copy link
Contributor Author

sapir commented Aug 7, 2024

I was trying mips-unknown-linux-gnu. Since (edit: after) you asked, I also tried some other architectures including m68k but so far it hasn't happened anywhere except the mips one.

Also, it only happens with --release (I mean for cargo, e.g. y.sh cargo build --release). Without the --release, the argument gets passed fine.

@antoyo
Copy link
Contributor

antoyo commented Aug 9, 2024

Do you know if it would be easy to add a test for this fix?

@sapir
Copy link
Contributor Author

sapir commented Aug 21, 2024

I think a test for the behavior might not be perfectly reliable because the bug could accidentally be optimized out, but I'll try. I think maybe with a higher field alignment it'll happen on m68k, too.

@antoyo
Copy link
Contributor

antoyo commented Oct 8, 2024

Did you want to add a test or would you want to merge this as is?

@sapir
Copy link
Contributor Author

sapir commented Oct 13, 2024

I'd like to try to add a test that works on the architectures currently being tested in CI.

@antoyo
Copy link
Contributor

antoyo commented Oct 13, 2024

Ok, please tell me when it's ready!
Thanks a lot for your work!

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.

2 participants