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.

Sign up for GitHub

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

read_target_uint => read_target_ptr outside rustc_middle (nfc) #84826

Closed
wants to merge 3 commits into from
Closed

read_target_uint => read_target_ptr outside rustc_middle (nfc) #84826

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e00196d
Lock down write_target_uint
workingjubilee Apr 25, 2021
c04c0b9
read_target_uint => read_target_ptr (nfc)
workingjubilee May 2, 2021
c7d4c8b
Update compiler/rustc_codegen_cranelift/src/intrinsics/simd.rs
workingjubilee May 2, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions compiler/rustc_codegen_cranelift/src/constant.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ use rustc_data_structures::fx::FxHashSet;
use rustc_errors::ErrorReported;
use rustc_middle::middle::codegen_fn_attrs::CodegenFnAttrFlags;
use rustc_middle::mir::interpret::{
read_target_uint, AllocId, Allocation, ConstValue, ErrorHandled, GlobalAlloc, Pointer, Scalar,
read_target_ptr, AllocId, Allocation, ConstValue, ErrorHandled, GlobalAlloc, Pointer, Scalar,
};
use rustc_middle::ty::ConstKind;

Expand Down Expand Up @@ -379,11 +379,11 @@ fn define_all_allocs(tcx: TyCtxt<'_>, module: &mut dyn Module, cx: &mut Constant
let addend = {
let endianness = tcx.data_layout.endian;
let offset = offset.bytes() as usize;
let ptr_size = tcx.data_layout.pointer_size;
let ptr_size = tcx.data_layout.pointer_size.bytes() as usize;
let bytes = &alloc.inspect_with_uninit_and_ptr_outside_interpreter(
offset..offset + ptr_size.bytes() as usize,
offset..offset + ptr_size
);
read_target_uint(endianness, bytes).unwrap()
read_target_ptr(endianness, ptr_size, bytes)
Copy link
Member

Choose a reason for hiding this comment

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

This name makes it seem like it reads the whole pointer instead of just the addend, leaving the target to which the addend is relative out.

Copy link
Member Author

Choose a reason for hiding this comment

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

read_target_usize perhaps?

Copy link
Member

Choose a reason for hiding this comment

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

Maybe

};

let reloc_target_alloc = tcx.get_global_alloc(reloc).unwrap();
Expand Down
11 changes: 7 additions & 4 deletions compiler/rustc_codegen_cranelift/src/intrinsics/simd.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,8 @@ pub(super) fn codegen_simd_intrinsic_call<'tcx>(

let indexes = {
use rustc_middle::mir::interpret::*;
use rustc_target::abi::Endian;

let idx_const = crate::constant::mir_operand_get_const_val(fx, idx).expect("simd_shuffle* idx not const");

let idx_bytes = match idx_const {
Expand All @@ -96,10 +98,11 @@ pub(super) fn codegen_simd_intrinsic_call<'tcx>(

(0..ret_lane_count).map(|i| {
let i = usize::try_from(i).unwrap();
let idx = rustc_middle::mir::interpret::read_target_uint(
fx.tcx.data_layout.endian,
&idx_bytes[4*i.. 4*i + 4],
).expect("read_target_uint");
let idx = match (fx.tcx.data_layout.endian, &idx_bytes[4*i.. 4*i + 4]) {
(Endian::Little, &[a, b, c, d]) => u32::from_le_bytes([a, b, c, d]),
(Endian::Big, &[a, b, c, d]) => u32::from_be_bytes([a, b, c, d]),
(_, _) => unreachable!(),
};
Copy link
Contributor

Choose a reason for hiding this comment

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

This has become significantly more complex when the complexity was hidden behind a function before. Why was this change made?

u16::try_from(idx).expect("try_from u32")
}).collect::<Vec<u16>>()
};
Expand Down
14 changes: 7 additions & 7 deletions compiler/rustc_codegen_llvm/src/consts.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ use rustc_codegen_ssa::traits::*;
use rustc_hir::def_id::DefId;
use rustc_middle::middle::codegen_fn_attrs::{CodegenFnAttrFlags, CodegenFnAttrs};
use rustc_middle::mir::interpret::{
read_target_uint, Allocation, ErrorHandled, GlobalAlloc, Pointer,
read_target_ptr, Allocation, ErrorHandled, GlobalAlloc, Pointer,
};
use rustc_middle::mir::mono::MonoItem;
use rustc_middle::ty::{self, Instance, Ty};
Expand All @@ -22,6 +22,7 @@ use tracing::debug;
pub fn const_alloc_to_llvm(cx: &CodegenCx<'ll, '_>, alloc: &Allocation) -> &'ll Value {
let mut llvals = Vec::with_capacity(alloc.relocations().len() + 1);
let dl = cx.data_layout();
let endian = dl.endian;
let pointer_size = dl.pointer_size.bytes() as usize;

let mut next_offset = 0;
Expand All @@ -39,16 +40,15 @@ pub fn const_alloc_to_llvm(cx: &CodegenCx<'ll, '_>, alloc: &Allocation) -> &'ll
let bytes = alloc.inspect_with_uninit_and_ptr_outside_interpreter(next_offset..offset);
llvals.push(cx.const_bytes(bytes));
}
let ptr_offset = read_target_uint(
dl.endian,

let ptr_offset = read_target_ptr(
endian,
pointer_size,
// This `inspect` is okay since it is within the bounds of the allocation, it doesn't
// affect interpreter execution (we inspect the result after interpreter execution),
// and we properly interpret the relocation as a relocation pointer offset.
alloc.inspect_with_uninit_and_ptr_outside_interpreter(offset..(offset + pointer_size)),
)
.expect("const_alloc_to_llvm: could not read relocation pointer")
as u64;

);
let address_space = match cx.tcx.global_alloc(alloc_id) {
GlobalAlloc::Function(..) => cx.data_layout().instruction_address_space,
GlobalAlloc::Static(..) | GlobalAlloc::Memory(..) => AddressSpace::DATA,
Expand Down
23 changes: 20 additions & 3 deletions compiler/rustc_middle/src/mir/interpret/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -555,12 +555,12 @@ impl<'tcx> TyCtxt<'tcx> {
////////////////////////////////////////////////////////////////////////////////

#[inline]
pub fn write_target_uint(
fn write_target_uint(
endianness: Endian,
mut target: &mut [u8],
data: u128,
) -> Result<(), io::Error> {
// This u128 holds an "any-size uint" (since smaller uints can fits in it)
// This u128 holds an "any-size uint" (since smaller uints can fit in it)
// So we do not write all bytes of the u128, just the "payload".
match endianness {
Endian::Little => target.write(&data.to_le_bytes())?,
Expand All @@ -571,7 +571,7 @@ pub fn write_target_uint(
}

#[inline]
pub fn read_target_uint(endianness: Endian, mut source: &[u8]) -> Result<u128, io::Error> {
fn read_target_uint(endianness: Endian, mut source: &[u8]) -> Result<u128, io::Error> {
// This u128 holds an "any-size uint" (since smaller uints can fits in it)
let mut buf = [0u8; std::mem::size_of::<u128>()];
// So we do not read exactly 16 bytes into the u128, just the "payload".
Expand All @@ -588,3 +588,20 @@ pub fn read_target_uint(endianness: Endian, mut source: &[u8]) -> Result<u128, i
debug_assert!(source.len() == 0); // We should have consumed the source buffer.
uint
}

/// Read a pointer-sized thing from a target, so it will fit in a u64
/// as the modules using this don't support wider pointer values
/// FIXME(jubilee): Move this out of here and closer to the things using it!
#[inline]
pub fn read_target_ptr(endian: Endian, ptr_size: usize, bytes: &[u8]) -> u64 {
Copy link
Member

Choose a reason for hiding this comment

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

This should probably take Size instead of usize.

Copy link
Member Author

@workingjubilee workingjubilee May 2, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Size is a u64, so on a 32-bit host that is over-weighting this input (even tho' the output should be u64 regardless). Might not matter after inlining, I guess.

I thought about adding an enum to rustc_target so that can be matched on directly for more type-strictness, though.

Copy link
Member

Choose a reason for hiding this comment

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

It's just standard to use Size to describe such a property of the target, and the consistency helps avoid bugs. Also 32-bit host performance shouldn't be a significant concern (even tho memory usage in data structures may be).

Copy link
Member Author

Choose a reason for hiding this comment

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

@workingjubilee, this code doesn't seem terribly idiomatic, and if your concern is input overhead, why not just call bytes.len()? Besides, it's definitely going to be optimized away during inlining, and if you call bytes.len() then it's easier for the optimizer to see what the variable means.

Copy link
Member Author

Choose a reason for hiding this comment

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

@workingjubilee Oh, you're probably right! Even taking an input seems a bit silly, now that I think of it. I must be reading too many C codebases lately. 😅

use core::convert::TryInto;
match (ptr_size, endian) {
(2, Endian::Little) => u16::from_le_bytes(bytes.try_into().unwrap()) as u64,
(2, Endian::Big) => u16::from_be_bytes(bytes.try_into().unwrap()) as u64,
(4, Endian::Little) => u32::from_le_bytes(bytes.try_into().unwrap()) as u64,
(4, Endian::Big) => u32::from_be_bytes(bytes.try_into().unwrap()) as u64,
(8, Endian::Little) => u64::from_le_bytes(bytes.try_into().unwrap()),
(8, Endian::Big) => u64::from_be_bytes(bytes.try_into().unwrap()),
Comment on lines +599 to +604
Copy link
Member

Choose a reason for hiding this comment

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

I don't like explicitly handling several possibilities using host integers, when it can be done by e.g. copying bytes into an [0u8; 8], reversing the ..bytes.len() slice of the array if Endian::Big, and then using u64::fom_le_bytes.

@oli does miri already have some uniform (over integer sizes) code for doing this?

Actually, why not just keep using read_target_uint? It seems like the correct abstraction, am I missing something? If you don't want it to part of the rustc_mir::mir::interpret interface, we just have to move it around a bit, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

does miri already have some uniform (over integer sizes) code for doing this?

Yes, as you noted, read_target_uint is that abstraction. I do see the point in an abstraction returning u64 for usize purposes, but imo that should just be a wrapper around the existing functions.

As to where to place them... if rustc_middle doesn't need them they can go to rustc_mir, otherwise idk

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, I definitely was overthinking this, in retrospect. Anyways, my main thought was that it should probably exist in, say, rustc_target or something like that.

(_, _) => panic!("unknown pointer size and endianness combination"),
}
}
8 changes: 4 additions & 4 deletions compiler/rustc_mir/src/util/pretty.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ use rustc_data_structures::fx::FxHashMap;
use rustc_hir::def_id::{DefId, LOCAL_CRATE};
use rustc_index::vec::Idx;
use rustc_middle::mir::interpret::{
read_target_uint, AllocId, Allocation, ConstValue, GlobalAlloc, Pointer,
read_target_ptr, AllocId, Allocation, ConstValue, GlobalAlloc, Pointer,
};
use rustc_middle::mir::visit::Visitor;
use rustc_middle::mir::*;
Expand Down Expand Up @@ -833,6 +833,7 @@ fn write_allocation_bytes<Tag: Copy + Debug, Extra>(
let mut line_start = Size::ZERO;

let ptr_size = tcx.data_layout.pointer_size;
let ptr_bytesize = ptr_size.bytes_usize();

let mut ascii = String::new();

Expand All @@ -852,9 +853,8 @@ fn write_allocation_bytes<Tag: Copy + Debug, Extra>(
if let Some(&(tag, target_id)) = alloc.relocations().get(&i) {
// Memory with a relocation must be defined
let j = i.bytes_usize();
let offset = alloc
.inspect_with_uninit_and_ptr_outside_interpreter(j..j + ptr_size.bytes_usize());
let offset = read_target_uint(tcx.data_layout.endian, offset).unwrap();
let offset = alloc.inspect_with_uninit_and_ptr_outside_interpreter(j..j + ptr_bytesize);
let offset = read_target_ptr(tcx.data_layout.endian, ptr_bytesize, offset);
let offset = Size::from_bytes(offset);
let relocation_width = |bytes| bytes * 3;
let ptr = Pointer::new_with_tag(target_id, offset, tag);
Expand Down