Hi Fengguang,
I also check new static checker warnings and sometimes email people.
I wonder if we are duplicating each others work. For example, did
you send an email asking about the following Sparse warning:
sound/soc/codecs/ab8500-codec.c:1959:53: warning: cast truncates bits from constant value (1013 becomes 13)
Quite often those messages are false positive and the value is
truncated deliberately. In this case it looks suspicious and I
would maybe email about it. If I knew what kind of messages you
check and which you ignore that would help me.
Perhaps there is an email list I could subscribe to to see if you
had already sent a message.
regards,
dan carpenter
Hi Dan,
On Thu, Jun 14, 2012 at 08:25:23PM +0300, Dan Carpenter wrote:
> Hi Fengguang,
>
> I also check new static checker warnings and sometimes email people.
That would be nice!
> I wonder if we are duplicating each others work. For example, did
> you send an email asking about the following Sparse warning:
>
> sound/soc/codecs/ab8500-codec.c:1959:53: warning: cast truncates bits from constant value (1013 becomes 13)
I did try sending out sparse warnings to people, some time ago...
> Quite often those messages are false positive and the value is
> truncated deliberately. In this case it looks suspicious and I
> would maybe email about it. If I knew what kind of messages you
> check and which you ignore that would help me.
The lots of false warnings are a big problem. It makes the automated
notification more noises than signals to people. So I end up disabling
the sparse check totally..
These days I only send out gcc errors/warnings to the commit author,
committer, signers and relevant mailing lists reported by
scripts/get_maintainer.pl (but minus LKML).
> Perhaps there is an email list I could subscribe to to see if you
> had already sent a message.
In an average working day, 1-2 build errors will be caught and email
notified. I guess there will be more sparse warnings if it's turned
on.
Perhaps the sparse warnings can be enabled, but only sent to the patch
author. If you and anyone else are interested, they could be sent to
some mailing list, too. One thing I'm sure is, we probably never want
to disturb the busy maintainers with these warnings.
Thanks,
Fengguang
On Fri, Jun 15, 2012 at 09:48:35AM +0800, Fengguang Wu wrote:
> The lots of false warnings are a big problem. It makes the automated
> notification more noises than signals to people. So I end up disabling
> the sparse check totally..
>
I do a basic sanity check of my emails before I send them.
Sometimes I do send false positives. If the warning is introduced
by a very new code then probably the patch author can answer my
question off the top of her head. Also I send some false positives
just to try learn what the rules are.
> In an average working day, 1-2 build errors will be caught and email
> notified. I guess there will be more sparse warnings if it's turned
> on.
>
> Perhaps the sparse warnings can be enabled, but only sent to the patch
> author. If you and anyone else are interested, they could be sent to
> some mailing list, too. One thing I'm sure is, we probably never want
> to disturb the busy maintainers with these warnings.
Eventually I think we will want to set up a mailing list for this or
we will start sending duplicate messages.
regards,
dan carpenter
[CC sparse developers]
// on setting up a list and sending newly found sparse warnings to it
On Fri, Jun 15, 2012 at 10:12:22AM +0300, Dan Carpenter wrote:
> On Fri, Jun 15, 2012 at 09:48:35AM +0800, Fengguang Wu wrote:
> > The lots of false warnings are a big problem. It makes the automated
> > notification more noises than signals to people. So I end up disabling
> > the sparse check totally..
> >
>
> I do a basic sanity check of my emails before I send them.
Yeah that would be better.
> Sometimes I do send false positives. If the warning is introduced
> by a very new code then probably the patch author can answer my
> question off the top of her head. Also I send some false positives
> just to try learn what the rules are.
Heh.
> > In an average working day, 1-2 build errors will be caught and email
> > notified. I guess there will be more sparse warnings if it's turned
> > on.
> >
> > Perhaps the sparse warnings can be enabled, but only sent to the patch
> > author. If you and anyone else are interested, they could be sent to
> > some mailing list, too. One thing I'm sure is, we probably never want
> > to disturb the busy maintainers with these warnings.
>
> Eventually I think we will want to set up a mailing list for this or
> we will start sending duplicate messages.
Fair enough. How can we setup the mailing list? Once the list up, it
would be trivial for me to send sparse warnings out there.
Thanks,
Fengguang
On Fri, Jun 15, 2012 at 03:58:10PM +0800, Fengguang Wu wrote:
> On Fri, Jun 15, 2012 at 10:12:22AM +0300, Dan Carpenter wrote:
> > On Fri, Jun 15, 2012 at 09:48:35AM +0800, Fengguang Wu wrote:
> > > In an average working day, 1-2 build errors will be caught and email
> > > notified. I guess there will be more sparse warnings if it's turned
> > > on.
> > >
> > > Perhaps the sparse warnings can be enabled, but only sent to the patch
> > > author. If you and anyone else are interested, they could be sent to
> > > some mailing list, too. One thing I'm sure is, we probably never want
> > > to disturb the busy maintainers with these warnings.
> >
> > Eventually I think we will want to set up a mailing list for this or
> > we will start sending duplicate messages.
>
> Fair enough. How can we setup the mailing list? Once the list up, it
> would be trivial for me to send sparse warnings out there.
Rather than a mailing list, how about something like test.kernel.org for
sparse warnings?
- Josh Triplett
On Fri, Jun 15, 2012 at 01:31:00AM -0700, Josh Triplett wrote:
> On Fri, Jun 15, 2012 at 03:58:10PM +0800, Fengguang Wu wrote:
> > On Fri, Jun 15, 2012 at 10:12:22AM +0300, Dan Carpenter wrote:
> > > On Fri, Jun 15, 2012 at 09:48:35AM +0800, Fengguang Wu wrote:
> > > > In an average working day, 1-2 build errors will be caught and email
> > > > notified. I guess there will be more sparse warnings if it's turned
> > > > on.
> > > >
> > > > Perhaps the sparse warnings can be enabled, but only sent to the patch
> > > > author. If you and anyone else are interested, they could be sent to
> > > > some mailing list, too. One thing I'm sure is, we probably never want
> > > > to disturb the busy maintainers with these warnings.
> > >
> > > Eventually I think we will want to set up a mailing list for this or
> > > we will start sending duplicate messages.
> >
> > Fair enough. How can we setup the mailing list? Once the list up, it
> > would be trivial for me to send sparse warnings out there.
>
> Rather than a mailing list, how about something like test.kernel.org for
> sparse warnings?
It's much more trivial to send new build/sparse errors/warnings to a
list than to setup a website :-) As the errors come and go every day,
and they are mostly unstructured, it seems the mailing list would be a
more natural fit. People can search for known errors there and/or CC
fixes there.
Anyway, we just sent an request for creating
[email protected]
Thanks,
Fengguang
> > Eventually I think we will want to set up a mailing list for this or
> > we will start sending duplicate messages.
>
> Fair enough. How can we setup the mailing list? Once the list up, it
> would be trivial for me to send sparse warnings out there.
I'm not completely sure that a mailing list would completely eliminate
duplicate messages. But still, it could be the place for people who are
interested in seeing such messages to go to, so it seems like a good
thing. I would be happy to contribute content :)
julia
On Fri, Jun 15, 2012 at 06:40:51AM -0400, Julia Lawall wrote:
> > > Eventually I think we will want to set up a mailing list for this or
> > > we will start sending duplicate messages.
> >
> > Fair enough. How can we setup the mailing list? Once the list up, it
> > would be trivial for me to send sparse warnings out there.
>
> I'm not completely sure that a mailing list would completely eliminate
> duplicate messages. But still, it could be the place for people who are
> interested in seeing such messages to go to, so it seems like a good
> thing. I would be happy to contribute content :)
Yeah. That might be interesting. If you don't know whether a bug
is a false positive or not you could submit it to the list for
people to look at.
I don't know if anyone will actually look at them. I had been
planning to filter them to a mail box and automatically ignore
anything that was a duplicate. But it might actually be worth
looking at them as well. Especially if you email had enough useful
context so I could tell from the message what the bug is.
Probably we could use something like the attached script to print
out the line of code which causes the bug and some other script to
querry git blame and attach the offending commit?
regards,
dan carpenter
Added git-blame support...
On Fri, Jun 15, 2012 at 8:19 AM, Dan Carpenter <[email protected]> wrote:
> On Fri, Jun 15, 2012 at 06:40:51AM -0400, Julia Lawall wrote:
>> > > Eventually I think we will want to set up a mailing list for this or
>> > > we will start sending duplicate messages.
>> >
>> > Fair enough. How can we setup the mailing list? Once the list up, it
>> > would be trivial for me to send sparse warnings out there.
>>
>> I'm not completely sure that a mailing list would completely eliminate
>> duplicate messages. ?But still, it could be the place for people who are
>> interested in seeing such messages to go to, so it seems like a good
>> thing. ?I would be happy to contribute content :)
>
> Yeah. ?That might be interesting. ?If you don't know whether a bug
> is a false positive or not you could submit it to the list for
> people to look at.
>
> I don't know if anyone will actually look at them. ?I had been
> planning to filter them to a mail box and automatically ignore
> anything that was a duplicate. ?But it might actually be worth
> looking at them as well. ?Especially if you email had enough useful
> context so I could tell from the message what the bug is.
>
> Probably we could use something like the attached script to print
> out the line of code which causes the bug and some other script to
> querry git blame and attach the offending commit?
>
> regards,
> dan carpenter
>
--
Peter Senna Tschudin
[email protected]
gpg id: 48274C36
Dropped the Sparse people from the CC list.
On Fri, Jun 15, 2012 at 10:33:50AM -0300, Peter Senna Tschudin wrote:
> Added git-blame support...
>
No. I meant that it would run git blame on the offending line, take
the hash and then do a git show $hash.
regards,
dan carpenter
On Fri, Jun 15, 2012 at 02:19:14PM +0300, Dan Carpenter wrote:
> On Fri, Jun 15, 2012 at 06:40:51AM -0400, Julia Lawall wrote:
> > > > Eventually I think we will want to set up a mailing list for this or
> > > > we will start sending duplicate messages.
> > >
> > > Fair enough. How can we setup the mailing list? Once the list up, it
> > > would be trivial for me to send sparse warnings out there.
> >
> > I'm not completely sure that a mailing list would completely eliminate
> > duplicate messages. But still, it could be the place for people who are
> > interested in seeing such messages to go to, so it seems like a good
> > thing. I would be happy to contribute content :)
>
> Yeah. That might be interesting. If you don't know whether a bug
> is a false positive or not you could submit it to the list for
> people to look at.
>
> I don't know if anyone will actually look at them. I had been
> planning to filter them to a mail box and automatically ignore
> anything that was a duplicate. But it might actually be worth
> looking at them as well. Especially if you email had enough useful
> context so I could tell from the message what the bug is.
>
> Probably we could use something like the attached script to print
> out the line of code which causes the bug and some other script to
> querry git blame and attach the offending commit?
cat -n $code_file | tail -n +$(($lineno - (($context + 1) / 2))) | head -n $(($context + 1))
That's handy, I'll use it to show the source file context for the
first error/warning :-)
Example:
drivers/leds/led-triggers.c: In function ‘led_trigger_event’:
drivers/leds/led-triggers.c:227:3: error: implicit declaration of function ‘led_set_brightness’ [-Werror=implicit-function-declaration]
drivers/leds/led-triggers.c:227:
224 struct led_classdev *led_cdev;
225
226 led_cdev = list_entry(entry, struct led_classdev, trig_list);
> 227 led_set_brightness(led_cdev, brightness);
228 }
229 read_unlock(&trigger->leddev_list_lock);
230 }
Thanks!
Fengguang
On Fri, Jun 15, 2012 at 10:53 AM, Dan Carpenter
<[email protected]> wrote:
> Dropped the Sparse people from the CC list.
>
> On Fri, Jun 15, 2012 at 10:33:50AM -0300, Peter Senna Tschudin wrote:
>> Added git-blame support...
>>
>
> No. ?I meant that it would run git blame on the offending line, take
> the hash and then do a git show $hash.
http://pastebin.com/cZCPQuzH
>
> regards,
> dan carpenter
>
--
Peter Senna Tschudin
[email protected]
gpg id: 48274C36
On 06/15/2012 01:54 AM, Fengguang Wu wrote:
> On Fri, Jun 15, 2012 at 01:31:00AM -0700, Josh Triplett wrote:
>> On Fri, Jun 15, 2012 at 03:58:10PM +0800, Fengguang Wu wrote:
>>> On Fri, Jun 15, 2012 at 10:12:22AM +0300, Dan Carpenter wrote:
>>>> On Fri, Jun 15, 2012 at 09:48:35AM +0800, Fengguang Wu wrote:
>>>>> In an average working day, 1-2 build errors will be caught and email
>>>>> notified. I guess there will be more sparse warnings if it's turned
>>>>> on.
>>>>>
>>>>> Perhaps the sparse warnings can be enabled, but only sent to the patch
>>>>> author. If you and anyone else are interested, they could be sent to
>>>>> some mailing list, too. One thing I'm sure is, we probably never want
>>>>> to disturb the busy maintainers with these warnings.
>>>>
>>>> Eventually I think we will want to set up a mailing list for this or
>>>> we will start sending duplicate messages.
>>>
>>> Fair enough. How can we setup the mailing list? Once the list up, it
>>> would be trivial for me to send sparse warnings out there.
>>
>> Rather than a mailing list, how about something like test.kernel.org for
>> sparse warnings?
>
> It's much more trivial to send new build/sparse errors/warnings to a
> list than to setup a website :-) As the errors come and go every day,
> and they are mostly unstructured, it seems the mailing list would be a
> more natural fit. People can search for known errors there and/or CC
> fixes there.
>
> Anyway, we just sent an request for creating
>
> [email protected]
and you will let us know when it has been created??
Although I had just as soon use an existing list, like
kernel-janitors or kernel-testers.
thanks,
--
~Randy
On Fri, Jun 15, 2012 at 10:34 PM, Fengguang Wu <[email protected]> wrote:
> On Fri, Jun 15, 2012 at 02:19:14PM +0300, Dan Carpenter wrote:
>>
>> Probably we could use something like the attached script to print
>> out the line of code which causes the bug and some other script to
>> querry git blame and attach the offending commit?
>
> cat -n $code_file | tail -n +$(($lineno - (($context + 1) / 2))) | head -n $(($context + 1))
>
> That's handy, I'll use it to show the source file context for the
> first error/warning :-)
Well, you can use sed/awk, it will be much shorter:
cat -n drivers/leds/led-triggers.c | sed -ne '224,230p'
224 struct led_classdev *led_cdev;
225
226 led_cdev = list_entry(entry, struct led_classdev, trig_list);
227 led_set_brightness(led_cdev, brightness);
228 }
229 read_unlock(&trigger->leddev_list_lock);
230 }
(replace the hard-coded "224,230" with a shell variable)
And if you want to find the offending commit:
git show `git blame drivers/leds/led-triggers.c | awk 'NR==227{print $1}'`
[drop sparse developers]
On Sat, Jun 16, 2012 at 03:50:36PM +0800, Cong Wang wrote:
> On Fri, Jun 15, 2012 at 10:34 PM, Fengguang Wu <[email protected]> wrote:
> > On Fri, Jun 15, 2012 at 02:19:14PM +0300, Dan Carpenter wrote:
> >>
> >> Probably we could use something like the attached script to print
> >> out the line of code which causes the bug and some other script to
> >> querry git blame and attach the offending commit?
> >
> > cat -n $code_file | tail -n +$(($lineno - (($context + 1) / 2))) | head -n $(($context + 1))
> >
> > That's handy, I'll use it to show the source file context for the
> > first error/warning :-)
>
> Well, you can use sed/awk, it will be much shorter:
>
> cat -n drivers/leds/led-triggers.c | sed -ne '224,230p'
> 224 struct led_classdev *led_cdev;
> 225
> 226 led_cdev = list_entry(entry, struct led_classdev, trig_list);
> 227 led_set_brightness(led_cdev, brightness);
> 228 }
> 229 read_unlock(&trigger->leddev_list_lock);
> 230 }
>
> (replace the hard-coded "224,230" with a shell variable)
Thanks! I'll use it this way:
lineno=227
context=3
cat -n drivers/leds/led-triggers.c | sed -e "s/ $lineno\t/> $lineno\t/" -ne "$((lineno - context)),$((lineno + context))p"
224 struct led_classdev *led_cdev;
225
226 led_cdev = list_entry(entry, struct led_classdev, trig_list);
> 227 led_set_brightness(led_cdev, brightness);
228 }
229 read_unlock(&trigger->leddev_list_lock);
230 }
> And if you want to find the offending commit:
>
> git show `git blame drivers/leds/led-triggers.c | awk 'NR==227{print $1}'`
The code offered by Peter should run faster:
hash=$(git blame $code_file -L $lineno,$lineno |cut -d " " -f 1)
git --no-pager show $hash $code_file
Anyway, Dan's original idea of using git blame to find out the offending
commit is valuable. The process can be automated like this:
1) use git-blame to find out the commit that changed $lineno in recent 100 days
2) checkout out $commit and build test and check build error
3) checkout out $commit~1 and build test and check build error
If (1) succeeded and (2) got the expected build error while (3) don't
have the build error, we reliably catch the bad commit. Otherwise the
script may try building test other commits that modified the file
recently.
Thanks,
Fengguang
On Fri, Jun 15, 2012 at 09:48:26AM -0700, Randy Dunlap wrote:
> On 06/15/2012 01:54 AM, Fengguang Wu wrote:
>
> > On Fri, Jun 15, 2012 at 01:31:00AM -0700, Josh Triplett wrote:
> >> On Fri, Jun 15, 2012 at 03:58:10PM +0800, Fengguang Wu wrote:
> >>> On Fri, Jun 15, 2012 at 10:12:22AM +0300, Dan Carpenter wrote:
> >>>> On Fri, Jun 15, 2012 at 09:48:35AM +0800, Fengguang Wu wrote:
> >>>>> In an average working day, 1-2 build errors will be caught and email
> >>>>> notified. I guess there will be more sparse warnings if it's turned
> >>>>> on.
> >>>>>
> >>>>> Perhaps the sparse warnings can be enabled, but only sent to the patch
> >>>>> author. If you and anyone else are interested, they could be sent to
> >>>>> some mailing list, too. One thing I'm sure is, we probably never want
> >>>>> to disturb the busy maintainers with these warnings.
> >>>>
> >>>> Eventually I think we will want to set up a mailing list for this or
> >>>> we will start sending duplicate messages.
> >>>
> >>> Fair enough. How can we setup the mailing list? Once the list up, it
> >>> would be trivial for me to send sparse warnings out there.
> >>
> >> Rather than a mailing list, how about something like test.kernel.org for
> >> sparse warnings?
> >
> > It's much more trivial to send new build/sparse errors/warnings to a
> > list than to setup a website :-) As the errors come and go every day,
> > and they are mostly unstructured, it seems the mailing list would be a
> > more natural fit. People can search for known errors there and/or CC
> > fixes there.
> >
> > Anyway, we just sent an request for creating
> >
> > [email protected]
>
>
> and you will let us know when it has been created??
Well, the request has been rejected anyway..
> Although I had just as soon use an existing list, like
> kernel-janitors or kernel-testers.
>From http://kernelnewbies.org/KernelJanitors :
Some suggestions to kernel newbies:
avoid fixing compiler warnings because the goal is to fix the
CAUSE of the warnings (which is usually not obvious), not just
to make the warnings go away
Does that suggest the commit author be the best people to fix
warnings? The typical situation may be, the author is not aware of the
warnings at all: they are buried in the tedious output of make...
Thanks,
Fengguang
On 06/16/2012 02:17 AM, Fengguang Wu wrote:
> On Fri, Jun 15, 2012 at 09:48:26AM -0700, Randy Dunlap wrote:
>> On 06/15/2012 01:54 AM, Fengguang Wu wrote:
>>
>>> On Fri, Jun 15, 2012 at 01:31:00AM -0700, Josh Triplett wrote:
>>>> On Fri, Jun 15, 2012 at 03:58:10PM +0800, Fengguang Wu wrote:
>>>>> On Fri, Jun 15, 2012 at 10:12:22AM +0300, Dan Carpenter wrote:
>>>>>> On Fri, Jun 15, 2012 at 09:48:35AM +0800, Fengguang Wu wrote:
>>>>>>> In an average working day, 1-2 build errors will be caught and email
>>>>>>> notified. I guess there will be more sparse warnings if it's turned
>>>>>>> on.
>>>>>>>
>>>>>>> Perhaps the sparse warnings can be enabled, but only sent to the patch
>>>>>>> author. If you and anyone else are interested, they could be sent to
>>>>>>> some mailing list, too. One thing I'm sure is, we probably never want
>>>>>>> to disturb the busy maintainers with these warnings.
>>>>>>
>>>>>> Eventually I think we will want to set up a mailing list for this or
>>>>>> we will start sending duplicate messages.
>>>>>
>>>>> Fair enough. How can we setup the mailing list? Once the list up, it
>>>>> would be trivial for me to send sparse warnings out there.
>>>>
>>>> Rather than a mailing list, how about something like test.kernel.org for
>>>> sparse warnings?
>>>
>>> It's much more trivial to send new build/sparse errors/warnings to a
>>> list than to setup a website :-) As the errors come and go every day,
>>> and they are mostly unstructured, it seems the mailing list would be a
>>> more natural fit. People can search for known errors there and/or CC
>>> fixes there.
>>>
>>> Anyway, we just sent an request for creating
>>>
>>> [email protected]
>>
>>
>> and you will let us know when it has been created??
>
> Well, the request has been rejected anyway..
>
>> Although I had just as soon use an existing list, like
>> kernel-janitors or kernel-testers.
>
> From http://kernelnewbies.org/KernelJanitors :
>
> Some suggestions to kernel newbies:
>
> avoid fixing compiler warnings because the goal is to fix the
> CAUSE of the warnings (which is usually not obvious), not just
> to make the warnings go away
>
> Does that suggest the commit author be the best people to fix
> warnings? The typical situation may be, the author is not aware of the
> warnings at all: they are buried in the tedious output of make...
It's a shame when a patch creates lots of warnings and they are
ignored. I would suggest that the patch should not be merged. :)
We should at least bring the warnings to the attention of the
patch author. Sure, in some cases we (I) might make a patch that
the author wouldn't want and would have better solutions for.
That's OK. It happens often. It's part of how Linux development works.
--
~Randy