Message ID | 5347B7EE.6020507@mentor.com |
---|---|
State | Committed |
Delegated to: | Chung-Lin Tang |
Headers |
Return-Path: <x14307373@homiemail-mx21.g.dreamhost.com> X-Original-To: siddhesh@wilcox.dreamhost.com Delivered-To: siddhesh@wilcox.dreamhost.com Received: from homiemail-mx21.g.dreamhost.com (peon2454.g.dreamhost.com [208.113.200.127]) by wilcox.dreamhost.com (Postfix) with ESMTP id 8D363360078 for <siddhesh@wilcox.dreamhost.com>; Fri, 11 Apr 2014 02:38:03 -0700 (PDT) Received: by homiemail-mx21.g.dreamhost.com (Postfix, from userid 14307373) id 3BED012026A2; Fri, 11 Apr 2014 02:38:03 -0700 (PDT) X-Original-To: glibc@patchwork.siddhesh.in Delivered-To: x14307373@homiemail-mx21.g.dreamhost.com Received: from sourceware.org (server1.sourceware.org [209.132.180.131]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by homiemail-mx21.g.dreamhost.com (Postfix) with ESMTPS id 0D55E120268E for <glibc@patchwork.siddhesh.in>; Fri, 11 Apr 2014 02:38:03 -0700 (PDT) DomainKey-Signature: a=rsa-sha1; c=nofws; d=sourceware.org; h=list-id :list-unsubscribe:list-subscribe:list-archive:list-post :list-help:sender:message-id:date:from:mime-version:to:cc :subject:content-type; q=dns; s=default; b=eS3hOvALOOkArIv1uYYgB dMq43f06lf2It8d2jJfv5wOt8bBEGsExUcbMTZab2lGuoqZfZpl3zj+fDcbPen5x hYdmS49CyAn5MWzBojKEDXCY7AILhXzIRolOhFUGgmFUl3UZS1W9y8j6EVhWaFOj Xoxb43qecjeuM5/iPeDdEo= DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=sourceware.org; h=list-id :list-unsubscribe:list-subscribe:list-archive:list-post :list-help:sender:message-id:date:from:mime-version:to:cc :subject:content-type; s=default; bh=YRD8EJ0hSBtKe+cEVBWdd1sTLQ8 =; b=m69oypeWBFW6e1s4ZjVWhPn4S7ZKNEm88MLKVnU9ihJPPc61H3sU3S5PrRS 7ehW4DP/uc1HQhU5cAFIG6k+47x0wOaoUCeHa+IG/SN1TW70ToMoyAEdaGhrzL+n uOflQXU4bUJY/MRltq/Y/2bD3+WfyPVsfWPDvHDjWaS1mGeM= Received: (qmail 19108 invoked by alias); 11 Apr 2014 09:38:01 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: <libc-alpha.sourceware.org> List-Unsubscribe: <mailto:libc-alpha-unsubscribe-glibc=patchwork.siddhesh.in@sourceware.org> List-Subscribe: <mailto:libc-alpha-subscribe@sourceware.org> List-Archive: <http://sourceware.org/ml/libc-alpha/> List-Post: <mailto:libc-alpha@sourceware.org> List-Help: <mailto:libc-alpha-help@sourceware.org>, <http://sourceware.org/ml/#faqs> Sender: libc-alpha-owner@sourceware.org Delivered-To: mailing list libc-alpha@sourceware.org Received: (qmail 19098 invoked by uid 89); 11 Apr 2014 09:38:00 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.6 required=5.0 tests=AWL, BAYES_00 autolearn=ham version=3.3.2 X-HELO: relay1.mentorg.com Message-ID: <5347B7EE.6020507@mentor.com> Date: Fri, 11 Apr 2014 17:37:50 +0800 From: Chung-Lin Tang <chunglin_tang@mentor.com> User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:24.0) Gecko/20100101 Thunderbird/24.4.0 MIME-Version: 1.0 To: <libc-alpha@sourceware.org> CC: Carlos O'Donell <carlos@redhat.com>, David Miller <davem@davemloft.net>, "Joseph S. Myers" <joseph@codesourcery.com> Subject: [PATCH] Remove divide from _ELF_DYNAMIC_DO_RELOC Content-Type: multipart/mixed; boundary="------------070007070108010009060500" X-DH-Original-To: glibc@patchwork.siddhesh.in |
Commit Message
Chung-Lin Tang
April 11, 2014, 9:37 a.m. UTC
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.
Comments
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. >> >
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)) \