| Message ID | 20210520104343.317119-1-joerichey94@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Don't use BIT() macro in UAPI headers | expand |
On Thu, May 20, 2021 at 03:43:37AM -0700, Joe Richey wrote: > This patch series changes all UAPI uses of BIT() to just be open-coded. > However, there really should be a check for this in checkpatch.pl Wanna add that check too? > Currently, the script actually _encourages_ users to use the BIT macro > even if adding things to UAPI. How so? This is with your first patch: $ ./scripts/checkpatch.pl /tmp/bit.01 total: 0 errors, 0 warnings, 7 lines checked /tmp/bit.01 has no obvious style problems and is ready for submission. Also, in your commit messages you refer to patches with patchwork links - please use the respective upstream commit IDs instead. Grep for "Fixes:" here: Documentation/process/submitting-patches.rst for more info. Thx.
On Thu, May 20, 2021 at 4:11 AM Mark Rutland <mark.rutland@arm.com> wrote: > > Hi, > > On Thu, May 20, 2021 at 03:43:37AM -0700, Joe Richey wrote: > > From: Joe Richey <joerichey@google.com> > > > > The BIT(n) macro is used in the kernel as an alias for (1 << n). > > However, it is not defined in the UAPI headers, which means that any > > UAPI header files must be careful not to use it, or else the user > > will get a linker error. > > Beware that the common definition of BIT() (in include/vdso/bits.h) is: > > | #define BIT(nr) (UL(1) << (nr)) > > That UL() can be important if `nr` is ever greater than bits per int. > > > For example, compiling the following program: > > > > #include <sys/auxv.h> > > #include <asm/hwcap2.h> > > > > // Detect if FSGSBASE instructions are enabled > > int main() { > > unsigned long val = getauxval(AT_HWCAP2); > > return !(val & HWCAP2_FSGSBASE); > > } > > > > Results in the following likner error: > > > > /usr/bin/ld: /tmp/cceFpAdR.o: in function `main': > > gs.c:(.text+0x21): undefined reference to `BIT' > > > > This patch series changes all UAPI uses of BIT() to just be open-coded. > > In include/uapi/linux/const.h we have an equivaleint _BITUL() macro, > which I think should be used in preference of open-coding this (and is > already used in a number of uapi headers). That's a good idea. I mostly just did the open-coding for consistency with the surrounding code, but aside from kvm.h there aren't really many usages to change, so I can do it. For kvm.h, I think it might be best to leave it open-coded and move the entire file to _BITUL() at once. > > > However, there really should be a check for this in checkpatch.pl > > Currently, the script actually _encourages_ users to use the BIT macro > > even if adding things to UAPI. > > I think having something that suggests s/BIT()/_BITUL()/ under uapi > would be good. I'll just change the script to recommend _BITUL() (instead of BIT()) if the code path contains "uapi". > > Thanks, > Mark. > > > > > Running `rg "BIT\(" **/uapi/**` shows no more usage of BIT() in any > > UAPI headers. Tested by building a basic kernel. Changes are trivial. > > > > Joe Richey (6): > > x86/elf: Don't use BIT() macro in UAPI headers > > KVM: X86: Don't use BIT() macro in UAPI headers > > drivers: firmware: psci: Don't use BIT() macro in UAPI headers > > uacce: Don't use BIT() macro in UAPI headers > > media: vicodec: Don't use BIT() macro in UAPI headers > > tools headers UAPI: Sync pkt_sched.h with the kernel sources > > > > arch/x86/include/uapi/asm/hwcap2.h | 2 +- > > include/uapi/linux/kvm.h | 4 +- > > include/uapi/linux/psci.h | 2 +- > > include/uapi/linux/v4l2-controls.h | 22 ++--- > > include/uapi/misc/uacce/uacce.h | 2 +- > > tools/include/uapi/linux/kvm.h | 4 +- > > tools/include/uapi/linux/pkt_sched.h | 122 ++++++++++++++++++++++++--- > > 7 files changed, 130 insertions(+), 28 deletions(-) > > > > -- > > 2.31.1 > >
> > Currently, the script actually _encourages_ users to use the BIT macro > > even if adding things to UAPI. > > How so? Running checkpatch.pl with --strict gives: CHECK: Prefer using the BIT macro #26: FILE: arch/x86/include/uapi/asm/hwcap2.h:9: +#define HWCAP2_FSGSBASE (1 << 1) It should probably just recommend the _BITUL macro. > Also, in your commit messages you refer to patches with patchwork links > - please use the respective upstream commit IDs instead. Grep for > "Fixes:" here: Ahhhhh, I figured there was a more standard way. Will fix.
On 20/05/21 12:43, Joe Richey wrote: > From: Joe Richey <joerichey@google.com> > > The BIT(n) macro is used in the kernel as an alias for (1 << n). > However, it is not defined in the UAPI headers, which means that any > UAPI header files must be careful not to use it, or else the user > will get a linker error. For example, compiling the following program: > > #include <sys/auxv.h> > #include <asm/hwcap2.h> > > // Detect if FSGSBASE instructions are enabled > int main() { > unsigned long val = getauxval(AT_HWCAP2); > return !(val & HWCAP2_FSGSBASE); > } > > Results in the following likner error: > > /usr/bin/ld: /tmp/cceFpAdR.o: in function `main': > gs.c:(.text+0x21): undefined reference to `BIT' > > This patch series changes all UAPI uses of BIT() to just be open-coded. > However, there really should be a check for this in checkpatch.pl > Currently, the script actually _encourages_ users to use the BIT macro > even if adding things to UAPI. > > Running `rg "BIT\(" **/uapi/**` shows no more usage of BIT() in any > UAPI headers. Tested by building a basic kernel. Changes are trivial. > > Joe Richey (6): > x86/elf: Don't use BIT() macro in UAPI headers > KVM: X86: Don't use BIT() macro in UAPI headers > drivers: firmware: psci: Don't use BIT() macro in UAPI headers > uacce: Don't use BIT() macro in UAPI headers > media: vicodec: Don't use BIT() macro in UAPI headers > tools headers UAPI: Sync pkt_sched.h with the kernel sources > > arch/x86/include/uapi/asm/hwcap2.h | 2 +- > include/uapi/linux/kvm.h | 4 +- > include/uapi/linux/psci.h | 2 +- > include/uapi/linux/v4l2-controls.h | 22 ++--- > include/uapi/misc/uacce/uacce.h | 2 +- > tools/include/uapi/linux/kvm.h | 4 +- > tools/include/uapi/linux/pkt_sched.h | 122 ++++++++++++++++++++++++--- > 7 files changed, 130 insertions(+), 28 deletions(-) > Will queue the KVM one, thanks! Paolo
On Thu, May 20, 2021, Joe Richey wrote: > From: Joe Richey <joerichey@google.com> > > The BIT(n) macro is used in the kernel as an alias for (1 << n). > However, it is not defined in the UAPI headers, which means that any > UAPI header files must be careful not to use it, or else the user > will get a linker error. For example, compiling the following program: > > #include <sys/auxv.h> > #include <asm/hwcap2.h> > > // Detect if FSGSBASE instructions are enabled > int main() { > unsigned long val = getauxval(AT_HWCAP2); > return !(val & HWCAP2_FSGSBASE); > } > > Results in the following likner error: > > /usr/bin/ld: /tmp/cceFpAdR.o: in function `main': > gs.c:(.text+0x21): undefined reference to `BIT' > > This patch series changes all UAPI uses of BIT() to just be open-coded. > However, there really should be a check for this in checkpatch.pl Any reason not to provide such a patch in this series? :-) > Currently, the script actually _encourages_ users to use the BIT macro > even if adding things to UAPI. > > Running `rg "BIT\(" **/uapi/**` shows no more usage of BIT() in any > UAPI headers. Tested by building a basic kernel. Changes are trivial.
On Thu, May 20, 2021, Borislav Petkov wrote: > On Thu, May 20, 2021 at 03:43:37AM -0700, Joe Richey wrote: > > This patch series changes all UAPI uses of BIT() to just be open-coded. > > However, there really should be a check for this in checkpatch.pl > > Wanna add that check too? > > > Currently, the script actually _encourages_ users to use the BIT macro > > even if adding things to UAPI. > > How so? > > This is with your first patch: > > $ ./scripts/checkpatch.pl /tmp/bit.01 > total: 0 errors, 0 warnings, 7 lines checked > > /tmp/bit.01 has no obvious style problems and is ready for submission. > > Also, in your commit messages you refer to patches with patchwork links > - please use the respective upstream commit IDs instead. Grep for > "Fixes:" here: Gah, beat me to the punch. Stupid weird mutt sorting.
On Thu, May 20, 2021 at 04:50:11AM -0700, Joseph Richey wrote: > Running checkpatch.pl with --strict gives: Yah, people must *remember* to use --strict. > It should probably just recommend the _BITUL macro. Right. > Ahhhhh, I figured there was a more standard way. Will fix. Yeah, Fixes: is very useful, as we've come to realize. Thx.
From: Joe Richey <joerichey@google.com>
The BIT(n) macro is used in the kernel as an alias for (1 << n).
However, it is not defined in the UAPI headers, they should instead use
the _BITUL(n) macro. This patch chages all existing usages in UAPI
headers and updates ./scripts/checkpatch.pl to properly reccomend the
correct macro depending on context.
Running the below commands shows no more incorrect macro usages:
rg "BIT\(" **/uapi/**
rg "BIT_ULL\(" **/uapi/**
Tested by building a basic kernel. Changes are trivial.
I encountered this issue when compiling the following program:
#include <sys/auxv.h>
#include <asm/hwcap2.h>
// Detect if FSGSBASE instructions are enabled
int main() {
unsigned long val = getauxval(AT_HWCAP2);
return !(val & HWCAP2_FSGSBASE);
}
Resulting in the following likner error:
/usr/bin/ld: /tmp/cceFpAdR.o: in function `main':
gs.c:(.text+0x21): undefined reference to `BIT'
Changes from V1 to V2:
- Use _BITUL() macro instead of open-coding
- Fixup HWCAP2_RING3MWAIT as well
- Shorten commits and added "Fixes" per reviewer comments
- checkpatch: Broaden UAPI regex
- checkpatch: Reccomend _BITULL()/_BITUL() for UAPI headers
Joe Richey (7):
x86/elf: Use _BITUL() macro in UAPI headers
KVM: X86: Use _BITUL() macro in UAPI headers
drivers: firmware: psci: Use _BITUL() macro in UAPI headers
uacce: Use _BITUL() macro in UAPI headers
media: vicodec: Use _BITUL() macro in UAPI headers
tools headers UAPI: Sync pkt_sched.h with the kernel sources
checkpatch: suggest _BITULL() and _BITUL() for UAPI headers
arch/x86/include/uapi/asm/hwcap2.h | 6 +-
include/uapi/linux/kvm.h | 5 +-
include/uapi/linux/psci.h | 4 +-
include/uapi/linux/v4l2-controls.h | 23 ++---
include/uapi/misc/uacce/uacce.h | 3 +-
scripts/checkpatch.pl | 16 ++--
tools/include/uapi/linux/kvm.h | 5 +-
tools/include/uapi/linux/pkt_sched.h | 122 ++++++++++++++++++++++++---
8 files changed, 148 insertions(+), 36 deletions(-)
On Thu, May 20, 2021 at 03:43:37AM -0700, Joe Richey wrote: > This patch series changes all UAPI uses of BIT() to just be open-coded. > However, there really should be a check for this in checkpatch.pl > Currently, the script actually _encourages_ users to use the BIT macro > even if adding things to UAPI. Yes. In fact it should warn about BIT() in general. It is totally pointless obsfucation that doesn't even save any typing at all.
On Mon, May 24, 2021 at 12:46:26PM +0100, Christoph Hellwig wrote: > On Thu, May 20, 2021 at 03:43:37AM -0700, Joe Richey wrote: > > This patch series changes all UAPI uses of BIT() to just be open-coded. > > However, there really should be a check for this in checkpatch.pl > > Currently, the script actually _encourages_ users to use the BIT macro > > even if adding things to UAPI. > > Yes. In fact it should warn about BIT() in general. It is totally > pointless obsfucation that doesn't even save any typing at all. That's not quite true; the point is that if you use BIT() consistently, then even when you refer to bits 32 to 63 you won't accidentally introduce shifts of more than the width of int, and the definition will work equally well for assembly and C, which isn't true if you use `1UL` in the definition. With that in mind it's shorter and clearer than its functional equivalent: BIT(x) (UL(1) << (x)) So IMO it's preferable to use BIT() generally, or _BITUL() in uapi headers. Thanks, Mark.
From: Mark Rutland > Sent: 24 May 2021 13:29 > > On Mon, May 24, 2021 at 12:46:26PM +0100, Christoph Hellwig wrote: > > On Thu, May 20, 2021 at 03:43:37AM -0700, Joe Richey wrote: > > > This patch series changes all UAPI uses of BIT() to just be open-coded. > > > However, there really should be a check for this in checkpatch.pl > > > Currently, the script actually _encourages_ users to use the BIT macro > > > even if adding things to UAPI. > > > > Yes. In fact it should warn about BIT() in general. It is totally > > pointless obsfucation that doesn't even save any typing at all. > > That's not quite true; the point is that if you use BIT() consistently, > then even when you refer to bits 32 to 63 you won't accidentally > introduce shifts of more than the width of int, and the definition will > work equally well for assembly and C, which isn't true if you use `1UL` > in the definition. > > With that in mind it's shorter and clearer than its functional > equivalent: > > BIT(x) > (UL(1) << (x)) > > So IMO it's preferable to use BIT() generally, or _BITUL() in uapi > headers. And then, suddenly the compiler warns about truncation of the high bits when ~BIT(x) is used to mask a 32bit value on 64bit systems. Once the C standard committee had decided to change from K&R's 'sign preserving' integer promotions to 'value preserving' you always lose somewhere. Personally I prefer hex constants - I can't count bits at all. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
From: Joe Richey <joerichey@google.com> The BIT(n) macro is used in the kernel as an alias for (1 << n). However, it is not defined in the UAPI headers, which means that any UAPI header files must be careful not to use it, or else the user will get a linker error. For example, compiling the following program: #include <sys/auxv.h> #include <asm/hwcap2.h> // Detect if FSGSBASE instructions are enabled int main() { unsigned long val = getauxval(AT_HWCAP2); return !(val & HWCAP2_FSGSBASE); } Results in the following likner error: /usr/bin/ld: /tmp/cceFpAdR.o: in function `main': gs.c:(.text+0x21): undefined reference to `BIT' This patch series changes all UAPI uses of BIT() to just be open-coded. However, there really should be a check for this in checkpatch.pl Currently, the script actually _encourages_ users to use the BIT macro even if adding things to UAPI. Running `rg "BIT\(" **/uapi/**` shows no more usage of BIT() in any UAPI headers. Tested by building a basic kernel. Changes are trivial. Joe Richey (6): x86/elf: Don't use BIT() macro in UAPI headers KVM: X86: Don't use BIT() macro in UAPI headers drivers: firmware: psci: Don't use BIT() macro in UAPI headers uacce: Don't use BIT() macro in UAPI headers media: vicodec: Don't use BIT() macro in UAPI headers tools headers UAPI: Sync pkt_sched.h with the kernel sources arch/x86/include/uapi/asm/hwcap2.h | 2 +- include/uapi/linux/kvm.h | 4 +- include/uapi/linux/psci.h | 2 +- include/uapi/linux/v4l2-controls.h | 22 ++--- include/uapi/misc/uacce/uacce.h | 2 +- tools/include/uapi/linux/kvm.h | 4 +- tools/include/uapi/linux/pkt_sched.h | 122 ++++++++++++++++++++++++--- 7 files changed, 130 insertions(+), 28 deletions(-)