| Message ID | 87dfa677-e748-ff53-28c8-ef24c9f4f553@suse.cz |
|---|---|
| State | New |
| Headers | show |
| Series | Fix emission of exception dispatch (PR middle-end/82154). | expand |
On 09/12/2017 01:43 AM, Martin Liška wrote: > Hello. > > In transition to simple_case_node, I forgot to initialize m_high to m_low if a case > does not have CASE_HIGH. > > Patch can bootstrap on ppc64le-redhat-linux and survives regression tests. > > Ready to be installed? > Martin > > gcc/ChangeLog: > > 2017-09-11 Martin Liska <mliska@suse.cz> > > PR middle-end/82154 > * stmt.c (struct simple_case_node): Assign low to high when > high is equal to null. > > gcc/testsuite/ChangeLog: > > 2017-09-11 Martin Liska <mliska@suse.cz> > > PR middle-end/82154 > * g++.dg/torture/pr82154.C: New test. OK. THough I have to wonder if we should unify the HIGH handling -- having two different conventions is bound to be confusing. In a CASE_LABEL_EXPR the CASE_HIGH can be NULL, which means the label refers to a singleton value that is found in CASE_LOW. That means we end up doing stuff like this: if (CASE_HIGH (elt)) maxval = fold_convert (index_type, CASE_HIGH (elt)); else maxval = fold_convert (index_type, CASE_LOW (elt)); You could legitimately argue for changing how this works for tree nodes so that there's always a non-null CASE_HIGH. jeff
On 09/12/2017 05:21 PM, Jeff Law wrote: > On 09/12/2017 01:43 AM, Martin Liška wrote: >> Hello. >> >> In transition to simple_case_node, I forgot to initialize m_high to m_low if a case >> does not have CASE_HIGH. >> >> Patch can bootstrap on ppc64le-redhat-linux and survives regression tests. >> >> Ready to be installed? >> Martin >> >> gcc/ChangeLog: >> >> 2017-09-11 Martin Liska <mliska@suse.cz> >> >> PR middle-end/82154 >> * stmt.c (struct simple_case_node): Assign low to high when >> high is equal to null. >> >> gcc/testsuite/ChangeLog: >> >> 2017-09-11 Martin Liska <mliska@suse.cz> >> >> PR middle-end/82154 >> * g++.dg/torture/pr82154.C: New test. > OK. > > THough I have to wonder if we should unify the HIGH handling -- having > two different conventions is bound to be confusing. > > In a CASE_LABEL_EXPR the CASE_HIGH can be NULL, which means the label > refers to a singleton value that is found in CASE_LOW. > > That means we end up doing stuff like this: > > if (CASE_HIGH (elt)) > maxval = fold_convert (index_type, CASE_HIGH (elt)); > else > maxval = fold_convert (index_type, CASE_LOW (elt)); > > > > You could legitimately argue for changing how this works for tree nodes > so that there's always a non-null CASE_HIGH. Hi. Agree with you that we have a lot of code that does what you just described. I tent to change IL representation, where CASE_HIGH is always non-null. $ git grep 'CASE_HIGH\>' | wc -l 125 Which is reasonable amount of code that should be changed. I'll prepare patch for that. Martin > > jeff >
On 09/13/2017 03:08 PM, Martin Liška wrote: > On 09/12/2017 05:21 PM, Jeff Law wrote: >> On 09/12/2017 01:43 AM, Martin Liška wrote: >>> Hello. >>> >>> In transition to simple_case_node, I forgot to initialize m_high to m_low if a case >>> does not have CASE_HIGH. >>> >>> Patch can bootstrap on ppc64le-redhat-linux and survives regression tests. >>> >>> Ready to be installed? >>> Martin >>> >>> gcc/ChangeLog: >>> >>> 2017-09-11 Martin Liska <mliska@suse.cz> >>> >>> PR middle-end/82154 >>> * stmt.c (struct simple_case_node): Assign low to high when >>> high is equal to null. >>> >>> gcc/testsuite/ChangeLog: >>> >>> 2017-09-11 Martin Liska <mliska@suse.cz> >>> >>> PR middle-end/82154 >>> * g++.dg/torture/pr82154.C: New test. >> OK. >> >> THough I have to wonder if we should unify the HIGH handling -- having >> two different conventions is bound to be confusing. >> >> In a CASE_LABEL_EXPR the CASE_HIGH can be NULL, which means the label >> refers to a singleton value that is found in CASE_LOW. >> >> That means we end up doing stuff like this: >> >> if (CASE_HIGH (elt)) >> maxval = fold_convert (index_type, CASE_HIGH (elt)); >> else >> maxval = fold_convert (index_type, CASE_LOW (elt)); >> >> >> >> You could legitimately argue for changing how this works for tree nodes >> so that there's always a non-null CASE_HIGH. > > Hi. > > Agree with you that we have a lot of code that does what you just described. > I tent to change IL representation, where CASE_HIGH is always non-null. > > $ git grep 'CASE_HIGH\>' | wc -l > 125 > > Which is reasonable amount of code that should be changed. I'll prepare patch for that. Hm, there's one question that pops up: Should we compare the values via pointer equality, or tree_int_cst_eq should be done? The later one can messy the code a bit. Martin > > Martin > >> >> jeff >> >
On 09/13/2017 07:42 AM, Martin Liška wrote: > On 09/13/2017 03:08 PM, Martin Liška wrote: >> On 09/12/2017 05:21 PM, Jeff Law wrote: >>> On 09/12/2017 01:43 AM, Martin Liška wrote: >>>> Hello. >>>> >>>> In transition to simple_case_node, I forgot to initialize m_high to m_low if a case >>>> does not have CASE_HIGH. >>>> >>>> Patch can bootstrap on ppc64le-redhat-linux and survives regression tests. >>>> >>>> Ready to be installed? >>>> Martin >>>> >>>> gcc/ChangeLog: >>>> >>>> 2017-09-11 Martin Liska <mliska@suse.cz> >>>> >>>> PR middle-end/82154 >>>> * stmt.c (struct simple_case_node): Assign low to high when >>>> high is equal to null. >>>> >>>> gcc/testsuite/ChangeLog: >>>> >>>> 2017-09-11 Martin Liska <mliska@suse.cz> >>>> >>>> PR middle-end/82154 >>>> * g++.dg/torture/pr82154.C: New test. >>> OK. >>> >>> THough I have to wonder if we should unify the HIGH handling -- having >>> two different conventions is bound to be confusing. >>> >>> In a CASE_LABEL_EXPR the CASE_HIGH can be NULL, which means the label >>> refers to a singleton value that is found in CASE_LOW. >>> >>> That means we end up doing stuff like this: >>> >>> if (CASE_HIGH (elt)) >>> maxval = fold_convert (index_type, CASE_HIGH (elt)); >>> else >>> maxval = fold_convert (index_type, CASE_LOW (elt)); >>> >>> >>> >>> You could legitimately argue for changing how this works for tree nodes >>> so that there's always a non-null CASE_HIGH. >> >> Hi. >> >> Agree with you that we have a lot of code that does what you just described. >> I tent to change IL representation, where CASE_HIGH is always non-null. >> >> $ git grep 'CASE_HIGH\>' | wc -l >> 125 >> >> Which is reasonable amount of code that should be changed. I'll prepare patch for that. > > Hm, there's one question that pops up: Should we compare the values via pointer equality, > or tree_int_cst_eq should be done? The later one can messy the code a bit. I'd like to say pointer equality, but it's probably safer to use tree_int_cst_eq. jeff
On 09/13/2017 04:22 PM, Jeff Law wrote: > On 09/13/2017 07:42 AM, Martin Liška wrote: >> On 09/13/2017 03:08 PM, Martin Liška wrote: >>> On 09/12/2017 05:21 PM, Jeff Law wrote: >>>> On 09/12/2017 01:43 AM, Martin Liška wrote: >>>>> Hello. >>>>> >>>>> In transition to simple_case_node, I forgot to initialize m_high to m_low if a case >>>>> does not have CASE_HIGH. >>>>> >>>>> Patch can bootstrap on ppc64le-redhat-linux and survives regression tests. >>>>> >>>>> Ready to be installed? >>>>> Martin >>>>> >>>>> gcc/ChangeLog: >>>>> >>>>> 2017-09-11 Martin Liska <mliska@suse.cz> >>>>> >>>>> PR middle-end/82154 >>>>> * stmt.c (struct simple_case_node): Assign low to high when >>>>> high is equal to null. >>>>> >>>>> gcc/testsuite/ChangeLog: >>>>> >>>>> 2017-09-11 Martin Liska <mliska@suse.cz> >>>>> >>>>> PR middle-end/82154 >>>>> * g++.dg/torture/pr82154.C: New test. >>>> OK. >>>> >>>> THough I have to wonder if we should unify the HIGH handling -- having >>>> two different conventions is bound to be confusing. >>>> >>>> In a CASE_LABEL_EXPR the CASE_HIGH can be NULL, which means the label >>>> refers to a singleton value that is found in CASE_LOW. >>>> >>>> That means we end up doing stuff like this: >>>> >>>> if (CASE_HIGH (elt)) >>>> maxval = fold_convert (index_type, CASE_HIGH (elt)); >>>> else >>>> maxval = fold_convert (index_type, CASE_LOW (elt)); >>>> >>>> >>>> >>>> You could legitimately argue for changing how this works for tree nodes >>>> so that there's always a non-null CASE_HIGH. >>> >>> Hi. >>> >>> Agree with you that we have a lot of code that does what you just described. >>> I tent to change IL representation, where CASE_HIGH is always non-null. >>> >>> $ git grep 'CASE_HIGH\>' | wc -l >>> 125 >>> >>> Which is reasonable amount of code that should be changed. I'll prepare patch for that. >> >> Hm, there's one question that pops up: Should we compare the values via pointer equality, >> or tree_int_cst_eq should be done? The later one can messy the code a bit. > I'd like to say pointer equality, but it's probably safer to use > tree_int_cst_eq. > > jeff > Ok, so I'll put it on my TODO list as it will take some time to do the transformation. In order to fix the PR, I suggest following patch. Ready for trunk? Martin From 1cd8987ad3de4a7bd3e71014a0df9ccb3be40277 Mon Sep 17 00:00:00 2001 From: marxin <mliska@suse.cz> Date: Mon, 11 Sep 2017 13:34:41 +0200 Subject: [PATCH] Fix emission of exception dispatch (PR middle-end/82154). gcc/ChangeLog: 2017-09-11 Martin Liska <mliska@suse.cz> PR middle-end/82154 * stmt.c (expand_sjlj_dispatch_table): Use CASE_LOW when CASE_HIGH is NULL_TREE. gcc/testsuite/ChangeLog: 2017-09-11 Martin Liska <mliska@suse.cz> PR middle-end/82154 * g++.dg/torture/pr82154.C: New test. --- gcc/stmt.c | 6 ++-- gcc/testsuite/g++.dg/torture/pr82154.C | 50 ++++++++++++++++++++++++++++++++++ 2 files changed, 54 insertions(+), 2 deletions(-) create mode 100644 gcc/testsuite/g++.dg/torture/pr82154.C diff --git a/gcc/stmt.c b/gcc/stmt.c index 39d29ff3da9..92bd209ad64 100644 --- a/gcc/stmt.c +++ b/gcc/stmt.c @@ -1063,8 +1063,10 @@ expand_sjlj_dispatch_table (rtx dispatch_index, for (int i = ncases - 1; i >= 0; --i) { tree elt = dispatch_table[i]; - case_list.safe_push (simple_case_node (CASE_LOW (elt), - CASE_HIGH (elt), + tree high = CASE_HIGH (elt); + if (high == NULL_TREE) + high = CASE_LOW (elt); + case_list.safe_push (simple_case_node (CASE_LOW (elt), high, CASE_LABEL (elt))); } diff --git a/gcc/testsuite/g++.dg/torture/pr82154.C b/gcc/testsuite/g++.dg/torture/pr82154.C new file mode 100644 index 00000000000..f4e1c3ea139 --- /dev/null +++ b/gcc/testsuite/g++.dg/torture/pr82154.C @@ -0,0 +1,50 @@ +// { dg-do compile } +// { dg-additional-options "-Wno-deprecated" } + +namespace a { +int b; +class c +{ +}; +} +class g +{ +public: + g (); +}; +using a::b; +class d +{ +public: + d (); + void e (); +}; +class f +{ + d + i () + { + static d j; + } + int *k () throw (a::c); +}; + + +int *f::k () throw (a::c) +{ + static g h; + i (); + int l = 2; + while (l) + { + --l; + try + { + operator new (b); + } + catch (a::c) + { + } + } + i ().e (); +}
On 09/13/2017 12:09 PM, Martin Liška wrote: > On 09/13/2017 04:22 PM, Jeff Law wrote: >> On 09/13/2017 07:42 AM, Martin Liška wrote: >>> On 09/13/2017 03:08 PM, Martin Liška wrote: >>>> On 09/12/2017 05:21 PM, Jeff Law wrote: >>>>> On 09/12/2017 01:43 AM, Martin Liška wrote: >>>>>> Hello. >>>>>> >>>>>> In transition to simple_case_node, I forgot to initialize m_high to m_low if a case >>>>>> >>>>>> does not have CASE_HIGH. >>>>>> >>>>>> Patch can bootstrap on ppc64le-redhat-linux and survives regression tests. >>>>>> >>>>>> >>>>>> Ready to be installed? >>>>>> Martin >>>>>> >>>>>> gcc/ChangeLog: >>>>>> >>>>>> 2017-09-11 Martin Liska <mliska@suse.cz> >>>>>> >>>>>> PR middle-end/82154 >>>>>> * stmt.c (struct simple_case_node): Assign low to high when >>>>>> high is equal to null. >>>>>> >>>>>> gcc/testsuite/ChangeLog: >>>>>> >>>>>> 2017-09-11 Martin Liska <mliska@suse.cz> >>>>>> >>>>>> PR middle-end/82154 >>>>>> * g++.dg/torture/pr82154.C: New test. >>>>> OK. >>>>> >>>>> THough I have to wonder if we should unify the HIGH handling -- having >>>>> two different conventions is bound to be confusing. >>>>> >>>>> In a CASE_LABEL_EXPR the CASE_HIGH can be NULL, which means the label >>>>> refers to a singleton value that is found in CASE_LOW. >>>>> >>>>> That means we end up doing stuff like this: >>>>> >>>>> if (CASE_HIGH (elt)) >>>>> maxval = fold_convert (index_type, CASE_HIGH (elt)); >>>>> else >>>>> maxval = fold_convert (index_type, CASE_LOW (elt)); >>>>> >>>>> >>>>> >>>>> You could legitimately argue for changing how this works for tree nodes >>>>> >>>>> so that there's always a non-null CASE_HIGH. >>>> >>>> Hi. >>>> >>>> Agree with you that we have a lot of code that does what you just described. >>>> >>>> I tent to change IL representation, where CASE_HIGH is always non-null. >>>> >>>> $ git grep 'CASE_HIGH\>' | wc -l >>>> 125 >>>> >>>> Which is reasonable amount of code that should be changed. I'll prepare patch for that. >>>> >>> >>> Hm, there's one question that pops up: Should we compare the values via pointer equality, >>> >>> or tree_int_cst_eq should be done? The later one can messy the code a bit. >>> >> I'd like to say pointer equality, but it's probably safer to use >> tree_int_cst_eq. >> >> jeff >> > > Ok, so I'll put it on my TODO list as it will take some time to do the transformation. NP. There's nothing inherently wrong with the code today, it's just a cleanup (I hope :-) > > > In order to fix the PR, I suggest following patch. > > Ready for trunk? > Martin > > 0001-Fix-emission-of-exception-dispatch-PR-middle-end-821.patch > > > From 1cd8987ad3de4a7bd3e71014a0df9ccb3be40277 Mon Sep 17 00:00:00 2001 > From: marxin <mliska@suse.cz> > Date: Mon, 11 Sep 2017 13:34:41 +0200 > Subject: [PATCH] Fix emission of exception dispatch (PR middle-end/82154). > > gcc/ChangeLog: > > 2017-09-11 Martin Liska <mliska@suse.cz> > > PR middle-end/82154 > * stmt.c (expand_sjlj_dispatch_table): Use CASE_LOW when > CASE_HIGH is NULL_TREE. > > gcc/testsuite/ChangeLog: > > 2017-09-11 Martin Liska <mliska@suse.cz> > > PR middle-end/82154 > * g++.dg/torture/pr82154.C: New test. OK. Jeff
On 09/13/2017 08:41 PM, Jeff Law wrote: > On 09/13/2017 12:09 PM, Martin Liška wrote: >> On 09/13/2017 04:22 PM, Jeff Law wrote: >>> On 09/13/2017 07:42 AM, Martin Liška wrote: >>>> On 09/13/2017 03:08 PM, Martin Liška wrote: >>>>> On 09/12/2017 05:21 PM, Jeff Law wrote: >>>>>> On 09/12/2017 01:43 AM, Martin Liška wrote: >>>>>>> Hello. >>>>>>> >>>>>>> In transition to simple_case_node, I forgot to initialize m_high to m_low if a case >>>>>>> >>>>>>> does not have CASE_HIGH. >>>>>>> >>>>>>> Patch can bootstrap on ppc64le-redhat-linux and survives regression tests. >>>>>>> >>>>>>> >>>>>>> Ready to be installed? >>>>>>> Martin >>>>>>> >>>>>>> gcc/ChangeLog: >>>>>>> >>>>>>> 2017-09-11 Martin Liska <mliska@suse.cz> >>>>>>> >>>>>>> PR middle-end/82154 >>>>>>> * stmt.c (struct simple_case_node): Assign low to high when >>>>>>> high is equal to null. >>>>>>> >>>>>>> gcc/testsuite/ChangeLog: >>>>>>> >>>>>>> 2017-09-11 Martin Liska <mliska@suse.cz> >>>>>>> >>>>>>> PR middle-end/82154 >>>>>>> * g++.dg/torture/pr82154.C: New test. >>>>>> OK. >>>>>> >>>>>> THough I have to wonder if we should unify the HIGH handling -- having >>>>>> two different conventions is bound to be confusing. >>>>>> >>>>>> In a CASE_LABEL_EXPR the CASE_HIGH can be NULL, which means the label >>>>>> refers to a singleton value that is found in CASE_LOW. >>>>>> >>>>>> That means we end up doing stuff like this: >>>>>> >>>>>> if (CASE_HIGH (elt)) >>>>>> maxval = fold_convert (index_type, CASE_HIGH (elt)); >>>>>> else >>>>>> maxval = fold_convert (index_type, CASE_LOW (elt)); >>>>>> >>>>>> >>>>>> >>>>>> You could legitimately argue for changing how this works for tree nodes >>>>>> >>>>>> so that there's always a non-null CASE_HIGH. >>>>> >>>>> Hi. >>>>> >>>>> Agree with you that we have a lot of code that does what you just described. >>>>> >>>>> I tent to change IL representation, where CASE_HIGH is always non-null. >>>>> >>>>> $ git grep 'CASE_HIGH\>' | wc -l >>>>> 125 >>>>> >>>>> Which is reasonable amount of code that should be changed. I'll prepare patch for that. >>>>> >>>> >>>> Hm, there's one question that pops up: Should we compare the values via pointer equality, >>>> >>>> or tree_int_cst_eq should be done? The later one can messy the code a bit. >>>> >>> I'd like to say pointer equality, but it's probably safer to use >>> tree_int_cst_eq. >>> >>> jeff >>> >> >> Ok, so I'll put it on my TODO list as it will take some time to do the transformation. > NP. There's nothing inherently wrong with the code today, it's just a > cleanup (I hope :-) Yes. > >> >> >> In order to fix the PR, I suggest following patch. >> >> Ready for trunk? >> Martin >> >> 0001-Fix-emission-of-exception-dispatch-PR-middle-end-821.patch >> >> >> From 1cd8987ad3de4a7bd3e71014a0df9ccb3be40277 Mon Sep 17 00:00:00 2001 >> From: marxin <mliska@suse.cz> >> Date: Mon, 11 Sep 2017 13:34:41 +0200 >> Subject: [PATCH] Fix emission of exception dispatch (PR middle-end/82154). >> >> gcc/ChangeLog: >> >> 2017-09-11 Martin Liska <mliska@suse.cz> >> >> PR middle-end/82154 >> * stmt.c (expand_sjlj_dispatch_table): Use CASE_LOW when >> CASE_HIGH is NULL_TREE. >> >> gcc/testsuite/ChangeLog: >> >> 2017-09-11 Martin Liska <mliska@suse.cz> >> >> PR middle-end/82154 >> * g++.dg/torture/pr82154.C: New test. > OK. > > Jeff > Thanks, I've just installed it. Martin
diff --git a/gcc/stmt.c b/gcc/stmt.c index 39d29ff3da9..9b33657372f 100644 --- a/gcc/stmt.c +++ b/gcc/stmt.c @@ -71,7 +71,8 @@ along with GCC; see the file COPYING3. If not see struct simple_case_node { simple_case_node (tree low, tree high, tree code_label): - m_low (low), m_high (high), m_code_label (code_label) + m_low (low), m_high (high != NULL_TREE ? high : low), + m_code_label (code_label) {} /* Lowest index value for this label. */ diff --git a/gcc/testsuite/g++.dg/torture/pr82154.C b/gcc/testsuite/g++.dg/torture/pr82154.C new file mode 100644 index 00000000000..f4e1c3ea139 --- /dev/null +++ b/gcc/testsuite/g++.dg/torture/pr82154.C @@ -0,0 +1,50 @@ +// { dg-do compile } +// { dg-additional-options "-Wno-deprecated" } + +namespace a { +int b; +class c +{ +}; +} +class g +{ +public: + g (); +}; +using a::b; +class d +{ +public: + d (); + void e (); +}; +class f +{ + d + i () + { + static d j; + } + int *k () throw (a::c); +}; + + +int *f::k () throw (a::c) +{ + static g h; + i (); + int l = 2; + while (l) + { + --l; + try + { + operator new (b); + } + catch (a::c) + { + } + } + i ().e (); +}