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.
> +
> + 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?
> 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;
?
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.
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.
> @@ -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.
Jakub
