libthr: Add an annotated _thread_start to satisfy unwinders on x86 and arm64

[dchagin]

Summary

The right unwinding stop indicator should be CFI-undefined PC.
https://dwarfstd.org/doc/Dwarf3.pdf - page 118:

• If a Return Address register is defined in the virtual unwind table,

and its rule is undefined (for example, by DW_CFA_undefined), then
there is no return address and no call address, and the virtual
unwind of stack activations is complete.

[emaste]

lib/libthr/arch/i386/i386/_thread_start.S

35

This seems surprising, why is i386 this way?

[dchagin]

lib/libthr/arch/i386/i386/_thread_start.S

35

The thread_start() arg is on the stack, the 'call' bellow will move stack down, so this popl is just to skip return address, perhaps it should be add $4, %esp, however I think the pop is faster

[kib]

lib/libthr/arch/amd64/amd64/_thread_start.S

36

Zero double-word is already pushed by kernel on the stack, and %rbp is zeroed.

lib/libthr/arch/i386/i386/_thread_start.S

35

No, the word on stack is the argument. Also, kernel already cleared %ebp. By popping the arg, I believe you destroy the arg.

lib/libthr/thread/thr_create.c

50

This ought to have __dead2 annotation.

51

There, __hidden annotation is due, but even better is to provide a symbol from the implementation namespace.

158

253

I do not understand how this should work. You provide asm-defined _thread_start, but then state that _thread_start is a weak alias for thread_start.

[kib]

lib/libthr/arch/amd64/amd64/_thread_start.S

36

Also this breaks the stack alignment, I believe.

[dchagin]

lib/libthr/arch/i386/i386/_thread_start.S

35

well, the stack layout on _thread_start is:
%esp+4 = arg
%esp+0 = 0 (fake return address)
%esp = aligned %esp - 4 (for fake return address), by popl I just skip ra for next call

I want to remove %ebp and fake ra zeroing, however I believe it would break the ABI in case when the old world is on the new kernel (eg, old world in a jail)

lib/libthr/thread/thr_create.c

253

Hi, I do not implement _thread_start for all supported achitectures due to I don't know ppc & riscv assembler. The _week alias is here just for it. Could you please suppose right way?

[kib]

lib/libthr/arch/i386/i386/_thread_start.S

35

Yes so why do that? If you really want to add _thread_start with this `.cfi_undefined %eip', add one more proper frame, copying the arg down, without hacking kernel-created frame.

lib/libthr/thread/thr_create.c

253

Only add weak alias on arches where you do not plan to add asm. Do it in arch-specific source files, not by #ifdef ing arches in thread/thr_create.c.

[dchagin]

lib/libthr/arch/i386/i386/_thread_start.S

35

What do you think about replacement to https://reviews.freebsd.org/D40709?
I don't want to add any frames, I just want to finish libunwind fixing

[dchagin]

Ugh, I hope all issues addressed, for now gdb and libunwind happy

[dchagin]

Manually upload diff

[kib]

lib/libthr/arch/amd64/amd64/_thread_start.S

35

I do not understand the comment. If anything, this addq aligns the stack.

lib/libthr/arch/arm/include/pthread_md.h

53

This weak reference becomes stuffed into each compilation unit of libthr.

lib/libthr/thread/thr_private.h

805

Can you change this prototype to void _thread_start(void *) and avoid cast in _pthread_create()?

[dchagin]

lib/libthr/arch/amd64/amd64/_thread_start.S

35

Sure, OTOH, it skips ra which the kernel leaves and zeroes for thread_start, I’ll propose a new comment

lib/libthr/arch/arm/include/pthread_md.h

53

I haven't figured out where to put it, also I don’t want to create a separate header file, I beleive this is temporary, maybe replace it with #define _thread_start thread_start?

[kib]

lib/libthr/arch/amd64/amd64/_thread_start.S

35

What do you mean by 'skip'? It does not skip, it pushes (an undefined) value on stack, below zero return address.

lib/libthr/arch/arm/include/pthread_md.h

53

IMO the best route is to provide empty _thread_start for other arches.

[dchagin]

By https://reviews.freebsd.org/D40841 and https://github.com/libunwind/libunwind/pull/571 unwinding througt thread_start is fine now on arm64 too.