orlyatomics/orly — pull request #13 by cmaloney (2014-04-23 21:08:07+00:00)
Title: Double destruct

Double Destruction of Sabot State object
----------------------------------------------------
In a number of cases we reused a state alloc for multiple sabots. This would be fine, but we then wrapped them in Sabot::TAny::TWrappers. This wrapper manually calls the destructor on destruction. When we would reuse the state allocation for multiple sabots, and the lifetimes of their wrappers would overlap, we would end up destructing the same sabot / same memory twice.

This causes a segfault with GCC 4.9 rather than just silently working.

Alloca merging
-------------------
It is not necessary to merge allocas by hand. All modern compilers don't use alloca as a function call, and they all merge allocas (It's actually clang's very first step), so just using alloca directly, in addition to being more legible / harder to get wrong, will result in the same optimized code generation.

Longer term fixes
----------------------
Practically speaking how to properly fix this / refactor the code so this can't happen again is two parts.
The first is to make the sabot state alloc just be an object which lives on the stack. This eliminates the need to call alloca() by hand, making the code simpler / cleaner etc.

The second part is to make the object aware of whether or not it is in use. This makes it so you get the same behavior the TWrapper() gave, but always uniformly enforced and we at least get runtime errors when proper reuse is not followed (Although we could also just make things replace, depending on the behavior we think is best)

Perf wise this should have roughly the same perf / overhead when optimized as not having it. The only difference should just be that there is an extra bool (1 byte) of space allocated for each state variable (to keep track of whether or not the destructor needs to be called).

@JasonL9000, @jmlvanre: Do these changes look reasonable to you? Should I work on restructuring the sabot state alloca to be cleaner / simpler / harder to get wrong as proposed above?