Although not documented, is_signed_type() must support the 'bool' and
pointer types next to scalar and enumeration types. Add a selftest that
verifies that this macro handles all supported types correctly.
Cc: Andrew Morton <[email protected]>
Cc: Arnd Bergmann <[email protected]>
Cc: Dan Williams <[email protected]>
Cc: Eric Dumazet <[email protected]>
Cc: Ingo Molnar <[email protected]>
Cc: Isabella Basso <[email protected]>
Cc: "Jason A. Donenfeld" <[email protected]>
Cc: Josh Poimboeuf <[email protected]>
Cc: Luc Van Oostenryck <[email protected]>
Cc: Masami Hiramatsu <[email protected]>
Cc: Nathan Chancellor <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Rasmus Villemoes <[email protected]>
Cc: Sander Vanheule <[email protected]>
Cc: Steven Rostedt <[email protected]>
Cc: Vlastimil Babka <[email protected]>
Cc: Yury Norov <[email protected]>
Signed-off-by: Bart Van Assche <[email protected]>
---
lib/Kconfig.debug | 12 ++++++++++
lib/Makefile | 1 +
lib/is_signed_type_test.c | 48 +++++++++++++++++++++++++++++++++++++++
3 files changed, 61 insertions(+)
create mode 100644 lib/is_signed_type_test.c
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index 072e4b289c13..36455953d306 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -2506,6 +2506,18 @@ config MEMCPY_KUNIT_TEST
If unsure, say N.
+config IS_SIGNED_TYPE_KUNIT_TEST
+ tristate "Test is_signed_type() macro" if !KUNIT_ALL_TESTS
+ depends on KUNIT
+ default KUNIT_ALL_TESTS
+ help
+ Builds unit tests for the is_signed_type() macro.
+
+ For more information on KUnit and unit tests in general please refer
+ to the KUnit documentation in Documentation/dev-tools/kunit/.
+
+ If unsure, say N.
+
config OVERFLOW_KUNIT_TEST
tristate "Test check_*_overflow() functions at runtime" if !KUNIT_ALL_TESTS
depends on KUNIT
diff --git a/lib/Makefile b/lib/Makefile
index 5927d7fa0806..70176ff17023 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -377,6 +377,7 @@ obj-$(CONFIG_BITS_TEST) += test_bits.o
obj-$(CONFIG_CMDLINE_KUNIT_TEST) += cmdline_kunit.o
obj-$(CONFIG_SLUB_KUNIT_TEST) += slub_kunit.o
obj-$(CONFIG_MEMCPY_KUNIT_TEST) += memcpy_kunit.o
+obj-$(CONFIG_IS_SIGNED_TYPE_KUNIT_TEST) += is_signed_type_test.o
obj-$(CONFIG_OVERFLOW_KUNIT_TEST) += overflow_kunit.o
CFLAGS_stackinit_kunit.o += $(call cc-disable-warning, switch-unreachable)
obj-$(CONFIG_STACKINIT_KUNIT_TEST) += stackinit_kunit.o
diff --git a/lib/is_signed_type_test.c b/lib/is_signed_type_test.c
new file mode 100644
index 000000000000..f2eedb1f0935
--- /dev/null
+++ b/lib/is_signed_type_test.c
@@ -0,0 +1,48 @@
+// SPDX-License-Identifier: GPL-2.0 OR MIT
+/*
+ * ./tools/testing/kunit/kunit.py run is_signed_type [--raw_output]
+ */
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <kunit/test.h>
+#include <linux/overflow.h>
+
+enum unsigned_enum {
+ constant_a = 3,
+};
+
+enum signed_enum {
+ constant_b = -1,
+ constant_c = 2,
+};
+
+static void is_signed_type_test(struct kunit *test)
+{
+ KUNIT_EXPECT_EQ(test, is_signed_type(bool), false);
+ KUNIT_EXPECT_EQ(test, is_signed_type(signed char), true);
+ KUNIT_EXPECT_EQ(test, is_signed_type(unsigned char), false);
+ KUNIT_EXPECT_EQ(test, is_signed_type(int), true);
+ KUNIT_EXPECT_EQ(test, is_signed_type(unsigned int), false);
+ KUNIT_EXPECT_EQ(test, is_signed_type(long), true);
+ KUNIT_EXPECT_EQ(test, is_signed_type(unsigned long), false);
+ KUNIT_EXPECT_EQ(test, is_signed_type(long long), true);
+ KUNIT_EXPECT_EQ(test, is_signed_type(unsigned long long), false);
+ KUNIT_EXPECT_EQ(test, is_signed_type(enum unsigned_enum), false);
+ KUNIT_EXPECT_EQ(test, is_signed_type(enum signed_enum), true);
+ KUNIT_EXPECT_EQ(test, is_signed_type(void *), false);
+ KUNIT_EXPECT_EQ(test, is_signed_type(const char *), false);
+}
+
+static struct kunit_case is_signed_type_test_cases[] = {
+ KUNIT_CASE(is_signed_type_test),
+ {}
+};
+
+static struct kunit_suite is_signed_type_test_suite = {
+ .name = "is_signed_type",
+ .test_cases = is_signed_type_test_cases,
+};
+
+kunit_test_suite(is_signed_type_test_suite);
+
+MODULE_LICENSE("Dual MIT/GPL");
On Fri, Aug 26, 2022 at 09:21:15AM -0700, Bart Van Assche wrote:
> Although not documented, is_signed_type() must support the 'bool' and
> pointer types next to scalar and enumeration types. Add a selftest that
> verifies that this macro handles all supported types correctly.
>
> Cc: Andrew Morton <[email protected]>
> Cc: Arnd Bergmann <[email protected]>
> Cc: Dan Williams <[email protected]>
> Cc: Eric Dumazet <[email protected]>
> Cc: Ingo Molnar <[email protected]>
> Cc: Isabella Basso <[email protected]>
> Cc: "Jason A. Donenfeld" <[email protected]>
> Cc: Josh Poimboeuf <[email protected]>
> Cc: Luc Van Oostenryck <[email protected]>
> Cc: Masami Hiramatsu <[email protected]>
> Cc: Nathan Chancellor <[email protected]>
> Cc: Peter Zijlstra <[email protected]>
> Cc: Rasmus Villemoes <[email protected]>
> Cc: Sander Vanheule <[email protected]>
> Cc: Steven Rostedt <[email protected]>
> Cc: Vlastimil Babka <[email protected]>
> Cc: Yury Norov <[email protected]>
> Signed-off-by: Bart Van Assche <[email protected]>
Reviewed-by: Kees Cook <[email protected]>
--
Kees Cook
Hi, Bart,
Glad to see some more KUnit tests making it inside of the kernel’s lib folder :).
> Am 26/08/2022 um 1:21 PM schrieb Bart Van Assche <[email protected]>:
>
> Although not documented, is_signed_type() must support the 'bool' and
> pointer types next to scalar and enumeration types. Add a selftest that
> verifies that this macro handles all supported types correctly.
>
> Cc: Andrew Morton <[email protected]>
> Cc: Arnd Bergmann <[email protected]>
> Cc: Dan Williams <[email protected]>
> Cc: Eric Dumazet <[email protected]>
> Cc: Ingo Molnar <[email protected]>
> Cc: Isabella Basso <[email protected]>
> Cc: "Jason A. Donenfeld" <[email protected]>
> Cc: Josh Poimboeuf <[email protected]>
> Cc: Luc Van Oostenryck <[email protected]>
> Cc: Masami Hiramatsu <[email protected]>
> Cc: Nathan Chancellor <[email protected]>
> Cc: Peter Zijlstra <[email protected]>
> Cc: Rasmus Villemoes <[email protected]>
> Cc: Sander Vanheule <[email protected]>
> Cc: Steven Rostedt <[email protected]>
> Cc: Vlastimil Babka <[email protected]>
> Cc: Yury Norov <[email protected]>
> Signed-off-by: Bart Van Assche <[email protected]>
> ---
> lib/Kconfig.debug | 12 ++++++++++
> lib/Makefile | 1 +
> lib/is_signed_type_test.c | 48 +++++++++++++++++++++++++++++++++++++++
> 3 files changed, 61 insertions(+)
> create mode 100644 lib/is_signed_type_test.c
>
> diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
> index 072e4b289c13..36455953d306 100644
> --- a/lib/Kconfig.debug
> +++ b/lib/Kconfig.debug
> @@ -2506,6 +2506,18 @@ config MEMCPY_KUNIT_TEST
>
> If unsure, say N.
>
> +config IS_SIGNED_TYPE_KUNIT_TEST
> + tristate "Test is_signed_type() macro" if !KUNIT_ALL_TESTS
> + depends on KUNIT
> + default KUNIT_ALL_TESTS
> + help
> + Builds unit tests for the is_signed_type() macro.
> +
> + For more information on KUnit and unit tests in general please refer
> + to the KUnit documentation in Documentation/dev-tools/kunit/.
> +
> + If unsure, say N.
> +
> config OVERFLOW_KUNIT_TEST
> tristate "Test check_*_overflow() functions at runtime" if !KUNIT_ALL_TESTS
> depends on KUNIT
> diff --git a/lib/Makefile b/lib/Makefile
> index 5927d7fa0806..70176ff17023 100644
> --- a/lib/Makefile
> +++ b/lib/Makefile
> @@ -377,6 +377,7 @@ obj-$(CONFIG_BITS_TEST) += test_bits.o
> obj-$(CONFIG_CMDLINE_KUNIT_TEST) += cmdline_kunit.o
> obj-$(CONFIG_SLUB_KUNIT_TEST) += slub_kunit.o
> obj-$(CONFIG_MEMCPY_KUNIT_TEST) += memcpy_kunit.o
> +obj-$(CONFIG_IS_SIGNED_TYPE_KUNIT_TEST) += is_signed_type_test.o
> obj-$(CONFIG_OVERFLOW_KUNIT_TEST) += overflow_kunit.o
> CFLAGS_stackinit_kunit.o += $(call cc-disable-warning, switch-unreachable)
> obj-$(CONFIG_STACKINIT_KUNIT_TEST) += stackinit_kunit.o
> diff --git a/lib/is_signed_type_test.c b/lib/is_signed_type_test.c
> new file mode 100644
> index 000000000000..f2eedb1f0935
> --- /dev/null
> +++ b/lib/is_signed_type_test.c
> @@ -0,0 +1,48 @@
> +// SPDX-License-Identifier: GPL-2.0 OR MIT
> +/*
> + * ./tools/testing/kunit/kunit.py run is_signed_type [--raw_output]
> + */
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +
> +#include <kunit/test.h>
> +#include <linux/overflow.h>
Nit: I don’t know if that makes a huge difference but you might include
`<linux/compiler.h>` directly to make the final object smaller. Of course, that
would ideally be a change happening in 2/2 but that was already merged :).
> +
> +enum unsigned_enum {
> + constant_a = 3,
> +};
> +
> +enum signed_enum {
> + constant_b = -1,
> + constant_c = 2,
> +};
> +
> +static void is_signed_type_test(struct kunit *test)
> +{
> + KUNIT_EXPECT_EQ(test, is_signed_type(bool), false);
> + KUNIT_EXPECT_EQ(test, is_signed_type(signed char), true);
> + KUNIT_EXPECT_EQ(test, is_signed_type(unsigned char), false);
> + KUNIT_EXPECT_EQ(test, is_signed_type(int), true);
> + KUNIT_EXPECT_EQ(test, is_signed_type(unsigned int), false);
> + KUNIT_EXPECT_EQ(test, is_signed_type(long), true);
> + KUNIT_EXPECT_EQ(test, is_signed_type(unsigned long), false);
> + KUNIT_EXPECT_EQ(test, is_signed_type(long long), true);
> + KUNIT_EXPECT_EQ(test, is_signed_type(unsigned long long), false);
> + KUNIT_EXPECT_EQ(test, is_signed_type(enum unsigned_enum), false);
> + KUNIT_EXPECT_EQ(test, is_signed_type(enum signed_enum), true);
> + KUNIT_EXPECT_EQ(test, is_signed_type(void *), false);
> + KUNIT_EXPECT_EQ(test, is_signed_type(const char *), false);
> +}
> +
> +static struct kunit_case is_signed_type_test_cases[] = {
> + KUNIT_CASE(is_signed_type_test),
> + {}
> +};
> +
> +static struct kunit_suite is_signed_type_test_suite = {
> + .name = "is_signed_type",
> + .test_cases = is_signed_type_test_cases,
> +};
> +
> +kunit_test_suite(is_signed_type_test_suite);
> +
> +MODULE_LICENSE("Dual MIT/GPL“);
Tested-by: Isabella Basso <[email protected]>
Cheers,
--
Isabella Basso
On 8/29/22 19:33, Isabella Basso wrote:
>> +#include <kunit/test.h>
>> +#include <linux/overflow.h>
>
> Nit: I don’t know if that makes a huge difference but you might include
> `<linux/compiler.h>` directly to make the final object smaller. Of course, that
> would ideally be a change happening in 2/2 but that was already merged :).
Right, that could have been done in patch 2/2 but I think this can also
be done as a follow-up patch. I'm not sure what Kees prefers?
Thanks,
Bart.
On 26/08/2022 18.21, Bart Van Assche wrote:
> Although not documented, is_signed_type() must support the 'bool' and
> pointer types next to scalar and enumeration types. Add a selftest that
> verifies that this macro handles all supported types correctly.
>
> +static void is_signed_type_test(struct kunit *test)
> +{
> + KUNIT_EXPECT_EQ(test, is_signed_type(bool), false);
> + KUNIT_EXPECT_EQ(test, is_signed_type(signed char), true);
> + KUNIT_EXPECT_EQ(test, is_signed_type(unsigned char), false);
Nice. You could consider adding
#ifdef __UNSIGNED_CHAR__
KUNIT_EXPECT_EQ(test, is_signed_type(char), false);
#else
KUNIT_EXPECT_EQ(test, is_signed_type(char), true);
#endif
The kernel depends on the compiler providing __UNSIGNED_CHAR__ in two
places (one in ext4, one in printf test suite).
> + KUNIT_EXPECT_EQ(test, is_signed_type(int), true);
> + KUNIT_EXPECT_EQ(test, is_signed_type(unsigned int), false);
> + KUNIT_EXPECT_EQ(test, is_signed_type(long), true);
> + KUNIT_EXPECT_EQ(test, is_signed_type(unsigned long), false);
> + KUNIT_EXPECT_EQ(test, is_signed_type(long long), true);
> + KUNIT_EXPECT_EQ(test, is_signed_type(unsigned long long), false);
> + KUNIT_EXPECT_EQ(test, is_signed_type(enum unsigned_enum), false);
> + KUNIT_EXPECT_EQ(test, is_signed_type(enum signed_enum), true);
Yeah. But enum types are one of the weirdest aspects of C. Taking your
example and expanding with a positive value that doesn't fit an int:
#include <stdio.h>
#define is_signed_type(t) ((t)-1 < (t)1)
#define typeinfo(t) printf("%-24s signed %d, size %zu\n", #t,
is_signed_type(t), sizeof(t))
enum unsigned_enum {
constant_a = 3,
constant_d = 3000000000,
};
enum signed_enum {
constant_b = -1,
constant_c = 2,
};
int main(int argc, char *argv[])
{
enum unsigned_enum a = constant_a;
enum unsigned_enum d = constant_d;
enum signed_enum b = constant_b;
enum signed_enum c = constant_c;
typeinfo(enum unsigned_enum);
typeinfo(enum signed_enum);
typeinfo(typeof(constant_a));
typeinfo(typeof(constant_b));
typeinfo(typeof(constant_c));
typeinfo(typeof(constant_d));
typeinfo(typeof(a));
typeinfo(typeof(b));
typeinfo(typeof(c));
typeinfo(typeof(d));
return 0;
}
This gives me
enum unsigned_enum signed 0, size 4
enum signed_enum signed 1, size 4
typeof(constant_a) signed 1, size 4
typeof(constant_b) signed 1, size 4
typeof(constant_c) signed 1, size 4
typeof(constant_d) signed 0, size 4
typeof(a) signed 0, size 4
typeof(b) signed 1, size 4
typeof(c) signed 1, size 4
typeof(d) signed 0, size 4
That is, typeof(constant_a) is not the same type (different signedness)
as enum unsigned_enum! While both constant_d (due to its size) and
variables declared as 'enum unsigned_enum' do indeed have that
underlying unsigned type.
At least gcc and clang agree on this weirdness, but I haven't been able
to find a spec mandating this. Anyway, this was just an aside.
Acked-by: Rasmus Villemoes <[email protected]>
On Mon, Aug 29, 2022 at 08:30:54PM -0700, Bart Van Assche wrote:
> On 8/29/22 19:33, Isabella Basso wrote:
> > > +#include <kunit/test.h>
> > > +#include <linux/overflow.h>
> >
> > Nit: I don’t know if that makes a huge difference but you might include
> > `<linux/compiler.h>` directly to make the final object smaller. Of course, that
> > would ideally be a change happening in 2/2 but that was already merged :).
>
> Right, that could have been done in patch 2/2 but I think this can also be
> done as a follow-up patch. I'm not sure what Kees prefers?
A follow-up would be easier for me. And perhaps could include Rasmus's
suggestions too?
--
Kees Cook
On 8/30/22 02:41, Rasmus Villemoes wrote:
> On 26/08/2022 18.21, Bart Van Assche wrote:
>> Although not documented, is_signed_type() must support the 'bool' and
>> pointer types next to scalar and enumeration types. Add a selftest that
>> verifies that this macro handles all supported types correctly.
>>
>
>> +static void is_signed_type_test(struct kunit *test)
>> +{
>> + KUNIT_EXPECT_EQ(test, is_signed_type(bool), false);
>> + KUNIT_EXPECT_EQ(test, is_signed_type(signed char), true);
>> + KUNIT_EXPECT_EQ(test, is_signed_type(unsigned char), false);
>
> Nice. You could consider adding
>
> #ifdef __UNSIGNED_CHAR__
> KUNIT_EXPECT_EQ(test, is_signed_type(char), false);
> #else
> KUNIT_EXPECT_EQ(test, is_signed_type(char), true);
> #endif
>
> The kernel depends on the compiler providing __UNSIGNED_CHAR__ in two
> places (one in ext4, one in printf test suite).
(reduced Cc-list)
Hi Rasmus,
Since I would like to implement the above suggestion I tried to look up
other uses of the __UNSIGNED_CHAR__ macro. However, I couldn't find any.
Did I perhaps do something wrong?
$ git grep -w __UNSIGNED_CHAR__ origin/master | wc
0 0 0
Thanks,
Bart.
On 07/09/2022 00.42, Bart Van Assche wrote:
> Since I would like to implement the above suggestion I tried to look up
> other uses of the __UNSIGNED_CHAR__ macro. However, I couldn't find any.
> Did I perhaps do something wrong?
> $ git grep -w __UNSIGNED_CHAR__ origin/master | wc
> 0 0 0
No, sorry, I did. It's __CHAR_UNSIGNED__ . Here's the description from
'info cpp':
'__CHAR_UNSIGNED__'
GCC defines this macro if and only if the data type 'char' is
unsigned on the target machine.
Rasmus