Re: [PATCH 2/2] PR c++/68819: libcpp fallbacks and -Wmisleading-indentation

[Jeff Law <law at redhat dot com>]

On 12/18/2015 01:21 PM, David Malcolm wrote:

> I don't think there's a way to fix -Wmisleading-indentation if we're
> in this state, so the first part of the following patch detects if
> this has happened, and effectively turns off -Wmisleading-indentation
> from that point onwards.  To avoid a false sense of security, the
> patch issues a "sorry" at the that point, currently with this wording:
> location-overflow-test-1.c:17:0: sorry, unimplemented: -Wmisleading-indentation is disabled from this point onwards, since column-tracking was disabled due to the size of the code/headers
Seems reasonable.  I can't see any way to get indentation warnings if we 
don't have column info.

> Should this greater chance of hitting LINE_MAP_MAX_LOCATION_WITH_COLS
> be filed as a separate PR?
I was originally going to say no, but I suspect there'll be a few folks 
that are going to bump up against it.  Might as well have a canonical BZ 
for it.


> The second part of the patch resolves this by adding an additional
> level of fallback: a new LINE_MAP_MAX_LOCATION_WITH_PACKED_RANGES
> threshold (currently 0x50000000) that occurs well before
> the LINE_MAP_MAX_LOCATION_WITH_COLS threshold (0x60000000).
> Once we reach the new LINE_MAP_MAX_LOCATION_WITH_PACKED_RANGES
> threshold, the range-packing optimization is disabled (with a transition
> to an ordinary map with m_range_bits == 0), effectively giving us a
> much "longer runway" before the LINE_MAP_MAX_LOCATION_WITH_COLS
> threshold is hit, at the cost to requiring the use of the ad-hoc
> table for every location (e.g. every token of length > 1).
> I haven't yet done performance testing on this.
>
> The patch adds test coverage for this by using a plugin to simulate
> the two levels of degraded locations.
>
> Rough calculations, assuming 7 bits of columns,
> LINE_MAP_MAX_LOCATION_WITH_COLS == 0x60000000
> LINE_MAP_MAX_LOCATION_WITH_PACKED_RANGES == 0x50000000
>
> gcc 5:
>    0x60000000 / 128 per line = 12,582,912 lines of code before
>    hitting the has-column-information limit.
>
> gcc 6 trunk:
>    0x60000000 / (128 * 32) per line = 393,216 lines of code before
>    hitting the has-column-information limit.
>
> with this patch:
>    0x50000000 / (128 * 32) per line = 327,680 lines of code before
>    hitting the range-packing limit, then:
>    0x10000000 / 128 = 2,097,152 lines of code before hitting the
>    has-column-information limit.
>    giving 2,424,832 lines of code total before hitting the
>    has-column-information limit.
>
> These numbers will be less in the face of lines longer than
> 127 characters.
>
> If the increased use of the ad-hoc table is an issue, another
> approach might be to simply disable range-handling for locations
> that go beyond a threshold location_t value: attempts to combine
> locations above that value lead to you simply getting the caret
> location.  If we take this approach, I think we'd still want to
> have a range threshold before the column one, so that we preserve
> the ability to have column information for these pure-caret
> locations.
>
> Alternatively, the range bits could be lowered from 5 to 4,
> doubling the lines we can handle before hitting the limit:
> 0x60000000 / (128 * 16) = 786,432, though that doesn't buy
> us much compared to the approach in this patch.
>
> Successfully bootstrapped&regrtested on x86_64-pc-linux-gnu.
>
> Thoughts?
>
> gcc/c-family/ChangeLog:
> 	PR c++/68819
> 	* c-indentation.c (get_visual_column): Handle the column
> 	number being zero by effectively disabling the warning, with
> 	a "sorry".
This part is fine as-is.


> gcc/testsuite/ChangeLog:
> 	PR c++/68819
> 	* gcc.dg/plugin/location-overflow-test-1.c: New test case.
> 	* gcc.dg/plugin/location-overflow-test-2.c: New test case.
> 	* gcc.dg/plugin/location_overflow_plugin.c: New test plugin.
> 	* gcc.dg/plugin/plugin.exp (plugin_test_list): Add the above.
>
> libcpp/ChangeLog:
> 	PR c++/68819
> 	* line-map.c (LINE_MAP_MAX_LOCATION_WITH_PACKED_RANGES): New
> 	constant.
> 	(LINE_MAP_MAX_LOCATION_WITH_COLS): Add note about unit tests
> 	to comment.
> 	(can_be_stored_compactly_p): Reduce threshold from
> 	LINE_MAP_MAX_LOCATION_WITH_COLS to
> 	LINE_MAP_MAX_LOCATION_WITH_PACKED_RANGES.
> 	(get_combined_adhoc_loc): Likewise.
> 	(get_range_from_loc): Likewise.
> 	(linemap_line_start): Ensure that a new ordinary map is created
> 	when transitioning from range-packing being enabled to disabled,
> 	at the LINE_MAP_MAX_LOCATION_WITH_PACKED_RANGES threshold.  Set
> 	range_bits to 0 for new ordinary maps when beyond this limit.
> 	Prevent the "increase the column bits of a freshly created map"
> 	optimization if the range bits has reduced.

> +
> +/* We use location_overflow_plugin.c to inject the
> +   which injects the case that location_t values have exceeded
> +   LINE_MAP_MAX_LOCATION_WITH_COLS, and hence no column
> +   numbers are available.  */
It's just a test, but the comment doesn't parse.  "to inject the which" 
:-)  It's repeated in the second test as well.

With the comment fixes this is OK.

jeff