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

[Jason Merrill]

On 12/18/18 4:12 PM, H.J. Lu wrote:
On Tue, Dec 18, 2018 at 12:36 PM Jason Merrill <ja...@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