something to warry about...

Johannes Stezenbach js at sig21.net
Wed Jan 19 12:20:20 UTC 2011


On Wed, Jan 19, 2011 at 12:43:23PM +0100, Denys Vlasenko wrote:
> On Tue, Jan 18, 2011 at 1:22 PM, Johannes Stezenbach <js at sig21.net> wrote:
> > +union mii_ifreq {
> > +       struct ifreq ifreq;
> > +       struct mii_ioctl_data mii;
> > +};
> 
> The layout of the union is not what we want.
> mii doesn't overlay ifreq.ifr_data.
> This doesn't hurm, because you aren't using
> the union's overlay match for access, but it's
> untypical and therefore confusing.

I agree that it looks confusing or even misleading.
It needs a comment added.

> >  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.

> 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;

The warning we get from the current code is caused because
the C99 aliasing rules say ifreq and mii do not alias,
while the compiler knows from the assignment

	struct mii_ioctl_data *mii = (void *)&ifreq.ifr_data;

that they do alias.


Johannes


More information about the busybox mailing list