| Message ID | 80bae984fd5ca49b691bb35f2fd8f345f8bb67f1.1682405206.git.christophe.jaillet@wanadoo.fr (mailing list archive) |
|---|---|
| State | Awaiting Upstream |
| Headers | show |
| Series | fs/9p: Fix a datatype used with V9FS_DIRECT_IO | expand |
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 <christophe.jaillet@wanadoo.fr> Reviewed-by: Dominique Martinet <asmadeus@codewreck.org>
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> > >
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.
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 <asmadeus@codewreck.org> 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)
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;
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 <christophe.jaillet@wanadoo.fr> --- Un-tested --- fs/9p/v9fs.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)