On 1/23/19 4:22 PM, Jakub Jelinek wrote:
> On Tue, Jan 22, 2019 at 02:10:38PM +0000, Bernd Edlinger wrote:
>> --- gcc/c-family/c-warn.c (revision 268119)
>> +++ gcc/c-family/c-warn.c (working copy)
>> @@ -2796,6 +2796,10 @@ check_address_or_pointer_of_packed_membe
>> if (context)
>> break;
>> }
>> + if (TREE_CODE (TREE_TYPE (rhs)) == ARRAY_TYPE)
>> + rvalue = false;
>> + if (rvalue)
>> + return NULL_TREE;
>
> That looks like ARRAY_REF specific stuff, isn't it? We have various other
> handled components, does e.g. REALPART_EXPR/IMAGPART_EXPR need that?
> What about VIEW_CONVERT_EXPR? Or is that something you want to do for
> all of them? In any case, there should be testcases with _Complex and
> __real__/__imag__, address of that as well as value.
>
Okay, my guess it it will work with _Complex, but test cases would be
straight forward, and no reason not to have at least a few.
I have no idea what test case would be needed for VIEW_CONVERT_EXPR.
>> @@ -52,4 +54,6 @@ void foo (void)
>> i1 = t0.d; /* { dg-warning "may result in an unaligned
>> pointer value" } */
>> i1 = t1->d; /* { dg-warning "may result in an unaligned
>> pointer value" } */
>> i1 = t10[0].d; /* { dg-warning "may result in an unaligned
>> pointer value" } */
>> + i1 = (int*) &t10[0].e[0]; /* { dg-warning "may result in an unaligned
>> pointer value" } */
>> + i1 = (int*) t10[0].e; /* { dg-warning "may result in an unaligned
>> pointer value" } */
>
> Why not also i1 = &t10[0].e[0];
> and i1 = t10[0].e; tests?
>
I will add that.
> Unrelated to this patch, I'm worried e.g. about
> if (INDIRECT_REF_P (rhs))
> rhs = TREE_OPERAND (rhs, 0);
>
> at the start of the check_address_or_pointer_of_packed_member
> function, what if we have:
> i1 = *&t0.c;
> Do we want to warn when in the end we don't care if the address is unaligned
> or not, this is folded eventually into t0.c.
>
Porbably not. I tried your example, and find currently this is inconsistent
between C and C++, C folds *& away, before the warning, and C++ does not and
gets a warning. I feel like I want to fix that too.
As a side note, your *&t0.c has a VIEW_CONVERT_EXPR in C++ but not in C:
Breakpoint 1, check_address_or_pointer_of_packed_member (type=0x7ffff6eef9d8,
rhs=0x7ffff702d760) at ../../gcc-trunk/gcc/c-family/c-warn.c:2727
2727 bool rvalue = true;
(gdb) call debug(rhs)
*&VIEW_CONVERT_EXPR<struct t>(t0).c
... but it does not affect the warning.
> Plus as I said earlier to H.J., I think
> bool nop_p;
>
> while (TREE_CODE (rhs) == COMPOUND_EXPR)
> rhs = TREE_OPERAND (rhs, 1);
>
> tree orig_rhs = rhs;
> STRIP_NOPS (rhs);
> nop_p = orig_rhs != rhs;
> should be really:
> bool nop_p;
> tree orig_rhs = rhs;
> STRIP_NOPS (rhs);
> nop_p = orig_rhs != rhs;
>
> while (TREE_CODE (rhs) == COMPOUND_EXPR)
> rhs = TREE_OPERAND (rhs, 1);
>
> orig_rhs = rhs;
> STRIP_NOPS (rhs);
> nop_p |= orig_rhs != rhs;
>
> or similar, because if there are casts around COMPOUND_EXPR, it doesn't than
> look through those COMPOUND_EXPRs, and if e.g. in the COMPOUND_EXPR rhs
> there is then COND_EXPR or similar, it should handle that case.
>
Ah, yes. That was not on my radar.
So this works in C and C++:
i1 = (0, (int*)&t0.c);
But this is again inconsistent between C and C++:
i1 = (int*) (0, &t0.c);
C gets a warning, and C++ no warning.
I agree and see that consistency betwenn C and C++ as important goal.
So, I will try to fix that as well.
Bernd.
