| Message ID | 20220501190033.2579182-1-yhs@fb.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | bpf: Add 64bit enum value support | expand |
On 5/1/22 3:00 PM, Yonghong Song wrote: > The kflag is supported now for BTF_KIND_ENUM. > So remove the test which tests verifier failure > due to existence of kflag. > > With enum64 support in kernel and libbpf, > selftest btf_dump/btf_dump failed with > no-enum64 support llvm for the following > enum definition: > enum e2 { > C = 100, > D = 4294967295, > E = 0, > }; > > With the no-enum64 support llvm, the signedness is > 'signed' by default, and D (4294967295 = 0xffffffff) > will print as -1. With enum64 support llvm, the signedness > is 'unsigned' and the value of D will print as 4294967295. To confirm my understanding, this signedness-by-default change is for _printing btf in c format only_ and doesn't correspond to any difference in interpretation , since all BTF enums before addition of new kflag were assumed to be signed, and new kflag's default value is signed as well. > To support both old and new compilers, this patch > changed the value to 268435455 = 0xfffffff which works > with both enum64 or non-enum64 support llvm. > > Signed-off-by: Yonghong Song <yhs@fb.com> > --- Aside from the general question, for changes in this patch: Acked-by: Dave Marchevsky <davemarchevsky@fb.com>
On Sun, May 1, 2022 at 12:00 PM Yonghong Song <yhs@fb.com> wrote: > > The kflag is supported now for BTF_KIND_ENUM. > So remove the test which tests verifier failure > due to existence of kflag. > > With enum64 support in kernel and libbpf, > selftest btf_dump/btf_dump failed with > no-enum64 support llvm for the following > enum definition: > enum e2 { > C = 100, > D = 4294967295, > E = 0, > }; > > With the no-enum64 support llvm, the signedness is > 'signed' by default, and D (4294967295 = 0xffffffff) > will print as -1. With enum64 support llvm, the signedness > is 'unsigned' and the value of D will print as 4294967295. > To support both old and new compilers, this patch > changed the value to 268435455 = 0xfffffff which works > with both enum64 or non-enum64 support llvm. > > Signed-off-by: Yonghong Song <yhs@fb.com> > --- > tools/testing/selftests/bpf/prog_tests/btf.c | 20 ------------------- > .../bpf/progs/btf_dump_test_case_syntax.c | 2 +- > 2 files changed, 1 insertion(+), 21 deletions(-) > > diff --git a/tools/testing/selftests/bpf/prog_tests/btf.c b/tools/testing/selftests/bpf/prog_tests/btf.c > index ba5bde53d418..8e068e06b3e8 100644 > --- a/tools/testing/selftests/bpf/prog_tests/btf.c > +++ b/tools/testing/selftests/bpf/prog_tests/btf.c > @@ -2896,26 +2896,6 @@ static struct btf_raw_test raw_tests[] = { > .err_str = "Invalid btf_info kind_flag", > }, > > -{ > - .descr = "invalid enum kind_flag", > - .raw_types = { > - BTF_TYPE_INT_ENC(0, BTF_INT_SIGNED, 0, 32, 4), /* [1] */ > - BTF_TYPE_ENC(0, BTF_INFO_ENC(BTF_KIND_ENUM, 1, 1), 4), /* [2] */ > - BTF_ENUM_ENC(NAME_TBD, 0), > - BTF_END_RAW, > - }, > - BTF_STR_SEC("\0A"), > - .map_type = BPF_MAP_TYPE_ARRAY, > - .map_name = "enum_type_check_btf", > - .key_size = sizeof(int), > - .value_size = sizeof(int), > - .key_type_id = 1, > - .value_type_id = 1, > - .max_entries = 4, > - .btf_load_err = true, > - .err_str = "Invalid btf_info kind_flag", > -}, > - > { > .descr = "valid fwd kind_flag", > .raw_types = { > diff --git a/tools/testing/selftests/bpf/progs/btf_dump_test_case_syntax.c b/tools/testing/selftests/bpf/progs/btf_dump_test_case_syntax.c > index 1c7105fcae3c..4068cea4be53 100644 > --- a/tools/testing/selftests/bpf/progs/btf_dump_test_case_syntax.c > +++ b/tools/testing/selftests/bpf/progs/btf_dump_test_case_syntax.c > @@ -13,7 +13,7 @@ enum e1 { > > enum e2 { > C = 100, > - D = 4294967295, > + D = 268435455, > E = 0, > }; can you please also add btf_dump tests for >32-bit enums at the same time? Both signed and unsigned? > > -- > 2.30.2 >
On 5/8/22 7:21 PM, Dave Marchevsky wrote: > On 5/1/22 3:00 PM, Yonghong Song wrote: >> The kflag is supported now for BTF_KIND_ENUM. >> So remove the test which tests verifier failure >> due to existence of kflag. >> >> With enum64 support in kernel and libbpf, >> selftest btf_dump/btf_dump failed with >> no-enum64 support llvm for the following >> enum definition: >> enum e2 { >> C = 100, >> D = 4294967295, >> E = 0, >> }; >> >> With the no-enum64 support llvm, the signedness is >> 'signed' by default, and D (4294967295 = 0xffffffff) >> will print as -1. With enum64 support llvm, the signedness >> is 'unsigned' and the value of D will print as 4294967295. > > To confirm my understanding, this signedness-by-default change is for _printing > btf in c format only_ and doesn't correspond to any difference in interpretation > , since all BTF enums before addition of new kflag were assumed to be signed, > and new kflag's default value is signed as well. Yes, the signedness is only used for printing in C format. For values, the signedness is really depending on corresponding declaration type or the context where the value is used. In the next revision, I may change the default signedness to be unsigned to cover common case like enum { A = 0, B, C, D, ... } > >> To support both old and new compilers, this patch >> changed the value to 268435455 = 0xfffffff which works >> with both enum64 or non-enum64 support llvm. >> >> Signed-off-by: Yonghong Song <yhs@fb.com> >> --- > > Aside from the general question, for changes in this patch: > > Acked-by: Dave Marchevsky <davemarchevsky@fb.com>
On 5/9/22 4:34 PM, Andrii Nakryiko wrote: > On Sun, May 1, 2022 at 12:00 PM Yonghong Song <yhs@fb.com> wrote: >> >> The kflag is supported now for BTF_KIND_ENUM. >> So remove the test which tests verifier failure >> due to existence of kflag. >> >> With enum64 support in kernel and libbpf, >> selftest btf_dump/btf_dump failed with >> no-enum64 support llvm for the following >> enum definition: >> enum e2 { >> C = 100, >> D = 4294967295, >> E = 0, >> }; >> >> With the no-enum64 support llvm, the signedness is >> 'signed' by default, and D (4294967295 = 0xffffffff) >> will print as -1. With enum64 support llvm, the signedness >> is 'unsigned' and the value of D will print as 4294967295. >> To support both old and new compilers, this patch >> changed the value to 268435455 = 0xfffffff which works >> with both enum64 or non-enum64 support llvm. >> >> Signed-off-by: Yonghong Song <yhs@fb.com> >> --- >> tools/testing/selftests/bpf/prog_tests/btf.c | 20 ------------------- >> .../bpf/progs/btf_dump_test_case_syntax.c | 2 +- >> 2 files changed, 1 insertion(+), 21 deletions(-) >> >> diff --git a/tools/testing/selftests/bpf/prog_tests/btf.c b/tools/testing/selftests/bpf/prog_tests/btf.c >> index ba5bde53d418..8e068e06b3e8 100644 >> --- a/tools/testing/selftests/bpf/prog_tests/btf.c >> +++ b/tools/testing/selftests/bpf/prog_tests/btf.c >> @@ -2896,26 +2896,6 @@ static struct btf_raw_test raw_tests[] = { >> .err_str = "Invalid btf_info kind_flag", >> }, >> >> -{ >> - .descr = "invalid enum kind_flag", >> - .raw_types = { >> - BTF_TYPE_INT_ENC(0, BTF_INT_SIGNED, 0, 32, 4), /* [1] */ >> - BTF_TYPE_ENC(0, BTF_INFO_ENC(BTF_KIND_ENUM, 1, 1), 4), /* [2] */ >> - BTF_ENUM_ENC(NAME_TBD, 0), >> - BTF_END_RAW, >> - }, >> - BTF_STR_SEC("\0A"), >> - .map_type = BPF_MAP_TYPE_ARRAY, >> - .map_name = "enum_type_check_btf", >> - .key_size = sizeof(int), >> - .value_size = sizeof(int), >> - .key_type_id = 1, >> - .value_type_id = 1, >> - .max_entries = 4, >> - .btf_load_err = true, >> - .err_str = "Invalid btf_info kind_flag", >> -}, >> - >> { >> .descr = "valid fwd kind_flag", >> .raw_types = { >> diff --git a/tools/testing/selftests/bpf/progs/btf_dump_test_case_syntax.c b/tools/testing/selftests/bpf/progs/btf_dump_test_case_syntax.c >> index 1c7105fcae3c..4068cea4be53 100644 >> --- a/tools/testing/selftests/bpf/progs/btf_dump_test_case_syntax.c >> +++ b/tools/testing/selftests/bpf/progs/btf_dump_test_case_syntax.c >> @@ -13,7 +13,7 @@ enum e1 { >> >> enum e2 { >> C = 100, >> - D = 4294967295, >> + D = 268435455, >> E = 0, >> }; > > can you please also add btf_dump tests for >32-bit enums at the same > time? Both signed and unsigned? will do. > > >> >> -- >> 2.30.2 >>
diff --git a/tools/testing/selftests/bpf/prog_tests/btf.c b/tools/testing/selftests/bpf/prog_tests/btf.c index ba5bde53d418..8e068e06b3e8 100644 --- a/tools/testing/selftests/bpf/prog_tests/btf.c +++ b/tools/testing/selftests/bpf/prog_tests/btf.c @@ -2896,26 +2896,6 @@ static struct btf_raw_test raw_tests[] = { .err_str = "Invalid btf_info kind_flag", }, -{ - .descr = "invalid enum kind_flag", - .raw_types = { - BTF_TYPE_INT_ENC(0, BTF_INT_SIGNED, 0, 32, 4), /* [1] */ - BTF_TYPE_ENC(0, BTF_INFO_ENC(BTF_KIND_ENUM, 1, 1), 4), /* [2] */ - BTF_ENUM_ENC(NAME_TBD, 0), - BTF_END_RAW, - }, - BTF_STR_SEC("\0A"), - .map_type = BPF_MAP_TYPE_ARRAY, - .map_name = "enum_type_check_btf", - .key_size = sizeof(int), - .value_size = sizeof(int), - .key_type_id = 1, - .value_type_id = 1, - .max_entries = 4, - .btf_load_err = true, - .err_str = "Invalid btf_info kind_flag", -}, - { .descr = "valid fwd kind_flag", .raw_types = { diff --git a/tools/testing/selftests/bpf/progs/btf_dump_test_case_syntax.c b/tools/testing/selftests/bpf/progs/btf_dump_test_case_syntax.c index 1c7105fcae3c..4068cea4be53 100644 --- a/tools/testing/selftests/bpf/progs/btf_dump_test_case_syntax.c +++ b/tools/testing/selftests/bpf/progs/btf_dump_test_case_syntax.c @@ -13,7 +13,7 @@ enum e1 { enum e2 { C = 100, - D = 4294967295, + D = 268435455, E = 0, };
The kflag is supported now for BTF_KIND_ENUM. So remove the test which tests verifier failure due to existence of kflag. With enum64 support in kernel and libbpf, selftest btf_dump/btf_dump failed with no-enum64 support llvm for the following enum definition: enum e2 { C = 100, D = 4294967295, E = 0, }; With the no-enum64 support llvm, the signedness is 'signed' by default, and D (4294967295 = 0xffffffff) will print as -1. With enum64 support llvm, the signedness is 'unsigned' and the value of D will print as 4294967295. To support both old and new compilers, this patch changed the value to 268435455 = 0xfffffff which works with both enum64 or non-enum64 support llvm. Signed-off-by: Yonghong Song <yhs@fb.com> --- tools/testing/selftests/bpf/prog_tests/btf.c | 20 ------------------- .../bpf/progs/btf_dump_test_case_syntax.c | 2 +- 2 files changed, 1 insertion(+), 21 deletions(-)