[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