PR bfd/14207 changes vs *-*-nacl* targets

PR bfd/14207 changes vs *-*-nacl* targets

[Roland McGrath]

Since the changes to fix bug 14207, I'm now seeing these new failures
for arm-nacl, i686-nacl, and x86_64-nacl targets:

UNRESOLVED: strip -z relro (relro1)
UNRESOLVED: objcopy -z relro (relro1)
UNRESOLVED: objcopy -z relro (tdata1)
UNRESOLVED: objcopy -z relro (tdata2)

These are hitting the abort in assign_file_positions_for_non_load_sections
added by:

	2012-06-12  H.J. Lu  <hongjiu.lu@intel.com>

		PR bfd/14207
		* elf.c (assign_file_positions_for_non_load_sections): Abort if
		PT_GNU_RELRO segment doesn't fit in PT_LOAD segment.

(Incidentally, why is that a conditional call to abort instead of a use of
BFD_ASSERT?)

Dropping the new check I do indeed get a bogus PT_NULL header generated.
So it looks like earlier fix:

	2012-06-12  Alan Modra  <amodra@gmail.com>

		PR ld/14207
		* elf.c (_bfd_elf_map_sections_to_segments): Disregard bss type
		sections at end of PT_LOAD segment when searching for segment
		that contains end of relro extent.

for this issue did not cover all cases.  I lack the context that folks like
Alan have on how this stuff is organized in the linker, so it's probably
easier for someone else to build a --target=x86_64-nacl configuration and
debug this than for me to figure it all out myself.


Thanks,
Roland

Re: PR bfd/14207 changes vs *-*-nacl* targets

[H.J. Lu]

On Mon, Jul 2, 2012 at 12:08 PM, Roland McGrath <mcgrathr@google.com> wrote:
> Since the changes to fix bug 14207, I'm now seeing these new failures
> for arm-nacl, i686-nacl, and x86_64-nacl targets:
>
> UNRESOLVED: strip -z relro (relro1)
> UNRESOLVED: objcopy -z relro (relro1)
> UNRESOLVED: objcopy -z relro (tdata1)
> UNRESOLVED: objcopy -z relro (tdata2)
>
> These are hitting the abort in assign_file_positions_for_non_load_sections
> added by:
>
>         2012-06-12  H.J. Lu  <hongjiu.lu@intel.com>
>
>                 PR bfd/14207
>                 * elf.c (assign_file_positions_for_non_load_sections): Abort if
>                 PT_GNU_RELRO segment doesn't fit in PT_LOAD segment.
>
> (Incidentally, why is that a conditional call to abort instead of a use of
> BFD_ASSERT?)

Since this is a real linker bug and should never happen.

> Dropping the new check I do indeed get a bogus PT_NULL header generated.
> So it looks like earlier fix:
>
>         2012-06-12  Alan Modra  <amodra@gmail.com>
>
>                 PR ld/14207
>                 * elf.c (_bfd_elf_map_sections_to_segments): Disregard bss type
>                 sections at end of PT_LOAD segment when searching for segment
>                 that contains end of relro extent.
>
> for this issue did not cover all cases.  I lack the context that folks like
> Alan have on how this stuff is organized in the linker, so it's probably
> easier for someone else to build a --target=x86_64-nacl configuration and
> debug this than for me to figure it all out myself.
>

Can you revert Alan's patch and use mine in

http://sourceware.org/ml/binutils/2012-06/msg00112.html

to see if it works better for you?


-- 
H.J.

Re: PR bfd/14207 changes vs *-*-nacl* targets

[Roland McGrath]

On Mon, Jul 2, 2012 at 12:15 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
> Can you revert Alan's patch and use mine in
>
> http://sourceware.org/ml/binutils/2012-06/msg00112.html
>
> to see if it works better for you?

The results are the same.


Thanks,
Roland

Re: PR bfd/14207 changes vs *-*-nacl* targets

[H.J. Lu]

On Mon, Jul 2, 2012 at 12:08 PM, Roland McGrath <mcgrathr@google.com> wrote:
> Since the changes to fix bug 14207, I'm now seeing these new failures
> for arm-nacl, i686-nacl, and x86_64-nacl targets:
>
> UNRESOLVED: strip -z relro (relro1)
> UNRESOLVED: objcopy -z relro (relro1)
> UNRESOLVED: objcopy -z relro (tdata1)
> UNRESOLVED: objcopy -z relro (tdata2)
>
> These are hitting the abort in assign_file_positions_for_non_load_sections
> added by:
>
>         2012-06-12  H.J. Lu  <hongjiu.lu@intel.com>
>
>                 PR bfd/14207
>                 * elf.c (assign_file_positions_for_non_load_sections): Abort if
>                 PT_GNU_RELRO segment doesn't fit in PT_LOAD segment.
>
> (Incidentally, why is that a conditional call to abort instead of a use of
> BFD_ASSERT?)
>
> Dropping the new check I do indeed get a bogus PT_NULL header generated.
> So it looks like earlier fix:
>
>         2012-06-12  Alan Modra  <amodra@gmail.com>
>
>                 PR ld/14207
>                 * elf.c (_bfd_elf_map_sections_to_segments): Disregard bss type
>                 sections at end of PT_LOAD segment when searching for segment
>                 that contains end of relro extent.
>
> for this issue did not cover all cases.  I lack the context that folks like
> Alan have on how this stuff is organized in the linker, so it's probably
> easier for someone else to build a --target=x86_64-nacl configuration and
> debug this than for me to figure it all out myself.
>

The problem is _bfd_elf_map_sections_to_segments isn't consistent
with assign_file_positions_for_load_sections.  I don't think it is handled
properly between _bfd_elf_map_sections_to_segments and
assign_file_positions_for_load_sections.

Also I am not sure if NACL segment layout is compatible with GNU_RELRO.

-- 
H.J.

Re: PR bfd/14207 changes vs *-*-nacl* targets

[H.J. Lu]

On Mon, Jul 2, 2012 at 3:42 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
> On Mon, Jul 2, 2012 at 12:08 PM, Roland McGrath <mcgrathr@google.com> wrote:
>> Since the changes to fix bug 14207, I'm now seeing these new failures
>> for arm-nacl, i686-nacl, and x86_64-nacl targets:
>>
>> UNRESOLVED: strip -z relro (relro1)
>> UNRESOLVED: objcopy -z relro (relro1)
>> UNRESOLVED: objcopy -z relro (tdata1)
>> UNRESOLVED: objcopy -z relro (tdata2)
>>
>> These are hitting the abort in assign_file_positions_for_non_load_sections
>> added by:
>>
>>         2012-06-12  H.J. Lu  <hongjiu.lu@intel.com>
>>
>>                 PR bfd/14207
>>                 * elf.c (assign_file_positions_for_non_load_sections): Abort if
>>                 PT_GNU_RELRO segment doesn't fit in PT_LOAD segment.
>>
>> (Incidentally, why is that a conditional call to abort instead of a use of
>> BFD_ASSERT?)
>>
>> Dropping the new check I do indeed get a bogus PT_NULL header generated.
>> So it looks like earlier fix:
>>
>>         2012-06-12  Alan Modra  <amodra@gmail.com>
>>
>>                 PR ld/14207
>>                 * elf.c (_bfd_elf_map_sections_to_segments): Disregard bss type
>>                 sections at end of PT_LOAD segment when searching for segment
>>                 that contains end of relro extent.
>>
>> for this issue did not cover all cases.  I lack the context that folks like
>> Alan have on how this stuff is organized in the linker, so it's probably
>> easier for someone else to build a --target=x86_64-nacl configuration and
>> debug this than for me to figure it all out myself.
>>
>
> The problem is _bfd_elf_map_sections_to_segments isn't consistent
> with assign_file_positions_for_load_sections.  I don't think it is handled
> properly between _bfd_elf_map_sections_to_segments and
> assign_file_positions_for_load_sections.
>
> Also I am not sure if NACL segment layout is compatible with GNU_RELRO.
>

You can either fix assign_file_positions_for_load_sections for
NACL segment layout or disable GNU_RELRO for NACL.

-- 
H.J.

Re: PR bfd/14207 changes vs *-*-nacl* targets

[Roland McGrath]

On Mon, Jul 2, 2012 at 3:42 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
> The problem is _bfd_elf_map_sections_to_segments isn't consistent
> with assign_file_positions_for_load_sections.  I don't think it is handled
> properly between _bfd_elf_map_sections_to_segments and
> assign_file_positions_for_load_sections.

I don't know what this means at all.  Can you elaborate?

> Also I am not sure if NACL segment layout is compatible with GNU_RELRO.

There's no reason it shouldn't be.  RELRO makes sense for any layout where
the writable data segment has a leading portion composed of sections in
which RELRO treatment makes sense.  The NaCl layout does not change
anything about that.


Thanks,
Roland

Re: PR bfd/14207 changes vs *-*-nacl* targets

[H.J. Lu]

On Mon, Jul 2, 2012 at 4:11 PM, Roland McGrath <mcgrathr@google.com> wrote:
> On Mon, Jul 2, 2012 at 3:42 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
>> The problem is _bfd_elf_map_sections_to_segments isn't consistent
>> with assign_file_positions_for_load_sections.  I don't think it is handled
>> properly between _bfd_elf_map_sections_to_segments and
>> assign_file_positions_for_load_sections.
>
> I don't know what this means at all.  Can you elaborate?

In _bfd_elf_map_sections_to_segments, NACL calls
nacl_modify_segment_map after sections are mapped
to segment.  When assign_file_positions_for_load_sections
is called, segment map isn't the same as the one used
to set up RELRO segment.  assign_file_positions_for_load_sections
isn't prepared to deal with it.


-- 
H.J.

Re: PR bfd/14207 changes vs *-*-nacl* targets

[Roland McGrath]

On Mon, Jul 2, 2012 at 5:02 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
> In _bfd_elf_map_sections_to_segments, NACL calls
> nacl_modify_segment_map after sections are mapped
> to segment.  When assign_file_positions_for_load_sections
> is called, segment map isn't the same as the one used
> to set up RELRO segment.  assign_file_positions_for_load_sections
> isn't prepared to deal with it.

I see.  I'll see if I can make sense of it one way or the other.


Thanks,
Roland

Re: PR bfd/14207 changes vs *-*-nacl* targets

[Alan Modra]

My guess is that you'll find the problem is due to SIZEOF_HEADERS
padding inserted by the nacl linker scripts at the start of rodata
sections.  That sort of gap is a new thing for the relro segment
calculations.  The pr14207 changes didn't really change anything for
your target, except of course the addition of the abort.

-- 
Alan Modra
Australia Development Lab, IBM

Re: PR bfd/14207 changes vs *-*-nacl* targets

[Alan Modra]

On Tue, Jul 03, 2012 at 10:43:57AM +0930, Alan Modra wrote:
> My guess is that you'll find the problem is due to SIZEOF_HEADERS
> padding inserted by the nacl linker scripts at the start of rodata
> sections.  That sort of gap is a new thing for the relro segment
> calculations.  The pr14207 changes didn't really change anything for
> your target, except of course the addition of the abort.

Well it wasn't so much the spacing but the fact that the headers sit
at the start of rodata.  Fixed as follows.

	PR ld/14207
	* elf.c (assign_file_positions_for_load_sections): Remove assertions
	that only PT_LOAD headers include file header and section headers.
	(assign_file_positions_for_non_load_sections): Similarly don't
	assert PT_GNU_RELRO header does not include file and section headers.
	Compare first section vma rather than PT_LOAD p_vaddr against
	relro_start when looking for PT_LOAD covering PT_GNU_RELRO.  Replace
	abort with assertion.

Index: bfd/elf.c
===================================================================
RCS file: /cvs/src/src/bfd/elf.c,v
retrieving revision 1.564
diff -u -p -r1.564 elf.c
--- bfd/elf.c	1 Jul 2012 08:23:13 -0000	1.564
+++ bfd/elf.c	3 Jul 2012 05:09:39 -0000
@@ -4575,8 +4575,6 @@ assign_file_positions_for_load_sections 
 	  p->p_memsz = bed->s->sizeof_ehdr;
 	  if (m->count > 0)
 	    {
-	      BFD_ASSERT (p->p_type == PT_LOAD);
-
 	      if (p->p_vaddr < (bfd_vma) off)
 		{
 		  (*_bfd_error_handler)
@@ -4603,7 +4601,6 @@ assign_file_positions_for_load_sections 
 
 	      if (m->count > 0)
 		{
-		  BFD_ASSERT (p->p_type == PT_LOAD);
 		  p->p_vaddr -= off - p->p_offset;
 		  if (!m->p_paddr_valid)
 		    p->p_paddr -= off - p->p_offset;
@@ -4965,26 +4962,27 @@ assign_file_positions_for_non_load_secti
       if (p->p_type == PT_GNU_RELRO)
 	{
 	  const Elf_Internal_Phdr *lp;
-
-	  BFD_ASSERT (!m->includes_filehdr && !m->includes_phdrs);
+	  struct elf_segment_map *lm;
 
 	  if (link_info != NULL)
 	    {
 	      /* During linking the range of the RELRO segment is passed
 		 in link_info.  */
-	      for (lp = phdrs; lp < phdrs + count; ++lp)
+	      for (lm = elf_tdata (abfd)->segment_map, lp = phdrs;
+		   lm != NULL;
+		   lm = lm->next, lp++)
 		{
 		  if (lp->p_type == PT_LOAD
-		      && lp->p_vaddr >= link_info->relro_start
 		      && lp->p_vaddr < link_info->relro_end
-		      && lp->p_vaddr + lp->p_filesz >= link_info->relro_end)
+		      && lp->p_vaddr + lp->p_filesz >= link_info->relro_end
+		      && lm->count != 0
+		      && lm->sections[0]->vma >= link_info->relro_start)
 		    break;
 		}
 
 	      /* PR ld/14207.  If the RELRO segment doesn't fit in the
 		 LOAD segment, it should be removed.  */
-	      if (lp == (phdrs + count))
-		abort ();
+	      BFD_ASSERT (lm != NULL);
 	    }
 	  else
 	    {

-- 
Alan Modra
Australia Development Lab, IBM

Re: PR bfd/14207 changes vs *-*-nacl* targets

[Roland McGrath]

On Mon, Jul 2, 2012 at 10:49 PM, Alan Modra <amodra@gmail.com> wrote:
> Well it wasn't so much the spacing but the fact that the headers sit
> at the start of rodata.  Fixed as follows.

That did the trick.  Thanks muchly!