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

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

On Tuesday, May 3, 2022 10:32:53 AM CDT Dodji Seketeli wrote:
> Hello Guillermo,
> 
> Thanks a lot for this patch!  I like it.
oohh, thanks!!
> I do have some comments however.
> Please find them below.
great,
> [...]
> 
> diff --git a/include/abg-ir.h b/include/abg-ir.h
> index a2f4e1a7..033e3708 100644
> --- a/include/abg-ir.h
> +++ b/include/abg-ir.h
> @@ -136,7 +136,16 @@ class environment
>  public:
>    struct priv;
>    std::unique_ptr<priv> priv_;
> +  /// The possible debug format types. Default is DWARF_FORMAT_TYPE
> +  enum debug_format_type
> +  {
> +    DWARF_FORMAT_TYPE,
> +#ifdef WITH_CTF
> +    CTF_FORMAT_TYPE,
> +#endif
> +  };
I added this enum to handle the options::use_ctf (--ctf) from command line,
this option is passed thought abigail::ir::environment instance to know which
reader needs to be instantiated:

if (opts.use_ctf)
    env->set_debug_format_type(environment::CTF_FORMAT_TYPE);

if (env->get_debug_format_type() == environment::DWARF_FORMAT_TYPE)
  {
     // dwarf reader
  }
else if (env->get_debug_format_type() == environment::CTF_FORMAT_TYPE)
  { 
     // ctf reader
  }

By default is environment::DWARF_FORMAT_TYPE in include/abg-ir.h

Otherwise I think that we need to change the arguments in 
`build_corpus_group_from_kernel_dist_under'. For sure I can do this :-).

> I'd prefer abg-ir.h to stay agnostic from front-end considerations
> such as the kind of input (DWARF, CTF etc) as much as possible.  Those
> considerations are handled in abg-corpus.h with the
> abigail::corpus::origin enum, which is currently defined as:
> 
>   /// This abstracts where the corpus comes from.  That is, either it
>   /// has been read from the native xml format, from DWARF or built
>   /// artificially using the library's API.
>   enum origin
>   {
>     ARTIFICIAL_ORIGIN = 0,
>     NATIVE_XML_ORIGIN,
>     DWARF_ORIGIN,
>     CTF_ORIGIN,
>     LINUX_KERNEL_BINARY_ORIGIN
>   };
> 
> You can modify it to make it be a bitmap defined as:
> 
>   enum origin
>   {
>     ARTIFICIAL_ORIGIN = 0,
>     NATIVE_XML_ORIGIN = 1,
>     DWARF_ORIGIN = 1 << 1,
>     CTF_ORIGIN = 1 << 2,
>     LINUX_KERNEL_BINARY_ORIGIN = 1 << 3
>   };
> 
> That way, you can modify abg-{ctf-reader,dwarf-reader,ir}.cc so that
> instead of saying:
> 
>     if (corp->get_origin() == corpus::LINUX_KERNEL_BINARY_ORIGIN)
>       // blah
> 
> it would instead say:
> 
>     if (corp->get_origin() & corpus::LINUX_KERNEL_BINARY_ORIGIN)
>       // blah
> 
> or even:
> 
>     if (corp->get_origin()
> 	& corpus::LINUX_KERNEL_BINARY_ORIGIN
> 	& corpus:: CTF_ORIGIN)
>       // blah
> 
> The different parts of the code that test for return of
> corpus::get_origin() should be updated to take into account that the
> value returned is now a bitmap.
> 
> Of course, in abg-ctf-reader.cc, the origin of the corpus would be set by doing:
> 
>     corp->set_origin(corpus::LINUX_KERNEL_BINARY_ORIGIN
> 		     | corpus::CTF_ORIGIN);
> 
> and in abg-dwarf-reader.cc, the origin of the corpus would be set by
> doing: 
> 
>     ctxt.current_corpus()->set_origin(corpus::LINUX_KERNEL_BINARY_ORIGIN
> 				      | corpus::DWARF_ORIGIN);
> 
> [...]
ok, totally agree.
> +  debug_format_type debug_format_;
It is used to know which reader should be instantiated depending of command line.

> So, the abigail::ir::environment type should not have a debug_format_
> data member.  If anything, all the data members are hidden in the
> abigail::ir::environment::priv_, which is a pointer to
> abigail::ir::environment::priv, defined in abg-ir-priv.h.  But I think
> it's not neccessary to add anything there.  More on that below.
> 
> [...]
> 
> diff --git a/src/abg-ctf-reader.cc b/src/abg-ctf-reader.cc
> index 2c6839cb..dcc65d4e 100644
> --- a/src/abg-ctf-reader.cc
> +++ b/src/abg-ctf-reader.cc
> 
> [...]
> 
> +  void initialize(const string& elf_path, ir::environment *env)
>    {
> 
> Please, add a doxygen comment to this function.
ok.
> [...]
> 
> +std::string
> +dic_type_key(ctf_dict_t *dic, ctf_id_t ctf_type)
> +{
> 
> Likewise.
ok.
> diff --git a/src/abg-ir.cc b/src/abg-ir.cc
> index 0ef5e8b2..5eebcfa3 100644
> --- a/src/abg-ir.cc
> +++ b/src/abg-ir.cc
> @@ -3165,7 +3165,8 @@ typedef unordered_map<interned_string,
>  
>  /// Default constructor of the @ref environment type.
>  environment::environment()
> -  :priv_(new priv)
> +  :priv_(new priv),
> +  debug_format_(DWARF_FORMAT_TYPE)
>  {}
> 
> As said above, the debug_format_ data member should not be added to
> the environment type.
> 
> [...]
> 
> diff --git a/src/abg-tools-utils.cc b/src/abg-tools-utils.cc
> index 1f0f6fa8..faa7243f 100644
> --- a/src/abg-tools-utils.cc
> +++ b/src/abg-tools-utils.cc
> 
> [...]
> 
> @@ -2543,12 +2576,21 @@ build_corpus_group_from_kernel_dist_under(const string&	root,
>    t.start();
>    bool got_binary_paths =
>      get_binary_paths_from_kernel_dist(root, debug_info_root, vmlinux, modules);
> +#ifdef WITH_CTF
> +  string vmlinux_ctfa;
> +  if (got_binary_paths &&
> +      env->get_debug_format_type() == environment::CTF_FORMAT_TYPE)
> +    {
> +      got_binary_paths = get_vmlinux_ctfa_path_from_kernel_dist(root, vmlinux_ctfa);
> +      ABG_ASSERT(!vmlinux_ctfa.empty());
> +    }
> +#endif
> +
> 
> I think, rather than calling env->get_debug_format_type()
> build_corpus_group_from_kernel_dist_under can be passed an additional
> 'corpus_origin' parameter of type corpus::origin.  The parameter would
> have corpus::DWARF_ORIGIN as default argument, for instance.
great!. So, consider it done.
> Also, I think the rest of this function that follows could be
> encapsulated into two functions that would be called:
> 
>     maybe_load_vmlinux_dwarf_corpus()
ok.
> This function would load the vmlinux corpus from the ELF information.
> That function would contain the code that is inside the block:
> 
> +      if (env->get_debug_format_type() == environment::DWARF_FORMAT_TYPE)
> +        {
> 
> The new condition inside the function would rather be something like:
> 
>     if (corpus_origin & corpus::DWARF_ORIGIN)
>       {
>          //blah
>       }
> The function wouldn't do anything if the origin of the corpus is not DWARF.
> 
> 
> The second function would be this one:
> 
> maybe_load_vmlinux_ctf_corpus()
> 
> it's invocation would be protected by an #ifdef WITH_CTF and its body
> would be inside an if-block that would read:
> 
>     if (corpus_origin & corpus::CTF_ORIGIN)
>       {
>         // blah
nice!.
> I hope this all makes sense.  If not, please do not hesitate to discuss
> it.
All make sense,
> Thanks again.
Thanks for these useful comments!!. I will send soon the patch v2 :-)

I was thinking to merge here the patch: 
https://sourceware.org/pipermail/libabigail/2022q2/004310.html[1]

Dodji, What do you think?

> Cheers,
Kind regards, 
Guillermo 



--------
[1] https://sourceware.org/pipermail/libabigail/2022q2/004310.html