[PATCH 1/2] kernel/bpf: change "char *" string form to "char []"

[PATCH 1/2] kernel/bpf: change "char *" string form to "char []"

[liqiong]

The string form of "char []" declares a single variable. It is better
than "char *" which creates two variables.

Signed-off-by: liqiong <liqiong@nfschina.com>
---
 kernel/bpf/btf.c      | 4 ++--
 kernel/bpf/verifier.c | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
index 0918a39279f6..218a8ac73644 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -894,10 +894,10 @@ static const struct btf_type *btf_type_skip_qualifiers(const struct btf *btf,
 static const char *btf_show_name(struct btf_show *show)
 {
 	/* BTF_MAX_ITER array suffixes "[]" */
-	const char *array_suffixes = "[][][][][][][][][][]";
+	static const char array_suffixes[] = "[][][][][][][][][][]";
 	const char *array_suffix = &array_suffixes[strlen(array_suffixes)];
 	/* BTF_MAX_ITER pointer suffixes "*" */
-	const char *ptr_suffixes = "**********";
+	static const char ptr_suffixes[] = "**********";
 	const char *ptr_suffix = &ptr_suffixes[strlen(ptr_suffixes)];
 	const char *name = NULL, *prefix = "", *parens = "";
 	const struct btf_member *m = show->state.member;
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index d175b70067b3..78a090fcbc72 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -7346,7 +7346,7 @@ static int sanitize_err(struct bpf_verifier_env *env,
 			const struct bpf_reg_state *off_reg,
 			const struct bpf_reg_state *dst_reg)
 {
-	static const char *err = "pointer arithmetic with it prohibited for !root";
+	static const char err[] = "pointer arithmetic with it prohibited for !root";
 	const char *op = BPF_OP(insn->code) == BPF_ADD ? "add" : "sub";
 	u32 dst = insn->dst_reg, src = insn->src_reg;
 
-- 
2.25.1

Re: [PATCH 1/2] kernel/bpf: change "char *" string form to "char []"

[Yonghong Song]

On 5/12/22 7:28 AM, liqiong wrote:
> The string form of "char []" declares a single variable. It is better
> than "char *" which creates two variables.

Could you explain in details about why it is better in generated codes?
It is not clear to me why your patch is better than the original code.

> 
> Signed-off-by: liqiong <liqiong@nfschina.com>
> ---
>   kernel/bpf/btf.c      | 4 ++--
>   kernel/bpf/verifier.c | 2 +-
>   2 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> index 0918a39279f6..218a8ac73644 100644
> --- a/kernel/bpf/btf.c
> +++ b/kernel/bpf/btf.c
> @@ -894,10 +894,10 @@ static const struct btf_type *btf_type_skip_qualifiers(const struct btf *btf,
>   static const char *btf_show_name(struct btf_show *show)
>   {
>   	/* BTF_MAX_ITER array suffixes "[]" */
> -	const char *array_suffixes = "[][][][][][][][][][]";
> +	static const char array_suffixes[] = "[][][][][][][][][][]";
>   	const char *array_suffix = &array_suffixes[strlen(array_suffixes)];
>   	/* BTF_MAX_ITER pointer suffixes "*" */
> -	const char *ptr_suffixes = "**********";
> +	static const char ptr_suffixes[] = "**********";
>   	const char *ptr_suffix = &ptr_suffixes[strlen(ptr_suffixes)];
>   	const char *name = NULL, *prefix = "", *parens = "";
>   	const struct btf_member *m = show->state.member;
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index d175b70067b3..78a090fcbc72 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -7346,7 +7346,7 @@ static int sanitize_err(struct bpf_verifier_env *env,
>   			const struct bpf_reg_state *off_reg,
>   			const struct bpf_reg_state *dst_reg)
>   {
> -	static const char *err = "pointer arithmetic with it prohibited for !root";
> +	static const char err[] = "pointer arithmetic with it prohibited for !root";
>   	const char *op = BPF_OP(insn->code) == BPF_ADD ? "add" : "sub";
>   	u32 dst = insn->dst_reg, src = insn->src_reg;
>   

Re: [PATCH 1/2] kernel/bpf: change "char *" string form to "char []"

[liqiong]

在 2022年05月12日 23:16, Yonghong Song 写道:
>
>
> On 5/12/22 7:28 AM, liqiong wrote:
>> The string form of "char []" declares a single variable. It is better
>> than "char *" which creates two variables.
>
> Could you explain in details about why it is better in generated codes?
> It is not clear to me why your patch is better than the original code.

Hi there，

The  string form of "char *" creates two variables in the final assembly output,
a static string, and a char pointer to the static string.  Use  "objdump -S -D  *.o",
can find out the static string  occurring  at "Contents of section .rodata".

>
>>
>> Signed-off-by: liqiong <liqiong@nfschina.com>
>> ---
>>   kernel/bpf/btf.c      | 4 ++--
>>   kernel/bpf/verifier.c | 2 +-
>>   2 files changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
>> index 0918a39279f6..218a8ac73644 100644
>> --- a/kernel/bpf/btf.c
>> +++ b/kernel/bpf/btf.c
>> @@ -894,10 +894,10 @@ static const struct btf_type *btf_type_skip_qualifiers(const struct btf *btf,
>>   static const char *btf_show_name(struct btf_show *show)
>>   {
>>       /* BTF_MAX_ITER array suffixes "[]" */
>> -    const char *array_suffixes = "[][][][][][][][][][]";
>> +    static const char array_suffixes[] = "[][][][][][][][][][]";
>>       const char *array_suffix = &array_suffixes[strlen(array_suffixes)];
>>       /* BTF_MAX_ITER pointer suffixes "*" */
>> -    const char *ptr_suffixes = "**********";
>> +    static const char ptr_suffixes[] = "**********";
>>       const char *ptr_suffix = &ptr_suffixes[strlen(ptr_suffixes)];
>>       const char *name = NULL, *prefix = "", *parens = "";
>>       const struct btf_member *m = show->state.member;
>> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
>> index d175b70067b3..78a090fcbc72 100644
>> --- a/kernel/bpf/verifier.c
>> +++ b/kernel/bpf/verifier.c
>> @@ -7346,7 +7346,7 @@ static int sanitize_err(struct bpf_verifier_env *env,
>>               const struct bpf_reg_state *off_reg,
>>               const struct bpf_reg_state *dst_reg)
>>   {
>> -    static const char *err = "pointer arithmetic with it prohibited for !root";
>> +    static const char err[] = "pointer arithmetic with it prohibited for !root";
>>       const char *op = BPF_OP(insn->code) == BPF_ADD ? "add" : "sub";
>>       u32 dst = insn->dst_reg, src = insn->src_reg;
>>   

-- 
李力琼 <13524287433>
上海市浦东新区海科路99号中科院上海高等研究院3号楼3楼

Re: [PATCH 1/2] kernel/bpf: change "char *" string form to "char []"

[Daniel Borkmann]

On 5/12/22 7:08 PM, liqiong wrote:
> 在 2022年05月12日 23:16, Yonghong Song 写道:
>>
>> On 5/12/22 7:28 AM, liqiong wrote:
>>> The string form of "char []" declares a single variable. It is better
>>> than "char *" which creates two variables.
>>
>> Could you explain in details about why it is better in generated codes?
>> It is not clear to me why your patch is better than the original code.
> 
> The  string form of "char *" creates two variables in the final assembly output,
> a static string, and a char pointer to the static string.  Use  "objdump -S -D  *.o",
> can find out the static string  occurring  at "Contents of section .rodata".

There are ~360 instances of this type in the tree from a quick grep, do you
plan to convert all them ?

Thanks,
Daniel

Re: [PATCH 1/2] kernel/bpf: change "char *" string form to "char []"

[liqiong]

在 2022年05月13日 04:59, Daniel Borkmann 写道:
> On 5/12/22 7:08 PM, liqiong wrote:
>> 在 2022年05月12日 23:16, Yonghong Song 写道:
>>>
>>> On 5/12/22 7:28 AM, liqiong wrote:
>>>> The string form of "char []" declares a single variable. It is better
>>>> than "char *" which creates two variables.
>>>
>>> Could you explain in details about why it is better in generated codes?
>>> It is not clear to me why your patch is better than the original code.
>>
>> The  string form of "char *" creates two variables in the final assembly output,
>> a static string, and a char pointer to the static string.  Use  "objdump -S -D  *.o",
>> can find out the static string  occurring  at "Contents of section .rodata".
>
> There are ~360 instances of this type in the tree from a quick grep, do you
> plan to convert all them ?

Hi daniel,

I have fixed all the string form in kernel tree, summited  two patches. 
Have searched the kernel tree by "grep  -nHre char.*\*.*=.*\"",  and checked all the "char *foo = "bar" "
string form,  only five  instances are needed to fix in bpf direcotry (2 files) and trace directory (2 files).
In most cases, need a char pointer anyway, just like this:

[const] char *foo = "bar";
if (xxx)
    foo = "blash";

In this situation, can't  change  "char *foo"  to  "char foo[]".

This work was published in "KernelJanitors/Todo".  So, I fixed.

Thanks.

>
> Thanks,
> Daniel

RE: [PATCH 1/2] kernel/bpf: change "char *" string form to "char []"

[David Laight]

From: Daniel Borkmann
> Sent: 12 May 2022 22:00
> 
> On 5/12/22 7:08 PM, liqiong wrote:
> > 在 2022年05月12日 23:16, Yonghong Song 写道:
> >>
> >> On 5/12/22 7:28 AM, liqiong wrote:
> >>> The string form of "char []" declares a single variable. It is better
> >>> than "char *" which creates two variables.
> >>
> >> Could you explain in details about why it is better in generated codes?
> >> It is not clear to me why your patch is better than the original code.
> >
> > The  string form of "char *" creates two variables in the final assembly output,
> > a static string, and a char pointer to the static string.  Use  "objdump -S -D  *.o",
> > can find out the static string  occurring  at "Contents of section .rodata".
> 
> There are ~360 instances of this type in the tree from a quick grep, do you
> plan to convert all them ?

There are also all the places with const char *names[] = ...;
where the actual names are all similar length so replacing with
const char names[][n] saves space.

Although that transformation has a bigger effect on shared libs.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)