[roland/objcopy-merge-notes-qsort] Fix objcopy --merge-notes dependency on qsort implementation behavior.

Message ID CAB=4xhpfsDVkBpqsS2AQrb9ow8omU86ozxzv37FeJYxyM9UYDw@mail.gmail.com
State New
Headers show
Series
  • [roland/objcopy-merge-notes-qsort] Fix objcopy --merge-notes dependency on qsort implementation behavior.
Related show

Commit Message

Jose E. Marchesi via Binutils Jan. 27, 2020, 12:04 a.m.
The comparison function used in the sort to prepare to merge
.gnu.build.attributes notes returns different results for a comparison
of overlapping ranges depending on the order of the arguments, i.e.
compare(a, b) == -compare(b, a) does not hold.  This means the sort
order that results depends on the system C library's qsort function's
implementation behavior in what calls it makes to the comparison
function.  The practical result for me was that the note-6-64.d test
case failed in a macOS-hosted build when it worked in a Linux-hosted
build.

This change makes the behavior consistent with what happens on Linux,
which is what the test suite expects.  However, note that this appears
to contradict the behavior described in the comments: In the test case
there is a func entry (result of merging two adjacent func entries
under rule 2b) wholly contained by the range of an OPEN entry (result
of merging two adjacent OPEN entries under rule 2b).  The comments
seem to say that this func entry should have been removed under rule
2a (fully covered range, regardless of type).  This is the behavior I
saw (breaking the test suite) on the macOS build before this change.
If that is indeed the correct behavior, then some different fix is
needed to achieve that and the test suite needs to change its
expectations (though certainly a fix like this one is also
appropriate--the compare(a,b) == -compare(b, a) invariant should
always hold for any qsort callback!).  If not, then the comment should
be clarified to say that rule 2a does not apply to entries of
different types.

Ok for trunk and 2.34?  (Otherwise if the test's expectation should
indeed change, please fix it on both branches!)


Thanks,
Roland


binutils/
2020-01-26  Roland McGrath  <mcgrathr@google.com>

        * objcopy.c (compare_gnu_build_notes): Fix comparison results
        for overlapping ranges so that (A == B) == (B == A) holds.

Comments

Nick Clifton Jan. 27, 2020, 12:54 p.m. | #1
Hi Roland,

> The comparison function used in the sort to prepare to merge

> .gnu.build.attributes notes returns different results for a comparison

> of overlapping ranges depending on the order of the arguments, i.e.

> compare(a, b) == -compare(b, a) does not hold.


Oops - that is embarrassing.


> This change makes the behavior consistent with what happens on Linux,

> which is what the test suite expects.  However, note that this appears

> to contradict the behavior described in the comments: In the test case

> there is a func entry (result of merging two adjacent func entries

> under rule 2b) wholly contained by the range of an OPEN entry (result

> of merging two adjacent OPEN entries under rule 2b).  The comments

> seem to say that this func entry should have been removed under rule

> 2a (fully covered range, regardless of type).  This is the behavior I

> saw (breaking the test suite) on the macOS build before this change.

> If that is indeed the correct behavior,


It is.  Or at least that is what should happen for an optimal merge.
It is not an error for the notes not to be merged, it is just a case
that an opportunity for merging was missed.


> then some different fix is

> needed to achieve that and the test suite needs to change its

> expectations


If you would like to look into this and suggest a patch then please
do.  Otherwise I will add it to my list of things to do.


> (though certainly a fix like this one is also

> appropriate--the compare(a,b) == -compare(b, a) invariant should

> always hold for any qsort callback!).


> Ok for trunk and 2.34? 


Yes for both - please apply.

Cheers
  Nick
Jose E. Marchesi via Binutils Jan. 27, 2020, 7:19 p.m. | #2
On Mon, Jan 27, 2020 at 4:54 AM Nick Clifton <nickc@redhat.com> wrote:
> It is.  Or at least that is what should happen for an optimal merge.

> It is not an error for the notes not to be merged, it is just a case

> that an opportunity for merging was missed.


Ok.  That's what I thought.  Thanks for the clarification.

> > then some different fix is

> > needed to achieve that and the test suite needs to change its

> > expectations

>

> If you would like to look into this and suggest a patch then please

> do.  Otherwise I will add it to my list of things to do.


I'm not going to work on this, sorry.

> Yes for both - please apply.


Done.

Thanks,
Roland

Patch

diff --git a/binutils/objcopy.c b/binutils/objcopy.c
index ef3b693be49..212e25144e6 100644
--- a/binutils/objcopy.c
+++ b/binutils/objcopy.c
@@ -2052,6 +2052,8 @@  compare_gnu_build_notes (const void * data1, const void *$
     return -1;
   if (pnote1->end > pnote2->end)
     return 1;
+  if (pnote1->end < pnote2->end)
+    return -1;

   /* Put OPEN notes before function notes.  */
   if (is_open_note (pnote1) && ! is_open_note (pnote2))