Re: [PATCH v2 7/7] tools/testing/selftests/bpf: replace open-coded 16 with TASK_COMM_LEN

Hi,

David Hildenbrand [off-list ref] writes:

On 29.11.21 15:21, Sven Schnelle wrote:

quoted

Yafang Shao [off-list ref] writes:

quoted

Thanks for the report and debugging!
Seems we should explicitly define it as signed ?
Could you pls. help verify it?

diff --git a/include/linux/sched.h b/include/linux/sched.h
index cecd4806edc6..44d36c6af3e1 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h

@@ -278,7 +278,7 @@ struct task_group;
  * Define the task command name length as enum, then it can be visible to
  * BPF programs.
  */
-enum {
+enum SignedEnum {
        TASK_COMM_LEN = 16,
 };

Umm no. What you're doing here is to define the name of the enum as
'SignedEnum'. This doesn't change the type. I think before C++0x you
couldn't force an enum type.

I think there are only some "hacks" to modify the type with GCC. For
example, with "__attribute__((packed))" we can instruct GCC to use the
smallest type possible for the defined enum values.

Yes, i meant no way that the standard defines. You could force it to
signed by having a negative member.

I think with some fake entries one can eventually instruct GCC to use an
unsigned type in some cases:

https://stackoverflow.com/questions/14635833/is-there-a-way-to-make-an-enum-unsigned-in-the-c90-standard-misra-c-2004-compl

enum {
	TASK_COMM_LEN = 16,
	TASK_FORCE_UNSIGNED = 0x80000000,
};

Haven't tested it, though, and I'm not sure if we should really do that
... :)

TBH, i would vote for reverting the change. defining an array size as
enum feels really odd.

Regards
Sven

On Mon, 29 Nov 2021 23:33:33 +0800
Yafang Shao [off-list ref] wrote:

quoted

TBH, i would vote for reverting the change. defining an array size as
enum feels really odd.
 

We changed it to enum because the BTF can't parse macro while it can
parse the enum type.

I wonder if BTF could take advantage of the tracing:

TRACE_DEFINE_ENUM() macros?

This is how they are converted for user space tooling.

Anyway, I'd have to go and look at why that trigger test failed. I don't
see how the size of the array caused it to change the signage of value.

-- Steve

Anyway I don't insist on keeping this change if you think reverting it
is better.

On Mon, Nov 29, 2021 at 10:38 PM Sven Schnelle [off-list ref] wrote:

Hi,

David Hildenbrand [off-list ref] writes:

quoted

On 29.11.21 15:21, Sven Schnelle wrote:

quoted

Yafang Shao [off-list ref] writes:

quoted

Thanks for the report and debugging!
Seems we should explicitly define it as signed ?
Could you pls. help verify it?

diff --git a/include/linux/sched.h b/include/linux/sched.h
index cecd4806edc6..44d36c6af3e1 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h

@@ -278,7 +278,7 @@ struct task_group;
  * Define the task command name length as enum, then it can be visible to
  * BPF programs.
  */
-enum {
+enum SignedEnum {
        TASK_COMM_LEN = 16,
 };

Umm no. What you're doing here is to define the name of the enum as
'SignedEnum'. This doesn't change the type. I think before C++0x you
couldn't force an enum type.

I think there are only some "hacks" to modify the type with GCC. For
example, with "__attribute__((packed))" we can instruct GCC to use the
smallest type possible for the defined enum values.

Yes, i meant no way that the standard defines. You could force it to
signed by having a negative member.

quoted

I think with some fake entries one can eventually instruct GCC to use an
unsigned type in some cases:

https://stackoverflow.com/questions/14635833/is-there-a-way-to-make-an-enum-unsigned-in-the-c90-standard-misra-c-2004-compl

enum {
      TASK_COMM_LEN = 16,
      TASK_FORCE_UNSIGNED = 0x80000000,
};

Haven't tested it, though, and I'm not sure if we should really do that
... :)

TBH, i would vote for reverting the change. defining an array size as
enum feels really odd.

We changed it to enum because the BTF can't parse macro while it can
parse the enum type.
Anyway I don't insist on keeping this change if you think reverting it
is better.

Andrew, would you pls. help drop this patch from the -mm tree (the
other 6 patches in this series can be kept) ?


--
Thanks
Yafang

On Mon, 29 Nov 2021 11:07:55 -0500
Steven Rostedt [off-list ref] wrote:

I wonder if BTF could take advantage of the tracing:

TRACE_DEFINE_ENUM() macros?

Bah' BTF does handle enums, it doesn't handle macros. But I wonder if we
could do something similar for BTF. That is, force it.

-- Steve