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

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