[01/23] hurd: Add some missing includes

Message ID 20240103171502.1358371-2-bugaevc@gmail.com
State Superseded
Headers
Series aarch64-gnu port |

Checks

Context Check Description
redhat-pt-bot/TryBot-apply_patch success Patch applied to master at the time it was sent
redhat-pt-bot/TryBot-still_applies warning Patch no longer applies to master

Commit Message

Sergey Bugaev Jan. 3, 2024, 5:14 p.m. UTC
  Signed-off-by: Sergey Bugaev <bugaevc@gmail.com>
---
 hurd/hurdsig.c                     | 2 +-
 mach/lowlevellock.h                | 1 +
 sysdeps/hurd/include/hurd.h        | 1 +
 sysdeps/hurd/include/hurd/signal.h | 1 +
 sysdeps/mach/hurd/mig-reply.c      | 1 +
 5 files changed, 5 insertions(+), 1 deletion(-)
  

Comments

Samuel Thibault Jan. 3, 2024, 8:43 p.m. UTC | #1
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?

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

Samuel
  
Samuel Thibault Jan. 3, 2024, 9 p.m. UTC | #2
Applied the rest, thanks!

Sergey Bugaev, le mer. 03 janv. 2024 20:14:34 +0300, a ecrit:
> Signed-off-by: Sergey Bugaev <bugaevc@gmail.com>
> ---
>  hurd/hurdsig.c                     | 2 +-
>  mach/lowlevellock.h                | 1 +
>  sysdeps/hurd/include/hurd.h        | 1 +
>  sysdeps/hurd/include/hurd/signal.h | 1 +
>  sysdeps/mach/hurd/mig-reply.c      | 1 +
>  5 files changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/hurd/hurdsig.c b/hurd/hurdsig.c
> index 3deb94f9..fe788193 100644
> --- a/hurd/hurdsig.c
> +++ b/hurd/hurdsig.c
> @@ -15,6 +15,7 @@
>     License along with the GNU C Library; if not, see
>     <https://www.gnu.org/licenses/>.  */
>  
> +#include <assert.h>
>  #include <stdio.h>
>  #include <stdlib.h>
>  #include <string.h>
> @@ -260,7 +261,6 @@ _hurd_sigstate_actions (struct hurd_sigstate *ss)
>  #include <hurd/msg_server.h>
>  #include <hurd/msg_reply.h>	/* For __msg_sig_post_reply.  */
>  #include <hurd/interrupt.h>
> -#include <assert.h>
>  #include <unistd.h>
>  
>  
> diff --git a/mach/lowlevellock.h b/mach/lowlevellock.h
> index 129a7bd9..c5bb0879 100644
> --- a/mach/lowlevellock.h
> +++ b/mach/lowlevellock.h
> @@ -19,6 +19,7 @@
>  #ifndef _MACH_LOWLEVELLOCK_H
>  #define _MACH_LOWLEVELLOCK_H   1
>  
> +#include <mach.h>
>  #include <mach/gnumach.h>
>  #include <atomic.h>
>  
> 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
> diff --git a/sysdeps/mach/hurd/mig-reply.c b/sysdeps/mach/hurd/mig-reply.c
> index 57bcd808..85c3af13 100644
> --- a/sysdeps/mach/hurd/mig-reply.c
> +++ b/sysdeps/mach/hurd/mig-reply.c
> @@ -15,6 +15,7 @@
>     License along with the GNU C Library; if not, see
>     <https://www.gnu.org/licenses/>.  */
>  
> +#include <assert.h>
>  #include <mach.h>
>  #include <mach/mig_support.h>
>  #include <tls.h>
> -- 
> 2.43.0
> 
>
  
Sergey Bugaev Jan. 3, 2024, 9:08 p.m. UTC | #3
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
  
Sergey Bugaev Jan. 3, 2024, 9:24 p.m. UTC | #4
On Thu, Jan 4, 2024 at 12:08 AM Sergey Bugaev <bugaevc@gmail.com> wrote:
> 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.

Should the include <tls.h> be guarded by ifndef _ISOMAC perhaps? And
if we indeed move the inline functions to the internal hurd.h, they'd
also be guarded by ifndef _ISOMAC?

But really, why doesn't the test filter out the internal include directories?

Sergey
  
Samuel Thibault Jan. 3, 2024, 9:35 p.m. UTC | #5
Sergey Bugaev, le jeu. 04 janv. 2024 00:24:30 +0300, a ecrit:
> On Thu, Jan 4, 2024 at 12:08 AM Sergey Bugaev <bugaevc@gmail.com> wrote:
> > 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.
> 
> Should the include <tls.h> be guarded by ifndef _ISOMAC perhaps? And
> if we indeed move the inline functions to the internal hurd.h, they'd
> also be guarded by ifndef _ISOMAC?

Yes.

> But really, why doesn't the test filter out the internal include directories?

I'll leave that to the people who would know examples of needing
internal definitions.

Samuel
  

Patch

diff --git a/hurd/hurdsig.c b/hurd/hurdsig.c
index 3deb94f9..fe788193 100644
--- a/hurd/hurdsig.c
+++ b/hurd/hurdsig.c
@@ -15,6 +15,7 @@ 
    License along with the GNU C Library; if not, see
    <https://www.gnu.org/licenses/>.  */
 
+#include <assert.h>
 #include <stdio.h>
 #include <stdlib.h>
 #include <string.h>
@@ -260,7 +261,6 @@  _hurd_sigstate_actions (struct hurd_sigstate *ss)
 #include <hurd/msg_server.h>
 #include <hurd/msg_reply.h>	/* For __msg_sig_post_reply.  */
 #include <hurd/interrupt.h>
-#include <assert.h>
 #include <unistd.h>
 
 
diff --git a/mach/lowlevellock.h b/mach/lowlevellock.h
index 129a7bd9..c5bb0879 100644
--- a/mach/lowlevellock.h
+++ b/mach/lowlevellock.h
@@ -19,6 +19,7 @@ 
 #ifndef _MACH_LOWLEVELLOCK_H
 #define _MACH_LOWLEVELLOCK_H   1
 
+#include <mach.h>
 #include <mach/gnumach.h>
 #include <atomic.h>
 
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
diff --git a/sysdeps/mach/hurd/mig-reply.c b/sysdeps/mach/hurd/mig-reply.c
index 57bcd808..85c3af13 100644
--- a/sysdeps/mach/hurd/mig-reply.c
+++ b/sysdeps/mach/hurd/mig-reply.c
@@ -15,6 +15,7 @@ 
    License along with the GNU C Library; if not, see
    <https://www.gnu.org/licenses/>.  */
 
+#include <assert.h>
 #include <mach.h>
 #include <mach/mig_support.h>
 #include <tls.h>