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