bugGNU troff - Bugs: bug #59276, [PATCH] #include...

 
 

bug #59276: [PATCH] #include "config.h" before <stdio.h>

Submitted by:  Ingo Schwarze <schwarze>
Submitted on:  Thu 15 Oct 2020 01:27:04 PM UTC  
 
Category:  Core Severity:  3 - Normal
Item Group:  Build/Installation Status:  Fixed
Privacy:  Public Assigned to:  Ingo Schwarze <schwarze>
Open/Closed:  Closed Planned Release:  1.23.0

Add a New Comment (Rich Markup)
   

You are not logged in

Please log in, so followups can be emailed to you.

 

( Jump to the original submission)

Tue 27 Oct 2020 11:56:58 AM UTC, comment #7: 

Re: comment #6

Thanks, Dave, for noting that.

I decided to not waste my time replying to gnulib because:

1. They did not reply to the observation that their tests are wrong because they assume that __restrict, if available, is the same as "restrict", which is not guaranteed by the C standard.

2. They did not reply to the observation that their stdio.h violates the C++ standard.

3. They did not reply to the concern that gnulib actively and silently disables security features, even on GNU/Linux.

So, they are clearly not interested, neither in standard conformance and correctness nor in security.

It seems rather silly to me that we have to include "config.h" even in files that don't need any portability help in the first place because they are already completely portable on their own, in particular since the only reason for that requirement is that gnulib itself violates standards...  In that sense, gnulib causes more portability problems than it solves.

Ingo Schwarze <schwarze>
Project MemberIn charge of this item.
Tue 27 Oct 2020 10:37:57 AM UTC, comment #6: 

For the historical record: Ingo reported this to bug-gnulib (http://lists.gnu.org/archive/html/bug-gnulib/2020-10/msg00144.html), and the conclusions downthread were:

Dave <barx>
Project Member
Tue 20 Oct 2020 01:10:34 PM UTC, comment #5: 

Fortunately, nothing more is broken here on the side of groff or its build system.

First, note that "restrict" is a C99 keyword, but not a C++ keyword.  Consequently, in C++, the string "restrict" is available as an identifier to be used by the program (though i'm not saying that using "restrict" as an identifier in a C++ program is a good idea; it is likely to cause compatibility issues on some platforms).

The groff C++ files that i added #include "config.h" to use <stdio.h>, which is valid in C++.  The system header /usr/include/stdio.h on most operating systems typically use an implementation-specific keyword like "__restrict" or "__restrict__" specifically such that these headers can be included in C++ programs without stomping on the "restrict" identifdier in the application namespace.  So far, all is fine.

What is subtly broken is gnulib.  It is broken in the following way.  It does not test availability separately for the C and C++ languages, but does only one set of tests, and only with the C compiler, not with the C++ compiler.  It first tests whether a C99 program using "__restrict" (which is a commonly supported compiler extension) can be compiled, and if that succeeds, it adds "#define restrict __restrict" to "config.h".  That is WRONG for two reasons:

1. Theoretically, a standard-conforming C99 compiler might (unwisely) use the implementation-namespace keyword "__restrict" for different functionality.  In that case, application programs correctly using the "restrict" keyword will be miscompiled.

2. If a C++ program uses "restrict" as an identifier, that program will be miscompiled.  With most compilers, compilation will simply fail because "__restrict" can rarely be used at places where an identifier is valid.

Gnulib is also broken is so far as it uses the "restrict" keyword in its own replacement version of <stdio.h>.  Strictly speaking, that makes the gnulib <stdio.h> unusable for C++ code.  To be correct, it would instead have to use a string from the reserved namespace like "__restrict" or "__restrict__".

For practical purposes, both gnulib bugs probably matter little for groff.  Ad (2), groff does not (and should not) try to use "restrict" as an identifier.  Ad (1), i doubt that groff intends to support any compilers using "__restrict" for different purposes.

Consequently, i'm closing the ticket again.  If we want to pursue this issue further, we should take it upstream to gnulib, but i doubt that anybody is willing to deal with that repository of exceedingly convoluted code.

P.S.
Here is an example of a valid, but unwise C++ program:

 $ cat restrict.cpp                                            
int
main(void)
{
        int restrict;

        restrict = 42;
        return restrict;
}
 $ make restrict
c++ -O2 -pipe    -o restrict restrict.cpp
 $ ./restrict
 $ echo $?
42

It is not valid C:

restrict_c.c:4:6: error: restrict requires a pointer or reference
        int restrict;
restrict_c.c:6:11: error: expected identifier or '('
        restrict = 42;
restrict_c.c:7:9: error: expected expression
        return restrict;

Ingo Schwarze <schwarze>
Project MemberIn charge of this item.
Mon 19 Oct 2020 12:32:32 AM UTC, comment #4: 

Yeah, I wasn't sure how concerned to be about it, but it seemed worth noting since the original bug report limited its scope to older gcc's.  Files uploaded.

(file #50010, file #50011, file #50012)

Dave <barx>
Project Member
Sun 18 Oct 2020 11:04:03 PM UTC, comment #3: 

re comment #2:

This is unexpected and worrying.  In a nutshell, even though it does not clearly say so, what the message you are seeing means is that even your new GCC does not recognize the C99 "restrict" keyword and autoconf/gnulib replaces it with something else in your build.  That may mean that something else is wrong in addition to what i patched.

I think we should try to understand what is going on in your build to figure out whether there is an additional portability error.  Can you please post the following files from the FAILING build:

  • [build/]config.status
  • [build/]config.log
  • [build/]src/include/config.h

For comparison, with clang 10.0.1, things work for me even without this patch (as expected).  Note about the machine i tested on: clang 10.0.1 is the default compiler there, but gcc 4.2.1 is also installed for testing purposes.  The groff build decides that it somehow prefers gcc 4.2.1 over clang 10.0.1 - weird, isn't it, but whatever.  :)

 $ cc --version
OpenBSD clang version 10.0.1
Target: amd64-unknown-openbsd6.8
 $ gcc --version
gcc (GCC) 4.2.1 20070719
Copyright (C) 2007 Free Software Foundation, Inc.

Ingo Schwarze <schwarze>
Project MemberIn charge of this item.
Sun 18 Oct 2020 10:02:55 PM UTC, comment #2: 

FWIW, I hit this bug with a much newer version of gcc (9.3.0), so it's not just crusty old versions that were failing.  My output is different but it appears to be the same bug.  The update specified in comment #1 solved it.

  CXX      src/libs/libgroff/libgroff_a-cset.o
In file included from src/libs/libgroff/assert.cpp:19:
./lib/stdio.h:851:1: error: expected ',' or '...' before 'fp'
  851 | _GL_FUNCDECL_RPL (fprintf, int,
      | ^~~~~~~~~~~~~~~~
  CXX      src/libs/libgroff/libgroff_a-curtime.o
src/libs/libgroff/assert.cpp: In function 'void assertion_failed(int, const char*, const char*, const char*)':
src/libs/libgroff/assert.cpp:29:41: error: too many arguments to function 'int rpl_fprintf(FILE*)'
   29 |     fprintf(stderr, "%s: ", program_name);
      |                                         ^
In file included from src/libs/libgroff/assert.cpp:19:
./lib/stdio.h:851:1: note: declared here
  851 | _GL_FUNCDECL_RPL (fprintf, int,
      | ^~~~~~~~~~~~~~~~
src/libs/libgroff/assert.cpp:31:25: error: too many arguments to function 'int rpl_fprintf(FILE*)'
   31 |    lineno, function, msg);
      |                         ^
In file included from src/libs/libgroff/assert.cpp:19:
./lib/stdio.h:851:1: note: declared here
  851 | _GL_FUNCDECL_RPL (fprintf, int,
      | ^~~~~~~~~~~~~~~~
make[1]: *** [Makefile:7043: src/libs/libgroff/libgroff_a-assert.o] Error 1

Dave <barx>
Project Member
Sun 18 Oct 2020 02:32:12 PM UTC, comment #1: 

Fixed in fe121eeacd53c96105f23209b2c205f436f97359;
thanks to cjwatson@ for checking.

Ingo Schwarze <schwarze>
Project MemberIn charge of this item.
Thu 15 Oct 2020 01:27:04 PM UTC, original submission:  

Since the gnulib commit 182afcba2635cbff91240656c7fb3742dd23ab6f
  Author: Bruno Haible <bruno@clisp.org>
  Date: Sat Feb 22 20:57:30 2020 +0100
gnulib/lib/stdio.in.h uses the "restrict" keyword.

For example with GCC 4.2.1, this may cause the build to fail with a message like this:

  In file included from ../src/libs/libgroff/assert.cpp:19:
  ./lib/stdio.h:851: error: expected ',' or '...' before 'fp'
  ./lib/stdio.h:851: error: format string argument not a string type
  ./lib/stdio.h:851: error: nonnull argument with out-of-range operand number (argument 1, operand 2)
  ./lib/stdio.h: In function 'void assertion_failed(int, const char*, const char*, const char*)':
  ./lib/stdio.h:851: error: too many arguments to function 'int rpl_fprintf(FILE*)'
  ../src/libs/libgroff/assert.cpp:29: error: at this point in file
  ./lib/stdio.h:851: error: too many arguments to function 'int rpl_fprintf(FILE*)'
  ../src/libs/libgroff/assert.cpp:31: error: at this point in file
  *** Error 1 in . (Makefile:6874 'src/libs/libgroff/libgroff_aassert.o': @echo "  CXX     " src/libs/libgroff /libgroff_a-assert.o;g++ -DHAVE...)
  *** Error 2 in /co/groff/build (Makefile:5317 'all')

The attached patch fixes the build failure by making sure "restict" is appropriately #defined (if needed) before the gnulib version of <stdio.h> is included.

Ingo Schwarze <schwarze>
Project MemberIn charge of this item.

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attach Files:
   
   
Comment:
   

Attached Files
file #50010:  config.status added by barx (57KiB - application/octet-stream - config.* files requested in comment #3)
file #50011:  config.h added by barx (39KiB - text/x-chdr - config.* files requested in comment #3)
file #50012:  config.log added by barx (188KiB - text/x-log - config.* files requested in comment #3)
file #49983:  stdio-config.patch added by schwarze (3KiB - text/x-patch)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by barx (Posted a comment)
  • -email is unavailable- added by schwarze (Submitted the item)
  •  

    Do you think this task is very important?
    If so, you can add your encouragement to it.
    This task has 0 encouragements so far.

    Only logged-in users can vote.

     

     

     

    Follow 11 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2020-10-20 schwarze StatusNeed Info => Fixed
        Open/ClosedOpen => Closed
    2020-10-19 barx Attached File- => Added config.status, #50010
        Attached File- => Added config.h, #50011
        Attached File- => Added config.log, #50012
    2020-10-18 schwarze StatusFixed => Need Info
        Open/ClosedClosed => Open
    2020-10-18 schwarze StatusNone => Fixed
        Open/ClosedOpen => Closed
    2020-10-15 schwarze Planned ReleaseNone => 1.23.0
    2020-10-15 schwarze Attached File- => Added stdio-config.patch, #49983

    Back to the top


    Powered by Savane 3.5