-Wlto-type-mismatch warning in error()

-Wlto-type-mismatch warning in error()

[Sam James]

[-- Attachment #1: Type: text/plain, Size: 1967 bytes --]

Hi,

Compiling texinfo 7.0.1 with CFLAGS="-O2 -flto -Werror=lto-type-mismatch" results
in the following:
```
make[3]: Entering directory '/var/tmp/portage/sys-apps/texinfo-7.0.1/work/texinfo-7.0.1/install-info'
x86_64-pc-linux-gnu-gcc -DHAVE_CONFIG_H -I. -I..  -I.. -I../gnulib/lib -I../gnulib/lib -DLOCALEDIR=\"/usr/share/locale\"   -O2 -flto -Werror=lto-type-mismatch     -O2 -flto -Werror=lto-type-mismatch -ggdb3 -Werror=implicit-function-declaration -Werror=implicit-int -c -o install-info.o install-info.c
x86_64-pc-linux-gnu-gcc  -O2 -flto -Werror=lto-type-mismatch     -O2 -flto -Werror=lto-type-mismatch -ggdb3 -Werror=implicit-function-declaration -Werror=implicit-int  -Wl,-O1 -Wl,--as-needed -Wl,--defsym=__gentoo_check_ldflags__=0 -Wl,-z,pack-relative-relocs -o ginstall-info install-info.o ../gnulib/lib/libgnu.a
../gnulib/lib/error.h:33:13: error: type of ‘error’ does not match original declaration [-Werror=lto-type-mismatch]
   33 | extern void error (int __status, int __errnum, const char *__format, ...)
      |             ^
install-info.c:218:1: note: type mismatch in parameter 1
  218 | error (const char *fmt, ...)
      | ^
install-info.c:218:1: note: ‘error’ was previously declared here
install-info.c:218:1: note: code may be misoptimized unless ‘-fno-strict-aliasing’ is used
lto1: some warnings being treated as errors
lto-wrapper: fatal error: //usr/bin/x86_64-pc-linux-gnu-gcc returned 1 exit status
compilation terminated.
/usr/lib/gcc/x86_64-pc-linux-gnu/12/../../../../x86_64-pc-linux-gnu/bin/ld: error: lto-wrapper failed
collect2: error: ld returned 1 exit status
make[3]: *** [Makefile:1514: ginstall-info] Error 1
```

This is with GCC 12.2.1 20221203.

We've also seen this in GNU make, so not sure if it's a
gnulib problem or not, as it may be in lib/error.h (hence CCing bug-gnulib):
- https://bugs.gentoo.org/863713 (texinfo)
- https://bugs.gentoo.org/863824 (make)

Best,
sam

[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 358 bytes --]

Re: -Wlto-type-mismatch warning in error()

[Arsen Arsenović]

[-- Attachment #1: Type: text/plain, Size: 620 bytes --]


Sam James <sam@gentoo.org> writes:

> ../gnulib/lib/error.h:33:13: error: type of ‘error’ does not match original declaration [-Werror=lto-type-mismatch]
>    33 | extern void error (int __status, int __errnum, const char *__format, ...)
>       |             ^
> install-info.c:218:1: note: type mismatch in parameter 1
>   218 | error (const char *fmt, ...)
>       | ^
> install-info.c:218:1: note: ‘error’ was previously declared here

This appears to be a result of error in install-info.c being non-static.
It could be static, and avoid a potential symbol collision.

-- 
Arsen Arsenović

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 381 bytes --]

Re: -Wlto-type-mismatch warning in error()

[Gavin Smith]

On Wed, Dec 07, 2022 at 08:57:45PM +0000, Sam James wrote:

> ../gnulib/lib/error.h:33:13: error: type of ‘error’ does not match original declaration [-Werror=lto-type-mismatch]
>    33 | extern void error (int __status, int __errnum, const char *__format, ...)
>       |             ^
> install-info.c:218:1: note: type mismatch in parameter 1
>   218 | error (const char *fmt, ...)
>       | ^
> install-info.c:218:1: note: ‘error’ was previously declared here
> install-info.c:218:1: note: code may be misoptimized unless ‘-fno-strict-aliasing’ is used
> lto1: some warnings being treated as errors
> lto-wrapper: fatal error: //usr/bin/x86_64-pc-linux-gnu-gcc returned 1 exit status
> compilation terminated.
> /usr/lib/gcc/x86_64-pc-linux-gnu/12/../../../../x86_64-pc-linux-gnu/bin/ld: error: lto-wrapper failed
> collect2: error: ld returned 1 exit status
> make[3]: *** [Makefile:1514: ginstall-info] Error 1
> ```
> 
> This is with GCC 12.2.1 20221203.
> 
> We've also seen this in GNU make, so not sure if it's a
> gnulib problem or not, as it may be in lib/error.h (hence CCing bug-gnulib):
> - https://bugs.gentoo.org/863713 (texinfo)
> - https://bugs.gentoo.org/863824 (make)

I expect it is not a gnulib problem.  install-info/install-info.c declares
a function called "error" which appears to clash with a function from glibc.

The function called "error" in install-info is correctly used when
e.g. "install-info one two three" is run, printing

install-info: excess command line argument `three'

It may depend on the configuration process and what parts of gnulib are
being used.  The "error" module is brought in by "xalloc" (which we
ask for explicitly).

From the Gentoo bug report
> -Werror=lto-type-mismatch:
> User to find possible runtime issues in packages. It likely means the package is unsafe to build & use with LTO.
> For projects using the same identifier but with different types across different files, they must be fixed to be consistent across the codebase.

The simplest way to fix this problem would probably be to rename the "error"
function in install-info.c.  Perhaps this issue has never come up before
because people have not used the LTO options for building.

Re: -Wlto-type-mismatch warning in error()

[Bruno Haible]

Gavin Smith wrote:
> I expect it is not a gnulib problem.  install-info/install-info.c declares
> a function called "error" which appears to clash with a function from glibc.
> ... The "error" module is brought in by "xalloc" (which we
> ask for explicitly).

Yes, I think the problems already exists without use of Gnulib, as it's
a conflict between install-info.c and glibc. But with the Gnulib 'error'
module, the problem becomes bigger.

Namely, see:

$ nm --dynamic /lib/x86_64-linux-gnu/libc.so.6 | grep ' error'
00000000001214e0 W error@@GLIBC_2.2.5
0000000000121700 W error_at_line@@GLIBC_2.2.5
0000000000221484 B error_message_count@@GLIBC_2.2.5
0000000000221480 B error_one_per_line@@GLIBC_2.2.5
0000000000221488 B error_print_progname@@GLIBC_2.2.5

glibc exports 'error' as a weak symbol. This means, without use of Gnulib,
the symbol from install-info.c overrides the one from glibc, and there is
a problem if and only if the 'install-info' program links dynamically
(or loads via 'dlopen') a shared library which happens to use error()
and expects the semantics from glibc.

Whereas with the Gnulib 'error' module, there is a conflict between the
two global function definitions (with 'T' linkage) in install-info.c and
in error.c *always*.

> The simplest way to fix this problem would probably be to rename the "error"
> function in install-info.c.

Yes, or make it 'static' (like Arsen suggested).

Bruno

Re: -Wlto-type-mismatch warning in error()

[Eli Zaretskii]

> From: Bruno Haible <bruno@clisp.org>
> Date: Thu, 08 Dec 2022 01:21:51 +0100
> 
> Gavin Smith wrote:
> > I expect it is not a gnulib problem.  install-info/install-info.c declares
> > a function called "error" which appears to clash with a function from glibc.
> > ... The "error" module is brought in by "xalloc" (which we
> > ask for explicitly).
> 
> Yes, I think the problems already exists without use of Gnulib, as it's
> a conflict between install-info.c and glibc. But with the Gnulib 'error'
> module, the problem becomes bigger.
> 
> Namely, see:
> 
> $ nm --dynamic /lib/x86_64-linux-gnu/libc.so.6 | grep ' error'
> 00000000001214e0 W error@@GLIBC_2.2.5
> 0000000000121700 W error_at_line@@GLIBC_2.2.5
> 0000000000221484 B error_message_count@@GLIBC_2.2.5
> 0000000000221480 B error_one_per_line@@GLIBC_2.2.5
> 0000000000221488 B error_print_progname@@GLIBC_2.2.5
> 
> glibc exports 'error' as a weak symbol. This means, without use of Gnulib,
> the symbol from install-info.c overrides the one from glibc, and there is
> a problem if and only if the 'install-info' program links dynamically
> (or loads via 'dlopen') a shared library which happens to use error()
> and expects the semantics from glibc.

I don't think I understand how dynamic linking changes the situation
here.  If the application defines a function named 'error', why would
the dynamic linker pull it from a shared libc?

> Whereas with the Gnulib 'error' module, there is a conflict between the
> two global function definitions (with 'T' linkage) in install-info.c and
> in error.c *always*.
> 
> > The simplest way to fix this problem would probably be to rename the "error"
> > function in install-info.c.
> 
> Yes, or make it 'static' (like Arsen suggested).

Shouldn't we report this to the GCC folks?  It could be a bug in lto,
no?  I mean, 'error' is not a symbol that applications cannot use, and
if an application defines a function by that name, it should not be
imported from the standard library.  Right?

Re: -Wlto-type-mismatch warning in error()

[Arsen Arsenović via Gnulib discussion list]

[-- Attachment #1.1: Type: text/plain, Size: 2443 bytes --]

Hi,

Eli Zaretskii <eliz@gnu.org> writes:

>> Whereas with the Gnulib 'error' module, there is a conflict between the
>> two global function definitions (with 'T' linkage) in install-info.c and
>> in error.c *always*.
>> 
>> > The simplest way to fix this problem would probably be to rename the "error"
>> > function in install-info.c.
>> 
>> Yes, or make it 'static' (like Arsen suggested).
>
> Shouldn't we report this to the GCC folks?  It could be a bug in lto,
> no?  I mean, 'error' is not a symbol that applications cannot use, and
> if an application defines a function by that name, it should not be
> imported from the standard library.  Right?

I believe this would make them part of the same program.  On top of
that, Gnulib is pulling in error anyway:

$ nm ./gnulib/lib/libgnu.a | grep error
                 U error
$ nm install-info.o ../gnulib/lib/libgnu.a |& grep '\<error\>'
00000000 T error
         U error

My guess is that libgnu_a-xalloc-die.o (the file emitting the U error
symbol) includes gnulib/lib/error.h, GCC records that declaration
(through it's use in xalloc_die), and then detects a mismatch with the
one emitted by install-info.o (the T error symbol) and hence warns.

I imagine this would result is some very strange runtime failures if
anyone ever observed install-info hit an xalloc_die condition.

As a test, building on musl (which lacks the error API, for some reason)
causes the executable to be compiled with the install-info error, rather
than the Gnulib one, demonstrating why this warning happens.

Attempting to --whole-archive link on that platform grants us:

$ x86_64-linux-musl-gcc -o ginstall-info install-info.o \
  -Wl,--whole-archive ../gnulib/lib/libgnu.a -Wl,--no-whole-archive
/usr/libexec/gcc/x86_64-linux-musl/ld: ../gnulib/lib/libgnu.a(libgnu_a-error.o): in function `error':
error.c:(.text+0xe0): multiple definition of `error'; install-info.o:install-info.c:(.text+0x4a0): first defined here
collect2: error: ld returned 1 exit status

I imagine a similar thing would happen with a static glibc link.
Renaming the functions or adding ``static'' elides this issue.

So, GCC appears to be doing the right thing.

Since I went through the process of making all the symbols in that file
(besides main) local, here's the patch that does that, though it's based
on a not-particularly-clean head (so, ChangeLog might conflict):


[-- Attachment #1.2: install-info: Make local symbols static --]
[-- Type: text/x-patch, Size: 8505 bytes --]

From 65b7657bfc0bc84178c95211690be94c767d725f Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Arsen=20Arsenovi=C4=87?= <arsen@aarsen.me>
Date: Thu, 8 Dec 2022 09:52:13 +0100
Subject: [PATCH] install-info: Make local symbols static

* install-info/install-info.c: Make local symbols static.
---
 ChangeLog                   |  5 +++
 install-info/install-info.c | 62 ++++++++++++++++++-------------------
 2 files changed, 36 insertions(+), 31 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index 9bfba11abb..98c2764b99 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,8 @@
+2022-12-08  Arsen Arsenović  <arsen@aarsen.me>
+
+	install-info: Make local symbols static.
+	* install-info/install-info.c: Make local symbols static.
+
 2022-12-01  Arsen Arsenović  <arsen@aarsen.me>
 
 	Re-enable copyable anchors in HTML output
diff --git a/install-info/install-info.c b/install-info/install-info.c
index 8950288f6b..761cbfb4d4 100644
--- a/install-info/install-info.c
+++ b/install-info/install-info.c
@@ -28,11 +28,11 @@ static char *default_section = NULL;
 struct spec_entry;
 struct spec_section;
 
-struct line_data *findlines (char *data, int size, int *nlinesp);
-void insert_entry_here (struct spec_entry *entry, int line_number,
-                        struct line_data *dir_lines, int n_entries); 
-int compare_section_names (const void *s1, const void *s2);
-int compare_entries_text (const void *e1, const void *e2); 
+static struct line_data *findlines (char *data, int size, int *nlinesp);
+static void insert_entry_here (struct spec_entry *entry, int line_number,
+                               struct line_data *dir_lines, int n_entries); 
+static int compare_section_names (const void *s1, const void *s2);
+static int compare_entries_text (const void *e1, const void *e2); 
 \f
 /* Data structures.  */
 
@@ -136,7 +136,7 @@ struct menu_section
 /* This table defines all the long-named options, says whether they
    use an argument, and maps them into equivalent single-letter options.  */
 
-struct option longopts[] =
+static struct option longopts[] =
 {
   { "add-once",  no_argument, NULL, '1'},
   { "align",     required_argument, NULL, 'A'},
@@ -172,39 +172,39 @@ struct option longopts[] =
   { 0 }
 };
 
-regex_t *psecreg = NULL;
+static regex_t *psecreg = NULL;
 
 /* Nonzero means that the name specified for the Info file will be used
    (without removing .gz, .info extension or leading path) to match the
    entries that must be removed.  */
-int remove_exactly = 0;
+static int remove_exactly = 0;
 
 /* Nonzero means that sections that don't have entries in them will be
    deleted.  */
-int remove_empty_sections = 1;
+static int remove_empty_sections = 1;
 
 /* Nonzero means that new Info entries into the DIR file go into all 
    sections that match with --section-regex or --section.  Zero means 
    that new entries go into only the first section that matches.  */
-int add_entries_into_all_matching_sections = 1;
+static int add_entries_into_all_matching_sections = 1;
 
 /* Nonzero means we do not replace same-named info entries.  */
-int keep_old_flag = 0;
+static int keep_old_flag = 0;
 
 /* Nonzero means --test was specified, to inhibit updating the dir file.  */
-int chicken_flag = 0;
+static int chicken_flag = 0;
 
 /* Zero means that entries will not be formatted when they are either 
    added or replaced. */
-int indent_flag = 1;
+static int indent_flag = 1;
 
 /* Zero means that new sections will be added at the end of the DIR file. */
-int order_new_sections_alphabetically_flag = 1;
+static int order_new_sections_alphabetically_flag = 1;
 
 \f
 /* Error message functions.  */
 
-void
+static void
 vdiag (const char *fmt, const char *diagtype, va_list ap)
 {
   fprintf (stderr, "%s: ", progname);
@@ -214,7 +214,7 @@ vdiag (const char *fmt, const char *diagtype, va_list ap)
   putc ('\n', stderr);
 }
 
-void
+static void
 error (const char *fmt, ...)
 {
   va_list ap;
@@ -225,7 +225,7 @@ error (const char *fmt, ...)
 }
 
 /* VARARGS1 */
-void
+static void
 warning (const char *fmt, ...)
 {
   va_list ap;
@@ -237,7 +237,7 @@ warning (const char *fmt, ...)
 
 /* Print error message and exit.  */
 
-void
+static void
 fatal (const char *fmt, ...)
 {
   va_list ap;
@@ -250,7 +250,7 @@ fatal (const char *fmt, ...)
 \f
 /* Return a newly-allocated string
    whose contents concatenate those of S1, S2, S3.  */
-char *
+static char *
 concat (const char *s1, const char *s2, const char *s3)
 {
   int len1 = strlen (s1), len2 = strlen (s2), len3 = strlen (s3);
@@ -267,7 +267,7 @@ concat (const char *s1, const char *s2, const char *s3)
 /* Return a string containing SIZE characters
    copied from starting at STRING.  */
 
-char *
+static char *
 copy_string (const char *string, int size)
 {
   int i;
@@ -280,7 +280,7 @@ copy_string (const char *string, int size)
 
 /* Print fatal error message based on errno, with file name NAME.  */
 
-void
+static void
 pfatal_with_name (const char *name)
 {
   /* Empty files don't set errno, so we get something like
@@ -348,7 +348,7 @@ menu_line_equal (char *line1, int len1, char *line2, int len2)
 /* Given the full text of a menu entry, null terminated,
    return just the menu item name (copied).  */
 
-char *
+static char *
 extract_menu_item_name (char *item_text)
 {
   char *p;
@@ -487,7 +487,7 @@ menu_item_equal (const char *item, char term_char, const char *name)
 
 
 \f
-void
+static void
 suggest_asking_for_help (void)
 {
   fprintf (stderr, _("\tTry `%s --help' for a complete list of options.\n"),
@@ -495,7 +495,7 @@ suggest_asking_for_help (void)
   exit (EXIT_FAILURE);
 }
 
-void
+static void
 print_help (void)
 {
   printf (_("Usage: %s [OPTION]... [INFO-FILE [DIR-FILE]]\n"), progname);
@@ -641,7 +641,7 @@ The first time you invoke Info you start off looking at this node.\n\
    Return the actual name of the file we tried to open in
    OPENED_FILENAME and the compress program to (de)compress it in
    COMPRESSION_PROGRAM. */
-FILE *
+static FILE *
 open_possibly_compressed_file (char *filename,
     void (*create_callback) (char *),
     char **opened_filename, char **compression_program) 
@@ -864,7 +864,7 @@ determine_file_type:
    into COMPRESSION_PROGRAM (if that is non-NULL).  If trouble, return
    a null pointer. */
 
-char *
+static char *
 readfile (char *filename, int *sizep,
     void (*create_callback) (char *), char **opened_filename,
     char **compression_program)
@@ -1054,7 +1054,7 @@ output_dirfile (char *dirfile, int dir_nlines, struct line_data *dir_lines,
    is recorded in next->entry_sections and next->entry_sections_tail, where
    next is the new entry.  Return the number of entries to add from this 
    file.  */
-int
+static int
 parse_input (const struct line_data *lines, int nlines,
              struct spec_section **sections, struct spec_entry **entries,
              int delete_flag) 
@@ -1286,7 +1286,7 @@ parse_dir_file (struct line_data *lines, int nlines, struct node **nodes)
    that matches NAME.  If such an entry is found, flag the entry for 
    deletion later on. */
 
-int
+static int
 mark_entry_for_deletion (struct line_data *lines, int nlines, char *name)
 {
   int something_deleted = 0;
@@ -1671,7 +1671,7 @@ reformat_new_entries (struct spec_entry *entries, int calign_cli, int align_cli,
    NAME is the basename of the Info file being installed. 
    The idea here is that there was a --name on the command-line
    and we need to put the basename in the empty parentheses. */
-void
+static void
 add_missing_basenames (struct spec_entry *entries, char *name)
 {
   struct spec_entry *entry;
@@ -1706,7 +1706,7 @@ add_missing_basenames (struct spec_entry *entries, char *name)
 /* Add NAME to the start of any entry in ENTRIES that is missing a name 
    component.  If NAME doesn't start with `*', it is formatted to look 
    like an Info entry.  */
-void
+static void
 add_missing_names (struct spec_entry *entries, char *name)
 {
   struct spec_entry *entry;
@@ -1746,7 +1746,7 @@ add_missing_names (struct spec_entry *entries, char *name)
 
 /* Append DESC to every entry in ENTRIES that needs it. */
 
-void
+static void
 add_missing_descriptions (struct spec_entry *entries, char *desc)
 {
   struct spec_entry *entry;
-- 
2.38.1


[-- Attachment #1.3: Type: text/plain, Size: 45 bytes --]


Have a lovely day.
-- 
Arsen Arsenović

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 381 bytes --]

Re: -Wlto-type-mismatch warning in error()

[Eli Zaretskii]

> From: Arsen Arsenović <arsen@aarsen.me>
> Cc: Bruno Haible <bruno@clisp.org>, GavinSmith0123@gmail.com,
>  sam@gentoo.org, bug-texinfo@gnu.org, bug-gnulib@gnu.org
> Date: Thu, 08 Dec 2022 09:25:01 +0100
> 
> > Shouldn't we report this to the GCC folks?  It could be a bug in lto,
> > no?  I mean, 'error' is not a symbol that applications cannot use, and
> > if an application defines a function by that name, it should not be
> > imported from the standard library.  Right?
> 
> I believe this would make them part of the same program.

It shouldn't: the linker should only pull functions that it cannot
resolve until it gets to processing the library.  And 'error' should
have been resolved already to the version that is in install-info.c.

> On top of that, Gnulib is pulling in error anyway:
> 
> $ nm ./gnulib/lib/libgnu.a | grep error
>                  U error
> $ nm install-info.o ../gnulib/lib/libgnu.a |& grep '\<error\>'
> 00000000 T error
>          U error

Not sure what you wanted to demonstrate with this.  The above says
that install-info.o includes the implementation of 'error', whereas
libgnu.a only references 'error' as an unresolved symbol (which should
then be resolved by the linker).

> My guess is that libgnu_a-xalloc-die.o (the file emitting the U error
> symbol) includes gnulib/lib/error.h, GCC records that declaration
> (through it's use in xalloc_die), and then detects a mismatch with the
> one emitted by install-info.o (the T error symbol) and hence warns.

The "U" has nothing to do with declaration, it is there because
xalloc-die.c calls 'error', and the compiler sees the call.  AFAIU,
Gnulib's intention in the above case is that this unresolved call will
be resolved by linking against libc.

> I imagine this would result is some very strange runtime failures if
> anyone ever observed install-info hit an xalloc_die condition.

Not if the reference in xalloc_die was resolved to the libc function
by the same name.

> As a test, building on musl (which lacks the error API, for some reason)
> causes the executable to be compiled with the install-info error, rather
> than the Gnulib one, demonstrating why this warning happens.
> 
> Attempting to --whole-archive link on that platform grants us:
> 
> $ x86_64-linux-musl-gcc -o ginstall-info install-info.o \
>   -Wl,--whole-archive ../gnulib/lib/libgnu.a -Wl,--no-whole-archive
> /usr/libexec/gcc/x86_64-linux-musl/ld: ../gnulib/lib/libgnu.a(libgnu_a-error.o): in function `error':
> error.c:(.text+0xe0): multiple definition of `error'; install-info.o:install-info.c:(.text+0x4a0): first defined here
> collect2: error: ld returned 1 exit status

Now _that_ IMO is a problem with Gnulib's use in this case: Gnulib
evidently assumes that no application will define its own 'error'
function, something that applications are free to do.

> I imagine a similar thing would happen with a static glibc link.
> Renaming the functions or adding ``static'' elides this issue.

IMO, doing that sweeps the problem under the carpet, without solving
it.  We should try finding a better solution.

> So, GCC appears to be doing the right thing.

I'm not convinced it does.

> Since I went through the process of making all the symbols in that file
> (besides main) local, here's the patch that does that, though it's based
> on a not-particularly-clean head (so, ChangeLog might conflict):

It's up to Gavin, but a change like this means install-info will never
be able to have more than one source file, with calls between these
multiple files.  E.g., should install-info acquire a new source file
called, say, utils.c, the functions in utils.c will be unable to call
the function 'error' defined in install-info.c.

So I don't think this kind of change is TRT, certainly not in general.

In general, I believe certain names used by a Standard C Library are
"reserved", and applications must not redefine them.  But 'error' is
not one of those reserved names, AFAIK.  So an application is in its
full rights when it defines its own 'error' that is not compatible
with that from libc.

Re: -Wlto-type-mismatch warning in error()

[Arsen Arsenović]

[-- Attachment #1: Type: text/plain, Size: 6237 bytes --]

Hi,

Eli Zaretskii <eliz@gnu.org> writes:

>> On top of that, Gnulib is pulling in error anyway:
>> 
>> $ nm ./gnulib/lib/libgnu.a | grep error
>>                  U error
>> $ nm install-info.o ../gnulib/lib/libgnu.a |& grep '\<error\>'
>> 00000000 T error
>>          U error
>
> Not sure what you wanted to demonstrate with this.  The above says
> that install-info.o includes the implementation of 'error', whereas
> libgnu.a only references 'error' as an unresolved symbol (which should
> then be resolved by the linker).

I was trying to say that the files being linked (in this case,
install-info.o and libgnu.a) already contain calls to error, the call
above is in lib/xalloc-die.c, which is compiled against the declaration
in lib/error.h, both in Gnulib.  Due to LTO being enabled, that call
would record error as the type error.h declares it:

  void error (int __status, int __errnum, const char *__format, ...)

Later, LTO mushes all that together, sees an undefined reference to
``error'', and sees a definition for it, compares the type, and sees
that they are mismatched, hence the warning.

>> My guess is that libgnu_a-xalloc-die.o (the file emitting the U error
>> symbol) includes gnulib/lib/error.h, GCC records that declaration
>> (through it's use in xalloc_die), and then detects a mismatch with the
>> one emitted by install-info.o (the T error symbol) and hence warns.
>
> The "U" has nothing to do with declaration, it is there because
> xalloc-die.c calls 'error', and the compiler sees the call.  AFAIU,
> Gnulib's intention in the above case is that this unresolved call will
> be resolved by linking against libc.

Indeed.

>> I imagine this would result is some very strange runtime failures if
>> anyone ever observed install-info hit an xalloc_die condition.
>
> Not if the reference in xalloc_die was resolved to the libc function
> by the same name.

It isn't:

  (gdb) info symbol 0x55555555a1b0
  error in section .text of .../install-info/ginstall-info
  (gdb) info symbol 0x7ffff7eb56c0
  error_at_line in section .text of /usr/lib64/libc.so.6
  (gdb) # symbol values obtained via p &...

And it wouldn't be, since the executable contains:

   76: 00000000000061b0    249 FUNC    GLOBAL DEFAULT       14 error
   68: 00000000000061b0    249 FUNC    GLOBAL DEFAULT       14 error

>> As a test, building on musl (which lacks the error API, for some reason)
>> causes the executable to be compiled with the install-info error, rather
>> than the Gnulib one, demonstrating why this warning happens.
>> 
>> Attempting to --whole-archive link on that platform grants us:
>> 
>> $ x86_64-linux-musl-gcc -o ginstall-info install-info.o \
>>   -Wl,--whole-archive ../gnulib/lib/libgnu.a -Wl,--no-whole-archive
>> /usr/libexec/gcc/x86_64-linux-musl/ld: ../gnulib/lib/libgnu.a(libgnu_a-error.o): in function `error':
>> error.c:(.text+0xe0): multiple definition of `error'; install-info.o:install-info.c:(.text+0x4a0): first defined here
>> collect2: error: ld returned 1 exit status
>
> Now _that_ IMO is a problem with Gnulib's use in this case: Gnulib
> evidently assumes that no application will define its own 'error'
> function, something that applications are free to do.

That is fair, yes.  It'd be a bit difficult to fix this issue, I think,
in Gnulib, since there's two cases:

- The system exposes error (), Gnulib doesn't touch it
- The system lacks error (), Gnulib provides it.

In the latter instance, Gnulib could just use a private alias
(e.g. gl_error) to call error(3), but that wouldn't work in the former
case because Glibc (et al.) lacks such an alias.  Aliases and static
libraries also might not mix as we'd want them to here.  I cannot
remember.

[[ Note that in the above musl case, (I lost the backlog) the function]]
[[ gets in the final executable is install-info.c:error rather than   ]]
[[ Gnulib error, so xalloc_die would behave the same wrong way there. ]]

>> I imagine a similar thing would happen with a static glibc link.
>> Renaming the functions or adding ``static'' elides this issue.
>
> IMO, doing that sweeps the problem under the carpet, without solving
> it.  We should try finding a better solution.

In that case, renaming the error () function in install-info is likely a
better path forward.

>> So, GCC appears to be doing the right thing.
>
> I'm not convinced it does.

It might be best to ask about this on gcc@gcc.gnu.org then, though I do
believe it's acting as it ought to be, seeing as the program is
referring to two different ``error''s in the same namespace.

>> Since I went through the process of making all the symbols in that file
>> (besides main) local, here's the patch that does that, though it's based
>> on a not-particularly-clean head (so, ChangeLog might conflict):
>
> It's up to Gavin, but a change like this means install-info will never
> be able to have more than one source file, with calls between these
> multiple files.  E.g., should install-info acquire a new source file
> called, say, utils.c, the functions in utils.c will be unable to call
> the function 'error' defined in install-info.c.
>
> So I don't think this kind of change is TRT, certainly not in general.

Indeed, if Gavin prefers, simply renaming the function would have the
benefit of being less work later, potentially.

> In general, I believe certain names used by a Standard C Library are
> "reserved", and applications must not redefine them.  But 'error' is
> not one of those reserved names, AFAIK.  So an application is in its
> full rights when it defines its own 'error' that is not compatible
> with that from libc.

Indeed, and if Gnulib is considered part of the said application, and it
usually acts mostly as if it were, this application (ginstall-info)
would have two conflicting ideas of what ``error'' means, and hence
getting this warning.

Have a lovely day.

[[ PS: I wrote this draft hours ago, and I only got to send it now. ]]
[[ Something came up in the meanwhile.  Sentences written based on  ]]
[[ Memory are marked with these brackets.  They could be wrong.     ]]
-- 
Arsen Arsenović

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 381 bytes --]

Re: namespacing issues with Gnulib

[Bruno Haible]

Eli Zaretskii wrote:
> > Attempting to --whole-archive link on that platform grants us:
> > 
> > $ x86_64-linux-musl-gcc -o ginstall-info install-info.o \
> >   -Wl,--whole-archive ../gnulib/lib/libgnu.a -Wl,--no-whole-archive
> > /usr/libexec/gcc/x86_64-linux-musl/ld: ../gnulib/lib/libgnu.a(libgnu_a-error.o): in function `error':
> > error.c:(.text+0xe0): multiple definition of `error'; install-info.o:install-info.c:(.text+0x4a0): first defined here
> > collect2: error: ld returned 1 exit status
> 
> Now _that_ IMO is a problem with Gnulib's use in this case: Gnulib
> evidently assumes that no application will define its own 'error'
> function, something that applications are free to do.
> ...
> In general, I believe certain names used by a Standard C Library are
> "reserved", and applications must not redefine them.  But 'error' is
> not one of those reserved names, AFAIK.  So an application is in its
> full rights when it defines its own 'error' that is not compatible
> with that from libc.

The standards make namespacing statements only w.r.t. the libc vs. the
application. From the point of view of the standards, install-info.c and
the Gnulib code are both on the application side; therefore the standards
cannot help resolving a conflict between install-info.c and Gnulib code.

What are the possible ways to resolve the conflict?

(A) Could Gnulib define the symbol 'error' as weak?

    The concept of "weak" symbols exists only in ELF; therefore this
    approach would be non-portable (not working with Windows DLLs or
    COFF or MachO binary formats).

(B) An ad-hoc solution: Near the top of install-info.c, add
      #define error glibc_compatible_error
    and later, after all #includes:
      #undef error

    This is ugly, but it is cheap. And it does not cause problems if
    install-info.c is split into several .c files in the future.

(C) gnulib-tool could infer from the command-line arguments that the
    module 'error' is only indirectly used, i.e. not explicitly requested,
    and then arrange for the gnulib-lib subdirectory to have a config.h
    that does

       #include "../config.h"    /* include definitions from autoconf */
       /* put indirectly used symbols into a namespace */
       #define error libgnu_error

    This approach is more generic, but
      - It is not (yet) implemented in gnulib-tool.
      - "make" will compile most Gnulib-derived object files twice, once
        to detect which symbols to redefine in config.h, and once for real.

    So, with this approach, the build times of your package are increased.

Any other ideas?

Bruno

Re: namespacing issues with Gnulib

[Eli Zaretskii]

> From: Bruno Haible <bruno@clisp.org>
> Cc: GavinSmith0123@gmail.com, sam@gentoo.org, bug-texinfo@gnu.org, bug-gnulib@gnu.org
> Date: Thu, 08 Dec 2022 20:00:31 +0100
> 
> Eli Zaretskii wrote:
> > > Attempting to --whole-archive link on that platform grants us:
> > > 
> > > $ x86_64-linux-musl-gcc -o ginstall-info install-info.o \
> > >   -Wl,--whole-archive ../gnulib/lib/libgnu.a -Wl,--no-whole-archive
> > > /usr/libexec/gcc/x86_64-linux-musl/ld: ../gnulib/lib/libgnu.a(libgnu_a-error.o): in function `error':
> > > error.c:(.text+0xe0): multiple definition of `error'; install-info.o:install-info.c:(.text+0x4a0): first defined here
> > > collect2: error: ld returned 1 exit status
> > 
> > Now _that_ IMO is a problem with Gnulib's use in this case: Gnulib
> > evidently assumes that no application will define its own 'error'
> > function, something that applications are free to do.
> > ...
> > In general, I believe certain names used by a Standard C Library are
> > "reserved", and applications must not redefine them.  But 'error' is
> > not one of those reserved names, AFAIK.  So an application is in its
> > full rights when it defines its own 'error' that is not compatible
> > with that from libc.
> 
> The standards make namespacing statements only w.r.t. the libc vs. the
> application. From the point of view of the standards, install-info.c and
> the Gnulib code are both on the application side; therefore the standards
> cannot help resolving a conflict between install-info.c and Gnulib code.

The original complaint was about building install-info on GNU/Linux,
where 'error' is supposed to come from glibc, not from Gnulib.  And
that's the situation about which I thought when I asked whether LTO
has a bug; in that case, we _are_ talking abouyt an application vs an
implementation of the Standard C Library.

Re: namespacing issues with Gnulib

[Arsen Arsenović]

[-- Attachment #1: Type: text/plain, Size: 242 bytes --]


Bruno Haible <bruno@clisp.org> writes:

> Any other ideas?

The one that pops to mind is prefixing everything with ``gl_'', and
exposing aliases maybe.  The former is unergonomic and latter is
fragile, though.
-- 
Arsen Arsenović

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 381 bytes --]

Re: namespacing issues with Gnulib

[Paul Eggert]

On 2022-12-08 12:19, Arsen Arsenović wrote:
> The one that pops to mind is prefixing everything with ``gl_'', and
> exposing aliases maybe.  The former is unergonomic and latter is
> fragile, though.

Yes, my take on this is to leave Gnulib alone, and change the app to not 
attempt to redefine Gnulib's symbols. Which is what appears to be happening.

This is one of the "advantages" of using a source-code library like Gnulib.

Re: -Wlto-type-mismatch warning in error()

[Gavin Smith]

On Thu, Dec 08, 2022 at 09:25:01AM +0100, Arsen Arsenović wrote:
> > Shouldn't we report this to the GCC folks?  It could be a bug in lto,
> > no?  I mean, 'error' is not a symbol that applications cannot use, and
> > if an application defines a function by that name, it should not be
> > imported from the standard library.  Right?
> 
> I believe this would make them part of the same program.  On top of
> that, Gnulib is pulling in error anyway:
> 
> $ nm ./gnulib/lib/libgnu.a | grep error
>                  U error
> $ nm install-info.o ../gnulib/lib/libgnu.a |& grep '\<error\>'
> 00000000 T error
>          U error
> 
> My guess is that libgnu_a-xalloc-die.o (the file emitting the U error
> symbol) includes gnulib/lib/error.h, GCC records that declaration
> (through it's use in xalloc_die), and then detects a mismatch with the
> one emitted by install-info.o (the T error symbol) and hence warns.
> 
> I imagine this would result is some very strange runtime failures if
> anyone ever observed install-info hit an xalloc_die condition.

Your analysis makes sense here.  I have committed a change to make it
static.  I have also cherry-picked this commit to the release/7.0
branch in case another release is made from this branch.

Unless there is some way in Gnulib to prefer the glibc symbols when
linking, this seems unavoidable.  Defining the Gnulib symbols as "weak"
wouldn't help; as Arsen has said, it is the definition in install-info.c
itself that shouldn't be used when resolving the reference to "error"
in libgnu.a.

Making the symbols provided by install-info.c weak might work,
so one idea is that when a program uses Gnulib, all of the global
symbols from the program (excluding Gnulib) should be marked as weak
in produced object files, so that Gnulib code preferentially uses
code from glibc or other libraries.  I have no idea what would be
needed to achieve this or what other implications there might be.
(This won't help if the symbol is weak in those libraries too, though.)

I consulted some documentation on the ELF format but there appears
only to be one type of "undefined" symbol - it wouldn't be possible
to make the undefined symbols in libgnu.a preferentially resolve to
glibc symbols rather than other files in the program.   I'm very ignorant
of these matters though so it possible I missed something.

Likewise something might also be possible with how the linker (ld) is run
but somebody would have to research this.

> Since I went through the process of making all the symbols in that file
> (besides main) local, here's the patch that does that

Thanks but no thanks.  install-info.c is a single-file program so there's
no point in adding the static keyword everywhere.

Re: -Wlto-type-mismatch warning in error()

[Gavin Smith]

On Sat, Dec 10, 2022 at 01:48:23AM +0000, Gavin Smith wrote:
> Making the symbols provided by install-info.c weak might work,
> so one idea is that when a program uses Gnulib, all of the global
> symbols from the program (excluding Gnulib) should be marked as weak
> in produced object files, so that Gnulib code preferentially uses
> code from glibc or other libraries.  I have no idea what would be
> needed to achieve this or what other implications there might be.
> (This won't help if the symbol is weak in those libraries too, though.)

On second thought this would be a bad idea in case the program uses
symbols that clash with library symbols that are marked weak.

Re: -Wlto-type-mismatch warning in error()

[Paul Eggert]

On 2022-12-09 17:48, Gavin Smith wrote:
>> Since I went through the process of making all the symbols in that file
>> (besides main) local, here's the patch that does that
> Thanks but no thanks.  install-info.c is a single-file program so there's
> no point in adding the static keyword everywhere.

On the contrary, not only can adding 'static' prevent future issues like 
this with other symbols, it can help compilers generate better code in 
cases where adding 'static' lets them easily infer that a function 
cannot be called from outside the current compilation unit.

As a general rule in C, it's good to make all symbols static unless they 
really need to be extern.

Re: -Wlto-type-mismatch warning in error()

[Jeffrey Walton]

On Thu, Dec 8, 2022 at 3:58 AM Arsen Arsenović via Gnulib discussion
list <bug-gnulib@gnu.org> wrote:
>
> Eli Zaretskii <eliz@gnu.org> writes:
>
> >> Whereas with the Gnulib 'error' module, there is a conflict between the
> >> two global function definitions (with 'T' linkage) in install-info.c and
> >> in error.c *always*.
> >>
> >> > The simplest way to fix this problem would probably be to rename the "error"
> >> > function in install-info.c.
> >>
> >> Yes, or make it 'static' (like Arsen suggested).
> >
> > Shouldn't we report this to the GCC folks?  It could be a bug in lto,
> > no?  I mean, 'error' is not a symbol that applications cannot use, and
> > if an application defines a function by that name, it should not be
> > imported from the standard library.  Right?
>
> I believe this would make them part of the same program.  On top of
> that, Gnulib is pulling in error anyway:
>
> $ nm ./gnulib/lib/libgnu.a | grep error
>                  U error
> $ nm install-info.o ../gnulib/lib/libgnu.a |& grep '\<error\>'
> 00000000 T error
>          U error
>
> My guess is that libgnu_a-xalloc-die.o (the file emitting the U error
> symbol) includes gnulib/lib/error.h, GCC records that declaration
> (through it's use in xalloc_die), and then detects a mismatch with the
> one emitted by install-info.o (the T error symbol) and hence warns.
>
> I imagine this would result is some very strange runtime failures if
> anyone ever observed install-info hit an xalloc_die condition.
>
> As a test, building on musl (which lacks the error API, for some reason)
> causes the executable to be compiled with the install-info error, rather
> than the Gnulib one, demonstrating why this warning happens.
>
> Attempting to --whole-archive link on that platform grants us:
>
> $ x86_64-linux-musl-gcc -o ginstall-info install-info.o \
>   -Wl,--whole-archive ../gnulib/lib/libgnu.a -Wl,--no-whole-archive
> /usr/libexec/gcc/x86_64-linux-musl/ld: ../gnulib/lib/libgnu.a(libgnu_a-error.o): in function `error':
> error.c:(.text+0xe0): multiple definition of `error'; install-info.o:install-info.c:(.text+0x4a0): first defined here
> collect2: error: ld returned 1 exit status
>
> I imagine a similar thing would happen with a static glibc link.
> Renaming the functions or adding ``static'' elides this issue.
>
> So, GCC appears to be doing the right thing.
>
> Since I went through the process of making all the symbols in that file
> (besides main) local, here's the patch that does that, though it's based
> on a not-particularly-clean head (so, ChangeLog might conflict):
>

I believe multiple definitions with the definitions in disagreement is
undefined behavior. https://stackoverflow.com/a/34986350 .

Jeff

Re: -Wlto-type-mismatch warning in error()

[Florian Weimer]

* Bruno Haible:

> Gavin Smith wrote:
>> I expect it is not a gnulib problem.  install-info/install-info.c declares
>> a function called "error" which appears to clash with a function from glibc.
>> ... The "error" module is brought in by "xalloc" (which we
>> ask for explicitly).
>
> Yes, I think the problems already exists without use of Gnulib, as it's
> a conflict between install-info.c and glibc. But with the Gnulib 'error'
> module, the problem becomes bigger.
>
> Namely, see:
>
> $ nm --dynamic /lib/x86_64-linux-gnu/libc.so.6 | grep ' error'
> 00000000001214e0 W error@@GLIBC_2.2.5
> 0000000000121700 W error_at_line@@GLIBC_2.2.5
> 0000000000221484 B error_message_count@@GLIBC_2.2.5
> 0000000000221480 B error_one_per_line@@GLIBC_2.2.5
> 0000000000221488 B error_print_progname@@GLIBC_2.2.5
>
> glibc exports 'error' as a weak symbol. This means, without use of Gnulib,
> the symbol from install-info.c overrides the one from glibc, and there is
> a problem if and only if the 'install-info' program links dynamically
> (or loads via 'dlopen') a shared library which happens to use error()
> and expects the semantics from glibc.

Note that weak vs non-weak does not matter for ELF dynamic linking.  The
definition in the main program always interposes those present in
depended-upon libraries.  This is necessary so that we can add functions
to glibc without an implementation-defined namespace prefix and support
multiple, conflicting standards in parallel (e..g, pthread_create is
reserved in POSIX, but not in C).

The weak flag for symbols is merely and artifact of some internal glibc
symbol management macros.

glibc does not participate in LTO, either, so it shouldn't matter here
at all.

> Whereas with the Gnulib 'error' module, there is a conflict between the
> two global function definitions (with 'T' linkage) in install-info.c and
> in error.c *always*.
>
>> The simplest way to fix this problem would probably be to rename the "error"
>> function in install-info.c.
>
> Yes, or make it 'static' (like Arsen suggested).

Right.

Thanks,
Florian