gcc warning in my trace_benchmark() code

gcc warning in my trace_benchmark() code

[Steven Rostedt]

I'm going through some of the warnings that Fengguang Wu's test bot has
discovered, and one of them is from a MIPS allmodconfig build.

https://lists.01.org/pipermail/kbuild-all/2014-May/004751.html

   kernel/trace/trace_benchmark.c: In function 'trace_do_benchmark':
>> kernel/trace/trace_benchmark.c:84:3: warning: comparison of distinct pointer types lacks a cast [enabled by default]
>> kernel/trace/trace_benchmark.c:85:3: warning: comparison of distinct pointer types lacks a cast [enabled by default]
   kernel/trace/trace_benchmark.c:38:6: warning: unused variable 'seedsq' [-Wunused-variable]

vim +84 kernel/trace/trace_benchmark.c

    78		if (bm_cnt > 1) {
    79			/*
    80			 * Apply Welford's method to calculate standard deviation:
    81			 * s^2 = 1 / (n * (n-1)) * (n * \Sum (x_i)^2 - (\Sum x_i)^2)
    82			 */
    83			stddev = (u64)bm_cnt * bm_totalsq - bm_total * bm_total;
  > 84			do_div(stddev, bm_cnt);
  > 85			do_div(stddev, bm_cnt - 1);
    86		} else
    87			stddev = 0;
    88	



Is there something special with do_div in mips that I should be aware
of?

-- Steve

Re: gcc warning in my trace_benchmark() code

[David Daney]

On 06/05/2014 09:12 AM, Steven Rostedt wrote:
> I'm going through some of the warnings that Fengguang Wu's test bot has
> discovered, and one of them is from a MIPS allmodconfig build.
>
> https://lists.01.org/pipermail/kbuild-all/2014-May/004751.html
>
>     kernel/trace/trace_benchmark.c: In function 'trace_do_benchmark':
>>> kernel/trace/trace_benchmark.c:84:3: warning: comparison of distinct pointer types lacks a cast [enabled by default]
>>> kernel/trace/trace_benchmark.c:85:3: warning: comparison of distinct pointer types lacks a cast [enabled by default]
>     kernel/trace/trace_benchmark.c:38:6: warning: unused variable 'seedsq' [-Wunused-variable]
>
> vim +84 kernel/trace/trace_benchmark.c
>
>      78		if (bm_cnt > 1) {
>      79			/*
>      80			 * Apply Welford's method to calculate standard deviation:
>      81			 * s^2 = 1 / (n * (n-1)) * (n * \Sum (x_i)^2 - (\Sum x_i)^2)
>      82			 */
>      83			stddev = (u64)bm_cnt * bm_totalsq - bm_total * bm_total;
>    > 84			do_div(stddev, bm_cnt);
>    > 85			do_div(stddev, bm_cnt - 1);
>      86		} else
>      87			stddev = 0;
>      88	
>
>
>
> Is there something special with do_div in mips that I should be aware
> of?

Yes.  MIPS is using the implementation in asm-generic/div64.h,  which 
per the comments in that file has a useless pointer compare to find just 
this type of issue.


Ralf:  As a side note, while looking at arch/mips/include/asm/div64.h, I 
saw that the implementation of __div64_32 in that file will be unused, 
and is also completely broken due to the first parameter never being used.

David Daney

Re: gcc warning in my trace_benchmark() code

[Steven Rostedt]

On Thu, 05 Jun 2014 10:12:16 -0700
David Daney <ddaney.cavm@gmail.com> wrote:

> On 06/05/2014 09:12 AM, Steven Rostedt wrote:
> > I'm going through some of the warnings that Fengguang Wu's test bot has
> > discovered, and one of them is from a MIPS allmodconfig build.
> >
> > https://lists.01.org/pipermail/kbuild-all/2014-May/004751.html
> >
> >     kernel/trace/trace_benchmark.c: In function 'trace_do_benchmark':
> >>> kernel/trace/trace_benchmark.c:84:3: warning: comparison of distinct pointer types lacks a cast [enabled by default]
> >>> kernel/trace/trace_benchmark.c:85:3: warning: comparison of distinct pointer types lacks a cast [enabled by default]
> >     kernel/trace/trace_benchmark.c:38:6: warning: unused variable 'seedsq' [-Wunused-variable]
> >
> > vim +84 kernel/trace/trace_benchmark.c
> >
> >      78		if (bm_cnt > 1) {
> >      79			/*
> >      80			 * Apply Welford's method to calculate standard deviation:
> >      81			 * s^2 = 1 / (n * (n-1)) * (n * \Sum (x_i)^2 - (\Sum x_i)^2)
> >      82			 */
> >      83			stddev = (u64)bm_cnt * bm_totalsq - bm_total * bm_total;
> >    > 84			do_div(stddev, bm_cnt);
> >    > 85			do_div(stddev, bm_cnt - 1);
> >      86		} else
> >      87			stddev = 0;
> >      88	
> >
> >
> >
> > Is there something special with do_div in mips that I should be aware
> > of?
> 
> Yes.  MIPS is using the implementation in asm-generic/div64.h,  which 
> per the comments in that file has a useless pointer compare to find just 
> this type of issue.

You mean this comment?

/* The unnecessary pointer compare is there
 * to check for type safety (n must be 64bit)
 */

But stddev is s64. Ah, but the compare is:

(void)(((typeof((n)) *)0) == ((uint64_t *)0));

so it's complaining about a signed verses unsigned compare, not length.
I think I can ignore this warning then.

Thoughts?

-- Steve


> 
> 
> Ralf:  As a side note, while looking at arch/mips/include/asm/div64.h, I 
> saw that the implementation of __div64_32 in that file will be unused, 
> and is also completely broken due to the first parameter never being used.
> 
> David Daney
> 

Re: gcc warning in my trace_benchmark() code

[David Daney]

On 06/05/2014 10:35 AM, Steven Rostedt wrote:
> On Thu, 05 Jun 2014 10:12:16 -0700
> David Daney <ddaney.cavm@gmail.com> wrote:
>
>> On 06/05/2014 09:12 AM, Steven Rostedt wrote:
>>> I'm going through some of the warnings that Fengguang Wu's test bot has
>>> discovered, and one of them is from a MIPS allmodconfig build.
>>>
>>> https://lists.01.org/pipermail/kbuild-all/2014-May/004751.html
>>>
>>>      kernel/trace/trace_benchmark.c: In function 'trace_do_benchmark':
>>>>> kernel/trace/trace_benchmark.c:84:3: warning: comparison of distinct pointer types lacks a cast [enabled by default]
>>>>> kernel/trace/trace_benchmark.c:85:3: warning: comparison of distinct pointer types lacks a cast [enabled by default]
>>>      kernel/trace/trace_benchmark.c:38:6: warning: unused variable 'seedsq' [-Wunused-variable]
>>>
>>> vim +84 kernel/trace/trace_benchmark.c
>>>
>>>       78		if (bm_cnt > 1) {
>>>       79			/*
>>>       80			 * Apply Welford's method to calculate standard deviation:
>>>       81			 * s^2 = 1 / (n * (n-1)) * (n * \Sum (x_i)^2 - (\Sum x_i)^2)
>>>       82			 */
>>>       83			stddev = (u64)bm_cnt * bm_totalsq - bm_total * bm_total;
>>>     > 84			do_div(stddev, bm_cnt);
>>>     > 85			do_div(stddev, bm_cnt - 1);
>>>       86		} else
>>>       87			stddev = 0;
>>>       88	
>>>
>>>
>>>
>>> Is there something special with do_div in mips that I should be aware
>>> of?
>>
>> Yes.  MIPS is using the implementation in asm-generic/div64.h,  which
>> per the comments in that file has a useless pointer compare to find just
>> this type of issue.
>
> You mean this comment?
>
> /* The unnecessary pointer compare is there
>   * to check for type safety (n must be 64bit)
>   */
>

Yes.

> But stddev is s64. Ah, but the compare is:
>
> (void)(((typeof((n)) *)0) == ((uint64_t *)0));
>
> so it's complaining about a signed verses unsigned compare, not length.
> I think I can ignore this warning then.

The pedant in me thinks that you should fix your code if using do_div() 
on a signed object is undefined.  But if you aren't planning on merging 
the code, then it probably doesn't matter.

>
> Thoughts?

I think I will have lunch now...

David Daney

>

Re: gcc warning in my trace_benchmark() code

[Steven Rostedt]

On Thu, 05 Jun 2014 11:44:45 -0700
David Daney <ddaney.cavm@gmail.com> wrote:

> > But stddev is s64. Ah, but the compare is:
> >
> > (void)(((typeof((n)) *)0) == ((uint64_t *)0));
> >
> > so it's complaining about a signed verses unsigned compare, not length.
> > I think I can ignore this warning then.
> 
> The pedant in me thinks that you should fix your code if using do_div() 
> on a signed object is undefined.  But if you aren't planning on merging 
> the code, then it probably doesn't matter.

It's undefined on signed 64 numbers? Where is that documented. I don't
see it in the comments, and I don't see anything in the Documentation
directory. It only states that n must be 64bit. It doesn't say unsigned
64 bit.

And yes I do plan on merging this. It's in my 3.16 queue right now and
in linux-next. But it's just a benchmark tracepoint that requires a
config option to enable it. It will show up in your allmodconfig builds
but nothing important.

Worse comes to worse, I can add a (u64) to the call to do_div() I guess.

> 
> >
> > Thoughts?
> 
> I think I will have lunch now...
> 

I just came back from lunch. It was quite delicious!

-- Steve

Re: gcc warning in my trace_benchmark() code

[David Daney]

On 06/05/2014 11:53 AM, Steven Rostedt wrote:
> On Thu, 05 Jun 2014 11:44:45 -0700
> David Daney <ddaney.cavm@gmail.com> wrote:
>
>>> But stddev is s64. Ah, but the compare is:
>>>
>>> (void)(((typeof((n)) *)0) == ((uint64_t *)0));
>>>
>>> so it's complaining about a signed verses unsigned compare, not length.
>>> I think I can ignore this warning then.
>>
>> The pedant in me thinks that you should fix your code if using do_div()
>> on a signed object is undefined.  But if you aren't planning on merging
>> the code, then it probably doesn't matter.
>
> It's undefined on signed 64 numbers?

Evidently it is.

The top of asm-generic/div64.h has:

.
.
.
  * The semantics of do_div() are:
  *
  * uint32_t do_div(uint64_t *n, uint32_t base)
  * {
.
.
.

do_div() really passes the first parameter by reference, and C doesn't 
have by reference parameters, so the example is not quite right.  But it 
does seem to imply the thing should be an *unsigned* 64-bit wide variable.

It has been like this since the beginning of the git epoch.

> Where is that documented.

The code is the documentation.

> I don't
> see it in the comments, and I don't see anything in the Documentation
> directory. It only states that n must be 64bit. It doesn't say unsigned
> 64 bit.

The handful of call sites I examined, seem to all use u64 or unsigned 
long long.

I get:

   $ grep -r do_div Documentation | wc
       0       0       0

So it would seem that most of the do_div() documentation actually is the 
code.

David Daney

Re: gcc warning in my trace_benchmark() code

[Steven Rostedt]

On Thu, 05 Jun 2014 14:56:24 -0700
David Daney <ddaney.cavm@gmail.com> wrote:

 
> Evidently it is.
> 
> The top of asm-generic/div64.h has:
> 
> .
> .
> .
>   * The semantics of do_div() are:
>   *
>   * uint32_t do_div(uint64_t *n, uint32_t base)
>   * {

That's somewhat lacking. Does this mean that we have no consistent way
to divide a s64 number?

> .
> .
> .
> 
> do_div() really passes the first parameter by reference, and C doesn't 
> have by reference parameters, so the example is not quite right.  But it 
> does seem to imply the thing should be an *unsigned* 64-bit wide variable.
> 
> It has been like this since the beginning of the git epoch.
> 
> > Where is that documented.
> 
> The code is the documentation.
> 
> > I don't
> > see it in the comments, and I don't see anything in the Documentation
> > directory. It only states that n must be 64bit. It doesn't say unsigned
> > 64 bit.
> 
> The handful of call sites I examined, seem to all use u64 or unsigned 
> long long.

and u64 and unsigned long long are usually the standard type to use for
64 bits.

> 
> I get:
> 
>    $ grep -r do_div Documentation | wc
>        0       0       0
> 
> So it would seem that most of the do_div() documentation actually is the 
> code.
> 

Which means there isn't documentation for it.

Anyway, this probably can be safely converted to an unsigned. As I'm
not sure standard deviation can ever be negative.

-- Steve

Re: gcc warning in my trace_benchmark() code

[Geert Uytterhoeven]

On Thu, Jun 5, 2014 at 11:56 PM, David Daney <ddaney.cavm@gmail.com> wrote:
>> I don't
>> see it in the comments, and I don't see anything in the Documentation
>> directory. It only states that n must be 64bit. It doesn't say unsigned
>> 64 bit.
>
> The handful of call sites I examined, seem to all use u64 or unsigned long
> long.

And the ones that don't, cause warnings, and are being fixed:

https://www.mail-archive.com/linuxppc-dev@lists.ozlabs.org/msg78445.html

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: gcc warning in my trace_benchmark() code

[Ralf Baechle]

On Thu, Jun 05, 2014 at 10:12:16AM -0700, David Daney wrote:

> >vim +84 kernel/trace/trace_benchmark.c
> >
> >     78		if (bm_cnt > 1) {
> >     79			/*
> >     80			 * Apply Welford's method to calculate standard deviation:
> >     81			 * s^2 = 1 / (n * (n-1)) * (n * \Sum (x_i)^2 - (\Sum x_i)^2)
> >     82			 */
> >     83			stddev = (u64)bm_cnt * bm_totalsq - bm_total * bm_total;
> >   > 84			do_div(stddev, bm_cnt);
> >   > 85			do_div(stddev, bm_cnt - 1);
> >     86		} else
> >     87			stddev = 0;
> >     88	
> >
> >
> >
> >Is there something special with do_div in mips that I should be aware
> >of?
> 
> Yes.  MIPS is using the implementation in asm-generic/div64.h,
> which per the comments in that file has a useless pointer compare to
> find just this type of issue.

And it's not the only warning it's picking up.

> Ralf:  As a side note, while looking at
> arch/mips/include/asm/div64.h, I saw that the implementation of
> __div64_32 in that file will be unused, and is also completely
> broken due to the first parameter never being used.

Seems I broke c21004cd5b4cb7d479514d470a62366e8307412c "MIPS: Rewrite
<asm/div64.h> to work with gcc 4.4.0."  Took only five years until
somebody noticed ...

  Ralf

Re: gcc warning in my trace_benchmark() code

[Maciej W. Rozycki]

On Thu, 5 Jun 2014, Ralf Baechle wrote:

> > Ralf:  As a side note, while looking at
> > arch/mips/include/asm/div64.h, I saw that the implementation of
> > __div64_32 in that file will be unused, and is also completely
> > broken due to the first parameter never being used.
> 
> Seems I broke c21004cd5b4cb7d479514d470a62366e8307412c "MIPS: Rewrite
> <asm/div64.h> to work with gcc 4.4.0."  Took only five years until
> somebody noticed ...

 Well, it's not really possible to track all the breakage introduced 
unless at least a warning is spat for some config.  Which is why I think 
it's a good idea to have all non-trivial changes put through a review 
process or at least posted to the relevant mailing list.

 This change didn't appear anywhere, only the commit log message was 
posted, so there was no chance to spot the damage, unless there was 
someone who actively studied changes made to the tree.

  Maciej