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

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

[Sven Schnelle]

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.

> +		default:
> +			putchar(*s);
> +		}
> +	}
> +}

Thanks,
Sven

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

[Quentin Monnet]

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?

Quentin

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

[Sven Schnelle]

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.

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

[Quentin Monnet]

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