From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-io1-xd33.google.com (mail-io1-xd33.google.com [IPv6:2607:f8b0:4864:20::d33]) by sourceware.org (Postfix) with ESMTPS id 163E93857C4E for ; Wed, 9 Mar 2022 10:42:22 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 163E93857C4E Received: by mail-io1-xd33.google.com with SMTP id c23so2230947ioi.4 for ; Wed, 09 Mar 2022 02:42:22 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc:content-transfer-encoding; bh=qNHpqKj4p2oRc3X4KG00mtV3O+FTLglodoyYKsUpdjI=; b=TdXjH5CHhba9WGYV5a+pAHzqnUXahmIrLq/Ey+7q2hWWxETvhXMfROajs3fOFraxXa v46KYaZnKZCu1FVWWuhBv8Y+Sk6Ecq76cs5aLgX/tXB/AxP8PqxEJ5S6XnmNP0YbkvT2 rfAGdttymY7PhnSC+KFnlUVQ+UQPjZkUL921BJ4AJsVzcd+ly3af4ft8bDT9nrVxPlVh PXQHqH6k6kDRJrCqOgLkhTftQGozfAuP4K8n6l7cDjWe1kNWfeY94G194pHXh08iSNS4 p0gaq2mcSKOBzCmV8JYgKtTuVO87i+9aWx2XxAu0FvYd/p56HtsIvae4pk1jYE/JyU9i AkBg== X-Gm-Message-State: AOAM531ZF3fXmtRGKz3O3nKmyqFKVpkBDliheh2KUdeHTVEjuCqCO9Cp kEONYuHUc9AH36XzIskEA3fb0fyIg3+g2ezFfH9LCQ== X-Google-Smtp-Source: ABdhPJzcrGvpfEXjUTLwWA5ssDWd3adis2fZUF+CEPPkOEqx4sM23n9cfeKAOYGo76MwrFO7aSriCqycXWhkaHXwQ8s= X-Received: by 2002:a02:2a8d:0:b0:314:4128:f724 with SMTP id w135-20020a022a8d000000b003144128f724mr18691250jaw.92.1646822541114; Wed, 09 Mar 2022 02:42:21 -0800 (PST) MIME-Version: 1.0 References: <20220303145619.385117-1-gprocida@google.com> <87r17cgx3t.fsf@seketeli.org> In-Reply-To: <87r17cgx3t.fsf@seketeli.org> From: Giuliano Procida Date: Wed, 9 Mar 2022 10:41:44 +0000 Message-ID: Subject: Re: [PATCH] Increase stability of child diff order To: Dodji Seketeli Cc: libabigail@sourceware.org, kernel-team@android.com, maennich@google.com Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Spam-Status: No, score=-21.3 required=5.0 tests=BAYES_00, DKIMWL_WL_MED, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, ENV_AND_HDR_SPF_MATCH, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP, T_SCC_BODY_TEXT_LINE, USER_IN_DEF_DKIM_WL, USER_IN_DEF_SPF_WL autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org X-BeenThere: libabigail@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Mailing list of the Libabigail project List-Unsubscribe: , List-Archive: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 09 Mar 2022 10:42:23 -0000 Hi. On Tue, 8 Mar 2022 at 17:42, Dodji Seketeli wrote: > > Hello Giuliano, > > Giuliano Procida a =C3=A9crit: > > [...] > > I am putting below a copy of the error reported on the bug, that is > allegedly triggered by a different implementation of std::sort: > > --- tests/data/test-abidiff-exit/test-member-size-report0.txt > 2022-01-30 18:02:19.412941913 -0800 > +++ tests/output/test-abidiff-exit/test-member-size-report0.txt > 2022-01-30 18:02:20.180941537 -0800 > @@ -4,16 +4,15 @@ > 2 functions with some indirect sub-type change: > > [C] 'function void reg1(S*, T*, T*)' at test-member-size-v1.cc:26:= 1 > has some indirect sub-type changes: > - parameter 1 of type 'S*' has sub-type changes: > - in pointed to type 'struct S' at test-member-size-v1.cc:3:1= : > - type size changed from 128 to 192 (in bits) > - 1 data member insertion: > - 'int y', at offset 128 (in bits) at > test-member-size-v1.cc:6:1 > - no data member change (1 filtered); > - parameter 2 of type 'T*' has sub-type changes: > + parameter 3 of type 'T*' has sub-type changes: > in pointed to type 'struct T' at test-member-size-v1.cc:14:= 1: > type size changed from 192 to 256 (in bits) > - 1 data member changes (1 filtered): > + 2 data member changes: > + type of 'S s' changed: > + type size changed from 128 to 192 (in > bits) > + 1 data member insertion: > + 'int y', at offset 128 (in bits) at > test-member-size-v1.cc:6:1 > + no data member change (1 filtered); > 'int a' offset changed from 128 to 192 (in bits) (by +6= 4 bits) > > From what I am seeing here, it looks like this is because the order in > which the diff nodes representing the function parameter diffs are > present as children of the diff node for the function type diff. > > Said otherwise, the diff node for the function type diff has children > nodes that are the diff nodes representing function parameter diffs. > > You can see that in the function > function_type_diff::chain_into_hierarchy in abg-comparison.cc: > > for (vector::const_iterator i =3D > priv_->sorted_changed_parms_by_id_.begin(); > i !=3D priv_->sorted_changed_parms_by_id_.end(); > ++i) > if (diff_sptr d =3D *i) > append_child_node(d); > > So, the parameters diffs are sorted by parameter id; meaning, the > parameter diff for the first function parameter comes before the > parameter diff for the second function parameter, which comes before the > parameter diff for the third function parameter, etc. > > But then in the problem report, it seems that the order of those > parameter diff nodes is changed somehow, because the diff node for the > third parameter comes first. > > Let's look at the code of diff::append_child_node that is called in the > code snippet above, in the expression "append_child_node(d)". It > contains this snippet: > > > @@ -1999,9 +1999,9 @@ diff::append_child_node(diff_sptr d) > > priv_->children_.push_back(d.get()); > > > > diff_less_than_functor comp; > > std::sort(priv_->children_.begin(), > > priv_->children_.end(), > > comp); > > I think that std::sort should not be there, because the children nodes > are already sorted, using a sorting criterion that is specific for > function parameter diffs. So that would be the bug. I think that call > to std::sort should be purely removed. > > I am thus proposing the patch below. Would it address your issue? > [snip] Absolutely! There is no chance of sort causing stability issues if it's not used at all. More importantly, it's a real bug fix. Thank you, Giuliano.