Skip to content

Make size_of, align_of, and element_offset functions return u64 instead of uint in trans::machine #11759

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

Merged
merged 1 commit into from
Jan 31, 2014

Conversation

nathanielherman
Copy link
Contributor

For #5172

@@ -341,7 +341,7 @@ pub fn trans_native_call<'a>(
let llalign = cmp::min(llforeign_align, llrust_align);
debug!("llrust_size={:?}", llrust_size);
base::call_memcpy(bcx, llretptr_i8, llscratch_i8,
C_uint(ccx, llrust_size), llalign as u32);
C_u64(llrust_size), llalign as u32);
Copy link
Member

Choose a reason for hiding this comment

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

This looks a little suspicious, I don't think that we want to change a target's call to memcpy, we only want to change the compiler's representation of offsets and sizes to u64 to accommodate when a 32-bit platform cross-compiles to a 64-bit platform.

@alexcrichton
Copy link
Member

cc @jld, I have some comments, but I'm unfamiliar with this code enough to know whether this is all of the places which need to get changed.

@nathanielherman
Copy link
Contributor Author

Ah so C_* represents values in the code the compiler produces (target)? That makes sense.

Though, how could I safely create a C_uint for a 64-bit platform when compiled on a 32-bit platform? Wouldn't C_uint(cx, someU64 as uint) downcast to u32 if cross-compiled on a 32-bit platform, for a 64-bit platform?

@alexcrichton
Copy link
Member

A 32-bit platform cannot create any structs or have any offsets larger than 32-bits, so downcasting should always be fine.

@nathanielherman
Copy link
Contributor Author

OK I've updated it so that there shouldn't be anymore target code using C_u64's.

@alexcrichton
Copy link
Member

This looks good to me!

Could you rebase the commits into one commit?

@nathanielherman
Copy link
Contributor Author

Yup should be good now.

@bors bors merged commit 89278f7 into rust-lang:master Jan 31, 2014
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