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.

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

Align the start pointer in ink allocator #2100

Merged
merged 13 commits into from
Feb 24, 2024
1 change: 1 addition & 0 deletions .config/cargo_spellcheck.dic
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ Wasm32
WebAssembly
adjunctive
bitvector
bitmask
bitwise
callee
codegen
Expand Down
3 changes: 3 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

## [Unreleased]

### Fixed
- Fix alignment in allocator [#2100](https://github.com/paritytech/ink/pull/2100)

## Version 5.0.0-rc.1

### Added
Expand Down
37 changes: 32 additions & 5 deletions crates/allocator/src/bump.rs
Original file line number Diff line number Diff line change
Expand Up @@ -174,9 +174,10 @@ impl InnerAlloc {
/// Note: This implementation results in internal fragmentation when allocating across
/// pages.
fn alloc(&mut self, layout: Layout) -> Option<usize> {
let alloc_start = self.next;
let alloc_start = self.align_ptr(&layout);

let aligned_size = layout.size();

let aligned_size = layout.pad_to_align().size();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just out of curiosity, how is padding performed for fields in a struct, considering they field types have their own alignment requirements?

Copy link
Contributor

Choose a reason for hiding this comment

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

That is not really the concern of the allocator. The allocator will just create an allocation that adheres to the Layout passed. For a struct the alignment will be set to the alignment for the whole struct (AFAIK the alignment of the largest type in the struct). The padding within the struct is given implicitly by the size of the Layout. Rust will just assume the padding when accessing the struct.

let alloc_end = alloc_start.checked_add(aligned_size)?;

if alloc_end > self.upper_limit {
Expand All @@ -194,6 +195,20 @@ impl InnerAlloc {
Some(alloc_start)
}
}

/// Aligns the start pointer of the next allocation.
///
/// We inductively calculate the start index
/// of a layout in the linear memory.
/// - Initially `self.next` is `0`` and aligned
/// - `layout.align() - 1` accounts for `0` as the first index.
/// - the binary with the inverse of the align creates a
/// bitmask that is used to zero out bits, ensuring alignment according to type
/// requirements and ensures that the next allocated pointer address is of the
/// power of 2.
fn align_ptr(&self, layout: &Layout) -> usize {
(self.next + layout.align() - 1) & !(layout.align() - 1)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please add a comment with the reasoning behind the calculation and the AND here.

Copy link
Contributor

Choose a reason for hiding this comment

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

maybe some concrete example will help here

}
}

/// Calculates the number of pages of memory needed for an allocation of `size` bytes.
Expand Down Expand Up @@ -329,6 +344,18 @@ mod tests {
let expected_alloc_start = 2 * PAGE_SIZE + size_of::<u8>();
assert_eq!(inner.next, expected_alloc_start);
}

#[test]
fn correct_alloc_types() {
let mut inner = InnerAlloc::new();
let layout1 = Layout::for_value(&Vec::<u128>::with_capacity(3));
assert_eq!(inner.alloc(layout1), Some(0));
assert_eq!(inner.next, 24);

let layout2 = Layout::for_value(&Vec::<u128>::with_capacity(1));
assert_eq!(inner.alloc(layout2), Some(24));
assert_eq!(inner.next, 48);
}
}

#[cfg(all(test, feature = "ink-fuzz-tests"))]
Expand All @@ -351,7 +378,7 @@ mod fuzz_tests {
Err(_) => return TestResult::discard(),
};

let size = layout.pad_to_align().size();
let size = layout.size();
assert_eq!(
inner.alloc(layout),
Some(0),
Expand Down Expand Up @@ -390,7 +417,7 @@ mod fuzz_tests {
Err(_) => return TestResult::discard(),
};

let size = layout.pad_to_align().size();
let size = layout.size();
assert_eq!(
inner.alloc(layout),
Some(0),
Expand Down Expand Up @@ -459,7 +486,7 @@ mod fuzz_tests {
Err(_) => return TestResult::discard(),
};

let size = layout.pad_to_align().size();
let size = layout.size();

let current_page_limit = PAGE_SIZE * required_pages(inner.next).unwrap();
let is_too_big_for_current_page = inner.next + size > current_page_limit;
Expand Down
40 changes: 40 additions & 0 deletions integration-tests/static-buffer/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@

#[ink::contract]
pub mod static_buffer {
use ink::prelude::vec::Vec;

#[allow(unused_imports)]
use ink::env::BUFFER_SIZE;
#[ink(storage)]
Expand All @@ -28,6 +30,15 @@ pub mod static_buffer {
pub fn get_caller(&self) -> AccountId {
self.env().caller()
}

#[ink(message)]
pub fn buffer(&self) {
let _buf1 = Vec::<u8>::with_capacity(3);
let _buf2 = Vec::<u64>::with_capacity(1);
ink::env::debug_println!("{:?}", _buf1.as_ptr());
ink::env::debug_println!("{:?}", _buf2.as_ptr());
ink::env::debug_println!("{}", core::mem::align_of::<Vec<bool>>());
}
}

#[cfg(test)]
Expand Down Expand Up @@ -75,5 +86,34 @@ pub mod static_buffer {

Ok(())
}

#[ink_e2e::test]
async fn buffer<Client: E2EBackend>(mut client: Client) -> E2EResult<()> {
// given
let mut constructor = StaticBufferRef::new_default();

// when
let contract = client
.instantiate("static_buffer", &ink_e2e::bob(), &mut constructor)
.submit()
.await
.expect("instantiate failed");
let call_builder = contract.call_builder::<StaticBuffer>();

// then
let get = call_builder.buffer();
let get_res = client.call(&ink_e2e::bob(), &get).submit().await?;

let debug_msg = get_res.debug_message();
let msgs: Vec<&str> = debug_msg.split('\n').collect();
let ptr1 = u64::from_str_radix(msgs[0].trim_start_matches("0x"), 16).unwrap();
let ptr2 = u64::from_str_radix(msgs[1].trim_start_matches("0x"), 16).unwrap();
let align = u64::from_str_radix(msgs[2], 10).unwrap();

Copy link
Contributor

Choose a reason for hiding this comment

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

This seems a bit hacky to rely on the debug_message here.
Can't we just do that in the ink! message itself and assert it does not panic, or return the values and assert here?

assert_eq!(align, 4);
assert_eq!((ptr2 - ptr1), 8);

Ok(())
}
}
}
Loading