Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Mon, 22 May 2023 23:35:44 +0200
From:   David Sterba <dsterba@suse.cz>
To:     Qu Wenruo <quwenruo.btrfs@gmx.com>
Cc:     Anand Jain <anand.jain@oracle.com>,
        zhangshida <starzhangzsd@gmail.com>, clm@fb.com,
        josef@toxicpanda.com, dsterba@suse.com,
        linux-btrfs@vger.kernel.org, linux-kernel@vger.kernel.org,
        zhangshida@kylinos.cn, k2ci <kernel-bot@kylinos.cn>
Subject: Re: [PATCH] btrfs: fix uninitialized warning in btrfs_log_inode
Message-ID: <20230522213544.GR32559@twin.jikos.cz>
Reply-To: dsterba@suse.cz
References: <20230516013430.2712449-1-zhangshida@kylinos.cn>
 <f90eb6c1-4491-ecd2-1fdd-56580088c993@gmx.com>
 <28116d72-d3a6-540d-fea8-542dd5d5ca54@oracle.com>
 <315c0548-835c-d54b-2086-cd3823c9b7b9@gmx.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <315c0548-835c-d54b-2086-cd3823c9b7b9@gmx.com>
User-Agent: Mutt/1.5.23.1-rc1 (2014-03-12)
Precedence: bulk

On Wed, May 17, 2023 at 05:39:09PM +0800, Qu Wenruo wrote:
> On 2023/5/17 17:13, Anand Jain wrote:
> > On 17/5/23 15:46, Qu Wenruo wrote:
> >> On 2023/5/16 09:34, zhangshida wrote:
> >>> From: Shida Zhang <zhangshida@kylinos.cn>
> >>>
> >>> This fixes the following warning reported by gcc 10 under x86_64:
> >>
> >> Full gcc version please.
> >> Especially you need to check if your gcc10 is the latest release.
> >>
> >> If newer gcc (12.2.1) tested without such error, it may very possible to
> >> be a false alert.
> >>
> >> And in fact it is.
> >
> > I also noticed that last_range_start is not actually uninitialized.
> > However, it is acceptable to initialize a variable to silence the
> > compiler if necessary, right? We have done something similar in the
> > past.
> 
> I tend not to. Uninitialized variable warning itself is a good indicator
> to let compiler help us to expose branches we didn't consider.
> 
> Without a no-brainer "int whatever = 0;" we may even hit bugs that some
> corner cases may even use that initialized zero, but we didn't even
> expect to go.

We're in a situation that is not perfect, we got several reports that
were from old compilers but we know most of them were false positives. On
the other hand we want to enable more warnings that make sense to us
(because we can fix/work around them) but can't be yet enabled globally
for linux.

I take the reports or patches as the price for that, so far the number
hasn't been anything near unmanageable. I evaluate each fix in the
context of the code regardless of the compiler, i.e. the code logic must
be correct so it's not a 'no-brainer initialize to 0'. If 0 is the sane
default or detectable invalid value then yes, of course. It might need
some more code restructuring but I don't remember or have an example of
that. Usually just initializing the variable was sufficient.

We cannot expect that everybody has the most up to date version of the
compiler, the minimum currently required for gcc is 5.1
(Documentation/process/changes.rst). I take it as this is the minimum
and anything from that is relevant for reports and fixes. This is what
provides testing coverage, build and runtime.

You can simply skip the warning fix patches, I need to look at them
anyway and I don't mind. I might ask you for an opinion if the suggested
fix is correct in case it's the code I know you do understand well.