enable optimizer in tests and procmacros#67
Conversation
alexpyattaev
force-pushed
the
enable_optimizer_in_tests
branch
from
8013d9a to
0a64077
Compare
alexpyattaev
force-pushed
the
enable_optimizer_in_tests
branch
from
3a7d0b7 to
7424850
Compare
joncinque
left a comment
joncinque
left a comment
There was a problem hiding this comment.
The change looks great overall, just a little point and a question
| description = "The Solana BPF program entrypoint supported by the latest BPF loader." | ||
| documentation = "https://docs.rs/solana-program-entrypoint" | ||
| version = "2.2.1" | ||
| version = "2.2.2" |
There was a problem hiding this comment.
The publish job takes care of bumping the version, so there's no need to do it here
There was a problem hiding this comment.
ok should I revert it?
| // create a pointer to the start of the arena | ||
| // that will hold an address of the byte following free space | ||
| let pos_ptr = arena.as_mut_ptr() as *mut usize; | ||
| // initialize the data there | ||
| *pos_ptr = pos_ptr as usize + arena.len(); |
There was a problem hiding this comment.
The change looks good, but for my own understanding, how does this avoid undefined behavior exactly?
Is the idea that the new call forces the optimizer to keep heap around in the tests because some part of it is written to? Would there still be UB if this code here were removed?
There was a problem hiding this comment.
fn new() correctly initializes the arena content where the allocated amount is, which was not done before, as the code assumed all arenas come zeroed. If you were to instantiate the BumpAllocator over a non-zeroed slice, it would cause UB the moment you call alloc().
The reason bug UB was triggered before this change was due to heap variable not marked as mut, so compiler assumed that it does not need to reinitialize it (since noone would write into it).
There was a problem hiding this comment.
Ah very interesting, thanks for the explanation!
revert version bump
| pub unsafe fn new(arena: &mut [u8]) -> Self { | ||
| debug_assert!( | ||
| arena.len() > size_of::<usize>(), | ||
| "Arena should be larger than usize" | ||
| ); | ||
|
|
||
| // create a pointer to the start of the arena | ||
| // that will hold an address of the byte following free space | ||
| let pos_ptr = arena.as_mut_ptr() as *mut usize; | ||
| // initialize the data there | ||
| *pos_ptr = pos_ptr as usize + arena.len(); |
There was a problem hiding this comment.
I think there is a potential alignment issue here.
arena: &mut [u8] is not guaranteed to have proper alignment for it to be cast to a pointer to a usize.
A [u8] can have any alignment, while a pointer to usize needs to have an alignment of 4 or higher.
You can use write_unaligned() to avoid the undefined behavior.
write_unaligned() example is almost literary the code you are writing here.
There was a problem hiding this comment.
Write unaligned could work, yes, but I think to properly address this, the entire BumpAllocator should be rewritten to use some form of interior mutability in the struct itself rather than storing the position pointer inline with the allocated data. It would make code a lot cleaner too. I will make a PR for that.
3 participants
Problem
Same as anza-xyz/agave#5155 but for sdk
Summary of Changes
If this fails to build there are bugs in tests=)