On 1/31/22 13:48, Pádraig Brady wrote:
src/expr.c: In function 'single_binary_main_expr':
error: function might be candidate for attribute 'noreturn'
OK, thanks, running in single-binary found some other issues too, plus
one actual-albeit-minor memory leak in df that wasn't found with a
normal build presumably due to memory layout differences.
I installed the attached to fix the problems I found. The last patch
should fix the diagnostic you mentioned.
I've never really gotten the point of why commit messages should start
with 'maint:' vs 'shred:' vs whatever. If it were up to me, I'd remove
the requirement that commit messages start with those prefixes, as that
requirement consumes valuable space in the commit message's first line,
space that can often be put to better purposes. In the meantime I'll try
to remember your suggestion to use 'maint:' a lot, though I admit I
didn't quite get there with the attached patch batch.From 3a79d84f54e05cc15c152b9cf0e7a7ac4bfebf79 Mon Sep 17 00:00:00 2001
From: Paul Eggert <egg...@cs.ucla.edu>
Date: Mon, 31 Jan 2022 19:52:43 -0800
Subject: [PATCH 1/5] df: fix memory leak
* src/df.c (devlist_free): Remove.
(filter_mount_list): Free all of devlist, instead of merely
the entries in devlist_table.
---
src/df.c | 14 ++++----------
1 file changed, 4 insertions(+), 10 deletions(-)
diff --git a/src/df.c b/src/df.c
index 7d3207807..4b2cfb77a 100644
--- a/src/df.c
+++ b/src/df.c
@@ -710,12 +710,6 @@ devlist_for_dev (dev_t dev)
return found->seen_last;
}
-static void
-devlist_free (void *p)
-{
- free (p);
-}
-
/* Filter mount list by skipping duplicate entries.
In the case of duplicates - based on the device number - the mount entry
with a '/' in its me_devname (i.e., not pseudo name like tmpfs) wins.
@@ -736,9 +730,7 @@ filter_mount_list (bool devices_only)
mount_list_size++;
devlist_table = hash_initialize (mount_list_size, NULL,
- devlist_hash,
- devlist_compare,
- devlist_free);
+ devlist_hash, devlist_compare, NULL);
if (devlist_table == NULL)
xalloc_die ();
@@ -845,7 +837,9 @@ filter_mount_list (bool devices_only)
me = device_list->me;
me->me_next = mount_list;
mount_list = me;
- device_list = device_list->next;
+ struct devlist *next = device_list->next;
+ free (device_list);
+ device_list = next;
}
hash_free (devlist_table);
--
2.32.0
From 52d96e0919b3bfeb3a7b67b9dfe4ae58f07be61a Mon Sep 17 00:00:00 2001
From: Paul Eggert <egg...@cs.ucla.edu>
Date: Mon, 31 Jan 2022 19:55:54 -0800
Subject: [PATCH 2/5] maint: mark some _Noreturn functions
* src/basenc.c (finish_and_exit, do_encode, do_decode):
* src/comm.c (compare_files):
* src/tsort.c (tsort):
* src/uptime.c (uptime):
Mark with _Noreturn. Otherwise, unoptimized compilations may warn
that the calling renamed-main function doesn't return a value,
when !lint and when single-binary.
---
src/basenc.c | 6 +++---
src/comm.c | 2 +-
src/tsort.c | 2 +-
src/uptime.c | 2 +-
4 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/src/basenc.c b/src/basenc.c
index b37545929..04857d59e 100644
--- a/src/basenc.c
+++ b/src/basenc.c
@@ -949,7 +949,7 @@ wrap_write (char const *buffer, idx_t len,
}
}
-static void
+static _Noreturn void
finish_and_exit (FILE *in, char const *infile)
{
if (fclose (in) != 0)
@@ -963,7 +963,7 @@ finish_and_exit (FILE *in, char const *infile)
exit (EXIT_SUCCESS);
}
-static void
+static _Noreturn void
do_encode (FILE *in, char const *infile, FILE *out, idx_t wrap_column)
{
idx_t current_column = 0;
@@ -1007,7 +1007,7 @@ do_encode (FILE *in, char const *infile, FILE *out, idx_t wrap_column)
finish_and_exit (in, infile);
}
-static void
+static _Noreturn void
do_decode (FILE *in, char const *infile, FILE *out, bool ignore_garbage)
{
char *inbuf, *outbuf;
diff --git a/src/comm.c b/src/comm.c
index 9cb7a61b0..947463638 100644
--- a/src/comm.c
+++ b/src/comm.c
@@ -251,7 +251,7 @@ check_order (struct linebuffer const *prev,
merge them and output the result.
Exit the program when done. */
-static void
+static _Noreturn void
compare_files (char **infiles)
{
/* For each file, we have four linebuffers in lba. */
diff --git a/src/tsort.c b/src/tsort.c
index 19b991bed..383c7a083 100644
--- a/src/tsort.c
+++ b/src/tsort.c
@@ -428,7 +428,7 @@ walk_tree (struct item *root, bool (*action) (struct item *))
/* Do a topological sort on FILE. Exit with appropriate exit status. */
-static void
+static _Noreturn void
tsort (char const *file)
{
bool ok = true;
diff --git a/src/uptime.c b/src/uptime.c
index f1cb84a6b..1adacaada 100644
--- a/src/uptime.c
+++ b/src/uptime.c
@@ -173,7 +173,7 @@ print_uptime (size_t n, const STRUCT_UTMP *this)
according to utmp file FILENAME. Use read_utmp OPTIONS to read the
utmp file. */
-static void
+static _Noreturn void
uptime (char const *filename, int options)
{
size_t n_users;
--
2.32.0
From c5ba288562b866b2814e0c16147f9dbbd358346f Mon Sep 17 00:00:00 2001
From: Paul Eggert <egg...@cs.ucla.edu>
Date: Mon, 31 Jan 2022 19:57:14 -0800
Subject: [PATCH 3/5] chgrp: fix typo in previous change
* src/chgrp.c (main): Use main_exit, not exit.
---
src/chgrp.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/chgrp.c b/src/chgrp.c
index 0c0f62949..31a6136cc 100644
--- a/src/chgrp.c
+++ b/src/chgrp.c
@@ -313,5 +313,5 @@ main (int argc, char **argv)
(uid_t) -1, gid,
(uid_t) -1, (gid_t) -1, &chopt);
- exit (ok ? EXIT_SUCCESS : EXIT_FAILURE);
+ main_exit (ok ? EXIT_SUCCESS : EXIT_FAILURE);
}
--
2.32.0
From 5bbd4ea306af48509d676d6c00fc5d771a202c81 Mon Sep 17 00:00:00 2001
From: Paul Eggert <egg...@cs.ucla.edu>
Date: Mon, 31 Jan 2022 19:58:15 -0800
Subject: [PATCH 4/5] tr: pacify -fsanitizer=leak
* src/tr.c (main): Use main_exit, not return, in a couple of
places missed last time.
---
src/tr.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/tr.c b/src/tr.c
index f291d1b8e..16dff94a6 100644
--- a/src/tr.c
+++ b/src/tr.c
@@ -1778,13 +1778,13 @@ main (int argc, char **argv)
spec_init (s1);
if (!parse_str (argv[optind], s1))
- return EXIT_FAILURE;
+ main_exit (EXIT_FAILURE);
if (non_option_args == 2)
{
spec_init (s2);
if (!parse_str (argv[optind + 1], s2))
- return EXIT_FAILURE;
+ main_exit (EXIT_FAILURE);
}
else
s2 = NULL;
--
2.32.0
From 55851c56e373356622fe1e063d8230d6d9acaad9 Mon Sep 17 00:00:00 2001
From: Paul Eggert <egg...@cs.ucla.edu>
Date: Mon, 31 Jan 2022 22:08:56 -0800
Subject: [PATCH 5/5] maint: suppress bogus noreturn warnings
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
* configure.ac: Move the single-binary code before the
gcc-warnings code, so that the latter can depend on the former.
Suppress -Wsuggest-attribute=noreturn with single binaries,
to avoid diagnostics like the following:
src/expr.c: In function 'single_binary_main_expr':
error: function might be candidate for attribute 'noreturn'
Problem reported by Pádraig Brady in:
https://lists.gnu.org/r/coreutils/2022-01/msg00061.html
---
configure.ac | 66 ++++++++++++++++++++++++++++------------------------
1 file changed, 36 insertions(+), 30 deletions(-)
diff --git a/configure.ac b/configure.ac
index bfbc68cf7..453baccff 100644
--- a/configure.ac
+++ b/configure.ac
@@ -84,6 +84,36 @@ AC_DEFUN([gl_GCC_VERSION_IFELSE],
]
)
+AC_ARG_ENABLE([single-binary],
+ [AS_HELP_STRING([--enable-single-binary=[shebangs|symlinks]],
+ [Compile all the tools in a single binary, reducing the overall size.
+ When compiled this way, shebangs (default when enabled) or symlinks are
+ installed for each tool that points to the single binary.])],
+ [gl_single_binary=no ;
+ case $enableval in
+ yes) gl_single_binary=shebangs ;;
+ no|shebangs|symlinks) gl_single_binary=$enableval ;;
+ *) AC_MSG_ERROR([bad value $enableval for single-binary option.
+ Options are: symlinks, shebangs, no.]) ;;
+ esac],
+ [gl_single_binary=no]
+)
+AC_ARG_ENABLE([single-binary-exceptions],
+ [AS_HELP_STRING([--enable-single-binary-exceptions=PROG_LIST],
+ [When used with --enable-single-binary, exclude the PROG_LIST from
+ it, so these programs are compiled as separated files
+ (comma-separated, default none))])],
+ [gl_single_binary_exceptions=$enableval],
+ [gl_single_binary_exceptions=]
+)
+if test "$gl_single_binary" = 'symlinks'; then
+ if ! test "`echo ls | sed \"$program_transform_name\"`" = 'ls'; then
+ AC_MSG_ERROR([program name transformations are not currently supported
+ with --enable-single-binary=symlinks.])
+ fi
+fi
+AM_CONDITIONAL([SINGLE_BINARY], [test "$gl_single_binary" != no])
+
AC_ARG_ENABLE([gcc-warnings],
[AS_HELP_STRING([--enable-gcc-warnings@<:@=TYPE@:>@],
[control generation of GCC warnings. The TYPE 'no' disables
@@ -148,6 +178,12 @@ if test $gl_gcc_warnings != no; then
nw="$nw -Winline" # system.h's readdir_ignoring_dot_and_dotdot
nw="$nw -Wvector-operation-performance" # warns about randperm.c
+ # Suppress noreturn warnings with single binaries; otherwise
+ # GCC complains about the renamed 'main' not being declared noreturn
+ # because 'main_exit' calls 'exit' when linting.
+ if test "$gl_single_binary" != no; then
+ nw="$nw -Wsuggest-attribute=noreturn"
+ fi
# Using -Wstrict-overflow is a pain, but the alternative is worse.
# For an example, see the code that provoked this report:
@@ -226,36 +262,6 @@ if test $gl_gcc_warnings != no; then
AC_SUBST([GNULIB_TEST_WARN_CFLAGS])
fi
-AC_ARG_ENABLE([single-binary],
- [AS_HELP_STRING([--enable-single-binary=[shebangs|symlinks]],
- [Compile all the tools in a single binary, reducing the overall size.
- When compiled this way, shebangs (default when enabled) or symlinks are
- installed for each tool that points to the single binary.])],
- [gl_single_binary=no ;
- case $enableval in
- yes) gl_single_binary=shebangs ;;
- no|shebangs|symlinks) gl_single_binary=$enableval ;;
- *) AC_MSG_ERROR([bad value $enableval for single-binary option.
- Options are: symlinks, shebangs, no.]) ;;
- esac],
- [gl_single_binary=no]
-)
-AC_ARG_ENABLE([single-binary-exceptions],
- [AS_HELP_STRING([--enable-single-binary-exceptions=PROG_LIST],
- [When used with --enable-single-binary, exclude the PROG_LIST from
- it, so these programs are compiled as separated files
- (comma-separated, default none))])],
- [gl_single_binary_exceptions=$enableval],
- [gl_single_binary_exceptions=]
-)
-if test "$gl_single_binary" = 'symlinks'; then
- if ! test "`echo ls | sed \"$program_transform_name\"`" = 'ls'; then
- AC_MSG_ERROR([program name transformations are not currently supported
- with --enable-single-binary=symlinks.])
- fi
-fi
-AM_CONDITIONAL([SINGLE_BINARY], [test "$gl_single_binary" != no])
-
AC_FUNC_FORK
optional_bin_progs=
--
2.32.0
