On Wed, Jan 19, 2011 at 02:48:26PM +0100, Denys Vlasenko wrote:
> On Wed, Jan 19, 2011 at 1:20 PM, Johannes Stezenbach <[email protected]> wrote:
> >> > static smallint detect_link_mii(void)
> >> > {
> >> > - struct ifreq ifreq;
> >> > - struct mii_ioctl_data *mii = (void *)&ifreq.ifr_data;
> >> > + union mii_ifreq mii_ifreq;
> >> > + union mii_ifreq *mii = (void *)&mii_ifreq.ifreq.ifr_data;
> >>
> >> Here you again assign void** to union mii_ifreq* -
> >> types still do not match.
> >
> > That's why I suggested to replace ifr_data with ifr_ifru,
> > which is less confusing but the result is the same.
>
> No, it should not be changed.
>
> We do not write for compiler. We write for *humans*
> to read our code.
The code must still be correct, though...
> ifr_data isn't called "data" for no reason. It means
> "a generic data field", and here we use it exactly
> for that purpose.
If ifr_data were a char array, OK. But it is a void*,
suggesting it contains a pointer to data stored somewhere else.
So it's completely misleading to use ifr_data.
FWIW, kernel code uses this (include/linux/mii.h):
static inline struct mii_ioctl_data *if_mii(struct ifreq *rq)
{
return (struct mii_ioctl_data *) &rq->ifr_ifru;
}
> >> This doesn't emit warnings probably because gcc
> >> takes a brute-force approach to handling unions
> >> by marking every pointer derived from any union member
> >> "potentially aliases anything". Future versions of gcc
> >> may become "more clever" again, and warnings will return.
> >
> > I disagree. C99 aliasing rules say two pointers of the
> > same type can alias. Consider the follwing:
> >
> > void *p = malloc(1000);
> > union mii_ifreq *m1 = p;
> > union mii_ifreq *m2 = p + 8;
> >
> > m1 and m2 can alias, and this does not change if you
> > change the line to
> >
> > union mii_ifreq *m2 = (void *)&m1->ifreq.ifr_ifru;
>
> >> > + union mii_ifreq mii_ifreq;
> >> > + union mii_ifreq *mii = (void *)&mii_ifreq.ifreq.ifr_data;
>
> In your patch, the pointers have an offset relative each other:
> even if compiler will assume they are aliased, aliasing means
> mii_ifreq.foo and mii->foo need to be considered as possibly the same.
> But in your case, mii_ifreq.foo doesn't map to mii->foo,
> it maps to mii->bar!
The actual values of the pointers are totally irrelevant.
Aliasing is relevant for code generation: if pointers m1 and m2 alias,
gcc must finish a store via m1 before generating a load via m2.
E.g "mii->reg_num = 1;" must be executed before doing the
network_ioctl() call. If you break the aliasing gcc may decide that
"mii->reg_num = 1;" can be pushed after the call or dropped completely.
> But I have a deeper problem here. I do not see it as a good decision
> to butcher a readable piece of code into a obfuscated mess *only*
> to shut up gcc. gcc has to provide means to do it in some cleaner way.
The gcc warning means: This is invalid code, it will break with
other compilers (including future gcc versions). The C99 standard
defines the aliasing rules precisely.
Anyway, the char buffer version I sent should be clearer.
Johannes
_______________________________________________
busybox mailing list
[email protected]
http://lists.busybox.net/mailman/listinfo/busybox
