Re: PATCH: PR gold/13507: Gold assumes GOT entry size is the same as ELF class size

[Ian Lance Taylor <iant at google dot com>]

"H.J. Lu" <hjl.tools@gmail.com> writes:

> 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.

But if you have 64-bit GOT entries, then when you call add_constant, you
should be able to pass a 64-bit value.


>> 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.

I agree.  That is exactly what I am suggesting.


> 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.

My goal is not the least intrusive change.  My goal is the change which
gives us the best source code going forward.  I'm fairly sure that
passing in the GOT size as a parameter is not the best change.

Ian