Re: scratch_buffer.h, scratch_buffer_dupfree.c sync
On 11/3/22 13:40, Karl Berry wrote: Whatever happens, can one of you make the desired changes in gnulib? Already done and installed, here: https://git.savannah.gnu.org/cgit/gnulib.git/commit/?id=ddfcbc95a6be3ddc588a93f21edb69cc7c214d9c As part of that patch, I did the same sort of sync that you regularly do (and thanks for that).
Re: scratch_buffer.h, scratch_buffer_dupfree.c sync
Paul Eggert wrote: > > Here are proposed patches to rename the modules. > > Thanks, those look good to me too. Thanks for the review. Pushed. Bruno
Re: scratch_buffer.h, scratch_buffer_dupfree.c sync
Whatever happens, can one of you make the desired changes in gnulib? I have never tried to follow the glibc/gnulib stuff. This one is above my pay grade :). I just blindly sync the changes ... --thanks, karl.
Re: scratch_buffer.h, scratch_buffer_dupfree.c sync
* Paul Eggert: > What problems do you see with the interfaces, and are there efforts to > come up with a better API? The need is there in GNU apps, each of > which tends to roll its own code here. dynarray has an aliasing violation in its implementation. The embedded pointer should be void * (not DYNARRAY_ELEMENT *), with casts added in the generated code, so that the out-of-line code can use the correct types. Defining the element free function has a trap for the unwary programmer: it's easy to miss the required pointer indirection. I don't know a good fix for that. I view dynarray as a stop-gap until we can use a C++ template inside glibc. We won't be able to use std::vector because of gaps in memory allocation failure handling, but application code interested in basic type-generic data structures should really use libstdc++ and not preprocessor hacks like dynarray. With such wide use of xmalloc & friends, the memory allocation issue would impact few applications. The scratch buffer interface is mainly intended as a helper for calling NSS functions because of their highly problematic buffer interface: it's not just that the caller allocates, the callee does not provide any size hint at all if the provided buffer is not large enough. So the only thing you can do is keep retrying with larger and larger buffers. No one should create interfaces like that. One could argue that scratch buffers are needed because the NSS interfaces exist today, but from a glibc perspective, I think we should provide replacements for the problematic NSS functions that are significantly easier to use, rather than relying on developers to put something together using scratch buffers. Maybe it's sufficient to make the original functions like getpwuid thread-safe, although such functions manipulating global state wouldn't be really library-safe. (You wouldn't want to call getpwuid in a library because it might clobber another getpwuid result still in use elsewhere, but that's an issue that is present with or without thread safety.) I added scratch_buffer_grow_preserve and scratch_buffer_set_array_size as an afterthought. Conceptually, they are unrelated to the NSS usage. They predate dynarrays. If dynarrays were easier to define, we should have switched over to them because they have implicit overflow checking for allocation sizes. Thanks, Florian
Re: scratch_buffer.h, scratch_buffer_dupfree.c sync
On 2022-11-03 05:41, Bruno Haible wrote: Here are proposed patches to rename the modules. Thanks, those look good to me too.
Re: scratch_buffer.h, scratch_buffer_dupfree.c sync
On 2022-11-03 04:03, Florian Weimer wrote: I must say I was surprised to see dynarray and scratch_buffer end up in gnulib. I never intended them to escape this way from glibc. They escaped from glibc because they're used by code shared with Gnulib (e.g., canonicalize.c). The interfaces and their implementation are problematic in some ways, and I can't recommend them for general use. Thanks for letting us know. What problems do you see with the interfaces, and are there efforts to come up with a better API? The need is there in GNU apps, each of which tends to roll its own code here.
Re: scratch_buffer.h, scratch_buffer_dupfree.c sync
Florian Weimer wrote:
> > But it means that we cannot promise that these .h files are even remotely
> > stable APIs.
>
> I must say I was surprised to see dynarray and scratch_buffer end up in
> gnulib. I never intended them to escape this way from glibc. The
> interfaces and their implementation are problematic in some ways, and I
> can't recommend them for general use.
Thanks for this clear statement, Florian.
So I was too optimistic when I wrote
- "It looks like the 'scratch_buffer' could be useful to some programs
outside of glibc." [1]
- "dynarray looks useful" [2]
Here are proposed patches to rename the modules.
[1] https://lists.gnu.org/archive/html/bug-gnulib/2021-02/msg00073.html
[2] https://lists.gnu.org/archive/html/bug-gnulib/2021-03/msg00042.html
2022-11-03 Bruno Haible
dynarray: Rename to glibc-internal/dynarray.
* modules/glibc-internal/dynarray: Renamed from modules/dynarray.
* modules/glibc-internal/dynarray-tests: Renamed from
modules/dynarray-tests.
* modules/regex (Depends-on): Update.
* NEWS: Mention this change and the previous one.
2022-11-03 Bruno Haible
scratch_buffer: Rename to glibc-internal/scratch_buffer.
* modules/glibc-internal/scratch_buffer: Renamed from
modules/scratch_buffer.
* modules/glibc-internal/scratch_buffer-tests: Renamed from
modules/scratch_buffer-tests.
* modules/canonicalize (Depends-on): Update.
* modules/canonicalize-lgpl (Depends-on): Likewise.
* modules/glob (Depends-on): Likewise.
>From 371a0f95a7e90d29a7272d0e7cfde1eb0b61a235 Mon Sep 17 00:00:00 2001
From: Bruno Haible
Date: Thu, 3 Nov 2022 13:30:04 +0100
Subject: [PATCH 1/2] scratch_buffer: Rename to glibc-internal/scratch_buffer.
* modules/glibc-internal/scratch_buffer: Renamed from
modules/scratch_buffer.
* modules/glibc-internal/scratch_buffer-tests: Renamed from
modules/scratch_buffer-tests.
* modules/canonicalize (Depends-on): Update.
* modules/canonicalize-lgpl (Depends-on): Likewise.
* modules/glob (Depends-on): Likewise.
---
ChangeLog | 11 +
modules/canonicalize | 2 +-
modules/canonicalize-lgpl | 36 -
modules/{ => glibc-internal}/scratch_buffer | 0
.../{ => glibc-internal}/scratch_buffer-tests | 0
modules/glob | 40 +--
6 files changed, 50 insertions(+), 39 deletions(-)
rename modules/{ => glibc-internal}/scratch_buffer (100%)
rename modules/{ => glibc-internal}/scratch_buffer-tests (100%)
diff --git a/ChangeLog b/ChangeLog
index a7a3f30c0d..70e717284b 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,14 @@
+2022-11-03 Bruno Haible
+
+ scratch_buffer: Rename to glibc-internal/scratch_buffer.
+ * modules/glibc-internal/scratch_buffer: Renamed from
+ modules/scratch_buffer.
+ * modules/glibc-internal/scratch_buffer-tests: Renamed from
+ modules/scratch_buffer-tests.
+ * modules/canonicalize (Depends-on): Update.
+ * modules/canonicalize-lgpl (Depends-on): Likewise.
+ * modules/glob (Depends-on): Likewise.
+
2022-11-02 Bruno Haible
scratch_buffer: Document last change.
diff --git a/modules/canonicalize b/modules/canonicalize
index ddd83bc841..25667c431b 100644
--- a/modules/canonicalize
+++ b/modules/canonicalize
@@ -24,7 +24,7 @@ mempcpy
nocrash
rawmemchr
readlink
-scratch_buffer
+glibc-internal/scratch_buffer
stat
stdbool
sys_stat
diff --git a/modules/canonicalize-lgpl b/modules/canonicalize-lgpl
index 252618e95e..7842cda2da 100644
--- a/modules/canonicalize-lgpl
+++ b/modules/canonicalize-lgpl
@@ -11,24 +11,24 @@ Depends-on:
extensions
stdlib
nocrash
-double-slash-root [test $HAVE_CANONICALIZE_FILE_NAME = 0 || test $REPLACE_CANONICALIZE_FILE_NAME = 1]
-eloop-threshold [test $HAVE_CANONICALIZE_FILE_NAME = 0 || test $REPLACE_CANONICALIZE_FILE_NAME = 1]
-errno [test $HAVE_CANONICALIZE_FILE_NAME = 0 || test $REPLACE_CANONICALIZE_FILE_NAME = 1]
-fcntl-h [test $HAVE_CANONICALIZE_FILE_NAME = 0 || test $REPLACE_CANONICALIZE_FILE_NAME = 1]
-filename [test $HAVE_CANONICALIZE_FILE_NAME = 0 || test $REPLACE_CANONICALIZE_FILE_NAME = 1]
-idx [test $HAVE_CANONICALIZE_FILE_NAME = 0 || test $REPLACE_CANONICALIZE_FILE_NAME = 1]
-intprops [test $HAVE_CANONICALIZE_FILE_NAME = 0 || test $REPLACE_CANONICALIZE_FILE_NAME = 1]
-libc-config [test $HAVE_CANONICALIZE_FILE_NAME = 0 || test $REPLACE_CANONICALIZE_FILE_NAME = 1]
-memmove [test $HAVE_CANONICALIZE_FILE_NAME = 0 || test $REPLACE_CANONICALIZE_FILE_NAME = 1]
-mempcpy [test $HAVE_CANONICALIZE_FILE_NAME = 0 || test $REPLACE_CANONICALIZE_FILE_NAME = 1]
-pathmax [test $HAVE_CANONICALIZE_FILE_NAME = 0 || test $REPLACE_CANONICALIZE_FILE_NAME = 1]
-rawmemchr [test $HAVE_CANONICALIZE_FILE_NAME = 0 || test $REPLACE_CANONICALIZE_FILE_NAME = 1]
-readlink
Re: scratch_buffer.h, scratch_buffer_dupfree.c sync
* Bruno Haible: > But when it comes to scratch_buffer or dynarray code, this week's experience > has shown that we cannot count on as much care for Gnulib users. Rather, > the mindset on the glibc side seems to be: "This is glibc internal code; > we can refactor / reshuffle / trim it as we like." [1][2] > > So, if we want to offer a Gnulib module from glibc-internal code, it would > be our job to maintain compatibility across glibc's refactorings. In this > particular case, it would have meant to add the scratch_buffer_dupfree.c > as a Gnulib-owned source file. But it the long run, this is going to be > a growing maintenance effort. (We have a similar situation: gettext's libintl > is derived from glibc/intl/, and it's a continuing effort to keep the two > more or less in sync.) > > Therefore I agree with what you did yesterday: remove scratch_buffer_dupfree.c > from Gnulib, since glibc dropped it. > > But it means that we cannot promise that these .h files are even remotely > stable APIs. I must say I was surprised to see dynarray and scratch_buffer end up in gnulib. I never intended them to escape this way from glibc. The interfaces and their implementation are problematic in some ways, and I can't recommend them for general use. Thanks, Florian
Re: scratch_buffer.h, scratch_buffer_dupfree.c sync
Hi Paul, Paul Eggert wrote: > > In other words, I'm suggesting to rename the modules > >scratch_buffer -> glibc-internal/scratch_buffer > >dynarray -> glibc-internal/dynarray > > I don't see any problem with that, since as far as I know the only > current users of the two modules are glibc-related modules in Gnulib > itself. Yes, as far as I can see, no packages have started to use these two modules yet. > However, if in the future programs find these two modules > useful, I suppose we'll need to rename them back. No, I don't think this is going to fly, since we would be offering an API for use, for which we cannot guarantee a reasonable degree of stability. The liaison between Gnulib and Glibc is mostly you. If the glibc people want to make changes to intprops.h, idx.h, filename.h, or the regex code, you will certainly be aware of it, and be able to prevent breakage for Gnulib users. But when it comes to scratch_buffer or dynarray code, this week's experience has shown that we cannot count on as much care for Gnulib users. Rather, the mindset on the glibc side seems to be: "This is glibc internal code; we can refactor / reshuffle / trim it as we like." [1][2] So, if we want to offer a Gnulib module from glibc-internal code, it would be our job to maintain compatibility across glibc's refactorings. In this particular case, it would have meant to add the scratch_buffer_dupfree.c as a Gnulib-owned source file. But it the long run, this is going to be a growing maintenance effort. (We have a similar situation: gettext's libintl is derived from glibc/intl/, and it's a continuing effort to keep the two more or less in sync.) Therefore I agree with what you did yesterday: remove scratch_buffer_dupfree.c from Gnulib, since glibc dropped it. But it means that we cannot promise that these .h files are even remotely stable APIs. Bruno [1] https://sourceware.org/pipermail/libc-alpha/2022-October/143030.html [2] https://sourceware.org/pipermail/libc-alpha/2022-October/143033.html
Re: scratch_buffer.h, scratch_buffer_dupfree.c sync
On 11/2/22 19:37, Bruno Haible wrote: In other words, I'm suggesting to rename the modules scratch_buffer -> glibc-internal/scratch_buffer dynarray -> glibc-internal/dynarray I don't see any problem with that, since as far as I know the only current users of the two modules are glibc-related modules in Gnulib itself. However, if in the future programs find these two modules useful, I suppose we'll need to rename them back. The scratch_buffer_dupfree function was added to glibc in 2020 only to make it easier to sync canonicalize.c with Gnulib at the time. So it is a bit ironic that removing scratch_buffer_dupfree from Gnulib now would cause us to in some sense deprecate scratch_buffer.
Re: scratch_buffer.h, scratch_buffer_dupfree.c sync
Paul Eggert wrote: > Yes, it's a nontrivial merge. However, I think we can still sync > LIBC/include/scratch_buffer.h. I installed the attached patch It's a backward-incompatible change: A documented function is now suddenly gone, without a deprecation period. Let me document it in NEWS. I think this has some consequences on how we deal with glibc internals in Gnulib. We exported the 'scratch_buffer' module, thinking that it's a welcome addition to the Gnulib API. But we are seeing that either the glibc people did not know that this API is exported from Gnulib, or they knew but ignored the fact that this API is exported from Gnulib. Could anything be done to avoid such things from occurring again? If not, then I'm inclined to view this way of exporting glibc internals as a failed experiment. And as a consequence, we should only take and export glibc code if it is - either a POSIX API, or - documented in the glibc manual. And if we have Gnulib modules that contain glibc-internal code, we should make it clear that it is not Gnulib public API. In other words, I'm suggesting to rename the modules scratch_buffer -> glibc-internal/scratch_buffer dynarray -> glibc-internal/dynarray 2022-11-02 Bruno Haible scratch_buffer: Document last change. * NEWS: Mention last change. diff --git a/NEWS b/NEWS index 840564703a..327fc8ceee 100644 --- a/NEWS +++ b/NEWS @@ -74,6 +74,8 @@ User visible incompatible changes DateModules Changes +2022-11-02 scratch_buffer The function 'gl_scratch_buffer_dupfree' is removed. + 2022-09-10 stdbool This module now assumes C99 and provides C23, instead of providing C99. For the old behavior, use the already-deprecated stdbool-c99 module.
Re: scratch_buffer.h, scratch_buffer_dupfree.c sync
On 11/2/22 15:37, Karl Berry wrote: 1) LIBC/include/scratch_buffer.h has introduced some substantial changes over GNULIB/lib/malloc/scratch_buffer.h. I'm not sure if it is safe to sync them any more. Especially because: 2) LIBC/nalloc/scratch_buffer_dupfree.c no longer exists. There is no such file in libc any more. Yes, it's a nontrivial merge. However, I think we can still sync LIBC/include/scratch_buffer.h. I installed the attached patch; please let me know of any further problems in this area. From ddfcbc95a6be3ddc588a93f21edb69cc7c214d9c Mon Sep 17 00:00:00 2001 From: Paul Eggert Date: Wed, 2 Nov 2022 18:14:15 -0700 Subject: [PATCH] scratch_buffer: adjust to glibc changes Problem reported by Karl Berry in: https://lists.gnu.org/r/bug-gnulib/2022-11/msg4.html * build-aux/depcomp, doc/make-stds.texi, lib/malloc/scratch_buffer.h: Autoupdate. * build-aux/install-reloc (func_create_wrapper): Omit removed file scratch_buffer_dupfree.c. * config/srclist.txt: Remove lib/malloc/scratch_buffer_dupfree.c * lib/canonicalize-lgpl.c: Merge changes from glibc through its commit ef074bf0dccf493a5e8e21f71d9e7972ea9f dated 2022-07-05 11:04:45 +0200. (__strdup) [!_LIBC]: New macro. (struct realpath_bufs): New type. (realpath_stk): Use it as the extra argument. All uses changed. No longer any need for noinline or GCC_BOGUS_WRETURN_LOCAL_ADDR. * lib/canonicalize.c (struct realpath_bufs) (canonicalize_filename_mode_stk): Likewise. * lib/malloc/scratch_buffer_dupfree.c: Remove, since it was removed in glibc. * lib/scratch_buffer.h (scratch_buffer_dupfree) [0]: (__libc_scratch_buffer_dupfree): Remove decls. * modules/relocatable-prog-wrapper (Files): * modules/scratch_buffer (Files, lib_SOURCES): Remove lib/malloc/scratch_buffer_dupfree.c. --- ChangeLog | 27 ++ build-aux/depcomp | 4 +- build-aux/install-reloc | 2 - config/srclist.txt | 1 - doc/make-stds.texi | 4 +- lib/canonicalize-lgpl.c | 122 ++-- lib/canonicalize.c | 91 + lib/malloc/scratch_buffer.h | 16 lib/malloc/scratch_buffer_dupfree.c | 41 -- lib/scratch_buffer.h| 10 --- modules/relocatable-prog-wrapper| 1 - modules/scratch_buffer | 4 +- 12 files changed, 129 insertions(+), 194 deletions(-) delete mode 100644 lib/malloc/scratch_buffer_dupfree.c diff --git a/ChangeLog b/ChangeLog index 3dc7d8966f..0defe2469e 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,30 @@ +2022-11-02 Paul Eggert + + scratch_buffer: adjust to glibc changes + Problem reported by Karl Berry in: + https://lists.gnu.org/r/bug-gnulib/2022-11/msg4.html + * build-aux/depcomp, doc/make-stds.texi, lib/malloc/scratch_buffer.h: + Autoupdate. + * build-aux/install-reloc (func_create_wrapper): + Omit removed file scratch_buffer_dupfree.c. + * config/srclist.txt: Remove lib/malloc/scratch_buffer_dupfree.c + * lib/canonicalize-lgpl.c: Merge changes from glibc through its + commit ef074bf0dccf493a5e8e21f71d9e7972ea9f dated 2022-07-05 + 11:04:45 +0200. + (__strdup) [!_LIBC]: New macro. + (struct realpath_bufs): New type. + (realpath_stk): Use it as the extra argument. All uses changed. + No longer any need for noinline or GCC_BOGUS_WRETURN_LOCAL_ADDR. + * lib/canonicalize.c (struct realpath_bufs) + (canonicalize_filename_mode_stk): Likewise. + * lib/malloc/scratch_buffer_dupfree.c: + Remove, since it was removed in glibc. + * lib/scratch_buffer.h (scratch_buffer_dupfree) [0]: + (__libc_scratch_buffer_dupfree): Remove decls. + * modules/relocatable-prog-wrapper (Files): + * modules/scratch_buffer (Files, lib_SOURCES): + Remove lib/malloc/scratch_buffer_dupfree.c. + 2022-11-01 Bruno Haible relocatable-script: Relax license. diff --git a/build-aux/depcomp b/build-aux/depcomp index 75323b7392..703eed2750 100755 --- a/build-aux/depcomp +++ b/build-aux/depcomp @@ -1,7 +1,7 @@ #! /bin/sh # depcomp - compile a program generating dependencies as side-effects -scriptversion=2018-03-07.03; # UTC +scriptversion=2022-09-18.14; # UTC # Copyright (C) 1999-2022 Free Software Foundation, Inc. @@ -197,7 +197,7 @@ gcc3) ;; gcc) -## Note that this doesn't just cater to obsosete pre-3.x GCC compilers. +## Note that this doesn't just cater to obsolete pre-3.x GCC compilers. ## but also to in-use compilers like IMB xlc/xlC and the HP C compiler. ## (see the conditional assignment to $gccflag above). ## There are various ways to get dependency output from gcc. Here's diff --git a/build-aux/install-reloc b/build-aux/install-reloc index 63aeb9af1c..22739681ae 100755 --- a/build-aux/install-reloc +++ b/build-aux/install-reloc @@ -237,7 +237,6 @@ func_create_wrapper () "$srcdir"/readlink.c \ "$srcdir"/stat.c \ "$srcdir"/canonicalize-lgpl.c \ -
