-
Notifications
You must be signed in to change notification settings - Fork 340
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
implement packed struct field access #108
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, assuming that disadvantage you mentioned doesn't come up.
I agree this has a smaller impact than adding flags to Lvalue
and Pointer
, even if that would be a more "direct" representation of what we're doing.
} | ||
|
||
#[derive(Eq, PartialEq, Ord, PartialOrd)] | ||
struct Entry(AllocId, u64, u64); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use named fields to make it clear what these u64
fields are. Can you please also add a comment describing why the derived Ord implementation is correct here?
let alloc = self.get(ptr.alloc_id)?; | ||
// check whether the memory was marked as aligned |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/aligned/packed/?
// check whether the memory was marked as aligned | ||
let start = Entry(ptr.alloc_id, 0, ptr.offset + len); | ||
let end = Entry(ptr.alloc_id, ptr.offset + len, 0); | ||
for &Entry(_, start, end) in self.packed.range(start..end) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: I would avoid shadowing start
and end
here.
@@ -120,6 +120,8 @@ pub struct Memory<'a, 'tcx> { | |||
function_alloc_cache: HashMap<FunctionDefinition<'tcx>, AllocId>, | |||
next_id: AllocId, | |||
pub layout: &'a TargetDataLayout, | |||
/// List of memory regions containing packed structures | |||
packed: BTreeSet<Entry>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you document this PR's strategy in some comments on this field?
@@ -28,6 +28,7 @@ impl<'a, 'tcx> EvalContext<'a, 'tcx> { | |||
|
|||
/// Returns true as long as there are more things to do. | |||
pub fn step(&mut self) -> EvalResult<'tcx, bool> { | |||
self.memory.clear_packed(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Either here or on the definition of clear_packed
it'd be nice to have a comment saying "see the docs on the packed
field for why we do this".
b: 99, | ||
}; | ||
assert_eq!(x.a, 42); | ||
assert_eq!(x.b, 99); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test seems good, but can you add a compile-fail
test that instead takes let p = &x.b; *p;
to make sure it fails the alignment check on deref?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nope, because rustc will forbid us taking references to fields of packed structs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, cool.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh... I thought that was fixed... apparently it is not: rust-lang/rust#27060
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good catch. Miri didn't check alignment in various cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
just for the record: Rustc LLVM now traces alignment down to the memory accesses, it's quite intrusive: https://github.com/rust-lang/rust/pull/39586/files |
This is a little roundabout. We mark memory as "packed" or "unaligned" for a single statement, and clear the marking afterwards. This has the advantage over the below mentioned solution, that in the case where no packed structs are present, it's just a single emptyness check of a set instead of heavily influencing all memory access code.
Not sure how to do this better.
We could add some flag to the
Lvalue
type that is set when we encounter some packed struct. But for reading from a packed struct, I'm not sure how to do that properly. Also that would require us to modify all the code related to writing and reading from memory, modifying the alignment checks...One disadvantage of this solution is the fact that you can cast a pointer to a packed struct to a pointer to a normal struct and if you access a field of both in the same MIR statement, the normal struct access will succeed even though it shouldn't. But even with mir optimizations, I'm not sure if that situation is even producible.