> --- linux/drivers/net/ppp_generic.c.orig Sat Aug 3 17:13:58 2002
> +++ linux/drivers/net/ppp_generic.c Sat Aug 3 19:11:54 2002
> @@ -378,7 +378,7 @@
> {
> struct ppp_file *pf = file->private_data;
> DECLARE_WAITQUEUE(wait, current);
> - ssize_t ret;
> + ssize_t ret = 0; /* suppress compiler warning */
> struct sk_buff *skb = 0;
>
> if (pf == 0)
Please don't do this. I'm regularly having to fix drivers where people
hid bugs this way rather than working out if it was a real problem. If
it is genuinely a compiler corner case then let the gcc folks know and
comment it but leave the warning.
Alan Cox writes:
> > --- linux/drivers/net/ppp_generic.c.orig Sat Aug 3 17:13:58 2002
> > +++ linux/drivers/net/ppp_generic.c Sat Aug 3 19:11:54 2002
> > @@ -378,7 +378,7 @@
> > {
> > struct ppp_file *pf = file->private_data;
> > DECLARE_WAITQUEUE(wait, current);
> > - ssize_t ret;
> > + ssize_t ret = 0; /* suppress compiler warning */
> > struct sk_buff *skb = 0;
> >
> > if (pf == 0)
>
>
> Please don't do this. I'm regularly having to fix drivers where people
> hid bugs this way rather than working out if it was a real problem. If
> it is genuinely a compiler corner case then let the gcc folks know and
> comment it but leave the warning.
The code is in ppp_read actually OK; if you trace through the logic
you can prove that ret is never actually used without being set first.
Paul.
> --- linux/drivers/net/ppp_generic.c.orig Sat Aug 3 17:13:58 2002
> +++ linux/drivers/net/ppp_generic.c Sat Aug 3 19:11:54 2002
> @@ -378,7 +378,7 @@
> {
> struct ppp_file *pf = file->private_data;
> DECLARE_WAITQUEUE(wait, current);
> - ssize_t ret;
> + ssize_t ret = 0; /* suppress compiler warning */
Please don't do this. I'm regularly having to fix drivers where people
hid bugs this way rather than working out if it was a real problem. If
it is genuinely a compiler corner case then let the gcc folks know and
comment it but leave the warning.
A compiler isn't able to work out the control flow which
makes sure ret is indeed initialized on every path to
a use. Solving such a problem is traveling salesman'ish :-)
for (;;) {
...
if (skb)
break;
...
set 'ret' to something
more break statements
}
if (skb == 0)
goto out; /* where 'ret' is used' */
set 'ret' to something
See? :-)
Alan Cox <[email protected]> writes:
> > --- linux/drivers/net/ppp_generic.c.orig Sat Aug 3 17:13:58 2002
> > +++ linux/drivers/net/ppp_generic.c Sat Aug 3 19:11:54 2002
> > @@ -378,7 +378,7 @@
> > {
> > struct ppp_file *pf = file->private_data;
> > DECLARE_WAITQUEUE(wait, current);
> > - ssize_t ret;
> > + ssize_t ret = 0; /* suppress compiler warning */
> > struct sk_buff *skb = 0;
> >
> > if (pf == 0)
>
>
> Please don't do this. I'm regularly having to fix drivers where people
> hid bugs this way rather than working out if it was a real problem. If
> it is genuinely a compiler corner case then let the gcc folks know and
> comment it but leave the warning.
Ok.
--
Krzysztof Halasa
Network Administrator
Paul Mackerras <[email protected]> writes:
> > > --- linux/drivers/net/ppp_generic.c.orig Sat Aug 3 17:13:58 2002
> > > +++ linux/drivers/net/ppp_generic.c Sat Aug 3 19:11:54 2002
> > > @@ -378,7 +378,7 @@
> > > {
> > > struct ppp_file *pf = file->private_data;
> > > DECLARE_WAITQUEUE(wait, current);
> > > - ssize_t ret;
> > > + ssize_t ret = 0; /* suppress compiler warning */
> > > struct sk_buff *skb = 0;
> > >
> The code is in ppp_read actually OK; if you trace through the logic
> you can prove that ret is never actually used without being set first.
That's right - that's exactly why I wrote "suppress compiler warning".
It's just the compiler not smart enough (of course, i looked at the code
paths and I'd just fix it if it was broken).
Anyway, it seems there are more places like that in the kernel tree, so,
as Alan said, the correct thing to improve is the compiler (not even sure
if gcc3 would print the warning, I'm using RH "2.96" gcc).
--
Krzysztof Halasa
Network Administrator
In article <[email protected]>,
David S. Miller <[email protected]> wrote:
| A compiler isn't able to work out the control flow which
| makes sure ret is indeed initialized on every path to
| a use. Solving such a problem is traveling salesman'ish :-)
This is true, but on the other hand I don't see much virtue in taking
the attitude that I'm sure the compiler is wrong to avoid the overhead
of initializing the variable or clarifying the code.
I totally agree that there are cases in which the compiler can't be sure
and the programmer is based on some outside assumptions of input values
or whatever, but I don't mind making the code robust in cases other than
innermost loops where I just can't fix it.
I'd rather set the initial value to NOTSET or NULL or some value which
will clearly show if you are setting what you think you are. I've
occasionally been surprised, particularly when trusting values passed
into a procedure.
Just my take on reliable vs. fast.
--
bill davidsen <[email protected]>
CTO, TMR Associates, Inc
Doing interesting things with little computers since 1979.