> Am 27.02.2025 um 20:54 schrieb Jakub Jelinek <ja...@redhat.com>:
>
> Hi!
>
> The following testcase ICEs, because we first construct file_cache object
> inside of *global_dc, then process options and then call file_cache::tune.
> The earlier construction allocates the m_file_slots array (using new)
> based on the static data member file_cache::num_file_slots, but then tune
> changes it, without actually reallocating all m_file_slots arrays in already
> constructed file_cache objects.
>
> I think it is just weird to have the count be a static data member and
> the pointer be non-static data member, that is just asking for issues like
> this.
>
> So, this patch changes num_file_slots into m_num_file_slots and turns tune
> into a non-static member function and changes toplev.cc to call it on the
> global_gc->get_file_cache () object. And let's the tune just delete the
> array and allocate it freshly if there is a change in the number of slots
> or lines.
>
> Note, file_cache_slot has similar problem, but because there are many, I
> haven't moved the count into those objects; I just hope that when tune
> is called there is exactly one file_cache constructed and all the
> file_cache_slot objects constructed are pointed by its m_file_slots member,
> so also on lines change it just deletes it and allocates again. I think
> it should be unlikely that the cache actually has any used slots by the time
> it is called.
>
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
Ok
Richard
> 2025-02-27 Jakub Jelinek <ja...@redhat.com>
>
> PR middle-end/118860
> * input.h (file_cache::tune): No longer static. Rename argument
> from num_file_slots_ to num_file_slots. Formatting fix.
> (file_cache::num_file_slots): Renamed to ...
> (file_cache::m_num_file_slots): ... this. No longer static.
> * input.cc (file_cache_slot::tune): Change return type from void to
> size_t, return previous file_cache_slot::line_record_size value.
> Formatting fixes.
> (file_cache::tune): Rename argument from num_file_slots_ to
> num_file_slots. Set m_num_file_slots rather than num_file_slots.
> If m_num_file_slots or file_cache_slot::line_record_size changes,
> delete[] m_file_slots and new it again.
> (file_cache::num_file_slots): Remove definition.
> (file_cache::lookup_file): Use m_num_file_slots rather than
> num_file_slots.
> (file_cache::evicted_cache_tab_entry): Likewise.
> (file_cache::file_cache): Likewise. Initialize m_num_file_slots
> to 16.
> (file_cache::dump): Use m_num_file_slots rather than num_file_slots.
> (file_cache_slot::get_next_line): Formatting fixes.
> (file_cache_slot::read_line_num): Likewise.
> (get_source_text_between): Likewise.
> * toplev.cc (toplev::main): Call global_dc->get_file_cache ().tune
> rather than file_cache::tune.
>
> * gcc.dg/pr118860.c: New test.
>
> --- gcc/input.h.jj 2025-02-03 11:05:04.833651537 +0100
> +++ gcc/input.h 2025-02-27 10:38:00.943010438 +0100
> @@ -161,7 +161,7 @@ class file_cache
> const char *buffer,
> size_t sz);
>
> - static void tune(size_t num_file_slots_, size_t lines);
> + void tune (size_t num_file_slots, size_t lines);
>
> private:
> file_cache_slot *evicted_cache_tab_entry (unsigned *highest_use_count);
> @@ -169,7 +169,7 @@ class file_cache
> file_cache_slot *lookup_file (const char *file_path);
>
> private:
> - static size_t num_file_slots;
> + size_t m_num_file_slots;
> file_cache_slot *m_file_slots;
> input_context m_input_context;
> };
> --- gcc/input.cc.jj 2025-02-24 00:06:26.036728909 +0100
> +++ gcc/input.cc 2025-02-27 11:00:59.351807727 +0100
> @@ -79,8 +79,11 @@ public:
> void evict ();
> void set_content (const char *buf, size_t sz);
>
> - static void tune(size_t line_record_size_) {
> - line_record_size = line_record_size_;
> + static size_t tune (size_t line_record_size_)
> + {
> + size_t ret = line_record_size;
> + line_record_size = line_record_size_;
> + return ret;
> }
>
> private:
> @@ -200,14 +203,17 @@ size_t file_cache_slot::recent_cached_li
>
> /* Tune file_cache. */
> void
> -file_cache::tune (size_t num_file_slots_, size_t lines)
> +file_cache::tune (size_t num_file_slots, size_t lines)
> {
> - num_file_slots = num_file_slots_;
> - file_cache_slot::tune (lines);
> + if (file_cache_slot::tune (lines) != lines
> + || m_num_file_slots != num_file_slots)
> + {
> + delete[] m_file_slots;
> + m_file_slots = new file_cache_slot[num_file_slots];
> + }
> + m_num_file_slots = num_file_slots;
> }
>
> -size_t file_cache::num_file_slots = 16;
> -
> static const char *
> find_end_of_line (const char *s, size_t len);
>
> @@ -325,7 +331,7 @@ file_cache::lookup_file (const char *fil
>
> /* This will contain the found cached file. */
> file_cache_slot *r = NULL;
> - for (unsigned i = 0; i < num_file_slots; ++i)
> + for (unsigned i = 0; i < m_num_file_slots; ++i)
> {
> file_cache_slot *c = &m_file_slots[i];
> if (c->get_file_path () && !strcmp (c->get_file_path (), file_path))
> @@ -419,7 +425,7 @@ file_cache::evicted_cache_tab_entry (uns
> {
> file_cache_slot *to_evict = &m_file_slots[0];
> unsigned huc = to_evict->get_use_count ();
> - for (unsigned i = 1; i < num_file_slots; ++i)
> + for (unsigned i = 1; i < m_num_file_slots; ++i)
> {
> file_cache_slot *c = &m_file_slots[i];
> bool c_is_empty = (c->get_file_path () == NULL);
> @@ -449,7 +455,7 @@ file_cache::evicted_cache_tab_entry (uns
> accessed by caret diagnostic. This cache is added to an array of
> cache and can be retrieved by lookup_file_in_cache_tab. This
> function returns the created cache. Note that only the last
> - num_file_slots files are cached.
> + m_num_file_slots files are cached.
>
> This can return nullptr if the FILE_PATH can't be opened for
> reading, or if the content can't be converted to the input_charset. */
> @@ -558,7 +564,7 @@ file_cache_slot::set_content (const char
> /* file_cache's ctor. */
>
> file_cache::file_cache ()
> -: m_file_slots (new file_cache_slot[num_file_slots])
> +: m_num_file_slots (16), m_file_slots (new file_cache_slot[m_num_file_slots])
> {
> initialize_input_context (nullptr, false);
> }
> @@ -573,7 +579,7 @@ file_cache::~file_cache ()
> void
> file_cache::dump (FILE *out, int indent) const
> {
> - for (size_t i = 0; i < num_file_slots; ++i)
> + for (size_t i = 0; i < m_num_file_slots; ++i)
> {
> fprintf (out, "%*sslot[%i]:\n", indent, "", (int)i);
> m_file_slots[i].dump (out, indent + 2);
> @@ -869,10 +875,12 @@ file_cache_slot::get_next_line (char **l
> /* Only update when beyond the previously cached region. */
> if (rlen == 0 || m_line_record[rlen - 1].line_num < m_line_num)
> {
> - size_t spacing = rlen >= 2 ?
> - m_line_record[rlen - 1].line_num - m_line_record[rlen - 2].line_num : 1;
> - size_t delta = rlen >= 1 ?
> - m_line_num - m_line_record[rlen - 1].line_num : 1;
> + size_t spacing
> + = (rlen >= 2
> + ? (m_line_record[rlen - 1].line_num
> + - m_line_record[rlen - 2].line_num) : 1);
> + size_t delta
> + = rlen >= 1 ? m_line_num - m_line_record[rlen - 1].line_num : 1;
>
> size_t max_size = line_record_size;
> /* One anchor per hundred input lines. */
> @@ -880,7 +888,7 @@ file_cache_slot::get_next_line (char **l
> max_size = m_line_num / 100;
>
> /* If we're too far beyond drop half of the lines to rebalance. */
> - if (rlen == max_size && delta >= spacing*2)
> + if (rlen == max_size && delta >= spacing * 2)
> {
> size_t j = 0;
> for (size_t i = 1; i < rlen; i += 2)
> @@ -891,24 +899,25 @@ file_cache_slot::get_next_line (char **l
> }
>
> if (rlen < max_size && delta >= spacing)
> - m_line_record.safe_push
> - (file_cache_slot::line_info (m_line_num,
> - m_line_start_idx,
> - line_end - m_data));
> + {
> + file_cache_slot::line_info li (m_line_num, m_line_start_idx,
> + line_end - m_data);
> + m_line_record.safe_push (li);
> + }
> }
>
> /* Cache recent tail lines separately for fast access. This assumes
> most accesses do not skip backwards. */
> if (m_line_recent_last == m_line_recent_first
> - || m_line_recent[m_line_recent_last].line_num == m_line_num - 1)
> + || m_line_recent[m_line_recent_last].line_num == m_line_num - 1)
> {
> - size_t mask = ((size_t)1 << recent_cached_lines_shift) - 1;
> + size_t mask = ((size_t) 1 << recent_cached_lines_shift) - 1;
> m_line_recent_last = (m_line_recent_last + 1) & mask;
> if (m_line_recent_last == m_line_recent_first)
> m_line_recent_first = (m_line_recent_first + 1) & mask;
> - m_line_recent[m_line_recent_last] =
> - file_cache_slot::line_info (m_line_num, m_line_start_idx,
> - line_end - m_data);
> + m_line_recent[m_line_recent_last]
> + = file_cache_slot::line_info (m_line_num, m_line_start_idx,
> + line_end - m_data);
> }
>
> /* Update m_line_start_idx so that it points to the next line to be
> @@ -952,7 +961,7 @@ file_cache_slot::goto_next_line ()
>
> bool
> file_cache_slot::read_line_num (size_t line_num,
> - char ** line, ssize_t *line_len)
> + char ** line, ssize_t *line_len)
> {
> gcc_assert (line_num > 0);
>
> @@ -1044,10 +1053,10 @@ file_cache::get_source_line (const char
> char *
> get_source_text_between (file_cache &fc, location_t start, location_t end)
> {
> - expanded_location expstart =
> - expand_location_to_spelling_point (start, LOCATION_ASPECT_START);
> - expanded_location expend =
> - expand_location_to_spelling_point (end, LOCATION_ASPECT_FINISH);
> + expanded_location expstart
> + = expand_location_to_spelling_point (start, LOCATION_ASPECT_START);
> + expanded_location expend
> + = expand_location_to_spelling_point (end, LOCATION_ASPECT_FINISH);
>
> /* If the locations are in different files or the end comes before the
> start, give up and return nothing. */
> --- gcc/toplev.cc.jj 2025-02-04 09:16:14.453667879 +0100
> +++ gcc/toplev.cc 2025-02-27 10:59:48.251798091 +0100
> @@ -2333,7 +2333,8 @@ toplev::main (int argc, char **argv)
> UNKNOWN_LOCATION, global_dc,
> targetm.target_option.override);
>
> - file_cache::tune (param_file_cache_files, param_file_cache_lines);
> + global_dc->get_file_cache ().tune (param_file_cache_files,
> + param_file_cache_lines);
>
> handle_common_deferred_options ();
>
> --- gcc/testsuite/gcc.dg/pr118860.c.jj 2025-02-27 11:15:07.877988430 +0100
> +++ gcc/testsuite/gcc.dg/pr118860.c 2025-02-27 11:14:46.445286981 +0100
> @@ -0,0 +1,5 @@
> +/* PR middle-end/118860 */
> +/* { dg-do compile } */
> +/* { dg-options "--param=file-cache-files=180 -Wunused" } */
> +
> +static void foo (); /* { dg-warning "'foo' declared 'static' but never
> defined" } */
>
> Jakub
>
