hppa: Fix stack alignment passed to clone

The hppa architecture requires strict alignment for loads and stores.  As a result, the
minimum stack alignment that will work is 8 bytes.  This patch adjust __clone() to align
the stack argument passed to it.  It also adjusts slightly some formatting.

This fixes the nptl/tst-tls1 test.

Okay?

Dave

 sysdeps/unix/sysv/linux/hppa/clone.S | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

* John David Anglin:

> The hppa architecture requires strict alignment for loads and
> stores.  As a result, the minimum stack alignment that will work is
> 8 bytes.  This patch adjust __clone() to align the stack argument
> passed to it.  It also adjusts slightly some formatting.
>
> This fixes the nptl/tst-tls1 test.

I wonder if we should change this in nptl/tst-tls1

  xpthread_attr_setstack (&a, thr_stack + 1, STACK_SIZE);

to:

  xpthread_attr_setstack (&a, thr_stack + _Alignof (max_align_t), STACK_SIZE);

This should fix the test on hppa.  Since the alignment of the test TLS
variables is much larger than that of max_align_t, I don't think it
interferes with the objective of the test.

On 2019-10-19 3:17 p.m., Florian Weimer wrote:
> * John David Anglin:
>
>> The hppa architecture requires strict alignment for loads and
>> stores.  As a result, the minimum stack alignment that will work is
>> 8 bytes.  This patch adjust __clone() to align the stack argument
>> passed to it.  It also adjusts slightly some formatting.
>>
>> This fixes the nptl/tst-tls1 test.
> I wonder if we should change this in nptl/tst-tls1
>
>   xpthread_attr_setstack (&a, thr_stack + 1, STACK_SIZE);
>
> to:
>
>   xpthread_attr_setstack (&a, thr_stack + _Alignof (max_align_t), STACK_SIZE);
>
> This should fix the test on hppa.  Since the alignment of the test TLS
> variables is much larger than that of max_align_t, I don't think it
> interferes with the objective of the test.
Yes, that should fix the test.  However, I still think the hppa fix should be applied because it
avoids misaligned accesses and then a segmentation fault in ld.so.1.  We have this code in
__clone():

        /* Ensure stack argument is 8-byte aligned.  */
        ldo             7(%r25),%r25
        depi            0,31,3,%r25

        /* Save the function pointer, arg, and flags on the new stack.  */
        stwm    %r26, 64(%r25)
        stw     %r23, -60(%r25)
        stw     %r24, -56(%r25)

The stwm instruction adds 64 to %r25.  The preceding two instructions round the stack argument
to an 8-byte aligned value.  So, we are just adding an additional 0 to 7.  Not a big deal.

If %r25 is not aligned, the stores for %r23 and %r24 generated unaligned traps.  Then, the kernel mucks
with the stack argument because the PSW W bit is stored in the bottom of sp:

        /* for now we can *always* set the W bit on entry to the syscall
         * since we don't support wide userland processes.  We could
         * also save the current SM other than in r0 and restore it on
         * exit from the syscall, and also use that value to know
         * whether to do narrow or wide syscalls. -PB
         */
        ssm     PSW_SM_W, %r1
        extrd,u %r1,PSW_W_BIT,1,%r1
        /* sp must be aligned on 4, so deposit the W bit setting into
         * the bottom of sp temporarily */
        or,ev   %r1,%r30,%r30

The child crashes when clone returns.  With the alignment, the code runs correctly.  So, I think we
shouldn't worry about the extra two instructions.

The other alternative is to generate an error when passed a misaligned stack argument.  I looked at
the opengroup spec but it didn't clarify the situations where arguments are invalid.

Dave

* John David Anglin:

> The other alternative is to generate an error when passed a
> misaligned stack argument.  I looked at the opengroup spec but it
> didn't clarify the situations where arguments are invalid.

I think an unaligned stack triggers undefined behavior, so a crash
would be acceptable in this context.  We could maybe add an assert to
the thread creation (although that will cause backwards compatibility
concerns on i386 due to the silent ABI change for SSE2).

On 2019-10-19 2:33 p.m., John David Anglin wrote:
> This fixes the nptl/tst-tls1 test.
This is bug 25066.

Dave

On 2019-10-19 4:07 p.m., Florian Weimer wrote:
> I think an unaligned stack triggers undefined behavior, so a crash
> would be acceptable in this context.  We could maybe add an assert to
> the thread creation (although that will cause backwards compatibility
> concerns on i386 due to the silent ABI change for SSE2).
The pthread_create() function is supposed to return EINVAL if the attributes specified by
the attr argument are invalid.  The Open Group specification doesn't say a crash is acceptable
behavior.  We don't actually have a "stack" at this point.  We have a pointer to memory to
be used as the stack for the new thread.

As I see it, we have two alternatives in the hppa __clone() implementation:

1) Return EINVAL when passed a misaligned pointer, or
2) Align the pointer.

Dave