something to warry about...

Johannes Stezenbach js at sig21.net
Wed Jan 19 14:45:24 UTC 2011


On Wed, Jan 19, 2011 at 02:48:26PM +0100, Denys Vlasenko wrote:
> On Wed, Jan 19, 2011 at 1:20 PM, Johannes Stezenbach <js at sig21.net> 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


More information about the busybox mailing list