> --- a/csu/libc-start.c
> +++ b/csu/libc-start.c
> @@ -189,10 +189,12 @@ LIBC_START_MAIN (int (*main) (int, char **, char **
> MAIN_AUXVEC_DECL),
> /* Perform IREL{,A} relocations. */
> apply_irel ();
>
> +#ifndef __GNU__
> /* Initialize the thread library at least a bit since the libgcc
> functions are using thread functions if these are available and
> we need to setup errno. */
> __pthread_initialize_minimal ();
> +#endif
This is not the way we do things. Leaving aside why you are avoiding this
call, if you really need to then the way to do that is to add a
sysdeps/mach/hurd/libc-start.c that defines a static stub function and then
does #include <csu/libc-start.c>.
In that new file, you need some comments explaining why you are avoiding
the normal path to __libc_setup_tls.
> --- a/include/errno.h
> +++ b/include/errno.h
Same issue here. You can add a sysdeps/mach/hurd/include/errno.h,
which can use #include_next.
> --- a/mach/mach.h
> +++ b/mach/mach.h
> @@ -100,5 +100,8 @@ kern_return_t mach_setup_thread (task_t task, thread_t
> thread, void *pc,
> vm_address_t *stack_base,
> vm_size_t *stack_size);
>
> +/* Give THREAD a TLS area. */
> +kern_return_t __mach_setup_tls (thread_t thread);
> +kern_return_t mach_setup_tls (thread_t thread);
This doesn't really seem like a useful function to have in the public API.
It really does nothing but fiddle with libc internals that don't have a
public API, and the only thing outside libc itself that would call this
would be a threads library. But since you're not exporting it in any
Versions file, it really seems that it's purely internal. So put it in an
internal header and give it attribute_hidden.
Conversely, is there really any reason not to just roll this into
mach_setup_thread? You're calling it in all the internal places that use
__mach_setup_thread. mach_setup_thread has an existing interface contract
described pretty precisely by the comment in <mach.h>, albeit if TLS
support is going to exist at all in a given process, mach_setup_thread also
implicitly doing TLS setup is probably more useful than not. But if we are
actually worried that existing users of mach_setup_thread outside libc
itself would be perturbed by a new libc.so doing it, then we can add a new
symbol version for mach_setup_thread.
> +/* Give THREAD a TLS area. */
> +kern_return_t
> +__mach_setup_tls (thread_t thread)
> +{
> + kern_return_t error;
> + struct machine_thread_state ts;
> + mach_msg_type_number_t tssize = MACHINE_THREAD_STATE_COUNT;
> + tcbhead_t *tcb;
> +
> + if (error = __thread_get_state (thread, MACHINE_THREAD_STATE_FLAVOR,
> + (natural_t *) &ts, &tssize))
> + return error;
> + assert (tssize == MACHINE_THREAD_STATE_COUNT);
> +
> + tcb = _dl_allocate_tls (NULL);
> + if (!tcb)
No implicit Boolean coercion (use "tcb == NULL").
> + return KERN_RESOURCE_SHORTAGE;
Why not do _dl_allocate_tls first at the very top of the function,
before any of the other variable declarations? Then it's very clear,
and in the failure case you don't bother with __thread_get_state.
> + _hurd_tls_new (thread, &ts, tcb);
Technically code in mach/ shouldn't be calling code in mach/hurd/ like
this. What about the TLS details is actually Hurd-specific, anyway?
(But I won't really quibble about this separation, since it is useless
in practice.)
> --- a/sysdeps/generic/thread_state.h
> +++ b/sysdeps/generic/thread_state.h
> @@ -22,6 +22,7 @@
>
> /* Replace <machine> with "i386" or "mips" or whatever. */
>
> +#define MACHINE_NEW_THREAD_STATE_FLAVOR <machine>_NEW_THREAD_STATE
> #define MACHINE_THREAD_STATE_FLAVOR <machine>_THREAD_STATE
> #define MACHINE_THREAD_STATE_COUNT <machine>_THREAD_STATE_COUNT
AFAIK there is no Mach convention of flavors named
<machine>_NEW_THREAD_STATE. So you really need some comments here about
what MACHINE_NEW_THREAD_STATE_FLAVOR is for and how it ought to be used.
Having just looked at the kernel to see what the addition of
i386_REGS_SEGS_STATE was about, I am mystified. It enforces a few
constraints on the segment registers, which, if not met, should already
have meant that the thread would trap as soon as it was run.
I don't get it. If this is important to the uses in libc, then comments
in the libc code should make me get it.
> diff --git a/sysdeps/mach/hurd/bits/libc-lock.h
> b/sysdeps/mach/hurd/bits/libc-lock.h
> index 4ffb311..8bf5656 100644
> --- a/sysdeps/mach/hurd/bits/libc-lock.h
> +++ b/sysdeps/mach/hurd/bits/libc-lock.h
> @@ -20,6 +20,9 @@
> #define _BITS_LIBC_LOCK_H 1
>
> #if (_LIBC - 0) || (_CTHREADS_ - 0)
> +#if (_LIBC - 0)
> +#include <tls.h>
> +#endif
> #include <cthreads.h>
> #include <hurd/threadvar.h>
Why does it need tls.h if you're not touching this file otherwise?
And "# include" inside "#if".
> diff --git a/sysdeps/mach/hurd/fork.c b/sysdeps/mach/hurd/fork.c
> index 321421f..f19cfc4 100644
> --- a/sysdeps/mach/hurd/fork.c
> +++ b/sysdeps/mach/hurd/fork.c
> @@ -528,6 +528,11 @@ __fork (void)
> #endif
> MACHINE_THREAD_STATE_SET_PC (&state,
> (unsigned long int) _hurd_msgport_receive);
> +
> + /* Do special thread setup for TLS if needed. */
> + if (err = _hurd_tls_fork (sigthread, _hurd_msgport_thread, &state))
> + LOSE;
It's slightly confusing that this has exactly the same comment as the
call below. Maybe it's only because I was reading the patch before I
went to re-read the whole context. But it would be clearer if the
comment explicitly said this is doing TLS for the signal thread, while
the later one is for the main thread.
> diff --git a/sysdeps/mach/hurd/i386/init-first.c
> b/sysdeps/mach/hurd/i386/init-first.c
> index 8fb613b..74b3a56 100644
> --- a/sysdeps/mach/hurd/i386/init-first.c
> +++ b/sysdeps/mach/hurd/i386/init-first.c
> @@ -113,31 +113,11 @@ init1 (int argc, char *arg0, ...)
> data block; the argument strings start there. */
> if ((void *) d == argv[0])
> {
> -#ifndef SHARED
> - /* With a new enough linker (binutils-2.23 or better),
> - the magic __ehdr_start symbol will be available and
> - __libc_start_main will have done this that way already. */
> - if (_dl_phdr == NULL)
> - {
> - /* We may need to see our own phdrs, e.g. for TLS setup.
> - Try the usual kludge to find the headers without help from
> - the exec server. */
> - extern const void __executable_start;
> - const ElfW(Ehdr) *const ehdr = &__executable_start;
> - _dl_phdr = (const void *) ehdr + ehdr->e_phoff;
> - _dl_phnum = ehdr->e_phnum;
> - assert (ehdr->e_phentsize == sizeof (ElfW(Phdr)));
> - }
> -#endif
> return;
> }
Drop the braces around the single statement (return) in the if.
> @@ -193,6 +173,39 @@ init (int *data)
> ++envp;
> d = (void *) ++envp;
>
> +#ifndef SHARED
> + /* If we are the bootstrap task started by the kernel,
> + then after the environment pointers there is no Hurd
> + data block; the argument strings start there. */
> + if ((void *) d == argv[0])
> + {
> + /* With a new enough linker (binutils-2.23 or better),
> + the magic __ehdr_start symbol will be available and
> + __libc_start_main will have done this that way already. */
> + if (_dl_phdr == NULL)
> + {
> + /* We may need to see our own phdrs, e.g. for TLS setup.
> + Try the usual kludge to find the headers without help from
> + the exec server. */
> + extern const void __executable_start;
> + const ElfW(Ehdr) *const ehdr = &__executable_start;
> + _dl_phdr = (const void *) ehdr + ehdr->e_phoff;
> + _dl_phnum = ehdr->e_phnum;
> + assert (ehdr->e_phentsize == sizeof (ElfW(Phdr)));
> + }
> + }
> + else
> + {
> + _dl_phdr = (ElfW(Phdr) *) d->phdr;
> + _dl_phnum = d->phdrsz / sizeof (ElfW(Phdr));
> + assert (d->phdrsz % sizeof (ElfW(Phdr)) == 0);
> + }
I take it the reason for moving this from init1 to init is that the phdr
vars are needed by some TLS setup, which needs to happen earlier than
init1. You should add a comment before this bit, saying why it's
important that it be done early. It wouldn't hurt to put all the phdr
setup into a subroutine, and then you could just put that comment on the
call site.
Also, if you folks would be OK with requiring binutils-2.23 or better
for new Hurd builds of libc, then we could just drop all the nonsense
entirely and just have an assert, since _dl_phdr should already have
been set up in __libc_start_main. (If that's not actually true, then
the comment here needs to be changed.)
> + /* We need to setup TLS before starting sigthread */
Proper punctuation and two spaces after the sentence.
Say "the signal thread".
> + extern void __pthread_initialize_minimal(void);
Space before paren.
> @@ -70,7 +70,7 @@ _hurd_tls_init (tcbhead_t *tcb, int secondcall)
>
> /* Get the first available selector. */
> int sel = -1;
> - error_t err = __i386_set_gdt (tcb->self, &sel, desc);
> + kern_return_t err = __i386_set_gdt (tcb->self, &sel, desc);
What have you got against error_t? It's used for a superset of the
values kern_return_t can take.
> @@ -94,16 +94,16 @@ _hurd_tls_init (tcbhead_t *tcb, int secondcall)
> /* Fetch the selector set by the first call. */
> int sel;
> asm ("mov %%gs, %w0" : "=q" (sel) : "0" (0));
> - if (__builtin_expect (sel, 0x50) & 4) /* LDT selector */
> + if (__builtin_expect (sel, 0x48) & 4) /* LDT selector */
This change is quite meaningless in practice: (0x50&4) == (0x48&4).
But you should make it just __glibc_unlikely (sel & 4) anyway.
Do we not have some header file that defines macros for these bits.
The magic 4 is ugly. At least make a local function/macro for the
multiple uses here: bool __i386_selector_is_ldt or whatever.
> --- a/sysdeps/mach/thread_state.h
> +++ b/sysdeps/mach/thread_state.h
> @@ -37,6 +37,9 @@
> ((ts)->SP = (unsigned long int) (stack) + (size))
> #endif
> #endif
> +#ifndef MACHINE_THREAD_STATE_FIX_NEW
> +#define MACHINE_THREAD_STATE_FIX_NEW(ts)
> +#endif
"# define" inside #ifndef. And provide a comment explaining what the
macro is required to do.
Thanks,
Roland
