From: Florian Weimer <fw@deneb.enyo.de>
To: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Cc: libc-alpha <libc-alpha@sourceware.org>,
carlos <carlos@redhat.com>, Rich Felker <dalias@libc.org>,
linux-api <linux-api@vger.kernel.org>,
Boqun Feng <boqun.feng@gmail.com>,
Will Deacon <will.deacon@arm.com>,
linux-kernel <linux-kernel@vger.kernel.org>,
Peter Zijlstra <peterz@infradead.org>,
Ben Maurer <bmaurer@fb.com>, Dave Watson <davejwatson@fb.com>,
Thomas Gleixner <tglx@linutronix.de>,
Paul <paulmck@linux.vnet.ibm.com>, Paul Turner <pjt@google.com>,
Joseph Myers <joseph@codesourcery.com>
Subject: Re: [RFC PATCH glibc 4/8] glibc: Perform rseq(2) registration at C startup and thread creation (v15)
Date: Thu, 19 Mar 2020 19:34:48 +0100 [thread overview]
Message-ID: <87k13ggpmf.fsf@mid.deneb.enyo.de> (raw)
In-Reply-To: <1103782439.4046.1584642531222.JavaMail.zimbra@efficios.com> (Mathieu Desnoyers's message of "Thu, 19 Mar 2020 14:28:51 -0400 (EDT)")
* Mathieu Desnoyers:
> ----- On Mar 19, 2020, at 2:16 PM, Florian Weimer fw@deneb.enyo.de 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.
next prev parent reply other threads:[~2020-03-19 18:36 UTC|newest]
Thread overview: 21+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-03-19 14:41 [RFC PATCH glibc 0/8] Restartable Sequences enablement Mathieu Desnoyers
2020-03-19 14:41 ` [RFC PATCH glibc 1/8] Introduce <elf_machine_sym_no_match.h> Mathieu Desnoyers
2020-03-19 14:41 ` [RFC PATCH glibc 2/8] Implement __libc_early_init Mathieu Desnoyers
2020-03-19 14:41 ` [RFC PATCH glibc 3/8] nptl: Start new threads with all signals blocked [BZ #25098] Mathieu Desnoyers
2020-03-19 14:41 ` [RFC PATCH glibc 4/8] glibc: Perform rseq(2) registration at C startup and thread creation (v15) Mathieu Desnoyers
2020-03-19 14:53 ` Florian Weimer
2020-03-19 15:56 ` Mathieu Desnoyers
2020-03-19 16:03 ` Florian Weimer
2020-03-19 18:09 ` Mathieu Desnoyers
2020-03-19 18:16 ` Florian Weimer
2020-03-19 18:28 ` Mathieu Desnoyers
2020-03-19 18:34 ` Florian Weimer [this message]
2020-03-19 18:55 ` Mathieu Desnoyers
2020-03-19 19:05 ` Florian Weimer
2020-03-19 19:46 ` Mathieu Desnoyers
2020-03-20 13:44 ` Mathieu Desnoyers
2020-03-20 14:47 ` Mathieu Desnoyers
2020-03-19 14:41 ` [RFC PATCH glibc 5/8] glibc: sched_getcpu(): use rseq cpu_id TLS on Linux (v6) Mathieu Desnoyers
2020-03-19 14:41 ` [RFC PATCH glibc 6/8] support record failure: allow use from constructor Mathieu Desnoyers
2020-03-19 14:41 ` [RFC PATCH glibc 7/8] support: implement xpthread key create/delete (v4) Mathieu Desnoyers
2020-03-19 14:41 ` [RFC PATCH glibc 8/8] rseq registration tests (v8) Mathieu Desnoyers
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87k13ggpmf.fsf@mid.deneb.enyo.de \
--to=fw@deneb.enyo.de \
--cc=bmaurer@fb.com \
--cc=boqun.feng@gmail.com \
--cc=carlos@redhat.com \
--cc=dalias@libc.org \
--cc=davejwatson@fb.com \
--cc=joseph@codesourcery.com \
--cc=libc-alpha@sourceware.org \
--cc=linux-api@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=mathieu.desnoyers@efficios.com \
--cc=paulmck@linux.vnet.ibm.com \
--cc=peterz@infradead.org \
--cc=pjt@google.com \
--cc=tglx@linutronix.de \
--cc=will.deacon@arm.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).