From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 29899 invoked by alias); 27 May 2005 22:40:28 -0000 Mailing-List: contact binutils-help@sources.redhat.com; run by ezmlm Precedence: bulk List-Subscribe: List-Archive: List-Post: List-Help: , Sender: binutils-owner@sources.redhat.com Received: (qmail 29755 invoked by uid 22791); 27 May 2005 22:40:20 -0000 Received: from w098.z064220152.sjc-ca.dsl.cnc.net (HELO bluesmobile.specifixinc.com) (64.220.152.98) by sourceware.org (qpsmtpd/0.30-dev) with ESMTP; Fri, 27 May 2005 22:40:20 +0000 Received: from [127.0.0.1] (bluesmobile.corp.specifix.com [192.168.1.2]) by bluesmobile.specifixinc.com (Postfix) with ESMTP id C919B16B41; Fri, 27 May 2005 15:40:17 -0700 (PDT) Subject: Re: [PATCH]: add ia64 vDSO support to bfd_from_remote_memory From: James E Wilson To: Jeff Johnston Cc: binutils@sourceware.org In-Reply-To: <428CC218.1040304@redhat.com> References: <428CC218.1040304@redhat.com> Content-Type: text/plain Message-Id: <1117233617.24670.25.camel@aretha.corp.specifixinc.com> Mime-Version: 1.0 Date: Fri, 27 May 2005 23:09:00 -0000 Content-Transfer-Encoding: 7bit X-SW-Source: 2005-05/txt/msg00724.txt.bz2 On Thu, 2005-05-19 at 09:43, Jeff Johnston wrote: > The problem is that the ia64 vDSO has two pages in the address space that map to > the same single page of contents. This is because the vsyscall entry point > requires a special kind of mapping which must be executable, but not readable. > There is a 2nd mapping required which is readable and non-executable that allows > reading of the ELF info. This means that the program headers in the ia64 vDSO > are non-standard with two PT_LOAD segments, both with p_offset of 0, but > different p_vaddr values. This is code and concepts I am not familiar with, but it has been 8 days and no one else has commented, so I will try. The first thing I notice is that there are no comments in the patch explaining why the code changed. The second thing is that the patch doesn't seem to exactly match the explanation you provided. You mentioned that we have to use the program header with the smaller p_vaddr, but the patch doesn't test for that. The patch simply uses the first one. I suppose we can assume that the first one has the smaller p_vaddr, but if so that assumption should probably be documented. Looking at the code, I see that it is computing the offset between vaddrs mentioned in the ELF headers from the object file, and the actual vaddrs used after the ELF headers are loaded. Since the ELF object is intended to be loaded as a single unit, this offset should be the same for all sections, so we only need and want to compute it once. The fact that the current code can compute it multiple times is unnecessary. So that part of the patch makes sense. The question remains as to whether it is safe to assume the first one is OK. You mentioned that we have two PT_LOAD segments, one executable only, and one read only. We want to use the read-only one. In that case, shouldn't we be checking the program header flags for the read flag? The read flag is always set for normal segments, so this should exclude only the special IA-64 vDSO executable-only mapping. I have an another issue here, which is that we scan for PT_LOAD segments twice. Once to compute loadbase, and a second time to read the segments. This patch fixes the first loop to ignore the executable-only segment (assuming it always comes after the read-only one), but doesn't do anything about the second scan loop. That means we will be reading the segment twice. This is clearly inefficient. Also, since you indicated that the virtual offsets are different, this implies that the second read could perhaps clobber the first one if we are reading at the wrong offset (and we know that the wrong offset executable one always comes second). This seems like a problem. If not, why not? Putting this together, gives the following alternative patch which I have attached, which may be a better solution. I have no way to test this, as I don't know how to reproduce the problem, and probably don't even have the right OS versions necessary to reproduce. Your patch has clearly been well tested; I don't have any issues with that. If there is a reason why checking for the program header read flag doesn't work then your patch looks OK to me with an appropriate comment explaining what is going on. -- Jim Wilson, GNU Tools Support, http://www.SpecifixInc.com