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

Fix segfault in the Breadcrumbs system #180

Merged
merged 3 commits into from
Jun 5, 2020

Conversation

azeey
Copy link
Contributor

@azeey azeey commented Jun 4, 2020

The segfault occurred because a pointer to a component is invalidated by the creation of new components. This doesn't always happen, but if a component type had 99 items, and we take get a pointer to a component, as soon as new components of the same type are created, the underlying std::vector containing the component gets expanded invalidating our pointer. The underlying std::vector gets expanded every 100 components.

To test, run the attached world (extension is .txt because github doesn't allow .sdf files). It should crash after 2 sim seconds.

debug_breadcrumbs_crash.txt

@azeey azeey requested a review from chapulina as a code owner June 4, 2020 20:51
@azeey azeey self-assigned this Jun 4, 2020
@nkoenig
Copy link
Contributor

nkoenig commented Jun 4, 2020

The segfault occurred because a pointer to a component is invalidated by the creation of new components. This doesn't always happen, but if a component type had 99 items, and we take get a pointer to a component, as soon as new components of the same type are created, the underlying std::vector containing the component gets expanded invalidating our pointer. The underlying std::vector gets expanded every 100 components.

This feels like an issue we should resolve in ECS, correct? I could image this biting us and others in the future. If this is the case, could you create an issue for it?

Copy link
Contributor

@chapulina chapulina left a comment

Choose a reason for hiding this comment

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

+1 to @nkoenig 's suggestion to ticket an issue.

And a mental note to always get Data() asap.

Copy link
Contributor

@iche033 iche033 left a comment

Choose a reason for hiding this comment

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

confirmed that this fixes the crash.

I think this exposed a bigger issue with components that needs to be fixed? I imagine there are other places in the code base that do something similar with these pointers.

Edit Just refreshed my page and saw the previous comments. Yes we should ticket an issue for this.

@azeey
Copy link
Contributor Author

azeey commented Jun 5, 2020

I can create an issue, but I'm not sure what can be done because this is inherent to using a contiguous data container for the components. We can move to using something like std::list, but that defeats the performance benefits of ECS.

azeey added 2 commits June 4, 2020 21:04
Signed-off-by: Addisu Z. Taddese <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants