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
