Re: PATCH: PR ld/4701: binutils generates invalid klibc-based binary on Linux x86_64

["H. J. Lu" <hjl at lucon dot org>]

On Thu, Jun 28, 2007 at 06:02:20AM -0700, H. J. Lu wrote:
> On Thu, Jun 28, 2007 at 03:05:15PM +0930, Alan Modra wrote:
> > On Wed, Jun 27, 2007 at 09:20:51PM -0700, H. J. Lu wrote:
> > > On Thu, Jun 28, 2007 at 12:30:27PM +0930, Alan Modra wrote:
> > > > On Wed, Jun 27, 2007 at 07:40:27PM -0700, H. J. Lu wrote:
> > > > > Linux ld.so isn't OK with your change. We haven't seen the problem
> > > > > because we won't get a PT_LOAD segment with .bss sections only
> > > > > using the default linker script. I uploaded a testcase to:
> > > > > 
> > > > > http://sourceware.org/bugzilla/show_bug.cgi?id=4701
> > > > 
> > > > Arggh!  I was sure this worked!  So we should go back to using my voff
> > > > code probably, or otherwise hack p_offset.
> > > 
> > > Whatever we do, we should follow gABI.
> > > > 
> > > > Meanwhile, please let me know exactly why klibc was unhappy, as I'm
> > > > concerned that whatever is causing the segv there might also be
> > > > affected by overlapping p_offset for bss segments.
> > > 
> > > My testcase is derived from klibc. Executables have a one PT_LOAD
> > > segment with only a .bss section. The only difference is the failed
> > > ones in klibc are executables, which are loaded by kernel, and mine
> > > is a shared library, which is loaded by ld.so.
> > 
> > This should comply with alignment requirement but not waste file
> > space.  Can you try this against klibc executables?
> > 
> > 	* elf.c (assign_file_positions_for_load_sections): Ensure bss
> > 	segments meet gABI alignment requirements.
> > 
> 
> It works on klibc. However, I got
> 
> FAIL: overlay size
> 
> on Linux/x86-64. You should be able to see it with a cross binutils.
> 
> 

This modified patch works on Linux/x86-64. I added

if ((ufile_ptr) off >= align)
  {
  }

so that we always pad if offset < align.


H.J.
-----
--- bfd/elf.c.bss	2007-06-27 19:07:44.000000000 -0700
+++ bfd/elf.c	2007-06-28 06:37:46.000000000 -0700
@@ -4429,6 +4429,7 @@ assign_file_positions_for_load_sections 
        m = m->next, p++, j++)
     {
       asection **secpp;
+      bfd_vma off_adjust;
 
       /* If elf_segment_map is not from map_sections_to_segments, the
          sections may not be correctly ordered.  NOTE: sorting should
@@ -4484,11 +4485,11 @@ assign_file_positions_for_load_sections 
       else
 	p->p_align = 0;
 
+      off_adjust = 0;
       if (p->p_type == PT_LOAD
 	  && m->count > 0)
 	{
 	  bfd_size_type align;
-	  bfd_vma adjust;
 	  unsigned int align_power = 0;
 
 	  if (m->p_align_valid)
@@ -4515,27 +4516,38 @@ assign_file_positions_for_load_sections 
 		 set via struct bfd_elf_special_section.  */
 	      elf_section_type (m->sections[i]) = SHT_NOBITS;
 
-	  adjust = vma_page_aligned_bias (m->sections[0]->vma, off, align);
-	  if (adjust != 0)
+	  off_adjust = vma_page_aligned_bias (m->sections[0]->vma, off, align);
+	  if (off_adjust != 0)
 	    {
 	      /* If the first section isn't loadable, the same holds
-		 for any other sections.  We don't need to align the
-		 segment on disk since the segment doesn't need file
-		 space.  */
-	      i = 0;
-	      while (elf_section_type (m->sections[i]) == SHT_NOBITS)
+		 for any other sections.  We shouldn't need to align
+		 the segment on disk since the segment doesn't need
+		 file space, but the gABI arguably requires the
+		 alignment and glibc ld.so checks it.  So to comply
+		 with the alignment requirement but not waste file
+		 space, we adjust p_offset for just this segment.
+		 This may put p_offset past the end of file, but
+		 that shouldn't matter.  */
+	      bfd_boolean must_pad = TRUE;
+	      if ((ufile_ptr) off >= align)
 		{
-		  /* If a segment starts with .tbss, we need to look
-		     at the next section to decide whether the segment
-		     has any loadable sections.  */
-		  if ((elf_section_flags (m->sections[i]) & SHF_TLS) == 0
-		      || ++i >= m->count)
+		  i = 0;
+		  while (elf_section_type (m->sections[i]) == SHT_NOBITS)
 		    {
-		      adjust = 0;
-		      break;
+		      /* If a segment starts with .tbss, we need to look
+			 at the next section to decide whether the segment
+			 has any loadable sections.  */
+		      if (!(elf_section_flags (m->sections[i]) & SHF_TLS)
+			  || ++i >= m->count)
+			{
+			  must_pad = FALSE;
+			  break;
+			}
 		    }
 		}
-	      off += adjust;
+	      off += off_adjust;
+	      if (must_pad)
+		off_adjust = 0;
 	    }
 	}
       /* Make sure the .dynamic section is the first section in the
@@ -4732,6 +4744,7 @@ assign_file_positions_for_load_sections 
 		p->p_flags |= PF_W;
 	    }
 	}
+      off -= off_adjust;
 
       /* Check that all sections are in a PT_LOAD segment.
 	 Don't check funky gdb generated core files.  */