Re: [PATCH v4 4/8] diagnostics: Support obtaining source code lines from generated data buffers
On Tue, Aug 15, 2023 at 04:08:47PM -0400, Lewis Hyatt wrote:
> On Tue, Aug 15, 2023 at 3:46 PM David Malcolm wrote:
> >
> > On Tue, 2023-08-15 at 14:15 -0400, Lewis Hyatt wrote:
> > > 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.
> > > > >
> >
> > [...]
> >
> > > > >
> > > > >
> > > > > 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.
> >
> > (nods)
> >
> > >
> > > > [...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 :)
> >
> > :)
> >
> > Thanks for splitting it up, BTW.
> >
> > [...]
> >
> >
> > > >
> > > > > @@ -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
> > >
Re: [PATCH v4 4/8] diagnostics: Support obtaining source code lines from generated data buffers
On Tue, Aug 15, 2023 at 3:46 PM David Malcolm wrote:
>
> On Tue, 2023-08-15 at 14:15 -0400, Lewis Hyatt wrote:
> > 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.
> > > >
>
> [...]
>
> > > >
> > > >
> > > > 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.
>
> (nods)
>
> >
> > > [...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 :)
>
> :)
>
> Thanks for splitting it up, BTW.
>
> [...]
>
>
> > >
> > > > @@ -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
Re: [PATCH v4 4/8] diagnostics: Support obtaining source code lines from generated data buffers
On Tue, 2023-08-15 at 14:15 -0400, Lewis Hyatt wrote:
> 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.
> > >
[...]
> > >
> > >
> > > 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.
(nods)
>
> > [...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 :)
:)
Thanks for splitting it up, BTW.
[...]
> >
> > > @@ -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.)
Yes, it's OK for this ctor to accept NULL;
source_id (const char *filename = nullptr)
and I see you added the default arg.
I was referring to this ctor:
source_id (const char *buffer, unsigned buffer_len)
Is it ever OK for "buffer" to be NULL in this 2-param ctor, or can we
assert
Re: [PATCH v4 4/8] diagnostics: Support obtaining source code lines from generated data buffers
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 _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
Re: [PATCH v4 4/8] diagnostics: Support obtaining source code lines from generated data buffers
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?
[...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.
[...snip...]
> @@ -525,10 +582,22 @@ file_cache_slot::create (const
> file_cache::input_context _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.
[...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
[PATCH v4 4/8] diagnostics: Support obtaining source code lines from generated data buffers
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
+{
+public:
+ void create (const char *data, unsigned int data_len,
+ unsigned int highest_use_count);
+ bool represents_data (const char *data, unsigned int) const
+ {
+/* We can just use pointer equality here since the generated data lives in
+ memory in one persistent place. It isn't anticipated there would be
+ several generated data buffers with the same content, so we don't mind
+ that in such a case we will store it twice. */
+return m_data_begin == data;
+ }
+
+protected:
+ /* In contrast to file_cache_slot, we do not own a buffer. The buffer
+ passed to create() needs to outlive this object. */
+ bool get_more_data () override { return false; }
+};
+
/* Current position in real source file. */
location_t input_location = UNKNOWN_LOCATION;
@@ -382,6 +404,21 @@ file_cache::lookup_file (const char *file_path)
return r;
}
+data_cache_slot *
+file_cache::lookup_data (const char *data, unsigned int data_len)
+{
+ for (unsigned int i = 0; i != num_file_slots; ++i)
+{
+ const auto slot = m_data_slots + i;
+ if (slot->represents_data (data, data_len))
+ {
+ slot->inc_use_count ();
+ return slot;
+ }
+}
+ return nullptr;
+}
+
/* Purge any mention of FILENAME from the cache of files used for
printing source code. For use in selftests when working
with tempfiles. */
@@ -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);
+}
+
void
file_cache::forcibly_evict_file (const char *file_path)
{
@@ -410,36 +456,36 @@ file_cache::forcibly_evict_file (const char *file_path)
r->reset ();
}
+void
+file_cache::forcibly_evict_data (const char *data, unsigned int data_len)
+{
+ if (auto r = lookup_data (data, data_len))
+r->reset ();
+}
+
/* Return the cache that has been less used, recently, or the
first empty one. If HIGHEST_USE_COUNT is non-null,
*HIGHEST_USE_COUNT is set to the highest use count of the entries
in the cache table. */
-file_cache_slot*
-file_cache::evicted_cache_tab_entry (unsigned *highest_use_count)
+template
+Slot *
+file_cache::evicted_cache_tab_entry (Slot
