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.
> > >
> > > /* 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.
>
> > > -
> > > - /* Verify that we fully covered the test matrix. */
> > > - ASSERT_EQ (num_cases_tested, 2 * 12);
> > > }
> > >
> > > /* Verify that when presented with a consecutive pair of locations
> > > with
> >
>
> Otherwise looks good to me.
>
> Thanks
> Dave
>
Thanks again...
-Lewis
