Fix remaining compiler warnings in sloffs.c

Segher Boessenkool <segher@kernel.crashing.org> writes:

> On Mon, Aug 08, 2016 at 02:45:35PM +0530, Nikunj A Dadhania wrote:
>> >>> In the end, we just might want to add -fno-strict-aliasing to the
>> >>> HOSTCFLAGS, just like we already did it in make.rules for the normal
>> >>> CFLAGS, and call it a day.
>> >> 
>> >> Or you could fix the problems.  SLOF used to work with -fstrict-aliasing,
>> >> and it was a nice performance win.
>> >
>> > That's a question for Nikunj who committed that change a couple of years
>> > ago ...
>> > (see http://git.qemu.org/?p=SLOF.git;a=commitdiff;h=7eca6a5e56f468)
>> 
>> I remember that we had a discussion on this and BenH had suggested that
>> we cannot re-audit all the code for correctness with strict-aliasing. So
>> disable it.
>
> That's what -Wstrict-aliasing is for (needs -fstrict-aliasing active as
> well, to work).
>
> Rewriting the code to make it better can be quite a bit of work, of
> course.  But you could e.g. only use -fno-strict-aliasing on those files
> where -Wstrict-aliasing warns.

I think this is what is needed. I have to test this well, as I am not
exactly sure what was the way we were hitting this.

On 09.08.2016 04:16, Nikunj A Dadhania wrote:
> Segher Boessenkool <segher@kernel.crashing.org> writes:
> 
>> On Mon, Aug 08, 2016 at 02:45:35PM +0530, Nikunj A Dadhania wrote:
>>>>>> In the end, we just might want to add -fno-strict-aliasing to the
>>>>>> HOSTCFLAGS, just like we already did it in make.rules for the normal
>>>>>> CFLAGS, and call it a day.
>>>>>
>>>>> Or you could fix the problems.  SLOF used to work with -fstrict-aliasing,
>>>>> and it was a nice performance win.
>>>>
>>>> That's a question for Nikunj who committed that change a couple of years
>>>> ago ...
>>>> (see http://git.qemu.org/?p=SLOF.git;a=commitdiff;h=7eca6a5e56f468)
>>>
>>> I remember that we had a discussion on this and BenH had suggested that
>>> we cannot re-audit all the code for correctness with strict-aliasing. So
>>> disable it.
>>
>> That's what -Wstrict-aliasing is for (needs -fstrict-aliasing active as
>> well, to work).
>>
>> Rewriting the code to make it better can be quite a bit of work, of
>> course.  But you could e.g. only use -fno-strict-aliasing on those files
>> where -Wstrict-aliasing warns.
> 
> I think this is what is needed. I have to test this well, as I am not
> exactly sure what was the way we were hitting this.
> 
> diff --git a/clients/net-snk/app/netlib/ipv4.c b/clients/net-snk/app/netlib/ipv4.c
> index 3a1a789..157d71c 100644
> --- a/clients/net-snk/app/netlib/ipv4.c
> +++ b/clients/net-snk/app/netlib/ipv4.c
> @@ -547,7 +547,7 @@ static void fill_udp_checksum(struct iphdr *ipv4_hdr)
>         unsigned i;
>         unsigned long checksum = 0;
>         struct iphdr ip_hdr;
> -       char *ptr;
> +       uint16_t *ptr;
>         udp_hdr_t *udp_hdr;
>  
>         udp_hdr = (udp_hdr_t *) (ipv4_hdr + 1);
> @@ -559,13 +559,13 @@ static void fill_udp_checksum(struct iphdr *ipv4_hdr)
>         ip_hdr.ip_len    = udp_hdr->uh_ulen;
>         ip_hdr.ip_p      = ipv4_hdr->ip_p;
>  
> -       ptr = (char*) udp_hdr;
> -       for (i = 0; i < udp_hdr->uh_ulen; i+=2)
> -               checksum += *((uint16_t*) &ptr[i]);
> +       ptr = (uint16_t*) udp_hdr;
> +       for (i = 0; i < udp_hdr->uh_ulen; i++)

Don't you need to rather check for " i < udp_hdr->uh_ulen / 2" now?

> +               checksum += ptr[i];
>  
> -       ptr = (char*) &ip_hdr;
> -       for (i = 0; i < sizeof(struct iphdr); i+=2)
> -               checksum += *((uint16_t*) &ptr[i]);
> +       ptr = (uint16_t*) &ip_hdr;
> +       for (i = 0; i < sizeof(struct iphdr); i++)
> +               checksum += ptr[i];

Dito, the limit should be divided by 2 now?

 Thomas

Thomas Huth <thuth@redhat.com> writes:

> On 09.08.2016 04:16, Nikunj A Dadhania wrote:
>> Segher Boessenkool <segher@kernel.crashing.org> writes:
>> 
>>> On Mon, Aug 08, 2016 at 02:45:35PM +0530, Nikunj A Dadhania wrote:
>>>>>>> In the end, we just might want to add -fno-strict-aliasing to the
>>>>>>> HOSTCFLAGS, just like we already did it in make.rules for the normal
>>>>>>> CFLAGS, and call it a day.
>>>>>>
>>>>>> Or you could fix the problems.  SLOF used to work with -fstrict-aliasing,
>>>>>> and it was a nice performance win.
>>>>>
>>>>> That's a question for Nikunj who committed that change a couple of years
>>>>> ago ...
>>>>> (see http://git.qemu.org/?p=SLOF.git;a=commitdiff;h=7eca6a5e56f468)
>>>>
>>>> I remember that we had a discussion on this and BenH had suggested that
>>>> we cannot re-audit all the code for correctness with strict-aliasing. So
>>>> disable it.
>>>
>>> That's what -Wstrict-aliasing is for (needs -fstrict-aliasing active as
>>> well, to work).
>>>
>>> Rewriting the code to make it better can be quite a bit of work, of
>>> course.  But you could e.g. only use -fno-strict-aliasing on those files
>>> where -Wstrict-aliasing warns.
>> 
>> I think this is what is needed. I have to test this well, as I am not
>> exactly sure what was the way we were hitting this.
>> 
>> diff --git a/clients/net-snk/app/netlib/ipv4.c b/clients/net-snk/app/netlib/ipv4.c
>> index 3a1a789..157d71c 100644
>> --- a/clients/net-snk/app/netlib/ipv4.c
>> +++ b/clients/net-snk/app/netlib/ipv4.c
>> @@ -547,7 +547,7 @@ static void fill_udp_checksum(struct iphdr *ipv4_hdr)
>>         unsigned i;
>>         unsigned long checksum = 0;
>>         struct iphdr ip_hdr;
>> -       char *ptr;
>> +       uint16_t *ptr;
>>         udp_hdr_t *udp_hdr;
>>  
>>         udp_hdr = (udp_hdr_t *) (ipv4_hdr + 1);
>> @@ -559,13 +559,13 @@ static void fill_udp_checksum(struct iphdr *ipv4_hdr)
>>         ip_hdr.ip_len    = udp_hdr->uh_ulen;
>>         ip_hdr.ip_p      = ipv4_hdr->ip_p;
>>  
>> -       ptr = (char*) udp_hdr;
>> -       for (i = 0; i < udp_hdr->uh_ulen; i+=2)
>> -               checksum += *((uint16_t*) &ptr[i]);
>> +       ptr = (uint16_t*) udp_hdr;
>> +       for (i = 0; i < udp_hdr->uh_ulen; i++)
>
> Don't you need to rather check for " i < udp_hdr->uh_ulen / 2" now?

Right, missed that. And I think we need to take care for odd length as
well.

i < (udp_hdr->uh_ulen + 1)/ 2

>> +               checksum += ptr[i];
>>  
>> -       ptr = (char*) &ip_hdr;
>> -       for (i = 0; i < sizeof(struct iphdr); i+=2)
>> -               checksum += *((uint16_t*) &ptr[i]);
>> +       ptr = (uint16_t*) &ip_hdr;
>> +       for (i = 0; i < sizeof(struct iphdr); i++)
>> +               checksum += ptr[i];
>
> Dito, the limit should be divided by 2 now?

Sure.

Regards,
Nikunj

On Tue, Aug 09, 2016 at 12:15:42PM +0530, Nikunj A Dadhania wrote:
> > Don't you need to rather check for " i < udp_hdr->uh_ulen / 2" now?
> 
> Right, missed that. And I think we need to take care for odd length as
> well.

Yes, both those.  The original code doesn't handle odd length correctly
either it seems.

The checksum magically works out with either BE or LE 16-bit accesses,
but you might want to make that explicit, too.


Segher