Re: [RFC PATCH] Add -Werror flag when building GNU/HURD.

Hello,

Thanks for the curation work!

Mikhail Karpov, le mar. 28 juil. 2026 17:31:23 +0700, a ecrit:
> When compiling GNU/Hurd, quite a few warnings were generated. I fixed some
> of
> them, and now it compiles with flags:
> CFLAGS += -std=gnu99 $(gnu89-inline-CFLAGS) -Wall -Werror \
>                     -Werror=old-style-definition \
>                     -Werror=implicit-function-declaration \
>                     -Werror=strict-prototypes \
>                     -g -O3 -fno-strict-aliasing $($*-CFLAGS)
> Completes successfully.
> 
> I have a few questions:
> 1. Is this flag even necessary?

Adding a complete -Werror would pose problem to people. When you are
writing code, you leave temporary variables and whatnot that would
produce warnings, you don't want to be hindered by that.

At best we'd want to have -Werror in a version release process, or in a
CI, to avoid warning regressions.

> 2. Should I remove the -fno-strict-aliasing flag?

Ideally yes but there be dragons. We'd first want to make *sure* that
the existing code doesn't make any type punning.

> 3. Should I add the -Wextra flag?

Ideally we'd fix -Wextra warnings too indeed.

> Now on to the patches.  I'm not sure about some of them, and they
> probably require more time to study. For example, in pfinet/linux-src

In general, we don't want to bother fixing pfinet/linux-src things if
we don't really need to. The long-term plan is to use lwip, so it's not
useful to maintain the old pfinet code.

If we are to add -Werror in some CI etc., we'd add -Wno-error to pfinet.

> diff --git a/libdiskfs/diskfs.h b/libdiskfs/diskfs.h
> index d95e1b80..b8316d96 100644
> --- a/libdiskfs/diskfs.h
> +++ b/libdiskfs/diskfs.h
> @@ -890,7 +890,7 @@ diskfs_disknode_node (struct disknode *disknode)
>     This function is a wrapper for diskfs_lookup_hard.
>  */
>  error_t diskfs_lookup (struct node *dp,
> -		       char *name, lookup_flags_t l_flags,
> +		       const char *name, lookup_flags_t l_flags,

It's not that simple because some current code somewhere does modify
name. See the comment:

“
NAME will be modified in place if there are slashes in it; it is
therefore an error to specify a constant NAME which contains slashes.
”

> diff --git a/libstore/device.c b/libstore/device.c
> index 56f3d93d..15f346e4 100644
> --- a/libstore/device.c
> +++ b/libstore/device.c
> @@ -128,7 +128,7 @@ dopen (const char *name, device_t *device, int *mod_flags)
>    char *rest;
>  
>    /* Parse @master:/dev/hello */
> -  if ( (name[0] == '@') && (pos = strchr (name, ':')) )
> +  if ( (name[0] == '@') && (pos = (char *) strchr (name, ':')) )

Better make pos and rest const char *.

> From 251f2a8b5505727f720adfb27b0b7912f72efecc Mon Sep 17 00:00:00 2001
> From: Mikhail Karpov <[email protected]>
> Date: Tue, 28 Jul 2026 13:01:15 +0700
> Subject: [PATCH 3/3] Fixing -Werror
> 
> ---
>  boot/userland-boot.c                      | 2 +-
>  fatfs/main.c                              | 2 +-
>  libbpf/bpf_impl.h                         | 2 --
>  pfinet/glue-include/asm/uaccess.h         | 1 -
>  pfinet/linux-src/include/linux/in_route.h | 4 ----
>  pfinet/linux-src/include/net/flow.h       | 2 ++
>  6 files changed, 4 insertions(+), 9 deletions(-)
> 
> diff --git a/boot/userland-boot.c b/boot/userland-boot.c
> index 1739eb91..2c92f6bd 100644
> --- a/boot/userland-boot.c
> +++ b/boot/userland-boot.c
> @@ -212,7 +212,7 @@ load_image (task_t t,
>  
>  	    buf = (vm_address_t) mmap (0, bufsz,
>  				       PROT_READ|PROT_WRITE, MAP_ANON, 0, 0);
> -	    assert_backtrace (buf != MAP_FAILED);
> +	    assert_backtrace ((void *) buf != MAP_FAILED);

Rather than back-casting, better add a void* variable that receives the
result of mmap(), use that for the assert, and cast to put into buf.

> diff --git a/fatfs/main.c b/fatfs/main.c
> index 8f44ed8b..ab5db03e 100644
> --- a/fatfs/main.c
> +++ b/fatfs/main.c
> @@ -235,7 +235,7 @@ main (int argc, char **argv)
>  
>    zerocluster = (vm_address_t) mmap (0, bytes_per_cluster, PROT_READ|PROT_WRITE,
>  				     MAP_ANON, 0, 0);
> -  assert_backtrace (zerocluster != MAP_FAILED);
> +  assert_backtrace ((void *) zerocluster != MAP_FAILED);

Ditto.

> diff --git a/libbpf/bpf_impl.h b/libbpf/bpf_impl.h
> index fb0fb490..d80e0058 100644
> --- a/libbpf/bpf_impl.h
> +++ b/libbpf/bpf_impl.h
> @@ -51,8 +51,6 @@ typedef struct
>  typedef	unsigned short	filter_t;
>  typedef filter_t	*filter_array_t;
>  
> -#define	NET_MAX_FILTER		512 /* bpf programs are big */
> -
>  #define NET_HASH_SIZE   256
>  #define N_NET_HASH      4
>  #define N_NET_HASH_KEYS 4

We don't really want to be taking a value from elsewhere: we have no
reason to follow somebody else, we can just pick up the value we prefer
here. See 6cb04484b1 which increased the value, as requested by somebody
on the list (or IRC, can't remember).

So better rename it to avoid the clash.

> From 9f6c94f90f985163806842f41179c766ace15f2f Mon Sep 17 00:00:00 2001
> From: Mikhail Karpov <[email protected]>
> Date: Tue, 28 Jul 2026 13:43:15 +0700
> Subject: [PATCH 4/4] Fixing-Werror=shift-overflow
> 
> ---
>  libps/ps.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/libps/ps.h b/libps/ps.h
> index 3e59c66a..ad0aab77 100644
> --- a/libps/ps.h
> +++ b/libps/ps.h
> @@ -356,7 +356,7 @@ struct proc_stat
>  #define PSTAT_NO_MSGPORT     0x1000000 /* Don't use the msgport at all */
>  
>  /* Bits from PSTAT_USER_BASE on up are available for user-use.  */
> -#define PSTAT_USER_BASE      0x20000000
> +#define PSTAT_USER_BASE      0x20000000ULL
>  #define PSTAT_USER_MASK      ~(PSTAT_USER_BASE - 1)

No, it's not that simple, that won't solve the actual problem:
W_PSTAT_LOGIN is getting beyond 32b, and will be truncated anyway.

We don't want to hide this warning, we want to see it so that somebody
does actually take the time to do something about it instead of hiding
the issue away.

> diff --git a/console-client/ncursesw.c b/console-client/ncursesw.c
> index 116f86cc..c556aba6 100644
> --- a/console-client/ncursesw.c
> +++ b/console-client/ncursesw.c
> @@ -453,7 +453,9 @@ mvwputsn (conchar_t *str, size_t len, off_t x, off_t y)
>    wmove (conspad, y, x);
>    while (len)
>      {
> +#if 0
>        int ret;
> +#endif
>        chtype ac;
>  
>        if (last_attr != *(uint32_t *) &str->attr)
> @@ -468,23 +470,28 @@ mvwputsn (conchar_t *str, size_t len, off_t x, off_t y)
>        else
>  	{      
>  	  wch[0] = str->chr;
> -	  ret = setcchar (&chr, wch, attr, color_pair, NULL);
>  #if 0
> +	  ret = setcchar (&chr, wch, attr, color_pair, NULL);
>  	  if (ret == ERR)
>  	    {
>  	      printf ("setcchar failed: %s\n", strerror (errno));
>  	      printf ("[%lc]\n", wch[0]);
>  	      assert_backtrace (!"Do something if setcchar fails.");
>  	    }
> +#else
> +	  setcchar (&chr, wch, attr, color_pair, NULL);
>  #endif
> -	  ret = wadd_wch (conspad, &chr);
> +
>  #if 0
> +	  ret = wadd_wch (conspad, &chr);
>  	  if (ret == ERR)
>  	    {
>  	      printf ("add_wch failed: %i, %s\n", ret, strerror (errno));
>  	      printf ("[%lc]\n", wch[0]);
>  	      assert_backtrace (!"Do something if add_wchr fails.");
>  	    }
> +#else
> +	  wadd_wch (conspad, &chr);
>  #endif

This is duplicating code... Better put a (void) ret; to express that we
ignore the value on purpose.

> --- rpc_msg.h	2026-07-28 13:12:44.249567980 +0700
> +++ rpc_msg_new.h	2026-07-28 13:13:37.564304500 +0700
> @@ -165,6 +165,11 @@
>  extern "C" {
>  #endif
>  /*
> + * The rcp error
> + */
> +struct rpc_err;
> +
> +/*
>   * XDR routine to handle a rpc message.
>   * xdr_callmsg(xdrs, cmsg)
>   * 	XDR *xdrs;

This file is not ours. Better fix our usage: we should be including
<rpc/clnt.h> to get the structure definition before including
<rpc/rpc_msg.h> (similarly to having to include <sys/types.h> first on
some operating systems).

-- 
With regards,
Samuel