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

[Jason Merrill <jason@redhat.com>]

On 12/18/18 4:12 PM, H.J. Lu wrote:
> On Tue, Dec 18, 2018 at 12:36 PM Jason Merrill <jason@redhat.com> wrote:
>>
>> On 12/18/18 9:10 AM, H.J. Lu wrote:
>>> +  switch (TREE_CODE (rhs))
>>> +    {
>>> +    case ADDR_EXPR:
>>> +      base = TREE_OPERAND (rhs, 0);
>>> +      while (handled_component_p (base))
>>> +     {
>>> +       if (TREE_CODE (base) == COMPONENT_REF)
>>> +         break;
>>> +       base = TREE_OPERAND (base, 0);
>>> +     }
>>> +      if (TREE_CODE (base) != COMPONENT_REF)
>>> +     return NULL_TREE;
>>> +      object = TREE_OPERAND (base, 0);
>>> +      field = TREE_OPERAND (base, 1);
>>> +      break;
>>> +    case COMPONENT_REF:
>>> +      object = TREE_OPERAND (rhs, 0);
>>> +      field = TREE_OPERAND (rhs, 1);
>>> +      break;
>>> +    default:
>>> +      return NULL_TREE;
>>> +    }
>>> +
>>> +  tree context = check_alignment_of_packed_member (type, field);
>>> +  if (context)
>>> +    return context;
>>> +
>>> +  /* Check alignment of the object.  */
>>> +  while (TREE_CODE (object) == COMPONENT_REF)
>>> +    {
>>> +      field = TREE_OPERAND (object, 1);
>>> +      context = check_alignment_of_packed_member (type, field);
>>> +      if (context)
>>> +     return context;
>>> +      object = TREE_OPERAND (object, 0);
>>> +    }
>>> +
>>
>> You can see interleaved COMPONENT_REF and ARRAY_REF that this still
>> doesn't look like it will handle, something like
>>
>> struct A
>> {
>>     int i;
>> };
>>
>> struct B
>> {
>>     char c;
>>     __attribute ((packed)) A ar[4];
>> };
>>
>> B b;
>>
>> int *p = &b.ar[1].i;
>>
>> Rather than have a loop in the ADDR_EXPR case of the switch, you can
>> handle everything in the lower loop.  And not have a switch at all, just
>> strip any ADDR_EXPR before the loop.
> 
> I changed it to
> 
>   if (TREE_CODE (rhs) == ADDR_EXPR)
>      rhs = TREE_OPERAND (rhs, 0);
>    while (handled_component_p (rhs))
>      {
>        if (TREE_CODE (rhs) == COMPONENT_REF)
>          break;
>        rhs = TREE_OPERAND (rhs, 0);
>      }
> 
>    if (TREE_CODE (rhs) != COMPONENT_REF)
>      return NULL_TREE;
> 
>    object = TREE_OPERAND (rhs, 0);
>    field = TREE_OPERAND (rhs, 1);

That still doesn't warn about my testcase above.

> [hjl@gnu-cfl-1 pr51628-6]$ cat a.i
> struct A
> {
>     int i;
> } __attribute ((packed));
> 
> struct B
> {
>     char c;
>     struct A ar[4];
> };
> 
> struct B b;
> 
> int *p = &b.ar[1].i;

This testcase is importantly different because 'i' is packed, whereas in 
my testcase only the ar member of B is packed.

My suggestion was that this loop:

> +  /* Check alignment of the object.  */
> +  while (TREE_CODE (object) == COMPONENT_REF)
> +    {
> +      field = TREE_OPERAND (object, 1);
> +      context = check_alignment_of_packed_member (type, field);
> +      if (context)
> +	return context;
> +      object = TREE_OPERAND (object, 0);
> +    }

could loop over all handled_component_p, but only call 
check_alignment_of_packed_member for COMPONENT_REF.

> +  if (TREE_CODE (rhs) != COND_EXPR)
> +    {
> +      while (TREE_CODE (rhs) == COMPOUND_EXPR)
> +	rhs = TREE_OPERAND (rhs, 1);

What if you have a COND_EXPR inside a COMPOUND_EXPR?

Jason