From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-74.mimecast.com (us-smtp-delivery-74.mimecast.com [63.128.21.74]) by sourceware.org (Postfix) with ESMTP id 58C8B3887575 for ; Mon, 30 Mar 2020 16:06:24 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 58C8B3887575 Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-116-h6k5BfEyNXufmlJr4AKWDA-1; Mon, 30 Mar 2020 12:06:20 -0400 X-MC-Unique: h6k5BfEyNXufmlJr4AKWDA-1 Received: from smtp.corp.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 083FE800EB6; Mon, 30 Mar 2020 16:06:19 +0000 (UTC) Received: from t470.redhat.com (ovpn-112-87.phx2.redhat.com [10.3.112.87]) by smtp.corp.redhat.com (Postfix) with ESMTP id 7514460BE1; Mon, 30 Mar 2020 16:06:15 +0000 (UTC) From: David Malcolm To: Vladimir Makarov Cc: Andrea Corallo , gcc-patches@gcc.gnu.org, jit@gcc.gnu.org, nd , David Malcolm Subject: [PATCH] lra: set insn_code_data to NULL when freeing Date: Mon, 30 Mar 2020 12:06:08 -0400 Message-Id: <20200330160608.10383-1-dmalcolm@redhat.com> MIME-Version: 1.0 X-Scanned-By: MIMEDefang 2.79 on 10.5.11.12 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: quoted-printable X-Spam-Status: No, score=-31.1 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=unavailable autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: jit@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Jit mailing list List-Unsubscribe: , List-Archive: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 30 Mar 2020 16:06:25 -0000 On Sun, 2020-03-29 at 21:31 +0100, Andrea Corallo wrote: > David Malcolm writes: >=20 > > On Wed, 2020-03-18 at 23:51 +0100, Andrea Corallo wrote: > > Please add the new test to the header in its alphabetical location, > > i.e. between: > >=20 > > /* test-vector-types.cc: We don't use this, since it's C++. */ > >=20 > > and > >=20 > > /* test-volatile.c */ > >=20 > > An entry also needs to be added to the "testcases" array at the end > > of > > the header (again, in the alphabetical-sorted location). >=20 > I tried adding the test into the "testcases" array but this makes the > threads test failing. >=20 > I think this has nothing to do with this patch and happen just > because > this test does not define any code. Infact I see the same happening > just adding "test-empty.c" to the "testcases" array on the current > master. >=20 > The error is not very reproducible, I tried a run under valgrind but > have found nothing so far :/ >=20 > Dave do you recall if there was a specific reason not to have > "test-empty.c" into the "testcases" array? >=20 > Andrea It's a double-free bug in lra.c, albeit one that requires being used in a multithreaded way from libgccjit to be triggered. libgccjit's test-threads.c repeatedly compiles and runs numerous tests, each in a separate thread. Attempting to add an empty test that generates no code leads to a double-free ICE within that thread, within lra.c's finish_insn_code_data_once. The root cause is that the insn_code_data array is cleared in init_insn_code_data_once, but this is only called the first time a cgraph_node is expanded [1], whereas the "loop-over-all-elements and free them" is unconditionally called in finalize [2]. Hence if there are no functions: * the array is not re-initialized for the empty context * when finish_insn_code_data_once is called for the empty context it still contains the freed pointers from the previous context that held the jit mutex, and hence the free is a double-free. This patch sets the pointers to NULL after freeing them, fixing the ICE. The calls to free are still guarded by a check for NULL, which is redundant, but maybe there's a reason for not wanting to call "free" on a possibly-NULL value many times on process exit? (it makes the diff cleaner, at least) Fixes the issue in jit.dg. Full bootstrap & regression test in progress. Is it OK for master if it passes? Thanks Dave [1] init_insn_code_data_once is called via lra_init_once called by ira_init_once called by initialize_rtl, via: if (!rtl_initialized) ira_init_once (); called by init_function_start called by cgraph_node::expand [2]: finish_insn_code_data_once is called by: lra_finish_once called by finalize gcc/ChangeLog: =09* lra.c (finish_insn_code_data_once): Set the array elements =09to NULL after freeing them. gcc/testsuite/ChangeLog: =09* jit.dg/all-non-failing-tests.h: Add test-empty.c --- gcc/lra.c | 5 ++++- gcc/testsuite/jit.dg/all-non-failing-tests.h | 10 ++++++++++ 2 files changed, 14 insertions(+), 1 deletion(-) diff --git a/gcc/lra.c b/gcc/lra.c index d5ea3622686..5e8b75b1fda 100644 --- a/gcc/lra.c +++ b/gcc/lra.c @@ -653,7 +653,10 @@ finish_insn_code_data_once (void) for (unsigned int i =3D 0; i < NUM_INSN_CODES; i++) { if (insn_code_data[i] !=3D NULL) -=09free (insn_code_data[i]); +=09{ +=09 free (insn_code_data[i]); +=09 insn_code_data[i] =3D NULL; +=09} } } =20 diff --git a/gcc/testsuite/jit.dg/all-non-failing-tests.h b/gcc/testsuite/j= it.dg/all-non-failing-tests.h index b2acc74ae95..af744192a73 100644 --- a/gcc/testsuite/jit.dg/all-non-failing-tests.h +++ b/gcc/testsuite/jit.dg/all-non-failing-tests.h @@ -116,6 +116,13 @@ #undef create_code #undef verify_code =20 +/* test-empty.c */ +#define create_code create_code_empty +#define verify_code verify_code_empty +#include "test-empty.c" +#undef create_code +#undef verify_code + /* test-error-*.c: We don't use these test cases, since they deliberately introduce errors, which we don't want here. */ =20 @@ -328,6 +335,9 @@ const struct testcase testcases[] =3D { {"expressions", create_code_expressions, verify_code_expressions}, + {"empty", + create_code_empty, + verify_code_empty}, {"factorial", create_code_factorial, verify_code_factorial}, --=20 2.21.0