Re: [PATCH 0/3] yacc: compute the best type for the state number

[Akim Demaille]

Hi!

> Le 3 oct. 2019 à 20:20, Paul Eggert <address@hidden> a écrit :
> 
> On 10/2/19 11:28 PM, Akim Demaille wrote:
> 
>> Please, in the future, make smaller commits, per topic.
> 
> Yes, I'll try to split things up better.

Thanks.


>>  And run the test suite with an old compiler too.
> How old? Are we backward compatible to (say) GCC 3.4.3? That's the oldest I 
> have convenient access to, though it's on reaally slow hardware.

Not that old.  I wish I could easily reach older compilers but so far, I'm 
going back to Clang 3.3 and GCC 4.6.  That's all I managed to set up on Travis. 
 I would also like to try tcc, but only to run the tests, not build bison 
itself.  Travis also supports MSVC; when I feel brave enough, I'll see if I can 
use it for Bison.

Also, don't forget about ./configure --enable-gcc-warnings.


> The Bison manual says that Bison itself needs only C99, and that 
> Bison-generated C parsers need only C89 (or C99 for glr) or C++98, but I 
> don't know how that relates to compiler (and presumably library) version 
> numbers.

Can't help you here.

> At some point I suppose we should drop support for C89 as it is now more 
> trouble than it's worth. But that can wait.

Agreed.


>> I don't want to break the API for the generated locations in C++.  At least 
>> we need to talk about this with users.
> 
> Yes, that makes sense. Also, if we're going to change the API for line and 
> column numbers, I suggest we go to a wider type instead of stopping at 'int'. 
> These days, 'int' and 'unsigned' are both too narrow for some possible 
> applications of Bison, and the only portable-to-all-modern-C solutions that 
> are signed are 'intmax_t' and 'long long'. I could go with either of those.

Sounds good.

> How can we change the API to switch from 'unsigned' to 'intmax_t' (or to 
> 'long long') without breaking existing apps?

We can introduce a %define variable to enable the new type for 
columns/locations, and hook it to %require 3.5 to promote the conversion.

> I don't have enough C++ expertise to know how to do this in the best way.

We could play with overloading to support both types, but it will probably be 
nasty to disambiguate, to document.  I think we should rather address that at 
generation time.


>> [...] clutters the documentation.
> 
> OK, I changed it back, and put in a little note saying "this code assumes 
> that malloc and realloc calls always succeed" or something like that. See the 
> first attached patch. The second attachment fixes a lack of clarity with the 
> documentation of malloc elsewhere, which I noticed with the first patch.

Both are fine, thanks!

>> Old compilers choke on the current code in master, we have to stop and focus 
>> on this first.
> 
> I can help with that if you'll give me advice about how old to worry about 
> (as long as it's not before GCC 3.4.3 :-).


There are several types of errors.  Some are due to your removing some casts, 
and replacing them with pragmas.  Actually, I don't understand what you meant:

> /* Suppress bogus -Wconversion warnings from GCC.  */
> #if 4 < __GNUC__ + (7 <= __GNUC_MINOR__)
> # define YY_CONVERT_INT_BEGIN \
>     _Pragma ("GCC diagnostic push") \
>     _Pragma ("GCC diagnostic ignored \"-Wconversion\"")
> # define YY_CONVERT_INT_END \
>     _Pragma ("GCC diagnostic pop")
> #else
> # define YY_CONVERT_INT_BEGIN
> # define YY_CONVERT_INT_END
> #endif
> 
>     typedef yytype_int8 yy_state_num;
>     int yystate;
>     yy_state_num *yyssp;
> 
>   YY_CONVERT_INT_BEGIN
>   *yyssp = yystate;
>   YY_CONVERT_INT_END

Why would a warning here be bogus?  It looks to me like a perfect place to use 
a good old cast, as we're narrowing the type.  And that would help fixing the 
warnings we get from clang.  For instance:

Clang 3.3 (https://api.travis-ci.org/v3/job/592926546/log.txt)

> 25. input.at:1201: testing Torturing the Scanner ...
> ../../tests/input.at:1205: COLUMNS=1000; export COLUMNS; 
> VALGRIND_OPTS="$VALGRIND_OPTS --leak-check=summary --show-reachable=no"; 
> export VALGRIND_OPTS;  bison --color=no -fno-caret input.y
> ../../tests/input.at:1213: COLUMNS=1000; export COLUMNS; 
> VALGRIND_OPTS="$VALGRIND_OPTS --leak-check=summary --show-reachable=no"; 
> export VALGRIND_OPTS;  bison --color=no -fno-caret -fcaret  input.y
> ../../tests/input.at:1343: COLUMNS=1000; export COLUMNS;  bison --color=no 
> -fno-caret -d -v -o input.c input.y
> ../../tests/input.at:1344: $CC $CFLAGS $CPPFLAGS  -c -o input.o input.c 
> stderr:
> input.c:1166:12: error: implicit conversion loses integer precision: 'int' to 
> 'yy_state_num' (aka 'signed char') [-Werror,-Wconversion]
>   *yyssp = yystate;
>          ~ ^~~~~~~
> 1 error generated.
> stdout:
> ../../tests/input.at:1344: exit code was 1, expected 0
> 25. input.at:1201: 25. Torturing the Scanner (input.at:1201): FAILED 
> (input.at:1344)


Clang 7 (https://api.travis-ci.org/v3/job/592926536/log.txt)

> 25. input.at:1201: testing Torturing the Scanner ...
> [...]
> ../../tests/input.at:1344: $CC $CFLAGS $CPPFLAGS  -c -o input.o input.c 
> stderr:
> input.c:1166:12: error: implicit conversion loses integer precision: 'int' to 
> 'yy_state_num' (aka 'signed char') [-Werror,-Wconversion]
>   *yyssp = yystate;
>          ~ ^~~~~~~
> 1 error generated.


GCC 8 with ASAN (https://api.travis-ci.org/v3/job/592926526/log.txt):

> 25. input.at:1201: testing Torturing the Scanner ...
> [...]
> stderr:
> input.c:1166:12: error: implicit conversion loses integer precision: 'int' to 
> 'yy_state_num' (aka 'signed char') [-Werror,-Wimplicit-int-conversion]
>   *yyssp = yystate;
>          ~ ^~~~~~~
> 1 error generated.




We have other types of errors: for instance:

GCC 8 with ASAN (https://api.travis-ci.org/v3/job/592926526/log.txt):

> 113. output.at:293: testing Output file name: `~!@#$%^&*()-=_+{}[]|\:;<>, .' 
> ...
> ../../tests/output.at:293: touch "\`~!@#\$%^&*()-=_+{}[]|\\:;<>, .'.tmp" || 
> exit 77
> ../../tests/output.at:293: COLUMNS=1000; export COLUMNS;  bison --color=no 
> -fno-caret -o "\`~!@#\$%^&*()-=_+{}[]|\\:;<>, .'.c" 
> --defines="\`~!@#\$%^&*()-=_+{}[]|\\:;<>, .'.h" glr.y
> ../../tests/output.at:293: ls "\`~!@#\$%^&*()-=_+{}[]|\\:;<>, .'.c" 
> "\`~!@#\$%^&*()-=_+{}[]|\\:;<>, .'.h"
> stdout:
> `~!@#$%^&*()-=_+{}[]|\:;<>, .'.c
> `~!@#$%^&*()-=_+{}[]|\:;<>, .'.h
> ../../tests/output.at:293: $CC $CFLAGS $CPPFLAGS  -c -o glr.o -c 
> "\`~!@#\$%^&*()-=_+{}[]|\\:;<>, .'.c" 
> stderr:
> `~!@#$%^&*()-=_+{}[]|\:;<>, .'.c:1467:37: error: operand of ? changes 
> signedness: 'ptrdiff_t' (aka 'long') to 'unsigned long' 
> [-Werror,-Wsign-conversion]
>       ptrdiff_t half_max_capacity = YYSIZEMAX / (2 * sizeof yynewStates[0]);
>                                     ^~~~~~~~~ ~
> `~!@#$%^&*()-=_+{}[]|\:;<>, .'.c:132:59: note: expanded from macro 'YYSIZEMAX'
> #define YYSIZEMAX (PTRDIFF_MAX < SIZE_MAX ? PTRDIFF_MAX : (ptrdiff_t) 
> SIZE_MAX)
>                                                           ^~~~~~~~~~~~~~~~~~~~



There's also the problem of using __STDC_VERSION__ which might be undefined.  I 
addressed it with the appended patch.  The CI is currently running it 
(https://travis-ci.org/akimd/bison/builds/593375411), it should cut a large 
number of failures.



commit bc96b757ca129aa63e41aa6e5dcfbbbad3916e01
Author: Akim Demaille <address@hidden>
Date:   Fri Oct 4 06:49:40 2019 +0200

    yacc.c: fix warnings about undefined macros
    
    For instance with GCC 4.9 and --enable-gcc-warnings:
    
        25. input.at:1201: testing Torturing the Scanner ...
        ../../tests/input.at:1344: $CC $CFLAGS $CPPFLAGS  -c -o input.o input.c
        stderr:
        input.c:239:18: error: "__STDC_VERSION__" is not defined [-Werror=undef]
         # elif 199901 <= __STDC_VERSION__
                          ^
        input.c:256:18: error: "__STDC_VERSION__" is not defined [-Werror=undef]
         # elif 199901 <= __STDC_VERSION__
                          ^
    
    * data/skeletons/yacc.c: Check that __STDC_VERSION__ is defined before
    using it.

diff --git a/data/skeletons/yacc.c b/data/skeletons/yacc.c
index ce9e2865..f3ef63fe 100644
--- a/data/skeletons/yacc.c
+++ b/data/skeletons/yacc.c
@@ -419,7 +419,7 @@ typedef short yytype_int16;
 # elif defined ptrdiff_t && defined PTRDIFF_MAX
 #  define YYPTRDIFF_T ptrdiff_t
 #  define YYPTRDIFF_MAXIMUM PTRDIFF_MAX
-# elif 199901 <= __STDC_VERSION__
+# elif defined __STDC_VERSION__ && 199901 <= __STDC_VERSION__
 #  include <stddef.h> /* INFRINGES ON USER NAME SPACE */
 #  define YYPTRDIFF_T ptrdiff_t
 #  include <stdint.h> /* INFRINGES ON USER NAME SPACE */
@@ -436,7 +436,7 @@ typedef short yytype_int16;
 #  define YYSIZE_T __SIZE_TYPE__
 # elif defined size_t
 #  define YYSIZE_T size_t
-# elif 199901 <= __STDC_VERSION__
+# elif defined __STDC_VERSION__ && 199901 <= __STDC_VERSION__
 #  include <stddef.h> /* INFRINGES ON USER NAME SPACE */
 #  define YYSIZE_T size_t
 # else