Re: [PATCH] avoid modifying type in attribute access handler [PR94098]
On 3/26/20 6:52 PM, Jason Merrill wrote: On 3/26/20 4: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? Both, yes. We should give the decl(s) a new type rather than modify their existing types. 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? Those also seem unsafe. 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.) It's a general correctness issue, the C++ front end just catches the issues better. If you have two declarations with the same type, and then you give one an attribute, it shouldn't give that same attribute to the other declaration that shared the type. 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? We can't modify the existing attribute chain; if we need to remove an attribute from the middle, it must be by copying all the list nodes before the attribute. 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). Sounds good. Thanks for the answers. I'll see about implementing some checking of these restrictions in stage 1. Martin 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
Re: [PATCH] avoid modifying type in attribute access handler [PR94098]
On 3/26/20 4: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? Both, yes. We should give the decl(s) a new type rather than modify their existing types. 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? Those also seem unsafe. 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.) It's a general correctness issue, the C++ front end just catches the issues better. If you have two declarations with the same type, and then you give one an attribute, it shouldn't give that same attribute to the other declaration that shared the type. 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? We can't modify the existing attribute chain; if we need to remove an attribute from the middle, it must be by copying all the list nodes before the attribute. 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). Sounds good. 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
Re: [PATCH] avoid modifying type in attribute access handler [PR94098]
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
Re: [PATCH] avoid modifying type in attribute access handler [PR94098]
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
Re: [PATCH] avoid modifying type in attribute access handler [PR94098]
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. Jason
Re: [PATCH] avoid modifying type in attribute access handler [PR94098]
Thanks for the suggestion to use type_cache_hasher::equal. I'll
revisit the idea of adding an assertion to decl_attribute in
stage 1 and will look into using it there.
Until then, unless you or anyone else has any concerns with
the original patch I will plan to commit tomorrow:
https://gcc.gnu.org/pipermail/gcc-patches/2020-March/542098.html
Martin
On 3/18/20 10:10 PM, Jason Merrill wrote:
On 3/18/20 6:04 PM, Martin Sebor wrote:
On 3/16/20 3:10 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.
I'm puzzled that the ICE only triggers in the C++ front-end and not
also in C. The code that issues the internal error is in comptypes()
in cp/typeck.c and has this comment:
/* The two types are structurally equivalent, but their
canonical types were different. This is a failure of the
canonical type propagation code.*/
internal_error
("canonical types differ for identical types %qT and %qT",
t1, t2);
What is "the canonical type propagation code" it refers to?
Generally, code that makes sure that TYPE_CANONICAL equality is
equivalent to type identity, not any one specific place.
Is it valid to modify the type in an attribute handler
Only if (flags & ATTR_FLAG_TYPE_IN_PLACE).
If not, and if modifying a type in place is not valid I'd expect
decl_attributes to enforce it. I looked for other attribute handlers
to see if any also modify the type argument in place (by adding or
removing attributes) but couldn't really find any. So if it is
invalid I'd like to add such an assertion (probably for GCC 11) but
before I do I want to make sure I'm not missing something.
Generally messing with _ATTRIBUTES happens in decl_attributes:
changing it directly if it's a DECL or ATTR_FLAG_TYPE_IN_PLACE,
otherwise using build_type_attribute_variant. If you need to do it
in your handler, you should follow the same pattern.
That's the conclusion I came to as well, but thanks for confirming
it. With the patch I don't need to make the change but since it's
not obvious that it's a no-no and since it's apparently only detected
under very special conditions I'm wondering is if it's possible to
detect it more reliably than only in C++ comptypes. The trouble is
that I don't exactly what is allowed and what isn't and what to look
for to tell if the handler did something that's not allowed.
The C++ ICE triggered because the redeclared function's type is
considered the same as the original (structural_comptypes()
returns true) but the declarations' canonical types are different.
structural_comptypes() is C++-specific and I don't know what
alternative to call in the middle-end to compare the types and
get the equivalent result.
I think type_cache_hasher::equal is the closest, but it looks like it
doesn't check everything.
PS As a data point, I found just two attribute handlers in
c-attribs.c that modify a type in place: one for attribute const
and the other noreturn. They both do it for function pointers
to set the 'const' and 'noreturn' bits on the pointed to types,
and by calling build_type_variant.
Hmm, yes, that does sound like a bug.
Jason
Re: [PATCH] avoid modifying type in attribute access handler [PR94098]
On 3/18/20 6:04 PM, Martin Sebor wrote:
On 3/16/20 3:10 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.
I'm puzzled that the ICE only triggers in the C++ front-end and not
also in C. The code that issues the internal error is in comptypes()
in cp/typeck.c and has this comment:
/* The two types are structurally equivalent, but their
canonical types were different. This is a failure of the
canonical type propagation code.*/
internal_error
("canonical types differ for identical types %qT and %qT",
t1, t2);
What is "the canonical type propagation code" it refers to?
Generally, code that makes sure that TYPE_CANONICAL equality is
equivalent to type identity, not any one specific place.
Is it valid to modify the type in an attribute handler
Only if (flags & ATTR_FLAG_TYPE_IN_PLACE).
If not, and if modifying a type in place is not valid I'd expect
decl_attributes to enforce it. I looked for other attribute handlers
to see if any also modify the type argument in place (by adding or
removing attributes) but couldn't really find any. So if it is
invalid I'd like to add such an assertion (probably for GCC 11) but
before I do I want to make sure I'm not missing something.
Generally messing with _ATTRIBUTES happens in decl_attributes:
changing it directly if it's a DECL or ATTR_FLAG_TYPE_IN_PLACE,
otherwise using build_type_attribute_variant. If you need to do it in
your handler, you should follow the same pattern.
That's the conclusion I came to as well, but thanks for confirming
it. With the patch I don't need to make the change but since it's
not obvious that it's a no-no and since it's apparently only detected
under very special conditions I'm wondering is if it's possible to
detect it more reliably than only in C++ comptypes. The trouble is
that I don't exactly what is allowed and what isn't and what to look
for to tell if the handler did something that's not allowed.
The C++ ICE triggered because the redeclared function's type is
considered the same as the original (structural_comptypes()
returns true) but the declarations' canonical types are different.
structural_comptypes() is C++-specific and I don't know what
alternative to call in the middle-end to compare the types and
get the equivalent result.
I think type_cache_hasher::equal is the closest, but it looks like it
doesn't check everything.
PS As a data point, I found just two attribute handlers in
c-attribs.c that modify a type in place: one for attribute const
and the other noreturn. They both do it for function pointers
to set the 'const' and 'noreturn' bits on the pointed to types,
and by calling build_type_variant.
Hmm, yes, that does sound like a bug.
Jason
Re: [PATCH] avoid modifying type in attribute access handler [PR94098]
On 3/16/20 3:10 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.
I'm puzzled that the ICE only triggers in the C++ front-end and not
also in C. The code that issues the internal error is in comptypes()
in cp/typeck.c and has this comment:
/* The two types are structurally equivalent, but their
canonical types were different. This is a failure of the
canonical type propagation code.*/
internal_error
("canonical types differ for identical types %qT and %qT",
t1, t2);
What is "the canonical type propagation code" it refers to?
Generally, code that makes sure that TYPE_CANONICAL equality is
equivalent to type identity, not any one specific place.
Is it valid to modify the type in an attribute handler
Only if (flags & ATTR_FLAG_TYPE_IN_PLACE).
If not, and if modifying a type in place is not valid I'd expect
decl_attributes to enforce it. I looked for other attribute handlers
to see if any also modify the type argument in place (by adding or
removing attributes) but couldn't really find any. So if it is
invalid I'd like to add such an assertion (probably for GCC 11) but
before I do I want to make sure I'm not missing something.
Generally messing with _ATTRIBUTES happens in decl_attributes: changing
it directly if it's a DECL or ATTR_FLAG_TYPE_IN_PLACE, otherwise using
build_type_attribute_variant. If you need to do it in your handler, you
should follow the same pattern.
That's the conclusion I came to as well, but thanks for confirming
it. With the patch I don't need to make the change but since it's
not obvious that it's a no-no and since it's apparently only detected
under very special conditions I'm wondering is if it's possible to
detect it more reliably than only in C++ comptypes. The trouble is
that I don't exactly what is allowed and what isn't and what to look
for to tell if the handler did something that's not allowed.
The C++ ICE triggered because the redeclared function's type is
considered the same as the original (structural_comptypes()
returns true) but the declarations' canonical types are different.
structural_comptypes() is C++-specific and I don't know what
alternative to call in the middle-end to compare the types and
get the equivalent result.
Martin
PS As a data point, I found just two attribute handlers in
c-attribs.c that modify a type in place: one for attribute const
and the other noreturn. They both do it for function pointers
to set the 'const' and 'noreturn' bits on the pointed to types,
and by calling build_type_variant.
Re: [PATCH] avoid modifying type in attribute access handler [PR94098]
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.
I'm puzzled that the ICE only triggers in the C++ front-end and not
also in C. The code that issues the internal error is in comptypes()
in cp/typeck.c and has this comment:
/* The two types are structurally equivalent, but their
canonical types were different. This is a failure of the
canonical type propagation code.*/
internal_error
("canonical types differ for identical types %qT and %qT",
t1, t2);
What is "the canonical type propagation code" it refers to?
Generally, code that makes sure that TYPE_CANONICAL equality is
equivalent to type identity, not any one specific place.
Is it valid to modify the type in an attribute handler
Only if (flags & ATTR_FLAG_TYPE_IN_PLACE).
If not, and if modifying a type in place is not valid I'd expect
decl_attributes to enforce it. I looked for other attribute handlers
to see if any also modify the type argument in place (by adding or
removing attributes) but couldn't really find any. So if it is
invalid I'd like to add such an assertion (probably for GCC 11) but
before I do I want to make sure I'm not missing something.
Generally messing with _ATTRIBUTES happens in decl_attributes: changing
it directly if it's a DECL or ATTR_FLAG_TYPE_IN_PLACE, otherwise using
build_type_attribute_variant. If you need to do it in your handler, you
should follow the same pattern.
Jason
