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 <akpm@linux-foundation.org>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Dan Williams <dan.j.williams@intel.com>
Cc: Eric Dumazet <edumazet@google.com>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Isabella Basso <isabbasso@riseup.net>
Cc: "Jason A. Donenfeld" <Jason@zx2c4.com>
Cc: Josh Poimboeuf <jpoimboe@kernel.org>
Cc: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>
Cc: Masami Hiramatsu <mhiramat@kernel.org>
Cc: Nathan Chancellor <nathan@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Rasmus Villemoes <linux@rasmusvillemoes.dk>
Cc: Sander Vanheule <sander@svanheule.net>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Vlastimil Babka <vbabka@suse.cz>
Cc: Yury Norov <yury.norov@gmail.com>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 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 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 <linux@rasmusvillemoes.dk>

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

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 <bvanassche@acm.org>:
> 
> 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 <akpm@linux-foundation.org>
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: Dan Williams <dan.j.williams@intel.com>
> Cc: Eric Dumazet <edumazet@google.com>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: Isabella Basso <isabbasso@riseup.net>
> Cc: "Jason A. Donenfeld" <Jason@zx2c4.com>
> Cc: Josh Poimboeuf <jpoimboe@kernel.org>
> Cc: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>
> Cc: Masami Hiramatsu <mhiramat@kernel.org>
> Cc: Nathan Chancellor <nathan@kernel.org>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Rasmus Villemoes <linux@rasmusvillemoes.dk>
> Cc: Sander Vanheule <sander@svanheule.net>
> Cc: Steven Rostedt <rostedt@goodmis.org>
> Cc: Vlastimil Babka <vbabka@suse.cz>
> Cc: Yury Norov <yury.norov@gmail.com>
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
> 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 <isabbasso@riseup.net>

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 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 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 <akpm@linux-foundation.org>
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: Dan Williams <dan.j.williams@intel.com>
> Cc: Eric Dumazet <edumazet@google.com>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: Isabella Basso <isabbasso@riseup.net>
> Cc: "Jason A. Donenfeld" <Jason@zx2c4.com>
> Cc: Josh Poimboeuf <jpoimboe@kernel.org>
> Cc: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>
> Cc: Masami Hiramatsu <mhiramat@kernel.org>
> Cc: Nathan Chancellor <nathan@kernel.org>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Rasmus Villemoes <linux@rasmusvillemoes.dk>
> Cc: Sander Vanheule <sander@svanheule.net>
> Cc: Steven Rostedt <rostedt@goodmis.org>
> Cc: Vlastimil Babka <vbabka@suse.cz>
> Cc: Yury Norov <yury.norov@gmail.com>
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>

Reviewed-by: Kees Cook <keescook@chromium.org>

-- 
Kees Cook