svn commit: r250986 - head/sys/dev/usb

[Bruce Evans]

On Sat, 25 May 2013 mdf at FreeBSD.org wrote:

> On Sat, May 25, 2013 at 10:09 AM, Hans Petter Selasky
> <hselasky at freebsd.org>wrote:
>> ...
>> 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