[RFA] c-family: attribute ((aligned, mode)) [PR100545]
Commit Message
The problem here was that handle_mode_attribute clobbered the changes of any
previous attribute, only copying type qualifiers to the new type. And
common_handle_aligned_attribute had previously set up the typedef, so when
we later called set_underlying_type it saw DECL_ORIGINAL_TYPE set and just
returned, even though handle_mode_attribute had messed up the TREE_TYPE.
So, let's fix handle_mode_attribute to copy attributes, alignment, and
typedefness to the new type.
Tested x86_64-pc-linux-gnu, OK for trunk now or after 12.1?
PR c/100545
gcc/c-family/ChangeLog:
* c-attribs.cc (handle_mode_attribute): Copy attributes, aligned,
and typedef.
* c-common.cc (set_underlying_type): Add assert.
gcc/testsuite/ChangeLog:
* c-c++-common/attr-mode-1.c: New test.
---
gcc/c-family/c-attribs.cc | 15 ++++++++++++++-
gcc/c-family/c-common.cc | 7 ++++---
gcc/testsuite/c-c++-common/attr-mode-1.c | 3 +++
3 files changed, 21 insertions(+), 4 deletions(-)
create mode 100644 gcc/testsuite/c-c++-common/attr-mode-1.c
base-commit: 6a460a2007dd9c527c5f9d5bbbedb852db7c1373
Comments
On Wed, Apr 27, 2022 at 11:19:57AM -0400, Jason Merrill via Gcc-patches wrote:
> The problem here was that handle_mode_attribute clobbered the changes of any
> previous attribute, only copying type qualifiers to the new type. And
> common_handle_aligned_attribute had previously set up the typedef, so when
> we later called set_underlying_type it saw DECL_ORIGINAL_TYPE set and just
> returned, even though handle_mode_attribute had messed up the TREE_TYPE.
> So, let's fix handle_mode_attribute to copy attributes, alignment, and
> typedefness to the new type.
>
> Tested x86_64-pc-linux-gnu, OK for trunk now or after 12.1?
I think I'd slightly prefer 12.1, it doesn't seem to be a regression.
> PR c/100545
>
> gcc/c-family/ChangeLog:
>
> * c-attribs.cc (handle_mode_attribute): Copy attributes, aligned,
> and typedef.
> * c-common.cc (set_underlying_type): Add assert.
>
> gcc/testsuite/ChangeLog:
>
> * c-c++-common/attr-mode-1.c: New test.
> ---
> gcc/c-family/c-attribs.cc | 15 ++++++++++++++-
> gcc/c-family/c-common.cc | 7 ++++---
> gcc/testsuite/c-c++-common/attr-mode-1.c | 3 +++
> 3 files changed, 21 insertions(+), 4 deletions(-)
> create mode 100644 gcc/testsuite/c-c++-common/attr-mode-1.c
>
> diff --git a/gcc/c-family/c-attribs.cc b/gcc/c-family/c-attribs.cc
> index 111a33f405a..26876d0f309 100644
> --- a/gcc/c-family/c-attribs.cc
> +++ b/gcc/c-family/c-attribs.cc
> @@ -2199,7 +2199,20 @@ handle_mode_attribute (tree *node, tree name, tree args,
> return NULL_TREE;
> }
>
> - *node = build_qualified_type (typefm, TYPE_QUALS (type));
> + /* Copy any quals and attributes to the new type. */
> + *node = build_type_attribute_qual_variant (typefm, TYPE_ATTRIBUTES (type),
> + TYPE_QUALS (type));
> + if (TYPE_USER_ALIGN (type))
> + *node = build_aligned_type (*node, TYPE_ALIGN (type));
> + if (typedef_variant_p (type))
> + {
> + /* Set up the typedef all over again. */
> + tree decl = TYPE_NAME (type);
> + DECL_ORIGINAL_TYPE (decl) = NULL_TREE;
> + TREE_TYPE (decl) = *node;
> + set_underlying_type (decl);
> + *node = TREE_TYPE (decl);
> + }
> }
>
> return NULL_TREE;
> diff --git a/gcc/c-family/c-common.cc b/gcc/c-family/c-common.cc
> index bb0544eeaea..730faa9e87f 100644
> --- a/gcc/c-family/c-common.cc
> +++ b/gcc/c-family/c-common.cc
> @@ -8153,15 +8153,16 @@ check_missing_format_attribute (tree ltype, tree rtype)
> void
> set_underlying_type (tree x)
> {
> - if (x == error_mark_node)
> + if (x == error_mark_node || TREE_TYPE (x) == error_mark_node)
Maybe error_operand_p?
> return;
> if (DECL_IS_UNDECLARED_BUILTIN (x) && TREE_CODE (TREE_TYPE (x)) != ARRAY_TYPE)
> {
> if (TYPE_NAME (TREE_TYPE (x)) == 0)
> TYPE_NAME (TREE_TYPE (x)) = x;
> }
> - else if (TREE_TYPE (x) != error_mark_node
> - && DECL_ORIGINAL_TYPE (x) == NULL_TREE)
> + else if (DECL_ORIGINAL_TYPE (x))
> + gcc_checking_assert (TYPE_NAME (TREE_TYPE (x)) == x);
> + else
> {
> tree tt = TREE_TYPE (x);
> DECL_ORIGINAL_TYPE (x) = tt;
> diff --git a/gcc/testsuite/c-c++-common/attr-mode-1.c b/gcc/testsuite/c-c++-common/attr-mode-1.c
> new file mode 100644
> index 00000000000..59b20cd99e8
> --- /dev/null
> +++ b/gcc/testsuite/c-c++-common/attr-mode-1.c
> @@ -0,0 +1,3 @@
> +// PR c/100545
> +
> +typedef int fatp_t __attribute__((aligned, mode(SI)));
I think you need to add "dg-options -g", otherwise the bug doesn't
manifest.
Marek
On Wed, 27 Apr 2022, Jason Merrill via Gcc-patches wrote:
> + if (typedef_variant_p (type))
> + {
> + /* Set up the typedef all over again. */
This seems wrong when the typedef is just being used in another
declaration with the mode attribute, as opposed to being defined using the
mode attribute. E.g. the following test is valid and accepted before the
patch, but wrongly rejected after the patch because the typedef has had
its type changed.
typedef int I;
int x;
I y __attribute__ ((mode(QI)));
extern I x;
On 4/27/22 13:02, Joseph Myers wrote:
> On Wed, 27 Apr 2022, Jason Merrill via Gcc-patches wrote:
>
>> + if (typedef_variant_p (type))
>> + {
>> + /* Set up the typedef all over again. */
>
> This seems wrong when the typedef is just being used in another
> declaration with the mode attribute, as opposed to being defined using the
> mode attribute. E.g. the following test is valid and accepted before the
> patch, but wrongly rejected after the patch because the typedef has had
> its type changed.
>
> typedef int I;
> int x;
> I y __attribute__ ((mode(QI)));
> extern I x;
Ah, good point. Fixed thus:
On 4/27/22 19:48, Jason Merrill wrote:
> On 4/27/22 13:02, Joseph Myers wrote:
>> On Wed, 27 Apr 2022, Jason Merrill via Gcc-patches wrote:
>>
>>> + if (typedef_variant_p (type))
>>> + {
>>> + /* Set up the typedef all over again. */
>>
>> This seems wrong when the typedef is just being used in another
>> declaration with the mode attribute, as opposed to being defined using
>> the
>> mode attribute. E.g. the following test is valid and accepted before the
>> patch, but wrongly rejected after the patch because the typedef has had
>> its type changed.
>>
>> typedef int I;
>> int x;
>> I y __attribute__ ((mode(QI)));
>> extern I x;
>
> Ah, good point. Fixed thus:
Marek pointed out elsewhere that the first testcase should have
// { dg-additional-options -g }
OK for 13 with that change?
On Fri, 29 Apr 2022, Jason Merrill via Gcc-patches wrote:
> Marek pointed out elsewhere that the first testcase should have
>
> // { dg-additional-options -g }
>
> OK for 13 with that change?
OK.
@@ -2199,7 +2199,20 @@ handle_mode_attribute (tree *node, tree name, tree args,
return NULL_TREE;
}
- *node = build_qualified_type (typefm, TYPE_QUALS (type));
+ /* Copy any quals and attributes to the new type. */
+ *node = build_type_attribute_qual_variant (typefm, TYPE_ATTRIBUTES (type),
+ TYPE_QUALS (type));
+ if (TYPE_USER_ALIGN (type))
+ *node = build_aligned_type (*node, TYPE_ALIGN (type));
+ if (typedef_variant_p (type))
+ {
+ /* Set up the typedef all over again. */
+ tree decl = TYPE_NAME (type);
+ DECL_ORIGINAL_TYPE (decl) = NULL_TREE;
+ TREE_TYPE (decl) = *node;
+ set_underlying_type (decl);
+ *node = TREE_TYPE (decl);
+ }
}
return NULL_TREE;
@@ -8153,15 +8153,16 @@ check_missing_format_attribute (tree ltype, tree rtype)
void
set_underlying_type (tree x)
{
- if (x == error_mark_node)
+ if (x == error_mark_node || TREE_TYPE (x) == error_mark_node)
return;
if (DECL_IS_UNDECLARED_BUILTIN (x) && TREE_CODE (TREE_TYPE (x)) != ARRAY_TYPE)
{
if (TYPE_NAME (TREE_TYPE (x)) == 0)
TYPE_NAME (TREE_TYPE (x)) = x;
}
- else if (TREE_TYPE (x) != error_mark_node
- && DECL_ORIGINAL_TYPE (x) == NULL_TREE)
+ else if (DECL_ORIGINAL_TYPE (x))
+ gcc_checking_assert (TYPE_NAME (TREE_TYPE (x)) == x);
+ else
{
tree tt = TREE_TYPE (x);
DECL_ORIGINAL_TYPE (x) = tt;
new file mode 100644
@@ -0,0 +1,3 @@
+// PR c/100545
+
+typedef int fatp_t __attribute__((aligned, mode(SI)));