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 31B10386F467 for ; Thu, 18 Feb 2021 14:20:51 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 31B10386F467 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 446D0AFF7; Thu, 18 Feb 2021 14:20:50 +0000 (UTC) Subject: Re: [PATCH] Fix CK_BAD propagation for --odr From: Tom de Vries To: dwz@sourceware.org, jakub@redhat.com, mark@klomp.org References: <20210216091146.GA32276@delia> Message-ID: <54d0e486-a712-13be-1f72-d5df15d1152b@suse.de> Date: Thu, 18 Feb 2021 15:20:49 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.7.0 MIME-Version: 1.0 In-Reply-To: <20210216091146.GA32276@delia> Content-Type: multipart/mixed; boundary="------------067C8DB144FEFA0FB5B6809C" Content-Language: en-US X-Spam-Status: No, score=-12.3 required=5.0 tests=BAYES_00, GIT_PATCH_0, KAM_DMARC_STATUS, NICE_REPLY_A, 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: Thu, 18 Feb 2021 14:20:53 -0000 This is a multi-part message in MIME format. --------------067C8DB144FEFA0FB5B6809C Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit On 2/16/21 10:11 AM, Tom de Vries wrote: > The problem is caused by the odr code in checksum_ref_die, which skips > checksum calculation for the children of odr types, with as unintended > side-effect that it break the CK_BAD propagation to toplevel DIEs. > > Fix this by making the skipping of the checksum calculation less intrusive. > > Also, add the assert in partition_found_dups to catch this earlier and > easier. > > Unfortunately, the fix also means that the earlier reported performance > improvement of odr on cc1, of 42.30%: > ... > $ ./dwz.master cc1 -o 1 -lnone > $ ./diff.sh cc1 1 > .debug_info red: 44.84% 111527248 61527733 > .debug_abbrev red: 40.28% 1722726 1028968 > .debug_str red: 0% 6609355 6609355 > total red: 42.30% 119859329 69166056 > ... > to 54.55%: > ... > $ ./dwz.master cc1 -o 2 -lnone --odr > $ ./diff.sh cc1 2 > .debug_info red: 57.46% 111527248 47449258 > .debug_abbrev red: 75.08% 1722726 429434 > .debug_str red: 0% 6609355 6609355 > total red: 54.55% 119859329 54488047 > ... > pretty much disappears: > ... > $ ./dwz.fix cc1 -o 3 -lnone --odr > $ ./diff.sh cc1 3 > .debug_info red: 45.44% 111527248 60858640 > .debug_abbrev red: 42.88% 1722726 984084 > .debug_str red: 0% 6609355 6609355 > total red: 42.89% 119859329 68452079 > ... > I managed to come up with a fix that doesn't suffer from this performance regression. Any comments? Thanks, - Tom --------------067C8DB144FEFA0FB5B6809C Content-Type: text/x-patch; charset=UTF-8; name="0001-Fix-CK_BAD-propagation-for-odr.patch" Content-Transfer-Encoding: 7bit Content-Disposition: inline; filename="0001-Fix-CK_BAD-propagation-for-odr.patch" Fix CK_BAD propagation for --odr With the reproducer from PR26252 we get: ... $ dwz 2 -o 2.z --odr -lnone dwz: dwz.c:7396: partition_found_dups: \ Assertion `die->die_ck_state == CK_KNOWN' failed. Aborted (core dumped) ... The problem is caused by the odr code in checksum_ref_die, which skips checksum calculation for the children of odr types, with as unintended side-effect that it break the CK_BAD propagation to toplevel DIEs. Fix this by making the skipping of the checksum calculation less intrusive. Specially: - undo all modifications related to odr in checksum_ref_die - After calling checksum_ref_die in read_debug_info: - override die_ref_hash for odr DIEs, and - recalculate die_ref_hash for all other DIEs. We still have the same amount of compression with cc1, that is: without odr we have 42.30% reduction: ... $ dwz cc1 -o cc1.z -lnone --no-odr $ diff.sh cc1 cc1.z .debug_info red: 44.84% 111527248 61527733 .debug_abbrev red: 40.28% 1722726 1028968 .debug_str red: 0% 6609355 6609355 total red: 42.30% 119859329 69166056 ... and with odr but without the bug fix we have 54.55%: ... $ dwz cc1 -o cc1.z -lnone --odr $ diff.sh cc1 cc1.z .debug_info red: 57.46% 111527248 47449258 .debug_abbrev red: 75.08% 1722726 429434 .debug_str red: 0% 6609355 6609355 total red: 54.55% 119859329 54488047 ... and with odr and the bug fix still 54.55%: ... $ dwz cc1 -o cc1.z -lnone --odr $ ../measure/diff.sh cc1 cc1.z .debug_info red: 57.46% 111527248 47446501 .debug_abbrev red: 75.51% 1722726 422027 .debug_str red: 0% 6609355 6609355 total red: 54.55% 119859329 54477883 ... 2021-02-18 Tom de Vries PR dwz/26252 * dwz.c (checksum_ref_die): Undo modifications related to odr. (read_debug_info): After calling checksum_ref_die, override die_ref_hash for odr DIEs, and recalculate die_ref_hash for all other DIEs. --- dwz.c | 89 ++++++++++++++++++++++++++++++++++++++++++++++++++++--------------- 1 file changed, 69 insertions(+), 20 deletions(-) diff --git a/dwz.c b/dwz.c index 0b751fc..62ebe97 100644 --- a/dwz.c +++ b/dwz.c @@ -3994,7 +3994,6 @@ checksum_ref_die (dw_cu_ref cu, dw_die_ref top_die, dw_die_ref die, unsigned int i, ret = 0; unsigned char *ptr; dw_die_ref child; - bool only_hash_name_p; if (top_die == die) { @@ -4025,7 +4024,6 @@ checksum_ref_die (dw_cu_ref cu, dw_die_ref top_die, dw_die_ref die, else assert (top_die == NULL || die->die_ck_state == CK_KNOWN); t = die->die_abbrev; - only_hash_name_p = odr && die_odr_state (die_cu (die), die) != ODR_NONE; for (i = 0; i < t->nattr; ++i) if (t->attr[i].attr != DW_AT_sibling) switch (t->attr[i].form) @@ -4209,24 +4207,25 @@ checksum_ref_die (dw_cu_ref cu, dw_die_ref top_die, dw_die_ref die, } } - if (!only_hash_name_p - && (top_die == NULL || top_die->die_ck_state != CK_BAD)) - for (child = die->die_child; child; child = child->die_sib) - { - unsigned int r - = checksum_ref_die (cu, - top_die ? top_die - : child->die_named_namespace - ? NULL : child, child, - second_idx, second_hash); - if (top_die == NULL) - assert (r == 0 && obstack_object_size (&ob) == 0); - - if (ret == 0 || (r && r < ret)) - ret = r; - if (top_die && top_die->die_ck_state == CK_BAD) - break; - } + if (top_die == NULL || top_die->die_ck_state != CK_BAD) + { + for (child = die->die_child; child; child = child->die_sib) + { + unsigned int r + = checksum_ref_die (cu, + top_die ? top_die + : child->die_named_namespace + ? NULL : child, child, + second_idx, second_hash); + if (top_die == NULL) + assert (r == 0 && obstack_object_size (&ob) == 0); + + if (ret == 0 || (r && r < ret)) + ret = r; + if (top_die && top_die->die_ck_state == CK_BAD) + break; + } + } if (top_die == die) { @@ -6742,6 +6741,24 @@ read_debug_info (DSO *dso, int kind, unsigned int *die_count) for (cu = cuf; cu; cu = cu->cu_next) checksum_ref_die (cu, NULL, cu->cu_die, NULL, NULL); + if (odr) + { + for (cu = cuf; cu; cu = cu->cu_next) + { + dw_die_ref die; + FOREACH_LOW_TOPLEVEL_DIE_IN_CU (die, cu) + { + if (die->die_ck_state != CK_KNOWN) + continue; + if (die_odr_state (die_cu (die), die) != ODR_NONE) + die->u.p1.die_ref_hash = die->u.p1.die_hash; + else + die->die_ref_hash_computed = 0; + } + } + for (cu = cuf; cu; cu = cu->cu_next) + checksum_ref_die (cu, NULL, cu->cu_die, NULL, NULL); + } if (dump_dies_p) for (cu = cuf; cu; cu = cu->cu_next) @@ -7220,6 +7237,20 @@ read_debug_info (DSO *dso, int kind, unsigned int *die_count) if (checksum_die (dso, cu, NULL, cu->cu_die)) goto fail; checksum_ref_die (cu, NULL, cu->cu_die, NULL, NULL); + if (odr) + { + dw_die_ref die; + FOREACH_LOW_TOPLEVEL_DIE_IN_CU (die, cu) + { + if (die->die_ck_state != CK_KNOWN) + continue; + if (die_odr_state (die_cu (die), die) != ODR_NONE) + die->u.p1.die_ref_hash = die->u.p1.die_hash; + else + die->die_ref_hash_computed = 0; + } + checksum_ref_die (cu, NULL, cu->cu_die, NULL, NULL); + } if (dump_dies_p) dump_dies (0, cu->cu_die); @@ -7292,6 +7323,24 @@ read_debug_info (DSO *dso, int kind, unsigned int *die_count) for (cu = first_cu; cu; cu = cu->cu_next) checksum_ref_die (cu, NULL, cu->cu_die, NULL, NULL); + if (odr) + { + for (cu = first_cu; cu; cu = cu->cu_next) + { + dw_die_ref die; + FOREACH_LOW_TOPLEVEL_DIE_IN_CU (die, cu) + { + if (die->die_ck_state != CK_KNOWN) + continue; + if (die_odr_state (die_cu (die), die) != ODR_NONE) + die->u.p1.die_ref_hash = die->u.p1.die_hash; + else + die->die_ref_hash_computed = 0; + } + } + for (cu = first_cu; cu; cu = cu->cu_next) + checksum_ref_die (cu, NULL, cu->cu_die, NULL, NULL); + } if (dump_dies_p) for (cu = first_cu; cu; cu = cu->cu_next) --------------067C8DB144FEFA0FB5B6809C--