| Message ID | 20241002152715.4043037-1-qing.zhao@oracle.com |
|---|---|
| State | New |
| Headers | show |
| Series | C/116735 - ICE in build_counted_by_ref | expand |
On Wed, Oct 02, 2024 at 03:26:26PM +0000, Qing Zhao wrote: > From: qing zhao <qing.zhao@oracle.com> > > When handling the counted_by attribute, if the corresponding field > doesn't exit, in additiion to issue error, we should also remove > the already added non-existing "counted_by" attribute from the > field_decl. > > bootstrapped and regression tested on both x86 and aarch64. > Okay for committing? > > thanks. > > Qing For next time, the subject should look more like: [PATCH] c: ICE in build_counted_by_ref [PR116735] > ============================== > > C/PR 116735 This needs to be PR c/116735 > gcc/c/ChangeLog: > > * c-decl.cc (verify_counted_by_attribute): Remove the attribute > when error. > > gcc/testsuite/ChangeLog: > > * gcc.dg/flex-array-counted-by-pr116735.c: New test. > --- > gcc/c/c-decl.cc | 31 ++++++++++++------- > .../gcc.dg/flex-array-counted-by-pr116735.c | 19 ++++++++++++ > 2 files changed, 38 insertions(+), 12 deletions(-) > create mode 100644 gcc/testsuite/gcc.dg/flex-array-counted-by-pr116735.c > > diff --git a/gcc/c/c-decl.cc b/gcc/c/c-decl.cc > index aa7f69d1b7b..ce28b0a1022 100644 > --- a/gcc/c/c-decl.cc > +++ b/gcc/c/c-decl.cc > @@ -9502,14 +9502,18 @@ verify_counted_by_attribute (tree struct_type, tree field_decl) > > tree counted_by_field = lookup_field (struct_type, fieldname); > > - /* Error when the field is not found in the containing structure. */ > + /* Error when the field is not found in the containing structure and > + remove the corresponding counted_by attribute from the field_decl. */ > if (!counted_by_field) > - error_at (DECL_SOURCE_LOCATION (field_decl), > - "argument %qE to the %qE attribute is not a field declaration" > - " in the same structure as %qD", fieldname, > - (get_attribute_name (attr_counted_by)), > - field_decl); > - > + { > + error_at (DECL_SOURCE_LOCATION (field_decl), > + "argument %qE to the %qE attribute is not a field declaration" > + " in the same structure as %qD", fieldname, > + (get_attribute_name (attr_counted_by)), Why use get_attribute_name when we know it must be "counted_by"? And below too. > + field_decl); > + DECL_ATTRIBUTES (field_decl) > + = remove_attribute ("counted_by", DECL_ATTRIBUTES (field_decl)); > + } LGTM. > else > /* Error when the field is not with an integer type. */ > { > @@ -9518,11 +9522,14 @@ verify_counted_by_attribute (tree struct_type, tree field_decl) > tree real_field = TREE_VALUE (counted_by_field); > > if (!INTEGRAL_TYPE_P (TREE_TYPE (real_field))) > - error_at (DECL_SOURCE_LOCATION (field_decl), > - "argument %qE to the %qE attribute is not a field declaration" > - " with an integer type", fieldname, > - (get_attribute_name (attr_counted_by))); > - > + { > + error_at (DECL_SOURCE_LOCATION (field_decl), > + "argument %qE to the %qE attribute is not a field declaration" This line is too long now. > + " with an integer type", fieldname, > + (get_attribute_name (attr_counted_by))); > + DECL_ATTRIBUTES (field_decl) > + = remove_attribute ("counted_by", DECL_ATTRIBUTES (field_decl)); > + } > } Is there a test for this second hunk? > return; This return is pointless. > diff --git a/gcc/testsuite/gcc.dg/flex-array-counted-by-pr116735.c b/gcc/testsuite/gcc.dg/flex-array-counted-by-pr116735.c > new file mode 100644 > index 00000000000..958636512b7 > --- /dev/null > +++ b/gcc/testsuite/gcc.dg/flex-array-counted-by-pr116735.c Please rename this to flex-array-counted-by-9.c. > @@ -0,0 +1,19 @@ > +/* { dg-do compile } */ Please add /* PR c/116735 */ here. > +/* { dg-options "-O" } */ Why -O? > +struct foo { > + int len; > + int element[] __attribute__ ((__counted_by__ (lenx))); /* { dg-error "attribute is not a field declaration in the same structure as" } */ > +}; > + > +int main () > +{ > + struct foo *p = __builtin_malloc (sizeof (struct foo) + 3 * sizeof (int)); > + p->len = 1; > + p->element[0] = 17; > + p->len = 2; > + p->element[1] = 13; > + p->len = 1; > + int x = p->element[1]; > + return x; > +} > + > -- > 2.43.5 > Thanks, Marek
On Wed, Oct 02, 2024 at 11:48:16AM -0400, Marek Polacek wrote: > > + error_at (DECL_SOURCE_LOCATION (field_decl), > > + "argument %qE to the %qE attribute is not a field declaration" > > + " in the same structure as %qD", fieldname, > > + (get_attribute_name (attr_counted_by)), > > Why use get_attribute_name when we know it must be "counted_by"? And below > too. There might be a reason if the message would be used by multiple spots with different attributes and the other uses would need that %qE, rather than say %qs or %<counted_by%> (to make it easier for translators). If the message is only for this attribute, just use %<counted_by%>, or if it would be for several attributes but in each case you'd know the name as constant literal, %qs with "counted_by" operand would be best. That said, the ()s around the call are also superfluous, so if it isn't changed, it should be just get_attribute_name (attr_counted_by), Jakub
> On Oct 2, 2024, at 11:48, Marek Polacek <polacek@redhat.com> wrote: > > On Wed, Oct 02, 2024 at 03:26:26PM +0000, Qing Zhao wrote: >> From: qing zhao <qing.zhao@oracle.com> >> >> When handling the counted_by attribute, if the corresponding field >> doesn't exit, in additiion to issue error, we should also remove >> the already added non-existing "counted_by" attribute from the >> field_decl. >> >> bootstrapped and regression tested on both x86 and aarch64. >> Okay for committing? >> >> thanks. >> >> Qing > > For next time, the subject should look more like: > [PATCH] c: ICE in build_counted_by_ref [PR116735] Okay. > >> ============================== >> >> C/PR 116735 > > This needs to be PR c/116735 Okay. > >> gcc/c/ChangeLog: >> >> * c-decl.cc (verify_counted_by_attribute): Remove the attribute >> when error. >> >> gcc/testsuite/ChangeLog: >> >> * gcc.dg/flex-array-counted-by-pr116735.c: New test. >> --- >> gcc/c/c-decl.cc | 31 ++++++++++++------- >> .../gcc.dg/flex-array-counted-by-pr116735.c | 19 ++++++++++++ >> 2 files changed, 38 insertions(+), 12 deletions(-) >> create mode 100644 gcc/testsuite/gcc.dg/flex-array-counted-by-pr116735.c >> >> diff --git a/gcc/c/c-decl.cc b/gcc/c/c-decl.cc >> index aa7f69d1b7b..ce28b0a1022 100644 >> --- a/gcc/c/c-decl.cc >> +++ b/gcc/c/c-decl.cc >> @@ -9502,14 +9502,18 @@ verify_counted_by_attribute (tree struct_type, tree field_decl) >> >> tree counted_by_field = lookup_field (struct_type, fieldname); >> >> - /* Error when the field is not found in the containing structure. */ >> + /* Error when the field is not found in the containing structure and >> + remove the corresponding counted_by attribute from the field_decl. */ >> if (!counted_by_field) >> - error_at (DECL_SOURCE_LOCATION (field_decl), >> - "argument %qE to the %qE attribute is not a field declaration" >> - " in the same structure as %qD", fieldname, >> - (get_attribute_name (attr_counted_by)), >> - field_decl); >> - >> + { >> + error_at (DECL_SOURCE_LOCATION (field_decl), >> + "argument %qE to the %qE attribute is not a field declaration" >> + " in the same structure as %qD", fieldname, >> + (get_attribute_name (attr_counted_by)), > > Why use get_attribute_name when we know it must be "counted_by"? And below > too. > >> + field_decl); >> + DECL_ATTRIBUTES (field_decl) >> + = remove_attribute ("counted_by", DECL_ATTRIBUTES (field_decl)); >> + } > > LGTM. > >> else >> /* Error when the field is not with an integer type. */ >> { >> @@ -9518,11 +9522,14 @@ verify_counted_by_attribute (tree struct_type, tree field_decl) >> tree real_field = TREE_VALUE (counted_by_field); >> >> if (!INTEGRAL_TYPE_P (TREE_TYPE (real_field))) >> - error_at (DECL_SOURCE_LOCATION (field_decl), >> - "argument %qE to the %qE attribute is not a field declaration" >> - " with an integer type", fieldname, >> - (get_attribute_name (attr_counted_by))); >> - >> + { >> + error_at (DECL_SOURCE_LOCATION (field_decl), >> + "argument %qE to the %qE attribute is not a field declaration" > > This line is too long now. Okay, will fix this. > >> + " with an integer type", fieldname, >> + (get_attribute_name (attr_counted_by))); >> + DECL_ATTRIBUTES (field_decl) >> + = remove_attribute ("counted_by", DECL_ATTRIBUTES (field_decl)); >> + } >> } > > Is there a test for this second hunk? Will add one. > >> return; > > This return is pointless. > >> diff --git a/gcc/testsuite/gcc.dg/flex-array-counted-by-pr116735.c b/gcc/testsuite/gcc.dg/flex-array-counted-by-pr116735.c >> new file mode 100644 >> index 00000000000..958636512b7 >> --- /dev/null >> +++ b/gcc/testsuite/gcc.dg/flex-array-counted-by-pr116735.c > > Please rename this to flex-array-counted-by-9.c. Sure. > >> @@ -0,0 +1,19 @@ >> +/* { dg-do compile } */ > > Please add /* PR c/116735 */ here. sure. > >> +/* { dg-options "-O" } */ > > Why -O? Delete the optimization flag should also repeat the issue, I will delete it. thanks. Qing > >> +struct foo { >> + int len; >> + int element[] __attribute__ ((__counted_by__ (lenx))); /* { dg-error "attribute is not a field declaration in the same structure as" } */ >> +}; >> + >> +int main () >> +{ >> + struct foo *p = __builtin_malloc (sizeof (struct foo) + 3 * sizeof (int)); >> + p->len = 1; >> + p->element[0] = 17; >> + p->len = 2; >> + p->element[1] = 13; >> + p->len = 1; >> + int x = p->element[1]; >> + return x; >> +} >> + >> -- >> 2.43.5 >> > > Thanks, > > Marek
> On Oct 2, 2024, at 12:05, Jakub Jelinek <jakub@redhat.com> wrote: > > On Wed, Oct 02, 2024 at 11:48:16AM -0400, Marek Polacek wrote: >>> + error_at (DECL_SOURCE_LOCATION (field_decl), >>> + "argument %qE to the %qE attribute is not a field declaration" >>> + " in the same structure as %qD", fieldname, >>> + (get_attribute_name (attr_counted_by)), >> >> Why use get_attribute_name when we know it must be "counted_by"? And below >> too. > > There might be a reason if the message would be used by multiple > spots with different attributes and the other uses would need that %qE, > rather than say %qs or %<counted_by%> (to make it easier for translators). > If the message is only for this attribute, just use %<counted_by%>, or > if it would be for several attributes but in each case you'd know the name > as constant literal, %qs with "counted_by" operand would be best. > > That said, the ()s around the call are also superfluous, so if it isn't > changed, it should be just > get_attribute_name (attr_counted_by), Sure, I will fix this in the next version. thanks. Qing > > Jakub >
diff --git a/gcc/c/c-decl.cc b/gcc/c/c-decl.cc index aa7f69d1b7b..ce28b0a1022 100644 --- a/gcc/c/c-decl.cc +++ b/gcc/c/c-decl.cc @@ -9502,14 +9502,18 @@ verify_counted_by_attribute (tree struct_type, tree field_decl) tree counted_by_field = lookup_field (struct_type, fieldname); - /* Error when the field is not found in the containing structure. */ + /* Error when the field is not found in the containing structure and + remove the corresponding counted_by attribute from the field_decl. */ if (!counted_by_field) - error_at (DECL_SOURCE_LOCATION (field_decl), - "argument %qE to the %qE attribute is not a field declaration" - " in the same structure as %qD", fieldname, - (get_attribute_name (attr_counted_by)), - field_decl); - + { + error_at (DECL_SOURCE_LOCATION (field_decl), + "argument %qE to the %qE attribute is not a field declaration" + " in the same structure as %qD", fieldname, + (get_attribute_name (attr_counted_by)), + field_decl); + DECL_ATTRIBUTES (field_decl) + = remove_attribute ("counted_by", DECL_ATTRIBUTES (field_decl)); + } else /* Error when the field is not with an integer type. */ { @@ -9518,11 +9522,14 @@ verify_counted_by_attribute (tree struct_type, tree field_decl) tree real_field = TREE_VALUE (counted_by_field); if (!INTEGRAL_TYPE_P (TREE_TYPE (real_field))) - error_at (DECL_SOURCE_LOCATION (field_decl), - "argument %qE to the %qE attribute is not a field declaration" - " with an integer type", fieldname, - (get_attribute_name (attr_counted_by))); - + { + error_at (DECL_SOURCE_LOCATION (field_decl), + "argument %qE to the %qE attribute is not a field declaration" + " with an integer type", fieldname, + (get_attribute_name (attr_counted_by))); + DECL_ATTRIBUTES (field_decl) + = remove_attribute ("counted_by", DECL_ATTRIBUTES (field_decl)); + } } return; diff --git a/gcc/testsuite/gcc.dg/flex-array-counted-by-pr116735.c b/gcc/testsuite/gcc.dg/flex-array-counted-by-pr116735.c new file mode 100644 index 00000000000..958636512b7 --- /dev/null +++ b/gcc/testsuite/gcc.dg/flex-array-counted-by-pr116735.c @@ -0,0 +1,19 @@ +/* { dg-do compile } */ +/* { dg-options "-O" } */ +struct foo { + int len; + int element[] __attribute__ ((__counted_by__ (lenx))); /* { dg-error "attribute is not a field declaration in the same structure as" } */ +}; + +int main () +{ + struct foo *p = __builtin_malloc (sizeof (struct foo) + 3 * sizeof (int)); + p->len = 1; + p->element[0] = 17; + p->len = 2; + p->element[1] = 13; + p->len = 1; + int x = p->element[1]; + return x; +} +
From: qing zhao <qing.zhao@oracle.com> When handling the counted_by attribute, if the corresponding field doesn't exit, in additiion to issue error, we should also remove the already added non-existing "counted_by" attribute from the field_decl. bootstrapped and regression tested on both x86 and aarch64. Okay for committing? thanks. Qing ============================== C/PR 116735 gcc/c/ChangeLog: * c-decl.cc (verify_counted_by_attribute): Remove the attribute when error. gcc/testsuite/ChangeLog: * gcc.dg/flex-array-counted-by-pr116735.c: New test. --- gcc/c/c-decl.cc | 31 ++++++++++++------- .../gcc.dg/flex-array-counted-by-pr116735.c | 19 ++++++++++++ 2 files changed, 38 insertions(+), 12 deletions(-) create mode 100644 gcc/testsuite/gcc.dg/flex-array-counted-by-pr116735.c