On 1/20/19 10:10 AM, Bernd Edlinger wrote:
> On 1/20/19 4:40 PM, H.J. Lu wrote:
>> On Sun, Jan 20, 2019 at 5:29 AM Bernd Edlinger
>> <bernd.edlin...@hotmail.de> wrote:
>>> Hi,
>>>
>>>
>>> I tried to build linux yesterday, and became aware that there are a few
>>> false-positive warnings with -Waddress-of-packed-member:
>>>
>>> struct t {
>>> char a;
>>> int b;
>>> int *c;
>>> int d[10];
>>> } __attribute__((packed));
>>>
>>> struct t t0;
>>> struct t *t1;
>>> struct t **t2;
>>>
>>> t2 = &t1;
>>> i1 = t0.c;
>>>
>>> I fixed them quickly with the attached patch, and added a new test case,
>>> which also revealed a few missing warnings:
>>>
>>> struct t t100[10][10];
>>> struct t (*baz())[10];
>>>
>>> t2 = (struct t**) t100;
>>> t2 = (struct t**) baz();
>>>
>>>
>>> Well I remembered that Joseph wanted me to use min_align_of_type instead
>>> of TYPE_ALIGN in the -Wcast-align warning, so I changed
>>> -Waddress-of-packed-member
>>> to do that as well.
>>>
>>> Since I was not sure if the test coverage is good enough, I added a few more
>>> test cases, which just make sure the existing warning works properly.
>> You should also fix
>>
>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=88928
>>
> Yes, thanks. I added a check for NULL from TYPE_STUB_DECL here:
>
> + tree decl = TYPE_STUB_DECL (rhstype);
> + if (decl)
> + inform (DECL_SOURCE_LOCATION (decl), "defined here");
>
> and the test case from the PR.
>
>>> I am however not sure of a warning that is as noisy as this one, should be
>>> default-enabled, because it might interfere with configure tests. That
>>> might
>> These codes can lead unaligned access which may be very hard to track
>> down or fix since the codes in question may be in a library. I think it is
>> a
>> very useful warning. Besides, these noisy warnings also reveal
>> implementation
>> issues in GCC, which, otherwise, may not be known for a long time.
>>
> No doubt that is a useful warning there is no question about that.
>
> Are you concerned about production code that gets built w/o at least -Wall ?
>
> I am however not sure, why it is limited to packed structures.
>
>>> be better to enable this warning, in -Wall or -Wextra, and, well maybe
>>> enable -Wcast-align=strict also at the same warning level, since it is
>>> currently
>>> not enabled at all, unless explicitly requested, what do you think?
>>>
> I just see, your warning as being similar in spirit as -Wcast-align, since
> if type casts are involved, you don't need packed structures to get unaligned
> pointers. So what I wonder now is, if it is a bit inconsistent to have
> -Wcast-align=strict not being enabled at least with -Wextra, while
> -Waddress-of-packed-member being default enabled. I am just unsure if one day
> -Wcast-align should be enabled by -Wall/-Wextra or per default as well, or
> being
> superseded by -Waddress-of-packed-member. I mean, except for the limitation
> on requiring the packed keyword on structures, -Waddress-of-packed-member is
> now
> a superset of -Wcast-align=strict, you know.
>
>
> Anyway, new patch version with fix for PR 88928 attached.
>
>
> Bootstrapped and reg-tested on x86_64-pc-linux-gnu.
> Is it OK for trunk?
>
>
> Thanks
> Bernd.
>
>
> patch-fix-packed-warning-v2.diff
>
> 2019-01-20 Bernd Edlinger <bernd.edlin...@hotmail.de>
>
> PR c/88928
> * c-warn.c (check_alignment_of_packed_member): Add a boolean parameter
> for rvalue context. Handle rvalues correctly. Use min_align_of_type
> instead of TYPE_ALIGN.
> (check_address_or_pointer_of_packed_member): Handle rvalues coorrectly.
> Use min_align_of_type instead of TYPE_ALIGN_UNIT. Check for NULL
> pointer from TYPE_STUB_DECL.
>
> testsuite:
> 2019-01-20 Bernd Edlinger <bernd.edlin...@hotmail.de>
>
> PR c/88928
> * c-c++-common/Waddress-of-packed-member-1.c: New test case.
> * gcc.dg/pr88928.c: New test case.
OK. Thanks for digging into this.
jeff
