| Message ID | 5535F7EF.1000305@mentor.com |
|---|---|
| State | New |
| Headers | show |
On Tue, 21 Apr 2015, Tom de Vries wrote: > Hi, > > this patch fixes PR65802. > > The problem described in PR65802 is that when compiling the test-case > (included in the patch below) at -O0, the compiler runs into a gcc_assert ICE > in redirect_eh_edge_1 during pass_cleanup_eh: > ... > gcc_assert (lookup_stmt_eh_lp (throw_stmt) == old_lp_nr); > ... > > > In more detail, during compilation the ifn_va_arg is marked at as a throwing > function. That causes exception handling code to be generated, with exception > handling edges: > ... > ;; basic block 2, loop depth 0, count 0, freq 0, maybe hot > ;; prev block 0, next block 3, flags: (NEW, REACHABLE) > ;; pred: ENTRY (FALLTHRU) > [LP 1] # .MEM_5 = VDEF <.MEM_4(D)> > # USE = anything > # CLB = anything > _6 = VA_ARG (&cD.2333, 0B); > ;; succ: 7 (EH) > ;; 3 (FALLTHRU) > ... > > After pass_lower_vaarg, the expansion of ifn_va_arg is spread over several > basic blocks: > ... > ;; basic block 2, loop depth 0, count 0, freq 0, maybe hot > ;; prev block 0, next block 11, flags: (NEW, REACHABLE) > ;; pred: ENTRY (FALLTHRU) > ;; succ: 11 [100.0%] (FALLTHRU) > > ;; basic block 11, loop depth 0, count 0, freq 0, maybe hot > ;; prev block 2, next block 12, flags: (NEW) > ;; pred: 2 [100.0%] (FALLTHRU) > # VUSE <.MEM_4(D)> > _22 = cD.2333.gp_offsetD.5; > if (_22 >= 48) > goto <bb 13> (<L6>); > else > goto <bb 12> (<L5>); > ;; succ: 13 (TRUE_VALUE) > ;; 12 (FALSE_VALUE) > > ;; basic block 12, loop depth 0, count 0, freq 0, maybe hot > ;; prev block 11, next block 13, flags: (NEW) > ;; pred: 11 (FALSE_VALUE) > <L5>: > # VUSE <.MEM_4(D)> > _23 = cD.2333.reg_save_areaD.8; > # VUSE <.MEM_4(D)> > _24 = cD.2333.gp_offsetD.5; > _25 = (sizetype) _24; > addr.1_26 = _23 + _25; > # VUSE <.MEM_4(D)> > _27 = cD.2333.gp_offsetD.5; > _28 = _27 + 8; > # .MEM_29 = VDEF <.MEM_4(D)> > cD.2333.gp_offsetD.5 = _28; > goto <bb 14> (<L7>); > ;; succ: 14 (FALLTHRU) > > ;; basic block 13, loop depth 0, count 0, freq 0, maybe hot > ;; prev block 12, next block 14, flags: (NEW) > ;; pred: 11 (TRUE_VALUE) > <L6>: > # VUSE <.MEM_4(D)> > _30 = cD.2333.overflow_arg_areaD.7; > addr.1_31 = _30; > _32 = _30 + 8; > # .MEM_33 = VDEF <.MEM_4(D)> > cD.2333.overflow_arg_areaD.7 = _32; > ;; succ: 14 (FALLTHRU) > > ;; basic block 14, loop depth 0, count 0, freq 0, maybe hot > ;; prev block 13, next block 15, flags: (NEW) > ;; pred: 12 (FALLTHRU) > ;; 13 (FALLTHRU) > # .MEM_20 = PHI <.MEM_29(12), .MEM_33(13)> > # addr.1_21 = PHI <addr.1_26(12), addr.1_31(13)> > <L7>: > # VUSE <.MEM_20> > _6 = MEM[(intD.9 * * {ref-all})addr.1_21]; > ;; succ: 15 (FALLTHRU) > > ;; basic block 15, loop depth 0, count 0, freq 0, maybe hot > ;; prev block 14, next block 3, flags: (NEW) > ;; pred: 14 (FALLTHRU) > ;; succ: 7 (EH) > ;; 3 (FALLTHRU) > ... > > And an ICE is triggered in redirect_eh_edge_1, because the code expects the > last statement in a BB with an outgoing EH edge to be a throwing statement. > > That's obviously not the case, since bb15 is empty. But also all the other > statements in the expansion are non-throwing. > > > Looking at the representation before the ifn_va_arg, VA_ARG_EXPR is > non-throwing (even with -fnon-call-exceptions). > > And looking at the situation before the introduction of ifn_va_arg, the > expansion of VA_ARG_EXPR also didn't contain any throwing statements. > > > This patch fixes the ICE by marking ifn_va_arg with ECF_NOTHROW. > > Bootstrapped and reg-tested on x86_64. > > OK for trunk? Ok. Thanks, Richard. > Thanks, > - Tom >
> Mark ifn_va_arg with ECF_NOTHROW You can defnitly make it ECF_LEAF too. I wonder if we can make it ECF_CONST or at leat PURE this would help to keep variadic functions const/pure that may be moderately interesting in practice. Honza > > 2015-04-20 Tom de Vries <tom@codesourcery.com> > > PR tree-optimization/65802 > * internal-fn.def (VA_ARG): Add ECF_NOTROW to flags. > > * g++.dg/pr65802.C: New test. > --- > gcc/internal-fn.def | 2 +- > gcc/testsuite/g++.dg/pr65802.C | 29 +++++++++++++++++++++++++++++ > 2 files changed, 30 insertions(+), 1 deletion(-) > create mode 100644 gcc/testsuite/g++.dg/pr65802.C > > diff --git a/gcc/internal-fn.def b/gcc/internal-fn.def > index f557c64..7e19313 100644 > --- a/gcc/internal-fn.def > +++ b/gcc/internal-fn.def > @@ -62,4 +62,4 @@ DEF_INTERNAL_FN (ADD_OVERFLOW, ECF_CONST | ECF_LEAF | ECF_NOTHROW, NULL) > DEF_INTERNAL_FN (SUB_OVERFLOW, ECF_CONST | ECF_LEAF | ECF_NOTHROW, NULL) > DEF_INTERNAL_FN (MUL_OVERFLOW, ECF_CONST | ECF_LEAF | ECF_NOTHROW, NULL) > DEF_INTERNAL_FN (TSAN_FUNC_EXIT, ECF_NOVOPS | ECF_LEAF | ECF_NOTHROW, NULL) > -DEF_INTERNAL_FN (VA_ARG, 0, NULL) > +DEF_INTERNAL_FN (VA_ARG, ECF_NOTHROW, NULL) > diff --git a/gcc/testsuite/g++.dg/pr65802.C b/gcc/testsuite/g++.dg/pr65802.C > new file mode 100644 > index 0000000..26e5317 > --- /dev/null > +++ b/gcc/testsuite/g++.dg/pr65802.C > @@ -0,0 +1,29 @@ > +// { dg-do compile } > +// { dg-options "-O0" } > + > +typedef int tf (); > + > +struct S > +{ > + tf m_fn1; > +} a; > + > +void > +fn1 () > +{ > + try > + { > + __builtin_va_list c; > + { > + int *d = __builtin_va_arg (c, int *); > + int **e = &d; > + __asm__("" : "=d"(e)); > + a.m_fn1 (); > + } > + a.m_fn1 (); > + } > + catch (...) > + { > + > + } > +} > -- > 1.9.1 >
On Tue, 21 Apr 2015, Jan Hubicka wrote: > > Mark ifn_va_arg with ECF_NOTHROW > > You can defnitly make it ECF_LEAF too. I wonder if we can make it ECF_CONST or at leat PURE > this would help to keep variadic functions const/pure that may be moderately interesting > in practice. Yes to ECF_LEAF but it isn't const or pure as it modifies the valist argument so you can't for example DCE va_arg (...) if the result isn't needed. Richard. > Honza > > > > 2015-04-20 Tom de Vries <tom@codesourcery.com> > > > > PR tree-optimization/65802 > > * internal-fn.def (VA_ARG): Add ECF_NOTROW to flags. > > > > * g++.dg/pr65802.C: New test. > > --- > > gcc/internal-fn.def | 2 +- > > gcc/testsuite/g++.dg/pr65802.C | 29 +++++++++++++++++++++++++++++ > > 2 files changed, 30 insertions(+), 1 deletion(-) > > create mode 100644 gcc/testsuite/g++.dg/pr65802.C > > > > diff --git a/gcc/internal-fn.def b/gcc/internal-fn.def > > index f557c64..7e19313 100644 > > --- a/gcc/internal-fn.def > > +++ b/gcc/internal-fn.def > > @@ -62,4 +62,4 @@ DEF_INTERNAL_FN (ADD_OVERFLOW, ECF_CONST | ECF_LEAF | ECF_NOTHROW, NULL) > > DEF_INTERNAL_FN (SUB_OVERFLOW, ECF_CONST | ECF_LEAF | ECF_NOTHROW, NULL) > > DEF_INTERNAL_FN (MUL_OVERFLOW, ECF_CONST | ECF_LEAF | ECF_NOTHROW, NULL) > > DEF_INTERNAL_FN (TSAN_FUNC_EXIT, ECF_NOVOPS | ECF_LEAF | ECF_NOTHROW, NULL) > > -DEF_INTERNAL_FN (VA_ARG, 0, NULL) > > +DEF_INTERNAL_FN (VA_ARG, ECF_NOTHROW, NULL) > > diff --git a/gcc/testsuite/g++.dg/pr65802.C b/gcc/testsuite/g++.dg/pr65802.C > > new file mode 100644 > > index 0000000..26e5317 > > --- /dev/null > > +++ b/gcc/testsuite/g++.dg/pr65802.C > > @@ -0,0 +1,29 @@ > > +// { dg-do compile } > > +// { dg-options "-O0" } > > + > > +typedef int tf (); > > + > > +struct S > > +{ > > + tf m_fn1; > > +} a; > > + > > +void > > +fn1 () > > +{ > > + try > > + { > > + __builtin_va_list c; > > + { > > + int *d = __builtin_va_arg (c, int *); > > + int **e = &d; > > + __asm__("" : "=d"(e)); > > + a.m_fn1 (); > > + } > > + a.m_fn1 (); > > + } > > + catch (...) > > + { > > + > > + } > > +} > > -- > > 1.9.1 > > > >
On Tue, Apr 21, 2015 at 3:10 PM, Tom de Vries <Tom_deVries@mentor.com> wrote: > Hi, > > this patch fixes PR65802. > > diff --git a/gcc/testsuite/g++.dg/ pr65802.C b/gcc/testsuite/g++.dg/pr65802.C > new file mode 100644 > index 0000000..26e5317 > --- /dev/null > +++ b/gcc/testsuite/g++.dg/pr65802.C > @@ -0,0 +1,29 @@ > +// { dg-do compile } > +// { dg-options "-O0" } > + > +typedef int tf (); > + > +struct S > +{ > + tf m_fn1; > +} a; > + > +void > +fn1 () > +{ > + try > + { > + __builtin_va_list c; > + { > + int *d = __builtin_va_arg (c, int *); > + int **e = &d; > + __asm__("" : "=d"(e)); Hi, thanks for fixing the issue. But 'd' is a machine specific constraint? This case failed on all arm processors. Thanks, bin > + a.m_fn1 (); > + } > + a.m_fn1 (); > + } > + catch (...) > + { > + > + } > +} > OK for trunk? > > Thanks, > - Tom
Mark ifn_va_arg with ECF_NOTHROW 2015-04-20 Tom de Vries <tom@codesourcery.com> PR tree-optimization/65802 * internal-fn.def (VA_ARG): Add ECF_NOTROW to flags. * g++.dg/pr65802.C: New test. --- gcc/internal-fn.def | 2 +- gcc/testsuite/g++.dg/pr65802.C | 29 +++++++++++++++++++++++++++++ 2 files changed, 30 insertions(+), 1 deletion(-) create mode 100644 gcc/testsuite/g++.dg/pr65802.C diff --git a/gcc/internal-fn.def b/gcc/internal-fn.def index f557c64..7e19313 100644 --- a/gcc/internal-fn.def +++ b/gcc/internal-fn.def @@ -62,4 +62,4 @@ DEF_INTERNAL_FN (ADD_OVERFLOW, ECF_CONST | ECF_LEAF | ECF_NOTHROW, NULL) DEF_INTERNAL_FN (SUB_OVERFLOW, ECF_CONST | ECF_LEAF | ECF_NOTHROW, NULL) DEF_INTERNAL_FN (MUL_OVERFLOW, ECF_CONST | ECF_LEAF | ECF_NOTHROW, NULL) DEF_INTERNAL_FN (TSAN_FUNC_EXIT, ECF_NOVOPS | ECF_LEAF | ECF_NOTHROW, NULL) -DEF_INTERNAL_FN (VA_ARG, 0, NULL) +DEF_INTERNAL_FN (VA_ARG, ECF_NOTHROW, NULL) diff --git a/gcc/testsuite/g++.dg/pr65802.C b/gcc/testsuite/g++.dg/pr65802.C new file mode 100644 index 0000000..26e5317 --- /dev/null +++ b/gcc/testsuite/g++.dg/pr65802.C @@ -0,0 +1,29 @@ +// { dg-do compile } +// { dg-options "-O0" } + +typedef int tf (); + +struct S +{ + tf m_fn1; +} a; + +void +fn1 () +{ + try + { + __builtin_va_list c; + { + int *d = __builtin_va_arg (c, int *); + int **e = &d; + __asm__("" : "=d"(e)); + a.m_fn1 (); + } + a.m_fn1 (); + } + catch (...) + { + + } +} -- 1.9.1
Hi, this patch fixes PR65802. The problem described in PR65802 is that when compiling the test-case (included in the patch below) at -O0, the compiler runs into a gcc_assert ICE in redirect_eh_edge_1 during pass_cleanup_eh: ... gcc_assert (lookup_stmt_eh_lp (throw_stmt) == old_lp_nr); ... In more detail, during compilation the ifn_va_arg is marked at as a throwing function. That causes exception handling code to be generated, with exception handling edges: ... ;; basic block 2, loop depth 0, count 0, freq 0, maybe hot ;; prev block 0, next block 3, flags: (NEW, REACHABLE) ;; pred: ENTRY (FALLTHRU) [LP 1] # .MEM_5 = VDEF <.MEM_4(D)> # USE = anything # CLB = anything _6 = VA_ARG (&cD.2333, 0B); ;; succ: 7 (EH) ;; 3 (FALLTHRU) ... After pass_lower_vaarg, the expansion of ifn_va_arg is spread over several basic blocks: ... ;; basic block 2, loop depth 0, count 0, freq 0, maybe hot ;; prev block 0, next block 11, flags: (NEW, REACHABLE) ;; pred: ENTRY (FALLTHRU) ;; succ: 11 [100.0%] (FALLTHRU) ;; basic block 11, loop depth 0, count 0, freq 0, maybe hot ;; prev block 2, next block 12, flags: (NEW) ;; pred: 2 [100.0%] (FALLTHRU) # VUSE <.MEM_4(D)> _22 = cD.2333.gp_offsetD.5; if (_22 >= 48) goto <bb 13> (<L6>); else goto <bb 12> (<L5>); ;; succ: 13 (TRUE_VALUE) ;; 12 (FALSE_VALUE) ;; basic block 12, loop depth 0, count 0, freq 0, maybe hot ;; prev block 11, next block 13, flags: (NEW) ;; pred: 11 (FALSE_VALUE) <L5>: # VUSE <.MEM_4(D)> _23 = cD.2333.reg_save_areaD.8; # VUSE <.MEM_4(D)> _24 = cD.2333.gp_offsetD.5; _25 = (sizetype) _24; addr.1_26 = _23 + _25; # VUSE <.MEM_4(D)> _27 = cD.2333.gp_offsetD.5; _28 = _27 + 8; # .MEM_29 = VDEF <.MEM_4(D)> cD.2333.gp_offsetD.5 = _28; goto <bb 14> (<L7>); ;; succ: 14 (FALLTHRU) ;; basic block 13, loop depth 0, count 0, freq 0, maybe hot ;; prev block 12, next block 14, flags: (NEW) ;; pred: 11 (TRUE_VALUE) <L6>: # VUSE <.MEM_4(D)> _30 = cD.2333.overflow_arg_areaD.7; addr.1_31 = _30; _32 = _30 + 8; # .MEM_33 = VDEF <.MEM_4(D)> cD.2333.overflow_arg_areaD.7 = _32; ;; succ: 14 (FALLTHRU) ;; basic block 14, loop depth 0, count 0, freq 0, maybe hot ;; prev block 13, next block 15, flags: (NEW) ;; pred: 12 (FALLTHRU) ;; 13 (FALLTHRU) # .MEM_20 = PHI <.MEM_29(12), .MEM_33(13)> # addr.1_21 = PHI <addr.1_26(12), addr.1_31(13)> <L7>: # VUSE <.MEM_20> _6 = MEM[(intD.9 * * {ref-all})addr.1_21]; ;; succ: 15 (FALLTHRU) ;; basic block 15, loop depth 0, count 0, freq 0, maybe hot ;; prev block 14, next block 3, flags: (NEW) ;; pred: 14 (FALLTHRU) ;; succ: 7 (EH) ;; 3 (FALLTHRU) ... And an ICE is triggered in redirect_eh_edge_1, because the code expects the last statement in a BB with an outgoing EH edge to be a throwing statement. That's obviously not the case, since bb15 is empty. But also all the other statements in the expansion are non-throwing. Looking at the representation before the ifn_va_arg, VA_ARG_EXPR is non-throwing (even with -fnon-call-exceptions). And looking at the situation before the introduction of ifn_va_arg, the expansion of VA_ARG_EXPR also didn't contain any throwing statements. This patch fixes the ICE by marking ifn_va_arg with ECF_NOTHROW. Bootstrapped and reg-tested on x86_64. OK for trunk? Thanks, - Tom