From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-io1-xd2c.google.com (mail-io1-xd2c.google.com [IPv6:2607:f8b0:4864:20::d2c]) by sourceware.org (Postfix) with ESMTPS id 9F06A39540A8 for ; Thu, 17 Nov 2022 08:30:45 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 9F06A39540A8 Authentication-Results: sourceware.org; dmarc=pass (p=reject dis=none) header.from=google.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=google.com Received: by mail-io1-xd2c.google.com with SMTP id r81so871165iod.2 for ; Thu, 17 Nov 2022 00:30:45 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=92z9IH3pMq2NaVnbxwY0euw7mZ6GQLWTBCurpnZ6JXo=; b=nPIoSClMXia2bYWCaeaqV1j1QmZE2i1VNW4fZARyzXoc14ZOjsmgyxFVX0nIetCl0d spGKMC/IsrH8jmDEuZx6UQVffhot+hSC+JUkujtsthEqDd5Pha6g864j86Vj1XqEn0O8 hVPI5wE8qoo/5Z5vpTgqO7nIxnFIEAPCpeGhtdJ3NkoowCM5LsobE+M/TgzlbG6csYOt F6h2HBS0F8GecSS4jaqEUWxO45+4rAe1KtyfHTxOnriiZi1SjK6lTQ0YK76iKYvqSHuU kFCWEApr1HlGZhR79mxu1KsQ1v+z6hz15XP94BrI3Bfg+7GSjDPdDik2MgKAzDthQIu0 +eAg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=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=92z9IH3pMq2NaVnbxwY0euw7mZ6GQLWTBCurpnZ6JXo=; b=Bt3uxQ6xTJm9749hFlRU5JijonADroGzGmbpan+NkxF+VhbLj+8SVN3DBoeujsmNrC AZje4cgUR7P8LejL0MLDMz0G4taI337ZMgfuTm1/TAQT9p9mSA/6suKY6aBeZVVAQdSD rkFnL8h/8Mn32G0HLhciafW3FNDron7hp3EeMsp9LoLoBANdaruZkgOHwKGx+Rin3SS3 87S/kdzjftJoidcOg4mwvdhjBCkEE5I1zPIfRzTLAhS5iDcHAOcLCo2A7zJcEoKDokIB L93hJpd5J+I8RunIYIOB6FckA89/UPx9m26Z6pQulRCWqawImhugNromIDdSJIg3Dwb1 Jvgg== X-Gm-Message-State: ANoB5pltdkwYindPsMsLaNGQSjVudacH1Tw5TeZf+f9YbRqpuoxiI4Ul V8FQeYl6zz2cOKMrzs46slxXLBd6M9COTriSv07Jcg== X-Google-Smtp-Source: AA0mqf45uhz6yER/CuLA4ezUwCp9N9omj3XT/vEpGSv352F+Q/XoyDcZ2+GkDZQktkntujo/J/L5NHPRLdkn/oWp2Iw= X-Received: by 2002:a6b:d610:0:b0:6d0:46f3:6746 with SMTP id w16-20020a6bd610000000b006d046f36746mr923770ioa.139.1668673844564; Thu, 17 Nov 2022 00:30:44 -0800 (PST) MIME-Version: 1.0 References: <87lep4os22.fsf@redhat.com> <87h6zsorrf.fsf@redhat.com> <87sfiionv0.fsf@redhat.com> In-Reply-To: <87sfiionv0.fsf@redhat.com> From: Giuliano Procida Date: Thu, 17 Nov 2022 08:30:07 +0000 Message-ID: Subject: Re: [PATCH 1/1, RFC] Make Front Ends first class citizens To: Dodji Seketeli Cc: "Guillermo E. Martinez" , libabigail@sourceware.org Content-Type: text/plain; charset="UTF-8" X-Spam-Status: No, score=-20.8 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,USER_IN_DEF_DKIM_WL,USER_IN_DEF_SPF_WL 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: Hi. On Wed, 16 Nov 2022 at 17:27, Dodji Seketeli wrote: > > Hello, > > Giuliano Procida writes: > > > It sounds like fe_iface should be a very slim pure interface. If you give it > > private data and implementation functionality, you will tie the various > > concrete loader implementations together in perhaps unnecessary > > ways. > > I actually started that way, with a pure interface, namely only with the > fe_iface::read_corpus() pure method to implement. The methods that I > have added to it were those that were strictly common to all the > front-ends. So, in practise, it really is the smallest interface > exposed by front-ends as they are today. > > The only method that a hacker who wants to write a front-end will have > to implement is the fe_iface::read_corpus() one. > > I could have have had, for instance, a fe_iface_base interface that is > super slim with only fe_iface_base::read_corpus() method. fe_iface would > inherit that one. But then what would be the /practical/ point of it > really ... > > > The common functionality mentioned should perhaps be part of corpus > > Hmmh, I don't think so. The front-end can be used, for instance, to > analyze an application which itself loads "plugins". As a result of the > analysis, the ABI of the union of the application and its plugins would > then be abstracted as a ABI corpus-group, not an ABI corpus. > > I guess what I want to say there is that the front-end interface is a > concept that is different from the concept of ABI corpus. Thus > properties and behaviours that are carried by the front-end (and thus > the fe_iface) are inherently different from the properties and behaviour > of the corpus. So the methods of the front-end interface don't > logically belong to the corpus. At least, that is how these two > concepts are designed here. > > For instance, the suppression specifications provided by the user to the > front-end are considered as a property carried by the front end, as they > don't necessary apply to the ABI corpus. They might apply to entities > that are not relevant to the resulting ABI corpus. > > > or just exist standalone if they don't need any privileged access to either > > side of things. > > They can exist somewhere else, indeed. But in this case, it seems to me > that those properties and behaviour logically belong to the front-end as > being the entity that reads the input format and performs the necessary > front-end-level analysis. > > > Overall this sounds like a good idea. If the net result is fewer lines of > > code needed to achieve the same result then it will definitely have been > > a success. > > Thank you. > > The other side effect that I wanted is to reduce the amount of work > needed to come up with a new front-end. Especially, an elf-based front > end. This is done by maximizing the re-use of functionalities by > "classes" of front-ends. For instance, ELF based front-ends are a class > of front-end, as opposed to a front-end that groks a text-based format, > for instance. > > > > >> That class provides properties and behaviours that are shared by all > >> front-ends that libabigail supports today. Namely, that class > >> provides access to (properties of) the ABI corpus, corpus groups, > >> associated suppression specifications. It also provides an abstract > >> interface to perform the actual loading of the ABI corpus. That > >> abstract interface has be implemented by every single concrete > >> front-end that is provided in libabigail. > > Pfff, sorry, I think that text of mine was not clear. What I meant to > say is that the fe_iface gives access to the ABI corpus (group) being > constructed. I fixed that in the next version. > > > Corpus group. This bit is not directed at the RFC here, but it's an > > opportunity to pass on some thoughts. > > > > I think the history of this feature is that it was added to support vmlinux > > plus kernel modules. It could also be used, in theory, to support other > > kinds of multi-object ABIs. > > Actually, I started it to support applications loading plugins (like the > Xorg server). And it proved useful for the vmlinux + modules, indeed. Thanks for correcting my view of history here. I did spend a little time thinking about the more general issue of multiple binary objects and ABIs. There is the issue of inter-object dependency which is (partly?) modelled by the "needs" section in the ABI XML. I think there are at least two cases which can overlap, making things even more complicated. 1. a single ABI All the binary objects must be link-compatible (whatever "link" means in the given context) and when linked expose a combined ABI surface. This is useful for the: kernel alone, the kernel with bundled modules, the kernel with bundled and third party modules (so 3 ABIs). Adding more modules in a compatible fashion can extend an ABI. I don't think this needs the concept of a corpus group. 2. a collection of objects Some, but in general not all, combinations of the objects are meaningfully linkable, and there's perhaps no sensible way of talking about combined ABIs. I'm guessing this is how GIMP, Xorg etc. (dl_open) plugins work. This probably does need the corpus group concept, if you really need to keep track of all the plugins. How is this different from libc + /bin/ls though? Probably plugins supply an interface of some kind. > > We are not going to support the equivalent container functionality in > > our own ABI tooling implementation. For now, the perspective we're > > taking on vmlinux + kernel objects is that they present a single ABI > > which can be obtained by merging (i.e., linking) the ABIs of the > > constituent objects. > > Fair enough. This was used after some discussions with some kernel > hackers who found it useful to be able to detect (and review) cases like > a function/variable that is part of the kernel ABI, defined in a module, > got moved into a different module, or things like that. > > But if you don't want that kind of functionality, then it's indeed > unnecessary to go through the hassle. > > > The implementation in libabigail is incomplete in at least sense, > > abidiff on corpus group XML does not handle symbols in the > > second, third etc. corpora. > > Hmmh, if this is the case, then this is just a bug that ought to be > fixed. I don't remember having seen it reported though. Thank you for > mentioning it. I should have checked. Possibly mentioned somewhere parenthetically. Sorry! > > Corpus group is not a free abstraction and I would suggest you do > > as much as you can to avoid it spreading into new places. As Ben > > said in his reply, it may be better to put the corpus group functionality > > elsewhere. > > Interesting thought. I'll reply to Ben's email right away. Thanks. > > [...] > > > > >> Then, there is the "ELF Reader" front-end. Its class name is > >> abigail::elf_reader. It inherits the abigail::fe_iface class and > >> implements the fe_iface::load_corpus() so that the ELF properties of > >> the ELF file be loaded and exposed in the ABI corpus as returned by > >> the fe_iface::corpus() accessor. This ELF reader front-end also > >> provides lots of capabilities that are specific to accessing ELF > >> content. > >> > >> Then, there is a common base class for ELF-based front-ends to come, > >> named abigail::elf_based_reader, which inherits the abigail::fe_iface > >> class. The purpose of this base class is to provide common properties > >> and behaviours that are necessary to implement things like a DWARF or > >> a CTF front-end, or any other front-end to support an ELF-based debug > >> info format. > > > > I'd prefer composition over inheritance (i.e, making the common ELF > > functionality a reusable component). > > It's both actually. Facilities to access ELF parts are in > src/abg-elf-helpers.{h,cc} and are re-usable. But then, the > elf::reader::read_corpus() knows how to read the ELF properties and > populate the current corpus with their ELF part. So when implementing > their own read_corpus() member function, elf based front-ends just have > to call their parent elf::reader::read_corpus() to populate the generic > ELF bits of the current ABI corpus being built. > > Of course, this could have been done by an external function. I guess > the inheritance flavour is a convenient way to share context > implicitly. > > I understand that some people prefer to avoid this implicit context > sharing for various reasons. I guess it's also a matter of the kind of > constraints one wants to enforce. > > Here, what I want is to ease adding a new (ELF-based) front-end. I > didn't know we'd have to support things other than DWARF. Now that I > know it and that the need for other ones is emerging, I really want that > to be easy to do for fellows that are tools-oriented hackers, without > having them look around for days before being done: Just write a class > that inherits elf_based_reader. If you do nothing, > elf_based_reader::read_corpus() will at least load the generic elf bits > of the ABI and you are done. > > > However, libabigail is built around inheritance hierarchies, so > > continuing in this fashion is certainly a consistent design choice. > > Exactly. > > >> Then, there is a CTF front-end which class is named > >> abigail::ctf_reader::reader. It inherits the > > > > abigail::ctf::reader? > > Indeed, fixed. Thanks. > > > >> abigail::elf_based_reader class and implements the > >> fe_iface::load_corpus() interface to load and analyse the CTF-specific > >> properties of the ELF file. To do this, abigail::ctf_reader::reader > >> re-uses the abigail::elf_reader::load_corpus() member function to load > >> the generic ELF parts of the ABI corpus. This reader then constructs > >> the internal representation of the ABI corpus and passes it to the > >> middle-end for further analysis. > >> > >> Then, there is a DWARF front-end which class is named > >> abigail::dwarf_reader::reader. It inherits the > > > > abigail::dwarf::reader? > > Yup, fixed. > > > > >> abigail::elf_based_reader class and implements the > >> fe_iface::load_corpus() interface to load and analyse the > >> DWARF-specific properties of the ELF file. To do this, > >> abigail::dwarf_reader::reader re-uses the > >> abigail::elf_reader::load_corpus() member function to load the generic > >> ELF parts of the ABI corpus, just like what the CTF front-end does. > >> And then, just like the CTF front-end, this reader then constructs the > >> internal representation of the ABI corpus and passes it to the > >> middle-end for further analysis. > >> > >> Lastly, there is an ABIXML front-end which class is named > >> abigail::abixml_reader::reader. It inherits the abigail::fe_iface > > > > abigail::xml::reader? > > I called it abigail::abixml::reader instead. > > The 'xml' namespace is already used for libxml helpers. > > But yeah, good catch, thanks! > > [...] > > >> So, the internal representation types have been changed to refer to > >> the environment by reference, rather than requiring a pointer to it. > > > > Good choice. > > > > As a general suggestion, something like the switch to passing around > > environment by reference could be factored into a separate (earlier) > > commit. This would make it easier to review the code. > > Heh, I thought I could sneak that in quickly like this and you caught me > right there, oops ;-) > > Fair enough, you are of course right. I have done the split. I'll be > sending it as part of the updated patch-set soon. > > > I've had to switch Gmail to basic HTML mode in order to be able to reply to > > this commit. However, that may be a problem with my laptop or browser > > rather than purely being down to the size of the email message. :-) > > Pff, sorry about that. > > Thanks a lot for your review, it really is appreciated. Welcome! Giuliano. > [...] > > Cheers, > > -- > Dodji >