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

[Arsen Arsenović]

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):

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); 

 /* 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;
 

 /* 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, ...)

 /* 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)
 
 

-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

Have a lovely day.
-- 
Arsen Arsenović

signature.asc
Description: PGP signature