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: V4 [PATCH] C/C++: Add -Waddress-of-packed-member


On 9/25/18 11:46 AM, H.J. Lu wrote:
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.

You can take the address of an element of complex:

  __complex int i;
  int *p = &__real(i);

You may get VIEW_CONVERT_EXPR with location wrappers.

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

In that testcase, 'i' is the top COMPONENT_EXPR. What I was talking about would be more like

 struct A { int i; };
 struct B { struct A a; };
 struct C { struct B b __attribute__ ((packed)); };

 extern struct C *p;

 int* g8 (void) { return &p->b.a.i; }

Jason


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