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

[H.J. Lu]

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);

[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;
[hjl@gnu-cfl-1 pr51628-6]$ make a.s
/export/build/gnu/tools-build/gcc-debug/build-x86_64-linux/gcc/xgcc
-B/export/build/gnu/tools-build/gcc-debug/build-x86_64-linux/gcc/ -O2
-S a.i
a.i:14:10: warning: taking address of packed member of ‘struct A’ may
result in an unaligned pointer value [-Waddress-of-packed-member]
   14 | int *p = &b.ar[1].i;
      |          ^~~~~~~~~~
[hjl@gnu-cfl-1 pr51628-6]$

> > +check_and_warn_address_of_packed_member (tree type, tree rhs)
> > +{
> > +  if (TREE_CODE (rhs) != COND_EXPR)
> > +    {
> > +      tree context = check_address_of_packed_member (type, rhs);
> > +      if (context)
> > +     {
> > +       location_t loc = EXPR_LOC_OR_LOC (rhs, input_location);
> > +       warning_at (loc, OPT_Waddress_of_packed_member,
> > +                   "taking address of packed member of %qT may result "
> > +                   "in an unaligned pointer value",
> > +                   context);
> > +     }
> > +      return;
> > +    }
> > +
> > +  /* Check the THEN path.  */
> > +  check_and_warn_address_of_packed_member (type, TREE_OPERAND (rhs, 1));
> > +
> > +  /* Check the ELSE path.  */
> > +  check_and_warn_address_of_packed_member (type, TREE_OPERAND (rhs, 2));
> > +}
>
> You probably also want to handle COMPOUND_EXPR.
>

Done.

[hjl@gnu-cfl-1 pr51628-5]$ cat c.i
struct A {
  int i;
} __attribute__ ((packed));

int*
foo3 (struct A *p1, int *q1, int *q2, struct A *p2)
{
  return q1 ? (*q1 = 1, &p1->i) : (q2 ? (*q2 = 2, &p2->i): q2);
}
[hjl@gnu-cfl-1 pr51628-5]$
/export/build/gnu/tools-build/gcc-debug/build-x86_64-linux/gcc/xgcc
-B/export/build/gnu/tools-build/gcc-debug/build-x86_64-linux/gcc/ -O2
-S c.i
c.i: In function ‘foo3’:
c.i:8:25: warning: taking address of packed member of ‘struct A’ may
result in an unaligned pointer value [-Waddress-of-packed-member]
    8 |   return q1 ? (*q1 = 1, &p1->i) : (q2 ? (*q2 = 2, &p2->i): q2);
      |                         ^~~~~~
c.i:8:51: warning: taking address of packed member of ‘struct A’ may
result in an unaligned pointer value [-Waddress-of-packed-member]
    8 |   return q1 ? (*q1 = 1, &p1->i) : (q2 ? (*q2 = 2, &p2->i): q2);
      |                                                   ^~~~~~
[hjl@gnu-cfl-1 pr51628-5]$

Here is the updated patch.  OK for trunk?

Thanks.


-- 
H.J.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-C-C-Add-Waddress-of-packed-member.patch
Type: text/x-patch
Size: 51900 bytes
Desc: not available
URL: <http://gcc.gnu.org/pipermail/gcc-patches/attachments/20181218/d1ee6d27/attachment.bin>