Re: svn commit: r250986 - head/sys/dev/usb
On 26 May 2013, at 03:45, Bruce Evans b...@optusnet.com.au wrote: Hmm, it would be useful to have a compiler flag for initializing all local variables to trap representations on entry to functions. The clang memory sanitizer does this. David ___ svn-src-head@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-head To unsubscribe, send any mail to svn-src-head-unsubscr...@freebsd.org
svn commit: r250986 - head/sys/dev/usb
Author: hselasky
Date: Sat May 25 17:09:58 2013
New Revision: 250986
URL: http://svnweb.freebsd.org/changeset/base/250986
Log:
Fix some statical clang analyzer warnings.
Modified:
head/sys/dev/usb/usb_device.c
head/sys/dev/usb/usb_hub.c
head/sys/dev/usb/usb_msctest.c
Modified: head/sys/dev/usb/usb_device.c
==
--- head/sys/dev/usb/usb_device.c Sat May 25 16:58:12 2013
(r250985)
+++ head/sys/dev/usb/usb_device.c Sat May 25 17:09:58 2013
(r250986)
@@ -799,9 +799,6 @@ usb_config_parse(struct usb_device *udev
/* find maximum number of endpoints */
if (ep_max temp)
ep_max = temp;
-
- /* optimalisation */
- id = (struct usb_interface_descriptor *)ed;
}
}
Modified: head/sys/dev/usb/usb_hub.c
==
--- head/sys/dev/usb/usb_hub.c Sat May 25 16:58:12 2013(r250985)
+++ head/sys/dev/usb/usb_hub.c Sat May 25 17:09:58 2013(r250986)
@@ -341,7 +341,6 @@ uhub_reattach_port(struct uhub_softc *sc
DPRINTF(reattaching port %d\n, portno);
- err = 0;
timeout = 0;
udev = sc-sc_udev;
child = usb_bus_port_get_device(udev-bus,
Modified: head/sys/dev/usb/usb_msctest.c
==
--- head/sys/dev/usb/usb_msctest.c Sat May 25 16:58:12 2013
(r250985)
+++ head/sys/dev/usb/usb_msctest.c Sat May 25 17:09:58 2013
(r250986)
@@ -802,7 +802,6 @@ usb_msc_eject(struct usb_device *udev, u
if (sc == NULL)
return (USB_ERR_INVAL);
- err = 0;
switch (method) {
case MSC_EJECT_STOPUNIT:
err = bbb_command_start(sc, DIR_IN, 0, NULL, 0,
@@ -844,6 +843,7 @@ usb_msc_eject(struct usb_device *udev, u
break;
default:
DPRINTF(Unknown eject method (%d)\n, method);
+ err = 0;
break;
}
DPRINTF(Eject CD command status: %s\n, usbd_errstr(err));
___
svn-src-head@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to svn-src-head-unsubscr...@freebsd.org
Re: svn commit: r250986 - head/sys/dev/usb
On Sat, May 25, 2013 at 10:09 AM, Hans Petter Selasky
hsela...@freebsd.orgwrote:
Author: hselasky
Date: Sat May 25 17:09:58 2013
New Revision: 250986
URL: http://svnweb.freebsd.org/changeset/base/250986
Log:
Fix some statical clang analyzer warnings.
Modified:
head/sys/dev/usb/usb_device.c
head/sys/dev/usb/usb_hub.c
head/sys/dev/usb/usb_msctest.c
Modified: head/sys/dev/usb/usb_device.c
==
--- head/sys/dev/usb/usb_device.c Sat May 25 16:58:12 2013
(r250985)
+++ head/sys/dev/usb/usb_device.c Sat May 25 17:09:58 2013
(r250986)
@@ -799,9 +799,6 @@ usb_config_parse(struct usb_device *udev
/* find maximum number of endpoints */
if (ep_max temp)
ep_max = temp;
-
- /* optimalisation */
- id = (struct usb_interface_descriptor *)ed;
}
}
Modified: head/sys/dev/usb/usb_hub.c
==
--- head/sys/dev/usb/usb_hub.c Sat May 25 16:58:12 2013(r250985)
+++ head/sys/dev/usb/usb_hub.c Sat May 25 17:09:58 2013(r250986)
@@ -341,7 +341,6 @@ uhub_reattach_port(struct uhub_softc *sc
DPRINTF(reattaching port %d\n, portno);
- err = 0;
timeout = 0;
udev = sc-sc_udev;
child = usb_bus_port_get_device(udev-bus,
Modified: head/sys/dev/usb/usb_msctest.c
==
--- head/sys/dev/usb/usb_msctest.c Sat May 25 16:58:12 2013
(r250985)
+++ head/sys/dev/usb/usb_msctest.c Sat May 25 17:09:58 2013
(r250986)
@@ -802,7 +802,6 @@ usb_msc_eject(struct usb_device *udev, u
if (sc == NULL)
return (USB_ERR_INVAL);
- err = 0;
switch (method) {
case MSC_EJECT_STOPUNIT:
err = bbb_command_start(sc, DIR_IN, 0, NULL, 0,
@@ -844,6 +843,7 @@ usb_msc_eject(struct usb_device *udev, u
break;
default:
DPRINTF(Unknown eject method (%d)\n, method);
+ err = 0;
break;
}
DPRINTF(Eject CD command status: %s\n, usbd_errstr(err));
I don't know about the top one, but the bottom two are the safer way to
code, and should not have been changed. Unless we feel guaranteed the
compiler can detect all uninitialized use and will break the build, an
early initialization to a sane value is absolutely the right thing to do,
even if it will be overwritten. If the compiler feels sure the
initialization isn't needed, it does not have to emit the code. But any
coding change after the (missing) initialization can create a bug now
(well, it depends on how the code is structured, but from the three lines
of context svn diff provides it's not clear a bug couldn't easily be
introduced).
Thanks,
matthew
___
svn-src-head@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to svn-src-head-unsubscr...@freebsd.org
Re: svn commit: r250986 - head/sys/dev/usb
On 05/25/13 20:03, m...@freebsd.org wrote:
On Sat, May 25, 2013 at 10:09 AM, Hans Petter Selasky
hsela...@freebsd.orgwrote:
Author: hselasky
Date: Sat May 25 17:09:58 2013
New Revision: 250986
URL: http://svnweb.freebsd.org/changeset/base/250986
Log:
Fix some statical clang analyzer warnings.
Modified:
head/sys/dev/usb/usb_device.c
head/sys/dev/usb/usb_hub.c
head/sys/dev/usb/usb_msctest.c
Modified: head/sys/dev/usb/usb_device.c
==
--- head/sys/dev/usb/usb_device.c Sat May 25 16:58:12 2013
(r250985)
+++ head/sys/dev/usb/usb_device.c Sat May 25 17:09:58 2013
(r250986)
@@ -799,9 +799,6 @@ usb_config_parse(struct usb_device *udev
/* find maximum number of endpoints */
if (ep_max temp)
ep_max = temp;
-
- /* optimalisation */
- id = (struct usb_interface_descriptor *)ed;
}
}
Modified: head/sys/dev/usb/usb_hub.c
==
--- head/sys/dev/usb/usb_hub.c Sat May 25 16:58:12 2013(r250985)
+++ head/sys/dev/usb/usb_hub.c Sat May 25 17:09:58 2013(r250986)
@@ -341,7 +341,6 @@ uhub_reattach_port(struct uhub_softc *sc
DPRINTF(reattaching port %d\n, portno);
- err = 0;
timeout = 0;
udev = sc-sc_udev;
child = usb_bus_port_get_device(udev-bus,
Modified: head/sys/dev/usb/usb_msctest.c
==
--- head/sys/dev/usb/usb_msctest.c Sat May 25 16:58:12 2013
(r250985)
+++ head/sys/dev/usb/usb_msctest.c Sat May 25 17:09:58 2013
(r250986)
@@ -802,7 +802,6 @@ usb_msc_eject(struct usb_device *udev, u
if (sc == NULL)
return (USB_ERR_INVAL);
- err = 0;
switch (method) {
case MSC_EJECT_STOPUNIT:
err = bbb_command_start(sc, DIR_IN, 0, NULL, 0,
@@ -844,6 +843,7 @@ usb_msc_eject(struct usb_device *udev, u
break;
default:
DPRINTF(Unknown eject method (%d)\n, method);
+ err = 0;
break;
}
DPRINTF(Eject CD command status: %s\n, usbd_errstr(err));
I don't know about the top one, but the bottom two are the safer way to
code, and should not have been changed. Unless we feel guaranteed the
compiler can detect all uninitialized use and will break the build, an
early initialization to a sane value is absolutely the right thing to do,
even if it will be overwritten. If the compiler feels sure the
initialization isn't needed, it does not have to emit the code. But any
coding change after the (missing) initialization can create a bug now
(well, it depends on how the code is structured, but from the three lines
of context svn diff provides it's not clear a bug couldn't easily be
introduced).
Hi,
The last case is a switch case, and err must be set in all cases.
In the second case, err = was used two times in a row.
First case is OK. It is leftover code after some earlier patches.
Thanks for the review!
--HPS
___
svn-src-head@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to svn-src-head-unsubscr...@freebsd.org
Re: svn commit: r250986 - head/sys/dev/usb
On Sat, 25 May 2013 m...@freebsd.org wrote:
On Sat, May 25, 2013 at 10:09 AM, Hans Petter Selasky
hsela...@freebsd.orgwrote:
...
Log:
Fix some statical clang analyzer warnings.
...
==
--- head/sys/dev/usb/usb_msctest.c Sat May 25 16:58:12 2013
(r250985)
+++ head/sys/dev/usb/usb_msctest.c Sat May 25 17:09:58 2013
(r250986)
@@ -802,7 +802,6 @@ usb_msc_eject(struct usb_device *udev, u
if (sc == NULL)
return (USB_ERR_INVAL);
- err = 0;
switch (method) {
case MSC_EJECT_STOPUNIT:
err = bbb_command_start(sc, DIR_IN, 0, NULL, 0,
@@ -844,6 +843,7 @@ usb_msc_eject(struct usb_device *udev, u
break;
default:
DPRINTF(Unknown eject method (%d)\n, method);
+ err = 0;
break;
}
DPRINTF(Eject CD command status: %s\n, usbd_errstr(err));
I don't know about the top one, but the bottom two are the safer way to
code, and should not have been changed. Unless we feel guaranteed the
compiler can detect all uninitialized use and will break the build, an
early initialization to a sane value is absolutely the right thing to do,
even if it will be overwritten. If the compiler feels sure the
initialization isn't needed, it does not have to emit the code. But any
coding change after the (missing) initialization can create a bug now
(well, it depends on how the code is structured, but from the three lines
of context svn diff provides it's not clear a bug couldn't easily be
introduced).
No, initializing a variable early to a sane value obfuscates the code
and makes it impossible for the compiler to detect that the variable
is not properly intialized. Initializing early to an insane value
that will be overwritten in all cases is not so bad. This makes it
clear to human readers that the initial value should not be used, and
gives runtime errors if it is used (best an immediate trap), but still
prevents the compiler detecting that the variable is not properly
initialized.
Hmm, it would be useful to have a compiler flag for initializing all
local variables to trap representations on entry to functions. This
gives the runtime check in addition to the compiler check, without
writing huge code to initialize all the variables. Then early
initialization would break to sane values would break this feature.
Of course, in practical code, you often reuse a variable without
uninitializing it (by setting it to an insane value) after each of its
uses becomes dead. Then you lose the compiler check. If the uses are
unrelated, then it is a style bug (optimization for 30 year old
compilers with no lifetime analysis) to use the same variable for them
all. Otherwise, it is too painful to uninitialize variables or use
block scope for them to make their lifetimes more obvious to all readers.
One exception is when pointer variables are set to NULL after they are
freed even when the pointers are not expected to be reused. This is done
mainly for non-local variables.
Bruce
___
svn-src-head@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to svn-src-head-unsubscr...@freebsd.org
