Re: [PATCH v4 4/8] diagnostics: Support obtaining source code lines from generated data buffers

[Lewis Hyatt <lhyatt@gmail.com>]

On Tue, Aug 15, 2023 at 12:15:15PM -0400, David Malcolm wrote:
> On Wed, 2023-08-09 at 18:14 -0400, Lewis Hyatt wrote:
> > This patch enhances location_get_source_line(), which is the primary
> > interface provided by the diagnostics infrastructure to obtain the line of
> > source code corresponding to a given location, so that it understands
> > generated data locations in addition to normal file-based locations. This
> > involves changing the argument to location_get_source_line() from a plain
> > file name, to a source_id object that can represent either type of location.
> > 
> > gcc/ChangeLog:
> > 
> >         * input.cc (class data_cache_slot): New class.
> >         (file_cache::lookup_data): New function.
> >         (diagnostics_file_cache_forcibly_evict_data): New function.
> >         (file_cache::forcibly_evict_data): New function.
> >         (file_cache::evicted_cache_tab_entry): Generalize (via a template)
> >         to work for both file_cache_slot and data_cache_slot.
> >         (file_cache::add_file): Adapt for new interface to
> >         evicted_cache_tab_entry.
> >         (file_cache::add_data): New function.
> >         (data_cache_slot::create): New function.
> >         (file_cache::file_cache): Support the new m_data_slots member.
> >         (file_cache::~file_cache): Likewise.
> >         (file_cache::lookup_or_add_data): New function.
> >         (file_cache::lookup_or_add): New function that calls either
> >         lookup_or_add_data or lookup_or_add_file as appropriate.
> >         (location_get_source_line): Change the FILE_PATH argument to a
> >         source_id SRC, and use it to support obtaining source lines from
> >         generated data as well as from files.
> >         (location_compute_display_column): Support generated data using the
> >         new features of location_get_source_line.
> >         (dump_location_info): Likewise.
> >         * input.h (location_get_source_line): Adjust prototype. Add a new
> >         convenience overload taking an expanded_location.
> >         (class cache_data_source): Declare.
> >         (class data_cache_slot): Declare.
> >         (class file_cache): Declare new members.
> >         (diagnostics_file_cache_forcibly_evict_data): Declare.
> > ---
> >  gcc/input.cc | 171 ++++++++++++++++++++++++++++++++++++++++-----------
> >  gcc/input.h  |  23 +++++--
> >  2 files changed, 153 insertions(+), 41 deletions(-)
> > 
> > diff --git a/gcc/input.cc b/gcc/input.cc
> > index 9377020b460..790279d4273 100644
> > --- a/gcc/input.cc
> > +++ b/gcc/input.cc
> > @@ -207,6 +207,28 @@ private:
> >    void maybe_grow ();
> >  };
> >  
> > +/* This is the implementation of cache_data_source for generated
> > +   data that is already in memory.  */
> > +class data_cache_slot final : public cache_data_source
> 
> It occurred to me: why are we caching accessing a buffer that's already
> in memory - but we're also caching the line-splitting information, and
> providing the line-splitting algorithm with a consistent interface to
> the data, right?
>

Yeah, for the current _Pragma use case, multi-line buffers are not going to
be common, but they can occur. I was mainly motivated by the consistent
interface, and by the assumption that the overhead is not critical given a
diagnostic is being issued.

> [...snip...]
> 
> > @@ -397,6 +434,15 @@ diagnostics_file_cache_forcibly_evict_file (const char *file_path)
> >    global_dc->m_file_cache->forcibly_evict_file (file_path);
> >  }
> >  
> > +void
> > +diagnostics_file_cache_forcibly_evict_data (const char *data,
> > +                                           unsigned int data_len)
> > +{
> > +  if (!global_dc->m_file_cache)
> > +    return;
> > +  global_dc->m_file_cache->forcibly_evict_data (data, data_len);
> 
> Maybe we should rename diagnostic_context's m_file_cache to
> m_source_cache?  (and class file_cache for that matter?)  But if so,
> that can/should be a followup/separate patch.
>

Yes, we should. Believe it or not, I was trying to minimize the size of the
patch :) So I didn't make such changes, but they will make things more
clear.

> [...snip...]
>  
> > @@ -525,10 +582,22 @@ file_cache_slot::create (const file_cache::input_context &in_context,
> >    return true;
> >  }
> >  
> > +void
> > +data_cache_slot::create (const char *data, unsigned int data_len,
> > +                        unsigned int highest_use_count)
> > +{
> > +  reset ();
> > +  on_create (highest_use_count + 1,
> > +            total_lines_num (source_id {data, data_len}));
> > +  m_data_begin = data;
> > +  m_data_end = data + data_len;
> > +}
> > +
> >  /* file_cache's ctor.  */
> >  
> >  file_cache::file_cache ()
> > -: m_file_slots (new file_cache_slot[num_file_slots])
> > +  : m_file_slots (new file_cache_slot[num_file_slots]),
> > +    m_data_slots (new data_cache_slot[num_file_slots])
> 
> Should "num_file_slots" be renamed to "num_slots"?
> 
> I assume you're using the same value for both kinds of slot since the
> file_cache::evicted_cache_tab_entry template uses this.  I suppose the
> number could be passed in as an argument to that function if we wanted
> to have different sizes for the two kinds, but I don't think it
> matters.
>

Yes that's right... would rename num_file_slots too.

> [...snip...]
> 
> > @@ -912,26 +1000,22 @@ cache_data_source::read_line_num (size_t line_num,
> >     If the function fails, a NULL char_span is returned.  */
> >  
> >  char_span
> > -location_get_source_line (const char *file_path, int line)
> > +location_get_source_line (source_id src, int line)
> >  {
> > -  const char *buffer = NULL;
> > -  ssize_t len;
> > -
> > -  if (line == 0)
> > -    return char_span (NULL, 0);
> > -
> > -  if (file_path == NULL)
> > -    return char_span (NULL, 0);
> > +  const char_span fail (nullptr, 0);
> > +  if (!src || line <= 0)
> > +    return fail;
> 
> Looking at source_id's operator bool, are there effectively three kinds
> of source_id?
> 
> (a) file names
> (b) generated buffer
> (c) NULL == m_filename_or_buffer
> 
> What does (c) mean?  Is it a "something's gone wrong/error" state?  Or
> is this more a special-case of (a)? (in that the m_len for such a case
> would be zero)
> 
> Should source_id's 2-param ctor have an assert that the ptr is non-
> NULL?
> 
> [...snip...]
> 
> The patch is OK for trunk as-is, but note the question about the
> source_id ctor above.
> 

Thanks. (c) has the same meaning as a NULL file name currently does, so a
default-constructed source_id is not an in-memory buffer, but is rather a
NULL filename. linemap_add() for instance, will interpret a NULL filename
for an LC_LEAVE map, as a request to copy it from the natural values being
returned to. I think the source_id constructor needs to accept a NULL
filename to remain backwards compatible. With the current design of
source_id, it is safe always to change a 'const char*' file name argument to
a source_id argument instead; it will work just how it did before because it
has an implicit constructor. But if the constructor would assert on a
non-NULL pointer, that would necessitate changing all call sites that
currently expect they can pass a NULL pointer there. (For example, there are
several calls to _cpp_do_file_change() within libcpp that take advantage of
being able to pass a NULL filename to linemap_add.)

-Lewis