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

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

[David Malcolm]

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;
-- 
2.26.3

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

[Antoni Boucher]

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;

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

[Richard Biener]

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
>

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

[Antoni Boucher]

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
> > 

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

[Antoni Boucher]

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
> > > 
> 

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

[David Malcolm]

On Tue, 2023-09-12 at 13:36 -0400, Antoni Boucher wrote:
> 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");
>     }
> }

Can we get this in bugzilla please?  If you generate a .c version of
the context (via gcc_jit_context_dump_reproducer_to_file) I can try to
debug it.

Thanks
Dave

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

[Antoni Boucher]

I added it to bugzilla here:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=111396

Since this only reproduces part of the issue, please let me test again
with rustc_codegen_gcc after adding the missing fix.

I confirmed that the fix is in
https://github.com/antoyo/gcc/commit/9d5b6b20efa20825926196759d50706a604c64a8
so you might as well include all of this (except the linetable
condition in toplev.cc).

On Tue, 2023-09-12 at 14:38 -0400, David Malcolm wrote:
> On Tue, 2023-09-12 at 13:36 -0400, Antoni Boucher wrote:
> > 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");
> >     }
> > }
> 
> Can we get this in bugzilla please?  If you generate a .c version of
> the context (via gcc_jit_context_dump_reproducer_to_file) I can try
> to
> debug it.
> 
> Thanks
> Dave
> 

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

[David Malcolm]

On Tue, 2023-09-12 at 15:20 -0400, Antoni Boucher wrote:

FWIW I've pushed the "ggc, jit: forcibly clear GTY roots in jit" to
trunk after retesting it, as r14-4003-geaa8e8541349df.

> I added it to bugzilla here:
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=111396

I don't yet have a fix for this issue.

Dave


> 
> Since this only reproduces part of the issue, please let me test
> again
> with rustc_codegen_gcc after adding the missing fix.
> 
> I confirmed that the fix is in
> https://github.com/antoyo/gcc/commit/9d5b6b20efa20825926196759d50706a604c64a8
> so you might as well include all of this (except the linetable
> condition in toplev.cc).
> 
> On Tue, 2023-09-12 at 14:38 -0400, David Malcolm wrote:
> > On Tue, 2023-09-12 at 13:36 -0400, Antoni Boucher wrote:
> > > 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");
> > >     }
> > > }
> > 
> > Can we get this in bugzilla please?  If you generate a .c version
> > of
> > the context (via gcc_jit_context_dump_reproducer_to_file) I can try
> > to
> > debug it.
> > 
> > Thanks
> > Dave
> > 
>