On Fri, Aug 31, 2018 at 2:04 PM, Jason Merrill <jason@redhat.com> wrote:
> On 07/23/2018 05:24 PM, H.J. Lu wrote:
>>
>> On Mon, Jun 18, 2018 at 12:26 PM, Joseph Myers <joseph@codesourcery.com>
>> wrote:
>>>
>>> On Mon, 18 Jun 2018, Jason Merrill wrote:
>>>
>>>> On Mon, Jun 18, 2018 at 11:59 AM, Joseph Myers <joseph@codesourcery.com>
>>>> wrote:
>>>>>
>>>>> On Mon, 18 Jun 2018, Jason Merrill wrote:
>>>>>
>>>>>>> +  if (TREE_CODE (rhs) == COND_EXPR)
>>>>>>> +    {
>>>>>>> +      /* Check the THEN path first.  */
>>>>>>> +      tree op1 = TREE_OPERAND (rhs, 1);
>>>>>>> +      context = check_address_of_packed_member (type, op1);
>>>>>>
>>>>>>
>>>>>> This should handle the GNU extension of re-using operand 0 if operand
>>>>>> 1 is omitted.
>>>>>
>>>>>
>>>>> Doesn't that just use a SAVE_EXPR?
>>>>
>>>>
>>>> Hmm, I suppose it does, but many places in the compiler seem to expect
>>>> that it produces a COND_EXPR with TREE_OPERAND 1 as NULL_TREE.
>>>
>>>
>>> Maybe that's used somewhere inside the C++ front end.  For C a SAVE_EXPR
>>> is produced directly.
>>
>>
>> Here is the updated patch.  Changes from the last one:
>>
>> 1. Handle COMPOUND_EXPR.
>> 2. Fixed typos in comments.
>> 3. Combined warn_for_pointer_of_packed_member and
>> warn_for_address_of_packed_member into
>> warn_for_address_or_pointer_of_packed_member.
>
>
>> c.i:4:33: warning: converting a packed ‘struct C *’ pointer increases the
>> alignment of ‘long int *’ pointer from 1 to 8 [-Waddress-of-packed-member]
>
>
> I think this would read better as
>
> c.i:4:33: warning: converting a packed ‘struct C *’ pointer (alignment 1) to
> ‘long int *’ (alignment 8) may result in an unaligned pointer value
> [-Waddress-of-packed-member]

Fixed.

>> +      while (TREE_CODE (base) == ARRAY_REF)
>> +       base = TREE_OPERAND (base, 0);
>> +      if (TREE_CODE (base) != COMPONENT_REF)
>> +       return NULL_TREE;
>
>
> Are you deliberately not handling the other handled_component_p cases? If
> so, there should be a comment.

I changed it to

     while (handled_component_p (base))
        {
          enum tree_code code = TREE_CODE (base);
          if (code == COMPONENT_REF)
            break;
          switch (code)
            {
            case ARRAY_REF:
              base = TREE_OPERAND (base, 0);
              break;
            default:
              /* FIXME: Can it ever happen?  */
              gcc_unreachable ();
              break;
            }
        }

Is there a testcase to trigger this ICE? I couldn't find one.

>> +  /* Check alignment of the object.  */
>> +  if (TREE_CODE (object) == COMPONENT_REF)
>> +    {
>> +      field = TREE_OPERAND (object, 1);
>> +      if (TREE_CODE (field) == FIELD_DECL && DECL_PACKED (field))
>> +       {
>> +         type_align = TYPE_ALIGN (type);
>> +         context = DECL_CONTEXT (field);
>> +         record_align = TYPE_ALIGN (context);
>> +         if ((record_align % type_align) != 0)
>> +           return context;
>> +       }
>> +    }
>
>
> Why doesn't this recurse?  What if you have a packed field three
> COMPONENT_REFs down?

My patch works on
[hjl@gnu-cfl-1 pr51628-4]$ cat x.i
struct A { int i; } __attribute__ ((packed));
struct B { struct A a; };
struct C { struct B b; };

extern struct C *p;

int* g8 (void) { return &p->b.a.i; }
[hjl@gnu-cfl-1 pr51628-4]$ make x.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 x.i
x.i: In function ‘g8’:
x.i:7:25: warning: taking address of packed member of ‘struct A’ may
result in an unaligned pointer value [-Waddress-of-packed-member]
7 | int* g8 (void) { return &p->b.a.i; }
  |                         ^~~~~~~~~
[hjl@gnu-cfl-1 pr51628-4]$

If it isn't what you had in mind, can you give me a testcase?

>> +  if (TREE_CODE (rhs) == COND_EXPR)
>> +    {
>> +      /* Check the THEN path first.  */
>> +      tree op1 = TREE_OPERAND (rhs, 1);
>> +      context = check_address_of_packed_member (type, op1);
>> +      if (context)
>> +       rhs = op1;
>> +      else
>> +       {
>> +         /* Check the ELSE path.  */
>> +         rhs = TREE_OPERAND (rhs, 2);
>> +         context = check_address_of_packed_member (type, rhs);
>> +       }
>> +    }
>
>
> Likewise, what if you have more levels of COND_EXPR?  Or COMPOUND_EXPR
> within COND_EXPR?

Fixed, now I got

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

int*
foo3 (struct A *p1, int *q1, int *q2, struct A *p2)
{
  return (q1
          ? &p1->i
          : (q2 ? &p2->i : q2));
}
[hjl@gnu-cfl-1 pr51628-5]$ make z.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 z.i
z.i: In function ‘foo3’:
z.i:9:13: warning: taking address of packed member of ‘struct A’ may
result in an unaligned pointer value [-Waddress-of-packed-member]
9 |           ? &p1->i
  |             ^~~~~~
z.i:10:19: warning: taking address of packed member of ‘struct A’ may
result in an unaligned pointer value [-Waddress-of-packed-member]
10 |           : (q2 ? &p2->i : q2));
   |                   ^~~~~~
[hjl@gnu-cfl-1 pr51628-5]$

>> @@ -7470,6 +7470,9 @@ convert_for_arg_passing (tree type, tree val,
>> tsubst_flags_t complain)
>> +  warn_for_address_or_pointer_of_packed_member (true, type, val);
>
>
>> @@ -8914,6 +8914,8 @@ convert_for_assignment (tree type, tree rhs,
>> +  warn_for_address_or_pointer_of_packed_member (true, type, rhs);
>
>
> Why would address_p be true in these calls?  It seems that you are warning
> at the point of assignment but looking for the warning about taking the
> address rather than the one about assignment.

It happens only with C for incompatible pointer conversion:

[hjl@gnu-cfl-1 pr51628-2]$ cat c.i
struct B { int i; };
struct C { struct B b; } __attribute__ ((packed));

long* g8 (struct C *p) { return p; }
[hjl@gnu-cfl-1 pr51628-2]$ make c.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 c.i
c.i: In function ‘g8’:
c.i:4:33: warning: returning ‘struct C *’ from a function with
incompatible return type ‘long int *’ [-Wincompatible-pointer-types]
4 | long* g8 (struct C *p) { return p; }
  |                                 ^
c.i:4:18: warning: converting a packed ‘struct C *’ pointer (alignment
1) to ‘long int *’ (alignment 8) may may result in an unaligned
pointer value [-Waddress-of-packed-member]
4 | long* g8 (struct C *p) { return p; }
  |                  ^
c.i:2:8: note: defined here
2 | struct C { struct B b; } __attribute__ ((packed));
  |        ^
[hjl@gnu-cfl-1 pr51628-2]$

address_p is false in this case and rhs is PARM_DECL, VAR_DECL or
NOP_EXPR.  This comes from convert_for_assignment in c/c-typeck.c.

For other compatible pointer assignment, address_p is true and rhs is
ADDR_EXPR, PARM_DECL, VAR_DECL or NOP_EXPR.   Check
for  ADDR_EXPR won't work.

address_p isn't an appropriate parameter name.  I changed it to convert_p
to indicate that it is an incompatible pointer type conversion.

> If you want to warn about taking the address, shouldn't that happen under
> cp_build_addr_expr?  Alternately, drop the address_p parameter and choose
> your path inside warn_for_*_packed_member based on whether rhs is an
> ADDR_EXPR there rather than in the caller.
>

Here is the updated patch.  OK for trunk?

Thanks.


-- 
H.J.