Re: [PATCH v4 3/8] diagnostics: Refactor class file_cache_slot

[Lewis Hyatt <lhyatt@gmail.com>]

On Tue, Aug 15, 2023 at 11:43:05AM -0400, David Malcolm wrote:
> On Wed, 2023-08-09 at 18:14 -0400, Lewis Hyatt wrote:
> > Class file_cache_slot in input.cc is used to query specific lines of source
> > code from a file when needed by diagnostics infrastructure. This will be
> > extended in a subsequent patch to support obtaining the source code from
> > in-memory generated buffers rather than from a file. The present patch
> > refactors class file_cache_slot, putting most of the logic into a new base
> > class cache_data_source, in preparation for reusing that code in the next
> > patch. There is no change in functionality yet.
> > 
> > gcc/ChangeLog:
> > 
> >         * input.cc (class file_cache_slot): Refactor functionality into a
> >         new base class...
> >         (class cache_data_source): ...here.
> >         (file_cache::forcibly_evict_file): Adapt for refactoring.
> >         (file_cache_slot::evict): Renamed to...
> >         (file_cache_slot::reset): ...this, and partially refactored into
> >         base class...
> >         (cache_data_source::reset): ...here.
> >         (file_cache_slot::get_full_file_content): Moved into base class...
> >         (cache_data_source::get_full_file_content): ...here.
> >         (file_cache_slot::create): Adapt for refactoring.
> >         (file_cache_slot::file_cache_slot): Refactor partially into...
> >         (cache_data_source::cache_data_source): ...here.
> >         (file_cache_slot::~file_cache_slot): Refactor partially into...
> >         (cache_data_source::~cache_data_source): ...here.
> >         (file_cache_slot::needs_read_p): Remove.
> >         (file_cache_slot::needs_grow_p): Remove.
> >         (file_cache_slot::maybe_grow): Adapt for refactoring.
> >         (file_cache_slot::read_data): Refactored, along with...
> >         (file_cache_slot::maybe_read_data): this, into...
> >         (file_cache_slot::get_more_data): ...here.
> >         (find_end_of_line): Change interface to take a pair of pointers,
> >         rather than a pointer + length.
> >         (file_cache_slot::get_next_line): Refactored into...
> >         (cache_data_source::get_next_line): ...here.
> >         (file_cache_slot::goto_next_line): Refactored into...
> >         (cache_data_source::goto_next_line): ...here.
> >         (file_cache_slot::read_line_num): Refactored into...
> >         (cache_data_source::read_line_num): ...here.
> >         (location_get_source_line): Fix const-correctness as necessitated by
> >         new interface.
> > ---
> >  gcc/input.cc | 513 +++++++++++++++++++++++----------------------------
> >  1 file changed, 235 insertions(+), 278 deletions(-)
> > 
> 
> I confess I had to reread both this and patch 4/8 to make sense of
> this; this is probably one of those cases where it's harder to read in
> patch form than as source, but I think I now understand the new
> implementation.

Yes, sorry about that. I hope at least splitting into two patches here made it
a little easier.

> 
> Did you try testing this with valgrind (e.g. "make selftest-valgrind")?
>

Oh interesting, was not aware of this. I think it shows that new leaks were
not introduced with the patch series.

BEFORE patch series:
==1572278==
-fself-test: 7634593 pass(es) in 22.799240 seconds
==1572278==
==1572278== HEAP SUMMARY:
==1572278==     in use at exit: 1,083,255 bytes in 2,394 blocks
==1572278==   total heap usage: 2,704,869 allocs, 2,702,475 frees, 1,257,334,536 bytes allocated
==1572278==
==1572278== 8,032 bytes in 1 blocks are possibly lost in loss record 639 of 657
==1572278==    at 0x4848899: malloc (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
==1572278==    by 0x21FE1CB: xmalloc (xmalloc.c:149)
==1572278==    by 0x21B02E0: new_buff (lex.cc:4767)
==1572278==    by 0x21B02E0: _cpp_get_buff (lex.cc:4800)
==1572278==    by 0x21ACC80: cpp_create_reader(c_lang, ht*, line_maps*) (init.cc:289)
==1572278==    by 0xA64282: c_common_init_options(unsigned int, cl_decoded_option*) (c-opts.cc:237)
==1572278==    by 0x95E479: toplev::main(int, char**) (toplev.cc:2241)
==1572278==    by 0x960B2D: main (main.cc:39)
==1572278==
==1572278== LEAK SUMMARY:
==1572278==    definitely lost: 0 bytes in 0 blocks
==1572278==    indirectly lost: 0 bytes in 0 blocks
==1572278==      possibly lost: 8,032 bytes in 1 blocks
==1572278==    still reachable: 1,075,223 bytes in 2,393 blocks
==1572278==         suppressed: 0 bytes in 0 blocks
==1572278== Reachable blocks (those to which a pointer was found) are not shown.
==1572278== To see them, rerun with: --leak-check=full --show-leak-kinds=all
==1572278==
==1572278== For lists of detected and suppressed errors, rerun with: -s
==1572278== ERROR SUMMARY: 1 errors from 1 contexts (suppressed: 0 from 0)

AFTER patch series:
==1594840==
-fself-test: 7638403 pass(es) in 23.671784 seconds
==1594840==
==1594840== HEAP SUMMARY:
==1594840==     in use at exit: 1,081,759 bytes in 2,367 blocks
==1594840==   total heap usage: 2,728,561 allocs, 2,726,194 frees, 1,272,214,526 bytes allocated
==1594840==
==1594840== 8,032 bytes in 1 blocks are possibly lost in loss record 609 of 628
==1594840==    at 0x4848899: malloc (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
==1594840==    by 0x2200CCB: xmalloc (xmalloc.c:149)
==1594840==    by 0x21B2440: new_buff (lex.cc:4767)
==1594840==    by 0x21B2440: _cpp_get_buff (lex.cc:4800)
==1594840==    by 0x21AEDA0: cpp_create_reader(c_lang, ht*, line_maps*) (init.cc:289)
==1594840==    by 0xA64592: c_common_init_options(unsigned int, cl_decoded_option*) (c-opts.cc:237)
==1594840==    by 0x95E529: toplev::main(int, char**) (toplev.cc:2241)
==1594840==    by 0x960BDD: main (main.cc:39)
==1594840==
==1594840== LEAK SUMMARY:
==1594840==    definitely lost: 0 bytes in 0 blocks
==1594840==    indirectly lost: 0 bytes in 0 blocks
==1594840==      possibly lost: 8,032 bytes in 1 blocks
==1594840==    still reachable: 1,073,727 bytes in 2,366 blocks
==1594840==         suppressed: 0 bytes in 0 blocks

> I don't think we have any selftest coverage for "\r" in the line-break
> handling; that would be good to add.
> 
> This patch is OK for trunk once the rest of the kit is approved.

Thank you. To be clear, were you suggesting to add selftest coverage for \r
endings now, or in a follow up?

-Lewis