I have removed both calls to remove_attribute because with the changes
to calls.c they are no longer needed. The revised patch is attached.
I will plan to commit it tomorrow unless there's something else.
In hindsight, I think changing the attribute from the human-readable
form to the internal, condensed, form was a mistake (it was done to
address concerns about the costs of parsing the attribute raised
during code review). I don't want to make further changes here now
but I will plan to revisit it in stage 1.
I would still appreciate answers to the questions below so I can work
on beefing up the verification in decl_attributes (also in stage 1).
Thanks
Martin
On 3/26/20 2:57 PM, Martin Sebor wrote:
On 3/25/20 3:56 PM, Jason Merrill wrote:
On 3/16/20 4:41 PM, Martin Sebor wrote:
The recent fix to avoid modifying in place the type argument in
handle_access_attribute (PR 92721) was incomplete and didn't fully
resolve the problem (an ICE in the C++ front-end). The attached
patch removes the remaining modification that causes the ICE. In
addition, the change adjusts checking calls to functions declared
with the attribute to scan all its instances.
The attached patch was tested on x86_64-linux.
attrs = remove_attribute (IDENTIFIER_POINTER (name), attrs);
This unchanged line can still modify existing types; I think if attrs
already has a matching attribute you need to work harder to replace it.
I have a number of questions.
1) There are two instances of the call above in the code (only one in
the context of the patch). Are you referring specifically to the one
in the patch or to both? The one in the patch manipulates the type
attributes of the last DECL (NODE[1]). The other one, attributes of
the current type (NODE[0]). Are both a problem?
2) What all do you include in "modifying existing types?" A number
of attribute handlers unconditionally modify the type of a *NODE
without first testing ATTR_FLAG_TYPE_IN_PLACE (e.g., by setting
TYPE_USER_ALIGN, or TREE_USED). Are those modifications safe? If
they are, what makes the difference between those and the code above?
3) Is the restriction on "modifying existing types" specific to
the C++ front end or a general one that applies to the whole C/C++
family? (I've only seen a failure in the C++ FE.)
4) What exactly does "work harder" mean and how do I test it? Are
you suggesting to call build_type_attribute_variant (or maybe
build_variant_type_copy like the existing handlers do) to replace
the type? Or are you implying that unless I need to replace
the existing type I should avoid modifying the TYPE's
TYPE_ATTRIBUTES and instead work with a copy of the attribute chain?
These restrictions and the lack of enforcement (only in the C++ FE)
make nontrivial handlers extremely fragile. Unless the restrictions
are entirely C++-specific I would really like to add assertions to
decl_attributes to catch these problems earlier (and in C as well).
Either way, I also want to add test cases to exercise them. But
I need to understand them first (i.e., get answers to the questions
above). Nothing I've tried so far has triggered a problem due to
the code you point out above.
Martin
PR c++/94098 - ICE on attribute access redeclaration
gcc/c-family/ChangeLog:
PR c++/94098
* c-attribs.c (handle_access_attribute): Avoid setting TYPE_ATTRIBUTES
here.
gcc/ChangeLog:
PR c++/94098
* calls.c (init_attr_rdwr_indices): Iterate over all access attributes.
gcc/testsuite/ChangeLog:
PR c++/94098
* g++.dg/ext/attr-access-2.C: New test.
diff --git a/gcc/c-family/c-attribs.c b/gcc/c-family/c-attribs.c
index 9abf81d0248..f30158a258b 100644
--- a/gcc/c-family/c-attribs.c
+++ b/gcc/c-family/c-attribs.c
@@ -4182,7 +4182,6 @@ handle_access_attribute (tree *node, tree name, tree args,
/* Replace any existing access attribute specification with
the concatenation above. */
- attrs = remove_attribute (IDENTIFIER_POINTER (name), attrs);
new_attrs = tree_cons (NULL_TREE, new_attrs, NULL_TREE);
new_attrs = tree_cons (name, new_attrs, attrs);
@@ -4190,15 +4189,12 @@ handle_access_attribute (tree *node, tree name, tree args,
{
/* Repeat for the previously declared type. */
attrs = TYPE_ATTRIBUTES (TREE_TYPE (node[1]));
- tree new_attrs
- = append_access_attrs (node[1], attrs, attrstr, code, idxs);
- if (!new_attrs)
+ tree attrs1 = append_access_attrs (node[1], attrs, attrstr, code, idxs);
+ if (!attrs1)
return NULL_TREE;
- attrs = remove_attribute (IDENTIFIER_POINTER (name), attrs);
- new_attrs = tree_cons (NULL_TREE, new_attrs, NULL_TREE);
- new_attrs = tree_cons (name, new_attrs, attrs);
- TYPE_ATTRIBUTES (TREE_TYPE (node[1])) = new_attrs;
+ attrs1 = tree_cons (NULL_TREE, attrs1, NULL_TREE);
+ new_attrs = tree_cons (name, attrs1, attrs);
}
/* Recursively call self to "replace" the documented/external form
diff --git a/gcc/calls.c b/gcc/calls.c
index 4c3a8f3c215..5bd922779af 100644
--- a/gcc/calls.c
+++ b/gcc/calls.c
@@ -1873,7 +1873,7 @@ struct rdwr_access_hash: int_hash<int, -1> { };
typedef hash_map<rdwr_access_hash, attr_access> rdwr_map;
/* Initialize a mapping for a call to function FNDECL declared with
- attribute access. Each attribute poisitional operand inserts one
+ attribute access. Each attribute positional operand inserts one
entry into the mapping with the operand number as the key. */
static void
@@ -1882,54 +1882,50 @@ init_attr_rdwr_indices (rdwr_map *rwm, tree fntype)
if (!fntype)
return;
- tree access = TYPE_ATTRIBUTES (fntype);
- /* If the function's type has no attributes there's nothing to do. */
- if (!access)
- return;
-
- access = lookup_attribute ("access", access);
- if (!access)
- return;
-
- /* The TREE_VALUE of an attribute is a TREE_LIST whose TREE_VALUE
- is the attribute argument's value. */
- tree mode = TREE_VALUE (access);
- gcc_assert (TREE_CODE (mode) == TREE_LIST);
- mode = TREE_VALUE (mode);
- gcc_assert (TREE_CODE (mode) == STRING_CST);
-
- const char *modestr = TREE_STRING_POINTER (mode);
- for (const char *m = modestr; *m; )
+ for (tree access = TYPE_ATTRIBUTES (fntype);
+ (access = lookup_attribute ("access", access));
+ access = TREE_CHAIN (access))
{
- attr_access acc = { };
-
- switch (*m)
+ /* The TREE_VALUE of an attribute is a TREE_LIST whose TREE_VALUE
+ is the attribute argument's value. */
+ tree mode = TREE_VALUE (access);
+ gcc_assert (TREE_CODE (mode) == TREE_LIST);
+ mode = TREE_VALUE (mode);
+ gcc_assert (TREE_CODE (mode) == STRING_CST);
+
+ const char *modestr = TREE_STRING_POINTER (mode);
+ for (const char *m = modestr; *m; )
{
- case 'r': acc.mode = acc.read_only; break;
- case 'w': acc.mode = acc.write_only; break;
- default: acc.mode = acc.read_write; break;
- }
+ attr_access acc = { };
- char *end;
- acc.ptrarg = strtoul (++m, &end, 10);
- m = end;
- if (*m == ',')
- {
- acc.sizarg = strtoul (++m, &end, 10);
+ switch (*m)
+ {
+ case 'r': acc.mode = acc.read_only; break;
+ case 'w': acc.mode = acc.write_only; break;
+ default: acc.mode = acc.read_write; break;
+ }
+
+ char *end;
+ acc.ptrarg = strtoul (++m, &end, 10);
m = end;
- }
- else
- acc.sizarg = UINT_MAX;
+ if (*m == ',')
+ {
+ acc.sizarg = strtoul (++m, &end, 10);
+ m = end;
+ }
+ else
+ acc.sizarg = UINT_MAX;
- acc.ptr = NULL_TREE;
- acc.size = NULL_TREE;
+ acc.ptr = NULL_TREE;
+ acc.size = NULL_TREE;
- /* Unconditionally add an entry for the required pointer
- operand of the attribute, and one for the optional size
- operand when it's specified. */
- rwm->put (acc.ptrarg, acc);
- if (acc.sizarg != UINT_MAX)
- rwm->put (acc.sizarg, acc);
+ /* Unconditionally add an entry for the required pointer
+ operand of the attribute, and one for the optional size
+ operand when it's specified. */
+ rwm->put (acc.ptrarg, acc);
+ if (acc.sizarg != UINT_MAX)
+ rwm->put (acc.sizarg, acc);
+ }
}
}
diff --git a/gcc/testsuite/g++.dg/ext/attr-access-2.C b/gcc/testsuite/g++.dg/ext/attr-access-2.C
new file mode 100644
index 00000000000..46f9075a6c6
--- /dev/null
+++ b/gcc/testsuite/g++.dg/ext/attr-access-2.C
@@ -0,0 +1,88 @@
+/* PR c++/94098 - checking ICE on attribute access redeclaration
+ { dg-do compile }
+ { dg-options "-Wall" } */
+
+#define RO(...) __attribute__ ((access (read_only, __VA_ARGS__)))
+#define RW(...) __attribute__ ((access (read_write, __VA_ARGS__)))
+#define WO(...) __attribute__ ((access (write_only, __VA_ARGS__)))
+
+typedef __INT32_TYPE__ int32_t;
+
+int rdwr1_2_3_4 (void*, void*, void*, void*);
+int RW (1) rdwr1_2_3_4 (void*, void*, void*, void*);
+int RW (2) rdwr1_2_3_4 (void*, void*, void*, void*);
+int RW (3) rdwr1_2_3_4 (void*, void*, void*, void*);
+int RW (4) rdwr1_2_3_4 (void*, void*, void*, void*);
+
+extern int32_t x[1];
+
+void call_rdwrp1_2_3_4 (void)
+{
+ rdwr1_2_3_4 (x, x, x, x);
+ rdwr1_2_3_4 (x, x, x, x + 1); // { dg-warning "\\\[-Wstringop-overflow" }
+ rdwr1_2_3_4 (x, x, x + 1, x); // { dg-warning "\\\[-Wstringop-overflow" }
+ rdwr1_2_3_4 (x, x + 1, x, x); // { dg-warning "\\\[-Wstringop-overflow" }
+ rdwr1_2_3_4 (x + 1, x, x, x); // { dg-warning "\\\[-Wstringop-overflow" }
+}
+
+
+int rdwr4_3_2_1 (void*, void*, void*, void*);
+int RW (4) rdwr4_3_2_1 (void*, void*, void*, void*);
+int RW (3) rdwr4_3_2_1 (void*, void*, void*, void*);
+int RW (2) rdwr4_3_2_1 (void*, void*, void*, void*);
+int RW (1) rdwr4_3_2_1 (void*, void*, void*, void*);
+
+void call_rdwr4_3_2_1 (void)
+{
+ rdwr4_3_2_1 (x, x, x, x);
+ rdwr4_3_2_1 (x, x, x, x + 1); // { dg-warning "\\\[-Wstringop-overflow" }
+ rdwr4_3_2_1 (x, x, x + 1, x); // { dg-warning "\\\[-Wstringop-overflow" }
+ rdwr4_3_2_1 (x, x + 1, x, x); // { dg-warning "\\\[-Wstringop-overflow" }
+ rdwr4_3_2_1 (x + 1, x, x, x); // { dg-warning "\\\[-Wstringop-overflow" }
+}
+
+
+int rdwrall (void*, void*, void*, void*);
+int RW (1) rdwrall (void*, void*, void*, void*);
+int RW (1) RW (2) rdwrall (void*, void*, void*, void*);
+int RW (1) RW (2) RW (3) rdwrall (void*, void*, void*, void*);
+int RW (1) RW (2) RW (3) RW (4) rdwrall (void*, void*, void*, void*);
+
+void call_rdwrall (void)
+{
+ rdwrall (x, x, x, x);
+ rdwrall (x, x, x, x + 1); // { dg-warning "\\\[-Wstringop-overflow" }
+ rdwrall (x, x, x + 1, x); // { dg-warning "\\\[-Wstringop-overflow" }
+ rdwrall (x, x + 1, x, x); // { dg-warning "\\\[-Wstringop-overflow" }
+ rdwrall (x + 1, x, x, x); // { dg-warning "\\\[-Wstringop-overflow" }
+}
+
+
+// Verify the attribute is a part of the function's type.
+typedef __typeof__ (rdwrall) F;
+
+void call_fnptr_typeof (F *f)
+{
+ f (x, x, x, x);
+ f (x, x, x, x + 1); // { dg-warning "\\\[-Wstringop-overflow" }
+ f (x, x, x + 1, x); // { dg-warning "\\\[-Wstringop-overflow" }
+ f (x, x + 1, x, x); // { dg-warning "\\\[-Wstringop-overflow" }
+ f (x + 1, x, x, x); // { dg-warning "\\\[-Wstringop-overflow" }
+}
+
+
+// Verify the attribute is effective on a typedef.
+typedef void FWRall (void*, void*, void*, void*);
+typedef RW (1) void FWRall (void*, void*, void*, void*);
+typedef RW (2) void FWRall (void*, void*, void*, void*);
+typedef RW (3) void FWRall (void*, void*, void*, void*);
+typedef RW (4) void FWRall (void*, void*, void*, void*);
+
+void call_fnptr (FWRall *f)
+{
+ f (x, x, x, x);
+ f (x, x, x, x + 1); // { dg-warning "\\\[-Wstringop-overflow" "pr94171" { xfail *-*-* } }
+ f (x, x, x + 1, x); // { dg-warning "\\\[-Wstringop-overflow" "pr94171" { xfail *-*-* } }
+ f (x, x + 1, x, x); // { dg-warning "\\\[-Wstringop-overflow" "pr94171" { xfail *-*-* } }
+ f (x + 1, x, x, x); // { dg-warning "\\\[-Wstringop-overflow" "pr94171" { xfail *-*-* } }
+}
