This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH] Remove divide from _ELF_DYNAMIC_DO_RELOC
- From: Chung-Lin Tang <cltang at codesourcery dot com>
- To: Carlos O'Donell <carlos at redhat dot com>, Chung-Lin Tang <chunglin_tang at mentor dot com>, <libc-alpha at sourceware dot org>
- Cc: David Miller <davem at davemloft dot net>, "Joseph S. Myers" <joseph at codesourcery dot com>, OndÅej BÃlka <neleai at seznam dot cz>
- Date: Sat, 10 Jan 2015 01:43:13 +0800
- Subject: Re: [PATCH] Remove divide from _ELF_DYNAMIC_DO_RELOC
- Authentication-results: sourceware.org; auth=none
- References: <5347B7EE dot 6020507 at mentor dot com> <537F542B dot 5040409 at redhat dot com> <53C4C1D5 dot 40002 at mentor dot com> <53C58F53 dot 8080506 at redhat dot com> <54AEACB7 dot 3070609 at codesourcery dot com>
On 2015/1/9 12:13 AM, Chung-Lin Tang wrote:
> 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
Patch committed after a re-test on x86_64 and powerpc64.
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.
>>
>