[PATCH]: add ia64 vDSO support to bfd_from_remote_memory

[Jeff Johnston]

James E Wilson wrote:
> 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.
>

Thanks.

> The first thing I notice is that there are no comments in the patch
> explaining why the code changed.
> 

Yes, my bad.

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

That indeed was the case.

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

I have tested your alternate patch and it works great, thanks.  The following 
shows a sample traceback of threads in syscalls with your patch plus my gdb 
patch applied.

(gdb) thread apply all bt
[New Thread 2305843009227338368 (LWP 10212)]

Thread 3 (Thread 2305843009227338368 (LWP 10212)):
#0  0xa000000000010641 in __kernel_syscall_via_break ()
#1  0x20000000001a9920 in __GC___libc_nanosleep () from /lib/tls/libc.so.6.1
#2  0x20000000001a9570 in sleep () from /lib/tls/libc.so.6.1
#3  0x4000000000000b90 in threadA (tname=0x4000000000000ef0) at thread.c:40
#4  0x2000000000061cc0 in start_thread () from /lib/tls/libpthread.so.0
#5  0x2000000000225930 in __clone2 () from /lib/tls/libc.so.6.1

Thread 2 (Thread 2305843009237824128 (LWP 10213)):
#0  threadB (tname=0x4000000000000f28) at thread.c:49
#1  0x2000000000061cc0 in start_thread () from /lib/tls/libpthread.so.0
#2  0x2000000000225930 in __clone2 () from /lib/tls/libc.so.6.1

Thread 1 (Thread 2305843009216791584 (LWP 10209)):
#0  0xa000000000010641 in __kernel_syscall_via_break ()
#1  0x20000000001a9920 in __GC___libc_nanosleep () from /lib/tls/libc.so.6.1
#2  0x20000000001a9570 in sleep () from /lib/tls/libc.so.6.1
#3  0x40000000000009e0 in main () at thread.c:25
49	     sleep(10);


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

No, there is no reason to pursue my patch as yours is better.  Can you check it 
in please?

-- Jeff J.