| Message ID | 20210209211954.490077-1-songliubraving@fb.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [v4] checkpatch: do not apply "initialise globals to 0" check to BPF progs | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
On Tue, 2021-02-09 at 13:19 -0800, Song Liu wrote: > BPF programs explicitly initialise global variables to 0 to make sure > clang (v10 or older) do not put the variables in the common section. Acked-by: Joe Perches <joe@perches.com> So the patch is OK now, but I have a question about the concept: Do you mean that these initialized to 0 global variables should go into bss or another section? Perhaps it'd be useful to somehow mark variables into specific sections rather than bss when initialized to 0 and data when not initialized to 0. $ clang --version clang version 10.0.0 (git://github.com/llvm/llvm-project.git 305b961f64b75e73110e309341535f6d5a48ed72) Target: x86_64-unknown-linux-gnu Thread model: posix $ cat t_common.c int a = 0; int b = 1; int foo_a(void) { return a; } int foo_b(void) { return b; } $ clang -c -O3 t_common.c $ objdump -x t_common.o t_common.o: file format elf64-x86-64 t_common.o architecture: i386:x86-64, flags 0x00000011: HAS_RELOC, HAS_SYMS start address 0x0000000000000000 Sections: Idx Name Size VMA LMA File off Algn 0 .text 00000017 0000000000000000 0000000000000000 00000040 2**4 CONTENTS, ALLOC, LOAD, RELOC, READONLY, CODE 1 .bss 00000004 0000000000000000 0000000000000000 00000058 2**2 ALLOC 2 .data 00000004 0000000000000000 0000000000000000 00000058 2**2 CONTENTS, ALLOC, LOAD, DATA 3 .comment 00000068 0000000000000000 0000000000000000 0000005c 2**0 CONTENTS, READONLY 4 .note.GNU-stack 00000000 0000000000000000 0000000000000000 000000c4 2**0 CONTENTS, READONLY 5 .eh_frame 00000040 0000000000000000 0000000000000000 000000c8 2**3 CONTENTS, ALLOC, LOAD, RELOC, READONLY, DATA 6 .llvm_addrsig 00000000 0000000000000000 0000000000000000 00000210 2**0 CONTENTS, READONLY, EXCLUDE SYMBOL TABLE: 0000000000000000 l df *ABS* 0000000000000000 t_common.c 0000000000000000 l d .text 0000000000000000 .text 0000000000000000 g O .bss 0000000000000004 a 0000000000000000 g O .data 0000000000000004 b 0000000000000000 g F .text 0000000000000007 foo_a 0000000000000010 g F .text 0000000000000007 foo_b RELOCATION RECORDS FOR [.text]: OFFSET TYPE VALUE 0000000000000002 R_X86_64_PC32 a-0x0000000000000004 0000000000000012 R_X86_64_PC32 b-0x0000000000000004 RELOCATION RECORDS FOR [.eh_frame]: OFFSET TYPE VALUE 0000000000000020 R_X86_64_PC32 .text 0000000000000034 R_X86_64_PC32 .text+0x0000000000000010 Perhaps instead something like: $ cat t_common_bpf.c __attribute__((__section__("bpf"))) int a = 0; __attribute__((__section__("bpf"))) int b = 1; int foo_a(void) { return a; } int foo_b(void) { return b; } $ clang -c -O3 t_common_bpf.c $ objdump -x t_common_bpf.o t_common_bpf.o: file format elf64-x86-64 t_common_bpf.o architecture: i386:x86-64, flags 0x00000011: HAS_RELOC, HAS_SYMS start address 0x0000000000000000 Sections: Idx Name Size VMA LMA File off Algn 0 .text 00000017 0000000000000000 0000000000000000 00000040 2**4 CONTENTS, ALLOC, LOAD, RELOC, READONLY, CODE 1 bpf 00000008 0000000000000000 0000000000000000 00000058 2**2 CONTENTS, ALLOC, LOAD, DATA 2 .comment 00000068 0000000000000000 0000000000000000 00000060 2**0 CONTENTS, READONLY 3 .note.GNU-stack 00000000 0000000000000000 0000000000000000 000000c8 2**0 CONTENTS, READONLY 4 .eh_frame 00000040 0000000000000000 0000000000000000 000000c8 2**3 CONTENTS, ALLOC, LOAD, RELOC, READONLY, DATA 5 .llvm_addrsig 00000000 0000000000000000 0000000000000000 00000210 2**0 CONTENTS, READONLY, EXCLUDE SYMBOL TABLE: 0000000000000000 l df *ABS* 0000000000000000 t_common_bpf.c 0000000000000000 l d .text 0000000000000000 .text 0000000000000000 g O bpf 0000000000000004 a 0000000000000004 g O bpf 0000000000000004 b 0000000000000000 g F .text 0000000000000007 foo_a 0000000000000010 g F .text 0000000000000007 foo_b RELOCATION RECORDS FOR [.text]: OFFSET TYPE VALUE 0000000000000002 R_X86_64_PC32 a-0x0000000000000004 0000000000000012 R_X86_64_PC32 b-0x0000000000000004 RELOCATION RECORDS FOR [.eh_frame]: OFFSET TYPE VALUE 0000000000000020 R_X86_64_PC32 .text 0000000000000034 R_X86_64_PC32 .text+0x0000000000000010
> On Feb 9, 2021, at 6:10 PM, Joe Perches <joe@perches.com> wrote: > > On Tue, 2021-02-09 at 13:19 -0800, Song Liu wrote: >> BPF programs explicitly initialise global variables to 0 to make sure >> clang (v10 or older) do not put the variables in the common section. > > Acked-by: Joe Perches <joe@perches.com> > > So the patch is OK now, but I have a question about the concept: > > Do you mean that these initialized to 0 global variables > should go into bss or another section? We want these variables to go to bss. > Perhaps it'd be useful to somehow mark variables into specific > sections rather than bss when initialized to 0 and data when not > initialized to 0. Currently, libbpf expects zero initialized global data in bss. This convention works well so far. Is there any reason that we specify section for global data? Thanks, Song > > $ clang --version > clang version 10.0.0 (git://github.com/llvm/llvm-project.git 305b961f64b75e73110e309341535f6d5a48ed72) > Target: x86_64-unknown-linux-gnu > Thread model: posix > > $ cat t_common.c > int a = 0; > int b = 1; > > int foo_a(void) > { > return a; > } > > int foo_b(void) > { > return b; > } > > $ clang -c -O3 t_common.c > > $ objdump -x t_common.o > > t_common.o: file format elf64-x86-64 > t_common.o > architecture: i386:x86-64, flags 0x00000011: > HAS_RELOC, HAS_SYMS > start address 0x0000000000000000 > > Sections: > Idx Name Size VMA LMA File off Algn > 0 .text 00000017 0000000000000000 0000000000000000 00000040 2**4 > CONTENTS, ALLOC, LOAD, RELOC, READONLY, CODE > 1 .bss 00000004 0000000000000000 0000000000000000 00000058 2**2 > ALLOC > 2 .data 00000004 0000000000000000 0000000000000000 00000058 2**2 > CONTENTS, ALLOC, LOAD, DATA > 3 .comment 00000068 0000000000000000 0000000000000000 0000005c 2**0 > CONTENTS, READONLY > 4 .note.GNU-stack 00000000 0000000000000000 0000000000000000 000000c4 2**0 > CONTENTS, READONLY > 5 .eh_frame 00000040 0000000000000000 0000000000000000 000000c8 2**3 > CONTENTS, ALLOC, LOAD, RELOC, READONLY, DATA > 6 .llvm_addrsig 00000000 0000000000000000 0000000000000000 00000210 2**0 > CONTENTS, READONLY, EXCLUDE > SYMBOL TABLE: > 0000000000000000 l df *ABS* 0000000000000000 t_common.c > 0000000000000000 l d .text 0000000000000000 .text > 0000000000000000 g O .bss 0000000000000004 a > 0000000000000000 g O .data 0000000000000004 b > 0000000000000000 g F .text 0000000000000007 foo_a > 0000000000000010 g F .text 0000000000000007 foo_b > > > RELOCATION RECORDS FOR [.text]: > OFFSET TYPE VALUE > 0000000000000002 R_X86_64_PC32 a-0x0000000000000004 > 0000000000000012 R_X86_64_PC32 b-0x0000000000000004 > > > RELOCATION RECORDS FOR [.eh_frame]: > OFFSET TYPE VALUE > 0000000000000020 R_X86_64_PC32 .text > 0000000000000034 R_X86_64_PC32 .text+0x0000000000000010 > > > Perhaps instead something like: > > $ cat t_common_bpf.c > __attribute__((__section__("bpf"))) int a = 0; > __attribute__((__section__("bpf"))) int b = 1; > > int foo_a(void) > { > return a; > } > > int foo_b(void) > { > return b; > } > > $ clang -c -O3 t_common_bpf.c > > $ objdump -x t_common_bpf.o > > t_common_bpf.o: file format elf64-x86-64 > t_common_bpf.o > architecture: i386:x86-64, flags 0x00000011: > HAS_RELOC, HAS_SYMS > start address 0x0000000000000000 > > Sections: > Idx Name Size VMA LMA File off Algn > 0 .text 00000017 0000000000000000 0000000000000000 00000040 2**4 > CONTENTS, ALLOC, LOAD, RELOC, READONLY, CODE > 1 bpf 00000008 0000000000000000 0000000000000000 00000058 2**2 > CONTENTS, ALLOC, LOAD, DATA > 2 .comment 00000068 0000000000000000 0000000000000000 00000060 2**0 > CONTENTS, READONLY > 3 .note.GNU-stack 00000000 0000000000000000 0000000000000000 000000c8 2**0 > CONTENTS, READONLY > 4 .eh_frame 00000040 0000000000000000 0000000000000000 000000c8 2**3 > CONTENTS, ALLOC, LOAD, RELOC, READONLY, DATA > 5 .llvm_addrsig 00000000 0000000000000000 0000000000000000 00000210 2**0 > CONTENTS, READONLY, EXCLUDE > SYMBOL TABLE: > 0000000000000000 l df *ABS* 0000000000000000 t_common_bpf.c > 0000000000000000 l d .text 0000000000000000 .text > 0000000000000000 g O bpf 0000000000000004 a > 0000000000000004 g O bpf 0000000000000004 b > 0000000000000000 g F .text 0000000000000007 foo_a > 0000000000000010 g F .text 0000000000000007 foo_b > > > RELOCATION RECORDS FOR [.text]: > OFFSET TYPE VALUE > 0000000000000002 R_X86_64_PC32 a-0x0000000000000004 > 0000000000000012 R_X86_64_PC32 b-0x0000000000000004 > > > RELOCATION RECORDS FOR [.eh_frame]: > OFFSET TYPE VALUE > 0000000000000020 R_X86_64_PC32 .text > 0000000000000034 R_X86_64_PC32 .text+0x0000000000000010 > > > >
On Wed, 2021-02-10 at 04:07 +0000, Song Liu wrote: > > On Feb 9, 2021, at 6:10 PM, Joe Perches <joe@perches.com> wrote: > > On Tue, 2021-02-09 at 13:19 -0800, Song Liu wrote: > > > BPF programs explicitly initialise global variables to 0 to make sure > > > clang (v10 or older) do not put the variables in the common section. > > > > Acked-by: Joe Perches <joe@perches.com> > > > > So the patch is OK now, but I have a question about the concept: > > > > Do you mean that these initialized to 0 global variables > > should go into bss or another section? > > We want these variables to go to bss. OK, then the patch is fine. > > Perhaps it'd be useful to somehow mark variables into specific > > sections rather than bss when initialized to 0 and data when not > > initialized to 0. > > Currently, libbpf expects zero initialized global data in bss. This > convention works well so far. Is there any reason that we specify > section for global data? There's no need I know of. cheers, Joe
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl index 1afe3af1cc097..f5b7ec9170999 100755 --- a/scripts/checkpatch.pl +++ b/scripts/checkpatch.pl @@ -2428,6 +2428,15 @@ sub get_raw_comment { return $comment; } +sub exclude_global_initialisers { + my ($realfile) = @_; + + # Do not check for BPF programs (tools/testing/selftests/bpf/progs/*.c, samples/bpf/*_kern.c, *.bpf.c). + return $realfile =~ m@^tools/testing/selftests/bpf/progs/.*\.c$@ || + $realfile =~ m@^samples/bpf/.*_kern\.c$@ || + $realfile =~ m@/bpf/.*\.bpf\.c$@; +} + sub process { my $filename = shift; @@ -4323,7 +4332,8 @@ sub process { } # check for global initialisers. - if ($line =~ /^\+$Type\s*$Ident(?:\s+$Modifier)*\s*=\s*($zero_initializer)\s*;/) { + if ($line =~ /^\+$Type\s*$Ident(?:\s+$Modifier)*\s*=\s*($zero_initializer)\s*;/ && + !exclude_global_initialisers($realfile)) { if (ERROR("GLOBAL_INITIALISERS", "do not initialise globals to $1\n" . $herecurr) && $fix) {
BPF programs explicitly initialise global variables to 0 to make sure clang (v10 or older) do not put the variables in the common section. Skip "initialise globals to 0" check for BPF programs to elimiate error messages like: ERROR: do not initialise globals to 0 #19: FILE: samples/bpf/tracex1_kern.c:21: Cc: Andy Whitcroft <apw@canonical.com> Cc: Joe Perches <joe@perches.com> Signed-off-by: Song Liu <songliubraving@fb.com> --- Changes v3 => v4: 1. Yet more fixes to regexes. Changes v2 => v3: 1. Fix regex. Changes v1 => v2: 1. Add function exclude_global_initialisers() to keep the code clean. --- scripts/checkpatch.pl | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-)