Messages in this thread | | | Subject | Re: [char-misc:char-misc-linus 3/3] drivers/misc/cardreader/rtsx_usb.c:639:6: warning: variable 'ret' is used uninitialized whenever 'if' condition is true | From | Shuah Khan <> | Date | Fri, 1 Jul 2022 10:59:11 -0600 |
| |
On 7/1/22 10:27 AM, Nathan Chancellor wrote: > On Fri, Jul 01, 2022 at 10:09:45AM -0600, Shuah Khan wrote: >> On 7/1/22 9:52 AM, Shuah Khan wrote: >>> On 7/1/22 9:39 AM, Nathan Chancellor wrote: >>>> On Fri, Jul 01, 2022 at 08:48:11AM -0600, Shuah Khan wrote: >>>>> On 7/1/22 2:59 AM, Greg Kroah-Hartman wrote: >> >>>>>>> All warnings (new ones prefixed by >>): >>>>>>> >>>>>>>>> drivers/misc/cardreader/rtsx_usb.c:639:6: warning: variable 'ret' is used uninitialized whenever 'if' condition is true [-Wsometimes-uninitialized] >>>>>>> if (!ucr->rsp_buf) >>>>>>> ^~~~~~~~~~~~~ >>>>>>> drivers/misc/cardreader/rtsx_usb.c:678:9: note: uninitialized use occurs here >>>>>>> return ret; >>>>>>> ^~~ >>>>>>> drivers/misc/cardreader/rtsx_usb.c:639:2: note: remove the 'if' if its condition is always false >>>>>>> if (!ucr->rsp_buf) >>>>>>> ^~~~~~~~~~~~~~~~~~ >>>>>>> drivers/misc/cardreader/rtsx_usb.c:622:9: note: initialize the variable 'ret' to silence this warning >>>>>>> int ret; >>>>>>> ^ >>>>>>> = 0 >>>>>>> 1 warning generated. >>>>>> >>>>>> Odd, gcc doesn't show this for me. Shuah, can you send a follow-on >>>>>> patch to fix this? The warning does look correct. >>>>>> >>>>> >>>>> gcc didn't complain when I compiled either. I will send a follow-on patch. >>>> >>>> Unfortunately, GCC won't warn for most uninitialized variables by >>>> default after 5.7, which included commit 78a5255ffb6a ("Stop the ad-hoc >>>> games with -Wno-maybe-initialized"). They will potentially show up at >>>> W=2 or with an explicit KCFLAGS=-Wmaybe-uninitialized (it does in this >>>> case): >>>> >>> >>> Thank you. >>> >>>> | drivers/misc/cardreader/rtsx_usb.c: In function ‘rtsx_usb_probe’: >>>> | drivers/misc/cardreader/rtsx_usb.c:678:16: error: ‘ret’ may be used uninitialized [-Werror=maybe-uninitialized] >>>> | 678 | return ret; >>>> | | ^~~ >>>> | drivers/misc/cardreader/rtsx_usb.c:622:13: note: ‘ret’ was declared here >>>> | 622 | int ret; >>>> | | ^~~ >>>> | cc1: all warnings being treated as errors >>>> >>> >>> This is a bug and a good find. ret should have been set >>> in the if (!ucr->rsp_buf) before going to error handling. >>> >>> I wonder if it would have been flagged if ret were to be >>> initialized to 0. Something to experiment. >>> >> >> I had to try. As I suspected initializing ret will mask this bug. >> >> KCFLAGS=-Wmaybe-uninitialized will not flag it even though >> the bug still exists. It will return 0 w hen memory allocation >> fails. > > Right, if the variable is unconditonally initialized at the top of the > function, it can never be flagged as uninitialized but that doesn't mean > the value is correct for every branch within the function. >
+1
>> Initializing isn't always the right answer for these kinds of >> warnings. > > I would say "unconditional initialization", as ret has to be initialized > somewhere to fix this warning, right? If you are referring to clang's > 'ret = 0' suggestion, I agree that it can be misleading (especially in > the case of suggesting initializing dereferenced pointers to NULL..., > see [1]) but at the end of the day, it is still on the programmer to > analyze their code fully in light of the warning before implementing a > fix. Clang cannot know that the return code should be set in the if > statement rather than at the top of the function. >
+1 on "programmer to analyze their code fully in light of the warning"
I am not necessarily referring to the clang warning. I agree that it could be misleading and suggest or send us towards a wrong fix. It is on us to be careful - this would be when fixing the problem and reviewing the fix.
thanks, -- Shuah
| |