| Message ID | 4DB10B29.9040407@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Apr 21, 2011, at 9:59 PM, Jason Merrill wrote: > On 04/21/2011 10:55 PM, Nathan Froyd wrote: >> On Thu, Apr 21, 2011 at 10:49:05PM -0400, Jason Merrill wrote: >>> Hunh. How does that work? They fill in CASE_LABEL later? Can that be >>> changed? >> >> Yeah, tree-eh.c:lower_try_finally_switch. I don't know how easy it is >> to fix; it certainly looks non-trivial. > > Well, I tried adjusting it and regression testing seems fine so far. Unsurprising... It will never fail during testsuite run, and won't always fail during a bootstrap. > I can't think what the comment would be talking about with pointers not providing a stable order; I don't see anything that would rely on that. http://gcc.gnu.org/ml/gcc/2005-04/msg00161.html has the details of why the code was put in. Essentially, the Ada boostrap on x86 linux. What's worse is, at the time, it would only occasionally fail, so, a bootstrap that works won't prove anything.
On Fri, Apr 22, 2011 at 8:13 AM, Mike Stump <mikestump@comcast.net> wrote: > On Apr 21, 2011, at 9:59 PM, Jason Merrill wrote: >> On 04/21/2011 10:55 PM, Nathan Froyd wrote: >>> On Thu, Apr 21, 2011 at 10:49:05PM -0400, Jason Merrill wrote: >>>> Hunh. How does that work? They fill in CASE_LABEL later? Can that be >>>> changed? >>> >>> Yeah, tree-eh.c:lower_try_finally_switch. I don't know how easy it is >>> to fix; it certainly looks non-trivial. >> >> Well, I tried adjusting it and regression testing seems fine so far. > > Unsurprising... It will never fail during testsuite run, and won't always fail during a bootstrap. > >> I can't think what the comment would be talking about with pointers not providing a stable order; I don't see anything that would rely on that. > > http://gcc.gnu.org/ml/gcc/2005-04/msg00161.html > > has the details of why the code was put in. Essentially, the Ada boostrap on x86 linux. What's worse is, at the time, it would only occasionally fail, so, a bootstrap that works won't prove anything. Well, unless we are not walking a pointer-based hashtable I don't see how this matters here. To Nathan: yes, UNKNOWN_LOCATION would be correct. Whoever then sets the label should adjust it accordingly. Richard.
On Fri, Apr 22, 2011 at 11:12:01AM +0200, Richard Guenther wrote: > On Fri, Apr 22, 2011 at 8:13 AM, Mike Stump <mikestump@comcast.net> wrote: > > Unsurprising... It will never fail during testsuite run, and won't > > always fail during a bootstrap. > > > >> I can't think what the comment would be talking about with pointers > >> not providing a stable order; I don't see anything that would rely > >> on that. > > > > http://gcc.gnu.org/ml/gcc/2005-04/msg00161.html > > > > has the details of why the code was put in. Essentially, the Ada > > boostrap on x86 linux. What's worse is, at the time, it would only > > occasionally fail, so, a bootstrap that works won't prove anything. > > Well, unless we are not walking a pointer-based hashtable I don't see > how this matters here. I can't see the pointer traversal, either--unless there's some subtlety in how things are added to the goto_queue. I'm going to leave the code alone for the moment. > To Nathan: yes, UNKNOWN_LOCATION would be correct. Whoever then sets > the label should adjust it accordingly. Will commit with that change in build_case_label, then. I will leave the location-setting to a separate commit, if any. -Nathan
On 04/22/2011 02:13 AM, Mike Stump wrote: > http://gcc.gnu.org/ml/gcc/2005-04/msg00161.html > > has the details of why the code was put in. Right. At the time, we were sorting the goto queue based on pointer values, which caused the problem. We no longer do that, so we shouldn't need this workaround (deferring creation of case label labels) anymore. What do you think, Alex? Jason
On Apr 22, 2011, at 8:55 AM, Jason Merrill wrote: > On 04/22/2011 02:13 AM, Mike Stump wrote: >> http://gcc.gnu.org/ml/gcc/2005-04/msg00161.html >> >> has the details of why the code was put in. > > Right. At the time, we were sorting the goto queue based on pointer values, which caused the problem. We no longer do that, Excellent. That was the only concern I knew of.
On Fri, Apr 22, 2011 at 12:59:21AM -0400, Jason Merrill wrote: > On 04/21/2011 10:55 PM, Nathan Froyd wrote: >> On Thu, Apr 21, 2011 at 10:49:05PM -0400, Jason Merrill wrote: >>> Hunh. How does that work? They fill in CASE_LABEL later? Can that be >>> changed? >> >> Yeah, tree-eh.c:lower_try_finally_switch. I don't know how easy it is >> to fix; it certainly looks non-trivial. > > Well, I tried adjusting it and regression testing seems fine so far. I > can't think what the comment would be talking about with pointers not > providing a stable order; I don't see anything that would rely on > that. Based on discussion downthread, I plan to commit something like your patch (I think `label' is unused after this, so requires trivial changes) on your behalf tomorrow, unless you beat me to it or unless somebody yells. I'd rather have this not mixed up with the rest of the build_case_label changes. -Nathan
On Apr 22, 2011, Jason Merrill <jason@redhat.com> wrote: > On 04/22/2011 02:13 AM, Mike Stump wrote: >> http://gcc.gnu.org/ml/gcc/2005-04/msg00161.html >> >> has the details of why the code was put in. > Right. At the time, we were sorting the goto queue based on pointer > values, which caused the problem. We no longer do that, so we > shouldn't need this workaround (deferring creation of case label > labels) anymore. > What do you think, Alex? Yeah, this change looks safe now.
diff --git a/gcc/tree-eh.c b/gcc/tree-eh.c index 60e2236..feee182 100644 --- a/gcc/tree-eh.c +++ b/gcc/tree-eh.c @@ -1419,11 +1419,9 @@ lower_try_finally_switch (struct leh_state *state, struct leh_tf_state *tf) void **slot; case_lab = build3 (CASE_LABEL_EXPR, void_type_node, build_int_cst (NULL, switch_id), - NULL, NULL); + NULL, create_artificial_label (tf_loc)); /* We store the cont_stmt in the pointer map, so that we can recover - it in the loop below. We don't create the new label while - walking the goto_queue because pointers don't offer a stable - order. */ + it in the loop below. */ if (!cont_map) cont_map = pointer_map_create (); slot = pointer_map_insert (cont_map, case_lab); @@ -1443,13 +1441,10 @@ lower_try_finally_switch (struct leh_state *state, struct leh_tf_state *tf) gcc_assert (cont_map); slot = pointer_map_contains (cont_map, last_case); - /* As the comment above suggests, CASE_LABEL (last_case) was just a - placeholder, it does not store an actual label, yet. */ gcc_assert (slot); cont_stmt = *(gimple *) slot; - label = create_artificial_label (tf_loc); - CASE_LABEL (last_case) = label; + label = CASE_LABEL (last_case); x = gimple_build_label (label); gimple_seq_add_stmt (&switch_body, x);