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.133.124]) by sourceware.org (Postfix) with ESMTPS id 746B4395C00A for ; Wed, 16 Nov 2022 17:27:26 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 746B4395C00A Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=redhat.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=redhat.com DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1668619646; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=OWKQzcsA5wgF6j4qEKHgA17qsIfS7gMxGrDNLOQiSEE=; b=C+DQPustURqTTkkzHKgTFKion3rhCcfGO0S6KWOgfMxlITfJYZwww8+KZSbVj+p6AhddJN R8b+POWGhcdt4oS3Sc6v7KgQrmGM7IyPug2SJZ/vFoPgl4upGae6PLpsjSvybXNqsnGHNR CeZNAVv5SJ6zmIt876AElr2eMIge0EE= Received: from mail-qv1-f71.google.com (mail-qv1-f71.google.com [209.85.219.71]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_128_GCM_SHA256) id us-mta-88-nrQ1JppxOsO2ZCloCbmChw-1; Wed, 16 Nov 2022 12:27:24 -0500 X-MC-Unique: nrQ1JppxOsO2ZCloCbmChw-1 Received: by mail-qv1-f71.google.com with SMTP id b2-20020a0cfe62000000b004bbfb15297dso13797494qvv.19 for ; Wed, 16 Nov 2022 09:27:24 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=mime-version:user-agent:message-id:in-reply-to:date:references :organization:subject:cc:to:from:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=OWKQzcsA5wgF6j4qEKHgA17qsIfS7gMxGrDNLOQiSEE=; b=tXNFW9fEfFuzy/Xxg8QXgg1JIWu+v2dWBywIQXTlTRaWlY4LUfegYeRV7kTMu3cobE sY2mU/AVXgs61Vn0fNOiHV/3qRNHydoesFithUs/Xa/iZJifq8XcRQrANMRnqxvdb+8+ 6o+FRDoiVMUozTd1g+1oXWGjUP+KGR89O6AcWVOOoiO/b+J7dd67su03Uc8P8VjMWGSo SUvOITdYGvRhv8g2G/RYs3bsz3nO1rNVN0KbBQPHONlqGG87GahxCtOyDnGQlCD01xzM r+Jp3rHK591dHrehoRP3cOVxhpdpBuWN+AmFjqCwcxK2ARIQ62iEcBp+dT5P6/tcaSLJ 1JTQ== X-Gm-Message-State: ANoB5pl0Hyf/JuGBl3h8zLVsHLmoWs2x9zqC4aWbYw2KFvtZLW5IHYkW juGYxwWeUf4hxl9PJMd3PPH3br0cJLGhKaJvLNP7CKlSPHbDho0HAQDPgA/N4RntWUKNdKYoV2q VCR/rOZ1/jAk3mh0cpywc X-Received: by 2002:ad4:4045:0:b0:4bf:1a71:8655 with SMTP id r5-20020ad44045000000b004bf1a718655mr21773327qvp.33.1668619644135; Wed, 16 Nov 2022 09:27:24 -0800 (PST) X-Google-Smtp-Source: AA0mqf77D67yi29y10cPuT1rXue38Qv/bXkLmE666sc8U88c9upeyONdCeMYUNb8jCCXm837qMUYWA== X-Received: by 2002:ad4:4045:0:b0:4bf:1a71:8655 with SMTP id r5-20020ad44045000000b004bf1a718655mr21773293qvp.33.1668619643791; Wed, 16 Nov 2022 09:27:23 -0800 (PST) Received: from localhost ([88.120.130.27]) by smtp.gmail.com with ESMTPSA id f21-20020a05620a409500b006fa4a81e895sm10832544qko.67.2022.11.16.09.27.22 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 16 Nov 2022 09:27:23 -0800 (PST) Received: by localhost (Postfix, from userid 1000) id C7F83581D0C; Wed, 16 Nov 2022 17:39:15 +0100 (CET) From: Dodji Seketeli To: Giuliano Procida Cc: "Guillermo E. Martinez" , libabigail@sourceware.org Subject: Re: [PATCH 1/1, RFC] Make Front Ends first class citizens Organization: Red Hat / France References: <87lep4os22.fsf@redhat.com> <87h6zsorrf.fsf@redhat.com> X-Operating-System: Fedora 38 X-URL: http://www.redhat.com Date: Wed, 16 Nov 2022 17:39:15 +0100 In-Reply-To: (Giuliano Procida's message of "Tue, 1 Nov 2022 10:49:12 +0000") Message-ID: <87sfiionv0.fsf@redhat.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/27.1 (gnu/linux) MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain X-Spam-Status: No, score=-5.6 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H2,SPF_HELO_NONE,SPF_NONE,TXREP 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: 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. > 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. > 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. [...] Cheers, -- Dodji