| Message ID | 20150901143909.GB55610@msticlxl57.ims.intel.com |
|---|---|
| State | New |
| Headers | show |
On Tue, Sep 1, 2015 at 5:03 PM, Ilya Enkovich <enkovich.gnu@gmail.com> wrote: > Hi, > > This fixes an ICE by adding a NULL check. Bootstrapped and regtested for x86_64-unknown-linux-gnu. Applied to trunk. Does this need to be ported to gcc-5-branch? > > Thanks, > Ilya > -- > gcc/ > > 2015-09-01 Ilya Enkovich <enkovich.gnu@gmail.com> > > PR target/67405 > * tree-chkp.c (chkp_find_bound_slots_1): Add NULL check. > > gcc/testsuite/ > > 2015-09-01 Ilya Enkovich <enkovich.gnu@gmail.com> > > PR target/67405 > * g++.dg/pr67405.C: New test. > > > diff --git a/gcc/testsuite/g++.dg/pr67405.C b/gcc/testsuite/g++.dg/pr67405.C > new file mode 100644 > index 0000000..5055921 > --- /dev/null > +++ b/gcc/testsuite/g++.dg/pr67405.C > @@ -0,0 +1,11 @@ > +// { dg-do compile } > + > +struct S > +{ > + S f; // { dg-error "incomplete type" } > +}; > + > +void > +fn1 (S p1) > +{ > +} > diff --git a/gcc/tree-chkp.c b/gcc/tree-chkp.c > index 8c1b48c..2489abb 100644 > --- a/gcc/tree-chkp.c > +++ b/gcc/tree-chkp.c > @@ -1667,8 +1667,9 @@ chkp_find_bound_slots_1 (const_tree type, bitmap have_bound, > for (field = TYPE_FIELDS (type); field; field = DECL_CHAIN (field)) > if (TREE_CODE (field) == FIELD_DECL) > { > - HOST_WIDE_INT field_offs > - = TREE_INT_CST_LOW (DECL_FIELD_BIT_OFFSET (field)); > + HOST_WIDE_INT field_offs = 0; > + if (DECL_FIELD_BIT_OFFSET (field)) DECL_FIELD_BIT_OFFSET should be never NULL. Whoever created that FIELD_DECL created an invalid one. Richard. > + field_offs += TREE_INT_CST_LOW (DECL_FIELD_BIT_OFFSET (field)); > if (DECL_FIELD_OFFSET (field)) > field_offs += TREE_INT_CST_LOW (DECL_FIELD_OFFSET (field)) * 8; > chkp_find_bound_slots_1 (TREE_TYPE (field), have_bound,
2015-09-02 15:35 GMT+03:00 Richard Biener <richard.guenther@gmail.com>: > On Tue, Sep 1, 2015 at 5:03 PM, Ilya Enkovich <enkovich.gnu@gmail.com> wrote: >> Hi, >> >> This fixes an ICE by adding a NULL check. Bootstrapped and regtested for x86_64-unknown-linux-gnu. Applied to trunk. Does this need to be ported to gcc-5-branch? >> >> Thanks, >> Ilya >> -- >> gcc/ >> >> 2015-09-01 Ilya Enkovich <enkovich.gnu@gmail.com> >> >> PR target/67405 >> * tree-chkp.c (chkp_find_bound_slots_1): Add NULL check. >> >> gcc/testsuite/ >> >> 2015-09-01 Ilya Enkovich <enkovich.gnu@gmail.com> >> >> PR target/67405 >> * g++.dg/pr67405.C: New test. >> >> >> diff --git a/gcc/testsuite/g++.dg/pr67405.C b/gcc/testsuite/g++.dg/pr67405.C >> new file mode 100644 >> index 0000000..5055921 >> --- /dev/null >> +++ b/gcc/testsuite/g++.dg/pr67405.C >> @@ -0,0 +1,11 @@ >> +// { dg-do compile } >> + >> +struct S >> +{ >> + S f; // { dg-error "incomplete type" } >> +}; >> + >> +void >> +fn1 (S p1) >> +{ >> +} >> diff --git a/gcc/tree-chkp.c b/gcc/tree-chkp.c >> index 8c1b48c..2489abb 100644 >> --- a/gcc/tree-chkp.c >> +++ b/gcc/tree-chkp.c >> @@ -1667,8 +1667,9 @@ chkp_find_bound_slots_1 (const_tree type, bitmap have_bound, >> for (field = TYPE_FIELDS (type); field; field = DECL_CHAIN (field)) >> if (TREE_CODE (field) == FIELD_DECL) >> { >> - HOST_WIDE_INT field_offs >> - = TREE_INT_CST_LOW (DECL_FIELD_BIT_OFFSET (field)); >> + HOST_WIDE_INT field_offs = 0; >> + if (DECL_FIELD_BIT_OFFSET (field)) > > DECL_FIELD_BIT_OFFSET should be never NULL. Whoever created that > FIELD_DECL created an invalid one. I'll check where this decl comes from. Is there a proper checker to add a NULL test for DECL_FIELD_BIT_OFFSET BTW?. Thanks, Ilya > > Richard. > >> + field_offs += TREE_INT_CST_LOW (DECL_FIELD_BIT_OFFSET (field)); >> if (DECL_FIELD_OFFSET (field)) >> field_offs += TREE_INT_CST_LOW (DECL_FIELD_OFFSET (field)) * 8; >> chkp_find_bound_slots_1 (TREE_TYPE (field), have_bound,
On Wed, Sep 2, 2015 at 2:51 PM, Ilya Enkovich <enkovich.gnu@gmail.com> wrote: > 2015-09-02 15:35 GMT+03:00 Richard Biener <richard.guenther@gmail.com>: >> On Tue, Sep 1, 2015 at 5:03 PM, Ilya Enkovich <enkovich.gnu@gmail.com> wrote: >>> Hi, >>> >>> This fixes an ICE by adding a NULL check. Bootstrapped and regtested for x86_64-unknown-linux-gnu. Applied to trunk. Does this need to be ported to gcc-5-branch? >>> >>> Thanks, >>> Ilya >>> -- >>> gcc/ >>> >>> 2015-09-01 Ilya Enkovich <enkovich.gnu@gmail.com> >>> >>> PR target/67405 >>> * tree-chkp.c (chkp_find_bound_slots_1): Add NULL check. >>> >>> gcc/testsuite/ >>> >>> 2015-09-01 Ilya Enkovich <enkovich.gnu@gmail.com> >>> >>> PR target/67405 >>> * g++.dg/pr67405.C: New test. >>> >>> >>> diff --git a/gcc/testsuite/g++.dg/pr67405.C b/gcc/testsuite/g++.dg/pr67405.C >>> new file mode 100644 >>> index 0000000..5055921 >>> --- /dev/null >>> +++ b/gcc/testsuite/g++.dg/pr67405.C >>> @@ -0,0 +1,11 @@ >>> +// { dg-do compile } >>> + >>> +struct S >>> +{ >>> + S f; // { dg-error "incomplete type" } >>> +}; >>> + >>> +void >>> +fn1 (S p1) >>> +{ >>> +} >>> diff --git a/gcc/tree-chkp.c b/gcc/tree-chkp.c >>> index 8c1b48c..2489abb 100644 >>> --- a/gcc/tree-chkp.c >>> +++ b/gcc/tree-chkp.c >>> @@ -1667,8 +1667,9 @@ chkp_find_bound_slots_1 (const_tree type, bitmap have_bound, >>> for (field = TYPE_FIELDS (type); field; field = DECL_CHAIN (field)) >>> if (TREE_CODE (field) == FIELD_DECL) >>> { >>> - HOST_WIDE_INT field_offs >>> - = TREE_INT_CST_LOW (DECL_FIELD_BIT_OFFSET (field)); >>> + HOST_WIDE_INT field_offs = 0; >>> + if (DECL_FIELD_BIT_OFFSET (field)) >> >> DECL_FIELD_BIT_OFFSET should be never NULL. Whoever created that >> FIELD_DECL created an invalid one. > > I'll check where this decl comes from. Is there a proper checker to > add a NULL test for DECL_FIELD_BIT_OFFSET BTW?. The type verifier Honza added recently I guess. Richard. > Thanks, > Ilya > >> >> Richard. >> >>> + field_offs += TREE_INT_CST_LOW (DECL_FIELD_BIT_OFFSET (field)); >>> if (DECL_FIELD_OFFSET (field)) >>> field_offs += TREE_INT_CST_LOW (DECL_FIELD_OFFSET (field)) * 8; >>> chkp_find_bound_slots_1 (TREE_TYPE (field), have_bound,
2015-09-02 15:35 GMT+03:00 Richard Biener <richard.guenther@gmail.com>: > > DECL_FIELD_BIT_OFFSET should be never NULL. Whoever created that > FIELD_DECL created an invalid one. > > Richard. > layout_class_type doesn't place fields with no type and thus we have nothing for DECL_FIELD_BIT_OFFSET. We still continue compilation and function parameters gimplification causes a call to chkp_find_bound_slots_1 which tries to access. So probably I should handle gracefully fields with error_mark_node as a type? Or we better put something into DECL_FIELD_BIT_OFFSET (zero? error_mark_node?) for such fields. Ilya
On Mon, Sep 7, 2015 at 2:39 PM, Ilya Enkovich <enkovich.gnu@gmail.com> wrote: > 2015-09-02 15:35 GMT+03:00 Richard Biener <richard.guenther@gmail.com>: >> >> DECL_FIELD_BIT_OFFSET should be never NULL. Whoever created that >> FIELD_DECL created an invalid one. >> >> Richard. >> > > layout_class_type doesn't place fields with no type Err - that's because fields should also have a type. > and thus we have > nothing for DECL_FIELD_BIT_OFFSET. We still continue compilation and > function parameters gimplification causes a call to > chkp_find_bound_slots_1 which tries to access. So probably I should > handle gracefully fields with error_mark_node as a type? Or we better > put something into DECL_FIELD_BIT_OFFSET (zero? error_mark_node?) for > such fields. > > Ilya
2015-09-13 16:36 GMT+03:00 Richard Biener <richard.guenther@gmail.com>: > On Mon, Sep 7, 2015 at 2:39 PM, Ilya Enkovich <enkovich.gnu@gmail.com> wrote: >> 2015-09-02 15:35 GMT+03:00 Richard Biener <richard.guenther@gmail.com>: >>> >>> DECL_FIELD_BIT_OFFSET should be never NULL. Whoever created that >>> FIELD_DECL created an invalid one. >>> >>> Richard. >>> >> >> layout_class_type doesn't place fields with no type > > Err - that's because fields should also have a type. Sure. But we are talking about a wrong code and still want to continue compilation to some point even if some field misses a type. It means everything possibly invoked at this stage should check type against error_mark_node. Thus I need to handle it gracefully in chkp_find_bound_slots I suppose. Ilya > >> and thus we have >> nothing for DECL_FIELD_BIT_OFFSET. We still continue compilation and >> function parameters gimplification causes a call to >> chkp_find_bound_slots_1 which tries to access. So probably I should >> handle gracefully fields with error_mark_node as a type? Or we better >> put something into DECL_FIELD_BIT_OFFSET (zero? error_mark_node?) for >> such fields. >> >> Ilya
On Tue, Sep 15, 2015 at 11:28 AM, Ilya Enkovich <enkovich.gnu@gmail.com> wrote: > 2015-09-13 16:36 GMT+03:00 Richard Biener <richard.guenther@gmail.com>: >> On Mon, Sep 7, 2015 at 2:39 PM, Ilya Enkovich <enkovich.gnu@gmail.com> wrote: >>> 2015-09-02 15:35 GMT+03:00 Richard Biener <richard.guenther@gmail.com>: >>>> >>>> DECL_FIELD_BIT_OFFSET should be never NULL. Whoever created that >>>> FIELD_DECL created an invalid one. >>>> >>>> Richard. >>>> >>> >>> layout_class_type doesn't place fields with no type >> >> Err - that's because fields should also have a type. > > Sure. But we are talking about a wrong code and still want to continue > compilation to some point even if some field misses a type. It means > everything possibly invoked at this stage should check type against > error_mark_node. Thus I need to handle it gracefully in > chkp_find_bound_slots I suppose. I see. I wonder why we even call chkp_find_bound_slots if seen_errors(). I suppose only recursing for COMPLETE_TYPE_P () would work? Richard. > Ilya > >> >>> and thus we have >>> nothing for DECL_FIELD_BIT_OFFSET. We still continue compilation and >>> function parameters gimplification causes a call to >>> chkp_find_bound_slots_1 which tries to access. So probably I should >>> handle gracefully fields with error_mark_node as a type? Or we better >>> put something into DECL_FIELD_BIT_OFFSET (zero? error_mark_node?) for >>> such fields. >>> >>> Ilya
2015-09-15 13:32 GMT+03:00 Richard Biener <richard.guenther@gmail.com>: > On Tue, Sep 15, 2015 at 11:28 AM, Ilya Enkovich <enkovich.gnu@gmail.com> wrote: >> 2015-09-13 16:36 GMT+03:00 Richard Biener <richard.guenther@gmail.com>: >>> On Mon, Sep 7, 2015 at 2:39 PM, Ilya Enkovich <enkovich.gnu@gmail.com> wrote: >>>> 2015-09-02 15:35 GMT+03:00 Richard Biener <richard.guenther@gmail.com>: >>>>> >>>>> DECL_FIELD_BIT_OFFSET should be never NULL. Whoever created that >>>>> FIELD_DECL created an invalid one. >>>>> >>>>> Richard. >>>>> >>>> >>>> layout_class_type doesn't place fields with no type >>> >>> Err - that's because fields should also have a type. >> >> Sure. But we are talking about a wrong code and still want to continue >> compilation to some point even if some field misses a type. It means >> everything possibly invoked at this stage should check type against >> error_mark_node. Thus I need to handle it gracefully in >> chkp_find_bound_slots I suppose. > > I see. I wonder why we even call chkp_find_bound_slots if seen_errors(). Even with errors we still gimplify function. Function parameters gimplification checks where parameters are passed to possibly copy some of them. It triggers ix86_function_arg_advance which uses chkp_find_bound_slots to skip required amount of bounds registers. > I suppose only recursing for COMPLETE_TYPE_P () would work? Yep, it should work. I'll rework my fix. Thanks, Ilya > > Richard. >
2015-09-15 14:01 GMT+03:00 Ilya Enkovich <enkovich.gnu@gmail.com>: > 2015-09-15 13:32 GMT+03:00 Richard Biener <richard.guenther@gmail.com>: >> On Tue, Sep 15, 2015 at 11:28 AM, Ilya Enkovich <enkovich.gnu@gmail.com> wrote: >> >> I see. I wonder why we even call chkp_find_bound_slots if seen_errors(). > > Even with errors we still gimplify function. Function parameters > gimplification checks where parameters are passed to possibly copy > some of them. It triggers ix86_function_arg_advance which uses > chkp_find_bound_slots to skip required amount of bounds registers. > >> I suppose only recursing for COMPLETE_TYPE_P () would work? > > Yep, it should work. I'll rework my fix. It turned out to be wrong. For this test struct S { S f; }; void fn1 (S p1) {} Structure S is considered as complete (has size 8 for some reason) at fn1 gimplification. Thus even with complete type check I still hit this field with error_node instead of a type and NULL at DECL_FIELD_BIT_OFFSET. Should my current fix be OK then? Thanks, Ilya
On Thu, Sep 24, 2015 at 4:07 PM, Ilya Enkovich <enkovich.gnu@gmail.com> wrote: > 2015-09-15 14:01 GMT+03:00 Ilya Enkovich <enkovich.gnu@gmail.com>: >> 2015-09-15 13:32 GMT+03:00 Richard Biener <richard.guenther@gmail.com>: >>> On Tue, Sep 15, 2015 at 11:28 AM, Ilya Enkovich <enkovich.gnu@gmail.com> wrote: >>> >>> I see. I wonder why we even call chkp_find_bound_slots if seen_errors(). >> >> Even with errors we still gimplify function. Function parameters >> gimplification checks where parameters are passed to possibly copy >> some of them. It triggers ix86_function_arg_advance which uses >> chkp_find_bound_slots to skip required amount of bounds registers. >> >>> I suppose only recursing for COMPLETE_TYPE_P () would work? >> >> Yep, it should work. I'll rework my fix. > > It turned out to be wrong. For this test > > struct S > { > S f; > }; > > void fn1 (S p1) {} > > Structure S is considered as complete (has size 8 for some reason) at > fn1 gimplification. Thus even with complete type check I still hit > this field with error_node instead of a type and NULL at > DECL_FIELD_BIT_OFFSET. Should my current fix be OK then? What's the current fix again? The NULL check on DECL_FIELD_BIT_OFFSET? I still don't like that. The frontend should leave us with something easier here :/ And I wonder if we really need to gimplify when we've seen errors (yeah, we'll get more diagnostics but also ICE-after-errors like this). Richard. > Thanks, > Ilya
diff --git a/gcc/testsuite/g++.dg/pr67405.C b/gcc/testsuite/g++.dg/pr67405.C new file mode 100644 index 0000000..5055921 --- /dev/null +++ b/gcc/testsuite/g++.dg/pr67405.C @@ -0,0 +1,11 @@ +// { dg-do compile } + +struct S +{ + S f; // { dg-error "incomplete type" } +}; + +void +fn1 (S p1) +{ +} diff --git a/gcc/tree-chkp.c b/gcc/tree-chkp.c index 8c1b48c..2489abb 100644 --- a/gcc/tree-chkp.c +++ b/gcc/tree-chkp.c @@ -1667,8 +1667,9 @@ chkp_find_bound_slots_1 (const_tree type, bitmap have_bound, for (field = TYPE_FIELDS (type); field; field = DECL_CHAIN (field)) if (TREE_CODE (field) == FIELD_DECL) { - HOST_WIDE_INT field_offs - = TREE_INT_CST_LOW (DECL_FIELD_BIT_OFFSET (field)); + HOST_WIDE_INT field_offs = 0; + if (DECL_FIELD_BIT_OFFSET (field)) + field_offs += TREE_INT_CST_LOW (DECL_FIELD_BIT_OFFSET (field)); if (DECL_FIELD_OFFSET (field)) field_offs += TREE_INT_CST_LOW (DECL_FIELD_OFFSET (field)) * 8; chkp_find_bound_slots_1 (TREE_TYPE (field), have_bound,