[PATCH] avoid modifying type in attribute access handler [PR94098]

[Martin Sebor]

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