On Wed, Oct 7, 2020 at 8:46 AM Dave Martin <Dave.Martin@xxxxxxx> wrote:
>
> On Wed, Oct 07, 2020 at 06:30:03AM -0700, H.J. Lu wrote:
> > On Wed, Oct 7, 2020 at 3:47 AM Dave Martin <Dave.Martin@xxxxxxx> wrote:
> > >
> > > On Tue, Oct 06, 2020 at 10:44:14AM -0700, H.J. Lu wrote:
>
> [...]
>
> > > > I updated my glibc patch to add both _SC_MINSIGSTKSZ and _SC_SIGSTKSZ.
> > > > _SC_MINSIGSTKSZ is the minimum signal stack size from AT_MINSIGSTKSZ,
> > > > which is the signal frame size used by kernel, and _SC_SIGSTKSZ is the value
> > > > of sysconf (_SC_MINSIGSTKSZ) + 6KB for user application.
> > >
> > > Can I suggest sysconf (_SC_MINSIGSTKSZ) * 4 instead?
> >
> > Done.
>
> OK. I was prepared to have to argue my case a bit more, but if you
> think this is OK then I will stop arguing ;)
>
>
> > > If the arch has more or bigger registers to save in the signal frame,
> > > the chances are that they're going to get saved in some userspace stack
> > > frames too. So I suspect that the user signal handler stack usage may
> > > scale up to some extent rather than being a constant.
> > >
> > >
> > > To help people migrate without unpleasant surprises, I also figured it
> > > would be a good idea to make sure that sysconf (_SC_MINSIGSTKSZ) >=
> > > legacy MINSIGSTKSZ, and sysconf (_SC_SIGSTKSZ) >= legacy SIGSTKSZ.
> > > This should makes it safer to use sysconf (_SC_MINSIGSTKSZ) as a
> > > drop-in replacement for MINSIGSTKSZ, etc.
> > >
> > > (To explain: AT_MINSIGSTKSZ may actually be < MINSIGSTKSZ on AArch64.
> > > My idea was that sysconf () should hide this surprise, but people who
> > > really want to know the kernel value would tolerate some
> > > nonportability and read AT_MINSIGSTKSZ directly.)
> > >
> > >
> > > So then:
> > >
> > > kernel_minsigstksz = getauxval(AT_MINSIGSTKSZ);
> > > minsigstksz = LEGACY_MINSIGSTKSZ;
> > > if (kernel_minsigstksz > minsigstksz)
> > > minsistksz = kernel_minsigstksz;
> > >
> > > sigstksz = LEGACY_SIGSTKSZ;
> > > if (minsigstksz * 4 > sigstksz)
> > > sigstksz = minsigstksz * 4;
> >
> > I updated users/hjl/AT_MINSIGSTKSZ branch with
> >
> > +@item _SC_MINSIGSTKSZ
> > +@standards{GNU, unistd.h}
>
> Can we specify these more agressively now?
>
> > +Inquire about the signal stack size used by the kernel.
>
> I think we've already concluded that this should included all mandatory
> overheads, including those imposed by the compiler and glibc?
>
> e.g.:
>
> --8<--
>
> The returned value is the minimum number of bytes of free stack space
> required in order to gurantee successful, non-nested handling of a
> single signal whose handler is an empty function.
>
> -->8--
>
> > +
> > +@item _SC_SIGSTKSZ
> > +@standards{GNU, unistd.h}
> > +Inquire about the default signal stack size for a signal handler.
>
> Similarly:
>
> --8<--
>
> The returned value is the suggested minimum number of bytes of stack
> space required for a signal stack.
>
> This is not guaranteed to be enough for any specific purpose other than
> the invocation of a single, non-nested, empty handler, but nonetheless
> should be enough for basic scenarios involving simple signal handlers
> and very low levels of signal nesting (say, 2 or 3 levels at the very
> most).
>
> This value is provided for developer convenience and to ease migration
> from the legacy SIGSTKSZ constant. Programs requiring stronger
> guarantees should avoid using it if at all possible.
>
> -->8--
Done.
>
> If these descriptions are too wordy, we might want to move some of it
> out to signal.texi, though.
>
> >
> > case _SC_MINSIGSTKSZ:
> > assert (GLRO(dl_minsigstacksize) != 0);
> > return GLRO(dl_minsigstacksize);
> >
> > case _SC_SIGSTKSZ:
> > {
> > /* Return MAX (MINSIGSTKSZ, sysconf (_SC_MINSIGSTKSZ)) * 4. */
> > long int minsigstacksize = GLRO(dl_minsigstacksize);
> > _Static_assert (__builtin_constant_p (MINSIGSTKSZ),
> > "MINSIGSTKSZ is constant");
> > if (minsigstacksize < MINSIGSTKSZ)
> > minsigstacksize = MINSIGSTKSZ;
> > return minsigstacksize * 4;
> > }
> >
> > >
> > > (Or something like that, unless the architecture provides its own
> > > definitions. ia64's MINSIGSTKSZ is enormous, so it probably doesn't
> > > want this.)
> > >
> > >
> > > Also: should all these values be rounded up to a multiple of the
> > > architecture's preferred stack alignment?
> >
> > Kernel should provide a properly aligned AT_MINSIGSTKSZ.
>
> OK. Can you comment on Chang S. Bae's series? I wasn't sure that the
> proposal produces an aligned value for AT_MINSIGSTKSZ on x86, but maybe
> I just worked it out wrong.
In Linux kernel, the signal frame data is composed of the following areas
and laid out as:
------------------------------
| alignment padding |
------------------------------
| xsave buffer | <<<<<<< This must be 64 byte aligned
------------------------------
| fsave header (32-bit only) |
------------------------------
| siginfo + ucontext |
------------------------------
In order to get the proper alignment for xsave buffer, the signal stake
size should be rounded up to 64 byte aligned. In glibc, I have
/* Size of xsave buffer. */
unsigned int sigframe_size = ebx;
if (64 bit)
/* NB: sizeof(struct rt_sigframe) in Linux kernel. */
sigframe_size += 440;
else
/* NB: sizeof(struct sigframe_ia32) + sizeof(struct fregs_state)) in
Linux kernel. */
sigframe_size += 736 + 112;
endif
/* Round up to 64-byte aligned. */
sigframe_size = ALIGN_UP (sigframe_size, 64);
GLRO(dl_minsigstacksize) = sigframe_size;
Kernel should do something similar.
>
> > > Should the preferred stack alignment also be exposed through sysconf?
> > > Portable code otherwise has no way to know this, though if the
> > > preferred alignment is <= the minimum malloc()/alloca() alignment then
> > > this is generally not an issue.)
> >
> > Good question. But it is orthogonal to the signal stack size issue.
>
> Ack.
>
> There are various other brokennesses around this area, such as the fact
> that the register data may now run off the end of the mcontext_t object
> that is supposed to contain it. Ideally we should fork ucontext_t or
> mcontext_t into two types, one for the ucontext API and one for the
> signal API (which are anyway highly incompatible with each other).
>
> If this stuff is addressed, I guess we could bundle it under a more
> general feature test macro. But it's probably best to nail down this
> series in something like its current form first.
Agreed.
>
> I'll follow up on libc-alpha with a wishlist, but that will be a
> separate conversation...
>
> [...]
>
> Cheers
> ---Dave
--
H.J.
