Re: [PATCH] issues with configure --enable-checking option

[Roman Zhuykov <zhroma@ispras.ru>]

07.02.2020 20:20, Richard Sandiford writes:
> Roman Zhuykov <zhroma@ispras.ru> writes:
>> Hi!
>> I've investigated a bit, because some of the following confused me while 
>> working with some local 9.2-based branch.
>>
>> Documentation issues:
>> (0) See patch for install.texi at the bottom, two possible values are 
>> not documented. Ok for master? Backports?
>> (1) For me it seems confusing to have 'tree' and 'gimple' values, but 
>> not sure how to solve this.
>> (2) Developer has to look into configure scripts to understand which 
>> macro will be defined. E.q. 'misc' means "CHECKING_P".
>> (3) Install.texi IMHO doesn't properly describe what happens when 
>> --enable-checking is used without "=list". Maybe we should explicitly 
>> tell this means same as "=yes".
>> (4) Statement "This is ‘yes,extra’ by default when building from the 
>> source repository or snapshots." is wrong, because 'snapshots' may 
>> relate to released branches (e.q. GCC 9-20200125 Snapshot), and 
>> gcc/DEV-PHASE is empty there.
>> (5) Statement "‘extra’ adds for ‘misc’ checking extra checks ..." is 
>> also confusing, one can run 'configure --enable-checking=extra' and will 
>> have only ENABLE_EXTRA_CHECKING, but not CHECKING_P, and in common.opt 
>> flag_checking will have Init(0).
>>
>> Behavior issues:
>> (6) It is not obvious to have default --enable-checking=release on any 
>> commit in releases/* branches (DEV-PHASE is empty there). IMHO it's 
>> enough 'experimental' when picking for example some commit between 9.1 
>> and 9.2. This also can confuse 'git bisect run' scenario when good 
>> revision is old enough and bad revision is on release branch. See also (4).
>> (7) Running "configure --enable-checking" means less (!) checks on 
>> master branch (where DEV-PHASE is experimental). Default is "yes+extra" 
>> and you get only "yes" with that option.
>> (8) Why we always start with "release" values ('assert'+'runtime') as 
>> default? If developer runs "configure --enable-checking=df,rtl,tree" 
>> probably it should mean only picked values are enabled. Why we silently 
>> add 'assert' and 'runtime' into that set?
>>
>> I haven't tried to find additional issues with related 
>> '--enable-stage1-checking' option.
>>
>> Roman
>>
>> PS. I see some lines have more than 80 chars in install.texi, few of 
>> them were added recently while cleaning references to SVN. Patch fixes 
>> this only forthe paragraph it touches.
>> --
>>
>> gcc/ChangeLog:
>>
>> 2020-01-29  Roman Zhuykov <zhroma@ispras.ru>
>>
>>      * doc/install.texi: Document 'types' and 'gimple' values for
>>      '--enable-checking' configure option.
>>
>> diff --git a/gcc/doc/install.texi b/gcc/doc/install.texi
>> --- a/gcc/doc/install.texi
>> +++ b/gcc/doc/install.texi
>> @@ -1845,19 +1845,19 @@ consistency checks of the requested complexity.  
>> This does not change the
>>   generated code, but adds error checking within the compiler.  This will
>>   slow down the compiler and may only work properly if you are building
>>   the compiler with GCC@.  This is @samp{yes,extra} by default when building
>> -from the source repository or snapshots, but @samp{release} for 
>> releases.  The default
>> -for building the stage1 compiler is @samp{yes}.  More control
>> +from the source repository or snapshots, but @samp{release} for releases.
>> +The default for building the stage1 compiler is @samp{yes}.  More control
> Pre-existing problem, but: it looks like the current default is yes,types:
>
> [if test "x$enable_checking" = xno || test "x$enable_checking" = x; then
>   # For --disable-checking or implicit --enable-checking=release, avoid
>   # setting --enable-checking=gc in the default stage1 checking for LTO
>   # bootstraps.  See PR62077.
>   case $BUILD_CONFIG in
>     *lto*)
>       stage1_checking=--enable-checking=release,misc,gimple,rtlflag,tree,types;;
>     *)
>       stage1_checking=--enable-checking=yes,types;;
>   esac
>   if test "x$enable_checking" = x && \
>      test -d ${srcdir}/gcc && \
>      test x"`cat ${srcdir}/gcc/DEV-PHASE`" = xexperimental; then
>     stage1_checking=--enable-checking=yes,types,extra
>   fi
> else
>   stage1_checking=--enable-checking=$enable_checking,types
> fi])
>
> Could you fix that while you're there?

No change needed, documentation is correct here, that global
configure.ac yes+types change was r126951 and a bit later in r133479 we
have 'types' included into 'yes' set in gcc/configure.ac. In patch v1
I've already included 'types' into 'yes' set description few lines below.

I hope somebody will have a look at those behavior issues (6), (7), (8),
probably on stage1 later. So, lets add
(9) Remove unneeded 'types' item in stage1_checking in global configure.ac.

>>   over the checks may be had by specifying @var{list}.  The categories of
>>   checks available are @samp{yes} (most common checks
>> -@samp{assert,misc,tree,gc,rtlflag,runtime}), @samp{no} (no checks at
>> -all), @samp{all} (all but @samp{valgrind}), @samp{release} (cheapest
>> +@samp{assert,misc,tree,gc,gimple,rtlflag,runtime,types}), @samp{no} (no 
>> checks
>> +at all), @samp{all} (all but @samp{valgrind}), @samp{release} (cheapest
>>   checks @samp{assert,runtime}) or @samp{none} (same as @samp{no}).
>>   Individual checks can be enabled with these flags @samp{assert},
>> -@samp{df}, @samp{fold}, @samp{gc}, @samp{gcac}, @samp{misc}, @samp{rtl},
>> -@samp{rtlflag}, @samp{runtime}, @samp{tree}, @samp{extra} and 
>> @samp{valgrind}.
>> -@samp{extra} adds for @samp{misc} checking extra checks that might affect
>> -code generation and should therefore not differ between stage1 and later
>> -stages.
>> +@samp{df}, @samp{fold}, @samp{gc}, @samp{gcac}, @samp{gimple}, @samp{misc},
>> +@samp{rtl}, @samp{rtlflag}, @samp{runtime}, @samp{tree}, @samp{types},
>> +@samp{extra} and @samp{valgrind}. @samp{extra} adds for @samp{misc} 
>> checking
> Both of these are again pre-existing, but s/adds for/adds/.

Fixed that in patch v2 with another wording.

> Would also be good to put @samp{extra} in alphabetical order wrt the other options.

Done.

> OK with those changes,

Since I have to ask again about backports, I've decided to make few more
steps and with Alexander's help created new patch which rewords the
whole option description and covers items (3), (4) and (8).  CCing Jakub
and Richard as release managers, also ask Sandra to take a quick look if
new wording is alright.  New patch suits all active branches.  OK for
10-9-8 ?

Roman
--

gcc/ChangeLog:

2020-02-11  Roman Zhuykov  <zhroma@ispras.ru>

    * doc/install.texi: Properly document current behavior of
    '--enable-checking' and '--enable-stage1-checking' configure
    options.

diff --git a/gcc/doc/install.texi b/gcc/doc/install.texi
--- a/gcc/doc/install.texi
+++ b/gcc/doc/install.texi
@@ -1839,42 +1839,49 @@ final releases.  The specific files which get
@option{-Werror} are
 controlled by the Makefiles.
 
 @item --enable-checking
+@itemx --disable-checking
 @itemx --enable-checking=@var{list}
-When you specify this option, the compiler is built to perform internal
-consistency checks of the requested complexity.  This does not change the
-generated code, but adds error checking within the compiler.  This will
-slow down the compiler and may only work properly if you are building
-the compiler with GCC@.  This is @samp{yes,extra} by default when building
-from the source repository or snapshots, but @samp{release} for
releases.  The default
-for building the stage1 compiler is @samp{yes}.  More control
-over the checks may be had by specifying @var{list}.  The categories of
-checks available are @samp{yes} (most common checks
-@samp{assert,misc,tree,gc,rtlflag,runtime}), @samp{no} (no checks at
-all), @samp{all} (all but @samp{valgrind}), @samp{release} (cheapest
-checks @samp{assert,runtime}) or @samp{none} (same as @samp{no}).
-Individual checks can be enabled with these flags @samp{assert},
-@samp{df}, @samp{fold}, @samp{gc}, @samp{gcac}, @samp{misc}, @samp{rtl},
-@samp{rtlflag}, @samp{runtime}, @samp{tree}, @samp{extra} and
@samp{valgrind}.
-@samp{extra} adds for @samp{misc} checking extra checks that might affect
-code generation and should therefore not differ between stage1 and later
-stages.
-
-The @samp{valgrind} check requires the external @command{valgrind}
-simulator, available from @uref{http://valgrind.org/}.  The
-@samp{df}, @samp{rtl}, @samp{gcac} and @samp{valgrind} checks are very
expensive.
-To disable all checking, @samp{--disable-checking} or
-@samp{--enable-checking=none} must be explicitly requested.  Disabling
-assertions will make the compiler and runtime slightly faster but
-increase the risk of undetected internal errors causing wrong code to be
-generated.
+This option controls performing internal consistency checks in the
compiler.
+It does not change the generated code, but adds error checking of the
+requested complexity.  This will slow down the compiler and may only work
+properly if you are building the compiler with GCC@.
+
+When the option is not specified, the active set of checks depends on
context.
+Namely, bootstrap stage 1 defaults to @samp{--enable-checking=yes}, builds
+from release archives default to @samp{--enable-checking=release}, and
+otherwise @samp{--enable-checking=yes,extra} is used.  When the option is
+specified without a @var{list}, the result is the same as
+@samp{--enable-checking=yes}.  Likewise, @samp{--disable-checking} is
+equivalent to @samp{--enable-checking=no}.
+
+The categories of checks available in @var{list} are @samp{yes} (most
common
+checks @samp{assert,misc,gc,gimple,rtlflag,runtime,tree,types}), @samp{no}
+(no checks at all), @samp{all} (all but @samp{valgrind}), @samp{release}
+(cheapest checks @samp{assert,runtime}) or @samp{none} (same as @samp{no}).
+@samp{release} checks are always on and to disable them
+@samp{--disable-checking} or @samp{--enable-checking=no[,<other checks>]}
+must be explicitly requested.  Disabling assertions will make the compiler
+and runtime slightly faster but increase the risk of undetected internal
+errors causing wrong code to be generated.
+
+Individual checks can be enabled with these flags: @samp{assert},
@samp{df},
+@samp{extra}, @samp{fold}, @samp{gc}, @samp{gcac}, @samp{gimple},
+@samp{misc}, @samp{rtl}, @samp{rtlflag}, @samp{runtime}, @samp{tree},
+@samp{types} and @samp{valgrind}.  @samp{extra} extends @samp{misc}
+checking with extra checks that might affect code generation and should
+therefore not differ between stage1 and later stages in bootstrap.
+
+The @samp{valgrind} check requires the external @command{valgrind}
simulator,
+available from @uref{http://valgrind.org/}.  The @samp{df}, @samp{rtl},
+@samp{gcac} and @samp{valgrind} checks are very expensive.
 
 @item --disable-stage1-checking
 @itemx --enable-stage1-checking
 @itemx --enable-stage1-checking=@var{list}
-If no @option{--enable-checking} option is specified the stage1
-compiler will be built with @samp{yes} checking enabled, otherwise
-the stage1 checking flags are the same as specified by
-@option{--enable-checking}.  To build the stage1 compiler with
+This option affects only bootstrap build.  If no @option{--enable-checking}
+option is specified the stage1 compiler will be built with @samp{yes}
+checking enabled, otherwise the stage1 checking flags are the same as
+specified by @option{--enable-checking}.  To build the stage1 compiler with
 different checking options use @option{--enable-stage1-checking}.
 The list of checking options is the same as for @option{--enable-checking}.
 If your system is too slow or too small to bootstrap a released compiler