MIPS: Fix GOT page counter in multi-got links

Message ID 20180220175141.23620-1-james.cowgill@mips.com
State New
Headers show
Series
  • MIPS: Fix GOT page counter in multi-got links
Related show

Commit Message

James Cowgill Feb. 20, 2018, 5:51 p.m.
The record_got_page_entry function records and updates the maximum
number of GOT page entries which may be required by an object. In the
case where an existing GOT page entry was expanded, only the entry
belonging to output GOT would have its page count updated. This leaves
the entry belonging to the object GOT with the num_pages count of 1 it
was originally initialized with. Later on when GOTs are being merged in a
multi-got link, this causes the value of entry->num_pages in
add_got_page_entries to always be 1 and underestimates the number of pages
required for the new entry. This in turn leads to an assertion failure in
get_got_page_offset where we run out of pages.

Fix by obtaining the object's GOT entry unconditionally and not just
the first time it gets created. Now that entry2 is always valid, remove
the useless NULL checks.

I need someone to commit this for me.

gold/
2018-02-20  James Cowgill  <james.cowgill@mips.com>

	PR gold/22770
	* mips.cc (Mips_got_info::record_got_page_entry): Fetch existing
	page entries for the object's GOT.
---
 gold/mips.cc | 19 +++++++++----------
 1 file changed, 9 insertions(+), 10 deletions(-)

-- 
2.16.1

Comments

Cary Coutant April 5, 2018, 3:50 p.m. | #1
> The record_got_page_entry function records and updates the maximum

> number of GOT page entries which may be required by an object. In the

> case where an existing GOT page entry was expanded, only the entry

> belonging to output GOT would have its page count updated. This leaves

> the entry belonging to the object GOT with the num_pages count of 1 it

> was originally initialized with. Later on when GOTs are being merged in a

> multi-got link, this causes the value of entry->num_pages in

> add_got_page_entries to always be 1 and underestimates the number of pages

> required for the new entry. This in turn leads to an assertion failure in

> get_got_page_offset where we run out of pages.

>

> Fix by obtaining the object's GOT entry unconditionally and not just

> the first time it gets created. Now that entry2 is always valid, remove

> the useless NULL checks.

>

> I need someone to commit this for me.

>

> gold/

> 2018-02-20  James Cowgill  <james.cowgill@mips.com>

>

>         PR gold/22770

>         * mips.cc (Mips_got_info::record_got_page_entry): Fetch existing

>         page entries for the object's GOT.


Sorry, this patch slipped through my mail filters. I've committed the
patch on your behalf.

-cary
James Cowgill April 5, 2018, 4:10 p.m. | #2
Hi Cary,

On 05/04/18 16:50, Cary Coutant wrote:
>> The record_got_page_entry function records and updates the maximum

>> number of GOT page entries which may be required by an object. In the

>> case where an existing GOT page entry was expanded, only the entry

>> belonging to output GOT would have its page count updated. This leaves

>> the entry belonging to the object GOT with the num_pages count of 1 it

>> was originally initialized with. Later on when GOTs are being merged in a

>> multi-got link, this causes the value of entry->num_pages in

>> add_got_page_entries to always be 1 and underestimates the number of pages

>> required for the new entry. This in turn leads to an assertion failure in

>> get_got_page_offset where we run out of pages.

>>

>> Fix by obtaining the object's GOT entry unconditionally and not just

>> the first time it gets created. Now that entry2 is always valid, remove

>> the useless NULL checks.

>>

>> I need someone to commit this for me.

>>

>> gold/

>> 2018-02-20  James Cowgill  <james.cowgill@mips.com>

>>

>>         PR gold/22770

>>         * mips.cc (Mips_got_info::record_got_page_entry): Fetch existing

>>         page entries for the object's GOT.

> 

> Sorry, this patch slipped through my mail filters. I've committed the

> patch on your behalf.


Thanks, however there is a problem. When I originally submitted the
patch I thought the FSF copyright assignment for *@mips.com had been
sorted out but I have now learned that it was not (I think), so the
commit may have to be reverted.

Also, off list, someone pointed out some improvements to the patch (only
simplifications - the patch as committed still works). These ended up in
v3 which was attached to the bug report just now. Someone poked me to
submit the v3 which I was about to do until you committed the original
patch :)

James
Cary Coutant April 5, 2018, 4:48 p.m. | #3
> Thanks, however there is a problem. When I originally submitted the

> patch I thought the FSF copyright assignment for *@mips.com had been

> sorted out but I have now learned that it was not (I think), so the

> commit may have to be reverted.


These patch (the original and the revised one) are small enough, in my
judgement, that we don't need the assignment.

> Also, off list, someone pointed out some improvements to the patch (only

> simplifications - the patch as committed still works). These ended up in

> v3 which was attached to the bug report just now. Someone poked me to

> submit the v3 which I was about to do until you committed the original

> patch :)


I've reverted the original and applied your revised patch, which for
the record, is attached.

-cary

gold/
2018-04-05  James Cowgill  <james.cowgill@mips.com>

        PR gold/22770
        * mips.cc (Mips_got_info::record_got_page_entry): Don't insert
        Got_page_entry for object's GOT.
        (Mips_got_info::add_got_page_entries): Add all pages from from's GOT.
        Rename to add_got_page_count.
        (Got_page_entry): Remove num_pages.

Patch

diff --git a/gold/mips.cc b/gold/mips.cc
index 543a23462f..1107e30aeb 100644
--- a/gold/mips.cc
+++ b/gold/mips.cc
@@ -5795,13 +5795,14 @@  Mips_got_info<size, big_endian>::record_got_page_entry(
     this->got_page_entries_.insert(entry);
 
   // Add the same entry to the OBJECT's GOT.
-  Got_page_entry* entry2 = NULL;
+  Got_page_entry* entry2 = new Got_page_entry(*entry);
   Mips_got_info<size, big_endian>* g2 = object->get_or_create_got_info();
-  if (g2->got_page_entries_.find(entry) == g2->got_page_entries_.end())
-    {
-      entry2 = new Got_page_entry(*entry);
-      g2->got_page_entries_.insert(entry2);
-    }
+  typename Got_page_entry_set::iterator it2 =
+    g2->got_page_entries_.find(entry);
+  if (it2 != g2->got_page_entries_.end())
+    entry2 = *it2;
+  else
+    g2->got_page_entries_.insert(entry2);
 
   // Skip over ranges whose maximum extent cannot share a page entry
   // with ADDEND.
@@ -5822,8 +5823,7 @@  Mips_got_info<size, big_endian>::record_got_page_entry(
 
       *range_ptr = range;
       ++entry->num_pages;
-      if (entry2 != NULL)
-        ++entry2->num_pages;
+      ++entry2->num_pages;
       ++this->page_gotno_;
       ++g2->page_gotno_;
       return;
@@ -5852,8 +5852,7 @@  Mips_got_info<size, big_endian>::record_got_page_entry(
   if (old_pages != new_pages)
     {
       entry->num_pages += new_pages - old_pages;
-      if (entry2 != NULL)
-        entry2->num_pages += new_pages - old_pages;
+      entry2->num_pages += new_pages - old_pages;
       this->page_gotno_ += new_pages - old_pages;
       g2->page_gotno_ += new_pages - old_pages;
     }