Skip to content

Replace manual pointer alignment with std::aligned_alloc #11203

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

Merged
merged 1 commit into from
May 29, 2025
Merged
Changes from all commits
Commits
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
66 changes: 10 additions & 56 deletions extension/data_loader/file_descriptor_data_loader.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -41,19 +41,6 @@ static bool is_power_of_2(size_t value) {
return value > 0 && (value & ~(value - 1)) == value;
}

/**
* Returns the next alignment for a given pointer.
*/
static uint8_t* align_pointer(void* ptr, size_t alignment) {
intptr_t addr = reinterpret_cast<intptr_t>(ptr);
if ((addr & (alignment - 1)) == 0) {
// Already aligned.
return reinterpret_cast<uint8_t*>(ptr);
}
// Bump forward.
addr = (addr | (alignment - 1)) + 1;
return reinterpret_cast<uint8_t*>(addr);
}
} // namespace

FileDescriptorDataLoader::~FileDescriptorDataLoader() {
Expand Down Expand Up @@ -139,13 +126,13 @@ namespace {
/**
* FreeableBuffer::FreeFn-compatible callback.
*
* `context` is actually a ptrdiff_t value (not a pointer) that contains the
* offset in bytes between `data` and the actual pointer to free.
* `context` is the original buffer pointer. It is allocated with
* ET_ALIGNED_ALLOC, and must be freed with ET_ALIGNED_FREE.
*
* `data` and `size` are unused.
*/
void FreeSegment(void* context, void* data, ET_UNUSED size_t size) {
ptrdiff_t offset = reinterpret_cast<ptrdiff_t>(context);
ET_DCHECK_MSG(offset >= 0, "Unexpected offset %ld", (long int)offset);
std::free(static_cast<uint8_t*>(data) - offset);
ET_ALIGNED_FREE(context);
}
} // namespace

Expand Down Expand Up @@ -173,57 +160,24 @@ Result<FreeableBuffer> FileDescriptorDataLoader::load(
}

// Allocate memory for the FreeableBuffer.
size_t alloc_size = size;
if (alignment_ > alignof(std::max_align_t)) {
// malloc() will align to smaller values, but we must manually align to
// larger values.
alloc_size += alignment_;
}
void* buffer = std::malloc(alloc_size);
if (buffer == nullptr) {
void* aligned_buffer = ET_ALIGNED_ALLOC(alignment_, size);
if (aligned_buffer == nullptr) {
ET_LOG(
Error,
"Reading from %s at offset %zu: malloc(%zd) failed",
"Reading from %s at offset %zu: ET_ALIGNED_ALLOC(%zd) failed",
file_descriptor_uri_,
offset,
size);
return Error::MemoryAllocationFailed;
}

// Align.
void* aligned_buffer = align_pointer(buffer, alignment_);

// Assert that the alignment didn't overflow the buffer.
ET_DCHECK_MSG(
reinterpret_cast<uintptr_t>(aligned_buffer) + size <=
reinterpret_cast<uintptr_t>(buffer) + alloc_size,
"aligned_buffer %p + size %zu > buffer %p + alloc_size %zu",
aligned_buffer,
size,
buffer,
alloc_size);

auto err = load_into(offset, size, segment_info, aligned_buffer);
if (err != Error::Ok) {
// Free `buffer`, which is what malloc() gave us, not `aligned_buffer`.
std::free(buffer);
ET_ALIGNED_FREE(aligned_buffer);
return err;
}

// We can't naively free this pointer, since it may not be what malloc() gave
// us. Pass the offset to the real buffer as context. This is the number of
// bytes that need to be subtracted from the FreeableBuffer::data() pointer to
// find the actual pointer to free.
return FreeableBuffer(
aligned_buffer,
size,
FreeSegment,
/*free_fn_context=*/
reinterpret_cast<void*>(
// Using signed types here because it will produce a signed ptrdiff_t
// value, though for us it will always be non-negative.
reinterpret_cast<intptr_t>(aligned_buffer) -
reinterpret_cast<intptr_t>(buffer)));
return FreeableBuffer(aligned_buffer, size, FreeSegment, aligned_buffer);
}

Result<size_t> FileDescriptorDataLoader::size() const {
Expand Down
Loading