From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx2.suse.de (mx2.suse.de [195.135.220.15]) by sourceware.org (Postfix) with ESMTPS id 9F8983846454 for ; Tue, 23 Feb 2021 10:00:25 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 9F8983846454 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=suse.de Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=tdevries@suse.de X-Virus-Scanned: by amavisd-new at test-mx.suse.de Received: from relay2.suse.de (unknown [195.135.221.27]) by mx2.suse.de (Postfix) with ESMTP id AB0B3AFB4; Tue, 23 Feb 2021 10:00:24 +0000 (UTC) Subject: [PATCH] Clean up die_odr_state interface To: Mark Wielaard Cc: dwz@sourceware.org, jakub@redhat.com References: <20210222153902.GA412@delia.home> <20210222222749.GF2992@wildebeest.org> From: Tom de Vries Message-ID: Date: Tue, 23 Feb 2021 11:00:24 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.7.1 MIME-Version: 1.0 In-Reply-To: <20210222222749.GF2992@wildebeest.org> Content-Type: multipart/mixed; boundary="------------A874B789B3F03ED617740D8C" Content-Language: en-US X-Spam-Status: No, score=-12.3 required=5.0 tests=BAYES_00, GIT_PATCH_0, KAM_DMARC_STATUS, RCVD_IN_MSPIKE_H3, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: dwz@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Dwz mailing list List-Unsubscribe: , List-Archive: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 23 Feb 2021 10:00:27 -0000 This is a multi-part message in MIME format. --------------A874B789B3F03ED617740D8C Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit [ was: Re: [committed] Don't call die_odr_state with unnecessarily defined cu arg ] On 2/22/21 11:27 PM, Mark Wielaard wrote: > Hi Tom, > > On Mon, Feb 22, 2021 at 04:39:03PM +0100, Tom de Vries wrote: >> Hi, >> >> When compiling dwz with this patch: >> ... >> die_odr_state (dw_cu_ref cu, dw_die_ref die) >> { >> if (die->die_odr_state != ODR_UNKNOWN) >> - return die->die_odr_state; >> + { >> + assert (cu == NULL); >> + return die->die_odr_state; >> + } >> ... >> and running f.i. odr-struct.sh, we run into the abort. >> >> The recent commit 3312feb "Fix CK_BAD propagation for --odr" introduced this >> code: >> ... >> if (die_odr_state (die_cu (die), die) != ODR_NONE) >> die->u.p1.die_ref_hash = die->u.p1.die_hash; >> ... >> and there's no need to pass a CU argument, which makes the abort trigger. >> >> Fix this by passing a NULL CU instead. >> >> Committed to trunk. >> >> Thanks, >> - Tom >> >> Don't call die_odr_state with unnecessarily defined cu arg >> >> 2021-02-22 Tom de Vries >> >> * dwz.c (read_debug_info): Pass NULL CU to die_odr_state call. >> >> --- >> dwz.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/dwz.c b/dwz.c >> index 076f39c..86863ce 100644 >> --- a/dwz.c >> +++ b/dwz.c >> @@ -7253,7 +7253,7 @@ read_debug_info (DSO *dso, int kind, unsigned int *die_count) >> { >> if (die->die_ck_state != CK_KNOWN) >> continue; >> - if (die_odr_state (die_cu (die), die) != ODR_NONE) >> + if (die_odr_state (NULL, die) != ODR_NONE) >> die->u.p1.die_ref_hash = die->u.p1.die_hash; >> else >> die->die_ref_hash_computed = 0; > > I might be a little lost in the odr code. Hi, Yeah, I can imagine. Anyway, review much appreciated. > It seems there is only one > call to die_odr_state that passes a non-NULL cu (the call in > checksum_die). > Ack, good observation. > What does passing a NULL cu signify in this code? If cu is NULL it > seems the code that calls set_die_odr_state at the end of > die_ord_state is unsafe (because it might check cu). > Correct, so the NULL argument should only be used when set_die_odr_state isn't called. > Just wondering if when passing NULL in as cu the result isn't simply > always die->die_odr_state? It is. > (And if so, do we even need the function > call?) I've left it in for now, for the sake of the assert. Instead, I've dropped the cu parameter for the function. WDYT? Thanks, - Tom --------------A874B789B3F03ED617740D8C Content-Type: text/x-patch; charset=UTF-8; name="0001-Clean-up-die_odr_state-interface.patch" Content-Transfer-Encoding: quoted-printable Content-Disposition: inline; filename="0001-Clean-up-die_odr_state-interface.patch" Clean up die_odr_state interface The die_odr_state function returns the odr state for a die, and if it has= n't been calculated, it calculates it first using a call to set_die_odr_state= : =2E.. static unsigned int UNUSED die_odr_state (dw_cu_ref cu, dw_die_ref die) { if (die->die_odr_state !=3D ODR_UNKNOWN) return die->die_odr_state; set_die_odr_state (cu, die); return die->die_odr_state; } =2E.. The call to set_die_odr_state needs a cu argument, and consequently die_odr_state also has one. That means a call to die_odr_state needs a p= roper CU argument if set_die_odr_state gets called, and for optimality a NULL C= U otherwise. As it happens, there's only one place where the proper CU argument is required, and it's easy to accidentally do: =2E.. die_odr_state (die_cu (die), die) =2E.. where a NULL CU would suffice. Fix this by explicitly calling set_die_odr_state and dropping the cu para= meter from die_odr_state. 2021-02-23 Tom de Vries * dwz.c (set_die_odr_state): Assert die->die_odr_state =3D=3D ODR_UNKNOW= N. (die_odr_state): Drop cu parameter. Assert die->die_odr_state !=3D ODR_UNKNOWN. (checksum_die): Call set_die_odr_state. Update call to die_odr_state. (read_debug_info, split_dups, reorder_dups, merged_singleton) (partition_dups): Update call to die_odr_state. --- dwz.c | 37 +++++++++++++++++++------------------ 1 file changed, 19 insertions(+), 18 deletions(-) diff --git a/dwz.c b/dwz.c index 6c516cf..9fb11b9 100644 --- a/dwz.c +++ b/dwz.c @@ -3227,6 +3227,7 @@ set_die_odr_state (dw_cu_ref cu, dw_die_ref die) bool name_p; bool other_p; =20 + assert (die->die_odr_state =3D=3D ODR_UNKNOWN); die->die_odr_state =3D ODR_NONE; =20 if (low_mem) @@ -3310,12 +3311,9 @@ set_die_odr_state (dw_cu_ref cu, dw_die_ref die) =20 /* Return the initialized die_odr_state field for DIE with CU. */ static unsigned int UNUSED -die_odr_state (dw_cu_ref cu, dw_die_ref die) +die_odr_state (dw_die_ref die) { - if (die->die_odr_state !=3D ODR_UNKNOWN) - return die->die_odr_state; - - set_die_odr_state (cu, die); + assert (die->die_odr_state !=3D ODR_UNKNOWN); return die->die_odr_state; } =20 @@ -3380,7 +3378,10 @@ checksum_die (DSO *dso, dw_cu_ref cu, dw_die_ref t= op_die, dw_die_ref die) fprintf (stderr, "DIE %x, hash: %x, lang\n", die->die_offset, die->u.p1.die_hash); } - only_hash_name_p =3D odr && die_odr_state (die_cu (die), die) !=3D ODR= _NONE; + + if (odr && die->die_odr_state =3D=3D ODR_UNKNOWN) + set_die_odr_state (die_cu (die), die); + only_hash_name_p =3D odr && die_odr_state (die) !=3D ODR_NONE; die_hash2 =3D 0; if (only_hash_name_p) die_hash2 =3D die->u.p1.die_hash; @@ -7254,7 +7255,7 @@ read_debug_info (DSO *dso, int kind, unsigned int *= die_count) { if (die->die_ck_state !=3D CK_KNOWN) continue; - if (die_odr_state (NULL, die) !=3D ODR_NONE) + if (die_odr_state (die) !=3D ODR_NONE) die->u.p1.die_ref_hash =3D die->u.p1.die_hash; else die->die_ref_hash_computed =3D 0; @@ -7656,7 +7657,7 @@ split_dups (dw_die_ref die, struct obstack *vec) for (i =3D 0; i < count; i++) { d =3D arr[i]; - if (die_odr_state (NULL, d) !=3D ODR_DECL) + if (die_odr_state (d) !=3D ODR_DECL) continue; if (!head) head =3D d; @@ -7675,14 +7676,14 @@ split_dups (dw_die_ref die, struct obstack *vec) { d =3D arr[i]; if (d->die_dup || d->die_nextdup - || die_odr_state (NULL, d) =3D=3D ODR_DECL) + || die_odr_state (d) =3D=3D ODR_DECL) continue; tail =3D d; for (j =3D i + 1; j < count; j++) { d2 =3D arr[j]; if (d2->die_dup || d2->die_nextdup - || die_odr_state (NULL, d2) =3D=3D ODR_DECL) + || die_odr_state (d2) =3D=3D ODR_DECL) continue; die_eq (d, d2); } @@ -7696,7 +7697,7 @@ split_dups (dw_die_ref die, struct obstack *vec) for (i =3D 0; i < count; i++) { d =3D arr[i]; - if (die_odr_state (NULL, d) =3D=3D ODR_DECL + if (die_odr_state (d) =3D=3D ODR_DECL || d->die_dup !=3D NULL) continue; def =3D d; @@ -7774,12 +7775,12 @@ reorder_dups (dw_die_ref die) unsigned def_count =3D 0; dw_die_ref d; - if (die_odr_state (NULL, die) =3D=3D ODR_NONE) + if (die_odr_state (die) =3D=3D ODR_NONE) return die; =20 for (d =3D die; d; d =3D d->die_nextdup) { - if (die_odr_state (NULL, d) =3D=3D ODR_DECL) + if (die_odr_state (d) =3D=3D ODR_DECL) decl_count++; else def_count++; @@ -7787,14 +7788,14 @@ reorder_dups (dw_die_ref die) if (def_count =3D=3D 0 || decl_count =3D=3D 0) return die; =20 - if (die_odr_state (NULL, die) !=3D ODR_DECL) + if (die_odr_state (die) !=3D ODR_DECL) return die; =20 dw_die_ref def =3D NULL; dw_die_ref prev =3D NULL; for (d =3D die; d; prev =3D d, d =3D d->die_nextdup) { - if (die_odr_state (NULL, d) =3D=3D ODR_DECL) + if (die_odr_state (d) =3D=3D ODR_DECL) continue; def =3D d; break; @@ -8253,7 +8254,7 @@ merged_singleton (dw_die_ref die) size_t decl_cnt =3D 0; =20 for (d =3D die; d; d =3D d->die_nextdup) - switch (die_odr_state (NULL, d)) + switch (die_odr_state (d)) { case ODR_DEF: if (res) @@ -8313,7 +8314,7 @@ partition_dups (void) for (i =3D 0; i < vec_size; i++) { dw_die_ref die =3D arr[i]; - if (die_odr_state (NULL, die) =3D=3D ODR_NONE) + if (die_odr_state (die) =3D=3D ODR_NONE) continue; die =3D split_dups (die, &ob2); assert (die !=3D NULL); @@ -8340,7 +8341,7 @@ partition_dups (void) && die->die_nextdup =3D=3D NULL) mark_singletons (die_cu (die), die, die, &ob2); else if (odr_mode !=3D ODR_BASIC - && die_odr_state (NULL, die) !=3D ODR_NONE) + && die_odr_state (die) !=3D ODR_NONE) { dw_die_ref s =3D merged_singleton (die); if (s) --------------A874B789B3F03ED617740D8C--