Re: [PATCH 01/23] hurd: Add some missing includes

[Sergey Bugaev]

On Wed, Jan 3, 2024 at 11:43 PM Samuel Thibault <samuel.thibault@gnu.org> wrote:
> Sergey Bugaev, le mer. 03 janv. 2024 20:14:34 +0300, a ecrit:
> > diff --git a/sysdeps/hurd/include/hurd.h b/sysdeps/hurd/include/hurd.h
> > index 568092d6..189fd44e 100644
> > --- a/sysdeps/hurd/include/hurd.h
> > +++ b/sysdeps/hurd/include/hurd.h
> > @@ -1,4 +1,5 @@
> >  #ifndef      _HURD_H
> > +#include <tls.h>
> >  #include_next <hurd.h>
> >
> >  void _hurd_libc_proc_init (char **argv);
> > diff --git a/sysdeps/hurd/include/hurd/signal.h 
> > b/sysdeps/hurd/include/hurd/signal.h
> > index 1dc8a1f3..9b1bf3df 100644
> > --- a/sysdeps/hurd/include/hurd/signal.h
> > +++ b/sysdeps/hurd/include/hurd/signal.h
> > @@ -6,6 +6,7 @@ extern struct hurd_sigstate *_hurd_self_sigstate (void) 
> > __attribute__ ((__const_
> >  libc_hidden_proto (_hurd_self_sigstate)
> >  #endif
> >
> > +#include <tls.h>
> >  #include_next <hurd/signal.h>
> >
> >  #ifndef _ISOMAC
>
> Why?

Because hurd/hurd/signal.h is using tls.h macros (THREAD_GETMEM /
THREAD_SETMEM / THREAD_SELF), guarded under "defined _LIBC", and
sysdeps/hurd/include/hurd/signal.h being the internal version of that
header seemed to be the appropriate place to add the missing #include
<tls.h>. Otherwise, I get this:

In file included from ../sysdeps/hurd/include/hurd/signal.h:9,
                 from siginfo.c:18:
../hurd/hurd/signal.h: In function ‘_hurd_self_sigstate’:
../hurd/hurd/signal.h:169:30: error: implicit declaration of function
‘THREAD_GETMEM’ [-Wimplicit-function-declaration]
  169 |   struct hurd_sigstate *ss = THREAD_GETMEM (THREAD_SELF,
_hurd_sigstate);

and so on. This must have happened to work on both x86 architectures
due to some other header implicitly pulling <tls.h> in, but we should
not rely on that.

Perhaps a better solution would be to move the inline versions of
_hurd_self_sigstate and _hurd_critical_section_lock/unlock to the
internal header. Is there any reason why they have to be in the public
one?

> These are breaking hurd/check-installed-headers-c

Indeed, thanks for pointing that out. But the error I seem to get:

:: hurd.h
::::
In file included from ../sysdeps/unix/i386/sysdep.h:18,
                 from ../sysdeps/mach/x86/sysdep.h:47,
                 from ../sysdeps/mach/hurd/tls.h:27,
                 from ../sysdeps/mach/hurd/i386/tls.h:24,
                 from ../sysdeps/hurd/include/hurd.h:2,
                 from /tmp/cih_test_z99fCI.c:10:
../sysdeps/unix/sysdep.h:111:5: error: "IS_IN" is not defined,
evaluates to 0 [-Werror=undef]
  111 | #if IS_IN (rtld)
      |     ^~~~~
../sysdeps/unix/sysdep.h:111:11: error: missing binary operator before token "("
  111 | #if IS_IN (rtld)
      |           ^
../sysdeps/mach/hurd/i386/tls.h:123:32: error: missing binary operator
before token "("
  123 | #if !defined (SHARED) || IS_IN (rtld)
      |                                ^
../sysdeps/mach/hurd/i386/tls.h: In function ‘_hurd_tls_fork’:
../sysdeps/mach/hurd/i386/tls.h:379:3: error: unknown type name ‘error_t’
  379 |   error_t err;
      |   ^~~~~~~

...makes no sense. This is testing installed headers, isn't it? — then
how come sysdeps/hurd/include/hurd.h is what gets found for <hurd.h>?
I'm rather sure the installed <hurd.h> is a different file. So it
would look like the test setup is broken, and this patch just exposes
that.

Sergey