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
