Re: [PATCH v3] ctf-reader: Add support to read CTF information from Linux kernel

["Guillermo E. Martinez" <guillermo.e.martinez@oracle.com>]

On Thursday, May 12, 2022 11:51:08 AM CDT Dodji Seketeli wrote:

> Hello Guillermo,
Hello Dodji
 
> "Guillermo E. Martinez via Libabigail" <libabigail@sourceware.org> a
> écrit:
> 
> So, I have applied the patch to the master branch, thanks!
> 
> I made some few cosmetic changes, though.
> 
> So please read the comments about the changes I made belows.
> 
> 
> [...]
> 
> > diff --git a/include/abg-corpus.h b/include/abg-corpus.h
> > index 652a8294..690edba5 100644
> > --- a/include/abg-corpus.h
> > +++ b/include/abg-corpus.h
> > @@ -44,10 +44,10 @@ public:
> >    enum origin
> >    {
> >      ARTIFICIAL_ORIGIN = 0,
> > -    NATIVE_XML_ORIGIN,
> > -    DWARF_ORIGIN,
> > -    CTF_ORIGIN,
> > -    LINUX_KERNEL_BINARY_ORIGIN
> > +    NATIVE_XML_ORIGIN = 1,
> > +    DWARF_ORIGIN      = 1 << 1,
> > +    CTF_ORIGIN        = 1 << 2,
> > +    LINUX_KERNEL_BINARY_ORIGIN = 1 << 3
> >    };
> >  
> >  private:
> > @@ -115,11 +115,11 @@ public:
> >    corpus_group*
> >    get_group();
> >  
> > -  origin
> > +  uint32_t
> >    get_origin() const;
> >  
> >    void
> > -  set_origin(origin);
> > +  set_origin(uint32_t);
> >  
> >    string&
> >    get_format_major_version_number() const;
> 
> These corpus::{get_origin, set_origin} accessors should return/take a
> corpus::origin type, for the sake of having a strongly typed system.
> But I think I understand why you changed them to return/take a
> corpus::origin.  It's because of ...
> 
> [...]
> 
> diff --git a/src/abg-ctf-reader.cc b/src/abg-ctf-reader.cc
> index 2c6839cb..030f8fc8 100644
> --- a/src/abg-ctf-reader.cc
> +++ b/src/abg-ctf-reader.cc
> 
> [...]
> 
> -  /* Set some properties of the corpus first.  */
> -  corp->set_origin(corpus::CTF_ORIGIN);
> -  if (!slurp_elf_info(ctxt, corp))
> +  bool is_linux_kernel = elf_helpers::is_linux_kernel(ctxt->elf_handler);
> +  uint32_t origin = corpus::CTF_ORIGIN;
> +
> +  if (is_linux_kernel)
> +    origin |= corpus::LINUX_KERNEL_BINARY_ORIGIN;
> 
> ... this, right?
Yes, it is correct, because we can use the corpus::{get_origin, set_origin}
with oring corpus::origin values.

> I think the C++ compiler wasn't happy by the fact that "origin" is set
> to the result of the |= operator, which returns an integer, by
> default, right?
That's right!

> In that case, you need to declare a new '|=' operator that takes two
> corpus::origin operands and return a corpus::origin.  That way, that
> line can just compile fine.
> 
> I did just that for you. That is, in abg-corpus.h, I declared these:
> 
>     corpus::origin
>     operator|(corpus::origin l, corpus::origin r);
> 
>     corpus::origin
>     operator|=(corpus::origin &l, corpus::origin r);
> 
>     corpus::origin
>     operator&(corpus::origin l, corpus::origin r);
> 
>     corpus::origin
>     operator&=(corpus::origin &l, corpus::origin r);
> 
> These are defined in abg-corpus.cc, so things should just work as
> intended, hopefully.
it's more elegant. thanks!

> [...]
> 
> diff --git a/src/abg-corpus-priv.h b/src/abg-corpus-priv.h
> index 778e3365..28fd1d9b 100644
> --- a/src/abg-corpus-priv.h
> +++ b/src/abg-corpus-priv.h
> @@ -670,7 +670,7 @@ struct corpus::priv
>    environment*					env;
>    corpus_group*				group;
>    corpus::exported_decls_builder_sptr		exported_decls_builder;
> -  origin					origin_;
> +  uint32_t					origin_;
> 
> I reverted this change as per what I said above.
> 
> [...]
> 
> diff --git a/src/abg-corpus.cc b/src/abg-corpus.cc
> index a517f384..94f2268c 100644
> --- a/src/abg-corpus.cc
> +++ b/src/abg-corpus.cc
> @@ -851,7 +851,7 @@ corpus::init_format_version()
>  /// Getter for the origin of the corpus.
>  ///
>  /// @return the origin of the corpus.
> -corpus::origin
> +uint32_t
>  corpus::get_origin() const
>  {return priv_->origin_;}
>  
> @@ -859,7 +859,7 @@ corpus::get_origin() const
>  ///
>  /// @param o the new origin for the corpus.
>  void
> -corpus::set_origin(origin o)
> +corpus::set_origin(uint32_t o)
>  {priv_->origin_ = o;}
> 
> Likewise.
> 
> [...]
> 
> diff --git a/src/abg-ctf-reader.cc b/src/abg-ctf-reader.cc
> index 2c6839cb..030f8fc8 100644
> --- a/src/abg-ctf-reader.cc
> +++ b/src/abg-ctf-reader.cc
> @@ -16,6 +16,8 @@
>  #include <fcntl.h> /* For open(3) */
>  #include <iostream>
>  #include <memory>
> +#include <map>
> +#include <algorithm>
>  
>  #include "ctf-api.h"
>  
> @@ -58,13 +60,19 @@ public:
>  
>    /// A map associating CTF type ids with libabigail IR types.  This
>    /// is used to reuse already generated types.
> -  unordered_map<ctf_id_t,type_base_sptr> types_map;
> +  std::map<std::string,type_base_sptr> types_map;
> 
> Any reason why don't use
>     unordered_map<string, type_base_sptr> types_map ?
> 
> I am asking just because an unordered_map is much faster than just a
> map.
oohh, let then change it for a unordered_map, thanks !.
> There is nothing wrong, though, with using a map.
> 
> [...]
> 
>    /// Associate a given CTF type ID with a given libabigail IR type.
> -  void add_type(ctf_id_t ctf_type, type_base_sptr type)
> +  void add_type(ctf_dict_t *dic, ctf_id_t ctf_type, type_base_sptr type)
>    {
> 
> I have added a doxygen description for the function parameters.
> 
> I have also added a newline between the return type (void) and the
> function name, to comply with the rest of the project.  I did so with
> a few other member function definitions in there as well.
> 
> [...]
> 
>    /// Lookup a given CTF type ID in the types map.
>    ///
>    /// @param ctf_type the type ID of the type to lookup.
> -  type_base_sptr lookup_type(ctf_id_t ctf_type)
> +  type_base_sptr lookup_type(ctf_dict_t *dic, ctf_id_t ctf_type)
>    {
> 
> Likewise.
> 
> 
> Please find below the patch that I committed.
> [...]

It looks much better!
Thanks so much Dodji!