V2 [PATCH] c-family: Update unaligned adress of packed member check

[H.J. Lu]

On Thu, Jan 17, 2019 at 7:36 AM Jakub Jelinek <jakub@redhat.com> wrote:
>
> On Wed, Jan 16, 2019 at 08:57:25PM -0800, H.J. Lu wrote:
> > Check unaligned pointer conversion and strip NOPS.
>
> > -check_address_of_packed_member (tree type, tree rhs)
> > +check_address_or_pointer_of_packed_member (tree type, tree rhs)
> >  {
> >    if (INDIRECT_REF_P (rhs))
> >      rhs = TREE_OPERAND (rhs, 0);
> > @@ -2726,6 +2728,36 @@ check_address_of_packed_member (tree type, tree rhs)
> >    if (TREE_CODE (rhs) == ADDR_EXPR)
> >      rhs = TREE_OPERAND (rhs, 0);
> >
> > +  if (!TYPE_P (type) || POINTER_TYPE_P (type))
> > +      type = TREE_TYPE (type);
>
> Bad formatting.  Plus, when would you pass around a non-type here?
> And, isn't type always a POINTER_TYPE_P here anyway?  If not, whether
> you use TREE_TYPE on it or not shouldn't depend on whether it is a pointer,
> but on some other conditions, because a field can have pointer type too,
> so if you come in through sometimes type being the address of the var and
> sometimes the type of its value, the bug is in allowing that.

Fixed.

> > +
> > +  if (TREE_CODE (rhs) == PARM_DECL || TREE_CODE (rhs) == VAR_DECL)
>
> VAR_P (rhs) instead of TREE_CODE (rhs) == VAR_DECL.  What about RESULT_DECL?

Fixed.  I couldn't get RESULT_DECL.  I checked CALL_EXPR instead
with a testcase.

> >  static void
> > -check_and_warn_address_of_packed_member (tree type, tree rhs)
> > +check_and_warn_address_or_pointer_of_packed_member (tree type, tree rhs)
> >  {
> > +  bool nop_p;
> > +
> > +  if (TREE_CODE (rhs) == NOP_EXPR)
>
> This should be probably if (CONVERT_EXPR_P (rhs)) or maybe just do
>   tree orig_rhs = rhs;
>   STRIP_NOPS (rhs);
>   nop_p = orig_rhs != rhs;
> ?

Fixed.

> I must say I don't fully understand the nop_p stuff, why you handle
> it differently if there were any nops vs. if there were none.
> And, e.g. if you have NOP_EXPR around COND_EXPR, that outer nop_p isn't
> taken into account.  So, again, what exactly do you want to achieve,
> why do you care if there are any conversions in between or not.
> Isn't all that matters what the innermost ADDR_EXPR is and what the
> outermost type is?
>
> >    if (TREE_CODE (rhs) != COND_EXPR)
>
> I think it would be more readable to do:
>   if (TREE_CODE (rhs) == COND_EXPR)
>     {
>       recurse;
>       recurse;
>       return;
>     }
> and handle the remaining code (longer) normally indented below that.

Fixed.

> Another thing is, the NOP_EXPRs/CONVERT_EXPRs, COMPOUND_EXPRs and
> COND_EXPRs can be arbitrarily nested, while you handle only a subset
> of those cases.  You could e.g. move the
>   while (TREE_CODE (rhs) == COMPOUND_EXPR)
>    rhs = TREE_OPERAND (rhs, 1);
>
> before the if (TREE_CODE (rhs) == COND_EXPR) check and stick another
> STRIP_NOPS in between.

Fixed.

> > @@ -2795,58 +2862,5 @@ warn_for_address_or_pointer_of_packed_member (bool convert_p, tree type,
> >    while (TREE_CODE (rhs) == COMPOUND_EXPR)
> >      rhs = TREE_OPERAND (rhs, 1);
>
> and then it would be pointless to do this here.

Fixed.

Here is the updated patch I am testing.

-- 
H.J.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-c-family-Update-unaligned-adress-of-packed-member-ch.patch
Type: text/x-patch
Size: 15373 bytes
Desc: not available
URL: <http://gcc.gnu.org/pipermail/gcc-patches/attachments/20190117/7f9000cf/attachment.bin>