Hi.
I'll do another test to make sure this is enough since I tested with a
few more finalize functions.
Thanks a lot for finding this!
On Wed, 2023-09-06 at 09:40 -0400, David Malcolm via Jit wrote:
> As part of Antoyo's work on supporting LTO in rustc_codegen_gcc, he
> noticed an ICE inside libgccjit when compiling certain rust files.
>
> Debugging libgccjit showed that outdated information from a previous
> in-memory compile was referring to ad-hoc locations in the previous
> compile's line_table.
>
> The issue turned out to be the function decls in
> internal_fn_fnspec_array
> from the previous compile keeping alive the symtab nodes for these
> functions, and from this finding other functions in the previous
> compile, walking their CFGs, and finding ad-hoc data pointers in an
> edge
> with a location_t using ad-hoc data from the previous line_table
> instance, and thus a use-after-free ICE attempting to use this ad-hoc
> data.
>
> Previously in toplev::finalize we've fixed global state "piecemeal"
> by
> calling out to individual source_name_cc_finalize functions.
> However,
> it occurred to me that we have run-time information on where the
> GTY-marked pointers are.
>
> Hence this patch takes something of a "big hammer" approach by adding
> a
> new ggc_common_finalize that walks the GC roots, zeroing all of the
> pointers. I stepped through this in the debugger and observed that,
> in
> particular, this correctly zeroes the internal_fn_fnspec_array at the
> end
> of a libgccjit compile. Antoyo reports that this fixes the ICE for
> him.
> Doing so uncovered an ICE with libgccjit in dwarf2cfi.cc due to reuse
> of
> global variables from the previous compile, which this patch also
> fixes.
>
> I noticed that in ggc_mark_roots when clearing deletable roots we
> only
> clear the initial element in each gcc_root_tab_t. This looks like a
> latent bug to me, which the patch fixes. That said, there don't seem
> to
> be any deletable roots where the number of elements != 1.
>
> Successfully bootstrapped & regrtested on x86_64-pc-linux-gnu.
>
> OK for trunk?
>
> Thanks
> Dave
>
> gcc/ChangeLog:
> * dwarf2cfi.cc (dwarf2cfi_cc_finalize): New.
> * dwarf2out.h (dwarf2cfi_cc_finalize): New decl.
> * ggc-common.cc (ggc_mark_roots): Multiply by rti->nelt when
> clearing the deletable gcc_root_tab_t.
> (ggc_common_finalize): New.
> * ggc.h (ggc_common_finalize): New decl.
> * toplev.cc (toplev::finalize): Call dwarf2cfi_cc_finalize
> and
> ggc_common_finalize.
> ---
> gcc/dwarf2cfi.cc | 9 +++++++++
> gcc/dwarf2out.h | 1 +
> gcc/ggc-common.cc | 23 ++++++++++++++++++++++-
> gcc/ggc.h | 2 ++
> gcc/toplev.cc | 3 +++
> 5 files changed, 37 insertions(+), 1 deletion(-)
>
> diff --git a/gcc/dwarf2cfi.cc b/gcc/dwarf2cfi.cc
> index ddc728f4ad00..f1777c0a4cf1 100644
> --- a/gcc/dwarf2cfi.cc
> +++ b/gcc/dwarf2cfi.cc
> @@ -3822,4 +3822,13 @@ make_pass_dwarf2_frame (gcc::context *ctxt)
> return new pass_dwarf2_frame (ctxt);
> }
>
> +void dwarf2cfi_cc_finalize ()
> +{
> + add_cfi_insn = NULL;
> + add_cfi_vec = NULL;
> + cur_trace = NULL;
> + cur_row = NULL;
> + cur_cfa = NULL;
> +}
> +
> #include "gt-dwarf2cfi.h"
> diff --git a/gcc/dwarf2out.h b/gcc/dwarf2out.h
> index 870b56a6a372..61a996050ff9 100644
> --- a/gcc/dwarf2out.h
> +++ b/gcc/dwarf2out.h
> @@ -419,6 +419,7 @@ struct fixed_point_type_info
> } scale_factor;
> };
>
> +void dwarf2cfi_cc_finalize (void);
> void dwarf2out_cc_finalize (void);
>
> /* Some DWARF internals are exposed for the needs of DWARF-based
> debug
> diff --git a/gcc/ggc-common.cc b/gcc/ggc-common.cc
> index bed7a9d4d021..95803fa95a17 100644
> --- a/gcc/ggc-common.cc
> +++ b/gcc/ggc-common.cc
> @@ -86,7 +86,7 @@ ggc_mark_roots (void)
>
> for (rt = gt_ggc_deletable_rtab; *rt; rt++)
> for (rti = *rt; rti->base != NULL; rti++)
> - memset (rti->base, 0, rti->stride);
> + memset (rti->base, 0, rti->stride * rti->nelt);
>
> for (rt = gt_ggc_rtab; *rt; rt++)
> ggc_mark_root_tab (*rt);
> @@ -1293,3 +1293,24 @@ report_heap_memory_use ()
> SIZE_AMOUNT (MALLINFO_FN ().arena));
> #endif
> }
> +
> +/* Forcibly clear all GTY roots. */
> +
> +void
> +ggc_common_finalize ()
> +{
> + const struct ggc_root_tab *const *rt;
> + const_ggc_root_tab_t rti;
> +
> + for (rt = gt_ggc_deletable_rtab; *rt; rt++)
> + for (rti = *rt; rti->base != NULL; rti++)
> + memset (rti->base, 0, rti->stride * rti->nelt);
> +
> + for (rt = gt_ggc_rtab; *rt; rt++)
> + for (rti = *rt; rti->base != NULL; rti++)
> + memset (rti->base, 0, rti->stride * rti->nelt);
> +
> + for (rt = gt_pch_scalar_rtab; *rt; rt++)
> + for (rti = *rt; rti->base != NULL; rti++)
> + memset (rti->base, 0, rti->stride * rti->nelt);
> +}
> diff --git a/gcc/ggc.h b/gcc/ggc.h
> index 34108e2f0061..3280314f8481 100644
> --- a/gcc/ggc.h
> +++ b/gcc/ggc.h
> @@ -368,4 +368,6 @@ inline void gt_ggc_mx (unsigned long int) { }
> inline void gt_ggc_mx (long long int) { }
> inline void gt_ggc_mx (unsigned long long int) { }
>
> +extern void ggc_common_finalize ();
> +
> #endif
> diff --git a/gcc/toplev.cc b/gcc/toplev.cc
> index 6c1a6f443c16..db62e3e995ec 100644
> --- a/gcc/toplev.cc
> +++ b/gcc/toplev.cc
> @@ -2336,6 +2336,7 @@ toplev::finalize (void)
> cgraph_cc_finalize ();
> cgraphunit_cc_finalize ();
> symtab_thunks_cc_finalize ();
> + dwarf2cfi_cc_finalize ();
> dwarf2out_cc_finalize ();
> gcse_cc_finalize ();
> ipa_cp_cc_finalize ();
> @@ -2350,6 +2351,8 @@ toplev::finalize (void)
> save_decoded_options = NULL;
> save_decoded_options_count = 0;
>
> + ggc_common_finalize ();
> +
> /* Clean up the context (and pass_manager etc). */
> delete g;
> g = NULL;
