From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by sourceware.org (Postfix) with ESMTPS id 0D5E03858D1E for ; Thu, 18 Aug 2022 17:52:33 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 0D5E03858D1E Received: from mail-pj1-f70.google.com (mail-pj1-f70.google.com [209.85.216.70]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_128_GCM_SHA256) id us-mta-611-N5YBfa5nOLSUrsa2LGJnaQ-1; Thu, 18 Aug 2022 13:52:31 -0400 X-MC-Unique: N5YBfa5nOLSUrsa2LGJnaQ-1 Received: by mail-pj1-f70.google.com with SMTP id q6-20020a17090a1b0600b001f558bbb924so1343724pjq.3 for ; Thu, 18 Aug 2022 10:52:30 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=to:in-reply-to:cc:references:message-id:date:subject:mime-version :from:content-transfer-encoding:x-gm-message-state:from:to:cc; bh=tDtnMoBgTdxcnRv57nO+XhMZ2I4DWZXyoNQ6uZwwRrg=; b=g9m1LX8pfKTZtM19qWtu/S+iIn/YIDN2A0sYGQxz0LLeIfdEinJq7wyargymM5PNBe 3Vzd6YOeDkq2VA6e5g5aBA6mr9PvilQhtFf/jwZkceDHzbPO1uPWQAEa28KUCuhaUvw7 zYjATQFtn2TonCG00rnzjVhUqutpBcVRRUTLCrfWBE0GReB71h9X73Zq9lXTdRHc9piG aiJpUJL7cD3IeUpXBc/VhtLOGSJ+e1+qCMyHCMrr18dbeaw9sItQxzM1v8XQI/iskZTL ey0NFLvnIYRhSq/c5JNVvjQM4m2bbPV7aMc51OTTXjQ0vbctKMzMzE8vpSjMoFiJvkao S7SQ== X-Gm-Message-State: ACgBeo3N29iQq8bnRjB3GSiBZPanRnYKWOOmA2G7nhbnR5wA5DGI+rJ6 ON8gVXZZm4IIoWiPw8VeoCy5f2ov8AqmpL85kr17GOp1B0gsf361O2RZQeowbCg9c/mx5mz7WB+ +UhYTwRdP3k4X9gRCJgC5 X-Received: by 2002:a05:6a00:cc7:b0:52f:2ada:11c5 with SMTP id b7-20020a056a000cc700b0052f2ada11c5mr3889303pfv.19.1660845149955; Thu, 18 Aug 2022 10:52:29 -0700 (PDT) X-Google-Smtp-Source: AA6agR4oXDdNgh52G3QjHbNwYfmOGg4jxYCCAXk39hUhxLG5QkEcVuvh42NrGrAigOqEbiHVBTji1w== X-Received: by 2002:a05:6a00:cc7:b0:52f:2ada:11c5 with SMTP id b7-20020a056a000cc700b0052f2ada11c5mr3889284pfv.19.1660845149378; Thu, 18 Aug 2022 10:52:29 -0700 (PDT) Received: from smtpclient.apple ([47.208.199.57]) by smtp.gmail.com with ESMTPSA id n16-20020a170902e55000b001709f01c423sm1699733plf.32.2022.08.18.10.52.28 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Thu, 18 Aug 2022 10:52:29 -0700 (PDT) From: Ben Woodard Mime-Version: 1.0 (1.0) Subject: Re: [PATCH 3/3] ir: Consider integral types of same kind and size as equivalent Date: Thu, 18 Aug 2022 10:52:27 -0700 Message-Id: <1022329F-61A4-4E74-B449-D582A929515F@redhat.com> References: <877d35ms6k.fsf@seketeli.org> Cc: Giuliano Procida , libabigail@sourceware.org, =?utf-8?Q?Matthias_M=C3=A4nnich?= In-Reply-To: <877d35ms6k.fsf@seketeli.org> To: Dodji Seketeli X-Mailer: iPad Mail (19G71) X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-Spam-Status: No, score=-5.4 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, RCVD_IN_DNSWL_LOW, SPF_HELO_NONE, SPF_NONE, TXREP, T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) 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: Thu, 18 Aug 2022 17:52:35 -0000 I do not believe applying the fundamental type folding only to self compari= son will meet our needs. The real reason that self comparison is important = to us is we need to throw away the binary and then save the abixml. Then la= ter on we can compare the abixml to a new build to find out if something ha= ppened that broke ABI. Thus for our needs, we will not be in a self compari= son operation at the time when we come across this in a non-testing environ= ment. I believe that the ordering of the TUs that make up the binary could = make this problem appear, thus I do not think it will serve our purposes. I know that C doesn=E2=80=99t have a ODR but I=E2=80=99m fairly certain tha= t code along the lines of what is in btrfs-progs will have portability prob= lems between architectures. Therefore, I think that programs like this shou= ld be fixed. I=E2=80=99m start a discussion with the btrfs guys and explain= the problem. I can imagine a possible reason for this problem in btrfs-pro= gs though, it could be caused by the need to read a FS created on one arch = on a machine of another arch. I ran into that a few times many years ago an= d worked with upstream to fix them. (Ia64 era IIRC) However, the fact that libabigail catches this problem is far too late. At = the very least I=E2=80=99d say the compiler should warn about issues like t= his. We should talk to the compiler guys and see if we can have them create= a warning about this situation. Dodji could you start a discussion with ou= r compiler team on this and I will weigh in on it. As for libabigail, this problem is too subtle for a normal user to understa= nd given the current output and so we need to do something. When reading a = corpus of a binary when you come across a case where two different typedefs= from two different TUs have different types but the ABI of the fundamental= type is compatible, you should emit a warning saying something like: =E2= =80=9Ctwo typedefs types from different TUs have conflicting types. ABI com= parisons cannot be reliably done. file1.c: and file2.c:= =E2=80=9C. So basically at the place where you currently fold the fundamen= tal integral types in this patch if they are ABI compatible, print a warnin= g instead. That will let me know what the problem is when I do a test run a= nd I will report it to the upstream package rather than you. I=E2=80=99ll t= ry to fix the world and it won=E2=80=99t be your problem. Maybe have a big comment near the logic in the DWARF reader or where this i= s detected that explains the decision for the next guy.=20 So in summary:=20 1) Revert or replace this patch and add warning to libabigail. That should = satisfy both me and Giuliano.=20 2) Start a discussion with the GCC folk about adding a warning. I=E2=80=99l= l follow up. 3) I=E2=80=99ll use the GCC warning discussion to bring this up with upstre= am btrfs people and possibly suggest a patch to fix their stuff. 4) I=E2=80=99ll do the same with any other packages that have a similar pro= blem. -ben > On Aug 18, 2022, at 9:29 AM, Dodji Seketeli wrote: >=20 > =EF=BB=BFGiuliano Procida a =C3=A9crit: >=20 >> Sorry, I have to be brief... >=20 > No problem. >=20 > [...] >=20 >>> Right. But as usual with these things (API vs ABI conformance), we try >>> to accommodate people's need as much as possible, with a preference wit= h >>> ABI conformance when we can't ensure both at the same time. That's wha= t >>> we have done historically, but of course, my stance is not cast in >>> stone. I am open to discussion and change. >>>=20 >>> In this particular case of /C type/ program, it seems to be that the >>> programmers expect the types to be ABI compatible. >>=20 >> I think with so much multi-architecture code about and the difficulty >> of (say) ABI >> monitoring less common architectures, detecting API changes that will be= ABI >> breaks on another architecture seems like a big positive. >=20 > This is interesting. >=20 > Historically, I chose to analyse binaries rather than source code > precisely because I wanted to compare the ABIs of the binaries directly, > architecture per architecture, rather than trying to infer what API > change might incur an ABI change. The main assumption is that we do > *HAVE* access to the actual binaries. And what I really cared about was > actual ABI changes, not API changes. >=20 > Doing the API compatibility verification came afterwards, in a "nice to > have manner", from the request of users over time, like icing on the > cake, so to speak. The core of what I wanted really was ABI comparison. >=20 > It's funny to see how the API side of the requirement got stronger over > time. >=20 > Anyway, I think I get your point. >=20 > [...] >=20 >>>>> Otherwise, that causes spurious type changes that lead to self >>>>> comparison change down the road. For instance, the following command >>>>> fails: >>>>>=20 >>>>> $ tools/fedabipkgdiff --debug --self-compare -a --from fc36 btrfs-= progs >>>>=20 >>>> Shouldn't any tweaking of behaviour happen with abidiff rather than ab= idw? >>>=20 >>>=20 >>> I am not sure to understand the question. This kind of adjustment of >>> what is is read from the binary typically tends to happen at the core >>> level (either DWARF reader, IR construction/transformation, comparison, >>> etc) rather at the level of a specific tool. Am I missing something >>> from what you have in mind? >>=20 >> The earlier the re-interpretation is, the less visible it is and the ori= ginal >> information cannot be recovered. >=20 > Of course. But keep some information all the way can be more costly > than trimming them off early, if we know we don't need them. It's a > tradeoff that seems clear in my mind. >=20 >> Alternatively, isn't this sort of thing exactly what the suppression log= ic in >> abidiff is supposed to be used for? >=20 > Self-comparing the IR from a binary against it's abixml counterpart is > supposed to be done without any suppression specification applied. >=20 > OK, so here is what I am proposing. >=20 > Either I disable this thing altogether (namely, saying that int, short > int, long int and long long int are the same if the types have the same > size; note that other integral types are not touched by this) and give > up on the self-comparison check failure of the btrfs-progs package or I c= an > use this "abi-only-comparison" only when we do self-comparison check, > i.e, when we do abidw --abidiff and abipkgdiff --self-check. >=20 > I have a candidate patch for the later and the former is even easier to > do. >=20 > @Ben and @Giuliano, what would you prefer? >=20 > Cheers, >=20 > --=20 > Dodji >=20