Skip to content

Make sure that the future fragment's storage pointer is properly aligned #39821

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

Make sure that the future fragment's storage pointer is properly aligned #39821

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
20 changes: 9 additions & 11 deletions include/swift/ABI/Task.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -482,25 +482,23 @@ class AsyncTask : public Job {

/// Retrieve a pointer to the storage of result.
OpaqueValue *getStoragePtr() {
auto *startAddr = reinterpret_cast<char *>(this) + sizeof(FutureFragment);
uintptr_t startAddrVal = (uintptr_t)startAddr;
uintptr_t alignment = resultType->vw_alignment();
startAddrVal = (startAddrVal + alignment -1) & ~(alignment -1);
return reinterpret_cast<OpaqueValue *>(
reinterpret_cast<char *>(this) + storageOffset(resultType));
reinterpret_cast<char *>(startAddrVal));
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm. Normally this would be problematic because of the potential mismatch between an abstract offset calculation and an adjustment on an actual pointer. I guess this particular example might be fine because the overall allocation is known to be aligned at least as well as alignment, so in (mod K) arithmetic we get the same result? Is that true? If so, it's at least worth a comment.

Copy link
Contributor Author

@aschwaighofer aschwaighofer Oct 20, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

That is a good question. I had assume when writing this that we would be just calling swift_alloc which would have been ‘max alignment’ aligned. Reading this code: https://github.com/apple/swift/blob/1ab88ed090aadaceb295c22abd503b5d85017fd0/stdlib/public/Concurrency/Task.cpp#L534 I am not sure. There is a call to plain malloc which on Linux I don’t know what it returns. And there is that pointer that we get from some preallocated buffer.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, if we're allocating tasks with just malloc, we might need to adjust that. We can do that in a separate PR, though.

}

/// Retrieve the error.
SwiftError *&getError() { return error; }

/// Compute the offset of the storage from the base of the future
/// fragment.
static size_t storageOffset(const Metadata *resultType) {
size_t offset = sizeof(FutureFragment);
size_t alignment = resultType->vw_alignment();
return (offset + alignment - 1) & ~(alignment - 1);
}

/// Determine the size of the future fragment given a particular future
/// result type.
static size_t fragmentSize(const Metadata *resultType) {
return storageOffset(resultType) + resultType->vw_size();
static size_t fragmentSize(size_t initialOffset, const Metadata *resultType) {
size_t alignment = resultType->vw_alignment();
size_t padding = alignment - ((sizeof(FutureFragment) + initialOffset) % alignment);
return sizeof(FutureFragment) + padding + resultType->vw_size();
}
};

Expand Down
2 changes: 1 addition & 1 deletion stdlib/public/Concurrency/Task.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -513,7 +513,7 @@ static AsyncTaskAndContext swift_task_create_commonImpl(
headerSize += sizeof(AsyncTask::GroupChildFragment);
}
if (futureResultType) {
headerSize += FutureFragment::fragmentSize(futureResultType);
headerSize += FutureFragment::fragmentSize(headerSize, futureResultType);
// Add the future async context prefix.
headerSize += sizeof(FutureAsyncContextPrefix);
} else {
Expand Down