This is the mail archive of the binutils@sources.redhat.com mailing list for the binutils project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH]: add ia64 vDSO support to bfd_from_remote_memory


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.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]