Open Bug 1343717 Opened 3 years ago Updated 3 years ago

Remove mozilla::AlignedStorage2

Categories

(Core :: MFBT, enhancement)

enhancement
Not set

Tracking

()

People

(Reporter: Waldo, Unassigned)

Details

It's too easy for people to misuse AlignedStorage2, by presuming that memcpying or memmoving it around is safe.  See the extensive discussion of that problem at this URL:

http://whereswalden.com/2017/02/27/a-pitfall-in-c-low-level-object-creation-and-storage-and-how-to-avoid-it/

Each use of AlignedStorage2 should be replaced with |alignas(T) unsigned char buf[sizeof(T)];|, a sibling |void* data() { return buf; }| to evade the GCC warning mentioned in that blog post, a placement-new into |data()|, and careful examination of the enclosing type's copy constructor, assignment operator, and move variants of these to ensure they comply with C++'s object model.

I'd like to somewhat prioritize this, because the compiler problems we hit if the strict aliasing rules are improperly followed are hard to track down -- see bug 1269319, for example.  But realistically, I'm not going to have the time to do it, except maybe piecemeal as occasional palate cleanser.

This feels like Good First Bug territory for someone relatively new to Mozilla coding, who nonetheless has a good grip on C++ programming generally -- and ideally has some interest in understanding some of the details of the C++ memory/object models.  It's somewhat unlikely to require deep knowledge of any individual area of code that would need to be touched, although it's not *impossible* it'd require more knowledge, and potentially (see bug 1341951, for example) require assistance from an expert in the relevant code.
I should also say -- replacement with |alignas(T) unsigned char buf[...]| and so on is *not* the preferred solution, if a utility class like mozilla::Variant, mozilla::Maybe, mozilla::MaybeOneOf, or so on is adequate to the task.  So some judgment is required to be sure that the correct replacement is provided, in each case.  And possibly, if we see enough of these places, a new utility class may want to be implemented to augment the existing set of them.
"new utility class"

If I may take this opportunity to suggest a few things I could have used in the past:

A. Extend Variant or create a separate class, that accepts the same type more than once; access would be done through numeric indices like with std::variant::get<N>(). I've actually opened a bug for that already: bug 1338389.


B. A kind of Maybe that doesn't keep track of the state. So it just offers accepted ways to construct/access/destroy the stored object, leaving the responsibility to track the state to the owner. Think of it as the storage that Maybe would use internally.

C. Similarly, a kind of Variant that doesn't keep track of the currently-stored type. (In fact, the Maybe-storage in proposal B could possibly just be this Variant-storage with only one type.)

These last two could potentially be the building bricks needed&vetted to build other classes we would want.
Though I'm scared I'm just proposing to re-implement AlignedStorage, in which case please ignore me!
You need to log in before you can comment on or make changes to this bug.