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.
Fixes: 6deffc8924b5 ("fs/9p: Add new mount modes")
Signed-off-by: Christophe JAILLET <[email protected]>
---
Un-tested
---
fs/9p/v9fs.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/fs/9p/v9fs.h b/fs/9p/v9fs.h
index 06a2514f0d88..698c43dd5dc8 100644
--- a/fs/9p/v9fs.h
+++ b/fs/9p/v9fs.h
@@ -108,7 +108,7 @@ enum p9_cache_bits {
struct v9fs_session_info {
/* options */
- unsigned char flags;
+ unsigned int flags;
unsigned char nodev;
unsigned short debug;
unsigned int afid;
--
2.34.1
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!
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?
Would probably be interesting to run some form of the same in our
automation.
> 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 <[email protected]>
Reviewed-by: Dominique Martinet <[email protected]>
--
Dominique
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 <[email protected]>
> 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 <[email protected]>
>
> Reviewed-by: Dominique Martinet <[email protected]>
>
>
Christian Schoenebeck wrote on Tue, Apr 25, 2023 at 11:18:37AM +0200:
> > 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.
Right, `v9ses->flags = V9FS_DIRECT_IO` would have triggered it but not
with `|=` -- but in this case I was also expecting the check
`v9ses->flags & V9fs_DIRECT_IO` to flag something odd...
But nothing seems to care; testing with this snippet:
---
int foo(char x) {
if (x & 0x200)
return 1;
return 0;
}
int foo2(unsigned char x) {
if (x < 0)
return 1;
return 0;
}
---
gcc warns that the x < 0 is always false (clang actually doesn't, even
with scan-build, I must be missing a flag?), but I didn't find anything
complaining about the &.
I'd expect something like coverity to perform a bit better here but it's
a pain to use the "free for open source" version (... I just requested
access to https://scan.coverity.com/projects/128 but I have no idea if
they build next or not)
Oh, well; glad Christophe noticed anyway.
> > 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.
There's at least the intel bot building with W=1 and warning if any new
such warning pops up (and I'd like to say I check myself, but I probably
forget about half the time; I looked at making W=1 default for our part
of the tree but it didn't look trivial? I'll try to have another look);
but I'm not aware of anyone testing with scan-build or something else
that'd contact us on new defects.
--
Dominique Martinet | Asmadeus
On Tuesday, April 25, 2023 12:40:22 PM CEST Dominique Martinet wrote:
> Christian Schoenebeck wrote on Tue, Apr 25, 2023 at 11:18:37AM +0200:
> > > 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.
>
> Right, `v9ses->flags = V9FS_DIRECT_IO` would have triggered it but not
> with `|=` -- but in this case I was also expecting the check
> `v9ses->flags & V9fs_DIRECT_IO` to flag something odd...
No, because before that binary expression is "reduced" by the compiler,
`v9ses->flags` is already auto-promoted from `char` to `int`. So it is
effectively:
INT_RVALUE BITWISE_AND INT_LITERAL
And not:
CHAR_LVALUE BITWISE_AND INT_LITERAL
And up to this parser state that's absolutely valid.
The compiler would only able to detect this issue if it would carry the
information (min. used bits at runtime) along over multiple reductions,
up to the parser state where it eventually reduces the assignment, which is
apparently not implemented ATM.
I am however more suprised that neither clang's sanitizer, nor static analyzer
detect this issue either.
> But nothing seems to care; testing with this snippet:
> ---
> int foo(char x) {
> if (x & 0x200)
> return 1;
> return 0;
> }
> int foo2(unsigned char x) {
> if (x < 0)
> return 1;
> return 0;
> }
> ---
> gcc warns that the x < 0 is always false (clang actually doesn't, even
> with scan-build, I must be missing a flag?), but I didn't find anything
> complaining about the &.
-Wtype-limits (or specifically -Wtautological-unsigned-zero-compare) does it.
> I'd expect something like coverity to perform a bit better here but it's
> a pain to use the "free for open source" version (... I just requested
> access to https://scan.coverity.com/projects/128 but I have no idea if
> they build next or not)
>
> Oh, well; glad Christophe noticed anyway.
>
> > > 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.
>
> There's at least the intel bot building with W=1 and warning if any new
> such warning pops up (and I'd like to say I check myself, but I probably
> forget about half the time; I looked at making W=1 default for our part
> of the tree but it didn't look trivial? I'll try to have another look);
> but I'm not aware of anyone testing with scan-build or something else
> that'd contact us on new defects.
>
>
On Tue, Apr 25, 2023 at 8:12 AM Dominique Martinet
<[email protected]> wrote:
>
> Christophe JAILLET wrote on Tue, Apr 25, 2023 at 08:47:27AM +0200:
> > 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)
This is fine.
The hash is constant unless Eric does a rebase. When a maintainer rebases
then updating the fixes tags is just part of the process. Often they end up
folding the fix into the original patch at that point so the Fixes tag is not
required. If a maintainer doesn't update the tags then the linux-next
maintainers
will notice and complain.
#GitMagic
regards,
dan carpenter
Dan Carpenter wrote on Tue, Apr 25, 2023 at 02:14:52PM +0100:
> The hash is constant unless Eric does a rebase. When a maintainer rebases
> then updating the fixes tags is just part of the process. Often they end up
> folding the fix into the original patch at that point so the Fixes tag is not
> required. If a maintainer doesn't update the tags then the linux-next
> maintainers will notice and complain.
Good to know this is checked as part of the linux-next tree checks.
> #GitMagic
This isn't magic, this is painful to update manually and easy to forget,
which is why as a maintainer I'd appreciate having a heads up here and
why I mentioned it.
(I'm sure Eric would have noticed anyway given this is fixing one of the
patchs he really wants to get in this merge window... But, well, in
general)
Re: folding into the original patch or not is also tricky as it weakens
recognition to the contributor, so I tend to keep such fixes separate
unless the tree becomes completely unusable (e.g. doesn't build) for
bisectability.
(I really, really wish there was a more mainlined maintainer process
though, so each maintainer wouldn't have to come up with their own rules
and tricks for everything... But I think that's a lost battle at this
point)
--
Dominique Martinet | Asmadeus