This is the mail archive of the
binutils@sourceware.org
mailing list for the binutils project.
Re: PATCH: PR gold/13507: Gold assumes GOT entry size is the same as ELF class size
On Fri, Dec 16, 2011 at 8:43 AM, Ian Lance Taylor <iant@google.com> wrote:
> "H.J. Lu" <hjl.tools@gmail.com> writes:
>
>> How do we deal with
>>
>> typedef typename elfcpp::Elf_types<size>::Elf_Addr Valtype;
>> typedef Output_data_reloc<elfcpp::SHT_REL, true, size, big_endian> Rel_dyn;
>> typedef Output_data_reloc<elfcpp::SHT_RELA, true, size, big_endian> Rela_dyn;
>>
>> <size> here should be the ELF class size.
>
> As far as I can see in the definition of Valtype the size should be the
> size of a GOT entry, not the ELF class size.
For 32bit ELF class, address is 32bit. When we use
elfcpp::Swap<64, big_endian>::writeval(pov, val);
to write 64bit GOT entry for 32bit ELF class, we can use ELF class size for
Valtype.
> The simple way to handle Rel_dyn and Rela_dyn would be to change
> Output_data_reloc<>::add_global and add_local to virtual functions, add
> them to Output_data_reloc_generic, and use Output_data_reloc_generic in
> Output_data_got. ?Perhaps that would lead to other issues, I'm not sure.
> If that works it would let us eliminate add_global_with_rela and
> friends.
>
> Of course since add_global and add_local take addresses it would be
> necessary for the generic versions to take uint64_t and for specific
> versions to take the appropriate size address, and use convert_types to
> make sure that there is no overflow. ?Actually it might be simpler to
> leave add_global and add_local as they are and add virtual functions
> with a different name to Output_data_reloc_generic.
>
Unless we can remove all dependencies on the ELF class from
Output_data_got, I don't see how it will work.
>
>>> In general I think the size of entries in the GOT ought to be a template
>>> parameter for Output_data_got.
>>>
>>> As far as I can tell your patch is incorrect, because you haven't
>>> changed Output_data_got<size, big_endian>::Got_entry::write. ?When that
>>> function calls elfcpp::Swap<size, big_endian>::writeval, it has to use
>>> the size of a GOT entry.
>>
>> How should it be fixed? Should we add another template parameter just
>> for GOT entry size?
>
> That would be another approach, yes.
Considering there are codes in Output_data_got like
Sized_symbol<size>* sgsym;
// This cast is a bit ugly. We don't want to put a
// virtual method in Symbol, because we want Symbol to be
// as small as possible.
sgsym = static_cast<Sized_symbol<size>*>(gsym);
val = sgsym->value();
add another template parameter for GOT entry size is less intrusive.
FWIW, I enclosed a new patch without adding a template parameter for
GOT entry size. If it isn't acceptable, I will work on a patch to add
a new template parameter, which will be added to most of template
classes.
Thanks.
--
H.J
---
.2011-12-16 H.J. Lu <hongjiu.lu@intel.com>
PR gold/13507
* arm.cc (Arm_output_data_got): Pass 32 to Output_data_got
constructor as got_entry_size.
* i386.cc (Target_i386::got_section): Likewise.
(Target_i386::got_section): Likewise.
* output.cc (write): Add a parameter, got_entry_size, and
use it to write GOT entry.
(add_global_pair_with_rel) Replace size with this->got_entry_size_.
(add_global_pair_with_rela): Likewise.
(add_local_pair_with_rel): Likewise.
(add_local_pair_with_rela): Likewise.
(add_got_entry): Likewise.
(add_got_entry_pair): Likewise.
(do_write): Likewise. Pass this->got_entry_size_ to write.
* output.h (Output_data_got): Add a parameter, got_entry_size,
to constructor. Replace size with with got_entry_size.
(reserve_slot): Replace size with this->got_entry_size_.
(got_offset): Likewise.
(write): Add parameter, got_entry_size.
(got_entry_size_): New.
* powerpc.cc (got_section): Pass size to Output_data_got
constructor as got_entry_size.
* sparc.cc (got_section): Likewise.
* x86_64.cc (got_section): Pass 64 to Output_data_got
constructor as got_entry_size.
(init_got_plt_for_update): Likewise.
2011-12-16 H.J. Lu <hongjiu.lu@intel.com>
PR gold/13507
* arm.cc (Arm_output_data_got): Pass 32 to Output_data_got
constructor as got_entry_size.
* i386.cc (Target_i386::got_section): Likewise.
(Target_i386::got_section): Likewise.
* output.cc (write): Add a parameter, got_entry_size, and
use it to write GOT entry.
(add_global_pair_with_rel) Replace size with this->got_entry_size_.
(add_global_pair_with_rela): Likewise.
(add_local_pair_with_rel): Likewise.
(add_local_pair_with_rela): Likewise.
(add_got_entry): Likewise.
(add_got_entry_pair): Likewise.
(do_write): Likewise. Pass this->got_entry_size_ to write.
* output.h (Output_data_got): Add a parameter, got_entry_size,
to constructor. Replace size with with got_entry_size.
(reserve_slot): Replace size with this->got_entry_size_.
(got_offset): Likewise.
(write): Add parameter, got_entry_size.
(got_entry_size_): New.
* powerpc.cc (got_section): Pass size to Output_data_got
constructor as got_entry_size.
* sparc.cc (got_section): Likewise.
* x86_64.cc (got_section): Pass 64 to Output_data_got
constructor as got_entry_size.
(init_got_plt_for_update): Likewise.
diff --git a/gold/arm.cc b/gold/arm.cc
index 72c3670..55b7595 100644
--- a/gold/arm.cc
+++ b/gold/arm.cc
@@ -1898,7 +1898,7 @@ class Arm_output_data_got : public Output_data_got<32, big_endian>
{
public:
Arm_output_data_got(Symbol_table* symtab, Layout* layout)
- : Output_data_got<32, big_endian>(), symbol_table_(symtab), layout_(layout)
+ : Output_data_got<32, big_endian>(32), symbol_table_(symtab), layout_(layout)
{ }
// Add a static entry for the GOT entry at OFFSET. GSYM is a global
diff --git a/gold/i386.cc b/gold/i386.cc
index 191a915..a21cd68 100644
--- a/gold/i386.cc
+++ b/gold/i386.cc
@@ -713,7 +713,7 @@ Target_i386::got_section(Symbol_table* symtab, Layout* layout)
{
gold_assert(symtab != NULL && layout != NULL);
- this->got_ = new Output_data_got<32, false>();
+ this->got_ = new Output_data_got<32, false>(32);
// When using -z now, we can treat .got.plt as a relro section.
// Without -z now, it is modified after program startup by lazy
@@ -768,7 +768,7 @@ Target_i386::got_section(Symbol_table* symtab, Layout* layout)
// If there are any TLSDESC relocations, they get GOT entries in
// .got.plt after the jump slot entries.
- this->got_tlsdesc_ = new Output_data_got<32, false>();
+ this->got_tlsdesc_ = new Output_data_got<32, false>(32);
layout->add_output_section_data(".got.plt", elfcpp::SHT_PROGBITS,
(elfcpp::SHF_ALLOC
| elfcpp::SHF_WRITE),
diff --git a/gold/output.cc b/gold/output.cc
index 7633c73..f3725e3 100644
--- a/gold/output.cc
+++ b/gold/output.cc
@@ -1347,7 +1347,8 @@ Output_data_group<size, big_endian>::do_write(Output_file* of)
template<int size, bool big_endian>
void
-Output_data_got<size, big_endian>::Got_entry::write(unsigned char* pov) const
+Output_data_got<size, big_endian>::Got_entry::write(unsigned int got_entry_size,
+ unsigned char* pov) const
{
Valtype val = 0;
@@ -1402,7 +1403,17 @@ Output_data_got<size, big_endian>::Got_entry::write(unsigned char* pov) const
break;
}
- elfcpp::Swap<size, big_endian>::writeval(pov, val);
+ switch (got_entry_size)
+ {
+ case 4:
+ elfcpp::Swap<32, big_endian>::writeval(pov, val);
+ break;
+ case 8:
+ elfcpp::Swap<64, big_endian>::writeval(pov, val);
+ break;
+ default:
+ gold_unreachable();
+ }
}
// Output_data_got methods.
@@ -1495,7 +1506,8 @@ Output_data_got<size, big_endian>::add_global_pair_with_rel(
rel_dyn->add_global(gsym, r_type_1, this, got_offset);
if (r_type_2 != 0)
- rel_dyn->add_global(gsym, r_type_2, this, got_offset + size / 8);
+ rel_dyn->add_global(gsym, r_type_2, this,
+ got_offset + this->got_entry_size_);
}
template<int size, bool big_endian>
@@ -1515,7 +1527,8 @@ Output_data_got<size, big_endian>::add_global_pair_with_rela(
rela_dyn->add_global(gsym, r_type_1, this, got_offset, 0);
if (r_type_2 != 0)
- rela_dyn->add_global(gsym, r_type_2, this, got_offset + size / 8, 0);
+ rela_dyn->add_global(gsym, r_type_2, this,
+ got_offset + this->got_entry_size_, 0);
}
// Add an entry for a local symbol to the GOT. This returns true if
@@ -1618,7 +1631,8 @@ Output_data_got<size, big_endian>::add_local_pair_with_rel(
rel_dyn->add_output_section(os, r_type_1, this, got_offset);
if (r_type_2 != 0)
- rel_dyn->add_output_section(os, r_type_2, this, got_offset + size / 8);
+ rel_dyn->add_output_section(os, r_type_2, this,
+ got_offset + this->got_entry_size_);
}
template<int size, bool big_endian>
@@ -1643,7 +1657,8 @@ Output_data_got<size, big_endian>::add_local_pair_with_rela(
rela_dyn->add_output_section(os, r_type_1, this, got_offset, 0);
if (r_type_2 != 0)
- rela_dyn->add_output_section(os, r_type_2, this, got_offset + size / 8, 0);
+ rela_dyn->add_output_section(os, r_type_2, this,
+ got_offset + this->got_entry_size_, 0);
}
// Reserve a slot in the GOT for a local symbol or the second slot of a pair.
@@ -1679,7 +1694,7 @@ template<int size, bool big_endian>
void
Output_data_got<size, big_endian>::do_write(Output_file* of)
{
- const int add = size / 8;
+ const int add = this->got_entry_size_;
const off_t off = this->offset();
const off_t oview_size = this->data_size();
@@ -1690,7 +1705,7 @@ Output_data_got<size, big_endian>::do_write(Output_file* of)
p != this->entries_.end();
++p)
{
- p->write(pov);
+ p->write(this->got_entry_size_, pov);
pov += add;
}
@@ -1717,11 +1732,12 @@ Output_data_got<size, big_endian>::add_got_entry(Got_entry got_entry)
else
{
// For an incremental update, find an available slot.
- off_t got_offset = this->free_list_.allocate(size / 8, size / 8, 0);
+ off_t got_offset = this->free_list_.allocate(this->got_entry_size_,
+ this->got_entry_size_, 0);
if (got_offset == -1)
gold_fallback(_("out of patch space (GOT);"
" relink with --incremental-full"));
- unsigned int got_index = got_offset / (size / 8);
+ unsigned int got_index = got_offset / this->got_entry_size_;
gold_assert(got_index < this->entries_.size());
this->entries_[got_index] = got_entry;
return static_cast<unsigned int>(got_offset);
@@ -1747,11 +1763,12 @@ Output_data_got<size, big_endian>::add_got_entry_pair(Got_entry got_entry_1,
else
{
// For an incremental update, find an available pair of slots.
- off_t got_offset = this->free_list_.allocate(2 * size / 8, size / 8, 0);
+ off_t got_offset = this->free_list_.allocate(2 * this->got_entry_size_,
+ this->got_entry_size_, 0);
if (got_offset == -1)
gold_fallback(_("out of patch space (GOT);"
" relink with --incremental-full"));
- unsigned int got_index = got_offset / (size / 8);
+ unsigned int got_index = got_offset / this->got_entry_size_;
gold_assert(got_index < this->entries_.size());
this->entries_[got_index] = got_entry_1;
this->entries_[got_index + 1] = got_entry_2;
diff --git a/gold/output.h b/gold/output.h
index e704213..81ca494 100644
--- a/gold/output.h
+++ b/gold/output.h
@@ -1983,19 +1983,19 @@ class Output_data_got : public Output_section_data_build
typedef Output_data_reloc<elfcpp::SHT_REL, true, size, big_endian> Rel_dyn;
typedef Output_data_reloc<elfcpp::SHT_RELA, true, size, big_endian> Rela_dyn;
- Output_data_got()
- : Output_section_data_build(Output_data::default_alignment_for_size(size)),
- entries_(), free_list_()
+ Output_data_got(int got_entry_size)
+ : Output_section_data_build(Output_data::default_alignment_for_size(got_entry_size)),
+ got_entry_size_(got_entry_size / 8), entries_(), free_list_()
{ }
- Output_data_got(off_t data_size)
+ Output_data_got(int got_entry_size, off_t data_size)
: Output_section_data_build(data_size,
- Output_data::default_alignment_for_size(size)),
- entries_(), free_list_()
+ Output_data::default_alignment_for_size(got_entry_size)),
+ got_entry_size_(got_entry_size / 8), entries_(), free_list_()
{
// For an incremental update, we have an existing GOT section.
// Initialize the list of entries and the free list.
- this->entries_.resize(data_size / (size / 8));
+ this->entries_.resize(data_size / (got_entry_size / 8));
this->free_list_.init(data_size, false);
}
@@ -2083,7 +2083,8 @@ class Output_data_got : public Output_section_data_build
// Reserve a slot in the GOT.
void
reserve_slot(unsigned int i)
- { this->free_list_.remove(i * size / 8, (i + 1) * size / 8); }
+ { this->free_list_.remove(i * this->got_entry_size_,
+ (i + 1) * this->got_entry_size_); }
// Reserve a slot in the GOT for a local symbol.
void
@@ -2139,7 +2140,7 @@ class Output_data_got : public Output_section_data_build
// Write the GOT entry to an output view.
void
- write(unsigned char* pov) const;
+ write(unsigned int got_entry_size, unsigned char* pov) const;
private:
enum
@@ -2178,7 +2179,7 @@ class Output_data_got : public Output_section_data_build
// Return the offset into the GOT of GOT entry I.
unsigned int
got_offset(unsigned int i) const
- { return i * (size / 8); }
+ { return i * this->got_entry_size_; }
// Return the offset into the GOT of the last entry added.
unsigned int
@@ -2190,6 +2191,9 @@ class Output_data_got : public Output_section_data_build
set_got_size()
{ this->set_current_data_size(this->got_offset(this->entries_.size())); }
+ // GOT entry size in bytes.
+ unsigned int got_entry_size_;
+
// The list of GOT entries.
Got_entries entries_;
diff --git a/gold/powerpc.cc b/gold/powerpc.cc
index 62a17ca..948333f 100644
--- a/gold/powerpc.cc
+++ b/gold/powerpc.cc
@@ -765,7 +765,7 @@ Target_powerpc<size, big_endian>::got_section(Symbol_table* symtab,
{
gold_assert(symtab != NULL && layout != NULL);
- this->got_ = new Output_data_got<size, big_endian>();
+ this->got_ = new Output_data_got<size, big_endian>(size);
layout->add_output_section_data(".got", elfcpp::SHT_PROGBITS,
elfcpp::SHF_ALLOC | elfcpp::SHF_WRITE,
diff --git a/gold/sparc.cc b/gold/sparc.cc
index 12e1dee..4d88a86 100644
--- a/gold/sparc.cc
+++ b/gold/sparc.cc
@@ -1075,7 +1075,7 @@ Target_sparc<size, big_endian>::got_section(Symbol_table* symtab,
{
gold_assert(symtab != NULL && layout != NULL);
- this->got_ = new Output_data_got<size, big_endian>();
+ this->got_ = new Output_data_got<size, big_endian>(size);
layout->add_output_section_data(".got", elfcpp::SHT_PROGBITS,
(elfcpp::SHF_ALLOC
diff --git a/gold/x86_64.cc b/gold/x86_64.cc
index 0762926..5a7a807 100644
--- a/gold/x86_64.cc
+++ b/gold/x86_64.cc
@@ -849,7 +849,7 @@ Target_x86_64::got_section(Symbol_table* symtab, Layout* layout)
? ORDER_RELRO
: ORDER_NON_RELRO_FIRST);
- this->got_ = new Output_data_got<64, false>();
+ this->got_ = new Output_data_got<64, false>(64);
layout->add_output_section_data(".got", elfcpp::SHT_PROGBITS,
(elfcpp::SHF_ALLOC
@@ -893,7 +893,7 @@ Target_x86_64::got_section(Symbol_table* symtab, Layout* layout)
// If there are any TLSDESC relocations, they get GOT entries in
// .got.plt after the jump slot and IRELATIVE entries.
- this->got_tlsdesc_ = new Output_data_got<64, false>();
+ this->got_tlsdesc_ = new Output_data_got<64, false>(64);
layout->add_output_section_data(".got.plt", elfcpp::SHT_PROGBITS,
(elfcpp::SHF_ALLOC
| elfcpp::SHF_WRITE),
@@ -1471,7 +1471,7 @@ Target_x86_64::init_got_plt_for_update(Symbol_table* symtab,
{
gold_assert(this->got_ == NULL);
- this->got_ = new Output_data_got<64, false>(got_count * 8);
+ this->got_ = new Output_data_got<64, false>(64, got_count * 8);
layout->add_output_section_data(".got", elfcpp::SHT_PROGBITS,
(elfcpp::SHF_ALLOC
| elfcpp::SHF_WRITE),
@@ -1499,7 +1499,7 @@ Target_x86_64::init_got_plt_for_update(Symbol_table* symtab,
// If there are any TLSDESC relocations, they get GOT entries in
// .got.plt after the jump slot entries.
// FIXME: Get the count for TLSDESC entries.
- this->got_tlsdesc_ = new Output_data_got<64, false>(0);
+ this->got_tlsdesc_ = new Output_data_got<64, false>(64, 0);
layout->add_output_section_data(".got.plt", elfcpp::SHT_PROGBITS,
elfcpp::SHF_ALLOC | elfcpp::SHF_WRITE,
this->got_tlsdesc_,