From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-lf1-x131.google.com (mail-lf1-x131.google.com [IPv6:2a00:1450:4864:20::131]) by sourceware.org (Postfix) with ESMTPS id 17CDD3858032; Tue, 12 Sep 2023 09:37:53 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 17CDD3858032 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=gmail.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=gmail.com Received: by mail-lf1-x131.google.com with SMTP id 2adb3069b0e04-5029e4bfa22so6412890e87.3; Tue, 12 Sep 2023 02:37:53 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20221208; t=1694511471; x=1695116271; darn=gcc.gnu.org; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:from:to:cc:subject:date :message-id:reply-to; bh=+CI+HJmtNwJ9wbsPrsLdOSmNvHLpOHCil8R8wlh/YJc=; b=TmB29K+ZCJ3gtlTYHz/F0RkDpiWP9iDUvpYU2M01QemkseQ0mmEnaxIVayZz/6Y9AZ dUII++thR44j5s3/jqm5eeqteMcfgjX+c+b/09iiE9xgGvR61Wvd89BDRAjCI63IWI7K B1/7xUtzRSS4ZalpKVmmZjg0UC53Zy6WMEa/2a1oqLMchd8DXRnRE1ns4XvqkqijOA85 d8WuiGwO4e4gEWJ7Q8pQdUn8o2QoVZTbmlKJ7kc+ScwcbeXT5fvpLG0lAPYEO8QzR0+j 3yuWezV6MJixwKezeFncwMpm0MRHmbQPguRRN97WCE7pvkfat4hpEvuaPLDg1uS660hc S5cg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1694511471; x=1695116271; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=+CI+HJmtNwJ9wbsPrsLdOSmNvHLpOHCil8R8wlh/YJc=; b=JnYRrRMpFUxhkCuUe58wzc/B6W1jnZdM9j+LsSBBH+HGFNZz+Lz72l2eWSlbJNdoys pphDun/38B3GRU4YOfuoNQaA1nzYLSPUduuQYqwXPNRE/xCiy04fe3pZx65Ge0FBQyk/ NpvgPIcLIhuafbHqXcbzG1lvvZJyOm3x1dpdsMYmL51DGQGFdPc7ZZdmofuwDlWxnL3M jPYqmjYM8Chd/v9NYZKzbUMy39ka9j9XxOIJgc5uZdKF1QMUMe0YjnY1u9rb1hVv2rAT QAOves4+HKIGThUHzcYFxlRziMa7XIzc+WH6WBiilfcupjrmU4/LgPZ7Lu8GFohh9Xzb aTeg== X-Gm-Message-State: AOJu0YyTaEm1B2TiClgVV6VtbDap4Xpdty6w9bTp/5eNg8hAHFqjgC1T C/Kr6GMDCm4jsRzv4iUSARdi8LQs23jQSIWhNeg= X-Google-Smtp-Source: AGHT+IGg/kx/zV5U7n3PvZCUVAfcAKdsRy1V/a6aZy8fZxe4ERZ+aR53CEXM//Qftv++3qD3cW3UqxvKQCDnHDkRHDo= X-Received: by 2002:a05:6512:3a87:b0:4fd:fc36:68a2 with SMTP id q7-20020a0565123a8700b004fdfc3668a2mr11984157lfu.1.1694511471083; Tue, 12 Sep 2023 02:37:51 -0700 (PDT) MIME-Version: 1.0 References: <20230906134001.681629-1-dmalcolm@redhat.com> In-Reply-To: <20230906134001.681629-1-dmalcolm@redhat.com> From: Richard Biener Date: Tue, 12 Sep 2023 11:35:49 +0200 Message-ID: Subject: Re: [PATCH] ggc, jit: forcibly clear GTY roots in jit To: David Malcolm Cc: gcc-patches@gcc.gnu.org, jit@gcc.gnu.org, antoyo@gcc.gnu.org Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Spam-Status: No, score=-6.2 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FREEMAIL_FROM,GIT_PATCH_0,RCVD_IN_DNSWL_NONE,SPF_HELO_NONE,SPF_PASS,THIS_AD,TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: On Wed, Sep 6, 2023 at 3:41=E2=80=AFPM David Malcolm via Gcc-patches 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 !=3D 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 =3D NULL; > + add_cfi_vec =3D NULL; > + cur_trace =3D NULL; > + cur_row =3D NULL; > + cur_cfa =3D 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 =3D gt_ggc_deletable_rtab; *rt; rt++) > for (rti =3D *rt; rti->base !=3D NULL; rti++) > - memset (rti->base, 0, rti->stride); > + memset (rti->base, 0, rti->stride * rti->nelt); > > for (rt =3D 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 =3D gt_ggc_deletable_rtab; *rt; rt++) > + for (rti =3D *rt; rti->base !=3D NULL; rti++) > + memset (rti->base, 0, rti->stride * rti->nelt); > + > + for (rt =3D gt_ggc_rtab; *rt; rt++) > + for (rti =3D *rt; rti->base !=3D NULL; rti++) > + memset (rti->base, 0, rti->stride * rti->nelt); > + > + for (rt =3D gt_pch_scalar_rtab; *rt; rt++) > + for (rti =3D *rt; rti->base !=3D 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 =3D NULL; > save_decoded_options_count =3D 0; > > + ggc_common_finalize (); > + > /* Clean up the context (and pass_manager etc). */ > delete g; > g =3D NULL; > -- > 2.26.3 >