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

[stdlib] Work around the materialization bug present in #2825 #2826

Open
wants to merge 91 commits into
base: nightly
Choose a base branch
from

Conversation

gabrieldemarmiesse
Copy link
Contributor

@gabrieldemarmiesse gabrieldemarmiesse commented May 25, 2024

This PR solves part of #2467

This PR is part of three PRs to read and merge in the following order

The important diff is the one between this branch and the branch of the PR #2825 .

You can take a look at this diff here: gabrieldemarmiesse/mojo@sbo_take_2...gabrieldemarmiesse:mojo:fix_materializing
As you can see it's fairly small.

The bug

To understand the workaround, one must understand the bug.
This PR is here to work around the issue #2637
Long story short, when materializing a struct, if the struct holds a pointer to one of its elements, the compiler is a bit lost and will call __del__ on a struct with the pointer value not updated. (I think it moves the data but doesn't change the pointer value accordingly).
This causes us to call free() on a pointer that points to stack allocated data, because we check if SBO is in use by using self.data == self._small_buffer.unsafe_ptr(). Thus crashing the program. And it's really common to materilize things, especially with Strings that have default values in signatures.

The workaround

We don't trust the compiler to copy pointers to some elements of self, but we trust the compiler to copy correctly a bool. As such, we add a flag to know if sbo is in use or not, and we use it. This flag can be removed when the compiler bug is fixed.

Consequences on the default List

Because of this flag, List has one more byte. I wanted to add this flag only when using a small_buffer_size > 0 (you pay for what you use and all that), so I wanted to store this flag inside an InlineArray[Bool, Int(Self.sbo_enabled)], so that it doesn't take space when small_buffer_size = 0. At first, this worked. Then another compiler bug was introduced the 25/05/2024 which @JoeLoser managed to biscect down to some given commit. (feel free to link it here @JoeLoser ). Thus the InlineArray wasn't an option anymore and a simple permanant Bool must be used. Users will see List take one more byte until one of those two bugs are fixed.

Storing the flag inside the capacity or size at the most significant bit

This is an option to save one byte. But since the workaround is supposed to be temporary, I chose to avoid adding complexity (and possibly bugs) to the codebase for something like that. Furthermore, it doesn't impact performance much (see the next benchmarks), it only increases the size of the List.

Signed-off-by: gabrieldemarmiesse <gabrieldemarmiesse@gmail.com>
Signed-off-by: gabrieldemarmiesse <gabrieldemarmiesse@gmail.com>
@gabrieldemarmiesse gabrieldemarmiesse marked this pull request as ready for review May 25, 2024 22:41
@gabrieldemarmiesse gabrieldemarmiesse requested a review from a team as a code owner May 25, 2024 22:41
Signed-off-by: gabrieldemarmiesse <gabrieldemarmiesse@gmail.com>
Signed-off-by: gabrieldemarmiesse <gabrieldemarmiesse@gmail.com>
@JoeLoser
Copy link
Collaborator

!sync

@modularbot modularbot added the imported-internally Signals that a given pull request has been imported internally. label May 26, 2024
Signed-off-by: gabrieldemarmiesse <gabrieldemarmiesse@gmail.com>
Signed-off-by: gabrieldemarmiesse <gabrieldemarmiesse@gmail.com>
Signed-off-by: gabrieldemarmiesse <gabrieldemarmiesse@gmail.com>
Signed-off-by: gabrieldemarmiesse <gabrieldemarmiesse@gmail.com>
Signed-off-by: gabrieldemarmiesse <gabrieldemarmiesse@gmail.com>
Signed-off-by: gabrieldemarmiesse <gabrieldemarmiesse@gmail.com>
Signed-off-by: gabrieldemarmiesse <gabrieldemarmiesse@gmail.com>
Signed-off-by: gabrieldemarmiesse <gabrieldemarmiesse@gmail.com>
Signed-off-by: gabrieldemarmiesse <gabrieldemarmiesse@gmail.com>
Signed-off-by: gabrieldemarmiesse <gabrieldemarmiesse@gmail.com>
Signed-off-by: gabrieldemarmiesse <gabrieldemarmiesse@gmail.com>
…nstructor

Signed-off-by: gabrieldemarmiesse <gabrieldemarmiesse@gmail.com>
Signed-off-by: gabrieldemarmiesse <gabrieldemarmiesse@gmail.com>
Signed-off-by: gabrieldemarmiesse <gabrieldemarmiesse@gmail.com>
Signed-off-by: gabrieldemarmiesse <gabrieldemarmiesse@gmail.com>
Signed-off-by: gabrieldemarmiesse <gabrieldemarmiesse@gmail.com>
Signed-off-by: gabrieldemarmiesse <gabrieldemarmiesse@gmail.com>
Signed-off-by: gabrieldemarmiesse <gabrieldemarmiesse@gmail.com>
Signed-off-by: gabrieldemarmiesse <gabrieldemarmiesse@gmail.com>
Signed-off-by: gabrieldemarmiesse <gabrieldemarmiesse@gmail.com>
Signed-off-by: gabrieldemarmiesse <gabrieldemarmiesse@gmail.com>
Signed-off-by: gabrieldemarmiesse <gabrieldemarmiesse@gmail.com>
Signed-off-by: gabrieldemarmiesse <gabrieldemarmiesse@gmail.com>
Signed-off-by: gabrieldemarmiesse <gabrieldemarmiesse@gmail.com>
Signed-off-by: gabrieldemarmiesse <gabrieldemarmiesse@gmail.com>
… remove_copyable_from_collectionelement

Signed-off-by: gabrieldemarmiesse <gabrieldemarmiesse@gmail.com>
Signed-off-by: gabrieldemarmiesse <gabrieldemarmiesse@gmail.com>
…nstructor

Signed-off-by: gabrieldemarmiesse <gabrieldemarmiesse@gmail.com>
…_branch

Signed-off-by: gabrieldemarmiesse <gabrieldemarmiesse@gmail.com>
Signed-off-by: gabrieldemarmiesse <gabrieldemarmiesse@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
imported-internally Signals that a given pull request has been imported internally.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants