libgccjit: Fix GGC segfault when using -flto
Checks
Commit Message
Hi.
This patch fixes the segfault when using -flto with libgccjit (bug
111396).
You mentioned in bugzilla that this didn't fix the reproducer for you,
but it does for me.
At first, the test case would not pass, but running "make install" made
it pass.
Not sure if this is normal.
Could you please check if this fixes the issue on your side as well?
Since this patch changes files outside of gcc/jit, what tests should I
run to make sure it didn't break anything?
Thanks for the review.
Comments
On Fri, 2023-11-10 at 11:02 -0500, Antoni Boucher wrote:
> Hi.
> This patch fixes the segfault when using -flto with libgccjit (bug
> 111396).
>
> You mentioned in bugzilla that this didn't fix the reproducer for
> you,
Rereading https://gcc.gnu.org/bugzilla/show_bug.cgi?id=111396 it looks
like all I tested back in August was your reproducer; I didn't yet test
your patch.
> but it does for me.
> At first, the test case would not pass, but running "make install"
> made
> it pass.
> Not sure if this is normal.
>
> Could you please check if this fixes the issue on your side as well?
> Since this patch changes files outside of gcc/jit, what tests should
> I
> run to make sure it didn't break anything?
I'm trying your patch in my tester now.
BTW, we shouldn't add test-ggc-bugfix to since it adds options to the
context: this would affect all the other tests.
Dave
On Fri, 2023-11-10 at 18:14 -0500, David Malcolm wrote:
> On Fri, 2023-11-10 at 11:02 -0500, Antoni Boucher wrote:
> > Hi.
> > This patch fixes the segfault when using -flto with libgccjit (bug
> > 111396).
> >
> > You mentioned in bugzilla that this didn't fix the reproducer for
> > you,
>
> Rereading https://gcc.gnu.org/bugzilla/show_bug.cgi?id=111396 it
> looks
> like all I tested back in August was your reproducer; I didn't yet
> test
> your patch.
>
> > but it does for me.
> > At first, the test case would not pass, but running "make install"
> > made
> > it pass.
> > Not sure if this is normal.
> >
> > Could you please check if this fixes the issue on your side as
> > well?
> > Since this patch changes files outside of gcc/jit, what tests
> > should
> > I
> > run to make sure it didn't break anything?
>
> I'm trying your patch in my tester now.
Bootstrapped with x86_64-pc-linux-gnu/build. No changes to non-jit
tests, but had this effect on jit.sum:
Changes to jit.sum
------------------
FAIL: 9->11 (+2)
PASS: 14827->11434 (-3393)
apparently due to:
FAIL: test-combination.c.exe iteration 1 of 5: verify_code_accessing_bitfield: result is NULL
FAIL: test-combination.c.exe killed: 997638 exp16 0 0 CHILDKILLED SIGABRT SIGABRT
>
> BTW, we shouldn't add test-ggc-bugfix to since it adds options to the
> context: this would affect all the other tests.
Here's the updated patch.
The failure was due to the test being in the test array while it should
not have been there since it changes the context.
Thanks for the review.
On Sun, 2023-11-12 at 18:03 -0500, David Malcolm wrote:
> On Fri, 2023-11-10 at 18:14 -0500, David Malcolm wrote:
> > On Fri, 2023-11-10 at 11:02 -0500, Antoni Boucher wrote:
> > > Hi.
> > > This patch fixes the segfault when using -flto with libgccjit
> > > (bug
> > > 111396).
> > >
> > > You mentioned in bugzilla that this didn't fix the reproducer for
> > > you,
> >
> > Rereading https://gcc.gnu.org/bugzilla/show_bug.cgi?id=111396 it
> > looks
> > like all I tested back in August was your reproducer; I didn't yet
> > test
> > your patch.
> >
> > > but it does for me.
> > > At first, the test case would not pass, but running "make
> > > install"
> > > made
> > > it pass.
> > > Not sure if this is normal.
> > >
> > > Could you please check if this fixes the issue on your side as
> > > well?
> > > Since this patch changes files outside of gcc/jit, what tests
> > > should
> > > I
> > > run to make sure it didn't break anything?
> >
> > I'm trying your patch in my tester now.
>
> Bootstrapped with x86_64-pc-linux-gnu/build. No changes to non-jit
> tests, but had this effect on jit.sum:
>
> Changes to jit.sum
> ------------------
>
> FAIL: 9->11 (+2)
> PASS: 14827->11434 (-3393)
>
> apparently due to:
> FAIL: test-combination.c.exe iteration 1 of 5:
> verify_code_accessing_bitfield: result is NULL
> FAIL: test-combination.c.exe killed: 997638 exp16 0 0 CHILDKILLED
> SIGABRT SIGABRT
>
> >
> > BTW, we shouldn't add test-ggc-bugfix to since it adds options to
> > the
> > context: this would affect all the other tests.
>
>
On Thu, 2023-11-30 at 17:13 -0500, Antoni Boucher wrote:
> Here's the updated patch.
> The failure was due to the test being in the test array while it
> should
> not have been there since it changes the context.
Thanks for the updated patch.
Did you do a full bootstrap and regression test with this one, or do
you want me to?
Dave
I'm not sure how to do this. I tried the following commands, but this
fails even on master:
../../gcc/configure --enable-host-shared --enable-
languages=c,jit,c++,fortran,objc,lto --enable-checking=release --
disable-werror --prefix=/opt/gcc
make bootstrap -j24
make -k check -j24
From what I can understand, the unexpected failures are in g++:
=== g++ Summary ===
# of expected passes 72790
# of unexpected failures 1
# of expected failures 1011
# of unsupported tests 3503
=== g++ Summary ===
# of expected passes 4750
# of unexpected failures 27
# of expected failures 16
# of unsupported tests 43
Am I doing something wrong?
On Fri, 2023-12-01 at 12:49 -0500, David Malcolm wrote:
> On Thu, 2023-11-30 at 17:13 -0500, Antoni Boucher wrote:
> > Here's the updated patch.
> > The failure was due to the test being in the test array while it
> > should
> > not have been there since it changes the context.
>
> Thanks for the updated patch.
>
> Did you do a full bootstrap and regression test with this one, or do
> you want me to?
>
> Dave
>
On Mon, 2023-12-11 at 19:20 -0500, Antoni Boucher wrote:
> I'm not sure how to do this. I tried the following commands, but this
> fails even on master:
>
> ../../gcc/configure --enable-host-shared --enable-
> languages=c,jit,c++,fortran,objc,lto --enable-checking=release --
> disable-werror --prefix=/opt/gcc
>
> make bootstrap -j24
> make -k check -j24
>
> From what I can understand, the unexpected failures are in g++:
>
> === g++ Summary ===
>
> # of expected passes 72790
> # of unexpected failures 1
> # of expected failures 1011
> # of unsupported tests 3503
>
> === g++ Summary ===
>
> # of expected passes 4750
> # of unexpected failures 27
> # of expected failures 16
> # of unsupported tests 43
>
>
> Am I doing something wrong?
I normally do a pair of bootstrap/tests: a "control" build with a
pristine copy of the source tree, and an "experiment" build containing
the patch(s) of interest, then compare the results. FWIW given that
each one takes 2 hours on my machine, I normally just do one control
build on a Monday, rebase all my working copies to that revision, and
then use that control build throughout the week for comparison when
testing patches.
I can have a go at testing an updated patch if you like; presumably the
latest version is this one:
https://gcc.gnu.org/pipermail/gcc-patches/2023-November/638841.html
right?
Dave
>
> On Fri, 2023-12-01 at 12:49 -0500, David Malcolm wrote:
> > On Thu, 2023-11-30 at 17:13 -0500, Antoni Boucher wrote:
> > > Here's the updated patch.
> > > The failure was due to the test being in the test array while it
> > > should
> > > not have been there since it changes the context.
> >
> > Thanks for the updated patch.
> >
> > Did you do a full bootstrap and regression test with this one, or
> > do
> > you want me to?
> >
> > Dave
> >
>
On Wed, 2024-01-10 at 10:19 -0500, David Malcolm wrote:
> On Mon, 2023-12-11 at 19:20 -0500, Antoni Boucher wrote:
> > I'm not sure how to do this. I tried the following commands, but
> > this
> > fails even on master:
> >
> > ../../gcc/configure --enable-host-shared --enable-
> > languages=c,jit,c++,fortran,objc,lto --enable-checking=release --
> > disable-werror --prefix=/opt/gcc
> >
> > make bootstrap -j24
> > make -k check -j24
> >
> > From what I can understand, the unexpected failures are in g++:
> >
> > === g++ Summary ===
> >
> > # of expected passes 72790
> > # of unexpected failures 1
> > # of expected failures 1011
> > # of unsupported tests 3503
> >
> > === g++ Summary ===
> >
> > # of expected passes 4750
> > # of unexpected failures 27
> > # of expected failures 16
> > # of unsupported tests 43
> >
> >
> > Am I doing something wrong?
>
> I normally do a pair of bootstrap/tests: a "control" build with a
> pristine copy of the source tree, and an "experiment" build
> containing
> the patch(s) of interest, then compare the results. FWIW given that
> each one takes 2 hours on my machine, I normally just do one control
> build on a Monday, rebase all my working copies to that revision, and
> then use that control build throughout the week for comparison when
> testing patches.
>
> I can have a go at testing an updated patch if you like; presumably
> the
> latest version is this one:
> https://gcc.gnu.org/pipermail/gcc-patches/2023-November/638841.html
> right?
Thanks. I would appreciate if you do it.
Yes, this is the latest patch.
>
> Dave
>
>
>
> >
> > On Fri, 2023-12-01 at 12:49 -0500, David Malcolm wrote:
> > > On Thu, 2023-11-30 at 17:13 -0500, Antoni Boucher wrote:
> > > > Here's the updated patch.
> > > > The failure was due to the test being in the test array while
> > > > it
> > > > should
> > > > not have been there since it changes the context.
> > >
> > > Thanks for the updated patch.
> > >
> > > Did you do a full bootstrap and regression test with this one, or
> > > do
> > > you want me to?
> > >
> > > Dave
> > >
> >
>
On Wed, 2024-01-10 at 10:27 -0500, Antoni Boucher wrote:
> On Wed, 2024-01-10 at 10:19 -0500, David Malcolm wrote:
> > On Mon, 2023-12-11 at 19:20 -0500, Antoni Boucher wrote:
> > > I'm not sure how to do this. I tried the following commands, but
> > > this
> > > fails even on master:
> > >
> > > ../../gcc/configure --enable-host-shared --enable-
> > > languages=c,jit,c++,fortran,objc,lto --enable-checking=release --
> > > disable-werror --prefix=/opt/gcc
> > >
> > > make bootstrap -j24
> > > make -k check -j24
> > >
> > > From what I can understand, the unexpected failures are in g++:
> > >
> > > === g++ Summary ===
> > >
> > > # of expected passes 72790
> > > # of unexpected failures 1
> > > # of expected failures 1011
> > > # of unsupported tests 3503
> > >
> > > === g++ Summary ===
> > >
> > > # of expected passes 4750
> > > # of unexpected failures 27
> > > # of expected failures 16
> > > # of unsupported tests 43
> > >
> > >
> > > Am I doing something wrong?
> >
> > I normally do a pair of bootstrap/tests: a "control" build with a
> > pristine copy of the source tree, and an "experiment" build
> > containing
> > the patch(s) of interest, then compare the results. FWIW given
> > that
> > each one takes 2 hours on my machine, I normally just do one
> > control
> > build on a Monday, rebase all my working copies to that revision,
> > and
> > then use that control build throughout the week for comparison when
> > testing patches.
> >
> > I can have a go at testing an updated patch if you like; presumably
> > the
> > latest version is this one:
> > https://gcc.gnu.org/pipermail/gcc-patches/2023-November/638841.html
> > right?
>
> Thanks. I would appreciate if you do it.
> Yes, this is the latest patch.
Successfully bootstrapped & regrtested on x86_64-pc-linux-gnu; the only
change in results was jit.sum's # of PASS results increased by
15, as expected. No jit.sum failures, and no regressions elsewhere in
the testsuites.
I've pushed it to trunk as r14-7117-g8415bceea9d3ca.
Dave
>
> >
> > Dave
> >
> >
> >
> > >
> > > On Fri, 2023-12-01 at 12:49 -0500, David Malcolm wrote:
> > > > On Thu, 2023-11-30 at 17:13 -0500, Antoni Boucher wrote:
> > > > > Here's the updated patch.
> > > > > The failure was due to the test being in the test array while
> > > > > it
> > > > > should
> > > > > not have been there since it changes the context.
> > > >
> > > > Thanks for the updated patch.
> > > >
> > > > Did you do a full bootstrap and regression test with this one,
> > > > or
> > > > do
> > > > you want me to?
> > > >
> > > > Dave
> > > >
> > >
> >
>
From f26d0f37e8d83bce1f5aa53c393961a8bd518d16 Mon Sep 17 00:00:00 2001
From: Antoni Boucher <bouanto@zoho.com>
Date: Fri, 10 Nov 2023 09:52:32 -0500
Subject: [PATCH] libgccjit: Fix GGC segfault when using -flto
gcc/ChangeLog:
PR jit/111396
* ipa-fnsummary.cc (ipa_fnsummary_cc_finalize): Call
ipa_free_size_summary.
* ipa-icf.cc (ipa_icf_cc_finalize): New function.
* ipa-profile.cc (ipa_profile_cc_finalize): New function.
* ipa-prop.cc (ipa_prop_cc_finalize): New function.
* ipa-prop.h (ipa_prop_cc_finalize): New function.
* ipa-sra.cc (ipa_sra_cc_finalize): New function.
* ipa-utils.h (ipa_profile_cc_finalize, ipa_icf_cc_finalize,
ipa_sra_cc_finalize): New functions.
* toplev.cc (toplev::finalize): Call ipa_icf_cc_finalize,
ipa_prop_cc_finalize, ipa_profile_cc_finalize and
ipa_sra_cc_finalize
Include ipa-utils.h.
gcc/testsuite/ChangeLog:
PR jit/111396
* jit.dg/all-non-failing-tests.h: Add new test-ggc-bugfix.
* jit.dg/test-ggc-bugfix.c: New test.
---
gcc/ipa-fnsummary.cc | 1 +
gcc/ipa-icf.cc | 9 ++++++
gcc/ipa-profile.cc | 10 ++++++
gcc/ipa-prop.cc | 18 +++++++++++
gcc/ipa-prop.h | 2 ++
gcc/ipa-sra.cc | 12 +++++++
gcc/ipa-utils.h | 7 ++++
gcc/testsuite/jit.dg/all-non-failing-tests.h | 12 ++++++-
gcc/testsuite/jit.dg/test-ggc-bugfix.c | 34 ++++++++++++++++++++
gcc/toplev.cc | 5 +++
10 files changed, 109 insertions(+), 1 deletion(-)
create mode 100644 gcc/testsuite/jit.dg/test-ggc-bugfix.c
@@ -5090,4 +5090,5 @@ void
ipa_fnsummary_cc_finalize (void)
{
ipa_free_fn_summary ();
+ ipa_free_size_summary ();
}
@@ -3657,3 +3657,12 @@ make_pass_ipa_icf (gcc::context *ctxt)
{
return new ipa_icf::pass_ipa_icf (ctxt);
}
+
+/* Reset all state within ipa-icf.cc so that we can rerun the compiler
+ within the same process. For use by toplev::finalize. */
+
+void
+ipa_icf_cc_finalize (void)
+{
+ ipa_icf::optimizer = NULL;
+}
@@ -1065,3 +1065,13 @@ make_pass_ipa_profile (gcc::context *ctxt)
{
return new pass_ipa_profile (ctxt);
}
+
+/* Reset all state within ipa-profile.cc so that we can rerun the compiler
+ within the same process. For use by toplev::finalize. */
+
+void
+ipa_profile_cc_finalize (void)
+{
+ delete call_sums;
+ call_sums = NULL;
+}
@@ -5904,5 +5904,23 @@ ipcp_transform_function (struct cgraph_node *node)
return modified_mem_access ? TODO_update_ssa_only_virtuals : 0;
}
+/* Reset all state within ipa-prop.cc so that we can rerun the compiler
+ within the same process. For use by toplev::finalize. */
+
+void
+ipa_prop_cc_finalize (void)
+{
+ if (function_insertion_hook_holder)
+ symtab->remove_cgraph_insertion_hook (function_insertion_hook_holder);
+ function_insertion_hook_holder = NULL;
+
+ if (ipa_edge_args_sum)
+ ggc_delete (ipa_edge_args_sum);
+ ipa_edge_args_sum = NULL;
+
+ if (ipa_node_params_sum)
+ ggc_delete (ipa_node_params_sum);
+ ipa_node_params_sum = NULL;
+}
#include "gt-ipa-prop.h"
@@ -1255,6 +1255,8 @@ tree ipcp_get_aggregate_const (struct function *func, tree parm, bool by_ref,
bool unadjusted_ptr_and_unit_offset (tree op, tree *ret,
poly_int64 *offset_ret);
+void ipa_prop_cc_finalize (void);
+
/* From tree-sra.cc: */
tree build_ref_for_offset (location_t, tree, poly_int64, bool, tree,
gimple_stmt_iterator *, bool);
@@ -4707,5 +4707,17 @@ make_pass_ipa_sra (gcc::context *ctxt)
return new pass_ipa_sra (ctxt);
}
+/* Reset all state within ipa-sra.cc so that we can rerun the compiler
+ within the same process. For use by toplev::finalize. */
+
+void
+ipa_sra_cc_finalize (void)
+{
+ if (func_sums)
+ ggc_delete (func_sums);
+ func_sums = NULL;
+ delete call_sums;
+ call_sums = NULL;
+}
#include "gt-ipa-sra.h"
@@ -57,6 +57,13 @@ bool ipa_make_function_pure (cgraph_node *, bool, bool);
/* In ipa-profile.cc */
bool ipa_propagate_frequency (struct cgraph_node *node);
+void ipa_profile_cc_finalize (void);
+
+/* In ipa-icf.cc */
+void ipa_icf_cc_finalize (void);
+
+/* In ipa-sra.cc */
+void ipa_sra_cc_finalize (void);
/* In ipa-devirt.cc */
@@ -377,6 +377,13 @@
#undef create_code
#undef verify_code
+/* test-ggc-bugfix.c */
+#define create_code create_code_ggc_bugfix
+#define verify_code verify_code_ggc_bugfix
+#include "test-ggc-bugfix.c"
+#undef create_code
+#undef verify_code
+
/* Now expose the individual testcases as instances of this struct. */
struct testcase
@@ -529,7 +536,10 @@ const struct testcase testcases[] = {
verify_code_version},
{"volatile",
create_code_volatile,
- verify_code_volatile}
+ verify_code_volatile},
+ {"ggc_bugfix",
+ create_code_ggc_bugfix,
+ verify_code_ggc_bugfix},
};
const int num_testcases = (sizeof (testcases) / sizeof (testcases[0]));
new file mode 100644
@@ -0,0 +1,34 @@
+/* { dg-do compile { target x86_64-*-* } } */
+
+#include <stdlib.h>
+#include <stdio.h>
+
+#include "libgccjit.h"
+
+#include "harness.h"
+
+void
+create_code (gcc_jit_context *ctxt, void *user_data)
+{
+ gcc_jit_context_add_command_line_option (ctxt, "-flto");
+ gcc_jit_context_add_driver_option (ctxt, "-nostdlib");
+
+ gcc_jit_type *type_int = gcc_jit_context_get_type (ctxt, GCC_JIT_TYPE_INT);
+ gcc_jit_param *params_for_func_main[0] = {
+ };
+ gcc_jit_function *func_main =
+ gcc_jit_context_new_function (ctxt, NULL, GCC_JIT_FUNCTION_EXPORTED,
+ type_int, "main", 0, params_for_func_main,
+ 0);
+ gcc_jit_block *block_start =
+ gcc_jit_function_new_block (func_main, "start");
+ gcc_jit_rvalue *rvalue__int_42 =
+ gcc_jit_context_new_rvalue_from_int (ctxt, type_int, 42);
+ gcc_jit_block_end_with_return (block_start, NULL, rvalue__int_42);
+}
+
+void
+verify_code (gcc_jit_context *ctxt, gcc_jit_result *result)
+{
+ CHECK_NON_NULL (result);
+}
@@ -75,6 +75,7 @@ along with GCC; see the file COPYING3. If not see
#include "symbol-summary.h"
#include "tree-vrp.h"
#include "ipa-prop.h"
+#include "ipa-utils.h"
#include "gcse.h"
#include "omp-offload.h"
#include "edit-context.h"
@@ -2337,7 +2338,11 @@ toplev::finalize (void)
ipa_fnsummary_cc_finalize ();
ipa_modref_cc_finalize ();
ipa_edge_modifications_finalize ();
+ ipa_icf_cc_finalize ();
+ ipa_prop_cc_finalize ();
+ ipa_profile_cc_finalize ();
+ ipa_sra_cc_finalize ();
cgraph_cc_finalize ();
cgraphunit_cc_finalize ();
symtab_thunks_cc_finalize ();
--
2.42.0