| Message ID | 56727936.4030605@samsung.com |
|---|---|
| State | New |
| Headers | show |
On Thu, Dec 17, 2015 at 11:58:30AM +0300, Yury Gribov wrote: > 2015-12-17 Yury Gribov <tetra2005@gmail.com> > > * c-family/c-common.c (resort_field_decl_cmp): > Make symmteric. > * cp/class.c (method_name_cmp): Ditto. > (resort_method_name_cmp): Ditto. > * fortran/interface.c (pair_cmp): Ditto. Note, c-family, cp and fortran have their own ChangeLog files, so the entries without those prefixes need to go into each one and can't refer to other ChangeLog through Ditto/Likewise etc. Typo in symmteric. That said, is this actually really a problem? I mean, is qsort allowed to call the comparison function with the same arguments? I think lots of the comparison functions just assume that for int cmpfn (const void *x, const void *y) x != y. And if qsort can't call the comparison function with the same argument, then perhaps the caller has some knowledge your checker does not, say that the entries that would compare equal by the comparison function simply can't appear in the array (so the caller knows that the comparison function should never return 0). > --- a/gcc/tree-vrp.c > +++ b/gcc/tree-vrp.c > @@ -5882,7 +5882,9 @@ compare_case_labels (const void *p1, const void *p2) > else if (idx1 == idx2) > { > /* Make sure the default label is first in a group. */ > - if (!CASE_LOW (ci1->expr)) > + if (!CASE_LOW (ci1->expr) && !CASE_LOW (ci2->expr)) > + return 0; > + else if (!CASE_LOW (ci1->expr)) > return -1; > else if (!CASE_LOW (ci2->expr)) > return 1; > -- > 1.9.1 Say here, we know there is at most one default label in a switch, never more. So, unless qsort is allowed to call compare_case_labels with p1 == p2 (which really doesn't make sense), this case just won't happen. Jakub
On Thu, 17 Dec 2015, Yury Gribov wrote: > Some obvious symmetry fixes. > > Cc-ing > * Andrey (Belevantsev) for bb_top_order_comparator > * Andrew (MacLeod) for compare_case_labels > * Andrew (Pinski) for resort_field_decl_cmp > * Diego for pair_cmp > * Geoff for resort_method_name_cmp > * Jakub for compare_case_labels > * Jason for method_name_cmp > * Richard for insert_phi_nodes_compare_var_infos, compare_case_labels > * Steven for cmp_v_in_regset_pool So for compare_case_labels we only ever have one label with !CASE_LOW - which means you only run into the case that needs !CASE_LOW && !CASE_LOW if comparing an element with itself, correct? In this case (missing "same element" handling rather than symmetry fixing) I'd prefer a if (case1 == case2) return 0; So just to confirm - do the patches also contain same element compare fixings? Richard.
On 12/17/2015 02:41 PM, Richard Biener wrote: > On Thu, 17 Dec 2015, Yury Gribov wrote: > >> Some obvious symmetry fixes. >> >> Cc-ing >> * Andrey (Belevantsev) for bb_top_order_comparator >> * Andrew (MacLeod) for compare_case_labels >> * Andrew (Pinski) for resort_field_decl_cmp >> * Diego for pair_cmp >> * Geoff for resort_method_name_cmp >> * Jakub for compare_case_labels >> * Jason for method_name_cmp >> * Richard for insert_phi_nodes_compare_var_infos, compare_case_labels >> * Steven for cmp_v_in_regset_pool > > So for compare_case_labels we only ever have one label with > !CASE_LOW - which means you only run into the case that needs > !CASE_LOW && !CASE_LOW if comparing an element with itself, correct? > > In this case (missing "same element" handling rather than symmetry > fixing) I'd prefer a > > if (case1 == case2) > return 0; > > So just to confirm - do the patches also contain same element > compare fixings? Yes, that's a fix for same element. How about adding if + gcc_assert that both cases can't be NULL otherwise? /Yury
On Thu, Dec 17, 2015 at 02:47:29PM +0300, Yury Gribov wrote: > On 12/17/2015 02:41 PM, Richard Biener wrote: > >On Thu, 17 Dec 2015, Yury Gribov wrote: > > > >>Some obvious symmetry fixes. > >> > >>Cc-ing > >>* Andrey (Belevantsev) for bb_top_order_comparator > >>* Andrew (MacLeod) for compare_case_labels > >>* Andrew (Pinski) for resort_field_decl_cmp > >>* Diego for pair_cmp > >>* Geoff for resort_method_name_cmp > >>* Jakub for compare_case_labels > >>* Jason for method_name_cmp > >>* Richard for insert_phi_nodes_compare_var_infos, compare_case_labels > >>* Steven for cmp_v_in_regset_pool > > > >So for compare_case_labels we only ever have one label with > >!CASE_LOW - which means you only run into the case that needs > >!CASE_LOW && !CASE_LOW if comparing an element with itself, correct? > > > >In this case (missing "same element" handling rather than symmetry > >fixing) I'd prefer a > > > > if (case1 == case2) > > return 0; > > > >So just to confirm - do the patches also contain same element > >compare fixings? > > Yes, that's a fix for same element. How about adding if + gcc_assert that > both cases can't be NULL otherwise? Some of the qsort comparison functions are hot paths, we don't really want to slow them down. So, if anything, gcc_checking_assert rather than gcc_assert. But, which qsort is so lame that it calls comparison on the same element? Jakub
Hello, On 17.12.2015 11:58, Yury Gribov wrote: > Some obvious symmetry fixes. > > Cc-ing > * Andrey (Belevantsev) for bb_top_order_comparator Here, as Jakub mentioned, we assume that the argument addresses will never be equal, thus that would always be different basic blocks (the comparator is used for providing a custom sort over loop body bbs) and you don't need a return 0 there. You can put there gcc_unreachable instead as in ... > * Andrew (MacLeod) for compare_case_labels > * Andrew (Pinski) for resort_field_decl_cmp > * Diego for pair_cmp > * Geoff for resort_method_name_cmp > * Jakub for compare_case_labels > * Jason for method_name_cmp > * Richard for insert_phi_nodes_compare_var_infos, compare_case_labels > * Steven for cmp_v_in_regset_pool ... this case -- here gcc_unreachable () marks that we're sorting pool pointers and their values are always different. Please do not remove it. Yours, Andrey > > /Yury
On Thu, 17 Dec 2015, Yury Gribov wrote: > On 12/17/2015 02:41 PM, Richard Biener wrote: > > On Thu, 17 Dec 2015, Yury Gribov wrote: > > > > > Some obvious symmetry fixes. > > > > > > Cc-ing > > > * Andrey (Belevantsev) for bb_top_order_comparator > > > * Andrew (MacLeod) for compare_case_labels > > > * Andrew (Pinski) for resort_field_decl_cmp > > > * Diego for pair_cmp > > > * Geoff for resort_method_name_cmp > > > * Jakub for compare_case_labels > > > * Jason for method_name_cmp > > > * Richard for insert_phi_nodes_compare_var_infos, compare_case_labels > > > * Steven for cmp_v_in_regset_pool > > > > So for compare_case_labels we only ever have one label with > > !CASE_LOW - which means you only run into the case that needs > > !CASE_LOW && !CASE_LOW if comparing an element with itself, correct? > > > > In this case (missing "same element" handling rather than symmetry > > fixing) I'd prefer a > > > > if (case1 == case2) > > return 0; > > > > So just to confirm - do the patches also contain same element > > compare fixings? > > Yes, that's a fix for same element. How about adding if + gcc_assert that > both cases can't be NULL otherwise? Well, does qsort require the compare function to result in zero for same elements when the sequence to be sorted doesn't contain duplicates? If an assert works for you that hints at these places found via static analysis rather than a runtime fuzzer? Richard.
On 12/17/2015 02:39 PM, Jakub Jelinek wrote: > On Thu, Dec 17, 2015 at 11:58:30AM +0300, Yury Gribov wrote: >> 2015-12-17 Yury Gribov <tetra2005@gmail.com> >> >> * c-family/c-common.c (resort_field_decl_cmp): >> Make symmteric. >> * cp/class.c (method_name_cmp): Ditto. >> (resort_method_name_cmp): Ditto. >> * fortran/interface.c (pair_cmp): Ditto. > > Note, c-family, cp and fortran have their own ChangeLog files, so > the entries without those prefixes need to go into each one and can't > refer to other ChangeLog through Ditto/Likewise etc. > Typo in symmteric. Right, thanks. > That said, is this actually really a problem? I mean, is qsort > allowed to call the comparison function with the same arguments? > I think lots of the comparison functions just assume that > for int cmpfn (const void *x, const void *y) x != y. > And if qsort can't call the comparison function with the same argument, > then perhaps the caller has some knowledge your checker does not, say > that the entries that would compare equal by the comparison function > simply can't appear in the array (so the caller knows that the comparison > function should never return 0). Self-comparisons are certainly less dangerous than transitive ones. I personally not aware about libc's which can compare element to itself. However * comparing an element to itself still a valid thing for qsort to do * most other comparison functions in GCC support this >> --- a/gcc/tree-vrp.c >> +++ b/gcc/tree-vrp.c >> @@ -5882,7 +5882,9 @@ compare_case_labels (const void *p1, const void *p2) >> else if (idx1 == idx2) >> { >> /* Make sure the default label is first in a group. */ >> - if (!CASE_LOW (ci1->expr)) >> + if (!CASE_LOW (ci1->expr) && !CASE_LOW (ci2->expr)) >> + return 0; >> + else if (!CASE_LOW (ci1->expr)) >> return -1; >> else if (!CASE_LOW (ci2->expr)) >> return 1; >> -- >> 1.9.1 > > Say here, we know there is at most one default label in a switch, never > more. So, unless qsort is allowed to call compare_case_labels > with p1 == p2 (which really doesn't make sense), this case just won't > happen.
On 12/17/2015 02:58 PM, Andrey Belevantsev wrote: > Hello, > > On 17.12.2015 11:58, Yury Gribov wrote: >> Some obvious symmetry fixes. >> >> Cc-ing >> * Andrey (Belevantsev) for bb_top_order_comparator > > Here, as Jakub mentioned, we assume that the argument addresses will > never be equal, The problem is that this is not guaranteed. > thus that would always be different basic blocks (the > comparator is used for providing a custom sort over loop body bbs) and > you don't need a return 0 there. You can put there gcc_unreachable > instead as in ... > >> * Andrew (MacLeod) for compare_case_labels >> * Andrew (Pinski) for resort_field_decl_cmp >> * Diego for pair_cmp >> * Geoff for resort_method_name_cmp >> * Jakub for compare_case_labels >> * Jason for method_name_cmp >> * Richard for insert_phi_nodes_compare_var_infos, compare_case_labels >> * Steven for cmp_v_in_regset_pool > > ... this case -- here gcc_unreachable () marks that we're sorting pool > pointers and their values are always different. Please do not remove it. Same here. /Yury
On 12/17/2015 02:59 PM, Richard Biener wrote: > On Thu, 17 Dec 2015, Yury Gribov wrote: > >> On 12/17/2015 02:41 PM, Richard Biener wrote: >>> On Thu, 17 Dec 2015, Yury Gribov wrote: >>> >>>> Some obvious symmetry fixes. >>>> >>>> Cc-ing >>>> * Andrey (Belevantsev) for bb_top_order_comparator >>>> * Andrew (MacLeod) for compare_case_labels >>>> * Andrew (Pinski) for resort_field_decl_cmp >>>> * Diego for pair_cmp >>>> * Geoff for resort_method_name_cmp >>>> * Jakub for compare_case_labels >>>> * Jason for method_name_cmp >>>> * Richard for insert_phi_nodes_compare_var_infos, compare_case_labels >>>> * Steven for cmp_v_in_regset_pool >>> >>> So for compare_case_labels we only ever have one label with >>> !CASE_LOW - which means you only run into the case that needs >>> !CASE_LOW && !CASE_LOW if comparing an element with itself, correct? >>> >>> In this case (missing "same element" handling rather than symmetry >>> fixing) I'd prefer a >>> >>> if (case1 == case2) >>> return 0; >>> >>> So just to confirm - do the patches also contain same element >>> compare fixings? >> >> Yes, that's a fix for same element. How about adding if + gcc_assert that >> both cases can't be NULL otherwise? > > Well, does qsort require the compare function to result in zero > for same elements when the sequence to be sorted doesn't contain > duplicates? Sure, that's part of total ordering requirement in standard. > If an assert works for you that hints at these places found via static > analysis rather than a runtime fuzzer? Sorry, not sure I fully understood but - yes, adding assertion would typically allow for better checking by static analyzers. /Yura
On Thu, 17 Dec 2015, Yury Gribov wrote: > On 12/17/2015 02:59 PM, Richard Biener wrote: > > On Thu, 17 Dec 2015, Yury Gribov wrote: > > > > > On 12/17/2015 02:41 PM, Richard Biener wrote: > > > > On Thu, 17 Dec 2015, Yury Gribov wrote: > > > > > > > > > Some obvious symmetry fixes. > > > > > > > > > > Cc-ing > > > > > * Andrey (Belevantsev) for bb_top_order_comparator > > > > > * Andrew (MacLeod) for compare_case_labels > > > > > * Andrew (Pinski) for resort_field_decl_cmp > > > > > * Diego for pair_cmp > > > > > * Geoff for resort_method_name_cmp > > > > > * Jakub for compare_case_labels > > > > > * Jason for method_name_cmp > > > > > * Richard for insert_phi_nodes_compare_var_infos, compare_case_labels > > > > > * Steven for cmp_v_in_regset_pool > > > > > > > > So for compare_case_labels we only ever have one label with > > > > !CASE_LOW - which means you only run into the case that needs > > > > !CASE_LOW && !CASE_LOW if comparing an element with itself, correct? > > > > > > > > In this case (missing "same element" handling rather than symmetry > > > > fixing) I'd prefer a > > > > > > > > if (case1 == case2) > > > > return 0; > > > > > > > > So just to confirm - do the patches also contain same element > > > > compare fixings? > > > > > > Yes, that's a fix for same element. How about adding if + gcc_assert that > > > both cases can't be NULL otherwise? > > > > Well, does qsort require the compare function to result in zero > > for same elements when the sequence to be sorted doesn't contain > > duplicates? > > Sure, that's part of total ordering requirement in standard. > > > If an assert works for you that hints at these places found via static > > analysis rather than a runtime fuzzer? > > Sorry, not sure I fully understood but - yes, adding assertion would typically > allow for better checking by static analyzers. The question was if you actually observed the case to happen with a testcase (and whatever mungled qsort implementation) or whether it was a theoretical outcome computed by a static analyzer. That is, whether you could hand me a testcase where it happens or not. Richard.
On 12/17/2015 03:25 PM, Richard Biener wrote: > On Thu, 17 Dec 2015, Yury Gribov wrote: > >> On 12/17/2015 02:59 PM, Richard Biener wrote: >>> On Thu, 17 Dec 2015, Yury Gribov wrote: >>> >>>> On 12/17/2015 02:41 PM, Richard Biener wrote: >>>>> On Thu, 17 Dec 2015, Yury Gribov wrote: >>>>> >>>>>> Some obvious symmetry fixes. >>>>>> >>>>>> Cc-ing >>>>>> * Andrey (Belevantsev) for bb_top_order_comparator >>>>>> * Andrew (MacLeod) for compare_case_labels >>>>>> * Andrew (Pinski) for resort_field_decl_cmp >>>>>> * Diego for pair_cmp >>>>>> * Geoff for resort_method_name_cmp >>>>>> * Jakub for compare_case_labels >>>>>> * Jason for method_name_cmp >>>>>> * Richard for insert_phi_nodes_compare_var_infos, compare_case_labels >>>>>> * Steven for cmp_v_in_regset_pool >>>>> >>>>> So for compare_case_labels we only ever have one label with >>>>> !CASE_LOW - which means you only run into the case that needs >>>>> !CASE_LOW && !CASE_LOW if comparing an element with itself, correct? >>>>> >>>>> In this case (missing "same element" handling rather than symmetry >>>>> fixing) I'd prefer a >>>>> >>>>> if (case1 == case2) >>>>> return 0; >>>>> >>>>> So just to confirm - do the patches also contain same element >>>>> compare fixings? >>>> >>>> Yes, that's a fix for same element. How about adding if + gcc_assert that >>>> both cases can't be NULL otherwise? >>> >>> Well, does qsort require the compare function to result in zero >>> for same elements when the sequence to be sorted doesn't contain >>> duplicates? >> >> Sure, that's part of total ordering requirement in standard. >> >>> If an assert works for you that hints at these places found via static >>> analysis rather than a runtime fuzzer? >> >> Sorry, not sure I fully understood but - yes, adding assertion would typically >> allow for better checking by static analyzers. > > The question was if you actually observed the case to happen with a > testcase (and whatever mungled qsort implementation) or whether > it was a theoretical outcome computed by a static analyzer. > > That is, whether you could hand me a testcase where it happens > or not. Well, this was detected by calling qsort(x, x) and checking that return value is zero in qsort interceptor. So I guess it's more of "theoretical" sort. /Yura
On 17.12.2015 15:13, Yury Gribov wrote: > On 12/17/2015 02:58 PM, Andrey Belevantsev wrote: >> Hello, >> >> On 17.12.2015 11:58, Yury Gribov wrote: >>> Some obvious symmetry fixes. >>> >>> Cc-ing >>> * Andrey (Belevantsev) for bb_top_order_comparator >> >> Here, as Jakub mentioned, we assume that the argument addresses will >> never be equal, > > The problem is that this is not guaranteed. Well, if the consensus is that this is indeed the case, you're free to change both places as you suggest. Yours, Andrey > >> thus that would always be different basic blocks (the >> comparator is used for providing a custom sort over loop body bbs) and >> you don't need a return 0 there. You can put there gcc_unreachable >> instead as in ... >> >>> * Andrew (MacLeod) for compare_case_labels >>> * Andrew (Pinski) for resort_field_decl_cmp >>> * Diego for pair_cmp >>> * Geoff for resort_method_name_cmp >>> * Jakub for compare_case_labels >>> * Jason for method_name_cmp >>> * Richard for insert_phi_nodes_compare_var_infos, compare_case_labels >>> * Steven for cmp_v_in_regset_pool >> >> ... this case -- here gcc_unreachable () marks that we're sorting pool >> pointers and their values are always different. Please do not remove it. > > Same here. > > /Yury
On Thu, Dec 17, 2015 at 12:58 AM, Yury Gribov <y.gribov@samsung.com> wrote: > Some obvious symmetry fixes. > > Cc-ing > * Andrey (Belevantsev) for bb_top_order_comparator > * Andrew (MacLeod) for compare_case_labels > * Andrew (Pinski) for resort_field_decl_cmp IIRC this was actually not written by me but I copied it back from an older version. But then again this was over 10 years ago so I don't remember the history on this any more. Thanks, Andrew > * Diego for pair_cmp > * Geoff for resort_method_name_cmp > * Jakub for compare_case_labels > * Jason for method_name_cmp > * Richard for insert_phi_nodes_compare_var_infos, compare_case_labels > * Steven for cmp_v_in_regset_pool > > /Yury
The C++ changes are also for handling comparing an element to itself, which shouldn't happen; I'd prefer a gcc_checking_assert that it doesn't. Jason
On 12/17/2015 03:04 PM, Yury Gribov wrote: > On 12/17/2015 02:39 PM, Jakub Jelinek wrote: >> On Thu, Dec 17, 2015 at 11:58:30AM +0300, Yury Gribov wrote: >>> 2015-12-17 Yury Gribov <tetra2005@gmail.com> >>> >>> * c-family/c-common.c (resort_field_decl_cmp): >>> Make symmteric. >>> * cp/class.c (method_name_cmp): Ditto. >>> (resort_method_name_cmp): Ditto. >>> * fortran/interface.c (pair_cmp): Ditto. >> >> Note, c-family, cp and fortran have their own ChangeLog files, so >> the entries without those prefixes need to go into each one and can't >> refer to other ChangeLog through Ditto/Likewise etc. >> Typo in symmteric. > > Right, thanks. > >> That said, is this actually really a problem? I mean, is qsort >> allowed to call the comparison function with the same arguments? >> I think lots of the comparison functions just assume that >> for int cmpfn (const void *x, const void *y) x != y. >> And if qsort can't call the comparison function with the same argument, >> then perhaps the caller has some knowledge your checker does not, say >> that the entries that would compare equal by the comparison function >> simply can't appear in the array (so the caller knows that the comparison >> function should never return 0). > > Self-comparisons are certainly less dangerous than transitive ones. I > personally not aware about libc's which can compare element to itself. Jakub, So it seems most people generally agree that self-comparisons (cmp(x,x) == 0) are useless and don't need to be checked or fixed. What about ensuring symmetry i.e. that cmp(x, y) == -cmp(y, x) forall x, y? One of the bugs (pair_cmp in fortran/interface.c) is exactly about this. > However > * comparing an element to itself still a valid thing for qsort to do > * most other comparison functions in GCC support this > >>> --- a/gcc/tree-vrp.c >>> +++ b/gcc/tree-vrp.c >>> @@ -5882,7 +5882,9 @@ compare_case_labels (const void *p1, const void >>> *p2) >>> else if (idx1 == idx2) >>> { >>> /* Make sure the default label is first in a group. */ >>> - if (!CASE_LOW (ci1->expr)) >>> + if (!CASE_LOW (ci1->expr) && !CASE_LOW (ci2->expr)) >>> + return 0; >>> + else if (!CASE_LOW (ci1->expr)) >>> return -1; >>> else if (!CASE_LOW (ci2->expr)) >>> return 1; >>> -- >>> 1.9.1 >> >> Say here, we know there is at most one default label in a switch, never >> more. So, unless qsort is allowed to call compare_case_labels >> with p1 == p2 (which really doesn't make sense), this case just won't >> happen. > > >
On Fri, Dec 18, 2015 at 10:40:40PM +0300, Yury Gribov wrote: > So it seems most people generally agree that self-comparisons (cmp(x,x) == > 0) are useless and don't need to be checked or fixed. What about ensuring > symmetry i.e. that cmp(x, y) == -cmp(y, x) forall x, y? One of the bugs > (pair_cmp in fortran/interface.c) is exactly about this. Ensuring symmetry for x != y is of course very much desirable. So, if you could change your qsort interposer so that it for each comparison x != y calls both cmp (x, y) and cmp (y, x) and asserts that int r = cmp (x, y); int ir = cmp (y, x); if (r > 0) assert (ir < 0); else if (r < 0) assert (ir > 0); else assert (ir == 0); it would be greatly appreciated. Note, the standard only talks about < 0, 0 and > 0, so it is fine if cmp (x, y) returns 231 and cmp (y, x) returns -142. Jakub
On 12/18/2015 11:07 PM, Jakub Jelinek wrote: > On Fri, Dec 18, 2015 at 10:40:40PM +0300, Yury Gribov wrote: >> So it seems most people generally agree that self-comparisons (cmp(x,x) == >> 0) are useless and don't need to be checked or fixed. What about ensuring >> symmetry i.e. that cmp(x, y) == -cmp(y, x) forall x, y? One of the bugs >> (pair_cmp in fortran/interface.c) is exactly about this. > > Ensuring symmetry for x != y is of course very much desirable. > So, if you could change your qsort interposer so that it for each comparison > x != y calls both cmp (x, y) and cmp (y, x) and asserts that > int r = cmp (x, y); > int ir = cmp (y, x); > if (r > 0) assert (ir < 0); > else if (r < 0) assert (ir > 0); > else assert (ir == 0); > it would be greatly appreciated. Note, the standard only talks about < 0, 0 > and > 0, so it is fine if cmp (x, y) returns 231 and cmp (y, x) returns -142. Sure, I've already bumped into this with other projects. I'll update my checker and get back with a reduced patchset then. /Yura
From bf924dca4ccc3f8640438400e923a4c508e898e0 Mon Sep 17 00:00:00 2001 From: Yury Gribov <tetra2005@gmail.com> Date: Sat, 12 Dec 2015 09:51:54 +0300 Subject: [PATCH 1/5] Fix asymmetric comparison functions. Qsort requires user-defined comparison function to be a total order. One of the requirements for this is being symmetric i.e. return inverse results on element swap. This patch fixes comparison functions to satisfy these conditions. 2015-12-17 Yury Gribov <tetra2005@gmail.com> * c-family/c-common.c (resort_field_decl_cmp): Make symmteric. * cp/class.c (method_name_cmp): Ditto. (resort_method_name_cmp): Ditto. * fortran/interface.c (pair_cmp): Ditto. * gimple.c (compare_case_labels): Ditto. * tree-into-ssa.c (insert_phi_nodes_compare_var_infos): Ditto. * tree-vrp.c (compare_case_labels): Ditto. * sel-sched-ir.c (cmp_v_in_regset_pool): Ditto. (bb_top_order_comparator): Ditto. --- gcc/c-family/c-common.c | 4 +++- gcc/cp/class.c | 10 ++++++---- gcc/fortran/interface.c | 6 +++++- gcc/gimple.c | 4 +++- gcc/sel-sched-ir.c | 5 +++-- gcc/tree-into-ssa.c | 5 +---- gcc/tree-vrp.c | 4 +++- 7 files changed, 24 insertions(+), 14 deletions(-) diff --git a/gcc/c-family/c-common.c b/gcc/c-family/c-common.c index 9bc02fc..eecdfb5 100644 --- a/gcc/c-family/c-common.c +++ b/gcc/c-family/c-common.c @@ -9956,8 +9956,10 @@ resort_field_decl_cmp (const void *x_p, const void *y_p) resort_data.new_value (&d2, resort_data.cookie); if (d1 < d2) return -1; + if (d1 > d2) + return 1; } - return 1; + return 0; } /* Resort DECL_SORTED_FIELDS because pointers have been reordered. */ diff --git a/gcc/cp/class.c b/gcc/cp/class.c index 216a301..3a740d2 100644 --- a/gcc/cp/class.c +++ b/gcc/cp/class.c @@ -2188,9 +2188,9 @@ method_name_cmp (const void* m1_p, const void* m2_p) return -1; if (*m2 == NULL_TREE) return 1; - if (DECL_NAME (OVL_CURRENT (*m1)) < DECL_NAME (OVL_CURRENT (*m2))) - return -1; - return 1; + tree d1 = DECL_NAME (OVL_CURRENT (*m1)); + tree d2 = DECL_NAME (OVL_CURRENT (*m2)); + return d1 < d2 ? -1 : d1 > d2 ? 1 : 0; } /* This routine compares two fields like method_name_cmp but using the @@ -2214,8 +2214,10 @@ resort_method_name_cmp (const void* m1_p, const void* m2_p) resort_data.new_value (&d2, resort_data.cookie); if (d1 < d2) return -1; + if (d1 > d2) + return 1; } - return 1; + return 0; } /* Resort TYPE_METHOD_VEC because pointers have been reordered. */ diff --git a/gcc/fortran/interface.c b/gcc/fortran/interface.c index bfd5d36..e4b93c8 100644 --- a/gcc/fortran/interface.c +++ b/gcc/fortran/interface.c @@ -3109,7 +3109,11 @@ pair_cmp (const void *p1, const void *p2) } if (a2->expr->expr_type != EXPR_VARIABLE) return 1; - return a1->expr->symtree->n.sym < a2->expr->symtree->n.sym; + if (a1->expr->symtree->n.sym < a2->expr->symtree->n.sym) + return 1; + if (a1->expr->symtree->n.sym > a2->expr->symtree->n.sym) + return -1; + return 0; } diff --git a/gcc/gimple.c b/gcc/gimple.c index bf552a7..51f515e 100644 --- a/gcc/gimple.c +++ b/gcc/gimple.c @@ -2774,7 +2774,9 @@ compare_case_labels (const void *p1, const void *p2) const_tree const case2 = *(const_tree const*)p2; /* The 'default' case label always goes first. */ - if (!CASE_LOW (case1)) + if (!CASE_LOW (case1) && !CASE_LOW (case2)) + return 0; + else if (!CASE_LOW (case1)) return -1; else if (!CASE_LOW (case2)) return 1; diff --git a/gcc/sel-sched-ir.c b/gcc/sel-sched-ir.c index 2a9aa10..2f53d22 100644 --- a/gcc/sel-sched-ir.c +++ b/gcc/sel-sched-ir.c @@ -959,7 +959,7 @@ cmp_v_in_regset_pool (const void *x, const void *xx) return 1; else if (r1 < r2) return -1; - gcc_unreachable (); + return 0; } /* Free the regset pool possibly checking for memory leaks. */ @@ -5935,8 +5935,9 @@ bb_top_order_comparator (const void *x, const void *y) bbs with greater number should go earlier. */ if (rev_top_order_index[bb1->index] > rev_top_order_index[bb2->index]) return -1; - else + else if (rev_top_order_index[bb1->index] < rev_top_order_index[bb2->index]) return 1; + return 0; } /* Create a region for LOOP and return its number. If we don't want diff --git a/gcc/tree-into-ssa.c b/gcc/tree-into-ssa.c index 5486d5c..f3b8c02 100644 --- a/gcc/tree-into-ssa.c +++ b/gcc/tree-into-ssa.c @@ -1041,10 +1041,7 @@ insert_phi_nodes_compare_var_infos (const void *a, const void *b) { const var_info *defa = *(var_info * const *)a; const var_info *defb = *(var_info * const *)b; - if (DECL_UID (defa->var) < DECL_UID (defb->var)) - return -1; - else - return 1; + return DECL_UID (defa->var) - DECL_UID (defb->var); } /* Insert PHI nodes at the dominance frontier of blocks with variable diff --git a/gcc/tree-vrp.c b/gcc/tree-vrp.c index acbb70b..cd4eec8 100644 --- a/gcc/tree-vrp.c +++ b/gcc/tree-vrp.c @@ -5882,7 +5882,9 @@ compare_case_labels (const void *p1, const void *p2) else if (idx1 == idx2) { /* Make sure the default label is first in a group. */ - if (!CASE_LOW (ci1->expr)) + if (!CASE_LOW (ci1->expr) && !CASE_LOW (ci2->expr)) + return 0; + else if (!CASE_LOW (ci1->expr)) return -1; else if (!CASE_LOW (ci2->expr)) return 1; -- 1.9.1