| Message ID | 20210210223319.2507189-1-polacek@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | c-family: ICE with assume_aligned attribute [PR99062] | expand |
On 2/10/21 3:33 PM, Marek Polacek wrote: > We ICE in handle_assume_aligned_attribute since r271338 which added > > @@ -2935,8 +2936,8 @@ handle_assume_aligned_attribute (tree *node, tree name, tree args, int, > /* The misalignment specified by the second argument > must be non-negative and less than the alignment. */ > warning (OPT_Wattributes, > - "%qE attribute argument %E is not in the range [0, %E)", > - name, val, align); > + "%qE attribute argument %E is not in the range [0, %wu]", > + name, val, tree_to_uhwi (align) - 1); > *no_add_attrs = true; > return NULL_TREE; > } > because align is INT_MIN and tree_to_uhwi asserts tree_fits_uhwi_p -- which > ALIGN does not and the prior tree_fits_shwi_p check is fine with it, as > well as the integer_pow2p check. > > Since neither of the arguments to assume_aligned can be negative, I've > hoisted the tree_int_cst_sgn check. And add the missing "argument" > word to an existing warning. > > Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk/10? Thanks for taking this on! As I mentioned in the bug, I should relax the warning to understand that [x, y) is a half-open range so that these changes aren't necessary. I'm surprised that integer_pow2p() returns true for negative values. That seems like a trap for the unwary. The comment above the function says: Return 1 if EXPR is an integer constant that is a power of 2 (i.e., has only one bit on), or a location wrapper for such a constant. but an "integer power of 2" isn't the same as "has only one bit on." I would suggest to rename the function (independently of the fix for the ICE). There aren't too many uses of it so it shouldn't be too intrusive. I can do that for GCC 12 if no-one objects. Just one comment on the patch: > > gcc/c-family/ChangeLog: > > PR c++/99062 > * c-attribs.c (handle_assume_aligned_attribute): Check that the > alignment argument is non-negative. Tweak a warning message. > > gcc/testsuite/ChangeLog: > > PR c++/99062 > * gcc.dg/attr-assume_aligned-4.c: Adjust dg-warning. > * g++.dg/ext/attr-assume-aligned.C: New test. > --- > gcc/c-family/c-attribs.c | 12 ++++++++++-- > gcc/testsuite/g++.dg/ext/attr-assume-aligned.C | 5 +++++ > gcc/testsuite/gcc.dg/attr-assume_aligned-4.c | 4 ++-- > 3 files changed, 17 insertions(+), 4 deletions(-) > create mode 100644 gcc/testsuite/g++.dg/ext/attr-assume-aligned.C > > diff --git a/gcc/c-family/c-attribs.c b/gcc/c-family/c-attribs.c > index 84ec86b2091..e343429f934 100644 > --- a/gcc/c-family/c-attribs.c > +++ b/gcc/c-family/c-attribs.c > @@ -3536,7 +3536,15 @@ handle_assume_aligned_attribute (tree *node, tree name, tree args, int, > if (!tree_fits_shwi_p (val)) > { > warning (OPT_Wattributes, > - "%qE attribute %E is not an integer constant", > + "%qE attribute argument %E is not an integer constant", > + name, val); > + *no_add_attrs = true; > + return NULL_TREE; > + } > + else if (tree_int_cst_sgn (val) < 0) > + { > + warning (OPT_Wattributes, > + "%qE attribute argument %E must be non-negative", > name, val); The phrasing here doesn't sound quite right. For the test case it will print: warning: 'assume_aligned' attribute argument -1 must be non-negative which isn't possible: -1 can't be non-negative. I'd suggest either making that descriptive rather than prescriptive (along the lines of the other warnings): warning (OPT_Wattributes, "%qE attribute argument %E is not positive", name, val); or referring to the positional argument instead. Martin > *no_add_attrs = true; > return NULL_TREE; > @@ -3556,7 +3564,7 @@ handle_assume_aligned_attribute (tree *node, tree name, tree args, int, > > align = val; > } > - else if (tree_int_cst_sgn (val) < 0 || tree_int_cst_le (align, val)) > + else if (tree_int_cst_le (align, val)) > { > /* The misalignment specified by the second argument > must be non-negative and less than the alignment. */ > diff --git a/gcc/testsuite/g++.dg/ext/attr-assume-aligned.C b/gcc/testsuite/g++.dg/ext/attr-assume-aligned.C > new file mode 100644 > index 00000000000..7d073a90d74 > --- /dev/null > +++ b/gcc/testsuite/g++.dg/ext/attr-assume-aligned.C > @@ -0,0 +1,5 @@ > +// PR c++/99062 > +// { dg-do compile } > + > +#define INT_MIN (-__INT_MAX__ - 1) > +void *f () __attribute__ ((assume_aligned (INT_MIN, 4))); // { dg-warning "must be non-negative" } > diff --git a/gcc/testsuite/gcc.dg/attr-assume_aligned-4.c b/gcc/testsuite/gcc.dg/attr-assume_aligned-4.c > index 2571ab8a683..2c69c8b9f56 100644 > --- a/gcc/testsuite/gcc.dg/attr-assume_aligned-4.c > +++ b/gcc/testsuite/gcc.dg/attr-assume_aligned-4.c > @@ -8,7 +8,7 @@ A (1) void fv_1 (void); /* { dg-warning ".assume_aligned. attribute ignore > > A (1) int fi_1 (void); /* { dg-warning ".assume_aligned. attribute ignored on a function returning .int." } */ > > -A (-1) void* fpv_m1 (void); /* { dg-warning ".assume_aligned. attribute argument -1 is not a power of 2" } */ > +A (-1) void* fpv_m1 (void); /* { dg-warning ".assume_aligned. attribute argument -1 must be non-negative" } */ > > A (0) void* fpv_0 (void); /* { dg-warning ".assume_aligned. attribute argument 0 is not a power of 2" } */ > > @@ -23,7 +23,7 @@ A (16385) void* fpv_16kp1 (void); /* { dg-warning ".assume_aligned. attribute > > A (32767) void* fpv_32km1 (void); /* { dg-warning ".assume_aligned. attribute argument 32767 is not a power of 2" } */ > > -A (4, -1) void* fpv_4_m1 (void); /* { dg-warning ".assume_aligned. attribute argument -1 is not in the range \\\[0, 3]" } */ > +A (4, -1) void* fpv_4_m1 (void); /* { dg-warning ".assume_aligned. attribute argument -1 must be non-negative" } */ > > A (4, 0) void* fpv_4_0 (void); > A (4, 1) void* fpv_4_1 (void); > > base-commit: 21c6ad7a12fecc4c85ac26289d9096379b550585 >
On Wed, Feb 10, 2021 at 03:54:52PM -0700, Martin Sebor via Gcc-patches wrote: > On 2/10/21 3:33 PM, Marek Polacek wrote: > > We ICE in handle_assume_aligned_attribute since r271338 which added > > > > @@ -2935,8 +2936,8 @@ handle_assume_aligned_attribute (tree *node, tree name, tree args, int, > > /* The misalignment specified by the second argument > > must be non-negative and less than the alignment. */ > > warning (OPT_Wattributes, > > - "%qE attribute argument %E is not in the range [0, %E)", > > - name, val, align); > > + "%qE attribute argument %E is not in the range [0, %wu]", > > + name, val, tree_to_uhwi (align) - 1); > > *no_add_attrs = true; > > return NULL_TREE; > > } > > because align is INT_MIN and tree_to_uhwi asserts tree_fits_uhwi_p -- which > > ALIGN does not and the prior tree_fits_shwi_p check is fine with it, as > > well as the integer_pow2p check. > > > > Since neither of the arguments to assume_aligned can be negative, I've > > hoisted the tree_int_cst_sgn check. And add the missing "argument" > > word to an existing warning. > > > > Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk/10? > > Thanks for taking this on! As I mentioned in the bug, I should > relax the warning to understand that [x, y) is a half-open range > so that these changes aren't necessary. > > I'm surprised that integer_pow2p() returns true for negative values. > That seems like a trap for the unwary. The comment above the function > says: > > Return 1 if EXPR is an integer constant that is a power of 2 > (i.e., has only one bit on), or a location wrapper for such > a constant. > > but an "integer power of 2" isn't the same as "has only one bit > on." I would suggest to rename the function (independently of > the fix for the ICE). There aren't too many uses of it so it > shouldn't be too intrusive. I can do that for GCC 12 if no-one > objects. > > Just one comment on the patch: > > > > > gcc/c-family/ChangeLog: > > > > PR c++/99062 > > * c-attribs.c (handle_assume_aligned_attribute): Check that the > > alignment argument is non-negative. Tweak a warning message. > > > > gcc/testsuite/ChangeLog: > > > > PR c++/99062 > > * gcc.dg/attr-assume_aligned-4.c: Adjust dg-warning. > > * g++.dg/ext/attr-assume-aligned.C: New test. > > --- > > gcc/c-family/c-attribs.c | 12 ++++++++++-- > > gcc/testsuite/g++.dg/ext/attr-assume-aligned.C | 5 +++++ > > gcc/testsuite/gcc.dg/attr-assume_aligned-4.c | 4 ++-- > > 3 files changed, 17 insertions(+), 4 deletions(-) > > create mode 100644 gcc/testsuite/g++.dg/ext/attr-assume-aligned.C > > > > diff --git a/gcc/c-family/c-attribs.c b/gcc/c-family/c-attribs.c > > index 84ec86b2091..e343429f934 100644 > > --- a/gcc/c-family/c-attribs.c > > +++ b/gcc/c-family/c-attribs.c > > @@ -3536,7 +3536,15 @@ handle_assume_aligned_attribute (tree *node, tree name, tree args, int, > > if (!tree_fits_shwi_p (val)) > > { > > warning (OPT_Wattributes, > > - "%qE attribute %E is not an integer constant", > > + "%qE attribute argument %E is not an integer constant", > > + name, val); > > + *no_add_attrs = true; > > + return NULL_TREE; > > + } > > + else if (tree_int_cst_sgn (val) < 0) > > + { > > + warning (OPT_Wattributes, > > + "%qE attribute argument %E must be non-negative", > > name, val); > > The phrasing here doesn't sound quite right. For the test case it > will print: > > warning: 'assume_aligned' attribute argument -1 must be non-negative > > which isn't possible: -1 can't be non-negative. I'd suggest either > making that descriptive rather than prescriptive (along the lines > of the other warnings): > > warning (OPT_Wattributes, > "%qE attribute argument %E is not positive", > name, val); > > or referring to the positional argument instead. Good point, that was very poor wording. Fixed in v2: Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk/10? -- >8 -- We ICE in handle_assume_aligned_attribute since r271338 which added @@ -2935,8 +2936,8 @@ handle_assume_aligned_attribute (tree *node, tree name, tree args, int, /* The misalignment specified by the second argument must be non-negative and less than the alignment. */ warning (OPT_Wattributes, - "%qE attribute argument %E is not in the range [0, %E)", - name, val, align); + "%qE attribute argument %E is not in the range [0, %wu]", + name, val, tree_to_uhwi (align) - 1); *no_add_attrs = true; return NULL_TREE; } because align is INT_MIN and tree_to_uhwi asserts tree_fits_uhwi_p -- which ALIGN does not and the prior tree_fits_shwi_p check is fine with it, as well as the integer_pow2p check. Since neither of the arguments to assume_aligned can be negative, I've hoisted the tree_int_cst_sgn check. And add the missing "argument" word to an existing warning. gcc/c-family/ChangeLog: PR c++/99062 * c-attribs.c (handle_assume_aligned_attribute): Check that the alignment argument is non-negative. Tweak a warning message. gcc/testsuite/ChangeLog: PR c++/99062 * gcc.dg/attr-assume_aligned-4.c: Adjust dg-warning. * g++.dg/ext/attr-assume-aligned.C: New test. --- gcc/c-family/c-attribs.c | 11 +++++++++-- gcc/testsuite/g++.dg/ext/attr-assume-aligned.C | 5 +++++ gcc/testsuite/gcc.dg/attr-assume_aligned-4.c | 4 ++-- 3 files changed, 16 insertions(+), 4 deletions(-) create mode 100644 gcc/testsuite/g++.dg/ext/attr-assume-aligned.C diff --git a/gcc/c-family/c-attribs.c b/gcc/c-family/c-attribs.c index 84ec86b2091..0cb51fddfaa 100644 --- a/gcc/c-family/c-attribs.c +++ b/gcc/c-family/c-attribs.c @@ -3536,11 +3536,18 @@ handle_assume_aligned_attribute (tree *node, tree name, tree args, int, if (!tree_fits_shwi_p (val)) { warning (OPT_Wattributes, - "%qE attribute %E is not an integer constant", + "%qE attribute argument %E is not an integer constant", name, val); *no_add_attrs = true; return NULL_TREE; } + else if (tree_int_cst_sgn (val) < 0) + { + warning (OPT_Wattributes, + "%qE attribute argument %E is not positive", name, val); + *no_add_attrs = true; + return NULL_TREE; + } if (!align) { @@ -3556,7 +3563,7 @@ handle_assume_aligned_attribute (tree *node, tree name, tree args, int, align = val; } - else if (tree_int_cst_sgn (val) < 0 || tree_int_cst_le (align, val)) + else if (tree_int_cst_le (align, val)) { /* The misalignment specified by the second argument must be non-negative and less than the alignment. */ diff --git a/gcc/testsuite/g++.dg/ext/attr-assume-aligned.C b/gcc/testsuite/g++.dg/ext/attr-assume-aligned.C new file mode 100644 index 00000000000..aa57cbb39c7 --- /dev/null +++ b/gcc/testsuite/g++.dg/ext/attr-assume-aligned.C @@ -0,0 +1,5 @@ +// PR c++/99062 +// { dg-do compile } + +#define INT_MIN (-__INT_MAX__ - 1) +void *f () __attribute__ ((assume_aligned (INT_MIN, 4))); // { dg-warning "is not positive" } diff --git a/gcc/testsuite/gcc.dg/attr-assume_aligned-4.c b/gcc/testsuite/gcc.dg/attr-assume_aligned-4.c index 2571ab8a683..f6eb6dc4e59 100644 --- a/gcc/testsuite/gcc.dg/attr-assume_aligned-4.c +++ b/gcc/testsuite/gcc.dg/attr-assume_aligned-4.c @@ -8,7 +8,7 @@ A (1) void fv_1 (void); /* { dg-warning ".assume_aligned. attribute ignore A (1) int fi_1 (void); /* { dg-warning ".assume_aligned. attribute ignored on a function returning .int." } */ -A (-1) void* fpv_m1 (void); /* { dg-warning ".assume_aligned. attribute argument -1 is not a power of 2" } */ +A (-1) void* fpv_m1 (void); /* { dg-warning ".assume_aligned. attribute argument -1 is not positive" } */ A (0) void* fpv_0 (void); /* { dg-warning ".assume_aligned. attribute argument 0 is not a power of 2" } */ @@ -23,7 +23,7 @@ A (16385) void* fpv_16kp1 (void); /* { dg-warning ".assume_aligned. attribute A (32767) void* fpv_32km1 (void); /* { dg-warning ".assume_aligned. attribute argument 32767 is not a power of 2" } */ -A (4, -1) void* fpv_4_m1 (void); /* { dg-warning ".assume_aligned. attribute argument -1 is not in the range \\\[0, 3]" } */ +A (4, -1) void* fpv_4_m1 (void); /* { dg-warning ".assume_aligned. attribute argument -1 is not positive" } */ A (4, 0) void* fpv_4_0 (void); A (4, 1) void* fpv_4_1 (void); base-commit: 21c6ad7a12fecc4c85ac26289d9096379b550585
On 2/10/21 5:24 PM, Marek Polacek via Gcc-patches wrote: > On Wed, Feb 10, 2021 at 03:54:52PM -0700, Martin Sebor via Gcc-patches wrote: >> On 2/10/21 3:33 PM, Marek Polacek wrote: >>> We ICE in handle_assume_aligned_attribute since r271338 which added >>> >>> @@ -2935,8 +2936,8 @@ handle_assume_aligned_attribute (tree *node, tree name, tree args, int, >>> /* The misalignment specified by the second argument >>> must be non-negative and less than the alignment. */ >>> warning (OPT_Wattributes, >>> - "%qE attribute argument %E is not in the range [0, %E)", >>> - name, val, align); >>> + "%qE attribute argument %E is not in the range [0, %wu]", >>> + name, val, tree_to_uhwi (align) - 1); >>> *no_add_attrs = true; >>> return NULL_TREE; >>> } >>> because align is INT_MIN and tree_to_uhwi asserts tree_fits_uhwi_p -- which >>> ALIGN does not and the prior tree_fits_shwi_p check is fine with it, as >>> well as the integer_pow2p check. >>> >>> Since neither of the arguments to assume_aligned can be negative, I've >>> hoisted the tree_int_cst_sgn check. And add the missing "argument" >>> word to an existing warning. >>> >>> Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk/10? >> >> Thanks for taking this on! As I mentioned in the bug, I should >> relax the warning to understand that [x, y) is a half-open range >> so that these changes aren't necessary. >> >> I'm surprised that integer_pow2p() returns true for negative values. >> That seems like a trap for the unwary. The comment above the function >> says: >> >> Return 1 if EXPR is an integer constant that is a power of 2 >> (i.e., has only one bit on), or a location wrapper for such >> a constant. >> >> but an "integer power of 2" isn't the same as "has only one bit >> on." I would suggest to rename the function (independently of >> the fix for the ICE). There aren't too many uses of it so it >> shouldn't be too intrusive. I can do that for GCC 12 if no-one >> objects. >> >> Just one comment on the patch: >> >>> >>> gcc/c-family/ChangeLog: >>> >>> PR c++/99062 >>> * c-attribs.c (handle_assume_aligned_attribute): Check that the >>> alignment argument is non-negative. Tweak a warning message. >>> >>> gcc/testsuite/ChangeLog: >>> >>> PR c++/99062 >>> * gcc.dg/attr-assume_aligned-4.c: Adjust dg-warning. >>> * g++.dg/ext/attr-assume-aligned.C: New test. >>> --- >>> gcc/c-family/c-attribs.c | 12 ++++++++++-- >>> gcc/testsuite/g++.dg/ext/attr-assume-aligned.C | 5 +++++ >>> gcc/testsuite/gcc.dg/attr-assume_aligned-4.c | 4 ++-- >>> 3 files changed, 17 insertions(+), 4 deletions(-) >>> create mode 100644 gcc/testsuite/g++.dg/ext/attr-assume-aligned.C >>> >>> diff --git a/gcc/c-family/c-attribs.c b/gcc/c-family/c-attribs.c >>> index 84ec86b2091..e343429f934 100644 >>> --- a/gcc/c-family/c-attribs.c >>> +++ b/gcc/c-family/c-attribs.c >>> @@ -3536,7 +3536,15 @@ handle_assume_aligned_attribute (tree *node, tree name, tree args, int, >>> if (!tree_fits_shwi_p (val)) >>> { >>> warning (OPT_Wattributes, >>> - "%qE attribute %E is not an integer constant", >>> + "%qE attribute argument %E is not an integer constant", >>> + name, val); >>> + *no_add_attrs = true; >>> + return NULL_TREE; >>> + } >>> + else if (tree_int_cst_sgn (val) < 0) >>> + { >>> + warning (OPT_Wattributes, >>> + "%qE attribute argument %E must be non-negative", >>> name, val); >> >> The phrasing here doesn't sound quite right. For the test case it >> will print: >> >> warning: 'assume_aligned' attribute argument -1 must be non-negative >> >> which isn't possible: -1 can't be non-negative. I'd suggest either >> making that descriptive rather than prescriptive (along the lines >> of the other warnings): >> >> warning (OPT_Wattributes, >> "%qE attribute argument %E is not positive", >> name, val); >> >> or referring to the positional argument instead. > > Good point, that was very poor wording. Fixed in v2: > > Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk/10? Thanks, the rewording looks good to me! Martin > > -- >8 -- > We ICE in handle_assume_aligned_attribute since r271338 which added > > @@ -2935,8 +2936,8 @@ handle_assume_aligned_attribute (tree *node, tree name, tree args, int, > /* The misalignment specified by the second argument > must be non-negative and less than the alignment. */ > warning (OPT_Wattributes, > - "%qE attribute argument %E is not in the range [0, %E)", > - name, val, align); > + "%qE attribute argument %E is not in the range [0, %wu]", > + name, val, tree_to_uhwi (align) - 1); > *no_add_attrs = true; > return NULL_TREE; > } > because align is INT_MIN and tree_to_uhwi asserts tree_fits_uhwi_p -- which > ALIGN does not and the prior tree_fits_shwi_p check is fine with it, as > well as the integer_pow2p check. > > Since neither of the arguments to assume_aligned can be negative, I've > hoisted the tree_int_cst_sgn check. And add the missing "argument" > word to an existing warning. > > gcc/c-family/ChangeLog: > > PR c++/99062 > * c-attribs.c (handle_assume_aligned_attribute): Check that the > alignment argument is non-negative. Tweak a warning message. > > gcc/testsuite/ChangeLog: > > PR c++/99062 > * gcc.dg/attr-assume_aligned-4.c: Adjust dg-warning. > * g++.dg/ext/attr-assume-aligned.C: New test. > --- > gcc/c-family/c-attribs.c | 11 +++++++++-- > gcc/testsuite/g++.dg/ext/attr-assume-aligned.C | 5 +++++ > gcc/testsuite/gcc.dg/attr-assume_aligned-4.c | 4 ++-- > 3 files changed, 16 insertions(+), 4 deletions(-) > create mode 100644 gcc/testsuite/g++.dg/ext/attr-assume-aligned.C > > diff --git a/gcc/c-family/c-attribs.c b/gcc/c-family/c-attribs.c > index 84ec86b2091..0cb51fddfaa 100644 > --- a/gcc/c-family/c-attribs.c > +++ b/gcc/c-family/c-attribs.c > @@ -3536,11 +3536,18 @@ handle_assume_aligned_attribute (tree *node, tree name, tree args, int, > if (!tree_fits_shwi_p (val)) > { > warning (OPT_Wattributes, > - "%qE attribute %E is not an integer constant", > + "%qE attribute argument %E is not an integer constant", > name, val); > *no_add_attrs = true; > return NULL_TREE; > } > + else if (tree_int_cst_sgn (val) < 0) > + { > + warning (OPT_Wattributes, > + "%qE attribute argument %E is not positive", name, val); > + *no_add_attrs = true; > + return NULL_TREE; > + } > > if (!align) > { > @@ -3556,7 +3563,7 @@ handle_assume_aligned_attribute (tree *node, tree name, tree args, int, > > align = val; > } > - else if (tree_int_cst_sgn (val) < 0 || tree_int_cst_le (align, val)) > + else if (tree_int_cst_le (align, val)) > { > /* The misalignment specified by the second argument > must be non-negative and less than the alignment. */ > diff --git a/gcc/testsuite/g++.dg/ext/attr-assume-aligned.C b/gcc/testsuite/g++.dg/ext/attr-assume-aligned.C > new file mode 100644 > index 00000000000..aa57cbb39c7 > --- /dev/null > +++ b/gcc/testsuite/g++.dg/ext/attr-assume-aligned.C > @@ -0,0 +1,5 @@ > +// PR c++/99062 > +// { dg-do compile } > + > +#define INT_MIN (-__INT_MAX__ - 1) > +void *f () __attribute__ ((assume_aligned (INT_MIN, 4))); // { dg-warning "is not positive" } > diff --git a/gcc/testsuite/gcc.dg/attr-assume_aligned-4.c b/gcc/testsuite/gcc.dg/attr-assume_aligned-4.c > index 2571ab8a683..f6eb6dc4e59 100644 > --- a/gcc/testsuite/gcc.dg/attr-assume_aligned-4.c > +++ b/gcc/testsuite/gcc.dg/attr-assume_aligned-4.c > @@ -8,7 +8,7 @@ A (1) void fv_1 (void); /* { dg-warning ".assume_aligned. attribute ignore > > A (1) int fi_1 (void); /* { dg-warning ".assume_aligned. attribute ignored on a function returning .int." } */ > > -A (-1) void* fpv_m1 (void); /* { dg-warning ".assume_aligned. attribute argument -1 is not a power of 2" } */ > +A (-1) void* fpv_m1 (void); /* { dg-warning ".assume_aligned. attribute argument -1 is not positive" } */ > > A (0) void* fpv_0 (void); /* { dg-warning ".assume_aligned. attribute argument 0 is not a power of 2" } */ > > @@ -23,7 +23,7 @@ A (16385) void* fpv_16kp1 (void); /* { dg-warning ".assume_aligned. attribute > > A (32767) void* fpv_32km1 (void); /* { dg-warning ".assume_aligned. attribute argument 32767 is not a power of 2" } */ > > -A (4, -1) void* fpv_4_m1 (void); /* { dg-warning ".assume_aligned. attribute argument -1 is not in the range \\\[0, 3]" } */ > +A (4, -1) void* fpv_4_m1 (void); /* { dg-warning ".assume_aligned. attribute argument -1 is not positive" } */ > > A (4, 0) void* fpv_4_0 (void); > A (4, 1) void* fpv_4_1 (void); > > base-commit: 21c6ad7a12fecc4c85ac26289d9096379b550585 >
On 2/16/21 12:04 PM, Martin Sebor via Gcc-patches wrote: > On 2/10/21 5:24 PM, Marek Polacek via Gcc-patches wrote: >> On Wed, Feb 10, 2021 at 03:54:52PM -0700, Martin Sebor via >> Gcc-patches wrote: >>> On 2/10/21 3:33 PM, Marek Polacek wrote: >>>> We ICE in handle_assume_aligned_attribute since r271338 which added >>>> >>>> @@ -2935,8 +2936,8 @@ handle_assume_aligned_attribute (tree *node, >>>> tree name, tree args, int, >>>> /* The misalignment specified by the second argument >>>> must be non-negative and less than the alignment. */ >>>> warning (OPT_Wattributes, >>>> - "%qE attribute argument %E is not in the range >>>> [0, %E)", >>>> - name, val, align); >>>> + "%qE attribute argument %E is not in the range >>>> [0, %wu]", >>>> + name, val, tree_to_uhwi (align) - 1); >>>> *no_add_attrs = true; >>>> return NULL_TREE; >>>> } >>>> because align is INT_MIN and tree_to_uhwi asserts tree_fits_uhwi_p >>>> -- which >>>> ALIGN does not and the prior tree_fits_shwi_p check is fine with >>>> it, as >>>> well as the integer_pow2p check. >>>> >>>> Since neither of the arguments to assume_aligned can be negative, I've >>>> hoisted the tree_int_cst_sgn check. And add the missing "argument" >>>> word to an existing warning. >>>> >>>> Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk/10? >>> >>> Thanks for taking this on! As I mentioned in the bug, I should >>> relax the warning to understand that [x, y) is a half-open range >>> so that these changes aren't necessary. >>> >>> I'm surprised that integer_pow2p() returns true for negative values. >>> That seems like a trap for the unwary. The comment above the function >>> says: >>> >>> Return 1 if EXPR is an integer constant that is a power of 2 >>> (i.e., has only one bit on), or a location wrapper for such >>> a constant. >>> >>> but an "integer power of 2" isn't the same as "has only one bit >>> on." I would suggest to rename the function (independently of >>> the fix for the ICE). There aren't too many uses of it so it >>> shouldn't be too intrusive. I can do that for GCC 12 if no-one >>> objects. >>> >>> Just one comment on the patch: >>> >>>> >>>> gcc/c-family/ChangeLog: >>>> >>>> PR c++/99062 >>>> * c-attribs.c (handle_assume_aligned_attribute): Check that the >>>> alignment argument is non-negative. Tweak a warning message. >>>> >>>> gcc/testsuite/ChangeLog: >>>> >>>> PR c++/99062 >>>> * gcc.dg/attr-assume_aligned-4.c: Adjust dg-warning. >>>> * g++.dg/ext/attr-assume-aligned.C: New test. >>>> --- >>>> gcc/c-family/c-attribs.c | 12 ++++++++++-- >>>> gcc/testsuite/g++.dg/ext/attr-assume-aligned.C | 5 +++++ >>>> gcc/testsuite/gcc.dg/attr-assume_aligned-4.c | 4 ++-- >>>> 3 files changed, 17 insertions(+), 4 deletions(-) >>>> create mode 100644 gcc/testsuite/g++.dg/ext/attr-assume-aligned.C >>>> >>>> diff --git a/gcc/c-family/c-attribs.c b/gcc/c-family/c-attribs.c >>>> index 84ec86b2091..e343429f934 100644 >>>> --- a/gcc/c-family/c-attribs.c >>>> +++ b/gcc/c-family/c-attribs.c >>>> @@ -3536,7 +3536,15 @@ handle_assume_aligned_attribute (tree *node, >>>> tree name, tree args, int, >>>> if (!tree_fits_shwi_p (val)) >>>> { >>>> warning (OPT_Wattributes, >>>> - "%qE attribute %E is not an integer constant", >>>> + "%qE attribute argument %E is not an integer constant", >>>> + name, val); >>>> + *no_add_attrs = true; >>>> + return NULL_TREE; >>>> + } >>>> + else if (tree_int_cst_sgn (val) < 0) >>>> + { >>>> + warning (OPT_Wattributes, >>>> + "%qE attribute argument %E must be non-negative", >>>> name, val); >>> >>> The phrasing here doesn't sound quite right. For the test case it >>> will print: >>> >>> warning: 'assume_aligned' attribute argument -1 must be non-negative >>> >>> which isn't possible: -1 can't be non-negative. I'd suggest either >>> making that descriptive rather than prescriptive (along the lines >>> of the other warnings): >>> >>> warning (OPT_Wattributes, >>> "%qE attribute argument %E is not positive", >>> name, val); >>> >>> or referring to the positional argument instead. >> >> Good point, that was very poor wording. Fixed in v2: >> >> Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk/10? > Thanks, the rewording looks good to me! And with that, this is fine for the trunk. Thanks Martin & Marek. jeff
diff --git a/gcc/c-family/c-attribs.c b/gcc/c-family/c-attribs.c index 84ec86b2091..e343429f934 100644 --- a/gcc/c-family/c-attribs.c +++ b/gcc/c-family/c-attribs.c @@ -3536,7 +3536,15 @@ handle_assume_aligned_attribute (tree *node, tree name, tree args, int, if (!tree_fits_shwi_p (val)) { warning (OPT_Wattributes, - "%qE attribute %E is not an integer constant", + "%qE attribute argument %E is not an integer constant", + name, val); + *no_add_attrs = true; + return NULL_TREE; + } + else if (tree_int_cst_sgn (val) < 0) + { + warning (OPT_Wattributes, + "%qE attribute argument %E must be non-negative", name, val); *no_add_attrs = true; return NULL_TREE; @@ -3556,7 +3564,7 @@ handle_assume_aligned_attribute (tree *node, tree name, tree args, int, align = val; } - else if (tree_int_cst_sgn (val) < 0 || tree_int_cst_le (align, val)) + else if (tree_int_cst_le (align, val)) { /* The misalignment specified by the second argument must be non-negative and less than the alignment. */ diff --git a/gcc/testsuite/g++.dg/ext/attr-assume-aligned.C b/gcc/testsuite/g++.dg/ext/attr-assume-aligned.C new file mode 100644 index 00000000000..7d073a90d74 --- /dev/null +++ b/gcc/testsuite/g++.dg/ext/attr-assume-aligned.C @@ -0,0 +1,5 @@ +// PR c++/99062 +// { dg-do compile } + +#define INT_MIN (-__INT_MAX__ - 1) +void *f () __attribute__ ((assume_aligned (INT_MIN, 4))); // { dg-warning "must be non-negative" } diff --git a/gcc/testsuite/gcc.dg/attr-assume_aligned-4.c b/gcc/testsuite/gcc.dg/attr-assume_aligned-4.c index 2571ab8a683..2c69c8b9f56 100644 --- a/gcc/testsuite/gcc.dg/attr-assume_aligned-4.c +++ b/gcc/testsuite/gcc.dg/attr-assume_aligned-4.c @@ -8,7 +8,7 @@ A (1) void fv_1 (void); /* { dg-warning ".assume_aligned. attribute ignore A (1) int fi_1 (void); /* { dg-warning ".assume_aligned. attribute ignored on a function returning .int." } */ -A (-1) void* fpv_m1 (void); /* { dg-warning ".assume_aligned. attribute argument -1 is not a power of 2" } */ +A (-1) void* fpv_m1 (void); /* { dg-warning ".assume_aligned. attribute argument -1 must be non-negative" } */ A (0) void* fpv_0 (void); /* { dg-warning ".assume_aligned. attribute argument 0 is not a power of 2" } */ @@ -23,7 +23,7 @@ A (16385) void* fpv_16kp1 (void); /* { dg-warning ".assume_aligned. attribute A (32767) void* fpv_32km1 (void); /* { dg-warning ".assume_aligned. attribute argument 32767 is not a power of 2" } */ -A (4, -1) void* fpv_4_m1 (void); /* { dg-warning ".assume_aligned. attribute argument -1 is not in the range \\\[0, 3]" } */ +A (4, -1) void* fpv_4_m1 (void); /* { dg-warning ".assume_aligned. attribute argument -1 must be non-negative" } */ A (4, 0) void* fpv_4_0 (void); A (4, 1) void* fpv_4_1 (void);
We ICE in handle_assume_aligned_attribute since r271338 which added @@ -2935,8 +2936,8 @@ handle_assume_aligned_attribute (tree *node, tree name, tree args, int, /* The misalignment specified by the second argument must be non-negative and less than the alignment. */ warning (OPT_Wattributes, - "%qE attribute argument %E is not in the range [0, %E)", - name, val, align); + "%qE attribute argument %E is not in the range [0, %wu]", + name, val, tree_to_uhwi (align) - 1); *no_add_attrs = true; return NULL_TREE; } because align is INT_MIN and tree_to_uhwi asserts tree_fits_uhwi_p -- which ALIGN does not and the prior tree_fits_shwi_p check is fine with it, as well as the integer_pow2p check. Since neither of the arguments to assume_aligned can be negative, I've hoisted the tree_int_cst_sgn check. And add the missing "argument" word to an existing warning. Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk/10? gcc/c-family/ChangeLog: PR c++/99062 * c-attribs.c (handle_assume_aligned_attribute): Check that the alignment argument is non-negative. Tweak a warning message. gcc/testsuite/ChangeLog: PR c++/99062 * gcc.dg/attr-assume_aligned-4.c: Adjust dg-warning. * g++.dg/ext/attr-assume-aligned.C: New test. --- gcc/c-family/c-attribs.c | 12 ++++++++++-- gcc/testsuite/g++.dg/ext/attr-assume-aligned.C | 5 +++++ gcc/testsuite/gcc.dg/attr-assume_aligned-4.c | 4 ++-- 3 files changed, 17 insertions(+), 4 deletions(-) create mode 100644 gcc/testsuite/g++.dg/ext/attr-assume-aligned.C base-commit: 21c6ad7a12fecc4c85ac26289d9096379b550585