Re: [PATCH bpf-next v3 3/7] bpftool: Support inline annotations when dumping the CFG of a program

[Quentin Monnet <quentin@isovalent.com>]

2023-04-12 14:26 UTC+0200 ~ Sven Schnelle <svens@linux.ibm.com>
> Quentin Monnet <quentin@isovalent.com> writes:
> 
>> 2023-04-12 08:04 UTC+0200 ~ Sven Schnelle <svens@linux.ibm.com>
>>> Quentin Monnet <quentin@isovalent.com> writes:
>>>
>>>> diff --git a/tools/bpf/bpftool/btf_dumper.c b/tools/bpf/bpftool/btf_dumper.c
>>>> index e7f6ec3a8f35..583aa843df92 100644
>>>> --- a/tools/bpf/bpftool/btf_dumper.c
>>>> +++ b/tools/bpf/bpftool/btf_dumper.c
>>>> @@ -821,3 +821,37 @@ void btf_dump_linfo_json(const struct btf *btf,
>>>>  					BPF_LINE_INFO_LINE_COL(linfo->line_col));
>>>>  	}
>>>>  }
>>>> +
>>>> +static void dotlabel_puts(const char *s)
>>>> +{
>>>> +	for (; *s; ++s) {
>>>> +		switch (*s) {
>>>> +		case '\\':
>>>> +		case '"':
>>>> +		case '{':
>>>> +		case '}':
>>>> +		case '<':
>>>> +		case '>':
>>>> +		case '|':
>>>> +		case ' ':
>>>> +			putchar('\\');
>>>> +			__fallthrough;
>>>
>>> Is __fallthrough correct? I see the following compile error on s390 in
>>> linux-next (20230412):
>>>
>>>   CC      btf_dumper.o
>>> btf_dumper.c: In function ‘dotlabel_puts’:
>>> btf_dumper.c:838:25: error: ‘__fallthrough’ undeclared (first use in this function); did you mean ‘fallthrough’?
>>>   838 |                         __fallthrough;
>>>       |                         ^~~~~~~~~~~~~
>>>
>>> removing the two underscores fixes this.
>>
>> I thought so? Perf seems to use the double underscores as well. Just
>> "fallthrough" does not seem to be the right fix anyway, it gives me an
>> error similar to yours on x86_64 with "fallthrough" undeclared.
>>
>> The definition should be pulled from tools/include/linux/compiler.h (and
>> .../compiler-gcc.h). I thought this file would be at least included from
>> bpftool's main.h, in turn included in btf_dumper.c. Looking at the chain
>> of inclusions, on my system I get the following path:
>>
>>     $ CFLAGS=-H make btf_dumper.o
>>     [...]
>>     . /root/dev/linux/tools/include/linux/bitops.h
>>     [...]
>>     .. /root/dev/linux/tools/include/linux/bits.h
>>     [...]
>>     ... /root/dev/linux/tools/include/linux/build_bug.h
>>     .... /root/dev/linux/tools/include/linux/compiler.h
>>     ..... /root/dev/linux/tools/include/linux/compiler_types.h
>>     ...... /root/dev/linux/tools/include/linux/compiler-gcc.h
>>     [...]
>>
>> What do you get on your side?
>>
>> If you add "#include <linux/compiler.h>" to btf_dumper.c directly, does
>> it fix the issue?
> 
> This seems to clash with:
> 
> commit f7a858bffcddaaf70c71b6b656e7cc21b6107cec
> Author: Liam Howlett <liam.howlett@oracle.com>
> Date:   Fri Nov 25 15:50:16 2022 +0000
> 
>     tools: Rename __fallthrough to fallthrough
> 
>     Rename the fallthrough attribute to better align with the kernel
>     version.  Copy the definition from include/linux/compiler_attributes.h
>     including the #else clause.  Adding the #else clause allows the tools
>     compiler.h header to drop the check for a definition entirely and keeps
>     both definitions together.
> 
>     Change any __fallthrough statements to fallthrough anywhere it was used
>     within perf.
> 
>     This allows other tools to use the same key word as the kernel.
> 
> Which was also merged in linux-next.

Right, I was not aware of this commit. In that case, replacing with
"fallthrough" in linux-next makes sense indeed.

Thomas Richter just submitted that fix at
https://lore.kernel.org/all/20230412123636.2358949-1-tmricht@linux.ibm.com/.

Thanks!
Quentin