On Wed, 2024-11-27 at 13:18 -0500, Lewis Hyatt wrote:
> On Wed, Nov 27, 2024 at 09:41:13AM -0500, David Malcolm wrote:
> > On Wed, 2024-11-27 at 14:56 +0100, Richard Biener wrote:
> > > On Sun, Nov 3, 2024 at 11:28 PM Lewis Hyatt <lhy...@gmail.com>
> > > wrote:
> > > >
> > > > Several of the selftests in diagnostic-show-locus.cc and
> > > > input.cc
> > > > are
> > > > sensitive to linemap internals. Adjust them here so they will
> > > > support 64-bit
> > > > location_t if configured.
> > > >
> > > > Likewise, handle 64-bit location_t in the support for
> > > > -fdump-internal-locations. As was done with the analyzer,
> > > > convert
> > > > to
> > > > (unsigned long long) explicitly so that 32- and 64-bit can be
> > > > handled with
> > > > the same printf formats.
> > >
> > > I was hoping David would have a look here. Absent from comments
> > > from
> > > him
> > > this is OK when all else is approved and after giving him another
> > > week.
> >
> > Mostly looks good, but I have a couple of questions below...
>
> Thanks for taking a look.
>
> >
> >
> > >
> > > What's missing review now? I've lost track ...
> > >
> > > Thanks,
> > > Richard.
> > >
> > > > gcc/ChangeLog:
> > > >
> > > > * diagnostic-show-locus.cc
> > > > (test_one_liner_fixit_validation_adhoc_locations):
> > > > Adapt so
> > > > it can
> > > > effectively test 7-bit ranges instead of 5-bit ranges.
> > > > (test_one_liner_fixit_validation_adhoc_locations_utf8):
> > > > Likewise.
> > > > * input.cc (get_end_location): Adjust types to support
> > > > 64-
> > > > bit
> > > > location_t.
> > > > (write_digit_row): Likewise.
> > > > (dump_location_range): Likewise.
> > > > (dump_location_info): Likewise.
> > > > (class line_table_case): Likewise.
> > > > (test_accessing_ordinary_linemaps): Replace some hard-
> > > > coded
> > > > constants with the values defined in line-map.h.
> > > > (for_each_line_table_case): Likewise.
> > > > ---
> > > > gcc/diagnostic-show-locus.cc | 128
> > > > +++++++++++++++++++++++++++++--
> > > > ----
> > > > gcc/input.cc | 100 ++++++++++++++-------------
> > > > 2 files changed, 157 insertions(+), 71 deletions(-)
> > > >
> >
> > [...snip...]
> >
> > > > diff --git a/gcc/input.cc b/gcc/input.cc
> > > > index 04462ef6f5a..1629e4aeee8 100644
> > > > --- a/gcc/input.cc
> > > > +++ b/gcc/input.cc
> >
> > [...snip...]
> >
> > > > @@ -3865,11 +3870,11 @@ static const location_t
> > > > boundary_locations[] = {
> > > > LINE_MAP_MAX_LOCATION_WITH_PACKED_RANGES + 0x100,
> > > >
> > > > /* Values near LINE_MAP_MAX_LOCATION_WITH_COLS. */
> > > > - LINE_MAP_MAX_LOCATION_WITH_COLS - 0x100,
> > > > + LINE_MAP_MAX_LOCATION_WITH_COLS - 0x200,
> > > > LINE_MAP_MAX_LOCATION_WITH_COLS - 1,
> > > > LINE_MAP_MAX_LOCATION_WITH_COLS,
> > > > LINE_MAP_MAX_LOCATION_WITH_COLS + 1,
> > > > - LINE_MAP_MAX_LOCATION_WITH_COLS + 0x100,
> > > > + LINE_MAP_MAX_LOCATION_WITH_COLS + 0x200,
> > > > };
> >
> > I see that this updates the offsets from 0x100 to 0x200 for the
> > _WITH_COLS case, but doesn't for the _WITH_PACKED_RANGES case.
> >
> > What's the reasoning here?
> >
> > In theory we can simply add new entries to boundary_locations to
> > get
> > more test coverage, but I don't know the extent to which this part
> > of
> > the selftests is slowing builds down on the slower configurations;
> > the
> > selftests are meant to be fast to run.
> >
>
> I needed to change it for _WITH_COLS because otherwise the test
> test_lexer_string_locations_concatenation_1 fails. This is because
> the
> assert_char_at_range() utility it uses does not handle the case of a
> token
> which straddles LINE_MAP_MAX_LOCATION_WITH_COLS; it assumes that the
> column
> information will be available in this case, but it is not. Once the
> number
> of range bits increases, the 0x100 buffer is not enough to avoid
> straddling
> the cutoff. I could also modify that selftest, I just did this change
> since
> it preserves what's currently being tested. I could change
> _WITH_PACKED_RANGES too if you prefer for consistency? It wasn't
> necessary
> but it will work either way, or could do both.
No need, I just wanted the reasoning documented here.
>
> > > >
> > > > /* Run TESTCASE multiple times, once for each case in our test
> > > > matrix. */
> > > > @@ -3884,10 +3889,9 @@ for_each_line_table_case (void
> > > > (*testcase)
> > > > (const line_table_case &))
> > > >
> > > > /* Run all tests with:
> > > > (a) line_table->default_range_bits == 0, and
> > > > - (b) line_table->default_range_bits == 5. */
> > > > - int num_cases_tested = 0;
> > > > - for (int default_range_bits = 0; default_range_bits <= 5;
> > > > - default_range_bits += 5)
> > > > + (b) line_table->default_range_bits ==
> > > > line_map_suggested_range_bits. */
> > > > +
> > > > + for (int default_range_bits: {0,
> > > > line_map_suggested_range_bits})
> > > > {
> > > > /* ...and use each of the "interesting" location values
> > > > as
> > > > the starting location within line_table. */
> > > > @@ -3895,15 +3899,9 @@ for_each_line_table_case (void
> > > > (*testcase)
> > > > (const line_table_case &))
> > > > for (int loc_idx = 0; loc_idx < num_boundary_locations;
> > > > loc_idx++)
> > > > {
> > > > line_table_case c (default_range_bits,
> > > > boundary_locations[loc_idx]);
> > > > -
> > > > testcase (c);
> > > > -
> > > > - num_cases_tested++;
> > > > }
> > > > }
> >
> > I see that this eliminates the tracking of num_cases_tested and the
> > assert on it below. Was this deliberate, or was the removal meant
> > to
> > be a temporary thing whilst developing the patch kit?
> >
>
> In an earlier version of the patch where the location_t type was a
> configuration option, I was experimenting with adding some new
> boundary
> locations here, conditional on whether 64-bit location_t was defined.
> In
> that case, the number of iterations had to be computed so I wasn't
> sure it
> was worth preserving this check, which basically amounts to just
> confirming
> that the for loop executed the requested number of times. Now that
> it's not
> configurable in v2 anymore, there's no need to change this part
> though so I
> can just put it back how it was too.
No need; sounds fair enough. I like the use of
for (int default_range_bits: {0, line_map_suggested_range_bits})
that's a nice improvement.
The patch you posted is OK by me.
Thanks
Dave
