On Fri, Jan 10, 2020 at 9:22 AM Dan Carpenter dan.carpenter@oracle.com wrote:
On Fri, Jan 10, 2020 at 08:57:35AM +0100, Jens Wiklander wrote:
drivers/tee/amdtee/core.c 435 static int __init amdtee_driver_init(void) 436 { 437 struct amdtee *amdtee = NULL; ^^^^^^^^^^^^^ 438 struct tee_device *teedev; 439 struct tee_shm_pool *pool = ERR_PTR(-EINVAL); ^^^^^^^^^^^^^^^^^^^^^^^^
These look like they are left overs from one error label style error handling. We're trying to enable the warning about unused assignments so it would be good to get rid of these.
We're bound to have corner cases with false positives. How will those be handled?
Okay, I will remove unused assignments in the driver.
Is this really such a good idea? I had the impression we were moving in the direction of always assigning default values to variables, unless they are implicitly zero initialized.
All that does is it disables GCC and other static analysis tools from finding uninitialized error bugs. It means we won't be able to turn on the GCC warning about unused assignments. That warning finds bugs where we accidentally use the wrong variable etc. Assigning bogus unused values makes the code more difficult to read.
This is a tradeoff, tools cannot find 100% of the cases with uninitialized variables on the stack. So in the cases where tools doesn't catch this we're going to use garbage from the stack. This is boiling down to a matter of taste and policy since both approaches have pros and cons. Myself I prefer initializing locals since I think it's more safe and then I'm also guarded against the false positives about usage of uninitialized variables you get from compilers from time to time. However, I'm flexible if this is what we really want, but I don't want to change back and forth as it's error prone and annoying.
Cheers, Jens