On Fri, Jan 10, 2020 at 12:11:18PM +0100, Jens Wiklander wrote:
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?
There aren't a lot of "false positives". GCC knows when an assignment is really never used.
There are some times where people think unused assignments help readability but probably we'll just silence the warning. We generally just silence GCC warnings even when GCC is wrong (like for uninitialized variables).
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.
GCC is actually broken currently with regards to uninitialized variables. It often initializes stack variables to zero and doesn't print a warning. But Smatch and other static checkers are working so we end up detecting most of the bugs I think.
regards, dan carpenter