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

[Qing Zhao via Gcc-patches]

Hi, Richard And Dave:

Thanks a lot for the review and comments.

> On Apr 21, 2020, at 1:46 PM, Richard Sandiford  
> wrote:
> 
> David Malcolm  writes:
>> On Tue, 2020-04-21 at 15:04 +0100, Richard Sandiford wrote
>>> 
>>> Please add:
>>> 
>>> PR c/94230

Will do. 

>>> 
* common.opt: Add -flocation-ranges.
* doc/invoke.texi: Document it.
* toplev.c (process_options): set line_table-
> default_range_bits
to 0 when flag_location_ranges is false. 
>>> 
>>> I think it would be worth adding a hint to use the new option to
>>> get_visual_column, when warning about column tracking being disabled.
>>> This should probably be a second inform(), immediately after the
>>> current one.

Sounds reasonable to me, I will add that.

>>> 
 @@ -14151,6 +14151,13 @@ 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 -flocation-ranges
 +@opindex flocation-ranges
>>> 
>>> Normally the documented option should be the non-default one,
>>> so -fno-... in this case.

Okay. 

>>> 
 +Enable range tracking when recording source locations.
 +By default, GCC enables range tracking when recording source
 locations.
 +If disable range tracking by -fno-location-ranges, more location
 space
 +will be saved for column tracking.
>>> 
>>> My understanding is that the patch doesn't actually disable location-
>>> range
>>> tracking, but simply uses a less efficient form for *all* ranges,
>>> rather
>>> than only using the less efficient form for ranges that aren't "caret
>>> at
>>> start, length < 64 chars".
>> 
>> Indeed.

Okay, I see. 
Providing a good documentation at the user level for this option might be a 
challenge to me, I will try.  -:)

>> 
>>> I know you're simply following the suggestion in the PR, sorry,
>> 
>> Sorry.  I did put a caveat on the suggestion FWIW.
>> 
>>> but I wonder if the option should instead be:
>>> 
>>> -flarge-source-files
>>> 
>>> since that seems like a more user-facing concept.  The option would
>>> tell GCC that the source files are likely to be very large and that
>>> GCC should adapt accordingly.  In particular, the option makes GCC
>>> cope with more source lines at the expense of slowing down
>>> compilation
>>> and using more memory.
>> 
>> Another approach would be to go lower-level and introduce a param for
>> this; something like "--param location-range-bits" defaulting to 5; the
>> user can set it to 0 to disable the range-bit optimization to deal with
>> bigger files, and it allows for experimentation without rebuilding the
>> compiler.
>> 
>> Again, I don't know if this is a good idea; I'm thinking aloud; I'm not
>> sure what the best direction here is.
> 
> The reason I like the -flarge-source-files (suggestion for better
> names welcome) is that the user is giving user-level information and
> letting the compiler decide how to deal with that.  What the option
> actually does can change with the implementation as necessary.
> 
> Potentially any user could hit the -Wmisleading-indent note, or could
> hit the limit at which columns get dropped from diagnostics.  So while
> this option isn't going to be used all that often, it also doesn't feel
> like an option designed specifically for “power users” who like to
> experiment with compiler internals.

Agreed, I prefer to use -flarge-source-files for this functionality. 

Let me know if you have any other suggestions for this patch.

Thanks.

Qing


> 
> Thanks,
> Richard

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

[Richard Sandiford]

David Malcolm  writes:
> On Tue, 2020-04-21 at 15:04 +0100, Richard Sandiford wrote:
>> Qing Zhao via Gcc-patches  writes:
>> > Hi,
>> > 
>> > Please take a look at the attached patch and let me know your
>> > comments.
>> > 
>> > Thanks.
>> > 
>> > Qing
>> 
>> Acting as a reviewer of last resort since this isn't really my area,
>
> Sorry; life has been crazy lately.

NP, just thought I'd better justify treading in this area. :-)

>> but FWIW, I agree losing column tracking at the current limit is a
>> genuine regression and should be fixed for GCC 10.
>
> Is this a regression in GCC 10 though?  I think this has been the case
> since GCC 6 onwards.

Agree it's not a regression in GCC 10, but it still seems fair game
as a regression in general.

>> > gcc/ChangeLog:
>> > 
>> > 2020-04-03  qing zhao  
>> > 
>> 
>> Please add:
>> 
>>  PR c/94230
>> 
>> >* common.opt: Add -flocation-ranges.
>> >* doc/invoke.texi: Document it.
>> >* toplev.c (process_options): set line_table-
>> > >default_range_bits
>> >to 0 when flag_location_ranges is false. 
>> 
>> I think it would be worth adding a hint to use the new option to
>> get_visual_column, when warning about column tracking being disabled.
>> This should probably be a second inform(), immediately after the
>> current one.
>> 
>> > @@ -14151,6 +14151,13 @@ 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 -flocation-ranges
>> > +@opindex flocation-ranges
>> 
>> Normally the documented option should be the non-default one,
>> so -fno-... in this case.
>> 
>> > +Enable range tracking when recording source locations.
>> > +By default, GCC enables range tracking when recording source
>> > locations.
>> > +If disable range tracking by -fno-location-ranges, more location
>> > space
>> > +will be saved for column tracking.
>> 
>> My understanding is that the patch doesn't actually disable location-
>> range
>> tracking, but simply uses a less efficient form for *all* ranges,
>> rather
>> than only using the less efficient form for ranges that aren't "caret
>> at
>> start, length < 64 chars".
>
> Indeed.
>
>> I know you're simply following the suggestion in the PR, sorry,
>
> Sorry.  I did put a caveat on the suggestion FWIW.
>
>> but I wonder if the option should instead be:
>> 
>> -flarge-source-files
>> 
>> since that seems like a more user-facing concept.  The option would
>> tell GCC that the source files are likely to be very large and that
>> GCC should adapt accordingly.  In particular, the option makes GCC
>> cope with more source lines at the expense of slowing down
>> compilation
>> and using more memory.
>
> Another approach would be to go lower-level and introduce a param for
> this; something like "--param location-range-bits" defaulting to 5; the
> user can set it to 0 to disable the range-bit optimization to deal with
> bigger files, and it allows for experimentation without rebuilding the
> compiler.
>
> Again, I don't know if this is a good idea; I'm thinking aloud; I'm not
> sure what the best direction here is.

The reason I like the -flarge-source-files (suggestion for better
names welcome) is that the user is giving user-level information and
letting the compiler decide how to deal with that.  What the option
actually does can change with the implementation as necessary.

Potentially any user could hit the -Wmisleading-indent note, or could
hit the limit at which columns get dropped from diagnostics.  So while
this option isn't going to be used all that often, it also doesn't feel
like an option designed specifically for “power users” who like to
experiment with compiler internals.

Thanks,
Richard

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

[David Malcolm via Gcc-patches]

On Tue, 2020-04-21 at 15:04 +0100, Richard Sandiford wrote:
> Qing Zhao via Gcc-patches  writes:
> > Hi,
> > 
> > Please take a look at the attached patch and let me know your
> > comments.
> > 
> > Thanks.
> > 
> > Qing
> 
> Acting as a reviewer of last resort since this isn't really my area,

Sorry; life has been crazy lately.

> but FWIW, I agree losing column tracking at the current limit is a
> genuine regression and should be fixed for GCC 10.

Is this a regression in GCC 10 though?  I think this has been the case
since GCC 6 onwards.

> > gcc/ChangeLog:
> > 
> > 2020-04-03  qing zhao  
> > 
> 
> Please add:
> 
>   PR c/94230
> 
> > * common.opt: Add -flocation-ranges.
> > * doc/invoke.texi: Document it.
> > * toplev.c (process_options): set line_table-
> > >default_range_bits
> > to 0 when flag_location_ranges is false. 
> 
> I think it would be worth adding a hint to use the new option to
> get_visual_column, when warning about column tracking being disabled.
> This should probably be a second inform(), immediately after the
> current one.
> 
> > @@ -14151,6 +14151,13 @@ 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 -flocation-ranges
> > +@opindex flocation-ranges
> 
> Normally the documented option should be the non-default one,
> so -fno-... in this case.
> 
> > +Enable range tracking when recording source locations.
> > +By default, GCC enables range tracking when recording source
> > locations.
> > +If disable range tracking by -fno-location-ranges, more location
> > space
> > +will be saved for column tracking.
> 
> My understanding is that the patch doesn't actually disable location-
> range
> tracking, but simply uses a less efficient form for *all* ranges,
> rather
> than only using the less efficient form for ranges that aren't "caret
> at
> start, length < 64 chars".

Indeed.

> I know you're simply following the suggestion in the PR, sorry,

Sorry.  I did put a caveat on the suggestion FWIW.

> but I wonder if the option should instead be:
> 
> -flarge-source-files
> 
> since that seems like a more user-facing concept.  The option would
> tell GCC that the source files are likely to be very large and that
> GCC should adapt accordingly.  In particular, the option makes GCC
> cope with more source lines at the expense of slowing down
> compilation
> and using more memory.

Another approach would be to go lower-level and introduce a param for
this; something like "--param location-range-bits" defaulting to 5; the
user can set it to 0 to disable the range-bit optimization to deal with
bigger files, and it allows for experimentation without rebuilding the
compiler.

Again, I don't know if this is a good idea; I'm thinking aloud; I'm not
sure what the best direction here is.

Sorry again about the belated response on this patch.
Dave

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

[Richard Sandiford]

Qing Zhao via Gcc-patches  writes:
> Hi,
>
> Please take a look at the attached patch and let me know your comments.
>
> Thanks.
>
> Qing

Acting as a reviewer of last resort since this isn't really my area,
but FWIW, I agree losing column tracking at the current limit is a
genuine regression and should be fixed for GCC 10.

> gcc/ChangeLog:
>
> 2020-04-03  qing zhao  
>

Please add:

PR c/94230

>   * common.opt: Add -flocation-ranges.
>   * doc/invoke.texi: Document it.
>   * toplev.c (process_options): set line_table->default_range_bits
>   to 0 when flag_location_ranges is false. 

I think it would be worth adding a hint to use the new option to
get_visual_column, when warning about column tracking being disabled.
This should probably be a second inform(), immediately after the
current one.

> @@ -14151,6 +14151,13 @@ 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 -flocation-ranges
> +@opindex flocation-ranges

Normally the documented option should be the non-default one,
so -fno-... in this case.

> +Enable range tracking when recording source locations.
> +By default, GCC enables range tracking when recording source locations.
> +If disable range tracking by -fno-location-ranges, more location space
> +will be saved for column tracking.

My understanding is that the patch doesn't actually disable location-range
tracking, but simply uses a less efficient form for *all* ranges, rather
than only using the less efficient form for ranges that aren't "caret at
start, length < 64 chars".

I know you're simply following the suggestion in the PR, sorry,
but I wonder if the option should instead be:

-flarge-source-files

since that seems like a more user-facing concept.  The option would
tell GCC that the source files are likely to be very large and that
GCC should adapt accordingly.  In particular, the option makes GCC
cope with more source lines at the expense of slowing down compilation
and using more memory.

Thanks,
Richard

Fwd: PING [PATCH][gcc][PR94230]provide an option to change the size limitation for -Wmisleading-indent

[Qing Zhao via Gcc-patches]

Ping.

We need this patch for our product building.

thanks.

Qing

> Begin forwarded message:
> 
> From: Qing Zhao via Gcc-patches 
> Subject: PING [PATCH][gcc][PR94230]provide an option to change the size 
> limitation for -Wmisleading-indent 
> Date: April 8, 2020 at 2:39:22 PM CDT
> To: dmalc...@redhat.com, ja...@redhat.com
> Cc: gcc-patches@gcc.gnu.org
> Reply-To: Qing Zhao 
> 
> Hi,
> 
> Please take a look at the attached patch and let me know your comments.
> 
> Thanks.
> 
> Qing
> 
> gcc/ChangeLog:
> 
> 2020-04-03  qing zhao  
> 
>   * common.opt: Add -flocation-ranges.
>   * doc/invoke.texi: Document it.
>   * toplev.c (process_options): set line_table->default_range_bits
>   to 0 when flag_location_ranges is false. 
> 
> 
> 
>> Begin forwarded message:
>> 
>> From: Qing Zhao via Gcc-patches 
>> Subject: [PATCH][gcc][PR94230]provide an option to change the size 
>> limitation for -Wmisleading-indent 
>> Date: April 3, 2020 at 2:55:53 PM CDT
>> To: dmalc...@redhat.com, ja...@redhat.com
>> Cc: gcc-patches@gcc.gnu.org
>> Reply-To: Qing Zhao 
>> 
>> Hi, David and Jakub,
>> 
>> Per the discussion we had for PR94230: provide an option to change the size 
>> limitation for -Wmisleading-indent
>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94230 
>> 
>> 
>> I come up with the following simple patch per David’s suggestion:
>> 
>> Provide a new first class option -flocation-ranges to control 
>> enabling/disablng range tracking when recording source locations.
>> The default value for this option is enabling the range tracking.
>> 
>> When specify -fno-location-ranges, disable the range tracking to save space 
>> for column tracking. 
>> 
>> I have tested this GCC to build our huge application by adding 
>> -fno-location-ranges, the whole build completed without any issue. and
>> -Wmisleading-indent also emitted several helpful warning message during 
>> building.
>> 
>> I am running bootstrap right now.
>> 
>> Could you please take a look at the attached patch?
>> 
>> thanks.
>> 
>> Qing
>> 
>> 
>> 
>> 
> 

PING [PATCH][gcc][PR94230]provide an option to change the size limitation for -Wmisleading-indent

[Qing Zhao via Gcc-patches]

Hi,

Please take a look at the attached patch and let me know your comments.

Thanks.

Qing

gcc/ChangeLog:

2020-04-03  qing zhao  

* common.opt: Add -flocation-ranges.
* doc/invoke.texi: Document it.
* toplev.c (process_options): set line_table->default_range_bits
to 0 when flag_location_ranges is false. 



PR94230.patch
Description: Binary data


> Begin forwarded message:
> 
> From: Qing Zhao via Gcc-patches 
> Subject: [PATCH][gcc][PR94230]provide an option to change the size limitation 
> for -Wmisleading-indent 
> Date: April 3, 2020 at 2:55:53 PM CDT
> To: dmalc...@redhat.com, ja...@redhat.com
> Cc: gcc-patches@gcc.gnu.org
> Reply-To: Qing Zhao 
> 
> Hi, David and Jakub,
> 
> Per the discussion we had for PR94230: provide an option to change the size 
> limitation for -Wmisleading-indent
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94230 
> 
> 
> I come up with the following simple patch per David’s suggestion:
> 
> Provide a new first class option -flocation-ranges to control 
> enabling/disablng range tracking when recording source locations.
> The default value for this option is enabling the range tracking.
> 
> When specify -fno-location-ranges, disable the range tracking to save space 
> for column tracking. 
> 
> I have tested this GCC to build our huge application by adding 
> -fno-location-ranges, the whole build completed without any issue. and
> -Wmisleading-indent also emitted several helpful warning message during 
> building.
> 
> I am running bootstrap right now.
> 
> Could you please take a look at the attached patch?
> 
> thanks.
> 
> Qing
> 
> 
> 
>