Re: [PATCH 1/1, RFC] Make Front Ends first class citizens

[Dodji Seketeli <dodji@redhat.com>]

Hello,

Giuliano Procida <gprocida@google.com> 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