deleting relocs, objcopy and BFD

deleting relocs, objcopy and BFD

[Jose E. Marchesi]

Howdy!  Apologies in advance for the long email :)

The recent commit

commit 1d15e434f43bc41a07bc7b0648fcb7e6ccbe8dcc
Author: Nick Clifton <nickc@redhat.com>
Date:   Thu Apr 13 14:50:56 2017 +0100

    Add note merging to strip and add code to merge stack size notes.

    * objcopy.c: Add --no-merge-notes option to disable note merging.
      Add --[no-]merge-notes option to strip, and enable it by default.
      (num_bytes): New function.
      (merge_gnu_build_notes): Add code to merge stack size notes.
    * binutils.texi: Update strip and objcopy documentation.
    * readelf.c (print_gnu_build_attribute_name): Use defined
      constants for note types.

Introduced a regression in two ELF targets: elf64-sparc and elf64-mips:

FAIL: merge notes section (64-bits)

This happens because objcopy segfauls when relocs get deleted as the
result of removing GNU build notes in a merge:

$ ./objcopy --merge-notes tmpdir/bintest.o tmpdir/copy.o
Segmentation fault

While investigating it I found two different problems: one in the way
BFD handles relocations and another in the way objcopy deletes
relocations.

Let me describe these problems and suggest some solutions.

Problem 1: incoherence with internal relocs and external relocs breaks
the API.

The main fields involved in the handling of relocations in `asection'
structs are (from section.c):

.  {* If an input section, a pointer to a vector of relocation
.     records for the data in this section.  *}
.  struct reloc_cache_entry *relocation;
.
.  {* If an output section, a pointer to a vector of pointers to
.     relocation records for the data in this section.  *}
.  struct reloc_cache_entry **orelocation;
.
.  {* The number of relocation records in one of the above.  *}
.  unsigned reloc_count;

For the vast majority of the targets supported by BFD, the semantics
documented above are correct: there is a 1-1 relationship between
internal relocs (arelents) and the external relocs.

However, there are two ELF targets, sparc64 and mips64, for which the
relationship is different:

- In mips64 every external relocation is implementing using three
  internal relocations.

- In sparc64 all external relocations are implemented using a single
  internal relocation, with the exception of R_SPARC_OLO10, which
  is implemented using two internal relocations.

Due to the above, `sec->reloc_count' really contains the number of
external relocations, and thus:

- In mips64 `sec->relocation' contains `sec->reloc_count * 3' entries.

- In sparc64 `sec->relocation' contains `sec->reloc_count + N' entries,
  where N can only be determined traversing all the SHR_RELA external
  relocations.

- In any other target, `sec->relocation' contains `sec->reloc_count'
  entries.

The ELF backend handles this in two ways in two different areas: linking
and other purposes.

The ELF linker routines use a constant in `struct elf_size_info', which
is defined by the ELF backends:

/* The number of internal relocations to allocate per external
   relocation entry.  */
unsigned char int_rels_per_ext_rel;

Like all the other fields in this struct, `int_rels_per_ext_rel' is a
constant, and it is defined:

elf64-mips: int_rels_per_ext_rel = 3
elf64-sparc: int_rels_per_ext_rel = 1
All other targets: int_rels_per_ext_rel = 1

Note that `1' is right for sparc64 because it looks like the
R_SPARC_OLO10 relocation is not used during linking.  So no problems
there.

For every other purpose (i.e. assembler and binutils) mips64 and sparc64
accomodated their peculiar requirements by providing special versions of
`bfd_canonicalize_reloc'.

`bfd_canonicalize_reloc' is used to get a copy of the `sec->relocation'
array in `*relpp'.  The documentation of this function in bfd.c says:

  "Call the back end associated with the open BFD @var{abfd} and
   translate the external form of the relocation information attached to
   @var{sec} into the internal canonical form.  Place the table into
   memory at @var{loc}, which has been preallocated, usually by a call
   to <<bfd_get_reloc_upper_bound>>.  Returns the number of relocs, or
   -1 on error."

By "the number of relocs" it means the number of arelents stored in
`sec->relocation' after the operation, i.e. the number of internal
relocations.

- The default ELF implementation in elf.c first calls
  `slurp_reloc_table', that is expected to read the SHT_REL and SHT_RELA
  sections, convert from the external ELF relocation entries to internal
  arelents and update `sec->relocation'.

  Then it copies `sec->reloc_count' entries from `sec->relocation' to
  the output argument `relpp' and finally returns `sec->reloc_count' to
  the caller.

- The mips64 ELF target does something slightly different: it also calls
  `slurp_reloc_table' that updates `sec->relocation'.

  Then it copies `sec->reloc_count * 3' entries from `sec->relocation'
  to the output argument `relpp' and finally returns `sec->reloc_count *
  3' to the caller.

- The ELF64 ELF target is also different: it calls `slurp_reloc_table'
  that updates `sec->relocation'.

  Then it copies `elf_section_data(sec)->reloc_count' entries from
  `sec->relocation' to the output argument `relpp' and finally returns
  `elf_section_data(sec)->reloc_count'.

  Note that `bfd_elf_section_data(sec)->reloc_count' is _not_
  `sec->reloc_count'.  It is an additional field added by the SPARC ELF
  backend in elfxx-sparc.h used to store the exact number of arelents
  generated from the external relocs.

Lets now look at `slurp_reloc_table'.  As mentioned above, this function
is expected to scan the SHT_REL and SHT_RELA sections, transform the ELF
relocations to arelents and set the value of `sec->relocation'
accordingly.  mips64 and sparc64 also have to provide specialized
versions of this internal function:

- The default ELF implementation in elfcode.h first checks that
  `sec->relocation' is not NULL.  If it is, it just returns.

  Then it counts the number of relocations stored in SHR_REL
  (reloc_count) and SH_RELA (reloc_count2) for the section, and
  allocates space for `sec->relocation', sets its contents, and return:

  amt = (reloc_count + reloc_count2) * sizeof (arelent);
  relents = (arelent *) bfd_alloc (abfd, amt);
  [...]
  /* Set contents of `relents'.  */
  [...]
  asection->relocation = relents;
  return TRUE;

- The mips64 ELF target also checks `sec->relocation' and happily
  returns if it is NULL.

  Then it counts the number of relocations stored in SHR_REL and
  SHR_RELA for the section, and allocates space for `sec->relocation',
  but it knows that an ELF relocation translates into 3 arelents, so it
  does:

  amt = (reloc_count + reloc_count2) * 3 * sizeof (arelent);
  relents = bfd_alloc (abfd, amt);

  Interestingly, this implementation _do_ update `sec->reloc_count' as
  it scans the sections reading the relocation information.
  Effectively, `sec->reloc_count' is set to `reloc_count +
  reloc_count2'.

  It finally returns the arelents just created:

  asect->relocation = relents;
  return TRUE;

- The sparc64 ELF target also checks `sec->relocation' and happily
  returns if it is NULL.

  But then it doesn't count the number of relocations stored in SHT_REL
  and SHR_RELA to determine the number of relocations in the section,
  but relies on the current contents of `sec->reloc_count' instead
  (note, _not_ `bfd_elf_section_data(sec)->reloc_count' that is
  0 at this point):

  amt = asect->reloc_count;
  amt *= 2 * sizeof (arelent);
  asect->relocation = (arelent *) bfd_alloc (abfd, amt);
           
  This is an upper bound, as the ratio between ELF relocations and
  arelents is not constant (like 1-1 for generic and 1-3 for mips64)
  since only one SPARC relocation type (R_SPARC_OLO10) needs two
  arelents.  So, this implementation updates
  `bfd_elf_section_data(sec)->reloc_count' as it scans and transforms
  the relocations, setting it to the exact number of ELF relocations.

  Then it returns.

So we can conclude:

1. Before being called for the first (and unique) time,
   `sec->reloc_count' is expected to contain the number of external
   relocations associated with the section.

2. After being called, `sec->reloc_count' still contains the number of
   external relocations associated with the section.  In the case of
   sparc64, `bfd_elf_section_data(sec)->reloc_count' also contains the
   exact number of internal relocations generated.  This is needed
   because, unlike in generic and mips64, the ratio is not constant.

Another function for which mips64 and sparc64 have to provide
specialized implementations is `bfd_get_reloc_upper_bound'.  This one is
easy:

- The default ELF implementation returns space for `sec->reloc_count + 1'
  arelents.

- The mips64 ELF implementation returns space for `sec->reloc_count * 3
  + 1' arelents.

- The sparc64 ELF implementation returns space for `sec->reloc_count * 2
  + 1' arelents.  This is a worst-case scenario in which all external
  relocs are R_SPARC_OLO10.

However, there is a problem with `bfd_set_reloc'.  There is a single
implementation that is used for all targets, and all it does is:

void
bfd_set_reloc (bfd *ignore_abfd ATTRIBUTE_UNUSED,
               sec_ptr asect,
               arelent **location,
               unsigned int count)
{
  asect->orelocation = location;
  asect->reloc_count = count;
}
                                                
i.e. it assumes a 1-1 relationship between internal relocs and external
relocs.

In the simple use case:

  relcount = bfd_canonicalize_reloc (ibfd, isec, relpp, sympp);

  bfd_set_reloc (ibfd, isec, relpp, relcount);

  /* ... Work with relocs in `relpp' ...  */

  sec_ptr osec = isec->output_section;
  relcount = bfd_canonicalize_reloc (obfd, osec, relpp, isympp);

  bfd_close (obfd);

`bfd_close' calls `_bfd_elf_write_object_contents (obfd)' for
every section, which in turn calls `elfNN_BE_write_relocs'.

Fortunately, both `elf64_mips_write_relocs' and
`elf64_sparc_write_relocs' interpret `sec->reloc_count' as the number of
_internal relocations_ stored in `sec->orelocation', so all is
good. (even if pretty confusing to the casual reader.)

But now consider this:

  relcount = bfd_canonicalize_reloc (ibfd, isec, relpp, sympp); /* (a) */

  /* ... Work with relocs in `relpp' ... */

  bfd_set_reloc (ibfd, isec, relpp, relcount);                  /* (b) */

  [...]

  relcount = bfd_canonicalize_reloc (ibfd, isec, relpp, sympp); /* (c) */

  /* ... Work with relocs in `relpp' ... */

  sec_ptr osec = isec->output_section;
  bfd_set_reloc (obfd, osec, relpp, relcount);                  /* (d) */

  bfd_close (obfd);

The above code is buggy in both mips64 and sparc64, for related but
slightly different reasons:

- In mips64 it fails because the `sec->reloc_count' set in (b) is the
  number of internal relocations, which is misinterpreted by the
  `bfd_canonicalize_reloc' in (c), that returns a bogus (bigger) count.
  This bogus count is then set in (d), and `bfd_close' will likely
  segfault (or corrupt memory) while writing relocs.

- In sparc64 it fails if the relcount set in (b) or (d) is different
  than the internally saved `bfd_elf_section_data(sec)->reloc_count' the
  first (and only) time `slurp_reloc_table' scans the SHT_RELA relocs.

The above sequence of events is what happens in objcopy when it removes
relocations in `merge_gnu_build_notes':

  relsize = bfd_get_reloc_upper_bound (abfd, sec);

  [...]

  relpp = (arelent **) xmalloc (relsize);
  relcount = bfd_canonicalize_reloc (abfd, sec, relpp, isympp);

  [...]

  for (rel = relpp; rel < relpp + relcount; rel ++)
   {
    [...]
    if ((* rel)->address >= (bfd_vma) ((new + note_size) - new_contents))
        (* rel)->address -= note_size;
    else   
      (* rel)->howto = NULL;
   }

   [...]

   for (rel = relpp; rel < relpp + relcount; rel ++)
     if ((* rel)->howto == NULL)
      {
        /* Delete eliminated relocs.
           FIXME: There are better ways to do this. */
        memmove (rel, rel + 1, ((relcount - (rel - relpp)) - 1) * sizeof (*rel));
        relcount --;
      }
   bfd_set_reloc (abfd, sec, relpp, relcount);

and then later copies the relocations of the merged section in
`copy_relocations_in_section':

  [...]

  relpp = (arelent **) xmalloc (relsize);
  relcount = bfd_canonicalize_reloc (ibfd, isection, relpp, isympp);

  [...]

  bfd_set_reloc (obfd, osection, relcount == 0 ? NULL : relpp, relcount);


So, how to fix this problem?  I can think on two approaches:

a) To document in the BFD API that after `bfd_set_reloc' is invoked on
   some section, it is invalid to use `bfd_canonicalize_reloc' in the
   same section, adding an assert to detect such invalid uses (the
   assert could check for the presence of a non-NULL sec->orelocation).

   Client code will have to save the number of internal relocations in a
   variable to avoid having to call `bfd_canonicalize_reloc' again.

   This would obviously involve to fix objcopy to not call
   `bfd_canonicalize_reloc' twice.

b) To remove the API limitation/bug in BFD, somehow.  Maybe adding
   end-of-list sentinels to `sec->relocation' and `sec->orelocation',
   adjusting `bfd_set_reloc' to install it according to its `relcount'
   argument, leaving `sec->reloc_count' untouched, and also making
   `elfNN_BE_write_relocs' to use the sentinel when writing.

WDYT?  a), b) or something else?  Better ideas for b)?

---

Problem 2: objcopy may break internal relocation sequences while merging
build attribute notes.
  
Both mips64 and sparc64 use sequences of internal relocs that conform a
single external relocation.  When objcopy deletes relocations (as the
result of merged build notes, or while stripping symbols) it may break
some of these sequences.

For example:

1. Build binutils with --target=mips64-unknown-openbsd

2. ./gas/as-new binutils/testsuite/binutils-all/note-2-64.s -o foo.o

3. ./binutils/objcopy --merge-notes foo.o bar.o will segfault.

4. Run the above command under GDB and check that:

   - At the beginning of `merge_gnu_build_notes' the
    .gnu_build_attributes section has 3 external relocations.  This
    makes `bfd_canonicalize_reloc' to return a count of 3 * 3 = 9
    internal relocs.

  - The deletion logic in `merge_gnu_build_notes' then deletes two
    internal relocs, leaving 7.  This sounds like a sequence of three
    internal relocs gets broken.  I am not sure if this really a problem
    (don't know much of mips64) but it is worth a check.

The same problem could happen with the sparc64 R_SPARC_LO10,R_SPARC_13
sequence.  Note that in both the mips64 triads and the sparc64 tuples
all the relocations share the same address.

Thoughts?

Re: deleting relocs, objcopy and BFD

[Alan Modra]

On Thu, Apr 27, 2017 at 05:42:31PM +0200, Jose E. Marchesi wrote:
>   - The deletion logic in `merge_gnu_build_notes' then deletes two
>     internal relocs, leaving 7.  This sounds like a sequence of three
>     internal relocs gets broken.  I am not sure if this really a problem
>     (don't know much of mips64) but it is worth a check.

This part of the problem is simply an error in Nick's code, fixed as
follows.  I've read over your description of the reloc count problem
and am not too sure what to do, but my inclination would be to not
return a count from bfd_canonicalize_reloc and not set one in
bfd_set_reloc.  ie. rely on the sentinel.  That will mean rewriting
some code..

This patch doesn't fix the mips64 segfault which is caused by *3 reloc
count finding its way into section->reloc_count.

diff --git a/binutils/ChangeLog b/binutils/ChangeLog
index 9eea3a0..ae8defb 100644
--- a/binutils/ChangeLog
+++ b/binutils/ChangeLog
@@ -1,3 +1,8 @@
+2017-05-01  Alan Modra  <amodra@gmail.com>
+
+	* objcopy.c (merge_gnu_build_notes): Correct code deleting
+	relocs.
+
 2017-04-28  Nick Clifton  <nickc@redhat.com>
 
 	PR binutils/21439
diff --git a/binutils/objcopy.c b/binutils/objcopy.c
index 9bad4b7..42c7775 100644
--- a/binutils/objcopy.c
+++ b/binutils/objcopy.c
@@ -2185,15 +2185,18 @@ merge_gnu_build_notes (bfd * abfd, asection * sec, bfd_size_type size, bfd_byte
 
       if (relcount > 0)
 	{
-	  arelent ** rel;
+	  arelent **rel = relpp;
 
-	  for (rel = relpp; rel < relpp + relcount; rel ++)
-	    if ((* rel)->howto == NULL)
+	  while (rel < relpp + relcount)
+	    if ((*rel)->howto != NULL)
+	      rel++;
+	    else
 	      {
 		/* Delete eliminated relocs.
 		   FIXME: There are better ways to do this.  */
-		memmove (rel, rel + 1, ((relcount - (rel - relpp)) - 1) * sizeof (* rel));
-		relcount --;
+		memmove (rel, rel + 1,
+			 ((relcount - (rel - relpp)) - 1) * sizeof (*rel));
+		relcount--;
 	      }
 	  bfd_set_reloc (abfd, sec, relpp, relcount);
 	}

-- 
Alan Modra
Australia Development Lab, IBM

Re: deleting relocs, objcopy and BFD

[Jose E. Marchesi]

    >   - The deletion logic in `merge_gnu_build_notes' then deletes two
    >     internal relocs, leaving 7.  This sounds like a sequence of three
    >     internal relocs gets broken.  I am not sure if this really a problem
    >     (don't know much of mips64) but it is worth a check.
    
    This part of the problem is simply an error in Nick's code, fixed as
    follows.

Ah, nice, thanks.  I thought that if the code was deleting relocs
defined on the same address, they all should be removed, or none.

    I've read over your description of the reloc count problem and am
    not too sure what to do, but my inclination would be to not return a
    count from bfd_canonicalize_reloc and not set one in bfd_set_reloc.
    ie. rely on the sentinel.  That will mean rewriting some code..

I am exploring that approach.

Re: deleting relocs, objcopy and BFD

[Maciej W. Rozycki]

On Thu, 27 Apr 2017, Jose E. Marchesi wrote:

> The recent commit
> 
> commit 1d15e434f43bc41a07bc7b0648fcb7e6ccbe8dcc
> Author: Nick Clifton <nickc@redhat.com>
> Date:   Thu Apr 13 14:50:56 2017 +0100
> 
>     Add note merging to strip and add code to merge stack size notes.
> 
>     * objcopy.c: Add --no-merge-notes option to disable note merging.
>       Add --[no-]merge-notes option to strip, and enable it by default.
>       (num_bytes): New function.
>       (merge_gnu_build_notes): Add code to merge stack size notes.
>     * binutils.texi: Update strip and objcopy documentation.
>     * readelf.c (print_gnu_build_attribute_name): Use defined
>       constants for note types.
> 
> Introduced a regression in two ELF targets: elf64-sparc and elf64-mips:
> 
> FAIL: merge notes section (64-bits)
[...]
> For example:
> 
> 1. Build binutils with --target=mips64-unknown-openbsd
> 
> 2. ./gas/as-new binutils/testsuite/binutils-all/note-2-64.s -o foo.o
> 
> 3. ./binutils/objcopy --merge-notes foo.o bar.o will segfault.
> 
> 4. Run the above command under GDB and check that:
> 
>    - At the beginning of `merge_gnu_build_notes' the
>     .gnu_build_attributes section has 3 external relocations.  This
>     makes `bfd_canonicalize_reloc' to return a count of 3 * 3 = 9
>     internal relocs.
> 
>   - The deletion logic in `merge_gnu_build_notes' then deletes two
>     internal relocs, leaving 7.  This sounds like a sequence of three
>     internal relocs gets broken.  I am not sure if this really a problem
>     (don't know much of mips64) but it is worth a check.

 Thanks for raising this issue.  I actually noticed the regression, but 
incorrectly attributed it to the usual test framework breakage (that I 
mean to address in a systematic way sometime) for MIPS targets that 
default to n64.  I'll see what I can do about it unless Alan or Nick beat 
me to it.

  Maciej

Re: deleting relocs, objcopy and BFD

[Nick Clifton]

Hi Guys,

On 01/05/17 05:45, Alan Modra wrote:

> This part of the problem is simply an error in Nick's code, fixed as
> follows. 

Doh - thanks for fixing this.

Cheers
  Nick

Re: deleting relocs, objcopy and BFD

[Nick Clifton]

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

Hi Guys,

> Problem 1: incoherence with internal relocs and external relocs breaks> the API.
I am checking in the attached patch in order to stop objcopy from attempting
to merge notes when the internal reloc count is greater than the external 
reloc count.  This does not fix the underlying problem, but it should at
least make objcopy safe, for now.

> b) To remove the API limitation/bug in BFD, somehow.  Maybe adding
>     end-of-list sentinels to `sec->relocation' and `sec->orelocation',
>     adjusting `bfd_set_reloc' to install it according to its `relcount'
>     argument, leaving `sec->reloc_count' untouched, and also making
>     `elfNN_BE_write_relocs' to use the sentinel when writing.

I think that option b) would be better.  I wonder though whether it might
be simpler to just let targets override the bfd_set_reloc () function with
their own implementation, should they have special requirements.  I have not
investigated this yet, but it seems like it would be the simplest solution,
provided that it can be made to work.

Cheers
  Nick

[-- Attachment #2: objcopy.reloc-count.patch --]
[-- Type: text/x-patch, Size: 697 bytes --]

diff --git a/binutils/objcopy.c b/binutils/objcopy.c
index 42c7775..36952ec 100644
--- a/binutils/objcopy.c
+++ b/binutils/objcopy.c
@@ -2137,6 +2137,13 @@ merge_gnu_build_notes (bfd * abfd, asection * sec, bfd_size_type size, bfd_byte
 	    relcount = 0;
 	}
 
+      /* A few targets (eg MIPS, SPARC) create multiple internal relocs to
+	 represent a single external reloc.  Unfortunately the current BFD
+	 API does not handle deleting relocs in such situations very well
+	 and so it is unsafe to proceed.  */
+      if (relcount > sec->reloc_count)
+	goto done;
+
       /* Eliminate the duplicates.  */
       new = new_contents = xmalloc (size);
       for (pnote = pnotes, old = contents;

Re: deleting relocs, objcopy and BFD

[Jose E. Marchesi]

Hi Nick.
    
    > Problem 1: incoherence with internal relocs and external relocs breaks> the API.
    I am checking in the attached patch in order to stop objcopy from attempting
    to merge notes when the internal reloc count is greater than the external 
    reloc count.  This does not fix the underlying problem, but it should at
    least make objcopy safe, for now.

That hack doesn't fix the problem for SPARC, nor the regression in the
testsuite.

This is because in SPARC in most cases (everything other than
R_SPARC_OLO10) there are the same number of internal relocs than
external relocs, and thus `relcount == sec->reloc_count' in this case.

However, in SPARC bfd_canonicalize_reloc always returns
((struct _bfd_sparc_elf_section_data *) elf_section_data (sec))->reloc_count
and not sec->reloc_count, so the segfault is still there..
    
    > b) To remove the API limitation/bug in BFD, somehow.  Maybe adding
    >     end-of-list sentinels to `sec->relocation' and `sec->orelocation',
    >     adjusting `bfd_set_reloc' to install it according to its `relcount'
    >     argument, leaving `sec->reloc_count' untouched, and also making
    >     `elfNN_BE_write_relocs' to use the sentinel when writing.
    
    I think that option b) would be better.  I wonder though whether it might
    be simpler to just let targets override the bfd_set_reloc () function with
    their own implementation, should they have special requirements.  I have not
    investigated this yet, but it seems like it would be the simplest solution,
    provided that it can be made to work.

Yeah...  I agree it would probably be easier, and keeps the current API,
so no code rewrite is hopefully needed.

If nobody objects to this approach I will give it a try.

Re: deleting relocs, objcopy and BFD

[H.J. Lu]

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

On Tue, May 2, 2017 at 7:50 AM, Nick Clifton <nickc@redhat.com> wrote:
> Hi Guys,
>
>> Problem 1: incoherence with internal relocs and external relocs breaks> the API.
> I am checking in the attached patch in order to stop objcopy from attempting
> to merge notes when the internal reloc count is greater than the external
> reloc count.  This does not fix the underlying problem, but it should at
> least make objcopy safe, for now.
>
>> b) To remove the API limitation/bug in BFD, somehow.  Maybe adding
>>     end-of-list sentinels to `sec->relocation' and `sec->orelocation',
>>     adjusting `bfd_set_reloc' to install it according to its `relcount'
>>     argument, leaving `sec->reloc_count' untouched, and also making
>>     `elfNN_BE_write_relocs' to use the sentinel when writing.
>
> I think that option b) would be better.  I wonder though whether it might
> be simpler to just let targets override the bfd_set_reloc () function with
> their own implementation, should they have special requirements.  I have not
> investigated this yet, but it seems like it would be the simplest solution,
> provided that it can be made to work.
>

I checked in this to fix build on 32-bit hosts.

-- 
H.J.

[-- Attachment #2: 0001-Cast-relcount-to-unsigned-long-when-comparing-with-s.patch --]
[-- Type: text/x-patch, Size: 1763 bytes --]

From 2ecf0cc317d065cfeb960c61688897351521bce0 Mon Sep 17 00:00:00 2001
From: "H.J. Lu" <hjl.tools@gmail.com>
Date: Tue, 2 May 2017 12:16:26 -0700
Subject: [PATCH] Cast relcount to unsigned long when comparing with
 sec->reloc_count

The type of relcount is long and the type of sec->reloc_count is
unsigned int.  On 32-bit hosts, GCC issues an error:

objcopy.c:2144:20: error: comparison between signed and unsigned integer expressions [-Werror=sign-compare]
       if (relcount > sec->reloc_count)

Cast relcount to unsigned long to silence GCC.

	* objcopy.c (merge_gnu_build_notes): Cast relcount to unsigned
	long when comparing with sec->reloc_count.
---
 binutils/ChangeLog | 5 +++++
 binutils/objcopy.c | 2 +-
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/binutils/ChangeLog b/binutils/ChangeLog
index d13dbb6..b3a539a 100644
--- a/binutils/ChangeLog
+++ b/binutils/ChangeLog
@@ -1,3 +1,8 @@
+2017-05-02  H.J. Lu  <hongjiu.lu@intel.com>
+
+	* objcopy.c (merge_gnu_build_notes): Cast relcount to unsigned
+	long when comparing with sec->reloc_count.
+
 2017-05-02  Nick Clifton  <nickc@redhat.com>
 
 	* objcopy.c (merge_gnu_build_notes): Disable merge if there are
diff --git a/binutils/objcopy.c b/binutils/objcopy.c
index 36952ec..ccb5e12 100644
--- a/binutils/objcopy.c
+++ b/binutils/objcopy.c
@@ -2141,7 +2141,7 @@ merge_gnu_build_notes (bfd * abfd, asection * sec, bfd_size_type size, bfd_byte
 	 represent a single external reloc.  Unfortunately the current BFD
 	 API does not handle deleting relocs in such situations very well
 	 and so it is unsafe to proceed.  */
-      if (relcount > sec->reloc_count)
+      if ((unsigned long) relcount > sec->reloc_count)
 	goto done;
 
       /* Eliminate the duplicates.  */
-- 
2.9.3