Re: elf.c assign_file_positions_for_segments

[Alan Modra <amodra@bigpond.net.au>]

On Fri, Sep 24, 2004 at 09:47:57AM +0930, Alan Modra wrote:
> On Fri, Sep 24, 2004 at 09:12:22AM +0930, Alan Modra wrote:
> > On Thu, Sep 23, 2004 at 05:51:17PM +0200, Mark Kettenis wrote:
> > > I'm not sure where to fix this.
> > 
> > I didn't think about gdb using bfd to generate core files.  Clearly, I
> > need to fix my breakage of assign_file_positions_for_segments.
> 
> While waiting for gdb to build, I took a look at gcore.c.  For read-only
> sections, I see you clear SEC_LOAD but leave SEC_HAS_CONTENTS set.
> This is very likely why you're getting filesz non-zero for these
> sections.  The new code consistently checks both SEC_LOAD and
> SEC_HAS_CONTENTS whereas the old code just checked SEC_HAS_CONTENTS in
> one place.
> 
> I'll take a good look at exactly why the SEC_HAS_CONTENTS check is
> needed, ie. whether the following comment reflects current reality.
> /* We check SEC_HAS_CONTENTS here because if NOLOAD is used in a linker
>    script we may have a section with SEC_LOAD clear but which is
>    supposed to have contents.  */
> 
> If we really don't need the SEC_HAS_CONTENTS test, then I'll take it out
> and gdb gcore should be OK, otherwise I might ask you to clear
> SEC_HAS_CONTENTS in gdb.

I think it likely that the SEC_HAS_CONTENTS test is just a hack to work
around another problem.  I'm applying the following to go back to the
old behaviour.

You might like to clear SEC_HAS_CONTENTS in gdb too, as it will result
in needless file space being allocated.

	* elf.c (IS_LOADED): Delete.
	(assign_file_positions_for_segments): Just test SEC_LOAD instead.
	Restore SEC_HAS_CONTENTS test to the one place it was used prior
	to 2004-09-22.

Index: bfd/elf.c
===================================================================
RCS file: /cvs/src/src/bfd/elf.c,v
retrieving revision 1.246
diff -u -p -r1.246 elf.c
--- bfd/elf.c	22 Sep 2004 06:45:38 -0000	1.246
+++ bfd/elf.c	24 Sep 2004 02:59:52 -0000
@@ -3787,12 +3787,6 @@ vma_page_aligned_bias (bfd_vma vma, ufil
   return ((vma - off) % maxpagesize);
 }
 
-/* We check SEC_HAS_CONTENTS here because if NOLOAD is used in a linker
-   script we may have a section with SEC_LOAD clear but which is
-   supposed to have contents.  */
-#define IS_LOADED(FLAGS) \
-  (((FLAGS) & SEC_LOAD) != 0 || ((FLAGS) & SEC_HAS_CONTENTS) != 0)
-
 /* Assign file positions to the sections based on the mapping from
    sections to segments.  This function also sets up some fields in
    the file header, and writes out the program headers.  */
@@ -3959,7 +3953,7 @@ assign_file_positions_for_segments (bfd 
 		 .tbss, we need to look at the next section to decide
 		 whether the segment has any loadable sections.  */
 	      i = 0;
-	      while (!IS_LOADED (m->sections[i]->flags))
+	      while ((m->sections[i]->flags & SEC_LOAD) == 0)
 		{
 		  if ((m->sections[i]->flags & SEC_THREAD_LOCAL) == 0
 		      || ++i >= m->count)
@@ -4107,7 +4101,7 @@ assign_file_positions_for_segments (bfd 
 	    {
 	      bfd_signed_vma adjust;
 
-	      if (IS_LOADED (flags))
+	      if ((flags & SEC_LOAD) != 0)
 		{
 		  adjust = sec->lma - (p->p_paddr + p->p_filesz);
 		  if (adjust < 0)
@@ -4164,11 +4158,26 @@ assign_file_positions_for_segments (bfd 
 	      if (p->p_type == PT_LOAD)
 		{
 		  sec->filepos = off;
-		  if (IS_LOADED (flags))
+		  /* FIXME: The SEC_HAS_CONTENTS test here dates back to
+		     1997, and the exact reason for it isn't clear.  One
+		     plausible explanation is that it is to work around
+		     a problem we have with linker scripts using data
+		     statements in NOLOAD sections.  I don't think it
+		     makes a great deal of sense to have such a section
+		     assigned to a PT_LOAD segment, but apparently
+		     people do this.  The data statement results in a
+		     bfd_data_link_order being built, and these need
+		     section contents to write into.  Eventually, we get
+		     to _bfd_elf_write_object_contents which writes any
+		     section with contents to the output.  Make room
+		     here for the write, so that following segments are
+		     not trashed.  */
+		  if ((flags & SEC_LOAD) != 0
+		      || (flags & SEC_HAS_CONTENTS) != 0)
 		    off += sec->size;
 		}
 
-	      if (IS_LOADED (flags))
+	      if ((flags & SEC_LOAD) != 0)
 		{
 		  p->p_filesz += sec->size;
 		  p->p_memsz += sec->size;


-- 
Alan Modra
IBM OzLabs - Linux Technology Centre