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

Organism refactor with multiple inheritance #198

Draft
wants to merge 26 commits into
base: complex-genomes
Choose a base branch
from

Conversation

naalit
Copy link
Contributor

@naalit naalit commented Nov 11, 2022

I don't know if we actually want to go through with all of this, I'm basically trying to take it as far as I can and then we can figure out how much we want to keep. This PR also adds more cc files, but they're still just #included instead of actually being compiled separately because Empirical doesn't support that. Unfortunately, that means they have to be included in each entry point file.

Comment on lines +116 to +120
emp::Ptr<PGGSymbiont> sym = syms[i].DynamicCast<PGGSymbiont>();
if (sym == nullptr) {
throw "Non-PGG symbiont in PGG host";
}
double hostPool = sym->ProcessPool();
Copy link
Contributor Author

@naalit naalit Dec 1, 2022

Choose a reason for hiding this comment

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

This is the worst part of this change in my opinion, unfortunately I haven't really found any way to get around it without either adding stubs to BaseSymbiont (so the "Organism has every method needed by any mode" approach used before this PR) or using far too many templates. At least it makes each place where we have a requirement for a specific subclass explicit.

@naalit
Copy link
Contributor Author

naalit commented Dec 6, 2022

After an unreasonable amount of time spent debugging this, it looks like Empirical's pointer tracking infrastructure doesn't actually support inheritance correctly. It's only appearing now that the inheritance is more complex with the multiple inheritance changes, but it's not actually specific to multiple inheritance - it assumes that the address of an object doesn't change when it's casted to a base class, but that's not actually true in general. Specifically, we create a pointer to Host, and Empirical notes down its address as a valid pointer; then we use it as a pointer to Organism, and the compiler cuts off the Host vtables and moves the pointer forward a few bytes. When we try to delete it, the compiler knows how to delete the correct object, but to Empirical it looks like we're deleting an object that was never allocated, since the address is different.

I'm not sure what the best approach to get around this is, but the easiest option would be to just give up on Empirical's pointer tracking - remove -DEMP_TRACK_MEM from the debug mode flags. In my experience AddressSanitizer works significantly better anyway, but if you want to keep Empirical's pointer tracking, we can try to find another solution.

(Pointer tracking is disabled for now but a fix is hopefully on the way, see devosoft/Empirical#476 and devosoft/Empirical#477)

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.

1 participant