Remove divide from _ELF_DYNAMIC_DO_RELOC

  Hi, this follows a part of my Nios II port submission, where there was a
patch to add a target macro to rtld.c to provide an inline divide during
RTLD_BOOTSTRAP:
https://sourceware.org/ml/libc-alpha/2014-03/msg00931.html

Joseph's pointers to the older Xtensa port submission in April 2007:
https://sourceware.org/ml/libc-alpha/2007-04/msg00002.html
which had exactly the same problem, prompted me to investigate into the
role of this divide in the dynamic relocation code in the current trunk.

First of all, as the xtensa thread points out, the original context
for using this MIN(nrelative,relsize/sizeof (ElfW(Rel))) expression is:
https://www.sourceware.org/ml/libc-alpha/2002-06/msg00150.html

which as this old Jun-2002 thread began with, is due to not initializing
ranges[2] in _ELF_DYNAMIC_DO_RELOC. This is a part of the code handling
the third range of the ELF_MACHINE_PLTREL_OVERLAP case, a case which has
been deleted since April 2012:
https://www.sourceware.org/ml/libc-alpha/2012-04/msg00103.html

Also, since this 2011 patch:
https://sourceware.org/git/?p=glibc.git;a=commitdiff;h=e453f6cd0ccdd64a3f5f156e2c5f70085e9289e7

which abstracted out nrelative as a parameter, and its calculation only
applied to for ranges[0], the need for doing this form of safe-guarding
seems no longer true. IIUC, if binutils always gets this correct,
DT_REL[A]COUNT <= relsize/sizeof(ElfW(reloc)) should always
hold.

The attached patch assigns the raw DT_REL[A]COUNT value to
ranges[0].nrelative, removing the divide and MIN().  We might add other
checks in elf_dynamic_do_Rel() to ensure r+=nrelative is really within
the range, though I think this is probably not needed.

I've CCed some of the people that appeared in that 2012 PLTREL overlap
thread. Can anybody more familiar with this part of ld.so comment on if
this is correct?  In terms of improving performance this is probably an
insignificant change, however for targets like Nios II (and xtensa)
avoiding the need for a __udivsi3 routine really lets us avoid ugly hacks.

FWIW, I've ran tests on a i686-linux machine with no regressions, though
this might not be a valid corner case check.

(note: on the question of why GCC did not transform the divide by
constant 12 into a multiply form, which GCC is indeed capable of, is
still to be investigated, but is a separate compiler problem)

Thanks,
Chung-Lin

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.
  

From: Chung-Lin Tang <chunglin_tang@mentor.com>
Date: Fri, 11 Apr 2014 17:37:50 +0800

> I've CCed some of the people that appeared in that 2012 PLTREL overlap
> thread. Can anybody more familiar with this part of ld.so comment on if
> this is correct?

The change looks fine to me.
  

On 14/4/13 5:43 AM, David Miller wrote:
> From: Chung-Lin Tang <chunglin_tang@mentor.com>
> Date: Fri, 11 Apr 2014 17:37:50 +0800
> 
>> I've CCed some of the people that appeared in that 2012 PLTREL overlap
>> thread. Can anybody more familiar with this part of ld.so comment on if
>> this is correct?
> 
> The change looks fine to me.
> 

Hi David, thanks for taking a look.

Carlos, do you agree? I see you're listed as reviewer of the dynamic
linker, so I think I have to have your approval.

And please, if approved, commit/push it for me. I don't have apply
rights for glibc yet.

Thanks,
Chung-Lin
  

On 14/4/14 5:19 PM, Chung-Lin Tang wrote:
> On 14/4/13 5:43 AM, David Miller wrote:
>> From: Chung-Lin Tang <chunglin_tang@mentor.com>
>> Date: Fri, 11 Apr 2014 17:37:50 +0800
>>
>>> I've CCed some of the people that appeared in that 2012 PLTREL overlap
>>> thread. Can anybody more familiar with this part of ld.so comment on if
>>> this is correct?
>>
>> The change looks fine to me.
>>
> 
> Hi David, thanks for taking a look.
> 
> Carlos, do you agree? I see you're listed as reviewer of the dynamic
> linker, so I think I have to have your approval.
> 
> And please, if approved, commit/push it for me. I don't have apply
> rights for glibc yet.
> 
> Thanks,
> Chung-Lin
> 

Ping?
  

On 14/4/25 下午9:21, Chung-Lin Tang wrote:
> On 14/4/14 5:19 PM, Chung-Lin Tang wrote:
>> On 14/4/13 5:43 AM, David Miller wrote:
>>> From: Chung-Lin Tang <chunglin_tang@mentor.com>
>>> Date: Fri, 11 Apr 2014 17:37:50 +0800
>>>
>>>> I've CCed some of the people that appeared in that 2012 PLTREL overlap
>>>> thread. Can anybody more familiar with this part of ld.so comment on if
>>>> this is correct?
>>>
>>> The change looks fine to me.
>>>
>>
>> Hi David, thanks for taking a look.
>>
>> Carlos, do you agree? I see you're listed as reviewer of the dynamic
>> linker, so I think I have to have your approval.
>>
>> And please, if approved, commit/push it for me. I don't have apply
>> rights for glibc yet.
>>
>> Thanks,
>> Chung-Lin
>>
> 
> Ping?
> 

Ping x2

Thanks,
Chung-Lin
  

On Fri, Apr 25, 2014 at 09:21:50PM +0800, Chung-Lin Tang wrote:
> On 14/4/14 5:19 PM, Chung-Lin Tang wrote:
> > On 14/4/13 5:43 AM, David Miller wrote:
> >> From: Chung-Lin Tang <chunglin_tang@mentor.com>
> >> Date: Fri, 11 Apr 2014 17:37:50 +0800
> >>
> >>> I've CCed some of the people that appeared in that 2012 PLTREL overlap
> >>> thread. Can anybody more familiar with this part of ld.so comment on if
> >>> this is correct?
> >>
> >> The change looks fine to me.
> >>
> > 
> > Hi David, thanks for taking a look.
> > 
> > Carlos, do you agree? I see you're listed as reviewer of the dynamic
> > linker, so I think I have to have your approval.
> > 
> > And please, if approved, commit/push it for me. I don't have apply
> > rights for glibc yet.
> > 
> > Thanks,
> > Chung-Lin
> > 
> 
> Ping?

As sometimes you need more ping I ping for you.

Carlos, are you ok with that?
  

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 with changes.
 
> 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;	      \
>        }									      \
>      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.

OK with that.

Should we assert if SZ/sizeof(ElfW(reloc)) != COUNT?

Cheers,
Carlos.
  

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 with changes.
>  
>> 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;	      \
>>        }									      \
>>      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.

> OK with that.
> 
> Should we assert if SZ/sizeof(ElfW(reloc)) != COUNT?

Please, please don't introduce another divide, defeating my original
purpose...

Thanks,
Chung-Lin

> Cheers,
> Carlos.
  

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.
  

On 14/7/16 4: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.

Thanks!

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

Hi Allan, is this okay for check-in? (and please do for me if okay,
since I don't have write rights for glibc).

Also BTW, is it too late for the Nios II port to be added to 2.20? Its
already gone through a round of review earlier, and I'll be posting
updated patches within this week.

Thanks,
Chung-Lin
  

On 07/16/2014 05:34 AM, Chung-Lin Tang wrote:
> Also BTW, is it too late for the Nios II port to be added to 2.20? Its
> already gone through a round of review earlier, and I'll be posting
> updated patches within this week.

It's too late for the Nios II port IMO.

Please resend the patches when 2.21 opens.

Others may have different opinions though.

Cheers,
Carlos.
  

> It's too late for the Nios II port IMO.
> 
> Please resend the patches when 2.21 opens.
> 
> Others may have different opinions though.

If it doesn't touch any shared files (except perhaps the usual elf.h
additions), then there isn't any strong reason to keep it from going in.
But it would be up to the new machine's maintainer to attest that enough
real-world testing has been done to be sure that there are no ABI snafus
and the like.  Certainly brewing in trunk for most of a cycle seems wiser.
  

On Wed, 16 Jul 2014, Chung-Lin Tang wrote:

> Also BTW, is it too late for the Nios II port to be added to 2.20? Its
> already gone through a round of review earlier, and I'll be posting
> updated patches within this week.

The elf.h patch was approved over two months ago.  It should just be 
committed, not including in any new patch series posted.

As for the rest, is there rough consensus in the Linux kernel community on 
the kernel/userspace ABI for Nios II (in particular, the size of time_t, 
but also other issues such as whether renameat will need implementing in 
userspace in terms of the renameat2 syscall)?  That's needed for it to be 
safe to add the port to glibc.
  

On 2014/7/17 05:43 AM, Joseph S. Myers wrote:
> On Wed, 16 Jul 2014, Chung-Lin Tang wrote:
> 
>> Also BTW, is it too late for the Nios II port to be added to 2.20? Its
>> already gone through a round of review earlier, and I'll be posting
>> updated patches within this week.
> 
> The elf.h patch was approved over two months ago.  It should just be 
> committed, not including in any new patch series posted.
> 
> As for the rest, is there rough consensus in the Linux kernel community on 
> the kernel/userspace ABI for Nios II (in particular, the size of time_t, 
> but also other issues such as whether renameat will need implementing in 
> userspace in terms of the renameat2 syscall)?  That's needed for it to be 
> safe to add the port to glibc.

I don't see either of those patches in 3.16-rc5 at the moment, so I
guess it won't be an issue for 3.16.

I'm not sure about the time_t/timespec changes, but renameat2 should be
pretty straightforward, just a new __ASSUME_RENAMEAT2 in
kernel-features.h for 3.16 (if it gets in later), and maybe adding
renameat2() as an API function.

Anyways, I'll work on getting the nios2 patches posted again.

Thanks,
Chung-Lin
  

On Thu, 17 Jul 2014, Chung-Lin Tang wrote:

> > As for the rest, is there rough consensus in the Linux kernel community on 
> > the kernel/userspace ABI for Nios II (in particular, the size of time_t, 
> > but also other issues such as whether renameat will need implementing in 
> > userspace in terms of the renameat2 syscall)?  That's needed for it to be 
> > safe to add the port to glibc.
> 
> I don't see either of those patches in 3.16-rc5 at the moment, so I
> guess it won't be an issue for 3.16.
> 
> I'm not sure about the time_t/timespec changes, but renameat2 should be

Well, that ABI needs sorting out - you need to get kernel community 
consensus about time_t for Nios II - before it's safe to include the port.

> pretty straightforward, just a new __ASSUME_RENAMEAT2 in
> kernel-features.h for 3.16 (if it gets in later), and maybe adding
> renameat2() as an API function.

That's the wrong way round.  There's no need for __ASSUME_RENAMEAT2 unless 
either (a) it's added as an API function (regarding which, see 
<https://sourceware.org/ml/libc-alpha/2014-06/msg00421.html>) and you want 
a fallback version of that API function for older kernels, or (b) some 
interface can be implemented with renameat2, or in a more complicated way 
for kernels not supporting renameat2.  There's no apparent value in 
implementing the rename and renameat APIs using renameat2 on existing 
architectures, so no need for __ASSUME_RENAMEAT2 (rather, if Nios II gets 
only the renameat2 syscall but not the renameat syscall, it and any 
other asm-generic architectures added to the kernel in future would use 
the renameat2 syscall to implement the rename and renameat APIs, while 
AArch64, Tile and any other asm-generic architectures already supported in 
the kernel but not glibc would continue to use the existing code 
implementing those functions with the renameat syscall).

I suppose you might have a new __ASSUME_RENAMEAT_SYSCALL to be defined by 
the subset of asm-generic architectures that have such a syscall (if the 
direction is that new architectures don't have that syscall).
  

On 2014/7/17 下午 10:19, Joseph S. Myers wrote:
> On Thu, 17 Jul 2014, Chung-Lin Tang wrote:
> 
>>> As for the rest, is there rough consensus in the Linux kernel community on 
>>> the kernel/userspace ABI for Nios II (in particular, the size of time_t, 
>>> but also other issues such as whether renameat will need implementing in 
>>> userspace in terms of the renameat2 syscall)?  That's needed for it to be 
>>> safe to add the port to glibc.
>>
>> I don't see either of those patches in 3.16-rc5 at the moment, so I
>> guess it won't be an issue for 3.16.
>>
>> I'm not sure about the time_t/timespec changes, but renameat2 should be
> 
> Well, that ABI needs sorting out - you need to get kernel community 
> consensus about time_t for Nios II - before it's safe to include the port.

There's another review of the nios2 kernel port in progress, which
should cover this.

>> pretty straightforward, just a new __ASSUME_RENAMEAT2 in
>> kernel-features.h for 3.16 (if it gets in later), and maybe adding
>> renameat2() as an API function.
> 
> That's the wrong way round.  There's no need for __ASSUME_RENAMEAT2 unless 
> either (a) it's added as an API function (regarding which, see 
> <https://sourceware.org/ml/libc-alpha/2014-06/msg00421.html>) and you want 
> a fallback version of that API function for older kernels, or (b) some 
> interface can be implemented with renameat2, or in a more complicated way 
> for kernels not supporting renameat2.  There's no apparent value in 
> implementing the rename and renameat APIs using renameat2 on existing 
> architectures, so no need for __ASSUME_RENAMEAT2 (rather, if Nios II gets 
> only the renameat2 syscall but not the renameat syscall, it and any 
> other asm-generic architectures added to the kernel in future would use 
> the renameat2 syscall to implement the rename and renameat APIs, while 
> AArch64, Tile and any other asm-generic architectures already supported in 
> the kernel but not glibc would continue to use the existing code 
> implementing those functions with the renameat syscall).

Yes, the original intention seemed to be for the renameat syscall to be
removed by default in 3.16, leaving only renameat2, unless
__ARCH_WANT_RENAMEAT is defined in the kernel port's
include/asm/unistd.h (which would be all current ports)  That has not
happened yet; things seem still the same as 3.15 ATM.

If renameat is removed, and only renameat2 available, we need a
__ASSUME_RENAMEAT2 code path to support that, so I think this is sort of
independent from whether we provide a renameat2() API or not.

Anyways, we'll need a bit more clarification on what exactly will enter
3.16.

Chung-Lin

> I suppose you might have a new __ASSUME_RENAMEAT_SYSCALL to be defined by 
> the subset of asm-generic architectures that have such a syscall (if the 
> direction is that new architectures don't have that syscall).
>
  

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

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