On Mon, Jul 26, 2021 at 11:41 PM Greg Kroah-Hartman
<
gre...@linuxfoundation.org> wrote:
>
> On Mon, Jul 26, 2021 at 11:15:52PM -0700, Bill Wendling wrote:
> > On Mon, Jul 26, 2021 at 10:27 PM Greg Kroah-Hartman
> > <
gre...@linuxfoundation.org> wrote:
> > > On Mon, Jul 26, 2021 at 01:47:33PM -0700, Nathan Chancellor wrote:
> > > > + Greg and Rafael as the maintainer and reviewer of drivers/base/module.c
> > > > respectively, drop everyone else.
> > >
> > > Odd no one cc:ed us originally, I guess they didn't want the patch ever
> > > merged? :(
Bill,
$ wget
https://lore.kernel.org/lkml/20210726201924....@google.com/raw
-O bill.patch
$ ./scripts/
get_maintainer.pl bill.patch
Nathan Chancellor <
nat...@kernel.org> (supporter:CLANG/LLVM BUILD SUPPORT)
Nick Desaulniers <
ndesau...@google.com> (supporter:CLANG/LLVM BUILD SUPPORT)
linux-...@vger.kernel.org (open list)
clang-bu...@googlegroups.com (open list:CLANG/LLVM BUILD SUPPORT)
You can use something like this shell function for small patches:
$ which kpatch
kpatch () {
patch=$1
shift
if [[ -d $patch ]]
then
echo "Directory given"
else
git send-email --cc-cmd="./scripts/
get_maintainer.pl
--norolestats $patch" $@ $patch
fi
}
$ kpatch bill.patch --to "Greg Kroah-Hartman
<
gre...@linuxfoundation.org>" --to "Rafael J. Wysocki
<
raf...@kernel.org>"
(I recommend always putting the maintainers or people you'd expect to
pick up the patches in To:.)
> > >
> > > >
> > > > Original post:
> > > >
> > > >
https://lore.kernel.org/r/20210726201924....@google.com/
> > > >
> > > > On 7/26/2021 1:19 PM, 'Bill Wendling' via Clang Built Linux wrote:
> > > > > Fix the following build warning:
> > > > >
> > > > > drivers/base/module.c:36:6: error: variable 'no_warn' set but not used [-Werror,-Wunused-but-set-variable]
> > > > > int no_warn;
> > >
> > > That's not going to be a good warning to ever have the kernel use due to
> > > how lots of hardware works (i.e. we need to do a read after a write but
> > > we can throw the read away as it does not matter).
> > >
> > >
> > > > >
> > > > > This variable is used to remove another warning, but causes a warning
> > > > > itself. Mark it as 'unused' to avoid that.
> > > > >
> > > > > Signed-off-by: Bill Wendling <
mo...@google.com>
> > > >
> > > > Even though they evaluate to the same thing, it might be worth using
> > > > "__always_unused" here because it is :)
> > >
> > > But it is not unused, the value is written into it.
> > >
> > I believe that only matters if the variable is marked "volatile".
>
> "volatile" means nothing anymore, never use it or even think about it
> again please :)
What Greg is getting at is that the use of the volatile keyword in
variable declarations is slightly frowned on by the kernel community.
It's less flexible than making accesses volatile qualified via casts.
Then you have flexibility for some accesses to be volatile (ie. not
CSE'd), and some not (ie. CSE'd), if needed.
Though just because you assign to a variable doesn't mean that the
compiler generates an access, especially if the result is unused.
This warning is all about dead stores. The cast to a volatile
qualified pointer then dereference is what guarantees the access.
https://godbolt.org/z/7K7369bGG
(To be explicit, IMO Greg's point about volatile stores is orthogonal
to discussions about dead stores.)
I think warn_unused_result should only really be used for functions
where the return value should be used 100% of the time. If there are
cases where it's ok to not check the return value, consider not using
warn_unused_result on function declarations.
That said, we have a very similar issue throughout LLVM that Bill
should recognize. In LLVM, we have pretty aggressive usage of
assertions. Rather than:
assert(someReallyLongExpression && "error message");
where that statement might wrap across multiple lines, instead it
might be clearer to write:
bool IsOk = someReallyLongExpression;
assert(IsOk && "error message");
which looks nicer but now produces -Wunused-but-set-variable on IsOk
for release builds where assertions are disabled. The common fix in
LLVM is to write:
bool IsOk = someReallyLongExpression;
assert(IsOk && "error message");
(void)IsOk;
The cast to void is technically a use that doesn't result in a dead
store. That pattern could be used in the kernel rather than
int no_warn;
no_warn = warn_unused_result_fn();
at least to avoid -Wunused-but-set-variable. Oh, looks like a curious
difference between compilers:
https://godbolt.org/z/GvznMM6o1
Filed
https://bugs.llvm.org/show_bug.cgi?id=51228. So I guess we
can't use the cast-to-void to avoid -Wunused-but-set-variable, since
that triggers -Wunused-result, at least with GCC. :( Nevermind...
Though I still think the use of warn_unused_result on
sysfs_create_link() is worth revisiting.
(Orthogonally, I wonder if C would have been more ergonomic or less to
have all functions implicitly warn_unused_result then have callers be
explicit when they didn't want a result, rather than what we have
today which is the opposite. Maybe that's a terrible idea, but
sometimes you can't tell until some volume of code has been written in
such a language. Maybe it makes hello world less pretty, but maybe it
avoids more bugs in real code.)
>
> > Would you rather the warning be turned off on some level?
>
> Which warning?
>
> The code here, as-is, is correct. We already have 1 compiler warning
> work around in place, do you want to add another one? How many can we
> stack on top of each other?
Isn't -Wunused-but-set-variable enabled only for W=1 builds?
>
> And again, why did you not cc: the maintainers of this code for this
> change? That's not good...
>
> thanks,
>
> greg k-h
--
Thanks,
~Nick Desaulniers