diff mbox series

ctf-reader: Add support for arbitrary CTF archive member as the CTF parent

Message ID 20240716181731.3498609-1-claudiu.zissulescu-ianculescu@oracle.com
State New
Headers
Series ctf-reader: Add support for arbitrary CTF archive member as the CTF parent |

Commit Message

Claudiu Zissulescu-Ianculescu July 16, 2024, 6:17 p.m. UTC 
  From: Claudiu Zissulescu <claudiu.zissulescu-ianculescu@oracle.com>

If the CTF section contains ambiguously-defined types, it will consist
of an archive of many CTF dictionaries, all inheriting from one
dictionary containing unambiguous types.  This member is by default
named .ctf, like the section containing it, but it is possible to
change this name using the "ctf_link_set_memb_name_changer" function
at link time.  When looking at CTF archives that have been created by
a linker that uses the name changer to rename the parent archive
member, --ctf-parent can be used to specify the name used for the
parent.  This new option can be used with abidw, abidiff and
abipkgdiff tools.

	* include/abg-ctf-reader.h (parent_name): New function.
	* src/abg-ctf-reader.cc (ctf_parent_name): New variable.
	(ctf_parent_name): Sets ctf parent name.
	(parent_name): Gets the elf base reader and sets the parent name.
	* tools/abidiff.cc (ctf_parent): New option.
	(parse_command_line): Parse the new option.
	(main): Handle the new option.
	* tools/abidw.cc (ctf_parent): New option.
	(parse_command_line): Parse the new option.
	(load_corpus_and_write_abixml): Handle the new option.
	* tools/abipkgdiff.cc (ctf_parent): New option.
	(compare): Handle the new option.
	(compare_to_self): Likewise.
	(parse_command_line): Parse the new option.

Signed-off-by: Claudiu Zissulescu <claudiu.zissulescu-ianculescu@oracle.com>
---
 include/abg-ctf-reader.h |  5 ++++-
 src/abg-ctf-reader.cc    | 25 +++++++++++++++++++++++++
 tools/abidiff.cc         | 27 +++++++++++++++++++++++++++
 tools/abidw.cc           | 19 +++++++++++++++++++
 tools/abipkgdiff.cc      | 38 ++++++++++++++++++++++++++++++++++++++
 5 files changed, 113 insertions(+), 1 deletion(-)

Comments

Dodji Seketeli July 17, 2024, 1:38 p.m. UTC | #1 
Hello Claudiu,

claudiu.zissulescu-ianculescu@oracle.com a écrit:

> From: Claudiu Zissulescu <claudiu.zissulescu-ianculescu@oracle.com>
>
> If the CTF section contains ambiguously-defined types,

Just to be sure I understand it correctly.  What do you mean by an
"ambiguously-defined type"?

For instance, suppose that in a translation unit a.c, we define a
(struct) type named T.

And then in another translation unit b.c, (in the same binary) we define
another type named T as well, but with a definition different from the
one coming from a.c.

Is T an ambiguously defined type?  If yes, would that be what the CTF
doc calls "types with identical names but conflicting definitions" at
https://sourceware.org/binutils/docs/ctf-spec.html#CTF-archive?


> it will consist of an archive of many CTF dictionaries, all inheriting
> from one dictionary containing unambiguous types.

Right. (Assuming my understanding above is correct).

> This member is by default named .ctf, like the section containing it,

By "member", do you mean the parent dictionary that contains the
unambiguous types?

> but it is possible to change this name using the
> "ctf_link_set_memb_name_changer" function at link time.

OK.  In that case, when looking at the child dictionary referencing the
parent dictionary, is it possible to discover the name of the parent?
Is it possible to do that using libctf?

> When looking at CTF archives that have been created by
> a linker that uses the name changer to rename the parent archive
> member, --ctf-parent can be used to specify the name used for the
> parent.  This new option can be used with abidw, abidiff and
> abipkgdiff tools.

OK.  If the name can be discovered by looking at the binary itself
(maybe using libctf) can we do that instead, rather than relying the
--ctf-parent option?  I am not asking for this patch to be dropped.
It's just by curiosity.  If it's possible to discover the parent name,
maybe that could be a subsequent patch, who knows.

[...]

> diff --git a/src/abg-ctf-reader.cc b/src/abg-ctf-reader.cc
> index 4b6c6cd8..6ee96265 100644
> --- a/src/abg-ctf-reader.cc
> +++ b/src/abg-ctf-reader.cc
> @@ -156,6 +156,9 @@ class reader : public elf_based_reader
>    ctf_sect_t strtab_sect;
>    translation_unit_sptr cur_tu_;
>  
> +  ///CTF parent name.

Please insert a space before the CTF word, for the sake of consistency
with the rest of the comments of the front-end.

> +  std::string ctf_parent_name;
> +
>  public:
>  
>    /// Getter of the exported decls builder object.
> @@ -464,6 +467,10 @@ public:
>  
>  	std::replace(dict_name.begin(), dict_name.end(), '-', '_');
>        }
> +    else
> +      {
> +	dict_name = ctf_parent_name;
> +      }

I don't understand the condition that you intend for setting dict_name
to the argument of the --ctf-parent option here.

My understanding is that the dictionary name is going to be set to the
argument of the --ctf-parent option (provided to the tool) if we are
looking at a user-space binary or if we are looking at a single kernel
binary (i.e, there is no corpus_group being currently built).  Is that
what you want to do?  If yes, why?

Also, the curly brackets are not necessary.  Again, this is for the sake
of consistency with the rest of the project.

[...]


> diff --git a/tools/abidiff.cc b/tools/abidiff.cc
> index cadb742b..515b5128 100644
> --- a/tools/abidiff.cc
> +++ b/tools/abidiff.cc
> @@ -134,6 +134,7 @@ struct options
>  #endif
>  #ifdef WITH_CTF
>    bool			use_ctf;
> +  string                ctf_parent;

Ah I think the rest of the data member are aligned using tabs and this
new one is aligned using spaces, hence the inconsistency.

[...]


> +
> +#ifdef WITH_CTF
> +	    // If the user sets the ctf parent make sure we propagate it.
> +	    if (!opts.ctf_parent.empty()
> +		&& opts.use_ctf)
> +	      {
> +		abigail::ctf::parent_name(*rdr, opts.ctf_parent);
> +	      }
> +#endif

The curly brackets are unnecessary.

[...]

> +#ifdef WITH_CTF
> +	    // If the user sets the ctf parent make sure we propagate it.
> +	    if (!opts.ctf_parent.empty()
> +		&& opts.use_ctf)
> +	      {
> +		abigail::ctf::parent_name(*rdr, opts.ctf_parent);
> +	      }
> +#endif

Likewise.

>  	    c2 = rdr->read_corpus(c2_status);
>  
>  	    if (!c2
> diff --git a/tools/abidw.cc b/tools/abidw.cc
> index 14f2078c..41909512 100644
> --- a/tools/abidw.cc
> +++ b/tools/abidw.cc
> @@ -118,6 +118,7 @@ struct options
>    bool			list_dependencies;
>  #ifdef WITH_CTF
>    bool			use_ctf;
> +  string                ctf_parent;

Space vs tabs here as well.

[...]


> +#ifdef WITH_CTF
> +  // If the user sets the ctf parent make sure we propagate it.
> +  if (!opts.ctf_parent.empty()
> +      && opts.use_ctf)
> +    {
> +      abigail::ctf::parent_name(*reader, opts.ctf_parent);
> +    }
> +#endif
> +

The curly brackets are unnecessary here as well.

[...]

> diff --git a/tools/abipkgdiff.cc b/tools/abipkgdiff.cc
> index d1ddd341..0d6a1fb2 100644
> --- a/tools/abipkgdiff.cc
> +++ b/tools/abipkgdiff.cc
> @@ -214,6 +214,7 @@ public:
>    optional<bool> exported_interfaces_only;
>  #ifdef WITH_CTF
>    bool		use_ctf;
> +  string        ctf_parent;

Spaces vs tabs here as well.

[...]

> +#ifdef WITH_CTF
> +    // If the user sets the ctf parent make sure we propagate it.
> +    if (!opts.ctf_parent.empty()
> +	&& opts.use_ctf)
> +      {
> +	abigail::ctf::parent_name(*reader, opts.ctf_parent);
> +      }
> +#endif
> +

The curly brackets are unnecessary here as well.

[...]

> @@ -1575,6 +1586,16 @@ compare(const elf_file&		elf1,
>      reader->add_suppressions(priv_types_supprs2);
>      set_generic_options(*reader, opts);
>  
> +#ifdef WITH_CTF
> +    // If the user sets the ctf parent make sure we propagate it.
> +    // Note: Do we need to have sparate parents per package?
> +    if (!opts.ctf_parent.empty()
> +	&& opts.use_ctf)
> +      {
> +	abigail::ctf::parent_name(*reader, opts.ctf_parent);
> +      }
> +#endif
> +

Likewise.

[...]

> @@ -1741,6 +1762,15 @@ compare_to_self(const elf_file&		elf,
>      reader->add_suppressions(supprs);
>      corp = reader->read_corpus(c_status);
>  
> +#ifdef WITH_CTF
> +    // If the user sets the ctf parent make sure we propagate it.
> +    if (!opts.ctf_parent.empty()
> +	&& opts.use_ctf)
> +      {
> +	abigail::ctf::parent_name(*reader, opts.ctf_parent);
> +      }
> +#endif
> +

Likewise.

>      if (!(c_status & abigail::fe_iface::STATUS_OK))
>        {
>  	if (opts.verbose)
> @@ -3631,6 +3661,14 @@ parse_command_line(int argc, char* argv[], options& opts)
>  #ifdef WITH_CTF
>  	else if (!strcmp(argv[i], "--ctf"))
>            opts.use_ctf = true;
> +        else if (!strcmp(argv[i], "--ctf-parent"))
> +	  {
> +	    int j = i + 1;
> +	    if (j >= argc)
> +	      return false;
> +	    opts.ctf_parent = argv[j];
> +	    ++i;
> +	  }
>  #endif
>  #ifdef WITH_BTF
>  	else if (!strcmp(argv[i], "--btf"))

I think the patch would be OK to go with my above comments addressed.  I
have one more question.  Would it be possible to have some test in place
for it?  If it uses a small binary then it could go into tests/
Otherwise, it could go in the libabigail-tests.git repository as a
subsequent patch.

Anyway, many thanks for your work!

Cheers,
diff mbox series

Patch

diff --git a/include/abg-ctf-reader.h b/include/abg-ctf-reader.h
index c1f2a3f9..4e7f5a12 100644
--- a/include/abg-ctf-reader.h
+++ b/include/abg-ctf-reader.h
@@ -37,7 +37,10 @@  void
 reset_reader(elf_based_reader&		ctxt,
 	     const std::string&	elf_path,
 	     const vector<char**>&	debug_info_root_path);
+
+void
+parent_name(elf_based_reader& rdr,
+	    const std::string& parent_name);
 } // end namespace ctf_reader
 } // end namespace abigail
-
 #endif // ! __ABG_CTF_READER_H__
diff --git a/src/abg-ctf-reader.cc b/src/abg-ctf-reader.cc
index 4b6c6cd8..6ee96265 100644
--- a/src/abg-ctf-reader.cc
+++ b/src/abg-ctf-reader.cc
@@ -156,6 +156,9 @@  class reader : public elf_based_reader
   ctf_sect_t strtab_sect;
   translation_unit_sptr cur_tu_;
 
+  ///CTF parent name.
+  std::string ctf_parent_name;
+
 public:
 
   /// Getter of the exported decls builder object.
@@ -464,6 +467,10 @@  public:
 
 	std::replace(dict_name.begin(), dict_name.end(), '-', '_');
       }
+    else
+      {
+	dict_name = ctf_parent_name;
+      }
 
     if ((ctf_dict = ctf_dict_open(ctfa,
 				  dict_name.empty() ? NULL : dict_name.c_str(),
@@ -745,6 +752,13 @@  public:
     return corp;
   }
 
+  /// Sets ctf_parent_name.
+  void
+  set_parent_name(const std::string& parent_name)
+  {
+    ctf_parent_name = parent_name;
+  }
+
   /// Destructor of the CTF reader.
   ~reader()
   {
@@ -1748,6 +1762,17 @@  reset_reader(elf_based_reader&		rdr,
   r.initialize(elf_path, debug_info_root_path);
 }
 
+/// Set a given parent name.
+///
+/// @param ctf_parent_name the custom ctf parent name.
+void
+parent_name(elf_based_reader& rdr,
+	    const std::string& parent_name)
+{
+  ctf::reader& r = dynamic_cast<reader&>(rdr);
+  r.set_parent_name(parent_name);
+}
+
 /// Returns a key to be use in types_map dict conformed by
 /// dictionary id and the CTF type id for a given type.
 ///
diff --git a/tools/abidiff.cc b/tools/abidiff.cc
index cadb742b..515b5128 100644
--- a/tools/abidiff.cc
+++ b/tools/abidiff.cc
@@ -134,6 +134,7 @@  struct options
 #endif
 #ifdef WITH_CTF
   bool			use_ctf;
+  string                ctf_parent;
 #endif
 #ifdef WITH_BTF
   bool			use_btf;
@@ -310,6 +311,7 @@  display_usage(const string& prog_name, ostream& out)
     <<  " --stats  show statistics about various internal stuff\n"
 #ifdef WITH_CTF
     << " --ctf use CTF instead of DWARF in ELF files\n"
+    << " --ctf-parent <name> set the CTF archive parent name\n"
 #endif
 #ifdef WITH_BTF
     << " --btf use BTF instead of DWARF in ELF files\n"
@@ -756,6 +758,14 @@  parse_command_line(int argc, char* argv[], options& opts)
 #ifdef WITH_CTF
       else if (!strcmp(argv[i], "--ctf"))
         opts.use_ctf = true;
+      else if (!strcmp(argv[i], "--ctf-parent"))
+	{
+	  int j = i + 1;
+	  if (j >= argc)
+	    return false;
+	  opts.ctf_parent = argv[j];
+	  ++i;
+	}
 #endif
 #ifdef WITH_BTF
       else if (!strcmp(argv[i], "--btf"))
@@ -1432,6 +1442,15 @@  main(int argc, char* argv[])
             ABG_ASSERT(rdr);
 	    set_generic_options(*rdr, opts);
 	    set_suppressions(*rdr, opts);
+
+#ifdef WITH_CTF
+	    // If the user sets the ctf parent make sure we propagate it.
+	    if (!opts.ctf_parent.empty()
+		&& opts.use_ctf)
+	      {
+		abigail::ctf::parent_name(*rdr, opts.ctf_parent);
+	      }
+#endif
 	    c1 = rdr->read_corpus(c1_status);
 
 	    if (!c1
@@ -1524,6 +1543,14 @@  main(int argc, char* argv[])
 	    set_generic_options(*rdr, opts);
 	    set_suppressions(*rdr, opts);
 
+#ifdef WITH_CTF
+	    // If the user sets the ctf parent make sure we propagate it.
+	    if (!opts.ctf_parent.empty()
+		&& opts.use_ctf)
+	      {
+		abigail::ctf::parent_name(*rdr, opts.ctf_parent);
+	      }
+#endif
 	    c2 = rdr->read_corpus(c2_status);
 
 	    if (!c2
diff --git a/tools/abidw.cc b/tools/abidw.cc
index 14f2078c..41909512 100644
--- a/tools/abidw.cc
+++ b/tools/abidw.cc
@@ -118,6 +118,7 @@  struct options
   bool			list_dependencies;
 #ifdef WITH_CTF
   bool			use_ctf;
+  string                ctf_parent;
 #endif
 #ifdef WITH_BTF
   bool			use_btf;
@@ -261,6 +262,7 @@  display_usage(const string& prog_name, ostream& out)
 #endif
 #ifdef WITH_CTF
     << "  --ctf use CTF instead of DWARF in ELF files\n"
+    << "  --ctf-parent <name> set the CTF archive parent name\n"
 #endif
     << "  --no-leverage-dwarf-factorization  do not use DWZ optimisations to "
     "speed-up the analysis of the binary\n"
@@ -402,6 +404,14 @@  parse_command_line(int argc, char* argv[], options& opts)
 #ifdef WITH_CTF
         else if (!strcmp(argv[i], "--ctf"))
           opts.use_ctf = true;
+        else if (!strcmp(argv[i], "--ctf-parent"))
+	  {
+	    int j = i + 1;
+	    if (j >= argc)
+	      return false;
+	    opts.ctf_parent = argv[j];
+	    ++i;
+	  }
 #endif
 #ifdef WITH_BTF
         else if (!strcmp(argv[i], "--btf"))
@@ -683,6 +693,15 @@  load_corpus_and_write_abixml(char* argv[],
   set_generic_options(*reader, opts);
   set_suppressions(*reader, opts);
 
+#ifdef WITH_CTF
+  // If the user sets the ctf parent make sure we propagate it.
+  if (!opts.ctf_parent.empty()
+      && opts.use_ctf)
+    {
+      abigail::ctf::parent_name(*reader, opts.ctf_parent);
+    }
+#endif
+
   // If the user asked us to check if we found the "alternate debug
   // info file" associated to the input binary, then proceed to do so
   // ...
diff --git a/tools/abipkgdiff.cc b/tools/abipkgdiff.cc
index d1ddd341..0d6a1fb2 100644
--- a/tools/abipkgdiff.cc
+++ b/tools/abipkgdiff.cc
@@ -214,6 +214,7 @@  public:
   optional<bool> exported_interfaces_only;
 #ifdef WITH_CTF
   bool		use_ctf;
+  string        ctf_parent;
 #endif
 #ifdef WITH_BTF
   bool		use_btf;
@@ -922,6 +923,7 @@  display_usage(const string& prog_name, ostream& out)
     "binaries inside the input package against their ABIXML representation\n"
 #ifdef WITH_CTF
     << " --ctf                          use CTF instead of DWARF in ELF files\n"
+    << " --ctf-parent <name>            set the CTF archive parent name\n"
 #endif
 #ifdef WITH_BTF
     << " --btf                          use BTF instead of DWARF in ELF files\n"
@@ -1486,6 +1488,15 @@  compare(const elf_file&		elf1,
     reader->add_suppressions(supprs);
     set_generic_options(*reader, opts);
 
+#ifdef WITH_CTF
+    // If the user sets the ctf parent make sure we propagate it.
+    if (!opts.ctf_parent.empty()
+	&& opts.use_ctf)
+      {
+	abigail::ctf::parent_name(*reader, opts.ctf_parent);
+      }
+#endif
+
     corpus1 = reader->read_corpus(c1_status);
 
     bool bail_out = false;
@@ -1575,6 +1586,16 @@  compare(const elf_file&		elf1,
     reader->add_suppressions(priv_types_supprs2);
     set_generic_options(*reader, opts);
 
+#ifdef WITH_CTF
+    // If the user sets the ctf parent make sure we propagate it.
+    // Note: Do we need to have sparate parents per package?
+    if (!opts.ctf_parent.empty()
+	&& opts.use_ctf)
+      {
+	abigail::ctf::parent_name(*reader, opts.ctf_parent);
+      }
+#endif
+
     corpus2 = reader->read_corpus(c2_status);
 
     bool bail_out = false;
@@ -1741,6 +1762,15 @@  compare_to_self(const elf_file&		elf,
     reader->add_suppressions(supprs);
     corp = reader->read_corpus(c_status);
 
+#ifdef WITH_CTF
+    // If the user sets the ctf parent make sure we propagate it.
+    if (!opts.ctf_parent.empty()
+	&& opts.use_ctf)
+      {
+	abigail::ctf::parent_name(*reader, opts.ctf_parent);
+      }
+#endif
+
     if (!(c_status & abigail::fe_iface::STATUS_OK))
       {
 	if (opts.verbose)
@@ -3631,6 +3661,14 @@  parse_command_line(int argc, char* argv[], options& opts)
 #ifdef WITH_CTF
 	else if (!strcmp(argv[i], "--ctf"))
           opts.use_ctf = true;
+        else if (!strcmp(argv[i], "--ctf-parent"))
+	  {
+	    int j = i + 1;
+	    if (j >= argc)
+	      return false;
+	    opts.ctf_parent = argv[j];
+	    ++i;
+	  }
 #endif
 #ifdef WITH_BTF
 	else if (!strcmp(argv[i], "--btf"))