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;
From:   Christian Schoenebeck <linux_oss@crudebyte.com>
To:     Christophe JAILLET <christophe.jaillet@wanadoo.fr>,
        Dominique Martinet <asmadeus@codewreck.org>
Cc:     Eric Van Hensbergen <ericvh@kernel.org>,
        Latchesar Ionkov <lucho@ionkov.net>,
        linux-kernel@vger.kernel.org, kernel-janitors@vger.kernel.org,
        v9fs@lists.linux.dev
Subject: Re: [PATCH] fs/9p: Fix a datatype used with V9FS_DIRECT_IO
Date:   Tue, 25 Apr 2023 11:18:37 +0200
Message-ID: <2755033.v0V8SJffbf@silver>
In-Reply-To: <ZEd8d7W6HnHE_66m@codewreck.org>
References: <80bae984fd5ca49b691bb35f2fd8f345f8bb67f1.1682405206.git.christophe.jaillet@wanadoo.fr>
 <ZEd8d7W6HnHE_66m@codewreck.org>
MIME-Version: 1.0
Content-Transfer-Encoding: 7Bit
Content-Type: text/plain; charset="us-ascii"
Precedence: bulk

On Tuesday, April 25, 2023 9:08:39 AM CEST Dominique Martinet wrote:
> Christophe JAILLET wrote on Tue, Apr 25, 2023 at 08:47:27AM +0200:
> > The commit in Fixes has introduced some "enum p9_session_flags" values
> > larger than a char.
> > Such values are stored in "v9fs_session_info->flags" which is a char only.
> > 
> > Turn it into an int so that the "enum p9_session_flags" values can fit in
> > it.
> 
> Good catch, thanks!

Indeed!

Reviewed-by: Christian Schoenebeck <linux_oss@crudebyte.com>

> I'm surprised W=1 doesn't catch this... and now I'm checking higher
> (noisy) W=, or even clang doesn't seem to print anything about e.g.
> 'v9ses->flags & V9FS_DIRECT_IO is never true' or other warnings I'd have
> expected to come up -- out of curiosity how did you find this?

Both gcc and clang only trigger an implicit conversion warning if the value of
the expression can be evaluated at compile time (i.e. all operands are
constant), then compiler realizes that the compile-time evaluated constant
value is too big for the assignment destination and triggers the warning.

However as soon as any variable is involved in the expression, like in this
code, then the final value of the expression cannot be evaluated at compile-
time. Small operands (e.g. `char` types) in the expression are auto-promoted
to `int`, hence no warning at this stage, and finally you have an assignment
with unknown `int` value.

This could certainly be improved by carrying along the information that an
expression evaluates to at least x bits at runtime (when the compiler reduces
the expression).

> Would probably be interesting to run some form of the same in our
> automation.

If there is any ATM? I als tried this issue with clang's undefined behaviour
sanitizer and with the clang static analyzer. Both did not detect it.

> 
> > Fixes: 6deffc8924b5 ("fs/9p: Add new mount modes")
> 
> (Not a problem per se: but note this commit hasn't been merged yet, so
> using commit IDs is a bit dangerous. Might want to remark this in the
> free comment section so Eric pays attention to not break that when applying)
> 
> > Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> 
> Reviewed-by: Dominique Martinet <asmadeus@codewreck.org>
> 
>