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


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]

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

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

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

@@ -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.

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.

Jason


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