Re: [PATCH 2/2] jmpbuf: Add paddings for target specific usage

["H.J. Lu" <hjl dot tools at gmail dot com>]

On Wed, Nov 15, 2017 at 5:20 AM, H.J. Lu <hjl.tools@gmail.com> wrote:
> On Wed, Nov 15, 2017 at 3:10 AM, Florian Weimer <fweimer@redhat.com> wrote:
>> On 11/14/2017 02:10 PM, H.J. Lu wrote:
>>>
>>> +static int
>>> +do_test (void)
>>> +{
>>> +  sigjmp_buf sj;
>>> +  struct support_next_to_fault sigset_t_buf
>>> +    = support_next_to_fault_allocate (SIZEOF_SIGSET_T);
>>> +  sigset_t *m_p = (sigset_t *) sigset_t_buf.buffer;
>>> +  sigset_t m;
>>> +
>>> +  sigemptyset (&m);
>>> +  memcpy (m_p, &m, SIZEOF_SIGSET_T);
>>> +  sigprocmask (SIG_SETMASK, m_p, NULL);
>>> +  memcpy (&m, m_p, SIZEOF_SIGSET_T);
>>> +  if (sigsetjmp (sj, 0) == 0)
>>> +    {
>>> +      sigaddset (&m, SIGUSR1);
>>> +      memcpy (m_p, &m, SIZEOF_SIGSET_T);
>>> +      sigprocmask (SIG_SETMASK, m_p, NULL);
>>> +      memcpy (&m, m_p, SIZEOF_SIGSET_T);
>>> +      siglongjmp (sj, 1);
>>> +      return EXIT_FAILURE;
>>> +    }
>>> +  sigprocmask (SIG_SETMASK, NULL, m_p);
>>> +  memcpy (&m, m_p, SIZEOF_SIGSET_T);
>>> +  return sigismember (&m, SIGUSR1) ? EXIT_SUCCESS : EXIT_FAILURE;
>>> +}
>>
>>
>> Sorry, I don't understand anymore what this test is supposed to test and
>> how.
>
> This tests the reduced __jmp_buf_sigset_t used by __saved_mask is
> bigger than sigset expected by kernel.
>
>> To be honest, I don't like how you inject the internal definition of
>> jmp_buf.  Is this the way we do it for the nptl types?
>
> I only need to make some room in
>
> /* Calling environment, plus possibly a saved signal mask.  */
> struct __jmp_buf_tag
>   {
>     /* NOTE: The machine-dependent definitions of `__sigsetjmp'
>        assume that a `jmp_buf' begins with a `__jmp_buf' and that
>        `__mask_was_saved' follows it.  Do not move these members
>        or add others before it.  */
>     __jmp_buf __jmpbuf; /* Calling environment.  */
>     int __mask_was_saved; /* Saved the signal mask?  */
>     __sigset_t __saved_mask; /* Saved signal mask.  */
>   };
>
> for target specific purpose.   I changed it to
>
> struct __jmp_buf_tag
>   {
>     /* NOTE: The machine-dependent definitions of `__sigsetjmp'
>        assume that a `jmp_buf' begins with a `__jmp_buf' and that
>        `__mask_was_saved' follows it.  Do not move these members
>        or add others before it.  */
>     __jmp_buf __jmpbuf; /* Calling environment.  */
>     int __mask_was_saved; /* Saved the signal mask?  */
>     union
>     {
>     __sigset_t __saved_mask_compat;
>     struct
>       {
> __jmp_buf_sigset_t __saved_mask;
> /* Paddings for architecture specific usage.  */
> unsigned long int __padding[12];
>       } __saved;
>     }  __saved_mask;
>   };
>
> #define __saved_mask __saved_mask.__saved.__saved_mask
>
> I did only to  __sigset_t  in __jmp_buf_tag and this test verifies that the size
> of __jmp_buf_sigset_t works with sigprocmask.
>
> Are you suggesting we make some room in __sigset_t directly?  This will
> require very extensive changes. If not, what do you exactly suggest?
>
>> I think you should check _JUMP_BUF_SIGSET_NSIG against a kernel constant
>> (_NSIGS?) somewhere.
>
> There is a _Static_assert in include/setjmp.h.  But it isn't triggered.  This
> updated patch moves it to sysdeps/unix/sysv/linux/__saved_mask.h.   Now
> I got
>
> ../sysdeps/unix/sysv/linux/__saved_mask.h:32:1: error: static
> assertion failed: "size of ___saved_mask < size of
> __sigprocmask_sigset_t"
>  _Static_assert (sizeof (___saved_mask) >= sizeof (__sigprocmask_sigset_t),
>
> if __jmp_buf_sigset_t is too small.
>
> --
> H.J.

Any comments, suggestions or objections?  The patch is at

https://sourceware.org/ml/libc-alpha/2017-11/msg00510.html

-- 
H.J.