On Sun, Jan 20, 2019 at 9:10 AM Bernd Edlinger
<bernd.edlin...@hotmail.de> 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.
Unlike -Wcast-align=strict in
typedef char __attribute__ ((__aligned__(__BIGGEST_ALIGNMENT__))) c;
typedef struct __attribute__ ((__aligned__(__BIGGEST_ALIGNMENT__)))
{
char x;
} d;
char *x;
c *y;
d *z;
struct s { long long x; } *p;
struct t { double x; } *q;
which has a workaround by increasing source alignment, there is no workaround
for -Waddress-of-packed-member.
>
> Anyway, new patch version with fix for PR 88928 attached.
Thanks.
>
> Bootstrapped and reg-tested on x86_64-pc-linux-gnu.
> Is it OK for trunk?
>
--
H.J.
