Re: [PATCH 2/2] PR c++/68819: libcpp fallbacks and -Wmisleading-indentation
On Wed, Jan 06, 2016 at 03:02:05PM -0500, David Malcolm wrote: > On Mon, 2015-12-21 at 22:20 +0100, Jakub Jelinek wrote: > > On Mon, Dec 21, 2015 at 02:10:17PM -0700, Jeff Law wrote: > > > 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. > > > > sorry will set sorrycount to non-zero though, so seen_error () will be true > > and the compiler will exit with non-zero exit status. That is IMHO not > > appripriate for warning (at least unless -Werror=misleading-indentation). > > Some possibilities here: > > (A, the patch): issue a "sorry" to indicate that the warning isn't > available anymore, leading to a nonzero exit status > > (B) silently disable the warning > > (C) issue a "warning" about the impaired warning, using > OPT_Wmisleading_indentation, so that it becomes an error if > -Werror=misleading-indentation. > > (D) something else? > > Do you have a preference as to what approach I should try? I think I > like option (C) above. My preference would be inform (). That is e.g. what var-tracking uses in a similar case: if (MAY_HAVE_DEBUG_INSNS) inform (DECL_SOURCE_LOCATION (cfun->decl), "variable tracking size limit exceeded with " "-fvar-tracking-assignments, retrying without"); else inform (DECL_SOURCE_LOCATION (cfun->decl), "variable tracking size limit exceeded"); when the tables are too large and computing good quality debug info would be too expensive. Jakub
Re: [PATCH 2/2] PR c++/68819: libcpp fallbacks and -Wmisleading-indentation
On Mon, 2015-12-21 at 22:20 +0100, Jakub Jelinek wrote: > On Mon, Dec 21, 2015 at 02:10:17PM -0700, Jeff Law wrote: > > 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. > > sorry will set sorrycount to non-zero though, so seen_error () will be true > and the compiler will exit with non-zero exit status. That is IMHO not > appripriate for warning (at least unless -Werror=misleading-indentation). Some possibilities here: (A, the patch): issue a "sorry" to indicate that the warning isn't available anymore, leading to a nonzero exit status (B) silently disable the warning (C) issue a "warning" about the impaired warning, using OPT_Wmisleading_indentation, so that it becomes an error if -Werror=misleading-indentation. (D) something else? Do you have a preference as to what approach I should try? I think I like option (C) above. Dave
Re: [PATCH 2/2] PR c++/68819: libcpp fallbacks and -Wmisleading-indentation
On Mon, 2015-12-21 at 14:10 -0700, Jeff Law wrote: > 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. I've opened PR preprocessor/69177 to track fixing the increased tendency to hit the LINE_MAP_MAX_LOCATION_WITH_COLS limit. [...snip...]
Re: [PATCH 2/2] PR c++/68819: libcpp fallbacks and -Wmisleading-indentation
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 0x5000) that occurs well before the LINE_MAP_MAX_LOCATION_WITH_COLS threshold (0x6000). 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 == 0x6000 LINE_MAP_MAX_LOCATION_WITH_PACKED_RANGES == 0x5000 gcc 5: 0x6000 / 128 per line = 12,582,912 lines of code before hitting the has-column-information limit. gcc 6 trunk: 0x6000 / (128 * 32) per line = 393,216 lines of code before hitting the has-column-information limit. with this patch: 0x5000 / (128 * 32) per line = 327,680 lines of code before hitting the range-packing limit, then: 0x1000 / 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: 0x6000 / (128 * 16) = 786,432, though that doesn't buy us much compared to the approach in this patch. Successfully bootstrapped 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
Re: [PATCH 2/2] PR c++/68819: libcpp fallbacks and -Wmisleading-indentation
On Mon, Dec 21, 2015 at 02:10:17PM -0700, Jeff Law wrote: > 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. sorry will set sorrycount to non-zero though, so seen_error () will be true and the compiler will exit with non-zero exit status. That is IMHO not appripriate for warning (at least unless -Werror=misleading-indentation). Jakub
[PATCH 2/2] PR c++/68819: libcpp fallbacks and -Wmisleading-indentation
libcpp has a degraded fallback mode for large source files where if a location_t > LINE_MAP_MAX_LOCATION_WITH_COLS we fall back to just tracking lines, not columns (currently 0x6000), and every location_t expands to having a column == 0. Sadly we're more likely to see this case in gcc 6 than in gcc 5 since I'm using up 5 bits of location_t, for packing short ranges into them (to avoid needing to go to the ad-hoc lookaside during tokenization; this change was part of r230331). -Wmisleading-indentation expects to have meaningful column data, and so interacts poorly with this fallback, issuing a warning at every if/for/while, I believe. 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 On attempting to bootstrap with just this change, I ran into a problem: gimple-match.c is hitting the LINE_MAP_MAX_LOCATION_WITH_COLS limit, and I got a "sorry" at line 50437:0: sorry, unimplemented: etc std::swap (o30, o31); i.e. the bit-packing optimization is causing us to hit that LINE_MAP_MAX_LOCATION_WITH_COLS limit earlier than before, and presumably this will affect people with other very large source files (e.g. as in the initial PR c++/68819 report). Should this greater chance of hitting LINE_MAP_MAX_LOCATION_WITH_COLS be filed as a separate PR? 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 0x5000) that occurs well before the LINE_MAP_MAX_LOCATION_WITH_COLS threshold (0x6000). 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 == 0x6000 LINE_MAP_MAX_LOCATION_WITH_PACKED_RANGES == 0x5000 gcc 5: 0x6000 / 128 per line = 12,582,912 lines of code before hitting the has-column-information limit. gcc 6 trunk: 0x6000 / (128 * 32) per line = 393,216 lines of code before hitting the has-column-information limit. with this patch: 0x5000 / (128 * 32) per line = 327,680 lines of code before hitting the range-packing limit, then: 0x1000 / 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: 0x6000 / (128 * 16) = 786,432, though that doesn't buy us much compared to the approach in this patch. Successfully bootstrapped 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". 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.
