[PATCH]: add ia64 vDSO support to bfd_from_remote_memory

[PATCH]: add ia64 vDSO support to bfd_from_remote_memory

[Jeff Johnston]

The following patch is required to get ia64 vsyscall support working in gdb.  It 
was created by Roland McGrath.

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.

The bfd_from_remote_memory code in bfd/elfcode.h doesn't handle this scenario 
correctly.  It needs to use the segment with the lowest p_vaddr whose p_offset 
is zero modulo p_align.  The following patch adds an additional test which 
causes the loadbase to be correctly calculated.  With the patch, gdb can support 
ia64 vsyscall backtracing.

This change has been present in Red Hat gdb sources for some time and has been 
tested on i686, ia64, s390, s390x, ppc, ppc64, and x86_64.

Ok to commit to general sources?

2005-05-19  Roland McGrath  <roland@redhat.com>

	* elfcode.h (NAME (_bfd_elf, bfd_from_remote_memory): Adjust to support
	ia64 vDSO ideosyncracies.

Index: elfcode.h
===================================================================
RCS file: /cvs/src/src/bfd/elfcode.h,v
retrieving revision 1.68
diff -u -p -r1.68 elfcode.h
--- elfcode.h	9 May 2005 03:35:38 -0000	1.68
+++ elfcode.h	19 May 2005 16:39:08 -0000
@@ -1641,7 +1641,8 @@ NAME(_bfd_elf,bfd_from_remote_memory)
  	  if (segment_end > (bfd_vma) contents_size)
  	    contents_size = segment_end;

-	  if ((i_phdrs[i].p_offset & -i_phdrs[i].p_align) == 0)
+	  if ((i_phdrs[i].p_offset & -i_phdrs[i].p_align) == 0
+	      && loadbase == ehdr_vma)
  	    loadbase = ehdr_vma - (i_phdrs[i].p_vaddr & -i_phdrs[i].p_align);

  	  last_phdr = &i_phdrs[i];

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

[James E Wilson]

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

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

[James E Wilson]

[-- Attachment #1: Type: text/plain, Size: 416 bytes --]

On Fri, 2005-05-27 at 15:40, James E Wilson wrote:
> 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.

This time really attached.
-- 
Jim Wilson, GNU Tools Support, http://www.SpecifixInc.com


[-- Attachment #2: patch.ia64.vdso --]
[-- Type: text/x-patch, Size: 1794 bytes --]

Index: elfcode.h
===================================================================
RCS file: /cvs/src/src/bfd/elfcode.h,v
retrieving revision 1.69
diff -p -p -r1.69 elfcode.h
*** elfcode.h	26 May 2005 07:41:13 -0000	1.69
--- elfcode.h	27 May 2005 19:27:28 -0000
*************** NAME(_bfd_elf,bfd_from_remote_memory)
*** 1656,1662 ****
    for (i = 0; i < i_ehdr.e_phnum; ++i)
      {
        elf_swap_phdr_in (templ, &x_phdrs[i], &i_phdrs[i]);
!       if (i_phdrs[i].p_type == PT_LOAD)
  	{
  	  bfd_vma segment_end;
  	  segment_end = (i_phdrs[i].p_offset + i_phdrs[i].p_filesz
--- 1656,1665 ----
    for (i = 0; i < i_ehdr.e_phnum; ++i)
      {
        elf_swap_phdr_in (templ, &x_phdrs[i], &i_phdrs[i]);
!       /* IA-64 vDSO may have two mappings for one segment, where one mapping
! 	 is executable only, and one is read only.  We must not use the
! 	 executable one.  */
!       if (i_phdrs[i].p_type == PT_LOAD && (i_phdrs[i].p_flags & PF_R))
  	{
  	  bfd_vma segment_end;
  	  segment_end = (i_phdrs[i].p_offset + i_phdrs[i].p_filesz
*************** NAME(_bfd_elf,bfd_from_remote_memory)
*** 1703,1709 ****
      }
  
    for (i = 0; i < i_ehdr.e_phnum; ++i)
!     if (i_phdrs[i].p_type == PT_LOAD)
        {
  	bfd_vma start = i_phdrs[i].p_offset & -i_phdrs[i].p_align;
  	bfd_vma end = (i_phdrs[i].p_offset + i_phdrs[i].p_filesz
--- 1706,1715 ----
      }
  
    for (i = 0; i < i_ehdr.e_phnum; ++i)
!     /* IA-64 vDSO may have two mappings for one segment, where one mapping
!        is executable only, and one is read only.  We must not use the
!        executable one.  */
!     if (i_phdrs[i].p_type == PT_LOAD && (i_phdrs[i].p_flags & PF_R))
        {
  	bfd_vma start = i_phdrs[i].p_offset & -i_phdrs[i].p_align;
  	bfd_vma end = (i_phdrs[i].p_offset + i_phdrs[i].p_filesz

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

[Andreas Schwab]

James E Wilson <wilson@specifixinc.com> writes:

> On Fri, 2005-05-27 at 15:40, James E Wilson wrote:
>> 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.
>
> This time really attached.

Neither patch makes any difference in behviour of backtraces through
signal handler or system calls.  They still don't work.

Andreas.

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE Linux Products GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

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

[Jeff Johnston]

Andreas Schwab wrote:
> James E Wilson <wilson@specifixinc.com> writes:
> 
> 
>>On Fri, 2005-05-27 at 15:40, James E Wilson wrote:
>>
>>>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.
>>
>>This time really attached.
> 
> 
> Neither patch makes any difference in behviour of backtraces through
> signal handler or system calls.  They still don't work.
> 
> Andreas.
> 

Because I haven't updated FSF gdb due to its dependence on this patch...

-- Jeff J.

Re: [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.

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

[James E Wilson]

[-- Attachment #1: Type: text/plain, Size: 270 bytes --]

On Mon, 2005-05-30 at 11:22, Jeff Johnston wrote:
> No, there is no reason to pursue my patch as yours is better.  Can you check it 
> in please?

OK.  I've checked it in with the following ChangeLog entry.
-- 
Jim Wilson, GNU Tools Support, http://www.SpecifixInc.com


[-- Attachment #2: patch.ia64.vdso --]
[-- Type: text/plain, Size: 165 bytes --]

2005-05-31  James E Wilson  <wilson@specifixinc.com>

	* elfcode.h (NAME(bfd_elf,bfd_from_remote_memory)): Check for program
	header PF_R flag on PT_LOAD segments.