Avoid gnulib redefinitions - MDA, free-posix

Avoid gnulib redefinitions - MDA, free-posix

[Gavin Smith]

Previously in the Texinfo project, we added variables to configure.ac to
stop the redefinition of "Microsoft deprecated aliases":

https://lists.gnu.org/archive/html/bug-gnulib/2021-03/msg00004.html

For example, GNULIB_MDA_FDOPEN to stop the redefinition of 'fdopen'.

Recently, it was reported that this didn't work when building on
MS-Windows.  I found that it should maybe be GL_GNULIB_MDA_FDOPEN instead:

https://lists.gnu.org/archive/html/bug-texinfo/2022-10/msg00180.html

I had identified the following change as potentially being responsible:

2021-04-11  Bruno Haible  <bruno@clisp.org>

        Support several gnulib-tool invocations under the same configure.ac.
        Reported by Reuben Thomas <rrt@sc3d.org> in
        <https://lists.gnu.org/archive/html/bug-gnulib/2021-04/msg00104.html>.  
        This is done by defining the Gnulib module indicator variables per
        gnulib-tool invocation. So that a generated .h file is no longer
        influenced by the set of modules used in other gnulib-tool invocations. 
        * gnulib-tool (func_compute_include_guard_prefix): Set
        module_indicator_prefix.

Should we use the variables with the GL_ prefix now and is this something
we can rely on?  Or should we simply #undef fdopen and the other symbols?

We don't use fdopen, putenv or mktemp in the library being built, but these
are pulled in by Gnulib dependencies.

I don't know if it is possible at all but it would seem to be better
for Gnulib not to redefine symbols that are not actually used in the
program.  I'd like to keep the use of Gnulib code to a minimum, only
using it where there is a significant portability benefit.

Likewise, there is a warning about the redefinition of "free", that
comes from the free-posix module.  The issue is, when building a
Perl extension module, some source files include Perl headers that
also redefine "free".

free-posix is to work around the possibility that free overwrites
the value of errno.  It's unlikely that this would cause a problem
in our code, but free-posix is required internally by Gnulib code.

It seems undesirable to override such a simple function as 'free',
when this is not something the user has asked for - as in our case,
it caused a conflict with other uses (Perl headers).

This may be hard for you to change, though.  Here is one idea.  When
using a module like 'free-posix', if it is loaded as a dependency only,
use the redefinition in Gnulib code only, but do not override symbols
in user code.  It would be as if there were two modules, say
gl-free-posix and free-posix, where gl-free-posix made the redefinition
of rpl_gl_free and free-posix (requested by the user) redirected rpl_free
to rpl_gl_free.

Re: Avoid gnulib redefinitions - MDA, free-posix

[Paul Eggert]

On 2022-10-29 06:36, Gavin Smith wrote:
> Here is one idea.  When
> using a module like 'free-posix', if it is loaded as a dependency only,
> use the redefinition in Gnulib code only, but do not override symbols
> in user code.  It would be as if there were two modules, say
> gl-free-posix and free-posix, where gl-free-posix made the redefinition
> of rpl_gl_free and free-posix (requested by the user) redirected rpl_free
> to rpl_gl_free.

If it's just 'free', I might prefer the latter solution, to underline 
the special case and to avoid even more complexity in gnulib-tool. Is 
that something you could write?

If this problem occurs with lots of other symbols perhaps we'd need 
something more like the former, though. This would need more hacking, I 
expect.

Re: Avoid gnulib redefinitions - MDA, free-posix

[Gavin Smith]

On Sat, Oct 29, 2022 at 09:58:25AM -0700, Paul Eggert wrote:
> If it's just 'free', I might prefer the latter solution, to underline the
> special case and to avoid even more complexity in gnulib-tool. Is that
> something you could write?

Here is a first attempt.  I am not very familiar with the Gnulib code.

The main idea is to put a define in AM_CPPFLAGS in the Makefile that
gnulib-tool generates.  Then the gnulib headers can distinguish whether
they are being used as part of building gnulib, or building the user's
program.

This avoided looking through all the gnulib code and deciding where
to replace "free" with "gl_internal_free" or whatever the replacement
would be called.

I tried to minimize the number of lines of code that would change, but
if this approach was adopted, then the following changes should also be
made:
* Rename free-posix to free-posix-internal and free-posix-public to free-posix
* REPLACE_FREE to be changed to REPLACE_FREE_INTERNAL throughout gnulib,
  likewise REPACE_FREE_PUBLIC to REPLACE_FREE.

I tested this on a project with a gnulib import.

Let me know if this is the right approach and if there are changes to
make to the patch.

diff --git a/ChangeLog b/ChangeLog
index 6f4bea5c1c..994f457e70 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,21 @@
+2020-10-29  Gavin Smith  <gavinsmith0123@gmail.com>
+
+	Avoid unrequested 'free' redefinition
+
+	* gnulib-tool (func_emit_lib_Makefile_am):
+	Add -DGNULIB_INTERNAL to AM_CPPFLAGS.
+	* m4/free-public.m4, modules/free-posix-public:
+	New module to set @REPLACE_FREE_PUBLIC@ from @REPLACE_FREE@.
+	* modules/stdlib, m4/stdlib_h.m4:
+	Substitute for @REPLACE_FREE_PUBLIC@.
+	* lib/stdlib.in.h <@GNULIB_FREE_POSIX@>: Only replace 'free' if
+	both GNULIB_INTERNAL and @REPLACE_FREE@ are set, or if
+	@REPLACE_FREE_PUBLIC@ is set.
+
+	This allows Gnulib to use the free redefinition internally without
+	replacing it in user code, unless they explicitly request the
+	module.
+
 2022-10-23  Bruno Haible  <bruno@clisp.org>
 
 	assert-h: Make static_assert work on Solaris 11.4.
diff --git a/gnulib-tool b/gnulib-tool
index 028bcf36ad..84a77171d7 100755
--- a/gnulib-tool
+++ b/gnulib-tool
@@ -3945,7 +3945,7 @@ func_emit_lib_Makefile_am ()
   fi
   if test -z "$makefile_name"; then
     echo
-    echo "AM_CPPFLAGS =$cppflags_part1$cppflags_part2"
+    echo "AM_CPPFLAGS = -DGNULIB_INTERNAL$cppflags_part1$cppflags_part2"
     echo "AM_CFLAGS ="
   else
     if test -n "$cppflags_part1$cppflags_part2"; then
diff --git a/lib/stdlib.in.h b/lib/stdlib.in.h
index 8e0a609f1f..f75b8621f4 100644
--- a/lib/stdlib.in.h
+++ b/lib/stdlib.in.h
@@ -179,7 +179,7 @@ _GL_WARN_ON_USE (_Exit, "_Exit is unportable - "
 
 
 #if @GNULIB_FREE_POSIX@
-# if @REPLACE_FREE@
+# if @REPLACE_FREE@ && defined(GNULIB_INTERNAL) || @REPLACE_FREE_PUBLIC@
 #  if !(defined __cplusplus && defined GNULIB_NAMESPACE)
 #   undef free
 #   define free rpl_free
diff --git a/m4/free-public.m4 b/m4/free-public.m4
new file mode 100644
index 0000000000..497abf81ae
--- /dev/null
+++ b/m4/free-public.m4
@@ -0,0 +1 @@
+AC_DEFUN([gl_PREREQ_FREE_PUBLIC], [:])
diff --git a/m4/stdlib_h.m4 b/m4/stdlib_h.m4
index 9e2096976f..b096054284 100644
--- a/m4/stdlib_h.m4
+++ b/m4/stdlib_h.m4
@@ -171,6 +171,7 @@ AC_DEFUN([gl_STDLIB_H_DEFAULTS],
   REPLACE_CALLOC_FOR_CALLOC_POSIX=0;  AC_SUBST([REPLACE_CALLOC_FOR_CALLOC_POSIX])
   REPLACE_CANONICALIZE_FILE_NAME=0;  AC_SUBST([REPLACE_CANONICALIZE_FILE_NAME])
   REPLACE_FREE=0;            AC_SUBST([REPLACE_FREE])
+  REPLACE_FREE_PUBLIC=0;     AC_SUBST([REPLACE_FREE_PUBLIC])
   REPLACE_INITSTATE=0;       AC_SUBST([REPLACE_INITSTATE])
   REPLACE_MALLOC_FOR_MALLOC_GNU=0;    AC_SUBST([REPLACE_MALLOC_FOR_MALLOC_GNU])
   REPLACE_MALLOC_FOR_MALLOC_POSIX=0;  AC_SUBST([REPLACE_MALLOC_FOR_MALLOC_POSIX])
diff --git a/modules/free-posix-public b/modules/free-posix-public
new file mode 100644
index 0000000000..050a06bf76
--- /dev/null
+++ b/modules/free-posix-public
@@ -0,0 +1,27 @@
+Description:
+Work around systems where free clobbers errno.
+
+Files:
+m4/free-public.m4
+
+Depends-on:
+free-posix
+
+configure.ac:
+REPLACE_FREE_PUBLIC=$REPLACE_FREE
+gl_CONDITIONAL([GL_COND_OBJ_FREE_PUBLIC], [test $REPLACE_FREE_PUBLIC = 1])
+AM_COND_IF([GL_COND_OBJ_FREE_PUBLIC], [
+  gl_PREREQ_FREE_PUBLIC
+])
+gl_STDLIB_MODULE_INDICATOR([free-posix-public])
+
+Makefile.am:
+
+Include:
+<stdlib.h>
+
+License:
+LGPLv2+
+
+Maintainer:
+None
diff --git a/modules/stdlib b/modules/stdlib
index 45d8f59331..2266e68d52 100644
--- a/modules/stdlib
+++ b/modules/stdlib
@@ -131,6 +131,7 @@ stdlib.h: stdlib.in.h $(top_builddir)/config.status $(CXXDEFS_H) \
 	      -e 's|@''REPLACE_CALLOC_FOR_CALLOC_POSIX''@|$(REPLACE_CALLOC_FOR_CALLOC_POSIX)|g' \
 	      -e 's|@''REPLACE_CANONICALIZE_FILE_NAME''@|$(REPLACE_CANONICALIZE_FILE_NAME)|g' \
 	      -e 's|@''REPLACE_FREE''@|$(REPLACE_FREE)|g' \
+	      -e 's|@''REPLACE_FREE_PUBLIC''@|$(REPLACE_FREE_PUBLIC)|g' \
 	      -e 's|@''REPLACE_INITSTATE''@|$(REPLACE_INITSTATE)|g' \
 	      -e 's|@''REPLACE_MALLOC_FOR_MALLOC_GNU''@|$(REPLACE_MALLOC_FOR_MALLOC_GNU)|g' \
 	      -e 's|@''REPLACE_MALLOC_FOR_MALLOC_POSIX''@|$(REPLACE_MALLOC_FOR_MALLOC_POSIX)|g' \

Re: Avoid gnulib redefinitions - MDA

[Bruno Haible]

Hi Gavin,

> Previously in the Texinfo project, we added variables to configure.ac to
> stop the redefinition of "Microsoft deprecated aliases":
> 
> https://lists.gnu.org/archive/html/bug-gnulib/2021-03/msg00004.html
> 
> For example, GNULIB_MDA_FDOPEN to stop the redefinition of 'fdopen'.
> 
> Recently, it was reported that this didn't work when building on
> MS-Windows.  I found that it should maybe be GL_GNULIB_MDA_FDOPEN instead:
> 
> https://lists.gnu.org/archive/html/bug-texinfo/2022-10/msg00180.html
> 
> I had identified the following change as potentially being responsible:
> 
> 2021-04-11  Bruno Haible  <bruno@clisp.org>
> 
>         Support several gnulib-tool invocations under the same configure.ac.
>         Reported by Reuben Thomas <rrt@sc3d.org> in
>         <https://lists.gnu.org/archive/html/bug-gnulib/2021-04/msg00104.html>.  
>         This is done by defining the Gnulib module indicator variables per
>         gnulib-tool invocation. So that a generated .h file is no longer
>         influenced by the set of modules used in other gnulib-tool invocations. 
>         * gnulib-tool (func_compute_include_guard_prefix): Set
>         module_indicator_prefix.
> 
> Should we use the variables with the GL_ prefix now and is this something
> we can rely on?  Or should we simply #undef fdopen and the other symbols?

The way to avoid a particular MDA symbol definition (GNULIB_MDA_FDOPEN=0
before April 2021, GL_GNULIB_MDA_FDOPEN=0 after April 2021) is an undocumented
functionality. It is not expected that it will break soon. The 2021-04-11
change that you cited above was a once-in-a-decade change. But it may break
theoretically, since it is not in the form of a stable functionality.
(A stable, supported functionality would be something like a gnulib-tool
option and/or a module name.)

> We don't use fdopen, putenv or mktemp in the library being built, but these
> are pulled in by Gnulib dependencies.
>
> I don't know if it is possible at all but it would seem to be better
> for Gnulib not to redefine symbols that are not actually used in the
> program.

I chose not to do so for the following reasons:

  * These "Microsoft deprecated aliases" are deprecated. This means, they
    can break at any moment, causing bug reports regarding all released
    tarballs. It is better (for 99% of the package maintainers) to have
    this problem dealt with, in advance, _before_ it becomes an FTBFS.

  * Already now, these "Microsoft deprecated alias" symbols cause link
    errors in a particular native Windows platform. Namely, when clang-cl
    is used in combination with the MSVC header files. (clang for Windows
    does not come with its own Win32 API header files, like mingw does.)

  * There are 50 "Microsoft deprecated aliases". If Gnulib would use an
    opt-in approach for these, i.e. if the package maintainer would have
    to enumerate all of these that their package uses one-by-one, it
    would be too much maintenance effort / too high risk of mistake.

  * Given that in this list you find symbols like 'open' / 'creat' / 'close'
    and 'strdup', most programs that rely on POSIX APIs need the
    "Microsoft deprecated aliases" handling. Only programs that use only
    ISO C APIs wouldn't care; but these programs usually don't need Gnulib
    anyway.

So, to me, an opt-out approach seems to be the best approach here.

It broke because the hint that I gave you in March 2021 was not a stable API.
But we don't have stable APIs for everything.

Bruno

Re: Avoid gnulib redefinitions - MDA

[Gavin Smith]

On Sat, Oct 29, 2022 at 11:48:41PM +0200, Bruno Haible wrote:
> > Should we use the variables with the GL_ prefix now and is this something
> > we can rely on?  Or should we simply #undef fdopen and the other symbols?
> 
> The way to avoid a particular MDA symbol definition (GNULIB_MDA_FDOPEN=0
> before April 2021, GL_GNULIB_MDA_FDOPEN=0 after April 2021) is an undocumented
> functionality. It is not expected that it will break soon. The 2021-04-11
> change that you cited above was a once-in-a-decade change. But it may break
> theoretically, since it is not in the form of a stable functionality.
> (A stable, supported functionality would be something like a gnulib-tool
> option and/or a module name.)

Right, so it sounds like we are better off using #undef before
including the Perl headers to avoid depending on undocumented
functionalities.  Thanks.

Re: Avoid gnulib redefinitions - MDA

[Bruno Haible]

Gavin Smith wrote:
> so it sounds like we are better off using #undef before
> including the Perl headers to avoid depending on undocumented
> functionalities.  Thanks.

Using #undef means to decline all corrections done by Gnulib.
These are documented in the manual. For example, for 'fdopen' [1]
we have documented

  Portability problems fixed by Gnulib:

    This function crashes when invoked with invalid arguments on
    some platforms: MSVC 14.
    On Windows platforms (excluding Cygwin), this function does not
    set errno upon failure. 

So, if you always use fdopen() with correct arguments and if, in
case of a NULL return of this function, the code does not look at
errno, then the '#undef fdopen' is OK. (On those platforms where
the Microsoft deprecated alias still work, or when it is overridden
by a redirect to some perl internal function.)

Bruno

[1] https://www.gnu.org/software/gnulib/manual/html_node/fdopen.html

Re: Avoid gnulib redefinitions - MDA

[Gavin Smith]

On Sun, Oct 30, 2022 at 12:09:50AM +0200, Bruno Haible wrote:
> Gavin Smith wrote:
> > so it sounds like we are better off using #undef before
> > including the Perl headers to avoid depending on undocumented
> > functionalities.  Thanks.
> 
> Using #undef means to decline all corrections done by Gnulib.
> These are documented in the manual.

In our case, using #undef only avoids a compilation warning about
symbols being redefined.  Perl itself redefines fdopen and other
functions, on MS-Windows at least.  (I haven't checked why.)  We
don't use fdopen, but if we did, it's very likely we would need to
use Perl's redefinition for the module to build properly.  I don't
think we'd be able to use Gnulib redefinitions in any case.

The one redefined function from Perl that we do use is "free".
Apparently it's necessary to use Perl's version here (on MS-Windows),
otherwise there can be weird errors.

Re: Avoid gnulib redefinitions - free-posix

[Bruno Haible]

[Removing bug-texinfo from the CC.]

Gavin Smith wrote:
> This avoided looking through all the gnulib code and deciding where
> to replace "free" with "gl_internal_free" or whatever the replacement
> would be called.

Yes, this would not be maintainable. Also because some Gnulib code
is shared with glibc, and we want to keep using 'free' in these files.

> The main idea is to put a define in AM_CPPFLAGS in the Makefile that
> gnulib-tool generates.  Then the gnulib headers can distinguish whether
> they are being used as part of building gnulib, or building the user's
> program.

That's an interesting implementation idea. And it readily generalizes to
other modules, via the "module indicator macro" that we define in
gnulib-common.m4 lines 529..619.

> Let me know if this is the right approach and if there are changes to
> make to the patch.

I'll try to come up with a patch that
  - works for other modules too,
  - requires a gnulib-tool option to be activated. Your patch changes the
    behaviour of Gnulib unconditionally, which would cause problems in
    other packages.

I'm currently thinking to call this gnulib-tool option
  --hide-decls=free-posix
but I'd like to have a better option name than that.

Also, I'm thinking about an option --hide-all-decls, that would be like
--hide-decls for all modules of the transitive closure that were *not*
specified on the command line. That would be a very different way of
using Gnulib, where the package maintainer would be forced to explicitly
enumerate the modules for which they want to see the declarations in their
code. Hmm, maybe that's too radical...

Bruno