Re: V6 [PATCH] C/C++: Add -Waddress-of-packed-member

[Jason Merrill <jason@redhat.com>]

On 12/17/18 7:42 AM, H.J. Lu wrote:
> On Mon, Dec 17, 2018 at 1:39 AM Richard Biener
> <richard.guenther@gmail.com> wrote:
>>
>> On Fri, Dec 14, 2018 at 11:48 PM H.J. Lu <hjl.tools@gmail.com> wrote:
>>>
>>> On Fri, Dec 14, 2018 at 2:10 PM Jason Merrill <jason@redhat.com> wrote:
>>>>
>>>> On 12/13/18 6:56 PM, H.J. Lu wrote:
>>>>> On Thu, Dec 13, 2018 at 12:50 PM Jason Merrill <jason@redhat.com> wrote:
>>>>>>
>>>>>> On 9/25/18 11:46 AM, H.J. Lu wrote:
>>>>>>> On Fri, Aug 31, 2018 at 2:04 PM, Jason Merrill <jason@redhat.com> wrote:
>>>>>>>> On 07/23/2018 05:24 PM, H.J. Lu wrote:
>>>>>>>>>
>>>>>>>>> On Mon, Jun 18, 2018 at 12:26 PM, Joseph Myers <joseph@codesourcery.com>
>>>>>>>>> wrote:
>>>>>>>>>>
>>>>>>>>>> On Mon, 18 Jun 2018, Jason Merrill wrote:
>>>>>>>>>>
>>>>>>>>>>> On Mon, Jun 18, 2018 at 11:59 AM, Joseph Myers <joseph@codesourcery.com>
>>>>>>>>>>> wrote:
>>>>>>>>>>>>
>>>>>>>>>>>> On Mon, 18 Jun 2018, Jason Merrill wrote:
>>>>>>>>>>>>
>>>>>>>>>>>>>> +  if (TREE_CODE (rhs) == COND_EXPR)
>>>>>>>>>>>>>> +    {
>>>>>>>>>>>>>> +      /* Check the THEN path first.  */
>>>>>>>>>>>>>> +      tree op1 = TREE_OPERAND (rhs, 1);
>>>>>>>>>>>>>> +      context = check_address_of_packed_member (type, op1);
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> This should handle the GNU extension of re-using operand 0 if operand
>>>>>>>>>>>>> 1 is omitted.
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> Doesn't that just use a SAVE_EXPR?
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> Hmm, I suppose it does, but many places in the compiler seem to expect
>>>>>>>>>>> that it produces a COND_EXPR with TREE_OPERAND 1 as NULL_TREE.
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Maybe that's used somewhere inside the C++ front end.  For C a SAVE_EXPR
>>>>>>>>>> is produced directly.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Here is the updated patch.  Changes from the last one:
>>>>>>>>>
>>>>>>>>> 1. Handle COMPOUND_EXPR.
>>>>>>>>> 2. Fixed typos in comments.
>>>>>>>>> 3. Combined warn_for_pointer_of_packed_member and
>>>>>>>>> warn_for_address_of_packed_member into
>>>>>>>>> warn_for_address_or_pointer_of_packed_member.
>>>>>>>>
>>>>>>>>
>>>>>>>>> c.i:4:33: warning: converting a packed ‘struct C *’ pointer increases the
>>>>>>>>> alignment of ‘long int *’ pointer from 1 to 8 [-Waddress-of-packed-member]
>>>>>>>>
>>>>>>>>
>>>>>>>> I think this would read better as
>>>>>>>>
>>>>>>>> c.i:4:33: warning: converting a packed ‘struct C *’ pointer (alignment 1) to
>>>>>>>> ‘long int *’ (alignment 8) may result in an unaligned pointer value
>>>>>>>> [-Waddress-of-packed-member]
>>>>>>>
>>>>>>> Fixed.
>>>>>>>
>>>>>>>>> +      while (TREE_CODE (base) == ARRAY_REF)
>>>>>>>>> +       base = TREE_OPERAND (base, 0);
>>>>>>>>> +      if (TREE_CODE (base) != COMPONENT_REF)
>>>>>>>>> +       return NULL_TREE;
>>>>>>>>
>>>>>>>>
>>>>>>>> Are you deliberately not handling the other handled_component_p cases? If
>>>>>>>> so, there should be a comment.
>>>>>>>
>>>>>>> I changed it to
>>>>>>>
>>>>>>>         while (handled_component_p (base))
>>>>>>>            {
>>>>>>>              enum tree_code code = TREE_CODE (base);
>>>>>>>              if (code == COMPONENT_REF)
>>>>>>>                break;
>>>>>>>              switch (code)
>>>>>>>                {
>>>>>>>                case ARRAY_REF:
>>>>>>>                  base = TREE_OPERAND (base, 0);
>>>>>>>                  break;
>>>>>>>                default:
>>>>>>>                  /* FIXME: Can it ever happen?  */
>>>>>>>                  gcc_unreachable ();
>>>>>>>                  break;
>>>>>>>                }
>>>>>>>            }
>>>>>>>
>>>>>>> Is there a testcase to trigger this ICE? I couldn't find one.
>>>>>>
>>>>>> You can take the address of an element of complex:
>>>>>>
>>>>>>      __complex int i;
>>>>>>      int *p = &__real(i);
>>>>>>
>>>>>> You may get VIEW_CONVERT_EXPR with location wrappers.
>>>>>
>>>>> Fixed.  I replaced gcc_unreachable with return NULL_TREE;
>>>>
>>>> Then we're back to my earlier question: are you deliberately not
>>>> handling the other cases?  Why not look through them as well?  What if
>>>> e.g. the operand of __real is a packed field?
>>>>
>>>
>>> Here is the updated patch with
>>>
>>> diff --git a/gcc/c-family/c-warn.c b/gcc/c-family/c-warn.c
>>> index 615134cfdac..f105742598e 100644
>>> --- a/gcc/c-family/c-warn.c
>>> +++ b/gcc/c-family/c-warn.c
>>> @@ -2669,6 +2669,9 @@ check_address_of_packed_member (tree type, tree rhs)
>>>      switch (code)
>>>        {
>>>        case ARRAY_REF:
>>> +     case REALPART_EXPR:
>>> +     case IMAGPART_EXPR:
>>> +     case VIEW_CONVERT_EXPR:
>>>          base = TREE_OPERAND (base, 0);
>>>          break;
>>>        default:
>>
>> don't we have handled_component_p () for this?  (you're still
>> missing BIT_FIELD_REF which might be used for vector
>> element accesses)
>>
> 
> Do you have a testcase?

Is there a reason you only want to handle some component references and 
not others?  If not, checking handled_component_p is simpler and more 
future proof than enumerating specific codes.

Jason