Re: [PATCH] Remove divide from _ELF_DYNAMIC_DO_RELOC

[Chung-Lin Tang <cltang at codesourcery dot com>]

Hi Carlos,
I got your okay on this patch late in the 2.20 cycle, but it's been a
while, so I'm notifying here first. If you don't object, I plan to
commit this tomorrow.

Thanks,
Chung-Lin

On 2014/7/16 04:30 AM, Carlos O'Donell wrote:
> On 07/15/2014 01:53 AM, Chung-Lin Tang wrote:
>> Hi Carlos, thanks for the review, and sorry about the long delay to reply.
>>
>> On 14/5/23 9:59 PM, Carlos O'Donell wrote:
>>> On 04/11/2014 05:37 AM, Chung-Lin Tang wrote:
>>>> 2014-04-11  Chung-Lin Tang  <cltang@codesourcery.com>
>>>>
>>>> 	* elf/dynamic-link.h (_ELF_DYNAMIC_DO_RELOC): Remove MIN() and
>>>> 	assign raw DT_REL[A]COUNT value to ranges[0].nrelative.
> 
> OK to checkin.
> 
>>>> diff --git a/elf/dynamic-link.h b/elf/dynamic-link.h
>>>> index 7b3e295..34ef88a 100644
>>>> --- a/elf/dynamic-link.h
>>>> +++ b/elf/dynamic-link.h
>>>> @@ -122,8 +122,7 @@ elf_machine_lazy_rel (struct link_map *map,
>>>>  	ranges[0].size = (map)->l_info[DT_##RELOC##SZ]->d_un.d_val;	      \
>>>>  	if (map->l_info[VERSYMIDX (DT_##RELOC##COUNT)] != NULL)		      \
>>>>  	  ranges[0].nrelative						      \
>>>> -	    = MIN (map->l_info[VERSYMIDX (DT_##RELOC##COUNT)]->d_un.d_val,    \
>>>> -		   ranges[0].size / sizeof (ElfW(reloc)));		      \
>>>> +	    = map->l_info[VERSYMIDX (DT_##RELOC##COUNT)]->d_un.d_val;	      \
> 
> I want to note that the resulting code is less robust against binutils
> bugs where the COUNT is more than the number of relocations in the list.
> However I would rather this fail here quickly than magically select
> DT_RELSZ/sizeof(ElfW(reloc)) as some random fallback e.g. all the relocs
> are relative.
> 
>>>>        }									      \
>>>>      if ((map)->l_info[DT_PLTREL]					      \
>>>>  	&& (!test_rel || (map)->l_info[DT_PLTREL]->d_un.d_val == DT_##RELOC)) \
>>>
>>> Please add a top-level comment saying that 'COUNT', when present, overrides the
>>> use of 'SZ' to compute the size of the range.
>>
>> When 'COUNT' is not present, nrelative seems to be simply initialized as
>> zero. I don't think it has to do with range here.
> 
> You are correct, I incorrectly remembered the purpose of DT_RELCOUNT which is
> to handle the combined R_*_RELATIVE relocations more optimally in one block.
> 
>>> OK with that.
>>>
>>> Should we assert if SZ/sizeof(ElfW(reloc)) != COUNT?
>>
>> Please, please don't introduce another divide, defeating my original
>> purpose...
> 
> Given that I incorrectly remembered by DT_RELCOUNT was for this recommendation
> is not correct anyway.
> 
> Therefore your patch as-is can be checked in. Please ask Allan McRae for permission
> since this is a freeze period and we are fixing bugs, documentation and other things.
> 
> Cheers,
> Carlos.
>