This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


On Tue, Dec 18, 2018 at 2:14 PM Jason Merrill <jason@redhat.com> wrote:
>
> 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.

Thanks for the hint.  I changed it to

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

> > +  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?
>

It works for me:

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

int*
foo3 (struct A *p1, int **q1, int *q2, int *q3, struct A *p2)
{
  return q1 ? (*q1 = 1, &p1->i) : (q2 ? (*q1 = &p1->i, *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 \u2018foo3\u2019:
c.i:8:20: warning: assignment to \u2018int *\u2019 from
\u2018int\u2019 makes pointer from integer without a cast
[-Wint-conversion]
    8 |   return q1 ? (*q1 = 1, &p1->i) : (q2 ? (*q1 = &p1->i, *q2 =
2, &p2->i): q2);
      |                    ^
c.i:8:48: warning: taking address of packed member of \u2018struct
A\u2019 may result in an unaligned pointer value
[-Waddress-of-packed-member]
    8 |   return q1 ? (*q1 = 1, &p1->i) : (q2 ? (*q1 = &p1->i, *q2 =
2, &p2->i): q2);
      |                                                ^~~~~~
c.i:8:25: warning: taking address of packed member of \u2018struct
A\u2019 may result in an unaligned pointer value
[-Waddress-of-packed-member]
    8 |   return q1 ? (*q1 = 1, &p1->i) : (q2 ? (*q1 = &p1->i, *q2 =
2, &p2->i): q2);
      |                         ^~~~~~
c.i:8:65: warning: taking address of packed member of \u2018struct
A\u2019 may result in an unaligned pointer value
[-Waddress-of-packed-member]
    8 |   return q1 ? (*q1 = 1, &p1->i) : (q2 ? (*q1 = &p1->i, *q2 =
2, &p2->i): q2);
      |                                                                 ^~~~~~
[hjl@gnu-cfl-1 pr51628-5]$

If it isn't what you meant, can you give me a testcase?

Thanks.

-- 
H.J.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]