[GOLD] Got_offset_list: addend field

Message ID YTBE8iKfttN+uRNh@squeak.grove.modra.org
State New
Headers show
Series
  • [GOLD] Got_offset_list: addend field
Related show

Commit Message

Alan Modra via Binutils Sept. 2, 2021, 3:28 a.m.
This is the first in a series of patches aimed at supporting GOT
entries against symbol plus addend generally for PowerPC64 rather than
just section symbol plus addend as gold has currently.

This patch adds an addend field to Got_offset_list, so that both local
and global symbols can have GOT entries with addend.  Note the FIXME
in incremental.cc, which will need attention if powerpc is to ever
support incremental linking.

As per the recommendation in
https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rf-default-args
the changes to symtab.h Symbol methods could be done with what looks
like a good use of default arguments but they seem to be against the
gold maintainers' coding practice.  So instead we get more overloads.

Tested with a power10 gcc-11 bootstrap and regression test where
ld.gold was the default linker.  The series cures an enormous number
of testsuite failures (4656) without introducing regressions other
than some in the guality testsuite.

	PR 28192
	* object.h (Got_offset_list): Add addend_ field, init in both
	constructors.  Adjust all accessors to suit.
	(Sized_relobj::do_local_has_got_offset): Adjust to suit.
	(Sized_relobj::do_local_got_offset): Likewise.
	(Sized_relobj::do_set_local_got_offset): Likewise.
	* symtab.h (Symbol::has_got_offset): New overload with addend
	param.  Make old overload call this one.
	(Symbol::got_offset, Symbol::set_got_offset): Likewise.
	* incremental.cc (Local_got_offset_visitor::visit): Add unused
	uint64_t parameter with FIXME.
	(Global_got_offset_visitor::visit): Add unused uint64_t parameter.


-- 
Alan Modra
Australia Development Lab, IBM

Comments

Alan Modra via Binutils Sept. 3, 2021, 1:13 a.m. | #1
> This is the first in a series of patches aimed at supporting GOT

> entries against symbol plus addend generally for PowerPC64 rather than

> just section symbol plus addend as gold has currently.

>

> This patch adds an addend field to Got_offset_list, so that both local

> and global symbols can have GOT entries with addend.  Note the FIXME

> in incremental.cc, which will need attention if powerpc is to ever

> support incremental linking.


Do you think you'd ever want incremental linking on powerpc? Frankly,
the effort for just the one target platform was pretty high, the
maintenance on it is burdensome, and I'm tempted to deprecate it and
rip it out at some point in the future.

> As per the recommendation in

> https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rf-default-args

> the changes to symtab.h Symbol methods could be done with what looks

> like a good use of default arguments but they seem to be against the

> gold maintainers' coding practice.  So instead we get more overloads.


For the record, I've got nothing against default parameter values, as
long as they're obvious default values that shouldn't ever need to be
changed. I'm not sure what Ian's position was, but he did use a
default parameter value in at least one place (Descriptors::open in
descriptors.h). I wouldn't be surprised to find places where I
probably should have used a default parameter value instead of an
overload.

> Tested with a power10 gcc-11 bootstrap and regression test where

> ld.gold was the default linker.  The series cures an enormous number

> of testsuite failures (4656) without introducing regressions other

> than some in the guality testsuite.

>

>         PR 28192

>         * object.h (Got_offset_list): Add addend_ field, init in both

>         constructors.  Adjust all accessors to suit.

>         (Sized_relobj::do_local_has_got_offset): Adjust to suit.

>         (Sized_relobj::do_local_got_offset): Likewise.

>         (Sized_relobj::do_set_local_got_offset): Likewise.

>         * symtab.h (Symbol::has_got_offset): New overload with addend

>         param.  Make old overload call this one.

>         (Symbol::got_offset, Symbol::set_got_offset): Likewise.

>         * incremental.cc (Local_got_offset_visitor::visit): Add unused

>         uint64_t parameter with FIXME.

>         (Global_got_offset_visitor::visit): Add unused uint64_t parameter.


OK, with or without your suggestion to use a default parameter value
for has_got_offset.

Thanks!

-cary
Alan Modra via Binutils Sept. 3, 2021, 3:22 a.m. | #2
On Thu, Sep 02, 2021 at 06:13:57PM -0700, Cary Coutant wrote:
> > This is the first in a series of patches aimed at supporting GOT

> > entries against symbol plus addend generally for PowerPC64 rather than

> > just section symbol plus addend as gold has currently.

> >

> > This patch adds an addend field to Got_offset_list, so that both local

> > and global symbols can have GOT entries with addend.  Note the FIXME

> > in incremental.cc, which will need attention if powerpc is to ever

> > support incremental linking.

> 

> Do you think you'd ever want incremental linking on powerpc?


No, I don't think so.  And I'll argue against it if someone tries to
talk me into implementing it.  :-)

> Frankly,

> the effort for just the one target platform was pretty high, the

> maintenance on it is burdensome, and I'm tempted to deprecate it and

> rip it out at some point in the future.

> 

> > As per the recommendation in

> > https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rf-default-args

> > the changes to symtab.h Symbol methods could be done with what looks

> > like a good use of default arguments but they seem to be against the

> > gold maintainers' coding practice.  So instead we get more overloads.

> 

> For the record, I've got nothing against default parameter values, as

> long as they're obvious default values that shouldn't ever need to be

> changed. I'm not sure what Ian's position was,


There's this:
https://sourceware.org/pipermail/binutils/2010-October/069026.html

> but he did use a

> default parameter value in at least one place (Descriptors::open in

> descriptors.h).


Heh.

> OK, with or without your suggestion to use a default parameter value

> for has_got_offset.


Thanks.  I wrote the patch initially with default parameter values so
it will be easy to go back to that.

-- 
Alan Modra
Australia Development Lab, IBM
Alan Modra via Binutils Sept. 3, 2021, 6:24 p.m. | #3
> > > As per the recommendation in

> > > https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rf-default-args

> > > the changes to symtab.h Symbol methods could be done with what looks

> > > like a good use of default arguments but they seem to be against the

> > > gold maintainers' coding practice.  So instead we get more overloads.

> >

> > For the record, I've got nothing against default parameter values, as

> > long as they're obvious default values that shouldn't ever need to be

> > changed. I'm not sure what Ian's position was,

>

> There's this:

> https://sourceware.org/pipermail/binutils/2010-October/069026.html


Ah, good find!

I think I'd take exception to that, as introducing a new version of
the function with the same name is exactly what adding an overload
does.

-cary

Patch

diff --git a/gold/incremental.cc b/gold/incremental.cc
index ab258dd1896..52941985e6f 100644
--- a/gold/incremental.cc
+++ b/gold/incremental.cc
@@ -1848,7 +1848,7 @@  class Local_got_offset_visitor : public Got_offset_list::Visitor
   { }
 
   void
-  visit(unsigned int got_type, unsigned int got_offset)
+  visit(unsigned int got_type, unsigned int got_offset, uint64_t)
   {
     unsigned int got_index = got_offset / this->info_.got_entry_size;
     gold_assert(got_index < this->info_.got_count);
@@ -1860,6 +1860,12 @@  class Local_got_offset_visitor : public Got_offset_list::Visitor
     unsigned char* pov = this->info_.got_desc_p + got_index * 8;
     elfcpp::Swap<32, big_endian>::writeval(pov, this->info_.sym_index);
     elfcpp::Swap<32, big_endian>::writeval(pov + 4, this->info_.input_index);
+    // FIXME: the uint64_t addend should be written here if powerpc64
+    // sym+addend got entries are to be supported, with similar changes
+    // to Global_got_offset_visitor and support to read them back in
+    // do_process_got_plt.
+    // FIXME: don't we need this for section symbol plus addend anyway?
+    // (See 2015-12-03 commit 7ef8ae7c5f35)
   }
 
  private:
@@ -1879,7 +1885,7 @@  class Global_got_offset_visitor : public Got_offset_list::Visitor
   { }
 
   void
-  visit(unsigned int got_type, unsigned int got_offset)
+  visit(unsigned int got_type, unsigned int got_offset, uint64_t)
   {
     unsigned int got_index = got_offset / this->info_.got_entry_size;
     gold_assert(got_index < this->info_.got_count);
diff --git a/gold/object.h b/gold/object.h
index 38f4a2fd3ff..66e565c1b4c 100644
--- a/gold/object.h
+++ b/gold/object.h
@@ -214,11 +214,13 @@  class Got_offset_list
 {
  public:
   Got_offset_list()
-    : got_type_(-1U), got_offset_(0), got_next_(NULL)
+    : got_type_(-1U), got_offset_(0), addend_(0), got_next_(NULL)
   { }
 
-  Got_offset_list(unsigned int got_type, unsigned int got_offset)
-    : got_type_(got_type), got_offset_(got_offset), got_next_(NULL)
+  Got_offset_list(unsigned int got_type, unsigned int got_offset,
+		  uint64_t addend)
+    : got_type_(got_type), got_offset_(got_offset), addend_(addend),
+      got_next_(NULL)
   { }
 
   ~Got_offset_list()
@@ -236,29 +238,31 @@  class Got_offset_list
   {
     this->got_type_ = -1U;
     this->got_offset_ = 0;
+    this->addend_ = 0;
     this->got_next_ = NULL;
   }
 
   // Set the offset for the GOT entry of type GOT_TYPE.
   void
-  set_offset(unsigned int got_type, unsigned int got_offset)
+  set_offset(unsigned int got_type, unsigned int got_offset, uint64_t addend)
   {
     if (this->got_type_ == -1U)
       {
         this->got_type_ = got_type;
         this->got_offset_ = got_offset;
+        this->addend_ = addend;
       }
     else
       {
         for (Got_offset_list* g = this; g != NULL; g = g->got_next_)
           {
-            if (g->got_type_ == got_type)
+            if (g->got_type_ == got_type && g->addend_ == addend)
               {
                 g->got_offset_ = got_offset;
                 return;
               }
           }
-        Got_offset_list* g = new Got_offset_list(got_type, got_offset);
+        Got_offset_list* g = new Got_offset_list(got_type, got_offset, addend);
         g->got_next_ = this->got_next_;
         this->got_next_ = g;
       }
@@ -266,11 +270,11 @@  class Got_offset_list
 
   // Return the offset for a GOT entry of type GOT_TYPE.
   unsigned int
-  get_offset(unsigned int got_type) const
+  get_offset(unsigned int got_type, uint64_t addend) const
   {
     for (const Got_offset_list* g = this; g != NULL; g = g->got_next_)
       {
-        if (g->got_type_ == got_type)
+        if (g->got_type_ == got_type && g->addend_ == addend)
           return g->got_offset_;
       }
     return -1U;
@@ -297,7 +301,7 @@  class Got_offset_list
     { }
 
     virtual void
-    visit(unsigned int, unsigned int) = 0;
+    visit(unsigned int, unsigned int, uint64_t) = 0;
   };
 
   // Loop over all GOT offset entries, calling a visitor class V for each.
@@ -307,12 +311,13 @@  class Got_offset_list
     if (this->got_type_ == -1U)
       return;
     for (const Got_offset_list* g = this; g != NULL; g = g->got_next_)
-      v->visit(g->got_type_, g->got_offset_);
+      v->visit(g->got_type_, g->got_offset_, g->addend_);
   }
 
  private:
   unsigned int got_type_;
   unsigned int got_offset_;
+  uint64_t addend_;
   Got_offset_list* got_next_;
 };
 
@@ -2134,7 +2139,7 @@  class Sized_relobj : public Relobj
     Local_got_offsets::const_iterator p =
         this->local_got_offsets_.find(key);
     return (p != this->local_got_offsets_.end()
-            && p->second->get_offset(got_type) != -1U);
+            && p->second->get_offset(got_type, addend) != -1U);
   }
 
   // Return the GOT offset of type GOT_TYPE of the local symbol
@@ -2147,7 +2152,7 @@  class Sized_relobj : public Relobj
     Local_got_offsets::const_iterator p =
         this->local_got_offsets_.find(key);
     gold_assert(p != this->local_got_offsets_.end());
-    unsigned int off = p->second->get_offset(got_type);
+    unsigned int off = p->second->get_offset(got_type, addend);
     gold_assert(off != -1U);
     return off;
   }
@@ -2162,10 +2167,10 @@  class Sized_relobj : public Relobj
     Local_got_offsets::const_iterator p =
         this->local_got_offsets_.find(key);
     if (p != this->local_got_offsets_.end())
-      p->second->set_offset(got_type, got_offset);
+      p->second->set_offset(got_type, got_offset, addend);
     else
       {
-        Got_offset_list* g = new Got_offset_list(got_type, got_offset);
+	Got_offset_list* g = new Got_offset_list(got_type, got_offset, addend);
         std::pair<Local_got_offsets::iterator, bool> ins =
             this->local_got_offsets_.insert(std::make_pair(key, g));
         gold_assert(ins.second);
diff --git a/gold/symtab.h b/gold/symtab.h
index 104429a5af1..45e0842f593 100644
--- a/gold/symtab.h
+++ b/gold/symtab.h
@@ -427,23 +427,35 @@  class Symbol
 
   // Return whether this symbol has an entry in the GOT section.
   // For a TLS symbol, this GOT entry will hold its tp-relative offset.
+  bool
+  has_got_offset(unsigned int got_type, uint64_t addend) const
+  { return this->got_offsets_.get_offset(got_type, addend) != -1U; }
+
   bool
   has_got_offset(unsigned int got_type) const
-  { return this->got_offsets_.get_offset(got_type) != -1U; }
+  { return this->has_got_offset(got_type, 0); }
 
   // Return the offset into the GOT section of this symbol.
   unsigned int
-  got_offset(unsigned int got_type) const
+  got_offset(unsigned int got_type, uint64_t addend) const
   {
-    unsigned int got_offset = this->got_offsets_.get_offset(got_type);
+    unsigned int got_offset = this->got_offsets_.get_offset(got_type, addend);
     gold_assert(got_offset != -1U);
     return got_offset;
   }
 
+  unsigned int
+  got_offset(unsigned int got_type) const
+  { return this->got_offset(got_type, 0); }
+
   // Set the GOT offset of this symbol.
+  void
+  set_got_offset(unsigned int got_type, unsigned int got_offset, uint64_t addend)
+  { this->got_offsets_.set_offset(got_type, got_offset, addend); }
+
   void
   set_got_offset(unsigned int got_type, unsigned int got_offset)
-  { this->got_offsets_.set_offset(got_type, got_offset); }
+  { this->set_got_offset(got_type, got_offset, 0); }
 
   // Return the GOT offset list.
   const Got_offset_list*