Re: [Version 3][PATCH][gcc][PR94230]provide an option to change the size limitation for -Wmisleading-indent

[David Malcolm <dmalcolm@redhat.com>]

On Thu, 2020-04-23 at 16:05 -0500, Qing Zhao wrote:
> Hi, Richard,
> 
> This is the 3rd version of the patch, updated based on your previous
> comments.
> 
> Please take a look at it and let me know whether it’s okay to commit?
> 
> Thanks a lot for all your help.
> 
> Qing.
> 
> gcc/ChangeLog:
> 
> 2020-04-22  qing zhao  <qing.zhao@oracle.com>
> 
> 	PR c/94230
> 	* common.opt: Add -flarge-source-files.
> 	* doc/invoke.texi: Document it.
> 	* toplev.c (process_options): set line_table-
> >default_range_bits
> 	to 0 when flag_large_source_files is true.
> 
> 
> gcc/c-family/ChangeLog:
> 
> 2020-04-22  qing zhao  <qing.zhao@oracle.com>
> 
> 	PR c/94230
> 	* c-indentation.c (get_visual_column): Add a hint to use the
> new
> 	-flarge-source-files option.
> 
> gcc/testsuite/ChangeLog:
> 
> 2020-04-22  qing zhao  <qing.zhao@oracle.com>
> 
> 	PR c/94230
> 	* gcc.dg/plugin/location-overflow-test-1.c (fn_1): New message
> to 
> 	provide hint to use the new -flarge-source-files option.
> 
> 
> > On Apr 23, 2020, at 1:27 PM, Richard Sandiford <
> > richard.sandiford@arm.com> wrote:
> > 
> > Qing Zhao <QING.ZHAO@ORACLE.COM> writes:
> > > ---
> > > gcc/c-family/c-indentation.c                           |  3 +++
> > > gcc/common.opt                                         |  5 +++++
> > > gcc/doc/invoke.texi                                    | 15
> > > ++++++++++++++-
> > > gcc/testsuite/gcc.dg/plugin/location-overflow-test-1.c |  2 +-
> > > gcc/toplev.c                                           |  3 +++
> > > 5 files changed, 26 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/gcc/c-family/c-indentation.c b/gcc/c-family/c-
> > > indentation.c
> > > index f737555..7074b10 100644
> > > --- a/gcc/c-family/c-indentation.c
> > > +++ b/gcc/c-family/c-indentation.c
> > > @@ -67,6 +67,9 @@ get_visual_column (expanded_location exploc,
> > > location_t loc,
> > > 		  "%<-Wmisleading-indentation%> is disabled from this
> > > point"
> > > 		  " onwards, since column-tracking was disabled due to"
> > > 		  " the size of the code/headers");
> > > +	  inform (loc,
> > > +		  "please add %<-flarge-source-files%> to invoke more" 
> > > +		  " column-tracking for large source files");
> > > 	}
> > >       return false;
> > >     }
> > 
> > This should be conditional on !flag_large_source_files.
> > 
> > > diff --git a/gcc/common.opt b/gcc/common.opt
> > > index 4368910..10a3d5b 100644
> > > --- a/gcc/common.opt
> > > +++ b/gcc/common.opt
> > > @@ -1597,6 +1597,11 @@ fkeep-gc-roots-live
> > > Common Undocumented Report Var(flag_keep_gc_roots_live)
> > > Optimization
> > > ; Always keep a pointer to a live memory block
> > > 
> > > +flarge-source-files
> > > +Common Report Var(flag_large_source_files) Init(0)
> > > +Adjust GCC to cope with large source files to provide more
> > > accurate
> > > +column information.
> > > +
> > 
> > I'm having difficulty suggesting wording here, but I think would be
> > good
> > to mention the downside.  How about:
> > 
> > ----------------------
> > Improve GCC's ability to track column numbers in large source
> > files,
> > at the expense of slower compilation.
> > ----------------------
> > 
> > > floop-parallelize-all
> > > Common Report Var(flag_loop_parallelize_all) Optimization
> > > Mark all loops as parallel.
> > > diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
> > > index 96a9516..c6ea9ef 100644
> > > --- a/gcc/doc/invoke.texi
> > > +++ b/gcc/doc/invoke.texi
> > > @@ -574,7 +574,7 @@ Objective-C and Objective-C++ Dialects}.
> > > -fdebug-cpp  -fdirectives-only  -fdollars-in-identifiers  @gol
> > > -fexec-charset=@var{charset}  -fextended-identifiers  @gol
> > > -finput-charset=@var{charset}  -fmacro-prefix-map=@var{old}=@var{
> > > new}  @gol
> > > --fmax-include-depth=@var{depth} @gol
> > > +-fmax-include-depth=@var{depth} -flarge-source-files @gol
> > > -fno-canonical-system-headers  -fpch-deps  -fpch-preprocess  @gol
> > > -fpreprocessed  -ftabstop=@var{width}  -ftrack-macro-
> > > expansion  @gol
> > > -fwide-exec-charset=@var{charset}  -fworking-directory @gol
> > 
> > This should be kept in alphabetical order, so after -finput-
> > charset.
> > 
> > > @@ -14151,6 +14151,19 @@ This option may be useful in conjunction
> > > with the @option{-B} or
> > > perform additional processing of the program source between
> > > normal preprocessing and compilation.
> > > 
> > > +@item -flarge-source-files
> > > +@opindex flarge-source-files
> > > +Adjust GCC to cope with large source files to provide more
> > > accurate
> > > +column information. 
> > > +By default, GCC will lose accurate column information if the
> > > source 
> > > +file is very large.
> > > +If this option is provided, GCC will adapt accordingly to
> > > provide more
> > > +accurate column information. 
> > > +This option may be useful when any user hits the @option{-
> > > Wmisleading-indent} 
> > > +note, "is disabled from this point onwards, since column-
> > > tracking was disabled 
> > > +due to the size of the code/headers", or hits the limit at which
> > > columns get
> > > +dropped from diagnostics.
> > > +
> > 
> > On a similar vein, how about:
> > 
> > ----------------------
> > Adjust GCC to expect large source files, at the expense of slower
> > compilation and higher memory usage.
> > 
> > Specifically, GCC normally tracks both column numbers and line
> > numbers
> > within source files and it normally prints both of these numbers in
> > diagnostics.  However, once it has processed a certain number of
> > source
> > lines, it stops tracking column numbers and only tracks line
> > numbers.
> > This means that diagnostics for later lines do not include column
> > numbers.
> > It also means that options like @option{-Wmisleading-indent} cease
> > to work
> > at that point, although the compiler prints a note if this happens.
> > Passing @option{-flarge-source-files} significantly increases the
> > number
> > of source lines that GCC can process before it stops tracking
> > column
> > numbers.
> > ----------------------
> > 
> > Thanks,
> > Richard


> Hi, Richard,
> 
> This is the 3rd version of the patch, updated based on your previous comments.

Thanks for working on this, and to Richard for the useful comments.

> Please take a look at it and let me know whether it’s okay to commit?

I like the overall approach.

Some nits inline.

> gcc/ChangeLog:
> 
> 2020-04-22  qing zhao  <qing.zhao@oracle.com>
> 
>         PR c/94230
>         * common.opt: Add -flarge-source-files.
>         * doc/invoke.texi: Document it.
>         * toplev.c (process_options): set line_table->default_range_bits
>         to 0 when flag_large_source_files is true.
> 
> 
> gcc/c-family/ChangeLog:
> 
> 2020-04-22  qing zhao  <qing.zhao@oracle.com>
> 
>         PR c/94230
>         * c-indentation.c (get_visual_column): Add a hint to use the new
>         -flarge-source-files option.
> 
> gcc/testsuite/ChangeLog:
> 
> 2020-04-22  qing zhao  <qing.zhao@oracle.com>
> 
>         PR c/94230
>         * gcc.dg/plugin/location-overflow-test-1.c (fn_1): New message to 
>         provide hint to use the new -flarge-source-files option.

Maybe: "Verify that hint about -flarge-source-files is emitted".


> From 97696d2c1c146fd7d3161f592d42cd7d73c6d5a8 Mon Sep 17 00:00:00 2001
> From: qing zhao <qing.zhao@oracle.com>
> Date: Thu, 23 Apr 2020 16:55:24 -0400
> Subject: [PATCH] Fix PR94230
> 
> ---
>  gcc/c-family/c-indentation.c                          |  4 ++++
>  gcc/common.opt                                        |  5 +++++
>  gcc/doc/invoke.texi                                   | 19 +++++++++++++++++--
>  .../gcc.dg/plugin/location-overflow-test-1.c          |  2 +-
>  gcc/toplev.c                                          |  3 +++
>  5 files changed, 30 insertions(+), 3 deletions(-)
> 
> diff --git a/gcc/c-family/c-indentation.c b/gcc/c-family/c-indentation.c
> index f737555..e929030 100644
> --- a/gcc/c-family/c-indentation.c
> +++ b/gcc/c-family/c-indentation.c
> @@ -67,6 +67,10 @@ get_visual_column (expanded_location exploc, location_t loc,
>  		  "%<-Wmisleading-indentation%> is disabled from this point"
>  		  " onwards, since column-tracking was disabled due to"
>  		  " the size of the code/headers");
> +	  if (!flag_large_source_files)
> +	    inform (loc,
> +		    "please add %<-flarge-source-files%> to invoke more" 
> +		    " column-tracking for large source files");

It's great having a hint here, but I don't love the wording of the
hint.

Maybe reword to:

"adding %<-flarge-source-files%> will allow for more column-tracking
support, at the expense of compilation time and memory".

or somesuch.


>  	}
>        return false;
>      }

[...]

> @@ -14151,6 +14151,21 @@ This option may be useful in conjunction with the @option{-B} or
>  perform additional processing of the program source between
>  normal preprocessing and compilation.
>  
> +@item -flarge-source-files
> +@opindex flarge-source-files
> +Adjust GCC to expect large source files, at the expense of slower
> +compilation and higher memory usage.
> +
> +Specifically, GCC normally tracks both column numbers and line numbers
> +within source files and it normally prints both of these numbers in
> +diagnostics.  However, once it has processed a certain number of source
> +lines, it stops tracking column numbers and only tracks line numbers.
> +This means that diagnostics for later lines do not include column numbers.
> +It also means that options like @option{-Wmisleading-indent} cease to work
                                           ^^^^^^^^^^^^^^^^^^^
This should be:                            -Wmisleading-indentation

> +at that point, although the compiler prints a note if this happens.
> +Passing @option{-flarge-source-files} significantly increases the number
> +of source lines that GCC can process before it stops tracking column.
                                                                 ^^^^^^
This should be:							 columns

>  @node Assembler Options
> diff --git a/gcc/testsuite/gcc.dg/plugin/location-overflow-test-1.c b/gcc/testsuite/gcc.dg/plugin/location-overflow-test-1.c
> index 1a80a66..fbc8211 100644
> --- a/gcc/testsuite/gcc.dg/plugin/location-overflow-test-1.c
> +++ b/gcc/testsuite/gcc.dg/plugin/location-overflow-test-1.c
> @@ -13,7 +13,7 @@ int
>  fn_1 (int flag)
>  {
>    int x = 4, y = 5;
> -  if (flag) x = 3; y = 2; /* { dg-message "-:disabled from this point" } */
> +  if (flag) x = 3; y = 2; /* { dg-message "-:disabled from this point" "add '-flarge-source-files'" } */

May need updating to match the hint message.

[...]

This is OK for stage 1 with those nits fixed.

There was talk earlier in the thread about fixing this in GCC 10.

Richi/Jakub: is this OK for stage 4?   Although it adds a new command-
line option, it's a workaround for a regression introduced in GCC 6 and
should be low risk.

Thanks
Dave