Closed Bug 1391103 Opened 3 years ago Closed 2 years ago

Align mStorage in Maybe as when the type in a struct

Categories

(Core :: MFBT, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox57 --- fixed

People

(Reporter: xidorn, Assigned: xidorn)

References

Details

Attachments

(1 file)

On some platforms, a type can have two alignment: one is when it is used as itself, and the other is when it is used as a struct field. And alignof and alignas reports the first one, which isn't really useful for Maybe, because we are using it inside a struct.

For example, on 32bit Linux, uint64_t and double have 8byte alignment as their own, but when they are inside a struct, they are aligned to 4byte instead. [1]

Make Maybe::mStroage use the second alignment helps better packing in those cases, and that can also workaround a Rust inconsistency with C++ [2] which causes bindgen issue [3] on 32bit Linux.


[1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79024
[2] https://github.com/rust-lang/rust/issues/43899
[3] https://github.com/rust-lang-nursery/rust-bindgen/issues/917
Perhaps it should be called MOZ_ALIGNAS_IN_STRUCT to make it more clear that it's intended to be different from alignas (rather than, say, a replacement for use for compilers that don't support it)?
I cannot really think of a use case where the original alignas is more useful than this macro...
Comment on attachment 8898050 [details]
Bug 1391103 - Align Maybe::mStorage like when the type is in a struct.

https://reviewboard.mozilla.org/r/169356/#review175934

::: mfbt/Alignment.h:55
(Diff revision 1)
> + *
> + * Known examples are 64bit types (uint64_t, double) on 32bit Linux,
> + * where they have 8byte alignment on their own, and 4byte alignment
> + * when in struct.
> + */
> +#define MOZ_ALIGNAS(T) alignas(mozilla::detail::AlignasHelper<T>)

I think dbaron's suggestion of `MOZ_ALIGNAS_IN_STRUCT` is a better name for this macro, for the reasons he cites.
Attachment #8898050 - Flags: review?(nfroyd) → review+
Comment on attachment 8898050 [details]
Bug 1391103 - Align Maybe::mStorage like when the type is in a struct.

https://reviewboard.mozilla.org/r/169356/#review175938

r=me with the below change.
Pushed by xquan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/06d2f579a7f3
Align Maybe::mStorage like when the type is in a struct. r=froydnj
https://hg.mozilla.org/mozilla-central/rev/06d2f579a7f3
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.