| Message ID | 52F53574.9070809@redhat.com |
|---|---|
| State | New |
| Headers | show |
On February 7, 2014 8:35:16 PM GMT+01:00, Richard Henderson <rth@redhat.com> wrote: >In the testcases with the PR, we have a bit of type punning going on, > > *(int *) &s2.f = 0; > s2 = s1; > >which SRA trasforms to > > # DEBUG s2 => 0 > MEM[(int *)&s2] = 0; > # DEBUG s2 => s1$f_7 > # DEBUG s2$g => s1$g_6 > s2 ={v} {CLOBBER}; > >Note that it has chosen not to expand s1.f like s1.g, but to expand >that field >as the type-punned integer. Which means that "s2 => s1$f_7" has >mismatched >types across lhs and rhs: SI => SF. Which understandibly ICEs during >rtl >expansion. > >I'm not really sure how this is avoided for the actual code generation, >but >this minimal patch (aka hack) simply drops the debug info to avoid the >ICE. > >Thoughts on how this might really be solved? Add a VIEW_CONVERT_EXPR around the rhs of the debug statement. Richard. > >r~
On 02/07/2014 03:12 PM, Richard Biener wrote: > On February 7, 2014 8:35:16 PM GMT+01:00, Richard Henderson <rth@redhat.com> wrote: >> In the testcases with the PR, we have a bit of type punning going on, >> >> *(int *) &s2.f = 0; >> s2 = s1; >> >> which SRA trasforms to >> >> # DEBUG s2 => 0 >> MEM[(int *)&s2] = 0; >> # DEBUG s2 => s1$f_7 >> # DEBUG s2$g => s1$g_6 >> s2 ={v} {CLOBBER}; >> >> Note that it has chosen not to expand s1.f like s1.g, but to expand >> that field >> as the type-punned integer. Which means that "s2 => s1$f_7" has >> mismatched >> types across lhs and rhs: SI => SF. Which understandibly ICEs during >> rtl >> expansion. >> >> I'm not really sure how this is avoided for the actual code generation, >> but >> this minimal patch (aka hack) simply drops the debug info to avoid the >> ICE. >> >> Thoughts on how this might really be solved? > > Add a VIEW_CONVERT_EXPR around the rhs of the debug statement. Well, ok, though I'm pretty sure that the debug info will pretty much barf on that immediately. What I really meant is: where's a better place to put this check, since such a check _must_ exist somewhere else for the regular code generation. r~
On Fri, Feb 07, 2014 at 04:37:22PM -0800, Richard Henderson wrote: > >> Thoughts on how this might really be solved? > > > > Add a VIEW_CONVERT_EXPR around the rhs of the debug statement. > > Well, ok, though I'm pretty sure that the debug info will pretty much barf on > that immediately. Why? That is typically DW_OP_GNU_reinterpret. Jakub
On Sat, Feb 8, 2014 at 1:37 AM, Richard Henderson <rth@redhat.com> wrote: > On 02/07/2014 03:12 PM, Richard Biener wrote: >> On February 7, 2014 8:35:16 PM GMT+01:00, Richard Henderson <rth@redhat.com> wrote: >>> In the testcases with the PR, we have a bit of type punning going on, >>> >>> *(int *) &s2.f = 0; >>> s2 = s1; >>> >>> which SRA trasforms to >>> >>> # DEBUG s2 => 0 >>> MEM[(int *)&s2] = 0; >>> # DEBUG s2 => s1$f_7 >>> # DEBUG s2$g => s1$g_6 >>> s2 ={v} {CLOBBER}; >>> >>> Note that it has chosen not to expand s1.f like s1.g, but to expand >>> that field >>> as the type-punned integer. Which means that "s2 => s1$f_7" has >>> mismatched >>> types across lhs and rhs: SI => SF. Which understandibly ICEs during >>> rtl >>> expansion. >>> >>> I'm not really sure how this is avoided for the actual code generation, >>> but >>> this minimal patch (aka hack) simply drops the debug info to avoid the >>> ICE. >>> >>> Thoughts on how this might really be solved? >> >> Add a VIEW_CONVERT_EXPR around the rhs of the debug statement. > > Well, ok, though I'm pretty sure that the debug info will pretty much barf on > that immediately. > > What I really meant is: where's a better place to put this check, since such a > check _must_ exist somewhere else for the regular code generation. What do you mean with "check"? We have stmt verifiers in tree-cfg.c but they skip debug statement contents because "those may contain anything" (including crap it seems ;)). That was an early design decision appearantly - given the above ICE maybe not the best one. IIRC the debug stmt expander is the one that is supposed to throw away anything it cannot handle (and thus it has to expect any sort of werid stuff). Richard. > > r~
Hi, On Fri, Feb 07, 2014 at 04:37:22PM -0800, Richard Henderson wrote: > On 02/07/2014 03:12 PM, Richard Biener wrote: > > On February 7, 2014 8:35:16 PM GMT+01:00, Richard Henderson <rth@redhat.com> wrote: > >> In the testcases with the PR, we have a bit of type punning going on, > >> > >> *(int *) &s2.f = 0; > >> s2 = s1; > >> > >> which SRA trasforms to > >> > >> # DEBUG s2 => 0 > >> MEM[(int *)&s2] = 0; > >> # DEBUG s2 => s1$f_7 > >> # DEBUG s2$g => s1$g_6 > >> s2 ={v} {CLOBBER}; > >> > >> Note that it has chosen not to expand s1.f like s1.g, but to expand > >> that field > >> as the type-punned integer. Which means that "s2 => s1$f_7" has > >> mismatched > >> types across lhs and rhs: SI => SF. Which understandibly ICEs during > >> rtl > >> expansion. > >> > >> I'm not really sure how this is avoided for the actual code generation, > >> but > >> this minimal patch (aka hack) simply drops the debug info to avoid the > >> ICE. > >> > >> Thoughts on how this might really be solved? > > > > Add a VIEW_CONVERT_EXPR around the rhs of the debug statement. > > Well, ok, though I'm pretty sure that the debug info will pretty much barf on > that immediately. > > What I really meant is: where's a better place to put this check, since such a > check _must_ exist somewhere else for the regular code generation. > That is what SRA does for regular code, see the useless_type_conversion_p check earlier in the same function. If you want me to, I can prepare the patch (at some point next week). Thanks for analysis of the bug, Martin
diff --git a/gcc/testsuite/gcc.dg/debug/pr59776.c b/gcc/testsuite/gcc.dg/debug/pr59776.c new file mode 100644 index 0000000..245c3d7 --- /dev/null +++ b/gcc/testsuite/gcc.dg/debug/pr59776.c @@ -0,0 +1,12 @@ +/* { dg-do compile } */ + +struct S { float f, g; }; + +void +sub_ (struct S *p) +{ + struct S s1, s2; + s1 = *p; + *(int *) &s2.f = 0; + s2 = s1; +} diff --git a/gcc/tree-sra.c b/gcc/tree-sra.c index 4992b4c..f9ff0a4 100644 --- a/gcc/tree-sra.c +++ b/gcc/tree-sra.c @@ -2950,9 +2950,15 @@ load_assign_lhs_subreplacements (struct access *lacc, struct access *top_racc, lacc); else drhs = NULL_TREE; - ds = gimple_build_debug_bind (get_access_replacement (lacc), - drhs, gsi_stmt (*old_gsi)); - gsi_insert_after (new_gsi, ds, GSI_NEW_STMT); + + // ??? Can this type mismatch only happen with debuginfo, + // or can it happen with real code as well? + if (drhs && types_compatible_p (lacc->type, TREE_TYPE (drhs))) + { + ds = gimple_build_debug_bind (get_access_replacement (lacc), + drhs, gsi_stmt (*old_gsi)); + gsi_insert_after (new_gsi, ds, GSI_NEW_STMT); + } } }
In the testcases with the PR, we have a bit of type punning going on, *(int *) &s2.f = 0; s2 = s1; which SRA trasforms to # DEBUG s2 => 0 MEM[(int *)&s2] = 0; # DEBUG s2 => s1$f_7 # DEBUG s2$g => s1$g_6 s2 ={v} {CLOBBER}; Note that it has chosen not to expand s1.f like s1.g, but to expand that field as the type-punned integer. Which means that "s2 => s1$f_7" has mismatched types across lhs and rhs: SI => SF. Which understandibly ICEs during rtl expansion. I'm not really sure how this is avoided for the actual code generation, but this minimal patch (aka hack) simply drops the debug info to avoid the ICE. Thoughts on how this might really be solved? r~