Re: [PATCH] Fix emission of exception dispatch (PR middle-end/82154).

[Martin Liška]

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  

 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  

 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 
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  

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  

PR middle-end/82154
* g++.dg/torture/pr82154.C: New test.

OK.

Jeff



Thanks, I've just installed it.

Martin

Re: [PATCH] Fix emission of exception dispatch (PR middle-end/82154).

[Jeff Law]

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  
>>
>> 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  
>>
>> 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 
> 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  
> 
>   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  
> 
>   PR middle-end/82154
>   * g++.dg/torture/pr82154.C: New test.
OK.

Jeff

Re: [PATCH] Fix emission of exception dispatch (PR middle-end/82154).

[Martin Liška]

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  

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  

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 
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  

	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  

	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 000..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 ();
+}
-- 
2.14.1

Re: [PATCH] Fix emission of exception dispatch (PR middle-end/82154).

[Jeff Law]

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  

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  

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

Re: [PATCH] Fix emission of exception dispatch (PR middle-end/82154).

[Martin Liška]

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  
>>>
>>> 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  
>>>
>>> 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
>>
> 

Re: [PATCH] Fix emission of exception dispatch (PR middle-end/82154).

[Martin Liška]

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  
>>
>>  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  
>>
>>  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
> 

Re: [PATCH] Fix emission of exception dispatch (PR middle-end/82154).

[Jeff Law]

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  
> 
>   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  
> 
>   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