[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <20250111084656.GH10433@brightrain.aerifal.cx>
Date: Sat, 11 Jan 2025 03:46:56 -0500
From: Rich Felker <dalias@...ifal.cx>
To: Abhisit Sangjan <abhisit.sangjan@...il.com>
Cc: musl@...ts.openwall.com
Subject: Re: [PATCH] cleanup unused include headers
Could you explain the methodology and motivation for this patch? As
submitted it's incorrect and breaking (see inline comments below) -
seems untested. Testing for such a patch would involved comparing .o
files before and after and checking that they're identical, since
presumably this patch is not intended to alter codegen.
Aside from breaking changes though, there are two non-functional
principles this patch seems to be violating:
1. All source files for public interfaces include the public header
for the interface. This is to ensure the compiler checks that the
implementatin signature matches the public signature. Many files will
build fine with the include removed, but then the checking is lost.
2. All files are expected to directly include the headaer declaring
any interfaces they need, not assume another header has indirectly
included it. For example pthread_impl.h included atomic.h probably for
historical reasons (I think at one point it had inline functions using
it), but this should be treated as an implementation detail, and we
should be able to remove that if it's no longer needed, without
breaking other files.
See further below:
On Fri, Jan 10, 2025 at 09:44:00PM +0700, Abhisit Sangjan wrote:
> diff --git a/compat/time32/adjtime32.c b/compat/time32/adjtime32.c
> index b0042c63..10b52779 100644
> --- a/compat/time32/adjtime32.c
> +++ b/compat/time32/adjtime32.c
> @@ -1,6 +1,5 @@
> #define _GNU_SOURCE
> #include "time32.h"
> -#include <time.h>
> #include <sys/time.h>
> #include <sys/timex.h>
These time32 changes may be ok for interfaces not declared in or using
time.h.
> diff --git a/crt/crt1.c b/crt/crt1.c
> index 10601215..d024df1a 100644
> --- a/crt/crt1.c
> +++ b/crt/crt1.c
> @@ -1,5 +1,4 @@
> #include <features.h>
> -#include "libc.h"
>
> #define START "_start"
>
> diff --git a/ldso/dlstart.c b/ldso/dlstart.c
> index 259f5e18..287e0bac 100644
> --- a/ldso/dlstart.c
> +++ b/ldso/dlstart.c
> @@ -1,6 +1,5 @@
> #include <stddef.h>
> #include "dynlink.h"
> -#include "libc.h"
>
> #ifndef START
> #define START "_dlstart"
These might be ok, worth checking. Some things were probably missed in
the last round of removing unnecessary libc.h inclusion.
> diff --git a/src/ctype/__ctype_get_mb_cur_max.c b/src/ctype/__ctype_get_mb_cur_max.c
> index 8e946fc1..4b20e81f 100644
> --- a/src/ctype/__ctype_get_mb_cur_max.c
> +++ b/src/ctype/__ctype_get_mb_cur_max.c
> @@ -1,5 +1,4 @@
> #include <stdlib.h>
> -#include "locale_impl.h"
>
> size_t __ctype_get_mb_cur_max()
> {
This was the first flagrantly breaking change I noticed. It converts
__ctype_get_mb_cur_max() into an infinitely recursive call to itself.
> diff --git a/src/linux/statx.c b/src/linux/statx.c
> index 4fb96e4b..540d2c0a 100644
> --- a/src/linux/statx.c
> +++ b/src/linux/statx.c
> @@ -1,6 +1,5 @@
> #define _GNU_SOURCE
> #include <sys/stat.h>
> -#include <string.h>
> #include <syscall.h>
> #include <sys/sysmacros.h>
> #include <errno.h>
This is probably right. Leftover from a removed memset.
> diff --git a/src/malloc/mallocng/malloc.c b/src/malloc/mallocng/malloc.c
> index d695ab8e..b5bc69b3 100644
> --- a/src/malloc/mallocng/malloc.c
> +++ b/src/malloc/mallocng/malloc.c
> @@ -1,6 +1,5 @@
> #include <stdlib.h>
> #include <stdint.h>
> -#include <limits.h>
> #include <string.h>
> #include <sys/mman.h>
> #include <errno.h>
This breaks use of malloc.c in contexts outside of musl with musl's
glue.h
I haven't reviewed the remainder in detail. If you'd like to pursue
this further, please submit a patch that doesn't make functional
changes and that follow the two principles above.
Rich
Powered by blists - more mailing lists
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
