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
--
2.29.2
