Re: endian.h

[Bruno Haible]

Collin Funk wrote:
> Does glibc take bug reports for that or is it waiting for the
> specification to be official (i.e. not a draft)?

glibc often implements new functionalities from the standards ahead
of time. (Look at Joseph Myers' commits in glibc.) The reasoning is
the same as the one I gave in [1].

Bruno

[1] https://lists.gnu.org/archive/html/bug-gnulib/2024-05/msg00081.html

Re: byteswap.h behavior

[Collin Funk]

On 5/12/24 1:30 PM, Paul Eggert wrote:
> I sense a bit of confusion here. Although POSIX allows  symbols 
> like be16toh to be macros, I don't see where it allows be16toh(X) to evaluate 
> X more than once, so an expression like be16toh(i++) has well-defined 
> behavior even though it has a side effect.

Ah, okay that makes sense thanks. I didn't realize that it would be
written explicitly I guess.

I started working on the module a bit using the POSIX specification.
When writing the test module I noticed that glibc doesn't define
uint16_t, uint32_t, and uint64_t in endian.h.

Does glibc take bug reports for that or is it waiting for the
specification to be official (i.e. not a draft)? I should probably
request a bugzilla account anyways so I can report bugs...

Collin

Re: [PATCH] announce-gen: Mention git commit in release announcement.

[Paul Eggert]

On 2024-05-12 12:50, Bruno Haible wrote:


Your real goal, which is — AFAIU — to allow distros to discard part
or all of the tarball and to redo the packaging work done by the release
manager in a different way, would be better served with a
   1) machine-readable file,
   2) somewhere in or in the vicinity of the tarball.


Although that's a good goal to have (see below), it's reasonably 
ambitious. For example, the tool lists we've been mentioning are 
incomplete, partly due to indirect dependencies (no glibc?) and partly 
not (no coreutils?) and it will be a pain to complete them (no firmware 
IDs? no motherboard or CPU IDs? you get the picture...). So I suspect 
that having an intermediate goal could be worthwhile.







In other words, if you really want to go this way — against the advice that
I gave you above —, better replace the two lines with:

   This release is based on the inetutils git, available at
 https://git.savannah.gnu.org/git/inetutils.git
   commit 524d4b6934db12b9f43be410d2f201fdb40cfc97, also labelled v1.42.


This is good advice. It's similar to wording I use in announcements of 
TZDB distributions (e.g., 
).


For what it's worth, those TZDB announcements have always intended to be 
human readable but various downstream projects parse the text and grab 
checksums and signatures and whatnot. The pull of automation is indeed 
strong, and your advice about going for automation is good.

Re: byteswap.h behavior

[Paul Eggert]

On 2024-05-12 12:47, Collin Funk wrote:

Yeah, I read the POSIX draft and it says that they may be macros. I
doubt the byteswap.h and endian.h functions are used with non-constant
expression arguments that often.


I sense a bit of confusion here. Although POSIX allows  
symbols like be16toh to be macros, I don't see where it allows 
be16toh(X) to evaluate X more than once, so an expression like 
be16toh(i++) has well-defined behavior even though it has a side effect.


A few function-like macros, like getc, are explicitly allowed to 
evaluate their arguments more than once, but ordinarily function-like 
macros are supposed to act like their corresponding functions.

Re: byteswap.h behavior

[Collin Funk]

On 5/12/24 1:02 PM, Bruno Haible wrote:
> The simple fix to your worry is: Let the user use '-Wall' routinely.
> gcc has a warning option '-Wsequence-point', included in '-Wall'.
> clang has a warning option '-Wunsequenced', included in '-Wall'.
> The do the job, and I have not seen them produce false warnings ever.
> 
> $ gcc -Wall foo.c
> foo.c: In function ‘main’:
> foo.c:15:59: warning: operation on ‘value_macro’ may be undefined 
> [-Wsequence-point]
>15 |   printf ("2. %#" PRIX32 "\n", bswap_32_macro (value_macro++));
>   |   ^

Oh, I completely forgot about this warning. It is very rare that I
write something that angers it. That alleviates my worry since those
messages are hard to miss. Thanks.

Collin

Re: byteswap.h behavior

[Bruno Haible]

Collin Funk wrote:
> I worry though that in some cases programs will be accustomed to
> glibc's behavior. Since all of the functions are just macros to static
> inline functions in  arguments will only be evaluated
> once. It seems like it could be the cause of some unexpected bugs...

The simple fix to your worry is: Let the user use '-Wall' routinely.
gcc has a warning option '-Wsequence-point', included in '-Wall'.
clang has a warning option '-Wunsequenced', included in '-Wall'.
The do the job, and I have not seen them produce false warnings ever.

$ gcc -Wall foo.c
foo.c: In function ‘main’:
foo.c:15:59: warning: operation on ‘value_macro’ may be undefined 
[-Wsequence-point]
   15 |   printf ("2. %#" PRIX32 "\n", bswap_32_macro (value_macro++));
  |   ^
foo.c:7:31: note: in definition of macro ‘bswap_32_macro’
7 |(((x) & 0x00FF) >> 8) |  \
  |   ^
foo.c:15:59: warning: operation on ‘value_macro’ may be undefined 
[-Wsequence-point]
   15 |   printf ("2. %#" PRIX32 "\n", bswap_32_macro (value_macro++));
  |   ^
foo.c:7:31: note: in definition of macro ‘bswap_32_macro’
7 |(((x) & 0x00FF) >> 8) |  \
  |   ^
foo.c:15:59: warning: operation on ‘value_macro’ may be undefined 
[-Wsequence-point]
   15 |   printf ("2. %#" PRIX32 "\n", bswap_32_macro (value_macro++));
  |   ^
foo.c:7:31: note: in definition of macro ‘bswap_32_macro’
7 |(((x) & 0x00FF) >> 8) |  \
  |   ^

$ clang -Wall foo.c
foo.c:15:59: warning: multiple unsequenced modifications to 'value_macro' 
[-Wunsequenced]
   15 |   printf ("2. %#" PRIX32 "\n", bswap_32_macro (value_macro++));
  |   ^~
foo.c:5:31: note: expanded from macro 'bswap_32_macro'
5 | #define bswap_32_macro(x) x) & 0x00FF) << 24) | \
  |   ^
6 |(((x) & 0xFF00) << 8) |  \
  |   ~
1 warning generated.

Assume this warning option is used. Then you can leave byteswap.h as it is.

Bruno

Re: [PATCH] announce-gen: Mention git commit in release announcement.

[Bruno Haible]

Hi Simon,

> including the commit hash provide some additional information that may
> be useful further down the line

That is a bit weak as a rationale. There are many additional informations
that "may be useful" to include. Isn't this move part of a bigger plan of
yours, which is to decompose release tarballs [1][2][3]?

The problem I see with your patch is that it goes into a wrong direction.
In [1], I stated my objection: The release announcement is the wrong place
to add such information. You replied "It is better than nothing, though."
No, it's not better than nothing. It increases the future costs of moving
the ship into the right direction. The longer one has been sailing in the
wrong direction, the more costly it becomes to sail to the real goal.

More precisely, the release announcement is
  1) written for humans, not meant to be machine-readable,
  2) published on a different channel than tarballs.
Your real goal, which is — AFAIU — to allow distros to discard part
or all of the tarball and to redo the packaging work done by the release
manager in a different way, would be better served with a
  1) machine-readable file,
  2) somewhere in or in the vicinity of the tarball.

That is, it would be better if you design a new file format (*), containing

  vcs:
type: git
url: https://git.savannah.gnu.org/git/libidn.git
commit: d0eb2b5a596f8bca26b0b2ccf0c70311b9819e6f
label: v1.42
  build-tools:
- gnulib
type: git
url: https://git.savannah.gnu.org/git/gnulib.git
commit: aacceb6eff58eba91290d930ea9b8275699057cf
- autoconf
type: release
distributor: gnu.org
version: 2.71
- automake
type: release
distributor: gnu.org
version: 1.16.5
- bison
type: release
distributor: gnu.org
version: 3.8.2
- m4
type: release
distributor: gnu.org
version: 1.4.18
- makeinfo
type: release
distributor: gnu.org
version: 6.8
- help2man
type: release
distributor: gnu.org
version: 1.49.1
- make
type: release
distributor: gnu.org
version: 4.3
- gzip
type: release
distributor: gnu.org
version: 1.10
- tar
type: release
distributor: gnu.org
version: 1.34

Then call that file format the "metainfo" of the release, and either
  - include a .tarball-metainfo in the tarball itself. (Some packages
already have a .tarball-version.)
or
  - formulate a convention that this file be available on the same ftp
server, under the name PACKAGE.metainfo, next to PACKAGE.tar.gz.

(*) My example uses YAML syntax, but JSON or XML would be just as good.

> and turn that into this:
> 
> This release was built bootstrapped with the following tools
> using inetutils git commit 524d4b6934db12b9f43be410d2f201fdb40cfc97:
> 
>   Gnulib aacceb6eff
>   Autoconf 2.71
>   Automake 1.16.5
>   Bison 3.8.2
>   M4 1.4.18
>   Makeinfo 6.8
>   Help2man 1.49.1
>   Make 4.3
>   Gzip 1.10
>   Tar 1.34
> 
> Does this make sense?

As a naïve user, I would have two thoughts:

  * Why is he telling me a git commit this way? Has he not heard of
"git tag" and "git push --tags"?

  * Why do you make my brain jump from the tools to the commit id and then
back again to the tools?

In other words, if you really want to go this way — against the advice that
I gave you above —, better replace the two lines with:

  This release is based on the inetutils git, available at
https://git.savannah.gnu.org/git/inetutils.git
  commit 524d4b6934db12b9f43be410d2f201fdb40cfc97, also labelled v1.42.
  The release tarball was built using the following tools:

Bruno

[1] https://lists.gnu.org/archive/html/bug-gnulib/2024-04/msg00242.html
[2] https://lists.gnu.org/archive/html/bug-gnulib/2024-04/msg00431.html
[3] https://lists.gnu.org/archive/html/bug-gnulib/2024-05/msg00124.html

Re: byteswap.h behavior

[Jeffrey Walton]

On Sun, May 12, 2024 at 3:23 PM Collin Funk  wrote:

> A few months ago there was a suggestion on emacs-devel to use
> __builtin_bswap functions when available [1]. While I agree that GCC
> can deal with the optimizations properly, I noted an important
> difference between the macros in byteswap.h.in and inline functions
> provided by glibc.
>
> Using this test program to compare the real glibc header to a macro
> copy-pasted from the replacement header:
>
> ==
> #define _GNU_SOURCE 1
> #include 
> #include 
> #include 
> #define bswap_32_macro(x) x) & 0x00FF) << 24) | \
>(((x) & 0xFF00) << 8) |  \
>(((x) & 0x00FF) >> 8) |  \
>(((x) & 0xFF00) >> 24))
> int
> main (void)
> {
>   uint32_t value = 0x12345678;
>   uint32_t value_macro = 0x12345678;
>   printf ("1. %#" PRIX32 "\n", bswap_32 (value++));
>   printf ("2. %#" PRIX32 "\n", bswap_32_macro (value_macro++));
>   printf ("3. %#" PRIX32 "\n", value);
>   printf ("4. %#" PRIX32 "\n", value_macro);
>   return 0;
> }
> ==
>
> We get the output:
>
> $ ./a.out
> 1. 0X78563412
> 2. 0X78563412
> 3. 0X12345679
> 4. 0X1234567C
>
> I would like to deal with this concern before I implement the
> replacement for . I think the best decision is to use
> 'extern inline' to match the behavior of glibc. Any other opinions on
> this?
>
> Also if we can agree upon making sure these are defined as functions,
> what is the proper way to test it in a configure script? My instinct
> tells me that assigning a function pointer to bswap_16, etc. would
> fail if they are macros but I am not sure the "standard" way of
> performing that check.
>
> Collin
>
> [1] https://lists.gnu.org/archive/html/emacs-devel/2024-03/msg00736.html
>

I think extern is the least of your worries. Pre- and post- increment are
not safe in a macro. Maybe you should make a local copy of the value in a
do {} while, and then operate on the local value. Or make it a function.

Jeff

Re: byteswap.h behavior

[Collin Funk]

On 5/12/24 12:38 PM, Paul Eggert wrote:
>> Also if we can agree upon making sure these are defined as functions,
>> what is the proper way to test it in a configure script? My instinct
>> tells me that assigning a function pointer to bswap_16, etc. would
>> fail if they are macros
> 
> I don't see the need to check that they are functions. POSIX does not require 
> endian.h symbols like be16toh to be defined as functions, and the glibc 
> manual doesn't mention endian.h, so it sounds like Gnulib-using code 
> shouldn't assume that these symbols are functions.

Yeah, I read the POSIX draft and it says that they may be macros. I
doubt the byteswap.h and endian.h functions are used with non-constant
expression arguments that often.

I worry though that in some cases programs will be accustomed to
glibc's behavior. Since all of the functions are just macros to static
inline functions in  arguments will only be evaluated
once. It seems like it could be the cause of some unexpected bugs...

Collin

Re: byteswap.h behavior

[Paul Eggert]

On 2024-05-12 12:23, Collin Funk wrote:

I think the best decision is to use
'extern inline' to match the behavior of glibc.


Yes, with the usual _GL_EXTERN business.



Also if we can agree upon making sure these are defined as functions,
what is the proper way to test it in a configure script? My instinct
tells me that assigning a function pointer to bswap_16, etc. would
fail if they are macros


I don't see the need to check that they are functions. POSIX does not 
require endian.h symbols like be16toh to be defined as functions, and 
the glibc manual doesn't mention endian.h, so it sounds like 
Gnulib-using code shouldn't assume that these symbols are functions.

Re: [PATCH 1/4] stdbit: new module

[Paul Eggert]

On 2024-05-12 11:43, Collin Funk wrote:

I have similar feelings about some stuff in gnulib-tool.py. We have
lines like this:

#
# Define GLImport class
#
class GLImport:


+1 on that. It's too close to the classic:

i = i + 1;  /* Add 1 to i.  */

byteswap.h behavior

[Collin Funk]

A few months ago there was a suggestion on emacs-devel to use
__builtin_bswap functions when available [1]. While I agree that GCC
can deal with the optimizations properly, I noted an important
difference between the macros in byteswap.h.in and inline functions
provided by glibc.

Using this test program to compare the real glibc header to a macro
copy-pasted from the replacement header:

==
#define _GNU_SOURCE 1
#include 
#include 
#include 
#define bswap_32_macro(x) x) & 0x00FF) << 24) | \
   (((x) & 0xFF00) << 8) |  \
   (((x) & 0x00FF) >> 8) |  \
   (((x) & 0xFF00) >> 24))
int
main (void)
{
  uint32_t value = 0x12345678;
  uint32_t value_macro = 0x12345678;
  printf ("1. %#" PRIX32 "\n", bswap_32 (value++));
  printf ("2. %#" PRIX32 "\n", bswap_32_macro (value_macro++));
  printf ("3. %#" PRIX32 "\n", value);
  printf ("4. %#" PRIX32 "\n", value_macro);
  return 0;
}
==

We get the output:

$ ./a.out 
1. 0X78563412
2. 0X78563412
3. 0X12345679
4. 0X1234567C

I would like to deal with this concern before I implement the
replacement for . I think the best decision is to use
'extern inline' to match the behavior of glibc. Any other opinions on
this?

Also if we can agree upon making sure these are defined as functions,
what is the proper way to test it in a configure script? My instinct
tells me that assigning a function pointer to bswap_16, etc. would
fail if they are macros but I am not sure the "standard" way of
performing that check.

Collin

[1] https://lists.gnu.org/archive/html/emacs-devel/2024-03/msg00736.html

Re: execinfo: Document known bugs

[Collin Funk]

Hi Bruno,

On 5/12/24 7:09 AM, Bruno Haible wrote:
> Now that we have unit tests for the backtrace* functions, let's see what
> they produce:

Nice work!

> Collin, feel free to report the FreeBSD, NetBSD, OpenBSD bugs if you have
> time for that. It's only by reporting bugs that we can trigger that they get
> fixed.

I would like too but sadly don't get around to testing much. My main
system has GNU/Linux always and occasionally I use the latest FreeBSD
release on another machine.

Collin

Re: [PATCH 1/4] stdbit: new module

[Collin Funk]

On 5/12/24 8:23 AM, Paul Eggert wrote:
> Is this stuff documented somewhere in the .texi files? If not, I suppose it 
> should be.

I'm not sure. The only reason I noticed was because I was working on
endian.h and using stdbit as a template since my m4 talents are lacking...

> Frankly I continue to be annoyed by having to read and write a line "# 
> FILENAME.m4" at the start of every m4 file FILENAME.m4. It's better for a 
> file's first line to be a human-readable comment explaining what the file is 
> for. Any automated procedure already knows the file name, so why do we need 
> to put the name there manually, an error-prone process?

I have similar feelings about some stuff in gnulib-tool.py. We have
lines like this:

#
# Define GLImport class
#
class GLImport:
  

But when searching for a class in Python you can simply look up
'^class NAME:' since whitespace matters in that language. No need for
the comment in my opinion.

Collin

Re: [PATCH] announce-gen: Mention git commit in release announcement.

[Paul Eggert]

On 2024-05-12 09:27, Jim Meyering wrote:

I like it. Wording and placement are spot on.


Looks good to me too.

I at first thought to suggest adding announce-gen's own version number 
to the announcement (to help people who want to check the announcement 
itself), but there's no need as it's derivable from Gnulib's version number.

Re: [PATCH] announce-gen: Mention git commit in release announcement.

[Jim Meyering]

I like it. Wording and placement are spot on.

Thanks.

On Sun, May 12, 2024, 08:42 Simon Josefsson via Gnulib discussion list <
bug-gnulib@gnu.org> wrote:

> All,
>
> Our release announcements does not mention the git commit hash that was
> used to prepare the release.  While SHA1 is broken, I still think
> including the commit hash provide some additional information that may
> be useful further down the line, and hopefully including doesn't incur
> too much cognitive load on the reader (that isn't already present..).
>
> I haven't pushed the attached patch since I'm not a native speaker.
> Could someone suggest better wording, if needed?  Or better placement in
> the announcement?
>
> To read the result of the patch in context, take some earlier
> announcement:
>
> https://lists.gnu.org/archive/html/info-gnu/2024-03/msg6.html
>
> and then consider that the patch would turn the following snippet (for a
> hypothethical upcoming GNU inetutils release) text:
>
> This release was bootstrapped with the following tools:
>   Gnulib aacceb6eff
>   Autoconf 2.71
>   Automake 1.16.5
>   Bison 3.8.2
>   M4 1.4.18
>   Makeinfo 6.8
>   Help2man 1.49.1
>   Make 4.3
>   Gzip 1.10
>   Tar 1.34
>
> and turn that into this:
>
> This release was built bootstrapped with the following tools
> using inetutils git commit 524d4b6934db12b9f43be410d2f201fdb40cfc97:
>
>   Gnulib aacceb6eff
>   Autoconf 2.71
>   Automake 1.16.5
>   Bison 3.8.2
>   M4 1.4.18
>   Makeinfo 6.8
>   Help2man 1.49.1
>   Make 4.3
>   Gzip 1.10
>   Tar 1.34
>
> Does this make sense?  Is the location in the announcement e-mail a good
> one?  This hides it a bit further down which I think makes sense.  Few
> readers care about git commit and bootstrapping versions, and the
> information is related.  The new version adds an empty line which I
> think is more consistent with the other paragraphs.
>
> Thoughts?
>
> /Simon
>

[PATCH] announce-gen: Mention git commit in release announcement.

[Simon Josefsson via Gnulib discussion list]

All,

Our release announcements does not mention the git commit hash that was
used to prepare the release.  While SHA1 is broken, I still think
including the commit hash provide some additional information that may
be useful further down the line, and hopefully including doesn't incur
too much cognitive load on the reader (that isn't already present..).

I haven't pushed the attached patch since I'm not a native speaker.
Could someone suggest better wording, if needed?  Or better placement in
the announcement?

To read the result of the patch in context, take some earlier
announcement:

https://lists.gnu.org/archive/html/info-gnu/2024-03/msg6.html

and then consider that the patch would turn the following snippet (for a
hypothethical upcoming GNU inetutils release) text:

This release was bootstrapped with the following tools:
  Gnulib aacceb6eff
  Autoconf 2.71
  Automake 1.16.5
  Bison 3.8.2
  M4 1.4.18
  Makeinfo 6.8
  Help2man 1.49.1
  Make 4.3
  Gzip 1.10
  Tar 1.34

and turn that into this:

This release was built bootstrapped with the following tools
using inetutils git commit 524d4b6934db12b9f43be410d2f201fdb40cfc97:

  Gnulib aacceb6eff
  Autoconf 2.71
  Automake 1.16.5
  Bison 3.8.2
  M4 1.4.18
  Makeinfo 6.8
  Help2man 1.49.1
  Make 4.3
  Gzip 1.10
  Tar 1.34

Does this make sense?  Is the location in the announcement e-mail a good
one?  This hides it a bit further down which I think makes sense.  Few
readers care about git commit and bootstrapping versions, and the
information is related.  The new version adds an empty line which I
think is more consistent with the other paragraphs.

Thoughts?

/Simon
From 8be372e8ddfaa5b7202e2b58b22e55c00d9016c5 Mon Sep 17 00:00:00 2001
From: Simon Josefsson 
Date: Sun, 12 May 2024 17:31:51 +0200
Subject: [PATCH] announce-gen: Mention git commit in release announcement.

* build-aux/announce-gen (this_commit_hash): New variable.
(main): Print commit hash.
---
 ChangeLog  | 6 ++
 build-aux/announce-gen | 6 --
 2 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index b6aa21d7f7..20dbe3c2a3 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,9 @@
+2024-05-12  Simon Josefsson  
+
+	announce-gen: Mention git commit in release announcement.
+	* build-aux/announce-gen (this_commit_hash): New variable.
+	(main): Print commit hash.
+
 2024-05-12  Simon Josefsson  
 
 	maintainer-makefile: Silence announce-gen error with GNULIB_REVISION.
diff --git a/build-aux/announce-gen b/build-aux/announce-gen
index f9e20129dd..3d47ceb9a7 100755
--- a/build-aux/announce-gen
+++ b/build-aux/announce-gen
@@ -35,7 +35,7 @@
 eval 'exec perl -wSx "$0" "$@"'
  if 0;
 
-my $VERSION = '2023-12-29 18:26'; # UTC
+my $VERSION = '2024-05-12 15:30'; # UTC
 # The definition above must lie within the first 8 lines in order
 # for the Emacs time-stamp write hook (at end) to update it.
 # If you change this file with Emacs, please let the write hook
@@ -551,6 +551,8 @@ EOF
   chomp (my $n_ci = `git rev-list "v$v0..v$v1" | wc -l`);
   chomp (my $n_p = `git shortlog "v$v0..v$v1" | grep -c '^[^ ]'`);
 
+  my $this_commit_hash = `git log --pretty=%H -1 "v$v1"`;
+  chop $this_commit_hash;
   my $prev_release_date = `git log --pretty=%ct -1 "v$v0"`;
   my $this_release_date = `git log --pretty=%ct -1 "v$v1"`;
   my $n_seconds = $this_release_date - $prev_release_date;
@@ -672,7 +674,7 @@ EOF
 
   my @tool_versions = get_tool_versions (\@tool_list, $gnulib_version);
   @tool_versions
-and print "\nThis release was bootstrapped with the following tools:",
+and print "\nThis release was built bootstrapped with the following tools\nusing $package_name git commit $this_commit_hash:\n",
   join ('', map {"\n  $_"} @tool_versions), "\n";
 
   print_news_deltas ($_, $prev_version, $curr_version)
-- 
2.41.0



signature.asc
Description: PGP signature

Re: [PATCH 1/4] stdbit: new module

[Paul Eggert]

On 2024-05-11 23:48, Collin Funk wrote:

+# stddef_h.m4
+# serial 1

I see that you use my method of making sure files have license
headers, just copying another file. :)


Thanks, I fixed that.

Is this stuff documented somewhere in the .texi files? If not, I suppose 
it should be.


Frankly I continue to be annoyed by having to read and write a line "# 
FILENAME.m4" at the start of every m4 file FILENAME.m4. It's better for 
a file's first line to be a human-readable comment explaining what the 
file is for. Any automated procedure already knows the file name, so why 
do we need to put the name there manually, an error-prone process?

[PATCH] maintainer-makefile: Silence announce-gen error with GNULIB_REVISION.

[Simon Josefsson via Gnulib discussion list]

On running 'make release' I got this error message:

  GEN  release-prep
fatal: No names found, cannot describe anything.
make[1]: Entering directory '/home/jas/src/inetutils'

The error message is harmless since the code already handled this
situation, but the error message should be silenced since it looks
pretty alarming and the alternative code path using git rev-parse work
correctly as intended.

/Simon
From 0c52a761fbe563f2aa6731fbb18b0572005bc548 Mon Sep 17 00:00:00 2001
From: Simon Josefsson 
Date: Sun, 12 May 2024 17:07:30 +0200
Subject: [PATCH] maintainer-makefile: Silence announce-gen error with
 GNULIB_REVISION.

* top/maint.mk (gnulib-version): Silence git describe on failure.
---
 ChangeLog| 5 +
 top/maint.mk | 2 +-
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/ChangeLog b/ChangeLog
index 2e2311e7b2..b6aa21d7f7 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,8 @@
+2024-05-12  Simon Josefsson  
+
+	maintainer-makefile: Silence announce-gen error with GNULIB_REVISION.
+	* top/maint.mk (gnulib-version): Silence git describe on failure.
+
 2024-05-12  Bruno Haible  
 
 	execinfo: Document known bugs.
diff --git a/top/maint.mk b/top/maint.mk
index 32228f4366..ecd8971900 100644
--- a/top/maint.mk
+++ b/top/maint.mk
@@ -1502,7 +1502,7 @@ vc-diff-check:
 rel-files = $(DIST_ARCHIVES)
 
 gnulib-version = $$(cd $(gnulib_dir)\
-&& { git describe || git rev-parse --short=10 HEAD; } )
+&& { git describe 2> /dev/null || git rev-parse --short=10 HEAD; } )
 bootstrap-tools ?= autoconf,automake,gnulib
 
 gpgv = $$(gpgv2 --version >/dev/null && echo gpgv2 || echo gpgv)
-- 
2.41.0



signature.asc
Description: PGP signature

execinfo: Document known bugs

[Bruno Haible]

Now that we have unit tests for the backtrace* functions, let's see what
they produce:

* I see a test failure on FreeBSD 12.0/i386: The returned size is not in range,
  it is (size_t)(-1).
  The test passes on FreeBSD 14.0/i386.
  In the FreeBSD git history, I can see that the bug fix was in commit
  895a7e604db4ddf2b5d58833a82c2b4956585d02. That is, the last FreeBSD release
  in which it should be reproducible is 12.4.

* An empty stack trace is produced in
  - FreeBSD 11.0/i386.
  - FreeBSD 12.2/sparc64.

* Instead of a truncated stack trace, an empty stack trace is returned on
  FreeBSD (all tested versions from 11.0 to 14.0),
  NetBSD (all tested versions from 7.0 to 10.0),
  OpenBSD (all tested versions from 7.0 to 7.5).

* The strings printed by backtrace_symbols_fd are not consistent with
  backtrace_symbols, on glibc (all tested versions, up to 2.39).

I have reported the glibc bug.

Collin, feel free to report the FreeBSD, NetBSD, OpenBSD bugs if you have
time for that. It's only by reporting bugs that we can trigger that they get
fixed.


2024-05-12  Bruno Haible  

execinfo: Document known bugs.
* doc/glibc-functions/backtrace.texi: Mention the various bugs on
FreeBSD, NetBSD, OpenBSD.
* doc/glibc-functions/backtrace_symbols_fd.texi: Mention the glibc bug.

diff --git a/doc/glibc-functions/backtrace.texi 
b/doc/glibc-functions/backtrace.texi
index b9a56094c1..ef2a3ac657 100644
--- a/doc/glibc-functions/backtrace.texi
+++ b/doc/glibc-functions/backtrace.texi
@@ -32,6 +32,16 @@
 The second argument is of type @code{size_t}, not @code{int}, on some 
platforms:
 FreeBSD, NetBSD, OpenBSD.
 @item
+The return value may be @code{(size_t)(-1)} instead of 0 on some platforms:
+FreeBSD 12.4/i386.
+@item
+The returned stack trace is empty if it would not entirely fit into the
+provided buffer on some platforms:
+FreeBSD 14.0, NetBSD 10.0, OpenBSD 7.5.
+@item
+The returned stack trace is always empty on some platforms:
+FreeBSD 11.0/i386, FreeBSD 12.2/sparc64.
+@item
 On platforms where the function is missing,
 the Gnulib substitute implementation is just a stub, and does nothing.
 @end itemize
diff --git a/doc/glibc-functions/backtrace_symbols_fd.texi 
b/doc/glibc-functions/backtrace_symbols_fd.texi
index fe97b44b35..71461ebcac 100644
--- a/doc/glibc-functions/backtrace_symbols_fd.texi
+++ b/doc/glibc-functions/backtrace_symbols_fd.texi
@@ -32,6 +32,11 @@
 The second argument is of type @code{size_t}, not @code{int}, on some 
platforms:
 FreeBSD, NetBSD, OpenBSD.
 @item
+The strings printed by this function not the same as the strings returned by
+@code{backtrace_symbols} on some platforms:
+@c https://sourceware.org/bugzilla/show_bug.cgi?id=31730
+glibc 2.39.
+@item
 On platforms where the function is missing,
 the Gnulib substitute implementation is just a stub, and does nothing.
 @end itemize

Re: execinfo: Add tests.

[Bruno Haible]

Hi Collin,

> The reasoning for printing the backtrace was so that the output can be
> compared with the BSD implementation or various linker flags, etc.
> Seemed like it might be helpful.

Yes, of course it is helpful. Some functions have a specification that
leaves a lot of room to the implementation. Printing the result helps
to determine quality-of-implementation issues.

> I left backtrace_symbols_fd untested because I did not see too much
> benefit in testing it.

Oh, every function is worth testing. Sometimes the implementors were lazy
and the function always crashes :) (See doc/posix-functions/wctrans.texi.)

Bruno

Re: execinfo: Add tests.

[Collin Funk]

On 5/12/24 4:39 AM, Collin Funk wrote:
> I left backtrace_symbols_fd untested because I did not see too much
> benefit in testing it. Its job is similar enough to backtrace_symbols
> but would require thinking about module dependencies and headers for
> create () and unlink (), or similar.

I forgot about file descriptor 1..

Collin

Re: execinfo: Add tests.

[Collin Funk]

Hi Bruno,

On 5/12/24 3:43 AM, Bruno Haible wrote:
> Reading through your test, I notice that when size == 0 && symbols != NULL,
> the symbols pointer is not freed. So I went to look deeper.
> 
> Reading through the documentation at
> https://www.gnu.org/software/libc/manual/html_node/Backtraces.html
> I found the following opportunities that can be tested in a unit test:
>   - backtrace: "The return value is the actual number of entries of buffer
> that are obtained, and is at most size." This is something that can be 
> tested.
>   - backtrace_symbols: "The return value of backtrace_symbols is a pointer 
> obtained
> via the malloc function ... The return value is NULL if sufficient memory 
> for
> the strings cannot be obtained." Since in unit tests, out-of-memory can be
> assumed to not happen, we can check that the return value is != NULL.
>   - backtrace_symbols: "The return value is a pointer to an array of strings,
> which has size entries just like the array buffer" This sounds like we can
> pass a too-small buffer, and the stack trace will be truncated. This can 
> be
> tested as well.
>   - The third function, backtrace_symbols_fd, is not tested.

Oops, yes you are correct. Thanks for improving the tests. I have
never used these functions before so I am not surprised I made a
mistake...

The reasoning for printing the backtrace was so that the output can be
compared with the BSD implementation or various linker flags, etc.
Seemed like it might be helpful.

I left backtrace_symbols_fd untested because I did not see too much
benefit in testing it. Its job is similar enough to backtrace_symbols
but would require thinking about module dependencies and headers for
create () and unlink (), or similar.

> Note the change in the module description. If we want to verify that there is
> an AC_SUBST for LIB_EXECINFO, the way to do it is to write @LIB_EXECINFO@.
> If the AC_SUBST was missing, $(LIB_EXECINFO) would come out as empty whereas
> @LIB_EXECINFO@ would lead to a build failure.

Ah, thanks for the explanation. When writing the module description I
was a bit confused why some tests used @LIB_NAME@ instead of
$(LIB_NAME). Looks like my guess was unlucky, but now I know for next
time. :)

Collin

Re: execinfo: Add tests.

[Bruno Haible]

Hi Collin,

> I've pushed a basic test for execinfo. Since BSD needs to link to
> -lexecinfo unlike glibc. Should catch any bugs that occur there or if
> another system adds it as a library.

Thanks.

Reading through your test, I notice that when size == 0 && symbols != NULL,
the symbols pointer is not freed. So I went to look deeper.

Reading through the documentation at
https://www.gnu.org/software/libc/manual/html_node/Backtraces.html
I found the following opportunities that can be tested in a unit test:
  - backtrace: "The return value is the actual number of entries of buffer
that are obtained, and is at most size." This is something that can be 
tested.
  - backtrace_symbols: "The return value of backtrace_symbols is a pointer 
obtained
via the malloc function ... The return value is NULL if sufficient memory 
for
the strings cannot be obtained." Since in unit tests, out-of-memory can be
assumed to not happen, we can check that the return value is != NULL.
  - backtrace_symbols: "The return value is a pointer to an array of strings,
which has size entries just like the array buffer" This sounds like we can
pass a too-small buffer, and the stack trace will be truncated. This can be
tested as well.
  - The third function, backtrace_symbols_fd, is not tested.

So, this is what I am committing.

Note the change in the module description. If we want to verify that there is
an AC_SUBST for LIB_EXECINFO, the way to do it is to write @LIB_EXECINFO@.
If the AC_SUBST was missing, $(LIB_EXECINFO) would come out as empty whereas
@LIB_EXECINFO@ would lead to a build failure.


2024-05-12  Bruno Haible  

execinfo tests: Strengthen tests.
* tests/test-execinfo.c (test_backtrace): Add an argument. Check the
return value of backtrace(). Check that backtrace_symbols_fd is defined.
Check the return value of backtrace_symbols().
(main): Test also the case of a short buffer.
* modules/execinfo-tests (Makefile.am): Verify that LIB_EXECINFO is
defined.

diff --git a/modules/execinfo-tests b/modules/execinfo-tests
index 191345cd5c..5d6a28c69b 100644
--- a/modules/execinfo-tests
+++ b/modules/execinfo-tests
@@ -9,4 +9,4 @@ configure.ac:
 Makefile.am:
 TESTS += test-execinfo
 check_PROGRAMS += test-execinfo
-test_execinfo_LDADD = $(LDADD) $(LIB_EXECINFO)
+test_execinfo_LDADD = $(LDADD) @LIB_EXECINFO@
diff --git a/tests/test-execinfo.c b/tests/test-execinfo.c
index af457c468f..5745306ed5 100644
--- a/tests/test-execinfo.c
+++ b/tests/test-execinfo.c
@@ -27,17 +27,28 @@
 #include "macros.h"
 
 static void
-test_backtrace (void)
+test_backtrace (int pass)
 {
   void *buffer[10];
-  char **symbols;
+  int max_size;
   int size;
+  char **symbols;
+
+  max_size = (pass == 0 ? SIZEOF (buffer) : 1);
+  size = backtrace (buffer, max_size);
+  ASSERT (size >= 0 && size <= max_size);
+
+  /* Print the backtrace to a file descriptor.  */
+  backtrace_symbols_fd (buffer, size, 1);
+  printf ("\n");
 
-  size = backtrace (buffer, SIZEOF (buffer));
   symbols = backtrace_symbols (buffer, size);
+  if (size > 0)
+/* We have enough memory available.  */
+ASSERT (symbols != NULL);
 
   /* Print the backtrace if possible.  */
-  if (0 < size && symbols != NULL)
+  if (symbols != NULL)
 {
   for (int i = 0; i < size; ++i)
 printf ("%s\n", symbols[i]);
@@ -48,7 +59,10 @@ test_backtrace (void)
 int
 main (void)
 {
-  test_backtrace ();
+  printf ("Full stack trace:\n"); fflush (stdout);
+  test_backtrace (0);
+  printf ("\nTruncated stack trace:\n"); fflush (stdout);
+  test_backtrace (1);
 
   return 0;
 }

Re: gnulib-tool (Python) bug: "module count-one-bits doesn't exist"

[Bruno Haible]

Collin Funk wrote:
> I've applied the attached patch to fix this crash.

Thanks. Looks good.

> I'm not sure why gnulib-tool.sh handles the tabs correctly to be
> honest...

It's because after
  deps=`func_get_dependencies $module | sed -e 
"$sed_dependencies_without_conditions"`
the variable deps contains

"count-leading-zeros[$GL_GENERATE_STDBIT_H]
count-trailing-zeros[$GL_GENERATE_STDBIT_H]
count-one-bits  [$GL_GENERATE_STDBIT_H]
stdbool [$GL_GENERATE_STDBIT_H]
"

and the subsequent
  for dep in $deps; do
loop does word splitting and thus iterates over 8 words:
  count-leading-zeros
  [$GL_GENERATE_STDBIT_H]
  count-trailing-zeros
  [$GL_GENERATE_STDBIT_H]
  count-one-bits
  [$GL_GENERATE_STDBIT_H]
  stdbool
  [$GL_GENERATE_STDBIT_H]
Four of these are not valid module names and are therefore filtered out
(silently!).

Bruno

Re: [PATCH 1/4] stdbit: new module

[Collin Funk]

Hi Paul,

On 5/11/24 12:08 PM, Paul Eggert wrote:
> diff --git a/m4/stdbit_h.m4 b/m4/stdbit_h.m4
> new file mode 100644
> index 00..9ce3cb3146
> --- /dev/null
> +++ b/m4/stdbit_h.m4
> @@ -0,0 +1,19 @@
> +# stddef_h.m4
> +# serial 1

I see that you use my method of making sure files have license
headers, just copying another file. :)

Referring to the '# stddef_h.m4' comment.

Collin