diff mbox series

[02/20] abg-ir: elf_symbol: add is_in_ksymtab field

Message ID 20210127125853.886677-3-maennich@google.com
State Committed
Headers
Series Refactor (k)symtab reader |

Commit Message

Matthias Männich Jan. 27, 2021, 12:58 p.m. UTC 
  Being exported through a ksymtab (in case of Linux Kernel binaries) is
actually a property of the Elf symbol itself and we can therefore track
it along with the symbol that we collect from symtab. While tracking is
currently done by keeping separate symbol lists and maps for symtab and
ksymtab symbols, they can be consolidated having a property to indicate
whether this symbol also appeared as a ksymtab entry.

Hence, and for future changes in this area, add this property and update
all references. The flag is false initially unless otherwise specified.

	* include/abg-ir.h (elf_symbol::elf_symbol): Add is_in_ksymtab
	parameter.
	(elf_symbol::create): Likewise.
	(elf_symbol::is_in_ksymtab): New getter declaration.
	(elf_symbol::set_is_in_ksymtab): New setter declaration.
	* src/abg-ir.cc (elf_symbol::priv::priv): Add is_in_ksymtab
	parameter.
	(elf_symbol::priv::is_in_ksymtab_): New field.
	(elf_symbol::elf_symbol): Add is_in_ksymtab parameter.
	(elf_symbol::create): Likewise.
	(elf_symbol::is_in_ksymtab): New getter implementation.
	(elf_symbol::set_is_in_ksymtab): New setter implementation.

Reviewed-by: Giuliano Procida <gprocida@google.com>
Signed-off-by: Matthias Maennich <maennich@google.com>
---
 include/abg-ir.h |  34 +++++++++------
 src/abg-ir.cc    | 105 +++++++++++++++++++++++++++++------------------
 2 files changed, 86 insertions(+), 53 deletions(-)

Comments

Dodji Seketeli March 9, 2021, 2:05 p.m. UTC | #1 
Hello,

Matthias Maennich <maennich@google.com> a écrit:

> Being exported through a ksymtab (in case of Linux Kernel binaries) is
> actually a property of the Elf symbol itself and we can therefore track
> it along with the symbol that we collect from symtab. While tracking is
> currently done by keeping separate symbol lists and maps for symtab and
> ksymtab symbols, they can be consolidated having a property to indicate
> whether this symbol also appeared as a ksymtab entry.

Just for some context, the reason why I kept that property out of the
[ir] module is that in my mind, the abigail::ir::elf_symbol type was
meant to be "independant" from whatever specificity a particular
platform would have, regarding an ELF symbol.  For instance, when
looking at user space ELF symbol, "is_in_ksymtab" does not really make
sense.  I guess this "ideal" got destroyed by the addition of the
"is_linux_string_cst" property to that type, and I think I mourned that
ideal at this point.

I think we can now say that the userspace and kernel use case are first
citizen use cases and they just ought to be there :-)

[...]

> diff --git a/src/abg-ir.cc b/src/abg-ir.cc
> index d1d02f3aad5d..22532cc87dfd 100644
> --- a/src/abg-ir.cc
> +++ b/src/abg-ir.cc

[...]
> @@ -1686,6 +1703,14 @@ bool
>  elf_symbol::is_variable() const
>  {return get_type() == OBJECT_TYPE || get_type() == TLS_TYPE;}
>  
> +bool
> +elf_symbol::is_in_ksymtab() const
> +{return priv_->is_in_ksymtab_;}
> +
> +void
> +elf_symbol::set_is_in_ksymtab(bool is_in_ksymtab)
> +{priv_->is_in_ksymtab_ = is_in_ksymtab;}
> +

These two member function definitions lack doxygen comment.  I took the
liberty to add some there.

[...]


> Reviewed-by: Giuliano Procida <gprocida@google.com>
> Signed-off-by: Matthias Maennich <maennich@google.com>

Applied to master with the changes above.

[...]

Thanks!
diff mbox series

Patch

diff --git a/include/abg-ir.h b/include/abg-ir.h
index c32b2f7fe989..d82c83592f82 100644
--- a/include/abg-ir.h
+++ b/include/abg-ir.h
@@ -837,8 +837,9 @@  private:
 	     bool		d,
 	     bool		c,
 	     const version&	ve,
-	     visibility	vi,
-	     bool		is_linux_string_cst = false);
+	     visibility		vi,
+	     bool		is_linux_string_cst = false,
+	     bool		is_in_ksymtab = false);
 
   elf_symbol(const elf_symbol&);
 
@@ -851,17 +852,18 @@  public:
   create();
 
   static elf_symbol_sptr
-  create(const environment*	e,
-	 size_t		i,
-	 size_t		s,
-	 const string&		n,
-	 type			t,
-	 binding		b,
-	 bool			d,
-	 bool			c,
-	 const version&	ve,
-	 visibility		vi,
-	 bool			is_linux_string_cst = false);
+  create(const environment* e,
+	 size_t		    i,
+	 size_t		    s,
+	 const string&	    n,
+	 type		    t,
+	 binding	    b,
+	 bool		    d,
+	 bool		    c,
+	 const version&	    ve,
+	 visibility	    vi,
+	 bool		    is_linux_string_cst = false,
+	 bool		    is_in_ksymtab = false);
 
   const environment*
   get_environment() const;
@@ -929,6 +931,12 @@  public:
   bool
   is_variable() const;
 
+  bool
+  is_in_ksymtab() const;
+
+  void
+  set_is_in_ksymtab(bool is_in_ksymtab);
+
   const elf_symbol_sptr
   get_main_symbol() const;
 
diff --git a/src/abg-ir.cc b/src/abg-ir.cc
index d1d02f3aad5d..22532cc87dfd 100644
--- a/src/abg-ir.cc
+++ b/src/abg-ir.cc
@@ -1310,6 +1310,7 @@  struct elf_symbol::priv
   //     STT_COMMON definition of that name that has the largest size.
   bool			is_common_;
   bool			is_linux_string_cst_;
+  bool			is_in_ksymtab_;
   elf_symbol_wptr	main_symbol_;
   elf_symbol_wptr	next_alias_;
   elf_symbol_wptr	next_common_instance_;
@@ -1324,20 +1325,22 @@  struct elf_symbol::priv
       visibility_(elf_symbol::DEFAULT_VISIBILITY),
       is_defined_(false),
       is_common_(false),
-      is_linux_string_cst_(false)
+      is_linux_string_cst_(false),
+      is_in_ksymtab_(false)
   {}
 
-  priv(const environment*		e,
-       size_t				i,
-       size_t				s,
-       const string&			n,
-       elf_symbol::type		t,
-       elf_symbol::binding		b,
-       bool				d,
-       bool				c,
-       const elf_symbol::version&	ve,
-       elf_symbol::visibility		vi,
-       bool				is_linux_string_cst)
+  priv(const environment*	  e,
+       size_t			  i,
+       size_t			  s,
+       const string&		  n,
+       elf_symbol::type		  t,
+       elf_symbol::binding	  b,
+       bool			  d,
+       bool			  c,
+       const elf_symbol::version& ve,
+       elf_symbol::visibility	  vi,
+       bool			  is_linux_string_cst,
+       bool			  is_in_ksymtab)
     : env_(e),
       index_(i),
       size_(s),
@@ -1348,7 +1351,8 @@  struct elf_symbol::priv
       visibility_(vi),
       is_defined_(d),
       is_common_(c),
-      is_linux_string_cst_(is_linux_string_cst)
+      is_linux_string_cst_(is_linux_string_cst),
+      is_in_ksymtab_(is_in_ksymtab)
   {
     if (!is_common_)
       is_common_ = type_ == COMMON_TYPE;
@@ -1394,19 +1398,30 @@  elf_symbol::elf_symbol()
 ///
 /// @param is_linux_string_cst true if the symbol is a Linux Kernel
 /// string constant defined in the __ksymtab_strings section.
-elf_symbol::elf_symbol(const environment*	e,
-		       size_t			i,
-		       size_t			s,
-		       const string&		n,
-		       type			t,
-		       binding			b,
-		       bool			d,
-		       bool			c,
-		       const version&		ve,
-		       visibility		vi,
-		       bool			is_linux_string_cst)
-  : priv_(new priv(e, i, s, n, t, b, d,
-		   c, ve, vi, is_linux_string_cst))
+elf_symbol::elf_symbol(const environment* e,
+		       size_t		  i,
+		       size_t		  s,
+		       const string&	  n,
+		       type		  t,
+		       binding		  b,
+		       bool		  d,
+		       bool		  c,
+		       const version&	  ve,
+		       visibility	  vi,
+		       bool		  is_linux_string_cst,
+		       bool		  is_in_ksymtab)
+  : priv_(new priv(e,
+		   i,
+		   s,
+		   n,
+		   t,
+		   b,
+		   d,
+		   c,
+		   ve,
+		   vi,
+		   is_linux_string_cst,
+		   is_in_ksymtab))
 {}
 
 /// Factory of instances of @ref elf_symbol.
@@ -1453,20 +1468,22 @@  elf_symbol::create()
 /// @return a (smart) pointer to a newly created instance of @ref
 /// elf_symbol.
 elf_symbol_sptr
-elf_symbol::create(const environment*	e,
-		   size_t		i,
-		   size_t		s,
-		   const string&	n,
-		   type		t,
-		   binding		b,
-		   bool		d,
-		   bool		c,
-		   const version&	ve,
-		   visibility		vi,
-		   bool		is_linux_string_cst)
-{
-  elf_symbol_sptr sym(new elf_symbol(e, i, s, n, t, b, d, c, ve,
-				     vi, is_linux_string_cst));
+elf_symbol::create(const environment* e,
+		   size_t	      i,
+		   size_t	      s,
+		   const string&      n,
+		   type		      t,
+		   binding	      b,
+		   bool		      d,
+		   bool		      c,
+		   const version&     ve,
+		   visibility	      vi,
+		   bool		      is_linux_string_cst,
+		   bool		      is_in_ksymtab)
+{
+  elf_symbol_sptr sym(new elf_symbol(e, i, s, n, t, b, d, c, ve, vi,
+				     is_linux_string_cst,
+				     is_in_ksymtab));
   sym->priv_->main_symbol_ = sym;
   return sym;
 }
@@ -1686,6 +1703,14 @@  bool
 elf_symbol::is_variable() const
 {return get_type() == OBJECT_TYPE || get_type() == TLS_TYPE;}
 
+bool
+elf_symbol::is_in_ksymtab() const
+{return priv_->is_in_ksymtab_;}
+
+void
+elf_symbol::set_is_in_ksymtab(bool is_in_ksymtab)
+{priv_->is_in_ksymtab_ = is_in_ksymtab;}
+
 /// @name Elf symbol aliases
 ///
 /// An alias A for an elf symbol S is a symbol that is defined at the