Add #[repr(C)] for more future-proof byte mangling #33085
Conversation
|
@t-nelson once this pr lands and #32961 is rebased, this code change should go away: #32961 (comment) |
ryoqun
force-pushed
the
hashing-test-repr-c
branch
from
29ddc37 to
99ec8d5
Compare
ryoqun
changed the title
| // repr(C) is needed for abi-stability in the dirtiest variant of std::mem::transmute(). | ||
| #[repr(C)] | ||
| struct InputFields( |
There was a problem hiding this comment.
I'm a bit surprised that plain tuple and tuple struct's abi differ; but i guess this is a life. ;)
There was a problem hiding this comment.
In case you were curious about the the layout change for this specific tuple, like I was last night, I logged the first byte of each tuple field in 1.69.0 and 1.71.0+:
slot_int, meta_int, account_meta_int, data_int, offset_int, hash_int
1.69.0: 112 160 96 40 104 37
1.71.0: 104 96 160 40 48 37
Note the underlying data array bytes are in reverse order: [160..1].
So in memory the layout order is:
1.69.0: [meta, slot, offset, account_meta, data, hash]
1.71.0: [account_meta, slot, meta, offset, data, hash]
both still take 160 bytes, but just have a different order 🤷♂️
| let expected_account_hash = if cfg!(debug_assertions) { | ||
| Hash::from_str("6qtBXmRrLdTdAV5bK6bZZJxQA4fPSUBxzQGq2BQSat25").unwrap() | ||
| Hash::from_str("8GiQSN2VvWASKPUuZgFkH4v66ihEanrDVXAkMFvLwEa8").unwrap() | ||
| } else { | ||
| Hash::from_str("5HL9MtsQmxZQ8XSgcAhSkqnrayQFXUY8FT1JsHjDNKbi").unwrap() | ||
| Hash::from_str("9MYASra3mm8oXzMapYUonB6TcRsKFPtjhNXVgY3MPPUX").unwrap() | ||
| }; |
There was a problem hiding this comment.
Why are the hashes different for debug and release builds?
Doesn't it indicate there is still some unresolved instability?
There was a problem hiding this comment.
good point. I'm sure some rustc optimization is causing the difference.
it indicates some unresolved UB but i think this is tolerable and stable as long as this shouldn't change that often with repr(C).
There was a problem hiding this comment.
The issue is indeed that this test assigns a value that is not 0 or 1 into the bool fields, which is an undefined behavior.
Here is a change that makes the hashes identical: #33125
I still think that it is better to remove unsafe and let the compiler make sure that these tricky issues do not happen: #33083
Problem
the setup part of the test introduced at #7415 (comment) is fragile against rustc's abi-related changes.
For example, hashing change is needed due to updating rustc to 1.72: #32961 (comment)
Summary of Changes
Use
repr(C)to make test byte pattern generation more resilient to any abi changes.i confirmed locally the updated hash in this pr is emitted now with both rust 1.69 and 1.72.
also some comment clarification is done for future me and others. :)