Re: [PATCH] ggc, jit: forcibly clear GTY roots in jit

[Antoni Boucher <bouanto@zoho.com>]

In the mean time, here's a (Rust) reproducer for the issue:

fn main() {
    for _ in 0..5 {
        let context = Context::default();
        context.add_command_line_option("-flto");
        context.set_optimization_level(OptimizationLevel::Aggressive);
        context.add_driver_option("-nostdlib");

        let int_type = context.new_type::<i32>();

        let function = context.new_function(None,
FunctionType::Exported, int_type, &[], "main", false);
        let block = function.new_block("start");
        let value = context.new_rvalue_from_int(int_type, 42);
        block.end_with_return(None, value);

        context.compile_to_file(OutputKind::Executable, "my_exe");
    }
}

On Tue, 2023-09-12 at 12:00 -0400, Antoni Boucher via Jit wrote:
> It seems to not be enough to fix the issue.
> Let me find out what's missing from my patch.
> 
> On Tue, 2023-09-12 at 11:35 +0200, Richard Biener via Jit wrote:
> > On Wed, Sep 6, 2023 at 3:41 PM David Malcolm via Gcc-patches
> > <gcc-patches@gcc.gnu.org> 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?
> > 
> > OK.
> > 
> > Thanks,
> > Richard.
> > 
> > > 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;
> > > --
> > > 2.26.3
> > > 
>