* Mathieu Desnoyers: > ----- On Mar 19, 2020, at 2:16 PM, Florian Weimer fw@xxxxxxxxxxxxx wrote: > >> * Mathieu Desnoyers: >> >>>> You also need to add an assert that the compiler supports >>>> __attribute__ ((aligned)) because ignoring it produces an >>>> ABI-incompatible header. >>> >>> Are you aware of some helper macro I should use to do this, or >>> is it done elsewhere in glibc ? >> >> I don't think we have any such GCC-only types yet. max_align_t is >> provided by GCC itself. > > I was thinking of adding the following to > > sysdeps/unix/sysv/linux/rseq-internal.h: rseq_register_current_thread() > > + /* Ensure the compiler supports __attribute__ ((aligned)). */ > + _Static_assert (__alignof__ (struct rseq_cs) >= 4 * sizeof(uint64_t), > + "alignment"); > + _Static_assert (__alignof__ (struct rseq) >= 4 * sizeof(uint64_t), > + "alignment"); > + Something like it would have to go into the *public* header. Inside glibc, you can assume __attribute__ support. >>>> The struct rseq/struct rseq_cs definitions >>>> are broken, they should not try to change the alignment. >>> >>> AFAIU, this means we should ideally not have used __attribute__((aligned)) >>> in the uapi headers in the first place. Why is it broken ? >> >> Compilers which are not sufficiently GCC-compatible define >> __attribute__(X) as the empty expansion, so you silently get a >> different ABI. > > It is worth noting that rseq.h is not the only Linux uapi header > which uses __attribute__ ((aligned)), so this ABI problem exists today > anyway for those compilers. Yuck. Even with larger-than-16 alignment? >> There is really no need to specify 32-byte alignment here. Is not >> even the size of a standard cache line. It can result in crashes if >> these structs are heap-allocated using malloc, when optimizing for >> AVX2. > > Why would it be valid to allocate those with malloc ? Isn't it the > purpose of posix_memalign() ? It would not be valid, but I don't think we have diagnostics for C like we have them for C++'s operator new. >>> However, now that it is in the wild, it's a bit late to change that. >> >> I had forgotten about the alignment crashes. I think we should >> seriously consider changing the types. 8-( > > I don't think this is an option at this stage given that it is part > of the Linux kernel UAPI. I am not convinced that it is valid at all > to allocate struct rseq or struct rseq_cs with malloc(), because it > does not guarantee any alignment. The kernel ABI doesn't change. The kernel cannot use the alignment information anyway. Userspace struct layout may change in subtle ways, though.
