[ovs-dev] [PATCH 1/2] vswitchd: Add miimon support.

Jesse Gross jesse at nicira.com
Thu Jan 13 14:02:49 UTC 2011


I'm using GCC 4.4.5 without any special options besides those
configured by the build system.  That's not the point though.  It's
not a false positive, it's a real issue so whether or not the compiler
warns about it is irrelevant.

This isn't a question of style, it's a bug.  I believe that there are
other occurrences of this in the code base and I also know that it's
pretty rare that this will actually result in a problem.  However,
that's no reason to not fix this.

On Wed, Jan 12, 2011 at 9:00 PM, Ethan Jackson <ethan at nicira.com> wrote:
> I can only reproduce the warnings with -Wstrict-aliasing=1, the most
> aggressive (and prone to false positives) level.  When I run on this
> level I find that we break strict aliasing rules in many places
> throughout the code base.  Are you configuring with this option?  What
> version of gcc are you using?
>
> I think we should either let this go, or change the compiler options
> to -Wstrict-aliasing=1 and document in the CodingStyle that we require
> strict aliasing to be respected in all cases, fix up the existing
> issues, and enforce it as a matter of style.
>
> Ethan
>
> On Wed, Jan 12, 2011 at 5:01 PM, Ethan Jackson <ethan at nicira.com> wrote:
>> Oh interesting.  Didn't cause warnings for me.  I'll submit a patch.
>>
>> Ethan
>>
>> On Wed, Jan 12, 2011 at 4:55 PM, Jesse Gross <jesse at nicira.com> wrote:
>>> On Wed, Jan 12, 2011 at 6:27 PM, Ethan Jackson <ethan at nicira.com> wrote:
>>>> diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c
>>>> index 97f9a64..8333bf5 100644
>>>> --- a/lib/netdev-linux.c
>>>> +++ b/lib/netdev-linux.c
>>> [...]
>>>> +static int
>>>> +netdev_linux_get_miimon(const struct netdev *netdev_, bool *miimon)
>>>> +{
>>>> +    int error;
>>>> +    struct ifreq ifr;
>>>> +    const char *name = netdev_get_name(netdev_);
>>>> +
>>>> +    *miimon = false;
>>>> +    memset(&ifr, 0, sizeof ifr);
>>>> +
>>>> +    error = netdev_linux_do_ioctl(name, &ifr, SIOCGMIIPHY, "SIOCGMIIPHY");
>>>> +    if (!error) {
>>>> +        struct mii_ioctl_data *data = (struct mii_ioctl_data *)&ifr.ifr_data;
>>>> +
>>>> +        data->reg_num = MII_BMSR;
>>>> +        error = netdev_linux_do_ioctl(name, &ifr, SIOCGMIIREG, "SIOCGMIIREG");
>>>> +
>>>> +        if (!error) {
>>>> +            *miimon = !!(data->val_out & BMSR_LSTATUS);
>>>
>>> This causes new warnings for me:
>>> lib/netdev-linux.c: In function ‘netdev_linux_get_miimon’:
>>> lib/netdev-linux.c:1030: warning: dereferencing pointer ‘data’ does
>>> break strict-aliasing rules
>>> lib/netdev-linux.c:1026: warning: dereferencing pointer ‘data’ does
>>> break strict-aliasing rules
>>> lib/netdev-linux.c:1023: note: initialized from here
>>> lib/netdev-linux.c:1045: warning: dereferencing pointer ‘eval’ does
>>> break strict-aliasing rules
>>> lib/netdev-linux.c:1036: note: initialized from here
>>>
>>> You're not allowed to have dereferenced data with two different types.
>>>  In this case, the compiler would be within its rights to optimize out
>>> the assignment to data->reg_num, since from its perspective nothing
>>> ever uses it.
>>>
>>
>




More information about the dev mailing list